Re: [U-Boot] [PATCH v5 7/8] riscv: sifive: fu540: Setup ethaddr env variable using OTP

[Bin Meng]

Hi Anup,

On Fri, Jun 21, 2019 at 8:49 PM Anup Patel <a...@brainfault.org> wrote:
>
> On Fri, Jun 21, 2019 at 12:48 PM Bin Meng <bmeng...@gmail.com> wrote:
> >
> > Hi Anup,
> >
> > On Thu, Jun 20, 2019 at 2:49 PM Anup Patel <anup.pa...@wdc.com> wrote:
> > >
> > > This patch extends SiFive FU540 board support to setup ethaddr
> > > env variable based on board serialnum read from OTP.
> > >
> > > Signed-off-by: Anup Patel <anup.pa...@wdc.com>
> > > ---
> > >  board/sifive/fu540/fu540.c     | 118 +++++++++++++++++++++++++++++++++
> > >  configs/sifive_fu540_defconfig |   1 +
> > >  2 files changed, 119 insertions(+)
> > >
> > > diff --git a/board/sifive/fu540/fu540.c b/board/sifive/fu540/fu540.c
> > > index 5adc4a3d4a..81f2025402 100644
> > > --- a/board/sifive/fu540/fu540.c
> > > +++ b/board/sifive/fu540/fu540.c
> > > @@ -8,6 +8,124 @@
> > >
> > >  #include <common.h>
> > >  #include <dm.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/io.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);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static u32 fu540_read_serialnum(void)
> > > +{
> > > +       int ret;
> > > +       u32 serial[2] = {0};
> > > +
> > > +       for (int i = 0xfe * 4; i > 0; i -= 8) {
> > > +               ret = fu540_otp_read(i, serial, sizeof(serial));
> > > +               if (ret) {
> > > +                       printf("%s: error reading from OTP\n", __func__);
> > > +                       break;
> > > +               }
> > > +               if (serial[0] == ~serial[1])
> > > +                       return serial[0];
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void fu540_setup_macaddr(u32 serialnum)
> > > +{
> > > +       /* Default MAC address */
> > > +       unsigned char mac[6] = { 0x70, 0xb3, 0xd5, 0x92, 0xf0, 0x00 };
> >
> > Why are the last 3 bytes of the default MAC address not zero? My
> > understanding is that the first 3 numbers indicate the vendor and
> > vendor is free to use the last 3 bytes for different devices.
>
> Your understanding about MAC address is correct.
>
> I don't know why last three bytes are ORed and not assigned. The
> logic is taken from FSBL.
>
> I did not want users to see different MAC address after this patch
> so I have kept the logic same.
>

OK, thanks for the clarification. Please put some comments here.

> >
> > > +
> > > +       /* OR the serial into the MAC -- see SiFive FSBL */
> > > +       mac[5] |= (serialnum >>  0) & 0xff;
> > > +       mac[4] |= (serialnum >>  8) & 0xff;
> > > +       mac[3] |= (serialnum >> 16) & 0xff;
> > > +
> > > +       /* Skip if environment variable already set */
> > > +       if (env_get("ethaddr"))
> > > +               return;
> >
> > I think we should make the function clear by:
> > * ether move this logic to out of this function, and do such check in
> > misc_init_r()
> >
> > OR
> > * unconditionally overwrite the "ethaddr" by removing this check
> >
> > I am OK with either way.
>
> Sure, I will move the env_get() check to misc_init_r().

Regards,
Bin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot