Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3927 bytes --]

Hi,

On Fri, Mar 28, 2014 at 02:20:24PM +0100, Geert Uytterhoeven wrote:
> Hi Felipe,
> 
> On Wed, Mar 12, 2014 at 4:37 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Friday 31 January 2014 12:12:45 Felipe Balbi wrote:
> >> Still TODO a commit log. Not for merging!!!!!
> >>
> >> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> >> ---
> >>
> >> This patch is an idea I've had recently in order to combine several
> >> different PM implementations into the platform-bus.
> >>
> >> This patch is bare minimum for platforms which need to handle functional and
> >> interface clocks but the whole thing is made optional.
> >>
> >> Note that this patch makes sure that by the time a platform_driver's probe
> >> is called, we already have clocks enabled and pm_runtime_set_active() has
> >> been called, thus making sure that a device driver's pm_runtime_get_sync()
> >> will solely increase the pm usage counter.
> >>
> >> I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause
> >> any issues since the clock API has ref counting too.
> >>
> >> Would really like to get some review from several folks involved with ARM
> >> SoC PM so that's the reason for the wide audience. If I have missed
> >> anybody, please add them to Cc.
> >>
> >> As mentioned above, this is *NOT* meant for merging, but serves as a
> >> starting point for discussing some convergence of several PM domain
> >> implementations on different arch/arm/mach-* directories.
> >>
> >> For OMAP, this has the added benefit of removing clock handling from
> >> omap_device/omap_hwmod and, thus, dropping the need for so many DT_CLK()
> >> tables under drivers/clk/ti/
> >>
> >>  drivers/base/platform.c         | 169 +++++++++++++++++++++++++++++++++++--
> >>  include/linux/platform_device.h |   9 +++
> >>  2 files changed, 173 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> >> index 3a94b79..86aeb5b 100644
> >> --- a/drivers/base/platform.c
> >> +++ b/drivers/base/platform.c
> >> @@ -484,6 +484,21 @@ static int platform_drv_probe(struct device *_dev)
> >>       if (ACPI_HANDLE(_dev))
> >>               acpi_dev_pm_attach(_dev, true);
> >>
> >> +     dev->fck = devm_clk_get(_dev, "fck");
> >> +     dev->ick = devm_clk_get(_dev, "ick");
> >
> > My concern with this that some devices might want to handle their functional
> > and interface clocks manually if they have exotic requirements. They would
> > thus need a way to signal that the platform bus core should not try to perform
> > clock management on its own.
> >
> > One possible way would be to name the clocks differently for those devices,
> > but we might then have a backward compatibility issue with DT.
> >
> > Another concern is how to handle the DT backward compatibility for devices
> > that would benefit from core management of their functional and interface
> > clocks, but that currently don't name those clocks "fck" and "ick". Renaming
> > those clocks in DT wouldn't be a problem for the future, but backward
> > compatibility needs to be handled. Maybe platforms could provide aliases in
> > that case.
> 
> IIUC, on OMAP the "fck" and "ick" clock names are also not present in DT.
> 
> Device nodes have "ti,hwmods" properties, while the board code uses
> struct omap_hwmod to match the "ti,hwmods" values with clock names, and
> creates fck aliases for them, right?

right, the idea was to move clock handling away from that, because that
has been creating a few issues for drivers the biggest of which is the
fact that we can't know, by probe() time, if the device is active or
suspended. Ideally, by probe() the device would be fully active with is
runtime pm usage counter incremented so that the first pm_runtime_get*
in our probe() doesn't try to call ->runtime_resume().

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]