mv88e6xxx: Timeout waiting for EEPROM done

mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

Hi,

On an imx8mn-based board with an 88E6320 switch, the following error
started showing up after the commit below on the 6.1 LTS branch:

mv88e6085 30be0000.ethernet-1:00: Timeout waiting for EEPROM done

commit df83af3b996d79d7eb51eaefdffb7f4352e55052
Author: Alfred Lee <l00g33k@gmail.com>
Date:   Mon Aug 14 17:13:23 2023 -0700

    net: dsa: mv88e6xxx: Wait for EEPROM done before HW reset

    [ Upstream commit 23d775f12dcd23d052a4927195f15e970e27ab26 ]

    If the switch is reset during active EEPROM transactions, as in
    just after an SoC reset after power up, the I2C bus transaction
    may be cut short leaving the EEPROM internal I2C state machine
    in the wrong state.  When the switch is reset again, the bad
    state machine state may result in data being read from the wrong
    memory location causing the switch to enter unexpected mode
    rendering it inoperational.

    Fixes: a3dcb3e7e70c ("net: dsa: mv88e6xxx: Wait for EEPROM done
after HW reset")
    Signed-off-by: Alfred Lee <l00g33k@gmail.com>
    Reviewed-by: Andrew Lunn <andrew@lunn.ch>
    Link: https://lore.kernel.org/r/20230815001323.24739-1-l00g33k@gmail.com
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>
    Signed-off-by: Sasha Levin <sashal@kernel.org>

What is the proper way to avoid this error?

Thanks

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Andrew Lunn]

On Thu, Sep 14, 2023 at 11:31:37AM -0300, Fabio Estevam wrote:
> Hi,
> 
> On an imx8mn-based board with an 88E6320 switch, the following error
> started showing up after the commit below on the 6.1 LTS branch:
> 
> mv88e6085 30be0000.ethernet-1:00: Timeout waiting for EEPROM done

Does this board actually have an EEPROM attached to the switch?

In mv88e6xxx_g1_wait_eeprom_done() what value is being returned for
the read of MV88E6XXX_G1_STS?

    Andrew

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

Hi Andrew,

On Thu, Sep 14, 2023 at 12:15 PM Andrew Lunn <andrew@lunn.ch> wrote:

> Does this board actually have an EEPROM attached to the switch?

No, there is no EEPROM attached to the switch on this board.

> In mv88e6xxx_g1_wait_eeprom_done() what value is being returned for
> the read of MV88E6XXX_G1_STS?

[    1.594043] *************** G1_STS is 0xc800
[    1.600348] *************** G1_STS is 0xc800
[    1.606648] *************** G1_STS is 0xc800
[    1.612950] *************** G1_STS is 0xc800
[    1.619250] *************** G1_STS is 0xc800
[    1.625547] mv88e6085 30be0000.ethernet-1:00: Timeout waiting for EEPROM done
[    1.673477] *************** G1_STS is 0xc801

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

On Thu, Sep 14, 2023 at 1:22 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Andrew,
>
> On Thu, Sep 14, 2023 at 12:15 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Does this board actually have an EEPROM attached to the switch?
>
> No, there is no EEPROM attached to the switch on this board.
>
> > In mv88e6xxx_g1_wait_eeprom_done() what value is being returned for
> > the read of MV88E6XXX_G1_STS?
>
> [    1.594043] *************** G1_STS is 0xc800
> [    1.600348] *************** G1_STS is 0xc800
> [    1.606648] *************** G1_STS is 0xc800
> [    1.612950] *************** G1_STS is 0xc800
> [    1.619250] *************** G1_STS is 0xc800
> [    1.625547] mv88e6085 30be0000.ethernet-1:00: Timeout waiting for EEPROM done
> [    1.673477] *************** G1_STS is 0xc801

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>;

Can anyone confirm what is the recommended hardware reset sequence?

Thanks

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Andrew Lunn]

> 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

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

Hi Andrew,

On Thu, Sep 14, 2023 at 6:38 PM Andrew Lunn <andrew@lunn.ch> wrote:

> 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.

What does the datasheet say about the minimal duration for the reset
pin being asserted?

Unfortunately, I don't have access to it.

Usually, the datasheets specify a minimum duration for the reset to be
at logic level low.

For example:
https://www.marvell.com/content/dam/marvell/en/public-collateral/phys-transceivers/marvell-phys-transceivers-fast-ethernet-88e301x-functional-specifications.pdf

(4.6.1 Reset and Configuration Timing - says the reset pin should be
low for at least 10 ms).

> 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

My concern is that the implemented method to bring the reset pin out
of reset may violate the datasheet by not waiting the required amount
of time.

Someone who has access to the datasheet may confirm, please.

> 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:

Yes, this is my understanding.

>  * 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,

No, it is not. On my tests, I needed to force the reset GPIO to be low
for a certain duration,

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Andrew Lunn]

On Thu, Sep 14, 2023 at 09:40:05PM -0300, Fabio Estevam wrote:
> Hi Andrew,
> 
> On Thu, Sep 14, 2023 at 6:38 PM Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > 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.
> 
> What does the datasheet say about the minimal duration for the reset
> pin being asserted?

It is a bit ambiguous. RESETn is both an input and an output. It says
this about input for the For the 6352:

     As in input, when RESETn is driven low by an external device,
     this device will then driver RESETn low as an output for 8 to
     14ms (10ms typically). In this mode RESETn can be used to
     debounce a hardware reset switch.

So i would say it needs to be low long enough not to be a glitch, but
can be short.

> > 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
> 
> My concern is that the implemented method to bring the reset pin out
> of reset may violate the datasheet by not waiting the required amount
> of time.
> 
> Someone who has access to the datasheet may confirm, please.
> 
> > 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:
> 
> Yes, this is my understanding.
> 
> >  * 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,
> 
> No, it is not. On my tests, I needed to force the reset GPIO to be low
> for a certain duration,

Is you device held in reset before the driver loads? As i said, the
aim of this code is not to actually reset the switch, but to ensure it
is taken out of reset if it was being held in reset. And if it was
being held in reset, i would expect that to be for a long time, at
least the current Linux boot time.

   Andrew

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

Hi Andrew,

On Fri, Sep 15, 2023 at 10:08 AM Andrew Lunn <andrew@lunn.ch> wrote:

> > What does the datasheet say about the minimal duration for the reset
> > pin being asserted?
>
> It is a bit ambiguous. RESETn is both an input and an output. It says
> this about input for the For the 6352:
>
>      As in input, when RESETn is driven low by an external device,
>      this device will then driver RESETn low as an output for 8 to
>      14ms (10ms typically). In this mode RESETn can be used to
>      debounce a hardware reset switch.
>
> So i would say it needs to be low long enough not to be a glitch, but
> can be short.

Thanks for confirming with the datasheet.

> Is you device held in reset before the driver loads? As i said, the

Just checked with a scope here and no, the reset pin is not held in
reset before the driver loads.

> aim of this code is not to actually reset the switch, but to ensure it
> is taken out of reset if it was being held in reset. And if it was
> being held in reset, i would expect that to be for a long time, at
> least the current Linux boot time.

That's a point I am concerned about: why don't we follow the datasheet
with respect to taking the reset pin out of reset?

Isn't the sequence I used below better suited as it follows the
datasheet by guaranteeing the 10ms at a low level?

       chip->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
       ....
       if (chip->reset) {
                usleep_range(10000, 20000);
                gpiod_set_value(chip->reset, 0);
                usleep_range(10000, 20000);
       }

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Andrew Lunn]

> Just checked with a scope here and no, the reset pin is not held in
> reset before the driver loads.
> 
> > aim of this code is not to actually reset the switch, but to ensure it
> > is taken out of reset if it was being held in reset. And if it was
> > being held in reset, i would expect that to be for a long time, at
> > least the current Linux boot time.
> 
> That's a point I am concerned about: why don't we follow the datasheet
> with respect to taking the reset pin out of reset?
> 
> Isn't the sequence I used below better suited as it follows the
> datasheet by guaranteeing the 10ms at a low level?
> 
>        chip->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>        ....
>        if (chip->reset) {
>                 usleep_range(10000, 20000);
>                 gpiod_set_value(chip->reset, 0);
>                 usleep_range(10000, 20000);
>        }

As i said, we do a hardware reset later. All we are trying to do at
this stage is probe the device, does it exist on the bus, and what ID
value does it have. I want to avoid the overhead of doing a reset now,
and then doing it again later.

So you say your device is not held in reset. So ideally there should
not be a change in the GPIO with devm_gpiod_get_optional(), and the
delay afterwards is pointless in your case.

