Re: [PATCH v4 3/6] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.
From: Komal Bajaj
Date: Tue Jun 27 2023 - 04:37:45 EST
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 <quic_kbajaj@xxxxxxxxxxx>
---
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