Re: [PATCH phy 13/14] dt-bindings: phy: lynx-28g: add compatible strings per SerDes and instantiation

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

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.