[PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Biju Das]

As per RZ/N1 peripheral user manual(r01uh0752ej0100-rzn1-peripheral.pdf)
rev 1.0.0 Mar,2019, the value for 8_WORD_BURST is 4(b2,b1=2’b10).

This patch fixes the macro as per the user manual.

Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow controlling devices")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/tty/serial/8250/8250_dw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index f57bbd32ef11..931490b27d6b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -47,7 +47,7 @@
 #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
 #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
 #define RZN1_UART_xDMACR_4_WORD_BURST	(1 << 1)
-#define RZN1_UART_xDMACR_8_WORD_BURST	(3 << 1)
+#define RZN1_UART_xDMACR_8_WORD_BURST	(2 << 1)
 #define RZN1_UART_xDMACR_BLK_SZ(x)	((x) << 3)
 
 /* Quirks */
-- 
2.25.1

RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Phil Edworthy]

Hi Biju,

On 30 June 2022 09:39 Biju Das wrote:
> As per RZ/N1 peripheral user manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> rev 1.0.0 Mar,2019, the value for 8_WORD_BURST is 4(b2,b1=2’b10).
> 
> This patch fixes the macro as per the user manual.
> 
> Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow
> controlling devices")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Phil Edworthy <phil.edworthy@renesas.com>

> ---
>  drivers/tty/serial/8250/8250_dw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index f57bbd32ef11..931490b27d6b 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -47,7 +47,7 @@
>  #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
>  #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
>  #define RZN1_UART_xDMACR_4_WORD_BURST	(1 << 1)
> -#define RZN1_UART_xDMACR_8_WORD_BURST	(3 << 1)
> +#define RZN1_UART_xDMACR_8_WORD_BURST	(2 << 1)
>  #define RZN1_UART_xDMACR_BLK_SZ(x)	((x) << 3)
> 
>  /* Quirks */
> --
> 2.25.1

Thanks
Phil

Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Jiri Slaby]

On 30. 06. 22, 10:39, Biju Das wrote:
> As per RZ/N1 peripheral user manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> rev 1.0.0 Mar,2019,

Is this public anywhere?

> the value for 8_WORD_BURST is 4(b2,b1=2’b10).
> 
> This patch fixes the macro as per the user manual.

I'm curious, is the bottom bit from "3" ignored by the HW or does this 
fix a real problem in behavior? Stating that might help backporters to 
decide if to take the patch or not.

> Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow controlling devices")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>   drivers/tty/serial/8250/8250_dw.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index f57bbd32ef11..931490b27d6b 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -47,7 +47,7 @@
>   #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
>   #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
>   #define RZN1_UART_xDMACR_4_WORD_BURST	(1 << 1)
> -#define RZN1_UART_xDMACR_8_WORD_BURST	(3 << 1)
> +#define RZN1_UART_xDMACR_8_WORD_BURST	(2 << 1)
>   #define RZN1_UART_xDMACR_BLK_SZ(x)	((x) << 3)
>   
>   /* Quirks */

thanks,
-- 
js

RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Biju Das]

Hi Jiri,

Thanks for the feedback.

> Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> RZN1_UART_xDMACR_8_WORD_BURST
> 
> On 30. 06. 22, 10:39, Biju Das wrote:
> > As per RZ/N1 peripheral user
> > manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> > rev 1.0.0 Mar,2019,
> 
> Is this public anywhere?

Yes, It is available here[1] see page 72 and 73.

[1] https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-peripherals?language=en&r=1054561


> 
> > the value for 8_WORD_BURST is 4(b2,b1=2’b10).
> >
> > This patch fixes the macro as per the user manual.
> 
> I'm curious, is the bottom bit from "3" ignored by the HW or does this
> fix a real problem in behavior? Stating that might help backporters to
> decide if to take the patch or not.

See page 72 and 73.

Yes, it fixes a real problem as by using a value of 8 , you are wrongly configuring
DMA_BURST_SIZE of 1 instead of DMA_BURST_SIZE of 8.

b2, b1 bUart_DEST_BURST
_SIZE
DEST_BURST_SIZE
Destination Burst Transaction Size in Transmit FIFO.
UART is the flow controller. Thus, the user must write this field before or at the same
time the DMA mode is enabled. Number of data byte, to be written to the Transmit
FIFO every time a transmit burst transaction request are made on DMA request.
2’b00 = 1 byte
2’b01 = 4 bytes
2’b10 = 8 bytes
2’b11 = Reserved, not used

Cheers,
Biju


> 
> > Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow
> > controlling devices")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >   drivers/tty/serial/8250/8250_dw.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > b/drivers/tty/serial/8250/8250_dw.c
> > index f57bbd32ef11..931490b27d6b 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -47,7 +47,7 @@
> >   #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
> >   #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
> >   #define RZN1_UART_xDMACR_4_WORD_BURST	(1 << 1)
> > -#define RZN1_UART_xDMACR_8_WORD_BURST	(3 << 1)
> > +#define RZN1_UART_xDMACR_8_WORD_BURST	(2 << 1)
> >   #define RZN1_UART_xDMACR_BLK_SZ(x)	((x) << 3)
> >
> >   /* Quirks */
> 
> thanks,
> --
> js

RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Biju Das]

Hi Jiri and Miquel,

While testing serial driver with RZ/N1 on 5.15 kernel, which is the backport of mainline kernel,
I seen performance issue with serial DMA for higher baud rates.

The test app is taking 25 minutes finish, whereas with the below patch[1] it takes only 3 minutes to finish.

Not sure has anyone seen this performance issue?

[1]
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 468d1aca5968..321430176698 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2680,7 +2680,7 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port,
                max = (port->uartclk + tolerance) / 4;
        } else {
                min = port->uartclk / 16 / UART_DIV_MAX;
-               max = (port->uartclk + tolerance) / 16;
+               max = port->uartclk;
        }

Note:-
I have added below change on 5.15 kernel to test on all possible use cases.

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 7884fcd66d39..6d352981fb3e 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -643,6 +643,26 @@ static int dw8250_probe(struct platform_device *pdev)
                up->dma = &data->data.dma;
        }
 
+       if (data->pdata->quirks & DW_UART_QUIRK_IS_DMA_FC) {
+               /*
+                * When the 'char timeout' irq fires because no more data has
+                * been received in some time, the 8250 driver stops the DMA.
+                * However, if the DMAC has been setup to write more data to mem
+                * than is read from the UART FIFO, the data will *not* be
+                * written to memory.
+                * Therefore, we limit the width of writes to mem so that it is
+                * the same amount of data as read from the FIFO. You can use
+                * anything less than or equal, but same size is optimal
+                */
+               data->data.dma.rxconf.dst_addr_width = p->fifosize / 4;
+
+               /*
+                * Unless you set the maxburst to 1, if you send only 1 char, it
+                * doesn't get transmitted
+                */
+               data->data.dma.txconf.dst_maxburst = 1;
+       }
+

Cheers,
Biju

> -----Original Message-----
> From: Biju Das
> Sent: 04 July 2022 08:12
> To: Jiri Slaby <jirislaby@kernel.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>; Emil Renner Berthing <kernel@esmil.dk>; Phil
> Edworthy <phil.edworthy@renesas.com>; Johan Hovold <johan@kernel.org>;
> linux-serial@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Chris Paterson <Chris.Paterson2@renesas.com>;
> Biju Das <biju.das@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: RE: [PATCH] serial: 8250: dw: Fix the macro
> RZN1_UART_xDMACR_8_WORD_BURST
> 
> Hi Jiri,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> > RZN1_UART_xDMACR_8_WORD_BURST
> >
> > On 30. 06. 22, 10:39, Biju Das wrote:
> > > As per RZ/N1 peripheral user
> > > manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> > > rev 1.0.0 Mar,2019,
> >
> > Is this public anywhere?
> 
> Yes, It is available here[1] see page 72 and 73.
> 
> [1] https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-
> rzn1l-group-users-manual-peripherals?language=en&r=1054561
> 
> 
> >
> > > the value for 8_WORD_BURST is 4(b2,b1=2’b10).
> > >
> > > This patch fixes the macro as per the user manual.
> >
> > I'm curious, is the bottom bit from "3" ignored by the HW or does this
> > fix a real problem in behavior? Stating that might help backporters to
> > decide if to take the patch or not.
> 
> See page 72 and 73.
> 
> Yes, it fixes a real problem as by using a value of 8 , you are wrongly
> configuring DMA_BURST_SIZE of 1 instead of DMA_BURST_SIZE of 8.
> 
> b2, b1 bUart_DEST_BURST
> _SIZE
> DEST_BURST_SIZE
> Destination Burst Transaction Size in Transmit FIFO.
> UART is the flow controller. Thus, the user must write this field before
> or at the same time the DMA mode is enabled. Number of data byte, to be
> written to the Transmit FIFO every time a transmit burst transaction
> request are made on DMA request.
> 2’b00 = 1 byte
> 2’b01 = 4 bytes
> 2’b10 = 8 bytes
> 2’b11 = Reserved, not used
> 
> Cheers,
> Biju
> 
> 
> >
> > > Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow
> > > controlling devices")
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >   drivers/tty/serial/8250/8250_dw.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > > b/drivers/tty/serial/8250/8250_dw.c
> > > index f57bbd32ef11..931490b27d6b 100644
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -47,7 +47,7 @@
> > >   #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
> > >   #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
> > >   #define RZN1_UART_xDMACR_4_WORD_BURST	(1 << 1)
> > > -#define RZN1_UART_xDMACR_8_WORD_BURST	(3 << 1)
> > > +#define RZN1_UART_xDMACR_8_WORD_BURST	(2 << 1)
> > >   #define RZN1_UART_xDMACR_BLK_SZ(x)	((x) << 3)
> > >
> > >   /* Quirks */
> >
> > thanks,
> > --
> > js

Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Andy Shevchenko]

+Cc: Ilpo, the 8250_dw maintainer
+Cc: Serge, who I believe is the author of the lines in 8250_port you
cited, sorry if I'm mistaken.

On Tue, Jul 5, 2022 at 8:25 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Jiri and Miquel,
>
> While testing serial driver with RZ/N1 on 5.15 kernel, which is the backport of mainline kernel,
> I seen performance issue with serial DMA for higher baud rates.
>
> The test app is taking 25 minutes finish, whereas with the below patch[1] it takes only 3 minutes to finish.
>
> Not sure has anyone seen this performance issue?
>
> [1]
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 468d1aca5968..321430176698 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2680,7 +2680,7 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port,
>                 max = (port->uartclk + tolerance) / 4;
>         } else {
>                 min = port->uartclk / 16 / UART_DIV_MAX;
> -               max = (port->uartclk + tolerance) / 16;
> +               max = port->uartclk;
>         }
>
> Note:-
> I have added below change on 5.15 kernel to test on all possible use cases.
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 7884fcd66d39..6d352981fb3e 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -643,6 +643,26 @@ static int dw8250_probe(struct platform_device *pdev)
>                 up->dma = &data->data.dma;
>         }
>
> +       if (data->pdata->quirks & DW_UART_QUIRK_IS_DMA_FC) {
> +               /*
> +                * When the 'char timeout' irq fires because no more data has
> +                * been received in some time, the 8250 driver stops the DMA.
> +                * However, if the DMAC has been setup to write more data to mem
> +                * than is read from the UART FIFO, the data will *not* be
> +                * written to memory.
> +                * Therefore, we limit the width of writes to mem so that it is
> +                * the same amount of data as read from the FIFO. You can use
> +                * anything less than or equal, but same size is optimal
> +                */
> +               data->data.dma.rxconf.dst_addr_width = p->fifosize / 4;
> +
> +               /*
> +                * Unless you set the maxburst to 1, if you send only 1 char, it
> +                * doesn't get transmitted
> +                */
> +               data->data.dma.txconf.dst_maxburst = 1;
> +       }
> +
>
> Cheers,
> Biju
>
> > -----Original Message-----
> > From: Biju Das
> > Sent: 04 July 2022 08:12
> > To: Jiri Slaby <jirislaby@kernel.org>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Miquel Raynal
> > <miquel.raynal@bootlin.com>; Emil Renner Berthing <kernel@esmil.dk>; Phil
> > Edworthy <phil.edworthy@renesas.com>; Johan Hovold <johan@kernel.org>;
> > linux-serial@vger.kernel.org; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Chris Paterson <Chris.Paterson2@renesas.com>;
> > Biju Das <biju.das@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> > Subject: RE: [PATCH] serial: 8250: dw: Fix the macro
> > RZN1_UART_xDMACR_8_WORD_BURST
> >
> > Hi Jiri,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> > > RZN1_UART_xDMACR_8_WORD_BURST
> > >
> > > On 30. 06. 22, 10:39, Biju Das wrote:
> > > > As per RZ/N1 peripheral user
> > > > manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> > > > rev 1.0.0 Mar,2019,
> > >
> > > Is this public anywhere?
> >
> > Yes, It is available here[1] see page 72 and 73.
> >
> > [1] https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-
> > rzn1l-group-users-manual-peripherals?language=en&r=1054561
> >
> >
> > >
> > > > the value for 8_WORD_BURST is 4(b2,b1=2’b10).
> > > >
> > > > This patch fixes the macro as per the user manual.
> > >
> > > I'm curious, is the bottom bit from "3" ignored by the HW or does this
> > > fix a real problem in behavior? Stating that might help backporters to
> > > decide if to take the patch or not.
> >
> > See page 72 and 73.
> >
> > Yes, it fixes a real problem as by using a value of 8 , you are wrongly
> > configuring DMA_BURST_SIZE of 1 instead of DMA_BURST_SIZE of 8.
> >
> > b2, b1 bUart_DEST_BURST
> > _SIZE
> > DEST_BURST_SIZE
> > Destination Burst Transaction Size in Transmit FIFO.
> > UART is the flow controller. Thus, the user must write this field before
> > or at the same time the DMA mode is enabled. Number of data byte, to be
> > written to the Transmit FIFO every time a transmit burst transaction
> > request are made on DMA request.
> > 2’b00 = 1 byte
> > 2’b01 = 4 bytes
> > 2’b10 = 8 bytes
> > 2’b11 = Reserved, not used
> >
> > Cheers,
> > Biju
> >
> >
> > >
> > > > Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow
> > > > controlling devices")
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >   drivers/tty/serial/8250/8250_dw.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > > > b/drivers/tty/serial/8250/8250_dw.c
> > > > index f57bbd32ef11..931490b27d6b 100644
> > > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > > @@ -47,7 +47,7 @@
> > > >   #define RZN1_UART_xDMACR_DMA_EN         BIT(0)
> > > >   #define RZN1_UART_xDMACR_1_WORD_BURST   (0 << 1)
> > > >   #define RZN1_UART_xDMACR_4_WORD_BURST   (1 << 1)
> > > > -#define RZN1_UART_xDMACR_8_WORD_BURST    (3 << 1)
> > > > +#define RZN1_UART_xDMACR_8_WORD_BURST    (2 << 1)
> > > >   #define RZN1_UART_xDMACR_BLK_SZ(x)      ((x) << 3)
> > > >
> > > >   /* Quirks */
> > >
> > > thanks,
> > > --
> > > js



-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Biju Das]

Thanks Andy.

Please find the Test transfer size and baud rates in which this issue can be 
seen on RZ/N1 Platform. 

One thing I noticed is, because of that wrong calculation, the PLL baudrate divisor is set to higher value(128) instead of smaller one(13).

test_one () {
	local LENGTH=$1
	#echo "Length $1"
	${DIR}/uart-loopback -o ${TXUART} -i ${RXUART} -s ${LENGTH} -r
	if [ "$?" -ne "0" ]; then ERRORS=$((ERRORS+1)); fi
}

# Test transfers of lengths that typically throw problems
test_one_cfg () {
	local SPEED=$1
	./uart-baud/uart-baud ${TXUART} $SPEED
	./uart-baud/uart-baud ${RXUART} $SPEED

	for length in `seq 1 33`; do
		test_one ${length}
	done

	for length in `seq 2043 2053`; do
		test_one ${length}
	done

	for length in `seq 4091 4101`; do
		test_one ${length}
	done

	for length in `seq 8187 8297`; do
		test_one ${length}
	done

	for length in `seq 16379 16389`; do
		test_one ${length}
	done
}

# You need hardware flow control at this speed, otherwise you get overflow errors
echo "Parity: none Stop: 1 bit, 8 data bits, 'raw' mode, with RTS/CTS hardware flow control"
STTY_SETTING="raw -parenb -cstopb crtscts cs8 -echo -echonl -iexten"
stty -F ${TXUART} 9600 $STTY_SETTING
stty -F ${RXUART} 9600 $STTY_SETTING

for div in `seq 12 20`; do
	clk=$((1000000000 / $div))
	# Round up as the clk driver ensures the clock rate is below the requested value
	clk=$(( $clk + 1))

	# Max bitrate
	bitrate=$(( $clk / 16))
	echo "Bitrate: $bitrate"
	test_one_cfg $bitrate

	# Max bitrate / 2
	bitrate=$(( $bitrate / 2))
	echo ""
	echo "Bitrate: $bitrate"
	test_one_cfg $bitrate
done

Cheers,
Biju

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: 05 July 2022 10:30
> To: Biju Das <biju.das.jz@bp.renesas.com>; Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com>; Serge Semin <fancer.lancer@gmail.com>
> Cc: Jiri Slaby <jirislaby@kernel.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>; Emil Renner Berthing <kernel@esmil.dk>; Phil
> Edworthy <phil.edworthy@renesas.com>; Johan Hovold <johan@kernel.org>;
> linux-serial@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Chris Paterson <Chris.Paterson2@renesas.com>;
> Biju Das <biju.das@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> RZN1_UART_xDMACR_8_WORD_BURST
> 
> +Cc: Ilpo, the 8250_dw maintainer
> +Cc: Serge, who I believe is the author of the lines in 8250_port you
> cited, sorry if I'm mistaken.
> 
> On Tue, Jul 5, 2022 at 8:25 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> >
> > Hi Jiri and Miquel,
> >
> > While testing serial driver with RZ/N1 on 5.15 kernel, which is the
> > backport of mainline kernel, I seen performance issue with serial DMA
> for higher baud rates.
> >
> > The test app is taking 25 minutes finish, whereas with the below
> patch[1] it takes only 3 minutes to finish.
> >
> > Not sure has anyone seen this performance issue?
> >
> > [1]
> > diff --git a/drivers/tty/serial/8250/8250_port.c
> > b/drivers/tty/serial/8250/8250_port.c
> > index 468d1aca5968..321430176698 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -2680,7 +2680,7 @@ static unsigned int
> serial8250_get_baud_rate(struct uart_port *port,
> >                 max = (port->uartclk + tolerance) / 4;
> >         } else {
> >                 min = port->uartclk / 16 / UART_DIV_MAX;
> > -               max = (port->uartclk + tolerance) / 16;
> > +               max = port->uartclk;
> >         }
> >
> > Note:-
> > I have added below change on 5.15 kernel to test on all possible use
> cases.
> >
> > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > b/drivers/tty/serial/8250/8250_dw.c
> > index 7884fcd66d39..6d352981fb3e 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -643,6 +643,26 @@ static int dw8250_probe(struct platform_device
> *pdev)
> >                 up->dma = &data->data.dma;
> >         }
> >
> > +       if (data->pdata->quirks & DW_UART_QUIRK_IS_DMA_FC) {
> > +               /*
> > +                * When the 'char timeout' irq fires because no more
> data has
> > +                * been received in some time, the 8250 driver stops
> the DMA.
> > +                * However, if the DMAC has been setup to write more
> data to mem
> > +                * than is read from the UART FIFO, the data will *not*
> be
> > +                * written to memory.
> > +                * Therefore, we limit the width of writes to mem so
> that it is
> > +                * the same amount of data as read from the FIFO. You
> can use
> > +                * anything less than or equal, but same size is
> optimal
> > +                */
> > +               data->data.dma.rxconf.dst_addr_width = p->fifosize /
> > + 4;
> > +
> > +               /*
> > +                * Unless you set the maxburst to 1, if you send only 1
> char, it
> > +                * doesn't get transmitted
> > +                */
> > +               data->data.dma.txconf.dst_maxburst = 1;
> > +       }
> > +
> >
> > Cheers,
> > Biju
> >
> > > -----Original Message-----
> > > From: Biju Das
> > > Sent: 04 July 2022 08:12
> > > To: Jiri Slaby <jirislaby@kernel.org>; Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Miquel
> > > Raynal <miquel.raynal@bootlin.com>; Emil Renner Berthing
> > > <kernel@esmil.dk>; Phil Edworthy <phil.edworthy@renesas.com>; Johan
> > > Hovold <johan@kernel.org>; linux-serial@vger.kernel.org; Geert
> > > Uytterhoeven <geert+renesas@glider.be>; Chris Paterson
> > > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>;
> > > linux-renesas-soc@vger.kernel.org
> > > Subject: RE: [PATCH] serial: 8250: dw: Fix the macro
> > > RZN1_UART_xDMACR_8_WORD_BURST
> > >
> > > Hi Jiri,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> > > > RZN1_UART_xDMACR_8_WORD_BURST
> > > >
> > > > On 30. 06. 22, 10:39, Biju Das wrote:
> > > > > As per RZ/N1 peripheral user
> > > > > manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> > > > > rev 1.0.0 Mar,2019,
> > > >
> > > > Is this public anywhere?
> > >
> > > Yes, It is available here[1] see page 72 and 73.
> > >
> > > [1]
> > > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-
> > > rzn1l-group-users-manual-peripherals?language=en&r=1054561
> > >
> > >
> > > >
> > > > > the value for 8_WORD_BURST is 4(b2,b1=2'b10).
> > > > >
> > > > > This patch fixes the macro as per the user manual.
> > > >
> > > > I'm curious, is the bottom bit from "3" ignored by the HW or does
> > > > this fix a real problem in behavior? Stating that might help
> > > > backporters to decide if to take the patch or not.
> > >
> > > See page 72 and 73.
> > >
> > > Yes, it fixes a real problem as by using a value of 8 , you are
> > > wrongly configuring DMA_BURST_SIZE of 1 instead of DMA_BURST_SIZE of
> 8.
> > >
> > > b2, b1 bUart_DEST_BURST
> > > _SIZE
> > > DEST_BURST_SIZE
> > > Destination Burst Transaction Size in Transmit FIFO.
> > > UART is the flow controller. Thus, the user must write this field
> > > before or at the same time the DMA mode is enabled. Number of data
> > > byte, to be written to the Transmit FIFO every time a transmit burst
> > > transaction request are made on DMA request.
> > > 2'b00 = 1 byte
> > > 2'b01 = 4 bytes
> > > 2'b10 = 8 bytes
> > > 2'b11 = Reserved, not used
> > >
> > > Cheers,
> > > Biju
> > >
> > >
> > > >
> > > > > Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow
> > > > > controlling devices")
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > >   drivers/tty/serial/8250/8250_dw.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > > > > b/drivers/tty/serial/8250/8250_dw.c
> > > > > index f57bbd32ef11..931490b27d6b 100644
> > > > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > > > @@ -47,7 +47,7 @@
> > > > >   #define RZN1_UART_xDMACR_DMA_EN         BIT(0)
> > > > >   #define RZN1_UART_xDMACR_1_WORD_BURST   (0 << 1)
> > > > >   #define RZN1_UART_xDMACR_4_WORD_BURST   (1 << 1)
> > > > > -#define RZN1_UART_xDMACR_8_WORD_BURST    (3 << 1)
> > > > > +#define RZN1_UART_xDMACR_8_WORD_BURST    (2 << 1)
> > > > >   #define RZN1_UART_xDMACR_BLK_SZ(x)      ((x) << 3)
> > > > >
> > > > >   /* Quirks */
> > > >
> > > > thanks,
> > > > --
> > > > js
> 
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Serge Semin]

Hi Andy,

On Tue, Jul 05, 2022 at 11:30:01AM +0200, Andy Shevchenko wrote:
> +Cc: Ilpo, the 8250_dw maintainer

> +Cc: Serge, who I believe is the author of the lines in 8250_port you
> cited, sorry if I'm mistaken.

