[PATCH v1 0/3] gpio: Add APM X-Gene standy platform GPIO driver

[PATCH v1 0/3] gpio: Add APM X-Gene standy platform GPIO driver

[Y Vo]

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

-- 
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 v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Y Vo]

Add APM X-Gene standby GPIO controller driver.

Signed-off-by: Y Vo <yvo-qTEPVZfXA3Y@public.gmane.org>
---
 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

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..7969c2e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -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
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d024e3..c9eae63 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -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
diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
new file mode 100755
index 0000000..858e383
--- /dev/null
+++ b/drivers/gpio/gpio-xgene-sb.c
@@ -0,0 +1,232 @@
+/*
+ * AppliedMicro X-Gene SoC GPIO-Standby Driver
+ *
+ * Copyright (c) 2014, Applied Micro Circuits Corporation
+ * Author: Tin Huynh <tnhuynh-qTEPVZfXA3Y@public.gmane.org>.
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
+#include <linux/of_irq.h>
+#include <linux/acpi.h>
+#include <linux/efi.h>
+#include <linux/string.h>
+#include <linux/of_address.h>
+
+#define XGENE_MAX_GPIO_DS		22
+#define XGENE_MAX_GPIO_DS_IRQ		6
+
+#define GPIO_MASK(x)			(1U << ((x) % 32))
+#define GPIO_DIR_IN			0
+#define GPIO_DIR_OUT			1
+
+#define MPA_GPIO_INT_LVL		0x0290
+#define MPA_GPIO_OE_ADDR		0x029c
+#define MPA_GPIO_OUT_ADDR		0x02a0
+#define MPA_GPIO_IN_ADDR		0x02a4
+#define MPA_GPIO_SEL_LO			0x0294
+#define MPA_GPIO_SEL_HIGH		0x029c
+
+#define GICD_SPI_BASE			0x78010000
+#define GICD_SPIR1			0x00000d08
+
+struct xgene_gpio_sb {
+	struct of_mm_gpio_chip mm;
+	u32 *irq;
+	u32 nirq;
+	void __iomem *gic_regs;
+	spinlock_t lock; /* mutual exclusion */
+};
+
+static inline struct xgene_gpio_sb *to_xgene_gpio_sb(struct of_mm_gpio_chip *mm)
+{
+	return container_of(mm, struct xgene_gpio_sb, mm);
+}
+
+static void xgene_gpio_set_bit(void __iomem *reg, u32 gpio, int val)
+{
+	u32 data;
+
+	data = ioread32(reg);
+	if (val)
+		data |= GPIO_MASK(gpio);
+	else
+		data &= ~GPIO_MASK(gpio);
+	iowrite32(data, reg);
+}
+
+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;
+
+	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;
+}
+
+static void xgene_gpio_sb_set(struct gpio_chip *gc, u32 gpio, int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct xgene_gpio_sb *bank = to_xgene_gpio_sb(mm_gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	xgene_gpio_set_bit(mm_gc->regs + MPA_GPIO_OUT_ADDR, gpio, val);
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+}
+
+static int xgene_gpio_sb_dir_out(struct gpio_chip *gc, u32 gpio,
+				 int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct xgene_gpio_sb *bank = to_xgene_gpio_sb(mm_gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	xgene_gpio_set_bit(mm_gc->regs + MPA_GPIO_OE_ADDR, gpio, GPIO_DIR_OUT);
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	return 0;
+}
+
+static int xgene_gpio_sb_dir_in(struct gpio_chip *gc, u32 gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct xgene_gpio_sb *bank = to_xgene_gpio_sb(mm_gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	xgene_gpio_set_bit(mm_gc->regs + MPA_GPIO_OE_ADDR, gpio, GPIO_DIR_IN);
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	return 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];
+
+	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};
+	struct resource *res;
+
+	apm_gc = devm_kzalloc(&pdev->dev, sizeof(*apm_gc), GFP_KERNEL);
+	if (!apm_gc)
+		return -ENOMEM;
+
+	mm = &apm_gc->mm;
+	mm->gc.direction_input = xgene_gpio_sb_dir_in;
+	mm->gc.direction_output = xgene_gpio_sb_dir_out;
+	mm->gc.get = xgene_gpio_sb_get;
+	mm->gc.set = xgene_gpio_sb_set;
+	mm->gc.to_irq = apm_gpio_sb_to_irq;
+	mm->gc.base = -1;
+	mm->gc.label = dev_name(&pdev->dev);
+	platform_set_drvdata(pdev, mm);
+
+	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);
+
+	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");
+	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);
+}
+
+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);
+}
+
+static const struct of_device_id xgene_gpio_sb_of_match[] = {
+	{.compatible = "apm,xgene-gpio-sb", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
+
+static struct platform_driver xgene_gpio_sb_driver = {
+	.driver = {
+		   .name = "xgene-gpio-sb",
+		   .of_match_table = xgene_gpio_sb_of_match,
+		   },
+	.probe = xgene_gpio_sb_probe,
+	.remove = xgene_gpio_sb_remove,
+};
+
+module_platform_driver(xgene_gpio_sb_driver);
+
+MODULE_AUTHOR("AppliedMicro");
+MODULE_DESCRIPTION("APM X-Gene GPIO Standby driver");
+MODULE_LICENSE("GPL");
-- 
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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/3] Documentation: gpio: Add APM X-Gene standby GPIO controller DTS binding

[Y Vo]

Documentation for APM X-Gene standby GPIO controller DTS binding.

Signed-off-by: Y Vo <yvo@apm.com>
---
 .../devicetree/bindings/gpio/gpio-xgene-sb.txt     |   31 ++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
new file mode 100644
index 0000000..3215e4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
@@ -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@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>;
+	};
-- 
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 v1 3/3] arm64:dts: Add APM X-Gene standby GPIO controller DTS entries

