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@xxxxxxxxxxxxxx>
> > 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@xxxxxxxxxx>; Greg Kroah-Hartman
> > > > > <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Miquel
> > > > > Raynal <miquel.raynal@xxxxxxxxxxx>; Emil Renner Berthing
> > > > > <kernel@xxxxxxxx>; Phil Edworthy <phil.edworthy@xxxxxxxxxxx>;
> > > > > Johan Hovold <johan@xxxxxxxxxx>; linux-serial@xxxxxxxxxxxxxxx;
> > > > > Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; Chris Paterson
> > > > > <Chris.Paterson2@xxxxxxxxxxx>; Biju Das <biju.das@xxxxxxxxxxxxxx>;
> > > > > linux-renesas-soc@xxxxxxxxxxxxxxx
> > > > > 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@xxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > 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
