Re: [PATCH net-next 01/28] dt-bindings: phy: Add QorIQ SerDes binding

[Sean Anderson <sean.anderson@seco.com>]

On 6/19/22 7:24 AM, Krzysztof Kozlowski wrote:
> On 18/06/2022 05:38, Sean Anderson wrote:
>> Hi Krzysztof,
>>
>> On 6/17/22 9:15 PM, Krzysztof Kozlowski wrote:
>>> On 17/06/2022 13:32, Sean Anderson wrote:
>>>> This adds a binding for the SerDes module found on QorIQ processors. The
>>>> phy reference has two cells, one for the first lane and one for the
>>>> last. This should allow for good support of multi-lane protocols when
>>>> (if) they are added. There is no protocol option, because the driver is
>>>> designed to be able to completely reconfigure lanes at runtime.
>>>> Generally, the phy consumer can select the appropriate protocol using
>>>> set_mode. For the most part there is only one protocol controller
>>>> (consumer) per lane/protocol combination. The exception to this is the
>>>> B4860 processor, which has some lanes which can be connected to
>>>> multiple MACs. For that processor, I anticipate the easiest way to
>>>> resolve this will be to add an additional cell with a "protocol
>>>> controller instance" property.
>>>>
>>>> Each serdes has a unique set of supported protocols (and lanes). The
>>>> support matrix is stored in the driver and is selected based on the
>>>> compatible string. It is anticipated that a new compatible string will
>>>> need to be added for each serdes on each SoC that drivers support is
>>>> added for.
>>>>
>>>> There are two PLLs, each of which can be used as the master clock for
>>>> each lane. Each PLL has its own reference. For the moment they are
>>>> required, because it simplifies the driver implementation. Absent
>>>> reference clocks can be modeled by a fixed-clock with a rate of 0.
>>>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>> ---
>>>>
>>>>    .../bindings/phy/fsl,qoriq-serdes.yaml        | 78 +++++++++++++++++++
>>>>    1 file changed, 78 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/phy/fsl,qoriq-serdes.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/fsl,qoriq-serdes.yaml b/Documentation/devicetree/bindings/phy/fsl,qoriq-serdes.yaml
>>>> new file mode 100644
>>>> index 000000000000..4b9c1fcdab10
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/fsl,qoriq-serdes.yaml
>>>> @@ -0,0 +1,78 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/phy/fsl,qoriq-serdes.yaml#
>>>
>>> File name: fsl,ls1046a-serdes.yaml
>>
>> This is not appropriate, since this binding will be used for many QorIQ
>> devices, not just LS1046A.
> 
> This is the DT bindings convention and naming style, so why do you say
> it is not appropriate? If the new SoC at some point requires different
> binding what filename do you use? fsl,qoriq-serdes2.yaml? And then again
> fsl,qoriq-serdes3.yaml?

Correct. This serdes has been present in almost every QorIQ product over
a period of 10-15 years.

> Please follow DT bindings convention and name it after first compatible
> in the bindings.

As noted by Ioana, this is apparently a "lynx-10g" serdes, and will be
named appropriately.

>> The LS1046A is not even an "ur" device (first
>> model, etc.) but simply the one I have access to.
> 
> It does not matter that much if it is first in total. Use the first one
> from the documented compatibles.
> 
>>
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: NXP QorIQ SerDes Device Tree Bindings
>>>
>>> s/Device Tree Bindings//
>>
>> OK
>>
>>>> +
>>>> +maintainers:
>>>> +  - Sean Anderson <sean.anderson@seco.com>
>>>> +
>>>> +description: |
>>>> +  This binding describes the SerDes devices found in NXP's QorIQ line of
>>>
>>> Describe the device, not the binding, so wording "This binding" is not
>>> appropriate.
>>
>> OK
>>
>>>> +  processors. The SerDes provides up to eight lanes. Each lane may be
>>>> +  configured individually, or may be combined with adjacent lanes for a
>>>> +  multi-lane protocol. The SerDes supports a variety of protocols, including up
>>>> +  to 10G Ethernet, PCIe, SATA, and others. The specific protocols supported for
>>>> +  each lane depend on the particular SoC.
>>>> +
>>>> +properties:
>>>
>>> Compatible goes first.
>>>
>>>> +  "#phy-cells":
>>>> +    const: 2
>>>> +    description: |
>>>> +      The cells contain the following arguments.
>>>> +
>>>> +      - description: |
>>>
>>> Not a correct schema. What is this "- description" attached to? There is
>>> no items here...
>>
>> This is the same format as used by
>> Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> 
> I'll fix it.
> 
>>
>> How should the cells be documented?
> 
> Could be something like that:
> Documentation/devicetree/bindings/phy/microchip,lan966x-serdes.yaml
> 
>>
>>>> +          The first lane in the group. Lanes are numbered based on the register
>>>> +          offsets, not the I/O ports. This corresponds to the letter-based
>>>> +          ("Lane A") naming scheme, and not the number-based ("Lane 0") naming
>>>> +          scheme. On most SoCs, "Lane A" is "Lane 0", but not always.
>>>> +        minimum: 0
>>>> +        maximum: 7
>>>> +      - description: |
>>>> +          Last lane. For single-lane protocols, this should be the same as the
>>>> +          first lane.
>>>> +        minimum: 0
>>>> +        maximum: 7
>>>> +
>>>> +  compatible:
>>>> +    enum:
>>>> +      - fsl,ls1046a-serdes-1
>>>> +      - fsl,ls1046a-serdes-2
>>>
>>> Does not look like proper compatible and your explanation from commit
>>> msg did not help me. What "1" and "2" stand for? Usually compatibles
>>> cannot have some arbitrary properties encoded.
>>
>> Each serdes has a different set of supported protocols for each lane. This is encoded
>> in the driver data associated with the compatible
> 
> Implementation does not matter.

Of *course* implementation matters. Devicetree bindings do not happen in a vacuum. They
describe the hardware, but only in service to the implementation.

>> , along with the appropriate values
>> to plug into the protocol control registers. Because each serdes has a different set
>> of supported protocols
> 
> Another way is to express it with a property.
> 
>> and register configuration,
> 
> What does it mean exactly? The same protocols have different programming
> model on the instances?

(In the below paragraph, when I say "register" I mean "register or field within a
register")

Yes. Every serdes instance has a different way to program protocols into lanes. While
there is a little bit of orthogonality (the same registers are typically used for the
same protocols), each serdes is different. The values programmed into the registers are
unique to the serdes, and the lane which they apply to is also unique (e.g. the same
register may be used to program a different lane with a different protocol).

>> adding support for a new SoC will
>> require adding the appropriate configuration to the driver, and adding a new compatible
>> string. Although most of the driver is generic, this critical portion is shared only
>> between closely-related SoCs (such as variants with differing numbers of cores).
>>
> 
> Again implementation - we do not talk here about driver, but the bindings.
> 
>> The 1 and 2 stand for the number of the SerDes on that SoC. e.g. the documentation will
>> refer to SerDes1 and SerDes2.
>>    
>> So e.g. other compatibles might be
>>
>> - fsl,ls1043a-serdes-1 # There's only one serdes on this SoC
>> - fsl,t4042-serdes-1 # This SoC has four serdes
>> - fsl,t4042-serdes-2
>> - fsl,t4042-serdes-3
>> - fsl,t4042-serdes-4
> 
> If the devices are really different - there is no common parts in the
> programming model (registers) - then please find some descriptive
> compatible. However if the programming model of common part is
> consistent and the differences are only for different protocols (kind of
> expected), this should be rather a property describing which protocols
> are supported.
> 

I do not want to complicate the driver by attempting to encode such information in the
bindings. Storing the information in the driver is extremely common. Please refer to e.g.

- mvebu_comphy_cp110_modes in drivers/phy/marvell/phy-mvebu-cp110-comphy.c
- mvebu_a3700_comphy_modes in drivers/phy/marvell/phy-mvebu-a3700-comphy.c
- icm_matrix in drivers/phy/xilinx/phy-zynqmp.c
- samsung_usb2_phy_config in drivers/phy/samsung/
- 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

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.

There is, frankly, a large amount of variation between devices as implemented on different
SoCs. Especially because (AIUI) drivers must remain compatible with old devicetrees, I
think using a specific compatible string is especially appropriate here. 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.

--Sean