[PATCH net-next v3 0/9] net: sunhme: Probe/IRQ cleanups

[PATCH net-next v3 0/9] net: sunhme: Probe/IRQ cleanups

[Sean Anderson]

Well, I've had these patches kicking around in my tree since last October, so I
guess I had better get around to posting them. This series is mainly a
cleanup/consolidation of the probe process, with some interrupt changes as well.
Some of these changes are SBUS- (AKA SPARC-) specific, so this should really get
some testing there as well to ensure nothing breaks. I've CC'd a few SPARC
mailing lists in hopes that someone there can try this out. I also have an SBUS
card I ordered by mistake if anyone has a SPARC computer but lacks this card.

I had originally planned on adding phylib support to this driver in the hopes of
being able to use real phy drivers, but I don't think I'm going to end up doing
that. I wanted to be able to use an external (homegrown) phy, but as it turns
out you can't buy MII cables in $CURRENTYEAR for under $250 a pop, and even if
you could get them you can't buy the connectors either. Oh well...

Changes in v3:
- Incorporate a fix from another series into this commit

Changes in v2:
- Move happy_meal_begin_auto_negotiation earlier and remove forward declaration
- Make some more includes common
- Clean up mac address init
- Inline error returns

Sean Anderson (9):
  net: sunhme: Just restart autonegotiation if we can't bring the link
    up
  net: sunhme: Remove residual polling code
  net: sunhme: Unify IRQ requesting
  net: sunhme: Alphabetize includes
  net: sunhme: Switch SBUS to devres
  net: sunhme: Consolidate mac address initialization
  net: sunhme: Clean up mac address init
  net: sunhme: Inline error returns
  net: sunhme: Consolidate common probe tasks

 drivers/net/ethernet/sun/sunhme.c | 1161 ++++++++++-------------------
 drivers/net/ethernet/sun/sunhme.h |    6 +-
 2 files changed, 415 insertions(+), 752 deletions(-)

-- 
2.37.1

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

[Sean Anderson]

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@gmail.com>
---

(no changes since v2)

Changes in v2:
- Move happy_meal_begin_auto_negotiation earlier and remove forward declaration

 drivers/net/ethernet/sun/sunhme.c | 245 +++++++++++++++---------------
 1 file changed, 119 insertions(+), 126 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index b0c7ab74a82e..65733ee5ddd9 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -589,8 +589,6 @@ static int set_happy_link_modes(struct happy_meal *hp, void __iomem *tregs)
 	return 1;
 }
 
-static int happy_meal_init(struct happy_meal *hp);
-
 static int is_lucent_phy(struct happy_meal *hp)
 {
 	void __iomem *tregs = hp->tcvregs;
@@ -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);
+		}
+		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. */
+	add_timer(&hp->happy_timer);
+}
+
 static void happy_meal_timer(struct timer_list *t)
 {
 	struct happy_meal *hp = from_timer(hp, t, happy_timer);
@@ -743,12 +859,7 @@ static void happy_meal_timer(struct timer_list *t)
 					netdev_notice(hp->dev,
 						      "Link down, cable problem?\n");
 
-					ret = happy_meal_init(hp);
-					if (ret) {
-						/* ho hum... */
-						netdev_err(hp->dev,
-							   "Error, cannot re-init the Happy Meal.\n");
-					}
+					happy_meal_begin_auto_negotiation(hp, tregs, NULL);
 					goto out;
 				}
 				if (!is_lucent_phy(hp)) {
@@ -1201,124 +1312,6 @@ static void happy_meal_init_rings(struct happy_meal *hp)
 	HMD("done\n");
 }
 
-/* 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);
-		}
-		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. */
-	add_timer(&hp->happy_timer);
-}
-
 /* hp->happy_lock must be held */
 static int happy_meal_init(struct happy_meal *hp)
 {
-- 
2.37.1

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

[Simon Horman]

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@gmail.com>

Hi Sean,

This patch looks fine to me, as do patches 3 - 4, which is as far as I have
got with my review.

I do, however, have a general question regarding most of the patches in this
series: to what extent have they been tested on HW?

And my follow-up question is: to what extent should we consider removing
support for hardware that isn't being tested and therefore has/will likely
have become broken break at some point? Quattro, the subject of a latter
patch in this series, seems to be a case in point.

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

[Sean Anderson]

On 3/15/23 04:20, 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@gmail.com>
> 
> Hi Sean,
> 
> This patch looks fine to me, as do patches 3 - 4, which is as far as I have
> got with my review.
> 
> I do, however, have a general question regarding most of the patches in this
> series: to what extent have they been tested on HW?

I have tested them with some PCI cards, mostly with the other end autonegotiating
100M. This series doesn't really touch the phy state machines, so I think it is
fine to just make sure the link comes up (and things work after bringing the
interface down and up).

> And my follow-up question is: to what extent should we consider removing
> support for hardware that isn't being tested and therefore has/will likely
> have become broken break at some point? Quattro, the subject of a latter
> patch in this series, seems to be a case in point.

Well, I ordered a quattro card (this hardware is quite cheap on ebay) so
hopefully I can test that. The real question is whether there's anyone using this
on sparc. I tried CCing some sparc users mailing lists in the cover letter, but no
luck so far.

--Sean

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

[Simon Horman]

On Wed, Mar 15, 2023 at 11:35:19AM -0400, Sean Anderson wrote:
> On 3/15/23 04:20, 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@gmail.com>
> > 
> > Hi Sean,
> > 
> > This patch looks fine to me, as do patches 3 - 4, which is as far as I have
> > got with my review.
> > 
> > I do, however, have a general question regarding most of the patches in this
> > series: to what extent have they been tested on HW?
> 
> I have tested them with some PCI cards, mostly with the other end
> autonegotiating 100M. This series doesn't really touch the phy state
> machines, so I think it is fine to just make sure the link comes up (and
> things work after bringing the interface down and up).

Understood, I'll proceed with my review on that basis.

> > And my follow-up question is: to what extent should we consider removing
> > support for hardware that isn't being tested and therefore has/will likely
> > have become broken break at some point? Quattro, the subject of a latter
> > patch in this series, seems to be a case in point.
> 
> Well, I ordered a quattro card (this hardware is quite cheap on ebay) so
> hopefully I can test that.

Yes, for some reason I was searching on eBay too :)

> The real question is whether there's anyone using this on sparc. I tried
> CCing some sparc users mailing lists in the cover letter, but no luck so
> far.

Yes, that is the question.

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

[Simon Horman]

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@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

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

[Simon Horman]

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@gmail.com>

...

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

> +		}
> +		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);

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

...

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@gmail.com>
> 
> ...
> 
>> @@ -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

[PATCH net-next v3 2/9] net: sunhme: Remove residual polling code

[Sean Anderson]

The sunhme driver never used the hardware MII polling feature. Even the
if-def'd out happy_meal_poll_start was removed by 2002 [1]. Remove the
various places in the driver which needlessly guard against MII interrupts
which will never be enabled.

[1] https://lwn.net/2002/0411/a/2.5.8-pre3.php3

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/net/ethernet/sun/sunhme.c | 134 ++++--------------------------
 drivers/net/ethernet/sun/sunhme.h |   6 +-
 2 files changed, 18 insertions(+), 122 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index 65733ee5ddd9..6bcafe820a9e 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -985,32 +985,6 @@ static void happy_meal_get_counters(struct happy_meal *hp, void __iomem *bregs)
 	hme_write32(hp, bregs + BMAC_LTCTR, 0);
 }
 
-/* hp->happy_lock must be held */
-static void happy_meal_poll_stop(struct happy_meal *hp, void __iomem *tregs)
-{
-	/* If polling disabled or not polling already, nothing to do. */
-	if ((hp->happy_flags & (HFLAG_POLLENABLE | HFLAG_POLL)) !=
-	   (HFLAG_POLLENABLE | HFLAG_POLL)) {
-		ASD("not polling, return\n");
-		return;
-	}
-
-	/* Shut up the MIF. */
-	ASD("were polling, mif ints off, polling off\n");
-	hme_write32(hp, tregs + TCVR_IMASK, 0xffff);
-
-	/* Turn off polling. */
-	hme_write32(hp, tregs + TCVR_CFG,
-		    hme_read32(hp, tregs + TCVR_CFG) & ~(TCV_CFG_PENABLE));
-
-	/* We are no longer polling. */
-	hp->happy_flags &= ~(HFLAG_POLL);
-
-	/* Let the bits set. */
-	udelay(200);
-	ASD("done\n");
-}
-
 /* Only Sun can take such nice parts and fuck up the programming interface
  * like this.  Good job guys...
  */
