[PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode

[PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode

[Christian Lamparter]

The RGMII spec allows compliance for devices that implement an internal
delay on TXC and/or RXC inside the transmitter. This patch adds the
necessary RGMII_[RX|TX]ID mode code to handle such PHYs with the
emac driver.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>

---
v2: - utilize phy_interface_mode_is_rgmii()
---
 drivers/net/ethernet/ibm/emac/core.c  |  4 ++--
 drivers/net/ethernet/ibm/emac/emac.h  |  3 +++
 drivers/net/ethernet/ibm/emac/rgmii.c | 10 ++++++++--
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 7feff2450ed6..043e72e28bba 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -199,8 +199,8 @@ static void __emac_set_multicast_list(struct emac_instance *dev);
 
 static inline int emac_phy_supports_gige(int phy_mode)
 {
-	return  phy_mode == PHY_MODE_GMII ||
-		phy_mode == PHY_MODE_RGMII ||
+	return  phy_interface_mode_is_rgmii(phy_mode) ||
+		phy_mode == PHY_MODE_GMII ||
 		phy_mode == PHY_MODE_SGMII ||
 		phy_mode == PHY_MODE_TBI ||
 		phy_mode == PHY_MODE_RTBI;
diff --git a/drivers/net/ethernet/ibm/emac/emac.h b/drivers/net/ethernet/ibm/emac/emac.h
index 5afcc27ceebb..8c6d2af7281b 100644
--- a/drivers/net/ethernet/ibm/emac/emac.h
+++ b/drivers/net/ethernet/ibm/emac/emac.h
@@ -112,6 +112,9 @@ struct emac_regs {
 #define PHY_MODE_RMII	PHY_INTERFACE_MODE_RMII
 #define PHY_MODE_SMII	PHY_INTERFACE_MODE_SMII
 #define PHY_MODE_RGMII	PHY_INTERFACE_MODE_RGMII
+#define PHY_MODE_RGMII_ID	PHY_INTERFACE_MODE_RGMII_ID
+#define PHY_MODE_RGMII_RXID	PHY_INTERFACE_MODE_RGMII_RXID
+#define PHY_MODE_RGMII_TXID	PHY_INTERFACE_MODE_RGMII_TXID
 #define PHY_MODE_TBI	PHY_INTERFACE_MODE_TBI
 #define PHY_MODE_GMII	PHY_INTERFACE_MODE_GMII
 #define PHY_MODE_RTBI	PHY_INTERFACE_MODE_RTBI
diff --git a/drivers/net/ethernet/ibm/emac/rgmii.c b/drivers/net/ethernet/ibm/emac/rgmii.c
index c4a1ac38bba8..124b0473d2b7 100644
--- a/drivers/net/ethernet/ibm/emac/rgmii.c
+++ b/drivers/net/ethernet/ibm/emac/rgmii.c
@@ -52,9 +52,9 @@
 /* RGMII bridge supports only GMII/TBI and RGMII/RTBI PHYs */
 static inline int rgmii_valid_mode(int phy_mode)
 {
-	return  phy_mode == PHY_MODE_GMII ||
+	return  phy_interface_mode_is_rgmii(phy_mode) ||
+		phy_mode == PHY_MODE_GMII ||
 		phy_mode == PHY_MODE_MII ||
-		phy_mode == PHY_MODE_RGMII ||
 		phy_mode == PHY_MODE_TBI ||
 		phy_mode == PHY_MODE_RTBI;
 }
@@ -63,6 +63,9 @@ static inline const char *rgmii_mode_name(int mode)
 {
 	switch (mode) {
 	case PHY_MODE_RGMII:
+	case PHY_MODE_RGMII_ID:
+	case PHY_MODE_RGMII_RXID:
+	case PHY_MODE_RGMII_TXID:
 		return "RGMII";
 	case PHY_MODE_TBI:
 		return "TBI";
@@ -81,6 +84,9 @@ static inline u32 rgmii_mode_mask(int mode, int input)
 {
 	switch (mode) {
 	case PHY_MODE_RGMII:
+	case PHY_MODE_RGMII_ID:
+	case PHY_MODE_RGMII_RXID:
+	case PHY_MODE_RGMII_TXID:
 		return RGMII_FER_RGMII(input);
 	case PHY_MODE_TBI:
 		return RGMII_FER_TBI(input);
-- 
2.15.1

Re: [PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode

[David Miller]

From: Christian Lamparter <chunkeey@gmail.com>
Date: Wed, 20 Dec 2017 17:02:01 +0100

> diff --git a/drivers/net/ethernet/ibm/emac/emac.h b/drivers/net/ethernet/ibm/emac/emac.h
> index 5afcc27ceebb..8c6d2af7281b 100644
> --- a/drivers/net/ethernet/ibm/emac/emac.h
> +++ b/drivers/net/ethernet/ibm/emac/emac.h
> @@ -112,6 +112,9 @@ struct emac_regs {
>  #define PHY_MODE_RMII	PHY_INTERFACE_MODE_RMII
>  #define PHY_MODE_SMII	PHY_INTERFACE_MODE_SMII
>  #define PHY_MODE_RGMII	PHY_INTERFACE_MODE_RGMII
> +#define PHY_MODE_RGMII_ID	PHY_INTERFACE_MODE_RGMII_ID
> +#define PHY_MODE_RGMII_RXID	PHY_INTERFACE_MODE_RGMII_RXID
> +#define PHY_MODE_RGMII_TXID	PHY_INTERFACE_MODE_RGMII_TXID
>  #define PHY_MODE_TBI	PHY_INTERFACE_MODE_TBI
>  #define PHY_MODE_GMII	PHY_INTERFACE_MODE_GMII
>  #define PHY_MODE_RTBI	PHY_INTERFACE_MODE_RTBI

Why does this driver do all of this CPP macro aliasing?

It's really cruddy because anyone who now reads this code:

>  static inline int rgmii_valid_mode(int phy_mode)
>  {
> -	return  phy_mode == PHY_MODE_GMII ||
> +	return  phy_interface_mode_is_rgmii(phy_mode) ||
> +		phy_mode == PHY_MODE_GMII ||
>  		phy_mode == PHY_MODE_MII ||
> -		phy_mode == PHY_MODE_RGMII ||
>  		phy_mode == PHY_MODE_TBI ||
>  		phy_mode == PHY_MODE_RTBI;
>  }

will read this and say "Oh the function tests against these weird
PHY_MODE_* aliases, but the helper function phy_interface_mode_is_rgmii()
tests against PHY_INTERFACE_MODE_*, what is going on?"

I hate to do this to you, but please get rid of these confusing and
completely unnecessary PHY_MODE_* CPP aliases first, and just use the
proper PHY_INTERFACE_MODE_* values consistently.

Thank you.

Re: [PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode

[Christian Lamparter]

On Wednesday, December 20, 2017 3:10:46 PM CET David Miller wrote:
> From: Christian Lamparter <chunkeey@gmail.com>
> Date: Wed, 20 Dec 2017 17:02:01 +0100
> 
> > diff --git a/drivers/net/ethernet/ibm/emac/emac.h b/drivers/net/ethernet/ibm/emac/emac.h
> > index 5afcc27ceebb..8c6d2af7281b 100644
> > --- a/drivers/net/ethernet/ibm/emac/emac.h
> > +++ b/drivers/net/ethernet/ibm/emac/emac.h
> > @@ -112,6 +112,9 @@ struct emac_regs {
> >  #define PHY_MODE_RMII	PHY_INTERFACE_MODE_RMII
> >  #define PHY_MODE_SMII	PHY_INTERFACE_MODE_SMII
> >  #define PHY_MODE_RGMII	PHY_INTERFACE_MODE_RGMII
> > +#define PHY_MODE_RGMII_ID	PHY_INTERFACE_MODE_RGMII_ID
> > +#define PHY_MODE_RGMII_RXID	PHY_INTERFACE_MODE_RGMII_RXID
> > +#define PHY_MODE_RGMII_TXID	PHY_INTERFACE_MODE_RGMII_TXID
> >  #define PHY_MODE_TBI	PHY_INTERFACE_MODE_TBI
> >  #define PHY_MODE_GMII	PHY_INTERFACE_MODE_GMII
> >  #define PHY_MODE_RTBI	PHY_INTERFACE_MODE_RTBI
> 
> Why does this driver do all of this CPP macro aliasing?
Ah, well the emac driver is almost as old as dirt ;).
I added Benjamin Herrenschmidt since he seems to be the
original author. maybe he can provide some valuable insights
in this archaeological dig.

I don't know when the ibm_emac driver was added, but it does
predate the shared PHY_INTERFACE_MODE_ macros by a few years.

For example 2.6.11 had the driver and the defines.:
<http://elixir.free-electrons.com/linux/v2.6.11/source/drivers/net/ibm_emac/ibm_emac_phy.h#L41>
But there's no PHY_INTERFACE_MODE_* yet.

Fast forward to 2011:
The patch <https://patchwork.kernel.org/patch/945642/> wedded the
PHY_MODE_* macros to the PHY_INTERFACE_MODE_ enums. But this was
not a complete convertion.

So, as far as I can tell, these definitions have been there since
the beginning.
> 
> It's really cruddy because anyone who now reads this code:
> 
> >  static inline int rgmii_valid_mode(int phy_mode)
> >  {
> > -	return  phy_mode == PHY_MODE_GMII ||
> > +	return  phy_interface_mode_is_rgmii(phy_mode) ||
> > +		phy_mode == PHY_MODE_GMII ||
> >  		phy_mode == PHY_MODE_MII ||
> > -		phy_mode == PHY_MODE_RGMII ||
> >  		phy_mode == PHY_MODE_TBI ||
> >  		phy_mode == PHY_MODE_RTBI;
> >  }
> 
> will read this and say "Oh the function tests against these weird
> PHY_MODE_* aliases, but the helper function phy_interface_mode_is_rgmii()
> tests against PHY_INTERFACE_MODE_*, what is going on?"
> 
> I hate to do this to you, but please get rid of these confusing and
> completely unnecessary PHY_MODE_* CPP aliases first, and just use the
> proper PHY_INTERFACE_MODE_* values consistently.

Yeah, I can do that. no problem. 

Question is, should I also replace the rgmii_mode_name() with phy_modes() too?

The only user of rgmii_mode_name() is this notice printk in rgmii_attach():
<http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/ibm/emac/rgmii.c#L117>

Regards,
Christian

Re: [PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode

[David Miller]

From: Christian Lamparter <chunkeey@gmail.com>
Date: Wed, 20 Dec 2017 22:07:43 +0100

> Yeah, I can do that. no problem. 
> 
> Question is, should I also replace the rgmii_mode_name() with phy_modes() too?
> 
> The only user of rgmii_mode_name() is this notice printk in rgmii_attach():
> <http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/ibm/emac/rgmii.c#L117>

I would recommend you use phy_modes() instead of that custom rgmii_mode_name() thing,
yes.

Thanks for asking.

Re: [PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode

[Benjamin Herrenschmidt]

On Wed, 2017-12-20 at 22:07 +0100, Christian Lamparter wrote:
> 
> > will read this and say "Oh the function tests against these weird
> > PHY_MODE_* aliases, but the helper function phy_interface_mode_is_rgmii()
> > tests against PHY_INTERFACE_MODE_*, what is going on?"
> > 
> > I hate to do this to you, but please get rid of these confusing and
> > completely unnecessary PHY_MODE_* CPP aliases first, and just use the
> > proper PHY_INTERFACE_MODE_* values consistently.
> 
> Yeah, I can do that. no problem. 
> 
> Question is, should I also replace the rgmii_mode_name() with phy_modes() too?
> 
> The only user of rgmii_mode_name() is this notice printk in rgmii_attach():
> <http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/ibm/emac/rgmii.c#L117>

Yup, this is all pre-hisorical gunk, feel free to replace it.

Cheers,
Ben.