[PATCHv5 0/2] Fix imx53-ppd UART configuration

[PATCHv5 0/2] Fix imx53-ppd UART configuration

[Sebastian Reichel]

Hi,

IMHO PATCHv4 was better, but in the end I don't have strong feelings
about this. Btw. I think this patchset is a good demonstration of
frustrating upstream kernel development can be considering PATCHv5
is basically the same as PATCHv1. Thanks for making us go in
circles :(

Changes since PATCHv4:
 * https://lore.kernel.org/lkml/20210305115058.92284-1-sebastian.reichel@collabora.com/
 * use DT property instead of sysfs config option, like the initial patch
   version did as requested by Greg.

Changes since PATCHv3:
 * https://lore.kernel.org/lkml/1539249903-6316-1-git-send-email-fabien.lahoudere@collabora.com/
 * rewrote commit message to provide a lot more details why this is needed
 * rebased to torvalds/master (5.12-rc1-dontuse), also applies on top of linux-next
 * use sysfs_emit() instead of sprintf

-- Sebastian

Fabien Lahoudere (2):
  serial: imx: Add DMA buffer configuration via DT
  ARM: dts: imx53-ppd: add dma-info nodes

 .../bindings/serial/fsl-imx-uart.yaml         | 12 +++++++++
 arch/arm/boot/dts/imx53-ppd.dts               |  2 ++
 drivers/tty/serial/imx.c                      | 25 +++++++++++++------
 3 files changed, 32 insertions(+), 7 deletions(-)

-- 
2.30.2

[PATCHv5 1/2] serial: imx: Add DMA buffer configuration via DT

[Sebastian Reichel]

From: Fabien Lahoudere <fabien.lahoudere@collabora.com>

In order to optimize serial communication (performance/throughput VS
latency), we may need to tweak DMA period number and size. This adds
DT properties to configure those values before initialising DMA.
The defaults will stay the same as before.

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
[update documentation and commit message, rebase to current master,
switch back to DT instead of sysfs]
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../bindings/serial/fsl-imx-uart.yaml         | 12 +++++++++
 drivers/tty/serial/imx.c                      | 25 +++++++++++++------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml b/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
index 2b06c6ce4a75..9d949296a142 100644
--- a/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
@@ -71,6 +71,18 @@ properties:
       received, and that the peripheral should invert its input using the
       INVR registers.
 
+  fsl,dma-info:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 2
+    maxItems: 2
+    description: |
+      First cell contains the size of DMA buffer chunks, second cell contains
+      the amount of chunks used for the device. Multiplying both numbers is
+      the total size of memory used for receiving data.
+      When not being configured the system will use default settings, which
+      are sensible for most use cases. If you need low latency processing on
+      slow connections this needs to be configured appropriately.
+
   uart-has-rtscts: true
 
   rs485-rts-delay: true
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 7d5a8dfa3e91..9e72e382a867 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -225,6 +225,8 @@ struct imx_port {
 	struct scatterlist	rx_sgl, tx_sgl[2];
 	void			*rx_buf;
 	struct circ_buf		rx_ring;
+	unsigned int		rx_buf_size;
+	unsigned int		rx_period_length;
 	unsigned int		rx_periods;
 	dma_cookie_t		rx_cookie;
 	unsigned int		tx_bytes;
@@ -1183,10 +1185,6 @@ static void imx_uart_dma_rx_callback(void *data)
 	}
 }
 
