[PATCH] platform: Facilitate the creation of pseduo-platform busses

Previous thread: Compat-wireless for 2.6.35 final - 802.11, Bluetooth, Ethernet backported by Luis R. Rodriguez on Wednesday, August 4, 2010 - 2:50 pm. (1 message)

Next thread: [GIT PULL] Xen updates by Jeremy Fitzhardinge on Wednesday, August 4, 2010 - 3:15 pm. (1 message)
From: Patrick Pannuto
Date: Wednesday, August 4, 2010 - 3:14 pm

Inspiration for this comes from:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html

RFC: http://lkml.org/lkml/2010/8/3/496
Patch is unchanged from the RFC. Reviews seemed generally positive
and it seemed this was desired functionality.

INTRO

As SOCs become more popular, the desire to quickly define a simple,
but functional, bus type with only a few unique properties becomes
desirable. As they become more complicated, the ability to nest these
simple busses and otherwise orchestrate them to match the actual
topology also becomes desirable.

EXAMPLE USAGE

/arch/ARCH/MY_ARCH/my_bus.c:

	#include <linux/device.h>
	#include <linux/platform_device.h>

	struct bus_type my_bus_type = {
		.name	= "mybus",
	};
	EXPORT_SYMBOL_GPL(my_bus_type);

	struct platform_device sub_bus1 = {
		.name		= "sub_bus1",
		.id		= -1,
		.dev.bus	= &my_bus_type,
	}
	EXPORT_SYMBOL_GPL(sub_bus1);

	struct platform_device sub_bus2 = {
		.name		= "sub_bus2",
		.id		= -1,
		.dev.bus	= &my_bus_type,
	}
	EXPORT_SYMBOL_GPL(sub_bus2);

	static int __init my_bus_init(void)
	{
		int error;
		platform_bus_type_init(&my_bus_type);

		error = bus_register(&my_bus_type);
		if (error)
			return error;

		error = platform_device_register(&sub_bus1);
		if (error)
			goto fail_sub_bus1;

		error = platform_device_register(&sub_bus2);
		if (error)
			goto fail_sub_bus2;

		return error;

	fail_sub_bus2:
		platform_device_unregister(&sub_bus1);
	fail_sub_bus2:
		bus_unregister(&my_bus_type);

		return error;
	}
	postcore_initcall(my_bus_init);
	EXPORT_SYMBOL_GPL(my_bus_init);

/drivers/my_driver.c
	static struct platform_driver my_driver = {
		.driver	= {
			.name	= "my-driver",
			.owner	= THIS_MODULE,
			.bus	= &my_bus_type,
		},
	};

/somewhere/my_device.c
	static struct platform_device my_device = {
		.name		= "my-device",
		.id		= -1,
		.dev.bus	= &my_bus_type,
		.dev.parent	= &sub_bus_1.dev,
	};

Notice that for a device / driver, only 3 lines ...
From: Kevin Hilman
Date: Wednesday, August 4, 2010 - 5:16 pm

Also, later in that thread I also wrote[1] what seems to be the core of
what you've done here: namely, allow platform_devices and
platform_drivers to to be used on custom busses.  Patch is at the end of
this mail with a more focused changelog.  As Greg suggested in his reply
to your first version, this part could be merged today, and the
platform_bus_init stuff could be added later, after some more review.

Except it requires a re-compile.

Rather than doing this at compile time, it would be better to support
legacy devices at runtime.  You could handle this by simply registering
the driver on the custom bus and the platform_bus and let the bus
matching code handle it.  Then, the same binary would work on both



With this approach, you should note in the comments/changelog that
any selective customization of the bus PM methods must be done after 
calling platform_bus_type_init().

Also, You've left out the legacy PM methods here.  That implies that
moving a driver from the platform_bus to the custom bus is not entirely
transparent.  If the driver still has legacy PM methods, it would stop
working on the custom bus.

While this is good motivation for converting a driver to dev_pm_ops, at
a minimum it should be clear in the changelog that the derivative busses
do not support legacy PM methods.  However, since it's quite easy to do,
and you want the derivative busses to be *exactly* like the platform bus
except where explicitly changed, I'd suggest you also check/copy the
legacy PM methods.

In addition, you've missed several fields in 'struct bus_type'
(bus_attr, drv_attr, p, etc.)  Without digging deeper into the driver
core, I'm not sure if they are all needed at init time, but it should be
clear in the comments why they can be excluded.

Kevin

[1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31289.html


From b784009af1d0a7065dc5e58a13ce29afa3432d3e Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@deeprootsystems.com>
Date: Mon, 28 Jun 2010 ...
From: Patrick Pannuto
Date: Wednesday, August 4, 2010 - 5:57 pm

I can split this into 2 patches.


Can you safely register a driver on more than one bus? I didn't think
that was safe -- normally it's impossible since you're calling

struct BUS_TYPE_driver mydriver;
BUS_TYPE_driver_register(&mydriver)

but now we have multiple "bus types" that are all actually platform type; still,
at a minimum you would need:
	struct platform_driver mydrvier1 = {
		.driver.bus = &sub_bus1,
	};
	struct platform_driver mydrvier2 = {
		.driver.bus = &sub_bus2,
	};
which would all point to the same driver functions, yet the respective devices
attached for the "same" driver would be on different buses. I fear this might
confuse some drivers. I don't think dynamic bus assignment is this easy

In short: I do not believe the same driver can be registered on multiple



No they don't. If you call platform_bus_type_init first then you'll
just overwrite them with new values; if you call it second then they

I copied everything that was defined for platform_bus_type:

struct bus_type platform_bus_type = {
	.name		= "platform",
	.dev_attrs	= platform_dev_attrs,
	.match		= platform_match,
	.uevent		= platform_uevent,
	.pm		= &platform_dev_pm_ops,
};
EXPORT_SYMBOL_GPL(platform_bus_type);

struct bus_type {
	const char		*name;
	struct bus_attribute	*bus_attrs;
	struct device_attribute	*dev_attrs;
	struct driver_attribute	*drv_attrs;

	int (*match)(struct device *dev, struct device_driver *drv);
	int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
	int (*probe)(struct device *dev);
	int (*remove)(struct device *dev);
	void (*shutdown)(struct device *dev);

	int (*suspend)(struct device *dev, pm_message_t state);
	int (*resume)(struct device *dev);

	const struct dev_pm_ops *pm;

	struct bus_type_private *p;
};

It is my understanding that everything that I did not copy *should* remain
unique to each bus; remaining fields will be filled in by bus_register and

If you would like to lead this effort, please do so; I did not mean to ...
From: Kevin Hilman
Date: Thursday, August 5, 2010 - 8:57 am

It is possible, and currently done in powerpc land where some
drivers handle devices on the platform_bus and the custom OF bus.

However, as noted by Magnus, what we really need here is a way for
drivers to not care at all what kind of bus they are on.  There are an
increasing number of drivers that are re-used not just across different
SoCs in the same family, but across totally different SoCs (e.g. drivers

Ah, OK.   The changelog was missing credits to that affect, but I was
more concerned that you hadn't seen my example and didn't want to be


Right, they will not be overwritten, but you'll be left with a mostly
empty dev_pm_ops on the custom bus.

IOW, Most of these custom busses will only want to customize a small
subset of the dev_pm_ops methods (e.g. only the runtime PM methods.)  If
you setup your sparsly populated custom dev_pm_ops and then call
platform_bus_type_init() second, dev_pm_ops on the new buswill have *only*
your custom fields, and none of the defaults from platform_dev_pm_ops.

So, what I was getting at is that it should probably be clearer to the
users of platform_bus_type_init() that any customization of dev_pm_ops



Great, I'm glad you forged ahead.  There is definitely a broader need
for something like this, and I have no personal attachment to the code.

I have no problems with you continuing the work (in fact, I'd prefer it.
I have lots of other things to catch up on after my vacation.)

In the future though, it's common (and kind) to note the original author
in the changelog when basing a patch on previous work.  Something like
"originally written by..."  or "based on the work of..." etc.

Thanks,

Kevin
--

From: Patrick Pannuto
Date: Thursday, August 5, 2010 - 9:31 am

Ok, I can do that; that was the intention of the "original inspiration from"
line at the beginning.  Is there a more formal way of indicating this in the
next version of the patch? Should I add you as a "From:" or an "Author:"?

-Pat


-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--

From: Kevin Hilman
Date: Thursday, August 5, 2010 - 3:24 pm

I don't know if there is an official way of doing this.

I typically add something like "based on the idea/code originally
proposed/written by John Doe" in the changelog, and then add Cc: John
Doe <...> in the changelog too, but AFAIK, none of this is formalized.

Kevin
--

From: Grant Likely
Date: Thursday, August 5, 2010 - 4:16 pm

On Thu, Aug 5, 2010 at 9:57 AM, Kevin Hilman

As of now, the of_platform_bus_type has been removed.  It was a bad
idea because it tried to encode non-bus-specific information into
something that was just a clone of the platform_bus.  Drivers that
worked on both had to be bound to both busses.  I do actually have
code that automatically registers a driver on more than one bus, but
it is rather a hack and was only a temporary measure.

The relevant question before going down this path is, "Is the
omap/sh/other-soc behaviour something fundamentally different from the
platform bus?  Or is it something complementary that would be better
handled with a notifier or some orthogonal method of adding new
behaviour?"

I don't have a problem with multiple platform_bus instances using the
same code (I did suggest it after all), but I do worry about muddying
the Linux device model or making it overly complex.  Binding single
drivers to multiple device types could be messy.

Cheers,
g.
--

From: Patrick Pannuto
Date: Thursday, August 5, 2010 - 6:25 pm

From your other email, the distinction of /bus_types/ versus /physical
or logical buses/ was very valuable (thanks). I am becoming less
convinced that I need this infrastructure or the ability to create
pseudo-platform buses. Let me outline some thoughts below, which I
think at least solves my problems ( ;) ), and if some of the OMAP folks
and Alan could chime in as to whether they can apply something similar,
or if they still have need of creating actual new bus types?


As we are exploring all of this, let me put a concrete (ish) scenario
out there to discuss:

        SUB_BUS1---DEVICE1
       /
CPU ---
       \
        SUB_BUS2---DEVICE2

DEVICE1 and DEVICE2 are the same device (say, usb host controller, or
whatever), and they should be driven by the same driver. However,
physically they are attached to different buses.

SUB_BUS1 and SUB_BUS2 are *different* and let's say require a different
suspend method. If I understand correctly, the right way to build the
topology would be:

struct device_type SUB_BUS1_type = {
	.name = "sub-bus1-type",
	.pm   = &sub_bus1_pm_ops;
};

struct platform_device SUB_BUS1 = {
	.init_name = "sub-bus1",
	.dev.type  = &sub_bus1_type,
};

struct platform_device DEVICE1 = {
	.name       = "device1",
	.dev.parent = &SUB_BUS1.dev,
};

platform_device_register(&SUB_BUS1);
platform_device_register(&DEVICE1);

[analogous for *2]

which would yield:

/sys/bus/platform/devices/
 |
 |-SUB_BUS1
 |  |
 |  |-DEVICE1
 |
 |-SUB_BUS2
 |  |
 |  |-DEVICE2

Which is the correct topology, and logically provides all the correct
interfaces.  The only thing that becomes confusing now, is that SUB_BUS*
is *actually* a physically different bus, yet it's not in /sys/bus;
although I suppose this is actually how the world works presently (you
don't see an individual entry in /sys/bus for each usb host controller,
which is technically defining a sub-bus...)

Thoughts? Am I understanding everything correctly?

Is there anything more not ...
From: Grant Likely
Date: Friday, August 6, 2010 - 11:53 pm

Not quite.  The device_type is intended to collect similar, but
different things (ie, all keyboards, regardless of how they are
attached to the system).  Trying to capture the device-specific
behavour really belongs in the specific device driver attached to the
SUB_BUS* device.  In fact, the way you've constructed your example,
SUB_BUS1 and SUB_BUS2 don't really need to be platform_devices at all;
you could have used a plain struct device because in this example you
aren't binding them to a device driver).

That being said, because each bus *does* have custom behaviour, it
probably does make sense to either bind each to a device driver, or to
do something to each child device that changes the PM behaviour.  I
haven't dug into the problem domain deep enough to give you absolute
answers, but the devres approach looks promising, as does creating a
derived platform_bus_type (although I'm not fond of sharing device
drivers between multiple bus_types).  Another option might be giving
the parent struct device some runtime PM operations that it performs

You're looking in the wrong place.  Look in /sys/devices instead of

/sys/bus is for bus_types, not bus instances.  All USB devices will be
listed in /sys/bus/usb/devices regardless of the bus instance that
they attached to.

However, look carefully at the files in /sys/bus/*/devices.  Notice
that they are all symlinks?  Notice that the all of them point to
directories in /sys/devices?  /sys/bus tells you which devices are
registered against each bus type, but /sys/devices is the actual
structure.  Always look in /sys/devices when you want to know the
topology of the system.

Cheers,
g.
--

From: Magnus Damm
Date: Wednesday, August 4, 2010 - 7:32 pm

Thanks for your patch, it's really nice to see work done in this area!
I'd like to see something like this merged in the not so distant
future. At this point I'm not so concerned about the details, so I'll

I would really prefer not to have the bus type in the here. I
understand it's needed at this point, but I wonder if it's possible to
adjust the device<->driver matching for platform devices to allow any
type of pseudo-platform bus_type.

The reason why I'd like to avoid having the bus type in the driver is
that I'd like to reuse the platform driver across multiple
architectures and buses. For instance, on the SH architecture and
SH-Mobile ARM we have SoCs with SCIF hardware blocks driven by the
sh-sci.c serial driver. The sh-sci.c platform driver supports a wide
range of different SCI(F)(A)(B) hardware blocks, and on any given SoC
there is a mix of SCIF blocks spread out on different buses.

At this point our SH platform drivers are unaware where their driver
instanced are located on the SoC. The I/O address and IRQs are
assigned via struct resource and clocks are managed through clkdev. I
believe that adding the bus type in the driver will violate this
abstraction and make it more difficult to just instantiate a driver

This I don't mind at all. Actually, this is the place where the
topology should be defined IMO.

Cheers,

/ magnus
--

From: Kevin Hilman
Date: Thursday, August 5, 2010 - 8:27 am

I totally agree here.  Keeping the drivers ignorant of the bus (or SoC)
they are on will make them much more portable.

Kevin
--

From: Patrick Pannuto
Date: Thursday, August 5, 2010 - 10:43 am

So would I :). That's where this was all heading eventually, I was just
originally doing it in two passes. I have some ideas for how to do this



-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--

Previous thread: Compat-wireless for 2.6.35 final - 802.11, Bluetooth, Ethernet backported by Luis R. Rodriguez on Wednesday, August 4, 2010 - 2:50 pm. (1 message)

Next thread: [GIT PULL] Xen updates by Jeremy Fitzhardinge on Wednesday, August 4, 2010 - 3:15 pm. (1 message)