SMCCC v1.2 adds a new optional function SMCCC_ARCH_SOC_ID to obtain a
SiP defined SoC identification value. Add support for the same.
Also using the SoC bus infrastructure, let us expose the platform
specific SoC atrributes under sysfs. We also provide custom sysfs for
the vendor ID as JEP-106 bank and identification code.
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/psci/Makefile | 2 +-
drivers/firmware/psci/soc_id.c | 148 +++++++++++++++++++++++++++++++++
include/linux/arm-smccc.h | 5 ++
3 files changed, 154 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/psci/soc_id.c
diff --git a/drivers/firmware/psci/Makefile b/drivers/firmware/psci/Makefile
index 1956b882470f..c0b0c9ca57e4 100644
--- a/drivers/firmware/psci/Makefile
+++ b/drivers/firmware/psci/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
#
-obj-$(CONFIG_ARM_PSCI_FW) += psci.o
+obj-$(CONFIG_ARM_PSCI_FW) += psci.o soc_id.o
obj-$(CONFIG_ARM_PSCI_CHECKER) += psci_checker.o
diff --git a/drivers/firmware/psci/soc_id.c b/drivers/firmware/psci/soc_id.c
new file mode 100644
index 000000000000..820f69dad7f5
--- /dev/null
+++ b/drivers/firmware/psci/soc_id.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Arm Limited
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+
+#define SOCID_JEP106_BANK_IDX_MASK GENMASK(30, 24)
+#define SOCID_JEP106_ID_CODE_MASK GENMASK(23, 16)
+#define SOCID_IMP_DEF_SOC_ID_MASK GENMASK(15, 0)
+#define JEP106_BANK_IDX(x) (u8)(FIELD_GET(SOCID_JEP106_BANK_IDX_MASK, (x)))
+#define JEP106_ID_CODE(x) (u8)(FIELD_GET(SOCID_JEP106_ID_CODE_MASK, (x)))
+#define IMP_DEF_SOC_ID(x) (u16)(FIELD_GET(SOCID_IMP_DEF_SOC_ID_MASK, (x)))
+
+static int soc_id_version;
+static struct soc_device *soc_dev;
+static struct soc_device_attribute *soc_dev_attr;
+
+static int smccc_map_error_codes(unsigned long a0)
+{
+ if (a0 == SMCCC_RET_INVALID_PARAMETER)
+ return -EINVAL;
+ if (a0 == SMCCC_RET_NOT_SUPPORTED)
+ return -EOPNOTSUPP;
+ return 0;
+}
+
+static int smccc_soc_id_support_check(void)
+{
+ struct arm_smccc_res res;
+
+ if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) {
+ pr_err("%s: invalid SMCCC conduit\n", __func__);
+ return -EOPNOTSUPP;
+ }
+
+ arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
+ ARM_SMCCC_ARCH_SOC_ID, &res);
+
+ return smccc_map_error_codes(res.a0);
+}
+
+static ssize_t
+jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%02x\n", JEP106_BANK_IDX(soc_id_version) + 1);
+}
+
+static DEVICE_ATTR_RO(jep106_cont_bank_code);
+
+static ssize_t
+jep106_identification_code_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%02x\n", JEP106_ID_CODE(soc_id_version) & 0x7F);
+}
+
+static DEVICE_ATTR_RO(jep106_identification_code);
+
+static struct attribute *jep106_id_attrs[] = {
+ &dev_attr_jep106_cont_bank_code.attr,
+ &dev_attr_jep106_identification_code.attr,
+ NULL
+};
+
+ATTRIBUTE_GROUPS(jep106_id);
+
+static int __init smccc_soc_init(void)
+{
+ struct device *dev;
+ int ret, soc_id_rev;
+ struct arm_smccc_res res;
+ static char soc_id_str[8], soc_id_rev_str[12];
+
+ if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
+ return 0;
+
+ ret = smccc_soc_id_support_check();
+ if (ret)
+ return ret;
+
+ arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
+
+ ret = smccc_map_error_codes(res.a0);
+ if (ret)
+ return ret;
+
+ soc_id_version = res.a0;
+
+ arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
+
+ ret = smccc_map_error_codes(res.a0);
+ if (ret)
+ return ret;
+
+ soc_id_rev = res.a0;
+
+ soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+ if (!soc_dev_attr)
+ return -ENOMEM;
+
+ sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
+ sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
+
+ soc_dev_attr->soc_id = soc_id_str;
+ soc_dev_attr->revision = soc_id_rev_str;
+
+ soc_dev = soc_device_register(soc_dev_attr);
+ if (IS_ERR(soc_dev)) {
+ ret = PTR_ERR(soc_dev);
+ goto free_soc;
+ }
+
+ dev = soc_device_to_device(soc_dev);
+
+ ret = devm_device_add_groups(dev, jep106_id_groups);
+ if (ret) {
+ dev_err(dev, "sysfs create failed: %d\n", ret);
+ goto unregister_soc;
+ }
+
+ pr_info("SMCCC SoC ID: %s Revision %s\n", soc_dev_attr->soc_id,
+ soc_dev_attr->revision);
+
+ return 0;
+
+unregister_soc:
+ soc_device_unregister(soc_dev);
+free_soc:
+ kfree(soc_dev_attr);
+ return ret;
+}
+module_init(smccc_soc_init);
+
+static void __exit smccc_soc_exit(void)
+{
+ if (soc_dev)
+ soc_device_unregister(soc_dev);
+ kfree(soc_dev_attr);
+}
+module_exit(smccc_soc_exit);
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index d6b0f4acc707..04414fc2000f 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -68,6 +68,11 @@
ARM_SMCCC_SMC_32, \
0, 1)
+#define ARM_SMCCC_ARCH_SOC_ID \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_32, \
+ 0, 2)
+
#define ARM_SMCCC_ARCH_WORKAROUND_1 \
ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
ARM_SMCCC_SMC_32, \
--
2.17.1
On 30/04/2020 12:48, Sudeep Holla wrote:
> SMCCC v1.2 adds a new optional function SMCCC_ARCH_SOC_ID to obtain a
> SiP defined SoC identification value. Add support for the same.
>
> Also using the SoC bus infrastructure, let us expose the platform
> specific SoC atrributes under sysfs. We also provide custom sysfs for
> the vendor ID as JEP-106 bank and identification code.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/psci/Makefile | 2 +-
> drivers/firmware/psci/soc_id.c | 148 +++++++++++++++++++++++++++++++++
> include/linux/arm-smccc.h | 5 ++
> 3 files changed, 154 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/psci/soc_id.c
>
> diff --git a/drivers/firmware/psci/Makefile b/drivers/firmware/psci/Makefile
> index 1956b882470f..c0b0c9ca57e4 100644
> --- a/drivers/firmware/psci/Makefile
> +++ b/drivers/firmware/psci/Makefile
> @@ -1,4 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> #
> -obj-$(CONFIG_ARM_PSCI_FW) += psci.o
> +obj-$(CONFIG_ARM_PSCI_FW) += psci.o soc_id.o
> obj-$(CONFIG_ARM_PSCI_CHECKER) += psci_checker.o
> diff --git a/drivers/firmware/psci/soc_id.c b/drivers/firmware/psci/soc_id.c
> new file mode 100644
> index 000000000000..820f69dad7f5
> --- /dev/null
> +++ b/drivers/firmware/psci/soc_id.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Arm Limited
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +
> +#define SOCID_JEP106_BANK_IDX_MASK GENMASK(30, 24)
> +#define SOCID_JEP106_ID_CODE_MASK GENMASK(23, 16)
> +#define SOCID_IMP_DEF_SOC_ID_MASK GENMASK(15, 0)
> +#define JEP106_BANK_IDX(x) (u8)(FIELD_GET(SOCID_JEP106_BANK_IDX_MASK, (x)))
> +#define JEP106_ID_CODE(x) (u8)(FIELD_GET(SOCID_JEP106_ID_CODE_MASK, (x)))
> +#define IMP_DEF_SOC_ID(x) (u16)(FIELD_GET(SOCID_IMP_DEF_SOC_ID_MASK, (x)))
> +
> +static int soc_id_version;
> +static struct soc_device *soc_dev;
> +static struct soc_device_attribute *soc_dev_attr;
> +
> +static int smccc_map_error_codes(unsigned long a0)
> +{
> + if (a0 == SMCCC_RET_INVALID_PARAMETER)
> + return -EINVAL;
> + if (a0 == SMCCC_RET_NOT_SUPPORTED)
> + return -EOPNOTSUPP;
> + return 0;
It seems odd to special case just those errors. While they are the only
errors we expect, any result with the high bit set is an error (arguably
a bug in the firmware) so should really cause an error return.
> +}
> +
> +static int smccc_soc_id_support_check(void)
> +{
> + struct arm_smccc_res res;
> +
> + if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) {
> + pr_err("%s: invalid SMCCC conduit\n", __func__);
> + return -EOPNOTSUPP;
> + }
> +
> + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> + ARM_SMCCC_ARCH_SOC_ID, &res);
> +
> + return smccc_map_error_codes(res.a0);
> +}
> +
> +static ssize_t
> +jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%02x\n", JEP106_BANK_IDX(soc_id_version) + 1);
> +}
> +
> +static DEVICE_ATTR_RO(jep106_cont_bank_code);
> +
> +static ssize_t
> +jep106_identification_code_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%02x\n", JEP106_ID_CODE(soc_id_version) & 0x7F);
It seems odd to have the mask defined to include a bit that is then
always masked off. From the spec I presume this is a parity bit, but it
would be good to have a comment explaining this.
> +}
> +
> +static DEVICE_ATTR_RO(jep106_identification_code);
> +
> +static struct attribute *jep106_id_attrs[] = {
> + &dev_attr_jep106_cont_bank_code.attr,
> + &dev_attr_jep106_identification_code.attr,
> + NULL
> +};
> +
> +ATTRIBUTE_GROUPS(jep106_id);
> +
> +static int __init smccc_soc_init(void)
> +{
> + struct device *dev;
> + int ret, soc_id_rev;
> + struct arm_smccc_res res;
> + static char soc_id_str[8], soc_id_rev_str[12];
> +
> + if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
> + return 0;
NIT: Do we actually need to check the version here - or would probing
ARM_SMCCC_ARCH_FEATURES_FUNC_ID as is done below sufficient? I'm not
aware of this relying on any new semantics that v1.2 added.
> +
> + ret = smccc_soc_id_support_check();
> + if (ret)
> + return ret;
This seems odd - if the version is <v1.2 then we return 0. But if it's
>=1.2 but doesn't support SOC_ID then it's an error return?
> +
> + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
> +
> + ret = smccc_map_error_codes(res.a0);
> + if (ret)
> + return ret;
> +
> + soc_id_version = res.a0;
> +
> + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
> +
> + ret = smccc_map_error_codes(res.a0);
> + if (ret)
> + return ret;
> +
> + soc_id_rev = res.a0;
> +
> + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> + if (!soc_dev_attr)
> + return -ENOMEM;
> +
> + sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> + sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> +
> + soc_dev_attr->soc_id = soc_id_str;
> + soc_dev_attr->revision = soc_id_rev_str;
> +
> + soc_dev = soc_device_register(soc_dev_attr);
> + if (IS_ERR(soc_dev)) {
> + ret = PTR_ERR(soc_dev);
> + goto free_soc;
> + }
> +
> + dev = soc_device_to_device(soc_dev);
> +
> + ret = devm_device_add_groups(dev, jep106_id_groups);
> + if (ret) {
> + dev_err(dev, "sysfs create failed: %d\n", ret);
> + goto unregister_soc;
> + }
> +
> + pr_info("SMCCC SoC ID: %s Revision %s\n", soc_dev_attr->soc_id,
> + soc_dev_attr->revision);
> +
> + return 0;
> +
> +unregister_soc:
> + soc_device_unregister(soc_dev);
> +free_soc:
> + kfree(soc_dev_attr);
> + return ret;
> +}
> +module_init(smccc_soc_init);
> +
> +static void __exit smccc_soc_exit(void)
> +{
> + if (soc_dev)
> + soc_device_unregister(soc_dev);
> + kfree(soc_dev_attr);
> +}
> +module_exit(smccc_soc_exit);
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index d6b0f4acc707..04414fc2000f 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -68,6 +68,11 @@
> ARM_SMCCC_SMC_32, \
> 0, 1)
>
> +#define ARM_SMCCC_ARCH_SOC_ID \
> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_SMC_32, \
> + 0, 2)
> +
> #define ARM_SMCCC_ARCH_WORKAROUND_1 \
> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> ARM_SMCCC_SMC_32, \
>
On 30/04/2020 12:48, Sudeep Holla wrote:
> +static int __init smccc_soc_init(void)
> +{
> + struct device *dev;
> + int ret, soc_id_rev;
> + struct arm_smccc_res res;
> + static char soc_id_str[8], soc_id_rev_str[12];
> +
> + if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
> + return 0;
> +
> + ret = smccc_soc_id_support_check();
> + if (ret)
> + return ret;
> +
> + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
> +
> + ret = smccc_map_error_codes(res.a0);
> + if (ret)
> + return ret;
> +
> + soc_id_version = res.a0;
> +
> + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
> +
> + ret = smccc_map_error_codes(res.a0);
> + if (ret)
> + return ret;
> +
> + soc_id_rev = res.a0;
> +
> + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> + if (!soc_dev_attr)
> + return -ENOMEM;
> +
> + sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> + sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> +
> + soc_dev_attr->soc_id = soc_id_str;
> + soc_dev_attr->revision = soc_id_rev_str;
> +
> + soc_dev = soc_device_register(soc_dev_attr);
> + if (IS_ERR(soc_dev)) {
> + ret = PTR_ERR(soc_dev);
> + goto free_soc;
> + }
> +
> + dev = soc_device_to_device(soc_dev);
> +
Just wondering, what about if the platform already had a SoC driver -
now it could have another one, such that we may have multiple sysfs soc
devices, right?
Thanks,
John
> + ret = devm_device_add_groups(dev, jep106_id_groups);
> + if (ret) {
> + dev_err(dev, "sysfs create failed: %d\n", ret);
> + goto unregister_soc;
> + }
> +
> + pr_info("SMCCC SoC ID: %s Revision %s\n", soc_dev_attr->soc_id,
> + soc_dev_attr->revision);
> +
> + return 0;
Hi Steve,
Thanks for taking a reviewing these patches.
On Fri, May 01, 2020 at 04:07:39PM +0100, Steven Price wrote:
> On 30/04/2020 12:48, Sudeep Holla wrote:
> > SMCCC v1.2 adds a new optional function SMCCC_ARCH_SOC_ID to obtain a
> > SiP defined SoC identification value. Add support for the same.
> >
> > Also using the SoC bus infrastructure, let us expose the platform
> > specific SoC atrributes under sysfs. We also provide custom sysfs for
> > the vendor ID as JEP-106 bank and identification code.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/firmware/psci/Makefile | 2 +-
> > drivers/firmware/psci/soc_id.c | 148 +++++++++++++++++++++++++++++++++
> > include/linux/arm-smccc.h | 5 ++
> > 3 files changed, 154 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/firmware/psci/soc_id.c
> >
> > diff --git a/drivers/firmware/psci/Makefile b/drivers/firmware/psci/Makefile
> > index 1956b882470f..c0b0c9ca57e4 100644
> > --- a/drivers/firmware/psci/Makefile
> > +++ b/drivers/firmware/psci/Makefile
> > @@ -1,4 +1,4 @@
> > # SPDX-License-Identifier: GPL-2.0
> > #
> > -obj-$(CONFIG_ARM_PSCI_FW) += psci.o
> > +obj-$(CONFIG_ARM_PSCI_FW) += psci.o soc_id.o
> > obj-$(CONFIG_ARM_PSCI_CHECKER) += psci_checker.o
> > diff --git a/drivers/firmware/psci/soc_id.c b/drivers/firmware/psci/soc_id.c
> > new file mode 100644
> > index 000000000000..820f69dad7f5
> > --- /dev/null
> > +++ b/drivers/firmware/psci/soc_id.c
> > @@ -0,0 +1,148 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2020 Arm Limited
> > + */
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/sys_soc.h>
> > +
> > +#define SOCID_JEP106_BANK_IDX_MASK GENMASK(30, 24)
> > +#define SOCID_JEP106_ID_CODE_MASK GENMASK(23, 16)
> > +#define SOCID_IMP_DEF_SOC_ID_MASK GENMASK(15, 0)
> > +#define JEP106_BANK_IDX(x) (u8)(FIELD_GET(SOCID_JEP106_BANK_IDX_MASK, (x)))
> > +#define JEP106_ID_CODE(x) (u8)(FIELD_GET(SOCID_JEP106_ID_CODE_MASK, (x)))
> > +#define IMP_DEF_SOC_ID(x) (u16)(FIELD_GET(SOCID_IMP_DEF_SOC_ID_MASK, (x)))
> > +
> > +static int soc_id_version;
> > +static struct soc_device *soc_dev;
> > +static struct soc_device_attribute *soc_dev_attr;
> > +
> > +static int smccc_map_error_codes(unsigned long a0)
> > +{
> > + if (a0 == SMCCC_RET_INVALID_PARAMETER)
> > + return -EINVAL;
> > + if (a0 == SMCCC_RET_NOT_SUPPORTED)
> > + return -EOPNOTSUPP;
> > + return 0;
>
> It seems odd to special case just those errors. While they are the only
> errors we expect, any result with the high bit set is an error (arguably a
> bug in the firmware) so should really cause an error return.
>
I agree and happy to change it. I too thought about the same for a while and
they left it for review time to finalise ????
> > +}
> > +
> > +static int smccc_soc_id_support_check(void)
> > +{
> > + struct arm_smccc_res res;
> > +
> > + if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) {
> > + pr_err("%s: invalid SMCCC conduit\n", __func__);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> > + ARM_SMCCC_ARCH_SOC_ID, &res);
> > +
> > + return smccc_map_error_codes(res.a0);
> > +}
> > +
> > +static ssize_t
> > +jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + return sprintf(buf, "%02x\n", JEP106_BANK_IDX(soc_id_version) + 1);
> > +}
> > +
> > +static DEVICE_ATTR_RO(jep106_cont_bank_code);
> > +
> > +static ssize_t
> > +jep106_identification_code_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + return sprintf(buf, "%02x\n", JEP106_ID_CODE(soc_id_version) & 0x7F);
>
> It seems odd to have the mask defined to include a bit that is then always
> masked off. From the spec I presume this is a parity bit, but it would be
> good to have a comment explaining this.
>
Sure, actually I can to make it part of the macro itself and add a note
there.
> > +}
> > +
> > +static DEVICE_ATTR_RO(jep106_identification_code);
> > +
> > +static struct attribute *jep106_id_attrs[] = {
> > + &dev_attr_jep106_cont_bank_code.attr,
> > + &dev_attr_jep106_identification_code.attr,
> > + NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(jep106_id);
> > +
> > +static int __init smccc_soc_init(void)
> > +{
> > + struct device *dev;
> > + int ret, soc_id_rev;
> > + struct arm_smccc_res res;
> > + static char soc_id_str[8], soc_id_rev_str[12];
> > +
> > + if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
> > + return 0;
>
> NIT: Do we actually need to check the version here - or would probing
> ARM_SMCCC_ARCH_FEATURES_FUNC_ID as is done below sufficient? I'm not aware
> of this relying on any new semantics that v1.2 added.
>
It should be sufficient, but I am trying to avoid raising error if it is
not SMCCC v1.2+, hence the return 0.
> > +
> > + ret = smccc_soc_id_support_check();
> > + if (ret)
> > + return ret;
>
> This seems odd - if the version is <v1.2 then we return 0. But if it's >=1.2
> but doesn't support SOC_ID then it's an error return?
>
You are right, I see that now. I can flag a note/info and return 0.
--
Regards,
Sudeep
On Fri, May 01, 2020 at 04:25:27PM +0100, John Garry wrote:
> On 30/04/2020 12:48, Sudeep Holla wrote:
> > +static int __init smccc_soc_init(void)
> > +{
> > + struct device *dev;
> > + int ret, soc_id_rev;
> > + struct arm_smccc_res res;
> > + static char soc_id_str[8], soc_id_rev_str[12];
> > +
> > + if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
> > + return 0;
> > +
> > + ret = smccc_soc_id_support_check();
> > + if (ret)
> > + return ret;
> > +
> > + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
> > +
> > + ret = smccc_map_error_codes(res.a0);
> > + if (ret)
> > + return ret;
> > +
> > + soc_id_version = res.a0;
> > +
> > + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
> > +
> > + ret = smccc_map_error_codes(res.a0);
> > + if (ret)
> > + return ret;
> > +
> > + soc_id_rev = res.a0;
> > +
> > + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > + if (!soc_dev_attr)
> > + return -ENOMEM;
> > +
> > + sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> > + sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> > +
> > + soc_dev_attr->soc_id = soc_id_str;
> > + soc_dev_attr->revision = soc_id_rev_str;
> > +
> > + soc_dev = soc_device_register(soc_dev_attr);
> > + if (IS_ERR(soc_dev)) {
> > + ret = PTR_ERR(soc_dev);
> > + goto free_soc;
> > + }
> > +
> > + dev = soc_device_to_device(soc_dev);
> > +
>
> Just wondering, what about if the platform already had a SoC driver - now it
> could have another one, such that we may have multiple sysfs soc devices,
> right?
>
Yes I had a quick look at that.
1. Such platform has option not to implement this SOC_ID if it doesn't
really require it.
2. If the firmware starts implementing it on some variants, then we can
distinguish them with compatibles and blacklist them from the other
SoC driver if having both is an issue
3. SoC bus layer supports adding multiple SoC ID driver and it may show
up as /sys/devices/soc<n> which may or may not be fine. But this
happens only if neither [1] nor [2] is done. I am happy to see if there's
any solution for this. Any suggestions ?
--
Regards,
Sudeep
On 01/05/2020 17:05, Sudeep Holla wrote:
> On Fri, May 01, 2020 at 04:25:27PM +0100, John Garry wrote:
>> On 30/04/2020 12:48, Sudeep Holla wrote:
>>> +static int __init smccc_soc_init(void)
>>> +{
>>> + struct device *dev;
>>> + int ret, soc_id_rev;
>>> + struct arm_smccc_res res;
>>> + static char soc_id_str[8], soc_id_rev_str[12];
>>> +
>>> + if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
>>> + return 0;
>>> +
>>> + ret = smccc_soc_id_support_check();
>>> + if (ret)
>>> + return ret;
>>> +
>>> + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
>>> +
>>> + ret = smccc_map_error_codes(res.a0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + soc_id_version = res.a0;
>>> +
>>> + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
>>> +
>>> + ret = smccc_map_error_codes(res.a0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + soc_id_rev = res.a0;
>>> +
>>> + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
>>> + if (!soc_dev_attr)
>>> + return -ENOMEM;
>>> +
>>> + sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
>>> + sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
>>> +
>>> + soc_dev_attr->soc_id = soc_id_str;
>>> + soc_dev_attr->revision = soc_id_rev_str;
>>> +
>>> + soc_dev = soc_device_register(soc_dev_attr);
>>> + if (IS_ERR(soc_dev)) {
>>> + ret = PTR_ERR(soc_dev);
>>> + goto free_soc;
>>> + }
>>> +
>>> + dev = soc_device_to_device(soc_dev);
>>> +
>>
>> Just wondering, what about if the platform already had a SoC driver - now it
>> could have another one, such that we may have multiple sysfs soc devices,
>> right?
>>
>
> Yes I had a quick look at that.
>
> 1. Such platform has option not to implement this SOC_ID if it doesn't
> really require it.
True
>
> 2. If the firmware starts implementing it on some variants, then we can
> distinguish them with compatibles and blacklist them from the other
> SoC driver if having both is an issue
>
> 3. SoC bus layer supports adding multiple SoC ID driver and it may show
> up as /sys/devices/soc<n> which may or may not be fine.
Yeah, it's this scenario which I'm concerned about, where some userspace
expects, for example, soc0 to have a soc id from a known, expected list,
and now may get something else. However it could be argued then that
userspace is just too fragile then and there is no read problem here.
But this
> happens only if neither [1] nor [2] is done. I am happy to see if there's
> any solution for this. Any suggestions ?
Not sure, but taking a slight deviation, maybe a way could be found to
supplement this dev attribute info to other ARM soc drivers.
Cheers,
John