When the device is held in reset, devm_gpiod_get_optional() should
release it from reset, and then we need a delay because experience has
shown the device will not actually respond on the MDIO bus for a short
while.

	Andrew

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

On Fri, Sep 15, 2023 at 11:34 AM Andrew Lunn <andrew@lunn.ch> wrote:

> As i said, we do a hardware reset later. All we are trying to do at
> this stage is probe the device, does it exist on the bus, and what ID
> value does it have. I want to avoid the overhead of doing a reset now,
> and then doing it again later.

Now I see your point, thanks.

Back to mv88e6xxx_g1_read(): should we check whether the EEPROM is
present and bail out if absent?

Did a quick hack just to show the idea. Please let me know what you think.

diff --git a/drivers/net/dsa/mv88e6xxx/global1.c
b/drivers/net/dsa/mv88e6xxx/global1.c
index 5848112036b0..0e8b8667e897 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -12,6 +12,7 @@

 #include "chip.h"
 #include "global1.h"
+#include "global2.h"

 int mv88e6xxx_g1_read(struct mv88e6xxx_chip *chip, int reg, u16 *val)
 {
@@ -80,6 +81,13 @@ void mv88e6xxx_g1_wait_eeprom_done(struct
mv88e6xxx_chip *chip)
        const unsigned long timeout = jiffies + 1 * HZ;
        u16 val;
        int err;
+       u16 data;
+       u8 addr = 0;
+
+       err = mv88e6xxx_g2_eeprom_read16(chip, addr, &data);
+       /* Test whether EEPROM is present and bail out if absent */
+       if (data == 0xffff)
+               return;

        /* Wait up to 1 second for the switch to finish reading the
         * EEPROM.
diff --git a/drivers/net/dsa/mv88e6xxx/global2.c
b/drivers/net/dsa/mv88e6xxx/global2.c
index ec49939968fa..e75d7c029280 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -411,7 +411,7 @@ static int mv88e6xxx_g2_eeprom_write8(struct
mv88e6xxx_chip *chip,
        return mv88e6xxx_g2_eeprom_cmd(chip, cmd | data);
 }

-static int mv88e6xxx_g2_eeprom_read16(struct mv88e6xxx_chip *chip,
+int mv88e6xxx_g2_eeprom_read16(struct mv88e6xxx_chip *chip,
                                      u8 addr, u16 *data)
 {
        u16 cmd = MV88E6XXX_G2_EEPROM_CMD_OP_READ | addr;
diff --git a/drivers/net/dsa/mv88e6xxx/global2.h
b/drivers/net/dsa/mv88e6xxx/global2.h
index c05fad5c9f19..5bb6d6d7ca86 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.h
+++ b/drivers/net/dsa/mv88e6xxx/global2.h
@@ -331,6 +331,7 @@ int mv88e6xxx_g2_get_eeprom16(struct mv88e6xxx_chip *chip,
                              struct ethtool_eeprom *eeprom, u8 *data);
 int mv88e6xxx_g2_set_eeprom16(struct mv88e6xxx_chip *chip,
                              struct ethtool_eeprom *eeprom, u8 *data);
+int mv88e6xxx_g2_eeprom_read16(struct mv88e6xxx_chip *chip, u8 addr,
u16 *data);

 int mv88e6xxx_g2_pvt_read(struct mv88e6xxx_chip *chip, int src_dev,
                          int src_port, u16 *data);

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Andrew Lunn]

> Back to mv88e6xxx_g1_read(): should we check whether the EEPROM is
> present and bail out if absent?
> 
> Did a quick hack just to show the idea. Please let me know what you think.
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/global1.c
> b/drivers/net/dsa/mv88e6xxx/global1.c
> index 5848112036b0..0e8b8667e897 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1.c
> @@ -12,6 +12,7 @@
> 
>  #include "chip.h"
>  #include "global1.h"
> +#include "global2.h"
> 
>  int mv88e6xxx_g1_read(struct mv88e6xxx_chip *chip, int reg, u16 *val)
>  {
> @@ -80,6 +81,13 @@ void mv88e6xxx_g1_wait_eeprom_done(struct
> mv88e6xxx_chip *chip)
>         const unsigned long timeout = jiffies + 1 * HZ;
>         u16 val;
>         int err;
> +       u16 data;
> +       u8 addr = 0;
> +
> +       err = mv88e6xxx_g2_eeprom_read16(chip, addr, &data);

The problem with this is that the way to read the contests of the
EEPROM depend on the switch family.

linux/drivers/net/dsa/mv88e6xxx$ grep \.get_eeprom chip.c
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,
	.get_eeprom = mv88e6xxx_g2_get_eeprom16,
	.get_eeprom = mv88e6xxx_g2_get_eeprom16,
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,
	.get_eeprom = mv88e6xxx_g2_get_eeprom16,
	.get_eeprom = mv88e6xxx_g2_get_eeprom16,
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,
	.get_eeprom = mv88e6xxx_g2_get_eeprom16,
	.get_eeprom = mv88e6xxx_g2_get_eeprom16,
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,
	.get_eeprom = mv88e6xxx_g2_get_eeprom16,
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,
	.get_eeprom = mv88e6xxx_g2_get_eeprom8,


> +       /* Test whether EEPROM is present and bail out if absent */
> +       if (data == 0xffff)
> +               return;

And how do you know the EEPROM does not in fact contain 0xffff?

What i found interesting in the datasheet for the 6352:

     The EEInt indicates the processing of the EEPROM contents is
     complete and the I/O registers are now available for CPU
     access. A CPU can use this interrupt to know it is OK to start
     accessing the device’s registers. The EEInt will assert the
     device’s INT pin even if not EEPROM is attached unless the EEPROM
     changes the contents of the EEIntMast register (Global 1, offset
     0x04) or if the Test SW_MODE has been configured (see 8888E6352,
     88E6240, 88E6176, and 88E6172 Functional Specification Datasheet,
     Part 1 of 3: Overview, Pinout, Applications, Mechanical and
     Electrical Specifications for details).  The StatsDone, VTUDone
     and ATUDone interrupts de-assert after the Switch Globa

So i would expect that EEInt is set when there is no EEPROM.

What strapping do you have for SW_MODE? Is the switch actually in
standalone mode?

	   Andrew

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Alfred Lee]

