[U-Boot] [PATCH v2 07/10] powerpc: mpc8xx: get rid of get_immr()

[Wolfgang Denk]

Dear Christophe,

In message <71a3900b-f61e-e9f8-c12a-5ec5aa1420bc at c-s.fr> you wrote:
> 
> >> -	immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000);
> >> +	immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR;
> > 
> > This is a totally unrelated change, which additionally changes the
> > behaviour of the code - the content of the argument "immr" is now
> > not longer used here.
> 
> In many other places (eg. checkdcache(), do_reset(), etc.), immap is 
> defined as this. Why not doing the same everywhere ?

Firwt, it is an unrelated and undocumented change - you don't
mention it in the commit message.  This _must_ be fixed.  Actually I
think this should be a separate patch, both for documentation and
bisectability.

The other question is if there really is a guarantee that IMMR ist
set to the value of CONFIG_SYS_IMMR.  If that was the case, the
whole code would make little sense, and I don't think it was written
like that just for fun.  So please double-check.

> The lower part of immr is still used later in the function.

Now if that is the case,, then your modification makes even less
sense.  If we need the value anyway, why implement two different
sources of information?  This looks bogus to me.

> Since SPRN_IMMR is defined regardless of the subarch, maybe I should 
> have just remove the #ifdef around get_immr()

Indeed that would make more sense, IMO.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Ninety-Ninety Rule of Project Schedules:
        The first ninety percent of the task takes ninety percent of
the time, and the last ten percent takes the other ninety percent.