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

From: Doug Anderson
Date: Thu May 14 2020 - 14:21:40 EST

Hi,

I notice that you didn't respond to any of my feedback [1], only
Srinivas's. Any reason why? It turns out that Srinivas had quite a
lot of the same feedback as I did, but please make sure you didn't
miss anything I suggested. More inline below.

On Thu, May 14, 2020 at 5:26 AM Ravi Kumar Bokka (Temp)
<rbokka@xxxxxxxxxxxxxx> 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'll let Srinivas comment if he cares. I'm really not sure why a
legal team cares about the Kconfig name in a GPL-licensed Linux
kernel.


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

If the underlying IP blocks are the same it should be one driver and
it should just work in read-only mode for the other range of stuff.


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

Generally I'd expect the SoC specific settings to be in the device
tree. Drivers don't need to specify this. Please respond to the
comments I posed in my review.


> >> + */
> >> +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.

How are you addressing? Can you go back to just having one range? If
you need to know where the raw fuse region is inside your range just
put the offset in the of_match data.


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

There is harm: it adds extra complexity. Please remove.

You are talking as if it was somehow important for this code to be the
same as the code on other OSes / in other contexts. It isn't. This
is a Linux driver and it should not be written to duplicate stuff that
Linux is already doing.


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

Please remove your disable here. Though the user initiates the call
you can still rely on Linux to make sure two users aren't trying to
blow fuses at the same time. The Linux driver should always leave
things in a "disabled" state and you can rely on that.

...besides the only way you aren't hitting an underflow on the
regulator enable count is that you are constantly enabling over and
over again.


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

Please remove. You are a Linux driver.


> >> +
> >> +/*
> >> + * 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.

The driver isn't done until it lands in Linux. While it's important
to test the driver before posting upstream it is completely expected
and normal that then posting upstream you will be asked to change
things. After you change things you will need to re-test. The fact
that you already tested this the old way is not an excuse. Please
fix.


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

Yes, but it still needs to be in the bindings.


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

Then other drivers are wrong. Please remove.


> >> +
> >> + 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");
> >>
>
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by the Linux Foundation.

[1] https://lore.kernel.org/r/CAD=FV=UZQRfBTbh2CLnwsRSpbXFf=8iF2MG20hdj47s42aP8HQ@xxxxxxxxxxxxxx