Re: [PATCH 8/8] ARM / shmobile: Support for I/O PM domains for SH7372 (v5)

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Wednesday, June 15, 2011, Magnus Damm wrote:
> On Wed, Jun 15, 2011 at 6:16 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday, June 14, 2011, Magnus Damm wrote:
> >> 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.
> 
> Thanks, I can trigger using sysfs and your kernel configuration.

Good.

> However, I assumed it also would work when the sceen saver kicked in.
> I recall it being fbcon that controls the screen save, perhaps
> something else. So just wait a bit and see if you also can reproduce
> it. The console gets black but the power is still on...

I noticed that, but I think it simply means pm_runtime_put() isn't
called in that code path.

> Also forcing to go back to powered-on state (see below) doesn't work that well:
> # echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> 
> It looks like we loose the panning information somehow. Most likely a
> LCDC driver bug. Unless the driver callbacks are not being invoked as
> expected.

That should be easy to verify, I'll do that.

> Also, there is garbage in on the screen if FB_SH_MOBILE_MERAM is
> enabled. The MERAM hardware is a 1.5 MiB memory block that can be used
> as a LCD cache. It sits in the same hardware power domain as the
> LCDCs. I don't think the MERAM software supports power down
> unfortunately. Disabling MERAM support removes the garbage on the
> screen.

Well, I can't really comment here.

> >> 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?
> 
> Ideally to ARCH_SHMOBILE in arch/arm/Kconfig.

OK

> >> > --- 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?
> 
> Yes, at least that's what I would do to keep the data together. I
> don't care that much though, so feel free to implement it however
> you'd like.

OK, I think I can merge them.

Thanks,
Rafael