Hi! Added Paul in Cc:. On Thu, Jun 14, 2012 at 10:05 AM, Jean Pihet <jean.pihet@xxxxxxxxxxxxxx> wrote: > Hi Richard, all, > > On Tue, Jun 12, 2012 at 6:34 PM, Woodruff, Richard <r-woodruff2@xxxxxx> wrote: >> Hi Tony, >> >>> From: Tony Lindgren [mailto:tony@xxxxxxxxxxx] >>> Sent: Friday, May 25, 2012 2:53 AM >> >> Thanks for quick input. Apologies on slow ack of it. >> >>> Before we had these frameworks in place it was often hard to debug when >>> something >>> did not work for some omaps. Things would just not work or would stop >>> working >>> for some drivers with no ideas what was going on. So yeah, having these >>> kind of >>> frameworks in place has helped a lot to avoid breakage like you're >>> describing >>> above. >> >> Trees which had to hit the lowest power states for full customer boards all employed some form of abstraction of clock, power, voltage. The one today in mainline today is the most clean. Aspects around auto-generation are very good even if a bit big in size. >> >> Perhaps main grump I hear is the number of abstraction layers sometimes makes debugging difficult. It would be nice to find smart tricks for compile to prune away some. >> >> Still one can experience some of the mystery issues, as the PRCM connection to IP-device utilizes a protocol which the device side can mess up. If the device does not shut down its local function and associated clocks cleanly it will show 'stuck in transition' and this gates final global changes. In one of the closed implementation we would bug() drivers who did not shut down their internal clocks properly before allowing global clock release. Most of the issues seen in field are at drivers/peripheral-ip. >> >> >>> For some external subsystems it might be possible to let the omap kernel >>> manage >>> powerdomains and other resource via remote proc interface, assuming these >>> registers are ioremapped in the omap kernel side and not owned by the >>> external >>> subsystem. The messaging to do this would add some undesired latencies >>> though, >>> but maybe those would not be so critical for the external subsystems as >>> they >>> tend to be full on or completely off type accelerators. >> >> Humm. Maybe for some. Guess walking what is used and sorting might tell. >> >> The way some subsystems 'ideal' operation is described from designers implies tight control of timing and sequence for things like power state. A RPC doesn't feel like it fits with intent. However... practically speaking from 'full system view' RPC may fit sometimes. A subsystem exporting hooks to save 100uW using its optimal state set against other components running in 500mW range minimizes usefulness. >> >>> The other alternative of course is to implement similar frameworks for the >>> external subsystems. Some of this might be possible to implement as simple >>> macros with some type checks if it's subsystem specific. For doing it for >>> all omap devices, macros were soon found not to be enough as the various >>> bits all over need to be linked together for managing various resources. >> >> Agree. >> >> I don't know insides of all subsystems. Though what I know or hear is kind of a mixed picture. >> >> OMAP has an ultra high level of integration. As such you might find something like DSP-Bios may provide a good hook but quick integration of a previously standalone single purpose piece does not have time to be readapted. >> >> The type checking question kind of grows out of this. Linux might today offer a clean run-time api which is place where high wall should be built with type check. But... the driver might not be able to function yet with the API alone given state of evolution of both ends. > > Regarding the API and type checking I think the following must be enforced: > 1. proper type checking in the API, possibly by the compiler, > 2. clean separation of an API into an internal part (used only by the > framework implementation) and an external part (used by the callers of > the API). > > About 1: using enum for the parameters did not help much AFACIT. The > compiler cannot tell if the parameter from a variable is in the > allowed range. > Any thought? > > About 2: while introducing the functional power states I came across > this problem. Ideally the current states macros (PWRDM_POWER_* and > PWRSTS_*) shall be used by the internal code only in powerdomain*.[ch] > while the callers (pm.c etc) shall use the new macros > (PWRDM_FUNC_PWRST_*) and API (mainly pwrdm_*_func_* and > omap_set_pwrdm_state). > > Here below is a patch extract (trimmed for brevity) to illustrate the problem. FYI the full patch is at http://marc.info/?l=linux-omap&m=133968581305050&w=2 > > From here is see a few possible options: > 1. clearly separate the internal and external parts of the API in the > header file using comments (as done in the patch below) or with a new > #ifdef POWERDOMAIN_INTERNAL_API (ugly I know), Some more details about that option: the internal values could be prefixed by '__' or similar which kinds of highlights a bad API usage. > 2. create a new internal only header file (powerdomain_internal.h) and > include it only from powerdomain*.[ch], > 3. move the external API to pm.h and keep the internal API in powerdomain.h > > Although I am using the func power states as an example I think this > is a applicable to all PM frameworks APIs. > > What do you think? Any thought? > diff --git a/arch/arm/mach-omap2/powerdomain.h > b/arch/arm/mach-omap2/powerdomain.h > index bab84fc..0404f9f 100644 > --- a/arch/arm/mach-omap2/powerdomain.h > +++ b/arch/arm/mach-omap2/powerdomain.h > @@ -26,7 +23,51 @@ > -/* Powerdomain basic power states */ > +/*************************************************************** > + * External API > + ***************************************************************/ > + > +/* Powerdomain functional power states, used by the external API functions */ > +enum pwrdm_func_state { > + PWRDM_FUNC_PWRST_OFF = 0x0, > + PWRDM_FUNC_PWRST_OSWR, > + PWRDM_FUNC_PWRST_CSWR, > + PWRDM_FUNC_PWRST_INACTIVE, > + PWRDM_FUNC_PWRST_ON, > + PWRDM_MAX_FUNC_PWRSTS /* Last value, used as the max value */ > +}; > + > +struct clockdomain; > +struct powerdomain; > + > ... > + > +int pwrdm_read_prev_func_pwrst(struct powerdomain *pwrdm); > +int pwrdm_read_func_pwrst(struct powerdomain *pwrdm); > +int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm); > +extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, > + enum pwrdm_func_state func_pwrst); > + > + > +/*************************************************************** > + * Internal code, only used in powerdomain*.[ch] > + ***************************************************************/ > + > +/* Powerdomain internal power states, internal use only */ > #define PWRDM_POWER_OFF 0x0 > #define PWRDM_POWER_RET 0x1 > #define PWRDM_POWER_INACTIVE 0x2 > @@ -45,7 +86,6 @@ > #define PWRSTS_RET_ON (PWRSTS_RET | PWRSTS_ON) > #define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | PWRSTS_ON) > > - > /* Powerdomain flags */ > #define PWRDM_HAS_HDWR_SAR (1 << 0) /* hardware save-and-restore support */ > #define PWRDM_HAS_MPU_QUIRK (1 << 1) /* MPU pwr domain has MEM bank 0 bits > > >> Review question was pointing re-hitting of bugs for what could be argued 'ideal' internal framework api. How to fix up what is in use by real drivers or to add/fix external api so its useful should be roadmap. >> >> Regards, >> Richard W. >> > > Thanks for starting the discussion! > > Regards, > Jean -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
