| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5dd3bbaa-bac2-441b-881d-1a2e0ff0e3db@solid-run.com>
Date: Thu, 2 Oct 2025 10:50:46 +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 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@...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;
>> }
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", ®)) {
>> + 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