[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250908153746.7mfobudenttdi4qd@skbuf>
Date: Mon, 8 Sep 2025 18:37:46 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Josua Mayer <josua@...id-run.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
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:
> > - 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.
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:
- 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.
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]).
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);
}
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. (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.
Powered by blists - more mailing lists