@@ -1115,57 +1089,26 @@ static int happy_meal_tcvr_reset(struct happy_meal *hp, void __iomem *tregs)
 static void happy_meal_transceiver_check(struct happy_meal *hp, void __iomem *tregs)
 {
 	unsigned long tconfig = hme_read32(hp, tregs + TCVR_CFG);
+	u32 reread = hme_read32(hp, tregs + TCVR_CFG);
 
 	ASD("tcfg=%08lx\n", tconfig);
-	if (hp->happy_flags & HFLAG_POLL) {
-		/* If we are polling, we must stop to get the transceiver type. */
-		if (hp->tcvr_type == internal) {
-			if (tconfig & TCV_CFG_MDIO1) {
-				happy_meal_poll_stop(hp, tregs);
-				hp->paddr = TCV_PADDR_ETX;
-				hp->tcvr_type = external;
-				tconfig &= ~(TCV_CFG_PENABLE);
-				tconfig |= TCV_CFG_PSELECT;
-				hme_write32(hp, tregs + TCVR_CFG, tconfig);
-				ASD("poll stop, internal->external\n");
-			}
-		} else {
-			if (hp->tcvr_type == external) {
-				if (!(hme_read32(hp, tregs + TCVR_STATUS) >> 16)) {
-					happy_meal_poll_stop(hp, tregs);
-					hp->paddr = TCV_PADDR_ITX;
-					hp->tcvr_type = internal;
-					hme_write32(hp, tregs + TCVR_CFG,
-						    hme_read32(hp, tregs + TCVR_CFG) &
-						    ~(TCV_CFG_PSELECT));
-					ASD("poll stop, external->internal\n");
-				}
-			} else {
-				ASD("polling, none\n");
-			}
-		}
+	if (reread & TCV_CFG_MDIO1) {
+		hme_write32(hp, tregs + TCVR_CFG, tconfig | TCV_CFG_PSELECT);
+		hp->paddr = TCV_PADDR_ETX;
+		hp->tcvr_type = external;
+		ASD("not polling, external\n");
 	} else {
-		u32 reread = hme_read32(hp, tregs + TCVR_CFG);
-
-		/* Else we can just work off of the MDIO bits. */
-		if (reread & TCV_CFG_MDIO1) {
-			hme_write32(hp, tregs + TCVR_CFG, tconfig | TCV_CFG_PSELECT);
-			hp->paddr = TCV_PADDR_ETX;
-			hp->tcvr_type = external;
-			ASD("not polling, external\n");
+		if (reread & TCV_CFG_MDIO0) {
+			hme_write32(hp, tregs + TCVR_CFG,
+				    tconfig & ~(TCV_CFG_PSELECT));
+			hp->paddr = TCV_PADDR_ITX;
+			hp->tcvr_type = internal;
+			ASD("not polling, internal\n");
 		} else {
-			if (reread & TCV_CFG_MDIO0) {
-				hme_write32(hp, tregs + TCVR_CFG,
-					    tconfig & ~(TCV_CFG_PSELECT));
-				hp->paddr = TCV_PADDR_ITX;
-				hp->tcvr_type = internal;
-				ASD("not polling, internal\n");
-			} else {
-				netdev_err(hp->dev,
-					   "Transceiver and a coke please.");
-				hp->tcvr_type = none; /* Grrr... */
-				ASD("not polling, none\n");
-			}
+			netdev_err(hp->dev,
+				   "Transceiver and a coke please.");
+			hp->tcvr_type = none; /* Grrr... */
+			ASD("not polling, none\n");
 		}
 	}
 }
@@ -1334,10 +1277,6 @@ static int happy_meal_init(struct happy_meal *hp)
 		happy_meal_get_counters(hp, bregs);
 	}
 
-	/* Stop polling. */
-	HMD("to happy_meal_poll_stop\n");
-	happy_meal_poll_stop(hp, tregs);
-
 	/* Stop transmitter and receiver. */
 	HMD("to happy_meal_stop\n");
 	happy_meal_stop(hp, gregs);
@@ -1346,11 +1285,6 @@ static int happy_meal_init(struct happy_meal *hp)
 	HMD("to happy_meal_init_rings\n");
 	happy_meal_init_rings(hp);
 
-	/* Shut up the MIF. */
-	HMD("Disable all MIF irqs (old[%08x])\n",
-	    hme_read32(hp, tregs + TCVR_IMASK));
-	hme_write32(hp, tregs + TCVR_IMASK, 0xffff);
-
 	/* See if we can enable the MIF frame on this card to speak to the DP83840. */
 	if (hp->happy_flags & HFLAG_FENABLE) {
 		HMD("use frame old[%08x]\n",
@@ -1605,7 +1539,6 @@ static void happy_meal_set_initial_advertisement(struct happy_meal *hp)
 	void __iomem *gregs	= hp->gregs;
 
 	happy_meal_stop(hp, gregs);
-	hme_write32(hp, tregs + TCVR_IMASK, 0xffff);
 	if (hp->happy_flags & HFLAG_FENABLE)
 		hme_write32(hp, tregs + TCVR_CFG,
 			    hme_read32(hp, tregs + TCVR_CFG) & ~(TCV_CFG_BENABLE));
@@ -1762,34 +1695,6 @@ static int happy_meal_is_not_so_happy(struct happy_meal *hp, u32 status)
 	return 0;
 }
 
-/* hp->happy_lock must be held */
-static void happy_meal_mif_interrupt(struct happy_meal *hp)
-{
-	void __iomem *tregs = hp->tcvregs;
-
-	netdev_info(hp->dev, "Link status change.\n");
-	hp->sw_bmcr = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
-	hp->sw_lpa = happy_meal_tcvr_read(hp, tregs, MII_LPA);
-
-	/* Use the fastest transmission protocol possible. */
-	if (hp->sw_lpa & LPA_100FULL) {
-		netdev_info(hp->dev, "Switching to 100Mbps at full duplex.\n");
-		hp->sw_bmcr |= (BMCR_FULLDPLX | BMCR_SPEED100);
-	} else if (hp->sw_lpa & LPA_100HALF) {
-		netdev_info(hp->dev, "Switching to 100MBps at half duplex.\n");
-		hp->sw_bmcr |= BMCR_SPEED100;
-	} else if (hp->sw_lpa & LPA_10FULL) {
-		netdev_info(hp->dev, "Switching to 10MBps at full duplex.\n");
-		hp->sw_bmcr |= BMCR_FULLDPLX;
-	} else {
-		netdev_info(hp->dev, "Using 10Mbps at half duplex.\n");
-	}
-	happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
-
-	/* Finally stop polling and shut up the MIF. */
-	happy_meal_poll_stop(hp, tregs);
-}
-
 /* hp->happy_lock must be held */
 static void happy_meal_tx(struct happy_meal *hp)
 {
@@ -1973,9 +1878,6 @@ static irqreturn_t happy_meal_interrupt(int irq, void *dev_id)
 			goto out;
 	}
 
-	if (happy_status & GREG_STAT_MIFIRQ)
-		happy_meal_mif_interrupt(hp);
-
 	if (happy_status & GREG_STAT_TXALL)
 		happy_meal_tx(hp);
 
@@ -2003,7 +1905,6 @@ static irqreturn_t quattro_sbus_interrupt(int irq, void *cookie)
 		HMD("status=%08x\n", happy_status);
 
 		if (!(happy_status & (GREG_STAT_ERRORS |
-				      GREG_STAT_MIFIRQ |
 				      GREG_STAT_TXALL |
 				      GREG_STAT_RXTOHOST)))
 			continue;
@@ -2014,9 +1915,6 @@ static irqreturn_t quattro_sbus_interrupt(int irq, void *cookie)
 			if (happy_meal_is_not_so_happy(hp, happy_status))
 				goto next;
 
-		if (happy_status & GREG_STAT_MIFIRQ)
-			happy_meal_mif_interrupt(hp);
-
 		if (happy_status & GREG_STAT_TXALL)
 			happy_meal_tx(hp);
 
diff --git a/drivers/net/ethernet/sun/sunhme.h b/drivers/net/ethernet/sun/sunhme.h
index 9118c60c9426..258b4c7fe962 100644
--- a/drivers/net/ethernet/sun/sunhme.h
+++ b/drivers/net/ethernet/sun/sunhme.h
@@ -462,22 +462,20 @@ struct happy_meal {
 };
 
 /* Here are the happy flags. */
-#define HFLAG_POLL                0x00000001      /* We are doing MIF polling          */
 #define HFLAG_FENABLE             0x00000002      /* The MII frame is enabled          */
 #define HFLAG_LANCE               0x00000004      /* We are using lance-mode           */
 #define HFLAG_RXENABLE            0x00000008      /* Receiver is enabled               */
 #define HFLAG_AUTO                0x00000010      /* Using auto-negotiation, 0 = force */
 #define HFLAG_FULL                0x00000020      /* Full duplex enable                */
 #define HFLAG_MACFULL             0x00000040      /* Using full duplex in the MAC      */
-#define HFLAG_POLLENABLE          0x00000080      /* Actually try MIF polling          */
 #define HFLAG_RXCV                0x00000100      /* XXX RXCV ENABLE                   */
 #define HFLAG_INIT                0x00000200      /* Init called at least once         */
 #define HFLAG_LINKUP              0x00000400      /* 1 = Link is up                    */
 #define HFLAG_PCI                 0x00000800      /* PCI based Happy Meal              */
 #define HFLAG_QUATTRO		  0x00001000      /* On QFE/Quattro card	       */
 
-#define HFLAG_20_21  (HFLAG_POLLENABLE | HFLAG_FENABLE)
-#define HFLAG_NOT_A0 (HFLAG_POLLENABLE | HFLAG_FENABLE | HFLAG_LANCE | HFLAG_RXCV)
+#define HFLAG_20_21  HFLAG_FENABLE
+#define HFLAG_NOT_A0 (HFLAG_FENABLE | HFLAG_LANCE | HFLAG_RXCV)
 
 /* Support for QFE/Quattro cards. */
 struct quattro {
-- 
2.37.1

Re: [PATCH net-next v3 2/9] net: sunhme: Remove residual polling code

[Simon Horman]

On Mon, Mar 13, 2023 at 08:36:06PM -0400, Sean Anderson wrote:
> The sunhme driver never used the hardware MII polling feature. Even the
> if-def'd out happy_meal_poll_start was removed by 2002 [1]. Remove the
> various places in the driver which needlessly guard against MII interrupts
> which will never be enabled.
> 
> [1] https://lwn.net/2002/0411/a/2.5.8-pre3.php3
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next v3 3/9] net: sunhme: Unify IRQ requesting

[Sean Anderson]

Instead of registering one interrupt handler for all four SBUS Quattro
HMEs, let each HME register its own handler. To make this work, we don't
handle the IRQ if none of the status bits are set. This reduces the
complexity of the driver, and makes it easier to ensure things happen
before/after enabling IRQs.

I'm not really sure why we request IRQs in two different places (and leave
them running after removing the driver!). A lot of things in this driver
seem to just be crusty, and not necessarily intentional. I'm assuming
that's the case here as well.

This really needs to be tested by someone with an SBUS Quattro card.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/net/ethernet/sun/sunhme.c | 131 +++---------------------------
 1 file changed, 10 insertions(+), 121 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index 6bcafe820a9e..5e17b1cdf016 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -1870,6 +1870,8 @@ static irqreturn_t happy_meal_interrupt(int irq, void *dev_id)
 	u32 happy_status       = hme_read32(hp, hp->gregs + GREG_STAT);
 
 	HMD("status=%08x\n", happy_status);
+	if (!happy_status)
+		return IRQ_NONE;
 
 	spin_lock(&hp->happy_lock);
 
@@ -1891,62 +1893,16 @@ static irqreturn_t happy_meal_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-#ifdef CONFIG_SBUS
-static irqreturn_t quattro_sbus_interrupt(int irq, void *cookie)
-{
-	struct quattro *qp = (struct quattro *) cookie;
-	int i;
-
-	for (i = 0; i < 4; i++) {
-		struct net_device *dev = qp->happy_meals[i];
-		struct happy_meal *hp  = netdev_priv(dev);
-		u32 happy_status       = hme_read32(hp, hp->gregs + GREG_STAT);
-
-		HMD("status=%08x\n", happy_status);
-
-		if (!(happy_status & (GREG_STAT_ERRORS |
-				      GREG_STAT_TXALL |
-				      GREG_STAT_RXTOHOST)))
-			continue;
-
-		spin_lock(&hp->happy_lock);
-
-		if (happy_status & GREG_STAT_ERRORS)
-			if (happy_meal_is_not_so_happy(hp, happy_status))
-				goto next;
-
-		if (happy_status & GREG_STAT_TXALL)
-			happy_meal_tx(hp);
-
-		if (happy_status & GREG_STAT_RXTOHOST)
-			happy_meal_rx(hp, dev);
-
-	next:
-		spin_unlock(&hp->happy_lock);
-	}
-	HMD("done\n");
-
-	return IRQ_HANDLED;
-}
-#endif
-
 static int happy_meal_open(struct net_device *dev)
 {
 	struct happy_meal *hp = netdev_priv(dev);
 	int res;
 
-	/* On SBUS Quattro QFE cards, all hme interrupts are concentrated
-	 * into a single source which we register handling at probe time.
-	 */
-	if ((hp->happy_flags & (HFLAG_QUATTRO|HFLAG_PCI)) != HFLAG_QUATTRO) {
-		res = request_irq(hp->irq, happy_meal_interrupt, IRQF_SHARED,
-				  dev->name, dev);
-		if (res) {
-			HMD("EAGAIN\n");
-			netdev_err(dev, "Can't order irq %d to go.\n", hp->irq);
-
-			return -EAGAIN;
-		}
+	res = request_irq(hp->irq, happy_meal_interrupt, IRQF_SHARED,
+			  dev->name, dev);
+	if (res) {
+		netdev_err(dev, "Can't order irq %d to go.\n", hp->irq);
+		return res;
 	}
 
 	HMD("to happy_meal_init\n");
@@ -1955,7 +1911,7 @@ static int happy_meal_open(struct net_device *dev)
 	res = happy_meal_init(hp);
 	spin_unlock_irq(&hp->happy_lock);
 
-	if (res && ((hp->happy_flags & (HFLAG_QUATTRO|HFLAG_PCI)) != HFLAG_QUATTRO))
+	if (res)
 		free_irq(hp->irq, dev);
 	return res;
 }
@@ -1973,12 +1929,7 @@ static int happy_meal_close(struct net_device *dev)
 
 	spin_unlock_irq(&hp->happy_lock);
 
-	/* On Quattro QFE cards, all hme interrupts are concentrated
-	 * into a single source which we register handling at probe
-	 * time and never unregister.
-	 */
-	if ((hp->happy_flags & (HFLAG_QUATTRO|HFLAG_PCI)) != HFLAG_QUATTRO)
-		free_irq(hp->irq, dev);
+	free_irq(hp->irq, dev);
 
 	return 0;
 }
@@ -2311,59 +2262,6 @@ static struct quattro *quattro_sbus_find(struct platform_device *child)
 	platform_set_drvdata(op, qp);
 	return qp;
 }
-
-/* After all quattro cards have been probed, we call these functions
- * to register the IRQ handlers for the cards that have been
- * successfully probed and skip the cards that failed to initialize
- */
-static int __init quattro_sbus_register_irqs(void)
-{
-	struct quattro *qp;
-
-	for (qp = qfe_sbus_list; qp != NULL; qp = qp->next) {
-		struct platform_device *op = qp->quattro_dev;
-		int err, qfe_slot, skip = 0;
-
-		for (qfe_slot = 0; qfe_slot < 4; qfe_slot++) {
-			if (!qp->happy_meals[qfe_slot])
-				skip = 1;
-		}
-		if (skip)
-			continue;
-
-		err = request_irq(op->archdata.irqs[0],
-				  quattro_sbus_interrupt,
-				  IRQF_SHARED, "Quattro",
-				  qp);
-		if (err != 0) {
-			dev_err(&op->dev,
-				"Quattro HME: IRQ registration error %d.\n",
-				err);
-			return err;
-		}
-	}
-
-	return 0;
-}
-
-static void quattro_sbus_free_irqs(void)
-{
-	struct quattro *qp;
-
-	for (qp = qfe_sbus_list; qp != NULL; qp = qp->next) {
-		struct platform_device *op = qp->quattro_dev;
-		int qfe_slot, skip = 0;
-
-		for (qfe_slot = 0; qfe_slot < 4; qfe_slot++) {
-			if (!qp->happy_meals[qfe_slot])
-				skip = 1;
-		}
-		if (skip)
-			continue;
-
-		free_irq(op->archdata.irqs[0], qp);
-	}
-}
 #endif /* CONFIG_SBUS */
 
 #ifdef CONFIG_PCI
@@ -3005,8 +2903,6 @@ static int hme_sbus_remove(struct platform_device *op)
 
 	unregister_netdev(net_dev);
 
-	/* XXX qfe parent interrupt... */
-
 	of_iounmap(&op->resource[0], hp->gregs, GREG_REG_SIZE);
 	of_iounmap(&op->resource[1], hp->etxregs, ETX_REG_SIZE);
 	of_iounmap(&op->resource[2], hp->erxregs, ERX_REG_SIZE);
@@ -3050,19 +2946,12 @@ static struct platform_driver hme_sbus_driver = {
 
 static int __init happy_meal_sbus_init(void)
 {
-	int err;
-
-	err = platform_driver_register(&hme_sbus_driver);
-	if (!err)
-		err = quattro_sbus_register_irqs();
-
-	return err;
+	return platform_driver_register(&hme_sbus_driver);
 }
 
 static void happy_meal_sbus_exit(void)
 {
 	platform_driver_unregister(&hme_sbus_driver);
-	quattro_sbus_free_irqs();
 
 	while (qfe_sbus_list) {
 		struct quattro *qfe = qfe_sbus_list;
-- 
2.37.1

Re: [PATCH net-next v3 3/9] net: sunhme: Unify IRQ requesting

[Simon Horman]

On Mon, Mar 13, 2023 at 08:36:07PM -0400, Sean Anderson wrote:
> Instead of registering one interrupt handler for all four SBUS Quattro
> HMEs, let each HME register its own handler. To make this work, we don't
> handle the IRQ if none of the status bits are set. This reduces the
> complexity of the driver, and makes it easier to ensure things happen
> before/after enabling IRQs.
> 
> I'm not really sure why we request IRQs in two different places (and leave
> them running after removing the driver!). A lot of things in this driver
> seem to just be crusty, and not necessarily intentional. I'm assuming
> that's the case here as well.
> 
> This really needs to be tested by someone with an SBUS Quattro card.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next v3 4/9] net: sunhme: Alphabetize includes

[Sean Anderson]

Alphabetize includes to make it clearer where to add new ones.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- Make some more includes common

 drivers/net/ethernet/sun/sunhme.c | 59 ++++++++++++++-----------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index 5e17b1cdf016..acdcae2cb49a 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -14,48 +14,43 @@
  *     argument : macaddr=0x00,0x10,0x20,0x30,0x40,0x50
  */
 
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/types.h>
+#include <asm/byteorder.h>
+#include <asm/dma.h>
+#include <asm/irq.h>
+#include <linux/bitops.h>
+#include <linux/crc32.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/errno.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
 #include <linux/fcntl.h>
-#include <linux/interrupt.h>
-#include <linux/ioport.h>
 #include <linux/in.h>
-#include <linux/slab.h>
-#include <linux/string.h>
-#include <linux/delay.h>
 #include <linux/init.h>
-#include <linux/ethtool.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
 #include <linux/mii.h>
-#include <linux/crc32.h>
-#include <linux/random.h>
-#include <linux/errno.h>
-#include <linux/netdevice.h>
-#include <linux/etherdevice.h>
-#include <linux/skbuff.h>
 #include <linux/mm.h>
-#include <linux/bitops.h>
-#include <linux/dma-mapping.h>
-
-#include <asm/io.h>
-#include <asm/dma.h>
-#include <asm/byteorder.h>
-
-#ifdef CONFIG_SPARC
-#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
 #include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/pci.h>
+#include <linux/random.h>
+#include <linux/skbuff.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+#ifdef CONFIG_SPARC
+#include <asm/auxio.h>
 #include <asm/idprom.h>
 #include <asm/openprom.h>
 #include <asm/oplib.h>
 #include <asm/prom.h>
-#include <asm/auxio.h>
-#endif
-#include <linux/uaccess.h>
-
-#include <asm/irq.h>
-
-#ifdef CONFIG_PCI
-#include <linux/pci.h>
 #endif
 
 #include "sunhme.h"
-- 
2.37.1

Re: [PATCH net-next v3 4/9] net: sunhme: Alphabetize includes

[Simon Horman]

On Mon, Mar 13, 2023 at 08:36:08PM -0400, Sean Anderson wrote:
> Alphabetize includes to make it clearer where to add new ones.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next v3 5/9] net: sunhme: Switch SBUS to devres

[Sean Anderson]

The PCI half of this driver was converted in commit 914d9b2711dd ("sunhme:
switch to devres"). Do the same for the SBUS half.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/net/ethernet/sun/sunhme.c | 118 +++++++++---------------------
 1 file changed, 35 insertions(+), 83 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index acdcae2cb49a..3072578c334a 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2313,29 +2313,28 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 	struct net_device *dev;
 	int i, qfe_slot = -1;
 	u8 addr[ETH_ALEN];
-	int err = -ENODEV;
+	int err;
 
 	sbus_dp = op->dev.parent->of_node;
 
 	/* We can match PCI devices too, do not accept those here. */
 	if (!of_node_name_eq(sbus_dp, "sbus") && !of_node_name_eq(sbus_dp, "sbi"))
-		return err;
+		return -ENODEV;
 
 	if (is_qfe) {
 		qp = quattro_sbus_find(op);
 		if (qp == NULL)
-			goto err_out;
+			return -ENODEV;
 		for (qfe_slot = 0; qfe_slot < 4; qfe_slot++)
 			if (qp->happy_meals[qfe_slot] == NULL)
 				break;
 		if (qfe_slot == 4)
-			goto err_out;
+			return -ENODEV;
 	}
 
-	err = -ENOMEM;
-	dev = alloc_etherdev(sizeof(struct happy_meal));
+	dev = devm_alloc_etherdev(&op->dev, sizeof(struct happy_meal));
 	if (!dev)
-		goto err_out;
+		return -ENOMEM;
 	SET_NETDEV_DEV(dev, &op->dev);
 
 	/* If user did not specify a MAC address specifically, use
@@ -2369,46 +2368,45 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 
 	spin_lock_init(&hp->happy_lock);
 
-	err = -ENODEV;
 	if (qp != NULL) {
 		hp->qfe_parent = qp;
 		hp->qfe_ent = qfe_slot;
 		qp->happy_meals[qfe_slot] = dev;
 	}
 
-	hp->gregs = of_ioremap(&op->resource[0], 0,
-			       GREG_REG_SIZE, "HME Global Regs");
-	if (!hp->gregs) {
+	hp->gregs = devm_platform_ioremap_resource(op, 0);
+	if (IS_ERR(hp->gregs)) {
 		dev_err(&op->dev, "Cannot map global registers.\n");
-		goto err_out_free_netdev;
+		err = PTR_ERR(hp->gregs);
+		goto err_out_clear_quattro;
 	}
 
-	hp->etxregs = of_ioremap(&op->resource[1], 0,
-				 ETX_REG_SIZE, "HME TX Regs");
-	if (!hp->etxregs) {
+	hp->etxregs = devm_platform_ioremap_resource(op, 1);
+	if (IS_ERR(hp->etxregs)) {
 		dev_err(&op->dev, "Cannot map MAC TX registers.\n");
-		goto err_out_iounmap;
+		err = PTR_ERR(hp->etxregs);
+		goto err_out_clear_quattro;
 	}
 
-	hp->erxregs = of_ioremap(&op->resource[2], 0,
-				 ERX_REG_SIZE, "HME RX Regs");
-	if (!hp->erxregs) {
+	hp->erxregs = devm_platform_ioremap_resource(op, 2);
+	if (IS_ERR(hp->erxregs)) {
 		dev_err(&op->dev, "Cannot map MAC RX registers.\n");
-		goto err_out_iounmap;
+		err = PTR_ERR(hp->erxregs);
+		goto err_out_clear_quattro;
 	}
 
-	hp->bigmacregs = of_ioremap(&op->resource[3], 0,
-				    BMAC_REG_SIZE, "HME BIGMAC Regs");
-	if (!hp->bigmacregs) {
+	hp->bigmacregs = devm_platform_ioremap_resource(op, 3);
+	if (IS_ERR(hp->bigmacregs)) {
 		dev_err(&op->dev, "Cannot map BIGMAC registers.\n");
-		goto err_out_iounmap;
+		err = PTR_ERR(hp->bigmacregs);
+		goto err_out_clear_quattro;
 	}
 
-	hp->tcvregs = of_ioremap(&op->resource[4], 0,
-				 TCVR_REG_SIZE, "HME Tranceiver Regs");
-	if (!hp->tcvregs) {
+	hp->tcvregs = devm_platform_ioremap_resource(op, 4);
+	if (IS_ERR(hp->tcvregs)) {
 		dev_err(&op->dev, "Cannot map TCVR registers.\n");
-		goto err_out_iounmap;
+		err = PTR_ERR(hp->tcvregs);
+		goto err_out_clear_quattro;
 	}
 
 	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
@@ -2428,13 +2426,12 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 	hp->happy_bursts = of_getintprop_default(sbus_dp,
 						 "burst-sizes", 0x00);
 
-	hp->happy_block = dma_alloc_coherent(hp->dma_dev,
-					     PAGE_SIZE,
-					     &hp->hblock_dvma,
-					     GFP_ATOMIC);
-	err = -ENOMEM;
-	if (!hp->happy_block)
-		goto err_out_iounmap;
+	hp->happy_block = dmam_alloc_coherent(&op->dev, PAGE_SIZE,
+					      &hp->hblock_dvma, GFP_KERNEL);
+	if (!hp->happy_block) {
+		err = -ENOMEM;
+		goto err_out_clear_quattro;
+	}
 
 	/* Force check of the link first time we are brought up. */
 	hp->linkcheck = 0;
@@ -2472,10 +2469,10 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 	happy_meal_set_initial_advertisement(hp);
 	spin_unlock_irq(&hp->happy_lock);
 
-	err = register_netdev(hp->dev);
+	err = devm_register_netdev(&op->dev, dev);
 	if (err) {
 		dev_err(&op->dev, "Cannot register net device, aborting.\n");
-		goto err_out_free_coherent;
+		goto err_out_clear_quattro;
 	}
 
 	platform_set_drvdata(op, hp);
@@ -2490,31 +2487,9 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 
 	return 0;
 
-err_out_free_coherent:
-	dma_free_coherent(hp->dma_dev,
-			  PAGE_SIZE,
-			  hp->happy_block,
-			  hp->hblock_dvma);
-
-err_out_iounmap:
-	if (hp->gregs)
-		of_iounmap(&op->resource[0], hp->gregs, GREG_REG_SIZE);
-	if (hp->etxregs)
-		of_iounmap(&op->resource[1], hp->etxregs, ETX_REG_SIZE);
-	if (hp->erxregs)
-		of_iounmap(&op->resource[2], hp->erxregs, ERX_REG_SIZE);
-	if (hp->bigmacregs)
-		of_iounmap(&op->resource[3], hp->bigmacregs, BMAC_REG_SIZE);
-	if (hp->tcvregs)
-		of_iounmap(&op->resource[4], hp->tcvregs, TCVR_REG_SIZE);
-
+err_out_clear_quattro:
 	if (qp)
 		qp->happy_meals[qfe_slot] = NULL;
-
-err_out_free_netdev:
-	free_netdev(dev);
-
-err_out:
 	return err;
 }
 #endif
@@ -2891,28 +2866,6 @@ static int hme_sbus_probe(struct platform_device *op)
 	return happy_meal_sbus_probe_one(op, is_qfe);
 }
 
-static int hme_sbus_remove(struct platform_device *op)
-{
-	struct happy_meal *hp = platform_get_drvdata(op);
-	struct net_device *net_dev = hp->dev;
-
-	unregister_netdev(net_dev);
-
-	of_iounmap(&op->resource[0], hp->gregs, GREG_REG_SIZE);
-	of_iounmap(&op->resource[1], hp->etxregs, ETX_REG_SIZE);
-	of_iounmap(&op->resource[2], hp->erxregs, ERX_REG_SIZE);
-	of_iounmap(&op->resource[3], hp->bigmacregs, BMAC_REG_SIZE);
-	of_iounmap(&op->resource[4], hp->tcvregs, TCVR_REG_SIZE);
-	dma_free_coherent(hp->dma_dev,
-			  PAGE_SIZE,
-			  hp->happy_block,
-			  hp->hblock_dvma);
-
-	free_netdev(net_dev);
-
-	return 0;
-}
-
 static const struct of_device_id hme_sbus_match[] = {
 	{
 		.name = "SUNW,hme",
@@ -2936,7 +2889,6 @@ static struct platform_driver hme_sbus_driver = {
 		.of_match_table = hme_sbus_match,
 	},
 	.probe		= hme_sbus_probe,
-	.remove		= hme_sbus_remove,
 };
 
 static int __init happy_meal_sbus_init(void)
-- 
2.37.1

Re: [PATCH net-next v3 5/9] net: sunhme: Switch SBUS to devres

[Simon Horman]

On Mon, Mar 13, 2023 at 08:36:09PM -0400, Sean Anderson wrote:
> The PCI half of this driver was converted in commit 914d9b2711dd ("sunhme:
> switch to devres"). Do the same for the SBUS half.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next v3 6/9] net: sunhme: Consolidate mac address initialization

[Sean Anderson]

The mac address initialization is braodly the same between PCI and SBUS,
and one was clearly copied from the other. Consolidate them. We still have
to have some ifdefs because pci_(un)map_rom is only implemented for PCI,
and idprom is only implemented for SPARC.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/net/ethernet/sun/sunhme.c | 282 ++++++++++++++----------------
 1 file changed, 134 insertions(+), 148 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index 3072578c334a..c2737f26afbe 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2304,6 +2304,133 @@ static const struct net_device_ops hme_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 };
 
+#ifdef CONFIG_PCI
+static int is_quattro_p(struct pci_dev *pdev)
+{
+	struct pci_dev *busdev = pdev->bus->self;
+	struct pci_dev *this_pdev;
+	int n_hmes;
+
+	if (!busdev || busdev->vendor != PCI_VENDOR_ID_DEC ||
+	    busdev->device != PCI_DEVICE_ID_DEC_21153)
+		return 0;
+
+	n_hmes = 0;
+	list_for_each_entry(this_pdev, &pdev->bus->devices, bus_list) {
+		if (this_pdev->vendor == PCI_VENDOR_ID_SUN &&
+		    this_pdev->device == PCI_DEVICE_ID_SUN_HAPPYMEAL)
+			n_hmes++;
+	}
+
+	if (n_hmes != 4)
+		return 0;
+
+	return 1;
+}
+
+/* Fetch MAC address from vital product data of PCI ROM. */
+static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsigned char *dev_addr)
+{
+	int this_offset;
+
+	for (this_offset = 0x20; this_offset < len; this_offset++) {
+		void __iomem *p = rom_base + this_offset;
+
+		if (readb(p + 0) != 0x90 ||
+		    readb(p + 1) != 0x00 ||
+		    readb(p + 2) != 0x09 ||
+		    readb(p + 3) != 0x4e ||
+		    readb(p + 4) != 0x41 ||
+		    readb(p + 5) != 0x06)
+			continue;
+
+		this_offset += 6;
+		p += 6;
+
+		if (index == 0) {
+			int i;
+
+			for (i = 0; i < 6; i++)
+				dev_addr[i] = readb(p + i);
+			return 1;
+		}
+		index--;
+	}
+	return 0;
+}
+
+static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
+						unsigned char *dev_addr)
+{
+	size_t size;
+	void __iomem *p = pci_map_rom(pdev, &size);
+
+	if (p) {
+		int index = 0;
+		int found;
+
+		if (is_quattro_p(pdev))
+			index = PCI_SLOT(pdev->devfn);
+
+		found = readb(p) == 0x55 &&
+			readb(p + 1) == 0xaa &&
+			find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr);
+		pci_unmap_rom(pdev, p);
+		if (found)
+			return;
+	}
+
+	/* Sun MAC prefix then 3 random bytes. */
+	dev_addr[0] = 0x08;
+	dev_addr[1] = 0x00;
+	dev_addr[2] = 0x20;
+	get_random_bytes(&dev_addr[3], 3);
+}
+#endif /* !(CONFIG_SPARC) */
+
+static void happy_meal_addr_init(struct happy_meal *hp,
+				 struct device_node *dp, int qfe_slot)
+{
+	int i;
+
+	for (i = 0; i < 6; i++) {
+		if (macaddr[i] != 0)
+			break;
+	}
+
+	if (i < 6) { /* a mac address was given */
+		u8 addr[ETH_ALEN];
+
+		for (i = 0; i < 6; i++)
+			addr[i] = macaddr[i];
+		eth_hw_addr_set(hp->dev, addr);
+		macaddr[5]++;
+	} else {
+#ifdef CONFIG_SPARC
+		const unsigned char *addr;
+		int len;
+
+		/* If user did not specify a MAC address specifically, use
+		 * the Quattro local-mac-address property...
+		 */
+		if (qfe_slot != -1) {
+			addr = of_get_property(dp, "local-mac-address", &len);
+			if (addr && len == 6) {
+				eth_hw_addr_set(hp->dev, addr);
+				return;
+			}
+		}
+
+		eth_hw_addr_set(hp->dev, idprom->id_ethaddr);
+#else
+		u8 addr[ETH_ALEN];
+
+		get_hme_mac_nonsparc(hp->happy_dev, addr);
+		eth_hw_addr_set(hp->dev, addr);
+#endif
+	}
+}
+
 #ifdef CONFIG_SBUS
 static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 {
@@ -2311,8 +2438,7 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 	struct quattro *qp = NULL;
 	struct happy_meal *hp;
 	struct net_device *dev;
-	int i, qfe_slot = -1;
-	u8 addr[ETH_ALEN];
+	int qfe_slot = -1;
 	int err;
 
 	sbus_dp = op->dev.parent->of_node;
@@ -2337,34 +2463,11 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 		return -ENOMEM;
 	SET_NETDEV_DEV(dev, &op->dev);
 
-	/* If user did not specify a MAC address specifically, use
-	 * the Quattro local-mac-address property...
-	 */
-	for (i = 0; i < 6; i++) {
-		if (macaddr[i] != 0)
-			break;
-	}
-	if (i < 6) { /* a mac address was given */
-		for (i = 0; i < 6; i++)
-			addr[i] = macaddr[i];
-		eth_hw_addr_set(dev, addr);
-		macaddr[5]++;
-	} else {
-		const unsigned char *addr;
-		int len;
-
-		addr = of_get_property(dp, "local-mac-address", &len);
-
-		if (qfe_slot != -1 && addr && len == ETH_ALEN)
-			eth_hw_addr_set(dev, addr);
-		else
-			eth_hw_addr_set(dev, idprom->id_ethaddr);
-	}
-
 	hp = netdev_priv(dev);
-
+	hp->dev = dev;
 	hp->happy_dev = op;
 	hp->dma_dev = &op->dev;
+	happy_meal_addr_init(hp, dp, qfe_slot);
 
 	spin_lock_init(&hp->happy_lock);
 
@@ -2442,7 +2545,6 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 
 	timer_setup(&hp->happy_timer, happy_meal_timer, 0);
 
-	hp->dev = dev;
 	dev->netdev_ops = &hme_netdev_ops;
 	dev->watchdog_timeo = 5*HZ;
 	dev->ethtool_ops = &hme_ethtool_ops;
@@ -2495,104 +2597,17 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 #endif
 
 #ifdef CONFIG_PCI
-#ifndef CONFIG_SPARC
-static int is_quattro_p(struct pci_dev *pdev)
-{
-	struct pci_dev *busdev = pdev->bus->self;
-	struct pci_dev *this_pdev;
-	int n_hmes;
-
-	if (busdev == NULL ||
-	    busdev->vendor != PCI_VENDOR_ID_DEC ||
-	    busdev->device != PCI_DEVICE_ID_DEC_21153)
-		return 0;
-
-	n_hmes = 0;
-	list_for_each_entry(this_pdev, &pdev->bus->devices, bus_list) {
-		if (this_pdev->vendor == PCI_VENDOR_ID_SUN &&
-		    this_pdev->device == PCI_DEVICE_ID_SUN_HAPPYMEAL)
-			n_hmes++;
-	}
-
-	if (n_hmes != 4)
-		return 0;
-
-	return 1;
-}
-
-/* Fetch MAC address from vital product data of PCI ROM. */
-static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsigned char *dev_addr)
-{
-	int this_offset;
-
-	for (this_offset = 0x20; this_offset < len; this_offset++) {
-		void __iomem *p = rom_base + this_offset;
-
-		if (readb(p + 0) != 0x90 ||
-		    readb(p + 1) != 0x00 ||
-		    readb(p + 2) != 0x09 ||
-		    readb(p + 3) != 0x4e ||
-		    readb(p + 4) != 0x41 ||
-		    readb(p + 5) != 0x06)
-			continue;
-
-		this_offset += 6;
-		p += 6;
-
-		if (index == 0) {
-			int i;
-
-			for (i = 0; i < 6; i++)
-				dev_addr[i] = readb(p + i);
-			return 1;
-		}
-		index--;
-	}
-	return 0;
-}
-
-static void get_hme_mac_nonsparc(struct pci_dev *pdev, unsigned char *dev_addr)
-{
-	size_t size;
-	void __iomem *p = pci_map_rom(pdev, &size);
-
-	if (p) {
-		int index = 0;
-		int found;
-
-		if (is_quattro_p(pdev))
-			index = PCI_SLOT(pdev->devfn);
-
-		found = readb(p) == 0x55 &&
-			readb(p + 1) == 0xaa &&
-			find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr);
-		pci_unmap_rom(pdev, p);
-		if (found)
-			return;
-	}
-
-	/* Sun MAC prefix then 3 random bytes. */
-	dev_addr[0] = 0x08;
-	dev_addr[1] = 0x00;
-	dev_addr[2] = 0x20;
-	get_random_bytes(&dev_addr[3], 3);
-}
-#endif /* !(CONFIG_SPARC) */
-
 static int happy_meal_pci_probe(struct pci_dev *pdev,
 				const struct pci_device_id *ent)
 {
 	struct quattro *qp = NULL;
-#ifdef CONFIG_SPARC
-	struct device_node *dp;
-#endif
+	struct device_node *dp = NULL;
 	struct happy_meal *hp;
 	struct net_device *dev;
 	void __iomem *hpreg_base;
 	struct resource *hpreg_res;
-	int i, qfe_slot = -1;
+	int qfe_slot = -1;
 	char prom_name[64];
-	u8 addr[ETH_ALEN];
 	int err;
 
 	/* Now make sure pci_dev cookie is there. */
@@ -2634,7 +2649,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	SET_NETDEV_DEV(dev, &pdev->dev);
 
 	hp = netdev_priv(dev);
-
+	hp->dev = dev;
 	hp->happy_dev = pdev;
 	hp->dma_dev = &pdev->dev;
 
@@ -2670,35 +2685,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 		goto err_out_clear_quattro;
 	}
 
-	for (i = 0; i < 6; i++) {
-		if (macaddr[i] != 0)
-			break;
-	}
-	if (i < 6) { /* a mac address was given */
-		for (i = 0; i < 6; i++)
-			addr[i] = macaddr[i];
-		eth_hw_addr_set(dev, addr);
-		macaddr[5]++;
-	} else {
-#ifdef CONFIG_SPARC
-		const unsigned char *addr;
-		int len;
-
-		if (qfe_slot != -1 &&
-		    (addr = of_get_property(dp, "local-mac-address", &len))
-			!= NULL &&
-		    len == 6) {
-			eth_hw_addr_set(dev, addr);
-		} else {
-			eth_hw_addr_set(dev, idprom->id_ethaddr);
-		}
-#else
-		u8 addr[ETH_ALEN];
-
-		get_hme_mac_nonsparc(pdev, addr);
-		eth_hw_addr_set(dev, addr);
-#endif
-	}
+	happy_meal_addr_init(hp, dp, qfe_slot);
 
 	/* Layout registers. */
 	hp->gregs      = (hpreg_base + 0x0000UL);
@@ -2747,7 +2734,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	timer_setup(&hp->happy_timer, happy_meal_timer, 0);
 
 	hp->irq = pdev->irq;
-	hp->dev = dev;
 	dev->netdev_ops = &hme_netdev_ops;
 	dev->watchdog_timeo = 5*HZ;
 	dev->ethtool_ops = &hme_ethtool_ops;
-- 
2.37.1

Re: [PATCH net-next v3 6/9] net: sunhme: Consolidate mac address initialization

[Simon Horman]

On Mon, Mar 13, 2023 at 08:36:10PM -0400, Sean Anderson wrote:
> The mac address initialization is braodly the same between PCI and SBUS,
> and one was clearly copied from the other. Consolidate them. We still have
> to have some ifdefs because pci_(un)map_rom is only implemented for PCI,
> and idprom is only implemented for SPARC.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

Hi Sean,

Nits aside, this looks good to me.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index 3072578c334a..c2737f26afbe 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c

...

> +static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
> +						unsigned char *dev_addr)
> +{
> +	size_t size;
> +	void __iomem *p = pci_map_rom(pdev, &size);

nit: reverse xmas tree - longest line to shortest - would be nice here.

	void __iomem *p;
	size_t size;

	p = pci_map_rom(pdev, &size);

> +
> +	if (p) {
> +		int index = 0;
> +		int found;
> +
> +		if (is_quattro_p(pdev))
> +			index = PCI_SLOT(pdev->devfn);
> +
> +		found = readb(p) == 0x55 &&
> +			readb(p + 1) == 0xaa &&
> +			find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr);
> +		pci_unmap_rom(pdev, p);
> +		if (found)
> +			return;
> +	}
> +
> +	/* Sun MAC prefix then 3 random bytes. */
> +	dev_addr[0] = 0x08;
> +	dev_addr[1] = 0x00;
> +	dev_addr[2] = 0x20;
> +	get_random_bytes(&dev_addr[3], 3);

nit: Maybe as a follow-up using eth_hw_addr_random() could be considered here.

> +}
> +#endif /* !(CONFIG_SPARC) */

...

>  static int happy_meal_pci_probe(struct pci_dev *pdev,
>  				const struct pci_device_id *ent)
>  {
>  	struct quattro *qp = NULL;
> -#ifdef CONFIG_SPARC
> -	struct device_node *dp;
> -#endif
> +	struct device_node *dp = NULL;

nit: if dp was added above qp then then
     things would move closer to reverse xmas tree.

>  	struct happy_meal *hp;
>  	struct net_device *dev;
>  	void __iomem *hpreg_base;
>  	struct resource *hpreg_res;
> -	int i, qfe_slot = -1;
> +	int qfe_slot = -1;

nit: if qfe_slot was added below prom_name[64] then then
     things would move closer to reverse xmas tree.

>  	char prom_name[64];
> -	u8 addr[ETH_ALEN];
>  	int err;
>  
>  	/* Now make sure pci_dev cookie is there. */

...

Re: [PATCH net-next v3 6/9] net: sunhme: Consolidate mac address initialization

[Simon Horman]

On Sat, Mar 18, 2023 at 09:58:35AM +0100, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:10PM -0400, Sean Anderson wrote:
> > The mac address initialization is braodly the same between PCI and SBUS,
> > and one was clearly copied from the other. Consolidate them. We still have
> > to have some ifdefs because pci_(un)map_rom is only implemented for PCI,
> > and idprom is only implemented for SPARC.
> > 
> > Signed-off-by: Sean Anderson <seanga2@gmail.com>
> 
> Hi Sean,
> 
> Nits aside, this looks good to me.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 
> > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> > index 3072578c334a..c2737f26afbe 100644
> > --- a/drivers/net/ethernet/sun/sunhme.c
> > +++ b/drivers/net/ethernet/sun/sunhme.c
> 
> ...
> 
> > +static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
> > +						unsigned char *dev_addr)
> > +{
> > +	size_t size;
> > +	void __iomem *p = pci_map_rom(pdev, &size);
> 
> nit: reverse xmas tree - longest line to shortest - would be nice here.
> 
> 	void __iomem *p;
> 	size_t size;
> 
> 	p = pci_map_rom(pdev, &size);

Oops, I now see that you got this in patch 7/9.
Thanks!

> > +
> > +	if (p) {
> > +		int index = 0;
> > +		int found;
> > +
> > +		if (is_quattro_p(pdev))
> > +			index = PCI_SLOT(pdev->devfn);
> > +
> > +		found = readb(p) == 0x55 &&
> > +			readb(p + 1) == 0xaa &&
> > +			find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr);
> > +		pci_unmap_rom(pdev, p);
> > +		if (found)
> > +			return;
> > +	}
> > +
> > +	/* Sun MAC prefix then 3 random bytes. */
> > +	dev_addr[0] = 0x08;
> > +	dev_addr[1] = 0x00;
> > +	dev_addr[2] = 0x20;
> > +	get_random_bytes(&dev_addr[3], 3);
> 
> nit: Maybe as a follow-up using eth_hw_addr_random() could be considered here.
> 
> > +}
> > +#endif /* !(CONFIG_SPARC) */
> 
> ...
> 
> >  static int happy_meal_pci_probe(struct pci_dev *pdev,
> >  				const struct pci_device_id *ent)
> >  {
> >  	struct quattro *qp = NULL;
> > -#ifdef CONFIG_SPARC
> > -	struct device_node *dp;
> > -#endif
> > +	struct device_node *dp = NULL;
> 
> nit: if dp was added above qp then then
>      things would move closer to reverse xmas tree.
> 
> >  	struct happy_meal *hp;
> >  	struct net_device *dev;
> >  	void __iomem *hpreg_base;
> >  	struct resource *hpreg_res;
> > -	int i, qfe_slot = -1;
> > +	int qfe_slot = -1;
> 
> nit: if qfe_slot was added below prom_name[64] then then
>      things would move closer to reverse xmas tree.
> 
> >  	char prom_name[64];
> > -	u8 addr[ETH_ALEN];
> >  	int err;
> >  
> >  	/* Now make sure pci_dev cookie is there. */
> 
> ...

Re: [PATCH net-next v3 6/9] net: sunhme: Consolidate mac address initialization

[Sean Anderson]

On 3/18/23 04:58, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:10PM -0400, Sean Anderson wrote:
>> The mac address initialization is braodly the same between PCI and SBUS,
>> and one was clearly copied from the other. Consolidate them. We still have
>> to have some ifdefs because pci_(un)map_rom is only implemented for PCI,
>> and idprom is only implemented for SPARC.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> 
> Hi Sean,
> 
> Nits aside, this looks good to me.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 
>> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
>> index 3072578c334a..c2737f26afbe 100644
>> --- a/drivers/net/ethernet/sun/sunhme.c
>> +++ b/drivers/net/ethernet/sun/sunhme.c
> 
> ...
> 
>> +static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
>> +						unsigned char *dev_addr)
>> +{
>> +	size_t size;
>> +	void __iomem *p = pci_map_rom(pdev, &size);
> 
> nit: reverse xmas tree - longest line to shortest - would be nice here.
> 
> 	void __iomem *p;
> 	size_t size;
> 
> 	p = pci_map_rom(pdev, &size);
> 
>> +
>> +	if (p) {
>> +		int index = 0;
>> +		int found;
>> +
>> +		if (is_quattro_p(pdev))
>> +			index = PCI_SLOT(pdev->devfn);
>> +
>> +		found = readb(p) == 0x55 &&
>> +			readb(p + 1) == 0xaa &&
>> +			find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr);
>> +		pci_unmap_rom(pdev, p);
>> +		if (found)
>> +			return;
>> +	}
>> +
>> +	/* Sun MAC prefix then 3 random bytes. */
>> +	dev_addr[0] = 0x08;
>> +	dev_addr[1] = 0x00;
>> +	dev_addr[2] = 0x20;
>> +	get_random_bytes(&dev_addr[3], 3);
> 
> nit: Maybe as a follow-up using eth_hw_addr_random() could be considered here.

Yes, I would like to come back to this later.

>> +}
>> +#endif /* !(CONFIG_SPARC) */
> 
> ...
> 
>>   static int happy_meal_pci_probe(struct pci_dev *pdev,
>>   				const struct pci_device_id *ent)
>>   {
>>   	struct quattro *qp = NULL;
>> -#ifdef CONFIG_SPARC
>> -	struct device_node *dp;
>> -#endif
>> +	struct device_node *dp = NULL;
> 
> nit: if dp was added above qp then then
>       things would move closer to reverse xmas tree.
> 
>>   	struct happy_meal *hp;
>>   	struct net_device *dev;
>>   	void __iomem *hpreg_base;
>>   	struct resource *hpreg_res;
>> -	int i, qfe_slot = -1;
>> +	int qfe_slot = -1;
> 
> nit: if qfe_slot was added below prom_name[64] then then
>       things would move closer to reverse xmas tree.

This is why I dislike this style...

>>   	char prom_name[64];
>> -	u8 addr[ETH_ALEN];
>>   	int err;
>>   
>>   	/* Now make sure pci_dev cookie is there. */
> 
> ...

[PATCH net-next v3 7/9] net: sunhme: Clean up mac address init

[Sean Anderson]

Clean up some oddities suggested during review.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- New

 drivers/net/ethernet/sun/sunhme.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index c2737f26afbe..1f27e99abf17 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2348,9 +2348,7 @@ static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsi
 		p += 6;
 
 		if (index == 0) {
-			int i;
-
-			for (i = 0; i < 6; i++)
+			for (int i = 0; i < 6; i++)
 				dev_addr[i] = readb(p + i);
 			return 1;
 		}
@@ -2362,9 +2360,10 @@ static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsi
 static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
 						unsigned char *dev_addr)
 {
+	void __iomem *p;
 	size_t size;
-	void __iomem *p = pci_map_rom(pdev, &size);
 
+	p = pci_map_rom(pdev, &size);
 	if (p) {
 		int index = 0;
 		int found;
@@ -2386,7 +2385,7 @@ static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
 	dev_addr[2] = 0x20;
 	get_random_bytes(&dev_addr[3], 3);
 }
-#endif /* !(CONFIG_SPARC) */
+#endif
 
 static void happy_meal_addr_init(struct happy_meal *hp,
 				 struct device_node *dp, int qfe_slot)
-- 
2.37.1

Re: [PATCH net-next v3 7/9] net: sunhme: Clean up mac address init

[Simon Horman]

On Mon, Mar 13, 2023 at 08:36:11PM -0400, Sean Anderson wrote:
> Clean up some oddities suggested during review.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - New
> 
>  drivers/net/ethernet/sun/sunhme.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index c2737f26afbe..1f27e99abf17 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c
> @@ -2348,9 +2348,7 @@ static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsi
>  		p += 6;
>  
>  		if (index == 0) {
> -			int i;
> -
> -			for (i = 0; i < 6; i++)
> +			for (int i = 0; i < 6; i++)
>  				dev_addr[i] = readb(p + i);
>  			return 1;
>  		}
> @@ -2362,9 +2360,10 @@ static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsi
>  static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
>  						unsigned char *dev_addr)
>  {
> +	void __iomem *p;
>  	size_t size;
> -	void __iomem *p = pci_map_rom(pdev, &size);
>  
> +	p = pci_map_rom(pdev, &size);
>  	if (p) {
>  		int index = 0;
>  		int found;
> @@ -2386,7 +2385,7 @@ static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
>  	dev_addr[2] = 0x20;
>  	get_random_bytes(&dev_addr[3], 3);
>  }
> -#endif /* !(CONFIG_SPARC) */
> +#endif

Hi Sean,

I think this problem was added by patch 6/9,
so perhaps best to squash it into that patch.

Actually, I'd squash all these changes into 6/9.
But I don't feel strongly about it.

So in any case,

Reviewed-by: Simon Horman <simon.horman@corigine.com>

I will pause my review here (again!) because I need to go to a football match.

Re: [PATCH net-next v3 7/9] net: sunhme: Clean up mac address init

[Sean Anderson]

On 3/18/23 05:03, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:11PM -0400, Sean Anderson wrote:
>> Clean up some oddities suggested during review.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - New
>>
>>   drivers/net/ethernet/sun/sunhme.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
>> index c2737f26afbe..1f27e99abf17 100644
>> --- a/drivers/net/ethernet/sun/sunhme.c
>> +++ b/drivers/net/ethernet/sun/sunhme.c
>> @@ -2348,9 +2348,7 @@ static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsi
>>   		p += 6;
>>   
>>   		if (index == 0) {
>> -			int i;
>> -
>> -			for (i = 0; i < 6; i++)
>> +			for (int i = 0; i < 6; i++)
>>   				dev_addr[i] = readb(p + i);
>>   			return 1;
>>   		}
>> @@ -2362,9 +2360,10 @@ static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsi
>>   static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
>>   						unsigned char *dev_addr)
>>   {
>> +	void __iomem *p;
>>   	size_t size;
>> -	void __iomem *p = pci_map_rom(pdev, &size);
>>   
>> +	p = pci_map_rom(pdev, &size);
>>   	if (p) {
>>   		int index = 0;
>>   		int found;
>> @@ -2386,7 +2385,7 @@ static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
>>   	dev_addr[2] = 0x20;
>>   	get_random_bytes(&dev_addr[3], 3);
>>   }
>> -#endif /* !(CONFIG_SPARC) */
>> +#endif
> 
> Hi Sean,
> 
> I think this problem was added by patch 6/9,
> so perhaps best to squash it into that patch.

> Actually, I'd squash all these changes into 6/9.
> But I don't feel strongly about it.

6/9 just moves code around. I am keeping the modifications for other commits.

--Sean

> So in any case,
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 
> I will pause my review here (again!) because I need to go to a football match.

Re: [PATCH net-next v3 7/9] net: sunhme: Clean up mac address init

[Simon Horman]

On Sat, Mar 18, 2023 at 11:43:16AM -0400, Sean Anderson wrote:
> On 3/18/23 05:03, Simon Horman wrote:
> > On Mon, Mar 13, 2023 at 08:36:11PM -0400, Sean Anderson wrote:
> > > Clean up some oddities suggested during review.
> > > 
> > > Signed-off-by: Sean Anderson <seanga2@gmail.com>

...

> > > @@ -2386,7 +2385,7 @@ static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev,
> > >   	dev_addr[2] = 0x20;
> > >   	get_random_bytes(&dev_addr[3], 3);
> > >   }
> > > -#endif /* !(CONFIG_SPARC) */
> > > +#endif
> > 
> > Hi Sean,
> > 
> > I think this problem was added by patch 6/9,
> > so perhaps best to squash it into that patch.
> 
> > Actually, I'd squash all these changes into 6/9.
> > But I don't feel strongly about it.
> 
> 6/9 just moves code around. I am keeping the modifications for other commits.

Ack.

[PATCH net-next v3 8/9] net: sunhme: Inline error returns

[Sean Anderson]

The err_out label used to have cleanup. Now that it just returns, inline it
everywhere. While we're at it, fix an uninitialized return code if we never
found a qfe slot.

Fixes: 96c6e9faecf1 ("sunhme: forward the error code from pci_enable_device()")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v3:
- Incorperate a fix from another series into this commit

Changes in v2:
- New

 drivers/net/ethernet/sun/sunhme.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index 1f27e99abf17..a59b998062d9 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2622,29 +2622,25 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 
 	err = pcim_enable_device(pdev);
 	if (err)