On Thu, Sep 14, 2023 at 2:38 PM Andrew Lunn <andrew@lunn.ch> wrote:

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

I will check the schematic when I get in the office on Monday.

Alfred

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Alfred Lee]

On Thu, Sep 14, 2023 at 2:38 PM Andrew Lunn <andrew@lunn.ch> wrote:

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

Hello Andrew,

It is indeed set to GPIO_ACTIVE_LOW:

    switch0: switch0@0 {
        compatible = "marvell,mv88e6190";
        pinctrl-0 = <&pinctrl_sw_reset>;
        pinctrl-names = "default";
        #address-cells = <1>;
        #size-cells = <0>;
        reg = <0>;
        eeprom-length = <512>;
        reset-gpios = <&gpio3 11 GPIO_ACTIVE_LOW>

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

Hi Andrew,

On Fri, Sep 15, 2023 at 3:23 PM Andrew Lunn <andrew@lunn.ch> wrote:

> The problem with this is that the way to read the contests of the
> EEPROM depend on the switch family.
>
> linux/drivers/net/dsa/mv88e6xxx$ grep \.get_eeprom chip.c
>         .get_eeprom = mv88e6xxx_g2_get_eeprom8,
>         .get_eeprom = mv88e6xxx_g2_get_eeprom16,

Indeed, there are two methods for reading the EEPROM.

> And how do you know the EEPROM does not in fact contain 0xffff?

The functional spec doc says:

"If the just read in Command is all one’s, terminate the serial EEPROM
reading process, go to 8."

> What i found interesting in the datasheet for the 6352:
>
>      The EEInt indicates the processing of the EEPROM contents is
>      complete and the I/O registers are now available for CPU
>      access. A CPU can use this interrupt to know it is OK to start
>      accessing the device’s registers. The EEInt will assert the
>      device’s INT pin even if not EEPROM is attached unless the EEPROM
>      changes the contents of the EEIntMast register (Global 1, offset
>      0x04) or if the Test SW_MODE has been configured (see 8888E6352,
>      88E6240, 88E6176, and 88E6172 Functional Specification Datasheet,
>      Part 1 of 3: Overview, Pinout, Applications, Mechanical and
>      Electrical Specifications for details).  The StatsDone, VTUDone
>      and ATUDone interrupts de-assert after the Switch Globa
>
> So i would expect that EEInt is set when there is no EEPROM.
>
> What strapping do you have for SW_MODE? Is the switch actually in
> standalone mode?

Pardon my ignorance, but I don't know the answer to these.

I do have access to the schematics. How can I tell?

Thanks

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Andrew Lunn]

