| Message ID | 1527924610-13135-3-git-send-email-sergio.paracuellos@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mediatek: driver for gpio chip in MT7621 SoC | expand |
On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote: > Add a devicetree binding documentation for the mt7621 driver. Bindings are for h/w, not a driver. > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > Reviewed-by: NeilBrown <neil@brown.name> Space ^ > --- > .../bindings/gpio/mediatek,mt7621-gpio.txt | 68 ++++++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt > > diff --git a/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt > new file mode 100644 > index 0000000..30d8a02 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt > @@ -0,0 +1,68 @@ > +Mediatek SoC GPIO controller bindings > + > +The IP core used inside these SoCs has 3 banks of 32 GPIOs each. > +The registers of all the banks are interwoven inside one single IO range. > +We load one GPIO controller instance per bank. To make this possible > +we support 2 types of nodes. The parent node defines the memory I/O range and > +has 3 children each describing a single bank. Also the GPIO controller can receive > +interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU > +using GIC INT12. > + > +Required properties for the top level node: > +- compatible: > + - "mediatek,mt7621-gpio" for Mediatek controllers > +- reg : Physical base address and length of the controller's registers > +- interrupt-parent : phandle of the parent interrupt controller. > +- interrupts : Interrupt specifier for the controllers interrupt. > +- interrupt-controller : Mark the device node as an interrupt controller. > +- #interrupt-cells : Should be 2. The first cell defines the interrupt number. > + The second cell bits[3:0] is used to specify trigger type as follows: > + - 1 = low-to-high edge triggered. > + - 2 = high-to-low edge triggered. > + - 4 = active high level-sensitive. > + - 8 = active low level-sensitive. Just refer to the common definition. > + > + > +Required properties for the GPIO bank node: > +- compatible: > + - "mediatek,mt7621-gpio-bank" for Mediatek banks > +- #gpio-cells : Should be two. The first cell is the GPIO pin number and the So interrupt numbers and gpio numbers are different? 0-95 and 3x 0-31 That doesn't seem ideal. > + second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. > + Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported. > +- gpio-controller : Marks the device node as a GPIO controller. > +- reg : The id of the bank that the node describes. I'd prefer to not have banks defined in DT. Do you have a variable number or resources that are per bank? If not, then you don't need them. > + > +Example: > + gpio@600 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + compatible = "mediatek,mt7621-gpio"; > + reg = <0x600 0x100>; > + > + interrupt-parent = <&gic>; > + interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + #interrupt-cells = <2>; > + > + gpio0: bank@0 { > + reg = <0>; > + compatible = "mediatek,mt7621-gpio-bank"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + > + gpio1: bank@1 { > + reg = <1>; > + compatible = "mediatek,mt7621-gpio-bank"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + > + gpio2: bank@2 { > + reg = <2>; > + compatible = "mediatek,mt7621-gpio-bank"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + }; > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 12, 2018 at 02:56:38PM -0600, Rob Herring wrote: > Bindings are for h/w, not a driver. > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > > Reviewed-by: NeilBrown <neil@brown.name> > > Space ^ Pretty sure that was deliberate... Otherwise he's been making that same mistake for over a decade now. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, Thanks for your time in reviewing this. On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring <robh@kernel.org> wrote: > On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote: >> Add a devicetree binding documentation for the mt7621 driver. > > Bindings are for h/w, not a driver. You are totally right, my english is not my best as you can see :-). I'll fix this for v2. > >> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> >> Reviewed-by: NeilBrown <neil@brown.name> > > Space ^ > Actually, this is deliberate, so it will not change. >> --- >> .../bindings/gpio/mediatek,mt7621-gpio.txt | 68 ++++++++++++++++++++++ >> 1 file changed, 68 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt >> >> diff --git a/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt >> new file mode 100644 >> index 0000000..30d8a02 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt >> @@ -0,0 +1,68 @@ >> +Mediatek SoC GPIO controller bindings >> + >> +The IP core used inside these SoCs has 3 banks of 32 GPIOs each. >> +The registers of all the banks are interwoven inside one single IO range. >> +We load one GPIO controller instance per bank. To make this possible >> +we support 2 types of nodes. The parent node defines the memory I/O range and >> +has 3 children each describing a single bank. Also the GPIO controller can receive >> +interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU >> +using GIC INT12. >> + >> +Required properties for the top level node: >> +- compatible: >> + - "mediatek,mt7621-gpio" for Mediatek controllers >> +- reg : Physical base address and length of the controller's registers >> +- interrupt-parent : phandle of the parent interrupt controller. >> +- interrupts : Interrupt specifier for the controllers interrupt. >> +- interrupt-controller : Mark the device node as an interrupt controller. >> +- #interrupt-cells : Should be 2. The first cell defines the interrupt number. >> + The second cell bits[3:0] is used to specify trigger type as follows: >> + - 1 = low-to-high edge triggered. >> + - 2 = high-to-low edge triggered. >> + - 4 = active high level-sensitive. >> + - 8 = active low level-sensitive. > > Just refer to the common definition. ok, thanks. I will. > >> + >> + >> +Required properties for the GPIO bank node: >> +- compatible: >> + - "mediatek,mt7621-gpio-bank" for Mediatek banks >> +- #gpio-cells : Should be two. The first cell is the GPIO pin number and the > > So interrupt numbers and gpio numbers are different? 0-95 and 3x 0-31 > > That doesn't seem ideal. Yes, that's true. Actually this is one of the things that has been changed for v2. > >> + second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. >> + Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported. >> +- gpio-controller : Marks the device node as a GPIO controller. >> +- reg : The id of the bank that the node describes. > > I'd prefer to not have banks defined in DT. Do you have a variable > number or resources that are per bank? If not, then you don't need them. Mmmm, That's what I understood from documentation: "Some system-on-chips (SoCs) use the concept of GPIO banks. ... Usually each such bank is exposed in the device tree as an individual gpio-controller node. ..." If this is not a good approach, could you please me point me out to a device tree example where the correct approach is being used? Thanks in advance. Best regards, Sergio Paracuellos >> + >> +Example: >> + gpio@600 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + compatible = "mediatek,mt7621-gpio"; >> + reg = <0x600 0x100>; >> + >> + interrupt-parent = <&gic>; >> + interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + >> + gpio0: bank@0 { >> + reg = <0>; >> + compatible = "mediatek,mt7621-gpio-bank"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> + >> + gpio1: bank@1 { >> + reg = <1>; >> + compatible = "mediatek,mt7621-gpio-bank"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> + >> + gpio2: bank@2 { >> + reg = <2>; >> + compatible = "mediatek,mt7621-gpio-bank"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> + }; >> -- >> 2.7.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 13, 2018 at 10:23 AM, Sergio Paracuellos <sergio.paracuellos@gmail.com> wrote: > Hi Rob, > > Thanks for your time in reviewing this. > > On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring <robh@kernel.org> wrote: >> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote: >>> Add a devicetree binding documentation for the mt7621 driver. >> >>> + second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. >>> + Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported. >>> +- gpio-controller : Marks the device node as a GPIO controller. >>> +- reg : The id of the bank that the node describes. >> >> I'd prefer to not have banks defined in DT. Do you have a variable >> number or resources that are per bank? If not, then you don't need them. > > Mmmm, That's what I understood from documentation: > > "Some system-on-chips (SoCs) use the concept of GPIO banks. ... > Usually each such bank is > exposed in the device tree as an individual gpio-controller node. ..." This should be conditioned on being able to divide up the registers by bank which seems like you can't. Or there's the case like the DW GPIO block and the number of banks is configurable. > If this is not a good approach, could you please me point me out to a > device tree example where > the correct approach is being used? I'm not sure offhand. There are lots of examples of single nodes I'm sure. Which ones have banks I haven't a clue. IIRC, there were some cases where the bank # was part of the GPIO cells, but I seem to recall Linus prefers not having 3 cells. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 13, 2018 at 01:28:35PM -0600, Rob Herring wrote: > On Wed, Jun 13, 2018 at 10:23 AM, Sergio Paracuellos > <sergio.paracuellos@gmail.com> wrote: > > Hi Rob, > > > > Thanks for your time in reviewing this. > > > > On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring <robh@kernel.org> wrote: > >> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote: > >>> Add a devicetree binding documentation for the mt7621 driver. > >> > > >>> + second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. > >>> + Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported. > >>> +- gpio-controller : Marks the device node as a GPIO controller. > >>> +- reg : The id of the bank that the node describes. > >> > >> I'd prefer to not have banks defined in DT. Do you have a variable > >> number or resources that are per bank? If not, then you don't need them. > > > > Mmmm, That's what I understood from documentation: > > > > "Some system-on-chips (SoCs) use the concept of GPIO banks. ... > > Usually each such bank is > > exposed in the device tree as an individual gpio-controller node. ..." > > This should be conditioned on being able to divide up the registers by > bank which seems like you can't. Or there's the case like the DW GPIO > block and the number of banks is configurable. I see. Thanks for clarifing this. > > > If this is not a good approach, could you please me point me out to a > > device tree example where > > the correct approach is being used? > > I'm not sure offhand. There are lots of examples of single nodes I'm > sure. Which ones have banks I haven't a clue. IIRC, there were some > cases where the bank # was part of the GPIO cells, but I seem to > recall Linus prefers not having 3 cells. Ok, so... does the following single node sounds acceptable? gpio: gpio@600 { #gpio-cells = <2>; #interrupt-cells = <2>; compatible = "mediatek,mt7621-gpio"; gpio-controller; interrupt-controller; reg = <0x600 0x60>; interrupt-parent = <&gic>; interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>; mediatek,gpio-bank-widths = <32 32 32>; } Changing definition for "reg" and adding a new one for "mediatek,gpio-bank-widths" as follows: reg: Define the base and range of the address space containing the mediatek GPIO controller registers mediatek,gpio-bank-widths: Number of GPIO lines for each bank. Number of elements must correspond to number of banks suggested by the 'reg' property. Thanks in advance. > > Rob Best regards, Sergio Paracuellos -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 13, 2018 at 9:28 PM, Rob Herring <robh@kernel.org> wrote: >> "Some system-on-chips (SoCs) use the concept of GPIO banks. ... >> Usually each such bank is >> exposed in the device tree as an individual gpio-controller node. ..." > > This should be conditioned on being able to divide up the registers by > bank which seems like you can't. Or there's the case like the DW GPIO > block and the number of banks is configurable. If it is possible to create one device per bank I usually prefer that approach, as it also (often) makes it possible to use the generic GPIO library, i.e. the hardware abstraction start to share more with other GPIO controllers. >> If this is not a good approach, could you please me point me out to a >> device tree example where >> the correct approach is being used? > > I'm not sure offhand. There are lots of examples of single nodes I'm > sure. Which ones have banks I haven't a clue. IIRC, there were some > cases where the bank # was part of the GPIO cells, but I seem to > recall Linus prefers not having 3 cells. I don't like 3 cells, stuff is complicated enough as it is already. Better in that case to concatenate the offsets and instead of having an extra cell 0, 1 and offsets 0-31, 0-31 have two cells and offsets 0-63. My reasoning is that since it is represented by a single device we are indexing into that one device from 0-n. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 14, 2018 at 6:45 AM, Sergio Paracuellos <sergio.paracuellos@gmail.com> wrote: > Ok, so... does the following single node sounds acceptable? > > gpio: gpio@600 { > #gpio-cells = <2>; > #interrupt-cells = <2>; > compatible = "mediatek,mt7621-gpio"; > gpio-controller; > interrupt-controller; > reg = <0x600 0x60>; > interrupt-parent = <&gic>; > interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>; > mediatek,gpio-bank-widths = <32 32 32>; Why would you need this? It is pretty obvious from the compatible string isn't it? Just use that to tell that "aha, it is mediatek,mt7321-gpio, so it has 3x32 GPIOs indexed from 0..95". No need of overspecifying stuff. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 14, 2018 at 8:14 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Jun 13, 2018 at 9:28 PM, Rob Herring <robh@kernel.org> wrote: > >>> "Some system-on-chips (SoCs) use the concept of GPIO banks. ... >>> Usually each such bank is >>> exposed in the device tree as an individual gpio-controller node. ..." >> >> This should be conditioned on being able to divide up the registers by >> bank which seems like you can't. Or there's the case like the DW GPIO >> block and the number of banks is configurable. > > If it is possible to create one device per bank I usually prefer that > approach, as it also (often) makes it possible to use the > generic GPIO library, i.e. the hardware abstraction start to > share more with other GPIO controllers. But that should be possible whether there are banks in DT or not, right? Rob -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 14, 2018 at 4:33 PM, Rob Herring <robh@kernel.org> wrote: > On Thu, Jun 14, 2018 at 8:14 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Wed, Jun 13, 2018 at 9:28 PM, Rob Herring <robh@kernel.org> wrote: >> >>>> "Some system-on-chips (SoCs) use the concept of GPIO banks. ... >>>> Usually each such bank is >>>> exposed in the device tree as an individual gpio-controller node. ..." >>> >>> This should be conditioned on being able to divide up the registers by >>> bank which seems like you can't. Or there's the case like the DW GPIO >>> block and the number of banks is configurable. >> >> If it is possible to create one device per bank I usually prefer that >> approach, as it also (often) makes it possible to use the >> generic GPIO library, i.e. the hardware abstraction start to >> share more with other GPIO controllers. > > But that should be possible whether there are banks in DT or not, right? Possible yes, but more complex, requireing a bigger and more complex chunk of code to get it right. Which we don't have for Linux (I don't know about $OS). For 1 bank = 1 device, all callbacks etc naturally offsets to something like 0..31 landing in (1 << offset) which makes for a simple support library. If there is more complex calculations, more complex helper libs are required and maybe not even worth it, ending up with more duplicated or slightly-different code. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 14, 2018 at 4:17 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Jun 14, 2018 at 6:45 AM, Sergio Paracuellos > <sergio.paracuellos@gmail.com> wrote: > >> Ok, so... does the following single node sounds acceptable? >> >> gpio: gpio@600 { >> #gpio-cells = <2>; >> #interrupt-cells = <2>; >> compatible = "mediatek,mt7621-gpio"; >> gpio-controller; >> interrupt-controller; >> reg = <0x600 0x60>; >> interrupt-parent = <&gic>; >> interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>; >> mediatek,gpio-bank-widths = <32 32 32>; > > Why would you need this? It is pretty obvious from the > compatible string isn't it? Just use that to tell that > "aha, it is mediatek,mt7321-gpio, so it has 3x32 GPIOs > indexed from 0..95". > > No need of overspecifying stuff. Ok, So just one node with no extra stuff. gpio: gpio@600 { #gpio-cells = <2>; #interrupt-cells = <2>; compatible = "mediatek,mt7621-gpio"; gpio-controller; interrupt-controller; reg = <0x600 0x60>; interrupt-parent = <&gic>; interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>; Thanks for your answers! > > Yours, > Linus Walleij Best regards, Sergio Paracuellos -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt new file mode 100644 index 0000000..30d8a02 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt @@ -0,0 +1,68 @@ +Mediatek SoC GPIO controller bindings + +The IP core used inside these SoCs has 3 banks of 32 GPIOs each. +The registers of all the banks are interwoven inside one single IO range. +We load one GPIO controller instance per bank. To make this possible +we support 2 types of nodes. The parent node defines the memory I/O range and +has 3 children each describing a single bank. Also the GPIO controller can receive +interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU +using GIC INT12. + +Required properties for the top level node: +- compatible: + - "mediatek,mt7621-gpio" for Mediatek controllers +- reg : Physical base address and length of the controller's registers +- interrupt-parent : phandle of the parent interrupt controller. +- interrupts : Interrupt specifier for the controllers interrupt. +- interrupt-controller : Mark the device node as an interrupt controller. +- #interrupt-cells : Should be 2. The first cell defines the interrupt number. + The second cell bits[3:0] is used to specify trigger type as follows: + - 1 = low-to-high edge triggered. + - 2 = high-to-low edge triggered. + - 4 = active high level-sensitive. + - 8 = active low level-sensitive. + + +Required properties for the GPIO bank node: +- compatible: + - "mediatek,mt7621-gpio-bank" for Mediatek banks +- #gpio-cells : Should be two. The first cell is the GPIO pin number and the + second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. + Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported. +- gpio-controller : Marks the device node as a GPIO controller. +- reg : The id of the bank that the node describes. + +Example: + gpio@600 { + #address-cells = <1>; + #size-cells = <0>; + + compatible = "mediatek,mt7621-gpio"; + reg = <0x600 0x100>; + + interrupt-parent = <&gic>; + interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + #interrupt-cells = <2>; + + gpio0: bank@0 { + reg = <0>; + compatible = "mediatek,mt7621-gpio-bank"; + gpio-controller; + #gpio-cells = <2>; + }; + + gpio1: bank@1 { + reg = <1>; + compatible = "mediatek,mt7621-gpio-bank"; + gpio-controller; + #gpio-cells = <2>; + }; + + gpio2: bank@2 { + reg = <2>; + compatible = "mediatek,mt7621-gpio-bank"; + gpio-controller; + #gpio-cells = <2>; + }; + };