Date:   Thu, 30 Jun 2022 20:08:28 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Sean Anderson <sean.anderson@...o.com>,
        Rob Herring <robh@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>, netdev@...r.kernel.org,
        Russell King <linux@...linux.org.uk>,
        Paolo Abeni <pabeni@...hat.com>,
        linux-arm-kernel@...ts.infradead.org,
        Eric Dumazet <edumazet@...gle.com>,
        linux-kernel@...r.kernel.org,
        Kishon Vijay Abraham I <kishon@...com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Vinod Koul <vkoul@...nel.org>, devicetree@...r.kernel.org,
        linux-phy@...ts.infradead.org
Subject: Re: [PATCH net-next v2 01/35] dt-bindings: phy: Add QorIQ SerDes
 binding

On 30/06/2022 20:01, Sean Anderson wrote:
> Hi Rob,
> 
> On 6/30/22 1:27 PM, Rob Herring wrote:
>> On Tue, Jun 28, 2022 at 06:13:30PM -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 is no "generic" compatible string for this reason.
>>>
>>> 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>
>>> ---
>>>
>>> Changes in v2:
>>> - Add #clock-cells. This will allow using assigned-clocks* to configure
>>>   the PLLs.
>>> - Allow a value of 1 for phy-cells. This allows for compatibility with
>>>   the similar (but according to Ioana Ciornei different enough) lynx-28g
>>>   binding.
>>> - Document phy cells in the description
>>> - Document the structure of the compatible strings
>>> - Fix example binding having too many cells in regs
>>> - Move compatible first
>>> - Refer to the device in the documentation, rather than the binding
>>> - Remove minItems
>>> - Rename to fsl,lynx-10g.yaml
>>> - Use list for clock-names
>>>
>>>  .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 93 +++++++++++++++++++
>>>  1 file changed, 93 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml
>>> new file mode 100644
>>> index 000000000000..b5a6f631df9f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml
>>> @@ -0,0 +1,93 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/phy/fsl,lynx-10g.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: NXP Lynx 10G SerDes
>>> +
>>> +maintainers:
>>> +  - Sean Anderson <sean.anderson@...o.com>
>>> +
>>> +description: |
>>> +  These Lynx "SerDes" devices are found in NXP's QorIQ line of 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:
>>> +    description: |
>>> +      Each compatible is of the form "fsl,<soc-name>-serdes-<instance>".
>>> +      Although many registers are compatible between different SoCs, the
>>> +      supported protocols and lane assignments tend to be unique to each SerDes.
>>> +      Additionally, the method of activating protocols may also be unique.
>>
>> We typically have properties for handling these variables. Numbering 
>> instances is something we avoid.
> 
> On v1, Krzysztof said that this was a better route...

I commented about "-1" and "-2" saying you have to make them properties.
You disagreed and with long messages were convincing me that "-1" and
"-2" is the only reasonable approach. I never said it is a better route.
I explicitly asked in several places for defining these as properties,
not as compatibles.

You are twisting the entire discussion now.

Best regards,
Krzysztof

Powered by blists - more mailing lists