linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/3] gpio: gpio-hisi: Add HiSilicon GPIO support
@ 2020-12-02  9:32 Luo Jiaxing
  2020-12-02  9:32 ` [PATCH v1 1/3] " Luo Jiaxing
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Luo Jiaxing @ 2020-12-02  9:32 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, catalin.marinas, will
  Cc: andy.shevchenko, andriy.shevchenko, linux-gpio, linux-kernel, linuxarm

This series is the GPIO driver for HiSilicon's ARM SoC.
It provide patches for device driver, MAINTAINER file, and enable gpio-hisi
at defconfig.

Thanks
Jiaxing

Luo Jiaxing (3):
  gpio: gpio-hisi: Add HiSilicon GPIO support
  MAINTAINERS: Add maintainer for HiSilicon GPIO driver
  arm64: defconfig: enable GPIO_HISI

 MAINTAINERS                  |   7 +
 arch/arm64/configs/defconfig |   1 +
 drivers/gpio/Kconfig         |  11 ++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-hisi.c     | 356 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 376 insertions(+)
 create mode 100644 drivers/gpio/gpio-hisi.c

-- 
2.7.4


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

* [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
  2020-12-02  9:32 [PATCH v1 0/3] gpio: gpio-hisi: Add HiSilicon GPIO support Luo Jiaxing
@ 2020-12-02  9:32 ` Luo Jiaxing
  2020-12-02 10:04   ` Andy Shevchenko
  2020-12-06 23:12   ` Linus Walleij
  2020-12-02  9:32 ` [PATCH v1 2/3] MAINTAINERS: Add maintainer for HiSilicon GPIO driver Luo Jiaxing
  2020-12-02  9:32 ` [PATCH v1 3/3] arm64: defconfig: enable GPIO_HISI Luo Jiaxing
  2 siblings, 2 replies; 9+ messages in thread
From: Luo Jiaxing @ 2020-12-02  9:32 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, catalin.marinas, will
  Cc: andy.shevchenko, andriy.shevchenko, linux-gpio, linux-kernel, linuxarm

This GPIO driver is for HiSilicon's ARM SoC.

HiSilicon's GPIO controller support double-edge interrupt and multi-core
concurrent access.

ACPI table example for this GPIO controller:
Device (GPO0)
{
	Name (_HID, "HISI0184")
	Device (PRTA)
	{
		Name (_ADR, Zero)
		Name (_UID, Zero)
		Name (_DSD, Package (0x01)
		{
			Package (0x02)
			{
				"hisi-ngpio",
				0x20
			}
		})
	}
}

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
---
 drivers/gpio/Kconfig     |  11 ++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-hisi.c | 356 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 368 insertions(+)
 create mode 100644 drivers/gpio/gpio-hisi.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5d4de5c..0f5d4c6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -296,6 +296,17 @@ config GPIO_GRGPIO
 	  Select this to support Aeroflex Gaisler GRGPIO cores from the GRLIB
 	  VHDL IP core library.
 
+config GPIO_HISI
+	tristate "HISILICON GPIO controller driver"
+	depends on (ARM64 && ACPI) || COMPILE_TEST
+	select GPIO_GENERIC
+	select GENERIC_IRQ_CHIP
+	help
+	  Say Y or M here to build support for the HiSilicon GPIO controller driver
+	  GPIO block.
+	  This controller support double-edge interrupt and multi-core concurrent
+	  access.
+
 config GPIO_HLWD
 	tristate "Nintendo Wii (Hollywood) GPIO"
 	depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 09dada8..260ae25 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_GPIO_GE_FPGA)		+= gpio-ge.o
 obj-$(CONFIG_GPIO_GPIO_MM)		+= gpio-gpio-mm.o
 obj-$(CONFIG_GPIO_GRGPIO)		+= gpio-grgpio.o
 obj-$(CONFIG_GPIO_GW_PLD)		+= gpio-gw-pld.o
+obj-$(CONFIG_GPIO_HISI)                 += gpio-hisi.o
 obj-$(CONFIG_GPIO_HLWD)			+= gpio-hlwd.o
 obj-$(CONFIG_HTC_EGPIO)			+= gpio-htc-egpio.o
 obj-$(CONFIG_GPIO_ICH)			+= gpio-ich.o
diff --git a/drivers/gpio/gpio-hisi.c b/drivers/gpio/gpio-hisi.c
new file mode 100644
index 0000000..ab076ca
--- /dev/null
+++ b/drivers/gpio/gpio-hisi.c
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020 HiSilicon Limited.
+ */
+#include <linux/acpi.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+#include "gpiolib.h"
+#include "gpiolib-acpi.h"
+
+#define HISI_GPIO_SWPORT_DR_SET_WX	0x0
+#define HISI_GPIO_SWPORT_DR_CLR_WX	0x4
+#define HISI_GPIO_SWPORT_DDR_SET_WX	0x10
+#define HISI_GPIO_SWPORT_DDR_CLR_WX	0x14
+#define HISI_GPIO_SWPORT_DDR_ST_WX	0x18
+#define HISI_GPIO_INTEN_SET_WX		0x20
+#define HISI_GPIO_INTEN_CLR_WX		0x24
+#define HISI_GPIO_INTMASK_SET_WX	0x30
+#define HISI_GPIO_INTMASK_CLR_WX	0x34
+#define HISI_GPIO_INTTYPE_EDGE_SET_WX	0x40
+#define HISI_GPIO_INTTYPE_EDGE_CLR_WX	0x44
+#define HISI_GPIO_INT_POLARITY_SET_WX	0x50
+#define HISI_GPIO_INT_POLARITY_CLR_WX	0x54
+#define HISI_GPIO_DEBOUNCE_SET_WX	0x60
+#define HISI_GPIO_DEBOUNCE_CLR_WX	0x64
+#define HISI_GPIO_INTSTATUS_WX		0x70
+#define HISI_GPIO_PORTA_EOI_WX		0x78
+#define HISI_GPIO_EXT_PORT_WX		0x80
+#define HISI_GPIO_INTCOMB_MASK_WX	0xa0
+#define HISI_GPIO_INT_DEDGE_SET		0xb0
+#define HISI_GPIO_INT_DEDGE_CLR		0xb4
+#define HISI_GPIO_INT_DEDGE_ST		0xb8
+
+#define HISI_GPIO_REG_SIZE	0x4
+#define HISI_GPIO_REG_MAX	0x100
+#define HISI_GPIO_PIN_NUM_MAX 32
+
+#define HISI_GPIO_DRIVER_NAME	"gpio-hisi"
+
+struct hisi_gpio {
+	struct device		*dev;
+	void __iomem		*reg_base;
+	unsigned int		pin_num;
+	struct gpio_chip	chip;
+	struct irq_chip		irq_chip;
+	int			irq;
+};
+
+static inline u32 hisi_gpio_read_reg(struct gpio_chip *chip,
+				     unsigned int off)
+{
+	struct hisi_gpio *hisi_gpio =
+			container_of(chip, struct hisi_gpio, chip);
+
+	return chip->read_reg(hisi_gpio->reg_base + off);
+}
+
+static inline void hisi_gpio_write_reg(struct gpio_chip *chip,
+				       unsigned int off, u32 val)
+{
+	struct hisi_gpio *hisi_gpio =
+			container_of(chip, struct hisi_gpio, chip);
+
+	chip->write_reg(hisi_gpio->reg_base + off, val);
+}
+
+static void hisi_gpio_set_debounce(struct gpio_chip *chip, unsigned int off,
+				   u32 debounce)
+{
+	unsigned long mask = BIT(off);
+
+	if (debounce)
+		hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_SET_WX, mask);
+	else
+		hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_CLR_WX, mask);
+}
+
+static int hisi_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+				unsigned long config)
+{
+	u32 config_para = pinconf_to_config_param(config);
+	u32 config_arg;
+
+	switch (config_para) {
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		config_arg = pinconf_to_config_argument(config);
+		hisi_gpio_set_debounce(chip, offset, config_arg);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int hisi_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
+{
+	/* Return 0 if output, 1 if input */
+	if (hisi_gpio_read_reg(chip, HISI_GPIO_SWPORT_DDR_ST_WX) & BIT(gpio))
+		return GPIO_LINE_DIRECTION_OUT;
+	return GPIO_LINE_DIRECTION_IN;
+}
+
+static int hisi_gpio_direction_output(struct gpio_chip *chip,
+				      unsigned int gpio,
+				      int val)
+{
+	hisi_gpio_write_reg(chip, HISI_GPIO_SWPORT_DDR_SET_WX, BIT(gpio));
+	chip->set(chip, gpio, val);
+	return 0;
+}
+
+static int hisi_gpio_direction_input(struct gpio_chip *chip,
+				     unsigned int gpio)
+{
+	hisi_gpio_write_reg(chip, HISI_GPIO_SWPORT_DDR_CLR_WX, BIT(gpio));
+	return 0;
+}
+
+void hisi_gpio_set_ack(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	hisi_gpio_write_reg(chip, HISI_GPIO_PORTA_EOI_WX, BIT(irqd_to_hwirq(d)));
+}
+
+static void hisi_gpio_irq_set_mask(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	hisi_gpio_write_reg(chip, HISI_GPIO_INTMASK_SET_WX, BIT(irqd_to_hwirq(d)));
+}
+
+static void hisi_gpio_irq_clr_mask(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	hisi_gpio_write_reg(chip, HISI_GPIO_INTMASK_CLR_WX, BIT(irqd_to_hwirq(d)));
+}
+
+static int hisi_gpio_irq_set_type(struct irq_data *d, u32 type)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int mask = BIT(irqd_to_hwirq(d));
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_BOTH:
+		hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_SET, mask);
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
+		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
+		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
+		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
+		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * The dual-edge interrupt and other interrupt's registers do not
+	 * take effect at the same time. The registers of the two-edge
+	 * interrupts have higher priorities, the configuration of
+	 * the dual-edge interrupts must be disabled before the configuration
+	 * of other kind of interrupts.
+	 */
+	if (type != IRQ_TYPE_EDGE_BOTH) {
+		unsigned int both = hisi_gpio_read_reg(chip, HISI_GPIO_INT_DEDGE_ST);
+
+		if (both & mask)
+			hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_CLR, mask);
+	}
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		irq_set_handler_locked(d, handle_level_irq);
+	else if (type & IRQ_TYPE_EDGE_BOTH)
+		irq_set_handler_locked(d, handle_edge_irq);
+
+	return 0;
+}
+
+static void hisi_gpio_irq_enable(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	hisi_gpio_irq_clr_mask(d);
+	hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_SET_WX, BIT(irqd_to_hwirq(d)));
+}
+
+static void hisi_gpio_irq_disable(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	hisi_gpio_irq_set_mask(d);
+	hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_CLR_WX, BIT(irqd_to_hwirq(d)));
+}
+
+static void hisi_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct hisi_gpio *hisi_gpio = irq_desc_get_handler_data(desc);
+	u32 irq_msk = hisi_gpio_read_reg(&hisi_gpio->chip,
+					 HISI_GPIO_INTSTATUS_WX);
+	struct irq_chip *irq_c = irq_desc_get_chip(desc);
+
+	chained_irq_enter(irq_c, desc);
+	while (irq_msk) {
+		int hwirq = fls(irq_msk) - 1;
+		int gpio_irq = irq_find_mapping(hisi_gpio->chip.irq.domain,
+						hwirq);
+
+		generic_handle_irq(gpio_irq);
+		irq_msk &= ~BIT(hwirq);
+	}
+	chained_irq_exit(irq_c, desc);
+}
+
+static void hisi_gpio_init_irq(struct hisi_gpio *hisi_gpio)
+{
+	struct gpio_chip *chip = &hisi_gpio->chip;
+	struct gpio_irq_chip *girq_chip = &chip->irq;
+
+	/* Set hooks for irq_chip */
+	hisi_gpio->irq_chip.irq_ack = hisi_gpio_set_ack;
+	hisi_gpio->irq_chip.irq_mask = hisi_gpio_irq_set_mask;
+	hisi_gpio->irq_chip.irq_unmask = hisi_gpio_irq_clr_mask;
+	hisi_gpio->irq_chip.irq_set_type = hisi_gpio_irq_set_type;
+	hisi_gpio->irq_chip.irq_enable = hisi_gpio_irq_enable;
+	hisi_gpio->irq_chip.irq_disable = hisi_gpio_irq_disable;
+
+	girq_chip->chip = &hisi_gpio->irq_chip;
+	girq_chip->default_type = IRQ_TYPE_NONE;
+	girq_chip->num_parents = 1;
+	girq_chip->parents = &hisi_gpio->irq;
+	girq_chip->parent_handler = hisi_gpio_irq_handler;
+	girq_chip->parent_handler_data = hisi_gpio;
+
+	/* Clear Mask of GPIO controller combine IRQ */
+	hisi_gpio_write_reg(chip, HISI_GPIO_INTCOMB_MASK_WX, 1);
+}
+
+static const struct acpi_device_id hisi_gpio_acpi_match[] = {
+	{"HISI0184", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, hisi_gpio_acpi_match);
+
+static void hisi_gpio_get_pdata(struct device *dev,
+				struct hisi_gpio *hisi_gpio)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct fwnode_handle *fwnode;
+	int idx = 0;
+
+	device_for_each_child_node(dev, fwnode)  {
+		if (fwnode_property_read_u32(fwnode, "hisi-ngpio",
+					     &hisi_gpio->pin_num)) {
+			dev_err(dev,
+				"failed to get number of gpios for port%d and use default value instead\n",
+				idx);
+			hisi_gpio->pin_num = HISI_GPIO_PIN_NUM_MAX;
+		}
+
+		if (hisi_gpio->pin_num > HISI_GPIO_PIN_NUM_MAX)
+			hisi_gpio->pin_num = HISI_GPIO_PIN_NUM_MAX;
+
+		hisi_gpio->irq = platform_get_irq(pdev, idx);
+
+		dev_info(dev,
+			 "get hisi_gpio[%d] with %d gpios and irq[%d]\n", idx,
+			 hisi_gpio->pin_num, hisi_gpio->irq);
+
+		idx++;
+	}
+}
+
+static int hisi_gpio_probe(struct platform_device *platform_dev)
+{
+	struct device *dev = &platform_dev->dev;
+	void __iomem *dat, *set, *clr;
+	struct hisi_gpio *hisi_gpio;
+	int port_num;
+	int res;
+
+	/* One HiSilicon GPIO controller own one port with 32 pins */
+	port_num = device_get_child_node_count(dev);
+	if (port_num != 1)
+		return -ENODEV;
+
+	hisi_gpio = devm_kzalloc(dev, sizeof(*hisi_gpio), GFP_KERNEL);
+	if (!hisi_gpio)
+		return -ENOMEM;
+
+	hisi_gpio->reg_base = devm_platform_ioremap_resource(platform_dev, 0);
+	if (IS_ERR(hisi_gpio->reg_base))
+		return PTR_ERR(hisi_gpio->reg_base);
+
+	hisi_gpio_get_pdata(dev, hisi_gpio);
+
+	hisi_gpio->dev = dev;
+
+	dat = hisi_gpio->reg_base + HISI_GPIO_EXT_PORT_WX;
+	set = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_SET_WX;
+	clr = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_CLR_WX;
+
+	res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE, dat, set,
+			 clr, NULL, NULL, 0);
+	if (res) {
+		dev_err(dev, "failed to init\n");
+		return res;
+	}
+
+	hisi_gpio->chip.direction_output = hisi_gpio_direction_output;
+	hisi_gpio->chip.direction_input = hisi_gpio_direction_input;
+	hisi_gpio->chip.get_direction = hisi_gpio_get_direction;
+	hisi_gpio->chip.set_config = hisi_gpio_set_config;
+	hisi_gpio->chip.ngpio = hisi_gpio->pin_num;
+	hisi_gpio->chip.base = -1;
+
+	if (hisi_gpio->irq > 0)
+		hisi_gpio_init_irq(hisi_gpio);
+
+	res = devm_gpiochip_add_data(dev, &hisi_gpio->chip, hisi_gpio);
+	if (res) {
+		dev_err(dev, "failed to register gpiochip\n");
+		return res;
+	}
+
+	return 0;
+}
+
+static struct platform_driver hisi_gpio_driver = {
+	.driver		= {
+		.name	= HISI_GPIO_DRIVER_NAME,
+		.acpi_match_table = ACPI_PTR(hisi_gpio_acpi_match),
+	},
+	.probe		= hisi_gpio_probe,
+};
+
+module_platform_driver(hisi_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Luo Jiaxing <luojiaxing@huawei.com>");
+MODULE_DESCRIPTION("HiSilicon GPIO controller driver");
+MODULE_ALIAS("platform:" HISI_GPIO_DRIVER_NAME);
-- 
2.7.4


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

* [PATCH v1 2/3] MAINTAINERS: Add maintainer for HiSilicon GPIO driver
  2020-12-02  9:32 [PATCH v1 0/3] gpio: gpio-hisi: Add HiSilicon GPIO support Luo Jiaxing
  2020-12-02  9:32 ` [PATCH v1 1/3] " Luo Jiaxing
@ 2020-12-02  9:32 ` Luo Jiaxing
  2020-12-02  9:32 ` [PATCH v1 3/3] arm64: defconfig: enable GPIO_HISI Luo Jiaxing
  2 siblings, 0 replies; 9+ messages in thread
From: Luo Jiaxing @ 2020-12-02  9:32 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, catalin.marinas, will
  Cc: andy.shevchenko, andriy.shevchenko, linux-gpio, linux-kernel, linuxarm

Here add maintainer information for HiSilicon GPIO driver.

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2daa6ee..8d13419a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7896,6 +7896,13 @@ L:	dmaengine@vger.kernel.org
 S:	Maintained
 F:	drivers/dma/hisi_dma.c
 
+HISILICON GPIO DRIVER
+M:	Luo Jiaxing <luojiaxing@huawei.com>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	drivers/gpio/gpio-hisi.c
+F:	include/linux/platform_data/gpio-hisi.h
+
 HISILICON HIGH PERFORMANCE RSA ENGINE DRIVER (HPRE)
 M:	Zaibo Xu <xuzaibo@huawei.com>
 L:	linux-crypto@vger.kernel.org
-- 
2.7.4


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

* [PATCH v1 3/3] arm64: defconfig: enable GPIO_HISI
  2020-12-02  9:32 [PATCH v1 0/3] gpio: gpio-hisi: Add HiSilicon GPIO support Luo Jiaxing
  2020-12-02  9:32 ` [PATCH v1 1/3] " Luo Jiaxing
  2020-12-02  9:32 ` [PATCH v1 2/3] MAINTAINERS: Add maintainer for HiSilicon GPIO driver Luo Jiaxing
@ 2020-12-02  9:32 ` Luo Jiaxing
  2 siblings, 0 replies; 9+ messages in thread
From: Luo Jiaxing @ 2020-12-02  9:32 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, catalin.marinas, will
  Cc: andy.shevchenko, andriy.shevchenko, linux-gpio, linux-kernel, linuxarm

Enable GPIO controller for HiSilicon's ARM SoC.

GPIO is common driver for HiSilicon's ARM SoC and it provide support for
some function of I2C and SPI.

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 5cfe3cf..b5cdf5e 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -498,6 +498,7 @@ CONFIG_PINCTRL_SM8150=y
 CONFIG_PINCTRL_SM8250=y
 CONFIG_GPIO_ALTERA=m
 CONFIG_GPIO_DWAPB=y
+CONFIG_GPIO_HISI=y
 CONFIG_GPIO_MB86S7X=y
 CONFIG_GPIO_MPC8XXX=y
 CONFIG_GPIO_MXC=y
-- 
2.7.4


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

* Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
  2020-12-02  9:32 ` [PATCH v1 1/3] " Luo Jiaxing
@ 2020-12-02 10:04   ` Andy Shevchenko
  2020-12-04  9:38     ` luojiaxing
  2020-12-06 23:12   ` Linus Walleij
  1 sibling, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-12-02 10:04 UTC (permalink / raw)
  To: Luo Jiaxing
  Cc: linus.walleij, bgolaszewski, catalin.marinas, will, linux-gpio,
	linux-kernel, linuxarm

On Wed, Dec 02, 2020 at 05:32:21PM +0800, Luo Jiaxing wrote:
> This GPIO driver is for HiSilicon's ARM SoC.
> 
> HiSilicon's GPIO controller support double-edge interrupt and multi-core
> concurrent access.
> 
> ACPI table example for this GPIO controller:
> Device (GPO0)
> {
> 	Name (_HID, "HISI0184")
> 	Device (PRTA)
> 	{
> 		Name (_ADR, Zero)
> 		Name (_UID, Zero)
> 		Name (_DSD, Package (0x01)
> 		{
> 			Package (0x02)
> 			{
> 				"hisi-ngpio",

Can it be standard property?
Please, fix firmware.

> 				0x20
> 			}
> 		})
> 	}
> }

...

> +config GPIO_HISI
> +	tristate "HISILICON GPIO controller driver"

> +	depends on (ARM64 && ACPI) || COMPILE_TEST

This is wrong. (Homework to understand why. Also see below)

> +	select GPIO_GENERIC
> +	select GENERIC_IRQ_CHIP
> +	help
> +	  Say Y or M here to build support for the HiSilicon GPIO controller driver
> +	  GPIO block.
> +	  This controller support double-edge interrupt and multi-core concurrent
> +	  access.

No module name?

...

> +/*
> + * Copyright (c) 2020 HiSilicon Limited.
> + */

One line.

...

> +#include <linux/acpi.h>

Don't see user of it (but see above and below as well).
At the same time missed mod_devicetable.h.

> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>

...

> +#include "gpiolib.h"

> +#include "gpiolib-acpi.h"

Any user of this?

...

> +#define HISI_GPIO_SWPORT_DR_SET_WX	0x0
	...
> +#define HISI_GPIO_INT_DEDGE_SET		0xb0
	...
> +#define HISI_GPIO_REG_MAX	0x100

Use fixed width for register offsets, like:
	0x000
	...
	0x0b0
	...
	0x100

...

> +struct hisi_gpio {
> +	struct device		*dev;
> +	void __iomem		*reg_base;
> +	unsigned int		pin_num;

> +	struct gpio_chip	chip;

Moving this to be a first member of the struct will make corresponding
container_of() no-op.

> +	struct irq_chip		irq_chip;
> +	int			irq;
> +};

...

> +	unsigned long mask = BIT(off);

No need to have temporary variable. Use directly BIT(off) which fits into 80.

> +
> +	if (debounce)
> +		hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_SET_WX, mask);
> +	else
> +		hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_CLR_WX, mask);

...

> +	switch (config_para) {
> +	case PIN_CONFIG_INPUT_DEBOUNCE:
> +		config_arg = pinconf_to_config_argument(config);
> +		hisi_gpio_set_debounce(chip, offset, config_arg);

> +		break;

Move...

> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	return 0;

...this above.

...

> +	/* Return 0 if output, 1 if input */

Useless comment.

...

> +static int hisi_gpio_irq_set_type(struct irq_data *d, u32 type)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +	unsigned int mask = BIT(irqd_to_hwirq(d));
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_BOTH:
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_SET, mask);
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * The dual-edge interrupt and other interrupt's registers do not
> +	 * take effect at the same time. The registers of the two-edge
> +	 * interrupts have higher priorities, the configuration of
> +	 * the dual-edge interrupts must be disabled before the configuration
> +	 * of other kind of interrupts.
> +	 */

This comment sounds like below should be moved before switch-case. Can you elaborate?

> +	if (type != IRQ_TYPE_EDGE_BOTH) {
> +		unsigned int both = hisi_gpio_read_reg(chip, HISI_GPIO_INT_DEDGE_ST);
> +
> +		if (both & mask)
> +			hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_CLR, mask);
> +	}
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		irq_set_handler_locked(d, handle_level_irq);
> +	else if (type & IRQ_TYPE_EDGE_BOTH)
> +		irq_set_handler_locked(d, handle_edge_irq);
> +
> +	return 0;
> +}

...

> +	while (irq_msk) {
> +		int hwirq = fls(irq_msk) - 1;

> +		irq_msk &= ~BIT(hwirq);
> +	}

NIH of for_each_set_bit().

...

> +	res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE, dat, set,
> +			 clr, NULL, NULL, 0);
> +	if (res) {
> +		dev_err(dev, "failed to init\n");
> +		return res;
> +	}

Wondering if you can use regmap GPIO.

...

> +static struct platform_driver hisi_gpio_driver = {
> +	.driver		= {
> +		.name	= HISI_GPIO_DRIVER_NAME,

> +		.acpi_match_table = ACPI_PTR(hisi_gpio_acpi_match),

This is wrong. If you use COMPILE_TEST the ACPI_PTR in !ACPI case is no op.
Compiler will warn you about unused variable. Have you compile tested it in
such conditions?

Hint: remove ACPI_PTR(). In 99% this macro shouldn't be used.

> +	},
> +	.probe		= hisi_gpio_probe,
> +};

> +

Redundant blank line.

> +module_platform_driver(hisi_gpio_driver);

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
  2020-12-02 10:04   ` Andy Shevchenko
@ 2020-12-04  9:38     ` luojiaxing
  0 siblings, 0 replies; 9+ messages in thread
From: luojiaxing @ 2020-12-04  9:38 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linus.walleij, bgolaszewski, catalin.marinas, will, linux-gpio,
	linux-kernel, linuxarm

Hi

On 2020/12/2 18:04, Andy Shevchenko wrote:
> On Wed, Dec 02, 2020 at 05:32:21PM +0800, Luo Jiaxing wrote:
>> This GPIO driver is for HiSilicon's ARM SoC.
>>
>> HiSilicon's GPIO controller support double-edge interrupt and multi-core
>> concurrent access.
>>
>> ACPI table example for this GPIO controller:
>> Device (GPO0)
>> {
>> 	Name (_HID, "HISI0184")
>> 	Device (PRTA)
>> 	{
>> 		Name (_ADR, Zero)
>> 		Name (_UID, Zero)
>> 		Name (_DSD, Package (0x01)
>> 		{
>> 			Package (0x02)
>> 			{
>> 				"hisi-ngpio",
> Can it be standard property?


sure, I think you mean that "ngpios" should be used here.


> Please, fix firmware.
>
>> 				0x20
>> 			}
>> 		})
>> 	}
>> }
> ...
>
>> +config GPIO_HISI
>> +	tristate "HISILICON GPIO controller driver"
>> +	depends on (ARM64 && ACPI) || COMPILE_TEST
> This is wrong. (Homework to understand why. Also see below)]


I think it should be

depends on (ARM64 || COMPILE_TEST) && ACPI


>
>> +	select GPIO_GENERIC
>> +	select GENERIC_IRQ_CHIP
>> +	help
>> +	  Say Y or M here to build support for the HiSilicon GPIO controller driver
>> +	  GPIO block.
>> +	  This controller support double-edge interrupt and multi-core concurrent
>> +	  access.
> No module name?


sorry, I didn't get what you mean. What module name should I add here?


>
> ...
>
>> +/*
>> + * Copyright (c) 2020 HiSilicon Limited.
>> + */
> One line.


ok


>
> ...
>
>> +#include <linux/acpi.h>
> Don't see user of it (but see above and below as well).
> At the same time missed mod_devicetable.h.


sure, let me check it.


>> +#include <linux/gpio/driver.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
> ...
>
>> +#include "gpiolib.h"
>> +#include "gpiolib-acpi.h"
> Any user of this?


This should be deleted, I used to use 
acpi_gpiochip_request_interrupts(), but delete it later.


>
> ...
>
>> +#define HISI_GPIO_SWPORT_DR_SET_WX	0x0
> 	...
>> +#define HISI_GPIO_INT_DEDGE_SET		0xb0
> 	...
>> +#define HISI_GPIO_REG_MAX	0x100
> Use fixed width for register offsets, like:
> 	0x000
> 	...
> 	0x0b0
> 	...
> 	0x100


ok


> ...
>
>> +struct hisi_gpio {
>> +	struct device		*dev;
>> +	void __iomem		*reg_base;
>> +	unsigned int		pin_num;
>> +	struct gpio_chip	chip;
> Moving this to be a first member of the struct will make corresponding
> container_of() no-op.


sure


>
>> +	struct irq_chip		irq_chip;
>> +	int			irq;
>> +};
> ...
>
>> +	unsigned long mask = BIT(off);
> No need to have temporary variable. Use directly BIT(off) which fits into 80.


sure


>> +
>> +	if (debounce)
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_SET_WX, mask);
>> +	else
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_CLR_WX, mask);
> ...
>
>> +	switch (config_para) {
>> +	case PIN_CONFIG_INPUT_DEBOUNCE:
>> +		config_arg = pinconf_to_config_argument(config);
>> +		hisi_gpio_set_debounce(chip, offset, config_arg);
>> +		break;
> Move...
>
>> +	default:
>> +		return -ENOTSUPP;
>> +	}
>> +
>> +	return 0;
> ...this above.


Sorry, what do you mean by Move ... this above?


>
> ...
>
>> +	/* Return 0 if output, 1 if input */
> Useless comment.


will be deleted


>
> ...
>
>> +static int hisi_gpio_irq_set_type(struct irq_data *d, u32 type)
>> +{
>> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +	unsigned int mask = BIT(irqd_to_hwirq(d));
>> +
>> +	switch (type) {
>> +	case IRQ_TYPE_EDGE_BOTH:
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_SET, mask);
>> +		break;
>> +	case IRQ_TYPE_EDGE_RISING:
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
>> +		break;
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
>> +		break;
>> +	case IRQ_TYPE_LEVEL_HIGH:
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
>> +		break;
>> +	case IRQ_TYPE_LEVEL_LOW:
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
>> +		hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * The dual-edge interrupt and other interrupt's registers do not
>> +	 * take effect at the same time. The registers of the two-edge
>> +	 * interrupts have higher priorities, the configuration of
>> +	 * the dual-edge interrupts must be disabled before the configuration
>> +	 * of other kind of interrupts.
>> +	 */
> This comment sounds like below should be moved before switch-case. Can you elaborate?


Our GPIO controller uses two separate registers to enable/disable the 
double-edge interrupt,

and once enable double-edge, the setting of edge and polarity register() 
will be ignored by the hardware.

Therefore, each time we update trigger type, an extra check is required: 
if the interrupt is not double-edges, ensure that it is disabled.


>
>> +	if (type != IRQ_TYPE_EDGE_BOTH) {
>> +		unsigned int both = hisi_gpio_read_reg(chip, HISI_GPIO_INT_DEDGE_ST);
>> +
>> +		if (both & mask)
>> +			hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_CLR, mask);
>> +	}
>> +
>> +	if (type & IRQ_TYPE_LEVEL_MASK)
>> +		irq_set_handler_locked(d, handle_level_irq);
>> +	else if (type & IRQ_TYPE_EDGE_BOTH)
>> +		irq_set_handler_locked(d, handle_edge_irq);
>> +
>> +	return 0;
>> +}
> ...
>
>> +	while (irq_msk) {
>> +		int hwirq = fls(irq_msk) - 1;
>> +		irq_msk &= ~BIT(hwirq);
>> +	}
> NIH of for_each_set_bit().


sure,  it's better than fls


>
> ...
>
>> +	res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE, dat, set,
>> +			 clr, NULL, NULL, 0);
>> +	if (res) {
>> +		dev_err(dev, "failed to init\n");
>> +		return res;
>> +	}
> Wondering if you can use regmap GPIO.


I looked at gpio-regmap.c, which is a newly uploaded feature. I think 
it's good, but I'd rather keep the current design.

Because it means my code needs to be modified and re-tested, this will 
take a while, but the actual business needs will not allow it.


>
> ...
>
>> +static struct platform_driver hisi_gpio_driver = {
>> +	.driver		= {
>> +		.name	= HISI_GPIO_DRIVER_NAME,
>> +		.acpi_match_table = ACPI_PTR(hisi_gpio_acpi_match),
> This is wrong. If you use COMPILE_TEST the ACPI_PTR in !ACPI case is no op.
> Compiler will warn you about unused variable. Have you compile tested it in
> such conditions?
>
> Hint: remove ACPI_PTR(). In 99% this macro shouldn't be used.


sure


>
>> +	},
>> +	.probe		= hisi_gpio_probe,
>> +};
>> +
> Redundant blank line.


sure


Thanks

Jiaxing


>
>> +module_platform_driver(hisi_gpio_driver);


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

* Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
  2020-12-02  9:32 ` [PATCH v1 1/3] " Luo Jiaxing
  2020-12-02 10:04   ` Andy Shevchenko
@ 2020-12-06 23:12   ` Linus Walleij
  2020-12-09  8:18     ` luojiaxing
  1 sibling, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2020-12-06 23:12 UTC (permalink / raw)
  To: Luo Jiaxing
  Cc: Bartosz Golaszewski, Catalin Marinas, Will Deacon,
	Andy Shevchenko, Andy Shevchenko, open list:GPIO SUBSYSTEM,
	linux-kernel, Linuxarm

Hi Luo!

thanks for your patch! I see that Andy already provided a crowd
of comments, here are some more!

On Wed, Dec 2, 2020 at 10:32 AM Luo Jiaxing <luojiaxing@huawei.com> wrote:

> +config GPIO_HISI
> +       tristate "HISILICON GPIO controller driver"
> +       depends on (ARM64 && ACPI) || COMPILE_TEST
> +       select GPIO_GENERIC

Thanks for using the generic driver!

> +#include <linux/acpi.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +#include "gpiolib.h"
> +#include "gpiolib-acpi.h"

No GPIO drivers should include these files. If you need
gpiolib.h then describe in a comment right above the include
why you have to do this.

I don't know about gpiolig-acpi.h but not unless  Andy
says it is OK it is not OK.

> +#define HISI_GPIO_SWPORT_DR_SET_WX     0x0
> +#define HISI_GPIO_SWPORT_DR_CLR_WX     0x4
> +#define HISI_GPIO_SWPORT_DDR_SET_WX    0x10
> +#define HISI_GPIO_SWPORT_DDR_CLR_WX    0x14
> +#define HISI_GPIO_SWPORT_DDR_ST_WX     0x18
> +#define HISI_GPIO_INTEN_SET_WX         0x20
> +#define HISI_GPIO_INTEN_CLR_WX         0x24
> +#define HISI_GPIO_INTMASK_SET_WX       0x30
> +#define HISI_GPIO_INTMASK_CLR_WX       0x34
> +#define HISI_GPIO_INTTYPE_EDGE_SET_WX  0x40
> +#define HISI_GPIO_INTTYPE_EDGE_CLR_WX  0x44
> +#define HISI_GPIO_INT_POLARITY_SET_WX  0x50
> +#define HISI_GPIO_INT_POLARITY_CLR_WX  0x54
> +#define HISI_GPIO_DEBOUNCE_SET_WX      0x60
> +#define HISI_GPIO_DEBOUNCE_CLR_WX      0x64
> +#define HISI_GPIO_INTSTATUS_WX         0x70
> +#define HISI_GPIO_PORTA_EOI_WX         0x78
> +#define HISI_GPIO_EXT_PORT_WX          0x80
> +#define HISI_GPIO_INTCOMB_MASK_WX      0xa0
> +#define HISI_GPIO_INT_DEDGE_SET                0xb0
> +#define HISI_GPIO_INT_DEDGE_CLR                0xb4
> +#define HISI_GPIO_INT_DEDGE_ST         0xb8

Nice idea with the double edge register! Hats off to the hardware
engineer who created this simple yet powerful GPIO.

> +#define HISI_GPIO_REG_SIZE     0x4
> +#define HISI_GPIO_REG_MAX      0x100
> +#define HISI_GPIO_PIN_NUM_MAX 32

This seems like a bit surplus definitions, I prefer to just inline
these. Some use cases will go away after you start using
bgpio_init().

> +struct hisi_gpio {
> +       struct device           *dev;
> +       void __iomem            *reg_base;
> +       unsigned int            pin_num;

I prefer "line_num", the reason I usually use the term "lines"
rather than "pins" is that some GPIO lines are internal in
chips and do not always go out to external pins.

> +       struct gpio_chip        chip;
> +       struct irq_chip         irq_chip;
> +       int                     irq;

Do you need to keep irq around in the state?

> +static inline u32 hisi_gpio_read_reg(struct gpio_chip *chip,
> +                                    unsigned int off)
> +{
> +       struct hisi_gpio *hisi_gpio =
> +                       container_of(chip, struct hisi_gpio, chip);
> +
> +       return chip->read_reg(hisi_gpio->reg_base + off);
> +}
> +
> +static inline void hisi_gpio_write_reg(struct gpio_chip *chip,
> +                                      unsigned int off, u32 val)
> +{
> +       struct hisi_gpio *hisi_gpio =
> +                       container_of(chip, struct hisi_gpio, chip);
> +
> +       chip->write_reg(hisi_gpio->reg_base + off, val);
> +}

OK it is a bit of reusing the register accessors inside the
GPIO chip generic MMIO abstraction, but to me it is really
better if you just address the registers directly.
The indirections through read_reg/write_reg doesn't
really buy you anything.

> +static void hisi_gpio_set_debounce(struct gpio_chip *chip, unsigned int off,
> +                                  u32 debounce)
> +{
> +       unsigned long mask = BIT(off);
> +
> +       if (debounce)
> +               hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_SET_WX, mask);
> +       else
> +               hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_CLR_WX, mask);
> +}

So debounce is just on/off? No ability to set "how much" or a timer?
Someone must be guessing inside the block that a certain number
of ms is perfect(TM) debouncing or is there a register for this that
you are not showing us? Like register 0x68 or 0x6c...

> +static int hisi_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
> +static int hisi_gpio_direction_output(struct gpio_chip *chip,
> +                                     unsigned int gpio,
> +                                     int val)
> +static int hisi_gpio_direction_input(struct gpio_chip *chip,
> +                                    unsigned int gpio)

These get replaced by the appropriate parameters to bgpio_init().
Read the doc in gpio-mmci.c above the function
bgpio_init() for help. If you still can't figure it out, ask and
describe your problem and we'll hash it out.

> +       /*
> +        * The dual-edge interrupt and other interrupt's registers do not
> +        * take effect at the same time. The registers of the two-edge
> +        * interrupts have higher priorities, the configuration of
> +        * the dual-edge interrupts must be disabled before the configuration
> +        * of other kind of interrupts.
> +        */
> +       if (type != IRQ_TYPE_EDGE_BOTH) {
> +               unsigned int both = hisi_gpio_read_reg(chip, HISI_GPIO_INT_DEDGE_ST);
> +
> +               if (both & mask)
> +                       hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_CLR, mask);
> +       }

Nice with this comment and overall the IRQ code looks very
good. Nice job!

> +static void hisi_gpio_irq_enable(struct irq_data *d)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +       hisi_gpio_irq_clr_mask(d);
> +       hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_SET_WX, BIT(irqd_to_hwirq(d)));
> +}
> +
> +static void hisi_gpio_irq_disable(struct irq_data *d)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +       hisi_gpio_irq_set_mask(d);
> +       hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_CLR_WX, BIT(irqd_to_hwirq(d)));
> +}

Interesting with a GPIO hardware that both as enable and mask
bits. I can't see why, usually they just have masks but I suppose
there is some reason.

> +static void hisi_gpio_irq_handler(struct irq_desc *desc)
> +{
> +       struct hisi_gpio *hisi_gpio = irq_desc_get_handler_data(desc);
> +       u32 irq_msk = hisi_gpio_read_reg(&hisi_gpio->chip,
> +                                        HISI_GPIO_INTSTATUS_WX);
> +       struct irq_chip *irq_c = irq_desc_get_chip(desc);
> +
> +       chained_irq_enter(irq_c, desc);
> +       while (irq_msk) {
> +               int hwirq = fls(irq_msk) - 1;
> +               int gpio_irq = irq_find_mapping(hisi_gpio->chip.irq.domain,
> +                                               hwirq);
> +
> +               generic_handle_irq(gpio_irq);
> +               irq_msk &= ~BIT(hwirq);
> +       }

What about just:

     for_each_set_bit(hwirq, &irq_msk, gc->ngpio)

generic_handle_irq(irq_find_mapping(hisi_gpio->chip.irq.domain,
                                                            hwirq));

> +       device_for_each_child_node(dev, fwnode)  {
> +               if (fwnode_property_read_u32(fwnode, "hisi-ngpio",
> +                                            &hisi_gpio->pin_num)) {
> +                       dev_err(dev,
> +                               "failed to get number of gpios for port%d and use default value instead\n",
> +                               idx);
> +                       hisi_gpio->pin_num = HISI_GPIO_PIN_NUM_MAX;
> +               }

Since the registers are only 32 bits suppose this should emit a
fat warning if line_num is > 32.

> +       dat = hisi_gpio->reg_base + HISI_GPIO_EXT_PORT_WX;
> +       set = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_SET_WX;
> +       clr = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_CLR_WX;
> +
> +       res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE, dat, set,
> +                        clr, NULL, NULL, 0);

That's a lot of variables for my taste, I usually just pass the registers
to the bgpio_init() call directly, split across a few lines, but it is a
matter of taste. Make sure you get the direction setting to be handled
by gpio-mmio as well as described above.

> +       hisi_gpio->chip.direction_output = hisi_gpio_direction_output;
> +       hisi_gpio->chip.direction_input = hisi_gpio_direction_input;
> +       hisi_gpio->chip.get_direction = hisi_gpio_get_direction;

So these should be handled by generic GPIO.

> +       hisi_gpio->chip.set_config = hisi_gpio_set_config;
> +       hisi_gpio->chip.ngpio = hisi_gpio->pin_num;
> +       hisi_gpio->chip.base = -1;

But these are fine.

Apart from this address everything Andy commented on as well.

Yours,
Linus Walleij

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

* Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
  2020-12-06 23:12   ` Linus Walleij
@ 2020-12-09  8:18     ` luojiaxing
  2020-12-09  9:03       ` Linus Walleij
  0 siblings, 1 reply; 9+ messages in thread
From: luojiaxing @ 2020-12-09  8:18 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Bartosz Golaszewski, Catalin Marinas, Will Deacon,
	Andy Shevchenko, Andy Shevchenko, open list:GPIO SUBSYSTEM,
	linux-kernel, Linuxarm


On 2020/12/7 7:12, Linus Walleij wrote:
> Hi Luo!
>
> thanks for your patch! I see that Andy already provided a crowd
> of comments, here are some more!
>
> On Wed, Dec 2, 2020 at 10:32 AM Luo Jiaxing <luojiaxing@huawei.com> wrote:
>
>> +config GPIO_HISI
>> +       tristate "HISILICON GPIO controller driver"
>> +       depends on (ARM64 && ACPI) || COMPILE_TEST
>> +       select GPIO_GENERIC
> Thanks for using the generic driver!
>
>> +#include <linux/acpi.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +
>> +#include "gpiolib.h"
>> +#include "gpiolib-acpi.h"
> No GPIO drivers should include these files. If you need
> gpiolib.h then describe in a comment right above the include
> why you have to do this.
>
> I don't know about gpiolig-acpi.h but not unless  Andy
> says it is OK it is not OK.


sure


>
>> +#define HISI_GPIO_SWPORT_DR_SET_WX     0x0
>> +#define HISI_GPIO_SWPORT_DR_CLR_WX     0x4
>> +#define HISI_GPIO_SWPORT_DDR_SET_WX    0x10
>> +#define HISI_GPIO_SWPORT_DDR_CLR_WX    0x14
>> +#define HISI_GPIO_SWPORT_DDR_ST_WX     0x18
>> +#define HISI_GPIO_INTEN_SET_WX         0x20
>> +#define HISI_GPIO_INTEN_CLR_WX         0x24
>> +#define HISI_GPIO_INTMASK_SET_WX       0x30
>> +#define HISI_GPIO_INTMASK_CLR_WX       0x34
>> +#define HISI_GPIO_INTTYPE_EDGE_SET_WX  0x40
>> +#define HISI_GPIO_INTTYPE_EDGE_CLR_WX  0x44
>> +#define HISI_GPIO_INT_POLARITY_SET_WX  0x50
>> +#define HISI_GPIO_INT_POLARITY_CLR_WX  0x54
>> +#define HISI_GPIO_DEBOUNCE_SET_WX      0x60
>> +#define HISI_GPIO_DEBOUNCE_CLR_WX      0x64
>> +#define HISI_GPIO_INTSTATUS_WX         0x70
>> +#define HISI_GPIO_PORTA_EOI_WX         0x78
>> +#define HISI_GPIO_EXT_PORT_WX          0x80
>> +#define HISI_GPIO_INTCOMB_MASK_WX      0xa0
>> +#define HISI_GPIO_INT_DEDGE_SET                0xb0
>> +#define HISI_GPIO_INT_DEDGE_CLR                0xb4
>> +#define HISI_GPIO_INT_DEDGE_ST         0xb8
> Nice idea with the double edge register! Hats off to the hardware
> engineer who created this simple yet powerful GPIO.
>
>> +#define HISI_GPIO_REG_SIZE     0x4
>> +#define HISI_GPIO_REG_MAX      0x100
>> +#define HISI_GPIO_PIN_NUM_MAX 32
> This seems like a bit surplus definitions, I prefer to just inline
> these. Some use cases will go away after you start using
> bgpio_init().


sure, will be set as inline


>
>> +struct hisi_gpio {
>> +       struct device           *dev;
>> +       void __iomem            *reg_base;
>> +       unsigned int            pin_num;
> I prefer "line_num", the reason I usually use the term "lines"
> rather than "pins" is that some GPIO lines are internal in
> chips and do not always go out to external pins.


yes, you are right.


>
>> +       struct gpio_chip        chip;
>> +       struct irq_chip         irq_chip;
>> +       int                     irq;
> Do you need to keep irq around in the state?


irq is used in several functions, such as hisi_gpio_init_irq and 
hisi_gpio_probe.

Although these functions can be combined, but the readability of the 
logic is affected.

Therefore, Irq needs to be saved.


>
>> +static inline u32 hisi_gpio_read_reg(struct gpio_chip *chip,
>> +                                    unsigned int off)
>> +{
>> +       struct hisi_gpio *hisi_gpio =
>> +                       container_of(chip, struct hisi_gpio, chip);
>> +
>> +       return chip->read_reg(hisi_gpio->reg_base + off);
>> +}
>> +
>> +static inline void hisi_gpio_write_reg(struct gpio_chip *chip,
>> +                                      unsigned int off, u32 val)
>> +{
>> +       struct hisi_gpio *hisi_gpio =
>> +                       container_of(chip, struct hisi_gpio, chip);
>> +
>> +       chip->write_reg(hisi_gpio->reg_base + off, val);
>> +}
> OK it is a bit of reusing the register accessors inside the
> GPIO chip generic MMIO abstraction, but to me it is really
> better if you just address the registers directly.
> The indirections through read_reg/write_reg doesn't
> really buy you anything.


yes, you are right, I can directly use readl() and writel() here


>
>> +static void hisi_gpio_set_debounce(struct gpio_chip *chip, unsigned int off,
>> +                                  u32 debounce)
>> +{
>> +       unsigned long mask = BIT(off);
>> +
>> +       if (debounce)
>> +               hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_SET_WX, mask);
>> +       else
>> +               hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_CLR_WX, mask);
>> +}
> So debounce is just on/off? No ability to set "how much" or a timer?
> Someone must be guessing inside the block that a certain number
> of ms is perfect(TM) debouncing or is there a register for this that
> you are not showing us? Like register 0x68 or 0x6c...


Yes. In our hardware design, debounce timer are fixed as several clock 
cycles。

and cannot be modified by the driver.


The 0x68 register is a status register, which is used to query whether 
debounce is enabled.

No drivers check status of debounce, so I didn't show it.


>
>> +static int hisi_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
>> +static int hisi_gpio_direction_output(struct gpio_chip *chip,
>> +                                     unsigned int gpio,
>> +                                     int val)
>> +static int hisi_gpio_direction_input(struct gpio_chip *chip,
>> +                                    unsigned int gpio)
> These get replaced by the appropriate parameters to bgpio_init().
> Read the doc in gpio-mmci.c above the function
> bgpio_init() for help. If you still can't figure it out, ask and
> describe your problem and we'll hash it out.


sure, will delete these three function.


>
>> +       /*
>> +        * The dual-edge interrupt and other interrupt's registers do not
>> +        * take effect at the same time. The registers of the two-edge
>> +        * interrupts have higher priorities, the configuration of
>> +        * the dual-edge interrupts must be disabled before the configuration
>> +        * of other kind of interrupts.
>> +        */
>> +       if (type != IRQ_TYPE_EDGE_BOTH) {
>> +               unsigned int both = hisi_gpio_read_reg(chip, HISI_GPIO_INT_DEDGE_ST);
>> +
>> +               if (both & mask)
>> +                       hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_CLR, mask);
>> +       }
> Nice with this comment and overall the IRQ code looks very
> good. Nice job!
>
>> +static void hisi_gpio_irq_enable(struct irq_data *d)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +       hisi_gpio_irq_clr_mask(d);
>> +       hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_SET_WX, BIT(irqd_to_hwirq(d)));
>> +}
>> +
>> +static void hisi_gpio_irq_disable(struct irq_data *d)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +       hisi_gpio_irq_set_mask(d);
>> +       hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_CLR_WX, BIT(irqd_to_hwirq(d)));
>> +}
> Interesting with a GPIO hardware that both as enable and mask
> bits. I can't see why, usually they just have masks but I suppose
> there is some reason.


I see gpio-dwapb.c distinguishes between enable and mask too.

In my opinion, enable indicates that the user uses the GPIO line as the 
interrupt trigger source,

and mask indicates that the user does not want to see an interrupts for 
a while.

The difference between the two types of flag is that interrupts 
generated during masking are recorded but not lost,

however, if interrupts are disabled, interrupts will lost.


>
>> +static void hisi_gpio_irq_handler(struct irq_desc *desc)
>> +{
>> +       struct hisi_gpio *hisi_gpio = irq_desc_get_handler_data(desc);
>> +       u32 irq_msk = hisi_gpio_read_reg(&hisi_gpio->chip,
>> +                                        HISI_GPIO_INTSTATUS_WX);
>> +       struct irq_chip *irq_c = irq_desc_get_chip(desc);
>> +
>> +       chained_irq_enter(irq_c, desc);
>> +       while (irq_msk) {
>> +               int hwirq = fls(irq_msk) - 1;
>> +               int gpio_irq = irq_find_mapping(hisi_gpio->chip.irq.domain,
>> +                                               hwirq);
>> +
>> +               generic_handle_irq(gpio_irq);
>> +               irq_msk &= ~BIT(hwirq);
>> +       }
> What about just:
>
>       for_each_set_bit(hwirq, &irq_msk, gc->ngpio)
>
> generic_handle_irq(irq_find_mapping(hisi_gpio->chip.irq.domain,
>                                                              hwirq));


sure


>
>> +       device_for_each_child_node(dev, fwnode)  {
>> +               if (fwnode_property_read_u32(fwnode, "hisi-ngpio",
>> +                                            &hisi_gpio->pin_num)) {
>> +                       dev_err(dev,
>> +                               "failed to get number of gpios for port%d and use default value instead\n",
>> +                               idx);
>> +                       hisi_gpio->pin_num = HISI_GPIO_PIN_NUM_MAX;
>> +               }
> Since the registers are only 32 bits suppose this should emit a
> fat warning if line_num is > 32.


ok


>
>> +       dat = hisi_gpio->reg_base + HISI_GPIO_EXT_PORT_WX;
>> +       set = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_SET_WX;
>> +       clr = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_CLR_WX;
>> +
>> +       res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE, dat, set,
>> +                        clr, NULL, NULL, 0);
> That's a lot of variables for my taste, I usually just pass the registers
> to the bgpio_init() call directly, split across a few lines, but it is a
> matter of taste. Make sure you get the direction setting to be handled
> by gpio-mmio as well as described above.


ok, you're recommending a more tidy way. I'll fix it.


>
>> +       hisi_gpio->chip.direction_output = hisi_gpio_direction_output;
>> +       hisi_gpio->chip.direction_input = hisi_gpio_direction_input;
>> +       hisi_gpio->chip.get_direction = hisi_gpio_get_direction;
> So these should be handled by generic GPIO.


sure


Thanks

Jiaxing


>
>> +       hisi_gpio->chip.set_config = hisi_gpio_set_config;
>> +       hisi_gpio->chip.ngpio = hisi_gpio->pin_num;
>> +       hisi_gpio->chip.base = -1;
> But these are fine.
>
> Apart from this address everything Andy commented on as well.
>
> Yours,
> Linus Walleij
>
> .
>


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

* Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
  2020-12-09  8:18     ` luojiaxing
@ 2020-12-09  9:03       ` Linus Walleij
  0 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2020-12-09  9:03 UTC (permalink / raw)
  To: luojiaxing
  Cc: Bartosz Golaszewski, Catalin Marinas, Will Deacon,
	Andy Shevchenko, Andy Shevchenko, open list:GPIO SUBSYSTEM,
	linux-kernel, Linuxarm

Hi Jiaxing,

thanks! waiting for the new patch!

On Wed, Dec 9, 2020 at 9:19 AM luojiaxing <luojiaxing@huawei.com> wrote:

> >> +static void hisi_gpio_irq_disable(struct irq_data *d)
> >> +{
> >> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> >> +
> >> +       hisi_gpio_irq_set_mask(d);
> >> +       hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_CLR_WX, BIT(irqd_to_hwirq(d)));
> >> +}
> >
> > Interesting with a GPIO hardware that both as enable and mask
> > bits. I can't see why, usually they just have masks but I suppose
> > there is some reason.
>
> I see gpio-dwapb.c distinguishes between enable and mask too.
>
> In my opinion, enable indicates that the user uses the GPIO line as the
> interrupt trigger source,
>
> and mask indicates that the user does not want to see an interrupts for
> a while.
>
> The difference between the two types of flag is that interrupts
> generated during masking are recorded but not lost,
>
> however, if interrupts are disabled, interrupts will lost.

Ah, that makes perfect sense! Thanks for explaining.

Yours,
Linus Walleij

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

end of thread, other threads:[~2020-12-09  9:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-02  9:32 [PATCH v1 0/3] gpio: gpio-hisi: Add HiSilicon GPIO support Luo Jiaxing
2020-12-02  9:32 ` [PATCH v1 1/3] " Luo Jiaxing
2020-12-02 10:04   ` Andy Shevchenko
2020-12-04  9:38     ` luojiaxing
2020-12-06 23:12   ` Linus Walleij
2020-12-09  8:18     ` luojiaxing
2020-12-09  9:03       ` Linus Walleij
2020-12-02  9:32 ` [PATCH v1 2/3] MAINTAINERS: Add maintainer for HiSilicon GPIO driver Luo Jiaxing
2020-12-02  9:32 ` [PATCH v1 3/3] arm64: defconfig: enable GPIO_HISI Luo Jiaxing

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).