Re: [RFC PATCH v2 2/8] net: sparx5: add the basic sparx5 driver

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?

How reproducible this is generation process? If you have to run it
again, will it keep the same order of lines?

       Andrew