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

[Philipp Puschmann <p.puschmann@pironex.com>]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip