[PATCH] net: marvell: mvgbe: Set PHY page 0 before phy_connect
Tony Dinh
mibodhi at gmail.com
Tue Apr 26 00:01:10 CEST 2022
Hi Stefan,
On Mon, Apr 25, 2022 at 4:18 AM Stefan Roese <sr at denx.de> wrote:
>
> Hi Tony,
>
> On 4/25/22 11:33, Tony Dinh wrote:
> > Hi Stefan,
> >
> > On Sun, Apr 24, 2022 at 11:00 PM Stefan Roese <sr at denx.de> wrote:
> >>
> >> Hi Tony,
> >>
> >> On 4/23/22 04:15, Tony Dinh wrote:
> >>> 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
> >>
> >> Still very strange. Perhaps really some HW pin strapping responsible
> >> for this difference?
> >
> > It could be. The Zyxel Kirkwood NAS series NSA310s, 320, and 325 all
> > behave like this. The Sheevaplug and Dreamplug don't have this
> > behavior, while using the same network chip (I've confirmed that the
> > detected PHY id is the same, it is 1410e90).
> >
> >>
> >>> ========
> >>>
> >>> 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.
> >>
> >> Yes. And the MAC driver should not know anything about any PHY
> >> specific registers IMHO, such as PHY page or whatever. This needs
> >> to be done in the PHY driver instead.
> >>
> >>> 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!
> >>
> >> One more question: I know that currently phy_connect() does not work on
> >> this board, since the PHY page register is non-zero. Is the PHY not
> >> detected in this case (PHY ID matching etc)?
> >
> > Right, the phy_find_by_mask() in phy.c failed to create the PHY, since
> > it cannot match with anything.
>
> Too bad. Newer PHYs seem to mirror the PHY ID registers on all pages,
> so at least identifying does work there, regardless of the configured
> PHY page address. The 88e1310 seems to be inferior here unfortunately.
> > Note that it is the cold start that
> > failed. During a warm start, the PHY is always found as an existing
> > PHY (if the network is working already in the kernel, and a warm
> > reboot is executed).
> >
> > Here is the log for the failure to create PHY:
> >
> > phy.c phy_connect:
> > phy.c phy_find_by_mask 2
> > phy.c get_phy_device_by_mask: mask: 0x2
> > create_phy_by_mask phy mask 0x2
> > smi_reg_read: phy_addr 1 reg_ofs 2 devad -1
> > __mvgbe_mdio_read:(adr 1, off 2) value= 0000
> > smi_reg_read: phy_addr 1 reg_ofs 3 devad -1
> > __mvgbe_mdio_read:(adr 1, off 3) value= 0000
> > create_phy_by_mask phy mask 0x2
> > smi_reg_read: phy_addr 1 reg_ofs 2 devad 1
> > __mvgbe_mdio_read:(adr 1, off 2) value= 0000
> > smi_reg_read: phy_addr 1 reg_ofs 3 devad 1
> > __mvgbe_mdio_read:(adr 1, off 3) value= 0000
> > create_phy_by_mask phy mask 0x2
> > smi_reg_read: phy_addr 1 reg_ofs 2 devad 2
> > __mvgbe_mdio_read:(adr 1, off 2) value= 0000
> > smi_reg_read: phy_addr 1 reg_ofs 3 devad 2
> > __mvgbe_mdio_read:(adr 1, off 3) value= 0000
> > create_phy_by_mask phy mask 0x2
> > smi_reg_read: phy_addr 1 reg_ofs 2 devad 3
> > __mvgbe_mdio_read:(adr 1, off 2) value= 0000
> > smi_reg_read: phy_addr 1 reg_ofs 3 devad 3
> > __mvgbe_mdio_read:(adr 1, off 3) value= 0000
> > create_phy_by_mask phy mask 0x2
> > smi_reg_read: phy_addr 1 reg_ofs 2 devad 4
> > __mvgbe_mdio_read:(adr 1, off 2) value= 0000
> > smi_reg_read: phy_addr 1 reg_ofs 3 devad 4
> > __mvgbe_mdio_read:(adr 1, off 3) value= 0000
> > create_phy_by_mask phy mask 0x2
> > smi_reg_read: phy_addr 1 reg_ofs 2 devad 30
> > __mvgbe_mdio_read:(adr 1, off 2) value= 0000
> > smi_reg_read: phy_addr 1 reg_ofs 3 devad 30
> > __mvgbe_mdio_read:(adr 1, off 3) value= 0000
> > phy.c get_phy_device_by_mask: not found and could not create PHY for
> > ethernet-controller at 72000
> > Could not get PHY for ethernet-controller at 72000: addr 1
> > phy_connect failed
> > ping failed; host 192.168.0.224 is not alive
> >
> > While in the successful case the log is like this:
> >
> > phy.c phy_connect:
> > phy.c phy_find_by_mask 2
> > phy.c get_phy_device_by_mask: mask: 0x2
> > create_phy_by_mask phy mask 0x2
> > smi_reg_read: phy_addr 1 reg_ofs 2 devad -1
> > __mvgbe_mdio_read:(adr 1, off 2) value= 0141
> > smi_reg_read: phy_addr 1 reg_ofs 3 devad -1
> > __mvgbe_mdio_read:(adr 1, off 3) value= 0e90
> > phy.c phy_device_create phy_id 21040784
> > phy.c get_phy_driver PHY driver found - PHY id 1410e90
> >
> >>
> >> Did you try calling miiphy_reset() directly before calling phy_connect?
> >> Like this:
> >>
> >> /* Set phy address of the port */
> >> miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST,
> >> phyid);
> >>
> >> + /* Soft-reset the PHY, especially needed on the NSA310s */
> >> + miiphy_reset(dev->name, phyid);
> >> +
> >> phydev = phy_connect(bus, phyid, dev, phy_interface);
> >> if (!phydev) {
> >> printf("phy_connect failed\n");
> >> return NULL;
> >> }
> >>
> >> Does this work? Might be I missed something.
> >
> > No, it did not work. I've mentioned previously that I tried this
> > miiphy_reset at this location, and the board just hung. I needed to
> > recycle the power to reboot. I also tried the soft reset (phy_reset)
> > and it also hung.
>
> Thanks for clarifying.
>
> >> If this still does not work then yes, we might need some generic PHY
> >> page reset function. BTW, I introduced a marvell_write_page() function
> >> with this patch, which is still pending:
> >>
> >> https://www.mail-archive.com/u-boot@lists.denx.de/msg436238.html
> >
> > Oh. I already tried this patch and have not mentioned that to you yet.
> > This reg-init occurs too late in the execution path (in Marvel PHY
> > driver config function). It would have been possibly a great solution
> > to set up the register 22 earlier for this problem.
> >
> > While I think you're absolutely right above about the MAC driver being
> > separated from the PHY driver, I think there is probably a need for
> > some additional PHY initialization when necessary. Currently, very
> > early on, we have several phy_register calls to register all the
> > Marvell drivers in phy_marvell_init(), but that's the only thing it
> > does.
> >
> > I think we might be lacking a small but necessary glue between the
> > generic PHY driver (phy.c), mvgbe uclass, and the Marvell PHY driver.
>
> That might very well be the case. Perhaps one of the net custodians
> whats to chime in. Joe, Ramon?
>
> >> I still need to re-work this a bit.
> >>
> >> And also I just noticed that the Kernel has a .write_page PHY function
> >> and exports this via the common function phy_select_page().
> >
> > Cool! sounds like something promising.
>
> Yes, a very handy addition. Still, somebody needs to work on this and
> also make sure, that this "page access" support does not bloat the image
> size on all platforms, not in need of this "feature".
>
> > I also thought of another approach we can take is to have a weak
> > function in the mvgbe.c like,
> > __weak void __mvgbe_board_eth_init(const char *name, int phyaddr) {}
> > The mvgbe uclass can call it at the end of __mvgbe_init(). And this
> > NSA310S board file can define the concrete function to set the PHY
> > page.
>
> This is definitely easier than the generic approach envisioned above.
> But there weak functions do not scale well. Frankly, I tend a just go
> with your current approach with setting the PHY page address in this
> MAC driver and be done with it. One reason being, that this driver is
> pretty outdated and we will very unlikely see many other new Kirwood
> and/or Orion SoC based boards come up in the U-Boot source tree that
> need support non-Marvell PHYs. I'm not totally happy with this version
> but perhaps it's just "good enough" for these platforms at this stage.
>
> Comment welcome.
I think so too. It is good enough for old boards like the Kirkwood
SoCs. Not likely we will see these boards with non-Marvell PHYs. We
can go with this version to set the PHY page in this MAC driver.
When you have the marvell_write_page function already in the tree and
exported in the header file, I could try that, too. It's all academic,
but I think it will make it a bit better, because the info about the
page register (MII_MARVELL_PHY_PAGE) is encapsulated in marvell.c.
Thanks,
Tony
>
> > But this weak-function approach is too much a cop-out. If we can solve
> > this nicely within the current design it would be much better.
>
> Agreed.
>
> Thanks,
> Stefan
More information about the U-Boot
mailing list