From: pshete <redacted>
These patches adds multiple interrupt support.
Each main GPIO is associated with 8 interrupts
per controller in case of NON-AON GPIO's and
4 interrupts per controller in AON GPIO.
This is new feature starting T194
The interrupt route map determines which interrupt line is to be used.
pshete (2):
gpio: tegra: add multiple interrupt support
arm64: tegra: GPIO Interrupt entries
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 49 +++++++++++++++++++++++-
drivers/gpio/gpio-tegra186.c | 25 ++++++++++--
2 files changed, 68 insertions(+), 6 deletions(-)
--
2.17.1
From: pshete <redacted>
T19x GPIO controller's support multiple interrupts. The GPIO
controller is capable to route 8 interrupts per controller in
case of NON-AON GPIO's and 4 interrupts per controller in AON GPIO.
This is new feature starting T194
The interrupt route map determines which interrupt line is to be used.
Signed-off-by: Prathamesh Shete <redacted>
---
drivers/gpio/gpio-tegra186.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
@@ -462,9 +464,20 @@ static void tegra186_gpio_irq(struct irq_desc *desc)base=gpio->base+port->bank*0x1000+port->port*0x200;-/* skip ports that are not associated with this bank */-if(parent!=gpio->irq[port->bank])-gotoskip;+if(!gpio->soc->multi_ints){+/* skip ports that are not associated with this bank */+if(parent!=gpio->irq[port->bank])+gotoskip;++}else{+flag=0;+for(j=0;j<8;j++){+if(parent!=gpio->irq[(port->bank*8)+j])+flag++;+}+if(!(flag&0xF))+gotoskip;+}value=readl(base+TEGRA186_GPIO_INTERRUPT_STATUS(1));
On Fri, Sep 03, 2021 at 03:45:11PM +0530, Prathamesh Shete wrote:
quoted hunk
From: pshete <redacted>
T19x GPIO controller's support multiple interrupts. The GPIO
controller is capable to route 8 interrupts per controller in
case of NON-AON GPIO's and 4 interrupts per controller in AON GPIO.
This is new feature starting T194
The interrupt route map determines which interrupt line is to be used.
Signed-off-by: Prathamesh Shete <redacted>
---
drivers/gpio/gpio-tegra186.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
Do we really have to add this? Can we not simply derive it from the
number of interrupts actually read from device tree? Doing so would also
make it easier to keep the code backwards-compatible. Remember that this
code must not fail if fed with an old device tree where not 8 interrupts
have been specified per controller.
quoted hunk
const struct tegra186_pin_range *pin_ranges;
unsigned int num_pin_ranges;
@@ -451,6 +452,7 @@ static void tegra186_gpio_irq(struct irq_desc *desc) struct irq_chip *chip = irq_desc_get_chip(desc); unsigned int parent = irq_desc_get_irq(desc); unsigned int i, offset = 0;+ int j, flag;
j can be unsigned in, so you can put it after i in the line above. Also,
maybe name the flag variable to something more specific to make it clear
what it's used for.
quoted hunk
chained_irq_enter(chip, desc);
@@ -462,9 +464,20 @@ static void tegra186_gpio_irq(struct irq_desc *desc) base = gpio->base + port->bank * 0x1000 + port->port * 0x200;- /* skip ports that are not associated with this bank */- if (parent != gpio->irq[port->bank])- goto skip;+ if (!gpio->soc->multi_ints) {+ /* skip ports that are not associated with this bank */+ if (parent != gpio->irq[port->bank])+ goto skip;++ } else {+ flag = 0;+ for (j = 0; j < 8; j++) {+ if (parent != gpio->irq[(port->bank * 8) + j])+ flag++;+ }+ if (!(flag & 0xF))+ goto skip;+ } value = readl(base + TEGRA186_GPIO_INTERRUPT_STATUS(1));
Going over this patch reminded me that I had written a similar patch a
while ago, which does things a bit differently. I've attached both
patches below. Please take a look. It's slightly bigger that your
version above, but it addresses the backwards-compatibility issue. It
also has a couple of comments that describe why the interrupt routing is
done the way it is.
For completeness I should say that I'm not sure if I've ever tested the
second patch because I had it marked "WIP", which I usually do if there
is work I know remains to be done and since there's no TODO comments or
anything in the code, I assume that I never tested it completely.
Looking at the history of the branch where I have that patch, I don't
see changes to the device tree files, so I probably never got around to
adding the multiple interrupts per bank and hence couldn't test it
properly. I can do that based on your second patch, but it'd be great if
you could go over the attached patches and let me know what you think.
Thierry
Answers to review comments inlined.
-----Original Message-----
From: Thierry Reding <redacted>
Sent: Monday, September 6, 2021 10:21 AM
To: Prathamesh Shete <redacted>
Cc: linus.walleij@linaro.org; bgolaszewski@baylibre.com; Jonathan Hunter <jonathanh@nvidia.com>; linux-gpio@vger.kernel.org; linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org; Suresh Mangipudi <redacted>
Subject: Re: [PATCH v2 1/2] gpio: tegra: add multiple interrupt support
On Fri, Sep 03, 2021 at 03:45:11PM +0530, Prathamesh Shete wrote:
quoted hunk
From: pshete <redacted>
T19x GPIO controller's support multiple interrupts. The GPIO
controller is capable to route 8 interrupts per controller in case of
NON-AON GPIO's and 4 interrupts per controller in AON GPIO.
This is new feature starting T194
The interrupt route map determines which interrupt line is to be used.
Signed-off-by: Prathamesh Shete <redacted>
---
drivers/gpio/gpio-tegra186.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-tegra186.c
b/drivers/gpio/gpio-tegra186.c index d38980b9923a..36bd8de6d401 100644
Do we really have to add this? Can we not simply derive it from the number of interrupts actually read from device tree? Doing so would also make it easier to keep the code backwards-compatible. Remember that this code must not fail if fed with an old device tree where not 8 interrupts have been specified per controller.
[PS]: True, we can derive from DT but adding variable in soc data which is static is more easier and effective to maintain as well. Yes I have verified the change on older DT blobs as well and it is working as expected.
quoted hunk
const struct tegra186_pin_range *pin_ranges;
unsigned int num_pin_ranges;
@@ -451,6 +452,7 @@ static void tegra186_gpio_irq(struct irq_desc *desc) struct irq_chip *chip = irq_desc_get_chip(desc); unsigned int parent = irq_desc_get_irq(desc); unsigned int i, offset = 0;+ int j, flag;
j can be unsigned in, so you can put it after i in the line above. Also, maybe name the flag variable to something more specific to make it clear what it's used for.
[PS]: Addressed this in version v3.
*desc)
base = gpio->base + port->bank * 0x1000 + port->port * 0x200;
- /* skip ports that are not associated with this bank */
- if (parent != gpio->irq[port->bank])
- goto skip;
+ if (!gpio->soc->multi_ints) {
+ /* skip ports that are not associated with this bank */
+ if (parent != gpio->irq[port->bank])
+ goto skip;
+
+ } else {
+ flag = 0;
+ for (j = 0; j < 8; j++) {
+ if (parent != gpio->irq[(port->bank * 8) + j])
+ flag++;
+ }
+ if (!(flag & 0xF))
+ goto skip;
+ }
value = readl(base + TEGRA186_GPIO_INTERRUPT_STATUS(1));
Going over this patch reminded me that I had written a similar patch a while ago, which does things a bit differently. I've attached both patches below. Please take a look. It's slightly bigger that your version above, but it addresses the backwards-compatibility issue. It also has a couple of comments that describe why the interrupt routing is done the way it is.
For completeness I should say that I'm not sure if I've ever tested the second patch because I had it marked "WIP", which I usually do if there is work I know remains to be done and since there's no TODO comments or anything in the code, I assume that I never tested it completely.
Looking at the history of the branch where I have that patch, I don't see changes to the device tree files, so I probably never got around to adding the multiple interrupts per bank and hence couldn't test it properly. I can do that based on your second patch, but it'd be great if you could go over the attached patches and let me know what you think.
[PS]: I think this change is much shorter and simpler as it does not add much code and hence reduce complexity. The change you are suggesting is lengthier and also I have not looked into your patch in more detailed level.
Thierry
From: pshete <redacted>
These patches adds multiple interrupt support.
Each main GPIO is associated with 8 interrupts
per controller in case of NON-AON GPIO's and
4 interrupts per controller in AON GPIO.
This is new feature starting T194
The interrupt route map determines which interrupt line is to be used.
pshete (2):
gpio: tegra: add multiple interrupt support
arm64: tegra: GPIO Interrupt entries
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 49 +++++++++++++++++++++++-
drivers/gpio/gpio-tegra186.c | 25 ++++++++++--
2 files changed, 68 insertions(+), 6 deletions(-)
--
2.17.1
From: pshete <redacted>
T19x GPIO controller's support multiple interrupts. The GPIO
controller is capable to route 8 interrupts per controller in
case of NON-AON GPIO's and 4 interrupts per controller in AON GPIO.
This is new feature starting T194
The interrupt route map determines which interrupt line is to be used.
Signed-off-by: Prathamesh Shete <redacted>
---
drivers/gpio/gpio-tegra186.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
@@ -462,9 +464,20 @@ static void tegra186_gpio_irq(struct irq_desc *desc)base=gpio->base+port->bank*0x1000+port->port*0x200;-/* skip ports that are not associated with this bank */-if(parent!=gpio->irq[port->bank])-gotoskip;+if(!gpio->soc->multi_ints){+/* skip ports that are not associated with this bank */+if(parent!=gpio->irq[port->bank])+gotoskip;++}else{+flag=0;+for(j=0;j<8;j++){+if(parent!=gpio->irq[(port->bank*8)+j])+flag++;+}+if(!(flag&0xF))+gotoskip;+}value=readl(base+TEGRA186_GPIO_INTERRUPT_STATUS(1));
From: pshete <redacted>
These patches adds multiple interrupt support.
Each main GPIO is associated with 8 interrupts per controller in case of NON-AON GPIO's and
4 interrupts per controller in AON GPIO.
This is new feature starting Tegra194
The interrupt route map determines which interrupt line is to be used.
pshete (2):
gpio: tegra: add multiple interrupt support
arm64: tegra: GPIO Interrupt entries
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 49 +++++++++++++++++++++++-
drivers/gpio/gpio-tegra186.c | 27 ++++++++++---
2 files changed, 69 insertions(+), 7 deletions(-)
--
2.17.1
From: pshete <redacted>
T19x GPIO controller's support multiple interrupts. The GPIO
controller is capable to route 8 interrupts per controller in
case of NON-AON GPIO's and 4 interrupts per controller in AON GPIO.
This is new feature starting Tegra194
The interrupt route map determines which interrupt line is to be used.
Signed-off-by: Prathamesh Shete <redacted>
---
drivers/gpio/gpio-tegra186.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
@@ -462,9 +464,20 @@ static void tegra186_gpio_irq(struct irq_desc *desc)base=gpio->base+port->bank*0x1000+port->port*0x200;-/* skip ports that are not associated with this bank */-if(parent!=gpio->irq[port->bank])-gotoskip;+if(!gpio->soc->multi_ints){+/* skip ports that are not associated with this bank */+if(parent!=gpio->irq[port->bank])+gotoskip;++}else{+intr_cntr=0;+for(j=0;j<8;j++){+if(parent!=gpio->irq[(port->bank*8)+j])+intr_cntr++;+}+if(!(intr_cntr&0xF))+gotoskip;+}value=readl(base+TEGRA186_GPIO_INTERRUPT_STATUS(1));
On Tue, Sep 07, 2021 at 01:02:23PM +0530, Prathamesh Shete wrote:
quoted hunk
From: pshete <redacted>
T19x GPIO controller's support multiple interrupts. The GPIO
controller is capable to route 8 interrupts per controller in
case of NON-AON GPIO's and 4 interrupts per controller in AON GPIO.
This is new feature starting Tegra194
The interrupt route map determines which interrupt line is to be used.
Signed-off-by: Prathamesh Shete <redacted>
---
drivers/gpio/gpio-tegra186.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
@@ -462,9 +464,20 @@ static void tegra186_gpio_irq(struct irq_desc *desc)base=gpio->base+port->bank*0x1000+port->port*0x200;-/* skip ports that are not associated with this bank */-if(parent!=gpio->irq[port->bank])-gotoskip;+if(!gpio->soc->multi_ints){+/* skip ports that are not associated with this bank */+if(parent!=gpio->irq[port->bank])+gotoskip;++}else{+intr_cntr=0;+for(j=0;j<8;j++){+if(parent!=gpio->irq[(port->bank*8)+j])
Again, I don't see how this would work. Currently the DT for Tegra194
(where you set multi_ints = true) lists 6 interrupts. So as soon as j
goes beyond 5, this will end up accessing data beyond the bounds of
the gpio->irq array.
I've revised the patches that I created to support this a while ago and
which I had sent earlier as a counter-proposal that keeps compatibility
with earlier device trees. I've now tested it and found a few issues I
had not run into earlier, but it should now work correctly with older
and updated device trees.
Thierry
Prathamesh: what are the changes between the three versions of this
patch I have in my inbox? Please always include a brief list of
updates when resending.
Thierry: does this make sense to you?
Bart
Prathamesh: what are the changes between the three versions of this
patch I have in my inbox? Please always include a brief list of
updates when resending.
Thierry: does this make sense to you?
Hi Bartosz,
the following patches from me that you applied earlier:
[PATCH 1/2] gpio: tegra186: Force one interrupt per bank
[PATCH 2/2] gpio: tegra186: Support multiple interrupts per bank
are replacements for patch 1 in this series, so that should no longer be
needed. Patch 2 of this series (the DT change) I plan to pick up into
the Tegra tree for v5.16.
Thierry