Hi, Roger Quadros <rog...@ti.com> writes: >>>>>>> I suggest merging this fix for 4.18-rc, and then Roger can rework the >>>>>>> driver so that it works also on OMAP. >>>>>> >>>>>> omap has its own glue layer for several reasons. If you're talking about >>>>>> Keystone devices, then okay, I understand. But in that case, this would >>>>>> mean Keystone is copying the same arguably broken PM domain design from >>>>>> OMAP and it would be best not to propagate that idea. >>>>> >>>>> Maybe so. I'm not sure what Roger's use case is, but perhaps the omap >>>>> glue driver could be used instead. >>>> >>>> unlikely. Keystone devices are very different from OMAP family. But >>>> we'll see what Roger says. >>>> >>> >>> Well, I was considering to use of-simple for the AM654 SoC [1] but now >>> I'm of the opinion that it might be better to add a new glue layer driver >> >> why isn't dwc3-keystone.c enough? > > It is doing too many things than we really need to for AM654.
I'm assuming you meant you really don't need for AM654. That can be made
optional with the use of a different compatible, right?
>>> for that because
>>> - it needs to poke a few registers in the wrapper region
>>
>> dwc3-keystone.c does that already
>>
>>> - it doesn't really need the driver to enable any clock
>>
>> Seems to me you're trying to port omap_device to arm64...
>
> It isn't an omap_device but is very similar to how it is done on
> keystone.
Fair enough
>>> - it needs a pm_runtime_get_sync() to be done in probe
>>
>> this really shouldn't be necessary. Keystone doesn't rely on all the
>> omap_device legacy. At least it didn't use to. Could it be that you're
>> just missing a struct dev_pm_domain definition for arm64?
>
> I don't think so. If I had missed it it wouldn't enable at all.
arm64 doesn't define own pm_domain.
>> I haven't seen how you guys implemented your PM for arm64 (is there a
>> publically accessible version somewhere?), but I'd say you should take
>> the opportunity to remove this relying on pm_runtime_get_sync() calls
>> from probe and just do what everybody else does; namely: enable clocks
>> on probe, pm_runtime_set_active, etc.
>
> This is something Tero can comment on.
>
>>
>> This helps drivers being able to make assumptions about devices being
>> enabled during probe. pm_runtime becomes easier to implement generically
>> too.
>>
>
> You keep mentioning that PM domain design is broken on OMAP. Could
> you please clarify what is broken? Is it the fact that the bus code
> doesn't enable the device before probe and that we have to do a
> pm_runtime_sync() in probe?
it's the fact that we need to rely on pm_runtime_get() to enable the
device from probe. Just consider for a second the situation this
creates:
probe() calls pm_runtime_enable() and pm_runtime_get*(). That will
eventually go back and call our own ->runtime_resume() callback. This
means that driver must make sure that *set_drvdata() is done before
pm_runtime_get() and driver must make sure that running
->runtime_resume() from probe() is okay in every case. What drivers end
up doing, is nonsense like below:
static int musb_runtime_resume(struct device *dev)
{
struct musb *musb = dev_to_musb(dev);
unsigned long flags;
int error;
/*
* When pm_runtime_get_sync called for the first time in driver
* init, some of the structure is still not initialized which is
* used in restore function. But clock needs to be
* enabled before any register access, so
* pm_runtime_get_sync has to be called.
* Also context restore without save does not make
* any sense
*/
if (!musb->is_initialized)
return 0;
[...]
}
This is a direct consequence of not paying attention to the order of
things. If driver were to assume that pm_domain->activate() would do the
right thing for the device -- meaning that probe would run with an
active device --, then we wouldn't need that pm_runtime_get() call on
probe at all. Rather we would follow the sequence:
pm_runtime_forbid()
pm_runtime_set_active()
pm_runtime_enable()
/* do your probe routine */
pm_runtime_put_noidle()
Then you remove you would need to call pm_runtime_get_noresume() to
balance out the pm_runtime_put_noidle() there.
Anyway, with an assumption like this, after all platform_devices are
converted over, the assumption can be moved into the bus code and, low
and behold, to enable runtime pm for your driver, all you have to is
implement your callbacks and add pm_runtime_put_noidle() to probe and
pm_runtime_get_noresume() to remove (apart from, of course, making sure
the device isn't allowed to runtime_suspend when it's actually busy).
Do you see the end goal?
Now, what omap_device did, even if accidentally, was create these
different approach which makes it more difficult to write a generic
driver that works for OMAPs as well as Exynos, PCI, Snapdragon,
Allwinner, Mediatek, etc.
(If you need to know why the pm_runtime_put_noidle(), remember that
pm_runtime_set_active() increments the usage counter, so
pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon
as userspace writes "auto" to /sys/..../power/control)
> I tried to discuss this here [1] but looks like you missed it.
>
> Re-iterating here.
>
> Platform bus doesn't seem to enable the device as part of
> of_platform_populate().
No, it doesn't. It has never been done; but how could it be?
platform_bus users aren't even ready to discuss methods to make sure
drivers can make assumptions about how PM should be implemented.
> see __device_attach() and driver_probe_device() in drivers/base/dd.c
>
> It does a pm_runtime_get_sync() on dev->parent but not on dev.
>
> Also, from section 5 of Documentation/power/runtime_pm.txt
>
> "In addition to that, the initial runtime PM status of all devices is
> 'suspended', but it need not reflect the actual physical state of the device.
> Thus, if the device is initially active (i.e. it is able to process I/O), its
> runtime PM status must be changed to 'active', with the help of
> pm_runtime_set_active(), before pm_runtime_enable() is called for the device."
>
> "Note, if the device may execute pm_runtime calls during the probe (such as
> if it is registers with a subsystem that may call back in) then the
> pm_runtime_get_sync() call paired with a pm_runtime_put() call will be
> appropriate to ensure that the device is not put back to sleep during the
> probe. This can happen with systems such as the network device layer."
I don't think that's saying what you think it's saying. That paragraph
is just telling you that pm_runtime doesn't know the state of the device
when probe is called, so it *always* defaults to suspended. If that's
not true, then you call pm_runtime_set_active().
Moreover, notice the little detail here:
"to ensure that the device is not put BACK to sleep during the probe"
It can only be put back to sleep if it was taken out of sleep before
hand. This is the same as calling pm_runtime_forbid() followed by
pm_runtime_set_active() followed by pm_runtime_enable(), here's
pm_runtime_forbid()'s implementation:
void pm_runtime_forbid(struct device *dev)
{
spin_lock_irq(&dev->power.lock);
if (!dev->power.runtime_auto)
goto out;
dev->power.runtime_auto = false;
atomic_inc(&dev->power.usage_count);
rpm_resume(dev, 0);
out:
spin_unlock_irq(&dev->power.lock);
}
> So looks like we can't assume that the device is "active" when probe()
> is called.
No, it's not a safe assumption for omap_device at least. I'm arguing
that it should be. There's no reason for you to not move
omap_device_enable() from _od_runtime_resume() to your pm_domain's
->activate() callback. Maybe not move, but copy it. Now you may need to
do something like below to make it work (clearly untested:
1 file changed, 14 insertions(+), 44 deletions(-)
arch/arm/mach-omap2/omap_device.c | 58 ++++++++++-----------------------------
modified arch/arm/mach-omap2/omap_device.c
@@ -594,46 +594,22 @@ static int _od_fail_runtime_resume(struct device *dev)
#endif
-#ifdef CONFIG_SUSPEND
-static int _od_suspend_noirq(struct device *dev)
+static void omap_device_activate(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct omap_device *od = to_omap_device(pdev);
- int ret;
-
- /* Don't attempt late suspend on a driver that is not bound */
- if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
- return 0;
-
- ret = pm_generic_suspend_noirq(dev);
-
- if (!ret && !pm_runtime_status_suspended(dev)) {
- if (pm_generic_runtime_suspend(dev) == 0) {
- omap_device_idle(pdev);
- od->flags |= OMAP_DEVICE_SUSPENDED;
- }
- }
- return ret;
+ od->flags &= ~OMAP_DEVICE_SUSPENDED;
+ omap_device_enable(pdev);
}
-static int _od_resume_noirq(struct device *dev)
+static void omap_device_activate(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- struct omap_device *od = to_omap_device(pdev);
- if (od->flags & OMAP_DEVICE_SUSPENDED) {
- od->flags &= ~OMAP_DEVICE_SUSPENDED;
- omap_device_enable(pdev);
- pm_generic_runtime_resume(dev);
- }
-
- return pm_generic_resume_noirq(dev);
+ omap_device_idle(pdev);
+ od->flags |= OMAP_DEVICE_SUSPENDED;
}
-#else
-#define _od_suspend_noirq NULL
-#define _od_resume_noirq NULL
-#endif
struct dev_pm_domain omap_device_fail_pm_domain = {
.ops = {
@@ -647,9 +623,9 @@ struct dev_pm_domain omap_device_pm_domain = {
SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
NULL)
USE_PLATFORM_PM_SLEEP_OPS
- SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq,
- _od_resume_noirq)
}
+ .activate = omap_device_activate,
+ .dismiss = omap_device_dismiss,
};
/**
@@ -690,12 +666,9 @@ int omap_device_enable(struct platform_device *pdev)
od = to_omap_device(pdev);
- if (od->_state == OMAP_DEVICE_STATE_ENABLED) {
- dev_warn(&pdev->dev,
- "omap_device: %s() called from invalid state %d\n",
- __func__, od->_state);
- return -EINVAL;
- }
+ /* already enabled, just bail out */
+ if (od->_state == OMAP_DEVICE_STATE_ENABLED)
+ return 0;
ret = _omap_device_enable_hwmods(od);
@@ -721,12 +694,9 @@ int omap_device_idle(struct platform_device *pdev)
od = to_omap_device(pdev);
- if (od->_state != OMAP_DEVICE_STATE_ENABLED) {
- dev_warn(&pdev->dev,
- "omap_device: %s() called from invalid state %d\n",
- __func__, od->_state);
- return -EINVAL;
- }
+ /* if not enabled, bail out */
+ if (od->_state != OMAP_DEVICE_STATE_ENABLED)
+ return 0;
ret = _omap_device_idle_hwmods(od);
> Which means, we need to do
>
> pm_runtime_get_sync();
> enable optional clocks;
shouldn't be necessary with changes above.
--
balbi
signature.asc
Description: PGP signature