-		goto err_out;
+		return err;
 	pci_set_master(pdev);
 
 	if (!strcmp(prom_name, "SUNW,qfe") || !strcmp(prom_name, "qfe")) {
 		qp = quattro_pci_find(pdev);
-		if (IS_ERR(qp)) {
-			err = PTR_ERR(qp);
-			goto err_out;
-		}
+		if (IS_ERR(qp))
+			return PTR_ERR(qp);
 
 		for (qfe_slot = 0; qfe_slot < 4; qfe_slot++)
 			if (!qp->happy_meals[qfe_slot])
 				break;
 
 		if (qfe_slot == 4)
-			goto err_out;
+			return -ENODEV;
 	}
 
 	dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));
-	if (!dev) {
-		err = -ENOMEM;
-		goto err_out;
-	}
+	if (!dev)
+		return -ENOMEM;
 	SET_NETDEV_DEV(dev, &pdev->dev);
 
 	hp = netdev_priv(dev);
@@ -2792,8 +2788,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 err_out_clear_quattro:
 	if (qp != NULL)
 		qp->happy_meals[qfe_slot] = NULL;
-
-err_out:
 	return err;
 }
 
-- 
2.37.1

Re: [PATCH net-next v3 8/9] net: sunhme: Inline error returns

[Jakub Kicinski]

On Mon, 13 Mar 2023 20:36:12 -0400 Sean Anderson wrote:
> Fixes: 96c6e9faecf1 ("sunhme: forward the error code from pci_enable_device()")

