| Message ID | 2042394.b4eZkzrJ5f@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 07, 2017 at 03:03:06PM +0200, Rafael J. Wysocki wrote: > On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote: > > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote: > > > > > > > > - Can I write new code for it or I need to depend on something? > > > > > > There is nothing this code needs to depend on AFAICS, but there are existing > > > solutions in this problem space (ACPI power management, genpd), so it needs to > > > be careful enough about possible overlaps etc. > > > > > > > I find there is already "power state" concept at documentation. > > > > Documentation/ABI/testing/sysfs-devices-power_state > > > > > > This is ACPI-specific and only in sysfs directories representing ACPI device > > > objects (which aren't physical devices). > > > > > > Anyway, since ACPI covers the problem space you are working in already, > > > your code has to be mutually exclusive with it. > > > > > > > - If I can write the new code for it, except the problems I want > > > > to fix, are there any other use cases I need to consider? > > > > > > I would start simple and focus on the particular problem at hand, that is > > > devices with two power states ("on" and "off") where the "on" state > > > depends on a number of clocks and/or GPIOs. Still, I'd also avoid making > > > design choices that might prevent it from being extended in the future > > > if need be. > > > > > > One major problem I can see is how to "attach" the power states framework > > > to a particular device (once we have discovered that it should be used with > > > that device). > > > > > > For bus types that don't do power management of their own you could follow > > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types > > > doing their own PM (like USB) will probably need to be treated differently. > > > In those cases the bus type code will have to know that it should call some > > > helpers to switch power states of devices. > > > > > > > After thinking more, using a power state framework is seems too heavy > > for this use case. This use case is just do some clock and gpio > > operations before device is created, and do some put operations > > after device is deleted. We just need some helpers in one structure > > (called "power sequence" or "power state") for this purpose. > > > > For the use case, the clock and gpio operation can be done after device > > is created, the power domain is more suitable. > > There is a problem with PM domains that they only provide hooks for runtime PM > and system suspend/resume (including hibernation) and not for generic > "power up" and "power down" operations that may need to be carried out at > probe time before the runtime PM framework can be used (and analogously > at remove time). > > I would consider starting with the patch below or similar. > > Then you can define something like POWER_STATE_SEQUENCE type for your > case and basically use almost what you have already with it, except that > struct pwrsec_generic will now become struct power_state_sequence and > struct power_state_info will be embedded in it instead of struct pwrsec. > > The major comceptual difference is that ->power_up and ->power_down are > now available at the level of the device that needs the power sequence and > pm_device_power_up/down() can be used wherever necessary (in the code, > in a bus type, in a controller driver or even in the driver for this particular > device). Rafeal, thanks for your patch. The biggest problem for my use case is the device is still not created. How can I call pm_device_power_up(dev)? Peter > > Most likely you will need a PM domain in addition to that, but mostly to avoid > code duplication. > > And in the future other types of power state definitions may be hooked up > to that, including ACPI etc. > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/base/power/common.c | 35 +++++++++++++++++++++++++++++++++++ > include/linux/pm.h | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 68 insertions(+) > > Index: linux-pm/include/linux/pm.h > =================================================================== > --- linux-pm.orig/include/linux/pm.h > +++ linux-pm/include/linux/pm.h > @@ -550,6 +550,30 @@ struct pm_subsys_data { > #endif > }; > > +enum power_state_type { > + POWER_STATE_GENERIC = 0, > +}; > + > +/** > + * struct power_state_info - information related to device power states. > + * > + * @type: Power states definition type. > + * @power_up: Device power up method. > + * @power_down: Device power down method. > + * > + * @power_up is expected to put the device into a power state in which it can > + * be operated by software (it doesn't have to be the full power state in > + * principle as long as the device will respond to all software accesses in > + * this state) and @power_down is expected to put the device into the lowest > + * power state the device can be put into given all of the applicable > + * constraints and limitations (it may not mean completely off). > + */ > +struct power_state_info { > + enum power_state_type type; > + int (*power_up)(struct device *dev); > + int (*power_down)(struct device *dev); > +}; > + > struct dev_pm_info { > pm_message_t power_state; > unsigned int can_wakeup:1; > @@ -600,6 +624,7 @@ struct dev_pm_info { > unsigned long active_jiffies; > unsigned long suspended_jiffies; > unsigned long accounting_timestamp; > + struct power_state_info *state; > #endif > struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ > void (*set_latency_tolerance)(struct device *, s32); > @@ -610,6 +635,14 @@ extern void update_pm_runtime_accounting > extern int dev_pm_get_subsys_data(struct device *dev); > extern void dev_pm_put_subsys_data(struct device *dev); > > +#ifdef CONFIG_PM > +extern int pm_device_power_up(struct device *dev); > +extern int pm_device_power_down(struct device *dev); > +#else > +static inline int pm_device_power_up(struct device *dev) { return 0; } > +static inline int pm_device_power_down(struct device *dev) { return 0; } > +#endif /* CONFIG_PM */ > + > /** > * struct dev_pm_domain - power management domain representation. > * > Index: linux-pm/drivers/base/power/common.c > =================================================================== > --- linux-pm.orig/drivers/base/power/common.c > +++ linux-pm/drivers/base/power/common.c > @@ -152,3 +152,38 @@ void dev_pm_domain_set(struct device *de > device_pm_check_callbacks(dev); > } > EXPORT_SYMBOL_GPL(dev_pm_domain_set); > + > +/** > + * pm_device_power_up - Power up a device using the power states framework. > + * @dev: Device to power up. > + * > + * Put the device into a power state in which it will respond to all software > + * accesses (that may not mean maximum power) using the callback provided > + * through the device power state framework, if present. > + */ > +int pm_device_power_up(struct device *dev) > +{ > + if (dev->power.state && dev->power.state->power_up) > + return dev->power.state->power_up(dev); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(pm_device_power_up); > + > +/** > + * pm_device_power_down - Power down a device using the power states framework. > + * @dev: Device to power down. > + * > + * Put the device into the lowest power state it can be put into given the > + * applicable constraints and limitations (that may not mean maximum power) > + * using the callback provided through the device power state framework, if > + * present. > + */ > +int pm_device_power_down(struct device *dev) > +{ > + if (dev->power.state && dev->power.state->power_down) > + return dev->power.state->power_down(dev); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(pm_device_power_down); >
On Saturday, July 08, 2017 01:51:15 PM Peter Chen wrote: > On Fri, Jul 07, 2017 at 03:03:06PM +0200, Rafael J. Wysocki wrote: > > On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote: > > > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote: > > > > > > > > > > - Can I write new code for it or I need to depend on something? > > > > > > > > There is nothing this code needs to depend on AFAICS, but there are existing > > > > solutions in this problem space (ACPI power management, genpd), so it needs to > > > > be careful enough about possible overlaps etc. > > > > > > > > > I find there is already "power state" concept at documentation. > > > > > Documentation/ABI/testing/sysfs-devices-power_state > > > > > > > > This is ACPI-specific and only in sysfs directories representing ACPI device > > > > objects (which aren't physical devices). > > > > > > > > Anyway, since ACPI covers the problem space you are working in already, > > > > your code has to be mutually exclusive with it. > > > > > > > > > - If I can write the new code for it, except the problems I want > > > > > to fix, are there any other use cases I need to consider? > > > > > > > > I would start simple and focus on the particular problem at hand, that is > > > > devices with two power states ("on" and "off") where the "on" state > > > > depends on a number of clocks and/or GPIOs. Still, I'd also avoid making > > > > design choices that might prevent it from being extended in the future > > > > if need be. > > > > > > > > One major problem I can see is how to "attach" the power states framework > > > > to a particular device (once we have discovered that it should be used with > > > > that device). > > > > > > > > For bus types that don't do power management of their own you could follow > > > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types > > > > doing their own PM (like USB) will probably need to be treated differently. > > > > In those cases the bus type code will have to know that it should call some > > > > helpers to switch power states of devices. > > > > > > > > > > After thinking more, using a power state framework is seems too heavy > > > for this use case. This use case is just do some clock and gpio > > > operations before device is created, and do some put operations > > > after device is deleted. We just need some helpers in one structure > > > (called "power sequence" or "power state") for this purpose. > > > > > > For the use case, the clock and gpio operation can be done after device > > > is created, the power domain is more suitable. > > > > There is a problem with PM domains that they only provide hooks for runtime PM > > and system suspend/resume (including hibernation) and not for generic > > "power up" and "power down" operations that may need to be carried out at > > probe time before the runtime PM framework can be used (and analogously > > at remove time). > > > > I would consider starting with the patch below or similar. > > > > Then you can define something like POWER_STATE_SEQUENCE type for your > > case and basically use almost what you have already with it, except that > > struct pwrsec_generic will now become struct power_state_sequence and > > struct power_state_info will be embedded in it instead of struct pwrsec. > > > > The major comceptual difference is that ->power_up and ->power_down are > > now available at the level of the device that needs the power sequence and > > pm_device_power_up/down() can be used wherever necessary (in the code, > > in a bus type, in a controller driver or even in the driver for this particular > > device). > > Rafeal, thanks for your patch. > > The biggest problem for my use case is the device is still not created. > How can I call pm_device_power_up(dev)? Can you please elaborate on that a bit? You surely need a device object before probing the device and why would the device be accessed before that point? I guess you have a bus with devices that are discoverable in principle, but they cannot be discovered before being powered up, so you need the information on which devices to power up in a DT, right? Thanks, Rafael
On Sat, Jul 08, 2017 at 02:14:56PM +0200, Rafael J. Wysocki wrote: > On Saturday, July 08, 2017 01:51:15 PM Peter Chen wrote: > > On Fri, Jul 07, 2017 at 03:03:06PM +0200, Rafael J. Wysocki wrote: > > > On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote: > > > > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote: > > > > > > > > > > > > - Can I write new code for it or I need to depend on something? > > > > > > > > > > There is nothing this code needs to depend on AFAICS, but there are existing > > > > > solutions in this problem space (ACPI power management, genpd), so it needs to > > > > > be careful enough about possible overlaps etc. > > > > > > > > > > > I find there is already "power state" concept at documentation. > > > > > > Documentation/ABI/testing/sysfs-devices-power_state > > > > > > > > > > This is ACPI-specific and only in sysfs directories representing ACPI device > > > > > objects (which aren't physical devices). > > > > > > > > > > Anyway, since ACPI covers the problem space you are working in already, > > > > > your code has to be mutually exclusive with it. > > > > > > > > > > > - If I can write the new code for it, except the problems I want > > > > > > to fix, are there any other use cases I need to consider? > > > > > > > > > > I would start simple and focus on the particular problem at hand, that is > > > > > devices with two power states ("on" and "off") where the "on" state > > > > > depends on a number of clocks and/or GPIOs. Still, I'd also avoid making > > > > > design choices that might prevent it from being extended in the future > > > > > if need be. > > > > > > > > > > One major problem I can see is how to "attach" the power states framework > > > > > to a particular device (once we have discovered that it should be used with > > > > > that device). > > > > > > > > > > For bus types that don't do power management of their own you could follow > > > > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types > > > > > doing their own PM (like USB) will probably need to be treated differently. > > > > > In those cases the bus type code will have to know that it should call some > > > > > helpers to switch power states of devices. > > > > > > > > > > > > > After thinking more, using a power state framework is seems too heavy > > > > for this use case. This use case is just do some clock and gpio > > > > operations before device is created, and do some put operations > > > > after device is deleted. We just need some helpers in one structure > > > > (called "power sequence" or "power state") for this purpose. > > > > > > > > For the use case, the clock and gpio operation can be done after device > > > > is created, the power domain is more suitable. > > > > > > There is a problem with PM domains that they only provide hooks for runtime PM > > > and system suspend/resume (including hibernation) and not for generic > > > "power up" and "power down" operations that may need to be carried out at > > > probe time before the runtime PM framework can be used (and analogously > > > at remove time). > > > > > > I would consider starting with the patch below or similar. > > > > > > Then you can define something like POWER_STATE_SEQUENCE type for your > > > case and basically use almost what you have already with it, except that > > > struct pwrsec_generic will now become struct power_state_sequence and > > > struct power_state_info will be embedded in it instead of struct pwrsec. > > > > > > The major comceptual difference is that ->power_up and ->power_down are > > > now available at the level of the device that needs the power sequence and > > > pm_device_power_up/down() can be used wherever necessary (in the code, > > > in a bus type, in a controller driver or even in the driver for this particular > > > device). > > > > Rafeal, thanks for your patch. > > > > The biggest problem for my use case is the device is still not created. > > How can I call pm_device_power_up(dev)? > > Can you please elaborate on that a bit? > Sorry, I should describe more. Let's take USB bus as an example, when the new USB device is at the host port, the device structure at device model is not created until it is discoverable by the USB bus. If this new USB device needs to be powered on before can be discoverable by the bus, the device structure will be not created without powering on operation. The code usb_alloc_dev (drivers/usb/core/usb.c) is only called for discoverable device. Unlike the other bus, eg, platform bus, it creates device structure according to DT node. The USB bus was designed for hot plug model, the device structure is for discoverable device. In recent years, we begin to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G Modem, etc at the market. It needs some board level power operation before it can be found by the USB bus. This patch set is designed primarily for fix this kind of problem. You will see at at pwrseq_generic.c, we use DT version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags instead of device structure version, like devm_clk_get and devm_gpiod_get_optional. MMC system has similar use case, it creates power sequence platform device for this issue, but all those power stuffs (clock, gpio, etc) may not be suitable as a dedicated virtual device at DT, they are belonged to one physical device, so this patch set is created to see if this issue can be fixed better. > You surely need a device object before probing the device and why would the > device be accessed before that point? See above. > > I guess you have a bus with devices that are discoverable in principle, but > they cannot be discovered before being powered up, Yes. > so you need the information > on which devices to power up in a DT, right? > The bus will power up all device nodes in this bus according to DT information, the device structure has not created at this time.
On Monday, July 10, 2017 10:28:15 AM Peter Chen wrote: > On Sat, Jul 08, 2017 at 02:14:56PM +0200, Rafael J. Wysocki wrote: > > On Saturday, July 08, 2017 01:51:15 PM Peter Chen wrote: > > > On Fri, Jul 07, 2017 at 03:03:06PM +0200, Rafael J. Wysocki wrote: > > > > On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote: > > > > > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote: > > > > > > > > > > > > > > - Can I write new code for it or I need to depend on something? > > > > > > > > > > > > There is nothing this code needs to depend on AFAICS, but there are existing > > > > > > solutions in this problem space (ACPI power management, genpd), so it needs to > > > > > > be careful enough about possible overlaps etc. > > > > > > > > > > > > > I find there is already "power state" concept at documentation. > > > > > > > Documentation/ABI/testing/sysfs-devices-power_state > > > > > > > > > > > > This is ACPI-specific and only in sysfs directories representing ACPI device > > > > > > objects (which aren't physical devices). > > > > > > > > > > > > Anyway, since ACPI covers the problem space you are working in already, > > > > > > your code has to be mutually exclusive with it. > > > > > > > > > > > > > - If I can write the new code for it, except the problems I want > > > > > > > to fix, are there any other use cases I need to consider? > > > > > > > > > > > > I would start simple and focus on the particular problem at hand, that is > > > > > > devices with two power states ("on" and "off") where the "on" state > > > > > > depends on a number of clocks and/or GPIOs. Still, I'd also avoid making > > > > > > design choices that might prevent it from being extended in the future > > > > > > if need be. > > > > > > > > > > > > One major problem I can see is how to "attach" the power states framework > > > > > > to a particular device (once we have discovered that it should be used with > > > > > > that device). > > > > > > > > > > > > For bus types that don't do power management of their own you could follow > > > > > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types > > > > > > doing their own PM (like USB) will probably need to be treated differently. > > > > > > In those cases the bus type code will have to know that it should call some > > > > > > helpers to switch power states of devices. > > > > > > > > > > > > > > > > After thinking more, using a power state framework is seems too heavy > > > > > for this use case. This use case is just do some clock and gpio > > > > > operations before device is created, and do some put operations > > > > > after device is deleted. We just need some helpers in one structure > > > > > (called "power sequence" or "power state") for this purpose. > > > > > > > > > > For the use case, the clock and gpio operation can be done after device > > > > > is created, the power domain is more suitable. > > > > > > > > There is a problem with PM domains that they only provide hooks for runtime PM > > > > and system suspend/resume (including hibernation) and not for generic > > > > "power up" and "power down" operations that may need to be carried out at > > > > probe time before the runtime PM framework can be used (and analogously > > > > at remove time). > > > > > > > > I would consider starting with the patch below or similar. > > > > > > > > Then you can define something like POWER_STATE_SEQUENCE type for your > > > > case and basically use almost what you have already with it, except that > > > > struct pwrsec_generic will now become struct power_state_sequence and > > > > struct power_state_info will be embedded in it instead of struct pwrsec. > > > > > > > > The major comceptual difference is that ->power_up and ->power_down are > > > > now available at the level of the device that needs the power sequence and > > > > pm_device_power_up/down() can be used wherever necessary (in the code, > > > > in a bus type, in a controller driver or even in the driver for this particular > > > > device). > > > > > > Rafeal, thanks for your patch. > > > > > > The biggest problem for my use case is the device is still not created. > > > How can I call pm_device_power_up(dev)? > > > > Can you please elaborate on that a bit? > > > > Sorry, I should describe more. > > Let's take USB bus as an example, when the new USB device is at the > host port, the device structure at device model is not created until > it is discoverable by the USB bus. If this new USB device needs to be > powered on before can be discoverable by the bus, the device structure > will be not created without powering on operation. The code usb_alloc_dev > (drivers/usb/core/usb.c) is only called for discoverable device. > > Unlike the other bus, eg, platform bus, it creates device structure > according to DT node. The USB bus was designed for hot plug model, the > device structure is for discoverable device. In recent years, we begin > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G > Modem, etc at the market. It needs some board level power operation before > it can be found by the USB bus. This patch set is designed primarily for > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags > instead of device structure version, like devm_clk_get and > devm_gpiod_get_optional. > > MMC system has similar use case, it creates power sequence platform > device for this issue, but all those power stuffs (clock, gpio, etc) > may not be suitable as a dedicated virtual device at DT, they are belonged > to one physical device, so this patch set is created to see if this issue > can be fixed better. OK, thanks for the explanation. The above needs to be part of your problem statement. > > You surely need a device object before probing the device and why would the > > device be accessed before that point? > > See above. > > > > > I guess you have a bus with devices that are discoverable in principle, but > > they cannot be discovered before being powered up, > > Yes. I was missing that point before, sorry about that. I can only say that this wasn't particularly clear from you patch changelogs etc. > > so you need the information > > on which devices to power up in a DT, right? > > > > The bus will power up all device nodes in this bus according to DT > information, the device structure has not created at this time. OK I still think that the information on power resources depended on by devices should be used for power management as well as for the initial power-up. The most straightforward way to arrange for that would be to make it possible to find the DT node matching the device after the device has been discovered and struct device created for it, say by USB. That would require adding some more information on the device to the DT node, probably. Then, the DT device nodes would be used for the initial power-up and next, after discovering a device, you'd do a lookup in the DT, find the node matching it and read the power resuources information from there to populate the device's power state structure. From that point on you can simply use the interface I suggested. Thanks, Rafael
On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote: > > Sorry, I should describe more. > > > > Let's take USB bus as an example, when the new USB device is at the > > host port, the device structure at device model is not created until > > it is discoverable by the USB bus. If this new USB device needs to be > > powered on before can be discoverable by the bus, the device structure > > will be not created without powering on operation. The code usb_alloc_dev > > (drivers/usb/core/usb.c) is only called for discoverable device. > > > > Unlike the other bus, eg, platform bus, it creates device structure > > according to DT node. The USB bus was designed for hot plug model, the > > device structure is for discoverable device. In recent years, we begin > > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G > > Modem, etc at the market. It needs some board level power operation before > > it can be found by the USB bus. This patch set is designed primarily for > > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT > > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags > > instead of device structure version, like devm_clk_get and > > devm_gpiod_get_optional. > > > > MMC system has similar use case, it creates power sequence platform > > device for this issue, but all those power stuffs (clock, gpio, etc) > > may not be suitable as a dedicated virtual device at DT, they are belonged > > to one physical device, so this patch set is created to see if this issue > > can be fixed better. > > OK, thanks for the explanation. > > The above needs to be part of your problem statement. Ok, I will add it to cover letter. > > > > The bus will power up all device nodes in this bus according to DT > > information, the device structure has not created at this time. > > OK > > I still think that the information on power resources depended on by devices > should be used for power management as well as for the initial power-up. > > The most straightforward way to arrange for that would be to make it possible > to find the DT node matching the device after the device has been discovered > and struct device created for it, say by USB. That would require adding some > more information on the device to the DT node, probably. After the device is created, the device node structure is under struct device, say dev->of_node. The most difficulty for this issue is the device creation is dynamic and is after the physical device is discovered by the bus, the initial power-up is needed before the device can be discovered by the bus. > > Then, the DT device nodes would be used for the initial power-up and next, after > discovering a device, you'd do a lookup in the DT, find the node matching it > and read the power resuources information from there to populate the device's > power state structure. From that point on you can simply use the interface I > suggested. > Just like I said above, without initial power-up, the device can't be discovered by the bus.
On Tue, Jul 18, 2017 at 6:29 AM, Peter Chen <hzpeterchen@gmail.com> wrote: > On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote: >> > Sorry, I should describe more. >> > >> > Let's take USB bus as an example, when the new USB device is at the >> > host port, the device structure at device model is not created until >> > it is discoverable by the USB bus. If this new USB device needs to be >> > powered on before can be discoverable by the bus, the device structure >> > will be not created without powering on operation. The code usb_alloc_dev >> > (drivers/usb/core/usb.c) is only called for discoverable device. >> > >> > Unlike the other bus, eg, platform bus, it creates device structure >> > according to DT node. The USB bus was designed for hot plug model, the >> > device structure is for discoverable device. In recent years, we begin >> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G >> > Modem, etc at the market. It needs some board level power operation before >> > it can be found by the USB bus. This patch set is designed primarily for >> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT >> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags >> > instead of device structure version, like devm_clk_get and >> > devm_gpiod_get_optional. >> > >> > MMC system has similar use case, it creates power sequence platform >> > device for this issue, but all those power stuffs (clock, gpio, etc) >> > may not be suitable as a dedicated virtual device at DT, they are belonged >> > to one physical device, so this patch set is created to see if this issue >> > can be fixed better. >> >> OK, thanks for the explanation. >> >> The above needs to be part of your problem statement. > > Ok, I will add it to cover letter. > >> > >> > The bus will power up all device nodes in this bus according to DT >> > information, the device structure has not created at this time. >> >> OK >> >> I still think that the information on power resources depended on by devices >> should be used for power management as well as for the initial power-up. >> >> The most straightforward way to arrange for that would be to make it possible >> to find the DT node matching the device after the device has been discovered >> and struct device created for it, say by USB. That would require adding some >> more information on the device to the DT node, probably. > > After the device is created, the device node structure is under struct > device, say dev->of_node. The most difficulty for this issue is the > device creation is dynamic and is after the physical device is > discovered by the bus, the initial power-up is needed before the device > can be discovered by the bus. So you power up all devices on the bus using the information from of_nodes upfront. Then you scan the bus and discover devices. For each of them, once it has been discovered, you look up a matching of_node and initialize power_state from there. >> >> Then, the DT device nodes would be used for the initial power-up and next, after >> discovering a device, you'd do a lookup in the DT, find the node matching it >> and read the power resuources information from there to populate the device's >> power state structure. From that point on you can simply use the interface I >> suggested. >> > > Just like I said above, without initial power-up, the device can't be > discovered by the bus. Right. So you do the power up upfront, as per the above. Thanks, Rafael
On Tue, Jul 18, 2017 at 07:06:05PM +0200, Rafael J. Wysocki wrote: > On Tue, Jul 18, 2017 at 6:29 AM, Peter Chen <hzpeterchen@gmail.com> wrote: > > On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote: > >> > Sorry, I should describe more. > >> > > >> > Let's take USB bus as an example, when the new USB device is at the > >> > host port, the device structure at device model is not created until > >> > it is discoverable by the USB bus. If this new USB device needs to be > >> > powered on before can be discoverable by the bus, the device structure > >> > will be not created without powering on operation. The code usb_alloc_dev > >> > (drivers/usb/core/usb.c) is only called for discoverable device. > >> > > >> > Unlike the other bus, eg, platform bus, it creates device structure > >> > according to DT node. The USB bus was designed for hot plug model, the > >> > device structure is for discoverable device. In recent years, we begin > >> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G > >> > Modem, etc at the market. It needs some board level power operation before > >> > it can be found by the USB bus. This patch set is designed primarily for > >> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT > >> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags > >> > instead of device structure version, like devm_clk_get and > >> > devm_gpiod_get_optional. > >> > > >> > MMC system has similar use case, it creates power sequence platform > >> > device for this issue, but all those power stuffs (clock, gpio, etc) > >> > may not be suitable as a dedicated virtual device at DT, they are belonged > >> > to one physical device, so this patch set is created to see if this issue > >> > can be fixed better. > >> > >> OK, thanks for the explanation. > >> > >> The above needs to be part of your problem statement. > > > > Ok, I will add it to cover letter. > > > >> > > >> > The bus will power up all device nodes in this bus according to DT > >> > information, the device structure has not created at this time. > >> > >> OK > >> > >> I still think that the information on power resources depended on by devices > >> should be used for power management as well as for the initial power-up. > >> > >> The most straightforward way to arrange for that would be to make it possible > >> to find the DT node matching the device after the device has been discovered > >> and struct device created for it, say by USB. That would require adding some > >> more information on the device to the DT node, probably. > > > > After the device is created, the device node structure is under struct > > device, say dev->of_node. The most difficulty for this issue is the > > device creation is dynamic and is after the physical device is > > discovered by the bus, the initial power-up is needed before the device > > can be discovered by the bus. > > So you power up all devices on the bus using the information from > of_nodes upfront. > I think your mean call pm_device_power_up(struct device *dev) for bus level device (eg, USB HUB for USB), yeah, I can do that. But we still have below problem: Where we can put the kinds of power sequence implementation (eg, pwrseq_generic.c) and the match mechanism between device node and kinds of power sequence(eg, of_pwrseq_on at core.c)? These stuffs can be shared among subsystems, and is better to use one framework for it. MMC subsystem adds such things at its core folder (drivers/mmc/core/pwrseq*) currently.
On Wednesday, July 19, 2017 10:56:00 AM Peter Chen wrote: > On Tue, Jul 18, 2017 at 07:06:05PM +0200, Rafael J. Wysocki wrote: > > On Tue, Jul 18, 2017 at 6:29 AM, Peter Chen <hzpeterchen@gmail.com> wrote: > > > On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote: > > >> > Sorry, I should describe more. > > >> > > > >> > Let's take USB bus as an example, when the new USB device is at the > > >> > host port, the device structure at device model is not created until > > >> > it is discoverable by the USB bus. If this new USB device needs to be > > >> > powered on before can be discoverable by the bus, the device structure > > >> > will be not created without powering on operation. The code usb_alloc_dev > > >> > (drivers/usb/core/usb.c) is only called for discoverable device. > > >> > > > >> > Unlike the other bus, eg, platform bus, it creates device structure > > >> > according to DT node. The USB bus was designed for hot plug model, the > > >> > device structure is for discoverable device. In recent years, we begin > > >> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G > > >> > Modem, etc at the market. It needs some board level power operation before > > >> > it can be found by the USB bus. This patch set is designed primarily for > > >> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT > > >> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags > > >> > instead of device structure version, like devm_clk_get and > > >> > devm_gpiod_get_optional. > > >> > > > >> > MMC system has similar use case, it creates power sequence platform > > >> > device for this issue, but all those power stuffs (clock, gpio, etc) > > >> > may not be suitable as a dedicated virtual device at DT, they are belonged > > >> > to one physical device, so this patch set is created to see if this issue > > >> > can be fixed better. > > >> > > >> OK, thanks for the explanation. > > >> > > >> The above needs to be part of your problem statement. > > > > > > Ok, I will add it to cover letter. > > > > > >> > > > >> > The bus will power up all device nodes in this bus according to DT > > >> > information, the device structure has not created at this time. > > >> > > >> OK > > >> > > >> I still think that the information on power resources depended on by devices > > >> should be used for power management as well as for the initial power-up. > > >> > > >> The most straightforward way to arrange for that would be to make it possible > > >> to find the DT node matching the device after the device has been discovered > > >> and struct device created for it, say by USB. That would require adding some > > >> more information on the device to the DT node, probably. > > > > > > After the device is created, the device node structure is under struct > > > device, say dev->of_node. The most difficulty for this issue is the > > > device creation is dynamic and is after the physical device is > > > discovered by the bus, the initial power-up is needed before the device > > > can be discovered by the bus. > > > > So you power up all devices on the bus using the information from > > of_nodes upfront. > > > > I think your mean call pm_device_power_up(struct device *dev) for bus > level device (eg, USB HUB for USB), yeah, I can do that. But we still > have below problem: > > Where we can put the kinds of power sequence implementation > (eg, pwrseq_generic.c) and the match mechanism between device > node and kinds of power sequence(eg, of_pwrseq_on at core.c)? These > stuffs can be shared among subsystems, and is better to use > one framework for it. MMC subsystem adds such things at its core > folder (drivers/mmc/core/pwrseq*) currently. The power sequence handling code can be generic IMO, because it doesn't depend on what bus the devices are on. It just needs to provide some services to its users, but you need a clean interface between it and its users, of course. There basically are two categories of devices, let's refer to them as USB-type and MMC-type. The difference between them, as far as power sequences go, boils down to initialization and discovery. For the USB-type ones you need an upfront power-up pass over of_nodes under the bus controller. That needs to be initiated by the controller driver, but may be carried out by the common code. After that, you need to match of_nodes to devices while discovering them. That also needs to be initiated by the bus controller, but there may be a helper function in the common code for that. It may take a struct device for the just discovered device and the bus controller's of_node as arguments, look for an of_node under the bus controller matching the device and then attach the power sequence information to the struct device as appropriate. After the power sequence information has been attached to the struct device, it can just be used for controlling its power state in a generic fashion. In the MMC-type devices case the discovery if through of_node lookup IIUC, so then you may power up things as you discover them and attach the power sequence information to their struct device objects right away. Again, once you have done that, the power sequence information is there and it can be used for controlling the power states of devices generically. Thanks, Rafael
On Wed, Jul 19, 2017 at 01:34:11PM +0200, Rafael J. Wysocki wrote: > On Wednesday, July 19, 2017 10:56:00 AM Peter Chen wrote: > > On Tue, Jul 18, 2017 at 07:06:05PM +0200, Rafael J. Wysocki wrote: > > > On Tue, Jul 18, 2017 at 6:29 AM, Peter Chen <hzpeterchen@gmail.com> wrote: > > > > On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote: > > > >> > Sorry, I should describe more. > > > >> > > > > >> > Let's take USB bus as an example, when the new USB device is at the > > > >> > host port, the device structure at device model is not created until > > > >> > it is discoverable by the USB bus. If this new USB device needs to be > > > >> > powered on before can be discoverable by the bus, the device structure > > > >> > will be not created without powering on operation. The code usb_alloc_dev > > > >> > (drivers/usb/core/usb.c) is only called for discoverable device. > > > >> > > > > >> > Unlike the other bus, eg, platform bus, it creates device structure > > > >> > according to DT node. The USB bus was designed for hot plug model, the > > > >> > device structure is for discoverable device. In recent years, we begin > > > >> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G > > > >> > Modem, etc at the market. It needs some board level power operation before > > > >> > it can be found by the USB bus. This patch set is designed primarily for > > > >> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT > > > >> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags > > > >> > instead of device structure version, like devm_clk_get and > > > >> > devm_gpiod_get_optional. > > > >> > > > > >> > MMC system has similar use case, it creates power sequence platform > > > >> > device for this issue, but all those power stuffs (clock, gpio, etc) > > > >> > may not be suitable as a dedicated virtual device at DT, they are belonged > > > >> > to one physical device, so this patch set is created to see if this issue > > > >> > can be fixed better. > > > >> > > > >> OK, thanks for the explanation. > > > >> > > > >> The above needs to be part of your problem statement. > > > > > > > > Ok, I will add it to cover letter. > > > > > > > >> > > > > >> > The bus will power up all device nodes in this bus according to DT > > > >> > information, the device structure has not created at this time. > > > >> > > > >> OK > > > >> > > > >> I still think that the information on power resources depended on by devices > > > >> should be used for power management as well as for the initial power-up. > > > >> > > > >> The most straightforward way to arrange for that would be to make it possible > > > >> to find the DT node matching the device after the device has been discovered > > > >> and struct device created for it, say by USB. That would require adding some > > > >> more information on the device to the DT node, probably. > > > > > > > > After the device is created, the device node structure is under struct > > > > device, say dev->of_node. The most difficulty for this issue is the > > > > device creation is dynamic and is after the physical device is > > > > discovered by the bus, the initial power-up is needed before the device > > > > can be discovered by the bus. > > > > > > So you power up all devices on the bus using the information from > > > of_nodes upfront. > > > > > > > I think your mean call pm_device_power_up(struct device *dev) for bus > > level device (eg, USB HUB for USB), yeah, I can do that. But we still > > have below problem: > > > > Where we can put the kinds of power sequence implementation > > (eg, pwrseq_generic.c) and the match mechanism between device > > node and kinds of power sequence(eg, of_pwrseq_on at core.c)? These > > stuffs can be shared among subsystems, and is better to use > > one framework for it. MMC subsystem adds such things at its core > > folder (drivers/mmc/core/pwrseq*) currently. > > The power sequence handling code can be generic IMO, because it doesn't > depend on what bus the devices are on. It just needs to provide some > services to its users, but you need a clean interface between it and its > users, of course. > > There basically are two categories of devices, let's refer to them as USB-type > and MMC-type. The difference between them, as far as power sequences go, > boils down to initialization and discovery. > > For the USB-type ones you need an upfront power-up pass over of_nodes > under the bus controller. That needs to be initiated by the controller driver, > but may be carried out by the common code. The upfront power-up can't use pm_device_power_up API, since the power sequence at device node is for the devices under the controller device, it has to use the power sequence APIs directly, eg, of_pwrseq_on_list or of_pwrseq_on. > > After that, you need to match of_nodes to devices while discovering them. > That also needs to be initiated by the bus controller, but there may be > a helper function in the common code for that. It may take a struct device > for the just discovered device and the bus controller's of_node as arguments, > look for an of_node under the bus controller matching the device and then > attach the power sequence information to the struct device as appropriate. > Yes, we can have a helper at pwrseq core, eg dev_add_pwrseq(dev->of_node) to do that, and call it at device_pm_init. At dev_add_pwrseq, we search the global pwrseq list, and do match according to the address of of_node, if matches, return the pointer of struct pwrseq. > After the power sequence information has been attached to the struct > device, it can just be used for controlling its power state in a generic > fashion. At least for USB use case, there is a problem here for this patch set, it doesn't need to control power state (on/off) during its struct device life-cycle, after the struct device is de-inintialized (call put_device(dev)), its (initial) power state is still on, its power state will be off when its parent (USB HUB or controller device) is going to be deleted. So, it is hard for me to have the use case to call pm_device_power_up and pm_device_power_off, the only thing I can do at my patch set is to set dev.state->type = POWER_STATE_ON when tries to add struct pwrseq to struct device. > > In the MMC-type devices case the discovery if through of_node lookup IIUC, > so then you may power up things as you discover them and attach the > power sequence information to their struct device objects right away. > > Again, once you have done that, the power sequence information is > there and it can be used for controlling the power states of devices > generically. Like I mentioned above, I can implement these helpers for that, but I only can update power state information for device, but no place to call and define its dev.state->power_on and dev.state->power_off.
Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -550,6 +550,30 @@ struct pm_subsys_data { #endif }; +enum power_state_type { + POWER_STATE_GENERIC = 0, +}; + +/** + * struct power_state_info - information related to device power states. + * + * @type: Power states definition type. + * @power_up: Device power up method. + * @power_down: Device power down method. + * + * @power_up is expected to put the device into a power state in which it can + * be operated by software (it doesn't have to be the full power state in + * principle as long as the device will respond to all software accesses in + * this state) and @power_down is expected to put the device into the lowest + * power state the device can be put into given all of the applicable + * constraints and limitations (it may not mean completely off). + */ +struct power_state_info { + enum power_state_type type; + int (*power_up)(struct device *dev); + int (*power_down)(struct device *dev); +}; + struct dev_pm_info { pm_message_t power_state; unsigned int can_wakeup:1; @@ -600,6 +624,7 @@ struct dev_pm_info { unsigned long active_jiffies; unsigned long suspended_jiffies; unsigned long accounting_timestamp; + struct power_state_info *state; #endif struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ void (*set_latency_tolerance)(struct device *, s32); @@ -610,6 +635,14 @@ extern void update_pm_runtime_accounting extern int dev_pm_get_subsys_data(struct device *dev); extern void dev_pm_put_subsys_data(struct device *dev); +#ifdef CONFIG_PM +extern int pm_device_power_up(struct device *dev); +extern int pm_device_power_down(struct device *dev); +#else +static inline int pm_device_power_up(struct device *dev) { return 0; } +static inline int pm_device_power_down(struct device *dev) { return 0; } +#endif /* CONFIG_PM */ + /** * struct dev_pm_domain - power management domain representation. * Index: linux-pm/drivers/base/power/common.c =================================================================== --- linux-pm.orig/drivers/base/power/common.c +++ linux-pm/drivers/base/power/common.c @@ -152,3 +152,38 @@ void dev_pm_domain_set(struct device *de device_pm_check_callbacks(dev); } EXPORT_SYMBOL_GPL(dev_pm_domain_set); + +/** + * pm_device_power_up - Power up a device using the power states framework. + * @dev: Device to power up. + * + * Put the device into a power state in which it will respond to all software + * accesses (that may not mean maximum power) using the callback provided + * through the device power state framework, if present. + */ +int pm_device_power_up(struct device *dev) +{ + if (dev->power.state && dev->power.state->power_up) + return dev->power.state->power_up(dev); + + return 0; +} +EXPORT_SYMBOL_GPL(pm_device_power_up); + +/** + * pm_device_power_down - Power down a device using the power states framework. + * @dev: Device to power down. + * + * Put the device into the lowest power state it can be put into given the + * applicable constraints and limitations (that may not mean maximum power) + * using the callback provided through the device power state framework, if + * present. + */ +int pm_device_power_down(struct device *dev) +{ + if (dev->power.state && dev->power.state->power_down) + return dev->power.state->power_down(dev); + + return 0; +} +EXPORT_SYMBOL_GPL(pm_device_power_down);