| Message ID | 1443824458-20799-4-git-send-email-f.fainelli@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Friday 02 October 2015 15:20:58 Florian Fainelli wrote: > This pull request contains the following Broadcom SoC platform and driver changes: > > - Brian Norris create a drivers/soc/brcmstb/ stub and then adds support for S2/S3/S5 > suspend/resume modes for the Broadcom BCM7xxx Set Top Box SoCs I'm not overly happy with this part of the code (sorry Brian): - it looks like it should be a cpuidle driver. Not completely sure about this, but I'd like to see at least an Ack from the cpuidle maintainers to confirm that they want it to be done in drivers/soc - any code here that is not going into a cpuidle driver for this part looks like it's better suited to go to arch/arm/mach-bcm, as the code doesn't feel like a driver. This is of course a gray area, and can be debated. - it's clearly not endian-safe. There really is no reason to use __raw mmio accessors here, or to define the interface to the firmware in terms of native endianess when the registers and firmware is known to be little-endian. Sorry for not noticing the driver earlier when it was discussed on the list, but I think the above is reason enough to do another revision. Can you submit a new pull request without the suspend/resume handling? Arnd
On 09/10/15 08:46, Arnd Bergmann wrote: > On Friday 02 October 2015 15:20:58 Florian Fainelli wrote: >> This pull request contains the following Broadcom SoC platform and driver changes: >> >> - Brian Norris create a drivers/soc/brcmstb/ stub and then adds support for S2/S3/S5 >> suspend/resume modes for the Broadcom BCM7xxx Set Top Box SoCs > > I'm not overly happy with this part of the code (sorry Brian): > > - it looks like it should be a cpuidle driver. Not completely sure about this, > but I'd like to see at least an Ack from the cpuidle maintainers to confirm > that they want it to be done in drivers/soc Could you clarify how you think this should be fitting in the cpuidle framework? This is system-wide S2/S3/S5 states we are talking about here, and that comes with specific constraints, like moving code from DRAM execution to SRAM execution for instance, not sure where cpuidle can help with that. And if it does, how we coordinate with that framework. > > - any code here that is not going into a cpuidle driver for this part looks > like it's better suited to go to arch/arm/mach-bcm, as the code doesn't > feel like a driver. This is of course a gray area, and can be debated. There are some large portions of this code that are shared between SoCs, past, current and future chips, with the exception of the small assembly part which needs to be architecture specific for obvious reasons. For instance, the power state machine code is fairly SoC-independant, and to some extent, the DDR controller code is as well, that is what motivated putting that code here, so it can be re-used in the future when we submit support for new chips as well. > > - it's clearly not endian-safe. There really is no reason to use __raw > mmio accessors here, or to define the interface to the firmware in terms > of native endianess when the registers and firmware is known to be > little-endian. Ok, that one is one me, I should have made sure it was. > > Sorry for not noticing the driver earlier when it was discussed on the list, > but I think the above is reason enough to do another revision. > Can you submit a new pull request without the suspend/resume handling? I can do that, but then I am really expecting someone to take a deep look at the implementation and make some educated recommendations about how this should be pieced together to achieve the expected ACPI-like suspend states. Thanks
On Friday 09 October 2015 10:01:39 Florian Fainelli wrote: > On 09/10/15 08:46, Arnd Bergmann wrote: > > On Friday 02 October 2015 15:20:58 Florian Fainelli wrote: > >> This pull request contains the following Broadcom SoC platform and driver changes: > >> > >> - Brian Norris create a drivers/soc/brcmstb/ stub and then adds support for S2/S3/S5 > >> suspend/resume modes for the Broadcom BCM7xxx Set Top Box SoCs > > > > I'm not overly happy with this part of the code (sorry Brian): > > > > - it looks like it should be a cpuidle driver. Not completely sure about this, > > but I'd like to see at least an Ack from the cpuidle maintainers to confirm > > that they want it to be done in drivers/soc > > Could you clarify how you think this should be fitting in the cpuidle > framework? This is system-wide S2/S3/S5 states we are talking about > here, and that comes with specific constraints, like moving code from > DRAM execution to SRAM execution for instance, not sure where cpuidle > can help with that. And if it does, how we coordinate with that framework. Ok, I see. So probably not cpuidle then, but I'd still have a better feeling if this was reviewed by someone who understands power management and is outside of the brcmstb team. Maybe just send it to linux-pm to get feedback from the suspend maintainers? > > - any code here that is not going into a cpuidle driver for this part looks > > like it's better suited to go to arch/arm/mach-bcm, as the code doesn't > > feel like a driver. This is of course a gray area, and can be debated. > > There are some large portions of this code that are shared between SoCs, > past, current and future chips, with the exception of the small assembly > part which needs to be architecture specific for obvious reasons. > > For instance, the power state machine code is fairly SoC-independant, > and to some extent, the DDR controller code is as well, that is what > motivated putting that code here, so it can be re-used in the future > when we submit support for new chips as well. Maybe drivers/firmware/broadcom/ then? That directory already exists and the main parts seem to be for a firmware interface. The DDR controller stuff probably better fits into drivers/memory/, which already has similar bits from other platforms. Of course that implies restructuring the code a bit to separate the two. Arnd
The following changes since commit 6ff33f3902c3b1c5d0db6b1e2c70b6d76fba357f: Linux 4.3-rc1 (2015-09-12 16:35:56 -0700) are available in the git repository at: http://github.com/Broadcom/stblinux.git tags/arm-soc/for-4.4/soc for you to fetch changes up to 7333ecdcd71fbbed1e2cab675fd129ab99f4db41: ARM: brcmstb: Setup BIU control registers during boot (2015-09-18 12:21:17 -0700) ---------------------------------------------------------------- This pull request contains the following Broadcom SoC platform and driver changes: - Brian Norris create a drivers/soc/brcmstb/ stub and then adds support for S2/S3/S5 suspend/resume modes for the Broadcom BCM7xxx Set Top Box SoCs - Florian Fainelli adds support for configuring the BCM7xxx SoCs Bus Interface Unit with their specific write-pairing setting, which must be saved and restored during system-wide suspend/resume, and consequently updates the brcmstb machine code to initialize the BIU - Jon Mason adds support for the Northstar Plus SoCs by introducing a custom machine descriptor matching their compatible string and setting up the PL310 L2 cache and enabling the relevant ARM errata for their Cortex-A9 ---------------------------------------------------------------- Brian Norris (2): soc: add stubs for brcmstb SoC's soc: brcmstb: add PM suspend/resume support (S2/S3/S5) Florian Fainelli (2): soc: brcmstb: Add Bus Interface Unit control setup ARM: brcmstb: Setup BIU control registers during boot Jon Mason (1): ARM: NSP: Add basic support for Broadcom Northstar Plus SoC arch/arm/mach-bcm/Kconfig | 15 ++ arch/arm/mach-bcm/Makefile | 5 +- arch/arm/mach-bcm/bcm_nsp.c | 25 ++ arch/arm/mach-bcm/brcmstb.c | 9 + drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/brcmstb/Kconfig | 19 ++ drivers/soc/brcmstb/Makefile | 3 + drivers/soc/brcmstb/biuctrl.c | 116 ++++++++ drivers/soc/brcmstb/common.c | 33 +++ drivers/soc/brcmstb/pm/Makefile | 2 + drivers/soc/brcmstb/pm/aon_defs.h | 85 ++++++ drivers/soc/brcmstb/pm/pm.c | 512 ++++++++++++++++++++++++++++++++++++ drivers/soc/brcmstb/pm/pm.h | 40 +++ drivers/soc/brcmstb/pm/s2.S | 73 +++++ include/linux/soc/brcmstb/brcmstb.h | 10 + include/soc/brcmstb/common.h | 15 ++ 17 files changed, 963 insertions(+), 1 deletion(-) create mode 100644 arch/arm/mach-bcm/bcm_nsp.c create mode 100644 drivers/soc/brcmstb/Kconfig create mode 100644 drivers/soc/brcmstb/Makefile create mode 100644 drivers/soc/brcmstb/biuctrl.c create mode 100644 drivers/soc/brcmstb/common.c create mode 100644 drivers/soc/brcmstb/pm/Makefile create mode 100644 drivers/soc/brcmstb/pm/aon_defs.h create mode 100644 drivers/soc/brcmstb/pm/pm.c create mode 100644 drivers/soc/brcmstb/pm/pm.h create mode 100644 drivers/soc/brcmstb/pm/s2.S create mode 100644 include/linux/soc/brcmstb/brcmstb.h create mode 100644 include/soc/brcmstb/common.h