| Message ID | 20121126124534.18106.44137.stgit@build.warmcat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 26 Nov 2012, Andy Green wrote: > This adds a small optional API into drivers/base which deals with generating, > matching and registration of wildcard device paths. > > >From a struct device * you can generate a string like > > /platform/usbhs_omap/ehci-omap.0/usb1/1-1 > > which enapsulates the path of the device's connection to the board. > > These can be used to match up other assets, for example struct regulators, > that have been registed elsewhere with a device instance that is probed > asynchronously from the other board assets. > > If your device is on a bus, as it probably is, the device path will feature > redundant bus indexes that do not contain information about the connectivity. > > For example if more than one driver can generate devices on the same bus, > then the ordering of device probing will change the path, despite the > connectivity remains the same. > > For that reason, to get a deterministic path for matching, wildcards are > allowed. If your target device has the path > > /platform/usbhs_omap/ehci-omap.0/usb1/1-1 > > generated, in the asset you wish to match with it you can instead use > > /platform/usbhs_omap/ehci-omap.0/usb*/*-1 > > in order to only leave the useful, invariant parts of the path to match > against. Have you considered limiting these wildcards in various useful ways? In this example, the wildcard must match a string of decimal digits. (Furthermore the two wildcard strings will always match each other, although it's probably not necessary to add this sort of constraint.) I don't know what sort of matches people will want in the future. Perhaps one for hex digits, or one for arbitrary text with the exception of a few characters (such as '/' but perhaps others too). To do what you want for now, the match should be restricted to digits. > To avoid having to adapt every kind of "search by string" api to also use > the wildcards, the api takes the approach you can register your wildcard, > and if a matching path is generated for a device, the wildcard itself is > handed back as the device path instead. > > So if your board code or a (not yet done) DT binding registers this wildcard > > /platform/usbhs_omap/ehci-omap.0/usb* > > and the usb hub driver asks to generate its device path > > device_path_generate(dev, name, sizeof name); > > that is actually > > /platform/usbhs_omap/ehci-omap.0/usb1 > > then what will be returned is > > /platform/usbhs_omap/ehci-omap.0/usb* > > providing the same literal string for ehci-omap.0 hub no matter how many\ > usb buses have been registered before. > > This wildcard string can then be matched to regulators or other string- > searchable assets intended for the device on that hardware path. That's not how I would do it, at least, not for this application. I would register tuples containing a name, a pointer, and two callback routines. For example, ("/platform/usbhs_omap/ehci-omap.0/usb*", &omap_vhub_device, turn_on_regulator, turn_off_regulator) The when a device is registered whose path matches the string in such a tuple, the device core would know to invoke the first callback. The arguments would be a pointer to the device being registered and the pointer in the tuple. Similarly, the device core would invoke the second callback when the device is unregistered. There doesn't have to be anything in this scheme that's specific to hubs or even to USB. In particular, no changes to the hub driver would be needed. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote: > +struct device_path list; > +DEFINE_MUTEX(lock); Those are some very descriptive global variables you just created :( -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote: > This adds a small optional API into drivers/base which deals with generating, > matching and registration of wildcard device paths. > > >From a struct device * you can generate a string like > > /platform/usbhs_omap/ehci-omap.0/usb1/1-1 > > which enapsulates the path of the device's connection to the board. > > These can be used to match up other assets, for example struct regulators, > that have been registed elsewhere with a device instance that is probed > asynchronously from the other board assets. > > If your device is on a bus, as it probably is, the device path will feature > redundant bus indexes that do not contain information about the connectivity. Huh? A bus "index" does show the connectivity, well, one type of connectivity, but perhaps it's not the one _you_ happen to want at the moment. Which is fine, but I don't see why you want to try to figure this out using the device path in the first place, surely you have some other way that the hardware can describe itself to the kernel as to where it needs to be hooked up to? > For example if more than one driver can generate devices on the same bus, > then the ordering of device probing will change the path, despite the > connectivity remains the same. That's an expected thing, I don't see the issue here. > For that reason, to get a deterministic path for matching, wildcards are > allowed. If your target device has the path Wait, no, why would you want a deterministic path and have that hard-coded into the kernel here? You can't rely on that any more than userspace can, so let's not start making the mistake that lots of userspace programmers originally did when they started using sysfs please. We have learned from our past mistakes. > /platform/usbhs_omap/ehci-omap.0/usb1/1-1 > > generated, in the asset you wish to match with it you can instead use > > /platform/usbhs_omap/ehci-omap.0/usb*/*-1 > > in order to only leave the useful, invariant parts of the path to match > against. > > To avoid having to adapt every kind of "search by string" api to also use > the wildcards, the api takes the approach you can register your wildcard, > and if a matching path is generated for a device, the wildcard itself is > handed back as the device path instead. > > So if your board code or a (not yet done) DT binding registers this wildcard > > /platform/usbhs_omap/ehci-omap.0/usb* Device tree lists sysfs paths in it's descriptions? That's news to me. > and the usb hub driver asks to generate its device path > > device_path_generate(dev, name, sizeof name); > > that is actually > > /platform/usbhs_omap/ehci-omap.0/usb1 > > then what will be returned is > > /platform/usbhs_omap/ehci-omap.0/usb* > > providing the same literal string for ehci-omap.0 hub no matter how many\ > usb buses have been registered before. > > This wildcard string can then be matched to regulators or other string- > searchable assets intended for the device on that hardware path. Ah, here's the root of your problem, right? You need a way for your hardware to tell the kernel that you have a regulator attached to a specific device? Using the device path and hard-coding it into the kernel is not the proper way to solve this, sorry. Use some other unique way to describe the hardware, surely the hardware designers couldn't have been that foolish not to provide this [1]? thanks, greg k-h [1] Yeah, I know I'm being hopeful here, and probably wrong, but then you need to push back. We are not helpless here. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 26, 2012 at 11:22:06AM -0800, Greg KH wrote: > On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote: > > This adds a small optional API into drivers/base which deals with generating, > > matching and registration of wildcard device paths. > > > > >From a struct device * you can generate a string like > > > > /platform/usbhs_omap/ehci-omap.0/usb1/1-1 > > > > which enapsulates the path of the device's connection to the board. > > > > These can be used to match up other assets, for example struct regulators, > > that have been registed elsewhere with a device instance that is probed > > asynchronously from the other board assets. > > > > If your device is on a bus, as it probably is, the device path will feature > > redundant bus indexes that do not contain information about the connectivity. > > Huh? A bus "index" does show the connectivity, well, one type of > connectivity, but perhaps it's not the one _you_ happen to want at the > moment. Which is fine, but I don't see why you want to try to figure > this out using the device path in the first place, surely you have some > other way that the hardware can describe itself to the kernel as to > where it needs to be hooked up to? > > > For example if more than one driver can generate devices on the same bus, > > then the ordering of device probing will change the path, despite the > > connectivity remains the same. > > That's an expected thing, I don't see the issue here. > > > For that reason, to get a deterministic path for matching, wildcards are > > allowed. If your target device has the path > > Wait, no, why would you want a deterministic path and have that > hard-coded into the kernel here? You can't rely on that any more than > userspace can, so let's not start making the mistake that lots of > userspace programmers originally did when they started using sysfs > please. We have learned from our past mistakes. > > > /platform/usbhs_omap/ehci-omap.0/usb1/1-1 Oh, and further proof of this, there are patches floating around to drop the "platform" name from the sys/drivers/ tree, so your driver just broke if that goes through, showing you really don't want to be hard-coding sysfs paths in any type of logic. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 26 Nov 2012, Greg KH wrote: > Ah, here's the root of your problem, right? You need a way for your > hardware to tell the kernel that you have a regulator attached to a > specific device? Using the device path and hard-coding it into the > kernel is not the proper way to solve this, sorry. Use some other > unique way to describe the hardware, surely the hardware designers > couldn't have been that foolish not to provide this [1]? As far as I know, the kernel has no other way to describe devices. What about using partial matches? In this example, instead of doing a wildcard match against /platform/usbhs_omap/ehci-omap.0/usb* (which would fail if the "platform" part of the path changes), suppose the string "ehci-omap.0/usb*" could be associated with the usbhs_omap component somehow. Or even better, the string "usb*" could be associated with the ehci-omap.0 device. Then the path-matching code could restrict its attention to that part of the path and to devices below the specified one. Naming changes wouldn't be an issue, because the changes themselves would be made in the same source file that contains the partial path string. On the other hand, this may be way more general than we really need. For this particular case, all we need to do is take special action the first time any device is registered below the ehci-omap.0 platform device. There ought to be a more direct way to accomplish this, without using string pattern-matching or sysfs pathnames (and without adding overhead every time a device is added or deleted). I don't know if such an approach would be sufficiently general for future requirements, but it would solve the problem at hand. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 26, 2012 at 04:07:38PM -0500, Alan Stern wrote: > On Mon, 26 Nov 2012, Greg KH wrote: > > > Ah, here's the root of your problem, right? You need a way for your > > hardware to tell the kernel that you have a regulator attached to a > > specific device? Using the device path and hard-coding it into the > > kernel is not the proper way to solve this, sorry. Use some other > > unique way to describe the hardware, surely the hardware designers > > couldn't have been that foolish not to provide this [1]? > > As far as I know, the kernel has no other way to describe devices. > > What about using partial matches? In this example, instead of doing a > wildcard match against > > /platform/usbhs_omap/ehci-omap.0/usb* > > (which would fail if the "platform" part of the path changes), suppose > the string "ehci-omap.0/usb*" could be associated with the usbhs_omap > component somehow. Or even better, the string "usb*" could be > associated with the ehci-omap.0 device. Yes, all you really care about here is the ehci-omap.0 device, so why even search for this, you "know" where the device is, you just created it :) > Then the path-matching code could restrict its attention to that part > of the path and to devices below the specified one. Naming changes > wouldn't be an issue, because the changes themselves would be made in > the same source file that contains the partial path string. > > > On the other hand, this may be way more general than we really need. > For this particular case, all we need to do is take special action the > first time any device is registered below the ehci-omap.0 platform > device. There ought to be a more direct way to accomplish this, > without using string pattern-matching or sysfs pathnames (and without > adding overhead every time a device is added or deleted). > > I don't know if such an approach would be sufficiently general for > future requirements, but it would solve the problem at hand. And given that this is a very specific problem, and the changes are only needed for one piece of problem hardware, I suggest keeping the existing code that implements it. It's smaller, more specific to the exact platform, and we don't end up getting stuck with device paths to describe hardware that might change in the future in ways we do not anticipate (i.e. the platform change.) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/27/2012 03:12 AM, the mail apparently from Alan Stern included: > On Mon, 26 Nov 2012, Andy Green wrote: > >> This adds a small optional API into drivers/base which deals with generating, >> matching and registration of wildcard device paths. >> >> >From a struct device * you can generate a string like >> >> /platform/usbhs_omap/ehci-omap.0/usb1/1-1 >> >> which enapsulates the path of the device's connection to the board. >> >> These can be used to match up other assets, for example struct regulators, >> that have been registed elsewhere with a device instance that is probed >> asynchronously from the other board assets. >> >> If your device is on a bus, as it probably is, the device path will feature >> redundant bus indexes that do not contain information about the connectivity. >> >> For example if more than one driver can generate devices on the same bus, >> then the ordering of device probing will change the path, despite the >> connectivity remains the same. >> >> For that reason, to get a deterministic path for matching, wildcards are >> allowed. If your target device has the path >> >> /platform/usbhs_omap/ehci-omap.0/usb1/1-1 >> >> generated, in the asset you wish to match with it you can instead use >> >> /platform/usbhs_omap/ehci-omap.0/usb*/*-1 >> >> in order to only leave the useful, invariant parts of the path to match >> against. > > Have you considered limiting these wildcards in various useful ways? > In this example, the wildcard must match a string of decimal digits. > (Furthermore the two wildcard strings will always match each other, > although it's probably not necessary to add this sort of constraint.) > > I don't know what sort of matches people will want in the future. > Perhaps one for hex digits, or one for arbitrary text with the > exception of a few characters (such as '/' but perhaps others too). > > To do what you want for now, the match should be restricted to digits. I'm not sure what we'd use that for... it doesn't seem the biggest problem we have at the moment ^^ >> To avoid having to adapt every kind of "search by string" api to also use >> the wildcards, the api takes the approach you can register your wildcard, >> and if a matching path is generated for a device, the wildcard itself is >> handed back as the device path instead. >> >> So if your board code or a (not yet done) DT binding registers this wildcard >> >> /platform/usbhs_omap/ehci-omap.0/usb* >> >> and the usb hub driver asks to generate its device path >> >> device_path_generate(dev, name, sizeof name); >> >> that is actually >> >> /platform/usbhs_omap/ehci-omap.0/usb1 >> >> then what will be returned is >> >> /platform/usbhs_omap/ehci-omap.0/usb* >> >> providing the same literal string for ehci-omap.0 hub no matter how many\ >> usb buses have been registered before. >> >> This wildcard string can then be matched to regulators or other string- >> searchable assets intended for the device on that hardware path. > > That's not how I would do it, at least, not for this application. I > would register tuples containing a name, a pointer, and two callback > routines. For example, > > ("/platform/usbhs_omap/ehci-omap.0/usb*", &omap_vhub_device, > turn_on_regulator, turn_off_regulator) > > The when a device is registered whose path matches the string in such a > tuple, the device core would know to invoke the first callback. The > arguments would be a pointer to the device being registered and the > pointer in the tuple. Similarly, the device core would invoke the > second callback when the device is unregistered. > > There doesn't have to be anything in this scheme that's specific to > hubs or even to USB. In particular, no changes to the hub driver would > be needed. By just using paths, it's not restricted to regulators or binary operations on them but anything that needs a "floating" binding to another named kernel object can leverage it. -Andy
On 11/27/2012 03:16 AM, the mail apparently from Greg KH included: > On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote: >> +struct device_path list; >> +DEFINE_MUTEX(lock); > > Those are some very descriptive global variables you just created :( > I guess I can add the "static" if that will heal the emotional damage I caused. -Andy
On 11/27/2012 03:22 AM, the mail apparently from Greg KH included: > On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote: >> This adds a small optional API into drivers/base which deals with generating, >> matching and registration of wildcard device paths. >> >> >From a struct device * you can generate a string like >> >> /platform/usbhs_omap/ehci-omap.0/usb1/1-1 >> >> which enapsulates the path of the device's connection to the board. >> >> These can be used to match up other assets, for example struct regulators, >> that have been registed elsewhere with a device instance that is probed >> asynchronously from the other board assets. >> >> If your device is on a bus, as it probably is, the device path will feature >> redundant bus indexes that do not contain information about the connectivity. > > Huh? A bus "index" does show the connectivity, well, one type of > connectivity, but perhaps it's not the one _you_ happen to want at the > moment. Which is fine, but I don't see why you want to try to figure > this out using the device path in the first place, surely you have some > other way that the hardware can describe itself to the kernel as to > where it needs to be hooked up to? The bus index is like a counter, it shows logical connectivity inside Linux that isn't repeatable and doesn't reflect hardware routing information directly. >> For example if more than one driver can generate devices on the same bus, >> then the ordering of device probing will change the path, despite the >> connectivity remains the same. > > That's an expected thing, I don't see the issue here. Alan brought up in a thread here the last days, "wouldn't it be nice if we could arbitrarily bind regulators to asynchronously probed objects", and after discussion proposed this wildcard matching scheme to mask these generated bus numbers. >> For that reason, to get a deterministic path for matching, wildcards are >> allowed. If your target device has the path > > Wait, no, why would you want a deterministic path and have that > hard-coded into the kernel here? You can't rely on that any more than > userspace can, so let's not start making the mistake that lots of > userspace programmers originally did when they started using sysfs > please. We have learned from our past mistakes. I guess that is a fair point. I was going to say that it's no different than using any kernel API in code, which history proves is very mutable; people deal with it by changing the usages as they change the API definition. But it's complicated a bit by DTs meant to be more stable and these paths would turn up in the DT. In "platform" case though, a heuristic that leaving off the initial / and allowing matches anywhere along the path then to the end would get around it. >> /platform/usbhs_omap/ehci-omap.0/usb1/1-1 >> >> generated, in the asset you wish to match with it you can instead use >> >> /platform/usbhs_omap/ehci-omap.0/usb*/*-1 >> >> in order to only leave the useful, invariant parts of the path to match >> against. >> >> To avoid having to adapt every kind of "search by string" api to also use >> the wildcards, the api takes the approach you can register your wildcard, >> and if a matching path is generated for a device, the wildcard itself is >> handed back as the device path instead. >> >> So if your board code or a (not yet done) DT binding registers this wildcard >> >> /platform/usbhs_omap/ehci-omap.0/usb* > > Device tree lists sysfs paths in it's descriptions? That's news to me. It does not say that... >> and the usb hub driver asks to generate its device path >> >> device_path_generate(dev, name, sizeof name); >> >> that is actually >> >> /platform/usbhs_omap/ehci-omap.0/usb1 >> >> then what will be returned is >> >> /platform/usbhs_omap/ehci-omap.0/usb* >> >> providing the same literal string for ehci-omap.0 hub no matter how many\ >> usb buses have been registered before. >> >> This wildcard string can then be matched to regulators or other string- >> searchable assets intended for the device on that hardware path. > > Ah, here's the root of your problem, right? You need a way for your > hardware to tell the kernel that you have a regulator attached to a > specific device? Using the device path and hard-coding it into the > kernel is not the proper way to solve this, sorry. Use some other > unique way to describe the hardware, surely the hardware designers > couldn't have been that foolish not to provide this [1]? > > thanks, > > greg k-h > > [1] Yeah, I know I'm being hopeful here, and probably wrong, but then > you need to push back. We are not helpless here. Specific hardware information is something that's kept hidden away and private in the driver for good reasons. We could add elective, additional information at the driver for every physical interface it represented and match that way. But I am not sure the effort involved is repaid by the relatively few instances that need what is basically asynchronously probed platform_data. -Andy
On 11/27/2012 05:07 AM, the mail apparently from Alan Stern included: > On Mon, 26 Nov 2012, Greg KH wrote: > >> Ah, here's the root of your problem, right? You need a way for your >> hardware to tell the kernel that you have a regulator attached to a >> specific device? Using the device path and hard-coding it into the >> kernel is not the proper way to solve this, sorry. Use some other >> unique way to describe the hardware, surely the hardware designers >> couldn't have been that foolish not to provide this [1]? > > As far as I know, the kernel has no other way to describe devices. > > What about using partial matches? In this example, instead of doing a > wildcard match against > > /platform/usbhs_omap/ehci-omap.0/usb* > > (which would fail if the "platform" part of the path changes), suppose > the string "ehci-omap.0/usb*" could be associated with the usbhs_omap > component somehow. Or even better, the string "usb*" could be > associated with the ehci-omap.0 device. > > Then the path-matching code could restrict its attention to that part > of the path and to devices below the specified one. Naming changes > wouldn't be an issue, because the changes themselves would be made in > the same source file that contains the partial path string. Yes just dropping the starting / on the match and allowing a fragment to match further up the string would solve it. ehci-omap.0 won't appear down any other earlier device paths than the right one. > On the other hand, this may be way more general than we really need. > For this particular case, all we need to do is take special action the > first time any device is registered below the ehci-omap.0 platform > device. There ought to be a more direct way to accomplish this, > without using string pattern-matching or sysfs pathnames (and without > adding overhead every time a device is added or deleted). There are no sysfs pathnames involved here at all. Greg invented that. > I don't know if such an approach would be sufficiently general for > future requirements, but it would solve the problem at hand. We can get a notification about device creation and do things there. But the cost of that is like the tuple suggestion, they'll only be able to do a fixed thing like operate on regulators. Actually the targeted device may have arbitrary associated assets like generic GPIO to turn on a "flash" for built-in webcam controlled out-of-band. These also can be reached by known names. And the driver may wish to do things with them that are more complex than enable / disable or follow logical lifecycle of the hub or whatever. However when the guidance from Greg is that we can do nothing except complain at hardware designers for some reason I failed to quite identify, I fear we are moving deckchairs on the titanic. -Andy
On Tue, Nov 27, 2012 at 5:07 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 26 Nov 2012, Greg KH wrote: > >> Ah, here's the root of your problem, right? You need a way for your >> hardware to tell the kernel that you have a regulator attached to a >> specific device? Using the device path and hard-coding it into the >> kernel is not the proper way to solve this, sorry. Use some other >> unique way to describe the hardware, surely the hardware designers >> couldn't have been that foolish not to provide this [1]? > > As far as I know, the kernel has no other way to describe devices. > > What about using partial matches? In this example, instead of doing a > wildcard match against > > /platform/usbhs_omap/ehci-omap.0/usb* IMO, all matches mean the devices are inside the ehci-omap bus, so the direct/simple way is to enable/disable the regulators in the probe() and remove() of ehci-omap controller driver. On the other side, both the two LAN95xx USB devices(hub + ethernet) are simple self-powered device. Just like other self-powered devices, the power should be provided from external world, instead of hub driver itself. And it is doable to power on the devices before creating the specific ehci-omap usb bus inside ehci-omap driver. Thanks,
On Tue, 27 Nov 2012, Ming Lei wrote: > IMO, all matches mean the devices are inside the ehci-omap bus, so > the direct/simple way is to enable/disable the regulators in the probe() and > remove() of ehci-omap controller driver. When this discussion started, Felipe argued against such an approach. He pointed out that the same chip could be used in other platforms, and then the code added to ehci-omap.c would have to be duplicated in several other places. We should have a more generic solution in a more central location, like the device core. For example, each struct platform_device could have a list of resources to be enabled when the device is bound to a driver and disabled when the device is unbound. Such a list could include regulators, clocks, and whatever else people can invent. In this case, when it is created the ehci-omap.0 platform device could get an entry pointing to the LAN95xx's regulator. Then the regulator would automatically be turned on when the platform device is bound to the ehci-omap driver. How does this sound? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 27 Nov 2012, Andy Green wrote: > > I don't know if such an approach would be sufficiently general for > > future requirements, but it would solve the problem at hand. > > We can get a notification about device creation and do things there. > But the cost of that is like the tuple suggestion, they'll only be able > to do a fixed thing like operate on regulators. I'm not quite sure what you mean by that. _Any_ function is capable of doing only a fixed thing. > Actually the targeted device may have arbitrary associated assets like > generic GPIO to turn on a "flash" for built-in webcam controlled > out-of-band. These also can be reached by known names. And the driver > may wish to do things with them that are more complex than enable / > disable or follow logical lifecycle of the hub or whatever. Let's worry about that when it arises. > However when the guidance from Greg is that we can do nothing except > complain at hardware designers for some reason I failed to quite > identify, I fear we are moving deckchairs on the titanic. Greg's advice was simply not to rely on pathnames in sysfs because they aren't fixed in stone. That leaves plenty of other ways to approach this problem. Basically, what you want is for something related to device A (the regulator or the GPIO) to happen whenever device B (the ehci-omap.0 platform device) is bound to a driver. The most straightforward way to arrange this is for A's driver to have a callback that is invoked whenever B is bound or unbound. The most straightforward way to arrange _that_ is to allow each platform_device to have a list of callbacks. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote: > On Tue, 27 Nov 2012, Ming Lei wrote: > > > IMO, all matches mean the devices are inside the ehci-omap bus, so > > the direct/simple way is to enable/disable the regulators in the probe() and > > remove() of ehci-omap controller driver. > > When this discussion started, Felipe argued against such an approach. > He pointed out that the same chip could be used in other platforms, and > then the code added to ehci-omap.c would have to be duplicated in > several other places. > > We should have a more generic solution in a more central location, like > the device core. > > For example, each struct platform_device could have a list of resources > to be enabled when the device is bound to a driver and disabled when > the device is unbound. Such a list could include regulators, clocks, > and whatever else people can invent. > > In this case, when it is created the ehci-omap.0 platform device could > get an entry pointing to the LAN95xx's regulator. Then the regulator > would automatically be turned on when the platform device is bound to > the ehci-omap driver. > > How does this sound? That sounds much better, and it might come in handy for other types of devices than platform devices, so feel free to put this on the core 'struct device' instead if needed. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 28, 2012 at 12:30 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Tue, 27 Nov 2012, Ming Lei wrote: > >> IMO, all matches mean the devices are inside the ehci-omap bus, so >> the direct/simple way is to enable/disable the regulators in the probe() and >> remove() of ehci-omap controller driver. > > When this discussion started, Felipe argued against such an approach. > He pointed out that the same chip could be used in other platforms, and > then the code added to ehci-omap.c would have to be duplicated in > several other places. From Andy's implementation, the 'general' code is to enable/disable the regulators(patch 3/5), I am wondering if it is general enough, so the 'duplicated' code are just several lines of regulator_enable/regulator_disable, which should be implemented in many platform drivers. Also from my intuition, power domain should be involved in the problem, because these hard-wired and self-powered USB devices should have its own power domains, and the ehci-omap driver may enable/disable these power domains before adding the bus. > > We should have a more generic solution in a more central location, like > the device core. > > For example, each struct platform_device could have a list of resources > to be enabled when the device is bound to a driver and disabled when > the device is unbound. Such a list could include regulators, clocks, > and whatever else people can invent. > > In this case, when it is created the ehci-omap.0 platform device could > get an entry pointing to the LAN95xx's regulator. Then the regulator > would automatically be turned on when the platform device is bound to > the ehci-omap driver. The LAN95xx's regulator is still platform dependent(even for same LAN95xx USB devices on different platforms(omap, tegra, ..)) , so I am wondering why we don't enable it directly in probe() of ehci-omap.0 platform device? Thanks,
On 11/28/2012 12:37 AM, the mail apparently from Alan Stern included: > On Tue, 27 Nov 2012, Andy Green wrote: > >>> I don't know if such an approach would be sufficiently general for >>> future requirements, but it would solve the problem at hand. >> >> We can get a notification about device creation and do things there. >> But the cost of that is like the tuple suggestion, they'll only be able >> to do a fixed thing like operate on regulators. > > I'm not quite sure what you mean by that. _Any_ function is capable of > doing only a fixed thing. > >> Actually the targeted device may have arbitrary associated assets like >> generic GPIO to turn on a "flash" for built-in webcam controlled >> out-of-band. These also can be reached by known names. And the driver >> may wish to do things with them that are more complex than enable / >> disable or follow logical lifecycle of the hub or whatever. > > Let's worry about that when it arises. > >> However when the guidance from Greg is that we can do nothing except >> complain at hardware designers for some reason I failed to quite >> identify, I fear we are moving deckchairs on the titanic. > > Greg's advice was simply not to rely on pathnames in sysfs because they > aren't fixed in stone. That leaves plenty of other ways to approach > this problem. It's sage advice, but there is zero code provided in my patches that "relies on pathnames in sysfs". > Basically, what you want is for something related to device A (the > regulator or the GPIO) to happen whenever device B (the ehci-omap.0 > platform device) is bound to a driver. The most straightforward way to > arrange this is for A's driver to have a callback that is invoked > whenever B is bound or unbound. The most straightforward way to > arrange _that_ is to allow each platform_device to have a list of > callbacks. Sorry I didn't really understand this proposal yet. You want "A", the regulator, driver to grow a callback function that gets called when the targeted platform_device ("B", ehci-omap.0) probe happens. Could you expand what the callback prototype or new members in the struct might look like? It's your tuple thing or we pass it an opaque pointer that is the struct regulator * or suchlike? Throwing out the path stuff and limiting this to platform_device means you cannot bind to dynamically created objects like hub or anything downstream of a hub. So Felipe's identification of the hub as the happening place to do this is out of luck. -Andy
On 11/28/2012 01:22 AM, the mail apparently from Ming Lei included: > On Wed, Nov 28, 2012 at 12:30 AM, Alan Stern <stern@rowland.harvard.edu> wrote: >> On Tue, 27 Nov 2012, Ming Lei wrote: >> >>> IMO, all matches mean the devices are inside the ehci-omap bus, so >>> the direct/simple way is to enable/disable the regulators in the probe() and >>> remove() of ehci-omap controller driver. >> >> When this discussion started, Felipe argued against such an approach. >> He pointed out that the same chip could be used in other platforms, and >> then the code added to ehci-omap.c would have to be duplicated in >> several other places. > > From Andy's implementation, the 'general' code is to enable/disable > the regulators(patch 3/5), I am wondering if it is general enough, so the > 'duplicated' code are just several lines of regulator_enable/regulator_disable, > which should be implemented in many platform drivers. Fair enough; my main interest was in the device path side when writing the patches. I stuck enough in one place to confirm the series works on Panda case for power control. So long as it doesn't get obsessed with just regulators some common implementation that seems to be discussed will be better. (BTW let me take this opportunity to thank you for your great urbs-in-flight limiting patch on smsc95xx a while back) > Also from my intuition, power domain should be involved in the problem, > because these hard-wired and self-powered USB devices should have > its own power domains, and the ehci-omap driver may enable/disable > these power domains before adding the bus. I don't know enough to have an opinion, but the arrangement on Panda is literally a linear regulator is being controlled by a gpio, which fits into struct regulator model. What else would a power domain for that buy us? >> We should have a more generic solution in a more central location, like >> the device core. >> >> For example, each struct platform_device could have a list of resources >> to be enabled when the device is bound to a driver and disabled when >> the device is unbound. Such a list could include regulators, clocks, >> and whatever else people can invent. >> >> In this case, when it is created the ehci-omap.0 platform device could >> get an entry pointing to the LAN95xx's regulator. Then the regulator >> would automatically be turned on when the platform device is bound to >> the ehci-omap driver. > > The LAN95xx's regulator is still platform dependent(even for same LAN95xx > USB devices on different platforms(omap, tegra, ..)) , so I am wondering > why we don't enable it directly in probe() of ehci-omap.0 platform device? I didn't get this point, ehci-omap doesn't help if it's non-omap platform. -Andy
On Wed, 28 Nov 2012, Andy Green wrote: > > Greg's advice was simply not to rely on pathnames in sysfs because they > > aren't fixed in stone. That leaves plenty of other ways to approach > > this problem. > > It's sage advice, but there is zero code provided in my patches that > "relies on pathnames in sysfs". In your 1/5 patch, _device_path_generate() concatenates device name strings, starting from a device root and separating elements with '/' characters. Isn't that the same as a sysfs pathname? > > Basically, what you want is for something related to device A (the > > regulator or the GPIO) to happen whenever device B (the ehci-omap.0 > > platform device) is bound to a driver. The most straightforward way to > > arrange this is for A's driver to have a callback that is invoked > > whenever B is bound or unbound. The most straightforward way to > > arrange _that_ is to allow each platform_device to have a list of > > callbacks. > > Sorry I didn't really understand this proposal yet. You want "A", the > regulator, driver to grow a callback function that gets called when the > targeted platform_device ("B", ehci-omap.0) probe happens. Could you > expand what the callback prototype or new members in the struct might > look like? It's your tuple thing or we pass it an opaque pointer that > is the struct regulator * or suchlike? Well, it won't be exactly the same as the tuple thing because no strings will be involved, but it would be similar. The callback would receive an opaque pointer (presumably to the regulator) and a device pointer (the B device). > Throwing out the path stuff and limiting this to platform_device means > you cannot bind to dynamically created objects like hub or anything > downstream of a hub. So Felipe's identification of the hub as the > happening place to do this is out of luck. Greg pointed out that this could be useful for arbitrary devices, not just platform devices, so it could be applied to dynamically created objects. As for what Felipe said... He suggested doing this when the hub driver binds to the controller's root hub. The root hub is created when the controller's driver registers the new USB bus. This happens as part of the driver's probe routine. So what I have been talking about is very similar (in terms of when it happens) to what Felipe wanted. Besides, Felipe wasn't thinking in the most general terms. (In fact, at first he confused the root hub with the LAN95xx's hub.) There's no reason to restrict this sort of thing to USB hubs (or to regulators, for that matter). The driver core is the right place for it. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included: > On Wed, 28 Nov 2012, Andy Green wrote: > >>> Greg's advice was simply not to rely on pathnames in sysfs because they >>> aren't fixed in stone. That leaves plenty of other ways to approach >>> this problem. >> >> It's sage advice, but there is zero code provided in my patches that >> "relies on pathnames in sysfs". > > In your 1/5 patch, _device_path_generate() concatenates device name > strings, starting from a device root and separating elements with '/' > characters. Isn't that the same as a sysfs pathname? It's nothing to do with sysfs... yes some unrelated bits of sysfs also walk the device path. If we want to talk about how fragile the device path is as an id scheme over time we need to talk about likelihood of individual device names changing, not "sysfs". Anyway --> >>> Basically, what you want is for something related to device A (the >>> regulator or the GPIO) to happen whenever device B (the ehci-omap.0 >>> platform device) is bound to a driver. The most straightforward way to >>> arrange this is for A's driver to have a callback that is invoked >>> whenever B is bound or unbound. The most straightforward way to >>> arrange _that_ is to allow each platform_device to have a list of >>> callbacks. >> >> Sorry I didn't really understand this proposal yet. You want "A", the >> regulator, driver to grow a callback function that gets called when the >> targeted platform_device ("B", ehci-omap.0) probe happens. Could you >> expand what the callback prototype or new members in the struct might >> look like? It's your tuple thing or we pass it an opaque pointer that >> is the struct regulator * or suchlike? > > Well, it won't be exactly the same as the tuple thing because no > strings will be involved, but it would be similar. The callback would > receive an opaque pointer (presumably to the regulator) and a device > pointer (the B device). OK. So I try to sketch it out iteractively to try to get in sync: device.h: enum asset_event { AE_PROBED, AE_REMOVED }; struct device_asset { char *name; /* name of regulator, clock, etc */ void *asset; /* regulator, clock, etc */ int (*handler)(struct device *dev_owner, enum asset_event asset_event, struct device_asset *asset); }; struct device { ... struct device_asset *assets; ... }; drivers/base/dd.c | really_probe(): ... struct device_asset *asset; ... asset = dev->assets; while (asset && asset->name) { if (asset->handler(dev, AE_PROBED, asset)) { /* clean up and bail */ } asset++; } /* do probe */ ... drivers/base/dd.c | __device_release_driver: (is this really the best place to oppose probe()?) ... struct device_asset *asset; ... /* call device ->remove() */ ... asset = dev->assets; while (asset && asset->name) { asset->handler(dev, AE_REMOVED, asset); asset++; } ... board file: static struct regulator myreg = { .name = "mydevice-regulator", }; static struct device_asset mydevice_assets[] = { { .name = "mydevice-regulator", .handler = regulator_default_asset_handler, }, { } }; static struct platform_device mydevice = { ... .dev = { .assets = mydevice_assets, }, ... }; regulator code: int regulator_default_asset_handler(struct device *dev_owner, enum asset_event asset_event, struct device_asset *asset) { struct regulator **reg = &asset->asset; int n; switch (asset_event) { case AE_PROBED: *reg = regulator_get(dev_owner, asset->name); if (IS_ERR(*reg)) return *reg; n = regulator_enable(*reg); if (n < 0) regulator_put(*reg); return n; case AE_REMOVED: regulator_put(*reg); break; } return 0; } EXPORT_SYMBOL_GPL(regulator_default_asset_handler); The subsystems that can expect to get used (clock...) might each want to define a default handler like the one for regulator. That'll be an end to the code duplication issue. The user can still do his own handler if he wants. I put a name field in so we can use regulator_get() nicely, we don't need access to the object pointer or that it exists at boardfile-time that way either. But I can see it's arguable. >> Throwing out the path stuff and limiting this to platform_device means >> you cannot bind to dynamically created objects like hub or anything >> downstream of a hub. So Felipe's identification of the hub as the >> happening place to do this is out of luck. > > Greg pointed out that this could be useful for arbitrary devices, not > just platform devices, so it could be applied to dynamically created > objects. Well that is cool, but to exploit that in the dynamic object case arrangements for identifying the appropriate object has appeared are needed. We have a nice array of platform_devices nicely there in the board file we can attach assets to like "pdev[1].dev.assets = xxx;" but that's not there in dynamic device case. Anyway this sounds like what we're discussing can be well worth establishing and might lead to that later. > As for what Felipe said... He suggested doing this when the hub driver > binds to the controller's root hub. The root hub is created when the > controller's driver registers the new USB bus. This happens as part of > the driver's probe routine. So what I have been talking about is very > similar (in terms of when it happens) to what Felipe wanted. > > Besides, Felipe wasn't thinking in the most general terms. (In fact, > at first he confused the root hub with the LAN95xx's hub.) There's no > reason to restrict this sort of thing to USB hubs (or to regulators, > for that matter). The driver core is the right place for it. Sounds good to me. -Andy
On Wed, 28 Nov 2012, Andy Green wrote: > OK. So I try to sketch it out iteractively to try to get in sync: > > device.h: > > enum asset_event { > AE_PROBED, > AE_REMOVED > }; > > struct device_asset { > char *name; /* name of regulator, clock, etc */ > void *asset; /* regulator, clock, etc */ > int (*handler)(struct device *dev_owner, enum asset_event asset_event, > struct device_asset *asset); > }; Another possibility is to have two handlers: one for pre-probe and the other for post-remove. Then of course asset_event wouldn't be needed. Linus tends to prefer this strategy -- separate functions for separate events. That's why struct dev_pm_ops has so many method pointers. > struct device { > ... > struct device_asset *assets; Or a list instead of a NULL-terminated array. It depends on whether people will want to add or remove assets dynamically. For now an array would be fine. > ... > }; > > > drivers/base/dd.c | really_probe(): > > ... > struct device_asset *asset; > ... > asset = dev->assets; > while (asset && asset->name) { Maybe it would be better to test asset->handler. Some assets might not need names. > if (asset->handler(dev, AE_PROBED, asset)) { > /* clean up and bail */ > } > asset++; > } > > /* do probe */ > ... > > > drivers/base/dd.c | __device_release_driver: (is this really the best > place to oppose probe()?) The right place is just after the remove method is called, so yes. > ... > struct device_asset *asset; > ... > > /* call device ->remove() */ > ... > asset = dev->assets; > while (asset && asset->name) { > asset->handler(dev, AE_REMOVED, asset); > asset++; > } It would be slightly safer to iterate in reverse order. > ... > > > board file: > > static struct regulator myreg = { > .name = "mydevice-regulator", > }; > > static struct device_asset mydevice_assets[] = { > { > .name = "mydevice-regulator", > .handler = regulator_default_asset_handler, > }, > { } > }; > > static struct platform_device mydevice = { > ... > .dev = { > .assets = mydevice_assets, > }, > ... > }; > > > regulator code: > > int regulator_default_asset_handler(struct device *dev_owner, enum > asset_event asset_event, struct device_asset *asset) > { > struct regulator **reg = &asset->asset; > int n; > > switch (asset_event) { > case AE_PROBED: > *reg = regulator_get(dev_owner, asset->name); > if (IS_ERR(*reg)) > return *reg; PTR_ERR. > n = regulator_enable(*reg); > if (n < 0) > regulator_put(*reg); > return n; > > case AE_REMOVED: > regulator_put(*reg); > break; > } > > return 0; > } > EXPORT_SYMBOL_GPL(regulator_default_asset_handler); > > > The subsystems that can expect to get used (clock...) might each want to > define a default handler like the one for regulator. That'll be an end > to the code duplication issue. The user can still do his own handler if > he wants. > > I put a name field in so we can use regulator_get() nicely, we don't > need access to the object pointer or that it exists at boardfile-time > that way either. But I can see it's arguable. It hadn't occurred to me, but it seems like a good idea. Yes, overall this is almost exactly what I had in mind. > >> Throwing out the path stuff and limiting this to platform_device means > >> you cannot bind to dynamically created objects like hub or anything > >> downstream of a hub. So Felipe's identification of the hub as the > >> happening place to do this is out of luck. > > > > Greg pointed out that this could be useful for arbitrary devices, not > > just platform devices, so it could be applied to dynamically created > > objects. > > Well that is cool, but to exploit that in the dynamic object case > arrangements for identifying the appropriate object has appeared are > needed. For example, this scheme wouldn't lend itself easily to associating the regulator with the particular root-hub port that the LAN95xx is attached to. I can't think of any reasonable way to do that other than the approaches we have already considered. > We have a nice array of platform_devices nicely there in the > board file we can attach assets to like "pdev[1].dev.assets = xxx;" but > that's not there in dynamic device case. Anyway this sounds like what > we're discussing can be well worth establishing and might lead to that > later. Agreed. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/28/2012 04:10 AM, the mail apparently from Alan Stern included: > On Wed, 28 Nov 2012, Andy Green wrote: > >> OK. So I try to sketch it out iteractively to try to get in sync: >> >> device.h: >> >> enum asset_event { >> AE_PROBED, >> AE_REMOVED >> }; >> >> struct device_asset { >> char *name; /* name of regulator, clock, etc */ >> void *asset; /* regulator, clock, etc */ >> int (*handler)(struct device *dev_owner, enum asset_event asset_event, >> struct device_asset *asset); >> }; > > Another possibility is to have two handlers: one for pre-probe and the > other for post-remove. Then of course asset_event wouldn't be needed. > Linus tends to prefer this strategy -- separate functions for separate > events. That's why struct dev_pm_ops has so many method pointers. OK. I wonder if this needs extending to PM ops passing in to the assets. Regulator is usually self-sufficient but sometimes clocks at least need meddling in suspend paths. Maybe it's enough instead to offer a standalone api for drivers that want to meddle with assets, like enable / disable an asset clock... void *device_find_asset(struct device *device, const char *name); ...if it wants to touch anything like that it needs to mandate a nonambiguous name for the asset like "reg-mydriver-ehci-omap.0". This is also handy since the driver can then adapt around absence or presence of optional assets if it wants. >> struct device { >> ... >> struct device_asset *assets; > > Or a list instead of a NULL-terminated array. It depends on whether > people will want to add or remove assets dynamically. For now an array > would be fine. OK. >> ... >> }; >> >> >> drivers/base/dd.c | really_probe(): >> >> ... >> struct device_asset *asset; >> ... >> asset = dev->assets; >> while (asset && asset->name) { > > Maybe it would be better to test asset->handler. Some assets might not > need names. Good point. >> if (asset->handler(dev, AE_PROBED, asset)) { >> /* clean up and bail */ >> } >> asset++; >> } >> >> /* do probe */ >> ... >> >> >> drivers/base/dd.c | __device_release_driver: (is this really the best >> place to oppose probe()?) > > The right place is just after the remove method is called, so yes. > >> ... >> struct device_asset *asset; >> ... >> >> /* call device ->remove() */ >> ... >> asset = dev->assets; >> while (asset && asset->name) { >> asset->handler(dev, AE_REMOVED, asset); >> asset++; >> } > > It would be slightly safer to iterate in reverse order. Good point. >> ... >> >> >> board file: >> >> static struct regulator myreg = { >> .name = "mydevice-regulator", >> }; >> >> static struct device_asset mydevice_assets[] = { >> { >> .name = "mydevice-regulator", >> .handler = regulator_default_asset_handler, >> }, >> { } >> }; >> >> static struct platform_device mydevice = { >> ... >> .dev = { >> .assets = mydevice_assets, >> }, >> ... >> }; >> >> >> regulator code: >> >> int regulator_default_asset_handler(struct device *dev_owner, enum >> asset_event asset_event, struct device_asset *asset) >> { >> struct regulator **reg = &asset->asset; >> int n; >> >> switch (asset_event) { >> case AE_PROBED: >> *reg = regulator_get(dev_owner, asset->name); >> if (IS_ERR(*reg)) >> return *reg; > > PTR_ERR. Right. I'll offer a series with these adaptations shortly. -Andy >> n = regulator_enable(*reg); >> if (n < 0) >> regulator_put(*reg); >> return n; >> >> case AE_REMOVED: >> regulator_put(*reg); >> break; >> } >> >> return 0; >> } >> EXPORT_SYMBOL_GPL(regulator_default_asset_handler); >> >> >> The subsystems that can expect to get used (clock...) might each want to >> define a default handler like the one for regulator. That'll be an end >> to the code duplication issue. The user can still do his own handler if >> he wants. >> >> I put a name field in so we can use regulator_get() nicely, we don't >> need access to the object pointer or that it exists at boardfile-time >> that way either. But I can see it's arguable. > > It hadn't occurred to me, but it seems like a good idea. > > Yes, overall this is almost exactly what I had in mind. > >>>> Throwing out the path stuff and limiting this to platform_device means >>>> you cannot bind to dynamically created objects like hub or anything >>>> downstream of a hub. So Felipe's identification of the hub as the >>>> happening place to do this is out of luck. >>> >>> Greg pointed out that this could be useful for arbitrary devices, not >>> just platform devices, so it could be applied to dynamically created >>> objects. >> >> Well that is cool, but to exploit that in the dynamic object case >> arrangements for identifying the appropriate object has appeared are >> needed. > > For example, this scheme wouldn't lend itself easily to associating the > regulator with the particular root-hub port that the LAN95xx is > attached to. I can't think of any reasonable way to do that other than > the approaches we have already considered. > >> We have a nice array of platform_devices nicely there in the >> board file we can attach assets to like "pdev[1].dev.assets = xxx;" but >> that's not there in dynamic device case. Anyway this sounds like what >> we're discussing can be well worth establishing and might lead to that >> later. > > Agreed. > > Alan Stern >
On 11/27/2012 09:22 PM, Andy Green wrote: > On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included: >> On Wed, 28 Nov 2012, Andy Green wrote: >> >>>> Greg's advice was simply not to rely on pathnames in sysfs because they >>>> aren't fixed in stone. That leaves plenty of other ways to approach >>>> this problem. >>> >>> It's sage advice, but there is zero code provided in my patches that >>> "relies on pathnames in sysfs". >> >> In your 1/5 patch, _device_path_generate() concatenates device name >> strings, starting from a device root and separating elements with '/' >> characters. Isn't that the same as a sysfs pathname? > > It's nothing to do with sysfs... yes some unrelated bits of sysfs also > walk the device path. If we want to talk about how fragile the device > path is as an id scheme over time we need to talk about likelihood of > individual device names changing, not "sysfs". Anyway --> > >>>> Basically, what you want is for something related to device A (the >>>> regulator or the GPIO) to happen whenever device B (the ehci-omap.0 >>>> platform device) is bound to a driver. The most straightforward way to >>>> arrange this is for A's driver to have a callback that is invoked >>>> whenever B is bound or unbound. The most straightforward way to >>>> arrange _that_ is to allow each platform_device to have a list of >>>> callbacks. >>> >>> Sorry I didn't really understand this proposal yet. You want "A", the >>> regulator, driver to grow a callback function that gets called when the >>> targeted platform_device ("B", ehci-omap.0) probe happens. Could you >>> expand what the callback prototype or new members in the struct might >>> look like? It's your tuple thing or we pass it an opaque pointer that >>> is the struct regulator * or suchlike? >> >> Well, it won't be exactly the same as the tuple thing because no >> strings will be involved, but it would be similar. The callback would >> receive an opaque pointer (presumably to the regulator) and a device >> pointer (the B device). > > OK. So I try to sketch it out iteractively to try to get in sync: > > device.h: > > enum asset_event { > AE_PROBED, > AE_REMOVED > }; > > struct device_asset { > char *name; /* name of regulator, clock, etc */ > void *asset; /* regulator, clock, etc */ > int (*handler)(struct device *dev_owner, enum asset_event > asset_event, struct device_asset *asset); > }; > > struct device { > ... > struct device_asset *assets; > ... > }; > > > drivers/base/dd.c | really_probe(): > > ... > struct device_asset *asset; > ... > asset = dev->assets; > while (asset && asset->name) { > if (asset->handler(dev, AE_PROBED, asset)) { > /* clean up and bail */ > } > asset++; > } > > /* do probe */ > ... > > > drivers/base/dd.c | __device_release_driver: (is this really the best > place to oppose probe()?) > > ... > struct device_asset *asset; > ... > > /* call device ->remove() */ > ... > asset = dev->assets; > while (asset && asset->name) { > asset->handler(dev, AE_REMOVED, asset); > asset++; > } > ... > > > board file: > > static struct regulator myreg = { > .name = "mydevice-regulator", > }; > > static struct device_asset mydevice_assets[] = { > { > .name = "mydevice-regulator", > .handler = regulator_default_asset_handler, > }, > { } > }; > > static struct platform_device mydevice = { > ... > .dev = { > .assets = mydevice_assets, > }, > ... > }; > From Pandaboard's point of view, is mydevice supposed to be referring to ehci-omap, LAN95xx or something else? Strictly speaking, the regulator doesn't belongs neither to ehci-omap nor LAN95xx. It belongs to a power domain on the board. And user should have control to switch it OFF when required without hampering operation of ehci-omap, so that the other USB ports are still usable. -- regards, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/28/2012 07:13 PM, the mail apparently from Roger Quadros included: > On 11/27/2012 09:22 PM, Andy Green wrote: >> On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included: >>> On Wed, 28 Nov 2012, Andy Green wrote: >>> >>>>> Greg's advice was simply not to rely on pathnames in sysfs because they >>>>> aren't fixed in stone. That leaves plenty of other ways to approach >>>>> this problem. >>>> >>>> It's sage advice, but there is zero code provided in my patches that >>>> "relies on pathnames in sysfs". >>> >>> In your 1/5 patch, _device_path_generate() concatenates device name >>> strings, starting from a device root and separating elements with '/' >>> characters. Isn't that the same as a sysfs pathname? >> >> It's nothing to do with sysfs... yes some unrelated bits of sysfs also >> walk the device path. If we want to talk about how fragile the device >> path is as an id scheme over time we need to talk about likelihood of >> individual device names changing, not "sysfs". Anyway --> >> >>>>> Basically, what you want is for something related to device A (the >>>>> regulator or the GPIO) to happen whenever device B (the ehci-omap.0 >>>>> platform device) is bound to a driver. The most straightforward way to >>>>> arrange this is for A's driver to have a callback that is invoked >>>>> whenever B is bound or unbound. The most straightforward way to >>>>> arrange _that_ is to allow each platform_device to have a list of >>>>> callbacks. >>>> >>>> Sorry I didn't really understand this proposal yet. You want "A", the >>>> regulator, driver to grow a callback function that gets called when the >>>> targeted platform_device ("B", ehci-omap.0) probe happens. Could you >>>> expand what the callback prototype or new members in the struct might >>>> look like? It's your tuple thing or we pass it an opaque pointer that >>>> is the struct regulator * or suchlike? >>> >>> Well, it won't be exactly the same as the tuple thing because no >>> strings will be involved, but it would be similar. The callback would >>> receive an opaque pointer (presumably to the regulator) and a device >>> pointer (the B device). >> >> OK. So I try to sketch it out iteractively to try to get in sync: >> >> device.h: >> >> enum asset_event { >> AE_PROBED, >> AE_REMOVED >> }; >> >> struct device_asset { >> char *name; /* name of regulator, clock, etc */ >> void *asset; /* regulator, clock, etc */ >> int (*handler)(struct device *dev_owner, enum asset_event >> asset_event, struct device_asset *asset); >> }; >> >> struct device { >> ... >> struct device_asset *assets; >> ... >> }; >> >> >> drivers/base/dd.c | really_probe(): >> >> ... >> struct device_asset *asset; >> ... >> asset = dev->assets; >> while (asset && asset->name) { >> if (asset->handler(dev, AE_PROBED, asset)) { >> /* clean up and bail */ >> } >> asset++; >> } >> >> /* do probe */ >> ... >> >> >> drivers/base/dd.c | __device_release_driver: (is this really the best >> place to oppose probe()?) >> >> ... >> struct device_asset *asset; >> ... >> >> /* call device ->remove() */ >> ... >> asset = dev->assets; >> while (asset && asset->name) { >> asset->handler(dev, AE_REMOVED, asset); >> asset++; >> } >> ... >> >> >> board file: >> >> static struct regulator myreg = { >> .name = "mydevice-regulator", >> }; >> >> static struct device_asset mydevice_assets[] = { >> { >> .name = "mydevice-regulator", >> .handler = regulator_default_asset_handler, >> }, >> { } >> }; >> >> static struct platform_device mydevice = { >> ... >> .dev = { >> .assets = mydevice_assets, >> }, >> ... >> }; >> > > From Pandaboard's point of view, is mydevice supposed to be referring to > ehci-omap, LAN95xx or something else? > > Strictly speaking, the regulator doesn't belongs neither to ehci-omap > nor LAN95xx. It belongs to a power domain on the board. And user should > have control to switch it OFF when required without hampering operation > of ehci-omap, so that the other USB ports are still usable. I'd prefer to deal with that after a try#1 with regulator, I didn't see any code about power domain in there right now. There's a lot to get consensus on already with this series. Notice that these assets are generic, I will provide clk and regulator handlers with try#1, and working examples for Panda case with both, but presumably power domain can fold into it as well. Since I am on usb-next and there's nothing to be seen about power domains, what is the situation with that support? -Andy
On 11/28/2012 01:47 PM, Andy Green wrote: > On 11/28/2012 07:13 PM, the mail apparently from Roger Quadros included: >> On 11/27/2012 09:22 PM, Andy Green wrote: >>> On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included: >>>> On Wed, 28 Nov 2012, Andy Green wrote: >>>> >>>>>> Greg's advice was simply not to rely on pathnames in sysfs because >>>>>> they >>>>>> aren't fixed in stone. That leaves plenty of other ways to approach >>>>>> this problem. >>>>> >>>>> It's sage advice, but there is zero code provided in my patches that >>>>> "relies on pathnames in sysfs". >>>> >>>> In your 1/5 patch, _device_path_generate() concatenates device name >>>> strings, starting from a device root and separating elements with '/' >>>> characters. Isn't that the same as a sysfs pathname? >>> >>> It's nothing to do with sysfs... yes some unrelated bits of sysfs also >>> walk the device path. If we want to talk about how fragile the device >>> path is as an id scheme over time we need to talk about likelihood of >>> individual device names changing, not "sysfs". Anyway --> >>> >>>>>> Basically, what you want is for something related to device A (the >>>>>> regulator or the GPIO) to happen whenever device B (the ehci-omap.0 >>>>>> platform device) is bound to a driver. The most straightforward >>>>>> way to >>>>>> arrange this is for A's driver to have a callback that is invoked >>>>>> whenever B is bound or unbound. The most straightforward way to >>>>>> arrange _that_ is to allow each platform_device to have a list of >>>>>> callbacks. >>>>> >>>>> Sorry I didn't really understand this proposal yet. You want "A", the >>>>> regulator, driver to grow a callback function that gets called when >>>>> the >>>>> targeted platform_device ("B", ehci-omap.0) probe happens. Could you >>>>> expand what the callback prototype or new members in the struct might >>>>> look like? It's your tuple thing or we pass it an opaque pointer that >>>>> is the struct regulator * or suchlike? >>>> >>>> Well, it won't be exactly the same as the tuple thing because no >>>> strings will be involved, but it would be similar. The callback would >>>> receive an opaque pointer (presumably to the regulator) and a device >>>> pointer (the B device). >>> >>> OK. So I try to sketch it out iteractively to try to get in sync: >>> >>> device.h: >>> >>> enum asset_event { >>> AE_PROBED, >>> AE_REMOVED >>> }; >>> >>> struct device_asset { >>> char *name; /* name of regulator, clock, etc */ >>> void *asset; /* regulator, clock, etc */ >>> int (*handler)(struct device *dev_owner, enum asset_event >>> asset_event, struct device_asset *asset); >>> }; >>> >>> struct device { >>> ... >>> struct device_asset *assets; >>> ... >>> }; >>> >>> >>> drivers/base/dd.c | really_probe(): >>> >>> ... >>> struct device_asset *asset; >>> ... >>> asset = dev->assets; >>> while (asset && asset->name) { >>> if (asset->handler(dev, AE_PROBED, asset)) { >>> /* clean up and bail */ >>> } >>> asset++; >>> } >>> >>> /* do probe */ >>> ... >>> >>> >>> drivers/base/dd.c | __device_release_driver: (is this really the best >>> place to oppose probe()?) >>> >>> ... >>> struct device_asset *asset; >>> ... >>> >>> /* call device ->remove() */ >>> ... >>> asset = dev->assets; >>> while (asset && asset->name) { >>> asset->handler(dev, AE_REMOVED, asset); >>> asset++; >>> } >>> ... >>> >>> >>> board file: >>> >>> static struct regulator myreg = { >>> .name = "mydevice-regulator", >>> }; >>> >>> static struct device_asset mydevice_assets[] = { >>> { >>> .name = "mydevice-regulator", >>> .handler = regulator_default_asset_handler, >>> }, >>> { } >>> }; >>> >>> static struct platform_device mydevice = { >>> ... >>> .dev = { >>> .assets = mydevice_assets, >>> }, >>> ... >>> }; >>> >> >> From Pandaboard's point of view, is mydevice supposed to be referring to >> ehci-omap, LAN95xx or something else? >> >> Strictly speaking, the regulator doesn't belongs neither to ehci-omap >> nor LAN95xx. It belongs to a power domain on the board. And user should >> have control to switch it OFF when required without hampering operation >> of ehci-omap, so that the other USB ports are still usable. > > I'd prefer to deal with that after a try#1 with regulator, I didn't see > any code about power domain in there right now. There's a lot to get > consensus on already with this series. Sure. I meant power domain in the general sense and not referring to any code or framework in the kernel. It could as well be implemented using the existing regulator framework. > > Notice that these assets are generic, I will provide clk and regulator > handlers with try#1, and working examples for Panda case with both, but > presumably power domain can fold into it as well. > > Since I am on usb-next and there's nothing to be seen about power > domains, what is the situation with that support? > Looking around there seems to be some power domain framework tied to runtime PM in drivers/base/power/domain.c The idea is that the power domain is powered on/off by the runtime PM core when the device is runtime resumed/suspended. I think this is meant for SoC power domains and appears to be too complicated for a GPIO based regulator control. -- regards, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 28 Nov 2012, Roger Quadros wrote: > > board file: > > > > static struct regulator myreg = { > > .name = "mydevice-regulator", > > }; > > > > static struct device_asset mydevice_assets[] = { > > { > > .name = "mydevice-regulator", > > .handler = regulator_default_asset_handler, > > }, > > { } > > }; > > > > static struct platform_device mydevice = { > > ... > > .dev = { > > .assets = mydevice_assets, > > }, > > ... > > }; > > > > From Pandaboard's point of view, is mydevice supposed to be referring to > ehci-omap, LAN95xx or something else? ehci-omap.0. > Strictly speaking, the regulator doesn't belongs neither to ehci-omap > nor LAN95xx. It belongs to a power domain on the board. And user should > have control to switch it OFF when required without hampering operation > of ehci-omap, so that the other USB ports are still usable. That is the one disadvantage of the approach we are discussing. But what API would you use to give the user this control? Neither the GPIO nor the regulator has a struct device, so you can't use sysfs directly. And even if you could, it would probably be a bad idea because the user might turn off power to the LAN95xx while the chip was being used. The natural answer is to associate the regulator with the USB port that the LAN95xx is connected to, so that the new port-power mechanism could provide the control you want. Then how should that association be set up? Lei Ming provided a partial answer, but his suggestion is tied to USB. It would be better to have a more general approach. So far nobody has come up with a suggestion that everybody approves of. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 29, 2012 at 12:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, 28 Nov 2012, Roger Quadros wrote: > >> > board file: >> > >> > static struct regulator myreg = { >> > .name = "mydevice-regulator", >> > }; >> > >> > static struct device_asset mydevice_assets[] = { >> > { >> > .name = "mydevice-regulator", >> > .handler = regulator_default_asset_handler, >> > }, >> > { } >> > }; >> > >> > static struct platform_device mydevice = { >> > ... >> > .dev = { >> > .assets = mydevice_assets, >> > }, >> > ... >> > }; >> > >> >> From Pandaboard's point of view, is mydevice supposed to be referring to >> ehci-omap, LAN95xx or something else? > > ehci-omap.0. > >> Strictly speaking, the regulator doesn't belongs neither to ehci-omap >> nor LAN95xx. It belongs to a power domain on the board. And user should >> have control to switch it OFF when required without hampering operation >> of ehci-omap, so that the other USB ports are still usable. > > That is the one disadvantage of the approach we are discussing. > > But what API would you use to give the user this control? Neither the > GPIO nor the regulator has a struct device, so you can't use sysfs > directly. And even if you could, it would probably be a bad idea > because the user might turn off power to the LAN95xx while the chip was > being used. After Tianyu introduced the power power on/off mechanism, sometimes one port power need to be switched off/on. Embedded system is more power sensitive than PC, sounds we have no reason to reject applying the mechanism on embedded world(non ACPI). Looks better to associate the power domain thing(regulator, clock, ...) with one usb port device in this USB problem. > > The natural answer is to associate the regulator with the USB port that > the LAN95xx is connected to, so that the new port-power mechanism could > provide the control you want. Then how should that association be set > up? As I suggested in below link, the association can be set up easily with one structure of 'struct port_power_domain'. http://www.spinics.net/lists/linux-omap/msg83158.html > > Lei Ming provided a partial answer, but his suggestion is tied to USB. If we want to set up the association between usb port and power domain, usb knowledge is required, at least bus info and port topology are needed. One difficulty is the fact that the device(such as usb port) is independent with the 'power domain', for example, the device isn't created(ports of the root hub is created after ehci-omap probe, and port device of non-root hub may depend on powering on the power domain) when defining the regulator things, so could we figure out one general way in theory to describe the associated device with the 'power domain'? Andy has tried the wildcard dev path, and port topology string is introduced in my suggestion, looks both are not general. I admit the suggestion is partial because we still have not a general abstract on 'power domain' in this problem, and once it is ready, the solution might be in a shape at least for USB. In PC world, ACPI might do sort of something of the 'power domain' Maybe we need to create a new thread on this discussion and make more guys involved(PM/USB/driver core/OMAP/....). I will study the problem further, and hope I can post something for starting the discussion later. > It would be better to have a more general approach. So far nobody has Yes, I agree. IMO, my suggestion is still in the direction to being general, because a general 'power domain' concept is introduced in it, for example the 'struct power_domain' is associated with 'struct port_power_domain'. I understand the same 'power domain' concept should be applied to other device or bus too, and the power associated with this device can be switched off sometimes too for saving power consumption. But still looks specific device/subsystem knowledge is required to set up the association. Alan, so could the above be your concern on a more general approach? Or you hope a more general way(such as, do it in driver core or dev PM core) to associate the 'power domain' with one device(for example, usb port in the LAN95xx problem) too? Or other things? Thanks,
On Thu, 29 Nov 2012, Ming Lei wrote: > If we want to set up the association between usb port and power domain, > usb knowledge is required, at least bus info and port topology are needed. > > One difficulty is the fact that the device(such as usb port) is independent > with the 'power domain', for example, the device isn't created(ports of the > root hub is created after ehci-omap probe, and port device of non-root > hub may depend on powering on the power domain) when defining the regulator > things, so could we figure out one general way in theory to describe the > associated device with the 'power domain'? Andy has tried the wildcard dev > path, and port topology string is introduced in my suggestion, looks both > are not general. The device path approach probably could be made to work, but it does have the problem that it depends on device names which may change in the future. However, I can't think of any other general-purpose approach. And most especially, I can't think of any approach that doesn't require extra overhead _every_ time a new device is registered. The port topology string is, of course, specific to USB. > I admit the suggestion is partial because we still have not a general abstract > on 'power domain' in this problem, and once it is ready, the solution might be > in a shape at least for USB. In PC world, ACPI might do sort of something of > the 'power domain' I'm not worried about the "power domain" part. For now, a regulator is all we need. It should be easy enough to expand that later on. > Maybe we need to create a new thread on this discussion and make more > guys involved(PM/USB/driver core/OMAP/....). I will study the problem further, > and hope I can post something for starting the discussion later. > > > It would be better to have a more general approach. So far nobody has > > Yes, I agree. IMO, my suggestion is still in the direction to being general, > because a general 'power domain' concept is introduced in it, for example > the 'struct power_domain' is associated with 'struct port_power_domain'. It's general, but in the wrong direction. The hard part isn't the power domain aspect; it is setting up the match between the power domain and the dynamically created device. > I understand the same 'power domain' concept should be applied to other > device or bus too, and the power associated with this device can be switched off > sometimes too for saving power consumption. But still looks specific > device/subsystem knowledge is required to set up the association. > > Alan, so could the above be your concern on a more general approach? > Or you hope a more general way(such as, do it in driver core or dev PM core) > to associate the 'power domain' with one device(for example, usb port in > the LAN95xx problem) too? Or other things? Right now we don't have _any_ general (i.e., not bus-specific) way to identify individual devices except for the device name strings. I don't know -- maybe this shouldn't have a general solution at all. Maybe we just need a platform-specific way to associate a regulator or clock with a USB port, something that the port driver would know about explicitly. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote: >> >> We should have a more generic solution in a more central location, like >> the device core. >> >> For example, each struct platform_device could have a list of resources >> to be enabled when the device is bound to a driver and disabled when >> the device is unbound. Such a list could include regulators, clocks, >> and whatever else people can invent. >> >> In this case, when it is created the ehci-omap.0 platform device could >> get an entry pointing to the LAN95xx's regulator. Then the regulator >> would automatically be turned on when the platform device is bound to >> the ehci-omap driver. >> >> How does this sound? > > That sounds much better, and it might come in handy for other types of > devices than platform devices, so feel free to put this on the core > 'struct device' instead if needed. > Isn't enabling/disabling of clocks and regulators what dev.probe()/remove() of any driver already does? If we mean only board specific setup/teardown, still it would mean having the device core to do an extra 'probe' call when the current probe() is already meant to do whatever it takes to bring the device up. To my untrained eyes, it seem like messing with the probe()/remove()'s semantics. IMHO, if we really must implement something like that, may be we should employ some 'broadcast mechanism' so that anything anywhere in kernel knows which new device has been probed()/removed() successfully. I haven't thought exactly how because I am not sure even that would be the right way to approach PandaBoard's problem. Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf gives me visions ... 1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So we should have a platform device for USB3320C, the probe() of which should simply a) Enable REFCLK (FREF_CLK3_OUT) b) Reset itself by cycling RESETB (GPIO_62) c) Create one "ehci-omap" platform device (or in appropriate order if the above isn't) That way insmod/rmmod'ing the USB3320C driver would power-up/down the whole subsystem. 2) Just like the user has to manually power-on/off any externally powered hub connected to a PC, maybe we should implement a user controlled 'soft' switch (reacting to UDEV events from ehci-omap?) to emulate LAN9514 power-on/off. I do realize it would be cool to have it automatically toggle in kernel when we (de)power the hub but isn't that outside of scope of Linux USB implementation? The above solution isn't most optimal for Panda but it seems a design more consistent with what we already have. Or so do I think. Cheers. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2012 03:49 PM, the mail apparently from Jassi Brar included: > On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote: >>> >>> We should have a more generic solution in a more central location, like >>> the device core. >>> >>> For example, each struct platform_device could have a list of resources >>> to be enabled when the device is bound to a driver and disabled when >>> the device is unbound. Such a list could include regulators, clocks, >>> and whatever else people can invent. >>> >>> In this case, when it is created the ehci-omap.0 platform device could >>> get an entry pointing to the LAN95xx's regulator. Then the regulator >>> would automatically be turned on when the platform device is bound to >>> the ehci-omap driver. >>> >>> How does this sound? >> >> That sounds much better, and it might come in handy for other types of >> devices than platform devices, so feel free to put this on the core >> 'struct device' instead if needed. >> > Isn't enabling/disabling of clocks and regulators what > dev.probe()/remove() of any driver already does? The particular issue this tries to address is stuff that is not part of the generic driver function, but which is tied to the device instance by hardware connection choices on the physical platform. External clocks, regulators and mux settings needed to make the device instance work on the board may all be out of scope for the generic driver, even when the generic driver has a SoC-specific part as in this case. So no, enabling regulators, clock and mux it has no idea about because the dependency only exists as a board feature is not the job of probe / remove in drivers already. > If we mean only board specific setup/teardown, still it would mean > having the device core to do an extra 'probe' call when the current > probe() is already meant to do whatever it takes to bring the device > up. To my untrained eyes, it seem like messing with the > probe()/remove()'s semantics. It's in the way of automating and simplifying code that would be repeated in each driver probe routine according to what you're suggesting. In fact the pre-probe activity happens at the start of the actual probe code, and post remove at the end of the actual remove, there is no duplicated activity. It's just a helper. > IMHO, if we really must implement something like that, may be we > should employ some 'broadcast mechanism' so that anything anywhere in > kernel knows which new device has been probed()/removed() > successfully. I haven't thought exactly how because I am not sure even > that would be the right way to approach PandaBoard's problem. I think this stuff is a bit more multifacted than you're giving it credit for, and indeed this thread has gone right into this aspect. Say we hooked to a device creation notifier, we still must identify the correct device, in the case the device is on a dynamic bus. Hence the discussion about device paths. Just throwing a notifier at it itself doesn't scratch the problem. The stuff I completed yesterday does solve this dynamic matching issue inexpensively. > Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of > http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf > gives me visions ... > > 1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is > USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So > we should have a platform device for USB3320C, the probe() of which > should simply > a) Enable REFCLK (FREF_CLK3_OUT) > b) Reset itself by cycling RESETB (GPIO_62) > c) Create one "ehci-omap" platform device > (or in appropriate order if the above isn't) > That way insmod/rmmod'ing the USB3320C driver would power-up/down the > whole subsystem. First there's the issue that Panda has the same signal to reset the ULPI PHY and the SMSC device, and that we must operate that reset after powering the SMSC device. The only nice way to do that is to arrange for the reset to happen once for both, at a time after the SMSC chip is powered, so we have no need to interrupt ULPI operation or touch the reset subsequently, until next powerup of the ULPI PHY + SMSC. That is why tying "foreign-to-the-generic-driver" assets to a device instantiation can get us out of the hole, even when we want a hub port device that is impossible to have as a platform_device to control the power and clock for the SMSC device. With Panda just trying to cleanly deal with ULPI reset in ULPI driver and SMSC reset in SMSC driver (how're you going to let that dynamically created smsc device know the gpio again?) are complicated by both getting reset by the same signal. Second, the choices of Panda about providing a refclock and reset to the ULPI PHY are not SoC level choices but board level ones. > 2) Just like the user has to manually power-on/off any externally > powered hub connected to a PC, maybe we should implement a user > controlled 'soft' switch (reacting to UDEV events from ehci-omap?) to > emulate LAN9514 power-on/off. I do realize it would be cool to have it > automatically toggle in kernel when we (de)power the hub but isn't > that outside of scope of Linux USB implementation? > > The above solution isn't most optimal for Panda but it seems a design > more consistent with what we already have. Or so do I think. Not sure why you think that's out of scope for USB when USB allows hubs to control port power. Hub port power state seems like the right thing to control the "enablement" of the stuff wired to the hub port afaics, it's a great indication if we transition a hub port from depowered to powered, even if that only has a logical effect rather than actually switching 5V, we can understand from that we should power + clock (+ anything else needed) the thing connected to the port then so it can be probed... it's definitely what is wanted. The device_asset stuff is able to do that and control similarly the lifecycle of arbitrary mux, clocks and regulators to go along with it, without contaminiating the hub driver (except with 2 calls to work around the fact that hub port devices have no driver). The power domain stuff will also work, but only as far as hooking up the regulator and not the clock and mux stuff. If there's a plan to allow to bind clocks and mux to a regulator, well, OK you can do it all that way, gluing it together by magic names for the power domain assets. Anyway there's a lot of angles to it, the device_asset technique is powerful and will definitely solve "out of band" prerequisites setup nicely without involving the driver. But I can't say I can see deep enough into the other considerations (especially other subsystems understanding all this thinking) that just trying to solve the regulator angle with a power domain is "wrong". However it won't properly solve even the Panda case by itself as well as device_asset - external clock will stay permanently up and the forest of mux code - it's code, not tables - in mach-omap2 will stay as a _init one-off. -Andy
On Sat, Dec 1, 2012 at 2:07 PM, Andy Green <andy.green@linaro.org> wrote: > On 12/01/2012 03:49 PM, the mail apparently from Jassi Brar included: > >> On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> >> wrote: >>> >>> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote: >>>> >>>> >>>> We should have a more generic solution in a more central location, like >>>> the device core. >>>> >>>> For example, each struct platform_device could have a list of resources >>>> to be enabled when the device is bound to a driver and disabled when >>>> the device is unbound. Such a list could include regulators, clocks, >>>> and whatever else people can invent. >>>> >>>> In this case, when it is created the ehci-omap.0 platform device could >>>> get an entry pointing to the LAN95xx's regulator. Then the regulator >>>> would automatically be turned on when the platform device is bound to >>>> the ehci-omap driver. >>>> >>>> How does this sound? >>> >>> >>> That sounds much better, and it might come in handy for other types of >>> devices than platform devices, so feel free to put this on the core >>> 'struct device' instead if needed. >>> >> Isn't enabling/disabling of clocks and regulators what >> dev.probe()/remove() of any driver already does? > > > The particular issue this tries to address is stuff that is not part of the > generic driver function, but which is tied to the device instance by > hardware connection choices on the physical platform. External clocks, > regulators and mux settings needed to make the device instance work on the > board may all be out of scope for the generic driver, even when the generic > driver has a SoC-specific part as in this case. > > So no, enabling regulators, clock and mux it has no idea about because the > dependency only exists as a board feature is not the job of probe / remove > in drivers already. > Yeah, I called that "board specific setup/teardown". Which is quite common in ASoC, where an soc has an I2S controller which itself doesn't do much without some third party CODEC chip onboard which has board specific OOB control. The CODEC setup usually involves occasional PLL configuring besides setting reference clock(s) and regulator supplies. > >> If we mean only board specific setup/teardown, still it would mean >> having the device core to do an extra 'probe' call when the current >> probe() is already meant to do whatever it takes to bring the device >> up. To my untrained eyes, it seem like messing with the >> probe()/remove()'s semantics. > > > It's in the way of automating and simplifying code that would be repeated in > each driver probe routine according to what you're suggesting. In fact the > pre-probe activity happens at the start of the actual probe code, and post > remove at the end of the actual remove, there is no duplicated activity. > It's just a helper. > I am not calling that code duplication. Just that device core shouldn't be bothered to first setup board specific stuff and then platform/soc specific stuff. Esp when both calls would be contiguous. dev.probe() should simply get things up and running. > >> IMHO, if we really must implement something like that, may be we >> should employ some 'broadcast mechanism' so that anything anywhere in >> kernel knows which new device has been probed()/removed() >> successfully. I haven't thought exactly how because I am not sure even >> that would be the right way to approach PandaBoard's problem. > > > I think this stuff is a bit more multifacted than you're giving it credit > for, and indeed this thread has gone right into this aspect. Say we hooked > to a device creation notifier, we still must identify the correct device, in > the case the device is on a dynamic bus. Hence the discussion about device > paths. Just throwing a notifier at it itself doesn't scratch the problem. > The stuff I completed yesterday does solve this dynamic matching issue > inexpensively. > Maybe your try#2 will brainwash me to fall in line :) > >> Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of >> >> http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf >> gives me visions ... >> >> 1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is >> USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So >> we should have a platform device for USB3320C, the probe() of which >> should simply >> a) Enable REFCLK (FREF_CLK3_OUT) >> b) Reset itself by cycling RESETB (GPIO_62) >> c) Create one "ehci-omap" platform device >> (or in appropriate order if the above isn't) >> That way insmod/rmmod'ing the USB3320C driver would power-up/down the >> whole subsystem. > > > First there's the issue that Panda has the same signal to reset the ULPI PHY > and the SMSC device, and that we must operate that reset after powering the > SMSC device. The only nice way to do that is to arrange for the reset to > happen once for both, at a time after the SMSC chip is powered, so we have > no need to interrupt ULPI operation or touch the reset subsequently, until > next powerup of the ULPI PHY + SMSC. > > That is why tying "foreign-to-the-generic-driver" assets to a device > instantiation can get us out of the hole, even when we want a hub port > device that is impossible to have as a platform_device to control the power > and clock for the SMSC device. > > With Panda just trying to cleanly deal with ULPI reset in ULPI driver and > SMSC reset in SMSC driver (how're you going to let that dynamically created > smsc device know the gpio again?) are complicated by both getting reset by > the same signal. > Well, Panda has exactly one device (SMSC's hub) _hardwired_ to the root-hub port (USB3320C). So does it really matter if we reset the SMSC device and the ULPI-PHY gets reset too or vice-versa? > Second, the choices of Panda about providing a refclock and reset to the > ULPI PHY are not SoC level choices but board level ones. > Of course and that's why I said let USB3320C (board specific) get its clock and reset line and then in probe() populate a device for ULPI-PHY (SoC specific) > >> 2) Just like the user has to manually power-on/off any externally >> powered hub connected to a PC, maybe we should implement a user >> controlled 'soft' switch (reacting to UDEV events from ehci-omap?) to >> emulate LAN9514 power-on/off. I do realize it would be cool to have it >> automatically toggle in kernel when we (de)power the hub but isn't >> that outside of scope of Linux USB implementation? >> >> The above solution isn't most optimal for Panda but it seems a design >> more consistent with what we already have. Or so do I think. > > > Not sure why you think that's out of scope for USB when USB allows hubs to > control port power. > I meant there is nothing in USB that switches on external power supply of a connected device. LAN9514 is not bus powered and USB subsystem doesn't manage OOB signaling. regards. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jassi, On 12/01/2012 09:49 AM, Jassi Brar wrote: > On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote: >>> >>> We should have a more generic solution in a more central location, like >>> the device core. >>> >>> For example, each struct platform_device could have a list of resources >>> to be enabled when the device is bound to a driver and disabled when >>> the device is unbound. Such a list could include regulators, clocks, >>> and whatever else people can invent. >>> >>> In this case, when it is created the ehci-omap.0 platform device could >>> get an entry pointing to the LAN95xx's regulator. Then the regulator >>> would automatically be turned on when the platform device is bound to >>> the ehci-omap driver. >>> >>> How does this sound? >> >> That sounds much better, and it might come in handy for other types of >> devices than platform devices, so feel free to put this on the core >> 'struct device' instead if needed. >> > Isn't enabling/disabling of clocks and regulators what > dev.probe()/remove() of any driver already does? > If we mean only board specific setup/teardown, still it would mean > having the device core to do an extra 'probe' call when the current > probe() is already meant to do whatever it takes to bring the device > up. To my untrained eyes, it seem like messing with the > probe()/remove()'s semantics. > IMHO, if we really must implement something like that, may be we > should employ some 'broadcast mechanism' so that anything anywhere in > kernel knows which new device has been probed()/removed() > successfully. I haven't thought exactly how because I am not sure even > that would be the right way to approach PandaBoard's problem. > > Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of > http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf > gives me visions ... > > 1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is > USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So > we should have a platform device for USB3320C, the probe() of which > should simply Actually the EHCI controller within OMAP provides the root hub with 3 ports but no PHY. One of them is connected to the USB3320 which is just a ULPI PHY. > a) Enable REFCLK (FREF_CLK3_OUT) > b) Reset itself by cycling RESETB (GPIO_62) > c) Create one "ehci-omap" platform device c) is not appropriate. ehci-omap must be created before usb3320. > (or in appropriate order if the above isn't) > That way insmod/rmmod'ing the USB3320C driver would power-up/down the > whole subsystem. Yes, this is where we can think of a generic PHY driver to make sure thy PHY has necessary resources enabled. In the Panda case it would be the PHY clock and RESET gpio. The LAN95xx chip still needs to be powered up and the PHY driver isn't the right place for that (though it could be done there as a hack). The closest we can get to emulating right USB behavior is to map the SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers the LAN95xx. This way, we still don't fall in the dynamically probed space, so we could still provide the regulator information to the ehci hub via platform data and handle the regulator in ehci_hub_control (Set/ClearPortFeature). I'm looking at this as a software workaround for all platforms not implementing the EHCI set port power feature correctly. The same could be repeated for other HCDs as well if required. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 5, 2012 at 7:39 PM, Roger Quadros <rogerq@ti.com> wrote: > Hi Jassi, > > On 12/01/2012 09:49 AM, Jassi Brar wrote: >> On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >>> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote: >>>> >>>> We should have a more generic solution in a more central location, like >>>> the device core. >>>> >>>> For example, each struct platform_device could have a list of resources >>>> to be enabled when the device is bound to a driver and disabled when >>>> the device is unbound. Such a list could include regulators, clocks, >>>> and whatever else people can invent. >>>> >>>> In this case, when it is created the ehci-omap.0 platform device could >>>> get an entry pointing to the LAN95xx's regulator. Then the regulator >>>> would automatically be turned on when the platform device is bound to >>>> the ehci-omap driver. >>>> >>>> How does this sound? >>> >>> That sounds much better, and it might come in handy for other types of >>> devices than platform devices, so feel free to put this on the core >>> 'struct device' instead if needed. >>> >> Isn't enabling/disabling of clocks and regulators what >> dev.probe()/remove() of any driver already does? >> If we mean only board specific setup/teardown, still it would mean >> having the device core to do an extra 'probe' call when the current >> probe() is already meant to do whatever it takes to bring the device >> up. To my untrained eyes, it seem like messing with the >> probe()/remove()'s semantics. >> IMHO, if we really must implement something like that, may be we >> should employ some 'broadcast mechanism' so that anything anywhere in >> kernel knows which new device has been probed()/removed() >> successfully. I haven't thought exactly how because I am not sure even >> that would be the right way to approach PandaBoard's problem. >> >> Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of >> http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf >> gives me visions ... >> >> 1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is >> USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So >> we should have a platform device for USB3320C, the probe() of which >> should simply > > Actually the EHCI controller within OMAP provides the root hub with 3 > ports but no PHY. One of them is connected to the USB3320 which is just > a ULPI PHY. > >> a) Enable REFCLK (FREF_CLK3_OUT) >> b) Reset itself by cycling RESETB (GPIO_62) >> c) Create one "ehci-omap" platform device > > c) is not appropriate. ehci-omap must be created before usb3320. > >> (or in appropriate order if the above isn't) >> That way insmod/rmmod'ing the USB3320C driver would power-up/down the >> whole subsystem. > > Yes, this is where we can think of a generic PHY driver to make sure thy > PHY has necessary resources enabled. In the Panda case it would be the > PHY clock and RESET gpio. > I wonder if ehci-omap should assume, besides regulator, a clock might also be need to setup for the transceiver chip. Maybe "usbhs_bdata" in board-omap4panda.c should have .reset_gpio_port[0] = GPIO_HUB_NRESET ? > The LAN95xx chip still needs to be powered up and the PHY driver isn't > the right place for that (though it could be done there as a hack). The > closest we can get to emulating right USB behavior is to map the > SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers > the LAN95xx. > > This way, we still don't fall in the dynamically probed space, so we > could still provide the regulator information to the ehci hub via > platform data and handle the regulator in ehci_hub_control > (Set/ClearPortFeature). I'm looking at this as a software workaround for > all platforms not implementing the EHCI set port power feature > correctly. The same could be repeated for other HCDs as well if required. > IMHO it's not a matter of platforms not implementing EHCI set port power feature correctly, we should think about onboard devices connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC too ?) that don't run on VBUS of USB, would like their local supply to be treated as if it came from the parent hub's port i.e, tie together the USB's set port power control with their OOB regulated power supply (U9 on PandaBoard). cheers. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/2012 04:34 PM, Jassi Brar wrote: > On Wed, Dec 5, 2012 at 7:39 PM, Roger Quadros <rogerq@ti.com> wrote: >> Hi Jassi, >> >> On 12/01/2012 09:49 AM, Jassi Brar wrote: >>> On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >>>> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote: >>>>> >>>>> We should have a more generic solution in a more central location, like >>>>> the device core. >>>>> >>>>> For example, each struct platform_device could have a list of resources >>>>> to be enabled when the device is bound to a driver and disabled when >>>>> the device is unbound. Such a list could include regulators, clocks, >>>>> and whatever else people can invent. >>>>> >>>>> In this case, when it is created the ehci-omap.0 platform device could >>>>> get an entry pointing to the LAN95xx's regulator. Then the regulator >>>>> would automatically be turned on when the platform device is bound to >>>>> the ehci-omap driver. >>>>> >>>>> How does this sound? >>>> >>>> That sounds much better, and it might come in handy for other types of >>>> devices than platform devices, so feel free to put this on the core >>>> 'struct device' instead if needed. >>>> >>> Isn't enabling/disabling of clocks and regulators what >>> dev.probe()/remove() of any driver already does? >>> If we mean only board specific setup/teardown, still it would mean >>> having the device core to do an extra 'probe' call when the current >>> probe() is already meant to do whatever it takes to bring the device >>> up. To my untrained eyes, it seem like messing with the >>> probe()/remove()'s semantics. >>> IMHO, if we really must implement something like that, may be we >>> should employ some 'broadcast mechanism' so that anything anywhere in >>> kernel knows which new device has been probed()/removed() >>> successfully. I haven't thought exactly how because I am not sure even >>> that would be the right way to approach PandaBoard's problem. >>> >>> Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of >>> http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf >>> gives me visions ... >>> >>> 1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is >>> USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So >>> we should have a platform device for USB3320C, the probe() of which >>> should simply >> >> Actually the EHCI controller within OMAP provides the root hub with 3 >> ports but no PHY. One of them is connected to the USB3320 which is just >> a ULPI PHY. >> >>> a) Enable REFCLK (FREF_CLK3_OUT) >>> b) Reset itself by cycling RESETB (GPIO_62) >>> c) Create one "ehci-omap" platform device >> >> c) is not appropriate. ehci-omap must be created before usb3320. >> >>> (or in appropriate order if the above isn't) >>> That way insmod/rmmod'ing the USB3320C driver would power-up/down the >>> whole subsystem. >> >> Yes, this is where we can think of a generic PHY driver to make sure thy >> PHY has necessary resources enabled. In the Panda case it would be the >> PHY clock and RESET gpio. >> > I wonder if ehci-omap should assume, besides regulator, a clock might > also be need to setup for the transceiver chip. > Maybe "usbhs_bdata" in board-omap4panda.c should have > .reset_gpio_port[0] = GPIO_HUB_NRESET ? > Just like the regulator, reset_gpio_port has nothing to do with OMAP EHCI. So we would want to get rid of that too from the OMAP USB driver. > >> The LAN95xx chip still needs to be powered up and the PHY driver isn't >> the right place for that (though it could be done there as a hack). The >> closest we can get to emulating right USB behavior is to map the >> SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers >> the LAN95xx. >> >> This way, we still don't fall in the dynamically probed space, so we >> could still provide the regulator information to the ehci hub via >> platform data and handle the regulator in ehci_hub_control >> (Set/ClearPortFeature). I'm looking at this as a software workaround for >> all platforms not implementing the EHCI set port power feature >> correctly. The same could be repeated for other HCDs as well if required. >> > IMHO it's not a matter of platforms not implementing EHCI set port > power feature correctly, we should think about onboard devices > connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC > too ?) that don't run on VBUS of USB, would like their local supply to > be treated as if it came from the parent hub's port i.e, tie together > the USB's set port power control with their OOB regulated power supply > (U9 on PandaBoard). > On Pandaboards we are still talking about root hub ports. Do you know any of such platforms which power their USB devices out of band for _non_ root hub ports? Assuming they do exist, the only solution is to match platform data for dynamically probed devices and deal with the regulators in the hub/port driver. Something like Andy already proposed. regards, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Dec 10, 2012 at 11:48:51AM +0200, Roger Quadros wrote: > >>> (or in appropriate order if the above isn't) > >>> That way insmod/rmmod'ing the USB3320C driver would power-up/down the > >>> whole subsystem. > >> > >> Yes, this is where we can think of a generic PHY driver to make sure thy > >> PHY has necessary resources enabled. In the Panda case it would be the > >> PHY clock and RESET gpio. > >> > > I wonder if ehci-omap should assume, besides regulator, a clock might > > also be need to setup for the transceiver chip. > > Maybe "usbhs_bdata" in board-omap4panda.c should have > > .reset_gpio_port[0] = GPIO_HUB_NRESET ? > > > > Just like the regulator, reset_gpio_port has nothing to do with OMAP > EHCI. So we would want to get rid of that too from the OMAP USB driver. +1 to that. That's a requirement from the LAN95xx hub controller, not OMAP EHCI ;-)
On Mon, Dec 10, 2012 at 3:18 PM, Roger Quadros <rogerq@ti.com> wrote: > On 12/06/2012 04:34 PM, Jassi Brar wrote: >>> >>> Yes, this is where we can think of a generic PHY driver to make sure thy >>> PHY has necessary resources enabled. In the Panda case it would be the >>> PHY clock and RESET gpio. >>> >> I wonder if ehci-omap should assume, besides regulator, a clock might >> also be need to setup for the transceiver chip. >> Maybe "usbhs_bdata" in board-omap4panda.c should have >> .reset_gpio_port[0] = GPIO_HUB_NRESET ? >> > > Just like the regulator, reset_gpio_port has nothing to do with OMAP > EHCI. So we would want to get rid of that too from the OMAP USB driver. > Looking at the code I realized we already book resources only for populated ports. Saving power from LAN9514 chip would come from a separate solution. So, for now when our transceiver, USB3320, has simple hardwired configuration probably just platform_data/DT would do. When we come across some programmable transceiver (like USB3503 over i2c), that might warrant a separate PHY driver. Still I don't think we could have a 'generic' phy driver. >> >>> The LAN95xx chip still needs to be powered up and the PHY driver isn't >>> the right place for that (though it could be done there as a hack). The >>> closest we can get to emulating right USB behavior is to map the >>> SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers >>> the LAN95xx. >>> >>> This way, we still don't fall in the dynamically probed space, so we >>> could still provide the regulator information to the ehci hub via >>> platform data and handle the regulator in ehci_hub_control >>> (Set/ClearPortFeature). I'm looking at this as a software workaround for >>> all platforms not implementing the EHCI set port power feature >>> correctly. The same could be repeated for other HCDs as well if required. >>> >> IMHO it's not a matter of platforms not implementing EHCI set port >> power feature correctly, we should think about onboard devices >> connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC >> too ?) that don't run on VBUS of USB, would like their local supply to >> be treated as if it came from the parent hub's port i.e, tie together >> the USB's set port power control with their OOB regulated power supply >> (U9 on PandaBoard). >> > > On Pandaboards we are still talking about root hub ports. > > Do you know any of such platforms which power their USB devices out of > band for _non_ root hub ports? > I don't know of any. But I do believe we shouldn't discount that scenario. IIANW lan9514 doesn't take in VBUS because it wants to avoid 5V->3.3V regulating. What if someone designs an omap4 platform with 3 high-speed devices and the same concern in mind ? > Assuming they do exist, the only solution is to match platform data for > dynamically probed devices and deal with the regulators in the hub/port > driver. Something like Andy already proposed. > Yes, but I doubt if that is the only implementation. One USB specific solution could be to abstract out OOB port power control in, say, port-power.c which constructs a regulator topology mapped directly onto onboard devices', from a generic DT binding, platform_data, ACPI whatever. And then catch any set/clear_port_feature(POWER) call to enable/disable the regulator corresponding to that port. Where each regulator could be a board-specific virtual one, that does all that is necessary (like clock/reg hierarchy setup) to power up the device. Regards. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/11/2012 11:12 AM, Jassi Brar wrote: > On Mon, Dec 10, 2012 at 3:18 PM, Roger Quadros <rogerq@ti.com> wrote: >> On 12/06/2012 04:34 PM, Jassi Brar wrote: >>>> >>>> Yes, this is where we can think of a generic PHY driver to make sure thy >>>> PHY has necessary resources enabled. In the Panda case it would be the >>>> PHY clock and RESET gpio. >>>> >>> I wonder if ehci-omap should assume, besides regulator, a clock might >>> also be need to setup for the transceiver chip. >>> Maybe "usbhs_bdata" in board-omap4panda.c should have >>> .reset_gpio_port[0] = GPIO_HUB_NRESET ? >>> >> >> Just like the regulator, reset_gpio_port has nothing to do with OMAP >> EHCI. So we would want to get rid of that too from the OMAP USB driver. >> > Looking at the code I realized we already book resources only for > populated ports. Saving power from LAN9514 chip would come from a > separate solution. So, for now when our transceiver, USB3320, has > simple hardwired configuration probably just platform_data/DT would > do. When we come across some programmable transceiver (like USB3503 > over i2c), that might warrant a separate PHY driver. Still I don't > think we could have a 'generic' phy driver. > Yes I2C based Phys might need a different driver. At least we could have a generic PHY driver for all ULPI based phys. (NOTE: the USB3320 is also configurable and can work in OTG mode) ULPI spec has defined a standard register map which could be used by the generic driver. As far as OMAP is concerned, the ULPI registers are accessed most of the time internally by the USB Host IP. We might only need to access them from the driver to cover some erratas. >>> >>>> The LAN95xx chip still needs to be powered up and the PHY driver isn't >>>> the right place for that (though it could be done there as a hack). The >>>> closest we can get to emulating right USB behavior is to map the >>>> SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers >>>> the LAN95xx. >>>> >>>> This way, we still don't fall in the dynamically probed space, so we >>>> could still provide the regulator information to the ehci hub via >>>> platform data and handle the regulator in ehci_hub_control >>>> (Set/ClearPortFeature). I'm looking at this as a software workaround for >>>> all platforms not implementing the EHCI set port power feature >>>> correctly. The same could be repeated for other HCDs as well if required. >>>> >>> IMHO it's not a matter of platforms not implementing EHCI set port >>> power feature correctly, we should think about onboard devices >>> connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC >>> too ?) that don't run on VBUS of USB, would like their local supply to >>> be treated as if it came from the parent hub's port i.e, tie together >>> the USB's set port power control with their OOB regulated power supply >>> (U9 on PandaBoard). >>> >> >> On Pandaboards we are still talking about root hub ports. >> >> Do you know any of such platforms which power their USB devices out of >> band for _non_ root hub ports? >> > I don't know of any. But I do believe we shouldn't discount that scenario. > IIANW lan9514 doesn't take in VBUS because it wants to avoid 5V->3.3V > regulating. What if someone designs an omap4 platform with 3 > high-speed devices and the same concern in mind ? > >> Assuming they do exist, the only solution is to match platform data for >> dynamically probed devices and deal with the regulators in the hub/port >> driver. Something like Andy already proposed. >> > Yes, but I doubt if that is the only implementation. > One USB specific solution could be to abstract out OOB port power > control in, say, port-power.c which constructs a regulator topology > mapped directly onto onboard devices', from a generic DT binding, > platform_data, ACPI whatever. And then catch any > set/clear_port_feature(POWER) call to enable/disable the regulator > corresponding to that port. Where each regulator could be a > board-specific virtual one, that does all that is necessary (like > clock/reg hierarchy setup) to power up the device. > Yes. This is what I too was suggesting earlier. The big question here is how to match the regulator to the hub port. One way was matching the device paths. regards, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 11, 2012 at 12:01:57PM +0200, Roger Quadros wrote: > On 12/11/2012 11:12 AM, Jassi Brar wrote: > > On Mon, Dec 10, 2012 at 3:18 PM, Roger Quadros <rogerq@ti.com> wrote: > >> On 12/06/2012 04:34 PM, Jassi Brar wrote: > >>>> > >>>> Yes, this is where we can think of a generic PHY driver to make sure thy > >>>> PHY has necessary resources enabled. In the Panda case it would be the > >>>> PHY clock and RESET gpio. > >>>> > >>> I wonder if ehci-omap should assume, besides regulator, a clock might > >>> also be need to setup for the transceiver chip. > >>> Maybe "usbhs_bdata" in board-omap4panda.c should have > >>> .reset_gpio_port[0] = GPIO_HUB_NRESET ? > >>> > >> > >> Just like the regulator, reset_gpio_port has nothing to do with OMAP > >> EHCI. So we would want to get rid of that too from the OMAP USB driver. > >> > > Looking at the code I realized we already book resources only for > > populated ports. Saving power from LAN9514 chip would come from a > > separate solution. So, for now when our transceiver, USB3320, has > > simple hardwired configuration probably just platform_data/DT would > > do. When we come across some programmable transceiver (like USB3503 > > over i2c), that might warrant a separate PHY driver. Still I don't > > think we could have a 'generic' phy driver. > > > Yes I2C based Phys might need a different driver. At least we could have > a generic PHY driver for all ULPI based phys. (NOTE: the USB3320 is also > configurable and can work in OTG mode) we already do, that's what nop-xceiv is for.
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index b34b5cd..3324a55 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -282,4 +282,10 @@ config CMA_AREAS endif +config DEVICEPATH + bool "Device path api" + default n + help + Include dynamicly probed path matching API + endmenu diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 5aa2d70..b8d5723 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -22,5 +22,7 @@ obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o obj-$(CONFIG_REGMAP) += regmap/ obj-$(CONFIG_SOC_BUS) += soc.o +obj-$(CONFIG_DEVICEPATH) += path.o + ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG diff --git a/drivers/base/core.c b/drivers/base/core.c index abea76c..cc0ba02 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1368,6 +1368,8 @@ int __init devices_init(void) if (!sysfs_dev_char_kobj) goto char_kobj_err; + device_path_init(); + return 0; char_kobj_err: diff --git a/drivers/base/path.c b/drivers/base/path.c new file mode 100644 index 0000000..384e792 --- /dev/null +++ b/drivers/base/path.c @@ -0,0 +1,181 @@ +/* + * drivers/base/path.c - device_path apis for matching dynamic / variable + * device paths on buses like usb / mmc to wildcard constants from + * platform or DT + * + * Copyright (c) 2012 Linaro, LTD + * Author: Andy Green <andy.green@linaro.org> + * + * This file is released under the GPLv2 + * + */ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/list.h> +#include <linux/mutex.h> +#include <linux/string.h> +#include <linux/export.h> +#include <linux/slab.h> + +struct device_path { + char *path; + struct list_head list; +}; + +struct device_path list; +DEFINE_MUTEX(lock); + +static int device_path_compare_wildcard(const char *awc, const char *b) +{ + while (*awc && *b) { + if (*awc == '*') { + awc++; + /* wildcard disallowed from extening past / */ + while (*b && *b != *awc && *b != '/') + b++; + } + if (*awc != *b) + return -ENOENT; + if (!*awc) + return 0; + awc++; + b++; + } + + if (!*awc && !*b) + return 0; + + return -ENOENT; +} + +static const char *device_path_find_wildcard(const char *device_path) +{ + struct device_path *dp; + + mutex_lock(&lock); + list_for_each_entry(dp, &list.list, list) { + if (device_path_compare_wildcard(dp->path, device_path) == 0) { + mutex_unlock(&lock); + return dp->path; + } + } + + mutex_unlock(&lock); + return NULL; +} + +static int _device_path_generate(struct device *device, char *name, int len) +{ + int n = 0; + int l; + + if (!device) + return -ENODEV; + + if (device->parent) { + n = _device_path_generate(device->parent, name, len); + if (n < 0) + return n; + } + + l = strlen(dev_name(device)); + + if ((len - n) < l + 3) + return -E2BIG; + + name[n++] = '/'; + strcpy(&name[n], dev_name(device)); + + return n + l; +} + +int device_path_generate(struct device *device, char *name, int len) +{ + int n; + const char *match; + + n = _device_path_generate(device, name, len); + if (n < 0) + return n; + + /* + * if any registered wildcard matches, report that instead + */ + match = device_path_find_wildcard(name); + if (!match) + return 0; + + n = strlen(match); + if (n >= len - 1) + return -E2BIG; + + memcpy(name, match, n); + + return 0; +} +EXPORT_SYMBOL_GPL(device_path_generate); + +int device_path_register_wildcard_path(const char *wildcard_devpath) +{ + struct device_path *dp; + int ret = -ENOMEM; + + if (strchr(wildcard_devpath, '*') == NULL) + return 0; + + mutex_lock(&lock); + dp = kmalloc(sizeof(struct device_path), GFP_KERNEL); + if (!dp) + goto bail; + + dp->path = kmalloc(strlen(wildcard_devpath) + 1, GFP_KERNEL); + if (!dp->path) { + kfree(dp); + goto bail; + } + + strcpy(dp->path, wildcard_devpath); + list_add(&dp->list, &list.list); + ret = 0; + +bail: + mutex_unlock(&lock); + + return ret; +} +EXPORT_SYMBOL_GPL(device_path_register_wildcard_path); + +static void device_path_free(struct device_path *dp) +{ + kfree(dp->path); + kfree(dp); +} + +int device_path_unregister_wildcard_path(const char *wildcard_devpath) +{ + struct device_path *dp; + struct list_head *pos, *q; + + mutex_lock(&lock); + list_for_each_safe(pos, q, &list.list) { + dp = list_entry(pos, struct device_path, list); + if (strcmp(dp->path, wildcard_devpath) == 0) { + list_del(pos); + device_path_free(dp); + mutex_unlock(&lock); + return 0; + } + } + + mutex_unlock(&lock); + return -EINVAL; +} +EXPORT_SYMBOL_GPL(device_path_unregister_wildcard_path); + +void __init device_path_init(void) +{ + INIT_LIST_HEAD(&list.list); + mutex_init(&lock); +} +EXPORT_SYMBOL_GPL(device_path_init); diff --git a/include/linux/device.h b/include/linux/device.h index 86ef6ab..ecaf3aa 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -875,6 +875,18 @@ extern int (*platform_notify)(struct device *dev); extern int (*platform_notify_remove)(struct device *dev); +/* + * optional device path api + */ +#ifdef CONFIG_DEVICEPATH +#define MAX_DEV_PATH_SIZE 512 +extern int device_path_generate(struct device *device, char *name, int len); +extern int device_path_unregister_wildcard_path(const char *wildcard_devpath); +extern int device_path_register_wildcard_path(const char *wildcard_devpath); +extern void device_path_init(void); +#else +static inline void device_path_init(void) { ; } +#endif /* * get_device - atomically increment the reference count for the device.