Re: [PATCH net-next v4 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: lan9645x: add basic dsa driver for LAN9645X

This patch introduces a new DSA driver for the Microchip LAN9645X switch
family, covering probe/setup/teardown, NPI CPU port wiring, per-port
initialisation, and phylink MAC integration. A few questions inline.

> diff --git a/drivers/net/dsa/microchip/lan9645x/Kconfig b/drivers/net/dsa/microchip/lan9645x/Kconfig
> new file mode 100644
> index 000000000000..4d9fdf34104e
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config NET_DSA_MICROCHIP_LAN9645X
> +	tristate "Microchip Lan9645x switch support"
> +	depends on NET_DSA
> +	select NET_DSA_TAG_LAN9645X
> +	help
> +	  This driver adds DSA support for Microchip Lan9645x switch chips.

The driver is non-functional without a parent MFD that registers the named
regmaps ("gcb", "qs", "chip_top", "rew", "sys", "hsio", "dev", "dev1" ..
"dev8", "qsys", "afi", "ana") consumed by lan9645x_request_target_regmaps().

Should this Kconfig depend on the parent provider symbol so that selecting
this option without the parent driver does not produce a module whose probe
always fails?

Is there a companion parent-driver patch this patch depends on? If so,
would it be worth naming it in the commit message and documenting the
ordering?

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> new file mode 100644
> index 000000000000..6fd66ea67cfd
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> @@ -0,0 +1,423 @@

[ ... ]

> +static const char *lan9645x_resource_names[NUM_TARGETS + 1] = {
> +	[TARGET_GCB]          = "gcb",
> +	[TARGET_QS]           = "qs",
> +	[TARGET_CHIP_TOP]     = "chip_top",
> +	[TARGET_REW]          = "rew",
> +	[TARGET_SYS]          = "sys",
> +	[TARGET_HSIO]         = "hsio",
> +	[TARGET_DEV]          = "dev",
> +	[TARGET_DEV + 1]      = "dev1",
> +	[TARGET_DEV + 2]      = "dev2",
> +	[TARGET_DEV + 3]      = "dev3",
> +	[TARGET_DEV + 4]      = "dev4",
> +	[TARGET_DEV + 5]      = "dev5",
> +	[TARGET_DEV + 6]      = "dev6",
> +	[TARGET_DEV + 7]      = "dev7",
> +	[TARGET_DEV + 8]      = "dev8",

The first DEV instance is named "dev", while the rest are "dev1" through
"dev8". Is this asymmetry intentional, or should the first be "dev0" to
match the natural per-port numbering? Without the parent driver available
in this series, it is hard to tell which naming scheme the parent actually
registers.

If the parent registers "dev0".."dev8" then dev_get_regmap(parent, "dev")
returns NULL and probe fails with -ENODEV.

[ ... ]

> +static void lan9645x_set_feat_dis(struct lan9645x *lan9645x)
> +{
> +	u32 feat_dis;
> +
> +	/* The features which can be physically disabled on some SKUs are:
> +	 * 1) Number of ports can be 5, 7 or 9. Any ports can be used, the chip
> +	 *    tracks how many are active.
> +	 * 2) HSR/PRP. The duplicate discard table can be disabled.
> +	 * 3) TAS, frame preemption and PSFP can be disabled.
> +	 */
> +	feat_dis = lan_rd(lan9645x, GCB_FEAT_DISABLE);
> +
> +	lan9645x->num_port_dis =
> +		GCB_FEAT_DISABLE_FEAT_NUM_PORTS_DIS_GET(feat_dis);
> +	lan9645x->dd_dis = GCB_FEAT_DISABLE_FEAT_DD_DIS_GET(feat_dis);
> +	lan9645x->tsn_dis = GCB_FEAT_DISABLE_FEAT_TSN_DIS_GET(feat_dis);
> +}

Where are num_port_dis, dd_dis and tsn_dis consulted after this read?

ds->num_ports is hardcoded to NUM_PHYS_PORTS (9) in lan9645x_probe(), and
there does not appear to be any rejection of TSN or HSR/PRP configuration
on SKUs where tsn_dis=1 or dd_dis=1 in this patch. On a 5-port SKU
(num_port_dis=4), would the driver still register 9 DSA ports and allow
DT to bring up a fused-off port?

> +static int lan9645x_setup(struct dsa_switch *ds)
> +{

[ ... ]

> +	dev_info(lan9645x->dev,
> +		 "SKU features: tsn_dis=%d hsr_dis=%d max_ports=%d\n",
> +		 lan9645x->tsn_dis, lan9645x->dd_dis,
> +		 lan9645x->num_phys_ports - lan9645x->num_port_dis);

The log prints the field called dd_dis in struct lan9645x under the name
hsr_dis. Should the label in the format string match the struct field
name, or should the field be renamed?

[ ... ]

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
> new file mode 100644
> index 000000000000..9eac32ca342c
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
> @@ -0,0 +1,383 @@

[ ... ]

> +/* Port 4 or 7 is RGMII_0 and port 8 is RGMII_1 */
> +#define RGMII_IDX(port) ((port) == 8 ? 1 : 0)
> +
> +void lan9645x_phylink_get_caps(struct lan9645x *lan9645x, int port,
> +			       struct phylink_config *c)
> +{
> +	c->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 |
> +			      MAC_100 | MAC_1000FD | MAC_2500FD;
> +
> +	switch (port) {
> +	case 0 ... 3:
> +		__set_bit(PHY_INTERFACE_MODE_GMII, c->supported_interfaces);
> +		break;
> +	case 4:
> +		__set_bit(PHY_INTERFACE_MODE_GMII, c->supported_interfaces);
> +		phy_interface_set_rgmii(c->supported_interfaces);
> +		break;
> +	case 5 ... 6:
> +		/* SerDes ports: QSGMII/SGMII/1000BASEX/2500BASEX modes
> +		 * require PCS support which is not yet implemented.
> +		 */
> +		break;
> +	case 7 ... 8:
> +		/* QSGMII mode on ports 7-8 requires SerDes PCS support,
> +		 * which is not yet implemented.
> +		 */
> +		phy_interface_set_rgmii(c->supported_interfaces);
> +		break;
> +	default:
> +		break;
> +	}
> +}

mac_capabilities advertises MAC_2500FD for every port, but none of the
supported_interfaces entries allow negotiating 2.5 Gb/s. GMII and RGMII
do not carry 2.5 Gb/s, and ports 5-6 have an empty supported_interfaces
bitmap. Should MAC_2500FD be dropped until a PCS-capable interface is
added, or should it be set only on ports that will actually support it?

For ports 5 and 6, supported_interfaces is left empty. Will phylink_create()
succeed for those ports given no interface mode is permitted, or will they
be silently unusable?

The commit message states:

    The lan9645x switch is a multi-port Gigabit AVB/TSN Ethernet Switch
    with five integrated 10/100/1000Base-T PHYs. In addition to the
    integrated PHYs, it supports up to 2 RGMII/RMII, up to 2
    BASE-X/SERDES/2.5GBASE-X and one Quad-SGMII/Quad-USGMII interfaces.

Given ports 5-6 have no supported_interfaces and ports 7-8 expose only
RGMII in this patch, would it help to note in the commit message that
SerDes, QSGMII/USGMII and 2.5GBASE-X are not wired up yet?

[ ... ]

> +static int lan9645x_phylink_mac_prepare(struct phylink_config *config,
> +					unsigned int mode,
> +					phy_interface_t iface)
> +{
> +	struct lan9645x_port *p = lan9645x_phylink_config_to_port(config);
> +	struct lan9645x *lan9645x = p->lan9645x;
> +	int port = p->chip_port;
> +	bool is_rgmii;
> +	u32 mask;
> +
> +	if (port == 5 || port == 6 || port > 8)
> +		return -EINVAL;
> +
> +	mask = HSIO_HW_CFG_GMII_ENA_SET(BIT(port));
> +	lan_rmw(mask, mask, lan9645x, HSIO_HW_CFG);
> +
> +	is_rgmii = phy_interface_mode_is_rgmii(iface);
> +	if (port == 4)
> +		lan_rmw(HSIO_HW_CFG_RGMII_0_CFG_SET(is_rgmii),
> +			HSIO_HW_CFG_RGMII_0_CFG,
> +			lan9645x, HSIO_HW_CFG);
> +
> +	return 0;
> +}

The HSIO_HW_CFG_RGMII_0_CFG bit is only written when port == 4, but the
comment above RGMII_IDX() states:

    /* Port 4 or 7 is RGMII_0 and port 8 is RGMII_1 */
    #define RGMII_IDX(port) ((port) == 8 ? 1 : 0)

Port 7 also maps to RGMII_0. If port 7 is configured for RGMII, does the
RGMII_0 mux ever get routed to port 7? And if port 4 is later configured
in GMII mode, the write HSIO_HW_CFG_RGMII_0_CFG_SET(0) would run and
appear to reroute RGMII_0 away from port 7.

Is there meant to be mutual-exclusion between port 4 and port 7 in RGMII
mode, and a write path for port 7 as well?

> +static void lan9645x_rgmii_dll_config(struct lan9645x_port *p)
> +{
> +	u32 rx_idx, tx_idx;
> +
> +	/* DLL register layout:
> +	 * (N*2):   RGMII_N_RX
> +	 * (N*2)+1: RGMII_N_TX
> +	 */
> +	rx_idx = RGMII_IDX(p->chip_port) * 2;
> +	tx_idx = RGMII_IDX(p->chip_port) * 2 + 1;
> +
> +	/* Enable DLL in RGMII clock paths, deassert DLL reset, and start the
> +	 * delay tune FSM.
> +	 */
> +	lan_rmw(HSIO_DLL_CFG_DLL_CLK_ENA_SET(1) |
> +		HSIO_DLL_CFG_DLL_RST_SET(0) |
> +		HSIO_DLL_CFG_DLL_ENA_SET(p->rx_internal_delay) |
> +		HSIO_DLL_CFG_DELAY_ENA_SET(p->rx_internal_delay),
> +		HSIO_DLL_CFG_DLL_CLK_ENA |
> +		HSIO_DLL_CFG_DLL_RST |
> +		HSIO_DLL_CFG_DLL_ENA |
> +		HSIO_DLL_CFG_DELAY_ENA,
> +		p->lan9645x, HSIO_DLL_CFG(rx_idx));
> +
> +	lan_rmw(HSIO_DLL_CFG_DLL_CLK_ENA_SET(1) |
> +		HSIO_DLL_CFG_DLL_RST_SET(0) |
> +		HSIO_DLL_CFG_DLL_ENA_SET(p->tx_internal_delay) |
> +		HSIO_DLL_CFG_DELAY_ENA_SET(p->tx_internal_delay),
> +		HSIO_DLL_CFG_DLL_CLK_ENA |
> +		HSIO_DLL_CFG_DLL_RST |
> +		HSIO_DLL_CFG_DLL_ENA |
> +		HSIO_DLL_CFG_DELAY_ENA,
> +		p->lan9645x, HSIO_DLL_CFG(tx_idx));
> +}

This function consults only p->rx_internal_delay and p->tx_internal_delay
and never looks at the phy_interface_t. Phylink expects the MAC to apply
internal delay only when the interface is RGMII_ID, RGMII_TXID or
RGMII_RXID, and not when the interface is plain PHY_INTERFACE_MODE_RGMII
(where the PHY provides delay).

With a DT that specifies phy-mode = "rgmii" and nonzero
rx-internal-delay-ps, will both the PHY and the MAC add delay?

And with phy-mode = "rgmii-id" but no {rx,tx}-internal-delay-ps property
in DT, will the MAC fail to add delay on either side?

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> new file mode 100644
> index 000000000000..394a20ee678f
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c

[ ... ]

> +int lan9645x_port_setup(struct dsa_switch *ds, int port)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct lan9645x_port *p;
> +
> +	p = lan9645x_to_port(lan9645x, port);
> +
> +	if (dp->dn) {
> +		u32 val;
> +
> +		if (!of_property_read_u32(dp->dn, "rx-internal-delay-ps", &val))
> +			p->rx_internal_delay = val > 0;
> +
> +		if (!of_property_read_u32(dp->dn, "tx-internal-delay-ps", &val))
> +			p->tx_internal_delay = val > 0;
> +	}

The u32 picosecond value from DT is collapsed to a boolean via "val > 0",
discarding the tap selection. Is the discarded precision intentional, and
if so, why read the value as u32 in the first place rather than using
of_property_read_bool() on a differently-named property?