Message-ID: <AANLkTikaO3_WgFjF04+kfQ_vxxpHhCmC1mpHJvjo7ZtS@mail.gmail.com>
Date:	Thu, 24 Mar 2011 14:50:35 -0400
From:	Mike Frysinger <vapier@...too.org>
To:	Jamie Iles <jamie@...ieiles.com>
Cc:	linux-kernel@...r.kernel.org, gregkh@...e.de
Subject: Re: [RFC PATCHv2 2/4] drivers/otp: add support for Picoxcell PC3X3 OTP

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 ?

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

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

> +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() ?

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

"neccessary" -> "necessary"

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

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

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

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

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

> +static int __devinit otp_probe(struct platform_device *pdev)

namespace this ?  so call it "pc3x3_otp_probe" ...

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

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

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

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

> +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() ?
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists