ARM: mach-shmobile: sh7372 A4R support V2

From: Magnus Damm <[email protected]>

This patch is V2 of sh7372 A4R power domain support.

The sh7372 A4R hardware power domain contains the SH CPU Core and a set of I/O devices including multimedia accelerators and I2C controllers.

One special case about A4R is the INTCS interrupt controller that needs to be saved and restored to keep working as expected. Also the LCDC hardware blocks are in a different hardware power domain but have their IRQs routed only through INTCS. So as long as LCDCs are active we cannot power down INTCS because that would risk losing interrupts.

---

Changes since V1:
- rebased to fit on pm-test branch of linux-pm github repo
- use simple power_down_ok() governor
- let A4LC depend on A4R to support INTCS

 arch/arm/mach-shmobile/board-ap4evb.c        |    1 
 arch/arm/mach-shmobile/board-mackerel.c      |    1 
 arch/arm/mach-shmobile/include/mach/sh7372.h |    4 ++
 arch/arm/mach-shmobile/intc-sh7372.c         |   52 +++++++++++++++++++++++++-
 arch/arm/mach-shmobile/pm-sh7372.c           |   35 ++++++++++++++++-
 arch/arm/mach-shmobile/setup-sh7372.c        |    8 ++++
 6 files changed, 98 insertions(+), 3 deletions(-)

@@ -1412,6 +1412,7 @@ static void __init ap4evb_init(void)
 	sh7372_add_device_to_domain(&sh7372_a3sp, &sh_mmcif_device);
 	sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi0_device);
 	sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi1_device);
+	sh7372_add_device_to_domain(&sh7372_a4r, &ceu_device);
 
 	hdmi_init_pm_clock();
 	fsi_init_pm_clock();

@@ -1596,6 +1596,7 @@ static void __init mackerel_init(void)
 	sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi1_device);
 #endif
 	sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi2_device);
+	sh7372_add_device_to_domain(&sh7372_a4r, &ceu_device);
 
 	hdmi_init_pm_clock();
 	sh7372_pm_init();

@@ -491,6 +491,7 @@ static inline struct sh7372_pm_domain *t
 extern struct sh7372_pm_domain sh7372_a4lc;
 extern struct sh7372_pm_domain sh7372_a4mp;
 extern struct sh7372_pm_domain sh7372_d4;
+extern struct sh7372_pm_domain sh7372_a4r;
 extern struct sh7372_pm_domain sh7372_a3rv;
 extern struct sh7372_pm_domain sh7372_a3ri;
 extern struct sh7372_pm_domain sh7372_a3sp;

