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

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

On Wednesday 12 September 2007, Vitaly Bordug wrote:

quoted

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

quoted

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