-/* RX DMA buffer periods */
-#define RX_DMA_PERIODS	16
-#define RX_BUF_SIZE	(RX_DMA_PERIODS * PAGE_SIZE / 4)
-
 static int imx_uart_start_rx_dma(struct imx_port *sport)
 {
 	struct scatterlist *sgl = &sport->rx_sgl;
@@ -1197,9 +1195,8 @@ static int imx_uart_start_rx_dma(struct imx_port *sport)
 
 	sport->rx_ring.head = 0;
 	sport->rx_ring.tail = 0;
-	sport->rx_periods = RX_DMA_PERIODS;
 
-	sg_init_one(sgl, sport->rx_buf, RX_BUF_SIZE);
+	sg_init_one(sgl, sport->rx_buf, sport->rx_buf_size);
 	ret = dma_map_sg(dev, sgl, 1, DMA_FROM_DEVICE);
 	if (ret == 0) {
 		dev_err(dev, "DMA mapping error for RX.\n");
@@ -1316,7 +1313,8 @@ static int imx_uart_dma_init(struct imx_port *sport)
 		goto err;
 	}
 
-	sport->rx_buf = kzalloc(RX_BUF_SIZE, GFP_KERNEL);
+	sport->rx_buf_size = sport->rx_period_length * sport->rx_periods;
+	sport->rx_buf = kzalloc(sport->rx_buf_size, GFP_KERNEL);
 	if (!sport->rx_buf) {
 		ret = -ENOMEM;
 		goto err;
@@ -2179,11 +2177,16 @@ static enum hrtimer_restart imx_trigger_stop_tx(struct hrtimer *t)
 	return HRTIMER_NORESTART;
 }
 
+/* Default RX DMA buffer configuration */
+#define RX_DMA_PERIODS		16
+#define RX_DMA_PERIOD_LEN	(PAGE_SIZE / 4)
+
 static int imx_uart_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct imx_port *sport;
 	void __iomem *base;
+	u32 dma_buf_conf[2];
 	int ret = 0;
 	u32 ucr1;
 	struct resource *res;
@@ -2218,6 +2221,14 @@ static int imx_uart_probe(struct platform_device *pdev)
 	if (of_get_property(np, "fsl,inverted-rx", NULL))
 		sport->inverted_rx = 1;
 
+	if (!of_property_read_u32_array(np, "fsl,dma-info", dma_buf_conf, 2)) {
+		sport->rx_period_length = dma_buf_conf[0];
+		sport->rx_periods = dma_buf_conf[1];
+	} else {
+		sport->rx_period_length = RX_DMA_PERIOD_LEN;
+		sport->rx_periods = RX_DMA_PERIODS;
+	}
+
 	if (sport->port.line >= ARRAY_SIZE(imx_uart_ports)) {
 		dev_err(&pdev->dev, "serial%d out of range\n",
 			sport->port.line);
-- 
2.30.2

[PATCHv5 2/2] ARM: dts: imx53-ppd: add dma-info nodes

[Sebastian Reichel]

From: Fabien Lahoudere <fabien.lahoudere@collabora.com>

GEHC CS ONE (codename is PPD), has multiple microcontrollers connected
via UART controlling. UART2 is connected to an on-board microcontroller
at 19200 baud, which constantly pushes critical data (so aging character
detect interrupt will never trigger). This data must be processed at
50-200 Hz, so UART should return data in less than 5-20ms. With 1024
byte DMA buffer (and a constant data stream) the read operation instead
needs 1024 byte / 19200 baud = 53.333ms, which is way too long (note:
Worst case would be remote processor sending data with short pauses <=
7 characters, which would further increase this number). The current
downstream kernel instead configures 24 bytes resulting in 1.25ms, but
that is obviously not sensible for normal UART use cases and cannot be
used as new default.

The same device also has another microcontroller with a 4M baud connected
to UART5 exchanging lots of data. For this the same mechanism can be used
to increase the buffer size (downstream uses 4K instead of the default 1K)
with potentially slightly reduced buffer count. At this baud rate latency
is not an issue (4096 byte / 4M baud = 0.977 ms). Before increasing the
default buffer count from 4 to 16 in 76c38d30fee7, this was required to
avoid data loss. With the changed default it's a performance optimization.

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
[replace commit message]
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm/boot/dts/imx53-ppd.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/imx53-ppd.dts b/arch/arm/boot/dts/imx53-ppd.dts
index be040b6a02fa..5a5fa6190a52 100644
--- a/arch/arm/boot/dts/imx53-ppd.dts
+++ b/arch/arm/boot/dts/imx53-ppd.dts
@@ -651,6 +651,7 @@ &uart1 {
 &uart2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart2>;
+	fsl,dma-info = <24 20>;
 	status = "okay";
 };
 
@@ -670,6 +671,7 @@ &uart4 {
 &uart5 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart5>;
+	fsl,dma-info = <4096 4>;
 	status = "okay";
 };
 
-- 
2.30.2

Re: [PATCHv5 0/2] Fix imx53-ppd UART configuration

[Sebastian Reichel]

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

Hi Greg,

On Sat, May 01, 2021 at 08:11:52AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> > IMHO PATCHv4 was better, but in the end I don't have strong feelings
> > about this. Btw. I think this patchset is a good demonstration of
> > frustrating upstream kernel development can be considering PATCHv5
> > is basically the same as PATCHv1. Thanks for making us go in
> > circles :(
> > 
> > Changes since PATCHv4:
> >  * https://lore.kernel.org/lkml/20210305115058.92284-1-sebastian.reichel@collabora.com/
> >  * use DT property instead of sysfs config option, like the initial patch
> >    version did as requested by Greg.
> > 
> > Changes since PATCHv3:
> >  * https://lore.kernel.org/lkml/1539249903-6316-1-git-send-email-fabien.lahoudere@collabora.com/
> >  * rewrote commit message to provide a lot more details why this is needed
> >  * rebased to torvalds/master (5.12-rc1-dontuse), also applies on top of linux-next
> >  * use sysfs_emit() instead of sprintf
> > 
> > -- Sebastian
> > 
> > Fabien Lahoudere (2):
> >   serial: imx: Add DMA buffer configuration via DT
> >   ARM: dts: imx53-ppd: add dma-info nodes
> > 
> >  .../bindings/serial/fsl-imx-uart.yaml         | 12 +++++++++
> >  arch/arm/boot/dts/imx53-ppd.dts               |  2 ++
> >  drivers/tty/serial/imx.c                      | 25 +++++++++++++------
> >  3 files changed, 32 insertions(+), 7 deletions(-)
>
> This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> You have sent him a patch that has triggered this response.
> 
> Right now, the development tree you have sent a patch for is "closed"
> due to the timing of the merge window.  Don't worry, the patch(es) you
> have sent are not lost, and will be looked at after the merge window is
> over (after the -rc1 kernel is released by Linus).
> 
> So thank you for your patience and your patches will be reviewed at this
> later time, you do not have to do anything further, this is just a short
> note to let you know the patch status and so you don't worry they didn't
> make it through.
> 
> thanks,
> 
> greg k-h's patch email bot

Any update on this? :)

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCHv5 0/2] Fix imx53-ppd UART configuration

[Greg Kroah-Hartman]

On Fri, May 28, 2021 at 02:49:52AM +0200, Sebastian Reichel wrote:
> Hi Greg,
> 
> On Sat, May 01, 2021 at 08:11:52AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> > > IMHO PATCHv4 was better, but in the end I don't have strong feelings
> > > about this. Btw. I think this patchset is a good demonstration of
> > > frustrating upstream kernel development can be considering PATCHv5
> > > is basically the same as PATCHv1. Thanks for making us go in
> > > circles :(
> > > 
> > > Changes since PATCHv4:
> > >  * https://lore.kernel.org/lkml/20210305115058.92284-1-sebastian.reichel@collabora.com/
> > >  * use DT property instead of sysfs config option, like the initial patch
> > >    version did as requested by Greg.
> > > 
> > > Changes since PATCHv3:
> > >  * https://lore.kernel.org/lkml/1539249903-6316-1-git-send-email-fabien.lahoudere@collabora.com/
> > >  * rewrote commit message to provide a lot more details why this is needed
> > >  * rebased to torvalds/master (5.12-rc1-dontuse), also applies on top of linux-next
> > >  * use sysfs_emit() instead of sprintf
> > > 
> > > -- Sebastian
> > > 
> > > Fabien Lahoudere (2):
> > >   serial: imx: Add DMA buffer configuration via DT
> > >   ARM: dts: imx53-ppd: add dma-info nodes
> > > 
> > >  .../bindings/serial/fsl-imx-uart.yaml         | 12 +++++++++
> > >  arch/arm/boot/dts/imx53-ppd.dts               |  2 ++
> > >  drivers/tty/serial/imx.c                      | 25 +++++++++++++------
> > >  3 files changed, 32 insertions(+), 7 deletions(-)
> >
> > This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> > You have sent him a patch that has triggered this response.
> > 
> > Right now, the development tree you have sent a patch for is "closed"
> > due to the timing of the merge window.  Don't worry, the patch(es) you
> > have sent are not lost, and will be looked at after the merge window is
> > over (after the -rc1 kernel is released by Linus).
> > 
> > So thank you for your patience and your patches will be reviewed at this
> > later time, you do not have to do anything further, this is just a short
> > note to let you know the patch status and so you don't worry they didn't
> > make it through.
> > 
> > thanks,
> > 
> > greg k-h's patch email bot
> 
> Any update on this? :)

I'm waiting for the DT maintainers to review the new changes before I
can take the driver changes.

thanks,

greg k-h

Re: [PATCHv5 0/2] Fix imx53-ppd UART configuration

[Uwe Kleine-König]

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

Hello Greg,

[adding Rob and devicetree@vger to Cc:]

On Fri, May 28, 2021 at 11:03:54AM +0200, Greg Kroah-Hartman wrote:
> On Fri, May 28, 2021 at 02:49:52AM +0200, Sebastian Reichel wrote:
> > On Sat, May 01, 2021 at 08:11:52AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> > > > IMHO PATCHv4 was better, but in the end I don't have strong feelings
> > > > about this. Btw. I think this patchset is a good demonstration of
> > > > frustrating upstream kernel development can be considering PATCHv5
> > > > is basically the same as PATCHv1. Thanks for making us go in
> > > > circles :(

I was the one who objected the v1 approach. My frustration is that I
wasn't Cc:d in the later iterations of this patch set. And I also liked
v4 better :-\

> > > > Changes since PATCHv4:
> > > >  * https://lore.kernel.org/lkml/20210305115058.92284-1-sebastian.reichel@collabora.com/
> > > >  * use DT property instead of sysfs config option, like the initial patch
> > > >    version did as requested by Greg.
> > > > 
> > > > Changes since PATCHv3:
> > > >  * https://lore.kernel.org/lkml/1539249903-6316-1-git-send-email-fabien.lahoudere@collabora.com/
> > > >  * rewrote commit message to provide a lot more details why this is needed
> > > >  * rebased to torvalds/master (5.12-rc1-dontuse), also applies on top of linux-next
> > > >  * use sysfs_emit() instead of sprintf
> > > > 
> > > > -- Sebastian
> > > > 
> > > > Fabien Lahoudere (2):
> > > >   serial: imx: Add DMA buffer configuration via DT
> > > >   ARM: dts: imx53-ppd: add dma-info nodes
> > > > 
> > > >  .../bindings/serial/fsl-imx-uart.yaml         | 12 +++++++++
> > > >  arch/arm/boot/dts/imx53-ppd.dts               |  2 ++
> > > >  drivers/tty/serial/imx.c                      | 25 +++++++++++++------
> > > >  3 files changed, 32 insertions(+), 7 deletions(-)
> > >
> > > This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> > > You have sent him a patch that has triggered this response.
> > > 
> > > Right now, the development tree you have sent a patch for is "closed"
> > > due to the timing of the merge window.  Don't worry, the patch(es) you
> > > have sent are not lost, and will be looked at after the merge window is
> > > over (after the -rc1 kernel is released by Linus).
> > > 
> > > So thank you for your patience and your patches will be reviewed at this
> > > later time, you do not have to do anything further, this is just a short
> > > note to let you know the patch status and so you don't worry they didn't
> > > make it through.
> > > 
> > > thanks,
> > > 
> > > greg k-h's patch email bot
> > 
> > Any update on this? :)
> 
> I'm waiting for the DT maintainers to review the new changes before I
> can take the driver changes.

I wonder what the DT maintainers said (and where [1]). The patch was
applied (db0a196bd8ad1d6bb4b1a9e54f54c09f8dc2cc25) and I think that's
wrong (as it was in v1) because fsl,dma-info isn't about hardware
description but about software tuning for different use cases.

Best regards
Uwe

[1] neither my mailbox nor lore.kernel.org know about an answer.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCHv5 0/2] Fix imx53-ppd UART configuration

[Uwe Kleine-König]

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

[Cc += dt maintainers]

On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> IMHO PATCHv4 was better, but in the end I don't have strong feelings
> about this. Btw. I think this patchset is a good demonstration of
> frustrating upstream kernel development can be considering PATCHv5
> is basically the same as PATCHv1. Thanks for making us go in
> circles :(

I still like v4 better than v1/v5. I'm sorry for the frustration this
created on your side. 

I'd ask Greg to reconsider given that dt is less flexible than a sysfs
knob and otherwise shares all downsides of sysfs (people don't want to
have to tune that, so a useful default for most cases is important; you
have to consult documentation to understand how to tune it).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCHv5 0/2] Fix imx53-ppd UART configuration

[Greg Kroah-Hartman]

On Fri, Dec 08, 2023 at 10:07:54AM +0100, Uwe Kleine-König wrote:
> [Cc += dt maintainers]
> 
> On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:

You are responding to a message from 2021???

I can't remember what I wrote in an email last week, let alone years
ago, are you sure any of this is relevant still?

thanks,

greg k-h

Re: [PATCHv5 0/2] Fix imx53-ppd UART configuration

[Krzysztof Kozlowski]

On 08/12/2023 11:32, Greg Kroah-Hartman wrote:
> On Fri, Dec 08, 2023 at 10:07:54AM +0100, Uwe Kleine-König wrote:
>> [Cc += dt maintainers]
>>
>> On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> 
> You are responding to a message from 2021???
> 
> I can't remember what I wrote in an email last week, let alone years
> ago, are you sure any of this is relevant still?
> 

Also, adding DT maintainers as Cc, does not magically allow us to
understand the discussion. Please shortly summarize what do you
need/expect from us.

Best regards,
Krzysztof

Re: EXT: Re: [PATCHv5 0/2] Fix imx53-ppd UART configuration

[Ian Ray]

On Fri, Dec 08, 2023 at 01:14:50PM +0100, Krzysztof Kozlowski wrote:
> 
> On 08/12/2023 11:32, Greg Kroah-Hartman wrote:
> > On Fri, Dec 08, 2023 at 10:07:54AM +0100, Uwe Kleine-K??nig wrote:
> >> [Cc += dt maintainers]
> >>
> >> On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> > 
> > You are responding to a message from 2021???
> > 
> > I can't remember what I wrote in an email last week, let alone years
> > ago, are you sure any of this is relevant still?
> > 
> 
> Also, adding DT maintainers as Cc, does not magically allow us to
> understand the discussion. Please shortly summarize what do you
> need/expect from us.

This thread is indeed old, but we are remain interested in getting
this resolved.


The topic itself is about the tuning of DMA buffers for the i.MX53
platform.

DMA is required (for the 4M baud UART case), and a specific buffer
configuration is required, too.


Two approaches have been attempted:

"V4" sysfs
https://lore.kernel.org/lkml/20210305115058.92284-1-sebastian.reichel@collabora.com/

"V5" DT
https://lore.kernel.org/lkml/20210430175038.103226-1-sebastian.reichel@collabora.com/
(this thread)


Uwe recently commented to [1], and (hoping to avoid any further
confusion) let's continue in that thread.

[1] https://lore.kernel.org/lkml/20231208090205.ioc76sych3snjiwb@pengutronix.de/


> 
> Best regards,
> Krzysztof
> 
>