[PATCH] net: marvell: mvgbe: Set PHY page 0 before phy_connect

[Tony Dinh]

Hi Stefan,

Please see my various comments below. And my thoughts at the end.

On Thu, Apr 21, 2022 at 11:15 PM Stefan Roese <sr at denx.de> wrote:
>
> Hi Tony,
>
> On 4/21/22 23:21, Tony Dinh wrote:
>
> <snip>
>
> >> What really puzzles me is, why the page address is set to a non-zero
> >> value at all at this early boot stage? Could you perhaps add some
> >> debugging code, to check, if and where the page address gets set?
> >> I find it hard to belief, that this starts with non-zero after
> >> powering up the device / PHY.
> >>
> >> Or did I miss something?
> >
> > Other Kirkwood boards behave correctly (such as the Sheevaplug,
> > Dreamplug, and Dell Kace M300). The Page Select register (22) contains
> > 0 in these boards, and all have PHY id 1410e90.  The NSA310s also has
> > PHY id 1410e90.
>
> Yes. I'm pretty sure that the page select register is set to 0 upon
> PHY startup. Even though there might be some strapping possibilities
> to configure some PHY registers, the page select is most likely always
> 0 after power-up. So if nobody writes to this reg, then is should be 0.

Agree. All other Kirkwood boards execute the same code so I think we
would see if somebody writes to this register.

> > But I could not find in the uclass MVGBE where the Page Select
> > register is set before phy_connect is called. So my guess is that
> > memory location just happens to be zero in other boards but not in
> > this NSA310S board. Perhaps the memory location needs to be set to
> > zero, to make sure all registers point to page 0 in the beginning.
>
> Please see above.
>
> > Possibly, it is here that the Page Select register should be zero out
> > after the priv data is copied:  dev_get_priv(). mvgbe_of_to_plat() is
> > called very early on (during the uclass MVGBE creation).
> >
> > static int mvgbe_of_to_plat(struct udevice *dev)
> > {
> > struct eth_pdata *pdata = dev_get_plat(dev);
> > struct mvgbe_device *dmvgbe = dev_get_priv(dev);
>
> Possibly. Again my suggestion is to add some debug code to check at
> different boot times, which value is currently set in the page select
> register. By just reading is out and printing it's value. You might need
> to add some "special code" at the early code paths, as the MDIO driver
> is not started there.
>
> Another idea is, if it's possible to issue a HW-reset to the PHY on the
> NSA310 board. Do you know if some GPIO is connected to the PHY's reset
> pin which could be toggled by the SoC?

I don't think there is a GPIO that does. I looked at the GPL source
code for this board from way back, and created the DTS for this based
on info in that GPL source. I don't recall that it was available.

> Note: We could of course just add the reset to 0 as you have done in the
> MAC driver or some board specific code. But I really would like to
> understand why the page select reg is non-zero in this case.

My conclusion is the register was polluted by something in the board
hardware. This is the comments (paraphrase)  we got from this board
GPL source:
        /* The ZyXEL NSA310S uses the 88E1310S Alaska (interface
identical to 88E1318) */
        /* and has an MCU attached to the LED[2] via tristate interrupt */

I've rebuilt and rerun the tests for both the Sheevaplug and NSA310S.
Please see the debug log from mvgbe_probe. This is as early as we can
see the uclass initializing.

==== NSA310S boot log
U-Boot 2022.04-00569-gca51a8dc04-dirty (Apr 22 2022 - 16:49:50 -0700)
ZyXEL NSA310S/320S 1/2-Bay Power Media Server

SoC:   Kirkwood 88F6281_A1
DRAM:  256 MiB
Core:  7 devices, 5 uclasses, devicetree: separate
NAND:  128 MiB
Loading Environment from NAND... OK
In:    serial
Out:   serial
Err:   serial

Net:   mvgbe_of_to_plat called
mvgbe_of_to_plat phy-mode 7
mvgbe_of_to_plat phy addr 1
mvgbe_probe called
smi_reg_read: phy_addr 1 reg_ofs 22 devad  -1
__mvgbe_mdio_read:(adr 1, off 22) value= 0011
eth0: ethernet-controller at 72000
Hit any key to stop autoboot:  0

===== Sheevaplug boot log

U-Boot 2022.04-00569-gca51a8dc04-dirty (Apr 22 2022 - 18:16:25 -0700)
Marvell-Sheevaplug

SoC:   Kirkwood 88F6281_A1
DRAM:  512 MiB
Core:  9 devices, 7 uclasses, devicetree: separate
NAND:  512 MiB
MMC:   mvsdio at 90000: 0
Loading Environment from NAND... OK
In:    serial
Out:   serial
Err:   serial

Net:   mvgbe_of_to_plat called
mvgbe_of_to_plat phy-mode 1
mvgbe_of_to_plat phy addr 0
mvgbe_probe called
smi_reg_read: phy_addr 0 reg_ofs 22 devad  -1
__mvgbe_mdio_read:(adr 0, off 22) value= 0000
eth0: ethernet-controller at 72000
Hit any key to stop autoboot:  0

========

My thoughts:

I think we probably need to refactor some constants in the uclass
drivers/net/mvgbe.c and/or create a helper in  the Marvell PHY driver
drivers/net/phy/marvell.c.

The mvgbe uclass is a generic Ethernet class for all Kirkwood and
Orion-5x boards (CONFIG_ARCH_KIRKWOOD and CONFIG_ARCH_ORION5X).
However, as of right now, it lacks knowledge about specific things
such as the PHY Page Select register for a specific network chip.
Those constants are defined only in drivers/net/phy/marvell.c. For
example,

#define MII_MARVELL_PHY_PAGE            22
#define MIIM_88E1121_PHY_PAGE           22
#define MIIM_88E1145_PHY_PAGE   29
#define MIIM_88E1310_PHY_PAGE           22

When the mvgbe uclass calls mvgbe_of_to_plat() during initialization,
it extracts several parameters from the DTS, such as PHY address, but
has no way to "reset" PHY page in a general way so it will work for
all network chips used in Kirkwood and Orion-5x boards.

I think my hack to reset the PHY page to 0 would not work for the
88E1145 chip as seen above. But none of the Kirkwood boards uses this
chip, AFAIK.

What do you think? Would the patch be OK as is, or perhaps we need to
add a helper to drivers/net/phy/marvell.c to get the PHY Page Select
register? or perhaps you have other thoughts about the solution!

Thanks,
Tony

>
> Thanks,
> Stefan