On 2021/02/06 9:31, Sean Anderson wrote:

On 2/5/21 6:32 PM, Damien Le Moal wrote:

quoted

On Fri, 2021-02-05 at 17:55 -0500, Sean Anderson wrote:

quoted

On 2/5/21 5:53 PM, Damien Le Moal wrote:

quoted

On Fri, 2021-02-05 at 14:02 -0600, Rob Herring wrote:

quoted

On Wed, Feb 3, 2021 at 6:47 PM Damien Le Moal [off-list ref] wrote:

quoted

On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote:

quoted

On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal [off-list ref] wrote:

quoted

On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote:

quoted

On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal [off-list ref] wrote:

quoted

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 <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Damien Le Moal <redacted>
---
   .../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.

quoted

./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.)

quoted

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.

quoted

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

quoted

quoted

--Sean

quoted

Thanks !

quoted

quoted

2) Use the modification proposed above

-- 
Damien Le Moal
Western Digital Research

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv