[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fcfbea96-7978-49f6-88c6-f78fe52edb7c@solid-run.com>
Date: Mon, 8 Sep 2025 16:04:45 +0000
From: Josua Mayer <josua@...id-run.com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Conor Dooley <conor@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Krzysztof Kozlowski <krzk@...nel.org>, "linux-phy@...ts.infradead.org"
<linux-phy@...ts.infradead.org>, 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>, Conor Dooley <conor+dt@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH phy 13/14] dt-bindings: phy: lynx-28g: add compatible
strings per SerDes and instantiation
Am 08.09.25 um 17:37 schrieb Vladimir Oltean:
> On Mon, Sep 08, 2025 at 02:02:35PM +0000, Josua Mayer wrote:
>>> My updated plan is to describe the schema rules for the compatible as
>>> follows. Is that ok with everyone?
>>>
>>> properties:
>>> compatible:
>>> oneOf:
>>> - const: fsl,lynx-28g
>>> deprecated: true
>>> - items:
>>> - const: fsl,lx2160a-serdes1
>>> - const: fsl,lynx-28g
>>> - enum:
missed fsl,lx2160a-serdes1
>>> - fsl,lx2160a-serdes2
>>> - fsl,lx2160a-serdes3
>>> - fsl,lx2162a-serdes1
>>> - fsl,lx2162a-serdes2
>> Weak objection, I think this is more complex than it should be.
>> Perhaps it was discussed before to keep two compatible strings ...:
>>
>> properties:
>> compatible:
>> items:
>> - enum:
>> - fsl,lx2160a-serdes2
>> - fsl,lx2160a-serdes3
>> - fsl,lx2162a-serdes1
>> - fsl,lx2162a-serdes2
>> - const: fsl,lynx-28g
>>
>> This will cause the dtbs_check to complain about anyone in the future
>> using it wrong.
My proposal requires two compatible strings always, or the schema will fail
to validate. Examples:
compatible = "fsl,lynx-28g";
// fails validation but driver can keep supporting it for backwards compatibility
compatible = "fsl,lx2160a-serdes1", "fsl,lynx-28g";
// valid per my proposal, functional with existing driver and future changes.
// this is how you will know it is SD #1
compatible = "fsl,lx2160a-serdes2", "fsl,lynx-28g";
// valid per my proposal, and driver can use it in the future to identify SD #2
The kernel looks in compatible strings for the *first match*.
> So just that we stay on track, this is what the submitted patch
> originally proposed:
>
> properties:
> compatible:
> oneOf:
> - items:
> - const: fsl,lynx-28g
> - items:
> - enum:
> - fsl,lx2160a-serdes1
> - fsl,lx2160a-serdes2
> - fsl,lx2160a-serdes3
> - fsl,lx2162a-serdes1
> - fsl,lx2162a-serdes2
> - const: fsl,lynx-28g
>
> Your proposal is different in the following ways:
- always require 2 compatible strings specified in combination,.
validation fails when fsl,lynx-28g string specified alone.
> - Just compatible = "fsl,lynx-28g" will produce a schema validation error, BUT
>
> - There is no compatible = "fsl,lx2160a-serdes1". I don't understand how
> you propose to describe that SerDes.
copy-paste failure, I intended to list them all, including sd1.
>
> I realize I've CCed you late on the patches. They are here:
> https://lore.kernel.org/lkml/20250904154402.300032-1-vladimir.oltean@nxp.com/
>
> One of Conor's objections was that keeping "fsl,lynx-28g" as a fallback
> compatible string may not make sense, and indeed I tried to highlight in
> my previous reply that it can lead to incorrect behaviour if SerDes #2
> is described in this way.
>
> Further trying to argue that SerDes #2 should have "fsl,lynx-28g" as a
> fallback without directly addressing the fact it results in incorrect
> behaviour is... strange.
>
> Also, SerDes #3 is not described at all, it's not necessary to introduce
> a fallback when it can be described precisely from the start.
>
>> The driver can still probe on fsl,lynx-28g alone for backwards compatibility,
>> and you can limit the feature-set as you see fit in such case.
>>
>> Main argument for always specifying lynx-28g is that the serdes blocks
>> do share a common programming model and register definitions.
> I think this is the sticking point. The blocks do share a common
> programming model, but that model does not give us a way to identify the
> supported protocols. You can try to enable a protocol converter that
> doesn't exist, and read back the enablement status, and you'll find the
> hardware reports it to be enabled (for example PCCC[SXGMIIA_CFG]).
Indeed.
>
> The snippet below is something you can try out and see for yourself (it
> will need adaptation depending on kernel revision).
>
> static void lynx_28g_lane_probe_supported(struct lynx_28g_lane *lane)
> {
> enum lynx_lane_mode lane_mode;
> unsigned long supported = 0;
> int err;
>
> for (lane_mode = LANE_MODE_UNKNOWN + 1; lane_mode < LANE_MODE_MAX; lane_mode++) {
> u32 orig_val, val;
>
> err = lynx_pccr_read(lane, lane_mode, &orig_val);
> if (err)
> continue;
>
> val = orig_val;
>
> switch (lane_mode) {
> case LANE_MODE_1000BASEKX:
> val |= PCC8_SGMIIa_KX;
> fallthrough;
> case LANE_MODE_1000BASEX_SGMII:
> val |= PCC8_SGMIIa_CFG;
> break;
> case LANE_MODE_10GBASER:
> case LANE_MODE_10GBASEKR:
> val |= PCCC_SXGMIIn_XFI;
> fallthrough;
> case LANE_MODE_USXGMII:
> val |= PCCC_SXGMIIn_CFG;
> break;
> case LANE_MODE_25GBASER:
> case LANE_MODE_25GBASEKR:
> val |= PCCD_E25Gn_CFG;
> break;
> case LANE_MODE_40GBASER_XLAUI:
> case LANE_MODE_40GBASEKR4:
> val |= PCCE_E40Gn_CFG;
> break;
> default:
> break;
> }
>
> err = lynx_pccr_write(lane, lane_mode, val);
> if (err)
> continue;
>
> err = lynx_pccr_read(lane, lane_mode, &val);
> if (err)
> continue;
>
> dev_info(&lane->phy->dev, "Protocol %d: PCCR was 0x%x, is 0x%x\n",
> lane_mode, orig_val, val);
>
> switch (lane_mode) {
> case LANE_MODE_1000BASEKX:
> if (val & PCC8_SGMIIa_KX)
> supported |= BIT(lane_mode);
> fallthrough;
> case LANE_MODE_1000BASEX_SGMII:
> if (val & PCC8_SGMIIa_CFG)
> supported |= BIT(lane_mode);
> break;
> case LANE_MODE_10GBASER:
> case LANE_MODE_10GBASEKR:
> if (val & PCCC_SXGMIIn_XFI)
> supported |= BIT(lane_mode);
> fallthrough;
> case LANE_MODE_USXGMII:
> if (val & PCCC_SXGMIIn_CFG)
> supported |= BIT(lane_mode);
> break;
> case LANE_MODE_25GBASER:
> case LANE_MODE_25GBASEKR:
> if (val & PCCD_E25Gn_CFG)
> supported |= BIT(lane_mode);
> break;
> case LANE_MODE_40GBASER_XLAUI:
> case LANE_MODE_40GBASEKR4:
> if (val & PCCE_E40Gn_CFG)
> supported |= BIT(lane_mode);
> break;
> default:
> break;
> }
>
> WARN_ON(lynx_pccr_write(lane, lane_mode, orig_val));
> }
>
> dev_info(&lane->phy->dev, "Lane supported modes: 0x%lx\n", supported);
> }
I did play with those in u-boot trying to derive dt dpmac node status from
protocol converter registers ... ... ...
> The fact that SerDes #2 works on the fsl-lx2162a-clearfog.dts is
> accidental and doesn't change the fact that describing it as
> "fsl,lynx-28g" is wrong.
It works because only the SGMII modes are used on that board.
You can however use this argument to drop driver support for the
lone fsl,lynx-28g compatible string.
> (of course, I stand corrected if someone finds
> a way to determine that 10GbE is unsupported on some lanes based on just
> the programming model, but I doubt it.)
>
> The only 3 ways to find the list of supported protocols, that are known
> to me to work, are:
> #1: list them all in the device tree (talking about tens, and the list
> is ever-expanding as the driver gets more development). This is by
> far the most complex and difficult to maintain solution and my least
> preferred.
> #2: hardcode them in the driver, based on SerDes compatible string (the
> current patch, or variations). This is my preferred variant for
> keeping the dt-bindings simple and the
> #3: like #2, but distinguish between two "fsl,lynx-28g" instances based
> on the "reg" value. This should work fine, as every SerDes block
> index is instatiated at a fixed physical address in every SoC (block
> #1: 0x1ea0000, #2: 0x1eb0000, #3: 0x1ec0000). It should directly
> address your objection, but:
> - it also requires dt-bindings maintainers buy-in.
> - this method can distinguish features of SerDes i from j, but not
> from SoC A vs B. There is an upcoming Lynx 10G driver where we
> need the per-SoC capabilities as well, and it would be good to
> have the same overall driver and dt-binding structure for both.
#4: by listing descriptive phy sub-nodes under the serdes blocks root node.
Presence indicates whether or not a lane is available,
while each node can specify the modes it supports.
Obviously this allows the device-tree author to describe invalid configurations.
But it avoids describing each combination in the driver.
Powered by blists - more mailing lists