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

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

 



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.

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