On 11/05/22 01:40, Krzysztof Kozlowski wrote:
> On 10/05/2022 11:44, Chris Packham wrote:
>> Convert the existing device tree binding to YAML format.
>>
>> The old binding listed the interrupt-controller and related properties
>> as required but there are sufficiently many existing usages without it
>> that the YAML binding does not make the interrupt properties required.
>>
>> The offset and marvell,pwm-offset properties weren't in the old binding
>> and are added to the YAML binding. The offset property is required when
>> the marvell,armada-8k-gpio compatible is used.
>>
>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>> ---
>> .../devicetree/bindings/gpio/gpio-mvebu.txt | 93 -----------
>> .../devicetree/bindings/gpio/gpio-mvebu.yaml | 147 ++++++++++++++++++
>> MAINTAINERS | 2 +-
>> 3 files changed, 148 insertions(+), 94 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
>> deleted file mode 100644
>> index 0fc6700ed800..000000000000
>> --- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
>> +++ /dev/null
>> @@ -1,93 +0,0 @@
>> -* Marvell EBU GPIO controller
>> -
>> -Required properties:
>> -
>> -- compatible : Should be "marvell,orion-gpio", "marvell,mv78200-gpio",
>> - "marvell,armadaxp-gpio" or "marvell,armada-8k-gpio".
>> -
>> - "marvell,orion-gpio" should be used for Orion, Kirkwood, Dove,
>> - Discovery (except MV78200) and Armada 370. "marvell,mv78200-gpio"
>> - should be used for the Discovery MV78200.
>> -
>> - "marvel,armadaxp-gpio" should be used for all Armada XP SoCs
>> - (MV78230, MV78260, MV78460).
>> -
>> - "marvell,armada-8k-gpio" should be used for the Armada 7K and 8K
>> - SoCs (either from AP or CP), see
>> - Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt
>> - for specific details about the offset property.
>> -
>> -- reg: Address and length of the register set for the device. Only one
>> - entry is expected, except for the "marvell,armadaxp-gpio" variant
>> - for which two entries are expected: one for the general registers,
>> - one for the per-cpu registers. Not used for marvell,armada-8k-gpio.
>> -
>> -- interrupts: The list of interrupts that are used for all the pins
>> - managed by this GPIO bank. There can be more than one interrupt
>> - (example: 1 interrupt per 8 pins on Armada XP, which means 4
>> - interrupts per bank of 32 GPIOs).
>> -
>> -- interrupt-controller: identifies the node as an interrupt controller
>> -
>> -- #interrupt-cells: specifies the number of cells needed to encode an
>> - interrupt source. Should be two.
>> - The first cell is the GPIO number.
>> - The second cell is used to specify flags:
>> - bits[3:0] trigger type and level flags:
>> - 1 = low-to-high edge triggered.
>> - 2 = high-to-low edge triggered.
>> - 4 = active high level-sensitive.
>> - 8 = active low level-sensitive.
>> -
>> -- gpio-controller: marks the device node as a gpio controller
>> -
>> -- ngpios: number of GPIOs this controller has
>> -
>> -- #gpio-cells: Should be two. The first cell is the pin number. The
>> - second cell is reserved for flags, unused at the moment.
>> -
>> -Optional properties:
>> -
>> -In order to use the GPIO lines in PWM mode, some additional optional
>> -properties are required.
>> -
>> -- compatible: Must contain "marvell,armada-370-gpio"
>> -
>> -- reg: an additional register set is needed, for the GPIO Blink
>> - Counter on/off registers.
>> -
>> -- reg-names: Must contain an entry "pwm" corresponding to the
>> - additional register range needed for PWM operation.
>> -
>> -- #pwm-cells: Should be two. The first cell is the GPIO line number. The
>> - second cell is the period in nanoseconds.
>> -
>> -- clocks: Must be a phandle to the clock for the GPIO controller.
>> -
>> -Example:
>> -
>> - gpio0: gpio@d0018100 {
>> - compatible = "marvell,armadaxp-gpio";
>> - reg = <0xd0018100 0x40>,
>> - <0xd0018800 0x30>;
>> - ngpios = <32>;
>> - gpio-controller;
>> - #gpio-cells = <2>;
>> - interrupt-controller;
>> - #interrupt-cells = <2>;
>> - interrupts = <16>, <17>, <18>, <19>;
>> - };
>> -
>> - gpio1: gpio@18140 {
>> - compatible = "marvell,armada-370-gpio";
>> - reg = <0x18140 0x40>, <0x181c8 0x08>;
>> - reg-names = "gpio", "pwm";
>> - ngpios = <17>;
>> - gpio-controller;
>> - #gpio-cells = <2>;
>> - #pwm-cells = <2>;
>> - interrupt-controller;
>> - #interrupt-cells = <2>;
>> - interrupts = <87>, <88>, <89>;
>> - clocks = <&coreclk 0>;
>> - };
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml b/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
>> new file mode 100644
>> index 000000000000..84b72e506526
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
>> @@ -0,0 +1,147 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://scanmail.trustwave.com/?c=20988&d=1ev64paEKF7o3yTVoF_fgiVBkeUi0_Dg3YQi4JPtpA&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fgpio%2fgpio-mvebu%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=1ev64paEKF7o3yTVoF_fgiVBkeUi0_Dg3YBxtMDuqw&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +title: Marvell EBU GPIO controller
>> +
>> +maintainers:
>> + - Thierry Reding <thierry.reding@xxxxxxxxx>
>> + - Lee Jones <lee.jones@xxxxxxxxxx>
>> +
>> +properties:
>> + compatible:
>> + oneOf:
>> + - enum:
>> + - marvell,orion-gpio
>> + - marvell,mv78200-gpio
>> + - marvell,armada-370-gpio
> It's expected to have orion fallback, so this does not look correct.
Except for marvell,armada-8k-gpio. I'll look in to how to capture that.
>
>> + - marvell,armadaxp-gpio
>> + - marvell,armada-8k-gpio
>> + - items:
>> + - const: marvell,armada-370-gpio
>> + - const: marvell,orion-gpio
>> +
>> + description: |
>> + "marvell,orion-gpio" should be used for Orion, Kirkwood, Dove, Discovery
>> + (except MV78200) and Armada 370. "marvell,mv78200-gpio" should be used
>> + for the Discovery MV78200.
>> +
>> + "marvel,armadaxp-gpio" should be used for all Armada XP SoCs (MV78230,
>> + MV78260, MV78460).
>> +
>> + "marvell,armada-8k-gpio" should be used for the Armada 7K and 8K SoCs
>> + (either from AP or CP), see
>> + Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt
>> + for specific details about the offset property.
> Why having the description? The usage should be obvious from the schema,
> so what is so special here?
I found the mapping of SoCs to compatible a bit non-obvious so I kept
it. The in-tree dts files don't actually follow what's documented
(armada-xp-*.dtsi don't use "marvel,armadaxp-gpio").
>> +
>> + reg:
>> + description: |
>> + Address and length of the register set for the device. Only one entry
>> + is expected, except for the "marvell,armadaxp-gpio" variant for which
>> + two entries are expected: one for the general registers, one for the
>> + per-cpu registers.
> This needs also entry in allOf with per-variant constraints.
>
>> Not used for marvell,armada-8k-gpio.
>> +
>> + An additional register set is needed, for the GPIO Blink
>> + Counter on/off registers.
>> + minItems: 1
>> + maxItems: 2
> PWM? the "reg" above was saying about per-cpu registers, so this is
> confusing. I understand old bindings wrote it like that, so maybe it
> should be fixed now.
I merged two sections from the old bindings. On the ArmadaXP the 2nd
entry is documented as for the per-CPU registers and appears to be
mandatory. For the others the 2nd entry is optional and is for the pwm
function. But as noted above the in-tree dts files don't actually use
the armadaxp compatible so this might be wrong in the old binding.
>
> Anyway, you need to describe the items and then apply constraints in allOf.
>
>> +
>> + reg-names:
>> + description:
>> + Must contain an entry "pwm" corresponding to the
>> + additional register range needed for PWM operation.
>> + minItems: 1
>> + maxItems: 2
>> +
>> + offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description: Offset in the register map for the gpio registers (in bytes)
>> +
>> + interrupts:
>> + description: |
>> + The list of interrupts that are used for all the pins managed by this
>> + GPIO bank. There can be more than one interrupt (example: 1 interrupt
>> + per 8 pins on Armada XP, which means 4 interrupts per bank of 32
>> + GPIOs).
>> + minItems: 1
>> + maxItems: 4
>> +
>> + interrupt-controller: true
>> +
>> + "#interrupt-cells":
>> + const: 2
>> +
>> + gpio-controller: true
>> +
>> + ngpios:
>> + description:
>> + number of GPIOs this controller has
> Skip description, it's obvious from generic bindings.
>
>> + minimum: 1
>> + maximum: 32
>> +
>> + "#gpio-cells":
>> + const: 2
>> +
>> + marvell,pwm-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description: Offset in the register map for the pwm registers (in bytes)
> It's the same as offset. Why allowing both? Isn't one deprecated?
As mentioned separately. This is for the pwm function which is different
to the other offset.
>> +
>> + "#pwm-cells":
>> + description:
>> + The first cell is the GPIO line number. The second cell is the period
>> + in nanoseconds.
>> + const: 2
>> +
>> + clocks:
>> + minItems: 1
>> + maxItems: 2
> This should be strictly defined, either here or per variant.
Will update
>> +
>> +required:
>> + - compatible
>> + - gpio-controller
>> + - ngpios
>> + - "#gpio-cells"
>> +
>> +if:
> Within allOf please.
>
> Best regards,
> Krzysztof
