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

fHi Stefan,

On Wed, Jan 19, 2022 at 6:37 AM Stefan Roese sr@denx.de wrote:

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.

I believe it is necessary. During testing Kirwood boards with Marvel Alaska chip, if the PHY reset call is timed out, we don't have a network. But I will try without phy_reset() using the new driver to see how it behaves.

...

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 ;) ).

I will :)

Also, I will resend this series to rearrange the patch order.

Thanks, Tony

Thanks, Stefan