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 <[EMAIL PROTECTED]>
> > Signed-off-by: Stefan Roese <[EMAIL PROTECTED]>
>
> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
