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@xxxxxxxxx/ > > 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@xxxxxxxxxxx> >>>> --- >>>> >>>> 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>; >>>> >>>> ... > -- Philipp Puschmann, M.Sc. Softwareentwicklung pironex GmbH Stangenland 4 18146 Rostock www.pironex.de Tel.: +49 (0) 381 7006 08 44 E-Mail: p.puschmann@xxxxxxxxxxx Firma: +49 (0) 381 70 06 08 0 Fax: +49 (0) 381 49 68 02 77 Web: http://www.pironex.de
