| Message ID | 20231215143906.3651122-1-emil.renner.berthing@canonical.com |
|---|---|
| Headers | show |
| Series | Add T-Head TH15020 SoC pin control | expand |
On Fri, 15 Dec 2023 15:39:02 +0100, Emil Renner Berthing wrote: > Allow the generic gpio-ranges property so GPIOs can be mapped to their > corresponding pin. This way control of GPIO on pins that are already used > by other peripherals can be denied and basic pinconf can be done on pin > controllers that support it. > > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com> > --- > Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring <robh@kernel.org>
Rob Herring wrote: > On Fri, Dec 15, 2023 at 03:38:59PM +0100, Emil Renner Berthing wrote: > > Add bindings for the pin controllers on the T-Head TH1520 RISC-V SoC. > > > > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com> > > --- > > .../pinctrl/thead,th1520-pinctrl.yaml | 156 ++++++++++++++++++ > > 1 file changed, 156 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml > > new file mode 100644 > > index 000000000000..1b1b446cd498 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml > > @@ -0,0 +1,156 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/thead,th1520-pinctrl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: T-Head TH1520 SoC pin controller > > + > > +maintainers: > > + - Emil Renner Berthing <emil.renner.berthing@canonical.com> > > + > > +description: | > > + Pinmux and pinconf controller in the T-Head TH1520 RISC-V SoC. > > + > > + The TH1520 has 3 groups of pads each controlled from different memory ranges. > > + Confusingly the memory ranges are named > > + PADCTRL_AOSYS -> PAD Group 1 > > + PADCTRL1_APSYS -> PAD Group 2 > > + PADCTRL0_APSYS -> PAD Group 3 > > Are the programming models different? Yes, they control different pads and different number of pads. Pad group 1 also has some special pins that have bespoke pinconf, no pinconf and/or no pinmux and a "gap" in the registers. Also if some day we'll need to set up pinmux on behald of the audio co-processor then pad group 1 will be even more special. > > + > > + Each pad can be muxed individually to up to 5 different functions. For most > > + pads only a few of those 5 configurations are valid though, and a few pads in > > + group 1 does not support muxing at all. > > + > > + Pinconf is fairly regular except for a few pads in group 1 that either can't > > + be configured or has some special functions. The rest have configurable drive > > + strength, input enable, schmitt trigger, slew rate, pull-up and pull-down in > > + addition to a special strong pull up. > > + > > + Certain pads in group 1 can be muxed to AUDIO_PA0 - AUDIO_PA30 functions and > > + are then meant to be used by the audio co-processor. Each such pad can then > > + be further muxed to either audio GPIO or one of 4 functions such as UART, I2C > > + and I2S. If the audio pad is muxed to one of the 4 functions then pinconf is > > + also configured in different registers. All of this is done from a different > > + AUDIO_IOCTRL memory range and is left to the audio co-processor for now. > > + > > +properties: > > + compatible: > > + enum: > > + - thead,th1520-group1-pinctrl > > + - thead,th1520-group2-pinctrl > > + - thead,th1520-group3-pinctrl > > + > > + reg: > > + maxItems: 1 > > + > > +patternProperties: > > + '-[0-9]+$': > > Please make this a bit more specific. "-grp-[0-9]+$"? Oh, I was just trying to copy what other drivers did, but I see now that eg. mediatek,mt6779-pinctrl is not in the majority. Unfortunately "group" already has 2 meanings in this context. One are the pad groups from the datasheet described above and then there are the mux groups in the pinctrl framework, which in this case just contains a single pin since each pin can be muxed individually. Can we come up with a better name or should we just add this 3rd type of group? > > + patternProperties: > > + '-pins$': > > + type: object > > + $ref: /schemas/pinctrl/pincfg-node.yaml > > + description: > > + A pinctrl node should contain at least one subnode describing one > > + or more pads and their associated pinmux and pinconf settings. > > + > > + properties: > > + pins: > > + $ref: /schemas/types.yaml#/definitions/string-array > > Type is defined in pinmux-node.yaml. You need to reference it and drop > this. > > Normally the possible values are listed out. This seems to work for me: allOf: - if: properties: compatible: const: thead,th1520-group1-pinctrl then: patternProperties: '-[0-9]+$': patternProperties: '-pins$': properties: pins: items: enum: - OSC_CLK_IN - OSC_CLK_OUT ... - if: properties: compatible: const: thead,th1520-group2-pinctrl then: patternProperties: '-[0-9]+$': patternProperties: '-pins$': properties: pins: items: enum: - QSPI1_SCLK - QSPI1_CSN0 ... ... Would that be the way to go about it? > > + description: List of pads that properties in the node apply to. > > + > > + function: > > + $ref: /schemas/types.yaml#/definitions/string > > + enum: [ "0", "1", "2", "3", "4", "5" ] > > + description: The mux function to select for the given pins. > > + > > + bias-disable: true > > + > > + bias-pull-up: > > + type: boolean > > + > > + bias-pull-down: > > + type: boolean > > + > > + drive-strength: > > + enum: [ 1, 2, 3, 5, 7, 8, 10, 12, 13, 15, 16, 18, 20, 21, 23, 25 ] > > + > > + input-enable: true > > + > > + input-disable: true > > + > > + input-schmitt-enable: true > > + > > + input-schmitt-disable: true > > + > > + slew-rate: > > + maximum: 1 > > + > > + thead,strong-pull-up: > > + oneOf: > > + - type: boolean > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [ 0, 2100 ] > > + description: Enable or disable strong 2.1kOhm pull-up. > > bias-pull-up can already specify the strength in Ohms. The strong pull up is a separate bit that can be enabled independently from the regular pull-up/down, so in theory you could enable both the regular pull-up and the strong pull-up at the same time, or even the regular poll-down and the strong pull-up which is probably not advised. So the idea here was just to make sure that you can do eg. thead,strong-pull-up = <0>; to make sure the bit is cleared. Thanks! /Emil
Linus Walleij wrote: > On Sat, Dec 16, 2023 at 2:57 PM Emil Renner Berthing > <emil.renner.berthing@canonical.com> wrote: > > > > > + thead,strong-pull-up: > > > > + oneOf: > > > > + - type: boolean > > > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > > > + enum: [ 0, 2100 ] > > > > + description: Enable or disable strong 2.1kOhm pull-up. > > > > > > bias-pull-up can already specify the strength in Ohms. > > > > The strong pull up is a separate bit that can be enabled independently from the > > regular pull-up/down, so in theory you could enable both the regular pull-up > > and the strong pull-up at the same time, or even the regular poll-down and the > > strong pull-up which is probably not advised. > > bias-pull-up; <- Just regular pulling up the ordinary > bias-pull-up = <100>; <- Same thing if the ordinary is 100 Ohm (figure out what > resistance it actually is....) > bias-pull-up = <21000000>; <- strong pull up > bias-pull-up = <21000100>; <- both at the same time Hmm.. the two pull-ups combined would be a stronger pull-up, eg. lower resistance, right? So you'd need to calculate it using https://en.wikipedia.org/wiki/Series_and_parallel_circuits#Resistance_units_2 The problem is that the documentation doesn't actually mention what will happen if you combine the strong pull-up with the regular bias. My best guess for what happens if you enable the strong pull-up and the regular pull-down is that you create a sort of voltage divider. But how do you represent that as an Ohm value? We would kind of have to, otherwise the pinconf_get callbacks have states that it can't represent. > > So the idea here was just to make sure that you can do eg. > > > > thead,strong-pull-up = <0>; > > > > to make sure the bit is cleared. > > No use bias-disable; for this. > > Yours, > Linus Walleij > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Linus Walleij wrote: > On Thu, Dec 21, 2023 at 1:28 PM Emil Renner Berthing > <emil.renner.berthing@canonical.com> wrote: > > Linus Walleij wrote: > > > On Sat, Dec 16, 2023 at 2:57 PM Emil Renner Berthing > > > <emil.renner.berthing@canonical.com> wrote: > > > > > > > > > + thead,strong-pull-up: > > > > > > + oneOf: > > > > > > + - type: boolean > > > > > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > + enum: [ 0, 2100 ] > > > > > > + description: Enable or disable strong 2.1kOhm pull-up. > > > > > > > > > > bias-pull-up can already specify the strength in Ohms. > > > > > > > > The strong pull up is a separate bit that can be enabled independently from the > > > > regular pull-up/down, so in theory you could enable both the regular pull-up > > > > and the strong pull-up at the same time, or even the regular poll-down and the > > > > strong pull-up which is probably not advised. > > > > > > bias-pull-up; <- Just regular pulling up the ordinary > > > bias-pull-up = <100>; <- Same thing if the ordinary is 100 Ohm (figure out what > > > resistance it actually is....) > > > bias-pull-up = <21000000>; <- strong pull up > > > bias-pull-up = <21000100>; <- both at the same time > > > > Hmm.. the two pull-ups combined would be a stronger pull-up, eg. lower > > resistance, right? So you'd need to calculate it using > > https://en.wikipedia.org/wiki/Series_and_parallel_circuits#Resistance_units_2 > > Yeah hehe elementary electronics beats me, of course it is in parallel. > > > The problem is that the documentation doesn't actually mention what will happen > > if you combine the strong pull-up with the regular bias. > > So why even allow it then? > > Do the people designing boards using this have better documentation than what > you have? Then either get that documentation or just don't give them > too much rope. We can certainly prevent Linux from ever combining the strong pull-up with the regular bias, but that doesn't mean that the vendor u-boot can't find a use for it and might hand over pins in such states Linux then wouldn't know how to handle. If you think its better we could just postpone that problem to when/if it ever happens.
On Thu, Dec 21, 2023 at 3:07 PM Emil Renner Berthing <emil.renner.berthing@canonical.com> wrote: > Linus Walleij wrote: > > Do the people designing boards using this have better documentation than what > > you have? Then either get that documentation or just don't give them > > too much rope. > > We can certainly prevent Linux from ever combining the strong pull-up with the > regular bias, but that doesn't mean that the vendor u-boot can't find a use for > it and might hand over pins in such states Linux then wouldn't know how to > handle. What you are saying is "there might be people who have access to documentation that I don't have so they do this crazy thing". Clearly you cannot design for that. Print a big fat warning and fail probe if it happens. If U-Boot is using some feature you definitely cannot deal with if this happens, and then the people doing this can very well write a patch for the kernel. > If you think its better we could just postpone that problem to when/if it ever > happens. Yes please. Yours, Linus Walleij