Re: [RFC PATCHv2 2/4] drivers/otp: add support for Picoxcell PC3X3OTP

From: Jamie Iles
Date: Thu Mar 24 2011 - 16:59:58 EST

Next message: Jens Axboe: "Re: [GIT PULL] Core block IO bits for 2.6.39 - early Oops"
Previous message: Ingo Molnar: "Re: [PATCH RFC] x86: avoid atomic operation in test_and_set_bit_lockif possible"
In reply to: Mike Frysinger: "Re: [RFC PATCHv2 2/4] drivers/otp: add support for Picoxcell PC3X3 OTP"
Next in thread: Mike Frysinger: "Re: [RFC PATCHv2 2/4] drivers/otp: add support for Picoxcell PC3X3 OTP"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Mike,

Thanks for another great review!

On Thu, Mar 24, 2011 at 02:50:35PM -0400, Mike Frysinger wrote:
> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> > --- a/drivers/otp/Kconfig
> > +++ b/drivers/otp/Kconfig
> > @@ -8,3 +8,14 @@ menuconfig OTP
> > Say y here to support OTP memory found in some embedded devices.
> > This memory can commonly be used to store boot code, cryptographic
> > keys and other persistent data.
> > +
> > +if OTP
> > +
> > +config OTP_PC3X3
> > + tristate "Enable support for Picochip PC3X3 OTP"
> > + depends on OTP && ARCH_PICOXCELL
>
> since every driver is going to need "depend OTP", the "if OTP" is
> redundant then right ?

Yes, I'll drop the "if OTP".

>
> > + help
> > + Say y or m here to allow support for the OTP found in PC3X3 devices.
> > + If you say m then the module will be called otp_pc3x3.
>
> "y" -> "y"
> "m" -> "M"

Ok.

