| Message ID | 1354460467-28006-5-git-send-email-tom.leiming@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/12/12 23:01, the mail apparently from Ming Lei included: Hi - > This patch defines power controller for powering on/off LAN95xx > USB hub and USB ethernet devices, and implements one match function > to associate the power controller with related USB port device. > The big problem of this approach is that it depends on the global > device ADD/DEL notifier. > > Another idea of associating power controller with port device > is by introducing usb port driver, and move all this port power > control stuff from platform code to the port driver, which is just > what I think of and looks doable. The problem of the idea is that > port driver is per board, so maybe cause lots of platform sort of > code to be put under drivers/usb/port/, but this approach can avoid > global device ADD/DEL notifier. > > I'd like to get some feedback about which one is better or other choice, > then I may do it in next cycle. > > Cc: Andy Green <andy.green@linaro.org> > Cc: Roger Quadros <rogerq@ti.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Felipe Balbi <balbi@ti.com> > Signed-off-by: Ming Lei <tom.leiming@gmail.com> > --- > arch/arm/mach-omap2/board-omap4panda.c | 99 +++++++++++++++++++++++++++++++- > 1 file changed, 96 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c > index 5c8e9ce..3183832 100644 > --- a/arch/arm/mach-omap2/board-omap4panda.c > +++ b/arch/arm/mach-omap2/board-omap4panda.c > @@ -32,6 +32,8 @@ > #include <linux/usb/musb.h> > #include <linux/wl12xx.h> > #include <linux/platform_data/omap-abe-twl6040.h> > +#include <linux/power_controller.h> > +#include <linux/usb/port.h> > > #include <asm/hardware/gic.h> > #include <asm/mach-types.h> > @@ -154,6 +156,99 @@ static struct gpio panda_ehci_gpios[] __initdata = { > { GPIO_HUB_NRESET, GPIOF_OUT_INIT_LOW, "hub_nreset" }, > }; > > +static void ehci_hub_power_on(struct power_controller *pc, struct device *dev) > +{ > + gpio_set_value(GPIO_HUB_NRESET, 1); > + gpio_set_value(GPIO_HUB_POWER, 1); > +} You should wait a bit after applying power to the smsc chip before letting go of nReset too. In the regulator-based implementation I sent it's handled by delays encoded in the regulator structs. > +static void ehci_hub_power_off(struct power_controller *pc, struct device *dev) > +{ > + gpio_set_value(GPIO_HUB_NRESET, 0); > + gpio_set_value(GPIO_HUB_POWER, 0); > +} > + > +static struct usb_port_power_switch_data root_hub_port_data = { > + .hub_tier = 0, > + .port_number = 1, > + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, > +}; > + > +static struct usb_port_power_switch_data smsc_hub_port_data = { > + .hub_tier = 1, > + .port_number = 1, > + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, > +}; > + > +static struct power_controller pc = { > + .name = "omap_hub_eth_pc", > + .count = ATOMIC_INIT(0), > + .power_on = ehci_hub_power_on, > + .power_off = ehci_hub_power_off, > +}; > + > +static inline int omap_ehci_hub_port(struct device *dev) > +{ > + /* we expect dev->parent points to ehcd controller */ > + if (dev->parent && !strcmp(dev_name(dev->parent), "ehci-omap.0")) > + return 1; > + return 0; > +} > + > +static inline int dev_pc_match(struct device *dev) > +{ > + struct device *anc; > + int ret = 0; > + > + if (likely(strcmp(dev_name(dev), "port1"))) > + goto exit; > + > + if (dev->parent && (anc = dev->parent->parent)) { > + if (omap_ehci_hub_port(anc)) { > + ret = 1; > + goto exit; > + } > + > + /* is it port of lan95xx hub? */ > + if ((anc = anc->parent) && omap_ehci_hub_port(anc)) { > + ret = 2; > + goto exit; > + } > + } > +exit: > + return ret; > +} > + > +/* > + * Notifications of device registration > + */ > +static int device_notify(struct notifier_block *nb, unsigned long action, void *data) > +{ > + struct device *dev = data; > + int ret; > + > + switch (action) { > + case DEV_NOTIFY_ADD_DEVICE: > + ret = dev_pc_match(dev); > + if (likely(!ret)) > + goto exit; > + if (ret == 1) > + dev_pc_bind(&pc, dev, &root_hub_port_data, sizeof(root_hub_port_data)); > + else > + dev_pc_bind(&pc, dev, &smsc_hub_port_data, sizeof(smsc_hub_port_data)); > + break; > + > + case DEV_NOTIFY_DEL_DEVICE: > + break; > + } > +exit: > + return 0; > +} > + > +static struct notifier_block usb_port_nb = { > + .notifier_call = device_notify, > +}; > + Some thoughts on trying to make this functionality specific to power only and ehci hub port only: - Quite a few boards have smsc95xx... they're all going to carry these additions in the board file? Surely you'll have to generalize the pieces that perform device_path business out of the omap4panda board file at least. At that point the path matching code becomes generic end-of-the-device-path matching code. - How could these literals like "port1" etc be nicely provided by Device Tree? In ARM-land there's pressure to eventually eliminate board files completely and pass in everything from dtb. device_asset can neatly grow DT bindings in a generic way, since the footprint in the board file is some regulators that already have dt bindings and some device_asset structs. Similarly there's pressure for magic code to service a board (rather than SoC) to go elsewhere than the board file. - Shouldn't this take care of enabling and disabling the ULPI PHY clock on Panda too? There's no purpose leaving it running if the one thing the ULPI PHY is connected to is depowered, and when you do power it, on Panda you will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY reset are connected together on Panda). Then you can eliminate omap4_ehci_init() in the board file. -Andy > static void __init omap4_ehci_init(void) > { > int ret; > @@ -178,12 +273,10 @@ static void __init omap4_ehci_init(void) > > gpio_export(GPIO_HUB_POWER, 0); > gpio_export(GPIO_HUB_NRESET, 0); > - gpio_set_value(GPIO_HUB_NRESET, 1); > > usbhs_init(&usbhs_bdata); > > - /* enable power to hub */ > - gpio_set_value(GPIO_HUB_POWER, 1); > + dev_register_notifier(&usb_port_nb); > } > > static struct omap_musb_board_data musb_board_data = { >
On Mon, Dec 3, 2012 at 12:37 AM, Andy Green <andy.green@linaro.org> wrote: > On 02/12/12 23:01, the mail apparently from Ming Lei included: > > Hi - > > >> This patch defines power controller for powering on/off LAN95xx >> USB hub and USB ethernet devices, and implements one match function >> to associate the power controller with related USB port device. >> The big problem of this approach is that it depends on the global >> device ADD/DEL notifier. >> >> Another idea of associating power controller with port device >> is by introducing usb port driver, and move all this port power >> control stuff from platform code to the port driver, which is just >> what I think of and looks doable. The problem of the idea is that >> port driver is per board, so maybe cause lots of platform sort of >> code to be put under drivers/usb/port/, but this approach can avoid >> global device ADD/DEL notifier. >> >> I'd like to get some feedback about which one is better or other choice, >> then I may do it in next cycle. >> >> Cc: Andy Green <andy.green@linaro.org> >> Cc: Roger Quadros <rogerq@ti.com> >> Cc: Alan Stern <stern@rowland.harvard.edu> >> Cc: Felipe Balbi <balbi@ti.com> >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> >> --- >> arch/arm/mach-omap2/board-omap4panda.c | 99 >> +++++++++++++++++++++++++++++++- >> 1 file changed, 96 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/board-omap4panda.c >> b/arch/arm/mach-omap2/board-omap4panda.c >> index 5c8e9ce..3183832 100644 >> --- a/arch/arm/mach-omap2/board-omap4panda.c >> +++ b/arch/arm/mach-omap2/board-omap4panda.c >> @@ -32,6 +32,8 @@ >> #include <linux/usb/musb.h> >> #include <linux/wl12xx.h> >> #include <linux/platform_data/omap-abe-twl6040.h> >> +#include <linux/power_controller.h> >> +#include <linux/usb/port.h> >> >> #include <asm/hardware/gic.h> >> #include <asm/mach-types.h> >> @@ -154,6 +156,99 @@ static struct gpio panda_ehci_gpios[] __initdata = { >> { GPIO_HUB_NRESET, GPIOF_OUT_INIT_LOW, "hub_nreset" }, >> }; >> >> +static void ehci_hub_power_on(struct power_controller *pc, struct device >> *dev) >> +{ >> + gpio_set_value(GPIO_HUB_NRESET, 1); >> + gpio_set_value(GPIO_HUB_POWER, 1); >> +} > > > You should wait a bit after applying power to the smsc chip before letting > go of nReset too. In the regulator-based implementation I sent it's handled > by delays encoded in the regulator structs. It isn't a big thing about the discussion. If these code is only platform code, we can use gpio or regulator or other thing. > > >> +static void ehci_hub_power_off(struct power_controller *pc, struct device >> *dev) >> +{ >> + gpio_set_value(GPIO_HUB_NRESET, 0); >> + gpio_set_value(GPIO_HUB_POWER, 0); >> +} >> + >> +static struct usb_port_power_switch_data root_hub_port_data = { >> + .hub_tier = 0, >> + .port_number = 1, >> + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, >> +}; >> + >> +static struct usb_port_power_switch_data smsc_hub_port_data = { >> + .hub_tier = 1, >> + .port_number = 1, >> + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, >> +}; >> + >> +static struct power_controller pc = { >> + .name = "omap_hub_eth_pc", >> + .count = ATOMIC_INIT(0), >> + .power_on = ehci_hub_power_on, >> + .power_off = ehci_hub_power_off, >> +}; >> + >> +static inline int omap_ehci_hub_port(struct device *dev) >> +{ >> + /* we expect dev->parent points to ehcd controller */ >> + if (dev->parent && !strcmp(dev_name(dev->parent), "ehci-omap.0")) >> + return 1; >> + return 0; >> +} >> + >> +static inline int dev_pc_match(struct device *dev) >> +{ >> + struct device *anc; >> + int ret = 0; >> + >> + if (likely(strcmp(dev_name(dev), "port1"))) >> + goto exit; >> + >> + if (dev->parent && (anc = dev->parent->parent)) { >> + if (omap_ehci_hub_port(anc)) { >> + ret = 1; >> + goto exit; >> + } >> + >> + /* is it port of lan95xx hub? */ >> + if ((anc = anc->parent) && omap_ehci_hub_port(anc)) { >> + ret = 2; >> + goto exit; >> + } >> + } >> +exit: >> + return ret; >> +} >> + >> +/* >> + * Notifications of device registration >> + */ >> +static int device_notify(struct notifier_block *nb, unsigned long action, >> void *data) >> +{ >> + struct device *dev = data; >> + int ret; >> + >> + switch (action) { >> + case DEV_NOTIFY_ADD_DEVICE: >> + ret = dev_pc_match(dev); >> + if (likely(!ret)) >> + goto exit; >> + if (ret == 1) >> + dev_pc_bind(&pc, dev, &root_hub_port_data, >> sizeof(root_hub_port_data)); >> + else >> + dev_pc_bind(&pc, dev, &smsc_hub_port_data, >> sizeof(smsc_hub_port_data)); >> + break; >> + >> + case DEV_NOTIFY_DEL_DEVICE: >> + break; >> + } >> +exit: >> + return 0; >> +} >> + >> +static struct notifier_block usb_port_nb = { >> + .notifier_call = device_notify, >> +}; >> + > > > Some thoughts on trying to make this functionality specific to power only > and ehci hub port only: > > - Quite a few boards have smsc95xx... they're all going to carry these > additions in the board file? Surely you'll have to generalize the pieces All things are board dependent because we are discussing peripheral device(not builtin SoC devices), so it is proper to put it in the board file. If some boards want to share it, we can put it in a single .c file and let board file include it. > that perform device_path business out of the omap4panda board file at least. > At that point the path matching code becomes generic end-of-the-device-path > matching code. Looks Alan has mentioned, there might be no generic way, and any device's name change in the path may make the way fragile. > > - How could these literals like "port1" etc be nicely provided by Device > Tree? In ARM-land there's pressure to eventually eliminate board files > completely and pass in everything from dtb. device_asset can neatly grow DT > bindings in a generic way, since the footprint in the board file is some IMO, it isn't necessary to expose these assets to device or users, from the view of device or user, which only cares two actions(poweron and poweroff) about the discussed problem. Also it should be better to put these assets into device resource list, instead of introducing them in 'struct device'. > regulators that already have dt bindings and some device_asset structs. > Similarly there's pressure for magic code to service a board (rather than > SoC) to go elsewhere than the board file. Looks you associate these assets with ehci-omap device, which mightn't be enough, because we need to control port's power for supporting port power off mechanism. Do you have generic way to associate these assets with usb port device and let port use it generally? > > - Shouldn't this take care of enabling and disabling the ULPI PHY clock on > Panda too? There's no purpose leaving it running if the one thing the ULPI > PHY is connected to is depowered, and when you do power it, on Panda you > will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY > reset are connected together on Panda). Then you can eliminate > omap4_ehci_init() in the board file. OK, we can include the ULPI PHY clock things easily in ->power_on() and ->power_off() of 'power controller' Thanks,
On 03/12/12 12:11, the mail apparently from Ming Lei included: > On Mon, Dec 3, 2012 at 12:37 AM, Andy Green <andy.green@linaro.org> wrote: >> On 02/12/12 23:01, the mail apparently from Ming Lei included: >> >> Hi - >> >> >>> This patch defines power controller for powering on/off LAN95xx >>> USB hub and USB ethernet devices, and implements one match function >>> to associate the power controller with related USB port device. >>> The big problem of this approach is that it depends on the global >>> device ADD/DEL notifier. >>> >>> Another idea of associating power controller with port device >>> is by introducing usb port driver, and move all this port power >>> control stuff from platform code to the port driver, which is just >>> what I think of and looks doable. The problem of the idea is that >>> port driver is per board, so maybe cause lots of platform sort of >>> code to be put under drivers/usb/port/, but this approach can avoid >>> global device ADD/DEL notifier. >>> >>> I'd like to get some feedback about which one is better or other choice, >>> then I may do it in next cycle. >>> >>> Cc: Andy Green <andy.green@linaro.org> >>> Cc: Roger Quadros <rogerq@ti.com> >>> Cc: Alan Stern <stern@rowland.harvard.edu> >>> Cc: Felipe Balbi <balbi@ti.com> >>> Signed-off-by: Ming Lei <tom.leiming@gmail.com> >>> --- >>> arch/arm/mach-omap2/board-omap4panda.c | 99 >>> +++++++++++++++++++++++++++++++- >>> 1 file changed, 96 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/board-omap4panda.c >>> b/arch/arm/mach-omap2/board-omap4panda.c >>> index 5c8e9ce..3183832 100644 >>> --- a/arch/arm/mach-omap2/board-omap4panda.c >>> +++ b/arch/arm/mach-omap2/board-omap4panda.c >>> @@ -32,6 +32,8 @@ >>> #include <linux/usb/musb.h> >>> #include <linux/wl12xx.h> >>> #include <linux/platform_data/omap-abe-twl6040.h> >>> +#include <linux/power_controller.h> >>> +#include <linux/usb/port.h> >>> >>> #include <asm/hardware/gic.h> >>> #include <asm/mach-types.h> >>> @@ -154,6 +156,99 @@ static struct gpio panda_ehci_gpios[] __initdata = { >>> { GPIO_HUB_NRESET, GPIOF_OUT_INIT_LOW, "hub_nreset" }, >>> }; >>> >>> +static void ehci_hub_power_on(struct power_controller *pc, struct device >>> *dev) >>> +{ >>> + gpio_set_value(GPIO_HUB_NRESET, 1); >>> + gpio_set_value(GPIO_HUB_POWER, 1); >>> +} >> >> >> You should wait a bit after applying power to the smsc chip before letting >> go of nReset too. In the regulator-based implementation I sent it's handled >> by delays encoded in the regulator structs. > > It isn't a big thing about the discussion. If these code is only platform code, > we can use gpio or regulator or other thing. Well, you need a delay there FYI. It's just a nit. >>> +static void ehci_hub_power_off(struct power_controller *pc, struct device >>> *dev) >>> +{ >>> + gpio_set_value(GPIO_HUB_NRESET, 0); >>> + gpio_set_value(GPIO_HUB_POWER, 0); >>> +} >>> + >>> +static struct usb_port_power_switch_data root_hub_port_data = { >>> + .hub_tier = 0, >>> + .port_number = 1, >>> + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, >>> +}; >>> + >>> +static struct usb_port_power_switch_data smsc_hub_port_data = { >>> + .hub_tier = 1, >>> + .port_number = 1, >>> + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, >>> +}; >>> + >>> +static struct power_controller pc = { >>> + .name = "omap_hub_eth_pc", >>> + .count = ATOMIC_INIT(0), >>> + .power_on = ehci_hub_power_on, >>> + .power_off = ehci_hub_power_off, >>> +}; >>> + >>> +static inline int omap_ehci_hub_port(struct device *dev) >>> +{ >>> + /* we expect dev->parent points to ehcd controller */ >>> + if (dev->parent && !strcmp(dev_name(dev->parent), "ehci-omap.0")) >>> + return 1; >>> + return 0; >>> +} >>> + >>> +static inline int dev_pc_match(struct device *dev) >>> +{ >>> + struct device *anc; >>> + int ret = 0; >>> + >>> + if (likely(strcmp(dev_name(dev), "port1"))) >>> + goto exit; >>> + >>> + if (dev->parent && (anc = dev->parent->parent)) { >>> + if (omap_ehci_hub_port(anc)) { >>> + ret = 1; >>> + goto exit; >>> + } >>> + >>> + /* is it port of lan95xx hub? */ >>> + if ((anc = anc->parent) && omap_ehci_hub_port(anc)) { >>> + ret = 2; >>> + goto exit; >>> + } >>> + } >>> +exit: >>> + return ret; >>> +} >>> + >>> +/* >>> + * Notifications of device registration >>> + */ >>> +static int device_notify(struct notifier_block *nb, unsigned long action, >>> void *data) >>> +{ >>> + struct device *dev = data; >>> + int ret; >>> + >>> + switch (action) { >>> + case DEV_NOTIFY_ADD_DEVICE: >>> + ret = dev_pc_match(dev); >>> + if (likely(!ret)) >>> + goto exit; >>> + if (ret == 1) >>> + dev_pc_bind(&pc, dev, &root_hub_port_data, >>> sizeof(root_hub_port_data)); >>> + else >>> + dev_pc_bind(&pc, dev, &smsc_hub_port_data, >>> sizeof(smsc_hub_port_data)); >>> + break; >>> + >>> + case DEV_NOTIFY_DEL_DEVICE: >>> + break; >>> + } >>> +exit: >>> + return 0; >>> +} >>> + >>> +static struct notifier_block usb_port_nb = { >>> + .notifier_call = device_notify, >>> +}; >>> + >> >> >> Some thoughts on trying to make this functionality specific to power only >> and ehci hub port only: >> >> - Quite a few boards have smsc95xx... they're all going to carry these >> additions in the board file? Surely you'll have to generalize the pieces > > All things are board dependent because we are discussing peripheral > device(not builtin SoC devices), so it is proper to put it in the board file. > If some boards want to share it, we can put it in a single .c file and > let board file include it. Where would the .c file go... SMSC is not platform, or even arch specific chip (eg, iMX or MIPS or even x86 boards can have it), so not arch/arm/mach-xxxx or arch/arm/plat-xxx or arch/arm. I guess it would go in drivers/usb or drivers/net. Push in ARM-Land is towards single kernels which support many platforms, a good case in point is omap2plus_defconfg which wants to allow to support all OMAP2/3/4/5 platforms in one kernel. If you "include" that C file over and over in board files, it's not very nice for that. They anyway want to eliminate board files for the single kernel binary reason, and just have one load of config come in by dtb. So it guides you towards having static helper code once in drivers/ for the scenarios you support... if that's where you end up smsc is less good a target for a helper than to have helpers for classes of object like regulator and clk, that you can combine and reuse on all sorts of target devices, which is device_asset approach. It also guides you to having the special platform sauce be something that can go into the dtb: per-board code can't. That's why device_asset stuff only places asset structs in the board file and is removing code from there. >> that perform device_path business out of the omap4panda board file at least. >> At that point the path matching code becomes generic end-of-the-device-path >> matching code. > > Looks Alan has mentioned, there might be no generic way, and any device's > name change in the path may make the way fragile. What you have provided is no less fragile if you allow "port1" and the ehci-omap.0 to be set from the outside. Unless someone NAKs it for sure already (if you're already sure you're going to, please do so to avoid wasting time), I'll issue a try#2 of my code later which demonstrates what I mean. At least I guess it's useful for comparative purposes. >> - How could these literals like "port1" etc be nicely provided by Device >> Tree? In ARM-land there's pressure to eventually eliminate board files >> completely and pass in everything from dtb. device_asset can neatly grow DT >> bindings in a generic way, since the footprint in the board file is some > > IMO, it isn't necessary to expose these assets to device or users, from the > view of device or user, which only cares two actions(poweron and poweroff) > about the discussed problem. Also it should be better to put these assets > into device resource list, instead of introducing them in 'struct device'. From the point of view of allowing it to be reused on different boards / platforms / arches, you are going to have to do something better than have literals in the code. >> regulators that already have dt bindings and some device_asset structs. >> Similarly there's pressure for magic code to service a board (rather than >> SoC) to go elsewhere than the board file. > > Looks you associate these assets with ehci-omap device, which mightn't be > enough, because we need to control port's power for supporting port > power off mechanism. Do you have generic way to associate these assets > with usb port device and let port use it generally? Yes, you need a parent device pointer (ehci host controller in this case) and the path rhs, but only stuff that is defined by usb stack code. Needing a parent pointer is OK because this stuff only has meaning for hardwired assets on the platform, so the parent device will always be known as a platform_device at boot time anyway. The code I'll provide will work the same in sdio or other bus case, just use mmc host controller pointer and the sdio device name the same way. >> - Shouldn't this take care of enabling and disabling the ULPI PHY clock on >> Panda too? There's no purpose leaving it running if the one thing the ULPI >> PHY is connected to is depowered, and when you do power it, on Panda you >> will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY >> reset are connected together on Panda). Then you can eliminate >> omap4_ehci_init() in the board file. > > OK, we can include the ULPI PHY clock things easily in ->power_on() and > ->power_off() of 'power controller' Yes if the ARM people will accept establishing more code in board files that doesn't have a DT story. -Andy
On Mon, 3 Dec 2012, Andy Green wrote: > Unless someone NAKs it for sure already (if you're already sure you're > going to, please do so to avoid wasting time), I'll issue a try#2 of my > code later which demonstrates what I mean. At least I guess it's useful > for comparative purposes. Before you go writing a whole lot more code, we should discuss the basics a bit more clearly. There are several unsettled issues here: 1. Should the LAN95xx stuff be associated with the ehci-omap.0's driver or with the hub port? The port would be more flexible, offering the ability to turn the power off and on while the system is running without affecting anything else. But the port code is currently in flux, which could cause this new addition to be delayed. (On the other hand, since the LAN95xx is the only thing connected to the root hub, it could be powered off and on by unbinding the ehci-omap.0 device from its driver and rebinding it.) 2. If we do choose the port, do we want to identify it by matching against a device name string or by matching a sequence of port numbers (in this case, a length-1 sequence)? The port numbers are fixed by the board design, whereas the device name strings might get changed in the future. On the other hand, the port numbers apply only to USB whereas device names can be used by any subsystem. 3. Should the matching mechanism go into the device core or into the USB port code? (This is related to the previous question.) 4. Should this be implemented simply as a regulator (or a pair of regulators)? Or should it be generalized to some sort of PM domain thing? The generic_pm_domain structure defined in include/linux/pm_domain.h seems like overkill, but maybe it's the most appropriate thing to use. Until we decide on the answers to these questions, there's no point writing detailed patches. 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, Dec 3, 2012 at 12:52 PM, Andy Green <andy.green@linaro.org> wrote: >>>> +static void ehci_hub_power_off(struct power_controller *pc, struct >>>> device >>>> *dev) >>>> +{ >>>> + gpio_set_value(GPIO_HUB_NRESET, 0); >>>> + gpio_set_value(GPIO_HUB_POWER, 0); >>>> +} >>>> + >>>> +static struct usb_port_power_switch_data root_hub_port_data = { >>>> + .hub_tier = 0, >>>> + .port_number = 1, >>>> + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, >>>> +}; >>>> + >>>> +static struct usb_port_power_switch_data smsc_hub_port_data = { >>>> + .hub_tier = 1, >>>> + .port_number = 1, >>>> + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, >>>> +}; >>>> + >>>> +static struct power_controller pc = { >>>> + .name = "omap_hub_eth_pc", >>>> + .count = ATOMIC_INIT(0), >>>> + .power_on = ehci_hub_power_on, >>>> + .power_off = ehci_hub_power_off, >>>> +}; >>>> + >>>> +static inline int omap_ehci_hub_port(struct device *dev) >>>> +{ >>>> + /* we expect dev->parent points to ehcd controller */ >>>> + if (dev->parent && !strcmp(dev_name(dev->parent), >>>> "ehci-omap.0")) >>>> + return 1; >>>> + return 0; >>>> +} >>>> + >>>> +static inline int dev_pc_match(struct device *dev) >>>> +{ >>>> + struct device *anc; >>>> + int ret = 0; >>>> + >>>> + if (likely(strcmp(dev_name(dev), "port1"))) >>>> + goto exit; >>>> + >>>> + if (dev->parent && (anc = dev->parent->parent)) { >>>> + if (omap_ehci_hub_port(anc)) { >>>> + ret = 1; >>>> + goto exit; >>>> + } >>>> + >>>> + /* is it port of lan95xx hub? */ >>>> + if ((anc = anc->parent) && omap_ehci_hub_port(anc)) { >>>> + ret = 2; >>>> + goto exit; >>>> + } >>>> + } >>>> +exit: >>>> + return ret; >>>> +} >>>> + >>>> +/* >>>> + * Notifications of device registration >>>> + */ >>>> +static int device_notify(struct notifier_block *nb, unsigned long >>>> action, >>>> void *data) >>>> +{ >>>> + struct device *dev = data; >>>> + int ret; >>>> + >>>> + switch (action) { >>>> + case DEV_NOTIFY_ADD_DEVICE: >>>> + ret = dev_pc_match(dev); >>>> + if (likely(!ret)) >>>> + goto exit; >>>> + if (ret == 1) >>>> + dev_pc_bind(&pc, dev, &root_hub_port_data, >>>> sizeof(root_hub_port_data)); >>>> + else >>>> + dev_pc_bind(&pc, dev, &smsc_hub_port_data, >>>> sizeof(smsc_hub_port_data)); >>>> + break; >>>> + >>>> + case DEV_NOTIFY_DEL_DEVICE: >>>> + break; >>>> + } >>>> +exit: >>>> + return 0; >>>> +} >>>> + >>>> +static struct notifier_block usb_port_nb = { >>>> + .notifier_call = device_notify, >>>> +}; >>>> + >>> >>> >>> >>> Some thoughts on trying to make this functionality specific to power only >>> and ehci hub port only: >>> >>> - Quite a few boards have smsc95xx... they're all going to carry these >>> additions in the board file? Surely you'll have to generalize the pieces >> >> >> All things are board dependent because we are discussing peripheral >> device(not builtin SoC devices), so it is proper to put it in the board >> file. >> If some boards want to share it, we can put it in a single .c file and >> let board file include it. > > > Where would the .c file go... SMSC is not platform, or even arch specific > chip (eg, iMX or MIPS or even x86 boards can have it), so not > arch/arm/mach-xxxx or arch/arm/plat-xxx or arch/arm. I guess it would go in > drivers/usb or drivers/net. How does drivers/usb or drivers/net know the SMSC is used on beagle or panda? Different power control approach is used in the two boards even same SMSC chip is shipped in the two boards. > > Push in ARM-Land is towards single kernels which support many platforms, a > good case in point is omap2plus_defconfg which wants to allow to support all > OMAP2/3/4/5 platforms in one kernel. If you "include" that C file over and > over in board files, it's not very nice for that. They anyway want to > eliminate board files for the single kernel binary reason, and just have one > load of config come in by dtb. I only mean it is reasonable to put the power control code into board file because each board may take different power control approach even same SMSC chip is used. I understand DT only describes the difference of the board via device node, and I am not sure if the current DT is enough to convert the board file into data as far as the problem is concerned. > > So it guides you towards having static helper code once in drivers/ for the > scenarios you support... if that's where you end up smsc is less good a > target for a helper than to have helpers for classes of object like > regulator and clk, that you can combine and reuse on all sorts of target > devices, which is device_asset approach. > > It also guides you to having the special platform sauce be something that > can go into the dtb: per-board code can't. That's why device_asset stuff > only places asset structs in the board file and is removing code from there. > > >>> that perform device_path business out of the omap4panda board file at >>> least. >>> At that point the path matching code becomes generic >>> end-of-the-device-path >>> matching code. >> >> >> Looks Alan has mentioned, there might be no generic way, and any device's >> name change in the path may make the way fragile. > > > What you have provided is no less fragile if you allow "port1" and the > ehci-omap.0 to be set from the outside. > > Unless someone NAKs it for sure already (if you're already sure you're going > to, please do so to avoid wasting time), I'll issue a try#2 of my code later > which demonstrates what I mean. At least I guess it's useful for > comparative purposes. > > >>> - How could these literals like "port1" etc be nicely provided by >>> Device >>> Tree? In ARM-land there's pressure to eventually eliminate board files >>> completely and pass in everything from dtb. device_asset can neatly grow >>> DT >>> bindings in a generic way, since the footprint in the board file is some >> >> >> IMO, it isn't necessary to expose these assets to device or users, from >> the >> view of device or user, which only cares two actions(poweron and poweroff) >> about the discussed problem. Also it should be better to put these assets >> into device resource list, instead of introducing them in 'struct device'. > > > From the point of view of allowing it to be reused on different boards / > platforms / arches, you are going to have to do something better than have > literals in the code. > > >>> regulators that already have dt bindings and some device_asset structs. >>> Similarly there's pressure for magic code to service a board (rather than >>> SoC) to go elsewhere than the board file. >> >> >> Looks you associate these assets with ehci-omap device, which mightn't be >> enough, because we need to control port's power for supporting port >> power off mechanism. Do you have generic way to associate these assets >> with usb port device and let port use it generally? > > > Yes, you need a parent device pointer (ehci host controller in this case) > and the path rhs, but only stuff that is defined by usb stack code. Needing > a parent pointer is OK because this stuff only has meaning for hardwired > assets on the platform, so the parent device will always be known as a > platform_device at boot time anyway. parent device pointer may work on the panda problem, but may not work on other case if one hardwired device is powered by another power domain. So it is not a general solution on usb port power off. > The code I'll provide will work the same in sdio or other bus case, just use > mmc host controller pointer and the sdio device name the same way. > > >>> - Shouldn't this take care of enabling and disabling the ULPI PHY clock >>> on >>> Panda too? There's no purpose leaving it running if the one thing the >>> ULPI >>> PHY is connected to is depowered, and when you do power it, on Panda you >>> will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY >>> reset are connected together on Panda). Then you can eliminate >>> omap4_ehci_init() in the board file. >> >> >> OK, we can include the ULPI PHY clock things easily in ->power_on() and >> ->power_off() of 'power controller' > > > Yes if the ARM people will accept establishing more code in board files that > doesn't have a DT story. As I explained above, it is reasonable to put the power control code in board file, but current DT could convert these board file into device node? Thanks,
On Tue, Dec 4, 2012 at 1:09 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 3 Dec 2012, Andy Green wrote: > >> Unless someone NAKs it for sure already (if you're already sure you're >> going to, please do so to avoid wasting time), I'll issue a try#2 of my >> code later which demonstrates what I mean. At least I guess it's useful >> for comparative purposes. > > Before you go writing a whole lot more code, we should discuss the > basics a bit more clearly. There are several unsettled issues here: > > 1. Should the LAN95xx stuff be associated with the ehci-omap.0's > driver or with the hub port? The port would be more flexible, IMO, it shouldn't be associated with ehci-omap controller driver because the LAN95xx is a external USB device and should be nothing to do with ehci, but it is reasonable to be associated with the hub port because it takes a out of band power control method. > offering the ability to turn the power off and on while the > system is running without affecting anything else. But the > port code is currently in flux, which could cause this new > addition to be delayed. > > (On the other hand, since the LAN95xx is the only thing > connected to the root hub, it could be powered off and on by > unbinding the ehci-omap.0 device from its driver and rebinding > it.) > > 2. If we do choose the port, do we want to identify it by matching > against a device name string or by matching a sequence of port > numbers (in this case, a length-1 sequence)? The port numbers > are fixed by the board design, whereas the device name strings > might get changed in the future. On the other hand, the port > numbers apply only to USB whereas device names can be used by > any subsystem. Alos, the same ehci-omap driver and same LAN95xx chip is used in beagle board and panda board with different power control approach, does port driver can distinguish these two cases? Port device is a general device(not platform device), how does port driver get platform/board dependent info? > > 3. Should the matching mechanism go into the device core or into > the USB port code? (This is related to the previous question.) > > 4. Should this be implemented simply as a regulator (or a pair of > regulators)? Or should it be generalized to some sort of PM > domain thing? The generic_pm_domain structure defined in > include/linux/pm_domain.h seems like overkill, but maybe it's > the most appropriate thing to use. Not only regulators involved, clock or other things might be involved too. Also the same power domain might be shared with more than one port, so it is better to introduce power domain to the problem. Looks generic_pm_domain is overkill, so I introduced power controller which only focuses on power on/off and being shared by multiple devices. Thanks,
On 04/12/12 01:09, the mail apparently from Alan Stern included: > On Mon, 3 Dec 2012, Andy Green wrote: > >> Unless someone NAKs it for sure already (if you're already sure you're >> going to, please do so to avoid wasting time), I'll issue a try#2 of my >> code later which demonstrates what I mean. At least I guess it's useful >> for comparative purposes. > > Before you go writing a whole lot more code, we should discuss the > basics a bit more clearly. There are several unsettled issues here: > 1. Should the LAN95xx stuff be associated with the ehci-omap.0's > driver or with the hub port? The port would be more flexible, > offering the ability to turn the power off and on while the > system is running without affecting anything else. But the > port code is currently in flux, which could cause this new > addition to be delayed. I think associating ULPI PHY reset and SMSC power and reset with the hub port power state is good. Then, you could have the driver in a device with multiple onboard USB devices, and individually control whether they're eating power or not. In the asset case, you'd associate mux assets with ehci-omap.0. Yesterday I studied the hub port code and have a couple of patches, one normalizes the hub port device to have a stub driver. The other then puts hub port power state signalling into runtime_pm handlers in the hub port device. Until now, actually there's no code in hub.c to switch off a port. Assuming that's not insane, what should the user interface to disable a port power look like, something in sysfs? Until now it doesn't seem to exist. > (On the other hand, since the LAN95xx is the only thing > connected to the root hub, it could be powered off and on by > unbinding the ehci-omap.0 device from its driver and rebinding > it.) We shouldn't get to tied up with Panda case, this will be there for all cases like PCs etc. It should work well if there are multiple ports with onboard assets. > 2. If we do choose the port, do we want to identify it by matching > against a device name string or by matching a sequence of port > numbers (in this case, a length-1 sequence)? The port numbers > are fixed by the board design, whereas the device name strings > might get changed in the future. On the other hand, the port > numbers apply only to USB whereas device names can be used by > any subsystem. USB device names contain the port information. The matching scheme I have currently just uses the right-hand side of the path information and nothing that is not defined by the USB subsystem. It uses a platform_device ancestor to restrict matches to descendants of the right host controller. So unlike try#1 the names are as stable as the subsystem code alone, however stable that is, it's not exposed to changes from anywhere else. As you mention it's then workable on any dynamically probed bus. > 3. Should the matching mechanism go into the device core or into > the USB port code? (This is related to the previous question.) Currently I am experimenting with having the asset pointer in struct device, but migrating the events into runtime_resume and runtime_suspend. If it works out that has advantages that assets follow not just the logical device existence but the pm state of the device closely. It also allows leveraging assets directly to the hub port runtime_pm state, so they follow enable state of the port without any additional code. > 4. Should this be implemented simply as a regulator (or a pair of > regulators)? Or should it be generalized to some sort of PM > domain thing? The generic_pm_domain structure defined in > include/linux/pm_domain.h seems like overkill, but maybe it's > the most appropriate thing to use. They should be regulators for that I think. But it's only part the problem since clocks and mux are also going to be commonly associated with device power state, and indeed are in Panda case. I realize restricting the scope is desirable to get something done, but I'm not sure supporting regulators only is enough of the job. > Until we decide on the answers to these questions, there's no point > writing detailed patches. I agree, however I can't get insight into and confidence about what to do without studying and meddling with the code. Some throwaway code is probably a reasonable price. -Andy
On 04/12/12 10:39, the mail apparently from Ming Lei included: > On Mon, Dec 3, 2012 at 12:52 PM, Andy Green <andy.green@linaro.org> wrote: >>>>> +static void ehci_hub_power_off(struct power_controller *pc, struct >>>>> device >>>>> *dev) >>>>> +{ >>>>> + gpio_set_value(GPIO_HUB_NRESET, 0); >>>>> + gpio_set_value(GPIO_HUB_POWER, 0); >>>>> +} >>>>> + >>>>> +static struct usb_port_power_switch_data root_hub_port_data = { >>>>> + .hub_tier = 0, >>>>> + .port_number = 1, >>>>> + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, >>>>> +}; >>>>> + >>>>> +static struct usb_port_power_switch_data smsc_hub_port_data = { >>>>> + .hub_tier = 1, >>>>> + .port_number = 1, >>>>> + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, >>>>> +}; >>>>> + >>>>> +static struct power_controller pc = { >>>>> + .name = "omap_hub_eth_pc", >>>>> + .count = ATOMIC_INIT(0), >>>>> + .power_on = ehci_hub_power_on, >>>>> + .power_off = ehci_hub_power_off, >>>>> +}; >>>>> + >>>>> +static inline int omap_ehci_hub_port(struct device *dev) >>>>> +{ >>>>> + /* we expect dev->parent points to ehcd controller */ >>>>> + if (dev->parent && !strcmp(dev_name(dev->parent), >>>>> "ehci-omap.0")) >>>>> + return 1; >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static inline int dev_pc_match(struct device *dev) >>>>> +{ >>>>> + struct device *anc; >>>>> + int ret = 0; >>>>> + >>>>> + if (likely(strcmp(dev_name(dev), "port1"))) >>>>> + goto exit; >>>>> + >>>>> + if (dev->parent && (anc = dev->parent->parent)) { >>>>> + if (omap_ehci_hub_port(anc)) { >>>>> + ret = 1; >>>>> + goto exit; >>>>> + } >>>>> + >>>>> + /* is it port of lan95xx hub? */ >>>>> + if ((anc = anc->parent) && omap_ehci_hub_port(anc)) { >>>>> + ret = 2; >>>>> + goto exit; >>>>> + } >>>>> + } >>>>> +exit: >>>>> + return ret; >>>>> +} >>>>> + >>>>> +/* >>>>> + * Notifications of device registration >>>>> + */ >>>>> +static int device_notify(struct notifier_block *nb, unsigned long >>>>> action, >>>>> void *data) >>>>> +{ >>>>> + struct device *dev = data; >>>>> + int ret; >>>>> + >>>>> + switch (action) { >>>>> + case DEV_NOTIFY_ADD_DEVICE: >>>>> + ret = dev_pc_match(dev); >>>>> + if (likely(!ret)) >>>>> + goto exit; >>>>> + if (ret == 1) >>>>> + dev_pc_bind(&pc, dev, &root_hub_port_data, >>>>> sizeof(root_hub_port_data)); >>>>> + else >>>>> + dev_pc_bind(&pc, dev, &smsc_hub_port_data, >>>>> sizeof(smsc_hub_port_data)); >>>>> + break; >>>>> + >>>>> + case DEV_NOTIFY_DEL_DEVICE: >>>>> + break; >>>>> + } >>>>> +exit: >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static struct notifier_block usb_port_nb = { >>>>> + .notifier_call = device_notify, >>>>> +}; >>>>> + >>>> >>>> >>>> >>>> Some thoughts on trying to make this functionality specific to power only >>>> and ehci hub port only: >>>> >>>> - Quite a few boards have smsc95xx... they're all going to carry these >>>> additions in the board file? Surely you'll have to generalize the pieces >>> >>> >>> All things are board dependent because we are discussing peripheral >>> device(not builtin SoC devices), so it is proper to put it in the board >>> file. >>> If some boards want to share it, we can put it in a single .c file and >>> let board file include it. >> >> >> Where would the .c file go... SMSC is not platform, or even arch specific >> chip (eg, iMX or MIPS or even x86 boards can have it), so not >> arch/arm/mach-xxxx or arch/arm/plat-xxx or arch/arm. I guess it would go in >> drivers/usb or drivers/net. > > How does drivers/usb or drivers/net know the SMSC is used on beagle or > panda? Different power control approach is used in the two boards even > same SMSC chip is shipped in the two boards. You mention you're going to "put it in a single .c file and let the board file include it", I am just wondering where that .c file will live. It seems it would have to live down drivers/ somewhere so MIPS, x86 etc that might have an SMSC chip onboard can also use it if they want. >> Push in ARM-Land is towards single kernels which support many platforms, a >> good case in point is omap2plus_defconfg which wants to allow to support all >> OMAP2/3/4/5 platforms in one kernel. If you "include" that C file over and >> over in board files, it's not very nice for that. They anyway want to >> eliminate board files for the single kernel binary reason, and just have one >> load of config come in by dtb. > > I only mean it is reasonable to put the power control code into board > file because > each board may take different power control approach even same SMSC chip > is used. I understand DT only describes the difference of the board via device > node, and I am not sure if the current DT is enough to convert the board file > into data as far as the problem is concerned. No the approach with DT is to provide a dummy SoC "board file" with all data provided by dtb. This is already established, see ./arch/arm/mach-omap2/board-generic.c for example. You can see there's nothing in it other than minimum dt match business. People with new boards are getting pointed at that and told to sort it out in dtb and disallowed from creating new board files. So whatever support or helper scheme you're adding needs a story about how dtb can express it (in dt language, "has bindings for it") or it can't be used on new boards. That's not a minor ding any more either. >> So it guides you towards having static helper code once in drivers/ for the >> scenarios you support... if that's where you end up smsc is less good a >> target for a helper than to have helpers for classes of object like >> regulator and clk, that you can combine and reuse on all sorts of target >> devices, which is device_asset approach. >> >> It also guides you to having the special platform sauce be something that >> can go into the dtb: per-board code can't. That's why device_asset stuff >> only places asset structs in the board file and is removing code from there. >> >> >>>> that perform device_path business out of the omap4panda board file at >>>> least. >>>> At that point the path matching code becomes generic >>>> end-of-the-device-path >>>> matching code. >>> >>> >>> Looks Alan has mentioned, there might be no generic way, and any device's >>> name change in the path may make the way fragile. >> >> >> What you have provided is no less fragile if you allow "port1" and the >> ehci-omap.0 to be set from the outside. >> >> Unless someone NAKs it for sure already (if you're already sure you're going >> to, please do so to avoid wasting time), I'll issue a try#2 of my code later >> which demonstrates what I mean. At least I guess it's useful for >> comparative purposes. >> >> >>>> - How could these literals like "port1" etc be nicely provided by >>>> Device >>>> Tree? In ARM-land there's pressure to eventually eliminate board files >>>> completely and pass in everything from dtb. device_asset can neatly grow >>>> DT >>>> bindings in a generic way, since the footprint in the board file is some >>> >>> >>> IMO, it isn't necessary to expose these assets to device or users, from >>> the >>> view of device or user, which only cares two actions(poweron and poweroff) >>> about the discussed problem. Also it should be better to put these assets >>> into device resource list, instead of introducing them in 'struct device'. >> >> >> From the point of view of allowing it to be reused on different boards / >> platforms / arches, you are going to have to do something better than have >> literals in the code. >> >> >>>> regulators that already have dt bindings and some device_asset structs. >>>> Similarly there's pressure for magic code to service a board (rather than >>>> SoC) to go elsewhere than the board file. >>> >>> >>> Looks you associate these assets with ehci-omap device, which mightn't be >>> enough, because we need to control port's power for supporting port >>> power off mechanism. Do you have generic way to associate these assets >>> with usb port device and let port use it generally? >> >> >> Yes, you need a parent device pointer (ehci host controller in this case) >> and the path rhs, but only stuff that is defined by usb stack code. Needing >> a parent pointer is OK because this stuff only has meaning for hardwired >> assets on the platform, so the parent device will always be known as a >> platform_device at boot time anyway. > > parent device pointer may work on the panda problem, but may not work > on other case if one hardwired device is powered by another power domain. > > So it is not a general solution on usb port power off. I am talking about, well, "ancestor" pointer only to filter descendant children. Not for any power control directly. It gets us away from caring about what the device path looked like prior to that host controller, and since we have confidence it's exactly the host controller we intended by knowing its platform_device, we can ignore everything between than at the right-hand side path fragment identifying the child. So it's a strong way to reduce fragility on the device_path stuff. >> The code I'll provide will work the same in sdio or other bus case, just use >> mmc host controller pointer and the sdio device name the same way. >> >> >>>> - Shouldn't this take care of enabling and disabling the ULPI PHY clock >>>> on >>>> Panda too? There's no purpose leaving it running if the one thing the >>>> ULPI >>>> PHY is connected to is depowered, and when you do power it, on Panda you >>>> will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY >>>> reset are connected together on Panda). Then you can eliminate >>>> omap4_ehci_init() in the board file. >>> >>> >>> OK, we can include the ULPI PHY clock things easily in ->power_on() and >>> ->power_off() of 'power controller' >> >> >> Yes if the ARM people will accept establishing more code in board files that >> doesn't have a DT story. > > As I explained above, it is reasonable to put the power control code in board > file, but current DT could convert these board file into device node? DT has lots of bindings now, you can describe regulators and things in there fully. The point is whatever scheme is workable for this must be able to get soaked up using DT too to be acceptable. Taking the approach you're going to drop literal strings in code in the board file, or throw code at the board file at all, will no longer fly. I have not provided a DT solution for my approach yet either, but I have taken care that the only footprint in the boardfile version of it are *structs*. That means translating them to dtb content by adding a binding for the asset stuff for example, will be a clean task. That's also why back at the beginning of this discussion I gave dts version of the regulator structs I was introducing in that patch... it's proof that the boardfile footprint of that scheme is ready to go into dtb. -Andy
[CC: list trimmed; the people who were on it should be subscribed to at least one of the lists anyway.] On Tue, 4 Dec 2012, Andy Green wrote: > I think associating ULPI PHY reset and SMSC power and reset with the hub > port power state is good. Then, you could have the driver in a device > with multiple onboard USB devices, and individually control whether > they're eating power or not. In the asset case, you'd associate mux > assets with ehci-omap.0. > > Yesterday I studied the hub port code and have a couple of patches, one > normalizes the hub port device to have a stub driver. > > The other then puts hub port power state signalling into runtime_pm > handlers in the hub port device. Until now, actually there's no code in > hub.c to switch off a port. In fact that's not quite true. You simply weren't aware of the new code; you can find a series of patches starting here: http://marc.info/?l=linux-usb&m=135314427413307&w=2 The parts of interest to us begin in patch 7/10. > Assuming that's not insane, what should the user interface to disable a > port power look like, something in sysfs? Until now it doesn't seem to > exist. It will be implemented through PM QOS. > > (On the other hand, since the LAN95xx is the only thing > > connected to the root hub, it could be powered off and on by > > unbinding the ehci-omap.0 device from its driver and rebinding > > it.) > > We shouldn't get to tied up with Panda case, this will be there for all > cases like PCs etc. It should work well if there are multiple ports > with onboard assets. Okay, I'm fine with tying this to the port. > > 2. If we do choose the port, do we want to identify it by matching > > against a device name string or by matching a sequence of port > > numbers (in this case, a length-1 sequence)? The port numbers > > are fixed by the board design, whereas the device name strings > > might get changed in the future. On the other hand, the port > > numbers apply only to USB whereas device names can be used by > > any subsystem. > > USB device names contain the port information. The matching scheme I > have currently just uses the right-hand side of the path information and > nothing that is not defined by the USB subsystem. It uses a > platform_device ancestor to restrict matches to descendants of the right > host controller. So unlike try#1 the names are as stable as the > subsystem code alone, however stable that is, it's not exposed to > changes from anywhere else. As you mention it's then workable on any > dynamically probed bus. > > > 3. Should the matching mechanism go into the device core or into > > the USB port code? (This is related to the previous question.) > > Currently I am experimenting with having the asset pointer in struct > device, but migrating the events into runtime_resume and > runtime_suspend. If it works out that has advantages that assets follow > not just the logical device existence but the pm state of the device > closely. > > It also allows leveraging assets directly to the hub port runtime_pm > state, so they follow enable state of the port without any additional code. If we use a PM domain then there won't be any need to hook the runtime PM events. The domain will automatically be notified about power changes. > > 4. Should this be implemented simply as a regulator (or a pair of > > regulators)? Or should it be generalized to some sort of PM > > domain thing? The generic_pm_domain structure defined in > > include/linux/pm_domain.h seems like overkill, but maybe it's > > the most appropriate thing to use. > > They should be regulators for that I think. But it's only part the > problem since clocks and mux are also going to be commonly associated > with device power state, and indeed are in Panda case. > > I realize restricting the scope is desirable to get something done, but > I'm not sure supporting regulators only is enough of the job. Then why not use a PM domain? It will allow us to do whatever we want in the callbacks. On Tue, 4 Dec 2012, Ming Lei wrote: > Alos, the same ehci-omap driver and same LAN95xx chip is used in > beagle board and panda board with different power control > approach, does port driver can distinguish these two cases? > Port device is a general device(not platform device), how does > port driver get platform/board dependent info? This is the part that Andy has been working on. The board-dependent info will be registered by the board file, and it will take effect either when the port is registered or when it is bound to a driver. The details of this aren't clear yet. For instance, should the device core try to match the port with the asset info, or should this be done by the USB code when the port is created? > Not only regulators involved, clock or other things might be involved too. > Also the same power domain might be shared with more than one port, > so it is better to introduce power domain to the problem. Looks > generic_pm_domain is overkill, so I introduced power controller which > only focuses on power on/off and being shared by multiple devices. Even though it is overkill, it may be better than introducing a new abstraction. After all, this is exactly the sort of thing that PM domains were originally created to handle. Rafael, do you have any advice on this? The generic_pm_domain structure is fairly complicated, there's a lot of code in drivers/base/power/domain.c (it's the biggest source file in its directory), and I'm not aware of any documentation. 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, Dec 04, 2012 at 11:40:05AM +0800, Andy Green wrote: > On 04/12/12 01:09, the mail apparently from Alan Stern included: > >On Mon, 3 Dec 2012, Andy Green wrote: > > > >>Unless someone NAKs it for sure already (if you're already sure you're > >>going to, please do so to avoid wasting time), I'll issue a try#2 of my > >>code later which demonstrates what I mean. At least I guess it's useful > >>for comparative purposes. > > > >Before you go writing a whole lot more code, we should discuss the > >basics a bit more clearly. There are several unsettled issues here: > > > 1. Should the LAN95xx stuff be associated with the ehci-omap.0's > > driver or with the hub port? The port would be more flexible, > > offering the ability to turn the power off and on while the > > system is running without affecting anything else. But the > > port code is currently in flux, which could cause this new > > addition to be delayed. > > I think associating ULPI PHY reset and SMSC power and reset with the > hub port power state is good. Then, you could have the driver in a > device with multiple onboard USB devices, and individually control > whether they're eating power or not. In the asset case, you'd > associate mux assets with ehci-omap.0. > > Yesterday I studied the hub port code and have a couple of patches, > one normalizes the hub port device to have a stub driver. > > The other then puts hub port power state signalling into runtime_pm > handlers in the hub port device. Until now, actually there's no > code in hub.c to switch off a port. Did you take a look at the most recent patches from Tianyu to add support to power off a port if a device is suspended? Start of the series: http://marc.info/?l=linux-usb&m=135314427413307&w=2 Patch that adds power off on device suspend: http://marc.info/?l=linux-usb&m=135314431913321&w=2 Tianyu also added some code to the xHCI host controller driver to call into the ACPI methods to power off a port when the USB hub driver clears the port power feature. Sarah Sharp -- 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 05/12/12 01:10, the mail apparently from Alan Stern included: > [CC: list trimmed; the people who were on it should be subscribed to at > least one of the lists anyway.] > > On Tue, 4 Dec 2012, Andy Green wrote: > >> I think associating ULPI PHY reset and SMSC power and reset with the hub >> port power state is good. Then, you could have the driver in a device >> with multiple onboard USB devices, and individually control whether >> they're eating power or not. In the asset case, you'd associate mux >> assets with ehci-omap.0. >> >> Yesterday I studied the hub port code and have a couple of patches, one >> normalizes the hub port device to have a stub driver. >> >> The other then puts hub port power state signalling into runtime_pm >> handlers in the hub port device. Until now, actually there's no code in >> hub.c to switch off a port. > > In fact that's not quite true. You simply weren't aware of the new > code; you can find a series of patches starting here: > > http://marc.info/?l=linux-usb&m=135314427413307&w=2 > > The parts of interest to us begin in patch 7/10. Yes I have been looking in usb-next. >> Assuming that's not insane, what should the user interface to disable a >> port power look like, something in sysfs? Until now it doesn't seem to >> exist. > > It will be implemented through PM QOS. OK. I saw "[PATCH 09/10] usb: expose usb port's pm qos flags to user space" now. >>> (On the other hand, since the LAN95xx is the only thing >>> connected to the root hub, it could be powered off and on by >>> unbinding the ehci-omap.0 device from its driver and rebinding >>> it.) >> >> We shouldn't get to tied up with Panda case, this will be there for all >> cases like PCs etc. It should work well if there are multiple ports >> with onboard assets. > > Okay, I'm fine with tying this to the port. OK. >>> 2. If we do choose the port, do we want to identify it by matching >>> against a device name string or by matching a sequence of port >>> numbers (in this case, a length-1 sequence)? The port numbers >>> are fixed by the board design, whereas the device name strings >>> might get changed in the future. On the other hand, the port >>> numbers apply only to USB whereas device names can be used by >>> any subsystem. >> >> USB device names contain the port information. The matching scheme I >> have currently just uses the right-hand side of the path information and >> nothing that is not defined by the USB subsystem. It uses a >> platform_device ancestor to restrict matches to descendants of the right >> host controller. So unlike try#1 the names are as stable as the >> subsystem code alone, however stable that is, it's not exposed to >> changes from anywhere else. As you mention it's then workable on any >> dynamically probed bus. >> >>> 3. Should the matching mechanism go into the device core or into >>> the USB port code? (This is related to the previous question.) >> >> Currently I am experimenting with having the asset pointer in struct >> device, but migrating the events into runtime_resume and >> runtime_suspend. If it works out that has advantages that assets follow >> not just the logical device existence but the pm state of the device >> closely. >> >> It also allows leveraging assets directly to the hub port runtime_pm >> state, so they follow enable state of the port without any additional code. > > If we use a PM domain then there won't be any need to hook the runtime > PM events. The domain will automatically be notified about power > changes. OK. >>> 4. Should this be implemented simply as a regulator (or a pair of >>> regulators)? Or should it be generalized to some sort of PM >>> domain thing? The generic_pm_domain structure defined in >>> include/linux/pm_domain.h seems like overkill, but maybe it's >>> the most appropriate thing to use. >> >> They should be regulators for that I think. But it's only part the >> problem since clocks and mux are also going to be commonly associated >> with device power state, and indeed are in Panda case. >> >> I realize restricting the scope is desirable to get something done, but >> I'm not sure supporting regulators only is enough of the job. > > Then why not use a PM domain? It will allow us to do whatever we want > in the callbacks. I see, I never met them before now is the reason ^^. You're right it's already in struct device too, and it's much more plumbed into the future apis than what I have been doing. I'll study how to change what I have to fit this and do so. > On Tue, 4 Dec 2012, Ming Lei wrote: > >> Alos, the same ehci-omap driver and same LAN95xx chip is used in >> beagle board and panda board with different power control >> approach, does port driver can distinguish these two cases? >> Port device is a general device(not platform device), how does >> port driver get platform/board dependent info? > > This is the part that Andy has been working on. The board-dependent > info will be registered by the board file, and it will take effect > either when the port is registered or when it is bound to a driver. > > The details of this aren't clear yet. For instance, should the device > core try to match the port with the asset info, or should this be done > by the USB code when the port is created? Currently what I have (this is before changing it to pm domain, but this should be unchanged) lets the asset define a callback op which will receive notifications for all added child devices that have the device the asset is bound to as an ancestor. So you would bind an asset to the host controller device... static struct device_asset assets_ehci_omap0[] = { { .name = "-0:1.0/port1", .asset = assets_ehci_omap0_smsc_port, .ops = &device_descendant_attach_default_asset_ops, }, { } }; ...with this descendant filter callback pointing to a generic "end of the device path" matcher. when device_descendant_attach_default_asset_ops() sees the child that was foretold has appeared (and it only hears about children of ehci-omap.0 in this case), it binds the assets pointed to by .asset to the new child before its probe. assets_ehci_omap0_smsc_port is an array of the actual regulator and clock that need switching by the child. So the effect is to magic the right assets to the child device just before it gets added (and probe called etc). This is working well and the matcher helper is small and simple. >> Not only regulators involved, clock or other things might be involved too. >> Also the same power domain might be shared with more than one port, >> so it is better to introduce power domain to the problem. Looks >> generic_pm_domain is overkill, so I introduced power controller which >> only focuses on power on/off and being shared by multiple devices. > > Even though it is overkill, it may be better than introducing a new > abstraction. After all, this is exactly the sort of thing that PM > domains were originally created to handle. It's looking good to me. -Andy > Rafael, do you have any advice on this? The generic_pm_domain > structure is fairly complicated, there's a lot of code in > drivers/base/power/domain.c (it's the biggest source file in its > directory), and I'm not aware of any documentation. > > Alan Stern >
On 05/12/12 02:14, the mail apparently from Sarah Sharp included: > On Tue, Dec 04, 2012 at 11:40:05AM +0800, Andy Green wrote: >> On 04/12/12 01:09, the mail apparently from Alan Stern included: >>> On Mon, 3 Dec 2012, Andy Green wrote: >>> >>>> Unless someone NAKs it for sure already (if you're already sure you're >>>> going to, please do so to avoid wasting time), I'll issue a try#2 of my >>>> code later which demonstrates what I mean. At least I guess it's useful >>>> for comparative purposes. >>> >>> Before you go writing a whole lot more code, we should discuss the >>> basics a bit more clearly. There are several unsettled issues here: >> >>> 1. Should the LAN95xx stuff be associated with the ehci-omap.0's >>> driver or with the hub port? The port would be more flexible, >>> offering the ability to turn the power off and on while the >>> system is running without affecting anything else. But the >>> port code is currently in flux, which could cause this new >>> addition to be delayed. >> >> I think associating ULPI PHY reset and SMSC power and reset with the >> hub port power state is good. Then, you could have the driver in a >> device with multiple onboard USB devices, and individually control >> whether they're eating power or not. In the asset case, you'd >> associate mux assets with ehci-omap.0. >> >> Yesterday I studied the hub port code and have a couple of patches, >> one normalizes the hub port device to have a stub driver. >> >> The other then puts hub port power state signalling into runtime_pm >> handlers in the hub port device. Until now, actually there's no >> code in hub.c to switch off a port. > > Did you take a look at the most recent patches from Tianyu to add > support to power off a port if a device is suspended? > > Start of the series: > http://marc.info/?l=linux-usb&m=135314427413307&w=2 > Patch that adds power off on device suspend: > http://marc.info/?l=linux-usb&m=135314431913321&w=2 > > Tianyu also added some code to the xHCI host controller driver to call > into the ACPI methods to power off a port when the USB hub driver clears > the port power feature. No I didn't know about it, I will study these along with pm_domain stuff thanks. -Andy
On Wed, 5 Dec 2012, Andy Green wrote: > > The details of this aren't clear yet. For instance, should the device > > core try to match the port with the asset info, or should this be done > > by the USB code when the port is created? > > Currently what I have (this is before changing it to pm domain, but this > should be unchanged) lets the asset define a callback op which will > receive notifications for all added child devices that have the device > the asset is bound to as an ancestor. > > So you would bind an asset to the host controller device... > > static struct device_asset assets_ehci_omap0[] = { > { > .name = "-0:1.0/port1", > .asset = assets_ehci_omap0_smsc_port, > .ops = &device_descendant_attach_default_asset_ops, > }, > { } > }; > > ...with this descendant filter callback pointing to a generic "end of > the device path" matcher. > > when device_descendant_attach_default_asset_ops() sees the child that > was foretold has appeared (and it only hears about children of > ehci-omap.0 in this case), it binds the assets pointed to by .asset to > the new child before its probe. assets_ehci_omap0_smsc_port is an array > of the actual regulator and clock that need switching by the child. So > the effect is to magic the right assets to the child device just before > it gets added (and probe called etc). > > This is working well and the matcher helper is small and simple. Right. The question is should it be done in this somewhat roundabout way (you've got two separate assets for setting up this one thing), or should the USB port code be responsible for explicitly checking if there are any applicable assets when the port is created? The advantange of the second approach is that it doesn't involve checking all the ancestors every time a new device is added (and it involves only one asset). The disadvantage is that it works only for USB ports. To answer the question, we need to know how other subsystems might want to use the asset-matching approach. My guess is that only a small number of device types would care about assets (in the same way that assets matter to USB ports but not to other USB device types). And it might not be necessary to check against every ancestor; we might know beforehand where to check (just as the USB port would know to look for an asset attached to the host controller device). 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
Hi, * Ming Lei <tom.leiming@gmail.com> [121202 07:05]: > --- a/arch/arm/mach-omap2/board-omap4panda.c > +++ b/arch/arm/mach-omap2/board-omap4panda.c ... > + > +static struct notifier_block usb_port_nb = { > + .notifier_call = device_notify, > +}; > + We'll be flipping omap4 over to be device tree only soon. So let's not make the conversion more complex by adding more platform code. This means that for am33xx, omap4, and omap5 this code can be device tree only code. Regards, Tony -- 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 06/12/12 00:42, the mail apparently from Alan Stern included: > On Wed, 5 Dec 2012, Andy Green wrote: > >>> The details of this aren't clear yet. For instance, should the device >>> core try to match the port with the asset info, or should this be done >>> by the USB code when the port is created? >> >> Currently what I have (this is before changing it to pm domain, but this >> should be unchanged) lets the asset define a callback op which will >> receive notifications for all added child devices that have the device >> the asset is bound to as an ancestor. >> >> So you would bind an asset to the host controller device... >> >> static struct device_asset assets_ehci_omap0[] = { >> { >> .name = "-0:1.0/port1", >> .asset = assets_ehci_omap0_smsc_port, >> .ops = &device_descendant_attach_default_asset_ops, >> }, >> { } >> }; >> >> ...with this descendant filter callback pointing to a generic "end of >> the device path" matcher. >> >> when device_descendant_attach_default_asset_ops() sees the child that >> was foretold has appeared (and it only hears about children of >> ehci-omap.0 in this case), it binds the assets pointed to by .asset to >> the new child before its probe. assets_ehci_omap0_smsc_port is an array >> of the actual regulator and clock that need switching by the child. So >> the effect is to magic the right assets to the child device just before >> it gets added (and probe called etc). >> >> This is working well and the matcher helper is small and simple. > > Right. The question is should it be done in this somewhat roundabout > way (you've got two separate assets for setting up this one thing), or > should the USB port code be responsible for explicitly checking if > there are any applicable assets when the port is created? > > The advantange of the second approach is that it doesn't involve > checking all the ancestors every time a new device is added (and it > involves only one asset). The disadvantage is that it works only for > USB ports. It's done in two steps to strongly filter candidate child devices against being children of a known platform device. If you go around that, you will be back to full device path matching with wildcards at the USB child to determine if he is "the one". So that's a feature not an issue I think. I can see doing it generically or not is equally a political issue as a technical one, but I think if we bother to add this kind of support we should prefer to do it generally. It's going to be about the same amount of code. As Tony Lindgren said, even board-omap4panda.c itself is deprecated, to project platform info into USB stack you either have to add DT code into usb stack then to go find things directly, or provide a generic methodology like assets which have the dt bindings done outside of any subsystem and apply their operations outside the subsystem too. > To answer the question, we need to know how other subsystems might > want to use the asset-matching approach. My guess is that only a small > number of device types would care about assets (in the same way that > assets matter to USB ports but not to other USB device types). And it > might not be necessary to check against every ancestor; we might know > beforehand where to check (just as the USB port would know to look for > an asset attached to the host controller device). Yes I think it boils down to "buses" in general can benefit from this. They're the thing that dynamically - later - create child devices you might need to target with what was ultimately platform information. On Panda the other bus thing that can benefit is the WLAN module on SDIO. In fact that's a very illuminating case for your question above. Faced with exactly the same problem, that they needed to project platform info on to SDIO-probed device instance to tell it what clock rate it had been given, they invented an api which you can see today in board-omap4panda.c and other boards there, wl12xx_set_platform_data(). This is from board-4430sdp: static struct wl12xx_platform_data omap4_sdp4430_wlan_data __initdata = { .board_ref_clock = WL12XX_REFCLOCK_26, .board_tcxo_clock = WL12XX_TCXOCLOCK_26, }; ... ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data); You can find the other end of it here in drivers/net/wireless/ti/wlcore/wl12xx_platform_data.c static struct wl12xx_platform_data *platform_data; int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data) { if (platform_data) return -EBUSY; if (!data) return -EINVAL; platform_data = kmemdup(data, sizeof(*data), GFP_KERNEL); if (!platform_data) return -ENOMEM; return 0; } when the driver for the device instance wants to "get" its platform data it reads the static pointer via another api. Obviously if you want two modules on your platform that's not so good. I doubt that's the only SDIO device that wants to know platform info. So I think by providing a generic way we help other buses and devices. -Andy
Hi, Well, I'm not any less busy than yesterday, as it turns out, but I'm expecting that to continue tomorrow, so I may as well have a look at it now. :-) On Tuesday, December 04, 2012 12:10:32 PM Alan Stern wrote: > [CC: list trimmed; the people who were on it should be subscribed to at > least one of the lists anyway.] > [...] > > > Not only regulators involved, clock or other things might be involved too. > > Also the same power domain might be shared with more than one port, > > so it is better to introduce power domain to the problem. Looks > > generic_pm_domain is overkill, so I introduced power controller which > > only focuses on power on/off and being shared by multiple devices. > > Even though it is overkill, it may be better than introducing a new > abstraction. After all, this is exactly the sort of thing that PM > domains were originally created to handle. > > Rafael, do you have any advice on this? The generic_pm_domain > structure is fairly complicated, there's a lot of code in > drivers/base/power/domain.c (it's the biggest source file in its > directory), and I'm not aware of any documentation. Yeah, documentation is missing, which obviously is my fault. That code is designed to cover the case in which multiple devices share a "power switch" that can be used to remove power from all of them at once (eg. through a register write). It also assumes that there will be a "stop" operation, such as "disable clock", allowing each device in the domain to be put into a shallow low-power state (individually) without the necessity to save its state. Device states only have to be saved when the "power switch" is about to be used, which generally happens when they all have been "stopped" (their runtime PM status is RPM_SUSPENDED). The "stop" operation may be defined in a different way for each device in the domain (actually, that applies to the "save state", "restore state", and "start" - opposite to "stop" - operations too) and PM QoS latency constraints are used to decide if and when to turn the whole domain off. Moreover, it supports hierarchies of power domains that may be pretty much arbitarily complicated. A big part of this code is for the handling of system suspend/resume in such a way that it is consistent with runtime PM. For USB ports I'd recommend to use something simpler. :-) Thanks, Rafael
On Thu, 6 Dec 2012, Andy Green wrote: > > Right. The question is should it be done in this somewhat roundabout > > way (you've got two separate assets for setting up this one thing), or > > should the USB port code be responsible for explicitly checking if > > there are any applicable assets when the port is created? > > > > The advantange of the second approach is that it doesn't involve > > checking all the ancestors every time a new device is added (and it > > involves only one asset). The disadvantage is that it works only for > > USB ports. > > It's done in two steps to strongly filter candidate child devices > against being children of a known platform device. If you go around > that, you will be back to full device path matching with wildcards at > the USB child to determine if he is "the one". So that's a feature not > an issue I think. I don't follow. Suppose an asset is attached to ehci-omap.0 with its string set to "-0:1.0/port1" and the other members pointing to the regulator, clock and so on. And suppose the USB port code, when creating a new port, checks for a name match against all the assets attached to its bus's host controller device structure. That should do exactly what you want while using only one asset. > I can see doing it generically or not is equally a political issue as a > technical one, but I think if we bother to add this kind of support we > should prefer to do it generally. It's going to be about the same > amount of code. Not quite. If you do it generally then you have to insert hooks at all the places where a subsystem _might_ need it. If you do it specifically then no hooks are needed at all -- just a match call at the right place in the subsystem that needs it. > As Tony Lindgren said, even board-omap4panda.c itself is deprecated, to > project platform info into USB stack you either have to add DT code into > usb stack then to go find things directly, or provide a generic > methodology like assets which have the dt bindings done outside of any > subsystem and apply their operations outside the subsystem too. Assuming DT can be extended to support assets, adding one asset will be simpler than adding two. > > To answer the question, we need to know how other subsystems might > > want to use the asset-matching approach. My guess is that only a small > > number of device types would care about assets (in the same way that > > assets matter to USB ports but not to other USB device types). And it > > might not be necessary to check against every ancestor; we might know > > beforehand where to check (just as the USB port would know to look for > > an asset attached to the host controller device). > > Yes I think it boils down to "buses" in general can benefit from this. > They're the thing that dynamically - later - create child devices you > might need to target with what was ultimately platform information. > > On Panda the other bus thing that can benefit is the WLAN module on > SDIO. In fact that's a very illuminating case for your question above. > Faced with exactly the same problem, that they needed to project > platform info on to SDIO-probed device instance to tell it what clock > rate it had been given, they invented an api which you can see today in > board-omap4panda.c and other boards there, wl12xx_set_platform_data(). > This is from board-4430sdp: > > static struct wl12xx_platform_data omap4_sdp4430_wlan_data __initdata = { > .board_ref_clock = WL12XX_REFCLOCK_26, > .board_tcxo_clock = WL12XX_TCXOCLOCK_26, > }; > > ... > > ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data); > > You can find the other end of it here in > drivers/net/wireless/ti/wlcore/wl12xx_platform_data.c > > static struct wl12xx_platform_data *platform_data; > > int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data) > { > if (platform_data) > return -EBUSY; > if (!data) > return -EINVAL; > > platform_data = kmemdup(data, sizeof(*data), GFP_KERNEL); > if (!platform_data) > return -ENOMEM; > > return 0; > } > > when the driver for the device instance wants to "get" its platform data > it reads the static pointer via another api. Obviously if you want two > modules on your platform that's not so good. Also it isn't type-safe, although that's probably not a big concern. > I doubt that's the only SDIO device that wants to know platform info. > So I think by providing a generic way we help other buses and devices. I agree, assets look like a good way to improve this. In fact, to some extent assets appear to be a generalization or extension of platform data. 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
diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c index 5c8e9ce..3183832 100644 --- a/arch/arm/mach-omap2/board-omap4panda.c +++ b/arch/arm/mach-omap2/board-omap4panda.c @@ -32,6 +32,8 @@ #include <linux/usb/musb.h> #include <linux/wl12xx.h> #include <linux/platform_data/omap-abe-twl6040.h> +#include <linux/power_controller.h> +#include <linux/usb/port.h> #include <asm/hardware/gic.h> #include <asm/mach-types.h> @@ -154,6 +156,99 @@ static struct gpio panda_ehci_gpios[] __initdata = { { GPIO_HUB_NRESET, GPIOF_OUT_INIT_LOW, "hub_nreset" }, }; +static void ehci_hub_power_on(struct power_controller *pc, struct device *dev) +{ + gpio_set_value(GPIO_HUB_NRESET, 1); + gpio_set_value(GPIO_HUB_POWER, 1); +} + +static void ehci_hub_power_off(struct power_controller *pc, struct device *dev) +{ + gpio_set_value(GPIO_HUB_NRESET, 0); + gpio_set_value(GPIO_HUB_POWER, 0); +} + +static struct usb_port_power_switch_data root_hub_port_data = { + .hub_tier = 0, + .port_number = 1, + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, +}; + +static struct usb_port_power_switch_data smsc_hub_port_data = { + .hub_tier = 1, + .port_number = 1, + .type = USB_PORT_CONNECT_TYPE_HARD_WIRED, +}; + +static struct power_controller pc = { + .name = "omap_hub_eth_pc", + .count = ATOMIC_INIT(0), + .power_on = ehci_hub_power_on, + .power_off = ehci_hub_power_off, +}; + +static inline int omap_ehci_hub_port(struct device *dev) +{ + /* we expect dev->parent points to ehcd controller */ + if (dev->parent && !strcmp(dev_name(dev->parent), "ehci-omap.0")) + return 1; + return 0; +} + +static inline int dev_pc_match(struct device *dev) +{ + struct device *anc; + int ret = 0; + + if (likely(strcmp(dev_name(dev), "port1"))) + goto exit; + + if (dev->parent && (anc = dev->parent->parent)) { + if (omap_ehci_hub_port(anc)) { + ret = 1; + goto exit; + } + + /* is it port of lan95xx hub? */ + if ((anc = anc->parent) && omap_ehci_hub_port(anc)) { + ret = 2; + goto exit; + } + } +exit: + return ret; +} + +/* + * Notifications of device registration + */ +static int device_notify(struct notifier_block *nb, unsigned long action, void *data) +{ + struct device *dev = data; + int ret; + + switch (action) { + case DEV_NOTIFY_ADD_DEVICE: + ret = dev_pc_match(dev); + if (likely(!ret)) + goto exit; + if (ret == 1) + dev_pc_bind(&pc, dev, &root_hub_port_data, sizeof(root_hub_port_data)); + else + dev_pc_bind(&pc, dev, &smsc_hub_port_data, sizeof(smsc_hub_port_data)); + break; + + case DEV_NOTIFY_DEL_DEVICE: + break; + } +exit: + return 0; +} + +static struct notifier_block usb_port_nb = { + .notifier_call = device_notify, +}; + static void __init omap4_ehci_init(void) { int ret; @@ -178,12 +273,10 @@ static void __init omap4_ehci_init(void) gpio_export(GPIO_HUB_POWER, 0); gpio_export(GPIO_HUB_NRESET, 0); - gpio_set_value(GPIO_HUB_NRESET, 1); usbhs_init(&usbhs_bdata); - /* enable power to hub */ - gpio_set_value(GPIO_HUB_POWER, 1); + dev_register_notifier(&usb_port_nb); } static struct omap_musb_board_data musb_board_data = {
This patch defines power controller for powering on/off LAN95xx USB hub and USB ethernet devices, and implements one match function to associate the power controller with related USB port device. The big problem of this approach is that it depends on the global device ADD/DEL notifier. Another idea of associating power controller with port device is by introducing usb port driver, and move all this port power control stuff from platform code to the port driver, which is just what I think of and looks doable. The problem of the idea is that port driver is per board, so maybe cause lots of platform sort of code to be put under drivers/usb/port/, but this approach can avoid global device ADD/DEL notifier. I'd like to get some feedback about which one is better or other choice, then I may do it in next cycle. Cc: Andy Green <andy.green@linaro.org> Cc: Roger Quadros <rogerq@ti.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Felipe Balbi <balbi@ti.com> Signed-off-by: Ming Lei <tom.leiming@gmail.com> --- arch/arm/mach-omap2/board-omap4panda.c | 99 +++++++++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 3 deletions(-)