| Message ID | 20201217075134.919699-1-steen.hegelund@microchip.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Adding the Sparx5 Switch Driver | expand |
On Thu, Dec 17, 2020 at 08:51:28AM +0100, Steen Hegelund wrote: > +static struct sparx5_io_resource sparx5_iomap[] = { This could be made const i think,. > + { TARGET_DEV2G5, 0, 0 }, /* 0x610004000: dev2g5_0 */ > + { TARGET_DEV5G, 0x4000, 0 }, /* 0x610008000: dev5g_0 */ > + { TARGET_PCS5G_BR, 0x8000, 0 }, /* 0x61000c000: pcs5g_br_0 */ > + { TARGET_DEV2G5 + 1, 0xc000, 0 }, /* 0x610010000: dev2g5_1 */ > +static int sparx5_create_targets(struct sparx5 *sparx5) > +{ > + int idx, jdx; > + struct resource *iores[IO_RANGES]; > + void __iomem *iomem[IO_RANGES]; > + void __iomem *begin[IO_RANGES]; > + int range_id[IO_RANGES]; Reverse Christmas tree. idx, jdx need to come last. > + > + /* Check if done previously (deferred by serdes load) */ > + if (sparx5->regs[sparx5_iomap[0].id]) > + return 0; Could you explain this a bit more. Do you mean -EPROBE_DEFER? > +static int sparx5_probe_port(struct sparx5 *sparx5, > + struct device_node *portnp, > + struct phy *serdes, > + u32 portno, > + struct sparx5_port_config *conf) > +{ > + struct sparx5_port *spx5_port; > + struct net_device *ndev; > + int err; > + > + err = sparx5_create_targets(sparx5); > + if (err) > + return err; This sees odd here. Don't sparx5_create_targets() create all the targets, where as this creates one specific port? Seems like sparx5_create_targets() should be in the devices as a whole probe, not the port probe. > + spx5_port = netdev_priv(ndev); > + spx5_port->of_node = portnp; > + spx5_port->serdes = serdes; > + spx5_port->pvid = NULL_VID; > + spx5_port->signd_internal = true; > + spx5_port->signd_active_high = true; > + spx5_port->signd_enable = true; > + spx5_port->flow_control = false; > + spx5_port->max_vlan_tags = SPX5_PORT_MAX_TAGS_NONE; > + spx5_port->vlan_type = SPX5_VLAN_PORT_TYPE_UNAWARE; > + spx5_port->custom_etype = 0x8880; /* Vitesse */ > + conf->portmode = conf->phy_mode; > + spx5_port->conf.speed = SPEED_UNKNOWN; > + spx5_port->conf.power_down = true; > + sparx5->ports[portno] = spx5_port; > + return 0; I'm also not sure this has the correct name. This does not look like a typical probe function. > +} > + > +static int sparx5_init_switchcore(struct sparx5 *sparx5) > +{ > + u32 value, pending, jdx, idx; > + struct { > + bool gazwrap; > + void __iomem *init_reg; > + u32 init_val; > + } ram, ram_init_list[] = { > + {false, spx5_reg_get(sparx5, ANA_AC_STAT_RESET), > + ANA_AC_STAT_RESET_RESET}, > + {false, spx5_reg_get(sparx5, ASM_STAT_CFG), > + ASM_STAT_CFG_STAT_CNT_CLR_SHOT}, > + {true, spx5_reg_get(sparx5, QSYS_RAM_INIT), 0}, > + {true, spx5_reg_get(sparx5, REW_RAM_INIT), 0}, > + {true, spx5_reg_get(sparx5, VOP_RAM_INIT), 0}, > + {true, spx5_reg_get(sparx5, ANA_AC_RAM_INIT), 0}, > + {true, spx5_reg_get(sparx5, ASM_RAM_INIT), 0}, > + {true, spx5_reg_get(sparx5, EACL_RAM_INIT), 0}, > + {true, spx5_reg_get(sparx5, VCAP_SUPER_RAM_INIT), 0}, > + {true, spx5_reg_get(sparx5, DSM_RAM_INIT), 0} > + }; Looks like this could be const as well. And this does not really fit reverse christmas tree. > + > + spx5_rmw(EACL_POL_EACL_CFG_EACL_FORCE_INIT_SET(1), > + EACL_POL_EACL_CFG_EACL_FORCE_INIT, > + sparx5, > + EACL_POL_EACL_CFG); > + > + spx5_rmw(EACL_POL_EACL_CFG_EACL_FORCE_INIT_SET(0), > + EACL_POL_EACL_CFG_EACL_FORCE_INIT, > + sparx5, > + EACL_POL_EACL_CFG); > + > + /* Initialize memories, if not done already */ > + value = spx5_rd(sparx5, HSCH_RESET_CFG); > + > + if (!(value & HSCH_RESET_CFG_CORE_ENA)) { > + for (idx = 0; idx < 10; idx++) { > + pending = ARRAY_SIZE(ram_init_list); > + for (jdx = 0; jdx < ARRAY_SIZE(ram_init_list); jdx++) { > + ram = ram_init_list[jdx]; > + if (ram.gazwrap) > + ram.init_val = QSYS_RAM_INIT_RAM_INIT; > + > + if (idx == 0) { > + writel(ram.init_val, ram.init_reg); > + } else { > + value = readl(ram.init_reg); > + if ((value & ram.init_val) != > + ram.init_val) { > + pending--; > + } > + } > + } > + if (!pending) > + break; > + usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); > + } You are getting pretty deeply nested here. Might be better to pull this out into a helpers. > + > + if (pending > 0) { > + /* Still initializing, should be complete in > + * less than 1ms > + */ > + dev_err(sparx5->dev, "Memory initialization error\n"); > + return -EINVAL; > + } > + } > + > + /* Reset counters */ > + spx5_wr(ANA_AC_STAT_RESET_RESET_SET(1), sparx5, ANA_AC_STAT_RESET); > + spx5_wr(ASM_STAT_CFG_STAT_CNT_CLR_SHOT_SET(1), sparx5, ASM_STAT_CFG); > + > + /* Enable switch-core and queue system */ > + spx5_wr(HSCH_RESET_CFG_CORE_ENA_SET(1), sparx5, HSCH_RESET_CFG); > + > + return 0; > +} > + > +static int sparx5_init_coreclock(struct sparx5 *sparx5) > +{ > + u32 clk_div, clk_period, pol_upd_int, idx; > + enum sparx5_core_clockfreq freq = sparx5->coreclock; More reverse christmas tree. Please review the whole driver. > + > + /* Verify if core clock frequency is supported on target. > + * If 'VTSS_CORE_CLOCK_DEFAULT' then the highest supported > + * freq. is used > + */ > + switch (sparx5->target_ct) { > + case SPX5_TARGET_CT_7546: > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > + freq = SPX5_CORE_CLOCK_250MHZ; > + else if (sparx5->coreclock != SPX5_CORE_CLOCK_250MHZ) > + freq = 0; /* Not supported */ > + break; > + case SPX5_TARGET_CT_7549: > + case SPX5_TARGET_CT_7552: > + case SPX5_TARGET_CT_7556: > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > + freq = SPX5_CORE_CLOCK_500MHZ; > + else if (sparx5->coreclock != SPX5_CORE_CLOCK_500MHZ) > + freq = 0; /* Not supported */ > + break; > + case SPX5_TARGET_CT_7558: > + case SPX5_TARGET_CT_7558TSN: > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > + freq = SPX5_CORE_CLOCK_625MHZ; > + else if (sparx5->coreclock != SPX5_CORE_CLOCK_625MHZ) > + freq = 0; /* Not supported */ > + break; > + case SPX5_TARGET_CT_7546TSN: > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > + freq = SPX5_CORE_CLOCK_625MHZ; > + break; > + case SPX5_TARGET_CT_7549TSN: > + case SPX5_TARGET_CT_7552TSN: > + case SPX5_TARGET_CT_7556TSN: > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > + freq = SPX5_CORE_CLOCK_625MHZ; > + else if (sparx5->coreclock == SPX5_CORE_CLOCK_250MHZ) > + freq = 0; /* Not supported */ > + break; > + default: > + dev_err(sparx5->dev, "Target (%#04x) not supported\n", sparx5->target_ct); netdev is staying with 80 character lines. Please fold this, here and every where else, where possible. The exception is, you should not split a string. > + return -ENODEV; > + } > + > + switch (freq) { > + case SPX5_CORE_CLOCK_250MHZ: > + clk_div = 10; > + pol_upd_int = 312; > + break; > + case SPX5_CORE_CLOCK_500MHZ: > + clk_div = 5; > + pol_upd_int = 624; > + break; > + case SPX5_CORE_CLOCK_625MHZ: > + clk_div = 4; > + pol_upd_int = 780; > + break; > + default: > + dev_err(sparx5->dev, "%s: Frequency (%d) not supported on target (%#04x)\n", > + __func__, > + sparx5->coreclock, sparx5->target_ct); > + return 0; -EINVAL? Or is it not fatal to use an unsupported frequency? > +static int sparx5_init(struct sparx5 *sparx5) > +{ > + u32 idx; > + > + if (sparx5_create_targets(sparx5)) > + return -ENODEV; Hum, sparx5_create_targets() again? > + > + /* Read chip ID to check CPU interface */ > + sparx5->chip_id = spx5_rd(sparx5, GCB_CHIP_ID); > + > + sparx5->target_ct = (enum spx5_target_chiptype) > + GCB_CHIP_ID_PART_ID_GET(sparx5->chip_id); > + > + /* Initialize Switchcore and internal RAMs */ > + if (sparx5_init_switchcore(sparx5)) { > + dev_err(sparx5->dev, "Switchcore initialization error\n"); > + return -EINVAL; > + } > + > + /* Initialize the LC-PLL (core clock) and set affected registers */ > + if (sparx5_init_coreclock(sparx5)) { > + dev_err(sparx5->dev, "LC-PLL initialization error\n"); > + return -EINVAL; > + } > + > + /* Setup own UPSIDs */ > + for (idx = 0; idx < 3; idx++) { > + spx5_wr(idx, sparx5, ANA_AC_OWN_UPSID(idx)); > + spx5_wr(idx, sparx5, ANA_CL_OWN_UPSID(idx)); > + spx5_wr(idx, sparx5, ANA_L2_OWN_UPSID(idx)); > + spx5_wr(idx, sparx5, REW_OWN_UPSID(idx)); > + } > + > + /* Enable switch ports */ > + for (idx = SPX5_PORTS; idx < SPX5_PORTS_ALL; idx++) { > + spx5_rmw(QFWD_SWITCH_PORT_MODE_PORT_ENA_SET(1), > + QFWD_SWITCH_PORT_MODE_PORT_ENA, > + sparx5, > + QFWD_SWITCH_PORT_MODE(idx)); > + } What happens when you enable the ports? Why is this here, and not in the port specific open call? > +/* Some boards needs to map the SGPIO for signal detect explicitly to the > + * port module > + */ > +static void sparx5_board_init(struct sparx5 *sparx5) > +{ > + int idx; > + > + if (!sparx5->sd_sgpio_remapping) > + return; > + > + /* Enable SGPIO Signal Detect remapping */ > + spx5_rmw(GCB_HW_SGPIO_SD_CFG_SD_MAP_SEL, > + GCB_HW_SGPIO_SD_CFG_SD_MAP_SEL, > + sparx5, > + GCB_HW_SGPIO_SD_CFG); > + > + /* Refer to LOS SGPIO */ > + for (idx = 0; idx < SPX5_PORTS; idx++) { > + if (sparx5->ports[idx]) { > + if (sparx5->ports[idx]->conf.sd_sgpio != ~0) { > + spx5_wr(sparx5->ports[idx]->conf.sd_sgpio, > + sparx5, > + GCB_HW_SGPIO_TO_SD_MAP_CFG(idx)); > + } > + } > + } > +} I've not looked at how you do SFP integration yet. Is this the LOS from the SFP socket? Is there a Linux GPIO controller exported by this driver, so the SFP driver can use the GPIOs? > + > +static int mchp_sparx5_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct sparx5 *sparx5; > + struct device_node *ports, *portnp; > + const u8 *mac_addr; > + int err = 0; > + > + if (!np && !pdev->dev.platform_data) > + return -ENODEV; > + > + sparx5 = devm_kzalloc(&pdev->dev, sizeof(*sparx5), GFP_KERNEL); > + if (!sparx5) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, sparx5); > + sparx5->pdev = pdev; > + sparx5->dev = &pdev->dev; > + > + /* Default values, some from DT */ > + sparx5->coreclock = SPX5_CORE_CLOCK_DEFAULT; > + > + mac_addr = of_get_mac_address(np); > + if (IS_ERR_OR_NULL(mac_addr)) { > + dev_info(sparx5->dev, "MAC addr was not set, use random MAC\n"); > + eth_random_addr(sparx5->base_mac); > + sparx5->base_mac[5] = 0; > + } else { > + ether_addr_copy(sparx5->base_mac, mac_addr); > + } The binding document does not say anything about a MAC address at the top level. What is this used for? + > + if (sparx5_init(sparx5)) { > + dev_err(sparx5->dev, "Init failed\n"); > + return -ENODEV; > + } > + ports = of_get_child_by_name(np, "ethernet-ports"); > + if (!ports) { > + dev_err(sparx5->dev, "no ethernet-ports child node found\n"); > + return -ENODEV; > + } > + sparx5->port_count = of_get_child_count(ports); > + > + for_each_available_child_of_node(ports, portnp) { > + struct sparx5_port_config config = {}; > + u32 portno; > + struct phy *serdes; > + > + err = of_property_read_u32(portnp, "reg", &portno); > + if (err) { > + dev_err(sparx5->dev, "port reg property error\n"); > + continue; > + } > + err = of_property_read_u32(portnp, "max-speed", > + &config.max_speed); > + if (err) { > + dev_err(sparx5->dev, "port max-speed property error\n"); > + continue; > + } > + config.speed = SPEED_UNKNOWN; > + err = of_property_read_u32(portnp, "sd_sgpio", &config.sd_sgpio); Not in the binding documentation. I think i need to withdraw my Reviewed-by :-( > + if (err) > + config.sd_sgpio = ~0; > + else > + sparx5->sd_sgpio_remapping = true; > + serdes = devm_of_phy_get(sparx5->dev, portnp, NULL); > + if (IS_ERR(serdes)) { > + err = PTR_ERR(serdes); > + if (err != -EPROBE_DEFER) > + dev_err(sparx5->dev, > + "missing SerDes phys for port%d\n", > + portno); > + return err; > + } > + > + err = of_get_phy_mode(portnp, &config.phy_mode); > + if (err) > + config.power_down = true; You should indicate in the binding it is optional. And what happens when it is missing. > + config.media_type = ETH_MEDIA_DAC; > + config.serdes_reset = true; > + config.portmode = config.phy_mode; > + err = sparx5_probe_port(sparx5, portnp, serdes, portno, &config); > + if (err) { > + dev_err(sparx5->dev, "port probe error\n"); > + goto cleanup_ports; > + } > + } > + sparx5_board_init(sparx5); > + > +cleanup_ports: > + return err; Seems missed named, no cleanup. > +static int __init sparx5_switch_reset(void) > +{ > + const char *syscon_cpu = "microchip,sparx5-cpu-syscon", > + *syscon_gcb = "microchip,sparx5-gcb-syscon"; > + struct regmap *cpu_ctrl, *gcb_ctrl; > + u32 val; > + > + cpu_ctrl = syscon_regmap_lookup_by_compatible(syscon_cpu); > + if (IS_ERR(cpu_ctrl)) { > + pr_err("No '%s' syscon map\n", syscon_cpu); > + return PTR_ERR(cpu_ctrl); > + } > + > + gcb_ctrl = syscon_regmap_lookup_by_compatible(syscon_gcb); > + if (IS_ERR(gcb_ctrl)) { > + pr_err("No '%s' syscon map\n", syscon_gcb); > + return PTR_ERR(gcb_ctrl); > + } > + > + /* Make sure the core is PROTECTED from reset */ > + regmap_update_bits(cpu_ctrl, RESET_PROT_STAT, > + SYS_RST_PROT_VCORE, SYS_RST_PROT_VCORE); > + > + regmap_write(gcb_ctrl, spx5_offset(GCB_SOFT_RST), > + GCB_SOFT_RST_SOFT_SWC_RST_SET(1)); > + > + return readx_poll_timeout(sparx5_read_gcb_soft_rst, gcb_ctrl, val, > + GCB_SOFT_RST_SOFT_SWC_RST_GET(val) == 0, > + 1, 100); > +} > +postcore_initcall(sparx5_switch_reset); That is pretty unusual. Why cannot this be done at probe time? > +/* Clock period in picoseconds */ > +static inline u32 sparx5_clk_period(enum sparx5_core_clockfreq cclock) > +{ > + switch (cclock) { > + case SPX5_CORE_CLOCK_250MHZ: > + return 4000; > + case SPX5_CORE_CLOCK_500MHZ: > + return 2000; > + case SPX5_CORE_CLOCK_625MHZ: > + default: > + return 1600; > + } > +} Is this something which is used in the hot path? > --- /dev/null > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h > @@ -0,0 +1,3922 @@ > +/* SPDX-License-Identifier: GPL-2.0+ > + * Microchip Sparx5 Switch driver > + * > + * Copyright (c) 2020 Microchip Technology Inc. > + */ > + > +/* This file is autogenerated by cml-utils 2020-11-19 10:41:34 +0100. > + * Commit ID: f34790e69dc252103e2cc3e85b1a5e4d9e3aa190 > + */ How reproducible this is generation process? If you have to run it again, will it keep the same order of lines? Andrew
On 12/16/2020 11:51 PM, Steen Hegelund wrote: > This series provides the Microchip Sparx5 Switch Driver > > The Sparx5 Carrier Ethernet and Industrial switch family delivers 64 > Ethernet ports and up to 200 Gbps of switching bandwidth. > > It provides a rich set of Ethernet switching features such as hierarchical > QoS, hardware-based OAM and service activation testing, protection > switching, IEEE 1588, and Synchronous Ethernet. > > Using provider bridging (Q-in-Q) and MPLS/MPLS-TP technology, it delivers > MEF CE > 2.0 Ethernet virtual connections (EVCs) and features advanced TCAM > classification in both ingress and egress. > > Per-EVC features include advanced L3-aware classification, a rich set of > statistics, OAM for end-to-end performance monitoring, and dual-rate > policing and shaping. > > Time sensitive networking (TSN) is supported through a comprehensive set of > features including frame preemption, cut-through, frame replication and > elimination for reliability, enhanced scheduling: credit-based shaping, > time-aware shaping, cyclic queuing, and forwarding, and per-stream policing > and filtering. > > Together with IEEE 1588 and IEEE 802.1AS support, this guarantees > low-latency deterministic networking for Fronthaul, Carrier, and Industrial > Ethernet. > > The Sparx5 switch family consists of following SKUs: > > - VSC7546 Sparx5-64 up to 64 Gbps of bandwidth with the following primary > port configurations: > - 6 *10G > - 16 * 2.5G + 2 * 10G > - 24 * 1G + 4 * 10G > > - VSC7549 Sparx5-90 up to 90 Gbps of bandwidth with the following primary > port configurations: > - 9 * 10G > - 16 * 2.5G + 4 * 10G > - 48 * 1G + 4 * 10G > > - VSC7552 Sparx5-128 up to 128 Gbps of bandwidth with the following primary > port configurations: > - 12 * 10G > - 16 * 2.5G + 8 * 10G > - 48 * 1G + 8 * 10G > > - VSC7556 Sparx5-160 up to 160 Gbps of bandwidth with the following primary > port configurations: > - 16 * 10G > - 10 * 10G + 2 * 25G > - 16 * 2.5G + 10 * 10G > - 48 * 1G + 10 * 10G > > - VSC7558 Sparx5-200 up to 200 Gbps of bandwidth with the following primary > port configurations: > - 20 * 10G > - 8 * 25G > > In addition, the device supports one 10/100/1000/2500/5000 Mbps > SGMII/SerDes node processor interface (NPI) Ethernet port. > > The Sparx5 support is developed on the PCB134 and PCB135 evaluation boards. > > - PCB134 main networking features: > - 12x SFP+ front 10G module slots (connected to Sparx5 through SFI). > - 8x SFP28 front 25G module slots (connected to Sparx5 through SFI high > speed). > - Optional, one additional 10/100/1000BASE-T (RJ45) Ethernet port > (on-board VSC8211 PHY connected to Sparx5 through SGMII). > > - PCB135 main networking features: > - 48x1G (10/100/1000M) RJ45 front ports using 12xVSC8514 QuadPHY’s each > connected to VSC7558 through QSGMII. > - 4x10G (1G/2.5G/5G/10G) RJ45 front ports using the AQR407 10G QuadPHY > each port connects to VSC7558 through SFI. > - 4x SFP28 25G module slots on back connected to VSC7558 through SFI high > speed. > - Optional, one additional 1G (10/100/1000M) RJ45 port using an on-board > VSC8211 PHY, which can be connected to VSC7558 NPI port through SGMII > using a loopback add-on PCB) > > This series provides support for: > - SFPs and DAC cables via PHYLINK with a number of 5G, 10G and 25G > devices and media types. > - Port module configuration for 10M to 25G speeds with SGMII, QSGMII, > 1000BASEX, 2500BASEX and 10GBASER as appropriate for these modes. > - SerDes configuration via the Sparx5 SerDes driver (see below). > - Host mode providing register based injection and extraction. > - Switch mode providing MAC/VLAN table learning and Layer2 switching > offloaded to the Sparx5 switch. > - STP state, VLAN support, host/bridge port mode, Forwarding DB, and > configuration and statistics via ethtool. > > More support will be added at a later stage. > > The Sparx5 Switch chip register model can be browsed here: > Link: https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html Out of curiosity, what tool was used to generate the register information page? It looks really neat and well organized.
Hi Florian, On Sun, 2020-12-20 at 16:58 -0800, Florian Fainelli wrote: > > > > The Sparx5 Switch chip register model can be browsed here: > > Link: > > https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html > > Out of curiosity, what tool was used to generate the register > information page? It looks really neat and well organized. It is an in-house tool that is used in our so-called VML-flow (Versatile Markup Language), so it is not out in the open yet. The same model file is used internally in many ways - but exposing it to the public is something we have not tried before, and having this view is so much nicer that the usual datasheet, I find... And thanks for the kind words - I passed them on to the author. BR Steen > -- > Florian
Florian Fainelli writes: > On 12/16/2020 11:51 PM, Steen Hegelund wrote: >> This series provides the Microchip Sparx5 Switch Driver >> >> The Sparx5 Carrier Ethernet and Industrial switch family delivers 64 >> Ethernet ports and up to 200 Gbps of switching bandwidth. >> >> It provides a rich set of Ethernet switching features such as hierarchical >> QoS, hardware-based OAM and service activation testing, protection >> switching, IEEE 1588, and Synchronous Ethernet. >> >> Using provider bridging (Q-in-Q) and MPLS/MPLS-TP technology, it delivers >> MEF CE >> 2.0 Ethernet virtual connections (EVCs) and features advanced TCAM >> classification in both ingress and egress. >> >> Per-EVC features include advanced L3-aware classification, a rich set of >> statistics, OAM for end-to-end performance monitoring, and dual-rate >> policing and shaping. >> >> Time sensitive networking (TSN) is supported through a comprehensive set of >> features including frame preemption, cut-through, frame replication and >> elimination for reliability, enhanced scheduling: credit-based shaping, >> time-aware shaping, cyclic queuing, and forwarding, and per-stream policing >> and filtering. >> >> Together with IEEE 1588 and IEEE 802.1AS support, this guarantees >> low-latency deterministic networking for Fronthaul, Carrier, and Industrial >> Ethernet. >> >> The Sparx5 switch family consists of following SKUs: >> >> - VSC7546 Sparx5-64 up to 64 Gbps of bandwidth with the following primary >> port configurations: >> - 6 *10G >> - 16 * 2.5G + 2 * 10G >> - 24 * 1G + 4 * 10G >> >> - VSC7549 Sparx5-90 up to 90 Gbps of bandwidth with the following primary >> port configurations: >> - 9 * 10G >> - 16 * 2.5G + 4 * 10G >> - 48 * 1G + 4 * 10G >> >> - VSC7552 Sparx5-128 up to 128 Gbps of bandwidth with the following primary >> port configurations: >> - 12 * 10G >> - 16 * 2.5G + 8 * 10G >> - 48 * 1G + 8 * 10G >> >> - VSC7556 Sparx5-160 up to 160 Gbps of bandwidth with the following primary >> port configurations: >> - 16 * 10G >> - 10 * 10G + 2 * 25G >> - 16 * 2.5G + 10 * 10G >> - 48 * 1G + 10 * 10G >> >> - VSC7558 Sparx5-200 up to 200 Gbps of bandwidth with the following primary >> port configurations: >> - 20 * 10G >> - 8 * 25G >> >> In addition, the device supports one 10/100/1000/2500/5000 Mbps >> SGMII/SerDes node processor interface (NPI) Ethernet port. >> >> The Sparx5 support is developed on the PCB134 and PCB135 evaluation boards. >> >> - PCB134 main networking features: >> - 12x SFP+ front 10G module slots (connected to Sparx5 through SFI). >> - 8x SFP28 front 25G module slots (connected to Sparx5 through SFI high >> speed). >> - Optional, one additional 10/100/1000BASE-T (RJ45) Ethernet port >> (on-board VSC8211 PHY connected to Sparx5 through SGMII). >> >> - PCB135 main networking features: >> - 48x1G (10/100/1000M) RJ45 front ports using 12xVSC8514 QuadPHY’s each >> connected to VSC7558 through QSGMII. >> - 4x10G (1G/2.5G/5G/10G) RJ45 front ports using the AQR407 10G QuadPHY >> each port connects to VSC7558 through SFI. >> - 4x SFP28 25G module slots on back connected to VSC7558 through SFI high >> speed. >> - Optional, one additional 1G (10/100/1000M) RJ45 port using an on-board >> VSC8211 PHY, which can be connected to VSC7558 NPI port through SGMII >> using a loopback add-on PCB) >> >> This series provides support for: >> - SFPs and DAC cables via PHYLINK with a number of 5G, 10G and 25G >> devices and media types. >> - Port module configuration for 10M to 25G speeds with SGMII, QSGMII, >> 1000BASEX, 2500BASEX and 10GBASER as appropriate for these modes. >> - SerDes configuration via the Sparx5 SerDes driver (see below). >> - Host mode providing register based injection and extraction. >> - Switch mode providing MAC/VLAN table learning and Layer2 switching >> offloaded to the Sparx5 switch. >> - STP state, VLAN support, host/bridge port mode, Forwarding DB, and >> configuration and statistics via ethtool. >> >> More support will be added at a later stage. >> >> The Sparx5 Switch chip register model can be browsed here: >> Link: https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html > > Out of curiosity, what tool was used to generate the register > information page? It looks really neat and well organized. Florian, It is an in-house tool. The input data is in a proprietary XML-like format. We're pleased that you like it, we do too. We are also pleased that being a Microchip entity, we can actually make this kind of information public. I'll pass your praise on. ---Lars -- Lars Povlsen, Microchip
Hi Andrew, On Sat, 2020-12-19 at 20:11 +0100, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, Dec 17, 2020 at 08:51:28AM +0100, Steen Hegelund wrote: > > > +static struct sparx5_io_resource sparx5_iomap[] = { > > This could be made const i think,. Yes > > > + { TARGET_DEV2G5, 0, 0 }, /* 0x610004000: > > dev2g5_0 */ > > + { TARGET_DEV5G, 0x4000, 0 }, /* 0x610008000: > > dev5g_0 */ > > + { TARGET_PCS5G_BR, 0x8000, 0 }, /* 0x61000c000: > > pcs5g_br_0 */ > > + { TARGET_DEV2G5 + 1, 0xc000, 0 }, /* 0x610010000: > > dev2g5_1 */ > > > +static int sparx5_create_targets(struct sparx5 *sparx5) > > +{ > > + int idx, jdx; > > + struct resource *iores[IO_RANGES]; > > + void __iomem *iomem[IO_RANGES]; > > + void __iomem *begin[IO_RANGES]; > > + int range_id[IO_RANGES]; > > Reverse Christmas tree. idx, jdx need to come last. Yes - I will check the entire file for RCT... > > > + > > + /* Check if done previously (deferred by serdes load) */ > > + if (sparx5->regs[sparx5_iomap[0].id]) > > + return 0; > > Could you explain this a bit more. Do you mean -EPROBE_DEFER? Yes that was the intended usage. I will change the startup flow to try to avoid checking this- > > > +static int sparx5_probe_port(struct sparx5 *sparx5, > > + struct device_node *portnp, > > + struct phy *serdes, > > + u32 portno, > > + struct sparx5_port_config *conf) > > +{ > > + struct sparx5_port *spx5_port; > > + struct net_device *ndev; > > + int err; > > + > > + err = sparx5_create_targets(sparx5); > > + if (err) > > + return err; > > This sees odd here. Don't sparx5_create_targets() create all the > targets, where as this creates one specific port? Seems like > sparx5_create_targets() should be in the devices as a whole probe, > not > the port probe. You are right - the name does not really fit (anymore). I will rework this. > > > + spx5_port = netdev_priv(ndev); > > + spx5_port->of_node = portnp; > > + spx5_port->serdes = serdes; > > + spx5_port->pvid = NULL_VID; > > + spx5_port->signd_internal = true; > > + spx5_port->signd_active_high = true; > > + spx5_port->signd_enable = true; > > + spx5_port->flow_control = false; > > + spx5_port->max_vlan_tags = SPX5_PORT_MAX_TAGS_NONE; > > + spx5_port->vlan_type = SPX5_VLAN_PORT_TYPE_UNAWARE; > > + spx5_port->custom_etype = 0x8880; /* Vitesse */ > > + conf->portmode = conf->phy_mode; > > + spx5_port->conf.speed = SPEED_UNKNOWN; > > + spx5_port->conf.power_down = true; > > + sparx5->ports[portno] = spx5_port; > > + return 0; > > I'm also not sure this has the correct name. This does not look like > a > typical probe function. Agree. > > > > +} > > + > > +static int sparx5_init_switchcore(struct sparx5 *sparx5) > > +{ > > + u32 value, pending, jdx, idx; > > + struct { > > + bool gazwrap; > > + void __iomem *init_reg; > > + u32 init_val; > > + } ram, ram_init_list[] = { > > + {false, spx5_reg_get(sparx5, ANA_AC_STAT_RESET), > > + ANA_AC_STAT_RESET_RESET}, > > + {false, spx5_reg_get(sparx5, ASM_STAT_CFG), > > + ASM_STAT_CFG_STAT_CNT_CLR_SHOT}, > > + {true, spx5_reg_get(sparx5, QSYS_RAM_INIT), 0}, > > + {true, spx5_reg_get(sparx5, REW_RAM_INIT), 0}, > > + {true, spx5_reg_get(sparx5, VOP_RAM_INIT), 0}, > > + {true, spx5_reg_get(sparx5, ANA_AC_RAM_INIT), 0}, > > + {true, spx5_reg_get(sparx5, ASM_RAM_INIT), 0}, > > + {true, spx5_reg_get(sparx5, EACL_RAM_INIT), 0}, > > + {true, spx5_reg_get(sparx5, VCAP_SUPER_RAM_INIT), > > 0}, > > + {true, spx5_reg_get(sparx5, DSM_RAM_INIT), 0} > > + }; > > Looks like this could be const as well. And this does not really fit > reverse christmas tree. I will update this. > > > + > > + spx5_rmw(EACL_POL_EACL_CFG_EACL_FORCE_INIT_SET(1), > > + EACL_POL_EACL_CFG_EACL_FORCE_INIT, > > + sparx5, > > + EACL_POL_EACL_CFG); > > + > > + spx5_rmw(EACL_POL_EACL_CFG_EACL_FORCE_INIT_SET(0), > > + EACL_POL_EACL_CFG_EACL_FORCE_INIT, > > + sparx5, > > + EACL_POL_EACL_CFG); > > + > > + /* Initialize memories, if not done already */ > > + value = spx5_rd(sparx5, HSCH_RESET_CFG); > > + > > + if (!(value & HSCH_RESET_CFG_CORE_ENA)) { > > + for (idx = 0; idx < 10; idx++) { > > + pending = ARRAY_SIZE(ram_init_list); > > + for (jdx = 0; jdx < > > ARRAY_SIZE(ram_init_list); jdx++) { > > + ram = ram_init_list[jdx]; > > + if (ram.gazwrap) > > + ram.init_val = > > QSYS_RAM_INIT_RAM_INIT; > > + > > + if (idx == 0) { > > + writel(ram.init_val, > > ram.init_reg); > > + } else { > > + value = readl(ram.init_reg); > > + if ((value & ram.init_val) != > > + ram.init_val) { > > + pending--; > > + } > > + } > > + } > > + if (!pending) > > + break; > > + usleep_range(USEC_PER_MSEC, 2 * > > USEC_PER_MSEC); > > + } > > You are getting pretty deeply nested here. Might be better to pull > this out into a helpers. Yes. > > > + > > + if (pending > 0) { > > + /* Still initializing, should be complete in > > + * less than 1ms > > + */ > > + dev_err(sparx5->dev, "Memory initialization > > error\n"); > > + return -EINVAL; > > + } > > + } > > + > > + /* Reset counters */ > > + spx5_wr(ANA_AC_STAT_RESET_RESET_SET(1), sparx5, > > ANA_AC_STAT_RESET); > > + spx5_wr(ASM_STAT_CFG_STAT_CNT_CLR_SHOT_SET(1), sparx5, > > ASM_STAT_CFG); > > + > > + /* Enable switch-core and queue system */ > > + spx5_wr(HSCH_RESET_CFG_CORE_ENA_SET(1), sparx5, > > HSCH_RESET_CFG); > > + > > + return 0; > > +} > > + > > +static int sparx5_init_coreclock(struct sparx5 *sparx5) > > +{ > > + u32 clk_div, clk_period, pol_upd_int, idx; > > + enum sparx5_core_clockfreq freq = sparx5->coreclock; > > More reverse christmas tree. Please review the whole driver. I will do that. > > > + > > + /* Verify if core clock frequency is supported on target. > > + * If 'VTSS_CORE_CLOCK_DEFAULT' then the highest supported > > + * freq. is used > > + */ > > + switch (sparx5->target_ct) { > > + case SPX5_TARGET_CT_7546: > > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > > + freq = SPX5_CORE_CLOCK_250MHZ; > > + else if (sparx5->coreclock != SPX5_CORE_CLOCK_250MHZ) > > + freq = 0; /* Not supported */ > > + break; > > + case SPX5_TARGET_CT_7549: > > + case SPX5_TARGET_CT_7552: > > + case SPX5_TARGET_CT_7556: > > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > > + freq = SPX5_CORE_CLOCK_500MHZ; > > + else if (sparx5->coreclock != SPX5_CORE_CLOCK_500MHZ) > > + freq = 0; /* Not supported */ > > + break; > > + case SPX5_TARGET_CT_7558: > > + case SPX5_TARGET_CT_7558TSN: > > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > > + freq = SPX5_CORE_CLOCK_625MHZ; > > + else if (sparx5->coreclock != SPX5_CORE_CLOCK_625MHZ) > > + freq = 0; /* Not supported */ > > + break; > > + case SPX5_TARGET_CT_7546TSN: > > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > > + freq = SPX5_CORE_CLOCK_625MHZ; > > + break; > > + case SPX5_TARGET_CT_7549TSN: > > + case SPX5_TARGET_CT_7552TSN: > > + case SPX5_TARGET_CT_7556TSN: > > + if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT) > > + freq = SPX5_CORE_CLOCK_625MHZ; > > + else if (sparx5->coreclock == SPX5_CORE_CLOCK_250MHZ) > > + freq = 0; /* Not supported */ > > + break; > > + default: > > + dev_err(sparx5->dev, "Target (%#04x) not > > supported\n", sparx5->target_ct); > > netdev is staying with 80 character lines. Please fold this, here and > every where else, where possible. The exception is, you should not > split a string. Will do. > > > + return -ENODEV; > > + } > > + > > + switch (freq) { > > + case SPX5_CORE_CLOCK_250MHZ: > > + clk_div = 10; > > + pol_upd_int = 312; > > + break; > > + case SPX5_CORE_CLOCK_500MHZ: > > + clk_div = 5; > > + pol_upd_int = 624; > > + break; > > + case SPX5_CORE_CLOCK_625MHZ: > > + clk_div = 4; > > + pol_upd_int = 780; > > + break; > > + default: > > + dev_err(sparx5->dev, "%s: Frequency (%d) not > > supported on target (%#04x)\n", > > + __func__, > > + sparx5->coreclock, sparx5->target_ct); > > + return 0; > > -EINVAL? Or is it not fatal to use an unsupported frequency? Yes - it should be fatal. > > > +static int sparx5_init(struct sparx5 *sparx5) > > +{ > > + u32 idx; > > + > > + if (sparx5_create_targets(sparx5)) > > + return -ENODEV; > > Hum, sparx5_create_targets() again? Yes that was due to the PROBE_DEFER - but I will go over this again. > > > + > > + /* Read chip ID to check CPU interface */ > > + sparx5->chip_id = spx5_rd(sparx5, GCB_CHIP_ID); > > + > > + sparx5->target_ct = (enum spx5_target_chiptype) > > + GCB_CHIP_ID_PART_ID_GET(sparx5->chip_id); > > + > > + /* Initialize Switchcore and internal RAMs */ > > + if (sparx5_init_switchcore(sparx5)) { > > + dev_err(sparx5->dev, "Switchcore initialization > > error\n"); > > + return -EINVAL; > > + } > > + > > + /* Initialize the LC-PLL (core clock) and set affected > > registers */ > > + if (sparx5_init_coreclock(sparx5)) { > > + dev_err(sparx5->dev, "LC-PLL initialization > > error\n"); > > + return -EINVAL; > > + } > > + > > + /* Setup own UPSIDs */ > > + for (idx = 0; idx < 3; idx++) { > > + spx5_wr(idx, sparx5, ANA_AC_OWN_UPSID(idx)); > > + spx5_wr(idx, sparx5, ANA_CL_OWN_UPSID(idx)); > > + spx5_wr(idx, sparx5, ANA_L2_OWN_UPSID(idx)); > > + spx5_wr(idx, sparx5, REW_OWN_UPSID(idx)); > > + } > > + > > + /* Enable switch ports */ > > + for (idx = SPX5_PORTS; idx < SPX5_PORTS_ALL; idx++) { > > + spx5_rmw(QFWD_SWITCH_PORT_MODE_PORT_ENA_SET(1), > > + QFWD_SWITCH_PORT_MODE_PORT_ENA, > > + sparx5, > > + QFWD_SWITCH_PORT_MODE(idx)); > > + } > > What happens when you enable the ports? Why is this here, and not in > the port specific open call? The comment is not correct. This is just enabling the CPU ports, so it belongs with the other switch core initialization. I will update the comment. > > > +/* Some boards needs to map the SGPIO for signal detect explicitly > > to the > > + * port module > > + */ > > +static void sparx5_board_init(struct sparx5 *sparx5) > > +{ > > + int idx; > > + > > + if (!sparx5->sd_sgpio_remapping) > > + return; > > + > > + /* Enable SGPIO Signal Detect remapping */ > > + spx5_rmw(GCB_HW_SGPIO_SD_CFG_SD_MAP_SEL, > > + GCB_HW_SGPIO_SD_CFG_SD_MAP_SEL, > > + sparx5, > > + GCB_HW_SGPIO_SD_CFG); > > + > > + /* Refer to LOS SGPIO */ > > + for (idx = 0; idx < SPX5_PORTS; idx++) { > > + if (sparx5->ports[idx]) { > > + if (sparx5->ports[idx]->conf.sd_sgpio != ~0) > > { > > + spx5_wr(sparx5->ports[idx]- > > >conf.sd_sgpio, > > + sparx5, > > + > > GCB_HW_SGPIO_TO_SD_MAP_CFG(idx)); > > + } > > + } > > + } > > +} > > I've not looked at how you do SFP integration yet. Is this the LOS > from the SFP socket? Is there a Linux GPIO controller exported by > this > driver, so the SFP driver can use the GPIOs? Yes the SFP driver (used by the Sparx5 SerDes driver) will use the SGPIO LOS, Module Detect etc, and the Port Modules are aware of the location of the LOS, and use this by default without any driver configuration. But on the PCB134 the SGPIOs are shifted one bit by a mistake, and they are not located in the expected position, so we have this board remapping function to handle that aspect. > > > + > > +static int mchp_sparx5_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + struct sparx5 *sparx5; > > + struct device_node *ports, *portnp; > > + const u8 *mac_addr; > > + int err = 0; > > + > > + if (!np && !pdev->dev.platform_data) > > + return -ENODEV; > > + > > + sparx5 = devm_kzalloc(&pdev->dev, sizeof(*sparx5), > > GFP_KERNEL); > > + if (!sparx5) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, sparx5); > > + sparx5->pdev = pdev; > > + sparx5->dev = &pdev->dev; > > + > > + /* Default values, some from DT */ > > + sparx5->coreclock = SPX5_CORE_CLOCK_DEFAULT; > > + > > + mac_addr = of_get_mac_address(np); > > + if (IS_ERR_OR_NULL(mac_addr)) { > > + dev_info(sparx5->dev, "MAC addr was not set, use > > random MAC\n"); > > + eth_random_addr(sparx5->base_mac); > > + sparx5->base_mac[5] = 0; > > + } else { > > + ether_addr_copy(sparx5->base_mac, mac_addr); > > + } > > The binding document does not say anything about a MAC address at the > top level. What is this used for? This the base MAC address used for generating the the switch NI's MAC addresses. > > + > > + if (sparx5_init(sparx5)) { > > + dev_err(sparx5->dev, "Init failed\n"); > > + return -ENODEV; > > + } > > + ports = of_get_child_by_name(np, "ethernet-ports"); > > + if (!ports) { > > + dev_err(sparx5->dev, "no ethernet-ports child node > > found\n"); > > + return -ENODEV; > > + } > > + sparx5->port_count = of_get_child_count(ports); > > + > > + for_each_available_child_of_node(ports, portnp) { > > + struct sparx5_port_config config = {}; > > + u32 portno; > > + struct phy *serdes; > > + > > + err = of_property_read_u32(portnp, "reg", &portno); > > + if (err) { > > + dev_err(sparx5->dev, "port reg property > > error\n"); > > + continue; > > + } > > + err = of_property_read_u32(portnp, "max-speed", > > + &config.max_speed); > > + if (err) { > > + dev_err(sparx5->dev, "port max-speed property > > error\n"); > > + continue; > > + } > > + config.speed = SPEED_UNKNOWN; > > + err = of_property_read_u32(portnp, "sd_sgpio", > > &config.sd_sgpio); > > Not in the binding documentation. I think i need to withdraw my > Reviewed-by :-( Ooops - yes that is a mistake that these 2 items were not included. > > > + if (err) > > + config.sd_sgpio = ~0; > > + else > > + sparx5->sd_sgpio_remapping = true; > > + serdes = devm_of_phy_get(sparx5->dev, portnp, NULL); > > + if (IS_ERR(serdes)) { > > + err = PTR_ERR(serdes); > > + if (err != -EPROBE_DEFER) > > + dev_err(sparx5->dev, > > + "missing SerDes phys for > > port%d\n", > > + portno); > > + return err; > > + } > > + > > + err = of_get_phy_mode(portnp, &config.phy_mode); > > + if (err) > > + config.power_down = true; > > You should indicate in the binding it is optional. And what happens > when it is missing. Will update the description. > > > + config.media_type = ETH_MEDIA_DAC; > > + config.serdes_reset = true; > > + config.portmode = config.phy_mode; > > + err = sparx5_probe_port(sparx5, portnp, serdes, > > portno, &config); > > + if (err) { > > + dev_err(sparx5->dev, "port probe error\n"); > > + goto cleanup_ports; > > + } > > + } > > + sparx5_board_init(sparx5); > > + > > +cleanup_ports: > > + return err; > > Seems missed named, no cleanup. Ah - this comes later (as the driver was split in functional groups for reviewing). I hope this is OK, as it is only temporary - I could add a comment to that effect. > > > +static int __init sparx5_switch_reset(void) > > +{ > > + const char *syscon_cpu = "microchip,sparx5-cpu-syscon", > > + *syscon_gcb = "microchip,sparx5-gcb-syscon"; > > + struct regmap *cpu_ctrl, *gcb_ctrl; > > + u32 val; > > + > > + cpu_ctrl = syscon_regmap_lookup_by_compatible(syscon_cpu); > > + if (IS_ERR(cpu_ctrl)) { > > + pr_err("No '%s' syscon map\n", syscon_cpu); > > + return PTR_ERR(cpu_ctrl); > > + } > > + > > + gcb_ctrl = syscon_regmap_lookup_by_compatible(syscon_gcb); > > + if (IS_ERR(gcb_ctrl)) { > > + pr_err("No '%s' syscon map\n", syscon_gcb); > > + return PTR_ERR(gcb_ctrl); > > + } > > + > > + /* Make sure the core is PROTECTED from reset */ > > + regmap_update_bits(cpu_ctrl, RESET_PROT_STAT, > > + SYS_RST_PROT_VCORE, SYS_RST_PROT_VCORE); > > + > > + regmap_write(gcb_ctrl, spx5_offset(GCB_SOFT_RST), > > + GCB_SOFT_RST_SOFT_SWC_RST_SET(1)); > > + > > + return readx_poll_timeout(sparx5_read_gcb_soft_rst, gcb_ctrl, > > val, > > + GCB_SOFT_RST_SOFT_SWC_RST_GET(val) > > == 0, > > + 1, 100); > > +} > > +postcore_initcall(sparx5_switch_reset); > > That is pretty unusual. Why cannot this be done at probe time? The problem is that the switch core reset also affects (reset) the SGPIO controller. We tried to put this in the reset driver, but it was rejected. If the reset is done at probe time, the SGPIO driver may already have initialized state. The switch core reset will then reset all SGPIO registers. > > > +/* Clock period in picoseconds */ > > +static inline u32 sparx5_clk_period(enum sparx5_core_clockfreq > > cclock) > > +{ > > + switch (cclock) { > > + case SPX5_CORE_CLOCK_250MHZ: > > + return 4000; > > + case SPX5_CORE_CLOCK_500MHZ: > > + return 2000; > > + case SPX5_CORE_CLOCK_625MHZ: > > + default: > > + return 1600; > > + } > > +} > > Is this something which is used in the hot path? No - so maybe this should just be a regular function? > > > --- /dev/null > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h > > @@ -0,0 +1,3922 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ > > + * Microchip Sparx5 Switch driver > > + * > > + * Copyright (c) 2020 Microchip Technology Inc. > > + */ > > + > > +/* This file is autogenerated by cml-utils 2020-11-19 10:41:34 > > +0100. > > + * Commit ID: f34790e69dc252103e2cc3e85b1a5e4d9e3aa190 > > + */ > > How reproducible this is generation process? If you have to run it > again, will it keep the same order of lines? As long as the CML (Chip Markup Language) file has not changed (documentation fields not considered), this is reproducible. The tool parses the XML nodes in a deterministic order. > > Andrew Thanks for your comments /Steen
> > > +static void sparx5_board_init(struct sparx5 *sparx5) > > > +{ > > > + int idx; > > > + > > > + if (!sparx5->sd_sgpio_remapping) > > > + return; > > > + > > > + /* Enable SGPIO Signal Detect remapping */ > > > + spx5_rmw(GCB_HW_SGPIO_SD_CFG_SD_MAP_SEL, > > > + GCB_HW_SGPIO_SD_CFG_SD_MAP_SEL, > > > + sparx5, > > > + GCB_HW_SGPIO_SD_CFG); > > > + > > > + /* Refer to LOS SGPIO */ > > > + for (idx = 0; idx < SPX5_PORTS; idx++) { > > > + if (sparx5->ports[idx]) { > > > + if (sparx5->ports[idx]->conf.sd_sgpio != ~0) > > > { > > > + spx5_wr(sparx5->ports[idx]- > > > >conf.sd_sgpio, > > > + sparx5, > > > + > > > GCB_HW_SGPIO_TO_SD_MAP_CFG(idx)); > > > + } > > > + } > > > + } > > > +} > > > > I've not looked at how you do SFP integration yet. Is this the LOS > > from the SFP socket? Is there a Linux GPIO controller exported by > > this > > driver, so the SFP driver can use the GPIOs? > > Yes the SFP driver (used by the Sparx5 SerDes driver) will use the > SGPIO LOS, Module Detect etc, and the Port Modules are aware of the > location of the LOS, and use this by default without any driver > configuration. > But on the PCB134 the SGPIOs are shifted one bit by a mistake, and they > are not located in the expected position, so we have this board > remapping function to handle that aspect. Is it possible to turn this off in the hardware? It might be less confusing if LOS it determined by phylink, not phylink and the switch itself. Especially when we get into race conditions between PHYLINK polling the GPIO and the hardware taking the short cut? > > > +static int mchp_sparx5_probe(struct platform_device *pdev) > > > +{ > > > + struct device_node *np = pdev->dev.of_node; > > > + struct sparx5 *sparx5; > > > + struct device_node *ports, *portnp; > > > + const u8 *mac_addr; > > > + int err = 0; > > > + > > > + if (!np && !pdev->dev.platform_data) > > > + return -ENODEV; > > > + > > > + sparx5 = devm_kzalloc(&pdev->dev, sizeof(*sparx5), > > > GFP_KERNEL); > > > + if (!sparx5) > > > + return -ENOMEM; > > > + > > > + platform_set_drvdata(pdev, sparx5); > > > + sparx5->pdev = pdev; > > > + sparx5->dev = &pdev->dev; > > > + > > > + /* Default values, some from DT */ > > > + sparx5->coreclock = SPX5_CORE_CLOCK_DEFAULT; > > > + > > > + mac_addr = of_get_mac_address(np); > > > + if (IS_ERR_OR_NULL(mac_addr)) { > > > + dev_info(sparx5->dev, "MAC addr was not set, use > > > random MAC\n"); > > > + eth_random_addr(sparx5->base_mac); > > > + sparx5->base_mac[5] = 0; > > > + } else { > > > + ether_addr_copy(sparx5->base_mac, mac_addr); > > > + } > > > > The binding document does not say anything about a MAC address at the > > top level. What is this used for? > > This the base MAC address used for generating the the switch NI's MAC > addresses. Yes, that is obvious from the code. But all DT properties must be in the binding Documentation. The DT verifier is going to complain when it finds a mac-address property which is not described in the yaml file. > > > + config.media_type = ETH_MEDIA_DAC; > > > + config.serdes_reset = true; > > > + config.portmode = config.phy_mode; > > > + err = sparx5_probe_port(sparx5, portnp, serdes, > > > portno, &config); > > > + if (err) { > > > + dev_err(sparx5->dev, "port probe error\n"); > > > + goto cleanup_ports; > > > + } > > > + } > > > + sparx5_board_init(sparx5); > > > + > > > +cleanup_ports: > > > + return err; > > > > Seems missed named, no cleanup. > > Ah - this comes later (as the driver was split in functional groups for > reviewing). I hope this is OK, as it is only temporary - I could add a > comment to that effect. Yes, this is fine. Here, and in other places, a comment like: /* More code to be added in later patches */ would of been nice, just as a heads up. That is the problem with linear patch review. > > > +static int __init sparx5_switch_reset(void) > > > +{ > > > + const char *syscon_cpu = "microchip,sparx5-cpu-syscon", > > > + *syscon_gcb = "microchip,sparx5-gcb-syscon"; > > > + struct regmap *cpu_ctrl, *gcb_ctrl; > > > + u32 val; > > > + > > > + cpu_ctrl = syscon_regmap_lookup_by_compatible(syscon_cpu); > > > + if (IS_ERR(cpu_ctrl)) { > > > + pr_err("No '%s' syscon map\n", syscon_cpu); > > > + return PTR_ERR(cpu_ctrl); > > > + } > > > + > > > + gcb_ctrl = syscon_regmap_lookup_by_compatible(syscon_gcb); > > > + if (IS_ERR(gcb_ctrl)) { > > > + pr_err("No '%s' syscon map\n", syscon_gcb); > > > + return PTR_ERR(gcb_ctrl); > > > + } > > > + > > > + /* Make sure the core is PROTECTED from reset */ > > > + regmap_update_bits(cpu_ctrl, RESET_PROT_STAT, > > > + SYS_RST_PROT_VCORE, SYS_RST_PROT_VCORE); > > > + > > > + regmap_write(gcb_ctrl, spx5_offset(GCB_SOFT_RST), > > > + GCB_SOFT_RST_SOFT_SWC_RST_SET(1)); > > > + > > > + return readx_poll_timeout(sparx5_read_gcb_soft_rst, gcb_ctrl, > > > val, > > > + GCB_SOFT_RST_SOFT_SWC_RST_GET(val) > > > == 0, > > > + 1, 100); > > > +} > > > +postcore_initcall(sparx5_switch_reset); > > > > That is pretty unusual. Why cannot this be done at probe time? > > The problem is that the switch core reset also affects (reset) the > SGPIO controller. > > We tried to put this in the reset driver, but it was rejected. If the > reset is done at probe time, the SGPIO driver may already have > initialized state. > > The switch core reset will then reset all SGPIO registers. Ah, O.K. Dumb question. Why is the SGPIO driver a separate driver? It sounds like it should be embedded inside this driver if it is sharing hardware. Another option would be to look at the reset subsystem, and have this driver export a reset controller, which the SGPIO driver can bind to. Given that the GPIO driver has been merged, if this will work, it is probably a better solution. Andrew
On 22/12/2020 16:01:22+0100, Andrew Lunn wrote: > > The problem is that the switch core reset also affects (reset) the > > SGPIO controller. > > > > We tried to put this in the reset driver, but it was rejected. If the > > reset is done at probe time, the SGPIO driver may already have > > initialized state. > > > > The switch core reset will then reset all SGPIO registers. > > Ah, O.K. Dumb question. Why is the SGPIO driver a separate driver? It > sounds like it should be embedded inside this driver if it is sharing > hardware. > > Another option would be to look at the reset subsystem, and have this > driver export a reset controller, which the SGPIO driver can bind to. > Given that the GPIO driver has been merged, if this will work, it is > probably a better solution. > That was my suggestion. Then you can ensure from the reset controller driver that this is done exactly once, either from the sgpio driver or from the switchdev driver. IIRC, the sgpio from the other SoCs are not affected by the reset.
On 22.12.2020 16:01, Andrew Lunn wrote: >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> > > +static void sparx5_board_init(struct sparx5 *sparx5) >> > > +{ >> > > + int idx; >> > > + >> > > + if (!sparx5->sd_sgpio_remapping) >> > > + return; >> > > + >> > > + /* Enable SGPIO Signal Detect remapping */ >> > > + spx5_rmw(GCB_HW_SGPIO_SD_CFG_SD_MAP_SEL, >> > > + GCB_HW_SGPIO_SD_CFG_SD_MAP_SEL, >> > > + sparx5, >> > > + GCB_HW_SGPIO_SD_CFG); >> > > + >> > > + /* Refer to LOS SGPIO */ >> > > + for (idx = 0; idx < SPX5_PORTS; idx++) { >> > > + if (sparx5->ports[idx]) { >> > > + if (sparx5->ports[idx]->conf.sd_sgpio != ~0) >> > > { >> > > + spx5_wr(sparx5->ports[idx]- >> > > >conf.sd_sgpio, >> > > + sparx5, >> > > + >> > > GCB_HW_SGPIO_TO_SD_MAP_CFG(idx)); >> > > + } >> > > + } >> > > + } >> > > +} >> > >> > I've not looked at how you do SFP integration yet. Is this the LOS >> > from the SFP socket? Is there a Linux GPIO controller exported by >> > this >> > driver, so the SFP driver can use the GPIOs? >> >> Yes the SFP driver (used by the Sparx5 SerDes driver) will use the >> SGPIO LOS, Module Detect etc, and the Port Modules are aware of the >> location of the LOS, and use this by default without any driver >> configuration. >> But on the PCB134 the SGPIOs are shifted one bit by a mistake, and they >> are not located in the expected position, so we have this board >> remapping function to handle that aspect. > >Is it possible to turn this off in the hardware? It might be less >confusing if LOS it determined by phylink, not phylink and the switch >itself. Especially when we get into race conditions between PHYLINK >polling the GPIO and the hardware taking the short cut? > OK - I get you point, but I think the message I got when investigating this, was that it was not possible to turn it off. I will check that again. On the other hand this is also used by our bare-metal API (MESA) so in that context it simpifies the setup, since the port modules are aware of the SFP state. > >> > > +static int mchp_sparx5_probe(struct platform_device *pdev) >> > > +{ >> > > + struct device_node *np = pdev->dev.of_node; >> > > + struct sparx5 *sparx5; >> > > + struct device_node *ports, *portnp; >> > > + const u8 *mac_addr; >> > > + int err = 0; >> > > + >> > > + if (!np && !pdev->dev.platform_data) >> > > + return -ENODEV; >> > > + >> > > + sparx5 = devm_kzalloc(&pdev->dev, sizeof(*sparx5), >> > > GFP_KERNEL); >> > > + if (!sparx5) >> > > + return -ENOMEM; >> > > + >> > > + platform_set_drvdata(pdev, sparx5); >> > > + sparx5->pdev = pdev; >> > > + sparx5->dev = &pdev->dev; >> > > + >> > > + /* Default values, some from DT */ >> > > + sparx5->coreclock = SPX5_CORE_CLOCK_DEFAULT; >> > > + >> > > + mac_addr = of_get_mac_address(np); >> > > + if (IS_ERR_OR_NULL(mac_addr)) { >> > > + dev_info(sparx5->dev, "MAC addr was not set, use >> > > random MAC\n"); >> > > + eth_random_addr(sparx5->base_mac); >> > > + sparx5->base_mac[5] = 0; >> > > + } else { >> > > + ether_addr_copy(sparx5->base_mac, mac_addr); >> > > + } >> > >> > The binding document does not say anything about a MAC address at the >> > top level. What is this used for? >> >> This the base MAC address used for generating the the switch NI's MAC >> addresses. > >Yes, that is obvious from the code. But all DT properties must be in >the binding Documentation. The DT verifier is going to complain when >it finds a mac-address property which is not described in the yaml >file. I will add a description for the MAC address to the bindings. > >> > > + config.media_type = ETH_MEDIA_DAC; >> > > + config.serdes_reset = true; >> > > + config.portmode = config.phy_mode; >> > > + err = sparx5_probe_port(sparx5, portnp, serdes, >> > > portno, &config); >> > > + if (err) { >> > > + dev_err(sparx5->dev, "port probe error\n"); >> > > + goto cleanup_ports; >> > > + } >> > > + } >> > > + sparx5_board_init(sparx5); >> > > + >> > > +cleanup_ports: >> > > + return err; >> > >> > Seems missed named, no cleanup. >> >> Ah - this comes later (as the driver was split in functional groups for >> reviewing). I hope this is OK, as it is only temporary - I could add a >> comment to that effect. > >Yes, this is fine. Here, and in other places, a comment like: > >/* More code to be added in later patches */ > >would of been nice, just as a heads up. That is the problem with >linear patch review. Will do > >> > > +static int __init sparx5_switch_reset(void) >> > > +{ >> > > + const char *syscon_cpu = "microchip,sparx5-cpu-syscon", >> > > + *syscon_gcb = "microchip,sparx5-gcb-syscon"; >> > > + struct regmap *cpu_ctrl, *gcb_ctrl; >> > > + u32 val; >> > > + >> > > + cpu_ctrl = syscon_regmap_lookup_by_compatible(syscon_cpu); >> > > + if (IS_ERR(cpu_ctrl)) { >> > > + pr_err("No '%s' syscon map\n", syscon_cpu); >> > > + return PTR_ERR(cpu_ctrl); >> > > + } >> > > + >> > > + gcb_ctrl = syscon_regmap_lookup_by_compatible(syscon_gcb); >> > > + if (IS_ERR(gcb_ctrl)) { >> > > + pr_err("No '%s' syscon map\n", syscon_gcb); >> > > + return PTR_ERR(gcb_ctrl); >> > > + } >> > > + >> > > + /* Make sure the core is PROTECTED from reset */ >> > > + regmap_update_bits(cpu_ctrl, RESET_PROT_STAT, >> > > + SYS_RST_PROT_VCORE, SYS_RST_PROT_VCORE); >> > > + >> > > + regmap_write(gcb_ctrl, spx5_offset(GCB_SOFT_RST), >> > > + GCB_SOFT_RST_SOFT_SWC_RST_SET(1)); >> > > + >> > > + return readx_poll_timeout(sparx5_read_gcb_soft_rst, gcb_ctrl, >> > > val, >> > > + GCB_SOFT_RST_SOFT_SWC_RST_GET(val) >> > > == 0, >> > > + 1, 100); >> > > +} >> > > +postcore_initcall(sparx5_switch_reset); >> > >> > That is pretty unusual. Why cannot this be done at probe time? >> >> The problem is that the switch core reset also affects (reset) the >> SGPIO controller. >> >> We tried to put this in the reset driver, but it was rejected. If the >> reset is done at probe time, the SGPIO driver may already have >> initialized state. >> >> The switch core reset will then reset all SGPIO registers. > >Ah, O.K. Dumb question. Why is the SGPIO driver a separate driver? It >sounds like it should be embedded inside this driver if it is sharing >hardware. The same SGPIO block is present (with suitable scaling of the number of SGPIOS) in all our switches, so this driver will be reused on all the platforms when we get them upstreamed (or at least that is the plan). > >Another option would be to look at the reset subsystem, and have this >driver export a reset controller, which the SGPIO driver can bind to. >Given that the GPIO driver has been merged, if this will work, it is >probably a better solution. Alex has already commented on this, but this is probably the goal as I understand. > > Andrew BR Steen --------------------------------------- Steen Hegelund steen.hegelund@microchip.com
Alexandre Belloni writes: > On 22/12/2020 16:01:22+0100, Andrew Lunn wrote: >> > The problem is that the switch core reset also affects (reset) the >> > SGPIO controller. >> > >> > We tried to put this in the reset driver, but it was rejected. If the >> > reset is done at probe time, the SGPIO driver may already have >> > initialized state. >> > >> > The switch core reset will then reset all SGPIO registers. >> >> Ah, O.K. Dumb question. Why is the SGPIO driver a separate driver? It >> sounds like it should be embedded inside this driver if it is sharing >> hardware. >> >> Another option would be to look at the reset subsystem, and have this >> driver export a reset controller, which the SGPIO driver can bind to. >> Given that the GPIO driver has been merged, if this will work, it is >> probably a better solution. >> > > That was my suggestion. Then you can ensure from the reset controller > driver that this is done exactly once, either from the sgpio driver or > from the switchdev driver. IIRC, the sgpio from the other SoCs are not > affected by the reset. I will take a look to see if we can change the implementation to use a reset controller. ---Lars -- Lars Povlsen, Microchip
This series provides the Microchip Sparx5 Switch Driver The Sparx5 Carrier Ethernet and Industrial switch family delivers 64 Ethernet ports and up to 200 Gbps of switching bandwidth. It provides a rich set of Ethernet switching features such as hierarchical QoS, hardware-based OAM and service activation testing, protection switching, IEEE 1588, and Synchronous Ethernet. Using provider bridging (Q-in-Q) and MPLS/MPLS-TP technology, it delivers MEF CE 2.0 Ethernet virtual connections (EVCs) and features advanced TCAM classification in both ingress and egress. Per-EVC features include advanced L3-aware classification, a rich set of statistics, OAM for end-to-end performance monitoring, and dual-rate policing and shaping. Time sensitive networking (TSN) is supported through a comprehensive set of features including frame preemption, cut-through, frame replication and elimination for reliability, enhanced scheduling: credit-based shaping, time-aware shaping, cyclic queuing, and forwarding, and per-stream policing and filtering. Together with IEEE 1588 and IEEE 802.1AS support, this guarantees low-latency deterministic networking for Fronthaul, Carrier, and Industrial Ethernet. The Sparx5 switch family consists of following SKUs: - VSC7546 Sparx5-64 up to 64 Gbps of bandwidth with the following primary port configurations: - 6 *10G - 16 * 2.5G + 2 * 10G - 24 * 1G + 4 * 10G - VSC7549 Sparx5-90 up to 90 Gbps of bandwidth with the following primary port configurations: - 9 * 10G - 16 * 2.5G + 4 * 10G - 48 * 1G + 4 * 10G - VSC7552 Sparx5-128 up to 128 Gbps of bandwidth with the following primary port configurations: - 12 * 10G - 16 * 2.5G + 8 * 10G - 48 * 1G + 8 * 10G - VSC7556 Sparx5-160 up to 160 Gbps of bandwidth with the following primary port configurations: - 16 * 10G - 10 * 10G + 2 * 25G - 16 * 2.5G + 10 * 10G - 48 * 1G + 10 * 10G - VSC7558 Sparx5-200 up to 200 Gbps of bandwidth with the following primary port configurations: - 20 * 10G - 8 * 25G In addition, the device supports one 10/100/1000/2500/5000 Mbps SGMII/SerDes node processor interface (NPI) Ethernet port. The Sparx5 support is developed on the PCB134 and PCB135 evaluation boards. - PCB134 main networking features: - 12x SFP+ front 10G module slots (connected to Sparx5 through SFI). - 8x SFP28 front 25G module slots (connected to Sparx5 through SFI high speed). - Optional, one additional 10/100/1000BASE-T (RJ45) Ethernet port (on-board VSC8211 PHY connected to Sparx5 through SGMII). - PCB135 main networking features: - 48x1G (10/100/1000M) RJ45 front ports using 12xVSC8514 QuadPHY’s each connected to VSC7558 through QSGMII. - 4x10G (1G/2.5G/5G/10G) RJ45 front ports using the AQR407 10G QuadPHY each port connects to VSC7558 through SFI. - 4x SFP28 25G module slots on back connected to VSC7558 through SFI high speed. - Optional, one additional 1G (10/100/1000M) RJ45 port using an on-board VSC8211 PHY, which can be connected to VSC7558 NPI port through SGMII using a loopback add-on PCB) This series provides support for: - SFPs and DAC cables via PHYLINK with a number of 5G, 10G and 25G devices and media types. - Port module configuration for 10M to 25G speeds with SGMII, QSGMII, 1000BASEX, 2500BASEX and 10GBASER as appropriate for these modes. - SerDes configuration via the Sparx5 SerDes driver (see below). - Host mode providing register based injection and extraction. - Switch mode providing MAC/VLAN table learning and Layer2 switching offloaded to the Sparx5 switch. - STP state, VLAN support, host/bridge port mode, Forwarding DB, and configuration and statistics via ethtool. More support will be added at a later stage. The Sparx5 Switch chip register model can be browsed here: Link: https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html The series depends on the following series currently on their way into the kernel: - Sparx5 SerDes Driver Link: https://lore.kernel.org/r/20201211090541.157926-1-steen.hegelund@microchip.com/ - Serial GPIO Controller Link: https://lore.kernel.org/r/20201113145151.68900-1-lars.povlsen@microchip.com/ ChangeLog: v2: - The driver patch has been split into 6 patches by functionality like this: - the basic sparx5 driver - hostmode with phylink support - port module support - switching, vlan and mactable support - calendar bandwidth allocation support - ethtool configuration and statistics support - IO ranges have been collapsed into just 2 (the SerDes driver uses the area inbetween) and the driver uses an offset table to get the target instances. - register macros have been converted to functions - register_netdev() moved to the end of the switch initialization. - sparx5_update_port_stats: use reverse christmas tree - sparx5_get_sset_strings: copy individual strings - sparx5_port_open: updated to better use phylink: just call phylink_of_phy_connect directly - sparx5_destroy_netdev: always take the NL lock - sparx5_attr_stp_state_set: added learning state. - sparx5_phylink_mac_config: use phylink to provide the status for the devices phylink controls. - sparx5_get_1000basex_status: renamed to sparx5_get_dev2g5_status and corrected an error when combining the sync and link status information. - let phylink provide link status for cuPHYs and SFPs - corrected the pause mode status handling - use ethtool's get_link function directly - remove the use of the phy_validate function - sparx5_update_counter function: no longer inline - Removed the wrapper functions around the mactable mutex Steen Hegelund (8): dt-bindings: net: sparx5: Add sparx5-switch bindings net: sparx5: add the basic sparx5 driver net: sparx5: add hostmode with phylink support net: sparx5: add port module support net: sparx5: add switching, vlan and mactable support net: sparx5: add calendar bandwidth allocation support net: sparx5: add ethtool configuration and statistics support arm64: dts: sparx5: Add the Sparx5 switch node .../bindings/net/microchip,sparx5-switch.yaml | 178 + arch/arm64/boot/dts/microchip/sparx5.dtsi | 60 + .../dts/microchip/sparx5_pcb134_board.dtsi | 424 +- .../dts/microchip/sparx5_pcb135_board.dtsi | 602 ++- drivers/net/ethernet/microchip/Kconfig | 2 + drivers/net/ethernet/microchip/Makefile | 2 + drivers/net/ethernet/microchip/sparx5/Kconfig | 8 + .../net/ethernet/microchip/sparx5/Makefile | 11 + .../microchip/sparx5/sparx5_calendar.c | 595 +++ .../microchip/sparx5/sparx5_ethtool.c | 979 ++++ .../microchip/sparx5/sparx5_mactable.c | 502 +++ .../ethernet/microchip/sparx5/sparx5_main.c | 855 ++++ .../ethernet/microchip/sparx5/sparx5_main.h | 372 ++ .../microchip/sparx5/sparx5_main_regs.h | 3922 +++++++++++++++++ .../ethernet/microchip/sparx5/sparx5_netdev.c | 246 ++ .../ethernet/microchip/sparx5/sparx5_packet.c | 279 ++ .../microchip/sparx5/sparx5_phylink.c | 193 + .../ethernet/microchip/sparx5/sparx5_port.c | 1140 +++++ .../ethernet/microchip/sparx5/sparx5_port.h | 98 + .../microchip/sparx5/sparx5_switchdev.c | 516 +++ .../ethernet/microchip/sparx5/sparx5_vlan.c | 223 + 21 files changed, 11147 insertions(+), 60 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml create mode 100644 drivers/net/ethernet/microchip/sparx5/Kconfig create mode 100644 drivers/net/ethernet/microchip/sparx5/Makefile create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_calendar.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_ethtool.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_main.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_main.h create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_packet.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_port.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_port.h create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_vlan.c -- 2.29.2