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

[Dragan Simic <dsimic@manjaro.org>]

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