Single-stepping with UBC on SH7785

Single-stepping with UBC on SH7785

[Thomas Schwinge]

[-- Attachment #1.1: Type: text/plain, Size: 1529 bytes --]

Hi!

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



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-WIP-Restore-single-stepping-functionality-on-our-SH7.patch --]
[-- Type: text/x-diff, Size: 4558 bytes --]

From 2942691b300a14eb3751aa687165e1c00da3cedd Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
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 <thomas@codesourcery.com>
---
 arch/sh/kernel/cpu/sh4a/ubc.c |   67 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 66 insertions(+), 1 deletions(-)

diff --git a/arch/sh/kernel/cpu/sh4a/ubc.c b/arch/sh/kernel/cpu/sh4a/ubc.c
index efb2745..86c54cc 100644
--- a/arch/sh/kernel/cpu/sh4a/ubc.c
+++ b/arch/sh/kernel/cpu/sh4a/ubc.c
@@ -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);
 
-- 
1.7.5.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0001-Wire-the-clock-of-the-SH7785-s-UBC-as-expected-in-ub.patch --]
[-- Type: text/x-diff, Size: 1109 bytes --]

From c6696f9fcffcce6739449ea681a38c30e4799017 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
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 <thomas@codesourcery.com>
---
 arch/sh/kernel/cpu/sh4a/clock-sh7785.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7785.c b/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
index e5b420c..2b31443 100644
--- a/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
+++ b/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
@@ -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]),
-- 
1.7.5.4


[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: Single-stepping with UBC on SH7785

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 7726 bytes --]

Hi!

Ping.

On Tue, 14 Feb 2012 17:18:52 +0100, I wrote:
> 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 <thomas@codesourcery.com>
> 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 <thomas@codesourcery.com>
> ---
>  arch/sh/kernel/cpu/sh4a/ubc.c |   67 ++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 66 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/sh/kernel/cpu/sh4a/ubc.c b/arch/sh/kernel/cpu/sh4a/ubc.c
> index efb2745..86c54cc 100644
> --- a/arch/sh/kernel/cpu/sh4a/ubc.c
> +++ b/arch/sh/kernel/cpu/sh4a/ubc.c
> @@ -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);
>  
> -- 
> 1.7.5.4
> 
> From c6696f9fcffcce6739449ea681a38c30e4799017 Mon Sep 17 00:00:00 2001
> From: Thomas Schwinge <thomas@codesourcery.com>
> 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 <thomas@codesourcery.com>
> ---
>  arch/sh/kernel/cpu/sh4a/clock-sh7785.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7785.c b/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
> index e5b420c..2b31443 100644
> --- a/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
> +++ b/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
> @@ -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]),
> -- 
> 1.7.5.4
> 


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: Single-stepping with UBC on SH7785

[Paul Mundt]

On Tue, Feb 14, 2012 at 05:18:52PM +0100, Thomas Schwinge wrote:
> 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).

> 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!

> ... 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.

> 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?

Re: Single-stepping with UBC on SH7785

[Thomas Schwinge]

[-- Attachment #1.1: Type: text/plain, Size: 5227 bytes --]

Hi Paul!

Thanks for the detailed answer -- that was helpful.  Some further
comments.


On Fri, 24 Feb 2012 14:36:47 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> On Tue, Feb 14, 2012 at 05:18:52PM +0100, Thomas Schwinge wrote:
> > 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).

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.


> > 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!

At least I learned a lot about all this Linux kernel code.  :-|


> > ... 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.

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?

> 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.
> 
> > 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).

For GDB, I have so far only needed one channel for single stepping.  We
are interested in adding user-space watchpoint support.


> 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?


> 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.


Grüße,
 Thomas



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Wire-the-clock-of-the-SH7785-s-UBC-as-expected-in-ub.patch --]
[-- Type: text/x-diff, Size: 1109 bytes --]

From c6696f9fcffcce6739449ea681a38c30e4799017 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
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 <thomas@codesourcery.com>
---
 arch/sh/kernel/cpu/sh4a/clock-sh7785.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7785.c b/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
index e5b420c..2b31443 100644
--- a/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
+++ b/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
@@ -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]),
-- 
1.7.5.4


[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: Single-stepping with UBC on SH7785

[Paul Mundt]

On Thu, Mar 01, 2012 at 04:50:46PM +0100, Thomas Schwinge wrote:
> On Fri, 24 Feb 2012 14:36:47 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> > 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?
> 
That's one option, yes. For starters it would also be worthwhile teaching
ptrace_resume() about user_enable_single_step() failures for pushing up
an the set_single_step() errno value.

> 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.
> 
Yes, this really should be fixed, too.

> > 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?
> 
There isn't much I'm afraid. I also generally only have the manual to go
with. The problem however is that the manual for the most part is either
woefully out of date or just completely inaccurate. It's a loose
reference, nothing more.

> > 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?
> 
Yes. Note that we're using PTEAEX unconditionally on parts that support
it, so that will need to be taken in to account, too. I can take care of
that part though if you're hacking something simple up for the easier
parts.

> > 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.
> 
I forgot to reply to your patch earlier, but Linus already merged it for
3.3-final.