| Message ID | 20240620175657.358273-1-piotr.wojtaszczyk@timesys.com |
|---|---|
| Headers | show |
| Series | Add audio support for LPC32XX CPUs | expand |
Hi Piotr, On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote: > When del_timer_sync() is called in an interrupt context it throws a warning > because of potential deadlock. Threaded irq handler fixes the potential > problem. > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> did you run into a lockdep splat? Anything against using del_timer(), instead? Have you tried? Thanks, Andi
> Adds properties declared in the new DT bindings:
Add? How do you think about to replace such an abbreviation?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n94
Regards,
Markus
> This allows to share the regmap with other simple-mfd devices like > nxp,lpc32xx-dmamux Please choose an imperative wording for an improved change description. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n94 Regards, Markus
… > this driver allows to route a signal request line thru the multiplexer for > given peripheral. Would you like to choose an imperative wording for an improved change description? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n94 Regards, Markus
> With the driver for nxp,lpc3220-dmamux we can remove the pl08x platform > data and let pl08x driver to create peripheral channels from the DT > properties. Do you see opportunities to improve such a change description? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n94 Regards, Markus
On 20/06/2024 19:56, Piotr Wojtaszczyk wrote: > Adds properties declared in the new DT bindings: > - nxp,lpc3220-i2s.yaml > - nxp,lpc3220-dmamux.yaml > for dma router/mux and I2S interface. > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> You are doing here multiple things at once. This should be One patch is dma properties. > > i2s0: i2s@20094000 { > compatible = "nxp,lpc3220-i2s"; > reg = <0x20094000 0x1000>; > + interrupts = <22 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk LPC32XX_CLK_I2S0>; > + dmas = <&dma 0 1>, <&dma 13 1>; > + dma-names = "rx", "tx"; > + #sound-dai-cells = <0>; Sound dai cells is another patch (or entire node is separate patch). > status = "disabled"; > }; > > @@ -231,12 +255,19 @@ sd: sd@20098000 { > <13 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&clk LPC32XX_CLK_SD>; > clock-names = "apb_pclk"; > + dmas = <&dma 4 1>; > + dma-names = "rx"; > status = "disabled"; > }; > > i2s1: i2s@2009c000 { > compatible = "nxp,lpc3220-i2s"; > reg = <0x2009c000 0x1000>; > + interrupts = <23 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk LPC32XX_CLK_I2S1>; > + dmas = <&dma 2 1>, <&dmamux 10 1 1>; > + dma-names = "rx", "tx"; > + #sound-dai-cells = <0>; > status = "disabled"; > }; > > @@ -312,21 +343,27 @@ fab { > compatible = "simple-bus"; > ranges = <0x20000000 0x20000000 0x30000000>; > > - /* System Control Block */ > - scb { > - compatible = "simple-bus"; > - ranges = <0x0 0x40004000 0x00001000>; > + syscon@40004000 { > + compatible = "nxp,lpc3220-creg", "syscon", "simple-mfd"; > + reg = <0x40004000 0x114>; This hunk is also separate patch. Best regards, Krzysztof
On 20/06/2024 19:56, Piotr Wojtaszczyk wrote: > > - base = of_iomap(np, 0); > - if (!base) { > - pr_err("failed to map system control block registers\n"); > - return; > - } > - > - clk_regmap = regmap_init_mmio(NULL, base, &lpc32xx_scb_regmap_config); > + clk_regmap = syscon_node_to_regmap(np->parent); > if (IS_ERR(clk_regmap)) { > pr_err("failed to regmap system control block: %ld\n", > PTR_ERR(clk_regmap)); > - iounmap(base); This looks backwards incompatible. You should keep the fallback way. Best regards, Krzysztof
On 20/06/2024 19:56, Piotr Wojtaszczyk wrote: > LPC32XX connects few of its peripherals to pl08x DMA thru a multiplexer, > this driver allows to route a signal request line thru the multiplexer for > given peripheral. > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > --- > Changes for v4: > - This patch is new in v4 > > MAINTAINERS | 1 + > drivers/dma/Kconfig | 9 ++ > drivers/dma/Makefile | 1 + > drivers/dma/lpc32xx-dmamux.c | 195 +++++++++++++++++++++++++++++++++++ > 4 files changed, 206 insertions(+) > create mode 100644 drivers/dma/lpc32xx-dmamux.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index fadf1baafd89..5ffe988ee282 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2403,6 +2403,7 @@ R: Vladimir Zapolskiy <vz@mleia.com> > L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) > S: Maintained > F: Documentation/devicetree/bindings/dma/nxp,lpc3220-dmamux.yaml > +F: drivers/dma/lpc32xx-dmamux.c > N: lpc32xx > > ARM/Marvell Dove/MV78xx0/Orion SOC support > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index 002a5ec80620..aeace3d7e066 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -378,6 +378,15 @@ config LPC18XX_DMAMUX > Enable support for DMA on NXP LPC18xx/43xx platforms > with PL080 and multiplexed DMA request lines. > > +config LPC32XX_DMAMUX > + bool "NXP LPC32xx DMA MUX for PL080" > + depends on ARCH_LPC32XX || COMPILE_TEST > + depends on OF && AMBA_PL08X > + select MFD_SYSCON > + help > + Support for PL080 multiplexed DMA request lines on > + LPC32XX platrofm. > + > config LS2X_APB_DMA > tristate "Loongson LS2X APB DMA support" > depends on LOONGARCH || COMPILE_TEST > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile > index 802ca916f05f..6f1350b62e7f 100644 > --- a/drivers/dma/Makefile > +++ b/drivers/dma/Makefile > @@ -50,6 +50,7 @@ obj-$(CONFIG_INTEL_IOATDMA) += ioat/ > obj-y += idxd/ > obj-$(CONFIG_K3_DMA) += k3dma.o > obj-$(CONFIG_LPC18XX_DMAMUX) += lpc18xx-dmamux.o > +obj-$(CONFIG_LPC32XX_DMAMUX) += lpc32xx-dmamux.o > obj-$(CONFIG_LS2X_APB_DMA) += ls2x-apb-dma.o > obj-$(CONFIG_MILBEAUT_HDMAC) += milbeaut-hdmac.o > obj-$(CONFIG_MILBEAUT_XDMAC) += milbeaut-xdmac.o > diff --git a/drivers/dma/lpc32xx-dmamux.c b/drivers/dma/lpc32xx-dmamux.c > new file mode 100644 > index 000000000000..4e6ce6026164 > --- /dev/null > +++ b/drivers/dma/lpc32xx-dmamux.c > @@ -0,0 +1,195 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Copyright 2024 Timesys Corporation <piotr.wojtaszczyk@timesys.com> > +// > +// Based on TI DMA Crossbar driver by: > +// Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com > +// Author: Peter Ujfalusi <peter.ujfalusi@ti.com> > + > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/mfd/syscon.h> > +#include <linux/of.h> > +#include <linux/of_dma.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/spinlock.h> > + > +#define LPC32XX_SSP_CLK_CTRL 0x78 > +#define LPC32XX_I2S_CLK_CTRL 0x7c > + > +struct lpc32xx_dmamux { > + int signal; > + char *name_sel0; > + char *name_sel1; > + int muxval; > + int muxreg; > + int bit; > + bool busy; > +}; > + > +/* From LPC32x0 User manual "3.2.1 DMA request signals" */ > +static struct lpc32xx_dmamux lpc32xx_muxes[] = { > + { > + .signal = 3, > + .name_sel0 = "spi2-rx-tx", > + .name_sel1 = "ssp1-rx", > + .muxreg = LPC32XX_SSP_CLK_CTRL, > + .bit = 5, > + }, > + { > + .signal = 10, > + .name_sel0 = "uart7-rx", > + .name_sel1 = "i2s1-dma1", > + .muxreg = LPC32XX_I2S_CLK_CTRL, > + .bit = 4, > + }, > + { > + .signal = 11, > + .name_sel0 = "spi1-rx-tx", > + .name_sel1 = "ssp1-tx", > + .muxreg = LPC32XX_SSP_CLK_CTRL, > + .bit = 4, > + }, > + { > + .signal = 14, > + .name_sel0 = "none", > + .name_sel1 = "ssp0-rx", > + .muxreg = LPC32XX_SSP_CLK_CTRL, > + .bit = 3, > + }, > + { > + .signal = 15, > + .name_sel0 = "none", > + .name_sel1 = "ssp0-tx", > + .muxreg = LPC32XX_SSP_CLK_CTRL, > + .bit = 2, > + }, > +}; > + > +struct lpc32xx_dmamux_data { > + struct dma_router dmarouter; > + struct regmap *reg; > + spinlock_t lock; /* protects busy status flag */ > +}; Please keep declarations of structures before definitions, so this should go before lpc32xx_muxes. > + ... > + dmamux->reg = syscon_node_to_regmap(np->parent); > + if (IS_ERR(dmamux->reg)) { > + dev_err(&pdev->dev, "syscon lookup failed\n"); > + return PTR_ERR(dmamux->reg); This should be rather: return dev_err_probe(). Best regards, Krzysztof
On 21/06/2024 07:56, Markus Elfring wrote: >> With the driver for nxp,lpc3220-dmamux we can remove the pl08x platform >> data and let pl08x driver to create peripheral channels from the DT >> properties. > > Do you see opportunities to improve such a change description? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n94 <form letter> Feel free to ignore all comments from Markus, regardless whether the suggestion is reasonable or not. This person is banned from LKML and several maintainers ignore Markus' feedback, because it is just a waste of time. </form letter> Best regards, Krzysztof
Hi Piotr, piotr.wojtaszczyk@timesys.com wrote on Thu, 20 Jun 2024 19:56:39 +0200: > Move away from pl08x platform data towards device tree. > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> I don't see any change regarding the NAND controller node in the device tree, is there any dependency with other patches from the same patchset or may I apply this directly to nand/next? Thanks, Miquèl
Hi Andi, On Fri, Jun 21, 2024 at 12:57 AM Andi Shyti <andi.shyti@kernel.org> wrote: > On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote: > > When del_timer_sync() is called in an interrupt context it throws a warning > > because of potential deadlock. Threaded irq handler fixes the potential > > problem. > > > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > > did you run into a lockdep splat? > > Anything against using del_timer(), instead? Have you tried? I didn't get a lockdep splat but console was flooded with warnings from https://github.com/torvalds/linux/blob/v6.10-rc4/kernel/time/timer.c#L1655 In the linux kernel v5.15 I didn't see these warnings. I'm not a maintainer of the driver and I didn't do any research on what kind of impact would have using del_timer() instad. Maybe Vladimir Zapolskiy will know that.
On Fri, Jun 21, 2024 at 10:30 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Piotr, > > piotr.wojtaszczyk@timesys.com wrote on Thu, 20 Jun 2024 19:56:39 +0200: > > > Move away from pl08x platform data towards device tree. > > > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > > I don't see any change regarding the NAND controller node in the device > tree, is there any dependency with other patches from the same patchset > or may I apply this directly to nand/next? > > Thanks, > Miquèl Yes, this patch depends on "[v4,04/10] ARM: dts: lpc32xx: Add missing dma and i2s properties" which will be splitted into two or more separate patches per request in the comments. I'd like to keep driver changes and corresponding changes in DTS in the same patch but I've made a separate patch for DTS per request from v2 of the patch set. -- Piotr Wojtaszczyk Timesys
Hi Piotr, Any reason why dmaengine parts cant be sent separately, why are they clubbed together, I dont see any obvious dependencies... On 20-06-24, 19:56, Piotr Wojtaszczyk wrote: > LPC32XX connects few of its peripherals to pl08x DMA thru a multiplexer, > this driver allows to route a signal request line thru the multiplexer for > given peripheral. What is the difference b/w this and lpc18xx driver, why not reuse that one? > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > --- > Changes for v4: > - This patch is new in v4 > > MAINTAINERS | 1 + > drivers/dma/Kconfig | 9 ++ > drivers/dma/Makefile | 1 + > drivers/dma/lpc32xx-dmamux.c | 195 +++++++++++++++++++++++++++++++++++ > 4 files changed, 206 insertions(+) > create mode 100644 drivers/dma/lpc32xx-dmamux.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index fadf1baafd89..5ffe988ee282 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2403,6 +2403,7 @@ R: Vladimir Zapolskiy <vz@mleia.com> > L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) > S: Maintained > F: Documentation/devicetree/bindings/dma/nxp,lpc3220-dmamux.yaml > +F: drivers/dma/lpc32xx-dmamux.c > N: lpc32xx > > ARM/Marvell Dove/MV78xx0/Orion SOC support > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index 002a5ec80620..aeace3d7e066 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -378,6 +378,15 @@ config LPC18XX_DMAMUX > Enable support for DMA on NXP LPC18xx/43xx platforms > with PL080 and multiplexed DMA request lines. > > +config LPC32XX_DMAMUX > + bool "NXP LPC32xx DMA MUX for PL080" > + depends on ARCH_LPC32XX || COMPILE_TEST > + depends on OF && AMBA_PL08X > + select MFD_SYSCON > + help > + Support for PL080 multiplexed DMA request lines on > + LPC32XX platrofm. > + > config LS2X_APB_DMA > tristate "Loongson LS2X APB DMA support" > depends on LOONGARCH || COMPILE_TEST > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile > index 802ca916f05f..6f1350b62e7f 100644 > --- a/drivers/dma/Makefile > +++ b/drivers/dma/Makefile > @@ -50,6 +50,7 @@ obj-$(CONFIG_INTEL_IOATDMA) += ioat/ > obj-y += idxd/ > obj-$(CONFIG_K3_DMA) += k3dma.o > obj-$(CONFIG_LPC18XX_DMAMUX) += lpc18xx-dmamux.o > +obj-$(CONFIG_LPC32XX_DMAMUX) += lpc32xx-dmamux.o > obj-$(CONFIG_LS2X_APB_DMA) += ls2x-apb-dma.o > obj-$(CONFIG_MILBEAUT_HDMAC) += milbeaut-hdmac.o > obj-$(CONFIG_MILBEAUT_XDMAC) += milbeaut-xdmac.o > diff --git a/drivers/dma/lpc32xx-dmamux.c b/drivers/dma/lpc32xx-dmamux.c > new file mode 100644 > index 000000000000..4e6ce6026164 > --- /dev/null > +++ b/drivers/dma/lpc32xx-dmamux.c > @@ -0,0 +1,195 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Copyright 2024 Timesys Corporation <piotr.wojtaszczyk@timesys.com> > +// > +// Based on TI DMA Crossbar driver by: > +// Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com > +// Author: Peter Ujfalusi <peter.ujfalusi@ti.com> > + > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/mfd/syscon.h> > +#include <linux/of.h> > +#include <linux/of_dma.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/spinlock.h> > + > +#define LPC32XX_SSP_CLK_CTRL 0x78 > +#define LPC32XX_I2S_CLK_CTRL 0x7c > + > +struct lpc32xx_dmamux { > + int signal; > + char *name_sel0; > + char *name_sel1; > + int muxval; > + int muxreg; > + int bit; > + bool busy; > +}; > + > +/* From LPC32x0 User manual "3.2.1 DMA request signals" */ > +static struct lpc32xx_dmamux lpc32xx_muxes[] = { > + { > + .signal = 3, > + .name_sel0 = "spi2-rx-tx", > + .name_sel1 = "ssp1-rx", > + .muxreg = LPC32XX_SSP_CLK_CTRL, > + .bit = 5, > + }, > + { > + .signal = 10, > + .name_sel0 = "uart7-rx", > + .name_sel1 = "i2s1-dma1", > + .muxreg = LPC32XX_I2S_CLK_CTRL, > + .bit = 4, > + }, > + { > + .signal = 11, > + .name_sel0 = "spi1-rx-tx", > + .name_sel1 = "ssp1-tx", > + .muxreg = LPC32XX_SSP_CLK_CTRL, > + .bit = 4, > + }, > + { > + .signal = 14, > + .name_sel0 = "none", > + .name_sel1 = "ssp0-rx", > + .muxreg = LPC32XX_SSP_CLK_CTRL, > + .bit = 3, > + }, > + { > + .signal = 15, > + .name_sel0 = "none", > + .name_sel1 = "ssp0-tx", > + .muxreg = LPC32XX_SSP_CLK_CTRL, > + .bit = 2, > + }, > +}; > + > +struct lpc32xx_dmamux_data { > + struct dma_router dmarouter; > + struct regmap *reg; > + spinlock_t lock; /* protects busy status flag */ > +}; > + > +static void lpc32xx_dmamux_release(struct device *dev, void *route_data) > +{ > + struct lpc32xx_dmamux_data *dmamux = dev_get_drvdata(dev); > + struct lpc32xx_dmamux *mux = route_data; > + unsigned long flags; > + > + dev_dbg(dev, "releasing dma request signal %d routed to %s\n", > + mux->signal, mux->muxval ? mux->name_sel1 : mux->name_sel1); > + > + guard(spinlock)(&dmamux->lock); > + > + mux->busy = false; > +} > + > +static void *lpc32xx_dmamux_reserve(struct of_phandle_args *dma_spec, > + struct of_dma *ofdma) > +{ > + struct platform_device *pdev = of_find_device_by_node(ofdma->of_node); > + struct device *dev = &pdev->dev; > + struct lpc32xx_dmamux_data *dmamux = platform_get_drvdata(pdev); > + unsigned long flags; > + struct lpc32xx_dmamux *mux = NULL; > + int i; > + > + if (dma_spec->args_count != 3) { > + dev_err(&pdev->dev, "invalid number of dma mux args\n"); > + return ERR_PTR(-EINVAL); > + } > + > + for (i = 0; i < ARRAY_SIZE(lpc32xx_muxes); i++) { > + if (lpc32xx_muxes[i].signal == dma_spec->args[0]) > + mux = &lpc32xx_muxes[i]; > + } > + if (!mux) { > + dev_err(&pdev->dev, "invalid mux request number: %d\n", > + dma_spec->args[0]); > + return ERR_PTR(-EINVAL); > + } > + > + if (dma_spec->args[2] > 1) { > + dev_err(&pdev->dev, "invalid dma mux value: %d\n", > + dma_spec->args[1]); > + return ERR_PTR(-EINVAL); > + } > + > + /* The of_node_put() will be done in the core for the node */ > + dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", 0); > + if (!dma_spec->np) { > + dev_err(&pdev->dev, "can't get dma master\n"); > + return ERR_PTR(-EINVAL); > + } > + > + spin_lock_irqsave(&dmamux->lock, flags); > + if (mux->busy) { > + spin_unlock_irqrestore(&dmamux->lock, flags); > + dev_err(dev, "dma request signal %d busy, routed to %s\n", > + mux->signal, mux->muxval ? mux->name_sel1 : mux->name_sel1); > + of_node_put(dma_spec->np); > + return ERR_PTR(-EBUSY); > + } > + > + mux->busy = true; > + mux->muxval = dma_spec->args[2] ? BIT(mux->bit) : 0; > + > + regmap_update_bits(dmamux->reg, mux->muxreg, BIT(mux->bit), mux->muxval); > + spin_unlock_irqrestore(&dmamux->lock, flags); > + > + dma_spec->args[2] = 0; > + dma_spec->args_count = 2; > + > + dev_dbg(dev, "dma request signal %d routed to %s\n", > + mux->signal, mux->muxval ? mux->name_sel1 : mux->name_sel1); > + > + return mux; > +} > + > +static int lpc32xx_dmamux_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct lpc32xx_dmamux_data *dmamux; > + int ret; > + > + dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL); > + if (!dmamux) > + return -ENOMEM; > + > + dmamux->reg = syscon_node_to_regmap(np->parent); > + if (IS_ERR(dmamux->reg)) { > + dev_err(&pdev->dev, "syscon lookup failed\n"); > + return PTR_ERR(dmamux->reg); > + } > + > + spin_lock_init(&dmamux->lock); > + platform_set_drvdata(pdev, dmamux); > + dmamux->dmarouter.dev = &pdev->dev; > + dmamux->dmarouter.route_free = lpc32xx_dmamux_release; > + > + return of_dma_router_register(np, lpc32xx_dmamux_reserve, > + &dmamux->dmarouter); > +} > + > +static const struct of_device_id lpc32xx_dmamux_match[] = { > + { .compatible = "nxp,lpc3220-dmamux" }, > + {}, > +}; > + > +static struct platform_driver lpc32xx_dmamux_driver = { > + .probe = lpc32xx_dmamux_probe, > + .driver = { > + .name = "lpc32xx-dmamux", > + .of_match_table = lpc32xx_dmamux_match, > + }, > +}; > + > +static int __init lpc32xx_dmamux_init(void) > +{ > + return platform_driver_register(&lpc32xx_dmamux_driver); > +} > +arch_initcall(lpc32xx_dmamux_init); > -- > 2.25.1
Hi Piotr, piotr.wojtaszczyk@timesys.com wrote on Fri, 21 Jun 2024 14:44:21 +0200: > On Fri, Jun 21, 2024 at 10:30 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > Hi Piotr, > > > > piotr.wojtaszczyk@timesys.com wrote on Thu, 20 Jun 2024 19:56:39 +0200: > > > > > Move away from pl08x platform data towards device tree. > > > > > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > > > > I don't see any change regarding the NAND controller node in the device > > tree, is there any dependency with other patches from the same patchset > > or may I apply this directly to nand/next? > > > > Thanks, > > Miquèl > > Yes, this patch depends on "[v4,04/10] ARM: dts: lpc32xx: Add missing > dma and i2s properties" > which will be splitted into two or more separate patches per request > in the comments. > I'd like to keep driver changes and corresponding changes in DTS in > the same patch > but I've made a separate patch for DTS per request from v2 of the patch set. These changes won't be applied to the same tries so they must be split. So I will not take this patch for the next merge window and instead will take it for the one after, if the DT patches have been applied. Please ping me at -rc1 again if the DT patches have gone through. Thanks, Miquèl
On Mon, Jun 24, 2024 at 7:39 AM Vinod Koul <vkoul@kernel.org> wrote: > Any reason why dmaengine parts cant be sent separately, why are they > clubbed together, I dont see any obvious dependencies... The I2S driver depends on the dmaengine parts > On 20-06-24, 19:56, Piotr Wojtaszczyk wrote: > > LPC32XX connects few of its peripherals to pl08x DMA thru a multiplexer, > > this driver allows to route a signal request line thru the multiplexer for > > given peripheral. > > What is the difference b/w this and lpc18xx driver, why not reuse that > one? The lpc18xx used the same dma peripheral (pl08x) but the request signal multiplexer around pl08x is completely different - there are no common parts.
Hi Piotr, On Fri, Jun 21, 2024 at 02:08:03PM GMT, Piotr Wojtaszczyk wrote: > On Fri, Jun 21, 2024 at 12:57 AM Andi Shyti <andi.shyti@kernel.org> wrote: > > On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote: > > > When del_timer_sync() is called in an interrupt context it throws a warning > > > because of potential deadlock. Threaded irq handler fixes the potential > > > problem. > > > > > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > > > > did you run into a lockdep splat? > > > > Anything against using del_timer(), instead? Have you tried? > > I didn't get a lockdep splat but console was flooded with warnings from > https://github.com/torvalds/linux/blob/v6.10-rc4/kernel/time/timer.c#L1655 > In the linux kernel v5.15 I didn't see these warnings. > > I'm not a maintainer of the driver and I didn't do any research on > what kind of impact > would have using del_timer() instad. Maybe Vladimir Zapolskiy will know that. Your patch is definitely correct, no doubt about that. And I don't have anything aginast changing irq handlers to threaded handlers. But I would be careful at doing that depending on the use of the controller and for accepting such change I would need an ack from someone who knows the device. Vladimir, perhaps? There are cases where using threaded handlers are not totally right, for example when the controller is used at early boot for power management handling. I don't think it's the case for this driver, but I can't be 100% sure. If you were able to see the flood of WARN_ON's, would be interesting to know how it behaves with del_timer(). Mind giving it a test? Thanks, Andi
On Tue, Jun 25, 2024 at 11:12 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > Hi Piotr, > > On Fri, Jun 21, 2024 at 02:08:03PM GMT, Piotr Wojtaszczyk wrote: > > On Fri, Jun 21, 2024 at 12:57 AM Andi Shyti <andi.shyti@kernel.org> wrote: > > > On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote: > > > > When del_timer_sync() is called in an interrupt context it throws a warning > > > > because of potential deadlock. Threaded irq handler fixes the potential > > > > problem. > > > > > > > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > > > > > > did you run into a lockdep splat? > > > > > > Anything against using del_timer(), instead? Have you tried? > > > > I didn't get a lockdep splat but console was flooded with warnings from > > https://github.com/torvalds/linux/blob/v6.10-rc4/kernel/time/timer.c#L1655 > > In the linux kernel v5.15 I didn't see these warnings. > > > > I'm not a maintainer of the driver and I didn't do any research on > > what kind of impact > > would have using del_timer() instad. Maybe Vladimir Zapolskiy will know that. > > Your patch is definitely correct, no doubt about that. > > And I don't have anything aginast changing irq handlers to > threaded handlers. But I would be careful at doing that depending > on the use of the controller and for accepting such change I > would need an ack from someone who knows the device. Vladimir, > perhaps? > > There are cases where using threaded handlers are not totally > right, for example when the controller is used at early boot for > power management handling. I don't think it's the case for this > driver, but I can't be 100% sure. > > If you were able to see the flood of WARN_ON's, would be > interesting to know how it behaves with del_timer(). Mind > giving it a test? > > Thanks, > Andi I took some time to take a closer look at this and it turns out that the timer is used only to exit from the wait_for_completion(), after each del_timer_sync() there is a complete() call. So I will remove the timer all together and replace wait_for_completion() with wait_for_completion_timeout()
On Mon, Jun 24, 2024 at 8:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > I don't see any change regarding the NAND controller node in the device > > > tree, is there any dependency with other patches from the same patchset > > > or may I apply this directly to nand/next? > > > > > > Thanks, > > > Miquèl > > > > Yes, this patch depends on "[v4,04/10] ARM: dts: lpc32xx: Add missing > > dma and i2s properties" > > which will be splitted into two or more separate patches per request > > in the comments. > > I'd like to keep driver changes and corresponding changes in DTS in > > the same patch > > but I've made a separate patch for DTS per request from v2 of the patch set. > > These changes won't be applied to the same tries so they must be split. > > So I will not take this patch for the next merge window and instead > will take it for the one after, if the DT patches have been applied. > Please ping me at -rc1 again if the DT patches have gone through. > > Thanks, > Miquèl Hi Miquèl, please check v5 of the patch, I've added fallback if a DMA can't be requested using DT, this is backward compatible with platform data and no longer depends on the DT changes. https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20240627150046.258795-11-piotr.wojtaszczyk@timesys.com/ -- Piotr Wojtaszczyk Timesys