On Tuesday, June 14, 2011, Magnus Damm wrote:
> Hi Rafael,

Hi,

> On Sun, Jun 12, 2011 at 5:40 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > Use the generic power domains support introduced by the previous
> > patch to implement support for power domains on SH7372.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> 
> Thanks for your work on this. I just tried this on my Mackerel board,
> but I can't seem to get the pd_power_up() and pd_power_down()
> callbacks to be executed. It is probably a misconfiguration from my
> side.

They trigger for me e.g. after doing

# echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank

Attached is the .config I've been using.

> Here's some feedback on the sh7372-specific code:
> 
> > --- linux-2.6.orig/arch/arm/mach-shmobile/Kconfig
> > +++ linux-2.6/arch/arm/mach-shmobile/Kconfig
> > @@ -19,6 +19,7 @@ config ARCH_SH7372
> >        select CPU_V7
> >        select SH_CLK_CPG
> >        select ARCH_WANT_OPTIONAL_GPIOLIB
> > +       select PM_GENERIC_DOMAINS
> 
> We want to support a single ARM binary for multiple boards,

Surely CONFIG_ARCH_SH7372 will be set in that binary?

> so this should be enabled for all SoCs in mach-shmobile as a whole.

OK, where exactly do you want me to move it?

> > --- linux-2.6.orig/arch/arm/mach-shmobile/pm-sh7372.c
> > +++ linux-2.6/arch/arm/mach-shmobile/pm-sh7372.c
> > @@ -15,16 +15,97 @@
> >  #include <linux/list.h>
> >  #include <linux/err.h>
> >  #include <linux/slab.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/platform_device.h>
> >  #include <asm/system.h>
> >  #include <asm/io.h>
> >  #include <asm/tlbflush.h>
> >  #include <mach/common.h>
> > +#include <mach/sh7372.h>
> >
> >  #define SMFRAM 0xe6a70000
> >  #define SYSTBCR 0xe6150024
> >  #define SBAR 0xe6180020
> >  #define APARMBAREA 0xe6f10020
> >
> > +#define SPDCR 0xe6180008
> > +#define SWUCR 0xe6180014
> > +#define PSTR 0xe6180080
> > +
> > +struct sh7372_domain_data {
> > +       unsigned int bit_shift;
> > +};
> 
> Is it possible to make struct sh7372_domain_data include struct
> generic_pm_domain?

It should be possible to do that.

Do I understand it correctly that you want one structure definition per
power domain instead of the two?

> I suspect so since these two data types seem to be linked together.
> I guess container_of() can be used for conversion between the types?

I don't think that will be necessary. :-)

> > +
> > +static int pd_power_down(struct dev_pm_domain *domain)
> > +{
> > +       struct sh7372_domain_data *dd = domain->platform_data;
> > +       unsigned int mask = 1 << dd->bit_shift;
> > +
> > +       if (__raw_readl(PSTR) & mask) {
> > +               __raw_writel(mask, SPDCR);
> > +
> > +               while (__raw_readl(SPDCR) & mask) {}
> 
> This probably wants a cpu_relax() in the polling loop. I realize that
> my prototype hack lacked that...

OK

> > +               pr_debug("sh7372 power domain down 0x%08x -> PSTR = 0x%08x\n",
> > +                        mask, __raw_readl(PSTR));
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int pd_power_up(struct dev_pm_domain *domain)
> > +{
> > +       struct sh7372_domain_data *dd = domain->platform_data;
> > +       unsigned int mask = 1 << dd->bit_shift;
> > +
> > +       if (!(__raw_readl(PSTR) & mask)) {
> > +               __raw_writel(mask, SWUCR);
> > +
> > +               while (__raw_readl(SWUCR) & mask) {}
> 
> Same cpu_relax() here.

OK

> > +static int __init sh7372_power_domains_init(void)
> > +{
> > +       sh7372_init_domain(&sh7372_a4lc_domain, &sh7372_a4lc_domain_data);
> > +       return 0;
> > +}
> > +core_initcall(sh7372_power_domains_init);
> 
> This initcall is going to be executed regardless which SoC we're
> running on. We only want it called for sh7372 though.

OK

> If you look at other SoC-specific code then you will notice that
> initcalls are only used for functions in mach-shmobile/ that are
> common for all SoCs implemented under mach-shmobile.
> 
> You most likely want to initialize from sh7372_pm_init(), but for that
> to work you probably have to reorder your code inside mackerel_init().

OK, I'll figure out how to do the initialization correctly.

Thanks,
Rafael