Re: [PATCH net-next v2 02/10] net: sparx5: add the basic sparx5 driver

[Steen Hegelund <steen.hegelund@microchip.com>]

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:
> > 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.
> 
> > +     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.

> 
> > +
> > +     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;
> > +}
> 
> > +     /* 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.

> 
> > +     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.

> 
> > +                     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.

> 
> > +     }
> > +
> > +     /* 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.

> 
> > +     }
> > +
> > +     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.

> 
> > +     }
> > +
> > +     kfree(configs);
> > +     return err;
> > +
> > +cleanup_ports:
> > +     /* Port cleanup to be added in later patches */
> > +cleanup_config:
> > +     kfree(configs);
> > +     return err;
> > +}
> 
> > +struct sparx5_port_config      {
> 
> Spurious tab before {?

Spurious spaces - but they will be removed.

> 
> > +     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?

> 
> > +     phy_interface_t phy_mode;
> > +     u32 sd_sgpio;
> > +};
> 
> > +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.

> 
> > +                       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.

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


Thanks!

-- 
BR
Steen

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