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@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxx> >>>>> Cc: Rob Herring <robh@xxxxxxxxxx> >>>>> Cc: devicetree@xxxxxxxxxxxxxxx >>>>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxx> >>>>> --- >>>>> .../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 ! -- Damien Le Moal Western Digital Research
