[RFC v1 net-next 0/8] add support for the the vsc7512 internal copper phys

[RFC v1 net-next 0/8] add support for the the vsc7512 internal copper phys

[Colin Foster]

This patch series is a continuation to add support for the VSC7512:
https://patchwork.kernel.org/project/netdevbpf/list/?series=674168&state=*

That series added the framework and initial functionality for the
VSC7512 chip. Several of these patches grew during the initial
development of the framework, which is why v1 will include changelogs.
It was during v9 of that original MFD patch set that these were dropped.

With that out of the way, the VSC7512 is mainly a subset of the VSC7514
chip. The 7512 lacks an internal MIPS processor, but otherwise many of
the register definitions are identical. That is why several of these
patches are simply to expose common resources from
drivers/net/ethernet/mscc/*.

This patch only adds support for the first four ports (swp0-swp3). The
remaining ports require more significant changes to the felix driver,
and will be handled in the future.

Lastly, this patch set currently won't apply cleanly to net-next, as it
requires a sync with the MFD tree. This is being sent as an RFC, and
additional changes (e.g. documentation) will be required, so I expect
this won't be ready to become a PATCH until the next version (v6.2)


v1 (from RFC v8 suggested above):
    * Utilize the MFD framework for creating regmaps, as well as
      dev_get_regmap() (patches 7 and 8 of this series)

Colin Foster (8):
  net: mscc: ocelot: expose ocelot wm functions
  net: mscc: ocelot: expose regfield definition to be used by other
    drivers
  net: mscc: ocelot: expose stats layout definition to be used by other
    drivers
  net: mscc: ocelot: expose vcap_props structure
  net: dsa: felix: add configurable device quirks
  net: dsa: felix: populate mac_capabilities for all ports
  mfd: ocelot: add regmaps for ocelot_ext
  net: dsa: ocelot: add external ocelot switch control

 drivers/mfd/ocelot-core.c                  |  91 +++++++-
 drivers/net/dsa/ocelot/Kconfig             |  14 ++
 drivers/net/dsa/ocelot/Makefile            |   5 +
 drivers/net/dsa/ocelot/felix.c             |  10 +-
 drivers/net/dsa/ocelot/felix.h             |   1 +
 drivers/net/dsa/ocelot/felix_vsc9959.c     |   1 +
 drivers/net/dsa/ocelot/ocelot_ext.c        | 254 +++++++++++++++++++++
 drivers/net/dsa/ocelot/seville_vsc9953.c   |   1 +
 drivers/net/ethernet/mscc/ocelot_devlink.c |  31 +++
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 137 +----------
 drivers/net/ethernet/mscc/vsc7514_regs.c   | 108 +++++++++
 include/linux/mfd/ocelot.h                 |   5 +
 include/soc/mscc/ocelot.h                  |   7 +
 include/soc/mscc/vsc7514_regs.h            |   6 +
 14 files changed, 529 insertions(+), 142 deletions(-)
 create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c

-- 
2.25.1

[RFC v1 net-next 1/8] net: mscc: ocelot: expose ocelot wm functions

[Colin Foster]

Expose ocelot_wm functions so they can be shared with other drivers.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---

v1:
    * No changes since previous RFC

---
 drivers/net/ethernet/mscc/ocelot_devlink.c | 31 ++++++++++++++++++++++
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 28 -------------------
 include/soc/mscc/ocelot.h                  |  5 ++++
 3 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_devlink.c b/drivers/net/ethernet/mscc/ocelot_devlink.c
index b8737efd2a85..d9ea75a14f2f 100644
--- a/drivers/net/ethernet/mscc/ocelot_devlink.c
+++ b/drivers/net/ethernet/mscc/ocelot_devlink.c
@@ -487,6 +487,37 @@ static void ocelot_watermark_init(struct ocelot *ocelot)
 	ocelot_setup_sharing_watermarks(ocelot);
 }
 
+/* Watermark encode
+ * Bit 8:   Unit; 0:1, 1:16
+ * Bit 7-0: Value to be multiplied with unit
+ */
+u16 ocelot_wm_enc(u16 value)
+{
+	WARN_ON(value >= 16 * BIT(8));
+
+	if (value >= BIT(8))
+		return BIT(8) | (value / 16);
+
+	return value;
+}
+EXPORT_SYMBOL(ocelot_wm_enc);
+
+u16 ocelot_wm_dec(u16 wm)
+{
+	if (wm & BIT(8))
+		return (wm & GENMASK(7, 0)) * 16;
+
+	return wm;
+}
+EXPORT_SYMBOL(ocelot_wm_dec);
+
+void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse)
+{
+	*inuse = (val & GENMASK(23, 12)) >> 12;
+	*maxuse = val & GENMASK(11, 0);
+}
+EXPORT_SYMBOL(ocelot_wm_stat);
+
 /* Pool size and type are fixed up at runtime. Keeping this structure to
  * look up the cell size multipliers.
  */
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index ae42bbba5747..80e88bfd38ad 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -214,34 +214,6 @@ static int ocelot_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-/* Watermark encode
- * Bit 8:   Unit; 0:1, 1:16
- * Bit 7-0: Value to be multiplied with unit
- */
-static u16 ocelot_wm_enc(u16 value)
-{
-	WARN_ON(value >= 16 * BIT(8));
-
-	if (value >= BIT(8))
-		return BIT(8) | (value / 16);
-
-	return value;
-}
-
-static u16 ocelot_wm_dec(u16 wm)
-{
-	if (wm & BIT(8))
-		return (wm & GENMASK(7, 0)) * 16;
-
-	return wm;
-}
-
-static void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse)
-{
-	*inuse = (val & GENMASK(23, 12)) >> 12;
-	*maxuse = val & GENMASK(11, 0);
-}
-
 static const struct ocelot_ops ocelot_ops = {
 	.reset			= ocelot_reset,
 	.wm_enc			= ocelot_wm_enc,
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 355cfdedc43b..17dd61f36563 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -1145,6 +1145,11 @@ void ocelot_port_assign_dsa_8021q_cpu(struct ocelot *ocelot, int port, int cpu);
 void ocelot_port_unassign_dsa_8021q_cpu(struct ocelot *ocelot, int port);
 u32 ocelot_port_assigned_dsa_8021q_cpu_mask(struct ocelot *ocelot, int port);
 
+/* Watermark interface */
+u16 ocelot_wm_enc(u16 value);
+u16 ocelot_wm_dec(u16 wm);
+void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse);
+
 /* DSA callbacks */
 void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data);
 void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data);
-- 
2.25.1

[RFC v1 net-next 2/8] net: mscc: ocelot: expose regfield definition to be used by other drivers

[Colin Foster]

The ocelot_regfields struct is common between several different chips, some
of which can only be controlled externally. Export this structure so it
doesn't have to be duplicated in these other drivers.

Rename the structure as well, to follow the conventions of other shared
resources.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

v1 from previous RFC:
    * Remove GCB_SOFT_RST_SWC_RST entry from the regfields struct - it
      isn't used.
    * Export the vsc7514_regfields symbol so it can be used as a module.

---
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 60 +---------------------
 drivers/net/ethernet/mscc/vsc7514_regs.c   | 59 +++++++++++++++++++++
 include/soc/mscc/vsc7514_regs.h            |  2 +
 3 files changed, 62 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 80e88bfd38ad..e9c7740f20e9 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -38,64 +38,6 @@ static const u32 *ocelot_regmap[TARGET_MAX] = {
 	[DEV_GMII] = vsc7514_dev_gmii_regmap,
 };
 
-static const struct reg_field ocelot_regfields[REGFIELD_MAX] = {
-	[ANA_ADVLEARN_VLAN_CHK] = REG_FIELD(ANA_ADVLEARN, 11, 11),
-	[ANA_ADVLEARN_LEARN_MIRROR] = REG_FIELD(ANA_ADVLEARN, 0, 10),
-	[ANA_ANEVENTS_MSTI_DROP] = REG_FIELD(ANA_ANEVENTS, 27, 27),
-	[ANA_ANEVENTS_ACLKILL] = REG_FIELD(ANA_ANEVENTS, 26, 26),
-	[ANA_ANEVENTS_ACLUSED] = REG_FIELD(ANA_ANEVENTS, 25, 25),
-	[ANA_ANEVENTS_AUTOAGE] = REG_FIELD(ANA_ANEVENTS, 24, 24),
-	[ANA_ANEVENTS_VS2TTL1] = REG_FIELD(ANA_ANEVENTS, 23, 23),
-	[ANA_ANEVENTS_STORM_DROP] = REG_FIELD(ANA_ANEVENTS, 22, 22),
-	[ANA_ANEVENTS_LEARN_DROP] = REG_FIELD(ANA_ANEVENTS, 21, 21),
-	[ANA_ANEVENTS_AGED_ENTRY] = REG_FIELD(ANA_ANEVENTS, 20, 20),
-	[ANA_ANEVENTS_CPU_LEARN_FAILED] = REG_FIELD(ANA_ANEVENTS, 19, 19),
-	[ANA_ANEVENTS_AUTO_LEARN_FAILED] = REG_FIELD(ANA_ANEVENTS, 18, 18),
-	[ANA_ANEVENTS_LEARN_REMOVE] = REG_FIELD(ANA_ANEVENTS, 17, 17),
-	[ANA_ANEVENTS_AUTO_LEARNED] = REG_FIELD(ANA_ANEVENTS, 16, 16),
-	[ANA_ANEVENTS_AUTO_MOVED] = REG_FIELD(ANA_ANEVENTS, 15, 15),
-	[ANA_ANEVENTS_DROPPED] = REG_FIELD(ANA_ANEVENTS, 14, 14),
-	[ANA_ANEVENTS_CLASSIFIED_DROP] = REG_FIELD(ANA_ANEVENTS, 13, 13),
-	[ANA_ANEVENTS_CLASSIFIED_COPY] = REG_FIELD(ANA_ANEVENTS, 12, 12),
-	[ANA_ANEVENTS_VLAN_DISCARD] = REG_FIELD(ANA_ANEVENTS, 11, 11),
-	[ANA_ANEVENTS_FWD_DISCARD] = REG_FIELD(ANA_ANEVENTS, 10, 10),
-	[ANA_ANEVENTS_MULTICAST_FLOOD] = REG_FIELD(ANA_ANEVENTS, 9, 9),
-	[ANA_ANEVENTS_UNICAST_FLOOD] = REG_FIELD(ANA_ANEVENTS, 8, 8),
-	[ANA_ANEVENTS_DEST_KNOWN] = REG_FIELD(ANA_ANEVENTS, 7, 7),
-	[ANA_ANEVENTS_BUCKET3_MATCH] = REG_FIELD(ANA_ANEVENTS, 6, 6),
-	[ANA_ANEVENTS_BUCKET2_MATCH] = REG_FIELD(ANA_ANEVENTS, 5, 5),
-	[ANA_ANEVENTS_BUCKET1_MATCH] = REG_FIELD(ANA_ANEVENTS, 4, 4),
-	[ANA_ANEVENTS_BUCKET0_MATCH] = REG_FIELD(ANA_ANEVENTS, 3, 3),
-	[ANA_ANEVENTS_CPU_OPERATION] = REG_FIELD(ANA_ANEVENTS, 2, 2),
-	[ANA_ANEVENTS_DMAC_LOOKUP] = REG_FIELD(ANA_ANEVENTS, 1, 1),
-	[ANA_ANEVENTS_SMAC_LOOKUP] = REG_FIELD(ANA_ANEVENTS, 0, 0),
-	[ANA_TABLES_MACACCESS_B_DOM] = REG_FIELD(ANA_TABLES_MACACCESS, 18, 18),
-	[ANA_TABLES_MACTINDX_BUCKET] = REG_FIELD(ANA_TABLES_MACTINDX, 10, 11),
-	[ANA_TABLES_MACTINDX_M_INDEX] = REG_FIELD(ANA_TABLES_MACTINDX, 0, 9),
-	[QSYS_TIMED_FRAME_ENTRY_TFRM_VLD] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 20, 20),
-	[QSYS_TIMED_FRAME_ENTRY_TFRM_FP] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 8, 19),
-	[QSYS_TIMED_FRAME_ENTRY_TFRM_PORTNO] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 4, 7),
-	[QSYS_TIMED_FRAME_ENTRY_TFRM_TM_SEL] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 1, 3),
-	[QSYS_TIMED_FRAME_ENTRY_TFRM_TM_T] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 0, 0),
-	[SYS_RESET_CFG_CORE_ENA] = REG_FIELD(SYS_RESET_CFG, 2, 2),
-	[SYS_RESET_CFG_MEM_ENA] = REG_FIELD(SYS_RESET_CFG, 1, 1),
-	[SYS_RESET_CFG_MEM_INIT] = REG_FIELD(SYS_RESET_CFG, 0, 0),
-	/* Replicated per number of ports (12), register size 4 per port */
-	[QSYS_SWITCH_PORT_MODE_PORT_ENA] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 14, 14, 12, 4),
-	[QSYS_SWITCH_PORT_MODE_SCH_NEXT_CFG] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 11, 13, 12, 4),
-	[QSYS_SWITCH_PORT_MODE_YEL_RSRVD] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 10, 10, 12, 4),
-	[QSYS_SWITCH_PORT_MODE_INGRESS_DROP_MODE] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 9, 9, 12, 4),
-	[QSYS_SWITCH_PORT_MODE_TX_PFC_ENA] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 1, 8, 12, 4),
-	[QSYS_SWITCH_PORT_MODE_TX_PFC_MODE] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 0, 0, 12, 4),
-	[SYS_PORT_MODE_DATA_WO_TS] = REG_FIELD_ID(SYS_PORT_MODE, 5, 6, 12, 4),
-	[SYS_PORT_MODE_INCL_INJ_HDR] = REG_FIELD_ID(SYS_PORT_MODE, 3, 4, 12, 4),
-	[SYS_PORT_MODE_INCL_XTR_HDR] = REG_FIELD_ID(SYS_PORT_MODE, 1, 2, 12, 4),
-	[SYS_PORT_MODE_INCL_HDR_ERR] = REG_FIELD_ID(SYS_PORT_MODE, 0, 0, 12, 4),
-	[SYS_PAUSE_CFG_PAUSE_START] = REG_FIELD_ID(SYS_PAUSE_CFG, 10, 18, 12, 4),
-	[SYS_PAUSE_CFG_PAUSE_STOP] = REG_FIELD_ID(SYS_PAUSE_CFG, 1, 9, 12, 4),
-	[SYS_PAUSE_CFG_PAUSE_ENA] = REG_FIELD_ID(SYS_PAUSE_CFG, 0, 1, 12, 4),
-};
-
 static const struct ocelot_stat_layout ocelot_stats_layout[OCELOT_NUM_STATS] = {
 	OCELOT_COMMON_STATS,
 };
@@ -138,7 +80,7 @@ static int ocelot_chip_init(struct ocelot *ocelot, const struct ocelot_ops *ops)
 	ocelot->num_mact_rows = 1024;
 	ocelot->ops = ops;
 
-	ret = ocelot_regfields_init(ocelot, ocelot_regfields);
+	ret = ocelot_regfields_init(ocelot, vsc7514_regfields);
 	if (ret)
 		return ret;
 
diff --git a/drivers/net/ethernet/mscc/vsc7514_regs.c b/drivers/net/ethernet/mscc/vsc7514_regs.c
index 9d2d3e13cacf..123175618251 100644
--- a/drivers/net/ethernet/mscc/vsc7514_regs.c
+++ b/drivers/net/ethernet/mscc/vsc7514_regs.c
@@ -9,6 +9,65 @@
 #include <soc/mscc/vsc7514_regs.h>
 #include "ocelot.h"
 
+const struct reg_field vsc7514_regfields[REGFIELD_MAX] = {
+	[ANA_ADVLEARN_VLAN_CHK] = REG_FIELD(ANA_ADVLEARN, 11, 11),
+	[ANA_ADVLEARN_LEARN_MIRROR] = REG_FIELD(ANA_ADVLEARN, 0, 10),
+	[ANA_ANEVENTS_MSTI_DROP] = REG_FIELD(ANA_ANEVENTS, 27, 27),
+	[ANA_ANEVENTS_ACLKILL] = REG_FIELD(ANA_ANEVENTS, 26, 26),
+	[ANA_ANEVENTS_ACLUSED] = REG_FIELD(ANA_ANEVENTS, 25, 25),
+	[ANA_ANEVENTS_AUTOAGE] = REG_FIELD(ANA_ANEVENTS, 24, 24),
+	[ANA_ANEVENTS_VS2TTL1] = REG_FIELD(ANA_ANEVENTS, 23, 23),
+	[ANA_ANEVENTS_STORM_DROP] = REG_FIELD(ANA_ANEVENTS, 22, 22),
+	[ANA_ANEVENTS_LEARN_DROP] = REG_FIELD(ANA_ANEVENTS, 21, 21),
+	[ANA_ANEVENTS_AGED_ENTRY] = REG_FIELD(ANA_ANEVENTS, 20, 20),
+	[ANA_ANEVENTS_CPU_LEARN_FAILED] = REG_FIELD(ANA_ANEVENTS, 19, 19),
+	[ANA_ANEVENTS_AUTO_LEARN_FAILED] = REG_FIELD(ANA_ANEVENTS, 18, 18),
+	[ANA_ANEVENTS_LEARN_REMOVE] = REG_FIELD(ANA_ANEVENTS, 17, 17),
+	[ANA_ANEVENTS_AUTO_LEARNED] = REG_FIELD(ANA_ANEVENTS, 16, 16),
+	[ANA_ANEVENTS_AUTO_MOVED] = REG_FIELD(ANA_ANEVENTS, 15, 15),
+	[ANA_ANEVENTS_DROPPED] = REG_FIELD(ANA_ANEVENTS, 14, 14),
+	[ANA_ANEVENTS_CLASSIFIED_DROP] = REG_FIELD(ANA_ANEVENTS, 13, 13),
+	[ANA_ANEVENTS_CLASSIFIED_COPY] = REG_FIELD(ANA_ANEVENTS, 12, 12),
+	[ANA_ANEVENTS_VLAN_DISCARD] = REG_FIELD(ANA_ANEVENTS, 11, 11),
+	[ANA_ANEVENTS_FWD_DISCARD] = REG_FIELD(ANA_ANEVENTS, 10, 10),
+	[ANA_ANEVENTS_MULTICAST_FLOOD] = REG_FIELD(ANA_ANEVENTS, 9, 9),
+	[ANA_ANEVENTS_UNICAST_FLOOD] = REG_FIELD(ANA_ANEVENTS, 8, 8),
+	[ANA_ANEVENTS_DEST_KNOWN] = REG_FIELD(ANA_ANEVENTS, 7, 7),
+	[ANA_ANEVENTS_BUCKET3_MATCH] = REG_FIELD(ANA_ANEVENTS, 6, 6),
+	[ANA_ANEVENTS_BUCKET2_MATCH] = REG_FIELD(ANA_ANEVENTS, 5, 5),
+	[ANA_ANEVENTS_BUCKET1_MATCH] = REG_FIELD(ANA_ANEVENTS, 4, 4),
+	[ANA_ANEVENTS_BUCKET0_MATCH] = REG_FIELD(ANA_ANEVENTS, 3, 3),
+	[ANA_ANEVENTS_CPU_OPERATION] = REG_FIELD(ANA_ANEVENTS, 2, 2),
+	[ANA_ANEVENTS_DMAC_LOOKUP] = REG_FIELD(ANA_ANEVENTS, 1, 1),
+	[ANA_ANEVENTS_SMAC_LOOKUP] = REG_FIELD(ANA_ANEVENTS, 0, 0),
+	[ANA_TABLES_MACACCESS_B_DOM] = REG_FIELD(ANA_TABLES_MACACCESS, 18, 18),
+	[ANA_TABLES_MACTINDX_BUCKET] = REG_FIELD(ANA_TABLES_MACTINDX, 10, 11),
+	[ANA_TABLES_MACTINDX_M_INDEX] = REG_FIELD(ANA_TABLES_MACTINDX, 0, 9),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_VLD] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 20, 20),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_FP] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 8, 19),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_PORTNO] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 4, 7),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_TM_SEL] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 1, 3),
+	[QSYS_TIMED_FRAME_ENTRY_TFRM_TM_T] = REG_FIELD(QSYS_TIMED_FRAME_ENTRY, 0, 0),
+	[SYS_RESET_CFG_CORE_ENA] = REG_FIELD(SYS_RESET_CFG, 2, 2),
+	[SYS_RESET_CFG_MEM_ENA] = REG_FIELD(SYS_RESET_CFG, 1, 1),
+	[SYS_RESET_CFG_MEM_INIT] = REG_FIELD(SYS_RESET_CFG, 0, 0),
+	/* Replicated per number of ports (12), register size 4 per port */
+	[QSYS_SWITCH_PORT_MODE_PORT_ENA] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 14, 14, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_SCH_NEXT_CFG] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 11, 13, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_YEL_RSRVD] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 10, 10, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_INGRESS_DROP_MODE] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 9, 9, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_TX_PFC_ENA] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 1, 8, 12, 4),
+	[QSYS_SWITCH_PORT_MODE_TX_PFC_MODE] = REG_FIELD_ID(QSYS_SWITCH_PORT_MODE, 0, 0, 12, 4),
+	[SYS_PORT_MODE_DATA_WO_TS] = REG_FIELD_ID(SYS_PORT_MODE, 5, 6, 12, 4),
+	[SYS_PORT_MODE_INCL_INJ_HDR] = REG_FIELD_ID(SYS_PORT_MODE, 3, 4, 12, 4),
+	[SYS_PORT_MODE_INCL_XTR_HDR] = REG_FIELD_ID(SYS_PORT_MODE, 1, 2, 12, 4),
+	[SYS_PORT_MODE_INCL_HDR_ERR] = REG_FIELD_ID(SYS_PORT_MODE, 0, 0, 12, 4),
+	[SYS_PAUSE_CFG_PAUSE_START] = REG_FIELD_ID(SYS_PAUSE_CFG, 10, 18, 12, 4),
+	[SYS_PAUSE_CFG_PAUSE_STOP] = REG_FIELD_ID(SYS_PAUSE_CFG, 1, 9, 12, 4),
+	[SYS_PAUSE_CFG_PAUSE_ENA] = REG_FIELD_ID(SYS_PAUSE_CFG, 0, 1, 12, 4),
+};
+EXPORT_SYMBOL(vsc7514_regfields);
+
 const u32 vsc7514_ana_regmap[] = {
 	REG(ANA_ADVLEARN,				0x009000),
 	REG(ANA_VLANMASK,				0x009004),
diff --git a/include/soc/mscc/vsc7514_regs.h b/include/soc/mscc/vsc7514_regs.h
index ceee26c96959..9b40e7d00ec5 100644
--- a/include/soc/mscc/vsc7514_regs.h
+++ b/include/soc/mscc/vsc7514_regs.h
@@ -10,6 +10,8 @@
 
 #include <soc/mscc/ocelot_vcap.h>
 
+extern const struct reg_field vsc7514_regfields[REGFIELD_MAX];
+
 extern const u32 vsc7514_ana_regmap[];
 extern const u32 vsc7514_qs_regmap[];
 extern const u32 vsc7514_qsys_regmap[];
-- 
2.25.1

Re: [RFC v1 net-next 2/8] net: mscc: ocelot: expose regfield definition to be used by other drivers

[Vladimir Oltean]

On Sun, Sep 11, 2022 at 01:02:38PM -0700, Colin Foster wrote:
> The ocelot_regfields struct is common between several different chips, some
> of which can only be controlled externally. Export this structure so it
> doesn't have to be duplicated in these other drivers.
> 
> Rename the structure as well, to follow the conventions of other shared
> resources.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [RFC v1 net-next 2/8] net: mscc: ocelot: expose regfield definition to be used by other drivers

[Colin Foster]

On Mon, Sep 12, 2022 at 03:47:15PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:38PM -0700, Colin Foster wrote:
> > The ocelot_regfields struct is common between several different chips, some
> > of which can only be controlled externally. Export this structure so it
> > doesn't have to be duplicated in these other drivers.
> > 
> > Rename the structure as well, to follow the conventions of other shared
> > resources.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

I'm assuming you'll agree with my a-ha moment regarding ocelot_reset()
being in the ocelot_lib.

There might be a few others as well. Should I add them as more "export
function X" commits, or squash them (and these already-reviewed commits)
in a larger "export a bunch of resources and symbols" type commit to
keep the patch count low?

Re: [RFC v1 net-next 2/8] net: mscc: ocelot: expose regfield definition to be used by other drivers

[Vladimir Oltean]

On Fri, Sep 16, 2022 at 10:44:56AM -0700, Colin Foster wrote:
> I'm assuming you'll agree with my a-ha moment regarding ocelot_reset()
> being in the ocelot_lib.
> 
> There might be a few others as well. Should I add them as more "export
> function X" commits, or squash them (and these already-reviewed commits)
> in a larger "export a bunch of resources and symbols" type commit to
> keep the patch count low?

Try to keep patches doing one thing and one thing well (and leave this
patch alone).

[RFC v1 net-next 3/8] net: mscc: ocelot: expose stats layout definition to be used by other drivers

[Colin Foster]

The ocelot_stats_layout array is common between several different chips,
some of which can only be controlled externally. Export this structure so
it doesn't have to be duplicated in these other drivers.

Rename the structure as well, to follow the conventions of other shared
resources.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

v1 from previous RFC:
    * Utilize OCELOT_COMMON_STATS

---
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 6 +-----
 drivers/net/ethernet/mscc/vsc7514_regs.c   | 5 +++++
 include/soc/mscc/vsc7514_regs.h            | 3 +++
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index e9c7740f20e9..7673ed76358b 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -38,10 +38,6 @@ static const u32 *ocelot_regmap[TARGET_MAX] = {
 	[DEV_GMII] = vsc7514_dev_gmii_regmap,
 };
 
-static const struct ocelot_stat_layout ocelot_stats_layout[OCELOT_NUM_STATS] = {
-	OCELOT_COMMON_STATS,
-};
-
 static void ocelot_pll5_init(struct ocelot *ocelot)
 {
 	/* Configure PLL5. This will need a proper CCF driver
@@ -76,7 +72,7 @@ static int ocelot_chip_init(struct ocelot *ocelot, const struct ocelot_ops *ops)
 	int ret;
 
 	ocelot->map = ocelot_regmap;
-	ocelot->stats_layout = ocelot_stats_layout;
+	ocelot->stats_layout = vsc7514_stats_layout;
 	ocelot->num_mact_rows = 1024;
 	ocelot->ops = ops;
 
diff --git a/drivers/net/ethernet/mscc/vsc7514_regs.c b/drivers/net/ethernet/mscc/vsc7514_regs.c
index 123175618251..d665522e18c6 100644
--- a/drivers/net/ethernet/mscc/vsc7514_regs.c
+++ b/drivers/net/ethernet/mscc/vsc7514_regs.c
@@ -9,6 +9,11 @@
 #include <soc/mscc/vsc7514_regs.h>
 #include "ocelot.h"
 
+const struct ocelot_stat_layout vsc7514_stats_layout[OCELOT_NUM_STATS] = {
+	OCELOT_COMMON_STATS,
+};
+EXPORT_SYMBOL(vsc7514_stats_layout);
+
 const struct reg_field vsc7514_regfields[REGFIELD_MAX] = {
 	[ANA_ADVLEARN_VLAN_CHK] = REG_FIELD(ANA_ADVLEARN, 11, 11),
 	[ANA_ADVLEARN_LEARN_MIRROR] = REG_FIELD(ANA_ADVLEARN, 0, 10),
diff --git a/include/soc/mscc/vsc7514_regs.h b/include/soc/mscc/vsc7514_regs.h
index 9b40e7d00ec5..d2b5b6b86aff 100644
--- a/include/soc/mscc/vsc7514_regs.h
+++ b/include/soc/mscc/vsc7514_regs.h
@@ -8,8 +8,11 @@
 #ifndef VSC7514_REGS_H
 #define VSC7514_REGS_H
 
+#include <soc/mscc/ocelot.h>
 #include <soc/mscc/ocelot_vcap.h>
 
+extern const struct ocelot_stat_layout vsc7514_stats_layout[];
+
 extern const struct reg_field vsc7514_regfields[REGFIELD_MAX];
 
 extern const u32 vsc7514_ana_regmap[];
-- 
2.25.1

[RFC v1 net-next 4/8] net: mscc: ocelot: expose vcap_props structure

[Colin Foster]

The vcap_props structure is common to other devices, specifically the
VSC7512 chip that can only be controlled externally. Export this structure
so it doesn't need to be recreated.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

v1 from previous RFC:
    * No changes

---
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 43 ---------------------
 drivers/net/ethernet/mscc/vsc7514_regs.c   | 44 ++++++++++++++++++++++
 include/soc/mscc/vsc7514_regs.h            |  1 +
 3 files changed, 45 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 7673ed76358b..6191bca7a9c4 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -161,49 +161,6 @@ static const struct ocelot_ops ocelot_ops = {
 	.netdev_to_port		= ocelot_netdev_to_port,
 };
 
-static struct vcap_props vsc7514_vcap_props[] = {
-	[VCAP_ES0] = {
-		.action_type_width = 0,
-		.action_table = {
-			[ES0_ACTION_TYPE_NORMAL] = {
-				.width = 73, /* HIT_STICKY not included */
-				.count = 1,
-			},
-		},
-		.target = S0,
-		.keys = vsc7514_vcap_es0_keys,
-		.actions = vsc7514_vcap_es0_actions,
-	},
-	[VCAP_IS1] = {
-		.action_type_width = 0,
-		.action_table = {
-			[IS1_ACTION_TYPE_NORMAL] = {
-				.width = 78, /* HIT_STICKY not included */
-				.count = 4,
-			},
-		},
-		.target = S1,
-		.keys = vsc7514_vcap_is1_keys,
-		.actions = vsc7514_vcap_is1_actions,
-	},
-	[VCAP_IS2] = {
-		.action_type_width = 1,
-		.action_table = {
-			[IS2_ACTION_TYPE_NORMAL] = {
-				.width = 49,
-				.count = 2
-			},
-			[IS2_ACTION_TYPE_SMAC_SIP] = {
-				.width = 6,
-				.count = 4
-			},
-		},
-		.target = S2,
-		.keys = vsc7514_vcap_is2_keys,
-		.actions = vsc7514_vcap_is2_actions,
-	},
-};
-
 static struct ptp_clock_info ocelot_ptp_clock_info = {
 	.owner		= THIS_MODULE,
 	.name		= "ocelot ptp",
diff --git a/drivers/net/ethernet/mscc/vsc7514_regs.c b/drivers/net/ethernet/mscc/vsc7514_regs.c
index d665522e18c6..c943da4dd1f1 100644
--- a/drivers/net/ethernet/mscc/vsc7514_regs.c
+++ b/drivers/net/ethernet/mscc/vsc7514_regs.c
@@ -644,3 +644,47 @@ const struct vcap_field vsc7514_vcap_is2_actions[] = {
 	[VCAP_IS2_ACT_HIT_CNT]			= { 49, 32 },
 };
 EXPORT_SYMBOL(vsc7514_vcap_is2_actions);
+
+struct vcap_props vsc7514_vcap_props[] = {
+	[VCAP_ES0] = {
+		.action_type_width = 0,
+		.action_table = {
+			[ES0_ACTION_TYPE_NORMAL] = {
+				.width = 73, /* HIT_STICKY not included */
+				.count = 1,
+			},
+		},
+		.target = S0,
+		.keys = vsc7514_vcap_es0_keys,
+		.actions = vsc7514_vcap_es0_actions,
+	},
+	[VCAP_IS1] = {
+		.action_type_width = 0,
+		.action_table = {
+			[IS1_ACTION_TYPE_NORMAL] = {
+				.width = 78, /* HIT_STICKY not included */
+				.count = 4,
+			},
+		},
+		.target = S1,
+		.keys = vsc7514_vcap_is1_keys,
+		.actions = vsc7514_vcap_is1_actions,
+	},
+	[VCAP_IS2] = {
+		.action_type_width = 1,
+		.action_table = {
+			[IS2_ACTION_TYPE_NORMAL] = {
+				.width = 49,
+				.count = 2
+			},
+			[IS2_ACTION_TYPE_SMAC_SIP] = {
+				.width = 6,
+				.count = 4
+			},
+		},
+		.target = S2,
+		.keys = vsc7514_vcap_is2_keys,
+		.actions = vsc7514_vcap_is2_actions,
+	},
+};
+EXPORT_SYMBOL(vsc7514_vcap_props);
diff --git a/include/soc/mscc/vsc7514_regs.h b/include/soc/mscc/vsc7514_regs.h
index d2b5b6b86aff..a939849efd91 100644
--- a/include/soc/mscc/vsc7514_regs.h
+++ b/include/soc/mscc/vsc7514_regs.h
@@ -12,6 +12,7 @@
 #include <soc/mscc/ocelot_vcap.h>
 
 extern const struct ocelot_stat_layout vsc7514_stats_layout[];
+extern struct vcap_props vsc7514_vcap_props[];
 
 extern const struct reg_field vsc7514_regfields[REGFIELD_MAX];
 
-- 
2.25.1

[RFC v1 net-next 5/8] net: dsa: felix: add configurable device quirks

[Colin Foster]

The define FELIX_MAC_QUIRKS was used directly in the felix.c shared driver.
Other devices (VSC7512 for example) don't require the same quirks, so they
need to be configured on a per-device basis.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---

v1 from previous RFC:
    * No changes

---
 drivers/net/dsa/ocelot/felix.c           | 7 +++++--
 drivers/net/dsa/ocelot/felix.h           | 1 +
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 1 +
 drivers/net/dsa/ocelot/seville_vsc9953.c | 1 +
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index c73ef5f7aa64..95a5c5d0815c 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -990,9 +990,12 @@ static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
 					phy_interface_t interface)
 {
 	struct ocelot *ocelot = ds->priv;
+	struct felix *felix;
+
+	felix = ocelot_to_felix(ocelot);
 
 	ocelot_phylink_mac_link_down(ocelot, port, link_an_mode, interface,
-				     FELIX_MAC_QUIRKS);
+				     felix->info->quirks);
 }
 
 static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
