login
Search
This has been split into two patches. The first, trivial patch, has already been submitted and greg k-h has it queued up for after the merge window; it is included here only for clarity. The second patch contains everything interesting :) [PATCHv2 1/2] platform: Use drv->driver.bus instead of assuming platform_bus_type [PATCHv2 2/2] platform: Facilitate the creation of pseudo-platform buses --
(lkml.org seems to have lost August 3rd...) RFC: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01342.html v1: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01942.html Based on the idea and code originally proposed by Kevin Hilman: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html Changes from v1: * "Pseduo" buses are no longer init'd, they are [un]registered by: - pseudo_platform_bus_register(struct bus_type *) - pseudo_platform_bus_unregister(struct bus_type *) * These registrations [de]allocate a dev_pm_ops structure for the pseudo bus_type * Do not overwrite the parent if .bus is already set (that is, allow pseudo bus devices to be root devices) * Split into 2 patches: - 1/2: Already sent separately, but included here for clarity - 2/2: The real meat of the patch (this patch) 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 SOC_bus_type = { .name = "SOC-bus-type", }; EXPORT_SYMBOL_GPL(SOC_bus_type); struct platform_device SOC_bus1 = { .name = "SOC-bus1", .id = -1, .dev.bus = &SOC_bus_type; }; EXPORT_SYMBOL_GPL(SOC_bus1); struct platform_device SOC_bus2 = { .name = "SOC-bus2", .id = -2, .dev.bus = &SOC_bus_type; }; EXPORT_SYMBOL_GPL(SOC_bus2); static int __init SOC_bus_init(void) { int error; error = pseudo_platform_bus_register(&SOC_bus_type); if (error) return error; error = platform_device_register(&SOC_bus1); if (error) goto fail_bus1; error = platform_device_register(&SOC_bus2); if (error) goto ...
-- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum --
Sorry, been busy with the -rc1 merge stuff, I'll have more time to look at it next week. Well, after you address Grant's issues first :) thanks, greg k-h --
On Tue, Aug 10, 2010 at 5:49 PM, Patrick Pannuto
Hi Patrick,
Before acking this as something that should be merged, I'd like to see
some real device drivers converted to use this interface so I can
For safety, I think you'd want to have a separate add function for
each new platform bus type, and change the name of this function to
explicitly pass the bus type:
int __platform_device_add(struct platform_device *pdev)
{
if (!pdev->dev.bus)
return -EINVAL;
[... existing code ...]
}
int platform_device_add(struct platform_device *pdev)
{
if (!pdev->dev.parent)
pdev->dev.parent = &platform_bus;
pdev->dev.bus = &platform_bus_type;
__platform_device_add(pdev);
}
And then for each bus type (in this example, I'll call it
foo_bus_type, and assume foo_device wraps platform_device):
int foo_device_add(struct foo_device *foo)
{
foo->pdev.dev.bus = &foo_bus_type;
__platform_device_add(&foo->pdev);
}
That will allow the new bus_type code to keep foo_bus_type as an
unexported static and will provide a bit more type safety. For
example, it avoids accidentally registering a plain (unwrapped)
Ditto here; better to have a separate foo_driver_register() for each
Kerneldoc style error. Should be:
+/**
Nit: heap_pm is an odd name. Typically Linux local vars are named
according to what they are, not where the memory is allocated from.
Ick, this is a nasty list of conditional statements. :-) Instead it
could have an unconditional initialize function which sets it up just
like the platform bus without registering. Then allow the
foo_bus_type initialization code override the ones it cares about, and
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
Yes, this makes sense. Originally, I was trying to avoid this due
to a misguided notion of backwards compatibility - namely that
devices could register themselves conditionally via
.bus = HAVE_FOO_BUS,
and always call the same platform_[device|driver]_register, but
thinking about this more, such a compile (or run)-time decision can
just as easily be made in the foo_[device|driver]_register. The
impact on legacy code is the same either way, but your suggested
interface is much better.
No, this won't work. (Unless, every pseudo_bus_type author did this same
kludge to work around const - better to do once IMHO)
struct bus_type {
...
const struct dev_pm_ops *pm;
...
};
That const is there to *prevent* device/driver writers from overriding
pm ops on accident, and to that end, it has value. What we would really
like here is 'const after registered' semantics, where the bus author
can fill in half the structure, and the platform code can fill in the
rest. However, allowing subclasses to modify private data elements isn't
possible in C :)
I believe the 'const' here provides valuable safety. My original attempt
at doing this removed the const, and I overwrote one of the pointers in
platform_dev_pm_ops on my first try at implementing it!
This was the best solution I could come up with while preserving the const
nature of the pm_ops and introducing minimal complexity / potential to
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
On Mon, Aug 16, 2010 at 12:47 PM, Patrick Pannuto Actually, I also made a kerneldoc style error here because the braces are missing. This should be: +/** + * pseudo_platform_bus_register() - register an "almost platform bus" See Documentation/kernel-doc-nano-HOWTO.txt for details. I'm also not fond of the "pseudo" name. It isn't really a pseudo bus, but rather a different bus type that happens to inherit most of the platform_bus infrastructure. It may be better just to expose the platform_bus helper functions without any reference to pseudo, and let the users be responsible for any naming differentiation. This is particularly true if the custom bus becomes responsible for registering itself and only the initialization functions are exposed. Okay then, here is a better approach: Don't do any allocation in this function. Just initialize the bus_type exactly the way it is initialized for the platform bus and assign the default dev_pm_ops. If the custom bus needs to override them, then it should be responsible to kmemdup the default dev_pm_ops structure and modify the Yes, overriding the const is a bad idea and you'd be wrong to override it. :-) Cheers, g. --
Actually you can do:
const struct dev_pm_ops test = {
DEFAULT_PLATFORM_PM_OPS,
.prepare = my_func,
...
};
where:
#define DEFAULT_PLATFORM_PM_OPS \
.prepare = platform_pm_prepare, \
.complete = platform_pm_complete, \
...
In case of repeated field assignments, gcc uses the last value (as per
http://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html).
Best Regards,
Michał Mirosław
--
Hmm...
To me this seems like a really painful implementation of what the OpenFirmware-esque "Flattened Device Tree" does on many embedded systems.
For example, to build an equivalent device tree using an OpenFirmware FDT file, I'd just use this:
/dts-v1/;
/ {
#address-cells = <1>;
#size-cells = <1>;
[...snip...]
soc-bus-1@fc000000 {
/* of_platform driver matches against this: */
compatible = "my-company-name,soc-bus-type";
/* Define base address and size of the bus */
reg = <0xfc000000 0x01000000>;
#address-cells = <1>;
#size-cells = <1>;
/*
* Define logical memory mapping relative to the bus addr:
* First field is the relative base address for children,
* second field is the address in the parent's memory map,
* third field is the size of the range.
*/
ranges = <0x0 0xfc000000 0x01000000>;
/* Now for sub-devices */
my-device@0x10000 {
compatible = "my-company-name,my-driver";
reg = <0x10000 0x100>; /* Address and size */
};
};
soc-bus-2@fd000000 {
/* of_platform driver matches against this: */
compatible = "my-company-name,soc-bus-type";
/* Define base address and size of the bus */
reg = <0xfd000000 0x01000000>;
#address-cells = <1>;
#size-cells = <1>;
/*
* Define logical memory mapping relative to the bus addr:
* First field is the relative base address for children,
* second field is the address in the parent's memory map,
* third field is the size of the range.
*/
ranges = <0x0 0xfd000000 0x01000000>;
};
};
If you don't need to actually do anything special at the bus level, you can just:
static struct of_device_id soc_bus_ids[] = {
{ .compatible = "soc-bus-type", },
{},
};
of_platform_bus_probe(NULL, &soc_bus_ids, NULL);
Any of_platform driver that matches something on one of those busses is automatically probed. Alternatively, if you need special bus behavior:
static struct of_device_id soc_bus_ids[] = {
{ .compatible = ...Patrick and Kevin have been trying to solve a different problem. The FDT is really good at describing the topology and interconnections of the system, but it doesn't solve the problem of how the different bus behaviour is implemented in the kernel. They need a method of registering devices that have subtly different behaviour from the plain-vanilla platform bus. Whether or not the device data originates from the FDT is irrelevant, and the problem remains the same. I'm not convinced (yet) that this is the right approach, and I'd like to see a few sample drivers converted to the new approach. Creating new bus_types that "inherit" from the platform_bus is actually not a bad idea, and it is an elegant way to change the behaviour (for example, how power management is implemented) for devices connected in a different way. A problem with the approach that Kevin pointed out is that drivers that need to work on both the platform_bus_type and the new soc_bus_type must explicitly register themselves on both bus types. There is no mechanism to allow drivers from one bus type to also be made available to another bus type. Certainly it would be possible to invent a mechanism, but the more I think about it, them more I think it will be a bad idea. The runtime-PM use-case that kicked this discussion off makes the assumption that a driver will behave identically when attached to either the platform_bus_type or the soc_bus_type. However, I think that in the general case that assumption will prove to be false. I strongly suspect that the new bus type will turn out to be not as similar to the platform_bus_type as originally assumed and that there will still be bus-type-specific impact on device drivers (but I digress; this paragraph is more directed to Patrick and Kevin, and doesn't address your comments). Be careful! This is actually a confusing example. Linux already has a concept of a bus_type, and it is not the same as what is shown in this example. Part of the problem is ...
And potentially more than 2 if a driver exists for a hardware block on different SoCs. Magnus raised this issue for drivers used across SH and ARM/SH-Mobile, and the same issue exists between TI OMAP and TI DaVinci What makes you suspect that? Maybe Patrick has other use-cases in mind, but from a PM perspective, I have not seen any impact. One of the primary goals of this (at least for me and it seems Magnus also) is to keep drivers ignorant of their bus type, and any bus-type-specific code should be done in the bus_type implementation. Both for SH and OMAP, we've been using the (admiteddly broken) weak-symbol-override approach to getting a custom bus-type based on the platform_bus. We've been using that in OMAP for a while now and have not seen any need to for the drivers to know if they are on the vanilla platform_bus or the customized one. I'm very curious to hear what type of impact you expect to the drivers. Kevin --
On Thu, Aug 19, 2010 at 1:20 PM, Kevin Hilman Call it a gut reaction if you like, but here's my line of thinking. In your use case, sure, there is zero impact on drivers, but will that continue to be the case for other bus architectures as they use this interface? Or, in other words, will this solution end up being short sighted and we'll be having this discussion all over again in a year? The big concern for me is the idea of partial divergence from the platform_bus_type (specifically having different bus_type, but same pool of drivers). For starts, how is it represented in the device model? Where do the device drivers live in /sys/bus/*/drivers? What happens when one bus type is unregistered but has drivers used by another bus type? Does it scale to the (granted, currently speculative) use-case where the device driver does need knowledge about which bus_type it is registered against? I'm also concerned about the complexity required to implement driver sharing and ensuring that correct locking and consistency is maintained between bus_types. Note: I consider partial divergence to be different from platform_bus_type inheritance which Patrick has been implementing. I don't have a technical issue with inheritance (drivers aren't shared), I'm just not convinced it is the best solution yet. :-) My other thought is this: If from both the device perspective and the driver perspective the interface is exactly the same as the platform_bus_type, then platform_bus_type really is the correct interface. That says to me that putting the runtime PM behaviour into a new bus_type is abstracting at the wrong level, and a different method should be used to modify PM behaviour, either based on device model topology (ie, the parent implement's the child's runtime PM behaviour), or maybe a "struct dev_pm_ops *bus_pm" in the struct device. However, the number of device drivers affected is still very small, and I don't have concerns if each driver registers itself on multiple bus ...
Grant Likely <grant.likely@secretlab.ca> writes: Heh, now I feel like we're going around in circles. Remember, I never wanted to create add a new bus_type. Someone else [ahem] suggested Yes, I'm working on the devres approach to that now, as is Magnus for IMHO, simply overriding the few dev_pm_ops methods was the cleanest and simplest. Since we seem to be in agreement now that the a new bus may not the right abstraction for this (since we want it to be completely transparent to the drivers), I'll go back to the original design. No new bus types, keep the platform_bus as is, but simply override the few dev_pm_ops methods I care about. This is what is done on SH, SH-Mobile[1] and my original version for OMAP that started this conversation. Yes, the weak-symbol method of overriding is not scalable, but that's a separate issue from whether or not to create a new bus. I have a proposed fix for the weak which I'll post shortly. Kevin [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022411.html --
On Fri, Aug 20, 2010 at 12:51 PM, Kevin Hilman Hey! I didn't suggest it either! I believe that was Greg at ELC. I Okay. One constraint remains though: If you can override the dev_pm_ops on a per-device or per-device-parent basis, then you've got my support. If the override (even when fixed to work at runtime) applies to every device on the platform_bus_type, then I'll nack it. My concern here is that the SoC or platform support code doesn't get to "own" the platform_bus_type. Other drivers/code can register their own set of platform_devices, which may also need to perform their own dev_pm_ops override. If the override is global to the platform_bus_type, then the model will not scale. Cheers, g. --
On Fri, Aug 20, 2010 at 6:10 PM, Kevin Hilman Nope! Not a new requirement; this is the issue that this entire thread has been about. Hijacking the entire platform_bus_type behaviour for a particular bus is the wrong thing to do because there Oops, I didn't phrase that very well. What I meant was that SoC or platform support code must not be allowed to "own" or hijack the platform_bus_type for it's own purposes. What I wrote sounds like I Alright, I can agree to that. I do agree that the runtime override is far and away better than the weak symbol approach. However, there needs to be some very clear rules in place for users of the override, namely that users must understand that they are stewards of the platform_bus_type, and must take care to preserve the default behaviour for "uninteresting" devices. Also, the expectation should be that it is a temporary measure until a better abstraction is implemented. It might be that a separate bus_type is the way to go (if the multiple driver registration problem can be solved) or it might be a way to differentiate pm_ops either per-device or per-parent. I'm not sure, but I'm starting on an OMAP project soon, so I may very well end up working on this. Cheers, g. --
Completely agree here. Any overrides of the dev_pm_ops functions that do not also call the pm_generic_* functions (or their equivalents) should be suspect. Yes, I'll be glad to work on this too, but first I need to get through reviewing the backlog of all the OMAP drivers we're converting to runtime PM. Kevin --
In theory (although not *yet* in practice), a driver being passed to platform_driver_probe might have driver.bus set to something other than platform_bus_type. Locking drv->driver.bus is always correct. Signed-off-by: Patrick Pannuto <ppannuto@codeaurora.org> --- drivers/base/platform.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 4d99c8b..b69ccb4 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -539,12 +539,12 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv, * if the probe was successful, and make sure any forced probes of * new devices fail. */ - spin_lock(&platform_bus_type.p->klist_drivers.k_lock); + spin_lock(&drv->driver.bus->p->klist_drivers.k_lock); drv->probe = NULL; if (code == 0 && list_empty(&drv->driver.p->klist_devices.k_list)) retval = -ENODEV; drv->driver.probe = platform_drv_probe_fail; - spin_unlock(&platform_bus_type.p->klist_drivers.k_lock); + spin_unlock(&drv->driver.bus->p->klist_drivers.k_lock); if (code != retval) platform_driver_unregister(drv); -- 1.7.2 --
| Greg KH | Og dreams of kernels |
| Jens Axboe | [PATCH 31/33] Fusion: sg chaining support |
| Arnd Bergmann | Re: finding your own dead "CONFIG_" variables |
| Mark Brown | [PATCH 2/2] Subject: natsemi: Allow users to disable workaround for DspCfg reset |
| Tony Breeds | [LGUEST] Look in object dir for .config |
git: | |
| Brian Downing |