* [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST @ 2022-06-30 8:39 Biju Das 2022-06-30 9:36 ` Phil Edworthy 2022-07-04 6:35 ` Jiri Slaby 0 siblings, 2 replies; 10+ messages in thread From: Biju Das @ 2022-06-30 8:39 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Biju Das, Andy Shevchenko, Jiri Slaby, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-06-30 8:39 [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST Biju Das @ 2022-06-30 9:36 ` Phil Edworthy 2022-07-04 6:35 ` Jiri Slaby 1 sibling, 0 replies; 10+ messages in thread From: Phil Edworthy @ 2022-06-30 9:36 UTC (permalink / raw) To: Biju Das, Greg Kroah-Hartman Cc: Biju Das, Andy Shevchenko, Jiri Slaby, Miquel Raynal, Emil Renner Berthing, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-06-30 8:39 [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST Biju Das 2022-06-30 9:36 ` Phil Edworthy @ 2022-07-04 6:35 ` Jiri Slaby 2022-07-04 7:12 ` Biju Das 1 sibling, 1 reply; 10+ messages in thread From: Jiri Slaby @ 2022-07-04 6:35 UTC (permalink / raw) To: Biju Das, Greg Kroah-Hartman Cc: Andy Shevchenko, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-07-04 6:35 ` Jiri Slaby @ 2022-07-04 7:12 ` Biju Das 2022-07-05 6:23 ` Biju Das 0 siblings, 1 reply; 10+ messages in thread From: Biju Das @ 2022-07-04 7:12 UTC (permalink / raw) To: Jiri Slaby, Greg Kroah-Hartman Cc: Andy Shevchenko, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-07-04 7:12 ` Biju Das @ 2022-07-05 6:23 ` Biju Das 2022-07-05 9:30 ` Andy Shevchenko 0 siblings, 1 reply; 10+ messages in thread From: Biju Das @ 2022-07-05 6:23 UTC (permalink / raw) To: Jiri Slaby, Greg Kroah-Hartman Cc: Andy Shevchenko, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-07-05 6:23 ` Biju Das @ 2022-07-05 9:30 ` Andy Shevchenko 2022-07-05 13:30 ` Biju Das 2022-07-05 16:31 ` Serge Semin 0 siblings, 2 replies; 10+ messages in thread From: Andy Shevchenko @ 2022-07-05 9:30 UTC (permalink / raw) To: Biju Das, Ilpo Järvinen, Serge Semin Cc: Jiri Slaby, Greg Kroah-Hartman, Andy Shevchenko, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc +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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-07-05 9:30 ` Andy Shevchenko @ 2022-07-05 13:30 ` Biju Das 2022-07-05 16:31 ` Serge Semin 1 sibling, 0 replies; 10+ messages in thread From: Biju Das @ 2022-07-05 13:30 UTC (permalink / raw) To: Andy Shevchenko, Ilpo Järvinen, Serge Semin Cc: Jiri Slaby, Greg Kroah-Hartman, Andy Shevchenko, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-07-05 9:30 ` Andy Shevchenko 2022-07-05 13:30 ` Biju Das @ 2022-07-05 16:31 ` Serge Semin 2022-07-10 8:46 ` Biju Das 1 sibling, 1 reply; 10+ messages in thread From: Serge Semin @ 2022-07-05 16:31 UTC (permalink / raw) To: Andy Shevchenko Cc: Biju Das, Ilpo Järvinen, Jiri Slaby, Greg Kroah-Hartman, Andy Shevchenko, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-07-05 16:31 ` Serge Semin @ 2022-07-10 8:46 ` Biju Das 2022-07-11 11:02 ` Serge Semin 0 siblings, 1 reply; 10+ messages in thread From: Biju Das @ 2022-07-10 8:46 UTC (permalink / raw) To: Serge Semin, Andy Shevchenko Cc: Ilpo Järvinen, Jiri Slaby, Greg Kroah-Hartman, Andy Shevchenko, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST 2022-07-10 8:46 ` Biju Das @ 2022-07-11 11:02 ` Serge Semin 0 siblings, 0 replies; 10+ messages in thread From: Serge Semin @ 2022-07-11 11:02 UTC (permalink / raw) To: Biju Das Cc: Andy Shevchenko, Ilpo Järvinen, Jiri Slaby, Greg Kroah-Hartman, Andy Shevchenko, Miquel Raynal, Emil Renner Berthing, Phil Edworthy, Johan Hovold, linux-serial, Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-07-11 11:27 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-30 8:39 [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST Biju Das 2022-06-30 9:36 ` Phil Edworthy 2022-07-04 6:35 ` Jiri Slaby 2022-07-04 7:12 ` Biju Das 2022-07-05 6:23 ` Biju Das 2022-07-05 9:30 ` Andy Shevchenko 2022-07-05 13:30 ` Biju Das 2022-07-05 16:31 ` Serge Semin 2022-07-10 8:46 ` Biju Das 2022-07-11 11:02 ` Serge Semin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.