@@ -1007,7 +1010,7 @@ static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
 
 	ocelot_phylink_mac_link_up(ocelot, port, phydev, link_an_mode,
 				   interface, speed, duplex, tx_pause, rx_pause,
-				   FELIX_MAC_QUIRKS);
+				   felix->info->quirks);
 
 	if (felix->info->port_sched_speed_set)
 		felix->info->port_sched_speed_set(ocelot, port, speed);
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index deb8dde1fc19..12a1c03bfeb8 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -33,6 +33,7 @@ struct felix_info {
 	u16				vcap_pol_base2;
 	u16				vcap_pol_max2;
 	const struct ptp_clock_info	*ptp_caps;
+	unsigned long			quirks;
 
 	/* Some Ocelot switches are integrated into the SoC without the
 	 * extraction IRQ line connected to the ARM GIC. By enabling this
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 459288d6222c..4adb109c2e77 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -2605,6 +2605,7 @@ static const struct felix_info felix_info_vsc9959 = {
 	.num_mact_rows		= 2048,
 	.num_ports		= VSC9959_NUM_PORTS,
 	.num_tx_queues		= OCELOT_NUM_TC,
+	.quirks			= FELIX_MAC_QUIRKS,
 	.quirk_no_xtr_irq	= true,
 	.ptp_caps		= &vsc9959_ptp_caps,
 	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 3ce1cd1a8d4a..ba71e5fa5921 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1071,6 +1071,7 @@ static const struct felix_info seville_info_vsc9953 = {
 	.vcap_pol_max		= VSC9953_VCAP_POLICER_MAX,
 	.vcap_pol_base2		= VSC9953_VCAP_POLICER_BASE2,
 	.vcap_pol_max2		= VSC9953_VCAP_POLICER_MAX2,
+	.quirks			= FELIX_MAC_QUIRKS,
 	.num_mact_rows		= 2048,
 	.num_ports		= VSC9953_NUM_PORTS,
 	.num_tx_queues		= OCELOT_NUM_TC,
-- 
2.25.1

[RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Colin Foster]

phylink_generic_validate() requires that mac_capabilities is correctly
populated. While no existing drivers have used phylink_generic_validate(),
the ocelot_ext.c driver will. Populate this element so the use of existing
functions is possible.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

v1 from previous RFC:
    * New patch

---
 drivers/net/dsa/ocelot/felix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 95a5c5d0815c..201bf3bdd67d 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -958,6 +958,9 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
 
 	__set_bit(ocelot->ports[port]->phy_mode,
 		  config->supported_interfaces);
+
+	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
+				   MAC_100 | MAC_1000FD | MAC_2500FD;
 }
 
 static void felix_phylink_validate(struct dsa_switch *ds, int port,
-- 
2.25.1

Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Russell King (Oracle)]

On Sun, Sep 11, 2022 at 01:02:42PM -0700, Colin Foster wrote:
> phylink_generic_validate() requires that mac_capabilities is correctly
> populated. While no existing drivers have used phylink_generic_validate(),
> the ocelot_ext.c driver will. Populate this element so the use of existing
> functions is possible.

Ocelot always fills in the .phylink_validate method in struct
dsa_switch_ops, mac_capabilities won't be used as
phylink_generic_validate() will not be called by
dsa_port_phylink_validate().

Also "no existing drivers have used phylink_generic_validate()" I
wonder which drivers you are referring to there. If you are referring
to DSA drivers, then it is extensively used. The following is from
Linus' tree as of today:

$ grep -rl 'dsa_switch_ops' drivers/net/dsa | xargs grep -l phylink_mac_ | xargs grep -L phylink_validate
drivers/net/dsa/xrs700x/xrs700x.c
drivers/net/dsa/mt7530.c
drivers/net/dsa/qca/ar9331.c
drivers/net/dsa/qca/qca8k-8xxx.c
drivers/net/dsa/bcm_sf2.c
drivers/net/dsa/rzn1_a5psw.c
drivers/net/dsa/b53/b53_common.c
drivers/net/dsa/mv88e6xxx/chip.c
drivers/net/dsa/microchip/ksz_common.c
drivers/net/dsa/sja1105/sja1105_main.c
drivers/net/dsa/lantiq_gswip.c
drivers/net/dsa/realtek/rtl8366rb.c
drivers/net/dsa/realtek/rtl8365mb.c

So, I don't think the commit description is anywhere near correct.

Secondly, I don't see a purpose for this patch in the following
patches, as Ocelot continues to always fill in .phylink_validate,
and as I mentioned above, as long as that member is filled in,
mac_capabilities won't be used unless you explicitly call
phylink_generic_validate() in your .phylink_validate() callback.

Therefore, I think you can drop this patch from your series and
you won't see any functional change.

> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
> 
> v1 from previous RFC:
>     * New patch
> 
> ---
>  drivers/net/dsa/ocelot/felix.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 95a5c5d0815c..201bf3bdd67d 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -958,6 +958,9 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
>  
>  	__set_bit(ocelot->ports[port]->phy_mode,
>  		  config->supported_interfaces);
> +
> +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
> +				   MAC_100 | MAC_1000FD | MAC_2500FD;
>  }
>  
>  static void felix_phylink_validate(struct dsa_switch *ds, int port,
> -- 
> 2.25.1
> 
> 

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

Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Vladimir Oltean]

On Mon, Sep 12, 2022 at 09:48:36AM +0100, Russell King (Oracle) wrote:
> On Sun, Sep 11, 2022 at 01:02:42PM -0700, Colin Foster wrote:
> > phylink_generic_validate() requires that mac_capabilities is correctly
> > populated. While no existing drivers have used phylink_generic_validate(),
> > the ocelot_ext.c driver will. Populate this element so the use of existing
> > functions is possible.
> 
> Ocelot always fills in the .phylink_validate method in struct
> dsa_switch_ops, mac_capabilities won't be used as
> phylink_generic_validate() will not be called by
> dsa_port_phylink_validate().

Correct, but felix_phylink_validate() _can_ still directly call
phylink_validate(), right? Colin does not have the full support for
ocelot_ext in this patch set, but this is what he intends to do.

> Also "no existing drivers have used phylink_generic_validate()" I
> wonder which drivers you are referring to there. If you are referring
> to DSA drivers, then it is extensively used. The following is from
> Linus' tree as of today:

By "existing drivers", it is meant felix_vsc9959.c and seville_vsc9953.c,
two drivers in their own right, which use the common felix.c to talk to
(a) DSA and (b) the ocelot switch lib in drivers/net/ethernet/mscc/.
It is true that these existing drivers do not use phylink_generic_validate().
Furthermore, Colin's new ocelot_ext.c is on the same level as
felix_vsc9959.c and seville_vsc9953.c, will use felix.c in the same way,
and will want to use phylink_generic_validate().

> Secondly, I don't see a purpose for this patch in the following
> patches, as Ocelot continues to always fill in .phylink_validate,
> and as I mentioned above, as long as that member is filled in,
> mac_capabilities won't be used unless you explicitly call
> phylink_generic_validate() in your .phylink_validate() callback.

Yes, explicit calling is what Colin explained that he wants to do.

> Therefore, I think you can drop this patch from your series and
> you won't see any functional change.

This is true. I am also a bit surprised at Colin's choices to
(a) not ask the netdev maintainers to pull into net-next the immutable
    branch that Lee provided here:
    https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/
    and instead send some patches for review which are difficult to
    apply directly to any tree
(b) split the work he submitted such that he populates mac_capabilities
    but does not make any use of it (not call phylink_generic_validate
    from anywhere). We try as much as possible to not leave dead code
    behind in the mainline tree, even if future work is intended to
    bring it to life. I do understand that this is an RFC so the patches
    weren't intended to be applied as is, but it is still confusing to
    review a change which, as you've correctly pointed out, has no
    effect to the git tree as it stands.

Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Vladimir Oltean]

On Mon, Sep 12, 2022 at 01:16:21PM +0300, Vladimir Oltean wrote:
> > Therefore, I think you can drop this patch from your series and
> > you won't see any functional change.
> 
> This is true. I am also a bit surprised at Colin's choices to
> (b) split the work he submitted such that he populates mac_capabilities
>     but does not make any use of it (not call phylink_generic_validate
>     from anywhere). We try as much as possible to not leave dead code
>     behind in the mainline tree, even if future work is intended to
>     bring it to life. I do understand that this is an RFC so the patches
>     weren't intended to be applied as is, but it is still confusing to
>     review a change which, as you've correctly pointed out, has no
>     effect to the git tree as it stands.

Ah, I retract this comment; after actually looking at all the patches, I
do see that in patch 8/8, Colin does call phylink_generic_validate().

Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Russell King (Oracle)]

On Mon, Sep 12, 2022 at 11:41:18AM +0000, Vladimir Oltean wrote:
> On Mon, Sep 12, 2022 at 01:16:21PM +0300, Vladimir Oltean wrote:
> > > Therefore, I think you can drop this patch from your series and
> > > you won't see any functional change.
> > 
> > This is true. I am also a bit surprised at Colin's choices to
> > (b) split the work he submitted such that he populates mac_capabilities
> >     but does not make any use of it (not call phylink_generic_validate
> >     from anywhere). We try as much as possible to not leave dead code
> >     behind in the mainline tree, even if future work is intended to
> >     bring it to life. I do understand that this is an RFC so the patches
> >     weren't intended to be applied as is, but it is still confusing to
> >     review a change which, as you've correctly pointed out, has no
> >     effect to the git tree as it stands.
> 
> Ah, I retract this comment; after actually looking at all the patches, I
> do see that in patch 8/8, Colin does call phylink_generic_validate().

Good point, I obviously missed that in the series.

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

Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Colin Foster]

On Mon, Sep 12, 2022 at 04:32:59PM +0100, Russell King (Oracle) wrote:
> On Mon, Sep 12, 2022 at 11:41:18AM +0000, Vladimir Oltean wrote:
> > On Mon, Sep 12, 2022 at 01:16:21PM +0300, Vladimir Oltean wrote:
> > > > Therefore, I think you can drop this patch from your series and
> > > > you won't see any functional change.
> > > 
> > > This is true. I am also a bit surprised at Colin's choices to
> > > (b) split the work he submitted such that he populates mac_capabilities
> > >     but does not make any use of it (not call phylink_generic_validate
> > >     from anywhere). We try as much as possible to not leave dead code
> > >     behind in the mainline tree, even if future work is intended to
> > >     bring it to life. I do understand that this is an RFC so the patches
> > >     weren't intended to be applied as is, but it is still confusing to
> > >     review a change which, as you've correctly pointed out, has no
> > >     effect to the git tree as it stands.
> > 
> > Ah, I retract this comment; after actually looking at all the patches, I
> > do see that in patch 8/8, Colin does call phylink_generic_validate().
> 
> Good point, I obviously missed that in the series.

Whew... I was getting confused as I was reading this. Double checking
that I did indeed add this to the set.

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

Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Colin Foster]

On Mon, Sep 12, 2022 at 10:16:21AM +0000, Vladimir Oltean wrote:
> On Mon, Sep 12, 2022 at 09:48:36AM +0100, Russell King (Oracle) wrote:
> > On Sun, Sep 11, 2022 at 01:02:42PM -0700, Colin Foster wrote:
> > > phylink_generic_validate() requires that mac_capabilities is correctly
> > > populated. While no existing drivers have used phylink_generic_validate(),
> > > the ocelot_ext.c driver will. Populate this element so the use of existing
> > > functions is possible.
> > 
> > Ocelot always fills in the .phylink_validate method in struct
> > dsa_switch_ops, mac_capabilities won't be used as
> > phylink_generic_validate() will not be called by
> > dsa_port_phylink_validate().
> 
> Correct, but felix_phylink_validate() _can_ still directly call
> phylink_validate(), right? Colin does not have the full support for
> ocelot_ext in this patch set, but this is what he intends to do.

As you mentioned, I do in fact call phylink_generic_validate() in 8/8.

> 
> > Also "no existing drivers have used phylink_generic_validate()" I
> > wonder which drivers you are referring to there. If you are referring
> > to DSA drivers, then it is extensively used. The following is from
> > Linus' tree as of today:
> 
> By "existing drivers", it is meant felix_vsc9959.c and seville_vsc9953.c,
> two drivers in their own right, which use the common felix.c to talk to
> (a) DSA and (b) the ocelot switch lib in drivers/net/ethernet/mscc/.
> It is true that these existing drivers do not use phylink_generic_validate().
> Furthermore, Colin's new ocelot_ext.c is on the same level as
> felix_vsc9959.c and seville_vsc9953.c, will use felix.c in the same way,
> and will want to use phylink_generic_validate().
> 
> > Secondly, I don't see a purpose for this patch in the following
> > patches, as Ocelot continues to always fill in .phylink_validate,
> > and as I mentioned above, as long as that member is filled in,
> > mac_capabilities won't be used unless you explicitly call
> > phylink_generic_validate() in your .phylink_validate() callback.
> 
> Yes, explicit calling is what Colin explained that he wants to do.
> 
> > Therefore, I think you can drop this patch from your series and
> > you won't see any functional change.
> 
> This is true. I am also a bit surprised at Colin's choices to
> (a) not ask the netdev maintainers to pull into net-next the immutable
>     branch that Lee provided here:
>     https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/
>     and instead send some patches for review which are difficult to
>     apply directly to any tree

As mentioned in the cover letter, I don't expect this to necessarily be
ready by the next merge window. But seemingly I misjudged whether
merging the net-next and Lee's tree would be more tedious for the netdev
maintainers than looking at the RFC for reviewers. I'm trying to create
as little hassle for people as I can. Apologies.

Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Vladimir Oltean]

On Mon, Sep 12, 2022 at 08:47:47AM -0700, Colin Foster wrote:
> > This is true. I am also a bit surprised at Colin's choices to
> > (a) not ask the netdev maintainers to pull into net-next the immutable
> >     branch that Lee provided here:
> >     https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/
> >     and instead send some patches for review which are difficult to
> >     apply directly to any tree
> 
> As mentioned in the cover letter, I don't expect this to necessarily be
> ready by the next merge window. But seemingly I misjudged whether
> merging the net-next and Lee's tree would be more tedious for the netdev
> maintainers than looking at the RFC for reviewers. I'm trying to create
> as little hassle for people as I can. Apologies.

What is it exactly that is keeping this patch set from being ready for 6.1?
There's still time...

It mostly looks ok to me, I'm in the process of reviewing it. You
mentioned documentation in the cover letter; I suppose you're talking
about dt-schema? If so, I just started off by converting ocelot.txt to
mscc,ocelot.yaml, since I know that the conversion process is typically
a bit daunting to even start.
https://patchwork.kernel.org/project/netdevbpf/patch/20220912153702.246206-1-vladimir.oltean@nxp.com/

Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

[Colin Foster]

On Mon, Sep 12, 2022 at 03:52:35PM +0000, Vladimir Oltean wrote:
> On Mon, Sep 12, 2022 at 08:47:47AM -0700, Colin Foster wrote:
> > > This is true. I am also a bit surprised at Colin's choices to
> > > (a) not ask the netdev maintainers to pull into net-next the immutable
> > >     branch that Lee provided here:
> > >     https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/
> > >     and instead send some patches for review which are difficult to
> > >     apply directly to any tree
> > 
> > As mentioned in the cover letter, I don't expect this to necessarily be
> > ready by the next merge window. But seemingly I misjudged whether
> > merging the net-next and Lee's tree would be more tedious for the netdev
> > maintainers than looking at the RFC for reviewers. I'm trying to create
> > as little hassle for people as I can. Apologies.
> 
> What is it exactly that is keeping this patch set from being ready for 6.1?
> There's still time...
> 
> It mostly looks ok to me, I'm in the process of reviewing it. You
> mentioned documentation in the cover letter; I suppose you're talking
> about dt-schema? If so, I just started off by converting ocelot.txt to
> mscc,ocelot.yaml, since I know that the conversion process is typically
> a bit daunting to even start.
> https://patchwork.kernel.org/project/netdevbpf/patch/20220912153702.246206-1-vladimir.oltean@nxp.com/

Yes - checkpatch correclty gave warnings about mscc,vsc7512-ext-switch being
undocumented. Thanks for that patch - I just saw it! I'll wait for your
review before I get optimistic. But if it boils down to separating the
last patch (per Lee's suggestion) and adding the dt-bindings, maybe it
could be ready in another round or two.

[RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext

[Colin Foster]

The Ocelot switch core driver relies heavily on a fixed array of resources
for both ports and peripherals. This is in contrast to existing peripherals
- pinctrl for example - which have a one-to-one mapping of driver <>
resource. As such, these regmaps must be created differently so that
enumeration-based offsets are preserved.

Register the regmaps to the core MFD device unconditionally so they can be
referenced by the Ocelot switch / Felix DSA systems.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

v1 from previous RFC:
    * New patch

---
 drivers/mfd/ocelot-core.c  | 88 +++++++++++++++++++++++++++++++++++---
 include/linux/mfd/ocelot.h |  5 +++
 2 files changed, 88 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 1816d52c65c5..aa7fa21b354c 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -34,16 +34,55 @@
 
 #define VSC7512_MIIM0_RES_START		0x7107009c
 #define VSC7512_MIIM1_RES_START		0x710700c0
-#define VSC7512_MIIM_RES_SIZE		0x024
+#define VSC7512_MIIM_RES_SIZE		0x00000024
 
 #define VSC7512_PHY_RES_START		0x710700f0
-#define VSC7512_PHY_RES_SIZE		0x004
+#define VSC7512_PHY_RES_SIZE		0x00000004
 
 #define VSC7512_GPIO_RES_START		0x71070034
-#define VSC7512_GPIO_RES_SIZE		0x06c
+#define VSC7512_GPIO_RES_SIZE		0x0000006c
 
 #define VSC7512_SIO_CTRL_RES_START	0x710700f8
-#define VSC7512_SIO_CTRL_RES_SIZE	0x100
+#define VSC7512_SIO_CTRL_RES_SIZE	0x00000100
+
+#define VSC7512_HSIO_RES_START		0x710d0000
+#define VSC7512_HSIO_RES_SIZE		0x00000128
+
+#define VSC7512_ANA_RES_START		0x71880000
+#define VSC7512_ANA_RES_SIZE		0x00010000
+
+#define VSC7512_QS_RES_START		0x71080000
+#define VSC7512_QS_RES_SIZE		0x00000100
+
+#define VSC7512_QSYS_RES_START		0x71800000
+#define VSC7512_QSYS_RES_SIZE		0x00200000
+
+#define VSC7512_REW_RES_START		0x71030000
+#define VSC7512_REW_RES_SIZE		0x00010000
+
+#define VSC7512_SYS_RES_START		0x71010000
+#define VSC7512_SYS_RES_SIZE		0x00010000
+
+#define VSC7512_S0_RES_START		0x71040000
+#define VSC7512_S1_RES_START		0x71050000
+#define VSC7512_S2_RES_START		0x71060000
+#define VSC7512_S_RES_SIZE		0x00000400
+
+#define VSC7512_GCB_RES_START		0x71070000
+#define VSC7512_GCB_RES_SIZE		0x0000022c
+
+#define VSC7512_PORT_0_RES_START	0x711e0000
+#define VSC7512_PORT_1_RES_START	0x711f0000
+#define VSC7512_PORT_2_RES_START	0x71200000
+#define VSC7512_PORT_3_RES_START	0x71210000
+#define VSC7512_PORT_4_RES_START	0x71220000
+#define VSC7512_PORT_5_RES_START	0x71230000
+#define VSC7512_PORT_6_RES_START	0x71240000
+#define VSC7512_PORT_7_RES_START	0x71250000
+#define VSC7512_PORT_8_RES_START	0x71260000
+#define VSC7512_PORT_9_RES_START	0x71270000
+#define VSC7512_PORT_10_RES_START	0x71280000
+#define VSC7512_PORT_RES_SIZE		0x00010000
 
 #define VSC7512_GCB_RST_SLEEP_US	100
 #define VSC7512_GCB_RST_TIMEOUT_US	100000
@@ -96,6 +135,34 @@ static const struct resource vsc7512_sgpio_resources[] = {
 	DEFINE_RES_REG_NAMED(VSC7512_SIO_CTRL_RES_START, VSC7512_SIO_CTRL_RES_SIZE, "gcb_sio"),
 };
 
+const struct resource vsc7512_target_io_res[TARGET_MAX] = {
+	[ANA] = DEFINE_RES_REG_NAMED(VSC7512_ANA_RES_START, VSC7512_ANA_RES_SIZE, "ana"),
+	[QS] = DEFINE_RES_REG_NAMED(VSC7512_QS_RES_START, VSC7512_QS_RES_SIZE, "qs"),
+	[QSYS] = DEFINE_RES_REG_NAMED(VSC7512_QSYS_RES_START, VSC7512_QSYS_RES_SIZE, "qsys"),
+	[REW] = DEFINE_RES_REG_NAMED(VSC7512_REW_RES_START, VSC7512_REW_RES_SIZE, "rew"),
+	[SYS] = DEFINE_RES_REG_NAMED(VSC7512_SYS_RES_START, VSC7512_SYS_RES_SIZE, "sys"),
+	[S0] = DEFINE_RES_REG_NAMED(VSC7512_S0_RES_START, VSC7512_S_RES_SIZE, "s0"),
+	[S1] = DEFINE_RES_REG_NAMED(VSC7512_S1_RES_START, VSC7512_S_RES_SIZE, "s1"),
+	[S2] = DEFINE_RES_REG_NAMED(VSC7512_S2_RES_START, VSC7512_S_RES_SIZE, "s2"),
+	[GCB] = DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE, "devcpu_gcb"),
+	[HSIO] = DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE, "hsio"),
+};
+
+const struct resource vsc7512_port_io_res[] = {
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_0_RES_START, VSC7512_PORT_RES_SIZE, "port0"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_1_RES_START, VSC7512_PORT_RES_SIZE, "port1"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_2_RES_START, VSC7512_PORT_RES_SIZE, "port2"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_3_RES_START, VSC7512_PORT_RES_SIZE, "port3"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_4_RES_START, VSC7512_PORT_RES_SIZE, "port4"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_5_RES_START, VSC7512_PORT_RES_SIZE, "port5"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_6_RES_START, VSC7512_PORT_RES_SIZE, "port6"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_7_RES_START, VSC7512_PORT_RES_SIZE, "port7"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_8_RES_START, VSC7512_PORT_RES_SIZE, "port8"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_9_RES_START, VSC7512_PORT_RES_SIZE, "port9"),
+	DEFINE_RES_REG_NAMED(VSC7512_PORT_10_RES_START, VSC7512_PORT_RES_SIZE, "port10"),
+	{}
+};
+
 static const struct mfd_cell vsc7512_devs[] = {
 	{
 		.name = "ocelot-pinctrl",
@@ -127,7 +194,7 @@ static const struct mfd_cell vsc7512_devs[] = {
 static void ocelot_core_try_add_regmap(struct device *dev,
 				       const struct resource *res)
 {
-	if (dev_get_regmap(dev, res->name))
+	if (!res->start || dev_get_regmap(dev, res->name))
 		return;
 
 	ocelot_spi_init_regmap(dev, res);
@@ -144,6 +211,7 @@ static void ocelot_core_try_add_regmaps(struct device *dev,
 
 int ocelot_core_init(struct device *dev)
 {
+	const struct resource *port_res;
 	int i, ndevs;
 
 	ndevs = ARRAY_SIZE(vsc7512_devs);
@@ -151,6 +219,16 @@ int ocelot_core_init(struct device *dev)
 	for (i = 0; i < ndevs; i++)
 		ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
 
+	/*
+	 * Both the target_io_res and tbe port_io_res structs need to be referenced directly by
+	 * the ocelot_ext driver, so they can't be attached to the dev directly
+	 */
+	for (i = 0; i < TARGET_MAX; i++)
+		ocelot_core_try_add_regmap(dev, &vsc7512_target_io_res[i]);
+
+	for (port_res = vsc7512_port_io_res; port_res->start; port_res++)
+		ocelot_core_try_add_regmap(dev, port_res);
+
 	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL);
 }
 EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);
diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h
index dd72073d2d4f..439ff5256cf0 100644
--- a/include/linux/mfd/ocelot.h
+++ b/include/linux/mfd/ocelot.h
@@ -11,8 +11,13 @@
 #include <linux/regmap.h>
 #include <linux/types.h>
 
+#include <soc/mscc/ocelot.h>
+
 struct resource;
 
+extern const struct resource vsc7512_target_io_res[TARGET_MAX];
+extern const struct resource vsc7512_port_io_res[];
+
 static inline struct regmap *
 ocelot_regmap_from_resource_optional(struct platform_device *pdev,
 				     unsigned int index,
-- 
2.25.1

Re: [RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext

[Vladimir Oltean]

On Sun, Sep 11, 2022 at 01:02:43PM -0700, Colin Foster wrote:
> The Ocelot switch core driver relies heavily on a fixed array of resources
> for both ports and peripherals. This is in contrast to existing peripherals
> - pinctrl for example - which have a one-to-one mapping of driver <>
> resource. As such, these regmaps must be created differently so that
> enumeration-based offsets are preserved.
> 
> Register the regmaps to the core MFD device unconditionally so they can be
> referenced by the Ocelot switch / Felix DSA systems.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
> 
> v1 from previous RFC:
>     * New patch
> 
> ---
>  drivers/mfd/ocelot-core.c  | 88 +++++++++++++++++++++++++++++++++++---
>  include/linux/mfd/ocelot.h |  5 +++
>  2 files changed, 88 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index 1816d52c65c5..aa7fa21b354c 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -34,16 +34,55 @@
>  
>  #define VSC7512_MIIM0_RES_START		0x7107009c
>  #define VSC7512_MIIM1_RES_START		0x710700c0
> -#define VSC7512_MIIM_RES_SIZE		0x024
> +#define VSC7512_MIIM_RES_SIZE		0x00000024
>  
>  #define VSC7512_PHY_RES_START		0x710700f0
> -#define VSC7512_PHY_RES_SIZE		0x004
> +#define VSC7512_PHY_RES_SIZE		0x00000004
>  
>  #define VSC7512_GPIO_RES_START		0x71070034
> -#define VSC7512_GPIO_RES_SIZE		0x06c
> +#define VSC7512_GPIO_RES_SIZE		0x0000006c
>  
>  #define VSC7512_SIO_CTRL_RES_START	0x710700f8
> -#define VSC7512_SIO_CTRL_RES_SIZE	0x100
> +#define VSC7512_SIO_CTRL_RES_SIZE	0x00000100

Split the gratuitous changes to _RES_SIZE to a separate patch please, as
they're just noise here?

> +
> +#define VSC7512_HSIO_RES_START		0x710d0000
> +#define VSC7512_HSIO_RES_SIZE		0x00000128
> +
> +#define VSC7512_ANA_RES_START		0x71880000
> +#define VSC7512_ANA_RES_SIZE		0x00010000
> +
> +#define VSC7512_QS_RES_START		0x71080000
> +#define VSC7512_QS_RES_SIZE		0x00000100
> +
> +#define VSC7512_QSYS_RES_START		0x71800000
> +#define VSC7512_QSYS_RES_SIZE		0x00200000
> +
> +#define VSC7512_REW_RES_START		0x71030000
> +#define VSC7512_REW_RES_SIZE		0x00010000
> +
> +#define VSC7512_SYS_RES_START		0x71010000
> +#define VSC7512_SYS_RES_SIZE		0x00010000
> +
> +#define VSC7512_S0_RES_START		0x71040000
> +#define VSC7512_S1_RES_START		0x71050000
> +#define VSC7512_S2_RES_START		0x71060000
> +#define VSC7512_S_RES_SIZE		0x00000400
> +
> +#define VSC7512_GCB_RES_START		0x71070000
> +#define VSC7512_GCB_RES_SIZE		0x0000022c
> +
> +#define VSC7512_PORT_0_RES_START	0x711e0000
> +#define VSC7512_PORT_1_RES_START	0x711f0000
> +#define VSC7512_PORT_2_RES_START	0x71200000
> +#define VSC7512_PORT_3_RES_START	0x71210000
> +#define VSC7512_PORT_4_RES_START	0x71220000
> +#define VSC7512_PORT_5_RES_START	0x71230000
> +#define VSC7512_PORT_6_RES_START	0x71240000
> +#define VSC7512_PORT_7_RES_START	0x71250000
> +#define VSC7512_PORT_8_RES_START	0x71260000
> +#define VSC7512_PORT_9_RES_START	0x71270000
> +#define VSC7512_PORT_10_RES_START	0x71280000
> +#define VSC7512_PORT_RES_SIZE		0x00010000
>  
>  #define VSC7512_GCB_RST_SLEEP_US	100
>  #define VSC7512_GCB_RST_TIMEOUT_US	100000
> @@ -96,6 +135,34 @@ static const struct resource vsc7512_sgpio_resources[] = {
>  	DEFINE_RES_REG_NAMED(VSC7512_SIO_CTRL_RES_START, VSC7512_SIO_CTRL_RES_SIZE, "gcb_sio"),
>  };
>  
> +const struct resource vsc7512_target_io_res[TARGET_MAX] = {
> +	[ANA] = DEFINE_RES_REG_NAMED(VSC7512_ANA_RES_START, VSC7512_ANA_RES_SIZE, "ana"),
> +	[QS] = DEFINE_RES_REG_NAMED(VSC7512_QS_RES_START, VSC7512_QS_RES_SIZE, "qs"),
> +	[QSYS] = DEFINE_RES_REG_NAMED(VSC7512_QSYS_RES_START, VSC7512_QSYS_RES_SIZE, "qsys"),
> +	[REW] = DEFINE_RES_REG_NAMED(VSC7512_REW_RES_START, VSC7512_REW_RES_SIZE, "rew"),
> +	[SYS] = DEFINE_RES_REG_NAMED(VSC7512_SYS_RES_START, VSC7512_SYS_RES_SIZE, "sys"),
> +	[S0] = DEFINE_RES_REG_NAMED(VSC7512_S0_RES_START, VSC7512_S_RES_SIZE, "s0"),
> +	[S1] = DEFINE_RES_REG_NAMED(VSC7512_S1_RES_START, VSC7512_S_RES_SIZE, "s1"),
> +	[S2] = DEFINE_RES_REG_NAMED(VSC7512_S2_RES_START, VSC7512_S_RES_SIZE, "s2"),
> +	[GCB] = DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE, "devcpu_gcb"),
> +	[HSIO] = DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE, "hsio"),
> +};

EXPORT_SYMBOL is required, I believe, for when ocelot_ext is built as
module?

> +
> +const struct resource vsc7512_port_io_res[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_0_RES_START, VSC7512_PORT_RES_SIZE, "port0"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_1_RES_START, VSC7512_PORT_RES_SIZE, "port1"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_2_RES_START, VSC7512_PORT_RES_SIZE, "port2"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_3_RES_START, VSC7512_PORT_RES_SIZE, "port3"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_4_RES_START, VSC7512_PORT_RES_SIZE, "port4"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_5_RES_START, VSC7512_PORT_RES_SIZE, "port5"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_6_RES_START, VSC7512_PORT_RES_SIZE, "port6"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_7_RES_START, VSC7512_PORT_RES_SIZE, "port7"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_8_RES_START, VSC7512_PORT_RES_SIZE, "port8"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_9_RES_START, VSC7512_PORT_RES_SIZE, "port9"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_10_RES_START, VSC7512_PORT_RES_SIZE, "port10"),
> +	{}
> +};