On Tue, Sep 19, 2023 at 04:23:01PM -0300, Fabio Estevam wrote:
> Hi Andrew,
> 
> On Fri, Sep 15, 2023 at 3:23 PM Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > The problem with this is that the way to read the contests of the
> > EEPROM depend on the switch family.
> >
> > linux/drivers/net/dsa/mv88e6xxx$ grep \.get_eeprom chip.c
> >         .get_eeprom = mv88e6xxx_g2_get_eeprom8,
> >         .get_eeprom = mv88e6xxx_g2_get_eeprom16,
> 
> Indeed, there are two methods for reading the EEPROM.
> 
> > And how do you know the EEPROM does not in fact contain 0xffff?
> 
> The functional spec doc says:
> 
> "If the just read in Command is all one’s, terminate the serial EEPROM
> reading process, go to 8."

So reading 0xffff means we have reached the end of the contents, not
that it actually exists.

> > What i found interesting in the datasheet for the 6352:
> >
> >      The EEInt indicates the processing of the EEPROM contents is
> >      complete and the I/O registers are now available for CPU
> >      access. A CPU can use this interrupt to know it is OK to start
> >      accessing the device’s registers. The EEInt will assert the
> >      device’s INT pin even if not EEPROM is attached unless the EEPROM
> >      changes the contents of the EEIntMast register (Global 1, offset
> >      0x04) or if the Test SW_MODE has been configured (see 8888E6352,
> >      88E6240, 88E6176, and 88E6172 Functional Specification Datasheet,
> >      Part 1 of 3: Overview, Pinout, Applications, Mechanical and
> >      Electrical Specifications for details).  The StatsDone, VTUDone
> >      and ATUDone interrupts de-assert after the Switch Globa
> >
> > So i would expect that EEInt is set when there is no EEPROM.
> >
> > What strapping do you have for SW_MODE? Is the switch actually in
> > standalone mode?
> 
> Pardon my ignorance, but I don't know the answer to these.
> 
> I do have access to the schematics. How can I tell?

Good question. The datasheets i have don't actually say!

I'm assuming there are two pins which can be strapped to give the
value of SW_MODE, and a value of 2 indicates standalone. But i've no
idea which pins they are.

     Andrew

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

On Tue, Sep 19, 2023 at 4:44 PM Andrew Lunn <andrew@lunn.ch> wrote:

> Good question. The datasheets i have don't actually say!
>
> I'm assuming there are two pins which can be strapped to give the
> value of SW_MODE, and a value of 2 indicates standalone. But i've no
> idea which pins they are.

What is the register I need to dump in the mv88e6xxx driver to check
the value of SW_MODE?

Thanks

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Andrew Lunn]

On Tue, Sep 19, 2023 at 09:47:24PM -0300, Fabio Estevam wrote:
> On Tue, Sep 19, 2023 at 4:44 PM Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > Good question. The datasheets i have don't actually say!
> >
> > I'm assuming there are two pins which can be strapped to give the
> > value of SW_MODE, and a value of 2 indicates standalone. But i've no
> > idea which pins they are.
> 
> What is the register I need to dump in the mv88e6xxx driver to check
> the value of SW_MODE?

This sort of thing, strapping, tends to appear in the scratch
registers. However, i cannot find any documentation about it. So i
have no idea where it is.

We probably need to go a different direction. Lets see if i understand
the issues correctly:

If there is an EEPROM, and the EEPROM contains a lot of data, it could
be that when we perform a hardware reset towards the end of probe, it
interrupts an I2C bus transaction, leaving the I2C bus in a bad state,
and future reads of the EEPROM do not work.

The work around for this was to poll the EEInt status and wait for it
to go true before performing the hardware reset.

However, we have discovered that for some boards which do not have an
EEPROM, EEInt never indicates complete. As a result,
mv88e6xxx_g1_wait_eeprom_done() spins for a second and then prints a
warning.

We probably need a different solution than calling
mv88e6xxx_g1_wait_eeprom_done(). The datasheet for 6352 documents the
EEPROM Command register:

