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