Here too.

> +
>  static const struct mfd_cell vsc7512_devs[] = {
>  	{
>  		.name = "ocelot-pinctrl",
> @@ -127,7 +194,7 @@ static const struct mfd_cell vsc7512_devs[] = {
>  static void ocelot_core_try_add_regmap(struct device *dev,
>  				       const struct resource *res)
>  {
> -	if (dev_get_regmap(dev, res->name))
> +	if (!res->start || dev_get_regmap(dev, res->name))

I didn't understand at first what this extra condition here is for.
I don't think that adding this extra condition here is the clearest
way to deal with the sparsity of the vsc7512_target_io_res[] array, plus
it seems to indicate the masking of a more unclean code design.

I would propose an alternative below, at the caller site....

>  		return;
>  
>  	ocelot_spi_init_regmap(dev, res);
> @@ -144,6 +211,7 @@ static void ocelot_core_try_add_regmaps(struct device *dev,
>  
>  int ocelot_core_init(struct device *dev)
>  {
> +	const struct resource *port_res;
>  	int i, ndevs;
>  
>  	ndevs = ARRAY_SIZE(vsc7512_devs);
> @@ -151,6 +219,16 @@ int ocelot_core_init(struct device *dev)
>  	for (i = 0; i < ndevs; i++)
>  		ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
>  
> +	/*
> +	 * Both the target_io_res and tbe port_io_res structs need to be referenced directly by

s/tbe/the

> +	 * the ocelot_ext driver, so they can't be attached to the dev directly

I don't exactly understand the meaning of "they can't be attached to the
dev *directly*". You mean that the "struct mfd_cell vsc7512_devs[]" entry
for "mscc,vsc7512-ext-switch" will not have a "resources" property, right?
Better to say "using mfd_add_devices()" rather than "directly"?

> +	 */
> +	for (i = 0; i < TARGET_MAX; i++)
> +		ocelot_core_try_add_regmap(dev, &vsc7512_target_io_res[i]);

	/*
	 * vsc7512_target_io_res[] is a sparse array, skip the missing
	 * elements
	 */
	for (i = 0; i < TARGET_MAX; i++) {
		res = &vsc7512_target_io_res[i];
		if (!res->start)
			continue;

		ocelot_core_try_add_regmap(dev, res);
	}

Something interesting that I stumbled upon in Documentation/process/6.Followthrough.rst
was:

| Andrew Morton has suggested that every review comment which does not result
| in a code change should result in an additional code comment instead; that
| can help future reviewers avoid the questions which came up the first time
| around.

so if you don't like my alternative, please at least do add a comment in
ocelot_core_try_add_regmap().

> +
> +	for (port_res = vsc7512_port_io_res; port_res->start; port_res++)
> +		ocelot_core_try_add_regmap(dev, port_res);
> +
>  	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL);
>  }
>  EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);
> diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h
> index dd72073d2d4f..439ff5256cf0 100644
> --- a/include/linux/mfd/ocelot.h
> +++ b/include/linux/mfd/ocelot.h
> @@ -11,8 +11,13 @@
>  #include <linux/regmap.h>
>  #include <linux/types.h>
>  
> +#include <soc/mscc/ocelot.h>
> +
>  struct resource;
>  
> +extern const struct resource vsc7512_target_io_res[TARGET_MAX];
> +extern const struct resource vsc7512_port_io_res[];
> +
>  static inline struct regmap *
>  ocelot_regmap_from_resource_optional(struct platform_device *pdev,
>  				     unsigned int index,
> -- 
> 2.25.1
>

Actually I don't like this mechanism too much, if at all. I have 4 mutt
windows open right now, plus the previous mfd patch at:
https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=ib-mfd-net-pinctrl-6.0&id=f3e893626abeac3cdd9ba41d3395dc6c1b7d5ad6
to follow what is going on. So I'll copy some code from other places
here, to concentrate the discussion in a single place:

From patch 8/8:

> +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
> +					     struct resource *res)
> +{
> +	return dev_get_regmap(ocelot->dev->parent, res->name);
> +}

> +static const struct felix_info vsc7512_info = {
> +	.target_io_res			= vsc7512_target_io_res, // exported by drivers/mfd/ocelot-core.c
> +	.port_io_res			= vsc7512_port_io_res, // exported by drivers/mfd/ocelot-core.c
> +	.init_regmap			= ocelot_ext_regmap_init,
> +};

From drivers/net/dsa/felix.c:

static int felix_init_structs(struct felix *felix, int num_phys_ports)
{
	for (i = 0; i < TARGET_MAX; i++) {
		struct regmap *target;

		if (!felix->info->target_io_res[i].name)
			continue;

		memcpy(&res, &felix->info->target_io_res[i], sizeof(res));
		res.flags = IORESOURCE_MEM;
		res.start += felix->switch_base;
		res.end += felix->switch_base;

		target = felix->info->init_regmap(ocelot, &res);
		if (IS_ERR(target)) {
			dev_err(ocelot->dev,
				"Failed to map device memory space\n");
			kfree(port_phy_modes);
			return PTR_ERR(target);
		}

		ocelot->targets[i] = target;
	}
}

So here's what I don't like. You export the resources from ocelot-mfd to
DSA, to get just their *string* names. Then you let the common code
create some bogus res.start and res.end in felix_init_structs(), which
you discard in felix->info->init_regmap() - ocelot_ext_regmap_init(),
and use just the name. You even discard the IORESOURCE_MEM flag, because
what you get back are IORESOURCE_REG resources. This is all very confusing.

So you need to retrieve a regmap for each ocelot target that you can.
Why don't you make it, via mfd_add_devices() and the "resources" array
of struct mfd_cell (i.e. the same mechanism as for every other peripheral),
such that the resources used by the DSA device have an index determined
by i = 0; i < TARGET_MAX; i++; platform_get_resource(dev, i, IORESOURCE_REG)?
This way, DSA needs to know no more than the index of the resource it
asks for.

[ yes, you'll need to revert your own commit 242bd0c10bbd ("net: dsa:
  ocelot: felix: add interface for custom regmaps"), which I asked you
  about if you're sure if this is the final way in which DSA will get
  its regmaps. Then you'll need to provide a different felix->info
  operation, such as felix->info->regmap_from_mfd() or something, where
  just the index is provided. If that isn't provided by the switch, we
  "fall back" to the code that exists right now, which, when reverted,
  does create an actual resource, and directly calls ocelot_regmap_init()
  on it, to create an MMIO regmap from it ]

Re: [RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext

[Colin Foster]

On Mon, Sep 12, 2022 at 05:08:09PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:43PM -0700, Colin Foster wrote:
> > The Ocelot switch core driver relies heavily on a fixed array of resources
> > for both ports and peripherals. This is in contrast to existing peripherals
> > - pinctrl for example - which have a one-to-one mapping of driver <>
> > resource. As such, these regmaps must be created differently so that
> > enumeration-based offsets are preserved.
> > 
> > Register the regmaps to the core MFD device unconditionally so they can be
> > referenced by the Ocelot switch / Felix DSA systems.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> > 
> > v1 from previous RFC:
> >     * New patch
> > 
> > ---
> >  drivers/mfd/ocelot-core.c  | 88 +++++++++++++++++++++++++++++++++++---
> >  include/linux/mfd/ocelot.h |  5 +++
> >  2 files changed, 88 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > index 1816d52c65c5..aa7fa21b354c 100644
> > --- a/drivers/mfd/ocelot-core.c
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -34,16 +34,55 @@
> >  
> >  #define VSC7512_MIIM0_RES_START		0x7107009c
> >  #define VSC7512_MIIM1_RES_START		0x710700c0
> > -#define VSC7512_MIIM_RES_SIZE		0x024
> > +#define VSC7512_MIIM_RES_SIZE		0x00000024
> >  
> >  #define VSC7512_PHY_RES_START		0x710700f0
> > -#define VSC7512_PHY_RES_SIZE		0x004
> > +#define VSC7512_PHY_RES_SIZE		0x00000004
> >  
> >  #define VSC7512_GPIO_RES_START		0x71070034
> > -#define VSC7512_GPIO_RES_SIZE		0x06c
> > +#define VSC7512_GPIO_RES_SIZE		0x0000006c
> >  
> >  #define VSC7512_SIO_CTRL_RES_START	0x710700f8
> > -#define VSC7512_SIO_CTRL_RES_SIZE	0x100
> > +#define VSC7512_SIO_CTRL_RES_SIZE	0x00000100
> 
> Split the gratuitous changes to _RES_SIZE to a separate patch please, as
> they're just noise here?

Will do.

> > +const struct resource vsc7512_target_io_res[TARGET_MAX] = {
> > +	[ANA] = DEFINE_RES_REG_NAMED(VSC7512_ANA_RES_START, VSC7512_ANA_RES_SIZE, "ana"),
> > +	[QS] = DEFINE_RES_REG_NAMED(VSC7512_QS_RES_START, VSC7512_QS_RES_SIZE, "qs"),
> > +	[QSYS] = DEFINE_RES_REG_NAMED(VSC7512_QSYS_RES_START, VSC7512_QSYS_RES_SIZE, "qsys"),
> > +	[REW] = DEFINE_RES_REG_NAMED(VSC7512_REW_RES_START, VSC7512_REW_RES_SIZE, "rew"),
> > +	[SYS] = DEFINE_RES_REG_NAMED(VSC7512_SYS_RES_START, VSC7512_SYS_RES_SIZE, "sys"),
> > +	[S0] = DEFINE_RES_REG_NAMED(VSC7512_S0_RES_START, VSC7512_S_RES_SIZE, "s0"),
> > +	[S1] = DEFINE_RES_REG_NAMED(VSC7512_S1_RES_START, VSC7512_S_RES_SIZE, "s1"),
> > +	[S2] = DEFINE_RES_REG_NAMED(VSC7512_S2_RES_START, VSC7512_S_RES_SIZE, "s2"),
> > +	[GCB] = DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE, "devcpu_gcb"),
> > +	[HSIO] = DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE, "hsio"),
> > +};
> 
> EXPORT_SYMBOL is required, I believe, for when ocelot_ext is built as
> module?

Agreed on this and the other symbol. Thanks.

> 
> > +
> >  static const struct mfd_cell vsc7512_devs[] = {
> >  	{
> >  		.name = "ocelot-pinctrl",
> > @@ -127,7 +194,7 @@ static const struct mfd_cell vsc7512_devs[] = {
> >  static void ocelot_core_try_add_regmap(struct device *dev,
> >  				       const struct resource *res)
> >  {
> > -	if (dev_get_regmap(dev, res->name))
> > +	if (!res->start || dev_get_regmap(dev, res->name))
> 
> I didn't understand at first what this extra condition here is for.
> I don't think that adding this extra condition here is the clearest
> way to deal with the sparsity of the vsc7512_target_io_res[] array, plus
> it seems to indicate the masking of a more unclean code design.

Yes, it was a way to deal with this struct. I see that I should have at
least added a comment, but the way you suggest below is cleaner (before
try_add_regmap())

> 
> I would propose an alternative below, at the caller site....
> 
> >  		return;
> >  
> >  	ocelot_spi_init_regmap(dev, res);
> > @@ -144,6 +211,7 @@ static void ocelot_core_try_add_regmaps(struct device *dev,
> >  
> >  int ocelot_core_init(struct device *dev)
> >  {
> > +	const struct resource *port_res;
> >  	int i, ndevs;
> >  
> >  	ndevs = ARRAY_SIZE(vsc7512_devs);
> > @@ -151,6 +219,16 @@ int ocelot_core_init(struct device *dev)
> >  	for (i = 0; i < ndevs; i++)
> >  		ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
> >  
> > +	/*
> > +	 * Both the target_io_res and tbe port_io_res structs need to be referenced directly by
> 
> s/tbe/the
> 
> > +	 * the ocelot_ext driver, so they can't be attached to the dev directly
> 
> I don't exactly understand the meaning of "they can't be attached to the
> dev *directly*". You mean that the "struct mfd_cell vsc7512_devs[]" entry
> for "mscc,vsc7512-ext-switch" will not have a "resources" property, right?
> Better to say "using mfd_add_devices()" rather than "directly"?

I'll reword the comment - but I think it might go away entirely with
what you're suggesting below.

> 
> > +	 */
> > +	for (i = 0; i < TARGET_MAX; i++)
> > +		ocelot_core_try_add_regmap(dev, &vsc7512_target_io_res[i]);
> 
> 	/*
> 	 * vsc7512_target_io_res[] is a sparse array, skip the missing
> 	 * elements
> 	 */
> 	for (i = 0; i < TARGET_MAX; i++) {
> 		res = &vsc7512_target_io_res[i];
> 		if (!res->start)
> 			continue;
> 
> 		ocelot_core_try_add_regmap(dev, res);
> 	}
> 
> Something interesting that I stumbled upon in Documentation/process/6.Followthrough.rst
> was:
> 
> | Andrew Morton has suggested that every review comment which does not result
> | in a code change should result in an additional code comment instead; that
> | can help future reviewers avoid the questions which came up the first time
> | around.
> 
> so if you don't like my alternative, please at least do add a comment in
> ocelot_core_try_add_regmap().

I'm due for another re-read of this documentation. That's a very practical
suggestion.

> 
> > +
> > +	for (port_res = vsc7512_port_io_res; port_res->start; port_res++)
> > +		ocelot_core_try_add_regmap(dev, port_res);
> > +
> >  	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL);
> >  }
> >  EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);
> > diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h
> > index dd72073d2d4f..439ff5256cf0 100644
> > --- a/include/linux/mfd/ocelot.h
> > +++ b/include/linux/mfd/ocelot.h
> > @@ -11,8 +11,13 @@
> >  #include <linux/regmap.h>
> >  #include <linux/types.h>
> >  
> > +#include <soc/mscc/ocelot.h>
> > +
> >  struct resource;
> >  
> > +extern const struct resource vsc7512_target_io_res[TARGET_MAX];
> > +extern const struct resource vsc7512_port_io_res[];
> > +
> >  static inline struct regmap *
> >  ocelot_regmap_from_resource_optional(struct platform_device *pdev,
> >  				     unsigned int index,
> > -- 
> > 2.25.1
> >
> 
> Actually I don't like this mechanism too much, if at all. I have 4 mutt
> windows open right now, plus the previous mfd patch at:
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=ib-mfd-net-pinctrl-6.0&id=f3e893626abeac3cdd9ba41d3395dc6c1b7d5ad6
> to follow what is going on. So I'll copy some code from other places
> here, to concentrate the discussion in a single place:
> 
> From patch 8/8:
> 
> > +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
> > +					     struct resource *res)
> > +{
> > +	return dev_get_regmap(ocelot->dev->parent, res->name);
> > +}
> 
> > +static const struct felix_info vsc7512_info = {
> > +	.target_io_res			= vsc7512_target_io_res, // exported by drivers/mfd/ocelot-core.c
> > +	.port_io_res			= vsc7512_port_io_res, // exported by drivers/mfd/ocelot-core.c
> > +	.init_regmap			= ocelot_ext_regmap_init,
> > +};
> 
> From drivers/net/dsa/felix.c:
> 
> static int felix_init_structs(struct felix *felix, int num_phys_ports)
> {
> 	for (i = 0; i < TARGET_MAX; i++) {
> 		struct regmap *target;
> 
> 		if (!felix->info->target_io_res[i].name)
> 			continue;
> 
> 		memcpy(&res, &felix->info->target_io_res[i], sizeof(res));
> 		res.flags = IORESOURCE_MEM;
> 		res.start += felix->switch_base;
> 		res.end += felix->switch_base;
> 
> 		target = felix->info->init_regmap(ocelot, &res);
> 		if (IS_ERR(target)) {
> 			dev_err(ocelot->dev,
> 				"Failed to map device memory space\n");
> 			kfree(port_phy_modes);
> 			return PTR_ERR(target);
> 		}
> 
> 		ocelot->targets[i] = target;
> 	}
> }
> 
> So here's what I don't like. You export the resources from ocelot-mfd to
> DSA, to get just their *string* names. Then you let the common code
> create some bogus res.start and res.end in felix_init_structs(), which
> you discard in felix->info->init_regmap() - ocelot_ext_regmap_init(),
> and use just the name. You even discard the IORESOURCE_MEM flag, because
> what you get back are IORESOURCE_REG resources. This is all very confusing.
> 
> So you need to retrieve a regmap for each ocelot target that you can.
> Why don't you make it, via mfd_add_devices() and the "resources" array
> of struct mfd_cell (i.e. the same mechanism as for every other peripheral),
> such that the resources used by the DSA device have an index determined
> by i = 0; i < TARGET_MAX; i++; platform_get_resource(dev, i, IORESOURCE_REG)?
> This way, DSA needs to know no more than the index of the resource it
> asks for.

That is exactly right. The ocelot_ext version of init_regmap() now uses
dev_get_regmap() which only cares about the name and essentially drops
the rest of the information. Previous versions hooked into the
ocelot-core / ocelot-spi MFD system to request that a new regmap be
created (with start and end being honored.) A benefit of this design is
that any regmaps that are named the same are automatically shared. A
drawback (or maybe a benefit?) is that the users have no control over
ranges / flags.

I think if this goes the way of index-based that'll work. I'm happy to
revert my previous change (sorry it snuck in) but it seems like there'll
still have to be some trickery... For reference:

enum ocelot_target {
	ANA = 1,
	QS,
	QSYS,
	REW,
	SYS,
	S0,
	S1,
	S2,
	HSIO,
	PTP,
	FDMA,
	GCB,
	DEV_GMII,
	TARGET_MAX,
};

mfd_add_devices will probably need to add a zero-size resource for HSIO,
PTP, FDMA, and anything else that might come along in the future. At
this point, regmap_from_mfd(PTP) might have to look like (pseudocode):

struct regmap *ocelot_ext_regmap_from_mfd(struct ocelot *ocelot, int index)
{
    return ocelot_regmap_from_resource_optional(pdev, index-1, config);

    /* Note this essentially expands to:
     * res = platform_get_resource(pdev, IORESOURCE_REG, index-1);
     * return dev_get_regmap(pdev->dev.parent, res->name);
     *
     * This essentially throws away everything that my current
     * implementation does, except for the IORESOURCE_REG flag
     */
}

Then drivers/net/dsa/felix.c felix_init_structs() would have two loops
(again, just as an example)

for (i = ANA; i < TARGET_MAX; i++) {
    if (felix->info->regmap_from_mfd)
        target = felix->info->regmap_from_mfd(ocelot, i);
    else {
        /* existing logic back to ocelot_regmap_init() */
    }
}

for (port = 0; port < num_phys_ports; port++) {
    ...
    if (felix->info->regmap_from_mfd)
        target = felix->info->regmap_from_mfd(ocelot, TARGET_MAX + port);
    else {
        /* existing logic back to ocelot_regmap_init() */
    }
}

And lastly, ocelot_core_init() in drivers/mfd/ocelot-core.c would need a
mechanism to say "don't add a regmap for cell->resources[PTP], even
though that resource exists" because... well I suppose it is only in
drivers/net/ethernet/mscc/ocelot_vsc7514.c for now, but the existance of
those regmaps invokes different behavior. For instance:

	if (ocelot->targets[FDMA])
		ocelot_fdma_init(pdev, ocelot);

I'm not sure whether this last point will have an effect on felix.c in
the end. My current patch set of adding the SERDES ports uses the
existance of targets[HSIO] to invoke ocelot_pll5_init() similar to the
ocelot_vsc7514.c FDMA / PTP conditionals, but I'd understand if that
gets rejected outright. That's for another day.



I'm happy to make these changes if you see them valid. I saw the fact
that dev_get_regmap(dev->parent, res->name) could be used directly in
ocelot_ext_regmap_init() as an elegant solution to felix / ocelot-lib /
ocelot-core, but I recognize that the subtle "throw away the
IORESOURCE_MEM flag and res->{start,end} information" isn't ideal.

> 
> [ yes, you'll need to revert your own commit 242bd0c10bbd ("net: dsa:
>   ocelot: felix: add interface for custom regmaps"), which I asked you
>   about if you're sure if this is the final way in which DSA will get
>   its regmaps. Then you'll need to provide a different felix->info
>   operation, such as felix->info->regmap_from_mfd() or something, where
>   just the index is provided. If that isn't provided by the switch, we
>   "fall back" to the code that exists right now, which, when reverted,
>   does create an actual resource, and directly calls ocelot_regmap_init()
>   on it, to create an MMIO regmap from it ]

Re: [RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext

[Vladimir Oltean]

On Mon, Sep 12, 2022 at 12:04:45PM -0700, Colin Foster wrote:
> That is exactly right. The ocelot_ext version of init_regmap() now uses
> dev_get_regmap() which only cares about the name and essentially drops
> the rest of the information. Previous versions hooked into the
> ocelot-core / ocelot-spi MFD system to request that a new regmap be
> created (with start and end being honored.) A benefit of this design is
> that any regmaps that are named the same are automatically shared. A
> drawback (or maybe a benefit?) is that the users have no control over
> ranges / flags.
> 
> I think if this goes the way of index-based that'll work. I'm happy to
> revert my previous change (sorry it snuck in) but it seems like there'll
> still have to be some trickery... For reference:
> 
> enum ocelot_target {
> 	ANA = 1,
> 	QS,
> 	QSYS,
> 	REW,
> 	SYS,
> 	S0,
> 	S1,
> 	S2,
> 	HSIO,
> 	PTP,
> 	FDMA,
> 	GCB,
> 	DEV_GMII,
> 	TARGET_MAX,
> };
> 
> mfd_add_devices will probably need to add a zero-size resource for HSIO,
> PTP, FDMA, and anything else that might come along in the future. At
> this point, regmap_from_mfd(PTP) might have to look like (pseudocode):
> 
> struct regmap *ocelot_ext_regmap_from_mfd(struct ocelot *ocelot, int index)
> {
>     return ocelot_regmap_from_resource_optional(pdev, index-1, config);
> 
>     /* Note this essentially expands to:
>      * res = platform_get_resource(pdev, IORESOURCE_REG, index-1);
>      * return dev_get_regmap(pdev->dev.parent, res->name);
>      *
>      * This essentially throws away everything that my current
>      * implementation does, except for the IORESOURCE_REG flag
>      */
> }
> 
> Then drivers/net/dsa/felix.c felix_init_structs() would have two loops
> (again, just as an example)
> 
> for (i = ANA; i < TARGET_MAX; i++) {
>     if (felix->info->regmap_from_mfd)
>         target = felix->info->regmap_from_mfd(ocelot, i);
>     else {
>         /* existing logic back to ocelot_regmap_init() */
>     }
> }
> 
> for (port = 0; port < num_phys_ports; port++) {
>     ...
>     if (felix->info->regmap_from_mfd)
>         target = felix->info->regmap_from_mfd(ocelot, TARGET_MAX + port);
>     else {
>         /* existing logic back to ocelot_regmap_init() */
>     }
> }
> 
> And lastly, ocelot_core_init() in drivers/mfd/ocelot-core.c would need a
> mechanism to say "don't add a regmap for cell->resources[PTP], even
> though that resource exists" because... well I suppose it is only in
> drivers/net/ethernet/mscc/ocelot_vsc7514.c for now, but the existance of
> those regmaps invokes different behavior. For instance:
> 
> 	if (ocelot->targets[FDMA])
> 		ocelot_fdma_init(pdev, ocelot);
> 
> I'm not sure whether this last point will have an effect on felix.c in
> the end. My current patch set of adding the SERDES ports uses the
> existance of targets[HSIO] to invoke ocelot_pll5_init() similar to the
> ocelot_vsc7514.c FDMA / PTP conditionals, but I'd understand if that
> gets rejected outright. That's for another day.
> 
> 
> 
> I'm happy to make these changes if you see them valid. I saw the fact
> that dev_get_regmap(dev->parent, res->name) could be used directly in
> ocelot_ext_regmap_init() as an elegant solution to felix / ocelot-lib /
> ocelot-core, but I recognize that the subtle "throw away the
> IORESOURCE_MEM flag and res->{start,end} information" isn't ideal.

Thinking some more about it, there will have to be even more trickery.
Say you solve the problem for the global targets, but then what do you
do for the port targets, how do you reference those by index?
TARGET_MAX + port? Hmm, that isn't great either.

What if we meet half way, and you just get the resources from the
platform device by name, instead of by index? You'd have to modify the
regmap creation procedure to look at a predefined array of strings,
containing names of all targets that are mandatory (a la mscc_ocelot_probe),
and match those
(a) iteration over target_io_res and strcmp(), in the case of vsc9959
    and vsc9953
(b) dev_get_regmap() in the case of ocelot_ext

This way there's still no direct communication between ocelot-mfd and
DSA, and I have the feeling that the problems we both mention are
solved. Hope I'm not missing something.

Re: [RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext

[Colin Foster]

On Mon, Sep 12, 2022 at 08:23:21PM +0000, Vladimir Oltean wrote:
> On Mon, Sep 12, 2022 at 12:04:45PM -0700, Colin Foster wrote:
> > That is exactly right. The ocelot_ext version of init_regmap() now uses
> > dev_get_regmap() which only cares about the name and essentially drops
> > the rest of the information. Previous versions hooked into the
> > ocelot-core / ocelot-spi MFD system to request that a new regmap be
> > created (with start and end being honored.) A benefit of this design is
> > that any regmaps that are named the same are automatically shared. A
> > drawback (or maybe a benefit?) is that the users have no control over
> > ranges / flags.
> > 
> > I think if this goes the way of index-based that'll work. I'm happy to
> > revert my previous change (sorry it snuck in) but it seems like there'll
> > still have to be some trickery... For reference:
> > 
> > enum ocelot_target {
> > 	ANA = 1,
> > 	QS,
> > 	QSYS,
> > 	REW,
> > 	SYS,
> > 	S0,
> > 	S1,
> > 	S2,
> > 	HSIO,
> > 	PTP,
> > 	FDMA,
> > 	GCB,
> > 	DEV_GMII,
> > 	TARGET_MAX,
> > };
> > 
> > mfd_add_devices will probably need to add a zero-size resource for HSIO,
> > PTP, FDMA, and anything else that might come along in the future. At
> > this point, regmap_from_mfd(PTP) might have to look like (pseudocode):
> > 
> > struct regmap *ocelot_ext_regmap_from_mfd(struct ocelot *ocelot, int index)
> > {
> >     return ocelot_regmap_from_resource_optional(pdev, index-1, config);
> > 
> >     /* Note this essentially expands to:
> >      * res = platform_get_resource(pdev, IORESOURCE_REG, index-1);
> >      * return dev_get_regmap(pdev->dev.parent, res->name);
> >      *
> >      * This essentially throws away everything that my current
> >      * implementation does, except for the IORESOURCE_REG flag
> >      */
> > }
> > 
> > Then drivers/net/dsa/felix.c felix_init_structs() would have two loops
> > (again, just as an example)
> > 
> > for (i = ANA; i < TARGET_MAX; i++) {
> >     if (felix->info->regmap_from_mfd)
> >         target = felix->info->regmap_from_mfd(ocelot, i);
> >     else {
> >         /* existing logic back to ocelot_regmap_init() */
> >     }
> > }
> > 
> > for (port = 0; port < num_phys_ports; port++) {
> >     ...
> >     if (felix->info->regmap_from_mfd)
> >         target = felix->info->regmap_from_mfd(ocelot, TARGET_MAX + port);
> >     else {
> >         /* existing logic back to ocelot_regmap_init() */
> >     }
> > }
> > 
> > And lastly, ocelot_core_init() in drivers/mfd/ocelot-core.c would need a
> > mechanism to say "don't add a regmap for cell->resources[PTP], even
> > though that resource exists" because... well I suppose it is only in
> > drivers/net/ethernet/mscc/ocelot_vsc7514.c for now, but the existance of
> > those regmaps invokes different behavior. For instance:
> > 
> > 	if (ocelot->targets[FDMA])
> > 		ocelot_fdma_init(pdev, ocelot);
> > 
> > I'm not sure whether this last point will have an effect on felix.c in
> > the end. My current patch set of adding the SERDES ports uses the
> > existance of targets[HSIO] to invoke ocelot_pll5_init() similar to the
> > ocelot_vsc7514.c FDMA / PTP conditionals, but I'd understand if that
> > gets rejected outright. That's for another day.
> > 
> > 
> > 
> > I'm happy to make these changes if you see them valid. I saw the fact
> > that dev_get_regmap(dev->parent, res->name) could be used directly in
> > ocelot_ext_regmap_init() as an elegant solution to felix / ocelot-lib /
> > ocelot-core, but I recognize that the subtle "throw away the
> > IORESOURCE_MEM flag and res->{start,end} information" isn't ideal.
> 
> Thinking some more about it, there will have to be even more trickery.
> Say you solve the problem for the global targets, but then what do you
> do for the port targets, how do you reference those by index?
> TARGET_MAX + port? Hmm, that isn't great either.

Yep, that's what my example above shows. Not my favorite.

> 
> What if we meet half way, and you just get the resources from the
> platform device by name, instead of by index? You'd have to modify the
> regmap creation procedure to look at a predefined array of strings,
> containing names of all targets that are mandatory (a la mscc_ocelot_probe),
> and match those
> (a) iteration over target_io_res and strcmp(), in the case of vsc9959
>     and vsc9953
> (b) dev_get_regmap() in the case of ocelot_ext
> 
> This way there's still no direct communication between ocelot-mfd and
> DSA, and I have the feeling that the problems we both mention are
> solved. Hope I'm not missing something.

This sounds reasonable. So long as it doesn't muddy up felix / seville
too much - I'll take a look. It seems like it would just be moving
a lot of the "resource configuration" code from felix_init_structs() into
the felix->info->init_regmap(), or similar.

Re: [RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext

[Vladimir Oltean]

On Mon, Sep 12, 2022 at 02:03:43PM -0700, Colin Foster wrote:
> Yep, that's what my example above shows. Not my favorite.

Yeah, sorry, I was on mobile and I had exactly one pass at reading your
response. You did point this out with the "two loops" comment.

> This sounds reasonable. So long as it doesn't muddy up felix / seville
> too much - I'll take a look. It seems like it would just be moving
> a lot of the "resource configuration" code from felix_init_structs() into
> the felix->info->init_regmap(), or similar.

Possibly so, yes. I don't have any other comments for this series, btw.
Just make sure to grab the attention of one of the maintainers somehow
to get Lee's branch pulled, before you send the next version. Not exactly
sure how; LPC has just started and most people have their attention
focused there, I guess.

[RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

Add control of an external VSC7512 chip by way of the ocelot-mfd interface.

Currently the four copper phy ports are fully functional. Communication to
external phys is also functional, but the SGMII / QSGMII interfaces are
currently non-functional.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

v1 from previous RFC:
    * Remove unnecessary byteorder and kconfig header includes.
    * Create OCELOT_EXT_PORT_MODE_SERDES macro to match vsc9959.
    * Utilize readx_poll_timeout for SYS_RESET_CFG_MEM_INIT.
    * *_io_res struct arrays have been moved to the MFD files.
    * Changes to utilize phylink_generic_validate() have been squashed.
    * dev_err_probe() is used in the probe function.
    * Make ocelot_ext_switch_of_match static.
    * Relocate ocelot_ext_ops structure to be next to vsc7512_info, to
      match what was done in other felix drivers.
    * Utilize dev_get_regmap() instead of the obsolete
      ocelot_init_regmap_from_resource() routine.

---
 drivers/mfd/ocelot-core.c           |   3 +
 drivers/net/dsa/ocelot/Kconfig      |  14 ++
 drivers/net/dsa/ocelot/Makefile     |   5 +
 drivers/net/dsa/ocelot/ocelot_ext.c | 254 ++++++++++++++++++++++++++++
 include/soc/mscc/ocelot.h           |   2 +
 5 files changed, 278 insertions(+)
 create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index aa7fa21b354c..b7b9f6855f74 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -188,6 +188,9 @@ static const struct mfd_cell vsc7512_devs[] = {
 		.use_of_reg = true,
 		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
 		.resources = vsc7512_miim1_resources,
+	}, {
+		.name = "ocelot-ext-switch",
+		.of_compatible = "mscc,vsc7512-ext-switch",
 	},
 };
 
diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index 08db9cf76818..d8b224f8dc97 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -1,4 +1,18 @@
 # SPDX-License-Identifier: GPL-2.0-only
+config NET_DSA_MSCC_OCELOT_EXT
+	tristate "Ocelot External Ethernet switch support"
+	depends on NET_DSA && SPI
+	depends on NET_VENDOR_MICROSEMI
+	select MDIO_MSCC_MIIM
+	select MFD_OCELOT_CORE
+	select MSCC_OCELOT_SWITCH_LIB
+	select NET_DSA_TAG_OCELOT_8021Q
+	select NET_DSA_TAG_OCELOT
+	help
+	  This driver supports the VSC7511, VSC7512, VSC7513 and VSC7514 chips
+	  when controlled through SPI. It can be used with the Microsemi dev
+	  boards and an external CPU or custom hardware.
+
 config NET_DSA_MSCC_FELIX
 	tristate "Ocelot / Felix Ethernet switch support"
 	depends on NET_DSA && PCI
diff --git a/drivers/net/dsa/ocelot/Makefile b/drivers/net/dsa/ocelot/Makefile
index f6dd131e7491..d7f3f5a4461c 100644
--- a/drivers/net/dsa/ocelot/Makefile
+++ b/drivers/net/dsa/ocelot/Makefile
@@ -1,11 +1,16 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_NET_DSA_MSCC_FELIX) += mscc_felix.o
+obj-$(CONFIG_NET_DSA_MSCC_OCELOT_EXT) += mscc_ocelot_ext.o
 obj-$(CONFIG_NET_DSA_MSCC_SEVILLE) += mscc_seville.o
 
 mscc_felix-objs := \
 	felix.o \
 	felix_vsc9959.o
 
+mscc_ocelot_ext-objs := \
+	felix.o \
+	ocelot_ext.o
+
 mscc_seville-objs := \
 	felix.o \
 	seville_vsc9953.o
diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c
new file mode 100644
index 000000000000..c821cc963787
--- /dev/null
+++ b/drivers/net/dsa/ocelot/ocelot_ext.c
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright 2021-2022 Innovative Advantage Inc.
+ */
+
+#include <linux/iopoll.h>
+#include <linux/mfd/ocelot.h>
+#include <linux/phylink.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <soc/mscc/ocelot_ana.h>
+#include <soc/mscc/ocelot_dev.h>
+#include <soc/mscc/ocelot_qsys.h>
+#include <soc/mscc/ocelot_vcap.h>
+#include <soc/mscc/ocelot_ptp.h>
+#include <soc/mscc/ocelot_sys.h>
+#include <soc/mscc/ocelot.h>
+#include <soc/mscc/vsc7514_regs.h>
+#include "felix.h"
+
+#define VSC7512_NUM_PORTS		11
+
+#define OCELOT_EXT_MEM_INIT_SLEEP_US	1000
+#define OCELOT_EXT_MEM_INIT_TIMEOUT_US	100000
+
+#define OCELOT_EXT_PORT_MODE_SERDES	(OCELOT_PORT_MODE_SGMII | \
+					 OCELOT_PORT_MODE_QSGMII)
+
+static const u32 vsc7512_port_modes[VSC7512_NUM_PORTS] = {
+	OCELOT_PORT_MODE_INTERNAL,
+	OCELOT_PORT_MODE_INTERNAL,
+	OCELOT_PORT_MODE_INTERNAL,
+	OCELOT_PORT_MODE_INTERNAL,
+	OCELOT_EXT_PORT_MODE_SERDES,
+	OCELOT_EXT_PORT_MODE_SERDES,
+	OCELOT_EXT_PORT_MODE_SERDES,
+	OCELOT_EXT_PORT_MODE_SERDES,
+	OCELOT_EXT_PORT_MODE_SERDES,
+	OCELOT_EXT_PORT_MODE_SERDES,
+	OCELOT_PORT_MODE_SGMII,
+	OCELOT_EXT_PORT_MODE_SERDES,
+};
+
+static const u32 vsc7512_gcb_regmap[] = {
+	REG(GCB_SOFT_RST,			0x0008),
+	REG(GCB_MIIM_MII_STATUS,		0x009c),
+	REG(GCB_PHY_PHY_CFG,			0x00f0),
+	REG(GCB_PHY_PHY_STAT,			0x00f4),
+};
+
+static const u32 *vsc7512_regmap[TARGET_MAX] = {
+	[ANA] = vsc7514_ana_regmap,
+	[QS] = vsc7514_qs_regmap,
+	[QSYS] = vsc7514_qsys_regmap,
+	[REW] = vsc7514_rew_regmap,
+	[SYS] = vsc7514_sys_regmap,
+	[S0] = vsc7514_vcap_regmap,
+	[S1] = vsc7514_vcap_regmap,
+	[S2] = vsc7514_vcap_regmap,
+	[PTP] = vsc7514_ptp_regmap,
+	[GCB] = vsc7512_gcb_regmap,
+	[DEV_GMII] = vsc7514_dev_gmii_regmap,
+};
+
+static void ocelot_ext_reset_phys(struct ocelot *ocelot)
+{
+	ocelot_write(ocelot, 0, GCB_PHY_PHY_CFG);
+	ocelot_write(ocelot, 0x1ff, GCB_PHY_PHY_CFG);
+	mdelay(500);
+}
+
+static int ocelot_ext_mem_init_status(struct ocelot *ocelot)
+{
+	int val, err;
+
+	err = regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], &val);
+
+	return err ?: val;
+}
+
+static int ocelot_ext_reset(struct ocelot *ocelot)
+{
+	int err, val;
+
+	ocelot_ext_reset_phys(ocelot);
+
+	/* Initialize chip memories */
+	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
+	if (err)
+		return err;
+
+	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
+	if (err)
+		return err;
+
+	/* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
+	 * 100us) before enabling the switch core
+	 */
+	err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
+				 OCELOT_EXT_MEM_INIT_SLEEP_US,
+				 OCELOT_EXT_MEM_INIT_TIMEOUT_US);
+
+	if (IS_ERR_VALUE(err))
+		return err;
+
+	return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
+}
+
+static void ocelot_ext_phylink_validate(struct ocelot *ocelot, int port,
+					unsigned long *supported,
+					struct phylink_link_state *state)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct dsa_switch *ds = felix->ds;
+	struct phylink_config *pl_config;
+	struct dsa_port *dp;
+
+	dp = dsa_to_port(ds, port);
+	pl_config = &dp->pl_config;
+
+	phylink_generic_validate(pl_config, supported, state);
+}
+
+static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
+					     struct resource *res)
+{
+	return dev_get_regmap(ocelot->dev->parent, res->name);
+}
+
+static const struct ocelot_ops ocelot_ext_ops = {
+	.reset		= ocelot_ext_reset,
+	.wm_enc		= ocelot_wm_enc,
+	.wm_dec		= ocelot_wm_dec,
+	.wm_stat	= ocelot_wm_stat,
+	.port_to_netdev	= felix_port_to_netdev,
+	.netdev_to_port	= felix_netdev_to_port,
+};
+
+static const struct felix_info vsc7512_info = {
+	.target_io_res			= vsc7512_target_io_res,
+	.port_io_res			= vsc7512_port_io_res,
+	.regfields			= vsc7514_regfields,
+	.map				= vsc7512_regmap,
+	.ops				= &ocelot_ext_ops,
+	.stats_layout			= vsc7514_stats_layout,
+	.vcap				= vsc7514_vcap_props,
+	.num_mact_rows			= 1024,
+	.num_ports			= VSC7512_NUM_PORTS,
+	.num_tx_queues			= OCELOT_NUM_TC,
+	.phylink_validate		= ocelot_ext_phylink_validate,
+	.port_modes			= vsc7512_port_modes,
+	.init_regmap			= ocelot_ext_regmap_init,
+};
+
+static int ocelot_ext_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dsa_switch *ds;
+	struct ocelot *ocelot;
+	struct felix *felix;
+	int err;
+
+	felix = kzalloc(sizeof(*felix), GFP_KERNEL);
+	if (!felix)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, felix);
+
+	ocelot = &felix->ocelot;
+	ocelot->dev = dev;
+
+	ocelot->num_flooding_pgids = 1;
+
+	felix->info = &vsc7512_info;
+
+	ds = kzalloc(sizeof(*ds), GFP_KERNEL);
+	if (!ds) {
+		err = -ENOMEM;
+		dev_err_probe(dev, err, "Failed to allocate DSA switch\n");
+		goto err_free_felix;
+	}
+
+	ds->dev = dev;
+	ds->num_ports = felix->info->num_ports;
+	ds->num_tx_queues = felix->info->num_tx_queues;
+
+	ds->ops = &felix_switch_ops;
+	ds->priv = ocelot;
+	felix->ds = ds;
+	felix->tag_proto = DSA_TAG_PROTO_OCELOT;
+
+	err = dsa_register_switch(ds);
+	if (err) {
+		dev_err_probe(dev, err, "Failed to register DSA switch\n");
+		goto err_free_ds;
+	}
+
+	return 0;
+
+err_free_ds:
+	kfree(ds);
+err_free_felix:
+	kfree(felix);
+	return err;
+}
+
+static int ocelot_ext_remove(struct platform_device *pdev)
+{
+	struct felix *felix = dev_get_drvdata(&pdev->dev);
+
+	if (!felix)
+		return 0;
+
+	dsa_unregister_switch(felix->ds);
+
+	kfree(felix->ds);
+	kfree(felix);
+
+	dev_set_drvdata(&pdev->dev, NULL);
+
+	return 0;
+}
+
+static void ocelot_ext_shutdown(struct platform_device *pdev)
+{
+	struct felix *felix = dev_get_drvdata(&pdev->dev);
+
+	if (!felix)
+		return;
+
+	dsa_switch_shutdown(felix->ds);
+
+	dev_set_drvdata(&pdev->dev, NULL);
+}
+
+static const struct of_device_id ocelot_ext_switch_of_match[] = {
+	{ .compatible = "mscc,vsc7512-ext-switch" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ocelot_ext_switch_of_match);
+
+static struct platform_driver ocelot_ext_switch_driver = {
+	.driver = {
+		.name = "ocelot-ext-switch",
+		.of_match_table = of_match_ptr(ocelot_ext_switch_of_match),
+	},
+	.probe = ocelot_ext_probe,
+	.remove = ocelot_ext_remove,
+	.shutdown = ocelot_ext_shutdown,
+};
+module_platform_driver(ocelot_ext_switch_driver);
+
+MODULE_DESCRIPTION("External Ocelot Switch driver");
+MODULE_LICENSE("GPL");
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 17dd61f36563..2ed38110a6cc 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -460,6 +460,8 @@ enum ocelot_reg {
 	GCB_MIIM_MII_STATUS,
 	GCB_MIIM_MII_CMD,
 	GCB_MIIM_MII_DATA,
+	GCB_PHY_PHY_CFG,
+	GCB_PHY_PHY_STAT,
 	DEV_CLOCK_CFG = DEV_GMII << TARGET_OFFSET,
 	DEV_PORT_MISC,
 	DEV_EVENTS,
-- 
2.25.1

Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Lee Jones]

On Sun, 11 Sep 2022, Colin Foster wrote:

> Add control of an external VSC7512 chip by way of the ocelot-mfd interface.
> 
> Currently the four copper phy ports are fully functional. Communication to
> external phys is also functional, but the SGMII / QSGMII interfaces are
> currently non-functional.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
> 
> v1 from previous RFC:
>     * Remove unnecessary byteorder and kconfig header includes.
>     * Create OCELOT_EXT_PORT_MODE_SERDES macro to match vsc9959.
>     * Utilize readx_poll_timeout for SYS_RESET_CFG_MEM_INIT.
>     * *_io_res struct arrays have been moved to the MFD files.
>     * Changes to utilize phylink_generic_validate() have been squashed.
>     * dev_err_probe() is used in the probe function.
>     * Make ocelot_ext_switch_of_match static.
>     * Relocate ocelot_ext_ops structure to be next to vsc7512_info, to
>       match what was done in other felix drivers.
>     * Utilize dev_get_regmap() instead of the obsolete
>       ocelot_init_regmap_from_resource() routine.
> 
> ---
>  drivers/mfd/ocelot-core.c           |   3 +
>  drivers/net/dsa/ocelot/Kconfig      |  14 ++
>  drivers/net/dsa/ocelot/Makefile     |   5 +
>  drivers/net/dsa/ocelot/ocelot_ext.c | 254 ++++++++++++++++++++++++++++
>  include/soc/mscc/ocelot.h           |   2 +
>  5 files changed, 278 insertions(+)
>  create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c
> 
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index aa7fa21b354c..b7b9f6855f74 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -188,6 +188,9 @@ static const struct mfd_cell vsc7512_devs[] = {
>  		.use_of_reg = true,
>  		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
>  		.resources = vsc7512_miim1_resources,
> +	}, {
> +		.name = "ocelot-ext-switch",
> +		.of_compatible = "mscc,vsc7512-ext-switch",
>  	},
>  };

Please separate this out into its own patch.

-- 
Lee Jones [李琼斯]

Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

On Mon, Sep 12, 2022 at 11:51:14AM +0100, Lee Jones wrote:
> On Sun, 11 Sep 2022, Colin Foster wrote:
> 
> > Add control of an external VSC7512 chip by way of the ocelot-mfd interface.
> > 
> > Currently the four copper phy ports are fully functional. Communication to
> > external phys is also functional, but the SGMII / QSGMII interfaces are
> > currently non-functional.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> > 
> > v1 from previous RFC:
> >     * Remove unnecessary byteorder and kconfig header includes.
> >     * Create OCELOT_EXT_PORT_MODE_SERDES macro to match vsc9959.
> >     * Utilize readx_poll_timeout for SYS_RESET_CFG_MEM_INIT.
> >     * *_io_res struct arrays have been moved to the MFD files.
> >     * Changes to utilize phylink_generic_validate() have been squashed.
> >     * dev_err_probe() is used in the probe function.
> >     * Make ocelot_ext_switch_of_match static.
> >     * Relocate ocelot_ext_ops structure to be next to vsc7512_info, to
> >       match what was done in other felix drivers.
> >     * Utilize dev_get_regmap() instead of the obsolete
> >       ocelot_init_regmap_from_resource() routine.
> > 
> > ---
> >  drivers/mfd/ocelot-core.c           |   3 +
> >  drivers/net/dsa/ocelot/Kconfig      |  14 ++
> >  drivers/net/dsa/ocelot/Makefile     |   5 +
> >  drivers/net/dsa/ocelot/ocelot_ext.c | 254 ++++++++++++++++++++++++++++
> >  include/soc/mscc/ocelot.h           |   2 +
> >  5 files changed, 278 insertions(+)
> >  create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c
> > 
> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > index aa7fa21b354c..b7b9f6855f74 100644
> > --- a/drivers/mfd/ocelot-core.c
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -188,6 +188,9 @@ static const struct mfd_cell vsc7512_devs[] = {
> >  		.use_of_reg = true,
> >  		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
> >  		.resources = vsc7512_miim1_resources,
> > +	}, {
> > +		.name = "ocelot-ext-switch",
> > +		.of_compatible = "mscc,vsc7512-ext-switch",
> >  	},
> >  };
> 
> Please separate this out into its own patch.

I'll do that.

> 
> -- 
> Lee Jones [李琼斯]

Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Vladimir Oltean]

On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> index 08db9cf76818..d8b224f8dc97 100644
> --- a/drivers/net/dsa/ocelot/Kconfig
> +++ b/drivers/net/dsa/ocelot/Kconfig
> @@ -1,4 +1,18 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +config NET_DSA_MSCC_OCELOT_EXT
> +	tristate "Ocelot External Ethernet switch support"
> +	depends on NET_DSA && SPI
> +	depends on NET_VENDOR_MICROSEMI
> +	select MDIO_MSCC_MIIM
> +	select MFD_OCELOT_CORE
> +	select MSCC_OCELOT_SWITCH_LIB
> +	select NET_DSA_TAG_OCELOT_8021Q
> +	select NET_DSA_TAG_OCELOT
> +	help
> +	  This driver supports the VSC7511, VSC7512, VSC7513 and VSC7514 chips
> +	  when controlled through SPI. It can be used with the Microsemi dev
> +	  boards and an external CPU or custom hardware.

I would drop the sentence about Microsemi dev boards or custom hardware.

> diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c
> new file mode 100644
> index 000000000000..c821cc963787
> --- /dev/null
> +++ b/drivers/net/dsa/ocelot/ocelot_ext.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright 2021-2022 Innovative Advantage Inc.
> + */
> +
> +#include <linux/iopoll.h>
> +#include <linux/mfd/ocelot.h>
> +#include <linux/phylink.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <soc/mscc/ocelot_ana.h>
> +#include <soc/mscc/ocelot_dev.h>
> +#include <soc/mscc/ocelot_qsys.h>
> +#include <soc/mscc/ocelot_vcap.h>
> +#include <soc/mscc/ocelot_ptp.h>
> +#include <soc/mscc/ocelot_sys.h>
> +#include <soc/mscc/ocelot.h>
> +#include <soc/mscc/vsc7514_regs.h>
> +#include "felix.h"
> +
> +#define VSC7512_NUM_PORTS		11
> +
> +#define OCELOT_EXT_MEM_INIT_SLEEP_US	1000
> +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US	100000
> +
> +#define OCELOT_EXT_PORT_MODE_SERDES	(OCELOT_PORT_MODE_SGMII | \
> +					 OCELOT_PORT_MODE_QSGMII)

