From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932215AbdEJSDB (ORCPT ); Wed, 10 May 2017 14:03:01 -0400 Received: from anholt.net ([50.246.234.109]:46444 "EHLO anholt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932193AbdEJSC6 (ORCPT ); Wed, 10 May 2017 14:02:58 -0400 From: Eric Anholt To: Florian Fainelli , Phil Elwell , Marc Zyngier Cc: Thomas Gleixner , Jason Cooper , Florian Fainelli , Ray Jui , Scott Branden , bcm-kernel-feedback-list@broadcom.com, linux-kernel@vger.kernel.org, linux-rpi-kernel@lists.infradead.org Subject: Re: [PATCH] irq_bcm2836: Send event when onlining sleeping cores In-Reply-To: <817a2689-15df-f2e4-3b3d-e5831fba7fa3@gmail.com> References: <87ziemm0fb.fsf@eliezer.anholt.net> <1e871525-3298-f16d-4f95-a44bc78068b5@arm.com> <87o9v1c37r.fsf@eliezer.anholt.net> <7f6f5e05-07b1-e64d-401c-a189623c22a0@arm.com> <3b9a0ec6-fc9e-5cd0-e40a-e91b5926f53e@raspberrypi.org> <93a69cf4-cbf6-e0a7-b7e3-d0e53040efe4@arm.com> <187815f0-1316-ee87-883d-11304064f8f9@raspberrypi.org> <06fad93c-a850-a99a-ae60-7306f9179032@arm.com> <87tw4thyzu.fsf@on-the-bus.cambridge.arm.com> <907820fc-c100-dad4-cf4f-95aa330667c5@arm.com> <1bbd3527-3328-b9c0-e9e0-d37092850a91@raspberrypi.org> <817a2689-15df-f2e4-3b3d-e5831fba7fa3@gmail.com> User-Agent: Notmuch/0.22.2+1~gb0bcfaa (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Wed, 10 May 2017 11:02:44 -0700 Message-ID: <87shkc1tff.fsf@eliezer.anholt.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Florian Fainelli writes: > On 05/10/2017 03:31 AM, Phil Elwell wrote: >> On 10/05/2017 11:09, Marc Zyngier wrote: >>> On 10/05/17 10:05, Phil Elwell wrote: >>>> On 10/05/2017 09:55, Marc Zyngier wrote: >>>>> On Wed, May 10 2017 at 9:27:10 am BST, Phil Elwell wrote: >>>>>> On 10/05/2017 08:42, Marc Zyngier wrote: >>>>>>> On 09/05/17 20:02, Phil Elwell wrote: >>>>>>>> On 09/05/2017 19:53, Marc Zyngier wrote: >>>>>>>>> On 09/05/17 19:52, Phil Elwell wrote: >>>>>>>>>> On 09/05/2017 19:14, Marc Zyngier wrote: >>>>>>>>>>> On 09/05/17 19:08, Eric Anholt wrote: >>>>>>>>>>>> Marc Zyngier writes: >>>>>>>>>>>> >>>>>>>>>>>>> On 09/05/17 17:59, Eric Anholt wrote: >>>>>>>>>>>>>> Phil Elwell writes: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> In order to reduce power consumption and bus traffic, it is= sensible >>>>>>>>>>>>>>> for secondary cores to enter a low-power idle state when wa= iting to >>>>>>>>>>>>>>> be started. The wfe instruction causes a core to wait until= an event >>>>>>>>>>>>>>> or interrupt arrives before continuing to the next instruct= ion. >>>>>>>>>>>>>>> The sev instruction sends a wakeup event to the other cores= , so call >>>>>>>>>>>>>>> it from bcm2836_smp_boot_secondary, the function that wakes= up the >>>>>>>>>>>>>>> waiting cores during booting. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> It is harmless to use this patch without the corresponding = change >>>>>>>>>>>>>>> adding wfe to the ARMv7/ARMv8-32 stubs, but if the stubs ar= e updated >>>>>>>>>>>>>>> and this patch is not applied then the other cores will sle= ep forever. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> See: https://github.com/raspberrypi/linux/issues/1989 >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Signed-off-by: Phil Elwell >>>>>>>>>>>>>>> --- >>>>>>>>>>>>>>> drivers/irqchip/irq-bcm2836.c | 3 +++ >>>>>>>>>>>>>>> 1 file changed, 3 insertions(+) >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchi= p/irq-bcm2836.c >>>>>>>>>>>>>>> index e10597c..6dccdf9 100644 >>>>>>>>>>>>>>> --- a/drivers/irqchip/irq-bcm2836.c >>>>>>>>>>>>>>> +++ b/drivers/irqchip/irq-bcm2836.c >>>>>>>>>>>>>>> @@ -248,6 +248,9 @@ static int __init bcm2836_smp_boot_seco= ndary(unsigned int cpu, >>>>>>>>>>>>>>> writel(secondary_startup_phys, >>>>>>>>>>>>>>> intc.base + LOCAL_MAILBOX3_SET0 + 16 * cpu); >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> + dsb(sy); /* Ensure write has completed before waking the = other CPUs */ >>>>>>>>>>>>>>> + sev(); >>>>>>>>>>>>>>> + >>>>>>>>>>>>>>> return 0; >>>>>>>>>>>>>>> } >>>>>>>>>>>>>> >>>>>>>>>>>>>> This is also the behavior that the standard arm64 spin-table >>>>>>>>>>>>>> method has, >>>>>>>>>>>>>> which we unfortunately can't quite use. >>>>>>>>>>>>> >>>>>>>>>>>>> And why is that so? Why do you have to reinvent the wheel (an= d hide the >>>>>>>>>>>>> cloned wheel in an interrupt controller driver)? >>>>>>>>>>>>> >>>>>>>>>>>>> That doesn't seem right to me. >>>>>>>>>>>> >>>>>>>>>>>> The armv8 stubs (firmware-supplied code in the low page that d= o the >>>>>>>>>>>> spinning) do actually implement arm64's spin-table method. It= 's the >>>>>>>>>>>> armv7 stubs that use these registers in the irqchip instead of= plain >>>>>>>>>>>> addresses in system memory. >>>>>>>>>>> >>>>>>>>>>> Let's put ARMv7 aside for the time being. If your firmware alre= ady >>>>>>>>>>> implements spin-tables, why don't you simply use that at least = on arm64? >>>>>>>>>> >>>>>>>>>> We do. >>>>>>>>> >>>>>>>>> Obviously not the way it is intended if you have to duplicate the= core >>>>>>>>> architectural code in the interrupt controller driver, which coul= dn't >>>>>>>>> care less. >>>>>>>> >>>>>>>> If we were using this method on arm64 then the other cores would n= ot start up >>>>>>>> because armstub8.S has always included a wfe. Nothing in the commi= t mentions >>>>>>>> arm64 - this is an ARCH=3Darm fix. >>>>>>> >>>>>>> Thanks for the clarification, which you could have added to the com= mit >>>>>>> message. >>>>>>> >>>>>>> The question still remains: why do we have CPU bring-up code in an >>>>>>> interrupt controller, instead of having it in the architecture code? >>>>>>> >>>>>>> The RPi-2 is the *only* platform to have its SMP bringup code outsi= de of >>>>>>> arch/arm, so the first course of action would be to move that code = where >>>>>>> it belongs. >>>>>> >>>>>> You were CC'd on the commit (41f4988cc287e5f836d3f6620c9f900bc9b560e= 9) that >>>>>> introduced bcm2836_smp_boot_secondary - it seems strange to start ob= jecting >>>>>> now. >>>>> >>>>> Well, I'm far from being perfect. If I had noticed it, I'd have NACKed >>>>> it. >>>>> >>>>>> Yes, I think it is odd that it didn't go into arch/arm/mach-bcm, but= in >>>>>> the interests of making changes in small, independent steps, do you = have a >>>>>> problem with this commit? >>>>> >>>>> On its own, no. I'm just not keen on adding more unrelated stuff to t= his >>>>> file, so let's start with dealing with the original bug, and you can >>>>> then add this fix on top. >>>> >>>> That's an interesting use of the word "bug". From Wikipedia: >>>> >>>> "A software bug is an error, flaw, failure or fault in a computer prog= ram or >>>> system that causes it to produce an incorrect or unexpected result, or= to >>>> behave in unintended ways." >>> >>> Whatever. Should I call it "pile of crap dumped in unsuitable locations" >>> instead? What does Wikipedia says about it? >>> >>>> Although your concerns are valid, the faults you are objecting to are = not causing >>>> a malfunction of any kind. If we were to update the RPi firmware befor= e this >>>> patch was merged then upstream users would be left with one wheel on t= heir wagon. >>> >>> And that'd be your problem, not mine. Look, you can argue around this >>> all day, or you can fix this mess. Your choice. >>=20 >> Is that the opinion of all here? > > The choice of word here got largely out of the original topic and I > surely did eat a ton of popcorn here. There are two things that need > fixing, and the time line and process for fixing these is clear: > > - your bugfix (Phil) is something that should be applied now, and > backported to -stable trees once the fix hits the irqchip tree (or Linus') > > - relocating the code that does the secondary boot out of > drivers/irqchip/ into arch/arm/mach-bcm/ needs to happen (Marc), and > this is 4.13 material, there is no urgency in doing this *right now*, > but it needs to happen > > Does that work for everyone? Agreed. This patch, which we'll want to go to -stable, should clearly go in first. Marc's patch can go in after, since it's not a -stable candidate. Thomas, could you add the cc to stable when picking this patch? --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlkTVcQACgkQtdYpNtH8 nug5xQ//eaizyPuKf90PC/lUO4KBrEyoqdW/sxX2i3Gm8CgFVhLyj+Heq56kIUWD KbexZiHZVtL7vg4iWN7jZnB6RBa9QuhbXjJ9GzKXPUY/1HLJYGsgzUFGkXXVPoiY KKLXoCO4vGmXvevWpA84AqrARDPytIlItswXhMykRW/wTP2MIUWtiAjLxXMj06p7 faTvPxrfDJV19iE0dUoWV94AdB17Vil/HlLMwpz8E6CnNzOIirWlIJI2uqoIe/2u rGUzk/DG0eOUvTIO1bhyh4s4jIYE1R9DJcxxFQN/k8iTe0Wo9cI3H+d72iSYELvp rL15YTuUgrGTGXolwb61z0CUbJ53bkRrff+h+R+UjIVlb1JS5TOTAlzIlzcKeoJN fXONGJqWycRkgUx2eYHyX1u6fboS1orP6G9Mq/L6BrceKeWju+u65bUZs0hN0PZj sj6U7VgmJ1CwYJLEKubjmW10gufN1+BMpXDUaR4++nMpDnOJ3egm9xAoPfGlfdLD 9iY9+n8VTIpUvIaLr18odZKihDUtnLDG/RmB4rrJzthJje/hgkHdVom/ucU52jkK zJpG+m/+PA/4hGRXWZ3wm+GEzNh478b6fTTV6WN03WEQ0iSzjBb2AIxoFA9NqN4y tZvjz8K4AwhXBG3i4Zl0oU37PO51P8Wkg1wRJ1llxnFlxcm+KtU= =oUCs -----END PGP SIGNATURE----- --=-=-=--