| Message ID | 1485340088-25481-1-git-send-email-m.szyprowski@samsung.com |
|---|---|
| Headers | show |
| Series | DMA Engine: switch PL330 driver to non-irq-safe runtime PM | expand |
On 01/25/2017 11:28 AM, Marek Szyprowski wrote: > Add pointer to slave device to of_dma_xlate to let DMA engine driver > to know which slave device is using given DMA channel. This will be > later used to implement non-irq-safe runtime PM for DMA engine driver. of_dma_xlate() is used to translate from a OF phandle and a specifier to a DMA channel. On one hand this does not necessarily mean that the channel is actually going to be used by the slave that called the xlate function. Modifying the driver state when a lookup of the channel is done is a layering violation. And this approach is also missing a way to disassociate a slave from a DMA channel, e.g. when it is no longer used. On the other hand there are other mechanisms to translate between some kind of firmware handle to a DMA channel which are completely ignored here. So this approach does not work. This is something that needs to be done at the dmaengine level, not a the firmware resource translation level. And it needs a matching method that is called when the channel is disassociated from a device, when the device no longer uses the DMA channel. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vinod, On 2017-02-09 05:11, Vinod Koul wrote: > On Thu, Jan 26, 2017 at 03:43:05PM +0100, Marek Szyprowski wrote: >> On 2017-01-25 14:12, Lars-Peter Clausen wrote: >>> On 01/25/2017 11:28 AM, Marek Szyprowski wrote: >>>> Add pointer to slave device to of_dma_xlate to let DMA engine driver >>>> to know which slave device is using given DMA channel. This will be >>>> later used to implement non-irq-safe runtime PM for DMA engine driver. >>> of_dma_xlate() is used to translate from a OF phandle and a specifier to a >>> DMA channel. On one hand this does not necessarily mean that the channel is >>> actually going to be used by the slave that called the xlate function. >>> Modifying the driver state when a lookup of the channel is done is a >>> layering violation. And this approach is also missing a way to disassociate >>> a slave from a DMA channel, e.g. when it is no longer used. >>> >>> On the other hand there are other mechanisms to translate between some kind >>> of firmware handle to a DMA channel which are completely ignored here. >>> >>> So this approach does not work. This is something that needs to be done at >>> the dmaengine level, not a the firmware resource translation level. And it >>> needs a matching method that is called when the channel is disassociated >> >from a device, when the device no longer uses the DMA channel. >> >> Frankly I agree that of_dma_xlate() should only return the requested channel >> to the dmaengine core and do not do any modification in the the >> driver state. > True.. > >> However the current dma engine design and implementation breaks this rule. >> Please check the drivers - how do they implement of_xlate callback. They >> usually call dma_get_any_slave_channel, dma_get_slave_channel or >> __dma_request_channel there, which in turn calls dma_chan_get, which then >> calls back to device_alloc_chan_resources callback. Some of the drivers also >> do a hardware configuration or other resource allocation in of_xlate. >> This is a bit messy design and leave no place in the core to set >> slave device >> before device_alloc_chan_resources callback, where one would expect to have >> it already set. > We shouldn't be doing much at this stage. We operate on a channel, so the > channel is returned to the client. We need to do these HW configurations > when the channel has to be prepred for a txn. IMHO, any HW configurations should be done in alloc_chan_resources callback and it would be best if a pointer of slave device will come as a parameter to it. >> The best place to add new calls to the dmaengine drivers to set slave device >> would be just before device_alloc_chan_resources(), what in turn means that >> the current dmaengine core should do in dma_chan_get(). This would >> require to >> forward the slave device pointer via even more layers including the of_xlate >> callback too. IMHO this is not worth the effort. >> >> DMA engine core and API definitely needs some cleanup. During such cleanup >> the slave device pointer might be moved out of xlate into separate callback >> when the core gets ready for such operation. > Yes agreed on that, plus the runtime handling needs to be built in, right > now the APIs dont work well with it, we disucssed these during the KS and > this goes without saying, patches are welcome :) Okay, so what is the conclusion? Do you want me to do the whole rework of dma engine core to get this runtime pm patchset for pl330 merged??? Is there any roadmap for this rework prepared, so I can at least take a look at the amount of work needed to be done? I'm rather new to dma engine framework and I only wanted to fix pl330 driver not to block turning off the power domain on Exynos5422/5433 SoCs. I can also check again if there is any other way to find the slave device in alloc_chan_resources, like for example scanning the device tree for phandles, to avoid changing dmaengine core as this turned out to be too problematic before one will do the proper dma engine core rework. > ... Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >> Yes agreed on that, plus the runtime handling needs to be built in, right >> now the APIs dont work well with it, we disucssed these during the KS and >> this goes without saying, patches are welcome :) > > > Okay, so what is the conclusion? Do you want me to do the whole rework of > dma > engine core to get this runtime pm patchset for pl330 merged??? Is there any > roadmap for this rework prepared, so I can at least take a look at the > amount > of work needed to be done? > > I'm rather new to dma engine framework and I only wanted to fix pl330 driver > not to block turning off the power domain on Exynos5422/5433 SoCs. As you probably know, this is a common problem for many dma devices, slave devices and platforms. If possible, it would be great if we could avoid a solution that doesn't force changes to lots of "dma consumer" drivers. > > I can also check again if there is any other way to find the slave device in > alloc_chan_resources, like for example scanning the device tree for > phandles, > to avoid changing dmaengine core as this turned out to be too problematic > before one will do the proper dma engine core rework. > Again, thanks for working on this! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, This patchset changes the way the runtime PM is implemented in the PL330 DMA engine driver. The main goal of such change is to add support for the audio power domain to Exynos5 SoCs (5250, 542x, 5433, probably others) and let it to be properly turned off, when no audio is being used. Switching to non-irq-safe runtime PM is required to properly let power domain to be turned off (irq-safe runtime PM keeps power domain turned on all the time) and to integrate with clock controller's runtime PM (this cannot be workarounded any other way, PL330 uses clocks from the controller, which belongs to the same power domain). For more details of the proposed change to the PL330 driver see patch #4. Audio power domain on Exynos5 SoCs contains following hardware modules: 1. clock controller 2. pin controller 3. PL330 DMA controller 4. I2S audio controller Patches for adding or fixing runtime PM for each of the above devices is handled separately. Runtime PM patches for clock controllers is possible and has been proposed in the following thread (pending review): "[PATCH v4 0/4] Add runtime PM support for clocks (on Exynos SoC example)", http://www.spinics.net/lists/arm-kernel/msg550747.html Runtime PM support for Exynos pin controller has been posted in the following thread: "[PATCH 0/9] Runtime PM for Exynos pin controller driver", http://www.spinics.net/lists/arm-kernel/msg550161.html Exynos I2S driver supports runtime PM, but some fixes were needed for it and they are already queued to linux-next. This patchset is based on linux-next from 25th January 2017 with "dmaengine: pl330: fix double lock" patch applied. Best regards Marek Szyprowski Samsung R&D Institute Poland Changelog: v7: - added missing of_dma_request_slave_channel API change to sound/soc/sh/rcar driver - extended commit message with information about drawbacks of irq-safe runtime pm - added Ulf's reviewed-by tags v6: https://www.spinics.net/lists/arm-kernel/msg557377.html - fixed pl330 system sleep suspend/resume callbacks, previous implementation incorrectly tried to unprepare clocks unconditionally - after a fix pl330 suspend/resume callbacks can be simply replaced by generic pm_runtime_force_{suspend,resume} helpers, what simplifies code even more v5: https://www.spinics.net/lists/arm-kernel/msg555001.html - added Acks - additional mutex is indeed not needed, rely on dma_list_mutex in dmaengine core, added comment about locking v4: http://www.spinics.net/lists/dmaengine/msg12329.html - rebased onto "dmaengine: pl330: fix double lock" patch: http://www.spinics.net/lists/dmaengine/msg12289.html - added a mutex to protect runtime PM links creation/removal to avoid races - moved mem2mem channel case handing to pl330_{add,del}_slave_pm_link functions to simplify code and error paths v3: http://www.spinics.net/lists/dmaengine/msg12245.html - removed pl330_filter function as suggested by Arnd Bergmann - removed pl330.h from arch/arm/plat-samsung/devs.c - fixes some minor style issues pointed by Krzysztof Kozlowski v2: https://www.spinics.net/lists/arm-kernel/msg552772.html - rebased onto linux next-20170109 - improved patch description - separated patch #3 from #4 (storing a pointer to slave device for each DMA channel) as requested by Krzysztof Kozlowski v1: https://www.spinics.net/lists/arm-kernel/msg550008.html - initial version Patch summary: Marek Szyprowski (4): dmaengine: pl330: remove pdata based initialization dmaengine: Forward slave device pointer to of_xlate callback dmaengine: pl330: Store pointer to slave device dmaengine: pl330: Don't require irq-safe runtime PM arch/arm/plat-samsung/devs.c | 1 - drivers/dma/amba-pl08x.c | 2 +- drivers/dma/at_hdmac.c | 4 +- drivers/dma/at_xdmac.c | 2 +- drivers/dma/bcm2835-dma.c | 2 +- drivers/dma/coh901318.c | 2 +- drivers/dma/cppi41.c | 2 +- drivers/dma/dma-jz4780.c | 2 +- drivers/dma/dmaengine.c | 2 +- drivers/dma/dw/platform.c | 2 +- drivers/dma/edma.c | 4 +- drivers/dma/fsl-edma.c | 2 +- drivers/dma/img-mdc-dma.c | 2 +- drivers/dma/imx-dma.c | 2 +- drivers/dma/imx-sdma.c | 2 +- drivers/dma/k3dma.c | 2 +- drivers/dma/lpc18xx-dmamux.c | 2 +- drivers/dma/mmp_pdma.c | 2 +- drivers/dma/mmp_tdma.c | 2 +- drivers/dma/moxart-dma.c | 2 +- drivers/dma/mxs-dma.c | 2 +- drivers/dma/nbpfaxi.c | 2 +- drivers/dma/of-dma.c | 19 ++-- drivers/dma/pl330.c | 220 ++++++++++++++++------------------------ drivers/dma/pxa_dma.c | 2 +- drivers/dma/qcom/bam_dma.c | 2 +- drivers/dma/sh/rcar-dmac.c | 2 +- drivers/dma/sh/shdma-of.c | 2 +- drivers/dma/sh/usb-dmac.c | 2 +- drivers/dma/sirf-dma.c | 2 +- drivers/dma/st_fdma.c | 2 +- drivers/dma/ste_dma40.c | 2 +- drivers/dma/stm32-dma.c | 2 +- drivers/dma/sun4i-dma.c | 2 +- drivers/dma/sun6i-dma.c | 2 +- drivers/dma/tegra20-apb-dma.c | 2 +- drivers/dma/tegra210-adma.c | 2 +- drivers/dma/xilinx/xilinx_dma.c | 2 +- drivers/dma/xilinx/zynqmp_dma.c | 2 +- drivers/dma/zx_dma.c | 2 +- include/linux/amba/pl330.h | 35 ------- include/linux/of_dma.h | 19 ++-- sound/soc/sh/rcar/dma.c | 5 +- sound/soc/sh/rcar/dvc.c | 3 +- sound/soc/sh/rcar/rsnd.h | 3 +- sound/soc/sh/rcar/src.c | 3 +- sound/soc/sh/rcar/ssi.c | 3 +- 47 files changed, 158 insertions(+), 231 deletions(-) delete mode 100644 include/linux/amba/pl330.h -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html