No such	commit in our trees :(

Re: [PATCH net-next v3 8/9] net: sunhme: Inline error returns

[Sean Anderson]

On 3/15/23 03:51, Jakub Kicinski wrote:
> On Mon, 13 Mar 2023 20:36:12 -0400 Sean Anderson wrote:
>> Fixes: 96c6e9faecf1 ("sunhme: forward the error code from pci_enable_device()")
> 
> No such	commit in our trees :(

Ah, looks like I forgot this was bad when copying the other commit. It should be acb3f35f920b.

--Sean

Re: [PATCH net-next v3 8/9] net: sunhme: Inline error returns

[Simon Horman]

On Wed, Mar 15, 2023 at 11:36:25AM -0400, Sean Anderson wrote:
> On 3/15/23 03:51, Jakub Kicinski wrote:
> > On Mon, 13 Mar 2023 20:36:12 -0400 Sean Anderson wrote:
> > > Fixes: 96c6e9faecf1 ("sunhme: forward the error code from pci_enable_device()")
> > 
> > No such	commit in our trees :(
> 
> Ah, looks like I forgot this was bad when copying the other commit. It should be acb3f35f920b.

I agree that is the correct commit.
And the code changes look good to me.

But I wonder if the fix should be separated into another patch,
separate from the cleanup.

[PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks

[Sean Anderson]

Most of the second half of the PCI/SBUS probe functions are the same.
Consolidate them into a common function.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/net/ethernet/sun/sunhme.c | 183 ++++++++++++------------------
 1 file changed, 71 insertions(+), 112 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index a59b998062d9..a384b162c46d 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2430,6 +2430,71 @@ static void happy_meal_addr_init(struct happy_meal *hp,
 	}
 }
 
+static int happy_meal_common_probe(struct happy_meal *hp,
+				   struct device_node *dp, int minor_rev)
+{
+	struct net_device *dev = hp->dev;
+	int err;
+
+#ifdef CONFIG_SPARC
+	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
+	if (hp->hm_revision == 0xff)
+		hp->hm_revision = 0xc0 | minor_rev;
+#else
+	/* works with this on non-sparc hosts */
+	hp->hm_revision = 0x20;
+#endif
+
+	/* Now enable the feature flags we can. */
+	if (hp->hm_revision == 0x20 || hp->hm_revision == 0x21)
+		hp->happy_flags |= HFLAG_20_21;
+	else if (hp->hm_revision != 0xa0)
+		hp->happy_flags |= HFLAG_NOT_A0;
+
+	hp->happy_block = dmam_alloc_coherent(hp->dma_dev, PAGE_SIZE,
+					      &hp->hblock_dvma, GFP_KERNEL);
+	if (!hp->happy_block)
+		return -ENOMEM;
+
+	/* Force check of the link first time we are brought up. */
+	hp->linkcheck = 0;
+
+	/* Force timer state to 'asleep' with count of zero. */
+	hp->timer_state = asleep;
+	hp->timer_ticks = 0;
+
+	timer_setup(&hp->happy_timer, happy_meal_timer, 0);
+
+	dev->netdev_ops = &hme_netdev_ops;
+	dev->watchdog_timeo = 5 * HZ;
+	dev->ethtool_ops = &hme_ethtool_ops;
+
+	/* Happy Meal can do it all... */
+	dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM;
+	dev->features |= dev->hw_features | NETIF_F_RXCSUM;
+
+#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
+	/* Hook up SBUS register/descriptor accessors. */
+	hp->read_desc32 = sbus_hme_read_desc32;
+	hp->write_txd = sbus_hme_write_txd;
+	hp->write_rxd = sbus_hme_write_rxd;
+	hp->read32 = sbus_hme_read32;
+	hp->write32 = sbus_hme_write32;
+#endif
+
+	/* Grrr, Happy Meal comes up by default not advertising
+	 * full duplex 100baseT capabilities, fix this.
+	 */
+	spin_lock_irq(&hp->happy_lock);
+	happy_meal_set_initial_advertisement(hp);
+	spin_unlock_irq(&hp->happy_lock);
+
+	err = devm_register_netdev(hp->dma_dev, dev);
+	if (err)
+		dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
+	return err;
+}
+
 #ifdef CONFIG_SBUS
 static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 {
@@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
 		goto err_out_clear_quattro;
 	}
 
-	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
-	if (hp->hm_revision == 0xff)
-		hp->hm_revision = 0xa0;
-
-	/* Now enable the feature flags we can. */
-	if (hp->hm_revision == 0x20 || hp->hm_revision == 0x21)
-		hp->happy_flags = HFLAG_20_21;
-	else if (hp->hm_revision != 0xa0)
-		hp->happy_flags = HFLAG_NOT_A0;
-
 	if (qp != NULL)
 		hp->happy_flags |= HFLAG_QUATTRO;
 
+	hp->irq = op->archdata.irqs[0];
+
 	/* Get the supported DVMA burst sizes from our Happy SBUS. */
 	hp->happy_bursts = of_getintprop_default(sbus_dp,
 						 "burst-sizes", 0x00);
 
-	hp->happy_block = dmam_alloc_coherent(&op->dev, PAGE_SIZE,
-					      &hp->hblock_dvma, GFP_KERNEL);
-	if (!hp->happy_block) {
-		err = -ENOMEM;
+	err = happy_meal_common_probe(hp, dp, 0);
+	if (err)
 		goto err_out_clear_quattro;
-	}
-
-	/* Force check of the link first time we are brought up. */
-	hp->linkcheck = 0;
-
-	/* Force timer state to 'asleep' with count of zero. */
-	hp->timer_state = asleep;
-	hp->timer_ticks = 0;
-
-	timer_setup(&hp->happy_timer, happy_meal_timer, 0);
-
-	dev->netdev_ops = &hme_netdev_ops;
-	dev->watchdog_timeo = 5*HZ;
-	dev->ethtool_ops = &hme_ethtool_ops;
-
-	/* Happy Meal can do it all... */
-	dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM;
-	dev->features |= dev->hw_features | NETIF_F_RXCSUM;
-
-	hp->irq = op->archdata.irqs[0];
-
-#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
-	/* Hook up SBUS register/descriptor accessors. */
-	hp->read_desc32 = sbus_hme_read_desc32;
-	hp->write_txd = sbus_hme_write_txd;
-	hp->write_rxd = sbus_hme_write_rxd;
-	hp->read32 = sbus_hme_read32;
-	hp->write32 = sbus_hme_write32;
-#endif
-
-	/* Grrr, Happy Meal comes up by default not advertising
-	 * full duplex 100baseT capabilities, fix this.
-	 */
-	spin_lock_irq(&hp->happy_lock);
-	happy_meal_set_initial_advertisement(hp);
-	spin_unlock_irq(&hp->happy_lock);
-
-	err = devm_register_netdev(&op->dev, dev);
-	if (err) {
-		dev_err(&op->dev, "Cannot register net device, aborting.\n");
-		goto err_out_clear_quattro;
-	}
 
 	platform_set_drvdata(op, hp);
 
@@ -2689,21 +2702,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	hp->bigmacregs = (hpreg_base + 0x6000UL);
 	hp->tcvregs    = (hpreg_base + 0x7000UL);
 
-#ifdef CONFIG_SPARC
-	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
-	if (hp->hm_revision == 0xff)
-		hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
-#else
-	/* works with this on non-sparc hosts */
-	hp->hm_revision = 0x20;
-#endif
-
-	/* Now enable the feature flags we can. */
-	if (hp->hm_revision == 0x20 || hp->hm_revision == 0x21)
-		hp->happy_flags = HFLAG_20_21;
-	else if (hp->hm_revision != 0xa0 && hp->hm_revision != 0xc0)
-		hp->happy_flags = HFLAG_NOT_A0;
-
 	if (qp != NULL)
 		hp->happy_flags |= HFLAG_QUATTRO;
 
@@ -2714,50 +2712,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	/* Assume PCI happy meals can handle all burst sizes. */
 	hp->happy_bursts = DMA_BURSTBITS;
 #endif
-
-	hp->happy_block = dmam_alloc_coherent(&pdev->dev, PAGE_SIZE,
-					      &hp->hblock_dvma, GFP_KERNEL);
-	if (!hp->happy_block) {
-		err = -ENOMEM;
-		goto err_out_clear_quattro;
-	}
-
-	hp->linkcheck = 0;
-	hp->timer_state = asleep;
-	hp->timer_ticks = 0;
-
-	timer_setup(&hp->happy_timer, happy_meal_timer, 0);
-
 	hp->irq = pdev->irq;
-	dev->netdev_ops = &hme_netdev_ops;
-	dev->watchdog_timeo = 5*HZ;
-	dev->ethtool_ops = &hme_ethtool_ops;
 
-	/* Happy Meal can do it all... */
-	dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM;
-	dev->features |= dev->hw_features | NETIF_F_RXCSUM;
-
-#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
-	/* Hook up PCI register/descriptor accessors. */
-	hp->read_desc32 = pci_hme_read_desc32;
-	hp->write_txd = pci_hme_write_txd;
-	hp->write_rxd = pci_hme_write_rxd;
-	hp->read32 = pci_hme_read32;
-	hp->write32 = pci_hme_write32;
-#endif
-
-	/* Grrr, Happy Meal comes up by default not advertising
-	 * full duplex 100baseT capabilities, fix this.
-	 */
-	spin_lock_irq(&hp->happy_lock);
-	happy_meal_set_initial_advertisement(hp);
-	spin_unlock_irq(&hp->happy_lock);
-
-	err = devm_register_netdev(&pdev->dev, dev);
-	if (err) {
-		dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
+	err = happy_meal_common_probe(hp, dp, pdev->revision & 0x0f);
+	if (err)
 		goto err_out_clear_quattro;
-	}
 
 	pci_set_drvdata(pdev, hp);
 
-- 
2.37.1

Re: [PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks

[Simon Horman]

On Mon, Mar 13, 2023 at 08:36:13PM -0400, Sean Anderson wrote:
> Most of the second half of the PCI/SBUS probe functions are the same.
> Consolidate them into a common function.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/net/ethernet/sun/sunhme.c | 183 ++++++++++++------------------
>  1 file changed, 71 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index a59b998062d9..a384b162c46d 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c
> @@ -2430,6 +2430,71 @@ static void happy_meal_addr_init(struct happy_meal *hp,
>  	}
>  }
>  
> +static int happy_meal_common_probe(struct happy_meal *hp,
> +				   struct device_node *dp, int minor_rev)
> +{
> +	struct net_device *dev = hp->dev;
> +	int err;
> +
> +#ifdef CONFIG_SPARC
> +	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> +	if (hp->hm_revision == 0xff)
> +		hp->hm_revision = 0xc0 | minor_rev;
> +#else
> +	/* works with this on non-sparc hosts */
> +	hp->hm_revision = 0x20;
> +#endif

...

> +#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> +	/* Hook up SBUS register/descriptor accessors. */
> +	hp->read_desc32 = sbus_hme_read_desc32;
> +	hp->write_txd = sbus_hme_write_txd;
> +	hp->write_rxd = sbus_hme_write_rxd;
> +	hp->read32 = sbus_hme_read32;
> +	hp->write32 = sbus_hme_write32;
> +#endif

This looks correct for the SBUS case.
But I'm not sure about the PCIE case.

gcc 12 tells me when compiling with sparc allmodconfig that the following
functions are now unused.

  pci_hme_read_desc32
  pci_hme_write_txd
  pci_hme_write_rxd
  pci_hme_read32
  pci_hme_write32

> +
> +	/* Grrr, Happy Meal comes up by default not advertising
> +	 * full duplex 100baseT capabilities, fix this.
b
> +	 */
> +	spin_lock_irq(&hp->happy_lock);
> +	happy_meal_set_initial_advertisement(hp);
> +	spin_unlock_irq(&hp->happy_lock);
> +
> +	err = devm_register_netdev(hp->dma_dev, dev);
> +	if (err)
> +		dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
> +	return err;
> +}
> +
>  #ifdef CONFIG_SBUS
>  static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>  {

> @@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>  		goto err_out_clear_quattro;
>  	}
>  
> -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> -	if (hp->hm_revision == 0xff)
> -		hp->hm_revision = 0xa0;

It's not clear to me that the same value will be set by the call to
happy_meal_common_probe(hp, dp, 0); where the logic is:

#ifdef CONFIG_SPARC
	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
	if (hp->hm_revision == 0xff)
		hp->hm_revision = 0xc0 | minor_rev;
#else
	/* works with this on non-sparc hosts */
	hp->hm_revision = 0x20;
#endif

I am assuming that the SPARC logic is run.
But another question: is it strictly true that SBUS means SPARC?

...

> -
> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> -	/* Hook up SBUS register/descriptor accessors. */
> -	hp->read_desc32 = sbus_hme_read_desc32;
> -	hp->write_txd = sbus_hme_write_txd;
> -	hp->write_rxd = sbus_hme_write_rxd;
> -	hp->read32 = sbus_hme_read32;
> -	hp->write32 = sbus_hme_write32;
> -#endif

...

> @@ -2689,21 +2702,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
>  	hp->bigmacregs = (hpreg_base + 0x6000UL);
>  	hp->tcvregs    = (hpreg_base + 0x7000UL);
>  
> -#ifdef CONFIG_SPARC
> -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> -	if (hp->hm_revision == 0xff)
> -		hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
> -#else
> -	/* works with this on non-sparc hosts */
> -	hp->hm_revision = 0x20;
> -#endif

...

> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> -	/* Hook up PCI register/descriptor accessors. */
> -	hp->read_desc32 = pci_hme_read_desc32;
> -	hp->write_txd = pci_hme_write_txd;
> -	hp->write_rxd = pci_hme_write_rxd;
> -	hp->read32 = pci_hme_read32;
> -	hp->write32 = pci_hme_write32;
> -#endif

...

Re: [PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks

[Sean Anderson]

On 3/18/23 09:53, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:13PM -0400, Sean Anderson wrote:
>> Most of the second half of the PCI/SBUS probe functions are the same.
>> Consolidate them into a common function.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v1)
>>
>>   drivers/net/ethernet/sun/sunhme.c | 183 ++++++++++++------------------
>>   1 file changed, 71 insertions(+), 112 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
>> index a59b998062d9..a384b162c46d 100644
>> --- a/drivers/net/ethernet/sun/sunhme.c
>> +++ b/drivers/net/ethernet/sun/sunhme.c
>> @@ -2430,6 +2430,71 @@ static void happy_meal_addr_init(struct happy_meal *hp,
>>   	}
>>   }
>>   
>> +static int happy_meal_common_probe(struct happy_meal *hp,
>> +				   struct device_node *dp, int minor_rev)
>> +{
>> +	struct net_device *dev = hp->dev;
>> +	int err;
>> +
>> +#ifdef CONFIG_SPARC
>> +	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> +	if (hp->hm_revision == 0xff)
>> +		hp->hm_revision = 0xc0 | minor_rev;
>> +#else
>> +	/* works with this on non-sparc hosts */
>> +	hp->hm_revision = 0x20;
>> +#endif
> 
> ...
> 
>> +#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> +	/* Hook up SBUS register/descriptor accessors. */
>> +	hp->read_desc32 = sbus_hme_read_desc32;
>> +	hp->write_txd = sbus_hme_write_txd;
>> +	hp->write_rxd = sbus_hme_write_rxd;
>> +	hp->read32 = sbus_hme_read32;
>> +	hp->write32 = sbus_hme_write32;
>> +#endif
> 
> This looks correct for the SBUS case.
> But I'm not sure about the PCIE case.
> 
> gcc 12 tells me when compiling with sparc allmodconfig that the following
> functions are now unused.
> 
>    pci_hme_read_desc32
>    pci_hme_write_txd
>    pci_hme_write_rxd
>    pci_hme_read32
>    pci_hme_write32

Oh, looks like I missed that these were different while refactoring.

That said, I haven't seen any issues here...

>> +
>> +	/* Grrr, Happy Meal comes up by default not advertising
>> +	 * full duplex 100baseT capabilities, fix this.
> b
>> +	 */
>> +	spin_lock_irq(&hp->happy_lock);
>> +	happy_meal_set_initial_advertisement(hp);
>> +	spin_unlock_irq(&hp->happy_lock);
>> +
>> +	err = devm_register_netdev(hp->dma_dev, dev);
>> +	if (err)
>> +		dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
>> +	return err;
>> +}
>> +
>>   #ifdef CONFIG_SBUS
>>   static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>>   {
> 
>> @@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>>   		goto err_out_clear_quattro;
>>   	}
>>   
>> -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> -	if (hp->hm_revision == 0xff)
>> -		hp->hm_revision = 0xa0;
> 
> It's not clear to me that the same value will be set by the call to
> happy_meal_common_probe(hp, dp, 0); where the logic is:
> 
> #ifdef CONFIG_SPARC
> 	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> 	if (hp->hm_revision == 0xff)
> 		hp->hm_revision = 0xc0 | minor_rev;

OK, so maybe this should be xor, with sbus passing in 0x30.

> #else
> 	/* works with this on non-sparc hosts */
> 	hp->hm_revision = 0x20;
> #endif
> 
> I am assuming that the SPARC logic is run.
> But another question: is it strictly true that SBUS means SPARC?

Yes.

>> -
>> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> -	/* Hook up SBUS register/descriptor accessors. */
>> -	hp->read_desc32 = sbus_hme_read_desc32;
>> -	hp->write_txd = sbus_hme_write_txd;
>> -	hp->write_rxd = sbus_hme_write_rxd;
>> -	hp->read32 = sbus_hme_read32;
>> -	hp->write32 = sbus_hme_write32;
>> -#endif
> 
> ...
> 
>> @@ -2689,21 +2702,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
>>   	hp->bigmacregs = (hpreg_base + 0x6000UL);
>>   	hp->tcvregs    = (hpreg_base + 0x7000UL);
>>   
>> -#ifdef CONFIG_SPARC
>> -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> -	if (hp->hm_revision == 0xff)
>> -		hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
>> -#else
>> -	/* works with this on non-sparc hosts */
>> -	hp->hm_revision = 0x20;
>> -#endif
> 
> ...
> 
>> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> -	/* Hook up PCI register/descriptor accessors. */
>> -	hp->read_desc32 = pci_hme_read_desc32;
>> -	hp->write_txd = pci_hme_write_txd;
>> -	hp->write_rxd = pci_hme_write_rxd;
>> -	hp->read32 = pci_hme_read32;
>> -	hp->write32 = pci_hme_write32;
>> -#endif
> 
> ...

--Sean

Re: [PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks

[Simon Horman]

On Sat, Mar 18, 2023 at 10:40:46AM -0400, Sean Anderson wrote:
> On 3/18/23 09:53, Simon Horman wrote:
> > On Mon, Mar 13, 2023 at 08:36:13PM -0400, Sean Anderson wrote:
> > > Most of the second half of the PCI/SBUS probe functions are the same.
> > > Consolidate them into a common function.
> > > 
> > > Signed-off-by: Sean Anderson <seanga2@gmail.com>

...

> > > @@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
> > >   		goto err_out_clear_quattro;
> > >   	}
> > > -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> > > -	if (hp->hm_revision == 0xff)
> > > -		hp->hm_revision = 0xa0;
> > 
> > It's not clear to me that the same value will be set by the call to
> > happy_meal_common_probe(hp, dp, 0); where the logic is:
> > 
> > #ifdef CONFIG_SPARC
> > 	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> > 	if (hp->hm_revision == 0xff)
> > 		hp->hm_revision = 0xc0 | minor_rev;
> 
> OK, so maybe this should be xor, with sbus passing in 0x30.

Maybe moving the math to the caller makes things easier.
I'm unsure.

> > #else
> > 	/* works with this on non-sparc hosts */
> > 	hp->hm_revision = 0x20;
> > #endif
> > 
> > I am assuming that the SPARC logic is run.
> > But another question: is it strictly true that SBUS means SPARC?
> 
> Yes.

Thanks, got it.