Message ID | 20220422150952.20587-3-alexandre.torgue@foss.st.com |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | Add SCMI version of ST boards | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/patch-applied | success | |
robh/dtbs-check | warning | build log |
robh/dt-meta-schema | success |
On 4/22/22 17:09, Alexandre Torgue wrote: > In case of "st,stm32mp1-rcc-secure" (stm32mp1 clock driver with RCC > security support hardened), "clocks" and "clock-names" describe oscillators > and are required. > > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> > > diff --git a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > index 7a251264582d..bb0e0b92e907 100644 > --- a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > +++ b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > @@ -58,14 +58,8 @@ properties: > - st,stm32mp1-rcc-secure > - st,stm32mp1-rcc > - const: syscon > - > - clocks: > - description: > - Specifies the external RX clock for ethernet MAC. > - maxItems: 1 > - > - clock-names: > - const: ETH_RX_CLK/ETH_REF_CLK > + clocks: true > + clock-names: true It looks like this should rather be a property than a compatible string -- the compatible string is used by the OS to determine which hardware is represented by a node, but here it is the same hardware in either case, "st,stm32mp1-rcc" and "st,stm32mp1-rcc-secure", it is still the same STM32MP1 RCC block, just configured differently by some bootloader stage. So why not just add one-liner property of the RCC block like ? st,rcc-in-secure-configuration
On Fri, Apr 22, 2022 at 06:31:25PM +0200, Marek Vasut wrote: > On 4/22/22 17:09, Alexandre Torgue wrote: > > In case of "st,stm32mp1-rcc-secure" (stm32mp1 clock driver with RCC > > security support hardened), "clocks" and "clock-names" describe oscillators > > and are required. > > > > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> > > > > diff --git a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > > index 7a251264582d..bb0e0b92e907 100644 > > --- a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > > +++ b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > > @@ -58,14 +58,8 @@ properties: > > - st,stm32mp1-rcc-secure > > - st,stm32mp1-rcc > > - const: syscon > > - > > - clocks: > > - description: > > - Specifies the external RX clock for ethernet MAC. > > - maxItems: 1 > > - > > - clock-names: > > - const: ETH_RX_CLK/ETH_REF_CLK > > + clocks: true > > + clock-names: true > > It looks like this should rather be a property than a compatible string -- > the compatible string is used by the OS to determine which hardware is > represented by a node, but here it is the same hardware in either case, > "st,stm32mp1-rcc" and "st,stm32mp1-rcc-secure", it is still the same > STM32MP1 RCC block, just configured differently by some bootloader stage. > > So why not just add one-liner property of the RCC block like ? > st,rcc-in-secure-configuration Because using compatible was already decided. Rob
On 4/25/22 21:11, Rob Herring wrote: > On Fri, Apr 22, 2022 at 06:31:25PM +0200, Marek Vasut wrote: >> On 4/22/22 17:09, Alexandre Torgue wrote: >>> In case of "st,stm32mp1-rcc-secure" (stm32mp1 clock driver with RCC >>> security support hardened), "clocks" and "clock-names" describe oscillators >>> and are required. >>> >>> Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> >>> >>> diff --git a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml >>> index 7a251264582d..bb0e0b92e907 100644 >>> --- a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml >>> +++ b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml >>> @@ -58,14 +58,8 @@ properties: >>> - st,stm32mp1-rcc-secure >>> - st,stm32mp1-rcc >>> - const: syscon >>> - >>> - clocks: >>> - description: >>> - Specifies the external RX clock for ethernet MAC. >>> - maxItems: 1 >>> - >>> - clock-names: >>> - const: ETH_RX_CLK/ETH_REF_CLK >>> + clocks: true >>> + clock-names: true >> >> It looks like this should rather be a property than a compatible string -- >> the compatible string is used by the OS to determine which hardware is >> represented by a node, but here it is the same hardware in either case, >> "st,stm32mp1-rcc" and "st,stm32mp1-rcc-secure", it is still the same >> STM32MP1 RCC block, just configured differently by some bootloader stage. >> >> So why not just add one-liner property of the RCC block like ? >> st,rcc-in-secure-configuration > > Because using compatible was already decided. I see ... may I ask why compatible is OK in this case even though this is encoding a policy (secure/non-secure configuration of the same clock IP) into DT ?
On Mon, Apr 25, 2022 at 09:35:13PM +0200, Marek Vasut wrote: > On 4/25/22 21:11, Rob Herring wrote: > > On Fri, Apr 22, 2022 at 06:31:25PM +0200, Marek Vasut wrote: > > > On 4/22/22 17:09, Alexandre Torgue wrote: > > > > In case of "st,stm32mp1-rcc-secure" (stm32mp1 clock driver with RCC > > > > security support hardened), "clocks" and "clock-names" describe oscillators > > > > and are required. > > > > > > > > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> > > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > > > > index 7a251264582d..bb0e0b92e907 100644 > > > > --- a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > > > > +++ b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > > > > @@ -58,14 +58,8 @@ properties: > > > > - st,stm32mp1-rcc-secure > > > > - st,stm32mp1-rcc > > > > - const: syscon > > > > - > > > > - clocks: > > > > - description: > > > > - Specifies the external RX clock for ethernet MAC. > > > > - maxItems: 1 > > > > - > > > > - clock-names: > > > > - const: ETH_RX_CLK/ETH_REF_CLK > > > > + clocks: true > > > > + clock-names: true > > > > > > It looks like this should rather be a property than a compatible string -- > > > the compatible string is used by the OS to determine which hardware is > > > represented by a node, but here it is the same hardware in either case, > > > "st,stm32mp1-rcc" and "st,stm32mp1-rcc-secure", it is still the same > > > STM32MP1 RCC block, just configured differently by some bootloader stage. > > > > > > So why not just add one-liner property of the RCC block like ? > > > st,rcc-in-secure-configuration > > > > Because using compatible was already decided. > > I see ... may I ask why compatible is OK in this case even though this is > encoding a policy (secure/non-secure configuration of the same clock IP) > into DT ? I see 'compatible' as an encoding of what is the programming model of the device. Secure vs. non-secure have different models. PCIe hosts vs endpoint mode is a similar case where we mostly have 2 compatibles (but not always). I wouldn't say which way we do things is set in stone, but in this case we already decided something. Rob
On Fri, 22 Apr 2022 17:09:46 +0200, Alexandre Torgue wrote: > In case of "st,stm32mp1-rcc-secure" (stm32mp1 clock driver with RCC > security support hardened), "clocks" and "clock-names" describe oscillators > and are required. > > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> > Reviewed-by: Rob Herring <robh@kernel.org>
Hi Rob On 5/5/22 16:11, Rob Herring wrote: > On Fri, Apr 22, 2022 at 10:10 AM Alexandre Torgue > <alexandre.torgue@foss.st.com> wrote: >> >> In case of "st,stm32mp1-rcc-secure" (stm32mp1 clock driver with RCC >> security support hardened), "clocks" and "clock-names" describe oscillators >> and are required. >> >> Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> > > This is now failing in linux-next: > > make[1]: *** Deleting file > 'Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.example.dts' > Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml: found > duplicate key "clocks" with value "{}" (original value: "True") > make[1]: *** [Documentation/devicetree/bindings/Makefile:26: > Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.example.dts] > Error 1 > ./Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml:64:3: > [error] duplication of key "clocks" in mapping (key-duplicates) > ./Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml:69:3: > [error] duplication of key "clock-names" in mapping (key-duplicates) > Traceback (most recent call last): > File "/usr/local/bin/dt-doc-validate", line 25, in check_doc > testtree = dtschema.load(filename, line_number=line_number) > File "/usr/local/lib/python3.10/dist-packages/dtschema/lib.py", line > 914, in load > return yaml.load(f.read()) > File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/main.py", > line 434, in load > return constructor.get_single_data() > File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", > line 121, in get_single_data > return self.construct_document(node) > File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", > line 131, in construct_document > for _dummy in generator: > File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", > line 674, in construct_yaml_map > value = self.construct_mapping(node) > File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", > line 445, in construct_mapping > return BaseConstructor.construct_mapping(self, node, deep=deep) > File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", > line 263, in construct_mapping > if self.check_mapping_key(node, key_node, mapping, key, value): > File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", > line 294, in check_mapping_key > raise DuplicateKeyError(*args) > ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping > in "<unicode string>", line 49, column 3 > found duplicate key "clocks" with value "{}" (original value: "True") > in "<unicode string>", line 64, column 3 > To suppress this check see: > http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys > During handling of the above exception, another exception occurred: > Traceback (most recent call last): > File "/usr/local/bin/dt-doc-validate", line 74, in <module> > ret = check_doc(f) > File "/usr/local/bin/dt-doc-validate", line 30, in check_doc > print(filename + ":", exc.path[-1], exc.message, file=sys.stderr) > AttributeError: 'DuplicateKeyError' object has no attribute 'path' It seems that we have a merge issue between: patch "dt-bindings: rcc: Add optional external ethernet RX clock properties" https://lore.kernel.org/r/20220410220514.21779-1-marex@denx.de and this one (dt-bindings: clock: stm32mp1: describes clocks if "st,stm32mp1-rcc-secure) On linux-next following part remains and creates issue above: clocks: description: Specifies the external RX clock for ethernet MAC. maxItems: 1 clock-names: const: ETH_RX_CLK/ETH_REF_CLK I don't know why this part is remaining. In my tree, I took care to take Marek patch first to avoid this kind of issue. Btw, how to fix that ? Note, that as soon as we will fix this point I'll send a fix to avoid issue in example build. cheers Alex
On 5/9/22 14:36, Rob Herring wrote: > On Fri, May 6, 2022 at 11:21 AM Rob Herring <robh+dt@kernel.org> wrote: >> >> On Fri, May 6, 2022 at 5:02 AM Alexandre TORGUE >> <alexandre.torgue@foss.st.com> wrote: >>> >>> Hi Rob >>> >>> On 5/5/22 16:11, Rob Herring wrote: >>>> On Fri, Apr 22, 2022 at 10:10 AM Alexandre Torgue >>>> <alexandre.torgue@foss.st.com> wrote: >>>>> >>>>> In case of "st,stm32mp1-rcc-secure" (stm32mp1 clock driver with RCC >>>>> security support hardened), "clocks" and "clock-names" describe oscillators >>>>> and are required. >>>>> >>>>> Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> >>>> >>>> This is now failing in linux-next: >>>> >>>> make[1]: *** Deleting file >>>> 'Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.example.dts' >>>> Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml: found >>>> duplicate key "clocks" with value "{}" (original value: "True") >>>> make[1]: *** [Documentation/devicetree/bindings/Makefile:26: >>>> Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.example.dts] >>>> Error 1 >>>> ./Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml:64:3: >>>> [error] duplication of key "clocks" in mapping (key-duplicates) >>>> ./Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml:69:3: >>>> [error] duplication of key "clock-names" in mapping (key-duplicates) >>>> Traceback (most recent call last): >>>> File "/usr/local/bin/dt-doc-validate", line 25, in check_doc >>>> testtree = dtschema.load(filename, line_number=line_number) >>>> File "/usr/local/lib/python3.10/dist-packages/dtschema/lib.py", line >>>> 914, in load >>>> return yaml.load(f.read()) >>>> File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/main.py", >>>> line 434, in load >>>> return constructor.get_single_data() >>>> File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", >>>> line 121, in get_single_data >>>> return self.construct_document(node) >>>> File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", >>>> line 131, in construct_document >>>> for _dummy in generator: >>>> File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", >>>> line 674, in construct_yaml_map >>>> value = self.construct_mapping(node) >>>> File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", >>>> line 445, in construct_mapping >>>> return BaseConstructor.construct_mapping(self, node, deep=deep) >>>> File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", >>>> line 263, in construct_mapping >>>> if self.check_mapping_key(node, key_node, mapping, key, value): >>>> File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", >>>> line 294, in check_mapping_key >>>> raise DuplicateKeyError(*args) >>>> ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping >>>> in "<unicode string>", line 49, column 3 >>>> found duplicate key "clocks" with value "{}" (original value: "True") >>>> in "<unicode string>", line 64, column 3 >>>> To suppress this check see: >>>> http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys >>>> During handling of the above exception, another exception occurred: >>>> Traceback (most recent call last): >>>> File "/usr/local/bin/dt-doc-validate", line 74, in <module> >>>> ret = check_doc(f) >>>> File "/usr/local/bin/dt-doc-validate", line 30, in check_doc >>>> print(filename + ":", exc.path[-1], exc.message, file=sys.stderr) >>>> AttributeError: 'DuplicateKeyError' object has no attribute 'path' >>> >>> It seems that we have a merge issue between: >>> >>> patch "dt-bindings: rcc: Add optional external ethernet RX clock properties" >>> https://lore.kernel.org/r/20220410220514.21779-1-marex@denx.de >>> >>> and this one (dt-bindings: clock: stm32mp1: describes clocks if >>> "st,stm32mp1-rcc-secure) >>> >>> On linux-next following part remains and creates issue above: >>> >>> clocks: >>> description: >>> Specifies the external RX clock for ethernet MAC. >>> maxItems: 1 >>> >>> clock-names: >>> const: ETH_RX_CLK/ETH_REF_CLK >>> >>> I don't know why this part is remaining. In my tree, I took care to take >>> Marek patch first to avoid this kind of issue. >>> >>> Btw, how to fix that ? >> >> Looks like I applied "dt-bindings: rcc: Add optional external ethernet >> RX clock properties" too. I've reverted it in my tree. > > Now with it reverted in today's next: > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.example.dtb: > rcc@50000000: 'clocks' is a required property > From schema: /builds/robherring/linux-dt/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > /builds/robherring/linux-dt/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.example.dtb: > rcc@50000000: 'clock-names' is a required property > From schema: /builds/robherring/linux-dt/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml > > The example needs updating. Sure, I send an update. Alex > > Rob
diff --git a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml index 7a251264582d..bb0e0b92e907 100644 --- a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml +++ b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml @@ -58,14 +58,8 @@ properties: - st,stm32mp1-rcc-secure - st,stm32mp1-rcc - const: syscon - - clocks: - description: - Specifies the external RX clock for ethernet MAC. - maxItems: 1 - - clock-names: - const: ETH_RX_CLK/ETH_REF_CLK + clocks: true + clock-names: true reg: maxItems: 1 @@ -76,6 +70,38 @@ required: - compatible - reg +if: + properties: + compatible: + contains: + enum: + - st,stm32mp1-rcc-secure +then: + properties: + clocks: + description: Specifies oscillators. + maxItems: 5 + + clock-names: + items: + - const: hse + - const: hsi + - const: csi + - const: lse + - const: lsi + required: + - clocks + - clock-names +else: + properties: + clocks: + description: + Specifies the external RX clock for ethernet MAC. + maxItems: 1 + + clock-names: + const: ETH_RX_CLK/ETH_REF_CLK + additionalProperties: false examples:
In case of "st,stm32mp1-rcc-secure" (stm32mp1 clock driver with RCC security support hardened), "clocks" and "clock-names" describe oscillators and are required. Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com>