gpio: Add APM X-Gene standy platform GPIO driver
Subsystems (2)
cover gpio: Add APM X-Gene standy platform GPIO driver
This patch add the GPIO standby controller in the APM X-Gene platform.
Y Vo (3):
gpio: Add APM X-Gene standby GPIO controller driver
Documentation: gpio: Add APM X-Gene standby GPIO controller DTS
binding
arm64:dts: Add APM X-Gene standby GPIO controller DTS entries
.../devicetree/bindings/gpio/gpio-xgene-sb.txt | 31 +++ arch/arm64/boot/dts/apm-storm.dtsi | 13 ++ drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xgene-sb.c | 232 ++++++++++++++++++++ 5 files changed, 284 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt create mode 100755 drivers/gpio/gpio-xgene-sb.c
Signature · 9 lines
1.7.9.5 CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Add APM X-Gene standby GPIO controller driver.
--- drivers/gpio/Kconfig | 7 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xgene-sb.c | 232 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 240 insertions(+) create mode 100755 drivers/gpio/gpio-xgene-sb.c
drivers/gpio/Kconfig +7 -0
@@ -334,6 +334,13 @@ config GPIO_TZ1090_PDC help Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs. +config GPIO_XGENE_SB + tristate "APM X-Gene GPIO standby controller support" + depends on ARCH_XGENE && OF_GPIO + help + This driver supports the GPIO block within the APM X-Gene + Standby Domain. Say yes here to enable the GPIO functionality. + config GPIO_XILINX bool "Xilinx GPIO support" depends on PPC_OF || MICROBLAZE || ARCH_ZYNQ
drivers/gpio/Makefile +1 -0
@@ -101,6 +101,7 @@ obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o obj-$(CONFIG_GPIO_WM831X) += gpio-wm831x.o obj-$(CONFIG_GPIO_WM8350) += gpio-wm8350.o obj-$(CONFIG_GPIO_WM8994) += gpio-wm8994.o +obj-$(CONFIG_GPIO_XGENE_SB) += gpio-xgene-sb.o obj-$(CONFIG_GPIO_XILINX) += gpio-xilinx.o obj-$(CONFIG_GPIO_XTENSA) += gpio-xtensa.o obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o
drivers/gpio/gpio-xgene-sb.c new file +232 -0
232 lines not on this page · raw message
Signature · 9 lines
1.7.9.5 CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Review
Quoted 2 lines · On Wednesday 08 October 2014 21:52:26 Y Vo wrote:
(quoted diff) +2
-0+ +#define GICD_SPI_BASE 0x78010000
You can't hardcode register locations. Please use the proper interfaces to do whatever you want.
It's probably not ok to map any GIC registers into the GPIO driver, it should operate as a nested irqchip.
Quoted 5 lines
drivers/gpio/gpio-xgene-sb.c new file +5
-0+ +static int xgene_gpio_sb_get (struct gpio_chip *gc,u32 gpio) +{ +struct of_mm_gpio_chip *mm_gc =to_of_mm_gpio_chip (gc); +struct xgene_gpio_sb *chip =to_xgene_gpio_sb (mm_gc);
drivers/gpio/gpio-xgene-sb.c line 87
Quoted 10 lines
drivers/gpio/gpio-xgene-sb.c new file +10
-0+u32 data; + +if (chip->irq[gpio]) { + data =ioread32 (chip->gic_regs +GICD_SPIR1 ); + }else { + data =ioread32 (mm_gc->regs +MPA_GPIO_IN_ADDR ); + } + +return (data &GPIO_MASK (gpio)) ?1 :0 ; +}
This should not assume that a particular upstream irqchip is used, and more importantly it should not touch its registers.
Quoted 7 lines
drivers/gpio/gpio-xgene-sb.c new file +7
-0+static int apm_gpio_sb_to_irq (struct gpio_chip *gc,u32 gpio) +{ +struct of_mm_gpio_chip *mm_gc =to_of_mm_gpio_chip (gc); +struct xgene_gpio_sb *chip =to_xgene_gpio_sb (mm_gc); + +if (chip->irq[gpio]) +return chip->irq[gpio];
drivers/gpio/gpio-xgene-sb.c line 149
Quoted 10 lines
drivers/gpio/gpio-xgene-sb.c new file +10
-0+ +return -ENXIO ; +} + +static int gpio_sb_probe (struct platform_device *pdev) +{ +struct of_mm_gpio_chip *mm; +struct xgene_gpio_sb *apm_gc; +u32 ret, i; +u32 default_pins[] = {0x08 ,0x09 ,0x0A ,0x0B ,0x0C ,0x0D };
Why are these hardcoded? The "apm,xgene-gpio-sb" compatible string seems very generic, but the list of pins here seems very specific to a particular implementation.
drivers/gpio/gpio-xgene-sb.c line 177
Quoted 12 lines
drivers/gpio/gpio-xgene-sb.c new file +12
-0+ mm->gc.ngpio =XGENE_MAX_GPIO_DS ; + apm_gc->nirq =XGENE_MAX_GPIO_DS_IRQ ; + + apm_gc->gic_regs =ioremap (GICD_SPI_BASE ,16 ); +if (!apm_gc->gic_regs) +return -ENOMEM ; + + apm_gc->irq =devm_kzalloc (&pdev->dev,sizeof (u32) *XGENE_MAX_GPIO_DS , +GFP_KERNEL ); +if (!apm_gc->irq) +return -ENOMEM ; +memset (apm_gc->irq,0 ,sizeof (u32) *XGENE_MAX_GPIO_DS );
kzalloc implies memset.
Quoted 15 lines
drivers/gpio/gpio-xgene-sb.c new file +15
-0+ res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + mm->regs =devm_ioremap_resource (&pdev->dev, res); +if (!mm->regs) +return PTR_ERR (mm->regs); + +for (i =0 ; i < apm_gc->nirq; i++) { + apm_gc->irq[default_pins[i]] =platform_get_irq (pdev, i); +xgene_gpio_set_bit (mm->regs +MPA_GPIO_SEL_LO , + default_pins[i] *2 ,1 ); +xgene_gpio_set_bit (mm->regs +MPA_GPIO_INT_LVL , i,1 ); + } + mm->gc.of_node = pdev->dev.of_node; + ret =gpiochip_add (&mm->gc); +if (ret) +dev_err (&pdev->dev,"failed to register X-Gene GPIO Standby driver" );
drivers/gpio/gpio-xgene-sb.c line 203
Quoted 10 lines
drivers/gpio/gpio-xgene-sb.c new file +10
-0+else +dev_info (&pdev->dev,"X-Gene GPIO Standby driver registered\n" ); + +return ret; +} + +static int xgene_gpio_sb_probe (struct platform_device *pdev) +{ +return gpio_sb_probe (pdev); +}
This function is pointless, just call the real one instead.
ARnd
A message that is not in the archive (sent off-list or not yet received): CAL4ahLfpubEqi49gy8T9pyrf=1j__LbAsKUyr5i=koNyp0Mmig@mail.gmail.com
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Quoted 3 lines · On Thursday 09 October 2014 16:31:18 Y Vo wrote:
Dear Arnd,
Thanks a lot for your review. Pls see my answer on blue text below.
Please do not send html-encoded email, it will get dropped by all mailing lists.
Quoted 14 lines · Arnd Bergmann
Quoted 12 lines · On Wednesday 08 October 2014 21:52:26 Y Vo wrote:
Review
Quoted 2 lines
(quoted diff) +2
-0+ +#define GICD_SPI_BASE 0x78010000You can't hardcode register locations. Please use the proper interfaces to do whatever you want.
*APM: We will do that.*
Quoted 7 lines
Quoted 3 lines
It's probably not ok to map any GIC registers into the GPIO driver, it should operate as a nested irqchip.
*APM: We will find the solution, the problem is we want to read the status
of that GPIO in case it is configured IRQ. In this case we must access to GIC to read the true value.*
Can you explain what the hardware does here? Do you mean you have no way to read the GPIO level from the GPIO controller for any pin that is configured as an interrupt?
Can you route all GPIO pins to arbitrary upstream IRQ lines, or is this hardwired in the GPIO block?
Arnd
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Dear Arnd,
Pls see my answer below:
Quoted 30 lines · Arnd Bergmann
Quoted 3 lines · On Thursday 09 October 2014 16:31:18 Y Vo wrote:
Dear Arnd,
Thanks a lot for your review. Pls see my answer on blue text below.
Please do not send html-encoded email, it will get dropped by all mailing lists.
Quoted 14 lines · Arnd Bergmann
Quoted 12 lines · On Wednesday 08 October 2014 21:52:26 Y Vo wrote:
Review
Quoted 2 lines
(quoted diff) +2
-0+ +#define GICD_SPI_BASE 0x78010000You can't hardcode register locations. Please use the proper interfaces to do whatever you want.
*APM: We will do that.*Quoted 7 lines
Quoted 3 lines
It's probably not ok to map any GIC registers into the GPIO driver, it should operate as a nested irqchip.
*APM: We will find the solution, the problem is we want to read the status
of that GPIO in case it is configured IRQ. In this case we must access to GIC to read the true value.*
Quoted 6 lines
Can you explain what the hardware does here? Do you mean you have no way to read the GPIO level from the GPIO controller for any pin that is configured as an interrupt?
Can you route all GPIO pins to arbitrary upstream IRQ lines, or is this hardwired in the GPIO block?
APM: There are 6 GPIOs which can support IRQ, they are fixed to use external IRQ from XGIC. (The XGIC is based on the ARM Generic Interrupt Controller Architecture Specification, Architecture version 2.0, The XGIC provides the mechanism to collect interrupt requests (IRQs) from both on-chip as well as off-chip sources and deliver them to the multiple X-Gene1 cores within the X-Gene1 processor), So there are no way to read the GPIO DS when configure as an interrupt. They are specific GPIO for boot another boot chip in X-Gene which can access when boot up. So this is hardwire in the GPIO block. GPIO_DS8 ---> External IRQ0 (XGIC40)GPIO_DS9 ---> External IRQ1 (XGIC41)
...GPIO_DS13 ---> External IRQ5(XGIC45)
Quoted 2 lines
Arnd
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
is for the sole use of the intended recipient(s) and contains information
that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited.
If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Quoted 5 lines · On Friday 10 October 2014 10:22:34 Y Vo wrote:
APM: There are 6 GPIOs which can support IRQ, they are fixed to use external IRQ from XGIC. (The XGIC is based on the ARM Generic Interrupt Controller Architecture Specification, Architecture version 2.0, The XGIC provides the mechanism to collect interrupt requests (IRQs) from both on-chip as well as off-chip sources and deliver them
Quoted 8 lines
to the multiple X-Gene1 cores within the X-Gene1 processor), So there are no way to read the GPIO DS when configure as an interrupt. They are specific GPIO for boot another boot chip in X-Gene which can access when boot up. So this is hardwire in the GPIO block.
GPIO_DS8---> External IRQ0 (XGIC40)GPIO_DS9---> External IRQ1 (XGIC41)
...GPIO_DS13---> External IRQ5(XGIC45)
Could you read the status of the external IRQ line using irq_get_pending() or another public interface?
If not, we may have to add an interface for doing this.
Arnd
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
In reply to Y VoQuoted 3 lines · Y Vo
Add APM X-Gene standby GPIO controller driver.
Signed-off-by Y Vo <[email protected]>
That's a very terse commit message. Please tell us a bit about the hardware and what platforms it is used on, etc.
For example that is uses ACPI, as seems to be the case.
drivers/gpio/Kconfig config GPIO_TZ1090_PDC · line 339
Quoted 3 lines
drivers/gpio/Kconfig +3
-0+config GPIO_XGENE_SB + tristate "APM X-Gene GPIO standby controller support" + depends on ARCH_XGENE && OF_GPIO
If this platform uses ACPI (as is implied below), why is it depending on OF_GPIO but not GPIO_ACPI...
(...)
drivers/gpio/gpio-xgene-sb.c line 27
Quoted 3 lines
drivers/gpio/gpio-xgene-sb.c new file +3
-0+#include <linux/of_gpio.h> +#include <linux/of_irq.h> +#include <linux/acpi.h>
So this platform uses some interesting combination of ACPI and device tree at the same time? Or alternatively?
drivers/gpio/gpio-xgene-sb.c line 55
Quoted 7 lines
drivers/gpio/gpio-xgene-sb.c new file +7
-0+struct xgene_gpio_sb { +struct of_mm_gpio_chip mm; +u32 *irq; +u32 nirq; +void __iomem *gic_regs; +spinlock_t lock;/* mutual exclusion */ +};
Document this struct using kerneldoc instead.
Quoted 5 lines
drivers/gpio/gpio-xgene-sb.c new file +5
-0+ apm_gc->irq =devm_kzalloc (&pdev->dev,sizeof (u32) *XGENE_MAX_GPIO_DS , +GFP_KERNEL ); +if (!apm_gc->irq) +return -ENOMEM ; +memset (apm_gc->irq,0 ,sizeof (u32) *XGENE_MAX_GPIO_DS );
(...)
drivers/gpio/gpio-xgene-sb.c line 185
Quoted 2 lines
drivers/gpio/gpio-xgene-sb.c new file +2
-0+for (i =0 ; i < apm_gc->nirq; i++) { + apm_gc->irq[default_pins[i]] =platform_get_irq (pdev, i);
So the IRQs for all the GPIO pins are handled somewhere else instead of being a cascaded IRQ controller.
This means that the IRQ lines from each individual GPIO pin is connected to a unique IRQ line on a secondary interrupt controller, instead of the GPIO block being a cascaded interrupt controller in its own right.
Is this correct?
Usually there is a single IRQ line out from a GPIO block... not one per GPIO.
I really want to look at the code for that interrupt controller to see what's going on here, please provide me a pointer to the relevant code.
drivers/gpio/gpio-xgene-sb.c line 209
Quoted 5 lines
drivers/gpio/gpio-xgene-sb.c new file +5
-0+static int xgene_gpio_sb_remove (struct platform_device *pdev) +{ +struct of_mm_gpio_chip *mm =platform_get_drvdata (pdev); + +return gpiochip_remove (&mm->gc);
This function has changed signature and doesn't return a value anymore.
Yours,
Linus Walleij
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Quoted 10 lines · On Friday 24 October 2014 14:14:43 Linus Walleij wrote:
Quoted 3 lines · Y Vo
Add APM X-Gene standby GPIO controller driver.
Signed-off-by Y Vo <[email protected]>That's a very terse commit message. Please tell us a bit about the hardware and what platforms it is used on, etc.
For example that is uses ACPI, as seems to be the case.
I think that was just the header file inclusion, but no actual ACPI support. Al Stone is experimenting with ACPI support for X-Gene, and some earlier patches from APM had rudimentary support as well, but we are not adding ACPI support to X-Gene drivers upstream until we know whether we can support the entire platform in that mode or not.
Quoted 2 lines
So this platform uses some interesting combination of ACPI and device tree at the same time? Or alternatively?
Just DT at the moment.
Quoted 12 lines
Quoted 5 lines
drivers/gpio/gpio-xgene-sb.c new file +5
-0+ apm_gc->irq =devm_kzalloc (&pdev->dev,sizeof (u32) *XGENE_MAX_GPIO_DS , +GFP_KERNEL ); +if (!apm_gc->irq) +return -ENOMEM ; +memset (apm_gc->irq,0 ,sizeof (u32) *XGENE_MAX_GPIO_DS );(...)
drivers/gpio/gpio-xgene-sb.c line 185
Quoted 2 lines
drivers/gpio/gpio-xgene-sb.c new file +2
-0+for (i =0 ; i < apm_gc->nirq; i++) { + apm_gc->irq[default_pins[i]] =platform_get_irq (pdev, i);So the IRQs for all the GPIO pins are handled somewhere else instead of being a cascaded IRQ controller.
Quoted 6 lines
This means that the IRQ lines from each individual GPIO pin is connected to a unique IRQ line on a secondary interrupt controller, instead of the GPIO block being a cascaded interrupt controller in its own right.
Is this correct?
See the discussion I had on this. Yes, each line is connected to a GIC SPI interrupt by itself. I've discussed this with Marc Zyngier and Thomas Gleixner at the conference last week, and we concluded that we will need a new generic interface to get data out of the parent interrupt controller in a proper way. The current implementation just maps the GIC registers and reads them directly, which of course is not a proper way to do it.
Arnd
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Quoted 22 lines · Arnd Bergmann
Quoted 14 lines · On Friday 24 October 2014 14:14:43 Linus Walleij wrote:
Quoted 5 lines · Y Vo
drivers/gpio/gpio-xgene-sb.c new file +5
-0+ apm_gc->irq =devm_kzalloc (&pdev->dev,sizeof (u32) *XGENE_MAX_GPIO_DS , +GFP_KERNEL ); +if (!apm_gc->irq) +return -ENOMEM ; +memset (apm_gc->irq,0 ,sizeof (u32) *XGENE_MAX_GPIO_DS );(...)
drivers/gpio/gpio-xgene-sb.c line 185
Quoted 2 lines
drivers/gpio/gpio-xgene-sb.c new file +2
-0+for (i =0 ; i < apm_gc->nirq; i++) { + apm_gc->irq[default_pins[i]] =platform_get_irq (pdev, i);So the IRQs for all the GPIO pins are handled somewhere else instead of being a cascaded IRQ controller.
Quoted 6 lines
This means that the IRQ lines from each individual GPIO pin is connected to a unique IRQ line on a secondary interrupt controller, instead of the GPIO block being a cascaded interrupt controller in its own right.
Is this correct?
Quoted 7 lines
See the discussion I had on this. Yes, each line is connected to a GIC SPI interrupt by itself. I've discussed this with Marc Zyngier and Thomas Gleixner at the conference last week, and we concluded that we will need a new generic interface to get data out of the parent interrupt controller in a proper way. The current implementation just maps the GIC registers and reads them directly, which of course is not a proper way to do it.
Hmmmmmm. OK shall we hold this driver until the infrastructure issues are resolved?
The following is a recurring pattern among GPIO controllers:
the GPIO controller can go offline (asycnhcronous) and while it is offline a secondary logic triggers an IRQ that wakes the system up, however the GPIO logic cannot really "see" that IRQ since it was sleeping when it arrived.
Thus a latent IRQ is pending in the wakeup logic. This concept exists in drivers/pinctrl/nomadik/pinctrl-nomadik.c and I strongly prefer to call these "latent irqs" as it's a clear unambigous terminology.
So is this a case of latent IRQs pending in the GIC?
Yours,
Linus Walleij
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Quoted 10 lines · On Wednesday 29 October 2014 10:52:47 Linus Walleij wrote:
Quoted 8 lines · Arnd Bergmann
On Friday 24 October 2014 14:14:43 Linus Walleij wrote:
See the discussion I had on this. Yes, each line is connected to a GIC SPI interrupt by itself. I've discussed this with Marc Zyngier and Thomas Gleixner at the conference last week, and we concluded that we will need a new generic interface to get data out of the parent interrupt controller in a proper way. The current implementation just maps the GIC registers and reads them directly, which of course is not a proper way to do it.
Quoted 2 lines
Hmmmmmm. OK shall we hold this driver until the infrastructure issues are resolved?
Y could send a first version that does not support the IRQ lines if he wants to speed up the process.
Quoted 6 lines
The following is a recurring pattern among GPIO controllers:
the GPIO controller can go offline (asycnhcronous) and while it is offline a secondary logic triggers an IRQ that wakes the system up, however the GPIO logic cannot really "see" that IRQ since it was sleeping when it arrived.
Quoted 6 lines
Thus a latent IRQ is pending in the wakeup logic. This concept exists in
drivers/pinctrl/nomadik/pinctrl-nomadik.cand I strongly prefer to call these "latent irqs" as it's a clear unambigous terminology.So is this a case of latent IRQs pending in the GIC?
I think this case is different, from what I understand, the GPIO controller cannot implement gpio_chip->get() for any line that is connected to the GIC, and it has to ask the GIC instead. This seems independent of the online/offline state of the controller.
Arnd
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Hi Arnd,
Per Linus, shall we hold this driver until the GIC submission complete ? Or we will send the version without access GIC to read status in case the GPIO is configured IRQ ?
+static int xgene_gpio_sb_get(struct gpio_chip *gc, u32 gpio) +{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); + struct xgene_gpio_sb *chip = to_xgene_gpio_sb(mm_gc); + u32 data;
+
+ data = ioread32(mm_gc->regs + MPA_GPIO_IN_ADDR);
+
+ return (data & GPIO_MASK(gpio)) ? 1 : 0;
+}
Regards,
Y
Quoted 36 lines · Arnd Bergmann
Quoted 10 lines · On Wednesday 29 October 2014 10:52:47 Linus Walleij wrote:
Quoted 8 lines · Arnd Bergmann
On Friday 24 October 2014 14:14:43 Linus Walleij wrote:
See the discussion I had on this. Yes, each line is connected to a GIC SPI interrupt by itself. I've discussed this with Marc Zyngier and Thomas Gleixner at the conference last week, and we concluded that we will need a new generic interface to get data out of the parent interrupt controller in a proper way. The current implementation just maps the GIC registers and reads them directly, which of course is not a proper way to do it.Quoted 2 lines
Hmmmmmm. OK shall we hold this driver until the infrastructure issues are resolved?
Y could send a first version that does not support the IRQ lines if he wants to speed up the process.
Quoted 6 lines
The following is a recurring pattern among GPIO controllers:
the GPIO controller can go offline (asycnhcronous) and while it is offline a secondary logic triggers an IRQ that wakes the system up, however the GPIO logic cannot really "see" that IRQ since it was sleeping when it arrived.Quoted 6 lines
Thus a latent IRQ is pending in the wakeup logic. This concept exists in
drivers/pinctrl/nomadik/pinctrl-nomadik.cand I strongly prefer to call these "latent irqs" as it's a clear unambigous terminology.So is this a case of latent IRQs pending in the GIC?
I think this case is different, from what I understand, the GPIO controller cannot implement
gpio_chip->get()for any line that is connected to the GIC, and it has to ask the GIC instead. This seems independent of the online/offline state of the controller.Arnd
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Quoted 5 lines · On Wednesday 29 October 2014 22:09:00 Y Vo wrote:
Hi Arnd,
Per Linus, shall we hold this driver until the GIC submission complete ? Or we will send the version without access GIC to read status in case the GPIO is configured IRQ ?
I'm fine with it either way.
drivers/gpio/gpio-xgene-sb.c line 87
Quoted 10 lines
drivers/gpio/gpio-xgene-sb.c new file +10
-0+static int xgene_gpio_sb_get (struct gpio_chip *gc,u32 gpio) +{ +struct of_mm_gpio_chip *mm_gc =to_of_mm_gpio_chip (gc); +struct xgene_gpio_sb *chip =to_xgene_gpio_sb (mm_gc); +u32 data; + + data =ioread32 (mm_gc->regs +MPA_GPIO_IN_ADDR ); + +return (data &GPIO_MASK (gpio)) ?1 :0 ; +}
This would be actually broken, right? Maybe it's better to change the driver so that it refuses to map GPIO lines that are hardwired to the interrupt controller, until we can support that.
Arnd
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Quoted 9 lines · Arnd Bergmann
Quoted 5 lines · On Wednesday 29 October 2014 22:09:00 Y Vo wrote:
Hi Arnd,
Per Linus, shall we hold this driver until the GIC submission complete ? Or we will send the version without access GIC to read status in case the GPIO is configured IRQ ?
I'm fine with it either way.
Quoted 14 lines
Review
Quoted 10 lines
(quoted diff) +10
-0+static int xgene_gpio_sb_get(struct gpio_chip *gc, u32 gpio) +{ + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); + struct xgene_gpio_sb *chip = to_xgene_gpio_sb(mm_gc); + u32 data; + + data = ioread32(mm_gc->regs + MPA_GPIO_IN_ADDR); + + return (data & GPIO_MASK(gpio)) ? 1 : 0; +}This would be actually broken, right? Maybe it's better to change the driver so that it refuses to map GPIO lines that are hardwired to the interrupt controller, until we can support that.
It still works if they are configure as GPIO.
Do you have any reference to resolve it ?
PATCH 1/3 gpio: Add APM X-Gene standby GPIO controller driver
Quoted 15 lines · On Tuesday 16 December 2014 16:43:02 Y Vo wrote:
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6
-0+static int xgene_gpio_sb_get(struct gpio_chip *gc, u32 gpio) +{ + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); + struct xgene_gpio_sb *chip = to_xgene_gpio_sb(mm_gc); + u32 data; +Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ data = ioread32(mm_gc->regs + MPA_GPIO_IN_ADDR); + + return (data & GPIO_MASK(gpio)) ? 1 : 0; +}This would be actually broken, right? Maybe it's better to change the driver so that it refuses to map GPIO lines that are hardwired to the interrupt controller, until we can support that.
Quoted 2 lines
It still works if they are configure as GPIO.
Do you have any reference to resolve it ?
Ok, I think I just misunderstood how this works, I assumed that you could not reconfigure the lines. Seems ok then.
Arnd
PATCH 2/3 Documentation: gpio: Add APM X-Gene standby GPIO controller DTS binding
In reply to Y VoDocumentation for APM X-Gene standby GPIO controller DTS binding.
--- .../devicetree/bindings/gpio/gpio-xgene-sb.txt | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt new file +31 -0
@@ -0,0 +1,31 @@ +APM X-Gene Standby GPIO controller bindings + +This is a gpio controller in standby domain. + +There are 20 GPIO pins from 0..21. There is no GPIO_DS14 and GPIO_DS15. +Only GPIO_DS8..GPIO_DS13 support interrupt. IRQ mapping 0x28..0x2d. + +Required properties: +- compatible: "apm,xgene-gpio-sb" for X-Gene Standby GPIO controller +- reg: Physical base address and size of the controller's registers +- #gpio-cells: Should be two. + - first cell is the pin number + - second cell is used to specify the gpio polarity: + 0 = active high + 1 = active low +- gpio-controller: Marks the device node as a GPIO controller. +- interrupts: Shall contains the interrupts. + +Example: + sbgpio: sbgpio at 17001000 { + compatible = "apm,xgene-gpio-sb"; + reg = <0x0 0x17001000 0x0 0x400>; + #gpio-cells = <2>; + gpio-controller; + interrupts = <0x0 0x28 0x1>, + <0x0 0x29 0x1>, + <0x0 0x2a 0x1>, + <0x0 0x2b 0x1>, + <0x0 0x2c 0x1>, + <0x0 0x2d 0x1>; + };
Signature · 9 lines
1.7.9.5 CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
PATCH 3/3 arm64:dts: Add APM X-Gene standby GPIO controller DTS entries
In reply to Y VoAdd standby domain gpio controller for APM X-Gene SoC platform.
--- arch/arm64/boot/dts/apm-storm.dtsi | 13 +++++++++++++ 1 file changed, 13 insertions(+)
arch/arm64/boot/dts/apm-storm.dtsi +13 -0
@@ -389,6 +389,19 @@ phy-names = "sata-phy"; }; + sbgpio: sbgpio at 17001000{ + compatible = "apm,xgene-gpio-sb"; + reg = <0x0 0x17001000 0x0 0x400>; + #gpio-cells = <2>; + gpio-controller; + interrupts = <0x0 0x28 0x1>, + <0x0 0x29 0x1>, + <0x0 0x2a 0x1>, + <0x0 0x2b 0x1>, + <0x0 0x2c 0x1>, + <0x0 0x2d 0x1>; + }; + rtc: rtc at 10510000 { compatible = "apm,xgene-rtc"; reg = <0x0 0x10510000 0x0 0x400>;
Signature · 9 lines
1.7.9.5 CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
cover gpio: Add APM X-Gene standy platform GPIO driver
In reply to Y VoQuoted 7 lines
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
is for the sole use of the intended recipient(s) and contains information
that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited.
If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
You will need to resend this series without this disclaimer on each and every patch.
Mark.
cover gpio: Add APM X-Gene standy platform GPIO driver
Quoted 10 lines · Mark Rutland
Quoted 7 lines
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
is for the sole use of the intended recipient(s) and contains information
that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited.
If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.You will need to resend this series without this disclaimer on each and every patch.
Good point, haha, I wonder if the GPIO maintainer is "intended audience"?
So many insecurities in this world, no matter what you do, some legal person think you're doing the wrong thing...
Yours,
Linus Walleij