| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<IA1PR20MB49536CC76EFDC406497844D4BB8B2@IA1PR20MB4953.namprd20.prod.outlook.com>
Date: Mon, 26 Aug 2024 12:19:12 +0800
From: Inochi Amaoto <inochiama@...look.com>
To: Yixun Lan <dlan@...too.org>, Krzysztof Kozlowski <krzk@...nel.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Conor Dooley <conor@...nel.org>, Yangyu Chen <cyy@...self.name>, Jesse Taube <jesse@...osinc.com>,
Jisheng Zhang <jszhang@...nel.org>, Inochi Amaoto <inochiama@...look.com>,
Icenowy Zheng <uwu@...nowy.me>, Meng Zhang <zhangmeng.kevin@...cemit.com>,
Meng Zhang <kevin.z.m@...mail.com>, devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 1/4] dt-binding: pinctrl: spacemit: add documents for
K1 SoC
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@...too.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@...too.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@...1e000 {
> > > + 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@...too.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
Powered by blists - more mailing lists