Intel MSIC MFD driver for Medfield platform

Hi,

This series adds support for the Intel MSIC (Mixed Signal IC) chip found on Intel Medfield platforms. The MSIC chip embeds audio codec, ADC, GPIOs etc.

Existing drivers which use plain SCU IPC will continue to work and they can be converted to the MSIC register interface later on. Any new drivers for devices in the MSIC should use this interface.

Thanks.

Alan Cox (1):
x86, mrst: Some drivers need to known when an SCU is available

Mika Westerberg (2):
mfd: add Intel MSIC driver
x86, mrst: add platform support for MSIC MFD driver

 arch/x86/include/asm/intel_scu_ipc.h |   22 ++
 arch/x86/platform/mrst/mrst.c        |  174 +++++++++++-
 drivers/mfd/Kconfig                  |    9 +
 drivers/mfd/Makefile                 |    1 +
 drivers/mfd/intel_msic.c             |  501 ++++++++++++++++++++++++++++++++++
 include/linux/mfd/intel_msic.h       |  456 +++++++++++++++++++++++++++++++
 6 files changed, 1149 insertions(+), 14 deletions(-)
 create mode 100644 drivers/mfd/intel_msic.c
 create mode 100644 include/linux/mfd/intel_msic.h

Signature · 1 line

1.7.5.4

Hi Samuel and x86 maintainers,

Do you have any comments on these patches? Could they be merged into mainline or do you want me to make some changes?

Add support for Intel MSIC chip found on Intel Medfield platforms. This chip embeds several subdevices: audio, ADC, GPIO, power button, etc. The driver creates platform device for each subdevice.

We also provide an MSIC register access API which should replace the more generic SCU IPC interface currently used. Existing drivers can choose whether they convert to this new API or stick with the SCU IPC interface.

+/*
+ * Other MSIC related devices which are not directly available via SFI DEVS
+ * table.

drivers/mfd/intel_msic.c new file +21 -0

+/**
+ * intel_msic_reg_read - read a single MSIC register
+ * @reg: register to read
+ * @val: register value is placed here
+ *
+ * Read a single register from MSIC. Returns %0 on success and negative
+ * errno in case of failure.
+ *
+ * Function may sleep.
+ */
+int intel_msic_reg_read(unsigned short reg, u8 *val)
+{
+	return intel_scu_ipc_ioread8(reg, val);
+}
+EXPORT_SYMBOL_GPL(intel_msic_reg_read);
+
+/**
+ * intel_msic_reg_write - write a single MSIC register
+ * @reg: register to write
+ * @val: value to write to that register
+ *

drivers/mfd/intel_msic.c new file +33 -0

+/**
+ * intel_msic_bulk_read - read an array of registers
+ * @reg: array of register addresses to read
+ * @buf: array where the read values are placed
+ * @count: number of registers to read
+ *
+ * Function reads @count registers from the MSIC using addresses passed in
+ * @reg. Read values are placed in @buf. Reads are performed atomically
+ * wrt. MSIC.
+ *
+ * Returns %0 in case of success and negative errno in case of failure.
+ *
+ * Function may sleep.
+ */
+int intel_msic_bulk_read(unsigned short *reg, u8 *buf, size_t count)
+{
+	if (WARN_ON(count > SCU_IPC_RWBUF_LIMIT))
+		return -EINVAL;
+
+	return intel_scu_ipc_readv(reg, buf, count);
+}
+EXPORT_SYMBOL_GPL(intel_msic_bulk_read);
+
+/**
+ * intel_msic_bulk_write - write an array of values to the MSIC registers
+ * @reg: array of registers to write
+ * @buf: values to write to each register
+ * @count: number of registers to write
+ *
+ * Function writes @count registers in @buf to MSIC. Writes are performed
+ * atomically wrt MSIC. Returns %0 in case of success and negative errno in
+ * case of failure.
+ *

drivers/mfd/intel_msic.c new file +26 -0

+static int __devinit intel_msic_init_devices(struct intel_msic *msic)
+{
+	struct platform_device *pdev = msic->pdev;
+	struct intel_msic_platform_data *pdata = pdev->dev.platform_data;
+	int ret, i;
+
+	if (pdata->gpio) {
+		struct mfd_cell *cell = &msic_devs[INTEL_MSIC_BLOCK_GPIO];
+
+		cell->platform_data = pdata->gpio;
+		cell->pdata_size = sizeof(*pdata->gpio);
+	}
+
+	if (pdata->ocd) {
+		unsigned gpio = pdata->ocd->gpio;
+
+		ret = gpio_request_one(gpio, GPIOF_IN, "ocd_gpio");
+		if (ret) {
+			dev_err(&pdev->dev, "failed to register OCD GPIO\n");
+			return ret;
+		}
+
+		ret = gpio_to_irq(gpio);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "no IRQ number for OCD GPIO\n");
+			gpio_free(gpio);

Hi Mika,

Quoted 7 lines · Mika Westerberg

Add support for Intel MSIC chip found on Intel Medfield platforms. This chip embeds several subdevices: audio, ADC, GPIO, power button, etc. The driver creates platform device for each subdevice.

We also provide an MSIC register access API which should replace the more generic SCU IPC interface currently used. Existing drivers can choose whether they convert to this new API or stick with the SCU IPC interface.

Looks good overall, I just have a few comments/questions:

Quoted 3 lines

+/*
+ * Other MSIC related devices which are not directly available via SFI DEVS
+ * table.

Would that mean a broken firmware, or something else I'm missing ? It looks odd.

Quoted 26 lines

drivers/mfd/intel_msic.c new file +26 -0

+/**
+ * intel_msic_reg_read - read a single MSIC register
+ * @reg: register to read
+ * @val: register value is placed here
+ *
+ * Read a single register from MSIC. Returns %0 on success and negative
+ * errno in case of failure.
+ *
+ * Function may sleep.
+ */
+int intel_msic_reg_read(unsigned short reg, u8 *val)
+{
+	return intel_scu_ipc_ioread8(reg, val);
+}
+EXPORT_SYMBOL_GPL(intel_msic_reg_read);
+
+/**
+ * intel_msic_reg_write - write a single MSIC register
+ * @reg: register to write
+ * @val: value to write to that register
+ *
+ * Write a single MSIC register. Returns 0 on success and negative
+ * errno in case of failure.
+ *
+ * Function may sleep.
+ */

drivers/mfd/intel_msic.c line 217

Quoted 5 lines

drivers/mfd/intel_msic.c new file +5 -0

+int intel_msic_reg_write(unsigned short reg, u8 val)
+{
+	return intel_scu_ipc_iowrite8(reg, val);
+}
+EXPORT_SYMBOL_GPL(intel_msic_reg_write);

What is the benefit of those wrappers since you're probably not going to get rid of the SCU IPC APIs, right ? What's wrong with the subdevices using the SCU IPC API directly ?

Quoted 36 lines

drivers/mfd/intel_msic.c new file +36 -0

+/**
+ * intel_msic_bulk_read - read an array of registers
+ * @reg: array of register addresses to read
+ * @buf: array where the read values are placed
+ * @count: number of registers to read
+ *
+ * Function reads @count registers from the MSIC using addresses passed in
+ * @reg. Read values are placed in @buf. Reads are performed atomically
+ * wrt. MSIC.
+ *
+ * Returns %0 in case of success and negative errno in case of failure.
+ *
+ * Function may sleep.
+ */
+int intel_msic_bulk_read(unsigned short *reg, u8 *buf, size_t count)
+{
+	if (WARN_ON(count > SCU_IPC_RWBUF_LIMIT))
+		return -EINVAL;
+
+	return intel_scu_ipc_readv(reg, buf, count);
+}
+EXPORT_SYMBOL_GPL(intel_msic_bulk_read);
+
+/**
+ * intel_msic_bulk_write - write an array of values to the MSIC registers
+ * @reg: array of registers to write
+ * @buf: values to write to each register
+ * @count: number of registers to write
+ *
+ * Function writes @count registers in @buf to MSIC. Writes are performed
+ * atomically wrt MSIC. Returns %0 in case of success and negative errno in
+ * case of failure.
+ *
+ * Function may sleep.
+ */
+int intel_msic_bulk_write(unsigned short *reg, u8 *buf, size_t count)

drivers/mfd/intel_msic.c line 280

Quoted 7 lines

drivers/mfd/intel_msic.c new file +7 -0

+{
+	if (WARN_ON(count > SCU_IPC_RWBUF_LIMIT))
+		return -EINVAL;
+
+	return intel_scu_ipc_writev(reg, buf, count);
+}
+EXPORT_SYMBOL_GPL(intel_msic_bulk_write);

The 2 routines above make more sense.

Quoted 31 lines

drivers/mfd/intel_msic.c new file +31 -0

