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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In lan9645x_phylink_mac_link_up()
  (drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c),…
- [Low] lan_rd(), lan_wr() and lan_rmw() in lan9645x_main.h discard
  regmap errors.
- [Low] On RGMII ports, the MAC delay handling ignores phy-mode.
- [Low] Nothing in the tree can bind this driver.

--

net: dsa: lan9645x: add basic dsa driver for LAN9645X

This commit adds a basic DSA driver for the Microchip LAN9645x switch.
It covers switch init, named regmaps obtained from a parent device,
NPI/CPU/front port setup and phylink MAC configuration. Only the internal
CuPHY ports 0-4 and the RGMII ports 7-8 are wired up. The SerDes modes
and the MFD parent are left for later series.

> 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 0000000000000..c7de3836e6967
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> @@ -0,0 +1,456 @@

[ ... ]

> +static int lan9645x_reset_switch(struct lan9645x *lan9645x)
> +{
> +	u32 val = 0;
> +	int err;
> +
> +	/* The parent must reset the whole chip before instantiating its
> +	 * children, so the switch core is disabled when we get here. The
> +	 * sequence below must not run more than once: initializing the
> +	 * memories on a running core corrupts pointers inside the queue
> +	 * system, which eventually fills up and stops passing traffic. If the
> +	 * core is already enabled the chip was not reset, so skip it.
> +	 */
> +	if (SYS_RESET_CFG_CORE_ENA_GET(lan_rd(lan9645x, SYS_RESET_CFG)))
> +		return 0;

[Severity: Low]
What happens here if the regmap_read() under lan_rd() fails? In
lan9645x_main.h, lan_rd() only warns and then returns the value it was
initialised with:

    u32 addr, val = 0;
    ...
    WARN_ON_ONCE(regmap_read(lan_tgt2rmap(lan9645x, t, tinst), addr, &val));

    return val;

A failed read would look like CORE_ENA=0. The CORE_ENA=0 / RAM_INIT
sequence would then run on a core that may already be enabled. The comment
above says that corrupts pointers inside the queue system. The parent is
expected to be SPI based, like ocelot-spi, so bus errors can happen.

The RAM_INIT write can't be checked either. lan_wr() returns void, and the
self-clearing bit already reads back as 0 if the write was lost.

The same thing happens in two other places:

  - lan9645x_set_feat_dis() turns a failed GCB_FEAT_DISABLE read into
    num_port_dis = 0. That skips the SKU port limit check in
    lan9645x_setup().

  - lan9645x_port_set_maxlen() always returns 0. dsa_user_change_mtu()
    therefore raises dev->mtu even when the DEV_MAC_MAXLEN_CFG write
    failed.

Could lan_rd(), lan_wr() and lan_rmw() pass the regmap error back, at least
on these paths?

> +
> +	lan_wr(SYS_RESET_CFG_CORE_ENA_SET(0), lan9645x, SYS_RESET_CFG);
> +	lan_wr(SYS_RAM_INIT_RAM_INIT_SET(1), lan9645x, SYS_RAM_INIT);

[ ... ]

> +static int lan9645x_request_target_regmaps(struct lan9645x *lan9645x)
> +{
> +	const char *resource_name;
> +	struct regmap *tgt_map;
> +
> +	for (int i = 0; i < NUM_TARGETS; i++) {
> +		resource_name = lan9645x_resource_names[i];
> +		if (!resource_name)
> +			continue;
> +
> +		tgt_map = dev_get_regmap(lan9645x->dev->parent, resource_name);
> +		if (IS_ERR_OR_NULL(tgt_map)) {
> +			dev_err(lan9645x->dev, "Failed to get regmap=%s\n",
> +				resource_name);
> +			return -ENODEV;
> +		}

[Severity: Low]
Does anything in the tree provide these regmaps?
lan9645x_request_target_regmaps() needs all 17 named regmaps from
dev->parent ("gcb", "qs", ..., "dev0" to "dev8", "qsys", "ana"). If any is
missing it returns -ENODEV. Nothing under drivers/mfd refers to lan9645x
or lan96455.

The commit message says:

    The MFD driver will come in a later series, because there are
    requirements on the number of child devices before a driver qualifies as
    a MFD device.

Until that lands, the driver cannot probe on any in-tree configuration.
Two things can't be tested in-tree:

  - the regmap naming contract with the parent

  - the assumption in lan9645x_reset_switch() that the parent has already
    reset the chip

The ocelot_ext driver cited as precedent was merged together with its
drivers/mfd/ocelot-spi.c parent.

Could the parent driver be merged in the same series?

[ ... ]

> 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 0000000000000..b57beee8329f9
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
> @@ -0,0 +1,371 @@

[ ... ]

> +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),

[Severity: Low]
Should the MAC delay here depend on the RGMII phy-mode?
lan9645x_rgmii_dll_config() enables the RX/TX DLL delays based only on the
rx-/tx-internal-delay-ps values parsed by lan9645x_parse_mac_delay().
dsa_port_phylink_create() passes the DT phy-mode unchanged to phylink and
the PHY.

The binding (microchip,lan96455s-switch.yaml) allows 2000 ps for all four
RGMII modes, including rgmii-id. Take port 7 or 8 with an external PHY,
phy-mode = "rgmii-id" and rx/tx-internal-delay-ps = <2000>. Both the MAC
and the PHY would add 2ns.

ethernet-controller.yaml says:

    When the MAC implements the delay, it must
    ensure that the PHY does not also implement the same delay. So it
    must modify the phy-mode it passes to the PHY, removing the delay it
    has added. Failure to remove the delay will result in a
    non-functioning link.

It also says:

    A value of 2000ps, i.e 2ns, and a phy-mode
    of 'rgmii' will not be accepted by Reviewers.

Should the driver reject this combination, or remove the MAC-added delay
from the phy-mode it passes to the PHY?

[ ... ]

> +	if (lan9645x_port_is_cuphy(lan9645x, port, interface)) {
> +		lan_rmw(CHIP_TOP_CUPHY_PORT_CFG_GTX_CLK_ENA_SET(gtx_clk),
> +			CHIP_TOP_CUPHY_PORT_CFG_GTX_CLK_ENA, lan9645x,
> +			CHIP_TOP_CUPHY_PORT_CFG(p->chip_port));
> +	}
> +
> +	lan_rmw(SYS_PAUSE_CFG_PAUSE_ENA_SET(1),
> +		SYS_PAUSE_CFG_PAUSE_ENA,
> +		lan9645x, SYS_PAUSE_CFG(p->chip_port));
> +
> +	/* Flow control */
> +	lan_rmw(SYS_MAC_FC_CFG_FC_LINK_SPEED_SET(fc_spd) |
> +		SYS_MAC_FC_CFG_FC_LATENCY_CFG_SET(0x7) |
> +		SYS_MAC_FC_CFG_ZERO_PAUSE_ENA_SET(1) |
> +		SYS_MAC_FC_CFG_PAUSE_VAL_CFG_SET(0xffff) |
> +		SYS_MAC_FC_CFG_RX_FC_ENA_SET(rx_pause ? 1 : 0) |
> +		SYS_MAC_FC_CFG_TX_FC_ENA_SET(tx_pause ? 1 : 0),

[Severity: Medium]
Can this turn on PAUSE frame transmission on the NPI port? On link up,
lan9645x_phylink_mac_link_up() sets PAUSE_ENA to 1 on every port.
TX_FC_ENA follows tx_pause, with no exception for lan9645x->npi.
lan9645x_npi_port_init() doesn't disable pause on the NPI port either.

The NPI port has the long prefix extraction header enabled:

lan9645x_npi_port_init()
    lan_rmw(SYS_PORT_MODE_INCL_XTR_HDR_SET(LAN9645X_TAG_PREFIX_LONG) |
    ...

A PAUSE frame sent on the NPI port would get the prefix and IFH like any
other extracted frame, so the conduit would not recognise it as PAUSE. The
port would run in lossless mode while the link partner never honours the
pause.

The DT example in the lan9645x binding gives the NPI port a fixed-link
with pause. MAC_SYM_PAUSE is advertised, so phylink resolves
tx_pause = rx_pause = true for the documented example.

mscc/ocelot uses the same NPI model and had this problem. It was fixed by
commit 33cb0ff30cff ("net: mscc: ocelot: don't let phylink re-enable TX
PAUSE on the NPI port"). That commit measured CPU termination throughput
at 271 Mbit/s before the fix and 926 Mbit/s after.

felix_npi_port_init() clears SYS_PAUSE_CFG_PAUSE_ENA on the NPI port, and
ocelot_phylink_mac_link_up() does:

    /* Don't attempt to send PAUSE frames on the NPI port, it's broken */
    if (port != ocelot->npi)
        ocelot_fields_write(ocelot, port, SYS_PAUSE_CFG_PAUSE_ENA,
                            tx_pause);

This also seems to still be the case at the end of the series.

Should PAUSE_ENA and TX_FC_ENA stay at 0 for lan9645x->npi? And should
PAUSE_ENA follow tx_pause on the other ports?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-dsa_lan9645x_switch_driver_base-v14-0-3d2a72661213%40microchip.com