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

[Coia Prant]

Rob Herring <robh@xxxxxxxxxx> 于2026年10月7日周三 00:14写道：
>
> On Tue, Oct 06, 2026 at 10:53:22PM +0800, Shawn Lin wrote:
> > Hi Coia
> >
> > 在 2026/10/06 星期二 15:47, 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 (or omitted), it
> > > remains at GMAC1.
> > >
> >
> > Thanks for your patch. I have a question below, please have a look.
> >
> > > This is necessary for boards such as the Ariaboard Photonicat, which
> > > uses the SGMII interface connected to GMAC0.
> > >
> > > Out-of-range values are rejected by dtschema, so the driver does not
> > > duplicate the range check.
> > >
> > > 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>
> > > ---
> > >   drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> > > index 7843356a4dd47..7b867e7520064 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,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;
> >
> > Before this patch, SGMMI was routed to GMACX depends on bootloader(if
> > setting), or routed to GMAC1 by chip default. Without knowing any
> > potential candidates setting it via bootloader, you might not force any
> > DTS files without "rockchip,sgmii-mac-sel" to pick GMAC1 as fallback.
> >
> > Maybe you could just keep the SGMII route as-it if the property is not
> > present. Meanwhile, should validate rockchip,sgmii-mac-sel is a valid
> > value passing from DT.
>
> That is the typical way tri-state properties work (as opposed to using a
> boolean). In that case, no 'default' in the schema would then be
> appropriate.
>
> Rob

Hi Shawn, Rob,

Thanks for the review and feedback. I've just posted v13, which addresses
both points:

- The driver no longer forces GMAC1 when the property is absent; it
  leaves the routing as-is.
- The driver validates the value and rejects anything other than 0 or 1.
- The binding drops 'default', since the property is tri-state.

I'll avoid replying to each comment individually to keep the thread
clean, but please let me know if anything is still off.

Thanks,
Coia