RE: [PATCH v2 2/3] soc: renesas: identify RZ/A2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thanks for your review!

On Thursday, July 26, 2018, Geert Uytterhoeven wrote:
> >  struct renesas_family {
> >         const char name[16];
> > -       u32 reg;                        /* CCCR or PRR, if not in DT */
> > +       u32 reg;                        /* CCCR, PRR or BSID, if not in
> DT */
> > +       u8 type;
> 
> The only reason we have the reg field in this struct is because we added
> SoC identification for existing SoCs, which didn't have PRR in DT from the
> start.

Oh, that's why this was like that. I was wondering why the duplication.


> As RZ/A2 is a new SoC family, we can assume there will always be a device
> node for the BSID.  Hence I think any code to support RZ/A2 without BSID
> in
> DT can be dropped (incl. the type field and enums, see below).

OK. I'll get rid of them.
> >         if (chipid) {
> >                 product = readl(chipid);
> >                 iounmap(chipid);
> > -               /* R-Car M3-W ES1.1 incorrectly identifies as ES2.0 */
> > -               if ((product & 0x7fff) == 0x5210)
> > -                       product ^= 0x11;
> > -               if (soc->id && ((product >> 8) & 0xff) != soc->id) {
> > -                       pr_warn("SoC mismatch (product = 0x%x)\n",
> product);
> > -                       return -ENODEV;
> > +
> > +               if (reg_type == TYPE_PRR) {
> > +                       /* R-Car M3-W ES1.1 incorrectly identifies as
> ES2.0 */
> > +                       if ((product & 0x7fff) == 0x5210)
> > +                               product ^= 0x11;
> > +                       if (soc->id && ((product >> 8) & 0xff) != soc-
> >id) {
> > +                               pr_warn("SoC mismatch (product =
> 0x%x)\n",
> > +                                       product);
> > +                               return -ENODEV;
> > +                       }
> > +               } else if (reg_type == TYPE_BSID) {
> > +                       if (soc->id && ((product >> 16) & 0xff) != soc-
> >id) {
> > +                               pr_warn("SoC mismatch (product =
> 0x%x)\n",
> > +                                       product);
> > +                               return -ENODEV;
> > +                       }
> >                 }
> >         }
> 
> The complexity in the code above lies in the handling of both DT and
> non-DT based product registers, and the quirk for M3-W ES1.1.
> As RZ/A2 will always have it in DT, I think it make sense to simplify it
> as e.g.
> 
>         np = of_find_compatible_node(NULL, NULL, "renesas,bsid");
>         if (np) {
>                 /* New BSID handling */
>                 ....
>         } else {
>                 /* Old CCCR/PRR handling for DT and non-DT */
>                 ...
>         }
> 
> Perhaps even using "goto done" instead of the "else", so you don't have to
> increase indentation of the old code?

Thank you! I like the goto idea. Makes things easier to read in my opinion.



> > @@ -302,7 +338,7 @@ static int __init renesas_soc_init(void)
> >         soc_dev_attr->family = kstrdup_const(family->name, GFP_KERNEL);
> >         soc_dev_attr->soc_id = kstrdup_const(strchr(match->compatible,
> ',') + 1,
> >                                              GFP_KERNEL);
> > -       if (chipid)
> > +       if (chipid && (reg_type == TYPE_PRR))
> 
> if you do
> 
>         unsigned int eshi = 0, eslo;
>         ...
>         if (/* CCCR/PRR */) {
>                  eshi = ((product >> 4) & 0x0f) + 1;
>                  eslo = product & 0xf;
>         }
> 
> then you can replace the check by
> 
>         if (eshi)
> 

OK. I get what you mean.


> BTW, the top four bits of BSID seem to indicate the chip version.
> Perhaps that corresponds to the ES version on R-Car?

Not sure.The thing is that we haven't had a revision yet for RZ/A,
so it hasn't been used.
They made the chips and whatever bugs were in there stayed.

The closest thing to a revision was RZ/A1L and RZ/A1LU ('U' for updated).
But, since that added new IP like the HW JPEG, it became a different
part number and a new BSID number:
  RZ/A1L:  081A6447
  RZ/A1LU: 082F4447

Chris





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux