The firmware that ships on sc7180 devices doesn't implement the smc call
that tells the kernel what calling convention is available. Instead, the
firmware returns an error code indicating the call isn't implemented
(that makes my head spin). To smooth things out here let's implement a
small workaround that checks the scm compatible string so we can force
the arm64 calling convention. This series also includes some fixes for
the "is call available" API because it doesn't seem to be used properly,
a documentation fix noticed while reading through the code, and
suppression of sysfs bind attributes to save us from rouge driver
removal.
Finally, the last patch is sort of an RFC, but I'd like to merge that
too so we can kick out the legacy API entirely on arm64 kernels. As far
as I know it isn't used so we can save some bytes by not compiling it or
using it unless the architecture is ARM. Let me know what you think.
Cc: Elliot Berman <[email protected]>
Cc: Brian Masney <[email protected]>
Cc: Stephan Gerhold <[email protected]>
Cc: Jeffrey Hugo <[email protected]>
Cc: Douglas Anderson <[email protected]>
Stephen Boyd (6):
firmware: qcom_scm: Make __qcom_scm_is_call_available() return bool
firmware: qcom_scm: Reduce locking section for __get_convention()
firmware: qcom_scm: Workaround lack of "is available" call on SC7180
firmware: qcom_scm: Suppress sysfs bind attributes
firmware: qcom_scm: Fix kernel-doc function names to match
firmware: qcom_scm: Only compile legacy calls on ARM
drivers/firmware/Makefile | 4 +-
drivers/firmware/qcom_scm-legacy.c | 137 ++++++++++++++++-
drivers/firmware/qcom_scm-smc.c | 12 +-
drivers/firmware/qcom_scm.c | 234 +++++++----------------------
drivers/firmware/qcom_scm.h | 40 ++++-
include/linux/qcom_scm.h | 21 ++-
6 files changed, 247 insertions(+), 201 deletions(-)
base-commit: 3b9cdafb5358eb9f3790de2f728f765fef100731
--
https://chromeos.dev
Make __qcom_scm_is_call_available() return bool instead of int. The
function has "is" in the name, so it should return a bool to indicate
the truth of the call being available. Unfortunately, it can return a
number < 0 which also looks "true", but not all callers expect that and
thus they think a call is available when really the check to see if the
call is available failed to figure it out.
Cc: Elliot Berman <[email protected]>
Cc: Brian Masney <[email protected]>
Cc: Stephan Gerhold <[email protected]>
Cc: Jeffrey Hugo <[email protected]>
Cc: Douglas Anderson <[email protected]>
Fixes: 0f206514749b ("scsi: firmware: qcom_scm: Add support for programming inline crypto keys")
Fixes: 0434a4061471 ("firmware: qcom: scm: add support to restore secure config to qcm_scm-32")
Fixes: b0a1614fb1f5 ("firmware: qcom: scm: add OCMEM lock/unlock interface")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/firmware/qcom_scm.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index f57779fc7ee9..2be5573dce53 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -113,9 +113,6 @@ static void qcom_scm_clk_disable(void)
clk_disable_unprepare(__scm->bus_clk);
}
-static int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
- u32 cmd_id);
-
enum qcom_scm_convention qcom_scm_convention;
static bool has_queried __read_mostly;
static DEFINE_SPINLOCK(query_lock);
@@ -219,8 +216,8 @@ static int qcom_scm_call_atomic(struct device *dev,
}
}
-static int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
- u32 cmd_id)
+static bool __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
+ u32 cmd_id)
{
int ret;
struct qcom_scm_desc desc = {
@@ -247,7 +244,7 @@ static int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
ret = qcom_scm_call(dev, &desc, &res);
- return ret ? : res.result[0];
+ return ret ? false : !!res.result[0];
}
/**
@@ -585,9 +582,8 @@ bool qcom_scm_pas_supported(u32 peripheral)
};
struct qcom_scm_res res;
- ret = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
- QCOM_SCM_PIL_PAS_IS_SUPPORTED);
- if (ret <= 0)
+ if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
+ QCOM_SCM_PIL_PAS_IS_SUPPORTED))
return false;
ret = qcom_scm_call(__scm->dev, &desc, &res);
@@ -1060,17 +1056,18 @@ EXPORT_SYMBOL(qcom_scm_ice_set_key);
*/
bool qcom_scm_hdcp_available(void)
{
+ bool avail;
int ret = qcom_scm_clk_enable();
if (ret)
return ret;
- ret = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
+ avail = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
QCOM_SCM_HDCP_INVOKE);
qcom_scm_clk_disable();
- return ret > 0;
+ return avail;
}
EXPORT_SYMBOL(qcom_scm_hdcp_available);
--
https://chromeos.dev
Some SC7180 firmwares don't implement the QCOM_SCM_INFO_IS_CALL_AVAIL
API, so we can't probe the calling convention. We detect the legacy
calling convention on these firmwares, because the availability call
always fails and legacy is the fallback. This leads to problems where
the rmtfs driver fails to probe, because it tries to assign memory with
a bad calling convention, which then leads to modem failing to load and
all networking, even wifi, to fail. Ouch!
Let's force the calling convention to be what it always is on this SoC,
i.e. arm64. Of course, the calling convention is not the same thing as
implementing the QCOM_SCM_INFO_IS_CALL_AVAIL API. The absence of the "is
this call available" API from the firmware means that any call to
__qcom_scm_is_call_available() fails. This is OK for now though because
none of the calls that are checked for existence are implemented on
firmware running on sc7180. If such a call needs to be checked for
existence in the future, we presume that firmware will implement this
API and then things will "just work".
Cc: Elliot Berman <[email protected]>
Cc: Brian Masney <[email protected]>
Cc: Stephan Gerhold <[email protected]>
Cc: Jeffrey Hugo <[email protected]>
Cc: Douglas Anderson <[email protected]>
Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/firmware/qcom_scm.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 21e07a464bd9..9ac84b5d6ce0 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -131,6 +131,7 @@ static enum qcom_scm_convention __get_convention(void)
struct qcom_scm_res res;
enum qcom_scm_convention probed_convention;
int ret;
+ bool forced = false;
if (likely(qcom_scm_convention != SMC_CONVENTION_UNKNOWN))
return qcom_scm_convention;
@@ -144,6 +145,18 @@ static enum qcom_scm_convention __get_convention(void)
if (!ret && res.result[0] == 1)
goto found;
+ /*
+ * Some SC7180 firmwares didn't implement the
+ * QCOM_SCM_INFO_IS_CALL_AVAIL call, so we fallback to forcing ARM_64
+ * calling conventions on these firmwares. Luckily we don't make any
+ * early calls into the firmware on these SoCs so the device pointer
+ * will be valid here to check if the compatible matches.
+ */
+ if (of_device_is_compatible(__scm ? __scm->dev->of_node : NULL, "qcom,scm-sc7180")) {
+ forced = true;
+ goto found;
+ }
+
probed_convention = SMC_CONVENTION_ARM_32;
ret = __scm_smc_call(NULL, &desc, probed_convention, &res, true);
if (!ret && res.result[0] == 1)
@@ -154,8 +167,9 @@ static enum qcom_scm_convention __get_convention(void)
spin_lock_irqsave(&scm_query_lock, flags);
if (probed_convention != qcom_scm_convention) {
qcom_scm_convention = probed_convention;
- pr_info("qcom_scm: convention: %s\n",
- qcom_scm_convention_names[qcom_scm_convention]);
+ pr_info("qcom_scm: convention: %s%s\n",
+ qcom_scm_convention_names[qcom_scm_convention],
+ forced ? " (forced)" : "");
}
spin_unlock_irqrestore(&scm_query_lock, flags);
--
https://chromeos.dev
We don't want userspace ejecting this driver at runtime. Various other
drivers call into this code because it provides the mechanism to
communicate with the secure world on qcom SoCs. It should probe once and
be present forever after that.
Cc: Elliot Berman <[email protected]>
Cc: Brian Masney <[email protected]>
Cc: Stephan Gerhold <[email protected]>
Cc: Jeffrey Hugo <[email protected]>
Cc: Douglas Anderson <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/firmware/qcom_scm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 9ac84b5d6ce0..ee9cb545e73b 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1301,6 +1301,7 @@ static struct platform_driver qcom_scm_driver = {
.driver = {
.name = "qcom_scm",
.of_match_table = qcom_scm_dt_match,
+ .suppress_bind_attrs = true,
},
.probe = qcom_scm_probe,
.shutdown = qcom_scm_shutdown,
--
https://chromeos.dev
These functions were renamed but the kernel doc didn't follow along. Fix
it.
Cc: Elliot Berman <[email protected]>
Cc: Brian Masney <[email protected]>
Cc: Stephan Gerhold <[email protected]>
Cc: Jeffrey Hugo <[email protected]>
Cc: Douglas Anderson <[email protected]>
Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/firmware/qcom_scm-legacy.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/qcom_scm-legacy.c b/drivers/firmware/qcom_scm-legacy.c
index eba6b60bfb61..1829ba220576 100644
--- a/drivers/firmware/qcom_scm-legacy.c
+++ b/drivers/firmware/qcom_scm-legacy.c
@@ -118,7 +118,7 @@ static void __scm_legacy_do(const struct arm_smccc_args *smc,
}
/**
- * qcom_scm_call() - Sends a command to the SCM and waits for the command to
+ * scm_legacy_call() - Sends a command to the SCM and waits for the command to
* finish processing.
*
* A note on cache maintenance:
@@ -209,7 +209,7 @@ int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
(n & 0xf))
/**
- * qcom_scm_call_atomic() - Send an atomic SCM command with up to 5 arguments
+ * scm_legacy_call_atomic() - Send an atomic SCM command with up to 5 arguments
* and 3 return values
* @desc: SCM call descriptor containing arguments
* @res: SCM call return values
--
https://chromeos.dev
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/7 up/down: 509/-4405 (-3896)
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
qcom_iommu_init 272 268 -4
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=66737642, After=66733746, 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 <[email protected]>
Cc: Brian Masney <[email protected]>
Cc: Stephan Gerhold <[email protected]>
Cc: Jeffrey Hugo <[email protected]>
Cc: Douglas Anderson <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/firmware/Makefile | 4 +-
drivers/firmware/qcom_scm-legacy.c | 133 ++++++++++++++++++++++++++
drivers/firmware/qcom_scm.c | 145 +----------------------------
drivers/firmware/qcom_scm.h | 33 +++++++
include/linux/qcom_scm.h | 21 +++--
5 files changed, 183 insertions(+), 153 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-legacy.c b/drivers/firmware/qcom_scm-legacy.c
index 1829ba220576..d909fa2716bc 100644
--- a/drivers/firmware/qcom_scm-legacy.c
+++ b/drivers/firmware/qcom_scm-legacy.c
@@ -3,6 +3,7 @@
* Copyright (C) 2015 Linaro Ltd.
*/
+#include <linux/cpumask.h>
#include <linux/slab.h>
#include <linux/io.h>
#include <linux/module.h>
@@ -240,3 +241,135 @@ int scm_legacy_call_atomic(struct device *unused,
return smc_res.a0;
}
+
+#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
+ * @cpus: The cpumask of cpus that will use the entry point
+ *
+ * Set the Linux entry point for the SCM to transfer control to when coming
+ * out of a power down. CPU power down may be executed on cpuidle or hotplug.
+ */
+int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
+{
+ int ret;
+ int flags = 0;
+ int cpu;
+ struct qcom_scm_desc desc = {
+ .svc = QCOM_SCM_SVC_BOOT,
+ .cmd = QCOM_SCM_BOOT_SET_ADDR,
+ .arginfo = QCOM_SCM_ARGS(2),
+ };
+
+ /*
+ * Reassign only if we are switching from hotplug entry point
+ * to cpuidle entry point or vice versa.
+ */
+ for_each_cpu(cpu, cpus) {
+ if (entry == qcom_scm_wb[cpu].entry)
+ continue;
+ flags |= qcom_scm_wb[cpu].flag;
+ }
+
+ /* No change in entry function */
+ if (!flags)
+ return 0;
+
+ desc.args[0] = flags;
+ desc.args[1] = virt_to_phys(entry);
+
+ ret = scm_legacy_call(__scm->dev, &desc, NULL);
+ if (!ret) {
+ for_each_cpu(cpu, cpus)
+ qcom_scm_wb[cpu].entry = entry;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(qcom_scm_set_warm_boot_addr);
+
+/**
+ * qcom_scm_set_cold_boot_addr() - Set the cold boot address for cpus
+ * @entry: Entry point function for the cpus
+ * @cpus: The cpumask of cpus that will use the entry point
+ *
+ * Set the cold boot address of the cpus. Any cpu outside the supported
+ * range would be removed from the cpu present mask.
+ */
+int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
+{
+ int flags = 0;
+ int cpu;
+ int scm_cb_flags[] = {
+ QCOM_SCM_FLAG_COLDBOOT_CPU0,
+ QCOM_SCM_FLAG_COLDBOOT_CPU1,
+ QCOM_SCM_FLAG_COLDBOOT_CPU2,
+ QCOM_SCM_FLAG_COLDBOOT_CPU3,
+ };
+ struct qcom_scm_desc desc = {
+ .svc = QCOM_SCM_SVC_BOOT,
+ .cmd = QCOM_SCM_BOOT_SET_ADDR,
+ .arginfo = QCOM_SCM_ARGS(2),
+ .owner = ARM_SMCCC_OWNER_SIP,
+ };
+
+ if (!cpus || (cpus && cpumask_empty(cpus)))
+ return -EINVAL;
+
+ for_each_cpu(cpu, cpus) {
+ if (cpu < ARRAY_SIZE(scm_cb_flags))
+ flags |= scm_cb_flags[cpu];
+ else
+ set_cpu_present(cpu, false);
+ }
+
+ desc.args[0] = flags;
+ desc.args[1] = virt_to_phys(entry);
+
+ return scm_legacy_call_atomic(NULL, &desc, NULL);
+}
+EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
+
+/**
+ * qcom_scm_cpu_power_down() - Power down the cpu
+ * @flags - Flags to flush cache
+ *
+ * This is an end point to power down cpu. If there was a pending interrupt,
+ * the control would return from this function, otherwise, the cpu jumps to the
+ * warm boot entry point set for this cpu upon reset.
+ */
+void qcom_scm_cpu_power_down(u32 flags)
+{
+ struct qcom_scm_desc desc = {
+ .svc = QCOM_SCM_SVC_BOOT,
+ .cmd = QCOM_SCM_BOOT_TERMINATE_PC,
+ .args[0] = flags & QCOM_SCM_FLUSH_FLAG_MASK,
+ .arginfo = QCOM_SCM_ARGS(1),
+ .owner = ARM_SMCCC_OWNER_SIP,
+ };
+
+ scm_legacy_call_atomic(NULL, &desc, NULL);
+}
+EXPORT_SYMBOL(qcom_scm_cpu_power_down);
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index ee9cb545e73b..29bce83f8a25 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -4,7 +4,6 @@
*/
#include <linux/platform_device.h>
#include <linux/init.h>
-#include <linux/cpumask.h>
#include <linux/export.h>
#include <linux/dma-mapping.h>
#include <linux/module.h>
@@ -26,16 +25,6 @@ module_param(download_mode, bool, 0);
#define SCM_HAS_IFACE_CLK BIT(1)
#define SCM_HAS_BUS_CLK BIT(2)
-struct qcom_scm {
- struct device *dev;
- struct clk *core_clk;
- struct clk *iface_clk;
- struct clk *bus_clk;
- struct reset_controller_dev reset;
-
- u64 dload_mode_addr;
-};
-
struct qcom_scm_current_perm_info {
__le32 vmid;
__le32 perm;
@@ -49,28 +38,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",
@@ -78,7 +45,7 @@ static const char *qcom_scm_convention_names[] = {
[SMC_CONVENTION_LEGACY] = "smc legacy",
};
-static struct qcom_scm *__scm;
+struct qcom_scm *__scm;
static int qcom_scm_clk_enable(void)
{
@@ -260,116 +227,6 @@ static bool __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
return ret ? false : !!res.result[0];
}
-/**
- * qcom_scm_set_warm_boot_addr() - Set the warm boot address for cpus
- * @entry: Entry point function for the cpus
- * @cpus: The cpumask of cpus that will use the entry point
- *
- * Set the Linux entry point for the SCM to transfer control to when coming
- * out of a power down. CPU power down may be executed on cpuidle or hotplug.
- */
-int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
-{
- int ret;
- int flags = 0;
- int cpu;
- struct qcom_scm_desc desc = {
- .svc = QCOM_SCM_SVC_BOOT,
- .cmd = QCOM_SCM_BOOT_SET_ADDR,
- .arginfo = QCOM_SCM_ARGS(2),
- };
-
- /*
- * Reassign only if we are switching from hotplug entry point
- * to cpuidle entry point or vice versa.
- */
- for_each_cpu(cpu, cpus) {
- if (entry == qcom_scm_wb[cpu].entry)
- continue;
- flags |= qcom_scm_wb[cpu].flag;
- }
-
- /* No change in entry function */
- if (!flags)
- return 0;
-
- desc.args[0] = flags;
- desc.args[1] = virt_to_phys(entry);
-
- ret = qcom_scm_call(__scm->dev, &desc, NULL);
- if (!ret) {
- for_each_cpu(cpu, cpus)
- qcom_scm_wb[cpu].entry = entry;
- }
-
- return ret;
-}
-EXPORT_SYMBOL(qcom_scm_set_warm_boot_addr);
-
-/**
- * qcom_scm_set_cold_boot_addr() - Set the cold boot address for cpus
- * @entry: Entry point function for the cpus
- * @cpus: The cpumask of cpus that will use the entry point
- *
- * Set the cold boot address of the cpus. Any cpu outside the supported
- * range would be removed from the cpu present mask.
- */
-int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
-{
- int flags = 0;
- int cpu;
- int scm_cb_flags[] = {
- QCOM_SCM_FLAG_COLDBOOT_CPU0,
- QCOM_SCM_FLAG_COLDBOOT_CPU1,
- QCOM_SCM_FLAG_COLDBOOT_CPU2,
- QCOM_SCM_FLAG_COLDBOOT_CPU3,
- };
- struct qcom_scm_desc desc = {
- .svc = QCOM_SCM_SVC_BOOT,
- .cmd = QCOM_SCM_BOOT_SET_ADDR,
- .arginfo = QCOM_SCM_ARGS(2),
- .owner = ARM_SMCCC_OWNER_SIP,
- };
-
- if (!cpus || (cpus && cpumask_empty(cpus)))
- return -EINVAL;
-
- for_each_cpu(cpu, cpus) {
- if (cpu < ARRAY_SIZE(scm_cb_flags))
- flags |= scm_cb_flags[cpu];
- else
- set_cpu_present(cpu, false);
- }
-
- desc.args[0] = flags;
- desc.args[1] = virt_to_phys(entry);
-
- return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
-}
-EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
-
-/**
- * qcom_scm_cpu_power_down() - Power down the cpu
- * @flags - Flags to flush cache
- *
- * This is an end point to power down cpu. If there was a pending interrupt,
- * the control would return from this function, otherwise, the cpu jumps to the
- * warm boot entry point set for this cpu upon reset.
- */
-void qcom_scm_cpu_power_down(u32 flags)
-{
- struct qcom_scm_desc desc = {
- .svc = QCOM_SCM_SVC_BOOT,
- .cmd = QCOM_SCM_BOOT_TERMINATE_PC,
- .args[0] = flags & QCOM_SCM_FLUSH_FLAG_MASK,
- .arginfo = QCOM_SCM_ARGS(1),
- .owner = ARM_SMCCC_OWNER_SIP,
- };
-
- qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
-}
-EXPORT_SYMBOL(qcom_scm_cpu_power_down);
-
int qcom_scm_set_remote_state(u32 state, u32 id)
{
struct qcom_scm_desc desc = {
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 632fe3142462..be23f96557da 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -4,6 +4,24 @@
#ifndef __QCOM_SCM_INT_H
#define __QCOM_SCM_INT_H
+#include <linux/types.h>
+#include <linux/reset-controller.h>
+
+struct clk;
+struct device;
+
+struct qcom_scm {
+ struct device *dev;
+ struct clk *core_clk;
+ struct clk *iface_clk;
+ struct clk *bus_clk;
+ struct reset_controller_dev reset;
+
+ u64 dload_mode_addr;
+};
+
+extern struct qcom_scm *__scm;
+
enum qcom_scm_convention {
SMC_CONVENTION_UNKNOWN,
SMC_CONVENTION_LEGACY,
@@ -68,11 +86,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
--
https://chromeos.dev
We shouldn't need to hold this spinlock here around the entire SCM call
into the firmware and back. Instead, we should be able to query the
firmware, potentially in parallel with other CPUs making the same
convention detection firmware call, and then grab the lock to update the
calling convention detected. The convention doesn't change at runtime so
calling into firmware more than once is possibly wasteful but simpler.
Besides, this is the slow path, not the fast path where we've already
detected the convention used.
More importantly, this allows us to add more logic here to workaround
the case where the firmware call to check for availability isn't
implemented in the firmware at all. In that case we can check the
firmware node compatible string and force a calling convention.
Note that we remove the 'has_queried' logic that is repeated twice. That
could lead to the calling convention being printed multiple times to the
kernel logs if the bool is true but __query_convention() is running on
multiple CPUs. We also shorten the time where the lock is held, but we
keep the lock held around the printk because it doesn't seem hugely
important to drop it for that.
Cc: Elliot Berman <[email protected]>
Cc: Brian Masney <[email protected]>
Cc: Stephan Gerhold <[email protected]>
Cc: Jeffrey Hugo <[email protected]>
Cc: Douglas Anderson <[email protected]>
Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/firmware/qcom_scm-smc.c | 12 ++++---
drivers/firmware/qcom_scm.c | 55 ++++++++++++++++-----------------
drivers/firmware/qcom_scm.h | 7 +++--
3 files changed, 39 insertions(+), 35 deletions(-)
diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index 497c13ba98d6..d111833364ba 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -77,8 +77,10 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
} while (res->a0 == QCOM_SCM_V2_EBUSY);
}
-int scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
- struct qcom_scm_res *res, bool atomic)
+
+int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
+ enum qcom_scm_convention qcom_convention,
+ struct qcom_scm_res *res, bool atomic)
{
int arglen = desc->arginfo & 0xf;
int i;
@@ -87,9 +89,8 @@ int scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
size_t alloc_len;
gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
- u32 qcom_smccc_convention =
- (qcom_scm_convention == SMC_CONVENTION_ARM_32) ?
- ARM_SMCCC_SMC_32 : ARM_SMCCC_SMC_64;
+ u32 qcom_smccc_convention = (qcom_convention == SMC_CONVENTION_ARM_32) ?
+ ARM_SMCCC_SMC_32 : ARM_SMCCC_SMC_64;
struct arm_smccc_res smc_res;
struct arm_smccc_args smc = {0};
@@ -148,4 +149,5 @@ int scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
}
return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
+
}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 2be5573dce53..21e07a464bd9 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -113,11 +113,10 @@ static void qcom_scm_clk_disable(void)
clk_disable_unprepare(__scm->bus_clk);
}
-enum qcom_scm_convention qcom_scm_convention;
-static bool has_queried __read_mostly;
-static DEFINE_SPINLOCK(query_lock);
+enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
+static DEFINE_SPINLOCK(scm_query_lock);
-static void __query_convention(void)
+static enum qcom_scm_convention __get_convention(void)
{
unsigned long flags;
struct qcom_scm_desc desc = {
@@ -130,36 +129,36 @@ static void __query_convention(void)
.owner = ARM_SMCCC_OWNER_SIP,
};
struct qcom_scm_res res;
+ enum qcom_scm_convention probed_convention;
int ret;
- spin_lock_irqsave(&query_lock, flags);
- if (has_queried)
- goto out;
+ if (likely(qcom_scm_convention != SMC_CONVENTION_UNKNOWN))
+ return qcom_scm_convention;
- qcom_scm_convention = SMC_CONVENTION_ARM_64;
- // Device isn't required as there is only one argument - no device
- // needed to dma_map_single to secure world
- ret = scm_smc_call(NULL, &desc, &res, true);
+ /*
+ * Device isn't required as there is only one argument - no device
+ * needed to dma_map_single to secure world
+ */
+ probed_convention = SMC_CONVENTION_ARM_64;
+ ret = __scm_smc_call(NULL, &desc, probed_convention, &res, true);
if (!ret && res.result[0] == 1)
- goto out;
+ goto found;
- qcom_scm_convention = SMC_CONVENTION_ARM_32;
- ret = scm_smc_call(NULL, &desc, &res, true);
+ probed_convention = SMC_CONVENTION_ARM_32;
+ ret = __scm_smc_call(NULL, &desc, probed_convention, &res, true);
if (!ret && res.result[0] == 1)
- goto out;
-
- qcom_scm_convention = SMC_CONVENTION_LEGACY;
-out:
- has_queried = true;
- spin_unlock_irqrestore(&query_lock, flags);
- pr_info("qcom_scm: convention: %s\n",
- qcom_scm_convention_names[qcom_scm_convention]);
-}
+ goto found;
+
+ probed_convention = SMC_CONVENTION_LEGACY;
+found:
+ spin_lock_irqsave(&scm_query_lock, flags);
+ if (probed_convention != qcom_scm_convention) {
+ qcom_scm_convention = probed_convention;
+ pr_info("qcom_scm: convention: %s\n",
+ qcom_scm_convention_names[qcom_scm_convention]);
+ }
+ spin_unlock_irqrestore(&scm_query_lock, flags);
-static inline enum qcom_scm_convention __get_convention(void)
-{
- if (unlikely(!has_queried))
- __query_convention();
return qcom_scm_convention;
}
@@ -1239,7 +1238,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
__scm = scm;
__scm->dev = &pdev->dev;
- __query_convention();
+ __get_convention();
/*
* If requested enable "download mode", from this point on warmboot
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 95cd1ac30ab0..632fe3142462 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -61,8 +61,11 @@ struct qcom_scm_res {
};
#define SCM_SMC_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))
-extern int scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
- struct qcom_scm_res *res, bool atomic);
+extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
+ enum qcom_scm_convention qcom_convention,
+ struct qcom_scm_res *res, bool atomic);
+#define scm_smc_call(dev, desc, res, atomic) \
+ __scm_smc_call((dev), (desc), qcom_scm_convention, (res), (atomic))
#define SCM_LEGACY_FNID(s, c) (((s) << 10) | ((c) & 0x3ff))
extern int scm_legacy_call_atomic(struct device *dev,
--
https://chromeos.dev
On 2/23/2021 2:45 PM, Stephen Boyd wrote:
> Some SC7180 firmwares don't implement the QCOM_SCM_INFO_IS_CALL_AVAIL
> API, so we can't probe the calling convention. We detect the legacy
> calling convention on these firmwares, because the availability call
> always fails and legacy is the fallback. This leads to problems where
> the rmtfs driver fails to probe, because it tries to assign memory with
> a bad calling convention, which then leads to modem failing to load and
> all networking, even wifi, to fail. Ouch!
>
> Let's force the calling convention to be what it always is on this SoC,
> i.e. arm64. Of course, the calling convention is not the same thing as
> implementing the QCOM_SCM_INFO_IS_CALL_AVAIL API. The absence of the "is
> this call available" API from the firmware means that any call to
> __qcom_scm_is_call_available() fails. This is OK for now though because
> none of the calls that are checked for existence are implemented on
> firmware running on sc7180. If such a call needs to be checked for
> existence in the future, we presume that firmware will implement this
> API and then things will "just work".
>
> Cc: Elliot Berman <[email protected]>
> Cc: Brian Masney <[email protected]>
> Cc: Stephan Gerhold <[email protected]>
> Cc: Jeffrey Hugo <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/firmware/qcom_scm.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 21e07a464bd9..9ac84b5d6ce0 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -131,6 +131,7 @@ static enum qcom_scm_convention __get_convention(void)
> struct qcom_scm_res res;
> enum qcom_scm_convention probed_convention;
> int ret;
> + bool forced = false;
>
> if (likely(qcom_scm_convention != SMC_CONVENTION_UNKNOWN))
> return qcom_scm_convention;
> @@ -144,6 +145,18 @@ static enum qcom_scm_convention __get_convention(void)
> if (!ret && res.result[0] == 1)
> goto found;
>
> + /*
> + * Some SC7180 firmwares didn't implement the
> + * QCOM_SCM_INFO_IS_CALL_AVAIL call, so we fallback to forcing ARM_64
> + * calling conventions on these firmwares. Luckily we don't make any
> + * early calls into the firmware on these SoCs so the device pointer
> + * will be valid here to check if the compatible matches.
> + */
> + if (of_device_is_compatible(__scm ? __scm->dev->of_node : NULL, "qcom,scm-sc7180")) {
> + forced = true;
> + goto found;
> + }
All SC7180 targets run DT? None have ACPI?
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoting Jeffrey Hugo (2021-02-23 15:38:38)
> On 2/23/2021 2:45 PM, Stephen Boyd wrote:
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 21e07a464bd9..9ac84b5d6ce0 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -144,6 +145,18 @@ static enum qcom_scm_convention __get_convention(void)
> > if (!ret && res.result[0] == 1)
> > goto found;
> >
> > + /*
> > + * Some SC7180 firmwares didn't implement the
> > + * QCOM_SCM_INFO_IS_CALL_AVAIL call, so we fallback to forcing ARM_64
> > + * calling conventions on these firmwares. Luckily we don't make any
> > + * early calls into the firmware on these SoCs so the device pointer
> > + * will be valid here to check if the compatible matches.
> > + */
> > + if (of_device_is_compatible(__scm ? __scm->dev->of_node : NULL, "qcom,scm-sc7180")) {
> > + forced = true;
> > + goto found;
> > + }
>
> All SC7180 targets run DT? None have ACPI?
>
Yes, as far as I know all sc7180 boards are using DT. If they aren't,
then presumably they implemented this QCOM_SCM_INFO_IS_CALL_AVAIL call
so this check is still fine.
On 2/23/2021 1:45 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/7 up/down: 509/-4405 (-3896)
> 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
> qcom_iommu_init 272 268 -4
> 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=66737642, After=66733746, 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.
There are arm targets which support SMCCC convention and use some of
these removed functions. Can these functions be kept in qcom-scm.c and
wrapped with #if IS_ENABLED(CONFIG_ARM)?
Quoting Elliot Berman (2021-03-03 19:35:08)
>
> On 2/23/2021 1:45 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/7 up/down: 509/-4405 (-3896)
> > 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
> > qcom_iommu_init 272 268 -4
> > 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=66737642, After=66733746, 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.
>
> There are arm targets which support SMCCC convention and use some of
> these removed functions. Can these functions be kept in qcom-scm.c and
> wrapped with #if IS_ENABLED(CONFIG_ARM)?
>
It can be wrapped in qcom-scm.c, but why? It's all the same object file
so I'm lost why it matters. I suppose it would make it so the struct
doesn't have to be moved around and declared in the header? Any other
reason? I moved it to the legacy file so that it was very obvious that
the API wasn't to be used except for "legacy" platforms that don't use
PSCI.
On 3/3/2021 10:14 PM, Stephen Boyd wrote:
> Quoting Elliot Berman (2021-03-03 19:35:08)
>>
>> On 2/23/2021 1:45 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/7 up/down: 509/-4405 (-3896)
>>> 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
>>> qcom_iommu_init 272 268 -4
>>> 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=66737642, After=66733746, 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.
>>
>> There are arm targets which support SMCCC convention and use some of
>> these removed functions. Can these functions be kept in qcom-scm.c and
>> wrapped with #if IS_ENABLED(CONFIG_ARM)?
>>
>
> It can be wrapped in qcom-scm.c, but why? It's all the same object file
> so I'm lost why it matters. I suppose it would make it so the struct
> doesn't have to be moved around and declared in the header? Any other
> reason? I moved it to the legacy file so that it was very obvious that
> the API wasn't to be used except for "legacy" platforms that don't use
> PSCI.
>
There are "legacy" arm platforms that use the SMCCC (scm_smc_call) and
use the qcom_scm_set_{warm,cold}_boot_addr and qcom_scm_cpu_power_down
functions.
> + desc.args[0] = flags;
> + desc.args[1] = virt_to_phys(entry);
> +
> + return scm_legacy_call_atomic(NULL, &desc, NULL);
> +}
> +EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
This should still be qcom_scm_call.
On Tue 23 Feb 15:45 CST 2021, Stephen Boyd wrote:
> Make __qcom_scm_is_call_available() return bool instead of int. The
> function has "is" in the name, so it should return a bool to indicate
> the truth of the call being available. Unfortunately, it can return a
> number < 0 which also looks "true", but not all callers expect that and
> thus they think a call is available when really the check to see if the
> call is available failed to figure it out.
>
> Cc: Elliot Berman <[email protected]>
> Cc: Brian Masney <[email protected]>
> Cc: Stephan Gerhold <[email protected]>
> Cc: Jeffrey Hugo <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Fixes: 0f206514749b ("scsi: firmware: qcom_scm: Add support for programming inline crypto keys")
> Fixes: 0434a4061471 ("firmware: qcom: scm: add support to restore secure config to qcm_scm-32")
> Fixes: b0a1614fb1f5 ("firmware: qcom: scm: add OCMEM lock/unlock interface")
> Signed-off-by: Stephen Boyd <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
> drivers/firmware/qcom_scm.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index f57779fc7ee9..2be5573dce53 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -113,9 +113,6 @@ static void qcom_scm_clk_disable(void)
> clk_disable_unprepare(__scm->bus_clk);
> }
>
> -static int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
> - u32 cmd_id);
> -
> enum qcom_scm_convention qcom_scm_convention;
> static bool has_queried __read_mostly;
> static DEFINE_SPINLOCK(query_lock);
> @@ -219,8 +216,8 @@ static int qcom_scm_call_atomic(struct device *dev,
> }
> }
>
> -static int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
> - u32 cmd_id)
> +static bool __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
> + u32 cmd_id)
> {
> int ret;
> struct qcom_scm_desc desc = {
> @@ -247,7 +244,7 @@ static int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>
> ret = qcom_scm_call(dev, &desc, &res);
>
> - return ret ? : res.result[0];
> + return ret ? false : !!res.result[0];
> }
>
> /**
> @@ -585,9 +582,8 @@ bool qcom_scm_pas_supported(u32 peripheral)
> };
> struct qcom_scm_res res;
>
> - ret = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
> - QCOM_SCM_PIL_PAS_IS_SUPPORTED);
> - if (ret <= 0)
> + if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
> + QCOM_SCM_PIL_PAS_IS_SUPPORTED))
> return false;
>
> ret = qcom_scm_call(__scm->dev, &desc, &res);
> @@ -1060,17 +1056,18 @@ EXPORT_SYMBOL(qcom_scm_ice_set_key);
> */
> bool qcom_scm_hdcp_available(void)
> {
> + bool avail;
> int ret = qcom_scm_clk_enable();
>
> if (ret)
> return ret;
>
> - ret = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
> + avail = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
> QCOM_SCM_HDCP_INVOKE);
>
> qcom_scm_clk_disable();
>
> - return ret > 0;
> + return avail;
> }
> EXPORT_SYMBOL(qcom_scm_hdcp_available);
>
> --
> https://chromeos.dev
>
Quoting Elliot Berman (2021-03-05 10:18:09)
> On 3/3/2021 10:14 PM, Stephen Boyd wrote:
> > Quoting Elliot Berman (2021-03-03 19:35:08)
> >>
> >> On 2/23/2021 1:45 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/7 up/down: 509/-4405 (-3896)
> >>> 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
> >>> qcom_iommu_init 272 268 -4
> >>> 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=66737642, After=66733746, 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.
> >>
> >> There are arm targets which support SMCCC convention and use some of
> >> these removed functions. Can these functions be kept in qcom-scm.c and
> >> wrapped with #if IS_ENABLED(CONFIG_ARM)?
> >>
> >
> > It can be wrapped in qcom-scm.c, but why? It's all the same object file
> > so I'm lost why it matters. I suppose it would make it so the struct
> > doesn't have to be moved around and declared in the header? Any other
> > reason? I moved it to the legacy file so that it was very obvious that
> > the API wasn't to be used except for "legacy" platforms that don't use
> > PSCI.
> >
>
> There are "legacy" arm platforms that use the SMCCC (scm_smc_call) and
> use the qcom_scm_set_{warm,cold}_boot_addr and qcom_scm_cpu_power_down
> functions.
Ah ok. Weird, but I get it. Amazing that SMCCC was adopted there but
PSCI wasn't!
>
> > + desc.args[0] = flags;
> > + desc.args[1] = virt_to_phys(entry);
> > +
> > + return scm_legacy_call_atomic(NULL, &desc, NULL);
> > +}
> > +EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
>
> This should still be qcom_scm_call.
You mean s/scm_legacy_call_atomic/qcom_scm_call/ right?
I don't really want to resend the rest of the patches if this last one
is the only one that needs an update. This was a semi-RFC anyway so
maybe it's fine if the first 5 patches get merged and then I can resend
this one? Otherwise I will resend this again next week or so with less
diff for this patch.
On Sat 06 Mar 00:18 CST 2021, Stephen Boyd wrote:
> Quoting Elliot Berman (2021-03-05 10:18:09)
> > On 3/3/2021 10:14 PM, Stephen Boyd wrote:
> > > Quoting Elliot Berman (2021-03-03 19:35:08)
> > >>
> > >> On 2/23/2021 1:45 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/7 up/down: 509/-4405 (-3896)
> > >>> 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
> > >>> qcom_iommu_init 272 268 -4
> > >>> 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=66737642, After=66733746, 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.
> > >>
> > >> There are arm targets which support SMCCC convention and use some of
> > >> these removed functions. Can these functions be kept in qcom-scm.c and
> > >> wrapped with #if IS_ENABLED(CONFIG_ARM)?
> > >>
> > >
> > > It can be wrapped in qcom-scm.c, but why? It's all the same object file
> > > so I'm lost why it matters. I suppose it would make it so the struct
> > > doesn't have to be moved around and declared in the header? Any other
> > > reason? I moved it to the legacy file so that it was very obvious that
> > > the API wasn't to be used except for "legacy" platforms that don't use
> > > PSCI.
> > >
> >
> > There are "legacy" arm platforms that use the SMCCC (scm_smc_call) and
> > use the qcom_scm_set_{warm,cold}_boot_addr and qcom_scm_cpu_power_down
> > functions.
>
> Ah ok. Weird, but I get it. Amazing that SMCCC was adopted there but
> PSCI wasn't!
>
> >
> > > + desc.args[0] = flags;
> > > + desc.args[1] = virt_to_phys(entry);
> > > +
> > > + return scm_legacy_call_atomic(NULL, &desc, NULL);
> > > +}
> > > +EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
> >
> > This should still be qcom_scm_call.
>
> You mean s/scm_legacy_call_atomic/qcom_scm_call/ right?
>
> I don't really want to resend the rest of the patches if this last one
> is the only one that needs an update. This was a semi-RFC anyway so
> maybe it's fine if the first 5 patches get merged and then I can resend
> this one? Otherwise I will resend this again next week or so with less
> diff for this patch.
I'm fine with merging the first 5, but was hoping that Elliot could
provide either a "Reviewed-by" or at least an "Acked-by" on these.
Regards,
Bjorn
Quoting Bjorn Andersson (2021-03-07 09:42:45)
> On Sat 06 Mar 00:18 CST 2021, Stephen Boyd wrote:
>
> > Quoting Elliot Berman (2021-03-05 10:18:09)
> > > On 3/3/2021 10:14 PM, Stephen Boyd wrote:
> > > > Quoting Elliot Berman (2021-03-03 19:35:08)
> > >
> > > > + desc.args[0] = flags;
> > > > + desc.args[1] = virt_to_phys(entry);
> > > > +
> > > > + return scm_legacy_call_atomic(NULL, &desc, NULL);
> > > > +}
> > > > +EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
> > >
> > > This should still be qcom_scm_call.
> >
> > You mean s/scm_legacy_call_atomic/qcom_scm_call/ right?
> >
> > I don't really want to resend the rest of the patches if this last one
> > is the only one that needs an update. This was a semi-RFC anyway so
> > maybe it's fine if the first 5 patches get merged and then I can resend
> > this one? Otherwise I will resend this again next week or so with less
> > diff for this patch.
>
> I'm fine with merging the first 5, but was hoping that Elliot could
> provide either a "Reviewed-by" or at least an "Acked-by" on these.
>
I'll take the silence as I should resend the whole series?
On 3/22/2021 8:36 PM, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-03-07 09:42:45)
>> On Sat 06 Mar 00:18 CST 2021, Stephen Boyd wrote:
>>
>>> Quoting Elliot Berman (2021-03-05 10:18:09)
>>>> On 3/3/2021 10:14 PM, Stephen Boyd wrote:
>>>>> Quoting Elliot Berman (2021-03-03 19:35:08)
>>>>
>>>> > + desc.args[0] = flags;
>>>> > + desc.args[1] = virt_to_phys(entry);
>>>> > +
>>>> > + return scm_legacy_call_atomic(NULL, &desc, NULL);
>>>> > +}
>>>> > +EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
>>>>
>>>> This should still be qcom_scm_call.
>>>
>>> You mean s/scm_legacy_call_atomic/qcom_scm_call/ right?
>>>
>>> I don't really want to resend the rest of the patches if this last one
>>> is the only one that needs an update. This was a semi-RFC anyway so
>>> maybe it's fine if the first 5 patches get merged and then I can resend
>>> this one? Otherwise I will resend this again next week or so with less
>>> diff for this patch.
>>
>> I'm fine with merging the first 5, but was hoping that Elliot could
>> provide either a "Reviewed-by" or at least an "Acked-by" on these.
>>
>
> I'll take the silence as I should resend the whole series?
>
I thought Bjorn was accepting the first 5 already, my apologies.
Patches 1-5:
Acked-by: Elliot Berman <[email protected]>
On Tue 23 Mar 13:27 CDT 2021, Elliot Berman wrote:
> On 3/22/2021 8:36 PM, Stephen Boyd wrote:
> > Quoting Bjorn Andersson (2021-03-07 09:42:45)
> > > On Sat 06 Mar 00:18 CST 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Elliot Berman (2021-03-05 10:18:09)
> > > > > On 3/3/2021 10:14 PM, Stephen Boyd wrote:
> > > > > > Quoting Elliot Berman (2021-03-03 19:35:08)
> > > > >
> > > > > > + desc.args[0] = flags;
> > > > > > + desc.args[1] = virt_to_phys(entry);
> > > > > > +
> > > > > > + return scm_legacy_call_atomic(NULL, &desc, NULL);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(qcom_scm_set_cold_boot_addr);
> > > > >
> > > > > This should still be qcom_scm_call.
> > > >
> > > > You mean s/scm_legacy_call_atomic/qcom_scm_call/ right?
> > > >
> > > > I don't really want to resend the rest of the patches if this last one
> > > > is the only one that needs an update. This was a semi-RFC anyway so
> > > > maybe it's fine if the first 5 patches get merged and then I can resend
> > > > this one? Otherwise I will resend this again next week or so with less
> > > > diff for this patch.
> > >
> > > I'm fine with merging the first 5, but was hoping that Elliot could
> > > provide either a "Reviewed-by" or at least an "Acked-by" on these.
> > >
> >
> > I'll take the silence as I should resend the whole series?
> >
>
> I thought Bjorn was accepting the first 5 already, my apologies.
>
> Patches 1-5:
> Acked-by: Elliot Berman <[email protected]>
Thanks Elliot. I'll pick up the first 5, so please skip these in your
repost, Stephen.
Regards,
Bjorn