Message-ID: <537d0c0f-8f29-ddb9-dc02-a9e9ad63702a@seco.com>
Date:   Thu, 30 Jun 2022 14:16:21 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        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 6/30/22 2:08 PM, Krzysztof Kozlowski wrote:
> 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.
>>>
>> 
>> 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.

Sorry, I didn't mean to come off that way.

>>> We typically have properties for handling these variables. Numbering 
>>> instances is something we avoid.

But I would like to point out that for the phy subsystem, it is typical
to select using the compatible (or just write a new driver).

--Sean

Powered by blists - more mailing lists