| Message ID | 20240710093356.3344056-1-p.puschmann@pironex.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: Add uart dma names to the SoC dtsi for RK356x | expand |
On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: > DMA names are required by of_dma_request_slave_channel function that is > called during uart probe. So to enable DMA for uarts add the names as in > the RK3568 TRM. Setting it on channels without flow control apparently causes issues. See https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@cknow.org/ > Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> > --- > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index d8543b5557ee..4ae40661ca6a > 100644 > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { > clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; > clock-names = "baudclk", "apb_pclk"; > dmas = <&dmac0 0>, <&dmac0 1>; > + dma-names = "tx", "rx"; > pinctrl-0 = <&uart0_xfer>; > pinctrl-names = "default"; > reg-io-width = <4>; > @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { > clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; > clock-names = "baudclk", "apb_pclk"; > dmas = <&dmac0 2>, <&dmac0 3>; > + dma-names = "tx", "rx"; > pinctrl-0 = <&uart1m0_xfer>; > pinctrl-names = "default"; > reg-io-width = <4>; > ...
Hi Diederik, Am 10.07.24 um 12:02 schrieb Diederik de Haas: > On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: >> DMA names are required by of_dma_request_slave_channel function that is >> called during uart probe. So to enable DMA for uarts add the names as in >> the RK3568 TRM. > > Setting it on channels without flow control apparently causes issues. See > > https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@cknow.org/ Ah is see. The only problem that i have is to enable/disable dmas by having or not having dma-names properties, where the latter case is followed by kernel error messages. That is very counterintuitive. Maybe a explicit boolean like dma-broken would be better. That could be set on dtsi level as default and deleted on board dts if wanted. With such a boolean we could also prevent the misleading "dma-names property of" error message and replace it with a hint that dma is disabled on purpose. Regards, Philipp Puschmann > >> Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> >> --- >> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index d8543b5557ee..4ae40661ca6a >> 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi >> @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { >> clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; >> clock-names = "baudclk", "apb_pclk"; >> dmas = <&dmac0 0>, <&dmac0 1>; >> + dma-names = "tx", "rx"; >> pinctrl-0 = <&uart0_xfer>; >> pinctrl-names = "default"; >> reg-io-width = <4>; >> @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { >> clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; >> clock-names = "baudclk", "apb_pclk"; >> dmas = <&dmac0 2>, <&dmac0 3>; >> + dma-names = "tx", "rx"; >> pinctrl-0 = <&uart1m0_xfer>; >> pinctrl-names = "default"; >> reg-io-width = <4>; >> ...
Hi Philipp, On Wednesday, 10 July 2024 12:20:20 CEST Philipp Puschmann wrote: > Am 10.07.24 um 12:02 schrieb Diederik de Haas: > > On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: > >> DMA names are required by of_dma_request_slave_channel function that is > >> called during uart probe. So to enable DMA for uarts add the names as in > >> the RK3568 TRM. > > > > Setting it on channels without flow control apparently causes issues. See > > > > https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@ > > cknow.org/ > Ah is see. The only problem that i have is to enable/disable dmas by having > or not having dma-names properties, where the latter case is followed by > kernel error messages. That is very counterintuitive. I forgot to link to my follow up patch where I added the property to some other Pine64 devices and added a cover letter inviting others to add it to other boards too if that seemed appropriate: https://lore.kernel.org/linux-rockchip/20240705163004.29678-2-didi.debian@cknow.org/ Maybe this applies to 'your' board too? > Maybe a explicit boolean like dma-broken would be better. That could be > set on dtsi level as default and deleted on board dts if wanted. That seems to invert the logic, which I believe was considered the 'wrong' solution: From https://lore.kernel.org/linux-rockchip/18284546.sWSEgdgrri@diego/ > > > Enabling dma globally can cause some interesting issues, > > > have you tested this fully? Maybe there is a better solution; possibly others will respond too. > With such a boolean we could also prevent the misleading > "dma-names property of" error message and > replace it with a hint that dma is disabled on purpose. Given that you're now at least the 4th person trying this, I guess a hint 'somewhere' would be beneficial. I do not know if the error message itself would be considered misleading and if something should be done about that. Cheers, Diederik > >> Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> > >> --- > >> > >> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > >> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index > >> d8543b5557ee..4ae40661ca6a > >> 100644 > >> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > >> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > >> @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { > >> > >> clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; > >> clock-names = "baudclk", "apb_pclk"; > >> dmas = <&dmac0 0>, <&dmac0 1>; > >> > >> + dma-names = "tx", "rx"; > >> > >> pinctrl-0 = <&uart0_xfer>; > >> pinctrl-names = "default"; > >> reg-io-width = <4>; > >> > >> @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { > >> > >> clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; > >> clock-names = "baudclk", "apb_pclk"; > >> dmas = <&dmac0 2>, <&dmac0 3>; > >> > >> + dma-names = "tx", "rx"; > >> > >> pinctrl-0 = <&uart1m0_xfer>; > >> pinctrl-names = "default"; > >> reg-io-width = <4>; > >> > >> ...
Am 10.07.24 um 12:53 schrieb Diederik de Haas: > Hi Philipp, > > On Wednesday, 10 July 2024 12:20:20 CEST Philipp Puschmann wrote: >> Am 10.07.24 um 12:02 schrieb Diederik de Haas: >>> On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: >>>> DMA names are required by of_dma_request_slave_channel function that is >>>> called during uart probe. So to enable DMA for uarts add the names as in >>>> the RK3568 TRM. >>> >>> Setting it on channels without flow control apparently causes issues. See >>> >>> https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@ >>> cknow.org/ >> Ah is see. The only problem that i have is to enable/disable dmas by having >> or not having dma-names properties, where the latter case is followed by >> kernel error messages. That is very counterintuitive. > > I forgot to link to my follow up patch where I added the property to > some other Pine64 devices and added a cover letter inviting others to > add it to other boards too if that seemed appropriate: > https://lore.kernel.org/linux-rockchip/20240705163004.29678-2-didi.debian@cknow.org/ > > Maybe this applies to 'your' board too? > >> Maybe a explicit boolean like dma-broken would be better. That could be >> set on dtsi level as default and deleted on board dts if wanted. > > That seems to invert the logic, which I believe was considered > the 'wrong' solution: > > From https://lore.kernel.org/linux-rockchip/18284546.sWSEgdgrri@diego/ >>>> Enabling dma globally can cause some interesting issues, >>>> have you tested this fully? > > Maybe there is a better solution; possibly others will respond too. > The reason for the suggested inverted logic was that a minimal fix would be to provide dma-names on SoC-level dtsi but to explicitly disable dma usage on SoC-level too, so that there wouldn't be a need to patch all the boards and keep the current SoC-level behaviour (that uart dma is disabled by default). >> With such a boolean we could also prevent the misleading >> "dma-names property of" error message and >> replace it with a hint that dma is disabled on purpose. > > Given that you're now at least the 4th person trying this, I guess > a hint 'somewhere' would be beneficial. > I do not know if the error message itself would be considered misleading > and if something should be done about that. The error message itself is okay in the case we actually want to use dma, but it's misleading if we disable dma on purpose through not providing dma-names. So i would prefer a explicit enable/disable switch and handle that one before looking for the dma-names. > > Cheers, > Diederik > >>>> Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> >>>> --- >>>> >>>> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index >>>> d8543b5557ee..4ae40661ca6a >>>> 100644 >>>> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>> @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { >>>> >>>> clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; >>>> clock-names = "baudclk", "apb_pclk"; >>>> dmas = <&dmac0 0>, <&dmac0 1>; >>>> >>>> + dma-names = "tx", "rx"; >>>> >>>> pinctrl-0 = <&uart0_xfer>; >>>> pinctrl-names = "default"; >>>> reg-io-width = <4>; >>>> >>>> @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { >>>> >>>> clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; >>>> clock-names = "baudclk", "apb_pclk"; >>>> dmas = <&dmac0 2>, <&dmac0 3>; >>>> >>>> + dma-names = "tx", "rx"; >>>> >>>> pinctrl-0 = <&uart1m0_xfer>; >>>> pinctrl-names = "default"; >>>> reg-io-width = <4>; >>>> >>>> ... >
Hello Philipp, On 2024-07-10 12:20, Philipp Puschmann wrote: > Am 10.07.24 um 12:02 schrieb Diederik de Haas: >> On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: >>> DMA names are required by of_dma_request_slave_channel function that >>> is >>> called during uart probe. So to enable DMA for uarts add the names as >>> in >>> the RK3568 TRM. >> >> Setting it on channels without flow control apparently causes issues. >> See >> >> https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@cknow.org/ > > Ah is see. The only problem that i have is to enable/disable dmas by > having or not having > dma-names properties, where the latter case is followed by kernel > error messages. That > is very counterintuitive. Maybe a explicit boolean like dma-broken > would be better. That > could be set on dtsi level as default and deleted on board dts if > wanted. With such > a boolean we could also prevent the misleading "dma-names property of" > error message > and replace it with a hint that dma is disabled on purpose. From what I've read in the prior discussions, this seems like a driver issue, so the driver should be fixed instead. >> >>> Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> >>> --- >>> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index >>> d8543b5557ee..4ae40661ca6a >>> 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>> @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { >>> clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; >>> clock-names = "baudclk", "apb_pclk"; >>> dmas = <&dmac0 0>, <&dmac0 1>; >>> + dma-names = "tx", "rx"; >>> pinctrl-0 = <&uart0_xfer>; >>> pinctrl-names = "default"; >>> reg-io-width = <4>; >>> @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { >>> clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; >>> clock-names = "baudclk", "apb_pclk"; >>> dmas = <&dmac0 2>, <&dmac0 3>; >>> + dma-names = "tx", "rx"; >>> pinctrl-0 = <&uart1m0_xfer>; >>> pinctrl-names = "default"; >>> reg-io-width = <4>; >>> ... > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi Dragan, Am 10.07.24 um 16:56 schrieb Dragan Simic: > Hello Philipp, > > On 2024-07-10 12:20, Philipp Puschmann wrote: >> Am 10.07.24 um 12:02 schrieb Diederik de Haas: >>> On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: >>>> DMA names are required by of_dma_request_slave_channel function that is >>>> called during uart probe. So to enable DMA for uarts add the names as in >>>> the RK3568 TRM. >>> >>> Setting it on channels without flow control apparently causes issues. See >>> >>> https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@cknow.org/ >> >> Ah is see. The only problem that i have is to enable/disable dmas by >> having or not having >> dma-names properties, where the latter case is followed by kernel >> error messages. That >> is very counterintuitive. Maybe a explicit boolean like dma-broken >> would be better. That >> could be set on dtsi level as default and deleted on board dts if >> wanted. With such >> a boolean we could also prevent the misleading "dma-names property of" >> error message >> and replace it with a hint that dma is disabled on purpose. > > From what I've read in the prior discussions, this seems like a driver > issue, so the driver should be fixed instead. I would tend to disagree. The serial driver just uses the generic dma API. The error message comes from of_dma_request_slave_channel() in drivers/dma/of-dma.c and is called from dma_request_chan() inn drivers/dma/dmaengine.c. The first function expects a device tree node and "dmas" and "dma-names" properties. And "dma-names" is misused as "enable" switch and if not present (aka disabled) it dumps "dma-names property of node X missing or empty". For me it's clear that a clean way to disable or enable using dma via dts would be better to tell the of_dma_request_slave_channel function that dma is disabled on purpose, so it could return ENODEV but without printing a misleading error level message. Regards, Philipp > >>> >>>> Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> >>>> --- >>>> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index d8543b5557ee..4ae40661ca6a >>>> 100644 >>>> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>> @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { >>>> clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; >>>> clock-names = "baudclk", "apb_pclk"; >>>> dmas = <&dmac0 0>, <&dmac0 1>; >>>> + dma-names = "tx", "rx"; >>>> pinctrl-0 = <&uart0_xfer>; >>>> pinctrl-names = "default"; >>>> reg-io-width = <4>; >>>> @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { >>>> clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; >>>> clock-names = "baudclk", "apb_pclk"; >>>> dmas = <&dmac0 2>, <&dmac0 3>; >>>> + dma-names = "tx", "rx"; >>>> pinctrl-0 = <&uart1m0_xfer>; >>>> pinctrl-names = "default"; >>>> reg-io-width = <4>; >>>> ... >> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip
On 2024-07-10 17:14, Philipp Puschmann wrote: > Am 10.07.24 um 16:56 schrieb Dragan Simic: >> On 2024-07-10 12:20, Philipp Puschmann wrote: >>> Am 10.07.24 um 12:02 schrieb Diederik de Haas: >>>> On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: >>>>> DMA names are required by of_dma_request_slave_channel function >>>>> that is >>>>> called during uart probe. So to enable DMA for uarts add the names >>>>> as in >>>>> the RK3568 TRM. >>>> >>>> Setting it on channels without flow control apparently causes >>>> issues. See >>>> >>>> https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@cknow.org/ >>> >>> Ah is see. The only problem that i have is to enable/disable dmas by >>> having or not having >>> dma-names properties, where the latter case is followed by kernel >>> error messages. That >>> is very counterintuitive. Maybe a explicit boolean like dma-broken >>> would be better. That >>> could be set on dtsi level as default and deleted on board dts if >>> wanted. With such >>> a boolean we could also prevent the misleading "dma-names property >>> of" >>> error message >>> and replace it with a hint that dma is disabled on purpose. >> >> From what I've read in the prior discussions, this seems like a driver >> issue, so the driver should be fixed instead. > > I would tend to disagree. The serial driver just uses the generic dma > API. The error > message comes from of_dma_request_slave_channel() in > drivers/dma/of-dma.c > and is called from dma_request_chan() inn drivers/dma/dmaengine.c. > > The first function expects a device tree node and "dmas" and > "dma-names" properties. > And "dma-names" is misused as "enable" switch and if not present (aka > disabled) it > dumps "dma-names property of node X missing or empty". For me it's > clear that > a clean way to disable or enable using dma via dts would be better to > tell the > of_dma_request_slave_channel function that dma is disabled on purpose, > so it > could return ENODEV but without printing a misleading error level > message. Hmm, please give me some time to investigate it further. >>>>> Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> >>>>> --- >>>>> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index >>>>> d8543b5557ee..4ae40661ca6a >>>>> 100644 >>>>> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>> @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { >>>>> clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; >>>>> clock-names = "baudclk", "apb_pclk"; >>>>> dmas = <&dmac0 0>, <&dmac0 1>; >>>>> + dma-names = "tx", "rx"; >>>>> pinctrl-0 = <&uart0_xfer>; >>>>> pinctrl-names = "default"; >>>>> reg-io-width = <4>; >>>>> @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { >>>>> clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; >>>>> clock-names = "baudclk", "apb_pclk"; >>>>> dmas = <&dmac0 2>, <&dmac0 3>; >>>>> + dma-names = "tx", "rx"; >>>>> pinctrl-0 = <&uart1m0_xfer>; >>>>> pinctrl-names = "default"; >>>>> reg-io-width = <4>; >>>>> ... >>> >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi Philipp. Hi Dragan, Am 10.07.24 um 17:14 schrieb Philipp Puschmann: > Hi Dragan, > > Am 10.07.24 um 16:56 schrieb Dragan Simic: >> Hello Philipp, >> >> On 2024-07-10 12:20, Philipp Puschmann wrote: >>> Am 10.07.24 um 12:02 schrieb Diederik de Haas: >>>> On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: >>>>> DMA names are required by of_dma_request_slave_channel function that is >>>>> called during uart probe. So to enable DMA for uarts add the names as in >>>>> the RK3568 TRM. >>>> >>>> Setting it on channels without flow control apparently causes issues. See >>>> >>>> https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@cknow.org/ >>> >>> Ah is see. The only problem that i have is to enable/disable dmas by >>> having or not having >>> dma-names properties, where the latter case is followed by kernel >>> error messages. That >>> is very counterintuitive. Maybe a explicit boolean like dma-broken >>> would be better. That >>> could be set on dtsi level as default and deleted on board dts if >>> wanted. With such >>> a boolean we could also prevent the misleading "dma-names property of" >>> error message >>> and replace it with a hint that dma is disabled on purpose. >> >> From what I've read in the prior discussions, this seems like a driver >> issue, so the driver should be fixed instead. > > I would tend to disagree. The serial driver just uses the generic dma API. The error > message comes from of_dma_request_slave_channel() in drivers/dma/of-dma.c > and is called from dma_request_chan() inn drivers/dma/dmaengine.c. > > The first function expects a device tree node and "dmas" and "dma-names" properties. > And "dma-names" is misused as "enable" switch and if not present (aka disabled) it > dumps "dma-names property of node X missing or empty". For me it's clear that > a clean way to disable or enable using dma via dts would be better to tell the > of_dma_request_slave_channel function that dma is disabled on purpose, so it > could return ENODEV but without printing a misleading error level message. > > Regards, > Philipp I'm with Philipp here. Topmost because DT should have to do nothing with a (speculative) driver issue, it represents hardware. I would go further and say boards using a (non-kernel console) uart, which can't use dma for whatever reason should disable it. It will never be an issue for the kernel console (i.e. "stdout-path = "serialX..." in DT or "console=ttySX" in cmdline) as the kernel will not use dma for this console. It's by the way even worse for RK3399 Soc DT, which just doesn't expose the dma channels for the uarts and for RK3368 which does not a expose a single dma channel of the peripheral dmac (I tent to speculate for the very same reason). Alex >> >>>> >>>>> Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> >>>>> --- >>>>> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index d8543b5557ee..4ae40661ca6a >>>>> 100644 >>>>> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>> @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { >>>>> clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; >>>>> clock-names = "baudclk", "apb_pclk"; >>>>> dmas = <&dmac0 0>, <&dmac0 1>; >>>>> + dma-names = "tx", "rx"; >>>>> pinctrl-0 = <&uart0_xfer>; >>>>> pinctrl-names = "default"; >>>>> reg-io-width = <4>; >>>>> @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { >>>>> clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; >>>>> clock-names = "baudclk", "apb_pclk"; >>>>> dmas = <&dmac0 2>, <&dmac0 3>; >>>>> + dma-names = "tx", "rx"; >>>>> pinctrl-0 = <&uart1m0_xfer>; >>>>> pinctrl-names = "default"; >>>>> reg-io-width = <4>; >>>>> ... >>> >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hello Alex, On 2024-07-10 20:05, Alex Bee wrote: > Am 10.07.24 um 17:14 schrieb Philipp Puschmann: >> Am 10.07.24 um 16:56 schrieb Dragan Simic: >>> On 2024-07-10 12:20, Philipp Puschmann wrote: >>>> Am 10.07.24 um 12:02 schrieb Diederik de Haas: >>>>> On Wednesday, 10 July 2024 11:33:56 CEST Philipp Puschmann wrote: >>>>>> DMA names are required by of_dma_request_slave_channel function >>>>>> that is >>>>>> called during uart probe. So to enable DMA for uarts add the names >>>>>> as in >>>>>> the RK3568 TRM. >>>>> >>>>> Setting it on channels without flow control apparently causes >>>>> issues. See >>>>> >>>>> https://lore.kernel.org/linux-rockchip/20240628120130.24076-1-didi.debian@cknow.org/ >>>> >>>> Ah is see. The only problem that i have is to enable/disable dmas by >>>> having or not having >>>> dma-names properties, where the latter case is followed by kernel >>>> error messages. That >>>> is very counterintuitive. Maybe a explicit boolean like dma-broken >>>> would be better. That >>>> could be set on dtsi level as default and deleted on board dts if >>>> wanted. With such >>>> a boolean we could also prevent the misleading "dma-names property >>>> of" >>>> error message >>>> and replace it with a hint that dma is disabled on purpose. >>> >>> From what I've read in the prior discussions, this seems like a >>> driver >>> issue, so the driver should be fixed instead. >> >> I would tend to disagree. The serial driver just uses the generic dma >> API. The error >> message comes from of_dma_request_slave_channel() in >> drivers/dma/of-dma.c >> and is called from dma_request_chan() inn drivers/dma/dmaengine.c. >> >> The first function expects a device tree node and "dmas" and >> "dma-names" properties. >> And "dma-names" is misused as "enable" switch and if not present (aka >> disabled) it >> dumps "dma-names property of node X missing or empty". For me it's >> clear that >> a clean way to disable or enable using dma via dts would be better to >> tell the >> of_dma_request_slave_channel function that dma is disabled on purpose, >> so it >> could return ENODEV but without printing a misleading error level >> message. > > I'm with Philipp here. Topmost because DT should have to do nothing > with a > (speculative) driver issue, it represents hardware. I agree that, ideally, it should actually be seen and treated as a SoC feature, i.e. DMA should be described and enabled at the SoC dtsi level. > I would go further and > say boards using a (non-kernel console) uart, which can't use dma for > whatever reason should disable it. It will never be an issue for the > kernel > console (i.e. "stdout-path = "serialX..." in DT or "console=ttySX" in > cmdline) as the kernel will not use dma for this console. It's by the > way > even worse for RK3399 Soc DT, which just doesn't expose the dma > channels > for the uarts and for RK3368 which does not a expose a single dma > channel > of the peripheral dmac (I tent to speculate for the very same reason). I also agree about the specific boards disabling DMA if they cannot use it for whatever reason, simply because DMA would (or should) be enabled at the SoC dtsi level. Though, I'd like to have some time for researching it further, in an attempt to figure out what's actually going on. >>>>>> Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> >>>>>> --- >>>>>> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ >>>>>> 1 file changed, 10 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>>> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index >>>>>> d8543b5557ee..4ae40661ca6a >>>>>> 100644 >>>>>> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi >>>>>> @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { >>>>>> clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; >>>>>> clock-names = "baudclk", "apb_pclk"; >>>>>> dmas = <&dmac0 0>, <&dmac0 1>; >>>>>> + dma-names = "tx", "rx"; >>>>>> pinctrl-0 = <&uart0_xfer>; >>>>>> pinctrl-names = "default"; >>>>>> reg-io-width = <4>; >>>>>> @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { >>>>>> clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; >>>>>> clock-names = "baudclk", "apb_pclk"; >>>>>> dmas = <&dmac0 2>, <&dmac0 3>; >>>>>> + dma-names = "tx", "rx"; >>>>>> pinctrl-0 = <&uart1m0_xfer>; >>>>>> pinctrl-names = "default"; >>>>>> reg-io-width = <4>; >>>>>> ... >>>> >>>> _______________________________________________ >>>> Linux-rockchip mailing list >>>> Linux-rockchip@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index d8543b5557ee..4ae40661ca6a 100644 --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi @@ -489,6 +489,7 @@ uart0: serial@fdd50000 { clocks = <&pmucru SCLK_UART0>, <&pmucru PCLK_UART0>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 0>, <&dmac0 1>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1389,6 +1390,7 @@ uart1: serial@fe650000 { clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 2>, <&dmac0 3>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart1m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1403,6 +1405,7 @@ uart2: serial@fe660000 { clocks = <&cru SCLK_UART2>, <&cru PCLK_UART2>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 4>, <&dmac0 5>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart2m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1417,6 +1420,7 @@ uart3: serial@fe670000 { clocks = <&cru SCLK_UART3>, <&cru PCLK_UART3>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 6>, <&dmac0 7>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart3m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1431,6 +1435,7 @@ uart4: serial@fe680000 { clocks = <&cru SCLK_UART4>, <&cru PCLK_UART4>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 8>, <&dmac0 9>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart4m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1445,6 +1450,7 @@ uart5: serial@fe690000 { clocks = <&cru SCLK_UART5>, <&cru PCLK_UART5>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 10>, <&dmac0 11>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart5m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1459,6 +1465,7 @@ uart6: serial@fe6a0000 { clocks = <&cru SCLK_UART6>, <&cru PCLK_UART6>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 12>, <&dmac0 13>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart6m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1473,6 +1480,7 @@ uart7: serial@fe6b0000 { clocks = <&cru SCLK_UART7>, <&cru PCLK_UART7>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 14>, <&dmac0 15>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart7m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1487,6 +1495,7 @@ uart8: serial@fe6c0000 { clocks = <&cru SCLK_UART8>, <&cru PCLK_UART8>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 16>, <&dmac0 17>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart8m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>; @@ -1501,6 +1510,7 @@ uart9: serial@fe6d0000 { clocks = <&cru SCLK_UART9>, <&cru PCLK_UART9>; clock-names = "baudclk", "apb_pclk"; dmas = <&dmac0 18>, <&dmac0 19>; + dma-names = "tx", "rx"; pinctrl-0 = <&uart9m0_xfer>; pinctrl-names = "default"; reg-io-width = <4>;
DMA names are required by of_dma_request_slave_channel function that is called during uart probe. So to enable DMA for uarts add the names as in the RK3568 TRM. Signed-off-by: Philipp Puschmann <p.puschmann@pironex.com> --- arch/arm64/boot/dts/rockchip/rk356x.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+)