[PATCH 0/2] Disable DMA on secondary UART on PX30 Ringneck

[PATCH 0/2] Disable DMA on secondary UART on PX30 Ringneck

[Lukasz Czechowski]

The PX30-uQ7 (Ringneck) SoM has two external UARTs, connected to
uart0 and uart5 controllers on the PX30 SoC. The uart5 does not
expose RTS/CTS pins on the Q7 connector, as they are used for
different purposes. It was observed that UART controllers without
hardware flow controller behave unstable if the DMA is enabled.
This patch series moves the pinctrl-0 to SoM dtsi file and uses
/delete-property/ to remove DMA from this UART controller.

Lukasz Czechowski (2):
  arm64: dts: rockchip: Move uart5 pin configuration to SoM dtsi
  arm64: dts: rockchip: Disable DMA for uart5 on px30-ringneck

 arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts | 1 -
 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi       | 6 ++++++
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/2] arm64: dts: rockchip: Move uart5 pin configuration to SoM dtsi

[Lukasz Czechowski]

In the PX30-uQ7 (Ringneck) SoM, the hardware CTS and RTS pins for
uart5 cannot be used for the UART CTS/RTS, because they are already
allocated for different purposes. CTS pin is routed to SUS_S3#
signal, while RTS pin is used internally and is not available on
Q7 connector. Move definition of the pinctrl-0 property from
px30-ringneck-haikou.dts to px30-ringneck.dtsi.

Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
---
 arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts | 1 -
 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi       | 4 ++++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
