| 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: <6185966d-ad60-48e3-b4a2-3808f3b53b3c@amlogic.com>
Date: Wed, 18 Dec 2024 16:47:10 +0800
From: Xianwei Zhao <xianwei.zhao@...ogic.com>
To: Rob Herring <robh@...nel.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>,
Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic
SoCs
Hi Rob,
Thanks for your reply.
On 2024/12/17 22:20, Rob Herring wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed, Dec 11, 2024 at 02:47:49PM +0800, Xianwei Zhao wrote:
>> Add the dt-bindings for Amlogic pin controller, and add a new
>> dt-binding header file which document the GPIO bank names and
>> alternative func value of all Amlogic subsequent SoCs.
>>
>> Signed-off-by: Xianwei Zhao <xianwei.zhao@...ogic.com>
>> ---
>> .../bindings/pinctrl/amlogic,pinctrl.yaml | 150 +++++++++++++++++++++
>> include/dt-bindings/pinctrl/amlogic,pinctrl.h | 68 ++++++++++
>> 2 files changed, 218 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml
>> new file mode 100644
>> index 000000000000..b0c2ae585d7d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml
>> @@ -0,0 +1,150 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/amlogic,pinctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic pinmux controller
>> +
>> +maintainers:
>> + - Xianwei Zhao <xianwei.zhao@...ogic.com>
>> +
>> +allOf:
>> + - $ref: pinctrl.yaml#
>> +
>> +properties:
>> + compatible:
>> + const: amlogic,pinctrl
>
> Only one Amlogic pinctrl block? Pretty sure there's more than 1 SoC and
> pinctrl is unlikely identical for all. This must be SoC specific.
>
Will do, "amlogic,pinctrl" -> "amlogic,pinctrl-a4".
>> +
>> + reg:
>> + minItems: 2
>> +
>> + reg-names:
>> + items:
>> + - const: mux
>> + - const: gpio
>> +
>> + "#address-cells":
>> + const: 2
>> +
>> + "#size-cells":
>> + const: 2
>
> Why do you need these? You don't have any child nodes with addresses.
> But maybe you should?
>
I will modify "reg_mux_offset" to "reg", So
#address-cells = <1>
#size-cells = <0>
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - reg-names
>> + - "#address-cells"
>> + - "#size-cells"
>> +
>> +patternProperties:
>> + "^gpio[a-z]":
>
> This allows "gpioanythingyouwant" for example. If there is no
> unit-address, then it should be '^gpio-[0-9]+$'.
>
Will do.
>> + type: object
>> +
>> + properties:
>> + gpio-controller: true
>> +
>> + "#gpio-cells":
>> + const: 2
>> +
>> + bank-name:
>> + $ref: /schemas/types.yaml#/definitions/string
>
> You must define values, but why do you need this? No other GPIO
> controller needs something like this.
>
We define the bank name(GPIOA,GPIOB) on the chip specification, so it is
more readable to describe the source code with such a name in the
pin(GPIOA-0,GPIOA-1).
Bank indexes are defined primarily for use in pinmux implicit queries.
Bank-name and bank-index, they both describe the characteristics of the
Bank, they just use it differently. I can keep bank-index, I can query
the bank-name through bank-index.
I will delete this property, keep bank-index property.
>> +
>> + npins:
>
> We have a standard property for this: ngpios
>
Will do.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + minimum: 1
>> + maximum: 32
>> +
>> + bank-index:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + minimum: 0
>> + maximum: 29
>
> Consider if 'bank' should be part of the gpio cells rather than
> separate controller nodes. That works better if each bank is not its own
> separate h/w block. If banks share a register, then that's not separate
> blocks.
>
Each bank is its own separate h/w block(With TEST_N bank exception,it
share mux reg with AO bank).
Change the name of this property to "bank-identity" make it more
appropriate.
>> +
>> + reg-mux-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> + bit-mux-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> + reg-gpio-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>
> Put these into 'reg'. We even have a way to deal with bit offsets. See
> leds/register-bit-led.yaml for example.
>
Will do.
> >> +
>> + required:
>> + - gpio-controller
>> + - "#gpio-cells"
>> + - npins
>> + - bank-index
>> + - reg-mux-offset
>> + - reg-gpio-offset
>> + - bank-name
>> +
>> + additionalProperties: false
>> + unevaluatedProperties: false
>
> Don't need both. 'additionalProperties' is more restrictive, so drop
> unevaluatedProperties.
>
Will drop unevaluatedProperties.
>> +
>> + "^func-[0-9a-z]":
>
> "^func-[0-9a-z-]+$"
>
Will do.
>> + type: object
>> + additionalProperties:
>> + type: object
>> + allOf:
>> + - $ref: pincfg-node.yaml#
>> + - $ref: pinmux-node.yaml#
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + apb {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + periphs_pinctrl: pinctrl@...00 {
>> + compatible = "amlogic,pinctrl";
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + reg = <0x0 0x8e700 0x0 0x04>,
>> + <0x0 0x8e704 0x0 0x60>;
>> + reg-names = "mux", "gpio";
>> +
>> + gpiob {
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + npins = <10>;
>> + bank-index = <1>;
>> + reg-mux-offset = <0x14>;
>> + bit-mux-offset = <0x14>;
>> + reg-gpio-offset = <0x30>;
>> + bank-name = "GPIOB";
>> + };
>> +
>> + gpioe {
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + npins = <10>;
>> + bank-index = <5>;
>> + reg-mux-offset = <0x14>;
>> + reg-gpio-offset = <0x30>;
>> + bank-name = "GPIOE";
>> + };
>> +
>> + func-uart-b {
>> + uart-b-default{
>> + pinmux = <3>;
>> + bias-pull-up;
>> + drive-strength-microamp = <4000>;
>> + };
>> +
>> + uart-c-default{
>> + pinmux = <4>;
>> + bias-pull-up;
>> + drive-strength-microamp = <4000>;
>> + };
>> + };
>> +
>> + func-uart-c {
>> + uart-c-default{
>> + pinmux = <3>;
>> + bias-pull-up;
>> + drive-strength-microamp = <4000>;
>> + };
>> + };
>> + };
>> + };
>> diff --git a/include/dt-bindings/pinctrl/amlogic,pinctrl.h b/include/dt-bindings/pinctrl/amlogic,pinctrl.h
>> new file mode 100644
>> index 000000000000..03db0a730e8b
>> --- /dev/null
>> +++ b/include/dt-bindings/pinctrl/amlogic,pinctrl.h
>> @@ -0,0 +1,68 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>> +/*
>> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
>> + * Author: Xianwei Zhao <xianwei.zhao@...ogic.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_AMLOGIC_PINCTRL_H
>> +#define _DT_BINDINGS_AMLOGIC_PINCTRL_H
>> +
>> +/* define PIN modes */
>> +#define AF0 0x0
>> +#define AF1 0x1
>> +#define AF2 0x2
>> +#define AF3 0x3
>> +#define AF4 0x4
>> +#define AF5 0x5
>> +#define AF6 0x6
>> +#define AF7 0x7
>> +#define AF8 0x8
>> +#define AF9 0x9
>> +#define AF10 0xa
>> +#define AF11 0xb
>> +#define AF12 0xc
>> +#define AF13 0xd
>> +#define AF14 0xe
>> +#define AF15 0xf
>> +
>> +#define AML_PIN_ALT_FUNC_MASK 0xf
>> +
>> +/* Normal PIN bank */
>> +#define AMLOGIC_GPIO_A 0
>> +#define AMLOGIC_GPIO_B 1
>> +#define AMLOGIC_GPIO_C 2
>> +#define AMLOGIC_GPIO_D 3
>> +#define AMLOGIC_GPIO_E 4
>> +#define AMLOGIC_GPIO_F 5
>> +#define AMLOGIC_GPIO_G 6
>> +#define AMLOGIC_GPIO_H 7
>> +#define AMLOGIC_GPIO_I 8
>> +#define AMLOGIC_GPIO_J 9
>> +#define AMLOGIC_GPIO_K 10
>> +#define AMLOGIC_GPIO_L 11
>> +#define AMLOGIC_GPIO_M 12
>> +#define AMLOGIC_GPIO_N 13
>> +#define AMLOGIC_GPIO_O 14
>> +#define AMLOGIC_GPIO_P 15
>> +#define AMLOGIC_GPIO_Q 16
>> +#define AMLOGIC_GPIO_R 17
>> +#define AMLOGIC_GPIO_S 18
>> +#define AMLOGIC_GPIO_T 19
>> +#define AMLOGIC_GPIO_U 20
>> +#define AMLOGIC_GPIO_V 21
>> +#define AMLOGIC_GPIO_W 22
>> +#define AMLOGIC_GPIO_X 23
>> +#define AMLOGIC_GPIO_Y 24
>> +#define AMLOGIC_GPIO_Z 25
>> +
>> +/* Special PIN bank */
>> +#define AMLOGIC_GPIO_DV 26
>> +#define AMLOGIC_GPIO_AO 27
>> +#define AMLOGIC_GPIO_CC 28
>> +#define AMLOGIC_GPIO_TEST_N 29
>> +
>> +#define AML_PINMUX(bank, offset, mode) (((((bank) << 8) + (offset)) << 8) | (mode))
>> +#define AML_PINMUX_TO_BANK(pinmux) (((pinmux) >> 16) & 0xff)
>> +#define AML_PINMUX_TO_OFFSET(pinmux) (((pinmux) >> 8) & 0xff)
>> +
>> +#endif /* _DT_BINDINGS_AMLOGIC_PINCTRL_H */
>>
>> --
>> 2.37.1
>>
Powered by blists - more mailing lists