MFD cell structure and sharing of resources

MFD cell structure and sharing of resources

[Daniel Drake]

Hi Samuel,

I'm hoping you can give us your opinion on a design challenge we are facing.
You recently merged Andres's recent patches to convert cs5536 to a
MFD, with GPIO and MFGPT now as mfd cells.

We also have the olpc-xo1 driver specific to a specific laptop model,
providing power management functions, and this needs to use the cs5536
resources "acpi" and "pms".
https://patchwork.kernel.org/patch/278991/

At the same time, we are working on other improvements to olpc-xo1
(adding suspend/resume support). Due to review comments we are going
to rename this to olpc-xo1-pm and make it builtin only.

Then we are going to add a new driver, perhaps named olpc-xo1-sci,
which deals with things like power button input device, lid switch,
etc. This driver needs to use the "cs5536-acpi" resource which is also
used by olpc-xo1-pm.

So the question is: how can 2 drivers share this MFD resource?

We do not want to merge olpc-xo1-sci and olpc-xo1-pm, because
olpc-xo1-pm can only be builtin (but thankfully is small), and
olpc-xo1-sci will become a bit more bloated and we'd like to make that
modular (in the interests of generic distros).

A few options that spring to mind:

1. Adjust the list of mfd cells in drivers/mfd/cs5535-mfd so that
rather than being a list of resources, it is a list of drivers that
rely on the mfd driver. The entries would be: gpio, mfgpt,
olpc-xo1-pm, olpc-xo1-sci. olpc-xo1-pm would have the 2 resources it
needs (pms & acpi), and the acpi resource would be duplicated into
olpc-xo1-sci  too.

We'd then add a machine_is_olpc()  check inside cs5535-mfd so that the
OLPC-specific cells are only passed to mfd_add_devices on OLPC
laptops.

2. Continue with Andres' olpc-xo1-pm patch that makes that driver act
as a driver for both acpi & pms. When both resources are available, it
could probe an olpc-xo1-sci platform device as a child of itself,
having the ACPI resource shared on the platform_device level (similar
to how MFD shares resources via cells).

3. For olpc-xo1-pm and olpc-xo1-sci, forget about hooking into MFD and
go back to the route of looking for the appropriate device on the PCI
bus and accessing the regions that way.


Thoughts?

Thanks,
Daniel

Re: MFD cell structure and sharing of resources

[Daniel Drake]

On 10 December 2010 15:37, Daniel Drake <dsd@laptop.org> wrote:
> 3. For olpc-xo1-pm and olpc-xo1-sci, forget about hooking into MFD and
> go back to the route of looking for the appropriate device on the PCI
> bus and accessing the regions that way.

That isn't really an option either, because pci_request_region makes
the region exclusive to 1 device.
However, something similar is possible: olpc-xo1-pm reserves the
region and makes it enable to olpc-xo1-sci through an exported symbol.

Daniel

Re: MFD cell structure and sharing of resources

[Samuel Ortiz]

Hi Daniel,

On Fri, Dec 10, 2010 at 03:37:30PM +0000, Daniel Drake wrote:
> A few options that spring to mind:
> 
> 1. Adjust the list of mfd cells in drivers/mfd/cs5535-mfd so that
> rather than being a list of resources, it is a list of drivers that
> rely on the mfd driver. The entries would be: gpio, mfgpt,
> olpc-xo1-pm, olpc-xo1-sci. olpc-xo1-pm would have the 2 resources it
> needs (pms & acpi), and the acpi resource would be duplicated into
> olpc-xo1-sci  too.
> 
> We'd then add a machine_is_olpc()  check inside cs5535-mfd so that the
> OLPC-specific cells are only passed to mfd_add_devices on OLPC
> laptops.
> 
> 2. Continue with Andres' olpc-xo1-pm patch that makes that driver act
> as a driver for both acpi & pms. When both resources are available, it
> could probe an olpc-xo1-sci platform device as a child of itself,
> having the ACPI resource shared on the platform_device level (similar
> to how MFD shares resources via cells).
This sounds like an acceptable solution to me.

> 3. For olpc-xo1-pm and olpc-xo1-sci, forget about hooking into MFD and
> go back to the route of looking for the appropriate device on the PCI
> bus and accessing the regions that way.

I see a 4th solution though:

Define and export a set of I/O routines from the MFD driver, for the shared
resources. The routines would take as arguments one of the shared module
(ACPI, PMS, etc..) and a register (and obviously a value for the writing
parts...), and do the I/O operation for your driver. This is similar to what
the twl driver does, although over i2c.
That means requesting and releasing the shared regions from the MFD driver,
before knowing if there is a user for it or not. I realize this could lead to
some resource conflicts, although this is very unlikely. Potentially, we could
have a callback into the MFD driver from the subdevice to let it know that the
resource is requested. But then your #2 solution would look better to me.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: MFD cell structure and sharing of resources

[Daniel Drake]

[-- Attachment #1: Type: text/plain, Size: 3119 bytes --]

Hi Samuel,

Thanks for your valuable input here!

On 16 December 2010 10:35, Samuel Ortiz <sameo@linux.intel.com> wrote:
>> 2. Continue with Andres' olpc-xo1-pm patch that makes that driver act
>> as a driver for both acpi & pms. When both resources are available, it
>> could probe an olpc-xo1-sci platform device as a child of itself,
>> having the ACPI resource shared on the platform_device level (similar
>> to how MFD shares resources via cells).
> This sounds like an acceptable solution to me.

This is also the one that stuck out to me.

There are complications though.


Firstly, passing down the resources from mfd to olpc-xo1-pm, then from
olpc-xo1-pm using "struct resource" doesn't work - the 2nd passing
down fails because the platform layer checks that the resources are
free.

This can be worked around by using platform_data to pass the required
info from olpc-xo1-pm to olpc-xo1-sci (the base register addresses).


Secondly, the olpc-xo1-pm driver is going to have a couple of sysfs
nodes that will be used by userspace.

Under the current design they appear as e.g.:
/sys/devices/pci0000:00/0000:00:0f.0/cs5535-acpi/wakeup_reason

However, it only appears under cs5535-acpi because that's the device
that appeared second (out of acpi + pms). If the probe order ever
changed, the path would change too.
This could be worked around by storing both pointers (acpi + pms) and
choosing one that the olpc-xo1-pm parts will always live under. But as
this detail represents an interface to userspace we should be careful
and try and get it right first time. That wakeup_reason node is not
really related to cs5535, it's an OLPC-specific thing (since the
wakeups can be caused by things totally separate from the geode hw).
So I'd feel a lot more comfortable if that path was less related to
cs5535.

This problem extends to olpc-xo1-sci which becomes a child of
olpc-xo1-pm, and creates another bunch of sysfs nodes e.g.
/sys/devices/pci0000:00/0000:00:0f.0/cs5535-acpi/olpc-xo1-sci/lid_wake_mode



I have one solution in mind but I'm not sure if it goes beyond your
definition of what a mfd cell should be:

cs5535-mfd creates and registers a MFD cell specifically for
olpc-xo1-pm if it finds itself running on an XO laptop. The cell has
the 2 resources that are needed by that driver, and has name
"olpc-xo1-pm". Therefore our sysfs paths start with:
/sys/devices/pci0000:00/0000:00:0f.0/olpc-xo1-pm/

olpc-xo1-pm is then simplified and doesn't have to pretend to be a
driver for 2 devices. It just waits for that mfd cell to cause it to
be probed, then immediately has both resources available to it.

The first problem is also solved (clean sharing of resources between
olpc-xo1-pm and olpc-xo1-sci). olpc-xo1-pm creates olpc-xo1-sci as a
child device, therefore olpc-xo1-sci can access the resources of its
parent as follows:
    platform_get_resource(to_platform_device(pdev->dev.parent),
IORESOURCE_IO, 0);

Attaching a cs5535-mfd patch to illustrate what I mean a bit clearer
(compile tested only). The OLPC-specific code in cs5536-mfd compiles
away to nothing on CONFIG_OLPC=n

Thoughts?

Daniel

[-- Attachment #2: mfd-scratch.txt --]
[-- Type: text/plain, Size: 4798 bytes --]

diff --git a/arch/x86/platform/olpc/olpc-xo1.c b/arch/x86/platform/olpc/olpc-xo1.c
index 1277756..4723139 100644
--- a/arch/x86/platform/olpc/olpc-xo1.c
+++ b/arch/x86/platform/olpc/olpc-xo1.c
@@ -19,7 +19,7 @@
 #include <asm/io.h>
 #include <asm/olpc.h>
 
-#define DRV_NAME "olpc-xo1"
+#define DRV_NAME "olpc-xo1-pm"
 
 /* PMC registers (PMS block) */
 #define PM_SCLK		0x10
@@ -72,17 +72,23 @@ static int __devinit olpc_xo1_probe(struct platform_device *pdev)
 		return -EIO;
 	}
 
-	if (strcmp(pdev->name, "cs5535-pms") == 0)
-		pms_base = res->start;
-	else if (strcmp(pdev->name, "cs5535-acpi") == 0)
-		acpi_base = res->start;
+	pms_base = res->start;
 
-	/* If we have both addresses, we can override the poweroff hook */
-	if (pms_base && acpi_base) {
-		pm_power_off = xo1_power_off;
-		printk(KERN_INFO "OLPC XO-1 support registered\n");
+	res = platform_get_resource(pdev, IORESOURCE_IO, 1);
+	if (!res) {
+		dev_err(&pdev->dev, "can't fetch device resource info\n");
+		return -EIO;
 	}
 
+	if (!request_region(res->start, resource_size(res), DRV_NAME)) {
+		dev_err(&pdev->dev, "can't request region\n");
+		return -EIO;
+	}
+
+	acpi_base = res->start;
+
+	pm_power_off = xo1_power_off;
+	printk(KERN_INFO "OLPC XO-1 support registered\n");
 	return 0;
 }
 
@@ -93,27 +99,19 @@ static int __devexit olpc_xo1_remove(struct platform_device *pdev)
 	r = platform_get_resource(pdev, IORESOURCE_IO, 0);
 	release_region(r->start, resource_size(r));
 
-	if (strcmp(pdev->name, "cs5535-pms") == 0)
-		pms_base = 0;
-	else if (strcmp(pdev->name, "cs5535-acpi") == 0)
-		acpi_base = 0;
+	r = platform_get_resource(pdev, IORESOURCE_IO, 1);
+	release_region(r->start, resource_size(r));
+
+	pms_base = 0;
+	acpi_base = 0;
 
 	pm_power_off = NULL;
 	return 0;
 }
 
-static struct platform_driver cs5535_pms_drv = {
+static struct platform_driver xo1_pm_drv = {
 	.driver = {
-		.name = "cs5535-pms",
-		.owner = THIS_MODULE,
-	},
-	.probe = olpc_xo1_probe,
-	.remove = __devexit_p(olpc_xo1_remove),
-};
-
-static struct platform_driver cs5535_acpi_drv = {
-	.driver = {
-		.name = "cs5535-acpi",
+		.name = DRV_NAME,
 		.owner = THIS_MODULE,
 	},
 	.probe = olpc_xo1_probe,
@@ -122,28 +120,17 @@ static struct platform_driver cs5535_acpi_drv = {
 
 static int __init olpc_xo1_init(void)
 {
-	int r;
-
-	r = platform_driver_register(&cs5535_pms_drv);
-	if (r)
-		return r;
-
-	r = platform_driver_register(&cs5535_acpi_drv);
-	if (r)
-		platform_driver_unregister(&cs5535_pms_drv);
-
-	return r;
+	return platform_driver_register(&xo1_pm_drv);
 }
 
 static void __exit olpc_xo1_exit(void)
 {
-	platform_driver_unregister(&cs5535_acpi_drv);
-	platform_driver_unregister(&cs5535_pms_drv);
+	platform_driver_unregister(&xo1_pm_drv);
 }
 
 MODULE_AUTHOR("Daniel Drake <dsd@laptop.org>");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:olpc-xo1");
+MODULE_ALIAS("platform:" DRV_NAME);
 
 module_init(olpc_xo1_init);
 module_exit(olpc_xo1_exit);
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3a7b891..203245d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -540,7 +540,7 @@ config AB3550_CORE
 config MFD_CS5535
 	tristate "Support for CS5535 and CS5536 southbridge core functions"
 	select MFD_CORE
-	depends on PCI
+	depends on PCI && X86
 	---help---
 	  This is the core driver for CS5535/CS5536 MFD functions.  This is
           necessary for using the board's GPIO and MFGPT functionality.
diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 59ca6f1..9ef5dd9 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -28,6 +28,8 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 
+#include <asm/olpc.h>
+
 #define DRV_NAME "cs5535-mfd"
 
 enum cs5535_mfd_bars {
@@ -74,6 +76,15 @@ static __devinitdata struct mfd_cell cs5535_mfd_cells[] = {
 	},
 };
 
+static __devinitdata struct resource cs5535_mfd_resources_xo1[2];
+
+static __devinitdata struct mfd_cell cs5535_mfd_cell_xo1 = {
+	.id = -1,
+	.name = "olpc-xo1-pm",
+	.num_resources = 2,
+	.resources = cs5535_mfd_resources_xo1,
+};
+
 static int __devinit cs5535_mfd_probe(struct pci_dev *pdev,
 		const struct pci_device_id *id)
 {
@@ -106,8 +117,22 @@ static int __devinit cs5535_mfd_probe(struct pci_dev *pdev,
 	dev_info(&pdev->dev, "%zu devices registered.\n",
 			ARRAY_SIZE(cs5535_mfd_cells));
 
+	if (machine_is_olpc()) {
+		cs5535_mfd_resources_xo1[0] = cs5535_mfd_resources[PMS_BAR];
+		cs5535_mfd_resources_xo1[1] = cs5535_mfd_resources[ACPI_BAR];
+		err = mfd_add_devices(&pdev->dev, -1, &cs5535_mfd_cell_xo1, 1,
+				      NULL, 0);
+		if (err) {
+			dev_err(&pdev->dev,
+				"MFD add OLPC device fail: %d\n", err);
+			goto err_olpc;
+		}
+	}
+
 	return 0;
 
+err_olpc:
+	mfd_remove_devices(&pdev->dev);
 err_disable:
 	pci_disable_device(pdev);
 	return err;

Re: MFD cell structure and sharing of resources

[Samuel Ortiz]

Hi Daniel,

On Thu, Dec 16, 2010 at 03:38:52PM +0000, Daniel Drake wrote:
> Hi Samuel,
> 
> Thanks for your valuable input here!
Thanks for your patience. I've been swamped at work with other projects, sorry
for the delay.

> I have one solution in mind but I'm not sure if it goes beyond your
> definition of what a mfd cell should be:
> 
> cs5535-mfd creates and registers a MFD cell specifically for
> olpc-xo1-pm if it finds itself running on an XO laptop. The cell has
> the 2 resources that are needed by that driver, and has name
> "olpc-xo1-pm". Therefore our sysfs paths start with:
> /sys/devices/pci0000:00/0000:00:0f.0/olpc-xo1-pm/
That is stretching things a bit, but I'd be ok with that solution.
In theory, you wouldn't even need to check if you're on an XO laptop, except
for saving a few bytes at runtime.

One more thing: What about my proposal of defining IO routines from the MFD
driver, and using those from your subdevices driver ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: MFD cell structure and sharing of resources

[Andres Salomon]

On Fri, 24 Dec 2010 11:45:19 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:
[...]
> 
> One more thing: What about my proposal of defining IO routines from
> the MFD driver, and using those from your subdevices driver ?
> 
>

Having looked at the twl4030 driver, I do like that approach.  Is this
something you want to see for cs5535 only, or are enough other mfd
drivers doing this that you'd like the mfd framework itself to define
hooks for cell/resource sharing?

Re: MFD cell structure and sharing of resources

[Mark Brown]

On Fri, Dec 24, 2010 at 10:48:51PM -0800, Andres Salomon wrote:

> Having looked at the twl4030 driver, I do like that approach.  Is this
> something you want to see for cs5535 only, or are enough other mfd
> drivers doing this that you'd like the mfd framework itself to define
> hooks for cell/resource sharing?

This is extremely common - all the I2C/SPI attached embedded PMICs have
register access functions like this.  We have some factored out code for
this in ASoC but I'm not sure how worthwhile it is pulling that out into
something more generic as there's often a lot of special tweaks needed
for the register maps in PMICs (for things like locked registers),
though things like read/modify/write are quite common.

Re: MFD cell structure and sharing of resources

[Andres Salomon]

On Thu, 16 Dec 2010
15:38:52 +0000 Daniel Drake <dsd@laptop.org> wrote:
[...]
> 
> Secondly, the olpc-xo1-pm driver is going to have a couple of sysfs
> nodes that will be used by userspace.
> 
> Under the current design they appear as e.g.:
> /sys/devices/pci0000:00/0000:00:0f.0/cs5535-acpi/wakeup_reason
> 
> However, it only appears under cs5535-acpi because that's the device
> that appeared second (out of acpi + pms). If the probe order ever
> changed, the path would change too.
> This could be worked around by storing both pointers (acpi + pms) and
> choosing one that the olpc-xo1-pm parts will always live under. But as
> this detail represents an interface to userspace we should be careful
> and try and get it right first time. That wakeup_reason node is not
> really related to cs5535, it's an OLPC-specific thing (since the
> wakeups can be caused by things totally separate from the geode hw).
> So I'd feel a lot more comfortable if that path was less related to
> cs5535.

I'd just like to point out that power_kobj is available (from
linux/kobject.h) as an option.  If we're short on
options, /sys/power/wakeup_reason is a potential solution that
would be extremely simple for userspace to deal with.

Currently OLPC patches throw random strings like "lid" and "key press"
into wakeup_reason (well, wackup_source in the ancient patches that I'm
looking at), but one can imagine a more structured approach that's
more generally useful.  Perhaps a list of sources that are
registered, with each being a kobj - either defined specifically for
the purpose of being a wakeup source, or an existing device object
(eg, a wlan object for WOL events).

That said, I'm failing to recall why we need a sysfs file exporting a
wakeup reason in the first place; can't we just send netlink/udev events
when a wakeup occurs with the reason for the wakeup?