Re: [PATCH] arm64: dts: rockchip: Add uart dma names to the SoC dtsi for RK356x

[Diederik de Haas <didi.debian@cknow.org>]

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

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>;
> >> 
> >> ...


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]