[PATCH] net: marvell: mvgbe: Set PHY page 0 before phy_connect
Stefan Roese
sr at denx.de
Thu Apr 21 16:26:41 CEST 2022
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?
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