On Fri, Feb 5, 2021 at 6:52 PM Damien Le Moal <Damien.LeMoal@xxxxxxx> wrote:
>
> 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...
Yes, 'default' is simply what a driver should assume if the property
is not present. Drivers had to assume something already, right? Or
simply the value doesn't matter as I've suggested.
> > 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.
Either that or an enum is required IMO.
32 is clearly the max here or the register layout would have to change.
Rob
