diff mbox

[v7,1/5] powerpc/85xx: implement hardware timebase sync

Message ID 1341310879-5468-1-git-send-email-chenhui.zhao@freescale.com (mailing list archive)
State Accepted, archived
Delegated to: Kumar Gala
Headers show

Commit Message

chenhui zhao July 3, 2012, 10:21 a.m. UTC 
Do hardware timebase sync. Firstly, stop all timebases, and transfer
the timebase value of the boot core to the other core. Finally,
start all timebases.

Only apply to dual-core chips, such as MPC8572, P2020, etc.

Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
v7:
 * removed CONFIG_85xx_TB_SYNC
 * incorporated Timur's comments

 arch/powerpc/include/asm/fsl_guts.h |    2 +
 arch/powerpc/platforms/85xx/smp.c   |   82 +++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 0 deletions(-)

Comments

Tabi Timur-B04825 July 3, 2012, 12:46 p.m. UTC | #1 
On Tue, Jul 3, 2012 at 5:21 AM, Zhao Chenhui <chenhui.zhao@freescale.com> wrote:

> +       np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
> +       if (np) {
> +               guts = of_iomap(np, 0);
> +               of_node_put(np);
> +               if (!guts) {
> +                       pr_err("%s: Could not map guts node address\n",
> +                                                               __func__);
> +                       return;
> +               }
> +               smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> +               smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
> +       }

I had this in mind:

               guts = of_iomap(np, 0);
               of_node_put(np);
               if (guts) {
                       smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
                       smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
                } else {
                       pr_err("%s: Could not map guts node address\n",
                                                               __func__);
               }

That way, a missing GUTS node does not break everything.
chenhui zhao July 4, 2012, 3:14 a.m. UTC | #2 
On Tue, Jul 03, 2012 at 07:46:24AM -0500, Tabi Timur-B04825 wrote:
> On Tue, Jul 3, 2012 at 5:21 AM, Zhao Chenhui <chenhui.zhao@freescale.com> wrote:
> 
> > +       np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
> > +       if (np) {
> > +               guts = of_iomap(np, 0);
> > +               of_node_put(np);
> > +               if (!guts) {
> > +                       pr_err("%s: Could not map guts node address\n",
> > +                                                               __func__);
> > +                       return;
> > +               }
> > +               smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> > +               smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
> > +       }
> 
> I had this in mind:
> 
>                guts = of_iomap(np, 0);
>                of_node_put(np);
>                if (guts) {
>                        smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
>                        smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
>                 } else {
>                        pr_err("%s: Could not map guts node address\n",
>                                                                __func__);
>                }
> 
> That way, a missing GUTS node does not break everything.
> 

If the guts variable is NULL, it indicates there is error in dts or kernel.
We should fix the error, rather than ignore it.

Moreover, if smp_85xx_ops.give/take_timebase is NULL, kernel can not do the timebase sync.

-Chenhui
Tabi Timur-B04825 July 4, 2012, 3:17 a.m. UTC | #3 
Zhao Chenhui wrote:
> If the guts variable is NULL, it indicates there is error in dts or kernel.
> We should fix the error, rather than ignore it.

And that's why there's a warning message.  Crashing the kernel is not 
going to fix anything.

> Moreover, if smp_85xx_ops.give/take_timebase is NULL, kernel can not do the timebase sync.

Is that necessary for the kernel to boot?
chenhui zhao July 4, 2012, 3:45 a.m. UTC | #4 
On Tue, Jul 03, 2012 at 10:17:12PM -0500, Tabi Timur-B04825 wrote:
> Zhao Chenhui wrote:
> > If the guts variable is NULL, it indicates there is error in dts or kernel.
> > We should fix the error, rather than ignore it.
> 
> And that's why there's a warning message.  Crashing the kernel is not 
> going to fix anything.
> 

This error likely crashes the kenel somewhere.

> > Moreover, if smp_85xx_ops.give/take_timebase is NULL, kernel can not do the timebase sync.
> 
> Is that necessary for the kernel to boot?
> 

No. But in the cpu hotplug context, we need do the timebase sync.

-Chenhui
Tabi Timur-B04825 July 4, 2012, 3:19 p.m. UTC | #5 
Zhao Chenhui wrote:
> On Tue, Jul 03, 2012 at 10:17:12PM -0500, Tabi Timur-B04825 wrote:
>> Zhao Chenhui wrote:
>>> If the guts variable is NULL, it indicates there is error in dts or kernel.
>>> We should fix the error, rather than ignore it.
>>
>> And that's why there's a warning message.  Crashing the kernel is not
>> going to fix anything.
>>
>
> This error likely crashes the kenel somewhere.

Can you test this, please?

The point I'm trying to make is that it's wrong to intentionally halt the 
kernel unless you're sure that it's the best option.  A missing device 
tree node is supposed to only disable a given feature, not break everything.
chenhui zhao July 5, 2012, 10:25 a.m. UTC | #6 
On Wed, Jul 04, 2012 at 10:19:54AM -0500, Tabi Timur-B04825 wrote:
> Zhao Chenhui wrote:
> > On Tue, Jul 03, 2012 at 10:17:12PM -0500, Tabi Timur-B04825 wrote:
> >> Zhao Chenhui wrote:
> >>> If the guts variable is NULL, it indicates there is error in dts or kernel.
> >>> We should fix the error, rather than ignore it.
> >>
> >> And that's why there's a warning message.  Crashing the kernel is not
> >> going to fix anything.
> >>
> >
> > This error likely crashes the kenel somewhere.
> 
> Can you test this, please?
> 
> The point I'm trying to make is that it's wrong to intentionally halt the 
> kernel unless you're sure that it's the best option.  A missing device 
> tree node is supposed to only disable a given feature, not break everything.
> 

I think there is some misunderstanding here.

	np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
	if (np) {
		guts = of_iomap(np, 0);
		of_node_put(np);
		if (!guts) {
			pr_err("%s: Could not map guts node address\n",
								__func__);
			return;
		}
		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
	}

If the guts node is missing, this code snippet will be skipped. If the guts node is existed,
the return value of of_iomap(), namely guts, will be tested. If it is NULL, it shows
that there is error in dts, or the ioremap() in of_iomap() failed. I think
these errors are fatal errors, so I print an error info and return.

-Chenhui
Tabi Timur-B04825 July 5, 2012, 3:30 p.m. UTC | #7 
Zhao Chenhui wrote:
> If the guts node is missing, this code snippet will be skipped. If the guts node is existed,
> the return value of of_iomap(), namely guts, will be tested. If it is NULL, it shows
> that there is error in dts, or the ioremap() in of_iomap() failed. I think
> these errors are fatal errors, so I print an error info and return.

Ok, I see your point now.  I'm concerned about what might happen if
someone else updates mpc85xx_smp_init() in the future, but there's nothing
actually wrong with your patch today.
Tabi Timur-B04825 July 5, 2012, 3:31 p.m. UTC | #8 
On Tue, Jul 3, 2012 at 5:21 AM, Zhao Chenhui <chenhui.zhao@freescale.com> wrote:
> Do hardware timebase sync. Firstly, stop all timebases, and transfer
> the timebase value of the boot core to the other core. Finally,
> start all timebases.
>
> Only apply to dual-core chips, such as MPC8572, P2020, etc.
>
> Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>

Acked-by: Timur Tabi <timur@freescale.com>
Scott Wood July 5, 2012, 5:11 p.m. UTC | #9 
On 07/03/2012 10:45 PM, Zhao Chenhui wrote:
> On Tue, Jul 03, 2012 at 10:17:12PM -0500, Tabi Timur-B04825 wrote:
>> Zhao Chenhui wrote:
>>> If the guts variable is NULL, it indicates there is error in dts or kernel.
>>> We should fix the error, rather than ignore it.
>>
>> And that's why there's a warning message.  Crashing the kernel is not 
>> going to fix anything.
>>
> 
> This error likely crashes the kenel somewhere.

Only if you're doing cpu hotplug or kexec.

-Scott
diff mbox

Patch

diff --git a/arch/powerpc/include/asm/fsl_guts.h b/arch/powerpc/include/asm/fsl_guts.h
index aa4c488..dd5ba2c 100644
--- a/arch/powerpc/include/asm/fsl_guts.h
+++ b/arch/powerpc/include/asm/fsl_guts.h
@@ -48,6 +48,8 @@  struct ccsr_guts {
         __be32  dmuxcr;		/* 0x.0068 - DMA Mux Control Register */
         u8	res06c[0x70 - 0x6c];
 	__be32	devdisr;	/* 0x.0070 - Device Disable Control */
+#define CCSR_GUTS_DEVDISR_TB1	0x00001000
+#define CCSR_GUTS_DEVDISR_TB0	0x00004000
 	__be32	devdisr2;	/* 0x.0074 - Device Disable Control 2 */
 	u8	res078[0x7c - 0x78];
 	__be32  pmjcr;		/* 0x.007c - 4 Power Management Jog Control Register */
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index ff42490..2e65fe8 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -24,6 +24,7 @@ 
 #include <asm/mpic.h>
 #include <asm/cacheflush.h>
 #include <asm/dbell.h>
+#include <asm/fsl_guts.h>
 
 #include <sysdev/fsl_soc.h>
 #include <sysdev/mpic.h>
@@ -42,6 +43,64 @@  extern void __early_start(void);
 #define NUM_BOOT_ENTRY		8
 #define SIZE_BOOT_ENTRY		(NUM_BOOT_ENTRY * sizeof(u32))
 
+static struct ccsr_guts __iomem *guts;
+static u64 timebase;
+static int tb_req;
+static int tb_valid;
+
+static void mpc85xx_timebase_freeze(int freeze)
+{
+	uint32_t mask;
+
+	mask = CCSR_GUTS_DEVDISR_TB0 | CCSR_GUTS_DEVDISR_TB1;
+	if (freeze)
+		setbits32(&guts->devdisr, mask);
+	else
+		clrbits32(&guts->devdisr, mask);
+
+	in_be32(&guts->devdisr);
+}
+
+static void mpc85xx_give_timebase(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+
+	while (!tb_req)
+		barrier();
+	tb_req = 0;
+
+	mpc85xx_timebase_freeze(1);
+	timebase = get_tb();
+	mb();
+	tb_valid = 1;
+
+	while (tb_valid)
+		barrier();
+
+	mpc85xx_timebase_freeze(0);
+
+	local_irq_restore(flags);
+}
+
+static void mpc85xx_take_timebase(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+
+	tb_req = 1;
+	while (!tb_valid)
+		barrier();
+
+	set_tb(timebase >> 32, timebase & 0xffffffff);
+	isync();
+	tb_valid = 0;
+
+	local_irq_restore(flags);
+}
+
 static int __init
 smp_85xx_kick_cpu(int nr)
 {
@@ -228,6 +287,16 @@  smp_85xx_setup_cpu(int cpu_nr)
 		doorbell_setup_this_cpu();
 }
 
+static const struct of_device_id mpc85xx_smp_guts_ids[] = {
+	{ .compatible = "fsl,mpc8572-guts", },
+	{ .compatible = "fsl,p1020-guts", },
+	{ .compatible = "fsl,p1021-guts", },
+	{ .compatible = "fsl,p1022-guts", },
+	{ .compatible = "fsl,p1023-guts", },
+	{ .compatible = "fsl,p2020-guts", },
+	{},
+};
+
 void __init mpc85xx_smp_init(void)
 {
 	struct device_node *np;
@@ -249,6 +318,19 @@  void __init mpc85xx_smp_init(void)
 		smp_85xx_ops.cause_ipi = doorbell_cause_ipi;
 	}
 
+	np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
+	if (np) {
+		guts = of_iomap(np, 0);
+		of_node_put(np);
+		if (!guts) {
+			pr_err("%s: Could not map guts node address\n",
+								__func__);
+			return;
+		}
+		smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
+		smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
+	}
+
 	smp_ops = &smp_85xx_ops;
 
 #ifdef CONFIG_KEXEC