From: Swapnil Jakhade <[email protected]>
Add sub-node bindings for each group of PHY lanes based on PHY
type. Only PHY_TYPE_DP is supported currently. Each sub-node
includes properties such as master lane number, link reset, phy
type, number of lanes etc.
Signed-off-by: Swapnil Jakhade <[email protected]>
---
.../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
- 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
+ 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.
+
+ resets:
+ description:
+ Contains list of resets to get all the link lanes out of reset.
+
+ "#phy-cells":
+ description:
+ Generic PHY binding.
+ const: 0
+
+ cdns,phy-type:
+ description:
+ Should be PHY_TYPE_DP.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ cdns,num-lanes:
+ description:
+ Number of DisplayPort lanes.
+ allOf:
+ - $ref: /schemas/types.yaml#/definitions/uint32
+ - enum: [1, 2, 4]
+
+ cdns,max-bit-rate:
+ 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]
+
+ required:
+ - reg
+ - resets
+ - "#phy-cells"
+ - cdns,phy-type
required:
- compatible
+ - "#address-cells"
+ - "#size-cells"
- clocks
- clock-names
- reg
- reg-names
- - "#phy-cells"
+ - resets
+ - reset-names
additionalProperties: false
examples:
- |
- dp_phy: phy@f0fb500000 {
+ #include <dt-bindings/phy/phy.h>
+ torrent_phy: phy@f0fb500000 {
compatible = "cdns,torrent-phy";
reg = <0xf0 0xfb500000 0x0 0x00100000>,
<0xf0 0xfb030a00 0x0 0x00000040>;
reg-names = "torrent_phy", "dptx_phy";
- num_lanes = <4>;
- max_bit_rate = <8100>;
- #phy-cells = <0>;
+ resets = <&phyrst 0>;
+ reset-names = "torrent_reset";
clocks = <&ref_clk>;
clock-names = "refclk";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ torrent_phy_dp: torrent-phy@0 {
+ reg = <0>;
+ resets = <&phyrst 1>;
+ #phy-cells = <0>;
+ cdns,phy-type = <PHY_TYPE_DP>;
+ cdns,num-lanes = <4>;
+ cdns,max-bit-rate = <8100>;
+ };
};
...
--
2.4.5
On Wed, Jan 22, 2020 at 11:45:17AM +0100, Yuti Amonkar wrote:
> From: Swapnil Jakhade <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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'.
> + 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.
> +
> + resets:
> + description:
> + Contains list of resets to get all the link lanes out of reset.
Needs a schema for how many? 1 per lane?
> +
> + "#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.
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + cdns,num-lanes:
> + description:
> + Number of DisplayPort lanes.
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
> + - enum: [1, 2, 4]
> +
> + cdns,max-bit-rate:
> + 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]
> +
> + required:
> + - reg
> + - resets
> + - "#phy-cells"
> + - cdns,phy-type
Add (for the child nodes):
addtionalProperties: false
>
> required:
> - compatible
> + - "#address-cells"
> + - "#size-cells"
> - clocks
> - clock-names
> - reg
> - reg-names
> - - "#phy-cells"
> + - resets
> + - reset-names
>
> additionalProperties: false
>
> examples:
> - |
> - dp_phy: phy@f0fb500000 {
> + #include <dt-bindings/phy/phy.h>
> + torrent_phy: phy@f0fb500000 {
> compatible = "cdns,torrent-phy";
> reg = <0xf0 0xfb500000 0x0 0x00100000>,
> <0xf0 0xfb030a00 0x0 0x00000040>;
> reg-names = "torrent_phy", "dptx_phy";
> - num_lanes = <4>;
> - max_bit_rate = <8100>;
> - #phy-cells = <0>;
> + resets = <&phyrst 0>;
> + reset-names = "torrent_reset";
> clocks = <&ref_clk>;
> clock-names = "refclk";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + torrent_phy_dp: torrent-phy@0 {
Just 'phy@...'
> + reg = <0>;
> + resets = <&phyrst 1>;
> + #phy-cells = <0>;
> + cdns,phy-type = <PHY_TYPE_DP>;
> + cdns,num-lanes = <4>;
> + cdns,max-bit-rate = <8100>;
> + };
> };
> ...
> --
> 2.4.5
>
On Wed, Jan 22, 2020 at 11:45:17AM +0100, Yuti Amonkar wrote:
> From: Swapnil Jakhade <[email protected]>
>
> Add sub-node bindings for each group of PHY lanes based on PHY
> type. Only PHY_TYPE_DP is supported currently. Each sub-node
> includes properties such as master lane number, link reset, phy
> type, number of lanes etc.
>
> Signed-off-by: Swapnil Jakhade <[email protected]>
Also, if you send patches from another author, your S-o-b should also be
present.
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> .../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.
>> +
>> + 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.
>> +
>> + "#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.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> + cdns,num-lanes:
>> + description:
>> + Number of DisplayPort lanes.
>> + allOf:
>> + - $ref: /schemas/types.yaml#/definitions/uint32
>> + - enum: [1, 2, 4]
>> +
>> + cdns,max-bit-rate:
>> + 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]
>> +
>> + required:
>> + - reg
>> + - resets
>> + - "#phy-cells"
>> + - cdns,phy-type
>
> Add (for the child nodes):
>
> addtionalProperties: false
>
>>
>> required:
>> - compatible
>> + - "#address-cells"
>> + - "#size-cells"
>> - clocks
>> - clock-names
>> - reg
>> - reg-names
>> - - "#phy-cells"
>> + - resets
>> + - reset-names
>>
>> additionalProperties: false
>>
>> examples:
>> - |
>> - dp_phy: phy@f0fb500000 {
>> + #include <dt-bindings/phy/phy.h>
>> + torrent_phy: phy@f0fb500000 {
>> compatible = "cdns,torrent-phy";
>> reg = <0xf0 0xfb500000 0x0 0x00100000>,
>> <0xf0 0xfb030a00 0x0 0x00000040>;
>> reg-names = "torrent_phy", "dptx_phy";
>> - num_lanes = <4>;
>> - max_bit_rate = <8100>;
>> - #phy-cells = <0>;
>> + resets = <&phyrst 0>;
>> + reset-names = "torrent_reset";
>> clocks = <&ref_clk>;
>> clock-names = "refclk";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + torrent_phy_dp: torrent-phy@0 {
>
> Just 'phy@...'
>
>> + reg = <0>;
>> + resets = <&phyrst 1>;
>> + #phy-cells = <0>;
>> + cdns,phy-type = <PHY_TYPE_DP>;
>> + cdns,num-lanes = <4>;
>> + cdns,max-bit-rate = <8100>;
>> + };
>> };
>> ...
>> --
>> 2.4.5
>>
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/tree/Documentation/devicetree/bindings/phy/phy-cadence-sierra.txt?h=next
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/tree/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml?h=next
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Tue, Jan 28, 2020 at 4:04 AM Jyri Sarha <[email protected]> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> .../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