[PATCH] net: marvell: mvgbe: Set PHY page 0 before phy_connect
Tony Dinh
mibodhi at gmail.com
Thu Apr 21 23:21:53 CEST 2022
Hi Stefan,
On Thu, Apr 21, 2022 at 7:26 AM Stefan Roese <sr at denx.de> wrote:
>
> Hi Tony,
>
> On 4/21/22 03:45, Tony Dinh wrote:
> > Hi Stefan,
> >
> > On Tue, Apr 19, 2022 at 1:47 PM Tony Dinh <mibodhi at gmail.com> wrote:
> >>
> >> Hi Stefan,
> >>
> >> On Tue, Apr 19, 2022 at 3:29 AM Stefan Roese <sr at denx.de> wrote:
> >>>
> >>> Hi Tony,
> >>>
> >>> On 4/12/22 22:18, Tony Dinh wrote:
> >>>> For most Kirkwood boards, the PHY page is already set to page 0
> >>>> (in register 22) before phy_connect is invoked. But some board like
> >>>> the Zyxel NSA310S (which uses the network chip MV88E1318S), the PHY page
> >>>> is not set to page 0. There seems to be some bad data remained in
> >>>> register 22 when the uclass MVGBE about to invoke phy_connect().
> >>>>
> >>>> This patch enables the uclass MVGBE to always set the PHY page to 0
> >>>> before phy_connect.
> >>>>
> >>>> For reference, please see this discussion:
> >>>> [RFC PATCH v2] arm: kirkwood: nsa310s: Use Marvell uclass mvgbe
> >>>> and PHY driver for DM Ethernet.
> >>>> https://lists.denx.de/pipermail/u-boot/2022-April/480946.html
> >>>
> >>> I don't think that this is the correct approach. We should not set
> >>> the *Marvell* PHY page address in this MAC driver IMHO. This driver
> >>> should not have any PHY specific changes. This should be done in the
> >>> PHY driver instead. Some PHY driver or board specific code seems to be
> >>> responsible for setting a non-zero PHY page and not resetting it back
> >>> to zero. A better solution would be to locate this code path and add
> >>> this page reset to 0 there.
> >>>
> >>> What do you think?
> >>
> >> My initial thought was similar to yours, in that if we can do this in
> >> the Marvell PHY driver, it would be the best. However, the Marvell PHY
> >> driver start() and config() are invoked too late in the code calling
> >> sequence. The phy_connect() would fail if the page is not 0. And since
> >> we have the mmi_phy_write() call to set the "Set phy address of the
> >> port" (see code excerpt below), I thought it is also appropriate to
> >> set the page to 0 here.
> >>
> >> I realized it is an assumption to say because this MVGBE uclass is
> >> only used for the Marvell Alaska chip in the Kirkwood boards, the
> >> register 22 is the correct one.
> >>
> >> So perhaps we need a suggestion from the maintainers (to: cc: on this
> >> email) where it would be the best place to do this. In the old ad-hoc
> >> code, this setting to page 0 was done in the board file reset_phy(),
> >> which is invoked early on by board_r.
> >
> > To add more info to this discussion, below is the code path for this
> > mvgbe uclass. As you can see in the boot log below, in DM Ethernet for
> > Kirkwood boards, no board code is involved at all. The mvgbe uclass is
> > all that we have executed before the phy_connect(). And the Marvell
> > PHY driver code is executed very late, after the PHY has been
> > connected. So I think whatever we do probably should be in this
> > uclass.
>
> 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.
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.
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);
Thanks,
Tony
> Thanks,
> Stefan
>
> > There is a struct in drivers/net/mvgbe.h that seems to be where the
> > Page Select register could be. I'm still trying to decipher these
> > struct members and what they are, and whether it is possible to set
> > the register to 0. Very hard to understand this code without any
> > documentation.
> >
> > struct mvgbe_registers {
> > u32 phyadr;
> > u32 smi;
> > u32 euda;
> > u32 eudid;
> > <snip>
> >
> > Thanks,
> > Tony
> >
> > ============================================
> >
> > Begin boot log (phy_connect failed because register 22 is non 0):
> >
> > U-Boot 2022.04-00569-gca51a8dc04-dirty (Apr 20 2022 - 15:01:37 -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
> > eth0: ethernet-controller at 72000
> > Hit any key to stop autoboot: 0
> > NSA310s> ping 192.168.0.224
> > mvgbe_start UCLASS_ETH mvgbe
> > mvgbe_start PHY addr 1
> > __mvgbe_init
> > __mvgbe_init MVGBE_PGADR_REG at start value = 0x11
> > __mvgbe_init successful, returning 0
> > __mvgbe_init and MVGBE_PGADR_REG at end value = 0x11
> > __mvgbe_phy_init phy name = ethernet-controller at 72000
> > __mvgbe_phy_init phyid = 1
> > __mvgbe_phy_init phy_interface = rgmii
> > __mvgbe_phy_init MVGBE_PGADR_REG current value = 0x11
> > 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
> > create_phy_by_mask phy mask 0x2
> > create_phy_by_mask phy mask 0x2
> > create_phy_by_mask phy mask 0x2
> > create_phy_by_mask phy mask 0x2
> > create_phy_by_mask phy mask 0x2
> > 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
> >
> > End log
> > =============
> >
> >
> >> Thanks,
> >> Tony
> >>
> >> =================
> >>
> >> drivers/net/mvgbe.c
> >>
> >> static struct phy_device *__mvgbe_phy_init(struct udevice *dev,
> >> struct mii_dev *bus,
> >> phy_interface_t phy_interface,
> >> int phyid)
> >> #else
> >> static struct phy_device *__mvgbe_phy_init(struct eth_device *dev,
> >> struct mii_dev *bus,
> >> phy_interface_t phy_interface,
> >> int phyid)
> >> #endif
> >> {
> >> struct phy_device *phydev;
> >>
> >> /* Set phy address of the port */
> >> miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST,
> >> phyid);
> >>
> >> /* Make sure the selected PHY page is 0 before connecting */
> >> miiphy_write(dev->name, phyid, MVGBE_PGADR_REG, 0);
> >>
> >> phydev = phy_connect(bus, phyid, dev, phy_interface);
> >> if (!phydev) {
> >> printf("phy_connect failed\n");
> >> return NULL;
> >> }
> >>
> >> phy_config(phydev);
> >> phy_startup(phydev);
> >>
> >> return phydev;
> >> }
> >> #endif /* CONFIG_PHYLIB || CONFIG_DM_ETH */
> >>
> >
> >
> >
> >>
> >>
> >>> Thanks,
> >>> Stefan
> >>>
> >>>> This patch has been tested with the following Kirkwood boards:
> >>>>
> >>>> NSA310S (88F6702, network chip MV88E1318S)
> >>>> Sheevaplug (88F6281, network chip MV88E1318)
> >>>> Pogo V4 (88F6192, network chip 88E1116R)
> >>>> GF Home(88F6281, network chip 88E1116R)
> >>>> Dreamplug (88F6281, network chip MV88E1318)
> >>>> Dell Kace M300 (88F6282, network chip MV88E1318) - out of tree u-boot
> >>>>
> >>>> Signed-off-by: Tony Dinh <mibodhi at gmail.com>
> >>>> ---
> >>>>
> >>>> drivers/net/mvgbe.c | 4 ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c
> >>>> index 954bf86121..d2db1e584a 100644
> >>>> --- a/drivers/net/mvgbe.c
> >>>> +++ b/drivers/net/mvgbe.c
> >>>> @@ -43,6 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
> >>>>
> >>>> #define MV_PHY_ADR_REQUEST 0xee
> >>>> #define MVGBE_SMI_REG (((struct mvgbe_registers *)MVGBE0_BASE)->smi)
> >>>> +#define MVGBE_PGADR_REG 22
> >>>>
> >>>> #if defined(CONFIG_PHYLIB) || defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
> >>>> static int smi_wait_ready(struct mvgbe_device *dmvgbe)
> >>>> @@ -745,6 +746,9 @@ static struct phy_device *__mvgbe_phy_init(struct eth_device *dev,
> >>>> miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST,
> >>>> phyid);
> >>>>
> >>>> + /* Make sure the selected PHY page is 0 before connecting */
> >>>> + miiphy_write(dev->name, phyid, MVGBE_PGADR_REG, 0);
> >>>> +
> >>>> phydev = phy_connect(bus, phyid, dev, phy_interface);
> >>>> if (!phydev) {
> >>>> printf("phy_connect failed\n");
> >>>
> >>> Viele Grüße,
> >>> Stefan Roese
> >>>
> >>> --
> >>> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> >>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> >>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
>
> Viele Grüße,
> Stefan Roese
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
More information about the U-Boot
mailing list