Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Russell,
On Wed, Aug 13, 2014 at 04:47:06PM -0700, Brian Norris wrote:
> Picking up this thread again, as things are now set for dropping this
> patch and resubmitting SMP support for 3.18.
Any further comment on this? I'd like to submit v10 of this patch soon.
As of now, my patch will still look essentially the same (with only
minor changes for some of the review comments), since I'm convinced
neither that my current per-cpu flag-based solution is unsound nor that
a delayed work queue can solve all of the same problems.
> On Sat, Aug 02, 2014 at 10:27:56AM +0100, Russell King wrote:
> > On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote:
> > > Yes, I noticed this. What I meant is that smp_ops.cpu_die() and
> > > smp_ops.cpu_kill() are not synchronized.
> > ...
> > > We're not relying on the L1 cache, though. Don't sync_cache_{r,w}()
> > > ensure all reads/writes reach at least the L2?
> >
> > What exactly are you trying to achieve here?
>
> Synchronization between v7_exit_coherency_flush() (on the dying CPU) and
> yanking the power (brcmstb_cpu_kill(), on the murderous CPU). The core
> completion-based synchronization is not sufficient, since it allows
> brcmstb_smp_ops.smp_kill and brcmstb_smp_ops.smp_die to race.
>
> Am I somehow not achieving what I intend here?
>
> > > How does that ensure that the CPU is down by the time the work is
> > > scheduled? It seems like this would just defer the work long enough that
> > > it *most likely* has quiesced, but I don't see how this gives us a
> > > better guarantee. Or maybe I'm missing something. (If so, please do
> > > enlighten!)
> >
> > Note that I said a delayed work queue. The dying CPU runs a predictable
> > sequence once cpu_die() has been entered - interrupts at the GIC have
> > been programmed to be routed to other CPUs, interrupts at the CPU are
> > masked, so the CPU isn't going to be doing anything else except executing
> > that code path. So, it's going to be a predictable number of CPU cycles.
> >
> > That allows you to arrange a delayed workqueue (or a timer) to fire
> > after a period of time that you can guarantee that the dying CPU has
> > reached that wfi().
>
> OK, that sounds workable for the active hotplug case.
>
> But what about for the suspend case? CPUs are hot-unplugged during
> disable_nonboot_cpus(), and I don't see that this would guarantee the
> workqueue will complete before we enter suspend.
>
> > Another point which raises itself in your patch is this:
> >
> > + /* Settle-time from Broadcom-internal DVT reference code */
> > + udelay(7);
> >
[...]
> I'm looking into this specific delay. [...]
BTW, I think this delay can be dropped. It was retained from an early
non-production release of this CPU.
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Russell,
On Wed, Aug 13, 2014 at 04:47:06PM -0700, Brian Norris wrote:
Picking up this thread again, as things are now set for dropping this
patch and resubmitting SMP support for 3.18.
Any further comment on this? I'd like to submit v10 of this patch soon.
As of now, my patch will still look essentially the same (with only
minor changes for some of the review comments), since I'm convinced
neither that my current per-cpu flag-based solution is unsound nor that
a delayed work queue can solve all of the same problems.
On Sat, Aug 02, 2014 at 10:27:56AM +0100, Russell King wrote:
On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote:
Yes, I noticed this. What I meant is that smp_ops.cpu_die() and
smp_ops.cpu_kill() are not synchronized.
...
We're not relying on the L1 cache, though. Don't sync_cache_{r,w}()
ensure all reads/writes reach at least the L2?
What exactly are you trying to achieve here?
Synchronization between v7_exit_coherency_flush() (on the dying CPU) and
yanking the power (brcmstb_cpu_kill(), on the murderous CPU). The core
completion-based synchronization is not sufficient, since it allows
brcmstb_smp_ops.smp_kill and brcmstb_smp_ops.smp_die to race.
Am I somehow not achieving what I intend here?
How does that ensure that the CPU is down by the time the work is
scheduled? It seems like this would just defer the work long enough that
it *most likely* has quiesced, but I don't see how this gives us a
better guarantee. Or maybe I'm missing something. (If so, please do
enlighten!)
Note that I said a delayed work queue. The dying CPU runs a predictable
sequence once cpu_die() has been entered - interrupts at the GIC have
been programmed to be routed to other CPUs, interrupts at the CPU are
masked, so the CPU isn't going to be doing anything else except executing
that code path. So, it's going to be a predictable number of CPU cycles.
That allows you to arrange a delayed workqueue (or a timer) to fire
after a period of time that you can guarantee that the dying CPU has
reached that wfi().
OK, that sounds workable for the active hotplug case.
But what about for the suspend case? CPUs are hot-unplugged during
disable_nonboot_cpus(), and I don't see that this would guarantee the
workqueue will complete before we enter suspend.
Another point which raises itself in your patch is this:
+ /* Settle-time from Broadcom-internal DVT reference code */
+ udelay(7);
[...]
I'm looking into this specific delay. [...]
BTW, I think this delay can be dropped. It was retained from an early
non-production release of this CPU.
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Russell,
Picking up this thread again, as things are now set for dropping this
patch and resubmitting SMP support for 3.18.
On Sat, Aug 02, 2014 at 10:27:56AM +0100, Russell King wrote:
> On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote:
> > Yes, I noticed this. What I meant is that smp_ops.cpu_die() and
> > smp_ops.cpu_kill() are not synchronized.
> ...
> > We're not relying on the L1 cache, though. Don't sync_cache_{r,w}()
> > ensure all reads/writes reach at least the L2?
>
> What exactly are you trying to achieve here?
Synchronization between v7_exit_coherency_flush() (on the dying CPU) and
yanking the power (brcmstb_cpu_kill(), on the murderous CPU). The core
completion-based synchronization is not sufficient, since it allows
brcmstb_smp_ops.smp_kill and brcmstb_smp_ops.smp_die to race.
Am I somehow not achieving what I intend here?
> > How does that ensure that the CPU is down by the time the work is
> > scheduled? It seems like this would just defer the work long enough that
> > it *most likely* has quiesced, but I don't see how this gives us a
> > better guarantee. Or maybe I'm missing something. (If so, please do
> > enlighten!)
>
> Note that I said a delayed work queue. The dying CPU runs a predictable
> sequence once cpu_die() has been entered - interrupts at the GIC have
> been programmed to be routed to other CPUs, interrupts at the CPU are
> masked, so the CPU isn't going to be doing anything else except executing
> that code path. So, it's going to be a predictable number of CPU cycles.
>
> That allows you to arrange a delayed workqueue (or a timer) to fire
> after a period of time that you can guarantee that the dying CPU has
> reached that wfi().
OK, that sounds workable for the active hotplug case.
But what about for the suspend case? CPUs are hot-unplugged during
disable_nonboot_cpus(), and I don't see that this would guarantee the
workqueue will complete before we enter suspend.
> Another point which raises itself in your patch is this:
>
> + /* Settle-time from Broadcom-internal DVT reference code */
> + udelay(7);
>
> 7us looks very precise, but udelay() may not be that precise. What is
> the actual specification? Does it say "you must wait at least 7us"?
>
> udelay() _may_ return early, especially if it is using the CPU delay
> loop to perform the delay - I've explained why this happens previously,
> and why it isn't a bug.
>
> If you're using a timer-based delay for udelay() (which you should be
> using if you support cpufreq) then the delay should be more accurate,
> but it's still good practise to give a little leeway on the figure.
I'm looking into this specific delay. I'd bet it's just "wait at least
7us." I could probably factor in some leeway to be safe.
Thanks,
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Russell,
Picking up this thread again, as things are now set for dropping this
patch and resubmitting SMP support for 3.18.
On Sat, Aug 02, 2014 at 10:27:56AM +0100, Russell King wrote:
On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote:
Yes, I noticed this. What I meant is that smp_ops.cpu_die() and
smp_ops.cpu_kill() are not synchronized.
...
We're not relying on the L1 cache, though. Don't sync_cache_{r,w}()
ensure all reads/writes reach at least the L2?
What exactly are you trying to achieve here?
Synchronization between v7_exit_coherency_flush() (on the dying CPU) and
yanking the power (brcmstb_cpu_kill(), on the murderous CPU). The core
completion-based synchronization is not sufficient, since it allows
brcmstb_smp_ops.smp_kill and brcmstb_smp_ops.smp_die to race.
Am I somehow not achieving what I intend here?
How does that ensure that the CPU is down by the time the work is
scheduled? It seems like this would just defer the work long enough that
it *most likely* has quiesced, but I don't see how this gives us a
better guarantee. Or maybe I'm missing something. (If so, please do
enlighten!)
Note that I said a delayed work queue. The dying CPU runs a predictable
sequence once cpu_die() has been entered - interrupts at the GIC have
been programmed to be routed to other CPUs, interrupts at the CPU are
masked, so the CPU isn't going to be doing anything else except executing
that code path. So, it's going to be a predictable number of CPU cycles.
That allows you to arrange a delayed workqueue (or a timer) to fire
after a period of time that you can guarantee that the dying CPU has
reached that wfi().
OK, that sounds workable for the active hotplug case.
But what about for the suspend case? CPUs are hot-unplugged during
disable_nonboot_cpus(), and I don't see that this would guarantee the
workqueue will complete before we enter suspend.
Another point which raises itself in your patch is this:
+ /* Settle-time from Broadcom-internal DVT reference code */
+ udelay(7);
7us looks very precise, but udelay() may not be that precise. What is
the actual specification? Does it say you must wait at least 7us?
udelay() _may_ return early, especially if it is using the CPU delay
loop to perform the delay - I've explained why this happens previously,
and why it isn't a bug.
If you're using a timer-based delay for udelay() (which you should be
using if you support cpufreq) then the delay should be more accurate,
but it's still good practise to give a little leeway on the figure.
I'm looking into this specific delay. I'd bet it's just wait at least
7us. I could probably factor in some leeway to be safe.
Thanks,
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Sat, Aug 02, 2014 at 09:19:24AM +0100, Russell King wrote:
> Here's some more comments on this.
>
> On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:
> > +static void brcmstb_cpu_die(u32 cpu)
> > +{
> > + v7_exit_coherency_flush(all);
>
> This is ultimately what causes my builds to break:
>
> /tmp/ccSPowmq.s:171: Error: selected processor does not support ARM mode `isb
> '
> /tmp/ccSPowmq.s:177: Error: selected processor does not support ARM mode `isb
> '
> /tmp/ccSPowmq.s:178: Error: selected processor does not support ARM mode `dsb
> '
> make[2]: *** [arch/arm/mach-bcm/platsmp-brcmstb.o] Error 1
>
> It seems that v7_exit_coherency_flush() can only be used with code which
> is ARMv7 only.
Yes, I noticed this already, and I proposed a solution:
http://article.gmane.org/gmane.linux.drivers.devicetree/84517
> > + /* Prevent all interrupts from reaching this CPU. */
> > + arch_local_irq_disable();
>
> Why do you think it is necessary to disable interrupts here? Where
> have they been re-enabled since this bit of generic code:
>
> void __ref cpu_die(void)
> {
> unsigned int cpu = smp_processor_id();
>
> idle_task_exit();
>
> local_irq_disable();
>
> and why arch_local_irq_disable() at that? Even if interrupts were
> enabled prior to your call to arch_local_irq_disable(), what do you
> think would be the effect of receiving an interrupt after you've
> exited coherency?
This mistake was already noted. No need for the extra IRQ disable.
(http://article.gmane.org/gmane.linux.drivers.devicetree/84516)
> > +
> > + /*
> > +* Final full barrier to ensure everything before this instruction has
> > +* quiesced.
> > +*/
> > + isb();
> > + dsb();
>
> If the call to arch_local_irq_disable() is removed, and
> v7_exit_coherency_flush() is fixed, then this is not required, because
> v7_exit_coherency_flush() already does this at the very end.
Right. Will drop.
> > +
> > + per_cpu_sw_state_wr(cpu, 0);
> > +
> > + /* Sit and wait to die */
> > + wfi();
> > +
> > + /* We should never get here... */
> > + panic("Spurious interrupt on CPU %d received!\n", cpu);
>
> You really should /not/ be calling panic here, because that uses data
> shared with the CPUs which are still coherent. This is akin to doing
> DMA into bits of the kernel space without dealing with the cache
> coherency issues.
OK.
> Moreover, if you read the comments on
> v7_exit_coherency_flush() about ldrex/strex, which are two instructions
> spinlocks use, you'll see that ldrex/strex must not be executed, which
> means you can't call any function which uses spinlocks. That rules
> out printk() et.al. printascii is fine, but that's only available when
> the low level debug stuff is enabled.
OK, so I'll drop the panic(). printascii doesn't look extremely useful,
but I suppose we could use it for debugging. Seems like a while (1) loop
might be a suitable replacement. If we get this far, we'll likely get
locked up trying to power this CPU off anyway, so it'll be apparent that
there was power-down failure.
Thanks,
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Sat, Aug 02, 2014 at 09:19:24AM +0100, Russell King wrote:
Here's some more comments on this.
On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:
+static void brcmstb_cpu_die(u32 cpu)
+{
+ v7_exit_coherency_flush(all);
This is ultimately what causes my builds to break:
/tmp/ccSPowmq.s:171: Error: selected processor does not support ARM mode `isb
'
/tmp/ccSPowmq.s:177: Error: selected processor does not support ARM mode `isb
'
/tmp/ccSPowmq.s:178: Error: selected processor does not support ARM mode `dsb
'
make[2]: *** [arch/arm/mach-bcm/platsmp-brcmstb.o] Error 1
It seems that v7_exit_coherency_flush() can only be used with code which
is ARMv7 only.
Yes, I noticed this already, and I proposed a solution:
http://article.gmane.org/gmane.linux.drivers.devicetree/84517
+ /* Prevent all interrupts from reaching this CPU. */
+ arch_local_irq_disable();
Why do you think it is necessary to disable interrupts here? Where
have they been re-enabled since this bit of generic code:
void __ref cpu_die(void)
{
unsigned int cpu = smp_processor_id();
idle_task_exit();
local_irq_disable();
and why arch_local_irq_disable() at that? Even if interrupts were
enabled prior to your call to arch_local_irq_disable(), what do you
think would be the effect of receiving an interrupt after you've
exited coherency?
This mistake was already noted. No need for the extra IRQ disable.
(http://article.gmane.org/gmane.linux.drivers.devicetree/84516)
+
+ /*
+* Final full barrier to ensure everything before this instruction has
+* quiesced.
+*/
+ isb();
+ dsb();
If the call to arch_local_irq_disable() is removed, and
v7_exit_coherency_flush() is fixed, then this is not required, because
v7_exit_coherency_flush() already does this at the very end.
Right. Will drop.
+
+ per_cpu_sw_state_wr(cpu, 0);
+
+ /* Sit and wait to die */
+ wfi();
+
+ /* We should never get here... */
+ panic(Spurious interrupt on CPU %d received!\n, cpu);
You really should /not/ be calling panic here, because that uses data
shared with the CPUs which are still coherent. This is akin to doing
DMA into bits of the kernel space without dealing with the cache
coherency issues.
OK.
Moreover, if you read the comments on
v7_exit_coherency_flush() about ldrex/strex, which are two instructions
spinlocks use, you'll see that ldrex/strex must not be executed, which
means you can't call any function which uses spinlocks. That rules
out printk() et.al. printascii is fine, but that's only available when
the low level debug stuff is enabled.
OK, so I'll drop the panic(). printascii doesn't look extremely useful,
but I suppose we could use it for debugging. Seems like a while (1) loop
might be a suitable replacement. If we get this far, we'll likely get
locked up trying to power this CPU off anyway, so it'll be apparent that
there was power-down failure.
Thanks,
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote:
> Hi Russell,
>
> Yes, I noticed this. What I meant is that smp_ops.cpu_die() and
> smp_ops.cpu_kill() are not synchronized.
...
> We're not relying on the L1 cache, though. Don't sync_cache_{r,w}()
> ensure all reads/writes reach at least the L2?
What exactly are you trying to achieve here?
> How does that ensure that the CPU is down by the time the work is
> scheduled? It seems like this would just defer the work long enough that
> it *most likely* has quiesced, but I don't see how this gives us a
> better guarantee. Or maybe I'm missing something. (If so, please do
> enlighten!)
Note that I said a delayed work queue. The dying CPU runs a predictable
sequence once cpu_die() has been entered - interrupts at the GIC have
been programmed to be routed to other CPUs, interrupts at the CPU are
masked, so the CPU isn't going to be doing anything else except executing
that code path. So, it's going to be a predictable number of CPU cycles.
That allows you to arrange a delayed workqueue (or a timer) to fire
after a period of time that you can guarantee that the dying CPU has
reached that wfi().
Another point which raises itself in your patch is this:
+ /* Settle-time from Broadcom-internal DVT reference code */
+ udelay(7);
7us looks very precise, but udelay() may not be that precise. What is
the actual specification? Does it say "you must wait at least 7us"?
udelay() _may_ return early, especially if it is using the CPU delay
loop to perform the delay - I've explained why this happens previously,
and why it isn't a bug.
If you're using a timer-based delay for udelay() (which you should be
using if you support cpufreq) then the delay should be more accurate,
but it's still good practise to give a little leeway on the figure.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Fri, Aug 01, 2014 at 09:33:44AM -0500, Rob Herring wrote: > On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris > wrote: > > Like what? > > Errata work-arounds, performance bits, etc. I don't recall exactly > which ones are per core vs. global. I assume this is an A9, but cores > with virt extensions would also need to drop to non-secure mode. The kernel's bringup just before enabling the CPU deals with that stuff provided your CPU is running in secure mode, but you are right - we /do/ want to encourage platforms to do the errata fixups before they call the kernel via any entry point. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Here's some more comments on this.
On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:
> +static void brcmstb_cpu_die(u32 cpu)
> +{
> + v7_exit_coherency_flush(all);
This is ultimately what causes my builds to break:
/tmp/ccSPowmq.s:171: Error: selected processor does not support ARM mode `isb '
/tmp/ccSPowmq.s:177: Error: selected processor does not support ARM mode `isb '
/tmp/ccSPowmq.s:178: Error: selected processor does not support ARM mode `dsb '
make[2]: *** [arch/arm/mach-bcm/platsmp-brcmstb.o] Error 1
It seems that v7_exit_coherency_flush() can only be used with code which
is ARMv7 only.
> + /* Prevent all interrupts from reaching this CPU. */
> + arch_local_irq_disable();
Why do you think it is necessary to disable interrupts here? Where
have they been re-enabled since this bit of generic code:
void __ref cpu_die(void)
{
unsigned int cpu = smp_processor_id();
idle_task_exit();
local_irq_disable();
and why arch_local_irq_disable() at that? Even if interrupts were
enabled prior to your call to arch_local_irq_disable(), what do you
think would be the effect of receiving an interrupt after you've
exited coherency?
> +
> + /*
> + * Final full barrier to ensure everything before this instruction has
> + * quiesced.
> + */
> + isb();
> + dsb();
If the call to arch_local_irq_disable() is removed, and
v7_exit_coherency_flush() is fixed, then this is not required, because
v7_exit_coherency_flush() already does this at the very end.
> +
> + per_cpu_sw_state_wr(cpu, 0);
> +
> + /* Sit and wait to die */
> + wfi();
> +
> + /* We should never get here... */
> + panic("Spurious interrupt on CPU %d received!\n", cpu);
You really should /not/ be calling panic here, because that uses data
shared with the CPUs which are still coherent. This is akin to doing
DMA into bits of the kernel space without dealing with the cache
coherency issues. Moreover, if you read the comments on
v7_exit_coherency_flush() about ldrex/strex, which are two instructions
spinlocks use, you'll see that ldrex/strex must not be executed, which
means you can't call any function which uses spinlocks. That rules
out printk() et.al. printascii is fine, but that's only available when
the low level debug stuff is enabled.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Here's some more comments on this.
On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:
+static void brcmstb_cpu_die(u32 cpu)
+{
+ v7_exit_coherency_flush(all);
This is ultimately what causes my builds to break:
/tmp/ccSPowmq.s:171: Error: selected processor does not support ARM mode `isb '
/tmp/ccSPowmq.s:177: Error: selected processor does not support ARM mode `isb '
/tmp/ccSPowmq.s:178: Error: selected processor does not support ARM mode `dsb '
make[2]: *** [arch/arm/mach-bcm/platsmp-brcmstb.o] Error 1
It seems that v7_exit_coherency_flush() can only be used with code which
is ARMv7 only.
+ /* Prevent all interrupts from reaching this CPU. */
+ arch_local_irq_disable();
Why do you think it is necessary to disable interrupts here? Where
have they been re-enabled since this bit of generic code:
void __ref cpu_die(void)
{
unsigned int cpu = smp_processor_id();
idle_task_exit();
local_irq_disable();
and why arch_local_irq_disable() at that? Even if interrupts were
enabled prior to your call to arch_local_irq_disable(), what do you
think would be the effect of receiving an interrupt after you've
exited coherency?
+
+ /*
+ * Final full barrier to ensure everything before this instruction has
+ * quiesced.
+ */
+ isb();
+ dsb();
If the call to arch_local_irq_disable() is removed, and
v7_exit_coherency_flush() is fixed, then this is not required, because
v7_exit_coherency_flush() already does this at the very end.
+
+ per_cpu_sw_state_wr(cpu, 0);
+
+ /* Sit and wait to die */
+ wfi();
+
+ /* We should never get here... */
+ panic(Spurious interrupt on CPU %d received!\n, cpu);
You really should /not/ be calling panic here, because that uses data
shared with the CPUs which are still coherent. This is akin to doing
DMA into bits of the kernel space without dealing with the cache
coherency issues. Moreover, if you read the comments on
v7_exit_coherency_flush() about ldrex/strex, which are two instructions
spinlocks use, you'll see that ldrex/strex must not be executed, which
means you can't call any function which uses spinlocks. That rules
out printk() et.al. printascii is fine, but that's only available when
the low level debug stuff is enabled.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Fri, Aug 01, 2014 at 09:33:44AM -0500, Rob Herring wrote: On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris computersforpe...@gmail.com wrote: Like what? Errata work-arounds, performance bits, etc. I don't recall exactly which ones are per core vs. global. I assume this is an A9, but cores with virt extensions would also need to drop to non-secure mode. The kernel's bringup just before enabling the CPU deals with that stuff provided your CPU is running in secure mode, but you are right - we /do/ want to encourage platforms to do the errata fixups before they call the kernel via any entry point. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote:
Hi Russell,
Yes, I noticed this. What I meant is that smp_ops.cpu_die() and
smp_ops.cpu_kill() are not synchronized.
...
We're not relying on the L1 cache, though. Don't sync_cache_{r,w}()
ensure all reads/writes reach at least the L2?
What exactly are you trying to achieve here?
How does that ensure that the CPU is down by the time the work is
scheduled? It seems like this would just defer the work long enough that
it *most likely* has quiesced, but I don't see how this gives us a
better guarantee. Or maybe I'm missing something. (If so, please do
enlighten!)
Note that I said a delayed work queue. The dying CPU runs a predictable
sequence once cpu_die() has been entered - interrupts at the GIC have
been programmed to be routed to other CPUs, interrupts at the CPU are
masked, so the CPU isn't going to be doing anything else except executing
that code path. So, it's going to be a predictable number of CPU cycles.
That allows you to arrange a delayed workqueue (or a timer) to fire
after a period of time that you can guarantee that the dying CPU has
reached that wfi().
Another point which raises itself in your patch is this:
+ /* Settle-time from Broadcom-internal DVT reference code */
+ udelay(7);
7us looks very precise, but udelay() may not be that precise. What is
the actual specification? Does it say you must wait at least 7us?
udelay() _may_ return early, especially if it is using the CPU delay
loop to perform the delay - I've explained why this happens previously,
and why it isn't a bug.
If you're using a timer-based delay for udelay() (which you should be
using if you support cpufreq) then the delay should be more accurate,
but it's still good practise to give a little leeway on the figure.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Fri, Aug 01, 2014 at 12:29:11PM -0700, Florian Fainelli wrote: > Hello, > > 2014-08-01 7:33 GMT-07:00 Rob Herring : > > On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris > > wrote: > >> Hi Rob, > >> > >> I appreciate your comments, but where were many of these 5 months ago on > >> the first 7 revisions? :) > > > > Sorry, but that is the nature of upstreaming. But given some of the > > issues, it is obvious the reviews were not sufficient. > > > >> On a practical note: v9 is already queued for 3.17. Should I send > >> patches for the 3.17 cycle (or later) to fixup some of these issues? Or > >> would you recommend pulling the patches out of Matt Porter's tree now, > >> and reintroducing for 3.18? (I would be much happier with the first.) > > > > Things can always be un-queued. I guess that's Matt's and arm-soc's > > decision. > > Does that mean we should get all those patches un-queued, because that > specific patch adding SMP support that needs to be reworked, or does > that mean that if we drop this specific patch we are good with the > remainder of the patch series? Well, keep in mind that there's no specific patch adding SMP support. The patch here contains *all* of the actual code that goes through mach-bcm. The rest will go through Russell. Given what was missed, if we drop just this patch, we're left with just the DT, Kconfig, and MAINTAINERS changes. It doesn't seem like there's time to fix the problems now. It might be better to drop the whole series from arm-soc since it won't be functional in 3.17 if we drop just this patch. I'd like to see what Arnd and Olof think. I think there's value in leaving all the DT bits for 3.17. -Matt -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hello,
2014-08-01 7:33 GMT-07:00 Rob Herring :
> On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris
> wrote:
>> Hi Rob,
>>
>> I appreciate your comments, but where were many of these 5 months ago on
>> the first 7 revisions? :)
>
> Sorry, but that is the nature of upstreaming. But given some of the
> issues, it is obvious the reviews were not sufficient.
>
>> On a practical note: v9 is already queued for 3.17. Should I send
>> patches for the 3.17 cycle (or later) to fixup some of these issues? Or
>> would you recommend pulling the patches out of Matt Porter's tree now,
>> and reintroducing for 3.18? (I would be much happier with the first.)
>
> Things can always be un-queued. I guess that's Matt's and arm-soc's decision.
Does that mean we should get all those patches un-queued, because that
specific patch adding SMP support that needs to be reworked, or does
that mean that if we drop this specific patch we are good with the
remainder of the patch series?
>
>>> > +static const char *brcmstb_match[] __initconst = {
>>> > + "brcm,bcm7445",
>>> > + "brcm,brcmstb",
>>> > + NULL
>>> > +};
>>> > +
>>> > +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)")
>>> > + .dt_compat = brcmstb_match,
>>> > +MACHINE_END
>>>
>>> Do you plan to add more here? Otherwise you don't need this file.
>>
>> Probably eventually, but not yet (there's out-of-tree stuff that hasn't
>> been integrated); should we drop the file until it's needed?
>
> I would say yes if you re-spin the patches.
>
>>> > +ENTRY(brcmstb_secondary_startup)
>>> > +/*
>>> > + * Ensure CPU is in a sane state by disabling all IRQs and
>>> > switching
>>> > + * into SVC mode.
>>> > + */
>>> > +setmodePSR_I_BIT | PSR_F_BIT | SVC_MODE, r0
>>> > +
>>> > +bl v7_invalidate_l1
>>>
>>> This should be in your boot code. That is part of the documented entry
>>> requirements.
>>
>> By "documented" are you referring to the ARM TRM? Wouldn't any "boot
>> code" requirements only apply to CPU0? Linux prepares the non-boot CPUs
>> for SMP, as you see here.
>>
>>> Also, if you are coming straight from reset, there is
>>> other setup probably missing.
>>
>> Like what?
>
> Errata work-arounds, performance bits, etc. I don't recall exactly
> which ones are per core vs. global. I assume this is an A9, but cores
> with virt extensions would also need to drop to non-secure mode.
This is a Brahma B15 CPU, maybe we should have mentioned that again in
this specific patch. The only ARM erratum we are affected (as of
today) is 798181, and it requires a one-time initialization.
>
> Also, your entry (and many of the custom entry points) would not work
> for big endian kernels. You may not care, but there's really no reason
> why it should not work.
I don't think we want to support big-endian kernels at all, although I
can't see a good way to enforce that restriction.
>
>>> > + per_cpu(per_cpu_sw_state, cpu) = val;
>>> > + dmb();
>>> > + sync_cache_w(SHIFT_PERCPU_PTR(_cpu_sw_state,
>>> > per_cpu_offset(cpu)));
>>> > + dsb_sev();
>>>
>>> Barriers need to be documented why they are needed.
>>
>> (NB: I see approximately 3 total existing cases where there is a comment
>> near a dsb()/dsb_sev().)
>>
>> This was actually adapted from the mcpm code, and
>
> Why? Blindly copying other implementations seems to be a common issue here...
>
>> (1) I think the dmb() is in the wrong place (it should be the first
>> thing in this function);
>> (2) the SEV is unnecessary (we don't use WFE; just a cache-invalidated
>> polling loop); and
>> (3) the DSB is also unnecessary, since sync_cache_w() handles its own
>> barriers.
>>
>> Plus, we should probably add some comments to describe what's going on
>> here. (Follow-up patch?)
>
> I'm not sure you need the state variable at all. You appear to be able
> to fully control power on and off of the cores, so you should not need
> any spin loops or synchronization.
>
>>> > +static int brcmstb_cpu_kill(u32 cpu)
>>> > +{
>>> > + u32 tmp;
>>> > +
>>> > + pr_info("SMP: Powering down CPU%d...\n", cpu);
>>> > +
>>> > + while (per_cpu_sw_state_rd(cpu))
>>> > + ;
>>>
>>> I don't think this is necessary. You don't need to synchronize die and
>>> kill. Look at other implementations that actually power down the core
>>> (vs. just wfi).
>>
>> Hmm, I'm pretty sure the synchronization is required for our B15 core.
>> If we yank the power before the core has properly quiesced, the whole
>> CPU complex will lock up. (Similar story for the power-up while loop you
>> complained about above.)
>
> That is true for every SMP system which is why it is done in the core
> code as Russell pointed out.
>
>>> > +static void brcmstb_secondary_init(unsigned int cpu)
>>> > +{
>>> > + /*
>>> > +* Synchronise with the boot thread.
>>> > +*/
>>> > + spin_lock(_lock);
>>> > +
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris
wrote:
> Hi Rob,
>
> I appreciate your comments, but where were many of these 5 months ago on
> the first 7 revisions? :)
Sorry, but that is the nature of upstreaming. But given some of the
issues, it is obvious the reviews were not sufficient.
> On a practical note: v9 is already queued for 3.17. Should I send
> patches for the 3.17 cycle (or later) to fixup some of these issues? Or
> would you recommend pulling the patches out of Matt Porter's tree now,
> and reintroducing for 3.18? (I would be much happier with the first.)
Things can always be un-queued. I guess that's Matt's and arm-soc's decision.
>> > +static const char *brcmstb_match[] __initconst = {
>> > + "brcm,bcm7445",
>> > + "brcm,brcmstb",
>> > + NULL
>> > +};
>> > +
>> > +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)")
>> > + .dt_compat = brcmstb_match,
>> > +MACHINE_END
>>
>> Do you plan to add more here? Otherwise you don't need this file.
>
> Probably eventually, but not yet (there's out-of-tree stuff that hasn't
> been integrated); should we drop the file until it's needed?
I would say yes if you re-spin the patches.
>> > +ENTRY(brcmstb_secondary_startup)
>> > +/*
>> > + * Ensure CPU is in a sane state by disabling all IRQs and
>> > switching
>> > + * into SVC mode.
>> > + */
>> > +setmodePSR_I_BIT | PSR_F_BIT | SVC_MODE, r0
>> > +
>> > +bl v7_invalidate_l1
>>
>> This should be in your boot code. That is part of the documented entry
>> requirements.
>
> By "documented" are you referring to the ARM TRM? Wouldn't any "boot
> code" requirements only apply to CPU0? Linux prepares the non-boot CPUs
> for SMP, as you see here.
>
>> Also, if you are coming straight from reset, there is
>> other setup probably missing.
>
> Like what?
Errata work-arounds, performance bits, etc. I don't recall exactly
which ones are per core vs. global. I assume this is an A9, but cores
with virt extensions would also need to drop to non-secure mode.
Also, your entry (and many of the custom entry points) would not work
for big endian kernels. You may not care, but there's really no reason
why it should not work.
>> > + per_cpu(per_cpu_sw_state, cpu) = val;
>> > + dmb();
>> > + sync_cache_w(SHIFT_PERCPU_PTR(_cpu_sw_state,
>> > per_cpu_offset(cpu)));
>> > + dsb_sev();
>>
>> Barriers need to be documented why they are needed.
>
> (NB: I see approximately 3 total existing cases where there is a comment
> near a dsb()/dsb_sev().)
>
> This was actually adapted from the mcpm code, and
Why? Blindly copying other implementations seems to be a common issue here...
> (1) I think the dmb() is in the wrong place (it should be the first
> thing in this function);
> (2) the SEV is unnecessary (we don't use WFE; just a cache-invalidated
> polling loop); and
> (3) the DSB is also unnecessary, since sync_cache_w() handles its own
> barriers.
>
> Plus, we should probably add some comments to describe what's going on
> here. (Follow-up patch?)
I'm not sure you need the state variable at all. You appear to be able
to fully control power on and off of the cores, so you should not need
any spin loops or synchronization.
>> > +static int brcmstb_cpu_kill(u32 cpu)
>> > +{
>> > + u32 tmp;
>> > +
>> > + pr_info("SMP: Powering down CPU%d...\n", cpu);
>> > +
>> > + while (per_cpu_sw_state_rd(cpu))
>> > + ;
>>
>> I don't think this is necessary. You don't need to synchronize die and
>> kill. Look at other implementations that actually power down the core
>> (vs. just wfi).
>
> Hmm, I'm pretty sure the synchronization is required for our B15 core.
> If we yank the power before the core has properly quiesced, the whole
> CPU complex will lock up. (Similar story for the power-up while loop you
> complained about above.)
That is true for every SMP system which is why it is done in the core
code as Russell pointed out.
>> > +static void brcmstb_secondary_init(unsigned int cpu)
>> > +{
>> > + /*
>> > +* Synchronise with the boot thread.
>> > +*/
>> > + spin_lock(_lock);
>> > + spin_unlock(_lock);
>>
>> I suggest you read previous discussions on attempts to make this
>> common before replying to Russell.
>
> Can you point me to this discussion? linux-arm-kernel is a big and noisy
> world...
Here's a recent example:
http://www.spinics.net/lists/arm-kernel/msg318585.html
> BTW, I'm not sure the boot_lock is actually necessary at all for us; it
> was just borrowed from one of the other mach-*'s. The synchronization we
> need is done in the while loops above.
The blindly copying code without knowing what it does problem...
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris
computersforpe...@gmail.com wrote:
Hi Rob,
I appreciate your comments, but where were many of these 5 months ago on
the first 7 revisions? :)
Sorry, but that is the nature of upstreaming. But given some of the
issues, it is obvious the reviews were not sufficient.
On a practical note: v9 is already queued for 3.17. Should I send
patches for the 3.17 cycle (or later) to fixup some of these issues? Or
would you recommend pulling the patches out of Matt Porter's tree now,
and reintroducing for 3.18? (I would be much happier with the first.)
Things can always be un-queued. I guess that's Matt's and arm-soc's decision.
+static const char *brcmstb_match[] __initconst = {
+ brcm,bcm7445,
+ brcm,brcmstb,
+ NULL
+};
+
+DT_MACHINE_START(BRCMSTB, Broadcom STB (Flattened Device Tree))
+ .dt_compat = brcmstb_match,
+MACHINE_END
Do you plan to add more here? Otherwise you don't need this file.
Probably eventually, but not yet (there's out-of-tree stuff that hasn't
been integrated); should we drop the file until it's needed?
I would say yes if you re-spin the patches.
+ENTRY(brcmstb_secondary_startup)
+/*
+ * Ensure CPU is in a sane state by disabling all IRQs and
switching
+ * into SVC mode.
+ */
+setmodePSR_I_BIT | PSR_F_BIT | SVC_MODE, r0
+
+bl v7_invalidate_l1
This should be in your boot code. That is part of the documented entry
requirements.
By documented are you referring to the ARM TRM? Wouldn't any boot
code requirements only apply to CPU0? Linux prepares the non-boot CPUs
for SMP, as you see here.
Also, if you are coming straight from reset, there is
other setup probably missing.
Like what?
Errata work-arounds, performance bits, etc. I don't recall exactly
which ones are per core vs. global. I assume this is an A9, but cores
with virt extensions would also need to drop to non-secure mode.
Also, your entry (and many of the custom entry points) would not work
for big endian kernels. You may not care, but there's really no reason
why it should not work.
+ per_cpu(per_cpu_sw_state, cpu) = val;
+ dmb();
+ sync_cache_w(SHIFT_PERCPU_PTR(per_cpu_sw_state,
per_cpu_offset(cpu)));
+ dsb_sev();
Barriers need to be documented why they are needed.
(NB: I see approximately 3 total existing cases where there is a comment
near a dsb()/dsb_sev().)
This was actually adapted from the mcpm code, and
Why? Blindly copying other implementations seems to be a common issue here...
(1) I think the dmb() is in the wrong place (it should be the first
thing in this function);
(2) the SEV is unnecessary (we don't use WFE; just a cache-invalidated
polling loop); and
(3) the DSB is also unnecessary, since sync_cache_w() handles its own
barriers.
Plus, we should probably add some comments to describe what's going on
here. (Follow-up patch?)
I'm not sure you need the state variable at all. You appear to be able
to fully control power on and off of the cores, so you should not need
any spin loops or synchronization.
+static int brcmstb_cpu_kill(u32 cpu)
+{
+ u32 tmp;
+
+ pr_info(SMP: Powering down CPU%d...\n, cpu);
+
+ while (per_cpu_sw_state_rd(cpu))
+ ;
I don't think this is necessary. You don't need to synchronize die and
kill. Look at other implementations that actually power down the core
(vs. just wfi).
Hmm, I'm pretty sure the synchronization is required for our B15 core.
If we yank the power before the core has properly quiesced, the whole
CPU complex will lock up. (Similar story for the power-up while loop you
complained about above.)
That is true for every SMP system which is why it is done in the core
code as Russell pointed out.
+static void brcmstb_secondary_init(unsigned int cpu)
+{
+ /*
+* Synchronise with the boot thread.
+*/
+ spin_lock(boot_lock);
+ spin_unlock(boot_lock);
I suggest you read previous discussions on attempts to make this
common before replying to Russell.
Can you point me to this discussion? linux-arm-kernel is a big and noisy
world...
Here's a recent example:
http://www.spinics.net/lists/arm-kernel/msg318585.html
BTW, I'm not sure the boot_lock is actually necessary at all for us; it
was just borrowed from one of the other mach-*'s. The synchronization we
need is done in the while loops above.
The blindly copying code without knowing what it does problem...
Rob
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hello,
2014-08-01 7:33 GMT-07:00 Rob Herring robherri...@gmail.com:
On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris
computersforpe...@gmail.com wrote:
Hi Rob,
I appreciate your comments, but where were many of these 5 months ago on
the first 7 revisions? :)
Sorry, but that is the nature of upstreaming. But given some of the
issues, it is obvious the reviews were not sufficient.
On a practical note: v9 is already queued for 3.17. Should I send
patches for the 3.17 cycle (or later) to fixup some of these issues? Or
would you recommend pulling the patches out of Matt Porter's tree now,
and reintroducing for 3.18? (I would be much happier with the first.)
Things can always be un-queued. I guess that's Matt's and arm-soc's decision.
Does that mean we should get all those patches un-queued, because that
specific patch adding SMP support that needs to be reworked, or does
that mean that if we drop this specific patch we are good with the
remainder of the patch series?
+static const char *brcmstb_match[] __initconst = {
+ brcm,bcm7445,
+ brcm,brcmstb,
+ NULL
+};
+
+DT_MACHINE_START(BRCMSTB, Broadcom STB (Flattened Device Tree))
+ .dt_compat = brcmstb_match,
+MACHINE_END
Do you plan to add more here? Otherwise you don't need this file.
Probably eventually, but not yet (there's out-of-tree stuff that hasn't
been integrated); should we drop the file until it's needed?
I would say yes if you re-spin the patches.
+ENTRY(brcmstb_secondary_startup)
+/*
+ * Ensure CPU is in a sane state by disabling all IRQs and
switching
+ * into SVC mode.
+ */
+setmodePSR_I_BIT | PSR_F_BIT | SVC_MODE, r0
+
+bl v7_invalidate_l1
This should be in your boot code. That is part of the documented entry
requirements.
By documented are you referring to the ARM TRM? Wouldn't any boot
code requirements only apply to CPU0? Linux prepares the non-boot CPUs
for SMP, as you see here.
Also, if you are coming straight from reset, there is
other setup probably missing.
Like what?
Errata work-arounds, performance bits, etc. I don't recall exactly
which ones are per core vs. global. I assume this is an A9, but cores
with virt extensions would also need to drop to non-secure mode.
This is a Brahma B15 CPU, maybe we should have mentioned that again in
this specific patch. The only ARM erratum we are affected (as of
today) is 798181, and it requires a one-time initialization.
Also, your entry (and many of the custom entry points) would not work
for big endian kernels. You may not care, but there's really no reason
why it should not work.
I don't think we want to support big-endian kernels at all, although I
can't see a good way to enforce that restriction.
+ per_cpu(per_cpu_sw_state, cpu) = val;
+ dmb();
+ sync_cache_w(SHIFT_PERCPU_PTR(per_cpu_sw_state,
per_cpu_offset(cpu)));
+ dsb_sev();
Barriers need to be documented why they are needed.
(NB: I see approximately 3 total existing cases where there is a comment
near a dsb()/dsb_sev().)
This was actually adapted from the mcpm code, and
Why? Blindly copying other implementations seems to be a common issue here...
(1) I think the dmb() is in the wrong place (it should be the first
thing in this function);
(2) the SEV is unnecessary (we don't use WFE; just a cache-invalidated
polling loop); and
(3) the DSB is also unnecessary, since sync_cache_w() handles its own
barriers.
Plus, we should probably add some comments to describe what's going on
here. (Follow-up patch?)
I'm not sure you need the state variable at all. You appear to be able
to fully control power on and off of the cores, so you should not need
any spin loops or synchronization.
+static int brcmstb_cpu_kill(u32 cpu)
+{
+ u32 tmp;
+
+ pr_info(SMP: Powering down CPU%d...\n, cpu);
+
+ while (per_cpu_sw_state_rd(cpu))
+ ;
I don't think this is necessary. You don't need to synchronize die and
kill. Look at other implementations that actually power down the core
(vs. just wfi).
Hmm, I'm pretty sure the synchronization is required for our B15 core.
If we yank the power before the core has properly quiesced, the whole
CPU complex will lock up. (Similar story for the power-up while loop you
complained about above.)
That is true for every SMP system which is why it is done in the core
code as Russell pointed out.
+static void brcmstb_secondary_init(unsigned int cpu)
+{
+ /*
+* Synchronise with the boot thread.
+*/
+ spin_lock(boot_lock);
+ spin_unlock(boot_lock);
I suggest you read previous discussions on attempts to make this
common before replying to Russell.
Can you point me to this discussion? linux-arm-kernel is a big and noisy
world...
Here's a recent example:
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Fri, Aug 01, 2014 at 12:29:11PM -0700, Florian Fainelli wrote: Hello, 2014-08-01 7:33 GMT-07:00 Rob Herring robherri...@gmail.com: On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris computersforpe...@gmail.com wrote: Hi Rob, I appreciate your comments, but where were many of these 5 months ago on the first 7 revisions? :) Sorry, but that is the nature of upstreaming. But given some of the issues, it is obvious the reviews were not sufficient. On a practical note: v9 is already queued for 3.17. Should I send patches for the 3.17 cycle (or later) to fixup some of these issues? Or would you recommend pulling the patches out of Matt Porter's tree now, and reintroducing for 3.18? (I would be much happier with the first.) Things can always be un-queued. I guess that's Matt's and arm-soc's decision. Does that mean we should get all those patches un-queued, because that specific patch adding SMP support that needs to be reworked, or does that mean that if we drop this specific patch we are good with the remainder of the patch series? Well, keep in mind that there's no specific patch adding SMP support. The patch here contains *all* of the actual code that goes through mach-bcm. The rest will go through Russell. Given what was missed, if we drop just this patch, we're left with just the DT, Kconfig, and MAINTAINERS changes. It doesn't seem like there's time to fix the problems now. It might be better to drop the whole series from arm-soc since it won't be functional in 3.17 if we drop just this patch. I'd like to see what Arnd and Olof think. I think there's value in leaving all the DT bits for 3.17. -Matt -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Russell,
On Thu, Jul 31, 2014 at 09:43:15AM +0100, Russell King wrote:
> On Wed, Jul 30, 2014 at 07:23:20PM -0700, Brian Norris wrote:
> > I appreciate your comments, but where were many of these 5 months ago on
> > the first 7 revisions? :)
> >
> > On a practical note: v9 is already queued for 3.17. Should I send
> > patches for the 3.17 cycle (or later) to fixup some of these issues? Or
> > would you recommend pulling the patches out of Matt Porter's tree now,
> > and reintroducing for 3.18? (I would be much happier with the first.)
> >
> > I do note that we at least need to fix allmodconfig ASAP; I'll reply to
> > Russell on that one.
> >
> > On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote:
> > > On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris
> > > wrote:
> > > > +
> > > > + per_cpu_sw_state_wr(cpu, 1);
> > >
> > > The kernel already tracks the state.
> >
> > Yes, but it doesn't synchronize cpu_die() and cpu_kill(). But I see you
> > objected below.
>
> Err.
>
> static DECLARE_COMPLETION(cpu_died);
Yes, I noticed this. What I meant is that smp_ops.cpu_die() and
smp_ops.cpu_kill() are not synchronized.
> /*
> * called on the thread which is asking for a CPU to be shutdown -
> * waits until shutdown has completed, or it is timed out.
> */
> void __cpu_die(unsigned int cpu)
> {
> if (!wait_for_completion_timeout(_died, msecs_to_jiffies(5000))) {
> pr_err("CPU%u: cpu didn't die\n", cpu);
> return;
> }
> printk(KERN_NOTICE "CPU%u: shutdown\n", cpu);
> ...
> if (!platform_cpu_kill(cpu))
> printk("CPU%u: unable to kill\n", cpu);
> }
>
> void __ref cpu_die(void)
> {
> ...
> /*
> * Tell __cpu_die() that this CPU is now safe to dispose of. Once
> * this returns, power and/or clocks can be removed at any point
> * from this CPU and its cache by platform_cpu_kill().
> */
> complete(_died);
>
> There _is_ synchronisation between these two. Your cpu_kill() function
> will not be called until we have flushed all data from the dying CPU's
> cache. That's the best we can do, because if we cause the dying CPU to
> exit the coherency domain, any kind of locking or completion will no
> longer work (neither will your state tracking solution because the L1
> caches will no longer snoop.)
We're not relying on the L1 cache, though. Don't sync_cache_{r,w}()
ensure all reads/writes reach at least the L2?
Besides, we modeled this after the MCPM code (e.g., __mcpm_cpu_down()).
Are both implementations incorrect?
> > Hmm, I'm pretty sure the synchronization is required for our B15 core.
> > If we yank the power before the core has properly quiesced, the whole
> > CPU complex will lock up. (Similar story for the power-up while loop you
> > complained about above.)
>
> If you need to ensure that the power isn't turned off too soon, how about
> using a delayed work queue to do the power-off of the CPU (remembering to
> cancel the delayed work queue when powering the CPU back up.)
How does that ensure that the CPU is down by the time the work is
scheduled? It seems like this would just defer the work long enough that
it *most likely* has quiesced, but I don't see how this gives us a
better guarantee. Or maybe I'm missing something. (If so, please do
enlighten!)
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Wed, Jul 30, 2014 at 07:23:20PM -0700, Brian Norris wrote:
> Hi Rob,
>
> I appreciate your comments, but where were many of these 5 months ago on
> the first 7 revisions? :)
>
> On a practical note: v9 is already queued for 3.17. Should I send
> patches for the 3.17 cycle (or later) to fixup some of these issues? Or
> would you recommend pulling the patches out of Matt Porter's tree now,
> and reintroducing for 3.18? (I would be much happier with the first.)
>
> I do note that we at least need to fix allmodconfig ASAP; I'll reply to
> Russell on that one.
>
> On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote:
> > On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris
> > wrote:
> > > +
> > > + per_cpu_sw_state_wr(cpu, 1);
> >
> > The kernel already tracks the state.
>
> Yes, but it doesn't synchronize cpu_die() and cpu_kill(). But I see you
> objected below.
Err.
static DECLARE_COMPLETION(cpu_died);
/*
* called on the thread which is asking for a CPU to be shutdown -
* waits until shutdown has completed, or it is timed out.
*/
void __cpu_die(unsigned int cpu)
{
if (!wait_for_completion_timeout(_died, msecs_to_jiffies(5000))) {
pr_err("CPU%u: cpu didn't die\n", cpu);
return;
}
printk(KERN_NOTICE "CPU%u: shutdown\n", cpu);
...
if (!platform_cpu_kill(cpu))
printk("CPU%u: unable to kill\n", cpu);
}
void __ref cpu_die(void)
{
...
/*
* Tell __cpu_die() that this CPU is now safe to dispose of. Once
* this returns, power and/or clocks can be removed at any point
* from this CPU and its cache by platform_cpu_kill().
*/
complete(_died);
There _is_ synchronisation between these two. Your cpu_kill() function
will not be called until we have flushed all data from the dying CPU's
cache. That's the best we can do, because if we cause the dying CPU to
exit the coherency domain, any kind of locking or completion will no
longer work (neither will your state tracking solution because the L1
caches will no longer snoop.)
> Hmm, I'm pretty sure the synchronization is required for our B15 core.
> If we yank the power before the core has properly quiesced, the whole
> CPU complex will lock up. (Similar story for the power-up while loop you
> complained about above.)
If you need to ensure that the power isn't turned off too soon, how about
using a delayed work queue to do the power-off of the CPU (remembering to
cancel the delayed work queue when powering the CPU back up.)
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Wed, Jul 30, 2014 at 07:23:20PM -0700, Brian Norris wrote:
Hi Rob,
I appreciate your comments, but where were many of these 5 months ago on
the first 7 revisions? :)
On a practical note: v9 is already queued for 3.17. Should I send
patches for the 3.17 cycle (or later) to fixup some of these issues? Or
would you recommend pulling the patches out of Matt Porter's tree now,
and reintroducing for 3.18? (I would be much happier with the first.)
I do note that we at least need to fix allmodconfig ASAP; I'll reply to
Russell on that one.
On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote:
On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris computersforpe...@gmail.com
wrote:
+
+ per_cpu_sw_state_wr(cpu, 1);
The kernel already tracks the state.
Yes, but it doesn't synchronize cpu_die() and cpu_kill(). But I see you
objected below.
Err.
static DECLARE_COMPLETION(cpu_died);
/*
* called on the thread which is asking for a CPU to be shutdown -
* waits until shutdown has completed, or it is timed out.
*/
void __cpu_die(unsigned int cpu)
{
if (!wait_for_completion_timeout(cpu_died, msecs_to_jiffies(5000))) {
pr_err(CPU%u: cpu didn't die\n, cpu);
return;
}
printk(KERN_NOTICE CPU%u: shutdown\n, cpu);
...
if (!platform_cpu_kill(cpu))
printk(CPU%u: unable to kill\n, cpu);
}
void __ref cpu_die(void)
{
...
/*
* Tell __cpu_die() that this CPU is now safe to dispose of. Once
* this returns, power and/or clocks can be removed at any point
* from this CPU and its cache by platform_cpu_kill().
*/
complete(cpu_died);
There _is_ synchronisation between these two. Your cpu_kill() function
will not be called until we have flushed all data from the dying CPU's
cache. That's the best we can do, because if we cause the dying CPU to
exit the coherency domain, any kind of locking or completion will no
longer work (neither will your state tracking solution because the L1
caches will no longer snoop.)
Hmm, I'm pretty sure the synchronization is required for our B15 core.
If we yank the power before the core has properly quiesced, the whole
CPU complex will lock up. (Similar story for the power-up while loop you
complained about above.)
If you need to ensure that the power isn't turned off too soon, how about
using a delayed work queue to do the power-off of the CPU (remembering to
cancel the delayed work queue when powering the CPU back up.)
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Russell,
On Thu, Jul 31, 2014 at 09:43:15AM +0100, Russell King wrote:
On Wed, Jul 30, 2014 at 07:23:20PM -0700, Brian Norris wrote:
I appreciate your comments, but where were many of these 5 months ago on
the first 7 revisions? :)
On a practical note: v9 is already queued for 3.17. Should I send
patches for the 3.17 cycle (or later) to fixup some of these issues? Or
would you recommend pulling the patches out of Matt Porter's tree now,
and reintroducing for 3.18? (I would be much happier with the first.)
I do note that we at least need to fix allmodconfig ASAP; I'll reply to
Russell on that one.
On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote:
On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris
computersforpe...@gmail.com wrote:
+
+ per_cpu_sw_state_wr(cpu, 1);
The kernel already tracks the state.
Yes, but it doesn't synchronize cpu_die() and cpu_kill(). But I see you
objected below.
Err.
static DECLARE_COMPLETION(cpu_died);
Yes, I noticed this. What I meant is that smp_ops.cpu_die() and
smp_ops.cpu_kill() are not synchronized.
/*
* called on the thread which is asking for a CPU to be shutdown -
* waits until shutdown has completed, or it is timed out.
*/
void __cpu_die(unsigned int cpu)
{
if (!wait_for_completion_timeout(cpu_died, msecs_to_jiffies(5000))) {
pr_err(CPU%u: cpu didn't die\n, cpu);
return;
}
printk(KERN_NOTICE CPU%u: shutdown\n, cpu);
...
if (!platform_cpu_kill(cpu))
printk(CPU%u: unable to kill\n, cpu);
}
void __ref cpu_die(void)
{
...
/*
* Tell __cpu_die() that this CPU is now safe to dispose of. Once
* this returns, power and/or clocks can be removed at any point
* from this CPU and its cache by platform_cpu_kill().
*/
complete(cpu_died);
There _is_ synchronisation between these two. Your cpu_kill() function
will not be called until we have flushed all data from the dying CPU's
cache. That's the best we can do, because if we cause the dying CPU to
exit the coherency domain, any kind of locking or completion will no
longer work (neither will your state tracking solution because the L1
caches will no longer snoop.)
We're not relying on the L1 cache, though. Don't sync_cache_{r,w}()
ensure all reads/writes reach at least the L2?
Besides, we modeled this after the MCPM code (e.g., __mcpm_cpu_down()).
Are both implementations incorrect?
Hmm, I'm pretty sure the synchronization is required for our B15 core.
If we yank the power before the core has properly quiesced, the whole
CPU complex will lock up. (Similar story for the power-up while loop you
complained about above.)
If you need to ensure that the power isn't turned off too soon, how about
using a delayed work queue to do the power-off of the CPU (remembering to
cancel the delayed work queue when powering the CPU back up.)
How does that ensure that the CPU is down by the time the work is
scheduled? It seems like this would just defer the work long enough that
it *most likely* has quiesced, but I don't see how this gives us a
better guarantee. Or maybe I'm missing something. (If so, please do
enlighten!)
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Russell,
On Wed, Jul 30, 2014 at 10:26:35AM +0100, Russell King wrote:
> On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:
> > +static DEFINE_SPINLOCK(boot_lock);
> > +
> > +static void brcmstb_secondary_init(unsigned int cpu)
> > +{
> > + /*
> > +* Synchronise with the boot thread.
> > +*/
> > + spin_lock(_lock);
> > + spin_unlock(_lock);
> > +}
> > +
> > +static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct
> > *idle)
> > +{
> > + /*
> > +* set synchronisation state between this boot processor
> > +* and the secondary one
> > +*/
> > + spin_lock(_lock);
> > +
> > + /* Bring up power to the core if necessary */
> > + if (brcmstb_cpu_get_power_state(cpu) == 0)
> > + brcmstb_cpu_power_on(cpu);
> > +
> > + brcmstb_cpu_boot(cpu);
> > +
> > + /*
> > +* now the secondary core is starting up let it run its
> > +* calibrations, then wait for it to finish
> > +*/
> > + spin_unlock(_lock);
>
> I've just read through this code (because it caused my allmodconfig to
> break) and spotted this.
Sorry about the allmodconfig problems. I never compile-tested with ARMv6
enabled. This look OK?
diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
index f3665121729b..5ce82b4ba931 100644
--- a/arch/arm/mach-bcm/Makefile
+++ b/arch/arm/mach-bcm/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_BCM2835)+= board_bcm2835.o
obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o
ifeq ($(CONFIG_ARCH_BRCMSTB),y)
+CFLAGS_platsmp-brcmstb.o += -march=armv7-a
obj-y += brcmstb.o
obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o
endif
> What function does boot_lock perform here? Please, don't quote the
> comments (I know where the comments came from) but what I want to hear
> is your comments about why you decided to retain this.
You might glean a little more from my response to Rob, but I'm not sure
there was a good reason for retaining this. We do need to be sure the
CPU is fully powered online before bringing it out of reset, but the
spinlock seems overkill AFAICT.
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Rob,
I appreciate your comments, but where were many of these 5 months ago on
the first 7 revisions? :)
On a practical note: v9 is already queued for 3.17. Should I send
patches for the 3.17 cycle (or later) to fixup some of these issues? Or
would you recommend pulling the patches out of Matt Porter's tree now,
and reintroducing for 3.18? (I would be much happier with the first.)
I do note that we at least need to fix allmodconfig ASAP; I'll reply to
Russell on that one.
On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote:
> On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris
> wrote:
> > From: Marc Carino
> >
> > The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes.
> >
> > This patch adds machine support for the ARM-based Broadcom SoCs.
> >
> > Signed-off-by: Marc Carino
> > Signed-off-by: Brian Norris
> > ---
> > arch/arm/configs/multi_v7_defconfig | 1 +
> > arch/arm/mach-bcm/Kconfig | 14 ++
> > arch/arm/mach-bcm/Makefile | 5 +
> > arch/arm/mach-bcm/brcmstb.c | 28 +++
> > arch/arm/mach-bcm/brcmstb.h | 19 ++
> > arch/arm/mach-bcm/headsmp-brcmstb.S | 33
> > arch/arm/mach-bcm/platsmp-brcmstb.c | 363
> >
> > 7 files changed, 463 insertions(+)
> > create mode 100644 arch/arm/mach-bcm/brcmstb.c
> > create mode 100644 arch/arm/mach-bcm/brcmstb.h
> > create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S
> > create mode 100644 arch/arm/mach-bcm/platsmp-brcmstb.c
> >
> > diff --git a/arch/arm/configs/multi_v7_defconfig
> > b/arch/arm/configs/multi_v7_defconfig
> > index 534836497998..bf0df396a3cf 100644
> > --- a/arch/arm/configs/multi_v7_defconfig
> > +++ b/arch/arm/configs/multi_v7_defconfig
> > @@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y
> > CONFIG_ARCH_BCM=y
> > CONFIG_ARCH_BCM_MOBILE=y
> > CONFIG_ARCH_BCM_5301X=y
> > +CONFIG_ARCH_BRCMSTB=y
> > CONFIG_ARCH_BERLIN=y
> > CONFIG_MACH_BERLIN_BG2=y
> > CONFIG_MACH_BERLIN_BG2CD=y
> > diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
> > index 41c839167e87..0073633e7699 100644
> > --- a/arch/arm/mach-bcm/Kconfig
> > +++ b/arch/arm/mach-bcm/Kconfig
> > @@ -87,4 +87,18 @@ config ARCH_BCM_5301X
> > different SoC or with the older BCM47XX and BCM53XX based
> > network SoC using a MIPS CPU, they are supported by
> > arch/mips/bcm47xx
> >
> > +config ARCH_BRCMSTB
> > + bool "Broadcom BCM7XXX based boards" if ARCH_MULTI_V7
> > + depends on MMU
>
> This was implied, but there are some patches from Arnd in this area.
> Does it really not build with !MMU?
I suppose it probably builds, it likely won't run. I can drop this line
and then reassess if ARCH_MULTIPLATFORM becomes buildable with !MMU.
> > + select ARM_GIC
>
> > + select MIGHT_HAVE_PCI
> > + select HAVE_SMP
>
> At least HAVE_SMP is for sure, but I think both of these are selected already
> .
You're correct. ARCH_MULTIPLATFORM selects MIGHT_HAVE_PCI and
ARCH_MULTI_V7 selects HAVE_SMP. I can look at dropping these.
> > + select HAVE_ARM_ARCH_TIMER
> > + help
> > + Say Y if you intend to run the kernel on a Broadcom ARM-based STB
> > + chipset.
> > +
> > + This enables support for Broadcom ARM-based set-top box chipsets,
> > + including the 7445 family of chips.
> > +
> > endif
> > diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
> > index 731292114975..f3665121729b 100644
> > --- a/arch/arm/mach-bcm/Makefile
> > +++ b/arch/arm/mach-bcm/Makefile
> > @@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835)+= board_bcm2835.o
> >
> > # BCM5301X
> > obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o
> > +
> > +ifeq ($(CONFIG_ARCH_BRCMSTB),y)
> > +obj-y += brcmstb.o
> > +obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o
> > +endif
> > diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c
> > new file mode 100644
> > index ..60a5afa06ed7
> > --- /dev/null
> > +++ b/arch/arm/mach-bcm/brcmstb.c
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (C) 2013-2014 Broadcom Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +
> > +static const char *brcmstb_match[] __initconst = {
> > + "brcm,bcm7445",
> > + "brcm,brcmstb",
> > + NULL
> > +};
> > +
> > +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)")
> > + .dt_compat = brcmstb_match,
>
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris
wrote:
> From: Marc Carino
>
> The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes.
>
> This patch adds machine support for the ARM-based Broadcom SoCs.
>
> Signed-off-by: Marc Carino
> Signed-off-by: Brian Norris
> ---
> arch/arm/configs/multi_v7_defconfig | 1 +
> arch/arm/mach-bcm/Kconfig | 14 ++
> arch/arm/mach-bcm/Makefile | 5 +
> arch/arm/mach-bcm/brcmstb.c | 28 +++
> arch/arm/mach-bcm/brcmstb.h | 19 ++
> arch/arm/mach-bcm/headsmp-brcmstb.S | 33
> arch/arm/mach-bcm/platsmp-brcmstb.c | 363
>
> 7 files changed, 463 insertions(+)
> create mode 100644 arch/arm/mach-bcm/brcmstb.c
> create mode 100644 arch/arm/mach-bcm/brcmstb.h
> create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S
> create mode 100644 arch/arm/mach-bcm/platsmp-brcmstb.c
>
> diff --git a/arch/arm/configs/multi_v7_defconfig
> b/arch/arm/configs/multi_v7_defconfig
> index 534836497998..bf0df396a3cf 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y
> CONFIG_ARCH_BCM=y
> CONFIG_ARCH_BCM_MOBILE=y
> CONFIG_ARCH_BCM_5301X=y
> +CONFIG_ARCH_BRCMSTB=y
> CONFIG_ARCH_BERLIN=y
> CONFIG_MACH_BERLIN_BG2=y
> CONFIG_MACH_BERLIN_BG2CD=y
> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
> index 41c839167e87..0073633e7699 100644
> --- a/arch/arm/mach-bcm/Kconfig
> +++ b/arch/arm/mach-bcm/Kconfig
> @@ -87,4 +87,18 @@ config ARCH_BCM_5301X
> different SoC or with the older BCM47XX and BCM53XX based
> network SoC using a MIPS CPU, they are supported by
> arch/mips/bcm47xx
>
> +config ARCH_BRCMSTB
> + bool "Broadcom BCM7XXX based boards" if ARCH_MULTI_V7
> + depends on MMU
This was implied, but there are some patches from Arnd in this area.
Does it really not build with !MMU?
> + select ARM_GIC
> + select MIGHT_HAVE_PCI
> + select HAVE_SMP
At least HAVE_SMP is for sure, but I think both of these are selected already .
> + select HAVE_ARM_ARCH_TIMER
> + help
> + Say Y if you intend to run the kernel on a Broadcom ARM-based STB
> + chipset.
> +
> + This enables support for Broadcom ARM-based set-top box chipsets,
> + including the 7445 family of chips.
> +
> endif
> diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
> index 731292114975..f3665121729b 100644
> --- a/arch/arm/mach-bcm/Makefile
> +++ b/arch/arm/mach-bcm/Makefile
> @@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835)+= board_bcm2835.o
>
> # BCM5301X
> obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o
> +
> +ifeq ($(CONFIG_ARCH_BRCMSTB),y)
> +obj-y += brcmstb.o
> +obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o
> +endif
> diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c
> new file mode 100644
> index ..60a5afa06ed7
> --- /dev/null
> +++ b/arch/arm/mach-bcm/brcmstb.c
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (C) 2013-2014 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +
> +#include
> +#include
> +
> +static const char *brcmstb_match[] __initconst = {
> + "brcm,bcm7445",
> + "brcm,brcmstb",
> + NULL
> +};
> +
> +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)")
> + .dt_compat = brcmstb_match,
> +MACHINE_END
Do you plan to add more here? Otherwise you don't need this file.
> diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h
> new file mode 100644
> index ..ec0c3d112b36
> --- /dev/null
> +++ b/arch/arm/mach-bcm/brcmstb.h
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (C) 2013-2014 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __BRCMSTB_H__
> +#define __BRCMSTB_H__
> +
> +void brcmstb_secondary_startup(void);
> +
> +#endif /* __BRCMSTB_H__ */
> diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S
>
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:
> +static DEFINE_SPINLOCK(boot_lock);
> +
> +static void brcmstb_secondary_init(unsigned int cpu)
> +{
> + /*
> + * Synchronise with the boot thread.
> + */
> + spin_lock(_lock);
> + spin_unlock(_lock);
> +}
> +
> +static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct *idle)
> +{
> + /*
> + * set synchronisation state between this boot processor
> + * and the secondary one
> + */
> + spin_lock(_lock);
> +
> + /* Bring up power to the core if necessary */
> + if (brcmstb_cpu_get_power_state(cpu) == 0)
> + brcmstb_cpu_power_on(cpu);
> +
> + brcmstb_cpu_boot(cpu);
> +
> + /*
> + * now the secondary core is starting up let it run its
> + * calibrations, then wait for it to finish
> + */
> + spin_unlock(_lock);
I've just read through this code (because it caused my allmodconfig to
break) and spotted this.
What function does boot_lock perform here? Please, don't quote the
comments (I know where the comments came from) but what I want to hear
is your comments about why you decided to retain this.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:
+static DEFINE_SPINLOCK(boot_lock);
+
+static void brcmstb_secondary_init(unsigned int cpu)
+{
+ /*
+ * Synchronise with the boot thread.
+ */
+ spin_lock(boot_lock);
+ spin_unlock(boot_lock);
+}
+
+static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+ /*
+ * set synchronisation state between this boot processor
+ * and the secondary one
+ */
+ spin_lock(boot_lock);
+
+ /* Bring up power to the core if necessary */
+ if (brcmstb_cpu_get_power_state(cpu) == 0)
+ brcmstb_cpu_power_on(cpu);
+
+ brcmstb_cpu_boot(cpu);
+
+ /*
+ * now the secondary core is starting up let it run its
+ * calibrations, then wait for it to finish
+ */
+ spin_unlock(boot_lock);
I've just read through this code (because it caused my allmodconfig to
break) and spotted this.
What function does boot_lock perform here? Please, don't quote the
comments (I know where the comments came from) but what I want to hear
is your comments about why you decided to retain this.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris
computersforpe...@gmail.com wrote:
From: Marc Carino marc.cee...@gmail.com
The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes.
This patch adds machine support for the ARM-based Broadcom SoCs.
Signed-off-by: Marc Carino marc.cee...@gmail.com
Signed-off-by: Brian Norris computersforpe...@gmail.com
---
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/mach-bcm/Kconfig | 14 ++
arch/arm/mach-bcm/Makefile | 5 +
arch/arm/mach-bcm/brcmstb.c | 28 +++
arch/arm/mach-bcm/brcmstb.h | 19 ++
arch/arm/mach-bcm/headsmp-brcmstb.S | 33
arch/arm/mach-bcm/platsmp-brcmstb.c | 363
7 files changed, 463 insertions(+)
create mode 100644 arch/arm/mach-bcm/brcmstb.c
create mode 100644 arch/arm/mach-bcm/brcmstb.h
create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S
create mode 100644 arch/arm/mach-bcm/platsmp-brcmstb.c
diff --git a/arch/arm/configs/multi_v7_defconfig
b/arch/arm/configs/multi_v7_defconfig
index 534836497998..bf0df396a3cf 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y
CONFIG_ARCH_BCM=y
CONFIG_ARCH_BCM_MOBILE=y
CONFIG_ARCH_BCM_5301X=y
+CONFIG_ARCH_BRCMSTB=y
CONFIG_ARCH_BERLIN=y
CONFIG_MACH_BERLIN_BG2=y
CONFIG_MACH_BERLIN_BG2CD=y
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 41c839167e87..0073633e7699 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -87,4 +87,18 @@ config ARCH_BCM_5301X
different SoC or with the older BCM47XX and BCM53XX based
network SoC using a MIPS CPU, they are supported by
arch/mips/bcm47xx
+config ARCH_BRCMSTB
+ bool Broadcom BCM7XXX based boards if ARCH_MULTI_V7
+ depends on MMU
This was implied, but there are some patches from Arnd in this area.
Does it really not build with !MMU?
+ select ARM_GIC
+ select MIGHT_HAVE_PCI
+ select HAVE_SMP
At least HAVE_SMP is for sure, but I think both of these are selected already .
+ select HAVE_ARM_ARCH_TIMER
+ help
+ Say Y if you intend to run the kernel on a Broadcom ARM-based STB
+ chipset.
+
+ This enables support for Broadcom ARM-based set-top box chipsets,
+ including the 7445 family of chips.
+
endif
diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
index 731292114975..f3665121729b 100644
--- a/arch/arm/mach-bcm/Makefile
+++ b/arch/arm/mach-bcm/Makefile
@@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835)+= board_bcm2835.o
# BCM5301X
obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o
+
+ifeq ($(CONFIG_ARCH_BRCMSTB),y)
+obj-y += brcmstb.o
+obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o
+endif
diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c
new file mode 100644
index ..60a5afa06ed7
--- /dev/null
+++ b/arch/arm/mach-bcm/brcmstb.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/init.h
+#include linux/of_platform.h
+
+#include asm/mach-types.h
+#include asm/mach/arch.h
+
+static const char *brcmstb_match[] __initconst = {
+ brcm,bcm7445,
+ brcm,brcmstb,
+ NULL
+};
+
+DT_MACHINE_START(BRCMSTB, Broadcom STB (Flattened Device Tree))
+ .dt_compat = brcmstb_match,
+MACHINE_END
Do you plan to add more here? Otherwise you don't need this file.
diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h
new file mode 100644
index ..ec0c3d112b36
--- /dev/null
+++ b/arch/arm/mach-bcm/brcmstb.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __BRCMSTB_H__
+#define __BRCMSTB_H__
+
+void brcmstb_secondary_startup(void);
+
+#endif /* __BRCMSTB_H__ */
diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Rob,
I appreciate your comments, but where were many of these 5 months ago on
the first 7 revisions? :)
On a practical note: v9 is already queued for 3.17. Should I send
patches for the 3.17 cycle (or later) to fixup some of these issues? Or
would you recommend pulling the patches out of Matt Porter's tree now,
and reintroducing for 3.18? (I would be much happier with the first.)
I do note that we at least need to fix allmodconfig ASAP; I'll reply to
Russell on that one.
On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote:
On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris computersforpe...@gmail.com
wrote:
From: Marc Carino marc.cee...@gmail.com
The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes.
This patch adds machine support for the ARM-based Broadcom SoCs.
Signed-off-by: Marc Carino marc.cee...@gmail.com
Signed-off-by: Brian Norris computersforpe...@gmail.com
---
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/mach-bcm/Kconfig | 14 ++
arch/arm/mach-bcm/Makefile | 5 +
arch/arm/mach-bcm/brcmstb.c | 28 +++
arch/arm/mach-bcm/brcmstb.h | 19 ++
arch/arm/mach-bcm/headsmp-brcmstb.S | 33
arch/arm/mach-bcm/platsmp-brcmstb.c | 363
7 files changed, 463 insertions(+)
create mode 100644 arch/arm/mach-bcm/brcmstb.c
create mode 100644 arch/arm/mach-bcm/brcmstb.h
create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S
create mode 100644 arch/arm/mach-bcm/platsmp-brcmstb.c
diff --git a/arch/arm/configs/multi_v7_defconfig
b/arch/arm/configs/multi_v7_defconfig
index 534836497998..bf0df396a3cf 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y
CONFIG_ARCH_BCM=y
CONFIG_ARCH_BCM_MOBILE=y
CONFIG_ARCH_BCM_5301X=y
+CONFIG_ARCH_BRCMSTB=y
CONFIG_ARCH_BERLIN=y
CONFIG_MACH_BERLIN_BG2=y
CONFIG_MACH_BERLIN_BG2CD=y
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 41c839167e87..0073633e7699 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -87,4 +87,18 @@ config ARCH_BCM_5301X
different SoC or with the older BCM47XX and BCM53XX based
network SoC using a MIPS CPU, they are supported by
arch/mips/bcm47xx
+config ARCH_BRCMSTB
+ bool Broadcom BCM7XXX based boards if ARCH_MULTI_V7
+ depends on MMU
This was implied, but there are some patches from Arnd in this area.
Does it really not build with !MMU?
I suppose it probably builds, it likely won't run. I can drop this line
and then reassess if ARCH_MULTIPLATFORM becomes buildable with !MMU.
+ select ARM_GIC
+ select MIGHT_HAVE_PCI
+ select HAVE_SMP
At least HAVE_SMP is for sure, but I think both of these are selected already
.
You're correct. ARCH_MULTIPLATFORM selects MIGHT_HAVE_PCI and
ARCH_MULTI_V7 selects HAVE_SMP. I can look at dropping these.
+ select HAVE_ARM_ARCH_TIMER
+ help
+ Say Y if you intend to run the kernel on a Broadcom ARM-based STB
+ chipset.
+
+ This enables support for Broadcom ARM-based set-top box chipsets,
+ including the 7445 family of chips.
+
endif
diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
index 731292114975..f3665121729b 100644
--- a/arch/arm/mach-bcm/Makefile
+++ b/arch/arm/mach-bcm/Makefile
@@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835)+= board_bcm2835.o
# BCM5301X
obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o
+
+ifeq ($(CONFIG_ARCH_BRCMSTB),y)
+obj-y += brcmstb.o
+obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o
+endif
diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c
new file mode 100644
index ..60a5afa06ed7
--- /dev/null
+++ b/arch/arm/mach-bcm/brcmstb.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/init.h
+#include linux/of_platform.h
+
+#include asm/mach-types.h
+#include asm/mach/arch.h
+
+static const char *brcmstb_match[] __initconst = {
+ brcm,bcm7445,
+ brcm,brcmstb,
+ NULL
+};
+
+DT_MACHINE_START(BRCMSTB, Broadcom STB (Flattened Device Tree))
+ .dt_compat = brcmstb_match,
+MACHINE_END
Do you plan to add more here? Otherwise you don't need
Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
Hi Russell,
On Wed, Jul 30, 2014 at 10:26:35AM +0100, Russell King wrote:
On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:
+static DEFINE_SPINLOCK(boot_lock);
+
+static void brcmstb_secondary_init(unsigned int cpu)
+{
+ /*
+* Synchronise with the boot thread.
+*/
+ spin_lock(boot_lock);
+ spin_unlock(boot_lock);
+}
+
+static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct
*idle)
+{
+ /*
+* set synchronisation state between this boot processor
+* and the secondary one
+*/
+ spin_lock(boot_lock);
+
+ /* Bring up power to the core if necessary */
+ if (brcmstb_cpu_get_power_state(cpu) == 0)
+ brcmstb_cpu_power_on(cpu);
+
+ brcmstb_cpu_boot(cpu);
+
+ /*
+* now the secondary core is starting up let it run its
+* calibrations, then wait for it to finish
+*/
+ spin_unlock(boot_lock);
I've just read through this code (because it caused my allmodconfig to
break) and spotted this.
Sorry about the allmodconfig problems. I never compile-tested with ARMv6
enabled. This look OK?
diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
index f3665121729b..5ce82b4ba931 100644
--- a/arch/arm/mach-bcm/Makefile
+++ b/arch/arm/mach-bcm/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_BCM2835)+= board_bcm2835.o
obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o
ifeq ($(CONFIG_ARCH_BRCMSTB),y)
+CFLAGS_platsmp-brcmstb.o += -march=armv7-a
obj-y += brcmstb.o
obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o
endif
What function does boot_lock perform here? Please, don't quote the
comments (I know where the comments came from) but what I want to hear
is your comments about why you decided to retain this.
You might glean a little more from my response to Rob, but I'm not sure
there was a good reason for retaining this. We do need to be sure the
CPU is fully powered online before bringing it out of reset, but the
spinlock seems overkill AFAICT.
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
From: Marc Carino
The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes.
This patch adds machine support for the ARM-based Broadcom SoCs.
Signed-off-by: Marc Carino
Signed-off-by: Brian Norris
---
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/mach-bcm/Kconfig | 14 ++
arch/arm/mach-bcm/Makefile | 5 +
arch/arm/mach-bcm/brcmstb.c | 28 +++
arch/arm/mach-bcm/brcmstb.h | 19 ++
arch/arm/mach-bcm/headsmp-brcmstb.S | 33
arch/arm/mach-bcm/platsmp-brcmstb.c | 363
7 files changed, 463 insertions(+)
create mode 100644 arch/arm/mach-bcm/brcmstb.c
create mode 100644 arch/arm/mach-bcm/brcmstb.h
create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S
create mode 100644 arch/arm/mach-bcm/platsmp-brcmstb.c
diff --git a/arch/arm/configs/multi_v7_defconfig
b/arch/arm/configs/multi_v7_defconfig
index 534836497998..bf0df396a3cf 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y
CONFIG_ARCH_BCM=y
CONFIG_ARCH_BCM_MOBILE=y
CONFIG_ARCH_BCM_5301X=y
+CONFIG_ARCH_BRCMSTB=y
CONFIG_ARCH_BERLIN=y
CONFIG_MACH_BERLIN_BG2=y
CONFIG_MACH_BERLIN_BG2CD=y
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 41c839167e87..0073633e7699 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -87,4 +87,18 @@ config ARCH_BCM_5301X
different SoC or with the older BCM47XX and BCM53XX based
network SoC using a MIPS CPU, they are supported by arch/mips/bcm47xx
+config ARCH_BRCMSTB
+ bool "Broadcom BCM7XXX based boards" if ARCH_MULTI_V7
+ depends on MMU
+ select ARM_GIC
+ select MIGHT_HAVE_PCI
+ select HAVE_SMP
+ select HAVE_ARM_ARCH_TIMER
+ help
+ Say Y if you intend to run the kernel on a Broadcom ARM-based STB
+ chipset.
+
+ This enables support for Broadcom ARM-based set-top box chipsets,
+ including the 7445 family of chips.
+
endif
diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
index 731292114975..f3665121729b 100644
--- a/arch/arm/mach-bcm/Makefile
+++ b/arch/arm/mach-bcm/Makefile
@@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835)+= board_bcm2835.o
# BCM5301X
obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o
+
+ifeq ($(CONFIG_ARCH_BRCMSTB),y)
+obj-y += brcmstb.o
+obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o
+endif
diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c
new file mode 100644
index ..60a5afa06ed7
--- /dev/null
+++ b/arch/arm/mach-bcm/brcmstb.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+
+#include
+#include
+
+static const char *brcmstb_match[] __initconst = {
+ "brcm,bcm7445",
+ "brcm,brcmstb",
+ NULL
+};
+
+DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)")
+ .dt_compat = brcmstb_match,
+MACHINE_END
diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h
new file mode 100644
index ..ec0c3d112b36
--- /dev/null
+++ b/arch/arm/mach-bcm/brcmstb.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __BRCMSTB_H__
+#define __BRCMSTB_H__
+
+void brcmstb_secondary_startup(void);
+
+#endif /* __BRCMSTB_H__ */
diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S
b/arch/arm/mach-bcm/headsmp-brcmstb.S
new file mode 100644
index ..199c1ea58248
--- /dev/null
+++ b/arch/arm/mach-bcm/headsmp-brcmstb.S
@@ -0,0 +1,33 @@
+/*
+ * SMP boot code for secondary CPUs
+ * Based on arch/arm/mach-tegra/headsmp.S
+ *
+ * Copyright (C) 2010 NVIDIA, Inc.
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ *
[PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
From: Marc Carino marc.cee...@gmail.com
The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes.
This patch adds machine support for the ARM-based Broadcom SoCs.
Signed-off-by: Marc Carino marc.cee...@gmail.com
Signed-off-by: Brian Norris computersforpe...@gmail.com
---
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/mach-bcm/Kconfig | 14 ++
arch/arm/mach-bcm/Makefile | 5 +
arch/arm/mach-bcm/brcmstb.c | 28 +++
arch/arm/mach-bcm/brcmstb.h | 19 ++
arch/arm/mach-bcm/headsmp-brcmstb.S | 33
arch/arm/mach-bcm/platsmp-brcmstb.c | 363
7 files changed, 463 insertions(+)
create mode 100644 arch/arm/mach-bcm/brcmstb.c
create mode 100644 arch/arm/mach-bcm/brcmstb.h
create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S
create mode 100644 arch/arm/mach-bcm/platsmp-brcmstb.c
diff --git a/arch/arm/configs/multi_v7_defconfig
b/arch/arm/configs/multi_v7_defconfig
index 534836497998..bf0df396a3cf 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y
CONFIG_ARCH_BCM=y
CONFIG_ARCH_BCM_MOBILE=y
CONFIG_ARCH_BCM_5301X=y
+CONFIG_ARCH_BRCMSTB=y
CONFIG_ARCH_BERLIN=y
CONFIG_MACH_BERLIN_BG2=y
CONFIG_MACH_BERLIN_BG2CD=y
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 41c839167e87..0073633e7699 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -87,4 +87,18 @@ config ARCH_BCM_5301X
different SoC or with the older BCM47XX and BCM53XX based
network SoC using a MIPS CPU, they are supported by arch/mips/bcm47xx
+config ARCH_BRCMSTB
+ bool Broadcom BCM7XXX based boards if ARCH_MULTI_V7
+ depends on MMU
+ select ARM_GIC
+ select MIGHT_HAVE_PCI
+ select HAVE_SMP
+ select HAVE_ARM_ARCH_TIMER
+ help
+ Say Y if you intend to run the kernel on a Broadcom ARM-based STB
+ chipset.
+
+ This enables support for Broadcom ARM-based set-top box chipsets,
+ including the 7445 family of chips.
+
endif
diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
index 731292114975..f3665121729b 100644
--- a/arch/arm/mach-bcm/Makefile
+++ b/arch/arm/mach-bcm/Makefile
@@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835)+= board_bcm2835.o
# BCM5301X
obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o
+
+ifeq ($(CONFIG_ARCH_BRCMSTB),y)
+obj-y += brcmstb.o
+obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o
+endif
diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c
new file mode 100644
index ..60a5afa06ed7
--- /dev/null
+++ b/arch/arm/mach-bcm/brcmstb.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/init.h
+#include linux/of_platform.h
+
+#include asm/mach-types.h
+#include asm/mach/arch.h
+
+static const char *brcmstb_match[] __initconst = {
+ brcm,bcm7445,
+ brcm,brcmstb,
+ NULL
+};
+
+DT_MACHINE_START(BRCMSTB, Broadcom STB (Flattened Device Tree))
+ .dt_compat = brcmstb_match,
+MACHINE_END
diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h
new file mode 100644
index ..ec0c3d112b36
--- /dev/null
+++ b/arch/arm/mach-bcm/brcmstb.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __BRCMSTB_H__
+#define __BRCMSTB_H__
+
+void brcmstb_secondary_startup(void);
+
+#endif /* __BRCMSTB_H__ */
diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S
b/arch/arm/mach-bcm/headsmp-brcmstb.S
new file mode 100644
index ..199c1ea58248
--- /dev/null
+++ b/arch/arm/mach-bcm/headsmp-brcmstb.S
@@ -0,0 +1,33 @@
+/*
+ * SMP boot code for secondary CPUs
+ * Based on arch/arm/mach-tegra/headsmp.S
+ *
+ * Copyright (C) 2010 NVIDIA, Inc.
+ * Copyright (C) 2013-2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published
