Re: [PATCH v13 2/2] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[Coia Prant]

Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> 于2026年10月7日周三 08:53写道：
>
> 在 2026/10/07 星期三 0:28, Coia Prant 写道:
> > On RK3568, the SGMII interface can be routed to either GMAC0 or
> > GMAC1 via the GRF register pipe_sgmii_mac_sel.
> >
> > Add support for this selection by introducing the
> > "rockchip,sgmii-mac-sel" DT property.
> >
> >  From the RK3568 TRM (Part1, Page 229), the PIPE_GRF_XPCS_CON0
> > bit 1 (pipe_sgmii_mac_sel) is defined as:
> >
> >      0: SGMII routed to GMAC0
> >      1: SGMII routed to GMAC1
> >
> > The hardware reset value is 1 (GMAC1). If the property is set to 0,
> > the driver routes SGMII to GMAC0; if set to 1, it routes to GMAC1.
> > If the property is absent, the driver leaves the routing unchanged,
> > so the effective value is whatever the bootloader or hardware reset
> > left.
> >
> > The driver validates the value and rejects anything other than 0 or 1.
> >
> > This is necessary for boards such as the Ariaboard Photonicat, which
> > uses the SGMII interface connected to GMAC0.
> >
> > Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)
> > Signed-off-by: Coia Prant <coiaprant@xxxxxxxxx>
> > ---
> >   .../rockchip/phy-rockchip-naneng-combphy.c    | 42 +++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> > index 7843356a4dd47..f76b48009673d 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> > @@ -186,6 +186,7 @@ struct rockchip_combphy_grfcfg {
> >       struct combphy_reg pipe_xpcs_phy_ready;
> >       struct combphy_reg pipe_pcie1l0_sel;
> >       struct combphy_reg pipe_pcie1l1_sel;
> > +     struct combphy_reg pipe_sgmii_mac_sel;
> >       struct combphy_reg u3otg0_port_en;
> >       struct combphy_reg u3otg1_port_en;
> >   };
> > @@ -212,6 +213,8 @@ struct rockchip_combphy_priv {
> >       bool enable_ssc;
> >       bool ext_refclk;
> >       struct clk *refclk;
> > +     bool sgmii_mac_sel_present;
> > +     u32 sgmii_mac_sel;
> >   };
> >
> >   static void rockchip_combphy_updatel(struct rockchip_combphy_priv *priv,
> > @@ -341,6 +344,7 @@ static struct phy *rockchip_combphy_xlate(struct device *dev, const struct of_ph
> >   static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy_priv *priv)
> >   {
> >       int i;
> > +     int ret;
> >
> >       priv->num_clks = devm_clk_bulk_get_all(dev, &priv->clks);
> >       if (priv->num_clks < 1)
> > @@ -375,6 +379,16 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
> >
> >       priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
> >
> > +     if (device_property_present(dev, "rockchip,sgmii-mac-sel")) {
> > +             ret = device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
> > +             if (ret) {
> > +                     dev_err(dev, "failed to read sgmii-mac-sel property\n");
> > +                     return ret;
> > +             }
> > +
>
> device_property_read_u32() should return non-zero value if it's absent.
> How about this:
>
>         struct rockchip_combphy_priv {
>                 int sgmii_mac_sel;
>         }
>
> static int rockchip_combphy_parse_dt:
>         u32 val;
>
>          priv->sgmii_mac_sel = -1;
>          if (!device_property_read_u32(dev, "rockchip,sgmii-mac-sel",
> &val)) {
>                  if (val > 1)
>                          return dev_err_probe(dev, -EINVAL,
>                                               "invalid sgmii-mac-sel
> %u\n", val);
>                  priv->sgmii_mac_sel = val;
>          }
>
>
> static int rk3568_combphy_cfg:
>          case PHY_TYPE_SGMII:
>                  if (priv->sgmii_mac_sel >= 0)
>                          rockchip_combphy_param_write(priv->pipe_grf,
>
> &cfg->pipe_sgmii_mac_sel,
>
> priv->sgmii_mac_sel > 0);

Hi,

Thanks for the suggestion. I'd prefer to keep the current form, because
it keeps "missing" and "malformed" as two distinct cases.

Current logic:

if (device_property_present(dev, "rockchip,sgmii-mac-sel")) {
    ret = device_property_read_u32(dev, "rockchip,sgmii-mac-sel",
                &priv->sgmii_mac_sel);
    if (ret)
        return ret;
    priv->sgmii_mac_sel_present = true;
}

- device_property_present() checks whether the property exists.
- If it exists, device_property_read_u32() reads it. A non-integer value
  (e.g. a string) returns an error, which is propagated -- it is not
  silently treated as "absent".
- The range check is done later, in rk3568_combphy_cfg's SGMII case, so
  a bad value only fails when SGMII is actually used. That keeps non-SGMII
  users (PCIe, SATA) working, and leaves room for future SoCs that may
  support a different set of MACs.

With int + -1 as the "absent" sentinel, a DT value like 0xFFFFFFFF would
wrap to -1 and be mistaken for "absent", bypassing the range check. The
u32 + present flag avoids that ambiguity.

So I'd like to keep v13 as-is. Let me know if you still feel strongly
about it.

Thanks,
Coia