All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512
@ 2014-09-08 11:47 Mika Westerberg
  2014-09-08 11:48 ` [PATCH 2/2] gpio: Add support for Intel Cherryview/Braswell GPIO controller Mika Westerberg
  2014-09-09  7:24 ` [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Linus Walleij
  0 siblings, 2 replies; 9+ messages in thread
From: Mika Westerberg @ 2014-09-08 11:47 UTC (permalink / raw)
  To: Linus Walleij, Alexandre Courbot
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Ning Li,
	Alan Cox, Mika Westerberg, Mark Brown, linux-kernel, linux-gpio

Some newer Intel SoCs like Braswell already have more than 256 GPIOs
available so the default limit is exceeded. In order to support these add
back the custom GPIO header with limit of 512 GPIOs for x86.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 arch/x86/Kconfig            |  1 +
 arch/x86/include/asm/gpio.h | 54 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 778178f4c7d1..0bf6fe76e7ba 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -136,6 +136,7 @@ config X86
 	select HAVE_ACPI_APEI if ACPI
 	select HAVE_ACPI_APEI_NMI if ACPI
 	select ACPI_LEGACY_TABLES_LOOKUP if ACPI
+	select ARCH_HAVE_CUSTOM_GPIO_H
 
 config INSTRUCTION_DECODER
 	def_bool y
diff --git a/arch/x86/include/asm/gpio.h b/arch/x86/include/asm/gpio.h
index b3799d88ffcf..152b71788f2d 100644
--- a/arch/x86/include/asm/gpio.h
+++ b/arch/x86/include/asm/gpio.h
@@ -1,4 +1,50 @@
-#ifndef __LINUX_GPIO_H
-#warning Include linux/gpio.h instead of asm/gpio.h
-#include <linux/gpio.h>
-#endif
+/*
+ * GPIO customization for x86.
+ *
+ * Based on the original code:
+ *
+ * Copyright (c) 2007-2008  MontaVista Software, Inc.
+ * Author: Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __ASM_X86_GPIO_H
+#define __ASM_X86_GPIO_H
+
+#define ARCH_NR_GPIOS 512
+#include <asm-generic/gpio.h>
+
+#ifdef CONFIG_GPIOLIB
+static inline int gpio_get_value(unsigned int gpio)
+{
+	return __gpio_get_value(gpio);
+}
+
+static inline void gpio_set_value(unsigned int gpio, int value)
+{
+	__gpio_set_value(gpio, value);
+}
+
+static inline int gpio_cansleep(unsigned int gpio)
+{
+	return __gpio_cansleep(gpio);
+}
+
+static inline int gpio_to_irq(unsigned int gpio)
+{
+	return __gpio_to_irq(gpio);
+}
+
+static inline int irq_to_gpio(unsigned int irq)
+{
+	return -EINVAL;
+}
+#endif /* CONFIG_GPIOLIB */
+
+#endif /* __ASM_X86_GPIO_H */
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] gpio: Add support for Intel Cherryview/Braswell GPIO controller
  2014-09-08 11:47 [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Mika Westerberg
@ 2014-09-08 11:48 ` Mika Westerberg
  2014-09-09  7:24 ` [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Linus Walleij
  1 sibling, 0 replies; 9+ messages in thread
From: Mika Westerberg @ 2014-09-08 11:48 UTC (permalink / raw)
  To: Linus Walleij, Alexandre Courbot
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Ning Li,
	Alan Cox, Mika Westerberg, Mark Brown, linux-kernel, linux-gpio

From: Ning Li <ning.li@intel.com>

This driver supports the GPIO controllers found in newer Intel SoCs like
Cherryview and Braswell.

Signed-off-by: Ning Li <ning.li@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/Kconfig           |   8 +
 drivers/gpio/Makefile          |   1 +
 drivers/gpio/gpio-cherryview.c | 947 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 956 insertions(+)
 create mode 100644 drivers/gpio/gpio-cherryview.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515e5808..73a317d7b7bf 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -736,6 +736,14 @@ config GPIO_INTEL_MID
 	help
 	  Say Y here to support Intel Mid GPIO.
 
+config GPIO_CHERRYVIEW
+	tristate "Intel CherryView/Braswell GPIO support"
+	depends on ACPI
+	select GPIOLIB_IRQCHIP
+	help
+	  Enable support for GPIO host controllers found on Intel SoCs
+	  such as CherryView and Braswell.
+
 config GPIO_PCH
 	tristate "Intel EG20T PCH/LAPIS Semiconductor IOH(ML7223/ML7831) GPIO"
 	depends on PCI && (X86_32 || COMPILE_TEST)
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d024e396622..1162b08564a7 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_GPIO_AMD8111)	+= gpio-amd8111.o
 obj-$(CONFIG_GPIO_ARIZONA)	+= gpio-arizona.o
 obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
+obj-$(CONFIG_GPIO_CHERRYVIEW)	+= gpio-cherryview.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_CRYSTAL_COVE)	+= gpio-crystalcove.o
diff --git a/drivers/gpio/gpio-cherryview.c b/drivers/gpio/gpio-cherryview.c
new file mode 100644
index 000000000000..ba45741a345c
--- /dev/null
+++ b/drivers/gpio/gpio-cherryview.c
@@ -0,0 +1,947 @@
+/*
+ * GPIO controller driver for Intel Cherryview/Braswell.
+ *
+ * Copyright (C) 2014, Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/gpio/driver.h>
+#include <linux/seq_file.h>
+#include <linux/ioport.h>
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+
+#define FAMILY0_PAD_REGS_OFF	0x4400
+#define FAMILY_PAD_REGS_SIZE	0x400
+#define MAX_FAMILY_PAD_GPIO_NO	15
+#define GPIO_REGS_SIZE		8
+
+#define CV_PADCTRL0_REG		0x000
+#define CV_PADCTRL1_REG		0x004
+#define CV_INT_STAT_REG		0x300
+#define CV_INT_MASK_REG		0x380
+
+#define CV_GPIO_RX_STAT		BIT(0)
+#define CV_GPIO_TX_STAT		BIT(1)
+#define CV_GPIO_EN		BIT(15)
+
+#define CV_CFG_LOCK_MASK	BIT(31)
+#define CV_INT_CFG_MASK		(BIT(0) | BIT(1) | BIT(2))
+#define CV_PAD_MODE_MASK	(0xf << 16)
+
+#define CV_GPIO_CFG_MASK	(BIT(8) | BIT(9) | BIT(10))
+#define CV_GPIO_TX_EN		(1 << 8)
+#define CV_GPIO_RX_EN		(2 << 8)
+
+#define CV_INV_RX_DATA		BIT(6)
+
+#define CV_INT_SEL_MASK		(0xf << 28)
+
+enum {
+	CV_INTR_DISABLE,
+	CV_TRIG_EDGE_FALLING,
+	CV_TRIG_EDGE_RISING,
+	CV_TRIG_EDGE_BOTH,
+	CV_TRIG_LEVEL,
+};
+
+struct chv_gpio_bank {
+	const char *name;
+	const char *uid;
+	const char * const *pads;
+	size_t npads;
+};
+
+struct chv_gpio {
+	struct gpio_chip chip;
+	spinlock_t lock;
+	void __iomem *reg_base;
+	int intr_lines[16];
+	const struct chv_gpio_bank *bank;
+};
+
+static const char * const north_pads[] = {
+	"GPIO_DFX_0",
+	"GPIO_DFX_3",
+	"GPIO_DFX_7",
+	"GPIO_DFX_1",
+	"GPIO_DFX_5",
+	"GPIO_DFX_4",
+	"GPIO_DFX_8",
+	"GPIO_DFX_2",
+	"GPIO_DFX_6",
+
+	[9 ... 14] = 0,
+
+	"GPIO_SUS0",
+	"SEC_GPIO_SUS10",
+	"GPIO_SUS3",
+	"GPIO_SUS7",
+	"GPIO_SUS1",
+	"GPIO_SUS5",
+	"SEC_GPIO_SUS11",
+	"GPIO_SUS4",
+	"SEC_GPIO_SUS8",
+	"GPIO_SUS2",
+	"GPIO_SUS6",
+	"CX_PREQ_B",
+	"SEC_GPIO_SUS9",
+
+	[28 ... 29] = 0,
+
+	"TRST_B",
+	"TCK",
+	"PROCHOT_B",
+	"SVIDO_DATA",
+	"TMS",
+	"CX_PRDY_B_2",
+	"TDO_2",
+	"CX_PRDY_B",
+	"SVIDO_ALERT_B",
+	"TDO",
+	"SVIDO_CLK",
+	"TDI",
+
+	[42 ... 44] = 0,
+
+	"GP_CAMERASB_05",
+	"GP_CAMERASB_02",
+	"GP_CAMERASB_08",
+	"GP_CAMERASB_00",
+	"GP_CAMERASB_06",
+	"GP_CAMERASB_10",
+	"GP_CAMERASB_03",
+	"GP_CAMERASB_09",
+	"GP_CAMERASB_01",
+	"GP_CAMERASB_07",
+	"GP_CAMERASB_11",
+	"GP_CAMERASB_04",
+
+	[57 ... 59] = 0,
+
+	"PANEL0_BKLTEN",
+	"HV_DDI0_HPD",
+	"HV_DDI2_DDC_SDA",
+	"PANEL1_BKLTCTL",
+	"HV_DDI1_HPD",
+	"PANEL0_BKLTCTL",
+	"HV_DDI0_DDC_SDA",
+	"HV_DDI2_DDC_SCL",
+	"HV_DDI2_HPD",
+	"PANEL1_VDDEN",
+	"PANEL1_BKLTEN",
+	"HV_DDI0_DDC_SCL",
+	"PANEL0_VDDEN",
+};
+
+static const char * const southeast_pads[] = {
+	"MF_PLT_CLK0",
+	"PWM1",
+	"MF_PLT_CLK1",
+	"MF_PLT_CLK4",
+	"MF_PLT_CLK3",
+	"PWM0",
+	"MF_PLT_CLK5",
+	"MF_PLT_CLK2",
+
+	[9 ... 14] = 0,
+
+	"SDMMC2_D3_CD_B",
+	"SDMMC1_CLK",
+	"SDMMC1_D0",
+	"SDMMC2_D1",
+	"SDMMC2_CLK",
+	"SDMMC1_D2",
+	"SDMMC2_D2",
+	"SDMMC2_CMD",
+	"SDMMC1_CMD",
+	"SDMMC1_D1",
+	"SDMMC2_D0",
+	"SDMMC1_D3_CD_B",
+
+	[27 ... 29] = 0,
+
+	"SDMMC3_D1",
+	"SDMMC3_CLK",
+	"SDMMC3_D3",
+	"SDMMC3_D2",
+	"SDMMC3_CMD",
+	"SDMMC3_D0",
+
+	[36 ... 44] = 0,
+
+	"MF_LPC_AD2",
+	"LPC_CLKRUNB",
+	"MF_LPC_AD0",
+	"LPC_FRAMEB",
+	"MF_LPC_CLKOUT1",
+	"MF_LPC_AD3",
+	"MF_LPC_CLKOUT0",
+	"MF_LPC_AD1",
+
+	[53 ... 59] = 0,
+
+	"SPI1_MISO",
+	"SPI1_CSO_B",
+	"SPI1_CLK",
+	"MMC1_D6",
+	"SPI1_MOSI",
+	"MMC1_D5",
+	"SPI1_CS1_B",
+	"MMC1_D4_SD_WE",
+	"MMC1_D7",
+	"MMC1_RCLK",
+
+	[70 ... 74] = 0,
+
+	"USB_OC1_B",
+	"PMU_RESETBUTTON_B",
+	"GPIO_ALERT",
+	"SDMMC3_PWR_EN_B",
+	"ILB_SERIRQ",
+	"USB_OC0_B",
+	"SDMMC3_CD_B",
+	"SPKR",
+	"SUSPWRDNACK",
+	"SPARE_PIN",
+	"SDMMC3_1P8_EN",
+};
+
+static const char * const east_pads[] = {
+	"PMU_SLP_S3_B",
+	"PMU_BATLOW_B",
+	"SUS_STAT_B",
+	"PMU_SLP_S0IX_B",
+	"PMU_AC_PRESENT",
+	"PMU_PLTRST_B",
+	"PMU_SUSCLK",
+	"PMU_SLP_LAN_B",
+	"PMU_PWRBTN_B",
+	"PMU_SLP_S4_B",
+	"PMU_WAKE_B",
+	"PMU_WAKE_LAN_B",
+
+	[12 ... 14] = 0,
+
+	"MF_ISH_GPIO_3",
+	"MF_ISH_GPIO_7",
+	"MF_ISH_I2C1_SCL",
+	"MF_ISH_GPIO_1",
+	"MF_ISH_GPIO_5",
+	"MF_ISH_GPIO_9",
+	"MF_ISH_GPIO_0",
+	"MF_ISH_GPIO_4",
+	"MF_ISH_GPIO_8",
+	"MF_ISH_GPIO_2",
+	"MF_ISH_GPIO_6",
+	"MF_ISH_I2C1_SDA",
+};
+
+static const char * const southwest_pads[] = {
+	"FST_SPI_D2",
+	"FST_SPI_D0",
+	"FST_SPI_CLK",
+	"FST_SPI_D3",
+	"FST_SPI_CS1_B",
+	"FST_SPI_D1",
+	"FST_SPI_CS0_B",
+	"FST_SPI_CS2_B",
+
+	[8 ... 14] = 0,
+
+	"UART1_RTS_B",
+	"UART1_RXD",
+	"UART2_RXD",
+	"UART1_CTS_B",
+	"UART2_RTS_B",
+	"UART1_TXD",
+	"UART2_TXD",
+	"UART2_CTS_B",
+
+	[23 ... 29] = 0,
+
+	"MF_HDA_CLK",
+	"MF_HDA_RSTB",
+	"MF_HDA_SDIO",
+	"MF_HDA_SDO",
+	"MF_HDA_DOCKRSTB",
+	"MF_HDA_SYNC",
+	"MF_HDA_SDI1",
+	"MF_HDA_DOCKENB",
+
+	[38 ... 44] = 0,
+
+	"I2C5_SDA",
+	"I2C4_SDA",
+	"I2C6_SDA",
+	"I2C5_SCL",
+	"I2C_NFC_SDA",
+	"I2C4_SCL",
+	"I2C6_SCL",
+	"I2C_NFC_SCL",
+
+	[53 ... 59] = 0,
+
+	"I2C1_SDA",
+	"I2C0_SDA",
+	"I2C2_SDA",
+	"I2C1_SCL",
+	"I2C3_SDA",
+	"I2C0_SCL",
+	"I2C2_SCL",
+	"I2C3_SCL",
+
+	[68 ... 74] = 0,
+
+	"SATA_GP0",
+	"SATA_GP1",
+	"SATA_LEDN",
+	"SATA_GP2",
+	"MF_SMB_ALERTB",
+	"SATA_GP3",
+	"MF_SMB_CLK",
+	"MF_SMB_DATA",
+
+	[83 ... 89] = 0,
+
+	"PCIE_CLKREQ0B",
+	"PCIE_CLKREQ1B",
+	"GP_SSP_2_CLK",
+	"PCIE_CLKREQ2B",
+	"GP_SSP_2_RXD",
+	"PCIE_CLKREQ3B",
+	"GP_SSP_2_FS",
+	"GP_SSP_2_TXD",
+};
+
+static const struct chv_gpio_bank chv_banks[] = {
+	{
+		.name = "SW",
+		.uid = "1",
+		.pads = southwest_pads,
+		.npads = ARRAY_SIZE(southwest_pads),
+	},
+	{
+		.name = "N",
+		.uid = "2",
+		.pads = north_pads,
+		.npads = ARRAY_SIZE(north_pads),
+	},
+	{
+		.name = "E",
+		.uid = "3",
+		.pads = east_pads,
+		.npads = ARRAY_SIZE(east_pads),
+	},
+	{
+		.name = "SE",
+		.uid = "4",
+		.pads = southeast_pads,
+		.npads = ARRAY_SIZE(southeast_pads),
+	},
+};
+
+#define to_chv_gpio(c)	container_of(c, struct chv_gpio, chip)
+
+static void __iomem *chv_gpio_reg(struct chv_gpio *cg, unsigned offset, int reg)
+{
+	u32 reg_offset;
+
+	if (reg == CV_INT_STAT_REG || reg == CV_INT_MASK_REG) {
+		reg_offset = 0;
+	} else {
+		reg_offset = FAMILY0_PAD_REGS_OFF +
+		      FAMILY_PAD_REGS_SIZE * (offset / MAX_FAMILY_PAD_GPIO_NO) +
+		      GPIO_REGS_SIZE * (offset % MAX_FAMILY_PAD_GPIO_NO);
+	}
+
+	return cg->reg_base + reg_offset + reg;
+}
+
+static inline void chv_writel(u32 value, void __iomem *reg)
+{
+	writel(value, reg);
+	/* simple readback to confirm the bus transferring done */
+	readl(reg);
+}
+
+/* When Pad Cfg is locked, driver can only change GPIOTXState or GPIORXState */
+static inline bool chv_gpio_pad_locked(struct chv_gpio *cg, unsigned offset)
+{
+	void __iomem *reg;
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL1_REG);
+	return readl(reg) & CV_CFG_LOCK_MASK;
+}
+
+static int chv_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	struct chv_gpio *cg = to_chv_gpio(chip);
+	void __iomem *reg;
+	u32 value;
+
+	if (!cg->bank->pads[offset])
+		return -EINVAL;
+
+	if (chv_gpio_pad_locked(cg, offset))
+		return 0;
+
+	/* Disable interrupt generation */
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL1_REG);
+	value = readl(reg);
+	value &= ~(CV_INT_CFG_MASK | CV_INV_RX_DATA);
+	chv_writel(value, reg);
+
+	/* Switch to a GPIO mode */
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+	value = readl(reg) | CV_GPIO_EN;
+	chv_writel(value, reg);
+
+	return 0;
+}
+
+static void chv_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	struct chv_gpio *cg = to_chv_gpio(chip);
+	void __iomem *reg;
+	u32 value;
+
+	if (chv_gpio_pad_locked(cg, offset))
+		return;
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+	value = readl(reg) & ~CV_GPIO_EN;
+	chv_writel(value, reg);
+}
+
+static void chv_update_irq_type(struct chv_gpio *cg, unsigned type,
+				void __iomem *reg)
+{
+	u32 value;
+
+	value = readl(reg);
+	value &= ~CV_INT_CFG_MASK;
+	value &= ~CV_INV_RX_DATA;
+
+	if (type & IRQ_TYPE_EDGE_BOTH) {
+		if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH)
+			value |= CV_TRIG_EDGE_BOTH;
+		else if (type & IRQ_TYPE_EDGE_RISING)
+			value |= CV_TRIG_EDGE_RISING;
+		else if (type & IRQ_TYPE_EDGE_FALLING)
+			value |= CV_TRIG_EDGE_FALLING;
+	} else if (type & IRQ_TYPE_LEVEL_MASK) {
+			value |= CV_TRIG_LEVEL;
+		if (type & IRQ_TYPE_LEVEL_LOW)
+			value |= CV_INV_RX_DATA;
+	}
+
+	chv_writel(value, reg);
+}
+
+/* BIOS programs IntSel bits for shared interrupt. GPIO driver follows it. */
+static void pad_intr_line_save(struct chv_gpio *cg, unsigned offset)
+{
+	void __iomem *reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+	u32 value, intr_line;
+
+	value = readl(reg);
+	intr_line = (value & CV_INT_SEL_MASK) >> 28;
+	cg->intr_lines[intr_line] = offset;
+}
+
+static int chv_irq_type(struct irq_data *d, unsigned type)
+{
+	struct chv_gpio *cg = irq_data_get_irq_chip_data(d);
+	u32 offset = irqd_to_hwirq(d);
+	void __iomem *reg;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cg->lock, flags);
+
+	/*
+	 * Pins which can be used as shared interrupt are configured in
+	 * BIOS. Driver trusts BIOS configurations and assigns different
+	 * handler according to the irq type.
+	 *
+	 * Driver needs to save the mapping between each pin and
+	 * its interrupt line.
+	 * 1. If the pin cfg is locked in BIOS:
+	 *	Trust BIOS has programmed IntWakeCfg bits correctly,
+	 *	driver just needs to save the mapping.
+	 * 2. If the pin cfg is not locked in BIOS:
+	 *	Driver programs the IntWakeCfg bits and save the mapping.
+	 */
+	if (!chv_gpio_pad_locked(cg, offset)) {
+		reg = chv_gpio_reg(cg, offset, CV_PADCTRL1_REG);
+		chv_update_irq_type(cg, type, reg);
+	}
+
+	pad_intr_line_save(cg, offset);
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		__irq_set_handler_locked(d->irq, handle_edge_irq);
+	else if (type & IRQ_TYPE_LEVEL_MASK)
+		__irq_set_handler_locked(d->irq, handle_level_irq);
+
+	spin_unlock_irqrestore(&cg->lock, flags);
+
+	return 0;
+}
+
+static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct chv_gpio *cg = to_chv_gpio(chip);
+	unsigned long flags;
+	void __iomem *reg;
+	u32 value;
+	int ret;
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+
+	spin_lock_irqsave(&cg->lock, flags);
+
+	value = readl(reg);
+	if (value & CV_GPIO_TX_EN)
+		ret = !!(value & CV_GPIO_TX_STAT);
+	else
+		ret = !!(value & CV_GPIO_RX_STAT);
+
+	spin_unlock_irqrestore(&cg->lock, flags);
+
+	return ret;
+}
+
+static void chv_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct chv_gpio *cg = to_chv_gpio(chip);
+	void __iomem *reg;
+	unsigned long flags;
+	u32 old_val;
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+
+	spin_lock_irqsave(&cg->lock, flags);
+
+	old_val = readl(reg);
+
+	if (value)
+		chv_writel(old_val | CV_GPIO_TX_STAT, reg);
+	else
+		chv_writel(old_val & ~CV_GPIO_TX_STAT, reg);
+
+	spin_unlock_irqrestore(&cg->lock, flags);
+}
+
+static int chv_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	struct chv_gpio *cg = to_chv_gpio(chip);
+	unsigned long flags;
+	void __iomem *reg;
+	u32 value;
+
+	if (chv_gpio_pad_locked(cg, offset))
+		return 0;
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+
+	spin_lock_irqsave(&cg->lock, flags);
+
+	value = readl(reg) & ~CV_GPIO_CFG_MASK;
+	/* Disable TX and Enable RX */
+	value |= CV_GPIO_RX_EN;
+	chv_writel(value, reg);
+
+	spin_unlock_irqrestore(&cg->lock, flags);
+
+	return 0;
+}
+
+static int chv_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+				     int value)
+{
+	struct chv_gpio *cg = to_chv_gpio(chip);
+	unsigned long flags;
+	void __iomem *reg;
+	u32 reg_val;
+
+	if (chv_gpio_pad_locked(cg, offset))
+		return 0;
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+
+	spin_lock_irqsave(&cg->lock, flags);
+	reg_val = readl(reg) & ~CV_GPIO_CFG_MASK;
+	reg_val |= CV_GPIO_TX_EN;
+
+	/* Control TX State */
+	if (value)
+		reg_val |= CV_GPIO_TX_STAT;
+	else
+		reg_val &= ~CV_GPIO_TX_STAT;
+
+	chv_writel(reg_val, reg);
+
+	spin_unlock_irqrestore(&cg->lock, flags);
+	return 0;
+}
+
+static void chv_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+	struct chv_gpio *cg = to_chv_gpio(chip);
+	u32 ctrl0, ctrl1, offs;
+	unsigned long flags;
+	void __iomem *reg;
+	int i;
+
+	spin_lock_irqsave(&cg->lock, flags);
+
+	for (i = 0; i < cg->chip.ngpio; i++) {
+		const char *intcfg;
+		const char *label;
+		const char *value;
+		const char *dir;
+		char pin[8];
+
+		if (!cg->bank->pads[i])
+			continue;
+
+		offs = FAMILY0_PAD_REGS_OFF +
+		      FAMILY_PAD_REGS_SIZE * (i / MAX_FAMILY_PAD_GPIO_NO) +
+		      GPIO_REGS_SIZE * (i % MAX_FAMILY_PAD_GPIO_NO);
+
+		ctrl0 = readl(chv_gpio_reg(cg, i, CV_PADCTRL0_REG));
+		ctrl1 = readl(chv_gpio_reg(cg, i, CV_PADCTRL1_REG));
+
+		snprintf(pin, sizeof(pin), "%s%02d", cg->bank->name, i);
+
+		switch (ctrl1 & CV_INT_CFG_MASK) {
+		case CV_INTR_DISABLE:
+			intcfg = "disabled";
+			break;
+
+		case CV_TRIG_EDGE_FALLING:
+			intcfg = "falling";
+			break;
+
+		case CV_TRIG_EDGE_RISING:
+			intcfg = "rising";
+			break;
+
+		case CV_TRIG_EDGE_BOTH:
+			intcfg = "both";
+			break;
+
+		case CV_TRIG_LEVEL:
+			if (ctrl1 & CV_INV_RX_DATA)
+				intcfg = "low";
+			else
+				intcfg = "high";
+			break;
+
+		default:
+			intcfg = "unknown";
+			break;
+		}
+
+		switch ((ctrl0 & CV_GPIO_CFG_MASK) >> 8) {
+		case 0:
+			dir = "in out";
+			break;
+		case 1:
+			dir = "   out";
+			break;
+		case 2:
+			dir = "in";
+			break;
+		case 3:
+			dir = "HiZ";
+			break;
+		default:
+			dir = "unknown";
+			break;
+		}
+
+		if (ctrl0 & CV_GPIO_TX_EN)
+			value = ctrl0 & CV_GPIO_TX_STAT ? "high" : "low";
+		else
+			value = ctrl0 & CV_GPIO_RX_STAT ? "high" : "low";
+
+		seq_printf(s,
+			   "%c%-4s %-17s %-8s %-4s 0x%03x %d %-8s %02d 0x%08x 0x%08x",
+			   chv_gpio_pad_locked(cg, i) ? '*' : ' ',
+			   pin, cg->bank->pads[i], dir, value, offs,
+			   (ctrl0 & CV_PAD_MODE_MASK) >> 16, intcfg,
+			   (ctrl0 & CV_INT_SEL_MASK) >> 28, ctrl0, ctrl1);
+
+		label = gpiochip_is_requested(&cg->chip, i);
+		if (label)
+			seq_printf(s, " %s\n", label);
+		else
+			seq_puts(s, "\n");
+	}
+
+	reg = chv_gpio_reg(cg, 0, CV_INT_STAT_REG);
+	seq_printf(s, "CV_INT_STAT_REG: 0x%08x\n", readl(reg));
+
+	reg = chv_gpio_reg(cg, 0, CV_INT_MASK_REG);
+	seq_printf(s, "CV_INT_MASK_REG: 0x%08x\n", readl(reg));
+
+	for (i = 0; i < ARRAY_SIZE(cg->intr_lines); i++) {
+		if (cg->intr_lines[i] >= 0)
+			seq_printf(s, "intline: %d, offset: %d\n", i,
+				   cg->intr_lines[i]);
+	}
+
+	seq_puts(s, "\n");
+
+	spin_unlock_irqrestore(&cg->lock, flags);
+}
+
+static void chv_irq_unmask(struct irq_data *d)
+{
+	struct chv_gpio *cg = irq_data_get_irq_chip_data(d);
+	u32 offset = irqd_to_hwirq(d);
+	u32 value, intr_line;
+	unsigned long flags;
+	void __iomem *reg;
+
+	spin_lock_irqsave(&cg->lock, flags);
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+	intr_line = (readl(reg) & CV_INT_SEL_MASK) >> 28;
+
+	reg = chv_gpio_reg(cg, 0, CV_INT_MASK_REG);
+	value = readl(reg);
+	value |= (1 << intr_line);
+	chv_writel(value, reg);
+
+	spin_unlock_irqrestore(&cg->lock, flags);
+}
+
+static void chv_irq_mask(struct irq_data *d)
+{
+	struct chv_gpio *cg = irq_data_get_irq_chip_data(d);
+	u32 offset = irqd_to_hwirq(d);
+	u32 value, intr_line;
+	unsigned long flags;
+	void __iomem *reg;
+
+	spin_lock_irqsave(&cg->lock, flags);
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL0_REG);
+	intr_line = (readl(reg) & CV_INT_SEL_MASK) >> 28;
+
+	value = readl(reg);
+	value &= ~(1 << intr_line);
+	chv_writel(value, reg);
+
+	spin_unlock_irqrestore(&cg->lock, flags);
+}
+
+static void chv_irq_ack(struct irq_data *d)
+{
+}
+
+static void chv_irq_shutdown(struct irq_data *d)
+{
+	struct chv_gpio *cg = irq_data_get_irq_chip_data(d);
+	u32 offset = irqd_to_hwirq(d);
+	void __iomem *reg;
+	unsigned long flags;
+
+	reg = chv_gpio_reg(cg, offset, CV_PADCTRL1_REG);
+
+	chv_irq_mask(d);
+
+	if (!chv_gpio_pad_locked(cg, offset)) {
+		spin_lock_irqsave(&cg->lock, flags);
+		chv_update_irq_type(cg, IRQ_TYPE_NONE, reg);
+		spin_unlock_irqrestore(&cg->lock, flags);
+	}
+}
+
+static struct irq_chip chv_irqchip = {
+	.name = "CHV-GPIO",
+	.irq_mask = chv_irq_mask,
+	.irq_unmask = chv_irq_unmask,
+	.irq_set_type = chv_irq_type,
+	.irq_ack = chv_irq_ack,
+	.irq_shutdown = chv_irq_shutdown,
+	.flags = IRQCHIP_SKIP_SET_WAKE,
+};
+
+static void chv_gpio_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+	struct irq_data *data = irq_desc_get_irq_data(desc);
+	struct chv_gpio *cg = to_chv_gpio(irq_desc_get_handler_data(desc));
+	struct irq_chip *chip = irq_data_get_irq_chip(data);
+	u32 intr_line, mask, offset;
+	void __iomem *reg, *mask_reg;
+	u32 pending;
+
+	/* each GPIO controller has one INT_STAT reg */
+	reg = chv_gpio_reg(cg, 0, CV_INT_STAT_REG);
+	mask_reg = chv_gpio_reg(cg, 0, CV_INT_MASK_REG);
+	while ((pending = (readl(reg) & readl(mask_reg) & 0xffff))) {
+		unsigned irq;
+
+		intr_line = __ffs(pending);
+		mask = BIT(intr_line);
+		chv_writel(mask, reg);
+		offset = cg->intr_lines[intr_line];
+		if (unlikely(offset < 0)) {
+			dev_warn(cg->chip.dev, "unregistered shared irq\n");
+			continue;
+		}
+
+		irq = irq_find_mapping(cg->chip.irqdomain, offset);
+		generic_handle_irq(irq);
+	}
+
+	chip->irq_eoi(data);
+}
+
+static void chv_gpio_irq_init_hw(struct chv_gpio *cg)
+{
+	void __iomem *reg;
+
+	/* Mask all interrupts */
+	reg = chv_gpio_reg(cg, 0, CV_INT_MASK_REG);
+	chv_writel(0, reg);
+
+	reg = chv_gpio_reg(cg, 0, CV_INT_STAT_REG);
+	chv_writel(0xffff, reg);
+}
+
+static const struct gpio_chip chv_gpio_chip = {
+	.owner = THIS_MODULE,
+	.request = chv_gpio_request,
+	.free = chv_gpio_free,
+	.direction_input = chv_gpio_direction_input,
+	.direction_output = chv_gpio_direction_output,
+	.get = chv_gpio_get,
+	.set = chv_gpio_set,
+	.dbg_show = chv_gpio_dbg_show,
+	.base = -1,
+};
+
+static int chv_gpio_probe(struct platform_device *pdev)
+{
+	struct resource *mem_rc, *irq_rc;
+	const struct chv_gpio_bank *bank;
+	struct acpi_device *adev;
+	struct chv_gpio *cg;
+	int i, ret = 0;
+
+	adev = ACPI_COMPANION(&pdev->dev);
+	if (!adev)
+		return -ENODEV;
+
+	cg = devm_kzalloc(&pdev->dev, sizeof(*cg), GFP_KERNEL);
+	if (!cg)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(chv_banks); i++) {
+		bank = &chv_banks[i];
+		if (!strcmp(adev->pnp.unique_id, bank->uid)) {
+			cg->bank = bank;
+			break;
+		}
+	}
+	if (i == ARRAY_SIZE(chv_banks))
+		return -ENODEV;
+
+	mem_rc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	cg->reg_base = devm_ioremap_resource(&pdev->dev, mem_rc);
+	if (IS_ERR(cg->reg_base))
+		return PTR_ERR(cg->reg_base);
+
+	spin_lock_init(&cg->lock);
+	cg->chip = chv_gpio_chip;
+	cg->chip.ngpio = cg->bank->npads;
+	cg->chip.label = dev_name(&pdev->dev);
+	cg->chip.dev = &pdev->dev;
+
+	/* Initialize interrupt lines array with negative value */
+	for (i = 0; i < ARRAY_SIZE(cg->intr_lines); i++)
+		cg->intr_lines[i] = -1;
+
+	ret = gpiochip_add(&cg->chip);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed adding GPIO chip\n");
+		return ret;
+	}
+
+	irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (irq_rc && irq_rc->start) {
+		chv_gpio_irq_init_hw(cg);
+
+		ret = gpiochip_irqchip_add(&cg->chip, &chv_irqchip, 0,
+					   handle_simple_irq, IRQ_TYPE_NONE);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to add irqchip\n");
+			gpiochip_remove(&cg->chip);
+			return ret;
+		}
+
+		gpiochip_set_chained_irqchip(&cg->chip, &chv_irqchip,
+					     (unsigned)irq_rc->start,
+					     chv_gpio_irq_handler);
+	}
+
+	return 0;
+}
+
+static int chv_gpio_remove(struct platform_device *pdev)
+{
+	struct chv_gpio *cg = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&cg->chip);
+	return 0;
+}
+
+static const struct acpi_device_id chv_gpio_acpi_match[] = {
+	{ "INT33FF" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, chv_gpio_acpi_match);
+
+static struct platform_driver chv_gpio_driver = {
+	.probe = chv_gpio_probe,
+	.remove = chv_gpio_remove,
+	.driver = {
+		.name = "chv_gpio",
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(chv_gpio_acpi_match),
+	},
+};
+
+static int __init chv_gpio_init(void)
+{
+	return platform_driver_register(&chv_gpio_driver);
+}
+subsys_initcall(chv_gpio_init);
+
+static void __exit chv_gpio_exit(void)
+{
+	platform_driver_unregister(&chv_gpio_driver);
+}
+module_exit(chv_gpio_exit);
+
+MODULE_DESCRIPTION("GPIO driver for Intel Cherryview/Braswell");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Ning Li <ning.li@intel.com>");
+MODULE_AUTHOR("Alan Cox <alan@linux.intel.com>");
+MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
+MODULE_ALIAS("platform:chv_gpio");
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512
  2014-09-08 11:47 [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Mika Westerberg
  2014-09-08 11:48 ` [PATCH 2/2] gpio: Add support for Intel Cherryview/Braswell GPIO controller Mika Westerberg
@ 2014-09-09  7:24 ` Linus Walleij
  2014-09-09  7:41   ` H. Peter Anvin
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: Linus Walleij @ 2014-09-09  7:24 UTC (permalink / raw)
  To: Mika Westerberg, Alexandre Courbot
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Ning Li,
	Alan Cox, Mark Brown, linux-kernel, linux-gpio

On Mon, Sep 8, 2014 at 1:47 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> Some newer Intel SoCs like Braswell already have more than 256 GPIOs
> available so the default limit is exceeded. In order to support these add
> back the custom GPIO header with limit of 512 GPIOs for x86.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Argh! This is the kind of stuff I want to get rid of ....

Preferably gpio should be a subsystem without a lot of hooks all over
the place with arch-specific modifications for this and that, including
the max number of GPIOs.

I would actually prefer if you bump the value in
include/asm-generic/gpio.h to 512 over this.

But better still, now that we have descriptors etc would be to define
some new per-arch selectable config option like
CONFIG_ONLY_DYNAMIC_GPIO that changes the GPIO
core to use something like a radix tree to store and retrieve
descriptors.

I.e. in drivers/gpio/gpiolib.c get rid of this:
static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];

Replace it with a radix tree of descriptors.

This however makes it *impossible* to use things like desc_to_gpio()
and/or gpio_to_desc() so the code has to be augmented all over the
place to avoid any uses of GPIO numbers on that architecture,
but I am sure it *can* be done on pure ACPI or device tree
systems, and that's what we should aim for.

Comments?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512
  2014-09-09  7:24 ` [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Linus Walleij
@ 2014-09-09  7:41   ` H. Peter Anvin
  2014-09-09  9:46   ` Mika Westerberg
  2014-09-19  7:20   ` Alexandre Courbot
  2 siblings, 0 replies; 9+ messages in thread
From: H. Peter Anvin @ 2014-09-09  7:41 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Mika Westerberg, Alexandre Courbot, Thomas Gleixner, Ingo Molnar,
	x86, Ning Li, Alan Cox, Mark Brown, linux-kernel, linux-gpio

Isn't this exactly what ida is for?

Sent from my tablet, pardon any formatting problems.

> On Sep 9, 2014, at 0:24, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> On Mon, Sep 8, 2014 at 1:47 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
>> Some newer Intel SoCs like Braswell already have more than 256 GPIOs
>> available so the default limit is exceeded. In order to support these add
>> back the custom GPIO header with limit of 512 GPIOs for x86.
>> 
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Argh! This is the kind of stuff I want to get rid of ....
> 
> Preferably gpio should be a subsystem without a lot of hooks all over
> the place with arch-specific modifications for this and that, including
> the max number of GPIOs.
> 
> I would actually prefer if you bump the value in
> include/asm-generic/gpio.h to 512 over this.
> 
> But better still, now that we have descriptors etc would be to define
> some new per-arch selectable config option like
> CONFIG_ONLY_DYNAMIC_GPIO that changes the GPIO
> core to use something like a radix tree to store and retrieve
> descriptors.
> 
> I.e. in drivers/gpio/gpiolib.c get rid of this:
> static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
> 
> Replace it with a radix tree of descriptors.
> 
> This however makes it *impossible* to use things like desc_to_gpio()
> and/or gpio_to_desc() so the code has to be augmented all over the
> place to avoid any uses of GPIO numbers on that architecture,
> but I am sure it *can* be done on pure ACPI or device tree
> systems, and that's what we should aim for.
> 
> Comments?
> 
> Yours,
> Linus Walleij

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512
  2014-09-09  7:24 ` [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Linus Walleij
  2014-09-09  7:41   ` H. Peter Anvin
@ 2014-09-09  9:46   ` Mika Westerberg
  2014-09-19  7:20   ` Alexandre Courbot
  2 siblings, 0 replies; 9+ messages in thread
From: Mika Westerberg @ 2014-09-09  9:46 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Alexandre Courbot, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Ning Li, Alan Cox, Mark Brown, linux-kernel, linux-gpio

On Tue, Sep 09, 2014 at 09:24:59AM +0200, Linus Walleij wrote:
> On Mon, Sep 8, 2014 at 1:47 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > Some newer Intel SoCs like Braswell already have more than 256 GPIOs
> > available so the default limit is exceeded. In order to support these add
> > back the custom GPIO header with limit of 512 GPIOs for x86.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Argh! This is the kind of stuff I want to get rid of ....
> 
> Preferably gpio should be a subsystem without a lot of hooks all over
> the place with arch-specific modifications for this and that, including
> the max number of GPIOs.
> 
> I would actually prefer if you bump the value in
> include/asm-generic/gpio.h to 512 over this.

OK, this makes sense and I can do the change for the next revision of
the patch.

> But better still, now that we have descriptors etc would be to define
> some new per-arch selectable config option like
> CONFIG_ONLY_DYNAMIC_GPIO that changes the GPIO
> core to use something like a radix tree to store and retrieve
> descriptors.
> 
> I.e. in drivers/gpio/gpiolib.c get rid of this:
> static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
> 
> Replace it with a radix tree of descriptors.
> 
> This however makes it *impossible* to use things like desc_to_gpio()
> and/or gpio_to_desc() so the code has to be augmented all over the
> place to avoid any uses of GPIO numbers on that architecture,
> but I am sure it *can* be done on pure ACPI or device tree
> systems, and that's what we should aim for.
> 
> Comments?

That's a rather big rework to the GPIO subsystem and its users. I agree
that it should be the goal eventually. For x86 such conversion is not
that simple because we have systems out there that have neither ACPI nor
DT available.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512
  2014-09-09  7:24 ` [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Linus Walleij
  2014-09-09  7:41   ` H. Peter Anvin
  2014-09-09  9:46   ` Mika Westerberg
@ 2014-09-19  7:20   ` Alexandre Courbot
  2014-09-19 10:46     ` Mika Westerberg
  2014-09-19 17:48     ` Linus Walleij
  2 siblings, 2 replies; 9+ messages in thread
From: Alexandre Courbot @ 2014-09-19  7:20 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Mika Westerberg, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Ning Li, Alan Cox, Mark Brown, linux-kernel, linux-gpio

I should have responded to that thread long ago, but I am currently on
holidays and strangely tend to check my mail less often than I should.
:P

On Tue, Sep 9, 2014 at 4:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Argh! This is the kind of stuff I want to get rid of ....
>
> Preferably gpio should be a subsystem without a lot of hooks all over
> the place with arch-specific modifications for this and that, including
> the max number of GPIOs.
>
> I would actually prefer if you bump the value in
> include/asm-generic/gpio.h to 512 over this.
>
> But better still, now that we have descriptors etc would be to define
> some new per-arch selectable config option like
> CONFIG_ONLY_DYNAMIC_GPIO that changes the GPIO
> core to use something like a radix tree to store and retrieve
> descriptors.
>
> I.e. in drivers/gpio/gpiolib.c get rid of this:
> static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
>
> Replace it with a radix tree of descriptors.
>
> This however makes it *impossible* to use things like desc_to_gpio()
> and/or gpio_to_desc() so the code has to be augmented all over the
> place to avoid any uses of GPIO numbers on that architecture,
> but I am sure it *can* be done on pure ACPI or device tree
> systems, and that's what we should aim for.

desc_to_gpio()/gpio_to_desc() could still work even if we remove the
big array of GPIO descriptors. Actually that's what I intended to do
when I first submitted the gpiod patches some time ago but it was
rejected for performance reasons.

desc_to_gpio() actually doesn't even access this array - it does its
job using the chip base and the beginning address of its descriptors
array.

gpio_to_desc() would suffer a performance hit. What I initially
proposed was to parse the linked list of GPIO chips and check if the
requested number is in their assigned range. This is obviously slower
than accessing an array, but if we consider that we generally don't
have too many GPIO chips on a given hardware I don't think the hit
would be that bad. It would also give some incentive for people to
move to the gpiod interface.

I also have a patch in my queue that enables multiple users on the
same GPIO descriptor (something requested since some time already).
What happens with it is that descriptors ownership is fully
transferred to the gpio_chip instances, and the static array becomes a
array of double-pointers, making it considerable smaller and reducing
the impact of increasing its size. Maybe I should submit that change
just for this case?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512
  2014-09-19  7:20   ` Alexandre Courbot
@ 2014-09-19 10:46     ` Mika Westerberg
  2014-09-19 17:48     ` Linus Walleij
  1 sibling, 0 replies; 9+ messages in thread
From: Mika Westerberg @ 2014-09-19 10:46 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Linus Walleij, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Ning Li, Alan Cox, Mark Brown, linux-kernel, linux-gpio

On Fri, Sep 19, 2014 at 04:20:22PM +0900, Alexandre Courbot wrote:
> I should have responded to that thread long ago, but I am currently on
> holidays and strangely tend to check my mail less often than I should.
> :P
> 
> On Tue, Sep 9, 2014 at 4:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > Argh! This is the kind of stuff I want to get rid of ....
> >
> > Preferably gpio should be a subsystem without a lot of hooks all over
> > the place with arch-specific modifications for this and that, including
> > the max number of GPIOs.
> >
> > I would actually prefer if you bump the value in
> > include/asm-generic/gpio.h to 512 over this.
> >
> > But better still, now that we have descriptors etc would be to define
> > some new per-arch selectable config option like
> > CONFIG_ONLY_DYNAMIC_GPIO that changes the GPIO
> > core to use something like a radix tree to store and retrieve
> > descriptors.
> >
> > I.e. in drivers/gpio/gpiolib.c get rid of this:
> > static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
> >
> > Replace it with a radix tree of descriptors.
> >
> > This however makes it *impossible* to use things like desc_to_gpio()
> > and/or gpio_to_desc() so the code has to be augmented all over the
> > place to avoid any uses of GPIO numbers on that architecture,
> > but I am sure it *can* be done on pure ACPI or device tree
> > systems, and that's what we should aim for.
> 
> desc_to_gpio()/gpio_to_desc() could still work even if we remove the
> big array of GPIO descriptors. Actually that's what I intended to do
> when I first submitted the gpiod patches some time ago but it was
> rejected for performance reasons.
> 
> desc_to_gpio() actually doesn't even access this array - it does its
> job using the chip base and the beginning address of its descriptors
> array.
> 
> gpio_to_desc() would suffer a performance hit. What I initially
> proposed was to parse the linked list of GPIO chips and check if the
> requested number is in their assigned range. This is obviously slower
> than accessing an array, but if we consider that we generally don't
> have too many GPIO chips on a given hardware I don't think the hit
> would be that bad. It would also give some incentive for people to
> move to the gpiod interface.

>From what I can tell based on x86 based systems, there is typically only
one GPIO controller in the PCH/SoC that controls all the pins. A good
example is the Braswell/Cherryview controller.

So I agree with you that the performance hit would be negliglible.

> I also have a patch in my queue that enables multiple users on the
> same GPIO descriptor (something requested since some time already).
> What happens with it is that descriptors ownership is fully
> transferred to the gpio_chip instances, and the static array becomes a
> array of double-pointers, making it considerable smaller and reducing
> the impact of increasing its size. Maybe I should submit that change
> just for this case?

Go for it :)

I can try to assist if any testing is needed.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512
  2014-09-19  7:20   ` Alexandre Courbot
  2014-09-19 10:46     ` Mika Westerberg
@ 2014-09-19 17:48     ` Linus Walleij
  2014-09-20  5:56       ` Alexandre Courbot
  1 sibling, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2014-09-19 17:48 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Mika Westerberg, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Ning Li, Alan Cox, Mark Brown, linux-kernel, linux-gpio

On Fri, Sep 19, 2014 at 12:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Sep 9, 2014 at 4:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> This however makes it *impossible* to use things like desc_to_gpio()
>> and/or gpio_to_desc() so the code has to be augmented all over the
>> place to avoid any uses of GPIO numbers on that architecture,
>> but I am sure it *can* be done on pure ACPI or device tree
>> systems, and that's what we should aim for.
>
> desc_to_gpio()/gpio_to_desc() could still work even if we remove the
> big array of GPIO descriptors. Actually that's what I intended to do
> when I first submitted the gpiod patches some time ago but it was
> rejected for performance reasons.

OK. I'm ready to revisit the subject.

> desc_to_gpio() actually doesn't even access this array - it does its
> job using the chip base and the beginning address of its descriptors
> array.

Right.

> gpio_to_desc() would suffer a performance hit. What I initially
> proposed was to parse the linked list of GPIO chips and check if the
> requested number is in their assigned range. This is obviously slower
> than accessing an array, but if we consider that we generally don't
> have too many GPIO chips on a given hardware I don't think the hit
> would be that bad. It would also give some incentive for people to
> move to the gpiod interface.

I think the performance hit is acceptable, because this should
not be on a hot path anyway. I would say go ahead with this refactoring.

> I also have a patch in my queue that enables multiple users on the
> same GPIO descriptor (something requested since some time already).
> What happens with it is that descriptors ownership is fully
> transferred to the gpio_chip instances, and the static array becomes a
> array of double-pointers, making it considerable smaller and reducing
> the impact of increasing its size. Maybe I should submit that change
> just for this case?

Ummmmm I think that is an orthogonal thing, but honestly I'm
not following the double-pointers thing, so I guess I need to see
the patch.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512
  2014-09-19 17:48     ` Linus Walleij
@ 2014-09-20  5:56       ` Alexandre Courbot
  0 siblings, 0 replies; 9+ messages in thread
From: Alexandre Courbot @ 2014-09-20  5:56 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Mika Westerberg, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Ning Li, Alan Cox, Mark Brown, linux-kernel, linux-gpio

On Sat, Sep 20, 2014 at 2:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 19, 2014 at 12:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Tue, Sep 9, 2014 at 4:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> This however makes it *impossible* to use things like desc_to_gpio()
>>> and/or gpio_to_desc() so the code has to be augmented all over the
>>> place to avoid any uses of GPIO numbers on that architecture,
>>> but I am sure it *can* be done on pure ACPI or device tree
>>> systems, and that's what we should aim for.
>>
>> desc_to_gpio()/gpio_to_desc() could still work even if we remove the
>> big array of GPIO descriptors. Actually that's what I intended to do
>> when I first submitted the gpiod patches some time ago but it was
>> rejected for performance reasons.
>
> OK. I'm ready to revisit the subject.
>
>> desc_to_gpio() actually doesn't even access this array - it does its
>> job using the chip base and the beginning address of its descriptors
>> array.
>
> Right.
>
>> gpio_to_desc() would suffer a performance hit. What I initially
>> proposed was to parse the linked list of GPIO chips and check if the
>> requested number is in their assigned range. This is obviously slower
>> than accessing an array, but if we consider that we generally don't
>> have too many GPIO chips on a given hardware I don't think the hit
>> would be that bad. It would also give some incentive for people to
>> move to the gpiod interface.
>
> I think the performance hit is acceptable, because this should
> not be on a hot path anyway. I would say go ahead with this refactoring.

Great! I will come with something once my holidays are over. It should
not be a complex change.

>
>> I also have a patch in my queue that enables multiple users on the
>> same GPIO descriptor (something requested since some time already).
>> What happens with it is that descriptors ownership is fully
>> transferred to the gpio_chip instances, and the static array becomes a
>> array of double-pointers, making it considerable smaller and reducing
>> the impact of increasing its size. Maybe I should submit that change
>> just for this case?
>
> Ummmmm I think that is an orthogonal thing, but honestly I'm
> not following the double-pointers thing, so I guess I need to see
> the patch.

Yes, this is completely orthogonal, and actually this won't be needed
if we decide to get rid of that array.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-09-20  5:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-08 11:47 [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Mika Westerberg
2014-09-08 11:48 ` [PATCH 2/2] gpio: Add support for Intel Cherryview/Braswell GPIO controller Mika Westerberg
2014-09-09  7:24 ` [PATCH 1/2] x86, gpio: Increase ARCH_NR_GPIOs to 512 Linus Walleij
2014-09-09  7:41   ` H. Peter Anvin
2014-09-09  9:46   ` Mika Westerberg
2014-09-19  7:20   ` Alexandre Courbot
2014-09-19 10:46     ` Mika Westerberg
2014-09-19 17:48     ` Linus Walleij
2014-09-20  5:56       ` Alexandre Courbot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.