[PATCH] gpio: Add Intel gpio controller support

[PATCH] gpio: Add Intel gpio controller support

[Tomas Marek]

Signed-off-by: Tomas Marek <tomas.marek@elrest.cz>
---
 drivers/gpio/Kconfig               |   5 +
 drivers/gpio/Makefile              |   1 +
 drivers/gpio/gpio-intel.c          | 198 +++++++++++++++++++++++++++++
 include/platform_data/gpio-intel.h |  10 ++
 4 files changed, 214 insertions(+)
 create mode 100644 drivers/gpio/gpio-intel.c
 create mode 100644 include/platform_data/gpio-intel.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9f27addaa2..094c9b7fd4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -219,6 +219,11 @@ config GPIO_LATCH
 	  Say yes here to enable a driver for GPIO multiplexers based on latches
 	  connected to other GPIOs.
 
+config GPIO_INTEL
+	tristate "Intel GPIO driver"
+	help
+	  Say Y or M here to build support for the Intel GPIO driver.
+
 endmenu
 
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 628e975285..b0575ccf8c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_GPIO_SIFIVE)	+= gpio-sifive.o
 obj-$(CONFIG_GPIO_STARFIVE)	+= gpio-starfive-vic.o
 obj-$(CONFIG_GPIO_ZYNQ)		+= gpio-zynq.o
 obj-$(CONFIG_GPIO_LATCH)	+= gpio-latch.o
