Hi Stefan, On Wed, Jan 19, 2022 at 3:23 PM Tony Dinh <mibo...@gmail.com> wrote: > > fHi Stefan, > > On Wed, Jan 19, 2022 at 6:37 AM Stefan Roese <s...@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 |= (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 ;) ). >
Following up on this topic about phy_reset(). I've dug a bit deeper and now see that Kirkwood SoCs also have uclass mvgbe supports in arch/arm/mach-kirkwood/cpu.c. So all we need to do is to let the Kirkwood cpu_eth_init() function start the whole sequence of execution, and the uclass mvgbe would do all the work to bring up the network PHY! So that comes down to defining a function in the board file, similar to the Armada SoC boards. int board_eth_init(struct bd_info *bis) { return cpu_eth_init(bis); } Thanks, Tony