[U-Boot] [PATCH V2 12/12] imx: mx7dsabresd: add board revision check

[Peng Fan]

Hi Stefano,

> -----Original Message-----
> From: Stefano Babic [mailto:sbabic at denx.de]
> Sent: Thursday, May 11, 2017 7:33 PM
> To: Peng Fan <peng.fan at nxp.com>; Fabio Estevam <festevam at gmail.com>
> Cc: Stefano Babic <sbabic at denx.de>; U-Boot-Denx <u-boot at lists.denx.de>
> Subject: Re: [U-Boot] [PATCH V2 12/12] imx: mx7dsabresd: add board revision
> check
> 
> Hi Peng,
> 
> On 18/04/2017 02:54, Peng Fan wrote:
> > Hi Fabio,
> >
> >> -----Original Message-----
> >> From: Fabio Estevam [mailto:festevam at gmail.com]
> >> Sent: Monday, April 17, 2017 11:00 PM
> >> To: Peng Fan <peng.fan at nxp.com>
> >> Cc: Stefano Babic <sbabic at denx.de>; U-Boot-Denx
> >> <u-boot at lists.denx.de>
> >> Subject: Re: [U-Boot] [PATCH V2 12/12] imx: mx7dsabresd: add board
> >> revision check
> >>
> >> On Thu, Apr 13, 2017 at 3:10 AM, Peng Fan <peng.fan at nxp.com> wrote:
> >>
> >>> +#define BOARD_REV_C  0x300
> >>> +#define BOARD_REV_B  0x200
> >>> +#define BOARD_REV_A  0x100
> >>> +
> >>> +static int mx7sabre_rev(void)
> >>> +{
> >>> +       /*
> >>> +        * Get Board ID information from OCOTP_GP1[15:8]
> >>> +        * i.MX7D SDB RevA: 0x41
> >>> +        * i.MX7D SDB RevB: 0x42
> >>
> >> Isn't this versioning scheme shared with other NXP boards? If so, it
> >> would be better to put this in common code.
> >
> > I prefer to keep the code here. There are board revision fuse for the
> > boards from NXP, but this is not always true, I think.
> 
> Anyway, there is "quite" same code for mx6 sabre:
> 
> static int mx6sabre_rev(void)
> {
>         /*
>          * Get Board ID information from OCOTP_GP1[15:8]
>          * i.MX6Q ARD RevA: 0x01
>          * i.MX6Q ARD RevB: 0x02
>          */
>         struct ocotp_regs *ocotp = (struct ocotp_regs *)OCOTP_BASE_ADDR;
>         struct fuse_bank *bank = &ocotp->bank[4];
>         struct fuse_bank4_regs *fuse =
>                         (struct fuse_bank4_regs *)bank->fuse_regs;
>         int reg = readl(&fuse->gp1);
>         int ret;
> 
>         switch (reg >> 8 & 0x0F) {
>         case 0x02:
>                 ret = BOARD_REV_B;
>                 break;
>         case 0x01:
>         default:
> 
> And the version number is simple an integer and we do not need to add
> defines - if we simply returns the read value (1,2,3,..), the code works even
> with future versions. Are you sure that this is not at least "sabre common code
> " ?

Could I use a follow up patch to move the revision check to a common place?
I would not send a whole V3 patch set if no more comments.

Thanks,
Peng.

> 
> Regards,
> Stefano
> 
> --
> ============================================================
> =========
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
> ============================================================
> =========