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.
v3
* Fix allmodconfig build (patch 8)
* Change documentation wording (patch 12)
* Import module namespace (patch 13)
* Fix array initializer (patch 13)
v2
* Utilize common ocelot_reset routine (new patch 5, modified patch 13)
* Change init_regmap() routine to be string-based (new patch 8)
* Split patches where necessary (patches 9 and 14)
* Add documentation (patch 12) and MAINTAINERS (patch 13)
* Upgrade to PATCH status
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 (14):
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: mscc: ocelot: expose ocelot_reset routine
net: dsa: felix: add configurable device quirks
net: dsa: felix: populate mac_capabilities for all ports
net: dsa: felix: update init_regmap to be string-based
pinctrl: ocelot: avoid macro redefinition
mfd: ocelot: prepend resource size macros to be 32-bit
mfd: ocelot: add regmaps for ocelot_ext
dt-bindings: net: dsa: ocelot: add ocelot-ext documentation
net: dsa: ocelot: add external ocelot switch control
mfd: ocelot: add external ocelot switch control
.../bindings/net/dsa/mscc,ocelot.yaml | 59 ++++++
MAINTAINERS | 1 +
drivers/mfd/ocelot-core.c | 98 ++++++++-
drivers/net/dsa/ocelot/Kconfig | 19 ++
drivers/net/dsa/ocelot/Makefile | 5 +
drivers/net/dsa/ocelot/felix.c | 68 ++++--
drivers/net/dsa/ocelot/felix.h | 5 +-
drivers/net/dsa/ocelot/felix_vsc9959.c | 3 +-
drivers/net/dsa/ocelot/ocelot_ext.c | 194 ++++++++++++++++++
drivers/net/dsa/ocelot/seville_vsc9953.c | 3 +-
drivers/net/ethernet/mscc/ocelot.c | 48 ++++-
drivers/net/ethernet/mscc/ocelot_devlink.c | 31 +++
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 181 +---------------
drivers/net/ethernet/mscc/vsc7514_regs.c | 108 ++++++++++
drivers/pinctrl/pinctrl-ocelot.c | 1 +
include/linux/mfd/ocelot.h | 5 +
include/soc/mscc/ocelot.h | 6 +
include/soc/mscc/vsc7514_regs.h | 6 +
18 files changed, 637 insertions(+), 204 deletions(-)
create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c
--
2.25.1
Expose ocelot_wm functions so they can be shared with other drivers.
Signed-off-by: Colin Foster <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
v1 - v3:
* 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 6f22aea08a64..bac0ee9126f8 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -234,34 +234,6 @@ static int ocelot_reset(struct ocelot *ocelot)
return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
}
-/* 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 967ba30ea636..55bbd5319128 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
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 <[email protected]>
---
v1 - v3 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 4fb525f071ac..19e5486d1dbd 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -181,49 +181,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
The *_RES_SIZE macros are initally <= 0x100. Future resource sizes will be
upwards of 0x200000 in size.
To keep things clean, fully align the RES_SIZE macros to 32-bit to do
nothing more than make the code more consistent.
Signed-off-by: Colin Foster <[email protected]>
---
b3
* No change
v2
* New patch - broken out from a different one
---
drivers/mfd/ocelot-core.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 1816d52c65c5..013e83173062 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -34,16 +34,16 @@
#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_GCB_RST_SLEEP_US 100
#define VSC7512_GCB_RST_TIMEOUT_US 100000
--
2.25.1
Resetting the switch core is the same whether it is done internally or
externally. Move this routine to the ocelot library so it can be used by
other drivers.
Signed-off-by: Colin Foster <[email protected]>
---
v3
* No changes
v2
* New patch
---
drivers/net/ethernet/mscc/ocelot.c | 48 +++++++++++++++++++++-
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 44 +-------------------
include/soc/mscc/ocelot.h | 1 +
3 files changed, 48 insertions(+), 45 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 7a613b52787d..d43106e386e6 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -6,12 +6,16 @@
*/
#include <linux/dsa/ocelot.h>
#include <linux/if_bridge.h>
+#include <linux/iopoll.h>
#include <soc/mscc/ocelot_vcap.h>
#include "ocelot.h"
#include "ocelot_vcap.h"
-#define TABLE_UPDATE_SLEEP_US 10
-#define TABLE_UPDATE_TIMEOUT_US 100000
+#define TABLE_UPDATE_SLEEP_US 10
+#define TABLE_UPDATE_TIMEOUT_US 100000
+#define MEM_INIT_SLEEP_US 1000
+#define MEM_INIT_TIMEOUT_US 100000
+
#define OCELOT_RSV_VLAN_RANGE_START 4000
struct ocelot_mact_entry {
@@ -2708,6 +2712,46 @@ static void ocelot_detect_features(struct ocelot *ocelot)
ocelot->num_frame_refs = QSYS_MMGT_EQ_CTRL_FP_FREE_CNT(eq_ctrl);
}
+static int ocelot_mem_init_status(struct ocelot *ocelot)
+{
+ unsigned int val;
+ int err;
+
+ err = regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],
+ &val);
+
+ return err ?: val;
+}
+
+int ocelot_reset(struct ocelot *ocelot)
+{
+ int err;
+ u32 val;
+
+ err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
+ if (err)
+ return err;
+
+ err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
+ if (err)
+ return err;
+
+ /* MEM_INIT is a self-clearing bit. Wait for it to be cleared (should be
+ * 100us) before enabling the switch core.
+ */
+ err = readx_poll_timeout(ocelot_mem_init_status, ocelot, val, !val,
+ MEM_INIT_SLEEP_US, MEM_INIT_TIMEOUT_US);
+ if (err)
+ return err;
+
+ err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
+ if (err)
+ return err;
+
+ return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
+}
+EXPORT_SYMBOL(ocelot_reset);
+
int ocelot_init(struct ocelot *ocelot)
{
int i, ret;
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 19e5486d1dbd..822b11d33288 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -6,7 +6,6 @@
*/
#include <linux/dsa/ocelot.h>
#include <linux/interrupt.h>
-#include <linux/iopoll.h>
#include <linux/module.h>
#include <linux/of_net.h>
#include <linux/netdevice.h>
@@ -17,6 +16,7 @@
#include <linux/skbuff.h>
#include <net/switchdev.h>
+#include <soc/mscc/ocelot.h>
#include <soc/mscc/ocelot_vcap.h>
#include <soc/mscc/ocelot_hsio.h>
#include <soc/mscc/vsc7514_regs.h>
@@ -26,9 +26,6 @@
#define VSC7514_VCAP_POLICER_BASE 128
#define VSC7514_VCAP_POLICER_MAX 191
-#define MEM_INIT_SLEEP_US 1000
-#define MEM_INIT_TIMEOUT_US 100000
-
static const u32 *ocelot_regmap[TARGET_MAX] = {
[ANA] = vsc7514_ana_regmap,
[QS] = vsc7514_qs_regmap,
@@ -133,45 +130,6 @@ static const struct of_device_id mscc_ocelot_match[] = {
};
MODULE_DEVICE_TABLE(of, mscc_ocelot_match);
-static int ocelot_mem_init_status(struct ocelot *ocelot)
-{
- unsigned int val;
- int err;
-
- err = regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],
- &val);
-
- return err ?: val;
-}
-
-static int ocelot_reset(struct ocelot *ocelot)
-{
- int err;
- u32 val;
-
- err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
- if (err)
- return err;
-
- err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
- if (err)
- return err;
-
- /* MEM_INIT is a self-clearing bit. Wait for it to be cleared (should be
- * 100us) before enabling the switch core.
- */
- err = readx_poll_timeout(ocelot_mem_init_status, ocelot, val, !val,
- MEM_INIT_SLEEP_US, MEM_INIT_TIMEOUT_US);
- if (err)
- return err;
-
- err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
- if (err)
- return err;
-
- return regmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA], 1);
-}
-
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 55bbd5319128..9c1c9b8c43f5 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -1134,6 +1134,7 @@ void ocelot_ptp_rx_timestamp(struct ocelot *ocelot, struct sk_buff *skb,
int ocelot_regfields_init(struct ocelot *ocelot,
const struct reg_field *const regfields);
struct regmap *ocelot_regmap_init(struct ocelot *ocelot, struct resource *res);
+int ocelot_reset(struct ocelot *ocelot);
int ocelot_init(struct ocelot *ocelot);
void ocelot_deinit(struct ocelot *ocelot);
void ocelot_init_port(struct ocelot *ocelot, int port);
--
2.25.1
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 <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
v3
* No changes
v2
* Add Reviewed tag
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 bac0ee9126f8..6d695375b14b 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -42,64 +42,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,
};
@@ -142,7 +84,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
Utilize the existing ocelot MFD interface to add switch functionality to
the Microsemi VSC7512 chip.
Signed-off-by: Colin Foster <[email protected]>
---
v3
* No change
v2
* New patch, broken out from a previous one
---
drivers/mfd/ocelot-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 702555fbdcc5..8b4d813d3139 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -190,6 +190,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-switch",
+ .of_compatible = "mscc,vsc7512-switch",
},
};
--
2.25.1
The ocelot-ext driver is another sub-device of the Ocelot / Felix driver
system, which currently supports the four internal copper phys.
Signed-off-by: Colin Foster <[email protected]>
---
v3
* Remove "currently supported" verbage
The Seville and Felix 9959 all list their supported modes following
the sentence "The following PHY interface types are supported".
During V2, I had used "currently supported" to suggest more interface
modes are around the corner, though this had raised questions.
The suggestion was to drop the entire sentence. I did leave the
modified sentence there because it exactly matches the other two
supported products.
v2
* New patch
---
.../bindings/net/dsa/mscc,ocelot.yaml | 59 +++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
index 8d93ed9c172c..49450a04e589 100644
--- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
@@ -54,9 +54,22 @@ description: |
- phy-mode = "1000base-x": on ports 0, 1, 2, 3
- phy-mode = "2500base-x": on ports 0, 1, 2, 3
+ VSC7412 (Ocelot-Ext):
+
+ The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513,
+ and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS
+ processor that natively support Linux. Additionally, all four devices
+ support control over external interfaces, SPI and PCIe. The Ocelot-Ext
+ driver is for the external control portion.
+
+ The following PHY interface types are supported:
+
+ - phy-mode = "internal": on ports 0, 1, 2, 3
+
properties:
compatible:
enum:
+ - mscc,vsc7512-switch
- mscc,vsc9953-switch
- pci1957,eef0
@@ -258,3 +271,49 @@ examples:
};
};
};
+ # Ocelot-ext VSC7512
+ - |
+ spi {
+ soc@0 {
+ compatible = "mscc,vsc7512";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ ethernet-switch@0 {
+ compatible = "mscc,vsc7512-switch";
+ reg = <0 0>;
+
+ ethernet-ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ label = "cpu";
+ ethernet = <&mac_sw>;
+ phy-handle = <&phy0>;
+ phy-mode = "internal";
+ };
+
+ port@1 {
+ reg = <1>;
+ label = "swp1";
+ phy-mode = "internal";
+ phy-handle = <&phy1>;
+ };
+
+ port@2 {
+ reg = <2>;
+ phy-mode = "internal";
+ phy-handle = <&phy2>;
+ };
+
+ port@3 {
+ reg = <3>;
+ phy-mode = "internal";
+ phy-handle = <&phy3>;
+ };
+ };
+ };
+ };
+ };
--
2.25.1
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 <[email protected]>
---
v2 & v3
* No change
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 6d695375b14b..4fb525f071ac 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -42,10 +42,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
@@ -80,7 +76,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
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 <[email protected]>
---
v3
* No change
v2
* Alignment of variables broken out to a separate patch
* Structs now correctly use EXPORT_SYMBOL*
* Logic moved and comments added to clear up conditionals around
vsc7512_target_io_res[i].start
v1 from previous RFC:
* New patch
---
drivers/mfd/ocelot-core.c | 87 ++++++++++++++++++++++++++++++++++++++
include/linux/mfd/ocelot.h | 5 +++
2 files changed, 92 insertions(+)
diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 013e83173062..702555fbdcc5 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -45,6 +45,45 @@
#define VSC7512_SIO_CTRL_RES_START 0x710700f8
#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,36 @@ 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_NS(vsc7512_target_io_res, MFD_OCELOT);
+
+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"),
+ {}
+};
+EXPORT_SYMBOL_NS(vsc7512_port_io_res, MFD_OCELOT);
+
static const struct mfd_cell vsc7512_devs[] = {
{
.name = "ocelot-pinctrl",
@@ -144,6 +213,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 +221,23 @@ 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 the port_io_res structs need to be referenced directly by
+ * the ocelot_ext driver, so they can't be attached to the dev directly and referenced by
+ * offset like the rest of the drivers. Instead, create these regmaps always and allow any
+ * children look these up by name.
+ */
+ for (i = 0; i < TARGET_MAX; i++)
+ /*
+ * The target_io_res array is sparsely populated. Use .start as an indication that
+ * the entry isn't defined
+ */
+ if (vsc7512_target_io_res[i].start)
+ 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
The macro REG gets defined in at least two locations -
drivers/pinctrl/pinctrl-ocelot.c and include/soc/mscc/ocelot.h. While
pinctrl-ocelot.c doesn't include include/soc/mscc/ocelot.h, it does in fact
include include/linux/mfd/ocelot.h.
This was all fine, until include/linux/mfd/ocelot.h needed resources from
include/soc/mscc/ocelot.h. At that point the REG macro becomes redefined,
and will throw a compiler error.
Undefine the REG macro in drivers/pinctrl/pinctrl-ocelot.c before it is
defined to avoid this error.
Signed-off-by: Colin Foster <[email protected]>
---
v3
* No change
v2
* New patch
---
drivers/pinctrl/pinctrl-ocelot.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 62ce3957abe4..525c6ae898e2 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -1237,6 +1237,7 @@ static int lan966x_pinmux_set_mux(struct pinctrl_dev *pctldev,
return 0;
}
+#undef REG
#define REG(r, info, p) ((r) * (info)->stride + (4 * ((p) / 32)))
static int ocelot_gpio_set_direction(struct pinctrl_dev *pctldev,
--
2.25.1
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 <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
v2 & v3
* No changes
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 d2a9d292160c..07c2f1b6913d 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1082,9 +1082,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,
@@ -1099,7 +1102,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 e4fd5eef57a0..f94a445c2542 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 2ec49e42b3f4..2fd2bb499e9c 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 5b29fa930627..e589d07f84db 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
Add control of an external VSC7512 chip.
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 <[email protected]>
---
v3
* Remove additional entry in vsc7512_port_modes array
* Add MFD_OCELOT namespace import, which is needed for
vsc7512_*_io_res
v2
* Add MAINTAINERS update
* Remove phrase "by way of the ocelot-mfd interface" from the commit
message
* Move MFD resource addition to a separate patch
* Update Kconfig help
* Remove "ocelot_ext_reset()" - it is now shared with ocelot_lib
* Remove unnecessary includes
* Remove "_EXT" from OCELOT_EXT_PORT_MODE_SERDES
* Remove _ext from the compatible string
* Remove no-longer-necessary GCB register definitions
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.
---
MAINTAINERS | 1 +
drivers/net/dsa/ocelot/Kconfig | 19 +++
drivers/net/dsa/ocelot/Makefile | 5 +
drivers/net/dsa/ocelot/ocelot_ext.c | 194 ++++++++++++++++++++++++++++
4 files changed, 219 insertions(+)
create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 1415a1498d68..214b3f836446 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14752,6 +14752,7 @@ M: Colin Foster <[email protected]>
S: Supported
F: Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml
F: drivers/mfd/ocelot*
+F: drivers/net/dsa/ocelot/ocelot_ext.c
F: include/linux/mfd/ocelot.h
OCXL (Open Coherent Accelerator Processor Interface OpenCAPI) DRIVER
diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index 08db9cf76818..74a900e16d76 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -1,4 +1,23 @@
# 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.
+
+ The Ocelot switch family is a set of multi-port networking chips. All
+ of these chips have the ability to be controlled externally through
+ SPI or PCIe interfaces.
+
+ Say "Y" here to enable external control to these chips.
+
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..fb9dbb31fea1
--- /dev/null
+++ b/drivers/net/dsa/ocelot/ocelot_ext.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright 2021-2022 Innovative Advantage Inc.
+ */
+
+#include <linux/mfd/ocelot.h>
+#include <linux/phylink.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <soc/mscc/ocelot.h>
+#include <soc/mscc/vsc7514_regs.h>
+#include "felix.h"
+
+#define VSC7512_NUM_PORTS 11
+
+#define OCELOT_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_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SGMII,
+};
+
+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,
+ [DEV_GMII] = vsc7514_dev_gmii_regmap,
+};
+
+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 dsa_port *dp;
+
+ dp = dsa_to_port(ds, port);
+
+ phylink_generic_validate(&dp->pl_config, supported, state);
+}
+
+static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
+ const char *name)
+{
+ /* In the ocelot-mfd configuration, regmaps are attached to the device
+ * by name alone, so dev_get_regmap will return the requested regmap
+ * without the need to fully define the resource
+ */
+ return dev_get_regmap(ocelot->dev->parent, name);
+}
+
+static const struct ocelot_ops ocelot_ext_ops = {
+ .reset = ocelot_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-switch" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, ocelot_ext_switch_of_match);
+
+static struct platform_driver ocelot_ext_switch_driver = {
+ .driver = {
+ .name = "ocelot-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");
+MODULE_IMPORT_NS(MFD_OCELOT);
--
2.25.1
phylink_generic_validate() requires that mac_capabilities is correctly
populated. While no existing felix 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 <[email protected]>
---
v3
* No change
v2
* Updated commit message to indicate "no existing felix drivers"
instead of "no existing drivers"
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 07c2f1b6913d..a8196cdedcc5 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1050,6 +1050,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
During development, it was believed that a wrapper for ocelot_regmap_init()
would be sufficient for the felix driver to work in non-mmio scenarios.
This was merged in during commit 242bd0c10bbd ("net: dsa: ocelot: felix:
add interface for custom regmaps")
As the external ocelot DSA driver grew closer to an acceptable state, it
was realized that most of the parameters that were passed in from struct
resource *res were useless and ignored. This is due to the fact that the
external ocelot DSA driver utilizes dev_get_regmap(dev, resource->name).
Instead of simply ignoring those parameters, refactor the API to only
require the name as an argument. MMIO scenarios this will reconstruct the
struct resource before calling ocelot_regmap_init(ocelot, resource). MFD
scenarios need only call dev_get_regmap(dev, name).
Signed-off-by: Colin Foster <[email protected]>
---
v3
* Assign match = NULL for the default case
* Don't export felix_init_regmap symbol - the felix.o object is
compiled directly into "mscc_felix-objs" and "mscc_seville-objs"
v2
* New patch
---
drivers/net/dsa/ocelot/felix.c | 58 ++++++++++++++++++------
drivers/net/dsa/ocelot/felix.h | 4 +-
drivers/net/dsa/ocelot/felix_vsc9959.c | 2 +-
drivers/net/dsa/ocelot/seville_vsc9953.c | 2 +-
4 files changed, 48 insertions(+), 18 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index a8196cdedcc5..b01482b24e7a 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1318,11 +1318,48 @@ static int felix_parse_dt(struct felix *felix, phy_interface_t *port_phy_modes)
return err;
}
+struct regmap *felix_init_regmap(struct ocelot *ocelot, const char *name)
+{
+ struct felix *felix = ocelot_to_felix(ocelot);
+ const struct resource *match = NULL;
+ struct resource res;
+ int i;
+
+ for (i = 0; i < TARGET_MAX; i++) {
+ if (!felix->info->target_io_res[i].name)
+ continue;
+
+ if (!strcmp(name, felix->info->target_io_res[i].name)) {
+ match = &felix->info->target_io_res[i];
+ break;
+ }
+ }
+
+ if (!match) {
+ for (i = 0; i < ocelot->num_phys_ports; i++) {
+ if (!strcmp(name, felix->info->port_io_res[i].name)) {
+ match = &felix->info->port_io_res[i];
+ break;
+ }
+ }
+ }
+
+ if (!match)
+ return ERR_PTR(-EINVAL);
+
+ memcpy(&res, match, sizeof(res));
+ res.flags = IORESOURCE_MEM;
+ res.start += felix->switch_base;
+ res.end += felix->switch_base;
+
+ return ocelot_regmap_init(ocelot, &res);
+}
+
static int felix_init_structs(struct felix *felix, int num_phys_ports)
{
struct ocelot *ocelot = &felix->ocelot;
phy_interface_t *port_phy_modes;
- struct resource res;
+ const char *name;
int port, i, err;
ocelot->num_phys_ports = num_phys_ports;
@@ -1358,15 +1395,12 @@ 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;
+ name = felix->info->target_io_res[i].name;
- memcpy(&res, &felix->info->target_io_res[i], sizeof(res));
- res.flags = IORESOURCE_MEM;
- res.start += felix->switch_base;
- res.end += felix->switch_base;
+ if (!name)
+ continue;
- target = felix->info->init_regmap(ocelot, &res);
+ target = felix->info->init_regmap(ocelot, name);
if (IS_ERR(target)) {
dev_err(ocelot->dev,
"Failed to map device memory space\n");
@@ -1398,12 +1432,8 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
return -ENOMEM;
}
- memcpy(&res, &felix->info->port_io_res[port], sizeof(res));
- res.flags = IORESOURCE_MEM;
- res.start += felix->switch_base;
- res.end += felix->switch_base;
-
- target = felix->info->init_regmap(ocelot, &res);
+ name = felix->info->port_io_res[port].name;
+ target = felix->info->init_regmap(ocelot, name);
if (IS_ERR(target)) {
dev_err(ocelot->dev,
"Failed to map memory space for port %d\n",
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index f94a445c2542..e623806eb8ee 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -57,8 +57,7 @@ struct felix_info {
void (*tas_guard_bands_update)(struct ocelot *ocelot, int port);
void (*port_sched_speed_set)(struct ocelot *ocelot, int port,
u32 speed);
- struct regmap *(*init_regmap)(struct ocelot *ocelot,
- struct resource *res);
+ struct regmap *(*init_regmap)(struct ocelot *ocelot, const char *name);
};
/* Methods for initializing the hardware resources specific to a tagging
@@ -97,5 +96,6 @@ struct felix {
struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port);
int felix_netdev_to_port(struct net_device *dev);
+struct regmap *felix_init_regmap(struct ocelot *ocelot, const char *name);
#endif
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 2fd2bb499e9c..e20d5d5d2de9 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -2615,7 +2615,7 @@ static const struct felix_info felix_info_vsc9959 = {
.port_setup_tc = vsc9959_port_setup_tc,
.port_sched_speed_set = vsc9959_sched_speed_set,
.tas_guard_bands_update = vsc9959_tas_guard_bands_update,
- .init_regmap = ocelot_regmap_init,
+ .init_regmap = felix_init_regmap,
};
static irqreturn_t felix_irq_handler(int irq, void *data)
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index e589d07f84db..7c698e19d818 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1079,7 +1079,7 @@ static const struct felix_info seville_info_vsc9953 = {
.mdio_bus_free = vsc9953_mdio_bus_free,
.phylink_validate = vsc9953_phylink_validate,
.port_modes = vsc9953_port_modes,
- .init_regmap = ocelot_regmap_init,
+ .init_regmap = felix_init_regmap,
};
static int seville_probe(struct platform_device *pdev)
--
2.25.1
Hi Colin,
On Sun, Sep 25, 2022 at 05:29:22PM -0700, Colin Foster wrote:
> During development, it was believed that a wrapper for ocelot_regmap_init()
> would be sufficient for the felix driver to work in non-mmio scenarios.
> This was merged in during commit 242bd0c10bbd ("net: dsa: ocelot: felix:
> add interface for custom regmaps")
>
> As the external ocelot DSA driver grew closer to an acceptable state, it
> was realized that most of the parameters that were passed in from struct
> resource *res were useless and ignored. This is due to the fact that the
> external ocelot DSA driver utilizes dev_get_regmap(dev, resource->name).
>
> Instead of simply ignoring those parameters, refactor the API to only
> require the name as an argument. MMIO scenarios this will reconstruct the
> struct resource before calling ocelot_regmap_init(ocelot, resource). MFD
> scenarios need only call dev_get_regmap(dev, name).
>
> Signed-off-by: Colin Foster <[email protected]>
> ---
I don't like how this turned out. I was expecting you not to look at the
exported resources from the ocelot-core anymore - that was kind of the
point of using just the names rather than the whole resource definitions.
I am also sorry for the mess that the felix driver currently is in, and
the fact that some things may have confused you. I will prepare a patch
set which offers an alternative to this, and send it for review.
Hi Vladimir,
On Tue, Sep 27, 2022 at 08:53:53PM +0300, Vladimir Oltean wrote:
> Hi Colin,
>
> On Sun, Sep 25, 2022 at 05:29:22PM -0700, Colin Foster wrote:
> > During development, it was believed that a wrapper for ocelot_regmap_init()
> > would be sufficient for the felix driver to work in non-mmio scenarios.
> > This was merged in during commit 242bd0c10bbd ("net: dsa: ocelot: felix:
> > add interface for custom regmaps")
> >
> > As the external ocelot DSA driver grew closer to an acceptable state, it
> > was realized that most of the parameters that were passed in from struct
> > resource *res were useless and ignored. This is due to the fact that the
> > external ocelot DSA driver utilizes dev_get_regmap(dev, resource->name).
> >
> > Instead of simply ignoring those parameters, refactor the API to only
> > require the name as an argument. MMIO scenarios this will reconstruct the
> > struct resource before calling ocelot_regmap_init(ocelot, resource). MFD
> > scenarios need only call dev_get_regmap(dev, name).
> >
> > Signed-off-by: Colin Foster <[email protected]>
> > ---
>
> I don't like how this turned out. I was expecting you not to look at the
> exported resources from the ocelot-core anymore - that was kind of the
> point of using just the names rather than the whole resource definitions.
I see your point. The init_regmap(name) interface collides with the
*_io_res arrays. Changing the init_regmap() interface doesn't really
change the underlying issue - *_io_res[] is the thing that you're
suggesting to go.
I'm interested to see where this is going. I feel like it might be a
constant names[] array, then felix_vsc9959_init_regmap() where the
specific name <> resource mapping happens. Maybe a common
felix_match_resource_to_name(name, res, len)?
That would definitely remove the need for exporting the
vsc7512_*_io_res[] arrays, which I didn't understand from your v1
review.
Something like:
include/soc/mscc/ocelot.h
#define OCELOT_RES_NAME_ANA "ana"
const char *ocelot_target_names[TARGET_MAX] = {[ANA] = OCELOT_RES_NAME_ANA};
...
drivers/net/dsa/ocelot/felix.c
for (i = 0; i < TARGET_MAX; i++)
target = felix->info->init_regmap(ocelot_target_names[i]);
...
drivers/net/dsa/ocelot/felix_vsc9959.c
static const struct resource vsc9959_target_io_res[TARGET_MAX] = ...;
vsc9959_init_regmap(name)
{
/* more logic for port_io_res, but you get the point */
return felix_init_regmap(name, &vsc9959_target_io_res, TARGET_MAX);
}
>
> I am also sorry for the mess that the felix driver currently is in, and
> the fact that some things may have confused you.
Vladimir, you might be the last person on earth who owes me an apology.
On Tue, Sep 27, 2022 at 11:43:46AM -0700, Colin Foster wrote:
> I see your point. The init_regmap(name) interface collides with the
> *_io_res arrays. Changing the init_regmap() interface doesn't really
> change the underlying issue - *_io_res[] is the thing that you're
> suggesting to go.
>
> I'm interested to see where this is going. I feel like it might be a
> constant names[] array, then felix_vsc9959_init_regmap() where the
> specific name <> resource mapping happens. Maybe a common
> felix_match_resource_to_name(name, res, len)?
>
> That would definitely remove the need for exporting the
> vsc7512_*_io_res[] arrays, which I didn't understand from your v1
> review.
Yes, having an array of strings, meaning which targets are required by
each driver, is what I wanted to see. Isn't that what I said in v1?
> vsc9959_init_regmap(name)
> {
> /* more logic for port_io_res, but you get the point */
> return felix_init_regmap(name, &vsc9959_target_io_res, TARGET_MAX);
> }
Yeah, wait a minute, you'll see.
> > I am also sorry for the mess that the felix driver currently is in, and
> > the fact that some things may have confused you.
>
> Vladimir, you might be the last person on earth who owes me an apology.
I have some more comments on the other patches. This driver looks weird
not only because the hardware is complicated and all over the place, but
also because you're working on a driver (felix) which was designed
around NXP variations of Microchip hardware, and this really transpires
especially around the probing and dt-bindings. The goal, otherwise,
would be for you to have dt-bindings for vsc7512 that are identical to
what Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
provides. It doesn't matter how the driver probes, that is to some
extent independent from how the drivers look like. Anyway, I'm getting
ahead of myself.
On Sun, Sep 25, 2022 at 05:29:27PM -0700, Colin Foster wrote:
> Add control of an external VSC7512 chip.
>
> 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 <[email protected]>
> ---
>
> v3
> * Remove additional entry in vsc7512_port_modes array
> * Add MFD_OCELOT namespace import, which is needed for
> vsc7512_*_io_res
and which hopefully will no longer be
> diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c
> new file mode 100644
> index 000000000000..fb9dbb31fea1
> --- /dev/null
> +++ b/drivers/net/dsa/ocelot/ocelot_ext.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright 2021-2022 Innovative Advantage Inc.
> + */
> +
> +#include <linux/mfd/ocelot.h>
> +#include <linux/phylink.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <soc/mscc/ocelot.h>
> +#include <soc/mscc/vsc7514_regs.h>
> +#include "felix.h"
> +
> +#define VSC7512_NUM_PORTS 11
Is there a difference in port count between VSC7512 and VSC7514? Nope.
Could you please give naming preference to "vsc7514" for things that are
identical?
> +
> +#define OCELOT_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_PORT_MODE_SERDES,
> + OCELOT_PORT_MODE_SERDES,
> + OCELOT_PORT_MODE_SERDES,
> + OCELOT_PORT_MODE_SERDES,
> + OCELOT_PORT_MODE_SERDES,
> + OCELOT_PORT_MODE_SERDES,
> + OCELOT_PORT_MODE_SGMII,
> +};
> +
> +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,
> + [DEV_GMII] = vsc7514_dev_gmii_regmap,
> +};
Isn't this precisely the same as ocelot_regmap from
drivers/net/ethernet/mscc/ocelot_vsc7514.c?
> +
> +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 dsa_port *dp;
> +
> + dp = dsa_to_port(ds, port);
> +
> + phylink_generic_validate(&dp->pl_config, supported, state);
It would be good to transition everybody to phylink_generic_validate(),
now that Sean Anderson's PHY rate matching work was accepted. I haven't
found the time to test this on a LS1028A-QDS board, but I hope I will
soon.
> +}
> +
> +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
> + const char *name)
> +{
> + /* In the ocelot-mfd configuration, regmaps are attached to the device
> + * by name alone, so dev_get_regmap will return the requested regmap
> + * without the need to fully define the resource
> + */
> + return dev_get_regmap(ocelot->dev->parent, name);
As discussed: nope.
> +}
> +
> +static const struct ocelot_ops ocelot_ext_ops = {
> + .reset = ocelot_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);
The pattern was changed again, so can you please delete this line now,
to be in sync with the other drivers?
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
> +
> + 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-switch" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, ocelot_ext_switch_of_match);
> +
> +static struct platform_driver ocelot_ext_switch_driver = {
> + .driver = {
> + .name = "ocelot-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");
> +MODULE_IMPORT_NS(MFD_OCELOT);
> --
> 2.25.1
>
On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote:
> ---
> .../bindings/net/dsa/mscc,ocelot.yaml | 59 +++++++++++++++++++
> 1 file changed, 59 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> index 8d93ed9c172c..49450a04e589 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> @@ -54,9 +54,22 @@ description: |
> - phy-mode = "1000base-x": on ports 0, 1, 2, 3
> - phy-mode = "2500base-x": on ports 0, 1, 2, 3
>
> + VSC7412 (Ocelot-Ext):
VSC7512
> +
> + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513,
> + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS
> + processor that natively support Linux. Additionally, all four devices
> + support control over external interfaces, SPI and PCIe. The Ocelot-Ext
> + driver is for the external control portion.
> +
> + The following PHY interface types are supported:
> +
> + - phy-mode = "internal": on ports 0, 1, 2, 3
More PHY interface types are supported. Please document them all.
It doesn't matter what the driver supports. Drivers and device tree
blobs should be able to have different lifetimes. A driver which doesn't
support the SERDES ports should work with a device tree that defines
them, and a driver that supports the SERDES ports should work with a
device tree that doesn't.
Similar for the other stuff which isn't documented (interrupts, SERDES
PHY handles etc). Since there is already an example with vsc7514, you
know how they need to look, even if they don't work yet on your
hardware, no?
> +
> properties:
> compatible:
> enum:
> + - mscc,vsc7512-switch
> - mscc,vsc9953-switch
> - pci1957,eef0
>
> @@ -258,3 +271,49 @@ examples:
> };
> };
> };
> + # Ocelot-ext VSC7512
> + - |
> + spi {
> + soc@0 {
> + compatible = "mscc,vsc7512";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + ethernet-switch@0 {
> + compatible = "mscc,vsc7512-switch";
> + reg = <0 0>;
What is the idea behind reg = <0 0> here? I would expect this driver to
follow the same conventions as Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml.
The hardware is mostly the same, so the switch portion of the DT bindings
should be mostly plug and play between the switchdev and the DSA variant.
So you can pick the "sys" target as the one giving the address of the
node, and define all targets via "reg" and "reg-names" here.
Like so:
reg = <0x71010000 0x00010000>,
<0x71030000 0x00010000>,
<0x71080000 0x00000100>,
<0x710e0000 0x00010000>,
<0x711e0000 0x00000100>,
<0x711f0000 0x00000100>,
<0x71200000 0x00000100>,
<0x71210000 0x00000100>,
<0x71220000 0x00000100>,
<0x71230000 0x00000100>,
<0x71240000 0x00000100>,
<0x71250000 0x00000100>,
<0x71260000 0x00000100>,
<0x71270000 0x00000100>,
<0x71280000 0x00000100>,
<0x71800000 0x00080000>,
<0x71880000 0x00010000>,
<0x71040000 0x00010000>,
<0x71050000 0x00010000>,
<0x71060000 0x00010000>;
reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
"port2", "port3", "port4", "port5", "port6",
"port7", "port8", "port9", "port10", "qsys",
"ana", "s0", "s1", "s2";
The mfd driver can use these resources or can choose to ignore them, but
I don't see a reason why the dt-bindings should diverge from vsc7514,
its closest cousin.
> +
> + ethernet-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + label = "cpu";
label = "cpu" is not used, please remove.
> + ethernet = <&mac_sw>;
> + phy-handle = <&phy0>;
> + phy-mode = "internal";
> + };
> +
> + port@1 {
> + reg = <1>;
> + label = "swp1";
> + phy-mode = "internal";
> + phy-handle = <&phy1>;
> + };
> +
> + port@2 {
> + reg = <2>;
> + phy-mode = "internal";
> + phy-handle = <&phy2>;
> + };
> +
> + port@3 {
> + reg = <3>;
> + phy-mode = "internal";
> + phy-handle = <&phy3>;
> + };
> + };
> + };
> + };
> + };
> --
> 2.25.1
>
On Sun, Sep 25, 2022 at 05:29:25PM -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 <[email protected]>
> ---
>
> v3
> * No change
>
> v2
> * Alignment of variables broken out to a separate patch
> * Structs now correctly use EXPORT_SYMBOL*
> * Logic moved and comments added to clear up conditionals around
> vsc7512_target_io_res[i].start
>
> v1 from previous RFC:
> * New patch
>
> ---
> drivers/mfd/ocelot-core.c | 87 ++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/ocelot.h | 5 +++
> 2 files changed, 92 insertions(+)
>
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index 013e83173062..702555fbdcc5 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -45,6 +45,45 @@
> #define VSC7512_SIO_CTRL_RES_START 0x710700f8
> #define VSC7512_SIO_CTRL_RES_SIZE 0x00000100
>
> +#define VSC7512_HSIO_RES_START 0x710d0000
> +#define VSC7512_HSIO_RES_SIZE 0x00000128
I don't think you should give the HSIO resource to the switching driver.
In drivers/net/ethernet/mscc/ocelot_vsc7514.c, there is this comment:
static void ocelot_pll5_init(struct ocelot *ocelot)
{
/* Configure PLL5. This will need a proper CCF driver
* The values are coming from the VTSS API for Ocelot
*/
I believe CCF stands for Common Clock Framework.
> +
> +#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
VCAP_RES_SIZE?
> +
> +#define VSC7512_GCB_RES_START 0x71070000
> +#define VSC7512_GCB_RES_SIZE 0x0000022c
Again, I don't think devcpu_gcb should be given to a switching-only
driver. There's nothing switching-related about it.
> +#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,36 @@ 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_NS(vsc7512_target_io_res, MFD_OCELOT);
> +
> +const struct resource vsc7512_port_io_res[] = {
I hope you will merge these 2 arrays now.
> + 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"),
> + {}
> +};
> +EXPORT_SYMBOL_NS(vsc7512_port_io_res, MFD_OCELOT);
> +
> static const struct mfd_cell vsc7512_devs[] = {
> {
> .name = "ocelot-pinctrl",
> @@ -144,6 +213,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 +221,23 @@ 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 the port_io_res structs need to be referenced directly by
> + * the ocelot_ext driver, so they can't be attached to the dev directly and referenced by
> + * offset like the rest of the drivers. Instead, create these regmaps always and allow any
> + * children look these up by name.
> + */
> + for (i = 0; i < TARGET_MAX; i++)
> + /*
> + * The target_io_res array is sparsely populated. Use .start as an indication that
> + * the entry isn't defined
> + */
> + if (vsc7512_target_io_res[i].start)
> + 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);
> +
Will need to be updated.
> 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>
> +
Is this the problematic include that makes it necessary to have the
pinctrl hack? Can we drop the #undef REG now?
> struct resource;
>
> +extern const struct resource vsc7512_target_io_res[TARGET_MAX];
> +extern const struct resource vsc7512_port_io_res[];
> +
Will need to be removed.
> static inline struct regmap *
> ocelot_regmap_from_resource_optional(struct platform_device *pdev,
> unsigned int index,
> --
> 2.25.1
>
On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote:
> On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote:
> > ---
> > .../bindings/net/dsa/mscc,ocelot.yaml | 59 +++++++++++++++++++
> > 1 file changed, 59 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> > index 8d93ed9c172c..49450a04e589 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> > @@ -54,9 +54,22 @@ description: |
> > - phy-mode = "1000base-x": on ports 0, 1, 2, 3
> > - phy-mode = "2500base-x": on ports 0, 1, 2, 3
> >
> > + VSC7412 (Ocelot-Ext):
>
> VSC7512
Oops. Thanks.
>
> > +
> > + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513,
> > + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS
> > + processor that natively support Linux. Additionally, all four devices
> > + support control over external interfaces, SPI and PCIe. The Ocelot-Ext
> > + driver is for the external control portion.
> > +
> > + The following PHY interface types are supported:
> > +
> > + - phy-mode = "internal": on ports 0, 1, 2, 3
>
> More PHY interface types are supported. Please document them all.
> It doesn't matter what the driver supports. Drivers and device tree
> blobs should be able to have different lifetimes. A driver which doesn't
> support the SERDES ports should work with a device tree that defines
> them, and a driver that supports the SERDES ports should work with a
> device tree that doesn't.
>
> Similar for the other stuff which isn't documented (interrupts, SERDES
> PHY handles etc). Since there is already an example with vsc7514, you
> know how they need to look, even if they don't work yet on your
> hardware, no?
Understood. My concern was "oh, all these ports are supported in the
documentation, so they must work" when in actuality they won't. But I
understand DTB compatibility.
This is the same thing Krzysztof was saying as well I belive. I'll
update for v4, with apologies.
>
> > +
> > properties:
> > compatible:
> > enum:
> > + - mscc,vsc7512-switch
> > - mscc,vsc9953-switch
> > - pci1957,eef0
> >
> > @@ -258,3 +271,49 @@ examples:
> > };
> > };
> > };
> > + # Ocelot-ext VSC7512
> > + - |
> > + spi {
> > + soc@0 {
> > + compatible = "mscc,vsc7512";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + ethernet-switch@0 {
> > + compatible = "mscc,vsc7512-switch";
> > + reg = <0 0>;
>
> What is the idea behind reg = <0 0> here? I would expect this driver to
> follow the same conventions as Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml.
> The hardware is mostly the same, so the switch portion of the DT bindings
> should be mostly plug and play between the switchdev and the DSA variant.
> So you can pick the "sys" target as the one giving the address of the
> node, and define all targets via "reg" and "reg-names" here.
>
> Like so:
>
> reg = <0x71010000 0x00010000>,
> <0x71030000 0x00010000>,
> <0x71080000 0x00000100>,
> <0x710e0000 0x00010000>,
> <0x711e0000 0x00000100>,
> <0x711f0000 0x00000100>,
> <0x71200000 0x00000100>,
> <0x71210000 0x00000100>,
> <0x71220000 0x00000100>,
> <0x71230000 0x00000100>,
> <0x71240000 0x00000100>,
> <0x71250000 0x00000100>,
> <0x71260000 0x00000100>,
> <0x71270000 0x00000100>,
> <0x71280000 0x00000100>,
> <0x71800000 0x00080000>,
> <0x71880000 0x00010000>,
> <0x71040000 0x00010000>,
> <0x71050000 0x00010000>,
> <0x71060000 0x00010000>;
> reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
> "port2", "port3", "port4", "port5", "port6",
> "port7", "port8", "port9", "port10", "qsys",
> "ana", "s0", "s1", "s2";
>
> The mfd driver can use these resources or can choose to ignore them, but
> I don't see a reason why the dt-bindings should diverge from vsc7514,
> its closest cousin.
This one I can answer. (from November 2021). Also I'm not saying that my
interpretation is correct. Historically when there are things up for
interpretation, I choose the incorrect path. (case in point... the other
part of this email)
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/#24620755
'''
The thing with putting the targets in the device tree is that you're
inflicting yourself unnecessary pain. Take a look at
Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
they mark the "ptp" target as optional because it wasn't needed when
they first published the device tree, and now they need to maintain
compatibility with those old blobs. To me that is one of the sillier
reasons why you would not support PTP, because you don't know where your
registers are. And that document is not even up to date, it hasn't been
updated when VCAP ES0, IS1, IS2 were added. I don't think that Horatiu
even bothered to maintain backwards compatibility when he initially
added tc-flower offload for VCAP IS2, and as a result, I did not bother
either when extending it for the S0 and S1 targets. At some point
afterwards, the Microchip people even stopped complaining and just went
along with it. (the story is pretty much told from memory, I'm sorry if
I mixed up some facts). It's pretty messy, and that's what you get for
creating these micro-maps of registers spread through the guts of the
SoC and then a separate reg-name for each. When we worked on the device
tree for LS1028A and then T1040, it was very much a conscious decision
for the driver to have a single, big register map and split it up pretty
much in whichever way it wants to. In fact I think we wouldn't be
having the discussion about how to split things right now if we didn't
have that flexibility.
'''
I'm happy to go any way. The two that make the most sense might be:
micro-maps to make the VSC7512 "switch" portion match the VSC7514. The
ethernet switch portion might still have to ignore these...
A 'mega-map' that would also be ignored by the switch. It would be less
arbitrary than the <0 0> that I went with. Maybe something like
<0x70000000 0x02000000> to at least point to some valid region.
>
> > +
> > + ethernet-ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > + label = "cpu";
>
> label = "cpu" is not used, please remove.
Will do. This sounds familiar, so I'm sorry if it fell through the
cracks.
On Wed, Sep 28, 2022 at 12:04:11AM +0300, Vladimir Oltean wrote:
> On Sun, Sep 25, 2022 at 05:29:25PM -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 <[email protected]>
> > ---
> >
> > v3
> > * No change
> >
> > v2
> > * Alignment of variables broken out to a separate patch
> > * Structs now correctly use EXPORT_SYMBOL*
> > * Logic moved and comments added to clear up conditionals around
> > vsc7512_target_io_res[i].start
> >
> > v1 from previous RFC:
> > * New patch
> >
> > ---
> > drivers/mfd/ocelot-core.c | 87 ++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/ocelot.h | 5 +++
> > 2 files changed, 92 insertions(+)
> >
> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > index 013e83173062..702555fbdcc5 100644
> > --- a/drivers/mfd/ocelot-core.c
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -45,6 +45,45 @@
> > #define VSC7512_SIO_CTRL_RES_START 0x710700f8
> > #define VSC7512_SIO_CTRL_RES_SIZE 0x00000100
> >
> > +#define VSC7512_HSIO_RES_START 0x710d0000
> > +#define VSC7512_HSIO_RES_SIZE 0x00000128
>
> I don't think you should give the HSIO resource to the switching driver.
> In drivers/net/ethernet/mscc/ocelot_vsc7514.c, there is this comment:
>
> static void ocelot_pll5_init(struct ocelot *ocelot)
> {
> /* Configure PLL5. This will need a proper CCF driver
> * The values are coming from the VTSS API for Ocelot
> */
>
> I believe CCF stands for Common Clock Framework.
It does stand for common clock framework. And this function / comment
keeps me up at night.
Agreed that the HSIO resource isn't currently used, and should be
dropped from this set. The resource will be brought back in as soon as I
add in phy-ocelot-serdes support during the third and final (?) patch
set of adding 7512 copper ethernet support.
My fear is that the lines:
if (ocelot->targets[HSIO])
ocelot_pll5_init(ocelot);
won't fly inside felix_setup(), and I'm not sure how far that scope will
creep. Maybe it is easier than I suspect.
But I'm getting ahead of myself. I'll remove this for now.
>
> > +
> > +#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
>
> VCAP_RES_SIZE?
I'll change this name to VCAP_RES_SIZE.
>
> > +
> > +#define VSC7512_GCB_RES_START 0x71070000
> > +#define VSC7512_GCB_RES_SIZE 0x0000022c
>
> Again, I don't think devcpu_gcb should be given to a switching-only
> driver. There's nothing switching-related about it.
Yes, this is no longer necessary and I missed this. I think you caught
them all, but I'll do another sweep just in case.
> > +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_NS(vsc7512_target_io_res, MFD_OCELOT);
> > +
> > +const struct resource vsc7512_port_io_res[] = {
>
> I hope you will merge these 2 arrays now.
Yep. And with that I should be able to add them via the standard
.num_resources, .resources method all the other drivers use. As
mentioned, without the GCB and HSIO entries.
> > int ocelot_core_init(struct device *dev)
> > {
> > + const struct resource *port_res;
> > int i, ndevs;
> >
> > ndevs = ARRAY_SIZE(vsc7512_devs);
> > @@ -151,6 +221,23 @@ 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 the port_io_res structs need to be referenced directly by
> > + * the ocelot_ext driver, so they can't be attached to the dev directly and referenced by
> > + * offset like the rest of the drivers. Instead, create these regmaps always and allow any
> > + * children look these up by name.
> > + */
> > + for (i = 0; i < TARGET_MAX; i++)
> > + /*
> > + * The target_io_res array is sparsely populated. Use .start as an indication that
> > + * the entry isn't defined
> > + */
> > + if (vsc7512_target_io_res[i].start)
> > + 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);
> > +
>
> Will need to be updated.
Yep. I think it can all go away for the above
ocelot_core_try_add_regmaps() call now.
>
> > 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>
> > +
>
> Is this the problematic include that makes it necessary to have the
> pinctrl hack? Can we drop the #undef REG now?
Yes, this include was specifically for TARGET_MAX below. That undef REG
should not be necessary anymore. I'll drop it.
>
> > struct resource;
> >
> > +extern const struct resource vsc7512_target_io_res[TARGET_MAX];
> > +extern const struct resource vsc7512_port_io_res[];
> > +
>
> Will need to be removed.
Gladly :-)
>
> > static inline struct regmap *
> > ocelot_regmap_from_resource_optional(struct platform_device *pdev,
> > unsigned int index,
> > --
> > 2.25.1
> >
On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote:
> On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote:
> > ---
> > + - phy-mode = "internal": on ports 0, 1, 2, 3
>
> More PHY interface types are supported. Please document them all.
> It doesn't matter what the driver supports. Drivers and device tree
> blobs should be able to have different lifetimes. A driver which doesn't
> support the SERDES ports should work with a device tree that defines
> them, and a driver that supports the SERDES ports should work with a
> device tree that doesn't.
This will change my patch a little bit then. I didn't undersand this
requirement.
My current device tree has all 8 ethernet ports populated. ocelot_ext
believes "all these port modes are accepted" by way of a fully-populated
vsc7512_port_modes[] array.
As a result, when I'm testing, swp4 through swp7 all enumerate as
devices, though they don't actually function. It isn't until serdes /
phylink / pcs / pll5 come along that they become functional ports.
I doubt this is desired. Though if I'm using the a new macro
OCELOT_PORT_MODE_NONE, felix.c stops after felix_validate_phy_mode.
I think the only thing I can do is to allow felix to ignore invalid phy
modes on some ports (which might be desired) and continue on with the
most it can do. That seems like a potential improvement to the felix
driver...
The other option is to allow the ports to enumerate, but leave them
non-functional. This is how my system currently acts, but as I said, I
bet it would be confusing to any user.
Thoughts?
On Fri, Sep 30, 2022 at 02:15:58PM -0700, Colin Foster wrote:
> On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote:
> > On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote:
> > > ---
> > > + - phy-mode = "internal": on ports 0, 1, 2, 3
> >
> > More PHY interface types are supported. Please document them all.
> > It doesn't matter what the driver supports. Drivers and device tree
> > blobs should be able to have different lifetimes. A driver which doesn't
> > support the SERDES ports should work with a device tree that defines
> > them, and a driver that supports the SERDES ports should work with a
> > device tree that doesn't.
>
> This will change my patch a little bit then. I didn't undersand this
> requirement.
>
> My current device tree has all 8 ethernet ports populated. ocelot_ext
> believes "all these port modes are accepted" by way of a fully-populated
> vsc7512_port_modes[] array.
>
> As a result, when I'm testing, swp4 through swp7 all enumerate as
> devices, though they don't actually function. It isn't until serdes /
> phylink / pcs / pll5 come along that they become functional ports.
>
> I doubt this is desired. Though if I'm using the a new macro
> OCELOT_PORT_MODE_NONE, felix.c stops after felix_validate_phy_mode.
>
> I think the only thing I can do is to allow felix to ignore invalid phy
> modes on some ports (which might be desired) and continue on with the
> most it can do. That seems like a potential improvement to the felix
> driver...
>
> The other option is to allow the ports to enumerate, but leave them
> non-functional. This is how my system currently acts, but as I said, I
> bet it would be confusing to any user.
>
> Thoughts?
>
Also, for what its worth, I tried this just now by making this change:
err = felix_validate_phy_mode(felix, port, phy_mode);
if (err < 0) {
dev_err(dev, "Unsupported PHY mode %s on port %d\n",
phy_modes(phy_mode), port);
of_node_put(child);
- return err;
+ continue;
}
This functions in that I only see swp1-swp3, but I don't think it
should - it is just leaving phy_mode set to 0 (PHY_INTERFACE_MODE_NA).
My guess is it'll need more logic to say "don't add these DSA ports because
the driver doesn't support those PHY interfaces"
[ 3.555367] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 4
[ 3.563551] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 5
[ 3.571570] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 6
[ 3.579459] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 7
[ 4.271832] ocelot-switch ocelot-switch.4.auto: PHY [ocelot-miim0.2.auto-mii:00] driver [Generic PHY] (irq=POLL)
[ 4.282715] ocelot-switch ocelot-switch.4.auto: configuring for phy/internal link mode
[ 4.296478] ocelot-switch ocelot-switch.4.auto swp1 (uninitialized): PHY [ocelot-miim0.2.auto-mii:01] driver [Generic PHY] (irq=POLL)
[ 4.312876] ocelot-switch ocelot-switch.4.auto swp2 (uninitialized): PHY [ocelot-miim0.2.auto-mii:02] driver [Generic PHY] (irq=POLL)
[ 4.328897] ocelot-switch ocelot-switch.4.auto swp3 (uninitialized): PHY [ocelot-miim0.2.auto-mii:03] driver [Generic PHY] (irq=POLL)
[ 5.032849] ocelot-switch ocelot-switch.4.auto swp4 (uninitiailized): validation of qsgmii with support 00000000,00000000,000062ff and advertisement 00000000,00000000,000062ff failed: -EINVAL
[ 5.051265] ocelot-switch ocelot-switch.4.auto swp4 (uninitialized): failed to connect to PHY: -EINVAL
[ 5.060670] ocelot-switch ocelot-switch.4.auto swp4 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 4
(repeated for swp5-7)
On Fri, Sep 30, 2022 at 05:20:22PM -0700, Colin Foster wrote:
> On Fri, Sep 30, 2022 at 02:15:58PM -0700, Colin Foster wrote:
> > On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote:
> > > On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote:
> > > > ---
> > > > + - phy-mode = "internal": on ports 0, 1, 2, 3
> > >
> > > More PHY interface types are supported. Please document them all.
> > > It doesn't matter what the driver supports. Drivers and device tree
> > > blobs should be able to have different lifetimes. A driver which doesn't
> > > support the SERDES ports should work with a device tree that defines
> > > them, and a driver that supports the SERDES ports should work with a
> > > device tree that doesn't.
> >
> > This will change my patch a little bit then. I didn't undersand this
> > requirement.
> >
> > My current device tree has all 8 ethernet ports populated. ocelot_ext
> > believes "all these port modes are accepted" by way of a fully-populated
> > vsc7512_port_modes[] array.
> >
> > As a result, when I'm testing, swp4 through swp7 all enumerate as
> > devices, though they don't actually function. It isn't until serdes /
> > phylink / pcs / pll5 come along that they become functional ports.
> >
> > I doubt this is desired. Though if I'm using the a new macro
> > OCELOT_PORT_MODE_NONE, felix.c stops after felix_validate_phy_mode.
> >
> > I think the only thing I can do is to allow felix to ignore invalid phy
> > modes on some ports (which might be desired) and continue on with the
> > most it can do. That seems like a potential improvement to the felix
> > driver...
> >
> > The other option is to allow the ports to enumerate, but leave them
> > non-functional. This is how my system currently acts, but as I said, I
> > bet it would be confusing to any user.
> >
> > Thoughts?
Having the interfaces probe but not work isn't the worst, but if we
could make just the SERDES ports fail to probe, it would be better.
> Also, for what its worth, I tried this just now by making this change:
>
> err = felix_validate_phy_mode(felix, port, phy_mode);
> if (err < 0) {
> dev_err(dev, "Unsupported PHY mode %s on port %d\n",
> phy_modes(phy_mode), port);
> of_node_put(child);
> - return err;
> + continue;
> }
>
> This functions in that I only see swp1-swp3, but I don't think it
> should - it is just leaving phy_mode set to 0 (PHY_INTERFACE_MODE_NA).
You could add a comment above the "continue" statement explaining this.
> My guess is it'll need more logic to say "don't add these DSA ports because
> the driver doesn't support those PHY interfaces"
>
> [ 3.555367] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 4
> [ 3.563551] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 5
> [ 3.571570] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 6
> [ 3.579459] ocelot-switch ocelot-switch.4.auto: Unsupported PHY mode qsgmii on port 7
> [ 4.271832] ocelot-switch ocelot-switch.4.auto: PHY [ocelot-miim0.2.auto-mii:00] driver [Generic PHY] (irq=POLL)
> [ 4.282715] ocelot-switch ocelot-switch.4.auto: configuring for phy/internal link mode
> [ 4.296478] ocelot-switch ocelot-switch.4.auto swp1 (uninitialized): PHY [ocelot-miim0.2.auto-mii:01] driver [Generic PHY] (irq=POLL)
> [ 4.312876] ocelot-switch ocelot-switch.4.auto swp2 (uninitialized): PHY [ocelot-miim0.2.auto-mii:02] driver [Generic PHY] (irq=POLL)
> [ 4.328897] ocelot-switch ocelot-switch.4.auto swp3 (uninitialized): PHY [ocelot-miim0.2.auto-mii:03] driver [Generic PHY] (irq=POLL)
> [ 5.032849] ocelot-switch ocelot-switch.4.auto swp4 (uninitiailized): validation of qsgmii with support 00000000,00000000,000062ff and advertisement 00000000,00000000,000062ff failed: -EINVAL
> [ 5.051265] ocelot-switch ocelot-switch.4.auto swp4 (uninitialized): failed to connect to PHY: -EINVAL
> [ 5.060670] ocelot-switch ocelot-switch.4.auto swp4 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 4
> (repeated for swp5-7)
I think the behavior is correct and sufficient. The ocelot driver always
requires a valid phy-mode in the device tree for all ports, and
PHY_INTERFACE_MODE_NA means the lack of one. In turn, this is enough to
make phylink_validate() fail with any valid device tree. And DSA is
smart enough to limp on with the rest of its ports if phylink setup
failed for some of them - see dsa_port_setup_as_unused() in the current
net-next git tree.
If you don't think this is enough, you could also patch felix_phylink_get_caps()
to exclude ocelot->ports[port]->phy_mode == PHY_INTERFACE_MODE_NA from
applying this assignment (which would make config->supported_interfaces
remain empty):
__set_bit(ocelot->ports[port]->phy_mode,
config->supported_interfaces);
On 26/09/2022 02:29, Colin Foster wrote:
> The ocelot-ext driver is another sub-device of the Ocelot / Felix driver
> system, which currently supports the four internal copper phys.
>
> Signed-off-by: Colin Foster <[email protected]>
> ---
>
> v3
> * Remove "currently supported" verbage
> The Seville and Felix 9959 all list their supported modes following
> the sentence "The following PHY interface types are supported".
> During V2, I had used "currently supported" to suggest more interface
> modes are around the corner, though this had raised questions.
>
> The suggestion was to drop the entire sentence. I did leave the
> modified sentence there because it exactly matches the other two
> supported products.
>
> v2
> * New patch
>
> ---
> .../bindings/net/dsa/mscc,ocelot.yaml | 59 +++++++++++++++++++
> 1 file changed, 59 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> index 8d93ed9c172c..49450a04e589 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> @@ -54,9 +54,22 @@ description: |
> - phy-mode = "1000base-x": on ports 0, 1, 2, 3
> - phy-mode = "2500base-x": on ports 0, 1, 2, 3
>
> + VSC7412 (Ocelot-Ext):
> +
> + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513,
> + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS
> + processor that natively support Linux. Additionally, all four devices
> + support control over external interfaces, SPI and PCIe. The Ocelot-Ext
> + driver is for the external control portion.
> +
> + The following PHY interface types are supported:
> +
> + - phy-mode = "internal": on ports 0, 1, 2, 3
> +
> properties:
> compatible:
> enum:
> + - mscc,vsc7512-switch
> - mscc,vsc9953-switch
> - pci1957,eef0
>
> @@ -258,3 +271,49 @@ examples:
> };
> };
> };
> + # Ocelot-ext VSC7512
> + - |
> + spi {
> + soc@0 {
soc in spi is a bit confusing.
Does it even pass the tests? You have unit address but no reg.
> + compatible = "mscc,vsc7512";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + ethernet-switch@0 {
> + compatible = "mscc,vsc7512-switch";
> + reg = <0 0>;
0 is the address on which soc bus?
> +
> + ethernet-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + label = "cpu";
> + ethernet = <&mac_sw>;
> + phy-handle = <&phy0>;
> + phy-mode = "internal";
> + };
> +
> + port@1 {
> + reg = <1>;
> + label = "swp1";
> + phy-mode = "internal";
> + phy-handle = <&phy1>;
> + };
> +
> + port@2 {
> + reg = <2>;
> + phy-mode = "internal";
> + phy-handle = <&phy2>;
> + };
> +
> + port@3 {
> + reg = <3>;
> + phy-mode = "internal";
> + phy-handle = <&phy3>;
> + };
How is this example different than previous one (existing soc example)?
If by compatible and number of ports, then there is no much value here.
> + };
> + };
> + };
> + };
Best regards,
Krzysztof
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
> > + # Ocelot-ext VSC7512
> > + - |
> > + spi {
> > + soc@0 {
>
> soc in spi is a bit confusing.
Do you have a better suggestion for a node name? This is effectively a
container for peripherals which would otherwise live under a /soc node,
if they were accessed over MMIO by the internal microprocessor of the
SoC, rather than by an external processor over SPI.
> How is this example different than previous one (existing soc example)?
> If by compatible and number of ports, then there is no much value here.
The positioning relative to the other nodes is what's different.
On 04/10/2022 14:15, Vladimir Oltean wrote:
> On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
>>> + # Ocelot-ext VSC7512
>>> + - |
>>> + spi {
>>> + soc@0 {
>>
>> soc in spi is a bit confusing.
>
> Do you have a better suggestion for a node name? This is effectively a
> container for peripherals which would otherwise live under a /soc node,
/soc node implies it does not live under /spi node. Otherwise it would
be /spi/soc, right?
> if they were accessed over MMIO by the internal microprocessor of the
> SoC, rather than by an external processor over SPI.
>
>> How is this example different than previous one (existing soc example)?
>> If by compatible and number of ports, then there is no much value here.
>
> The positioning relative to the other nodes is what's different.
Positioning of nodes is not worth another example, if everything else is
the same. What is here exactly tested or shown by example? Using a
device in SPI controller?
Best regards,
Krzysztof
On Tue, Oct 04, 2022 at 04:59:02PM +0200, Krzysztof Kozlowski wrote:
> On 04/10/2022 14:15, Vladimir Oltean wrote:
> > On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
> >>> + # Ocelot-ext VSC7512
> >>> + - |
> >>> + spi {
> >>> + soc@0 {
> >>
> >> soc in spi is a bit confusing.
> >
> > Do you have a better suggestion for a node name? This is effectively a
> > container for peripherals which would otherwise live under a /soc node,
>
> /soc node implies it does not live under /spi node. Otherwise it would
> be /spi/soc, right?
Did you read what's written right below? I can explain if you want, but
there's no point if you're not going to read or ask other clarification
questions.
> > if they were accessed over MMIO by the internal microprocessor of the
> > SoC, rather than by an external processor over SPI.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The /spi/soc@0 node actually has a compatible of "mscc,vsc7512" which
Colin did not show in the example (it is not "simple-bus"). It is covered
by Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. Still waiting
for a better suggestion for how to name the mfd container node.
> >> How is this example different than previous one (existing soc example)?
> >> If by compatible and number of ports, then there is no much value here.
> >
> > The positioning relative to the other nodes is what's different.
>
> Positioning of nodes is not worth another example, if everything else is
> the same. What is here exactly tested or shown by example? Using a
> device in SPI controller?
Everything is not the same, it is not the same hardware as what is currenly
covered by Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml.
The "existing soc example" (mscc,vsc9953-switch) has a different port
count, integration with a different SERDES, interrupt controller, pin
controller, things like that. The examples already differ in port count
and phy-mode values, I expect they will start diverging more in the
future. If you still believe it's not worth having an example of how to
instantiate a SPI-controlled VSC7512 because there also exists an
example of an MMIO-controlled VSC9953, then what can I say.
------ cut here ------
Unrelated to your "existing soc example" (the VSC9953), but relevant and
you may want to share your opinion on this:
The same hardware present in the VSC7514 SoC can also be driven by an
integrated MIPS processor, and in that case, it is indeed expected that
the same dt-bindings cover both the /soc and the /spi/soc@0/ relative
positioning of their OF node. This is true for simpler peripherals like
"mscc,ocelot-miim", "mscc,ocelot-pinctrl", "mscc,ocelot-sgpio". However
it is not true for the main switching IP of the SoC itself.
When driven by a switchdev driver, by the internal MIPS processor (the
DMA engine is what is used for packet I/O), the switching IP follows the
Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml binding
document.
When driven by a DSA driver (external processor, host frames are
redirected through an Ethernet port instead of DMA controller),
the switching IP follows the Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
document.
The switching IP is special in this regard because the hardware is not
used in the same way. The DSA dt-binding also needs the 'ethernet'
phandle to be present in a port node. The different placement of the
bindings according to the use case of the hardware is a bit awkward, but
is a direct consequence of the separation between DSA and pure switchdev
drivers that has existed thus far (and the fact that DSA has its own
folder in the dt-bindings, with common properties in dsa.yaml and
dsa-port.yaml etc). It is relatively uncommon for a switching IP to have
provisioning to be used in both modes, and for Linux to support both
modes (using different drivers), yet this is what we have here.
Hi Krzysztof,
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
> On 26/09/2022 02:29, Colin Foster wrote:
> > The ocelot-ext driver is another sub-device of the Ocelot / Felix driver
> > system, which currently supports the four internal copper phys.
> >
> > Signed-off-by: Colin Foster <[email protected]>
...
> > + # Ocelot-ext VSC7512
> > + - |
> > + spi {
> > + soc@0 {
>
> soc in spi is a bit confusing.
>
> Does it even pass the tests? You have unit address but no reg.
I omitted those from the documentation. Rob's bot is usually quick to
alert me when I forgot to run dt_binding_check and something fails
though. I'll double check, but I thought everything passed.
>
> > + compatible = "mscc,vsc7512";
>
>
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + ethernet-switch@0 {
> > + compatible = "mscc,vsc7512-switch";
> > + reg = <0 0>;
>
> 0 is the address on which soc bus?
This one Vladimir brought up as well. The MIPS cousin of this chip
is the VSC7514. They have exactly (or almost exactly) the same hardware,
except the 7514 has an internal MIPS while the 7512 has an 8051.
Both chips can be controlled externally via SPI or PCIe. This is adding
control for the chip via SPI.
For the 7514, you can see there's an array of 20 register ranges that
all get mmap'd to 20 different regmaps.
(Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml)
switch@1010000 {
compatible = "mscc,vsc7514-switch";
reg = <0x1010000 0x10000>,
<0x1030000 0x10000>,
<0x1080000 0x100>,
<0x10e0000 0x10000>,
<0x11e0000 0x100>,
<0x11f0000 0x100>,
<0x1200000 0x100>,
<0x1210000 0x100>,
<0x1220000 0x100>,
<0x1230000 0x100>,
<0x1240000 0x100>,
<0x1250000 0x100>,
<0x1260000 0x100>,
<0x1270000 0x100>,
<0x1280000 0x100>,
<0x1800000 0x80000>,
<0x1880000 0x10000>,
<0x1040000 0x10000>,
<0x1050000 0x10000>,
<0x1060000 0x10000>,
<0x1a0 0x1c4>;
reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
"port2", "port3", "port4", "port5", "port6",
"port7", "port8", "port9", "port10", "qsys",
"ana", "s0", "s1", "s2", "fdma";
The suggestion was to keep the device trees of the 7512 and 7514 as
similar as possible, so this will essentially become:
switch@71010000 {
compatible = "mscc,vsc7512-switch";
reg = <0x71010000 0x10000>,
<0x71030000 0x10000>,
...
On 04/10/2022 18:01, Vladimir Oltean wrote:
> On Tue, Oct 04, 2022 at 04:59:02PM +0200, Krzysztof Kozlowski wrote:
>> On 04/10/2022 14:15, Vladimir Oltean wrote:
>>> On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
>>>>> + # Ocelot-ext VSC7512
>>>>> + - |
>>>>> + spi {
>>>>> + soc@0 {
>>>>
>>>> soc in spi is a bit confusing.
>>>
>>> Do you have a better suggestion for a node name? This is effectively a
>>> container for peripherals which would otherwise live under a /soc node,
>>
>> /soc node implies it does not live under /spi node. Otherwise it would
>> be /spi/soc, right?
>
> Did you read what's written right below? I can explain if you want, but
> there's no point if you're not going to read or ask other clarification
> questions.
>
>>> if they were accessed over MMIO by the internal microprocessor of the
>>> SoC, rather than by an external processor over SPI.
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The /spi/soc@0 node actually has a compatible of "mscc,vsc7512" which
> Colin did not show in the example (it is not "simple-bus"). It is covered
> by Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. Still waiting
> for a better suggestion for how to name the mfd container node.
Then still the /spi node does not seem related. If I understand
correctly, your device described in this bindings is a child of soc@0.
Sounds fine. How that soc@0 is connected to the parent - via SPI or
whatever - is not related to this binding, is it? It is related to the
soc binding, but not here.
>
>>>> How is this example different than previous one (existing soc example)?
>>>> If by compatible and number of ports, then there is no much value here.
>>>
>>> The positioning relative to the other nodes is what's different.
>>
>> Positioning of nodes is not worth another example, if everything else is
>> the same. What is here exactly tested or shown by example? Using a
>> device in SPI controller?
>
> Everything is not the same, it is not the same hardware as what is currenly
> covered by Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml.
> The "existing soc example" (mscc,vsc9953-switch) has a different port
> count, integration with a different SERDES, interrupt controller, pin
> controller, things like that. The examples already differ in port count
> and phy-mode values, I expect they will start diverging more in the
> future. If you still believe it's not worth having an example of how to
> instantiate a SPI-controlled VSC7512 because there also exists an
> example of an MMIO-controlled VSC9953, then what can I say.
>
> ------ cut here ------
>
> Unrelated to your "existing soc example" (the VSC9953), but relevant and
> you may want to share your opinion on this:
>
> The same hardware present in the VSC7514 SoC can also be driven by an
> integrated MIPS processor, and in that case, it is indeed expected that
> the same dt-bindings cover both the /soc and the /spi/soc@0/ relative
> positioning of their OF node. This is true for simpler peripherals like
> "mscc,ocelot-miim", "mscc,ocelot-pinctrl", "mscc,ocelot-sgpio". However
> it is not true for the main switching IP of the SoC itself.
>
> When driven by a switchdev driver, by the internal MIPS processor (the
> DMA engine is what is used for packet I/O), the switching IP follows the
> Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml binding
> document.
>
> When driven by a DSA driver (external processor, host frames are
> redirected through an Ethernet port instead of DMA controller),
> the switching IP follows the Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> document.
>
> The switching IP is special in this regard because the hardware is not
> used in the same way. The DSA dt-binding also needs the 'ethernet'
> phandle to be present in a port node. The different placement of the
> bindings according to the use case of the hardware is a bit awkward, but
> is a direct consequence of the separation between DSA and pure switchdev
> drivers that has existed thus far (and the fact that DSA has its own
> folder in the dt-bindings, with common properties in dsa.yaml and
> dsa-port.yaml etc). It is relatively uncommon for a switching IP to have
> provisioning to be used in both modes, and for Linux to support both
> modes (using different drivers), yet this is what we have here.
Is there a question here to me? What shall I do with this paragraph? You
know, I do not have a problem of lack of material to read...
Best regards,
Krzysztof
On 05/10/2022 02:08, Colin Foster wrote:
> Hi Krzysztof,
>
> On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
>> On 26/09/2022 02:29, Colin Foster wrote:
>>> The ocelot-ext driver is another sub-device of the Ocelot / Felix driver
>>> system, which currently supports the four internal copper phys.
>>>
>>> Signed-off-by: Colin Foster <[email protected]>
> ...
>>> + # Ocelot-ext VSC7512
>>> + - |
>>> + spi {
>>> + soc@0 {
>>
>> soc in spi is a bit confusing.
>>
>> Does it even pass the tests? You have unit address but no reg.
>
> I omitted those from the documentation. Rob's bot is usually quick to
> alert me when I forgot to run dt_binding_check and something fails
> though. I'll double check, but I thought everything passed.
>
>>
>>> + compatible = "mscc,vsc7512";
>>
>>
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> +
>>> + ethernet-switch@0 {
>>> + compatible = "mscc,vsc7512-switch";
>>> + reg = <0 0>;
>>
>> 0 is the address on which soc bus?
>
> This one Vladimir brought up as well. The MIPS cousin of this chip
> is the VSC7514. They have exactly (or almost exactly) the same hardware,
> except the 7514 has an internal MIPS while the 7512 has an 8051.
>
> Both chips can be controlled externally via SPI or PCIe. This is adding
> control for the chip via SPI.
>
> For the 7514, you can see there's an array of 20 register ranges that
> all get mmap'd to 20 different regmaps.
>
> (Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml)
>
> switch@1010000 {
> compatible = "mscc,vsc7514-switch";
> reg = <0x1010000 0x10000>,
> <0x1030000 0x10000>,
> <0x1080000 0x100>,
> <0x10e0000 0x10000>,
> <0x11e0000 0x100>,
> <0x11f0000 0x100>,
> <0x1200000 0x100>,
> <0x1210000 0x100>,
> <0x1220000 0x100>,
> <0x1230000 0x100>,
> <0x1240000 0x100>,
> <0x1250000 0x100>,
> <0x1260000 0x100>,
> <0x1270000 0x100>,
> <0x1280000 0x100>,
> <0x1800000 0x80000>,
> <0x1880000 0x10000>,
> <0x1040000 0x10000>,
> <0x1050000 0x10000>,
> <0x1060000 0x10000>,
> <0x1a0 0x1c4>;
> reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
> "port2", "port3", "port4", "port5", "port6",
> "port7", "port8", "port9", "port10", "qsys",
> "ana", "s0", "s1", "s2", "fdma";
>
>
> The suggestion was to keep the device trees of the 7512 and 7514 as
> similar as possible, so this will essentially become:
> switch@71010000 {
> compatible = "mscc,vsc7512-switch";
> reg = <0x71010000 0x10000>,
> <0x71030000 0x10000>,
> ...
I don't understand how your answer relates to "reg=<0 0>;". How is it
going to become 0x71010000 if there is no other reg/ranges set in parent
nodes. The node has only one IO address, but you say the switch has 20
addresses...
Are we talking about same hardware?
Best regards,
Krzysztof
On Wed, Oct 05, 2022 at 10:03:04AM +0200, Krzysztof Kozlowski wrote:
> On 05/10/2022 02:08, Colin Foster wrote:
> > Hi Krzysztof,
> >
> > On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
> >> On 26/09/2022 02:29, Colin Foster wrote:
> >>> The ocelot-ext driver is another sub-device of the Ocelot / Felix driver
> >>> system, which currently supports the four internal copper phys.
> >>>
> >>> Signed-off-by: Colin Foster <[email protected]>
> > ...
> >>> + # Ocelot-ext VSC7512
> >>> + - |
> >>> + spi {
> >>> + soc@0 {
> >>
> >> soc in spi is a bit confusing.
> >>
> >> Does it even pass the tests? You have unit address but no reg.
> >
> > I omitted those from the documentation. Rob's bot is usually quick to
> > alert me when I forgot to run dt_binding_check and something fails
> > though. I'll double check, but I thought everything passed.
> >
> >>
> >>> + compatible = "mscc,vsc7512";
> >>
> >>
> >>> + #address-cells = <1>;
> >>> + #size-cells = <1>;
> >>> +
> >>> + ethernet-switch@0 {
> >>> + compatible = "mscc,vsc7512-switch";
> >>> + reg = <0 0>;
> >>
> >> 0 is the address on which soc bus?
> >
> > This one Vladimir brought up as well. The MIPS cousin of this chip
> > is the VSC7514. They have exactly (or almost exactly) the same hardware,
> > except the 7514 has an internal MIPS while the 7512 has an 8051.
> >
> > Both chips can be controlled externally via SPI or PCIe. This is adding
> > control for the chip via SPI.
> >
> > For the 7514, you can see there's an array of 20 register ranges that
> > all get mmap'd to 20 different regmaps.
> >
> > (Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml)
> >
> > switch@1010000 {
> > compatible = "mscc,vsc7514-switch";
> > reg = <0x1010000 0x10000>,
> > <0x1030000 0x10000>,
> > <0x1080000 0x100>,
> > <0x10e0000 0x10000>,
> > <0x11e0000 0x100>,
> > <0x11f0000 0x100>,
> > <0x1200000 0x100>,
> > <0x1210000 0x100>,
> > <0x1220000 0x100>,
> > <0x1230000 0x100>,
> > <0x1240000 0x100>,
> > <0x1250000 0x100>,
> > <0x1260000 0x100>,
> > <0x1270000 0x100>,
> > <0x1280000 0x100>,
> > <0x1800000 0x80000>,
> > <0x1880000 0x10000>,
> > <0x1040000 0x10000>,
> > <0x1050000 0x10000>,
> > <0x1060000 0x10000>,
> > <0x1a0 0x1c4>;
> > reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
> > "port2", "port3", "port4", "port5", "port6",
> > "port7", "port8", "port9", "port10", "qsys",
> > "ana", "s0", "s1", "s2", "fdma";
> >
> >
> > The suggestion was to keep the device trees of the 7512 and 7514 as
> > similar as possible, so this will essentially become:
> > switch@71010000 {
> > compatible = "mscc,vsc7512-switch";
> > reg = <0x71010000 0x10000>,
> > <0x71030000 0x10000>,
> > ...
>
> I don't understand how your answer relates to "reg=<0 0>;". How is it
> going to become 0x71010000 if there is no other reg/ranges set in parent
> nodes. The node has only one IO address, but you say the switch has 20
> addresses...
>
> Are we talking about same hardware?
Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps
depending on what capabilities it is to have. In the 7514 they are all
memory-mapped from the device tree. While the 7512 does need these
regmaps, they are managed by the MFD, not the device tree. So there
isn't a _need_ for them to be here, since at the end of the day they're
ignored.
The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I
understand that isn't desired. So moving forward I'll add all the
regmaps back into the device tree.
>
> Best regards,
> Krzysztof
>
On 05/10/2022 17:44, Colin Foster wrote:
> On Wed, Oct 05, 2022 at 10:03:04AM +0200, Krzysztof Kozlowski wrote:
>> On 05/10/2022 02:08, Colin Foster wrote:
>>> Hi Krzysztof,
>>>
>>> On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
>>>> On 26/09/2022 02:29, Colin Foster wrote:
>>>>> The ocelot-ext driver is another sub-device of the Ocelot / Felix driver
>>>>> system, which currently supports the four internal copper phys.
>>>>>
>>>>> Signed-off-by: Colin Foster <[email protected]>
>>> ...
>>>>> + # Ocelot-ext VSC7512
>>>>> + - |
>>>>> + spi {
>>>>> + soc@0 {
>>>>
>>>> soc in spi is a bit confusing.
>>>>
>>>> Does it even pass the tests? You have unit address but no reg.
>>>
>>> I omitted those from the documentation. Rob's bot is usually quick to
>>> alert me when I forgot to run dt_binding_check and something fails
>>> though. I'll double check, but I thought everything passed.
>>>
>>>>
>>>>> + compatible = "mscc,vsc7512";
>>>>
>>>>
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <1>;
>>>>> +
>>>>> + ethernet-switch@0 {
>>>>> + compatible = "mscc,vsc7512-switch";
>>>>> + reg = <0 0>;
>>>>
>>>> 0 is the address on which soc bus?
>>>
>>> This one Vladimir brought up as well. The MIPS cousin of this chip
>>> is the VSC7514. They have exactly (or almost exactly) the same hardware,
>>> except the 7514 has an internal MIPS while the 7512 has an 8051.
>>>
>>> Both chips can be controlled externally via SPI or PCIe. This is adding
>>> control for the chip via SPI.
>>>
>>> For the 7514, you can see there's an array of 20 register ranges that
>>> all get mmap'd to 20 different regmaps.
>>>
>>> (Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml)
>>>
>>> switch@1010000 {
>>> compatible = "mscc,vsc7514-switch";
>>> reg = <0x1010000 0x10000>,
>>> <0x1030000 0x10000>,
>>> <0x1080000 0x100>,
>>> <0x10e0000 0x10000>,
>>> <0x11e0000 0x100>,
>>> <0x11f0000 0x100>,
>>> <0x1200000 0x100>,
>>> <0x1210000 0x100>,
>>> <0x1220000 0x100>,
>>> <0x1230000 0x100>,
>>> <0x1240000 0x100>,
>>> <0x1250000 0x100>,
>>> <0x1260000 0x100>,
>>> <0x1270000 0x100>,
>>> <0x1280000 0x100>,
>>> <0x1800000 0x80000>,
>>> <0x1880000 0x10000>,
>>> <0x1040000 0x10000>,
>>> <0x1050000 0x10000>,
>>> <0x1060000 0x10000>,
>>> <0x1a0 0x1c4>;
>>> reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
>>> "port2", "port3", "port4", "port5", "port6",
>>> "port7", "port8", "port9", "port10", "qsys",
>>> "ana", "s0", "s1", "s2", "fdma";
>>>
>>>
>>> The suggestion was to keep the device trees of the 7512 and 7514 as
>>> similar as possible, so this will essentially become:
>>> switch@71010000 {
>>> compatible = "mscc,vsc7512-switch";
>>> reg = <0x71010000 0x10000>,
>>> <0x71030000 0x10000>,
>>> ...
>>
>> I don't understand how your answer relates to "reg=<0 0>;". How is it
>> going to become 0x71010000 if there is no other reg/ranges set in parent
>> nodes. The node has only one IO address, but you say the switch has 20
>> addresses...
>>
>> Are we talking about same hardware?
>
> Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps
> depending on what capabilities it is to have. In the 7514 they are all
> memory-mapped from the device tree. While the 7512 does need these
> regmaps, they are managed by the MFD, not the device tree. So there
> isn't a _need_ for them to be here, since at the end of the day they're
> ignored.
>
> The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I
> understand that isn't desired. So moving forward I'll add all the
> regmaps back into the device tree.
You need to describe the hardware. If hardware has IO address space, how
does it matter that some driver needs or needs not something?
You mentioned that address space is mapped to regmaps. Regmap is Linux
specific implementation detail, so this does not answer at all about
hardware.
On the other hand, if your DTS design requires this is a child of
something else and by itself it does not have address space, it would be
understandable to skip unit address entirely... but so far it is still
confusing, especially that you use arguments related to implementation
to justify the DTS.
Best regards,
Krzysztof
On Tue, Sep 27, 2022 at 11:26:00PM +0300, Vladimir Oltean wrote:
> On Sun, Sep 25, 2022 at 05:29:26PM -0700, Colin Foster wrote:
> > +
> > + The Ocelot family consists of four devices, the VSC7511, VSC7512, VSC7513,
> > + and the VSC7514. The VSC7513 and VSC7514 both have an internal MIPS
> > + processor that natively support Linux. Additionally, all four devices
> > + support control over external interfaces, SPI and PCIe. The Ocelot-Ext
> > + driver is for the external control portion.
> > +
> > + The following PHY interface types are supported:
> > +
> > + - phy-mode = "internal": on ports 0, 1, 2, 3
>
> More PHY interface types are supported. Please document them all.
> It doesn't matter what the driver supports. Drivers and device tree
> blobs should be able to have different lifetimes. A driver which doesn't
> support the SERDES ports should work with a device tree that defines
> them, and a driver that supports the SERDES ports should work with a
> device tree that doesn't.
>
> Similar for the other stuff which isn't documented (interrupts, SERDES
> PHY handles etc). Since there is already an example with vsc7514, you
> know how they need to look, even if they don't work yet on your
> hardware, no?
>
With regards to the interrupts - I don't really have a concept of how
those will work, since there isn't a processor for those lines to
interrupt. So while there is this for the 7514:
interrupts = <18 21 16>;
interrupt-names = "ptp_rdy", "xtr", "fdma";
it seems like there isn't anything to add there.
That is, unless there's something deeper that is going on that I don't
fully understand yet. It wouldn't be the first time and, realistically,
won't be the last. I'll copy the 7514 for now, as I plan to send out an
RFC shortly with all these updates.
On Fri, Oct 07, 2022 at 01:44:10PM -0700, Colin Foster wrote:
> With regards to the interrupts - I don't really have a concept of how
> those will work, since there isn't a processor for those lines to
> interrupt. So while there is this for the 7514:
>
> interrupts = <18 21 16>;
> interrupt-names = "ptp_rdy", "xtr", "fdma";
>
> it seems like there isn't anything to add there.
>
> That is, unless there's something deeper that is going on that I don't
> fully understand yet. It wouldn't be the first time and, realistically,
> won't be the last. I'll copy the 7514 for now, as I plan to send out an
> RFC shortly with all these updates.
I was under the impression that the interrupt controller could be
configured to route the interrupts to external destinations EXT_DST0 or
EXT_DST1, which have the indices 2 and 3, respectively, in the DST_INTR_*
set of registers of the ICPU_CFG:INTR block. I could be wrong, though,
maybe this is just for PCIe, I never looked at the pinout of this chip
to study whether it's possible to use these as I expect, but normally
for things like PTP TX timestamping, you'd expect that the switch
notifies the external host when a packet has been timestamped and that
timestamp is available in the FIFO. The interrupts out of this switch
could also be useful for the PHY state machine, to disable polling.
Although in the general sense I agree with you, it's better not to add
anything than to add something and be wrong about it. This is where the
limitations start showing for the idea that "device tree describes
hardware, which is independent of software implementation". It's all too
easy to say this when you have an implementation already written.
Anyway. DT doesn't describe hardware, but what software wants to
understand of it, and that makes it inseparable to some degree from
software implementation.
On Tue, Sep 27, 2022 at 03:20:47PM -0700, Colin Foster wrote:
> > The mfd driver can use these resources or can choose to ignore them, but
> > I don't see a reason why the dt-bindings should diverge from vsc7514,
> > its closest cousin.
>
> This one I can answer. (from November 2021). Also I'm not saying that my
> interpretation is correct. Historically when there are things up for
> interpretation, I choose the incorrect path. (case in point... the other
> part of this email)
>
> https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/#24620755
>
> '''
> The thing with putting the targets in the device tree is that you're
> inflicting yourself unnecessary pain. Take a look at
> Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
> they mark the "ptp" target as optional because it wasn't needed when
> they first published the device tree, and now they need to maintain
> compatibility with those old blobs. To me that is one of the sillier
> reasons why you would not support PTP, because you don't know where your
> registers are. And that document is not even up to date, it hasn't been
> updated when VCAP ES0, IS1, IS2 were added. I don't think that Horatiu
> even bothered to maintain backwards compatibility when he initially
> added tc-flower offload for VCAP IS2, and as a result, I did not bother
> either when extending it for the S0 and S1 targets. At some point
> afterwards, the Microchip people even stopped complaining and just went
> along with it. (the story is pretty much told from memory, I'm sorry if
> I mixed up some facts). It's pretty messy, and that's what you get for
> creating these micro-maps of registers spread through the guts of the
> SoC and then a separate reg-name for each. When we worked on the device
> tree for LS1028A and then T1040, it was very much a conscious decision
> for the driver to have a single, big register map and split it up pretty
> much in whichever way it wants to. In fact I think we wouldn't be
> having the discussion about how to split things right now if we didn't
> have that flexibility.
> '''
>
> I'm happy to go any way. The two that make the most sense might be:
>
> micro-maps to make the VSC7512 "switch" portion match the VSC7514. The
> ethernet switch portion might still have to ignore these...
>
> A 'mega-map' that would also be ignored by the switch. It would be less
> arbitrary than the <0 0> that I went with. Maybe something like
> <0x70000000 0x02000000> to at least point to some valid region.
A mega-map for the switch makes a lot more sense to me, if feasible
(it should not overlap with the regions of any other peripherals).
Something isn't quite right to me in having 20 reg-names for a single
device tree node, and I still stand for just describing the whole range
and letting the driver split it up according to its needs. I don't know
why this approach wasn't chosen for the ocelot switch and I did not have
the patience to map out the addresses that the peripherals use in the
Microchip SoCs relative to each other, so see if what I'm proposing is
possible.
But on the other hand this also needs to be balanced with the fact that
one day, someone might come along with a mscc,vsc7514-switch that's SPI
controlled, and expect that the dt-bindings for it in DSA mode expect
the same reg-names that they do in switchdev mode. Or maybe they
wouldn't expect that, I don't know. In any case, for NXP switches I
didn't have a compatibility issue with switchdev-mode Ocelot to concern
myself with, so I went with what made the most sense.
On Wed, Oct 05, 2022 at 10:09:06AM +0200, Krzysztof Kozlowski wrote:
> > The /spi/soc@0 node actually has a compatible of "mscc,vsc7512" which
> > Colin did not show in the example (it is not "simple-bus"). It is covered
> > by Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. Still waiting
> > for a better suggestion for how to name the mfd container node.
>
> Then still the /spi node does not seem related. If I understand
> correctly, your device described in this bindings is a child of soc@0.
> Sounds fine. How that soc@0 is connected to the parent - via SPI or
> whatever - is not related to this binding, is it? It is related to the
> soc binding, but not here.
It's an example, it's meant to be informative. It is the first DSA
driver of its kind. When everybody else ATM puts the ethernet-switch node
under the &spi controller node, this puts it under &spi/soc@<chip-select>/,
for reasons that have to do with scalability. If the examples aren't a
good place to make this more obvious, I don't know why we don't just
tell people to RTFD.
> > Unrelated to your "existing soc example" (the VSC9953), but relevant and
> > you may want to share your opinion on this:
> >
> > The same hardware present in the VSC7514 SoC can also be driven by an
> > integrated MIPS processor, and in that case, it is indeed expected that
> > the same dt-bindings cover both the /soc and the /spi/soc@0/ relative
> > positioning of their OF node. This is true for simpler peripherals like
> > "mscc,ocelot-miim", "mscc,ocelot-pinctrl", "mscc,ocelot-sgpio". However
> > it is not true for the main switching IP of the SoC itself.
> >
> > When driven by a switchdev driver, by the internal MIPS processor (the
> > DMA engine is what is used for packet I/O), the switching IP follows the
> > Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml binding
> > document.
> >
> > When driven by a DSA driver (external processor, host frames are
> > redirected through an Ethernet port instead of DMA controller),
> > the switching IP follows the Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
> > document.
> >
> > The switching IP is special in this regard because the hardware is not
> > used in the same way. The DSA dt-binding also needs the 'ethernet'
> > phandle to be present in a port node. The different placement of the
> > bindings according to the use case of the hardware is a bit awkward, but
> > is a direct consequence of the separation between DSA and pure switchdev
> > drivers that has existed thus far (and the fact that DSA has its own
> > folder in the dt-bindings, with common properties in dsa.yaml and
> > dsa-port.yaml etc). It is relatively uncommon for a switching IP to have
> > provisioning to be used in both modes, and for Linux to support both
> > modes (using different drivers), yet this is what we have here.
>
> Is there a question here to me? What shall I do with this paragraph? You
> know, I do not have a problem of lack of material to read...
For mscc,vsc7514-switch we have a switchdev driver. For mscc,vsc7512-switch,
Colin is working on a DSA driver. Their dt-bindings currently live in
different folders. The mscc,vsc7514-switch can also be used together
with a DSA driver, and support for that will inevitably be added. When
it will, how and where do you recommend the dt-bindings should be added?
In net/dsa/mscc,ocelot.yaml, together with the other switches used in
DSA mode, or in net/mscc,vsc7514-switch.yaml, because its compatible
string already exists there? We can't have a compatible string present
in multiple schemas, right?
This matters because it has implications upon what Colin should do with
the mscc,vsc7512-switch. If your answer to my question is "add $ref: dsa.yaml#
to net/mscc,vsc7514-switch.yaml", then I don't see why we wouldn't do
that now, and wait until the vsc7514 to make that move anyway.
On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote:
> >> I don't understand how your answer relates to "reg=<0 0>;". How is it
> >> going to become 0x71010000 if there is no other reg/ranges set in parent
> >> nodes. The node has only one IO address, but you say the switch has 20
> >> addresses...
> >>
> >> Are we talking about same hardware?
> >
> > Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps
> > depending on what capabilities it is to have. In the 7514 they are all
> > memory-mapped from the device tree. While the 7512 does need these
> > regmaps, they are managed by the MFD, not the device tree. So there
> > isn't a _need_ for them to be here, since at the end of the day they're
> > ignored.
> >
> > The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I
> > understand that isn't desired. So moving forward I'll add all the
> > regmaps back into the device tree.
>
> You need to describe the hardware. If hardware has IO address space, how
> does it matter that some driver needs or needs not something?
What do you mean by IO address space exactly? It is a SPI device with registers.
Does that constitute an IO address space to you?
The driver need matters because you don't usually see DT nodes of SPI,
I2C, MDIO devices describing the address space of their registers, and
having child nodes with unit addresses in that address space. Only when
those devices are so complex that the need arises to identify smaller
building blocks is when you will end up needing that. And this is an
implementation detail which shapes how the dt-bindings will look like.
> You mentioned that address space is mapped to regmaps. Regmap is Linux
> specific implementation detail, so this does not answer at all about
> hardware.
>
> On the other hand, if your DTS design requires this is a child of
> something else and by itself it does not have address space, it would be
> understandable to skip unit address entirely... but so far it is still
> confusing, especially that you use arguments related to implementation
> to justify the DTS.
If Colin skips the unit address entirely, then how could he distinguish
between the otherwise identical MDIO controllers mdio@7107009c and
mdio@710700c0 from Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml?
The ethernet-switch node added here is on the same hierarchical level
with the MDIO controller nodes, so it must have a unit address just like
them.
But I don't support Colin's choice of "reg=<0 0>;" either. A choice must
be made between 2 options:
- mapping all 20 regions of the SPI address space into "reg" values
- mapping a single region from the smallest until the largest address of
those 20, and hope nothing overlaps with some other peripheral, or
worse, that this region will never need to be expanded to the left.
What information do you need to provide some best practices that can be
applied here and are more useful than "you need to describe the
hardware"? Verilog/VHDL is what the hardware description that's
independent of software implementation is, good luck parsing that.
On Sat, Oct 08, 2022 at 01:48:08AM +0300, Vladimir Oltean wrote:
> On Tue, Sep 27, 2022 at 03:20:47PM -0700, Colin Foster wrote:
> > > The mfd driver can use these resources or can choose to ignore them, but
> > > I don't see a reason why the dt-bindings should diverge from vsc7514,
> > > its closest cousin.
> >
> > This one I can answer. (from November 2021). Also I'm not saying that my
> > interpretation is correct. Historically when there are things up for
> > interpretation, I choose the incorrect path. (case in point... the other
> > part of this email)
> >
> > https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/#24620755
> >
> > '''
> > The thing with putting the targets in the device tree is that you're
> > inflicting yourself unnecessary pain. Take a look at
> > Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
> > they mark the "ptp" target as optional because it wasn't needed when
> > they first published the device tree, and now they need to maintain
> > compatibility with those old blobs. To me that is one of the sillier
> > reasons why you would not support PTP, because you don't know where your
> > registers are. And that document is not even up to date, it hasn't been
> > updated when VCAP ES0, IS1, IS2 were added. I don't think that Horatiu
> > even bothered to maintain backwards compatibility when he initially
> > added tc-flower offload for VCAP IS2, and as a result, I did not bother
> > either when extending it for the S0 and S1 targets. At some point
> > afterwards, the Microchip people even stopped complaining and just went
> > along with it. (the story is pretty much told from memory, I'm sorry if
> > I mixed up some facts). It's pretty messy, and that's what you get for
> > creating these micro-maps of registers spread through the guts of the
> > SoC and then a separate reg-name for each. When we worked on the device
> > tree for LS1028A and then T1040, it was very much a conscious decision
> > for the driver to have a single, big register map and split it up pretty
> > much in whichever way it wants to. In fact I think we wouldn't be
> > having the discussion about how to split things right now if we didn't
> > have that flexibility.
> > '''
> >
> > I'm happy to go any way. The two that make the most sense might be:
> >
> > micro-maps to make the VSC7512 "switch" portion match the VSC7514. The
> > ethernet switch portion might still have to ignore these...
> >
> > A 'mega-map' that would also be ignored by the switch. It would be less
> > arbitrary than the <0 0> that I went with. Maybe something like
> > <0x70000000 0x02000000> to at least point to some valid region.
>
> A mega-map for the switch makes a lot more sense to me, if feasible
> (it should not overlap with the regions of any other peripherals).
It does overlap. At least DEVCPU_GCB and HSIO are in the middle of the
mega-map.
I'll stick with the 20-map solution for now. It does invoke this
warning from dt_bindings_check though:
/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.example.dtb: soc@0: ethernet-switch@0:reg: [[1895890944, 65536], [1896022016, 65536], [1896349696, 256], [1896742912, 65536], [1897791488, 256], [1897857024, 256], [1897922560, 256], [1897988096, 256], [1898053632, 256], [1898119168, 256], [1898184704, 256], [1898250240, 256], [1898315776, 256], [1898381312, 256], [1898446848, 256], [1904214016, 524288], [1904738304, 65536], [189087552, 65536], [1896153088, 65536], [1896218624, 65536]] is too long
> Something isn't quite right to me in having 20 reg-names for a single
> device tree node, and I still stand for just describing the whole range
> and letting the driver split it up according to its needs. I don't know
> why this approach wasn't chosen for the ocelot switch and I did not have
> the patience to map out the addresses that the peripherals use in the
> Microchip SoCs relative to each other, so see if what I'm proposing is
> possible.
>
> But on the other hand this also needs to be balanced with the fact that
> one day, someone might come along with a mscc,vsc7514-switch that's SPI
> controlled, and expect that the dt-bindings for it in DSA mode expect
> the same reg-names that they do in switchdev mode. Or maybe they
> wouldn't expect that, I don't know. In any case, for NXP switches I
> didn't have a compatibility issue with switchdev-mode Ocelot to concern
> myself with, so I went with what made the most sense.
On 08/10/2022 01:10, Vladimir Oltean wrote:
> On Wed, Oct 05, 2022 at 10:09:06AM +0200, Krzysztof Kozlowski wrote:
>>> The /spi/soc@0 node actually has a compatible of "mscc,vsc7512" which
>>> Colin did not show in the example (it is not "simple-bus"). It is covered
>>> by Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. Still waiting
>>> for a better suggestion for how to name the mfd container node.
>>
>> Then still the /spi node does not seem related. If I understand
>> correctly, your device described in this bindings is a child of soc@0.
>> Sounds fine. How that soc@0 is connected to the parent - via SPI or
>> whatever - is not related to this binding, is it? It is related to the
>> soc binding, but not here.
>
> It's an example, it's meant to be informative. It is the first DSA
> driver of its kind. When everybody else ATM puts the ethernet-switch node
> under the &spi controller node, this puts it under &spi/soc@<chip-select>/,
> for reasons that have to do with scalability. If the examples aren't a
> good place to make this more obvious, I don't know why we don't just
> tell people to RTFD.
It still does not help me to understand why do you need that &spi. The
device is part of the soc@CS and that's it. Where the soc@ is located,
does not matter for this device, right? The example shows usage of this
device, not of the soc@CS. Bindings for soc@CS should show how to use it
inside spi etc.
>
>>> Unrelated to your "existing soc example" (the VSC9953), but relevant and
>>> you may want to share your opinion on this:
>>>
>>> The same hardware present in the VSC7514 SoC can also be driven by an
>>> integrated MIPS processor, and in that case, it is indeed expected that
>>> the same dt-bindings cover both the /soc and the /spi/soc@0/ relative
>>> positioning of their OF node. This is true for simpler peripherals like
>>> "mscc,ocelot-miim", "mscc,ocelot-pinctrl", "mscc,ocelot-sgpio". However
>>> it is not true for the main switching IP of the SoC itself.
>>>
>>> When driven by a switchdev driver, by the internal MIPS processor (the
>>> DMA engine is what is used for packet I/O), the switching IP follows the
>>> Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml binding
>>> document.
>>>
>>> When driven by a DSA driver (external processor, host frames are
>>> redirected through an Ethernet port instead of DMA controller),
>>> the switching IP follows the Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
>>> document.
>>>
>>> The switching IP is special in this regard because the hardware is not
>>> used in the same way. The DSA dt-binding also needs the 'ethernet'
>>> phandle to be present in a port node. The different placement of the
>>> bindings according to the use case of the hardware is a bit awkward, but
>>> is a direct consequence of the separation between DSA and pure switchdev
>>> drivers that has existed thus far (and the fact that DSA has its own
>>> folder in the dt-bindings, with common properties in dsa.yaml and
>>> dsa-port.yaml etc). It is relatively uncommon for a switching IP to have
>>> provisioning to be used in both modes, and for Linux to support both
>>> modes (using different drivers), yet this is what we have here.
>>
>> Is there a question here to me? What shall I do with this paragraph? You
>> know, I do not have a problem of lack of material to read...
>
> For mscc,vsc7514-switch we have a switchdev driver. For mscc,vsc7512-switch,
> Colin is working on a DSA driver. Their dt-bindings currently live in
> different folders. The mscc,vsc7514-switch can also be used together
> with a DSA driver, and support for that will inevitably be added. When
> it will, how and where do you recommend the dt-bindings should be added?
The bindings should in general describe the hardware, not the Linux
drivers. I assume there is only one VSC7514 device, so there should be
only one binding file. If bindings are correct, then this one hardware
description can be used by two different driver implementations. That's
said, for practical reasons entirely different implementations might
require different bindings, but this should be rather exception
requiring serious reasons.
> In net/dsa/mscc,ocelot.yaml, together with the other switches used in
> DSA mode, or in net/mscc,vsc7514-switch.yaml, because its compatible
> string already exists there? We can't have a compatible string present
> in multiple schemas, right?
You can, if bindings are the same... but then why would you have the
same bindings in two files? Which leads to solution: have only one
binding file.
If bindings are entirely different (and not compatible to each other),
you cannot have same compatible in two different places... and this
leads to paragraph before - there should be only one binding, thus only
one place to document the compatible.
>
> This matters because it has implications upon what Colin should do with
> the mscc,vsc7512-switch. If your answer to my question is "add $ref: dsa.yaml#
> to net/mscc,vsc7514-switch.yaml", then I don't see why we wouldn't do
> that now, and wait until the vsc7514 to make that move anyway.
Best regards,
Krzysztof
On 08/10/2022 02:00, Vladimir Oltean wrote:
> On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote:
>>>> I don't understand how your answer relates to "reg=<0 0>;". How is it
>>>> going to become 0x71010000 if there is no other reg/ranges set in parent
>>>> nodes. The node has only one IO address, but you say the switch has 20
>>>> addresses...
>>>>
>>>> Are we talking about same hardware?
>>>
>>> Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps
>>> depending on what capabilities it is to have. In the 7514 they are all
>>> memory-mapped from the device tree. While the 7512 does need these
>>> regmaps, they are managed by the MFD, not the device tree. So there
>>> isn't a _need_ for them to be here, since at the end of the day they're
>>> ignored.
>>>
>>> The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I
>>> understand that isn't desired. So moving forward I'll add all the
>>> regmaps back into the device tree.
>>
>> You need to describe the hardware. If hardware has IO address space, how
>> does it matter that some driver needs or needs not something?
>
> What do you mean by IO address space exactly? It is a SPI device with registers.
> Does that constitute an IO address space to you?
By IO I meant MMIO (or similar) which resides in reg (thus in unit
address). The SPI devices have only chip-select as reg, AFAIR.
>
> The driver need matters because you don't usually see DT nodes of SPI,
> I2C, MDIO devices describing the address space of their registers, and
> having child nodes with unit addresses in that address space. Only when
> those devices are so complex that the need arises to identify smaller
> building blocks is when you will end up needing that. And this is an
> implementation detail which shapes how the dt-bindings will look like.
So probably I misunderstood here. If this is I2C or SPI device, then of
course reg and unit address do not represent registers.
>
>> You mentioned that address space is mapped to regmaps. Regmap is Linux
>> specific implementation detail, so this does not answer at all about
>> hardware.
>>
>> On the other hand, if your DTS design requires this is a child of
>> something else and by itself it does not have address space, it would be
>> understandable to skip unit address entirely... but so far it is still
>> confusing, especially that you use arguments related to implementation
>> to justify the DTS.
>
> If Colin skips the unit address entirely, then how could he distinguish
> between the otherwise identical MDIO controllers mdio@7107009c and
> mdio@710700c0 from Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml?
> The ethernet-switch node added here is on the same hierarchical level
> with the MDIO controller nodes, so it must have a unit address just like
> them.
So what is @710700c0? It's not chip-select, but MMIO or some other bus
(specific to the device), right?
The mscc,ocelot.yaml has a soc@0 SPI device. Children of soc@0 use unit
addresses/reg meaningful for that soc@0.
>
> But I don't support Colin's choice of "reg=<0 0>;" either. A choice must
> be made between 2 options:
> - mapping all 20 regions of the SPI address space into "reg" values
> - mapping a single region from the smallest until the largest address of
> those 20, and hope nothing overlaps with some other peripheral, or
> worse, that this region will never need to be expanded to the left.
Yeah, at least to my limited knowledge of this hardware.
> What information do you need to provide some best practices that can be
> applied here and are more useful than "you need to describe the
> hardware"?
Describe the hardware properties in terms of it fit in to the whole
system - so some inputs (clocks, GPIOs), outputs (interrupts),
characteristics of a device (e.g. clock provider -> clock cells) and
properties configuring hardware per specific implementation.
But mostly this argument "describe hardware" should be understood like:
do not describe software (Linux drivers) and software policies (driver
choices)...
> Verilog/VHDL is what the hardware description that's
> independent of software implementation is, good luck parsing that.
Best regards,
Krzysztof
On Sun, Oct 09, 2022 at 12:14:22PM -0400, Krzysztof Kozlowski wrote:
> On 08/10/2022 02:00, Vladimir Oltean wrote:
> > On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote:
> >>>> I don't understand how your answer relates to "reg=<0 0>;". How is it
> >>>> going to become 0x71010000 if there is no other reg/ranges set in parent
> >>>> nodes. The node has only one IO address, but you say the switch has 20
> >>>> addresses...
> >>>>
> >>>> Are we talking about same hardware?
> >>>
> >>> Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps
> >>> depending on what capabilities it is to have. In the 7514 they are all
> >>> memory-mapped from the device tree. While the 7512 does need these
> >>> regmaps, they are managed by the MFD, not the device tree. So there
> >>> isn't a _need_ for them to be here, since at the end of the day they're
> >>> ignored.
> >>>
> >>> The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I
> >>> understand that isn't desired. So moving forward I'll add all the
> >>> regmaps back into the device tree.
> >>
> >> You need to describe the hardware. If hardware has IO address space, how
> >> does it matter that some driver needs or needs not something?
> >
> > What do you mean by IO address space exactly? It is a SPI device with registers.
> > Does that constitute an IO address space to you?
>
> By IO I meant MMIO (or similar) which resides in reg (thus in unit
> address). The SPI devices have only chip-select as reg, AFAIR.
Again, the SPI device (soc@0) has a unit address that describes the chip
select, yes. The children of the SPI device have a unit address that
describes the address space of the SPI registers of the mini-peripherals
within that SPI device.
> > The driver need matters because you don't usually see DT nodes of SPI,
> > I2C, MDIO devices describing the address space of their registers, and
> > having child nodes with unit addresses in that address space. Only when
> > those devices are so complex that the need arises to identify smaller
> > building blocks is when you will end up needing that. And this is an
> > implementation detail which shapes how the dt-bindings will look like.
>
> So probably I misunderstood here. If this is I2C or SPI device, then of
> course reg and unit address do not represent registers.
Except we're not talking about the SPI device, I'm reminding you that we
are talking about "reg = <0 0>" which Colin used to describe the
/spi/soc@0/ethernet-switch node.
Colin made the incorrect decision to describe "reg = <0 0>" for the
switch OF node in an attempt to point out that "reg" will *not* be used
by his implementation, whatever value it has. You may want to revisit
some of the things that were said.
What *is* used in the implementation is the array of resources from
struct mfd_cell vsc7512_devs[] in drivers/mfd/ocelot-core.c, because MFD
allows you to do this (and I suppose because it is more convenient than
to rely on the DT). Colin's entire confusion comes from the fact that he
thought it wouldn't be necessary to describe the unit addresses of MFD
children if those addresses won't be retrieved from DT.
> >> You mentioned that address space is mapped to regmaps. Regmap is Linux
> >> specific implementation detail, so this does not answer at all about
> >> hardware.
> >>
> >> On the other hand, if your DTS design requires this is a child of
> >> something else and by itself it does not have address space, it would be
> >> understandable to skip unit address entirely... but so far it is still
> >> confusing, especially that you use arguments related to implementation
> >> to justify the DTS.
> >
> > If Colin skips the unit address entirely, then how could he distinguish
> > between the otherwise identical MDIO controllers mdio@7107009c and
> > mdio@710700c0 from Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml?
> > The ethernet-switch node added here is on the same hierarchical level
> > with the MDIO controller nodes, so it must have a unit address just like
> > them.
>
> So what is @710700c0?
@710700c0 is VSC7512_MIIM1_RES_START, i.e. the base address of the
second MDIO controller embedded within the SoC (accessed over whatever;
SPI or MMIO).
> It's not chip-select, but MMIO or some other bus (specific to the
> device), right?
Yes, it is not chip select. Think of the /spi/soc@0 node as an AHB to
SPI bridge (it is possibly not quite that, but for the sake of imagination
it's a good enough description), and the children of /spi/soc@0 are
nodes whose registers are accessed through that AHB to SPI bridge.
The same addresses can also be accessed via direct MMIO by the processor
*inside* the switch SoC, which in some cases can also run Linux
(although not here in VSC7512, but in VSC7514).
> The mscc,ocelot.yaml has a soc@0 SPI device. Children of soc@0 use unit
> addresses/reg meaningful for that soc@0.
Which they do.
> > But I don't support Colin's choice of "reg=<0 0>;" either. A choice must
> > be made between 2 options:
> > - mapping all 20 regions of the SPI address space into "reg" values
> > - mapping a single region from the smallest until the largest address of
> > those 20, and hope nothing overlaps with some other peripheral, or
> > worse, that this region will never need to be expanded to the left.
>
> Yeah, at least to my limited knowledge of this hardware.
Yeah what? That a decision must be made?
> > What information do you need to provide some best practices that can be
> > applied here and are more useful than "you need to describe the
> > hardware"?
>
> Describe the hardware properties in terms of it fit in to the whole
> system - so some inputs (clocks, GPIOs), outputs (interrupts),
> characteristics of a device (e.g. clock provider -> clock cells) and
> properties configuring hardware per specific implementation.
>
> But mostly this argument "describe hardware" should be understood like:
> do not describe software (Linux drivers) and software policies (driver
> choices)...
Let's bring this back on track. The discussion started with you saying:
| soc in spi is a bit confusing.
which I completely agree with, it really is. But it's also not wrong
(or at least you didn't point out reasons why it would be, despite being
asked to), and very descriptive of what actually takes place here:
SoC registers are being accessed over SPI by an external host.
If you're going to keep giving mechanical review to this, my fear is
that a very complex set of schemas is going to fall through the cracks
of bureaucracy, and while it will end up being formally correct,
absolutely no one will understand what is actually required when
piecing everything together.
In your review of the example provided by Colin here, you first have
this comment about "soc in spi" being confusing, then you seem to forget
everything about that, and ask "How is this example different than
previous one (existing soc example)?"
There are more things to unpack in order to answer that.
The main point is that we wanted to reuse the existing MMIO-based
drivers when accessing the devices over SPI. So the majority of
peripherals have the same dt-bindings whether they are on /soc or on
/spi/soc. Linux also provides us with the mfd and regmap abstractions,
so all is good there. So you are not completely wrong to expect that an
ethernet-switch with the "mscc,vsc7512-switch" compatible string should
have the same bindings regardless of whatever its parent is.
Except this is not actually true, and the risk is that this will appear
as seamless as just that when it actually isn't.
First (and here Colin is also wrong), the switch Colin adds support for
is not directly comparable with "the existing soc example" (vsc9953).
That is different (NXP) hardware which just happens to be supported by
the same driver (drivers/net/dsa/ocelot). It's worth reiterating that
dissimilar hardware driven by a common driver should not necessarily
have the same dt-bindings. Case in point, the NXP switches have a single
(larger) "reg", because the SoC integration was tidier and the switch
doesn't have 20 regions spread out through the SoC's guts, which overlap
with other peripherals as in the case of VSC7512/VSC7514.
Anyway, Colin's SPI-controlled VSC7512 switch is most similar to
mscc,vsc7514-switch.yaml (to the point of the hardware being identical),
and I believe that this is the schema he should append his information to,
rather than what he's currently proposing in his patches.
*But* accessing an Ethernet switch over SPI is not functionally
identical to accessing it over MMIO, unless you want to have an Ethernet
throughput in the order of tens of bits per second.
This is where implementation starts to matter, and while mscc,vsc7514-switch.yaml
describes a switch where packets are sent and received over MMIO (which
wouldn't be feasible over SPI), Colin's VSC7512 schema describes a
switch used in DSA mode (packets are sent/received over a host Ethernet
port, fact which helps overcome the bandwidth limitations of SPI).
To make matters worse, even VSC7514 can be used in DSA mode. When used
in DSA mode, a *different* driver, with *different* dt-bindings (because
of different histories) controls it.
So what must be done is that mscc,vsc7514-switch.yaml must incorporate
*elements* of dsa.yaml, but *only* when it is not accessed using MMIO
(i.e. the Linux on the MIPS VSC7514 doesn't support an external host
driving the switch in DSA mode).
I was kind of expecting this discussion to converge towards ways in
which we can modify mscc,vsc7514-switch.yaml to support a switch used
in DSA mode or in switchdev mode. Most notable in dsa-port.yaml is the
presence of an 'ethernet' phandle, but there are also other subtle
differences, like the 'label' property which mscc,vsc7514-switch.yaml
does not have (and which in the switchdev implementation at
drivers/net/ethernet/mscc/ does not support, either).
On 10/10/2022 09:07, Vladimir Oltean wrote:
> On Sun, Oct 09, 2022 at 12:14:22PM -0400, Krzysztof Kozlowski wrote:
>> On 08/10/2022 02:00, Vladimir Oltean wrote:
>>> On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote:
>>>>>> I don't understand how your answer relates to "reg=<0 0>;". How is it
>>>>>> going to become 0x71010000 if there is no other reg/ranges set in parent
>>>>>> nodes. The node has only one IO address, but you say the switch has 20
>>>>>> addresses...
>>>>>>
>>>>>> Are we talking about same hardware?
>>>>>
>>>>> Yes. The switch driver for both the VSC7512 and VSC7514 use up to ~20 regmaps
>>>>> depending on what capabilities it is to have. In the 7514 they are all
>>>>> memory-mapped from the device tree. While the 7512 does need these
>>>>> regmaps, they are managed by the MFD, not the device tree. So there
>>>>> isn't a _need_ for them to be here, since at the end of the day they're
>>>>> ignored.
>>>>>
>>>>> The "reg=<0 0>;" was my attempt to indicate that they are ignored, but I
>>>>> understand that isn't desired. So moving forward I'll add all the
>>>>> regmaps back into the device tree.
>>>>
>>>> You need to describe the hardware. If hardware has IO address space, how
>>>> does it matter that some driver needs or needs not something?
>>>
>>> What do you mean by IO address space exactly? It is a SPI device with registers.
>>> Does that constitute an IO address space to you?
>>
>> By IO I meant MMIO (or similar) which resides in reg (thus in unit
>> address). The SPI devices have only chip-select as reg, AFAIR.
>
> Again, the SPI device (soc@0) has a unit address that describes the chip
> select, yes. The children of the SPI device have a unit address that
> describes the address space of the SPI registers of the mini-peripherals
> within that SPI device.
>
>>> The driver need matters because you don't usually see DT nodes of SPI,
>>> I2C, MDIO devices describing the address space of their registers, and
>>> having child nodes with unit addresses in that address space. Only when
>>> those devices are so complex that the need arises to identify smaller
>>> building blocks is when you will end up needing that. And this is an
>>> implementation detail which shapes how the dt-bindings will look like.
>>
>> So probably I misunderstood here. If this is I2C or SPI device, then of
>> course reg and unit address do not represent registers.
>
> Except we're not talking about the SPI device, I'm reminding you that we
> are talking about "reg = <0 0>" which Colin used to describe the
> /spi/soc@0/ethernet-switch node.
>
> Colin made the incorrect decision to describe "reg = <0 0>" for the
> switch OF node in an attempt to point out that "reg" will *not* be used
> by his implementation, whatever value it has. You may want to revisit
> some of the things that were said.
>
> What *is* used in the implementation is the array of resources from
> struct mfd_cell vsc7512_devs[] in drivers/mfd/ocelot-core.c, because MFD
> allows you to do this (and I suppose because it is more convenient than
> to rely on the DT). Colin's entire confusion comes from the fact that he
> thought it wouldn't be necessary to describe the unit addresses of MFD
> children if those addresses won't be retrieved from DT.
>
>>>> You mentioned that address space is mapped to regmaps. Regmap is Linux
>>>> specific implementation detail, so this does not answer at all about
>>>> hardware.
>>>>
>>>> On the other hand, if your DTS design requires this is a child of
>>>> something else and by itself it does not have address space, it would be
>>>> understandable to skip unit address entirely... but so far it is still
>>>> confusing, especially that you use arguments related to implementation
>>>> to justify the DTS.
>>>
>>> If Colin skips the unit address entirely, then how could he distinguish
>>> between the otherwise identical MDIO controllers mdio@7107009c and
>>> mdio@710700c0 from Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml?
>>> The ethernet-switch node added here is on the same hierarchical level
>>> with the MDIO controller nodes, so it must have a unit address just like
>>> them.
>>
>> So what is @710700c0?
>
> @710700c0 is VSC7512_MIIM1_RES_START, i.e. the base address of the
> second MDIO controller embedded within the SoC (accessed over whatever;
> SPI or MMIO).
>
>> It's not chip-select, but MMIO or some other bus (specific to the
>> device), right?
>
> Yes, it is not chip select. Think of the /spi/soc@0 node as an AHB to
> SPI bridge (it is possibly not quite that, but for the sake of imagination
> it's a good enough description), and the children of /spi/soc@0 are
> nodes whose registers are accessed through that AHB to SPI bridge.
> The same addresses can also be accessed via direct MMIO by the processor
> *inside* the switch SoC, which in some cases can also run Linux
> (although not here in VSC7512, but in VSC7514).
>
>> The mscc,ocelot.yaml has a soc@0 SPI device. Children of soc@0 use unit
>> addresses/reg meaningful for that soc@0.
>
> Which they do.
>
>>> But I don't support Colin's choice of "reg=<0 0>;" either. A choice must
>>> be made between 2 options:
>>> - mapping all 20 regions of the SPI address space into "reg" values
>>> - mapping a single region from the smallest until the largest address of
>>> those 20, and hope nothing overlaps with some other peripheral, or
>>> worse, that this region will never need to be expanded to the left.
>>
>> Yeah, at least to my limited knowledge of this hardware.
>
> Yeah what? That a decision must be made?
Yep. That's it. You ask me to learn this hardware, read datasheet and
design it instead of Colin or other people working on it. I can give you
generic guidelines how it should look, but that's it.
>
>>> What information do you need to provide some best practices that can be
>>> applied here and are more useful than "you need to describe the
>>> hardware"?
>>
>> Describe the hardware properties in terms of it fit in to the whole
>> system - so some inputs (clocks, GPIOs), outputs (interrupts),
>> characteristics of a device (e.g. clock provider -> clock cells) and
>> properties configuring hardware per specific implementation.
>>
>> But mostly this argument "describe hardware" should be understood like:
>> do not describe software (Linux drivers) and software policies (driver
>> choices)...
>
> Let's bring this back on track. The discussion started with you saying:
>
> | soc in spi is a bit confusing.
>
> which I completely agree with, it really is. But it's also not wrong
> (or at least you didn't point out reasons why it would be, despite being
> asked to), and very descriptive of what actually takes place here:
> SoC registers are being accessed over SPI by an external host.
My comment was not only about this. My comment was about soc@0 not
having reg. And then having ethernet@0 with reg=<0,0> which is unusual,
because - as you explained - it is not a SPI device.
>
> If you're going to keep giving mechanical review to this, my fear is
> that a very complex set of schemas is going to fall through the cracks
> of bureaucracy, and while it will end up being formally correct,
> absolutely no one will understand what is actually required when
> piecing everything together.
>
> In your review of the example provided by Colin here, you first have
> this comment about "soc in spi" being confusing, then you seem to forget
> everything about that, and ask "How is this example different than
> previous one (existing soc example)?"
That one was a different topic, but we stopped discussing it. You
explained the differences and its fine.
>
> There are more things to unpack in order to answer that.
>
> The main point is that we wanted to reuse the existing MMIO-based
> drivers when accessing the devices over SPI. So the majority of
> peripherals have the same dt-bindings whether they are on /soc or on
> /spi/soc. Linux also provides us with the mfd and regmap abstractions,
> so all is good there. So you are not completely wrong to expect that an
> ethernet-switch with the "mscc,vsc7512-switch" compatible string should
> have the same bindings regardless of whatever its parent is.
>
> Except this is not actually true, and the risk is that this will appear
> as seamless as just that when it actually isn't.
>
> First (and here Colin is also wrong), the switch Colin adds support for
> is not directly comparable with "the existing soc example" (vsc9953).
> That is different (NXP) hardware which just happens to be supported by
> the same driver (drivers/net/dsa/ocelot).
If it is different hardware, then you have different compatible, so why
this is a problem?
> It's worth reiterating that
> dissimilar hardware driven by a common driver should not necessarily
> have the same dt-bindings.
Which is obvious...
> Case in point, the NXP switches have a single
> (larger) "reg", because the SoC integration was tidier and the switch
> doesn't have 20 regions spread out through the SoC's guts, which overlap
> with other peripherals as in the case of VSC7512/VSC7514.
>
> Anyway, Colin's SPI-controlled VSC7512 switch is most similar to
> mscc,vsc7514-switch.yaml (to the point of the hardware being identical),
> and I believe that this is the schema he should append his information to,
> rather than what he's currently proposing in his patches.
>
> *But* accessing an Ethernet switch over SPI is not functionally
> identical to accessing it over MMIO, unless you want to have an Ethernet
> throughput in the order of tens of bits per second.
>
> This is where implementation starts to matter, and while mscc,vsc7514-switch.yaml
Not really, implementation still does not matter to the bindings and
that argument proves nothing. No one forces you to model it as SPI in
bindings...
> describes a switch where packets are sent and received over MMIO (which
> wouldn't be feasible over SPI), Colin's VSC7512 schema describes a
> switch used in DSA mode (packets are sent/received over a host Ethernet
> port, fact which helps overcome the bandwidth limitations of SPI).
> To make matters worse, even VSC7514 can be used in DSA mode. When used
> in DSA mode, a *different* driver, with *different* dt-bindings (because
> of different histories) controls it.
The histories also do not matter here - you can deprecate bindings, e.g.
with a new compatible, and write everything a bit more generic (to cover
different setups).
>
> So what must be done is that mscc,vsc7514-switch.yaml must incorporate
> *elements* of dsa.yaml, but *only* when it is not accessed using MMIO
> (i.e. the Linux on the MIPS VSC7514 doesn't support an external host
> driving the switch in DSA mode).
Yes and? You write such stuff like there was any objection from my side...
>
> I was kind of expecting this discussion to converge towards ways in
> which we can modify mscc,vsc7514-switch.yaml to support a switch used
> in DSA mode or in switchdev mode. Most notable in dsa-port.yaml is the
> presence of an 'ethernet' phandle, but there are also other subtle
> differences, like the 'label' property which mscc,vsc7514-switch.yaml
> does not have (and which in the switchdev implementation at
> drivers/net/ethernet/mscc/ does not support, either).
What stops you from doing that? What do you need from me?
Best regards,
Krzysztof
On Mon, Oct 10, 2022 at 09:37:23AM -0400, Krzysztof Kozlowski wrote:
> What stops you from doing that? What do you need from me?
To end the discussion on a constructive note, I think if I were Colin,
I would do the following, in the following order, according to what was
expressed as a constraint:
1. Reword the "driver" word out of mscc,vsc7514-switch.yaml and express
the description in terms of what the switch can do, not what the
driver can do.
2. Make qca8k.yaml have "$ref: dsa.yaml#". Remove "$ref: dsa-port.yaml#"
from the same schema.
3. Remove "- $ref: dsa-port.yaml#" from mediatek,mt7530.yaml. It doesn't
seem to be needed, since dsa.yaml also has this. We need this because
we want to make sure no one except dsa.yaml references dsa-port.yaml.
4. Move the DSA-unspecific portion from dsa.yaml into a new
ethernet-switch.yaml. What remains in dsa.yaml is "dsa,member".
The dsa.yaml schema will have "$ref: ethernet-switch.yaml#" for the
"(ethernet-)switch" node, plus its custom additions.
5. Move the DSA-unspecific portion from dsa-port.yaml into a new
ethernet-switch-port.yaml. What remains in dsa-port.yaml is:
* ethernet phandle
* link phandle
* label property
* dsa-tag-protocol property
* the constraint that CPU and DSA ports must have phylink bindings
6. The ethernet-switch.yaml will have "$ref: ethernet-switch-port.yaml#"
and "$ref: dsa-port.yaml". The dsa-port.yaml schema will *not* have
"$ref: ethernet-switch-port.yaml#", just its custom additions.
I'm not 100% on this, but I think there will be a problem if:
- dsa.yaml references ethernet-switch.yaml
- ethernet-switch.yaml references ethernet-switch-port.yaml
- dsa.yaml also references dsa-port.yaml
- dsa-port.yaml references ethernet-switch-port.yaml
because ethernet-switch-port.yaml will be referenced twice. Again,
not sure if this is a problem. If it isn't, things can be simpler,
just make dsa-port.yaml reference ethernet-switch-port.yaml, and skip
steps 2 and 3 since dsa-port.yaml containing just the DSA specifics
is no longer problematic.
7. Make mscc,vsc7514-switch.yaml have "$ref: ethernet-switch.yaml#" for
the "mscc,vsc7514-switch.yaml" compatible string. This will eliminate
its own definitions for the generic properties: $nodename and
ethernet-ports (~45 lines of code if I'm not mistaken).
8. Introduce the "mscc,vsc7512-switch" compatible string as part of
mscc,vsc7514-switch.yaml, but this will have "$ref: dsa.yaml#" (this
will have to be referenced by full path because they are in different
folders) instead of "ethernet-switch.yaml". Doing this will include
the common bindings for a switch, plus the DSA specifics.
9. Optional: rework ti,cpsw-switch.yaml, microchip,lan966x-switch.yaml,
microchip,sparx5-switch.yaml to have "$ref: ethernet-switch.yaml#"
which should reduce some duplication in existing schemas.
10. Question for future support of VSC7514 in DSA mode: how do we decide
whether to $ref: ethernet-switch.yaml or dsa.yaml? If the parent MFD
node has a compatible string similar to "mscc,vsc7512", then use DSA,
otherwise use generic ethernet-switch?
Colin, how does this sound?
On Mon, Oct 10, 2022 at 08:48:56PM +0300, Vladimir Oltean wrote:
> On Mon, Oct 10, 2022 at 09:37:23AM -0400, Krzysztof Kozlowski wrote:
> > What stops you from doing that? What do you need from me?
>
> To end the discussion on a constructive note, I think if I were Colin,
> I would do the following, in the following order, according to what was
> expressed as a constraint:
>
> 1. Reword the "driver" word out of mscc,vsc7514-switch.yaml and express
> the description in terms of what the switch can do, not what the
> driver can do.
>
> 2. Make qca8k.yaml have "$ref: dsa.yaml#". Remove "$ref: dsa-port.yaml#"
> from the same schema.
>
> 3. Remove "- $ref: dsa-port.yaml#" from mediatek,mt7530.yaml. It doesn't
> seem to be needed, since dsa.yaml also has this. We need this because
> we want to make sure no one except dsa.yaml references dsa-port.yaml.
>
> 4. Move the DSA-unspecific portion from dsa.yaml into a new
> ethernet-switch.yaml. What remains in dsa.yaml is "dsa,member".
> The dsa.yaml schema will have "$ref: ethernet-switch.yaml#" for the
> "(ethernet-)switch" node, plus its custom additions.
>
> 5. Move the DSA-unspecific portion from dsa-port.yaml into a new
> ethernet-switch-port.yaml. What remains in dsa-port.yaml is:
> * ethernet phandle
> * link phandle
> * label property
> * dsa-tag-protocol property
> * the constraint that CPU and DSA ports must have phylink bindings
>
> 6. The ethernet-switch.yaml will have "$ref: ethernet-switch-port.yaml#"
> and "$ref: dsa-port.yaml". The dsa-port.yaml schema will *not* have
> "$ref: ethernet-switch-port.yaml#", just its custom additions.
> I'm not 100% on this, but I think there will be a problem if:
> - dsa.yaml references ethernet-switch.yaml
> - ethernet-switch.yaml references ethernet-switch-port.yaml
> - dsa.yaml also references dsa-port.yaml
> - dsa-port.yaml references ethernet-switch-port.yaml
> because ethernet-switch-port.yaml will be referenced twice. Again,
> not sure if this is a problem. If it isn't, things can be simpler,
> just make dsa-port.yaml reference ethernet-switch-port.yaml, and skip
> steps 2 and 3 since dsa-port.yaml containing just the DSA specifics
> is no longer problematic.
>
> 7. Make mscc,vsc7514-switch.yaml have "$ref: ethernet-switch.yaml#" for
> the "mscc,vsc7514-switch.yaml" compatible string. This will eliminate
> its own definitions for the generic properties: $nodename and
> ethernet-ports (~45 lines of code if I'm not mistaken).
>
> 8. Introduce the "mscc,vsc7512-switch" compatible string as part of
> mscc,vsc7514-switch.yaml, but this will have "$ref: dsa.yaml#" (this
> will have to be referenced by full path because they are in different
> folders) instead of "ethernet-switch.yaml". Doing this will include
> the common bindings for a switch, plus the DSA specifics.
>
> 9. Optional: rework ti,cpsw-switch.yaml, microchip,lan966x-switch.yaml,
> microchip,sparx5-switch.yaml to have "$ref: ethernet-switch.yaml#"
> which should reduce some duplication in existing schemas.
>
> 10. Question for future support of VSC7514 in DSA mode: how do we decide
> whether to $ref: ethernet-switch.yaml or dsa.yaml? If the parent MFD
> node has a compatible string similar to "mscc,vsc7512", then use DSA,
> otherwise use generic ethernet-switch?
>
> Colin, how does this sound?
Thank you for laying this path out for me. Hopefully when I go
heads-down to implement this there won't be any gotchas. It seems pretty
straightforward.
Maybe my only question would be where to send these patches. If these
can all go through net-next it seems like there'd be no issue when step
8 (add 7512 documentation) comes along with this current patch set.
Otherwise this sounds good. I'll switch to getting a patch set of steps
1-7 as you suggest.
On Mon, Oct 10, 2022 at 11:47:09AM -0700, Colin Foster wrote:
> Thank you for laying this path out for me. Hopefully when I go
> heads-down to implement this there won't be any gotchas. It seems pretty
> straightforward.
>
> Maybe my only question would be where to send these patches. If these
> can all go through net-next it seems like there'd be no issue when step
> 8 (add 7512 documentation) comes along with this current patch set.
>
> Otherwise this sounds good. I'll switch to getting a patch set of steps
> 1-7 as you suggest.
Generally patches on dt-bindings go through the subsystem to which they
belong, for example net-next etc. I don't think there are other dependencies?
On Mon, Oct 10, 2022 at 11:47:09AM -0700, Colin Foster wrote:
> Thank you for laying this path out for me. Hopefully when I go
> heads-down to implement this there won't be any gotchas. It seems pretty
> straightforward.
>
> Maybe my only question would be where to send these patches. If these
> can all go through net-next it seems like there'd be no issue when step
> 8 (add 7512 documentation) comes along with this current patch set.
>
> Otherwise this sounds good. I'll switch to getting a patch set of steps
> 1-7 as you suggest.
I have some doubts whether it would be good to also merge net/dsa/mscc,ocelot.yaml
(i.e. the NXP variants) into net/mscc,vsc7514-switch.yaml. A few "if" statements
on compatible string which decide the format of "reg" and of "reg-names" should cover
the differences. Not sure how the end result will look like.
On Mon, Oct 10, 2022 at 08:48:56PM +0300, Vladimir Oltean wrote:
> On Mon, Oct 10, 2022 at 09:37:23AM -0400, Krzysztof Kozlowski wrote:
> > What stops you from doing that? What do you need from me?
>
> To end the discussion on a constructive note, I think if I were Colin,
> I would do the following, in the following order, according to what was
> expressed as a constraint:
>
...
> 8. Introduce the "mscc,vsc7512-switch" compatible string as part of
> mscc,vsc7514-switch.yaml, but this will have "$ref: dsa.yaml#" (this
> will have to be referenced by full path because they are in different
> folders) instead of "ethernet-switch.yaml". Doing this will include
> the common bindings for a switch, plus the DSA specifics.
Resurrecting this conversation for a quick question / feedback, now that
steps 1-7 are essentially done with everyone's help.
I don't want to send out a full RFC / Patch, since I can't currently
test on hardware this week. But I'd really like feedback on the
documentation change that is coming up. And I also don't want to
necessarily do a separate RFC for just this patch.
What happens here is interrupts and interrupt-names work as expected.
They're required for the 7514, and optional for the 7512. Fantastic.
I'm not sure if the "$ref: ethernet-switch.yaml" and
"$ref: /schemas/net/dsa/dsa.yaml#" have an effect, since removing that
line outright doesn't seem to have an effect on dt_bindings_check.
The "fdma" doesn't make sense for the 7512, and seems to be handled
correctly by way of maxItems for the two scenarios.
The big miss in this patch is ethernet-switch-port vs dsa-port in the
two scenarios. It isn't working as I'd hoped, where the 7514 pulls in
ethernet-switch-port.yaml and the 7512 pulls in dsa-port.yaml. To squash
errors about the incorrect "ethernet" property I switched this line:
- $ref: ethernet-switch-port.yaml#
+ $ref: /schemas/net/dsa/dsa-port.yaml#
... knowing full well that the correct solution should be along the
lines of "remove this, and only reference them in the conditional". That
doesn't seem to work though...
Is what I'm trying to do possible? I utilized
Documentation/devicetree/bindings/net/dsa/*.yaml and
Documentation/devicetree/bindings/net/*.yaml and found examples to get
to my current state.
diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
index 5ffe831e59e4..f012c64a0da3 100644
--- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
+++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
@@ -18,13 +18,50 @@ description: |
packets using CPU. Additionally, PTP is supported as well as FDMA for faster
packet extraction/injection.
-$ref: ethernet-switch.yaml#
+allOf:
+ - if:
+ properties:
+ compatible:
+ const: mscc,vsc7514-switch
+ then:
+ $ref: ethernet-switch.yaml#
+ required:
+ - interrupts
+ - interrupt-names
+ properties:
+ reg:
+ minItems: 21
+ reg-names:
+ minItems: 21
+ ethernet-ports:
+ patternProperties:
+ "^port@[0-9a-f]+$":
+ $ref: ethernet-switch-port.yaml#
+
+ - if:
+ properties:
+ compatible:
+ const: mscc,vsc7512-switch
+ then:
+ $ref: /schemas/net/dsa/dsa.yaml#
+ properties:
+ reg:
+ maxItems: 20
+ reg-names:
+ maxItems: 20
+ ethernet-ports:
+ patternProperties:
+ "^port@[0-9a-f]+$":
+ $ref: /schemas/net/dsa/dsa-port.yaml#
properties:
compatible:
- const: mscc,vsc7514-switch
+ enum:
+ - mscc,vsc7512-switch
+ - mscc,vsc7514-switch
reg:
+ minItems: 20
items:
- description: system target
- description: rewriter target
@@ -49,6 +86,7 @@ properties:
- description: fdma target
reg-names:
+ minItems: 20
items:
- const: sys
- const: rew
@@ -100,7 +138,7 @@ properties:
patternProperties:
"^port@[0-9a-f]+$":
- $ref: ethernet-switch-port.yaml#
+ $ref: /schemas/net/dsa/dsa-port.yaml#
unevaluatedProperties: false
@@ -108,13 +146,12 @@ required:
- compatible
- reg
- reg-names
- - interrupts
- - interrupt-names
- ethernet-ports
additionalProperties: false
examples:
+ # VSC7514 (Switchdev)
- |
switch@1010000 {
compatible = "mscc,vsc7514-switch";
@@ -162,5 +199,51 @@ examples:
};
};
};
+ # VSC7512 (DSA)
+ - |
+ ethernet-switch@1{
+ compatible = "mscc,vsc7512-switch";
+ reg = <0x71010000 0x10000>,
+ <0x71030000 0x10000>,
+ <0x71080000 0x100>,
+ <0x710e0000 0x10000>,
+ <0x711e0000 0x100>,
+ <0x711f0000 0x100>,
+ <0x71200000 0x100>,
+ <0x71210000 0x100>,
+ <0x71220000 0x100>,
+ <0x71230000 0x100>,
+ <0x71240000 0x100>,
+ <0x71250000 0x100>,
+ <0x71260000 0x100>,
+ <0x71270000 0x100>,
+ <0x71280000 0x100>,
+ <0x71800000 0x80000>,
+ <0x71880000 0x10000>,
+ <0x71040000 0x10000>,
+ <0x71050000 0x10000>,
+ <0x71060000 0x10000>;
+ reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
+ "port2", "port3", "port4", "port5", "port6",
+ "port7", "port8", "port9", "port10", "qsys",
+ "ana", "s0", "s1", "s2";
+
+ ethernet-ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ ethernet = <&mac_sw>;
+ phy-handle = <&phy0>;
+ phy-mode = "internal";
+ };
+ port@1 {
+ reg = <1>;
+ phy-handle = <&phy1>;
+ phy-mode = "internal";
+ };
+ };
+ };
Hi Colin,
On Wed, Jan 18, 2023 at 12:28:36PM -1000, Colin Foster wrote:
> Resurrecting this conversation for a quick question / feedback, now that
> steps 1-7 are essentially done with everyone's help.
>
> I don't want to send out a full RFC / Patch, since I can't currently
> test on hardware this week. But I'd really like feedback on the
> documentation change that is coming up. And I also don't want to
> necessarily do a separate RFC for just this patch.
>
> What happens here is interrupts and interrupt-names work as expected.
> They're required for the 7514, and optional for the 7512. Fantastic.
>
> I'm not sure if the "$ref: ethernet-switch.yaml" and
> "$ref: /schemas/net/dsa/dsa.yaml#" have an effect, since removing that
> line outright doesn't seem to have an effect on dt_bindings_check.
>
> The "fdma" doesn't make sense for the 7512, and seems to be handled
> correctly by way of maxItems for the two scenarios.
>
>
> The big miss in this patch is ethernet-switch-port vs dsa-port in the
> two scenarios. It isn't working as I'd hoped, where the 7514 pulls in
> ethernet-switch-port.yaml and the 7512 pulls in dsa-port.yaml. To squash
> errors about the incorrect "ethernet" property I switched this line:
>
> - $ref: ethernet-switch-port.yaml#
> + $ref: /schemas/net/dsa/dsa-port.yaml#
>
> ... knowing full well that the correct solution should be along the
> lines of "remove this, and only reference them in the conditional". That
> doesn't seem to work though...
>
> Is what I'm trying to do possible? I utilized
> Documentation/devicetree/bindings/net/dsa/*.yaml and
> Documentation/devicetree/bindings/net/*.yaml and found examples to get
> to my current state.
>
>
> diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
> index 5ffe831e59e4..f012c64a0da3 100644
> --- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
> +++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
> @@ -18,13 +18,50 @@ description: |
> packets using CPU. Additionally, PTP is supported as well as FDMA for faster
> packet extraction/injection.
>
> -$ref: ethernet-switch.yaml#
> +allOf:
> + - if:
> + properties:
> + compatible:
> + const: mscc,vsc7514-switch
> + then:
> + $ref: ethernet-switch.yaml#
> + required:
> + - interrupts
> + - interrupt-names
> + properties:
> + reg:
> + minItems: 21
> + reg-names:
> + minItems: 21
> + ethernet-ports:
> + patternProperties:
> + "^port@[0-9a-f]+$":
> + $ref: ethernet-switch-port.yaml#
> +
> + - if:
> + properties:
> + compatible:
> + const: mscc,vsc7512-switch
> + then:
> + $ref: /schemas/net/dsa/dsa.yaml#
> + properties:
> + reg:
> + maxItems: 20
> + reg-names:
> + maxItems: 20
> + ethernet-ports:
> + patternProperties:
> + "^port@[0-9a-f]+$":
> + $ref: /schemas/net/dsa/dsa-port.yaml#
>
> properties:
> compatible:
> - const: mscc,vsc7514-switch
> + enum:
> + - mscc,vsc7512-switch
> + - mscc,vsc7514-switch
>
> reg:
> + minItems: 20
> items:
> - description: system target
> - description: rewriter target
> @@ -49,6 +86,7 @@ properties:
> - description: fdma target
>
> reg-names:
> + minItems: 20
> items:
> - const: sys
> - const: rew
> @@ -100,7 +138,7 @@ properties:
> patternProperties:
> "^port@[0-9a-f]+$":
>
> - $ref: ethernet-switch-port.yaml#
> + $ref: /schemas/net/dsa/dsa-port.yaml#
>
> unevaluatedProperties: false
I'm not sure at all why this chunk (is sub-schema the right word) even
exists, considering you have the other one?!
>
> @@ -108,13 +146,12 @@ required:
> - compatible
> - reg
> - reg-names
> - - interrupts
> - - interrupt-names
> - ethernet-ports
>
> additionalProperties: false
This should be "unevaluatedProperties: false" I guess? Maybe this is why
deleting the ethernet-switch.yaml or dsa.yaml schema appears to do nothing?
The following delta compared to net-next works for me, I think:
diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
index 5ffe831e59e4..dc3319ea40b9 100644
--- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
+++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
@@ -18,13 +18,52 @@ description: |
packets using CPU. Additionally, PTP is supported as well as FDMA for faster
packet extraction/injection.
-$ref: ethernet-switch.yaml#
+allOf:
+ - if:
+ properties:
+ compatible:
+ const: mscc,vsc7514-switch
+ then:
+ $ref: ethernet-switch.yaml#
+ required:
+ - interrupts
+ - interrupt-names
+ properties:
+ reg:
+ minItems: 21
+ reg-names:
+ minItems: 21
+ ethernet-ports:
+ patternProperties:
+ "^port@[0-9a-f]+$":
+ $ref: ethernet-switch-port.yaml#
+ unevaluatedProperties: false
+
+ - if:
+ properties:
+ compatible:
+ const: mscc,vsc7512-switch
+ then:
+ $ref: /schemas/net/dsa/dsa.yaml#
+ properties:
+ reg:
+ maxItems: 20
+ reg-names:
+ maxItems: 20
+ ethernet-ports:
+ patternProperties:
+ "^port@[0-9a-f]+$":
+ $ref: /schemas/net/dsa/dsa-port.yaml#
+ unevaluatedProperties: false
properties:
compatible:
- const: mscc,vsc7514-switch
+ enum:
+ - mscc,vsc7512-switch
+ - mscc,vsc7514-switch
reg:
+ minItems: 20
items:
- description: system target
- description: rewriter target
@@ -49,6 +88,7 @@ properties:
- description: fdma target
reg-names:
+ minItems: 20
items:
- const: sys
- const: rew
@@ -86,35 +126,16 @@ properties:
- const: xtr
- const: fdma
- ethernet-ports:
- type: object
-
- properties:
- '#address-cells':
- const: 1
- '#size-cells':
- const: 0
-
- additionalProperties: false
-
- patternProperties:
- "^port@[0-9a-f]+$":
-
- $ref: ethernet-switch-port.yaml#
-
- unevaluatedProperties: false
-
required:
- compatible
- reg
- reg-names
- - interrupts
- - interrupt-names
- ethernet-ports
-additionalProperties: false
+unevaluatedProperties: false
examples:
+ # VSC7514 (Switchdev)
- |
switch@1010000 {
compatible = "mscc,vsc7514-switch";
@@ -154,6 +175,7 @@ examples:
reg = <0>;
phy-handle = <&phy0>;
phy-mode = "internal";
+ ethernet = <&mac_sw>; # fails validation as expected
};
port1: port@1 {
reg = <1>;
@@ -162,5 +184,51 @@ examples:
};
};
};
+ # VSC7512 (DSA)
+ - |
+ ethernet-switch@1{
+ compatible = "mscc,vsc7512-switch";
+ reg = <0x71010000 0x10000>,
+ <0x71030000 0x10000>,
+ <0x71080000 0x100>,
+ <0x710e0000 0x10000>,
+ <0x711e0000 0x100>,
+ <0x711f0000 0x100>,
+ <0x71200000 0x100>,
+ <0x71210000 0x100>,
+ <0x71220000 0x100>,
+ <0x71230000 0x100>,
+ <0x71240000 0x100>,
+ <0x71250000 0x100>,
+ <0x71260000 0x100>,
+ <0x71270000 0x100>,
+ <0x71280000 0x100>,
+ <0x71800000 0x80000>,
+ <0x71880000 0x10000>,
+ <0x71040000 0x10000>,
+ <0x71050000 0x10000>,
+ <0x71060000 0x10000>;
+ reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
+ "port2", "port3", "port4", "port5", "port6",
+ "port7", "port8", "port9", "port10", "qsys",
+ "ana", "s0", "s1", "s2";
+
+ ethernet-ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ ethernet = <&mac_sw>;
+ phy-handle = <&phy0>;
+ phy-mode = "internal";
+ };
+ port@1 {
+ reg = <1>;
+ phy-handle = <&phy1>;
+ phy-mode = "internal";
+ };
+ };
+ };
...
Of course this is a completely uneducated attempt on my part.
On Thu, Jan 19, 2023 at 10:21:13PM +0200, Vladimir Oltean wrote:
> Hi Colin,
>
> On Wed, Jan 18, 2023 at 12:28:36PM -1000, Colin Foster wrote:
>
> - ethernet-ports:
> - type: object
> -
> - properties:
> - '#address-cells':
> - const: 1
> - '#size-cells':
> - const: 0
> -
> - additionalProperties: false
> -
> - patternProperties:
> - "^port@[0-9a-f]+$":
> -
> - $ref: ethernet-switch-port.yaml#
> -
> - unevaluatedProperties: false
> -
I think removing this entire section was the one thing I didn't try. And
sure enough - it seems to work exactly as I'd hope! Thanks!
Next week I'll do some verification and will hopefully get the next
patch set sent out.
> required:
> - compatible
> - reg
> - reg-names
> - - interrupts
> - - interrupt-names
> - ethernet-ports
>
> -additionalProperties: false
> +unevaluatedProperties: false
>
> examples:
> + # VSC7514 (Switchdev)
> - |
> switch@1010000 {
> compatible = "mscc,vsc7514-switch";
> @@ -154,6 +175,7 @@ examples:
> reg = <0>;
> phy-handle = <&phy0>;
> phy-mode = "internal";
> + ethernet = <&mac_sw>; # fails validation as expected
> };
> port1: port@1 {
> reg = <1>;
> @@ -162,5 +184,51 @@ examples:
> };
> };
> };
> + # VSC7512 (DSA)
> + - |
> + ethernet-switch@1{
> + compatible = "mscc,vsc7512-switch";
> + reg = <0x71010000 0x10000>,
> + <0x71030000 0x10000>,
> + <0x71080000 0x100>,
> + <0x710e0000 0x10000>,
> + <0x711e0000 0x100>,
> + <0x711f0000 0x100>,
> + <0x71200000 0x100>,
> + <0x71210000 0x100>,
> + <0x71220000 0x100>,
> + <0x71230000 0x100>,
> + <0x71240000 0x100>,
> + <0x71250000 0x100>,
> + <0x71260000 0x100>,
> + <0x71270000 0x100>,
> + <0x71280000 0x100>,
> + <0x71800000 0x80000>,
> + <0x71880000 0x10000>,
> + <0x71040000 0x10000>,
> + <0x71050000 0x10000>,
> + <0x71060000 0x10000>;
> + reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
> + "port2", "port3", "port4", "port5", "port6",
> + "port7", "port8", "port9", "port10", "qsys",
> + "ana", "s0", "s1", "s2";
> +
> + ethernet-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + ethernet = <&mac_sw>;
> + phy-handle = <&phy0>;
> + phy-mode = "internal";
> + };
> + port@1 {
> + reg = <1>;
> + phy-handle = <&phy1>;
> + phy-mode = "internal";
> + };
> + };
> + };
>
> ...
>
> Of course this is a completely uneducated attempt on my part.