Re: [PATCH net-next v4 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[Coia Prant]

Jakub Kicinski <kuba@xxxxxxxxxx> 于2026年9月13日周日 04:13写道：
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
>
> This adds a "rockchip,sgmii-mac-sel" DT property to the RK3568 combphy
> driver so the SGMII interface can be routed to GMAC0 or GMAC1 through the
> pipe GRF pipe_sgmii_mac_sel bit. The value defaults to 1 (GMAC1) when the
> property is absent, and is written during PHY_TYPE_SGMII configuration.
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)
>
> A few questions below.
>
> > diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> > index 7843356a4dd47..919bb97a4b182 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>
> [ ... ]
>
> > @@ -212,6 +213,7 @@ struct rockchip_combphy_priv {
> >       bool enable_ssc;
> >       bool ext_refclk;
> >       struct clk *refclk;
> > +     u32 sgmii_mac_sel;
> >  };
> >
> >  static void rockchip_combphy_updatel(struct rockchip_combphy_priv *priv,
> > @@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
> >
> >       priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
> >
> > +     priv->sgmii_mac_sel = 1;
> > +     device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
> > +
>
> [Severity: Medium]
> Since the return value of device_property_read_u32() is discarded here,
> rockchip_combphy_parse_dt() cannot distinguish "the DT asked for GMAC1"
> from "the DT said nothing about the mux".
>
> The result is that rk3568_combphy_cfg() writes PIPE_GRF 0x0040 bit 1 on
> every PHY_TYPE_SGMII bring-up, including for device trees that do not have
> the new property. Before this patch that bit was never touched, so whatever
> routing the SoC reset or the boot firmware established was left alone.
>
> The commit message states:
>
>   "The hardware reset value is GMAC1 (1). If the property is set to 0,
>   the driver routes SGMII to GMAC0; if set to 1 (or omitted), it
>   remains at GMAC1."
>
> The "or omitted" case now depends entirely on that reset value being 1. If
> the reset value is actually 0, or if firmware routed SGMII to GMAC0 on a
> board wired that way and its DT lacks the property, does phy_init() still
> return success while the XPCS ends up attached to the wrong MAC, giving a
> link that never comes up and no message explaining why?
>
> Would it make sense to only write the bit when the property is actually
> present, for example:
>
>         if (!device_property_read_u32(dev, "rockchip,sgmii-mac-sel",
>                                       &priv->sgmii_mac_sel))
>                 priv->sgmii_mac_sel_valid = true;
>
> so that omitting the property leaves the mux untouched?

SGMII has never been functional on RK3568 in mainline before this
series. There is no existing user that relies on the old behaviour,
and no in-tree device tree enables SGMII. Boards that have the SGMII
routing configured in their bootloader were never able to use it with
a mainline kernel, because the driver did not support it.

Given that, unconditionally writing the MAC-select bit (with a default
of GMAC1, which matches the hardware reset value) is reasonable. It
makes the driver's behaviour explicit and deterministic rather than
depending on whatever the bootloader happened to leave behind.

I'll keep the current logic, but I'll make the commit message clearer
about the hardware reset value and the fact that SGMII is new in
mainline.

> >       priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
> >       /* fallback to old behaviour */
> >       if (PTR_ERR(priv->phy_rst) == -ENOENT)
> > @@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
> >               break;
> >
> >       case PHY_TYPE_SGMII:
> > +             rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
> > +                                     priv->sgmii_mac_sel > 0);
>
> [Severity: Low]
> The binding added in the preceding patch of this series declares the
> property as:
>
>   rockchip,sgmii-mac-sel:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     enum: [0, 1]
>     default: 1
>
> but "priv->sgmii_mac_sel > 0" collapses the value to a boolean, and
> rockchip_combphy_parse_dt() does no range check.
>
> Does that mean a device tree setting the property to 2 or 0xffffffff is
> silently treated as GMAC1 rather than rejected? Similarly, a
> present-but-malformed property (wrong length or type) makes
> device_property_read_u32() fail, and the pre-seeded default of 1 is used
> with no diagnostic.
>
> dtschema only validates in-tree device trees, so for bootloader-supplied
> DTs or overlays the driver is the only place the enum can be enforced.
> Would checking the value against {0, 1} and returning an error, or
> alternatively documenting the property as a plain flag, close that gap?

The property is validated by dtschema. For in-tree device trees, values
other than 0 or 1 are rejected at build time. I don't think the driver
needs to duplicate this check.

> [Severity: Low]
> The mux write is only added to the PHY_TYPE_SGMII case. The adjacent
> PHY_TYPE_QSGMII case in rk3568_combphy_cfg() programs the same pipe-grf
> register (pipe_xpcs_phy_ready at offset 0x0040 bit 2) but never touches
> the MAC-select bit:
>
>         case PHY_TYPE_QSGMII:
>                 rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
>                 ...
>
> The binding gates the property on the compatible only, not on the PHY
> type, and rockchip_combphy_parse_dt() runs at probe time before the PHY
> type is known from rockchip_combphy_xlate().
>
> If a device tree sets rockchip,sgmii-mac-sel on a combphy that a consumer
> later claims with PHY_TYPE_QSGMII, is the requested routing simply dropped
> with no warning? If the property is meant to be SGMII-only, would
> rejecting or warning about it for other PHY types make that explicit?

You're right that the property is SGMII-only. For QSGMII, the hardware
routes to both MACs internally, so there is no MAC-selection bit to
program. We have not found any QSGMII device that exists outside of
OEM SDK EVB code, and mainline XPCS does not support QSGMII, so this
case is not currently reachable. I'll leave it as is.

> >               rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
> >               rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
> >               rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);
> > @@ -984,6 +991,7 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
> >       .con3_for_sata          = { 0x000c, 15, 0, 0x00, 0x4407 },
> >       /* pipe-grf */
> >       .pipe_con0_for_sata     = { 0x0000, 15, 0, 0x00, 0x2220 },
> > +     .pipe_sgmii_mac_sel     = { 0x0040, 1, 1, 0x00, 0x01 },
> >       .pipe_xpcs_phy_ready    = { 0x0040, 2, 2, 0x00, 0x01 },
> >       .u3otg0_port_en         = { 0x0104, 15, 0, 0x0181, 0x1100 },
> >       .u3otg1_port_en         = { 0x0144, 15, 0, 0x0181, 0x1100 },
>
> For reference on the first two comments: this disable/enable encoding
> ({ 0x0040, 1, 1, 0x00, 0x01 }) is what maps the DT value onto GMAC0/GMAC1,
> and rockchip_combphy_param_write() always sets the write-enable mask:
>
>         tmp = en ? reg->enable : reg->disable;
>         mask = GENMASK(reg->bitend, reg->bitstart);
>         val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);
>
> so the bit is committed to hardware on every SGMII bring-up regardless of
> whether the device tree expressed a preference. Could the TRM bit polarity
> and the claimed reset value of 1 be confirmed in the commit message?
>
> Note that no in-tree Rockchip device tree requests PHY_TYPE_SGMII before
> this series, and the only user (rk3568-photonicat.dts) is added later in
> the series, so nothing in-tree changes behaviour today.

You're right, the commit message should state this explicitly. I'll
update it to describe both the bit polarity and the hardware reset
value.

>From the RK3568 TRM Part1 (Page 229), the PIPE_GRF_XPCS_CON0 register
bit 1 is pipe_sgmii_mac_sel:

    0: SGMII interface is routed to GMAC0
    1: SGMII interface is routed to GMAC1

The hardware reset value is 1 (GMAC1). This matches the default I use
when the property is absent.

I'll include this in the commit message and the binding description so
that reviewers can verify the behaviour against the TRM.

Regarding the unconditional write: as I mentioned, SGMII has never been
functional on RK3568 in mainline before this series, and no in-tree
device tree enables it. So writing the bit on every SGMII bring-up does
not change behaviour for any existing user.

Thanks,
Coia