Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 14/05/2020 13:26, Ravi Kumar Bokka (Temp) wrote:
> Hi Srinivas,
> Thanks for your feedback by giving review comments. Please find my 
> inline comments.
>
>
> Regards,
> Ravi Kumar.B
>
> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
> > On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
> > > This patch adds new driver for QTI qfprom-efuse controller. This 
> > > driver can
> > > access the raw qfprom regions for fuse blowing.
> >
> > QTI?
>
> guidance I have received from internal Legal/LOST team is that the QCOM 
> prefix needs to be changed to QTI everywhere it is used

I dont mind this in comments or patch subject line but the valid vendor 
prefix for Qualcomm is "qcom".


> > > The current existed qfprom driver is only supports for cpufreq, 
> > > thermal sensors
> > > drivers by read out calibration data, speed bins..etc which is stored
> > > by qfprom efuses.
> >
> > Can you explain bit more about this QFPROM instance, Is this QFPROM 
> > part of secure controller address space?
> > Is this closely tied to SoC or Secure controller version?
> >
> > Any reason why this can not be integrated into qfprom driver with 
> > specific compatible.
>
> QFPROM driver communicates with sec_controller address space however 
> scope and functionalities of this driver is different and not limited as 
> existing qfprom fuse Read-Only driver for specific âfuse bucketsâ like 
> cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch 
> requires specific sequence to write/blow fuses unlike other driver. 
> Scope/functionalities are different and this is separate driver.

This is another variant of qfprom, so please add this support in the 
existing qfprom driver, you could deal with the differences using 
specific compatible within the driver.

Doug already covered most of the comments, consider them as mandatory 
requirements for upstreaming!

--srini


> > > Signed-off-by: Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx>
> > > ---
> > > Â drivers/nvmem/KconfigÂÂÂÂÂÂÂ |Â 10 +
> > > Â drivers/nvmem/MakefileÂÂÂÂÂÂ |ÂÂ 2 +
> > > Â drivers/nvmem/qfprom-efuse.c | 476 
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > Â 3 files changed, 488 insertions(+)
> > > Â create mode 100644 drivers/nvmem/qfprom-efuse.c
> > ...
> >
> > > diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
> > > new file mode 100644
> > > index 0000000..2e3c275
> > > --- /dev/null
> > > +++ b/drivers/nvmem/qfprom-efuse.c
> > > @@ -0,0 +1,476 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/device.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/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#define QFPROM_BLOW_STATUS_BUSY 0x1
> > > +#define QFPROM_BLOW_STATUS_READY 0x0
> > > +
> > > +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
> > > +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> > > +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
> > > +
> > > +/* Amount of time required to hold charge to blow fuse in 
> > > micro-seconds */
> > > +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
> > > +#define QFPROM_BLOW_STATUS_OFFSET 0x048
> > > +
> > > +#define QFPROM_ACCEL_OFFSET 0x044
> > > +
> > > +/**
> > > + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
> > > + * platform data
> > > + *
> > > + * @name: qfprom-efuse compatible name
> >
> > ??
>
> Thanks for your feedback. I will address this change
>
> > > + * @fuse_blow_time_in_us: Should contain the wait time when doing 
> > > the fuse blow
> > > + * @accel_value: Should contain qfprom accel value
> > > + * @accel_reset_value: The reset value of qfprom accel value
> > > + * @qfprom_blow_timer_value: The timer value of qfprom when doing 
> > > efuse blow
> > > + * @qfprom_blow_reset_freq: The frequency required to set when fuse 
> > > blowing
> > > + * is done
> > > + * @qfprom_blow_set_freq: The frequency required to set when we 
> > > start the
> > > + * fuse blowing
> > > + * @qfprom_max_vol: max voltage required to set fuse blow
> > > + * @qfprom_min_vol: min voltage required to set fuse blow
> >
> > How specific are these values per SoC?
>
> This voltage level may change based on SoC and/or fuse-hardware 
> technology, it would change for SoC with different technology, hence we 
> have kept it in SOC specific settings.
>
> > > + */
> > > +struct qfprom_efuse_platform_data {
> > > +ÂÂÂ const char *name;
> > > +ÂÂÂ u8 fuse_blow_time_in_us;
> > > +ÂÂÂ u32 accel_value;
> > > +ÂÂÂ u32 accel_reset_value;
> > > +ÂÂÂ u32 qfprom_blow_timer_value;
> > > +ÂÂÂ u32 qfprom_blow_reset_freq;
> > > +ÂÂÂ u32 qfprom_blow_set_freq;
> > > +ÂÂÂ u32 qfprom_max_vol;
> > > +ÂÂÂ u32 qfprom_min_vol;
> > > +};
> > > +
> > > +/**
> > > + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
> > > + *
> > > + * @qfpbase: iomapped memory space for qfprom base
> > > + * @qfpraw: iomapped memory space for qfprom raw fuse region
> > > + * @qfpmap: iomapped memory space for qfprom fuse blow timer
> > > +
> > > + * @dev: qfprom device structure
> > > + * @secclk: clock supply
> > > + * @vcc: regulator supply
> > > +
> > > + * @qfpraw_start: qfprom raw fuse start region
> > > + * @qfpraw_end: qfprom raw fuse end region
> > > + * @qfprom_efuse_platform_data: qfprom platform data
> > > + */
> > > +struct qfprom_efuse_priv {
> > > +ÂÂÂ void __iomem *qfpbase;
> > > +ÂÂÂ void __iomem *qfpraw;
> > > +ÂÂÂ void __iomem *qfpmap;
> >
> > Why are these memory regions split? Can't you just have complete 
> > qfprom area and add fixed offset for qfpraw within the driver?
>
> Thanks for your feedback. I will address this change.
> I have separated this memory regions because to identify raw fuse 
> regions separately and compare these raw fuse regions from the user 
> given input.
>
> > > +ÂÂÂ struct device *dev;
> > > +ÂÂÂ struct clk *secclk;
> > > +ÂÂÂ struct regulator *vcc;
> > > +ÂÂÂ resource_size_t qfpraw_start;
> > > +ÂÂÂ resource_size_t qfpraw_end;
> > Why do we need to check this range? as long as we set the nvmem_config 
> > with correct range then you should not need this check.
>
> There is no harm in this explicit check in QFPROM-fuse driver and based 
> on internal review with our security team, this check is important to 
> avoid dependency on other upper layer.
>
>
> > > +ÂÂÂ struct qfprom_efuse_platform_data efuse;
> > A pointer here should be good enough?
> > > +};
> > > +
>
> Thanks for your feedback. I will address this change
>
> > ...
> >
> > > +/*
> > > + * sets the value of the blow timer, accel register and the clock
> > > + * and voltage settings
> > > + */
> > > +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv 
> > > *priv)
> > > +{
> > > +ÂÂÂ int ret;
> > > +
> > > +ÂÂÂ ret = qfprom_disable_fuse_blowing(priv);
> > > +ÂÂÂ if (ret) {
> > > +ÂÂÂÂÂÂÂ dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
> > > +ÂÂÂÂÂÂÂ return ret;
> > > +ÂÂÂ }
> >
> > Why do we need to qfprom_disable_fuse_blowing() for every call to 
> > enable it?
> >
> > Or are we missing some error handling in the caller?
>
> We must disable/vote-off this QFPROM fuse power rail after blowing fuse, 
> it is the safe and right approach as per hardware programming guide for 
> fuse blowing process. Caller here is user space, canât control 
> fuse-power-rail or canât be relied to follow the required process. There 
> could also be unnecessary risk of leaving the vote/power-rail configured 
> at specific level after blowing the fuse. As per hardware requirement, 
> right after fuse blowing, we need to disable power rail.
>
> > > +
> > > +ÂÂÂ writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
> > > +ÂÂÂÂÂÂÂÂÂÂ QFPROM_BLOW_TIMER_OFFSET);
> > > +ÂÂÂ writel(priv->efuse.accel_value, priv->qfpmap + 
> > > QFPROM_ACCEL_OFFSET);
> > > +
> > > +ÂÂÂ ret = qfprom_set_clock_settings(priv);
> > > +ÂÂÂ if (ret) {
> > > +ÂÂÂÂÂÂÂ dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
> > > +ÂÂÂÂÂÂÂ return ret;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ priv->efuse.qfprom_max_vol);
> > > +ÂÂÂ if (ret) {
> > > +ÂÂÂÂÂÂÂ dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
> > > +ÂÂÂÂÂÂÂ return ret;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ return 0;
> > > +}
> > > +
> >
> > <<
> > > +/*
> > > + * verifying to make sure address being written or read is from qfprom
> > > + * raw address range
> > > + */
> > > +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 
> > > reg,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t bytes)
> > > +{
> > > +ÂÂÂ if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
> > > +ÂÂÂÂÂÂÂ ((reg + bytes) <= priv->qfpraw_end)) {
> > > +ÂÂÂÂÂÂÂ return 1;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ return 0;
> > > +}
> > Â>>
> > Above function is totally redundant, nvmem core already has checks for 
> > this.
>
> There is no harm in this explicit check in QFPROM-fuse driver and based 
> on internal review with our security team, this check is important to 
> avoid dependency on other upper layer.
>
> > > +
> > > +/*
> > > + * API for reading from raw qfprom region
> > > + */
> > > +static int qfprom_efuse_reg_read(void *context, unsigned int reg, 
> > > void *_val,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t bytes)
> > > +{
> > > +ÂÂÂ struct qfprom_efuse_priv *priv = context;
> > > +ÂÂÂ u32 *value = _val;
> > > +ÂÂÂ u32 align_check;
> > > +ÂÂÂ int i = 0, words = bytes / 4;
> > > +
> > > +ÂÂÂ dev_info(priv->dev,
> > > +ÂÂÂÂÂÂÂÂ "reading raw qfprom region offset: 0x%08x of size: %zd\n",
> > > +ÂÂÂÂÂÂÂÂ reg, bytes);
> >
> > In general there is lot of debug info across the code, do you really 
> > need all this? Consider removing these!
>
> Thanks for your feedback. I will address this change.
>
> > > +
> > > +ÂÂÂ if (bytes % 4 != 0x00) {
> > > +ÂÂÂÂÂÂÂ dev_err(priv->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂ "Bytes: %zd to read should be word align\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂ bytes);
> > > +ÂÂÂÂÂÂÂ return -EINVAL;
> > > +ÂÂÂ }
> >
> > This word align check is also redundant once you set nvmem_config with 
> > correct word_size.
>
> I understand that there may be different approach to handle this. We 
> have used this approach and tested this driver thoroughly. Unless there 
> is technical limitation, changing this word_size would end up requiring 
> re-writing write/read APIs and going through testing again, there is not 
> much difference in either approach, we would like to keep this approach 
> unless there is technical concern.
>
> > > +
> > > +ÂÂÂ if (!addr_in_qfprom_range(priv, reg, bytes)) {
> > > +ÂÂÂÂÂÂÂ dev_err(priv->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂ "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂ reg, bytes);
> > > +ÂÂÂÂÂÂÂ return -EINVAL;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ align_check = (reg & 0xF);
> > > +
> > > +ÂÂÂ if (((align_check & ~3) == align_check) && value != NULL)
> > > +ÂÂÂÂÂÂÂ while (words--)
> > > +ÂÂÂÂÂÂÂÂÂÂÂ *value++ = readl(priv->qfpbase + reg + (i++ * 4));
> > > +
> > > +ÂÂÂ else
> > > +ÂÂÂÂÂÂÂ dev_err(priv->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂ "Invalid input parameter 0x%08x fuse blow address\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂ reg);
> > > +
> > > +ÂÂÂ return 0;
> > > +}
> > ...
> >
> > > +
> > > +static int qfprom_efuse_probe(struct platform_device *pdev)
> > > +{
> > > +ÂÂÂ struct device *dev = &pdev->dev;
> > > +ÂÂÂ struct resource *qfpbase, *qfpraw, *qfpmap;
> > > +ÂÂÂ struct nvmem_device *nvmem;
> > > +ÂÂÂ struct nvmem_config *econfig;
> > > +ÂÂÂ struct qfprom_efuse_priv *priv;
> > > +ÂÂÂ const struct qfprom_efuse_platform_data *drvdata;
> > > +ÂÂÂ int ret;
> > > +
> > > +ÂÂÂ dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
> > > +
> >
> > too much debug!
>
> Thanks for your feedback. I will address this change.
>
> > > +ÂÂÂ drvdata = of_device_get_match_data(&pdev->dev);
> > > +ÂÂÂ if (!drvdata)
> > > +ÂÂÂÂÂÂÂ return -EINVAL;
> > Unnecessary check as this driver will not be probed unless there is a 
> > compatible match.
>
> Thanks for your feedback. I will address this change.
>
> > > +
> > > +ÂÂÂ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +ÂÂÂ if (!priv)
> > > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > > +
> > > +ÂÂÂ priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
> > > +ÂÂÂ priv->efuse.accel_value = drvdata->accel_value;
> > > +ÂÂÂ priv->efuse.accel_reset_value = drvdata->accel_reset_value;
> > > +ÂÂÂ priv->efuse.qfprom_blow_timer_value = 
> > > drvdata->qfprom_blow_timer_value;
> > > +ÂÂÂ priv->efuse.qfprom_blow_reset_freq = 
> > > drvdata->qfprom_blow_reset_freq;
> > > +ÂÂÂ priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
> > > +ÂÂÂ priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
> > > +ÂÂÂ priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
> > > +ÂÂÂ priv->dev = dev;
> > > +
> > > +ÂÂÂ qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +
> > > +ÂÂÂ priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
> > > +ÂÂÂ if (IS_ERR(priv->qfpbase)) {
> > > +ÂÂÂÂÂÂÂ ret = PTR_ERR(priv->qfpbase);
> > > +ÂÂÂÂÂÂÂ goto err;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > +
> > > +ÂÂÂ priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
> > > +ÂÂÂ if (IS_ERR(priv->qfpraw)) {
> > > +ÂÂÂÂÂÂÂ ret = PTR_ERR(priv->qfpraw);
> > > +ÂÂÂÂÂÂÂ goto err;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ priv->qfpraw_start = qfpraw->start - qfpbase->start;
> > > +ÂÂÂ priv->qfpraw_end = qfpraw->end - qfpbase->start;
> > > +
> > > +ÂÂÂ qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > > +
> > > +ÂÂÂ priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
> > > +ÂÂÂ if (IS_ERR(priv->qfpmap)) {
> > > +ÂÂÂÂÂÂÂ ret = PTR_ERR(priv->qfpmap);
> > > +ÂÂÂÂÂÂÂ goto err;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
> >
> > I see no reference to this regulator in dt bindings.
>
> This perameter kept in board specific file i.e., sc7180-idp.dts file
>
> > > +ÂÂÂ if (IS_ERR(priv->vcc)) {
> > > +ÂÂÂÂÂÂÂ ret = PTR_ERR(priv->vcc);
> > > +ÂÂÂÂÂÂÂ if (ret == -ENODEV)
> > > +ÂÂÂÂÂÂÂÂÂÂÂ ret = -EPROBE_DEFER;
> > Can you explain what is going on here?
>
> As i took other drivers reference, i have kept this check.
>
> > > +
> > > +ÂÂÂÂÂÂÂ goto err;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ priv->secclk = devm_clk_get(dev, "secclk");
> > > +ÂÂÂ if (IS_ERR(priv->secclk)) {
> > > +ÂÂÂÂÂÂÂ ret = PTR_ERR(priv->secclk);
> > > +ÂÂÂÂÂÂÂ if (ret != -EPROBE_DEFER)
> > > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "secclk error getting : %d\n", ret);
> > > +ÂÂÂÂÂÂÂ goto err;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ ret = clk_prepare_enable(priv->secclk);
> > > +ÂÂÂ if (ret) {
> > > +ÂÂÂÂÂÂÂ dev_err(dev, "clk_prepare_enable() failed\n");
> > > +ÂÂÂÂÂÂÂ goto err;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
> > > +ÂÂÂ if (!econfig)
> > Why not disabling the clk here?
> > > +ÂÂÂÂÂÂÂ return -ENOMEM;
>
> Thanks for your feedback. I will address this change.
>
> > > +
> > > +ÂÂÂ econfig->dev = dev;
> > > +ÂÂÂ econfig->name = "qfprom-efuse";
> > > +ÂÂÂ econfig->stride = 1;
> > > +ÂÂÂ econfig->word_size = 1;
> > > +ÂÂÂ econfig->reg_read = qfprom_efuse_reg_read;
> > > +ÂÂÂ econfig->reg_write = qfprom_efuse_reg_write;
> > > +ÂÂÂ econfig->size = resource_size(qfpraw);
> > > +ÂÂÂ econfig->priv = priv;
> > > +
> > > +ÂÂÂ nvmem = devm_nvmem_register(dev, econfig);
> > > +
> > > +ÂÂÂ return PTR_ERR_OR_ZERO(nvmem);
> > probably you should check the nvmem here before returning to disable 
> > the clk properly.
>
> Thanks for your feedback. I will address this change.
>
> > > +
> > > +err:
> > > +ÂÂÂ clk_disable_unprepare(priv->secclk);
> > > +ÂÂÂ return ret;
> > > +}
> > > +
> > > +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data 
> > > = {
> > > +ÂÂÂ .name = "sc7180-qfprom-efuse",
> > Redundant.
>
> Thanks for your feedback. I will address this change.
>
> > > +ÂÂÂ .fuse_blow_time_in_us = 10,
> > > +ÂÂÂ .accel_value = 0xD10,
> > > +ÂÂÂ .accel_reset_value = 0x800,
> > > +ÂÂÂ .qfprom_blow_timer_value = 25,
> > > +ÂÂÂ .qfprom_blow_reset_freq = 19200000,
> > > +ÂÂÂ .qfprom_blow_set_freq = 4800000,
> > > +ÂÂÂ .qfprom_max_vol = 1904000,
> > > +ÂÂÂ .qfprom_min_vol = 1800000,
> > > +};
> > > +
> > > +static const struct of_device_id qfprom_efuse_of_match[] = {
> > > +ÂÂÂ {
> > > +ÂÂÂÂÂÂÂ .compatible = "qcom,sc7180-qfprom-efuse",
> > > +ÂÂÂÂÂÂÂ .data = &sc7180_qfp_efuse_data
> > > +ÂÂÂ },
> > > +ÂÂÂ {/* sentinel */},
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
> > > +
> > > +static struct platform_driver qfprom_efuse_driver = {
> > > +ÂÂÂ .probe = qfprom_efuse_probe,
> > > +ÂÂÂ .driver = {
> > > +ÂÂÂÂÂÂÂ .name = "sc7180-qfprom-efuse",
> > > +ÂÂÂÂÂÂÂ .of_match_table = qfprom_efuse_of_match,
> > > +ÂÂÂ },
> > > +};
> > > +
> > > +module_platform_driver(qfprom_efuse_driver);
> > > +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
> > > +MODULE_LICENSE("GPL v2");