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

[Jakub Kicinski <kuba@kernel.org>]

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?

>  	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?

[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?

>  		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.