Re: [PATCH net-next v3 1/9] net: sunhme: Just restart autonegotiation if we can't bring the link up

[Sean Anderson]

On 3/18/23 04:37, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:05PM -0400, Sean Anderson wrote:
> > If we've tried regular autonegotiation and forcing the link mode, just
> > restart autonegotiation instead of reinitializing the whole NIC.
> >
> > Signed-off-by: Sean Anderson <seanga2@xxxxxxxxx>
>
> ...
>
> > @@ -606,6 +604,124 @@ static int is_lucent_phy(struct happy_meal *hp)
> >   	return ret;
> >   }
> >   
> > +/* hp->happy_lock must be held */
> > +static void
> > +happy_meal_begin_auto_negotiation(struct happy_meal *hp,
> > +				  void __iomem *tregs,
> > +				  const struct ethtool_link_ksettings *ep)
> > +{
> > +	int timeout;
> > +
> > +	/* Read all of the registers we are interested in now. */
> > +	hp->sw_bmsr      = happy_meal_tcvr_read(hp, tregs, MII_BMSR);
> > +	hp->sw_bmcr      = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
> > +	hp->sw_physid1   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID1);
> > +	hp->sw_physid2   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID2);
> > +
> > +	/* XXX Check BMSR_ANEGCAPABLE, should not be necessary though. */
> > +
> > +	hp->sw_advertise = happy_meal_tcvr_read(hp, tregs, MII_ADVERTISE);
> > +	if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
> > +		/* Advertise everything we can support. */
> > +		if (hp->sw_bmsr & BMSR_10HALF)
> > +			hp->sw_advertise |= (ADVERTISE_10HALF);
> > +		else
> > +			hp->sw_advertise &= ~(ADVERTISE_10HALF);
> > +
> > +		if (hp->sw_bmsr & BMSR_10FULL)
> > +			hp->sw_advertise |= (ADVERTISE_10FULL);
> > +		else
> > +			hp->sw_advertise &= ~(ADVERTISE_10FULL);
> > +		if (hp->sw_bmsr & BMSR_100HALF)
> > +			hp->sw_advertise |= (ADVERTISE_100HALF);
> > +		else
> > +			hp->sw_advertise &= ~(ADVERTISE_100HALF);
> > +		if (hp->sw_bmsr & BMSR_100FULL)
> > +			hp->sw_advertise |= (ADVERTISE_100FULL);
> > +		else
> > +			hp->sw_advertise &= ~(ADVERTISE_100FULL);
> > +		happy_meal_tcvr_write(hp, tregs, MII_ADVERTISE, hp->sw_advertise);
> > +
> > +		/* XXX Currently no Happy Meal cards I know off support 100BaseT4,
> > +		 * XXX and this is because the DP83840 does not support it, changes
> > +		 * XXX would need to be made to the tx/rx logic in the driver as well
> > +		 * XXX so I completely skip checking for it in the BMSR for now.
> > +		 */
> > +
> > +		ASD("Advertising [ %s%s%s%s]\n",
> > +		    hp->sw_advertise & ADVERTISE_10HALF ? "10H " : "",
> > +		    hp->sw_advertise & ADVERTISE_10FULL ? "10F " : "",
> > +		    hp->sw_advertise & ADVERTISE_100HALF ? "100H " : "",
> > +		    hp->sw_advertise & ADVERTISE_100FULL ? "100F " : "");
> > +
> > +		/* Enable Auto-Negotiation, this is usually on already... */
> > +		hp->sw_bmcr |= BMCR_ANENABLE;
> > +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
> > +
> > +		/* Restart it to make sure it is going. */
> > +		hp->sw_bmcr |= BMCR_ANRESTART;
> > +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
> > +
> > +		/* BMCR_ANRESTART self clears when the process has begun. */
> > +
> > +		timeout = 64;  /* More than enough. */
> > +		while (--timeout) {
> > +			hp->sw_bmcr = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
> > +			if (!(hp->sw_bmcr & BMCR_ANRESTART))
> > +				break; /* got it. */
> > +			udelay(10);
>
> nit: Checkpatch tells me that usleep_range() is preferred over udelay().
>       Perhaps it would be worth looking into that for a follow-up patch.

This will be fixed in another series.

> > +		}
> > +		if (!timeout) {
> > +			netdev_err(hp->dev,
> > +				   "Happy Meal would not start auto negotiation BMCR=0x%04x\n",
> > +				   hp->sw_bmcr);
> > +			netdev_notice(hp->dev,
> > +				      "Performing force link detection.\n");
> > +			goto force_link;
> > +		} else {
> > +			hp->timer_state = arbwait;
> > +		}
> > +	} else {
> > +force_link:
> > +		/* Force the link up, trying first a particular mode.
> > +		 * Either we are here at the request of ethtool or
> > +		 * because the Happy Meal would not start to autoneg.
> > +		 */
> > +
> > +		/* Disable auto-negotiation in BMCR, enable the duplex and
> > +		 * speed setting, init the timer state machine, and fire it off.
> > +		 */
> > +		if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
> > +			hp->sw_bmcr = BMCR_SPEED100;
> > +		} else {
> > +			if (ep->base.speed == SPEED_100)
> > +				hp->sw_bmcr = BMCR_SPEED100;
> > +			else
> > +				hp->sw_bmcr = 0;
> > +			if (ep->base.duplex == DUPLEX_FULL)
> > +				hp->sw_bmcr |= BMCR_FULLDPLX;
> > +		}
> > +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
> > +
> > +		if (!is_lucent_phy(hp)) {
> > +			/* OK, seems we need do disable the transceiver for the first
> > +			 * tick to make sure we get an accurate link state at the
> > +			 * second tick.
> > +			 */
> > +			hp->sw_csconfig = happy_meal_tcvr_read(hp, tregs,
> > +							       DP83840_CSCONFIG);
> > +			hp->sw_csconfig &= ~(CSCONFIG_TCVDISAB);
> > +			happy_meal_tcvr_write(hp, tregs, DP83840_CSCONFIG,
> > +					      hp->sw_csconfig);
> > +		}
> > +		hp->timer_state = ltrywait;
> > +	}
> > +
> > +	hp->timer_ticks = 0;
> > +	hp->happy_timer.expires = jiffies + (12 * HZ)/10;  /* 1.2 sec. */
>
> nit: as a follow-up perhaps you could consider something like this.
>       (* completely untested! * )
>
> 	hp->happy_timer.expires = jiffies + msecs_to_jiffies(1200);

ditto.

> > +	add_timer(&hp->happy_timer);
> > +}
> > +
>
> ...

--Sean