[2/4] can: mcp251xfd: mcp251xfd_regmap_crc_write(): workaround for errata 5

Commit Message

Gregor Herburger April 17, 2024, 1:43 p.m. UTC

According to Errata DS80000789E 5 writing IOCON register using one SPI
write command clears LAT0/LAT1.

Errata Fix/Work Around suggests to write registers with single byte write
instructions. However, it seems that every write to the second byte
causes the overrite of LAT0/LAT1.

Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.

Signed-off-by: Gregor Herburger <[e-mail]>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

Comments

Simon Horman April 23, 2024, 4:46 p.m. UTC | #1

On Wed, Apr 17, 2024 at 03:43:55PM +0200, Gregor Herburger wrote:
> According to Errata DS80000789E 5 writing IOCON register using one SPI
> write command clears LAT0/LAT1.
> 
> Errata Fix/Work Around suggests to write registers with single byte write
> instructions. However, it seems that every write to the second byte
> causes the overrite of LAT0/LAT1.

nit: overwrite

Flagged by ./scripts/checkpatch.pl --codespell

> 
> Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.
> 
> Signed-off-by: Gregor Herburger <[e-mail]>

...

Vincent Mailhol April 24, 2024, 10:54 a.m. UTC | #2

On Wed. 17 Apr. 2024 at 22:45, Gregor Herburger
<[e-mail]> wrote:
> According to Errata DS80000789E 5 writing IOCON register using one SPI
> write command clears LAT0/LAT1.
>
> Errata Fix/Work Around suggests to write registers with single byte write
> instructions. However, it seems that every write to the second byte
> causes the overrite of LAT0/LAT1.
>
> Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.
>
> Signed-off-by: Gregor Herburger <[e-mail]>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> index 92b7bc7f14b9..ab4e372baffb 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> @@ -229,14 +229,47 @@ mcp251xfd_regmap_crc_gather_write(void *context,
>         return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
>  }
>
> +static int
> +mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
                                                                            ^^^^
count is never used.

> +{
> +       const size_t data_offset = sizeof(__be16) +
> +               mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +       u16 reg = *(u16 *)data;

This line made me scratch my head a lot.

When I see a void * parameter named data, I expect this to be a memory
region. Here, if I got this correctly, data is just a pointer to a u16
which represents the low bit of a register.

So, if you are not passing an address to a memory region but just a
single scalar, why the void *? Wouldn't it be better to just do:

  mcp251xfd_regmap_crc_write_iocon(void *context, u16 reg)

> +       /* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
> +        *
> +        * According to Errata DS80000789E 5 writing IOCON register using one
> +        * SPI write command clears LAT0/LAT1.
> +        *
> +        * Errata Fix/Work Around suggests to write registers with single byte
> +        * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
> +        * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
                                                                  ^^^^^^^^
clearing

> +        */
> +
> +       /* Write IOCON[15:0] */
> +       mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +                                         data + data_offset, 2);
> +       reg += 3;
> +       /* Write IOCON[31:24] */
> +       mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +                                         data + data_offset + 3, 1);
> +
> +       return 0;
> +}
> +
>  static int
>  mcp251xfd_regmap_crc_write(void *context,
>                            const void *data, size_t count)

This also uses the const void* data, except that here, this is kind of
forced by the prototype of the write() callback function from struct
regmap_bus. Also, count is properly used.

