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.