| Message ID | 20210106054950.303244-1-vkoul@kernel.org |
|---|---|
| Headers | show |
| Series | pinctrl: qcom: Add SM8350 pinctrl support | expand |
Hi Bjorn, On 07-01-21, 11:17, Bjorn Andersson wrote: > On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote: > > +#PIN CONFIGURATION NODES > > +patternProperties: > > + '-pinmux$': > > I believe that what Rob was asking for was the matter of describing the > mux and config subnodes under this one. But I don't know really how to > express this, because the following are all valid: I looked at the pinmux-node.yaml which describes subnodes with function and groups, this is a generic description and should be in pinmux-node.yaml not in every device description.. said that I am not sure why else should we add here :) > > default_state: default-state { > pins = "gpio1"; > bias-disable; > }; > > default_state: default-state { > rx { > pins = "gpio1"; > function = "gpio"; > bias-disable; > }; > }; > > default_state: default-state { > pinmux { > pins = "gpio1"; > function = "gpio"; > }; > > pinconf { > pins = "gpio1"; > bias-disable; > }; > }; > > I.e. the properties described here applies either to this node directly, > or any subnodes (1 level) down. > > > Also we've been using different "patternProperties" for this node since > the introduction of the binding 7 years ago. But to be "-state$" seems > to best represent what the node actually describes. > > Regards, > Bjorn > > > + type: object > > + description: > > + Pinctrl node's client devices use subnodes for desired pin configuration. > > + Client device subnodes use below standard properties. > > + $ref: "/schemas/pinctrl/pincfg-node.yaml" > > + > > + properties: > > + pins: > > + description: > > + List of gpio pins affected by the properties specified in this subnode. > > + items: > > + oneOf: > > + - pattern: "^gpio([0-9]|[1-9][0-9]|1[0-9][0-9]|20[0-3])$" > > + - enum: [ sdc1_clk, sdc1_cmd, sdc1_data, sdc2_clk, sdc2_cmd, sdc2_data ] > > + minItems: 1 > > + maxItems: 36 > > + > > + function: > > + description: > > + Specify the alternative function to be configured for the specified > > + pins. Functions are only valid for gpio pins. > > + enum: [ atest_char, atest_usb, audio_ref, cam_mclk, cci_async, > > + cci_i2c, cci_timer, cmu_rng, coex_uart1, coex_uart2, cri_trng, > > + cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1, > > + ddr_pxi2, ddr_pxi3, dp_hot, dp_lcd, gcc_gp1, gcc_gp2, gcc_gp3, > > + gpio, ibi_i3c, jitter_bist, lpass_slimbus, mdp_vsync, mdp_vsync0, > > + mdp_vsync1, mdp_vsync2, mdp_vsync3, mi2s0_data0, mi2s0_data1, > > + mi2s0_sck, mi2s0_ws, mi2s1_data0, mi2s1_data1, mi2s1_sck, > > + mi2s1_ws, mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws, > > + mss_grfc0, mss_grfc1, mss_grfc10, mss_grfc11, mss_grfc12, > > + mss_grfc2, mss_grfc3, mss_grfc4, mss_grfc5, mss_grfc6, > > + mss_grfc7, mss_grfc8, mss_grfc9, nav_gpio, pa_indicator, > > + pcie0_clkreqn, pcie1_clkreqn, phase_flag, pll_bist, pll_clk, > > + pri_mi2s, prng_rosc, qdss_cti, qdss_gpio, qlink0_enable, > > + qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request, > > + qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss, qspi0, > > + qspi1, qspi2, qspi3, qspi_clk, qspi_cs, qup0, qup1, qup10, > > + qup11, qup12, qup13, qup14, qup15, qup16, qup17, qup18, qup19, > > + qup2, qup3, qup4, qup5, qup6, qup7, qup8, qup9, qup_l4, qup_l5, > > + qup_l6, sd_write, sdc40, sdc41, sdc42, sdc43, sdc4_clk, > > + sdc4_cmd, sec_mi2s, tb_trig, tgu_ch0, tgu_ch1, tgu_ch2, > > + tgu_ch3, tsense_pwm1, tsense_pwm2, uim0_clk, uim0_data, > > + uim0_present, uim0_reset, uim1_clk, uim1_data, uim1_present, > > + uim1_reset, usb2phy_ac, usb_phy, vfr_0, vfr_1, vsense_trigger ] > > + > > + > > + drive-strength: > > + enum: [2, 4, 6, 8, 10, 12, 14, 16] > > + default: 2 > > + description: > > + Selects the drive strength for the specified pins, in mA. > > + > > + bias-pull-down: true > > + > > + bias-pull-up: true > > + > > + bias-disable: true > > + > > + output-high: true > > + > > + output-low: true > > + > > + required: > > + - pins > > + - function > > + > > + additionalProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - interrupt-controller > > + - '#interrupt-cells' > > + - gpio-controller > > + - '#gpio-cells' > > + - gpio-ranges > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + tlmm: pinctrl@f000000 { > > + compatible = "qcom,sm8350-tlmm"; > > + reg = <0x0f100000 0x300000>; > > + interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-controller; > > + #interrupt-cells = <2>; > > + gpio-ranges = <&tlmm 0 0 203>; > > + serial-pinmux { > > + pins = "gpio18", "gpio19"; > > + function = "qup3"; > > + drive-strength = <8>; > > + bias-disable; > > + }; > > + }; > > + > > +... > > -- > > 2.26.2 > >
On Fri 08 Jan 01:57 CST 2021, Vinod Koul wrote: > Hi Bjorn, > > On 07-01-21, 11:17, Bjorn Andersson wrote: > > On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote: > > > +#PIN CONFIGURATION NODES > > > +patternProperties: > > > + '-pinmux$': > > > > I believe that what Rob was asking for was the matter of describing the > > mux and config subnodes under this one. But I don't know really how to > > express this, because the following are all valid: > > I looked at the pinmux-node.yaml which describes subnodes with function > and groups, this is a generic description and should be in > pinmux-node.yaml not in every device description.. said that I am not > sure why else should we add here :) > Unfortunately I don't think I understand how the pinmux-node.yaml applies to the two possible levels of nodes. Also the description of our state nodes would have to be a mix of pinmux-node.yaml, pincfg-node.yaml and then the properties/constraints described here, plus something indicating that many of the common properties are not valid ones for this binding. Regardless of how to describe that though, I would like the patternProperties to be "-state$" :) Regards, Bjorn > > > > default_state: default-state { > > pins = "gpio1"; > > bias-disable; > > }; > > > > default_state: default-state { > > rx { > > pins = "gpio1"; > > function = "gpio"; > > bias-disable; > > }; > > }; > > > > default_state: default-state { > > pinmux { > > pins = "gpio1"; > > function = "gpio"; > > }; > > > > pinconf { > > pins = "gpio1"; > > bias-disable; > > }; > > }; > > > > I.e. the properties described here applies either to this node directly, > > or any subnodes (1 level) down. > > > > > > Also we've been using different "patternProperties" for this node since > > the introduction of the binding 7 years ago. But to be "-state$" seems > > to best represent what the node actually describes. > > > > Regards, > > Bjorn > > > > > + type: object > > > + description: > > > + Pinctrl node's client devices use subnodes for desired pin configuration. > > > + Client device subnodes use below standard properties. > > > + $ref: "/schemas/pinctrl/pincfg-node.yaml" > > > + > > > + properties: > > > + pins: > > > + description: > > > + List of gpio pins affected by the properties specified in this subnode. > > > + items: > > > + oneOf: > > > + - pattern: "^gpio([0-9]|[1-9][0-9]|1[0-9][0-9]|20[0-3])$" > > > + - enum: [ sdc1_clk, sdc1_cmd, sdc1_data, sdc2_clk, sdc2_cmd, sdc2_data ] > > > + minItems: 1 > > > + maxItems: 36 > > > + > > > + function: > > > + description: > > > + Specify the alternative function to be configured for the specified > > > + pins. Functions are only valid for gpio pins. > > > + enum: [ atest_char, atest_usb, audio_ref, cam_mclk, cci_async, > > > + cci_i2c, cci_timer, cmu_rng, coex_uart1, coex_uart2, cri_trng, > > > + cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1, > > > + ddr_pxi2, ddr_pxi3, dp_hot, dp_lcd, gcc_gp1, gcc_gp2, gcc_gp3, > > > + gpio, ibi_i3c, jitter_bist, lpass_slimbus, mdp_vsync, mdp_vsync0, > > > + mdp_vsync1, mdp_vsync2, mdp_vsync3, mi2s0_data0, mi2s0_data1, > > > + mi2s0_sck, mi2s0_ws, mi2s1_data0, mi2s1_data1, mi2s1_sck, > > > + mi2s1_ws, mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws, > > > + mss_grfc0, mss_grfc1, mss_grfc10, mss_grfc11, mss_grfc12, > > > + mss_grfc2, mss_grfc3, mss_grfc4, mss_grfc5, mss_grfc6, > > > + mss_grfc7, mss_grfc8, mss_grfc9, nav_gpio, pa_indicator, > > > + pcie0_clkreqn, pcie1_clkreqn, phase_flag, pll_bist, pll_clk, > > > + pri_mi2s, prng_rosc, qdss_cti, qdss_gpio, qlink0_enable, > > > + qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request, > > > + qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss, qspi0, > > > + qspi1, qspi2, qspi3, qspi_clk, qspi_cs, qup0, qup1, qup10, > > > + qup11, qup12, qup13, qup14, qup15, qup16, qup17, qup18, qup19, > > > + qup2, qup3, qup4, qup5, qup6, qup7, qup8, qup9, qup_l4, qup_l5, > > > + qup_l6, sd_write, sdc40, sdc41, sdc42, sdc43, sdc4_clk, > > > + sdc4_cmd, sec_mi2s, tb_trig, tgu_ch0, tgu_ch1, tgu_ch2, > > > + tgu_ch3, tsense_pwm1, tsense_pwm2, uim0_clk, uim0_data, > > > + uim0_present, uim0_reset, uim1_clk, uim1_data, uim1_present, > > > + uim1_reset, usb2phy_ac, usb_phy, vfr_0, vfr_1, vsense_trigger ] > > > + > > > + > > > + drive-strength: > > > + enum: [2, 4, 6, 8, 10, 12, 14, 16] > > > + default: 2 > > > + description: > > > + Selects the drive strength for the specified pins, in mA. > > > + > > > + bias-pull-down: true > > > + > > > + bias-pull-up: true > > > + > > > + bias-disable: true > > > + > > > + output-high: true > > > + > > > + output-low: true > > > + > > > + required: > > > + - pins > > > + - function > > > + > > > + additionalProperties: false > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - interrupts > > > + - interrupt-controller > > > + - '#interrupt-cells' > > > + - gpio-controller > > > + - '#gpio-cells' > > > + - gpio-ranges > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > + tlmm: pinctrl@f000000 { > > > + compatible = "qcom,sm8350-tlmm"; > > > + reg = <0x0f100000 0x300000>; > > > + interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; > > > + gpio-controller; > > > + #gpio-cells = <2>; > > > + interrupt-controller; > > > + #interrupt-cells = <2>; > > > + gpio-ranges = <&tlmm 0 0 203>; > > > + serial-pinmux { > > > + pins = "gpio18", "gpio19"; > > > + function = "qup3"; > > > + drive-strength = <8>; > > > + bias-disable; > > > + }; > > > + }; > > > + > > > +... > > > -- > > > 2.26.2 > > > > > -- > ~Vinod
On Thu, Jan 07, 2021 at 11:17:22AM -0600, Bjorn Andersson wrote: > On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote: > > > Add device tree binding Documentation details for Qualcomm SM8350 > > pinctrl driver. > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > --- > > .../bindings/pinctrl/qcom,sm8350-tlmm.yaml | 149 ++++++++++++++++++ > > 1 file changed, 149 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml > > new file mode 100644 > > index 000000000000..abdafd25bfc2 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml > > @@ -0,0 +1,149 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/qcom,sm8350-tlmm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Qualcomm Technologies, Inc. SM8350 TLMM block > > + > > +maintainers: > > + - Vinod Koul <vkoul@kernel.org> > > + > > +description: | > > + This binding describes the Top Level Mode Multiplexer block found in the > > + SM8350 platform. > > + > > +properties: > > + compatible: > > + const: qcom,sm8350-tlmm > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + interrupt-controller: true > > + > > + '#interrupt-cells': > > + description: Specifies the PIN numbers and Flags, as defined in > > + include/dt-bindings/interrupt-controller/irq.h > > + const: 2 > > + > > + gpio-controller: true > > + > > + '#gpio-cells': > > + description: Specifying the pin number and flags, as defined in > > + include/dt-bindings/gpio/gpio.h > > + const: 2 > > + > > + gpio-ranges: > > + maxItems: 1 > > + > > + gpio-reserved-ranges: > > + maxItems: 1 > > + > > +#PIN CONFIGURATION NODES > > +patternProperties: > > + '-pinmux$': > > I believe that what Rob was asking for was the matter of describing the > mux and config subnodes under this one. But I don't know really how to > express this, because the following are all valid: > > default_state: default-state { > pins = "gpio1"; > bias-disable; > }; > > default_state: default-state { > rx { > pins = "gpio1"; > function = "gpio"; > bias-disable; > }; > }; > > default_state: default-state { > pinmux { > pins = "gpio1"; > function = "gpio"; > }; > > pinconf { > pins = "gpio1"; > bias-disable; > }; > }; > > I.e. the properties described here applies either to this node directly, > or any subnodes (1 level) down. Why!? You can create a definition and reuse it. Something like this: $defs: pin-node: type: object properties: ... patternProperties: '-state$': oneOf: - $ref: #/$defs/pin-node - patternProperties: '.*': $ref: #/$defs/pin-node Rob
On Tue 12 Jan 20:56 CST 2021, Rob Herring wrote: > On Thu, Jan 07, 2021 at 11:17:22AM -0600, Bjorn Andersson wrote: > > On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote: > > > > > Add device tree binding Documentation details for Qualcomm SM8350 > > > pinctrl driver. > > > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > > --- > > > .../bindings/pinctrl/qcom,sm8350-tlmm.yaml | 149 ++++++++++++++++++ > > > 1 file changed, 149 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml > > > new file mode 100644 > > > index 000000000000..abdafd25bfc2 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml > > > @@ -0,0 +1,149 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/pinctrl/qcom,sm8350-tlmm.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Qualcomm Technologies, Inc. SM8350 TLMM block > > > + > > > +maintainers: > > > + - Vinod Koul <vkoul@kernel.org> > > > + > > > +description: | > > > + This binding describes the Top Level Mode Multiplexer block found in the > > > + SM8350 platform. > > > + > > > +properties: > > > + compatible: > > > + const: qcom,sm8350-tlmm > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + interrupt-controller: true > > > + > > > + '#interrupt-cells': > > > + description: Specifies the PIN numbers and Flags, as defined in > > > + include/dt-bindings/interrupt-controller/irq.h > > > + const: 2 > > > + > > > + gpio-controller: true > > > + > > > + '#gpio-cells': > > > + description: Specifying the pin number and flags, as defined in > > > + include/dt-bindings/gpio/gpio.h > > > + const: 2 > > > + > > > + gpio-ranges: > > > + maxItems: 1 > > > + > > > + gpio-reserved-ranges: > > > + maxItems: 1 > > > + > > > +#PIN CONFIGURATION NODES > > > +patternProperties: > > > + '-pinmux$': > > > > I believe that what Rob was asking for was the matter of describing the > > mux and config subnodes under this one. But I don't know really how to > > express this, because the following are all valid: > > > > default_state: default-state { > > pins = "gpio1"; > > bias-disable; > > }; > > > > default_state: default-state { > > rx { > > pins = "gpio1"; > > function = "gpio"; > > bias-disable; > > }; > > }; > > > > default_state: default-state { > > pinmux { > > pins = "gpio1"; > > function = "gpio"; > > }; > > > > pinconf { > > pins = "gpio1"; > > bias-disable; > > }; > > }; > > > > I.e. the properties described here applies either to this node directly, > > or any subnodes (1 level) down. > > Why!? > That's is how the generic pinctrl framework in Linux has parsed these nodes for the last several years, so I expect all pinctrl bindings to show this. And in particular, rather than forcing people to use the third form above the free naming of the subnodes allows to describe an entire 4 pin UART in a single pin state (the last requires 3-4 nodes + duplicates for sleep state), and for single gpio things the first one doesn't force people to make up phony names - which typically lands them in the last case (using pinmux/pinconf). > You can create a definition and reuse it. Something like this: > > $defs: > pin-node: > type: object > properties: > ... > > patternProperties: > '-state$': > oneOf: > - $ref: #/$defs/pin-node > > - patternProperties: > '.*': > $ref: #/$defs/pin-node So I presume then that it would make sense to convert {pinmux,pinconf}-node.yaml this form and then in our binding we should somehow just refer to these. But what's the appropriate method of extending and limiting pin-node? We have additional constraints and not all the properties are valid for the Qualcomm TLMM. Regards, Bjorn > > > Rob