index e4517f47d519c..eb9470a00e549 100644
--- a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
@@ -226,7 +226,6 @@ &uart0 {
 };
 
 &uart5 {
-	pinctrl-0 = <&uart5_xfer>;
 	rts-gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
index ae050cc6cd050..2c87005c89bd3 100644
--- a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
@@ -396,6 +396,10 @@ &u2phy_host {
 	status = "okay";
 };
 
+&uart5 {
+	pinctrl-0 = <&uart5_xfer>;
+};
+
 /* Mule UCAN */
 &usb_host0_ehci {
 	status = "okay";
-- 
2.43.0

Re: [PATCH 1/2] arm64: dts: rockchip: Move uart5 pin configuration to SoM dtsi

[Quentin Schulz]

Hi Lukasz,

On 1/21/25 10:22 AM, Lukasz Czechowski wrote:
> In the PX30-uQ7 (Ringneck) SoM, the hardware CTS and RTS pins for
> uart5 cannot be used for the UART CTS/RTS, because they are already
> allocated for different purposes. CTS pin is routed to SUS_S3#
> signal, while RTS pin is used internally and is not available on
> Q7 connector. Move definition of the pinctrl-0 property from
> px30-ringneck-haikou.dts to px30-ringneck.dtsi.
> 
> Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Considering that I'll request the next patch to be backported to stable 
releases, we should probably backport this one as well to avoid a git 
conflict?

c.f. 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch

Essentially: Cc: stable@vger.kernel.org

@Heiko, this one patch isn't really a genuine candidate for stable 
backport as it's just moving things around but it'll result in a git 
conflict when backporting the other patch, how is it usually done for 
stable releases?

Thanks!
Quentin

Re: [PATCH 1/2] arm64: dts: rockchip: Move uart5 pin configuration to SoM dtsi

[Heiko Stübner]

Am Dienstag, 21. Januar 2025, 10:36:15 CET schrieb Quentin Schulz:
> Hi Lukasz,
> 
> On 1/21/25 10:22 AM, Lukasz Czechowski wrote:
> > In the PX30-uQ7 (Ringneck) SoM, the hardware CTS and RTS pins for
> > uart5 cannot be used for the UART CTS/RTS, because they are already
> > allocated for different purposes. CTS pin is routed to SUS_S3#
> > signal, while RTS pin is used internally and is not available on
> > Q7 connector. Move definition of the pinctrl-0 property from
> > px30-ringneck-haikou.dts to px30-ringneck.dtsi.
> > 
> > Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Considering that I'll request the next patch to be backported to stable 
> releases, we should probably backport this one as well to avoid a git 
> conflict?
> 
> c.f. 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch
> 
> Essentially: Cc: stable@vger.kernel.org
> 
> @Heiko, this one patch isn't really a genuine candidate for stable 
> backport as it's just moving things around but it'll result in a git 
> conflict when backporting the other patch, how is it usually done for 
> stable releases?

Just Cc-stable and possibly explain it being a dependency in the commit message.

And patch2 should ideally get a Fixes tag

[PATCH 2/2] arm64: dts: rockchip: Disable DMA for uart5 on px30-ringneck

[Lukasz Czechowski]

UART controllers without flow control seem to behave unstable
in case DMA is enabled. The issues were indicated in the message:
https://lore.kernel.org/linux-arm-kernel/CAMdYzYpXtMocCtCpZLU_xuWmOp2Ja_v0Aj0e6YFNRA-yV7u14g@mail.gmail.com/
In case of PX30-uQ7 Ringneck SoM, it was noticed that after couple
of hours of UART communication, the CPU stall was occurring,
leading to the system becoming unresponsive.
After disabling the DMA, extensive UART communication tests for
up to two weeks were performed, and no issues were further
observed.
The flow control pins for uart5 are not available on PX30-uQ7
Ringneck, as configured by pinctrl-0, so the DMA nodes were
removed on SoM dtsi.

Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
---
 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
index 2c87005c89bd3..e80412abec081 100644
--- a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
@@ -397,6 +397,8 @@ &u2phy_host {
 };
 
 &uart5 {
+	/delete-property/ dmas;
+	/delete-property/ dma-names;
 	pinctrl-0 = <&uart5_xfer>;
 };
 
-- 
2.43.0

Re: [PATCH 2/2] arm64: dts: rockchip: Disable DMA for uart5 on px30-ringneck

[Quentin Schulz]

Hi Lukasz,

On 1/21/25 10:22 AM, Lukasz Czechowski wrote:
> UART controllers without flow control seem to behave unstable
> in case DMA is enabled. The issues were indicated in the message:
> https://lore.kernel.org/linux-arm-kernel/CAMdYzYpXtMocCtCpZLU_xuWmOp2Ja_v0Aj0e6YFNRA-yV7u14g@mail.gmail.com/
> In case of PX30-uQ7 Ringneck SoM, it was noticed that after couple
> of hours of UART communication, the CPU stall was occurring,
> leading to the system becoming unresponsive.
> After disabling the DMA, extensive UART communication tests for
> up to two weeks were performed, and no issues were further
> observed.
> The flow control pins for uart5 are not available on PX30-uQ7
> Ringneck, as configured by pinctrl-0, so the DMA nodes were
> removed on SoM dtsi.
> 

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

We should backport this to stable releases too, so please follow the 
instructions from here: 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch

Essentially:

Cc: stable@vger.kernel.org

in the commit log and we'll need a

Fixes: <commit hash>

trailer as well with the commit hash of the commit introducing the issue 
(likely the one defining uart5 for Ringneck for us).

Considering that UART0 CTS and RTS are routed to Q7 header but only 
usable when Haikou exposes UART0 on the DB9 connector (via the SW2 
switch), which is NOT the default state (and in any case not supported 
by our current device tree), I believe we should make the same change to 
the uart0 node in haikou dts for Ringneck. What do you think? Can you 
send another patch for that one?

Thanks!
Quentin

Re: [PATCH 2/2] arm64: dts: rockchip: Disable DMA for uart5 on px30-ringneck

[Łukasz Czechowski]

Hi Quentin,

> On 1/21/25 10:41 AM, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Lukasz,
>
> On 1/21/25 10:22 AM, Lukasz Czechowski wrote:
> > UART controllers without flow control seem to behave unstable
> > in case DMA is enabled. The issues were indicated in the message:
> > https://lore.kernel.org/linux-arm-kernel/CAMdYzYpXtMocCtCpZLU_xuWmOp2Ja_v0Aj0e6YFNRA-yV7u14g@mail.gmail.com/
> > In case of PX30-uQ7 Ringneck SoM, it was noticed that after couple
> > of hours of UART communication, the CPU stall was occurring,
> > leading to the system becoming unresponsive.
> > After disabling the DMA, extensive UART communication tests for
> > up to two weeks were performed, and no issues were further
> > observed.
> > The flow control pins for uart5 are not available on PX30-uQ7
> > Ringneck, as configured by pinctrl-0, so the DMA nodes were
> > removed on SoM dtsi.
> >
>
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>
> We should backport this to stable releases too, so please follow the
> instructions from here:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch
>
> Essentially:
>
> Cc: stable@vger.kernel.org
>
> in the commit log and we'll need a
>
> Fixes: <commit hash>
>
> trailer as well with the commit hash of the commit introducing the issue
> (likely the one defining uart5 for Ringneck for us).
>
> Considering that UART0 CTS and RTS are routed to Q7 header but only
> usable when Haikou exposes UART0 on the DB9 connector (via the SW2
> switch), which is NOT the default state (and in any case not supported
> by our current device tree), I believe we should make the same change to
> the uart0 node in haikou dts for Ringneck. What do you think? Can you
> send another patch for that one?

It seems that in case of uart0, that is configured as kernel console, the DMA
is not used by the kernel:
https://lore.kernel.org/linux-serial/20200217114016.49856-7-andriy.shevchenko@linux.intel.com/
Which is likely why the issue was not observed so far. However it might be
good to do the same change to be on the safe side.
Should I extend this patch series with the fix for the Haikou device tree then,
or create a new one?

>
> Thanks!
> Quentin

Best Regards,
Lukasz

Re: [PATCH 2/2] arm64: dts: rockchip: Disable DMA for uart5 on px30-ringneck

[Quentin Schulz]

Hi Łukasz,

On 1/21/25 11:28 AM, Łukasz Czechowski wrote:
> Hi Quentin,
> 
>> On 1/21/25 10:41 AM, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Lukasz,
>>
>> On 1/21/25 10:22 AM, Lukasz Czechowski wrote:
>>> UART controllers without flow control seem to behave unstable
>>> in case DMA is enabled. The issues were indicated in the message:
>>> https://lore.kernel.org/linux-arm-kernel/CAMdYzYpXtMocCtCpZLU_xuWmOp2Ja_v0Aj0e6YFNRA-yV7u14g@mail.gmail.com/
>>> In case of PX30-uQ7 Ringneck SoM, it was noticed that after couple
>>> of hours of UART communication, the CPU stall was occurring,
>>> leading to the system becoming unresponsive.
>>> After disabling the DMA, extensive UART communication tests for
>>> up to two weeks were performed, and no issues were further
>>> observed.
>>> The flow control pins for uart5 are not available on PX30-uQ7
>>> Ringneck, as configured by pinctrl-0, so the DMA nodes were
>>> removed on SoM dtsi.
>>>
>>
>> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> We should backport this to stable releases too, so please follow the
>> instructions from here:
>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch
>>
>> Essentially:
>>
>> Cc: stable@vger.kernel.org
>>
>> in the commit log and we'll need a
>>
>> Fixes: <commit hash>
>>
>> trailer as well with the commit hash of the commit introducing the issue
>> (likely the one defining uart5 for Ringneck for us).
>>
>> Considering that UART0 CTS and RTS are routed to Q7 header but only
>> usable when Haikou exposes UART0 on the DB9 connector (via the SW2
>> switch), which is NOT the default state (and in any case not supported
>> by our current device tree), I believe we should make the same change to
>> the uart0 node in haikou dts for Ringneck. What do you think? Can you
>> send another patch for that one?
> 
> It seems that in case of uart0, that is configured as kernel console, the DMA
> is not used by the kernel:
> https://lore.kernel.org/linux-serial/20200217114016.49856-7-andriy.shevchenko@linux.intel.com/
> Which is likely why the issue was not observed so far. However it might be
> good to do the same change to be on the safe side.
> Should I extend this patch series with the fix for the Haikou device tree then,
> or create a new one?
> 

Ah, I had missed that one, thanks for the heads up. Just a small remark, 
better send links to merged commits rather than mailing list patches, 
it's a bit more difficult to figure out if the patch (and that version 
of the patch) is merged. If there was an interesting discussion on the 
patch and that information got lost when merging, nice to add the ML 
link though!
In the case of the linked patch, it is merged indeed: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/tty/serial/8250/8250_port.c?id=089b6d36549160b535e109cd07ed6d578b81de46

No, no need to send a patch. When Haikou is configured to have UART0 on 
the DB9 connector (and the Device Tree is adapted for it) and not use it 
as kernel console, we should be able to use hardware flow control and 
not have this issue. So we don't need to remove dmas/dma-names when it's 
used as kernel console, and we don't need to remove them when routed to 
DB9 connector either, so all good :)

Cheers,
Quentin