On Fri, May 12, 2023 at 05:51:25PM +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.
> With that motivation, add the support of reading secure
> regions in 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 | 1 +
> drivers/nvmem/qfprom.c | 69 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index b291b27048c7..3d896ba29b89 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -209,6 +209,7 @@ config NVMEM_QCOM_QFPROM
> tristate "QCOM QFPROM Support"
> depends on ARCH_QCOM || COMPILE_TEST
> depends on HAS_IOMEM
> + select QCOM_SCM
> help
> Say y here to enable QFPROM support. The QFPROM provides access
> functions for QFPROM data to rest of the drivers via nvmem interface.
> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> index c1e893c8a247..20662e2d3732 100644
> --- a/drivers/nvmem/qfprom.c
> +++ b/drivers/nvmem/qfprom.c
> @@ -16,6 +16,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/property.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
>
> /* Blow timer clock frequency in Mhz */
> #define QFPROM_BLOW_TIMER_OFFSET 0x03c
> @@ -59,21 +60,22 @@ struct qfprom_soc_data {
> /**
> * struct qfprom_priv - structure holding qfprom attributes
> *
> - * @qfpraw: iomapped memory space for qfprom-efuse raw address space.
> - * @qfpconf: iomapped memory space for qfprom-efuse configuration address
> - * space.
> + * @qfpraw: iomapped memory space for qfprom-efuse raw address space.
> + * @qfpconf: iomapped memory space for qfprom-efuse configuration address space.
Adjusting the indentation makes it unnecessarily hard to see what you
actually changed.
> * @qfpcorrected: iomapped memory space for qfprom corrected address space.
> - * @qfpsecurity: iomapped memory space for qfprom security control space.
> - * @dev: qfprom device structure.
> - * @secclk: Clock supply.
> - * @vcc: Regulator supply.
> - * @soc_data: Data that for things that varies from SoC to SoC.
> + * @qfpsecurity: iomapped memory space for qfprom security control space.
> + * @qfpseccorrected: starting physical address for qfprom secure corrected address space.
> + * @dev: qfprom device structure.
> + * @secclk: Clock supply.
> + * @vcc: Regulator supply.
> + * @soc_data: Data that for things that varies from SoC to SoC.
> */
> struct qfprom_priv {
> void __iomem *qfpraw;
> void __iomem *qfpconf;
> void __iomem *qfpcorrected;
> void __iomem *qfpsecurity;
> + phys_addr_t qfpseccorrected;
> struct device *dev;
> struct clk *secclk;
> struct regulator *vcc;
> @@ -99,10 +101,12 @@ struct qfprom_touched_values {
> *
> * @keepout: Array of keepout regions for this SoC.
> * @nkeepout: Number of elements in the keepout array.
> + * @secure: Is qfprom region for this SoC protected from non-secure access.
> */
> struct qfprom_soc_compatible_data {
> const struct nvmem_keepout *keepout;
> unsigned int nkeepout;
> + bool secure;
> };
>
> static const struct nvmem_keepout sc7180_qfprom_keepout[] = {
> @@ -334,6 +338,34 @@ static int qfprom_reg_read(void *context,
> return 0;
> }
>
> +static int qfprom_sec_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
> +{
> + struct qfprom_priv *priv = context;
> + u8 *val = _val;
> + int buf_start, buf_end, index, i = 0;
> + char *buffer;
> + u32 read_val;
> +
> + buf_start = ALIGN_DOWN(reg, 4);
> + buf_end = ALIGN(reg + bytes, 4);
> + buffer = kzalloc(buf_end - buf_start, GFP_KERNEL);
> + if (!buffer)
> + return -ENOMEM;
I don't you need all these variables, the full temp buffer or the two
memcpy... I think something like this should do the trick:
unsigned int i;
u8 *val = _val;
u8 tmp[4];
for (i = 0; i < bytes; i++, reg++)
if (i == 0 || reg % 4 == 0)
qcom_scm_io_readl(qfpseccorrected + (reg & ~3), tmp);
val[i] = tmp[reg & 3];
}
> +
> + for (index = buf_start; index < buf_end; index += 4, i += 4) {
> + if (qcom_scm_io_readl(priv->qfpseccorrected + index, &read_val)) {
> + dev_err(priv->dev, "Couldn't access feature register\n");
What's a "feature register"?
Regards,
Bjorn
> + kfree_sensitive(buffer);
> + return -EINVAL;
> + }
> + memcpy(buffer + i, &read_val, 4);
> + }
> +
> + memcpy(val, buffer + reg % 4, bytes);
> + kfree_sensitive(buffer);
> + return 0;
> +}
> +
> static void qfprom_runtime_disable(void *data)
> {
> pm_runtime_disable(data);
> @@ -373,13 +405,6 @@ static int qfprom_probe(struct platform_device *pdev)
> if (!priv)
> return -ENOMEM;
>
> - /* The corrected section is always provided */
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - priv->qfpcorrected = devm_ioremap_resource(dev, res);
> - if (IS_ERR(priv->qfpcorrected))
> - return PTR_ERR(priv->qfpcorrected);
> -
> - econfig.size = resource_size(res);
> econfig.dev = dev;
> econfig.priv = priv;
>
> @@ -390,6 +415,20 @@ static int qfprom_probe(struct platform_device *pdev)
> econfig.nkeepout = soc_data->nkeepout;
> }
>
> + /* The corrected section is always provided */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + if (soc_data && soc_data->secure) {
> + priv->qfpseccorrected = res->start;
> + econfig.reg_read = qfprom_sec_reg_read;
> + } else {
> + priv->qfpcorrected = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->qfpcorrected))
> + return PTR_ERR(priv->qfpcorrected);
> + }
> +
> + econfig.size = resource_size(res);
> +
> /*
> * If more than one region is provided then the OS has the ability
> * to write.
> --
> 2.17.1
>
