* [PATCH net-next v3] net: dsa: mxl-gsw1xx: force internal PHYs into a known reset state
@ 2026-09-15 9:18 A. Sverdlin
2026-09-15 11:17 ` Daniel Golle
2026-09-16 10:05 ` netdev-bot+sashiko
0 siblings, 2 replies; 4+ messages in thread
From: A. Sverdlin @ 2026-09-15 9:18 UTC (permalink / raw)
To: netdev
Cc: Alexander Sverdlin, Daniel Golle, Hauke Mehrtens, Andrew Lunn,
Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
When the switch is bootstrapped with PS_NOWAIT = 0, the internal PHYs
default to a held-in-reset state. The driver previously only worked with
PS_NOWAIT = 1 where PHYs are released from reset automatically.
Program the PHY reset lines via the RST_REQ register before the MDIO bus
is registered, so that PHY IDs can be read during bus scan. Release only
the PHYs of ports that have a phy-handle in the device tree and keep the
unused ones held in reset. Driving both directions makes the resulting
state identical regardless of the PS_NOWAIT bootstrap: unused PHYs no
longer stay powered when PS_NOWAIT = 1 released them automatically.
A single 300ms delay after deasserting the needed resets ensures the PHYs
are ready for MDIO communication (refer to Intel XWAY, Maxlinear GPY111
datasheets or the Lantiq counterpart of the driver). The delay is skipped
when no PHY is released from reset.
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v3:
- put unused PHYs into reset state (changed patch subject accordingly)
v2:
- Reverse Christmas tree.
- https://lore.kernel.org/all/20260908103030.4032992-1-alexander.sverdlin@siemens.com/
v1:
- https://lore.kernel.org/all/20260901165049.672446-1-alexander.sverdlin@siemens.com/
drivers/net/dsa/lantiq/lantiq_gswip.h | 2 +
drivers/net/dsa/lantiq/lantiq_gswip_common.c | 10 ++++
drivers/net/dsa/lantiq/mxl-gsw1xx.c | 51 ++++++++++++++++++++
drivers/net/dsa/lantiq/mxl-gsw1xx.h | 1 +
4 files changed, 64 insertions(+)
diff --git a/drivers/net/dsa/lantiq/lantiq_gswip.h b/drivers/net/dsa/lantiq/lantiq_gswip.h
index 0b75be14dc109..4f95877c59d1d 100644
--- a/drivers/net/dsa/lantiq/lantiq_gswip.h
+++ b/drivers/net/dsa/lantiq/lantiq_gswip.h
@@ -261,6 +261,8 @@ struct gswip_hw_info {
const struct gswip_pce_microcode (*pce_microcode)[];
size_t pce_microcode_size;
enum dsa_tag_protocol tag_protocol;
+
+ int (*setup)(struct dsa_switch *ds);
void (*phylink_get_caps)(struct dsa_switch *ds, int port,
struct phylink_config *config);
struct phylink_pcs *(*mac_select_pcs)(struct phylink_config *config,
diff --git a/drivers/net/dsa/lantiq/lantiq_gswip_common.c b/drivers/net/dsa/lantiq/lantiq_gswip_common.c
index 6150dc3ffb641..7c53ded178367 100644
--- a/drivers/net/dsa/lantiq/lantiq_gswip_common.c
+++ b/drivers/net/dsa/lantiq/lantiq_gswip_common.c
@@ -689,6 +689,16 @@ static int gswip_setup(struct dsa_switch *ds)
*/
regmap_write(priv->mdio, GSWIP_MDIO_MDC_CFG0, 0x0);
+ /* GSW1xx will wake up the PHYs here, so it makes sense that it happens
+ * after the auto-polling deactivation above, but before the MDIO bus
+ * registration below
+ */
+ if (priv->hw_info->setup) {
+ err = priv->hw_info->setup(ds);
+ if (err)
+ return err;
+ }
+
/* Configure the MDIO Clock 2.5 MHz */
regmap_write_bits(priv->mdio, GSWIP_MDIO_MDC_CFG1, 0xff, 0x09);
diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.c b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
index 66bf7e3319057..08c633b2caebf 100644
--- a/drivers/net/dsa/lantiq/mxl-gsw1xx.c
+++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
@@ -13,6 +13,7 @@
#include <linux/delay.h>
#include <linux/jiffies.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_mdio.h>
#include <linux/phy/phy-common-props.h>
@@ -589,6 +590,52 @@ static void gsw150_phylink_get_caps(struct dsa_switch *ds, int port,
gsw1xx_phylink_get_lpi_caps(config);
}
+static int gsw1xx_setup(struct dsa_switch *ds)
+{
+ struct gsw1xx_priv *priv = container_of(ds->priv, struct gsw1xx_priv, gswip);
+ struct gswip_priv *gswip_priv = ds->priv;
+ u32 phy_mask = 0, active_mask = 0;
+ struct device_node *phy_np;
+ struct dsa_port *dp;
+ int ret;
+
+ dsa_switch_for_each_user_port(dp, ds) {
+ struct phylink_config cfg = {};
+
+ /* Is there an internal PHY on this port? */
+ gswip_priv->hw_info->phylink_get_caps(ds, dp->index, &cfg);
+ if (!test_bit(PHY_INTERFACE_MODE_INTERNAL, cfg.supported_interfaces))
+ continue;
+
+ phy_mask |= GSW1XX_RST_REQ_PHY(dp->index);
+
+ /* Will the PHY be really used? */
+ phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
+ if (!phy_np)
+ continue;
+
+ of_node_put(phy_np);
+ active_mask |= GSW1XX_RST_REQ_PHY(dp->index);
+ }
+
+ if (!phy_mask)
+ return 0;
+
+ /* Deassert resets for PHYs referenced by active ports and hold the
+ * unused ones in reset, so the state is identical regardless of the
+ * PS_NOWAIT bootstrap.
+ */
+ ret = regmap_update_bits(priv->shell, GSW1XX_SHELL_RST_REQ, phy_mask,
+ phy_mask & ~active_mask);
+ if (ret)
+ return ret;
+
+ if (active_mask)
+ msleep(300);
+
+ return 0;
+}
+
static struct phylink_pcs *gsw1xx_phylink_mac_select_pcs(struct phylink_config *config,
phy_interface_t interface)
{
@@ -830,6 +877,7 @@ static const struct gswip_hw_info gsw12x_data = {
[GSW1XX_MII_PORT] = GSWIP_MII_PCDU0,
[GSW1XX_MII_PORT + 1 ... GSWIP_MAX_PORTS - 1] = -1,
},
+ .setup = gsw1xx_setup,
.mac_select_pcs = gsw1xx_phylink_mac_select_pcs,
.phylink_get_caps = &gsw1xx_phylink_get_caps,
.supports_2500m = true,
@@ -852,6 +900,7 @@ static const struct gswip_hw_info gsw140_data = {
[GSW1XX_MII_PORT] = GSWIP_MII_PCDU0,
[GSW1XX_MII_PORT + 1 ... GSWIP_MAX_PORTS - 1] = -1,
},
+ .setup = gsw1xx_setup,
.mac_select_pcs = gsw1xx_phylink_mac_select_pcs,
.phylink_get_caps = &gsw1xx_phylink_get_caps,
.supports_2500m = true,
@@ -874,6 +923,7 @@ static const struct gswip_hw_info gsw141_data = {
[GSW1XX_MII_PORT] = GSWIP_MII_PCDU0,
[GSW1XX_MII_PORT + 1 ... GSWIP_MAX_PORTS - 1] = -1,
},
+ .setup = gsw1xx_setup,
.mac_select_pcs = gsw1xx_phylink_mac_select_pcs,
.phylink_get_caps = gsw1xx_phylink_get_caps,
.port_setup = gsw1xx_port_setup,
@@ -895,6 +945,7 @@ static const struct gswip_hw_info gsw150_data = {
[5] = 1,
[6] = 11,
},
+ .setup = gsw1xx_setup,
.phylink_get_caps = gsw150_phylink_get_caps,
/* There is only a single RGMII_SLEW_CFG register in GSW150 and it is
* unknown if RGMII slew configuration affects both RGMII ports
diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.h b/drivers/net/dsa/lantiq/mxl-gsw1xx.h
index caa8f1008587a..a7976a5988bd8 100644
--- a/drivers/net/dsa/lantiq/mxl-gsw1xx.h
+++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.h
@@ -110,6 +110,7 @@
#define GSW1XX_SHELL_BASE 0xfa00
#define GSW1XX_SHELL_RST_REQ 0x01
#define GSW1XX_RST_REQ_SGMII_SHELL BIT(5)
+#define GSW1XX_RST_REQ_PHY(p) BIT(p)
#define GSW1XX_SHELL_MANU_ID 0x10
#define GSW1XX_SHELL_MANU_ID_PNUML GENMASK(15, 12)
#define GSW1XX_SHELL_MANU_ID_MANID GENMASK(11, 1)
--
2.55.0
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next v3] net: dsa: mxl-gsw1xx: force internal PHYs into a known reset state
2026-09-15 9:18 [PATCH net-next v3] net: dsa: mxl-gsw1xx: force internal PHYs into a known reset state A. Sverdlin
@ 2026-09-15 11:17 ` Daniel Golle
2026-09-15 11:35 ` Sverdlin, Alexander
2026-09-16 10:05 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: Daniel Golle @ 2026-09-15 11:17 UTC (permalink / raw)
To: A. Sverdlin
Cc: netdev, Hauke Mehrtens, Andrew Lunn, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Russell King, linux-kernel
On Tue, Sep 15, 2026 at 11:18:45AM +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>
> When the switch is bootstrapped with PS_NOWAIT = 0, the internal PHYs
> default to a held-in-reset state. The driver previously only worked with
> PS_NOWAIT = 1 where PHYs are released from reset automatically.
>
> Program the PHY reset lines via the RST_REQ register before the MDIO bus
> is registered, so that PHY IDs can be read during bus scan. Release only
> the PHYs of ports that have a phy-handle in the device tree and keep the
> unused ones held in reset. Driving both directions makes the resulting
> state identical regardless of the PS_NOWAIT bootstrap: unused PHYs no
> longer stay powered when PS_NOWAIT = 1 released them automatically.
>
> A single 300ms delay after deasserting the needed resets ensures the PHYs
> are ready for MDIO communication (refer to Intel XWAY, Maxlinear GPY111
> datasheets or the Lantiq counterpart of the driver). The delay is skipped
> when no PHY is released from reset.
> [...]
> diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.c b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> index 66bf7e3319057..08c633b2caebf 100644
> --- a/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> +++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> [...]
> @@ -589,6 +590,52 @@ static void gsw150_phylink_get_caps(struct dsa_switch *ds, int port,
> gsw1xx_phylink_get_lpi_caps(config);
> }
>
> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> + struct gsw1xx_priv *priv = container_of(ds->priv, struct gsw1xx_priv, gswip);
> + struct gswip_priv *gswip_priv = ds->priv;
> + u32 phy_mask = 0, active_mask = 0;
> + struct device_node *phy_np;
> + struct dsa_port *dp;
> + int ret;
> +
> + dsa_switch_for_each_user_port(dp, ds) {
This loop runs only over *user ports*. It skips DSA_PORT_TYPE_UNUSED.
> + struct phylink_config cfg = {};
> +
> + /* Is there an internal PHY on this port? */
> + gswip_priv->hw_info->phylink_get_caps(ds, dp->index, &cfg);
> + if (!test_bit(PHY_INTERFACE_MODE_INTERNAL, cfg.supported_interfaces))
> + continue;
> +
> + phy_mask |= GSW1XX_RST_REQ_PHY(dp->index);
> +
> + /* Will the PHY be really used? */
> + phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> + if (!phy_np)
> + continue;
Hence this check is redundant and doesn't make much sense.
> +
> + of_node_put(phy_np);
> + active_mask |= GSW1XX_RST_REQ_PHY(dp->index);
> + }
> +
> + if (!phy_mask)
> + return 0;
So this means if only the MII interfaces are in use the PHYs are
not being put into reset. Why?
> +
> + /* Deassert resets for PHYs referenced by active ports and hold the
> + * unused ones in reset, so the state is identical regardless of the
> + * PS_NOWAIT bootstrap.
> + */
> + ret = regmap_update_bits(priv->shell, GSW1XX_SHELL_RST_REQ, phy_mask,
> + phy_mask & ~active_mask);
> + if (ret)
> + return ret;
> +
> + if (active_mask)
> + msleep(300);
> +
> + return 0;
> +}
> +
> static struct phylink_pcs *gsw1xx_phylink_mac_select_pcs(struct phylink_config *config,
> phy_interface_t interface)
> {
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next v3] net: dsa: mxl-gsw1xx: force internal PHYs into a known reset state
2026-09-15 11:17 ` Daniel Golle
@ 2026-09-15 11:35 ` Sverdlin, Alexander
0 siblings, 0 replies; 4+ messages in thread
From: Sverdlin, Alexander @ 2026-09-15 11:35 UTC (permalink / raw)
To: daniel
Cc: hauke, olteanv, davem, andrew, linux, linux-kernel, pabeni,
netdev, edumazet, kuba
Thanks for the timely review, Daniel!
On Tue, 2026-09-15 at 12:17 +0100, Daniel Golle wrote:
> > When the switch is bootstrapped with PS_NOWAIT = 0, the internal PHYs
> > default to a held-in-reset state. The driver previously only worked with
> > PS_NOWAIT = 1 where PHYs are released from reset automatically.
> >
> > Program the PHY reset lines via the RST_REQ register before the MDIO bus
> > is registered, so that PHY IDs can be read during bus scan. Release only
> > the PHYs of ports that have a phy-handle in the device tree and keep the
> > unused ones held in reset. Driving both directions makes the resulting
> > state identical regardless of the PS_NOWAIT bootstrap: unused PHYs no
> > longer stay powered when PS_NOWAIT = 1 released them automatically.
> >
> > A single 300ms delay after deasserting the needed resets ensures the PHYs
> > are ready for MDIO communication (refer to Intel XWAY, Maxlinear GPY111
> > datasheets or the Lantiq counterpart of the driver). The delay is skipped
> > when no PHY is released from reset.
> > [...]
> > diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.c b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> > index 66bf7e3319057..08c633b2caebf 100644
> > --- a/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> > +++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> > [...]
> > @@ -589,6 +590,52 @@ static void gsw150_phylink_get_caps(struct dsa_switch *ds, int port,
> > gsw1xx_phylink_get_lpi_caps(config);
> > }
> >
> > +static int gsw1xx_setup(struct dsa_switch *ds)
> > +{
> > + struct gsw1xx_priv *priv = container_of(ds->priv, struct gsw1xx_priv, gswip);
> > + struct gswip_priv *gswip_priv = ds->priv;
> > + u32 phy_mask = 0, active_mask = 0;
> > + struct device_node *phy_np;
> > + struct dsa_port *dp;
> > + int ret;
> > +
> > + dsa_switch_for_each_user_port(dp, ds) {
>
> This loop runs only over *user ports*. It skips DSA_PORT_TYPE_UNUSED.
>
> > + struct phylink_config cfg = {};
> > +
> > + /* Is there an internal PHY on this port? */
> > + gswip_priv->hw_info->phylink_get_caps(ds, dp->index, &cfg);
> > + if (!test_bit(PHY_INTERFACE_MODE_INTERNAL, cfg.supported_interfaces))
> > + continue;
> > +
> > + phy_mask |= GSW1XX_RST_REQ_PHY(dp->index);
> > +
> > + /* Will the PHY be really used? */
> > + phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> > + if (!phy_np)
> > + continue;
>
> Hence this check is redundant and doesn't make much sense.
>
> > +
> > + of_node_put(phy_np);
> > + active_mask |= GSW1XX_RST_REQ_PHY(dp->index);
> > + }
> > +
> > + if (!phy_mask)
> > + return 0;
>
> So this means if only the MII interfaces are in use the PHYs are
> not being put into reset. Why?
Agree on both points! Will fix in v4.
--
Alexander Sverdlin
Siemens AG
www.siemens.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next v3] net: dsa: mxl-gsw1xx: force internal PHYs into a known reset state
2026-09-15 9:18 [PATCH net-next v3] net: dsa: mxl-gsw1xx: force internal PHYs into a known reset state A. Sverdlin
2026-09-15 11:17 ` Daniel Golle
@ 2026-09-16 10:05 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-16 10:05 UTC (permalink / raw)
To: alexander.sverdlin
Cc: netdev, daniel, hauke, andrew, olteanv, davem, edumazet, kuba,
pabeni, linux, linux-kernel
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 1 · Low: 3
- [High] gsw1xx_setup() (drivers/net/dsa/lantiq/mxl-gsw1xx.c:593-637)
builds the RST_REQ PHY mask exclusively inside…
- [Medium] gsw1xx_setup() decides that an internal PHY is "really used"
solely by of_parse_phandle(dp->dn, "phy-handle", 0)…
- [Low] The commit message states "The delay is skipped when no PHY is
released from reset", but the implemented condition is `if…
- [Low] The submission carries no `Fixes:` trailer.
- [Low] gsw1xx_setup() takes ownership of the internal PHY reset/power
lines (drivers/net/dsa/lantiq/mxl-gsw1xx.c:628-634) but nothing ever…
--
net: dsa: mxl-gsw1xx: force internal PHYs into a known reset state
This adds a per-variant .setup hook to struct gswip_hw_info, called from
gswip_setup() after MDIO auto-polling is disabled and before the MDIO bus
is registered. The GSW1xx implementation programs the internal PHY reset
lines in GSW1XX_SHELL_RST_REQ, releasing the PHYs of ports with a
phy-handle and holding the rest in reset, followed by a 300ms delay.
[Severity: Low]
There is no Fixes: tag on this submission. The behaviour being enabled
(internal PHYs discoverable when the switch is strapped PS_NOWAIT = 0) has
never worked since the driver was added in commit 22335939ec90 ("net: dsa:
add driver for MaxLinear GSW1xx switch family"), which registered the MDIO
bus without ever programming the GSW1XX_SHELL_RST_REQ PHY bits; the
pre-patch driver only touched GSW1XX_RST_REQ_SGMII_SHELL.
Would a Fixes: tag pointing at 22335939ec90 be appropriate here, or is the
change intended as net-next material only? The patch also changes
behaviour of already working configurations (unused PHYs are now asserted
into reset, and a 300ms delay is added), which argues against a stable
backport, so it would help to state the intent explicitly.
> diff --git a/drivers/net/dsa/lantiq/lantiq_gswip_common.c b/drivers/net/dsa/lantiq/lantiq_gswip_common.c
> index 6150dc3ffb641..7c53ded178367 100644
> --- a/drivers/net/dsa/lantiq/lantiq_gswip_common.c
> +++ b/drivers/net/dsa/lantiq/lantiq_gswip_common.c
> @@ -689,6 +689,16 @@ static int gswip_setup(struct dsa_switch *ds)
> */
> regmap_write(priv->mdio, GSWIP_MDIO_MDC_CFG0, 0x0);
>
> + /* GSW1xx will wake up the PHYs here, so it makes sense that it happens
> + * after the auto-polling deactivation above, but before the MDIO bus
> + * registration below
> + */
> + if (priv->hw_info->setup) {
> + err = priv->hw_info->setup(ds);
> + if (err)
> + return err;
> + }
> +
[Severity: Low]
Is anything expected to put the released PHYs back into reset?
Once gsw1xx_setup() has deasserted the PHY reset bits, no other code path
writes GSW1XX_SHELL_RST_REQ again. gswip_teardown() only clears the
global enable:
drivers/net/dsa/lantiq/lantiq_gswip_common.c:gswip_teardown() {
struct gswip_priv *priv = ds->priv;
regmap_clear_bits(priv->mdio, GSWIP_MDIO_GLOB, GSWIP_MDIO_GLOB_ENABLE);
}
There is no per-variant .teardown counterpart to the new .setup hook, and
gsw1xx_remove()/gsw1xx_shutdown() only unregister the switch and cancel the
delayed work, so after unbind or shutdown the internal PHYs stay powered
and out of reset.
The same applies to the failure paths that follow this hook inside
gswip_setup(), for example:
err = gswip_mdio(priv);
if (err) {
dev_err(priv->dev, "mdio bus setup failed\n");
return err;
}
and the later gswip_switch_r_timeout() "MAC flushing didn't finish" return.
Both leave the PHYs released, which is the indeterminate powered state the
commit message says it wants to eliminate. For comparison, the SGMII shell
reset is asserted and deasserted symmetrically in gsw1xx_pcs_disable() and
gsw1xx_serdes_pcs_init().
> /* Configure the MDIO Clock 2.5 MHz */
> regmap_write_bits(priv->mdio, GSWIP_MDIO_MDC_CFG1, 0xff, 0x09);
>
> diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.c b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> index 66bf7e3319057..08c633b2caebf 100644
> --- a/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> +++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> @@ -589,6 +590,52 @@ static void gsw150_phylink_get_caps(struct dsa_switch *ds, int port,
> gsw1xx_phylink_get_lpi_caps(config);
> }
>
> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> + struct gsw1xx_priv *priv = container_of(ds->priv, struct gsw1xx_priv, gswip);
> + struct gswip_priv *gswip_priv = ds->priv;
> + u32 phy_mask = 0, active_mask = 0;
> + struct device_node *phy_np;
> + struct dsa_port *dp;
> + int ret;
> +
> + dsa_switch_for_each_user_port(dp, ds) {
> + struct phylink_config cfg = {};
> +
> + /* Is there an internal PHY on this port? */
> + gswip_priv->hw_info->phylink_get_caps(ds, dp->index, &cfg);
> + if (!test_bit(PHY_INTERFACE_MODE_INTERNAL, cfg.supported_interfaces))
> + continue;
> +
> + phy_mask |= GSW1XX_RST_REQ_PHY(dp->index);
[Severity: High]
Can phy_mask miss internal PHYs on ports that are not DSA user ports?
dsa_switch_for_each_user_port() filters on dsa_port_is_user():
include/net/dsa.h:
#define dsa_switch_for_each_user_port(_dp, _ds) \
dsa_switch_for_each_port((_dp), (_ds)) \
if (dsa_port_is_user((_dp)))
But a dsa_port exists for every index up to ds->num_ports, whether or not
the device tree describes it:
net/dsa/dsa.c:dsa_switch_touch_ports() {
for (port = 0; port < ds->num_ports; port++) {
dp = dsa_port_touch(ds, port);
...
}
Ports absent from (or disabled in) the DT stay DSA_PORT_TYPE_UNUSED and are
skipped by the loop above, so their reset bits never enter phy_mask. Since
regmap_update_bits() only modifies bits inside the mask, those PHYs retain
their bootstrap value: released and powered with PS_NOWAIT = 1, held in
reset with PS_NOWAIT = 0.
gsw12x/gsw140/gsw141 declare built-in PHYs on ports 0..3 in
gsw1xx_phylink_get_caps(), and gsw150 on ports 0..4 in
gsw150_phylink_get_caps(). A board describing only ports 0 and 1, which is
the style of the in-tree GSW125 example in
Documentation/devicetree/bindings/net/dsa/lantiq,gswip.yaml, would leave
PHYs 2 and 3 outside the mask.
Should the mask be derived from the variant's set of internal PHYs rather
than from DSA user-port classification?
> +
> + /* Will the PHY be really used? */
> + phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> + if (!phy_np)
> + continue;
[Severity: Medium]
Is "phy-handle" the only property that should count as a PHY reference
here?
The PHY core accepts two legacy names as well:
drivers/net/phy/phy_device.c:fwnode_get_phy_node() {
phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
...
phy_node = fwnode_find_reference(fwnode, "phy", 0);
...
return fwnode_find_reference(fwnode, "phy-device", 0);
}
and phylink_fwnode_phy_connect() uses that helper. A port described with
phy = <&switchphyX> or phy-device = <&switchphyX> is therefore a valid PHY
reference that this loop classifies as unused, so its reset bit gets
asserted below.
Because the new hook runs before gswip_mdio(), the PHY is in reset when
of_mdiobus_register() reads its ID registers, no phy_device is created, and
phylink_fwnode_phy_connect() later returns -ENODEV:
net/dsa/user.c:dsa_user_phy_setup() {
ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);
if (ret == -ENODEV && ds->user_mii_bus) {
...
}
The ds->user_mii_bus fallback does not apply, since gswip_mdio() registers
a standalone mii_bus and never sets ds->user_mii_bus, so switch
registration would fail on hardware that worked before with PS_NOWAIT = 1.
A related case is fixed-link: phylink_fwnode_phy_connect() returns 0 early
when !phylink_expects_phy(pl), and such a port legitimately has no
phy-handle while its internal PHY still needs power. Would that port end
up with its PHY held in reset while phylink reports the link up?
> +
> + of_node_put(phy_np);
> + active_mask |= GSW1XX_RST_REQ_PHY(dp->index);
> + }
> +
> + if (!phy_mask)
> + return 0;
> +
> + /* Deassert resets for PHYs referenced by active ports and hold the
> + * unused ones in reset, so the state is identical regardless of the
> + * PS_NOWAIT bootstrap.
> + */
> + ret = regmap_update_bits(priv->shell, GSW1XX_SHELL_RST_REQ, phy_mask,
> + phy_mask & ~active_mask);
> + if (ret)
> + return ret;
> +
> + if (active_mask)
> + msleep(300);
[Severity: Low]
The commit message says:
"The delay is skipped when no PHY is released from reset."
The implemented condition is "at least one internal-PHY port has a
phy-handle", not "a reset was actually deasserted". On boards
bootstrapped with PS_NOWAIT = 1 the PHY bits are already zero, so
regmap_update_bits() changes nothing, yet the 300ms sleep is still taken on
every probe.
If the intent is to sleep only when a reset was really deasserted, would
regmap_update_bits_check() and a test on the change flag match the
description better? Otherwise, could the commit message be reworded to say
the delay is taken whenever an internal PHY is in use?
> +
> + return 0;
> +}
> +
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915091856.1375914-1-alexander.sverdlin%40siemens.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-09-16 10:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-15 9:18 [PATCH net-next v3] net: dsa: mxl-gsw1xx: force internal PHYs into a known reset state A. Sverdlin
2026-09-15 11:17 ` Daniel Golle
2026-09-15 11:35 ` Sverdlin, Alexander
2026-09-16 10:05 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®