@@ -507,4 +508,7 @@ extern void sh7372_pm_add_subdomain(stru
 #define sh7372_pm_add_subdomain(pd, sd) do { } while(0)
 #endif /* CONFIG_PM */
 
+extern void sh7372_intcs_suspend(void);
+extern void sh7372_intcs_resume(void);
+
 #endif /* __ASM_SH7372_H__ */

@@ -44,6 +44,7 @@
 #define SPDCR 0xe6180008
 #define SWUCR 0xe6180014
 #define SBAR 0xe6180020
+#define WUPRMSK 0xe6180028
 #define WUPSMSK 0xe618002c
 #define WUPSMSK2 0xe6180048
 #define PSTR 0xe6180080

@@ -132,6 +133,24 @@ static int pd_power_up(struct generic_pm
 	return ret;
 }
 
+static int pd_power_up_a4r(struct generic_pm_domain *genpd)
+{
+	int ret = pd_power_up(genpd);
+
+	if (ret = 0)
+		sh7372_intcs_resume();
+
+	return ret;
+}
+
+static int pd_power_down_a4r(struct generic_pm_domain *genpd)
+{
+	sh7372_intcs_suspend();
+	__raw_writel(0x300fffff, WUPRMSK); /* avoid wakeup */
+
+	return pd_power_down(genpd);
+}
+
 static bool pd_active_wakeup(struct device *dev)
 {
 	return true;

@@ -150,6 +169,9 @@ static bool sh7372_power_down_ok(struct
 	if (genpd = &sh7372_a3sp.genpd)
 		return false;
 
+	if (genpd = &sh7372_a4r.genpd)
+		return false;
+
 	return true;
 }
 

@@ -166,8 +188,13 @@ void sh7372_init_pm_domain(struct sh7372
 	genpd->start_device = pm_clk_resume;
 	genpd->dev_irq_safe = true;
 	genpd->active_wakeup = pd_active_wakeup;
-	genpd->power_off = pd_power_down;
-	genpd->power_on = pd_power_up;
+	if (genpd = &sh7372_a4r.genpd) {
+		genpd->power_off = pd_power_down_a4r;
+		genpd->power_on = pd_power_up_a4r;
+	} else {
+		genpd->power_off = pd_power_down;
+		genpd->power_on = pd_power_up;
+	}
 	genpd->power_on(&sh7372_pd->genpd);
 }
 

@@ -199,6 +226,10 @@ struct sh7372_pm_domain sh7372_d4 = {
 	.bit_shift = 3,
 };
 
+struct sh7372_pm_domain sh7372_a4r = {
+	.bit_shift = 5,
+};
+
 struct sh7372_pm_domain sh7372_a3rv = {
 	.bit_shift = 6,
 };

@@ -991,12 +991,14 @@ void __init sh7372_add_standard_devices(
 	sh7372_init_pm_domain(&sh7372_a4lc);
 	sh7372_init_pm_domain(&sh7372_a4mp);
 	sh7372_init_pm_domain(&sh7372_d4);
+	sh7372_init_pm_domain(&sh7372_a4r);
 	sh7372_init_pm_domain(&sh7372_a3rv);
 	sh7372_init_pm_domain(&sh7372_a3ri);
 	sh7372_init_pm_domain(&sh7372_a3sg);
 	sh7372_init_pm_domain(&sh7372_a3sp);
 
 	sh7372_pm_add_subdomain(&sh7372_a4lc, &sh7372_a3rv);
+	sh7372_pm_add_subdomain(&sh7372_a4r, &sh7372_a4lc);
 
 	platform_add_devices(sh7372_early_devices,
 			    ARRAY_SIZE(sh7372_early_devices));

@@ -1020,6 +1022,12 @@ void __init sh7372_add_standard_devices(
 	sh7372_add_device_to_domain(&sh7372_a3sp, &dma2_device);
 	sh7372_add_device_to_domain(&sh7372_a3sp, &usb_dma0_device);
 	sh7372_add_device_to_domain(&sh7372_a3sp, &usb_dma1_device);
+	sh7372_add_device_to_domain(&sh7372_a4r, &iic0_device);
+	sh7372_add_device_to_domain(&sh7372_a4r, &veu0_device);
+	sh7372_add_device_to_domain(&sh7372_a4r, &veu1_device);
+	sh7372_add_device_to_domain(&sh7372_a4r, &veu2_device);
+	sh7372_add_device_to_domain(&sh7372_a4r, &veu3_device);
+	sh7372_add_device_to_domain(&sh7372_a4r, &jpu_device);
 }
 
 void __init sh7372_add_early_devices(void)

Unfortunately, the original patch breaks resume from system suspend, because it causes the A4R domain be turned off during the _noirq phase of device suspend. The domain stays off during syscore_ops resume, so intc_resume() crashes, because it can't access the device. To address this problem it is sufficient to prevent the domain from being turned off, which is done in my version of the patch below (on top of my version of the A3SP patch sent a while
ago: http://article.gmane.org/gmane.linux.ports.sh.devel/12565).

Thanks,
Rafael

---
From: Magnus Damm <[email protected]>
Subject: ARM: mach-shmobile: sh7372 A4R support

This change adds support for the sh7372 A4R power domain.

The sh7372 A4R hardware power domain contains the
SH CPU Core and a set of I/O devices including
multimedia accelerators and I2C controllers.

One special case about A4R is the INTCS interrupt
controller that needs to be saved and restored to
keep working as expected. Also the LCDC hardware
blocks are in a different hardware power domain
but have their IRQs routed only through INTCS. So
as long as LCDCs are active we cannot power down
INTCS because that would risk losing interrupts.

---
 arch/arm/mach-shmobile/board-ap4evb.c        |    1 
 arch/arm/mach-shmobile/board-mackerel.c      |    1 
 arch/arm/mach-shmobile/include/mach/sh7372.h |    7 +++
 arch/arm/mach-shmobile/intc-sh7372.c         |   52 ++++++++++++++++++++++++++-
 arch/arm/mach-shmobile/pm-sh7372.c           |   29 ++++++++++++++-
 arch/arm/mach-shmobile/setup-sh7372.c        |    8 ++++
 6 files changed, 96 insertions(+), 2 deletions(-)

Index: linux/arch/arm/mach-shmobile/board-ap4evb.c =================================--- linux.orig/arch/arm/mach-shmobile/board-ap4evb.c +++ linux/arch/arm/mach-shmobile/board-ap4evb.c
@@ -1412,6 +1412,7 @@ static void __init ap4evb_init(void)
sh7372_add_device_to_domain(&sh7372_a3sp, &sh_mmcif_device); sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi0_device); sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi1_device);
+ sh7372_add_device_to_domain(&sh7372_a4r, &ceu_device);

hdmi_init_pm_clock();
fsi_init_pm_clock();
Index: linux/arch/arm/mach-shmobile/board-mackerel.c =================================--- linux.orig/arch/arm/mach-shmobile/board-mackerel.c +++ linux/arch/arm/mach-shmobile/board-mackerel.c
@@ -1596,6 +1596,7 @@ static void __init mackerel_init(void)
sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi1_device);
#endif
sh7372_add_device_to_domain(&sh7372_a3sp, &sdhi2_device);
+ sh7372_add_device_to_domain(&sh7372_a4r, &ceu_device);

