Re: [PATCH] [RFC][POWERPC] Merge 32 and 64 bit pci_process_bridge_OF_ranges() instances

[Vitaly Bordug <vitb@kernel.crashing.org>]

On Wed, 12 Sep 2007 00:57:17 +0200
Arnd Bergmann wrote:

> On Wednesday 12 September 2007, Vitaly Bordug wrote:
> > 
> > We are having 2 different instances of
> > pci_process_bridge_OF_ranges(), which makes describing 64-bit
> > physical addresses in non PPC64 case impossible.
> > 
> > This approach inherits pci space parsing, but has a new way to
> > behave equally good in both 32bit and 64bit environments. This
> > approach uses of_translate_address(), so implies proper ranges <>
> > definition in devicetree, where PCI node has its reg on soc bus,
> > and its ranges effectively belong to LAW addresses.
> > 
> > Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
> > Signed-off-by: Stefan Roese <sr@denx.de>
> 
> The patch looks really good, but it's hard to review when you move the
> code around at the same as you change it.
> 

Well, it's more a rewrite than a move, based on 64-bit implementation.

> Could you perhaps split the patch into two separate changesets, one
> that makes both functions identical in place, and one that merges
> them to live in a common location?
> 
I'm not sure I'm following what you are requesting. What is a benefit of code duplication? I was thinking about,
if it will look good enough, to provide this function at generic level but changing its name a little, while leaving old stuff in place, and encouraging people to use it in favour of 32 or 64-bit-specific approaches. That way we won't kill many boards at once(in case, for example,odd dts with missed ranges for pci subnode).
> > diff --git a/include/asm-powerpc/ppc-pci.h
> > b/include/asm-powerpc/ppc-pci.h index b847aa1..882b8bc 100644
> > --- a/include/asm-powerpc/ppc-pci.h
> > +++ b/include/asm-powerpc/ppc-pci.h
> > @@ -15,6 +15,13 @@
> >  #include <linux/pci.h>
> >  #include <asm/pci-bridge.h>
> >  
> > +struct ranges_pci {
> > +	unsigned int pci_space;
> > +	u64 pci_addr;
> > +	phys_addr_t phys_addr;
> > +	u64 size;
> > +} __attribute__((packed));
> > +
> 
> This structure definition uses unaligned members because of the
> 'packed' attribute. Is that really what you intended?
> 
yes, exactly, because I'm mapping this struct on ranges extracted from the dts instead of juggling with ranges[foo]
offsets.
> 	Arnd <><
Thanks for looking at it!

-- 
Sincerely, Vitaly