diff mbox series

[v2,1/4] dt-binding: pinctrl: spacemit: add documents for K1 SoC

Message ID 20240825-02-k1-pinctrl-v2-1-ddd38a345d12@gentoo.org (mailing list archive)
State Superseded
Headers show
Series riscv: spacemit: add pinctrl support to K1 SoC | expand

Checks

Context Check Description
conchuod/vmtest-fixes-PR fail merge-conflict

Commit Message

Yixun Lan Aug. 25, 2024, 1:10 p.m. UTC
Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.

Two vendor specific properties are introduced here, As the pinctrl
has dedicated slew rate enable control - bit[7], so we have
spacemit,slew-rate-{enable,disable} for this. For the same reason,
creating spacemit,strong-pull-up for the strong pull up control.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
 include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
 2 files changed, 295 insertions(+)

Comments

Krzysztof Kozlowski Aug. 25, 2024, 1:48 p.m. UTC | #1
On 25/08/2024 15:10, Yixun Lan wrote:
> Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.


Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

It's "dt-bindings:"

> 
> Two vendor specific properties are introduced here, As the pinctrl
> has dedicated slew rate enable control - bit[7], so we have
> spacemit,slew-rate-{enable,disable} for this. For the same reason,
> creating spacemit,strong-pull-up for the strong pull up control.

Huh, no, use generic properties. More on that below



> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
>  include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
>  2 files changed, 295 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> new file mode 100644
> index 0000000000000..8adfc5ebbce37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 SoC Pin Controller
> +
> +maintainers:
> +  - Yixun Lan <dlan@gentoo.org>
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-pinctrl
> +
> +  reg:
> +    items:
> +      - description: pinctrl io memory base
> +
> +patternProperties:
> +  '-cfg$':
> +    type: object
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      A pinctrl node should contain at least one subnode representing the
> +      pinctrl groups available on the machine.
> +
> +    additionalProperties: false

Keep it before description.

> +
> +    patternProperties:
> +      '-pins$':
> +        type: object
> +        description: |
> +          Each subnode will list the pins it needs, and how they should
> +          be configured, with regard to muxer configuration, bias, input
> +          enable/disable, input schmitt trigger, slew-rate enable/disable,
> +          slew-rate, drive strength, power source.
> +        $ref: /schemas/pinctrl/pincfg-node.yaml
> +
> +        allOf:
> +          - $ref: pincfg-node.yaml#
> +          - $ref: pinmux-node.yaml#

You are duplicating refs.

> +
> +        properties:
> +          pinmux:
> +            description: |
> +              The list of GPIOs and their mux settings that properties in the
> +              node apply to. This should be set using the K1_PADCONF macro to
> +              construct the value.
> +            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pinmux

Hm why you need the ref?

> +
> +          bias-disable: true
> +
> +          bias-pull-up: true
> +
> +          bias-pull-down: true
> +
> +          drive-strength-microamp:
> +            description: |
> +              typical current when output high level, but in mA.
> +              1.8V output: 11, 21, 32, 42 (mA)
> +              3.3V output: 7, 10, 13, 16, 19, 23, 26, 29 (mA)
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +
> +          input-schmitt:
> +            description: |
> +              typical threshold for schmitt trigger.
> +              0: buffer mode
> +              1: trigger mode
> +              2, 3: trigger mode
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3]
> +
> +          power-source:
> +            description: external power supplies at 1.8v or 3.3v.
> +            enum: [ 1800, 3300 ]
> +
> +          slew-rate:
> +            description: |
> +              slew rate for output buffer
> +              0, 1: Slow speed

Hm? Surprising, 0 is slow speed?

> +              2: Medium speed
> +              3: Fast speed
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3]
> +
> +          spacemit,slew-rate-enable:
> +            description: enable slew rate.

The presence of slew-rate enables it, doesn't it?

> +            type: boolean
> +
> +          spacemit,slew-rate-disable:
> +            description: disable slew rate.
> +            type: boolean

Just use slew-rate, 0 disable, some value to match real slew-rate.

> +
> +          spacemit,strong-pull-up:
> +            description: enable strong pull up.

Do not duplicate the property name in description. You did not say
anything useful here. What is "strong"? bias-pull-up takes also an argument.

> +            type: boolean
> +
> +        required:
> +          - pinmux
> +
> +        additionalProperties: false

This goes up, before description.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/pinctrl/spacemit,k1-pinctrl.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pinctrl@d401e000 {
> +            compatible = "spacemit,k1-pinctrl";
> +            reg = <0x0 0xd401e000 0x0 0x400>;
> +            #pinctrl-cells = <2>;
> +            #gpio-range-cells = <3>;

This wasn't ever tested... :(
...

> diff --git a/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> new file mode 100644
> index 0000000000000..13ef4aa6c53a3
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> @@ -0,0 +1,161 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * Copyright (c) 2022-2024 SpacemiT (Hangzhou) Technology Co. Ltd
> + * Copyright (c) 2024 Yixun Lan <dlan@gentoo.org>
> + *
> + */
> +
> +#ifndef _DT_BINDINGS_PINCTRL_K1_H
> +#define _DT_BINDINGS_PINCTRL_K1_H
> +
> +#define PINMUX(pin, mux) \
> +	(((pin) & 0xffff) | (((mux) & 0xff) << 16))
> +
> +/* pin offset */
> +#define PINID(x)	((x) + 1)
> +
> +#define GPIO_INVAL  0
> +#define GPIO_00     PINID(0)

Not really, pin numbers are not bindings. Drop entire header.

...

> +
> +#define SLEW_RATE_SLOW0		0
> +#define SLEW_RATE_SLOW1		1
> +#define SLEW_RATE_MEDIUM	2
> +#define SLEW_RATE_FAST		3

Not a binding, either. No usage in the driver.

> +
> +#define K1_PADCONF(pin, func) (((pin) << 16) | (func))

Not a binding.



Best regards,
Krzysztof
Rob Herring (Arm) Aug. 25, 2024, 2:22 p.m. UTC | #2
On Sun, 25 Aug 2024 13:10:02 +0000, Yixun Lan wrote:
> Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
> 
> Two vendor specific properties are introduced here, As the pinctrl
> has dedicated slew rate enable control - bit[7], so we have
> spacemit,slew-rate-{enable,disable} for this. For the same reason,
> creating spacemit,strong-pull-up for the strong pull up control.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
>  include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
>  2 files changed, 295 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml: patternProperties:-cfg$:patternProperties:-pins$:properties:drive-strength-microamp: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/mediatek,dsi-phy.example.dtb: dsi-phy@10215000: drive-strength-microamp: 4000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/phy/mediatek,dsi-phy.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/mediatek,dsi-phy.example.dtb: dsi-phy@10215000: drive-strength-microamp: 4000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/property-units.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/mediatek,mt8183-pinctrl.example.dtb: pinctrl@10005000: i2c0-pins:pins1:drive-strength-microamp: 1000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/pinctrl/mediatek,mt8183-pinctrl.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/mediatek,mt8183-pinctrl.example.dtb: pins1: drive-strength-microamp: 1000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/property-units.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/mediatek,mt8186-pinctrl.example.dtb: pinctrl@10005000: i2c0-pins:pins:drive-strength-microamp: 1000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/pinctrl/mediatek,mt8186-pinctrl.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/mediatek,mt8186-pinctrl.example.dtb: pins: drive-strength-microamp: 1000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/property-units.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/mediatek,mt8195-pinctrl.example.dtb: pinctrl@10005000: i2c0-pins:pins:drive-strength-microamp: 1000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/pinctrl/mediatek,mt8195-pinctrl.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/mediatek,mt8195-pinctrl.example.dtb: pins: drive-strength-microamp: 1000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/property-units.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/mediatek,mt8188-pinctrl.example.dtb: pinctrl@10005000: i2c0-pins:pins:drive-strength-microamp: 1000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/pinctrl/mediatek,mt8188-pinctrl.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/mediatek,mt8188-pinctrl.example.dtb: pins: drive-strength-microamp: 1000 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/property-units.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.example.dtb: pinctrl@d401e000: '#gpio-range-cells', '#pinctrl-cells' do not match any of the regexes: '-cfg$', 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.example.dtb: uart0-2-pins: drive-strength-microamp: 32 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/property-units.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240825-02-k1-pinctrl-v2-1-ddd38a345d12@gentoo.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Yixun Lan Aug. 26, 2024, 1:36 a.m. UTC | #3
Hi Krzysztof: 

On 15:48 Sun 25 Aug     , Krzysztof Kozlowski wrote:
> On 25/08/2024 15:10, Yixun Lan wrote:
> > Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
> 
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
> It's "dt-bindings:"
Ok, will fix in next version

> 
> > 
> > Two vendor specific properties are introduced here, As the pinctrl
> > has dedicated slew rate enable control - bit[7], so we have
> > spacemit,slew-rate-{enable,disable} for this. For the same reason,
> > creating spacemit,strong-pull-up for the strong pull up control.
> 
> Huh, no, use generic properties. More on that below
> 
see my reply below

> 
> 
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >  .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
> >  include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
> >  2 files changed, 295 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> > new file mode 100644
> > index 0000000000000..8adfc5ebbce37
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> > @@ -0,0 +1,134 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SpacemiT K1 SoC Pin Controller
> > +
> > +maintainers:
> > +  - Yixun Lan <dlan@gentoo.org>
> > +
> > +properties:
> > +  compatible:
> > +    const: spacemit,k1-pinctrl
> > +
> > +  reg:
> > +    items:
> > +      - description: pinctrl io memory base
> > +
> > +patternProperties:
> > +  '-cfg$':
> > +    type: object
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
Ok
> > +      A pinctrl node should contain at least one subnode representing the
> > +      pinctrl groups available on the machine.
> > +
> > +    additionalProperties: false
> 
> Keep it before description.
Ok
> 
> > +
> > +    patternProperties:
> > +      '-pins$':
> > +        type: object
> > +        description: |
> > +          Each subnode will list the pins it needs, and how they should
> > +          be configured, with regard to muxer configuration, bias, input
> > +          enable/disable, input schmitt trigger, slew-rate enable/disable,
> > +          slew-rate, drive strength, power source.
> > +        $ref: /schemas/pinctrl/pincfg-node.yaml
> > +
> > +        allOf:
> > +          - $ref: pincfg-node.yaml#
> > +          - $ref: pinmux-node.yaml#
> 
> You are duplicating refs.
ok, will fix it
> 
> > +
> > +        properties:
> > +          pinmux:
> > +            description: |
> > +              The list of GPIOs and their mux settings that properties in the
> > +              node apply to. This should be set using the K1_PADCONF macro to
> > +              construct the value.
> > +            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pinmux
> 
> Hm why you need the ref?
> 
will drop it
> > +
> > +          bias-disable: true
> > +
> > +          bias-pull-up: true
> > +
> > +          bias-pull-down: true
> > +
> > +          drive-strength-microamp:
> > +            description: |
> > +              typical current when output high level, but in mA.
> > +              1.8V output: 11, 21, 32, 42 (mA)
> > +              3.3V output: 7, 10, 13, 16, 19, 23, 26, 29 (mA)
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +          input-schmitt:
> > +            description: |
> > +              typical threshold for schmitt trigger.
> > +              0: buffer mode
> > +              1: trigger mode
> > +              2, 3: trigger mode
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            enum: [0, 1, 2, 3]
> > +
> > +          power-source:
> > +            description: external power supplies at 1.8v or 3.3v.
> > +            enum: [ 1800, 3300 ]
> > +
> > +          slew-rate:
> > +            description: |
> > +              slew rate for output buffer
> > +              0, 1: Slow speed
> 
> Hm? Surprising, 0 is slow speed?
> 
from docs, section 3.3.2.2 MFPR Register Description
0, 1 are same, both for slow speed
https://developer.spacemit.com/documentation?token=An1vwTwKaigaXRkYfwmcznTXned

> > +              2: Medium speed
> > +              3: Fast speed
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            enum: [0, 1, 2, 3]
> > +
> > +          spacemit,slew-rate-enable:
> > +            description: enable slew rate.
> 
> The presence of slew-rate enables it, doesn't it?
> 
yes, this should work, I will take this approach, thanks

> > +            type: boolean
> > +
> > +          spacemit,slew-rate-disable:
> > +            description: disable slew rate.
> > +            type: boolean
> 
> Just use slew-rate, 0 disable, some value to match real slew-rate.
> 
sounds good to me, since 0, 1 indicate same meaning, can re-use 0 for
disabling slew rate.

> > +
> > +          spacemit,strong-pull-up:
> > +            description: enable strong pull up.
> 
> Do not duplicate the property name in description. You did not say
> anything useful here. What is "strong"? bias-pull-up takes also an argument.
> 
there is a dedicated strong pull bit[3] for I2C, SD card kinds of pad
I don't know how 'strong' it is if in ohms, will see if can get
more info on this (may expand the description)

I think using 'bias-pull-up' property with argument should also work,
but it occur to me it's more intuitive to introduce a property here, which
reflect the underlying hardware functionality. this is similar to starfive's jh7100
bindings/pinctrl/starfive,jh7100-pinctrl.yaml:154
(refer to exist code doesn't mean always correct, so I need advice here)

I will keep this property unless there is objection, please let me know

> > +            type: boolean
> > +
> > +        required:
> > +          - pinmux
> > +
> > +        additionalProperties: false
> 
> This goes up, before description.
> 
Ok
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/pinctrl/spacemit,k1-pinctrl.h>
> > +
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        pinctrl@d401e000 {
> > +            compatible = "spacemit,k1-pinctrl";
> > +            reg = <0x0 0xd401e000 0x0 0x400>;
> > +            #pinctrl-cells = <2>;
> > +            #gpio-range-cells = <3>;
> 
> This wasn't ever tested... :(
> ...
will drop it
> 
> > diff --git a/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> > new file mode 100644
> > index 0000000000000..13ef4aa6c53a3
> > --- /dev/null
> > +++ b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> > @@ -0,0 +1,161 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> > +/*
> > + * Copyright (c) 2022-2024 SpacemiT (Hangzhou) Technology Co. Ltd
> > + * Copyright (c) 2024 Yixun Lan <dlan@gentoo.org>
> > + *
> > + */
> > +
> > +#ifndef _DT_BINDINGS_PINCTRL_K1_H
> > +#define _DT_BINDINGS_PINCTRL_K1_H
> > +
> > +#define PINMUX(pin, mux) \
> > +	(((pin) & 0xffff) | (((mux) & 0xff) << 16))
> > +
> > +/* pin offset */
> > +#define PINID(x)	((x) + 1)
> > +
> > +#define GPIO_INVAL  0
> > +#define GPIO_00     PINID(0)
> 
> Not really, pin numbers are not bindings. Drop entire header.
> 
Ok, I will move them to dts folder, which should be file
arch/riscv/boot/dts/spacemit/k1-pinctrl.h

> ...
> 
> > +
> > +#define SLEW_RATE_SLOW0		0
> > +#define SLEW_RATE_SLOW1		1
> > +#define SLEW_RATE_MEDIUM	2
> > +#define SLEW_RATE_FAST		3
> 
> Not a binding, either. No usage in the driver.
Ok, will drop it

> 
> > +
> > +#define K1_PADCONF(pin, func) (((pin) << 16) | (func))
> 
> Not a binding.
> 
same, move to dts

> 
> 
> Best regards,
> Krzysztof
Yixun Lan Aug. 26, 2024, 3:09 a.m. UTC | #4
On 13:10 Sun 25 Aug     , Yixun Lan wrote:
> Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
> 
> Two vendor specific properties are introduced here, As the pinctrl
> has dedicated slew rate enable control - bit[7], so we have
> spacemit,slew-rate-{enable,disable} for this. For the same reason,
> creating spacemit,strong-pull-up for the strong pull up control.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
>  include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
>  2 files changed, 295 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> new file mode 100644
> index 0000000000000..8adfc5ebbce37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 SoC Pin Controller
> +
> +maintainers:
> +  - Yixun Lan <dlan@gentoo.org>
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-pinctrl
> +
> +  reg:
> +    items:
> +      - description: pinctrl io memory base
> +
> +patternProperties:
> +  '-cfg$':
> +    type: object
> +    description: |
> +      A pinctrl node should contain at least one subnode representing the
> +      pinctrl groups available on the machine.
> +
> +    additionalProperties: false
> +
> +    patternProperties:
> +      '-pins$':
> +        type: object
> +        description: |
> +          Each subnode will list the pins it needs, and how they should
> +          be configured, with regard to muxer configuration, bias, input
> +          enable/disable, input schmitt trigger, slew-rate enable/disable,
> +          slew-rate, drive strength, power source.
> +        $ref: /schemas/pinctrl/pincfg-node.yaml
> +
> +        allOf:
> +          - $ref: pincfg-node.yaml#
> +          - $ref: pinmux-node.yaml#
> +
> +        properties:
> +          pinmux:
> +            description: |
> +              The list of GPIOs and their mux settings that properties in the
> +              node apply to. This should be set using the K1_PADCONF macro to
> +              construct the value.
> +            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pinmux
> +
> +          bias-disable: true
> +
> +          bias-pull-up: true
> +
> +          bias-pull-down: true
> +
> +          drive-strength-microamp:
just realise here should use 'drive-strength' which will comply to hw
will fix in next version

> +            description: |
> +              typical current when output high level, but in mA.
> +              1.8V output: 11, 21, 32, 42 (mA)
> +              3.3V output: 7, 10, 13, 16, 19, 23, 26, 29 (mA)
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +
> +          input-schmitt:
> +            description: |
> +              typical threshold for schmitt trigger.
> +              0: buffer mode
> +              1: trigger mode
> +              2, 3: trigger mode
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3]
> +
> +          power-source:
> +            description: external power supplies at 1.8v or 3.3v.
> +            enum: [ 1800, 3300 ]
> +
> +          slew-rate:
> +            description: |
> +              slew rate for output buffer
> +              0, 1: Slow speed
> +              2: Medium speed
> +              3: Fast speed
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3]
> +
> +          spacemit,slew-rate-enable:
> +            description: enable slew rate.
> +            type: boolean
> +
> +          spacemit,slew-rate-disable:
> +            description: disable slew rate.
> +            type: boolean
> +
> +          spacemit,strong-pull-up:
> +            description: enable strong pull up.
> +            type: boolean
> +
> +        required:
> +          - pinmux
> +
> +        additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/pinctrl/spacemit,k1-pinctrl.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pinctrl@d401e000 {
> +            compatible = "spacemit,k1-pinctrl";
> +            reg = <0x0 0xd401e000 0x0 0x400>;
> +            #pinctrl-cells = <2>;
> +            #gpio-range-cells = <3>;
> +
> +            uart0_2_cfg: uart0-2-cfg {
> +                uart0-2-pins {
> +                    pinmux = <K1_PADCONF(GPIO_68, 2)>,
> +                             <K1_PADCONF(GPIO_69, 2)>;
> +                    bias-pull-up;
> +                    drive-strength-microamp = <32>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> new file mode 100644
> index 0000000000000..13ef4aa6c53a3
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> @@ -0,0 +1,161 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * Copyright (c) 2022-2024 SpacemiT (Hangzhou) Technology Co. Ltd
> + * Copyright (c) 2024 Yixun Lan <dlan@gentoo.org>
> + *
> + */
> +
> +#ifndef _DT_BINDINGS_PINCTRL_K1_H
> +#define _DT_BINDINGS_PINCTRL_K1_H
> +
> +#define PINMUX(pin, mux) \
> +	(((pin) & 0xffff) | (((mux) & 0xff) << 16))
> +
> +/* pin offset */
> +#define PINID(x)	((x) + 1)
> +
> +#define GPIO_INVAL  0
> +#define GPIO_00     PINID(0)
> +#define GPIO_01     PINID(1)
> +#define GPIO_02     PINID(2)
> +#define GPIO_03     PINID(3)
> +#define GPIO_04     PINID(4)
> +#define GPIO_05     PINID(5)
> +#define GPIO_06     PINID(6)
> +#define GPIO_07     PINID(7)
> +#define GPIO_08     PINID(8)
> +#define GPIO_09     PINID(9)
> +#define GPIO_10     PINID(10)
> +#define GPIO_11     PINID(11)
> +#define GPIO_12     PINID(12)
> +#define GPIO_13     PINID(13)
> +#define GPIO_14     PINID(14)
> +#define GPIO_15     PINID(15)
> +#define GPIO_16     PINID(16)
> +#define GPIO_17     PINID(17)
> +#define GPIO_18     PINID(18)
> +#define GPIO_19     PINID(19)
> +#define GPIO_20     PINID(20)
> +#define GPIO_21     PINID(21)
> +#define GPIO_22     PINID(22)
> +#define GPIO_23     PINID(23)
> +#define GPIO_24     PINID(24)
> +#define GPIO_25     PINID(25)
> +#define GPIO_26     PINID(26)
> +#define GPIO_27     PINID(27)
> +#define GPIO_28     PINID(28)
> +#define GPIO_29     PINID(29)
> +#define GPIO_30     PINID(30)
> +#define GPIO_31     PINID(31)
> +
> +#define GPIO_32     PINID(32)
> +#define GPIO_33     PINID(33)
> +#define GPIO_34     PINID(34)
> +#define GPIO_35     PINID(35)
> +#define GPIO_36     PINID(36)
> +#define GPIO_37     PINID(37)
> +#define GPIO_38     PINID(38)
> +#define GPIO_39     PINID(39)
> +#define GPIO_40     PINID(40)
> +#define GPIO_41     PINID(41)
> +#define GPIO_42     PINID(42)
> +#define GPIO_43     PINID(43)
> +#define GPIO_44     PINID(44)
> +#define GPIO_45     PINID(45)
> +#define GPIO_46     PINID(46)
> +#define GPIO_47     PINID(47)
> +#define GPIO_48     PINID(48)
> +#define GPIO_49     PINID(49)
> +#define GPIO_50     PINID(50)
> +#define GPIO_51     PINID(51)
> +#define GPIO_52     PINID(52)
> +#define GPIO_53     PINID(53)
> +#define GPIO_54     PINID(54)
> +#define GPIO_55     PINID(55)
> +#define GPIO_56     PINID(56)
> +#define GPIO_57     PINID(57)
> +#define GPIO_58     PINID(58)
> +#define GPIO_59     PINID(59)
> +#define GPIO_60     PINID(60)
> +#define GPIO_61     PINID(61)
> +#define GPIO_62     PINID(62)
> +#define GPIO_63     PINID(63)
> +
> +#define GPIO_64     PINID(64)
> +#define GPIO_65     PINID(65)
> +#define GPIO_66     PINID(66)
> +#define GPIO_67     PINID(67)
> +#define GPIO_68     PINID(68)
> +#define GPIO_69     PINID(69)
> +#define GPIO_70     PINID(70)
> +#define GPIO_71     PINID(71)
> +#define GPIO_72     PINID(72)
> +#define GPIO_73     PINID(73)
> +#define GPIO_74     PINID(74)
> +#define GPIO_75     PINID(75)
> +#define GPIO_76     PINID(76)
> +#define GPIO_77     PINID(77)
> +#define GPIO_78     PINID(78)
> +#define GPIO_79     PINID(79)
> +#define GPIO_80     PINID(80)
> +#define GPIO_81     PINID(81)
> +#define GPIO_82     PINID(82)
> +#define GPIO_83     PINID(83)
> +#define GPIO_84     PINID(84)
> +#define GPIO_85     PINID(85)
> +
> +#define GPIO_101    PINID(89)
> +#define GPIO_100    PINID(90)
> +#define GPIO_99     PINID(91)
> +#define GPIO_98     PINID(92)
> +#define GPIO_103    PINID(93)
> +#define GPIO_102    PINID(94)
> +
> +#define GPIO_104    PINID(109)
> +#define GPIO_105    PINID(110)
> +#define GPIO_106    PINID(111)
> +#define GPIO_107    PINID(112)
> +#define GPIO_108    PINID(113)
> +#define GPIO_109    PINID(114)
> +#define GPIO_110    PINID(115)
> +
> +#define GPIO_93     PINID(116)
> +#define GPIO_94     PINID(117)
> +#define GPIO_95     PINID(118)
> +#define GPIO_96     PINID(119)
> +#define GPIO_97     PINID(120)
> +
> +#define GPIO_86     PINID(122)
> +#define GPIO_87     PINID(123)
> +#define GPIO_88     PINID(124)
> +#define GPIO_89     PINID(125)
> +#define GPIO_90     PINID(126)
> +#define GPIO_91     PINID(127)
> +#define GPIO_92     PINID(128)
> +
> +#define GPIO_111    PINID(130)
> +#define GPIO_112    PINID(131)
> +#define GPIO_113    PINID(132)
> +#define GPIO_114    PINID(133)
> +#define GPIO_115    PINID(134)
> +#define GPIO_116    PINID(135)
> +#define GPIO_117    PINID(136)
> +#define GPIO_118    PINID(137)
> +#define GPIO_119    PINID(138)
> +#define GPIO_120    PINID(139)
> +#define GPIO_121    PINID(140)
> +#define GPIO_122    PINID(141)
> +#define GPIO_123    PINID(142)
> +#define GPIO_124    PINID(143)
> +#define GPIO_125    PINID(144)
> +#define GPIO_126    PINID(145)
> +#define GPIO_127    PINID(146)
> +
> +#define SLEW_RATE_SLOW0		0
> +#define SLEW_RATE_SLOW1		1
> +#define SLEW_RATE_MEDIUM	2
> +#define SLEW_RATE_FAST		3
> +
> +#define K1_PADCONF(pin, func) (((pin) << 16) | (func))
> +
> +#endif /* _DT_BINDINGS_PINCTRL_K1_H */
> 
> -- 
> 2.45.2
Inochi Amaoto Aug. 26, 2024, 4:19 a.m. UTC | #5
On Mon, Aug 26, 2024 at 01:36:35AM GMT, Yixun Lan wrote:
> Hi Krzysztof: 
> 
> On 15:48 Sun 25 Aug     , Krzysztof Kozlowski wrote:
> > On 25/08/2024 15:10, Yixun Lan wrote:
> > > Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
> > 
> > 
> > Please use subject prefixes matching the subsystem. You can get them for
> > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> > your patch is touching. For bindings, the preferred subjects are
> > explained here:
> > https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> > 
> > It's "dt-bindings:"
> Ok, will fix in next version
> 
> > 
> > > 
> > > Two vendor specific properties are introduced here, As the pinctrl
> > > has dedicated slew rate enable control - bit[7], so we have
> > > spacemit,slew-rate-{enable,disable} for this. For the same reason,
> > > creating spacemit,strong-pull-up for the strong pull up control.
> > 
> > Huh, no, use generic properties. More on that below
> > 
> see my reply below
> 
> > 
> > 
> > > 
> > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > > ---
> > >  .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
> > >  include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
> > >  2 files changed, 295 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> > > new file mode 100644
> > > index 0000000000000..8adfc5ebbce37
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> > > @@ -0,0 +1,134 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: SpacemiT K1 SoC Pin Controller
> > > +
> > > +maintainers:
> > > +  - Yixun Lan <dlan@gentoo.org>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: spacemit,k1-pinctrl
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: pinctrl io memory base
> > > +
> > > +patternProperties:
> > > +  '-cfg$':
> > > +    type: object
> > > +    description: |
> > 
> > Do not need '|' unless you need to preserve formatting.
> > 
> Ok
> > > +      A pinctrl node should contain at least one subnode representing the
> > > +      pinctrl groups available on the machine.
> > > +
> > > +    additionalProperties: false
> > 
> > Keep it before description.
> Ok
> > 
> > > +
> > > +    patternProperties:
> > > +      '-pins$':
> > > +        type: object
> > > +        description: |
> > > +          Each subnode will list the pins it needs, and how they should
> > > +          be configured, with regard to muxer configuration, bias, input
> > > +          enable/disable, input schmitt trigger, slew-rate enable/disable,
> > > +          slew-rate, drive strength, power source.
> > > +        $ref: /schemas/pinctrl/pincfg-node.yaml
> > > +
> > > +        allOf:
> > > +          - $ref: pincfg-node.yaml#
> > > +          - $ref: pinmux-node.yaml#
> > 
> > You are duplicating refs.
> ok, will fix it
> > 
> > > +
> > > +        properties:
> > > +          pinmux:
> > > +            description: |
> > > +              The list of GPIOs and their mux settings that properties in the
> > > +              node apply to. This should be set using the K1_PADCONF macro to
> > > +              construct the value.
> > > +            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pinmux
> > 
> > Hm why you need the ref?
> > 
> will drop it
> > > +
> > > +          bias-disable: true
> > > +
> > > +          bias-pull-up: true
> > > +
> > > +          bias-pull-down: true
> > > +
> > > +          drive-strength-microamp:
> > > +            description: |
> > > +              typical current when output high level, but in mA.
> > > +              1.8V output: 11, 21, 32, 42 (mA)
> > > +              3.3V output: 7, 10, 13, 16, 19, 23, 26, 29 (mA)
> > > +            $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +          input-schmitt:
> > > +            description: |
> > > +              typical threshold for schmitt trigger.
> > > +              0: buffer mode
> > > +              1: trigger mode
> > > +              2, 3: trigger mode
> > > +            $ref: /schemas/types.yaml#/definitions/uint32
> > > +            enum: [0, 1, 2, 3]
> > > +
> > > +          power-source:
> > > +            description: external power supplies at 1.8v or 3.3v.
> > > +            enum: [ 1800, 3300 ]
> > > +
> > > +          slew-rate:
> > > +            description: |
> > > +              slew rate for output buffer
> > > +              0, 1: Slow speed
> > 
> > Hm? Surprising, 0 is slow speed?
> > 
> from docs, section 3.3.2.2 MFPR Register Description
> 0, 1 are same, both for slow speed
> https://developer.spacemit.com/documentation?token=An1vwTwKaigaXRkYfwmcznTXned
> 

I suspect this should not be set separately, but with the 
drive-strength. The document shows that the DS field sets
both drive strength and slew rate. This at least tell the
value 0 and 1 may be different.

> > > +              2: Medium speed
> > > +              3: Fast speed
> > > +            $ref: /schemas/types.yaml#/definitions/uint32
> > > +            enum: [0, 1, 2, 3]
> > > +
> > > +          spacemit,slew-rate-enable:
> > > +            description: enable slew rate.
> > 
> > The presence of slew-rate enables it, doesn't it?
> > 
> yes, this should work, I will take this approach, thanks
> 
> > > +            type: boolean
> > > +
> > > +          spacemit,slew-rate-disable:
> > > +            description: disable slew rate.
> > > +            type: boolean
> > 
> > Just use slew-rate, 0 disable, some value to match real slew-rate.
> > 
> sounds good to me, since 0, 1 indicate same meaning, can re-use 0 for
> disabling slew rate.
> 
> > > +
> > > +          spacemit,strong-pull-up:
> > > +            description: enable strong pull up.
> > 
> > Do not duplicate the property name in description. You did not say
> > anything useful here. What is "strong"? bias-pull-up takes also an argument.
> > 
> there is a dedicated strong pull bit[3] for I2C, SD card kinds of pad
> I don't know how 'strong' it is if in ohms, will see if can get
> more info on this (may expand the description)
> 
> I think using 'bias-pull-up' property with argument should also work,
> but it occur to me it's more intuitive to introduce a property here, which
> reflect the underlying hardware functionality. this is similar to starfive's jh7100
> bindings/pinctrl/starfive,jh7100-pinctrl.yaml:154
> (refer to exist code doesn't mean always correct, so I need advice here)
> 
> I will keep this property unless there is objection, please let me know
> 
> > > +            type: boolean
> > > +
> > > +        required:
> > > +          - pinmux
> > > +
> > > +        additionalProperties: false
> > 
> > This goes up, before description.
> > 
> Ok
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/pinctrl/spacemit,k1-pinctrl.h>
> > > +
> > > +    soc {
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +
> > > +        pinctrl@d401e000 {
> > > +            compatible = "spacemit,k1-pinctrl";
> > > +            reg = <0x0 0xd401e000 0x0 0x400>;
> > > +            #pinctrl-cells = <2>;
> > > +            #gpio-range-cells = <3>;
> > 
> > This wasn't ever tested... :(
> > ...
> will drop it
> > 
> > > diff --git a/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> > > new file mode 100644
> > > index 0000000000000..13ef4aa6c53a3
> > > --- /dev/null
> > > +++ b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> > > @@ -0,0 +1,161 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> > > +/*
> > > + * Copyright (c) 2022-2024 SpacemiT (Hangzhou) Technology Co. Ltd
> > > + * Copyright (c) 2024 Yixun Lan <dlan@gentoo.org>
> > > + *
> > > + */
> > > +
> > > +#ifndef _DT_BINDINGS_PINCTRL_K1_H
> > > +#define _DT_BINDINGS_PINCTRL_K1_H
> > > +
> > > +#define PINMUX(pin, mux) \
> > > +	(((pin) & 0xffff) | (((mux) & 0xff) << 16))
> > > +
> > > +/* pin offset */
> > > +#define PINID(x)	((x) + 1)
> > > +
> > > +#define GPIO_INVAL  0
> > > +#define GPIO_00     PINID(0)
> > 
> > Not really, pin numbers are not bindings. Drop entire header.
> > 
> Ok, I will move them to dts folder, which should be file
> arch/riscv/boot/dts/spacemit/k1-pinctrl.h
> 
> > ...
> > 
> > > +
> > > +#define SLEW_RATE_SLOW0		0
> > > +#define SLEW_RATE_SLOW1		1
> > > +#define SLEW_RATE_MEDIUM	2
> > > +#define SLEW_RATE_FAST		3
> > 
> > Not a binding, either. No usage in the driver.
> Ok, will drop it
> 
> > 
> > > +
> > > +#define K1_PADCONF(pin, func) (((pin) << 16) | (func))
> > 
> > Not a binding.
> > 
> same, move to dts
> 
> > 
> > 
> > Best regards,
> > Krzysztof
> 
> -- 
> Yixun Lan (dlan)
> Gentoo Linux Developer
> GPG Key ID AABEFD55
Inochi Amaoto Aug. 26, 2024, 4:23 a.m. UTC | #6
On Sun, Aug 25, 2024 at 01:10:02PM GMT, Yixun Lan wrote:
> Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
> 
> Two vendor specific properties are introduced here, As the pinctrl
> has dedicated slew rate enable control - bit[7], so we have
> spacemit,slew-rate-{enable,disable} for this. For the same reason,
> creating spacemit,strong-pull-up for the strong pull up control.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
>  include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
>  2 files changed, 295 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> new file mode 100644
> index 0000000000000..8adfc5ebbce37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 SoC Pin Controller
> +
> +maintainers:
> +  - Yixun Lan <dlan@gentoo.org>
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-pinctrl
> +
> +  reg:
> +    items:
> +      - description: pinctrl io memory base
> +
> +patternProperties:
> +  '-cfg$':
> +    type: object
> +    description: |
> +      A pinctrl node should contain at least one subnode representing the
> +      pinctrl groups available on the machine.
> +
> +    additionalProperties: false
> +
> +    patternProperties:
> +      '-pins$':
> +        type: object
> +        description: |
> +          Each subnode will list the pins it needs, and how they should
> +          be configured, with regard to muxer configuration, bias, input
> +          enable/disable, input schmitt trigger, slew-rate enable/disable,
> +          slew-rate, drive strength, power source.
> +        $ref: /schemas/pinctrl/pincfg-node.yaml
> +
> +        allOf:
> +          - $ref: pincfg-node.yaml#
> +          - $ref: pinmux-node.yaml#
> +
> +        properties:
> +          pinmux:
> +            description: |
> +              The list of GPIOs and their mux settings that properties in the
> +              node apply to. This should be set using the K1_PADCONF macro to
> +              construct the value.
> +            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pinmux
> +
> +          bias-disable: true
> +
> +          bias-pull-up: true
> +
> +          bias-pull-down: true
> +
> +          drive-strength-microamp:
> +            description: |
> +              typical current when output high level, but in mA.
> +              1.8V output: 11, 21, 32, 42 (mA)
> +              3.3V output: 7, 10, 13, 16, 19, 23, 26, 29 (mA)
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +

If there are no other runtime restrictions, you should add extra
check with the power-source, then you can always get vaild 
settings.

Regards,
Inochi

> +          input-schmitt:
> +            description: |
> +              typical threshold for schmitt trigger.
> +              0: buffer mode
> +              1: trigger mode
> +              2, 3: trigger mode
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3]
> +
> +          power-source:
> +            description: external power supplies at 1.8v or 3.3v.
> +            enum: [ 1800, 3300 ]
> +
> +          slew-rate:
> +            description: |
> +              slew rate for output buffer
> +              0, 1: Slow speed
> +              2: Medium speed
> +              3: Fast speed
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3]
> +
> +          spacemit,slew-rate-enable:
> +            description: enable slew rate.
> +            type: boolean
> +
> +          spacemit,slew-rate-disable:
> +            description: disable slew rate.
> +            type: boolean
> +
> +          spacemit,strong-pull-up:
> +            description: enable strong pull up.
> +            type: boolean
> +
> +        required:
> +          - pinmux
> +
> +        additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/pinctrl/spacemit,k1-pinctrl.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pinctrl@d401e000 {
> +            compatible = "spacemit,k1-pinctrl";
> +            reg = <0x0 0xd401e000 0x0 0x400>;
> +            #pinctrl-cells = <2>;
> +            #gpio-range-cells = <3>;
> +
> +            uart0_2_cfg: uart0-2-cfg {
> +                uart0-2-pins {
> +                    pinmux = <K1_PADCONF(GPIO_68, 2)>,
> +                             <K1_PADCONF(GPIO_69, 2)>;
> +                    bias-pull-up;
> +                    drive-strength-microamp = <32>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> new file mode 100644
> index 0000000000000..13ef4aa6c53a3
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
> @@ -0,0 +1,161 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * Copyright (c) 2022-2024 SpacemiT (Hangzhou) Technology Co. Ltd
> + * Copyright (c) 2024 Yixun Lan <dlan@gentoo.org>
> + *
> + */
> +
> +#ifndef _DT_BINDINGS_PINCTRL_K1_H
> +#define _DT_BINDINGS_PINCTRL_K1_H
> +
> +#define PINMUX(pin, mux) \
> +	(((pin) & 0xffff) | (((mux) & 0xff) << 16))
> +
> +/* pin offset */
> +#define PINID(x)	((x) + 1)
> +
> +#define GPIO_INVAL  0
> +#define GPIO_00     PINID(0)
> +#define GPIO_01     PINID(1)
> +#define GPIO_02     PINID(2)
> +#define GPIO_03     PINID(3)
> +#define GPIO_04     PINID(4)
> +#define GPIO_05     PINID(5)
> +#define GPIO_06     PINID(6)
> +#define GPIO_07     PINID(7)
> +#define GPIO_08     PINID(8)
> +#define GPIO_09     PINID(9)
> +#define GPIO_10     PINID(10)
> +#define GPIO_11     PINID(11)
> +#define GPIO_12     PINID(12)
> +#define GPIO_13     PINID(13)
> +#define GPIO_14     PINID(14)
> +#define GPIO_15     PINID(15)
> +#define GPIO_16     PINID(16)
> +#define GPIO_17     PINID(17)
> +#define GPIO_18     PINID(18)
> +#define GPIO_19     PINID(19)
> +#define GPIO_20     PINID(20)
> +#define GPIO_21     PINID(21)
> +#define GPIO_22     PINID(22)
> +#define GPIO_23     PINID(23)
> +#define GPIO_24     PINID(24)
> +#define GPIO_25     PINID(25)
> +#define GPIO_26     PINID(26)
> +#define GPIO_27     PINID(27)
> +#define GPIO_28     PINID(28)
> +#define GPIO_29     PINID(29)
> +#define GPIO_30     PINID(30)
> +#define GPIO_31     PINID(31)
> +
> +#define GPIO_32     PINID(32)
> +#define GPIO_33     PINID(33)
> +#define GPIO_34     PINID(34)
> +#define GPIO_35     PINID(35)
> +#define GPIO_36     PINID(36)
> +#define GPIO_37     PINID(37)
> +#define GPIO_38     PINID(38)
> +#define GPIO_39     PINID(39)
> +#define GPIO_40     PINID(40)
> +#define GPIO_41     PINID(41)
> +#define GPIO_42     PINID(42)
> +#define GPIO_43     PINID(43)
> +#define GPIO_44     PINID(44)
> +#define GPIO_45     PINID(45)
> +#define GPIO_46     PINID(46)
> +#define GPIO_47     PINID(47)
> +#define GPIO_48     PINID(48)
> +#define GPIO_49     PINID(49)
> +#define GPIO_50     PINID(50)
> +#define GPIO_51     PINID(51)
> +#define GPIO_52     PINID(52)
> +#define GPIO_53     PINID(53)
> +#define GPIO_54     PINID(54)
> +#define GPIO_55     PINID(55)
> +#define GPIO_56     PINID(56)
> +#define GPIO_57     PINID(57)
> +#define GPIO_58     PINID(58)
> +#define GPIO_59     PINID(59)
> +#define GPIO_60     PINID(60)
> +#define GPIO_61     PINID(61)
> +#define GPIO_62     PINID(62)
> +#define GPIO_63     PINID(63)
> +
> +#define GPIO_64     PINID(64)
> +#define GPIO_65     PINID(65)
> +#define GPIO_66     PINID(66)
> +#define GPIO_67     PINID(67)
> +#define GPIO_68     PINID(68)
> +#define GPIO_69     PINID(69)
> +#define GPIO_70     PINID(70)
> +#define GPIO_71     PINID(71)
> +#define GPIO_72     PINID(72)
> +#define GPIO_73     PINID(73)
> +#define GPIO_74     PINID(74)
> +#define GPIO_75     PINID(75)
> +#define GPIO_76     PINID(76)
> +#define GPIO_77     PINID(77)
> +#define GPIO_78     PINID(78)
> +#define GPIO_79     PINID(79)
> +#define GPIO_80     PINID(80)
> +#define GPIO_81     PINID(81)
> +#define GPIO_82     PINID(82)
> +#define GPIO_83     PINID(83)
> +#define GPIO_84     PINID(84)
> +#define GPIO_85     PINID(85)
> +
> +#define GPIO_101    PINID(89)
> +#define GPIO_100    PINID(90)
> +#define GPIO_99     PINID(91)
> +#define GPIO_98     PINID(92)
> +#define GPIO_103    PINID(93)
> +#define GPIO_102    PINID(94)
> +
> +#define GPIO_104    PINID(109)
> +#define GPIO_105    PINID(110)
> +#define GPIO_106    PINID(111)
> +#define GPIO_107    PINID(112)
> +#define GPIO_108    PINID(113)
> +#define GPIO_109    PINID(114)
> +#define GPIO_110    PINID(115)
> +
> +#define GPIO_93     PINID(116)
> +#define GPIO_94     PINID(117)
> +#define GPIO_95     PINID(118)
> +#define GPIO_96     PINID(119)
> +#define GPIO_97     PINID(120)
> +
> +#define GPIO_86     PINID(122)
> +#define GPIO_87     PINID(123)
> +#define GPIO_88     PINID(124)
> +#define GPIO_89     PINID(125)
> +#define GPIO_90     PINID(126)
> +#define GPIO_91     PINID(127)
> +#define GPIO_92     PINID(128)
> +
> +#define GPIO_111    PINID(130)
> +#define GPIO_112    PINID(131)
> +#define GPIO_113    PINID(132)
> +#define GPIO_114    PINID(133)
> +#define GPIO_115    PINID(134)
> +#define GPIO_116    PINID(135)
> +#define GPIO_117    PINID(136)
> +#define GPIO_118    PINID(137)
> +#define GPIO_119    PINID(138)
> +#define GPIO_120    PINID(139)
> +#define GPIO_121    PINID(140)
> +#define GPIO_122    PINID(141)
> +#define GPIO_123    PINID(142)
> +#define GPIO_124    PINID(143)
> +#define GPIO_125    PINID(144)
> +#define GPIO_126    PINID(145)
> +#define GPIO_127    PINID(146)
> +
> +#define SLEW_RATE_SLOW0		0
> +#define SLEW_RATE_SLOW1		1
> +#define SLEW_RATE_MEDIUM	2
> +#define SLEW_RATE_FAST		3
> +
> +#define K1_PADCONF(pin, func) (((pin) << 16) | (func))
> +
> +#endif /* _DT_BINDINGS_PINCTRL_K1_H */
> 
> -- 
> 2.45.2
>
Krzysztof Kozlowski Aug. 26, 2024, 5:37 a.m. UTC | #7
On 26/08/2024 03:36, Yixun Lan wrote:
> Hi Krzysztof: 
> 
> On 15:48 Sun 25 Aug     , Krzysztof Kozlowski wrote:
>> On 25/08/2024 15:10, Yixun Lan wrote:
>>> Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
>>
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching. For bindings, the preferred subjects are
>> explained here:
>> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>>
>> It's "dt-bindings:"
> Ok, will fix in next version
> 
>>
>>>
>>> Two vendor specific properties are introduced here, As the pinctrl
>>> has dedicated slew rate enable control - bit[7], so we have
>>> spacemit,slew-rate-{enable,disable} for this. For the same reason,
>>> creating spacemit,strong-pull-up for the strong pull up control.
>>
>> Huh, no, use generic properties. More on that below
>>
> see my reply below
> 
>>
>>
>>>
>>> Signed-off-by: Yixun Lan <dlan@gentoo.org>
>>> ---
>>>  .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
>>>  include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
>>>  2 files changed, 295 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
>>> new file mode 100644
>>> index 0000000000000..8adfc5ebbce37
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
>>> @@ -0,0 +1,134 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: SpacemiT K1 SoC Pin Controller
>>> +
>>> +maintainers:
>>> +  - Yixun Lan <dlan@gentoo.org>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: spacemit,k1-pinctrl
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: pinctrl io memory base
>>> +
>>> +patternProperties:
>>> +  '-cfg$':
>>> +    type: object
>>> +    description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
> Ok
>>> +      A pinctrl node should contain at least one subnode representing the
>>> +      pinctrl groups available on the machine.
>>> +
>>> +    additionalProperties: false
>>
>> Keep it before description.
> Ok
>>
>>> +
>>> +    patternProperties:
>>> +      '-pins$':
>>> +        type: object
>>> +        description: |
>>> +          Each subnode will list the pins it needs, and how they should
>>> +          be configured, with regard to muxer configuration, bias, input
>>> +          enable/disable, input schmitt trigger, slew-rate enable/disable,
>>> +          slew-rate, drive strength, power source.
>>> +        $ref: /schemas/pinctrl/pincfg-node.yaml
>>> +
>>> +        allOf:
>>> +          - $ref: pincfg-node.yaml#
>>> +          - $ref: pinmux-node.yaml#
>>
>> You are duplicating refs.
> ok, will fix it
>>
>>> +
>>> +        properties:
>>> +          pinmux:
>>> +            description: |
>>> +              The list of GPIOs and their mux settings that properties in the
>>> +              node apply to. This should be set using the K1_PADCONF macro to
>>> +              construct the value.
>>> +            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pinmux
>>
>> Hm why you need the ref?
>>
> will drop it
>>> +
>>> +          bias-disable: true
>>> +
>>> +          bias-pull-up: true
>>> +
>>> +          bias-pull-down: true
>>> +
>>> +          drive-strength-microamp:
>>> +            description: |
>>> +              typical current when output high level, but in mA.
>>> +              1.8V output: 11, 21, 32, 42 (mA)
>>> +              3.3V output: 7, 10, 13, 16, 19, 23, 26, 29 (mA)
>>> +            $ref: /schemas/types.yaml#/definitions/uint32
>>> +
>>> +          input-schmitt:
>>> +            description: |
>>> +              typical threshold for schmitt trigger.
>>> +              0: buffer mode
>>> +              1: trigger mode
>>> +              2, 3: trigger mode
>>> +            $ref: /schemas/types.yaml#/definitions/uint32
>>> +            enum: [0, 1, 2, 3]
>>> +
>>> +          power-source:
>>> +            description: external power supplies at 1.8v or 3.3v.
>>> +            enum: [ 1800, 3300 ]
>>> +
>>> +          slew-rate:
>>> +            description: |
>>> +              slew rate for output buffer
>>> +              0, 1: Slow speed
>>
>> Hm? Surprising, 0 is slow speed?
>>
> from docs, section 3.3.2.2 MFPR Register Description
> 0, 1 are same, both for slow speed
> https://developer.spacemit.com/documentation?token=An1vwTwKaigaXRkYfwmcznTXned

Don't store here register value.

> 
>>> +              2: Medium speed
>>> +              3: Fast speed
>>> +            $ref: /schemas/types.yaml#/definitions/uint32
>>> +            enum: [0, 1, 2, 3]
>>> +
>>> +          spacemit,slew-rate-enable:
>>> +            description: enable slew rate.
>>
>> The presence of slew-rate enables it, doesn't it?
>>
> yes, this should work, I will take this approach, thanks
> 
>>> +            type: boolean
>>> +
>>> +          spacemit,slew-rate-disable:
>>> +            description: disable slew rate.
>>> +            type: boolean
>>
>> Just use slew-rate, 0 disable, some value to match real slew-rate.
>>
> sounds good to me, since 0, 1 indicate same meaning, can re-use 0 for
> disabling slew rate.
> 
>>> +
>>> +          spacemit,strong-pull-up:
>>> +            description: enable strong pull up.
>>
>> Do not duplicate the property name in description. You did not say
>> anything useful here. What is "strong"? bias-pull-up takes also an argument.
>>
> there is a dedicated strong pull bit[3] for I2C, SD card kinds of pad
> I don't know how 'strong' it is if in ohms, will see if can get
> more info on this (may expand the description)
> 
> I think using 'bias-pull-up' property with argument should also work,
> but it occur to me it's more intuitive to introduce a property here, which
> reflect the underlying hardware functionality. this is similar to starfive's jh7100
> bindings/pinctrl/starfive,jh7100-pinctrl.yaml:154
> (refer to exist code doesn't mean always correct, so I need advice here)

No, avoid introducing properties which duplicate existing generic ones.

Same story was with qualcomm and it was possible to use specific value.

> 
> I will keep this property unless there is objection, please let me know
> 
>>> +            type: boolean
>>> +
>>> +        required:
>>> +          - pinmux
>>> +
>>> +        additionalProperties: false
>>
>> This goes up, before description.
>>
> Ok
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/pinctrl/spacemit,k1-pinctrl.h>
>>> +
>>> +    soc {
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +
>>> +        pinctrl@d401e000 {
>>> +            compatible = "spacemit,k1-pinctrl";
>>> +            reg = <0x0 0xd401e000 0x0 0x400>;
>>> +            #pinctrl-cells = <2>;
>>> +            #gpio-range-cells = <3>;
>>
>> This wasn't ever tested... :(
>> ...
> will drop it

Test your patches instead.



Best regards,
Krzysztof
Conor Dooley Aug. 26, 2024, 4:22 p.m. UTC | #8
On Mon, Aug 26, 2024 at 03:09:39AM +0000, Yixun Lan wrote:
> 
> On 13:10 Sun 25 Aug     , Yixun Lan wrote:
> > Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
> > 
> > Two vendor specific properties are introduced here, As the pinctrl
> > has dedicated slew rate enable control - bit[7], so we have
> > spacemit,slew-rate-{enable,disable} for this. For the same reason,
> > creating spacemit,strong-pull-up for the strong pull up control.
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>

I got this mail, and one of your other ones, 5 times. What's going wrong
with your mail setup? 

| 250   T Aug 25 Yixun Lan       (6.0K) ┌─>[PATCH v2 4/4] riscv: dts: spacemit: add pinctrl property to uart0 in BPI-F3
| 251 N T Aug 26 Inochi Amaoto   ( 12K) │ ┌─>
| 252   T Aug 25 Yixun Lan       (6.8K) ├─>[PATCH v2 3/4] riscv: dts: spacemit: add pinctrl support for K1 SoC
| 253 N T Aug 26 Inochi Amaoto   ( 46K) │ ┌─>
| 254   T Aug 25 Yixun Lan       ( 38K) ├─>[PATCH v2 2/4] pinctrl: spacemit: add support for SpacemiT K1 SoC
| 255 N T Aug 26 Inochi Amaoto   ( 22K) │ ┌─>
| 256   T Aug 26 Yixun Lan       ( 333) │ ├─>
| 257   T Aug 26 Yixun Lan       ( 338) │ ├─>
| 258   T Aug 26 Yixun Lan       ( 334) │ ├─>
| 259   T Aug 26 Yixun Lan       ( 334) │ ├─>
| 260   T Aug 26 Yixun Lan       ( 333) │ ├─>
| 261   C Aug 25 Rob Herring (Ar (9.0K) │ ├─>
| 262 N C Aug 26 Krzysztof Kozlo ( 14K) │ │ ┌─>
| 263 N C Aug 26 Inochi Amaoto   ( 19K) │ │ ├─>
| 264   C Aug 26 Yixun Lan       ( 285) │ │ ├─>
| 265   C Aug 26 Yixun Lan       ( 281) │ │ ├─>
| 266 N C Aug 26 Yixun Lan       ( 14K) │ │ ├─>
| 267 N C Aug 26 Yixun Lan       ( 12K) │ │ ├─>
| 268 N C Aug 26 Yixun Lan       ( 12K) │ │ ├─>
| 269   T Aug 25 Krzysztof Kozlo ( 13K) │ ├─>
| 270   T Aug 25 Yixun Lan       ( 14K) ├─>[PATCH v2 1/4] dt-binding: pinctrl: spacemit: add documents for K1 SoC
| 271   T Aug 25 Yixun Lan       (8.5K) [PATCH v2 0/4] riscv: spacemit: add pinctrl support to K1 SoC
Yixun Lan Aug. 27, 2024, 2:23 a.m. UTC | #9
Hi Conor:

On 17:22 Mon 26 Aug     , Conor Dooley wrote:
> On Mon, Aug 26, 2024 at 03:09:39AM +0000, Yixun Lan wrote:
> > 
> > On 13:10 Sun 25 Aug     , Yixun Lan wrote:
> > > Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
> > > 
> > > Two vendor specific properties are introduced here, As the pinctrl
> > > has dedicated slew rate enable control - bit[7], so we have
> > > spacemit,slew-rate-{enable,disable} for this. For the same reason,
> > > creating spacemit,strong-pull-up for the strong pull up control.
> > > 
> > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> 
> I got this mail, and one of your other ones, 5 times. What's going wrong
> with your mail setup? 
> 
Oops, sorry for this, it's the second time you complain this..
TBO, I have no idea what's happened, asked Yangyu, he didn't have this problem

I'm using mutt+msmtp to reply, while using b4 to send the patch series,
for all the mails you received, do they all have same message-id?
I have a local filter for duplicated mails myself, could this help?

(I leave only one address of your mail in this reply, see if still have problem?)

> | 250   T Aug 25 Yixun Lan       (6.0K) ┌─>[PATCH v2 4/4] riscv: dts: spacemit: add pinctrl property to uart0 in BPI-F3
> | 251 N T Aug 26 Inochi Amaoto   ( 12K) │ ┌─>
> | 252   T Aug 25 Yixun Lan       (6.8K) ├─>[PATCH v2 3/4] riscv: dts: spacemit: add pinctrl support for K1 SoC
> | 253 N T Aug 26 Inochi Amaoto   ( 46K) │ ┌─>
> | 254   T Aug 25 Yixun Lan       ( 38K) ├─>[PATCH v2 2/4] pinctrl: spacemit: add support for SpacemiT K1 SoC
> | 255 N T Aug 26 Inochi Amaoto   ( 22K) │ ┌─>
> | 256   T Aug 26 Yixun Lan       ( 333) │ ├─>
> | 257   T Aug 26 Yixun Lan       ( 338) │ ├─>
> | 258   T Aug 26 Yixun Lan       ( 334) │ ├─>
> | 259   T Aug 26 Yixun Lan       ( 334) │ ├─>
> | 260   T Aug 26 Yixun Lan       ( 333) │ ├─>
> | 261   C Aug 25 Rob Herring (Ar (9.0K) │ ├─>
> | 262 N C Aug 26 Krzysztof Kozlo ( 14K) │ │ ┌─>
> | 263 N C Aug 26 Inochi Amaoto   ( 19K) │ │ ├─>
> | 264   C Aug 26 Yixun Lan       ( 285) │ │ ├─>
> | 265   C Aug 26 Yixun Lan       ( 281) │ │ ├─>
> | 266 N C Aug 26 Yixun Lan       ( 14K) │ │ ├─>
> | 267 N C Aug 26 Yixun Lan       ( 12K) │ │ ├─>
> | 268 N C Aug 26 Yixun Lan       ( 12K) │ │ ├─>
> | 269   T Aug 25 Krzysztof Kozlo ( 13K) │ ├─>
> | 270   T Aug 25 Yixun Lan       ( 14K) ├─>[PATCH v2 1/4] dt-binding: pinctrl: spacemit: add documents for K1 SoC
> | 271   T Aug 25 Yixun Lan       (8.5K) [PATCH v2 0/4] riscv: spacemit: add pinctrl support to K1 SoC
>
Conor Dooley Aug. 27, 2024, 3:13 p.m. UTC | #10
On Tue, Aug 27, 2024 at 02:23:05AM +0000, Yixun Lan wrote:
> Hi Conor:
> 
> On 17:22 Mon 26 Aug     , Conor Dooley wrote:
> > On Mon, Aug 26, 2024 at 03:09:39AM +0000, Yixun Lan wrote:
> > > 
> > > On 13:10 Sun 25 Aug     , Yixun Lan wrote:
> > > > Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.
> > > > 
> > > > Two vendor specific properties are introduced here, As the pinctrl
> > > > has dedicated slew rate enable control - bit[7], so we have
> > > > spacemit,slew-rate-{enable,disable} for this. For the same reason,
> > > > creating spacemit,strong-pull-up for the strong pull up control.
> > > > 
> > > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > 
> > I got this mail, and one of your other ones, 5 times. What's going wrong
> > with your mail setup? 
> > 
> Oops, sorry for this, it's the second time you complain this..
> TBO, I have no idea what's happened, asked Yangyu, he didn't have this problem
> 
> I'm using mutt+msmtp to reply, while using b4 to send the patch series,

The patches only appear once, it's the replies that have the problem.

> for all the mails you received, do they all have same message-id?
> I have a local filter for duplicated mails myself, could this help?
> 
> (I leave only one address of your mail in this reply, see if still have problem?)

It's nothing to do with me having multiple addresses, the mails have
different message ids, and as you can see below, even different sizes too.
For example, two copies of the current reply came in as:
Message-ID: <66cd38b0.050a0220.113bce.d5acSMTPIN_ADDED_BROKEN@mx.google.com>
Message-ID: <66cd3abe.050a0220.bf184.b4feSMTPIN_ADDED_BROKEN@mx.google.com>

Your mails also don't properly appear on lore, you can see some [not
found] mails there:
https://lore.kernel.org/linux-devicetree/20240827033057.GYB208832.dlan.gentoo/

The SMPTIN_ADDED_BROKEN might be a hint as to what is wrong with your
setup?

> 
> > | 250   T Aug 25 Yixun Lan       (6.0K) ┌─>[PATCH v2 4/4] riscv: dts: spacemit: add pinctrl property to uart0 in BPI-F3
> > | 251 N T Aug 26 Inochi Amaoto   ( 12K) │ ┌─>
> > | 252   T Aug 25 Yixun Lan       (6.8K) ├─>[PATCH v2 3/4] riscv: dts: spacemit: add pinctrl support for K1 SoC
> > | 253 N T Aug 26 Inochi Amaoto   ( 46K) │ ┌─>
> > | 254   T Aug 25 Yixun Lan       ( 38K) ├─>[PATCH v2 2/4] pinctrl: spacemit: add support for SpacemiT K1 SoC
> > | 255 N T Aug 26 Inochi Amaoto   ( 22K) │ ┌─>
> > | 256   T Aug 26 Yixun Lan       ( 333) │ ├─>
> > | 257   T Aug 26 Yixun Lan       ( 338) │ ├─>
> > | 258   T Aug 26 Yixun Lan       ( 334) │ ├─>
> > | 259   T Aug 26 Yixun Lan       ( 334) │ ├─>
> > | 260   T Aug 26 Yixun Lan       ( 333) │ ├─>
> > | 261   C Aug 25 Rob Herring (Ar (9.0K) │ ├─>
> > | 262 N C Aug 26 Krzysztof Kozlo ( 14K) │ │ ┌─>
> > | 263 N C Aug 26 Inochi Amaoto   ( 19K) │ │ ├─>
> > | 264   C Aug 26 Yixun Lan       ( 285) │ │ ├─>
> > | 265   C Aug 26 Yixun Lan       ( 281) │ │ ├─>
> > | 266 N C Aug 26 Yixun Lan       ( 14K) │ │ ├─>
> > | 267 N C Aug 26 Yixun Lan       ( 12K) │ │ ├─>
> > | 268 N C Aug 26 Yixun Lan       ( 12K) │ │ ├─>
> > | 269   T Aug 25 Krzysztof Kozlo ( 13K) │ ├─>
> > | 270   T Aug 25 Yixun Lan       ( 14K) ├─>[PATCH v2 1/4] dt-binding: pinctrl: spacemit: add documents for K1 SoC
> > | 271   T Aug 25 Yixun Lan       (8.5K) [PATCH v2 0/4] riscv: spacemit: add pinctrl support to K1 SoC
> > 
> 
> 
> -- 
> Yixun Lan (dlan)
> Gentoo Linux Developer
> GPG Key ID AABEFD55
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
new file mode 100644
index 0000000000000..8adfc5ebbce37
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
@@ -0,0 +1,134 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 SoC Pin Controller
+
+maintainers:
+  - Yixun Lan <dlan@gentoo.org>
+
+properties:
+  compatible:
+    const: spacemit,k1-pinctrl
+
+  reg:
+    items:
+      - description: pinctrl io memory base
+
+patternProperties:
+  '-cfg$':
+    type: object
+    description: |
+      A pinctrl node should contain at least one subnode representing the
+      pinctrl groups available on the machine.
+
+    additionalProperties: false
+
+    patternProperties:
+      '-pins$':
+        type: object
+        description: |
+          Each subnode will list the pins it needs, and how they should
+          be configured, with regard to muxer configuration, bias, input
+          enable/disable, input schmitt trigger, slew-rate enable/disable,
+          slew-rate, drive strength, power source.
+        $ref: /schemas/pinctrl/pincfg-node.yaml
+
+        allOf:
+          - $ref: pincfg-node.yaml#
+          - $ref: pinmux-node.yaml#
+
+        properties:
+          pinmux:
+            description: |
+              The list of GPIOs and their mux settings that properties in the
+              node apply to. This should be set using the K1_PADCONF macro to
+              construct the value.
+            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pinmux
+
+          bias-disable: true
+
+          bias-pull-up: true
+
+          bias-pull-down: true
+
+          drive-strength-microamp:
+            description: |
+              typical current when output high level, but in mA.
+              1.8V output: 11, 21, 32, 42 (mA)
+              3.3V output: 7, 10, 13, 16, 19, 23, 26, 29 (mA)
+            $ref: /schemas/types.yaml#/definitions/uint32
+
+          input-schmitt:
+            description: |
+              typical threshold for schmitt trigger.
+              0: buffer mode
+              1: trigger mode
+              2, 3: trigger mode
+            $ref: /schemas/types.yaml#/definitions/uint32
+            enum: [0, 1, 2, 3]
+
+          power-source:
+            description: external power supplies at 1.8v or 3.3v.
+            enum: [ 1800, 3300 ]
+
+          slew-rate:
+            description: |
+              slew rate for output buffer
+              0, 1: Slow speed
+              2: Medium speed
+              3: Fast speed
+            $ref: /schemas/types.yaml#/definitions/uint32
+            enum: [0, 1, 2, 3]
+
+          spacemit,slew-rate-enable:
+            description: enable slew rate.
+            type: boolean
+
+          spacemit,slew-rate-disable:
+            description: disable slew rate.
+            type: boolean
+
+          spacemit,strong-pull-up:
+            description: enable strong pull up.
+            type: boolean
+
+        required:
+          - pinmux
+
+        additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/pinctrl/spacemit,k1-pinctrl.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pinctrl@d401e000 {
+            compatible = "spacemit,k1-pinctrl";
+            reg = <0x0 0xd401e000 0x0 0x400>;
+            #pinctrl-cells = <2>;
+            #gpio-range-cells = <3>;
+
+            uart0_2_cfg: uart0-2-cfg {
+                uart0-2-pins {
+                    pinmux = <K1_PADCONF(GPIO_68, 2)>,
+                             <K1_PADCONF(GPIO_69, 2)>;
+                    bias-pull-up;
+                    drive-strength-microamp = <32>;
+                };
+            };
+        };
+    };
+
+...
diff --git a/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
new file mode 100644
index 0000000000000..13ef4aa6c53a3
--- /dev/null
+++ b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h
@@ -0,0 +1,161 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Copyright (c) 2022-2024 SpacemiT (Hangzhou) Technology Co. Ltd
+ * Copyright (c) 2024 Yixun Lan <dlan@gentoo.org>
+ *
+ */
+
+#ifndef _DT_BINDINGS_PINCTRL_K1_H
+#define _DT_BINDINGS_PINCTRL_K1_H
+
+#define PINMUX(pin, mux) \
+	(((pin) & 0xffff) | (((mux) & 0xff) << 16))
+
+/* pin offset */
+#define PINID(x)	((x) + 1)
+
+#define GPIO_INVAL  0
+#define GPIO_00     PINID(0)
+#define GPIO_01     PINID(1)
+#define GPIO_02     PINID(2)
+#define GPIO_03     PINID(3)
+#define GPIO_04     PINID(4)
+#define GPIO_05     PINID(5)
+#define GPIO_06     PINID(6)
+#define GPIO_07     PINID(7)
+#define GPIO_08     PINID(8)
+#define GPIO_09     PINID(9)
+#define GPIO_10     PINID(10)
+#define GPIO_11     PINID(11)
+#define GPIO_12     PINID(12)
+#define GPIO_13     PINID(13)
+#define GPIO_14     PINID(14)
+#define GPIO_15     PINID(15)
+#define GPIO_16     PINID(16)
+#define GPIO_17     PINID(17)
+#define GPIO_18     PINID(18)
+#define GPIO_19     PINID(19)
+#define GPIO_20     PINID(20)
+#define GPIO_21     PINID(21)
+#define GPIO_22     PINID(22)
+#define GPIO_23     PINID(23)
+#define GPIO_24     PINID(24)
+#define GPIO_25     PINID(25)
+#define GPIO_26     PINID(26)
+#define GPIO_27     PINID(27)
+#define GPIO_28     PINID(28)
+#define GPIO_29     PINID(29)
+#define GPIO_30     PINID(30)
+#define GPIO_31     PINID(31)
+
+#define GPIO_32     PINID(32)
+#define GPIO_33     PINID(33)
+#define GPIO_34     PINID(34)
+#define GPIO_35     PINID(35)
+#define GPIO_36     PINID(36)
+#define GPIO_37     PINID(37)
+#define GPIO_38     PINID(38)
+#define GPIO_39     PINID(39)
+#define GPIO_40     PINID(40)
+#define GPIO_41     PINID(41)
+#define GPIO_42     PINID(42)
+#define GPIO_43     PINID(43)
+#define GPIO_44     PINID(44)
+#define GPIO_45     PINID(45)
+#define GPIO_46     PINID(46)
+#define GPIO_47     PINID(47)
+#define GPIO_48     PINID(48)
+#define GPIO_49     PINID(49)
+#define GPIO_50     PINID(50)
+#define GPIO_51     PINID(51)
+#define GPIO_52     PINID(52)
+#define GPIO_53     PINID(53)
+#define GPIO_54     PINID(54)
+#define GPIO_55     PINID(55)
+#define GPIO_56     PINID(56)
+#define GPIO_57     PINID(57)
+#define GPIO_58     PINID(58)
+#define GPIO_59     PINID(59)
+#define GPIO_60     PINID(60)
+#define GPIO_61     PINID(61)
+#define GPIO_62     PINID(62)
+#define GPIO_63     PINID(63)
+
+#define GPIO_64     PINID(64)
+#define GPIO_65     PINID(65)
+#define GPIO_66     PINID(66)
+#define GPIO_67     PINID(67)
+#define GPIO_68     PINID(68)
+#define GPIO_69     PINID(69)
+#define GPIO_70     PINID(70)
+#define GPIO_71     PINID(71)
+#define GPIO_72     PINID(72)
+#define GPIO_73     PINID(73)
+#define GPIO_74     PINID(74)
+#define GPIO_75     PINID(75)
+#define GPIO_76     PINID(76)
+#define GPIO_77     PINID(77)
+#define GPIO_78     PINID(78)
+#define GPIO_79     PINID(79)
+#define GPIO_80     PINID(80)
+#define GPIO_81     PINID(81)
+#define GPIO_82     PINID(82)
+#define GPIO_83     PINID(83)
+#define GPIO_84     PINID(84)
+#define GPIO_85     PINID(85)
+
+#define GPIO_101    PINID(89)
+#define GPIO_100    PINID(90)
+#define GPIO_99     PINID(91)
+#define GPIO_98     PINID(92)
+#define GPIO_103    PINID(93)
+#define GPIO_102    PINID(94)
+
+#define GPIO_104    PINID(109)
+#define GPIO_105    PINID(110)
+#define GPIO_106    PINID(111)
+#define GPIO_107    PINID(112)
+#define GPIO_108    PINID(113)
+#define GPIO_109    PINID(114)
+#define GPIO_110    PINID(115)
+
+#define GPIO_93     PINID(116)
+#define GPIO_94     PINID(117)
+#define GPIO_95     PINID(118)
+#define GPIO_96     PINID(119)
+#define GPIO_97     PINID(120)
+
+#define GPIO_86     PINID(122)
+#define GPIO_87     PINID(123)
+#define GPIO_88     PINID(124)
+#define GPIO_89     PINID(125)
+#define GPIO_90     PINID(126)
+#define GPIO_91     PINID(127)
+#define GPIO_92     PINID(128)
+
+#define GPIO_111    PINID(130)
+#define GPIO_112    PINID(131)
+#define GPIO_113    PINID(132)
+#define GPIO_114    PINID(133)
+#define GPIO_115    PINID(134)
+#define GPIO_116    PINID(135)
+#define GPIO_117    PINID(136)
+#define GPIO_118    PINID(137)
+#define GPIO_119    PINID(138)
+#define GPIO_120    PINID(139)
+#define GPIO_121    PINID(140)
+#define GPIO_122    PINID(141)
+#define GPIO_123    PINID(142)
+#define GPIO_124    PINID(143)
+#define GPIO_125    PINID(144)
+#define GPIO_126    PINID(145)
+#define GPIO_127    PINID(146)
+
+#define SLEW_RATE_SLOW0		0
+#define SLEW_RATE_SLOW1		1
+#define SLEW_RATE_MEDIUM	2
+#define SLEW_RATE_FAST		3
+
+#define K1_PADCONF(pin, func) (((pin) << 16) | (func))
+
+#endif /* _DT_BINDINGS_PINCTRL_K1_H */