Message-ID: <1ec4002f-6c5a-4f64-8ba7-7f991b0f3f75@solid-run.com>
Date: Thu, 2 Oct 2025 10:40:51 +0000
From: Josua Mayer <josua@...id-run.com>
To: Vladimir Oltean <vladimir.oltean@....com>, "linux-phy@...ts.infradead.org"
	<linux-phy@...ts.infradead.org>
CC: Ioana Ciornei <ioana.ciornei@....com>, Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 phy 13/17] phy: lynx-28g: probe on per-SoC and
 per-instance compatible strings

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@...nel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@...nel.org>
> Cc: Conor Dooley <conor+dt@...nel.org>
> Cc: devicetree@...r.kernel.org
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....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;
>  }
>  
> +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);

Powered by blists - more mailing lists