Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4133 bytes --]

On Wed, Dec 10, 2014 at 03:23:15PM +0100, Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 04:19:36PM +0200, Laurent Pinchart wrote:
> > (CC'ing the dmaengine mailing list)
> 
> Thanks!
> 
> > On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> > > On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > > > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > > >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> > > >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> > > >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> > > >>>> platform_device *dev)
> > > >>>>         if (ret)
> > > >>>>                 return ret;
> > > >>>> 
> > > >>>> +       /* Init DMA */
> > > >>>> +       sg_init_table(&pd->sg, 1);
> > > >>>> +       pd->dma_direction = DMA_NONE;
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_rx);
> > > >>>> +       if (ret == -EPROBE_DEFER)
> > > >>>> +               return ret;
> > > >>>> +
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_tx);
> > > >>>> +       if (ret == -EPROBE_DEFER) {
> > > >>>> +               sh_mobile_i2c_release_dma(pd);
> > > >>>> +               return ret;
> > > >>>> +       }
> > > >>>> +
> > > >>> 
> > > >>> If the DTS contains "dma" and "dma-names" properties, but
> > > >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan() returns
> > > >>> -EPROBE_DEFER, and the driver fails to initialize.
> > > >>> 
> > > >>> If I remove the "dma" and "dma-names" properties, the driver does fall
> > > >>> back to PIO mode.
> > > >>> 
> > > >>> I think this is a regression.
> > > >> 
> > > >> The only solution I can think of is to not bail out here and retry again
> > > >> before every transfer? Doesn't sound elegant, though...
> > > > 
> > > > I think we have to request for each and every transfer. And fall back
> > > > to PIO as default in a transparent way. This because the number of DMA
> > > > channels are limited compared to number of potential consumers, so
> > > > request failure may happen at any time.
> > > 
> > > AFAIR this scenario happens when submitting the transfer. The check
> > > for this is already in place. Requesting the channel is a different
> > > matter. Still, I'll cook up a patch and we will see what it looks
> > > like...
I think this is a limitation of driver may not be for HW. The right model for
dma_chan is to be viewed as SW channels and not the ones of HW (yes that is
how most of the drivers use that, but we can improve upon)

If we rework the driver to view dma_chan as SW channels, then you can accept
multiple channel requests and accept based on if we are able link the channel to
that peripheral or not. Here one txn maybe for peripheral A, subsequent one
for peripheral B. The driver needs to ensure the proper mux registers etc are
programmed while issuing next descriptor.

-- 
~Vinod

> > 
> > We could fix part of the issue by using virtual dma channels. In that case 
> > channel requests wouldn't fail anymore due to resource starvation with a large 
> > number of consumers. However, the request could still fail with -EPROBE_DEFER. 
> > For a driver that wants to fall back to PIO when DMA is unavailable I 
> > currently don't see another way than moving the channel request at the time of 
> > the transfer.
> 
> Note that the I2C drives uses subsys_initcall() for historic reasons,
> while the DMA driver uses module_init(). This is hard to revert without
> introducing potential regressions on older boards. So, the I2C DMA
> support needs to handle deferred probe definately. I am with Laurent, I
> don't see any other way, but I'd be glad to be enlightened...
> 



-- 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]