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

[christophe leroy]

Le 04/03/2018 à 15:51, Wolfgang Denk a écrit :
> 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.

I agree, my mistake.

> 
> 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.

In start.S:

	.globl	_start
_start:
	lis	r3, CONFIG_SYS_IMMR at h		/* position IMMR */
	mtspr	638, r3


In many fonctions, you have (in the same file cpu.c):

int checkicache(void)
{
	immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR;

int checkdcache(void)
{
	immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR;


void upmconfig(uint upm, uint *table, uint size)
{
	uint i;
	uint addr = 0;
	immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR;

int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
{
	ulong msr, addr;

	immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR;

In other files too (eg: 5 times in immap.c, 7 times in interrupts.c, 6 
times in mpc8xx_fec.c , etc.)

> 
>> 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.

What makes little sense to me is to define the immap pointer differently 
in three places, allthough it is equivalent.

It is not the same information we need later in the function. What this 
function (and only this one) needs in addition is the lower part of 
SPRN_IMMR while the immap pointer is the higher part of SPRN_IMMR (that 
we set earlier in start.S)

> 
>> 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.

I'll do that, and eventually make another patch for unifying the way the 
immap pointer is set.

Best regards
Christophe

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus