Re: [PATCH v5 2/8] dt-bindings: phy: Add Lynx 10G phy binding

[Rob Herring <robh@kernel.org>]

On Fri, Sep 02, 2022 at 05:37:15PM -0400, Sean Anderson wrote:
> This adds a binding for the SerDes module found on QorIQ processors.
> Each phy is a subnode of the top-level device, possibly supporting
> multiple lanes and protocols. This "thick" #phy-cells is used due to
> allow for better organization of parameters. Note that the particular
> parameters necessary to select a protocol-controller/lane combination
> vary across different SoCs, and even within different SerDes on the same
> SoC.
> 
> 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.
> 
> 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>
> ---
> 
> (no changes since v4)
> 
> Changes in v4:
> - Use subnodes to describe lane configuration, instead of describing
>   PCCRs. This is the same style used by phy-cadence-sierra et al.
> 
> Changes in v3:
> - Manually expand yaml references
> - Add mode configuration to device tree
> 
> Changes in v2:
> - Rename to fsl,lynx-10g.yaml
> - Refer to the device in the documentation, rather than the binding
> - Move compatible first
> - Document phy cells in the description
> - 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.
> - Remove minItems
> - Use list for clock-names
> - Fix example binding having too many cells in regs
> - Add #clock-cells. This will allow using assigned-clocks* to configure
>   the PLLs.
> - Document the structure of the compatible strings
> 
>  .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 236 ++++++++++++++++++
>  1 file changed, 236 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..59417e6172d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml
> @@ -0,0 +1,236 @@
> +# 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@seco.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:
> +    items:
> +      - enum:
> +          - fsl,ls1046a-serdes
> +          - fsl,ls1088a-serdes
> +      - const: fsl,lynx-10g
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  "#clock-cells":
> +    const: 1
> +    description: |
> +      The cell contains an ID as described in dt-bindings/clock/fsl,lynx-10g.h.
> +      Note that when assigning a rate to a PLL, the PLL's rate is divided by
> +      1000 to avoid overflow. A rate of 5000000 corresponds to 5GHz.
> +
> +  clocks:
> +    maxItems: 2
> +    description: |
> +      Clock for each PLL reference clock input.
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 2
> +    items:
> +      enum:
> +        - ref0
> +        - ref1
> +
> +  reg:
> +    maxItems: 1
> +
> +patternProperties:
> +  '^phy@':
> +    type: object
> +
> +    description: |
> +      A contiguous group of lanes which will be configured together. Each group
> +      corresponds to one phy device. Lanes not described by any group will be
> +      left as-is.
> +
> +    properties:
> +      "#phy-cells":
> +        const: 0
> +
> +      reg:
> +        minItems: 1
> +        maxItems: 8
> +        description:
> +          The lanes in the group. These must be listed in order. The first lane
> +          will have the FIRST_LANE bit set in GCR0. The order of lanes also
> +          determines the reset order (TRSTDIR).
> +
> +    patternProperties:
> +      '^(q?sgmii|xfi)':
> +        type: object
> +
> +        description: |
> +          A protocol controller which may control the group of lanes. Each
> +          controller is selected through the PCCRs. In addition to protocols
> +          desired for use by the OS, protocols which may have been configured
> +          by the bootloader must also be described. This ensures that only one
> +          protocol controller is attached to a group of lanes at once.
> +
> +        properties:
> +          fsl,pccr:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: |
> +              The index of the PCCR which configures this protocol controller.
> +              This is the same as the register name suffix. For example, PCCR8
> +              would use a value of 8 for an offset of 0x220 (0x200 + 4 * 8).
> +
> +          fsl,index:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: |
> +              The index of the protocol controller. This corresponds to the
> +              suffix in the documentation. For example, PEXa would be 0, PEXb
> +              1, etc. Generally, higher fields occupy lower bits.
> +
> +          fsl,cfg:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            minimum: 1
> +            description: |
> +              The configuration value to program into the protocol controller
> +              field.
> +
> +          fsl,type:

Use the common 'phy-type' here.

I don't love this binding, but it seems using phy cells here doesn't 
work and I don't have other suggestions. At the end of the day, it's 
just one device. I'll be less enthusiastic on the next one.

With the above change,

Reviewed-by: Rob Herring <robh@kernel.org>

Rob