[PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O

Raag Jadav posted 2 patches 3 months, 1 week ago
There is a newer version of this series
[PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O
Posted by Raag Jadav 3 months, 1 week ago
Intel Elkhart Lake Programmable Service Engine (PSE) includes two PCI
devices that expose two different capabilities of GPIO and Timed I/O
as a single PCI function through shared MMIO with below layout.

GPIO: 0x0000 - 0x1000
TIO:  0x1000 - 0x2000

This driver enumerates the PCI parent device and creates auxiliary child
devices for these capabilities. The actual functionalities are provided
by their respective auxiliary drivers.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 MAINTAINERS                             |   7 ++
 drivers/platform/x86/intel/Kconfig      |  13 +++
 drivers/platform/x86/intel/Makefile     |   1 +
 drivers/platform/x86/intel/ehl_pse_io.c | 128 ++++++++++++++++++++++++
 include/linux/ehl_pse_io_aux.h          |  30 ++++++
 5 files changed, 179 insertions(+)
 create mode 100644 drivers/platform/x86/intel/ehl_pse_io.c
 create mode 100644 include/linux/ehl_pse_io_aux.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 46126ce2f968..bd2a009d73c6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12499,6 +12499,13 @@ F:	drivers/gpu/drm/xe/
 F:	include/drm/intel/
 F:	include/uapi/drm/xe_drm.h
 
+INTEL ELKHART LAKE PSE I/O DRIVER
+M:	Raag Jadav <raag.jadav@intel.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Supported
+F:	drivers/platform/x86/intel/ehl_pse_io.c
+F:	include/linux/ehl_pse_io_aux.h
+
 INTEL ETHERNET DRIVERS
 M:	Tony Nguyen <anthony.l.nguyen@intel.com>
 M:	Przemek Kitszel <przemyslaw.kitszel@intel.com>
diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 19a2246f2770..2900407d6095 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -41,6 +41,19 @@ config INTEL_VBTN
 	  To compile this driver as a module, choose M here: the module will
 	  be called intel_vbtn.
 
+config INTEL_EHL_PSE_IO
+	tristate "Intel Elkhart Lake PSE I/O driver"
+	depends on PCI
+	select AUXILIARY_BUS
+	help
+	  Select this option to enable Intel Elkhart Lake PSE GPIO and Timed
+	  I/O support. This driver enumerates the PCI parent device and
+	  creates auxiliary child devices for these capabilities. The actual
+	  functionalities are provided by their respective auxiliary drivers.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called intel_ehl_pse_io.
+
 config INTEL_INT0002_VGPIO
 	tristate "Intel ACPI INT0002 Virtual GPIO driver"
 	depends on GPIOLIB && ACPI && PM_SLEEP
diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
index 78acb414e154..138b13756158 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -21,6 +21,7 @@ intel-target-$(CONFIG_INTEL_HID_EVENT)		+= hid.o
 intel-target-$(CONFIG_INTEL_VBTN)		+= vbtn.o
 
 # Intel miscellaneous drivers
+intel-target-$(CONFIG_INTEL_EHL_PSE_IO)		+= ehl_pse_io.o
 intel-target-$(CONFIG_INTEL_INT0002_VGPIO)	+= int0002_vgpio.o
 intel-target-$(CONFIG_INTEL_ISHTP_ECLITE)	+= ishtp_eclite.o
 intel-target-$(CONFIG_INTEL_OAKTRAIL)		+= oaktrail.o
diff --git a/drivers/platform/x86/intel/ehl_pse_io.c b/drivers/platform/x86/intel/ehl_pse_io.c
new file mode 100644
index 000000000000..f1cad102f856
--- /dev/null
+++ b/drivers/platform/x86/intel/ehl_pse_io.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Elkhart Lake Programmable Service Engine (PSE) I/O
+ *
+ * Copyright (c) 2025 Intel Corporation.
+ *
+ * Author: Raag Jadav <raag.jadav@intel.com>
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/dev_printk.h>
+#include <linux/device/devres.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gfp_types.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <linux/ehl_pse_io_aux.h>
+
+#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
+#define EHL_PSE_IO_DEV_SIZE	SZ_4K
+
+static void ehl_pse_io_dev_release(struct device *dev)
+{
+	struct auxiliary_device *aux_dev = to_auxiliary_dev(dev);
+	struct ehl_pse_io_dev *io_dev = auxiliary_dev_to_ehl_pse_io_dev(aux_dev);
+
+	kfree(io_dev);
+}
+
+static int ehl_pse_io_dev_add(struct pci_dev *pci, const char *name, int idx)
+{
+	struct auxiliary_device *aux_dev;
+	struct device *dev = &pci->dev;
+	struct ehl_pse_io_dev *io_dev;
+	resource_size_t start;
+	int ret;
+
+	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
+	if (!io_dev)
+		return -ENOMEM;
+
+	start = pci_resource_start(pci, 0);
+	io_dev->irq = pci_irq_vector(pci, idx);
+	io_dev->mem = DEFINE_RES_MEM(start + (EHL_PSE_IO_DEV_OFFSET * idx), EHL_PSE_IO_DEV_SIZE);
+
+	aux_dev = &io_dev->aux_dev;
+	aux_dev->name = name;
+	aux_dev->id = (pci_domain_nr(pci->bus) << 16) | pci_dev_id(pci);
+	aux_dev->dev.parent = dev;
+	aux_dev->dev.release = ehl_pse_io_dev_release;
+
+	ret = auxiliary_device_init(aux_dev);
+	if (ret)
+		goto free_io_dev;
+
+	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
+	if (ret)
+		goto uninit_aux_dev;
+
+	return 0;
+
+uninit_aux_dev:
+	/* io_dev will be freed with the put_device() and .release sequence */
+	auxiliary_device_uninit(aux_dev);
+free_io_dev:
+	kfree(io_dev);
+	return ret;
+}
+
+static int ehl_pse_io_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	int ret;
+
+	ret = pcim_enable_device(pci);
+	if (ret)
+		return ret;
+
+	pci_set_master(pci);
+
+	ret = pci_alloc_irq_vectors(pci, 2, 2, PCI_IRQ_MSI);
+	if (ret < 0)
+		return ret;
+
+	ret = ehl_pse_io_dev_add(pci, EHL_PSE_GPIO_NAME, 0);
+	if (ret)
+		return ret;
+
+	return ehl_pse_io_dev_add(pci, EHL_PSE_TIO_NAME, 1);
+}
+
+static int ehl_pse_io_dev_destroy(struct device *dev, void *data)
+{
+	auxiliary_device_destroy(to_auxiliary_dev(dev));
+
+	return 0;
+}
+
+static void ehl_pse_io_remove(struct pci_dev *pci)
+{
+	struct device *dev = &pci->dev;
+
+	device_for_each_child_reverse(dev, NULL, ehl_pse_io_dev_destroy);
+}
+
+static const struct pci_device_id ehl_pse_io_ids[] = {
+	{ PCI_VDEVICE(INTEL, 0x4b88) },
+	{ PCI_VDEVICE(INTEL, 0x4b89) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, ehl_pse_io_ids);
+
+static struct pci_driver ehl_pse_io_driver = {
+	.name		= EHL_PSE_IO_NAME,
+	.id_table	= ehl_pse_io_ids,
+	.probe		= ehl_pse_io_probe,
+	.remove		= ehl_pse_io_remove,
+};
+module_pci_driver(ehl_pse_io_driver);
+
+MODULE_AUTHOR("Raag Jadav <raag.jadav@intel.com>");
+MODULE_DESCRIPTION("Intel Elkhart Lake PSE I/O driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/ehl_pse_io_aux.h b/include/linux/ehl_pse_io_aux.h
new file mode 100644
index 000000000000..33eb5a86ce36
--- /dev/null
+++ b/include/linux/ehl_pse_io_aux.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Intel Elkhart Lake PSE I/O Auxiliary Device
+ *
+ * Copyright (c) 2025 Intel Corporation.
+ *
+ * Author: Raag Jadav <raag.jadav@intel.com>
+ */
+
+#ifndef _EHL_PSE_IO_AUX_H_
+#define _EHL_PSE_IO_AUX_H_
+
+#include <linux/auxiliary_bus.h>
+#include <linux/container_of.h>
+#include <linux/ioport.h>
+
+#define EHL_PSE_IO_NAME		"ehl-pse-io"
+#define EHL_PSE_GPIO_NAME	"gpio-elkhartlake"
+#define EHL_PSE_TIO_NAME	"pps-tio"
+
+struct ehl_pse_io_dev {
+	struct auxiliary_device aux_dev;
+	struct resource mem;
+	int irq;
+};
+
+#define auxiliary_dev_to_ehl_pse_io_dev(auxiliary_dev) \
+	container_of(auxiliary_dev, struct ehl_pse_io_dev, aux_dev)
+
+#endif /* _EHL_PSE_IO_AUX_H_ */
-- 
2.34.1
Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O
Posted by Andy Shevchenko 3 months, 1 week ago
On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:
> Intel Elkhart Lake Programmable Service Engine (PSE) includes two PCI
> devices that expose two different capabilities of GPIO and Timed I/O
> as a single PCI function through shared MMIO with below layout.
> 
> GPIO: 0x0000 - 0x1000
> TIO:  0x1000 - 0x2000
> 
> This driver enumerates the PCI parent device and creates auxiliary child
> devices for these capabilities. The actual functionalities are provided
> by their respective auxiliary drivers.

...

> +#include <linux/auxiliary_bus.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device/devres.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gfp_types.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>

> +#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
> +#define EHL_PSE_IO_DEV_SIZE	SZ_4K

Not sure if SZ_4K is a good idea for the _OFFSET, the _SIZE is fine. Also why
do we need two? If the devices are of the same size, we don't need to have a
separate offset.

...

> +static int ehl_pse_io_dev_add(struct pci_dev *pci, const char *name, int idx)
> +{
> +	struct auxiliary_device *aux_dev;
> +	struct device *dev = &pci->dev;
> +	struct ehl_pse_io_dev *io_dev;
> +	resource_size_t start;
> +	int ret;
> +
> +	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
> +	if (!io_dev)
> +		return -ENOMEM;

Why devm_kzalloc() can't be used? I don't see if the device lifetime is anyhow
different to this object. Am I wrong?

> +	start = pci_resource_start(pci, 0);
> +	io_dev->irq = pci_irq_vector(pci, idx);
> +	io_dev->mem = DEFINE_RES_MEM(start + (EHL_PSE_IO_DEV_OFFSET * idx), EHL_PSE_IO_DEV_SIZE);
> +
> +	aux_dev = &io_dev->aux_dev;
> +	aux_dev->name = name;
> +	aux_dev->id = (pci_domain_nr(pci->bus) << 16) | pci_dev_id(pci);
> +	aux_dev->dev.parent = dev;
> +	aux_dev->dev.release = ehl_pse_io_dev_release;
> +
> +	ret = auxiliary_device_init(aux_dev);
> +	if (ret)
> +		goto free_io_dev;
> +
> +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);

Hmm... Is it okay to use double underscored variant? Only a single driver uses
this so far... Care to elaborate?

> +	if (ret)
> +		goto uninit_aux_dev;
> +
> +	return 0;
> +
> +uninit_aux_dev:
> +	/* io_dev will be freed with the put_device() and .release sequence */

Right...

> +	auxiliary_device_uninit(aux_dev);
> +free_io_dev:
> +	kfree(io_dev);

...and this is a double free, correct?

> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O
Posted by Raag Jadav 3 months, 1 week ago
On Wed, Oct 29, 2025 at 10:36:19AM +0200, Andy Shevchenko wrote:
> On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:
> > Intel Elkhart Lake Programmable Service Engine (PSE) includes two PCI
> > devices that expose two different capabilities of GPIO and Timed I/O
> > as a single PCI function through shared MMIO with below layout.
> > 
> > GPIO: 0x0000 - 0x1000
> > TIO:  0x1000 - 0x2000
> > 
> > This driver enumerates the PCI parent device and creates auxiliary child
> > devices for these capabilities. The actual functionalities are provided
> > by their respective auxiliary drivers.
> 
> ...
> 
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/device/devres.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/gfp_types.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/sizes.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> 
> > +#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
> > +#define EHL_PSE_IO_DEV_SIZE	SZ_4K
> 
> Not sure if SZ_4K is a good idea for the _OFFSET, the _SIZE is fine. Also why
> do we need two? If the devices are of the same size, we don't need to have a
> separate offset.

Yes but they're semantically different, atleast as per DEFINE_RES_MEM().
Either way works for me.

...

> > +static int ehl_pse_io_dev_add(struct pci_dev *pci, const char *name, int idx)
> > +{
> > +	struct auxiliary_device *aux_dev;
> > +	struct device *dev = &pci->dev;
> > +	struct ehl_pse_io_dev *io_dev;
> > +	resource_size_t start;
> > +	int ret;
> > +
> > +	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
> > +	if (!io_dev)
> > +		return -ENOMEM;
> 
> Why devm_kzalloc() can't be used? I don't see if the device lifetime is anyhow
> different to this object. Am I wrong?

Looks like it but I don't know the code well enough to tell if there're
corner cases, so just following the documented rules. Your call.

> > +	start = pci_resource_start(pci, 0);
> > +	io_dev->irq = pci_irq_vector(pci, idx);
> > +	io_dev->mem = DEFINE_RES_MEM(start + (EHL_PSE_IO_DEV_OFFSET * idx), EHL_PSE_IO_DEV_SIZE);
> > +
> > +	aux_dev = &io_dev->aux_dev;
> > +	aux_dev->name = name;
> > +	aux_dev->id = (pci_domain_nr(pci->bus) << 16) | pci_dev_id(pci);
> > +	aux_dev->dev.parent = dev;
> > +	aux_dev->dev.release = ehl_pse_io_dev_release;
> > +
> > +	ret = auxiliary_device_init(aux_dev);
> > +	if (ret)
> > +		goto free_io_dev;
> > +
> > +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
> 
> Hmm... Is it okay to use double underscored variant? Only a single driver uses
> this so far... Care to elaborate?

The regular variant uses KBUILD_MODNAME which comes with 'intel' prefix
after commit df7f9acd8646, and with that we overshoot the max id string
length for leaf drivers.

> > +	if (ret)
> > +		goto uninit_aux_dev;
> > +
> > +	return 0;
> > +
> > +uninit_aux_dev:
> > +	/* io_dev will be freed with the put_device() and .release sequence */
> 
> Right...
> 
> > +	auxiliary_device_uninit(aux_dev);
> > +free_io_dev:
> > +	kfree(io_dev);
> 
> ...and this is a double free, correct?

Yeah, my sheer incompetence at stealing code :(

Raag
Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O
Posted by Andy Shevchenko 3 months, 1 week ago
On Fri, Oct 31, 2025 at 10:34:28AM +0100, Raag Jadav wrote:
> On Wed, Oct 29, 2025 at 10:36:19AM +0200, Andy Shevchenko wrote:
> > On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:

...

> > > +#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
> > > +#define EHL_PSE_IO_DEV_SIZE	SZ_4K
> > 
> > Not sure if SZ_4K is a good idea for the _OFFSET, the _SIZE is fine. Also why
> > do we need two? If the devices are of the same size, we don't need to have a
> > separate offset.
> 
> Yes but they're semantically different, atleast as per DEFINE_RES_MEM().
> Either way works for me.

They are "slices" in the HW, see also my "if the devices..." passage.

If you want to use SZ_* in _OFFSET, I would write it as (1 * SZ_4K) to point
out that size constant here is the _unit_ and not the size semantically.
Currently the definitions have the same values semantically, but you pointed
out that they should not be.

...

> > > +	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
> > > +	if (!io_dev)
> > > +		return -ENOMEM;
> > 
> > Why devm_kzalloc() can't be used? I don't see if the device lifetime is anyhow
> > different to this object. Am I wrong?
> 
> Looks like it but I don't know the code well enough to tell if there're
> corner cases, so just following the documented rules. Your call.

Do you expect this to be called in non-probe() contexts? If no --> devm.
Otherwise some comments are needed.

...

> > > +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
> > 
> > Hmm... Is it okay to use double underscored variant? Only a single driver uses
> > this so far... Care to elaborate?
> 
> The regular variant uses KBUILD_MODNAME which comes with 'intel' prefix
> after commit df7f9acd8646, and with that we overshoot the max id string
> length for leaf drivers.

At bare minimum this needs a comment, but I think ideally we need to bump the
limit by factor of 2.

> > > +	if (ret)
> > > +		goto uninit_aux_dev;

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O
Posted by Raag Jadav 3 months, 1 week ago
On Fri, Oct 31, 2025 at 12:02:14PM +0200, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 10:34:28AM +0100, Raag Jadav wrote:
> > On Wed, Oct 29, 2025 at 10:36:19AM +0200, Andy Shevchenko wrote:
> > > On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:
> 
> ...
> 
> > > > +#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
> > > > +#define EHL_PSE_IO_DEV_SIZE	SZ_4K
> > > 
> > > Not sure if SZ_4K is a good idea for the _OFFSET, the _SIZE is fine. Also why
> > > do we need two? If the devices are of the same size, we don't need to have a
> > > separate offset.
> > 
> > Yes but they're semantically different, atleast as per DEFINE_RES_MEM().
> > Either way works for me.
> 
> They are "slices" in the HW, see also my "if the devices..." passage.
> 
> If you want to use SZ_* in _OFFSET, I would write it as (1 * SZ_4K) to point
> out that size constant here is the _unit_ and not the size semantically.
> Currently the definitions have the same values semantically, but you pointed
> out that they should not be.

Fair. Will consolidate.

> > > > +	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
> > > > +	if (!io_dev)
> > > > +		return -ENOMEM;
> > > 
> > > Why devm_kzalloc() can't be used? I don't see if the device lifetime is anyhow
> > > different to this object. Am I wrong?
> > 
> > Looks like it but I don't know the code well enough to tell if there're
> > corner cases, so just following the documented rules. Your call.
> 
> Do you expect this to be called in non-probe() contexts? If no --> devm.
> Otherwise some comments are needed.

Sure.

> > > > +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
> > > 
> > > Hmm... Is it okay to use double underscored variant? Only a single driver uses
> > > this so far... Care to elaborate?
> > 
> > The regular variant uses KBUILD_MODNAME which comes with 'intel' prefix
> > after commit df7f9acd8646, and with that we overshoot the max id string
> > length for leaf drivers.
> 
> At bare minimum this needs a comment, but I think ideally we need to bump the
> limit by factor of 2.

Which will probably require a wider discussion, so perhaps let's pursue it
separately?

Raag
Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O
Posted by Andy Shevchenko 3 months, 1 week ago
On Fri, Oct 31, 2025 at 12:59:24PM +0100, Raag Jadav wrote:
> On Fri, Oct 31, 2025 at 12:02:14PM +0200, Andy Shevchenko wrote:
> > On Fri, Oct 31, 2025 at 10:34:28AM +0100, Raag Jadav wrote:
> > > On Wed, Oct 29, 2025 at 10:36:19AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:

...

> > > > > +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
> > > > 
> > > > Hmm... Is it okay to use double underscored variant? Only a single driver uses
> > > > this so far... Care to elaborate?
> > > 
> > > The regular variant uses KBUILD_MODNAME which comes with 'intel' prefix
> > > after commit df7f9acd8646, and with that we overshoot the max id string
> > > length for leaf drivers.
> > 
> > At bare minimum this needs a comment, but I think ideally we need to bump the
> > limit by factor of 2.
> 
> Which will probably require a wider discussion, so perhaps let's pursue it
> separately?

Send a patch starting a discussion. Looking at the history of bumping other
cases it's usually well accepted when properly justified. And since it's now
32, bumping to 40 maybe enough for several more years.

-- 
With Best Regards,
Andy Shevchenko