Hi Jacub,

Thanks for your comments.

On Sun, 2021-05-30 at 13:59 -0700, Jakub Kicinski wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Fri, 28 May 2021 14:34:11 +0200 Steen Hegelund wrote:

quoted

This adds the Sparx5 basic SwitchDev driver framework with IO range
mapping, switch device detection and core clock configuration.

Support for ports, phylink, netdev, mactable etc. are in the following
patches.

quoted

+     for (idx = 0; idx < 3; idx++) {
+             spx5_rmw(GCB_SIO_CLOCK_SYS_CLK_PERIOD_SET(clk_period / 100),
+                      GCB_SIO_CLOCK_SYS_CLK_PERIOD,
+                      sparx5,
+                      GCB_SIO_CLOCK(idx));
+     }

braces unnecessary, please fix everywhere.

Will do that.

quoted

+
+     spx5_rmw(HSCH_TAS_STATEMACHINE_CFG_REVISIT_DLY_SET
+              ((256 * 1000) / clk_period),
+              HSCH_TAS_STATEMACHINE_CFG_REVISIT_DLY,
+              sparx5,
+              HSCH_TAS_STATEMACHINE_CFG);
+
+     spx5_rmw(ANA_AC_POL_POL_UPD_INT_CFG_POL_UPD_INT_SET(pol_upd_int),
+              ANA_AC_POL_POL_UPD_INT_CFG_POL_UPD_INT,
+              sparx5,
+              ANA_AC_POL_POL_UPD_INT_CFG);
+
+     return 0;
+}

quoted

+     /* Default values, some from DT */
+     sparx5->coreclock = SPX5_CORE_CLOCK_DEFAULT;
+
+     ports = of_get_child_by_name(np, "ethernet-ports");

Don't you need to release the reference you got on @ports?

Yes that is missing.  I will update.

quoted

+     if (!ports) {
+             dev_err(sparx5->dev, "no ethernet-ports child node found\n");
+             return -ENODEV;
+     }
+     sparx5->port_count = of_get_child_count(ports);
+
+     configs = kcalloc(sparx5->port_count,
+                       sizeof(struct initial_port_config), GFP_KERNEL);
+     if (!configs)
+             return -ENOMEM;
+
+     for_each_available_child_of_node(ports, portnp) {
+             struct sparx5_port_config *conf;
+             struct phy *serdes;
+             u32 portno;
+
+             err = of_property_read_u32(portnp, "reg", &portno);
+             if (err) {
+                     dev_err(sparx5->dev, "port reg property error\n");
+                     continue;
+             }
+             config = &configs[idx];
+             conf = &config->conf;
+             err = of_get_phy_mode(portnp, &conf->phy_mode);
+             if (err) {
+                     dev_err(sparx5->dev, "port %u: missing phy-mode\n",
+                             portno);
+                     continue;
+             }
+             err = of_property_read_u32(portnp, "microchip,bandwidth",
+                                        &conf->bandwidth);
+             if (err) {
+                     dev_err(sparx5->dev, "port %u: missing bandwidth\n",
+                             portno);
+                     continue;
+             }
+             err = of_property_read_u32(portnp, "microchip,sd-sgpio", &conf->sd_sgpio);
+             if (err)
+                     conf->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,
+                                     "port %u: missing serdes\n",
+                                     portno);

dev_err_probe()

OK - did not know that one.

quoted

+                     goto cleanup_config;
+             }
+             config->portno = portno;
+             config->node = portnp;
+             config->serdes = serdes;
+
+             conf->media = PHY_MEDIA_DAC;
+             conf->serdes_reset = true;
+             conf->portmode = conf->phy_mode;
+             if (of_find_property(portnp, "sfp", NULL)) {
+                     conf->has_sfp = true;
+                     conf->power_down = true;
+             }
+             idx++;
+     }
+
+     err = sparx5_create_targets(sparx5);
+     if (err)
+             goto cleanup_config;
+
+     if (of_get_mac_address(np, 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);
+     }
+
+     /* Inj/Xtr IRQ support to be added in later patches */
+     /* 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");
+             goto cleanup_config;

Should @err be set?

Yes it should.  I will update here and below.

quoted

+     }
+
+     /* Initialize the LC-PLL (core clock) and set affected registers */
+     if (sparx5_init_coreclock(sparx5)) {
+             dev_err(sparx5->dev, "LC-PLL initialization error\n");
+             goto cleanup_config;

ditto

Yes.

quoted

+     }
+
+     for (idx = 0; idx < sparx5->port_count; ++idx) {
+             config = &configs[idx];
+             if (!config->node)
+                     continue;
+
+             err = sparx5_create_port(sparx5, config);
+             if (err) {
+                     dev_err(sparx5->dev, "port create error\n");
+                     goto cleanup_ports;
+             }
+     }
+
+     if (sparx5_start(sparx5)) {
+             dev_err(sparx5->dev, "Start failed\n");
+             goto cleanup_ports;

and here

Yes.

quoted

+     }
+
+     kfree(configs);
+     return err;
+
+cleanup_ports:
+     /* Port cleanup to be added in later patches */
+cleanup_config:
+     kfree(configs);
+     return err;
+}

quoted

+struct sparx5_port_config      {

Spurious tab before {?

Spurious spaces - but they will be removed.

quoted

+     phy_interface_t portmode;
+     bool has_sfp;
+     u32 bandwidth;
+     int speed;
+     int duplex;
+     enum phy_media media;
+     bool power_down;
+     bool autoneg;
+     u32 pause;
+     bool serdes_reset;

Group all 4 bools together for better packing?

Yes that saves some bytes.  Would bitfields be preferable or are bools sufficient?

quoted

+     phy_interface_t phy_mode;
+     u32 sd_sgpio;
+};

quoted

+static inline void spx5_rmw(u32 val, u32 mask, struct sparx5 *sparx5,
+                         int id, int tinst, int tcnt,
+                         int gbase, int ginst, int gcnt, int gwidth,
+                         int raddr, int rinst, int rcnt, int rwidth)
+{
+     u32 nval;
+     void __iomem *addr =
+             spx5_addr(sparx5->regs, id, tinst, tcnt,

Why try to initialize inline when it results in weird looking code and
no saved lines?

Hmm, I had not really noticed that... I will just use the spx5_addr call in both places.

quoted

+                       gbase, ginst, gcnt, gwidth,
+                       raddr, rinst, rcnt, rwidth);

Not to mention that you end up with no new line after variable
declaration.

Yes.  I will add an empty line.

quoted

+     nval = readl(addr);
+     nval = (nval & ~mask) | (val & mask);
+     writel(nval, addr);
+}

Thanks!

-- 
BR
Steen

-=-=-=-=-=-=-=-=-=-=-=-=-=-=
steen.hegelund@microchip.com



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel