Single-stepping with UBC on SH7785

Upgrading the kernel for it, we noticed that with ``recent'' kernels, single-stepping is broken for our SH7785-based board. I tracked this down to commit 09a072947791088b88ae15111cf68fc5aaaf758d (2009-11-09), ``preliminary support for the SH-4A UBC to the hw-breakpoints API''.

After learning from the SH7785 manual how to use and program the UBC, it seemed obvious to me that what it implemented nowadays in arch/sh/kernel/cpu/sh4a/ubc.c for programming the UBC does not match how it used to be, so I changed that according to my interpretation of the manual, as well as the pre-09a072's arch/sh/kernel/process_32.c:__switch_to implementation. This is the attached 0001-WIP-Restore-single-stepping-functionality-on-our-SH7.patch. Yet, it still wouldn't work.

After several hours of grief, I came up with the additional 0001-Wire-the-clock-of-the-SH7785-s-UBC-as-expected-in-ub.patch -- and it worked! (Meh, so simple...)

... and today I figured out that my first patch isn't even needed -- but I don't understand how the current ubc.c implementation gets away with not using the asid stuff, for example? And shouldn't it respect the reserved value UBC_CRR_RES as well as UBC_CRR_INIT and UBC_CBR_INIT that I re-introduced? Also the manual suggests a different order for programming the registers.

As soon as someone starts working on adding user-space controlled hardware breakpoint and/or watchpoint support, this will need further untangling/cleanup.

Grüße,
Thomas

From 2942691b300a14eb3751aa687165e1c00da3cedd Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <[email protected]>
Date: Tue, 14 Feb 2012 16:17:47 +0100
Subject: [PATCH] [WIP] Restore single-stepping functionality on our
SH7785-based board.

Signed-off-by Thomas Schwinge <[email protected]>

---
 arch/sh/kernel/cpu/sh4a/ubc.c |   67 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 66 insertions(+), 1 deletions(-)

arch/sh/kernel/cpu/sh4a/ubc.c +66 -1

index efb2745..86c54cc 100644
@@ -14,6 +14,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <asm/hw_breakpoint.h>
+#include <asm/mmu_context.h>
 
 #define UBC_CBR(idx)	(0xff200000 + (0x20 * idx))
 #define UBC_CRR(idx)	(0xff200004 + (0x20 * idx))

@@ -24,30 +25,83 @@
 #define UBC_CBCR	0xff200620
 
 /* CRR */
+#define UBC_CRR_RES	(1 << 13)
 #define UBC_CRR_PCB	(1 << 1)
 #define UBC_CRR_BIE	(1 << 0)
+#define UBC_CRR_INIT	0x00002000
 
 /* CBR */
+#define UBC_CBR_AIE	(1 << 30)
+#define UBC_CBR_ID_INST	(1 << 4)
 #define UBC_CBR_CE	(1 << 0)
+#define UBC_CBR_INIT	0x20000000
+
+#define UBC_CBR_AIV_SET(asid)	(((asid) << UBC_CBR_AIV_SHIFT) & UBC_CBR_AIV_MASK)
+#define UBC_CBR_AIV_SHIFT	16
+#define UBC_CBR_AIV_MASK	0x00ff0000
 
 static struct sh_ubc sh4a_ubc;
 
 static void sh4a_ubc_enable(struct arch_hw_breakpoint *info, int idx)
 {
+#if 0
 	__raw_writel(UBC_CBR_CE | info->len | info->type, UBC_CBR(idx));
+#else
+	int asid = 0;
+	unsigned long tmp;
+
+	printk("0x%p\t%s for 0x%lx on channel %d\n", current, __FUNCTION__, info->address, idx);
+
+//#ifdef CONFIG_MMU
+//	asid = cpu_asid(smp_processor_id(), current->mm);
+	asid = get_asid();
+//#endif
+
+	tmp = UBC_CBR_CE
+		| UBC_CBR_ID_INST
+		| UBC_CBR_AIE
+		| UBC_CBR_AIV_SET(asid)
+		/* | info->len */
+		/* | info->type */ | (1 << 1);
+	__raw_writel(tmp, UBC_CBR(idx));
+	printk("  asid 0x%x, CBR: %lx", asid, tmp);
+#endif
 	__raw_writel(info->address, UBC_CAR(idx));
+
+	__raw_writel(0, UBC_CAMR(idx));
+
+//	__raw_writel(0, UBC_CBCR);
+
+	tmp = UBC_CRR_BIE
+		| UBC_CRR_PCB
+		| UBC_CRR_RES;
+	__raw_writel(tmp, UBC_CRR(idx));
+	printk(", CRR: %lx\n", tmp);
+
+	/* dummy read for write posting */
+	(void) __raw_readl(UBC_CBR(idx));
+	(void) __raw_readl(UBC_CRR(idx));
 }
 
 static void sh4a_ubc_disable(struct arch_hw_breakpoint *info, int idx)
 {
+	printk("0x%p\t%s for 0x%lx on channel %d\n", current, __FUNCTION__, info->address, idx);
+
+#if 0
 	__raw_writel(0, UBC_CBR(idx));
 	__raw_writel(0, UBC_CAR(idx));
+#else
+	__raw_writel(UBC_CBR_INIT, UBC_CBR(idx));
+	__raw_writel(UBC_CRR_INIT, UBC_CRR(idx));
+#endif
 }
 
 static void sh4a_ubc_enable_all(unsigned long mask)
 {
 	int i;
 
+	printk("0x%p\t%s with mask 0x%lx\n", current, __FUNCTION__, mask);
+
 	for (i = 0; i < sh4a_ubc.num_events; i++)
 		if (mask & (1 << i))
 			__raw_writel(__raw_readl(UBC_CBR(i)) | UBC_CBR_CE,

@@ -58,6 +112,8 @@ static void sh4a_ubc_disable_all(void)
 {
 	int i;
 
+	printk("0x%p\t%s\n", current, __FUNCTION__);
+
 	for (i = 0; i < sh4a_ubc.num_events; i++)
 		__raw_writel(__raw_readl(UBC_CBR(i)) & ~UBC_CBR_CE,
 			     UBC_CBR(i));

@@ -68,6 +124,8 @@ static unsigned long sh4a_ubc_active_mask(void)
 	unsigned long active = 0;
 	int i;
 
+	printk("0x%p\t%s\n", current, __FUNCTION__);
+
 	for (i = 0; i < sh4a_ubc.num_events; i++)
 		if (__raw_readl(UBC_CBR(i)) & UBC_CBR_CE)
 			active |= (1 << i);

@@ -77,11 +135,15 @@ static unsigned long sh4a_ubc_active_mask(void)
 
 static unsigned long sh4a_ubc_triggered_mask(void)
 {
+	printk("0x%p\t%s\n", current, __FUNCTION__);
+
 	return __raw_readl(UBC_CCMFR);
 }
 
 static void sh4a_ubc_clear_triggered_mask(unsigned long mask)
 {
+	printk("0x%p\t%s with mask 0x%lx\n", current, __FUNCTION__, mask);
+
 	__raw_writel(__raw_readl(UBC_CCMFR) & ~mask, UBC_CCMFR);
 }
 

@@ -114,15 +176,18 @@ static int __init sh4a_ubc_init(void)
 
 	__raw_writel(0, UBC_CBCR);
 
+#if 0
 	for (i = 0; i < sh4a_ubc.num_events; i++) {
 		__raw_writel(0, UBC_CAMR(i));
 		__raw_writel(0, UBC_CBR(i));
 
-		__raw_writel(UBC_CRR_BIE | UBC_CRR_PCB, UBC_CRR(i));
+//		__raw_writel(UBC_CRR_BIE | UBC_CRR_PCB, UBC_CRR(i));
+		__raw_writel(UBC_CRR_BIE | UBC_CRR_PCB | UBC_CRR_RES, UBC_CRR(i));
 
 		/* dummy read for write posting */
 		(void)__raw_readl(UBC_CRR(i));
 	}
+#endif
 
 	clk_disable(ubc_iclk);
 

Signature · 11 lines

1.7.5.4

From c6696f9fcffcce6739449ea681a38c30e4799017 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <[email protected]>
Date: Tue, 14 Feb 2012 16:19:49 +0100
Subject: [PATCH] Wire the clock of the SH7785's UBC as expected in ubc.c.

Signed-off-by: Thomas Schwinge <[email protected]>
---
 arch/sh/kernel/cpu/sh4a/clock-sh7785.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/sh/kernel/cpu/sh4a/clock-sh7785.c +1 -1

index e5b420c..2b31443 100644
@@ -156,7 +156,7 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("siof_fck", &mstp_clks[MSTP003]),
 	CLKDEV_CON_ID("hspi_fck", &mstp_clks[MSTP002]),
 	CLKDEV_CON_ID("hudi_fck", &mstp_clks[MSTP119]),
-	CLKDEV_CON_ID("ubc_fck", &mstp_clks[MSTP117]),
+	CLKDEV_CON_ID("ubc0", &mstp_clks[MSTP117]),
 	CLKDEV_CON_ID("dmac_11_6_fck", &mstp_clks[MSTP105]),
 	CLKDEV_CON_ID("dmac_5_0_fck", &mstp_clks[MSTP104]),
 	CLKDEV_CON_ID("gdta_fck", &mstp_clks[MSTP100]),

Signature · 1 line

1.7.5.4

After learning from the SH7785 manual how to use and program the UBC, it seemed obvious to me that what it implemented nowadays in arch/sh/kernel/cpu/sh4a/ubc.c for programming the UBC does not match how it used to be

After several hours of grief, I came up with the additional 0001-Wire-the-clock-of-the-SH7785-s-UBC-as-expected-in-ub.patch -- and it worked! (Meh, so simple...)

... and today I figured out that my first patch isn't even needed -- but I don't understand how the current ubc.c implementation gets away with not using the asid stuff, for example? And shouldn't it respect the reserved value UBC_CRR_RES as well as UBC_CRR_INIT and UBC_CBR_INIT that I re-introduced? Also the manual suggests a different order for programming the registers.

As soon as someone starts working on adding user-space controlled hardware breakpoint and/or watchpoint support, this will need further untangling/cleanup.

Quoted 5 lines · Thomas Schwinge

After learning from the SH7785 manual how to use and program the UBC, it seemed obvious to me that what it implemented nowadays in arch/sh/kernel/cpu/sh4a/ubc.c for programming the UBC does not match how it used to be

That would be because we're using it differently. It was originally reworked in order to permit use of multiple UBC channels, and geared at the ksym tracer (subsequently superseded by generic perf hw_breakpoint API utilization). The intent was the model the same single-step behaviour as previously over top of the new API, but there are some caveats (ie, perf can have all of the available channels locked down, making set_single_step() fail).

Quoted 4 lines

After several hours of grief, I came up with the additional 0001-Wire-the-clock-of-the-SH7785-s-UBC-as-expected-in-ub.patch -- and it worked! (Meh, so simple...)

Sorry about that, I prototyped on 7786 and should have more diligently grepped for other UBC clock definitions!

Quoted 7 lines

... and today I figured out that my first patch isn't even needed -- but I don't understand how the current ubc.c implementation gets away with not using the asid stuff, for example? And shouldn't it respect the reserved value UBC_CRR_RES as well as UBC_CRR_INIT and UBC_CBR_INIT that I re-introduced? Also the manual suggests a different order for programming the registers.

The reserved bit is functionally immaterial. It's always wired to 1, so it makes no difference what is read/written to it. We can add a 1 write to it to line up with the manual if you like, but in general the UBC chapter has been cut-and-pasted from legacy parts for years, to the extent that the manual should really only be considered a loose guideline (for some CPUs the register map is nowhere near where the manual suggests, for example). Your CRR_INIT value is just setting CRR_RES anyways.

The CBR_INIT value relates to matching conditions for the given channel, and their values need to be derived from the channel being used rather than a fixed constant. You are correct that there is a bug here though, in that the CBR write forces a CCMFR.MF0 match, while we need to set the CBR.MFI relative to the channel index (you can tell this was only really tested one channel at a time!). This probably would have been caught earlier if we had set CBR.MFE, but we don't really need it for anything.

Quoted 4 lines

As soon as someone starts working on adding user-space controlled hardware breakpoint and/or watchpoint support, this will need further untangling/cleanup.

Now that the kernel uses the UBC alongside userspace it's quite possible that we'll have to adjust some of the settings, and I'm certainly interested in hearing about any troubles you encounter. I tested the single-step stuff with the utrace testsuite if I recall correctly, and it seemed to work alright (even with the ksym tracer tying down another channel for watchpoint use at the time).

I left the ASID stuff out initially to keep things simple (SH-X3 cores and later all have extended ASIDs, so we have an extra set of CBR registers to program for extended ASID matching -- this applies to anything with PTEAEX capabilities). We obviously don't require it on the kernel side, but you're right that we should add it back in for the userspace case. If you want to hack up some patches for that I'll certainly use them, otherwise I'll see about hacking something up for you to test.

It wasn't entirely obvious from your mail, but is the general conclusion from all this that with the clock properly registered for SH7785 that single-stepping works as you expected it to?

Quoted 7 lines · Paul Mundt

That would be because we're using it differently. It was originally reworked in order to permit use of multiple UBC channels, and geared at the ksym tracer (subsequently superseded by generic perf hw_breakpoint API utilization). The intent was the model the same single-step behaviour as previously over top of the new API, but there are some caveats (ie, perf can have all of the available channels locked down, making set_single_step() fail).

The interface of user_enable_single_step assumes it cannot fail. But in there, set_single_step may in fact fail (as I understand it), but its return value is currently ignored. What should happen in this case? Patch a breakpoint instruction into the code instead of using the UBC for single stepping?

In set_single_step, currently only ptrace_bps[0] is considered for use -- which is not a problem for the moment, as single stepping is the only user.

Quoted 8 lines

The reserved bit is functionally immaterial. It's always wired to 1, so it makes no difference what is read/written to it. We can add a 1 write to it to line up with the manual if you like, but in general the UBC chapter has been cut-and-pasted from legacy parts for years, to the extent that the manual should really only be considered a loose guideline (for some CPUs the register map is nowhere near where the manual suggests, for example). Your CRR_INIT value is just setting CRR_RES anyways.

Huh, OK... For me, the manual is the only reference I have. (For example, the manual says to always write reserved bits as they are read/defined.) This is why first worked a lot on aligning the implementation with the manual. Please tell which other documentation should I be looking at?

Quoted 8 lines

I left the ASID stuff out initially to keep things simple (SH-X3 cores and later all have extended ASIDs, so we have an extra set of CBR registers to program for extended ASID matching -- this applies to anything with PTEAEX capabilities). We obviously don't require it on the kernel side, but you're right that we should add it back in for the userspace case. If you want to hack up some patches for that I'll certainly use them, otherwise I'll see about hacking something up for you to test.

I will have to learn more about ASIDs. Is the manual the correct document to read?

Quoted 3 lines

It wasn't entirely obvious from your mail, but is the general conclusion from all this that with the clock properly registered for SH7785 that single-stepping works as you expected it to?

That is correct. For convenience, I'm again attaching the patch here -- please push that one for the moment.