| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <37309f64bf0bb94e55bc2db4c482c1e3e7f1be6f.camel@microchip.com>
Date: Tue, 22 Dec 2020 14:50:02 +0100
From: Steen Hegelund <steen.hegelund@...rochip.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Russell King <linux@...linux.org.uk>,
Lars Povlsen <lars.povlsen@...rochip.com>,
Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Madalin Bucur <madalin.bucur@....nxp.com>,
Nicolas Ferre <nicolas.ferre@...rochip.com>,
Mark Einon <mark.einon@...il.com>,
Masahiro Yamada <masahiroy@...nel.org>,
"Arnd Bergmann" <arnd@...db.de>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC PATCH v2 2/8] net: sparx5: add the basic sparx5 driver
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
Powered by blists - more mailing lists