diff mbox

dmaengine: pl330: use subsys_initcall

Message ID 1413506896-4536-1-git-send-email-rjui@broadcom.com (mailing list archive)
State Rejected
Headers show

Commit Message

Ray Jui Oct. 17, 2014, 12:48 a.m. UTC
As part of subsystem that many slave drivers depend on, it's more
appropriate for the pl330 DMA driver to be initialized at
subsys_initcall than device_initcall

Signed-off-by: Ray Jui <rjui@broadcom.com>
---
 drivers/dma/pl330.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Vinod Koul Oct. 17, 2014, 7:35 a.m. UTC | #1
On Fri, Oct 17, 2014 at 09:45:45AM +0200, Lars-Peter Clausen wrote:
> On 10/17/2014 02:48 AM, Ray Jui wrote:
> >As part of subsystem that many slave drivers depend on, it's more
> >appropriate for the pl330 DMA driver to be initialized at
> >subsys_initcall than device_initcall
> 
> Well, we do have -EPROBE_DEFER these days to handle these kinds of
> dependencies so we no longer have to these kinds of manual init
> reordering tricks.
How ould that work?

Consider for example SPI and dmanegine. SPI driver got probed, then to start
a transaction requested a channel... while dmaengine driver is still getting
probed/not probed yet. So SPI driver didnt get a channel.
Lars-Peter Clausen Oct. 17, 2014, 7:45 a.m. UTC | #2
On 10/17/2014 02:48 AM, Ray Jui wrote:
> As part of subsystem that many slave drivers depend on, it's more
> appropriate for the pl330 DMA driver to be initialized at
> subsys_initcall than device_initcall

Well, we do have -EPROBE_DEFER these days to handle these kinds of 
dependencies so we no longer have to these kinds of manual init reordering 
tricks.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Oct. 17, 2014, 9:44 a.m. UTC | #3
On 17.10.2014 02:48, Ray Jui wrote:
> As part of subsystem that many slave drivers depend on, it's more
> appropriate for the pl330 DMA driver to be initialized at
> subsys_initcall than device_initcall
> 
> Signed-off-by: Ray Jui <rjui@broadcom.com>
> ---
>  drivers/dma/pl330.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

For our setup this was not needed but anyway works fine.
Tested on Trats2 (Exynos4412) and Gear2 (Exynos3250).

Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index d5149aa..abb4cae 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2811,7 +2811,17 @@ static struct amba_driver pl330_driver = {
>  	.remove = pl330_remove,
>  };
>  
> -module_amba_driver(pl330_driver);
> +static int __init pl330_init(void)
> +{
> +	return amba_driver_register(&pl330_driver);
> +}
> +subsys_initcall(pl330_init);
> +
> +static void __exit pl330_exit(void)
> +{
> +	amba_driver_unregister(&pl330_driver);
> +}
> +module_exit(pl330_exit);
>  
>  MODULE_AUTHOR("Jaswinder Singh <jassi.brar@samsung.com>");
>  MODULE_DESCRIPTION("API Driver for PL330 DMAC");
> 

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen Oct. 17, 2014, 11:15 a.m. UTC | #4
On 10/17/2014 09:35 AM, Vinod Koul wrote:
> On Fri, Oct 17, 2014 at 09:45:45AM +0200, Lars-Peter Clausen wrote:
>> On 10/17/2014 02:48 AM, Ray Jui wrote:
>>> As part of subsystem that many slave drivers depend on, it's more
>>> appropriate for the pl330 DMA driver to be initialized at
>>> subsys_initcall than device_initcall
>>
>> Well, we do have -EPROBE_DEFER these days to handle these kinds of
>> dependencies so we no longer have to these kinds of manual init
>> reordering tricks.
> How ould that work?
>
> Consider for example SPI and dmanegine. SPI driver got probed, then to start
> a transaction requested a channel... while dmaengine driver is still getting
> probed/not probed yet. So SPI driver didnt get a channel.
>

Ideally the SPI driver requests the channel in probe function and if the DMA 
controller is not yet probed returns EPROBE_DEFER. If the SPI driver 
requests the channel in the transfer handler it needs to deal with being 
able to fall back to non DMA transfers anyway so this shouldn't be a problem.

But in any case fiddling around with the init sequences is just a quick hack 
and might makes the problem less likely to appear in some cases, but there 
is no guarantee that it works. And I think the proper solution at the moment 
is to use probe deferral.

Other subsystems have seen patches which moved drivers from using 
subsys_initcall to device_initcall/module_..._driver/ with the reasoning 
that this is no longer necessary because of EPROBE_DEFER. So I don't think 
we should be doing the exact opposite in DMA framework. Also if we'd apply 
this patch it won't take to long until somebody suggest going back to 
module_platform_driver() instead of subsys_initcall.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ray Jui Oct. 17, 2014, 4:18 p.m. UTC | #5
On 10/17/2014 4:15 AM, Lars-Peter Clausen wrote:
> On 10/17/2014 09:35 AM, Vinod Koul wrote:
>> On Fri, Oct 17, 2014 at 09:45:45AM +0200, Lars-Peter Clausen wrote:
>>> On 10/17/2014 02:48 AM, Ray Jui wrote:
>>>> As part of subsystem that many slave drivers depend on, it's more
>>>> appropriate for the pl330 DMA driver to be initialized at
>>>> subsys_initcall than device_initcall
>>>
>>> Well, we do have -EPROBE_DEFER these days to handle these kinds of
>>> dependencies so we no longer have to these kinds of manual init
>>> reordering tricks.
>> How ould that work?
>>
>> Consider for example SPI and dmanegine. SPI driver got probed, then to
>> start
>> a transaction requested a channel... while dmaengine driver is still
>> getting
>> probed/not probed yet. So SPI driver didnt get a channel.
>>
>
> Ideally the SPI driver requests the channel in probe function and if the
> DMA controller is not yet probed returns EPROBE_DEFER. If the SPI driver
> requests the channel in the transfer handler it needs to deal with being
> able to fall back to non DMA transfers anyway so this shouldn't be a
> problem.
So in the case of the spi-pl022 driver. It requests the channel in probe 
function. And obviously DMA is not mandatory, so when the channel 
request fails the probe won't fail and instead it falls back to PIO. In 
this case, can you recommend a different way to solve this problem 
without having the DMA driver probed earlier than its slaves?

>
> But in any case fiddling around with the init sequences is just a quick
> hack and might makes the problem less likely to appear in some cases,
> but there is no guarantee that it works. And I think the proper solution
> at the moment is to use probe deferral.
I think it makes sense to have the DMA driver, as one of the core 
components in various SoCs that a lot of peripheral drivers depend on, 
to be registered at the level of subsys_init or somewhere close. We are 
not changing this just to get SPI to work. We are changing this because 
we think DMA should be ready before a lot of its slaves, which are 
typically done at device_initcall.

I have no problem relying on EPROBE_DEFER for this, provided that it 
works. The issue is, like I mentioned above, for a lot of slave devices 
DMA is not mandatory, when DMA fails at probe they would fall back to 
PIO and never use DMA. Another disadvantage I see with EPROBE_DEFER is 
delayed boot time.

>
> Other subsystems have seen patches which moved drivers from using
> subsys_initcall to device_initcall/module_..._driver/ with the reasoning
> that this is no longer necessary because of EPROBE_DEFER. So I don't
> think we should be doing the exact opposite in DMA framework. Also if
> we'd apply this patch it won't take to long until somebody suggest going
> back to module_platform_driver() instead of subsys_initcall.
>
> - Lars
There are currently 12 DMA drivers under drivers/dma registering 
themselves at subsys_init. I don't see why pl330 cannot do the same. Is 
there any concern that it may not work for some other SoCs when it's 
done at subsys_init? So far I cannot think of any. The only dependency 
of pl330 is the ARM apb_pclk, required during AMBA bus probe. But that's 
usually ready before subsys_init.