[Y Vo]

Add standby domain gpio controller for APM X-Gene SoC platform.

Signed-off-by: Y Vo <yvo-qTEPVZfXA3Y@public.gmane.org>
---
 arch/arm64/boot/dts/apm-storm.dtsi |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index c0aceef..1d1da4c 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -389,6 +389,19 @@
 			phy-names = "sata-phy";
 		};
 
+		sbgpio: sbgpio@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@10510000 {
 			compatible = "apm,xgene-rtc";
 			reg = <0x0 0x10510000 0x0 0x400>;
-- 
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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Arnd Bergmann]

On Wednesday 08 October 2014 21:52:26 Y Vo wrote:
> +
> +#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.

> +
> +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;
> +
> +	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.

> +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];
> +
> +	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.

> +	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.

> +	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");
> +	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

Re: [PATCH v1 0/3] gpio: Add APM X-Gene standy platform GPIO driver

[Mark Rutland]

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

Re: [PATCH v1 0/3] gpio: Add APM X-Gene standy platform GPIO driver

[Linus Walleij]

On Thu, Oct 9, 2014 at 11:42 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> 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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Arnd Bergmann]

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.

> 
> On Wed, Oct 8, 2014 at 10:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Wednesday 08 October 2014 21:52:26 Y Vo wrote:
> > > +
> > > +#define GICD_SPI_BASE                        0x78010000
> >
> > You can't hardcode register locations. Please use the proper interfaces
> > to do whatever you want.
> > *APM: We will do that.*
> 
> 
> 
> >
> > 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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Y Vo]

Dear Arnd,

Pls see my answer below:

On Thu, Oct 9, 2014 at 7:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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.
>
>>
>> On Wed, Oct 8, 2014 at 10:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> > On Wednesday 08 October 2014 21:52:26 Y Vo wrote:
>> > > +
>> > > +#define GICD_SPI_BASE                        0x78010000
>> >
>> > You can't hardcode register locations. Please use the proper interfaces
>> > to do whatever you want.
>> > *APM: We will do that.*
>>
>>
>>
>> >
>> > 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?

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)

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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Arnd Bergmann]

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
> 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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Linus Walleij]

On Wed, Oct 8, 2014 at 4:52 PM, Y Vo <yvo@apm.com> wrote:

> Add APM X-Gene standby GPIO controller driver.
>
> Signed-off-by: Y Vo <yvo@apm.com>

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.

> +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...

(...)
> +#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?


> +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.

> +       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);
(...)
> +       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.

> +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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Arnd Bergmann]

On Friday 24 October 2014 14:14:43 Linus Walleij wrote:
> On Wed, Oct 8, 2014 at 4:52 PM, Y Vo <yvo@apm.com> wrote:
> 
> > Add APM X-Gene standby GPIO controller driver.
> >
> > Signed-off-by: Y Vo <yvo@apm.com>
> 
> 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.

> So this platform uses some interesting combination of ACPI and device tree
> at the same time? Or alternatively?

Just DT at the moment.

> > +       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);
> (...)
> > +       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?

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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Linus Walleij]

On Fri, Oct 24, 2014 at 3:46 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 24 October 2014 14:14:43 Linus Walleij wrote:
>> On Wed, Oct 8, 2014 at 4:52 PM, Y Vo <yvo@apm.com> wrote:

>> > +       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);
>> (...)
>> > +       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?
>
> 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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Arnd Bergmann]

On Wednesday 29 October 2014 10:52:47 Linus Walleij wrote:
> On Fri, Oct 24, 2014 at 3:46 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > 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.
> 
> 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.

> 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?

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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Y Vo]

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

On Wed, Oct 29, 2014 at 5:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 29 October 2014 10:52:47 Linus Walleij wrote:
>> On Fri, Oct 24, 2014 at 3:46 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > 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.
>>
>> 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.
>
>> 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?
>
> 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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Arnd Bergmann]

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.

> +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

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Y Vo]

On Wed, Oct 29, 2014 at 10:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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.
>
>> +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 ?

Re: [PATCH v1 1/3] gpio: Add APM X-Gene standby GPIO controller driver

[Arnd Bergmann]

On Tuesday 16 December 2014 16:43:02 Y Vo wrote:
> >> +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 ?

Ok, I think I just misunderstood how this works, I assumed that you
could not reconfigure the lines. Seems ok then.

	Arnd