From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:30:27
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
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:30:32
Expose ocelot_wm functions so they can be shared with other drivers.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1 - 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(-)
@@ -487,6 +487,37 @@ static void ocelot_watermark_init(struct ocelot *ocelot)ocelot_setup_sharing_watermarks(ocelot);}+/* Watermark encode+*Bit8:Unit;0:1,1:16+*Bit7-0:Valuetobemultipliedwithunit+*/+u16ocelot_wm_enc(u16value)+{+WARN_ON(value>=16*BIT(8));++if(value>=BIT(8))+returnBIT(8)|(value/16);++returnvalue;+}+EXPORT_SYMBOL(ocelot_wm_enc);++u16ocelot_wm_dec(u16wm)+{+if(wm&BIT(8))+return(wm&GENMASK(7,0))*16;++returnwm;+}+EXPORT_SYMBOL(ocelot_wm_dec);++voidocelot_wm_stat(u32val,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*lookupthecellsizemultipliers.*/
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:30:37
The ocelot_regfields struct is common between several different chips, some
of which can only be controlled externally. Export this structure so it
doesn't have to be duplicated in these other drivers.
Rename the structure as well, to follow the conventions of other shared
resources.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
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(-)
@@ -42,64 +42,6 @@ static const u32 *ocelot_regmap[TARGET_MAX] = {[DEV_GMII]=vsc7514_dev_gmii_regmap,};-staticconststructreg_fieldocelot_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),-};-staticconststructocelot_stat_layoutocelot_stats_layout[OCELOT_NUM_STATS]={OCELOT_COMMON_STATS,};
@@ -9,6 +9,65 @@#include<soc/mscc/vsc7514_regs.h>#include"ocelot.h"+conststructreg_fieldvsc7514_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);+constu32vsc7514_ana_regmap[]={REG(ANA_ADVLEARN,0x009000),REG(ANA_VLANMASK,0x009004),
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:30:42
The ocelot_stats_layout array is common between several different chips,
some of which can only be controlled externally. Export this structure so
it doesn't have to be duplicated in these other drivers.
Rename the structure as well, to follow the conventions of other shared
resources.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
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(-)
@@ -42,10 +42,6 @@ static const u32 *ocelot_regmap[TARGET_MAX] = {[DEV_GMII]=vsc7514_dev_gmii_regmap,};-staticconststructocelot_stat_layoutocelot_stats_layout[OCELOT_NUM_STATS]={-OCELOT_COMMON_STATS,-};-staticvoidocelot_pll5_init(structocelot*ocelot){/* Configure PLL5. This will need a proper CCF driver
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:31:12
The vcap_props structure is common to other devices, specifically the
VSC7512 chip that can only be controlled externally. Export this structure
so it doesn't need to be recreated.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
v1 - 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(-)
@@ -181,49 +181,6 @@ static const struct ocelot_ops ocelot_ops = {.netdev_to_port=ocelot_netdev_to_port,};-staticstructvcap_propsvsc7514_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,-},-};-staticstructptp_clock_infoocelot_ptp_clock_info={.owner=THIS_MODULE,.name="ocelot ptp",
@@ -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);++structvcap_propsvsc7514_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);
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:31:15
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 <colin.foster@in-advantage.com>
---
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(-)
@@ -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);}+staticintocelot_mem_init_status(structocelot*ocelot)+{+unsignedintval;+interr;++err=regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],+&val);++returnerr?:val;+}++intocelot_reset(structocelot*ocelot)+{+interr;+u32val;++err=regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],1);+if(err)+returnerr;++err=regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA],1);+if(err)+returnerr;++/* MEM_INIT is a self-clearing bit. Wait for it to be cleared (should be+*100us)beforeenablingtheswitchcore.+*/+err=readx_poll_timeout(ocelot_mem_init_status,ocelot,val,!val,+MEM_INIT_SLEEP_US,MEM_INIT_TIMEOUT_US);+if(err)+returnerr;++err=regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA],1);+if(err)+returnerr;++returnregmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA],1);+}+EXPORT_SYMBOL(ocelot_reset);+intocelot_init(structocelot*ocelot){inti,ret;
@@ -133,45 +130,6 @@ static const struct of_device_id mscc_ocelot_match[] = {};MODULE_DEVICE_TABLE(of,mscc_ocelot_match);-staticintocelot_mem_init_status(structocelot*ocelot)-{-unsignedintval;-interr;--err=regmap_field_read(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],-&val);--returnerr?:val;-}--staticintocelot_reset(structocelot*ocelot)-{-interr;-u32val;--err=regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT],1);-if(err)-returnerr;--err=regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA],1);-if(err)-returnerr;--/* MEM_INIT is a self-clearing bit. Wait for it to be cleared (should be-*100us)beforeenablingtheswitchcore.-*/-err=readx_poll_timeout(ocelot_mem_init_status,ocelot,val,!val,-MEM_INIT_SLEEP_US,MEM_INIT_TIMEOUT_US);-if(err)-returnerr;--err=regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA],1);-if(err)-returnerr;--returnregmap_field_write(ocelot->regfields[SYS_RESET_CFG_CORE_ENA],1);-}-staticconststructocelot_opsocelot_ops={.reset=ocelot_reset,.wm_enc=ocelot_wm_enc,
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:31:18
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 <colin.foster@in-advantage.com>
---
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(+)
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:31:33
The define FELIX_MAC_QUIRKS was used directly in the felix.c shared driver.
Other devices (VSC7512 for example) don't require the same quirks, so they
need to be configured on a per-device basis.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
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(-)
@@ -33,6 +33,7 @@ struct felix_info {u16vcap_pol_base2;u16vcap_pol_max2;conststructptp_clock_info*ptp_caps;+unsignedlongquirks;/* Some Ocelot switches are integrated into the SoC without the*extractionIRQlineconnectedtotheARMGIC.Byenablingthis
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:31:43
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 <colin.foster@in-advantage.com>
---
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(-)
@@ -1318,11 +1318,48 @@ static int felix_parse_dt(struct felix *felix, phy_interface_t *port_phy_modes)returnerr;}+structregmap*felix_init_regmap(structocelot*ocelot,constchar*name)+{+structfelix*felix=ocelot_to_felix(ocelot);+conststructresource*match=NULL;+structresourceres;+inti;++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)+returnERR_PTR(-EINVAL);++memcpy(&res,match,sizeof(res));+res.flags=IORESOURCE_MEM;+res.start+=felix->switch_base;+res.end+=felix->switch_base;++returnocelot_regmap_init(ocelot,&res);+}+staticintfelix_init_structs(structfelix*felix,intnum_phys_ports){structocelot*ocelot=&felix->ocelot;phy_interface_t*port_phy_modes;-structresourceres;+constchar*name;intport,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++){structregmap*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",
@@ -57,8 +57,7 @@ struct felix_info {void(*tas_guard_bands_update)(structocelot*ocelot,intport);void(*port_sched_speed_set)(structocelot*ocelot,intport,u32speed);-structregmap*(*init_regmap)(structocelot*ocelot,-structresource*res);+structregmap*(*init_regmap)(structocelot*ocelot,constchar*name);};/* Methods for initializing the hardware resources specific to a tagging
@@ -97,5 +96,6 @@ struct felix {structnet_device*felix_port_to_netdev(structocelot*ocelot,intport);intfelix_netdev_to_port(structnet_device*dev);+structregmap*felix_init_regmap(structocelot*ocelot,constchar*name);#endif
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:32:00
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 <colin.foster@in-advantage.com>
---
v3
* No change
v2
* New patch
---
drivers/pinctrl/pinctrl-ocelot.c | 1 +
1 file changed, 1 insertion(+)
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:32:26
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 <colin.foster@in-advantage.com>
---
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(-)
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:32:42
The Ocelot switch core driver relies heavily on a fixed array of resources
for both ports and peripherals. This is in contrast to existing peripherals
- pinctrl for example - which have a one-to-one mapping of driver <>
resource. As such, these regmaps must be created differently so that
enumeration-based offsets are preserved.
Register the regmaps to the core MFD device unconditionally so they can be
referenced by the Ocelot switch / Felix DSA systems.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
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(+)
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:32:56
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 <colin.foster@in-advantage.com>
---
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(+)
@@ -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
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:33:12
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 <colin.foster@in-advantage.com>
---
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
@@ -0,0 +1,194 @@+// SPDX-License-Identifier: (GPL-2.0 OR MIT)+/*+*Copyright2021-2022InnovativeAdvantageInc.+*/++#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)++staticconstu32vsc7512_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,+};++staticconstu32*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,+};++staticvoidocelot_ext_phylink_validate(structocelot*ocelot,intport,+unsignedlong*supported,+structphylink_link_state*state)+{+structfelix*felix=ocelot_to_felix(ocelot);+structdsa_switch*ds=felix->ds;+structdsa_port*dp;++dp=dsa_to_port(ds,port);++phylink_generic_validate(&dp->pl_config,supported,state);+}++staticstructregmap*ocelot_ext_regmap_init(structocelot*ocelot,+constchar*name)+{+/* In the ocelot-mfd configuration, regmaps are attached to the device+*bynamealone,sodev_get_regmapwillreturntherequestedregmap+*withouttheneedtofullydefinetheresource+*/+returndev_get_regmap(ocelot->dev->parent,name);+}++staticconststructocelot_opsocelot_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,+};++staticconststructfelix_infovsc7512_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,+};++staticintocelot_ext_probe(structplatform_device*pdev)+{+structdevice*dev=&pdev->dev;+structdsa_switch*ds;+structocelot*ocelot;+structfelix*felix;+interr;++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");+gotoerr_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");+gotoerr_free_ds;+}++return0;++err_free_ds:+kfree(ds);+err_free_felix:+kfree(felix);+returnerr;+}++staticintocelot_ext_remove(structplatform_device*pdev)+{+structfelix*felix=dev_get_drvdata(&pdev->dev);++if(!felix)+return0;++dsa_unregister_switch(felix->ds);++kfree(felix->ds);+kfree(felix);++dev_set_drvdata(&pdev->dev,NULL);++return0;+}++staticvoidocelot_ext_shutdown(structplatform_device*pdev)+{+structfelix*felix=dev_get_drvdata(&pdev->dev);++if(!felix)+return;++dsa_switch_shutdown(felix->ds);++dev_set_drvdata(&pdev->dev,NULL);+}++staticconststructof_device_idocelot_ext_switch_of_match[]={+{.compatible="mscc,vsc7512-switch"},+{},+};+MODULE_DEVICE_TABLE(of,ocelot_ext_switch_of_match);++staticstructplatform_driverocelot_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);
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-26 00:33:29
Utilize the existing ocelot MFD interface to add switch functionality to
the Microsemi VSC7512 chip.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
v3
* No change
v2
* New patch, broken out from a previous one
---
drivers/mfd/ocelot-core.c | 3 +++
1 file changed, 3 insertions(+)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-09-27 17:54:07
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 <colin.foster@in-advantage.com>
---
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.
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-27 18:44:21
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:
quoted
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 <colin.foster@in-advantage.com>
---
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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-09-27 18:57:07
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.
quoted
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.
++ 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?
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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-09-27 21:04:31
On Sun, Sep 25, 2022 at 05:29:25PM -0700, Colin Foster wrote:
quoted hunk
The Ocelot switch core driver relies heavily on a fixed array of resources
for both ports and peripherals. This is in contrast to existing peripherals
- pinctrl for example - which have a one-to-one mapping of driver <>
resource. As such, these regmaps must be created differently so that
enumeration-based offsets are preserved.
Register the regmaps to the core MFD device unconditionally so they can be
referenced by the Ocelot switch / Felix DSA systems.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
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(+)
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.
@@ -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);+
++ 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.
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/20211125201301.3748513-4-colin.foster@in-advantage.com/#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.
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-09-27 23:02:15
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:
quoted
The Ocelot switch core driver relies heavily on a fixed array of resources
for both ports and peripherals. This is in contrast to existing peripherals
- pinctrl for example - which have a one-to-one mapping of driver <>
resource. As such, these regmaps must be created differently so that
enumeration-based offsets are preserved.
Register the regmaps to the core MFD device unconditionally so they can be
referenced by the Ocelot switch / Felix DSA systems.
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
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(+)
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.
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.
quoted
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.
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?
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)
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);
From: Krzysztof Kozlowski <hidden> Date: 2022-10-04 11:19:44
On 26/09/2022 02:29, Colin Foster wrote:
quoted hunk
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 <colin.foster@in-advantage.com>
---
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(+)
@@ -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
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-04 12:15:34
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
quoted
+ # 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.
From: Krzysztof Kozlowski <hidden> Date: 2022-10-04 14:59:11
On 04/10/2022 14:15, Vladimir Oltean wrote:
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
quoted
quoted
+ # 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.
quoted
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
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-04 16:01:49
On Tue, Oct 04, 2022 at 04:59:02PM +0200, Krzysztof Kozlowski wrote:
On 04/10/2022 14:15, Vladimir Oltean wrote:
quoted
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
quoted
quoted
+ # 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.
quoted
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.
quoted
quoted
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.
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-10-05 00:08:54
Hi Krzysztof,
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
On 26/09/2022 02:29, Colin Foster wrote:
quoted
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 <colin.foster@in-advantage.com>
...
quoted
+ # 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.
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>,
...
From: Krzysztof Kozlowski <hidden> Date: 2022-10-05 08:03:25
On 05/10/2022 02:08, Colin Foster wrote:
Hi Krzysztof,
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
quoted
On 26/09/2022 02:29, Colin Foster wrote:
quoted
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 <colin.foster@in-advantage.com>
...
quoted
quoted
+ # 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.
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
From: Krzysztof Kozlowski <hidden> Date: 2022-10-05 08:09:17
On 04/10/2022 18:01, Vladimir Oltean wrote:
On Tue, Oct 04, 2022 at 04:59:02PM +0200, Krzysztof Kozlowski wrote:
quoted
On 04/10/2022 14:15, Vladimir Oltean wrote:
quoted
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
quoted
quoted
+ # 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.
quoted
quoted
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.
quoted
quoted
quoted
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
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-10-05 15:44:26
On Wed, Oct 05, 2022 at 10:03:04AM +0200, Krzysztof Kozlowski wrote:
On 05/10/2022 02:08, Colin Foster wrote:
quoted
Hi Krzysztof,
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
quoted
On 26/09/2022 02:29, Colin Foster wrote:
quoted
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 <colin.foster@in-advantage.com>
...
quoted
quoted
+ # 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.
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.
From: Krzysztof Kozlowski <hidden> Date: 2022-10-05 16:10:22
On 05/10/2022 17:44, Colin Foster wrote:
On Wed, Oct 05, 2022 at 10:03:04AM +0200, Krzysztof Kozlowski wrote:
quoted
On 05/10/2022 02:08, Colin Foster wrote:
quoted
Hi Krzysztof,
On Tue, Oct 04, 2022 at 01:19:33PM +0200, Krzysztof Kozlowski wrote:
quoted
On 26/09/2022 02:29, Colin Foster wrote:
quoted
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 <colin.foster@in-advantage.com>
...
quoted
quoted
+ # 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.
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
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-10-07 20:44:24
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:
quoted
++ 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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-07 22:38:36
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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-07 22:48:51
On Tue, Sep 27, 2022 at 03:20:47PM -0700, Colin Foster wrote:
quoted
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/20211125201301.3748513-4-colin.foster@in-advantage.com/#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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-07 23:10:23
On Wed, Oct 05, 2022 at 10:09:06AM +0200, Krzysztof Kozlowski wrote:
quoted
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.
quoted
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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-08 00:00:39
On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote:
quoted
quoted
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.
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-10-08 17:56:52
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:
quoted
quoted
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/20211125201301.3748513-4-colin.foster@in-advantage.com/#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.
From: Krzysztof Kozlowski <hidden> Date: 2022-10-09 15:49:39
On 08/10/2022 01:10, Vladimir Oltean wrote:
On Wed, Oct 05, 2022 at 10:09:06AM +0200, Krzysztof Kozlowski wrote:
quoted
quoted
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.
quoted
quoted
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.
From: Krzysztof Kozlowski <hidden> Date: 2022-10-09 16:15:20
On 08/10/2022 02:00, Vladimir Oltean wrote:
On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote:
quoted
quoted
quoted
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.
quoted
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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-10 13:07:38
On Sun, Oct 09, 2022 at 12:14:22PM -0400, Krzysztof Kozlowski wrote:
On 08/10/2022 02:00, Vladimir Oltean wrote:
quoted
On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote:
quoted
quoted
quoted
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.
quoted
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.
quoted
quoted
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.
quoted
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?
quoted
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).
From: Krzysztof Kozlowski <hidden> Date: 2022-10-10 13:37:34
On 10/10/2022 09:07, Vladimir Oltean wrote:
On Sun, Oct 09, 2022 at 12:14:22PM -0400, Krzysztof Kozlowski wrote:
quoted
On 08/10/2022 02:00, Vladimir Oltean wrote:
quoted
On Wed, Oct 05, 2022 at 06:09:59PM +0200, Krzysztof Kozlowski wrote:
quoted
quoted
quoted
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.
quoted
quoted
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.
quoted
quoted
quoted
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).
quoted
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).
quoted
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.
quoted
quoted
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.
quoted
quoted
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
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-10 17:49:11
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?
From: Colin Foster <colin.foster@in-advantage.com> Date: 2022-10-10 18:47:27
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:
quoted
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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-10 19:11:32
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?
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-10-11 09:53:27
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.
From: Colin Foster <colin.foster@in-advantage.com> Date: 2023-01-18 22:29:15
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:
quoted
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.
@@ -18,13 +18,50 @@ description: |packets using CPU. Additionally, PTP is supported as well as FDMA for fasterpacket 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-switchreg:+minItems:20items:-description:system target-description:rewriter target