use sparc64 version of of_device.c
RFC PATCH 1/1 use sparc64 version of of_device.c
This patch creates a combined kernel/of_device.c by starting with the sparc64 version and adding #ifdefs and sparc32 code.
The only sparc64 difference is a %llx printk change.
The sparc32 changes are:
change int rlen to unsigned int rlen in build_one_resource keep the sparc64 op->resources[] array size check
I know #ifdefs are ugly but the files were so similar. Is this acceptable?
Boot tested on sparc32.
Compile tested on sparc64.
arch/sparc/kernel/Makefile | 2 +-
arch/sparc/kernel/{of_device_64.c => of_device.c} | 133 ++++-
arch/sparc/kernel/of_device_32.c | 627 ---------------------
3 files changed, 127 insertions(+), 635 deletions(-)
rename arch/sparc/kernel/{of_device_64.c => of_device.c} (87%)
delete mode 100644 arch/sparc/kernel/of_device_32.carch/sparc/kernel/Makefile +1 -1
@@ -37,7 +37,7 @@ obj-y += una_asm_$(BITS).o obj-$(CONFIG_SPARC32) += muldiv.o obj-y += prom_common.o obj-y += prom_$(BITS).o -obj-y += of_device_$(BITS).o +obj-y += of_device.o obj-$(CONFIG_SPARC64) += prom_irqtrans.o obj-$(CONFIG_SPARC64) += reboot.o
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +126 -7
283 lines not on this page · raw message
arch/sparc/kernel/of_device_32.c deleted +0 -627
627 lines not on this page · raw message
Quoted 11 lines · Robert Reif
This patch creates a combined
kernel/of_device.cby starting with the sparc64 version and adding #ifdefs and sparc32 code.The only sparc64 difference is a %llx printk change.
The sparc32 changes are:
change int rlen to unsigned int rlen inbuild_one_resourcekeep the sparc64 op->resources[] array size checkI know #ifdefs are ugly but the files were so similar. Is this acceptable?
Hi Robert.
Sorry to be negative here. But this hurt readability too much. I will try to give some specific comments below.
That said I really like to see these two files merged. When merged the iffedery really show the minimal differences that is actually present.
Consider doing this as two steps.
First steps prepare of_device_64.c for the unification - which in some cases may add otherwise pointless changes (such as moving a function or a variable definition.
And the second step where you do the unification.
Sam
arch/sparc/kernel/of_device.c void of_iounmap(struct resource *res, void __iomem *base, unsigned long size) · line 38
Quoted 9 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-0+#ifdef CONFIG_SPARC64 void __iomem *of_ioremap (struct resource *res,unsigned long offset,unsigned long size,char *name) {unsigned long ret = res->start + offset;@@ -34,6 +35,7 @@ void of_iounmap(struct resource *res, void __iomem *base, unsigned long size) release_region((unsigned long) base, size); }EXPORT_SYMBOL (of_iounmap); +#endif
For sparc32 we have these in ioport.c.
So the better alternative would maybe have been to move them to a ioport_64 file?
That may seem counter productive but it uis best to keep similar things in similar named files.
Quoted 20 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +4
-1static int node_match (struct device *dev,void *data) {@@ -192,12 +194,15 @@ static unsigned long of_bus_default_get_flags(const u32 *addr, unsigned long flastatic int of_bus_pci_match (struct device_node *np) { +#ifdef CONFIG_SPARC32 +if (!strcmp (np->type,"pci" ) || !strcmp (np->type,"pciex" )) { +#else if (!strcmp (np->name,"pci" )) {const char *model =of_get_property (np,"model" ,NULL );if (model && !strcmp (model,"SUNW,simba" ))return 0 ; - +#endif /* Do not do PCI specific frobbing if the * PCI bridge lacks a ranges property. We * want to pass it through up to the next
arch/sparc/kernel/of_device.c static int of_bus_pci_match(struct device_node *np) · line 219
Quoted 2 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +0
-0@@ -213,6 +218,7 @@ static int of_bus_pci_match(struct device_node *np)return 0 ; }
Ifdeffing only the 'if' part makes the code almost unreadable. In this case having two node_match functions would be more readable. And then you could reuse the ifdef block you have below.
Quoted 11 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +4
-0static void of_bus_pci_count_cells (struct device_node *np,int *addrc,int *sizec)@@ -314,6 +321,22 @@ static void of_bus_sbus_count_cells(struct device_node *child, *sizec =1 ; } +#ifdef CONFIG_SPARC32 +static int of_bus_sbus_map (u32 *addr,const u32 *range,int na,int ns,int pna) +{ +return of_bus_default_map (addr, range, na, ns, pna);
arch/sparc/kernel/of_device.c static void of_bus_sbus_count_cells(struct device_node *child, · line 337
Quoted 10 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +10
-0+} + +static unsigned long of_bus_sbus_get_flags (const u32 *addr,unsigned long flags) +{ +return IORESOURCE_MEM ; +} +#else +#define of_bus_sbus_map of_bus_default_map +#define of_bus_sbus_get_flags of_bus_default_get_flags +#endif
This renaming could be dropped?of_bus_sbus_map is equal to of_bus_default_map.
And of_bus_sbus_get_flags is almost equal to of_bus_default_get_flags.
Maybe we can use the latter for sparc32 too (I did not check the use of the flags parameter).
arch/sparc/kernel/of_device.c static int of_bus_fhc_match(struct device_node *np) · line 356
Quoted 10 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +3
-0+ +#ifdef CONFIG_SPARC64 /* * FHC/Central bus specific translator. * @@ -330,6 +353,7 @@ static int of_bus_fhc_match(struct device_node *np) }#define of_bus_fhc_count_cells of_bus_sbus_count_cells +#endif
The rename above is not needed as only SPARC64 code reference of_bus_fhc_count_cells
Quoted 32 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +5
-2/* * Array of bus specific translators @@ -345,6 +369,7 @@ static struct of_bus of_busses[] = { .map = of_bus_pci_map, .get_flags = of_bus_pci_get_flags, }, +#ifdef CONFIG_SPARC64 /* SIMBA */ { .name = "simba",@@ -354,15 +379,17 @@ static struct of_bus of_busses[] = { .map = of_bus_simba_map, .get_flags = of_bus_pci_get_flags, }, +#endif /* SBUS */ { .name = "sbus", .addr_prop_name = "reg", .match = of_bus_sbus_match, .count_cells = of_bus_sbus_count_cells, - .map = of_bus_default_map, - .get_flags = of_bus_default_get_flags, + .map = of_bus_sbus_map, + .get_flags = of_bus_sbus_get_flags, }, +#ifdef CONFIG_SPARC64 /* FHC */ { .name = "fhc",
arch/sparc/kernel/of_device.c static struct of_bus of_busses[] = { · line 402
Quoted 4 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +1
-0@@ -372,6 +399,7 @@ static struct of_bus of_busses[] = { .map = of_bus_default_map, .get_flags = of_bus_default_get_flags, }, +#endif
If the order is not important then combine the two SPARC64 specific entries.
Quoted 20 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-1/* Default */ { .name = "default",@@ -425,13 +453,14 @@ static int __init build_one_resource(struct device_node *parent,return 0 ; } +#ifdef CONFIG_SPARC64 /* When we miss an I/O space match on PCI, just pass it up * to the next PCI bridge and/or controller. */ if (!strcmp (bus->name,"pci" ) && (addr[0 ] &0x03000000 ) =0x01000000 )return 0 ; - +#endif return 1 ; }
arch/sparc/kernel/of_device.c static int __init use_1to1_mapping(struct device_node *pp) · line 488
Quoted 4 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +1
-0@@ -456,13 +485,14 @@ static int __init use_1to1_mapping(struct device_node *pp) !strcmp(pp->name, "lebuffer"))return 0 ; +#ifdef _CONFIG_SPARC64
^
arch/sparc/kernel/of_device.c static int __init use_1to1_mapping(struct device_node *pp) · line 495
Quoted 8 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +1
-1/* Similarly for all PCI bridges, if we get this far * it lacks a ranges property, and this will include * cases like Simba. */ if (!strcmp (pp->name,"pci" ))return 0 ; - +#endif
Here an underscore slipped in.
Quoted 16 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +4
-2return 1 ; }@@ -554,22 +584,29 @@ static void __init build_device_resources(struct of_device *op,memset (r,0 ,sizeof (*r));if (of_resource_verbose) -printk ("%s reg[%d] -> %lx\n" , +printk ("%s reg[%d] -> %llx\n" , op->node->full_name, index, - result); + (unsigned long long )result);if (result !=OF_BAD_ADDR ) { +#ifdef CONFIG_SPARC32 + r->start = result &0xffffffff ;
arch/sparc/kernel/of_device.c static void __init build_device_resources(struct of_device *op, · line 603
Quoted 10 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +4
-0+ r->end = result + size -1 ; + r->flags = flags | ((result >>32ULL ) &0xffUL ); +#else if (tlb_type = hypervisor) result &=0x0fffffffffffffffUL ; r->start = result; r->end = result + size -1 ; r->flags = flags; +#endif
Rearrange this so it is obvious which two lines are not equal. The order os the assignmnet I expect does not matter.
Quoted 107 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +78
-0} r->name = op->node->name; } } +#ifdef CONFIG_SPARC64 static struct device_node *__init apply_interrupt_map(struct device_node *dp, struct device_node *pp, const u32 *imap, int imlen, const u32 *imask,@@ -785,12 +822,17 @@ out: return irq; } +#endif static struct of_device *__init scan_one_device (struct device_node *dp,struct device *parent) {struct of_device *op =kzalloc (sizeof (*op),GFP_KERNEL ); +#ifdef CONFIG_SPARC32 +const struct linux_prom_irqs *intr; +#else const unsigned int *irq; +#endif struct dev_archdata *sd;int len, i;@@ -809,6 +851,79 @@ static struct of_device * __init scan_one_device(struct device_node *dp,if (op->portid =-1 ) op->portid =of_getintprop_default (dp,"portid" ,-1 ); +#ifdef CONFIG_SPARC32 + intr =of_get_property (dp,"intr" , &len); +if (intr) { + op->num_irqs = len /sizeof (struct linux_prom_irqs ); +for (i =0 ; i < op->num_irqs; i++) + op->irqs[i] = intr[i].pri; + }else { + const unsigned int *irq > + of_get_property(dp, "interrupts", &len); + +if (irq) { + op->num_irqs = len /sizeof (unsigned int ); +for (i =0 ; i < op->num_irqs; i++) + op->irqs[i] = irq[i]; + }else { + op->num_irqs =0 ; + } + } +if (sparc_cpu_model = sun4d) { +static int pil_to_sbus[] = { +0 ,0 ,1 ,2 ,0 ,3 ,0 ,4 ,0 ,5 ,0 ,6 ,0 ,7 ,0 ,0 , + }; +struct device_node *io_unit, *sbi = dp->parent; +const struct linux_prom_registers *regs; +int board, slot; + +while (sbi) { +if (!strcmp (sbi->name,"sbi" )) +break ; + + sbi = sbi->parent; + } +if (!sbi) + goto build_resources; + + regs =of_get_property (dp,"reg" ,NULL ); +if (!regs) + goto build_resources; + + slot = regs->which_io; + +/* If SBI's parent is not io-unit or the io-unit lacks +* a "board#" property, something is very wrong. +*/ +if (!sbi->parent ||strcmp (sbi->parent->name,"io-unit" )) { +printk ("%s: Error, parent is not io-unit.\n" , + sbi->full_name); + goto build_resources; + } + io_unit = sbi->parent; + board =of_getintprop_default (io_unit,"board#" ,-1 ); +if (board =-1 ) { +printk ("%s: Error, lacks board# property.\n" , + io_unit->full_name); + goto build_resources; + } + +for (i =0 ; i < op->num_irqs; i++) { +int this_irq = op->irqs[i]; +int sbusl = pil_to_sbus[this_irq]; + +if (sbusl) + this_irq = (((board +1 ) <<5 ) + + (sbusl <<2 ) + + slot); + + op->irqs[i] = this_irq; + } + } + +build_resources: +build_device_resources (op, parent); +#else irq =of_get_property (dp,"interrupts" , &len);if (irq) {memcpy (op->irqs, irq, len);
arch/sparc/kernel/of_device.c static struct of_device * __init scan_one_device(struct device_node *dp, · line 947
Quoted 7 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-0@@ -826,8 +941,10 @@ static struct of_device * __init scan_one_device(struct device_node *dp, }build_device_resources (op, parent); +for (i =0 ; i < op->num_irqs; i++) op->irqs[i] =build_one_device_irq (op, parent, op->irqs[i]); +#endif
It would be better to move the relevant pices to two separate functions here.
arch/sparc/kernel/of_device.c static int __init of_debug(char *str) · line 1015
Quoted 13 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-0op->dev.parent = parent; op->dev.bus = &of_platform_bus_type;@@ -890,8 +1007,10 @@ static int __init of_debug(char *str)get_option (&str, &val);if (val &1 ) of_resource_verbose =1 ; +#ifdef CONFIG_SPARC64 if (val &2 ) of_irq_verbose =1 ; +#endif return 1 ; }
Define this variable for SPARC32 and get rid of the above ifdef.
Sam
David Miller
maintainerFrom: Sam Ravnborg <[email protected]>
Date: Mon, 15 Dec 2008 19:52:41 +0100
Quoted 7 lines
Sorry to be negative here. But this hurt readability too much. I will try to give some specific comments below.
That said I really like to see these two files merged. When merged the iffedery really show the minimal differences that is actually present.
Quoted 6 lines
Consider doing this as two steps.
First steps prepareof_device_64.cfor the unification - which in some cases may add otherwise pointless changes (such as moving a function or a variable definition.And the second step where you do the unification.
That's my feeling as well, it needs treatment similar to how prom.c was handled.
However in this case I think more unification is possible.
All of those OF device tree matching things can be merged and compiled in both cases. For example, keeping the "pcie" string test in there in generic not-ifdeffed code on sparc32 is fine because there will never be PCI-E busses on sparc32.
Yes, it will eat some space on sparc32 but the alternative is code duplication or messy ifdefs.
The truly different aspects which need platform specific code is the resource layout and ioremap handling. On sparc32 we store some of the I/O physical address in the resource flags so that the resource start and end can stay 32-bit (sun4m and sun4d have 36-bit physical addresses).
I may take a stab at this work, but feel free to beat me to it :-)
Hi,
My comments are probably irrelevant as I assume the object of this work is to merge the files without overly changing the binaries, however, for the record and to repeat what David said:
Quoted 8 lines · Robert Reif
index 0f616ae..625d605 100644
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +0
-0@@ -192,12 +194,15 @@ static unsigned long of_bus_default_get_flags(constu32 *addr, unsigned long fla
static int
of_bus_pci_match(struct device_node *np){
arch/sparc/kernel/of_device.c static unsigned long of_bus_default_get_flags(const u32 *addr, unsigned long fla · line 205
Quoted 10 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +4
-1+#ifdef CONFIG_SPARC32 +if (!strcmp (np->type,"pci" ) || !strcmp (np->type,"pciex" )) { +#else if (!strcmp (np->name,"pci" )) {const char *model =of_get_property (np,"model" ,NULL );if (model && !strcmp (model,"SUNW,simba" ))return 0 ; - +#endif
Is there really any functional difference here? - I.e. is sparc32 ever going to have a "simba" device and is sparc64 ever going to have a pciex type device node?
arch/sparc/kernel/of_device.c static int of_bus_simba_map(u32 *addr, const u32 *range, · line 245
Quoted 13 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-0@@ -213,6 +218,7 @@ static int of_bus_pci_match(struct device_node *np)return 0 ; } +#ifdef CONFIG_SPARC64 static int of_bus_simba_match (struct device_node *np) {const char *model =of_get_property (np,"model" ,NULL );@@ -236,6 +242,7 @@ static int of_bus_simba_map(u32 *addr, const u32 *range, {return 0 ; } +#endif
Is it harmful for sparc32 to have this function defined?
Quoted 11 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +5
-0@@ -314,6 +321,22 @@ static void of_bus_sbus_count_cells(struct device_node *child, *sizec =1 ; } +#ifdef CONFIG_SPARC32 +static int of_bus_sbus_map (u32 *addr,const u32 *range,int na,int ns,int pna) +{ +return of_bus_default_map (addr, range, na, ns, pna); +}
arch/sparc/kernel/of_device.c static void of_bus_sbus_count_cells(struct device_node · line 340
Quoted 10 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +9
-0+ +static unsigned long of_bus_sbus_get_flags (const u32 *addr,unsigned long flags) +{ +return IORESOURCE_MEM ; +} +#else +#define of_bus_sbus_map of_bus_default_map +#define of_bus_sbus_get_flags of_bus_default_get_flags +#endif
Aren't the two branches of this ifdef essentially identical for of_bus_sbus_map()?
arch/sparc/kernel/of_device.c static int of_bus_fhc_match(struct device_node *np) · line 356
Quoted 9 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-0+#ifdef CONFIG_SPARC64 /* * FHC/Central bus specific translator. * @@ -330,6 +353,7 @@ static int of_bus_fhc_match(struct device_node *np) }#define of_bus_fhc_count_cells of_bus_sbus_count_cells +#endif
Again, is it harmful for sparc32 to have this defined?
arch/sparc/kernel/of_device.c static struct of_bus of_busses[] = { · line 382
Quoted 13 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-0@@ -345,6 +369,7 @@ static struct of_bus of_busses[] = { .map = of_bus_pci_map, .get_flags = of_bus_pci_get_flags, }, +#ifdef CONFIG_SPARC64 /* SIMBA */ { .name = "simba",@@ -354,15 +379,17 @@ static struct of_bus of_busses[] = { .map = of_bus_simba_map, .get_flags = of_bus_pci_get_flags, }, +#endif
Is there any harm in having this code included in sparc32?
arch/sparc/kernel/of_device.c static struct of_bus of_busses[] = { · line 402
Quoted 9 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-0+#ifdef CONFIG_SPARC64 /* FHC */ { .name = "fhc",@@ -372,6 +399,7 @@ static struct of_bus of_busses[] = { .map = of_bus_default_map, .get_flags = of_bus_default_get_flags, }, +#endif
Ditto.
arch/sparc/kernel/of_device.c out: · line 825
Quoted 9 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +2
-0+#ifdef CONFIG_SPARC64 static struct device_node *__init apply_interrupt_map(struct device_node *dp, struct device_node *pp, const u32 *imap, int imlen, const u32 *imask,@@ -785,12 +822,17 @@ out: return irq; } +#endif
Is it harmful to have this defined for sparc32?
Thanks,
Signature · 3 lines
Julian Calaby Email: [email protected]
Quoted 9 lines · Robert Reif
This patch creates a combined
kernel/of_device.cby starting with the sparc64 version and adding #ifdefs and sparc32 code.The only sparc64 difference is a %llx printk change.
The sparc32 changes are:
change int rlen to unsigned int rlen inbuild_one_resourcekeep the sparc64 op->resources[] array size check
Quoted 5 lines
I know #ifdefs are ugly but the files were so similar. Is this acceptable?
Boot tested on sparc32.
Compile tested on sparc64.
Robert - do you still look after this one?
Note: It is a positive thing that three individuals actually
reviewed your work!
Sam
Quoted 6 lines · Sam Ravnborg
Quoted 6 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +1
-0+#ifdef CONFIG_SPARC64 void __iomem *of_ioremap (struct resource *res,unsigned long offset,unsigned long size,char *name) {unsigned long ret = res->start + offset;@@ -34,6 +35,7 @@ void of_iounmap(struct resource *res, void __iomem *base, unsigned long size)release_region ((unsigned long ) base, size);
Quoted 9 lines
arch/sparc/kernel/of_device.c void of_iounmap(struct resource *res, void __iomem *base, unsigned long size) · line 39
Quoted 4 lines
arch/sparc/kernel/of_device.c renamed from arch/sparc/kernel/of_device_64.c +1
-0}EXPORT_SYMBOL (of_iounmap); +#endif For sparc32 we have these in
ioport.c.
So the better alternative would maybe have been to move them to aioport_64file?
Wouldn't it be better to use these in sparc32? This would require using 64 bit resources. We could then use generic code. This and switching to bootmem allocations in irq.c would eliminate a good chunk of ioport.c by using generic or shared code.