| Message ID | 20231011090510.114476-3-ychuang570808@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for nuvoton ma35d1 pin control | expand |
On 11/10/2023 11:05, Jacky Huang wrote: > From: Jacky Huang <ychuang3@nuvoton.com> > > Add the dt-bindings header for nuvoton ma35d1 pinctrl, that gets shared > between the pin control driver and pin configuration in the dts. > > Add documentation to describe nuvoton ma35d1 pin control and GPIO. > > Signed-off-by: Jacky Huang <ychuang3@nuvoton.com> > --- > .../pinctrl/nuvoton,ma35d1-pinctrl.yaml | 180 ++++++++++++++++++ > include/dt-bindings/pinctrl/ma35d1-pinfunc.h | 38 ++++ > 2 files changed, 218 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml > create mode 100644 include/dt-bindings/pinctrl/ma35d1-pinfunc.h > > diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml > new file mode 100644 > index 000000000000..0ddedbad4b78 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml > @@ -0,0 +1,180 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/nuvoton,ma35d1-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nuvoton MA35D1 pin control and GPIO > + > +maintainers: > + - Shan-Chun Hung <schung@nuvoton.com> > + > +properties: > + compatible: > + enum: > + - nuvoton,ma35d1-pinctrl > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 1 > + > + nuvoton,sys: > + description: > + phandle to the syscon node sys is quite generic. Description explains nothing except duplicating known information. Drop duplicated info and instead explain to what this phandle points and how it is going to be used. > + $ref: /schemas/types.yaml#/definitions/phandle-array > + items: > + maxItems: 1 So just phandle, not phandle-array, unless it is defined like this in some other binding. > + > + ranges: true > + > +allOf: > + - $ref: pinctrl.yaml# allOf: goes after required: block. > + > +patternProperties: > + "gpio[a-n]@[0-9a-f]+$": ^gpio@[0-9a-f]+$": > + type: object > + additionalProperties: false > + properties: > + Drop blank line > + gpio-controller: true > + > + '#gpio-cells': > + const: 2 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + interrupt-controller: true > + > + '#interrupt-cells': > + const: 2 > + > + interrupts: > + description: > + The interrupt outputs to sysirq. > + maxItems: 1 > + > + required: > + - reg > + - interrupts > + - interrupt-controller > + - '#interrupt-cells' > + - gpio-controller > + - '#gpio-cells' Keep the same order as in list of properties. > + > + "pcfg-[a-z0-9-.]+$": Why using different naming than other Nuvoton SoCs? You also accept "foobarpcfg-1", which does not look intentional. > + type: object > + description: > + A pinctrl node should contain at least one subnodes representing the > + pinctrl groups available on the machine. Each subnode will list the > + pins it needs, and how they should be configured, with regard to muxer > + configuration, pullups, drive strength, input enable/disable and input > + schmitt. > + > + allOf: > + - $ref: pincfg-node.yaml# missing additional/unevaluatedProperties: false. > + > + properties: > + bias-disable: true Why do you need this and other ones? > + > + bias-pull-down: true > + > + bias-pull-up: true > + > + drive-strength: > + minimum: 0 0 mA? Is it really valid? Are you sure you used correct property? > + maximum: 7 > + > + input-enable: true > + > + input-schmitt-enable: true > + > + power-source: > + description: > + I/O voltage in millivolt. > + enum: [ 1800, 3300 ] Missing units in property name. power-source also does not really describe the property. > + > +additionalProperties: > + type: object > + additionalProperties: > + type: object Wait, what? What are you describing here? > + properties: > + nuvoton,pin: > + description: > + Each entry consists of 4 parameters and represents the mux and config > + setting for one pin. > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > + minItems: 1 > + items: > + items: > + - minimum: 0x80 > + maximum: 0xec > + description: > + The pinctrl register offset in syscon registers. > + - minimum: 0 > + maximum: 30 > + description: > + The bit offset in the pinctrl register. > + - minimum: 0 > + maximum: 15 > + description: > + The multi-function pin value. > + - description: > + The phandle of a node contains the generic pinconfig options > + to use as described in pinctrl-bindings.txt. > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> > + #include <dt-bindings/pinctrl/ma35d1-pinfunc.h> > + > + pinctrl@40040000 { > + compatible = "nuvoton,ma35d1-pinctrl"; > + #address-cells = <1>; > + #size-cells = <1>; > + nuvoton,sys = <&sys>; > + ranges = <0 0x40040000 0xc00>; > + > + gpioa@40040000 { > + reg = <0x0 0x40>; > + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk GPA_GATE>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-controller; > + #interrupt-cells = <2>; > + }; > + > + pcfg_default: pcfg-default { > + slew-rate = <0>; > + input-schmitt-disable; > + bias-disable; > + power-source = <3300>; > + drive-strength = <0>; Really 0 mA? Why this is so incomplete? > + }; > + }; > + > + pinctrl {> + uart13 { > + pinctrl_uart13: uart13grp { According to your bindings this does not belong here. > + nuvoton,pins = > + <MA35_SYS_REG_GPH_H 24 2 &pcfg_default>, > + <MA35_SYS_REG_GPH_H 28 2 &pcfg_default>; > + }; > + }; > + }; > + > + serial@407d0000 { Drop node, not related at all. > + compatible = "nuvoton,ma35d1-uart"; > + reg = <0x407d0000 0x100>; > + interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk UART13_GATE>; > + pinctrl-0 = <&pinctrl_uart13>; > + }; > diff --git a/include/dt-bindings/pinctrl/ma35d1-pinfunc.h b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h > new file mode 100644 > index 000000000000..a2609d466dc9 > --- /dev/null > +++ b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h Filename matching bindings. The same name. > @@ -0,0 +1,38 @@ > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */ > +/* > + * Copyright (C) 2023 Nuvoton Technologies. > + */ > + > +#ifndef __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H > +#define __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H > + > +#define MA35_SYS_REG_GPA_L 0x80 Registry addresses are not suitable for bindings. There is also no need to have REG address in the binding. Drop entire file. Best regards, Krzysztof
Dear Krzysztof, Thank you for the review. On 2023/10/13 上午 03:41, Krzysztof Kozlowski wrote: > On 11/10/2023 11:05, Jacky Huang wrote: >> From: Jacky Huang<ychuang3@nuvoton.com> >> >> Add the dt-bindings header for nuvoton ma35d1 pinctrl, that gets shared >> between the pin control driver and pin configuration in the dts. >> >> Add documentation to describe nuvoton ma35d1 pin control and GPIO. >> >> Signed-off-by: Jacky Huang<ychuang3@nuvoton.com> >> --- >> .../pinctrl/nuvoton,ma35d1-pinctrl.yaml | 180 ++++++++++++++++++ >> include/dt-bindings/pinctrl/ma35d1-pinfunc.h | 38 ++++ >> 2 files changed, 218 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml >> create mode 100644 include/dt-bindings/pinctrl/ma35d1-pinfunc.h >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml >> new file mode 100644 >> index 000000000000..0ddedbad4b78 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml >> @@ -0,0 +1,180 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id:http://devicetree.org/schemas/pinctrl/nuvoton,ma35d1-pinctrl.yaml# >> +$schema:http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Nuvoton MA35D1 pin control and GPIO >> + >> +maintainers: >> + - Shan-Chun Hung<schung@nuvoton.com> >> + >> +properties: >> + compatible: >> + enum: >> + - nuvoton,ma35d1-pinctrl >> + >> + '#address-cells': >> + const: 1 >> + >> + '#size-cells': >> + const: 1 >> + >> + nuvoton,sys: >> + description: >> + phandle to the syscon node > sys is quite generic. Description explains nothing except duplicating > known information. Drop duplicated info and instead explain to what this > phandle points and how it is going to be used. > > >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + items: >> + maxItems: 1 > So just phandle, not phandle-array, unless it is defined like this in > some other binding. I would like to update this as: nuvoton,sys: $ref: /schemas/types.yaml#/definitions/phandle description: Help pinctrl driver to access system registers by means of regmap. >> + >> + ranges: true >> + >> +allOf: >> + - $ref: pinctrl.yaml# > allOf: goes after required: block. I will fix it. >> + >> +patternProperties: >> + "gpio[a-n]@[0-9a-f]+$": > ^gpio@[0-9a-f]+$": I will fix this, and also fix the dtsi. >> + type: object >> + additionalProperties: false >> + properties: >> + > Drop blank line I will fix it. >> + gpio-controller: true >> + >> + '#gpio-cells': >> + const: 2 >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + interrupt-controller: true >> + >> + '#interrupt-cells': >> + const: 2 >> + >> + interrupts: >> + description: >> + The interrupt outputs to sysirq. >> + maxItems: 1 >> + >> + required: >> + - reg >> + - interrupts >> + - interrupt-controller >> + - '#interrupt-cells' >> + - gpio-controller >> + - '#gpio-cells' > Keep the same order as in list of properties. I will fix the order. >> + >> + "pcfg-[a-z0-9-.]+$": > Why using different naming than other Nuvoton SoCs? You also accept > "foobarpcfg-1", which does not look intentional. > I will use '"^pin-[a-z0-9-.]+$" instead. >> + type: object >> + description: >> + A pinctrl node should contain at least one subnodes representing the >> + pinctrl groups available on the machine. Each subnode will list the >> + pins it needs, and how they should be configured, with regard to muxer >> + configuration, pullups, drive strength, input enable/disable and input >> + schmitt. >> + >> + allOf: >> + - $ref: pincfg-node.yaml# > missing additional/unevaluatedProperties: false. I will add unevaluatedProperties: false. >> + >> + properties: >> + bias-disable: true > Why do you need this and other ones? We expect the pin configuration to select one of ==> bias-disable; bias-pull-down; bias-pull-up; This is the same as rockchip,pinctrl.yaml and renesas,rzv2m-pinctrl.yaml. >> + >> + bias-pull-down: true >> + >> + bias-pull-up: true >> + >> + drive-strength: >> + minimum: 0 > 0 mA? Is it really valid? Are you sure you used correct property? We treat this value as the value to be written to the control register, not as a current value in mA. I will correct this mistake. >> + maximum: 7 >> + >> + input-enable: true >> + >> + input-schmitt-enable: true >> + >> + power-source: >> + description: >> + I/O voltage in millivolt. >> + enum: [ 1800, 3300 ] > Missing units in property name. power-source also does not really > describe the property. The output voltage level of GPIO can be configured as 1.8V or 3.3V, but I cannot find any suitable output properties in 'pincfg-node.yaml.' I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use 'power source' to specify the output voltage. Should I follow their approach or define a vendor-specific one? >> + >> +additionalProperties: >> + type: object >> + additionalProperties: >> + type: object > Wait, what? What are you describing here? I will fix it as: "-grp[0-9]$": type: object description: Pinctrl node's client devices use subnodes for desired pin configuration. Client device subnodes use below standard properties. properties: nuvoton,pins: .... and fix the example dts also. >> + properties: >> + nuvoton,pin: >> + description: >> + Each entry consists of 4 parameters and represents the mux and config >> + setting for one pin. >> + $ref: /schemas/types.yaml#/definitions/uint32-matrix >> + minItems: 1 >> + items: >> + items: >> + - minimum: 0x80 >> + maximum: 0xec >> + description: >> + The pinctrl register offset in syscon registers. >> + - minimum: 0 >> + maximum: 30 >> + description: >> + The bit offset in the pinctrl register. >> + - minimum: 0 >> + maximum: 15 >> + description: >> + The multi-function pin value. >> + - description: >> + The phandle of a node contains the generic pinconfig options >> + to use as described in pinctrl-bindings.txt. >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/gpio/gpio.h> >> + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> >> + #include <dt-bindings/pinctrl/ma35d1-pinfunc.h> >> + >> + pinctrl@40040000 { >> + compatible = "nuvoton,ma35d1-pinctrl"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + nuvoton,sys = <&sys>; >> + ranges = <0 0x40040000 0xc00>; >> + >> + gpioa@40040000 { >> + reg = <0x0 0x40>; >> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&clk GPA_GATE>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + }; >> + >> + pcfg_default: pcfg-default { >> + slew-rate = <0>; >> + input-schmitt-disable; >> + bias-disable; >> + power-source = <3300>; >> + drive-strength = <0>; > Really 0 mA? > > Why this is so incomplete? We treat this value as the value to be written to the control register, not as a current value in mA. I will correct this mistake. >> + }; >> + }; >> + >> + pinctrl {> + uart13 { >> + pinctrl_uart13: uart13grp { > According to your bindings this does not belong here. I will fix. >> + nuvoton,pins = >> + <MA35_SYS_REG_GPH_H 24 2 &pcfg_default>, >> + <MA35_SYS_REG_GPH_H 28 2 &pcfg_default>; >> + }; >> + }; >> + }; >> + >> + serial@407d0000 { > Drop node, not related at all. Okay, I will drop this node. >> + compatible = "nuvoton,ma35d1-uart"; >> + reg = <0x407d0000 0x100>; >> + interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&clk UART13_GATE>; >> + pinctrl-0 = <&pinctrl_uart13>; >> + }; >> diff --git a/include/dt-bindings/pinctrl/ma35d1-pinfunc.h b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h >> new file mode 100644 >> index 000000000000..a2609d466dc9 >> --- /dev/null >> +++ b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h > Filename matching bindings. The same name. > >> @@ -0,0 +1,38 @@ >> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */ >> +/* >> + * Copyright (C) 2023 Nuvoton Technologies. >> + */ >> + >> +#ifndef __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H >> +#define __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H >> + >> +#define MA35_SYS_REG_GPA_L 0x80 > Registry addresses are not suitable for bindings. There is also no need > to have REG address in the binding. Drop entire file. > > Best regards, > Krzysztof > I will remove 'ma35d1-pinfunc.h' as it will be useless after the 'nuvoton,pin' definition changed. Best Regards, Jacky Huang
On 16/10/2023 06:32, Jacky Huang wrote: >>> + '#size-cells': >>> + const: 1 >>> + >>> + nuvoton,sys: >>> + description: >>> + phandle to the syscon node >> sys is quite generic. Description explains nothing except duplicating >> known information. Drop duplicated info and instead explain to what this >> phandle points and how it is going to be used. Read comments carefully. >> >> >>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>> + items: >>> + maxItems: 1 >> So just phandle, not phandle-array, unless it is defined like this in >> some other binding. > > I would like to update this as: > > nuvoton,sys: Nothing improved. > $ref: /schemas/types.yaml#/definitions/phandle > description: > Help pinctrl driver to access system registers by means of regmap. Driver is not relevant here. Say which part of syscon are necessary for pinctrl operation. > > > >>> + >>> + ranges: true >>> + >>> +allOf: >>> + - $ref: pinctrl.yaml# >> allOf: goes after required: block. > > I will fix it. > >>> + >>> +patternProperties: >>> + "gpio[a-n]@[0-9a-f]+$": >> ^gpio@[0-9a-f]+$": > > I will fix this, and also fix the dtsi. > >>> + type: object >>> + additionalProperties: false >>> + properties: >>> + >> Drop blank line > > I will fix it. > >>> + gpio-controller: true >>> + >>> + '#gpio-cells': >>> + const: 2 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + interrupt-controller: true >>> + >>> + '#interrupt-cells': >>> + const: 2 >>> + >>> + interrupts: >>> + description: >>> + The interrupt outputs to sysirq. >>> + maxItems: 1 >>> + >>> + required: >>> + - reg >>> + - interrupts >>> + - interrupt-controller >>> + - '#interrupt-cells' >>> + - gpio-controller >>> + - '#gpio-cells' >> Keep the same order as in list of properties. > > I will fix the order. > >>> + >>> + "pcfg-[a-z0-9-.]+$": >> Why using different naming than other Nuvoton SoCs? You also accept >> "foobarpcfg-1", which does not look intentional. >> > > I will use '"^pin-[a-z0-9-.]+$" instead. [.] is redundant... What exactly do you want to match? > > >>> + type: object >>> + description: >>> + A pinctrl node should contain at least one subnodes representing the >>> + pinctrl groups available on the machine. Each subnode will list the >>> + pins it needs, and how they should be configured, with regard to muxer >>> + configuration, pullups, drive strength, input enable/disable and input >>> + schmitt. >>> + >>> + allOf: >>> + - $ref: pincfg-node.yaml# >> missing additional/unevaluatedProperties: false. > > I will add unevaluatedProperties: false. > >>> + >>> + properties: >>> + bias-disable: true >> Why do you need this and other ones? > > We expect the pin configuration to select one of ==> > bias-disable; > bias-pull-down; > bias-pull-up; > > This is the same as rockchip,pinctrl.yaml and renesas,rzv2m-pinctrl.yaml. OK, then go with nuvoton approach. List the properties (:true) and use additionalProperties: false. > >>> + >>> + bias-pull-down: true >>> + >>> + bias-pull-up: true >>> + >>> + drive-strength: >>> + minimum: 0 >> 0 mA? Is it really valid? Are you sure you used correct property? > > We treat this value as the value to be written to the control register, > not as > a current value in mA. I will correct this mistake. Instead treat it as mA. Is this possible? > >>> + maximum: 7 >>> + >>> + input-enable: true >>> + >>> + input-schmitt-enable: true >>> + >>> + power-source: >>> + description: >>> + I/O voltage in millivolt. >>> + enum: [ 1800, 3300 ] >> Missing units in property name. power-source also does not really >> describe the property. > > > The output voltage level of GPIO can be configured as 1.8V or 3.3V, > but I cannot find any suitable output properties in 'pincfg-node.yaml.' There is actually power-source, but treated as actual choice of power supplies. > I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use > 'power source' to specify the output voltage. Should I follow their > approach or define a vendor-specific one? Maybe Rob or Linus have here some recommendation, but I would suggest to go either with rtd1319d-pinctrl.yaml approach or add a generic property to pincfg-node expressed in real units like "io-microvolt". Rob, Linus, any ideas for generic property replacing register-specific power-source? Best regards, Krzysztof
Dear Krzysztof, Thank you for the review. On 2023/10/17 上午 03:52, Krzysztof Kozlowski wrote: > On 16/10/2023 06:32, Jacky Huang wrote: >>>> + '#size-cells': >>>> + const: 1 >>>> + >>>> + nuvoton,sys: >>>> + description: >>>> + phandle to the syscon node >>> sys is quite generic. Description explains nothing except duplicating >>> known information. Drop duplicated info and instead explain to what this >>> phandle points and how it is going to be used. > Read comments carefully. I will update the description of 'nuvoton,sys'. > >>> >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>> + items: >>>> + maxItems: 1 >>> So just phandle, not phandle-array, unless it is defined like this in >>> some other binding. >> I would like to update this as: >> >> nuvoton,sys: > Nothing improved. Here just fix the 'phandle-array' to 'phandle' and remove 'maxItems'. >> $ref: /schemas/types.yaml#/definitions/phandle >> description: >> Help pinctrl driver to access system registers by means of regmap. > Driver is not relevant here. Say which part of syscon are necessary for > pinctrl operation. > I will update description as: nuvoton,sys: $ref: /schemas/types.yaml#/definitions/phandle description: The pin function control registers are located in the system control register space. This phandle provides pinctrl the ability to access the pin function control registers through the use of regmap. >> >> >>>> + >>>> + ranges: true >>>> + >>>> +allOf: >>>> + - $ref: pinctrl.yaml# >>> allOf: goes after required: block. >> I will fix it. >> >>>> + >>>> +patternProperties: >>>> + "gpio[a-n]@[0-9a-f]+$": >>> ^gpio@[0-9a-f]+$": >> I will fix this, and also fix the dtsi. >> >>>> + type: object >>>> + additionalProperties: false >>>> + properties: >>>> + >>> Drop blank line >> I will fix it. >> >>>> + gpio-controller: true >>>> + >>>> + '#gpio-cells': >>>> + const: 2 >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + clocks: >>>> + maxItems: 1 >>>> + >>>> + interrupt-controller: true >>>> + >>>> + '#interrupt-cells': >>>> + const: 2 >>>> + >>>> + interrupts: >>>> + description: >>>> + The interrupt outputs to sysirq. >>>> + maxItems: 1 >>>> + >>>> + required: >>>> + - reg >>>> + - interrupts >>>> + - interrupt-controller >>>> + - '#interrupt-cells' >>>> + - gpio-controller >>>> + - '#gpio-cells' >>> Keep the same order as in list of properties. >> I will fix the order. >> >>>> + >>>> + "pcfg-[a-z0-9-.]+$": >>> Why using different naming than other Nuvoton SoCs? You also accept >>> "foobarpcfg-1", which does not look intentional. >>> >> I will use '"^pin-[a-z0-9-.]+$" instead. > [.] is redundant... What exactly do you want to match? I want to match the name like "-1.8v" or "-3.3v". However, this should be specified in the property, so I will drop the "-.". >> >>>> + type: object >>>> + description: >>>> + A pinctrl node should contain at least one subnodes representing the >>>> + pinctrl groups available on the machine. Each subnode will list the >>>> + pins it needs, and how they should be configured, with regard to muxer >>>> + configuration, pullups, drive strength, input enable/disable and input >>>> + schmitt. >>>> + >>>> + allOf: >>>> + - $ref: pincfg-node.yaml# >>> missing additional/unevaluatedProperties: false. >> I will add unevaluatedProperties: false. >> >>>> + >>>> + properties: >>>> + bias-disable: true >>> Why do you need this and other ones? >> We expect the pin configuration to select one of ==> >> bias-disable; >> bias-pull-down; >> bias-pull-up; >> >> This is the same as rockchip,pinctrl.yaml and renesas,rzv2m-pinctrl.yaml. > OK, then go with nuvoton approach. List the properties (:true) and use > additionalProperties: false. I got it. >>>> + >>>> + bias-pull-down: true >>>> + >>>> + bias-pull-up: true >>>> + >>>> + drive-strength: >>>> + minimum: 0 >>> 0 mA? Is it really valid? Are you sure you used correct property? >> We treat this value as the value to be written to the control register, >> not as >> a current value in mA. I will correct this mistake. > Instead treat it as mA. Is this possible? I will update it as: drive-strength-microamp: oneOf: - enum: [ 2900, 4400, 5800, 7300, 8600, 10100, 11500, 13000 ] description: 1.8V I/O driving strength - enum: [ 17100, 25600, 34100, 42800, 48000, 56000, 77000, 82000 ] description: 3.3V I/O driving strength And use a lookup table in the pinctrl driver to translate it into register value. >>>> + maximum: 7 >>>> + >>>> + input-enable: true >>>> + >>>> + input-schmitt-enable: true >>>> + >>>> + power-source: >>>> + description: >>>> + I/O voltage in millivolt. >>>> + enum: [ 1800, 3300 ] >>> Missing units in property name. power-source also does not really >>> describe the property. >> >> The output voltage level of GPIO can be configured as 1.8V or 3.3V, >> but I cannot find any suitable output properties in 'pincfg-node.yaml.' > There is actually power-source, but treated as actual choice of power > supplies. > >> I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use >> 'power source' to specify the output voltage. Should I follow their >> approach or define a vendor-specific one? > Maybe Rob or Linus have here some recommendation, but I would suggest to > go either with rtd1319d-pinctrl.yaml approach or add a generic property > to pincfg-node expressed in real units like "io-microvolt". OK, I will update it as: power-source: description: | Valid arguments are described as below: 0: power supply of 1.8V 1: power supply of 3.3V enum: [0, 1] > Rob, Linus, any ideas for generic property replacing register-specific > power-source? > > > Best regards, > Krzysztof > Best Regards, Jacky Huang
On 18/10/2023 05:26, Jacky Huang wrote: > Dear Krzysztof, > > Thank you for the review. > > > On 2023/10/17 上午 03:52, Krzysztof Kozlowski wrote: >> On 16/10/2023 06:32, Jacky Huang wrote: >>>>> + '#size-cells': >>>>> + const: 1 >>>>> + >>>>> + nuvoton,sys: >>>>> + description: >>>>> + phandle to the syscon node >>>> sys is quite generic. Description explains nothing except duplicating >>>> known information. Drop duplicated info and instead explain to what this >>>> phandle points and how it is going to be used. >> Read comments carefully. > > > I will update the description of 'nuvoton,sys'. What is the full name of destination block? > >> >>>> >>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>> + items: >>>>> + maxItems: 1 >>>> So just phandle, not phandle-array, unless it is defined like this in >>>> some other binding. >>> I would like to update this as: >>> >>> nuvoton,sys: >> Nothing improved. > > Here just fix the 'phandle-array' to 'phandle' and remove 'maxItems'. > >>> $ref: /schemas/types.yaml#/definitions/phandle >>> description: >>> Help pinctrl driver to access system registers by means of regmap. >> Driver is not relevant here. Say which part of syscon are necessary for >> pinctrl operation. >> > > I will update description as: > > nuvoton,sys: > $ref: /schemas/types.yaml#/definitions/phandle > description: > The pin function control registers are located in the system > control register space. This phandle provides pinctrl the > ability to access the pin function control registers through > the use of regmap. regmap is unrelated to the bindings. > >>>>> + maximum: 7 >>>>> + >>>>> + input-enable: true >>>>> + >>>>> + input-schmitt-enable: true >>>>> + >>>>> + power-source: >>>>> + description: >>>>> + I/O voltage in millivolt. >>>>> + enum: [ 1800, 3300 ] >>>> Missing units in property name. power-source also does not really >>>> describe the property. >>> >>> The output voltage level of GPIO can be configured as 1.8V or 3.3V, >>> but I cannot find any suitable output properties in 'pincfg-node.yaml.' >> There is actually power-source, but treated as actual choice of power >> supplies. >> >>> I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use >>> 'power source' to specify the output voltage. Should I follow their >>> approach or define a vendor-specific one? >> Maybe Rob or Linus have here some recommendation, but I would suggest to >> go either with rtd1319d-pinctrl.yaml approach or add a generic property >> to pincfg-node expressed in real units like "io-microvolt". > > OK, I will update it as: > > power-source: > description: | > Valid arguments are described as below: > 0: power supply of 1.8V > 1: power supply of 3.3V > enum: [0, 1] > > >> Rob, Linus, any ideas for generic property replacing register-specific >> power-source? I proposed io-microvolt Best regards, Krzysztof
Dear Krzysztof, Thank you for the review. On 2023/10/18 下午 01:58, Krzysztof Kozlowski wrote: > On 18/10/2023 05:26, Jacky Huang wrote: >> Dear Krzysztof, >> >> Thank you for the review. >> >> >> On 2023/10/17 上午 03:52, Krzysztof Kozlowski wrote: >>> On 16/10/2023 06:32, Jacky Huang wrote: >>>>>> + '#size-cells': >>>>>> + const: 1 >>>>>> + >>>>>> + nuvoton,sys: >>>>>> + description: >>>>>> + phandle to the syscon node >>>>> sys is quite generic. Description explains nothing except duplicating >>>>> known information. Drop duplicated info and instead explain to what this >>>>> phandle points and how it is going to be used. >>> Read comments carefully. >> >> I will update the description of 'nuvoton,sys'. > What is the full name of destination block? The full name is 'system-management'. From: sys: system-management@40460000 { compatible = "nuvoton,ma35d1-reset", "syscon"; reg = <0x0 0x40460000 0x0 0x200>; #reset-cells = <1>; }; >>>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>> + items: >>>>>> + maxItems: 1 >>>>> So just phandle, not phandle-array, unless it is defined like this in >>>>> some other binding. >>>> I would like to update this as: >>>> >>>> nuvoton,sys: >>> Nothing improved. >> Here just fix the 'phandle-array' to 'phandle' and remove 'maxItems'. >> >>>> $ref: /schemas/types.yaml#/definitions/phandle >>>> description: >>>> Help pinctrl driver to access system registers by means of regmap. >>> Driver is not relevant here. Say which part of syscon are necessary for >>> pinctrl operation. >>> >> I will update description as: >> >> nuvoton,sys: >> $ref: /schemas/types.yaml#/definitions/phandle >> description: >> The pin function control registers are located in the system >> control register space. This phandle provides pinctrl the >> ability to access the pin function control registers through >> the use of regmap. > regmap is unrelated to the bindings. So, I will just update the description as: phandle of the system-management node. >>>>>> + maximum: 7 >>>>>> + >>>>>> + input-enable: true >>>>>> + >>>>>> + input-schmitt-enable: true >>>>>> + >>>>>> + power-source: >>>>>> + description: >>>>>> + I/O voltage in millivolt. >>>>>> + enum: [ 1800, 3300 ] >>>>> Missing units in property name. power-source also does not really >>>>> describe the property. >>>> The output voltage level of GPIO can be configured as 1.8V or 3.3V, >>>> but I cannot find any suitable output properties in 'pincfg-node.yaml.' >>> There is actually power-source, but treated as actual choice of power >>> supplies. >>> >>>> I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use >>>> 'power source' to specify the output voltage. Should I follow their >>>> approach or define a vendor-specific one? >>> Maybe Rob or Linus have here some recommendation, but I would suggest to >>> go either with rtd1319d-pinctrl.yaml approach or add a generic property >>> to pincfg-node expressed in real units like "io-microvolt". >> OK, I will update it as: >> >> power-source: >> description: | >> Valid arguments are described as below: >> 0: power supply of 1.8V >> 1: power supply of 3.3V >> enum: [0, 1] >> >> >>> Rob, Linus, any ideas for generic property replacing register-specific >>> power-source? > I proposed io-microvolt > > Best regards, > Krzysztof > I will use 'io-microvolt' once it is available. Best Regards, Jacky Huang
On Mon, Oct 16, 2023 at 9:52 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use > > 'power source' to specify the output voltage. Should I follow their > > approach or define a vendor-specific one? > > Maybe Rob or Linus have here some recommendation, but I would suggest to > go either with rtd1319d-pinctrl.yaml approach or add a generic property > to pincfg-node expressed in real units like "io-microvolt". > > Rob, Linus, any ideas for generic property replacing register-specific > power-source? The existing power-source is generally used to select between (usually two) different chip-internal power rails, such as 1.8V and 3.3V. The format is a driver-specific enumerator. We *could* just patch the documentation for power-source to say that microvolts is the preferred format but legacy users may be using a custom enumerator. io-microvolt seems like a more long-term viable option if a wider range of voltages are to be supported so I'm happy with that if the DT folks think it's nicer. However notice that the power-source property is already being hard-coded into things such as SCMI and ACPI so it's not like it will ever be replaced by io-microvolt and phased out as far as Linux is concerned. Not the next 50 years at least. Yours, Linus Walleij
On 18/10/2023 10:18, Linus Walleij wrote: > On Mon, Oct 16, 2023 at 9:52 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > >>> I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use >>> 'power source' to specify the output voltage. Should I follow their >>> approach or define a vendor-specific one? >> >> Maybe Rob or Linus have here some recommendation, but I would suggest to >> go either with rtd1319d-pinctrl.yaml approach or add a generic property >> to pincfg-node expressed in real units like "io-microvolt". >> >> Rob, Linus, any ideas for generic property replacing register-specific >> power-source? > > The existing power-source is generally used to select between (usually > two) different chip-internal power rails, such as 1.8V and 3.3V. > The format is a driver-specific enumerator. > > We *could* just patch the documentation for power-source to > say that microvolts is the preferred format but legacy users may > be using a custom enumerator. > > io-microvolt seems like a more long-term viable option if a wider > range of voltages are to be supported so I'm happy with that if the > DT folks think it's nicer. However notice that the power-source > property is already being hard-coded into things such as SCMI > and ACPI so it's not like it will ever be replaced by io-microvolt > and phased out as far as Linux is concerned. Not the next 50 > years at least. This I understand. I think It is better in general if generic properties use units (e.g. drive-strength-microamp, output-impedance-ohms), so it could be here "io-microvolt". At least for the new bindings. Best regards, Krzysztof
On Wed, Oct 18, 2023 at 11:53 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 18/10/2023 10:18, Linus Walleij wrote: > > On Mon, Oct 16, 2023 at 9:52 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > >>> I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use > >>> 'power source' to specify the output voltage. Should I follow their > >>> approach or define a vendor-specific one? > >> > >> Maybe Rob or Linus have here some recommendation, but I would suggest to > >> go either with rtd1319d-pinctrl.yaml approach or add a generic property > >> to pincfg-node expressed in real units like "io-microvolt". > >> > >> Rob, Linus, any ideas for generic property replacing register-specific > >> power-source? > > > > The existing power-source is generally used to select between (usually > > two) different chip-internal power rails, such as 1.8V and 3.3V. > > The format is a driver-specific enumerator. > > > > We *could* just patch the documentation for power-source to > > say that microvolts is the preferred format but legacy users may > > be using a custom enumerator. > > > > io-microvolt seems like a more long-term viable option if a wider > > range of voltages are to be supported so I'm happy with that if the > > DT folks think it's nicer. However notice that the power-source > > property is already being hard-coded into things such as SCMI > > and ACPI so it's not like it will ever be replaced by io-microvolt > > and phased out as far as Linux is concerned. Not the next 50 > > years at least. > > This I understand. > > I think It is better in general if generic properties use units (e.g. > drive-strength-microamp, output-impedance-ohms), so it could be here > "io-microvolt". At least for the new bindings. I agree. Even io-voltage-microvolt perhaps. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml new file mode 100644 index 000000000000..0ddedbad4b78 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml @@ -0,0 +1,180 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/nuvoton,ma35d1-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton MA35D1 pin control and GPIO + +maintainers: + - Shan-Chun Hung <schung@nuvoton.com> + +properties: + compatible: + enum: + - nuvoton,ma35d1-pinctrl + + '#address-cells': + const: 1 + + '#size-cells': + const: 1 + + nuvoton,sys: + description: + phandle to the syscon node + $ref: /schemas/types.yaml#/definitions/phandle-array + items: + maxItems: 1 + + ranges: true + +allOf: + - $ref: pinctrl.yaml# + +patternProperties: + "gpio[a-n]@[0-9a-f]+$": + type: object + additionalProperties: false + properties: + + gpio-controller: true + + '#gpio-cells': + const: 2 + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + interrupt-controller: true + + '#interrupt-cells': + const: 2 + + interrupts: + description: + The interrupt outputs to sysirq. + maxItems: 1 + + required: + - reg + - interrupts + - interrupt-controller + - '#interrupt-cells' + - gpio-controller + - '#gpio-cells' + + "pcfg-[a-z0-9-.]+$": + type: object + description: + A pinctrl node should contain at least one subnodes representing the + pinctrl groups available on the machine. Each subnode will list the + pins it needs, and how they should be configured, with regard to muxer + configuration, pullups, drive strength, input enable/disable and input + schmitt. + + allOf: + - $ref: pincfg-node.yaml# + + properties: + bias-disable: true + + bias-pull-down: true + + bias-pull-up: true + + drive-strength: + minimum: 0 + maximum: 7 + + input-enable: true + + input-schmitt-enable: true + + power-source: + description: + I/O voltage in millivolt. + enum: [ 1800, 3300 ] + +additionalProperties: + type: object + additionalProperties: + type: object + properties: + nuvoton,pin: + description: + Each entry consists of 4 parameters and represents the mux and config + setting for one pin. + $ref: /schemas/types.yaml#/definitions/uint32-matrix + minItems: 1 + items: + items: + - minimum: 0x80 + maximum: 0xec + description: + The pinctrl register offset in syscon registers. + - minimum: 0 + maximum: 30 + description: + The bit offset in the pinctrl register. + - minimum: 0 + maximum: 15 + description: + The multi-function pin value. + - description: + The phandle of a node contains the generic pinconfig options + to use as described in pinctrl-bindings.txt. + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> + #include <dt-bindings/pinctrl/ma35d1-pinfunc.h> + + pinctrl@40040000 { + compatible = "nuvoton,ma35d1-pinctrl"; + #address-cells = <1>; + #size-cells = <1>; + nuvoton,sys = <&sys>; + ranges = <0 0x40040000 0xc00>; + + gpioa@40040000 { + reg = <0x0 0x40>; + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clk GPA_GATE>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + }; + + pcfg_default: pcfg-default { + slew-rate = <0>; + input-schmitt-disable; + bias-disable; + power-source = <3300>; + drive-strength = <0>; + }; + }; + + pinctrl { + uart13 { + pinctrl_uart13: uart13grp { + nuvoton,pins = + <MA35_SYS_REG_GPH_H 24 2 &pcfg_default>, + <MA35_SYS_REG_GPH_H 28 2 &pcfg_default>; + }; + }; + }; + + serial@407d0000 { + compatible = "nuvoton,ma35d1-uart"; + reg = <0x407d0000 0x100>; + interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clk UART13_GATE>; + pinctrl-0 = <&pinctrl_uart13>; + }; diff --git a/include/dt-bindings/pinctrl/ma35d1-pinfunc.h b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h new file mode 100644 index 000000000000..a2609d466dc9 --- /dev/null +++ b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */ +/* + * Copyright (C) 2023 Nuvoton Technologies. + */ + +#ifndef __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H +#define __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H + +#define MA35_SYS_REG_GPA_L 0x80 +#define MA35_SYS_REG_GPA_H 0x84 +#define MA35_SYS_REG_GPB_L 0x88 +#define MA35_SYS_REG_GPB_H 0x8c +#define MA35_SYS_REG_GPC_L 0x90 +#define MA35_SYS_REG_GPC_H 0x94 +#define MA35_SYS_REG_GPD_L 0x98 +#define MA35_SYS_REG_GPD_H 0x9c +#define MA35_SYS_REG_GPE_L 0xa0 +#define MA35_SYS_REG_GPE_H 0xa4 +#define MA35_SYS_REG_GPF_L 0xa8 +#define MA35_SYS_REG_GPF_H 0xac +#define MA35_SYS_REG_GPG_L 0xb0 +#define MA35_SYS_REG_GPG_H 0xb4 +#define MA35_SYS_REG_GPH_L 0xb8 +#define MA35_SYS_REG_GPH_H 0xbc +#define MA35_SYS_REG_GPI_L 0xc0 +#define MA35_SYS_REG_GPI_H 0xc4 +#define MA35_SYS_REG_GPJ_L 0xc8 +#define MA35_SYS_REG_GPJ_H 0xcc +#define MA35_SYS_REG_GPK_L 0xd0 +#define MA35_SYS_REG_GPK_H 0xd4 +#define MA35_SYS_REG_GPL_L 0xd8 +#define MA35_SYS_REG_GPL_H 0xdc +#define MA35_SYS_REG_GPM_L 0xe0 +#define MA35_SYS_REG_GPM_H 0xe4 +#define MA35_SYS_REG_GPN_L 0xe8 +#define MA35_SYS_REG_GPN_H 0xec + +#endif /* __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H */