| Message ID | 20230113184334.287130-8-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: imx: work-around for hardware RX flood, and then isr improvements | expand |
On Fri, 13 Jan 2023, Sergey Organov wrote: > Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know > we read registers that must not be cached. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > drivers/tty/serial/imx.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index be00362b8b67..f4236e8995fa 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > struct imx_port *sport = dev_id; > unsigned int rx, flg; > struct tty_port *port = &sport->port.state->port; > + typeof(sport->port.membase) membase = sport->port.membase; > u32 usr2; > > /* If we received something, check for 0xff flood */ > - usr2 = imx_uart_readl(sport, USR2); > + usr2 = readl(membase + USR2); > if (usr2 & USR2_RDR) > imx_uart_check_flood(sport, usr2); > > - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { > + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { > flg = TTY_NORMAL; > sport->port.icount.rx++; I'd just make a uport local variable and use uport->membase + xx. There are plenty of sport->port constructs to replace with uport in that function anyway.
On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote: > Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know > we read registers that must not be cached. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > drivers/tty/serial/imx.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index be00362b8b67..f4236e8995fa 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > struct imx_port *sport = dev_id; > unsigned int rx, flg; > struct tty_port *port = &sport->port.state->port; > + typeof(sport->port.membase) membase = sport->port.membase; > u32 usr2; > > /* If we received something, check for 0xff flood */ > - usr2 = imx_uart_readl(sport, USR2); > + usr2 = readl(membase + USR2); > if (usr2 & USR2_RDR) > imx_uart_check_flood(sport, usr2); > > - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { > + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { > flg = TTY_NORMAL; > sport->port.icount.rx++; One of the motivations to introduce imx_uart_readl was to have a single place to add a debug output to be able to inspect what the driver is doing. I wonder where your need for higher speed comes from and if the compiler really generates more effective code with your change. Please either drop the patch from your series or provide the differences the compiler produces and a benchmark. Best regards Uwe
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote: >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know >> we read registers that must not be cached. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> drivers/tty/serial/imx.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> index be00362b8b67..f4236e8995fa 100644 >> --- a/drivers/tty/serial/imx.c >> +++ b/drivers/tty/serial/imx.c >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) >> struct imx_port *sport = dev_id; >> unsigned int rx, flg; >> struct tty_port *port = &sport->port.state->port; >> + typeof(sport->port.membase) membase = sport->port.membase; >> u32 usr2; >> >> /* If we received something, check for 0xff flood */ >> - usr2 = imx_uart_readl(sport, USR2); >> + usr2 = readl(membase + USR2); >> if (usr2 & USR2_RDR) >> imx_uart_check_flood(sport, usr2); >> >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { >> flg = TTY_NORMAL; >> sport->port.icount.rx++; > > One of the motivations to introduce imx_uart_readl was to have a single > place to add a debug output to be able to inspect what the driver is > doing. > > I wonder where your need for higher speed comes from and if the compiler > really generates more effective code with your change. Mostly it's because I'm obviously slowing things down a bit with the patch to fight the flood, so I feel obliged to get things back on par with the origin. Then, higher speed, let alone the time spent with interrupts disabled and/or spinlocks taken, is always one of generic goals for me. As for the generated code, with this patch I don't aim to affect code generation, I rather avoid execution of part of existing code while being on the most critical path. It should be quite obvious that not executing some code is at least not slower than executing it. > > Please either drop the patch from your series or provide the differences > the compiler produces and a benchmark. If your only objection against this patch is the desire to keep a single place to add debug output, I'll be happy to tune the resulting code to still have one. That said, before we make a decision, could you please tell why register shadows that the imx_uart_readl/writel are dealing with are needed in the first place? It looks like all the registers that are shadowed are readable as well. What's going on here, and if it happens to be a speed-up, do we have any benchmarks? Thanks, -- Sergey Organov
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes: > On Fri, 13 Jan 2023, Sergey Organov wrote: > >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know >> we read registers that must not be cached. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> drivers/tty/serial/imx.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> index be00362b8b67..f4236e8995fa 100644 >> --- a/drivers/tty/serial/imx.c >> +++ b/drivers/tty/serial/imx.c >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) >> struct imx_port *sport = dev_id; >> unsigned int rx, flg; >> struct tty_port *port = &sport->port.state->port; >> + typeof(sport->port.membase) membase = sport->port.membase; >> u32 usr2; >> >> /* If we received something, check for 0xff flood */ >> - usr2 = imx_uart_readl(sport, USR2); >> + usr2 = readl(membase + USR2); >> if (usr2 & USR2_RDR) >> imx_uart_check_flood(sport, usr2); >> >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { >> flg = TTY_NORMAL; >> sport->port.icount.rx++; > > I'd just make a uport local variable and use uport->membase + xx. There > are plenty of sport->port constructs to replace with uport in that > function anyway. OK, thanks, will do it this way. Probably with global rename over this function in a separate patch?
Sherry Sun <sherry.sun@nxp.com> writes: >> -----Original Message----- >> From: Sergey Organov <sorganov@gmail.com> >> Sent: 2023年1月14日 2:44 >> To: linux-serial@vger.kernel.org >> Cc: Fabio Estevam <festevam@gmail.com>; Greg Kroah-Hartman >> <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>; Richard >> Genoud <richard.genoud@gmail.com>; Sascha Hauer >> <s.hauer@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>; >> tharvey@gateworks.com; Tomasz Moń <tomasz.mon@camlingroup.com>; >> linux-arm-kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; >> Pengutronix Kernel Team <kernel@pengutronix.de>; Sergey Organov >> <sorganov@gmail.com> >> Subject: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know we >> read registers that must not be cached. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> drivers/tty/serial/imx.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index >> be00362b8b67..f4236e8995fa 100644 >> --- a/drivers/tty/serial/imx.c >> +++ b/drivers/tty/serial/imx.c >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void >> *dev_id) >> struct imx_port *sport = dev_id; >> unsigned int rx, flg; >> struct tty_port *port = &sport->port.state->port; >> + typeof(sport->port.membase) membase = sport->port.membase; >> u32 usr2; >> >> /* If we received something, check for 0xff flood */ >> - usr2 = imx_uart_readl(sport, USR2); >> + usr2 = readl(membase + USR2); >> if (usr2 & USR2_RDR) >> imx_uart_check_flood(sport, usr2); >> >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { > > Actually imx_uart_readl() only set shadow registers for UCRx and UFCR, > for the USR2 and URXD0 that you used here, they will not be cached. Sure, and that's why we here don't need to call imx_uart_readl(), that only needlessly checks for shadowing, thus producing pure overhead. Best Regards, Sergey
Hello Sergey, On Tue, Jan 17, 2023 at 04:22:51PM +0300, Sergey Organov wrote: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote: > >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know > >> we read registers that must not be cached. > >> > >> Signed-off-by: Sergey Organov <sorganov@gmail.com> > >> --- > >> drivers/tty/serial/imx.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > >> index be00362b8b67..f4236e8995fa 100644 > >> --- a/drivers/tty/serial/imx.c > >> +++ b/drivers/tty/serial/imx.c > >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > >> struct imx_port *sport = dev_id; > >> unsigned int rx, flg; > >> struct tty_port *port = &sport->port.state->port; > >> + typeof(sport->port.membase) membase = sport->port.membase; > >> u32 usr2; > >> > >> /* If we received something, check for 0xff flood */ > >> - usr2 = imx_uart_readl(sport, USR2); > >> + usr2 = readl(membase + USR2); > >> if (usr2 & USR2_RDR) > >> imx_uart_check_flood(sport, usr2); > >> > >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { > >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { > >> flg = TTY_NORMAL; > >> sport->port.icount.rx++; > > > > One of the motivations to introduce imx_uart_readl was to have a single > > place to add a debug output to be able to inspect what the driver is > > doing. > > > > I wonder where your need for higher speed comes from and if the compiler > > really generates more effective code with your change. > > Mostly it's because I'm obviously slowing things down a bit with the > patch to fight the flood, so I feel obliged to get things back on par > with the origin. Then, higher speed, let alone the time spent with > interrupts disabled and/or spinlocks taken, is always one of generic > goals for me. > > As for the generated code, with this patch I don't aim to affect code > generation, I rather avoid execution of part of existing code while > being on the most critical path. It should be quite obvious that not > executing some code is at least not slower than executing it. That's true, but I think it doesn't apply here. I would expect that the compiler "sees" for the call imx_uart_readl(sport, USR2) that the 2nd argument is constant and that for that value of offset the call is equivalent to readl(sport->port.membase + offset); So I doubt you're making anything quicker here. I tried the following patch on mainline (that is without the preceding patches in this series): diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 757825edb0cd..cfc2f7057345 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -807,7 +807,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) unsigned int rx, flg, ignored = 0; struct tty_port *port = &sport->port.state->port; - while (imx_uart_readl(sport, USR2) & USR2_RDR) { + while (readl(sport->port.membase + USR2) & USR2_RDR) { u32 usr2; flg = TTY_NORMAL; and the resulting code didn't change at all. For a bigger change (i.e. adding a variable for sport->port.membase and replacing two imx_uart_readl) the code changed quite a bit (it got 28 bytes bigger for imx_v6_v7_defconfig) and in the short time I tried I couldn't judge if the resulting code is better or not. So a change that explicitly doesn't execute the code that the compiler optimizes away anyhow isn't a win. Together with the fact that your patch makes register access use different idioms and so makes it harder to understand for a human I'd say the net benefit of your patch is negative. > > Please either drop the patch from your series or provide the differences > > the compiler produces and a benchmark. > > If your only objection against this patch is the desire to keep a single > place to add debug output, I'll be happy to tune the resulting code to > still have one. I don't see the need to optimize it. > That said, before we make a decision, could you please tell why register > shadows that the imx_uart_readl/writel are dealing with are needed in > the first place? It looks like all the registers that are shadowed are > readable as well. What's going on here, and if it happens to be a > speed-up, do we have any benchmarks? Not sure I did benchmarks back then, probably not. The main motivation was really to have that single access function. So I admit being guilty to have implemented an optimization without hard numbers just assuming that access to (cached) RAM is quicker than the register space. Best regards, Uwe
On Tue, 17 Jan 2023, Sergey Organov wrote: > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes: > > > On Fri, 13 Jan 2023, Sergey Organov wrote: > > > >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know > >> we read registers that must not be cached. > >> > >> Signed-off-by: Sergey Organov <sorganov@gmail.com> > >> --- > >> drivers/tty/serial/imx.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > >> index be00362b8b67..f4236e8995fa 100644 > >> --- a/drivers/tty/serial/imx.c > >> +++ b/drivers/tty/serial/imx.c > >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > >> struct imx_port *sport = dev_id; > >> unsigned int rx, flg; > >> struct tty_port *port = &sport->port.state->port; > >> + typeof(sport->port.membase) membase = sport->port.membase; > >> u32 usr2; > >> > >> /* If we received something, check for 0xff flood */ > >> - usr2 = imx_uart_readl(sport, USR2); > >> + usr2 = readl(membase + USR2); > >> if (usr2 & USR2_RDR) > >> imx_uart_check_flood(sport, usr2); > >> > >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { > >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { > >> flg = TTY_NORMAL; > >> sport->port.icount.rx++; > > > > I'd just make a uport local variable and use uport->membase + xx. There > > are plenty of sport->port constructs to replace with uport in that > > function anyway. > > OK, thanks, will do it this way. Probably with global rename over this > function in a separate patch? Yes, it is better to have it in own patch.
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > Hello Sergey, > > On Tue, Jan 17, 2023 at 04:22:51PM +0300, Sergey Organov wrote: >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: >> > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote: >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know >> >> we read registers that must not be cached. >> >> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> >> --- >> >> drivers/tty/serial/imx.c | 5 +++-- >> >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> >> index be00362b8b67..f4236e8995fa 100644 >> >> --- a/drivers/tty/serial/imx.c >> >> +++ b/drivers/tty/serial/imx.c >> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) >> >> struct imx_port *sport = dev_id; >> >> unsigned int rx, flg; >> >> struct tty_port *port = &sport->port.state->port; >> >> + typeof(sport->port.membase) membase = sport->port.membase; >> >> u32 usr2; >> >> >> >> /* If we received something, check for 0xff flood */ >> >> - usr2 = imx_uart_readl(sport, USR2); >> >> + usr2 = readl(membase + USR2); >> >> if (usr2 & USR2_RDR) >> >> imx_uart_check_flood(sport, usr2); >> >> >> >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { >> >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { >> >> flg = TTY_NORMAL; >> >> sport->port.icount.rx++; >> > >> > One of the motivations to introduce imx_uart_readl was to have a single >> > place to add a debug output to be able to inspect what the driver is >> > doing. >> > >> > I wonder where your need for higher speed comes from and if the compiler >> > really generates more effective code with your change. >> >> Mostly it's because I'm obviously slowing things down a bit with the >> patch to fight the flood, so I feel obliged to get things back on par >> with the origin. Then, higher speed, let alone the time spent with >> interrupts disabled and/or spinlocks taken, is always one of generic >> goals for me. >> >> As for the generated code, with this patch I don't aim to affect code >> generation, I rather avoid execution of part of existing code while >> being on the most critical path. It should be quite obvious that not >> executing some code is at least not slower than executing it. > > That's true, but I think it doesn't apply here. Well, "at least not slower" still applies ;-) > > I would expect that the compiler "sees" for the call > > imx_uart_readl(sport, USR2) > > that the 2nd argument is constant and that for that value of offset the > call is equivalent to readl(sport->port.membase + offset); > > So I doubt you're making anything quicker here. Yep, it's nice compiler is clever enough to optimize-out the switch for constant argument, though I still typically prefer to avoid over-relying on optimizations. That said, I now tend to agree with your POV in this particular case. > > I tried the following patch on mainline (that is without the preceding > patches in this series): > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index 757825edb0cd..cfc2f7057345 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -807,7 +807,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > unsigned int rx, flg, ignored = 0; > struct tty_port *port = &sport->port.state->port; > > - while (imx_uart_readl(sport, USR2) & USR2_RDR) { > + while (readl(sport->port.membase + USR2) & USR2_RDR) { > u32 usr2; > > flg = TTY_NORMAL; > > and the resulting code didn't change at all. For a bigger change (i.e. > adding a variable for sport->port.membase and replacing two > imx_uart_readl) the code changed quite a bit (it got 28 bytes bigger for > imx_v6_v7_defconfig) and in the short time I tried I couldn't judge if > the resulting code is better or not. > > So a change that explicitly doesn't execute the code that the compiler > optimizes away anyhow isn't a win. Together with the fact that your > patch makes register access use different idioms and so makes it harder > to understand for a human I'd say the net benefit of your patch is > negative. OK, you convinced me to drop it. > >> > Please either drop the patch from your series or provide the differences >> > the compiler produces and a benchmark. >> >> If your only objection against this patch is the desire to keep a single >> place to add debug output, I'll be happy to tune the resulting code to >> still have one. > > I don't see the need to optimize it. > >> That said, before we make a decision, could you please tell why register >> shadows that the imx_uart_readl/writel are dealing with are needed in >> the first place? It looks like all the registers that are shadowed are >> readable as well. What's going on here, and if it happens to be a >> speed-up, do we have any benchmarks? > > Not sure I did benchmarks back then, probably not. The main motivation > was really to have that single access function. So I admit being guilty > to have implemented an optimization without hard numbers just assuming > that access to (cached) RAM is quicker than the register space. Well, even if it is quicker, we still spend time writing to both RAM and register, and then there is no gain for the data Tx/Rx registers that aren't cached, yet are on most critical paths. So, if this is just caching and doesn't change behavior, I'd suggest to get rid of the shadowing altogether, making code simpler to follow. Besides, if it were so, I'd had no temptation to replace the imx_uart_readl() with raw readl(). Thanks, -- Sergey
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes: > On Tue, 17 Jan 2023, Sergey Organov wrote: > >> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes: >> >> > On Fri, 13 Jan 2023, Sergey Organov wrote: >> > >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know >> >> we read registers that must not be cached. >> >> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> >> --- >> >> drivers/tty/serial/imx.c | 5 +++-- >> >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> >> index be00362b8b67..f4236e8995fa 100644 >> >> --- a/drivers/tty/serial/imx.c >> >> +++ b/drivers/tty/serial/imx.c >> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) >> >> struct imx_port *sport = dev_id; >> >> unsigned int rx, flg; >> >> struct tty_port *port = &sport->port.state->port; >> >> + typeof(sport->port.membase) membase = sport->port.membase; >> >> u32 usr2; >> >> >> >> /* If we received something, check for 0xff flood */ >> >> - usr2 = imx_uart_readl(sport, USR2); >> >> + usr2 = readl(membase + USR2); >> >> if (usr2 & USR2_RDR) >> >> imx_uart_check_flood(sport, usr2); >> >> >> >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { >> >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { >> >> flg = TTY_NORMAL; >> >> sport->port.icount.rx++; >> > >> > I'd just make a uport local variable and use uport->membase + xx. There >> > are plenty of sport->port constructs to replace with uport in that >> > function anyway. >> >> OK, thanks, will do it this way. Probably with global rename over this >> function in a separate patch? > > Yes, it is better to have it in own patch. Well, it now seems that I'll drop this patch altogether, by agreement with Uwe. Do you think introducing of 'uport' still worth it in this one function? I figure it's probably not, provided the reset of the code in the driver still doesn't use the idiom. Thanks, -- Sergey
On Wed, 18 Jan 2023, Sergey Organov wrote: > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes: > > > On Tue, 17 Jan 2023, Sergey Organov wrote: > > > >> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes: > >> > >> > On Fri, 13 Jan 2023, Sergey Organov wrote: > >> > > >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know > >> >> we read registers that must not be cached. > >> >> > >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> > >> >> --- > >> >> drivers/tty/serial/imx.c | 5 +++-- > >> >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > >> >> index be00362b8b67..f4236e8995fa 100644 > >> >> --- a/drivers/tty/serial/imx.c > >> >> +++ b/drivers/tty/serial/imx.c > >> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > >> >> struct imx_port *sport = dev_id; > >> >> unsigned int rx, flg; > >> >> struct tty_port *port = &sport->port.state->port; > >> >> + typeof(sport->port.membase) membase = sport->port.membase; > >> >> u32 usr2; > >> >> > >> >> /* If we received something, check for 0xff flood */ > >> >> - usr2 = imx_uart_readl(sport, USR2); > >> >> + usr2 = readl(membase + USR2); > >> >> if (usr2 & USR2_RDR) > >> >> imx_uart_check_flood(sport, usr2); > >> >> > >> >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { > >> >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { > >> >> flg = TTY_NORMAL; > >> >> sport->port.icount.rx++; > >> > > >> > I'd just make a uport local variable and use uport->membase + xx. There > >> > are plenty of sport->port constructs to replace with uport in that > >> > function anyway. > >> > >> OK, thanks, will do it this way. Probably with global rename over this > >> function in a separate patch? > > > > Yes, it is better to have it in own patch. > > Well, it now seems that I'll drop this patch altogether, by agreement > with Uwe. Do you think introducing of 'uport' still worth it in this > one function? I figure it's probably not, provided the reset of the code > in the driver still doesn't use the idiom. I've no strong opinion either way. So feel free to leave them as they are now.
Hello Sergey, On Wed, Jan 18, 2023 at 06:40:17PM +0300, Sergey Organov wrote: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > > On Tue, Jan 17, 2023 at 04:22:51PM +0300, Sergey Organov wrote: > >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > >> > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote: > >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know > >> >> we read registers that must not be cached. > >> >> > >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> > >> >> --- > >> >> drivers/tty/serial/imx.c | 5 +++-- > >> >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > >> >> index be00362b8b67..f4236e8995fa 100644 > >> >> --- a/drivers/tty/serial/imx.c > >> >> +++ b/drivers/tty/serial/imx.c > >> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > >> >> struct imx_port *sport = dev_id; > >> >> unsigned int rx, flg; > >> >> struct tty_port *port = &sport->port.state->port; > >> >> + typeof(sport->port.membase) membase = sport->port.membase; > >> >> u32 usr2; > >> >> > >> >> /* If we received something, check for 0xff flood */ > >> >> - usr2 = imx_uart_readl(sport, USR2); > >> >> + usr2 = readl(membase + USR2); > >> >> if (usr2 & USR2_RDR) > >> >> imx_uart_check_flood(sport, usr2); > >> >> > >> >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { > >> >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { > >> >> flg = TTY_NORMAL; > >> >> sport->port.icount.rx++; > >> > > >> > One of the motivations to introduce imx_uart_readl was to have a single > >> > place to add a debug output to be able to inspect what the driver is > >> > doing. > >> > > >> > I wonder where your need for higher speed comes from and if the compiler > >> > really generates more effective code with your change. > >> > >> Mostly it's because I'm obviously slowing things down a bit with the > >> patch to fight the flood, so I feel obliged to get things back on par > >> with the origin. Then, higher speed, let alone the time spent with > >> interrupts disabled and/or spinlocks taken, is always one of generic > >> goals for me. > >> > >> As for the generated code, with this patch I don't aim to affect code > >> generation, I rather avoid execution of part of existing code while > >> being on the most critical path. It should be quite obvious that not > >> executing some code is at least not slower than executing it. > > > > That's true, but I think it doesn't apply here. > > Well, "at least not slower" still applies ;-) > > > > > I would expect that the compiler "sees" for the call > > > > imx_uart_readl(sport, USR2) > > > > that the 2nd argument is constant and that for that value of offset the > > call is equivalent to readl(sport->port.membase + offset); > > > > So I doubt you're making anything quicker here. > > Yep, it's nice compiler is clever enough to optimize-out the switch for > constant argument, though I still typically prefer to avoid over-relying > on optimizations. That said, I now tend to agree with your POV in this > particular case. > > > > > I tried the following patch on mainline (that is without the preceding > > patches in this series): > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > > index 757825edb0cd..cfc2f7057345 100644 > > --- a/drivers/tty/serial/imx.c > > +++ b/drivers/tty/serial/imx.c > > @@ -807,7 +807,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > > unsigned int rx, flg, ignored = 0; > > struct tty_port *port = &sport->port.state->port; > > > > - while (imx_uart_readl(sport, USR2) & USR2_RDR) { > > + while (readl(sport->port.membase + USR2) & USR2_RDR) { > > u32 usr2; > > > > flg = TTY_NORMAL; > > > > and the resulting code didn't change at all. For a bigger change (i.e. > > adding a variable for sport->port.membase and replacing two > > imx_uart_readl) the code changed quite a bit (it got 28 bytes bigger for > > imx_v6_v7_defconfig) and in the short time I tried I couldn't judge if > > the resulting code is better or not. > > > > So a change that explicitly doesn't execute the code that the compiler > > optimizes away anyhow isn't a win. Together with the fact that your > > patch makes register access use different idioms and so makes it harder > > to understand for a human I'd say the net benefit of your patch is > > negative. > > OK, you convinced me to drop it. > > > > >> > Please either drop the patch from your series or provide the differences > >> > the compiler produces and a benchmark. > >> > >> If your only objection against this patch is the desire to keep a single > >> place to add debug output, I'll be happy to tune the resulting code to > >> still have one. > > > > I don't see the need to optimize it. > > > >> That said, before we make a decision, could you please tell why register > >> shadows that the imx_uart_readl/writel are dealing with are needed in > >> the first place? It looks like all the registers that are shadowed are > >> readable as well. What's going on here, and if it happens to be a > >> speed-up, do we have any benchmarks? > > > > Not sure I did benchmarks back then, probably not. The main motivation > > was really to have that single access function. So I admit being guilty > > to have implemented an optimization without hard numbers just assuming > > that access to (cached) RAM is quicker than the register space. > > Well, even if it is quicker, we still spend time writing to both RAM and > register, and then there is no gain for the data Tx/Rx registers that > aren't cached, yet are on most critical paths. Well, assuming we're saving some time for the ctrl registers, it's worth keeping it even though there is no gain for RX/TX, right? There is no overhead for RX/TX. > So, if this is just caching and doesn't change behavior, I'd suggest to > get rid of the shadowing altogether, making code simpler to follow. Knowing it's subjective I don't think the shadowing is complicated. Functions are using the driver specific readl and writel functions and shadowing is limited to these two functions. in sum today I wouldn't change if the code does shadow the registers or not if there isn't at least a strong hint that the one or the other variant is better. So if you still want to work on that you're welcome, but I invite you to do some benchmarks first and not only assume one or the other variant is better. My (unproved) assumption is that for console usage there is hardly a difference and with a workflow that needs more changing of control settings (like half duplex rs485) shadowing is slightly better. Best regards Uwe
Hello Uwe, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > Hello Sergey, > > On Wed, Jan 18, 2023 at 06:40:17PM +0300, Sergey Organov wrote: >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: >> > On Tue, Jan 17, 2023 at 04:22:51PM +0300, Sergey Organov wrote: >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: >> >> > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote: >> >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know >> >> >> we read registers that must not be cached. >> >> >> >> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> >> >> --- >> >> >> drivers/tty/serial/imx.c | 5 +++-- >> >> >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> >> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> >> >> index be00362b8b67..f4236e8995fa 100644 >> >> >> --- a/drivers/tty/serial/imx.c >> >> >> +++ b/drivers/tty/serial/imx.c >> >> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) >> >> >> struct imx_port *sport = dev_id; >> >> >> unsigned int rx, flg; >> >> >> struct tty_port *port = &sport->port.state->port; >> >> >> + typeof(sport->port.membase) membase = sport->port.membase; >> >> >> u32 usr2; >> >> >> >> >> >> /* If we received something, check for 0xff flood */ >> >> >> - usr2 = imx_uart_readl(sport, USR2); >> >> >> + usr2 = readl(membase + USR2); >> >> >> if (usr2 & USR2_RDR) >> >> >> imx_uart_check_flood(sport, usr2); >> >> >> >> >> >> - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { >> >> >> + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { >> >> >> flg = TTY_NORMAL; >> >> >> sport->port.icount.rx++; >> >> > >> >> > One of the motivations to introduce imx_uart_readl was to have a single >> >> > place to add a debug output to be able to inspect what the driver is >> >> > doing. >> >> > >> >> > I wonder where your need for higher speed comes from and if the compiler >> >> > really generates more effective code with your change. >> >> >> >> Mostly it's because I'm obviously slowing things down a bit with the >> >> patch to fight the flood, so I feel obliged to get things back on par >> >> with the origin. Then, higher speed, let alone the time spent with >> >> interrupts disabled and/or spinlocks taken, is always one of generic >> >> goals for me. >> >> >> >> As for the generated code, with this patch I don't aim to affect code >> >> generation, I rather avoid execution of part of existing code while >> >> being on the most critical path. It should be quite obvious that not >> >> executing some code is at least not slower than executing it. >> > >> > That's true, but I think it doesn't apply here. >> >> Well, "at least not slower" still applies ;-) >> >> > >> > I would expect that the compiler "sees" for the call >> > >> > imx_uart_readl(sport, USR2) >> > >> > that the 2nd argument is constant and that for that value of offset the >> > call is equivalent to readl(sport->port.membase + offset); >> > >> > So I doubt you're making anything quicker here. >> >> Yep, it's nice compiler is clever enough to optimize-out the switch for >> constant argument, though I still typically prefer to avoid over-relying >> on optimizations. That said, I now tend to agree with your POV in this >> particular case. >> >> > >> > I tried the following patch on mainline (that is without the preceding >> > patches in this series): >> > >> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> > index 757825edb0cd..cfc2f7057345 100644 >> > --- a/drivers/tty/serial/imx.c >> > +++ b/drivers/tty/serial/imx.c >> > @@ -807,7 +807,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) >> > unsigned int rx, flg, ignored = 0; >> > struct tty_port *port = &sport->port.state->port; >> > >> > - while (imx_uart_readl(sport, USR2) & USR2_RDR) { >> > + while (readl(sport->port.membase + USR2) & USR2_RDR) { >> > u32 usr2; >> > >> > flg = TTY_NORMAL; >> > >> > and the resulting code didn't change at all. For a bigger change (i.e. >> > adding a variable for sport->port.membase and replacing two >> > imx_uart_readl) the code changed quite a bit (it got 28 bytes bigger for >> > imx_v6_v7_defconfig) and in the short time I tried I couldn't judge if >> > the resulting code is better or not. >> > >> > So a change that explicitly doesn't execute the code that the compiler >> > optimizes away anyhow isn't a win. Together with the fact that your >> > patch makes register access use different idioms and so makes it harder >> > to understand for a human I'd say the net benefit of your patch is >> > negative. >> >> OK, you convinced me to drop it. >> >> > >> >> > Please either drop the patch from your series or provide the differences >> >> > the compiler produces and a benchmark. >> >> >> >> If your only objection against this patch is the desire to keep a single >> >> place to add debug output, I'll be happy to tune the resulting code to >> >> still have one. >> > >> > I don't see the need to optimize it. >> > >> >> That said, before we make a decision, could you please tell why register >> >> shadows that the imx_uart_readl/writel are dealing with are needed in >> >> the first place? It looks like all the registers that are shadowed are >> >> readable as well. What's going on here, and if it happens to be a >> >> speed-up, do we have any benchmarks? >> > >> > Not sure I did benchmarks back then, probably not. The main motivation >> > was really to have that single access function. So I admit being guilty >> > to have implemented an optimization without hard numbers just assuming >> > that access to (cached) RAM is quicker than the register space. >> >> Well, even if it is quicker, we still spend time writing to both RAM and >> register, and then there is no gain for the data Tx/Rx registers that >> aren't cached, yet are on most critical paths. > > Well, assuming we're saving some time for the ctrl registers, it's worth > keeping it even though there is no gain for RX/TX, right? There is no > overhead for RX/TX. > >> So, if this is just caching and doesn't change behavior, I'd suggest to >> get rid of the shadowing altogether, making code simpler to follow. > > Knowing it's subjective I don't think the shadowing is complicated. > Functions are using the driver specific readl and writel functions and > shadowing is limited to these two functions. It's not complicated indeed, but it's still code, and the less code, -- the better. > > in sum today I wouldn't change if the code does shadow the registers >or not if there isn't at least a strong hint that the one or the other >variant is better. So if you still want to work on that you're welcome, >but I invite you to do some benchmarks first and not only assume one or >the other variant is better. No code is better hands down, unless proved otherwise. I dunno if a code that somehow sneaked into the kernel gets any specific significance from the maintenance POV though. If so, for me it'd be the only sound argument in favor of keeping the code intact. > > My (unproved) assumption is that for console usage there is hardly a > difference and with a workflow that needs more changing of control > settings (like half duplex rs485) shadowing is slightly better. Well, also unproved, I tend to disagree here. I'm afraid you'd rather have hard time finding a case where it is noticeably better, especially as on the write path the shadowing is pure overhead, and so you'd have hard time getting this code into the kernel in the first place, provided somebody would care to object. For what it's worth, I've removed the shadowing in my kernel version, and it gave me no troubles yet. For reference, I use RS232 only with DMA turned off. Overall, my feeling about the issue: the shadowing code is confusing and should not have been put there in the first place. Essentially it looks like pure code bloat, so removing it would be an improvement. Thus I figure I'll send a separate patch for this, and you guys decide if the patch goes in (and the code in question goes out) or not. Thanks, -- Sergey Organov
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index be00362b8b67..f4236e8995fa 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) struct imx_port *sport = dev_id; unsigned int rx, flg; struct tty_port *port = &sport->port.state->port; + typeof(sport->port.membase) membase = sport->port.membase; u32 usr2; /* If we received something, check for 0xff flood */ - usr2 = imx_uart_readl(sport, USR2); + usr2 = readl(membase + USR2); if (usr2 & USR2_RDR) imx_uart_check_flood(sport, usr2); - while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) { + while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) { flg = TTY_NORMAL; sport->port.icount.rx++;
Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know we read registers that must not be cached. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)