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

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Wolfram,

On Wednesday 10 December 2014 15:23:15 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 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...
> > 
> > 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...

While I believe that requesting the channel at transfer time is the good 
solution, I think we should still try to move to module initcalls where 
possible. The risk of regressions is real so proper testing is needed. My 
question is, have you tried it ?

-- 
Regards,

Laurent Pinchart