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.
Signed-off-by: Herve Codina <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
.../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:
+ 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:
--
2.35.1
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 <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> .../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
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
> <[email protected]> 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 Krzysztof,
On Sun, May 1, 2022 at 10:51 AM Krzysztof Kozlowski
<[email protected]> 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 <[email protected]>
> > Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Rob,
On Mon, May 2, 2022 at 9:44 PM Rob Herring <[email protected]> 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
> > <[email protected]> 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 -- [email protected]
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
Hi Krzysztof,
On Tue, May 3, 2022 at 11:29 AM Krzysztof Kozlowski
<[email protected]> 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 -- [email protected]
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
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 <[email protected]> wrote:
> Hi Krzysztof,
>
> On Tue, May 3, 2022 at 11:29 AM Krzysztof Kozlowski
> <[email protected]> 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é
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Hervé,
On Fri, May 20, 2022 at 10:23 AM Herve Codina <[email protected]> wrote:
> On Sun, 1 May 2022 10:51:43 +0200
> Krzysztof Kozlowski <[email protected]> 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 -- [email protected]
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
Hi Krzysztof,
On Sun, 1 May 2022 10:51:43 +0200
Krzysztof Kozlowski <[email protected]> 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é
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