| Message ID | 20211019231905.2974-1-digetx@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v1] dt-bindings: opp: Allow multi-worded node names | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | success |
On Wed, Oct 20, 2021 at 02:19:05AM +0300, Dmitry Osipenko wrote: > Not all OPP table names and OPP entries consist of a single word. In > particular NVIDIA Tegra OPP tables use multi-word names. Allow OPP node > and OPP entry name to have multi-worded names to silence DT checker > warnings about the multi-word names separated by hyphen. > > Reviewed-by: David Heidelberg <david@ixit.cz> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > index ae3ae4d39843..298cf24af270 100644 > --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > @@ -22,7 +22,7 @@ select: false > > properties: > $nodename: > - pattern: '^opp-table(-[a-z0-9]+)?$' > + pattern: '^opp-table(-[a-z0-9]+)*$' I don't see how this helps you. What I see needed upstream is a prefix: '-?opp-table(-[0-9]+)?$' Though really what I'd like to see is the OPP nodes moved into the device nodes they belong to when appropriate (i.e. when not shared between multiple devices). > > opp-shared: > description: > @@ -33,7 +33,7 @@ properties: > type: boolean > > patternProperties: > - '^opp-?[0-9]+$': > + '^opp(-[0-9]+)*$': > type: object > description: > One or more OPP nodes describing voltage-current-frequency combinations. > -- > 2.32.0 > >
20.10.2021 17:46, Rob Herring пишет: > On Wed, Oct 20, 2021 at 02:19:05AM +0300, Dmitry Osipenko wrote: >> Not all OPP table names and OPP entries consist of a single word. In >> particular NVIDIA Tegra OPP tables use multi-word names. Allow OPP node >> and OPP entry name to have multi-worded names to silence DT checker >> warnings about the multi-word names separated by hyphen. >> >> Reviewed-by: David Heidelberg <david@ixit.cz> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >> index ae3ae4d39843..298cf24af270 100644 >> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >> @@ -22,7 +22,7 @@ select: false >> >> properties: >> $nodename: >> - pattern: '^opp-table(-[a-z0-9]+)?$' >> + pattern: '^opp-table(-[a-z0-9]+)*$' > > I don't see how this helps you. What I see needed upstream is a prefix: > > '-?opp-table(-[0-9]+)?$' > > Though really what I'd like to see is the OPP nodes moved into the > device nodes they belong to when appropriate (i.e. when not shared > between multiple devices). I already prepared patches to rename Tegra OPP tables in accordance to the new naming scheme where opp-table- is the prefix.
20.10.2021 17:59, Dmitry Osipenko пишет: > 20.10.2021 17:46, Rob Herring пишет: >> On Wed, Oct 20, 2021 at 02:19:05AM +0300, Dmitry Osipenko wrote: >>> Not all OPP table names and OPP entries consist of a single word. In >>> particular NVIDIA Tegra OPP tables use multi-word names. Allow OPP node >>> and OPP entry name to have multi-worded names to silence DT checker >>> warnings about the multi-word names separated by hyphen. >>> >>> Reviewed-by: David Heidelberg <david@ixit.cz> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> index ae3ae4d39843..298cf24af270 100644 >>> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> @@ -22,7 +22,7 @@ select: false >>> >>> properties: >>> $nodename: >>> - pattern: '^opp-table(-[a-z0-9]+)?$' >>> + pattern: '^opp-table(-[a-z0-9]+)*$' >> >> I don't see how this helps you. What I see needed upstream is a prefix: >> >> '-?opp-table(-[0-9]+)?$' >> >> Though really what I'd like to see is the OPP nodes moved into the >> device nodes they belong to when appropriate (i.e. when not shared >> between multiple devices). > > I already prepared patches to rename Tegra OPP tables in accordance to > the new naming scheme where opp-table- is the prefix. > Now I see that there are other device-trees than Tegra which use the "postfix" naming scheme. Hence we should need to use this variant to cover both cases: '^([a-z0-9]+-)*opp-table|opp-table(-[a-z0-9]+)*$' I'll prepare v2, thanks.
On 20-10-21, 09:46, Rob Herring wrote: > On Wed, Oct 20, 2021 at 02:19:05AM +0300, Dmitry Osipenko wrote: > > Not all OPP table names and OPP entries consist of a single word. In > > particular NVIDIA Tegra OPP tables use multi-word names. Allow OPP node > > and OPP entry name to have multi-worded names to silence DT checker > > warnings about the multi-word names separated by hyphen. > > > > Reviewed-by: David Heidelberg <david@ixit.cz> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > > index ae3ae4d39843..298cf24af270 100644 > > --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > > +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > > @@ -22,7 +22,7 @@ select: false > > > > properties: > > $nodename: > > - pattern: '^opp-table(-[a-z0-9]+)?$' > > + pattern: '^opp-table(-[a-z0-9]+)*$' > > I don't see how this helps you. What I see needed upstream is a prefix: > > '-?opp-table(-[0-9]+)?$' I wonder if we should disallow that to keep naming more consistent. > Though really what I'd like to see is the OPP nodes moved into the > device nodes they belong to when appropriate (i.e. when not shared > between multiple devices). +1 The only reason for keeping it outside was because CPUs were sharing them.
22.10.2021 07:43, Viresh Kumar пишет: > On 20-10-21, 09:46, Rob Herring wrote: >> On Wed, Oct 20, 2021 at 02:19:05AM +0300, Dmitry Osipenko wrote: >>> Not all OPP table names and OPP entries consist of a single word. In >>> particular NVIDIA Tegra OPP tables use multi-word names. Allow OPP node >>> and OPP entry name to have multi-worded names to silence DT checker >>> warnings about the multi-word names separated by hyphen. >>> >>> Reviewed-by: David Heidelberg <david@ixit.cz> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> index ae3ae4d39843..298cf24af270 100644 >>> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> @@ -22,7 +22,7 @@ select: false >>> >>> properties: >>> $nodename: >>> - pattern: '^opp-table(-[a-z0-9]+)?$' >>> + pattern: '^opp-table(-[a-z0-9]+)*$' >> >> I don't see how this helps you. What I see needed upstream is a prefix: >> >> '-?opp-table(-[0-9]+)?$' > > I wonder if we should disallow that to keep naming more consistent. > >> Though really what I'd like to see is the OPP nodes moved into the >> device nodes they belong to when appropriate (i.e. when not shared >> between multiple devices). > > +1 > > The only reason for keeping it outside was because CPUs were sharing them. > Not very doable for Tegra device-trees. Those OPP tables are huge, it's impractical to put them into device nodes.
22.10.2021 07:43, Viresh Kumar пишет: > On 20-10-21, 09:46, Rob Herring wrote: >> On Wed, Oct 20, 2021 at 02:19:05AM +0300, Dmitry Osipenko wrote: >>> Not all OPP table names and OPP entries consist of a single word. In >>> particular NVIDIA Tegra OPP tables use multi-word names. Allow OPP node >>> and OPP entry name to have multi-worded names to silence DT checker >>> warnings about the multi-word names separated by hyphen. >>> >>> Reviewed-by: David Heidelberg <david@ixit.cz> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> index ae3ae4d39843..298cf24af270 100644 >>> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>> @@ -22,7 +22,7 @@ select: false >>> >>> properties: >>> $nodename: >>> - pattern: '^opp-table(-[a-z0-9]+)?$' >>> + pattern: '^opp-table(-[a-z0-9]+)*$' >> >> I don't see how this helps you. What I see needed upstream is a prefix: >> >> '-?opp-table(-[0-9]+)?$' > > I wonder if we should disallow that to keep naming more I also think that postfix variant should be disallowed for consistency. I sent out patches for both variants. V2 uses pattern that supports both naming schemes. If V2 is less appropriate, then please take this V1.
On 22-10-21, 09:40, Dmitry Osipenko wrote: > 22.10.2021 07:43, Viresh Kumar пишет: > > On 20-10-21, 09:46, Rob Herring wrote: > >> On Wed, Oct 20, 2021 at 02:19:05AM +0300, Dmitry Osipenko wrote: > >>> Not all OPP table names and OPP entries consist of a single word. In > >>> particular NVIDIA Tegra OPP tables use multi-word names. Allow OPP node > >>> and OPP entry name to have multi-worded names to silence DT checker > >>> warnings about the multi-word names separated by hyphen. > >>> > >>> Reviewed-by: David Heidelberg <david@ixit.cz> > >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >>> --- > >>> Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > >>> index ae3ae4d39843..298cf24af270 100644 > >>> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > >>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > >>> @@ -22,7 +22,7 @@ select: false > >>> > >>> properties: > >>> $nodename: > >>> - pattern: '^opp-table(-[a-z0-9]+)?$' > >>> + pattern: '^opp-table(-[a-z0-9]+)*$' > >> > >> I don't see how this helps you. What I see needed upstream is a prefix: > >> > >> '-?opp-table(-[0-9]+)?$' > > > > I wonder if we should disallow that to keep naming more > > I also think that postfix variant should be disallowed for consistency. > I sent out patches for both variants. > > V2 uses pattern that supports both naming schemes. If V2 is less > appropriate, then please take this V1. I didn't like V2, we can mandate to keep it like opp-* and opp-table-*. This patch looked okay to me, lets see what Rob has to say.
22.10.2021 09:50, Viresh Kumar пишет: > On 22-10-21, 09:40, Dmitry Osipenko wrote: >> 22.10.2021 07:43, Viresh Kumar пишет: >>> On 20-10-21, 09:46, Rob Herring wrote: >>>> On Wed, Oct 20, 2021 at 02:19:05AM +0300, Dmitry Osipenko wrote: >>>>> Not all OPP table names and OPP entries consist of a single word. In >>>>> particular NVIDIA Tegra OPP tables use multi-word names. Allow OPP node >>>>> and OPP entry name to have multi-worded names to silence DT checker >>>>> warnings about the multi-word names separated by hyphen. >>>>> >>>>> Reviewed-by: David Heidelberg <david@ixit.cz> >>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>> --- >>>>> Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>>>> index ae3ae4d39843..298cf24af270 100644 >>>>> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >>>>> @@ -22,7 +22,7 @@ select: false >>>>> >>>>> properties: >>>>> $nodename: >>>>> - pattern: '^opp-table(-[a-z0-9]+)?$' >>>>> + pattern: '^opp-table(-[a-z0-9]+)*$' >>>> >>>> I don't see how this helps you. What I see needed upstream is a prefix: >>>> >>>> '-?opp-table(-[0-9]+)?$' >>> >>> I wonder if we should disallow that to keep naming more >> >> I also think that postfix variant should be disallowed for consistency. >> I sent out patches for both variants. >> >> V2 uses pattern that supports both naming schemes. If V2 is less >> appropriate, then please take this V1. > > I didn't like V2, we can mandate to keep it like opp-* and opp-table-*. > > This patch looked okay to me, lets see what Rob has to say. > What we currently have for Tegra is a tegra-opps.dtsi and tegra.dtsi which includes the OPP's dtsi. the tegra-opps.dtsi has this structure: table: devname-opp-table { opp: ... }; and tegra.dtsi: #include "tegra-opps.dtsi" device@0000 { operating-points-v2 = <&table>; }; It just occurred to me that there is no need to move all tables to tegra.dtsi, but change structure of tegra-opps.dtsi to: device@0000 { operating-points-v2 = <&table>; table: opp-table { opp: ... }; }; Then there no need to change current naming scheme. Let me try to implement it and see how it goes.
On 22-10-21, 10:39, Dmitry Osipenko wrote: > What we currently have for Tegra is a tegra-opps.dtsi and tegra.dtsi > which includes the OPP's dtsi. > > the tegra-opps.dtsi has this structure: > > table: devname-opp-table { > opp: ... > }; > > and tegra.dtsi: > > #include "tegra-opps.dtsi" > > device@0000 { > operating-points-v2 = <&table>; > }; > > It just occurred to me that there is no need to move all tables to > tegra.dtsi, but change structure of tegra-opps.dtsi to: > > device@0000 { > operating-points-v2 = <&table>; > > table: opp-table { > opp: ... > }; > }; I thought you would have already thought about that and I was surprised when you saw the tables are big enough to be moved. I was wondering what does it really mean :) > Then there no need to change current naming scheme. Let me try to > implement it and see how it goes. That's good then.
22.10.2021 10:45, Viresh Kumar пишет: > On 22-10-21, 10:39, Dmitry Osipenko wrote: >> What we currently have for Tegra is a tegra-opps.dtsi and tegra.dtsi >> which includes the OPP's dtsi. >> >> the tegra-opps.dtsi has this structure: >> >> table: devname-opp-table { >> opp: ... >> }; >> >> and tegra.dtsi: >> >> #include "tegra-opps.dtsi" >> >> device@0000 { >> operating-points-v2 = <&table>; >> }; >> >> It just occurred to me that there is no need to move all tables to >> tegra.dtsi, but change structure of tegra-opps.dtsi to: >> >> device@0000 { >> operating-points-v2 = <&table>; >> >> table: opp-table { >> opp: ... >> }; >> }; > > I thought you would have already thought about that and I was surprised when you > saw the tables are big enough to be moved. I was wondering what does it really > mean :) > >> Then there no need to change current naming scheme. Let me try to >> implement it and see how it goes. > > That's good then. > I implemented that approach and it works, but there are two problems: 1. I had to factor out OPP tables from SPI device-tree nodes because DTC doesn't allow to have them within SPI nodes [1] and dtb fails to compile. [1] https://elixir.bootlin.com/linux/v5.15-rc6/source/scripts/dtc/checks.c#L1141 2. dtbs_check now warns about every opp-table sub-node, like this: /home/runner/work/linux/linux/arch/arm/boot/dts/tegra30-asus-nexus7-grouper-E1565.dt.yaml: memory-controller@7000f400: 'opp-table' does not match any of the regexes: '^emc-timings-[0-9]+$', 'pinctrl-[0-9]+' From schema: /home/runner/work/linux/linux/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml
23.10.2021 16:00, Dmitry Osipenko пишет: > 22.10.2021 10:45, Viresh Kumar пишет: >> On 22-10-21, 10:39, Dmitry Osipenko wrote: >>> What we currently have for Tegra is a tegra-opps.dtsi and tegra.dtsi >>> which includes the OPP's dtsi. >>> >>> the tegra-opps.dtsi has this structure: >>> >>> table: devname-opp-table { >>> opp: ... >>> }; >>> >>> and tegra.dtsi: >>> >>> #include "tegra-opps.dtsi" >>> >>> device@0000 { >>> operating-points-v2 = <&table>; >>> }; >>> >>> It just occurred to me that there is no need to move all tables to >>> tegra.dtsi, but change structure of tegra-opps.dtsi to: >>> >>> device@0000 { >>> operating-points-v2 = <&table>; >>> >>> table: opp-table { >>> opp: ... >>> }; >>> }; >> >> I thought you would have already thought about that and I was surprised when you >> saw the tables are big enough to be moved. I was wondering what does it really >> mean :) >> >>> Then there no need to change current naming scheme. Let me try to >>> implement it and see how it goes. >> >> That's good then. >> > > I implemented that approach and it works, but there are two problems: > > 1. I had to factor out OPP tables from SPI device-tree nodes because DTC doesn't allow to have them within SPI nodes [1] and dtb fails to compile. > > [1] https://elixir.bootlin.com/linux/v5.15-rc6/source/scripts/dtc/checks.c#L1141 > > 2. dtbs_check now warns about every opp-table sub-node, like this: > > /home/runner/work/linux/linux/arch/arm/boot/dts/tegra30-asus-nexus7-grouper-E1565.dt.yaml: memory-controller@7000f400: 'opp-table' does not match any of the regexes: '^emc-timings-[0-9]+$', 'pinctrl-[0-9]+' > From schema: /home/runner/work/linux/linux/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml > Thinking more about this, it's not a good idea to add new sub-node to nodes which already have sub-nodes because existing driver code that parses sub-nodes may not be ready to face the new opp-table sub-node. I'll revert back to the previous variant with distinct opp-table nodes.
23.10.2021 16:46, Dmitry Osipenko пишет: > 23.10.2021 16:00, Dmitry Osipenko пишет: >> 22.10.2021 10:45, Viresh Kumar пишет: >>> On 22-10-21, 10:39, Dmitry Osipenko wrote: >>>> What we currently have for Tegra is a tegra-opps.dtsi and tegra.dtsi >>>> which includes the OPP's dtsi. >>>> >>>> the tegra-opps.dtsi has this structure: >>>> >>>> table: devname-opp-table { >>>> opp: ... >>>> }; >>>> >>>> and tegra.dtsi: >>>> >>>> #include "tegra-opps.dtsi" >>>> >>>> device@0000 { >>>> operating-points-v2 = <&table>; >>>> }; >>>> >>>> It just occurred to me that there is no need to move all tables to >>>> tegra.dtsi, but change structure of tegra-opps.dtsi to: >>>> >>>> device@0000 { >>>> operating-points-v2 = <&table>; >>>> >>>> table: opp-table { >>>> opp: ... >>>> }; >>>> }; >>> >>> I thought you would have already thought about that and I was surprised when you >>> saw the tables are big enough to be moved. I was wondering what does it really >>> mean :) >>> >>>> Then there no need to change current naming scheme. Let me try to >>>> implement it and see how it goes. >>> >>> That's good then. >>> >> >> I implemented that approach and it works, but there are two problems: >> >> 1. I had to factor out OPP tables from SPI device-tree nodes because DTC doesn't allow to have them within SPI nodes [1] and dtb fails to compile. >> >> [1] https://elixir.bootlin.com/linux/v5.15-rc6/source/scripts/dtc/checks.c#L1141 >> >> 2. dtbs_check now warns about every opp-table sub-node, like this: >> >> /home/runner/work/linux/linux/arch/arm/boot/dts/tegra30-asus-nexus7-grouper-E1565.dt.yaml: memory-controller@7000f400: 'opp-table' does not match any of the regexes: '^emc-timings-[0-9]+$', 'pinctrl-[0-9]+' >> From schema: /home/runner/work/linux/linux/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml >> > > Thinking more about this, it's not a good idea to add new sub-node to > nodes which already have sub-nodes because existing driver code that > parses sub-nodes may not be ready to face the new opp-table sub-node. > > I'll revert back to the previous variant with distinct opp-table nodes. > I renamed OPP tables in accordance to the current opp-table-name pattern, but still OPP entry names have multiple words on Tegra and it's not feasible to change that without hurting readability. I'll prepare v3 that will only update pattern of OPP entries.
diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml index ae3ae4d39843..298cf24af270 100644 --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml @@ -22,7 +22,7 @@ select: false properties: $nodename: - pattern: '^opp-table(-[a-z0-9]+)?$' + pattern: '^opp-table(-[a-z0-9]+)*$' opp-shared: description: @@ -33,7 +33,7 @@ properties: type: boolean patternProperties: - '^opp-?[0-9]+$': + '^opp(-[0-9]+)*$': type: object description: One or more OPP nodes describing voltage-current-frequency combinations.