[PATCH v3 01/10] misc: add driver for the Sifive otp controller
Pragnesh Patel
pragnesh.patel at sifive.com
Mon Jan 27 11:18:18 CET 2020
>-----Original Message-----
>From: Jagan Teki <jagan at amarulasolutions.com>
>Sent: 24 January 2020 12:12
>To: Pragnesh Patel <pragnesh.patel at sifive.com>
>Cc: U-Boot-Denx <u-boot at lists.denx.de>; Atish Patra
><atish.patra at wdc.com>; palmerdabbelt at google.com; Bin Meng
><bmeng.cn at gmail.com>; Paul Walmsley ( Sifive)
><paul.walmsley at sifive.com>; Troy Benjegerdes ( Sifive)
><troy.benjegerdes at sifive.com>; Anup Patel <anup.patel at wdc.com>; Sagar
>Kadam <sagar.kadam at sifive.com>; Rick Chen <rick at andestech.com>; Simon
>Glass <sjg at chromium.org>; Heiko Stuebner <heiko.stuebner at theobroma-
>systems.com>; Michal Simek <michal.simek at xilinx.com>; Marcel Ziswiler
><marcel.ziswiler at toradex.com>; Finley Xiao <finley.xiao at rock-chips.com>;
>Peng Fan <peng.fan at nxp.com>; Tero Kristo <t-kristo at ti.com>; Eugen Hristev
><eugen.hristev at microchip.com>
>Subject: Re: [PATCH v3 01/10] misc: add driver for the Sifive otp controller
>
>On Fri, Jan 24, 2020 at 11:21 AM Pragnesh Patel <pragnesh.patel at sifive.com>
>wrote:
>>
>> Added a misc driver to handle OTP memory in FU540.
>>
>> Signed-off-by: Pragnesh Patel <pragnesh.patel at sifive.com>
>> Reviewed-by: Anup Patel <anup.patel at wdc.com>
>> ---
>> arch/riscv/dts/fu540-c000-u-boot.dtsi | 13 ++
>> .../dts/hifive-unleashed-a00-u-boot.dtsi | 6 +
>> board/sifive/fu540/fu540.c | 113 ++++------
>> configs/sifive_fu540_defconfig | 2 +
>> drivers/misc/Kconfig | 7 +
>> drivers/misc/Makefile | 1 +
>> drivers/misc/ememory-otp.c | 207 ++++++++++++++++++
>
>This patch need to break into
>1. add sifive otp driver
>2. enable the otp driver - board, dts, defconfig changes.
Will split in v4.
>
>> 7 files changed, 276 insertions(+), 73 deletions(-) create mode
>> 100644 arch/riscv/dts/fu540-c000-u-boot.dtsi
>> create mode 100644 arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi
>> create mode 100644 drivers/misc/ememory-otp.c
>>
>> diff --git a/arch/riscv/dts/fu540-c000-u-boot.dtsi
>> b/arch/riscv/dts/fu540-c000-u-boot.dtsi
>> new file mode 100644
>> index 0000000000..615a68c0e9
>> --- /dev/null
>> +++ b/arch/riscv/dts/fu540-c000-u-boot.dtsi
>> @@ -0,0 +1,13 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2019 SiFive, Inc
>> + */
>> +
>> +/ {
>> + soc {
>> + otp: otp at 10070000 {
>> + compatible = "sifive,fu540-otp";
>> + reg = <0x0 0x10070000 0x0 0x0FFF>;
>> + };
>> + };
>> +};
>> diff --git a/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi
>> b/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi
>> new file mode 100644
>> index 0000000000..bec0d19134
>> --- /dev/null
>> +++ b/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi
>> @@ -0,0 +1,6 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2019 SiFive, Inc
>> + */
>> +
>> +#include "fu540-c000-u-boot.dtsi"
>> diff --git a/board/sifive/fu540/fu540.c b/board/sifive/fu540/fu540.c
>> index 47a2090251..3a5e74f1fb 100644
>> --- a/board/sifive/fu540/fu540.c
>> +++ b/board/sifive/fu540/fu540.c
>> @@ -10,94 +10,61 @@
>> #include <dm.h>
>> #include <linux/delay.h>
>> #include <linux/io.h>
>> +#include <misc.h>
>>
>> -#ifdef CONFIG_MISC_INIT_R
>> -
>> -#define FU540_OTP_BASE_ADDR 0x10070000
>> -
>> -struct fu540_otp_regs {
>> - u32 pa; /* Address input */
>> - u32 paio; /* Program address input */
>> - u32 pas; /* Program redundancy cell selection input */
>> - u32 pce; /* OTP Macro enable input */
>> - u32 pclk; /* Clock input */
>> - u32 pdin; /* Write data input */
>> - u32 pdout; /* Read data output */
>> - u32 pdstb; /* Deep standby mode enable input (active low) */
>> - u32 pprog; /* Program mode enable input */
>> - u32 ptc; /* Test column enable input */
>> - u32 ptm; /* Test mode enable input */
>> - u32 ptm_rep;/* Repair function test mode enable input */
>> - u32 ptr; /* Test row enable input */
>> - u32 ptrim; /* Repair function enable input */
>> - u32 pwe; /* Write enable input (defines program cycle) */
>> -} __packed;
>> -
>> -#define BYTES_PER_FUSE 4
>> -#define NUM_FUSES 0x1000
>> -
>> -static int fu540_otp_read(int offset, void *buf, int size) -{
>> - struct fu540_otp_regs *regs = (void __iomem
>*)FU540_OTP_BASE_ADDR;
>> - unsigned int i;
>> - int fuseidx = offset / BYTES_PER_FUSE;
>> - int fusecount = size / BYTES_PER_FUSE;
>> - u32 fusebuf[fusecount];
>> -
>> - /* check bounds */
>> - if (offset < 0 || size < 0)
>> - return -EINVAL;
>> - if (fuseidx >= NUM_FUSES)
>> - return -EINVAL;
>> - if ((fuseidx + fusecount) > NUM_FUSES)
>> - return -EINVAL;
>> -
>> - /* init OTP */
>> - writel(0x01, ®s->pdstb); /* wake up from stand-by */
>> - writel(0x01, ®s->ptrim); /* enable repair function */
>> - writel(0x01, ®s->pce); /* enable input */
>> -
>> - /* read all requested fuses */
>> - for (i = 0; i < fusecount; i++, fuseidx++) {
>> - writel(fuseidx, ®s->pa);
>> -
>> - /* cycle clock to read */
>> - writel(0x01, ®s->pclk);
>> - mdelay(1);
>> - writel(0x00, ®s->pclk);
>> - mdelay(1);
>> -
>> - /* read the value */
>> - fusebuf[i] = readl(®s->pdout);
>> - }
>> -
>> - /* shut down */
>> - writel(0, ®s->pce);
>> - writel(0, ®s->ptrim);
>> - writel(0, ®s->pdstb);
>> -
>> - /* copy out */
>> - memcpy(buf, fusebuf, size);
>> +/*
>> + * This define is a value used for error/unknown serial.
>> + * If we really care about distinguishing errors and 0 is
>> + * valid, we'll need a different one.
>> + */
>> +#define ERROR_READING_SERIAL_NUMBER 0
>>
>> - return 0;
>> -}
>> +#ifdef CONFIG_MISC_INIT_R
>>
>> -static u32 fu540_read_serialnum(void)
>> +#if CONFIG_IS_ENABLED(EMEMORY_OTP)
>> +static u32 otp_read_serialnum(struct udevice *dev)
>> {
>> int ret;
>> u32 serial[2] = {0};
>>
>> for (int i = 0xfe * 4; i > 0; i -= 8) {
>> - ret = fu540_otp_read(i, serial, sizeof(serial));
>> + ret = misc_read(dev, i, serial, sizeof(serial));
>> +
>> if (ret) {
>> - printf("%s: error reading from OTP\n", __func__);
>> + printf("%s: error reading serial from OTP\n",
>> + __func__);
>> break;
>> }
>> +
>> if (serial[0] == ~serial[1])
>> return serial[0];
>> }
>>
>> - return 0;
>> + return ERROR_READING_SERIAL_NUMBER; } #endif
>> +
>> +static u32 fu540_read_serialnum(void) {
>> + u32 serial = ERROR_READING_SERIAL_NUMBER;
>> +
>> +#if CONFIG_IS_ENABLED(EMEMORY_OTP)
>> + struct udevice *dev;
>> + int ret;
>> +
>> + // init OTP
>> + ret = uclass_get_device_by_driver(UCLASS_MISC,
>> + DM_GET_DRIVER(hifive_otp),
>> + &dev);
>> +
>> + if (ret) {
>> + debug("%s: could not find otp device\n", __func__);
>> + return serial;
>> + }
>> +
>> + // read serial from OTP and set env var
>> + serial = otp_read_serialnum(dev); #endif
>> +
>> + return serial;
>> }
>>
>> static void fu540_setup_macaddr(u32 serialnum) diff --git
>> a/configs/sifive_fu540_defconfig b/configs/sifive_fu540_defconfig
>> index 6d61e6c960..40e78f12a2 100644
>> --- a/configs/sifive_fu540_defconfig
>> +++ b/configs/sifive_fu540_defconfig
>> @@ -6,6 +6,8 @@ CONFIG_ARCH_RV64I=y
>> CONFIG_RISCV_SMODE=y
>> CONFIG_DISTRO_DEFAULTS=y
>> CONFIG_FIT=y
>> +CONFIG_MISC=y
>> +CONFIG_EMEMORY_OTP=y
>
>Look like this opt is needed global to SiFive, If so select MISC in arch Kconfig.
Will update the "board/Sifive/fu540/Kconfig" in v4.
>
>> CONFIG_MISC_INIT_R=y
>> CONFIG_DISPLAY_CPUINFO=y
>> CONFIG_DISPLAY_BOARDINFO=y
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
>> f18aa8f7ba..cbda0deb14 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -68,6 +68,13 @@ config ROCKCHIP_OTP
>> addressing and a length or through child-nodes that are generated
>> based on the e-fuse map retrieved from the DTS.
>>
>> +config EMEMORY_OTP
>
>What about SIFIVE_OTP or SIFIVE_EMEM_OTP or similar?
Will change it to SIFIVE_OTP in v4.
>
>> + bool "Sifive Ememory OTP driver"
>> + depends on RISCV && MISC
>> + help
>> + Enable support for reading the Ememory OTP on the HiFive Unleashed
>> + OTP storage.
>> +
>> config VEXPRESS_CONFIG
>> bool "Enable support for Arm Versatile Express config bus"
>> depends on MISC
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
>> 2b843de93c..dcf9b628c8 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_$(SPL_)PWRSEQ) += pwrseq-uclass.o
>> obj-$(CONFIG_QFW) += qfw.o
>> obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o
>> obj-$(CONFIG_ROCKCHIP_OTP) += rockchip-otp.o
>> +obj-$(CONFIG_EMEMORY_OTP) += ememory-otp.o
>> obj-$(CONFIG_SANDBOX) += syscon_sandbox.o misc_sandbox.o
>> obj-$(CONFIG_SMSC_LPC47M) += smsc_lpc47m.o
>> obj-$(CONFIG_SMSC_SIO1007) += smsc_sio1007.o diff --git
>> a/drivers/misc/ememory-otp.c b/drivers/misc/ememory-otp.c new file
>> mode 100644 index 0000000000..73e7af496a
>> --- /dev/null
>> +++ b/drivers/misc/ememory-otp.c
>
>Again file name, would have platform or soc naming convention.
Agreed, will change in v4. Thanks for pointing me.
>
>> @@ -0,0 +1,207 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * This is a driver for the eMemory EG004K32TQ028XW01 NeoFuse
>> + * One-Time-Programmable (OTP) memory used within the SiFive FU540.
>> + * It is documented in the FU540 manual here:
>> + *
>> +https://www.sifive.com/documentation/chips/freedom-u540-c000-
>manual/
>> + *
>> + * Copyright (C) 2018 Philipp Hug <philipp at hug.cx>
>> + * Copyright (C) 2018 Joey Hewitt <joey at joeyhewitt.com>
>> + *
>> + * Copyright (C) 2020 SiFive, Inc
>> + */
>> +
>> +/*
>> + * The FU540 stores 4096x32 bit (16KiB) values.
>> + * Index 0x00-0xff are reserved for SiFive internal use. (first 1KiB)
>> +*/
>
>Sorry, I didn't understand this comments. If it is memory map detail that
>occupy otp, try to elaborate more by giving full details. Would helpful to
>understand anyone in future.
Right now, OTP is used only for serial number. First 1 KB is reserved for Sifive
Internal use, serial number is one of them.
>
>> +
>> +#include <common.h>
>> +#include <dm/device.h>
>> +#include <dm/read.h>
>> +#include <linux/io.h>
>> +#include <misc.h>
>> +
>> +struct hifive_otp_regs {
>> + u32 pa; /* Address input */
>> + u32 paio; /* Program address input */
>> + u32 pas; /* Program redundancy cell selection input */
>> + u32 pce; /* OTP Macro enable input */
>> + u32 pclk; /* Clock input */
>> + u32 pdin; /* Write data input */
>> + u32 pdout; /* Read data output */
>> + u32 pdstb; /* Deep standby mode enable input (active low) */
>> + u32 pprog; /* Program mode enable input */
>> + u32 ptc; /* Test column enable input */
>> + u32 ptm; /* Test mode enable input */
>> + u32 ptm_rep;/* Repair function test mode enable input */
>> + u32 ptr; /* Test row enable input */
>> + u32 ptrim; /* Repair function enable input */
>> + u32 pwe; /* Write enable input (defines program cycle) */
>> +} __packed;
>> +
>> +struct hifive_otp_platdata {
>
>hifive here represent the board name, butter use soc name that has this otp.
Will update in v4.
>
>> + struct hifive_otp_regs __iomem *regs; };
>> +
>> +typedef u32 fuse_value_t;
>
>No typdef please.
>No global variables(unless it really need)
Will update in v4, thanks.
>
>> +#define BYTES_PER_FUSE 4
>> +
>> +#define NUM_FUSES 0x1000
>
>May be use dt properties.
Will check this.
>
>> +
>> +/*
>> + * offset and size are assumed aligned to the size of the fuses (32bit).
>> + */
>> +static int hifive_otp_read(struct udevice *dev, int offset,
>> + void *buf, int size) {
>> + struct hifive_otp_platdata *plat = dev_get_platdata(dev);
>> + struct hifive_otp_regs *regs = (struct hifive_otp_regs
>> +*)plat->regs;
>> +
>> + int fuseidx = offset / BYTES_PER_FUSE;
>> + int fusecount = size / BYTES_PER_FUSE;
>> + fuse_value_t fusebuf[fusecount];
>> +
>> + // check bounds
>
>use proper comment style.
Will update in v4.
>
>> + if (offset < 0 || size < 0)
>> + return -EINVAL;
>> + if (fuseidx >= NUM_FUSES)
>> + return -EINVAL;
>> + if ((fuseidx + fusecount) > NUM_FUSES)
>> + return -EINVAL;
>> +
>> + // init OTP
>> + iowrite32(0x01, ®s->pdstb); // wake up from stand-by
>> + iowrite32(0x01, ®s->ptrim); // enable repair function
>> + iowrite32(0x01, ®s->pce); // enable input
>
>use macros with proper bit names, like 0x01 would have proper macro.
Will update in v4.
>
>> +
>> + // read all requested fuses
>> + for (unsigned int i = 0; i < fusecount; i++, fuseidx++) {
>> + iowrite32(fuseidx, ®s->pa);
>> +
>> + // cycle clock to read
>> + iowrite32(0x01, ®s->pclk);
>> + mdelay(1);
>> + iowrite32(0x00, ®s->pclk);
>> + mdelay(1);
>> +
>> + // read the value
>> + fusebuf[i] = ioread32(®s->pdout);
>> + }
>> +
>> + // shut down
>> + iowrite32(0, ®s->pce);
>> + iowrite32(0, ®s->ptrim);
>> + iowrite32(0, ®s->pdstb);
>> +
>> + // copy out
>> + memcpy(buf, fusebuf, size);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Caution:
>> + * OTP can be write only once, so use carefully.
>
>can be written
Thanks, will update in v4.
>
>> + *
>> + * offset and size are assumed aligned to the size of the fuses (32bit).
>> + */
>> +static int hifive_otp_write(struct udevice *dev, int offset,
>> + const void *buf, int size) {
>> + struct hifive_otp_platdata *plat = dev_get_platdata(dev);
>> + struct hifive_otp_regs *regs = (struct hifive_otp_regs
>> +*)plat->regs;
>> +
>> + int fuseidx = offset / BYTES_PER_FUSE;
>> + int fusecount = size / BYTES_PER_FUSE;
>> + u32 *write_buf = (u32 *)buf;
>> + u32 write_data;
>> + int i, pas, bit;
>> +
>> + // check bounds
>> + if (offset < 0 || size < 0)
>> + return -EINVAL;
>> + if (fuseidx >= NUM_FUSES)
>> + return -EINVAL;
>> + if ((fuseidx + fusecount) > NUM_FUSES)
>> + return -EINVAL;
>
>We have lib functions to do this bound, would you please try to use that.
Will check and update in v4, thanks for the review.
More information about the U-Boot
mailing list