Re: [PATCH net 1/2] net: dsa: mv88e6xxx: Add support for model-specific pre- and post-reset handlers

[Andrew Lunn]

On Thu, Apr 18, 2024 at 02:16:07PM +0200, Matthias Schiffer wrote:
> Instead of calling mv88e6xxx_g2_eeprom_wait() directly from
> mv88e6xxx_hardware_reset(), add configurable pre- and post-reset hard
> reset handlers. Initially, the handlers are set to
> mv88e6xxx_g2_eeprom_wait() for all families that have get/set_eeprom()
> to match the existing behavior. No functional change intended (except
> for additional error messages on failure).
> 
> Fixes: 6ccf50d4d474 ("net: dsa: mv88e6xxx: Avoid EEPROM timeout when EEPROM is absent")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>
> ---
> 
> One other thing I noticed about the patch referenced in the Fixes tag:
> It completely removed the wait for switches without a get_eeprom() op.
> Are there any switches that do load their configuration from an EEPROM,
> but don't provide read/write access, and thus don't have
> get/set_eeprom() ops, but that still need their config load to be
> completed before/after reset?

I _think_ we implement read/write support for all devices which have
the needed registers. Given the large number of ops structures, it is
possible we are missing entries, but nobody has reported any issues.

> I've left that behavior unchanged in this series, as I currently don't
> have access to other switch manuals and no way to test their behavior.
> 
> 
>  drivers/net/dsa/mv88e6xxx/chip.c | 53 ++++++++++++++++++++++++++------
>  drivers/net/dsa/mv88e6xxx/chip.h | 13 ++++++++
>  2 files changed, 56 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index c95787cb90867..8650d8646120a 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -3075,25 +3075,26 @@ static int mv88e6xxx_software_reset(struct mv88e6xxx_chip *chip)
>  static void mv88e6xxx_hardware_reset(struct mv88e6xxx_chip *chip)
>  {
>  	struct gpio_desc *gpiod = chip->reset;
> +	int err;
>  
>  	/* If there is a GPIO connected to the reset pin, toggle it */
>  	if (gpiod) {
> -		/* If the switch has just been reset and not yet completed
> -		 * loading EEPROM, the reset may interrupt the I2C transaction
> -		 * mid-byte, causing the first EEPROM read after the reset
> -		 * from the wrong location resulting in the switch booting
> -		 * to wrong mode and inoperable.
> -		 */

I prefer having the comment here, where it is visible.

>  	int (*ppu_enable)(struct mv88e6xxx_chip *chip);
>  	int (*ppu_disable)(struct mv88e6xxx_chip *chip);
>  
> +	/* Additional handlers to run before and after hard reset, to make sure
> +	 * that the switch and EEPROM are in a good state.

I would keep this sentence here.

    Andrew

---
pw-bot: cr