[PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephen Boyd]

These scm calls are never used outside of legacy ARMv7 based platforms.
That's because PSCI, mandated on arm64, implements them for modern SoCs
via the PSCI spec. Let's move them to the legacy file and only compile
the legacy file into the kernel when CONFIG_ARM=y. Otherwise provide
stubs and fail the calls. This saves a little bit of space in an
arm64 allmodconfig.

 $ ./scripts/bloat-o-meter vmlinux.before vmlinux.after
 add/remove: 0/8 grow/shrink: 5/6 up/down: 509/-4401 (-3892)
 Function                                     old     new   delta
 __qcom_scm_set_dload_mode.constprop          312     452    +140
 qcom_scm_qsmmu500_wait_safe_toggle           288     416    +128
 qcom_scm_io_writel                           288     408    +120
 qcom_scm_io_readl                            376     492    +116
 __param_str_download_mode                     23      28      +5
 __warned                                    4327    4326      -1
 e843419@0b3f_00010432_324                      8       -      -8
 qcom_scm_call                                228     208     -20
 CSWTCH                                      5925    5877     -48
 _sub_I_65535_1                            163100  163040     -60
 _sub_D_65535_0                            163100  163040     -60
 qcom_scm_wb                                   64       -     -64
 qcom_scm_lock                                320     160    -160
 qcom_scm_call_atomic                         212       -    -212
 qcom_scm_cpu_power_down                      308       -    -308
 scm_legacy_call_atomic                       520       -    -520
 qcom_scm_set_warm_boot_addr                  720       -    -720
 qcom_scm_set_cold_boot_addr                  728       -    -728
 scm_legacy_call                             1492       -   -1492
 Total: Before=66737606, After=66733714, chg -0.01%

Commit 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and
legacy conventions") didn't mention any motivating factors for keeping
the legacy code around on arm64 kernels, i.e. presumably that commit
wasn't trying to support these legacy APIs on arm64 kernels.

Cc: Elliot Berman <eberman@codeaurora.org>
Cc: Brian Masney <masneyb@onstation.org>
Cc: Stephan Gerhold <stephan@gerhold.net>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Followup to v1 (https://lore.kernel.org/r/20210223214539.1336155-7-swboyd@chromium.org):
 * Don't change the legacy file to use legacy calls only
 * Wrap more things in CONFIG_ARM checks

 drivers/firmware/Makefile   |  4 +++-
 drivers/firmware/qcom_scm.c | 47 ++++++++++++++++++++-----------------
 drivers/firmware/qcom_scm.h | 15 ++++++++++++
 include/linux/qcom_scm.h    | 21 ++++++++++-------
 4 files changed, 56 insertions(+), 31 deletions(-)

diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 5e013b6a3692..0b7b35555a6c 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -17,7 +17,9 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
-obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+obj-$(CONFIG_QCOM_SCM)		+= qcom_scm_objs.o
+qcom_scm_objs-$(CONFIG_ARM)	+= qcom_scm-legacy.o
+qcom_scm_objs-$(CONFIG_QCOM_SCM) += qcom_scm.o qcom_scm-smc.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index ee9cb545e73b..747808a8ddf4 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -49,28 +49,6 @@ struct qcom_scm_mem_map_info {
 	__le64 mem_size;
 };
 
-#define QCOM_SCM_FLAG_COLDBOOT_CPU0	0x00
-#define QCOM_SCM_FLAG_COLDBOOT_CPU1	0x01
-#define QCOM_SCM_FLAG_COLDBOOT_CPU2	0x08
-#define QCOM_SCM_FLAG_COLDBOOT_CPU3	0x20
-
-#define QCOM_SCM_FLAG_WARMBOOT_CPU0	0x04
-#define QCOM_SCM_FLAG_WARMBOOT_CPU1	0x02
-#define QCOM_SCM_FLAG_WARMBOOT_CPU2	0x10
-#define QCOM_SCM_FLAG_WARMBOOT_CPU3	0x40
-
-struct qcom_scm_wb_entry {
-	int flag;
-	void *entry;
-};
-
-static struct qcom_scm_wb_entry qcom_scm_wb[] = {
-	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU0 },
-	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU1 },
-	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU2 },
-	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU3 },
-};
-
 static const char *qcom_scm_convention_names[] = {
 	[SMC_CONVENTION_UNKNOWN] = "unknown",
 	[SMC_CONVENTION_ARM_32] = "smc arm 32",
@@ -260,6 +238,30 @@ static bool __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
 	return ret ? false : !!res.result[0];
 }
 
+#if IS_ENABLED(CONFIG_ARM)
+
+#define QCOM_SCM_FLAG_COLDBOOT_CPU0	0x00
+#define QCOM_SCM_FLAG_COLDBOOT_CPU1	0x01
+#define QCOM_SCM_FLAG_COLDBOOT_CPU2	0x08
+#define QCOM_SCM_FLAG_COLDBOOT_CPU3	0x20
+
+#define QCOM_SCM_FLAG_WARMBOOT_CPU0	0x04
+#define QCOM_SCM_FLAG_WARMBOOT_CPU1	0x02
+#define QCOM_SCM_FLAG_WARMBOOT_CPU2	0x10
+#define QCOM_SCM_FLAG_WARMBOOT_CPU3	0x40
+
+struct qcom_scm_wb_entry {
+	int flag;
+	void *entry;
+};
+
+static struct qcom_scm_wb_entry qcom_scm_wb[] = {
+	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU0 },
+	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU1 },
+	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU2 },
+	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU3 },
+};
+
 /**
  * qcom_scm_set_warm_boot_addr() - Set the warm boot address for cpus
  * @entry: Entry point function for the cpus
@@ -369,6 +371,7 @@ void qcom_scm_cpu_power_down(u32 flags)
 	qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL(qcom_scm_cpu_power_down);
+#endif
 
 int qcom_scm_set_remote_state(u32 state, u32 id)
 {
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 632fe3142462..735e975320e4 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -68,11 +68,26 @@ extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 	__scm_smc_call((dev), (desc), qcom_scm_convention, (res), (atomic))
 
 #define SCM_LEGACY_FNID(s, c)	(((s) << 10) | ((c) & 0x3ff))
+#if IS_ENABLED(CONFIG_ARM)
 extern int scm_legacy_call_atomic(struct device *dev,
 				  const struct qcom_scm_desc *desc,
 				  struct qcom_scm_res *res);
 extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 			   struct qcom_scm_res *res);
+#else
+static inline int scm_legacy_call_atomic(struct device *dev,
+					 const struct qcom_scm_desc *desc,
+					 struct qcom_scm_res *res)
+{
+	return -EINVAL;
+}
+
+static inline int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
+				  struct qcom_scm_res *res)
+{
+	return -EINVAL;
+}
+#endif
 
 #define QCOM_SCM_SVC_BOOT		0x01
 #define QCOM_SCM_BOOT_SET_ADDR		0x01
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index 0165824c5128..0ec905d56e1a 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -64,9 +64,6 @@ enum qcom_scm_ice_cipher {
 #if IS_ENABLED(CONFIG_QCOM_SCM)
 extern bool qcom_scm_is_available(void);
 
-extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
-extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
-extern void qcom_scm_cpu_power_down(u32 flags);
 extern int qcom_scm_set_remote_state(u32 state, u32 id);
 
 extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
@@ -115,11 +112,6 @@ extern int qcom_scm_qsmmu500_wait_safe_toggle(bool en);
 
 static inline bool qcom_scm_is_available(void) { return false; }
 
-static inline int qcom_scm_set_cold_boot_addr(void *entry,
-		const cpumask_t *cpus) { return -ENODEV; }
-static inline int qcom_scm_set_warm_boot_addr(void *entry,
-		const cpumask_t *cpus) { return -ENODEV; }
-static inline void qcom_scm_cpu_power_down(u32 flags) {}
 static inline u32 qcom_scm_set_remote_state(u32 state,u32 id)
 		{ return -ENODEV; }
 
@@ -171,4 +163,17 @@ static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 static inline int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
 		{ return -ENODEV; }
 #endif
+
+#if IS_ENABLED(CONFIG_ARM) && IS_ENABLED(CONFIG_QCOM_SCM)
+extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
+extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
+extern void qcom_scm_cpu_power_down(u32 flags);
+#else
+static inline int qcom_scm_set_cold_boot_addr(void *entry,
+		const cpumask_t *cpus) { return -ENODEV; }
+static inline int qcom_scm_set_warm_boot_addr(void *entry,
+		const cpumask_t *cpus) { return -ENODEV; }
+static inline void qcom_scm_cpu_power_down(u32 flags) {}
+#endif
+
 #endif

base-commit: 3b9cdafb5358eb9f3790de2f728f765fef100731
prerequisite-patch-id: 77da2cfd7591b1d7c35e879dca67d4f037f40e48
prerequisite-patch-id: 021337034973fa8ce52fc8c84787f40dabb33df6
prerequisite-patch-id: 5d374e97d8f0d384098a46e91006811ab89c84b0
prerequisite-patch-id: 892de894cc937f7fe6ddb8f95ec9e2e3557830c7
prerequisite-patch-id: 33b2442181aeb8adfa1c08d9a167d3bcbd1660fe
-- 
https://chromeos.dev

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephen Boyd]

Quoting Stephen Boyd (2021-03-23 15:43:36)
> These scm calls are never used outside of legacy ARMv7 based platforms.
> That's because PSCI, mandated on arm64, implements them for modern SoCs
> via the PSCI spec. Let's move them to the legacy file and only compile
> the legacy file into the kernel when CONFIG_ARM=y. Otherwise provide
> stubs and fail the calls. This saves a little bit of space in an
> arm64 allmodconfig.
> 
>  $ ./scripts/bloat-o-meter vmlinux.before vmlinux.after
>  add/remove: 0/8 grow/shrink: 5/6 up/down: 509/-4401 (-3892)
>  Function                                     old     new   delta
>  __qcom_scm_set_dload_mode.constprop          312     452    +140
>  qcom_scm_qsmmu500_wait_safe_toggle           288     416    +128
>  qcom_scm_io_writel                           288     408    +120
>  qcom_scm_io_readl                            376     492    +116
>  __param_str_download_mode                     23      28      +5
>  __warned                                    4327    4326      -1
>  e843419@0b3f_00010432_324                      8       -      -8
>  qcom_scm_call                                228     208     -20
>  CSWTCH                                      5925    5877     -48
>  _sub_I_65535_1                            163100  163040     -60
>  _sub_D_65535_0                            163100  163040     -60
>  qcom_scm_wb                                   64       -     -64
>  qcom_scm_lock                                320     160    -160
>  qcom_scm_call_atomic                         212       -    -212
>  qcom_scm_cpu_power_down                      308       -    -308
>  scm_legacy_call_atomic                       520       -    -520
>  qcom_scm_set_warm_boot_addr                  720       -    -720
>  qcom_scm_set_cold_boot_addr                  728       -    -728
>  scm_legacy_call                             1492       -   -1492
>  Total: Before=66737606, After=66733714, chg -0.01%
> 
> Commit 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and
> legacy conventions") didn't mention any motivating factors for keeping
> the legacy code around on arm64 kernels, i.e. presumably that commit
> wasn't trying to support these legacy APIs on arm64 kernels.
> 
> Cc: Elliot Berman <eberman@codeaurora.org>
> Cc: Brian Masney <masneyb@onstation.org>
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 

Elliot, can you ack/review this?

> Followup to v1 (https://lore.kernel.org/r/20210223214539.1336155-7-swboyd@chromium.org):
>  * Don't change the legacy file to use legacy calls only
>  * Wrap more things in CONFIG_ARM checks
>

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Bjorn Andersson]

On Tue 23 Mar 17:43 CDT 2021, Stephen Boyd wrote:

> These scm calls are never used outside of legacy ARMv7 based platforms.
> That's because PSCI, mandated on arm64, implements them for modern SoCs
> via the PSCI spec. Let's move them to the legacy file and only compile
> the legacy file into the kernel when CONFIG_ARM=y. Otherwise provide
> stubs and fail the calls. This saves a little bit of space in an
> arm64 allmodconfig.
> 
>  $ ./scripts/bloat-o-meter vmlinux.before vmlinux.after
>  add/remove: 0/8 grow/shrink: 5/6 up/down: 509/-4401 (-3892)
>  Function                                     old     new   delta
>  __qcom_scm_set_dload_mode.constprop          312     452    +140
>  qcom_scm_qsmmu500_wait_safe_toggle           288     416    +128
>  qcom_scm_io_writel                           288     408    +120
>  qcom_scm_io_readl                            376     492    +116
>  __param_str_download_mode                     23      28      +5
>  __warned                                    4327    4326      -1
>  e843419@0b3f_00010432_324                      8       -      -8
>  qcom_scm_call                                228     208     -20
>  CSWTCH                                      5925    5877     -48
>  _sub_I_65535_1                            163100  163040     -60
>  _sub_D_65535_0                            163100  163040     -60
>  qcom_scm_wb                                   64       -     -64
>  qcom_scm_lock                                320     160    -160
>  qcom_scm_call_atomic                         212       -    -212
>  qcom_scm_cpu_power_down                      308       -    -308
>  scm_legacy_call_atomic                       520       -    -520
>  qcom_scm_set_warm_boot_addr                  720       -    -720
>  qcom_scm_set_cold_boot_addr                  728       -    -728
>  scm_legacy_call                             1492       -   -1492
>  Total: Before=66737606, After=66733714, chg -0.01%
> 
> Commit 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and
> legacy conventions") didn't mention any motivating factors for keeping
> the legacy code around on arm64 kernels, i.e. presumably that commit
> wasn't trying to support these legacy APIs on arm64 kernels.
> 
> Cc: Elliot Berman <eberman@codeaurora.org>
> Cc: Brian Masney <masneyb@onstation.org>
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Followup to v1 (https://lore.kernel.org/r/20210223214539.1336155-7-swboyd@chromium.org):
>  * Don't change the legacy file to use legacy calls only
>  * Wrap more things in CONFIG_ARM checks
> 
>  drivers/firmware/Makefile   |  4 +++-
>  drivers/firmware/qcom_scm.c | 47 ++++++++++++++++++++-----------------
>  drivers/firmware/qcom_scm.h | 15 ++++++++++++
>  include/linux/qcom_scm.h    | 21 ++++++++++-------
>  4 files changed, 56 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 5e013b6a3692..0b7b35555a6c 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -17,7 +17,9 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
>  obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
>  obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
>  obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
> -obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
> +obj-$(CONFIG_QCOM_SCM)		+= qcom_scm_objs.o
> +qcom_scm_objs-$(CONFIG_ARM)	+= qcom_scm-legacy.o
> +qcom_scm_objs-$(CONFIG_QCOM_SCM) += qcom_scm.o qcom_scm-smc.o
>  obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
>  obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
>  obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index ee9cb545e73b..747808a8ddf4 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -49,28 +49,6 @@ struct qcom_scm_mem_map_info {
>  	__le64 mem_size;
>  };
>  
> -#define QCOM_SCM_FLAG_COLDBOOT_CPU0	0x00
> -#define QCOM_SCM_FLAG_COLDBOOT_CPU1	0x01
> -#define QCOM_SCM_FLAG_COLDBOOT_CPU2	0x08
> -#define QCOM_SCM_FLAG_COLDBOOT_CPU3	0x20
> -
> -#define QCOM_SCM_FLAG_WARMBOOT_CPU0	0x04
> -#define QCOM_SCM_FLAG_WARMBOOT_CPU1	0x02
> -#define QCOM_SCM_FLAG_WARMBOOT_CPU2	0x10
> -#define QCOM_SCM_FLAG_WARMBOOT_CPU3	0x40
> -
> -struct qcom_scm_wb_entry {
> -	int flag;
> -	void *entry;
> -};
> -
> -static struct qcom_scm_wb_entry qcom_scm_wb[] = {
> -	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU0 },
> -	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU1 },
> -	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU2 },
> -	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU3 },
> -};
> -
>  static const char *qcom_scm_convention_names[] = {
>  	[SMC_CONVENTION_UNKNOWN] = "unknown",
>  	[SMC_CONVENTION_ARM_32] = "smc arm 32",
> @@ -260,6 +238,30 @@ static bool __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>  	return ret ? false : !!res.result[0];
>  }
>  
> +#if IS_ENABLED(CONFIG_ARM)
> +
> +#define QCOM_SCM_FLAG_COLDBOOT_CPU0	0x00
> +#define QCOM_SCM_FLAG_COLDBOOT_CPU1	0x01
> +#define QCOM_SCM_FLAG_COLDBOOT_CPU2	0x08
> +#define QCOM_SCM_FLAG_COLDBOOT_CPU3	0x20
> +
> +#define QCOM_SCM_FLAG_WARMBOOT_CPU0	0x04
> +#define QCOM_SCM_FLAG_WARMBOOT_CPU1	0x02
> +#define QCOM_SCM_FLAG_WARMBOOT_CPU2	0x10
> +#define QCOM_SCM_FLAG_WARMBOOT_CPU3	0x40
> +
> +struct qcom_scm_wb_entry {
> +	int flag;
> +	void *entry;
> +};
> +
> +static struct qcom_scm_wb_entry qcom_scm_wb[] = {
> +	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU0 },
> +	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU1 },
> +	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU2 },
> +	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU3 },
> +};
> +
>  /**
>   * qcom_scm_set_warm_boot_addr() - Set the warm boot address for cpus
>   * @entry: Entry point function for the cpus
> @@ -369,6 +371,7 @@ void qcom_scm_cpu_power_down(u32 flags)
>  	qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL(qcom_scm_cpu_power_down);
> +#endif
>  
>  int qcom_scm_set_remote_state(u32 state, u32 id)
>  {
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 632fe3142462..735e975320e4 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -68,11 +68,26 @@ extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  	__scm_smc_call((dev), (desc), qcom_scm_convention, (res), (atomic))
>  
>  #define SCM_LEGACY_FNID(s, c)	(((s) << 10) | ((c) & 0x3ff))
> +#if IS_ENABLED(CONFIG_ARM)
>  extern int scm_legacy_call_atomic(struct device *dev,
>  				  const struct qcom_scm_desc *desc,
>  				  struct qcom_scm_res *res);
>  extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>  			   struct qcom_scm_res *res);
> +#else
> +static inline int scm_legacy_call_atomic(struct device *dev,
> +					 const struct qcom_scm_desc *desc,
> +					 struct qcom_scm_res *res)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> +				  struct qcom_scm_res *res)
> +{
> +	return -EINVAL;
> +}
> +#endif
>  
>  #define QCOM_SCM_SVC_BOOT		0x01
>  #define QCOM_SCM_BOOT_SET_ADDR		0x01
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index 0165824c5128..0ec905d56e1a 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -64,9 +64,6 @@ enum qcom_scm_ice_cipher {
>  #if IS_ENABLED(CONFIG_QCOM_SCM)
>  extern bool qcom_scm_is_available(void);
>  
> -extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
> -extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
> -extern void qcom_scm_cpu_power_down(u32 flags);
>  extern int qcom_scm_set_remote_state(u32 state, u32 id);
>  
>  extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
> @@ -115,11 +112,6 @@ extern int qcom_scm_qsmmu500_wait_safe_toggle(bool en);
>  
>  static inline bool qcom_scm_is_available(void) { return false; }
>  
> -static inline int qcom_scm_set_cold_boot_addr(void *entry,
> -		const cpumask_t *cpus) { return -ENODEV; }
> -static inline int qcom_scm_set_warm_boot_addr(void *entry,
> -		const cpumask_t *cpus) { return -ENODEV; }
> -static inline void qcom_scm_cpu_power_down(u32 flags) {}
>  static inline u32 qcom_scm_set_remote_state(u32 state,u32 id)
>  		{ return -ENODEV; }
>  
> @@ -171,4 +163,17 @@ static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
>  static inline int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
>  		{ return -ENODEV; }
>  #endif
> +
> +#if IS_ENABLED(CONFIG_ARM) && IS_ENABLED(CONFIG_QCOM_SCM)
> +extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
> +extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
> +extern void qcom_scm_cpu_power_down(u32 flags);
> +#else
> +static inline int qcom_scm_set_cold_boot_addr(void *entry,
> +		const cpumask_t *cpus) { return -ENODEV; }
> +static inline int qcom_scm_set_warm_boot_addr(void *entry,
> +		const cpumask_t *cpus) { return -ENODEV; }
> +static inline void qcom_scm_cpu_power_down(u32 flags) {}
> +#endif
> +
>  #endif
> 
> base-commit: 3b9cdafb5358eb9f3790de2f728f765fef100731
> prerequisite-patch-id: 77da2cfd7591b1d7c35e879dca67d4f037f40e48
> prerequisite-patch-id: 021337034973fa8ce52fc8c84787f40dabb33df6
> prerequisite-patch-id: 5d374e97d8f0d384098a46e91006811ab89c84b0
> prerequisite-patch-id: 892de894cc937f7fe6ddb8f95ec9e2e3557830c7
> prerequisite-patch-id: 33b2442181aeb8adfa1c08d9a167d3bcbd1660fe
> -- 
> https://chromeos.dev
> 

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Elliot Berman]

On 3/23/2021 3:43 PM, Stephen Boyd wrote:
> These scm calls are never used outside of legacy ARMv7 based platforms.
> That's because PSCI, mandated on arm64, implements them for modern SoCs
> via the PSCI spec. Let's move them to the legacy file and only compile
> the legacy file into the kernel when CONFIG_ARM=y. Otherwise provide
> stubs and fail the calls. This saves a little bit of space in an
> arm64 allmodconfig.
> 
>   $ ./scripts/bloat-o-meter vmlinux.before vmlinux.after
>   add/remove: 0/8 grow/shrink: 5/6 up/down: 509/-4401 (-3892)
>   Function                                     old     new   delta
>   __qcom_scm_set_dload_mode.constprop          312     452    +140
>   qcom_scm_qsmmu500_wait_safe_toggle           288     416    +128
>   qcom_scm_io_writel                           288     408    +120
>   qcom_scm_io_readl                            376     492    +116
>   __param_str_download_mode                     23      28      +5
>   __warned                                    4327    4326      -1
>   e843419@0b3f_00010432_324                      8       -      -8
>   qcom_scm_call                                228     208     -20
>   CSWTCH                                      5925    5877     -48
>   _sub_I_65535_1                            163100  163040     -60
>   _sub_D_65535_0                            163100  163040     -60
>   qcom_scm_wb                                   64       -     -64
>   qcom_scm_lock                                320     160    -160
>   qcom_scm_call_atomic                         212       -    -212
>   qcom_scm_cpu_power_down                      308       -    -308
>   scm_legacy_call_atomic                       520       -    -520
>   qcom_scm_set_warm_boot_addr                  720       -    -720
>   qcom_scm_set_cold_boot_addr                  728       -    -728
>   scm_legacy_call                             1492       -   -1492
>   Total: Before=66737606, After=66733714, chg -0.01%
> 
> Commit 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and
> legacy conventions") didn't mention any motivating factors for keeping
> the legacy code around on arm64 kernels, i.e. presumably that commit
> wasn't trying to support these legacy APIs on arm64 kernels.
> 
> Cc: Elliot Berman <eberman@codeaurora.org>
> Cc: Brian Masney <masneyb@onstation.org>
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
It might be a good idea to wrap these lines from qcom_scm_call with #if 
IS_ENABLED(CONFIG_ARM), and the corresponding ones in qcom_scm_call_atomic:

   case SMC_CONVENTION_LEGACY:
       return scm_legacy_call(dev, desc, res);

If something is wrong with loaded firmware and LEGACY convention is 
incorrectly selected, you would get a better hint about the problem: 
"Unknown current SCM calling convention." You would still get the hint 
earlier from __get_convention, but that may not be obvious to someone 
unfamiliar with the SCM driver.

I'll defer to your/Bjorn's preference:

Acked-by: Elliot Berman <eberman@codeaurora.org>

with or without modifying qcom_scm_call{_atomic}.

> 
> Followup to v1 (https://lore.kernel.org/r/20210223214539.1336155-7-swboyd@chromium.org):
>   * Don't change the legacy file to use legacy calls only
>   * Wrap more things in CONFIG_ARM checks
> 
>   drivers/firmware/Makefile   |  4 +++-
>   drivers/firmware/qcom_scm.c | 47 ++++++++++++++++++++-----------------
>   drivers/firmware/qcom_scm.h | 15 ++++++++++++
>   include/linux/qcom_scm.h    | 21 ++++++++++-------
>   4 files changed, 56 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 5e013b6a3692..0b7b35555a6c 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -17,7 +17,9 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
>   obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
>   obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
>   obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
> -obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
> +obj-$(CONFIG_QCOM_SCM)		+= qcom_scm_objs.o
> +qcom_scm_objs-$(CONFIG_ARM)	+= qcom_scm-legacy.o
> +qcom_scm_objs-$(CONFIG_QCOM_SCM) += qcom_scm.o qcom_scm-smc.o
>   obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
>   obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
>   obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index ee9cb545e73b..747808a8ddf4 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -49,28 +49,6 @@ struct qcom_scm_mem_map_info {
>   	__le64 mem_size;
>   };
>   
> -#define QCOM_SCM_FLAG_COLDBOOT_CPU0	0x00
> -#define QCOM_SCM_FLAG_COLDBOOT_CPU1	0x01
> -#define QCOM_SCM_FLAG_COLDBOOT_CPU2	0x08
> -#define QCOM_SCM_FLAG_COLDBOOT_CPU3	0x20
> -
> -#define QCOM_SCM_FLAG_WARMBOOT_CPU0	0x04
> -#define QCOM_SCM_FLAG_WARMBOOT_CPU1	0x02
> -#define QCOM_SCM_FLAG_WARMBOOT_CPU2	0x10
> -#define QCOM_SCM_FLAG_WARMBOOT_CPU3	0x40
> -
> -struct qcom_scm_wb_entry {
> -	int flag;
> -	void *entry;
> -};
> -
> -static struct qcom_scm_wb_entry qcom_scm_wb[] = {
> -	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU0 },
> -	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU1 },
> -	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU2 },
> -	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU3 },
> -};
> -
>   static const char *qcom_scm_convention_names[] = {
>   	[SMC_CONVENTION_UNKNOWN] = "unknown",
>   	[SMC_CONVENTION_ARM_32] = "smc arm 32",
> @@ -260,6 +238,30 @@ static bool __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>   	return ret ? false : !!res.result[0];
>   }
>   
> +#if IS_ENABLED(CONFIG_ARM)
> +
> +#define QCOM_SCM_FLAG_COLDBOOT_CPU0	0x00
> +#define QCOM_SCM_FLAG_COLDBOOT_CPU1	0x01
> +#define QCOM_SCM_FLAG_COLDBOOT_CPU2	0x08
> +#define QCOM_SCM_FLAG_COLDBOOT_CPU3	0x20
> +
> +#define QCOM_SCM_FLAG_WARMBOOT_CPU0	0x04
> +#define QCOM_SCM_FLAG_WARMBOOT_CPU1	0x02
> +#define QCOM_SCM_FLAG_WARMBOOT_CPU2	0x10
> +#define QCOM_SCM_FLAG_WARMBOOT_CPU3	0x40
> +
> +struct qcom_scm_wb_entry {
> +	int flag;
> +	void *entry;
> +};
> +
> +static struct qcom_scm_wb_entry qcom_scm_wb[] = {
> +	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU0 },
> +	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU1 },
> +	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU2 },
> +	{ .flag = QCOM_SCM_FLAG_WARMBOOT_CPU3 },
> +};
> +
>   /**
>    * qcom_scm_set_warm_boot_addr() - Set the warm boot address for cpus
>    * @entry: Entry point function for the cpus
> @@ -369,6 +371,7 @@ void qcom_scm_cpu_power_down(u32 flags)
>   	qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
>   }
>   EXPORT_SYMBOL(qcom_scm_cpu_power_down);
> +#endif
>   
>   int qcom_scm_set_remote_state(u32 state, u32 id)
>   {
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 632fe3142462..735e975320e4 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -68,11 +68,26 @@ extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>   	__scm_smc_call((dev), (desc), qcom_scm_convention, (res), (atomic))
>   
>   #define SCM_LEGACY_FNID(s, c)	(((s) << 10) | ((c) & 0x3ff))
> +#if IS_ENABLED(CONFIG_ARM)
>   extern int scm_legacy_call_atomic(struct device *dev,
>   				  const struct qcom_scm_desc *desc,
>   				  struct qcom_scm_res *res);
>   extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>   			   struct qcom_scm_res *res);
> +#else
> +static inline int scm_legacy_call_atomic(struct device *dev,
> +					 const struct qcom_scm_desc *desc,
> +					 struct qcom_scm_res *res)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> +				  struct qcom_scm_res *res)
> +{
> +	return -EINVAL;
> +}
> +#endif
>   
>   #define QCOM_SCM_SVC_BOOT		0x01
>   #define QCOM_SCM_BOOT_SET_ADDR		0x01
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index 0165824c5128..0ec905d56e1a 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -64,9 +64,6 @@ enum qcom_scm_ice_cipher {
>   #if IS_ENABLED(CONFIG_QCOM_SCM)
>   extern bool qcom_scm_is_available(void);
>   
> -extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
> -extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
> -extern void qcom_scm_cpu_power_down(u32 flags);
>   extern int qcom_scm_set_remote_state(u32 state, u32 id);
>   
>   extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
> @@ -115,11 +112,6 @@ extern int qcom_scm_qsmmu500_wait_safe_toggle(bool en);
>   
>   static inline bool qcom_scm_is_available(void) { return false; }
>   
> -static inline int qcom_scm_set_cold_boot_addr(void *entry,
> -		const cpumask_t *cpus) { return -ENODEV; }
> -static inline int qcom_scm_set_warm_boot_addr(void *entry,
> -		const cpumask_t *cpus) { return -ENODEV; }
> -static inline void qcom_scm_cpu_power_down(u32 flags) {}
>   static inline u32 qcom_scm_set_remote_state(u32 state,u32 id)
>   		{ return -ENODEV; }
>   
> @@ -171,4 +163,17 @@ static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
>   static inline int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
>   		{ return -ENODEV; }
>   #endif
> +
> +#if IS_ENABLED(CONFIG_ARM) && IS_ENABLED(CONFIG_QCOM_SCM)
> +extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
> +extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
> +extern void qcom_scm_cpu_power_down(u32 flags);
> +#else
> +static inline int qcom_scm_set_cold_boot_addr(void *entry,
> +		const cpumask_t *cpus) { return -ENODEV; }
> +static inline int qcom_scm_set_warm_boot_addr(void *entry,
> +		const cpumask_t *cpus) { return -ENODEV; }
> +static inline void qcom_scm_cpu_power_down(u32 flags) {}
> +#endif
> +
>   #endif
> 
> base-commit: 3b9cdafb5358eb9f3790de2f728f765fef100731
> prerequisite-patch-id: 77da2cfd7591b1d7c35e879dca67d4f037f40e48
> prerequisite-patch-id: 021337034973fa8ce52fc8c84787f40dabb33df6
> prerequisite-patch-id: 5d374e97d8f0d384098a46e91006811ab89c84b0
> prerequisite-patch-id: 892de894cc937f7fe6ddb8f95ec9e2e3557830c7
> prerequisite-patch-id: 33b2442181aeb8adfa1c08d9a167d3bcbd1660fe
> 

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephen Boyd]

Quoting Elliot Berman (2021-04-01 18:12:14)
> 
> It might be a good idea to wrap these lines from qcom_scm_call with #if 
> IS_ENABLED(CONFIG_ARM), and the corresponding ones in qcom_scm_call_atomic:
> 
>    case SMC_CONVENTION_LEGACY:
>        return scm_legacy_call(dev, desc, res);
> 
> If something is wrong with loaded firmware and LEGACY convention is 
> incorrectly selected, you would get a better hint about the problem: 
> "Unknown current SCM calling convention." You would still get the hint 
> earlier from __get_convention, but that may not be obvious to someone 
> unfamiliar with the SCM driver.
> 
> I'll defer to your/Bjorn's preference:
> 
> Acked-by: Elliot Berman <eberman@codeaurora.org>
> 
> with or without modifying qcom_scm_call{_atomic}.
> 

Maybe it would be better to catch that problem at the source and force
arm64 calling convention to be safe? I'm thinking of this patch, but it
could be even more fine tuned and probably the sc7180 check could be
removed in the process if the rest of this email makes sense.

If I understand correctly CONFIG_ARM64=y should never use legacy
convention (and never the 32-bit one either?), so we can figure that out
pretty easily and just force it to use 64-bit if the architecture is
arm64. If only the 64-bit convention is supported on arm64 then we
really don't even need to call into the firmware to figure it out on
arm64. We can do this convention detection stuff on arm32 (CONFIG_ARM)
and always assume 64-bit convention on CONFIG_ARM64. Is that right?

---8<---
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 747808a8ddf4..22187ebc1678 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -140,7 +140,13 @@ static enum qcom_scm_convention __get_convention(void)
 	if (!ret && res.result[0] == 1)
 		goto found;
 
-	probed_convention = SMC_CONVENTION_LEGACY;
+	if (IS_ENABLED(CONFIG_ARM)) {
+		probed_convention = SMC_CONVENTION_LEGACY;
+	} else {
+		probed_convention = SMC_CONVENTION_ARM_64;
+		forced = true;
+		WARN(1, "qcom_scm: Detected legacy convention on arm64; firmware is broken\n");
+	}
 found:
 	spin_lock_irqsave(&scm_query_lock, flags);
 	if (probed_convention != qcom_scm_convention) {

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephan Gerhold]

On Thu, Apr 01, 2021 at 11:58:48PM -0700, Stephen Boyd wrote:
> Quoting Elliot Berman (2021-04-01 18:12:14)
> > 
> > It might be a good idea to wrap these lines from qcom_scm_call with #if 
> > IS_ENABLED(CONFIG_ARM), and the corresponding ones in qcom_scm_call_atomic:
> > 
> >    case SMC_CONVENTION_LEGACY:
> >        return scm_legacy_call(dev, desc, res);
> > 
> > If something is wrong with loaded firmware and LEGACY convention is 
> > incorrectly selected, you would get a better hint about the problem: 
> > "Unknown current SCM calling convention." You would still get the hint 
> > earlier from __get_convention, but that may not be obvious to someone 
> > unfamiliar with the SCM driver.
> > 
> > I'll defer to your/Bjorn's preference:
> > 
> > Acked-by: Elliot Berman <eberman@codeaurora.org>
> > 
> > with or without modifying qcom_scm_call{_atomic}.
> > 
> 
> Maybe it would be better to catch that problem at the source and force
> arm64 calling convention to be safe? I'm thinking of this patch, but it
> could be even more fine tuned and probably the sc7180 check could be
> removed in the process if the rest of this email makes sense.
> 
> If I understand correctly CONFIG_ARM64=y should never use legacy
> convention (and never the 32-bit one either?), so we can figure that out
> pretty easily and just force it to use 64-bit if the architecture is
> arm64. If only the 64-bit convention is supported on arm64 then we
> really don't even need to call into the firmware to figure it out on
> arm64. We can do this convention detection stuff on arm32 (CONFIG_ARM)
> and always assume 64-bit convention on CONFIG_ARM64. Is that right?
> 

No, the detection is also needed on ARM64. On ARM32 there can be either
legacy or SMC32, but on ARM64 there can be either SMC32 or SMC64.
You cannot use SMC64 on 32-bit, but you can use SMC32 on ARM64 just
fine. SMC32 is used on MSM8916 for example.

Stephan

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephen Boyd]

Quoting Stephan Gerhold (2021-04-02 03:18:04)
> On Thu, Apr 01, 2021 at 11:58:48PM -0700, Stephen Boyd wrote:
> > Quoting Elliot Berman (2021-04-01 18:12:14)
> > > 
> > > It might be a good idea to wrap these lines from qcom_scm_call with #if 
> > > IS_ENABLED(CONFIG_ARM), and the corresponding ones in qcom_scm_call_atomic:
> > > 
> > >    case SMC_CONVENTION_LEGACY:
> > >        return scm_legacy_call(dev, desc, res);
> > > 
> > > If something is wrong with loaded firmware and LEGACY convention is 
> > > incorrectly selected, you would get a better hint about the problem: 
> > > "Unknown current SCM calling convention." You would still get the hint 
> > > earlier from __get_convention, but that may not be obvious to someone 
> > > unfamiliar with the SCM driver.
> > > 
> > > I'll defer to your/Bjorn's preference:
> > > 
> > > Acked-by: Elliot Berman <eberman@codeaurora.org>
> > > 
> > > with or without modifying qcom_scm_call{_atomic}.
> > > 
> > 
> > Maybe it would be better to catch that problem at the source and force
> > arm64 calling convention to be safe? I'm thinking of this patch, but it
> > could be even more fine tuned and probably the sc7180 check could be
> > removed in the process if the rest of this email makes sense.
> > 
> > If I understand correctly CONFIG_ARM64=y should never use legacy
> > convention (and never the 32-bit one either?), so we can figure that out
> > pretty easily and just force it to use 64-bit if the architecture is
> > arm64. If only the 64-bit convention is supported on arm64 then we
> > really don't even need to call into the firmware to figure it out on
> > arm64. We can do this convention detection stuff on arm32 (CONFIG_ARM)
> > and always assume 64-bit convention on CONFIG_ARM64. Is that right?
> > 
> 
> No, the detection is also needed on ARM64. On ARM32 there can be either
> legacy or SMC32, but on ARM64 there can be either SMC32 or SMC64.
> You cannot use SMC64 on 32-bit, but you can use SMC32 on ARM64 just
> fine. SMC32 is used on MSM8916 for example.
> 

Ah right, the whole secure world running in 32-bit mode thing. Is
msm8916 the only SoC that's using that? Or are there more? If only
msm8916 is affected then we could use a combination of CONFIG_ARM64 and
the compatible string to differentiate and then if more SoCs use 32-bit
secure world then we could have a new compatible string like qcom,scm-32
that tells us this fact. Maybe this was all discussed before and I
missed it. Either way, I'm trying to get rid of this boot call so that
we don't have to bounce to the firmware an extra time to figure out
something we can figure out from the kernel arch and scm compatible
string.

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephan Gerhold]

On Fri, Apr 02, 2021 at 10:21:58AM -0700, Stephen Boyd wrote:
> Quoting Stephan Gerhold (2021-04-02 03:18:04)
> > On Thu, Apr 01, 2021 at 11:58:48PM -0700, Stephen Boyd wrote:
> > > [...]
> > >
> > > Maybe it would be better to catch that problem at the source and force
> > > arm64 calling convention to be safe? I'm thinking of this patch, but it
> > > could be even more fine tuned and probably the sc7180 check could be
> > > removed in the process if the rest of this email makes sense.
> > > 
> > > If I understand correctly CONFIG_ARM64=y should never use legacy
> > > convention (and never the 32-bit one either?), so we can figure that out
> > > pretty easily and just force it to use 64-bit if the architecture is
> > > arm64. If only the 64-bit convention is supported on arm64 then we
> > > really don't even need to call into the firmware to figure it out on
> > > arm64. We can do this convention detection stuff on arm32 (CONFIG_ARM)
> > > and always assume 64-bit convention on CONFIG_ARM64. Is that right?
> > > 
> > 
> > No, the detection is also needed on ARM64. On ARM32 there can be either
> > legacy or SMC32, but on ARM64 there can be either SMC32 or SMC64.
> > You cannot use SMC64 on 32-bit, but you can use SMC32 on ARM64 just
> > fine. SMC32 is used on MSM8916 for example.
> > 
> 
> Ah right, the whole secure world running in 32-bit mode thing. Is
> msm8916 the only SoC that's using that? Or are there more? If only
> msm8916 is affected then we could use a combination of CONFIG_ARM64 and
> the compatible string to differentiate and then if more SoCs use 32-bit
> secure world then we could have a new compatible string like qcom,scm-32
> that tells us this fact. Maybe this was all discussed before and I
> missed it. Either way, I'm trying to get rid of this boot call so that
> we don't have to bounce to the firmware an extra time to figure out
> something we can figure out from the kernel arch and scm compatible
> string.

At least MSM8994 also uses SMC32 from what I heard. Overall it's
probably quite hard to get that right now since all boards were tested
with the dynamic detection so far. I suppose you could do the opposite,
add an optional qcom,scm-64 to skip the detection step and force SMC64.

Also note that this could even be firmware-specific, not necessarily
SoC-specific. There are some ancient MSM8916 firmwares that have legacy
instead of SMC32. I could also imagine that there is also some SoC where
there are different firmware versions with SMC32 or SMC64.

Plus, IMO the overhead for this detection is negligible. At least it
ensures that we always use the right calling convention. The PSCI code
probably does much more firmware calls to figure out all supported
features.

Stephan

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephen Boyd]

Quoting Stephan Gerhold (2021-04-05 05:50:26)
> On Fri, Apr 02, 2021 at 10:21:58AM -0700, Stephen Boyd wrote:
> > 
> > Ah right, the whole secure world running in 32-bit mode thing. Is
> > msm8916 the only SoC that's using that? Or are there more? If only
> > msm8916 is affected then we could use a combination of CONFIG_ARM64 and
> > the compatible string to differentiate and then if more SoCs use 32-bit
> > secure world then we could have a new compatible string like qcom,scm-32
> > that tells us this fact. Maybe this was all discussed before and I
> > missed it. Either way, I'm trying to get rid of this boot call so that
> > we don't have to bounce to the firmware an extra time to figure out
> > something we can figure out from the kernel arch and scm compatible
> > string.
> 
> At least MSM8994 also uses SMC32 from what I heard. Overall it's
> probably quite hard to get that right now since all boards were tested
> with the dynamic detection so far. I suppose you could do the opposite,
> add an optional qcom,scm-64 to skip the detection step and force SMC64.

Isn't SMC64 going to be the overall majority going forward? Legacy
convention is for sure limited to CONFIG_ARM so I'll send another
follow-up patch to add a warning if we find legacy on CONFIG_ARM64.
SMC32 is hopefully no longer being produced given that it was introduced
at the time that the bootloader team wasn't supporting PSCI and didn't
want to support it. So we're making all new boards/SoCs/firmwares do
this calling convention probing to figure out something they already
know?

Maybe we should probe the calling convention on msm8994/msm8916 and
otherwise assume SMC64 on CONFIG_ARM64 kernels. I'd expect the exception
list to be smaller that way.

> 
> Also note that this could even be firmware-specific, not necessarily
> SoC-specific. There are some ancient MSM8916 firmwares that have legacy
> instead of SMC32. I could also imagine that there is also some SoC where
> there are different firmware versions with SMC32 or SMC64.

Sure but in theory the firmware would update the DT to indicate what
sort of firmware is there.

> 
> Plus, IMO the overhead for this detection is negligible. At least it
> ensures that we always use the right calling convention. The PSCI code
> probably does much more firmware calls to figure out all supported
> features.
> 

Heh, it tried to ensure we use the right calling convention but broke
things in the process, because the way of detecting the convention isn't
always there. I wouldn't be surprised if this comes up again for other
boards that use TF-A.

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephan Gerhold]

On Wed, Apr 07, 2021 at 05:12:06PM -0700, Stephen Boyd wrote:
> Quoting Stephan Gerhold (2021-04-05 05:50:26)
> > On Fri, Apr 02, 2021 at 10:21:58AM -0700, Stephen Boyd wrote:
> > > 
> > > Ah right, the whole secure world running in 32-bit mode thing. Is
> > > msm8916 the only SoC that's using that? Or are there more? If only
> > > msm8916 is affected then we could use a combination of CONFIG_ARM64 and
> > > the compatible string to differentiate and then if more SoCs use 32-bit
> > > secure world then we could have a new compatible string like qcom,scm-32
> > > that tells us this fact. Maybe this was all discussed before and I
> > > missed it. Either way, I'm trying to get rid of this boot call so that
> > > we don't have to bounce to the firmware an extra time to figure out
> > > something we can figure out from the kernel arch and scm compatible
> > > string.
> > 
> > At least MSM8994 also uses SMC32 from what I heard. Overall it's
> > probably quite hard to get that right now since all boards were tested
> > with the dynamic detection so far. I suppose you could do the opposite,
> > add an optional qcom,scm-64 to skip the detection step and force SMC64.
> 
> Isn't SMC64 going to be the overall majority going forward? Legacy
> convention is for sure limited to CONFIG_ARM so I'll send another
> follow-up patch to add a warning if we find legacy on CONFIG_ARM64.
> SMC32 is hopefully no longer being produced given that it was introduced
> at the time that the bootloader team wasn't supporting PSCI and didn't
> want to support it. So we're making all new boards/SoCs/firmwares do
> this calling convention probing to figure out something they already
> know?
> 
> Maybe we should probe the calling convention on msm8994/msm8916 and
> otherwise assume SMC64 on CONFIG_ARM64 kernels. I'd expect the exception
> list to be smaller that way.
> 

Personally, I think it would be best to introduce a new, SMC64 only
compatible (e.g. "qcom,scm-64" like I mentioned). Then you can skip the
detection check for the boards that opt-in by adding the compatible.
You can then use it on all newer boards/SoCs/firmwares where you know
exactly that there is SMC64.

I would just like to avoid breaking any existing boards where we don't
know exactly if they have SMC32 or SMC64.

> > 
> > Also note that this could even be firmware-specific, not necessarily
> > SoC-specific. There are some ancient MSM8916 firmwares that have legacy
> > instead of SMC32. I could also imagine that there is also some SoC where
> > there are different firmware versions with SMC32 or SMC64.
> 
> Sure but in theory the firmware would update the DT to indicate what
> sort of firmware is there.
> 

In a perfect world the firmware would do that, but there is certainly
no such mechanism on any of the old SoCs :/

> > 
> > Plus, IMO the overhead for this detection is negligible. At least it
> > ensures that we always use the right calling convention. The PSCI code
> > probably does much more firmware calls to figure out all supported
> > features.
> > 
> 
> Heh, it tried to ensure we use the right calling convention but broke
> things in the process, because the way of detecting the convention isn't
> always there. I wouldn't be surprised if this comes up again for other
> boards that use TF-A.

Ah okay, this sounds like a better reason than just trying to avoid the
"overhead" of the detection step. :) I still think it should work if you
just start marking all newer boards/SoCs/... as "qcom,scm-64" or
something like that, right?

Thanks,
Stephan

Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

[Stephen Boyd]

Quoting Stephan Gerhold (2021-04-08 00:19:44)
> Personally, I think it would be best to introduce a new, SMC64 only
> compatible (e.g. "qcom,scm-64" like I mentioned). Then you can skip the
> detection check for the boards that opt-in by adding the compatible.
> You can then use it on all newer boards/SoCs/firmwares where you know
> exactly that there is SMC64.
> 
> I would just like to avoid breaking any existing boards where we don't
> know exactly if they have SMC32 or SMC64.

Ok that's fair.

> > 
> > Heh, it tried to ensure we use the right calling convention but broke
> > things in the process, because the way of detecting the convention isn't
> > always there. I wouldn't be surprised if this comes up again for other
> > boards that use TF-A.
> 
> Ah okay, this sounds like a better reason than just trying to avoid the
> "overhead" of the detection step. :) I still think it should work if you
> just start marking all newer boards/SoCs/... as "qcom,scm-64" or
> something like that, right?

Sure. I can cook up a set of patches for this.