Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Sun, May 21, 2023 at 06:16:50PM +0200, David Epping wrote:
> On Sun, May 21, 2023 at 04:43:56PM +0300, Vladimir Oltean wrote:
> > Not only bit 11 is reserved for VSC8530, but it's also read-only, so it
> > should not matter what is written there.
> 
> I agree and am ok with removing the PHY ID condition.
> 
> > Since vsc85xx_rgmii_enable_rx_clk() and vsc85xx_rgmii_set_skews() write
> > to the same register, would it not make sense to combine the two into a
> > single phy_modify_paged() call, and to zeroize bit 11 as part of that?
> 
> Since we found an explanation why the current driver works in some
> setups (U-Boot), I would go with the Microchip support statement, that
> writing bit 11 to 0 is required in all modes.
> It would thus stay a separate function, called without a phy mode
> condition, and not be combined with the RGMII skew setting function.
> 
> > The other caller of vsc85xx_rgmii_set_skews(), VSC8572, unfortunately
> > does not document bit 11 at all - it doesn't say if it's read-only or not.
> > We could conditionally include the VSC8502_RGMII_RX_CLK_DISABLE bit in the
> > "mask" argument of phy_modify_paged() based on rgmii_cntl == VSC8502_RGMII_CNTL,
> > such as to exclude VSC8572.
> 
> Because of the above, I would still call from vsc85xx_default_config(),
> so not for the PHYs where bit 11 is not documented.
> 
> > What do you think?
> 
> If you agree to the above, should the function be named
> vsc85xx_enable_rx_clk() or rather vsc8502_enable_rx_clk()?
> It is called for more than just VSC8502, but not for all of the PHYs
> the driver supports.
> The same is true for the existing vsc85xx_default_config(), however.
> I don't have a real preference.

Well, to be clear, I was suggesting:

/* Set the RGMII RX and TX clock skews individually, according to the PHY
 * interface type, to:
 *  * 0.2 ns (their default, and lowest, hardware value) if delays should
 *    not be enabled
 *  * 2.0 ns (which causes the data to be sampled at exactly half way between
 *    clock transitions at 1000 Mbps) if delays should be enabled
 */
static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
				   u16 rgmii_rx_delay_mask,
				   u16 rgmii_tx_delay_mask)
{
	u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1;
	u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
	u16 mask = rgmii_rx_delay_mask | rgmii_tx_delay_mask;
	u16 reg_val = 0;
	int rc;

	/* For traffic to pass, the VSC8502 family needs the RX_CLK disable bit
	 * to be unset for all PHY modes, so do that as part of the paged
	 * register modification.
	 */
	if (rgmii_cntl == VSC8502_RGMII_CNTL)
		mask |= VSC8502_RGMII_RX_CLK_DISABLE;

	mutex_lock(&phydev->lock);

	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
		reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_rx_delay_pos;
	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
		reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_tx_delay_pos;

	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
			      rgmii_cntl, mask, reg_val);

	mutex_unlock(&phydev->lock);

	return rc;
}

static int vsc85xx_default_config(struct phy_device *phydev)
{
	int rc;

	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;

	return vsc85xx_rgmii_set_skews(phydev, VSC8502_RGMII_CNTL,
				       VSC8502_RGMII_RX_DELAY_MASK,
				       VSC8502_RGMII_TX_DELAY_MASK);
}

// no changes to the vsc85xx_rgmii_set_skews() call from vsc8584_config_init()