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

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

[Adam Ford]

Some boards like the Beacon RZ/G2 SOM use either flags for
tx-internal-delay-ps, rx-internal-delay-ps or rgmii-rxid.

In Linux the APSR_RDM flag is set when either rx-internal-delay-ps
is set or the mode is rgmii-rxid, and the APSR_TDM is set when
tx-internal-delay-ps is found or rgmii-txid is set, and both
are set if rgmii-id is set.

The ravb driver in U-Boot driver was missing rgmii-rxid support,
so add that support in a similar fashion to what is done in Linux.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
index 1d1118c341..9e09e998e3 100644
--- a/drivers/net/ravb.c
+++ b/drivers/net/ravb.c
@@ -52,6 +52,7 @@
 #define CSR_OPS			0x0000000F
 #define CSR_OPS_CONFIG		BIT(1)
 
+#define APSR_RDM		BIT(13)
 #define APSR_TDM		BIT(14)
 
 #define TCCR_TSRQ0		BIT(0)
@@ -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;
+	}
+
+	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;
 }
-- 
2.32.0

[PATCH 2/2] arm: rmobile: rzg2_beacon: Enable proper Ethernet PHY

[Adam Ford]

The wrong phy was being enabled, because it worked and the proper
PHY did not.  After the Renesas maintainer made some adjustments
to the device tree, Linux was able to use the proper driver, and
when that device tree was ported to Linux, the ethernet stopped
working due to the lack of rgmii-rxid support.  Now that
rgmii-rxid is supported, enable the proper driver to restore
ethernet function.

Fixes: 1eaf61c84db6 ("arm: dts: beacon-rzg2: Resync device trees with Linux 5.16-rc3")
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/configs/rzg2_beacon_defconfig b/configs/rzg2_beacon_defconfig
index e6a0d68962..91b3fa2948 100644
--- a/configs/rzg2_beacon_defconfig
+++ b/configs/rzg2_beacon_defconfig
@@ -62,7 +62,7 @@ CONFIG_DM_MTD=y
 CONFIG_DM_SPI_FLASH=y
 CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_BITBANGMII=y
-CONFIG_PHY_REALTEK=y
+CONFIG_PHY_ATHEROS=y
 CONFIG_DM_ETH=y
 CONFIG_RENESAS_RAVB=y
 CONFIG_DM_REGULATOR=y
-- 
2.32.0

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

[Marek Vasut]

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 ?

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;
>   }

[...]

Re: [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@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;
> >   }
>
> [...]

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

[Marek Vasut]

On 2/21/22 02:45, Adam Ford wrote:
> On Sun, Feb 20, 2022 at 5:58 PM Marek Vasut <marex@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.

The kernel driver has comments there which clarify this:

drivers/net/ethernet/renesas/ravb_main.c
ravb_parse_delay_mode()

if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay)) {
/* Valid values are 0 and 1800, according to DT bindings */

if (!of_property_read_u32(np, "tx-internal-delay-ps", &delay)) {
/* Valid values are 0 and 2000, according to DT bindings */

So these internal-delay-ps have only two states, 0 or some-delay. Retain 
the comments, otherwise it is real inobvious why there are two identical 
ways to set the delays.

btw. the kernel driver also has this $explicit_delay check and if the 
internal-delay-ps is present , it skips the phy-mode PH_INTERFACE_MODE_ 
check, that's missing in this patch.