+obj-$(CONFIG_GPIO_INTEL)	+= gpio-intel.o
diff --git a/drivers/gpio/gpio-intel.c b/drivers/gpio/gpio-intel.c
new file mode 100644
index 0000000000..ff73169ec7
--- /dev/null
+++ b/drivers/gpio/gpio-intel.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// SPDX-FileCopyrightText: 2004 Tomas Marek, Elrest Solutions Company s.r.o.
+
+/*
+ * Based on the Linux kernel v6.8 drivers/pinctrl/intel/pinctrl-intel.c.
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <io.h>
+#include <gpio.h>
+
+#include <platform_data/gpio-intel.h>
+
+#define PADBAR				0x00c
+
+/* Offset from pad_regs */
+#define PADCFG0				0x000
+#define PADCFG0_RXEVCFG_MASK		GENMASK(26, 25)
+#define PADCFG0_RXEVCFG_LEVEL		(0 << 25)
+#define PADCFG0_RXEVCFG_EDGE		(1 << 25)
+#define PADCFG0_RXEVCFG_DISABLED	(2 << 25)
+#define PADCFG0_RXEVCFG_EDGE_BOTH	(3 << 25)
+#define PADCFG0_PREGFRXSEL		BIT(24)
+#define PADCFG0_RXINV			BIT(23)
+#define PADCFG0_GPIROUTIOXAPIC		BIT(20)
+#define PADCFG0_GPIROUTSCI		BIT(19)
+#define PADCFG0_GPIROUTSMI		BIT(18)
+#define PADCFG0_GPIROUTNMI		BIT(17)
+#define PADCFG0_PMODE_SHIFT		10
+#define PADCFG0_PMODE_MASK		GENMASK(13, 10)
+#define PADCFG0_PMODE_GPIO		0
+#define PADCFG0_GPIORXDIS		BIT(9)
+#define PADCFG0_GPIOTXDIS		BIT(8)
+#define PADCFG0_GPIORXSTATE		BIT(1)
+#define PADCFG0_GPIOTXSTATE		BIT(0)
+
+struct intel_gpio_chip {
+	struct gpio_chip chip;
+	void __iomem *community_pad_base;
+};
+
+static inline struct intel_gpio_chip *to_intel_gpio(struct gpio_chip *gc)
+{
+	return container_of(gc, struct intel_gpio_chip, chip);
+}
+
+static void __iomem *intel_gpio_padcfg0_reg(const struct intel_gpio_chip *chip,
+					    const unsigned int gpio)
+{
+	const u32 pad_cfg_offset = 16;
+
+	return chip->community_pad_base + pad_cfg_offset * gpio;
+}
+
+static u32 intel_gpio_padcfg0_value(const struct intel_gpio_chip *chip,
+				    const unsigned int gpio)
+{
+	return readl(intel_gpio_padcfg0_reg(chip, gpio));
+}
+
+static void intel_gpio_padcfg0_write(const struct intel_gpio_chip *chip,
+				     const unsigned int gpio, u32 value)
+{
+	writel(value, intel_gpio_padcfg0_reg(chip, gpio));
+}
+
+static void intel_gpio_set_value(struct gpio_chip *gc, unsigned int gpio,
+				 int value)
+{
+	struct intel_gpio_chip *chip = to_intel_gpio(gc);
+	u32 padcfg0;
+
+	padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
+	if (value)
+		padcfg0 |= PADCFG0_GPIOTXSTATE;
+	else
+		padcfg0 &= ~PADCFG0_GPIOTXSTATE;
+	intel_gpio_padcfg0_write(chip, gpio, padcfg0);
+}
+
+static int intel_gpio_get_value(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct intel_gpio_chip *chip = to_intel_gpio(gc);
+	u32 padcfg0;
+
+	padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
+	if (!(padcfg0 & PADCFG0_GPIOTXDIS))
+		return !!(padcfg0 & PADCFG0_GPIOTXSTATE);
+
+	return !!(padcfg0 & PADCFG0_GPIORXSTATE);
+}
+
+static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct intel_gpio_chip *chip = to_intel_gpio(gc);
+	u32 padcfg0;
+
+	padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
+
+	if (padcfg0 & PADCFG0_PMODE_MASK)
+		return -EINVAL;
+
+	if (padcfg0 & PADCFG0_GPIOTXDIS)
+		return GPIOF_DIR_IN;
+
+	return GPIOF_DIR_IN;
+}
+
+static int intel_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct intel_gpio_chip *chip = to_intel_gpio(gc);
+	u32 padcfg0;
+
+	padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
+	padcfg0 &= ~PADCFG0_GPIORXDIS;
+	padcfg0 |= PADCFG0_GPIOTXDIS;
+	intel_gpio_padcfg0_write(chip, gpio, padcfg0);
+
+	return 0;
+}
+
+static int intel_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
+				       int value)
+{
+	struct intel_gpio_chip *chip = to_intel_gpio(gc);
+	u32 padcfg0;
+
+	padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
+	padcfg0 &= ~PADCFG0_GPIOTXDIS;
+	padcfg0 |= PADCFG0_GPIORXDIS;
+	if (value)
+		padcfg0 |= PADCFG0_GPIOTXSTATE;
+	else
+		padcfg0 &= ~PADCFG0_GPIOTXSTATE;
+	intel_gpio_padcfg0_write(chip, gpio, padcfg0);
+
+	return 0;
+}
+
+static struct gpio_ops intel_gpio_ops = {
+	.direction_input = intel_gpio_direction_input,
+	.direction_output = intel_gpio_direction_output,
+	.get_direction = intel_gpio_get_direction,
+	.get = intel_gpio_get_value,
+	.set = intel_gpio_set_value,
+};
+
+static int intel_gpio_probe(struct device *dev)
+{
+	const struct gpio_intel_platform_data *pdata;
+	struct intel_gpio_chip *intel_gpio;
+	void __iomem *community_pad_base;
+	void __iomem *community_base;
+	struct resource *iores;
+	int ret;
+
+	pdata = (struct gpio_intel_platform_data *)dev->platform_data;
+	if (!pdata) {
+		dev_err(dev, "Configuration missing!\n");
+		return -EINVAL;
+	}
+
+	iores = dev_request_mem_resource(dev, 0);
+	if (IS_ERR(iores)) {
+		dev_err(dev, "Memory resource request failed: %ld\n",
+			PTR_ERR(iores));
+		return PTR_ERR(iores);
+	}
+
+	intel_gpio = xzalloc(sizeof(*intel_gpio));
+
+	intel_gpio->chip.ops = &intel_gpio_ops;
+	intel_gpio->chip.ngpio = pdata->ngpios;
+	intel_gpio->chip.dev = dev;
+
+	community_base = IOMEM(iores->start);
+	community_pad_base = IOMEM(community_base + readl(community_base + PADBAR));
+
+	intel_gpio->community_pad_base = community_pad_base;
+
+	ret = gpiochip_add(&intel_gpio->chip);
+
+	if (ret) {
+		dev_err(dev, "Couldn't add gpiochip: %d\n", ret);
+		kfree(intel_gpio);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct driver_d intel_gpio_driver = {
+	.name = "intel-gpio",
+	.probe = intel_gpio_probe,
+};
+
+coredevice_platform_driver(intel_gpio_driver);
diff --git a/include/platform_data/gpio-intel.h b/include/platform_data/gpio-intel.h
new file mode 100644
index 0000000000..f04baadd4d
--- /dev/null
+++ b/include/platform_data/gpio-intel.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __GPIO_INTEL_H
+#define __GPIO_INTEL_H
+
+struct gpio_intel_platform_data {
+	unsigned int	ngpios;
+};
+
+#endif /* __GPIO_INTEL_H */
-- 
2.39.2

Re: [PATCH] gpio: Add Intel gpio controller support

[Ahmad Fatoum]

Hello Tomas,

On 09.04.24 09:14, Tomas Marek wrote:
> Signed-off-by: Tomas Marek <tomas.marek@elrest.cz>

Thanks for your patch. I have a soft spot for barebox-as-efi-payload,
so it's cool to see you contributing new features.

It also makes me curious what more drivers are you intending to
contribute. :-)

Some review below. 

> ---
>  drivers/gpio/Kconfig               |   5 +
>  drivers/gpio/Makefile              |   1 +
>  drivers/gpio/gpio-intel.c          | 198 +++++++++++++++++++++++++++++
>  include/platform_data/gpio-intel.h |  10 ++
>  4 files changed, 214 insertions(+)
>  create mode 100644 drivers/gpio/gpio-intel.c
>  create mode 100644 include/platform_data/gpio-intel.h
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 9f27addaa2..094c9b7fd4 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -219,6 +219,11 @@ config GPIO_LATCH
>  	  Say yes here to enable a driver for GPIO multiplexers based on latches
>  	  connected to other GPIOs.
>  
> +config GPIO_INTEL
> +	tristate "Intel GPIO driver"

Please add a depends on X86 || COMPILE_TEST here, so other architectures
aren't prompted for this driver by default.

> +	help
> +	  Say Y or M here to build support for the Intel GPIO driver.

Nitpick: We only have [M]odule support for ARM, so tristate == bool in your
case and one couldn't set M here, despite what the help text suggests.

> +static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	struct intel_gpio_chip *chip = to_intel_gpio(gc);
> +	u32 padcfg0;
> +
> +	padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
> +
> +	if (padcfg0 & PADCFG0_PMODE_MASK)
> +		return -EINVAL;
> +
> +	if (padcfg0 & PADCFG0_GPIOTXDIS)
> +		return GPIOF_DIR_IN;
> +
> +	return GPIOF_DIR_IN;

Your never return GPIOF_DIR_OUT. Is this intended?

> +	ret = gpiochip_add(&intel_gpio->chip);
> +
> +	if (ret) {
> +		dev_err(dev, "Couldn't add gpiochip: %d\n", ret);

Nitpick: %pe\n", ERR_PTR(ret)

> +		kfree(intel_gpio);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct driver_d intel_gpio_driver = {
> +	.name = "intel-gpio",
> +	.probe = intel_gpio_probe,
> +};
> +
> +coredevice_platform_driver(intel_gpio_driver);

Who will register this device? Is it possible to add an ACPI table match
(like itco_wdt does for example) for your SoC and then register the device
there like Linux does?

This would make extension for more SoCs easier in future.

> diff --git a/include/platform_data/gpio-intel.h b/include/platform_data/gpio-intel.h
> new file mode 100644
> index 0000000000..f04baadd4d
> --- /dev/null
> +++ b/include/platform_data/gpio-intel.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __GPIO_INTEL_H
> +#define __GPIO_INTEL_H
> +
> +struct gpio_intel_platform_data {
> +	unsigned int	ngpios;
> +};

I'd suggest you add a add_intel_gpio_device helper here that would create a suitable
device. This could be then called from the ACPI driver probe or from board code if
discoverability is not possible.


Cheers,
Ahmad

> +
> +#endif /* __GPIO_INTEL_H */

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] gpio: Add Intel gpio controller support

[Tomas Marek]

Hello Ahmad,

many thanks for the review.

On Tue, Apr 09, 2024 at 09:41:28AM +0200, Ahmad Fatoum wrote:
> Hello Tomas,
> 
> On 09.04.24 09:14, Tomas Marek wrote:
> > Signed-off-by: Tomas Marek <tomas.marek@elrest.cz>
> 
> Thanks for your patch. I have a soft spot for barebox-as-efi-payload,
> so it's cool to see you contributing new features.
> 
> It also makes me curious what more drivers are you intending to
> contribute. :-)

Nice to hear someone is interested in :-).

> 
> Some review below. 
> 
> > ---
> >  drivers/gpio/Kconfig               |   5 +
> >  drivers/gpio/Makefile              |   1 +
> >  drivers/gpio/gpio-intel.c          | 198 +++++++++++++++++++++++++++++
> >  include/platform_data/gpio-intel.h |  10 ++
> >  4 files changed, 214 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-intel.c
> >  create mode 100644 include/platform_data/gpio-intel.h
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 9f27addaa2..094c9b7fd4 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -219,6 +219,11 @@ config GPIO_LATCH
> >  	  Say yes here to enable a driver for GPIO multiplexers based on latches
> >  	  connected to other GPIOs.
> >  
> > +config GPIO_INTEL
> > +	tristate "Intel GPIO driver"
> 
> Please add a depends on X86 || COMPILE_TEST here, so other architectures
> aren't prompted for this driver by default.

Make sense, I’ll do so.

> 
> > +	help
> > +	  Say Y or M here to build support for the Intel GPIO driver.
> 
> Nitpick: We only have [M]odule support for ARM, so tristate == bool in your
> case and one couldn't set M here, despite what the help text suggests.

I understand. I will change 'tristate' to 'bool' and update the help
to avoid any confusion.

> 
> > +static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> > +{
> > +	struct intel_gpio_chip *chip = to_intel_gpio(gc);
> > +	u32 padcfg0;
> > +
> > +	padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
> > +
> > +	if (padcfg0 & PADCFG0_PMODE_MASK)
> > +		return -EINVAL;
> > +
> > +	if (padcfg0 & PADCFG0_GPIOTXDIS)
> > +		return GPIOF_DIR_IN;
> > +
> > +	return GPIOF_DIR_IN;
> 
> Your never return GPIOF_DIR_OUT. Is this intended?

Silly me. No, it was not intentional; it's a mistake. The last statement
should return GPIOF_DIR_OUT. Thank you for pointing that out. I'll fix it.

> 
> > +	ret = gpiochip_add(&intel_gpio->chip);
> > +
> > +	if (ret) {
> > +		dev_err(dev, "Couldn't add gpiochip: %d\n", ret);
> 
> Nitpick: %pe\n", ERR_PTR(ret)

Thanks for hit.

> 
> > +		kfree(intel_gpio);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct driver_d intel_gpio_driver = {
> > +	.name = "intel-gpio",
> > +	.probe = intel_gpio_probe,
> > +};
> > +
> > +coredevice_platform_driver(intel_gpio_driver);
> 
> Who will register this device? Is it possible to add an ACPI table match
> (like itco_wdt does for example) for your SoC and then register the device
> there like Linux does?
> 
> This would make extension for more SoCs easier in future.

I have registered the device in the board code now. In theory, it is
definitely possible to register the device using ACPI match, and I agree
with you that it's useful.

Unfortunately, the GPIO community resource definition is inside the DSDT
ACPI table for my device. I might be wrong here, but I think that Barebox
parses root tables but doesn’t delve into the nested DSDT at the moment.
So it's getting a bit complicated here. I can take a closer look at it
later (without any guarantee of success, of course :-)), but I would
consider it a different patch if you don't mind.

Needless to say, I am coming from the ARM world and haven't found my way
the ACPI just yet :-).

> 
> > diff --git a/include/platform_data/gpio-intel.h b/include/platform_data/gpio-intel.h
> > new file mode 100644
> > index 0000000000..f04baadd4d
> > --- /dev/null
> > +++ b/include/platform_data/gpio-intel.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef __GPIO_INTEL_H
> > +#define __GPIO_INTEL_H
> > +
> > +struct gpio_intel_platform_data {
> > +	unsigned int	ngpios;
> > +};
> 
> I'd suggest you add a add_intel_gpio_device helper here that would create a suitable
> device. This could be then called from the ACPI driver probe or from board code if
> discoverability is not possible.

Is following code what you have in mind?

include/platform_data/gpio-intel.h:

	struct gpio_intel_platform_data {
		resource_size_t community_base;
		resource_size_t community_size;
		unsigned int	ngpios;
	};

	static inline struct device *add_intel_gpio_device(
		struct gpio_intel_platform_data *pdata
	)
	{
		return add_generic_device("intel-gpio", DEVICE_ID_DYNAMIC, NULL,
			pdata->community_base, pdata->community_size,
			IORESOURCE_MEM, pdata);
	}


Best regards

Tomas

> 
> 
> Cheers,
> Ahmad
> 
> > +
> > +#endif /* __GPIO_INTEL_H */
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

