| Message ID | 1431556453-11633-3-git-send-email-f.fainelli@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wednesday 13 May 2015 15:34:13 Florian Fainelli wrote: > This pull request contains the following changes: > > - BCM63138 SMP support which pulls in code to control the PMB bus, secondary CPU initialization > sequence and small changes suggested by Russell King to allow platforms to disable VFP > > ---------------------------------------------------------------- > Florian Fainelli (5): > ARM: BCM63xx: Add Broadcom BCM63xx PMB controller helpers > ARM: BCM63xx: Add secondary CPU PMB initialization sequence > ARM: vfp: Add include guards > ARM: vfp: Add vfp_disable for problematic platforms > ARM: BCM63xx: Add SMP support for BCM63138 > > Pulled into next/soc, thanks! Arnd
On Wednesday 13 May 2015 15:34:13 Florian Fainelli wrote: > This pull request contains the following changes: > > - BCM63138 SMP support which pulls in code to control the PMB bus, secondary CPU initialization > sequence and small changes suggested by Russell King to allow platforms to disable VFP > Pulled into next/soc now, sorry for missing it last week. Arnd
On Wednesday 13 May 2015 15:34:13 Florian Fainelli wrote: > arch/arm/include/asm/vfp.h | 9 ++ > arch/arm/mach-bcm/Makefile | 7 +- > arch/arm/mach-bcm/bcm63xx_headsmp.S | 23 ++++ > arch/arm/mach-bcm/bcm63xx_pmb.c | 221 ++++++++++++++++++++++++++++++++++++ > arch/arm/mach-bcm/bcm63xx_smp.c | 170 +++++++++++++++++++++++++++ > arch/arm/mach-bcm/bcm63xx_smp.h | 9 ++ > arch/arm/vfp/vfpmodule.c | 13 +++ > include/linux/bcm63xx_pmb.h | 76 +++++++++++++ > And taken out again, sorry. I got a build error in include/linux/bcm63xx_pmb.h and that triggered me to take a closer look. I tried to fix up the trivial error first, but then noticed several other things wrong with bcm63xx_pmb.h: - it is in include/linux/ where it clearly does not belong, as no other component should be including it. Even the function documentation in there mentions that one must hold the pmb_lock before calling it, and that is defined statically arch/arm/mach-bcm/bcm63xx_pmb.c, so it's impossible for other code to use. Just move it all into bcm63xx_pmb.c. - the code uses __raw_readl/__raw_writel for MMIO register access, which is broken on big-endian machines. You need to use readl_relaxed()/write_relaxed() or readl()/writel() instead. If you use the latter, you can also avoid introducing extra barriers. - finally, a heads-up for the "[PATCH] ARM: v7 setup function should invalidate L1 cache" patch that Russell submitted. I think your bcm63xx_headsmp.S function is completely fine for now, but with his patch applied, it will no longer be needed and you should follow up with a patch to replace it with a direct branch to secondary_startup. Arnd
On 20/05/15 13:59, Arnd Bergmann wrote: > On Wednesday 13 May 2015 15:34:13 Florian Fainelli wrote: >> arch/arm/include/asm/vfp.h | 9 ++ >> arch/arm/mach-bcm/Makefile | 7 +- >> arch/arm/mach-bcm/bcm63xx_headsmp.S | 23 ++++ >> arch/arm/mach-bcm/bcm63xx_pmb.c | 221 ++++++++++++++++++++++++++++++++++++ >> arch/arm/mach-bcm/bcm63xx_smp.c | 170 +++++++++++++++++++++++++++ >> arch/arm/mach-bcm/bcm63xx_smp.h | 9 ++ >> arch/arm/vfp/vfpmodule.c | 13 +++ >> include/linux/bcm63xx_pmb.h | 76 +++++++++++++ >> > > And taken out again, sorry. No problem, Rafal reminded me of a patch I forgot to submit earlier, so I will take that as an opportunity to re-do this pull request. > > I got a build error in include/linux/bcm63xx_pmb.h and that triggered me to take > a closer look. I tried to fix up the trivial error first, but then noticed > several other things wrong with bcm63xx_pmb.h: What kind of build error? Do you have it handy? > > - it is in include/linux/ where it clearly does not belong, as no other component > should be including it. Even the function documentation in there mentions that > one must hold the pmb_lock before calling it, and that is defined statically > arch/arm/mach-bcm/bcm63xx_pmb.c, so it's impossible for other code to use. > Just move it all into bcm63xx_pmb.c. This header will later be used by the bcm63138 reset controller, and I thought I had explained the reasoning behind that in either the commit message or cover letter, I will make sure the commit message explains it. The lock issue is not much of a problem since we do not support CPU hotplug and the reset controller subsystem is initialized later so there is no possibility for these two pieces of code to conflict. > > - the code uses __raw_readl/__raw_writel for MMIO register access, which is > broken on big-endian machines. You need to use readl_relaxed()/write_relaxed() > or readl()/writel() instead. If you use the latter, you can also avoid > introducing extra barriers. Fair point, will fix that. > > - finally, a heads-up for the "[PATCH] ARM: v7 setup function should invalidate > L1 cache" patch that Russell submitted. I think your bcm63xx_headsmp.S > function is completely fine for now, but with his patch applied, it will > no longer be needed and you should follow up with a patch to replace it > with a direct branch to secondary_startup. Thanks, I will keep that in mind.
On Wednesday 20 May 2015 14:05:35 Florian Fainelli wrote: > On 20/05/15 13:59, Arnd Bergmann wrote: > > On Wednesday 13 May 2015 15:34:13 Florian Fainelli wrote: > >> arch/arm/include/asm/vfp.h | 9 ++ > >> arch/arm/mach-bcm/Makefile | 7 +- > >> arch/arm/mach-bcm/bcm63xx_headsmp.S | 23 ++++ > >> arch/arm/mach-bcm/bcm63xx_pmb.c | 221 ++++++++++++++++++++++++++++++++++++ > >> arch/arm/mach-bcm/bcm63xx_smp.c | 170 +++++++++++++++++++++++++++ > >> arch/arm/mach-bcm/bcm63xx_smp.h | 9 ++ > >> arch/arm/vfp/vfpmodule.c | 13 +++ > >> include/linux/bcm63xx_pmb.h | 76 +++++++++++++ > >> > > > > And taken out again, sorry. > > No problem, Rafal reminded me of a patch I forgot to submit earlier, so > I will take that as an opportunity to re-do this pull request. > > > > > I got a build error in include/linux/bcm63xx_pmb.h and that triggered me to take > > a closer look. I tried to fix up the trivial error first, but then noticed > > several other things wrong with bcm63xx_pmb.h: > > What kind of build error? Do you have it handy? I sent it to you on IRC earlier, here is the full error log: /git/arm-soc/include/linux/bcm63xx_pmb.h: In function '__bpcm_do_op': /git/arm-soc/include/linux/bcm63xx_pmb.h:39:12: error: 'EIO' undeclared (first use in this function) return -EIO; ^ /git/arm-soc/include/linux/bcm63xx_pmb.h:39:12: note: each undeclared identifier is reported only once for each function it appears in /git/arm-soc/include/linux/bcm63xx_pmb.h:42:12: error: 'ETIMEDOUT' undeclared (first use in this function) return -ETIMEDOUT; ^ In file included from /git/arm-soc/arch/arm/mach-bcm/bcm63xx_pmb.c:15:0: /git/arm-soc/include/linux/bcm63xx_pmb.h:48:1: warning: control reaches end of non-void function [-Wreturn-type] This is fixed by including linux/errno.h in the header. > > - it is in include/linux/ where it clearly does not belong, as no other component > > should be including it. Even the function documentation in there mentions that > > one must hold the pmb_lock before calling it, and that is defined statically > > arch/arm/mach-bcm/bcm63xx_pmb.c, so it's impossible for other code to use. > > Just move it all into bcm63xx_pmb.c. > > This header will later be used by the bcm63138 reset controller, and I > thought I had explained the reasoning behind that in either the commit > message or cover letter, I will make sure the commit message explains it. I see. I still think it's a bit rude to place this header in the top-level include/linux directory though. I realize there are a lot of other headers like this, but I'm trying not to add too many more. Maybe a lesser evil would be to put the reset driver into arch/arm/mach-bcm/bcm63xx_pmb.c as well? How big is it? And is there anything else besides that driver which would need these functions? > The lock issue is not much of a problem since we do not support CPU > hotplug and the reset controller subsystem is initialized later so there > is no possibility for these two pieces of code to conflict. Ok Arnd
On 20/05/15 14:22, Arnd Bergmann wrote: > On Wednesday 20 May 2015 14:05:35 Florian Fainelli wrote: >> On 20/05/15 13:59, Arnd Bergmann wrote: >>> On Wednesday 13 May 2015 15:34:13 Florian Fainelli wrote: >>>> arch/arm/include/asm/vfp.h | 9 ++ >>>> arch/arm/mach-bcm/Makefile | 7 +- >>>> arch/arm/mach-bcm/bcm63xx_headsmp.S | 23 ++++ >>>> arch/arm/mach-bcm/bcm63xx_pmb.c | 221 ++++++++++++++++++++++++++++++++++++ >>>> arch/arm/mach-bcm/bcm63xx_smp.c | 170 +++++++++++++++++++++++++++ >>>> arch/arm/mach-bcm/bcm63xx_smp.h | 9 ++ >>>> arch/arm/vfp/vfpmodule.c | 13 +++ >>>> include/linux/bcm63xx_pmb.h | 76 +++++++++++++ >>>> >>> >>> And taken out again, sorry. >> >> No problem, Rafal reminded me of a patch I forgot to submit earlier, so >> I will take that as an opportunity to re-do this pull request. >> >>> >>> I got a build error in include/linux/bcm63xx_pmb.h and that triggered me to take >>> a closer look. I tried to fix up the trivial error first, but then noticed >>> several other things wrong with bcm63xx_pmb.h: >> >> What kind of build error? Do you have it handy? > > I sent it to you on IRC earlier, here is the full error log: > > /git/arm-soc/include/linux/bcm63xx_pmb.h: In function '__bpcm_do_op': > /git/arm-soc/include/linux/bcm63xx_pmb.h:39:12: error: 'EIO' undeclared (first use in this function) > return -EIO; > ^ > /git/arm-soc/include/linux/bcm63xx_pmb.h:39:12: note: each undeclared identifier is reported only once for each function it appears in > /git/arm-soc/include/linux/bcm63xx_pmb.h:42:12: error: 'ETIMEDOUT' undeclared (first use in this function) > return -ETIMEDOUT; > ^ > In file included from /git/arm-soc/arch/arm/mach-bcm/bcm63xx_pmb.c:15:0: > /git/arm-soc/include/linux/bcm63xx_pmb.h:48:1: warning: control reaches end of non-void function [-Wreturn-type] > > This is fixed by including linux/errno.h in the header. Yes, thanks I just reproduced it, not sure how I could have missed that before... > >>> - it is in include/linux/ where it clearly does not belong, as no other component >>> should be including it. Even the function documentation in there mentions that >>> one must hold the pmb_lock before calling it, and that is defined statically >>> arch/arm/mach-bcm/bcm63xx_pmb.c, so it's impossible for other code to use. >>> Just move it all into bcm63xx_pmb.c. >> >> This header will later be used by the bcm63138 reset controller, and I >> thought I had explained the reasoning behind that in either the commit >> message or cover letter, I will make sure the commit message explains it. > > I see. I still think it's a bit rude to place this header in the top-level > include/linux directory though. I realize there are a lot of other headers > like this, but I'm trying not to add too many more. Would a lesser evil be to create include/linux/resets/ and place this header file there? > > Maybe a lesser evil would be to put the reset driver into > arch/arm/mach-bcm/bcm63xx_pmb.c as well? > > How big is it? And is there anything else besides that driver which would > need these functions? It is going to be (once feature complete) as big as arch/arm/mach-bcm/bcm63xx_pmb.c except that it will also have to inspect the client asking for the reset, e.g: the reset procedure for the SATA block is a little different than the one for the PCIe PHY, or integrated Ethernet switch, or USB controlers... The way we power on a secondary CPU is code that is not shared with how other on-chip peripherals are powered on, hence the idea behind the separation. > >> The lock issue is not much of a problem since we do not support CPU >> hotplug and the reset controller subsystem is initialized later so there >> is no possibility for these two pieces of code to conflict. > > Ok > > Arnd >
On Wednesday 20 May 2015 14:50:37 Florian Fainelli wrote: > >>> - it is in include/linux/ where it clearly does not belong, as no other component > >>> should be including it. Even the function documentation in there mentions that > >>> one must hold the pmb_lock before calling it, and that is defined statically > >>> arch/arm/mach-bcm/bcm63xx_pmb.c, so it's impossible for other code to use. > >>> Just move it all into bcm63xx_pmb.c. > >> > >> This header will later be used by the bcm63138 reset controller, and I > >> thought I had explained the reasoning behind that in either the commit > >> message or cover letter, I will make sure the commit message explains it. > > > > I see. I still think it's a bit rude to place this header in the top-level > > include/linux directory though. I realize there are a lot of other headers > > like this, but I'm trying not to add too many more. > > Would a lesser evil be to create include/linux/resets/ and place this > header file there? I've also thought about that. It would certainly help. > > Maybe a lesser evil would be to put the reset driver into > > arch/arm/mach-bcm/bcm63xx_pmb.c as well? > > > > How big is it? And is there anything else besides that driver which would > > need these functions? > > It is going to be (once feature complete) as big as > arch/arm/mach-bcm/bcm63xx_pmb.c except that it will also have to inspect > the client asking for the reset, e.g: the reset procedure for the SATA > block is a little different than the one for the PCIe PHY, or integrated > Ethernet switch, or USB controlers... > > The way we power on a secondary CPU is code that is not shared with how > other on-chip peripherals are powered on, hence the idea behind the > separation. Ok, I see. Let's start with the include/linux/resets/ approach then. Yet another idea would be to expose the read/write interface here as a regmap and find a way to share that. That way, you could move a large part of bcm63xx_pmb.c and bcm63xx_pmb.h into the reset driver and just have one interface to get the regmap, like we do for syscon devices. It would still need a single function declaration though. Arnd
The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031: Linux 4.1-rc1 (2015-04-26 17:59:10 -0700) are available in the git repository at: http://github.com/broadcom/stblinux tags/arm-soc/for-4.2/soc for you to fetch changes up to d044ce725eea4d7b74bcb6e9e7b33996aae77f04: ARM: BCM63xx: Add SMP support for BCM63138 (2015-05-13 09:59:07 -0700) ---------------------------------------------------------------- This pull request contains the following changes: - BCM63138 SMP support which pulls in code to control the PMB bus, secondary CPU initialization sequence and small changes suggested by Russell King to allow platforms to disable VFP ---------------------------------------------------------------- Florian Fainelli (5): ARM: BCM63xx: Add Broadcom BCM63xx PMB controller helpers ARM: BCM63xx: Add secondary CPU PMB initialization sequence ARM: vfp: Add include guards ARM: vfp: Add vfp_disable for problematic platforms ARM: BCM63xx: Add SMP support for BCM63138 arch/arm/include/asm/vfp.h | 9 ++ arch/arm/mach-bcm/Makefile | 7 +- arch/arm/mach-bcm/bcm63xx_headsmp.S | 23 ++++ arch/arm/mach-bcm/bcm63xx_pmb.c | 221 ++++++++++++++++++++++++++++++++++++ arch/arm/mach-bcm/bcm63xx_smp.c | 170 +++++++++++++++++++++++++++ arch/arm/mach-bcm/bcm63xx_smp.h | 9 ++ arch/arm/vfp/vfpmodule.c | 13 +++ include/linux/bcm63xx_pmb.h | 76 +++++++++++++ 8 files changed, 527 insertions(+), 1 deletion(-) create mode 100644 arch/arm/mach-bcm/bcm63xx_headsmp.S create mode 100644 arch/arm/mach-bcm/bcm63xx_pmb.c create mode 100644 arch/arm/mach-bcm/bcm63xx_smp.c create mode 100644 arch/arm/mach-bcm/bcm63xx_smp.h create mode 100644 include/linux/bcm63xx_pmb.h