[PATCH 1/2] net: ravb: Add tx/rx delay flag checks and support for rgmii-rxid

[Adam Ford]

On Sun, Feb 20, 2022 at 5:58 PM Marek Vasut <marex at denx.de> wrote:
>
> On 2/20/22 22:45, Adam Ford wrote:
>
> Hi,
>
> [...]
>
> > @@ -376,6 +377,8 @@ static int ravb_dmac_init(struct udevice *dev)
> >       struct ravb_priv *eth = dev_get_priv(dev);
> >       struct eth_pdata *pdata = dev_get_plat(dev);
> >       int ret = 0;
> > +     int mode = 0;
> > +     unsigned int delay;
> >
> >       /* Set CONFIG mode */
> >       ret = ravb_reset(dev);
> > @@ -402,9 +405,25 @@ static int ravb_dmac_init(struct udevice *dev)
> >           (rmobile_get_cpu_type() == RMOBILE_CPU_TYPE_R8A77995))
> >               return 0;
> >
> > -     if ((pdata->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> > -         (pdata->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID))
> > -             writel(APSR_TDM, eth->iobase + RAVB_REG_APSR);
> > +     if (!dev_read_u32(dev, "tx-internal-delay-ps", &delay)) {
> > +             if (delay)
> > +                     mode |= APSR_TDM;
> > +     }
> > +
> > +     if (!dev_read_u32(dev, "rx-internal-delay-ps", &delay)) {
> > +             if (delay)
> > +                     mode |= APSR_RDM;
> > +     }
>
> Are these two conditionals above really needed ?

These two conditionals are present in the Linux kernel version [1], so
I assumed there is a reason and copied that.

The way Geert reconfigured the ethernet for the Beacon board [2][3],
he changed the mode rgmii-rxid and it already had both tx and rx
delays.  The rgmii-rxid flag by itself implies the APSR_RDM flag is
set, but adding the tx-internal-delay-ps appears to add the APSR_TDM.
I am not that familiar with phys to know the main differences between
this configuration and the PHY_INTERFACE_MODE_RGMII_ID which appears
to imply both.  If you want, I could merge the checks so the delays
are combined with the phy_interface type that follows.

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/renesas/ravb_main.c?h=v5.17-rc4
[2] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi?h=v5.17-rc4&id=59a8bda062f8646d99ff8c4956adf37dee1cb75e
[3] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi?h=v5.17-rc4&id=18a2427146bf8a3da8fc7825051d6aadb9c2d8fb

adam
>
> Isn't it enough to set the PHY mode to rgmii-NNN like it is checked below ?
>
> > +     if (pdata->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +         pdata->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> > +             mode |= APSR_RDM;
> > +
> > +     if (pdata->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +         pdata->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +             mode |= APSR_TDM;
> > +
> > +     writel(mode, eth->iobase + RAVB_REG_APSR);
> >
> >       return 0;
> >   }
>
> [...]