bit 15 is:

  EEPROM Unit Busy. This bit must be set to a one to start an EEPROM
  operation (see EEOp below). Only one EEPROM operation can be
  executing at one time so this bit must be zero before setting it to
  a one.  When the requested EEPROM operation completes this bit will
  automatically be cleared to a zero. The transition of this bit from
  a one to a zero can be used to generate an interrupt (the EEInt in
  Global 1, offset 0x00).

and more interesting is bit 11:

  Register Loader Running. This bit is set to one whenever the
  register loader is busy executing instructions contained in the
  EEPROM.

We have the helper mv88e6xxx_g2_eeprom_wait() which polls both bit 15
and bit 11. Maybe we should use this instead of
mv88e6xxx_g1_wait_eeprom_done()?

	Andrew

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Fabio Estevam]

On Wed, Sep 20, 2023 at 11:52 AM Andrew Lunn <andrew@lunn.ch> wrote:

> We have the helper mv88e6xxx_g2_eeprom_wait() which polls both bit 15
> and bit 11. Maybe we should use this instead of
> mv88e6xxx_g1_wait_eeprom_done()?

I tested your suggestion as shown below (sorry for the bad formatting):

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index a73008b9e0b3..6c93f2da5568 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3012,14 +3012,14 @@ static void mv88e6xxx_hardware_reset(struct
mv88e6xxx_chip *chip)
  * from the wrong location resulting in the switch booting
  * to wrong mode and inoperable.
  */
- mv88e6xxx_g1_wait_eeprom_done(chip);
+ mv88e6xxx_g2_eeprom_wait(chip);

  gpiod_set_value_cansleep(gpiod, 1);
  usleep_range(10000, 20000);
  gpiod_set_value_cansleep(gpiod, 0);
  usleep_range(10000, 20000);

- mv88e6xxx_g1_wait_eeprom_done(chip);
+ mv88e6xxx_g2_eeprom_wait(chip);
  }
 }

diff --git a/drivers/net/dsa/mv88e6xxx/global2.c
b/drivers/net/dsa/mv88e6xxx/global2.c
index ec49939968fa..ac302a935ce6 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -340,7 +340,7 @@ int mv88e6xxx_g2_pot_clear(struct mv88e6xxx_chip *chip)
  * Offset 0x15: EEPROM Addr (for 8-bit data access)
  */

-static int mv88e6xxx_g2_eeprom_wait(struct mv88e6xxx_chip *chip)
+int mv88e6xxx_g2_eeprom_wait(struct mv88e6xxx_chip *chip)
 {
  int bit = __bf_shf(MV88E6XXX_G2_EEPROM_CMD_BUSY);
  int err;
diff --git a/drivers/net/dsa/mv88e6xxx/global2.h
b/drivers/net/dsa/mv88e6xxx/global2.h
index c05fad5c9f19..6d8d38944b23 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.h
+++ b/drivers/net/dsa/mv88e6xxx/global2.h
@@ -360,6 +360,8 @@ int mv88e6xxx_g2_trunk_clear(struct mv88e6xxx_chip *chip);
 int mv88e6xxx_g2_device_mapping_write(struct mv88e6xxx_chip *chip, int target,
        int port);

+int mv88e6xxx_g2_eeprom_wait(struct mv88e6xxx_chip *chip);
+
 extern const struct mv88e6xxx_irq_ops mv88e6097_watchdog_ops;
 extern const struct mv88e6xxx_irq_ops mv88e6250_watchdog_ops;
 extern const struct mv88e6xxx_irq_ops mv88e6390_watchdog_ops;

If this works for Alfred, I can submit it as a proper patch.

Thanks

Re: mv88e6xxx: Timeout waiting for EEPROM done

[Andrew Lunn]

> If this works for Alfred, I can submit it as a proper patch.

Hi Fabio

Thanks for testing. This is the correct concept. But there is one
detail to take care of.

Not all generations of switches support accessing the EEPROM. So
mv88e6xxx_g2_eeprom_wait() cannot be used unconditionally. You need to
test chip->info->ops->get_eeprom and if it is not NULL, then you know
it is supported. If it is NULL, then all i can suggest is we do
nothing and hope for the best with the reset.

For stable, such a test is sufficient. For net-next, we might consider
adding a .wait_eeprom to struct mv88e6xxx_ops.

       Andrew