| Message ID | 20211118132152.15722-3-zajec5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: allow storing pins, groups & functions in DT | expand |
On Thu, 18 Nov 2021 14:21:49 +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > pinctrl bindings allow specifying pins, groups & functions now. Put some > entries in binding example to help writing DTS files. > > Specify pins, groups & functions in the example. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../bindings/pinctrl/brcm,ns-pinmux.yaml | 29 ++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: schemas/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml: 'additionalProperties' is a required property hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties" from schema $id: http://devicetree.org/meta-schemas/base.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: warning: no schema found in file: ./Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.example.dt.yaml:0:0: /example-0/pin-controller@1800c1c0: failed to match any schema with compatible: ['brcm,bcm4708-pinmux'] make[1]: *** Deleting file 'Documentation/devicetree/bindings/mfd/brcm,cru.example.dt.yaml' schemas/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: Traceback (most recent call last): File "/usr/local/bin/dt-validate", line 170, in <module> sg.check_trees(filename, testtree) File "/usr/local/bin/dt-validate", line 119, in check_trees self.check_subtree(dt, subtree, False, "/", "/", filename) File "/usr/local/bin/dt-validate", line 110, in check_subtree self.check_subtree(tree, value, disabled, name, fullname + name, filename) File "/usr/local/bin/dt-validate", line 110, in check_subtree self.check_subtree(tree, value, disabled, name, fullname + name, filename) File "/usr/local/bin/dt-validate", line 105, in check_subtree self.check_node(tree, subtree, disabled, nodename, fullname, filename) File "/usr/local/bin/dt-validate", line 49, in check_node errors = sorted(dtschema.DTValidator(schema).iter_errors(node), key=lambda e: e.linecol) File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 766, in iter_errors for error in super().iter_errors(instance, _schema): File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 224, in iter_errors for error in errors: File "/usr/local/lib/python3.8/dist-packages/jsonschema/_validators.py", line 25, in patternProperties yield from validator.descend( File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 240, in descend for error in self.evolve(schema=schema).iter_errors(instance): File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 766, in iter_errors for error in super().iter_errors(instance, _schema): File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 224, in iter_errors for error in errors: File "/usr/local/lib/python3.8/dist-packages/jsonschema/_validators.py", line 298, in ref yield from validator.descend(instance, resolved) File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 240, in descend for error in self.evolve(schema=schema).iter_errors(instance): File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 766, in iter_errors for error in super().iter_errors(instance, _schema): File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 214, in iter_errors scope = id_of(_schema) File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 90, in _id_of return schema.get("$id", "") AttributeError: 'NoneType' object has no attribute 'get' make[1]: *** [scripts/Makefile.lib:373: Documentation/devicetree/bindings/mfd/brcm,cru.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1413: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1556628 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On 18.11.2021 23:09, Rob Herring wrote: > schemas/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml: 'additionalProperties' is a required property > hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties" > from schema $id: http://devicetree.org/meta-schemas/base.yaml# I forgot to mention this patch is based on top of the [PATCH 2/2] dt-bindings: pinctrl: use pinctrl.yaml https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211110165720.30242-2-zajec5@gmail.com/
Hi, 200* Rafał Miłecki <zajec5@gmail.com> [211118 13:30]: > @@ -83,6 +83,33 @@ examples: > reg = <0x1800c1c0 0x24>; > reg-names = "cru_gpio_control"; > > + pins { > + #address-cells = <1>; > + #size-cells = <0>; > + > + pin@4 { > + reg = <4>; > + label = "i2c_scl"; > + }; > + > + pin@5 { > + reg = <5>; > + label = "i2c_sda"; > + }; > + }; The reg property should indicate the hardware offset from the device base address. The reg values above for 4 and 5 seem to be indexed instead :) Please update to use real register offsets from the 0x1800c1c0 base instead. If a reg offset + bit offset are needed, the #address-cells or #pinctrl-cells can be used. The main problem using an index is that you need to keep it in sync between the dts and device driver. And if a new SoC variant adds an entry between the registers, you end up having to renumber the index. Regards, Tony
On 23.11.2021 08:38, Tony Lindgren wrote: > 200* Rafał Miłecki <zajec5@gmail.com> [211118 13:30]: >> @@ -83,6 +83,33 @@ examples: >> reg = <0x1800c1c0 0x24>; >> reg-names = "cru_gpio_control"; >> >> + pins { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + pin@4 { >> + reg = <4>; >> + label = "i2c_scl"; >> + }; >> + >> + pin@5 { >> + reg = <5>; >> + label = "i2c_sda"; >> + }; >> + }; > > The reg property should indicate the hardware offset from the device base > address. The reg values above for 4 and 5 seem to be indexed instead :) > Please update to use real register offsets from the 0x1800c1c0 base > instead. If a reg offset + bit offset are needed, the #address-cells or > #pinctrl-cells can be used. It's true those are "reg"s are PINS indexes and not actual hw registers. That concept of changing "reg" context is nothing new here however. It's used in many other places. Some examples: 1. net/mdio-mux.yaml "reg" is used for "The sub-bus number" (not actual hw register) 2. usb/usb-device.yaml "reg" is used for USB port index on USB hub 3. spi/spi-controller.yaml "reg" is used for "Chip select used by the device." 4. mtd/partitions/partition.yaml "reg" is used for "partition's offset and size within the flash" Does it mean above "reg" usages are all incorrect and binding "reg" in such way is deprecated? This is something totally new to me, can you confirm that, please? > The main problem using an index is that you need to keep it in sync > between the dts and device driver. And if a new SoC variant adds an entry > between the registers, you end up having to renumber the index. I don't understand that part. That index ("reg" value in above example) is actual PIN number. It's expected to match hw datasheets. Order doesn't matter there. If new hw revision adds PIN 2, I could just add pin@2 { ... };
On 23.11.2021 08:56, Rafał Miłecki wrote: > On 23.11.2021 08:38, Tony Lindgren wrote: >> The main problem using an index is that you need to keep it in sync >> between the dts and device driver. And if a new SoC variant adds an entry >> between the registers, you end up having to renumber the index. > > I don't understand that part. That index ("reg" value in above example) > is actual PIN number. It's expected to match hw datasheets. Order > doesn't matter there. > > If new hw revision adds PIN 2, I could just add pin@2 { ... }; Actually if you check [PATCH 5/5] ARM: dts: BCM5301X: add pinctrl pins, groups & functions you can see that first Northstar SoCs (BCM4708, BCM47081) didn't have PINs 6, 7, 16, 17, 22 and 23 exposed. I define them only for later-released Northstar SoC BCM4709 (bcm4709.dtsi). That works just fine on my hw here.
* Rafał Miłecki <zajec5@gmail.com> [211123 07:56]: > Does it mean above "reg" usages are all incorrect and binding "reg" in > such way is deprecated? This is something totally new to me, can you > confirm that, please? Here you have a device with multiple control register instances at various register offsets. Using index here makes as much sense as the old interrupt number defines we used to have but got rid of. Please don't use an index to address them. Index makes sense when there is no real offset to use, like a SPI chip select, or a bit offset inside the register like a GPIO instance bit. Regards, Tony
On 23.11.2021 10:15, Tony Lindgren wrote: > * Rafał Miłecki <zajec5@gmail.com> [211123 07:56]: >> Does it mean above "reg" usages are all incorrect and binding "reg" in >> such way is deprecated? This is something totally new to me, can you >> confirm that, please? > > Here you have a device with multiple control register instances at > various register offsets. Using index here makes as much sense as > the old interrupt number defines we used to have but got rid of. > > Please don't use an index to address them. Index makes sense when > there is no real offset to use, like a SPI chip select, or a bit > offset inside the register like a GPIO instance bit. I think I'll simply trust you on this as there seems to be some thin line I can't really see. It may be however worth documenting somehwere what's the rule for changing "reg" context. So that in future less experienced developers (like me) don't bother maintainers with such bad concepts. What I understood from your e-mail is that it's a matter of "reg" usage in a hardware block binding. If reg contains "multiple control register instances" (I understand it as reg space size > 0x4) then children nodes may use "reg" only as address in that register space. My above understanding doesn't fit however what I see in various controllers. ***** Example 1: usb/generic-xhci.yaml / usb/usb-xhci.yaml That block binding covers *multiple* registers, see: reg = <0xf0931000 0x8c8>; However its children nodes are indexed and use "reg", see: hub@1 { compatible = "usb5e3,610"; reg = <1>; }; ***** Example 2: spi/spi-controller.yaml That binding uses "fsl,imx28-spi" as example. Its binding covers *multiple* registers, see: reg = <0x80010000 0x2000>; However its children nodes are indexed and use "reg", see: display@0 { compatible = "lg,lg4573"; spi-max-frequency = <1000000>; reg = <0>; }; ***** So it appears my understanding is wrong somewhere. It seems to be a bit tricky to get things right so I'd really appreciate some documentation on that.
diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml index 8d1e5b1cdd5f..154119981ad9 100644 --- a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml +++ b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml @@ -74,7 +74,7 @@ required: - reg - reg-names -additionalProperties: false +unevaluatedProperties: false examples: - | @@ -83,6 +83,33 @@ examples: reg = <0x1800c1c0 0x24>; reg-names = "cru_gpio_control"; + pins { + #address-cells = <1>; + #size-cells = <0>; + + pin@4 { + reg = <4>; + label = "i2c_scl"; + }; + + pin@5 { + reg = <5>; + label = "i2c_sda"; + }; + }; + + groups { + i2c_grp: i2c_grp { + pins = <4 5>; + }; + }; + + functions { + i2c { + groups = <&i2c_grp>; + }; + }; + spi-pins { function = "spi"; groups = "spi_grp";