Re: [PATCH] dmaengine: pl330: use subsys_initcall

From: Ray Jui
Date: Fri Oct 17 2014 - 12:56:50 EST


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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/