Message ID | 20220429134143.628428-4-herve.codina@bootlin.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | RZN1 USB Host support | expand |
On 29/04/2022 15:41, Herve Codina wrote: > Add internal PCI bridge support for the r9a06g032 SOC. The Renesas > RZ/N1D (R9A06G032) internal PCI bridge is compatible with the one > present in the R-Car Gen2 family. > Compared to the R-Car Gen2 family, it needs three clocks instead of > one. > > The 'resets' property for the RZ/N1 family is not required since > there is no reset-controller support yet for the RZ/N1 family. This should not be a reason why a property is or is not required. Either this is required for device operation or not. If it is required, should be in the bindings. Otherwise what are you going to do in the future? Add a required property breaking the ABI? > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > .../bindings/pci/renesas,pci-rcar-gen2.yaml | 46 ++++++++++++++++--- > 1 file changed, 39 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml b/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml > index 494eb975c146..a9f806794f12 100644 > --- a/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml > +++ b/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml > @@ -32,6 +32,10 @@ properties: > - renesas,pci-r8a7793 # R-Car M2-N > - renesas,pci-r8a7794 # R-Car E2 > - const: renesas,pci-rcar-gen2 # R-Car Gen2 and RZ/G1 > + - items: > + - enum: > + - renesas,pci-r9a06g032 # RZ/N1D > + - const: renesas,pci-rzn1 # RZ/N1 > > reg: > items: > @@ -41,13 +45,9 @@ properties: > interrupts: > maxItems: 1 > > - clocks: > - items: > - - description: Device clock > + clocks: true > > - clock-names: > - items: > - - const: pclk > + clock-names: true > > resets: > maxItems: 1 > @@ -106,13 +106,45 @@ required: > - interrupt-map > - interrupt-map-mask > - clocks > - - resets > - power-domains > - bus-range > - "#address-cells" > - "#size-cells" > - "#interrupt-cells" > > +if: allOf. > + properties: > + compatible: > + contains: > + enum: > + - renesas,pci-rzn1 > + > +then: > + properties: > + clocks: > + items: > + - description: Internal bus clock (AHB) for HOST > + - description: Internal bus clock (AHB) Power Management > + - description: PCI clock for USB subsystem > + clock-names: > + items: > + - const: hclkh > + - const: hclkpm > + - const: pciclk > + required: > + - clock-names > + > +else: > + properties: > + clocks: > + items: > + - description: Device clock > + clock-names: > + items: > + - const: pclk > + required: > + - resets > + > unevaluatedProperties: false > > examples: Best regards, Krzysztof
Hi Krzysztof, On Sun, May 1, 2022 at 10:51 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 29/04/2022 15:41, Herve Codina wrote: > > Add internal PCI bridge support for the r9a06g032 SOC. The Renesas > > RZ/N1D (R9A06G032) internal PCI bridge is compatible with the one > > present in the R-Car Gen2 family. > > Compared to the R-Car Gen2 family, it needs three clocks instead of > > one. > > > > The 'resets' property for the RZ/N1 family is not required since > > there is no reset-controller support yet for the RZ/N1 family. > > This should not be a reason why a property is or is not required. Either > this is required for device operation or not. If it is required, should > be in the bindings. Otherwise what are you going to do in the future? > Add a required property breaking the ABI? The problem is that there are no bindings for the reset controller (actually the reset controller feature of the system-controller) yet. Yeah, we can just add #reset-cells = <1> to the system-controller device node, but we cannot add the actual resets properties to the consumers, until the actual cell values are defined. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, May 02, 2022 at 11:19:19AM +0200, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Sun, May 1, 2022 at 10:51 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > On 29/04/2022 15:41, Herve Codina wrote: > > > Add internal PCI bridge support for the r9a06g032 SOC. The Renesas > > > RZ/N1D (R9A06G032) internal PCI bridge is compatible with the one > > > present in the R-Car Gen2 family. > > > Compared to the R-Car Gen2 family, it needs three clocks instead of > > > one. > > > > > > The 'resets' property for the RZ/N1 family is not required since > > > there is no reset-controller support yet for the RZ/N1 family. > > > > This should not be a reason why a property is or is not required. Either > > this is required for device operation or not. If it is required, should > > be in the bindings. Otherwise what are you going to do in the future? > > Add a required property breaking the ABI? > > The problem is that there are no bindings for the reset controller > (actually the reset controller feature of the system-controller) yet. > Yeah, we can just add #reset-cells = <1> to the system-controller > device node, but we cannot add the actual resets properties to the > consumers, until the actual cell values are defined. Sounds like you should implement providers first. Or just live with the warning as a reminder to implement the reset provider? Rob
Hi Rob, On Mon, May 2, 2022 at 9:44 PM Rob Herring <robh@kernel.org> wrote: > On Mon, May 02, 2022 at 11:19:19AM +0200, Geert Uytterhoeven wrote: > > On Sun, May 1, 2022 at 10:51 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > On 29/04/2022 15:41, Herve Codina wrote: > > > > Add internal PCI bridge support for the r9a06g032 SOC. The Renesas > > > > RZ/N1D (R9A06G032) internal PCI bridge is compatible with the one > > > > present in the R-Car Gen2 family. > > > > Compared to the R-Car Gen2 family, it needs three clocks instead of > > > > one. > > > > > > > > The 'resets' property for the RZ/N1 family is not required since > > > > there is no reset-controller support yet for the RZ/N1 family. > > > > > > This should not be a reason why a property is or is not required. Either > > > this is required for device operation or not. If it is required, should > > > be in the bindings. Otherwise what are you going to do in the future? > > > Add a required property breaking the ABI? > > > > The problem is that there are no bindings for the reset controller > > (actually the reset controller feature of the system-controller) yet. > > Yeah, we can just add #reset-cells = <1> to the system-controller > > device node, but we cannot add the actual resets properties to the > > consumers, until the actual cell values are defined. > > Sounds like you should implement providers first. Or just live with the > warning as a reminder to implement the reset provider? I'd go for the latter. The upstream r9a06g032.dtsi is still under active development. Until very recently, the only device supported was the serial console. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 03/05/2022 08:51, Geert Uytterhoeven wrote: >>>> This should not be a reason why a property is or is not required. Either >>>> this is required for device operation or not. If it is required, should >>>> be in the bindings. Otherwise what are you going to do in the future? >>>> Add a required property breaking the ABI? >>> >>> The problem is that there are no bindings for the reset controller >>> (actually the reset controller feature of the system-controller) yet. >>> Yeah, we can just add #reset-cells = <1> to the system-controller >>> device node, but we cannot add the actual resets properties to the >>> consumers, until the actual cell values are defined. >> >> Sounds like you should implement providers first. Or just live with the >> warning as a reminder to implement the reset provider? > > I'd go for the latter. The upstream r9a06g032.dtsi is still under active > development. Until very recently, the only device supported was the > serial console. For clocks we use in such cases fixed-clock placeholders or empty phandles. Maybe something like that would work here as well? Best regards, Krzysztof
Hi Krzysztof, On Tue, May 3, 2022 at 11:29 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 03/05/2022 08:51, Geert Uytterhoeven wrote: > >>>> This should not be a reason why a property is or is not required. Either > >>>> this is required for device operation or not. If it is required, should > >>>> be in the bindings. Otherwise what are you going to do in the future? > >>>> Add a required property breaking the ABI? > >>> > >>> The problem is that there are no bindings for the reset controller > >>> (actually the reset controller feature of the system-controller) yet. > >>> Yeah, we can just add #reset-cells = <1> to the system-controller > >>> device node, but we cannot add the actual resets properties to the > >>> consumers, until the actual cell values are defined. > >> > >> Sounds like you should implement providers first. Or just live with the > >> warning as a reminder to implement the reset provider? > > > > I'd go for the latter. The upstream r9a06g032.dtsi is still under active > > development. Until very recently, the only device supported was the > > serial console. > > For clocks we use in such cases fixed-clock placeholders or empty > phandles. Maybe something like that would work here as well? I don't think that works for resets. Besides, the driver doesn't need or use the reset anyway. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, May 03, 2022 at 11:29:53AM +0200, Krzysztof Kozlowski wrote: > On 03/05/2022 08:51, Geert Uytterhoeven wrote: > >>>> This should not be a reason why a property is or is not required. Either > >>>> this is required for device operation or not. If it is required, should > >>>> be in the bindings. Otherwise what are you going to do in the future? > >>>> Add a required property breaking the ABI? > >>> > >>> The problem is that there are no bindings for the reset controller > >>> (actually the reset controller feature of the system-controller) yet. > >>> Yeah, we can just add #reset-cells = <1> to the system-controller > >>> device node, but we cannot add the actual resets properties to the > >>> consumers, until the actual cell values are defined. > >> > >> Sounds like you should implement providers first. Or just live with the > >> warning as a reminder to implement the reset provider? > > > > I'd go for the latter. The upstream r9a06g032.dtsi is still under active > > development. Until very recently, the only device supported was the > > serial console. > > For clocks we use in such cases fixed-clock placeholders or empty > phandles. Maybe something like that would work here as well? IMO, we should move away from doing that for clocks. It's a guaranteed ABI break. Rob
Hi All, On Tue, 3 May 2022 11:37:31 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Krzysztof, > > On Tue, May 3, 2022 at 11:29 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > On 03/05/2022 08:51, Geert Uytterhoeven wrote: > > >>>> This should not be a reason why a property is or is not required. Either > > >>>> this is required for device operation or not. If it is required, should > > >>>> be in the bindings. Otherwise what are you going to do in the future? > > >>>> Add a required property breaking the ABI? > > >>> > > >>> The problem is that there are no bindings for the reset controller > > >>> (actually the reset controller feature of the system-controller) yet. > > >>> Yeah, we can just add #reset-cells = <1> to the system-controller > > >>> device node, but we cannot add the actual resets properties to the > > >>> consumers, until the actual cell values are defined. > > >> > > >> Sounds like you should implement providers first. Or just live with the > > >> warning as a reminder to implement the reset provider? > > > > > > I'd go for the latter. The upstream r9a06g032.dtsi is still under active > > > development. Until very recently, the only device supported was the > > > serial console. > > > > For clocks we use in such cases fixed-clock placeholders or empty > > phandles. Maybe something like that would work here as well? > > I don't think that works for resets. > Besides, the driver doesn't need or use the reset anyway. > Finally, related to the "resets" property, what should I do ? (a) Keep the property as not required an change the commit log (b) Set the property as required and live with a warning (Rob's suggestion) Regards, Hervé
Hi Krzysztof, On Sun, 1 May 2022 10:51:43 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: [...] > > resets: > > maxItems: 1 > > @@ -106,13 +106,45 @@ required: > > - interrupt-map > > - interrupt-map-mask > > - clocks > > - - resets > > - power-domains > > - bus-range > > - "#address-cells" > > - "#size-cells" > > - "#interrupt-cells" > > > > +if: > > allOf. > > > + properties: > > + compatible: > > + contains: > > + enum: I Have an issue with this allOf. The yaml has the following structure and so has 2 AllOf: ... allOf: - $ref: /schemas/pci/pci-bus.yaml# properties: compatible: ... allOf: - if: properties: compatible: contains: ... make dt_binding_check failed with the following error: $ make dt_binding_check DT_SCHEMA_FILES=renesas,pci-rcar-gen2.yaml LINT Documentation/devicetree/bindings ./Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml:115:1: [error] duplication of key "allOf" in mapping (key-duplicates) CHKDT Documentation/devicetree/bindings/processed-schema.json Traceback (most recent call last): File "/home/hcodina/.local/bin/dt-doc-validate", line 25, in check_doc testtree = dtschema.load(filename, line_number=line_number) File "/home/hcodina/.local/lib/python3.10/site-packages/dtschema/lib.py", line 912, in load return yaml.load(f.read()) File "/home/hcodina/.local/lib/python3.10/site-packages/ruamel/yaml/main.py", line 434, in load return constructor.get_single_data() File "/home/hcodina/.local/lib/python3.10/site-packages/ruamel/yaml/constructor.py", line 121, in get_single_data return self.construct_document(node) File "/home/hcodina/.local/lib/python3.10/site-packages/ruamel/yaml/constructor.py", line 131, in construct_document for _dummy in generator: File "/home/hcodina/.local/lib/python3.10/site-packages/ruamel/yaml/constructor.py", line 674, in construct_yaml_map value = self.construct_mapping(node) File "/home/hcodina/.local/lib/python3.10/site-packages/ruamel/yaml/constructor.py", line 445, in construct_mapping return BaseConstructor.construct_mapping(self, node, deep=deep) File "/home/hcodina/.local/lib/python3.10/site-packages/ruamel/yaml/constructor.py", line 263, in construct_mapping if self.check_mapping_key(node, key_node, mapping, key, value): File "/home/hcodina/.local/lib/python3.10/site-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 4, column 1 found duplicate key "allOf" with value "[]" (original value: "[]") in "<unicode string>", line 115, column 1 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 "/home/hcodina/.local/bin/dt-doc-validate", line 74, in <module> ret = check_doc(f) File "/home/hcodina/.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' SCHEMA Documentation/devicetree/bindings/processed-schema.json /home/hcodina/project/xxxx/dev/linux/upstream_usb_host/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml: ignoring, error parsing file DTEX Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.example.dts Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml: found duplicate key "allOf" with value "[]" (original value: "[]") make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.example.dts] Error 1 make[1]: *** Deleting file 'Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.example.dts' make: *** [Makefile:1401: dt_binding_check] Error 2 [hcodina@localhost upstream_usb_host]$ [hcodina@localhost upstream_usb_host]$ make ARCH=arm CROSS_COMPILE=/home/hcodina/toolchain/gcc-linaro-7.5.0-2019.12-x86_64_arm-linux-gnueabihf/bin/arm-linux-gnueabihf- dt_binding_check DT_SCHEMA_FILES=renesas,pci-rcar-gen2.yaml DTEX Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.example.dts Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml: found duplicate key "allOf" with value "[]" (original value: "[]") make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.example.dts] Error 1 make[1]: *** Deleting file 'Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.example.dts' make: *** [Makefile:1401: dt_binding_check] Error 2 Is having a 'allOf' for schemas inclusion and a 'allOf' for conditionnal parts allowed ? Regards, Hervé
Hi Hervé, On Fri, May 20, 2022 at 10:23 AM Herve Codina <herve.codina@bootlin.com> wrote: > On Sun, 1 May 2022 10:51:43 +0200 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > [...] > > > resets: > > > maxItems: 1 > > > @@ -106,13 +106,45 @@ required: > > > - interrupt-map > > > - interrupt-map-mask > > > - clocks > > > - - resets > > > - power-domains > > > - bus-range > > > - "#address-cells" > > > - "#size-cells" > > > - "#interrupt-cells" > > > > > > +if: > > > > allOf. > > > > > + properties: > > > + compatible: > > > + contains: > > > + enum: > > I Have an issue with this allOf. > > The yaml has the following structure and so has 2 AllOf: > ... > allOf: > - $ref: /schemas/pci/pci-bus.yaml# > > properties: > compatible: > ... > allOf: > - if: > properties: > compatible: > contains: > ... > Is having a 'allOf' for schemas inclusion and a 'allOf' for conditionnal > parts allowed ? Just combine them into a single "allOf". See e.g. Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml. {oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 20/05/2022 10:23, Herve Codina wrote: > The yaml has the following structure and so has 2 AllOf: > ... > allOf: > - $ref: /schemas/pci/pci-bus.yaml# > > properties: > compatible: > ... > allOf: > - if: (...) > > Is having a 'allOf' for schemas inclusion and a 'allOf' for conditionnal > parts allowed ? Only one allOf for all of such (ref + if), located before additionalProperties: https://elixir.bootlin.com/linux/v5.18-rc7/source/Documentation/devicetree/bindings/example-schema.yaml#L211 Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml b/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml index 494eb975c146..a9f806794f12 100644 --- a/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml +++ b/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml @@ -32,6 +32,10 @@ properties: - renesas,pci-r8a7793 # R-Car M2-N - renesas,pci-r8a7794 # R-Car E2 - const: renesas,pci-rcar-gen2 # R-Car Gen2 and RZ/G1 + - items: + - enum: + - renesas,pci-r9a06g032 # RZ/N1D + - const: renesas,pci-rzn1 # RZ/N1 reg: items: @@ -41,13 +45,9 @@ properties: interrupts: maxItems: 1 - clocks: - items: - - description: Device clock + clocks: true - clock-names: - items: - - const: pclk + clock-names: true resets: maxItems: 1 @@ -106,13 +106,45 @@ required: - interrupt-map - interrupt-map-mask - clocks - - resets - power-domains - bus-range - "#address-cells" - "#size-cells" - "#interrupt-cells" +if: + properties: + compatible: + contains: + enum: + - renesas,pci-rzn1 + +then: + properties: + clocks: + items: + - description: Internal bus clock (AHB) for HOST + - description: Internal bus clock (AHB) Power Management + - description: PCI clock for USB subsystem + clock-names: + items: + - const: hclkh + - const: hclkpm + - const: pciclk + required: + - clock-names + +else: + properties: + clocks: + items: + - description: Device clock + clock-names: + items: + - const: pclk + required: + - resets + unevaluatedProperties: false examples: