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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx> 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:
>>>>>
>>>>> 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
>>>>
>>>
>>
> 
> 


-- 
Damien Le Moal
Western Digital Research




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux