Re: [PATCH v14 07/16] dt-bindings: fix sifive gpio properties

[Sean Anderson <seanga2@gmail.com>]

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
>