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

[Tony Dinh]

Hi Stefan,

On Wed, Jan 19, 2022 at 3:23 PM Tony Dinh <mibodhi at gmail.com> wrote:
>
> fHi Stefan,
>
> On Wed, Jan 19, 2022 at 6:37 AM Stefan Roese <sr at 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 at 72000";
> > >>> +     char *eth0_path = "/ocp at f1000000/ethernet-controller at 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 ;) ).
>

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