Thanks,

Ray
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen Oct. 17, 2014, 4:39 p.m. UTC | #6
On 10/17/2014 06:18 PM, Ray Jui wrote:
> On 10/17/2014 4:15 AM, Lars-Peter Clausen wrote:
>> On 10/17/2014 09:35 AM, Vinod Koul wrote:
>>> On Fri, Oct 17, 2014 at 09:45:45AM +0200, Lars-Peter Clausen wrote:
>>>> On 10/17/2014 02:48 AM, Ray Jui wrote:
>>>>> As part of subsystem that many slave drivers depend on, it's more
>>>>> appropriate for the pl330 DMA driver to be initialized at
>>>>> subsys_initcall than device_initcall
>>>>
>>>> Well, we do have -EPROBE_DEFER these days to handle these kinds of
>>>> dependencies so we no longer have to these kinds of manual init
>>>> reordering tricks.
>>> How ould that work?
>>>
>>> Consider for example SPI and dmanegine. SPI driver got probed, then to
>>> start
>>> a transaction requested a channel... while dmaengine driver is still
>>> getting
>>> probed/not probed yet. So SPI driver didnt get a channel.
>>>
>>
>> Ideally the SPI driver requests the channel in probe function and if the
>> DMA controller is not yet probed returns EPROBE_DEFER. If the SPI driver
>> requests the channel in the transfer handler it needs to deal with being
>> able to fall back to non DMA transfers anyway so this shouldn't be a
>> problem.
> So in the case of the spi-pl022 driver. It requests the channel in probe
> function. And obviously DMA is not mandatory, so when the channel request
> fails the probe won't fail and instead it falls back to PIO. In this case,
> can you recommend a different way to solve this problem without having the
> DMA driver probed earlier than its slaves?


dma_request_slave_channel() has the problem that we can't differentiate 
between no channel provided and channel provided but the dma driver hasn't 
probed yet. The function will return NULL in both cases. But Stephen Warren 
added dma_request_slave_channel_reason() a while ago to solve this problem. 
This function returns a ERR_PTR. If it returns ERR_PTR(-EPROBE_DEFER) it 
means that a channel has been provided but the DMA driver hasn't probed yet. 
In this case the SPI driver should return -EPROBE_DEFER to try again later. 
If the function returns a different error code that means that it was not 
possible to get the DMA channel and it should fall back to PIO.

>
>>
>> But in any case fiddling around with the init sequences is just a quick
>> hack and might makes the problem less likely to appear in some cases,
>> but there is no guarantee that it works. And I think the proper solution
>> at the moment is to use probe deferral.
> I think it makes sense to have the DMA driver, as one of the core components
> in various SoCs that a lot of peripheral drivers depend on, to be registered
> at the level of subsys_init or somewhere close. We are not changing this
> just to get SPI to work. We are changing this because we think DMA should be
> ready before a lot of its slaves, which are typically done at device_initcall.

But if the DMA driver for example depends on a clock driver do you put the 
clock driver at a even earlier init level? The problem with using init 
levels for solving this problem is that there is only a small amount of init 
levels available and representing the dependency chains is neither possible 
with it nor were init level ever intended for solving this. EPROBE_DEFER on 
the other hand is.

>
> I have no problem relying on EPROBE_DEFER for this, provided that it works.
> The issue is, like I mentioned above, for a lot of slave devices DMA is not
> mandatory, when DMA fails at probe they would fall back to PIO and never use
> DMA. Another disadvantage I see with EPROBE_DEFER is delayed boot time.
>

Yea, the EPROBE_DEFER implementation is not ideal, but that is a problem 
that should be solved rather than working around it. I think there are 
patches somewhere for example that build a device dependency graph from the 
phandles in the devicetree and than probe devices in the correct order to 
reduce the number of times probe deferral is necessary.