hdmi_init_pm_clock();
sh7372_pm_init();
Index: linux/arch/arm/mach-shmobile/include/mach/sh7372.h =================================--- linux.orig/arch/arm/mach-shmobile/include/mach/sh7372.h +++ linux/arch/arm/mach-shmobile/include/mach/sh7372.h
@@ -480,8 +480,11 @@ struct platform_device;
struct sh7372_pm_domain {
struct generic_pm_domain genpd;
struct dev_power_governor *gov;
+ void (*suspend)(void);
+ void (*resume)(void);
unsigned int bit_shift;
bool no_debug;
+ bool stay_on;
};

static inline struct sh7372_pm_domain *to_sh7372_pd(struct generic_pm_domain *d)
@@ -493,6 +496,7 @@ static inline struct sh7372_pm_domain *t
extern struct sh7372_pm_domain sh7372_a4lc;
extern struct sh7372_pm_domain sh7372_a4mp;
extern struct sh7372_pm_domain sh7372_d4;
+extern struct sh7372_pm_domain sh7372_a4r;
extern struct sh7372_pm_domain sh7372_a3rv;
extern struct sh7372_pm_domain sh7372_a3ri;
extern struct sh7372_pm_domain sh7372_a3sp;
@@ -512,4 +516,7 @@ extern void sh7372_pm_add_subdomain(stru
#define sh7372_pm_add_subdomain(pd, sd) do { } while(0)
#endif /* CONFIG_PM */

+extern void sh7372_intcs_suspend(void);
+extern void sh7372_intcs_resume(void);
+
#endif /* __ASM_SH7372_H__ */
Index: linux/arch/arm/mach-shmobile/intc-sh7372.c =================================--- linux.orig/arch/arm/mach-shmobile/intc-sh7372.c +++ linux/arch/arm/mach-shmobile/intc-sh7372.c
@@ -606,9 +606,16 @@ static void intcs_demux(unsigned int irq
generic_handle_irq(intcs_evt2irq(evtcodeas));
}

+static void __iomem *intcs_ffd2;
+static void __iomem *intcs_ffd5;
+
void __init sh7372_init_irq(void)
{
- void __iomem *intevtsa = ioremap_nocache(0xffd20100, PAGE_SIZE);
+ void __iomem *intevtsa;
+
+ intcs_ffd2 = ioremap_nocache(0xffd20000, PAGE_SIZE);
+ intevtsa = intcs_ffd2 + 0x100;
+ intcs_ffd5 = ioremap_nocache(0xffd50000, PAGE_SIZE);

register_intc_controller(&intca_desc); register_intc_controller(&intcs_desc);
@@ -617,3 +624,46 @@ void __init sh7372_init_irq(void)
irq_set_handler_data(evt2irq(0xf80), (void *)intevtsa); irq_set_chained_handler(evt2irq(0xf80), intcs_demux);
}
+
+static unsigned short ffd2[0x200];
+static unsigned short ffd5[0x100];
+
+void sh7372_intcs_suspend(void)
+{
+ int k;
+
+ for (k = 0x00; k <= 0x30; k += 4)
+ ffd2[k] = __raw_readw(intcs_ffd2 + k);
+
+ for (k = 0x80; k <= 0xb0; k += 4)
+ ffd2[k] = __raw_readb(intcs_ffd2 + k);
+
+ for (k = 0x180; k <= 0x188; k += 4)
+ ffd2[k] = __raw_readb(intcs_ffd2 + k);
+
+ for (k = 0x00; k <= 0x3c; k += 4)
+ ffd5[k] = __raw_readw(intcs_ffd5 + k);
+
+ for (k = 0x80; k <= 0x9c; k += 4)
+ ffd5[k] = __raw_readb(intcs_ffd5 + k);
+}
+
+void sh7372_intcs_resume(void)
+{
+ int k;
+
+ for (k = 0x00; k <= 0x30; k += 4)
+ __raw_writew(ffd2[k], intcs_ffd2 + k);
+
+ for (k = 0x80; k <= 0xb0; k += 4)
+ __raw_writeb(ffd2[k], intcs_ffd2 + k);
+
+ for (k = 0x180; k <= 0x188; k += 4)
+ __raw_writeb(ffd2[k], intcs_ffd2 + k);
+
+ for (k = 0x00; k <= 0x3c; k += 4)
+ __raw_writew(ffd5[k], intcs_ffd5 + k);
+
+ for (k = 0x80; k <= 0x9c; k += 4)
+ __raw_writeb(ffd5[k], intcs_ffd5 + k);
+}
Index: linux/arch/arm/mach-shmobile/pm-sh7372.c =================================--- linux.orig/arch/arm/mach-shmobile/pm-sh7372.c +++ linux/arch/arm/mach-shmobile/pm-sh7372.c
@@ -44,6 +44,7 @@
#define SPDCR 0xe6180008
#define SWUCR 0xe6180014
#define SBAR 0xe6180020
+#define WUPRMSK 0xe6180028
#define WUPSMSK 0xe618002c
#define WUPSMSK2 0xe6180048
#define PSTR 0xe6180080
@@ -80,6 +81,12 @@ static int pd_power_down(struct generic_
struct sh7372_pm_domain *sh7372_pd = to_sh7372_pd(genpd); unsigned int mask = 1 << sh7372_pd->bit_shift;

+ if (sh7372_pd->suspend)
+ sh7372_pd->suspend();
+
+ if (sh7372_pd->stay_on)
+ return 0;
+
if (__raw_readl(PSTR) & mask) {
unsigned int retry_count;

@@ -106,6 +113,9 @@ static int pd_power_up(struct generic_pm
unsigned int retry_count;
int ret = 0;

+ if (sh7372_pd->stay_on)
+ goto out;
+
if (__raw_readl(PSTR) & mask)
goto out;

@@ -122,14 +132,23 @@ static int pd_power_up(struct generic_pm
if (__raw_readl(SWUCR) & mask)
ret = -EIO;

- out:
if (!sh7372_pd->no_debug)
pr_debug("sh7372 power domain up 0x%08x -> PSTR = 0x%08x\n",
mask, __raw_readl(PSTR));

+ out:
+ if (ret = 0 && sh7372_pd->resume)
+ sh7372_pd->resume();
+
return ret;
}

+static void sh7372_a4r_suspend(void)
+{
+ sh7372_intcs_suspend();
+ __raw_writel(0x300fffff, WUPRMSK); /* avoid wakeup */
+}
+
static bool pd_active_wakeup(struct device *dev) {
return true;
@@ -214,6 +233,14 @@ struct sh7372_pm_domain sh7372_d4 = {
.bit_shift = 3,
};

+struct sh7372_pm_domain sh7372_a4r = {
+ .bit_shift = 5,
+ .gov = &sh7372_always_on_gov,
+ .suspend = sh7372_a4r_suspend,
+ .resume = sh7372_intcs_resume,
+ .stay_on = true,
+};
+
struct sh7372_pm_domain sh7372_a3rv = {
.bit_shift = 6,
};
Index: linux/arch/arm/mach-shmobile/setup-sh7372.c =================================--- linux.orig/arch/arm/mach-shmobile/setup-sh7372.c +++ linux/arch/arm/mach-shmobile/setup-sh7372.c
@@ -991,12 +991,14 @@ void __init sh7372_add_standard_devices(
sh7372_init_pm_domain(&sh7372_a4lc); sh7372_init_pm_domain(&sh7372_a4mp); sh7372_init_pm_domain(&sh7372_d4);
+ sh7372_init_pm_domain(&sh7372_a4r);
sh7372_init_pm_domain(&sh7372_a3rv); sh7372_init_pm_domain(&sh7372_a3ri); sh7372_init_pm_domain(&sh7372_a3sg); sh7372_init_pm_domain(&sh7372_a3sp);

sh7372_pm_add_subdomain(&sh7372_a4lc, &sh7372_a3rv);
+ sh7372_pm_add_subdomain(&sh7372_a4r, &sh7372_a4lc);

platform_add_devices(sh7372_early_devices,
ARRAY_SIZE(sh7372_early_devices));
@@ -1020,6 +1022,12 @@ void __init sh7372_add_standard_devices(
sh7372_add_special_dev_to_domain(&sh7372_a3sp, &dma2_device); sh7372_add_special_dev_to_domain(&sh7372_a3sp, &usb_dma0_device); sh7372_add_special_dev_to_domain(&sh7372_a3sp, &usb_dma1_device);
+ sh7372_add_device_to_domain(&sh7372_a4r, &iic0_device);
+ sh7372_add_device_to_domain(&sh7372_a4r, &veu0_device);
+ sh7372_add_device_to_domain(&sh7372_a4r, &veu1_device);
+ sh7372_add_device_to_domain(&sh7372_a4r, &veu2_device);
+ sh7372_add_device_to_domain(&sh7372_a4r, &veu3_device);
+ sh7372_add_device_to_domain(&sh7372_a4r, &jpu_device);
}

void __init sh7372_add_early_devices(void)

Well, I'd prefer to do something like the patch below instead.

Please let me know what you think.

Thanks,
Rafael

---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / shmobile: Avoid restoring the INTCS state during initialization

The SH7372 PM domain initialization routine calls pd_power_up() that executes the domain's .resume() callback, if present, and for the A4R domain this callback attepmts to restore the INTCS state from uninitialized data. To avoid that, introduce __pd_power_up() that will only execute the domain's .resume() callback if its second argument is 'true' and make the SH7372 PM domain initialization use it with 'false' as its second argument. Redefine pd_power_up() as a wrapper around __pd_power_up().

---
 arch/arm/mach-shmobile/pm-sh7372.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: linux/arch/arm/mach-shmobile/pm-sh7372.c =================================--- linux.orig/arch/arm/mach-shmobile/pm-sh7372.c +++ linux/arch/arm/mach-shmobile/pm-sh7372.c
@@ -107,9 +107,8 @@ static int pd_power_down(struct generic_
return 0;
}

-static int pd_power_up(struct generic_pm_domain *genpd)
+static int __pd_power_up(struct sh7372_pm_domain *sh7372_pd, bool do_resume)
{
- struct sh7372_pm_domain *sh7372_pd = to_sh7372_pd(genpd);
unsigned int mask = 1 << sh7372_pd->bit_shift;
unsigned int retry_count;
int ret = 0;
@@ -138,12 +137,17 @@ static int pd_power_up(struct generic_pm
mask, __raw_readl(PSTR));

out:
- if (ret = 0 && sh7372_pd->resume)
+ if (ret = 0 && sh7372_pd->resume && do_resume)
sh7372_pd->resume();

return ret;
}

+static int pd_power_up(struct generic_pm_domain *genpd) +{
+ return __pd_power_up(to_sh7372_pd(genpd), true);
+}
+
static void sh7372_a4r_suspend(void)
{
sh7372_intcs_suspend();
@@ -209,7 +213,7 @@ void sh7372_init_pm_domain(struct sh7372
genpd->dev_irq_safe = true; genpd->power_off = pd_power_down; genpd->power_on = pd_power_up;
- genpd->power_on(&sh7372_pd->genpd);
+ __pd_power_up(sh7372_pd, false);
}

void sh7372_add_device_to_domain(struct sh7372_pm_domain *sh7372_pd,

Looks fine to me, from my point of view your implementation is better than the other alternatives.

It is correct that Guennadi reported this issue, and the Signed-off clearly shows that this specific implementation is coming from you. I suppose there are more important things in life than git attribution, but I do feel that I did spend a significant amount of time tracking down this issue, writing the initial patch and then also communicating with you guys. I may be wrong, but I suspect that if didn't do this then this issue wouldn't have been fixed.

Unfortunately there does not seem to be any tag for that kind of work, but if it existed then I would prefer to add this in between you and Guennadi:

Cheers,

/ magnus

Yes, I also was looking for a suitable attribution to mention you in my version of the patch, and ended up just saying "thanks." But, I think, we're allowed to be creative here, we're not prohibited to create our own tags - whether or not they're going to be standardised. Maybe we can put it even stronger to emphasise, that you not only tracked the problem down, but also proposed the first fix. Something like

Initial-fix-by:
or
Original-fix-by:

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

OK

I'll add your ack too, if you don't mind. :-)

Thanks,
Rafael