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

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

On Fri, 17 Jun 2022 16:32:45 -0400, 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
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/fsl,qoriq-serdes.example.dtb: phy@1ea0000: reg: [[0, 32112640], [0, 8192]] is too long
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/fsl,qoriq-serdes.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

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

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP QorIQ SerDes Device Tree Bindings

s/Device Tree Bindings//

> +
> +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.

> +  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...

> +          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.

> +
> +  clocks:
> +    minItems: 2

No need for minItems.

> +    maxItems: 2
> +    description: |
> +      Clock for each PLL reference clock input.
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 2
> +    items:
> +      pattern: "^ref[0-1]$"

No, instead describe actual items with "const". See other examples.

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - "#phy-cells"
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    serdes1: phy@1ea0000 {
> +      #phy-cells = <2>;
> +      compatible = "fsl,ls1046a-serdes-1";
> +      reg = <0x0 0x1ea0000 0x0 0x2000>;
> +      clocks = <&clk_100mhz>, <&clk_156mhz>;
> +      clock-names = "ref0", "ref1";
> +    };
> +
> +...


Best regards,
Krzysztof

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. The LS1046A is not even an "ur" device (first
model, etc.) but simply the one I have access to.

>> +$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

How should the cells be documented?

>> +          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, along with the appropriate values
to plug into the protocol control registers. Because each serdes has a different set
of supported protocols and register configuration, 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).

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

>> +
>> +  clocks:
>> +    minItems: 2
> 
> No need for minItems.

OK

>> +    maxItems: 2
>> +    description: |
>> +      Clock for each PLL reference clock input.
>> +
>> +  clock-names:
>> +    minItems: 2
>> +    maxItems: 2
>> +    items:
>> +      pattern: "^ref[0-1]$"
> 
> No, instead describe actual items with "const". See other examples.

Again, same format as xlnx,zynqmp-psgtr.yaml

I will update this to use items.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - "#phy-cells"
>> +  - compatible
>> +  - clocks
>> +  - clock-names
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    serdes1: phy@1ea0000 {
>> +      #phy-cells = <2>;
>> +      compatible = "fsl,ls1046a-serdes-1";
>> +      reg = <0x0 0x1ea0000 0x0 0x2000>;
>> +      clocks = <&clk_100mhz>, <&clk_156mhz>;
>> +      clock-names = "ref0", "ref1";
>> +    };
>> +
>> +...

--Sean

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

On 20/06/2022 19:19, Sean Anderson wrote:
> 
> 
> On 6/20/22 6:54 AM, Krzysztof Kozlowski wrote:
>> On 19/06/2022 17:53, Sean Anderson wrote:
>>>>>
>>>>>>> +          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.
>>
>> This is so not true. > Bindings do not service implementation. Bindings
>> happen in vacuum
> 
> Where are all the bindings for hardware without drivers?
> 
> Why don't device trees describe the entire hardware before any drivers are written?
> 
> Actually, I have seen some device trees written like that (baked into the chip's ROM),
> and they cannot be used because the bindings
> 
> - Do not fully describe the hardware (e.g. clocks, resets, interrupts, and other things)
> - Do not describe the hardware in a compatible way (e.g. using different names for
>   registers and clocks, or ordering fields differently).
> - Contain typos and errors (since they were never used)
> 
> These same issues apply to any new binding documentation. Claiming that bindings happen
> in a vacuum is de facto untrue, and would be unsound practice if it wasn't.
> 
>> because they are used by different implementations:
>> Linux, u-Boot, BSD and several other quite different systems.
> 
> U-Boot doesn't use devicetree for this device (and if it did the port would likely be
> based on the Linux driver). BSD doesn't support this hardware at all. We are the first
> to create a driver for this device, so we get to choose the binding.

You choose the binding in respect to the guidelines. You cannot choose
some random weirdness just because you are first. It does not matter
that there are no other implementations in that way - you must choose
reasonable bindings.

> 
>> Any references to implemention from the bindings is questionable,
>> although of course not always wrong.
>>
>> Building bindings per specific implementation is as well usually not
>> correct.
> 
> Sure, but there are of course many ways to create bindings, even for the same hardware.
> As an example, pinctrl bindings can be written like
> 
> pinctrl@cafebabe {
> 	uart-tx {
> 		function = "uart-tx";
> 		pins = "5";
> 	};
> };
> 
> or
> 
> pinctrl@deadbeef {
> 	uart-tx {
> 		pinmux = <SOME_MACRO(5, UART_TX)>;
> 	};
> };
> 
> or
> 
> pinctrl@d00dfeed {
> 	uart-tx {
> 		pinmux = <SOME_MACRO(5, FUNC3)>;
> 	};
> };
> 
> and which one to use depends both on the structure of the hardware, as well as the
> driver. These bindings require a different driver style under the hood, and using
> the wrong binding can unnecessarily complicate the driver for no reason.
> 
> To further beat home the point, someone might use a "fixed-clock" to describe a clock
> and then later change to a more detailed implementation. They could use "simple-pinctrl"
> and then later move to a device-specific driver. 
> 
> If the devicetree author is smart, then they will create a binding like
> 
> clock {
> 	compatible = "vendor,my-clock", "fixed-clock";
> 	...
> };
> 
> so that better support might be added in the future. In fact, that is *exactly* what I
> am suggesting here.
> 
>>>
>>>>> , 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).
>>
>> That's not answering the point here, but I'll respond to the later
>> paragraph.
>>
>>>
>>>>> 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.
>>
>> Yes, quirks are even more common, more flexible and are in general
>> recommended for more complicated cases. Yet you talk about driver
>> implementation, which I barely care.
>>
>>>
>>> - 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/
>>
>> 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.

> 
>>> - 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.

> - 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.

> 
> --Sean
> 
>> so my NAK
>> stays. These might be separate compatibles, although that would require
>> proper naming and proper justification (as you did not answer my actual
>> questions about differences when using same protocols). Judging by the
>> bindings and your current description (implementation does not matter),
>> this also looks like a property.


Best regards,
Krzysztof