Re: [PATCH v3 13/14] dt-bindings: phy: phy-cadence-torrent: Add subnode bindings.

[Rob Herring]

On Tue, Jan 28, 2020 at 4:04 AM Jyri Sarha <jsarha@xxxxxx> wrote:
>
> On 27/01/2020 18:42, Rob Herring wrote:
> > On Wed, Jan 22, 2020 at 11:45:17AM +0100, Yuti Amonkar wrote:
> >> From: Swapnil Jakhade <sjakhade@xxxxxxxxxxx>
> >>
> >> Add sub-node bindings for each group of PHY lanes based on PHY
> >> type. Only PHY_TYPE_DP is supported currently. Each sub-node
> >
> > What the driver supports is not relevant to the binding. Define all
> > modes.
> >
> >> includes properties such as master lane number, link reset, phy
> >> type, number of lanes etc.
> >
> > Given the conversion and this have no compatibility, just make the
> > commits delete the old binding and add this new binding. I'd rather not
> > have reviewed what just gets deleted here.
> >
> >>
> >> Signed-off-by: Swapnil Jakhade <sjakhade@xxxxxxxxxxx>
> >> ---
> >>  .../bindings/phy/phy-cadence-torrent.yaml          | 90 ++++++++++++++++++----
> >>  1 file changed, 73 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
> >> index dbb8aa5..eb21615 100644
> >> --- a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
> >> +++ b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
> >> @@ -19,6 +19,12 @@ properties:
> >>        - cdns,torrent-phy
> >>        - ti,j721e-serdes-10g
> >>
> >> +  '#address-cells':
> >> +    const: 1
> >> +
> >> +  '#size-cells':
> >> +    const: 0
> >> +
> >>    clocks:
> >>      maxItems: 1
> >>      description:
> >> @@ -41,44 +47,94 @@ properties:
> >>        - const: torrent_phy
> >>        - const: dptx_phy
> >>
> >> -  "#phy-cells":
> >> -    const: 0
> >> +  resets:
> >> +    description:
> >> +      Must contain an entry for each in reset-names.
> >> +      See Documentation/devicetree/bindings/reset/reset.txt
> >
> > How many reset entries? Needs a 'maxItems: 1' or an 'items' list if more
> > than 1.
> >
> >>
> >> -  num_lanes:
> >> +  reset-names:
> >>      description:
> >> -      Number of DisplayPort lanes.
> >> -    allOf:
> >> -      - $ref: /schemas/types.yaml#/definitions/uint32
> >> -      - enum: [1, 2, 4]
> >> +      Must be "torrent_reset". It controls the reset to the
> >
> > Should be a schema, not freeform text. However, not really a useful name
> > as there's only 1, so I'd just remove 'reset-names'.
> >
>
> This binding is trying to follow "cdns,sierra-phy-t0" binding [1] when
> it makes sense. Sierra defines two resets here. But if we can not name
> the other reset now (at least I can not), I guess we can just drop the
> reset-names here.
>
> >> +      torrent PHY.
> >>
> >> -  max_bit_rate:
> >> +patternProperties:
> >> +  '^torrent-phy@[0-7]+$':
> >> +    type: object
> >>      description:
> >> -      Maximum DisplayPort link bit rate to use, in Mbps
> >> -    allOf:
> >> -      - $ref: /schemas/types.yaml#/definitions/uint32
> >> -      - enum: [2160, 2430, 2700, 3240, 4320, 5400, 8100]
> >> +      Each group of PHY lanes with a single master lane should be represented as a sub-node.
> >> +    properties:
> >> +      reg:
> >> +        description:
> >> +          The master lane number. This is the lowest numbered lane in the lane group.
> >
> > Why not make it the list of lane numbers. Then you don't need num-lanes.
> >
>
> Sierra binding already defines this method [1] and my plan was to rely
> on this method when selecting the lane types in the
> "ti,phy-j721e-wiz"-driver [2].
>
> IOW, I would like the both Sierra and Torrent bindings (which both are
> wrapped by the wiz wrapper IP) to be compatible enough for wiz driver to
> peek the lane types from the wrapped phy-node.

Okay.

> >> +      resets:
> >> +        description:
> >> +          Contains list of resets to get all the link lanes out of reset.
> >
> > Needs a schema for how many? 1 per lane?
> >
>
> That is what the current implementation is, but do we have to lock it
> down in the binding? There can hardly be more than 1 / lane, but I can
> imagine it to be just one for a number of lanes.

Yes, you have to define it in the schema. If not, there's really no
point in doing schemas.

> >> +      "#phy-cells":
> >> +        description:
> >> +          Generic PHY binding.
> >
> > Not a useful description. Remove.
> >
> >> +        const: 0
> >> +
> >> +      cdns,phy-type:
> >> +        description:
> >> +          Should be PHY_TYPE_DP.
> >
> > Sounds like a constraint.
> >
>
> I do not think there is point to limit this to PHY_TYPE_DP only. The
> current implementation may not support anything else but DP, but we
> should not limit the binding because of it. I think referring to the
> include/dt-bindings/phy/phy.h header here would be appropriate.

Referring to the include is still not a constraint. You need const,
enum, or minimum/maximum.

Rob