| Message ID | 20210202103623.200809-8-damien.lemoal@wdc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On 2/2/21 1:45 PM, Atish Patra wrote: > On Tue, Feb 2, 2021 at 2:37 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >> >> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > > The spec here says 16 GPIOs > https://static.dev.sifive.com/FU540-C000-v1.0.pdf > > Is there a updated spec available ? This GPIO device is (AFAICT) from [1]. While the version instantiated on SiFive's cores has 16 GPIOs, the actual number of GPIOs is configurable. In Canaan's version, there are 32 GPIOs. Unfortunately, I am not aware of any non-implementation-specific documentation for this device. --Sean [1] https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/gpio > >> interrupts property description and maxItems. Also add the standard >> ngpios property to describe the number of GPIOs available on the >> implementation. >> >> Also add the "canaan,k210-gpiohs" compatible string to indicate the use >> of this gpio controller in the Canaan Kendryte K210 SoC. If this >> compatible string is used, do not define the clocks property as >> required as the K210 SoC does not have a software controllable clock >> for the Sifive gpio IP block. >> >> Cc: Paul Walmsley <paul.walmsley@sifive.com> >> Cc: Rob Herring <robh@kernel.org> >> Cc: devicetree@vger.kernel.org >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >> index ab22056f8b44..2cef18ca737c 100644 >> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >> @@ -16,6 +16,7 @@ properties: >> - enum: >> - sifive,fu540-c000-gpio >> - sifive,fu740-c000-gpio >> + - canaan,k210-gpiohs >> - const: sifive,gpio0 >> >> reg: >> @@ -23,9 +24,9 @@ properties: >> >> interrupts: >> description: >> - interrupt mapping one per GPIO. Maximum 16 GPIOs. >> + interrupt mapping one per GPIO. Maximum 32 GPIOs. >> minItems: 1 >> - maxItems: 16 >> + maxItems: 32 >> >> interrupt-controller: true >> >> @@ -38,6 +39,10 @@ properties: >> "#gpio-cells": >> const: 2 >> >> + ngpios: >> + minimum: 1 >> + maximum: 32 >> + >> gpio-controller: true >> >> required: >> @@ -46,10 +51,20 @@ required: >> - interrupts >> - interrupt-controller >> - "#interrupt-cells" >> - - clocks >> - "#gpio-cells" >> - gpio-controller >> >> +if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - sifive,fu540-c000-gpio >> + - sifive,fu740-c000-gpio >> +then: >> + required: >> + - clocks >> + >> additionalProperties: false >> >> examples: >> -- >> 2.29.2 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > >
On 2/2/21 2:02 PM, Rob Herring wrote: > On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >> >> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the >> interrupts property description and maxItems. Also add the standard >> ngpios property to describe the number of GPIOs available on the >> implementation. >> >> Also add the "canaan,k210-gpiohs" compatible string to indicate the use >> of this gpio controller in the Canaan Kendryte K210 SoC. If this >> compatible string is used, do not define the clocks property as >> required as the K210 SoC does not have a software controllable clock >> for the Sifive gpio IP block. >> >> Cc: Paul Walmsley <paul.walmsley@sifive.com> >> Cc: Rob Herring <robh@kernel.org> >> Cc: devicetree@vger.kernel.org >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >> index ab22056f8b44..2cef18ca737c 100644 >> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >> @@ -16,6 +16,7 @@ properties: >> - enum: >> - sifive,fu540-c000-gpio >> - sifive,fu740-c000-gpio >> + - canaan,k210-gpiohs >> - const: sifive,gpio0 >> >> reg: >> @@ -23,9 +24,9 @@ properties: >> >> interrupts: >> description: >> - interrupt mapping one per GPIO. Maximum 16 GPIOs. >> + interrupt mapping one per GPIO. Maximum 32 GPIOs. >> minItems: 1 >> - maxItems: 16 >> + maxItems: 32 >> >> interrupt-controller: true >> >> @@ -38,6 +39,10 @@ properties: >> "#gpio-cells": >> const: 2 >> >> + ngpios: >> + minimum: 1 >> + maximum: 32 > > What's the default as obviously drivers already assume something. The driver currently assumes 16. However, as noted in reply to Atish, the number of GPIOs is configurable. > Does a driver actually need to know this? For example, does the > register stride change or something? No. I believe that the number of GPIOs sets which bits in the control registers are valid. So the maximum number of GPIOs is the word width of the bus. > Please don't add it if the only purpose is error check your DT (IOW, > if it just checks the max cell value in gpios phandles). Why not? This seems like exactly the situation this property was designed for. > Optionally, a GPIO controller may have a "ngpios" property. This property > indicates the number of in-use slots of available slots for GPIOs. The > typical example is something like this: the hardware register is 32 bits > wide, but only 18 of the bits have a physical counterpart. The driver is > generally written so that all 32 bits can be used, but the IP block is reused > in a lot of designs, some using all 32 bits, some using 18 and some using > 12. In this case, setting "ngpios = <18>;" informs the driver that only the > first 18 GPIOs, at local offset 0 .. 17, are in use. --Sean
On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: > On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > > The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > > interrupts property description and maxItems. Also add the standard > > ngpios property to describe the number of GPIOs available on the > > implementation. > > > > Also add the "canaan,k210-gpiohs" compatible string to indicate the use > > of this gpio controller in the Canaan Kendryte K210 SoC. If this > > compatible string is used, do not define the clocks property as > > required as the K210 SoC does not have a software controllable clock > > for the Sifive gpio IP block. > > > > Cc: Paul Walmsley <paul.walmsley@sifive.com> > > Cc: Rob Herring <robh@kernel.org> > > Cc: devicetree@vger.kernel.org > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > > --- > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > index ab22056f8b44..2cef18ca737c 100644 > > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > @@ -16,6 +16,7 @@ properties: > > - enum: > > - sifive,fu540-c000-gpio > > - sifive,fu740-c000-gpio > > + - canaan,k210-gpiohs > > - const: sifive,gpio0 > > > > reg: > > @@ -23,9 +24,9 @@ properties: > > > > interrupts: > > description: > > - interrupt mapping one per GPIO. Maximum 16 GPIOs. > > + interrupt mapping one per GPIO. Maximum 32 GPIOs. > > minItems: 1 > > - maxItems: 16 > > + maxItems: 32 > > > > interrupt-controller: true > > > > @@ -38,6 +39,10 @@ properties: > > "#gpio-cells": > > const: 2 > > > > + ngpios: > > + minimum: 1 > > + maximum: 32 > > What's the default as obviously drivers already assume something. > > Does a driver actually need to know this? For example, does the > register stride change or something? > > Please don't add it if the only purpose is error check your DT (IOW, > if it just checks the max cell value in gpios phandles). If I remove that, make dtbs_check complains. Looking at othe gpio controller bindings, they all have it. So isn't it better to be consistent, and avoid make dtbs_check errors ? > > Rob -- Damien Le Moal Western Digital Research
On Tue, Feb 2, 2021 at 6:01 PM Sean Anderson <seanga2@gmail.com> wrote: > > On 2/2/21 2:02 PM, Rob Herring wrote: > > On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > >> > >> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > >> interrupts property description and maxItems. Also add the standard > >> ngpios property to describe the number of GPIOs available on the > >> implementation. > >> > >> Also add the "canaan,k210-gpiohs" compatible string to indicate the use > >> of this gpio controller in the Canaan Kendryte K210 SoC. If this > >> compatible string is used, do not define the clocks property as > >> required as the K210 SoC does not have a software controllable clock > >> for the Sifive gpio IP block. > >> > >> Cc: Paul Walmsley <paul.walmsley@sifive.com> > >> Cc: Rob Herring <robh@kernel.org> > >> Cc: devicetree@vger.kernel.org > >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > >> --- > >> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- > >> 1 file changed, 18 insertions(+), 3 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >> index ab22056f8b44..2cef18ca737c 100644 > >> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >> @@ -16,6 +16,7 @@ properties: > >> - enum: > >> - sifive,fu540-c000-gpio > >> - sifive,fu740-c000-gpio > >> + - canaan,k210-gpiohs > >> - const: sifive,gpio0 > >> > >> reg: > >> @@ -23,9 +24,9 @@ properties: > >> > >> interrupts: > >> description: > >> - interrupt mapping one per GPIO. Maximum 16 GPIOs. > >> + interrupt mapping one per GPIO. Maximum 32 GPIOs. > >> minItems: 1 > >> - maxItems: 16 > >> + maxItems: 32 > >> > >> interrupt-controller: true > >> > >> @@ -38,6 +39,10 @@ properties: > >> "#gpio-cells": > >> const: 2 > >> > >> + ngpios: > >> + minimum: 1 > >> + maximum: 32 > > > > What's the default as obviously drivers already assume something. > > The driver currently assumes 16. However, as noted in reply to Atish, > the number of GPIOs is configurable. So you need a 'default: 16' here. > > Does a driver actually need to know this? For example, does the > > register stride change or something? > > No. I believe that the number of GPIOs sets which bits in the control > registers are valid. So the maximum number of GPIOs is the word width of > the bus. So like register access size (e.g. readw vs readl)? If so, we have 'reg-io-width' for that purpose. > > Please don't add it if the only purpose is error check your DT (IOW, > > if it just checks the max cell value in gpios phandles). > > Why not? This seems like exactly the situation this property was > designed for. Because it is redundant. All the GPIO lines you want to use should be connected to something with a *-gpios property. If not, then that's a failure to describe part of the h/w. For comparison, we generally don't define how many interrupts an interrupt controller has. Or how many power-domains a power domain provider has. I can go on with every provider/consumer binding... Rob
On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: > > On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > > > > The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > > > interrupts property description and maxItems. Also add the standard > > > ngpios property to describe the number of GPIOs available on the > > > implementation. > > > > > > Also add the "canaan,k210-gpiohs" compatible string to indicate the use > > > of this gpio controller in the Canaan Kendryte K210 SoC. If this > > > compatible string is used, do not define the clocks property as > > > required as the K210 SoC does not have a software controllable clock > > > for the Sifive gpio IP block. > > > > > > Cc: Paul Walmsley <paul.walmsley@sifive.com> > > > Cc: Rob Herring <robh@kernel.org> > > > Cc: devicetree@vger.kernel.org > > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > > > --- > > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- > > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > index ab22056f8b44..2cef18ca737c 100644 > > > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > @@ -16,6 +16,7 @@ properties: > > > - enum: > > > - sifive,fu540-c000-gpio > > > - sifive,fu740-c000-gpio > > > + - canaan,k210-gpiohs > > > - const: sifive,gpio0 > > > > > > reg: > > > @@ -23,9 +24,9 @@ properties: > > > > > > interrupts: > > > description: > > > - interrupt mapping one per GPIO. Maximum 16 GPIOs. > > > + interrupt mapping one per GPIO. Maximum 32 GPIOs. > > > minItems: 1 > > > - maxItems: 16 > > > + maxItems: 32 > > > > > > interrupt-controller: true > > > > > > @@ -38,6 +39,10 @@ properties: > > > "#gpio-cells": > > > const: 2 > > > > > > + ngpios: > > > + minimum: 1 > > > + maximum: 32 > > > > What's the default as obviously drivers already assume something. > > > > Does a driver actually need to know this? For example, does the > > register stride change or something? > > > > Please don't add it if the only purpose is error check your DT (IOW, > > if it just checks the max cell value in gpios phandles). > > If I remove that, make dtbs_check complains. Looking at othe gpio controller > bindings, they all have it. So isn't it better to be consistent, and avoid make > dtbs_check errors ? That would mean you are already using 'ngpios' and it is undocumented (for this binding). If already in use and possibly having users then that changes things, but that's not what the commit msg says. Not *all* gpio controllers have ngpios. It's a good number, but probably more than need it though. If we wanted it everywhere, there would be a schema enforcing that. Rob
On 2/3/21 3:23 PM, Rob Herring wrote: > On Tue, Feb 2, 2021 at 6:01 PM Sean Anderson <seanga2@gmail.com> wrote: >> >> On 2/2/21 2:02 PM, Rob Herring wrote: >>> On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >>>> >>>> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the >>>> interrupts property description and maxItems. Also add the standard >>>> ngpios property to describe the number of GPIOs available on the >>>> implementation. >>>> >>>> Also add the "canaan,k210-gpiohs" compatible string to indicate the use >>>> of this gpio controller in the Canaan Kendryte K210 SoC. If this >>>> compatible string is used, do not define the clocks property as >>>> required as the K210 SoC does not have a software controllable clock >>>> for the Sifive gpio IP block. >>>> >>>> Cc: Paul Walmsley <paul.walmsley@sifive.com> >>>> Cc: Rob Herring <robh@kernel.org> >>>> Cc: devicetree@vger.kernel.org >>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>>> --- >>>> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- >>>> 1 file changed, 18 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>> index ab22056f8b44..2cef18ca737c 100644 >>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>> @@ -16,6 +16,7 @@ properties: >>>> - enum: >>>> - sifive,fu540-c000-gpio >>>> - sifive,fu740-c000-gpio >>>> + - canaan,k210-gpiohs >>>> - const: sifive,gpio0 >>>> >>>> reg: >>>> @@ -23,9 +24,9 @@ properties: >>>> >>>> interrupts: >>>> description: >>>> - interrupt mapping one per GPIO. Maximum 16 GPIOs. >>>> + interrupt mapping one per GPIO. Maximum 32 GPIOs. >>>> minItems: 1 >>>> - maxItems: 16 >>>> + maxItems: 32 >>>> >>>> interrupt-controller: true >>>> >>>> @@ -38,6 +39,10 @@ properties: >>>> "#gpio-cells": >>>> const: 2 >>>> >>>> + ngpios: >>>> + minimum: 1 >>>> + maximum: 32 >>> >>> What's the default as obviously drivers already assume something. >> >> The driver currently assumes 16. However, as noted in reply to Atish, >> the number of GPIOs is configurable. > > So you need a 'default: 16' here. There is no reasonable default. This device can be configured to have any number of GPIOs between 1 and 32. >>> Does a driver actually need to know this? For example, does the >>> register stride change or something? >> >> No. I believe that the number of GPIOs sets which bits in the control >> registers are valid. So the maximum number of GPIOs is the word width of >> the bus. > > So like register access size (e.g. readw vs readl)? If so, we have > 'reg-io-width' for that purpose. This is just the "maximum configurable" due to how the device's registers are laid out. If you wanted to have more GPIOs than the register access size, you would need to make more extensive (and likely incompatible) modifications to the device. However, I don't believe there are any devices with 64-bit register width (yet) and the driver does not support 64-bit accesses. > >>> Please don't add it if the only purpose is error check your DT (IOW, >>> if it just checks the max cell value in gpios phandles). >> >> Why not? This seems like exactly the situation this property was >> designed for. > > Because it is redundant. All the GPIO lines you want to use should be > connected to something with a *-gpios property. If not, then that's a > failure to describe part of the h/w. This is wrong. On SoCs with pinmuxing (such as this one), not all GPIO lines may be connected for any particular board. > For comparison, we generally don't define how many interrupts an > interrupt controller has. Or how many power-domains a power domain > provider has. I can go on with every provider/consumer binding... And yet the in-kernel documentation *specifically* recommends using this property in this situation. I suggest you submit a patch to remove that paragraph if you think that it is not necessary. --Sean
On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: > On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > > > On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: > > > On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > > > > > > The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > > > > interrupts property description and maxItems. Also add the standard > > > > ngpios property to describe the number of GPIOs available on the > > > > implementation. > > > > > > > > Also add the "canaan,k210-gpiohs" compatible string to indicate the use > > > > of this gpio controller in the Canaan Kendryte K210 SoC. If this > > > > compatible string is used, do not define the clocks property as > > > > required as the K210 SoC does not have a software controllable clock > > > > for the Sifive gpio IP block. > > > > > > > > Cc: Paul Walmsley <paul.walmsley@sifive.com> > > > > Cc: Rob Herring <robh@kernel.org> > > > > Cc: devicetree@vger.kernel.org > > > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > > > > --- > > > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- > > > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > index ab22056f8b44..2cef18ca737c 100644 > > > > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > @@ -16,6 +16,7 @@ properties: > > > > - enum: > > > > - sifive,fu540-c000-gpio > > > > - sifive,fu740-c000-gpio > > > > + - canaan,k210-gpiohs > > > > - const: sifive,gpio0 > > > > > > > > reg: > > > > @@ -23,9 +24,9 @@ properties: > > > > > > > > interrupts: > > > > description: > > > > - interrupt mapping one per GPIO. Maximum 16 GPIOs. > > > > + interrupt mapping one per GPIO. Maximum 32 GPIOs. > > > > minItems: 1 > > > > - maxItems: 16 > > > > + maxItems: 32 > > > > > > > > interrupt-controller: true > > > > > > > > @@ -38,6 +39,10 @@ properties: > > > > "#gpio-cells": > > > > const: 2 > > > > > > > > + ngpios: > > > > + minimum: 1 > > > > + maximum: 32 > > > > > > What's the default as obviously drivers already assume something. > > > > > > Does a driver actually need to know this? For example, does the > > > register stride change or something? > > > > > > Please don't add it if the only purpose is error check your DT (IOW, > > > if it just checks the max cell value in gpios phandles). > > > > If I remove that, make dtbs_check complains. Looking at othe gpio controller > > bindings, they all have it. So isn't it better to be consistent, and avoid make > > dtbs_check errors ? > > That would mean you are already using 'ngpios' and it is undocumented > (for this binding). If already in use and possibly having users then > that changes things, but that's not what the commit msg says. > > Not *all* gpio controllers have ngpios. It's a good number, but > probably more than need it though. If we wanted it everywhere, there > would be a schema enforcing that. If I remove the minimum and maximum lines, I get this error: ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty value in block mapping (empty-values) CHKDT Documentation/devicetree/bindings/processed-schema-examples.json /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive ,gpio.yaml: ignoring, error in schema: properties: ngpios warning: no schema found in file: ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml If I remove the ngpios property entirely, then I get a hit on the device tree: CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- [0-9]+' From schema: /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive ,gpio.yaml Now, If I change the property definition to this: diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml index 2cef18ca737c..5c7865180383 100644 --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml @@ -40,8 +40,11 @@ properties: const: 2 ngpios: - minimum: 1 - maximum: 32 + $ref: /schemas/types.yaml#/definitions/uint32 + description: + The number of GPIO pins implemented by the controller. + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. + gpio-controller: true Then all is OK. Which option should I go for here ? If we want to avoid a dtbs_check error, as far as I can see, we can: 1) Remove the ngpios property and remove its use from the DTS, which is not nice in my opinion 2) Use the modification proposed above Please advise. Thanks ! -- Damien Le Moal Western Digital Research
On 2021/02/04 9:47, Damien Le Moal wrote: > On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: >> On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >>> >>> On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: >>>> On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >>>>> >>>>> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the >>>>> interrupts property description and maxItems. Also add the standard >>>>> ngpios property to describe the number of GPIOs available on the >>>>> implementation. >>>>> >>>>> Also add the "canaan,k210-gpiohs" compatible string to indicate the use >>>>> of this gpio controller in the Canaan Kendryte K210 SoC. If this >>>>> compatible string is used, do not define the clocks property as >>>>> required as the K210 SoC does not have a software controllable clock >>>>> for the Sifive gpio IP block. >>>>> >>>>> Cc: Paul Walmsley <paul.walmsley@sifive.com> >>>>> Cc: Rob Herring <robh@kernel.org> >>>>> Cc: devicetree@vger.kernel.org >>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>>>> --- >>>>> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- >>>>> 1 file changed, 18 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>> index ab22056f8b44..2cef18ca737c 100644 >>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>> @@ -16,6 +16,7 @@ properties: >>>>> - enum: >>>>> - sifive,fu540-c000-gpio >>>>> - sifive,fu740-c000-gpio >>>>> + - canaan,k210-gpiohs >>>>> - const: sifive,gpio0 >>>>> >>>>> reg: >>>>> @@ -23,9 +24,9 @@ properties: >>>>> >>>>> interrupts: >>>>> description: >>>>> - interrupt mapping one per GPIO. Maximum 16 GPIOs. >>>>> + interrupt mapping one per GPIO. Maximum 32 GPIOs. >>>>> minItems: 1 >>>>> - maxItems: 16 >>>>> + maxItems: 32 >>>>> >>>>> interrupt-controller: true >>>>> >>>>> @@ -38,6 +39,10 @@ properties: >>>>> "#gpio-cells": >>>>> const: 2 >>>>> >>>>> + ngpios: >>>>> + minimum: 1 >>>>> + maximum: 32 >>>> >>>> What's the default as obviously drivers already assume something. >>>> >>>> Does a driver actually need to know this? For example, does the >>>> register stride change or something? >>>> >>>> Please don't add it if the only purpose is error check your DT (IOW, >>>> if it just checks the max cell value in gpios phandles). >>> >>> If I remove that, make dtbs_check complains. Looking at othe gpio controller >>> bindings, they all have it. So isn't it better to be consistent, and avoid make >>> dtbs_check errors ? >> >> That would mean you are already using 'ngpios' and it is undocumented >> (for this binding). If already in use and possibly having users then >> that changes things, but that's not what the commit msg says. >> >> Not *all* gpio controllers have ngpios. It's a good number, but >> probably more than need it though. If we wanted it everywhere, there >> would be a schema enforcing that. > > If I remove the minimum and maximum lines, I get this error: > > ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty > value in block mapping (empty-values) > CHKDT Documentation/devicetree/bindings/processed-schema-examples.json > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' > SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > ,gpio.yaml: ignoring, error in schema: properties: ngpios > warning: no schema found in file: > ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > If I remove the ngpios property entirely, then I get a hit on the device tree: > > CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml > /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: > gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- > [0-9]+' > From schema: > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > ,gpio.yaml > > Now, If I change the property definition to this: > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > index 2cef18ca737c..5c7865180383 100644 > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > @@ -40,8 +40,11 @@ properties: > const: 2 > > ngpios: > - minimum: 1 > - maximum: 32 > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + The number of GPIO pins implemented by the controller. > + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. > + > > gpio-controller: true > > Then all is OK. > > Which option should I go for here ? If we want to avoid a dtbs_check error, as > far as I can see, we can: > 1) Remove the ngpios property and remove its use from the DTS, which is not > nice in my opinion > 2) Use the modification proposed above > > Please advise. Thanks ! > Rob, Thanks for the reviews and acks on the other patches. I would like to send v16 with your suggested fixes. But what should I do about ngpios ? Option (1) or (2) above ? Any other option ? Thanks !
On Wed, Feb 3, 2021 at 6:47 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: > > On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > > > > > On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: > > > > On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > > > > > > > > The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > > > > > interrupts property description and maxItems. Also add the standard > > > > > ngpios property to describe the number of GPIOs available on the > > > > > implementation. > > > > > > > > > > Also add the "canaan,k210-gpiohs" compatible string to indicate the use > > > > > of this gpio controller in the Canaan Kendryte K210 SoC. If this > > > > > compatible string is used, do not define the clocks property as > > > > > required as the K210 SoC does not have a software controllable clock > > > > > for the Sifive gpio IP block. > > > > > > > > > > Cc: Paul Walmsley <paul.walmsley@sifive.com> > > > > > Cc: Rob Herring <robh@kernel.org> > > > > > Cc: devicetree@vger.kernel.org > > > > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > > > > > --- > > > > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- > > > > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > index ab22056f8b44..2cef18ca737c 100644 > > > > > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > @@ -16,6 +16,7 @@ properties: > > > > > - enum: > > > > > - sifive,fu540-c000-gpio > > > > > - sifive,fu740-c000-gpio > > > > > + - canaan,k210-gpiohs > > > > > - const: sifive,gpio0 > > > > > > > > > > reg: > > > > > @@ -23,9 +24,9 @@ properties: > > > > > > > > > > interrupts: > > > > > description: > > > > > - interrupt mapping one per GPIO. Maximum 16 GPIOs. > > > > > + interrupt mapping one per GPIO. Maximum 32 GPIOs. > > > > > minItems: 1 > > > > > - maxItems: 16 > > > > > + maxItems: 32 > > > > > > > > > > interrupt-controller: true > > > > > > > > > > @@ -38,6 +39,10 @@ properties: > > > > > "#gpio-cells": > > > > > const: 2 > > > > > > > > > > + ngpios: > > > > > + minimum: 1 > > > > > + maximum: 32 > > > > > > > > What's the default as obviously drivers already assume something. > > > > > > > > Does a driver actually need to know this? For example, does the > > > > register stride change or something? > > > > > > > > Please don't add it if the only purpose is error check your DT (IOW, > > > > if it just checks the max cell value in gpios phandles). > > > > > > If I remove that, make dtbs_check complains. Looking at othe gpio controller > > > bindings, they all have it. So isn't it better to be consistent, and avoid make > > > dtbs_check errors ? > > > > That would mean you are already using 'ngpios' and it is undocumented > > (for this binding). If already in use and possibly having users then > > that changes things, but that's not what the commit msg says. > > > > Not *all* gpio controllers have ngpios. It's a good number, but > > probably more than need it though. If we wanted it everywhere, there > > would be a schema enforcing that. > > If I remove the minimum and maximum lines, I get this error: I never said remove minimum/maximum. The suggestion is either add 'default: 16' or remove 'ngpios' entirely. > ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty > value in block mapping (empty-values) > CHKDT Documentation/devicetree/bindings/processed-schema-examples.json > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' > SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > ,gpio.yaml: ignoring, error in schema: properties: ngpios > warning: no schema found in file: > ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml ngpios: true or ngpios: {} Are the minimum valid values for a key. (Though not what should be done here.) > > If I remove the ngpios property entirely, then I get a hit on the device tree: > > CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml > /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: > gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- > [0-9]+' > From schema: > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > ,gpio.yaml That's not upstream, right? Then fix it. > Now, If I change the property definition to this: > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > index 2cef18ca737c..5c7865180383 100644 > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > @@ -40,8 +40,11 @@ properties: > const: 2 > > ngpios: > - minimum: 1 > - maximum: 32 > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + The number of GPIO pins implemented by the controller. > + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. > + > > gpio-controller: true > > Then all is OK. > > Which option should I go for here ? If we want to avoid a dtbs_check error, as > far as I can see, we can: > 1) Remove the ngpios property and remove its use from the DTS, which is not > nice in my opinion Again, it depends if there are users depending on it. A user being a GPIO driver somewhere, not a DTS file. The GPIO driver in the kernel doesn't need it. So u-boot? BSD? > 2) Use the modification proposed above
On Fri, 2021-02-05 at 14:02 -0600, Rob Herring wrote: > On Wed, Feb 3, 2021 at 6:47 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > > > On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: > > > On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > > > > > > > On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: > > > > > On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > > > > > > > > > > The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > > > > > > interrupts property description and maxItems. Also add the standard > > > > > > ngpios property to describe the number of GPIOs available on the > > > > > > implementation. > > > > > > > > > > > > Also add the "canaan,k210-gpiohs" compatible string to indicate the use > > > > > > of this gpio controller in the Canaan Kendryte K210 SoC. If this > > > > > > compatible string is used, do not define the clocks property as > > > > > > required as the K210 SoC does not have a software controllable clock > > > > > > for the Sifive gpio IP block. > > > > > > > > > > > > Cc: Paul Walmsley <paul.walmsley@sifive.com> > > > > > > Cc: Rob Herring <robh@kernel.org> > > > > > > Cc: devicetree@vger.kernel.org > > > > > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > > > > > > --- > > > > > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- > > > > > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > > index ab22056f8b44..2cef18ca737c 100644 > > > > > > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > > @@ -16,6 +16,7 @@ properties: > > > > > > - enum: > > > > > > - sifive,fu540-c000-gpio > > > > > > - sifive,fu740-c000-gpio > > > > > > + - canaan,k210-gpiohs > > > > > > - const: sifive,gpio0 > > > > > > > > > > > > reg: > > > > > > @@ -23,9 +24,9 @@ properties: > > > > > > > > > > > > interrupts: > > > > > > description: > > > > > > - interrupt mapping one per GPIO. Maximum 16 GPIOs. > > > > > > + interrupt mapping one per GPIO. Maximum 32 GPIOs. > > > > > > minItems: 1 > > > > > > - maxItems: 16 > > > > > > + maxItems: 32 > > > > > > > > > > > > interrupt-controller: true > > > > > > > > > > > > @@ -38,6 +39,10 @@ properties: > > > > > > "#gpio-cells": > > > > > > const: 2 > > > > > > > > > > > > + ngpios: > > > > > > + minimum: 1 > > > > > > + maximum: 32 > > > > > > > > > > What's the default as obviously drivers already assume something. > > > > > > > > > > Does a driver actually need to know this? For example, does the > > > > > register stride change or something? > > > > > > > > > > Please don't add it if the only purpose is error check your DT (IOW, > > > > > if it just checks the max cell value in gpios phandles). > > > > > > > > If I remove that, make dtbs_check complains. Looking at othe gpio controller > > > > bindings, they all have it. So isn't it better to be consistent, and avoid make > > > > dtbs_check errors ? > > > > > > That would mean you are already using 'ngpios' and it is undocumented > > > (for this binding). If already in use and possibly having users then > > > that changes things, but that's not what the commit msg says. > > > > > > Not *all* gpio controllers have ngpios. It's a good number, but > > > probably more than need it though. If we wanted it everywhere, there > > > would be a schema enforcing that. > > > > If I remove the minimum and maximum lines, I get this error: > > I never said remove minimum/maximum. The suggestion is either add > 'default: 16' or remove 'ngpios' entirely. > > > ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty > > value in block mapping (empty-values) > > CHKDT Documentation/devicetree/bindings/processed-schema-examples.json > > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > > ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' > > SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json > > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > > ,gpio.yaml: ignoring, error in schema: properties: ngpios > > warning: no schema found in file: > > ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > ngpios: true > > or > > ngpios: {} > > Are the minimum valid values for a key. (Though not what should be done here.) > > > > > If I remove the ngpios property entirely, then I get a hit on the device tree: > > > > CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml > > /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: > > gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- > > [0-9]+' > > From schema: > > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > > ,gpio.yaml > > That's not upstream, right? Then fix it. > > > Now, If I change the property definition to this: > > > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > index 2cef18ca737c..5c7865180383 100644 > > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > @@ -40,8 +40,11 @@ properties: > > const: 2 > > > > ngpios: > > - minimum: 1 > > - maximum: 32 > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + The number of GPIO pins implemented by the controller. > > + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. > > + > > > > gpio-controller: true > > > > Then all is OK. > > > > Which option should I go for here ? If we want to avoid a dtbs_check error, as > > far as I can see, we can: > > 1) Remove the ngpios property and remove its use from the DTS, which is not > > nice in my opinion > > Again, it depends if there are users depending on it. A user being a > GPIO driver somewhere, not a DTS file. The GPIO driver in the kernel > doesn't need it. So u-boot? BSD? The Linux driver uses the number of interrupts for the number of gpios but upstream U-Boot uses the ngpios property. So I will change this to use "default: 16" as you suggested. Thanks ! > > > 2) Use the modification proposed above -- Damien Le Moal Western Digital Research
On 2/5/21 5:53 PM, Damien Le Moal wrote: > On Fri, 2021-02-05 at 14:02 -0600, Rob Herring wrote: >> On Wed, Feb 3, 2021 at 6:47 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >>> >>> On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: >>>> On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >>>>> >>>>> On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: >>>>>> On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >>>>>>> >>>>>>> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the >>>>>>> interrupts property description and maxItems. Also add the standard >>>>>>> ngpios property to describe the number of GPIOs available on the >>>>>>> implementation. >>>>>>> >>>>>>> Also add the "canaan,k210-gpiohs" compatible string to indicate the use >>>>>>> of this gpio controller in the Canaan Kendryte K210 SoC. If this >>>>>>> compatible string is used, do not define the clocks property as >>>>>>> required as the K210 SoC does not have a software controllable clock >>>>>>> for the Sifive gpio IP block. >>>>>>> >>>>>>> Cc: Paul Walmsley <paul.walmsley@sifive.com> >>>>>>> Cc: Rob Herring <robh@kernel.org> >>>>>>> Cc: devicetree@vger.kernel.org >>>>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>>>>>> --- >>>>>>> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- >>>>>>> 1 file changed, 18 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>> index ab22056f8b44..2cef18ca737c 100644 >>>>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>> @@ -16,6 +16,7 @@ properties: >>>>>>> - enum: >>>>>>> - sifive,fu540-c000-gpio >>>>>>> - sifive,fu740-c000-gpio >>>>>>> + - canaan,k210-gpiohs >>>>>>> - const: sifive,gpio0 >>>>>>> >>>>>>> reg: >>>>>>> @@ -23,9 +24,9 @@ properties: >>>>>>> >>>>>>> interrupts: >>>>>>> description: >>>>>>> - interrupt mapping one per GPIO. Maximum 16 GPIOs. >>>>>>> + interrupt mapping one per GPIO. Maximum 32 GPIOs. >>>>>>> minItems: 1 >>>>>>> - maxItems: 16 >>>>>>> + maxItems: 32 >>>>>>> >>>>>>> interrupt-controller: true >>>>>>> >>>>>>> @@ -38,6 +39,10 @@ properties: >>>>>>> "#gpio-cells": >>>>>>> const: 2 >>>>>>> >>>>>>> + ngpios: >>>>>>> + minimum: 1 >>>>>>> + maximum: 32 >>>>>> >>>>>> What's the default as obviously drivers already assume something. >>>>>> >>>>>> Does a driver actually need to know this? For example, does the >>>>>> register stride change or something? >>>>>> >>>>>> Please don't add it if the only purpose is error check your DT (IOW, >>>>>> if it just checks the max cell value in gpios phandles). >>>>> >>>>> If I remove that, make dtbs_check complains. Looking at othe gpio controller >>>>> bindings, they all have it. So isn't it better to be consistent, and avoid make >>>>> dtbs_check errors ? >>>> >>>> That would mean you are already using 'ngpios' and it is undocumented >>>> (for this binding). If already in use and possibly having users then >>>> that changes things, but that's not what the commit msg says. >>>> >>>> Not *all* gpio controllers have ngpios. It's a good number, but >>>> probably more than need it though. If we wanted it everywhere, there >>>> would be a schema enforcing that. >>> >>> If I remove the minimum and maximum lines, I get this error: >> >> I never said remove minimum/maximum. The suggestion is either add >> 'default: 16' or remove 'ngpios' entirely. >> >>> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty >>> value in block mapping (empty-values) >>> CHKDT Documentation/devicetree/bindings/processed-schema-examples.json >>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>> ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' >>> SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json >>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>> ,gpio.yaml: ignoring, error in schema: properties: ngpios >>> warning: no schema found in file: >>> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >> >> ngpios: true >> >> or >> >> ngpios: {} >> >> Are the minimum valid values for a key. (Though not what should be done here.) >> >>> >>> If I remove the ngpios property entirely, then I get a hit on the device tree: >>> >>> CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml >>> /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: >>> gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- >>> [0-9]+' >>> From schema: >>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>> ,gpio.yaml >> >> That's not upstream, right? Then fix it. >> >>> Now, If I change the property definition to this: >>> >>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>> b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>> index 2cef18ca737c..5c7865180383 100644 >>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>> @@ -40,8 +40,11 @@ properties: >>> const: 2 >>> >>> ngpios: >>> - minimum: 1 >>> - maximum: 32 >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + The number of GPIO pins implemented by the controller. >>> + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. >>> + >>> >>> gpio-controller: true >>> >>> Then all is OK. >>> >>> Which option should I go for here ? If we want to avoid a dtbs_check error, as >>> far as I can see, we can: >>> 1) Remove the ngpios property and remove its use from the DTS, which is not >>> nice in my opinion >> >> Again, it depends if there are users depending on it. A user being a >> GPIO driver somewhere, not a DTS file. The GPIO driver in the kernel >> doesn't need it. So u-boot? BSD? > > The Linux driver uses the number of interrupts for the number of gpios but > upstream U-Boot uses the ngpios property. So I will change this to use > "default: 16" as you suggested. There is no reasonable default for this hardware. I would much rather you keep the schema as-is, or at least go with the second option. --Sean > > Thanks ! > >> >>> 2) Use the modification proposed above >
On Fri, 2021-02-05 at 17:55 -0500, Sean Anderson wrote: > On 2/5/21 5:53 PM, Damien Le Moal wrote: > > On Fri, 2021-02-05 at 14:02 -0600, Rob Herring wrote: > > > On Wed, Feb 3, 2021 at 6:47 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > > > > > > > On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: > > > > > On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > > > > > > > > > > > On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: > > > > > > > On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > > > > > > > > > > > > > > The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > > > > > > > > interrupts property description and maxItems. Also add the standard > > > > > > > > ngpios property to describe the number of GPIOs available on the > > > > > > > > implementation. > > > > > > > > > > > > > > > > Also add the "canaan,k210-gpiohs" compatible string to indicate the use > > > > > > > > of this gpio controller in the Canaan Kendryte K210 SoC. If this > > > > > > > > compatible string is used, do not define the clocks property as > > > > > > > > required as the K210 SoC does not have a software controllable clock > > > > > > > > for the Sifive gpio IP block. > > > > > > > > > > > > > > > > Cc: Paul Walmsley <paul.walmsley@sifive.com> > > > > > > > > Cc: Rob Herring <robh@kernel.org> > > > > > > > > Cc: devicetree@vger.kernel.org > > > > > > > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > > > > > > > > --- > > > > > > > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- > > > > > > > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > > > > index ab22056f8b44..2cef18ca737c 100644 > > > > > > > > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > > > > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > > > > @@ -16,6 +16,7 @@ properties: > > > > > > > > - enum: > > > > > > > > - sifive,fu540-c000-gpio > > > > > > > > - sifive,fu740-c000-gpio > > > > > > > > + - canaan,k210-gpiohs > > > > > > > > - const: sifive,gpio0 > > > > > > > > > > > > > > > > reg: > > > > > > > > @@ -23,9 +24,9 @@ properties: > > > > > > > > > > > > > > > > interrupts: > > > > > > > > description: > > > > > > > > - interrupt mapping one per GPIO. Maximum 16 GPIOs. > > > > > > > > + interrupt mapping one per GPIO. Maximum 32 GPIOs. > > > > > > > > minItems: 1 > > > > > > > > - maxItems: 16 > > > > > > > > + maxItems: 32 > > > > > > > > > > > > > > > > interrupt-controller: true > > > > > > > > > > > > > > > > @@ -38,6 +39,10 @@ properties: > > > > > > > > "#gpio-cells": > > > > > > > > const: 2 > > > > > > > > > > > > > > > > + ngpios: > > > > > > > > + minimum: 1 > > > > > > > > + maximum: 32 > > > > > > > > > > > > > > What's the default as obviously drivers already assume something. > > > > > > > > > > > > > > Does a driver actually need to know this? For example, does the > > > > > > > register stride change or something? > > > > > > > > > > > > > > Please don't add it if the only purpose is error check your DT (IOW, > > > > > > > if it just checks the max cell value in gpios phandles). > > > > > > > > > > > > If I remove that, make dtbs_check complains. Looking at othe gpio controller > > > > > > bindings, they all have it. So isn't it better to be consistent, and avoid make > > > > > > dtbs_check errors ? > > > > > > > > > > That would mean you are already using 'ngpios' and it is undocumented > > > > > (for this binding). If already in use and possibly having users then > > > > > that changes things, but that's not what the commit msg says. > > > > > > > > > > Not *all* gpio controllers have ngpios. It's a good number, but > > > > > probably more than need it though. If we wanted it everywhere, there > > > > > would be a schema enforcing that. > > > > > > > > If I remove the minimum and maximum lines, I get this error: > > > > > > I never said remove minimum/maximum. The suggestion is either add > > > 'default: 16' or remove 'ngpios' entirely. > > > > > > > ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty > > > > value in block mapping (empty-values) > > > > CHKDT Documentation/devicetree/bindings/processed-schema-examples.json > > > > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > > > > ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' > > > > SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json > > > > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > > > > ,gpio.yaml: ignoring, error in schema: properties: ngpios > > > > warning: no schema found in file: > > > > ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > > > ngpios: true > > > > > > or > > > > > > ngpios: {} > > > > > > Are the minimum valid values for a key. (Though not what should be done here.) > > > > > > > > > > > If I remove the ngpios property entirely, then I get a hit on the device tree: > > > > > > > > CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml > > > > /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: > > > > gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- > > > > [0-9]+' > > > > From schema: > > > > /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > > > > ,gpio.yaml > > > > > > That's not upstream, right? Then fix it. > > > > > > > Now, If I change the property definition to this: > > > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > index 2cef18ca737c..5c7865180383 100644 > > > > --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > > > > @@ -40,8 +40,11 @@ properties: > > > > const: 2 > > > > > > > > ngpios: > > > > - minimum: 1 > > > > - maximum: 32 > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + description: > > > > + The number of GPIO pins implemented by the controller. > > > > + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. > > > > + > > > > > > > > gpio-controller: true > > > > > > > > Then all is OK. > > > > > > > > Which option should I go for here ? If we want to avoid a dtbs_check error, as > > > > far as I can see, we can: > > > > 1) Remove the ngpios property and remove its use from the DTS, which is not > > > > nice in my opinion > > > > > > Again, it depends if there are users depending on it. A user being a > > > GPIO driver somewhere, not a DTS file. The GPIO driver in the kernel > > > doesn't need it. So u-boot? BSD? > > > > The Linux driver uses the number of interrupts for the number of gpios but > > upstream U-Boot uses the ngpios property. So I will change this to use > > "default: 16" as you suggested. > > There is no reasonable default for this hardware. I would much rather > you keep the schema as-is, or at least go with the second option. Since the SiFive official doc seems to say "16" as the number of gpio for this controller, we could assume that to be the default. No ? But I agree that clearly, the implementation can be hacked to have any number of GPIOs... > > --Sean > > > > > Thanks ! > > > > > > > > > 2) Use the modification proposed above > > > -- Damien Le Moal Western Digital Research
On 2/5/21 6:32 PM, Damien Le Moal wrote: > On Fri, 2021-02-05 at 17:55 -0500, Sean Anderson wrote: >> On 2/5/21 5:53 PM, Damien Le Moal wrote: >>> On Fri, 2021-02-05 at 14:02 -0600, Rob Herring wrote: >>>> On Wed, Feb 3, 2021 at 6:47 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >>>>> >>>>> On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: >>>>>> On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >>>>>>> >>>>>>> On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: >>>>>>>> On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >>>>>>>>> >>>>>>>>> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the >>>>>>>>> interrupts property description and maxItems. Also add the standard >>>>>>>>> ngpios property to describe the number of GPIOs available on the >>>>>>>>> implementation. >>>>>>>>> >>>>>>>>> Also add the "canaan,k210-gpiohs" compatible string to indicate the use >>>>>>>>> of this gpio controller in the Canaan Kendryte K210 SoC. If this >>>>>>>>> compatible string is used, do not define the clocks property as >>>>>>>>> required as the K210 SoC does not have a software controllable clock >>>>>>>>> for the Sifive gpio IP block. >>>>>>>>> >>>>>>>>> Cc: Paul Walmsley <paul.walmsley@sifive.com> >>>>>>>>> Cc: Rob Herring <robh@kernel.org> >>>>>>>>> Cc: devicetree@vger.kernel.org >>>>>>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>>>>>>>> --- >>>>>>>>> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- >>>>>>>>> 1 file changed, 18 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>>>> index ab22056f8b44..2cef18ca737c 100644 >>>>>>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>>>> @@ -16,6 +16,7 @@ properties: >>>>>>>>> - enum: >>>>>>>>> - sifive,fu540-c000-gpio >>>>>>>>> - sifive,fu740-c000-gpio >>>>>>>>> + - canaan,k210-gpiohs >>>>>>>>> - const: sifive,gpio0 >>>>>>>>> >>>>>>>>> reg: >>>>>>>>> @@ -23,9 +24,9 @@ properties: >>>>>>>>> >>>>>>>>> interrupts: >>>>>>>>> description: >>>>>>>>> - interrupt mapping one per GPIO. Maximum 16 GPIOs. >>>>>>>>> + interrupt mapping one per GPIO. Maximum 32 GPIOs. >>>>>>>>> minItems: 1 >>>>>>>>> - maxItems: 16 >>>>>>>>> + maxItems: 32 >>>>>>>>> >>>>>>>>> interrupt-controller: true >>>>>>>>> >>>>>>>>> @@ -38,6 +39,10 @@ properties: >>>>>>>>> "#gpio-cells": >>>>>>>>> const: 2 >>>>>>>>> >>>>>>>>> + ngpios: >>>>>>>>> + minimum: 1 >>>>>>>>> + maximum: 32 >>>>>>>> >>>>>>>> What's the default as obviously drivers already assume something. >>>>>>>> >>>>>>>> Does a driver actually need to know this? For example, does the >>>>>>>> register stride change or something? >>>>>>>> >>>>>>>> Please don't add it if the only purpose is error check your DT (IOW, >>>>>>>> if it just checks the max cell value in gpios phandles). >>>>>>> >>>>>>> If I remove that, make dtbs_check complains. Looking at othe gpio controller >>>>>>> bindings, they all have it. So isn't it better to be consistent, and avoid make >>>>>>> dtbs_check errors ? >>>>>> >>>>>> That would mean you are already using 'ngpios' and it is undocumented >>>>>> (for this binding). If already in use and possibly having users then >>>>>> that changes things, but that's not what the commit msg says. >>>>>> >>>>>> Not *all* gpio controllers have ngpios. It's a good number, but >>>>>> probably more than need it though. If we wanted it everywhere, there >>>>>> would be a schema enforcing that. >>>>> >>>>> If I remove the minimum and maximum lines, I get this error: >>>> >>>> I never said remove minimum/maximum. The suggestion is either add >>>> 'default: 16' or remove 'ngpios' entirely. >>>> >>>>> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty >>>>> value in block mapping (empty-values) >>>>> CHKDT Documentation/devicetree/bindings/processed-schema-examples.json >>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>>>> ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' >>>>> SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json >>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>>>> ,gpio.yaml: ignoring, error in schema: properties: ngpios >>>>> warning: no schema found in file: >>>>> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>> >>>> ngpios: true >>>> >>>> or >>>> >>>> ngpios: {} >>>> >>>> Are the minimum valid values for a key. (Though not what should be done here.) >>>> >>>>> >>>>> If I remove the ngpios property entirely, then I get a hit on the device tree: >>>>> >>>>> CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml >>>>> /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: >>>>> gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- >>>>> [0-9]+' >>>>> From schema: >>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>>>> ,gpio.yaml >>>> >>>> That's not upstream, right? Then fix it. >>>> >>>>> Now, If I change the property definition to this: >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>> b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>> index 2cef18ca737c..5c7865180383 100644 >>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>> @@ -40,8 +40,11 @@ properties: >>>>> const: 2 >>>>> >>>>> ngpios: >>>>> - minimum: 1 >>>>> - maximum: 32 >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: >>>>> + The number of GPIO pins implemented by the controller. >>>>> + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. >>>>> + >>>>> >>>>> gpio-controller: true >>>>> >>>>> Then all is OK. >>>>> >>>>> Which option should I go for here ? If we want to avoid a dtbs_check error, as >>>>> far as I can see, we can: >>>>> 1) Remove the ngpios property and remove its use from the DTS, which is not >>>>> nice in my opinion >>>> >>>> Again, it depends if there are users depending on it. A user being a >>>> GPIO driver somewhere, not a DTS file. The GPIO driver in the kernel >>>> doesn't need it. So u-boot? BSD? >>> >>> The Linux driver uses the number of interrupts for the number of gpios but >>> upstream U-Boot uses the ngpios property. So I will change this to use >>> "default: 16" as you suggested. >> >> There is no reasonable default for this hardware. I would much rather >> you keep the schema as-is, or at least go with the second option. > > Since the SiFive official doc seems to say "16" as the number of gpio for this > controller, we could assume that to be the default. No ? But I agree that > clearly, the implementation can be hacked to have any number of GPIOs... Keep in mind that those docs are for SiFive's particular instantiation of that IP, not for the IP in general. Although some parameters (e.g. dsWidth) have defaults, width does not. --Sean > > >> >> --Sean >> >>> >>> Thanks ! >>> >>>> >>>>> 2) Use the modification proposed above >>> >> >
On 2021/02/06 9:31, Sean Anderson wrote: > On 2/5/21 6:32 PM, Damien Le Moal wrote: >> On Fri, 2021-02-05 at 17:55 -0500, Sean Anderson wrote: >>> On 2/5/21 5:53 PM, Damien Le Moal wrote: >>>> On Fri, 2021-02-05 at 14:02 -0600, Rob Herring wrote: >>>>> On Wed, Feb 3, 2021 at 6:47 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >>>>>> >>>>>> On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: >>>>>>> On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >>>>>>>> >>>>>>>> On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: >>>>>>>>> On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >>>>>>>>>> >>>>>>>>>> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the >>>>>>>>>> interrupts property description and maxItems. Also add the standard >>>>>>>>>> ngpios property to describe the number of GPIOs available on the >>>>>>>>>> implementation. >>>>>>>>>> >>>>>>>>>> Also add the "canaan,k210-gpiohs" compatible string to indicate the use >>>>>>>>>> of this gpio controller in the Canaan Kendryte K210 SoC. If this >>>>>>>>>> compatible string is used, do not define the clocks property as >>>>>>>>>> required as the K210 SoC does not have a software controllable clock >>>>>>>>>> for the Sifive gpio IP block. >>>>>>>>>> >>>>>>>>>> Cc: Paul Walmsley <paul.walmsley@sifive.com> >>>>>>>>>> Cc: Rob Herring <robh@kernel.org> >>>>>>>>>> Cc: devicetree@vger.kernel.org >>>>>>>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>>>>>>>>> --- >>>>>>>>>> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- >>>>>>>>>> 1 file changed, 18 insertions(+), 3 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>>>>> index ab22056f8b44..2cef18ca737c 100644 >>>>>>>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>>>>>> @@ -16,6 +16,7 @@ properties: >>>>>>>>>> - enum: >>>>>>>>>> - sifive,fu540-c000-gpio >>>>>>>>>> - sifive,fu740-c000-gpio >>>>>>>>>> + - canaan,k210-gpiohs >>>>>>>>>> - const: sifive,gpio0 >>>>>>>>>> >>>>>>>>>> reg: >>>>>>>>>> @@ -23,9 +24,9 @@ properties: >>>>>>>>>> >>>>>>>>>> interrupts: >>>>>>>>>> description: >>>>>>>>>> - interrupt mapping one per GPIO. Maximum 16 GPIOs. >>>>>>>>>> + interrupt mapping one per GPIO. Maximum 32 GPIOs. >>>>>>>>>> minItems: 1 >>>>>>>>>> - maxItems: 16 >>>>>>>>>> + maxItems: 32 >>>>>>>>>> >>>>>>>>>> interrupt-controller: true >>>>>>>>>> >>>>>>>>>> @@ -38,6 +39,10 @@ properties: >>>>>>>>>> "#gpio-cells": >>>>>>>>>> const: 2 >>>>>>>>>> >>>>>>>>>> + ngpios: >>>>>>>>>> + minimum: 1 >>>>>>>>>> + maximum: 32 >>>>>>>>> >>>>>>>>> What's the default as obviously drivers already assume something. >>>>>>>>> >>>>>>>>> Does a driver actually need to know this? For example, does the >>>>>>>>> register stride change or something? >>>>>>>>> >>>>>>>>> Please don't add it if the only purpose is error check your DT (IOW, >>>>>>>>> if it just checks the max cell value in gpios phandles). >>>>>>>> >>>>>>>> If I remove that, make dtbs_check complains. Looking at othe gpio controller >>>>>>>> bindings, they all have it. So isn't it better to be consistent, and avoid make >>>>>>>> dtbs_check errors ? >>>>>>> >>>>>>> That would mean you are already using 'ngpios' and it is undocumented >>>>>>> (for this binding). If already in use and possibly having users then >>>>>>> that changes things, but that's not what the commit msg says. >>>>>>> >>>>>>> Not *all* gpio controllers have ngpios. It's a good number, but >>>>>>> probably more than need it though. If we wanted it everywhere, there >>>>>>> would be a schema enforcing that. >>>>>> >>>>>> If I remove the minimum and maximum lines, I get this error: >>>>> >>>>> I never said remove minimum/maximum. The suggestion is either add >>>>> 'default: 16' or remove 'ngpios' entirely. >>>>> >>>>>> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty >>>>>> value in block mapping (empty-values) >>>>>> CHKDT Documentation/devicetree/bindings/processed-schema-examples.json >>>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>>>>> ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' >>>>>> SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json >>>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>>>>> ,gpio.yaml: ignoring, error in schema: properties: ngpios >>>>>> warning: no schema found in file: >>>>>> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>> >>>>> ngpios: true >>>>> >>>>> or >>>>> >>>>> ngpios: {} >>>>> >>>>> Are the minimum valid values for a key. (Though not what should be done here.) >>>>> >>>>>> >>>>>> If I remove the ngpios property entirely, then I get a hit on the device tree: >>>>>> >>>>>> CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml >>>>>> /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: >>>>>> gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- >>>>>> [0-9]+' >>>>>> From schema: >>>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive >>>>>> ,gpio.yaml >>>>> >>>>> That's not upstream, right? Then fix it. >>>>> >>>>>> Now, If I change the property definition to this: >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>> b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>> index 2cef18ca737c..5c7865180383 100644 >>>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml >>>>>> @@ -40,8 +40,11 @@ properties: >>>>>> const: 2 >>>>>> >>>>>> ngpios: >>>>>> - minimum: 1 >>>>>> - maximum: 32 >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>> + description: >>>>>> + The number of GPIO pins implemented by the controller. >>>>>> + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. >>>>>> + >>>>>> >>>>>> gpio-controller: true >>>>>> >>>>>> Then all is OK. >>>>>> >>>>>> Which option should I go for here ? If we want to avoid a dtbs_check error, as >>>>>> far as I can see, we can: >>>>>> 1) Remove the ngpios property and remove its use from the DTS, which is not >>>>>> nice in my opinion >>>>> >>>>> Again, it depends if there are users depending on it. A user being a >>>>> GPIO driver somewhere, not a DTS file. The GPIO driver in the kernel >>>>> doesn't need it. So u-boot? BSD? >>>> >>>> The Linux driver uses the number of interrupts for the number of gpios but >>>> upstream U-Boot uses the ngpios property. So I will change this to use >>>> "default: 16" as you suggested. >>> >>> There is no reasonable default for this hardware. I would much rather >>> you keep the schema as-is, or at least go with the second option. >> >> Since the SiFive official doc seems to say "16" as the number of gpio for this >> controller, we could assume that to be the default. No ? But I agree that >> clearly, the implementation can be hacked to have any number of GPIOs... > > Keep in mind that those docs are for SiFive's particular instantiation > of that IP, not for the IP in general. Although some parameters (e.g. > dsWidth) have defaults, width does not. OK. Then I think the simplest is to keep the minimum/maximum. Many binding docs use that anyway. Rob, any objections ? > > --Sean > >> >> >>> >>> --Sean >>> >>>> >>>> Thanks ! >>>> >>>>> >>>>>> 2) Use the modification proposed above >>>> >>> >> > >
On Fri, Feb 5, 2021 at 6:52 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On 2021/02/06 9:31, Sean Anderson wrote: > > On 2/5/21 6:32 PM, Damien Le Moal wrote: > >> On Fri, 2021-02-05 at 17:55 -0500, Sean Anderson wrote: > >>> On 2/5/21 5:53 PM, Damien Le Moal wrote: > >>>> On Fri, 2021-02-05 at 14:02 -0600, Rob Herring wrote: > >>>>> On Wed, Feb 3, 2021 at 6:47 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > >>>>>> > >>>>>> On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote: > >>>>>>> On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > >>>>>>>> > >>>>>>>> On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote: > >>>>>>>>> On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > >>>>>>>>>> > >>>>>>>>>> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the > >>>>>>>>>> interrupts property description and maxItems. Also add the standard > >>>>>>>>>> ngpios property to describe the number of GPIOs available on the > >>>>>>>>>> implementation. > >>>>>>>>>> > >>>>>>>>>> Also add the "canaan,k210-gpiohs" compatible string to indicate the use > >>>>>>>>>> of this gpio controller in the Canaan Kendryte K210 SoC. If this > >>>>>>>>>> compatible string is used, do not define the clocks property as > >>>>>>>>>> required as the K210 SoC does not have a software controllable clock > >>>>>>>>>> for the Sifive gpio IP block. > >>>>>>>>>> > >>>>>>>>>> Cc: Paul Walmsley <paul.walmsley@sifive.com> > >>>>>>>>>> Cc: Rob Herring <robh@kernel.org> > >>>>>>>>>> Cc: devicetree@vger.kernel.org > >>>>>>>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > >>>>>>>>>> --- > >>>>>>>>>> .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- > >>>>>>>>>> 1 file changed, 18 insertions(+), 3 deletions(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >>>>>>>>>> index ab22056f8b44..2cef18ca737c 100644 > >>>>>>>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >>>>>>>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >>>>>>>>>> @@ -16,6 +16,7 @@ properties: > >>>>>>>>>> - enum: > >>>>>>>>>> - sifive,fu540-c000-gpio > >>>>>>>>>> - sifive,fu740-c000-gpio > >>>>>>>>>> + - canaan,k210-gpiohs > >>>>>>>>>> - const: sifive,gpio0 > >>>>>>>>>> > >>>>>>>>>> reg: > >>>>>>>>>> @@ -23,9 +24,9 @@ properties: > >>>>>>>>>> > >>>>>>>>>> interrupts: > >>>>>>>>>> description: > >>>>>>>>>> - interrupt mapping one per GPIO. Maximum 16 GPIOs. > >>>>>>>>>> + interrupt mapping one per GPIO. Maximum 32 GPIOs. > >>>>>>>>>> minItems: 1 > >>>>>>>>>> - maxItems: 16 > >>>>>>>>>> + maxItems: 32 > >>>>>>>>>> > >>>>>>>>>> interrupt-controller: true > >>>>>>>>>> > >>>>>>>>>> @@ -38,6 +39,10 @@ properties: > >>>>>>>>>> "#gpio-cells": > >>>>>>>>>> const: 2 > >>>>>>>>>> > >>>>>>>>>> + ngpios: > >>>>>>>>>> + minimum: 1 > >>>>>>>>>> + maximum: 32 > >>>>>>>>> > >>>>>>>>> What's the default as obviously drivers already assume something. > >>>>>>>>> > >>>>>>>>> Does a driver actually need to know this? For example, does the > >>>>>>>>> register stride change or something? > >>>>>>>>> > >>>>>>>>> Please don't add it if the only purpose is error check your DT (IOW, > >>>>>>>>> if it just checks the max cell value in gpios phandles). > >>>>>>>> > >>>>>>>> If I remove that, make dtbs_check complains. Looking at othe gpio controller > >>>>>>>> bindings, they all have it. So isn't it better to be consistent, and avoid make > >>>>>>>> dtbs_check errors ? > >>>>>>> > >>>>>>> That would mean you are already using 'ngpios' and it is undocumented > >>>>>>> (for this binding). If already in use and possibly having users then > >>>>>>> that changes things, but that's not what the commit msg says. > >>>>>>> > >>>>>>> Not *all* gpio controllers have ngpios. It's a good number, but > >>>>>>> probably more than need it though. If we wanted it everywhere, there > >>>>>>> would be a schema enforcing that. > >>>>>> > >>>>>> If I remove the minimum and maximum lines, I get this error: > >>>>> > >>>>> I never said remove minimum/maximum. The suggestion is either add > >>>>> 'default: 16' or remove 'ngpios' entirely. > >>>>> > >>>>>> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty > >>>>>> value in block mapping (empty-values) > >>>>>> CHKDT Documentation/devicetree/bindings/processed-schema-examples.json > >>>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > >>>>>> ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean' > >>>>>> SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json > >>>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > >>>>>> ,gpio.yaml: ignoring, error in schema: properties: ngpios > >>>>>> warning: no schema found in file: > >>>>>> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >>>>> > >>>>> ngpios: true > >>>>> > >>>>> or > >>>>> > >>>>> ngpios: {} > >>>>> > >>>>> Are the minimum valid values for a key. (Though not what should be done here.) > >>>>> > >>>>>> > >>>>>> If I remove the ngpios property entirely, then I get a hit on the device tree: > >>>>>> > >>>>>> CHECK arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml > >>>>>> /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml: > >>>>>> gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl- > >>>>>> [0-9]+' > >>>>>> From schema: > >>>>>> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive > >>>>>> ,gpio.yaml > >>>>> > >>>>> That's not upstream, right? Then fix it. > >>>>> > >>>>>> Now, If I change the property definition to this: > >>>>>> > >>>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >>>>>> b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >>>>>> index 2cef18ca737c..5c7865180383 100644 > >>>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >>>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml > >>>>>> @@ -40,8 +40,11 @@ properties: > >>>>>> const: 2 > >>>>>> > >>>>>> ngpios: > >>>>>> - minimum: 1 > >>>>>> - maximum: 32 > >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>>>>> + description: > >>>>>> + The number of GPIO pins implemented by the controller. > >>>>>> + It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC. > >>>>>> + > >>>>>> > >>>>>> gpio-controller: true > >>>>>> > >>>>>> Then all is OK. > >>>>>> > >>>>>> Which option should I go for here ? If we want to avoid a dtbs_check error, as > >>>>>> far as I can see, we can: > >>>>>> 1) Remove the ngpios property and remove its use from the DTS, which is not > >>>>>> nice in my opinion > >>>>> > >>>>> Again, it depends if there are users depending on it. A user being a > >>>>> GPIO driver somewhere, not a DTS file. The GPIO driver in the kernel > >>>>> doesn't need it. So u-boot? BSD? > >>>> > >>>> The Linux driver uses the number of interrupts for the number of gpios but > >>>> upstream U-Boot uses the ngpios property. So I will change this to use > >>>> "default: 16" as you suggested. > >>> > >>> There is no reasonable default for this hardware. I would much rather > >>> you keep the schema as-is, or at least go with the second option. > >> > >> Since the SiFive official doc seems to say "16" as the number of gpio for this > >> controller, we could assume that to be the default. No ? But I agree that > >> clearly, the implementation can be hacked to have any number of GPIOs... Yes, 'default' is simply what a driver should assume if the property is not present. Drivers had to assume something already, right? Or simply the value doesn't matter as I've suggested. > > Keep in mind that those docs are for SiFive's particular instantiation > > of that IP, not for the IP in general. Although some parameters (e.g. > > dsWidth) have defaults, width does not. > > OK. Then I think the simplest is to keep the minimum/maximum. Many binding docs > use that anyway. Either that or an enum is required IMO. 32 is clearly the max here or the register layout would have to change. Rob
diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml index ab22056f8b44..2cef18ca737c 100644 --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml @@ -16,6 +16,7 @@ properties: - enum: - sifive,fu540-c000-gpio - sifive,fu740-c000-gpio + - canaan,k210-gpiohs - const: sifive,gpio0 reg: @@ -23,9 +24,9 @@ properties: interrupts: description: - interrupt mapping one per GPIO. Maximum 16 GPIOs. + interrupt mapping one per GPIO. Maximum 32 GPIOs. minItems: 1 - maxItems: 16 + maxItems: 32 interrupt-controller: true @@ -38,6 +39,10 @@ properties: "#gpio-cells": const: 2 + ngpios: + minimum: 1 + maximum: 32 + gpio-controller: true required: @@ -46,10 +51,20 @@ required: - interrupts - interrupt-controller - "#interrupt-cells" - - clocks - "#gpio-cells" - gpio-controller +if: + properties: + compatible: + contains: + enum: + - sifive,fu540-c000-gpio + - sifive,fu740-c000-gpio +then: + required: + - clocks + additionalProperties: false examples:
The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the interrupts property description and maxItems. Also add the standard ngpios property to describe the number of GPIOs available on the implementation. Also add the "canaan,k210-gpiohs" compatible string to indicate the use of this gpio controller in the Canaan Kendryte K210 SoC. If this compatible string is used, do not define the clocks property as required as the K210 SoC does not have a software controllable clock for the Sifive gpio IP block. Cc: Paul Walmsley <paul.walmsley@sifive.com> Cc: Rob Herring <robh@kernel.org> Cc: devicetree@vger.kernel.org Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)