There are places where OCELOT_EXT doesn't make too much sense, like here.
The capabilities of the SERDES ports do not change depending on whether
the switch is controlled externally or not. Same for the memory init
delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?

There are more places as well below in function names, I'll let you be
the judge if whether ocelot is controlled externally is relevant to what
they do in any way.

> +static int ocelot_ext_reset(struct ocelot *ocelot)
> +{
> +	int err, val;
> +
> +	ocelot_ext_reset_phys(ocelot);
> +
> +	/* Initialize chip memories */
> +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
> +	if (err)
> +		return err;
> +
> +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
> +	if (err)
> +		return err;
> +
> +	/* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
> +	 * 100us) before enabling the switch core
> +	 */
> +	err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
> +				 OCELOT_EXT_MEM_INIT_SLEEP_US,
> +				 OCELOT_EXT_MEM_INIT_TIMEOUT_US);
> +

I think you can eliminate the newline between the err assignment and
checking for it.

> +	if (IS_ERR_VALUE(err))
> +		return err;
> +
> +	return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
> +}
> +
> +static void ocelot_ext_phylink_validate(struct ocelot *ocelot, int port,
> +					unsigned long *supported,
> +					struct phylink_link_state *state)
> +{
> +	struct felix *felix = ocelot_to_felix(ocelot);
> +	struct dsa_switch *ds = felix->ds;
> +	struct phylink_config *pl_config;
> +	struct dsa_port *dp;
> +
> +	dp = dsa_to_port(ds, port);
> +	pl_config = &dp->pl_config;
> +
> +	phylink_generic_validate(pl_config, supported, state);

You could save 2 lines here (defining *pl_config and assigning it) if
you would just call phylink_generic_validate(&dp->pl_config, ...);

> +}
> +
> +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
> +					     struct resource *res)
> +{
> +	return dev_get_regmap(ocelot->dev->parent, res->name);
> +}

I have more fundamental questions about this one, which I've formulated
on your patch 7/8. If nothing changes, at least I'd expect some comments
here explaining where the resources actually come from, and the regmaps.

> +static const struct of_device_id ocelot_ext_switch_of_match[] = {
> +	{ .compatible = "mscc,vsc7512-ext-switch" },

I think I've raised this before. How about removing "external" from the
compatible string? You can figure out it's external, because it's on a
SPI bus.

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ocelot_ext_switch_of_match);
> +
> +static struct platform_driver ocelot_ext_switch_driver = {
> +	.driver = {
> +		.name = "ocelot-ext-switch",
> +		.of_match_table = of_match_ptr(ocelot_ext_switch_of_match),
> +	},
> +	.probe = ocelot_ext_probe,
> +	.remove = ocelot_ext_remove,
> +	.shutdown = ocelot_ext_shutdown,
> +};
> +module_platform_driver(ocelot_ext_switch_driver);

Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

Hi Vladimir,

On Mon, Sep 12, 2022 at 05:21:10PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> > index 08db9cf76818..d8b224f8dc97 100644
> > --- a/drivers/net/dsa/ocelot/Kconfig
> > +++ b/drivers/net/dsa/ocelot/Kconfig
> > @@ -1,4 +1,18 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +config NET_DSA_MSCC_OCELOT_EXT
> > +	tristate "Ocelot External Ethernet switch support"
> > +	depends on NET_DSA && SPI
> > +	depends on NET_VENDOR_MICROSEMI
> > +	select MDIO_MSCC_MIIM
> > +	select MFD_OCELOT_CORE
> > +	select MSCC_OCELOT_SWITCH_LIB
> > +	select NET_DSA_TAG_OCELOT_8021Q
> > +	select NET_DSA_TAG_OCELOT
> > +	help
> > +	  This driver supports the VSC7511, VSC7512, VSC7513 and VSC7514 chips
> > +	  when controlled through SPI. It can be used with the Microsemi dev
> > +	  boards and an external CPU or custom hardware.
> 
> I would drop the sentence about Microsemi dev boards or custom hardware.

I'll do that. Also I need to add a paragraph (according to checkpatch)
about what the VSC751{1-4} chips actually are.

> 
> > diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c
> > new file mode 100644
> > index 000000000000..c821cc963787
> > --- /dev/null
> > +++ b/drivers/net/dsa/ocelot/ocelot_ext.c
> > @@ -0,0 +1,254 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2021-2022 Innovative Advantage Inc.
> > + */
> > +
> > +#include <linux/iopoll.h>
> > +#include <linux/mfd/ocelot.h>
> > +#include <linux/phylink.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <soc/mscc/ocelot_ana.h>
> > +#include <soc/mscc/ocelot_dev.h>
> > +#include <soc/mscc/ocelot_qsys.h>
> > +#include <soc/mscc/ocelot_vcap.h>
> > +#include <soc/mscc/ocelot_ptp.h>
> > +#include <soc/mscc/ocelot_sys.h>
> > +#include <soc/mscc/ocelot.h>
> > +#include <soc/mscc/vsc7514_regs.h>
> > +#include "felix.h"
> > +
> > +#define VSC7512_NUM_PORTS		11
> > +
> > +#define OCELOT_EXT_MEM_INIT_SLEEP_US	1000
> > +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US	100000
> > +
> > +#define OCELOT_EXT_PORT_MODE_SERDES	(OCELOT_PORT_MODE_SGMII | \
> > +					 OCELOT_PORT_MODE_QSGMII)
> 
> There are places where OCELOT_EXT doesn't make too much sense, like here.
> The capabilities of the SERDES ports do not change depending on whether
> the switch is controlled externally or not. Same for the memory init
> delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
> 
> There are more places as well below in function names, I'll let you be
> the judge if whether ocelot is controlled externally is relevant to what
> they do in any way.
> 
> > +static int ocelot_ext_reset(struct ocelot *ocelot)
> > +{
> > +	int err, val;
> > +
> > +	ocelot_ext_reset_phys(ocelot);
> > +
> > +	/* Initialize chip memories */
> > +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
> > +	if (err)
> > +		return err;
> > +
> > +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
> > +	if (err)
> > +		return err;
> > +
> > +	/* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
> > +	 * 100us) before enabling the switch core
> > +	 */
> > +	err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
> > +				 OCELOT_EXT_MEM_INIT_SLEEP_US,
> > +				 OCELOT_EXT_MEM_INIT_TIMEOUT_US);
> > +
> 
> I think you can eliminate the newline between the err assignment and
> checking for it.

Yes, you've pointed out this habit of mine in the past. I'm sorry you
have to keep reminding me - I'll try to commit this one to memory now.

> 
> > +	if (IS_ERR_VALUE(err))
> > +		return err;
> > +
> > +	return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
> > +}
> > +
> > +static void ocelot_ext_phylink_validate(struct ocelot *ocelot, int port,
> > +					unsigned long *supported,
> > +					struct phylink_link_state *state)
> > +{
> > +	struct felix *felix = ocelot_to_felix(ocelot);
> > +	struct dsa_switch *ds = felix->ds;
> > +	struct phylink_config *pl_config;
> > +	struct dsa_port *dp;
> > +
> > +	dp = dsa_to_port(ds, port);
> > +	pl_config = &dp->pl_config;
> > +
> > +	phylink_generic_validate(pl_config, supported, state);
> 
> You could save 2 lines here (defining *pl_config and assigning it) if
> you would just call phylink_generic_validate(&dp->pl_config, ...);

Fair enough. Thanks.

> 
> > +}
> > +
> > +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
> > +					     struct resource *res)
> > +{
> > +	return dev_get_regmap(ocelot->dev->parent, res->name);
> > +}
> 
> I have more fundamental questions about this one, which I've formulated
> on your patch 7/8. If nothing changes, at least I'd expect some comments
> here explaining where the resources actually come from, and the regmaps.

Yep. That discussion should address everything anyone might want to
know. Wherever this lands, I'll make sure it is clear to the reader.

> 
> > +static const struct of_device_id ocelot_ext_switch_of_match[] = {
> > +	{ .compatible = "mscc,vsc7512-ext-switch" },
> 
> I think I've raised this before. How about removing "external" from the
> compatible string? You can figure out it's external, because it's on a
> SPI bus.

I believe you're right. I'm sorry that slipped through the cracks.

> 
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ocelot_ext_switch_of_match);
> > +
> > +static struct platform_driver ocelot_ext_switch_driver = {
> > +	.driver = {
> > +		.name = "ocelot-ext-switch",
> > +		.of_match_table = of_match_ptr(ocelot_ext_switch_of_match),
> > +	},
> > +	.probe = ocelot_ext_probe,
> > +	.remove = ocelot_ext_remove,
> > +	.shutdown = ocelot_ext_shutdown,
> > +};
> > +module_platform_driver(ocelot_ext_switch_driver);

Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

On Mon, Sep 12, 2022 at 05:21:10PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> > +
> > +#define OCELOT_EXT_MEM_INIT_SLEEP_US	1000
> > +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US	100000
> > +
> > +#define OCELOT_EXT_PORT_MODE_SERDES	(OCELOT_PORT_MODE_SGMII | \
> > +					 OCELOT_PORT_MODE_QSGMII)
> 
> There are places where OCELOT_EXT doesn't make too much sense, like here.
> The capabilities of the SERDES ports do not change depending on whether
> the switch is controlled externally or not. Same for the memory init
> delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
> 
> There are more places as well below in function names, I'll let you be
> the judge if whether ocelot is controlled externally is relevant to what
> they do in any way.
> 
> > +static int ocelot_ext_reset(struct ocelot *ocelot)

I'm looking into these changes now. I was using "ocelot_ext_" not
necessarily as an indication as "this only matters when it is controlled
externally" but rather "this is a function within ocelot_ext.c"

So there's a weird line between what constitutes "external control" vs
"generic"

There are only two things that really matter for external control:
1. The regmaps are set up in a specific way by ocelot-mfd
2. The existence of a DSA CPU port

Going by 1 only - there's basically nothing in
drivers/net/dsa/ocelot/ocelot_ext.c that is inherently "external only".
And that's kindof the point. The only thing that can be done externally
that isn't done internally would be a whole chip reset.

Going by 2 only - the simple fact that it is in drivers/net/dsa/ means
that there is a CPU port, and therefore everything in the file requires
that it is externally controlled.



Unless you're going another way, and you're not actually talking about
"function names" but instead "should this actually live in ocelot_lib"

While I don't think that's what you were directly suggesting - I like
this! ocelot_ext_reset() shouldn't exist - I should move, update, and
utilize ocelot_reset() from drivers/net/ethernet/mscc/ocelot_vsc7514.c.

The ocelot_ext function list will dwindle down to:
*_probe
*_remove
*_shutdown
*_regmap_init
*_phylink_validate

Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Vladimir Oltean]

On Fri, Sep 16, 2022 at 09:55:55AM -0700, Colin Foster wrote:
> I'm looking into these changes now. I was using "ocelot_ext_" not
> necessarily as an indication as "this only matters when it is controlled
> externally" but rather "this is a function within ocelot_ext.c"
> 
> So there's a weird line between what constitutes "external control" vs
> "generic"
> 
> There are only two things that really matter for external control:
> 1. The regmaps are set up in a specific way by ocelot-mfd
> 2. The existence of a DSA CPU port
> 
> Going by 1 only - there's basically nothing in
> drivers/net/dsa/ocelot/ocelot_ext.c that is inherently "external only".
> And that's kindof the point. The only thing that can be done externally
> that isn't done internally would be a whole chip reset.
> 
> Going by 2 only - the simple fact that it is in drivers/net/dsa/ means
> that there is a CPU port, and therefore everything in the file requires
> that it is externally controlled.
> 
> 
> 
> Unless you're going another way, and you're not actually talking about
> "function names" but instead "should this actually live in ocelot_lib"
> 
> While I don't think that's what you were directly suggesting - I like
> this! ocelot_ext_reset() shouldn't exist - I should move, update, and
> utilize ocelot_reset() from drivers/net/ethernet/mscc/ocelot_vsc7514.c.
> 
> The ocelot_ext function list will dwindle down to:
> *_probe
> *_remove
> *_shutdown
> *_regmap_init
> *_phylink_validate

Yes, please use as much as possible from the ocelot switch library,
after all you are driving pretty much the same hardware. I'm glad for
your revelation and sorry that I didn't think of expressing it this way
sooner. I think the reset procedure used to be slightly different in the
times when the ocelot_ext DSA driver also took care of setting up what
is now the responsibility of the ocelot-mfd driver. Between then and
now, some time has passed (years if I'm not mistaken) and I forgot what
was and what wasn't said, I even forgot most of my own thoughts.

Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

On Fri, Sep 16, 2022 at 10:31:47PM +0000, Vladimir Oltean wrote:
> On Fri, Sep 16, 2022 at 09:55:55AM -0700, Colin Foster wrote:
> Yes, please use as much as possible from the ocelot switch library,
> after all you are driving pretty much the same hardware. I'm glad for
> your revelation and sorry that I didn't think of expressing it this way
> sooner. I think the reset procedure used to be slightly different in the
> times when the ocelot_ext DSA driver also took care of setting up what
> is now the responsibility of the ocelot-mfd driver.

That's exactly right. Early on ocelot_ext was doing all sorts of things
that are now the control of ocelot-mfd and the various subsystems. These
couple procedures seem to be the last relics of those early days.

As I cleaned this up, I realized ocelot_ext_reset_phys() is no longer
needed, as it is handled in the MDIO driver. It looks like there won't
be much to remove when this is all said and done :-)

> Between then and
> now, some time has passed (years if I'm not mistaken)

Thanks for reminding me. If I knew then what I know now... maybe I'd ask
the hardware person to spec a different chip ;-)

I'm joking of course, and looking forward to being able to use this
thing!

Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

[Colin Foster]

On Mon, Sep 12, 2022 at 05:21:10PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> > +#include <linux/iopoll.h>
> > +#include <linux/mfd/ocelot.h>
> > +#include <linux/phylink.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <soc/mscc/ocelot_ana.h>
> > +#include <soc/mscc/ocelot_dev.h>
> > +#include <soc/mscc/ocelot_qsys.h>
> > +#include <soc/mscc/ocelot_vcap.h>
> > +#include <soc/mscc/ocelot_ptp.h>
> > +#include <soc/mscc/ocelot_sys.h>
> > +#include <soc/mscc/ocelot.h>
> > +#include <soc/mscc/vsc7514_regs.h>
> > +#include "felix.h"
> > +
> > +#define VSC7512_NUM_PORTS		11
> > +
> > +#define OCELOT_EXT_MEM_INIT_SLEEP_US	1000
> > +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US	100000
> > +
> > +#define OCELOT_EXT_PORT_MODE_SERDES	(OCELOT_PORT_MODE_SGMII | \
> > +					 OCELOT_PORT_MODE_QSGMII)
> 
> There are places where OCELOT_EXT doesn't make too much sense, like here.
> The capabilities of the SERDES ports do not change depending on whether
> the switch is controlled externally or not. Same for the memory init
> delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
> 
> There are more places as well below in function names, I'll let you be
> the judge if whether ocelot is controlled externally is relevant to what
> they do in any way.
> 
> > +static int ocelot_ext_reset(struct ocelot *ocelot)
> > +{
> > +	int err, val;
> > +
> > +	ocelot_ext_reset_phys(ocelot);
> > +
> > +	/* Initialize chip memories */
> > +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
> > +	if (err)
> > +		return err;
> > +
> > +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
> > +	if (err)
> > +		return err;
> > +
> > +	/* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
> > +	 * 100us) before enabling the switch core
> > +	 */
> > +	err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
> > +				 OCELOT_EXT_MEM_INIT_SLEEP_US,
> > +				 OCELOT_EXT_MEM_INIT_TIMEOUT_US);
> > +
> 
> I think you can eliminate the newline between the err assignment and
> checking for it.

In my upcoming v2 set, "ocelot_ext_reset" is moved to the shared
"ocelot_reset" routine. As such, iopoll.h isn't needed. And all
soc/mscc/ocelot_*.h includes aren't necessary either, since there are
literally zero register writes in ocelot_ext.c now.

I'll wait a couple days for everyone to go through their backlog. If
my "clean up ocelot_reset()" and your Documentation yaml patches get
approved, I'll be ready to send this out.