Re: [PATCH v2] media: ov5640: Use runtime PM

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Aug 01, 2022 at 04:23:54PM +0900, Tomasz Figa wrote:
> [Fixed Jacopo's email address.]
> 
> On Mon, Aug 1, 2022 at 4:17 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > On Thu, Apr 14, 2022 at 6:30 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Laurent,
> > >
> > > On Tue, Mar 29, 2022 at 04:02:54PM +0300, Laurent Pinchart wrote:
> > > > Hi Sakari,
> > > >
> > > > On Mon, Mar 14, 2022 at 11:11:18PM +0200, Sakari Ailus wrote:
> > > > > On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
> > > > > ...
> > > > > > > > Yes, after reading the version register (or doing any other harware
> > > > > > > > access). Actually the full code would be
> > > > > > > >
> > > > > > > >
> > > > > > > >       pm_runtime_enable(dev);
> > > > > > > >       pm_runtime_resume_and_get(dev);
> > > > > > > >
> > > > > > > >       /* Hardware access */
> > > > > > > >
> > > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > > >       pm_runtime_put_autosuspend(dev);
> > > > > > > >
> > > > > > > > (plus error handling).
> > > > > > > >
> > > > > > > > If the probe function doesn't need to access the hardware, then
> > > > > > > > the above becomes
> > > > > > > >
> > > > > > > >       pm_runtime_enable(dev);
> > > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > > >
> > > > > > > > instead of having to power up the device just in case !PM.
> > > > > > > >
> > > > > > > > > Also the latter only works on DT-based systems so it's not an option for
> > > > > > > > > most of the drivers.
> > > >
> > > > Does the former work on ACPI systems ?
> > >
> > > Yes (i.e. the one that was above the quoted text).
> > >
> > > >
> > > > > > > > How so, what's wrong with the above for ACPI-based system ?
> > > > > > >
> > > > > > > I涎 devices are already powered on for probe on ACPI based systems.
> > > > > >
> > > > > > Not through RPM I suppose ?
> > > > >
> > > > > Runtime PM isn't involved, this takes place in the ACPI framework (via
> > > > > dev_pm_domain_attach() called in i2c_device_probe()).
> > > >
> > > > How can we fix this ? It may have made sense a long time ago, but it's
> > > > making RPM handling way too difficult in I2C drivers now. We need
> > > > something better instead of continuing to rely on cargo-cult for probe
> > > > functions. Most drivers are broken.
> > >
> > > Some could be broken, there's no question of that. A lot of drivers support
> > > either ACPI or DT, too, so not _that_ many need to work with both. Albeit
> > > that number is probably increasing constantly for the same devices are used
> > > on both.
> > >
> > > Then there are drivers that prefer not powering on the device in probe (see
> > > <URL:https://lore.kernel.org/linux-acpi/20210210230800.30291-2-sakari.ailus@linux.intel.com/T/>),
> > > it gets complicated to support all the combinatios of DT/ACPI (with or
> > > without the flag / property for waiving powering device on for probe) and
> > > CONFIG_PM enabled/disabled.
> > >
> > > What I think could be done to add a flag for drivers that handle power on
> > > their own, or perhaps rather change how I2C_DRV_ACPI_WAIVE_D0_PROBE flag
> > > works. Right now it expects a property on the device but that check could
> > > be moved to existing drivers using the flag. Not many drivers are currently
> > > using the flag. I think this would simplify driver implementation as both
> > > firmware interfaces would work the same way in this respect.
> > >
> > > You'd have to change one driver at a time, and people should be encouraged
> > > to write new drivers with that flag. Or add the flag to all existing
> > > drivers and not accept new ones with it.
> > >
> > > These devices I think are all I涎 but my understanding is that such
> > > differences exist elsewhere in the kernel, too. If they are to be
> > > addressed, it would probably be best to have a unified approach towards it.
> > >
> > > Added a few more people and lists to cc.
> >
> > + Hidenori from my team for visibility.
> >
> > I think we may want to take a step back and first define the problem
> > itself. To do that, let's take a look separately at DT and ACPI cases
> > (is platform data still relevant? are there any other firmware
> > interfaces that deal with I2C devices?).
> > For simplicity, let's forget about the ACPI waived power on in probe.
> >
> > DT:
> >  1) hardware state unknown when probe is called
> >  2) claim any independently managed resources (e.g. GPIOs)
> >  3) enable runtime PM
> >  4) if driver wants to access the hardware:
> >     a) runtime PM get
> >     b) enable any independently controlled resources (e.g. reset GPIO)

A small precision here, the resource handling is usually done in the
runtime PM resume/suspend handlers.

> >     c) [do access]
> >     d) disable any independently controlled resources
> >     e) runtime PM put
> >  5) after probe returns, regulators, clocks (and other similarly
> >     managed resources) would be force disabled if their enable count is 0
> >  6) hardware state is off (after the runtime PM state settles)
> >
> > ACPI:
> >  1) hardware state is active when probe is called
> >  2) [n/a]
> >  3) tell runtime PM framework that the state is active and then enable
> >     runtime PM
> >  4) if driver wants to access the hardware:
> >     a) runtime PM get
> >     b) [n/a]
> >     c) [do access]
> >     d) [n/a]
> >     e) runtime PM put
> >  5) [n/a]
> >  6) hardware state is off (after the runtime PM state settles)
> >
> > It seems like the relevant difference here is that for ACPI, the
> > driver needs to know that the initial state is active and also relay
> > this knowledge to the runtime PM subsystem. If we could make the ACPI
> > PM domain work the same way as regulators and clocks and eventually
> > power off some time later when the enable count is 0, then perhaps we
> > could avoid the problem in the first place?

Two additional questions if we're brainstorming this:

- Why is the I2C device hardware state active when probe is called, and
  would there be a way to change that (that is, beside the obvious issue
  that the transition could be painful, are there any other reasons to
  keep the status quo) ?

- If we have to keep this difference between the ACPI and DT models, how
  can we handle them in core code instead of drivers ? In particular,
  how could code core inform the RPM framework about the initial device
  state instead of leaving it to the driver ?

There's large set of RPM-related calls that have to be performed at
probe time in a very specific order, interleaved with manual power
handling. That is way over the threshold of what can be reasonably
expected from driver developers. I don't care much how it's done, but
this has to be dumbed down to make it dead simple in drivers.

-- 
Regards,

Laurent Pinchart