>
> > +
> > +endif
> > diff --git a/drivers/otp/Makefile b/drivers/otp/Makefile
> > index 84fd03e..c710ec4 100644
> > --- a/drivers/otp/Makefile
> > +++ b/drivers/otp/Makefile
> > @@ -1 +1,2 @@
> > obj-$(CONFIG_OTP) += otp.o
> > +obj-$(CONFIG_OTP_PC3X3) += otp_pc3x3.o
> > diff --git a/drivers/otp/otp_pc3x3.c b/drivers/otp/otp_pc3x3.c
> > new file mode 100644
> > index 0000000..855d664
> > --- /dev/null
> > +++ b/drivers/otp/otp_pc3x3.c
> > @@ -0,0 +1,1079 @@
> > +/*
> > + * Copyright 2010-2011 Picochip LTD, Jamie Iles
> > + * http://www.picochip.com
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + *
> > + * This driver implements a picoxcellotp backend for reading and writing the
> > + * OTP memory in Picochip PC3X3 devices. This OTP can be used for executing
> > + * secure boot code or for the secure storage of keys and any other user data.
> > + */
> > +#define pr_fmt(fmt) "pc3x3otp: " fmt
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/otp.h>
> > +#include <linux/platform_device.h>
> > +
> > +/*
> > + * To test the user interface and most of the driver logic, we have a test
> > + * mode whereby rather than writing to OTP we have a RAM buffer that simulates
> > + * the OTP. This means that we can test everything apart from:
> > + *
> > + * - The OTP state machines and commands.
> > + * - Failure to program bits.
> > + */
> > +static int test_mode;
> > +module_param(test_mode, bool, S_IRUSR);
> > +MODULE_PARM_DESC(test_mode,
> > + "Run in test mode (use a memory buffer rather than OTP");
> > +
> > +/*
> > + * This is the maximum number of times to try and soak a failed bit. We get
> > + * this from the Sidense documentation. After 16 attempts it is very unlikely
> > + * that anything will change.
> > + */
> > +#define MAX_PROGRAM_RETRIES 16
> > +
> > +#define OTP_MACRO_CMD_REG_OFFSET 0x00
> > +#define OTP_MACRO_STATUS_REG_OFFSET 0x04
> > +#define OTP_MACRO_CONFIG_REG_OFFSET 0x08
> > +#define OTP_MACRO_ADDR_REG_OFFSET 0x0C
> > +#define OTP_MACRO_D_LO_REG_OFFSET 0x10
> > +#define OTP_MACRO_D_HI_REG_OFFSET 0x14
> > +#define OTP_MACRO_Q_LO_REG_OFFSET 0x20
> > +#define OTP_MACRO_Q_HI_REG_OFFSET 0x24
> > +#define OTP_MACRO_Q_MR_REG_OFFSET 0x28
> > +#define OTP_MACRO_Q_MRAB_REG_OFFSET 0x2C
> > +#define OTP_MACRO_Q_SR_LO_REG_OFFSET 0x30
> > +#define OTP_MACRO_Q_SR_HI_REG_OFFSET 0x34
> > +#define OTP_MACRO_Q_RR_LO_REG_OFFSET 0x38
> > +#define OTP_MACRO_Q_RR_HI_REG_OFFSET 0x3C
> > +#define OTP_MACRO_TIME_RD_REG_OFFSET 0x40
> > +#define OTP_MACRO_TIME_WR_REG_OFFSET 0x44
> > +#define OTP_MACRO_TIME_PGM_REG_OFFSET 0x48
> > +#define OTP_MACRO_TIME_PCH_REG_OFFSET 0x4C
> > +#define OTP_MACRO_TIME_CMP_REG_OFFSET 0x50
> > +#define OTP_MACRO_TIME_RST_REG_OFFSET 0x54
> > +#define OTP_MACRO_TIME_PWR_REG_OFFSET 0x58
> > +#define OTP_MACRO_DIRECT_IO_REG_OFFSET 0x5C
> > +
> > +/*
> > + * The OTP addresses of the special register. This is in the boot
> > + * sector and we use words 0 and 2 of sector 0 in redundant format.
> > + */
> > +#define SR_ADDRESS_0 ((1 << 11) | 0x0)
> > +#define SR_ADDRESS_2 ((1 << 11) | 0x2)
> > +
> > +enum otp_command {
> > + OTP_COMMAND_IDLE,
> > + OTP_COMMAND_WRITE,
> > + OTP_COMMAND_PROGRAM,
> > + OTP_COMMAND_READ,
> > + OTP_COMMAND_WRITE_MR,
> > + OTP_COMMAND_PRECHARGE,
> > + OTP_COMMAND_COMPARE,
> > + OTP_COMMAND_RESET,
> > + OTP_COMMAND_RESET_M,
> > + OTP_COMMAND_POWER_DOWN,
> > + OTP_COMMAND_AUX_UPDATE_A,
> > + OTP_COMMAND_AUX_UPDATE_B,
> > + OTP_COMMAND_WRITE_PROGRAM,
> > + OTP_COMMAND_WRITE_MRA,
> > + OTP_COMMAND_WRITE_MRB,
> > + OTP_COMMAND_RESET_MR,
> > +};
> > +
> > +/* The control and status registers follow the AXI OTP map. */
> > +#define OTP_CTRL_BASE 0x4000
> > +
> > +/*
> > + * The number of words in the OTP device. The device is 16K bytes and the word
> > + * size is 64 bits.
> > + */
> > +#define OTP_NUM_WORDS (SZ_16K / OTP_WORD_SIZE)
> > +
> > +/*
> > + * The OTP device representation. We can have a static structure as there is
> > + * only ever one OTP device in a system.
> > + *
> > + * @iomem: the io memory for the device that should be accessed with the I/O
> > + * accessors.
> > + * @mem: the 16KB of OTP memory that can be accessed like normal memory. When
> > + * we probe, we force the __iomem away so we can read it directly.
> > + * @test_mode_sr0, test_mode_sr2 the values of the special register when we're
> > + * in test mode.
> > + */
> > +struct pc3x3_otp {
> > + struct otp_device *dev;
> > + void __iomem *iomem;
> > + void *mem;
> > + struct clk *clk;
> > + u64 test_mode_sr0, test_mode_sr2;
> > + unsigned long registered_regions;
> > +};
> > +
> > +static inline void otp_write_reg(struct pc3x3_otp *otp, unsigned reg_num,
> > + u32 value)
> > +{
> > + writel(value, otp->iomem + OTP_CTRL_BASE + reg_num);
> > +}
> > +
> > +static inline u32 otp_read_reg(struct pc3x3_otp *otp, unsigned reg_num)
> > +{
> > + return readl(otp->iomem + OTP_CTRL_BASE + reg_num);
> > +}
> > +
> > +static inline u32 otp_read_sr(struct pc3x3_otp *otp)
> > +{
> > + if (test_mode)
> > + return otp->test_mode_sr0 | otp->test_mode_sr2;
> > +
> > + return otp_read_reg(otp, OTP_MACRO_Q_SR_LO_REG_OFFSET);
> > +}
> > +
> > +/*
> > + * Get the region format. The region format encoding and number of regions are
> > + * encoded in the bottom 32 bis of the special register:
> > + *
> > + * 20: enable redundancy replacement.
> > + * [2:0]: AXI address mask - determines the number of address bits to use for
> > + * selecting the region to read from.
> > + * [m:n]: the format for region X where n := (X * 2) + 4 and m := n + 1.
> > + */
> > +static enum otp_redundancy_fmt
> > +__pc3x3_otp_region_get_fmt(struct pc3x3_otp *otp,
> > + const struct otp_region *region)
> > +{
> > + unsigned shift = (region->region_nr * 2) + 4;
> > +
> > + return (otp_read_sr(otp) >> shift) & 0x3;
> > +}
> > +
> > +static enum otp_redundancy_fmt
> > +pc3x3_otp_region_get_fmt(struct otp_region *region)
> > +{
> > + struct pc3x3_otp *otp = dev_get_drvdata(region->dev.parent);
> > +
> > + return __pc3x3_otp_region_get_fmt(otp, region);
> > +}
> > +
> > +/*
> > + * Find out how many regions the OTP is partitioned into. This can be 1, 2, 4
> > + * or 8.
> > + */
> > +static inline int otp_num_regions(struct pc3x3_otp *otp)
> > +{
> > +#define SR_AXI_ADDRESS_MASK 0x7
> > + u32 addr_mask;
> > + int nr_regions;
> > +
> > + addr_mask = otp_read_sr(otp) & SR_AXI_ADDRESS_MASK;
> > +
> > + if (0 == addr_mask)
> > + nr_regions = 1;
> > + else if (4 == addr_mask)
> > + nr_regions = 2;
> > + else if (6 == addr_mask)
> > + nr_regions = 4;
> > + else if (7 == addr_mask)
> > + nr_regions = 8;
> > + else
> > + nr_regions = 0;
> > +
> > + if (WARN_ON(0 == nr_regions))
> > + return -EINVAL;
> > +
> > + return nr_regions;
> > +}
>
> the "if" style is backwards from most of the kernel ... plus, this
> would probably look cleaner as a switch() statement
>
> is this sort of a big func to be forcing inline isnt it ?

Fair comments, I'll rework that, and I'm not sure why I made it inline
in the first place...

>
> > +static void otp_charge_pump_enable(struct pc3x3_otp *otp, int enable)
> > +{
> > +#define OTP_MRA_CHARGE_PUMP_ENABLE_MASK (1 << 12)
> > +#define OTP_MRA_CHARGE_PUMP_MONITOR_MASK (1 << 15)
> > +#define OTP_MRA_READ_REFERENCE_LEVEL9_MASK (1 << 9)
> > +#define OTP_MRA_READ_REFERENCE_LEVEL5_MASK (1 << 5)
> > +#define OTP_STATUS_VPP_APPLIED (1 << 4)
> > + u32 mra = enable ?
> > + (OTP_MRA_CHARGE_PUMP_ENABLE_MASK |
> > + OTP_MRA_CHARGE_PUMP_MONITOR_MASK |
> > + OTP_MRA_READ_REFERENCE_LEVEL9_MASK |
> > + OTP_MRA_READ_REFERENCE_LEVEL5_MASK) : 0;
> > +
> > + otp_write_MRA(otp, mra);
> > +
> > + /* Now wait for VPP to reach the correct level. */
> > + if (enable && !test_mode) {
> > + while (!(otp_read_reg(otp, OTP_MACRO_STATUS_REG_OFFSET) &
> > + OTP_STATUS_VPP_APPLIED))
> > + cpu_relax();
> > + }
> > +
> > + udelay(1);
> > +}
>
> is that udelay() really necessary ? could it be refined to a smaller ndelay() ?

It's what is specifed in the IP vendors datasheets so perhaps it could
be less but I'd like to err on the side of caution.

>
> > + * any read-modify-write that is neccessary. For example if address 0 contains
>
> "neccessary" -> "necessary"

OK.

>
> > +static int otp_raw_write_word(struct pc3x3_otp *otp,
> > + unsigned addr, u64 val)
> > +{
> > + /* The status of the last command. 1 == success. */
> > +#define OTP_STATUS_LCS (1 << 1)
> > +
> > +#define OTP_MR_SELF_TIMING (1 << 2)
> > +#define OTP_MR_PROGRAMMABLE_DELAY (1 << 5)
> > +#define OTP_MR_PROGRAMMABLE_DELAY_CONTROL (1 << 8)
> > +
> > +#define OTP_MRB_VREF_ADJUST_0 (1 << 0)
> > +#define OTP_MRB_VREF_ADJUST_1 (1 << 1)
> > +#define OTP_MRB_VREF_ADJUST_3 (1 << 3)
> > +#define OTP_MRB_READ_TIMER_DELAY_CONTROL (1 << 12)
>
> this driver sure likes to inline defines ... usually these are kept
> all together at the top, or in a sep file like drivers/otp/otp_pc3x3.h

Something I picked up from another driver but I can appreciate why it
isn't liked. I'll move them all to the top of the driver.

>
> > + /* Enable the charge pump to begin programming. */
> > + otp_charge_pump_enable(otp, 1);
> > + otp_write_MRB(otp, OTP_MRB_VREF_ADJUST_3 |
> > + OTP_MRB_READ_TIMER_DELAY_CONTROL);
> > + otp_write_MR(otp, OTP_MR_SELF_TIMING | OTP_MR_PROGRAMMABLE_DELAY |
> > + OTP_MR_PROGRAMMABLE_DELAY_CONTROL);
> > + otp_raw_program_word(otp, addr, v);
> > + udelay(1);
>
> i thought you had a helper func to do this ?

I don't think so, there are a few similar looking things with different
timing parameters so there may be potential for a helper...

>
> > +static int pc3x3_otp_region_read_word(struct otp_region *region,
> > + unsigned long addr, u64 *word)
> > +{
> > + switch (fmt) {
> > + case OTP_REDUNDANCY_FMT_SINGLE_ENDED:
> > + case OTP_REDUNDANCY_FMT_REDUNDANT:
> > + case OTP_REDUNDANCY_FMT_DIFFERENTIAL:
> > + case OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT:
> > + default:
> > + err = -EINVAL;
> > + }
> > +
> > + *word = result;
>
> should that write happen even when there's an error ?

I figured that it wouldn't matter as the caller shouldn't use it when it
gets an error returned but perhaps it's better to be safe.

>
> > +static ssize_t pc3x3_otp_region_get_size(struct otp_region *region)
> > +{
> > + size_t region_sz;
> > + return region_sz;
> > +}
>
> return type is ssize_t, but you're returning a size_t ...

Doh! Thanks.

>
> > +static int pc3x3_otp_get_nr_regions(struct otp_device *dev)
> > +{
> > + struct pc3x3_otp *otp = dev_get_drvdata(&dev->dev);
> > + unsigned long sr = otp_read_sr(otp);
> > + u32 addr_mask = sr & SR_AXI_ADDRESS_MASK;
> > +
> > + if (0 == addr_mask)
> > + return 1;
> > + else if (4 == addr_mask)
> > + return 2;
> > + else if (6 == addr_mask)
> > + return 4;
> > + else if (7 == addr_mask)
> > + return 8;
> > +
> > + return -EINVAL;
> > +}
>
> use a switch() statement instead ?

OK.

> > +static int __devinit otp_probe(struct platform_device *pdev)
>
> namespace this ? so call it "pc3x3_otp_probe" ...

Yes, I'll check for others too.

>
> > + if (!devm_request_mem_region(&pdev->dev, mem->start,
> > + resource_size(mem), "otp")) {
> > + dev_err(&pdev->dev, "unable to request i/o memory\n");
> > + return -EBUSY;
> > + }
> > +
> > + pc3x3_dev = devm_kzalloc(&pdev->dev, sizeof(*pc3x3_dev), GFP_KERNEL);
> > + if (!pc3x3_dev)
> > + return -ENOMEM;
>
> i'm not familiar with "devm_request_mem_region", but does it not need
> to be unrequested ?
>
> also, should you be using the driver's name "pc3x3" rather than "otp"
> when requesting things ?

No, devm_* automatically does the cleanup when either probe() fails or
remove() is called, and yes the naming is wrong here, I'll fix that.

>
> > + if (test_mode) {
> > + u64 *p = devm_kzalloc(&pdev->dev, SZ_16K + SZ_1K, GFP_KERNEL);
> > ...
> > + } else {
> > + pc3x3_dev->iomem = devm_ioremap(&pdev->dev, mem->start,
> > + resource_size(mem));
> > ...
> > +out_unregister:
> > + otp_device_unregister(otp);
> > +out_clk_disable:
> > + clk_disable(pc3x3_dev->clk);
> > + clk_put(pc3x3_dev->clk);
> > +out:
> > + return err;
>
> hmm, but you dont iounmap or free any of the memory from earlier when
> you error out ...

No, that's what the devm_* stuff does for us.

>
> > +static int __devexit otp_remove(struct platform_device *pdev)
> > +{
> > + struct pc3x3_otp *otp = platform_get_drvdata(pdev);
> > +
> > + otp_device_unregister(otp->dev);
> > + clk_disable(otp->clk);
> > + clk_put(otp->clk);
> > +
> > + return 0;
> > +}
>
> seems like you forgot to release iomem here

devm_ioremap() and devm_request_region() should cover that for us.

>
> > +#ifdef CONFIG_PM
> > +static int otp_suspend(struct device *dev)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct pc3x3_otp *otp = platform_get_drvdata(pdev);
> > +
> > + otp_write_reg(otp, OTP_MACRO_CMD_REG_OFFSET, OTP_COMMAND_POWER_DOWN);
> > + clk_disable(otp->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int otp_resume(struct device *dev)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct pc3x3_otp *otp = platform_get_drvdata(pdev);
> > +
> > + clk_enable(otp->clk);
> > + otp_write_reg(otp, OTP_MACRO_CMD_REG_OFFSET, OTP_COMMAND_IDLE);
> > +
> > + return 0;
> > +}
> > +#else /* CONFIG_PM */
> > +#define otp_suspend NULL
> > +#define otp_resume NULL
> > +#endif /* CONFIG_PM */
> > +
> > +static const struct dev_pm_ops otp_pm_ops = {
> > + .suspend = otp_suspend,
> > + .resume = otp_resume,
> > +};
>
> usually people put the dev_pm_ops struct behind CONFIG_PM too and then do:
> #ifdef CONFIG_PM
> ...
> #define DEV_PM_OPS &otp_pm_ops
> #else
> #define DEV_PM_OPS NULL
> #endif

Yes, that's much nicer. Thanks.

>
> > +static struct platform_driver otp_driver = {
> > + .remove = __devexit_p(otp_remove),
> > + .driver = {
> > + .name = "picoxcell-otp-pc3x3",
> > + .pm = &otp_pm_ops,
> > + },
> > +};
> >
> > +static int __init pc3x3_otp_init(void)
> > +{
> > + return platform_driver_probe(&otp_driver, otp_probe);
> > +}
>
> why call probe yourself ? why not platform_driver_register() ?

I made this comment in another driver myself and another reviewer
pointed out that if the device isn't some kind of hotplug device then it
probably isn't needed to let the bus do the matching and probing but I'm
happy to do what you've suggested, it feels a bit more natural anyway!

Jamie
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Jens Axboe: "Re: [GIT PULL] Core block IO bits for 2.6.39 - early Oops"
Previous message: Ingo Molnar: "Re: [PATCH RFC] x86: avoid atomic operation in test_and_set_bit_lockif possible"
In reply to: Mike Frysinger: "Re: [RFC PATCHv2 2/4] drivers/otp: add support for Picoxcell PC3X3 OTP"
Next in thread: Mike Frysinger: "Re: [RFC PATCHv2 2/4] drivers/otp: add support for Picoxcell PC3X3 OTP"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]