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

[Rob Herring]

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