Message-ID: <ZinsCZy0D7YBK0Gu@herburgerg-w2>
Date: Thu, 25 Apr 2024 07:37:13 +0200
From: Gregor Herburger <gregor.herburger@...tq-group.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Thomas Kopp <thomas.kopp@...rochip.com>,
	Vincent Mailhol <mailhol.vincent@...adoo.fr>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-can@...r.kernel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux@...tq-group.com,
	alexander.stein@...tq-group.com
Subject: Re: Re: [PATCH 2/4] can: mcp251xfd: mcp251xfd_regmap_crc_write(): workaround
 for errata 5

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 <gregor.herburger@...tq-group.com>
> > ---
> >  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: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |


Best regards,
Gregor
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Powered by blists - more mailing lists