Message-ID: <5d724f49-71c4-96ad-b756-06b5683fa112@seco.com>
Date:   Mon, 20 Jun 2022 14:51:00 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>, netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        Eric Dumazet <edumazet@...gle.com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Vinod Koul <vkoul@...nel.org>, devicetree@...r.kernel.org,
        linux-phy@...ts.infradead.org,
        Ioana Ciornei <ioana.ciornei@....com>
Subject: Re: [PATCH net-next 01/28] dt-bindings: phy: Add QorIQ SerDes binding

On 6/20/22 2:21 PM, Krzysztof Kozlowski wrote:
>>>> - samsung_usb2_phy_config in drivers/phy/samsung/
>>>
>>> This one is a good example - where do you see there compatibles with
>>> arbitrary numbers attached?
>> 
>> samsung_usb2_phy_of_match in drivers/phy/samsung/phy-samsung-usb2.c
>> 
>> There is a different compatible for each SoC variant. Each compatible selects a struct
>> containing
>> 
>> - A list of phys, each with custom power on and off functions
>> - A function which converts a rate to an arbitrary value to program into a register
>> 
>> This is further documented in Documentation/driver-api/phy/samsung-usb2.rst
> 
> Exactly, please follow this approach. Compatible is per different
> device, e.g. different SoC variant. Of course you could have different
> devices on same SoC, but "1" and "2" are not different devices.

(in this case they are)

>> 
>>>> - qmp_phy_init_tbl in drivers/phy/qualcomm/phy-qcom-qmp.c
>>>>
>>>> All of these drivers (and there are more)
>>>>
>>>> - Use a driver-internal struct to encode information specific to different device models.
>>>> - Select that struct based on the compatible
>>>
>>> Driver implementation. You can do it in many different ways. Does not
>>> matter for the bindings.
>> 
>> And because this both describes the hardware and is convenient to the implementation,
>> I have chosen this way.
>> 
>>>>
>>>> The other thing is that while the LS1046A SerDes are fairly generic, other SerDes of this
>>>> type have particular restructions on the clocks. E.g. on some SoCs, certain protocols
>>>> cannot be used together (even if they would otherwise be legal), and some protocols must
>>>> use particular PLLs (whereas in general there is no such restriction). There are also
>>>> some register fields which are required to program on some SoCs, and which are reserved
>>>> on others.
>>>
>>> Just to be clear, because you are quite unspecific here ("some
>>> protocols") - we talk about the same protocol programmed on two of these
>>> serdes (serdes-1 and serdes-2 how you call it). Does it use different
>>> registers?
>> 
>> Yes.
>> 
>>> Are some registers - for the same protocol - reserved in one version?
>> 
>> Yes.
>> 
>> For example, I excerpt part of the documentation for PCCR2 on the T4240:
>> 
>>> XFIa Configuration:
>>> XFIA_CFG Default value set by RCW configuration.
>>> This field must be 0 for SerDes 3 & 4
>>> All settings not shown are reserved
>>>
>>> 00 Disabled
>>> 01 x1 on Lane 3 to FM2 MAC 9
>> 
>> And here is part of the documentation for PCCR2 on the LS1046A:
>> 
>>> SATAa Configuration
>>> All others reserved
>>> NOTE: This field is not supported in every instance. The following table includes only
>>>       supported registers.
>>> Field supported in	Field not supported in
>>> SerDes1_PCCR2		—
>>> —			SerDes2_PCCR2
>>>
>>> 000b - Disabled
>>> 001b - x1 on Lane 3 (SerDes #2 only)
>> 
>> And here is part of the documentation for PCCRB on the LS1046A:
>> 
>>> XFIa Configuration
>>> All others reserved Default value set by RCW configuration.
>>>
>>> 000b - Disabled
>>> 010b - x1 on Lane 1 to XGMIIa (Serdes #1 only)
>> You may notice that
>> 
>> - For some SerDes on the same SoC, these fields are reserved
> 
> That all sounds like quite different devices, which indeed usually is
> described with different compatibles. Still "xxx-1" and "xxx-2" are not
> valid compatibles. You need to come with some more reasonable name
> describing them. Maybe the block has revision or different model/vendor.

There is none AFAIK. Maybe someone from NXP can comment (since there are many
undocumented registers).

>> - Between different SoCs, different protocols may be configured in different registers
>> - The same registers may be used for different protocols in different SoCs (though
>>   generally there are several general layouts)
> 
> Different SoCs give you different compatibles, so problem is solved and
> that's not exactly argument for this case.
> 
>> - Fields have particular values which must be programmed
>> 
>> In addition, the documentation also says
>> 
>>> Reserved registers and fields must be preserved on writes.
>> 
>> All of these combined issues make it so that we need detailed, serdes-specific
>> configuration. The easiest way to store this configuration is in the driver. This
>> is consistent with *many* existing phy implementations. I would like to write a
>> standard phy driver, not one twisted by unusual device tree requirements.
> 
> Sure.
> 
>> 
>>>>
>>>> There is, frankly, a large amount of variation between devices as implemented on different
>>>> SoCs. 
>>>
>>> This I don't get. You mean different SoCs have entirely different
>>> Serdes? Sure, no problem. We talk here only about this SoC, this
>>> serdes-1 and serdes-2.
>>>
>>>> Especially because (AIUI) drivers must remain compatible with old devicetrees, I
>>>> think using a specific compatible string is especially appropriate here. 
>>>
>>> This argument does not make any sense in case of new bindings and new
>>> drivers, unless you build on top of existing implementation. Anyway no
>>> one asks you to break existing bindings...
>> 
>> When there is a bug in the bindings how do you fix it? If I were to follow your suggested method, it would be difficult to determine the particular devices
>> 
>>>> It will give us
>>>> the ability to correct any implementation quirks as they are discovered (and I anticipate
>>>> that there will be) rather than having to determine everything up front.
>>>
>>> All the quirks can be also chosen by respective properties.
>> 
>> Quirks are *exactly* the sort of implementation-specific details that you were opposed to above.
>> 
>>> Anyway, "serdes-1" and "serdes-2" are not correct compatibles,
>> 
>> The compatibles suggested were "fsl,ls1046-serdes-1" and -2. As noted above, these are separate
>> devices which, while having many similarities, have different register layouts and protocol
>> support. They are *not* 100% compatible with each other. Would you require that clock drivers
>> for different SoCs use the same compatibles just because they had the same registers, even though
>> the clocks themselves had different functions and hierarchy?
> 
> You miss the point. Clock controllers on same SoC have different names
> used in compatibles. We do not describe them as "vendor,aa-clk-1" and
> "vendor,aa-clk-2".
> 
> Come with proper naming and entire discussion might be not valid
> (although with not perfect naming Rob might come with questions). I
> cannot propose the name because I don't know these hardware blocks and I
> do not have access to datasheet.
> 
> Other way, if any reasonable naming is not possible, could be also to
> describe the meaning of "-1" suffix, e.g. that it does not mean some
> index but a variant from specification.

The documentation refers to these devices as "SerDes1", "SerDes2", etc.

Wold you prefer something like

serdes0: phy@...0000 {
	compatible = "fsl,ls1046a-serdes";
	variant = <0>;
};

?

--Sean

Powered by blists - more mailing lists