Message ID | 20171220160201.17143-1-chunkeey@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | [v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode | expand |
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.
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
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.
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.
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);
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(-)