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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
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" <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" 
	<linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH] serial: 8250: dw: Fix the macro RZN1_UART_xDMACR_8_WORD_BURST
Date: Tue, 5 Jul 2022 11:30:01 +0200	[thread overview]
Message-ID: <CAHp75VfBPou1TLk4ygsqF3VSJV84_UQLpwSojELsOt9F42Z_4w@mail.gmail.com> (raw)
In-Reply-To: <OS0PR01MB5922189B75A4C3A93BFE273B86819@OS0PR01MB5922.jpnprd01.prod.outlook.com>

+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