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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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.