[v1,1/8] dt-bindings: pinctrl: Add thead,th1520-pinctrl bindings

  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
  

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?

> +
> +  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]+$"?

> +    type: object

blank line. And move additionalProperties here. Easier to read in the 
indented cases.

> +    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.

> +            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.

> +
> +        required:
> +          - pins
> +
> +        additionalProperties: false
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    padctrl0_apsys: pinctrl@ec007000 {
> +        compatible = "thead,th1520-group3-pinctrl";
> +        reg = <0xec007000 0x1000>;
> +
> +        uart0_pins: uart0-0 {
> +            tx-pins {
> +                pins = "UART0_TXD";
> +                function = "0";
> +                bias-disable;
> +                drive-strength = <3>;
> +                input-disable;
> +                input-schmitt-disable;
> +                slew-rate = <0>;
> +            };
> +
> +            rx-pins {
> +                pins = "UART0_RXD";
> +                function = "0";
> +                bias-disable;
> +                drive-strength = <1>;
> +                input-enable;
> +                input-schmitt-enable;
> +                slew-rate = <0>;
> +            };
> +        };
> +    };
> +
> +    padctrl1_apsys: pinctrl@e7f3c000 {
> +        compatible = "thead,th1520-group2-pinctrl";
> +        reg = <0xe7f3c000 0x1000>;
> +
> +        i2c5_pins: i2c5-0 {
> +            i2c-pins {
> +                pins = "QSPI1_CSN0",    /* I2C5_SCL */
> +                       "QSPI1_D0_MOSI"; /* I2C5_SDA */
> +                function = "2";
> +                bias-disable;
> +                drive-strength = <7>;
> +                input-enable;
> +                input-schmitt-enable;
> +                slew-rate = <0>;
> +                thead,strong-pull-up;
> +            };
> +        };
> +    };
> -- 
> 2.40.1
>
  

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
  

Hi Emil,

thanks for your patch!

On Fri, Dec 15, 2023 at 3:39 PM Emil Renner Berthing
<emil.renner.berthing@canonical.com> wrote:

> +  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

Really, even in the documentation? If you look at the layout on the actual
chip, does a pattern emerge?

I think some use the north/south/east/west as group names with the BGA
chip facing up with the package text correctly readable then it is a bit
like a map.

> +          function:
> +            $ref: /schemas/types.yaml#/definitions/string
> +            enum: [ "0", "1", "2", "3", "4", "5" ]
> +            description: The mux function to select for the given pins.

So why is the opaque names "0", "1" etc used, and they will be the same for
all pins I bet. Most drivers use a string identifying the actual function here.
Such as "i2c", "gpio", etc.

Names that are just figures are *impossible* to understand without access
to a datasheet.

The point of device trees sources are to be human readable, strings of
magic numbers are not human readable at all.

> +          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 ]

milliamperes? Then use drive-strength-microamp.

If not, explain what each setting means, i.e. the number of max microamps.

At which point using drive-strength-microamp and a translation table in the
driver may be a better idea.

The only reason to use opaque numbers is if 1, 2 (etc) mean something like
"number of driver stages" with a current output that varies with technology.

> +          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.

Just use bias-pull-up with an argument.

bias-pull-up = <2100000>;

No argument would be the default setting.

No need for custom bindings.

> +        uart0_pins: uart0-0 {
> +            tx-pins {
> +                pins = "UART0_TXD";

Pins have reasonable names, but...

> +                function = "0";

What about function = "uart_0" hmmm?

Yours,
Linus Walleij
  

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

> 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
  

Linus Walleij wrote:
> Hi Emil,
>
> thanks for your patch!
>
> On Fri, Dec 15, 2023 at 3:39 PM Emil Renner Berthing
> <emil.renner.berthing@canonical.com> wrote:
>
> > +  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
>
> Really, even in the documentation? If you look at the layout on the actual
> chip, does a pattern emerge?

Yes, the documentation is where I got this from:
https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf

The pinmux chapter starting on page 31 only talks about the 3 pad groups, but
if you match the base addresses, table 3-8 page 46, with the memory map, table
1-2 page 1, they same base addresses have the PADCTRL names above.

> I think some use the north/south/east/west as group names with the BGA
> chip facing up with the package text correctly readable then it is a bit
> like a map.

I don't know if or where such documentation is available.
Jisheng, Guo or Drew, do you know?

> > +          function:
> > +            $ref: /schemas/types.yaml#/definitions/string
> > +            enum: [ "0", "1", "2", "3", "4", "5" ]
> > +            description: The mux function to select for the given pins.
>
> So why is the opaque names "0", "1" etc used, and they will be the same for
> all pins I bet. Most drivers use a string identifying the actual function here.
> Such as "i2c", "gpio", etc.
>
> Names that are just figures are *impossible* to understand without access
> to a datasheet.
>
> The point of device trees sources are to be human readable, strings of
> magic numbers are not human readable at all.
>
> > +          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 ]
>
> milliamperes? Then use drive-strength-microamp.
>
> If not, explain what each setting means, i.e. the number of max microamps.

It *is* the number of mA. I can change it uA if that's better.

> At which point using drive-strength-microamp and a translation table in the
> driver may be a better idea.

That's what it does, just with mA.

> The only reason to use opaque numbers is if 1, 2 (etc) mean something like
> "number of driver stages" with a current output that varies with technology.
>
> > +          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.
>
> Just use bias-pull-up with an argument.
>
> bias-pull-up = <2100000>;
>
> No argument would be the default setting.
>
> No need for custom bindings.
>
> > +        uart0_pins: uart0-0 {
> > +            tx-pins {
> > +                pins = "UART0_TXD";
>
> Pins have reasonable names, but...
>
> > +                function = "0";
>
> What about function = "uart_0" hmmm?
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
  

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
  

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.

Yours,
Linus Walleij
  

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