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

[Sean Anderson <seanga2@gmail.com>]

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