[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <719d12fd-d720-40af-870a-6571be4fe06d@lunn.ch>
Date: Mon, 22 Apr 2024 15:25:00 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc: Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Fabio Estevam <festevam@...x.de>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux@...tq-group.com
Subject: Re: [PATCH net 1/2] net: dsa: mv88e6xxx: Add support for
model-specific pre- and post-reset handlers
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@...tq-group.com>
> ---
>
> 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
Powered by blists - more mailing lists