For some of the Qualcomm SoC's, it is possible that
some of the fuse regions or entire qfprom region is
protected from non-secure access. In such situations,
linux will have to use secure calls to read the region.
With that motivation, add secure qfprom driver. Ensuring
the address to read is word aligned since our secure I/O
only supports word size I/O.
Signed-off-by: Komal Bajaj <[email protected]>
---
drivers/nvmem/Kconfig | 12 ++++
drivers/nvmem/Makefile | 2 +
drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++
3 files changed, 130 insertions(+)
create mode 100644 drivers/nvmem/sec-qfprom.c
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index b291b27048c7..2b0dd73d899e 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -216,6 +216,18 @@ config NVMEM_QCOM_QFPROM
This driver can also be built as a module. If so, the module
will be called nvmem_qfprom.
+config NVMEM_QCOM_SEC_QFPROM
+ tristate "QCOM SECURE QFPROM Support"
+ depends on ARCH_QCOM || COMPILE_TEST
+ depends on HAS_IOMEM
+ select QCOM_SCM
+ help
+ Say y here to enable secure QFPROM support. The secure QFPROM provides access
+ functions for QFPROM data to rest of the drivers via nvmem interface.
+
+ This driver can also be built as a module. If so, the module will be called
+ nvmem_sec_qfprom.
+
config NVMEM_RAVE_SP_EEPROM
tristate "Rave SP EEPROM Support"
depends on RAVE_SP_CORE
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index f82431ec8aef..4b4bf5880488 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP) += nvmem-nintendo-otp.o
nvmem-nintendo-otp-y := nintendo-otp.o
obj-$(CONFIG_NVMEM_QCOM_QFPROM) += nvmem_qfprom.o
nvmem_qfprom-y := qfprom.o
+obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM) += nvmem_sec_qfprom.o
+nvmem_sec_qfprom-y := sec-qfprom.o
obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM) += nvmem-rave-sp-eeprom.o
nvmem-rave-sp-eeprom-y := rave-sp-eeprom.o
obj-$(CONFIG_NVMEM_RMEM) += nvmem-rmem.o
diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
new file mode 100644
index 000000000000..47b2c71593dd
--- /dev/null
+++ b/drivers/nvmem/sec-qfprom.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/nvmem-provider.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+
+
+/**
+ * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
+ *
+ * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
+ * @dev: qfprom device structure.
+ */
+struct sec_qfprom_priv {
+ phys_addr_t qfpseccorrected;
+ struct device *dev;
+};
+
+static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
+{
+ struct sec_qfprom_priv *priv = context;
+ u8 *val = _val;
+ u8 *tmp;
+ u32 read_val;
+ unsigned int i;
+
+ for (i = 0; i < bytes; i++, reg++) {
+ if (i == 0 || reg % 4 == 0) {
+ if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) {
+ dev_err(priv->dev, "Couldn't access fuse register\n");
+ return -EINVAL;
+ }
+ tmp = (u8 *)&read_val;
+ }
+
+ val[i] = tmp[reg & 3];
+ }
+
+ return 0;
+}
+
+static void sec_qfprom_runtime_disable(void *data)
+{
+ pm_runtime_disable(data);
+}
+
+static int sec_qfprom_probe(struct platform_device *pdev)
+{
+ struct nvmem_config econfig = {
+ .name = "sec-qfprom",
+ .stride = 1,
+ .word_size = 1,
+ .id = NVMEM_DEVID_AUTO,
+ .reg_read = sec_qfprom_reg_read,
+ };
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct nvmem_device *nvmem;
+ struct sec_qfprom_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->qfpseccorrected = res->start;
+ if (!priv->qfpseccorrected)
+ return -ENOMEM;
+
+ econfig.size = resource_size(res);
+ econfig.dev = dev;
+ econfig.priv = priv;
+
+ priv->dev = dev;
+
+ pm_runtime_enable(dev);
+ ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev);
+ if (ret)
+ return ret;
+
+ nvmem = devm_nvmem_register(dev, &econfig);
+
+ return PTR_ERR_OR_ZERO(nvmem);
+}
+
+static const struct of_device_id sec_qfprom_of_match[] = {
+ { .compatible = "qcom,sec-qfprom",},
+ {/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
+
+static struct platform_driver qfprom_driver = {
+ .probe = sec_qfprom_probe,
+ .driver = {
+ .name = "qcom,sec_qfprom",
+ .of_match_table = sec_qfprom_of_match,
+ },
+};
+module_platform_driver(qfprom_driver);
+MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
+MODULE_LICENSE("GPL v2");
--
2.40.1
On 23.06.2023 16:18, Komal Bajaj wrote:
> For some of the Qualcomm SoC's, it is possible that
> some of the fuse regions or entire qfprom region is
> protected from non-secure access. In such situations,
> linux will have to use secure calls to read the region.
> With that motivation, add secure qfprom driver. Ensuring
> the address to read is word aligned since our secure I/O
> only supports word size I/O.
>
> Signed-off-by: Komal Bajaj <[email protected]>
> ---
> drivers/nvmem/Kconfig | 12 ++++
> drivers/nvmem/Makefile | 2 +
> drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 130 insertions(+)
> create mode 100644 drivers/nvmem/sec-qfprom.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index b291b27048c7..2b0dd73d899e 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -216,6 +216,18 @@ config NVMEM_QCOM_QFPROM
> This driver can also be built as a module. If so, the module
> will be called nvmem_qfprom.
>
> +config NVMEM_QCOM_SEC_QFPROM
> + tristate "QCOM SECURE QFPROM Support"
> + depends on ARCH_QCOM || COMPILE_TEST
> + depends on HAS_IOMEM
> + select QCOM_SCM
You also need OF
> + help
> + Say y here to enable secure QFPROM support. The secure QFPROM provides access
> + functions for QFPROM data to rest of the drivers via nvmem interface.
> +
> + This driver can also be built as a module. If so, the module will be called
> + nvmem_sec_qfprom.
> +
> config NVMEM_RAVE_SP_EEPROM
> tristate "Rave SP EEPROM Support"
> depends on RAVE_SP_CORE
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index f82431ec8aef..4b4bf5880488 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP) += nvmem-nintendo-otp.o
> nvmem-nintendo-otp-y := nintendo-otp.o
> obj-$(CONFIG_NVMEM_QCOM_QFPROM) += nvmem_qfprom.o
> nvmem_qfprom-y := qfprom.o
> +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM) += nvmem_sec_qfprom.o
> +nvmem_sec_qfprom-y := sec-qfprom.o
> obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM) += nvmem-rave-sp-eeprom.o
> nvmem-rave-sp-eeprom-y := rave-sp-eeprom.o
> obj-$(CONFIG_NVMEM_RMEM) += nvmem-rmem.o
> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
> new file mode 100644
> index 000000000000..47b2c71593dd
> --- /dev/null
> +++ b/drivers/nvmem/sec-qfprom.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +
> +
> +/**
> + * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
> + *
> + * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
> + * @dev: qfprom device structure.
> + */
> +struct sec_qfprom_priv {
> + phys_addr_t qfpseccorrected;
> + struct device *dev;
> +};
> +
> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
> +{
> + struct sec_qfprom_priv *priv = context;
> + u8 *val = _val;
> + u8 *tmp;
> + u32 read_val;
> + unsigned int i;
Please sort this to look like a reverse-Christmas-tree
> +
> + for (i = 0; i < bytes; i++, reg++) {
> + if (i == 0 || reg % 4 == 0) {
> + if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) {
> + dev_err(priv->dev, "Couldn't access fuse register\n");
> + return -EINVAL;
> + }
> + tmp = (u8 *)&read_val;
> + }
I don't understand this read-4-times dance.. qcom_scm_io_readl() reads
u32, so this should be as simple as:
val[i + 0] = FIELD_GET(GENMASK(7, 0), reg);
val[i + 1] = ..
val[i + 2] = ..
val[i + 3] = ..
> +
> + val[i] = tmp[reg & 3];
> + }
> +
> + return 0;
> +}
> +
> +static void sec_qfprom_runtime_disable(void *data)
> +{
> + pm_runtime_disable(data);
> +}
> +
> +static int sec_qfprom_probe(struct platform_device *pdev)
> +{
> + struct nvmem_config econfig = {
> + .name = "sec-qfprom",
> + .stride = 1,
> + .word_size = 1,
> + .id = NVMEM_DEVID_AUTO,
> + .reg_read = sec_qfprom_reg_read,
> + };
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct nvmem_device *nvmem;
> + struct sec_qfprom_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->qfpseccorrected = res->start;
> + if (!priv->qfpseccorrected)
> + return -ENOMEM;
While it works all the same, I think checking if(!res) would be more
logical
Also, ENOMEM seems weird here.. Perhaps EINVAL?
> +
> + econfig.size = resource_size(res);
> + econfig.dev = dev;
> + econfig.priv = priv;
> +
> + priv->dev = dev;
> +
> + pm_runtime_enable(dev);
> + ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev);
> + if (ret)
> + return ret;
Wouldn't devm_pm_runtime_enable() take care of this? Or do we need
for this block to be always-powered?
> +
> + nvmem = devm_nvmem_register(dev, &econfig);
> +
> + return PTR_ERR_OR_ZERO(nvmem);
> +}
> +
> +static const struct of_device_id sec_qfprom_of_match[] = {
> + { .compatible = "qcom,sec-qfprom",},
The comma inside is unnecessary, replacing it with a space would also
make the whitespacing match
> + {/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
> +
> +static struct platform_driver qfprom_driver = {
> + .probe = sec_qfprom_probe,
> + .driver = {
> + .name = "qcom,sec_qfprom",
Commas in driver names are rather.. rare? Let's make it qcom_
> + .of_match_table = sec_qfprom_of_match,
> + },
> +};
> +module_platform_driver(qfprom_driver);
> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
> +MODULE_LICENSE("GPL v2");
Please run scripts/checkpatch.pl on your patches before sending.
Konrad
Hi Komal,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.4-rc7 next-20230623]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Komal-Bajaj/dt-bindings-nvmem-sec-qfprom-Add-bindings-for-secure-qfprom/20230623-222054
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230623141806.13388-4-quic_kbajaj%40quicinc.com
patch subject: [PATCH v4 3/6] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230624/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230624/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/nvmem/sec-qfprom.c:31: warning: expecting prototype for struct sec_sec_qfprom_priv. Prototype was for struct sec_qfprom_priv instead
vim +31 drivers/nvmem/sec-qfprom.c
20
21
22 /**
23 * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
24 *
25 * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
26 * @dev: qfprom device structure.
27 */
28 struct sec_qfprom_priv {
29 phys_addr_t qfpseccorrected;
30 struct device *dev;
> 31 };
32
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 6/23/2023 8:23 PM, Konrad Dybcio wrote:
> On 23.06.2023 16:18, Komal Bajaj wrote:
>> For some of the Qualcomm SoC's, it is possible that
>> some of the fuse regions or entire qfprom region is
>> protected from non-secure access. In such situations,
>> linux will have to use secure calls to read the region.
>> With that motivation, add secure qfprom driver. Ensuring
>> the address to read is word aligned since our secure I/O
>> only supports word size I/O.
>>
>> Signed-off-by: Komal Bajaj <[email protected]>
>> ---
>> drivers/nvmem/Kconfig | 12 ++++
>> drivers/nvmem/Makefile | 2 +
>> drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 130 insertions(+)
>> create mode 100644 drivers/nvmem/sec-qfprom.c
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index b291b27048c7..2b0dd73d899e 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -216,6 +216,18 @@ config NVMEM_QCOM_QFPROM
>> This driver can also be built as a module. If so, the module
>> will be called nvmem_qfprom.
>>
>> +config NVMEM_QCOM_SEC_QFPROM
>> + tristate "QCOM SECURE QFPROM Support"
>> + depends on ARCH_QCOM || COMPILE_TEST
>> + depends on HAS_IOMEM
>> + select QCOM_SCM
> You also need OF
Noted.
>
>> + help
>> + Say y here to enable secure QFPROM support. The secure QFPROM provides access
>> + functions for QFPROM data to rest of the drivers via nvmem interface.
>> +
>> + This driver can also be built as a module. If so, the module will be called
>> + nvmem_sec_qfprom.
>> +
>> config NVMEM_RAVE_SP_EEPROM
>> tristate "Rave SP EEPROM Support"
>> depends on RAVE_SP_CORE
>> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
>> index f82431ec8aef..4b4bf5880488 100644
>> --- a/drivers/nvmem/Makefile
>> +++ b/drivers/nvmem/Makefile
>> @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP) += nvmem-nintendo-otp.o
>> nvmem-nintendo-otp-y := nintendo-otp.o
>> obj-$(CONFIG_NVMEM_QCOM_QFPROM) += nvmem_qfprom.o
>> nvmem_qfprom-y := qfprom.o
>> +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM) += nvmem_sec_qfprom.o
>> +nvmem_sec_qfprom-y := sec-qfprom.o
>> obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM) += nvmem-rave-sp-eeprom.o
>> nvmem-rave-sp-eeprom-y := rave-sp-eeprom.o
>> obj-$(CONFIG_NVMEM_RMEM) += nvmem-rmem.o
>> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
>> new file mode 100644
>> index 000000000000..47b2c71593dd
>> --- /dev/null
>> +++ b/drivers/nvmem/sec-qfprom.c
>> @@ -0,0 +1,116 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/firmware/qcom/qcom_scm.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/property.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +
>> +/**
>> + * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
>> + *
>> + * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
>> + * @dev: qfprom device structure.
>> + */
>> +struct sec_qfprom_priv {
>> + phys_addr_t qfpseccorrected;
>> + struct device *dev;
>> +};
>> +
>> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
>> +{
>> + struct sec_qfprom_priv *priv = context;
>> + u8 *val = _val;
>> + u8 *tmp;
>> + u32 read_val;
>> + unsigned int i;
> Please sort this to look like a reverse-Christmas-tree
Okay, will sort it in that way.
>
>
>> +
>> + for (i = 0; i < bytes; i++, reg++) {
>> + if (i == 0 || reg % 4 == 0) {
>> + if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) {
>> + dev_err(priv->dev, "Couldn't access fuse register\n");
>> + return -EINVAL;
>> + }
>> + tmp = (u8 *)&read_val;
>> + }
> I don't understand this read-4-times dance.. qcom_scm_io_readl() reads
> u32, so this should be as simple as:
>
> val[i + 0] = FIELD_GET(GENMASK(7, 0), reg);
> val[i + 1] = ..
> val[i + 2] = ..
> val[i + 3] = ..
Won't it get too complex, I type-casted 32-bit read_val into u8 pointer,
so that I can
easily use it for the byte-level access of read_val's value.
Doing the way that you mentioned would be something like below -
val[i] = FIELD_GET(GENMASK((reg&3)*8+7, (reg&3)*8), read_val);
Thanks
Komal
>
>> +
>> + val[i] = tmp[reg & 3];
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void sec_qfprom_runtime_disable(void *data)
>> +{
>> + pm_runtime_disable(data);
>> +}
>> +
>> +static int sec_qfprom_probe(struct platform_device *pdev)
>> +{
>> + struct nvmem_config econfig = {
>> + .name = "sec-qfprom",
>> + .stride = 1,
>> + .word_size = 1,
>> + .id = NVMEM_DEVID_AUTO,
>> + .reg_read = sec_qfprom_reg_read,
>> + };
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct nvmem_device *nvmem;
>> + struct sec_qfprom_priv *priv;
>> + int ret;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + priv->qfpseccorrected = res->start;
>> + if (!priv->qfpseccorrected)
>> + return -ENOMEM;
> While it works all the same, I think checking if(!res) would be more
> logical
>
> Also, ENOMEM seems weird here.. Perhaps EINVAL?
>
>> +
>> + econfig.size = resource_size(res);
>> + econfig.dev = dev;
>> + econfig.priv = priv;
>> +
>> + priv->dev = dev;
>> +
>> + pm_runtime_enable(dev);
>> + ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev);
>> + if (ret)
>> + return ret;
> Wouldn't devm_pm_runtime_enable() take care of this? Or do we need
> for this block to be always-powered?
>
>> +
>> + nvmem = devm_nvmem_register(dev, &econfig);
>> +
>> + return PTR_ERR_OR_ZERO(nvmem);
>> +}
>> +
>> +static const struct of_device_id sec_qfprom_of_match[] = {
>> + { .compatible = "qcom,sec-qfprom",},
> The comma inside is unnecessary, replacing it with a space would also
> make the whitespacing match
>
>> + {/* sentinel */},
>> +};
>> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
>> +
>> +static struct platform_driver qfprom_driver = {
>> + .probe = sec_qfprom_probe,
>> + .driver = {
>> + .name = "qcom,sec_qfprom",
> Commas in driver names are rather.. rare? Let's make it qcom_
>
>> + .of_match_table = sec_qfprom_of_match,
>> + },
>> +};
>> +module_platform_driver(qfprom_driver);
>> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
>> +MODULE_LICENSE("GPL v2");
> Please run scripts/checkpatch.pl on your patches before sending.
>
> Konrad
On Fri, Jun 23, 2023 at 07:48:03PM +0530, Komal Bajaj wrote:
> For some of the Qualcomm SoC's, it is possible that
> some of the fuse regions or entire qfprom region is
> protected from non-secure access. In such situations,
> linux will have to use secure calls to read the region.
Linux
> With that motivation, add secure qfprom driver. Ensuring
> the address to read is word aligned since our secure I/O
> only supports word size I/O.
What do you mean with this last sentence? I don't see anything in your
patch demanding this. Perhaps just drop this sentence?
>
> Signed-off-by: Komal Bajaj <[email protected]>
> ---
> drivers/nvmem/Kconfig | 12 ++++
> drivers/nvmem/Makefile | 2 +
> drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 130 insertions(+)
> create mode 100644 drivers/nvmem/sec-qfprom.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index b291b27048c7..2b0dd73d899e 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -216,6 +216,18 @@ config NVMEM_QCOM_QFPROM
> This driver can also be built as a module. If so, the module
> will be called nvmem_qfprom.
>
> +config NVMEM_QCOM_SEC_QFPROM
> + tristate "QCOM SECURE QFPROM Support"
> + depends on ARCH_QCOM || COMPILE_TEST
> + depends on HAS_IOMEM
> + select QCOM_SCM
> + help
> + Say y here to enable secure QFPROM support. The secure QFPROM provides access
> + functions for QFPROM data to rest of the drivers via nvmem interface.
> +
> + This driver can also be built as a module. If so, the module will be called
> + nvmem_sec_qfprom.
> +
> config NVMEM_RAVE_SP_EEPROM
> tristate "Rave SP EEPROM Support"
> depends on RAVE_SP_CORE
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index f82431ec8aef..4b4bf5880488 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP) += nvmem-nintendo-otp.o
> nvmem-nintendo-otp-y := nintendo-otp.o
> obj-$(CONFIG_NVMEM_QCOM_QFPROM) += nvmem_qfprom.o
> nvmem_qfprom-y := qfprom.o
> +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM) += nvmem_sec_qfprom.o
> +nvmem_sec_qfprom-y := sec-qfprom.o
> obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM) += nvmem-rave-sp-eeprom.o
> nvmem-rave-sp-eeprom-y := rave-sp-eeprom.o
> obj-$(CONFIG_NVMEM_RMEM) += nvmem-rmem.o
> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
> new file mode 100644
> index 000000000000..47b2c71593dd
> --- /dev/null
> +++ b/drivers/nvmem/sec-qfprom.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
Please review your include list, this and a few others doesn't seem to
be used.
> +#include <linux/device.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +
> +
> +/**
> + * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
> + *
> + * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
> + * @dev: qfprom device structure.
> + */
> +struct sec_qfprom_priv {
There no risk of confusion here, so please drop the "_priv" suffix.
> + phys_addr_t qfpseccorrected;
Same thing here, so "base" or "addr" would suffice.
> + struct device *dev;
> +};
> +
> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
> +{
> + struct sec_qfprom_priv *priv = context;
> + u8 *val = _val;
> + u8 *tmp;
> + u32 read_val;
> + unsigned int i;
> +
> + for (i = 0; i < bytes; i++, reg++) {
> + if (i == 0 || reg % 4 == 0) {
> + if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) {
> + dev_err(priv->dev, "Couldn't access fuse register\n");
> + return -EINVAL;
> + }
> + tmp = (u8 *)&read_val;
> + }
> +
> + val[i] = tmp[reg & 3];
> + }
> +
> + return 0;
> +}
> +
> +static void sec_qfprom_runtime_disable(void *data)
> +{
> + pm_runtime_disable(data);
> +}
> +
> +static int sec_qfprom_probe(struct platform_device *pdev)
> +{
> + struct nvmem_config econfig = {
> + .name = "sec-qfprom",
> + .stride = 1,
> + .word_size = 1,
> + .id = NVMEM_DEVID_AUTO,
> + .reg_read = sec_qfprom_reg_read,
> + };
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct nvmem_device *nvmem;
> + struct sec_qfprom_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->qfpseccorrected = res->start;
> + if (!priv->qfpseccorrected)
> + return -ENOMEM;
> +
> + econfig.size = resource_size(res);
> + econfig.dev = dev;
> + econfig.priv = priv;
> +
> + priv->dev = dev;
> +
> + pm_runtime_enable(dev);
> + ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev);
> + if (ret)
> + return ret;
Please use devm_pm_runtime_enable().
> +
> + nvmem = devm_nvmem_register(dev, &econfig);
> +
> + return PTR_ERR_OR_ZERO(nvmem);
> +}
> +
> +static const struct of_device_id sec_qfprom_of_match[] = {
> + { .compatible = "qcom,sec-qfprom",},
> + {/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
> +
> +static struct platform_driver qfprom_driver = {
> + .probe = sec_qfprom_probe,
> + .driver = {
> + .name = "qcom,sec_qfprom",
No comma here please, qcom_sec_qfprom.
> + .of_match_table = sec_qfprom_of_match,
> + },
> +};
> +module_platform_driver(qfprom_driver);
> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
> +MODULE_LICENSE("GPL v2");
Please change this to "GPL".
Regards,
Bjorn
On Fri, Jun 23, 2023 at 04:53:44PM +0200, Konrad Dybcio wrote:
> On 23.06.2023 16:18, Komal Bajaj wrote:
> > diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
[..]
> > +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
> > +{
> > + struct sec_qfprom_priv *priv = context;
> > + u8 *val = _val;
> > + u8 *tmp;
> > + u32 read_val;
> > + unsigned int i;
> Please sort this to look like a reverse-Christmas-tree
>
>
> > +
> > + for (i = 0; i < bytes; i++, reg++) {
> > + if (i == 0 || reg % 4 == 0) {
> > + if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) {
> > + dev_err(priv->dev, "Couldn't access fuse register\n");
> > + return -EINVAL;
> > + }
> > + tmp = (u8 *)&read_val;
> > + }
> I don't understand this read-4-times dance.. qcom_scm_io_readl() reads
> u32, so this should be as simple as:
>
> val[i + 0] = FIELD_GET(GENMASK(7, 0), reg);
> val[i + 1] = ..
> val[i + 2] = ..
> val[i + 3] = ..
>
No need for FIELD_GET() to do that, you can just do *(u32*)val =
read_val; although this comes with alignment issues.
But, that this the length of val is always divisible by 4, and I
wasn't able to convince myself that it was the case...
So this is, pretty much, what I proposed in v3:
https://lore.kernel.org/linux-arm-msm/20230527205031.iwsujvlbxazukwfy@ripper/
Regards,
Bjorn
>
> > +
> > + val[i] = tmp[reg & 3];
> > + }
> > +
> > + return 0;