Message-ID: <535a0389-6c97-523d-382f-e54d69d3907e@seco.com>
Date:   Fri, 17 Jun 2022 23:38:33 -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
Subject: Re: [PATCH net-next 01/28] dt-bindings: phy: Add QorIQ SerDes binding

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@...o.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@...o.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@...0000 {
>> +      #phy-cells = <2>;
>> +      compatible = "fsl,ls1046a-serdes-1";
>> +      reg = <0x0 0x1ea0000 0x0 0x2000>;
>> +      clocks = <&clk_100mhz>, <&clk_156mhz>;
>> +      clock-names = "ref0", "ref1";
>> +    };
>> +
>> +...

--Sean

Powered by blists - more mailing lists