Re: [PATCH 1/2] Add MPC52xx Interrupt controller support for ARCH=powerpc

[Sylvain Munaut <tnt@246tNt.com>]

Kumar Gala wrote:
> On Oct 29, 2006, at 5:10 PM, Nicolas DET wrote:
>
>   
>> This patch add MPC52xx Interrupt controller for ARCH=powerpc.
>>
>> It includes the main code in arch/powerpc/sysdev/ ad well as an  
>> header file in
>> include/asm-powerpc.
>>
>> Signed-off-by: Nicolas DET <nd@bplan-gmbh.de>
>>     
>
> Can you see if you can figure out how to inline patches with your  
> mailer, its really difficult to comment on issues w/an attachment.
>   
.There are also scripts to post patches ... So you're sure it's not mangled
or anything.

> +/* MBAR position */
> +#define MPC52xx_MBAR		0xf0000000	/* Phys address */
> +#define MPC52xx_MBAR_VIRT	0xf0000000	/* Virt address */
> +#define MPC52xx_MBAR_SIZE	0x00010000
> +
> +#define MPC52xx_PA(x)		((phys_addr_t)(MPC52xx_MBAR + (x)))
> +#define MPC52xx_VA(x)		((void __iomem *)(MPC52xx_MBAR_VIRT + (x)))
>
> This should be handled dynamically (pulled from the device tree), I  
> doubt MBAR will be at the same location for all boards.
>   
Agreed.

A little explanation on why they were used before :

the _VA was used for very early access (mostly boot debug) before
anything is setup.
the _PA was used for addresses used at several places. Like, a lot of
driver / code
needs to access XLB and there was not much way to distribute that
address around.

Now with the device tree that should be doable.
Having a nice function/helper
 void *find_and_map_my_good_old_register_set(char *)
that would find and ioremap it for you would be nice ;) like
 struct mpc52xx_xlb __iomem *xlb_regs =
find_and_map_my_good_old_register_set("xlb");


> * lets drop all the other struct defn in mpc52xx.h.  This is a hold  
> over from arch/ppc and we really should only put defn that we  
> actually need closer to the code that uses them (ie, drivers, etc.)
>   
Most of the struct that were in mpc52xx.h are the ones used in more than
one driver.
(or don't really belong to a driver)
That's why they've been left there and not in the driver header, so I
would leave those
there.

(e.g. the IDE struct is not there, neither is the FEC. But sdma is used
at several place
for example, so is rtc, xlb and cdm, ...)


Personnal note to Nicolas : It may seem a long process to just post a
patch but
since it imports a new platform to the arch/powerpc, it kinda requires
to make all
the due cleanups ;)



Sylvain