Re: [PATCH v3 phy 13/17] phy: lynx-28g: probe on per-SoC and per-instance compatible strings

[Josua Mayer <josua@solid-run.com>]

Am 02.10.25 um 12:40 schrieb Josua Mayer:
> Am 26.09.25 um 20:05 schrieb Vladimir Oltean:
>
>> Add driver support for probing on the new, per-instance and per-SoC
>> bindings, which provide the following benefits:
>> - they allow rejecting unsupported protocols per lane (10GbE on SerDes 2
>>   lanes 0-5)
>> - individual lanes have their own OF nodes as PHY providers, which
>>   allows board device tree authors to tune electrical parameters such as
>>   TX amplitude, equalization etc.
>>
>> Probing on "fsl,lynx-28g" is still supported, but the feature set is
>> frozen in time to just 1GbE and 10GbE (essentially the feature set as of
>> this change). However, we encourage the user at probe time to update the
>> device tree.
>>
>> Refactor the per-lane logic from lynx_28g_probe() into
>> lynx_28g_lane_probe(), and call it from two distinct paths depending on
>> whether the modern or the legacy compatible string is used, with an OF
>> node for the lane or without.
>>
>> Notable implication of the above: when lanes have disabled OF nodes, we
>> skip creating PHYs for them, and must also skip the CDR lock workaround.
>>
>> lynx_28g_supports_lane_mode() was a SerDes-global function and now
>> becomes per lane, to reflect the specific capabilities each instance may
>> have.
>>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>> ---
>> v2->v3:
>> - reword commit message
>> - add some comments regarding the "fsl,lynx-28g" fallback mechanism
>> - skip CDR lock workaround for lanes with no PHY (disabled in the device
>>   tree in the new binding)
>> v1->v2:
>> - remove priv->info->get_pccr() and priv->info->get_pcvt_offset().
>>   These were always called directly as lynx_28g_get_pccr() and
>>   lynx_28g_get_pcvt_offset().
>> - Add forgotten priv->info->lane_supports_mode() test to
>>   lynx_28g_supports_lane_mode().
>> - Rename the "fsl,lynx-28g" drvdata as lynx_info_compat rather than
>>   lynx_info_lx2160a_serdes1, to reflect its treatment as less featured.
>> - Implement a separate lane probing path for the #phy-cells = <0> case.
>>
>>  drivers/phy/freescale/phy-fsl-lynx-28g.c | 202 ++++++++++++++++++++---
>>  1 file changed, 179 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
>> index 453e76e0a6b7..1ddca8b4de17 100644
>> --- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
>> +++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
>> @@ -433,9 +433,15 @@ struct lynx_28g_lane {
>>  	enum lynx_lane_mode mode;
>>  };
>>  
>> +struct lynx_info {
>> +	bool (*lane_supports_mode)(int lane, enum lynx_lane_mode mode);
>> +	int first_lane;
>> +};
>> +
>>  struct lynx_28g_priv {
>>  	void __iomem *base;
>>  	struct device *dev;
>> +	const struct lynx_info *info;
>>  	/* Serialize concurrent access to registers shared between lanes,
>>  	 * like PCCn
>>  	 */
>> @@ -500,11 +506,18 @@ static enum lynx_lane_mode phy_interface_to_lane_mode(phy_interface_t intf)
>>  	}
>>  }
>>  
>> -static bool lynx_28g_supports_lane_mode(struct lynx_28g_priv *priv,
>> +/* A lane mode is supported if we have a PLL that can provide its required
>> + * clock net, and if there is a protocol converter for that mode on that lane.
>> + */
>> +static bool lynx_28g_supports_lane_mode(struct lynx_28g_lane *lane,
>>  					enum lynx_lane_mode mode)
>>  {
>> +	struct lynx_28g_priv *priv = lane->priv;
>>  	int i;
>>  
>> +	if (!priv->info->lane_supports_mode(lane->id, mode))
>> +		return false;
>> +
>>  	for (i = 0; i < LYNX_28G_NUM_PLL; i++) {
>>  		if (PLLnRSTCTL_DIS(priv->pll[i].rstctl))
>>  			continue;
>> @@ -687,6 +700,86 @@ static int lynx_28g_get_pcvt_offset(int lane, enum lynx_lane_mode lane_mode)
>>  	}
>>  }
>>  
>> +static bool lx2160a_serdes1_lane_supports_mode(int lane,
>> +					       enum lynx_lane_mode mode)
>> +{
>> +	return true;
>> +}
>> +
>> +static bool lx2160a_serdes2_lane_supports_mode(int lane,
>> +					       enum lynx_lane_mode mode)
>> +{
>> +	switch (mode) {
>> +	case LANE_MODE_1000BASEX_SGMII:
>> +		return true;
>> +	case LANE_MODE_USXGMII:
>> +	case LANE_MODE_10GBASER:
>> +		return lane == 6 || lane == 7;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static bool lx2160a_serdes3_lane_supports_mode(int lane,
>> +					       enum lynx_lane_mode mode)
>> +{
>> +	/*
>> +	 * Non-networking SerDes, and this driver supports only
>> +	 * networking protocols
>> +	 */
>> +	return false;
>> +}
>> +
>> +static bool lx2162a_serdes1_lane_supports_mode(int lane,
>> +					       enum lynx_lane_mode mode)
>> +{
>> +	return true;
>> +}
>> +
>> +static bool lx2162a_serdes2_lane_supports_mode(int lane,
>> +					       enum lynx_lane_mode mode)
>> +{
>> +	return lx2160a_serdes2_lane_supports_mode(lane, mode);
>> +}
>> +
>> +static bool lynx_28g_compat_lane_supports_mode(int lane,
>> +					       enum lynx_lane_mode mode)
>> +{
>> +	switch (mode) {
>> +	case LANE_MODE_1000BASEX_SGMII:
>> +	case LANE_MODE_USXGMII:
>> +	case LANE_MODE_10GBASER:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static const struct lynx_info lynx_info_compat = {
>> +	.lane_supports_mode = lynx_28g_compat_lane_supports_mode,
>> +};
>> +
>> +static const struct lynx_info lynx_info_lx2160a_serdes1 = {
>> +	.lane_supports_mode = lx2160a_serdes1_lane_supports_mode,
>> +};
>> +
>> +static const struct lynx_info lynx_info_lx2160a_serdes2 = {
>> +	.lane_supports_mode = lx2160a_serdes2_lane_supports_mode,
>> +};
>> +
>> +static const struct lynx_info lynx_info_lx2160a_serdes3 = {
>> +	.lane_supports_mode = lx2160a_serdes3_lane_supports_mode,
>> +};
>> +
>> +static const struct lynx_info lynx_info_lx2162a_serdes1 = {
>> +	.lane_supports_mode = lx2162a_serdes1_lane_supports_mode,
>> +	.first_lane = 4,
>> +};
>> +
>> +static const struct lynx_info lynx_info_lx2162a_serdes2 = {
>> +	.lane_supports_mode = lx2162a_serdes2_lane_supports_mode,
>> +};
>> +
>>  static int lynx_pccr_read(struct lynx_28g_lane *lane, enum lynx_lane_mode mode,
>>  			  u32 *val)
>>  {
>> @@ -939,7 +1032,6 @@ static int lynx_28g_lane_enable_pcvt(struct lynx_28g_lane *lane,
>>  static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>>  {
>>  	struct lynx_28g_lane *lane = phy_get_drvdata(phy);
>> -	struct lynx_28g_priv *priv = lane->priv;
>>  	int powered_up = lane->powered_up;
>>  	enum lynx_lane_mode lane_mode;
>>  	int err = 0;
>> @@ -951,7 +1043,7 @@ static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>>  		return -EOPNOTSUPP;
>>  
>>  	lane_mode = phy_interface_to_lane_mode(submode);
>> -	if (!lynx_28g_supports_lane_mode(priv, lane_mode))
>> +	if (!lynx_28g_supports_lane_mode(lane, lane_mode))
>>  		return -EOPNOTSUPP;
>>  
>>  	if (lane_mode == lane->mode)
>> @@ -984,14 +1076,13 @@ static int lynx_28g_validate(struct phy *phy, enum phy_mode mode, int submode,
>>  			     union phy_configure_opts *opts __always_unused)
>>  {
>>  	struct lynx_28g_lane *lane = phy_get_drvdata(phy);
>> -	struct lynx_28g_priv *priv = lane->priv;
>>  	enum lynx_lane_mode lane_mode;
>>  
>>  	if (mode != PHY_MODE_ETHERNET)
>>  		return -EOPNOTSUPP;
>>  
>>  	lane_mode = phy_interface_to_lane_mode(submode);
>> -	if (!lynx_28g_supports_lane_mode(priv, lane_mode))
>> +	if (!lynx_28g_supports_lane_mode(lane, lane_mode))
>>  		return -EOPNOTSUPP;
>>  
>>  	return 0;
>> @@ -1067,8 +1158,10 @@ static void lynx_28g_cdr_lock_check(struct work_struct *work)
>>  	u32 rrstctl;
>>  	int i;
>>  
>> -	for (i = 0; i < LYNX_28G_NUM_LANE; i++) {
>> +	for (i = priv->info->first_lane; i < LYNX_28G_NUM_LANE; i++) {
>>  		lane = &priv->lane[i];
>> +		if (!lane->phy)
>> +			continue;
>>  
>>  		mutex_lock(&lane->phy->mutex);
>>  
>> @@ -1120,24 +1213,48 @@ static struct phy *lynx_28g_xlate(struct device *dev,
>>  	struct lynx_28g_priv *priv = dev_get_drvdata(dev);
>>  	int idx = args->args[0];
>>  
>> -	if (WARN_ON(idx >= LYNX_28G_NUM_LANE))
>> +	if (WARN_ON(idx >= LYNX_28G_NUM_LANE ||
>> +		    idx < priv->info->first_lane))
>>  		return ERR_PTR(-EINVAL);
>>  
>>  	return priv->lane[idx].phy;
>>  }
FYI as an example please see below how I handled this previously.
The xlate function below can translate both phandles with 0 and 1 arguments:

static struct phy *lynx_28g_xlate(struct device *dev,
                  struct of_phandle_args *args)
{
    struct lynx_28g_priv *priv;
    struct phy *phy;
    int idx;

    if (args->args_count == 0) {
        /* direct look-up */
        phy = of_phy_simple_xlate(dev, args);
    } else if (args->args_count == 1) {
        /* look-up from parent by index */
        idx = args->args[0];
        if (WARN_ON(idx >= LYNX_28G_NUM_LANE))
            return ERR_PTR(-EINVAL);

        priv = dev_get_drvdata(dev);
        phy = priv->lane[idx].phy;
        if (!phy)
            phy = ERR_PTR(-ENODEV);
    } else {
        phy = ERR_PTR(-EINVAL);
    }

    return phy;
}

While in probe only one phy_provider is registered.

>>  
>> +static int lynx_28g_probe_lane(struct lynx_28g_priv *priv, int id,
>> +			       struct device_node *dn)
>> +{
>> +	struct lynx_28g_lane *lane = &priv->lane[id];
>> +	struct phy *phy;
>> +
>> +	memset(lane, 0, sizeof(*lane));
>> +
>> +	phy = devm_phy_create(priv->dev, dn, &lynx_28g_ops);
>> +	if (IS_ERR(phy))
>> +		return PTR_ERR(phy);
>> +
>> +	lane->priv = priv;
>> +	lane->phy = phy;
>> +	lane->id = id;
>> +	phy_set_drvdata(phy, lane);
>> +	lynx_28g_lane_read_configuration(lane);
>> +
>> +	return 0;
>> +}
>> +
>>  static int lynx_28g_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> +	bool lane_phy_providers = true;
>>  	struct phy_provider *provider;
>>  	struct lynx_28g_priv *priv;
>> -	int i;
>> +	int err;
>>  
>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>  	if (!priv)
>>  		return -ENOMEM;
>>  
>>  	priv->dev = dev;
>> +	priv->info = of_device_get_match_data(dev);
>>  	dev_set_drvdata(dev, priv);
>>  	spin_lock_init(&priv->pcc_lock);
>>  	INIT_DELAYED_WORK(&priv->cdr_check, lynx_28g_cdr_lock_check);
>> @@ -1146,26 +1263,60 @@ static int lynx_28g_probe(struct platform_device *pdev)
>>  	if (IS_ERR(priv->base))
>>  		return PTR_ERR(priv->base);
>>  
>> -	lynx_28g_pll_read_configuration(priv);
>> +	if (priv->info == &lynx_info_compat) {
>> +		/*
>> +		 * If we get here it means we probed on a device tree where
>> +		 * "fsl,lynx-28g" wasn't the fallback, but the sole compatible
>> +		 * string.
>> +		 */
>> +		dev_warn(dev, "Please update device tree to use per-device compatible strings\n");
>> +		lane_phy_providers = false;
>> +	}
>>  
>> -	for (i = 0; i < LYNX_28G_NUM_LANE; i++) {
>> -		struct lynx_28g_lane *lane = &priv->lane[i];
>> -		struct phy *phy;
>> +	lynx_28g_pll_read_configuration(priv);
>>  
>> -		memset(lane, 0, sizeof(*lane));
>> +	if (lane_phy_providers) {
>> +		struct device_node *dn = dev_of_node(dev), *child;
>> +
>> +		for_each_available_child_of_node(dn, child) {
>> +			u32 reg;
>> +
>> +			/* PHY subnode name must be 'phy'. */
>> +			if (!(of_node_name_eq(child, "phy")))
>> +				continue;
>> +
>> +			if (of_property_read_u32(child, "reg", &reg)) {
>> +				dev_err(dev, "No \"reg\" property for %pOF\n", child);
>> +				of_node_put(child);
>> +				return -EINVAL;
>> +			}
>> +
>> +			if (reg < priv->info->first_lane || reg >= LYNX_28G_NUM_LANE) {
>> +				dev_err(dev, "\"reg\" property out of range for %pOF\n", child);
>> +				of_node_put(child);
>> +				return -EINVAL;
>> +			}
>> +
>> +			err = lynx_28g_probe_lane(priv, reg, child);
>> +			if (err) {
>> +				of_node_put(child);
>> +				return err;
>> +			}
>> +		}
>>  
>> -		phy = devm_phy_create(dev, NULL, &lynx_28g_ops);
>> -		if (IS_ERR(phy))
>> -			return PTR_ERR(phy);
>> +		provider = devm_of_phy_provider_register(&pdev->dev,
>> +							 of_phy_simple_xlate);
> Is this really necessary? I believe phy_provider is not needed to resolve phandle
> to a single phy, and is used only when one driver provides multiple phys.
>> +	} else {
>> +		for (int i = priv->info->first_lane; i < LYNX_28G_NUM_LANE; i++) {
>> +			err = lynx_28g_probe_lane(priv, i, NULL);
>> +			if (err)
>> +				return err;
>> +		}
>>  
>> -		lane->priv = priv;
>> -		lane->phy = phy;
>> -		lane->id = i;
>> -		phy_set_drvdata(phy, lane);
>> -		lynx_28g_lane_read_configuration(lane);
>> +		provider = devm_of_phy_provider_register(&pdev->dev,
>> +							 lynx_28g_xlate);
>>  	}
>>  
>> -	provider = devm_of_phy_provider_register(dev, lynx_28g_xlate);
>>  	if (IS_ERR(provider))
>>  		return PTR_ERR(provider);
> Keep this to have compatibility with phandles to the parent.
>>  
>> @@ -1184,7 +1335,12 @@ static void lynx_28g_remove(struct platform_device *pdev)
>>  }
>>  
>>  static const struct of_device_id lynx_28g_of_match_table[] = {
>> -	{ .compatible = "fsl,lynx-28g" },
>> +	{ .compatible = "fsl,lx2160a-serdes1", .data = &lynx_info_lx2160a_serdes1 },
>> +	{ .compatible = "fsl,lx2160a-serdes2", .data = &lynx_info_lx2160a_serdes2 },
>> +	{ .compatible = "fsl,lx2160a-serdes3", .data = &lynx_info_lx2160a_serdes3 },
>> +	{ .compatible = "fsl,lx2162a-serdes1", .data = &lynx_info_lx2162a_serdes1 },
>> +	{ .compatible = "fsl,lx2162a-serdes2", .data = &lynx_info_lx2162a_serdes2 },
>> +	{ .compatible = "fsl,lynx-28g", .data = &lynx_info_compat }, /* fallback, keep last */
>>  	{ },
>>  };
>>  MODULE_DEVICE_TABLE(of, lynx_28g_of_match_table);
-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy