[v3,01/10] nvmem: qfprom: Add support for secure reading

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@quicinc.com>
---
 drivers/nvmem/Kconfig  |  1 +
 drivers/nvmem/qfprom.c | 69 +++++++++++++++++++++++++++++++++---------
 2 files changed, 55 insertions(+), 15 deletions(-)

--
2.17.1

On 5/28/2023 2:20 AM, Bjorn Andersson wrote:
> 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@quicinc.com>
>> ---
>>   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.
Okay, will maintain the existing indentation.
>
>>    * @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];
> 	}
Thanks, for this. Will make the suggested change in the next patch series.
>> +
>> +	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"?
It's a fuse register that we are trying to read here.

Thanks
Komal

>
> 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
>>