| Message ID | 38623984f6235a1521e6b0ad2ea958abc84ad708.1670119328.git.piergiorgio.beruto@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/4] net/ethtool: Add netlink interface for the PLCA RS | expand |
On Sun, Dec 04, 2022 at 03:32:06AM +0100, Piergiorgio Beruto wrote: > This patch adds PLCA support to the ncn26000 driver. Also add helper > functions to read/write standard OPEN Alliance PLCA registers to > phylib (genphy_c45_plca_get_cfg, genphy_c45_plca_set_cfg, > genphy_c45_plca_get_status). > > Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com> > --- > drivers/net/phy/ncn26000.c | 20 +++- > drivers/net/phy/phy-c45.c | 187 +++++++++++++++++++++++++++++++++++++ > include/linux/phy.h | 10 +- > include/uapi/linux/mdio.h | 31 ++++++ > net/ethtool/plca.c | 2 +- > 5 files changed, 245 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c > index 65a34edc5b20..39197823cf16 100644 > --- a/drivers/net/phy/ncn26000.c > +++ b/drivers/net/phy/ncn26000.c > @@ -27,14 +27,27 @@ > #define NCN26000_IRQ_PLCAREC_BIT ((u16)(1 << 4)) > #define NCN26000_IRQ_PHYSCOL_BIT ((u16)(1 << 5)) > > +#define TO_TMR_DEFAULT ((u16)32) > + > struct ncn26000_priv { > u16 enabled_irqs; > }; > > static int ncn26000_config_init(struct phy_device *phydev) > { > - // TODO: add vendor-specific tuning (ENI, CMC, ...) > - return 0; > + int ret; > + > + /* HW bug workaround: the default value of the PLCA TO_TIMER should be > + * 32, where the current version of NCN26000 reports 24. This will be > + * fixed in future PHY versions. For the time being, we force the right > + * default here. > + */ > + ret = phy_write_mmd(phydev, > + MDIO_MMD_OATC14, > + MDIO_OATC14_PLCA_TOTMR, > + TO_TMR_DEFAULT); Better formatting please. return phy_write_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_TOTMR, TO_TMR_DEFAULT); is sufficient. No need for "ret" (and there are folk who will create a cleanup patch to do this, so might as well get it right on submission.) > +/** > + * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers > + * @phydev: target phy_device struct > + * @plca_cfg: output structure to store the PLCA configuration > + * > + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA > + * Management Registers specifications, this function can be used to retrieve > + * the current PLCA configuration from the standard registers in MMD 31. > + */ > +int genphy_c45_plca_get_cfg(struct phy_device *phydev, > + struct phy_plca_cfg *plca_cfg) > +{ > + int ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_IDVER); > + if (ret < 0) > + return ret; > + > + plca_cfg->version = (u32)ret; ->version has type s32, so is signed. Clearly, from the above code, it can't be negative (since negative integer values are an error.) So why is ->version declared in patch 1 as signed? The cast here to u32 also seems strange. Also, since the register you're reading can be no more than 16 bits wide, using s32 seems like a waste. > + > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_CTRL0); > + if (ret < 0) > + return ret; > + > + plca_cfg->enabled = !!(((u16)ret) & MDIO_OATC14_PLCA_EN); ->enabled has type s16, but it clearly boolean in nature. It could be a u8 instead. No need for that u16 cast either. > + > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_CTRL1); > + if (ret < 0) > + return ret; > + > + plca_cfg->node_cnt = (((u16)ret) & MDIO_OATC14_PLCA_NCNT) >> 8; > + plca_cfg->node_id = (((u16)ret) & MDIO_OATC14_PLCA_ID); Also declared to be signed, but clearly aren't. u16 cast is unnecessary. > + > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_TOTMR); > + if (ret < 0) > + return ret; > + > + plca_cfg->to_tmr = (u16)ret & MDIO_OATC14_PLCA_TOT; Ditto. > + > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_BURST); > + if (ret < 0) > + return ret; > + > + plca_cfg->burst_cnt = (((u16)ret) & MDIO_OATC14_PLCA_MAXBC) >> 8; > + plca_cfg->burst_tmr = (((u16)ret) & MDIO_OATC14_PLCA_BTMR); And same again. > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(genphy_c45_plca_get_cfg); > + > +/** > + * genphy_c45_plca_set_cfg - set PLCA configuration using standard registers > + * @phydev: target phy_device struct > + * @plca_cfg: structure containing the PLCA configuration. Fields set to -1 are > + * not to be changed. > + * > + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA > + * Management Registers specifications, this function can be used to modify > + * the PLCA configuration using the standard registers in MMD 31. > + */ > +int genphy_c45_plca_set_cfg(struct phy_device *phydev, > + const struct phy_plca_cfg *plca_cfg) > +{ > + int ret; > + u16 val; > + > + // PLCA IDVER is read-only > + if (plca_cfg->version >= 0) > + return -EINVAL; Given that we've established this can't be negative, this seems like a bit of a silly check. > + > + // first of all, disable PLCA if required > + if (plca_cfg->enabled == 0) { > + ret = phy_clear_bits_mmd(phydev, > + MDIO_MMD_OATC14, > + MDIO_OATC14_PLCA_CTRL0, > + MDIO_OATC14_PLCA_EN); > + > + if (ret < 0) > + return ret; > + } Does this need to be disabled when making changes? Just wondering why you handle this disable explicitly early. > + > + if (plca_cfg->node_cnt >= 0 || plca_cfg->node_id >= 0) { > + if (plca_cfg->node_cnt < 0 || plca_cfg->node_id < 0) { > + ret = phy_read_mmd(phydev, > + MDIO_MMD_OATC14, > + MDIO_OATC14_PLCA_CTRL1); > + > + if (ret < 0) > + return ret; > + > + val = (u16)ret; > + } > + > + if (plca_cfg->node_cnt >= 0) > + val = (val & ~MDIO_OATC14_PLCA_NCNT) | > + (u16)(plca_cfg->node_cnt << 8); > + > + if (plca_cfg->node_id >= 0) > + val = (val & ~MDIO_OATC14_PLCA_ID) | > + (u16)(plca_cfg->node_id); > + > + ret = phy_write_mmd(phydev, > + MDIO_MMD_OATC14, > + MDIO_OATC14_PLCA_CTRL1, > + val); Safer to use phy_modify_mmd() and build up the mask as necessary. It also seems odd to have this "positive numbers mean update" thing when every other ethtool API just programs whatever is passed. It seems to be a different API theory to the rest of the ethtool established principle that if one wishes to modify, one calls the _GET followed by a _SET. This applies throughout this function. > + > + if (ret < 0) > + return ret; > + } > + > + if (plca_cfg->to_tmr >= 0) { > + ret = phy_write_mmd(phydev, > + MDIO_MMD_OATC14, > + MDIO_OATC14_PLCA_TOTMR, > + (u16)plca_cfg->to_tmr); > + > + if (ret < 0) > + return ret; > + } > + > + if (plca_cfg->burst_cnt >= 0 || plca_cfg->burst_tmr >= 0) { > + if (plca_cfg->burst_cnt < 0 || plca_cfg->burst_tmr < 0) { > + ret = phy_read_mmd(phydev, > + MDIO_MMD_OATC14, > + MDIO_OATC14_PLCA_BURST); > + > + if (ret < 0) > + return ret; > + > + val = (u16)ret; > + } > + > + if (plca_cfg->burst_cnt >= 0) > + val = (val & ~MDIO_OATC14_PLCA_MAXBC) | > + (u16)(plca_cfg->burst_cnt << 8); > + > + if (plca_cfg->burst_tmr >= 0) > + val = (val & ~MDIO_OATC14_PLCA_BTMR) | > + (u16)(plca_cfg->burst_tmr); > + > + ret = phy_write_mmd(phydev, > + MDIO_MMD_OATC14, > + MDIO_OATC14_PLCA_BURST, > + val); > + > + if (ret < 0) > + return ret; > + } > + > + // if we need to enable PLCA, do it at the end > + if (plca_cfg->enabled > 0) { > + ret = phy_set_bits_mmd(phydev, > + MDIO_MMD_OATC14, > + MDIO_OATC14_PLCA_CTRL0, > + MDIO_OATC14_PLCA_EN); > + > + if (ret < 0) > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(genphy_c45_plca_set_cfg); > + > +/** > + * genphy_c45_plca_get_status - get PLCA status from standard registers > + * @phydev: target phy_device struct > + * @plca_st: output structure to store the PLCA status > + * > + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA > + * Management Registers specifications, this function can be used to retrieve > + * the current PLCA status information from the standard registers in MMD 31. > + */ > +int genphy_c45_plca_get_status(struct phy_device *phydev, > + struct phy_plca_status *plca_st) > +{ > + int ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_STATUS); > + if (ret < 0) > + return ret; > + > + plca_st->pst = !!(((u16)ret) & MDIO_OATC14_PLCA_PST); u16 cast not needed. > + return 0; > +} > +EXPORT_SYMBOL_GPL(genphy_c45_plca_get_status); > + > struct phy_driver genphy_c45_driver = { > .phy_id = 0xffffffff, > .phy_id_mask = 0xffffffff, > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 2dfb85c6e596..4548c8e8f6a9 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -811,7 +811,7 @@ struct phy_plca_cfg { > * struct phy_plca_status - Status of the PLCA (Physical Layer Collision > * Avoidance) Reconciliation Sublayer. > * > - * @status: The PLCA status as reported by the PST bit in the PLCA STATUS > + * @pst: The PLCA status as reported by the PST bit in the PLCA STATUS > * register(31.CA03), indicating BEACON activity. > * > * A structure containing status information of the PLCA RS configuration. > @@ -819,7 +819,7 @@ struct phy_plca_cfg { > * what is actually used. > */ > struct phy_plca_status { > - bool status; > + bool pst; > }; Shouldn't this be in patch 1? > > /** > @@ -1745,6 +1745,12 @@ int genphy_c45_loopback(struct phy_device *phydev, bool enable); > int genphy_c45_pma_resume(struct phy_device *phydev); > int genphy_c45_pma_suspend(struct phy_device *phydev); > int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable); > +int genphy_c45_plca_get_cfg(struct phy_device *phydev, > + struct phy_plca_cfg *plca_cfg); > +int genphy_c45_plca_set_cfg(struct phy_device *phydev, > + const struct phy_plca_cfg *plca_cfg); > +int genphy_c45_plca_get_status(struct phy_device *phydev, > + struct phy_plca_status *plca_st); > > /* Generic C45 PHY driver */ > extern struct phy_driver genphy_c45_driver; > diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h > index 75b7257a51e1..a9f166c511c0 100644 > --- a/include/uapi/linux/mdio.h > +++ b/include/uapi/linux/mdio.h > @@ -26,6 +26,7 @@ > #define MDIO_MMD_C22EXT 29 /* Clause 22 extension */ > #define MDIO_MMD_VEND1 30 /* Vendor specific 1 */ > #define MDIO_MMD_VEND2 31 /* Vendor specific 2 */ > +#define MDIO_MMD_OATC14 MDIO_MMD_VEND2 If this is in the vendor 2 register set, I doubt that this is a feature described by IEEE 802.3, since they allocated the entirety of this MMD over to manufacturers to do whatever they please with this space. If this is correct, then these definitions have no place being in this generic header file, since they are likely specific to the vendors PHY. > > /* Generic MDIO registers. */ > #define MDIO_CTRL1 MII_BMCR > @@ -89,6 +90,14 @@ > #define MDIO_PMA_LASI_TXSTAT 0x9004 /* TX_ALARM status */ > #define MDIO_PMA_LASI_STAT 0x9005 /* LASI status */ > > +/* Open Alliance TC14 registers */ > +#define MDIO_OATC14_PLCA_IDVER 0xca00 /* PLCA ID and version */ > +#define MDIO_OATC14_PLCA_CTRL0 0xca01 /* PLCA Control register 0 */ > +#define MDIO_OATC14_PLCA_CTRL1 0xca02 /* PLCA Control register 1 */ > +#define MDIO_OATC14_PLCA_STATUS 0xca03 /* PLCA Status register */ > +#define MDIO_OATC14_PLCA_TOTMR 0xca04 /* PLCA TO Timer register */ > +#define MDIO_OATC14_PLCA_BURST 0xca05 /* PLCA BURST mode register */ > + Same for these. > /* Control register 1. */ > /* Enable extended speed selection */ > #define MDIO_CTRL1_SPEEDSELEXT (BMCR_SPEED1000 | BMCR_SPEED100) > @@ -436,4 +445,26 @@ static inline __u16 mdio_phy_id_c45(int prtad, int devad) > #define MDIO_USXGMII_5000FULL 0x1a00 /* 5000Mbps full-duplex */ > #define MDIO_USXGMII_LINK 0x8000 /* PHY link with copper-side partner */ > > +/* Open Alliance TC14 PLCA IDVER register */ > +#define MDIO_OATC14_PLCA_IDM 0xff00 /* PLCA MAP ID */ > +#define MDIO_OATC14_PLCA_VER 0x00ff /* PLCA MAP version */ > + > +/* Open Alliance TC14 PLCA CTRL0 register */ > +#define MDIO_OATC14_PLCA_EN 0x8000 /* PLCA enable */ > +#define MDIO_OATC14_PLCA_RST 0x4000 /* PLCA reset */ > + > +/* Open Alliance TC14 PLCA CTRL1 register */ > +#define MDIO_OATC14_PLCA_NCNT 0xff00 /* PLCA node count */ > +#define MDIO_OATC14_PLCA_ID 0x00ff /* PLCA local node ID */ > + > +/* Open Alliance TC14 PLCA STATUS register */ > +#define MDIO_OATC14_PLCA_PST 0x8000 /* PLCA status indication */ > + > +/* Open Alliance TC14 PLCA TOTMR register */ > +#define MDIO_OATC14_PLCA_TOT 0x00ff > + > +/* Open Alliance TC14 PLCA BURST register */ > +#define MDIO_OATC14_PLCA_MAXBC 0xff00 > +#define MDIO_OATC14_PLCA_BTMR 0x00ff And these. > + > #endif /* _UAPI__LINUX_MDIO_H__ */ > diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c > index 371d8098225e..ab50d8b48bd6 100644 > --- a/net/ethtool/plca.c > +++ b/net/ethtool/plca.c > @@ -269,7 +269,7 @@ static int plca_get_status_fill_reply(struct sk_buff *skb, > const struct ethnl_reply_data *reply_base) > { > const struct plca_reply_data *data = PLCA_REPDATA(reply_base); > - const u8 status = data->plca_st.status; > + const u8 status = data->plca_st.pst; Shouldn't this be in a different patch? Thanks.
> It also seems odd to have this "positive numbers mean update" thing > when every other ethtool API just programs whatever is passed. It seems > to be a different API theory to the rest of the ethtool established > principle that if one wishes to modify, one calls the _GET followed > by a _SET. This applies throughout this function. Hi Russell That was my idea. This is a netlink only API, which gives some flexibility which the old ioctl interface did not. With netlink attributes, we can pass only the attributes we want to change. The question then is, what is the interface between the netlink code and the PHY driver. How do you express what attributes were in the request? I suggested -1 for not present. We could keep with the old read/modify/write model, but then we are not making use of what netlink actually offers. Andrew
On Sun, Dec 04, 2022 at 05:06:50PM +0000, Russell King (Oracle) wrote: > On Sun, Dec 04, 2022 at 03:32:06AM +0100, Piergiorgio Beruto wrote: > > --- a/include/uapi/linux/mdio.h > > +++ b/include/uapi/linux/mdio.h > > @@ -26,6 +26,7 @@ > > #define MDIO_MMD_C22EXT 29 /* Clause 22 extension */ > > #define MDIO_MMD_VEND1 30 /* Vendor specific 1 */ > > #define MDIO_MMD_VEND2 31 /* Vendor specific 2 */ > > +#define MDIO_MMD_OATC14 MDIO_MMD_VEND2 > > If this is in the vendor 2 register set, I doubt that this is a feature > described by IEEE 802.3, since they allocated the entirety of this MMD > over to manufacturers to do whatever they please with this space. > > If this is correct, then these definitions have no place being in this > generic header file, since they are likely specific to the vendors PHY. Piergiorgio can give you the full details. As i understand it, IEEE 802.3 defines the basic functionality, but did not extend the standard to define the registers. The Open Alliance member got together and added the missing parts, and published an Open Alliance document. Piergiorgio, i suggest you add a header file for these defines, named to reflect that the Open Alliance defined them. And put in a comment, explaining their origin, maybe a link to the standard. I also don't think this needs to be a uapi header, they are not needed outside of the kernel. I also would not use MDIO_MMD_OATC14, but rather MDIO_MMD_VEND2. There is no guarantee they are not being used for other things, and MDIO_MMD_VEND2 gives a gentle warning about this. Andrew
On Sun, Dec 04, 2022 at 07:48:24PM +0100, Andrew Lunn wrote: > On Sun, Dec 04, 2022 at 05:06:50PM +0000, Russell King (Oracle) wrote: > > On Sun, Dec 04, 2022 at 03:32:06AM +0100, Piergiorgio Beruto wrote: > > > --- a/include/uapi/linux/mdio.h > > > +++ b/include/uapi/linux/mdio.h > > > @@ -26,6 +26,7 @@ > > > #define MDIO_MMD_C22EXT 29 /* Clause 22 extension */ > > > #define MDIO_MMD_VEND1 30 /* Vendor specific 1 */ > > > #define MDIO_MMD_VEND2 31 /* Vendor specific 2 */ > > > +#define MDIO_MMD_OATC14 MDIO_MMD_VEND2 > > > > If this is in the vendor 2 register set, I doubt that this is a feature > > described by IEEE 802.3, since they allocated the entirety of this MMD > > over to manufacturers to do whatever they please with this space. > > > > If this is correct, then these definitions have no place being in this > > generic header file, since they are likely specific to the vendors PHY. > > Piergiorgio can give you the full details. > > As i understand it, IEEE 802.3 defines the basic functionality, but > did not extend the standard to define the registers. > > The Open Alliance member got together and added the missing parts, and > published an Open Alliance document. > > Piergiorgio, i suggest you add a header file for these defines, named > to reflect that the Open Alliance defined them. And put in a comment, > explaining their origin, maybe a link to the standard. I also don't > think this needs to be a uapi header, they are not needed outside of > the kernel. > > I also would not use MDIO_MMD_OATC14, but rather MDIO_MMD_VEND2. There > is no guarantee they are not being used for other things, and > MDIO_MMD_VEND2 gives a gentle warning about this. Thanks Andrew for commenting on this one. This is right, in the IEEE 802.3cg group we could not allocate an MMD for the PLCA reconciliation sublayer because of an 'unfriendly' wording in Clause 45 ruling out Reconciliation Sublayers from what can be configured via registers. Clause 45 says you can have registers for the PHY, while it should have said 'Physical Layer" and there is a subtle difference between the two words. PLCA, for example, is part of the Physical Layer but not of the PHY. Since we could not change that wording, we had to define configuration parameters in Clause 30, and let organizations outside the IEEE define memory maps for PHYs that integrate PLCA. The OPEN Alliance SIG (see the reference in the patches) defined registers for the PLCA RS in MMD31, which is in fact vendor-specific from an IEEE perspective, but part of it is now standardized in the OPEN Alliance. So unfortunately we have to live with this somehow. So ok, I can separate these definitions into a different non-UAPI header as Andrew is suggesting. I'll do this in the next patchset. Thanks, Piergiorgio
On Sun, Dec 04, 2022 at 09:09:08PM +0100, Piergiorgio Beruto wrote: > On Sun, Dec 04, 2022 at 07:48:24PM +0100, Andrew Lunn wrote: > > On Sun, Dec 04, 2022 at 05:06:50PM +0000, Russell King (Oracle) wrote: > > > On Sun, Dec 04, 2022 at 03:32:06AM +0100, Piergiorgio Beruto wrote: > > > > --- a/include/uapi/linux/mdio.h > > > > +++ b/include/uapi/linux/mdio.h > > > > @@ -26,6 +26,7 @@ > > > > #define MDIO_MMD_C22EXT 29 /* Clause 22 extension */ > > > > #define MDIO_MMD_VEND1 30 /* Vendor specific 1 */ > > > > #define MDIO_MMD_VEND2 31 /* Vendor specific 2 */ > > > > +#define MDIO_MMD_OATC14 MDIO_MMD_VEND2 > > > > > > If this is in the vendor 2 register set, I doubt that this is a feature > > > described by IEEE 802.3, since they allocated the entirety of this MMD > > > over to manufacturers to do whatever they please with this space. > > > > > > If this is correct, then these definitions have no place being in this > > > generic header file, since they are likely specific to the vendors PHY. > > > > Piergiorgio can give you the full details. > > > > As i understand it, IEEE 802.3 defines the basic functionality, but > > did not extend the standard to define the registers. > > > > The Open Alliance member got together and added the missing parts, and > > published an Open Alliance document. > > > > Piergiorgio, i suggest you add a header file for these defines, named > > to reflect that the Open Alliance defined them. And put in a comment, > > explaining their origin, maybe a link to the standard. I also don't > > think this needs to be a uapi header, they are not needed outside of > > the kernel. > > > > I also would not use MDIO_MMD_OATC14, but rather MDIO_MMD_VEND2. There > > is no guarantee they are not being used for other things, and > > MDIO_MMD_VEND2 gives a gentle warning about this. > Thanks Andrew for commenting on this one. This is right, in the IEEE > 802.3cg group we could not allocate an MMD for the PLCA reconciliation > sublayer because of an 'unfriendly' wording in Clause 45 ruling out > Reconciliation Sublayers from what can be configured via registers. > Clause 45 says you can have registers for the PHY, while it should have > said 'Physical Layer" and there is a subtle difference between the two > words. PLCA, for example, is part of the Physical Layer but not of the > PHY. Since we could not change that wording, we had to define > configuration parameters in Clause 30, and let organizations outside the > IEEE define memory maps for PHYs that integrate PLCA. > > The OPEN Alliance SIG (see the reference in the patches) defined > registers for the PLCA RS in MMD31, which is in fact vendor-specific > from an IEEE perspective, but part of it is now standardized in the OPEN > Alliance. So unfortunately we have to live with this somehow. > > So ok, I can separate these definitions into a different non-UAPI header > as Andrew is suggesting. I'll do this in the next patchset. Sounds like yet another clause 45 mess :(
On Sun, Dec 04, 2022 at 05:06:50PM +0000, Russell King (Oracle) wrote: > On Sun, Dec 04, 2022 at 03:32:06AM +0100, Piergiorgio Beruto wrote: > > + /* HW bug workaround: the default value of the PLCA TO_TIMER should be > > + * 32, where the current version of NCN26000 reports 24. This will be > > + * fixed in future PHY versions. For the time being, we force the right > > + * default here. > > + */ > > + ret = phy_write_mmd(phydev, > > + MDIO_MMD_OATC14, > > + MDIO_OATC14_PLCA_TOTMR, > > + TO_TMR_DEFAULT); > > Better formatting please. > > return phy_write_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_TOTMR, > TO_TMR_DEFAULT); > > is sufficient. No need for "ret" (and there are folk who will create a > cleanup patch to do this, so might as well get it right on submission.) Ok, I will change the formatting. On the use of ret, I did that just because I was planning to add more features to be pre-configured in the future. But for the time being I can do it as you suggest. > > +/** > > + * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers > > + * @phydev: target phy_device struct > > + * @plca_cfg: output structure to store the PLCA configuration > > + * > > + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA > > + * Management Registers specifications, this function can be used to retrieve > > + * the current PLCA configuration from the standard registers in MMD 31. > > + */ > > +int genphy_c45_plca_get_cfg(struct phy_device *phydev, > > + struct phy_plca_cfg *plca_cfg) > > +{ > > + int ret; > > + > > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_IDVER); > > + if (ret < 0) > > + return ret; > > + > > + plca_cfg->version = (u32)ret; > > ->version has type s32, so is signed. Clearly, from the above code, it > can't be negative (since negative integer values are an error.) So why > is ->version declared in patch 1 as signed? The cast here to u32 also > seems strange. > > Also, since the register you're reading can be no more than 16 bits > wide, using s32 seems like a waste. Please, see the discussion I had with Andrew on this topic. > > + > > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_CTRL0); > > + if (ret < 0) > > + return ret; > > + > > + plca_cfg->enabled = !!(((u16)ret) & MDIO_OATC14_PLCA_EN); > > ->enabled has type s16, but it clearly boolean in nature. It could be > a u8 instead. No need for that u16 cast either. I'll remove all the casts. I apologize, but in some environments the coding rules may be different, requiring unnecessary casts for safety/verification reasons. So I still need to adapt my style to match the kernel's. Please, have patience with me... > > + > > + // first of all, disable PLCA if required > > + if (plca_cfg->enabled == 0) { > > + ret = phy_clear_bits_mmd(phydev, > > + MDIO_MMD_OATC14, > > + MDIO_OATC14_PLCA_CTRL0, > > + MDIO_OATC14_PLCA_EN); > > + > > + if (ret < 0) > > + return ret; > > + } > > Does this need to be disabled when making changes? Just wondering > why you handle this disable explicitly early. It is just a way to avoig configuration glitches. If we need to disable PLCA, it is better to do it before changing any other parameter, so that you don't disturb the other nodes on the multi-drop network. > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(genphy_c45_plca_get_status); > > + > > struct phy_driver genphy_c45_driver = { > > .phy_id = 0xffffffff, > > .phy_id_mask = 0xffffffff, > > diff --git a/include/linux/phy.h b/include/linux/phy.h > > index 2dfb85c6e596..4548c8e8f6a9 100644 > > --- a/include/linux/phy.h > > +++ b/include/linux/phy.h > > @@ -811,7 +811,7 @@ struct phy_plca_cfg { > > * struct phy_plca_status - Status of the PLCA (Physical Layer Collision > > * Avoidance) Reconciliation Sublayer. > > * > > - * @status: The PLCA status as reported by the PST bit in the PLCA STATUS > > + * @pst: The PLCA status as reported by the PST bit in the PLCA STATUS > > * register(31.CA03), indicating BEACON activity. > > * > > * A structure containing status information of the PLCA RS configuration. > > @@ -819,7 +819,7 @@ struct phy_plca_cfg { > > * what is actually used. > > */ > > struct phy_plca_status { > > - bool status; > > + bool pst; > > }; > > Shouldn't this be in patch 1? I thought to first promote the changes to the "higher layers" of ethtool/netlink, then create the appropriate interface towards phylib. Are you suggesting to merge patches 1 & 2? > > + > > #endif /* _UAPI__LINUX_MDIO_H__ */ > > diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c > > index 371d8098225e..ab50d8b48bd6 100644 > > --- a/net/ethtool/plca.c > > +++ b/net/ethtool/plca.c > > @@ -269,7 +269,7 @@ static int plca_get_status_fill_reply(struct sk_buff *skb, > > const struct ethnl_reply_data *reply_base) > > { > > const struct plca_reply_data *data = PLCA_REPDATA(reply_base); > > - const u8 status = data->plca_st.status; > > + const u8 status = data->plca_st.pst; > > Shouldn't this be in a different patch? Ah, logically, yes. I thought since it is an aesthetic change to leave it there. But if you like I can try to merge it with patch 2. Thanks, Piergiorgio
On Sun, Dec 04, 2022 at 08:22:40PM +0000, Russell King (Oracle) wrote: > On Sun, Dec 04, 2022 at 09:09:08PM +0100, Piergiorgio Beruto wrote: > > On Sun, Dec 04, 2022 at 07:48:24PM +0100, Andrew Lunn wrote: > > > On Sun, Dec 04, 2022 at 05:06:50PM +0000, Russell King (Oracle) wrote: > > > > On Sun, Dec 04, 2022 at 03:32:06AM +0100, Piergiorgio Beruto wrote: > > > > > --- a/include/uapi/linux/mdio.h > > > > > +++ b/include/uapi/linux/mdio.h > > > > > @@ -26,6 +26,7 @@ > > > > > #define MDIO_MMD_C22EXT 29 /* Clause 22 extension */ > > > > > #define MDIO_MMD_VEND1 30 /* Vendor specific 1 */ > > > > > #define MDIO_MMD_VEND2 31 /* Vendor specific 2 */ > > > > > +#define MDIO_MMD_OATC14 MDIO_MMD_VEND2 > > > > > > > > If this is in the vendor 2 register set, I doubt that this is a feature > > > > described by IEEE 802.3, since they allocated the entirety of this MMD > > > > over to manufacturers to do whatever they please with this space. > > > > > > > > If this is correct, then these definitions have no place being in this > > > > generic header file, since they are likely specific to the vendors PHY. > > > > > > Piergiorgio can give you the full details. > > > > > > As i understand it, IEEE 802.3 defines the basic functionality, but > > > did not extend the standard to define the registers. > > > > > > The Open Alliance member got together and added the missing parts, and > > > published an Open Alliance document. > > > > > > Piergiorgio, i suggest you add a header file for these defines, named > > > to reflect that the Open Alliance defined them. And put in a comment, > > > explaining their origin, maybe a link to the standard. I also don't > > > think this needs to be a uapi header, they are not needed outside of > > > the kernel. > > > > > > I also would not use MDIO_MMD_OATC14, but rather MDIO_MMD_VEND2. There > > > is no guarantee they are not being used for other things, and > > > MDIO_MMD_VEND2 gives a gentle warning about this. > > Thanks Andrew for commenting on this one. This is right, in the IEEE > > 802.3cg group we could not allocate an MMD for the PLCA reconciliation > > sublayer because of an 'unfriendly' wording in Clause 45 ruling out > > Reconciliation Sublayers from what can be configured via registers. > > Clause 45 says you can have registers for the PHY, while it should have > > said 'Physical Layer" and there is a subtle difference between the two > > words. PLCA, for example, is part of the Physical Layer but not of the > > PHY. Since we could not change that wording, we had to define > > configuration parameters in Clause 30, and let organizations outside the > > IEEE define memory maps for PHYs that integrate PLCA. > > > > The OPEN Alliance SIG (see the reference in the patches) defined > > registers for the PLCA RS in MMD31, which is in fact vendor-specific > > from an IEEE perspective, but part of it is now standardized in the OPEN > > Alliance. So unfortunately we have to live with this somehow. > > > > So ok, I can separate these definitions into a different non-UAPI header > > as Andrew is suggesting. I'll do this in the next patchset. > > Sounds like yet another clause 45 mess :( I'm really sorry for this, I can assure you I personally pushed very hard to get this through, but eventually we got an hard stop. The IEEE has very strict formal rules too, and we were not allowed to change that portion of the specs. To get permission we should have delayed the standard by one year, and that would have upset many people from different industries... Thanks for your understanding. Piergiorgio
diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c index 65a34edc5b20..39197823cf16 100644 --- a/drivers/net/phy/ncn26000.c +++ b/drivers/net/phy/ncn26000.c @@ -27,14 +27,27 @@ #define NCN26000_IRQ_PLCAREC_BIT ((u16)(1 << 4)) #define NCN26000_IRQ_PHYSCOL_BIT ((u16)(1 << 5)) +#define TO_TMR_DEFAULT ((u16)32) + struct ncn26000_priv { u16 enabled_irqs; }; static int ncn26000_config_init(struct phy_device *phydev) { - // TODO: add vendor-specific tuning (ENI, CMC, ...) - return 0; + int ret; + + /* HW bug workaround: the default value of the PLCA TO_TIMER should be + * 32, where the current version of NCN26000 reports 24. This will be + * fixed in future PHY versions. For the time being, we force the right + * default here. + */ + ret = phy_write_mmd(phydev, + MDIO_MMD_OATC14, + MDIO_OATC14_PLCA_TOTMR, + TO_TMR_DEFAULT); + + return ret; } static int ncn26000_enable(struct phy_device *phydev) @@ -177,6 +190,9 @@ static struct phy_driver ncn26000_driver[] = { .config_intr = ncn26000_config_intr, .config_aneg = ncn26000_enable, .handle_interrupt = ncn26000_handle_interrupt, + .get_plca_cfg = genphy_c45_plca_get_cfg, + .set_plca_cfg = genphy_c45_plca_set_cfg, + .get_plca_status = genphy_c45_plca_get_status, }, }; diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index a87a4b3ffce4..9ed3b6858103 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -931,6 +931,193 @@ int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable) } EXPORT_SYMBOL_GPL(genphy_c45_fast_retrain); +/** + * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers + * @phydev: target phy_device struct + * @plca_cfg: output structure to store the PLCA configuration + * + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA + * Management Registers specifications, this function can be used to retrieve + * the current PLCA configuration from the standard registers in MMD 31. + */ +int genphy_c45_plca_get_cfg(struct phy_device *phydev, + struct phy_plca_cfg *plca_cfg) +{ + int ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_IDVER); + if (ret < 0) + return ret; + + plca_cfg->version = (u32)ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_CTRL0); + if (ret < 0) + return ret; + + plca_cfg->enabled = !!(((u16)ret) & MDIO_OATC14_PLCA_EN); + + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_CTRL1); + if (ret < 0) + return ret; + + plca_cfg->node_cnt = (((u16)ret) & MDIO_OATC14_PLCA_NCNT) >> 8; + plca_cfg->node_id = (((u16)ret) & MDIO_OATC14_PLCA_ID); + + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_TOTMR); + if (ret < 0) + return ret; + + plca_cfg->to_tmr = (u16)ret & MDIO_OATC14_PLCA_TOT; + + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_BURST); + if (ret < 0) + return ret; + + plca_cfg->burst_cnt = (((u16)ret) & MDIO_OATC14_PLCA_MAXBC) >> 8; + plca_cfg->burst_tmr = (((u16)ret) & MDIO_OATC14_PLCA_BTMR); + + return 0; +} +EXPORT_SYMBOL_GPL(genphy_c45_plca_get_cfg); + +/** + * genphy_c45_plca_set_cfg - set PLCA configuration using standard registers + * @phydev: target phy_device struct + * @plca_cfg: structure containing the PLCA configuration. Fields set to -1 are + * not to be changed. + * + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA + * Management Registers specifications, this function can be used to modify + * the PLCA configuration using the standard registers in MMD 31. + */ +int genphy_c45_plca_set_cfg(struct phy_device *phydev, + const struct phy_plca_cfg *plca_cfg) +{ + int ret; + u16 val; + + // PLCA IDVER is read-only + if (plca_cfg->version >= 0) + return -EINVAL; + + // first of all, disable PLCA if required + if (plca_cfg->enabled == 0) { + ret = phy_clear_bits_mmd(phydev, + MDIO_MMD_OATC14, + MDIO_OATC14_PLCA_CTRL0, + MDIO_OATC14_PLCA_EN); + + if (ret < 0) + return ret; + } + + if (plca_cfg->node_cnt >= 0 || plca_cfg->node_id >= 0) { + if (plca_cfg->node_cnt < 0 || plca_cfg->node_id < 0) { + ret = phy_read_mmd(phydev, + MDIO_MMD_OATC14, + MDIO_OATC14_PLCA_CTRL1); + + if (ret < 0) + return ret; + + val = (u16)ret; + } + + if (plca_cfg->node_cnt >= 0) + val = (val & ~MDIO_OATC14_PLCA_NCNT) | + (u16)(plca_cfg->node_cnt << 8); + + if (plca_cfg->node_id >= 0) + val = (val & ~MDIO_OATC14_PLCA_ID) | + (u16)(plca_cfg->node_id); + + ret = phy_write_mmd(phydev, + MDIO_MMD_OATC14, + MDIO_OATC14_PLCA_CTRL1, + val); + + if (ret < 0) + return ret; + } + + if (plca_cfg->to_tmr >= 0) { + ret = phy_write_mmd(phydev, + MDIO_MMD_OATC14, + MDIO_OATC14_PLCA_TOTMR, + (u16)plca_cfg->to_tmr); + + if (ret < 0) + return ret; + } + + if (plca_cfg->burst_cnt >= 0 || plca_cfg->burst_tmr >= 0) { + if (plca_cfg->burst_cnt < 0 || plca_cfg->burst_tmr < 0) { + ret = phy_read_mmd(phydev, + MDIO_MMD_OATC14, + MDIO_OATC14_PLCA_BURST); + + if (ret < 0) + return ret; + + val = (u16)ret; + } + + if (plca_cfg->burst_cnt >= 0) + val = (val & ~MDIO_OATC14_PLCA_MAXBC) | + (u16)(plca_cfg->burst_cnt << 8); + + if (plca_cfg->burst_tmr >= 0) + val = (val & ~MDIO_OATC14_PLCA_BTMR) | + (u16)(plca_cfg->burst_tmr); + + ret = phy_write_mmd(phydev, + MDIO_MMD_OATC14, + MDIO_OATC14_PLCA_BURST, + val); + + if (ret < 0) + return ret; + } + + // if we need to enable PLCA, do it at the end + if (plca_cfg->enabled > 0) { + ret = phy_set_bits_mmd(phydev, + MDIO_MMD_OATC14, + MDIO_OATC14_PLCA_CTRL0, + MDIO_OATC14_PLCA_EN); + + if (ret < 0) + return ret; + } + + return 0; +} +EXPORT_SYMBOL_GPL(genphy_c45_plca_set_cfg); + +/** + * genphy_c45_plca_get_status - get PLCA status from standard registers + * @phydev: target phy_device struct + * @plca_st: output structure to store the PLCA status + * + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA + * Management Registers specifications, this function can be used to retrieve + * the current PLCA status information from the standard registers in MMD 31. + */ +int genphy_c45_plca_get_status(struct phy_device *phydev, + struct phy_plca_status *plca_st) +{ + int ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_STATUS); + if (ret < 0) + return ret; + + plca_st->pst = !!(((u16)ret) & MDIO_OATC14_PLCA_PST); + return 0; +} +EXPORT_SYMBOL_GPL(genphy_c45_plca_get_status); + struct phy_driver genphy_c45_driver = { .phy_id = 0xffffffff, .phy_id_mask = 0xffffffff, diff --git a/include/linux/phy.h b/include/linux/phy.h index 2dfb85c6e596..4548c8e8f6a9 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -811,7 +811,7 @@ struct phy_plca_cfg { * struct phy_plca_status - Status of the PLCA (Physical Layer Collision * Avoidance) Reconciliation Sublayer. * - * @status: The PLCA status as reported by the PST bit in the PLCA STATUS + * @pst: The PLCA status as reported by the PST bit in the PLCA STATUS * register(31.CA03), indicating BEACON activity. * * A structure containing status information of the PLCA RS configuration. @@ -819,7 +819,7 @@ struct phy_plca_cfg { * what is actually used. */ struct phy_plca_status { - bool status; + bool pst; }; /** @@ -1745,6 +1745,12 @@ int genphy_c45_loopback(struct phy_device *phydev, bool enable); int genphy_c45_pma_resume(struct phy_device *phydev); int genphy_c45_pma_suspend(struct phy_device *phydev); int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable); +int genphy_c45_plca_get_cfg(struct phy_device *phydev, + struct phy_plca_cfg *plca_cfg); +int genphy_c45_plca_set_cfg(struct phy_device *phydev, + const struct phy_plca_cfg *plca_cfg); +int genphy_c45_plca_get_status(struct phy_device *phydev, + struct phy_plca_status *plca_st); /* Generic C45 PHY driver */ extern struct phy_driver genphy_c45_driver; diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h index 75b7257a51e1..a9f166c511c0 100644 --- a/include/uapi/linux/mdio.h +++ b/include/uapi/linux/mdio.h @@ -26,6 +26,7 @@ #define MDIO_MMD_C22EXT 29 /* Clause 22 extension */ #define MDIO_MMD_VEND1 30 /* Vendor specific 1 */ #define MDIO_MMD_VEND2 31 /* Vendor specific 2 */ +#define MDIO_MMD_OATC14 MDIO_MMD_VEND2 /* Generic MDIO registers. */ #define MDIO_CTRL1 MII_BMCR @@ -89,6 +90,14 @@ #define MDIO_PMA_LASI_TXSTAT 0x9004 /* TX_ALARM status */ #define MDIO_PMA_LASI_STAT 0x9005 /* LASI status */ +/* Open Alliance TC14 registers */ +#define MDIO_OATC14_PLCA_IDVER 0xca00 /* PLCA ID and version */ +#define MDIO_OATC14_PLCA_CTRL0 0xca01 /* PLCA Control register 0 */ +#define MDIO_OATC14_PLCA_CTRL1 0xca02 /* PLCA Control register 1 */ +#define MDIO_OATC14_PLCA_STATUS 0xca03 /* PLCA Status register */ +#define MDIO_OATC14_PLCA_TOTMR 0xca04 /* PLCA TO Timer register */ +#define MDIO_OATC14_PLCA_BURST 0xca05 /* PLCA BURST mode register */ + /* Control register 1. */ /* Enable extended speed selection */ #define MDIO_CTRL1_SPEEDSELEXT (BMCR_SPEED1000 | BMCR_SPEED100) @@ -436,4 +445,26 @@ static inline __u16 mdio_phy_id_c45(int prtad, int devad) #define MDIO_USXGMII_5000FULL 0x1a00 /* 5000Mbps full-duplex */ #define MDIO_USXGMII_LINK 0x8000 /* PHY link with copper-side partner */ +/* Open Alliance TC14 PLCA IDVER register */ +#define MDIO_OATC14_PLCA_IDM 0xff00 /* PLCA MAP ID */ +#define MDIO_OATC14_PLCA_VER 0x00ff /* PLCA MAP version */ + +/* Open Alliance TC14 PLCA CTRL0 register */ +#define MDIO_OATC14_PLCA_EN 0x8000 /* PLCA enable */ +#define MDIO_OATC14_PLCA_RST 0x4000 /* PLCA reset */ + +/* Open Alliance TC14 PLCA CTRL1 register */ +#define MDIO_OATC14_PLCA_NCNT 0xff00 /* PLCA node count */ +#define MDIO_OATC14_PLCA_ID 0x00ff /* PLCA local node ID */ + +/* Open Alliance TC14 PLCA STATUS register */ +#define MDIO_OATC14_PLCA_PST 0x8000 /* PLCA status indication */ + +/* Open Alliance TC14 PLCA TOTMR register */ +#define MDIO_OATC14_PLCA_TOT 0x00ff + +/* Open Alliance TC14 PLCA BURST register */ +#define MDIO_OATC14_PLCA_MAXBC 0xff00 +#define MDIO_OATC14_PLCA_BTMR 0x00ff + #endif /* _UAPI__LINUX_MDIO_H__ */ diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c index 371d8098225e..ab50d8b48bd6 100644 --- a/net/ethtool/plca.c +++ b/net/ethtool/plca.c @@ -269,7 +269,7 @@ static int plca_get_status_fill_reply(struct sk_buff *skb, const struct ethnl_reply_data *reply_base) { const struct plca_reply_data *data = PLCA_REPDATA(reply_base); - const u8 status = data->plca_st.status; + const u8 status = data->plca_st.pst; if (nla_put_u8(skb, ETHTOOL_A_PLCA_STATUS, !!status)) return -EMSGSIZE;
This patch adds PLCA support to the ncn26000 driver. Also add helper functions to read/write standard OPEN Alliance PLCA registers to phylib (genphy_c45_plca_get_cfg, genphy_c45_plca_set_cfg, genphy_c45_plca_get_status). Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com> --- drivers/net/phy/ncn26000.c | 20 +++- drivers/net/phy/phy-c45.c | 187 +++++++++++++++++++++++++++++++++++++ include/linux/phy.h | 10 +- include/uapi/linux/mdio.h | 31 ++++++ net/ethtool/plca.c | 2 +- 5 files changed, 245 insertions(+), 5 deletions(-)