>  {
>         const size_t data_offset = sizeof(__be16) +
>                 mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +       u16 reg = *(u16 *)data;
>
> -       return mcp251xfd_regmap_crc_gather_write(context,
> +       if (reg == MCP251XFD_REG_IOCON)
> +               return mcp251xfd_regmap_crc_write_iocon(context,
> +                                                data, count);

After changing the prototype of mcp251xfd_regmap_crc_write_iocon(),
this would then become:

                return mcp251xfd_regmap_crc_write_iocon(context, reg);

> +       else
> +               return mcp251xfd_regmap_crc_gather_write(context,
>                                                  data, data_offset,
>                                                  data + data_offset,
>                                                  count - data_offset);

Marc Kleine-Budde April 24, 2024, 11:51 a.m. UTC | #3

On 17.04.2024 15:43:55, Gregor Herburger wrote:
> According to Errata DS80000789E 5 writing IOCON register using one SPI
> write command clears LAT0/LAT1.
> 
> Errata Fix/Work Around suggests to write registers with single byte write
> instructions. However, it seems that every write to the second byte
> causes the overrite of LAT0/LAT1.

This change doesn't use single byte write instructions.

> Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.

I discovered that erratum, it's described in
mcp251xfd_chip_rx_int_enable():

	/* Configure GPIOs:
	 * - PIN0: GPIO Input
	 * - PIN1: GPIO Input/RX Interrupt
	 *
	 * PIN1 must be Input, otherwise there is a glitch on the
	 * rx-INT line. It happens between setting the PIN as output
	 * (in the first byte of the SPI transfer) and configuring the
	 * PIN as interrupt (in the last byte of the SPI transfer).
	 */

The problem is that the SPI writes 1 byte at a time, starting at the
lower address. The chip updates the GPIO pin's status after each written
byte.

This may leads to a glitch if you have an external pull up. The power on
default auf the chip is GPIO/input, the GPIO line is not driven by the
chip and with the external pull up this will result in a high level.

If you configure the GPIO as an output/high, the driver first writes
bits 0...7, which results in the GPIO line being configured as an
output; the subsequent bits 8...15 configure the level of the GPIO
line.

This change doesn't take care of this.

I'm not sure, if it's better to have 2 dedicated writes to IOCON in the
driver or try to hide it here in the regmap.

> Signed-off-by: Gregor Herburger <[e-mail]>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> index 92b7bc7f14b9..ab4e372baffb 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> @@ -229,14 +229,47 @@ mcp251xfd_regmap_crc_gather_write(void *context,
>  	return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
>  }
>  
> +static int
> +mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
> +{
> +	const size_t data_offset = sizeof(__be16) +
> +		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +	u16 reg = *(u16 *)data;
> +
> +	/* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
> +	 *
> +	 * According to Errata DS80000789E 5 writing IOCON register using one
> +	 * SPI write command clears LAT0/LAT1.
> +	 *
> +	 * Errata Fix/Work Around suggests to write registers with single byte
> +	 * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
> +	 * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
> +	 */
> +
> +	/* Write IOCON[15:0] */
> +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +					  data + data_offset, 2);

You write 15:0 in 1 go here.

> +	reg += 3;
> +	/* Write IOCON[31:24] */
> +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +					  data + data_offset + 3, 1);
> +
> +	return 0;
> +}
> +
>  static int
>  mcp251xfd_regmap_crc_write(void *context,
>  			   const void *data, size_t count)
>  {
>  	const size_t data_offset = sizeof(__be16) +
>  		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +	u16 reg = *(u16 *)data;
>  
> -	return mcp251xfd_regmap_crc_gather_write(context,
> +	if (reg == MCP251XFD_REG_IOCON)
> +		return mcp251xfd_regmap_crc_write_iocon(context,
> +						 data, count);
> +	else
> +		return mcp251xfd_regmap_crc_gather_write(context,
>  						 data, data_offset,
>  						 data + data_offset,
>  						 count - data_offset);

Marc

Gregor Herburger April 25, 2024, 5:37 a.m. UTC | #4

On Wed, Apr 24, 2024 at 01:51:37PM +0200, Marc Kleine-Budde wrote:
> On 17.04.2024 15:43:55, Gregor Herburger wrote:
> > According to Errata DS80000789E 5 writing IOCON register using one SPI
> > write command clears LAT0/LAT1.
> > 
> > Errata Fix/Work Around suggests to write registers with single byte write
> > instructions. However, it seems that every write to the second byte
> > causes the overrite of LAT0/LAT1.
> 
> This change doesn't use single byte write instructions.

Yes, because this is not necessary. Single byte write instructions
wont't fix the problem. The microchip errata sheet is wrong or at least
misleading expressed.

From my observation single byte insctructions won't fix the problem. No
write to bits [16:24] does fix the problem.

I talked to Thomas Kopp from Microchip about that and he confirmed my
observations.
> 
> > Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.
> 
> I discovered that erratum, it's described in
> mcp251xfd_chip_rx_int_enable():
> 
> 	/* Configure GPIOs:
> 	 * - PIN0: GPIO Input
> 	 * - PIN1: GPIO Input/RX Interrupt
> 	 *
> 	 * PIN1 must be Input, otherwise there is a glitch on the
> 	 * rx-INT line. It happens between setting the PIN as output
> 	 * (in the first byte of the SPI transfer) and configuring the
> 	 * PIN as interrupt (in the last byte of the SPI transfer).
> 	 */
> 
> The problem is that the SPI writes 1 byte at a time, starting at the
> lower address. The chip updates the GPIO pin's status after each written
> byte.
> 
> This may leads to a glitch if you have an external pull up. The power on
> default auf the chip is GPIO/input, the GPIO line is not driven by the
> chip and with the external pull up this will result in a high level.
> 
> If you configure the GPIO as an output/high, the driver first writes
> bits 0...7, which results in the GPIO line being configured as an
> output; the subsequent bits 8...15 configure the level of the GPIO
> line.
> 
> This change doesn't take care of this.

I'm not sure if this is the same problem. Anyway, with this fix we didn't
see any glitches on the gpio lines.
> 
> I'm not sure, if it's better to have 2 dedicated writes to IOCON in the
> driver or try to hide it here in the regmap.

What would be the alternative? Maybe add a mcp251xfd_write_iocon
function to the driver and call there regmap_update_bits twice?

> 
> > Signed-off-by: Gregor Herburger <[e-mail]>
> > ---
> >  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> > index 92b7bc7f14b9..ab4e372baffb 100644
> > --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> > +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> > @@ -229,14 +229,47 @@ mcp251xfd_regmap_crc_gather_write(void *context,
> >  	return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
> >  }
> >  
> > +static int
> > +mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
> > +{
> > +	const size_t data_offset = sizeof(__be16) +
> > +		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> > +	u16 reg = *(u16 *)data;
> > +
> > +	/* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
> > +	 *
> > +	 * According to Errata DS80000789E 5 writing IOCON register using one
> > +	 * SPI write command clears LAT0/LAT1.
> > +	 *
> > +	 * Errata Fix/Work Around suggests to write registers with single byte
> > +	 * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
> > +	 * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
> > +	 */
> > +
> > +	/* Write IOCON[15:0] */
> > +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> > +					  data + data_offset, 2);
> 
> You write 15:0 in 1 go here.
See above.
> 
> > +	reg += 3;
> > +	/* Write IOCON[31:24] */
> > +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> > +					  data + data_offset + 3, 1);
> > +
> > +	return 0;
> > +}
> > +
> >  static int
> >  mcp251xfd_regmap_crc_write(void *context,
> >  			   const void *data, size_t count)
> >  {
> >  	const size_t data_offset = sizeof(__be16) +
> >  		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> > +	u16 reg = *(u16 *)data;
> >  
> > -	return mcp251xfd_regmap_crc_gather_write(context,
> > +	if (reg == MCP251XFD_REG_IOCON)
> > +		return mcp251xfd_regmap_crc_write_iocon(context,
> > +						 data, count);
> > +	else
> > +		return mcp251xfd_regmap_crc_gather_write(context,
> >  						 data, data_offset,
> >  						 data + data_offset,
> >  						 count - data_offset);
> 
> Marc
> 
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |


Best regards,
Gregor

Patch

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
index 92b7bc7f14b9..ab4e372baffb 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
@@ -229,14 +229,47 @@  mcp251xfd_regmap_crc_gather_write(void *context,
 	return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
 }
 
+static int
+mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
+{
+	const size_t data_offset = sizeof(__be16) +
+		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
+	u16 reg = *(u16 *)data;
+
+	/* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
+	 *
+	 * According to Errata DS80000789E 5 writing IOCON register using one
+	 * SPI write command clears LAT0/LAT1.
+	 *
+	 * Errata Fix/Work Around suggests to write registers with single byte
+	 * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
+	 * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
+	 */
+
+	/* Write IOCON[15:0] */
+	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
+					  data + data_offset, 2);
+	reg += 3;
+	/* Write IOCON[31:24] */
+	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
+					  data + data_offset + 3, 1);
+
+	return 0;
+}
+
 static int
 mcp251xfd_regmap_crc_write(void *context,
 			   const void *data, size_t count)
 {
 	const size_t data_offset = sizeof(__be16) +
 		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
+	u16 reg = *(u16 *)data;
 
-	return mcp251xfd_regmap_crc_gather_write(context,
+	if (reg == MCP251XFD_REG_IOCON)
+		return mcp251xfd_regmap_crc_write_iocon(context,
+						 data, count);
+	else
+		return mcp251xfd_regmap_crc_gather_write(context,
 						 data, data_offset,
 						 data + data_offset,
 						 count - data_offset);