Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Sean Anderson <sean.anderson@seco.com>]

Hi Russell,

On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
> On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
>> +static void macb_pcs_get_state(struct phylink_pcs *pcs,
>> +			       struct phylink_link_state *state)
>> +{
>> +	struct macb *bp = pcs_to_macb(pcs);
>> +
>> +	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
>> +		state->interface = PHY_INTERFACE_MODE_SGMII;
>> +	else
>> +		state->interface = PHY_INTERFACE_MODE_1000BASEX;
>
> There is no requirement to set state->interface here. Phylink doesn't
> cater for interface changes when reading the state. As documented,
> phylink will set state->interface already before calling this function
> to indicate what interface mode it is currently expecting from the
> hardware.

Ok, so instead I should be doing something like

if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
	interface = PHY_INTERFACE_MODE_SGMII;
else
	interface = PHY_INTERFACE_MODE_1000BASEX;

if (interface != state->interface) {
	state->link = 0;
	return;
}

?

>> +static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
>> +			      phy_interface_t interface,
>> +			      const unsigned long *advertising)
>> +{
>> +	bool changed = false;
>> +	u16 old, new;
>> +
>> +	old = gem_readl(bp, PCSANADV);
>> +	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
>> +						       old);
>> +	if (old != new) {
>> +		changed = true;
>> +		gem_writel(bp, PCSANADV, new);
>> +	}
>> +
>> +	old = new = gem_readl(bp, PCSCNTRL);
>> +	if (mode == MLO_AN_INBAND)
>
> Please use phylink_autoneg_inband(mode) here.

Ok.

>
>> +		new |= BMCR_ANENABLE;
>> +	else
>> +		new &= ~BMCR_ANENABLE;
>> +	if (old != new) {
>> +		changed = true;
>> +		gem_writel(bp, PCSCNTRL, new);
>> +	}
>
> There has been the suggestion that we should allow in-band AN to be
> disabled in 1000base-X if we're in in-band mode according to the
> ethtool state.

This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
another _encode variant? I hadn't done this here because the logic was
only one if statement.

> I have a patch that adds that.

Have you posted it?

>> +	return changed;
>> +}
>> +
>> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>> +			   phy_interface_t interface,
>> +			   const unsigned long *advertising,
>> +			   bool permit_pause_to_mac)
>> +{
>> +	bool changed = false;
>> +	struct macb *bp = pcs_to_macb(pcs);
>> +	u16 old, new;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&bp->lock, flags);
>> +	old = new = gem_readl(bp, NCFGR);
>> +	if (interface == PHY_INTERFACE_MODE_SGMII) {
>> +		new |= GEM_BIT(SGMIIEN);
>> +	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
>> +		new &= ~GEM_BIT(SGMIIEN);
>> +	} else {
>> +		spin_lock_irqsave(&bp->lock, flags);
>> +		return -EOPNOTSUPP;
>
> You can't actually abort at this point - phylink will print the error
> and carry on regardless. The checking is all done via the validate()
> callback and if that indicates the interface mode is acceptable, then
> it should be accepted.

Ok, so where can the PCS NAK an interface? This is the only callback
which has a return code, so I assumed this was the correct place to say
"no, we don't support this." This is what lynx_pcs_config does as well.

>>  static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
>>  	.pcs_get_state = macb_usx_pcs_get_state,
>>  	.pcs_config = macb_usx_pcs_config,
>> -	.pcs_link_up = macb_usx_pcs_link_up,
>>  };
>>
>>  static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
>>  	.pcs_get_state = macb_pcs_get_state,
>> -	.pcs_an_restart = macb_pcs_an_restart,
>
> You don't want to remove this.

Hm, I didn't realized I had removed this, so I add it back in the next
patch. I will merge that one into this one.


> When operating in 1000BASE-X mode, it
> will be called if a restart is required (e.g. macb_pcs_config()
> returning positive, or an ethtool request.) You need to keep the empty
> function.

Ok, perhaps I can add some sanity checks for this in pcs_register.

--Sean

> That may also help the diff algorithm to produce a cleaner
> patch too.