[PATCH v4 02/14] riscv: sifive: fu540: Use OTP DM driver for serial environment variable

[Pragnesh Patel]

Hi Patrick,

>-----Original Message-----
>From: Patrick DELAUNAY <patrick.delaunay at st.com>
>Sent: 24 February 2020 23:30
>To: Pragnesh Patel <pragnesh.patel at sifive.com>; u-boot at lists.denx.de
>Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>bmeng.cn at gmail.com; Paul Walmsley ( Sifive) <paul.walmsley at sifive.com>;
>jagan at amarulasolutions.com; Troy Benjegerdes ( Sifive)
><troy.benjegerdes at sifive.com>; anup.patel at wdc.com; Sagar Kadam
><sagar.kadam at sifive.com>; Rick Chen <rick at andestech.com>
>Subject: RE: [PATCH v4 02/14] riscv: sifive: fu540: Use OTP DM driver for serial
>environment variable
>
>Hi,
>
>Just a warning as I had the same issue with 8729b1ae2cbd ("misc: Update
>read() and write()  methods to return bytes xfered")
>
>> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Pragnesh
>> Patel
>> Sent: lundi 24 février 2020 09:33
>>
>> Use the OTP DM driver to set the serial environment variable.
>>
>> Signed-off-by: Pragnesh Patel <pragnesh.patel at sifive.com>
>> ---
>>  arch/riscv/dts/fu540-c000-u-boot.dtsi         |  14 +++
>>  .../dts/hifive-unleashed-a00-u-boot.dtsi      |   6 +
>>  board/sifive/fu540/Kconfig                    |   2 +
>>  board/sifive/fu540/fu540.c                    | 113 +++++++-----------
>>  4 files changed, 62 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
>>
>> 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..31fd113c7d
>> --- /dev/null
>> +++ b/arch/riscv/dts/fu540-c000-u-boot.dtsi
>> @@ -0,0 +1,14 @@
>> +// 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>;
>> +			fuse-count = <0x1000>;
>> +		};
>> +	};
>> +};
>> 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/Kconfig b/board/sifive/fu540/Kconfig
>> index
>> 5ca21474de..900197bbb2 100644
>> --- a/board/sifive/fu540/Kconfig
>> +++ b/board/sifive/fu540/Kconfig
>> @@ -48,5 +48,7 @@ config BOARD_SPECIFIC_OPTIONS # dummy
>>  	imply SIFIVE_GPIO
>>  	imply CMD_GPIO
>>  	imply SMP
>> +	imply MISC
>> +	imply SIFIVE_OTP
>>
>>  endif
>> diff --git a/board/sifive/fu540/fu540.c b/board/sifive/fu540/fu540.c
>> index 47a2090251..409471effc 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, &regs->pdstb); /* wake up from stand-by */
>> -	writel(0x01, &regs->ptrim); /* enable repair function */
>> -	writel(0x01, &regs->pce);   /* enable input */
>> -
>> -	/* read all requested fuses */
>> -	for (i = 0; i < fusecount; i++, fuseidx++) {
>> -		writel(fuseidx, &regs->pa);
>> -
>> -		/* cycle clock to read */
>> -		writel(0x01, &regs->pclk);
>> -		mdelay(1);
>> -		writel(0x00, &regs->pclk);
>> -		mdelay(1);
>> -
>> -		/* read the value */
>> -		fusebuf[i] = readl(&regs->pdout);
>> -	}
>> -
>> -	/* shut down */
>> -	writel(0, &regs->pce);
>> -	writel(0, &regs->ptrim);
>> -	writel(0, &regs->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(SIFIVE_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) {
>
>Warning here, misc_read returns now the size of the data (sizeof(serial)),
>since commit 8729b1ae2cbd ("misc: Update read() and write() methods to
>return bytes xfered")
>
>So I think the test need to change to
>
>if (ret < 0)
>
>or
>
>if (ret != sizeof(serial))
>

Yes, you are right. Will update in v5. Thanks for the review.

>> -			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(SIFIVE_OTP)
>> +	struct udevice *dev;
>> +	int ret;
>> +
>> +	// init OTP
>> +	ret = uclass_get_device_by_driver(UCLASS_MISC,
>> +					  DM_GET_DRIVER(sifive_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)
>> --
>> 2.17.1
>
>Patrick