Message ID | 1341310879-5468-1-git-send-email-chenhui.zhao@freescale.com (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Kumar Gala |
Headers | show |
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.
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
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?
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
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.
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
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.
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>
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 --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