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