Right, I was the one who got back the line with the proper max baud rate
calculation procedure in commit 7b668c064ec3 ("serial: 8250: Fix max
baud limit in generic 8250 port"). In accordance with [1, 2] the
interface baud rate is (DIV*16)-th of the reference clock frequency. So
the patch suggested by Biju will work only until he gets to the zero
divisor value. Without my fix the baud-rate search algorithm may
end up with getting unsupported baud-rates causing to have
zero-divisor, which will lead to the serial interface freeze/disable
[2].

[1] DesignWare DW_apb_uart Databook 4.03a, December 2020, p. 24
[2] DesignWare DW_apb_uart Databook 4.03a, December 2020, p. 125

> 
> On Tue, Jul 5, 2022 at 8:25 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Hi Jiri and Miquel,
> >
> > While testing serial driver with RZ/N1 on 5.15 kernel, which is the backport of mainline kernel,
> > I seen performance issue with serial DMA for higher baud rates.
> >
> > The test app is taking 25 minutes finish, whereas with the below patch[1] it takes only 3 minutes to finish.
> >
> > Not sure has anyone seen this performance issue?
> >
> > [1]
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 468d1aca5968..321430176698 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -2680,7 +2680,7 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port,
> >                 max = (port->uartclk + tolerance) / 4;
> >         } else {
> >                 min = port->uartclk / 16 / UART_DIV_MAX;

> > -               max = (port->uartclk + tolerance) / 16;
> > +               max = port->uartclk;

Are you sure uartclk is initialized with a real reference clock value?

-Sergey

> >         }
> >
> > Note:-
> > I have added below change on 5.15 kernel to test on all possible use cases.
> >
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index 7884fcd66d39..6d352981fb3e 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -643,6 +643,26 @@ static int dw8250_probe(struct platform_device *pdev)
> >                 up->dma = &data->data.dma;
> >         }
> >
> > +       if (data->pdata->quirks & DW_UART_QUIRK_IS_DMA_FC) {
> > +               /*
> > +                * When the 'char timeout' irq fires because no more data has
> > +                * been received in some time, the 8250 driver stops the DMA.
> > +                * However, if the DMAC has been setup to write more data to mem
> > +                * than is read from the UART FIFO, the data will *not* be
> > +                * written to memory.
> > +                * Therefore, we limit the width of writes to mem so that it is
> > +                * the same amount of data as read from the FIFO. You can use
> > +                * anything less than or equal, but same size is optimal
> > +                */
> > +               data->data.dma.rxconf.dst_addr_width = p->fifosize / 4;
> > +
> > +               /*
> > +                * Unless you set the maxburst to 1, if you send only 1 char, it
> > +                * doesn't get transmitted
> > +                */
> > +               data->data.dma.txconf.dst_maxburst = 1;
> > +       }
> > +
> >
> > Cheers,
> > Biju
> >
> > > -----Original Message-----
> > > From: Biju Das
> > > Sent: 04 July 2022 08:12
> > > To: Jiri Slaby <jirislaby@kernel.org>; Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Miquel Raynal
> > > <miquel.raynal@bootlin.com>; Emil Renner Berthing <kernel@esmil.dk>; Phil
> > > Edworthy <phil.edworthy@renesas.com>; Johan Hovold <johan@kernel.org>;
> > > linux-serial@vger.kernel.org; Geert Uytterhoeven
> > > <geert+renesas@glider.be>; Chris Paterson <Chris.Paterson2@renesas.com>;
> > > Biju Das <biju.das@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> > > Subject: RE: [PATCH] serial: 8250: dw: Fix the macro
> > > RZN1_UART_xDMACR_8_WORD_BURST
> > >
> > > Hi Jiri,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> > > > RZN1_UART_xDMACR_8_WORD_BURST
> > > >
> > > > On 30. 06. 22, 10:39, Biju Das wrote:
> > > > > As per RZ/N1 peripheral user
> > > > > manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> > > > > rev 1.0.0 Mar,2019,
> > > >
> > > > Is this public anywhere?
> > >
> > > Yes, It is available here[1] see page 72 and 73.
> > >
> > > [1] https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-
> > > rzn1l-group-users-manual-peripherals?language=en&r=1054561
> > >
> > >
> > > >
> > > > > the value for 8_WORD_BURST is 4(b2,b1=2’b10).
> > > > >
> > > > > This patch fixes the macro as per the user manual.
> > > >
> > > > I'm curious, is the bottom bit from "3" ignored by the HW or does this
> > > > fix a real problem in behavior? Stating that might help backporters to
> > > > decide if to take the patch or not.
> > >
> > > See page 72 and 73.
> > >
> > > Yes, it fixes a real problem as by using a value of 8 , you are wrongly
> > > configuring DMA_BURST_SIZE of 1 instead of DMA_BURST_SIZE of 8.
> > >
> > > b2, b1 bUart_DEST_BURST
> > > _SIZE
> > > DEST_BURST_SIZE
> > > Destination Burst Transaction Size in Transmit FIFO.
> > > UART is the flow controller. Thus, the user must write this field before
> > > or at the same time the DMA mode is enabled. Number of data byte, to be
> > > written to the Transmit FIFO every time a transmit burst transaction
> > > request are made on DMA request.
> > > 2’b00 = 1 byte
> > > 2’b01 = 4 bytes
> > > 2’b10 = 8 bytes
> > > 2’b11 = Reserved, not used
> > >
> > > Cheers,
> > > Biju
> > >
> > >
> > > >
> > > > > Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow
> > > > > controlling devices")
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > >   drivers/tty/serial/8250/8250_dw.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > > > > b/drivers/tty/serial/8250/8250_dw.c
> > > > > index f57bbd32ef11..931490b27d6b 100644
> > > > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > > > @@ -47,7 +47,7 @@
> > > > >   #define RZN1_UART_xDMACR_DMA_EN         BIT(0)
> > > > >   #define RZN1_UART_xDMACR_1_WORD_BURST   (0 << 1)
> > > > >   #define RZN1_UART_xDMACR_4_WORD_BURST   (1 << 1)
> > > > > -#define RZN1_UART_xDMACR_8_WORD_BURST    (3 << 1)
> > > > > +#define RZN1_UART_xDMACR_8_WORD_BURST    (2 << 1)
> > > > >   #define RZN1_UART_xDMACR_BLK_SZ(x)      ((x) << 3)
> > > > >
> > > > >   /* Quirks */
> > > >
> > > > thanks,
> > > > --
> > > > js
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Biju Das]

Hi Serge Semin,

Thanks for the feedback.

> Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> RZN1_UART_xDMACR_8_WORD_BURST
> 
> Hi Andy,
> 
> On Tue, Jul 05, 2022 at 11:30:01AM +0200, Andy Shevchenko wrote:
> > +Cc: Ilpo, the 8250_dw maintainer
> 
> > +Cc: Serge, who I believe is the author of the lines in 8250_port you
> > cited, sorry if I'm mistaken.
> 
> Right, I was the one who got back the line with the proper max baud rate
> calculation procedure in commit 7b668c064ec3 ("serial: 8250: Fix max baud
> limit in generic 8250 port"). In accordance with [1, 2] the interface
> baud rate is (DIV*16)-th of the reference clock frequency. So the patch
> suggested by Biju will work only until he gets to the zero divisor value.
> Without my fix the baud-rate search algorithm may end up with getting
> unsupported baud-rates causing to have zero-divisor, which will lead to
> the serial interface freeze/disable [2].
> 
> [1] DesignWare DW_apb_uart Databook 4.03a, December 2020, p. 24 [2]
> DesignWare DW_apb_uart Databook 4.03a, December 2020, p. 125

Thanks for sharing the details.

> 
> >
> > On Tue, Jul 5, 2022 at 8:25 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > >
> > > Hi Jiri and Miquel,
> > >
> > > While testing serial driver with RZ/N1 on 5.15 kernel, which is the
> > > backport of mainline kernel, I seen performance issue with serial DMA
> for higher baud rates.
> > >
> > > The test app is taking 25 minutes finish, whereas with the below
> patch[1] it takes only 3 minutes to finish.
> > >
> > > Not sure has anyone seen this performance issue?
> > >
> > > [1]
> > > diff --git a/drivers/tty/serial/8250/8250_port.c
> > > b/drivers/tty/serial/8250/8250_port.c
> > > index 468d1aca5968..321430176698 100644
> > > --- a/drivers/tty/serial/8250/8250_port.c
> > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > @@ -2680,7 +2680,7 @@ static unsigned int
> serial8250_get_baud_rate(struct uart_port *port,
> > >                 max = (port->uartclk + tolerance) / 4;
> > >         } else {
> > >                 min = port->uartclk / 16 / UART_DIV_MAX;
> 
> > > -               max = (port->uartclk + tolerance) / 16;
> > > +               max = port->uartclk;
> 
> Are you sure uartclk is initialized with a real reference clock value?

Looks like your code is correct.

The reason for performance issue is because of the out of range and it defaults to 9600 instead of the nearest possible max baud value. The requested baud rate(for eg:-5M), is higher than max possible baud rate(4.9M), because of the rounding of the clocks and uart baud calculation defaults to 9600.

We have PLL(1 GHz)->PLL Divider(12to 128)-> UARTCLK
Currently clk driver uses default divider value of 21 which set by the
Bootloader and it doesn't allow to change this as it has other users.

With this max possible baud rate is (1G/21)/16=2.97M

If we want to test baudrate in the order of 5M(1G/12)/16= 83MHz/16 = 5.2M , we need to remove the hack from Clk driver[1]
[1] https://elixir.bootlin.com/linux/v5.19-rc5/source/drivers/clk/renesas/r9a06g032-clocks.c#L658

But the clk driver finds best divider as 13 instead of 12 and reduces the 
Rate from 83MHz to 76.9MHz.

With 76.9Mhz, max possible baud rate is ~4.8M as per your calculation. The original requested baud rate 5.2M is now out of bound and baud rate is defaulted to Very low value say 9600, this results in the performance issue.

Cheers,
Biju


> 
> -Sergey
> 
> > >         }
> > >
> > > Note:-
> > > I have added below change on 5.15 kernel to test on all possible use
> cases.
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > > b/drivers/tty/serial/8250/8250_dw.c
> > > index 7884fcd66d39..6d352981fb3e 100644
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -643,6 +643,26 @@ static int dw8250_probe(struct platform_device
> *pdev)
> > >                 up->dma = &data->data.dma;
> > >         }
> > >
> > > +       if (data->pdata->quirks & DW_UART_QUIRK_IS_DMA_FC) {
> > > +               /*
> > > +                * When the 'char timeout' irq fires because no more
> data has
> > > +                * been received in some time, the 8250 driver stops
> the DMA.
> > > +                * However, if the DMAC has been setup to write more
> data to mem
> > > +                * than is read from the UART FIFO, the data will
> *not* be
> > > +                * written to memory.
> > > +                * Therefore, we limit the width of writes to mem so
> that it is
> > > +                * the same amount of data as read from the FIFO. You
> can use
> > > +                * anything less than or equal, but same size is
> optimal
> > > +                */
> > > +               data->data.dma.rxconf.dst_addr_width = p->fifosize /
> > > + 4;
> > > +
> > > +               /*
> > > +                * Unless you set the maxburst to 1, if you send only
> 1 char, it
> > > +                * doesn't get transmitted
> > > +                */
> > > +               data->data.dma.txconf.dst_maxburst = 1;
> > > +       }
> > > +
> > >
> > > Cheers,
> > > Biju
> > >
> > > > -----Original Message-----
> > > > From: Biju Das
> > > > Sent: 04 July 2022 08:12
> > > > To: Jiri Slaby <jirislaby@kernel.org>; Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org>
> > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Miquel
> > > > Raynal <miquel.raynal@bootlin.com>; Emil Renner Berthing
> > > > <kernel@esmil.dk>; Phil Edworthy <phil.edworthy@renesas.com>;
> > > > Johan Hovold <johan@kernel.org>; linux-serial@vger.kernel.org;
> > > > Geert Uytterhoeven <geert+renesas@glider.be>; Chris Paterson
> > > > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>;
> > > > linux-renesas-soc@vger.kernel.org
> > > > Subject: RE: [PATCH] serial: 8250: dw: Fix the macro
> > > > RZN1_UART_xDMACR_8_WORD_BURST
> > > >
> > > > Hi Jiri,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> > > > > RZN1_UART_xDMACR_8_WORD_BURST
> > > > >
> > > > > On 30. 06. 22, 10:39, Biju Das wrote:
> > > > > > As per RZ/N1 peripheral user
> > > > > > manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> > > > > > rev 1.0.0 Mar,2019,
> > > > >
> > > > > Is this public anywhere?
> > > >
> > > > Yes, It is available here[1] see page 72 and 73.
> > > >
> > > > [1]
> > > > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group
> > > > -
> > > > rzn1l-group-users-manual-peripherals?language=en&r=1054561
> > > >
> > > >
> > > > >
> > > > > > the value for 8_WORD_BURST is 4(b2,b1=2'b10).
> > > > > >
> > > > > > This patch fixes the macro as per the user manual.
> > > > >
> > > > > I'm curious, is the bottom bit from "3" ignored by the HW or
> > > > > does this fix a real problem in behavior? Stating that might
> > > > > help backporters to decide if to take the patch or not.
> > > >
> > > > See page 72 and 73.
> > > >
> > > > Yes, it fixes a real problem as by using a value of 8 , you are
> > > > wrongly configuring DMA_BURST_SIZE of 1 instead of DMA_BURST_SIZE
> of 8.
> > > >
> > > > b2, b1 bUart_DEST_BURST
> > > > _SIZE
> > > > DEST_BURST_SIZE
> > > > Destination Burst Transaction Size in Transmit FIFO.
> > > > UART is the flow controller. Thus, the user must write this field
> > > > before or at the same time the DMA mode is enabled. Number of data
> > > > byte, to be written to the Transmit FIFO every time a transmit
> > > > burst transaction request are made on DMA request.
> > > > 2'b00 = 1 byte
> > > > 2'b01 = 4 bytes
> > > > 2'b10 = 8 bytes
> > > > 2'b11 = Reserved, not used
> > > >
> > > > Cheers,
> > > > Biju
> > > >
> > > >
> > > > >
> > > > > > Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA
> > > > > > flow controlling devices")
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > ---
> > > > > >   drivers/tty/serial/8250/8250_dw.c | 2 +-
> > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > > > > > b/drivers/tty/serial/8250/8250_dw.c
> > > > > > index f57bbd32ef11..931490b27d6b 100644
> > > > > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > > > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > > > > @@ -47,7 +47,7 @@
> > > > > >   #define RZN1_UART_xDMACR_DMA_EN         BIT(0)
> > > > > >   #define RZN1_UART_xDMACR_1_WORD_BURST   (0 << 1)
> > > > > >   #define RZN1_UART_xDMACR_4_WORD_BURST   (1 << 1)
> > > > > > -#define RZN1_UART_xDMACR_8_WORD_BURST    (3 << 1)
> > > > > > +#define RZN1_UART_xDMACR_8_WORD_BURST    (2 << 1)
> > > > > >   #define RZN1_UART_xDMACR_BLK_SZ(x)      ((x) << 3)
> > > > > >
> > > > > >   /* Quirks */
> > > > >
> > > > > thanks,
> > > > > --
> > > > > js
> >
> >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko

Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST

[Serge Semin]

On Sun, Jul 10, 2022 at 08:46:46AM +0000, Biju Das wrote:
> Hi Serge Semin,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> > RZN1_UART_xDMACR_8_WORD_BURST
> > 
> > Hi Andy,
> > 
> > On Tue, Jul 05, 2022 at 11:30:01AM +0200, Andy Shevchenko wrote:
> > > +Cc: Ilpo, the 8250_dw maintainer
> > 
> > > +Cc: Serge, who I believe is the author of the lines in 8250_port you
> > > cited, sorry if I'm mistaken.
> > 
> > Right, I was the one who got back the line with the proper max baud rate
> > calculation procedure in commit 7b668c064ec3 ("serial: 8250: Fix max baud
> > limit in generic 8250 port"). In accordance with [1, 2] the interface
> > baud rate is (DIV*16)-th of the reference clock frequency. So the patch
> > suggested by Biju will work only until he gets to the zero divisor value.
> > Without my fix the baud-rate search algorithm may end up with getting
> > unsupported baud-rates causing to have zero-divisor, which will lead to
> > the serial interface freeze/disable [2].
> > 
> > [1] DesignWare DW_apb_uart Databook 4.03a, December 2020, p. 24 [2]
> > DesignWare DW_apb_uart Databook 4.03a, December 2020, p. 125
> 
> Thanks for sharing the details.
> 
> > 
> > >
> > > On Tue, Jul 5, 2022 at 8:25 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > >
> > > > Hi Jiri and Miquel,
> > > >
> > > > While testing serial driver with RZ/N1 on 5.15 kernel, which is the
> > > > backport of mainline kernel, I seen performance issue with serial DMA
> > for higher baud rates.
> > > >
> > > > The test app is taking 25 minutes finish, whereas with the below
> > patch[1] it takes only 3 minutes to finish.
> > > >
> > > > Not sure has anyone seen this performance issue?
> > > >
> > > > [1]
> > > > diff --git a/drivers/tty/serial/8250/8250_port.c
> > > > b/drivers/tty/serial/8250/8250_port.c
> > > > index 468d1aca5968..321430176698 100644
> > > > --- a/drivers/tty/serial/8250/8250_port.c
> > > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > > @@ -2680,7 +2680,7 @@ static unsigned int
> > serial8250_get_baud_rate(struct uart_port *port,
> > > >                 max = (port->uartclk + tolerance) / 4;
> > > >         } else {
> > > >                 min = port->uartclk / 16 / UART_DIV_MAX;
> > 
> > > > -               max = (port->uartclk + tolerance) / 16;
> > > > +               max = port->uartclk;
> > 
> > Are you sure uartclk is initialized with a real reference clock value?
> 
> Looks like your code is correct.
> 
> The reason for performance issue is because of the out of range and it defaults to 9600 instead of the nearest possible max baud value. The requested baud rate(for eg:-5M), is higher than max possible baud rate(4.9M), because of the rounding of the clocks and uart baud calculation defaults to 9600.
> 
> We have PLL(1 GHz)->PLL Divider(12to 128)-> UARTCLK
> Currently clk driver uses default divider value of 21 which set by the
> Bootloader and it doesn't allow to change this as it has other users.
> 
> With this max possible baud rate is (1G/21)/16=2.97M
> 

> If we want to test baudrate in the order of 5M(1G/12)/16= 83MHz/16 = 5.2M , we need to remove the hack from Clk driver[1]
> [1] https://elixir.bootlin.com/linux/v5.19-rc5/source/drivers/clk/renesas/r9a06g032-clocks.c#L658
> 
> But the clk driver finds best divider as 13 instead of 12 and reduces the 
> Rate from 83MHz to 76.9MHz.

Hmm, the same approach has been implemented on our SoC too. There
are two DW APB UARTs with common reference clock attached. That's why
I've introduced the commit cc816969d7b5 ("serial: 8250_dw: Fix common
clocks usage race condition"). It provides a way to update the clock
divider of the uart interface in case if the reference clock rate has
changed. The implemented approach has some drawbacks though:
1. If the divider gets out of range, then the affected interfaces will
fallback to 9600.
2. For some time between the clock rate change and until the divider is
updated the affected UART interfaces will work with the undetermined baud
rate.

So to speak, if you don't mind about the drawbacks above you can at
least give it a try. Fixing the clock rate is a good alternative though.

-Sergey

> 
> With 76.9Mhz, max possible baud rate is ~4.8M as per your calculation. The original requested baud rate 5.2M is now out of bound and baud rate is defaulted to Very low value say 9600, this results in the performance issue.
> 
> Cheers,
> Biju
> 
> 
> > 
> > -Sergey
> > 
> > > >         }
> > > >
> > > > Note:-
> > > > I have added below change on 5.15 kernel to test on all possible use
> > cases.
> > > >
> > > > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > > > b/drivers/tty/serial/8250/8250_dw.c
> > > > index 7884fcd66d39..6d352981fb3e 100644
> > > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > > @@ -643,6 +643,26 @@ static int dw8250_probe(struct platform_device
> > *pdev)
> > > >                 up->dma = &data->data.dma;
> > > >         }
> > > >
> > > > +       if (data->pdata->quirks & DW_UART_QUIRK_IS_DMA_FC) {
> > > > +               /*
> > > > +                * When the 'char timeout' irq fires because no more
> > data has
> > > > +                * been received in some time, the 8250 driver stops
> > the DMA.
> > > > +                * However, if the DMAC has been setup to write more
> > data to mem
> > > > +                * than is read from the UART FIFO, the data will
> > *not* be
> > > > +                * written to memory.
> > > > +                * Therefore, we limit the width of writes to mem so
> > that it is
> > > > +                * the same amount of data as read from the FIFO. You
> > can use
> > > > +                * anything less than or equal, but same size is
> > optimal
> > > > +                */
> > > > +               data->data.dma.rxconf.dst_addr_width = p->fifosize /
> > > > + 4;
> > > > +
> > > > +               /*
> > > > +                * Unless you set the maxburst to 1, if you send only
> > 1 char, it
> > > > +                * doesn't get transmitted
> > > > +                */
> > > > +               data->data.dma.txconf.dst_maxburst = 1;
> > > > +       }
> > > > +
> > > >
> > > > Cheers,
> > > > Biju
> > > >
> > > > > -----Original Message-----
> > > > > From: Biju Das
> > > > > Sent: 04 July 2022 08:12
> > > > > To: Jiri Slaby <jirislaby@kernel.org>; Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org>
> > > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Miquel
> > > > > Raynal <miquel.raynal@bootlin.com>; Emil Renner Berthing
> > > > > <kernel@esmil.dk>; Phil Edworthy <phil.edworthy@renesas.com>;
> > > > > Johan Hovold <johan@kernel.org>; linux-serial@vger.kernel.org;
> > > > > Geert Uytterhoeven <geert+renesas@glider.be>; Chris Paterson
> > > > > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>;
> > > > > linux-renesas-soc@vger.kernel.org
> > > > > Subject: RE: [PATCH] serial: 8250: dw: Fix the macro
> > > > > RZN1_UART_xDMACR_8_WORD_BURST
> > > > >
> > > > > Hi Jiri,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH] serial: 8250: dw: Fix the macro
> > > > > > RZN1_UART_xDMACR_8_WORD_BURST
> > > > > >
> > > > > > On 30. 06. 22, 10:39, Biju Das wrote:
> > > > > > > As per RZ/N1 peripheral user
> > > > > > > manual(r01uh0752ej0100-rzn1-peripheral.pdf)
> > > > > > > rev 1.0.0 Mar,2019,
> > > > > >
> > > > > > Is this public anywhere?
> > > > >
> > > > > Yes, It is available here[1] see page 72 and 73.
> > > > >
> > > > > [1]
> > > > > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group
> > > > > -
> > > > > rzn1l-group-users-manual-peripherals?language=en&r=1054561
> > > > >
> > > > >
> > > > > >
> > > > > > > the value for 8_WORD_BURST is 4(b2,b1=2'b10).
> > > > > > >
> > > > > > > This patch fixes the macro as per the user manual.
> > > > > >
> > > > > > I'm curious, is the bottom bit from "3" ignored by the HW or
> > > > > > does this fix a real problem in behavior? Stating that might
> > > > > > help backporters to decide if to take the patch or not.
> > > > >
> > > > > See page 72 and 73.
> > > > >
> > > > > Yes, it fixes a real problem as by using a value of 8 , you are
> > > > > wrongly configuring DMA_BURST_SIZE of 1 instead of DMA_BURST_SIZE
> > of 8.
> > > > >
> > > > > b2, b1 bUart_DEST_BURST
> > > > > _SIZE
> > > > > DEST_BURST_SIZE
> > > > > Destination Burst Transaction Size in Transmit FIFO.
> > > > > UART is the flow controller. Thus, the user must write this field
> > > > > before or at the same time the DMA mode is enabled. Number of data
> > > > > byte, to be written to the Transmit FIFO every time a transmit
> > > > > burst transaction request are made on DMA request.
> > > > > 2'b00 = 1 byte
> > > > > 2'b01 = 4 bytes
> > > > > 2'b10 = 8 bytes
> > > > > 2'b11 = Reserved, not used
> > > > >
> > > > > Cheers,
> > > > > Biju
> > > > >
> > > > >
> > > > > >
> > > > > > > Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA
> > > > > > > flow controlling devices")
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > ---
> > > > > > >   drivers/tty/serial/8250/8250_dw.c | 2 +-
> > > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > > > > > > b/drivers/tty/serial/8250/8250_dw.c
> > > > > > > index f57bbd32ef11..931490b27d6b 100644
> > > > > > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > > > > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > > > > > @@ -47,7 +47,7 @@
> > > > > > >   #define RZN1_UART_xDMACR_DMA_EN         BIT(0)
> > > > > > >   #define RZN1_UART_xDMACR_1_WORD_BURST   (0 << 1)
> > > > > > >   #define RZN1_UART_xDMACR_4_WORD_BURST   (1 << 1)
> > > > > > > -#define RZN1_UART_xDMACR_8_WORD_BURST    (3 << 1)
> > > > > > > +#define RZN1_UART_xDMACR_8_WORD_BURST    (2 << 1)
> > > > > > >   #define RZN1_UART_xDMACR_BLK_SZ(x)      ((x) << 3)
> > > > > > >
> > > > > > >   /* Quirks */
> > > > > >
> > > > > > thanks,
> > > > > > --
> > > > > > js
> > >
> > >
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko