[PATCH net-next 1/4] net/ethtool: Add netlink interface for the PLCA RS

[PATCH net-next 1/4] net/ethtool: Add netlink interface for the PLCA RS

[Piergiorgio Beruto]

Add support for configuring the PLCA Reconciliation Sublayer on
multi-drop PHYs that support IEEE802.3cg-2019 Clause 148 (e.g.,
10BASE-T1S). This patch adds the appropriate netlink interface
to ethtool.

Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
---
 MAINTAINERS                          |   6 +
 drivers/net/phy/phy.c                |  34 ++++
 drivers/net/phy/phy_device.c         |   3 +
 include/linux/ethtool.h              |  11 +
 include/linux/phy.h                  |  64 ++++++
 include/uapi/linux/ethtool_netlink.h |  25 +++
 net/ethtool/Makefile                 |   2 +-
 net/ethtool/netlink.c                |  30 +++
 net/ethtool/netlink.h                |   6 +
 net/ethtool/plca.c                   | 290 +++++++++++++++++++++++++++
 10 files changed, 470 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/plca.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 955c1be1efb2..7952243e4b43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16396,6 +16396,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/chemical/plantower,pms7003.yaml
 F:	drivers/iio/chemical/pms7003.c
 
+PLCA RECONCILIATION SUBLAYER (IEEE802.3 Clause 148)
+M:	Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	net/ethtool/plca.c
+
 PLDMFW LIBRARY
 M:	Jacob Keller <jacob.e.keller@intel.com>
 S:	Maintained
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e5b6cb1a77f9..99e3497b6aa1 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -543,6 +543,40 @@ int phy_ethtool_get_stats(struct phy_device *phydev,
 }
 EXPORT_SYMBOL(phy_ethtool_get_stats);
 
+/**
+ *
+ */
+int phy_ethtool_get_plca_cfg(struct phy_device *dev,
+			     struct phy_plca_cfg *plca_cfg)
+{
+	// TODO
+	return 0;
+}
+EXPORT_SYMBOL(phy_ethtool_get_plca_cfg);
+
+/**
+ *
+ */
+int phy_ethtool_set_plca_cfg(struct phy_device *dev,
+			     struct netlink_ext_ack *extack,
+			     const struct phy_plca_cfg *plca_cfg)
+{
+	// TODO
+	return 0;
+}
+EXPORT_SYMBOL(phy_ethtool_set_plca_cfg);
+
+/**
+ *
+ */
+int phy_ethtool_get_plca_status(struct phy_device *dev,
+				struct phy_plca_status *plca_st)
+{
+	// TODO
+	return 0;
+}
+EXPORT_SYMBOL(phy_ethtool_get_plca_status);
+
 /**
  * phy_start_cable_test - Start a cable test
  *
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 716870a4499c..f248010c403d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3262,6 +3262,9 @@ static const struct ethtool_phy_ops phy_ethtool_phy_ops = {
 	.get_sset_count		= phy_ethtool_get_sset_count,
 	.get_strings		= phy_ethtool_get_strings,
 	.get_stats		= phy_ethtool_get_stats,
+	.get_plca_cfg		= phy_ethtool_get_plca_cfg,
+	.set_plca_cfg		= phy_ethtool_set_plca_cfg,
+	.get_plca_status	= phy_ethtool_get_plca_status,
 	.start_cable_test	= phy_start_cable_test,
 	.start_cable_test_tdr	= phy_start_cable_test_tdr,
 };
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9e0a76fc7de9..4bfe95ec1f0a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -802,12 +802,16 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
 
 struct phy_device;
 struct phy_tdr_config;
+struct phy_plca_cfg;
+struct phy_plca_status;
 
 /**
  * struct ethtool_phy_ops - Optional PHY device options
  * @get_sset_count: Get number of strings that @get_strings will write.
  * @get_strings: Return a set of strings that describe the requested objects
  * @get_stats: Return extended statistics about the PHY device.
+ * @get_plca_cfg: Return PLCA configuration.
+ * @set_plca_cfg: Set PLCA configuration.
  * @start_cable_test: Start a cable test
  * @start_cable_test_tdr: Start a Time Domain Reflectometry cable test
  *
@@ -819,6 +823,13 @@ struct ethtool_phy_ops {
 	int (*get_strings)(struct phy_device *dev, u8 *data);
 	int (*get_stats)(struct phy_device *dev,
 			 struct ethtool_stats *stats, u64 *data);
+	int (*get_plca_cfg)(struct phy_device *dev,
+			    struct phy_plca_cfg *plca_cfg);
+	int (*set_plca_cfg)(struct phy_device *dev,
+			    struct netlink_ext_ack *extack,
+			    const struct phy_plca_cfg *plca_cfg);
+	int (*get_plca_status)(struct phy_device *dev,
+			       struct phy_plca_status *plca_st);
 	int (*start_cable_test)(struct phy_device *phydev,
 				struct netlink_ext_ack *extack);
 	int (*start_cable_test_tdr)(struct phy_device *phydev,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 71eeb4e3b1fd..55160db311fa 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -765,6 +765,63 @@ struct phy_tdr_config {
 };
 #define PHY_PAIR_ALL -1
 
+/**
+ * struct phy_plca_cfg - Configuration of the PLCA (Physical Layer Collision
+ * Avoidance) Reconciliation Sublayer.
+ *
+ * @version: read-only PLCA register map version. 0 = not available. Ignored
+ *   when setting the configuration. Format is the same as reported by the PLCA
+ *   IDVER register (31.CA00). -1 = not available.
+ * @enabled: PLCA configured mode (enabled/disabled). -1 = not available / don't
+ *   set. 0 = disabled, anything else = enabled.
+ * @node_id: the PLCA local node identifier. -1 = not available / don't set.
+ *   Allowed values [0 .. 254]. 255 = node disabled.
+ * @node_cnt: the PLCA node count (maximum number of nodes having a TO). Only
+ *   meaningful for the coordinator (node_id = 0). -1 = not available / don't
+ *   set. Allowed values [0 .. 255].
+ * @to_tmr: The value of the PLCA to_timer in bit-times, which determines the
+ *   PLCA transmit opportunity window opening. See IEEE802.3 Clause 148 for
+ *   more details. The to_timer shall be set equal over all nodes.
+ *   -1 = not available / don't set. Allowed values [0 .. 255].
+ * @burst_cnt: controls how many additional frames a node is allowed to send in
+ *   single transmit opportunity (TO). The default value of 0 means that the
+ *   node is allowed exactly one frame per TO. A value of 1 allows two frames
+ *   per TO, and so on. -1 = not available / don't set.
+ *   Allowed values [0 .. 255].
+ * @burst_tmr: controls how many bit times to wait for the MAC to send a new
+ *   frame before interrupting the burst. This value should be set to a value
+ *   greater than the MAC inter-packet gap (which is typically 96 bits).
+ *   -1 = not available / don't set. Allowed values [0 .. 255].
+ *
+ * A structure containing configuration parameters for setting/getting the PLCA
+ * RS configuration. The driver does not need to implement all the parameters,
+ * but should report what is actually used.
+ */
+struct phy_plca_cfg {
+	s32 version;
+	s16 enabled;
+	s16 node_id;
+	s16 node_cnt;
+	s16 to_tmr;
+	s16 burst_cnt;
+	s16 burst_tmr;
+};
+
+/**
+ * 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
+ *	register(31.CA03), indicating BEACON activity.
+ *
+ * A structure containing status information of the PLCA RS configuration.
+ * The driver does not need to implement all the parameters, but should report
+ * what is actually used.
+ */
+struct phy_plca_status {
+	bool status;
+};
+
 /**
  * struct phy_driver - Driver structure for a particular PHY type
  *
@@ -1775,6 +1832,13 @@ int phy_ethtool_get_strings(struct phy_device *phydev, u8 *data);
 int phy_ethtool_get_sset_count(struct phy_device *phydev);
 int phy_ethtool_get_stats(struct phy_device *phydev,
 			  struct ethtool_stats *stats, u64 *data);
+int phy_ethtool_get_plca_cfg(struct phy_device *dev,
+			     struct phy_plca_cfg *plca_cfg);
+int phy_ethtool_set_plca_cfg(struct phy_device *dev,
+			     struct netlink_ext_ack *extack,
+			     const struct phy_plca_cfg *plca_cfg);
+int phy_ethtool_get_plca_status(struct phy_device *dev,
+				struct phy_plca_status *plca_st);
 
 static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
 {
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index aaf7c6963d61..81e3d7b42d0f 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -51,6 +51,9 @@ enum {
 	ETHTOOL_MSG_MODULE_SET,
 	ETHTOOL_MSG_PSE_GET,
 	ETHTOOL_MSG_PSE_SET,
+	ETHTOOL_MSG_PLCA_GET_CFG,
+	ETHTOOL_MSG_PLCA_SET_CFG,
+	ETHTOOL_MSG_PLCA_GET_STATUS,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -97,6 +100,9 @@ enum {
 	ETHTOOL_MSG_MODULE_GET_REPLY,
 	ETHTOOL_MSG_MODULE_NTF,
 	ETHTOOL_MSG_PSE_GET_REPLY,
+	ETHTOOL_MSG_PLCA_GET_CFG_REPLY,
+	ETHTOOL_MSG_PLCA_GET_STATUS_REPLY,
+	ETHTOOL_MSG_PLCA_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -880,6 +886,25 @@ enum {
 	ETHTOOL_A_PSE_MAX = (__ETHTOOL_A_PSE_CNT - 1)
 };
 
+/* PLCA */
+
+enum {
+	ETHTOOL_A_PLCA_UNSPEC,
+	ETHTOOL_A_PLCA_HEADER,				/* nest - _A_HEADER_* */
+	ETHTOOL_A_PLCA_VERSION,				/* u16 */
+	ETHTOOL_A_PLCA_ENABLED,				/* u8 */
+	ETHTOOL_A_PLCA_STATUS,				/* u8 */
+	ETHTOOL_A_PLCA_NODE_CNT,			/* u8 */
+	ETHTOOL_A_PLCA_NODE_ID,				/* u8 */
+	ETHTOOL_A_PLCA_TO_TMR,				/* u8 */
+	ETHTOOL_A_PLCA_BURST_CNT,			/* u8 */
+	ETHTOOL_A_PLCA_BURST_TMR,			/* u8 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_PLCA_CNT,
+	ETHTOOL_A_PLCA_MAX = (__ETHTOOL_A_PLCA_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 72ab0944262a..b18930e2ce9a 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
 		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o module.o \
-		   pse-pd.o
+		   pse-pd.o plca.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 1a4c11356c96..eb044f48cb24 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -287,6 +287,8 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PHC_VCLOCKS_GET]	= &ethnl_phc_vclocks_request_ops,
 	[ETHTOOL_MSG_MODULE_GET]	= &ethnl_module_request_ops,
 	[ETHTOOL_MSG_PSE_GET]		= &ethnl_pse_request_ops,
+	[ETHTOOL_MSG_PLCA_GET_CFG]	= &ethnl_plca_cfg_request_ops,
+	[ETHTOOL_MSG_PLCA_GET_STATUS]	= &ethnl_plca_status_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -602,6 +604,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_EEE_NTF]		= &ethnl_eee_request_ops,
 	[ETHTOOL_MSG_FEC_NTF]		= &ethnl_fec_request_ops,
 	[ETHTOOL_MSG_MODULE_NTF]	= &ethnl_module_request_ops,
+	[ETHTOOL_MSG_PLCA_NTF]		= &ethnl_plca_cfg_request_ops,
 };
 
 /* default notification handler */
@@ -695,6 +698,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_EEE_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_FEC_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_MODULE_NTF]	= ethnl_default_notify,
+	[ETHTOOL_MSG_PLCA_NTF]		= ethnl_default_notify,
 };
 
 void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
@@ -1040,6 +1044,32 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_pse_set_policy,
 		.maxattr = ARRAY_SIZE(ethnl_pse_set_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PLCA_GET_CFG,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_plca_get_cfg_policy,
+		.maxattr = ARRAY_SIZE(ethnl_plca_get_cfg_policy) - 1,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_PLCA_SET_CFG,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_set_plca_cfg,
+		.policy = ethnl_plca_set_cfg_policy,
+		.maxattr = ARRAY_SIZE(ethnl_plca_set_cfg_policy) - 1,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_PLCA_GET_STATUS,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_plca_get_status_policy,
+		.maxattr = ARRAY_SIZE(ethnl_plca_get_status_policy) - 1,
+	},
+
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 1bfd374f9718..c0ed1a6d0833 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -346,6 +346,8 @@ extern const struct ethnl_request_ops ethnl_stats_request_ops;
 extern const struct ethnl_request_ops ethnl_phc_vclocks_request_ops;
 extern const struct ethnl_request_ops ethnl_module_request_ops;
 extern const struct ethnl_request_ops ethnl_pse_request_ops;
+extern const struct ethnl_request_ops ethnl_plca_cfg_request_ops;
+extern const struct ethnl_request_ops ethnl_plca_status_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -386,6 +388,9 @@ extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER +
 extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1];
 extern const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1];
 extern const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1];
+extern const struct nla_policy ethnl_plca_get_cfg_policy[ETHTOOL_A_PLCA_HEADER + 1];
+extern const struct nla_policy ethnl_plca_set_cfg_policy[ETHTOOL_A_PLCA_MAX + 1];
+extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADER + 1];
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
@@ -406,6 +411,7 @@ int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_module(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_pse(struct sk_buff *skb, struct genl_info *info);
+int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
new file mode 100644
index 000000000000..371d8098225e
--- /dev/null
+++ b/net/ethtool/plca.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/phy.h>
+#include <linux/ethtool_netlink.h>
+
+#include "netlink.h"
+#include "common.h"
+
+struct plca_req_info {
+	struct ethnl_req_info		base;
+};
+
+struct plca_reply_data {
+	struct ethnl_reply_data		base;
+	struct phy_plca_cfg		plca_cfg;
+	struct phy_plca_status		plca_st;
+};
+
+#define PLCA_REPDATA(__reply_base) \
+	container_of(__reply_base, struct plca_reply_data, base)
+
+// PLCA get configuration message ------------------------------------------- //
+
+const struct nla_policy ethnl_plca_get_cfg_policy[] = {
+	[ETHTOOL_A_PLCA_HEADER]		=
+		NLA_POLICY_NESTED(ethnl_header_policy),
+};
+
+static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
+				     struct ethnl_reply_data *reply_base,
+				     struct genl_info *info)
+{
+	struct plca_reply_data *data = PLCA_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	const struct ethtool_phy_ops *ops;
+	int ret;
+
+	// check that the PHY device is available and connected
+	if (!dev->phydev) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	// note: rtnl_lock is held already by ethnl_default_doit
+	ops = ethtool_phy_ops;
+	if (!ops || !ops->get_plca_cfg) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out;
+
+	ret = ops->get_plca_cfg(dev->phydev, &data->plca_cfg);
+	if (ret < 0)
+		goto out;
+
+	ethnl_ops_complete(dev);
+
+out:
+	return ret;
+}
+
+static int plca_get_cfg_reply_size(const struct ethnl_req_info *req_base,
+				   const struct ethnl_reply_data *reply_base)
+{
+	return nla_total_size(sizeof(u16)) +	/* _VERSION */
+	       nla_total_size(sizeof(u8)) +	/* _ENABLED */
+	       nla_total_size(sizeof(u8)) +	/* _STATUS  */
+	       nla_total_size(sizeof(u8)) +	/* _NODE_CNT */
+	       nla_total_size(sizeof(u8)) +	/* _NODE_ID */
+	       nla_total_size(sizeof(u8)) +	/* _TO_TIMER */
+	       nla_total_size(sizeof(u8)) +	/* _BURST_COUNT */
+	       nla_total_size(sizeof(u8));	/* _BURST_TIMER */
+}
+
+static int plca_get_cfg_fill_reply(struct sk_buff *skb,
+				   const struct ethnl_req_info *req_base,
+				   const struct ethnl_reply_data *reply_base)
+{
+	const struct plca_reply_data *data = PLCA_REPDATA(reply_base);
+	const struct phy_plca_cfg *plca = &data->plca_cfg;
+
+	if ((plca->version >= 0 &&
+	     nla_put_u16(skb, ETHTOOL_A_PLCA_VERSION, (u16)plca->version)) ||
+	    (plca->enabled >= 0 &&
+	     nla_put_u8(skb, ETHTOOL_A_PLCA_ENABLED, !!plca->enabled)) ||
+	    (plca->node_id >= 0 &&
+	     nla_put_u8(skb, ETHTOOL_A_PLCA_NODE_ID, (u8)plca->node_id)) ||
+	    (plca->node_cnt >= 0 &&
+	     nla_put_u8(skb, ETHTOOL_A_PLCA_NODE_CNT, (u8)plca->node_cnt)) ||
+	    (plca->to_tmr >= 0 &&
+	     nla_put_u8(skb, ETHTOOL_A_PLCA_TO_TMR, (u8)plca->to_tmr)) ||
+	    (plca->burst_cnt >= 0 &&
+	     nla_put_u8(skb, ETHTOOL_A_PLCA_BURST_CNT, (u8)plca->burst_cnt)) ||
+	    (plca->burst_tmr >= 0 &&
+	     nla_put_u8(skb, ETHTOOL_A_PLCA_BURST_TMR, (u8)plca->burst_tmr)))
+		return -EMSGSIZE;
+
+	return 0;
+};
+
+const struct ethnl_request_ops ethnl_plca_cfg_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PLCA_GET_CFG,
+	.reply_cmd		= ETHTOOL_MSG_PLCA_GET_CFG_REPLY,
+	.hdr_attr		= ETHTOOL_A_PLCA_HEADER,
+	.req_info_size		= sizeof(struct plca_req_info),
+	.reply_data_size	= sizeof(struct plca_reply_data),
+
+	.prepare_data		= plca_get_cfg_prepare_data,
+	.reply_size		= plca_get_cfg_reply_size,
+	.fill_reply		= plca_get_cfg_fill_reply,
+};
+
+// PLCA set configuration message ------------------------------------------- //
+
+const struct nla_policy ethnl_plca_set_cfg_policy[] = {
+	[ETHTOOL_A_PLCA_HEADER]		=
+		NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_PLCA_ENABLED]	= { .type = NLA_U8 },
+	[ETHTOOL_A_PLCA_NODE_ID]	= { .type = NLA_U8 },
+	[ETHTOOL_A_PLCA_NODE_CNT]	= { .type = NLA_U8 },
+	[ETHTOOL_A_PLCA_TO_TMR]		= { .type = NLA_U8 },
+	[ETHTOOL_A_PLCA_BURST_CNT]	= { .type = NLA_U8 },
+	[ETHTOOL_A_PLCA_BURST_TMR]	= { .type = NLA_U8 },
+};
+
+int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info)
+{
+	struct ethnl_req_info req_info = {};
+	struct nlattr **tb = info->attrs;
+	const struct ethtool_phy_ops *ops;
+	struct phy_plca_cfg plca_cfg;
+	struct net_device *dev;
+
+	bool mod = false;
+	int ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info,
+					 tb[ETHTOOL_A_PLCA_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+
+	dev = req_info.dev;
+
+	// check that the PHY device is available and connected
+	if (!dev->phydev) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	rtnl_lock();
+
+	ops = ethtool_phy_ops;
+	if (!ops || !ops->set_plca_cfg) {
+		ret = -EOPNOTSUPP;
+		goto out_rtnl;
+	}
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	memset(&plca_cfg, 0xFF, sizeof(plca_cfg));
+
+	if (tb[ETHTOOL_A_PLCA_ENABLED]) {
+		plca_cfg.enabled = !!nla_get_u8(tb[ETHTOOL_A_PLCA_ENABLED]);
+		mod = true;
+	}
+
+	if (tb[ETHTOOL_A_PLCA_NODE_ID]) {
+		plca_cfg.node_id = nla_get_u8(tb[ETHTOOL_A_PLCA_NODE_ID]);
+		mod = true;
+	}
+
+	if (tb[ETHTOOL_A_PLCA_NODE_CNT]) {
+		plca_cfg.node_cnt = nla_get_u8(tb[ETHTOOL_A_PLCA_NODE_CNT]);
+		mod = true;
+	}
+
+	if (tb[ETHTOOL_A_PLCA_TO_TMR]) {
+		plca_cfg.to_tmr = nla_get_u8(tb[ETHTOOL_A_PLCA_TO_TMR]);
+		mod = true;
+	}
+
+	if (tb[ETHTOOL_A_PLCA_BURST_CNT]) {
+		plca_cfg.burst_cnt = nla_get_u8(tb[ETHTOOL_A_PLCA_BURST_CNT]);
+		mod = true;
+	}
+
+	if (tb[ETHTOOL_A_PLCA_BURST_TMR]) {
+		plca_cfg.burst_tmr = nla_get_u8(tb[ETHTOOL_A_PLCA_BURST_TMR]);
+		mod = true;
+	}
+
+	ret = 0;
+	if (!mod)
+		goto out_ops;
+
+	ret = ops->set_plca_cfg(dev->phydev, info->extack, &plca_cfg);
+
+	if (ret < 0)
+		goto out_ops;
+
+	ethtool_notify(dev, ETHTOOL_MSG_PLCA_NTF, NULL);
+
+out_ops:
+	ethnl_ops_complete(dev);
+out_rtnl:
+	rtnl_unlock();
+	ethnl_parse_header_dev_put(&req_info);
+out:
+	return ret;
+}
+
+// PLCA get status message -------------------------------------------------- //
+
+const struct nla_policy ethnl_plca_get_status_policy[] = {
+	[ETHTOOL_A_PLCA_HEADER]		=
+		NLA_POLICY_NESTED(ethnl_header_policy),
+};
+
+static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
+					struct ethnl_reply_data *reply_base,
+					struct genl_info *info)
+{
+	struct plca_reply_data *data = PLCA_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	const struct ethtool_phy_ops *ops;
+	int ret;
+
+	// check that the PHY device is available and connected
+	if (!dev->phydev) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	// note: rtnl_lock is held already by ethnl_default_doit
+	ops = ethtool_phy_ops;
+	if (!ops || !ops->get_plca_status) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out;
+
+	ret = ops->get_plca_status(dev->phydev, &data->plca_st);
+	if (ret < 0)
+		goto out;
+
+	ethnl_ops_complete(dev);
+out:
+	return ret;
+}
+
+static int plca_get_status_reply_size(const struct ethnl_req_info *req_base,
+				      const struct ethnl_reply_data *reply_base)
+{
+	return nla_total_size(sizeof(u8));	/* _STATUS */
+}
+
+static int plca_get_status_fill_reply(struct sk_buff *skb,
+				      const struct ethnl_req_info *req_base,
+				      const struct ethnl_reply_data *reply_base)
+{
+	const struct plca_reply_data *data = PLCA_REPDATA(reply_base);
+	const u8 status = data->plca_st.status;
+
+	if (nla_put_u8(skb, ETHTOOL_A_PLCA_STATUS, !!status))
+		return -EMSGSIZE;
+
+	return 0;
+};
+
+const struct ethnl_request_ops ethnl_plca_status_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PLCA_GET_STATUS,
+	.reply_cmd		= ETHTOOL_MSG_PLCA_GET_STATUS_REPLY,
+	.hdr_attr		= ETHTOOL_A_PLCA_HEADER,
+	.req_info_size		= sizeof(struct plca_req_info),
+	.reply_data_size	= sizeof(struct plca_reply_data),
+
+	.prepare_data		= plca_get_status_prepare_data,
+	.reply_size		= plca_get_status_reply_size,
+	.fill_reply		= plca_get_status_fill_reply,
+};
-- 
2.35.1

Re: [PATCH net-next 1/4] net/ethtool: Add netlink interface for the PLCA RS

[Randy Dunlap]

Hi--

On 12/3/22 18:30, Piergiorgio Beruto wrote:
> Add support for configuring the PLCA Reconciliation Sublayer on
> multi-drop PHYs that support IEEE802.3cg-2019 Clause 148 (e.g.,
> 10BASE-T1S). This patch adds the appropriate netlink interface
> to ethtool.
> 
> Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
> ---


> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e5b6cb1a77f9..99e3497b6aa1 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -543,6 +543,40 @@ int phy_ethtool_get_stats(struct phy_device *phydev,
>  }
>  EXPORT_SYMBOL(phy_ethtool_get_stats);
>  

What is the meaning of all these empty kernel-doc comment blocks?
Why are they here?
Please delete them.

> +/**
> + *
> + */
> +int phy_ethtool_get_plca_cfg(struct phy_device *dev,
> +			     struct phy_plca_cfg *plca_cfg)
> +{
> +	// TODO
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_ethtool_get_plca_cfg);
> +
> +/**
> + *
> + */
> +int phy_ethtool_set_plca_cfg(struct phy_device *dev,
> +			     struct netlink_ext_ack *extack,
> +			     const struct phy_plca_cfg *plca_cfg)
> +{
> +	// TODO
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_ethtool_set_plca_cfg);
> +
> +/**
> + *
> + */
> +int phy_ethtool_get_plca_status(struct phy_device *dev,
> +				struct phy_plca_status *plca_st)
> +{
> +	// TODO
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_ethtool_get_plca_status);

Thanks.
-- 
~Randy

Re: [PATCH net-next 1/4] net/ethtool: Add netlink interface for the PLCA RS

[Piergiorgio Beruto]

Hello Randy,
thank you for your feedback. Although I have worked on the kernel for
quite some time now, I'm pretty new to this process.

Please, see my answers interleaved.

On Sat, Dec 03, 2022 at 06:37:13PM -0800, Randy Dunlap wrote:
> Hi--
> 
> On 12/3/22 18:30, Piergiorgio Beruto wrote:
> > Add support for configuring the PLCA Reconciliation Sublayer on
> > multi-drop PHYs that support IEEE802.3cg-2019 Clause 148 (e.g.,
> > 10BASE-T1S). This patch adds the appropriate netlink interface
> > to ethtool.
> > 
> > Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
> > ---
> 
> 
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index e5b6cb1a77f9..99e3497b6aa1 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -543,6 +543,40 @@ int phy_ethtool_get_stats(struct phy_device *phydev,
> >  }
> >  EXPORT_SYMBOL(phy_ethtool_get_stats);
> >  
> 
> What is the meaning of all these empty kernel-doc comment blocks?
> Why are they here?
> Please delete them.
These functions are placeholders that I've used to have the kernel
compile. The next patch amends those functions and adds the proper
kernel-doc.

Do you want me to just remove the kernel-doc and leave the functions
TODO? Or would you like me to merge patches 1 and 2?

I did this to split the work into smaller, logically isolated and
compiling commits. Please, let me know if I did that wrong.
 
> > +/**
> > + *
> > + */
> > +int phy_ethtool_get_plca_cfg(struct phy_device *dev,
> > +			     struct phy_plca_cfg *plca_cfg)
> > +{
> > +	// TODO
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(phy_ethtool_get_plca_cfg);
> > +
> > +/**
> > + *
> > + */
> > +int phy_ethtool_set_plca_cfg(struct phy_device *dev,
> > +			     struct netlink_ext_ack *extack,
> > +			     const struct phy_plca_cfg *plca_cfg)
> > +{
> > +	// TODO
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(phy_ethtool_set_plca_cfg);
> > +
> > +/**
> > + *
> > + */
> > +int phy_ethtool_get_plca_status(struct phy_device *dev,
> > +				struct phy_plca_status *plca_st)
> > +{
> > +	// TODO
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(phy_ethtool_get_plca_status);
> 
> Thanks.
> -- 
> ~Randy
Thank you and kind regards,
   Piergiorgio

Re: [PATCH net-next 1/4] net/ethtool: Add netlink interface for the PLCA RS

[Randy Dunlap]

Hi,

On 12/3/22 18:49, Piergiorgio Beruto wrote:
> Hello Randy,
> thank you for your feedback. Although I have worked on the kernel for
> quite some time now, I'm pretty new to this process.
> 
> Please, see my answers interleaved.
> 
> On Sat, Dec 03, 2022 at 06:37:13PM -0800, Randy Dunlap wrote:
>> Hi--
>>
>> On 12/3/22 18:30, Piergiorgio Beruto wrote:
>>> Add support for configuring the PLCA Reconciliation Sublayer on
>>> multi-drop PHYs that support IEEE802.3cg-2019 Clause 148 (e.g.,
>>> 10BASE-T1S). This patch adds the appropriate netlink interface
>>> to ethtool.
>>>
>>> Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
>>> ---
>>
>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index e5b6cb1a77f9..99e3497b6aa1 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -543,6 +543,40 @@ int phy_ethtool_get_stats(struct phy_device *phydev,
>>>  }
>>>  EXPORT_SYMBOL(phy_ethtool_get_stats);
>>>  
>>
>> What is the meaning of all these empty kernel-doc comment blocks?
>> Why are they here?
>> Please delete them.
> These functions are placeholders that I've used to have the kernel
> compile. The next patch amends those functions and adds the proper
> kernel-doc.
> 
> Do you want me to just remove the kernel-doc and leave the functions
> TODO? Or would you like me to merge patches 1 and 2?

OK, sorry that I missed seeing that.
It seems a bit unusual to me -- IMO.

I would at least remove the empty kernel-doc comment blocks, but
it probably really doesn't matter either way, unless one of the
netdev maintainers wants to see it changed.

Thanks.

> I did this to split the work into smaller, logically isolated and
> compiling commits. Please, let me know if I did that wrong.
>  
>>> +/**
>>> + *
>>> + */
>>> +int phy_ethtool_get_plca_cfg(struct phy_device *dev,
>>> +			     struct phy_plca_cfg *plca_cfg)
>>> +{
>>> +	// TODO
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(phy_ethtool_get_plca_cfg);
>>> +
>>> +/**
>>> + *
>>> + */
>>> +int phy_ethtool_set_plca_cfg(struct phy_device *dev,
>>> +			     struct netlink_ext_ack *extack,
>>> +			     const struct phy_plca_cfg *plca_cfg)
>>> +{
>>> +	// TODO
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(phy_ethtool_set_plca_cfg);
>>> +
>>> +/**
>>> + *
>>> + */
>>> +int phy_ethtool_get_plca_status(struct phy_device *dev,
>>> +				struct phy_plca_status *plca_st)
>>> +{
>>> +	// TODO
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(phy_ethtool_get_plca_status);
>>
>> Thanks.
>> -- 
>> ~Randy
> Thank you and kind regards,
>    Piergiorgio

-- 
~Randy

[PATCH net-next 2/4] phylib: Add support for 10BASE-T1S link modes and PLCA config

[Piergiorgio Beruto]

This patch adds the required connection between netlink ethtool and
phylib to resolve PLCA get/set config and get status messages.
Additionally, it adds the link modes for the IEEE 802.3cg Clause 147
10BASE-T1S Ethernet PHY.

Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
---
 drivers/net/phy/phy-core.c   |   2 +-
 drivers/net/phy/phy.c        | 163 +++++++++++++++++++++++++++++++++--
 drivers/net/phy/phylink.c    |   6 +-
 include/linux/phy.h          |  17 +++-
 include/uapi/linux/ethtool.h |   3 +
 net/ethtool/common.c         |   8 ++
 6 files changed, 185 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 5d08c627a516..5d8085fffffc 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -13,7 +13,7 @@
  */
 const char *phy_speed_to_str(int speed)
 {
-	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 99,
+	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
 		"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
 		"If a speed or mode has been added please update phy_speed_to_str "
 		"and the PHY settings array.\n");
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 99e3497b6aa1..6bea928e405b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -544,36 +544,181 @@ int phy_ethtool_get_stats(struct phy_device *phydev,
 EXPORT_SYMBOL(phy_ethtool_get_stats);
 
 /**
+ * phy_ethtool_get_plca_cfg - Get PLCA RS configuration
  *
+ * @phydev: the phy_device struct
+ * @plca_cfg: where to store the retrieved configuration
  */
-int phy_ethtool_get_plca_cfg(struct phy_device *dev,
+int phy_ethtool_get_plca_cfg(struct phy_device *phydev,
 			     struct phy_plca_cfg *plca_cfg)
 {
-	// TODO
-	return 0;
+	int ret;
+
+	if (!phydev->drv) {
+		ret = -EIO;
+		goto out;
+	}
+
+	if (!phydev->drv->get_plca_cfg) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	memset(plca_cfg, 0xFF, sizeof(*plca_cfg));
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->get_plca_cfg(phydev, plca_cfg);
+
+	if (ret)
+		goto out_drv;
+
+out_drv:
+	mutex_unlock(&phydev->lock);
+out:
+	return ret;
 }
 EXPORT_SYMBOL(phy_ethtool_get_plca_cfg);
 
 /**
+ * phy_ethtool_set_plca_cfg - Set PLCA RS configuration
  *
+ * @phydev: the phy_device struct
+ * @extack: extack for reporting useful error messages
+ * @plca_cfg: new PLCA configuration to apply
  */
-int phy_ethtool_set_plca_cfg(struct phy_device *dev,
+int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
 			     struct netlink_ext_ack *extack,
 			     const struct phy_plca_cfg *plca_cfg)
 {
-	// TODO
-	return 0;
+	int ret;
+	struct phy_plca_cfg *curr_plca_cfg = 0;
+
+	if (!phydev->drv) {
+		ret = -EIO;
+		goto out;
+	}
+
+	if (!phydev->drv->set_plca_cfg ||
+	    !phydev->drv->get_plca_cfg) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	curr_plca_cfg = kmalloc(sizeof(*curr_plca_cfg), GFP_KERNEL);
+	memset(curr_plca_cfg, 0xFF, sizeof(*curr_plca_cfg));
+
+	mutex_lock(&phydev->lock);
+
+	ret = phydev->drv->get_plca_cfg(phydev, curr_plca_cfg);
+	if (ret)
+		goto out_drv;
+
+	if (curr_plca_cfg->enabled < 0 && plca_cfg->enabled >= 0) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY does not support changing the PLCA 'enable' attribute");
+		ret = -EINVAL;
+		goto out_drv;
+	}
+
+	if (curr_plca_cfg->node_id < 0 && plca_cfg->node_id >= 0) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY does not support changing the PLCA 'local node ID' attribute");
+		ret = -EINVAL;
+		goto out_drv;
+	}
+
+	if (curr_plca_cfg->node_cnt < 0 && plca_cfg->node_cnt >= 0) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY does not support changing the PLCA 'node count' attribute");
+		ret = -EINVAL;
+		goto out_drv;
+	}
+
+	if (curr_plca_cfg->to_tmr < 0 && plca_cfg->to_tmr >= 0) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY does not support changing the PLCA 'TO timer' attribute");
+		ret = -EINVAL;
+		goto out_drv;
+	}
+
+	if (curr_plca_cfg->burst_cnt < 0 && plca_cfg->burst_cnt >= 0) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY does not support changing the PLCA 'burst count' attribute");
+		ret = -EINVAL;
+		goto out_drv;
+	}
+
+	if (curr_plca_cfg->burst_tmr < 0 && plca_cfg->burst_tmr >= 0) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY does not support changing the PLCA 'burst timer' attribute");
+		ret = -EINVAL;
+		goto out_drv;
+	}
+
+	// if enabling PLCA, perform additional sanity checks
+	if (plca_cfg->enabled > 0) {
+		if (!linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
+				       phydev->advertising)) {
+			ret = -EOPNOTSUPP;
+			NL_SET_ERR_MSG(extack,
+				       "Point to Multi-Point mode is not enabled");
+		}
+
+		// allow setting node_id concurrently with enabled
+		if (plca_cfg->node_id >= 0)
+			curr_plca_cfg->node_id = plca_cfg->node_id;
+
+		if (curr_plca_cfg->node_id >= 255) {
+			NL_SET_ERR_MSG(extack, "PLCA node ID is not set");
+			ret = -EINVAL;
+			goto out_drv;
+		}
+	}
+
+	ret = phydev->drv->set_plca_cfg(phydev, plca_cfg);
+	if (ret)
+		goto out_drv;
+
+out_drv:
+	kfree(curr_plca_cfg);
+	mutex_unlock(&phydev->lock);
+out:
+	return ret;
+
 }
 EXPORT_SYMBOL(phy_ethtool_set_plca_cfg);
 
 /**
+ * phy_ethtool_get_plca_status - Get PLCA RS status information
  *
+ * @phydev: the phy_device struct
+ * @plca_st: where to store the retrieved status information
  */
-int phy_ethtool_get_plca_status(struct phy_device *dev,
+int phy_ethtool_get_plca_status(struct phy_device *phydev,
 				struct phy_plca_status *plca_st)
 {
-	// TODO
-	return 0;
+	int ret;
+
+	if (!phydev->drv) {
+		ret = -EIO;
+		goto out;
+	}
+
+	if (!phydev->drv->get_plca_status) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->get_plca_status(phydev, plca_st);
+
+	if (ret)
+		goto out_drv;
+
+out_drv:
+	mutex_unlock(&phydev->lock);
+out:
+	return ret;
 }
 EXPORT_SYMBOL(phy_ethtool_get_plca_status);
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 09cc65c0da93..319790221d7f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -241,12 +241,16 @@ void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps)
 	if (caps & MAC_ASYM_PAUSE)
 		__set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, linkmodes);
 
-	if (caps & MAC_10HD)
+	if (caps & MAC_10HD) {
 		__set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, linkmodes);
+		__set_bit(ETHTOOL_LINK_MODE_10baseT1S_Half_BIT, linkmodes);
+		__set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT, linkmodes);
+	}
 
 	if (caps & MAC_10FD) {
 		__set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, linkmodes);
 		__set_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT, linkmodes);
+		__set_bit(ETHTOOL_LINK_MODE_10baseT1S_Full_BIT, linkmodes);
 	}
 
 	if (caps & MAC_100HD) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 55160db311fa..2dfb85c6e596 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1036,6 +1036,17 @@ struct phy_driver {
 	int (*get_sqi)(struct phy_device *dev);
 	/** @get_sqi_max: Get the maximum signal quality indication */
 	int (*get_sqi_max)(struct phy_device *dev);
+
+	/* PLCA RS interface */
+	/** @get_plca_cfg: Return the current PLCA configuration */
+	int (*get_plca_cfg)(struct phy_device *dev,
+			    struct phy_plca_cfg *plca_cfg);
+	/** @set_plca_cfg: Set the PLCA configuration */
+	int (*set_plca_cfg)(struct phy_device *dev,
+			    const struct phy_plca_cfg *plca_cfg);
+	/** @get_plca_status: Return the current PLCA status info */
+	int (*get_plca_status)(struct phy_device *dev,
+			       struct phy_plca_status *plca_st);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
@@ -1832,12 +1843,12 @@ int phy_ethtool_get_strings(struct phy_device *phydev, u8 *data);
 int phy_ethtool_get_sset_count(struct phy_device *phydev);
 int phy_ethtool_get_stats(struct phy_device *phydev,
 			  struct ethtool_stats *stats, u64 *data);
-int phy_ethtool_get_plca_cfg(struct phy_device *dev,
+int phy_ethtool_get_plca_cfg(struct phy_device *phydev,
 			     struct phy_plca_cfg *plca_cfg);
-int phy_ethtool_set_plca_cfg(struct phy_device *dev,
+int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
 			     struct netlink_ext_ack *extack,
 			     const struct phy_plca_cfg *plca_cfg);
-int phy_ethtool_get_plca_status(struct phy_device *dev,
+int phy_ethtool_get_plca_status(struct phy_device *phydev,
 				struct phy_plca_status *plca_st);
 
 static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 58e587ba0450..5f414deacf23 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1741,6 +1741,9 @@ enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_800000baseDR8_2_Full_BIT	 = 96,
 	ETHTOOL_LINK_MODE_800000baseSR8_Full_BIT	 = 97,
 	ETHTOOL_LINK_MODE_800000baseVR8_Full_BIT	 = 98,
+	ETHTOOL_LINK_MODE_10baseT1S_Full_BIT		 = 99,
+	ETHTOOL_LINK_MODE_10baseT1S_Half_BIT		 = 100,
+	ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT	 = 101,
 
 	/* must be last entry */
 	__ETHTOOL_LINK_MODE_MASK_NBITS
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 21cfe8557205..c586db0c5e68 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -208,6 +208,9 @@ const char link_mode_names[][ETH_GSTRING_LEN] = {
 	__DEFINE_LINK_MODE_NAME(800000, DR8_2, Full),
 	__DEFINE_LINK_MODE_NAME(800000, SR8, Full),
 	__DEFINE_LINK_MODE_NAME(800000, VR8, Full),
+	__DEFINE_LINK_MODE_NAME(10, T1S, Full),
+	__DEFINE_LINK_MODE_NAME(10, T1S, Half),
+	__DEFINE_LINK_MODE_NAME(10, T1S_P2MP, Half),
 };
 static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 
@@ -244,6 +247,8 @@ static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 #define __LINK_MODE_LANES_X		1
 #define __LINK_MODE_LANES_FX		1
 #define __LINK_MODE_LANES_T1L		1
+#define __LINK_MODE_LANES_T1S		1
+#define __LINK_MODE_LANES_T1S_P2MP	1
 #define __LINK_MODE_LANES_VR8		8
 #define __LINK_MODE_LANES_DR8_2		8
 
@@ -366,6 +371,9 @@ const struct link_mode_info link_mode_params[] = {
 	__DEFINE_LINK_MODE_PARAMS(800000, DR8_2, Full),
 	__DEFINE_LINK_MODE_PARAMS(800000, SR8, Full),
 	__DEFINE_LINK_MODE_PARAMS(800000, VR8, Full),
+	__DEFINE_LINK_MODE_PARAMS(10, T1S, Full),
+	__DEFINE_LINK_MODE_PARAMS(10, T1S, Half),
+	__DEFINE_LINK_MODE_PARAMS(10, T1S_P2MP, Half),
 };
 static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 
-- 
2.35.1

Re: [PATCH net-next 2/4] phylib: Add support for 10BASE-T1S link modes and PLCA config

[Russell King (Oracle)]

Hi,

On Sun, Dec 04, 2022 at 03:30:52AM +0100, Piergiorgio Beruto wrote:
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index 5d08c627a516..5d8085fffffc 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -13,7 +13,7 @@
>   */
>  const char *phy_speed_to_str(int speed)
>  {
> -	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 99,
> +	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
>  		"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
>  		"If a speed or mode has been added please update phy_speed_to_str "
>  		"and the PHY settings array.\n");

I think you need to update settings[] in this file as well.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/4] phylib: Add support for 10BASE-T1S link modes and PLCA config

[Piergiorgio Beruto]

On Sun, Dec 04, 2022 at 04:45:22PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> On Sun, Dec 04, 2022 at 03:30:52AM +0100, Piergiorgio Beruto wrote:
> > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> > index 5d08c627a516..5d8085fffffc 100644
> > --- a/drivers/net/phy/phy-core.c
> > +++ b/drivers/net/phy/phy-core.c
> > @@ -13,7 +13,7 @@
> >   */
> >  const char *phy_speed_to_str(int speed)
> >  {
> > -	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 99,
> > +	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
> >  		"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
> >  		"If a speed or mode has been added please update phy_speed_to_str "
> >  		"and the PHY settings array.\n");
> 
> I think you need to update settings[] in this file as well.
Oh, sure. I've just added the following:

	PHY_SETTING(     10, FULL,     10baseT1S_Full		),
	PHY_SETTING(     10, HALF,     10baseT1S_Half		),
	PHY_SETTING(     10, HALF,     10baseT1S_P2MP_Half	),

I will amend the patch after I reviewed all the feedback.

Thanks,
Piergiorgio

Re: [PATCH net-next 2/4] phylib: Add support for 10BASE-T1S link modes and PLCA config

[Andrew Lunn]

On Sun, Dec 04, 2022 at 03:30:52AM +0100, Piergiorgio Beruto wrote:
> This patch adds the required connection between netlink ethtool and
> phylib to resolve PLCA get/set config and get status messages.
> Additionally, it adds the link modes for the IEEE 802.3cg Clause 147
> 10BASE-T1S Ethernet PHY.

Please break this patch up.

>  const char *phy_speed_to_str(int speed)
>  {
> -	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 99,
> +	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
>  		"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
>  		"If a speed or mode has been added please update phy_speed_to_str "
>  		"and the PHY settings array.\n");

> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1741,6 +1741,9 @@ enum ethtool_link_mode_bit_indices {
>  	ETHTOOL_LINK_MODE_800000baseDR8_2_Full_BIT	 = 96,
>  	ETHTOOL_LINK_MODE_800000baseSR8_Full_BIT	 = 97,
>  	ETHTOOL_LINK_MODE_800000baseVR8_Full_BIT	 = 98,
> +	ETHTOOL_LINK_MODE_10baseT1S_Full_BIT		 = 99,
> +	ETHTOOL_LINK_MODE_10baseT1S_Half_BIT		 = 100,
> +	ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT	 = 101,
>  
>  	/* must be last entry */
>  	__ETHTOOL_LINK_MODE_MASK_NBITS
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 21cfe8557205..c586db0c5e68 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -208,6 +208,9 @@ const char link_mode_names[][ETH_GSTRING_LEN] = {
>  	__DEFINE_LINK_MODE_NAME(800000, DR8_2, Full),
>  	__DEFINE_LINK_MODE_NAME(800000, SR8, Full),
>  	__DEFINE_LINK_MODE_NAME(800000, VR8, Full),
> +	__DEFINE_LINK_MODE_NAME(10, T1S, Full),
> +	__DEFINE_LINK_MODE_NAME(10, T1S, Half),
> +	__DEFINE_LINK_MODE_NAME(10, T1S_P2MP, Half),
>  };
>  static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
>  
> @@ -366,6 +371,9 @@ const struct link_mode_info link_mode_params[] = {
>  	__DEFINE_LINK_MODE_PARAMS(800000, DR8_2, Full),
>  	__DEFINE_LINK_MODE_PARAMS(800000, SR8, Full),
>  	__DEFINE_LINK_MODE_PARAMS(800000, VR8, Full),
> +	__DEFINE_LINK_MODE_PARAMS(10, T1S, Full),
> +	__DEFINE_LINK_MODE_PARAMS(10, T1S, Half),
> +	__DEFINE_LINK_MODE_PARAMS(10, T1S_P2MP, Half),
>  };
>  static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS);

This is one logical change, so makes one patch, for example.

You are aiming for lots of simple, easy to review, well described,
obviously correct patches.

     Andrew

Re: [PATCH net-next 2/4] phylib: Add support for 10BASE-T1S link modes and PLCA config

[Piergiorgio Beruto]

On Sun, Dec 04, 2022 at 07:12:03PM +0100, Andrew Lunn wrote:
> On Sun, Dec 04, 2022 at 03:30:52AM +0100, Piergiorgio Beruto wrote:
> > This patch adds the required connection between netlink ethtool and
> > phylib to resolve PLCA get/set config and get status messages.
> > Additionally, it adds the link modes for the IEEE 802.3cg Clause 147
> > 10BASE-T1S Ethernet PHY.
> 
> Please break this patch up.
> 
> >  const char *phy_speed_to_str(int speed)
> >  {
> > -	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 99,
> > +	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
> >  		"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
> >  		"If a speed or mode has been added please update phy_speed_to_str "
> >  		"and the PHY settings array.\n");
> 
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -1741,6 +1741,9 @@ enum ethtool_link_mode_bit_indices {
> >  	ETHTOOL_LINK_MODE_800000baseDR8_2_Full_BIT	 = 96,
> >  	ETHTOOL_LINK_MODE_800000baseSR8_Full_BIT	 = 97,
> >  	ETHTOOL_LINK_MODE_800000baseVR8_Full_BIT	 = 98,
> > +	ETHTOOL_LINK_MODE_10baseT1S_Full_BIT		 = 99,
> > +	ETHTOOL_LINK_MODE_10baseT1S_Half_BIT		 = 100,
> > +	ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT	 = 101,
> >  
> >  	/* must be last entry */
> >  	__ETHTOOL_LINK_MODE_MASK_NBITS
> > diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> > index 21cfe8557205..c586db0c5e68 100644
> > --- a/net/ethtool/common.c
> > +++ b/net/ethtool/common.c
> > @@ -208,6 +208,9 @@ const char link_mode_names[][ETH_GSTRING_LEN] = {
> >  	__DEFINE_LINK_MODE_NAME(800000, DR8_2, Full),
> >  	__DEFINE_LINK_MODE_NAME(800000, SR8, Full),
> >  	__DEFINE_LINK_MODE_NAME(800000, VR8, Full),
> > +	__DEFINE_LINK_MODE_NAME(10, T1S, Full),
> > +	__DEFINE_LINK_MODE_NAME(10, T1S, Half),
> > +	__DEFINE_LINK_MODE_NAME(10, T1S_P2MP, Half),
> >  };
> >  static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
> >  
> > @@ -366,6 +371,9 @@ const struct link_mode_info link_mode_params[] = {
> >  	__DEFINE_LINK_MODE_PARAMS(800000, DR8_2, Full),
> >  	__DEFINE_LINK_MODE_PARAMS(800000, SR8, Full),
> >  	__DEFINE_LINK_MODE_PARAMS(800000, VR8, Full),
> > +	__DEFINE_LINK_MODE_PARAMS(10, T1S, Full),
> > +	__DEFINE_LINK_MODE_PARAMS(10, T1S, Half),
> > +	__DEFINE_LINK_MODE_PARAMS(10, T1S_P2MP, Half),
> >  };
> >  static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS);
> 
> This is one logical change, so makes one patch, for example.
> 
> You are aiming for lots of simple, easy to review, well described,
> obviously correct patches.
Alright, will do.

Thanks,
Piergiorgio

[PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY

[Piergiorgio Beruto]

Add support for the onsemi NCN26000 10BASE-T1S industrial Ethernet PHY.
The driver supports Point-to-Multipoint operation without
auto-negotiation and with link control handling. The PLCA RS support
will be included on a separate patch.

Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
---
 MAINTAINERS                |   7 ++
 drivers/net/phy/Kconfig    |   7 ++
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/ncn26000.c | 193 +++++++++++++++++++++++++++++++++++++
 4 files changed, 208 insertions(+)
 create mode 100644 drivers/net/phy/ncn26000.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7952243e4b43..f07527baf321 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15357,6 +15357,13 @@ L:	linux-mips@vger.kernel.org
 S:	Maintained
 F:	arch/mips/boot/dts/ralink/omega2p.dts
 
+ONSEMI ETHERNET PHY DRIVERS
+M:	Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
+L:	netdev@vger.kernel.org
+S:	Supported
+W:	http://www.onsemi.com
+F:	drivers/net/phy/ncn*
+
 OP-TEE DRIVER
 M:	Jens Wiklander <jens.wiklander@linaro.org>
 L:	op-tee@lists.trustedfirmware.org
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index af00cf44cd97..7c466830c611 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -267,6 +267,13 @@ config NATIONAL_PHY
 	help
 	  Currently supports the DP83865 PHY.
 
+config NCN26000_PHY
+	tristate "onsemi 10BASE-T1S Ethernet PHY"
+	help
+	  Adds support for the onsemi 10BASE-T1S Ethernet PHY.
+	  Currently supports the NCN26000 10BASE-T1S Industrial PHY
+	  with MII interface.
+
 config NXP_C45_TJA11XX_PHY
 	tristate "NXP C45 TJA11XX PHYs"
 	depends on PTP_1588_CLOCK_OPTIONAL
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f7138d3c896b..b5138066ba04 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
 obj-$(CONFIG_MOTORCOMM_PHY)	+= motorcomm.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
+obj-$(CONFIG_NCN26000_PHY)	+= ncn26000.o
 obj-$(CONFIG_NXP_C45_TJA11XX_PHY)	+= nxp-c45-tja11xx.o
 obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
new file mode 100644
index 000000000000..65a34edc5b20
--- /dev/null
+++ b/drivers/net/phy/ncn26000.c
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/*
+ *  Driver for Analog Devices Industrial Ethernet T1L PHYs
+ *
+ * Copyright 2020 Analog Devices Inc.
+ */
+#include <linux/kernel.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+#include <linux/property.h>
+
+#define PHY_ID_NCN26000				0x180FF5A1
+
+#define NCN26000_REG_IRQ_CTL                    ((u16)16)
+#define NCN26000_REG_IRQ_STATUS                 ((u16)17)
+
+#define NCN26000_IRQ_LINKST_BIT                 ((u16)1)
+#define NCN26000_IRQ_PLCAST_BIT                 ((u16)(1 << 1))
+#define NCN26000_IRQ_LJABBER_BIT                ((u16)(1 << 2))
+#define NCN26000_IRQ_RJABBER_BIT                ((u16)(1 << 3))
+#define NCN26000_IRQ_RJABBER_BIT                ((u16)(1 << 3))
+#define NCN26000_IRQ_PLCAREC_BIT                ((u16)(1 << 4))
+#define NCN26000_IRQ_PHYSCOL_BIT                ((u16)(1 << 5))
+
+struct ncn26000_priv {
+	u16 enabled_irqs;
+};
+
+static int ncn26000_config_init(struct phy_device *phydev)
+{
+	// TODO: add vendor-specific tuning (ENI, CMC, ...)
+	return 0;
+}
+
+static int ncn26000_enable(struct phy_device *phydev)
+{
+	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+	phydev->mdix = ETH_TP_MDI;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+	phydev->speed = SPEED_10;
+	phydev->duplex = DUPLEX_HALF;
+
+	// bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
+	// clear also ISOLATE mode and Collision Test
+	return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
+}
+
+static int ncn26000_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
+
+	if (ret != 0)
+		return ret;
+
+	return phy_read_poll_timeout(phydev,
+				     MII_BMCR,
+				     ret,
+				     !(ret & BMCR_RESET),
+				     500,
+				     20000,
+				     true);
+}
+
+static int ncn26000_get_features(struct phy_device *phydev)
+{
+	linkmode_zero(phydev->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
+			 phydev->supported);
+
+	linkmode_copy(phydev->advertising, phydev->supported);
+	return 0;
+}
+
+static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
+{
+	const struct ncn26000_priv *const priv = phydev->priv;
+	u16 events;
+	int ret;
+
+	// read and aknowledge the IRQ status register
+	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+
+	if (unlikely(ret < 0))
+		return IRQ_NONE;
+
+	events = (u16)ret & priv->enabled_irqs;
+	if (events == 0)
+		return IRQ_NONE;
+
+	if (events & NCN26000_IRQ_LINKST_BIT) {
+		ret = phy_read(phydev, MII_BMSR);
+
+		if (unlikely(ret < 0)) {
+			phydev_err(phydev,
+				   "error reading the status register (%d)\n",
+				   ret);
+
+			return IRQ_NONE;
+		}
+
+		phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
+	}
+
+	// handle more IRQs here
+
+	phy_trigger_machine(phydev);
+	return IRQ_HANDLED;
+}
+
+static int ncn26000_config_intr(struct phy_device *phydev)
+{
+	int ret;
+	struct ncn26000_priv *priv = phydev->priv;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		// acknowledge IRQs
+		ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+		if (ret < 0)
+			return ret;
+
+		// get link status notifications
+		priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT;
+	} else {
+		// disable all IRQs
+		priv->enabled_irqs = 0;
+	}
+
+	ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs);
+	if (ret != 0)
+		return ret;
+
+	return 0;
+}
+
+static int ncn26000_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct ncn26000_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
+static void ncn26000_remove(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct ncn26000_priv *priv = phydev->priv;
+
+	// free the private structure pointer
+	devm_kfree(dev, priv);
+}
+
+static struct phy_driver ncn26000_driver[] = {
+	{
+		PHY_ID_MATCH_MODEL(PHY_ID_NCN26000),
+		.name			= "NCN26000",
+		.probe                  = ncn26000_probe,
+		.remove                 = ncn26000_remove,
+		.get_features		= ncn26000_get_features,
+		.config_init            = ncn26000_config_init,
+		.soft_reset             = ncn26000_soft_reset,
+		.config_intr            = ncn26000_config_intr,
+		.config_aneg		= ncn26000_enable,
+		.handle_interrupt       = ncn26000_handle_interrupt,
+	},
+};
+
+module_phy_driver(ncn26000_driver);
+
+static struct mdio_device_id __maybe_unused ncn26000_tbl[] = {
+	{ PHY_ID_MATCH_MODEL(PHY_ID_NCN26000) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, ncn26000_tbl);
+MODULE_AUTHOR("Piergiorgio Beruto");
+MODULE_DESCRIPTION("onsemi 10BASE-T1S PHY driver");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.35.1

Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY

[Russell King (Oracle)]

Hi,

On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
> new file mode 100644
> index 000000000000..65a34edc5b20
> --- /dev/null
> +++ b/drivers/net/phy/ncn26000.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/*
> + *  Driver for Analog Devices Industrial Ethernet T1L PHYs
> + *
> + * Copyright 2020 Analog Devices Inc.
> + */
> +#include <linux/kernel.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/phy.h>
> +#include <linux/property.h>
> +
> +#define PHY_ID_NCN26000				0x180FF5A1
> +
> +#define NCN26000_REG_IRQ_CTL                    ((u16)16)
> +#define NCN26000_REG_IRQ_STATUS                 ((u16)17)
> +
> +#define NCN26000_IRQ_LINKST_BIT                 ((u16)1)
> +#define NCN26000_IRQ_PLCAST_BIT                 ((u16)(1 << 1))
> +#define NCN26000_IRQ_LJABBER_BIT                ((u16)(1 << 2))
> +#define NCN26000_IRQ_RJABBER_BIT                ((u16)(1 << 3))
> +#define NCN26000_IRQ_RJABBER_BIT                ((u16)(1 << 3))
> +#define NCN26000_IRQ_PLCAREC_BIT                ((u16)(1 << 4))
> +#define NCN26000_IRQ_PHYSCOL_BIT                ((u16)(1 << 5))

There isn't much point in having the casts to u16 here. Also,
BIT() is useful for identifying single bits.

> +static int ncn26000_enable(struct phy_device *phydev)
> +{

This is actually the config_aneg() implementation, it should be named
as such.

> +	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> +	phydev->mdix = ETH_TP_MDI;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +	phydev->speed = SPEED_10;
> +	phydev->duplex = DUPLEX_HALF;

Is this initialisation actually necessary?

> +
> +	// bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
> +	// clear also ISOLATE mode and Collision Test
> +	return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);

You always use AN even when ethtool turns off AN? If AN is mandatory,
it seems there should be some way that phylib can force that to be
the case.

> +}
> +
> +static int ncn26000_soft_reset(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> +
> +	if (ret != 0)
> +		return ret;
> +
> +	return phy_read_poll_timeout(phydev,
> +				     MII_BMCR,
> +				     ret,
> +				     !(ret & BMCR_RESET),
> +				     500,
> +				     20000,
> +				     true);

Isn't this just genphy_reset() ?

> +}
> +
> +static int ncn26000_get_features(struct phy_device *phydev)
> +{
> +	linkmode_zero(phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
> +			 phydev->supported);
> +
> +	linkmode_copy(phydev->advertising, phydev->supported);
> +	return 0;
> +}
> +
> +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> +{
> +	const struct ncn26000_priv *const priv = phydev->priv;
> +	u16 events;
> +	int ret;
> +
> +	// read and aknowledge the IRQ status register
> +	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> +
> +	if (unlikely(ret < 0))
> +		return IRQ_NONE;
> +
> +	events = (u16)ret & priv->enabled_irqs;
> +	if (events == 0)
> +		return IRQ_NONE;
> +
> +	if (events & NCN26000_IRQ_LINKST_BIT) {
> +		ret = phy_read(phydev, MII_BMSR);
> +
> +		if (unlikely(ret < 0)) {
> +			phydev_err(phydev,
> +				   "error reading the status register (%d)\n",
> +				   ret);
> +
> +			return IRQ_NONE;
> +		}
> +
> +		phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;

1. aneg_complete shouldn't be used to set phydev->link.
2. phydev->link should be updated in the read_status() function, which
the state machine will call. Setting it here without taking the lock
introduces races.

> +	}
> +
> +	// handle more IRQs here
> +
> +	phy_trigger_machine(phydev);
> +	return IRQ_HANDLED;
> +}
> +
> +static int ncn26000_config_intr(struct phy_device *phydev)
> +{
> +	int ret;
> +	struct ncn26000_priv *priv = phydev->priv;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		// acknowledge IRQs
> +		ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> +		if (ret < 0)
> +			return ret;
> +
> +		// get link status notifications
> +		priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT;
> +	} else {
> +		// disable all IRQs
> +		priv->enabled_irqs = 0;
> +	}
> +
> +	ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs);
> +	if (ret != 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ncn26000_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct ncn26000_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	phydev->priv = priv;
> +
> +	return 0;
> +}
> +
> +static void ncn26000_remove(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct ncn26000_priv *priv = phydev->priv;
> +
> +	// free the private structure pointer
> +	devm_kfree(dev, priv);

No need to call devm_kfree() - the point of devm_*() is that resources
are automatically released.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY

[Andrew Lunn]

> > +
> > +	// bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
> > +	// clear also ISOLATE mode and Collision Test
> > +	return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> 
> You always use AN even when ethtool turns off AN? If AN is mandatory,
> it seems there should be some way that phylib can force that to be
> the case.

Hi Russell

The comment is trying to explain that the bit BMCR_ANENABLE does not
actually mean Enable Autoneg. Since AN is not supported by T1S PHYs,
and the standard, some vendors have repurposed this bit.

Maybe we need BMCR_T1S_LINK_CTRL, local to this driver.

      Andrew

Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY

[Andrew Lunn]

> On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> > +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> > +{
> > +	const struct ncn26000_priv *const priv = phydev->priv;
> > +	u16 events;
> > +	int ret;
> > +
> > +	// read and aknowledge the IRQ status register
> > +	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > +
> > +	if (unlikely(ret < 0))
> > +		return IRQ_NONE;
> > +
> > +	events = (u16)ret & priv->enabled_irqs;
> > +	if (events == 0)
> > +		return IRQ_NONE;
> > +
> > +	if (events & NCN26000_IRQ_LINKST_BIT) {
> > +		ret = phy_read(phydev, MII_BMSR);
> > +
> > +		if (unlikely(ret < 0)) {
> > +			phydev_err(phydev,
> > +				   "error reading the status register (%d)\n",
> > +				   ret);
> > +
> > +			return IRQ_NONE;
> > +		}
> > +
> > +		phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;

Hi Piergiorgio

Interrupt handling in PHY don't follow the usual model. Historically,
PHYs were always polled once per second. The read_status() function
gets called to report the current status of the PHY. Interrupt are
just used to indicate that poll should happen now. All the handler
needs to do is clear the interrupt line so it can be safely reenabled
and not cause an interrupt storm, and call phy_trigger_machine() to
trigger the poll.

	Andrew

Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY

[Piergiorgio Beruto]

On Sun, Dec 04, 2022 at 07:00:38PM +0100, Andrew Lunn wrote:
> > On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> > > +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> > > +{
> > > +	const struct ncn26000_priv *const priv = phydev->priv;
> > > +	u16 events;
> > > +	int ret;
> > > +
> > > +	// read and aknowledge the IRQ status register
> > > +	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > > +
> > > +	if (unlikely(ret < 0))
> > > +		return IRQ_NONE;
> > > +
> > > +	events = (u16)ret & priv->enabled_irqs;
> > > +	if (events == 0)
> > > +		return IRQ_NONE;
> > > +
> > > +	if (events & NCN26000_IRQ_LINKST_BIT) {
> > > +		ret = phy_read(phydev, MII_BMSR);
> > > +
> > > +		if (unlikely(ret < 0)) {
> > > +			phydev_err(phydev,
> > > +				   "error reading the status register (%d)\n",
> > > +				   ret);
> > > +
> > > +			return IRQ_NONE;
> > > +		}
> > > +
> > > +		phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
> 
> Hi Piergiorgio
> 
> Interrupt handling in PHY don't follow the usual model. Historically,
> PHYs were always polled once per second. The read_status() function
> gets called to report the current status of the PHY. Interrupt are
> just used to indicate that poll should happen now. All the handler
> needs to do is clear the interrupt line so it can be safely reenabled
> and not cause an interrupt storm, and call phy_trigger_machine() to
> trigger the poll.
To be honest, I sort of realized that. But I was confused again by the
read_status() function description. It looked to me it was not invoked
when AN is not supported, but I'm hearing this is not the case.
I'll rework this part.

Thanks
Piergiorgio

Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY

[Piergiorgio Beruto]

On Sun, Dec 04, 2022 at 04:52:04PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> > diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
> > new file mode 100644
> > index 000000000000..65a34edc5b20
> > --- /dev/null
> > +++ b/drivers/net/phy/ncn26000.c
> > @@ -0,0 +1,193 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/*
> > + *  Driver for Analog Devices Industrial Ethernet T1L PHYs
> > + *
> > + * Copyright 2020 Analog Devices Inc.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/mii.h>
> > +#include <linux/phy.h>
> > +#include <linux/property.h>
> > +
> > +#define PHY_ID_NCN26000				0x180FF5A1
> > +
> > +#define NCN26000_REG_IRQ_CTL                    ((u16)16)
> > +#define NCN26000_REG_IRQ_STATUS                 ((u16)17)
> > +
> > +#define NCN26000_IRQ_LINKST_BIT                 ((u16)1)
> > +#define NCN26000_IRQ_PLCAST_BIT                 ((u16)(1 << 1))
> > +#define NCN26000_IRQ_LJABBER_BIT                ((u16)(1 << 2))
> > +#define NCN26000_IRQ_RJABBER_BIT                ((u16)(1 << 3))
> > +#define NCN26000_IRQ_RJABBER_BIT                ((u16)(1 << 3))
> > +#define NCN26000_IRQ_PLCAREC_BIT                ((u16)(1 << 4))
> > +#define NCN26000_IRQ_PHYSCOL_BIT                ((u16)(1 << 5))
> 
> There isn't much point in having the casts to u16 here. Also,
> BIT() is useful for identifying single bits.
Ok, I'll fix, thanks.

> > +static int ncn26000_enable(struct phy_device *phydev)
> > +{
> 
> This is actually the config_aneg() implementation, it should be named
> as such.
I can certainly rename it, however I did this for a reason. The NCN26000
only supports P2MP mode. Therefore, it does not support AN (this is
clearly indicated in the IEEE specifications as well).

However, it is my understanding that the config_aneg() callback is
invoked also for PHYs that do not support AN, and this is actually the
only way to set a link_control bit to have the PHY enable the PMA/PCS
functions. So I thought to call this function "enable" to make it clear
we're not really implementing autoneg, but link_control.

But as I said, I am not strongly biased towards this name, I just wanted
to let you know the rationale behind my choice.

Please let me know if you wish to reconsider or you still prefer to
rename it.

> > +	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> > +	phydev->mdix = ETH_TP_MDI;
> > +	phydev->pause = 0;
> > +	phydev->asym_pause = 0;
> > +	phydev->speed = SPEED_10;
> > +	phydev->duplex = DUPLEX_HALF;
> 
> Is this initialisation actually necessary?
To be honest, I am not sure. Reading the code for genphy_c45_read_pma()
and genphy_c45_read_status() I can see those variables are set. In my
case, the driver is -not- invoking those functions, therefore I thought
this initialization should be needed. If not, I can certainly remove it.
Advices?
 
> > +
> > +	// bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
> > +	// clear also ISOLATE mode and Collision Test
> > +	return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> 
> You always use AN even when ethtool turns off AN? If AN is mandatory,
> it seems there should be some way that phylib can force that to be
> the case.
I need to explain this better. The NCN26000, as I said earlier, does
-not- support AN. However, it re-uses the AN bit to implement the
link_control function (described in the IEEE specifications). Therefore,
setting AN on this PHY actually means bringing up the link.

I don't know if it could be better to add a define (specific for this
PHY) for the link_control bit and set it == BMCR_ANENABLE? Would that be
more clear for the reader?

> > +}
> > +
> > +static int ncn26000_soft_reset(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> > +
> > +	if (ret != 0)
> > +		return ret;
> > +
> > +	return phy_read_poll_timeout(phydev,
> > +				     MII_BMCR,
> > +				     ret,
> > +				     !(ret & BMCR_RESET),
> > +				     500,
> > +				     20000,
> > +				     true);
> 
> Isn't this just genphy_reset() ?
Right, this was a leftover. I substituted with genphy_soft_reset() and
indeed it works just fine. Thanks for noticing.

> > +}
> > +
> > +static int ncn26000_get_features(struct phy_device *phydev)
> > +{
> > +	linkmode_zero(phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);
> > +
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
> > +			 phydev->supported);
> > +
> > +	linkmode_copy(phydev->advertising, phydev->supported);
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> > +{
> > +	const struct ncn26000_priv *const priv = phydev->priv;
> > +	u16 events;
> > +	int ret;
> > +
> > +	// read and aknowledge the IRQ status register
> > +	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > +
> > +	if (unlikely(ret < 0))
> > +		return IRQ_NONE;
> > +
> > +	events = (u16)ret & priv->enabled_irqs;
> > +	if (events == 0)
> > +		return IRQ_NONE;
> > +
> > +	if (events & NCN26000_IRQ_LINKST_BIT) {
> > +		ret = phy_read(phydev, MII_BMSR);
> > +
> > +		if (unlikely(ret < 0)) {
> > +			phydev_err(phydev,
> > +				   "error reading the status register (%d)\n",
> > +				   ret);
> > +
> > +			return IRQ_NONE;
> > +		}
> > +
> > +		phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
> 
> 1. aneg_complete shouldn't be used to set phydev->link.
> 2. phydev->link should be updated in the read_status() function, which
> the state machine will call. Setting it here without taking the lock
> introduces races.
Same as before. AN complete is used as an extended link status
indication for this PHY, considering the PLCA status as well. It is not
the result of AN (which is not supported).

> > +	}
> > +
> > +	// handle more IRQs here
> > +
> > +	phy_trigger_machine(phydev);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int ncn26000_config_intr(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +	struct ncn26000_priv *priv = phydev->priv;
> > +
> > +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > +		// acknowledge IRQs
> > +		ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		// get link status notifications
> > +		priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT;
> > +	} else {
> > +		// disable all IRQs
> > +		priv->enabled_irqs = 0;
> > +	}
> > +
> > +	ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs);
> > +	if (ret != 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ncn26000_probe(struct phy_device *phydev)
> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct ncn26000_priv *priv;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	phydev->priv = priv;
> > +
> > +	return 0;
> > +}
> > +
> > +static void ncn26000_remove(struct phy_device *phydev)
> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct ncn26000_priv *priv = phydev->priv;
> > +
> > +	// free the private structure pointer
> > +	devm_kfree(dev, priv);
> 
> No need to call devm_kfree() - the point of devm_*() is that resources
> are automatically released.
> 
> Thanks.
Got it, Thanks!

Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY

[Andrew Lunn]

> > > +static int ncn26000_enable(struct phy_device *phydev)
> > > +{
> > 
> > This is actually the config_aneg() implementation, it should be named
> > as such.
> I can certainly rename it, however I did this for a reason. The NCN26000
> only supports P2MP mode. Therefore, it does not support AN (this is
> clearly indicated in the IEEE specifications as well).
> 
> However, it is my understanding that the config_aneg() callback is
> invoked also for PHYs that do not support AN, and this is actually the
> only way to set a link_control bit to have the PHY enable the PMA/PCS
> functions. So I thought to call this function "enable" to make it clear
> we're not really implementing autoneg, but link_control.

Anybody familiar with PHY drivers knows the name is not ideal, but
when they see config_aneg() they have a good idea what it does without
having to look at the code. All PHY drivers should have the same basic
structure, naming etc, just to make knowledge transfer between drivers
easy, maintenance easy, etc.

	Andrew

Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY

[Piergiorgio Beruto]

On Sun, Dec 04, 2022 at 07:58:07PM +0100, Andrew Lunn wrote:
> > > > +static int ncn26000_enable(struct phy_device *phydev)
> > > > +{
> > > 
> > > This is actually the config_aneg() implementation, it should be named
> > > as such.
> > I can certainly rename it, however I did this for a reason. The NCN26000
> > only supports P2MP mode. Therefore, it does not support AN (this is
> > clearly indicated in the IEEE specifications as well).
> > 
> > However, it is my understanding that the config_aneg() callback is
> > invoked also for PHYs that do not support AN, and this is actually the
> > only way to set a link_control bit to have the PHY enable the PMA/PCS
> > functions. So I thought to call this function "enable" to make it clear
> > we're not really implementing autoneg, but link_control.
> 
> Anybody familiar with PHY drivers knows the name is not ideal, but
> when they see config_aneg() they have a good idea what it does without
> having to look at the code. All PHY drivers should have the same basic
> structure, naming etc, just to make knowledge transfer between drivers
> easy, maintenance easy, etc.
Fair enough, I'll change the name in the next patchset version

[PATCH net-next 4/4] driver/ncn26000: add PLCA support

[Piergiorgio Beruto]

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

Re: [PATCH net-next 4/4] driver/ncn26000: add PLCA support

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/4] driver/ncn26000: add PLCA support

[Andrew Lunn]

> 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

Re: [PATCH net-next 4/4] driver/ncn26000: add PLCA support

[Andrew Lunn]

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

Re: [PATCH net-next 4/4] driver/ncn26000: add PLCA support

[Piergiorgio Beruto]

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

Re: [PATCH net-next 4/4] driver/ncn26000: add PLCA support

[Russell King (Oracle)]

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 :(

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/4] driver/ncn26000: add PLCA support

[Piergiorgio Beruto]

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

Re: [PATCH net-next 4/4] driver/ncn26000: add PLCA support

[Piergiorgio Beruto]

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