Messages in this thread | | | Subject | Re: [PATCH v3 3/4] nvmem: qfprom: Add fuse blowing support | From | Srinivas Kandagatla <> | Date | Wed, 17 Jun 2020 16:18:51 +0100 |
| |
Thanks Doug, Overall the patch is looking good, I have few minor comments below!
On 17/06/2020 15:51, Douglas Anderson wrote: > From: Ravi Kumar Bokka <rbokka@codeaurora.org> > > This patch adds support for blowing fuses to the qfprom driver if the > required properties are defined in the device tree. > > Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > Please double-check that I got the major/minor version logic right > here. I don't have documentation for this, but Srinivas mentioned > that it was at address 0x6000 and I happened to find an "8" and a "7" > on sc7180 so I assumed that was the major and minor version. > > Changes in v3: > - Don't provide "reset" value for things; just save/restore. > - Use the major/minor version read from 0x6000. > - Reading should still read "corrected", not "raw". > - Added a sysfs knob to allow you to read "raw" instead of "corrected" > - Simplified the SoC data structure. > - No need for quite so many levels of abstraction for clocks/regulator. > - Don't set regulator voltage. Rely on device tree to make sure it's right. > - Properly undo things in the case of failure. > - Don't just keep enabling the regulator over and over again. > - Enable / disable the clock each time > - Polling every 100 us but timing out in 10 us didn't make sense; swap. > - No reason for 100 us to be SoC specific. > - No need for reg-names. > - We shouldn't be creating two separate nvmem devices. > > drivers/nvmem/qfprom.c | 314 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 303 insertions(+), 11 deletions(-) > > diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c > index 8a91717600be..486202860f84 100644 > --- a/drivers/nvmem/qfprom.c > +++ b/drivers/nvmem/qfprom.c > @@ -3,57 +3,349 @@ > * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > */ > > +#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/io.h> > #include <linux/nvmem-provider.h> > #include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > + > +/* Blow timer clock frequency in Mhz */ > +#define QFPROM_BLOW_TIMER_OFFSET 0x03c > + > +/* Amount of time required to hold charge to blow fuse in micro-seconds */ > +#define QFPROM_FUSE_BLOW_POLL_US 10 > +#define QFPROM_FUSE_BLOW_TIMEOUT_US 100 > + > +#define QFPROM_BLOW_STATUS_OFFSET 0x048 > +#define QFPROM_BLOW_STATUS_BUSY 0x1 > +#define QFPROM_BLOW_STATUS_READY 0x0 > + > +#define QFPROM_ACCEL_OFFSET 0x044 > + > +#define QFPROM_VERSION_OFFSET 0x0 > +#define QFPROM_MAJOR_VERSION_SHIFT 28 > +#define QFPROM_MAJOR_VERSION_MASK 0xf > +#define QFPROM_MINOR_VERSION_SHIFT 16 > +#define QFPROM_MINOR_VERSION_MASK 0xf
Using GENMASK here makes it much readable!
...
> > static int qfprom_probe(struct platform_device *pdev) > { > + struct nvmem_config econfig = { > + .name = "qfprom", > + .stride = 1, > + .word_size = 1, > + .reg_read = qfprom_reg_read, > + }; > struct device *dev = &pdev->dev; > struct resource *res; > struct nvmem_device *nvmem; > struct qfprom_priv *priv; > + int ret; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > > + /* The corrected section is always provided */ > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - priv->base = devm_ioremap_resource(dev, res); > - if (IS_ERR(priv->base)) > - return PTR_ERR(priv->base); > + 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; > > + priv->dev = dev; > + > + /* > + * If more than one region is provided then the OS has the ability > + * to write. > + */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (res) { > + u32 version; > + int major_version, minor_version; > + > + priv->qfpraw = devm_ioremap_resource(dev, res); > + if (IS_ERR(priv->qfpraw)) > + return PTR_ERR(priv->qfpraw); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); > + priv->qfpconf = devm_ioremap_resource(dev, res); > + if (IS_ERR(priv->qfpconf)) > + return PTR_ERR(priv->qfpconf); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 3); > + priv->qfpsecurity = devm_ioremap_resource(dev, res); > + if (IS_ERR(priv->qfpsecurity)) > + return PTR_ERR(priv->qfpsecurity); > + > + version = readl(priv->qfpsecurity + QFPROM_VERSION_OFFSET); > + major_version = (version >> QFPROM_MAJOR_VERSION_SHIFT) & > + QFPROM_MAJOR_VERSION_MASK; > + minor_version = (version >> QFPROM_MINOR_VERSION_SHIFT) & > + QFPROM_MINOR_VERSION_MASK; > + > + if (major_version == 7 && minor_version == 8) > + priv->soc_data = &qfprom_7_8_data; > + > + /* Only enable writing if we have SoC data. */ > + if (priv->soc_data) > + econfig.reg_write = qfprom_reg_write; > + } > +
<----------snip > + priv->vcc = devm_regulator_get(&pdev->dev, "vcc"); > + if (IS_ERR(priv->vcc)) > + return PTR_ERR(priv->vcc); > + > + priv->secclk = devm_clk_get_optional(dev, "sec"); > + if (IS_ERR(priv->secclk)) { > + ret = PTR_ERR(priv->secclk); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "sec error getting : %d\n", ret); > + return ret; > + } > + -----------> should you move both clk and regulator into the previous if (res) {} block? As I don't see them marked as optional for write cases.
> nvmem = devm_nvmem_register(dev, &econfig); > > return PTR_ERR_OR_ZERO(nvmem); >
| |