| Message ID | a5c1eef7-372d-082b-066e-ecd5e001d1cf@digi.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] pinctrl: ls1012a: Add pinctrl driver support | expand |
On Tue, 27 Aug 2024 12:10:44 +1000, David Leonard wrote: > > Add a binding schema and examples for the LS1012A's pinctrl function. > > Signed-off-by: David Leonard <David.Leonard@digi.com> > --- > .../bindings/pinctrl/fsl,ls1012a-pinctrl.yaml | 83 +++++++++++++++++++ > 1 file changed, 83 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.example.dtb: quadspi@1550000: $nodename:0: 'quadspi@1550000' does not match '^spi(@.*|-([0-9]|[1-9][0-9]+))?$' from schema $id: http://devicetree.org/schemas/spi/fsl,spi-fsl-qspi.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.example.dtb: quadspi@1550000: 'reg-names' is a required property from schema $id: http://devicetree.org/schemas/spi/fsl,spi-fsl-qspi.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.example.dtb: quadspi@1550000: 'clocks' is a required property from schema $id: http://devicetree.org/schemas/spi/fsl,spi-fsl-qspi.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.example.dtb: quadspi@1550000: 'clock-names' is a required property from schema $id: http://devicetree.org/schemas/spi/fsl,spi-fsl-qspi.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.example.dtb: quadspi@1550000: 'oneOf' conditional failed, one must be fixed: 'interrupts' is a required property 'interrupts-extended' is a required property from schema $id: http://devicetree.org/schemas/spi/fsl,spi-fsl-qspi.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/a5c1eef7-372d-082b-066e-ecd5e001d1cf@digi.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. 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 after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Tue, Aug 27, 2024 at 12:10:44PM +1000, David Leonard wrote: > > Add a binding schema and examples for the LS1012A's pinctrl function. > > Signed-off-by: David Leonard <David.Leonard@digi.com> > --- It does not look like you tested the bindings, at least after quick look. Please run (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. > .../bindings/pinctrl/fsl,ls1012a-pinctrl.yaml | 83 +++++++++++++++++++ > 1 file changed, 83 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml > new file mode 100644 > index 000000000000..599df49b44d4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml > @@ -0,0 +1,83 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/fsl,ls1012a-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NXP QorIQ LS1012A pin multiplexing > + > +maintainers: > + - David.Leonard@digi.com > + > +description: > Drop > > + Bindings for LS1012A pinmux control. Drop "Bindings for" and explain the hardware. > + > +properties: > + compatible: > + const: fsl,ls1012a-pinctrl > + > + reg: > + description: Specifies the base address of the PMUXCR0 register. > + maxItems: 2 Instead list and describe the items. > + > + big-endian: > + description: If present, the PMUXCR0 register is implemented in big-endian. Why is this here? Either it is or it is not? > + type: boolean > + > + dcfg-regmap: Missing vendor prefix. > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + The phandle of the syscon node for the DCFG registers. Instead explain what it is needed it for, how is it used. > + > +patternProperties: > + '^pinctrl-': Rather -pins$ or ^pins- > + type: object > + $ref: pinmux-node.yaml# > + unevaluatedProperties: false > + > + properties: > + function: > + enum: [ i2c, spi, gpio, gpio_reset ] > + > + groups: > + items: > + enum: [ qspi_1_grp, qspi_2_grp, qspi_3_grp ] > + > +allOf: > + - $ref: pinctrl.yaml# Thies goes after required. > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + pinctrl: pinctrl@1570430 { > + compatible = "fsl,ls1012a-pinctrl"; > + reg = <0x0 0x1570430 0x0 0x4>; > + big-endian; > + dcfg-regmap = <&dcfg>; > + pinctrl_qspi_1: pinctrl-qspi-1 { > + groups = "qspi_1_grp"; > + function = "spi"; > + }; > + pinctrl_qspi_2: pinctrl-qspi-2 { > + groups = "qspi_1_grp", "qspi_2_grp"; > + function = "spi"; > + }; > + pinctrl_qspi_4: pinctrl-qspi-4 { > + groups = "qspi_1_grp", "qspi_2_grp", "qspi_3_grp"; > + function = "spi"; > + }; > + }; > + - | > + qspi: quadspi@1550000 { Drop, useless and not related. Best regards, Krzysztof
On Tue, 27 Aug 2024, Krzysztof Kozlowski wrote: > On Tue, Aug 27, 2024 at 12:10:44PM +1000, David Leonard wrote: >> >> Add a binding schema and examples for the LS1012A's pinctrl function. >> >> Signed-off-by: David Leonard <David.Leonard@digi.com> >> --- > > It does not look like you tested the bindings, at least after quick > look. Please run (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > Maybe you need to update your dtschema and yamllint. I've since updated tools (dt-valdate 2024.5 and yamllint 1.33.0) and rebased onto v6.11-rc1. >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml >> @@ -0,0 +1,83 @@ >> +description: > > > Drop > > >> + Bindings for LS1012A pinmux control. > > Drop "Bindings for" and explain the hardware. Changed to description: The LS1012A pinmux controller can override the RCW and alter some pin groups' functions in a limited way. >> + >> +properties: >> + compatible: >> + const: fsl,ls1012a-pinctrl >> + >> + reg: >> + description: Specifies the base address of the PMUXCR0 register. >> + maxItems: 2 > > Instead list and describe the items. Changed to reg: items: - description: Physical base address of the PMUXCR0 register. - description: Size of the PMUXCR0 register (4). Is this what you meant? >> + >> + big-endian: >> + description: If present, the PMUXCR0 register is implemented in big-endian. > > Why is this here? Either it is or it is not? You're right. Changed to big-endian: true (This also lead to some code simplification) >> + type: boolean >> + >> + dcfg-regmap: > > Missing vendor prefix. Will use "fsl," >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + The phandle of the syscon node for the DCFG registers. > > Instead explain what it is needed it for, how is it used. Changed to fsl,dcfg-regmap: $ref: /schemas/types.yaml#/definitions/phandle description: The phandle of the syscon node for the DCFG registers providing the RCW's pin configuration that is in effect when the pinmux controller is disabled. >> + >> +patternProperties: >> + '^pinctrl-': > > Rather -pins$ or ^pins- Changed to ^pins- See example at the end. >> +allOf: >> + - $ref: pinctrl.yaml# > > Thies goes after required. > >> + >> +required: >> + - compatible >> + - reg Relocated >> +additionalProperties: false >> + >> +examples: >> + - | >> + pinctrl: pinctrl@1570430 { >> + compatible = "fsl,ls1012a-pinctrl"; >> + reg = <0x0 0x1570430 0x0 0x4>; >> + big-endian; >> + dcfg-regmap = <&dcfg>; >> + pinctrl_qspi_1: pinctrl-qspi-1 { >> + groups = "qspi_1_grp"; >> + function = "spi"; >> + }; >> + pinctrl_qspi_2: pinctrl-qspi-2 { >> + groups = "qspi_1_grp", "qspi_2_grp"; >> + function = "spi"; >> + }; >> + pinctrl_qspi_4: pinctrl-qspi-4 { >> + groups = "qspi_1_grp", "qspi_2_grp", "qspi_3_grp"; >> + function = "spi"; >> + }; >> + }; >> + - | >> + qspi: quadspi@1550000 { > > Drop, useless and not related. Dropped the qspi example. Examples are now examples: - | pinctrl: pinctrl@1570430 { compatible = "fsl,ls1012a-pinctrl"; reg = <0x0 0x1570430 0x0 0x4>; big-endian; fsl,dcfg-regmap = <&dcfg>; pinctrl_qspi_1: pins-qspi-1 { /* buswidth 1 */ groups = "qspi_1_grp"; function = "spi"; }; pinctrl_qspi_2: pins-qspi-2 { /* buswidth 2 */ groups = "qspi_1_grp", "qspi_2_grp"; function = "spi"; }; pinctrl_qspi_4: pins-qspi-4 { /* buswidth 4 */ groups = "qspi_1_grp", "qspi_2_grp", "qspi_3_grp"; function = "spi"; }; }; Thanks, David
On 27/08/2024 18:51, David Leonard wrote: >>> +properties: >>> + compatible: >>> + const: fsl,ls1012a-pinctrl >>> + >>> + reg: >>> + description: Specifies the base address of the PMUXCR0 register. >>> + maxItems: 2 >> >> Instead list and describe the items. > > Changed to > > reg: > items: > - description: Physical base address of the PMUXCR0 register. > - description: Size of the PMUXCR0 register (4). > > Is this what you meant? Almost, second reg is not a size. You claim there are two IO address spaces. Each address space contains base address and size. Look at other bindings how they do it. > >>> + >>> + big-endian: >>> + description: If present, the PMUXCR0 register is implemented in big-endian. >> >> Why is this here? Either it is or it is not? > > You're right. Changed to > > big-endian: true > > (This also lead to some code simplification) OK, but I still wonder why is it here. Without it the hardware will work in little-endian? > Best regards, Krzysztof
On Tue, 27 Aug 2024, Krzysztof Kozlowski wrote: > On 27/08/2024 18:51, David Leonard wrote: >>>> +properties: >>>> + compatible: >>>> + const: fsl,ls1012a-pinctrl >>>> + >>>> + reg: >>>> + description: Specifies the base address of the PMUXCR0 register. >>>> + maxItems: 2 >>> >>> Instead list and describe the items. >> >> Changed to >> >> reg: >> items: >> - description: Physical base address of the PMUXCR0 register. >> - description: Size of the PMUXCR0 register (4). >> >> Is this what you meant? > > Almost, second reg is not a size. You claim there are two IO address > spaces. Each address space contains base address and size. Look at other > bindings how they do it. Previously, dt_binding_check was giving me a warning that 'maxItems' needed to be supplied. Which confused me because I thought of reg as an opaque subspace descriptor, but I was just trying to placate the checks. After tool upgrades, that warning doesn't appear any more. So the neatest documentation would be: reg: description: Specifies the address of the PMUXCR0 register. >>>> + big-endian: >>>> + description: If present, the PMUXCR0 register is implemented in big-endian. >>> >>> Why is this here? Either it is or it is not? >> >> You're right. Changed to >> >> big-endian: true >> >> (This also lead to some code simplification) > > OK, but I still wonder why is it here. Without it the hardware will work > in little-endian? Well, it's here firstly because I was trying to follow a perceived convention in dts/freescale/fsl,ls1012a.dtsi. That DT uses big-endian in child nodes of /soc that match up with memory map tables from the datasheet. (Not only do different and adjacent parts of the register map have opposing endianess, some register layouts also seem to be reversed bitwise, others bytewise.) The second reason is practical/dodgy. The pinctrl driver should logically be a child of the scfg node, but the syscon driver doesn't populate its child nodes. To get the pinctrl driver to work meant making it a sibling node with an unsatisfactory overlap with the scfg's address region 0x1570000+0x10000. (No driver binds to "fsl,ls1012a-scfg".) soc: soc { compatible = "simple-bus"; #address-cells = <2>; #size-cells = <2>; ... scfg: scfg@1570000 { compatible = "fsl,ls1012a-scfg", "syscon"; reg = <0x0 0x1570000 0x0 0x10000>; big-endian; }; pinmux: pinmux@157040c { compatible = "fsl,ls1046a-pinctrl"; reg = <0 0x157040c 0 4>; big-endian; ... }; }; The better device tree would be: soc: soc { compatible = "simple-bus"; ... scfg: scfg@1570000 { compatible = "fsl,ls1046a-scfg", "syscon"; big-endian; #address-cells = <1>; #size-cells = <1>; ... pinmux: pinmux@40c { compatible = "fsl,ls1046a-pinctrl"; reg = <0x40c 4>; ... }; }; }; And this would resolve the big-endian property issue. There was a discussion of syscon populating its child nodes at https://lore.kernel.org/lkml/1403513950.4136.34.camel@paszta.hi.pengutronix.de/T/ Cheers, David
On Wed, 28 Aug 2024, David Leonard wrote: >> OK, but I still wonder why is it here. Without it the hardware will work >> in little-endian? > > Well, it's here firstly because I was trying to follow a perceived convention > in > dts/freescale/fsl,ls1012a.dtsi. That DT uses big-endian in child > nodes of /soc that match up with memory map tables from the datasheet. > (Not only do different and adjacent parts of the register map have > opposing endianess, some register layouts also seem to be reversed > bitwise, others bytewise.) > > The second reason is practical/dodgy. The pinctrl driver should logically > be a child of the scfg node, but the syscon driver doesn't populate its > child nodes. To get the pinctrl driver to work meant making it a sibling > node with an unsatisfactory overlap with the scfg's address region > 0x1570000+0x10000. (No driver binds to "fsl,ls1012a-scfg".) Sorry, my examples had mixed up some ls1046a with ls1012a, which must be confusing. Corrections follow, should the meaning have been lost: soc { compatible = "simple-bus"; #address-cells = <2>; #size-cells = <2>; ... scfg: scfg@1570000 { compatible = "fsl,ls1012a-scfg", "syscon"; reg = <0x0 0x1570000 0x0 0x10000>; big-endian; }; pinctrl: pinctrl@1570430 { compatible = "fsl,ls1012a-pinctrl"; reg = <0x0 0x1570430 0x0 0x4>; /* SCFG PMUXCR0 */ big-endian; fsl,dcfg-regmap = <&dcfg>; ... }; }; > The better device tree would be: soc { compatible = "simple-bus"; #address-cells = <2>; #size-cells = <2>; ... scfg: scfg@1570000 { compatible = "fsl,ls1012a-scfg", "syscon"; reg = <0x0 0x1570000 0x0 0x10000>; big-endian; #address-cells = <1>; #size-cells = <1>; ... pinctrl: pinctrl@430 { compatible = "fsl,ls1012a-pinctrl"; reg = <0x430 0x4>; /* SCFG PMUXCR0 */ fsl,dcfg-regmap = <&dcfg (416/8)>; ... }; }; }; > And this would resolve the big-endian property issue. > > There was a discussion of syscon populating its child nodes at > https://lore.kernel.org/lkml/1403513950.4136.34.camel@paszta.hi.pengutronix.de/T/
On 28/08/2024 02:43, David Leonard wrote: > On Tue, 27 Aug 2024, Krzysztof Kozlowski wrote: > >> On 27/08/2024 18:51, David Leonard wrote: >>>>> +properties: >>>>> + compatible: >>>>> + const: fsl,ls1012a-pinctrl >>>>> + >>>>> + reg: >>>>> + description: Specifies the base address of the PMUXCR0 register. >>>>> + maxItems: 2 >>>> >>>> Instead list and describe the items. >>> >>> Changed to >>> >>> reg: >>> items: >>> - description: Physical base address of the PMUXCR0 register. >>> - description: Size of the PMUXCR0 register (4). >>> >>> Is this what you meant? >> >> Almost, second reg is not a size. You claim there are two IO address >> spaces. Each address space contains base address and size. Look at other >> bindings how they do it. > > Previously, dt_binding_check was giving me a warning that 'maxItems' needed > to be supplied. Which confused me because I thought of reg as an opaque subspace No. > descriptor, but I was just trying to placate the checks. After tool upgrades, > that warning doesn't appear any more. > > So the neatest documentation would be: > > reg: > description: Specifies the address of the PMUXCR0 register. No. Please go through example-schema. Or any other binding... If you find any binding with above syntax, let me know. Instead: maxItems: 1. > >>>>> + big-endian: >>>>> + description: If present, the PMUXCR0 register is implemented in big-endian. >>>> >>>> Why is this here? Either it is or it is not? >>> >>> You're right. Changed to >>> >>> big-endian: true >>> >>> (This also lead to some code simplification) >> >> OK, but I still wonder why is it here. Without it the hardware will work >> in little-endian? > > Well, it's here firstly because I was trying to follow a perceived convention in > dts/freescale/fsl,ls1012a.dtsi. That DT uses big-endian in child I am confused. Are you adding new device or converting existing bindings? > nodes of /soc that match up with memory map tables from the datasheet. > (Not only do different and adjacent parts of the register map have > opposing endianess, some register layouts also seem to be reversed > bitwise, others bytewise.) > > The second reason is practical/dodgy. The pinctrl driver should logically > be a child of the scfg node, but the syscon driver doesn't populate its > child nodes. To get the pinctrl driver to work meant making it a sibling So fix driver... > node with an unsatisfactory overlap with the scfg's address region > 0x1570000+0x10000. (No driver binds to "fsl,ls1012a-scfg".) > Several big-endian properties were added unnecessarily and are now being removed. You cannot provide me answer whether this hardware is little/big endian, so it also feels unnecessary. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml new file mode 100644 index 000000000000..599df49b44d4 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml @@ -0,0 +1,83 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/fsl,ls1012a-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP QorIQ LS1012A pin multiplexing + +maintainers: + - David.Leonard@digi.com + +description: > + Bindings for LS1012A pinmux control. + +properties: + compatible: + const: fsl,ls1012a-pinctrl + + reg: + description: Specifies the base address of the PMUXCR0 register. + maxItems: 2 + + big-endian: + description: If present, the PMUXCR0 register is implemented in big-endian. + type: boolean + + dcfg-regmap: + $ref: /schemas/types.yaml#/definitions/phandle + description: + The phandle of the syscon node for the DCFG registers. + +patternProperties: + '^pinctrl-': + type: object + $ref: pinmux-node.yaml# + unevaluatedProperties: false + + properties: + function: + enum: [ i2c, spi, gpio, gpio_reset ] + + groups: + items: + enum: [ qspi_1_grp, qspi_2_grp, qspi_3_grp ] + +allOf: + - $ref: pinctrl.yaml# + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + pinctrl: pinctrl@1570430 { + compatible = "fsl,ls1012a-pinctrl"; + reg = <0x0 0x1570430 0x0 0x4>; + big-endian; + dcfg-regmap = <&dcfg>; + pinctrl_qspi_1: pinctrl-qspi-1 { + groups = "qspi_1_grp"; + function = "spi"; + }; + pinctrl_qspi_2: pinctrl-qspi-2 { + groups = "qspi_1_grp", "qspi_2_grp"; + function = "spi"; + }; + pinctrl_qspi_4: pinctrl-qspi-4 { + groups = "qspi_1_grp", "qspi_2_grp", "qspi_3_grp"; + function = "spi"; + }; + }; + - | + qspi: quadspi@1550000 { + compatible = "fsl,ls1021a-qspi"; + reg = <0 0x1550000 0 0x10000>; + /* QSPI pins for buswidth 2 */ + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_qspi_2>; + status = "okay"; + };
Add a binding schema and examples for the LS1012A's pinctrl function. Signed-off-by: David Leonard <David.Leonard@digi.com> --- .../bindings/pinctrl/fsl,ls1012a-pinctrl.yaml | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml