diff mbox series

[1/2] gpio: Add driver for PC Engines APU2/APU3 GPIOs

Message ID 20180809130302.18588-2-fe@dev.tdt.de
State New
Headers show
Series [1/2] gpio: Add driver for PC Engines APU2/APU3 GPIOs | expand

Commit Message

Florian Eckert Aug. 9, 2018, 1:03 p.m. UTC
Add a new device driver "gpio-apu" which will handle the GPIOs on APU2
and APU3 devices from PC Engines.

APU2 (https://pcengines.ch/schema/apu2c.pdf page 7):
- G32 is "button_reset" connected to the smd-button on the frontpanel
- G50 is "mpcie2_reset" connected to mPCIe2 reset line
- G51 is "mpcie3_reset" connected to mPCIe3 reset line

APU3 (https://pcengines.ch/schema/apu3c.pdf page 7):
- G32 is "button_reset" connected to the smd-button on the frontpanel
- G50 is "mpcie2_reset" connected to mPCIe2 reset line
- G51 is "mpcie3_reset" connected to mPCIe3 reset line
- G33 is "simswap" connected to SIM switch IC to swap the SIM between mPCIe2
  and mPCIe3 slot

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/gpio/Kconfig    |   8 ++
 drivers/gpio/Makefile   |   1 +
 drivers/gpio/gpio-apu.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)
 create mode 100644 drivers/gpio/gpio-apu.c

Comments

Andy Shevchenko Aug. 9, 2018, 3:57 p.m. UTC | #1
On Thu, 2018-08-09 at 15:03 +0200, Florian Eckert wrote:
> Add a new device driver "gpio-apu" which will handle the GPIOs on APU2
> and APU3 devices from PC Engines.
> 
> APU2 (https://pcengines.ch/schema/apu2c.pdf page 7):
> - G32 is "button_reset" connected to the smd-button on the frontpanel
> - G50 is "mpcie2_reset" connected to mPCIe2 reset line
> - G51 is "mpcie3_reset" connected to mPCIe3 reset line
> 
> APU3 (https://pcengines.ch/schema/apu3c.pdf page 7):
> - G32 is "button_reset" connected to the smd-button on the frontpanel
> - G50 is "mpcie2_reset" connected to mPCIe2 reset line
> - G51 is "mpcie3_reset" connected to mPCIe3 reset line
> - G33 is "simswap" connected to SIM switch IC to swap the SIM between
> mPCIe2
>   and mPCIe3 slot

Thanks for an update, my comments below.

> +/* PC Engines APU2/APU3 GPIO device driver
> + *
> + * Copyright (C) 2018 Florian Eckert <fe@dev.tdt.de>
> + */

Comment style issue.

> +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
> +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)

I think it's still unanswered.

Why do you have this hardcoded? What the issue with your firmware?
Why it's not provided via ACPI?

> +struct apu_gpio_pdata {
> +	struct platform_device *pdev;
> +	struct gpio_chip *chip;
> +	unsigned long *offset;
> +	void __iomem **addr;

> +	int iosize; /* for devm_ioremap() */

Why?

> +	spinlock_t lock;
> +};
> +

> +static struct apu_gpio_pdata *apu_gpio;

Why this is global variable?

> +static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL, NULL,
> NULL};

Ditto.

> +static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL,
> NULL, NULL};

Ditto.

> +static int gpio_apu_get_dir(struct gpio_chip *chip, unsigned offset)
> +{
> +	u32 val;
> +
> +	spin_lock(&apu_gpio->lock);
> +
> +	val = ~ioread32(apu_gpio->addr[offset]);

> +	val = (val >> APU_GPIO_BIT_DIR) & 1;

It seems you ignored my previous comments.

Why this is under spin lock?

So, I stopped here and waiting for next one which answers above and
previous comments.
Linus Walleij Aug. 10, 2018, 9:27 p.m. UTC | #2
On Thu, Aug 9, 2018 at 3:03 PM Florian Eckert <fe@dev.tdt.de> wrote:

> +config GPIO_APU
> +       tristate "PC Engines APU2/APU3 GPIO support"
> +       depends on X86
> +       select GPIO_GENERIC

So

> +static int gpio_apu_dir_out(struct gpio_chip *chip, unsigned offset,
> +static int gpio_apu_get_data(struct gpio_chip *chip, unsigned offset)
> +static void gpio_apu_set_data(struct gpio_chip *chip, unsigned offset, int value)
(...)
> +       .get_direction = gpio_apu_get_dir,
> +       .direction_input = gpio_apu_dir_in,
> +       .direction_output = gpio_apu_dir_out,
> +       .get = gpio_apu_get_data,
> +       .set = gpio_apu_set_data,

Please make use of GPIO_GENERIC (bgpio_init())
as detailed in my first answer.

Use the spinlock in the gpio_chip when doing this.

Yours,
Linus Walleij
Florian Eckert Aug. 24, 2018, 6:49 a.m. UTC | #3
Hello Andy,

thanks for reviewing my driver and sorry for the delayed answer.

>> +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
>> +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
> 
> I think it's still unanswered.
> 
> Why do you have this hardcoded? What the issue with your firmware?
> Why it's not provided via ACPI?

As mentioned in my previews mail I have copied this from the leds-apu 
driver
under /drivers/leds/leds-apu.c

I have to look into what mean to get this via ACPI.

>> +static struct apu_gpio_pdata *apu_gpio;
> 
> Why this is global variable?

I have added the static identifier so it should only be seen in this 
modul.
Or what do you mean?

> 
>> +static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL, NULL,
>> NULL};
> 
> Ditto.

Do you mean that this should get declared dynamic witch devm_kzalloc?

> 
>> +static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL,
>> NULL, NULL};
> 
> Ditto.

Do you mean that this should get declared dynamic witch devm_kzalloc?

I have also moved the reset button definition from /arch/x86/platform as 
suggested to /drivers/platform/x86/amd-apu-rbtn.c

Flo
Andy Shevchenko Oct. 29, 2018, 3:02 p.m. UTC | #4
On Fri, Aug 24, 2018 at 08:49:27AM +0200, Florian Eckert wrote:
> Hello Andy,
> 
> thanks for reviewing my driver and sorry for the delayed answer.
> 
> > > +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
> > > +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
> > 
> > I think it's still unanswered.
> > 
> > Why do you have this hardcoded? What the issue with your firmware?
> > Why it's not provided via ACPI?
> 
> As mentioned in my previews mail I have copied this from the leds-apu driver
> under /drivers/leds/leds-apu.c
> 
> I have to look into what mean to get this via ACPI.
> 
> > > +static struct apu_gpio_pdata *apu_gpio;
> > 
> > Why this is global variable?
> 
> I have added the static identifier so it should only be seen in this modul.
> Or what do you mean?
> 
> > 
> > > +static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL, NULL,
> > > NULL};
> > 
> > Ditto.
> 
> Do you mean that this should get declared dynamic witch devm_kzalloc?

That is my question. Usually for such drivers we don't need a global variables
but rather some chunks of memory from a heap.

> > > +static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL,
> > > NULL, NULL};
> > 
> > Ditto.
> 
> Do you mean that this should get declared dynamic witch devm_kzalloc?
> 
> I have also moved the reset button definition from /arch/x86/platform as
> suggested to /drivers/platform/x86/amd-apu-rbtn.c

Good!
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 71c0ab46f216..e4adf61e4082 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -117,6 +117,14 @@  config GPIO_AMDPT
 	  driver for GPIO functionality on Promontory IOHub
 	  Require ACPI ASL code to enumerate as a platform device.
 
+config GPIO_APU
+	tristate "PC Engines APU2/APU3 GPIO support"
+	depends on X86
+	select GPIO_GENERIC
+	help
+	  Say Y here to support GPIO functionality on APU2/APU3 boards
+	  from PC Engines.
+
 config GPIO_ASPEED
 	tristate "Aspeed GPIO support"
 	depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1324c8f966a7..feea4effcf29 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -29,6 +29,7 @@  obj-$(CONFIG_GPIO_ALTERA)  	+= gpio-altera.o
 obj-$(CONFIG_GPIO_ALTERA_A10SR)	+= gpio-altera-a10sr.o
 obj-$(CONFIG_GPIO_AMD8111)	+= gpio-amd8111.o
 obj-$(CONFIG_GPIO_AMDPT)	+= gpio-amdpt.o
+obj-$(CONFIG_GPIO_APU)		+= gpio-apu.o
 obj-$(CONFIG_GPIO_ARIZONA)	+= gpio-arizona.o
 obj-$(CONFIG_GPIO_ATH79)	+= gpio-ath79.o
 obj-$(CONFIG_GPIO_ASPEED)	+= gpio-aspeed.o
diff --git a/drivers/gpio/gpio-apu.c b/drivers/gpio/gpio-apu.c
new file mode 100644
index 000000000000..57a201cfeb11
--- /dev/null
+++ b/drivers/gpio/gpio-apu.c
@@ -0,0 +1,309 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* PC Engines APU2/APU3 GPIO device driver
+ *
+ * Copyright (C) 2018 Florian Eckert <fe@dev.tdt.de>
+ */
+
+#include <linux/dmi.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define DEVNAME                "gpio-apu"
+
+#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
+#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
+#define APU_GPIO_BIT_RD        16
+#define APU_GPIO_BIT_WR        22
+#define APU_GPIO_BIT_DIR       23
+
+#define APU2_NUM_GPIO          3
+#define APU3_NUM_GPIO          4
+
+struct apu_gpio_pdata {
+	struct platform_device *pdev;
+	struct gpio_chip *chip;
+	unsigned long *offset;
+	void __iomem **addr;
+	int iosize; /* for devm_ioremap() */
+	spinlock_t lock;
+};
+
+static struct apu_gpio_pdata *apu_gpio;
+
+/* APU2 */
+static unsigned long apu2_gpio_offset[APU2_NUM_GPIO] = {
+	APU_FCH_GPIO_BASE + 89 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 66 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 67 * sizeof(u32),
+};
+static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL, NULL, NULL};
+static const char * const apu2_gpio_names[] = {
+	"button_reset",
+	"mpcie2_reset",
+	"mpcie3_reset",
+};
+
+/* APU3 */
+static unsigned long apu3_gpio_offset[APU3_NUM_GPIO] = {
+	APU_FCH_GPIO_BASE + 89 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 66 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 67 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 90 * sizeof(u32),
+};
+static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL, NULL, NULL};
+static const char * const apu3_gpio_names[] = {
+	"button_reset",
+	"mpcie2_reset",
+	"mpcie3_reset",
+	"simswap",
+};
+
+static int gpio_apu_get_dir(struct gpio_chip *chip, unsigned offset)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ~ioread32(apu_gpio->addr[offset]);
+	val = (val >> APU_GPIO_BIT_DIR) & 1;
+
+	spin_unlock(&apu_gpio->lock);
+
+	return val;
+}
+
+static int gpio_apu_dir_in(struct gpio_chip *chip, unsigned offset)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	val &= ~BIT(APU_GPIO_BIT_DIR);
+	iowrite32(val, apu_gpio->addr[offset]);
+
+	spin_unlock(&apu_gpio->lock);
+
+	return 0;
+}
+
+static int gpio_apu_dir_out(struct gpio_chip *chip, unsigned offset,
+		int value)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	val |= BIT(APU_GPIO_BIT_DIR);
+	iowrite32(val, apu_gpio->addr[offset]);
+
+	spin_unlock(&apu_gpio->lock);
+
+	return 0;
+}
+
+static int gpio_apu_get_data(struct gpio_chip *chip, unsigned offset)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	val = (val >> APU_GPIO_BIT_RD) & 1;
+
+	spin_unlock(&apu_gpio->lock);
+
+	return val;
+}
+
+static void gpio_apu_set_data(struct gpio_chip *chip, unsigned offset, int value)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	if (value)
+		val |= BIT(APU_GPIO_BIT_WR);
+	else
+		val &= ~BIT(APU_GPIO_BIT_WR);
+	iowrite32(val, apu_gpio->addr[offset]);
+
+	spin_unlock(&apu_gpio->lock);
+}
+
+static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
+	/* PC Engines APU2 with "Legacy" bios < 4.0.8 */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "APU2")
+		}
+	},
+	/* PC Engines APU2 with "Legacy" bios >= 4.0.8 */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu2")
+		}
+	},
+	/* PC Engines APU2 with "Mainline" bios */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu2")
+		}
+	},
+	/* PC Engines APU3 with "Legacy" bios < 4.0.8 */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "APU3")
+		}
+	},
+	/* PC Engines APU3 with "Legacy" bios >= 4.0.8 */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu3")
+		}
+	},
+	/* PC Engines APU3 with "Mainline" bios */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu3")
+		}
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
+
+static struct gpio_chip gpio_apu_chip = {
+	.label = "gpio-apu",
+	.owner = THIS_MODULE,
+	.base = 20,
+	.get_direction = gpio_apu_get_dir,
+	.direction_input = gpio_apu_dir_in,
+	.direction_output = gpio_apu_dir_out,
+	.get = gpio_apu_get_data,
+	.set = gpio_apu_set_data,
+};
+
+static int __init apu_gpio_probe(struct platform_device *pdev)
+{
+	int i;
+
+	apu_gpio = devm_kzalloc(&pdev->dev, sizeof(*apu_gpio), GFP_KERNEL);
+
+	if (!apu_gpio)
+		return -ENOMEM;
+
+	apu_gpio->pdev = pdev;
+	apu_gpio->chip = &gpio_apu_chip;
+	spin_lock_init(&apu_gpio->lock);
+
+	if (dmi_match(DMI_BOARD_NAME, "APU3") ||
+	    dmi_match(DMI_BOARD_NAME, "apu3") ||
+	    dmi_match(DMI_BOARD_NAME, "PC Engines apu3")) {
+		apu_gpio->offset = apu3_gpio_offset;
+		apu_gpio->addr = apu3_gpio_addr;
+		apu_gpio->iosize = sizeof(u32);
+		apu_gpio->chip->names = apu3_gpio_names;
+		apu_gpio->chip->ngpio = ARRAY_SIZE(apu3_gpio_offset);
+		for( i = 0; i < ARRAY_SIZE(apu3_gpio_offset); i++) {
+			apu3_gpio_addr[i] = devm_ioremap(&pdev->dev,
+					apu_gpio->offset[i], apu_gpio->iosize);
+			if (!apu3_gpio_addr[i]) {
+				return -ENOMEM;
+			}
+		}
+	} else if (dmi_match(DMI_BOARD_NAME, "APU2") ||
+		   dmi_match(DMI_BOARD_NAME, "apu2") ||
+		   dmi_match(DMI_BOARD_NAME, "PC Engines apu2")) {
+		apu_gpio->offset = apu2_gpio_offset;
+		apu_gpio->addr = apu2_gpio_addr;
+		apu_gpio->iosize = sizeof(u32);
+		apu_gpio->chip->names = apu2_gpio_names;
+		apu_gpio->chip->ngpio = ARRAY_SIZE(apu2_gpio_offset);
+		for( i = 0; i < ARRAY_SIZE(apu2_gpio_offset); i++) {
+			apu2_gpio_addr[i] = devm_ioremap(&pdev->dev,
+					apu_gpio->offset[i], apu_gpio->iosize);
+			if (!apu2_gpio_addr[i]) {
+				return -ENOMEM;
+			}
+		}
+	}
+
+	return devm_gpiochip_add_data(&pdev->dev, apu_gpio->chip, NULL);
+}
+
+static struct platform_driver apu_gpio_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+static int __init apu_gpio_init(void)
+{
+	struct platform_device *pdev;
+	int err;
+
+	if (!dmi_match(DMI_SYS_VENDOR, "PC Engines")) {
+		pr_err("No PC Engines board detected\n");
+		return -ENODEV;
+	}
+	if (!(dmi_match(DMI_PRODUCT_NAME, "APU2") ||
+	      dmi_match(DMI_PRODUCT_NAME, "apu2") ||
+	      dmi_match(DMI_PRODUCT_NAME, "PC Engines apu2") ||
+	      dmi_match(DMI_PRODUCT_NAME, "APU3") ||
+	      dmi_match(DMI_PRODUCT_NAME, "apu3") ||
+	      dmi_match(DMI_PRODUCT_NAME, "PC Engines apu3"))) {
+		pr_err("Unknown PC Engines board: %s\n",
+				dmi_get_system_info(DMI_PRODUCT_NAME));
+		return -ENODEV;
+	}
+
+	pdev = platform_device_register_simple(KBUILD_MODNAME, -1, NULL, 0);
+	if (IS_ERR(pdev)) {
+		pr_err("Device allocation failed\n");
+		return PTR_ERR(pdev);
+	}
+
+	err = platform_driver_probe(&apu_gpio_driver, apu_gpio_probe);
+	if (err) {
+		pr_err("Probe platform driver failed\n");
+		platform_device_unregister(pdev);
+	}
+
+	pr_info ("%s: APU2/3 GPIO driver module loaded\n", DEVNAME);
+
+	return err;
+}
+
+static void __exit apu_gpio_exit(void)
+{
+	gpiochip_remove(apu_gpio->chip);
+	platform_device_unregister(apu_gpio->pdev);
+	platform_driver_unregister(&apu_gpio_driver);
+	pr_info ("%s: APU2/3 GPIO driver module unloaded\n", DEVNAME);
+}
+
+module_init(apu_gpio_init);
+module_exit(apu_gpio_exit);
+
+MODULE_AUTHOR("Florian Eckert");
+MODULE_DESCRIPTION("PC Engines APU2/3 family GPIO driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:gpio_apu");