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 11:37 schrieb Vladimir Oltean:
> On Fri, Sep 05, 2025 at 08:02:59PM +0100, Conor Dooley wrote:
>> On Fri, Sep 05, 2025 at 06:41:50PM +0300, Vladimir Oltean wrote:
>>> On Fri, Sep 05, 2025 at 10:29:33AM +0200, Krzysztof Kozlowski wrote:
>>>>>  properties:
>>>>>    compatible:
>>>>> -    enum:
>>>>> -      - fsl,lynx-28g
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - const: fsl,lynx-28g
>>>> Don't change that part. Previous enum was correct. You want oneOf and
>>>> enum.
>>> Combining the feedback from Conor and Josua, I should only be permitting
>>> the use of "fsl,lynx-28g" as a fallback to "fsl,lx216{0,2}a-serdes{1,2}",
>>> or standalone. The description below achieves just that. Does it look ok
>>> to you?
>>>
>>> properties:
>>>   compatible:
>>>     oneOf:
>>>       - enum:
>>>           - fsl,lx2160a-serdes1
>>>           - fsl,lx2160a-serdes2
>>>           - fsl,lx2160a-serdes3
>>>           - fsl,lx2162a-serdes1
>>>           - fsl,lx2162a-serdes2
>>>       - const: fsl,lynx-28g
>>>         deprecated: true
>>>       - items:
>>>           - const: fsl,lx2160a-serdes1
>>>           - const: fsl,lynx-28g
>>>         deprecated: true
>>>       - items:
>>>           - const: fsl,lx2160a-serdes2
>>>           - const: fsl,lynx-28g
>>>         deprecated: true
>>>       - items:
>>>           - const: fsl,lx2162a-serdes1
>>>           - const: fsl,lynx-28g
>>>         deprecated: true
>>>       - items:
>>>           - const: fsl,lx2162a-serdes2
>>>           - const: fsl,lynx-28g
>>>         deprecated: true
>> This doesn't really make sense, none of these are currently in use
>> right? Everything is just using fsl,lynx-28g right?
>> Adding new stuff and immediately marking it deprecated is a
>> contradiction, just don't add it at all if you don't want people using
>> it. Any users of it would be something you're going to retrofit in now,
>> so you may as well just retrofit to use what you want people to use
>> going forward, which has no fallbacks.
> You're right that it doesn't make sense to deprecate a newly introduced
> compatible string pair - my mistake for misunderstanding "deprecated".
>
>> I didn't read the back and forth with Josua (sorry!) but is the fallback
>> even valid? Do those devices have a common minimum set of features that
>> they share?
> I'll try to make an argument based on the facts presented below.
>
> The current Linux driver, which recognizes only "fsl,lynx-28g", supports
> only 1GbE and 10GbE. There are other SerDes protocols supported by the
> SerDes, but they are irrelevant for the purpose of discussing
> compatibility. Also, LX2160A SerDes #3 is also irrelevant, because it is
> not currently described in the device tree.
>
> 1GbE compatibility table
>
> SerDes              Lane 0  Lane 1  Lane 2  Lane 3  Lane 4  Lane 5  Lane 6  Lane 7   Comments
> LX2160A SerDes #1   y       y       y       y       y       y       y       y
> LX2160A SerDes #2   y       y       y       y       y       y       y       y
> LX2162A SerDes #1   n/a     n/a     n/a     n/a     y       y       y       y        LX2162A currently uses lx2160a.dtsi
> LX2162A SerDes #2   y       y       y       y       y       y       y       y        LX2162A currently uses lx2160a.dtsi
>
> 10GbE compatibility table
>
> SerDes              Lane 0  Lane 1  Lane 2  Lane 3  Lane 4  Lane 5  Lane 6  Lane 7   Comments
> LX2160A SerDes #1   y       y       y       y       y       y       y       y
> LX2160A SerDes #2   n       n       n       n       n       n       y       y
> LX2162A SerDes #1   n/a     n/a     n/a     n/a     y       y       y       y        LX2162A currently uses lx2160a.dtsi
> LX2162A SerDes #2   n       n       n       n       n       n       y       y        LX2162A currently uses lx2160a.dtsi
>
> As LX2160A SerDes #2 is treated like #1, the device tree is telling the
> driver that all lanes support 10GbE (which is false for lanes 0-5).
>
> If one of the SerDes PLLs happens to be provisioned for the 10GbE clock
> net frequency, as for example with the RCW[SRDS_PRTCL_S2]=6 setting,
> this will make the driver think that it can reconfigure lanes 0-5 as
> 10GbE.
>
> This will directly affect upper layers (phylink), which will advertise
> 10GbE modes to its link partner on ports which support only 1GbE, and
> the non-functional link mode might be resolved through negotiation, when
> a lower speed but functional link could have been established.
>
> You mention a common minimum feature set. That would be supporting 10GbE
> only on lanes 6-7, which would be disadvantageous to existing uses of
> 10GbE on lanes 0-5 of SerDes #1. In some cases, the change might also be
> breaking - there might be a PHY attached to these lanes whose firmware
> is hardcoded to expect 10GbE, so there won't be a graceful degradation
> to 1GbE in all cases.
>
> With Josua's permission, I would consider commit 2f2900176b44 ("arm64:
> dts: lx2160a: describe the SerDes block #2") as broken, for being an
> incorrect description of hardware - it is presented as identical to
> another device, which has a different supported feature set. I will not
> try to keep SerDes #2 compatible with "fsl,lynx-28g". This will remain
> synonymous only with SerDes #1. The users of the fsl-lx2162a-clearfog.dts
> will need updating if the "undetected lack of support for 10GbE" becomes
> an issue.
>
> 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.

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.

>
> Also, I will limit the driver support for the "fsl,lynx-28g" compatible
> to just 1GbE and 10GbE. The 25GbE feature introduced by this series will
> require a more precise compatible string.
Okay