Following some feedback from Greg, I've updated this series to be more
of a generic OTP layer. Everything is now registered under the "otp"
bus and I've also converted the blackfin OTP driver to use this
framework (which is the only current OTP driver I could find).
Mike, I wasn't 100% sure how big the blackfin OTP is but I found a
datasheet talking about 64KB so I've assumed that for now.
Main changes since v1:
- both devices and regions are now on the "otp" bus.
- moved to drivers/otp.
- converted bfin-otp to the generic OTP layer.
Jamie Iles (4):
drivers/otp: add initial support for OTP memory
drivers/otp: add support for Picoxcell PC3X3 OTP
drivers/otp: allow an ioctl to be specified
drivers/otp: convert bfin otp to generic OTP
Documentation/ABI/testing/sysfs-bus-otp | 68 ++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/char/Kconfig | 28 -
drivers/char/Makefile | 1 -
drivers/otp/Kconfig | 49 ++
drivers/otp/Makefile | 3 +
drivers/{char => otp}/bfin-otp.c | 174 +++---
drivers/otp/otp.c | 896 +++++++++++++++++++++++++
drivers/otp/otp_pc3x3.c | 1079 +++++++++++++++++++++++++++++++
include/linux/otp.h | 224 +++++++
11 files changed, 2397 insertions(+), 128 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-otp
create mode 100644 drivers/otp/Kconfig
create mode 100644 drivers/otp/Makefile
rename drivers/{char => otp}/bfin-otp.c (54%)
create mode 100644 drivers/otp/otp.c
create mode 100644 drivers/otp/otp_pc3x3.c
create mode 100644 include/linux/otp.h
--
1.7.4
The OTP in PC3X3 is 16KB and can be split into 1, 2, 4 or 8
regions each taking a different redundancy format. The formats
increase the redundancy of the memory at the expense of bit storage
to help protect against programming errors for devices in the field.
Signed-off-by: Jamie Iles <[email protected]>
---
drivers/otp/Kconfig | 11 +
drivers/otp/Makefile | 1 +
drivers/otp/otp_pc3x3.c | 1079 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 1091 insertions(+), 0 deletions(-)
create mode 100644 drivers/otp/otp_pc3x3.c
diff --git a/drivers/otp/Kconfig b/drivers/otp/Kconfig
index 5694216..9306ef6 100644
--- 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
+ 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.
+
+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;
+}
+
+/*
+ * Find the byte offset of the first word in the region from the base of the
+ * OTP.
+ */
+static unsigned otp_region_base(struct pc3x3_otp *otp,
+ const struct otp_region *region)
+{
+ int num_regions = otp_num_regions(otp);
+ unsigned real_region_sz = SZ_16K / num_regions;
+
+ return (region->region_nr * real_region_sz) / OTP_WORD_SIZE;
+}
+
+static void otp_do_command(struct pc3x3_otp *otp, enum otp_command cmd)
+{
+ otp_write_reg(otp, OTP_MACRO_CMD_REG_OFFSET, cmd);
+ wmb();
+
+ /*
+ * If we're talking to OTP then we need to wait for the command to
+ * finish.
+ */
+ if (!test_mode)
+ while (otp_read_reg(otp, OTP_MACRO_CMD_REG_OFFSET) !=
+ OTP_COMMAND_IDLE)
+ cpu_relax();
+}
+
+static void otp_write_MR(struct pc3x3_otp *otp, u32 value)
+{
+ /* Load the data register with the new contents. */
+ otp_write_reg(otp, OTP_MACRO_D_LO_REG_OFFSET, value);
+ otp_write_reg(otp, OTP_MACRO_D_HI_REG_OFFSET, 0);
+
+ /* Write the register and wait for the write to complete. */
+ otp_do_command(otp, OTP_COMMAND_WRITE_MR);
+}
+
+/*
+ * Create a write function for a given OTP auxillary mode register. This
+ * writes the auxillary mode register through the mode register then restores
+ * the contents of the mode register.
+ */
+#define OTP_REG_WRITE_FUNCTIONS(_name) \
+static void otp_write_##_name(struct pc3x3_otp *otp, u32 value) \
+{ \
+ u32 mr = otp_read_reg(otp, OTP_MACRO_Q_MR_REG_OFFSET); \
+ \
+ /* Load the data register with the new contents. */ \
+ otp_write_reg(otp, OTP_MACRO_D_LO_REG_OFFSET, value); \
+ otp_write_reg(otp, OTP_MACRO_D_HI_REG_OFFSET, 0); \
+ \
+ /* Write the register and wait for the write to complete. */ \
+ otp_do_command(otp, OTP_COMMAND_WRITE_##_name); \
+ \
+ /* Restore the original value of the MR. */ \
+ otp_write_MR(otp, mr); \
+}
+
+OTP_REG_WRITE_FUNCTIONS(MRA);
+OTP_REG_WRITE_FUNCTIONS(MRB);
+
+/*
+ * Enable the charge pump. This monitors the VPP voltage and waits for it to
+ * reach the correct programming level.
+ *
+ * @enable set to non-zero to enable the charge pump, zero to disable 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);
+}
+
+/*
+ * Read a word from OTP.
+ *
+ * @addr the word address to read from.
+ * @val the destination to store the value in.
+ *
+ * Prerequisites: the OTP must be in single-ended read mode so that we can
+ * correctly read the raw word.
+ */
+static int otp_raw_read_word(struct pc3x3_otp *otp, unsigned addr, u64 *val)
+{
+ if (SR_ADDRESS_0 == addr && test_mode)
+ *val = otp->test_mode_sr0;
+ else if (SR_ADDRESS_2 == addr && test_mode)
+ *val = otp->test_mode_sr2;
+ else {
+ union {
+ u64 d64;
+ u32 d32[2];
+ } converter;
+
+ otp_write_reg(otp, OTP_MACRO_ADDR_REG_OFFSET, addr);
+ otp_do_command(otp, OTP_COMMAND_READ);
+
+ converter.d32[0] = otp_read_reg(otp, OTP_MACRO_Q_LO_REG_OFFSET);
+ converter.d32[1] = otp_read_reg(otp, OTP_MACRO_Q_HI_REG_OFFSET);
+
+ if (!test_mode)
+ *val = converter.d64;
+ else
+ memcpy(val, otp->mem + addr * sizeof(u64), sizeof(u64));
+ }
+
+ return 0;
+}
+
+/*
+ * Program a word of OTP to a raw address. This will program an absolute value
+ * into the OTP so if the current word needs to be modified then this needs to
+ * be done with a read-modify-write cycle with the read-modify handled above.
+ *
+ * The actual write operation can't fail here but we don't do any verification
+ * to make sure that the correct data got written. That must be handled by the
+ * layer above.
+ */
+static void otp_raw_program_word(struct pc3x3_otp *otp, unsigned addr, u64 v)
+{
+ unsigned bit_offs;
+ u64 tmp;
+ int set_to_program = addr & 1 ? 0 : 1;
+
+ if (test_mode) {
+ if (addr != SR_ADDRESS_0 && addr != SR_ADDRESS_2) {
+ u64 old;
+
+ if (otp_raw_read_word(otp, addr, &old))
+ return;
+
+ v = (addr & 1) ? old & ~v : old | v;
+
+ memcpy(otp->mem + (addr * OTP_WORD_SIZE), &v,
+ sizeof(v));
+ } else {
+ /*
+ * The special register OTP values are stored in the
+ * boot rows that live outside of the 16KB of normal
+ * OTP so we can't address them directly.
+ */
+ if (SR_ADDRESS_0 == addr)
+ otp->test_mode_sr0 |= v;
+ else
+ otp->test_mode_sr2 |= v;
+ }
+ }
+
+ /* Set the address of the word that we're writing. */
+ otp_write_reg(otp, OTP_MACRO_ADDR_REG_OFFSET, addr);
+
+ for (bit_offs = 0; v && bit_offs < 64; ++bit_offs, v >>= 1) {
+ if (!(v & 0x1))
+ continue;
+
+ tmp = set_to_program ? ~(1LLU << bit_offs) :
+ (1LLU << bit_offs);
+ otp_write_reg(otp, OTP_MACRO_D_LO_REG_OFFSET,
+ (u32)tmp & 0xFFFFFFFF);
+ otp_write_reg(otp, OTP_MACRO_D_HI_REG_OFFSET,
+ (u32)(tmp >> 32) & 0xFFFFFFFF);
+
+ /* Start programming the bit and wait for it to complete. */
+ otp_do_command(otp, OTP_COMMAND_WRITE_PROGRAM);
+ }
+}
+
+static inline void otp_set_program_pulse_len(struct pc3x3_otp *otp,
+ unsigned len)
+{
+#define OTP_TIME_PGM_PULSE_MASK 0x7FF
+ u32 v = otp_read_reg(otp, OTP_MACRO_TIME_PGM_REG_OFFSET);
+ v &= ~OTP_TIME_PGM_PULSE_MASK;
+ v |= len;
+ otp_write_reg(otp, OTP_MACRO_TIME_PGM_REG_OFFSET, v);
+}
+
+/*
+ * Write a raw word in OTP. This will program a word into OTP memory and do
+ * any read-modify-write that is neccessary. For example if address 0 contains
+ * 0x00ef, then writing 0xbe00 will result in address 0 containing 0xbeef.
+ * This does not handle redundancy - this should be done at a higher level.
+ *
+ * @addr the word address to write to.
+ * @val the value to program into the OTP.
+ *
+ * Prerequisites: the OTP must be in single-ended read mode so that we can
+ * correctly verify the word.
+ */
+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)
+
+ /*
+ * Programming pulse times. For the normal pulse, we use a programming
+ * time of 51.2uS. For a soak pulse where bits fail to program we use
+ * a 1mS pulse.
+ */
+#define OTP_NORMAL_PGM_PULSE_LENGTH 0x50
+#define OTP_SOAK_PGM_PULSE_LENGTH 0x61B
+
+ /*
+ * We program even addresses by setting 0 bits to one and programm odd
+ * addresses by clearing 1 bits to 0.
+ */
+ int set_to_program = addr & 1 ? 0 : 1;
+ int retries = 0, err = 0;
+ u64 orig, v;
+
+ if (otp_raw_read_word(otp, addr, &orig))
+ return -EIO;
+
+ v = set_to_program ? val & ~orig : ~val & orig;
+
+ /* 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);
+
+ while (retries < MAX_PROGRAM_RETRIES) {
+ /* Update orig so we only reprogram the unprogrammed bits. */
+ if (otp_raw_read_word(otp, addr, &orig)) {
+ err = -EIO;
+ break;
+ }
+
+ /* If we've programmed correctly we have nothing else to do. */
+ if (val == orig) {
+ err = 0;
+ break;
+ }
+
+ /* Reset the mode register. */
+ otp_write_MRB(otp,
+ OTP_MRB_VREF_ADJUST_0 | OTP_MRB_VREF_ADJUST_1 |
+ OTP_MRB_VREF_ADJUST_3 |
+ OTP_MRB_READ_TIMER_DELAY_CONTROL);
+ otp_do_command(otp, OTP_COMMAND_RESET_MR);
+
+ /* Increase the programming pulse length. */
+ otp_set_program_pulse_len(otp, OTP_SOAK_PGM_PULSE_LENGTH);
+
+ /* Work out the failed bits. */
+ v = set_to_program ? val & ~orig : ~val & orig;
+ otp_raw_program_word(otp, addr, v);
+
+ /* Restore the programming pulse length. */
+ otp_set_program_pulse_len(otp, OTP_NORMAL_PGM_PULSE_LENGTH);
+
+ /* Update orig so we only reprogram the unprogrammed bits. */
+ if (otp_raw_read_word(otp, addr, &orig)) {
+ err = -EIO;
+ break;
+ }
+
+ /* If we've programmed correctly we have nothing else to do. */
+ if (val == orig) {
+ err = 0;
+ break;
+ }
+
+ 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);
+ udelay(1);
+ ++retries;
+ }
+
+ /* Disable the charge pump. We're done now. */
+ otp_charge_pump_enable(otp, 0);
+ otp_write_MRB(otp, 0);
+ otp_write_MRA(otp, 0);
+ otp_do_command(otp, OTP_COMMAND_RESET_MR);
+
+ if (!err && retries >= MAX_PROGRAM_RETRIES) {
+ dev_warn(&otp->dev->dev,
+ "writing to raw address %x failed to program after %d attempts\n",
+ addr, MAX_PROGRAM_RETRIES);
+ err = -EBADMSG;
+ }
+
+ return err;
+}
+
+/*
+ * Set the redundancy mode to a specific format. This only affects the
+ * readback through the AXI map and does not store the redundancy format in
+ * the special register.
+ */
+static void __pc3x3_otp_redundancy_mode_set(struct pc3x3_otp *otp,
+ enum otp_redundancy_fmt fmt)
+{
+#define OTP_MR_REDUNDANT_READ_MASK (1 << 4)
+#define OTP_MR_DIFFERENTIAL_READ_MASK (1 << 0)
+ u32 mr_lo = 0;
+
+ if (OTP_REDUNDANCY_FMT_REDUNDANT == fmt)
+ mr_lo |= OTP_MR_REDUNDANT_READ_MASK;
+ else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL == fmt)
+ mr_lo |= OTP_MR_DIFFERENTIAL_READ_MASK;
+ else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT == fmt)
+ mr_lo |= OTP_MR_REDUNDANT_READ_MASK |
+ OTP_MR_DIFFERENTIAL_READ_MASK;
+
+ /* Load the data register with the new MR contents. */
+ otp_write_reg(otp, OTP_MACRO_D_LO_REG_OFFSET, mr_lo);
+ otp_write_reg(otp, OTP_MACRO_D_HI_REG_OFFSET, 0);
+
+ /* Write the MR and wait for the write to complete. */
+ otp_do_command(otp, OTP_COMMAND_WRITE_MR);
+}
+
+static int pc3x3_otp_redundancy_mode_set(struct otp_device *dev,
+ enum otp_redundancy_fmt fmt)
+{
+ struct pc3x3_otp *otp = dev_get_drvdata(&dev->dev);
+
+ __pc3x3_otp_redundancy_mode_set(otp, fmt);
+
+ return 0;
+}
+
+/*
+ * Read a word from a specificied OTP region. The address is the user address
+ * for the word to be read and should not take the redundancy into account.
+ */
+static int pc3x3_otp_region_read_word(struct otp_region *region,
+ unsigned long addr, u64 *word)
+{
+ struct pc3x3_otp *otp = dev_get_drvdata(region->dev.parent);
+ enum otp_redundancy_fmt fmt = __pc3x3_otp_region_get_fmt(otp, region);
+ unsigned num_words, raw_addresses[4];
+ u64 result = 0, raw_values[4];
+ int err = 0;
+
+ /* Enter the single-ended read mode. */
+ __pc3x3_otp_redundancy_mode_set(otp, OTP_REDUNDANCY_FMT_SINGLE_ENDED);
+
+ /*
+ * If we're running with real OTP then the read is simple, just copy
+ * it from the AXI map.
+ */
+ if (!test_mode) {
+ memcpy(word, otp->mem + (otp_region_base(otp, region) + addr) *
+ OTP_WORD_SIZE, sizeof(*word));
+ return 0;
+ }
+
+ /*
+ * If we're in test mode then this is slightly more complicated. We
+ * need to decode the address into the raw address(s) that the block
+ * uses and handle the redundancy format. This allows us to test that
+ * we've programmed all of the redundant words in the correct format.
+ */
+ switch (fmt) {
+ case OTP_REDUNDANCY_FMT_SINGLE_ENDED:
+ num_words = 1;
+ raw_addresses[0] = otp_region_base(otp, region) + addr;
+ otp_raw_read_word(otp, raw_addresses[0], &raw_values[0]);
+ result = raw_values[0];
+ break;
+
+ case OTP_REDUNDANCY_FMT_REDUNDANT:
+ num_words = 2;
+ raw_addresses[0] = otp_region_base(otp, region) +
+ (((addr & 0xFFFE) << 1) | (addr & 1));
+ raw_addresses[1] = otp_region_base(otp, region) +
+ (((addr & 0xFFFE) << 1) | (addr & 1) | 2);
+ otp_raw_read_word(otp, raw_addresses[0], &raw_values[0]);
+ otp_raw_read_word(otp, raw_addresses[1], &raw_values[1]);
+ result = raw_values[0] | raw_values[1];
+ break;
+
+ case OTP_REDUNDANCY_FMT_DIFFERENTIAL:
+ num_words = 2;
+ raw_addresses[0] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 1));
+ raw_addresses[1] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 1) | 1);
+ otp_raw_read_word(otp, raw_addresses[0], &raw_values[0]);
+ otp_raw_read_word(otp, raw_addresses[1], &raw_values[1]);
+ result = raw_values[0] | ~raw_values[1];
+ break;
+
+ case OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT:
+ num_words = 4;
+ raw_addresses[0] = otp_region_base(otp, region) +
+ ((addr & 0xFFFF) << 2);
+ raw_addresses[1] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 2) | 0x1);
+ raw_addresses[2] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 2) | 0x2);
+ raw_addresses[3] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 2) | 0x3);
+ otp_raw_read_word(otp, raw_addresses[0], &raw_values[0]);
+ otp_raw_read_word(otp, raw_addresses[1], &raw_values[1]);
+ otp_raw_read_word(otp, raw_addresses[2], &raw_values[2]);
+ otp_raw_read_word(otp, raw_addresses[3], &raw_values[3]);
+ result = (raw_values[0] | ~raw_values[1]) |
+ (raw_values[2] | ~raw_values[3]);
+ break;
+
+ default:
+ err = -EINVAL;
+ }
+
+ *word = result;
+
+ return err;
+}
+
+/*
+ * Write a data word to an OTP region. The value will be used in a
+ * read-modify-write cycle to ensure that bits can't be flipped if they have
+ * already programmed (the hardware isn't capable of this). This also takes
+ * into account the redundancy addressing and formatting.
+ */
+static int pc3x3_otp_region_write_word(struct otp_region *region,
+ unsigned long addr,
+ u64 word)
+{
+ struct pc3x3_otp *otp = dev_get_drvdata(region->dev.parent);
+ enum otp_redundancy_fmt fmt = __pc3x3_otp_region_get_fmt(otp, region);
+ unsigned i, num_words, raw_addresses[4];
+ u64 result;
+ int err = 0;
+
+ /* Enter the single-ended read mode. */
+ __pc3x3_otp_redundancy_mode_set(otp, OTP_REDUNDANCY_FMT_SINGLE_ENDED);
+
+ /*
+ * Work out what raw addresses and values we need to write into the
+ * OTP to make sure that the value we want gets read back out
+ * correctly.
+ */
+ switch (fmt) {
+ case OTP_REDUNDANCY_FMT_SINGLE_ENDED:
+ num_words = 1;
+ raw_addresses[0] = otp_region_base(otp, region) + addr;
+ break;
+
+ case OTP_REDUNDANCY_FMT_REDUNDANT:
+ num_words = 2;
+ raw_addresses[0] = otp_region_base(otp, region) +
+ (((addr & 0xFFFE) << 1) | (addr & 1));
+ raw_addresses[1] = otp_region_base(otp, region) +
+ (((addr & 0xFFFE) << 1) | (addr & 1) | 2);
+ break;
+
+ case OTP_REDUNDANCY_FMT_DIFFERENTIAL:
+ num_words = 2;
+ raw_addresses[0] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 1));
+ raw_addresses[1] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 1) | 1);
+ break;
+
+ case OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT:
+ num_words = 4;
+ raw_addresses[0] = otp_region_base(otp, region) +
+ ((addr & 0xFFFF) << 2);
+ raw_addresses[1] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 2) | 0x1);
+ raw_addresses[2] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 2) | 0x2);
+ raw_addresses[3] = otp_region_base(otp, region) +
+ (((addr & 0xFFFF) << 2) | 0x3);
+ break;
+
+ default:
+ err = -EINVAL;
+ goto out;
+ }
+
+ /*
+ * Verify the raw words. If we are doing strict programming then they
+ * must all program correctly. If we aren't doing strict programming
+ * then allow failures to 'slip through' for now. If the word can be
+ * read back correctly in the redundant mode then that's fine with the
+ * user.
+ */
+ for (i = 0; i < num_words; ++i)
+ err = otp_raw_write_word(otp, raw_addresses[i], word);
+ if (err && otp_strict_programming_enabled())
+ goto out;
+
+ /* Go back to the real redundancy mode and verify the whole word. */
+ __pc3x3_otp_redundancy_mode_set(otp, fmt);
+
+ if (region->ops->read_word(region, addr, &result)) {
+ err = -EIO;
+ goto out;
+ }
+
+ /*
+ * Now check that the word has been correctly programmed with the
+ * region formatting.
+ */
+ if (result == word) {
+ err = 0;
+ } else {
+ dev_warn(®ion->dev,
+ "word at address %lx write failed %llx != %llx (result != expected)\n",
+ addr, result, word);
+ err = -EBADMSG;
+ }
+
+out:
+ return err;
+}
+
+/*
+ * Write the special register. In PC3X3, we only use the lower 32 bits of the
+ * SR to indicate the partitioning and the region formats so we do a
+ * read-modify-write of the whole 64 bit value.
+ */
+static int otp_write_sr(struct pc3x3_otp *otp, u32 sr_lo)
+{
+ if (otp_raw_write_word(otp, SR_ADDRESS_0, sr_lo)) {
+ dev_warn(&otp->dev->dev,
+ "failed to write special register (word 0)\n");
+ return -EIO;
+ }
+
+ if (otp_raw_write_word(otp, SR_ADDRESS_2, sr_lo)) {
+ dev_warn(&otp->dev->dev,
+ "failed to write special register (word 0)\n");
+ return -EIO;
+ }
+
+ /*
+ * Reset the OTP so that when we read the special register again we
+ * get the value that we've just written.
+ */
+ otp_do_command(otp, OTP_COMMAND_RESET);
+
+ return 0;
+}
+
+
+static int pc3x3_otp_region_set_fmt(struct otp_region *region,
+ enum otp_redundancy_fmt new_fmt)
+{
+ int err;
+ struct pc3x3_otp *otp = dev_get_drvdata(region->dev.parent);
+ enum otp_redundancy_fmt fmt = __pc3x3_otp_region_get_fmt(otp, region);
+ unsigned shift = (region->region_nr * 2) + 4;
+ unsigned long sr;
+
+ /*
+ * We can't clear format bits so we can only do certain transitions.
+ * It is possible to go from redundant to differential-redundant or
+ * differential to differential redundant but if the region is already
+ * programmed this could give unexpected results. However, the user
+ * _might_ know what they're doing.
+ */
+ if (fmt & ~new_fmt) {
+ err = -EINVAL;
+ goto out;
+ }
+ if (fmt == new_fmt) {
+ err = 0;
+ goto out;
+ }
+
+ sr = otp_read_sr(otp);
+ sr |= new_fmt << shift;
+ err = otp_write_sr(otp, sr);
+
+out:
+ return err;
+}
+
+/*
+ * Find out how big the region is. We have a 16KB device which can be split
+ * equally into 1, 2, 4 or 8 regions. If a partition is redundant or
+ * differential redundancy then this is 2 bits of storage per data bit so half
+ * the size. For differential-redundant redundancy, 1 bit of data takes 4 bits
+ * of storage so divide by 4.
+ */
+static ssize_t pc3x3_otp_region_get_size(struct otp_region *region)
+{
+ struct pc3x3_otp *otp = dev_get_drvdata(region->dev.parent);
+ int num_regions = otp_num_regions(otp);
+ enum otp_redundancy_fmt fmt = __pc3x3_otp_region_get_fmt(otp, region);
+ size_t region_sz;
+
+ region_sz = (SZ_16K / num_regions);
+ if (OTP_REDUNDANCY_FMT_REDUNDANT == fmt ||
+ OTP_REDUNDANCY_FMT_DIFFERENTIAL == fmt)
+ region_sz /= 2;
+ else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT == fmt)
+ region_sz /= 4;
+
+ return region_sz;
+}
+
+static const struct otp_region_ops pc3x3_region_ops = {
+ .set_fmt = pc3x3_otp_region_set_fmt,
+ .get_fmt = pc3x3_otp_region_get_fmt,
+ .write_word = pc3x3_otp_region_write_word,
+ .read_word = pc3x3_otp_region_read_word,
+ .get_size = pc3x3_otp_region_get_size,
+};
+
+static int pc3x3_otp_register_regions(struct pc3x3_otp *dev,
+ bool need_unlocked)
+{
+ struct otp_device *otp = dev->dev;
+ int err = 0, i, nr_regions = otp->ops->get_nr_regions(otp);
+
+ for (i = 0; i < nr_regions; ++i) {
+ struct otp_region *region;
+
+ if (test_and_set_bit(i, &dev->registered_regions))
+ continue;
+
+ region = need_unlocked ?
+ otp_region_alloc_unlocked(otp, &pc3x3_region_ops, i) :
+ otp_region_alloc(otp, &pc3x3_region_ops, i);
+ if (IS_ERR(region)) {
+ err = PTR_ERR(region);
+ break;
+ }
+ }
+
+ return err;
+}
+
+static int pc3x3_otp_set_nr_regions(struct otp_device *dev, int nr_regions)
+{
+ struct pc3x3_otp *otp = dev_get_drvdata(&dev->dev);
+ unsigned long sr = otp_read_sr(otp);
+ u32 new_mask, addr_mask = sr & SR_AXI_ADDRESS_MASK;
+ int err = 0;
+
+ if (1 == nr_regions)
+ new_mask = 0;
+ else if (2 == nr_regions)
+ new_mask = 4;
+ else if (4 == nr_regions)
+ new_mask = 6;
+ else if (8 == nr_regions)
+ new_mask = 7;
+ else
+ err = -EINVAL;
+
+ if (err)
+ goto out;
+
+ /*
+ * Check that we aren't trying to clear any bits and reduce the number
+ * of regions. This is OTP so we can only increase.
+ */
+ if (addr_mask & ~new_mask) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ if (addr_mask == new_mask) {
+ err = 0;
+ goto out;
+ }
+
+ err = otp_write_sr(otp, sr | new_mask);
+ if (err)
+ goto out;
+
+ err = pc3x3_otp_register_regions(otp, true);
+
+out:
+ return err;
+}
+
+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;
+}
+
+static const struct otp_device_ops pc3x3_otp_ops = {
+ .name = "PC3X3",
+ .owner = THIS_MODULE,
+ .get_nr_regions = pc3x3_otp_get_nr_regions,
+ .set_nr_regions = pc3x3_otp_set_nr_regions,
+ .set_fmt = pc3x3_otp_redundancy_mode_set,
+};
+
+static int __devinit otp_probe(struct platform_device *pdev)
+{
+ int err;
+ struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ struct otp_device *otp;
+ struct pc3x3_otp *pc3x3_dev;
+
+ if (!mem) {
+ dev_err(&pdev->dev, "no i/o memory\n");
+ return -ENXIO;
+ }
+
+ 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;
+
+ if (test_mode) {
+ u64 *p = devm_kzalloc(&pdev->dev, SZ_16K + SZ_1K, GFP_KERNEL);
+ int i;
+
+ if (!p) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ pc3x3_dev->mem = p;
+ pc3x3_dev->iomem = (void __force __iomem *)p;
+
+ for (i = 0; (u8 *)p < (u8 *)pc3x3_dev->mem + SZ_16K + SZ_1K;
+ ++p, ++i)
+ *p = (i & 1) ? ~0LLU : 0LLU;
+ } else {
+ pc3x3_dev->iomem = devm_ioremap(&pdev->dev, mem->start,
+ resource_size(mem));
+ if (!pc3x3_dev->iomem) {
+ err = -ENOMEM;
+ goto out;
+ }
+ pc3x3_dev->mem = (void __force *)pc3x3_dev->iomem;
+ }
+
+ pc3x3_dev->clk = clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pc3x3_dev->clk)) {
+ dev_err(&pdev->dev, "device has no clk\n");
+ err = PTR_ERR(pc3x3_dev->clk);
+ goto out;
+ }
+ clk_enable(pc3x3_dev->clk);
+
+ otp = otp_device_alloc(&pdev->dev, &pc3x3_otp_ops, SZ_16K);
+ if (IS_ERR(otp)) {
+ err = PTR_ERR(otp);
+ goto out_clk_disable;
+ }
+ dev_set_drvdata(&otp->dev, pc3x3_dev);
+
+ pc3x3_dev->dev = otp;
+ platform_set_drvdata(pdev, pc3x3_dev);
+
+ err = pc3x3_otp_register_regions(pc3x3_dev, false);
+ if (err)
+ goto out_unregister;
+
+ goto out;
+
+out_unregister:
+ otp_device_unregister(otp);
+out_clk_disable:
+ clk_disable(pc3x3_dev->clk);
+ clk_put(pc3x3_dev->clk);
+out:
+ return err;
+}
+
+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;
+}
+
+#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,
+};
+
+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);
+}
+module_init(pc3x3_otp_init);
+
+static void __exit pc3x3_otp_exit(void)
+{
+ platform_driver_unregister(&otp_driver);
+}
+module_exit(pc3x3_otp_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jamie Iles");
+MODULE_DESCRIPTION("OTP memory driver for Picochip PC3X3 devices");
--
1.7.4
Convert the blackfin OTP driver to the generic OTP layer.
Cc: Mike Frysinger <[email protected]>
Signed-off-by: Jamie Iles <[email protected]>
---
drivers/char/Kconfig | 28 ------
drivers/char/Makefile | 1 -
drivers/otp/Kconfig | 28 ++++++
drivers/otp/Makefile | 1 +
drivers/{char => otp}/bfin-otp.c | 174 ++++++++++++++++---------------------
5 files changed, 104 insertions(+), 128 deletions(-)
rename drivers/{char => otp}/bfin-otp.c (54%)
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 04f8b2d..21ad035 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -81,34 +81,6 @@ config BRIQ_PANEL
It's safe to say N here.
-config BFIN_OTP
- tristate "Blackfin On-Chip OTP Memory Support"
- depends on BLACKFIN && (BF51x || BF52x || BF54x)
- default y
- help
- If you say Y here, you will get support for a character device
- interface into the One Time Programmable memory pages that are
- stored on the Blackfin processor. This will not get you access
- to the secure memory pages however. You will need to write your
- own secure code and reader for that.
-
- To compile this driver as a module, choose M here: the module
- will be called bfin-otp.
-
- If unsure, it is safe to say Y.
-
-config BFIN_OTP_WRITE_ENABLE
- bool "Enable writing support of OTP pages"
- depends on BFIN_OTP
- default n
- help
- If you say Y here, you will enable support for writing of the
- OTP pages. This is dangerous by nature as you can only program
- the pages once, so only enable this option when you actually
- need it so as to not inadvertently clobber data.
-
- If unsure, say N.
-
config PRINTER
tristate "Parallel printer support"
depends on PARPORT
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 057f654..35b2403 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -16,7 +16,6 @@ obj-$(CONFIG_VIOTAPE) += viotape.o
obj-$(CONFIG_IBM_BSR) += bsr.o
obj-$(CONFIG_SGI_MBCS) += mbcs.o
obj-$(CONFIG_BRIQ_PANEL) += briq_panel.o
-obj-$(CONFIG_BFIN_OTP) += bfin-otp.o
obj-$(CONFIG_PRINTER) += lp.o
diff --git a/drivers/otp/Kconfig b/drivers/otp/Kconfig
index 9306ef6..15cc5fa 100644
--- a/drivers/otp/Kconfig
+++ b/drivers/otp/Kconfig
@@ -18,4 +18,32 @@ config OTP_PC3X3
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.
+config BFIN_OTP
+ tristate "Blackfin On-Chip OTP Memory Support"
+ depends on BLACKFIN && (BF51x || BF52x || BF54x) && OTP
+ default y
+ help
+ If you say Y here, you will get support for a character device
+ interface into the One Time Programmable memory pages that are
+ stored on the Blackfin processor. This will not get you access
+ to the secure memory pages however. You will need to write your
+ own secure code and reader for that.
+
+ To compile this driver as a module, choose M here: the module
+ will be called bfin-otp.
+
+ If unsure, it is safe to say Y.
+
+config BFIN_OTP_WRITE_ENABLE
+ bool "Enable writing support of OTP pages"
+ depends on BFIN_OTP
+ default n
+ help
+ If you say Y here, you will enable support for writing of the
+ OTP pages. This is dangerous by nature as you can only program
+ the pages once, so only enable this option when you actually
+ need it so as to not inadvertently clobber data.
+
+ If unsure, say N.
+
endif
diff --git a/drivers/otp/Makefile b/drivers/otp/Makefile
index c710ec4..db79667 100644
--- a/drivers/otp/Makefile
+++ b/drivers/otp/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_OTP) += otp.o
obj-$(CONFIG_OTP_PC3X3) += otp_pc3x3.o
+obj-$(CONFIG_BFIN_OTP) += bfin-otp.o
diff --git a/drivers/char/bfin-otp.c b/drivers/otp/bfin-otp.c
similarity index 54%
rename from drivers/char/bfin-otp.c
rename to drivers/otp/bfin-otp.c
index 44660f1..002058c 100644
--- a/drivers/char/bfin-otp.c
+++ b/drivers/otp/bfin-otp.c
@@ -9,12 +9,13 @@
*/
#include <linux/device.h>
+#include <linux/err.h>
#include <linux/errno.h>
#include <linux/fs.h>
#include <linux/init.h>
#include <linux/miscdevice.h>
#include <linux/module.h>
-#include <linux/mutex.h>
+#include <linux/otp.h>
#include <linux/types.h>
#include <mtd/mtd-abi.h>
@@ -28,53 +29,35 @@
#define DRIVER_NAME "bfin-otp"
#define PFX DRIVER_NAME ": "
+#define BFIN_OTP_SIZE (64 * 1024)
-static DEFINE_MUTEX(bfin_otp_lock);
+static struct otp_device *bfin_otp;
/**
* bfin_otp_read - Read OTP pages
*
* All reads must be in half page chunks (half page == 64 bits).
*/
-static ssize_t bfin_otp_read(struct file *file, char __user *buff, size_t count, loff_t *pos)
+static int bfin_region_read_word(struct otp_region *region, unsigned long addr,
+ u64 *word)
{
- ssize_t bytes_done;
+ int err;
u32 page, flags, ret;
- u64 content;
stampit();
-
- if (count % sizeof(u64))
- return -EMSGSIZE;
-
- if (mutex_lock_interruptible(&bfin_otp_lock))
- return -ERESTARTSYS;
-
- bytes_done = 0;
- page = *pos / (sizeof(u64) * 2);
- while (bytes_done < count) {
- flags = (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
- stamp("processing page %i (0x%x:%s)", page, flags,
- (flags & OTP_UPPER_HALF ? "upper" : "lower"));
- ret = bfrom_OtpRead(page, flags, &content);
- if (ret & OTP_MASTER_ERROR) {
- stamp("error from otp: 0x%x", ret);
- bytes_done = -EIO;
- break;
- }
- if (copy_to_user(buff + bytes_done, &content, sizeof(content))) {
- bytes_done = -EFAULT;
- break;
- }
- if (flags & OTP_UPPER_HALF)
- ++page;
- bytes_done += sizeof(content);
- *pos += sizeof(content);
- }
-
- mutex_unlock(&bfin_otp_lock);
-
- return bytes_done;
+ page = addr / 2;
+ flags = (addr & 0x1) ? OTP_UPPER_HALF : OTP_LOWER_HALF;
+ stamp("processing page %i (0x%x:%s)", page, flags,
+ (flags & OTP_UPPER_HALF ? "upper" : "lower"));
+
+ err = bfrom_OtpRead(page, flags, word);
+ if (err & OTP_MASTER_ERROR) {
+ stamp("error from otp: 0x%x", ret);
+ err = -EIO;
+ } else
+ err = 0;
+
+ return err;
}
#ifdef CONFIG_BFIN_OTP_WRITE_ENABLE
@@ -117,61 +100,39 @@ static void bfin_otp_deinit_timing(u32 timing)
*
* All writes must be in half page chunks (half page == 64 bits).
*/
-static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t count, loff_t *pos)
+static int bfin_region_write_word(struct otp_region *region, unsigned long addr,
+ u64 content)
{
- ssize_t bytes_done;
+ int err;
u32 timing, page, base_flags, flags, ret;
- u64 content;
if (!allow_writes)
return -EACCES;
- if (count % sizeof(u64))
- return -EMSGSIZE;
-
- if (mutex_lock_interruptible(&bfin_otp_lock))
- return -ERESTARTSYS;
-
stampit();
-
timing = bfin_otp_init_timing();
- if (timing == 0) {
- mutex_unlock(&bfin_otp_lock);
+ if (timing == 0)
return -EIO;
- }
-
base_flags = OTP_CHECK_FOR_PREV_WRITE;
- bytes_done = 0;
- page = *pos / (sizeof(u64) * 2);
- while (bytes_done < count) {
- flags = base_flags | (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
- stamp("processing page %i (0x%x:%s) from %p", page, flags,
- (flags & OTP_UPPER_HALF ? "upper" : "lower"), buff + bytes_done);
- if (copy_from_user(&content, buff + bytes_done, sizeof(content))) {
- bytes_done = -EFAULT;
- break;
- }
- ret = bfrom_OtpWrite(page, flags, &content);
- if (ret & OTP_MASTER_ERROR) {
- stamp("error from otp: 0x%x", ret);
- bytes_done = -EIO;
- break;
- }
- if (flags & OTP_UPPER_HALF)
- ++page;
- bytes_done += sizeof(content);
- *pos += sizeof(content);
- }
+ page = addr / 2;
+ flags = base_flags | (addr & 0x1) ? OTP_UPPER_HALF : OTP_LOWER_HALF;
+ stamp("processing page %i (0x%x:%s)", page, flags,
+ (flags & OTP_UPPER_HALF ? "upper" : "lower"));
+ ret = bfrom_OtpWrite(page, flags, &content);
+ if (ret & OTP_MASTER_ERROR) {
+ stamp("error from otp: 0x%x", ret);
+ err = -EIO;
+ } else
+ err = 0;
bfin_otp_deinit_timing(timing);
- mutex_unlock(&bfin_otp_lock);
-
- return bytes_done;
+ return err;
}
-static long bfin_otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
+static long bfin_region_ioctl(struct otp_region *region, unsigned cmd,
+ unsigned long arg)
{
stampit();
@@ -183,9 +144,6 @@ static long bfin_otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
if (!allow_writes)
return -EACCES;
- if (mutex_lock_interruptible(&bfin_otp_lock))
- return -ERESTARTSYS;
-
timing = bfin_otp_init_timing();
if (timing) {
u32 otp_result = bfrom_OtpWrite(arg, OTP_LOCK, NULL);
@@ -196,8 +154,6 @@ static long bfin_otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
bfin_otp_deinit_timing(timing);
}
- mutex_unlock(&bfin_otp_lock);
-
return ret;
}
@@ -213,22 +169,35 @@ static long bfin_otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
return -EINVAL;
}
#else
-# define bfin_otp_write NULL
-# define bfin_otp_ioctl NULL
+static int bfin_region_write_word(struct otp_region *region, unsigned long addr,
+ u64 val)
+{
+ return -EACCES;
+}
+# define bfin_region_ioctl NULL
#endif
-static const struct file_operations bfin_otp_fops = {
- .owner = THIS_MODULE,
- .unlocked_ioctl = bfin_otp_ioctl,
- .read = bfin_otp_read,
- .write = bfin_otp_write,
- .llseek = default_llseek,
+static int bfin_otp_get_nr_regions(struct otp_device *dev)
+{
+ return 1;
+}
+
+static const struct otp_device_ops bfin_otp_ops = {
+ .name = "BFIN",
+ .owner = THIS_MODULE,
+ .get_nr_regions = bfin_otp_get_nr_regions,
};
-static struct miscdevice bfin_otp_misc_device = {
- .minor = MISC_DYNAMIC_MINOR,
- .name = DRIVER_NAME,
- .fops = &bfin_otp_fops,
+static ssize_t bfin_region_get_size(struct otp_region *region)
+{
+ return BFIN_OTP_SIZE;
+}
+
+static const struct otp_region_ops bfin_region_ops = {
+ .read_word = bfin_region_read_word,
+ .write_word = bfin_region_write_word,
+ .get_size = bfin_region_get_size,
+ .ioctl = bfin_region_ioctl,
};
/**
@@ -239,14 +208,21 @@ static struct miscdevice bfin_otp_misc_device = {
*/
static int __init bfin_otp_init(void)
{
- int ret;
+ struct otp_region *region;
stampit();
- ret = misc_register(&bfin_otp_misc_device);
- if (ret) {
- pr_init(KERN_ERR PFX "unable to register a misc device\n");
- return ret;
+ bfin_otp = otp_device_alloc(NULL, &bfin_otp_ops, BFIN_OTP_SIZE);
+ if (IS_ERR(bfin_otp)) {
+ pr_init(KERN_ERR PFX "failed to create OTP device\n");
+ return PTR_ERR(bfin_otp);
+ }
+
+ region = otp_region_alloc(bfin_otp, &bfin_region_ops, 1);
+ if (IS_ERR(region)) {
+ pr_init(KERN_ERR PFX "failed to create OTP region\n");
+ otp_device_unregister(bfin_otp);
+ return PTR_ERR(region);
}
pr_init(KERN_INFO PFX "initialized\n");
@@ -264,7 +240,7 @@ static void __exit bfin_otp_exit(void)
{
stampit();
- misc_deregister(&bfin_otp_misc_device);
+ otp_device_unregister(bfin_otp);
}
module_init(bfin_otp_init);
--
1.7.4
OTP memory is typically found in some embedded devices and can be
used for storing boot code, cryptographic keys and other persistent
information onchip. This patch adds a generic layer that devices can
register OTP with and allows access through a set of character
device nodes.
Signed-off-by: Jamie Iles <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-otp | 68 +++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/otp/Kconfig | 10 +
drivers/otp/Makefile | 1 +
drivers/otp/otp.c | 878 +++++++++++++++++++++++++++++++
include/linux/otp.h | 221 ++++++++
7 files changed, 1181 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-otp
create mode 100644 drivers/otp/Kconfig
create mode 100644 drivers/otp/Makefile
create mode 100644 drivers/otp/otp.c
create mode 100644 include/linux/otp.h
diff --git a/Documentation/ABI/testing/sysfs-bus-otp b/Documentation/ABI/testing/sysfs-bus-otp
new file mode 100644
index 0000000..4dbc652
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-otp
@@ -0,0 +1,68 @@
+What: /sys/bus/otp/
+Date: March 2011
+KernelVersion: 2.6.40+
+Contact: Jamie Iles <[email protected]>
+Description:
+ The otp bus presents a number of devices where each
+ device represents a region or device in the SoC. Each region
+ will create a device node which allows the region to be
+ written with read()/write() calls and the device on the bus
+ has attributes for controlling the redundancy format and
+ getting the region size.
+
+What: /sys/bus/otp[a-z]/write_enable
+Date: March 2011
+KernelVersion: 2.6.40+
+Contact: "Jamie Iles" <[email protected]>
+Description:
+ This file controls whether the OTP device can be written to.
+ If set to "enabled" then the regions may be written, the
+ number of regions may be changed and the format of any region
+ may be changed.
+
+What: /sys/bus/otp[a-z]/num_regions
+Date: March 2011
+KernelVersion: 2.6.40+
+Contact: "Jamie Iles" <[email protected]>
+Description:
+ This file controls the number of regions in the OTP device.
+ Valid values are 1, 2, 4 and 8. The number of regions may be
+ increased but never decreased. Increasing the number of
+ regions will create new devices on the otp bus.
+
+What: /sys/bus/otp[a-z]/strict_programming
+Date: March 2011
+KernelVersion: 2.6.40+
+Contact: "Jamie Iles" <[email protected]>
+Description:
+ This file indicates whether all words in a redundant format
+ must be programmed correctly to indicate success. Disabling
+ this will mean that programming will be considered a success
+ if the word can be read back correctly in it's redundant
+ format.
+
+What: /sys/bus/otp/devices/otp[a-z][0-9]*/format
+Date: March 2011
+KernelVersion: 2.6.40+
+Contact: Jamie Iles <[email protected]>
+Description:
+ The redundancy format of the region. Valid values are:
+ - single-ended (1 bit of storage per data bit).
+ - redundant (2 bits of storage, wire-OR'd per data
+ bit).
+ - differential (2 bits of storage, differential
+ voltage per data bit).
+ - differential-redundant (4 bits of storage, combining
+ redundant and differential).
+ It is possible to increase redundancy of a region but care
+ will be needed if there is data already in the region.
+
+What: /sys/bus/otp/devices/otp[a-z][0-9]*/size
+Date: March 2011
+KernelVersion: 2.6.40+
+Contact: Jamie Iles <[email protected]>
+Description:
+ The effective storage size of the region. This is the amount
+ of data that a user can store in the region taking into
+ account the number of regions and the redundancy format of the
+ region itself.
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 177c7d1..77da156 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -119,4 +119,6 @@ source "drivers/platform/Kconfig"
source "drivers/clk/Kconfig"
source "drivers/hwspinlock/Kconfig"
+
+source "drivers/otp/Kconfig"
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 3f135b6..6ae2f815 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -119,3 +119,4 @@ obj-y += ieee802154/
obj-y += clk/
obj-$(CONFIG_HWSPINLOCK) += hwspinlock/
+obj-$(CONFIG_OTP) += otp/
diff --git a/drivers/otp/Kconfig b/drivers/otp/Kconfig
new file mode 100644
index 0000000..5694216
--- /dev/null
+++ b/drivers/otp/Kconfig
@@ -0,0 +1,10 @@
+#
+# Character device configuration
+#
+
+menuconfig OTP
+ bool "OTP memory support"
+ help
+ 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.
diff --git a/drivers/otp/Makefile b/drivers/otp/Makefile
new file mode 100644
index 0000000..84fd03e
--- /dev/null
+++ b/drivers/otp/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_OTP) += otp.o
diff --git a/drivers/otp/otp.c b/drivers/otp/otp.c
new file mode 100644
index 0000000..dd47cf1
--- /dev/null
+++ b/drivers/otp/otp.c
@@ -0,0 +1,878 @@
+/*
+ * 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.
+ */
+#define pr_fmt(fmt) "otp: " fmt
+
+#undef DEBUG
+#include <linux/cdev.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/otp.h>
+#include <linux/semaphore.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/uaccess.h>
+
+static int otp_open(struct inode *inode, struct file *filp);
+static int otp_release(struct inode *inode, struct file *filp);
+static ssize_t otp_write(struct file *filp, const char __user *buf,
+ size_t len, loff_t *offs);
+static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
+ loff_t *offs);
+static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
+
+static const struct file_operations otp_fops = {
+ .owner = THIS_MODULE,
+ .open = otp_open,
+ .release = otp_release,
+ .write = otp_write,
+ .read = otp_read,
+ .llseek = otp_llseek,
+};
+
+static DEFINE_SEMAPHORE(otp_sem);
+static int otp_we, otp_strict_programming;
+static struct otp_device *otp;
+static dev_t otp_devno;
+
+/*
+ * Given a device for one of the otpN devices, get the corresponding
+ * otp_region.
+ */
+static inline struct otp_region *to_otp_region(struct device *dev)
+{
+ return dev ? container_of(dev, struct otp_region, dev) : NULL;
+}
+
+static inline struct otp_device *to_otp_device(struct device *dev)
+{
+ return dev ? container_of(dev, struct otp_device, dev) : NULL;
+}
+
+bool otp_strict_programming_enabled(void)
+{
+ return otp_strict_programming;
+}
+EXPORT_SYMBOL_GPL(otp_strict_programming_enabled);
+
+static ssize_t otp_format_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct otp_region *region = to_otp_region(dev);
+ enum otp_redundancy_fmt fmt;
+ const char *fmt_string;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ if (region->ops->get_fmt(region))
+ fmt = region->ops->get_fmt(region);
+ else
+ fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
+
+ up(&otp_sem);
+
+ if (OTP_REDUNDANCY_FMT_SINGLE_ENDED == fmt)
+ fmt_string = "single-ended";
+ else if (OTP_REDUNDANCY_FMT_REDUNDANT == fmt)
+ fmt_string = "redundant";
+ else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL == fmt)
+ fmt_string = "differential";
+ else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT == fmt)
+ fmt_string = "differential-redundant";
+ else
+ fmt_string = NULL;
+
+ return fmt_string ? sprintf(buf, "%s\n", fmt_string) : -EINVAL;
+}
+
+static ssize_t otp_format_store(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t len)
+{
+ int err = 0;
+ struct otp_region *region = to_otp_region(dev);
+ enum otp_redundancy_fmt new_fmt;
+
+ if (!region->ops->set_fmt)
+ return -EOPNOTSUPP;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ /* This is irreversible so don't make it too easy to break it! */
+ if (!otp_we) {
+ err = -EPERM;
+ goto out;
+ }
+
+ if (sysfs_streq(buf, "single-ended"))
+ new_fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
+ else if (sysfs_streq(buf, "redundant"))
+ new_fmt = OTP_REDUNDANCY_FMT_REDUNDANT;
+ else if (sysfs_streq(buf, "differential"))
+ new_fmt = OTP_REDUNDANCY_FMT_DIFFERENTIAL;
+ else if (sysfs_streq(buf, "differential-redundant"))
+ new_fmt = OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT;
+ else {
+ err = -EINVAL;
+ goto out;
+ }
+
+ region->ops->set_fmt(region, new_fmt);
+
+out:
+ up(&otp_sem);
+
+ return err ?: len;
+}
+static DEVICE_ATTR(format, S_IRUSR | S_IWUSR, otp_format_show,
+ otp_format_store);
+
+static ssize_t otp_size_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct otp_region *region = to_otp_region(dev);
+ size_t region_sz;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ region_sz = region->ops->get_size(region);
+
+ up(&otp_sem);
+
+ return sprintf(buf, "%zu\n", region_sz);
+}
+static DEVICE_ATTR(size, S_IRUSR, otp_size_show, NULL);
+
+
+static struct bus_type otp_bus_type = {
+ .name = "otp",
+};
+
+struct attribute *region_attrs[] = {
+ &dev_attr_format.attr,
+ &dev_attr_size.attr,
+ NULL,
+};
+
+const struct attribute_group region_attr_group = {
+ .attrs = region_attrs,
+};
+
+const struct attribute_group *region_attr_groups[] = {
+ ®ion_attr_group,
+ NULL,
+};
+
+struct device_type region_type = {
+ .name = "region",
+ .groups = region_attr_groups,
+};
+
+/*
+ * Show the current write enable state of the OTP. Users can only program the
+ * OTP when this shows 'enabled'.
+ */
+static ssize_t otp_we_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ int ret;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ ret = sprintf(buf, "%s\n", otp_we ? "enabled" : "disabled");
+
+ up(&otp_sem);
+
+ return ret;
+}
+
+/*
+ * Set the write enable state of the OTP. 'enabled' will enable programming
+ * and 'disabled' will prevent further programming from occuring. On loading
+ * the module, this will default to 'disabled'.
+ */
+static ssize_t otp_we_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ int err = 0;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ if (sysfs_streq(buf, "enabled"))
+ otp_we = 1;
+ else if (sysfs_streq(buf, "disabled"))
+ otp_we = 0;
+ else
+ err = -EINVAL;
+
+ up(&otp_sem);
+
+ return err ?: len;
+}
+static DEVICE_ATTR(write_enable, S_IRUSR | S_IWUSR, otp_we_show, otp_we_store);
+
+/*
+ * Show the current strict programming state of the OTP. If set to "enabled",
+ * then when programming, all raw words must program correctly to succeed. If
+ * disabled, then as long as the word reads back correctly in the redundant
+ * mode, then some bits may be allowed to be incorrect in the raw words.
+ */
+static ssize_t otp_strict_programming_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ int ret;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ ret = sprintf(buf, "%s\n", otp_strict_programming ? "enabled" :
+ "disabled");
+
+ up(&otp_sem);
+
+ return ret;
+}
+
+/*
+ * Set the current strict programming state of the OTP. If set to "enabled",
+ * then when programming, all raw words must program correctly to succeed. If
+ * disabled, then as long as the word reads back correctly in the redundant
+ * mode, then some bits may be allowed to be incorrect in the raw words.
+ */
+static ssize_t otp_strict_programming_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ int err = 0;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ if (sysfs_streq(buf, "enabled"))
+ otp_strict_programming = 1;
+ else if (sysfs_streq(buf, "disabled"))
+ otp_strict_programming = 0;
+ else
+ err = -EINVAL;
+
+ up(&otp_sem);
+
+ return err ?: len;
+}
+static DEVICE_ATTR(strict_programming, S_IRUSR | S_IWUSR,
+ otp_strict_programming_show, otp_strict_programming_store);
+
+static ssize_t otp_num_regions_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int nr_regions;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ nr_regions = otp->ops->get_nr_regions(otp);
+
+ up(&otp_sem);
+
+ if (nr_regions <= 0)
+ return -EINVAL;
+
+ return sprintf(buf, "%u\n", nr_regions);
+}
+
+static ssize_t otp_num_regions_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ unsigned long nr_regions;
+ int err = 0;
+
+ if (!otp->ops->set_nr_regions)
+ return -EOPNOTSUPP;
+
+ err = strict_strtoul(buf, 0, &nr_regions);
+ if (err)
+ return err;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ if (!otp_we) {
+ err = -EPERM;
+ goto out;
+ }
+
+ err = otp->ops->set_nr_regions(otp, nr_regions);
+
+out:
+ up(&otp_sem);
+
+ return err ?: len;
+}
+static DEVICE_ATTR(num_regions, S_IRUSR | S_IWUSR, otp_num_regions_show,
+ otp_num_regions_store);
+
+struct attribute *otp_attrs[] = {
+ &dev_attr_strict_programming.attr,
+ &dev_attr_num_regions.attr,
+ &dev_attr_write_enable.attr,
+ NULL,
+};
+
+const struct attribute_group otp_attr_group = {
+ .attrs = otp_attrs,
+};
+
+const struct attribute_group *otp_attr_groups[] = {
+ &otp_attr_group,
+ NULL,
+};
+
+struct device_type otp_type = {
+ .name = "otp",
+ .groups = otp_attr_groups,
+};
+
+static void otp_dev_release(struct device *dev)
+{
+ struct otp_device *otp = to_otp_device(dev);
+
+ kfree(otp);
+ otp = NULL;
+}
+
+struct otp_device *otp_device_alloc(struct device *dev,
+ const struct otp_device_ops *ops,
+ size_t size)
+{
+ struct otp_device *otp_dev = NULL;
+ int err = -EINVAL;
+
+ down(&otp_sem);
+
+ if (dev && !get_device(dev)) {
+ err = -ENODEV;
+ goto out;
+ }
+
+ if (otp) {
+ pr_warning("an otp device already registered\n");
+ err = -EBUSY;
+ goto out_put;
+ }
+
+ otp_dev = kzalloc(sizeof(*otp_dev), GFP_KERNEL);
+ if (!otp_dev) {
+ err = -ENOMEM;
+ goto out_put;
+ }
+
+ INIT_LIST_HEAD(&otp_dev->regions);
+ otp_dev->ops = ops;
+ otp_dev->dev.release = otp_dev_release;
+ otp_dev->dev.bus = &otp_bus_type;
+ otp_dev->dev.type = &otp_type;
+ otp_dev->dev.parent = dev;
+ otp_dev->size = size;
+ dev_set_name(&otp_dev->dev, "otpa");
+
+ otp = otp_dev;
+ err = device_register(&otp_dev->dev);
+ if (err) {
+ dev_err(&otp_dev->dev, "couldn't add device\n");
+ goto out_put;
+ }
+ pr_info("device %s of %zu bytes registered\n", ops->name, size);
+ goto out;
+
+out_put:
+ if (dev)
+ put_device(dev);
+out:
+ up(&otp_sem);
+
+ return err ? ERR_PTR(err) : otp_dev;
+}
+EXPORT_SYMBOL_GPL(otp_device_alloc);
+
+void otp_device_unregister(struct otp_device *dev)
+{
+ struct otp_region *region, *tmp;
+
+ down(&otp_sem);
+ list_for_each_entry_safe(region, tmp, &dev->regions, head)
+ otp_region_unregister(region);
+ device_unregister(&dev->dev);
+ up(&otp_sem);
+}
+EXPORT_SYMBOL_GPL(otp_device_unregister);
+
+static void otp_region_release(struct device *dev)
+{
+ struct otp_region *region = to_otp_region(dev);
+
+ cdev_del(®ion->cdev);
+ list_del(®ion->head);
+ kfree(region);
+}
+
+struct otp_region *otp_region_alloc_unlocked(struct otp_device *dev,
+ const struct otp_region_ops *ops,
+ int region_nr)
+{
+ struct otp_region *region;
+ int err = 0;
+ dev_t devno = MKDEV(MAJOR(otp_devno), region_nr + 1);
+
+ region = kzalloc(sizeof(*region), GFP_KERNEL);
+ if (!region) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ region->ops = ops;
+ region->region_nr = region_nr;
+ region->dev.parent = &dev->dev;
+ region->dev.release = otp_region_release;
+ region->dev.devt = devno;
+ region->dev.bus = &otp_bus_type;
+ region->dev.type = ®ion_type;
+ dev_set_name(®ion->dev, "otpa%d", region_nr + 1);
+
+ cdev_init(®ion->cdev, &otp_fops);
+ err = cdev_add(®ion->cdev, devno, 1);
+ if (err) {
+ dev_err(®ion->dev, "couldn't create cdev\n");
+ goto out_free;
+ }
+
+ err = device_register(®ion->dev);
+ if (err) {
+ dev_err(®ion->dev, "couldn't add device\n");
+ goto out_cdev_del;
+ }
+
+ list_add_tail(®ion->head, &dev->regions);
+ goto out;
+
+out_cdev_del:
+ cdev_del(®ion->cdev);
+out_free:
+ kfree(region);
+out:
+ return err ? ERR_PTR(err) : region;
+}
+EXPORT_SYMBOL_GPL(otp_region_alloc_unlocked);
+
+struct otp_region *otp_region_alloc(struct otp_device *dev,
+ const struct otp_region_ops *ops,
+ int region_nr)
+{
+ struct otp_region *ret;
+
+ down(&otp_sem);
+ ret = otp_region_alloc_unlocked(dev, ops, region_nr);
+ up(&otp_sem);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(otp_region_alloc);
+
+void otp_region_unregister(struct otp_region *region)
+{
+ device_unregister(®ion->dev);
+}
+EXPORT_SYMBOL_GPL(otp_region_unregister);
+
+static int otp_open(struct inode *inode, struct file *filp)
+{
+ struct otp_region *region;
+ int ret = 0;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ if (!try_module_get(otp->ops->owner)) {
+ ret = -ENODEV;
+ goto out;
+ }
+
+ region = container_of(inode->i_cdev, struct otp_region, cdev);
+ if (!get_device(®ion->dev)) {
+ ret = -ENODEV;
+ goto out_put_module;
+ }
+ filp->private_data = region;
+
+ goto out;
+
+out_put_module:
+ module_put(otp->ops->owner);
+out:
+ up(&otp_sem);
+
+ return ret;
+}
+
+static int otp_release(struct inode *inode, struct file *filp)
+{
+ struct otp_region *region;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+ region = filp->private_data;
+ put_device(®ion->dev);
+ module_put(otp->ops->owner);
+ up(&otp_sem);
+
+ return 0;
+}
+
+/*
+ * Write to the OTP memory from a userspace buffer. This requires that the
+ * write_enable attribute is set to "enabled" in
+ * /sys/devices/platform/otp/write_enable
+ *
+ * If writing is not enabled, this should return -EPERM.
+ *
+ * The write method takes a buffer from userspace and writes it into the
+ * corresponding bits of the OTP. The current file offset refers to the byte
+ * address of the words in the OTP region that should be written to.
+ * Therefore:
+ *
+ * - we may have to do a read-modify-write to get up to an aligned
+ * boundary, then
+ * - do a series of word writes, followed by,
+ * - an optional final read-modify-write if there are less than
+ * OTP_WORD_SIZE bytes left to write.
+ *
+ * After writing, the file offset will be updated to the next byte address. If
+ * one word fails to write then the writing is aborted at that point and no
+ * further data is written. If the user can carry on then they may call
+ * write(2) again with an updated offset.
+ */
+static ssize_t otp_write(struct file *filp, const char __user *buf, size_t len,
+ loff_t *offs)
+{
+ ssize_t ret = 0;
+ u64 word;
+ ssize_t written = 0;
+ struct otp_region *region = filp->private_data;
+ unsigned pos = (unsigned)*offs;
+ enum otp_redundancy_fmt fmt;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ if (region->ops->get_fmt)
+ fmt = region->ops->get_fmt(region);
+ else
+ fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
+
+ if (*offs >= region->ops->get_size(region)) {
+ ret = -ENOSPC;
+ goto out;
+ }
+
+ if (!otp_we) {
+ ret = -EPERM;
+ goto out;
+ }
+
+ len = min(len, region->ops->get_size(region) - (unsigned)*offs);
+ if (!len) {
+ ret = 0;
+ goto out;
+ }
+
+ if (otp->ops->set_fmt)
+ otp->ops->set_fmt(otp, fmt);
+
+ if (pos & (OTP_WORD_SIZE - 1)) {
+ /*
+ * We're not currently on an 8 byte boundary so we need to do
+ * a read-modify-write.
+ */
+ unsigned word_addr = pos / OTP_WORD_SIZE;
+ unsigned offset = pos % OTP_WORD_SIZE;
+ size_t bytes = min_t(size_t, OTP_WORD_SIZE - offset, len);
+
+ if (region->ops->read_word(region, word_addr, &word)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ if (copy_from_user((void *)(&word) + offset, buf, bytes)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (region->ops->write_word(region, word_addr, word)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ written += bytes;
+ len -= bytes;
+ buf += bytes;
+ pos += bytes;
+ }
+
+ /*
+ * We're now aligned to an 8 byte boundary so we can simply copy words
+ * from userspace and write them into the OTP.
+ */
+ while (len >= OTP_WORD_SIZE) {
+ if (copy_from_user(&word, buf, OTP_WORD_SIZE)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (region->ops->write_word(region, pos / OTP_WORD_SIZE,
+ word)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ written += OTP_WORD_SIZE;
+ len -= OTP_WORD_SIZE;
+ buf += OTP_WORD_SIZE;
+ pos += OTP_WORD_SIZE;
+ }
+
+ /*
+ * We might have less than 8 bytes left so we'll need to do another
+ * read-modify-write.
+ */
+ if (len) {
+ if (region->ops->read_word(region, pos / OTP_WORD_SIZE,
+ &word)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ if (copy_from_user(&word, buf, len)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (region->ops->write_word(region, pos / OTP_WORD_SIZE,
+ word)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ written += len;
+ buf += len;
+ pos += len;
+ len = 0;
+ }
+
+ *offs += written;
+
+out:
+ up(&otp_sem);
+ return ret ?: written;
+}
+
+/*
+ * Read an OTP region. This switches the OTP into the appropriate redundancy
+ * format so we can simply read from the beginning of the region and copy it
+ * into the user buffer.
+ */
+static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
+ loff_t *offs)
+{
+ ssize_t ret = 0;
+ u64 word;
+ ssize_t bytes_read = 0;
+ struct otp_region *region = filp->private_data;
+ unsigned pos = (unsigned)*offs;
+ enum otp_redundancy_fmt fmt;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ if (region->ops->get_fmt)
+ fmt = region->ops->get_fmt(region);
+ else
+ fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
+
+ if (*offs >= region->ops->get_size(region)) {
+ ret = 0;
+ goto out;
+ }
+
+ len = min(len, region->ops->get_size(region) - (unsigned)*offs);
+ if (!len) {
+ ret = 0;
+ goto out;
+ }
+
+ if (otp->ops->set_fmt)
+ otp->ops->set_fmt(otp, fmt);
+
+ if (pos & (OTP_WORD_SIZE - 1)) {
+ /*
+ * We're not currently on an 8 byte boundary so we need to
+ * read to a bounce buffer then do the copy_to_user() with an
+ * offset.
+ */
+ unsigned word_addr = pos / OTP_WORD_SIZE;
+ unsigned offset = pos % OTP_WORD_SIZE;
+ size_t bytes = min_t(size_t, OTP_WORD_SIZE - offset, len);
+
+ if (region->ops->read_word(region, word_addr, &word)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ if (copy_to_user(buf, (void *)(&word) + offset, bytes)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ bytes_read += bytes;
+ len -= bytes;
+ buf += bytes;
+ pos += bytes;
+ }
+
+ /*
+ * We're now aligned to an 8 byte boundary so we can simply copy words
+ * from the bounce buffer directly with a copy_to_user().
+ */
+ while (len >= OTP_WORD_SIZE) {
+ if (region->ops->read_word(region, pos / OTP_WORD_SIZE,
+ &word)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ if (copy_to_user(buf, &word, OTP_WORD_SIZE)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ bytes_read += OTP_WORD_SIZE;
+ len -= OTP_WORD_SIZE;
+ buf += OTP_WORD_SIZE;
+ pos += OTP_WORD_SIZE;
+ }
+
+ /*
+ * We might have less than 8 bytes left so we'll need to do another
+ * copy_to_user() but with a partial word length.
+ */
+ if (len) {
+ if (region->ops->read_word(region, pos / OTP_WORD_SIZE,
+ &word)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ if (copy_to_user(buf, &word, len)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ bytes_read += len;
+ buf += len;
+ pos += len;
+ len = 0;
+ }
+
+ *offs += bytes_read;
+
+out:
+ up(&otp_sem);
+ return ret ?: bytes_read;
+}
+
+/*
+ * Seek to a specified position in the OTP region. This can be used if
+ * userspace doesn't have pread()/pwrite() and needs to write to a specified
+ * offset in the OTP.
+ */
+static loff_t otp_llseek(struct file *filp, loff_t offs, int origin)
+{
+ struct otp_region *region = filp->private_data;
+ int ret = 0;
+ loff_t end;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ switch (origin) {
+ case SEEK_CUR:
+ if (filp->f_pos + offs < 0 ||
+ filp->f_pos + offs >= region->ops->get_size(region))
+ ret = -EINVAL;
+ else
+ filp->f_pos += offs;
+ break;
+
+ case SEEK_SET:
+ if (offs < 0 || offs >= region->ops->get_size(region))
+ ret = -EINVAL;
+ else
+ filp->f_pos = offs;
+ break;
+
+ case SEEK_END:
+ end = region->ops->get_size(region) - 1;
+ if (end + offs < 0 || end + offs >= end)
+ ret = -EINVAL;
+ else
+ filp->f_pos = end + offs;
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ up(&otp_sem);
+
+ return ret ?: filp->f_pos;
+}
+
+static int __init otp_init(void)
+{
+ int err;
+
+ err = bus_register(&otp_bus_type);
+ if (err)
+ return err;
+
+ err = alloc_chrdev_region(&otp_devno, 0, 9, "otp");
+ if (err)
+ goto out_bus_unregister;
+
+ return 0;
+
+out_bus_unregister:
+ bus_unregister(&otp_bus_type);
+
+ return err;
+}
+module_init(otp_init);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jamie Iles");
+MODULE_DESCRIPTION("OTP interface driver");
diff --git a/include/linux/otp.h b/include/linux/otp.h
new file mode 100644
index 0000000..93ccf8b
--- /dev/null
+++ b/include/linux/otp.h
@@ -0,0 +1,221 @@
+/*
+ * 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 user interface for reading and writing OTP
+ * memory. This OTP can be used for executing secure boot code or for the
+ * secure storage of keys and any other user data. We support multiple
+ * backends for different OTP macros.
+ *
+ * The OTP is configured through sysfs and is read and written through device
+ * nodes. The OTP device in the device model (the platform device) gains
+ * write_enable, num_regions, and strict_programming attributes. We also
+ * create an otp bus to which we add a device per region. The OTP can supports
+ * multiple regions and when we divide the regions down we create a new child
+ * device on the otp bus. This child device has format and size attributes.
+ *
+ * To update the number of regions, the format of a region or to program a
+ * region, the write_enable attribute of the OTP device must be set to
+ * "enabled".
+ */
+#ifndef __OTP_H__
+#define __OTP_H__
+
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/module.h>
+
+/*
+ * The OTP works in 64 bit words. When we are programming or reading,
+ * everything is done with 64 bit word addresses.
+ */
+#define OTP_WORD_SIZE 8
+
+enum otp_redundancy_fmt {
+ OTP_REDUNDANCY_FMT_SINGLE_ENDED,
+ OTP_REDUNDANCY_FMT_REDUNDANT,
+ OTP_REDUNDANCY_FMT_DIFFERENTIAL,
+ OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT,
+};
+
+struct otp_device;
+struct otp_region;
+
+/**
+ * struct otp_device_ops - operations for the OTP device.
+ *
+ * @name: The name of the driver backend.
+ * @owner: The owning module.
+ * @get_nr_regions: Get the number of regions that the OTP is partitioned
+ * into. Note that this is the number of regions in the
+ * device, not the number of regions registered.
+ * @set_nr_regions: Increase the number of partitions in the device.
+ * Returns zero on success, negative errno on failure.
+ * @set_fmt: Set the read-mode redundancy for the region. The OTP
+ * devices need to be put into the right redundancy mode
+ * before reading/writing.
+ */
+struct otp_device_ops {
+ const char *name;
+ struct module *owner;
+ int (*get_nr_regions)(struct otp_device *dev);
+ int (*set_nr_regions)(struct otp_device *dev,
+ int nr_regions);
+ int (*set_fmt)(struct otp_device *dev,
+ enum otp_redundancy_fmt fmt);
+};
+
+/**
+ * struct otp_device - a picoxcell OTP device.
+ *
+ * @ops: The operations to use for manipulating the device.
+ * @dev: The parent device (typically a platform_device) that
+ * provides the OTP.
+ * @regions: The regions registered to the device.
+ * @size: The size of the OTP in bytes.
+ * @driver_data: Private driver data.
+ */
+struct otp_device {
+ const struct otp_device_ops *ops;
+ struct device dev;
+ struct list_head regions;
+ size_t size;
+ void *driver_data;
+};
+
+/**
+ * struct otp_region_ops - operations to manipulate OTP regions.
+ *
+ * @set_fmt: Permanently set the format of the region. Returns
+ * zero on success.
+ * @get_fmt: Get the redundancy format of the region.
+ * @write_word: Write a 64-bit word to the OTP.
+ * @read_word: Read a 64-bit word from the OTP.
+ * @get_size: Get the effective storage size of the region.
+ * Depending on the number of regions in the device and
+ * the redundancy format of the region, this may vary.
+ */
+struct otp_region_ops {
+ int (*set_fmt)(struct otp_region *region,
+ enum otp_redundancy_fmt fmt);
+ enum otp_redundancy_fmt (*get_fmt)(struct otp_region *region);
+ int (*write_word)(struct otp_region *region,
+ unsigned long word_addr,
+ u64 value);
+ int (*read_word)(struct otp_region *region,
+ unsigned long word_addr,
+ u64 *value);
+ ssize_t (*get_size)(struct otp_region *region);
+};
+
+/**
+ * struct otp_region: a single region of OTP.
+ *
+ * @ops: Operations for manipulating the region.
+ * @dev: The device to register with the driver model.
+ * @cdev: The character device used to provide userspace access to the
+ * region.
+ * @head: The position in the devices region list.
+ * @region_nr: The region number of the region. Devices number their regions
+ * from 1 upwards.
+ */
+struct otp_region {
+ const struct otp_region_ops *ops;
+ struct device dev;
+ struct cdev cdev;
+ struct list_head head;
+ unsigned region_nr;
+};
+
+/**
+ * otp_device_alloc - create a new picoxcell OTP device.
+ *
+ * Returns the newly created OTP device on success or a ERR_PTR() encoded
+ * errno on failure. The new device is automatically registered and can be
+ * released with otp_device_unregister(). This will increase the reference
+ * count on dev.
+ *
+ * @dev: The parent device that provides the OTP implementation.
+ * @ops: The operations to manipulate the OTP device.
+ * @size: The size, in bytes of the OTP device.
+ */
+struct otp_device *otp_device_alloc(struct device *dev,
+ const struct otp_device_ops *ops,
+ size_t size);
+
+/**
+ * otp_device_unregister - deregister an existing struct otp_device.
+ *
+ * This unregisters an otp_device and any regions that have been registered to
+ * it. Once all regions have been released, the parent reference count is
+ * decremented and the otp_device will be freed. Callers must assume that dev
+ * is invalidated during this call.
+ *
+ * @dev: The otp_device to unregister.
+ */
+void otp_device_unregister(struct otp_device *dev);
+
+/**
+ * otp_region_alloc - create and register a new OTP region.
+ *
+ * Create and register a new region in an existing device with specified
+ * region operations. Returns a pointer to the region on success, or an
+ * ERR_PTR() encoded errno on failure.
+ *
+ * Note: this takes the OTP semaphore so may not be called from one of the
+ * otp_device_ops or otp_region_ops callbacks or this may lead to deadlock.
+ *
+ * @dev: The device to add the region to.
+ * @ops: The operations for the region.
+ * @region_nr: The region ID. Must be unique for the region.
+ */
+struct otp_region *otp_region_alloc(struct otp_device *dev,
+ const struct otp_region_ops *ops,
+ int region_nr);
+
+/**
+ * otp_region_alloc_unlocked - create and register a new OTP region.
+ *
+ * This is the same as otp_region_alloc() but does not take the OTP semaphore
+ * so may only be called from inside one of the otp_device_ops or
+ * otp_region_ops callbacks.
+ *
+ * @dev: The device to add the region to.
+ * @ops: The operations for the region.
+ * @region_nr: The region ID. Must be unique for the region.
+ */
+struct otp_region *otp_region_alloc_unlocked(struct otp_device *dev,
+ const struct otp_region_ops *ops,
+ int region_nr);
+
+/**
+ * otp_region_unregister - unregister a given OTP region.
+ *
+ * This unregisters a region from the device and forms part of
+ * otp_device_unregister().
+ *
+ * @region: The region to unregister.
+ */
+void otp_region_unregister(struct otp_region *region);
+
+/**
+ * otp_strict_programming_enabled - check if we are in strict programming
+ * mode.
+ *
+ * Some OTP devices support different redundancy modes. These devices often
+ * need multiple words programmed to represent a single word in that
+ * redundancy format. If strict programming is enabled then all of the
+ * redundancy words must program correctly to indicate success. If strict
+ * programming is disabled then we will allow errors in the redundant word as
+ * long as the contents of the whole word are read back correctly with the
+ * required redundancy mode.
+ */
+bool otp_strict_programming_enabled(void);
+
+#endif /* __OTP_H__ */
--
1.7.4
Some drivers may need an ioctl method to provide device specific control
such as blackfin for providing locking. Regions can specify an ioctl to
be used but this field method is optional.
Signed-off-by: Jamie Iles <[email protected]>
---
drivers/otp/otp.c | 30 ++++++++++++++++++++++++------
include/linux/otp.h | 3 +++
2 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/otp/otp.c b/drivers/otp/otp.c
index dd47cf1..d362d09 100644
--- a/drivers/otp/otp.c
+++ b/drivers/otp/otp.c
@@ -30,14 +30,16 @@ static ssize_t otp_write(struct file *filp, const char __user *buf,
static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
loff_t *offs);
static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
+static long otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg);
static const struct file_operations otp_fops = {
- .owner = THIS_MODULE,
- .open = otp_open,
- .release = otp_release,
- .write = otp_write,
- .read = otp_read,
- .llseek = otp_llseek,
+ .owner = THIS_MODULE,
+ .open = otp_open,
+ .release = otp_release,
+ .write = otp_write,
+ .read = otp_read,
+ .llseek = otp_llseek,
+ .unlocked_ioctl = otp_ioctl,
};
static DEFINE_SEMAPHORE(otp_sem);
@@ -691,6 +693,22 @@ out:
return ret ?: written;
}
+static long otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
+{
+ struct otp_region *region = filp->private_data;
+ long ret = -ENOTTY;
+
+ if (down_interruptible(&otp_sem))
+ return -ERESTARTSYS;
+
+ if (region->ops->ioctl)
+ ret = region->ops->ioctl(region, cmd, arg);
+
+ up(&otp_sem);
+
+ return ret;
+}
+
/*
* Read an OTP region. This switches the OTP into the appropriate redundancy
* format so we can simply read from the beginning of the region and copy it
diff --git a/include/linux/otp.h b/include/linux/otp.h
index 93ccf8b..ce77f05 100644
--- a/include/linux/otp.h
+++ b/include/linux/otp.h
@@ -100,6 +100,7 @@ struct otp_device {
* @get_size: Get the effective storage size of the region.
* Depending on the number of regions in the device and
* the redundancy format of the region, this may vary.
+ * @ioctl: Optional region ioctl.
*/
struct otp_region_ops {
int (*set_fmt)(struct otp_region *region,
@@ -112,6 +113,8 @@ struct otp_region_ops {
unsigned long word_addr,
u64 *value);
ssize_t (*get_size)(struct otp_region *region);
+ long (*ioctl)(struct otp_region *dev,
+ unsigned cmd, unsigned long arg);
};
/**
--
1.7.4
On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> Following some feedback from Greg, I've updated this series to be more
> of a generic OTP layer. Everything is now registered under the "otp"
> bus and I've also converted the blackfin OTP driver to use this
> framework (which is the only current OTP driver I could find).
really, i'm the only one who wrote a driver ? that's boring.
i guess this isnt trying to handle OTP stuff that exists in the MTD
layer already ?
> Mike, I wasn't 100% sure how big the blackfin OTP is but I found a
> datasheet talking about 64KB so I've assumed that for now.
the datasheets say 64K *bits* :). i think all our datasheets tend to
use bits rather than bytes because they're stupid and bigger numbers
always means better parts !
but yes, on-chip OTP on all relevant Blackfin parts today are 8KiB in
size. 0x200 128bit pages is how things are organized.
-mike
Hi Mike,
On Thu, Mar 24, 2011 at 01:39:56PM -0400, Mike Frysinger wrote:
> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> > Following some feedback from Greg, I've updated this series to be more
> > of a generic OTP layer. ?Everything is now registered under the "otp"
> > bus and I've also converted the blackfin OTP driver to use this
> > framework (which is the only current OTP driver I could find).
>
> really, i'm the only one who wrote a driver ? that's boring.
Yes, and I'm quite surprised at that. Perhaps I've missed some.
> i guess this isnt trying to handle OTP stuff that exists in the MTD
> layer already ?
No, I believe that the OTP stuff in the MTD layer is where some flash
chips have a small section that can be write protected so it's more like
a permanent write disable to that sector whereas the thing I'm looking
at is on a per-bit basis.
I did have a look at if there was some kind of way to fit this stuff
into the MTD layer but it felt like it was really shoehorning it in.
> > Mike, I wasn't 100% sure how big the blackfin OTP is but I found a
> > datasheet talking about 64KB so I've assumed that for now.
>
> the datasheets say 64K *bits* :). i think all our datasheets tend to
> use bits rather than bytes because they're stupid and bigger numbers
> always means better parts !
That sounds very familiar!
Jamie
On Thu, Mar 24, 2011 at 13:47, Jamie Iles wrote:
> On Thu, Mar 24, 2011 at 01:39:56PM -0400, Mike Frysinger wrote:
>> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
>> > Mike, I wasn't 100% sure how big the blackfin OTP is but I found a
>> > datasheet talking about 64KB so I've assumed that for now.
>>
>> the datasheets say 64K *bits* :). i think all our datasheets tend to
>> use bits rather than bytes because they're stupid and bigger numbers
>> always means better parts !
>
> That sounds very familiar!
just an overview note here ... the Blackfin OTP is actually IP we
bought from someone (not sure if i can say more, although i dont know
the details myself ... i'd have to go ask around), so much of the
feedback i give about the structure of it most likely (hopefully) be
applicable to other people who have purchased this IP. if/when anyone
who is also using this IP steps forward, we could even see about
generalizing the bfin-otp driver.
-mike
On Thu, Mar 24, 2011 at 01:56:17PM -0400, Mike Frysinger wrote:
> just an overview note here ... the Blackfin OTP is actually IP we
> bought from someone (not sure if i can say more, although i dont know
> the details myself ... i'd have to go ask around), so much of the
> feedback i give about the structure of it most likely (hopefully) be
> applicable to other people who have purchased this IP. if/when anyone
> who is also using this IP steps forward, we could even see about
> generalizing the bfin-otp driver.
Yes, we have a similar thing here - a block of OTP from an IP vendor and
a wrapper to provide an AXI interface. As far as I know there are only
a few OTP vendors so there may well be plenty of common code in
different OTP implementations.
Jamie
On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> +What: /sys/bus/otp/
> +Description:
> + The otp bus presents a number of devices where each
> + device represents a region or device in the SoC.
is it limited to OTP devices inside of SoCs ? cant OTP devices be on
other busses like I2C or SPI ?
spurious space before "The" ...
> +Contact: Jamie Iles <[email protected]>
> +Contact: "Jamie Iles" <[email protected]>
sometimes you quote and sometimes you dont ... seems like quotes are
useless here
> +What: /sys/bus/otp[a-z]/write_enable
what's with the [a-z] ? how about using otp# like most other people
are doing now ? [a-z] can be a bit limited/confusing ...
> +What: /sys/bus/otp[a-z]/strict_programming
> +Description:
> + This file indicates whether all words in a redundant format
> + must be programmed correctly to indicate success. Disabling
> + this will mean that programming will be considered a success
> + if the word can be read back correctly in it's redundant
> + format.
i dont really grok what this is trying to say ...
"it's" -> "its"
> +What: /sys/bus/otp/devices/otp[a-z][0-9]*/format
you have /sys/bus/otp[a-z]/ and /sys/bus/otp/devices/ ? why not unify them ?
what are each of these subdirs trying to represent ?
> +Description:
> + The redundancy format of the region. Valid values are:
> + - single-ended (1 bit of storage per data bit).
> + - redundant (2 bits of storage, wire-OR'd per data
> + bit).
> + - differential (2 bits of storage, differential
> + voltage per data bit).
> + - differential-redundant (4 bits of storage, combining
> + redundant and differential).
> + It is possible to increase redundancy of a region but care
> + will be needed if there is data already in the region.
where does ECC fit in ? the Blackfin OTP is structured:
- first 4 pages are write control bitfields for all other pages (so
blowing bit 5 of page 0 prevents further writing to page 5)
- each 0x100 page region has 0x20 pages dedicated to ECC for the
other 0x80 pages ... this provides 1 bit error correction and 2 bits
of error detection (anymore and you're screwed!)
> --- /dev/null
> +++ b/drivers/otp/Kconfig
> @@ -0,0 +1,10 @@
> +#
> +# Character device configuration
> +#
old comment
> +menuconfig OTP
> + bool "OTP memory support"
> + help
> + 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.
is this limited to embedded devices ? i guess TPM keys and such are
already handled by the TPM layers ...
"y" -> "Y"
> --- /dev/null
> +++ b/drivers/otp/otp.c
> +#undef DEBUG
dead code
> +static int otp_open(struct inode *inode, struct file *filp);
> +static int otp_release(struct inode *inode, struct file *filp);
> +static ssize_t otp_write(struct file *filp, const char __user *buf,
> + size_t len, loff_t *offs);
> +static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
> + loff_t *offs);
> +static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
> +
> +static const struct file_operations otp_fops = {
> + .owner = THIS_MODULE,
> + .open = otp_open,
> + .release = otp_release,
> + .write = otp_write,
> + .read = otp_read,
> + .llseek = otp_llseek,
> +};
if you moved the fops down to where it is used, you wouldnt need the
redundant func prototypes
> +static DEFINE_SEMAPHORE(otp_sem);
> +static int otp_we, otp_strict_programming;
> +static struct otp_device *otp;
> +static dev_t otp_devno;
hrm, having these be global instead of per-device sounds like a really
bad idea ...
> +/*
> + * Given a device for one of the otpN devices, get the corresponding
> + * otp_region.
> + */
> +static inline struct otp_region *to_otp_region(struct device *dev)
> +{
> + return dev ? container_of(dev, struct otp_region, dev) : NULL;
> +}
> +
> +static inline struct otp_device *to_otp_device(struct device *dev)
> +{
> + return dev ? container_of(dev, struct otp_device, dev) : NULL;
> +}
do you need the NULL checks ? none of the callers of these funcs
check for NULL ...
> +static ssize_t otp_format_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct otp_region *region = to_otp_region(dev);
> + enum otp_redundancy_fmt fmt;
> + const char *fmt_string;
> +
> + if (down_interruptible(&otp_sem))
> + return -ERESTARTSYS;
> +
> + if (region->ops->get_fmt(region))
> + fmt = region->ops->get_fmt(region);
> + else
> + fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
> +
> + up(&otp_sem);
why are you using a semaphore when it looks like you're simply
treating it as a mutex ? make more sense to use a proper mutex
wouldnt it ?
> + if (OTP_REDUNDANCY_FMT_SINGLE_ENDED == fmt)
> + fmt_string = "single-ended";
> + else if (OTP_REDUNDANCY_FMT_REDUNDANT == fmt)
> + fmt_string = "redundant";
> + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL == fmt)
> + fmt_string = "differential";
> + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT == fmt)
> + fmt_string = "differential-redundant";
> + else
> + fmt_string = NULL;
> +
> + return fmt_string ? sprintf(buf, "%s\n", fmt_string) : -EINVAL;
i'm pretty sure printf in the kernel can handle NULL with %s, so the
NULL check can probably be punted
> +static struct bus_type otp_bus_type = {
> + .name = "otp",
> +};
can this be const ?
> +struct attribute *region_attrs[] = {
> + &dev_attr_format.attr,
> + &dev_attr_size.attr,
> + NULL,
> +};
> +
> +const struct attribute_group region_attr_group = {
> + .attrs = region_attrs,
> +};
> +
> +const struct attribute_group *region_attr_groups[] = {
> + ®ion_attr_group,
> + NULL,
> +};
> +
> +struct device_type region_type = {
> + .name = "region",
> + .groups = region_attr_groups,
> +};
should these be static ?
> +/*
> + * Show the current write enable state of the OTP. Users can only program the
> + * OTP when this shows 'enabled'.
> + */
> +static ssize_t otp_we_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
this func has a ssize_t type but you're using int here. seems to come
up a few times in this patch.
> +/*
> + * Set the write enable state of the OTP. 'enabled' will enable programming
> + * and 'disabled' will prevent further programming from occuring. On loading
"occuring" -> "occurring"
> + * the module, this will default to 'disabled'.
> + */
> +static ssize_t otp_we_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + int err = 0;
> +
> + if (down_interruptible(&otp_sem))
> + return -ERESTARTSYS;
> +
> + if (sysfs_streq(buf, "enabled"))
> + otp_we = 1;
> + else if (sysfs_streq(buf, "disabled"))
> + otp_we = 0;
> + else
> + err = -EINVAL;
> +
> + up(&otp_sem);
> +
> + return err ?: len;
> +}
> +static DEVICE_ATTR(write_enable, S_IRUSR | S_IWUSR, otp_we_show, otp_we_store);
you output and accept "enabled" and "disabled" for multiple values.
how about unifying these ?
return otp_attr_store_enabled(buf, len, &otp_we);
> +static ssize_t otp_num_regions_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int nr_regions;
> +
> + if (down_interruptible(&otp_sem))
> + return -ERESTARTSYS;
> +
> + nr_regions = otp->ops->get_nr_regions(otp);
> +
> + up(&otp_sem);
> +
> + if (nr_regions <= 0)
> + return -EINVAL;
> +
> + return sprintf(buf, "%u\n", nr_regions);
> +}
i'm not sure returning -EINVAL here makes sense ... shouldnt it just
be showing the user the result of get_nr_regions() ?
> +struct attribute *otp_attrs[] = {
> + &dev_attr_strict_programming.attr,
> + &dev_attr_num_regions.attr,
> + &dev_attr_write_enable.attr,
> + NULL,
> +};
> +
> +const struct attribute_group otp_attr_group = {
> + .attrs = otp_attrs,
> +};
> +
> +const struct attribute_group *otp_attr_groups[] = {
> + &otp_attr_group,
> + NULL,
> +};
> +
> +struct device_type otp_type = {
> + .name = "otp",
> + .groups = otp_attr_groups,
> +};
static ?
> +static void otp_dev_release(struct device *dev)
> +{
> + struct otp_device *otp = to_otp_device(dev);
> +
> + kfree(otp);
> + otp = NULL;
> +}
setting to NULL here is pointless when the pointer is on the stack
> +struct otp_device *otp_device_alloc(struct device *dev,
> + const struct otp_device_ops *ops,
> + size_t size)
> +{
> + struct otp_device *otp_dev = NULL;
> + int err = -EINVAL;
> +
> + down(&otp_sem);
> +
> + if (dev && !get_device(dev)) {
> + err = -ENODEV;
> + goto out;
> + }
should you really be allowing dev==NULL ? does that setup make sense ?
> + if (otp) {
> + pr_warning("an otp device already registered\n");
> + err = -EBUSY;
> + goto out_put;
> + }
you can only register one OTP device in the whole system ??
> +void otp_region_unregister(struct otp_region *region)
> +{
> + device_unregister(®ion->dev);
> +}
> +EXPORT_SYMBOL_GPL(otp_region_unregister);
wonder if it'd be better to simply #define this in the global otp.h header
> +static ssize_t otp_write(struct file *filp, const char __user *buf, size_t len,
> + loff_t *offs)
> +{
> + unsigned pos = (unsigned)*offs;
> +
> + len = min(len, region->ops->get_size(region) - (unsigned)*offs);
what's with the unsigned cast ? defeats the point of using loff_t doesnt it ?
> + /*
> + * We're now aligned to an 8 byte boundary so we can simply copy words
> + * from userspace and write them into the OTP.
> + */
> + /*
> + * We might have less than 8 bytes left so we'll need to do another
> + * read-modify-write.
> + */
> + while (len >= OTP_WORD_SIZE) {
i think "8 byte" should be "necessary byte" ?
> + if (region->ops->read_word(region, pos / OTP_WORD_SIZE,
> + &word)) {
> + ret = -EIO;
> + goto out;
> + }
> +
> + if (region->ops->write_word(region, pos / OTP_WORD_SIZE,
> + word)) {
> + ret = -EIO;
> + goto out;
> + }
shouldnt you pass the ret value up from read/write word ? this would
allow the lower layers to better describe the issue than just -EIO
wouldnt it ?
last three comments apply to the read func as well ...
> +static int __init otp_init(void)
> +{
> + int err;
> +
> + err = bus_register(&otp_bus_type);
> + if (err)
> + return err;
> +
> + err = alloc_chrdev_region(&otp_devno, 0, 9, "otp");
where'd that magic 9 come from ?
> +MODULE_DESCRIPTION("OTP interface driver");
i think this is also a bus driver ?
> +/*
> + * The OTP works in 64 bit words. When we are programming or reading,
> + * everything is done with 64 bit word addresses.
> + */
> +#define OTP_WORD_SIZE 8
should this be a per-device setting ? or wait until someone who
doesnt have 64bit chunks show up ?
> + * struct otp_device - a picoxcell OTP device.
> + * otp_device_alloc - create a new picoxcell OTP device.
this is no longer picoxcell-specific
> + * otp_device_unregister - deregister an existing struct otp_device.
"deregister" -> "unregister"
-mike
On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> Some drivers may need an ioctl method to provide device specific control
> such as blackfin for providing locking. Regions can specify an ioctl to
> be used but this field method is optional.
shouldnt this just be squashed into patch 1 since you havent merged
any of this yet ?
> +static long otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
> +{
> + struct otp_region *region = filp->private_data;
> + long ret = -ENOTTY;
> +
> + if (down_interruptible(&otp_sem))
> + return -ERESTARTSYS;
> +
> + if (region->ops->ioctl)
> + ret = region->ops->ioctl(region, cmd, arg);
> +
> + up(&otp_sem);
> +
> + return ret;
> +}
the existence check can be pulled out of the locking
long ret;
if (!region->ops->ioctl)
return -ENOTTY;
if (down_interruptible(&otp_sem))
return -ERESTARTSYS;
ret = region->ops->ioctl(region, cmd, arg);
up(&otp_sem);
return ret;
-mike
On Thu, Mar 24, 2011 at 14:32, Jamie Iles wrote:
> On Thu, Mar 24, 2011 at 01:56:17PM -0400, Mike Frysinger wrote:
>> just an overview note here ... the Blackfin OTP is actually IP we
>> bought from someone (not sure if i can say more, although i dont know
>> the details myself ... i'd have to go ask around), so much of the
>> feedback i give about the structure of it most likely (hopefully) be
>> applicable to other people who have purchased this IP. if/when anyone
>> who is also using this IP steps forward, we could even see about
>> generalizing the bfin-otp driver.
>
> Yes, we have a similar thing here - a block of OTP from an IP vendor and
> a wrapper to provide an AXI interface. As far as I know there are only
> a few OTP vendors so there may well be plenty of common code in
> different OTP implementations.
do you guys wrap the interface ? the Blackfin processors have an
on-chip ROM which provides higher level functions like
read/write/lock/etc... the Linux driver uses those rather than
programming the memory mapped OTP interface itself since this
interface is not publicly documented.
-mike
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
On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> +config BFIN_OTP_WRITE_ENABLE
> + bool "Enable writing support of OTP pages"
> + depends on BFIN_OTP
> + default n
> + help
> + If you say Y here, you will enable support for writing of the
> + OTP pages. This is dangerous by nature as you can only program
> + the pages once, so only enable this option when you actually
> + need it so as to not inadvertently clobber data.
if the generic layer is providing write enable access now, is this
still necessary ? we could have a common OTP Kconfig i think for
"enable OTP write support".
-mike
On Thu, Mar 24, 2011 at 03:21:08PM +0000, Jamie Iles wrote:
> +/*
> + * 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.
Do you _really_ mean "any later version"? Be sure about that please.
You have that wording in all of your files you add, please be careful
about it as you might have copied from code that did not have that
wording (I'm not saying you did, just be sure about this.)
> + */
> +#define pr_fmt(fmt) "otp: " fmt
> +
> +#undef DEBUG
What is this for?
> +#include <linux/cdev.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/otp.h>
> +#include <linux/semaphore.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/uaccess.h>
> +
> +static int otp_open(struct inode *inode, struct file *filp);
> +static int otp_release(struct inode *inode, struct file *filp);
> +static ssize_t otp_write(struct file *filp, const char __user *buf,
> + size_t len, loff_t *offs);
> +static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
> + loff_t *offs);
> +static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
> +
> +static const struct file_operations otp_fops = {
> + .owner = THIS_MODULE,
> + .open = otp_open,
> + .release = otp_release,
> + .write = otp_write,
> + .read = otp_read,
> + .llseek = otp_llseek,
> +};
> +
> +static DEFINE_SEMAPHORE(otp_sem);
> +static int otp_we, otp_strict_programming;
> +static struct otp_device *otp;
> +static dev_t otp_devno;
> +
> +/*
> + * Given a device for one of the otpN devices, get the corresponding
> + * otp_region.
> + */
> +static inline struct otp_region *to_otp_region(struct device *dev)
> +{
> + return dev ? container_of(dev, struct otp_region, dev) : NULL;
> +}
> +
> +static inline struct otp_device *to_otp_device(struct device *dev)
> +{
> + return dev ? container_of(dev, struct otp_device, dev) : NULL;
> +}
> +
> +bool otp_strict_programming_enabled(void)
> +{
> + return otp_strict_programming;
> +}
> +EXPORT_SYMBOL_GPL(otp_strict_programming_enabled);
> +
> +static ssize_t otp_format_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct otp_region *region = to_otp_region(dev);
> + enum otp_redundancy_fmt fmt;
> + const char *fmt_string;
> +
> + if (down_interruptible(&otp_sem))
> + return -ERESTARTSYS;
> +
> + if (region->ops->get_fmt(region))
> + fmt = region->ops->get_fmt(region);
> + else
> + fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
> +
> + up(&otp_sem);
> +
> + if (OTP_REDUNDANCY_FMT_SINGLE_ENDED == fmt)
While some people feel this somehow makes it harder to write bad C code,
it's not the kernel style. Please reverse this comparison. If you
accidentally put a '=' in there instead of '==', gcc would warn you
about it.
> + fmt_string = "single-ended";
> + else if (OTP_REDUNDANCY_FMT_REDUNDANT == fmt)
> + fmt_string = "redundant";
> + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL == fmt)
> + fmt_string = "differential";
> + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT == fmt)
> + fmt_string = "differential-redundant";
> + else
> + fmt_string = NULL;
Just return -EINVAL here.
> +
> + return fmt_string ? sprintf(buf, "%s\n", fmt_string) : -EINVAL;
Then you don't have to do the embedded if in this statement.
Same thing goes for your other show/store functions.
> +/**
> + * struct otp_device - a picoxcell OTP device.
> + *
> + * @ops: The operations to use for manipulating the device.
> + * @dev: The parent device (typically a platform_device) that
> + * provides the OTP.
> + * @regions: The regions registered to the device.
> + * @size: The size of the OTP in bytes.
> + * @driver_data: Private driver data.
> + */
> +struct otp_device {
> + const struct otp_device_ops *ops;
> + struct device dev;
> + struct list_head regions;
> + size_t size;
> + void *driver_data;
Why do you need this pointer, can't you use the one in struct device
that is there for this purpose? Then provide a get/set function to
access this field so that a driver doesn't go and poke in it directly.
thanks,
greg k-h
Hi Mike,
Thanks for the great feedback!
On Thu, Mar 24, 2011 at 02:32:41PM -0400, Mike Frysinger wrote:
> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> > +What: /sys/bus/otp/
> > +Description:
> > + The otp bus presents a number of devices where each
> > + device represents a region or device in the SoC.
>
> is it limited to OTP devices inside of SoCs ? cant OTP devices be on
> other busses like I2C or SPI ?
Yes, that needs clearing up, it shouldn't be SoC specific.
>
> spurious space before "The" ...
>
> > +Contact: Jamie Iles <[email protected]>
> > +Contact: "Jamie Iles" <[email protected]>
>
> sometimes you quote and sometimes you dont ... seems like quotes are
> useless here
Not sure why I did that, will clean it up.
>
> > +What: /sys/bus/otp[a-z]/write_enable
>
> what's with the [a-z] ? how about using otp# like most other people
> are doing now ? [a-z] can be a bit limited/confusing ...
So this should be /sys/bus/devices/otp[a-z]/write_enable. What I was
trying to get across (but didn't do a good job of) is that you could
have an otp device (e.g. otpa) that could have multiple regions (otpa1,
otpa2...).
>
> > +What: /sys/bus/otp[a-z]/strict_programming
> > +Description:
> > + This file indicates whether all words in a redundant format
> > + must be programmed correctly to indicate success. Disabling
> > + this will mean that programming will be considered a success
> > + if the word can be read back correctly in it's redundant
> > + format.
>
> i dont really grok what this is trying to say ...
Our OTP has some redundancy in there so for example to store one 64 bit
word, it may store it in two locations the wire-OR them together to get
a stronger read when reading back (there are other formats too).
Disabling this attribute means that say one of the redundant words
doesn't program correctly but when both words are read together and
wire-OR'd the result is correct then we treat that as success. It's
basically trying to cope with minor errors for infield programming that
may not cause a real problem. I'll try and make that clearer though
because it isn't obvious.
> "it's" -> "its"
>
> > +What: /sys/bus/otp/devices/otp[a-z][0-9]*/format
>
> you have /sys/bus/otp[a-z]/ and /sys/bus/otp/devices/ ? why not unify them ?
>
> what are each of these subdirs trying to represent ?
They should all be in /sys/bus/otp/devices and otp[a-z] should be the
actual otp device and otp[a-z][0-9]+ should be the regions. I'll try
and make that a bit clearer and maybe put an example in there.
> > +Description:
> > + The redundancy format of the region. Valid values are:
> > + - single-ended (1 bit of storage per data bit).
> > + - redundant (2 bits of storage, wire-OR'd per data
> > + bit).
> > + - differential (2 bits of storage, differential
> > + voltage per data bit).
> > + - differential-redundant (4 bits of storage, combining
> > + redundant and differential).
> > + It is possible to increase redundancy of a region but care
> > + will be needed if there is data already in the region.
>
> where does ECC fit in ? the Blackfin OTP is structured:
> - first 4 pages are write control bitfields for all other pages (so
> blowing bit 5 of page 0 prevents further writing to page 5)
> - each 0x100 page region has 0x20 pages dedicated to ECC for the
> other 0x80 pages ... this provides 1 bit error correction and 2 bits
> of error detection (anymore and you're screwed!)
How does the ECC correction work for bfin at the moment? Does the user
specify it manually when writing then check it themselves when reading
back or does the hardware handle that?
The way I was thinking that this sort of thing could be handled would be
for the ECC to be transparent to the user. Perhaps for bfin the OTP
could be registered as 4 regions, otpa1-otpa4 which default to
"single-ended". Writing "ecc" as the format would generate the ecc and
program it for that region then check the ECC when reading back.
> > --- /dev/null
> > +++ b/drivers/otp/Kconfig
> > @@ -0,0 +1,10 @@
> > +#
> > +# Character device configuration
> > +#
>
> old comment
Will remove.
> > +menuconfig OTP
> > + bool "OTP memory support"
> > + help
> > + 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.
>
> is this limited to embedded devices ? i guess TPM keys and such are
> already handled by the TPM layers ...
>
> "y" -> "Y"
OK, that needs cleaning up.
>
> > --- /dev/null
> > +++ b/drivers/otp/otp.c
> > +#undef DEBUG
>
> dead code
Will remove.
> > +static int otp_open(struct inode *inode, struct file *filp);
> > +static int otp_release(struct inode *inode, struct file *filp);
> > +static ssize_t otp_write(struct file *filp, const char __user *buf,
> > + size_t len, loff_t *offs);
> > +static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
> > + loff_t *offs);
> > +static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
> > +
> > +static const struct file_operations otp_fops = {
> > + .owner = THIS_MODULE,
> > + .open = otp_open,
> > + .release = otp_release,
> > + .write = otp_write,
> > + .read = otp_read,
> > + .llseek = otp_llseek,
> > +};
>
> if you moved the fops down to where it is used, you wouldnt need the
> redundant func prototypes
Ok, will do.
> > +static DEFINE_SEMAPHORE(otp_sem);
> > +static int otp_we, otp_strict_programming;
> > +static struct otp_device *otp;
> > +static dev_t otp_devno;
>
> hrm, having these be global instead of per-device sounds like a really
> bad idea ...
So at the moment the only devices I'm aware of have a single OTP device
so I figured the most important thing was to make sure that the sysfs
and device node naming permitted for multiple OTP devices in the future
without ABI breakage. Having said that, these probably could be moved
into the otp_device though so I'll have a go at that.
> > +/*
> > + * Given a device for one of the otpN devices, get the corresponding
> > + * otp_region.
> > + */
> > +static inline struct otp_region *to_otp_region(struct device *dev)
> > +{
> > + return dev ? container_of(dev, struct otp_region, dev) : NULL;
> > +}
> > +
> > +static inline struct otp_device *to_otp_device(struct device *dev)
> > +{
> > + return dev ? container_of(dev, struct otp_device, dev) : NULL;
> > +}
>
> do you need the NULL checks ? none of the callers of these funcs
> check for NULL ...
I think so, because at least that way if something does go wrong and you
dereference the NULL later you should get a nice backtrace, but if you
don't do the check then you could have container_of() yielding something
like "NULL - 0x4" giving a potentially valid address. If that's the
wrong thing to do then I'm happy to change though.
> > +static ssize_t otp_format_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct otp_region *region = to_otp_region(dev);
> > + enum otp_redundancy_fmt fmt;
> > + const char *fmt_string;
> > +
> > + if (down_interruptible(&otp_sem))
> > + return -ERESTARTSYS;
> > +
> > + if (region->ops->get_fmt(region))
> > + fmt = region->ops->get_fmt(region);
> > + else
> > + fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
> > +
> > + up(&otp_sem);
>
> why are you using a semaphore when it looks like you're simply
> treating it as a mutex ? make more sense to use a proper mutex
> wouldnt it ?
Yes, that makes more sense, I'll update to a mutex.
> > + if (OTP_REDUNDANCY_FMT_SINGLE_ENDED == fmt)
> > + fmt_string = "single-ended";
> > + else if (OTP_REDUNDANCY_FMT_REDUNDANT == fmt)
> > + fmt_string = "redundant";
> > + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL == fmt)
> > + fmt_string = "differential";
> > + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT == fmt)
> > + fmt_string = "differential-redundant";
> > + else
> > + fmt_string = NULL;
> > +
> > + return fmt_string ? sprintf(buf, "%s\n", fmt_string) : -EINVAL;
>
> i'm pretty sure printf in the kernel can handle NULL with %s, so the
> NULL check can probably be punted
It's more so that the read() returns an error rather than an empty
string, but Greg has suggested returning early so I'll do that.
>
> > +static struct bus_type otp_bus_type = {
> > + .name = "otp",
> > +};
>
> can this be const ?
No, I don't think so; bus_register() doesn't want a const bus_type.
> > +struct attribute *region_attrs[] = {
> > + &dev_attr_format.attr,
> > + &dev_attr_size.attr,
> > + NULL,
> > +};
> > +
> > +const struct attribute_group region_attr_group = {
> > + .attrs = region_attrs,
> > +};
> > +
> > +const struct attribute_group *region_attr_groups[] = {
> > + ®ion_attr_group,
> > + NULL,
> > +};
> > +
> > +struct device_type region_type = {
> > + .name = "region",
> > + .groups = region_attr_groups,
> > +};
>
> should these be static ?
Yes, they should.
>
> > +/*
> > + * Show the current write enable state of the OTP. Users can only program the
> > + * OTP when this shows 'enabled'.
> > + */
> > +static ssize_t otp_we_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + int ret;
>
> this func has a ssize_t type but you're using int here. seems to come
> up a few times in this patch.
Ok, I'll go over these and fix them up.
>
> > +/*
> > + * Set the write enable state of the OTP. 'enabled' will enable programming
> > + * and 'disabled' will prevent further programming from occuring. On loading
>
> "occuring" -> "occurring"
Well spotted!
>
> > + * the module, this will default to 'disabled'.
> > + */
> > +static ssize_t otp_we_store(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + int err = 0;
> > +
> > + if (down_interruptible(&otp_sem))
> > + return -ERESTARTSYS;
> > +
> > + if (sysfs_streq(buf, "enabled"))
> > + otp_we = 1;
> > + else if (sysfs_streq(buf, "disabled"))
> > + otp_we = 0;
> > + else
> > + err = -EINVAL;
> > +
> > + up(&otp_sem);
> > +
> > + return err ?: len;
> > +}
> > +static DEVICE_ATTR(write_enable, S_IRUSR | S_IWUSR, otp_we_show, otp_we_store);
>
> you output and accept "enabled" and "disabled" for multiple values.
> how about unifying these ?
> return otp_attr_store_enabled(buf, len, &otp_we);
Good idea.
>
> > +static ssize_t otp_num_regions_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int nr_regions;
> > +
> > + if (down_interruptible(&otp_sem))
> > + return -ERESTARTSYS;
> > +
> > + nr_regions = otp->ops->get_nr_regions(otp);
> > +
> > + up(&otp_sem);
> > +
> > + if (nr_regions <= 0)
> > + return -EINVAL;
> > +
> > + return sprintf(buf, "%u\n", nr_regions);
> > +}
>
> i'm not sure returning -EINVAL here makes sense ... shouldnt it just
> be showing the user the result of get_nr_regions() ?
I guess so, but in that case we're saying that get_nr_regions()
shouldn't fail in which case the return type for get_nr_regions() should
probably be unsigned.
>
> > +struct attribute *otp_attrs[] = {
> > + &dev_attr_strict_programming.attr,
> > + &dev_attr_num_regions.attr,
> > + &dev_attr_write_enable.attr,
> > + NULL,
> > +};
> > +
> > +const struct attribute_group otp_attr_group = {
> > + .attrs = otp_attrs,
> > +};
> > +
> > +const struct attribute_group *otp_attr_groups[] = {
> > + &otp_attr_group,
> > + NULL,
> > +};
> > +
> > +struct device_type otp_type = {
> > + .name = "otp",
> > + .groups = otp_attr_groups,
> > +};
>
> static ?
Yes, will do.
>
> > +static void otp_dev_release(struct device *dev)
> > +{
> > + struct otp_device *otp = to_otp_device(dev);
> > +
> > + kfree(otp);
> > + otp = NULL;
> > +}
>
> setting to NULL here is pointless when the pointer is on the stack
That's actually a nasty one, I meant that to be the global otp rather
than the shadowing one. Ok, that's really bad and needs fixing!
>
> > +struct otp_device *otp_device_alloc(struct device *dev,
> > + const struct otp_device_ops *ops,
> > + size_t size)
> > +{
> > + struct otp_device *otp_dev = NULL;
> > + int err = -EINVAL;
> > +
> > + down(&otp_sem);
> > +
> > + if (dev && !get_device(dev)) {
> > + err = -ENODEV;
> > + goto out;
> > + }
>
> should you really be allowing dev==NULL ? does that setup make sense ?
Originally I didn't have that but when I added the bfin driver this
doesn't use a device structure so I made this change. Perhaps the right
approach is to require a parent device and make the bfin driver use a
platform device?
>
> > + if (otp) {
> > + pr_warning("an otp device already registered\n");
> > + err = -EBUSY;
> > + goto out_put;
> > + }
>
> you can only register one OTP device in the whole system ??
At the moment yes, but that limitation could probably be removed fairly
easily. I'll have a look at doing that.
>
> > +void otp_region_unregister(struct otp_region *region)
> > +{
> > + device_unregister(®ion->dev);
> > +}
> > +EXPORT_SYMBOL_GPL(otp_region_unregister);
>
> wonder if it'd be better to simply #define this in the global otp.h header
This was doing more originally, but yes that would be simpler given what
it does now.
>
> > +static ssize_t otp_write(struct file *filp, const char __user *buf, size_t len,
> > + loff_t *offs)
> > +{
> > + unsigned pos = (unsigned)*offs;
> > +
> > + len = min(len, region->ops->get_size(region) - (unsigned)*offs);
>
> what's with the unsigned cast ? defeats the point of using loff_t doesnt it ?
Ok, I'll revisit this.
>
> > + /*
> > + * We're now aligned to an 8 byte boundary so we can simply copy words
> > + * from userspace and write them into the OTP.
> > + */
> > + /*
> > + * We might have less than 8 bytes left so we'll need to do another
> > + * read-modify-write.
> > + */
> > + while (len >= OTP_WORD_SIZE) {
>
> i think "8 byte" should be "necessary byte" ?
Yes, it should.
>
> > + if (region->ops->read_word(region, pos / OTP_WORD_SIZE,
> > + &word)) {
> > + ret = -EIO;
> > + goto out;
> > + }
> > +
> > + if (region->ops->write_word(region, pos / OTP_WORD_SIZE,
> > + word)) {
> > + ret = -EIO;
> > + goto out;
> > + }
>
> shouldnt you pass the ret value up from read/write word ? this would
> allow the lower layers to better describe the issue than just -EIO
> wouldnt it ?
>
> last three comments apply to the read func as well ...
Yes, I'll fix these up.
>
> > +static int __init otp_init(void)
> > +{
> > + int err;
> > +
> > + err = bus_register(&otp_bus_type);
> > + if (err)
> > + return err;
> > +
> > + err = alloc_chrdev_region(&otp_devno, 0, 9, "otp");
>
> where'd that magic 9 come from ?
Ok, that's not nice. I'm not sure how to best handle this, perhaps
register a chrdev region when allocating the initial otp device and have
the backend specify the maximum number of regions?
>
> > +MODULE_DESCRIPTION("OTP interface driver");
>
> i think this is also a bus driver ?
Fair enough.
>
> > +/*
> > + * The OTP works in 64 bit words. When we are programming or reading,
> > + * everything is done with 64 bit word addresses.
> > + */
> > +#define OTP_WORD_SIZE 8
>
> should this be a per-device setting ? or wait until someone who
> doesnt have 64bit chunks show up ?
I guess it probably should to be generic but then the read_word() and
write_word() wrappers need to do some masking and shifting and I figured
as it was something I couldn't test it I shouldn't do it. Perhaps make
this a per device setting and have a runtime check that only supports 64
bit words for now?
>
> > + * struct otp_device - a picoxcell OTP device.
> > + * otp_device_alloc - create a new picoxcell OTP device.
>
> this is no longer picoxcell-specific
Ok.
>
> > + * otp_device_unregister - deregister an existing struct otp_device.
>
> "deregister" -> "unregister"
Ok.
Jamie
Hi Mike,
On Thu, Mar 24, 2011 at 02:35:00PM -0400, Mike Frysinger wrote:
> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> > Some drivers may need an ioctl method to provide device specific control
> > such as blackfin for providing locking. ?Regions can specify an ioctl to
> > be used but this field method is optional.
>
> shouldnt this just be squashed into patch 1 since you havent merged
> any of this yet ?
Probably yes, I was just trying to keep the patches smaller to review,
but in this case it probably isn't a sensible split.
>
> > +static long otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
> > +{
> > + ? ? ? struct otp_region *region = filp->private_data;
> > + ? ? ? long ret = -ENOTTY;
> > +
> > + ? ? ? if (down_interruptible(&otp_sem))
> > + ? ? ? ? ? ? ? return -ERESTARTSYS;
> > +
> > + ? ? ? if (region->ops->ioctl)
> > + ? ? ? ? ? ? ? ret = region->ops->ioctl(region, cmd, arg);
> > +
> > + ? ? ? up(&otp_sem);
> > +
> > + ? ? ? return ret;
> > +}
>
> the existence check can be pulled out of the locking
>
> long ret;
>
> if (!region->ops->ioctl)
> return -ENOTTY;
>
> if (down_interruptible(&otp_sem))
>
> return -ERESTARTSYS;
>
> ret = region->ops->ioctl(region, cmd, arg);
>
> up(&otp_sem);
>
> return ret;
>
Ok, will do.
Jamie
On Thu, Mar 24, 2011 at 02:36:42PM -0400, Mike Frysinger wrote:
> On Thu, Mar 24, 2011 at 14:32, Jamie Iles wrote:
> > On Thu, Mar 24, 2011 at 01:56:17PM -0400, Mike Frysinger wrote:
> >> just an overview note here ... the Blackfin OTP is actually IP we
> >> bought from someone (not sure if i can say more, although i dont know
> >> the details myself ... i'd have to go ask around), so much of the
> >> feedback i give about the structure of it most likely (hopefully) be
> >> applicable to other people who have purchased this IP. ?if/when anyone
> >> who is also using this IP steps forward, we could even see about
> >> generalizing the bfin-otp driver.
> >
> > Yes, we have a similar thing here - a block of OTP from an IP vendor and
> > a wrapper to provide an AXI interface. ?As far as I know there are only
> > a few OTP vendors so there may well be plenty of common code in
> > different OTP implementations.
>
> do you guys wrap the interface ? the Blackfin processors have an
> on-chip ROM which provides higher level functions like
> read/write/lock/etc... the Linux driver uses those rather than
> programming the memory mapped OTP interface itself since this
> interface is not publicly documented.
I'm not 100% on the exact details but I understand that the macro
provides some non-AXI registers and signals that we wrap up into some
AXI registers. The wrapper doesn't handle all of the nasties of the
macro but it does handle some of the redundancy and region splitting.
Jamie
On Thu, Mar 24, 2011 at 02:52:20PM -0400, Mike Frysinger wrote:
> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> > +config BFIN_OTP_WRITE_ENABLE
> > + ? ? ? bool "Enable writing support of OTP pages"
> > + ? ? ? depends on BFIN_OTP
> > + ? ? ? default n
> > + ? ? ? help
> > + ? ? ? ? If you say Y here, you will enable support for writing of the
> > + ? ? ? ? OTP pages. ?This is dangerous by nature as you can only program
> > + ? ? ? ? the pages once, so only enable this option when you actually
> > + ? ? ? ? need it so as to not inadvertently clobber data.
>
> if the generic layer is providing write enable access now, is this
> still necessary ? we could have a common OTP Kconfig i think for
> "enable OTP write support".
Sounds like a good idea. I'll integrate this into the next revision.
Jamie
Hi Greg,
Thanks for the review, a few comments inline.
Jamie
On Thu, Mar 24, 2011 at 12:20:05PM -0700, Greg KH wrote:
> On Thu, Mar 24, 2011 at 03:21:08PM +0000, Jamie Iles wrote:
> > +/*
> > + * 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.
>
> Do you _really_ mean "any later version"? Be sure about that please.
> You have that wording in all of your files you add, please be careful
> about it as you might have copied from code that did not have that
> wording (I'm not saying you did, just be sure about this.)
No I didn't mean to do that and I'm not sure where that came from. I'll
fix this up and take a look at some of my other patches! Thanks for
spotting that one.
>
> > + */
> > +#define pr_fmt(fmt) "otp: " fmt
> > +
> > +#undef DEBUG
>
> What is this for?
That shouldn't be there, I'll get rid of that.
>
> > +#include <linux/cdev.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/fs.h>
> > +#include <linux/io.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/otp.h>
> > +#include <linux/semaphore.h>
> > +#include <linux/slab.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/uaccess.h>
> > +
> > +static int otp_open(struct inode *inode, struct file *filp);
> > +static int otp_release(struct inode *inode, struct file *filp);
> > +static ssize_t otp_write(struct file *filp, const char __user *buf,
> > + size_t len, loff_t *offs);
> > +static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
> > + loff_t *offs);
> > +static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
> > +
> > +static const struct file_operations otp_fops = {
> > + .owner = THIS_MODULE,
> > + .open = otp_open,
> > + .release = otp_release,
> > + .write = otp_write,
> > + .read = otp_read,
> > + .llseek = otp_llseek,
> > +};
> > +
> > +static DEFINE_SEMAPHORE(otp_sem);
> > +static int otp_we, otp_strict_programming;
> > +static struct otp_device *otp;
> > +static dev_t otp_devno;
> > +
> > +/*
> > + * Given a device for one of the otpN devices, get the corresponding
> > + * otp_region.
> > + */
> > +static inline struct otp_region *to_otp_region(struct device *dev)
> > +{
> > + return dev ? container_of(dev, struct otp_region, dev) : NULL;
> > +}
> > +
> > +static inline struct otp_device *to_otp_device(struct device *dev)
> > +{
> > + return dev ? container_of(dev, struct otp_device, dev) : NULL;
> > +}
> > +
> > +bool otp_strict_programming_enabled(void)
> > +{
> > + return otp_strict_programming;
> > +}
> > +EXPORT_SYMBOL_GPL(otp_strict_programming_enabled);
> > +
> > +static ssize_t otp_format_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct otp_region *region = to_otp_region(dev);
> > + enum otp_redundancy_fmt fmt;
> > + const char *fmt_string;
> > +
> > + if (down_interruptible(&otp_sem))
> > + return -ERESTARTSYS;
> > +
> > + if (region->ops->get_fmt(region))
> > + fmt = region->ops->get_fmt(region);
> > + else
> > + fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
> > +
> > + up(&otp_sem);
> > +
> > + if (OTP_REDUNDANCY_FMT_SINGLE_ENDED == fmt)
>
> While some people feel this somehow makes it harder to write bad C code,
> it's not the kernel style. Please reverse this comparison. If you
> accidentally put a '=' in there instead of '==', gcc would warn you
> about it.
I'm making a conscience effort _not_ to do that but it's still ingrained
at the back of my mind and this code has been lingering about for a bit.
>
> > + fmt_string = "single-ended";
> > + else if (OTP_REDUNDANCY_FMT_REDUNDANT == fmt)
> > + fmt_string = "redundant";
> > + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL == fmt)
> > + fmt_string = "differential";
> > + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT == fmt)
> > + fmt_string = "differential-redundant";
> > + else
> > + fmt_string = NULL;
>
> Just return -EINVAL here.
>
> > +
> > + return fmt_string ? sprintf(buf, "%s\n", fmt_string) : -EINVAL;
>
> Then you don't have to do the embedded if in this statement.
>
> Same thing goes for your other show/store functions.
Ok, I'll clean these up.
>
> > +/**
> > + * struct otp_device - a picoxcell OTP device.
> > + *
> > + * @ops: The operations to use for manipulating the device.
> > + * @dev: The parent device (typically a platform_device) that
> > + * provides the OTP.
> > + * @regions: The regions registered to the device.
> > + * @size: The size of the OTP in bytes.
> > + * @driver_data: Private driver data.
> > + */
> > +struct otp_device {
> > + const struct otp_device_ops *ops;
> > + struct device dev;
> > + struct list_head regions;
> > + size_t size;
> > + void *driver_data;
>
> Why do you need this pointer, can't you use the one in struct device
> that is there for this purpose? Then provide a get/set function to
> access this field so that a driver doesn't go and poke in it directly.
Hmm, I thought I'd got rid of this; it isn't actually being used. I am
using the one in struct device but I haven't added the getter and setter
so I'll put those in for next time.
Jamie
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
On Thu, Mar 24, 2011 at 16:35, Jamie Iles wrote:
> On Thu, Mar 24, 2011 at 02:32:41PM -0400, Mike Frysinger wrote:
>> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
>> > +What: /sys/bus/otp/devices/otp[a-z][0-9]*/format
>>
>> you have /sys/bus/otp[a-z]/ and /sys/bus/otp/devices/ ? why not unify them ?
>>
>> what are each of these subdirs trying to represent ?
>
> They should all be in /sys/bus/otp/devices and otp[a-z] should be the
> actual otp device and otp[a-z][0-9]+ should be the regions. I'll try
> and make that a bit clearer and maybe put an example in there.
hrm, i dont think that layout is terribly clean. i dont think regions
from diff devices should be at the same level. if we think of otp
regions as partitions, then we can use existing standards.
so, how about:
/sys/bus/otp/devices/otp0/region0/
/sys/bus/otp/devices/otp0/region1/
/sys/bus/otp/devices/otp1/region0/
...
>> > +Description:
>> > + The redundancy format of the region. Valid values are:
>> > + - single-ended (1 bit of storage per data bit).
>> > + - redundant (2 bits of storage, wire-OR'd per data
>> > + bit).
>> > + - differential (2 bits of storage, differential
>> > + voltage per data bit).
>> > + - differential-redundant (4 bits of storage, combining
>> > + redundant and differential).
>> > + It is possible to increase redundancy of a region but care
>> > + will be needed if there is data already in the region.
>>
>> where does ECC fit in ? the Blackfin OTP is structured:
>> - first 4 pages are write control bitfields for all other pages (so
>> blowing bit 5 of page 0 prevents further writing to page 5)
>> - each 0x100 page region has 0x20 pages dedicated to ECC for the
>> other 0x80 pages ... this provides 1 bit error correction and 2 bits
>> of error detection (anymore and you're screwed!)
>
> How does the ECC correction work for bfin at the moment? Does the user
> specify it manually when writing then check it themselves when reading
> back or does the hardware handle that?
it is taken care of by the layers below the Linux driver. when we say
"read a half page", the 64bits are returned via pointer args, and the
function return value will tell us if the contents read are valid (ECC
passed), corrected (1 bit error), or there was a read error (too may
bit errors to recover).
> The way I was thinking that this sort of thing could be handled would be
> for the ECC to be transparent to the user. Perhaps for bfin the OTP
> could be registered as 4 regions, otpa1-otpa4 which default to
> "single-ended". Writing "ecc" as the format would generate the ecc and
> program it for that region then check the ECC when reading back.
reads and writes atm always have ECC enabled, and the driver restricts
writes to the full half page (64bits). if you wanted to be crafty,
you could blow individual bits in a single half page over multiple
writes before writing out the final ECC, but that isnt supported, and
no one has complained.
>> > +static DEFINE_SEMAPHORE(otp_sem);
>> > +static int otp_we, otp_strict_programming;
>> > +static struct otp_device *otp;
>> > +static dev_t otp_devno;
>>
>> hrm, having these be global instead of per-device sounds like a really
>> bad idea ...
>
> So at the moment the only devices I'm aware of have a single OTP device
> so I figured the most important thing was to make sure that the sysfs
> and device node naming permitted for multiple OTP devices in the future
> without ABI breakage. Having said that, these probably could be moved
> into the otp_device though so I'll have a go at that.
the ABI step is probably one of the most important pieces, but i dont
think moving these fields into the otp_device structure would take
long.
>> > +static inline struct otp_region *to_otp_region(struct device *dev)
>> > +{
>> > + return dev ? container_of(dev, struct otp_region, dev) : NULL;
>> > +}
>> > +
>> > +static inline struct otp_device *to_otp_device(struct device *dev)
>> > +{
>> > + return dev ? container_of(dev, struct otp_device, dev) : NULL;
>> > +}
>>
>> do you need the NULL checks ? none of the callers of these funcs
>> check for NULL ...
>
> I think so, because at least that way if something does go wrong and you
> dereference the NULL later you should get a nice backtrace, but if you
> don't do the check then you could have container_of() yielding something
> like "NULL - 0x4" giving a potentially valid address. If that's the
> wrong thing to do then I'm happy to change though.
ok, but the callers immediately dereference the return of this without
checking NULL, so i dont see the crash output being all that
different. and if people are mapping the low page (by manually
tweaking mmap_min_addr), then this func wont really make a difference
...
so yes, i'd still say punt it. there's no valid reason for a NULL dev
to be seen this low down.
>> > +static struct bus_type otp_bus_type = {
>> > + .name = "otp",
>> > +};
>>
>> can this be const ?
>
> No, I don't think so; bus_register() doesn't want a const bus_type.
stupid bus_register() needs updating then. i certainly wont ask you
to do that though :).
>> > +static ssize_t otp_num_regions_show(struct device *dev,
>> > + struct device_attribute *attr, char *buf)
>> > +{
>> > + int nr_regions;
>> > +
>> > + if (down_interruptible(&otp_sem))
>> > + return -ERESTARTSYS;
>> > +
>> > + nr_regions = otp->ops->get_nr_regions(otp);
>> > +
>> > + up(&otp_sem);
>> > +
>> > + if (nr_regions <= 0)
>> > + return -EINVAL;
>> > +
>> > + return sprintf(buf, "%u\n", nr_regions);
>> > +}
>>
>> i'm not sure returning -EINVAL here makes sense ... shouldnt it just
>> be showing the user the result of get_nr_regions() ?
>
> I guess so, but in that case we're saying that get_nr_regions()
> shouldn't fail in which case the return type for get_nr_regions() should
> probably be unsigned.
i'm ok with it returning an error if nr_regions is negative, but i
think 0 should be a valid value to show to userspace.
>> > +struct otp_device *otp_device_alloc(struct device *dev,
>> > + const struct otp_device_ops *ops,
>> > + size_t size)
>> > +{
>> > + struct otp_device *otp_dev = NULL;
>> > + int err = -EINVAL;
>> > +
>> > + down(&otp_sem);
>> > +
>> > + if (dev && !get_device(dev)) {
>> > + err = -ENODEV;
>> > + goto out;
>> > + }
>>
>> should you really be allowing dev==NULL ? does that setup make sense ?
>
> Originally I didn't have that but when I added the bfin driver this
> doesn't use a device structure so I made this change. Perhaps the right
> approach is to require a parent device and make the bfin driver use a
> platform device?
i'm OK with that
>> > +static int __init otp_init(void)
>> > +{
>> > + int err;
>> > +
>> > + err = bus_register(&otp_bus_type);
>> > + if (err)
>> > + return err;
>> > +
>> > + err = alloc_chrdev_region(&otp_devno, 0, 9, "otp");
>>
>> where'd that magic 9 come from ?
>
> Ok, that's not nice. I'm not sure how to best handle this, perhaps
> register a chrdev region when allocating the initial otp device and have
> the backend specify the maximum number of regions?
i dont think there's an issue with dynamically allocating an entire
major # for yourself. there's plenty available.
>> > +/*
>> > + * The OTP works in 64 bit words. When we are programming or reading,
>> > + * everything is done with 64 bit word addresses.
>> > + */
>> > +#define OTP_WORD_SIZE 8
>>
>> should this be a per-device setting ? or wait until someone who
>> doesnt have 64bit chunks show up ?
>
> I guess it probably should to be generic but then the read_word() and
> write_word() wrappers need to do some masking and shifting and I figured
> as it was something I couldn't test it I shouldn't do it. Perhaps make
> this a per device setting and have a runtime check that only supports 64
> bit words for now?
using constants also lets gcc do nice optimizations where it otherwise
might need to call math funcs to do mult/div/etc... operations. so
for now, simply adding an if() check to the register point and
returning -ENOSUP when the size isnt 8 bytes is OK. Blackfin OTP min
transfer size is 64bits :).
-mike
On Thu, Mar 24, 2011 at 16:59, Jamie Iles wrote:
> On Thu, Mar 24, 2011 at 02:50:35PM -0400, Mike Frysinger wrote:
>> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
>> > +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.
if that's what the datasheet says, then np, it's fine. presumably
you're not charging it for fun which means you cant background the
wait.
>> > + 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.
hmm, wasnt aware of this devm* stuff. sounds like fun and i could use
it elsewhere.
>> > +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!
that's why the pseudo "platform" bus exists in the first place. i
could somewhat understand if the probe function actually did probing
to see if the device in question existed before going further, but
this probe function simply assumes that if it gets called, the device
exists. as such, it makes a lot more sense to force people to "opt
in" via their platform resources. otherwise, the whole "build one
kernel but deploy to multiple boards" pretty much goes out the window.
-mike
On Thu, Mar 24, 2011 at 06:32:05PM +0000, Jamie Iles wrote:
> Yes, we have a similar thing here - a block of OTP from an IP vendor and
> a wrapper to provide an AXI interface. As far as I know there are only
> a few OTP vendors so there may well be plenty of common code in
> different OTP implementations.
Seems possible, though I'm not sure how much of the register interface
the OTP vendors provide so how much low level compatibility there might
be.
On Thu, Mar 24, 2011 at 05:33:25PM -0400, Mike Frysinger wrote:
> On Thu, Mar 24, 2011 at 16:35, Jamie Iles wrote:
> > On Thu, Mar 24, 2011 at 02:32:41PM -0400, Mike Frysinger wrote:
> >> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> >> > +Description:
> >> > + The redundancy format of the region. Valid values are:
> >> > + - single-ended (1 bit of storage per data bit).
> >> > + - redundant (2 bits of storage, wire-OR'd per data
> >> > + bit).
> >> > + - differential (2 bits of storage, differential
> >> > + voltage per data bit).
> >> > + - differential-redundant (4 bits of storage, combining
> >> > + redundant and differential).
> >> > + It is possible to increase redundancy of a region but care
> >> > + will be needed if there is data already in the region.
> >>
> >> where does ECC fit in ? the Blackfin OTP is structured:
> >> - first 4 pages are write control bitfields for all other pages (so
> >> blowing bit 5 of page 0 prevents further writing to page 5)
> >> - each 0x100 page region has 0x20 pages dedicated to ECC for the
> >> other 0x80 pages ... this provides 1 bit error correction and 2 bits
> >> of error detection (anymore and you're screwed!)
> >
> > How does the ECC correction work for bfin at the moment? Does the user
> > specify it manually when writing then check it themselves when reading
> > back or does the hardware handle that?
>
> it is taken care of by the layers below the Linux driver. when we say
> "read a half page", the 64bits are returned via pointer args, and the
> function return value will tell us if the contents read are valid (ECC
> passed), corrected (1 bit error), or there was a read error (too may
> bit errors to recover).
>
> > The way I was thinking that this sort of thing could be handled would be
> > for the ECC to be transparent to the user. Perhaps for bfin the OTP
> > could be registered as 4 regions, otpa1-otpa4 which default to
> > "single-ended". Writing "ecc" as the format would generate the ecc and
> > program it for that region then check the ECC when reading back.
>
> reads and writes atm always have ECC enabled, and the driver restricts
> writes to the full half page (64bits). if you wanted to be crafty,
> you could blow individual bits in a single half page over multiple
> writes before writing out the final ECC, but that isnt supported, and
> no one has complained.
The current version I have will do read-modify-write if the file offset
isn't OTP word aligned so it sounds like I need to add a flag that says
only do full word writes.
I guess in this framework we could make the OTP look like a single
region of the 0x80 data pages concatenated or provide separate regions
for each group of 0x80 pages but the control and ECC pages aren't
directly accessible by the user.
The other question that brings up is whether any compatibility with the
old driver is required.
Jamie
On Thursday 24 March 2011 16:21:08 Jamie Iles wrote:
> OTP memory is typically found in some embedded devices and can be
> used for storing boot code, cryptographic keys and other persistent
> information onchip. This patch adds a generic layer that devices can
> register OTP with and allows access through a set of character
> device nodes.
Looks very nice overall, but I have a few minor comments.
> diff --git a/Documentation/ABI/testing/sysfs-bus-otp b/Documentation/ABI/testing/sysfs-bus-otp
> new file mode 100644
> index 0000000..4dbc652
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-otp
> @@ -0,0 +1,68 @@
> +What: /sys/bus/otp/
> +Date: March 2011
> +KernelVersion: 2.6.40+
> +Contact: Jamie Iles <[email protected]>
> +Description:
> + The otp bus presents a number of devices where each
> + device represents a region or device in the SoC. Each region
> + will create a device node which allows the region to be
> + written with read()/write() calls and the device on the bus
> + has attributes for controlling the redundancy format and
> + getting the region size.
Why is this a bus? You don't have any device matching code or similar,
and the devices typically are on an existing bus_type (e.g. platform_bus).
I think it would make more sense to do this as a class.
> +
> +menuconfig OTP
> + bool "OTP memory support"
> + help
> + 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.
Please spell out OTP at least once here, a lot of people might not
know the TLA.
> +
> +static int otp_open(struct inode *inode, struct file *filp);
> +static int otp_release(struct inode *inode, struct file *filp);
> +static ssize_t otp_write(struct file *filp, const char __user *buf,
> + size_t len, loff_t *offs);
> +static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
> + loff_t *offs);
> +static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
> +
> +static const struct file_operations otp_fops = {
> + .owner = THIS_MODULE,
> + .open = otp_open,
> + .release = otp_release,
> + .write = otp_write,
> + .read = otp_read,
> + .llseek = otp_llseek,
> +};
The common way to organize source files is to put all definitions in an
order that requires no forward declarations.
Just define the operations first, then the struct file_operations, and
then the rest.
> +
> +static DEFINE_SEMAPHORE(otp_sem);
Semaphores are basically deprecated. Use a mutex instead.
> +static int otp_we, otp_strict_programming;
> +static struct otp_device *otp;
> +static dev_t otp_devno;
Making these global variables limits you to a single device. I think
you can put everything here into struct otp_device and find that from
the device or chardev you get passed when entering the otp code.
> +static void otp_dev_release(struct device *dev)
> +{
> + struct otp_device *otp = to_otp_device(dev);
> +
> + kfree(otp);
> + otp = NULL;
> +}
The final assignment is not needed.
Arnd
On Fri, Mar 25, 2011 at 09:12:22PM +0100, Arnd Bergmann wrote:
> On Thursday 24 March 2011 16:21:08 Jamie Iles wrote:
> > OTP memory is typically found in some embedded devices and can be
> > used for storing boot code, cryptographic keys and other persistent
> > information onchip. This patch adds a generic layer that devices can
> > register OTP with and allows access through a set of character
> > device nodes.
>
> Looks very nice overall, but I have a few minor comments.
>
> > diff --git a/Documentation/ABI/testing/sysfs-bus-otp b/Documentation/ABI/testing/sysfs-bus-otp
> > new file mode 100644
> > index 0000000..4dbc652
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-otp
> > @@ -0,0 +1,68 @@
> > +What: /sys/bus/otp/
> > +Date: March 2011
> > +KernelVersion: 2.6.40+
> > +Contact: Jamie Iles <[email protected]>
> > +Description:
> > + The otp bus presents a number of devices where each
> > + device represents a region or device in the SoC. Each region
> > + will create a device node which allows the region to be
> > + written with read()/write() calls and the device on the bus
> > + has attributes for controlling the redundancy format and
> > + getting the region size.
>
> Why is this a bus? You don't have any device matching code or similar,
> and the devices typically are on an existing bus_type (e.g. platform_bus).
> I think it would make more sense to do this as a class.
No, for new things, we want to use busses instead of classes please.
Especially as this does create devices, which are best put on a bus
somewhere.
thanks,
greg k-h
On Fri, Mar 25, 2011 at 06:08, Jamie Iles wrote:
> On Thu, Mar 24, 2011 at 05:33:25PM -0400, Mike Frysinger wrote:
>> On Thu, Mar 24, 2011 at 16:35, Jamie Iles wrote:
>> > The way I was thinking that this sort of thing could be handled would be
>> > for the ECC to be transparent to the user. Perhaps for bfin the OTP
>> > could be registered as 4 regions, otpa1-otpa4 which default to
>> > "single-ended". Writing "ecc" as the format would generate the ecc and
>> > program it for that region then check the ECC when reading back.
>>
>> reads and writes atm always have ECC enabled, and the driver restricts
>> writes to the full half page (64bits). if you wanted to be crafty,
>> you could blow individual bits in a single half page over multiple
>> writes before writing out the final ECC, but that isnt supported, and
>> no one has complained.
>
> The current version I have will do read-modify-write if the file offset
> isn't OTP word aligned so it sounds like I need to add a flag that says
> only do full word writes.
yes, only full 64bit writes are allowed.
> I guess in this framework we could make the OTP look like a single
> region of the 0x80 data pages concatenated or provide separate regions
> for each group of 0x80 pages but the control and ECC pages aren't
> directly accessible by the user.
well, in the current driver, i let people dump the ECC. for
debugging, i think that's valuable.
> The other question that brings up is whether any compatibility with the
> old driver is required.
it's already going to be changing names ;). but what other
compatibility are you thinking of ? the requirements are you have to
be able to:
- read it
- write in full/aligned 64bit chunks
- do an OTPLOCK ioctl (the drivers uses this to manage the first few
pages which flags all other pages as "locked")
-mike
On Friday 25 March 2011 22:12:46 Greg KH wrote:
> > Why is this a bus? You don't have any device matching code or similar,
> > and the devices typically are on an existing bus_type (e.g. platform_bus).
> > I think it would make more sense to do this as a class.
>
> No, for new things, we want to use busses instead of classes please.
> Especially as this does create devices, which are best put on a bus
> somewhere.
I don't understand. Isn't that rather inconsistent?
I realize the same thing came up with the IIO subsystem, where I also
didn't understand it.
In my mental model of Linux drivers, a bus is something that physically
connects devices and the bus code matches devices with drivers, while a
class groups logical devices that get created by the driver itself.
Arnd
Hi Arnd,
On Fri, Mar 25, 2011 at 09:12:22PM +0100, Arnd Bergmann wrote:
> On Thursday 24 March 2011 16:21:08 Jamie Iles wrote:
> > OTP memory is typically found in some embedded devices and can be
> > used for storing boot code, cryptographic keys and other persistent
> > information onchip. This patch adds a generic layer that devices can
> > register OTP with and allows access through a set of character
> > device nodes.
>
> Looks very nice overall, but I have a few minor comments.
Thanks for taking the time to look at this. I think I've addressed all
of your comments in the v3 series apart from defining OTP (which really
does need to be done so I'll make sure I do that in v4) and the sysfs
bus/class stuff. I understand that the intention is for a bus to
actually represent a subsystem so that does make sense to me (I think!).
Jamie
On Fri, Mar 25, 2011 at 05:32:54PM -0400, Mike Frysinger wrote:
> On Fri, Mar 25, 2011 at 06:08, Jamie Iles wrote:
> > I guess in this framework we could make the OTP look like a single
> > region of the 0x80 data pages concatenated or provide separate regions
> > for each group of 0x80 pages but the control and ECC pages aren't
> > directly accessible by the user.
>
> well, in the current driver, i let people dump the ECC. for
> debugging, i think that's valuable.
Hmm, that does sound potentially useful. In v3 I've created a single
partition that excludes the control bits and the ecc. How about I
change that for the next spin so there are two partitions where the
first contains the raw data and the second is all of the ECC lumped
together. It may also be useful to have a name attribute for each
region to make this clear.
It's either that or add another ioctl() for getting the ECC but that
doesn't feel very nice.
> > The other question that brings up is whether any compatibility with the
> > old driver is required.
>
> it's already going to be changing names ;). but what other
> compatibility are you thinking of ? the requirements are you have to
> be able to:
> - read it
> - write in full/aligned 64bit chunks
> - do an OTPLOCK ioctl (the drivers uses this to manage the first few
> pages which flags all other pages as "locked")
Well the current version will do all of that so I think if we add the
ECC dumping ability then everything is there.
Jamie
On Friday 25 March 2011 23:23:46 Jamie Iles wrote:
> Thanks for taking the time to look at this. I think I've addressed all
> of your comments in the v3 series apart from defining OTP (which really
> does need to be done so I'll make sure I do that in v4) and the sysfs
> bus/class stuff.
Wow, that was quick!
One more thing that I just realized:
I think it would be better not to allow arbitrary ioctl commands to
be interpreted by the individual drivers. Instead, interpret them
in the common code and pass the data to the drivers through separate
otp_device_ops function pointers, one per ioctl command.
This will reduce the amount of code needed in each driver when you
have multiple ones implementing the same ioctls, and help to
ensure that they all treat the arguments in the same way.
Also, you should have a compat_ioctl file operation. As long as
your data structures are compatible between 32 and 64 bit, it
can point to the same function as the .unlocked_ioctl one.
Arnd
On Fri, Mar 25, 2011 at 18:27, Jamie Iles wrote:
> On Fri, Mar 25, 2011 at 05:32:54PM -0400, Mike Frysinger wrote:
>> On Fri, Mar 25, 2011 at 06:08, Jamie Iles wrote:
>> > I guess in this framework we could make the OTP look like a single
>> > region of the 0x80 data pages concatenated or provide separate regions
>> > for each group of 0x80 pages but the control and ECC pages aren't
>> > directly accessible by the user.
>>
>> well, in the current driver, i let people dump the ECC. for
>> debugging, i think that's valuable.
>
> Hmm, that does sound potentially useful. In v3 I've created a single
> partition that excludes the control bits and the ecc. How about I
> change that for the next spin so there are two partitions where the
> first contains the raw data and the second is all of the ECC lumped
> together. It may also be useful to have a name attribute for each
> region to make this clear.
this does sound useful. on Blackfin parts, we've already dedicated
specific pages to specific functions. so if there was a name
attribute to each region, i could potentially make each one a region.
-mike
On Fri, Mar 25, 2011 at 18:35, Arnd Bergmann wrote:
> One more thing that I just realized:
> I think it would be better not to allow arbitrary ioctl commands to
> be interpreted by the individual drivers. Instead, interpret them
> in the common code and pass the data to the drivers through separate
> otp_device_ops function pointers, one per ioctl command.
>
> This will reduce the amount of code needed in each driver when you
> have multiple ones implementing the same ioctls, and help to
> ensure that they all treat the arguments in the same way.
i think we should do what the rtc framework did ... there are common
ioctls which call specific function pointers in the driver, and any
unhandled ioctls get passed to the driver-specific ioctl function.
this should cover everyone's needs.
-mike
On Fri, Mar 25, 2011 at 06:38:00PM -0400, Mike Frysinger wrote:
> On Fri, Mar 25, 2011 at 18:35, Arnd Bergmann wrote:
> > One more thing that I just realized:
> > I think it would be better not to allow arbitrary ioctl commands to
> > be interpreted by the individual drivers. Instead, interpret them
> > in the common code and pass the data to the drivers through separate
> > otp_device_ops function pointers, one per ioctl command.
> >
> > This will reduce the amount of code needed in each driver when you
> > have multiple ones implementing the same ioctls, and help to
> > ensure that they all treat the arguments in the same way.
>
> i think we should do what the rtc framework did ... there are common
> ioctls which call specific function pointers in the driver, and any
> unhandled ioctls get passed to the driver-specific ioctl function.
> this should cover everyone's needs.
That's what I intended but I guess it isn't that clear from the code.
Perhaps instead of:
mutex_lock();
region->ops->ioctl();
mutex_unlock();
if I change it to:
mutex_lock();
switch (cmd) {
default:
region->ops->ioctl();
}
mutex_unlock();
then that's a bit clearer. If there's stuff common across different OTP
implementations then we can add it to the region ops and decode it here
and if not fall back to the specific implementation.
Jamie
On Friday 25 March 2011 23:38:00 Mike Frysinger wrote:
> On Fri, Mar 25, 2011 at 18:35, Arnd Bergmann wrote:
> > One more thing that I just realized:
> > I think it would be better not to allow arbitrary ioctl commands to
> > be interpreted by the individual drivers. Instead, interpret them
> > in the common code and pass the data to the drivers through separate
> > otp_device_ops function pointers, one per ioctl command.
> >
> > This will reduce the amount of code needed in each driver when you
> > have multiple ones implementing the same ioctls, and help to
> > ensure that they all treat the arguments in the same way.
>
> i think we should do what the rtc framework did ... there are common
> ioctls which call specific function pointers in the driver, and any
> unhandled ioctls get passed to the driver-specific ioctl function.
> this should cover everyone's needs.
It depends on how obscure the ioctls get, but I think the first
attempt should always be to standardize them across the subsystem.
A lot of things are wrong with the whole concept, but when they
are needed, you should at least try to shield individual drivers
from them.
Many subsystems end up needing both common and driver specific
ioctls as they grow, I just wouldn't do that to start out with.
There's always hope that the latest subsystem will be better than
the others ;-).
Arnd
On Friday 25 March 2011 23:52:20 Jamie Iles wrote:
> That's what I intended but I guess it isn't that clear from the code.
> Perhaps instead of:
>
> mutex_lock();
> region->ops->ioctl();
> mutex_unlock();
>
> if I change it to:
>
> mutex_lock();
> switch (cmd) {
> default:
> region->ops->ioctl();
> }
> mutex_unlock();
>
> then that's a bit clearer. If there's stuff common across different OTP
> implementations then we can add it to the region ops and decode it here
> and if not fall back to the specific implementation.
This will make it a lot harder to support compat_ioctl as well.
Just drop the ->ioctl callback into the driver and make one
callback per command. Any command that could be reused on
another driver will then be defined in the common header file
already and just work on new drivers.
If you are tempted to add commands that cannot possibly work
on other hardware, that is usually a good indication that this
command should not be added, or should be generalized in some way.
Arnd
On Fri, Mar 25, 2011 at 11:01:56PM +0100, Arnd Bergmann wrote:
> On Friday 25 March 2011 22:12:46 Greg KH wrote:
> > > Why is this a bus? You don't have any device matching code or similar,
> > > and the devices typically are on an existing bus_type (e.g. platform_bus).
> > > I think it would make more sense to do this as a class.
> >
> > No, for new things, we want to use busses instead of classes please.
> > Especially as this does create devices, which are best put on a bus
> > somewhere.
>
> I don't understand. Isn't that rather inconsistent?
>
> I realize the same thing came up with the IIO subsystem, where I also
> didn't understand it.
>
> In my mental model of Linux drivers, a bus is something that physically
> connects devices and the bus code matches devices with drivers, while a
> class groups logical devices that get created by the driver itself.
Yes, that is how it used to be, but then it turns out that both of them
are really just "subsystems" as far as it all goes. They are just ways
that devices are bound to drivers in a logical manner. We have patches
floating around that get rid of both busses and classes to merge them
together, which is the end goal here. I know Kay has posted detailed
reasons for why this all is on lkml in the past, and had working code
about 5 years ago, it's just been slow going...
So we recommend all new subsystems use the bus code, it's simple and
works well.
thanks,
greg k-h
On Sat, Mar 26, 2011 at 12:02:19AM +0100, Arnd Bergmann wrote:
> On Friday 25 March 2011 23:52:20 Jamie Iles wrote:
> > That's what I intended but I guess it isn't that clear from the code.
> > Perhaps instead of:
> >
> > mutex_lock();
> > region->ops->ioctl();
> > mutex_unlock();
> >
> > if I change it to:
> >
> > mutex_lock();
> > switch (cmd) {
> > default:
> > region->ops->ioctl();
> > }
> > mutex_unlock();
> >
> > then that's a bit clearer. If there's stuff common across different OTP
> > implementations then we can add it to the region ops and decode it here
> > and if not fall back to the specific implementation.
>
> This will make it a lot harder to support compat_ioctl as well.
> Just drop the ->ioctl callback into the driver and make one
> callback per command. Any command that could be reused on
> another driver will then be defined in the common header file
> already and just work on new drivers.
OK, you've convinced me :-)
The only use of an ioctl() at the moment is for locking portions of the
OTP so perhaps we add a new region op .lock(unsigned long start_addr,
size_t nr_words).
For the actual ioctl() we should assume byte addressing rather than
words though and do the conversion in the driver so we can cope with
devices that don't have 64-bit words and do the locking on a looping
word-by-word basis.
struct otp_lock_req {
__u32 start_addr;
__u32 byte_count;
};
The start_addr would be relative to the start of the region.
Mike, would this be OK with you if we used a different ioctl() to the
one bfin-otp is using currently? I notice that it's using the OTPLOCK
ioctl() from MTD but I think it's using the argument in a different way.
Jamie
On Fri, Mar 25, 2011 at 20:21, Jamie Iles wrote:
> For the actual ioctl() we should assume byte addressing rather than
> words though and do the conversion in the driver so we can cope with
> devices that don't have 64-bit words and do the locking on a looping
> word-by-word basis.
>
> struct otp_lock_req {
> __u32 start_addr;
> __u32 byte_count;
> };
i would add an ABI field here too so if in the future we want to add
stuff, we can do so without adding new ioctls. like "u16 version; u16
flags;". then in the ioctl, if version isnt 0, we return ENOTSUP. in
the future, we can add flags or bump the version.
> Mike, would this be OK with you if we used a different ioctl() to the
> one bfin-otp is using currently? I notice that it's using the OTPLOCK
> ioctl() from MTD but I think it's using the argument in a different way.
i re-used OTPLOCK because it's exactly the name i wanted and it was
easier than carving out my own namespace, but the args are different.
i can see how people might find this undesirable.
-mike
On Fri, Mar 25, 2011 at 10:16:48PM -0400, Mike Frysinger wrote:
> On Fri, Mar 25, 2011 at 20:21, Jamie Iles wrote:
> > For the actual ioctl() we should assume byte addressing rather than
> > words though and do the conversion in the driver so we can cope with
> > devices that don't have 64-bit words and do the locking on a looping
> > word-by-word basis.
> >
> > ? ? ? ?struct otp_lock_req {
> > ? ? ? ? ? ? ? ?__u32 ? start_addr;
> > ? ? ? ? ? ? ? ?__u32 ? byte_count;
> > ? ? ? ?};
>
> i would add an ABI field here too so if in the future we want to add
> stuff, we can do so without adding new ioctls. like "u16 version; u16
> flags;". then in the ioctl, if version isnt 0, we return ENOTSUP. in
> the future, we can add flags or bump the version.
Sounds like a good idea.
> > Mike, would this be OK with you if we used a different ioctl() to the
> > one bfin-otp is using currently? ?I notice that it's using the OTPLOCK
> > ioctl() from MTD but I think it's using the argument in a different way.
>
> i re-used OTPLOCK because it's exactly the name i wanted and it was
> easier than carving out my own namespace, but the args are different.
> i can see how people might find this undesirable.
OK, well seeing as we're making this generic we may as well reserve a
range of ioctls. I'll reserve a few of them and add OTP_LOCK_AREA for
now.
Jamie
On Saturday 26 March 2011 03:16:48 Mike Frysinger wrote:
> On Fri, Mar 25, 2011 at 20:21, Jamie Iles wrote:
> > For the actual ioctl() we should assume byte addressing rather than
> > words though and do the conversion in the driver so we can cope with
> > devices that don't have 64-bit words and do the locking on a looping
> > word-by-word basis.
> >
> > struct otp_lock_req {
> > __u32 start_addr;
> > __u32 byte_count;
> > };
Looks good to me.
> i would add an ABI field here too so if in the future we want to add
> stuff, we can do so without adding new ioctls. like "u16 version; u16
> flags;". then in the ioctl, if version isnt 0, we return ENOTSUP. in
> the future, we can add flags or bump the version.
No, versioned APIs are just a pain to maintain, it's easier to add
new ioctl commands when necessary.
Note that the kernel would always have to support all versions of
the API anyway.
Arnd
On Saturday 26 March 2011 00:28:47 Greg KH wrote:
> Yes, that is how it used to be, but then it turns out that both of them
> are really just "subsystems" as far as it all goes. They are just ways
> that devices are bound to drivers in a logical manner. We have patches
> floating around that get rid of both busses and classes to merge them
> together, which is the end goal here. I know Kay has posted detailed
> reasons for why this all is on lkml in the past, and had working code
> about 5 years ago, it's just been slow going...
How will that work? I suppose we can't really change the directory
structure anyway, so to users it will still look like it does today,
even if the kernel just uses the same code for bus and class internally.
> So we recommend all new subsystems use the bus code, it's simple and
> works well.
I'm not worried about the code at all, just how it shows up in
sysfs, which is rather confusing when things become a bus that
are not really buses by any stretch. Obviously there are grey
areas and corner cases where a bus makes as much sense as a class,
but I think this one is not such a case.
Arnd
On Sat, Mar 26, 2011 at 12:25:03PM +0100, Arnd Bergmann wrote:
> On Saturday 26 March 2011 00:28:47 Greg KH wrote:
>
> > Yes, that is how it used to be, but then it turns out that both of them
> > are really just "subsystems" as far as it all goes. They are just ways
> > that devices are bound to drivers in a logical manner. We have patches
> > floating around that get rid of both busses and classes to merge them
> > together, which is the end goal here. I know Kay has posted detailed
> > reasons for why this all is on lkml in the past, and had working code
> > about 5 years ago, it's just been slow going...
>
> How will that work? I suppose we can't really change the directory
> structure anyway, so to users it will still look like it does today,
> even if the kernel just uses the same code for bus and class internally.
Yes. But we can have symlinks instead of "real" /sys/class and
/sys/bus directories like we do for /sys/block today. All of the major
tools that use sysfs work properly with a /sys/subsystem/ setup today
thanks to Kay's efforts.
thanks,
greg k-h
On Sat, Mar 26, 2011 at 06:54, Arnd Bergmann wrote:
> On Saturday 26 March 2011 03:16:48 Mike Frysinger wrote:
>> On Fri, Mar 25, 2011 at 20:21, Jamie Iles wrote:
>> > For the actual ioctl() we should assume byte addressing rather than
>> > words though and do the conversion in the driver so we can cope with
>> > devices that don't have 64-bit words and do the locking on a looping
>> > word-by-word basis.
>> >
>> > struct otp_lock_req {
>> > __u32 start_addr;
>> > __u32 byte_count;
>> > };
>
> Looks good to me.
>
>> i would add an ABI field here too so if in the future we want to add
>> stuff, we can do so without adding new ioctls. like "u16 version; u16
>> flags;". then in the ioctl, if version isnt 0, we return ENOTSUP. in
>> the future, we can add flags or bump the version.
>
> No, versioned APIs are just a pain to maintain, it's easier to add
> new ioctl commands when necessary.
> Note that the kernel would always have to support all versions of
> the API anyway.
adding at least a flags field then should be fine
-mike
On Saturday 26 March 2011 18:55:39 Mike Frysinger wrote:
> >> i would add an ABI field here too so if in the future we want to add
> >> stuff, we can do so without adding new ioctls. like "u16 version; u16
> >> flags;". then in the ioctl, if version isnt 0, we return ENOTSUP. in
> >> the future, we can add flags or bump the version.
> >
> > No, versioned APIs are just a pain to maintain, it's easier to add
> > new ioctl commands when necessary.
> > Note that the kernel would always have to support all versions of
> > the API anyway.
>
> adding at least a flags field then should be fine
Fine with me, if you can name at least one possible use for a flag.
I think it would be better without a flags field, but it's not a
problem if you insist on having it.
Arnd
On Sat, Mar 26, 2011 at 16:51, Arnd Bergmann wrote:
> On Saturday 26 March 2011 18:55:39 Mike Frysinger wrote:
>> >> i would add an ABI field here too so if in the future we want to add
>> >> stuff, we can do so without adding new ioctls. like "u16 version; u16
>> >> flags;". then in the ioctl, if version isnt 0, we return ENOTSUP. in
>> >> the future, we can add flags or bump the version.
>> >
>> > No, versioned APIs are just a pain to maintain, it's easier to add
>> > new ioctl commands when necessary.
>> > Note that the kernel would always have to support all versions of
>> > the API anyway.
>>
>> adding at least a flags field then should be fine
>
> Fine with me, if you can name at least one possible use for a flag.
i tend to assume that i cant think of all cases when i first write up
something, so better to leave a hole for the future ;)
-mike
On Sunday 27 March 2011 05:52:04 Mike Frysinger wrote:
> On Sat, Mar 26, 2011 at 16:51, Arnd Bergmann wrote:
> > Fine with me, if you can name at least one possible use for a flag.
>
> i tend to assume that i cant think of all cases when i first write up
> something, so better to leave a hole for the future ;)
This makes sense for real syscalls, where adding a new one is a lot
of work. For an ioctl, adding a flag is almost exactly the same
amount of work as adding another ioctl command, but it adds complexity
for the first one, e.g. you need to decide how to handle unknown
flags passed to the kernel (ignore, bail out, or a combination).
It also makes correctly using the ioctl harder and adds a tiny
amount of overhead for parsing the flags and copying the addititional
word. Adding a flags word "just in case" is therfore counterproductive.
Arnd
On Saturday 26 March 2011, Greg KH wrote:
> On Sat, Mar 26, 2011 at 12:25:03PM +0100, Arnd Bergmann wrote:
> > On Saturday 26 March 2011 00:28:47 Greg KH wrote:
> >
> > > Yes, that is how it used to be, but then it turns out that both of them
> > > are really just "subsystems" as far as it all goes. They are just ways
> > > that devices are bound to drivers in a logical manner. We have patches
> > > floating around that get rid of both busses and classes to merge them
> > > together, which is the end goal here. I know Kay has posted detailed
> > > reasons for why this all is on lkml in the past, and had working code
> > > about 5 years ago, it's just been slow going...
> >
> > How will that work? I suppose we can't really change the directory
> > structure anyway, so to users it will still look like it does today,
> > even if the kernel just uses the same code for bus and class internally.
>
> Yes. But we can have symlinks instead of "real" /sys/class and
> /sys/bus directories like we do for /sys/block today. All of the major
> tools that use sysfs work properly with a /sys/subsystem/ setup today
> thanks to Kay's efforts.
I see. The migration from /sys/bus to /sys/subsystem plus symlinks
makes sense, but I still think that having symlinks that make sense
would be better than having symlinks that are purely there for historical
reasons.
One major flaw I see with registering abstract subsystems as a bus is
that a bus connected to the concept of a device_driver, and there is
a list of drivers with their respective devices in /sys/bus/*/drivers,
which would always be empty in this case. Similarly, the
/sys/bus/*/devices/*/driver symlinks for these subsystems would point
to drivers on other buses, unlike the respective symlinks for real
buses.
Arnd