+static int __devinit intel_msic_init_devices(struct intel_msic *msic)
+{
+	struct platform_device *pdev = msic->pdev;
+	struct intel_msic_platform_data *pdata = pdev->dev.platform_data;
+	int ret, i;
+
+	if (pdata->gpio) {
+		struct mfd_cell *cell = &msic_devs[INTEL_MSIC_BLOCK_GPIO];
+
+		cell->platform_data = pdata->gpio;
+		cell->pdata_size = sizeof(*pdata->gpio);
+	}
+
+	if (pdata->ocd) {
+		unsigned gpio = pdata->ocd->gpio;
+
+		ret = gpio_request_one(gpio, GPIOF_IN, "ocd_gpio");
+		if (ret) {
+			dev_err(&pdev->dev, "failed to register OCD GPIO\n");
+			return ret;
+		}
+
+		ret = gpio_to_irq(gpio);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "no IRQ number for OCD GPIO\n");
+			gpio_free(gpio);
+			return ret;
+		}
+
+		/* Update the IRQ number for the OCD */
+		pdata->irq[INTEL_MSIC_BLOCK_OCD] = ret;

drivers/mfd/intel_msic.c line 343

Quoted 5 lines

drivers/mfd/intel_msic.c new file +5 -0

+	}
+
+	for (i = 0; i < ARRAY_SIZE(msic_devs); i++) {
+		if (!pdata->irq[i])
+			continue;

I would expect some sort of warning here since it would mean your platform code defined a sub device platform data without giving you the right IRQ. And afaiu, all of your sub devices must have one.

Quoted 5 lines

Quoted 3 lines

+/*
+ * Other MSIC related devices which are not directly available via SFI DEVS
+ * table.

Would that mean a broken firmware, or something else I'm missing ? It looks odd.

Not all devices in the MSIC are listed in SFI DEVS table. For example regulators are missing. We currently don't have regulator support for MSIC but once it is added the devices will be created here.

Audio codec is similar - it does not exist in the SFI DEVS table.

Quoted 26 lines

Quoted 26 lines

drivers/mfd/intel_msic.c new file +26 -0

+/**
+ * intel_msic_reg_read - read a single MSIC register
+ * @reg: register to read
+ * @val: register value is placed here
+ *
+ * Read a single register from MSIC. Returns %0 on success and negative
+ * errno in case of failure.
+ *
+ * Function may sleep.
+ */
+int intel_msic_reg_read(unsigned short reg, u8 *val)
+{
+	return intel_scu_ipc_ioread8(reg, val);
+}
+EXPORT_SYMBOL_GPL(intel_msic_reg_read);
+
+/**
+ * intel_msic_reg_write - write a single MSIC register
+ * @reg: register to write
+ * @val: value to write to that register
+ *
+ * Write a single MSIC register. Returns 0 on success and negative
+ * errno in case of failure.
+ *
+ * Function may sleep.
+ */

Quoted 8 lines

drivers/mfd/intel_msic.c line 217

Quoted 5 lines

drivers/mfd/intel_msic.c new file +5 -0

+int intel_msic_reg_write(unsigned short reg, u8 val)
+{
+	return intel_scu_ipc_iowrite8(reg, val);
+}
+EXPORT_SYMBOL_GPL(intel_msic_reg_write);

What is the benefit of those wrappers since you're probably not going to get rid of the SCU IPC APIs, right ? What's wrong with the subdevices using the SCU IPC API directly ?

Right, we are not going to get rid of SCU IPC APIs. The idea behind having these register access wrapper functions is that we are now able to separate the MSIC subdevices from other SCU IPC usage. In other words we get a bit cleaner "architecture".

It also allows us to add caching and intercepting register accesses if a need rises.

Quoted 6 lines

drivers/mfd/intel_msic.c line 343

Quoted 3 lines

drivers/mfd/intel_msic.c new file +3 -0

+	for (i = 0; i < ARRAY_SIZE(msic_devs); i++) {
+		if (!pdata->irq[i])
+			continue;

I would expect some sort of warning here since it would mean your platform code defined a sub device platform data without giving you the right IRQ. And afaiu, all of your sub devices must have one.

The interface for platform data says that if irq is zero, it means that no platform device is created.

For example in patch 3/3 we add the platform data for battery, gpio, audio, power_button and ocd but the SFI table actually contains more devices. We don't yet have (upstream) driver for those so there is a little point of creating platform device for them at this point.

Hi Mika,

Quoted 6 lines · Mika Westerberg

Quoted 5 lines

Quoted 3 lines

+/*
+ * Other MSIC related devices which are not directly available via SFI DEVS
+ * table.

Would that mean a broken firmware, or something else I'm missing ? It looks odd.

Quoted 5 lines

Not all devices in the MSIC are listed in SFI DEVS table. For example regulators are missing. We currently don't have regulator support for MSIC but once it is added the devices will be created here.

Audio codec is similar - it does not exist in the SFI DEVS table.

Ok. So I can expect further patches removing this one then.

Quoted 36 lines

Quoted 35 lines

Quoted 22 lines

drivers/mfd/intel_msic.c new file +21 -0

+/**
+ * intel_msic_reg_read - read a single MSIC register
+ * @reg: register to read
+ * @val: register value is placed here
+ *
+ * Read a single register from MSIC. Returns %0 on success and negative
+ * errno in case of failure.
+ *
+ * Function may sleep.
+ */
+int intel_msic_reg_read(unsigned short reg, u8 *val)
+{
+	return intel_scu_ipc_ioread8(reg, val);
+}
+EXPORT_SYMBOL_GPL(intel_msic_reg_read);
+
+/**
+ * intel_msic_reg_write - write a single MSIC register
+ * @reg: register to write
+ * @val: value to write to that register
+ *

drivers/mfd/intel_msic.c line 217

Quoted 10 lines

drivers/mfd/intel_msic.c new file +10 -0

+ * Write a single MSIC register. Returns 0 on success and negative
+ * errno in case of failure.
+ *
+ * Function may sleep.
+ */
+int intel_msic_reg_write(unsigned short reg, u8 val)
+{
+	return intel_scu_ipc_iowrite8(reg, val);
+}
+EXPORT_SYMBOL_GPL(intel_msic_reg_write);

What is the benefit of those wrappers since you're probably not going to get rid of the SCU IPC APIs, right ? What's wrong with the subdevices using the SCU IPC API directly ?

Quoted 4 lines

Right, we are not going to get rid of SCU IPC APIs. The idea behind having these register access wrapper functions is that we are now able to separate the MSIC subdevices from other SCU IPC usage. In other words we get a bit cleaner "architecture".

I'm not entirely convinced that wrapping around the IPC API gives you a cleaner architecture.

Quoted 4 lines

It also allows us to add caching and intercepting register accesses if a need rises.

That would define a real need for this API, yes.

Quoted 9 lines

Quoted 8 lines

drivers/mfd/intel_msic.c line 343

Quoted 5 lines

drivers/mfd/intel_msic.c new file +3 -0

+	for (i = 0; i < ARRAY_SIZE(msic_devs); i++) {
+		if (!pdata->irq[i])
+			continue;

I would expect some sort of warning here since it would mean your platform code defined a sub device platform data without giving you the right IRQ. And afaiu, all of your sub devices must have one.

Quoted 7 lines

The interface for platform data says that if irq is zero, it means that no platform device is created.

For example in patch 3/3 we add the platform data for battery, gpio, audio, power_button and ocd but the SFI table actually contains more devices. We don't yet have (upstream) driver for those so there is a little point of creating platform device for them at this point.

Ok, I see.

So, I'm fine with this patch, at least the MFD part. I'll go ahead and apply it to my for-next branch. I'd appreciate to get ACKs or NACKs for the x86 parts from the relevant people though.

The MSIC MFD driver creates platform devices for MSIC device drivers so we don't need to create them in platform code anymore.

This patch adds a new runtime check which determines whether we are running on a Medfield platform and enables the MSIC MFD driver accordingly.

Hi Samuel,

Quoted 5 lines · Mika Westerberg

The MSIC MFD driver creates platform devices for MSIC device drivers so we don't need to create them in platform code anymore.

This patch adds a new runtime check which determines whether we are running on a Medfield platform and enables the MSIC MFD driver accordingly.

There is a fix in 3.1 which conflicts this patch. The fix is 153b19a3b9fd8b (x86, mrst: use a temporary variable for SFI irq) which is needed for Medfield platforms to get kexec working.

If you like, I can send you an updated version of this patch which doesn't conflict.

The MSIC MFD driver creates platform devices for MSIC device drivers so we don't need to create them in platform code anymore.

This patch adds a new runtime check which determines whether we are running on a Medfield platform and enables the MSIC MFD driver accordingly.

Signed-off-by Mika Westerberg <[email protected]>