[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 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.

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.