>>
>> Other subsystems have seen patches which moved drivers from using
>> subsys_initcall to device_initcall/module_..._driver/ with the reasoning
>> that this is no longer necessary because of EPROBE_DEFER. So I don't
>> think we should be doing the exact opposite in DMA framework. Also if
>> we'd apply this patch it won't take to long until somebody suggest going
>> back to module_platform_driver() instead of subsys_initcall.
>>
>> - Lars
> There are currently 12 DMA drivers under drivers/dma registering themselves
> at subsys_init. I don't see why pl330 cannot do the same. Is there any
> concern that it may not work for some other SoCs when it's done at
> subsys_init? So far I cannot think of any. The only dependency of pl330 is
> the ARM apb_pclk, required during AMBA bus probe. But that's usually ready
> before subsys_init.

Those other drivers should be converted to device_initcall rather than 
converting the PL330 driver to subsys_init. Using subsys_init for device 
drivers is a hack which was used to try to solve ordering problems. But it 
doesn't work that great, especially if you have more than two devices in 
your dependency chain. The solution that people have come up with to solve 
this problem in a better way is probe deferral by the means of -EPROBE_DEFER.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ray Jui Oct. 17, 2014, 4:56 p.m. UTC | #7
On 10/17/2014 9:39 AM, Lars-Peter Clausen wrote:
> On 10/17/2014 06:18 PM, Ray Jui wrote:
>> On 10/17/2014 4:15 AM, Lars-Peter Clausen wrote:
>>> On 10/17/2014 09:35 AM, Vinod Koul wrote:
>>>> On Fri, Oct 17, 2014 at 09:45:45AM +0200, Lars-Peter Clausen wrote:
>>>>> On 10/17/2014 02:48 AM, Ray Jui wrote:
>>>>>> As part of subsystem that many slave drivers depend on, it's more
>>>>>> appropriate for the pl330 DMA driver to be initialized at
>>>>>> subsys_initcall than device_initcall
>>>>>
>>>>> Well, we do have -EPROBE_DEFER these days to handle these kinds of
>>>>> dependencies so we no longer have to these kinds of manual init
>>>>> reordering tricks.
>>>> How ould that work?
>>>>
>>>> Consider for example SPI and dmanegine. SPI driver got probed, then to
>>>> start
>>>> a transaction requested a channel... while dmaengine driver is still
>>>> getting
>>>> probed/not probed yet. So SPI driver didnt get a channel.
>>>>
>>>
>>> Ideally the SPI driver requests the channel in probe function and if the
>>> DMA controller is not yet probed returns EPROBE_DEFER. If the SPI driver
>>> requests the channel in the transfer handler it needs to deal with being
>>> able to fall back to non DMA transfers anyway so this shouldn't be a
>>> problem.
>> So in the case of the spi-pl022 driver. It requests the channel in probe
>> function. And obviously DMA is not mandatory, so when the channel request
>> fails the probe won't fail and instead it falls back to PIO. In this
>> case,
>> can you recommend a different way to solve this problem without having
>> the
>> DMA driver probed earlier than its slaves?
>
>
> dma_request_slave_channel() has the problem that we can't differentiate
> between no channel provided and channel provided but the dma driver
> hasn't probed yet. The function will return NULL in both cases. But
> Stephen Warren added dma_request_slave_channel_reason() a while ago to
> solve this problem. This function returns a ERR_PTR. If it returns
> ERR_PTR(-EPROBE_DEFER) it means that a channel has been provided but the
> DMA driver hasn't probed yet. In this case the SPI driver should return
> -EPROBE_DEFER to try again later. If the function returns a different
> error code that means that it was not possible to get the DMA channel
> and it should fall back to PIO.
>
Thanks for the information. This will solve our problem.

