Re: [PATCH v3 4/4] arm: kirkwood: Pogoplug-V4 : Add board implementation files

Stefan Roese
2022-01-19T14:38:08Z

Hi Tony,

On 1/19/22 02:31, Tony Dinh wrote:

<snip>

>>> +#if defined(CONFIG_RESET_PHY_R)
>>> +/* Configure and initialize PHY */
>>> +void reset_phy(void)
>>> +{
>>> + u16 reg;
>>> + int phyaddr;
>>> + char *name = "ethernet-controller@72000";
>>> + char *eth0_path = "/ocp@f1000000/ethernet-controller@72000";
>>> +
>>> + if (miiphy_set_current_dev(name))
>>> + return;
>>> +
>>> + phyaddr = fdt_get_phy_addr(eth0_path);
>>> + if (phyaddr < 0)
>>> + return;
>>> +
>>> + /*
>>> + * Enable RGMII delay on Tx and Rx for CPU port
>>> + * Ref: sec 4.7.2 of chip datasheet
>>> + */
>>> + miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 2);
>>> + miiphy_read(name, phyaddr, MV88E1116_MAC_CTRL_REG, &reg);
>>> + reg |= (MV88E1116_RGMII_RXTM_CTRL | MV88E1116_RGMII_TXTM_CTRL);
>>> + miiphy_write(name, phyaddr, MV88E1116_MAC_CTRL_REG, reg);
>>> + miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 0);
>>
>> Please take a look at drivers/net/phy/marvell.c / m88e1310_config().
>> Here you will find (amongst others):
>>
>> /* Set RGMII delay */
>> phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_PAGE, 0x0002);
>> reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_RGMII_CTRL);
>> reg |= 0x0030;
>> phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_RGMII_CTRL, reg);
>>
>> Can't you use the Marvell PHY driver instead of this ad-hoc
>> implementation? I didn't check the compatibilty of your PHY and this
>> driver though.
>
> Thanks for the advice. Marek had a similar comment regarding this code.
>
> https://lists.denx.de/pipermail/u-boot/2021-December/470019.html
>
> Marek said,
> "There the m88e1118_config() method already does one thing of what you
> are doing here: enabling rgmii delays. It also sets LED config, but
> does not reset the PHY. You can add call to phy_reset() there..."
>
> Currently, all other Kirkwood boards also use this old ad-hoc
> implementation. And the PHY addr is different in some of these boards.
> If we add phy_reset() in m88e1118_config(), we would need to make sure
> all existing boards that use the driver will work... OTOH, perhaps we
> can keep the phy_reset() call in each Kirkwood board...

I should be checked, if this phy_reset() is really necessary. Perhaps
by checking in the Linux Kernel code as well and the documentation of
the PHY.

> Sounds like an approach that will potentially touch a common area, so
> I think it is best that I will do the modernization and more testing
> in a separate patch series, after we're done with this Pogo V4 board.
>
> Does that sound reasonable?

Yes, I'm fine with this approach. Let's handle this in some follow-up
patches ( from you ;) ).

Thanks,
Stefan