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

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Tue, 2026-05-05 at 18:45 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?
> 

I mention this in the cover letter. The plan is to send the parent MFD in
another series. This switch driver will be one of the child devices, and
from what I can tell there are requirements to how many child devices an
MFD driver should have before qualifying as such. So there is a bit of
and chicken and egg problem. The first version of the MFD driver will
have 3 children

* pinctrl
* MDIO controller
* dsa switch

and uses the same approach as drivers/mfd/ocelot-spi.c. I can not depend
on the Kconfig symbol here, because the symbol does not exist at this point.
I have not include the MFD patches here because it targets another tree,
and this was already quite large.

I am happy to do this is in some other way if the maintainers think that
would be better?

At any rate, I can add some of this information to the commit message,
so it is not just in the cover letter.

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

See my answer above. I thought it was best to send them seperately, but I
can include the MFD patches here if that is preferred.

> [ ... ]
> 
> > +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?
> 

Ah yes, there was supposed to be some validation during setup which makes
sure the number of ports enabled in the DT does not exceed the number of
supported ports by the SKU. I will add that, and remove the dd_dis/tsn_dis,
which are not used at this point.

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

I will remove MAC_2500FD and update the commit message to be clear about
current limitations vs. SerDes ports. With empty supported interfaces
DSA core will flip the ports to UNUSED if they are defined in the DT.

> [ ... ]
> 
> > +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?

Yes this is a 2:1 mux. I thought the interface modes were fixed from the DT,
except perhaps for SFPs, but I will update this to properly handle the
muxing in a dynamic fashion.

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

The bots like to come back to this. The function is only called when
phy_interface_mode_is_rgmii.

With respect to the mechanics of it all, I believe this is how you support
MAC-2-MAC configurations where one side needs to add the delays.
The PHY_INTERFACE_MODE_* is not supposed to be used for this, as I 
understand it.

I originally did it that way, and was instructed to change it to this.

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

I will add some explicit validation. The hardware only supports 2ns on/off. I will
add a comment about that, and reject unsupported values.