>>
>>>
>>> But in any case fiddling around with the init sequences is just a quick
>>> hack and might makes the problem less likely to appear in some cases,
>>> but there is no guarantee that it works. And I think the proper solution
>>> at the moment is to use probe deferral.
>> I think it makes sense to have the DMA driver, as one of the core
>> components
>> in various SoCs that a lot of peripheral drivers depend on, to be
>> registered
>> at the level of subsys_init or somewhere close. We are not changing this
>> just to get SPI to work. We are changing this because we think DMA
>> should be
>> ready before a lot of its slaves, which are typically done at
>> device_initcall.
>
> But if the DMA driver for example depends on a clock driver do you put
> the clock driver at a even earlier init level? The problem with using
> init levels for solving this problem is that there is only a small
> amount of init levels available and representing the dependency chains
> is neither possible with it nor were init level ever intended for
> solving this. EPROBE_DEFER on the other hand is.
>
>>
>> I have no problem relying on EPROBE_DEFER for this, provided that it
>> works.
>> The issue is, like I mentioned above, for a lot of slave devices DMA
>> is not
>> mandatory, when DMA fails at probe they would fall back to PIO and
>> never use
>> DMA. Another disadvantage I see with EPROBE_DEFER is delayed boot time.
>>
>
> Yea, the EPROBE_DEFER implementation is not ideal, but that is a problem
> that should be solved rather than working around it. I think there are
> patches somewhere for example that build a device dependency graph from
> the phandles in the devicetree and than probe devices in the correct
> order to reduce the number of times probe deferral is necessary.
>
Agreed. Yes, it would be very nice if we can eventually describe the 
dependencies of various components in a system by utilizing the device 
tree. This way the dependencies can be customized for each individual SoC.

>>>
>>> Other subsystems have seen patches which moved drivers from using
>>> subsys_initcall to device_initcall/module_..._driver/ with the reasoning
>>> that this is no longer necessary because of EPROBE_DEFER. So I don't
>>> think we should be doing the exact opposite in DMA framework. Also if
>>> we'd apply this patch it won't take to long until somebody suggest going
>>> back to module_platform_driver() instead of subsys_initcall.
>>>
>>> - Lars
>> There are currently 12 DMA drivers under drivers/dma registering
>> themselves
>> at subsys_init. I don't see why pl330 cannot do the same. Is there any
>> concern that it may not work for some other SoCs when it's done at
>> subsys_init? So far I cannot think of any. The only dependency of
>> pl330 is
>> the ARM apb_pclk, required during AMBA bus probe. But that's usually
>> ready
>> before subsys_init.
>
> Those other drivers should be converted to device_initcall rather than
> converting the PL330 driver to subsys_init. Using subsys_init for device
> drivers is a hack which was used to try to solve ordering problems. But
> it doesn't work that great, especially if you have more than two devices
> in your dependency chain. The solution that people have come up with to
> solve this problem in a better way is probe deferral by the means of
> -EPROBE_DEFER.
>
> - Lars
>
Thanks, Lars, for providing these information. They are very useful!

Ray
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul Oct. 21, 2014, 10:43 a.m. UTC | #8
On Fri, Oct 17, 2014 at 01:15:55PM +0200, Lars-Peter Clausen wrote:
> On 10/17/2014 09:35 AM, Vinod Koul wrote:
> >On Fri, Oct 17, 2014 at 09:45:45AM +0200, Lars-Peter Clausen wrote:
> >>On 10/17/2014 02:48 AM, Ray Jui wrote:
> >>>As part of subsystem that many slave drivers depend on, it's more
> >>>appropriate for the pl330 DMA driver to be initialized at
> >>>subsys_initcall than device_initcall
> >>
> >>Well, we do have -EPROBE_DEFER these days to handle these kinds of
> >>dependencies so we no longer have to these kinds of manual init
> >>reordering tricks.
> >How ould that work?
> >
> >Consider for example SPI and dmanegine. SPI driver got probed, then to start
> >a transaction requested a channel... while dmaengine driver is still getting
> >probed/not probed yet. So SPI driver didnt get a channel.
> >
> 
> Ideally the SPI driver requests the channel in probe function and if
> the DMA controller is not yet probed returns EPROBE_DEFER. If the
> SPI driver requests the channel in the transfer handler it needs to
> deal with being able to fall back to non DMA transfers anyway so
> this shouldn't be a problem.
> 
> But in any case fiddling around with the init sequences is just a
> quick hack and might makes the problem less likely to appear in some
> cases, but there is no guarantee that it works. And I think the
> proper solution at the moment is to use probe deferral.
> 
> Other subsystems have seen patches which moved drivers from using
> subsys_initcall to device_initcall/module_..._driver/ with the
> reasoning that this is no longer necessary because of EPROBE_DEFER.
> So I don't think we should be doing the exact opposite in DMA
> framework. Also if we'd apply this patch it won't take to long until
> somebody suggest going back to module_platform_driver() instead of
> subsys_initcall.

Sure, I don't mind moving the driver to module_ if that solves the problem
with defer probe. I am still not able to wrap my head around on how will
deferral probe solve the problem, for example in above SPI case.

So when we request & channel is not found, it can be all that channels are
given out so nothing to give or controller of interest not available. How do
we distinguish between these and how does controller driver say "looks like
device might not be probed, let me try later"?
Vinod Koul Oct. 21, 2014, 10:45 a.m. UTC | #9
On Fri, Oct 17, 2014 at 06:39:41PM +0200, Lars-Peter Clausen wrote:
> On 10/17/2014 06:18 PM, Ray Jui wrote:
> >On 10/17/2014 4:15 AM, Lars-Peter Clausen wrote:
> >>On 10/17/2014 09:35 AM, Vinod Koul wrote:
> >>>On Fri, Oct 17, 2014 at 09:45:45AM +0200, Lars-Peter Clausen wrote:
> >>>>On 10/17/2014 02:48 AM, Ray Jui wrote:
> >>>>>As part of subsystem that many slave drivers depend on, it's more
> >>>>>appropriate for the pl330 DMA driver to be initialized at
> >>>>>subsys_initcall than device_initcall
> >>>>
> >>>>Well, we do have -EPROBE_DEFER these days to handle these kinds of
> >>>>dependencies so we no longer have to these kinds of manual init
> >>>>reordering tricks.
> >>>How ould that work?
> >>>
> >>>Consider for example SPI and dmanegine. SPI driver got probed, then to
> >>>start
> >>>a transaction requested a channel... while dmaengine driver is still
> >>>getting
> >>>probed/not probed yet. So SPI driver didnt get a channel.
> >>>
> >>
> >>Ideally the SPI driver requests the channel in probe function and if the
> >>DMA controller is not yet probed returns EPROBE_DEFER. If the SPI driver
> >>requests the channel in the transfer handler it needs to deal with being
> >>able to fall back to non DMA transfers anyway so this shouldn't be a
> >>problem.
> >So in the case of the spi-pl022 driver. It requests the channel in probe
> >function. And obviously DMA is not mandatory, so when the channel request
> >fails the probe won't fail and instead it falls back to PIO. In this case,
> >can you recommend a different way to solve this problem without having the
> >DMA driver probed earlier than its slaves?
> 
> 
> dma_request_slave_channel() has the problem that we can't
> differentiate between no channel provided and channel provided but
> the dma driver hasn't probed yet. The function will return NULL in
> both cases. But Stephen Warren added
> dma_request_slave_channel_reason() a while ago to solve this
> problem. This function returns a ERR_PTR. If it returns
> ERR_PTR(-EPROBE_DEFER) it means that a channel has been provided but
> the DMA driver hasn't probed yet. In this case the SPI driver should
> return -EPROBE_DEFER to try again later. If the function returns a
> different error code that means that it was not possible to get the
> DMA channel and it should fall back to PIO.
So when should the SPI here check, if dmaengine is available. The client
doesn't grab channel in its probe, so the client cannot return
-EPROBE_DEFER.
Ray Jui Oct. 21, 2014, 4:17 p.m. UTC | #10
On 10/21/2014 3:45 AM, Vinod Koul wrote:
> On Fri, Oct 17, 2014 at 06:39:41PM +0200, Lars-Peter Clausen wrote:
>> On 10/17/2014 06:18 PM, Ray Jui wrote:
>>> On 10/17/2014 4:15 AM, Lars-Peter Clausen wrote:
>>>> On 10/17/2014 09:35 AM, Vinod Koul wrote:
>>>>> On Fri, Oct 17, 2014 at 09:45:45AM +0200, Lars-Peter Clausen wrote:
>>>>>> On 10/17/2014 02:48 AM, Ray Jui wrote:
>>>>>>> As part of subsystem that many slave drivers depend on, it's more
>>>>>>> appropriate for the pl330 DMA driver to be initialized at
>>>>>>> subsys_initcall than device_initcall
>>>>>>
>>>>>> Well, we do have -EPROBE_DEFER these days to handle these kinds of
>>>>>> dependencies so we no longer have to these kinds of manual init
>>>>>> reordering tricks.
>>>>> How ould that work?
>>>>>
>>>>> Consider for example SPI and dmanegine. SPI driver got probed, then to
>>>>> start
>>>>> a transaction requested a channel... while dmaengine driver is still
>>>>> getting
>>>>> probed/not probed yet. So SPI driver didnt get a channel.
>>>>>
>>>>
>>>> Ideally the SPI driver requests the channel in probe function and if the
>>>> DMA controller is not yet probed returns EPROBE_DEFER. If the SPI driver
>>>> requests the channel in the transfer handler it needs to deal with being
>>>> able to fall back to non DMA transfers anyway so this shouldn't be a
>>>> problem.
>>> So in the case of the spi-pl022 driver. It requests the channel in probe
>>> function. And obviously DMA is not mandatory, so when the channel request
>>> fails the probe won't fail and instead it falls back to PIO. In this case,
>>> can you recommend a different way to solve this problem without having the
>>> DMA driver probed earlier than its slaves?
>>
>>
>> dma_request_slave_channel() has the problem that we can't
>> differentiate between no channel provided and channel provided but
>> the dma driver hasn't probed yet. The function will return NULL in
>> both cases. But Stephen Warren added
>> dma_request_slave_channel_reason() a while ago to solve this
>> problem. This function returns a ERR_PTR. If it returns
>> ERR_PTR(-EPROBE_DEFER) it means that a channel has been provided but
>> the DMA driver hasn't probed yet. In this case the SPI driver should
>> return -EPROBE_DEFER to try again later. If the function returns a
>> different error code that means that it was not possible to get the
>> DMA channel and it should fall back to PIO.
> So when should the SPI here check, if dmaengine is available. The client
> doesn't grab channel in its probe, so the client cannot return
> -EPROBE_DEFER.
>
Currently the spi-pl022 driver requests its DMA channel in 
pl022_dma_autoprobe, called during driver probe. Lars suggested changing 
the dma_request_slave_channel call to dma_request_slave_channel_reason. 
 From my understanding, dma_request_slave_channel_reason should return 
-EPROBE_DEFER if the DMA controller (pl330) device node and its channels 
are declared in device tree but the driver hasn't been probed. The SPI 
driver can then return -EPROBE_DEFER, which will cause its probe to be 
done again, at a later time. By then the pl330 driver will be ready.

This will solve our problem, because we don't care when SPI is probed, 
but we want to use DMA instead of PIO. But for the original problem that 
Linus tried to solve by moving spi probe to subsys_initcall in 25c8e03b, 
if one wants spi probe to be done that early, it will not be able to use 
DMA with the currently proposed change.
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index d5149aa..abb4cae 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2811,7 +2811,17 @@  static struct amba_driver pl330_driver = {
 	.remove = pl330_remove,
 };
 
-module_amba_driver(pl330_driver);
+static int __init pl330_init(void)
+{
+	return amba_driver_register(&pl330_driver);
+}
+subsys_initcall(pl330_init);
+
+static void __exit pl330_exit(void)
+{
+	amba_driver_unregister(&pl330_driver);
+}
+module_exit(pl330_exit);
 
 MODULE_AUTHOR("Jaswinder Singh <jassi.brar@samsung.com>");
 MODULE_DESCRIPTION("API Driver for PL330 DMAC");