| Message ID | 1336737235-15370-1-git-send-email-chenhui.zhao@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
Hi Scott, Thanks for the valuable comment raised before and we have updated the patches accordingly. Please review the updated patch set and ACK if they are good to you. We hope it can be applied in this window. Leo On Fri, May 11, 2012 at 7:53 PM, 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> > --- > arch/powerpc/include/asm/fsl_guts.h | 2 + > arch/powerpc/platforms/85xx/smp.c | 93 +++++++++++++++++++++++++++++++++-- > 2 files changed, 91 insertions(+), 4 deletions(-) > > 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..6862dda 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> > @@ -115,13 +116,70 @@ smp_85xx_kick_cpu(int nr) > > struct smp_ops_t smp_85xx_ops = { > .kick_cpu = smp_85xx_kick_cpu, > -#ifdef CONFIG_KEXEC > - .give_timebase = smp_generic_give_timebase, > - .take_timebase = smp_generic_take_timebase, > -#endif > }; > > #ifdef CONFIG_KEXEC > +static struct ccsr_guts __iomem *guts; > +static u64 timebase; > +static int tb_req; > +static int tb_valid; > + > +static void mpc85xx_timebase_freeze(int freeze) > +{ > + unsigned int mask; > + > + if (!guts) > + return; > + > + 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); > + mb(); > + tb_valid = 0; > + > + local_irq_restore(flags); > +} > + > atomic_t kexec_down_cpus = ATOMIC_INIT(0); > > void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary) > @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr) > doorbell_setup_this_cpu(); > } > > +#ifdef CONFIG_KEXEC > +static const struct of_device_id guts_ids[] = { > + { .compatible = "fsl,mpc8572-guts", }, > + { .compatible = "fsl,mpc8560-guts", }, > + { .compatible = "fsl,mpc8536-guts", }, > + { .compatible = "fsl,p1020-guts", }, > + { .compatible = "fsl,p1021-guts", }, > + { .compatible = "fsl,p1022-guts", }, > + { .compatible = "fsl,p1023-guts", }, > + { .compatible = "fsl,p2020-guts", }, > + {}, > +}; > +#endif > + > void __init mpc85xx_smp_init(void) > { > struct device_node *np; > @@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void) > smp_85xx_ops.cause_ipi = doorbell_cause_ipi; > } > > +#ifdef CONFIG_KEXEC > + np = of_find_matching_node(NULL, guts_ids); > + if (np) { > + guts = of_iomap(np, 0); > + smp_85xx_ops.give_timebase = mpc85xx_give_timebase; > + smp_85xx_ops.take_timebase = mpc85xx_take_timebase; > + of_node_put(np); > + } else { > + smp_85xx_ops.give_timebase = smp_generic_give_timebase; > + smp_85xx_ops.take_timebase = smp_generic_take_timebase; > + } > +#endif > + > smp_ops = &smp_85xx_ops; > > #ifdef CONFIG_KEXEC > -- > 1.6.4.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Hi Kumar, There is no comment for these patches so far. Do you think these patches can be merged? We really want these patches to be merged in this merge window. Thanks. Best Regards, Chenhui > -----Original Message----- > From: Zhao Chenhui-B35336 > Sent: Friday, May 25, 2012 3:09 PM > To: Wood Scott-B07421; galak@kernel.crashing.org > Cc: Li Yang-R58472 > Subject: RE: [linuxppc-release] [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync > > Hi Scott and Kumar, > > Do you have comments for these patches? > > http://patchwork.ozlabs.org/patch/158484/ > http://patchwork.ozlabs.org/patch/158485/ > http://patchwork.ozlabs.org/patch/158487/ > http://patchwork.ozlabs.org/patch/158486/ > http://patchwork.ozlabs.org/patch/158488/ > > Thanks. > > Best Regards, > Chenhui > > > -----Original Message----- > > From: linuxppc-release-bounces@linux.freescale.net [mailto:linuxppc-release- > > bounces@linux.freescale.net] On Behalf Of Zhao Chenhui-B35336 > > Sent: Friday, May 11, 2012 7:54 PM > > To: linuxppc-dev@lists.ozlabs.org > > Cc: Wood Scott-B07421; Li Yang-R58472; linux-kernel@vger.kernel.org; galak@kernel.crashing.org > > Subject: [linuxppc-release] [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync > > > > 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> > > --- > > arch/powerpc/include/asm/fsl_guts.h | 2 + > > arch/powerpc/platforms/85xx/smp.c | 93 +++++++++++++++++++++++++++++++++-- > > 2 files changed, 91 insertions(+), 4 deletions(-) > > > > 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..6862dda 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> > > @@ -115,13 +116,70 @@ smp_85xx_kick_cpu(int nr) > > > > struct smp_ops_t smp_85xx_ops = { > > .kick_cpu = smp_85xx_kick_cpu, > > -#ifdef CONFIG_KEXEC > > - .give_timebase = smp_generic_give_timebase, > > - .take_timebase = smp_generic_take_timebase, > > -#endif > > }; > > > > #ifdef CONFIG_KEXEC > > +static struct ccsr_guts __iomem *guts; > > +static u64 timebase; > > +static int tb_req; > > +static int tb_valid; > > + > > +static void mpc85xx_timebase_freeze(int freeze) > > +{ > > + unsigned int mask; > > + > > + if (!guts) > > + return; > > + > > + 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); > > + mb(); > > + tb_valid = 0; > > + > > + local_irq_restore(flags); > > +} > > + > > atomic_t kexec_down_cpus = ATOMIC_INIT(0); > > > > void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary) > > @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr) > > doorbell_setup_this_cpu(); > > } > > > > +#ifdef CONFIG_KEXEC > > +static const struct of_device_id guts_ids[] = { > > + { .compatible = "fsl,mpc8572-guts", }, > > + { .compatible = "fsl,mpc8560-guts", }, > > + { .compatible = "fsl,mpc8536-guts", }, > > + { .compatible = "fsl,p1020-guts", }, > > + { .compatible = "fsl,p1021-guts", }, > > + { .compatible = "fsl,p1022-guts", }, > > + { .compatible = "fsl,p1023-guts", }, > > + { .compatible = "fsl,p2020-guts", }, > > + {}, > > +}; > > +#endif > > + > > void __init mpc85xx_smp_init(void) > > { > > struct device_node *np; > > @@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void) > > smp_85xx_ops.cause_ipi = doorbell_cause_ipi; > > } > > > > +#ifdef CONFIG_KEXEC > > + np = of_find_matching_node(NULL, guts_ids); > > + if (np) { > > + guts = of_iomap(np, 0); > > + smp_85xx_ops.give_timebase = mpc85xx_give_timebase; > > + smp_85xx_ops.take_timebase = mpc85xx_take_timebase; > > + of_node_put(np); > > + } else { > > + smp_85xx_ops.give_timebase = smp_generic_give_timebase; > > + smp_85xx_ops.take_timebase = smp_generic_take_timebase; > > + } > > +#endif > > + > > smp_ops = &smp_85xx_ops; > > > > #ifdef CONFIG_KEXEC > > -- > > 1.6.4.1 > > > > _______________________________________________ > > linuxppc-release mailing list > > linuxppc-release@linux.freescale.net > > http://linux.freescale.net/mailman/listinfo/linuxppc-release
On 05/11/2012 06:53 AM, Zhao Chenhui wrote: > #ifdef CONFIG_KEXEC > +static struct ccsr_guts __iomem *guts; > +static u64 timebase; > +static int tb_req; > +static int tb_valid; > + > +static void mpc85xx_timebase_freeze(int freeze) Why is this under CONFIG_KEXEC? It'll also be needed for CPU hotplug. > +{ > + unsigned int mask; > + > + if (!guts) > + return; > + > + 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); > + mb(); > + tb_valid = 0; > + > + local_irq_restore(flags); > +} I know you say this is for dual-core chips only, but it would be nice if you'd write this in a way that doesn't assume that (even if the corenet-specific timebase freezing comes later). Do we need an isync after setting the timebase, to ensure it's happened before we enable the timebase? Likewise, do we need a readback after disabling the timebase to ensure it's disabled before we read the timebase in give_timebase? > atomic_t kexec_down_cpus = ATOMIC_INIT(0); > > void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary) > @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr) > doorbell_setup_this_cpu(); > } > > +#ifdef CONFIG_KEXEC > +static const struct of_device_id guts_ids[] = { > + { .compatible = "fsl,mpc8572-guts", }, > + { .compatible = "fsl,mpc8560-guts", }, > + { .compatible = "fsl,mpc8536-guts", }, > + { .compatible = "fsl,p1020-guts", }, > + { .compatible = "fsl,p1021-guts", }, > + { .compatible = "fsl,p1022-guts", }, > + { .compatible = "fsl,p1023-guts", }, > + { .compatible = "fsl,p2020-guts", }, > + {}, > +}; > +#endif MPC8560 and MPC8536 are single-core... Also please use a more specific name, such as e500v2_smp_guts_ids or mpc85xx_smp_guts_ids -- when corenet support is added it will likely be in the same file. > void __init mpc85xx_smp_init(void) > { > struct device_node *np; > @@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void) > smp_85xx_ops.cause_ipi = doorbell_cause_ipi; > } > > +#ifdef CONFIG_KEXEC > + np = of_find_matching_node(NULL, guts_ids); > + if (np) { > + guts = of_iomap(np, 0); > + smp_85xx_ops.give_timebase = mpc85xx_give_timebase; > + smp_85xx_ops.take_timebase = mpc85xx_take_timebase; > + of_node_put(np); > + } else { > + smp_85xx_ops.give_timebase = smp_generic_give_timebase; > + smp_85xx_ops.take_timebase = smp_generic_take_timebase; > + } Do not use smp_generic_give/take_timebase, ever. If you don't have the guts node, then just assume the timebase is already synced. -Scott
On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote: > On 05/11/2012 06:53 AM, Zhao Chenhui wrote: > > #ifdef CONFIG_KEXEC > > +static struct ccsr_guts __iomem *guts; > > +static u64 timebase; > > +static int tb_req; > > +static int tb_valid; > > + > > +static void mpc85xx_timebase_freeze(int freeze) > > Why is this under CONFIG_KEXEC? It'll also be needed for CPU hotplug. Yes, the timebase sync is also needed for CPU hotplug, but this patch is unrelated to CPU hotplug. I added CONFIG_HOTPLUG_CPU in the next patch. > > > +{ > > + unsigned int mask; > > + > > + if (!guts) > > + return; > > + > > + 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); > > + mb(); > > + tb_valid = 0; > > + > > + local_irq_restore(flags); > > +} > > I know you say this is for dual-core chips only, but it would be nice if > you'd write this in a way that doesn't assume that (even if the > corenet-specific timebase freezing comes later). At this point, I have not thought about how to implement the cornet-specific timebase freezing. > > Do we need an isync after setting the timebase, to ensure it's happened > before we enable the timebase? Likewise, do we need a readback after > disabling the timebase to ensure it's disabled before we read the > timebase in give_timebase? I checked the e500 core manual (Chapter 2.16 Synchronization Requirements for SPRs). Only some SPR registers need an isync. The timebase registers do not. I did a readback in mpc85xx_timebase_freeze(). > > > atomic_t kexec_down_cpus = ATOMIC_INIT(0); > > > > void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary) > > @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr) > > doorbell_setup_this_cpu(); > > } > > > > +#ifdef CONFIG_KEXEC > > +static const struct of_device_id guts_ids[] = { > > + { .compatible = "fsl,mpc8572-guts", }, > > + { .compatible = "fsl,mpc8560-guts", }, > > + { .compatible = "fsl,mpc8536-guts", }, > > + { .compatible = "fsl,p1020-guts", }, > > + { .compatible = "fsl,p1021-guts", }, > > + { .compatible = "fsl,p1022-guts", }, > > + { .compatible = "fsl,p1023-guts", }, > > + { .compatible = "fsl,p2020-guts", }, > > + {}, > > +}; > > +#endif > > MPC8560 and MPC8536 are single-core... Thanks. I will remove them. > > Also please use a more specific name, such as e500v2_smp_guts_ids or > mpc85xx_smp_guts_ids -- when corenet support is added it will likely be > in the same file. > > > void __init mpc85xx_smp_init(void) > > { > > struct device_node *np; > > @@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void) > > smp_85xx_ops.cause_ipi = doorbell_cause_ipi; > > } > > > > +#ifdef CONFIG_KEXEC > > + np = of_find_matching_node(NULL, guts_ids); > > + if (np) { > > + guts = of_iomap(np, 0); > > + smp_85xx_ops.give_timebase = mpc85xx_give_timebase; > > + smp_85xx_ops.take_timebase = mpc85xx_take_timebase; > > + of_node_put(np); > > + } else { > > + smp_85xx_ops.give_timebase = smp_generic_give_timebase; > > + smp_85xx_ops.take_timebase = smp_generic_take_timebase; > > + } > > Do not use smp_generic_give/take_timebase, ever. If you don't have the > guts node, then just assume the timebase is already synced. > > -Scott smp_generic_give/take_timebase is the default in KEXEC before. If do not set them, it may make KEXEC fail on other platforms. -Chenhui
On 06/05/2012 04:08 AM, Zhao Chenhui wrote: > On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote: >> I know you say this is for dual-core chips only, but it would be nice if >> you'd write this in a way that doesn't assume that (even if the >> corenet-specific timebase freezing comes later). > > At this point, I have not thought about how to implement the cornet-specific timebase freezing. I wasn't asking you to. I was asking you to not have logic that breaks with more than 2 CPUs. >> Do we need an isync after setting the timebase, to ensure it's happened >> before we enable the timebase? Likewise, do we need a readback after >> disabling the timebase to ensure it's disabled before we read the >> timebase in give_timebase? > > I checked the e500 core manual (Chapter 2.16 Synchronization Requirements for SPRs). > Only some SPR registers need an isync. The timebase registers do not. I don't trust that, and the consequences of having the sync be imperfect are too unpleasant to chance it. > I did a readback in mpc85xx_timebase_freeze(). Sorry, missed that somehow. >>> +#ifdef CONFIG_KEXEC >>> + np = of_find_matching_node(NULL, guts_ids); >>> + if (np) { >>> + guts = of_iomap(np, 0); >>> + smp_85xx_ops.give_timebase = mpc85xx_give_timebase; >>> + smp_85xx_ops.take_timebase = mpc85xx_take_timebase; >>> + of_node_put(np); >>> + } else { >>> + smp_85xx_ops.give_timebase = smp_generic_give_timebase; >>> + smp_85xx_ops.take_timebase = smp_generic_take_timebase; >>> + } >> >> Do not use smp_generic_give/take_timebase, ever. If you don't have the >> guts node, then just assume the timebase is already synced. >> >> -Scott > > smp_generic_give/take_timebase is the default in KEXEC before. That was a mistake. > If do not set them, it may make KEXEC fail on other platforms. What platforms? -Scott
On Tue, Jun 05, 2012 at 11:07:41AM -0500, Scott Wood wrote: > On 06/05/2012 04:08 AM, Zhao Chenhui wrote: > > On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote: > >> I know you say this is for dual-core chips only, but it would be nice if > >> you'd write this in a way that doesn't assume that (even if the > >> corenet-specific timebase freezing comes later). > > > > At this point, I have not thought about how to implement the cornet-specific timebase freezing. > > I wasn't asking you to. I was asking you to not have logic that breaks > with more than 2 CPUs. These routines only called in the dual-core case. > > >> Do we need an isync after setting the timebase, to ensure it's happened > >> before we enable the timebase? Likewise, do we need a readback after > >> disabling the timebase to ensure it's disabled before we read the > >> timebase in give_timebase? > > > > I checked the e500 core manual (Chapter 2.16 Synchronization Requirements for SPRs). > > Only some SPR registers need an isync. The timebase registers do not. > > I don't trust that, and the consequences of having the sync be imperfect > are too unpleasant to chance it. > > > I did a readback in mpc85xx_timebase_freeze(). > > Sorry, missed that somehow. > > >>> +#ifdef CONFIG_KEXEC > >>> + np = of_find_matching_node(NULL, guts_ids); > >>> + if (np) { > >>> + guts = of_iomap(np, 0); > >>> + smp_85xx_ops.give_timebase = mpc85xx_give_timebase; > >>> + smp_85xx_ops.take_timebase = mpc85xx_take_timebase; > >>> + of_node_put(np); > >>> + } else { > >>> + smp_85xx_ops.give_timebase = smp_generic_give_timebase; > >>> + smp_85xx_ops.take_timebase = smp_generic_take_timebase; > >>> + } > >> > >> Do not use smp_generic_give/take_timebase, ever. If you don't have the > >> guts node, then just assume the timebase is already synced. > >> > >> -Scott > > > > smp_generic_give/take_timebase is the default in KEXEC before. > > That was a mistake. > > > If do not set them, it may make KEXEC fail on other platforms. > > What platforms? > > -Scott Such as P4080, P3041, etc. -Chenhui
On 06/06/2012 04:31 AM, Zhao Chenhui wrote: > On Tue, Jun 05, 2012 at 11:07:41AM -0500, Scott Wood wrote: >> On 06/05/2012 04:08 AM, Zhao Chenhui wrote: >>> On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote: >>>> I know you say this is for dual-core chips only, but it would be nice if >>>> you'd write this in a way that doesn't assume that (even if the >>>> corenet-specific timebase freezing comes later). >>> >>> At this point, I have not thought about how to implement the cornet-specific timebase freezing. >> >> I wasn't asking you to. I was asking you to not have logic that breaks >> with more than 2 CPUs. > > These routines only called in the dual-core case. Come on, you know we have chips with more than two cores. Why design such a limitation into it, just because you're not personally interested in supporting anything but e500v2? Is it so hard to make it work for an arbitrary number of cores? >>> If do not set them, it may make KEXEC fail on other platforms. >> >> What platforms? > > Such as P4080, P3041, etc. So we need to wait for corenet timebase sync before we stop causing problems in virtualization, simulators, etc. if a kernel has kexec or cpu hotplug enabled (whether used or not)? Can you at least make sure we're actually in a kexec/hotplug scenario at runtime? Or just implement corenet timebase sync -- it's not that different. -Scott
On Wed, Jun 06, 2012 at 01:26:16PM -0500, Scott Wood wrote: > On 06/06/2012 04:31 AM, Zhao Chenhui wrote: > > On Tue, Jun 05, 2012 at 11:07:41AM -0500, Scott Wood wrote: > >> On 06/05/2012 04:08 AM, Zhao Chenhui wrote: > >>> On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote: > >>>> I know you say this is for dual-core chips only, but it would be nice if > >>>> you'd write this in a way that doesn't assume that (even if the > >>>> corenet-specific timebase freezing comes later). > >>> > >>> At this point, I have not thought about how to implement the cornet-specific timebase freezing. > >> > >> I wasn't asking you to. I was asking you to not have logic that breaks > >> with more than 2 CPUs. > > > > These routines only called in the dual-core case. > > Come on, you know we have chips with more than two cores. Why design > such a limitation into it, just because you're not personally interested > in supporting anything but e500v2? > > Is it so hard to make it work for an arbitrary number of cores? > > >>> If do not set them, it may make KEXEC fail on other platforms. > >> > >> What platforms? > > > > Such as P4080, P3041, etc. > > So we need to wait for corenet timebase sync before we stop causing > problems in virtualization, simulators, etc. if a kernel has kexec or > cpu hotplug enabled (whether used or not)? > > Can you at least make sure we're actually in a kexec/hotplug scenario at > runtime? > > Or just implement corenet timebase sync -- it's not that different. > > -Scott We also work on the corenet timebase sync. Our plan is first the dual-core case, then the case of more than 2 cores. We will submit the corenet timebase sync patch soon. -Chenhui
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..6862dda 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> @@ -115,13 +116,70 @@ smp_85xx_kick_cpu(int nr) struct smp_ops_t smp_85xx_ops = { .kick_cpu = smp_85xx_kick_cpu, -#ifdef CONFIG_KEXEC - .give_timebase = smp_generic_give_timebase, - .take_timebase = smp_generic_take_timebase, -#endif }; #ifdef CONFIG_KEXEC +static struct ccsr_guts __iomem *guts; +static u64 timebase; +static int tb_req; +static int tb_valid; + +static void mpc85xx_timebase_freeze(int freeze) +{ + unsigned int mask; + + if (!guts) + return; + + 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); + mb(); + tb_valid = 0; + + local_irq_restore(flags); +} + atomic_t kexec_down_cpus = ATOMIC_INIT(0); void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary) @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr) doorbell_setup_this_cpu(); } +#ifdef CONFIG_KEXEC +static const struct of_device_id guts_ids[] = { + { .compatible = "fsl,mpc8572-guts", }, + { .compatible = "fsl,mpc8560-guts", }, + { .compatible = "fsl,mpc8536-guts", }, + { .compatible = "fsl,p1020-guts", }, + { .compatible = "fsl,p1021-guts", }, + { .compatible = "fsl,p1022-guts", }, + { .compatible = "fsl,p1023-guts", }, + { .compatible = "fsl,p2020-guts", }, + {}, +}; +#endif + void __init mpc85xx_smp_init(void) { struct device_node *np; @@ -249,6 +321,19 @@ void __init mpc85xx_smp_init(void) smp_85xx_ops.cause_ipi = doorbell_cause_ipi; } +#ifdef CONFIG_KEXEC + np = of_find_matching_node(NULL, guts_ids); + if (np) { + guts = of_iomap(np, 0); + smp_85xx_ops.give_timebase = mpc85xx_give_timebase; + smp_85xx_ops.take_timebase = mpc85xx_take_timebase; + of_node_put(np); + } else { + smp_85xx_ops.give_timebase = smp_generic_give_timebase; + smp_85xx_ops.take_timebase = smp_generic_take_timebase; + } +#endif + smp_ops = &smp_85xx_ops; #ifdef CONFIG_KEXEC