On Tue, Dec 19, 2023 at 12:19:01PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Move common code for EFR lock/unlock of mutex into functions for code reuse
> and clarity.
...
> @@ -333,6 +333,7 @@ struct sc16is7xx_one {
> struct sc16is7xx_one_config config;
> bool irda_mode;
> unsigned int old_mctrl;
> + u8 old_lcr; /* Value before EFR access. */
> };
Have you run `pahole`?
I believe with
unsigned int old_mctrl;
u8 old_lcr; /* Value before EFR access. */
bool irda_mode;
layout it will take less memory.
...
> +/* In an amazing feat of design, the Enhanced Features Register (EFR)
/*
* This is NOT the style we use for multi-line
* comments in the serial subsystem. On contrary
* this comment can be used as a proper example.
* (Yes, I noticed it's an old comment, but take
* a chance to fix it.)
*/
> + * shares the address of the Interrupt Identification Register (IIR).
> + * Access to EFR is switched on by writing a magic value (0xbf) to the
> + * Line Control Register (LCR). Any interrupt firing during this time will
> + * see the EFR where it expects the IIR to be, leading to
> + * "Unexpected interrupt" messages.
> + *
> + * Prevent this possibility by claiming a mutex while accessing the EFR,
> + * and claiming the same mutex from within the interrupt handler. This is
> + * similar to disabling the interrupt, but that doesn't work because the
> + * bulk of the interrupt processing is run as a workqueue job in thread
> + * context.
> + */
...
> + sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
> + SC16IS7XX_LCR_CONF_MODE_B);
One line. (Yes, 81 character, but readability is as good as before.
On Wed, 20 Dec 2023 18:03:18 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Tue, Dec 19, 2023 at 12:19:01PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > Move common code for EFR lock/unlock of mutex into functions for code reuse
> > and clarity.
>
> ...
>
> > @@ -333,6 +333,7 @@ struct sc16is7xx_one {
> > struct sc16is7xx_one_config config;
> > bool irda_mode;
> > unsigned int old_mctrl;
> > + u8 old_lcr; /* Value before EFR access. */
> > };
>
> Have you run `pahole`?
> I believe with
>
> unsigned int old_mctrl;
> u8 old_lcr; /* Value before EFR access. */
> bool irda_mode;
>
> layout it will take less memory.
Hi,
I did not know about this tool, nice.
$ pahole -C sc16is7xx_one drivers/tty/serial/sc16is7xx.o
Before:
/* size: 752, cachelines: 12, members: 10 */
With your proposed change:
/* size: 744, cachelines: 12, members: 10 */
Will add this modification for V2, as well as other issues
noted below.
Thank you,
Hugo
> > +/* In an amazing feat of design, the Enhanced Features Register (EFR)
>
> /*
> * This is NOT the style we use for multi-line
> * comments in the serial subsystem. On contrary
> * this comment can be used as a proper example.
> * (Yes, I noticed it's an old comment, but take
> * a chance to fix it.)
> */
>
> > + * shares the address of the Interrupt Identification Register (IIR).
> > + * Access to EFR is switched on by writing a magic value (0xbf) to the
> > + * Line Control Register (LCR). Any interrupt firing during this time will
> > + * see the EFR where it expects the IIR to be, leading to
> > + * "Unexpected interrupt" messages.
> > + *
> > + * Prevent this possibility by claiming a mutex while accessing the EFR,
> > + * and claiming the same mutex from within the interrupt handler. This is
> > + * similar to disabling the interrupt, but that doesn't work because the
> > + * bulk of the interrupt processing is run as a workqueue job in thread
> > + * context.
> > + */
>
> ...
>
> > + sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
> > + SC16IS7XX_LCR_CONF_MODE_B);
>
> One line. (Yes, 81 character, but readability is as good as before.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
>
@@ -333,6 +333,7 @@ struct sc16is7xx_one {
struct sc16is7xx_one_config config;
bool irda_mode;
unsigned int old_mctrl;
+ u8 old_lcr; /* Value before EFR access. */
};
struct sc16is7xx_port {
@@ -413,6 +414,49 @@ static void sc16is7xx_power(struct uart_port *port, int on)
on ? 0 : SC16IS7XX_IER_SLEEP_BIT);
}
+/* In an amazing feat of design, the Enhanced Features Register (EFR)
+ * shares the address of the Interrupt Identification Register (IIR).
+ * Access to EFR is switched on by writing a magic value (0xbf) to the
+ * Line Control Register (LCR). Any interrupt firing during this time will
+ * see the EFR where it expects the IIR to be, leading to
+ * "Unexpected interrupt" messages.
+ *
+ * Prevent this possibility by claiming a mutex while accessing the EFR,
+ * and claiming the same mutex from within the interrupt handler. This is
+ * similar to disabling the interrupt, but that doesn't work because the
+ * bulk of the interrupt processing is run as a workqueue job in thread
+ * context.
+ */
+static void sc16is7xx_efr_lock(struct uart_port *port)
+{
+ struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+ mutex_lock(&one->efr_lock);
+
+ /* Backup content of LCR. */
+ one->old_lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
+
+ /* Enable access to Enhanced register set */
+ sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
+ SC16IS7XX_LCR_CONF_MODE_B);
+
+ /* Disable cache updates when writing to EFR registers */
+ regcache_cache_bypass(one->regmap, true);
+}
+
+static void sc16is7xx_efr_unlock(struct uart_port *port)
+{
+ struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+ /* Re-enable cache updates when writing to normal registers */
+ regcache_cache_bypass(one->regmap, false);
+
+ /* Restore original content of LCR */
+ sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, one->old_lcr);
+
+ mutex_unlock(&one->efr_lock);
+}
+
static void sc16is7xx_ier_clear(struct uart_port *port, u8 bit)
{
struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
@@ -523,45 +567,19 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
div /= 4;
}
- /* In an amazing feat of design, the Enhanced Features Register shares
- * the address of the Interrupt Identification Register, and is
- * switched in by writing a magic value (0xbf) to the Line Control
- * Register. Any interrupt firing during this time will see the EFR
- * where it expects the IIR to be, leading to "Unexpected interrupt"
- * messages.
- *
- * Prevent this possibility by claiming a mutex while accessing the
- * EFR, and claiming the same mutex from within the interrupt handler.
- * This is similar to disabling the interrupt, but that doesn't work
- * because the bulk of the interrupt processing is run as a workqueue
- * job in thread context.
- */
- mutex_lock(&one->efr_lock);
-
- lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
-
- /* Open the LCR divisors for configuration */
- sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
- SC16IS7XX_LCR_CONF_MODE_B);
-
/* Enable enhanced features */
- regcache_cache_bypass(one->regmap, true);
+ sc16is7xx_efr_lock(port);
sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
SC16IS7XX_EFR_ENABLE_BIT,
SC16IS7XX_EFR_ENABLE_BIT);
-
- regcache_cache_bypass(one->regmap, false);
-
- /* Put LCR back to the normal mode */
- sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
-
- mutex_unlock(&one->efr_lock);
+ sc16is7xx_efr_unlock(port);
sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
SC16IS7XX_MCR_CLKSEL_BIT,
prescaler);
- /* Open the LCR divisors for configuration */
+ /* Backup LCR and access special register set (DLL/DLH) */
+ lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
SC16IS7XX_LCR_CONF_MODE_A);
@@ -571,7 +589,7 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
sc16is7xx_port_write(port, SC16IS7XX_DLL_REG, div % 256);
regcache_cache_bypass(one->regmap, false);
- /* Put LCR back to the normal mode */
+ /* Restore LCR and access to general register set */
sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
return DIV_ROUND_CLOSEST(clk / 16, div);
@@ -1049,17 +1067,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
if (!(termios->c_cflag & CREAD))
port->ignore_status_mask |= SC16IS7XX_LSR_BRK_ERROR_MASK;
- /* As above, claim the mutex while accessing the EFR. */
- mutex_lock(&one->efr_lock);
-
- sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
- SC16IS7XX_LCR_CONF_MODE_B);
-
/* Configure flow control */
- regcache_cache_bypass(one->regmap, true);
- sc16is7xx_port_write(port, SC16IS7XX_XON1_REG, termios->c_cc[VSTART]);
- sc16is7xx_port_write(port, SC16IS7XX_XOFF1_REG, termios->c_cc[VSTOP]);
-
port->status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS);
if (termios->c_cflag & CRTSCTS) {
flow |= SC16IS7XX_EFR_AUTOCTS_BIT |
@@ -1071,16 +1079,16 @@ static void sc16is7xx_set_termios(struct uart_port *port,
if (termios->c_iflag & IXOFF)
flow |= SC16IS7XX_EFR_SWFLOW1_BIT;
- sc16is7xx_port_update(port,
- SC16IS7XX_EFR_REG,
- SC16IS7XX_EFR_FLOWCTRL_BITS,
- flow);
- regcache_cache_bypass(one->regmap, false);
-
/* Update LCR register */
sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
- mutex_unlock(&one->efr_lock);
+ /* Update EFR registers */
+ sc16is7xx_efr_lock(port);
+ sc16is7xx_port_write(port, SC16IS7XX_XON1_REG, termios->c_cc[VSTART]);
+ sc16is7xx_port_write(port, SC16IS7XX_XOFF1_REG, termios->c_cc[VSTOP]);
+ sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
+ SC16IS7XX_EFR_FLOWCTRL_BITS, flow);
+ sc16is7xx_efr_unlock(port);
/* Get baud rate generator configuration */
baud = uart_get_baud_rate(port, termios, old,