On Wed, Sep 12, 2007 at 08:07:06PM +0400, Vitaly Bordug wrote:
> On Wed, 12 Sep 2007 10:13:50 +0200
> Arnd Bergmann wrote:
>
> > On Wednesday 12 September 2007, Vitaly Bordug wrote:
> >
> > >
> > > Well, it's more a rewrite than a move, based on 64-bit
> > > implementation.
> >
> > ok.
> >
> > > > 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).
> >
> > I wasn't suggesting to leave the duplicated code in, but rather to
> > make the review easier by first modifying the code in place.
> >
> > If you're taking the 64 bit code as a base, you can for instance make
> > the first patch leave pci_32 alone, and modify the 64 bit
> > pci_process_bridge_OF_ranges to look exactly like the merged version.
> > That allows us to see what changed in the 64 bit case.
> >
> > The second patch would then move the functions over, but leave the
> > code identical to the result of the first patch.
>
> ok, makes sense, will do it that way.
>
> > > > > 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.
> >
> > I see. It does however look wrong to me, because you are using a
> > hardcoded phys_addr_t type. This breaks when phys_addr has a
> > different size from what you expect, e.g. when booting a pure 32 bit
> > kernel on a machine that has a 64 bit physical address space.
> >
> I wondered around with "32 bit phys" and "64 bit phys" struct
> definitions first, but, well, it does not look good. In fact it
> already verified with alike case (on 4xx), and I thought it would be
> fair tradeoff to have 64 bit ranges definition.
>
> otoh, there might be cases when phys_addr_t is u64 and pci stuff
> resides on some 32-bit SoC bus. I will try to address that next
> iteration.
Yes, I was going to point out this sort of case. I don't think
including the parent-address in the structure is going to work.
Oh, also, since this structure is only used in this function, it
should go in the .c file, not a .h file.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
