[PATCH 0/4] Broadcom STB PM PSCI extensions

[PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

Hi all,

This patch series contains the Broadcom STB PSCI extensions which adds
some additional functions on top of the existing standard PSCI interface
which is the reason for having the driver implement a custom
suspend_ops.

These platforms have traditionally supported a mode that is akin to
ACPI's S2 with the CPU in WFI and all of the chip being clock gated
which is entered with "echo standby > /sys/power/state". Additional a
true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
mem > /sys/power/state".

These platforms also may have an external Broadcom PMIC chip which can
cause the SoC to be powercycled assuming that we communicate that intent
via a vendor specific PSCI SYSTEM_RESET2.

Since it is desirable to get any new functionality added to the kernel
to be loadable as a module as part of shipping said products in a Google
Kernel Image (GKI) environment, we need to export a couple of symbols from
drivers/firmware/psci/psci.c.

Thanks for your feedback.

Florian Fainelli (4):
  firmware: psci: Export a couple of suspend symbols
  soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW
  soc: bcm: brcmstb: Added support for PSCI system suspend operations
  Documentation: ABI: Document Broadcom STB PSCI firmware files

 .../ABI/testing/sysfs-firmware-brcmstb        |  16 +
 drivers/firmware/psci/psci.c                  |   9 +-
 drivers/soc/bcm/brcmstb/Kconfig               |   4 +-
 drivers/soc/bcm/brcmstb/pm/Makefile           |   3 +
 drivers/soc/bcm/brcmstb/pm/pm-psci.c          | 315 ++++++++++++++++++
 include/linux/psci.h                          |   2 +
 include/linux/soc/brcmstb/brcmstb-smccc.h     |  84 +++++
 7 files changed, 430 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-brcmstb
 create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
 create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h

-- 
2.25.1

[PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

Hi all,

This patch series contains the Broadcom STB PSCI extensions which adds
some additional functions on top of the existing standard PSCI interface
which is the reason for having the driver implement a custom
suspend_ops.

These platforms have traditionally supported a mode that is akin to
ACPI's S2 with the CPU in WFI and all of the chip being clock gated
which is entered with "echo standby > /sys/power/state". Additional a
true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
mem > /sys/power/state".

These platforms also may have an external Broadcom PMIC chip which can
cause the SoC to be powercycled assuming that we communicate that intent
via a vendor specific PSCI SYSTEM_RESET2.

Since it is desirable to get any new functionality added to the kernel
to be loadable as a module as part of shipping said products in a Google
Kernel Image (GKI) environment, we need to export a couple of symbols from
drivers/firmware/psci/psci.c.

Thanks for your feedback.

Florian Fainelli (4):
  firmware: psci: Export a couple of suspend symbols
  soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW
  soc: bcm: brcmstb: Added support for PSCI system suspend operations
  Documentation: ABI: Document Broadcom STB PSCI firmware files

 .../ABI/testing/sysfs-firmware-brcmstb        |  16 +
 drivers/firmware/psci/psci.c                  |   9 +-
 drivers/soc/bcm/brcmstb/Kconfig               |   4 +-
 drivers/soc/bcm/brcmstb/pm/Makefile           |   3 +
 drivers/soc/bcm/brcmstb/pm/pm-psci.c          | 315 ++++++++++++++++++
 include/linux/psci.h                          |   2 +
 include/linux/soc/brcmstb/brcmstb-smccc.h     |  84 +++++
 7 files changed, 430 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-brcmstb
 create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
 create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] firmware: psci: Export a couple of suspend symbols

[Florian Fainelli]

Export psci_system_suspend_enter() and psci_cpu_suspend_enter() for
the Broadcom STB PSCI extension to be usable and built as a loadable
module.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/firmware/psci/psci.c | 9 ++++++++-
 include/linux/psci.h         | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index cfb448eabdaa..9a60e340c248 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -355,7 +355,13 @@ int psci_cpu_suspend_enter(u32 state)
 
 	return ret;
 }
+#else
+int psci_cpu_suspend_enter(u32 state)
+{
+	return -EOPNOTSUPP;
+}
 #endif
+EXPORT_SYMBOL_GPL(psci_cpu_suspend_enter);
 
 static int psci_system_suspend(unsigned long unused)
 {
@@ -365,10 +371,11 @@ static int psci_system_suspend(unsigned long unused)
 			      pa_cpu_resume, 0, 0);
 }
 
-static int psci_system_suspend_enter(suspend_state_t state)
+int psci_system_suspend_enter(suspend_state_t state)
 {
 	return cpu_suspend(0, psci_system_suspend);
 }
+EXPORT_SYMBOL_GPL(psci_system_suspend_enter);
 
 static const struct platform_suspend_ops psci_suspend_ops = {
 	.valid          = suspend_valid_only_mem,
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 4ca0060a3fc4..ffeba6b08e10 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -9,6 +9,7 @@
 
 #include <linux/arm-smccc.h>
 #include <linux/init.h>
+#include <linux/suspend.h>
 #include <linux/types.h>
 
 #define PSCI_POWER_STATE_TYPE_STANDBY		0
@@ -17,6 +18,7 @@
 bool psci_tos_resident_on(int cpu);
 
 int psci_cpu_suspend_enter(u32 state);
+int psci_system_suspend_enter(suspend_state_t state);
 bool psci_power_state_is_valid(u32 state);
 int psci_set_osi_mode(bool enable);
 bool psci_has_osi_support(void);
-- 
2.25.1

[PATCH 1/4] firmware: psci: Export a couple of suspend symbols

[Florian Fainelli]

Export psci_system_suspend_enter() and psci_cpu_suspend_enter() for
the Broadcom STB PSCI extension to be usable and built as a loadable
module.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/firmware/psci/psci.c | 9 ++++++++-
 include/linux/psci.h         | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index cfb448eabdaa..9a60e340c248 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -355,7 +355,13 @@ int psci_cpu_suspend_enter(u32 state)
 
 	return ret;
 }
+#else
+int psci_cpu_suspend_enter(u32 state)
+{
+	return -EOPNOTSUPP;
+}
 #endif
+EXPORT_SYMBOL_GPL(psci_cpu_suspend_enter);
 
 static int psci_system_suspend(unsigned long unused)
 {
@@ -365,10 +371,11 @@ static int psci_system_suspend(unsigned long unused)
 			      pa_cpu_resume, 0, 0);
 }
 
-static int psci_system_suspend_enter(suspend_state_t state)
+int psci_system_suspend_enter(suspend_state_t state)
 {
 	return cpu_suspend(0, psci_system_suspend);
 }
+EXPORT_SYMBOL_GPL(psci_system_suspend_enter);
 
 static const struct platform_suspend_ops psci_suspend_ops = {
 	.valid          = suspend_valid_only_mem,
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 4ca0060a3fc4..ffeba6b08e10 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -9,6 +9,7 @@
 
 #include <linux/arm-smccc.h>
 #include <linux/init.h>
+#include <linux/suspend.h>
 #include <linux/types.h>
 
 #define PSCI_POWER_STATE_TYPE_STANDBY		0
@@ -17,6 +18,7 @@
 bool psci_tos_resident_on(int cpu);
 
 int psci_cpu_suspend_enter(u32 state);
+int psci_system_suspend_enter(suspend_state_t state);
 bool psci_power_state_is_valid(u32 state);
 int psci_set_osi_mode(bool enable);
 bool psci_has_osi_support(void);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] firmware: psci: Export a couple of suspend symbols

[kernel test robot]

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on arm/for-next]
[also build test ERROR on soc/for-next arm64/for-next/core clk/clk-next linus/master v5.16 next-20220121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: arm64-randconfig-r011-20220120 (https://download.01.org/0day-ci/archive/20220122/202201221911.EEIlXDWO-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 7b3d30728816403d1fd73cc5082e9fb761262bce)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/488fbdb7de4d39409045b679db3e370600a774d4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
        git checkout 488fbdb7de4d39409045b679db3e370600a774d4
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: cpu_suspend
   >>> referenced by psci.c
   >>> firmware/psci/psci.o:(psci_system_suspend_enter) in archive drivers/built-in.a
--
>> ld.lld: error: undefined symbol: cpu_resume
   >>> referenced by psci.c
   >>> firmware/psci/psci.o:(psci_system_suspend) in archive drivers/built-in.a
   >>> referenced by psci.c
   >>> firmware/psci/psci.o:(psci_system_suspend) in archive drivers/built-in.a

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/4] firmware: psci: Export a couple of suspend symbols

[kernel test robot]

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on arm/for-next]
[also build test ERROR on soc/for-next arm64/for-next/core clk/clk-next linus/master v5.16 next-20220121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: arm64-randconfig-r011-20220120 (https://download.01.org/0day-ci/archive/20220122/202201221911.EEIlXDWO-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 7b3d30728816403d1fd73cc5082e9fb761262bce)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/488fbdb7de4d39409045b679db3e370600a774d4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
        git checkout 488fbdb7de4d39409045b679db3e370600a774d4
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: cpu_suspend
   >>> referenced by psci.c
   >>> firmware/psci/psci.o:(psci_system_suspend_enter) in archive drivers/built-in.a
--
>> ld.lld: error: undefined symbol: cpu_resume
   >>> referenced by psci.c
   >>> firmware/psci/psci.o:(psci_system_suspend) in archive drivers/built-in.a
   >>> referenced by psci.c
   >>> firmware/psci/psci.o:(psci_system_suspend) in archive drivers/built-in.a

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW

[Florian Fainelli]

On ARM 32-bit the legacy PM code would conflict with a PSCI based
implementation that we are going to introduce in subsequent commits.
Make PM_BRCMSTB depend on !ARM_PSCI_FW to exclude the two
implementations from each other.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/Makefile b/drivers/soc/bcm/brcmstb/pm/Makefile
index 8e10abb14f8b..86004458260c 100644
--- a/drivers/soc/bcm/brcmstb/pm/Makefile
+++ b/drivers/soc/bcm/brcmstb/pm/Makefile
@@ -1,4 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
+ifndef CONFIG_ARM_PSCI_FW
 obj-$(CONFIG_ARM)		+= s2-arm.o pm-arm.o
 AFLAGS_s2-arm.o			:= -march=armv7-a
+endif
 obj-$(CONFIG_BMIPS_GENERIC)	+= s2-mips.o s3-mips.o pm-mips.o
-- 
2.25.1

[PATCH 2/4] soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW

[Florian Fainelli]

On ARM 32-bit the legacy PM code would conflict with a PSCI based
implementation that we are going to introduce in subsequent commits.
Make PM_BRCMSTB depend on !ARM_PSCI_FW to exclude the two
implementations from each other.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/Makefile b/drivers/soc/bcm/brcmstb/pm/Makefile
index 8e10abb14f8b..86004458260c 100644
--- a/drivers/soc/bcm/brcmstb/pm/Makefile
+++ b/drivers/soc/bcm/brcmstb/pm/Makefile
@@ -1,4 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
+ifndef CONFIG_ARM_PSCI_FW
 obj-$(CONFIG_ARM)		+= s2-arm.o pm-arm.o
 AFLAGS_s2-arm.o			:= -march=armv7-a
+endif
 obj-$(CONFIG_BMIPS_GENERIC)	+= s2-mips.o s3-mips.o pm-mips.o
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[Florian Fainelli]

Add support for the Broadcom STB system suspend operations which
leverage the standard PSCI functions and uses the
psci_cpu_suspend_enter() operation to power off the system with or
without retention ("echo standby > /sys/power/state").

The system reset path also supports a special "powercycle" mode which
signals to the ARM Trusted Firmware that an external PMIC chip must
force the SoC into a power cycle.

As much as possible extensions were built using the SIP namespace rather
than the standard PSCI namespace, however compatibility with the
standard PSCI implementation is retained when CONFIG_BRCMSTB_PM is not
selected.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/Kconfig           |   4 +-
 drivers/soc/bcm/brcmstb/pm/Makefile       |   1 +
 drivers/soc/bcm/brcmstb/pm/pm-psci.c      | 315 ++++++++++++++++++++++
 include/linux/soc/brcmstb/brcmstb-smccc.h |  84 ++++++
 4 files changed, 402 insertions(+), 2 deletions(-)
 create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
 create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h

diff --git a/drivers/soc/bcm/brcmstb/Kconfig b/drivers/soc/bcm/brcmstb/Kconfig
index 38e476905d96..a2b31717096e 100644
--- a/drivers/soc/bcm/brcmstb/Kconfig
+++ b/drivers/soc/bcm/brcmstb/Kconfig
@@ -2,8 +2,8 @@
 if SOC_BRCMSTB
 
 config BRCMSTB_PM
-	bool "Support suspend/resume for STB platforms"
-	default y
+	tristate "Support suspend/resume for STB platforms"
+	default ARCH_BRCMSTB || BMIPS_GENERIC
 	depends on PM
 	depends on ARCH_BRCMSTB || BMIPS_GENERIC
 	select ARM_CPU_SUSPEND if ARM
diff --git a/drivers/soc/bcm/brcmstb/pm/Makefile b/drivers/soc/bcm/brcmstb/pm/Makefile
index 86004458260c..4ece53db8937 100644
--- a/drivers/soc/bcm/brcmstb/pm/Makefile
+++ b/drivers/soc/bcm/brcmstb/pm/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_BRCMSTB_PM)	+= pm-psci.o
 ifndef CONFIG_ARM_PSCI_FW
 obj-$(CONFIG_ARM)		+= s2-arm.o pm-arm.o
 AFLAGS_s2-arm.o			:= -march=armv7-a
diff --git a/drivers/soc/bcm/brcmstb/pm/pm-psci.c b/drivers/soc/bcm/brcmstb/pm/pm-psci.c
new file mode 100644
index 000000000000..7ba34d01c2fc
--- /dev/null
+++ b/drivers/soc/bcm/brcmstb/pm/pm-psci.c
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Broadcom STB PSCI based system wide PM support
+ *
+ * Copyright © 2018-2022 Broadcom
+ */
+
+#define pr_fmt(fmt) "brcmstb-pm-psci: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/arm-smccc.h>
+#include <linux/panic_notifier.h>
+#include <linux/psci.h>
+#include <linux/suspend.h>
+#include <linux/soc/brcmstb/brcmstb.h>
+#include <linux/soc/brcmstb/brcmstb-smccc.h>
+#include <linux/reboot.h>
+#include <linux/kobject.h>
+
+#include <uapi/linux/psci.h>
+
+#include <asm/suspend.h>
+#include <asm/system_misc.h>
+
+#include "aon_defs.h"
+
+static psci_fn *invoke_psci_fn;
+static bool brcmstb_psci_system_reset2_supported;
+static bool brcmstb_psci_system_suspend_supported;
+static bool brcmstb_psci_cpu_retention = true;
+
+static int brcmstb_psci_integ_region(unsigned long function_id,
+				     unsigned long base,
+				     unsigned long size)
+{
+	unsigned long end;
+
+	if (!size)
+		return -EINVAL;
+
+	end = DIV_ROUND_UP(base + size, SIP_MIN_REGION_SIZE);
+	base /= SIP_MIN_REGION_SIZE;
+	size = end - base;
+
+	return invoke_psci_fn(function_id, base, size, 0);
+}
+
+static int __maybe_unused brcmstb_psci_integ_region_set(unsigned long base,
+							unsigned long size)
+{
+	return brcmstb_psci_integ_region(SIP_FUNC_INTEG_REGION_SET, base, size);
+}
+
+static int __maybe_unused brcmstb_psci_integ_region_del(unsigned long base,
+							unsigned long size)
+{
+	return brcmstb_psci_integ_region(SIP_FUNC_INTEG_REGION_DEL, base, size);
+}
+
+static int brcmstb_psci_integ_region_reset_all(void)
+{
+	return invoke_psci_fn(SIP_FUNC_INTEG_REGION_RESET_ALL, 0, 0, 0);
+}
+
+static int brcmstb_psci_sys_reset(struct notifier_block *nb,
+				  unsigned long action, void *data)
+{
+	const char *cmd = data;
+	/*
+	 * reset_type[31] = 0 (architectural)
+	 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
+	 * cookie = 0 (ignored by the implementation)
+	 */
+	uint32_t reboot_type = 0;
+
+	if ((action == REBOOT_COLD || action == REBOOT_WARM ||
+	    action == REBOOT_SOFT) &&
+	    brcmstb_psci_system_reset2_supported) {
+		if (cmd && !strcmp(cmd, "powercycle"))
+			reboot_type = BIT(31) | 1;
+		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), reboot_type, 0, 0);
+	} else {
+		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block brcmstb_psci_sys_reset_nb = {
+	.notifier_call	= brcmstb_psci_sys_reset,
+	.priority	= 128,
+};
+
+void brcmstb_psci_sys_poweroff(void)
+{
+	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
+}
+
+static int psci_features(u32 psci_func_id)
+{
+	u32 features_func_id;
+
+	switch (ARM_SMCCC_OWNER_NUM(psci_func_id)) {
+	case ARM_SMCCC_OWNER_SIP:
+		features_func_id = SIP_FUNC_PSCI_FEATURES;
+		break;
+	case ARM_SMCCC_OWNER_STANDARD:
+		features_func_id = PSCI_1_0_FN_PSCI_FEATURES;
+		break;
+	default:
+		return PSCI_RET_NOT_SUPPORTED;
+	}
+
+	return invoke_psci_fn(features_func_id, psci_func_id, 0, 0);
+}
+
+static int brcmstb_psci_enter(suspend_state_t state)
+{
+	/* Request a SYSTEM level power state with retention */
+	u32 pstate = 2 << PSCI_0_2_POWER_STATE_AFFL_SHIFT |
+		     !brcmstb_psci_cpu_retention << PSCI_0_2_POWER_STATE_TYPE_SHIFT;
+	int ret = -EINVAL;
+
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+		ret = psci_cpu_suspend_enter(pstate);
+		break;
+	case PM_SUSPEND_MEM:
+		ret = brcmstb_psci_system_suspend_supported ?
+			 psci_system_suspend_enter(state) : -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int brcmstb_psci_valid(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+		return true;
+	case PM_SUSPEND_MEM:
+		return brcmstb_psci_system_suspend_supported;
+	default:
+		return false;
+	}
+}
+
+static const struct platform_suspend_ops brcmstb_psci_ops = {
+	.enter	= brcmstb_psci_enter,
+	.valid	= brcmstb_psci_valid,
+};
+
+static int brcmstb_psci_panic_notify(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	int ret;
+
+	ret = invoke_psci_fn(SIP_FUNC_PANIC_NOTIFY, BRCMSTB_PANIC_MAGIC, 0, 0);
+	if (ret != PSCI_RET_SUCCESS)
+		return NOTIFY_BAD;
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block brcmstb_psci_nb = {
+	.notifier_call = brcmstb_psci_panic_notify,
+};
+
+static ssize_t brcmstb_psci_version_show(struct kobject *kobj,
+					 struct kobj_attribute *attr,
+					 char *buf)
+{
+	struct arm_smccc_res res = { };
+	u32 version;
+
+	if (invoke_psci_fn == __invoke_psci_fn_hvc)
+		arm_smccc_hvc(SIP_FUNC_PSCI_BRCMSTB_VERSION,
+			      0, 0, 0, 0, 0, 0, 0, &res);
+	else
+		arm_smccc_smc(SIP_FUNC_PSCI_BRCMSTB_VERSION,
+			      0, 0, 0, 0, 0, 0, 0, &res);
+
+	if (res.a0 != PSCI_RET_SUCCESS)
+		return -EOPNOTSUPP;
+
+	version = res.a1;
+
+	return sprintf(buf, "%d.%d.%d.%d\n",
+		       (version >> 24) & 0xff, (version >> 16) & 0xff,
+		       (version >> 8) & 0xff, version & 0xff);
+}
+
+static struct kobj_attribute brcmstb_psci_version_attr =
+	__ATTR(mon_version, 0400, brcmstb_psci_version_show, NULL);
+
+static ssize_t brcmstb_psci_cpu_retention_show(struct kobject *kobj,
+					       struct kobj_attribute *attr,
+					       char *buf)
+{
+	return sprintf(buf, "%d\n", brcmstb_psci_cpu_retention);
+}
+
+static ssize_t brcmstb_psci_cpu_retention_store(struct kobject *kobj,
+						struct kobj_attribute *attr,
+						const char *buf, size_t count)
+{
+	int ret, val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
+	brcmstb_psci_cpu_retention = !!val;
+
+	return count;
+}
+
+static struct kobj_attribute brcmstb_psci_cpu_retention_attr =
+	__ATTR(cpu_retention, 0644, brcmstb_psci_cpu_retention_show,
+	       brcmstb_psci_cpu_retention_store);
+
+static const struct attribute *brcmstb_psci_attributes[] = {
+	&brcmstb_psci_version_attr.attr,
+	&brcmstb_psci_cpu_retention_attr.attr,
+	NULL,
+};
+
+static int brcmstb_pm_psci_init(void)
+{
+	unsigned long funcs_id[] = {
+		PSCI_0_2_FN_SYSTEM_OFF,
+		SIP_FUNC_INTEG_REGION_SET,
+		SIP_FUNC_INTEG_REGION_DEL,
+		SIP_FUNC_INTEG_REGION_RESET_ALL,
+	};
+	struct arm_smccc_res res = { };
+	struct kobject *brcmstb_kobj;
+	unsigned int i;
+	int ret;
+
+	switch (arm_smccc_1_1_get_conduit()) {
+	case SMCCC_CONDUIT_HVC:
+		invoke_psci_fn = __invoke_psci_fn_hvc;
+		break;
+	case SMCCC_CONDUIT_SMC:
+		invoke_psci_fn = __invoke_psci_fn_smc;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Check the revision of monitor */
+	if (invoke_psci_fn == __invoke_psci_fn_hvc)
+		arm_smccc_hvc(SIP_SVC_REVISION,
+			      0, 0, 0, 0, 0, 0, 0, &res);
+	else
+		arm_smccc_smc(SIP_SVC_REVISION,
+			      0, 0, 0, 0, 0, 0, 0, &res);
+
+	/* Test for our supported features */
+	for (i = 0; i < ARRAY_SIZE(funcs_id); i++) {
+		ret = psci_features(funcs_id[i]);
+		if (ret == PSCI_RET_NOT_SUPPORTED) {
+			pr_err("Firmware does not support function 0x%lx\n",
+			       funcs_id[i]);
+			return -EOPNOTSUPP;
+		}
+	}
+
+	ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
+	if (ret != PSCI_RET_NOT_SUPPORTED)
+		brcmstb_psci_system_reset2_supported = true;
+
+	ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND));
+	if (ret != PSCI_RET_NOT_SUPPORTED)
+		brcmstb_psci_system_suspend_supported = true;
+
+	ret = brcmstb_psci_integ_region_reset_all();
+	if (ret != PSCI_RET_SUCCESS) {
+		pr_err("Error resetting all integrity checking regions\n");
+		return -EIO;
+	}
+
+	if (res.a0 == SIP_REVISION_MAJOR && res.a1 < SIP_REVISION_MINOR) {
+		pr_info("Firmware is too old! Please update\n");
+		return -EOPNOTSUPP;
+	}
+
+	brcmstb_kobj = kobject_create_and_add("brcmstb", firmware_kobj);
+	if (brcmstb_kobj) {
+		ret = sysfs_create_files(brcmstb_kobj, brcmstb_psci_attributes);
+		if (ret)
+			return ret;
+	}
+
+	pm_power_off = brcmstb_psci_sys_poweroff;
+	register_restart_handler(&brcmstb_psci_sys_reset_nb);
+	suspend_set_ops(&brcmstb_psci_ops);
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &brcmstb_psci_nb);
+
+	pr_info("Using PSCI based system PM (full featured)\n");
+
+	return 0;
+}
+module_init(brcmstb_pm_psci_init);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Broadcom STB PM PSCI operations");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/brcmstb/brcmstb-smccc.h b/include/linux/soc/brcmstb/brcmstb-smccc.h
new file mode 100644
index 000000000000..2863e894e1c7
--- /dev/null
+++ b/include/linux/soc/brcmstb/brcmstb-smccc.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __BRCMSTB_SMCCC_H
+#define __BRCMSTB_SMCCC_H
+
+#include <linux/arm-smccc.h>
+#include <uapi/linux/psci.h>
+
+#ifdef CONFIG_64BIT
+#define PSCI_FN_NATIVE(version, name)   PSCI_##version##_FN64_##name
+#else
+#define PSCI_FN_NATIVE(version, name)   PSCI_##version##_FN_##name
+#endif
+
+/* Broadcom STB custom SIP function calls */
+#define SIP_FUNC_INTEG_REGION_SET	\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   0)
+#define SIP_FUNC_INTEG_REGION_DEL	\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   1)
+#define SIP_FUNC_INTEG_REGION_RESET_ALL	\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   2)
+#define SIP_FUNC_PANIC_NOTIFY		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   3)
+#define SIP_FUNC_PSCI_FEATURES		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   4)
+#define SIP_FUNC_PSCI_BRCMSTB_VERSION		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   5)
+
+#define SIP_SVC_REVISION		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   0xFF02)
+
+#define SIP_MIN_REGION_SIZE	4096
+#define SIP_REVISION_MAJOR	0
+#define SIP_REVISION_MINOR	2
+
+typedef unsigned long (psci_fn)(unsigned long, unsigned long,
+				unsigned long, unsigned long);
+
+static inline unsigned long __invoke_psci_fn_hvc(unsigned long function_id,
+						 unsigned long arg0,
+						 unsigned long arg1,
+						 unsigned long arg2)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_hvc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
+static inline unsigned long __invoke_psci_fn_smc(unsigned long function_id,
+						 unsigned long arg0,
+						 unsigned long arg1,
+						 unsigned long arg2)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
+
+#endif /* __BRCMSTB_SMCCC_H */
-- 
2.25.1

[PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[Florian Fainelli]

Add support for the Broadcom STB system suspend operations which
leverage the standard PSCI functions and uses the
psci_cpu_suspend_enter() operation to power off the system with or
without retention ("echo standby > /sys/power/state").

The system reset path also supports a special "powercycle" mode which
signals to the ARM Trusted Firmware that an external PMIC chip must
force the SoC into a power cycle.

As much as possible extensions were built using the SIP namespace rather
than the standard PSCI namespace, however compatibility with the
standard PSCI implementation is retained when CONFIG_BRCMSTB_PM is not
selected.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/Kconfig           |   4 +-
 drivers/soc/bcm/brcmstb/pm/Makefile       |   1 +
 drivers/soc/bcm/brcmstb/pm/pm-psci.c      | 315 ++++++++++++++++++++++
 include/linux/soc/brcmstb/brcmstb-smccc.h |  84 ++++++
 4 files changed, 402 insertions(+), 2 deletions(-)
 create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
 create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h

diff --git a/drivers/soc/bcm/brcmstb/Kconfig b/drivers/soc/bcm/brcmstb/Kconfig
index 38e476905d96..a2b31717096e 100644
--- a/drivers/soc/bcm/brcmstb/Kconfig
+++ b/drivers/soc/bcm/brcmstb/Kconfig
@@ -2,8 +2,8 @@
 if SOC_BRCMSTB
 
 config BRCMSTB_PM
-	bool "Support suspend/resume for STB platforms"
-	default y
+	tristate "Support suspend/resume for STB platforms"
+	default ARCH_BRCMSTB || BMIPS_GENERIC
 	depends on PM
 	depends on ARCH_BRCMSTB || BMIPS_GENERIC
 	select ARM_CPU_SUSPEND if ARM
diff --git a/drivers/soc/bcm/brcmstb/pm/Makefile b/drivers/soc/bcm/brcmstb/pm/Makefile
index 86004458260c..4ece53db8937 100644
--- a/drivers/soc/bcm/brcmstb/pm/Makefile
+++ b/drivers/soc/bcm/brcmstb/pm/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_BRCMSTB_PM)	+= pm-psci.o
 ifndef CONFIG_ARM_PSCI_FW
 obj-$(CONFIG_ARM)		+= s2-arm.o pm-arm.o
 AFLAGS_s2-arm.o			:= -march=armv7-a
diff --git a/drivers/soc/bcm/brcmstb/pm/pm-psci.c b/drivers/soc/bcm/brcmstb/pm/pm-psci.c
new file mode 100644
index 000000000000..7ba34d01c2fc
--- /dev/null
+++ b/drivers/soc/bcm/brcmstb/pm/pm-psci.c
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Broadcom STB PSCI based system wide PM support
+ *
+ * Copyright © 2018-2022 Broadcom
+ */
+
+#define pr_fmt(fmt) "brcmstb-pm-psci: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/arm-smccc.h>
+#include <linux/panic_notifier.h>
+#include <linux/psci.h>
+#include <linux/suspend.h>
+#include <linux/soc/brcmstb/brcmstb.h>
+#include <linux/soc/brcmstb/brcmstb-smccc.h>
+#include <linux/reboot.h>
+#include <linux/kobject.h>
+
+#include <uapi/linux/psci.h>
+
+#include <asm/suspend.h>
+#include <asm/system_misc.h>
+
+#include "aon_defs.h"
+
+static psci_fn *invoke_psci_fn;
+static bool brcmstb_psci_system_reset2_supported;
+static bool brcmstb_psci_system_suspend_supported;
+static bool brcmstb_psci_cpu_retention = true;
+
+static int brcmstb_psci_integ_region(unsigned long function_id,
+				     unsigned long base,
+				     unsigned long size)
+{
+	unsigned long end;
+
+	if (!size)
+		return -EINVAL;
+
+	end = DIV_ROUND_UP(base + size, SIP_MIN_REGION_SIZE);
+	base /= SIP_MIN_REGION_SIZE;
+	size = end - base;
+
+	return invoke_psci_fn(function_id, base, size, 0);
+}
+
+static int __maybe_unused brcmstb_psci_integ_region_set(unsigned long base,
+							unsigned long size)
+{
+	return brcmstb_psci_integ_region(SIP_FUNC_INTEG_REGION_SET, base, size);
+}
+
+static int __maybe_unused brcmstb_psci_integ_region_del(unsigned long base,
+							unsigned long size)
+{
+	return brcmstb_psci_integ_region(SIP_FUNC_INTEG_REGION_DEL, base, size);
+}
+
+static int brcmstb_psci_integ_region_reset_all(void)
+{
+	return invoke_psci_fn(SIP_FUNC_INTEG_REGION_RESET_ALL, 0, 0, 0);
+}
+
+static int brcmstb_psci_sys_reset(struct notifier_block *nb,
+				  unsigned long action, void *data)
+{
+	const char *cmd = data;
+	/*
+	 * reset_type[31] = 0 (architectural)
+	 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
+	 * cookie = 0 (ignored by the implementation)
+	 */
+	uint32_t reboot_type = 0;
+
+	if ((action == REBOOT_COLD || action == REBOOT_WARM ||
+	    action == REBOOT_SOFT) &&
+	    brcmstb_psci_system_reset2_supported) {
+		if (cmd && !strcmp(cmd, "powercycle"))
+			reboot_type = BIT(31) | 1;
+		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), reboot_type, 0, 0);
+	} else {
+		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block brcmstb_psci_sys_reset_nb = {
+	.notifier_call	= brcmstb_psci_sys_reset,
+	.priority	= 128,
+};
+
+void brcmstb_psci_sys_poweroff(void)
+{
+	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
+}
+
+static int psci_features(u32 psci_func_id)
+{
+	u32 features_func_id;
+
+	switch (ARM_SMCCC_OWNER_NUM(psci_func_id)) {
+	case ARM_SMCCC_OWNER_SIP:
+		features_func_id = SIP_FUNC_PSCI_FEATURES;
+		break;
+	case ARM_SMCCC_OWNER_STANDARD:
+		features_func_id = PSCI_1_0_FN_PSCI_FEATURES;
+		break;
+	default:
+		return PSCI_RET_NOT_SUPPORTED;
+	}
+
+	return invoke_psci_fn(features_func_id, psci_func_id, 0, 0);
+}
+
+static int brcmstb_psci_enter(suspend_state_t state)
+{
+	/* Request a SYSTEM level power state with retention */
+	u32 pstate = 2 << PSCI_0_2_POWER_STATE_AFFL_SHIFT |
+		     !brcmstb_psci_cpu_retention << PSCI_0_2_POWER_STATE_TYPE_SHIFT;
+	int ret = -EINVAL;
+
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+		ret = psci_cpu_suspend_enter(pstate);
+		break;
+	case PM_SUSPEND_MEM:
+		ret = brcmstb_psci_system_suspend_supported ?
+			 psci_system_suspend_enter(state) : -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int brcmstb_psci_valid(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+		return true;
+	case PM_SUSPEND_MEM:
+		return brcmstb_psci_system_suspend_supported;
+	default:
+		return false;
+	}
+}
+
+static const struct platform_suspend_ops brcmstb_psci_ops = {
+	.enter	= brcmstb_psci_enter,
+	.valid	= brcmstb_psci_valid,
+};
+
+static int brcmstb_psci_panic_notify(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	int ret;
+
+	ret = invoke_psci_fn(SIP_FUNC_PANIC_NOTIFY, BRCMSTB_PANIC_MAGIC, 0, 0);
+	if (ret != PSCI_RET_SUCCESS)
+		return NOTIFY_BAD;
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block brcmstb_psci_nb = {
+	.notifier_call = brcmstb_psci_panic_notify,
+};
+
+static ssize_t brcmstb_psci_version_show(struct kobject *kobj,
+					 struct kobj_attribute *attr,
+					 char *buf)
+{
+	struct arm_smccc_res res = { };
+	u32 version;
+
+	if (invoke_psci_fn == __invoke_psci_fn_hvc)
+		arm_smccc_hvc(SIP_FUNC_PSCI_BRCMSTB_VERSION,
+			      0, 0, 0, 0, 0, 0, 0, &res);
+	else
+		arm_smccc_smc(SIP_FUNC_PSCI_BRCMSTB_VERSION,
+			      0, 0, 0, 0, 0, 0, 0, &res);
+
+	if (res.a0 != PSCI_RET_SUCCESS)
+		return -EOPNOTSUPP;
+
+	version = res.a1;
+
+	return sprintf(buf, "%d.%d.%d.%d\n",
+		       (version >> 24) & 0xff, (version >> 16) & 0xff,
+		       (version >> 8) & 0xff, version & 0xff);
+}
+
+static struct kobj_attribute brcmstb_psci_version_attr =
+	__ATTR(mon_version, 0400, brcmstb_psci_version_show, NULL);
+
+static ssize_t brcmstb_psci_cpu_retention_show(struct kobject *kobj,
+					       struct kobj_attribute *attr,
+					       char *buf)
+{
+	return sprintf(buf, "%d\n", brcmstb_psci_cpu_retention);
+}
+
+static ssize_t brcmstb_psci_cpu_retention_store(struct kobject *kobj,
+						struct kobj_attribute *attr,
+						const char *buf, size_t count)
+{
+	int ret, val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
+	brcmstb_psci_cpu_retention = !!val;
+
+	return count;
+}
+
+static struct kobj_attribute brcmstb_psci_cpu_retention_attr =
+	__ATTR(cpu_retention, 0644, brcmstb_psci_cpu_retention_show,
+	       brcmstb_psci_cpu_retention_store);
+
+static const struct attribute *brcmstb_psci_attributes[] = {
+	&brcmstb_psci_version_attr.attr,
+	&brcmstb_psci_cpu_retention_attr.attr,
+	NULL,
+};
+
+static int brcmstb_pm_psci_init(void)
+{
+	unsigned long funcs_id[] = {
+		PSCI_0_2_FN_SYSTEM_OFF,
+		SIP_FUNC_INTEG_REGION_SET,
+		SIP_FUNC_INTEG_REGION_DEL,
+		SIP_FUNC_INTEG_REGION_RESET_ALL,
+	};
+	struct arm_smccc_res res = { };
+	struct kobject *brcmstb_kobj;
+	unsigned int i;
+	int ret;
+
+	switch (arm_smccc_1_1_get_conduit()) {
+	case SMCCC_CONDUIT_HVC:
+		invoke_psci_fn = __invoke_psci_fn_hvc;
+		break;
+	case SMCCC_CONDUIT_SMC:
+		invoke_psci_fn = __invoke_psci_fn_smc;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Check the revision of monitor */
+	if (invoke_psci_fn == __invoke_psci_fn_hvc)
+		arm_smccc_hvc(SIP_SVC_REVISION,
+			      0, 0, 0, 0, 0, 0, 0, &res);
+	else
+		arm_smccc_smc(SIP_SVC_REVISION,
+			      0, 0, 0, 0, 0, 0, 0, &res);
+
+	/* Test for our supported features */
+	for (i = 0; i < ARRAY_SIZE(funcs_id); i++) {
+		ret = psci_features(funcs_id[i]);
+		if (ret == PSCI_RET_NOT_SUPPORTED) {
+			pr_err("Firmware does not support function 0x%lx\n",
+			       funcs_id[i]);
+			return -EOPNOTSUPP;
+		}
+	}
+
+	ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
+	if (ret != PSCI_RET_NOT_SUPPORTED)
+		brcmstb_psci_system_reset2_supported = true;
+
+	ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND));
+	if (ret != PSCI_RET_NOT_SUPPORTED)
+		brcmstb_psci_system_suspend_supported = true;
+
+	ret = brcmstb_psci_integ_region_reset_all();
+	if (ret != PSCI_RET_SUCCESS) {
+		pr_err("Error resetting all integrity checking regions\n");
+		return -EIO;
+	}
+
+	if (res.a0 == SIP_REVISION_MAJOR && res.a1 < SIP_REVISION_MINOR) {
+		pr_info("Firmware is too old! Please update\n");
+		return -EOPNOTSUPP;
+	}
+
+	brcmstb_kobj = kobject_create_and_add("brcmstb", firmware_kobj);
+	if (brcmstb_kobj) {
+		ret = sysfs_create_files(brcmstb_kobj, brcmstb_psci_attributes);
+		if (ret)
+			return ret;
+	}
+
+	pm_power_off = brcmstb_psci_sys_poweroff;
+	register_restart_handler(&brcmstb_psci_sys_reset_nb);
+	suspend_set_ops(&brcmstb_psci_ops);
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &brcmstb_psci_nb);
+
+	pr_info("Using PSCI based system PM (full featured)\n");
+
+	return 0;
+}
+module_init(brcmstb_pm_psci_init);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Broadcom STB PM PSCI operations");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/brcmstb/brcmstb-smccc.h b/include/linux/soc/brcmstb/brcmstb-smccc.h
new file mode 100644
index 000000000000..2863e894e1c7
--- /dev/null
+++ b/include/linux/soc/brcmstb/brcmstb-smccc.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __BRCMSTB_SMCCC_H
+#define __BRCMSTB_SMCCC_H
+
+#include <linux/arm-smccc.h>
+#include <uapi/linux/psci.h>
+
+#ifdef CONFIG_64BIT
+#define PSCI_FN_NATIVE(version, name)   PSCI_##version##_FN64_##name
+#else
+#define PSCI_FN_NATIVE(version, name)   PSCI_##version##_FN_##name
+#endif
+
+/* Broadcom STB custom SIP function calls */
+#define SIP_FUNC_INTEG_REGION_SET	\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   0)
+#define SIP_FUNC_INTEG_REGION_DEL	\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   1)
+#define SIP_FUNC_INTEG_REGION_RESET_ALL	\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   2)
+#define SIP_FUNC_PANIC_NOTIFY		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   3)
+#define SIP_FUNC_PSCI_FEATURES		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   4)
+#define SIP_FUNC_PSCI_BRCMSTB_VERSION		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   5)
+
+#define SIP_SVC_REVISION		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			   IS_ENABLED(CONFIG_64BIT), \
+			   ARM_SMCCC_OWNER_SIP, \
+			   0xFF02)
+
+#define SIP_MIN_REGION_SIZE	4096
+#define SIP_REVISION_MAJOR	0
+#define SIP_REVISION_MINOR	2
+
+typedef unsigned long (psci_fn)(unsigned long, unsigned long,
+				unsigned long, unsigned long);
+
+static inline unsigned long __invoke_psci_fn_hvc(unsigned long function_id,
+						 unsigned long arg0,
+						 unsigned long arg1,
+						 unsigned long arg2)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_hvc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
+static inline unsigned long __invoke_psci_fn_smc(unsigned long function_id,
+						 unsigned long arg0,
+						 unsigned long arg1,
+						 unsigned long arg2)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
+
+#endif /* __BRCMSTB_SMCCC_H */
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[kernel test robot]

Hi Florian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on arm/for-next]
[also build test WARNING on soc/for-next arm64/for-next/core clk/clk-next linus/master v5.16 next-20220121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: arm64-randconfig-r011-20220120 (https://download.01.org/0day-ci/archive/20220122/202201221442.7bCTuqul-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 7b3d30728816403d1fd73cc5082e9fb761262bce)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/30bf64821ad5d3cbbc91770b3927905d10d3bdf3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
        git checkout 30bf64821ad5d3cbbc91770b3927905d10d3bdf3
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/soc/bcm/brcmstb/pm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/soc/bcm/brcmstb/pm/pm-psci.c:95:6: warning: no previous prototype for function 'brcmstb_psci_sys_poweroff' [-Wmissing-prototypes]
   void brcmstb_psci_sys_poweroff(void)
        ^
   drivers/soc/bcm/brcmstb/pm/pm-psci.c:95:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void brcmstb_psci_sys_poweroff(void)
   ^
   static 
   1 warning generated.


vim +/brcmstb_psci_sys_poweroff +95 drivers/soc/bcm/brcmstb/pm/pm-psci.c

    94	
  > 95	void brcmstb_psci_sys_poweroff(void)
    96	{
    97		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
    98	}
    99	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[kernel test robot]

Hi Florian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on arm/for-next]
[also build test WARNING on soc/for-next arm64/for-next/core clk/clk-next linus/master v5.16 next-20220121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: arm64-randconfig-r011-20220120 (https://download.01.org/0day-ci/archive/20220122/202201221442.7bCTuqul-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 7b3d30728816403d1fd73cc5082e9fb761262bce)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/30bf64821ad5d3cbbc91770b3927905d10d3bdf3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
        git checkout 30bf64821ad5d3cbbc91770b3927905d10d3bdf3
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/soc/bcm/brcmstb/pm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/soc/bcm/brcmstb/pm/pm-psci.c:95:6: warning: no previous prototype for function 'brcmstb_psci_sys_poweroff' [-Wmissing-prototypes]
   void brcmstb_psci_sys_poweroff(void)
        ^
   drivers/soc/bcm/brcmstb/pm/pm-psci.c:95:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void brcmstb_psci_sys_poweroff(void)
   ^
   static 
   1 warning generated.


vim +/brcmstb_psci_sys_poweroff +95 drivers/soc/bcm/brcmstb/pm/pm-psci.c

    94	
  > 95	void brcmstb_psci_sys_poweroff(void)
    96	{
    97		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
    98	}
    99	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[kernel test robot]

Hi Florian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on arm/for-next]
[also build test WARNING on soc/for-next arm64/for-next/core clk/clk-next linus/master v5.16 next-20220121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: arm-multi_v7_defconfig (https://download.01.org/0day-ci/archive/20220122/202201221439.RaZlgS8d-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/30bf64821ad5d3cbbc91770b3927905d10d3bdf3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
        git checkout 30bf64821ad5d3cbbc91770b3927905d10d3bdf3
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash drivers/soc/bcm/brcmstb/pm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/soc/bcm/brcmstb/pm/pm-psci.c:95:6: warning: no previous prototype for 'brcmstb_psci_sys_poweroff' [-Wmissing-prototypes]
      95 | void brcmstb_psci_sys_poweroff(void)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/brcmstb_psci_sys_poweroff +95 drivers/soc/bcm/brcmstb/pm/pm-psci.c

    94	
  > 95	void brcmstb_psci_sys_poweroff(void)
    96	{
    97		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
    98	}
    99	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[kernel test robot]

Hi Florian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on arm/for-next]
[also build test WARNING on soc/for-next arm64/for-next/core clk/clk-next linus/master v5.16 next-20220121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: arm-multi_v7_defconfig (https://download.01.org/0day-ci/archive/20220122/202201221439.RaZlgS8d-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/30bf64821ad5d3cbbc91770b3927905d10d3bdf3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/Broadcom-STB-PM-PSCI-extensions/20220122-115551
        git checkout 30bf64821ad5d3cbbc91770b3927905d10d3bdf3
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash drivers/soc/bcm/brcmstb/pm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/soc/bcm/brcmstb/pm/pm-psci.c:95:6: warning: no previous prototype for 'brcmstb_psci_sys_poweroff' [-Wmissing-prototypes]
      95 | void brcmstb_psci_sys_poweroff(void)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/brcmstb_psci_sys_poweroff +95 drivers/soc/bcm/brcmstb/pm/pm-psci.c

    94	
  > 95	void brcmstb_psci_sys_poweroff(void)
    96	{
    97		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
    98	}
    99	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[Mark Rutland]

Hi,

On Fri, Jan 21, 2022 at 07:54:20PM -0800, Florian Fainelli wrote:
> Add support for the Broadcom STB system suspend operations which
> leverage the standard PSCI functions and uses the
> psci_cpu_suspend_enter() operation to power off the system with or
> without retention ("echo standby > /sys/power/state").

What exactly does this enable that can't be achieved with the existing PSCI
driver as-is?

When you say "retention", what specifically do you mean? Retention of CPU
state? DRAM contents?

We already have SYSTEM_SUSPEND for states where DRAM content is retained but
CPU (and some system state) is lost, and IIUC we can do suspend-to-idle with
CPU_SUSPEND states.

interface necessary?

> The system reset path also supports a special "powercycle" mode which
> signals to the ARM Trusted Firmware that an external PMIC chip must
> force the SoC into a power cycle.

How does that compare to the regular SYSTEM_RESET call?

The PSCI spec says of SYSTEM_RESET:

| This function provides a method for performing a system cold reset. To the
| caller, the behavior is equivalent to a hardware power-cycle sequence.

... so I don't follow how this is different, unless this platform's
SYSTEM_RESET implementation is *not* actually performing a system cold reset?

If that *doesn't* perform a cold rest, it seems like a bug?

> As much as possible extensions were built using the SIP namespace rather
> than the standard PSCI namespace, however compatibility with the
> standard PSCI implementation is retained when CONFIG_BRCMSTB_PM is not
> selected.

I really don't like this, because it seems to be creating a parallel
infrastructure for doing things that can *already* be done with standard PSCI
driver. The actual PSCI bits just seem to be playing about with the power_state
value, which we should be able to do in the regular PSCI driver, and the
SIP-specific functions seem to have nothing to do with PSCI.

At the least there needs to be a much better explanation of why this is
necessary, but overall I'd very much like to have *any* vendor specific code
for suspend states, and if there are limitations in the standard PSCI driver we
go and address those. Otherwise we're going to gain a plethora of
vendor-specific suspend implementations, which is exactly what PSCI was
intended to avoid in the first place.

I have some specific comments below, but even with those addressed, I don't
think this is the right approach, and as things stand, NAK to this.

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/soc/bcm/brcmstb/Kconfig           |   4 +-
>  drivers/soc/bcm/brcmstb/pm/Makefile       |   1 +
>  drivers/soc/bcm/brcmstb/pm/pm-psci.c      | 315 ++++++++++++++++++++++
>  include/linux/soc/brcmstb/brcmstb-smccc.h |  84 ++++++
>  4 files changed, 402 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
>  create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h
> 
> diff --git a/drivers/soc/bcm/brcmstb/Kconfig b/drivers/soc/bcm/brcmstb/Kconfig
> index 38e476905d96..a2b31717096e 100644
> --- a/drivers/soc/bcm/brcmstb/Kconfig
> +++ b/drivers/soc/bcm/brcmstb/Kconfig
> @@ -2,8 +2,8 @@
>  if SOC_BRCMSTB
>  
>  config BRCMSTB_PM
> -	bool "Support suspend/resume for STB platforms"
> -	default y
> +	tristate "Support suspend/resume for STB platforms"
> +	default ARCH_BRCMSTB || BMIPS_GENERIC
>  	depends on PM
>  	depends on ARCH_BRCMSTB || BMIPS_GENERIC
>  	select ARM_CPU_SUSPEND if ARM
> diff --git a/drivers/soc/bcm/brcmstb/pm/Makefile b/drivers/soc/bcm/brcmstb/pm/Makefile
> index 86004458260c..4ece53db8937 100644
> --- a/drivers/soc/bcm/brcmstb/pm/Makefile
> +++ b/drivers/soc/bcm/brcmstb/pm/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_BRCMSTB_PM)	+= pm-psci.o
>  ifndef CONFIG_ARM_PSCI_FW
>  obj-$(CONFIG_ARM)		+= s2-arm.o pm-arm.o
>  AFLAGS_s2-arm.o			:= -march=armv7-a
> diff --git a/drivers/soc/bcm/brcmstb/pm/pm-psci.c b/drivers/soc/bcm/brcmstb/pm/pm-psci.c
> new file mode 100644
> index 000000000000..7ba34d01c2fc
> --- /dev/null
> +++ b/drivers/soc/bcm/brcmstb/pm/pm-psci.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Broadcom STB PSCI based system wide PM support
> + *
> + * Copyright © 2018-2022 Broadcom
> + */
> +
> +#define pr_fmt(fmt) "brcmstb-pm-psci: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/panic_notifier.h>
> +#include <linux/psci.h>
> +#include <linux/suspend.h>
> +#include <linux/soc/brcmstb/brcmstb.h>
> +#include <linux/soc/brcmstb/brcmstb-smccc.h>
> +#include <linux/reboot.h>
> +#include <linux/kobject.h>
> +
> +#include <uapi/linux/psci.h>
> +
> +#include <asm/suspend.h>
> +#include <asm/system_misc.h>
> +
> +#include "aon_defs.h"
> +
> +static psci_fn *invoke_psci_fn;
> +static bool brcmstb_psci_system_reset2_supported;
> +static bool brcmstb_psci_system_suspend_supported;
> +static bool brcmstb_psci_cpu_retention = true;
> +
> +static int brcmstb_psci_integ_region(unsigned long function_id,
> +				     unsigned long base,
> +				     unsigned long size)
> +{
> +	unsigned long end;
> +
> +	if (!size)
> +		return -EINVAL;
> +
> +	end = DIV_ROUND_UP(base + size, SIP_MIN_REGION_SIZE);
> +	base /= SIP_MIN_REGION_SIZE;
> +	size = end - base;
> +
> +	return invoke_psci_fn(function_id, base, size, 0);
> +}
> +
> +static int __maybe_unused brcmstb_psci_integ_region_set(unsigned long base,
> +							unsigned long size)
> +{
> +	return brcmstb_psci_integ_region(SIP_FUNC_INTEG_REGION_SET, base, size);
> +}
> +
> +static int __maybe_unused brcmstb_psci_integ_region_del(unsigned long base,
> +							unsigned long size)
> +{
> +	return brcmstb_psci_integ_region(SIP_FUNC_INTEG_REGION_DEL, base, size);
> +}
> +
> +static int brcmstb_psci_integ_region_reset_all(void)
> +{
> +	return invoke_psci_fn(SIP_FUNC_INTEG_REGION_RESET_ALL, 0, 0, 0);
> +}

What's all this? Below I see the phrase "integrity checking regions", but only
the brcmstb_psci_integ_region_reset_all() function is used, and it's not clear
what this is supposed to be for.

> +static int brcmstb_psci_sys_reset(struct notifier_block *nb,
> +				  unsigned long action, void *data)
> +{
> +	const char *cmd = data;
> +	/*
> +	 * reset_type[31] = 0 (architectural)
> +	 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
> +	 * cookie = 0 (ignored by the implementation)
> +	 */
> +	uint32_t reboot_type = 0;
> +
> +	if ((action == REBOOT_COLD || action == REBOOT_WARM ||
> +	    action == REBOOT_SOFT) &&
> +	    brcmstb_psci_system_reset2_supported) {
> +		if (cmd && !strcmp(cmd, "powercycle"))
> +			reboot_type = BIT(31) | 1;
> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), reboot_type, 0, 0);
> +	} else {
> +		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +	}
> +
> +	return NOTIFY_DONE;
> +}

If there are a bunch of specific SYSTEM_RESET2 values we want to expose, I'd
rather we described those generically in the DT, and somehow handle that in the
generic driver.

> +
> +static struct notifier_block brcmstb_psci_sys_reset_nb = {
> +	.notifier_call	= brcmstb_psci_sys_reset,
> +	.priority	= 128,
> +};
> +
> +void brcmstb_psci_sys_poweroff(void)
> +{
> +	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +}
> +
> +static int psci_features(u32 psci_func_id)
> +{
> +	u32 features_func_id;
> +
> +	switch (ARM_SMCCC_OWNER_NUM(psci_func_id)) {
> +	case ARM_SMCCC_OWNER_SIP:
> +		features_func_id = SIP_FUNC_PSCI_FEATURES;
> +		break;
> +	case ARM_SMCCC_OWNER_STANDARD:
> +		features_func_id = PSCI_1_0_FN_PSCI_FEATURES;
> +		break;
> +	default:
> +		return PSCI_RET_NOT_SUPPORTED;
> +	}
> +
> +	return invoke_psci_fn(features_func_id, psci_func_id, 0, 0);
> +}
> +
> +static int brcmstb_psci_enter(suspend_state_t state)
> +{
> +	/* Request a SYSTEM level power state with retention */
> +	u32 pstate = 2 << PSCI_0_2_POWER_STATE_AFFL_SHIFT |
> +		     !brcmstb_psci_cpu_retention << PSCI_0_2_POWER_STATE_TYPE_SHIFT;
> +	int ret = -EINVAL;
> +
> +	switch (state) {
> +	case PM_SUSPEND_STANDBY:
> +		ret = psci_cpu_suspend_enter(pstate);
> +		break;
> +	case PM_SUSPEND_MEM:
> +		ret = brcmstb_psci_system_suspend_supported ?
> +			 psci_system_suspend_enter(state) : -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int brcmstb_psci_valid(suspend_state_t state)
> +{
> +	switch (state) {
> +	case PM_SUSPEND_STANDBY:
> +		return true;
> +	case PM_SUSPEND_MEM:
> +		return brcmstb_psci_system_suspend_supported;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct platform_suspend_ops brcmstb_psci_ops = {
> +	.enter	= brcmstb_psci_enter,
> +	.valid	= brcmstb_psci_valid,
> +};
> +
> +static int brcmstb_psci_panic_notify(struct notifier_block *nb,
> +				     unsigned long action, void *data)
> +{
> +	int ret;
> +
> +	ret = invoke_psci_fn(SIP_FUNC_PANIC_NOTIFY, BRCMSTB_PANIC_MAGIC, 0, 0);
> +	if (ret != PSCI_RET_SUCCESS)
> +		return NOTIFY_BAD;
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block brcmstb_psci_nb = {
> +	.notifier_call = brcmstb_psci_panic_notify,
> +};

This appears to have nothing to do with idle/suspend states (and so might be OK
on its own if you need it, but it should be in a separate patch with some
justification).

> +static ssize_t brcmstb_psci_version_show(struct kobject *kobj,
> +					 struct kobj_attribute *attr,
> +					 char *buf)
> +{
> +	struct arm_smccc_res res = { };
> +	u32 version;
> +
> +	if (invoke_psci_fn == __invoke_psci_fn_hvc)
> +		arm_smccc_hvc(SIP_FUNC_PSCI_BRCMSTB_VERSION,
> +			      0, 0, 0, 0, 0, 0, 0, &res);
> +	else
> +		arm_smccc_smc(SIP_FUNC_PSCI_BRCMSTB_VERSION,
> +			      0, 0, 0, 0, 0, 0, 0, &res);
> +
> +	if (res.a0 != PSCI_RET_SUCCESS)
> +		return -EOPNOTSUPP;
> +
> +	version = res.a1;
> +
> +	return sprintf(buf, "%d.%d.%d.%d\n",
> +		       (version >> 24) & 0xff, (version >> 16) & 0xff,
> +		       (version >> 8) & 0xff, version & 0xff);
> +}
> +
> +static struct kobj_attribute brcmstb_psci_version_attr =
> +	__ATTR(mon_version, 0400, brcmstb_psci_version_show, NULL);
> +
> +static ssize_t brcmstb_psci_cpu_retention_show(struct kobject *kobj,
> +					       struct kobj_attribute *attr,
> +					       char *buf)
> +{
> +	return sprintf(buf, "%d\n", brcmstb_psci_cpu_retention);
> +}
> +
> +static ssize_t brcmstb_psci_cpu_retention_store(struct kobject *kobj,
> +						struct kobj_attribute *attr,
> +						const char *buf, size_t count)
> +{
> +	int ret, val;
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val != 0 && val != 1)
> +		return -EINVAL;
> +
> +	brcmstb_psci_cpu_retention = !!val;
> +
> +	return count;
> +}
> +
> +static struct kobj_attribute brcmstb_psci_cpu_retention_attr =
> +	__ATTR(cpu_retention, 0644, brcmstb_psci_cpu_retention_show,
> +	       brcmstb_psci_cpu_retention_store);
> +
> +static const struct attribute *brcmstb_psci_attributes[] = {
> +	&brcmstb_psci_version_attr.attr,
> +	&brcmstb_psci_cpu_retention_attr.attr,
> +	NULL,
> +};
> +
> +static int brcmstb_pm_psci_init(void)
> +{
> +	unsigned long funcs_id[] = {
> +		PSCI_0_2_FN_SYSTEM_OFF,
> +		SIP_FUNC_INTEG_REGION_SET,
> +		SIP_FUNC_INTEG_REGION_DEL,
> +		SIP_FUNC_INTEG_REGION_RESET_ALL,
> +	};
> +	struct arm_smccc_res res = { };
> +	struct kobject *brcmstb_kobj;
> +	unsigned int i;
> +	int ret;
> +
> +	switch (arm_smccc_1_1_get_conduit()) {
> +	case SMCCC_CONDUIT_HVC:
> +		invoke_psci_fn = __invoke_psci_fn_hvc;
> +		break;
> +	case SMCCC_CONDUIT_SMC:
> +		invoke_psci_fn = __invoke_psci_fn_smc;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Immediately after this switch, we know we there is *a* SMCCCv1.1+
implementation, but we have no idea *which* implementation that is. It could be
Broadcom's brcmstb implementation, it could be KVM's implementation, or anyone
else's...

> +
> +	/* Check the revision of monitor */
> +	if (invoke_psci_fn == __invoke_psci_fn_hvc)
> +		arm_smccc_hvc(SIP_SVC_REVISION,
> +			      0, 0, 0, 0, 0, 0, 0, &res);
> +	else
> +		arm_smccc_smc(SIP_SVC_REVISION,
> +			      0, 0, 0, 0, 0, 0, 0, &res);

This tells us the SIP interface's revision (if implemented), but not that the
SIP is Broadcom, and we still don't know that this is the brcmstb
implementation specifically.

> +	/* Test for our supported features */
> +	for (i = 0; i < ARRAY_SIZE(funcs_id); i++) {
> +		ret = psci_features(funcs_id[i]);
> +		if (ret == PSCI_RET_NOT_SUPPORTED) {
> +			pr_err("Firmware does not support function 0x%lx\n",
> +			       funcs_id[i]);
> +			return -EOPNOTSUPP;
> +		}
> +	}

For the SIP FIDs, this will invoke SIP_FUNC_PSCI_FEATURES, and since we haven't
determined the specific FW implementation, that could be another SIP's function
that does arbitrary things (e.g. they might just return 0, and mislead you).

So this is unsafe.

> +
> +	ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
> +	if (ret != PSCI_RET_NOT_SUPPORTED)
> +		brcmstb_psci_system_reset2_supported = true;
> +
> +	ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND));
> +	if (ret != PSCI_RET_NOT_SUPPORTED)
> +		brcmstb_psci_system_suspend_supported = true;

Even if we've managed to get here, we still don't know that this is a brcmstb
implementation...

> +
> +	ret = brcmstb_psci_integ_region_reset_all();

... and as with all SIP calls, this might do arbitrary things.

> +	if (ret != PSCI_RET_SUCCESS) {
> +		pr_err("Error resetting all integrity checking regions\n");
> +		return -EIO;
> +	}
> +
> +	if (res.a0 == SIP_REVISION_MAJOR && res.a1 < SIP_REVISION_MINOR) {
> +		pr_info("Firmware is too old! Please update\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	brcmstb_kobj = kobject_create_and_add("brcmstb", firmware_kobj);
> +	if (brcmstb_kobj) {
> +		ret = sysfs_create_files(brcmstb_kobj, brcmstb_psci_attributes);
> +		if (ret)
> +			return ret;
> +	}

... and we can register this on a non-brcmstb implementation...

> +
> +	pm_power_off = brcmstb_psci_sys_poweroff;
> +	register_restart_handler(&brcmstb_psci_sys_reset_nb);
> +	suspend_set_ops(&brcmstb_psci_ops);
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &brcmstb_psci_nb);

... and we break power off, restart, and suspend on other implementations ...

> +	pr_info("Using PSCI based system PM (full featured)\n");

This should be explicit with something like "Overriding stnadard PSCI
functionaliy with brcmstb-specific code".

As it stands this is at best meaningless, and at worst misleading and
disparaging of standard PSCI.

> +
> +	return 0;
> +}
> +module_init(brcmstb_pm_psci_init);
> +
> +MODULE_AUTHOR("Broadcom");
> +MODULE_DESCRIPTION("Broadcom STB PM PSCI operations");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/soc/brcmstb/brcmstb-smccc.h b/include/linux/soc/brcmstb/brcmstb-smccc.h
> new file mode 100644
> index 000000000000..2863e894e1c7
> --- /dev/null
> +++ b/include/linux/soc/brcmstb/brcmstb-smccc.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __BRCMSTB_SMCCC_H
> +#define __BRCMSTB_SMCCC_H
> +
> +#include <linux/arm-smccc.h>
> +#include <uapi/linux/psci.h>
> +
> +#ifdef CONFIG_64BIT
> +#define PSCI_FN_NATIVE(version, name)   PSCI_##version##_FN64_##name
> +#else
> +#define PSCI_FN_NATIVE(version, name)   PSCI_##version##_FN_##name
> +#endif
> +
> +/* Broadcom STB custom SIP function calls */
> +#define SIP_FUNC_INTEG_REGION_SET	\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   0)
> +#define SIP_FUNC_INTEG_REGION_DEL	\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   1)
> +#define SIP_FUNC_INTEG_REGION_RESET_ALL	\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   2)
> +#define SIP_FUNC_PANIC_NOTIFY		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   3)
> +#define SIP_FUNC_PSCI_FEATURES		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   4)
> +#define SIP_FUNC_PSCI_BRCMSTB_VERSION		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   5)
> +
> +#define SIP_SVC_REVISION		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   0xFF02)

Looking at the SMCCC spec, isn't the "general service query" REVISION call
0xFF03? 0xFF02 is reserved.

Thanks,
Mark.

> +
> +#define SIP_MIN_REGION_SIZE	4096
> +#define SIP_REVISION_MAJOR	0
> +#define SIP_REVISION_MINOR	2
> +
> +typedef unsigned long (psci_fn)(unsigned long, unsigned long,
> +				unsigned long, unsigned long);
> +
> +static inline unsigned long __invoke_psci_fn_hvc(unsigned long function_id,
> +						 unsigned long arg0,
> +						 unsigned long arg1,
> +						 unsigned long arg2)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_hvc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
> +static inline unsigned long __invoke_psci_fn_smc(unsigned long function_id,
> +						 unsigned long arg0,
> +						 unsigned long arg1,
> +						 unsigned long arg2)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
> +
> +#endif /* __BRCMSTB_SMCCC_H */
> -- 
> 2.25.1
> 

Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[Mark Rutland]

Hi,

On Fri, Jan 21, 2022 at 07:54:20PM -0800, Florian Fainelli wrote:
> Add support for the Broadcom STB system suspend operations which
> leverage the standard PSCI functions and uses the
> psci_cpu_suspend_enter() operation to power off the system with or
> without retention ("echo standby > /sys/power/state").

What exactly does this enable that can't be achieved with the existing PSCI
driver as-is?

When you say "retention", what specifically do you mean? Retention of CPU
state? DRAM contents?

We already have SYSTEM_SUSPEND for states where DRAM content is retained but
CPU (and some system state) is lost, and IIUC we can do suspend-to-idle with
CPU_SUSPEND states.

interface necessary?

> The system reset path also supports a special "powercycle" mode which
> signals to the ARM Trusted Firmware that an external PMIC chip must
> force the SoC into a power cycle.

How does that compare to the regular SYSTEM_RESET call?

The PSCI spec says of SYSTEM_RESET:

| This function provides a method for performing a system cold reset. To the
| caller, the behavior is equivalent to a hardware power-cycle sequence.

... so I don't follow how this is different, unless this platform's
SYSTEM_RESET implementation is *not* actually performing a system cold reset?

If that *doesn't* perform a cold rest, it seems like a bug?

> As much as possible extensions were built using the SIP namespace rather
> than the standard PSCI namespace, however compatibility with the
> standard PSCI implementation is retained when CONFIG_BRCMSTB_PM is not
> selected.

I really don't like this, because it seems to be creating a parallel
infrastructure for doing things that can *already* be done with standard PSCI
driver. The actual PSCI bits just seem to be playing about with the power_state
value, which we should be able to do in the regular PSCI driver, and the
SIP-specific functions seem to have nothing to do with PSCI.

At the least there needs to be a much better explanation of why this is
necessary, but overall I'd very much like to have *any* vendor specific code
for suspend states, and if there are limitations in the standard PSCI driver we
go and address those. Otherwise we're going to gain a plethora of
vendor-specific suspend implementations, which is exactly what PSCI was
intended to avoid in the first place.

I have some specific comments below, but even with those addressed, I don't
think this is the right approach, and as things stand, NAK to this.

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/soc/bcm/brcmstb/Kconfig           |   4 +-
>  drivers/soc/bcm/brcmstb/pm/Makefile       |   1 +
>  drivers/soc/bcm/brcmstb/pm/pm-psci.c      | 315 ++++++++++++++++++++++
>  include/linux/soc/brcmstb/brcmstb-smccc.h |  84 ++++++
>  4 files changed, 402 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
>  create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h
> 
> diff --git a/drivers/soc/bcm/brcmstb/Kconfig b/drivers/soc/bcm/brcmstb/Kconfig
> index 38e476905d96..a2b31717096e 100644
> --- a/drivers/soc/bcm/brcmstb/Kconfig
> +++ b/drivers/soc/bcm/brcmstb/Kconfig
> @@ -2,8 +2,8 @@
>  if SOC_BRCMSTB
>  
>  config BRCMSTB_PM
> -	bool "Support suspend/resume for STB platforms"
> -	default y
> +	tristate "Support suspend/resume for STB platforms"
> +	default ARCH_BRCMSTB || BMIPS_GENERIC
>  	depends on PM
>  	depends on ARCH_BRCMSTB || BMIPS_GENERIC
>  	select ARM_CPU_SUSPEND if ARM
> diff --git a/drivers/soc/bcm/brcmstb/pm/Makefile b/drivers/soc/bcm/brcmstb/pm/Makefile
> index 86004458260c..4ece53db8937 100644
> --- a/drivers/soc/bcm/brcmstb/pm/Makefile
> +++ b/drivers/soc/bcm/brcmstb/pm/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_BRCMSTB_PM)	+= pm-psci.o
>  ifndef CONFIG_ARM_PSCI_FW
>  obj-$(CONFIG_ARM)		+= s2-arm.o pm-arm.o
>  AFLAGS_s2-arm.o			:= -march=armv7-a
> diff --git a/drivers/soc/bcm/brcmstb/pm/pm-psci.c b/drivers/soc/bcm/brcmstb/pm/pm-psci.c
> new file mode 100644
> index 000000000000..7ba34d01c2fc
> --- /dev/null
> +++ b/drivers/soc/bcm/brcmstb/pm/pm-psci.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Broadcom STB PSCI based system wide PM support
> + *
> + * Copyright © 2018-2022 Broadcom
> + */
> +
> +#define pr_fmt(fmt) "brcmstb-pm-psci: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/panic_notifier.h>
> +#include <linux/psci.h>
> +#include <linux/suspend.h>
> +#include <linux/soc/brcmstb/brcmstb.h>
> +#include <linux/soc/brcmstb/brcmstb-smccc.h>
> +#include <linux/reboot.h>
> +#include <linux/kobject.h>
> +
> +#include <uapi/linux/psci.h>
> +
> +#include <asm/suspend.h>
> +#include <asm/system_misc.h>
> +
> +#include "aon_defs.h"
> +
> +static psci_fn *invoke_psci_fn;
> +static bool brcmstb_psci_system_reset2_supported;
> +static bool brcmstb_psci_system_suspend_supported;
> +static bool brcmstb_psci_cpu_retention = true;
> +
> +static int brcmstb_psci_integ_region(unsigned long function_id,
> +				     unsigned long base,
> +				     unsigned long size)
> +{
> +	unsigned long end;
> +
> +	if (!size)
> +		return -EINVAL;
> +
> +	end = DIV_ROUND_UP(base + size, SIP_MIN_REGION_SIZE);
> +	base /= SIP_MIN_REGION_SIZE;
> +	size = end - base;
> +
> +	return invoke_psci_fn(function_id, base, size, 0);
> +}
> +
> +static int __maybe_unused brcmstb_psci_integ_region_set(unsigned long base,
> +							unsigned long size)
> +{
> +	return brcmstb_psci_integ_region(SIP_FUNC_INTEG_REGION_SET, base, size);
> +}
> +
> +static int __maybe_unused brcmstb_psci_integ_region_del(unsigned long base,
> +							unsigned long size)
> +{
> +	return brcmstb_psci_integ_region(SIP_FUNC_INTEG_REGION_DEL, base, size);
> +}
> +
> +static int brcmstb_psci_integ_region_reset_all(void)
> +{
> +	return invoke_psci_fn(SIP_FUNC_INTEG_REGION_RESET_ALL, 0, 0, 0);
> +}

What's all this? Below I see the phrase "integrity checking regions", but only
the brcmstb_psci_integ_region_reset_all() function is used, and it's not clear
what this is supposed to be for.

> +static int brcmstb_psci_sys_reset(struct notifier_block *nb,
> +				  unsigned long action, void *data)
> +{
> +	const char *cmd = data;
> +	/*
> +	 * reset_type[31] = 0 (architectural)
> +	 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
> +	 * cookie = 0 (ignored by the implementation)
> +	 */
> +	uint32_t reboot_type = 0;
> +
> +	if ((action == REBOOT_COLD || action == REBOOT_WARM ||
> +	    action == REBOOT_SOFT) &&
> +	    brcmstb_psci_system_reset2_supported) {
> +		if (cmd && !strcmp(cmd, "powercycle"))
> +			reboot_type = BIT(31) | 1;
> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), reboot_type, 0, 0);
> +	} else {
> +		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +	}
> +
> +	return NOTIFY_DONE;
> +}

If there are a bunch of specific SYSTEM_RESET2 values we want to expose, I'd
rather we described those generically in the DT, and somehow handle that in the
generic driver.

> +
> +static struct notifier_block brcmstb_psci_sys_reset_nb = {
> +	.notifier_call	= brcmstb_psci_sys_reset,
> +	.priority	= 128,
> +};
> +
> +void brcmstb_psci_sys_poweroff(void)
> +{
> +	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +}
> +
> +static int psci_features(u32 psci_func_id)
> +{
> +	u32 features_func_id;
> +
> +	switch (ARM_SMCCC_OWNER_NUM(psci_func_id)) {
> +	case ARM_SMCCC_OWNER_SIP:
> +		features_func_id = SIP_FUNC_PSCI_FEATURES;
> +		break;
> +	case ARM_SMCCC_OWNER_STANDARD:
> +		features_func_id = PSCI_1_0_FN_PSCI_FEATURES;
> +		break;
> +	default:
> +		return PSCI_RET_NOT_SUPPORTED;
> +	}
> +
> +	return invoke_psci_fn(features_func_id, psci_func_id, 0, 0);
> +}
> +
> +static int brcmstb_psci_enter(suspend_state_t state)
> +{
> +	/* Request a SYSTEM level power state with retention */
> +	u32 pstate = 2 << PSCI_0_2_POWER_STATE_AFFL_SHIFT |
> +		     !brcmstb_psci_cpu_retention << PSCI_0_2_POWER_STATE_TYPE_SHIFT;
> +	int ret = -EINVAL;
> +
> +	switch (state) {
> +	case PM_SUSPEND_STANDBY:
> +		ret = psci_cpu_suspend_enter(pstate);
> +		break;
> +	case PM_SUSPEND_MEM:
> +		ret = brcmstb_psci_system_suspend_supported ?
> +			 psci_system_suspend_enter(state) : -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int brcmstb_psci_valid(suspend_state_t state)
> +{
> +	switch (state) {
> +	case PM_SUSPEND_STANDBY:
> +		return true;
> +	case PM_SUSPEND_MEM:
> +		return brcmstb_psci_system_suspend_supported;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct platform_suspend_ops brcmstb_psci_ops = {
> +	.enter	= brcmstb_psci_enter,
> +	.valid	= brcmstb_psci_valid,
> +};
> +
> +static int brcmstb_psci_panic_notify(struct notifier_block *nb,
> +				     unsigned long action, void *data)
> +{
> +	int ret;
> +
> +	ret = invoke_psci_fn(SIP_FUNC_PANIC_NOTIFY, BRCMSTB_PANIC_MAGIC, 0, 0);
> +	if (ret != PSCI_RET_SUCCESS)
> +		return NOTIFY_BAD;
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block brcmstb_psci_nb = {
> +	.notifier_call = brcmstb_psci_panic_notify,
> +};

This appears to have nothing to do with idle/suspend states (and so might be OK
on its own if you need it, but it should be in a separate patch with some
justification).

> +static ssize_t brcmstb_psci_version_show(struct kobject *kobj,
> +					 struct kobj_attribute *attr,
> +					 char *buf)
> +{
> +	struct arm_smccc_res res = { };
> +	u32 version;
> +
> +	if (invoke_psci_fn == __invoke_psci_fn_hvc)
> +		arm_smccc_hvc(SIP_FUNC_PSCI_BRCMSTB_VERSION,
> +			      0, 0, 0, 0, 0, 0, 0, &res);
> +	else
> +		arm_smccc_smc(SIP_FUNC_PSCI_BRCMSTB_VERSION,
> +			      0, 0, 0, 0, 0, 0, 0, &res);
> +
> +	if (res.a0 != PSCI_RET_SUCCESS)
> +		return -EOPNOTSUPP;
> +
> +	version = res.a1;
> +
> +	return sprintf(buf, "%d.%d.%d.%d\n",
> +		       (version >> 24) & 0xff, (version >> 16) & 0xff,
> +		       (version >> 8) & 0xff, version & 0xff);
> +}
> +
> +static struct kobj_attribute brcmstb_psci_version_attr =
> +	__ATTR(mon_version, 0400, brcmstb_psci_version_show, NULL);
> +
> +static ssize_t brcmstb_psci_cpu_retention_show(struct kobject *kobj,
> +					       struct kobj_attribute *attr,
> +					       char *buf)
> +{
> +	return sprintf(buf, "%d\n", brcmstb_psci_cpu_retention);
> +}
> +
> +static ssize_t brcmstb_psci_cpu_retention_store(struct kobject *kobj,
> +						struct kobj_attribute *attr,
> +						const char *buf, size_t count)
> +{
> +	int ret, val;
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val != 0 && val != 1)
> +		return -EINVAL;
> +
> +	brcmstb_psci_cpu_retention = !!val;
> +
> +	return count;
> +}
> +
> +static struct kobj_attribute brcmstb_psci_cpu_retention_attr =
> +	__ATTR(cpu_retention, 0644, brcmstb_psci_cpu_retention_show,
> +	       brcmstb_psci_cpu_retention_store);
> +
> +static const struct attribute *brcmstb_psci_attributes[] = {
> +	&brcmstb_psci_version_attr.attr,
> +	&brcmstb_psci_cpu_retention_attr.attr,
> +	NULL,
> +};
> +
> +static int brcmstb_pm_psci_init(void)
> +{
> +	unsigned long funcs_id[] = {
> +		PSCI_0_2_FN_SYSTEM_OFF,
> +		SIP_FUNC_INTEG_REGION_SET,
> +		SIP_FUNC_INTEG_REGION_DEL,
> +		SIP_FUNC_INTEG_REGION_RESET_ALL,
> +	};
> +	struct arm_smccc_res res = { };
> +	struct kobject *brcmstb_kobj;
> +	unsigned int i;
> +	int ret;
> +
> +	switch (arm_smccc_1_1_get_conduit()) {
> +	case SMCCC_CONDUIT_HVC:
> +		invoke_psci_fn = __invoke_psci_fn_hvc;
> +		break;
> +	case SMCCC_CONDUIT_SMC:
> +		invoke_psci_fn = __invoke_psci_fn_smc;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Immediately after this switch, we know we there is *a* SMCCCv1.1+
implementation, but we have no idea *which* implementation that is. It could be
Broadcom's brcmstb implementation, it could be KVM's implementation, or anyone
else's...

> +
> +	/* Check the revision of monitor */
> +	if (invoke_psci_fn == __invoke_psci_fn_hvc)
> +		arm_smccc_hvc(SIP_SVC_REVISION,
> +			      0, 0, 0, 0, 0, 0, 0, &res);
> +	else
> +		arm_smccc_smc(SIP_SVC_REVISION,
> +			      0, 0, 0, 0, 0, 0, 0, &res);

This tells us the SIP interface's revision (if implemented), but not that the
SIP is Broadcom, and we still don't know that this is the brcmstb
implementation specifically.

> +	/* Test for our supported features */
> +	for (i = 0; i < ARRAY_SIZE(funcs_id); i++) {
> +		ret = psci_features(funcs_id[i]);
> +		if (ret == PSCI_RET_NOT_SUPPORTED) {
> +			pr_err("Firmware does not support function 0x%lx\n",
> +			       funcs_id[i]);
> +			return -EOPNOTSUPP;
> +		}
> +	}

For the SIP FIDs, this will invoke SIP_FUNC_PSCI_FEATURES, and since we haven't
determined the specific FW implementation, that could be another SIP's function
that does arbitrary things (e.g. they might just return 0, and mislead you).

So this is unsafe.

> +
> +	ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
> +	if (ret != PSCI_RET_NOT_SUPPORTED)
> +		brcmstb_psci_system_reset2_supported = true;
> +
> +	ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND));
> +	if (ret != PSCI_RET_NOT_SUPPORTED)
> +		brcmstb_psci_system_suspend_supported = true;

Even if we've managed to get here, we still don't know that this is a brcmstb
implementation...

> +
> +	ret = brcmstb_psci_integ_region_reset_all();

... and as with all SIP calls, this might do arbitrary things.

> +	if (ret != PSCI_RET_SUCCESS) {
> +		pr_err("Error resetting all integrity checking regions\n");
> +		return -EIO;
> +	}
> +
> +	if (res.a0 == SIP_REVISION_MAJOR && res.a1 < SIP_REVISION_MINOR) {
> +		pr_info("Firmware is too old! Please update\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	brcmstb_kobj = kobject_create_and_add("brcmstb", firmware_kobj);
> +	if (brcmstb_kobj) {
> +		ret = sysfs_create_files(brcmstb_kobj, brcmstb_psci_attributes);
> +		if (ret)
> +			return ret;
> +	}

... and we can register this on a non-brcmstb implementation...

> +
> +	pm_power_off = brcmstb_psci_sys_poweroff;
> +	register_restart_handler(&brcmstb_psci_sys_reset_nb);
> +	suspend_set_ops(&brcmstb_psci_ops);
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &brcmstb_psci_nb);

... and we break power off, restart, and suspend on other implementations ...

> +	pr_info("Using PSCI based system PM (full featured)\n");

This should be explicit with something like "Overriding stnadard PSCI
functionaliy with brcmstb-specific code".

As it stands this is at best meaningless, and at worst misleading and
disparaging of standard PSCI.

> +
> +	return 0;
> +}
> +module_init(brcmstb_pm_psci_init);
> +
> +MODULE_AUTHOR("Broadcom");
> +MODULE_DESCRIPTION("Broadcom STB PM PSCI operations");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/soc/brcmstb/brcmstb-smccc.h b/include/linux/soc/brcmstb/brcmstb-smccc.h
> new file mode 100644
> index 000000000000..2863e894e1c7
> --- /dev/null
> +++ b/include/linux/soc/brcmstb/brcmstb-smccc.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __BRCMSTB_SMCCC_H
> +#define __BRCMSTB_SMCCC_H
> +
> +#include <linux/arm-smccc.h>
> +#include <uapi/linux/psci.h>
> +
> +#ifdef CONFIG_64BIT
> +#define PSCI_FN_NATIVE(version, name)   PSCI_##version##_FN64_##name
> +#else
> +#define PSCI_FN_NATIVE(version, name)   PSCI_##version##_FN_##name
> +#endif
> +
> +/* Broadcom STB custom SIP function calls */
> +#define SIP_FUNC_INTEG_REGION_SET	\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   0)
> +#define SIP_FUNC_INTEG_REGION_DEL	\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   1)
> +#define SIP_FUNC_INTEG_REGION_RESET_ALL	\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   2)
> +#define SIP_FUNC_PANIC_NOTIFY		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   3)
> +#define SIP_FUNC_PSCI_FEATURES		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   4)
> +#define SIP_FUNC_PSCI_BRCMSTB_VERSION		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   5)
> +
> +#define SIP_SVC_REVISION		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +			   IS_ENABLED(CONFIG_64BIT), \
> +			   ARM_SMCCC_OWNER_SIP, \
> +			   0xFF02)

Looking at the SMCCC spec, isn't the "general service query" REVISION call
0xFF03? 0xFF02 is reserved.

Thanks,
Mark.

> +
> +#define SIP_MIN_REGION_SIZE	4096
> +#define SIP_REVISION_MAJOR	0
> +#define SIP_REVISION_MINOR	2
> +
> +typedef unsigned long (psci_fn)(unsigned long, unsigned long,
> +				unsigned long, unsigned long);
> +
> +static inline unsigned long __invoke_psci_fn_hvc(unsigned long function_id,
> +						 unsigned long arg0,
> +						 unsigned long arg1,
> +						 unsigned long arg2)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_hvc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
> +static inline unsigned long __invoke_psci_fn_smc(unsigned long function_id,
> +						 unsigned long arg0,
> +						 unsigned long arg1,
> +						 unsigned long arg2)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
> +
> +#endif /* __BRCMSTB_SMCCC_H */
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 7205 bytes --]

Hi Mark,

On 2/3/2022 4:09 AM, Mark Rutland wrote:
> Hi,
> 
> On Fri, Jan 21, 2022 at 07:54:20PM -0800, Florian Fainelli wrote:
>> Add support for the Broadcom STB system suspend operations which
>> leverage the standard PSCI functions and uses the
>> psci_cpu_suspend_enter() operation to power off the system with or
>> without retention ("echo standby > /sys/power/state").
> 
> What exactly does this enable that can't be achieved with the existing PSCI
> driver as-is?
> 
> When you say "retention", what specifically do you mean? Retention of CPU
> state? DRAM contents?
> 
> We already have SYSTEM_SUSPEND for states where DRAM content is retained but
> CPU (and some system state) is lost, and IIUC we can do suspend-to-idle with
> CPU_SUSPEND states.
> 
> interface necessary?
> 
>> The system reset path also supports a special "powercycle" mode which
>> signals to the ARM Trusted Firmware that an external PMIC chip must
>> force the SoC into a power cycle.
> 
> How does that compare to the regular SYSTEM_RESET call?
> 
> The PSCI spec says of SYSTEM_RESET:
> 
> | This function provides a method for performing a system cold reset. To the
> | caller, the behavior is equivalent to a hardware power-cycle sequence.
> 
> ... so I don't follow how this is different, unless this platform's
> SYSTEM_RESET implementation is *not* actually performing a system cold reset?
> 
> If that *doesn't* perform a cold rest, it seems like a bug?

I think you answered that by looking at the code down below and the use 
case was to define a vendor specific method of resetting the chip. This 
is something that we could sort of always override one way or the other 
by registering our own power off notifier callback with a higher 
priority to make it take precedence, assuming that the platform is 
indeed brcmstb so we don't override other people's systems, too.

> 
>> As much as possible extensions were built using the SIP namespace rather
>> than the standard PSCI namespace, however compatibility with the
>> standard PSCI implementation is retained when CONFIG_BRCMSTB_PM is not
>> selected.
> 
> I really don't like this, because it seems to be creating a parallel
> infrastructure for doing things that can *already* be done with standard PSCI
> driver. The actual PSCI bits just seem to be playing about with the power_state
> value, which we should be able to do in the regular PSCI driver, and the
> SIP-specific functions seem to have nothing to do with PSCI.

The implementation is standard in the sense that no PSCI function call 
had to be modified in a non-standard way for system wide suspend/resume 
operations to work, but yet the mix of SiP and PSCI is not properly used 
to differentiate the platform as you reported.

> 
> At the least there needs to be a much better explanation of why this is
> necessary, but overall I'd very much like to have *any* vendor specific code
> for suspend states, and if there are limitations in the standard PSCI driver we
> go and address those. Otherwise we're going to gain a plethora of
> vendor-specific suspend implementations, which is exactly what PSCI was
> intended to avoid in the first place.

Entirely fair.

> 
> I have some specific comments below, but even with those addressed, I don't
> think this is the right approach, and as things stand, NAK to this.

That is fair, I think I have a clearer view of how to support some of 
our uses cases by extending the existing PSCI in ways that is hopefully 
acceptable.

[snip]

>> +static int brcmstb_psci_integ_region_reset_all(void)
>> +{
>> +	return invoke_psci_fn(SIP_FUNC_INTEG_REGION_RESET_ALL, 0, 0, 0);
>> +}
> 
> What's all this? Below I see the phrase "integrity checking regions", but only
> the brcmstb_psci_integ_region_reset_all() function is used, and it's not clear
> what this is supposed to be for.

Will remove that. We have a set of function calls here that allow us to 
define which specific areas of DRAM are to be hash checked during 
suspend, and then hash checked again during resume. This is used both as 
a debugging tool to spot faulty board designs where DRAM power is not 
retained as it should as well as a security counter measure.

> 
>> +static int brcmstb_psci_sys_reset(struct notifier_block *nb,
>> +				  unsigned long action, void *data)
>> +{
>> +	const char *cmd = data;
>> +	/*
>> +	 * reset_type[31] = 0 (architectural)
>> +	 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
>> +	 * cookie = 0 (ignored by the implementation)
>> +	 */
>> +	uint32_t reboot_type = 0;
>> +
>> +	if ((action == REBOOT_COLD || action == REBOOT_WARM ||
>> +	    action == REBOOT_SOFT) &&
>> +	    brcmstb_psci_system_reset2_supported) {
>> +		if (cmd && !strcmp(cmd, "powercycle"))
>> +			reboot_type = BIT(31) | 1;
>> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), reboot_type, 0, 0);
>> +	} else {
>> +		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
> 
> If there are a bunch of specific SYSTEM_RESET2 values we want to expose, I'd
> rather we described those generically in the DT, and somehow handle that in the
> generic driver.

OK, it that seems appropriate, I will propose something that extends the 
DT binding and standard PSCI implementation.

[snip]

>> +
>> +static struct notifier_block brcmstb_psci_nb = {
>> +	.notifier_call = brcmstb_psci_panic_notify,
>> +};
> 
> This appears to have nothing to do with idle/suspend states (and so might be OK
> on its own if you need it, but it should be in a separate patch with some
> justification).

Correct this is orthogonal and this is just a signal to the firmware 
that kernel has panicked.

[snip]

> 
> Immediately after this switch, we know we there is *a* SMCCCv1.1+
> implementation, but we have no idea *which* implementation that is. It could be
> Broadcom's brcmstb implementation, it could be KVM's implementation, or anyone
> else's...
> 
>> +
>> +	/* Check the revision of monitor */
>> +	if (invoke_psci_fn == __invoke_psci_fn_hvc)
>> +		arm_smccc_hvc(SIP_SVC_REVISION,
>> +			      0, 0, 0, 0, 0, 0, 0, &res);
>> +	else
>> +		arm_smccc_smc(SIP_SVC_REVISION,
>> +			      0, 0, 0, 0, 0, 0, 0, &res);
> 
> This tells us the SIP interface's revision (if implemented), but not that the
> SIP is Broadcom, and we still don't know that this is the brcmstb
> implementation specifically.

Good point.

[snip]

> 
>> +	pr_info("Using PSCI based system PM (full featured)\n");
> 
> This should be explicit with something like "Overriding stnadard PSCI
> functionaliy with brcmstb-specific code".
> 
> As it stands this is at best meaningless, and at worst misleading and
> disparaging of standard PSCI.

This is meaningless and a left over from our downstream tree where it is 
used to determine the generational level of the implementation, I will 
definitively remove it.

[snip]

>> +#define SIP_SVC_REVISION		\
>> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
>> +			   IS_ENABLED(CONFIG_64BIT), \
>> +			   ARM_SMCCC_OWNER_SIP, \
>> +			   0xFF02)
> 
> Looking at the SMCCC spec, isn't the "general service query" REVISION call
> 0xFF03? 0xFF02 is reserved.

Sure is, whoever wrote that probably did not know it at the time (not me!).
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

[Florian Fainelli]

[-- Attachment #1.1: Type: text/plain, Size: 7205 bytes --]

Hi Mark,

On 2/3/2022 4:09 AM, Mark Rutland wrote:
> Hi,
> 
> On Fri, Jan 21, 2022 at 07:54:20PM -0800, Florian Fainelli wrote:
>> Add support for the Broadcom STB system suspend operations which
>> leverage the standard PSCI functions and uses the
>> psci_cpu_suspend_enter() operation to power off the system with or
>> without retention ("echo standby > /sys/power/state").
> 
> What exactly does this enable that can't be achieved with the existing PSCI
> driver as-is?
> 
> When you say "retention", what specifically do you mean? Retention of CPU
> state? DRAM contents?
> 
> We already have SYSTEM_SUSPEND for states where DRAM content is retained but
> CPU (and some system state) is lost, and IIUC we can do suspend-to-idle with
> CPU_SUSPEND states.
> 
> interface necessary?
> 
>> The system reset path also supports a special "powercycle" mode which
>> signals to the ARM Trusted Firmware that an external PMIC chip must
>> force the SoC into a power cycle.
> 
> How does that compare to the regular SYSTEM_RESET call?
> 
> The PSCI spec says of SYSTEM_RESET:
> 
> | This function provides a method for performing a system cold reset. To the
> | caller, the behavior is equivalent to a hardware power-cycle sequence.
> 
> ... so I don't follow how this is different, unless this platform's
> SYSTEM_RESET implementation is *not* actually performing a system cold reset?
> 
> If that *doesn't* perform a cold rest, it seems like a bug?

I think you answered that by looking at the code down below and the use 
case was to define a vendor specific method of resetting the chip. This 
is something that we could sort of always override one way or the other 
by registering our own power off notifier callback with a higher 
priority to make it take precedence, assuming that the platform is 
indeed brcmstb so we don't override other people's systems, too.

> 
>> As much as possible extensions were built using the SIP namespace rather
>> than the standard PSCI namespace, however compatibility with the
>> standard PSCI implementation is retained when CONFIG_BRCMSTB_PM is not
>> selected.
> 
> I really don't like this, because it seems to be creating a parallel
> infrastructure for doing things that can *already* be done with standard PSCI
> driver. The actual PSCI bits just seem to be playing about with the power_state
> value, which we should be able to do in the regular PSCI driver, and the
> SIP-specific functions seem to have nothing to do with PSCI.

The implementation is standard in the sense that no PSCI function call 
had to be modified in a non-standard way for system wide suspend/resume 
operations to work, but yet the mix of SiP and PSCI is not properly used 
to differentiate the platform as you reported.

> 
> At the least there needs to be a much better explanation of why this is
> necessary, but overall I'd very much like to have *any* vendor specific code
> for suspend states, and if there are limitations in the standard PSCI driver we
> go and address those. Otherwise we're going to gain a plethora of
> vendor-specific suspend implementations, which is exactly what PSCI was
> intended to avoid in the first place.

Entirely fair.

> 
> I have some specific comments below, but even with those addressed, I don't
> think this is the right approach, and as things stand, NAK to this.

That is fair, I think I have a clearer view of how to support some of 
our uses cases by extending the existing PSCI in ways that is hopefully 
acceptable.

[snip]

>> +static int brcmstb_psci_integ_region_reset_all(void)
>> +{
>> +	return invoke_psci_fn(SIP_FUNC_INTEG_REGION_RESET_ALL, 0, 0, 0);
>> +}
> 
> What's all this? Below I see the phrase "integrity checking regions", but only
> the brcmstb_psci_integ_region_reset_all() function is used, and it's not clear
> what this is supposed to be for.

Will remove that. We have a set of function calls here that allow us to 
define which specific areas of DRAM are to be hash checked during 
suspend, and then hash checked again during resume. This is used both as 
a debugging tool to spot faulty board designs where DRAM power is not 
retained as it should as well as a security counter measure.

> 
>> +static int brcmstb_psci_sys_reset(struct notifier_block *nb,
>> +				  unsigned long action, void *data)
>> +{
>> +	const char *cmd = data;
>> +	/*
>> +	 * reset_type[31] = 0 (architectural)
>> +	 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
>> +	 * cookie = 0 (ignored by the implementation)
>> +	 */
>> +	uint32_t reboot_type = 0;
>> +
>> +	if ((action == REBOOT_COLD || action == REBOOT_WARM ||
>> +	    action == REBOOT_SOFT) &&
>> +	    brcmstb_psci_system_reset2_supported) {
>> +		if (cmd && !strcmp(cmd, "powercycle"))
>> +			reboot_type = BIT(31) | 1;
>> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), reboot_type, 0, 0);
>> +	} else {
>> +		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
> 
> If there are a bunch of specific SYSTEM_RESET2 values we want to expose, I'd
> rather we described those generically in the DT, and somehow handle that in the
> generic driver.

OK, it that seems appropriate, I will propose something that extends the 
DT binding and standard PSCI implementation.

[snip]

>> +
>> +static struct notifier_block brcmstb_psci_nb = {
>> +	.notifier_call = brcmstb_psci_panic_notify,
>> +};
> 
> This appears to have nothing to do with idle/suspend states (and so might be OK
> on its own if you need it, but it should be in a separate patch with some
> justification).

Correct this is orthogonal and this is just a signal to the firmware 
that kernel has panicked.

[snip]

> 
> Immediately after this switch, we know we there is *a* SMCCCv1.1+
> implementation, but we have no idea *which* implementation that is. It could be
> Broadcom's brcmstb implementation, it could be KVM's implementation, or anyone
> else's...
> 
>> +
>> +	/* Check the revision of monitor */
>> +	if (invoke_psci_fn == __invoke_psci_fn_hvc)
>> +		arm_smccc_hvc(SIP_SVC_REVISION,
>> +			      0, 0, 0, 0, 0, 0, 0, &res);
>> +	else
>> +		arm_smccc_smc(SIP_SVC_REVISION,
>> +			      0, 0, 0, 0, 0, 0, 0, &res);
> 
> This tells us the SIP interface's revision (if implemented), but not that the
> SIP is Broadcom, and we still don't know that this is the brcmstb
> implementation specifically.

Good point.

[snip]

> 
>> +	pr_info("Using PSCI based system PM (full featured)\n");
> 
> This should be explicit with something like "Overriding stnadard PSCI
> functionaliy with brcmstb-specific code".
> 
> As it stands this is at best meaningless, and at worst misleading and
> disparaging of standard PSCI.

This is meaningless and a left over from our downstream tree where it is 
used to determine the generational level of the implementation, I will 
definitively remove it.

[snip]

>> +#define SIP_SVC_REVISION		\
>> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
>> +			   IS_ENABLED(CONFIG_64BIT), \
>> +			   ARM_SMCCC_OWNER_SIP, \
>> +			   0xFF02)
> 
> Looking at the SMCCC spec, isn't the "general service query" REVISION call
> 0xFF03? 0xFF02 is reserved.

Sure is, whoever wrote that probably did not know it at the time (not me!).
-- 
Florian

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] Documentation: ABI: Document Broadcom STB PSCI firmware files

[Florian Fainelli]

Document the attributes created by the Broadcom STB PSCI firmware code
to allow retrieving the firmware version and the control of the CPU
retention upon system suspend entry.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/ABI/testing/sysfs-firmware-brcmstb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-brcmstb

diff --git a/Documentation/ABI/testing/sysfs-firmware-brcmstb b/Documentation/ABI/testing/sysfs-firmware-brcmstb
new file mode 100644
index 000000000000..5ebd8d0a34be
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-brcmstb
@@ -0,0 +1,16 @@
+What:		/sys/firmware/brcmstb/mon_version
+Date:		Jan 2022
+Contact:	Florian Fainelli <f.fainelli@gmail.com>
+Description:
+		Reports the ARM trusted firmware (monitor) version running on
+		the Broadcom STB platforms under the format:
+		A.B.C.D
+
+What:		/sys/firmware/brcmstb/cpu_retention
+Date:		Jan 2022
+Contact:	Florian Fainelli <f.fainelli@gmail.com>
+Description:
+		Allows the toggling of whether S2 standby should be entered
+		with (default) or without CPU retention.
+
+		Accepted values are 0 or 1.
-- 
2.25.1

[PATCH 4/4] Documentation: ABI: Document Broadcom STB PSCI firmware files

[Florian Fainelli]

Document the attributes created by the Broadcom STB PSCI firmware code
to allow retrieving the firmware version and the control of the CPU
retention upon system suspend entry.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/ABI/testing/sysfs-firmware-brcmstb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-brcmstb

diff --git a/Documentation/ABI/testing/sysfs-firmware-brcmstb b/Documentation/ABI/testing/sysfs-firmware-brcmstb
new file mode 100644
index 000000000000..5ebd8d0a34be
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-brcmstb
@@ -0,0 +1,16 @@
+What:		/sys/firmware/brcmstb/mon_version
+Date:		Jan 2022
+Contact:	Florian Fainelli <f.fainelli@gmail.com>
+Description:
+		Reports the ARM trusted firmware (monitor) version running on
+		the Broadcom STB platforms under the format:
+		A.B.C.D
+
+What:		/sys/firmware/brcmstb/cpu_retention
+Date:		Jan 2022
+Contact:	Florian Fainelli <f.fainelli@gmail.com>
+Description:
+		Allows the toggling of whether S2 standby should be entered
+		with (default) or without CPU retention.
+
+		Accepted values are 0 or 1.
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

On 1/21/2022 7:54 PM, Florian Fainelli wrote:
> Hi all,
> 
> This patch series contains the Broadcom STB PSCI extensions which adds
> some additional functions on top of the existing standard PSCI interface
> which is the reason for having the driver implement a custom
> suspend_ops.
> 
> These platforms have traditionally supported a mode that is akin to
> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> which is entered with "echo standby > /sys/power/state". Additional a
> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> mem > /sys/power/state".
> 
> These platforms also may have an external Broadcom PMIC chip which can
> cause the SoC to be powercycled assuming that we communicate that intent
> via a vendor specific PSCI SYSTEM_RESET2.
> 
> Since it is desirable to get any new functionality added to the kernel
> to be loadable as a module as part of shipping said products in a Google
> Kernel Image (GKI) environment, we need to export a couple of symbols from
> drivers/firmware/psci/psci.c.
> 
> Thanks for your feedback.

I will be sending a v2 addressing the kbuild robot failures reported but 
would prefer to get some feedback first so it can be addressed 
simultaneously. Thanks!

> 
> Florian Fainelli (4):
>    firmware: psci: Export a couple of suspend symbols
>    soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW
>    soc: bcm: brcmstb: Added support for PSCI system suspend operations
>    Documentation: ABI: Document Broadcom STB PSCI firmware files
> 
>   .../ABI/testing/sysfs-firmware-brcmstb        |  16 +
>   drivers/firmware/psci/psci.c                  |   9 +-
>   drivers/soc/bcm/brcmstb/Kconfig               |   4 +-
>   drivers/soc/bcm/brcmstb/pm/Makefile           |   3 +
>   drivers/soc/bcm/brcmstb/pm/pm-psci.c          | 315 ++++++++++++++++++
>   include/linux/psci.h                          |   2 +
>   include/linux/soc/brcmstb/brcmstb-smccc.h     |  84 +++++
>   7 files changed, 430 insertions(+), 3 deletions(-)
>   create mode 100644 Documentation/ABI/testing/sysfs-firmware-brcmstb
>   create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
>   create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h
> 

-- 
Florian

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

On 1/21/2022 7:54 PM, Florian Fainelli wrote:
> Hi all,
> 
> This patch series contains the Broadcom STB PSCI extensions which adds
> some additional functions on top of the existing standard PSCI interface
> which is the reason for having the driver implement a custom
> suspend_ops.
> 
> These platforms have traditionally supported a mode that is akin to
> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> which is entered with "echo standby > /sys/power/state". Additional a
> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> mem > /sys/power/state".
> 
> These platforms also may have an external Broadcom PMIC chip which can
> cause the SoC to be powercycled assuming that we communicate that intent
> via a vendor specific PSCI SYSTEM_RESET2.
> 
> Since it is desirable to get any new functionality added to the kernel
> to be loadable as a module as part of shipping said products in a Google
> Kernel Image (GKI) environment, we need to export a couple of symbols from
> drivers/firmware/psci/psci.c.
> 
> Thanks for your feedback.

I will be sending a v2 addressing the kbuild robot failures reported but 
would prefer to get some feedback first so it can be addressed 
simultaneously. Thanks!

> 
> Florian Fainelli (4):
>    firmware: psci: Export a couple of suspend symbols
>    soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW
>    soc: bcm: brcmstb: Added support for PSCI system suspend operations
>    Documentation: ABI: Document Broadcom STB PSCI firmware files
> 
>   .../ABI/testing/sysfs-firmware-brcmstb        |  16 +
>   drivers/firmware/psci/psci.c                  |   9 +-
>   drivers/soc/bcm/brcmstb/Kconfig               |   4 +-
>   drivers/soc/bcm/brcmstb/pm/Makefile           |   3 +
>   drivers/soc/bcm/brcmstb/pm/pm-psci.c          | 315 ++++++++++++++++++
>   include/linux/psci.h                          |   2 +
>   include/linux/soc/brcmstb/brcmstb-smccc.h     |  84 +++++
>   7 files changed, 430 insertions(+), 3 deletions(-)
>   create mode 100644 Documentation/ABI/testing/sysfs-firmware-brcmstb
>   create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
>   create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h
> 

-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Mark Rutland]

On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
> Hi all,

Hi Florian,

> This patch series contains the Broadcom STB PSCI extensions which adds
> some additional functions on top of the existing standard PSCI interface
> which is the reason for having the driver implement a custom
> suspend_ops.

I *really* don't like the idea of having non-standard PSCI extensions, because
it somewhat defeats the point of PSCI being a standard, and opens the door for
the zoo of mechanisms we had on 32-bit.

I think this needs a fair amount more explanation and justification.

> These platforms have traditionally supported a mode that is akin to
> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> which is entered with "echo standby > /sys/power/state". Additional a
> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> mem > /sys/power/state".

Why isn't a combination of CPU_SUSPEND and SYSTEM_SUSPEND sufficient here?

What specifically *can't* you do with standard PSCI calls?

> These platforms also may have an external Broadcom PMIC chip which can
> cause the SoC to be powercycled assuming that we communicate that intent
> via a vendor specific PSCI SYSTEM_RESET2.
> 
> Since it is desirable to get any new functionality added to the kernel
> to be loadable as a module as part of shipping said products in a Google
> Kernel Image (GKI) environment, we need to export a couple of symbols from
> drivers/firmware/psci/psci.c.

I really don't want to export the guts of psci.c.

Thanks,
Mark.

> Thanks for your feedback.
> 
> Florian Fainelli (4):
>   firmware: psci: Export a couple of suspend symbols
>   soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW
>   soc: bcm: brcmstb: Added support for PSCI system suspend operations
>   Documentation: ABI: Document Broadcom STB PSCI firmware files
> 
>  .../ABI/testing/sysfs-firmware-brcmstb        |  16 +
>  drivers/firmware/psci/psci.c                  |   9 +-
>  drivers/soc/bcm/brcmstb/Kconfig               |   4 +-
>  drivers/soc/bcm/brcmstb/pm/Makefile           |   3 +
>  drivers/soc/bcm/brcmstb/pm/pm-psci.c          | 315 ++++++++++++++++++
>  include/linux/psci.h                          |   2 +
>  include/linux/soc/brcmstb/brcmstb-smccc.h     |  84 +++++
>  7 files changed, 430 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-brcmstb
>  create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
>  create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h
> 
> -- 
> 2.25.1
> 

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Mark Rutland]

On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
> Hi all,

Hi Florian,

> This patch series contains the Broadcom STB PSCI extensions which adds
> some additional functions on top of the existing standard PSCI interface
> which is the reason for having the driver implement a custom
> suspend_ops.

I *really* don't like the idea of having non-standard PSCI extensions, because
it somewhat defeats the point of PSCI being a standard, and opens the door for
the zoo of mechanisms we had on 32-bit.

I think this needs a fair amount more explanation and justification.

> These platforms have traditionally supported a mode that is akin to
> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> which is entered with "echo standby > /sys/power/state". Additional a
> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> mem > /sys/power/state".

Why isn't a combination of CPU_SUSPEND and SYSTEM_SUSPEND sufficient here?

What specifically *can't* you do with standard PSCI calls?

> These platforms also may have an external Broadcom PMIC chip which can
> cause the SoC to be powercycled assuming that we communicate that intent
> via a vendor specific PSCI SYSTEM_RESET2.
> 
> Since it is desirable to get any new functionality added to the kernel
> to be loadable as a module as part of shipping said products in a Google
> Kernel Image (GKI) environment, we need to export a couple of symbols from
> drivers/firmware/psci/psci.c.

I really don't want to export the guts of psci.c.

Thanks,
Mark.

> Thanks for your feedback.
> 
> Florian Fainelli (4):
>   firmware: psci: Export a couple of suspend symbols
>   soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW
>   soc: bcm: brcmstb: Added support for PSCI system suspend operations
>   Documentation: ABI: Document Broadcom STB PSCI firmware files
> 
>  .../ABI/testing/sysfs-firmware-brcmstb        |  16 +
>  drivers/firmware/psci/psci.c                  |   9 +-
>  drivers/soc/bcm/brcmstb/Kconfig               |   4 +-
>  drivers/soc/bcm/brcmstb/pm/Makefile           |   3 +
>  drivers/soc/bcm/brcmstb/pm/pm-psci.c          | 315 ++++++++++++++++++
>  include/linux/psci.h                          |   2 +
>  include/linux/soc/brcmstb/brcmstb-smccc.h     |  84 +++++
>  7 files changed, 430 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-brcmstb
>  create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-psci.c
>  create mode 100644 include/linux/soc/brcmstb/brcmstb-smccc.h
> 
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

Hello Mark,

On 2/3/2022 2:47 AM, Mark Rutland wrote:
> On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
>> Hi all,
> 
> Hi Florian,
> 
>> This patch series contains the Broadcom STB PSCI extensions which adds
>> some additional functions on top of the existing standard PSCI interface
>> which is the reason for having the driver implement a custom
>> suspend_ops.
> 
> I *really* don't like the idea of having non-standard PSCI extensions, because
> it somewhat defeats the point of PSCI being a standard, and opens the door for
> the zoo of mechanisms we had on 32-bit.
> 
> I think this needs a fair amount more explanation and justification.
> 
>> These platforms have traditionally supported a mode that is akin to
>> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
>> which is entered with "echo standby > /sys/power/state". Additional a
>> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
>> mem > /sys/power/state".
> 
> Why isn't a combination of CPU_SUSPEND and SYSTEM_SUSPEND sufficient here?

This is exactly what we are using, just the use of CPU_SUSPEND is not 
done via the CPU IDLE framework because our platforms did not wire up 
the ARM GIC power controller interrupt signals back to the power 
management controller of the system, but via registering a "standby" 
state into suspend_ops instead.

> 
> What specifically *can't* you do with standard PSCI calls?

Since you looked at the patches now, nothing at all, everything we do 
(with the exception of the funky SIP calls which are not strictly 
mandatory for system suspend operations) is done by using standard PSCI 
calls and leveraging the existing vendor space when needed (as with 
SYSTEM_RESET2 for instance).

> 
>> These platforms also may have an external Broadcom PMIC chip which can
>> cause the SoC to be powercycled assuming that we communicate that intent
>> via a vendor specific PSCI SYSTEM_RESET2.
>>
>> Since it is desirable to get any new functionality added to the kernel
>> to be loadable as a module as part of shipping said products in a Google
>> Kernel Image (GKI) environment, we need to export a couple of symbols from
>> drivers/firmware/psci/psci.c.
> 
> I really don't want to export the guts of psci.c.

I can appreciate that, and really the sticking point that required me to 
export the couple of symbols needed was because the alternatives would 
be to:

- to not make this code modular in the first place but that won't fly in 
the Google Kernel Image grand scheme of things where *everything* that 
is not necessary for boot must be a loadable module

- not support the "standby" mode which is not really an option since we 
rely on it to achieve our power targets

- export cpu_suspend from arch/*/kernel/suspend.c which is probably 
going to be a no-go plus duplicate the entire set of PSCI function calls 
to re-implement the psci_system_suspend_enter() functions

Thanks for taking a look!
-- 
Florian

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

Hello Mark,

On 2/3/2022 2:47 AM, Mark Rutland wrote:
> On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
>> Hi all,
> 
> Hi Florian,
> 
>> This patch series contains the Broadcom STB PSCI extensions which adds
>> some additional functions on top of the existing standard PSCI interface
>> which is the reason for having the driver implement a custom
>> suspend_ops.
> 
> I *really* don't like the idea of having non-standard PSCI extensions, because
> it somewhat defeats the point of PSCI being a standard, and opens the door for
> the zoo of mechanisms we had on 32-bit.
> 
> I think this needs a fair amount more explanation and justification.
> 
>> These platforms have traditionally supported a mode that is akin to
>> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
>> which is entered with "echo standby > /sys/power/state". Additional a
>> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
>> mem > /sys/power/state".
> 
> Why isn't a combination of CPU_SUSPEND and SYSTEM_SUSPEND sufficient here?

This is exactly what we are using, just the use of CPU_SUSPEND is not 
done via the CPU IDLE framework because our platforms did not wire up 
the ARM GIC power controller interrupt signals back to the power 
management controller of the system, but via registering a "standby" 
state into suspend_ops instead.

> 
> What specifically *can't* you do with standard PSCI calls?

Since you looked at the patches now, nothing at all, everything we do 
(with the exception of the funky SIP calls which are not strictly 
mandatory for system suspend operations) is done by using standard PSCI 
calls and leveraging the existing vendor space when needed (as with 
SYSTEM_RESET2 for instance).

> 
>> These platforms also may have an external Broadcom PMIC chip which can
>> cause the SoC to be powercycled assuming that we communicate that intent
>> via a vendor specific PSCI SYSTEM_RESET2.
>>
>> Since it is desirable to get any new functionality added to the kernel
>> to be loadable as a module as part of shipping said products in a Google
>> Kernel Image (GKI) environment, we need to export a couple of symbols from
>> drivers/firmware/psci/psci.c.
> 
> I really don't want to export the guts of psci.c.

I can appreciate that, and really the sticking point that required me to 
export the couple of symbols needed was because the alternatives would 
be to:

- to not make this code modular in the first place but that won't fly in 
the Google Kernel Image grand scheme of things where *everything* that 
is not necessary for boot must be a loadable module

- not support the "standby" mode which is not really an option since we 
rely on it to achieve our power targets

- export cpu_suspend from arch/*/kernel/suspend.c which is probably 
going to be a no-go plus duplicate the entire set of PSCI function calls 
to re-implement the psci_system_suspend_enter() functions

Thanks for taking a look!
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Sudeep Holla]

On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
> Hi all,
> 
> This patch series contains the Broadcom STB PSCI extensions which adds
> some additional functions on top of the existing standard PSCI interface
> which is the reason for having the driver implement a custom
> suspend_ops.
> 
> These platforms have traditionally supported a mode that is akin to
> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> which is entered with "echo standby > /sys/power/state". Additional a
> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> mem > /sys/power/state".

How different is the above "standby" state compare to the standard "idle"
(a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
Suspend to idle takes all the CPUs to lowest possible power state instead
of cpu-hotplug in S2R. Also I assume some userspace has to identify when
to enter "standby" vs "mem" right ? I am trying to see how addition of
"idle" changes that(if it does). Sorry for too many questions.

-- 
Regards,
Sudeep

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Sudeep Holla]

On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
> Hi all,
> 
> This patch series contains the Broadcom STB PSCI extensions which adds
> some additional functions on top of the existing standard PSCI interface
> which is the reason for having the driver implement a custom
> suspend_ops.
> 
> These platforms have traditionally supported a mode that is akin to
> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> which is entered with "echo standby > /sys/power/state". Additional a
> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> mem > /sys/power/state".

How different is the above "standby" state compare to the standard "idle"
(a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
Suspend to idle takes all the CPUs to lowest possible power state instead
of cpu-hotplug in S2R. Also I assume some userspace has to identify when
to enter "standby" vs "mem" right ? I am trying to see how addition of
"idle" changes that(if it does). Sorry for too many questions.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

On 2/3/2022 3:14 AM, Sudeep Holla wrote:
> On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
>> Hi all,
>>
>> This patch series contains the Broadcom STB PSCI extensions which adds
>> some additional functions on top of the existing standard PSCI interface
>> which is the reason for having the driver implement a custom
>> suspend_ops.
>>
>> These platforms have traditionally supported a mode that is akin to
>> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
>> which is entered with "echo standby > /sys/power/state". Additional a
>> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
>> mem > /sys/power/state".
> 
> How different is the above "standby" state compare to the standard "idle"
> (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?

There are a few differences:

- s2idle does not power gate the secondary CPUs

- s2idle requires the use of in-band interrupts for wake-up

The reasons for implementing "standby" are largely two fold:

- we need to achieve decent power savings (typically below 0.5W for the 
whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)

- we have a security subsystem that requires the CPUs to be either power 
gated or idle in order the hardware state machine that lets the system 
enter such a state and allows the out of band interrupts from being 
wake-up sources


> Suspend to idle takes all the CPUs to lowest possible power state instead
> of cpu-hotplug in S2R. Also I assume some userspace has to identify when
> to enter "standby" vs "mem" right ? I am trying to see how addition of
> "idle" changes that(if it does). Sorry for too many questions.
> 

Right that user-space in our case is either custom (like RDK, or 
completely custom), or is Android. For Android it looks like we are 
carrying a patch that makes "mem" de-generate into "standby" but this is 
largely because we had historically problems with "mem" that are being 
addressed (completely orthogonal).

I did not consider it as a viable option at the time, but if we were to 
implement "standby" in drivers/firmware/psci/psci.c would that be 
somewhat acceptable?
-- 
Florian

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

On 2/3/2022 3:14 AM, Sudeep Holla wrote:
> On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
>> Hi all,
>>
>> This patch series contains the Broadcom STB PSCI extensions which adds
>> some additional functions on top of the existing standard PSCI interface
>> which is the reason for having the driver implement a custom
>> suspend_ops.
>>
>> These platforms have traditionally supported a mode that is akin to
>> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
>> which is entered with "echo standby > /sys/power/state". Additional a
>> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
>> mem > /sys/power/state".
> 
> How different is the above "standby" state compare to the standard "idle"
> (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?

There are a few differences:

- s2idle does not power gate the secondary CPUs

- s2idle requires the use of in-band interrupts for wake-up

The reasons for implementing "standby" are largely two fold:

- we need to achieve decent power savings (typically below 0.5W for the 
whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)

- we have a security subsystem that requires the CPUs to be either power 
gated or idle in order the hardware state machine that lets the system 
enter such a state and allows the out of band interrupts from being 
wake-up sources


> Suspend to idle takes all the CPUs to lowest possible power state instead
> of cpu-hotplug in S2R. Also I assume some userspace has to identify when
> to enter "standby" vs "mem" right ? I am trying to see how addition of
> "idle" changes that(if it does). Sorry for too many questions.
> 

Right that user-space in our case is either custom (like RDK, or 
completely custom), or is Android. For Android it looks like we are 
carrying a patch that makes "mem" de-generate into "standby" but this is 
largely because we had historically problems with "mem" that are being 
addressed (completely orthogonal).

I did not consider it as a viable option at the time, but if we were to 
implement "standby" in drivers/firmware/psci/psci.c would that be 
somewhat acceptable?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Sudeep Holla]

Correction: it is known as "freeze" rather than "idle" in terms of values
as per /sys/power/state. Sorry for referring it as "idle" and creating any
confusion.

On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote:
> 
> 
> On 2/3/2022 3:14 AM, Sudeep Holla wrote:
> > On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
> > > Hi all,
> > > 
> > > This patch series contains the Broadcom STB PSCI extensions which adds
> > > some additional functions on top of the existing standard PSCI interface
> > > which is the reason for having the driver implement a custom
> > > suspend_ops.
> > > 
> > > These platforms have traditionally supported a mode that is akin to
> > > ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> > > which is entered with "echo standby > /sys/power/state". Additional a
> > > true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> > > mem > /sys/power/state".
> > 
> > How different is the above "standby" state compare to the standard "idle"
> > (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
> 
> There are a few differences:
> 
> - s2idle does not power gate the secondary CPUs
>

Not sure what you mean by that ? S2I takes CPUs to deepest idle state.
If you want shallower states, one possible option is the disable deeper
states from the userspace.

> - s2idle requires the use of in-band interrupts for wake-up
>

I am not sure if that is true. S2I behaves very similar to S2R except it
has low wake latency as all secondaries CPUs are not hotplugged out.

> The reasons for implementing "standby" are largely two fold:
>
> - we need to achieve decent power savings (typically below 0.5W for the
> whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)
>

I fail to understand how that is a problem from S2I. It is probably worth
checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section
IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other
wise in terms of unnecessary/spurious wakeup by in-band(to be precise
no-wake up) interrupts.

> - we have a security subsystem that requires the CPUs to be either power
> gated or idle in order the hardware state machine that lets the system enter
> such a state and allows the out of band interrupts from being wake-up
> sources
>

It should work unless I have completely misunderstood how S2I works.

> > Suspend to idle takes all the CPUs to lowest possible power state instead
> > of cpu-hotplug in S2R. Also I assume some userspace has to identify when
> > to enter "standby" vs "mem" right ? I am trying to see how addition of
> > "idle" changes that(if it does). Sorry for too many questions.
> > 
> 
> Right that user-space in our case is either custom (like RDK, or completely
> custom), or is Android. For Android it looks like we are carrying a patch
> that makes "mem" de-generate into "standby" but this is largely because we
> had historically problems with "mem" that are being addressed (completely
> orthogonal).
>

Thanks for the info.

> I did not consider it as a viable option at the time, but if we were to
> implement "standby" in drivers/firmware/psci/psci.c would that be somewhat
> acceptable?
>

We have been pointing anyone needing standby so far to S2I and so far no one
has shouted that it doesn't suffice. Let me know what is missing.

-- 
Regards,
Sudeep

[1] Documentation/power/suspend-and-interrupts.rst

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Sudeep Holla]

Correction: it is known as "freeze" rather than "idle" in terms of values
as per /sys/power/state. Sorry for referring it as "idle" and creating any
confusion.

On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote:
> 
> 
> On 2/3/2022 3:14 AM, Sudeep Holla wrote:
> > On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
> > > Hi all,
> > > 
> > > This patch series contains the Broadcom STB PSCI extensions which adds
> > > some additional functions on top of the existing standard PSCI interface
> > > which is the reason for having the driver implement a custom
> > > suspend_ops.
> > > 
> > > These platforms have traditionally supported a mode that is akin to
> > > ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> > > which is entered with "echo standby > /sys/power/state". Additional a
> > > true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> > > mem > /sys/power/state".
> > 
> > How different is the above "standby" state compare to the standard "idle"
> > (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
> 
> There are a few differences:
> 
> - s2idle does not power gate the secondary CPUs
>

Not sure what you mean by that ? S2I takes CPUs to deepest idle state.
If you want shallower states, one possible option is the disable deeper
states from the userspace.

> - s2idle requires the use of in-band interrupts for wake-up
>

I am not sure if that is true. S2I behaves very similar to S2R except it
has low wake latency as all secondaries CPUs are not hotplugged out.

> The reasons for implementing "standby" are largely two fold:
>
> - we need to achieve decent power savings (typically below 0.5W for the
> whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)
>

I fail to understand how that is a problem from S2I. It is probably worth
checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section
IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other
wise in terms of unnecessary/spurious wakeup by in-band(to be precise
no-wake up) interrupts.

> - we have a security subsystem that requires the CPUs to be either power
> gated or idle in order the hardware state machine that lets the system enter
> such a state and allows the out of band interrupts from being wake-up
> sources
>

It should work unless I have completely misunderstood how S2I works.

> > Suspend to idle takes all the CPUs to lowest possible power state instead
> > of cpu-hotplug in S2R. Also I assume some userspace has to identify when
> > to enter "standby" vs "mem" right ? I am trying to see how addition of
> > "idle" changes that(if it does). Sorry for too many questions.
> > 
> 
> Right that user-space in our case is either custom (like RDK, or completely
> custom), or is Android. For Android it looks like we are carrying a patch
> that makes "mem" de-generate into "standby" but this is largely because we
> had historically problems with "mem" that are being addressed (completely
> orthogonal).
>

Thanks for the info.

> I did not consider it as a viable option at the time, but if we were to
> implement "standby" in drivers/firmware/psci/psci.c would that be somewhat
> acceptable?
>

We have been pointing anyone needing standby so far to S2I and so far no one
has shouted that it doesn't suffice. Let me know what is missing.

-- 
Regards,
Sudeep

[1] Documentation/power/suspend-and-interrupts.rst

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

On 2/3/2022 10:52 AM, Sudeep Holla wrote:
> Correction: it is known as "freeze" rather than "idle" in terms of values
> as per /sys/power/state. Sorry for referring it as "idle" and creating any
> confusion.
> 
> On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/3/2022 3:14 AM, Sudeep Holla wrote:
>>> On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
>>>> Hi all,
>>>>
>>>> This patch series contains the Broadcom STB PSCI extensions which adds
>>>> some additional functions on top of the existing standard PSCI interface
>>>> which is the reason for having the driver implement a custom
>>>> suspend_ops.
>>>>
>>>> These platforms have traditionally supported a mode that is akin to
>>>> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
>>>> which is entered with "echo standby > /sys/power/state". Additional a
>>>> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
>>>> mem > /sys/power/state".
>>>
>>> How different is the above "standby" state compare to the standard "idle"
>>> (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
>>
>> There are a few differences:
>>
>> - s2idle does not power gate the secondary CPUs
>>
> 
> Not sure what you mean by that ? S2I takes CPUs to deepest idle state.
> If you want shallower states, one possible option is the disable deeper
> states from the userspace.

What I mean is that we do not get to call PSCI CPU_OFF here so the CPUs 
are idle, but not power gated. Those CPUs do not have any other idle 
state other than WFI because the HW designers sort of forgot or rather 
did not know that wiring up the ARM GIC power controller back to the 
power gating logic of the CPU was a good idea.

> 
>> - s2idle requires the use of in-band interrupts for wake-up
>>
> 
> I am not sure if that is true. S2I behaves very similar to S2R except it
> has low wake latency as all secondaries CPUs are not hotplugged out.

OK, the fact that secondary CPUs are not hot-plugged could be remedied 
by doing this ahead of entering s2idle by user-space so this is not a 
valid argument from me anymore.

> 
>> The reasons for implementing "standby" are largely two fold:
>>
>> - we need to achieve decent power savings (typically below 0.5W for the
>> whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)
>>
> 
> I fail to understand how that is a problem from S2I. It is probably worth
> checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section
> IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other
> wise in terms of unnecessary/spurious wakeup by in-band(to be precise
> no-wake up) interrupts.

I don't think your hyperlink referenced by [1] was provided, but my 
quick testing with:

echo s2idle > /sys/power/mem_sleep
echo mem > /sys/power/state

appears to work to some extent when I use peripherals that can generate 
in-band interrupts.

It looks like we have s2idle_ops that allows a platform to override some 
of the operations before/after entering s2idle, however the actual 
s2idle idle loop is still within the kernel, so we will not call into 
the ARM Trusted Firmware and engage the power management state machine.

This means that there will not be any of the clock gating that only the 
hardware state machine is capable of performing, the DRAM controller as 
a result will not enter self refresh power down, and in addition the 
side band wake-up interrupts will not be activate because the interrupt 
controller that aggregates them only outputs to the ARM GIC when the 
state machine has been engaged.

Essentially, what we need for our systems is a controlled system entry 
with semantics similar if not identical to that of S2R but with a 
shallower state that does not cut the power to 90% of the SoC (unlike 
S2R) such that we have a quicker suspend and resume latency. Years ago 
when we only had MIPS-based and 32-bit ARM SoCs, we did come up with 
using "standby" (see drivers/soc/bcm/brcmstb/pm/*) and we naturally 
mapped that when we switched over to ARMv8 capable devices.

> 
>> - we have a security subsystem that requires the CPUs to be either power
>> gated or idle in order the hardware state machine that lets the system enter
>> such a state and allows the out of band interrupts from being wake-up
>> sources
>>
> 
> It should work unless I have completely misunderstood how S2I works.
> 
>>> Suspend to idle takes all the CPUs to lowest possible power state instead
>>> of cpu-hotplug in S2R. Also I assume some userspace has to identify when
>>> to enter "standby" vs "mem" right ? I am trying to see how addition of
>>> "idle" changes that(if it does). Sorry for too many questions.
>>>
>>
>> Right that user-space in our case is either custom (like RDK, or completely
>> custom), or is Android. For Android it looks like we are carrying a patch
>> that makes "mem" de-generate into "standby" but this is largely because we
>> had historically problems with "mem" that are being addressed (completely
>> orthogonal).
>>
> 
> Thanks for the info.
> 
>> I did not consider it as a viable option at the time, but if we were to
>> implement "standby" in drivers/firmware/psci/psci.c would that be somewhat
>> acceptable?
>>
> 
> We have been pointing anyone needing standby so far to S2I and so far no one
> has shouted that it doesn't suffice. Let me know what is missing.
> 

-- 
Florian

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

On 2/3/2022 10:52 AM, Sudeep Holla wrote:
> Correction: it is known as "freeze" rather than "idle" in terms of values
> as per /sys/power/state. Sorry for referring it as "idle" and creating any
> confusion.
> 
> On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/3/2022 3:14 AM, Sudeep Holla wrote:
>>> On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
>>>> Hi all,
>>>>
>>>> This patch series contains the Broadcom STB PSCI extensions which adds
>>>> some additional functions on top of the existing standard PSCI interface
>>>> which is the reason for having the driver implement a custom
>>>> suspend_ops.
>>>>
>>>> These platforms have traditionally supported a mode that is akin to
>>>> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
>>>> which is entered with "echo standby > /sys/power/state". Additional a
>>>> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
>>>> mem > /sys/power/state".
>>>
>>> How different is the above "standby" state compare to the standard "idle"
>>> (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
>>
>> There are a few differences:
>>
>> - s2idle does not power gate the secondary CPUs
>>
> 
> Not sure what you mean by that ? S2I takes CPUs to deepest idle state.
> If you want shallower states, one possible option is the disable deeper
> states from the userspace.

What I mean is that we do not get to call PSCI CPU_OFF here so the CPUs 
are idle, but not power gated. Those CPUs do not have any other idle 
state other than WFI because the HW designers sort of forgot or rather 
did not know that wiring up the ARM GIC power controller back to the 
power gating logic of the CPU was a good idea.

> 
>> - s2idle requires the use of in-band interrupts for wake-up
>>
> 
> I am not sure if that is true. S2I behaves very similar to S2R except it
> has low wake latency as all secondaries CPUs are not hotplugged out.

OK, the fact that secondary CPUs are not hot-plugged could be remedied 
by doing this ahead of entering s2idle by user-space so this is not a 
valid argument from me anymore.

> 
>> The reasons for implementing "standby" are largely two fold:
>>
>> - we need to achieve decent power savings (typically below 0.5W for the
>> whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)
>>
> 
> I fail to understand how that is a problem from S2I. It is probably worth
> checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section
> IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other
> wise in terms of unnecessary/spurious wakeup by in-band(to be precise
> no-wake up) interrupts.

I don't think your hyperlink referenced by [1] was provided, but my 
quick testing with:

echo s2idle > /sys/power/mem_sleep
echo mem > /sys/power/state

appears to work to some extent when I use peripherals that can generate 
in-band interrupts.

It looks like we have s2idle_ops that allows a platform to override some 
of the operations before/after entering s2idle, however the actual 
s2idle idle loop is still within the kernel, so we will not call into 
the ARM Trusted Firmware and engage the power management state machine.

This means that there will not be any of the clock gating that only the 
hardware state machine is capable of performing, the DRAM controller as 
a result will not enter self refresh power down, and in addition the 
side band wake-up interrupts will not be activate because the interrupt 
controller that aggregates them only outputs to the ARM GIC when the 
state machine has been engaged.

Essentially, what we need for our systems is a controlled system entry 
with semantics similar if not identical to that of S2R but with a 
shallower state that does not cut the power to 90% of the SoC (unlike 
S2R) such that we have a quicker suspend and resume latency. Years ago 
when we only had MIPS-based and 32-bit ARM SoCs, we did come up with 
using "standby" (see drivers/soc/bcm/brcmstb/pm/*) and we naturally 
mapped that when we switched over to ARMv8 capable devices.

> 
>> - we have a security subsystem that requires the CPUs to be either power
>> gated or idle in order the hardware state machine that lets the system enter
>> such a state and allows the out of band interrupts from being wake-up
>> sources
>>
> 
> It should work unless I have completely misunderstood how S2I works.
> 
>>> Suspend to idle takes all the CPUs to lowest possible power state instead
>>> of cpu-hotplug in S2R. Also I assume some userspace has to identify when
>>> to enter "standby" vs "mem" right ? I am trying to see how addition of
>>> "idle" changes that(if it does). Sorry for too many questions.
>>>
>>
>> Right that user-space in our case is either custom (like RDK, or completely
>> custom), or is Android. For Android it looks like we are carrying a patch
>> that makes "mem" de-generate into "standby" but this is largely because we
>> had historically problems with "mem" that are being addressed (completely
>> orthogonal).
>>
> 
> Thanks for the info.
> 
>> I did not consider it as a viable option at the time, but if we were to
>> implement "standby" in drivers/firmware/psci/psci.c would that be somewhat
>> acceptable?
>>
> 
> We have been pointing anyone needing standby so far to S2I and so far no one
> has shouted that it doesn't suffice. Let me know what is missing.
> 

-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Sudeep Holla]

On Thu, Feb 03, 2022 at 11:33:26AM -0800, Florian Fainelli wrote:
>
>
> On 2/3/2022 10:52 AM, Sudeep Holla wrote:
> > Correction: it is known as "freeze" rather than "idle" in terms of values
> > as per /sys/power/state. Sorry for referring it as "idle" and creating any
> > confusion.
> >
> > On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote:
> > >
> > >
> > > On 2/3/2022 3:14 AM, Sudeep Holla wrote:
> > > > On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
> > > > > Hi all,
> > > > >
> > > > > This patch series contains the Broadcom STB PSCI extensions which adds
> > > > > some additional functions on top of the existing standard PSCI interface
> > > > > which is the reason for having the driver implement a custom
> > > > > suspend_ops.
> > > > >
> > > > > These platforms have traditionally supported a mode that is akin to
> > > > > ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> > > > > which is entered with "echo standby > /sys/power/state". Additional a
> > > > > true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> > > > > mem > /sys/power/state".
> > > >
> > > > How different is the above "standby" state compare to the standard "idle"
> > > > (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
> > >
> > > There are a few differences:
> > >
> > > - s2idle does not power gate the secondary CPUs
> > >
> >
> > Not sure what you mean by that ? S2I takes CPUs to deepest idle state.
> > If you want shallower states, one possible option is the disable deeper
> > states from the userspace.
>
> What I mean is that we do not get to call PSCI CPU_OFF here so the CPUs are
> idle, but not power gated. Those CPUs do not have any other idle state other
> than WFI because the HW designers sort of forgot or rather did not know that
> wiring up the ARM GIC power controller back to the power gating logic of the
> CPU was a good idea.
>

Nice 😄

> >
> > > - s2idle requires the use of in-band interrupts for wake-up
> > >
> >
> > I am not sure if that is true. S2I behaves very similar to S2R except it
> > has low wake latency as all secondaries CPUs are not hotplugged out.
>
> OK, the fact that secondary CPUs are not hot-plugged could be remedied by
> doing this ahead of entering s2idle by user-space so this is not a valid
> argument from me anymore.
>

Fair enough.

> >
> > > The reasons for implementing "standby" are largely two fold:
> > >
> > > - we need to achieve decent power savings (typically below 0.5W for the
> > > whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)
> > >
> >
> > I fail to understand how that is a problem from S2I. It is probably worth
> > checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section
> > IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other
> > wise in terms of unnecessary/spurious wakeup by in-band(to be precise
> > no-wake up) interrupts.
>
> I don't think your hyperlink referenced by [1] was provided, but my quick
> testing with:
>

Yikes, I meant to refer Documentation/power/suspend-and-interrupts.rst

> echo s2idle > /sys/power/mem_sleep
> echo mem > /sys/power/state
>
> appears to work to some extent when I use peripherals that can generate
> in-band interrupts.
>
> It looks like we have s2idle_ops that allows a platform to override some of
> the operations before/after entering s2idle, however the actual s2idle idle
> loop is still within the kernel, so we will not call into the ARM Trusted
> Firmware and engage the power management state machine.
>

Correct.

> This means that there will not be any of the clock gating that only the
> hardware state machine is capable of performing, the DRAM controller as a
> result will not enter self refresh power down, and in addition the side band
> wake-up interrupts will not be activate because the interrupt controller
> that aggregates them only outputs to the ARM GIC when the state machine has
> been engaged.
>

One possible solution IIUC the issue is to add this as additional CPU Idle
state disabled most of the time. Enable them from user-space just prior to
calling freeze/s2idle, so that PSCI CPU_SUSPEND is called with right param
to indicate this is deepest idle state(in your case just WFI) + DRAM self
refresh/retention mode. Also TF-A can take care to enable the side band
interrupts before entering the state.

Do you see any issue with this approach ? I am trying to find ways to avoid
deviating from standard PSCI.

> Essentially, what we need for our systems is a controlled system entry with
> semantics similar if not identical to that of S2R but with a shallower state
> that does not cut the power to 90% of the SoC (unlike S2R) such that we have
> a quicker suspend and resume latency. Years ago when we only had MIPS-based
> and 32-bit ARM SoCs, we did come up with using "standby" (see
> drivers/soc/bcm/brcmstb/pm/*) and we naturally mapped that when we switched
> over to ARMv8 capable devices.
>

Understood and I do remember that.

--
Regards,
Sudeep

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Sudeep Holla]

On Thu, Feb 03, 2022 at 11:33:26AM -0800, Florian Fainelli wrote:
>
>
> On 2/3/2022 10:52 AM, Sudeep Holla wrote:
> > Correction: it is known as "freeze" rather than "idle" in terms of values
> > as per /sys/power/state. Sorry for referring it as "idle" and creating any
> > confusion.
> >
> > On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote:
> > >
> > >
> > > On 2/3/2022 3:14 AM, Sudeep Holla wrote:
> > > > On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
> > > > > Hi all,
> > > > >
> > > > > This patch series contains the Broadcom STB PSCI extensions which adds
> > > > > some additional functions on top of the existing standard PSCI interface
> > > > > which is the reason for having the driver implement a custom
> > > > > suspend_ops.
> > > > >
> > > > > These platforms have traditionally supported a mode that is akin to
> > > > > ACPI's S2 with the CPU in WFI and all of the chip being clock gated
> > > > > which is entered with "echo standby > /sys/power/state". Additional a
> > > > > true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
> > > > > mem > /sys/power/state".
> > > >
> > > > How different is the above "standby" state compare to the standard "idle"
> > > > (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
> > >
> > > There are a few differences:
> > >
> > > - s2idle does not power gate the secondary CPUs
> > >
> >
> > Not sure what you mean by that ? S2I takes CPUs to deepest idle state.
> > If you want shallower states, one possible option is the disable deeper
> > states from the userspace.
>
> What I mean is that we do not get to call PSCI CPU_OFF here so the CPUs are
> idle, but not power gated. Those CPUs do not have any other idle state other
> than WFI because the HW designers sort of forgot or rather did not know that
> wiring up the ARM GIC power controller back to the power gating logic of the
> CPU was a good idea.
>

Nice 😄

> >
> > > - s2idle requires the use of in-band interrupts for wake-up
> > >
> >
> > I am not sure if that is true. S2I behaves very similar to S2R except it
> > has low wake latency as all secondaries CPUs are not hotplugged out.
>
> OK, the fact that secondary CPUs are not hot-plugged could be remedied by
> doing this ahead of entering s2idle by user-space so this is not a valid
> argument from me anymore.
>

Fair enough.

> >
> > > The reasons for implementing "standby" are largely two fold:
> > >
> > > - we need to achieve decent power savings (typically below 0.5W for the
> > > whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)
> > >
> >
> > I fail to understand how that is a problem from S2I. It is probably worth
> > checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section
> > IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other
> > wise in terms of unnecessary/spurious wakeup by in-band(to be precise
> > no-wake up) interrupts.
>
> I don't think your hyperlink referenced by [1] was provided, but my quick
> testing with:
>

Yikes, I meant to refer Documentation/power/suspend-and-interrupts.rst

> echo s2idle > /sys/power/mem_sleep
> echo mem > /sys/power/state
>
> appears to work to some extent when I use peripherals that can generate
> in-band interrupts.
>
> It looks like we have s2idle_ops that allows a platform to override some of
> the operations before/after entering s2idle, however the actual s2idle idle
> loop is still within the kernel, so we will not call into the ARM Trusted
> Firmware and engage the power management state machine.
>

Correct.

> This means that there will not be any of the clock gating that only the
> hardware state machine is capable of performing, the DRAM controller as a
> result will not enter self refresh power down, and in addition the side band
> wake-up interrupts will not be activate because the interrupt controller
> that aggregates them only outputs to the ARM GIC when the state machine has
> been engaged.
>

One possible solution IIUC the issue is to add this as additional CPU Idle
state disabled most of the time. Enable them from user-space just prior to
calling freeze/s2idle, so that PSCI CPU_SUSPEND is called with right param
to indicate this is deepest idle state(in your case just WFI) + DRAM self
refresh/retention mode. Also TF-A can take care to enable the side band
interrupts before entering the state.

Do you see any issue with this approach ? I am trying to find ways to avoid
deviating from standard PSCI.

> Essentially, what we need for our systems is a controlled system entry with
> semantics similar if not identical to that of S2R but with a shallower state
> that does not cut the power to 90% of the SoC (unlike S2R) such that we have
> a quicker suspend and resume latency. Years ago when we only had MIPS-based
> and 32-bit ARM SoCs, we did come up with using "standby" (see
> drivers/soc/bcm/brcmstb/pm/*) and we naturally mapped that when we switched
> over to ARMv8 capable devices.
>

Understood and I do remember that.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

On 2/7/22 8:27 AM, Sudeep Holla wrote:
> On Thu, Feb 03, 2022 at 11:33:26AM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/3/2022 10:52 AM, Sudeep Holla wrote:
>>> Correction: it is known as "freeze" rather than "idle" in terms of values
>>> as per /sys/power/state. Sorry for referring it as "idle" and creating any
>>> confusion.
>>>
>>> On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 2/3/2022 3:14 AM, Sudeep Holla wrote:
>>>>> On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This patch series contains the Broadcom STB PSCI extensions which adds
>>>>>> some additional functions on top of the existing standard PSCI interface
>>>>>> which is the reason for having the driver implement a custom
>>>>>> suspend_ops.
>>>>>>
>>>>>> These platforms have traditionally supported a mode that is akin to
>>>>>> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
>>>>>> which is entered with "echo standby > /sys/power/state". Additional a
>>>>>> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
>>>>>> mem > /sys/power/state".
>>>>>
>>>>> How different is the above "standby" state compare to the standard "idle"
>>>>> (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
>>>>
>>>> There are a few differences:
>>>>
>>>> - s2idle does not power gate the secondary CPUs
>>>>
>>>
>>> Not sure what you mean by that ? S2I takes CPUs to deepest idle state.
>>> If you want shallower states, one possible option is the disable deeper
>>> states from the userspace.
>>
>> What I mean is that we do not get to call PSCI CPU_OFF here so the CPUs are
>> idle, but not power gated. Those CPUs do not have any other idle state other
>> than WFI because the HW designers sort of forgot or rather did not know that
>> wiring up the ARM GIC power controller back to the power gating logic of the
>> CPU was a good idea.
>>
> 
> Nice 😄
> 
>>>
>>>> - s2idle requires the use of in-band interrupts for wake-up
>>>>
>>>
>>> I am not sure if that is true. S2I behaves very similar to S2R except it
>>> has low wake latency as all secondaries CPUs are not hotplugged out.
>>
>> OK, the fact that secondary CPUs are not hot-plugged could be remedied by
>> doing this ahead of entering s2idle by user-space so this is not a valid
>> argument from me anymore.
>>
> 
> Fair enough.
> 
>>>
>>>> The reasons for implementing "standby" are largely two fold:
>>>>
>>>> - we need to achieve decent power savings (typically below 0.5W for the
>>>> whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)
>>>>
>>>
>>> I fail to understand how that is a problem from S2I. It is probably worth
>>> checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section
>>> IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other
>>> wise in terms of unnecessary/spurious wakeup by in-band(to be precise
>>> no-wake up) interrupts.
>>
>> I don't think your hyperlink referenced by [1] was provided, but my quick
>> testing with:
>>
> 
> Yikes, I meant to refer Documentation/power/suspend-and-interrupts.rst
> 
>> echo s2idle > /sys/power/mem_sleep
>> echo mem > /sys/power/state
>>
>> appears to work to some extent when I use peripherals that can generate
>> in-band interrupts.
>>
>> It looks like we have s2idle_ops that allows a platform to override some of
>> the operations before/after entering s2idle, however the actual s2idle idle
>> loop is still within the kernel, so we will not call into the ARM Trusted
>> Firmware and engage the power management state machine.
>>
> 
> Correct.
> 
>> This means that there will not be any of the clock gating that only the
>> hardware state machine is capable of performing, the DRAM controller as a
>> result will not enter self refresh power down, and in addition the side band
>> wake-up interrupts will not be activate because the interrupt controller
>> that aggregates them only outputs to the ARM GIC when the state machine has
>> been engaged.
>>
> 
> One possible solution IIUC the issue is to add this as additional CPU Idle
> state disabled most of the time. Enable them from user-space just prior to
> calling freeze/s2idle, so that PSCI CPU_SUSPEND is called with right param
> to indicate this is deepest idle state(in your case just WFI) + DRAM self
> refresh/retention mode. Also TF-A can take care to enable the side band
> interrupts before entering the state.

Not knowing how to enable a disabled idle state from user-space, and
ensure that it does not race with cpuidle somehow choosing to enter that
state, I have all sorts of concerns about such interactions but can see
how this could be made to work. In fact, I am wondering if we had not
better off work around our broken HW and always advertise that state,
and just let cpuidle pick that "deep" idle state resulting in powering
down secondary core(s). In TF-A we would have to ensure that we save all
of the SPIs affine to that particular core, and probably re-configure
the PPIs and SGIs to be made secure such that TF-A can "trap" them and
wake-up the core that was just powered off.

It sounds like for your suggested approach plus requesting to enter
s2idle, we need to start trapping WFI at ELx into EL3 such that TF-A has
a chance of observing that all CPU cores are powered down and/or idle
and then can engage our power management state machine hardware to clock
gate the system and mux in the out of band interrupts. Unless the boot
CPU's default idle state has to either be modified, or we have to
advertise an additional "deeper" idle state that involves calling into
TF-A with PSCI CPU_SUSPEND, too?

What I really like about our approach other than it has been proven to
work over the past 10 years, is that it fits well with Linux system
suspend path via suspend_ops, with each layer taking care of defining
its points of no return etc. We know how to debug it, and it is not
opportunistic unlike cpuidle, which makes it easier to control. From a
non technical point of view, it is also the devil people are used to,
and no matter how we shape it, there will be resistance to change.

> 
> Do you see any issue with this approach ? I am trying to find ways to avoid
> deviating from standard PSCI.

Re-reading the PSCI specification and the SYSTEM_SUSPEND specifically it
does sound like ACPI S2 was taken into account but that lead to
SYSTEM_SUSPEND not allowing to differentiate between these two states on
the premise that "In practice, operating systems use only one suspend to
RAM state, so this is not seen as a limitation". It would have been nice
to leave provision for defining both instead of not.

I am sympathetic to avoid divergence of both interpretation and
implementation of the PSCI specification and its reference
implementation in Linux. So I can see a few paths forward:

- have me try what you suggested, which will take me weeks because I
have a TODO list long like my arm (and my arms are really long),
assuming that this even works for our use cases

- work with you to amend the PSCI specification such that we can
differentiate between an entry into S2 or S3 by defining which state we
want to enter, but I assume we will get lots of resistance here?
-- 
Florian

Re: [PATCH 0/4] Broadcom STB PM PSCI extensions

[Florian Fainelli]

On 2/7/22 8:27 AM, Sudeep Holla wrote:
> On Thu, Feb 03, 2022 at 11:33:26AM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/3/2022 10:52 AM, Sudeep Holla wrote:
>>> Correction: it is known as "freeze" rather than "idle" in terms of values
>>> as per /sys/power/state. Sorry for referring it as "idle" and creating any
>>> confusion.
>>>
>>> On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 2/3/2022 3:14 AM, Sudeep Holla wrote:
>>>>> On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This patch series contains the Broadcom STB PSCI extensions which adds
>>>>>> some additional functions on top of the existing standard PSCI interface
>>>>>> which is the reason for having the driver implement a custom
>>>>>> suspend_ops.
>>>>>>
>>>>>> These platforms have traditionally supported a mode that is akin to
>>>>>> ACPI's S2 with the CPU in WFI and all of the chip being clock gated
>>>>>> which is entered with "echo standby > /sys/power/state". Additional a
>>>>>> true suspend to DRAM as defined in ACPI by S3 is implemented with "echo
>>>>>> mem > /sys/power/state".
>>>>>
>>>>> How different is the above "standby" state compare to the standard "idle"
>>>>> (a.k.a suspend-to-idle which is different from system-to-ram/S3) ?
>>>>
>>>> There are a few differences:
>>>>
>>>> - s2idle does not power gate the secondary CPUs
>>>>
>>>
>>> Not sure what you mean by that ? S2I takes CPUs to deepest idle state.
>>> If you want shallower states, one possible option is the disable deeper
>>> states from the userspace.
>>
>> What I mean is that we do not get to call PSCI CPU_OFF here so the CPUs are
>> idle, but not power gated. Those CPUs do not have any other idle state other
>> than WFI because the HW designers sort of forgot or rather did not know that
>> wiring up the ARM GIC power controller back to the power gating logic of the
>> CPU was a good idea.
>>
> 
> Nice 😄
> 
>>>
>>>> - s2idle requires the use of in-band interrupts for wake-up
>>>>
>>>
>>> I am not sure if that is true. S2I behaves very similar to S2R except it
>>> has low wake latency as all secondaries CPUs are not hotplugged out.
>>
>> OK, the fact that secondary CPUs are not hot-plugged could be remedied by
>> doing this ahead of entering s2idle by user-space so this is not a valid
>> argument from me anymore.
>>
> 
> Fair enough.
> 
>>>
>>>> The reasons for implementing "standby" are largely two fold:
>>>>
>>>> - we need to achieve decent power savings (typically below 0.5W for the
>>>> whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.)
>>>>
>>>
>>> I fail to understand how that is a problem from S2I. It is probably worth
>>> checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section
>>> IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other
>>> wise in terms of unnecessary/spurious wakeup by in-band(to be precise
>>> no-wake up) interrupts.
>>
>> I don't think your hyperlink referenced by [1] was provided, but my quick
>> testing with:
>>
> 
> Yikes, I meant to refer Documentation/power/suspend-and-interrupts.rst
> 
>> echo s2idle > /sys/power/mem_sleep
>> echo mem > /sys/power/state
>>
>> appears to work to some extent when I use peripherals that can generate
>> in-band interrupts.
>>
>> It looks like we have s2idle_ops that allows a platform to override some of
>> the operations before/after entering s2idle, however the actual s2idle idle
>> loop is still within the kernel, so we will not call into the ARM Trusted
>> Firmware and engage the power management state machine.
>>
> 
> Correct.
> 
>> This means that there will not be any of the clock gating that only the
>> hardware state machine is capable of performing, the DRAM controller as a
>> result will not enter self refresh power down, and in addition the side band
>> wake-up interrupts will not be activate because the interrupt controller
>> that aggregates them only outputs to the ARM GIC when the state machine has
>> been engaged.
>>
> 
> One possible solution IIUC the issue is to add this as additional CPU Idle
> state disabled most of the time. Enable them from user-space just prior to
> calling freeze/s2idle, so that PSCI CPU_SUSPEND is called with right param
> to indicate this is deepest idle state(in your case just WFI) + DRAM self
> refresh/retention mode. Also TF-A can take care to enable the side band
> interrupts before entering the state.

Not knowing how to enable a disabled idle state from user-space, and
ensure that it does not race with cpuidle somehow choosing to enter that
state, I have all sorts of concerns about such interactions but can see
how this could be made to work. In fact, I am wondering if we had not
better off work around our broken HW and always advertise that state,
and just let cpuidle pick that "deep" idle state resulting in powering
down secondary core(s). In TF-A we would have to ensure that we save all
of the SPIs affine to that particular core, and probably re-configure
the PPIs and SGIs to be made secure such that TF-A can "trap" them and
wake-up the core that was just powered off.

It sounds like for your suggested approach plus requesting to enter
s2idle, we need to start trapping WFI at ELx into EL3 such that TF-A has
a chance of observing that all CPU cores are powered down and/or idle
and then can engage our power management state machine hardware to clock
gate the system and mux in the out of band interrupts. Unless the boot
CPU's default idle state has to either be modified, or we have to
advertise an additional "deeper" idle state that involves calling into
TF-A with PSCI CPU_SUSPEND, too?

What I really like about our approach other than it has been proven to
work over the past 10 years, is that it fits well with Linux system
suspend path via suspend_ops, with each layer taking care of defining
its points of no return etc. We know how to debug it, and it is not
opportunistic unlike cpuidle, which makes it easier to control. From a
non technical point of view, it is also the devil people are used to,
and no matter how we shape it, there will be resistance to change.

> 
> Do you see any issue with this approach ? I am trying to find ways to avoid
> deviating from standard PSCI.

Re-reading the PSCI specification and the SYSTEM_SUSPEND specifically it
does sound like ACPI S2 was taken into account but that lead to
SYSTEM_SUSPEND not allowing to differentiate between these two states on
the premise that "In practice, operating systems use only one suspend to
RAM state, so this is not seen as a limitation". It would have been nice
to leave provision for defining both instead of not.

I am sympathetic to avoid divergence of both interpretation and
implementation of the PSCI specification and its reference
implementation in Linux. So I can see a few paths forward:

- have me try what you suggested, which will take me weeks because I
have a TODO list long like my arm (and my arms are really long),
assuming that this even works for our use cases

- work with you to amend the PSCI specification such that we can
differentiate between an entry into S2 or S3 by defining which state we
want to enter, but I assume we will get lots of resistance here?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel