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