Re: [PATCH] gpio: Add Intel gpio controller support

[Ahmad Fatoum]

Hello Tomas,

On 10.04.24 09:39, Tomas Marek wrote:
> On Tue, Apr 09, 2024 at 09:41:28AM +0200, Ahmad Fatoum wrote:
>>> +static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
>>> +		kfree(intel_gpio);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct driver_d intel_gpio_driver = {
>>> +	.name = "intel-gpio",
>>> +	.probe = intel_gpio_probe,
>>> +};
>>> +
>>> +coredevice_platform_driver(intel_gpio_driver);
>>
>> Who will register this device? Is it possible to add an ACPI table match
>> (like itco_wdt does for example) for your SoC and then register the device

Typo: s/itco_wdt/WDAT/

>> there like Linux does?
>>
>> This would make extension for more SoCs easier in future.
> 
> I have registered the device in the board code now. In theory, it is
> definitely possible to register the device using ACPI match, and I agree
> with you that it's useful.
> 
> Unfortunately, the GPIO community resource definition is inside the DSDT
> ACPI table for my device. I might be wrong here, but I think that Barebox
> parses root tables but doesn’t delve into the nested DSDT at the moment.

We indeed do not do DSDT parsing yet.

> So it's getting a bit complicated here. I can take a closer look at it
> later (without any guarantee of success, of course :-)), but I would
> consider it a different patch if you don't mind.

I was thinking of matching against the SoC type (e.g. Alderlake) and just
register hardcoded values for the addresses/ngpios. I am not sure what's the
easiest way to get this information though (maybe SMBIOS?).

This shouldn't delay applying this driver though.

> Needless to say, I am coming from the ARM world and haven't found my way
> the ACPI just yet :-).

Same here ^^

>>> +#ifndef __GPIO_INTEL_H
>>> +#define __GPIO_INTEL_H
>>> +
>>> +struct gpio_intel_platform_data {
>>> +	unsigned int	ngpios;
>>> +};
>>
>> I'd suggest you add a add_intel_gpio_device helper here that would create a suitable
>> device. This could be then called from the ACPI driver probe or from board code if
>> discoverability is not possible.
> 
> Is following code what you have in mind?
> 
> include/platform_data/gpio-intel.h:
> 
> 	struct gpio_intel_platform_data {
> 		resource_size_t community_base;
> 		resource_size_t community_size;
> 		unsigned int	ngpios;
> 	};
> 
> 	static inline struct device *add_intel_gpio_device(
> 		struct gpio_intel_platform_data *pdata
> 	)
> 	{
> 		return add_generic_device("intel-gpio", DEVICE_ID_DYNAMIC, NULL,
> 			pdata->community_base, pdata->community_size,
> 			IORESOURCE_MEM, pdata);
> 	}

This looks ok.

Cheers,
Ahmad

> 
> 
> Best regards
> 
> Tomas
> 
>>
>>
>> Cheers,
>> Ahmad
>>
>>> +
>>> +#endif /* __GPIO_INTEL_H */
>>
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |