Am 08.09.25 um 17:37 schrieb Vladimir Oltean:

On Mon, Sep 08, 2025 at 02:02:35PM +0000, Josua Mayer wrote:

quoted

quoted

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

quoted

quoted

          - 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/ (local)

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.

quoted

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.