From: Zhao Chenhui <chenhui.zhao@freescale.com>
To: Scott Wood <scottwood@freescale.com>
Cc: <linuxppc-dev@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>,
<galak@kernel.crashing.org>, <leoli@freescale.com>
Subject: Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync
Date: Tue, 5 Jun 2012 17:08:31 +0800 [thread overview]
Message-ID: <20120605090831.GA21929@localhost.localdomain> (raw)
In-Reply-To: <4FC8E250.9090000@freescale.com>
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
next prev parent reply other threads:[~2012-06-05 9:08 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-11 11:53 [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync Zhao Chenhui
2012-05-11 11:53 ` [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support Zhao Chenhui
2012-06-01 21:27 ` Scott Wood
2012-06-04 11:04 ` Zhao Chenhui
2012-06-04 16:32 ` Scott Wood
2012-06-05 11:18 ` Zhao Chenhui
2012-06-05 16:15 ` Scott Wood
2012-06-06 9:59 ` Zhao Chenhui
2012-06-06 18:19 ` Scott Wood
2012-05-11 11:53 ` [PATCH v5 3/5] powerpc/85xx: add sleep and deep sleep support Zhao Chenhui
2012-06-01 21:54 ` Scott Wood
2012-06-04 11:12 ` Zhao Chenhui
2012-06-04 22:58 ` Scott Wood
2012-06-05 11:35 ` Zhao Chenhui
2012-06-05 16:13 ` Scott Wood
2012-05-11 11:53 ` [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source Zhao Chenhui
2012-06-01 22:08 ` Scott Wood
2012-06-04 11:36 ` Zhao Chenhui
2012-06-04 23:02 ` Scott Wood
2012-06-05 4:08 ` Li Yang-R58472
2012-06-05 16:11 ` Scott Wood
2012-06-05 16:49 ` Li Yang-R58472
2012-06-05 18:05 ` Scott Wood
2012-06-06 4:06 ` Li Yang
2012-06-06 18:29 ` Scott Wood
2012-06-07 4:10 ` Li Yang
2012-05-11 11:53 ` [PATCH v5 5/5] powerpc/85xx: add support to JOG feature using cpufreq interface Zhao Chenhui
2012-06-01 23:30 ` Scott Wood
2012-06-05 10:59 ` Zhao Chenhui
2012-06-05 15:58 ` Scott Wood
2012-06-06 10:19 ` Zhao Chenhui
2012-05-29 7:30 ` [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync Li Yang
2012-06-01 15:40 ` Scott Wood
2012-06-05 9:08 ` Zhao Chenhui [this message]
2012-06-05 16:07 ` Scott Wood
2012-06-06 9:31 ` Zhao Chenhui
2012-06-06 18:26 ` Scott Wood
2012-06-07 4:07 ` Zhao Chenhui
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120605090831.GA21929@localhost.localdomain \
--to=chenhui.zhao@freescale.com \
--cc=galak@kernel.crashing.org \
--cc=leoli@freescale.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=scottwood@freescale.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).