netdev - Re: mv88e6xxx: Timeout waiting for EEPROM done

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <597f21f0-e922-440c-91af-b12cb2a0b7a4@lunn.ch>
Date: Thu, 14 Sep 2023 23:38:32 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Fabio Estevam <festevam@...il.com>
Cc: Vladimir Oltean <olteanv@...il.com>, l00g33k@...il.com,
	netdev <netdev@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>,
	sashal@...nel.org
Subject: Re: mv88e6xxx: Timeout waiting for EEPROM done

> I have the impression that the hardware reset logic is not correctly
> implemented.
> 
> If I change it like this, I don't get the "Timeout waiting for EEPROM
> done" error:
> 
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -7076,13 +7076,16 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
> 
>         chip->info = compat_info;
> 
> -       chip->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +       chip->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>         if (IS_ERR(chip->reset)) {
>                 err = PTR_ERR(chip->reset);
>                 goto out;
>         }
> -       if (chip->reset)
> +       if (chip->reset) {
>                 usleep_range(10000, 20000);
> +               gpiod_set_value(chip->reset, 0);
> +               usleep_range(10000, 20000);
> +       }
> 
> In the devicetree I pass:
> reset-gpios = <&gpio1 5 GPIO_ACTIVE_LOW>;

Unfortunately, none of my boards appear to have the reset pin wired to
a GPIO.

The 6352 data sheet documents the reset pin is active low. So i can
understand using GPIO_ACTIVE_LOW.

In probe, we want to ensure the switch is taken out of reset, if the
bootloader etc has left it in reset. We don't actually perform a reset
here. That is done later. So we want the pin to have a high value. I
know gpiod_set_value() takes into account if the DT node has
GPIO_ACTIVE_LOW. So setting a value of 0 disables it, which means it
goes high. This is what we want. But the intention of the code is that
the actual devm_gpiod_get_optional() should set the GPIO to output a
high. But does devm_gpiod_get_optional() do the same mapping as
gpiod_set_value()? gpiod_direction_output() documents says:

 * Set the direction of the passed GPIO to output, such as gpiod_set_value() can
 * be called safely on it. The initial value of the output must be specified
 * as the logical value of the GPIO, i.e. taking its ACTIVE_LOW status into
 * account.

I don't know how to interpret this.

Is the first change on its own sufficient to make it work? As i said,
we don't aim to reset it here, just ensure it is out of reset. So
ideally all we need is devm_gpiod_get_optional() followed by a pause
just in case it was held in reset, and it will ignore MDIO requests
for a while until it sorts itself out.

Alfred: How do you have the reset GPIO configured in your DT?
GPIO_ACTIVE_LOW?

    Andrew