Re: [PATCH 13/14] 85xx: consolidate of_platform_bus_probe calls
On Sat, 23 Jul 2011 01:45:53 +0400 Dmitry Eremin-Solenikov wrote: > I see your point. I just wasn't thinking too much about ot-of-tree trees. > My thought was that if someone updates the kernel, he can also update the dtb. Sometimes there are firmware dependencies that make that difficult. And even if it's just user laziness/forgetfulness, that still translates to extra support requests. > Could you please update the lbc.txt suggesting the compatibility > with simple-bus for lbc? Or you thing that it would be wrong? > > I think we should define compatibility list as "fsl,mpc-localbus", > "fsl,pqX-localbus", "simple-bus", noting that by default new > platforms/boards should only use "simple-bus" internally. Does this > look reasonable for you? I can then try to provide a patch. I'm OK with saying that localbus nodes should have simple-bus in new trees, and defining canonical compatible values (chips with eLBC should be "fsl,-elbc", "fsl,elbc", "simple-bus"). I'm not sure what you mean by "should only use simple-bus internally", especially in the context of the binding. > What do you suggest/prefer? To add .name="localbus" to generic code > or to have board-specific hooks (like one for mpc834xemitx)? Just add localbus to the generic table. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 13/14] 85xx: consolidate of_platform_bus_probe calls
On 7/23/11, Scott Wood wrote:
> On Fri, 22 Jul 2011 23:44:01 +0400
> Dmitry Eremin-Solenikov wrote:
>
>> On 7/19/11, Scott Wood wrote:
>> > On Tue, 19 Jul 2011 12:53:50 +0400
>> > Dmitry Eremin-Solenikov wrote:
>> >
>> >> +static struct of_device_id __initdata mpc85xx_common_ids[] = {
>> >> + { .type = "soc", },
>> >> + { .compatible = "soc", },
>> >> + { .compatible = "simple-bus", },
>> >> + { .compatible = "gianfar", },
>> >> + { .compatible = "fsl,qe", },
>> >> + { .compatible = "fsl,cpm2", },
>> >> + {},
>> >> +};
>> >
>> > Same comment as for 83xx regarding localbus and compatibility with old
>> > device trees.
>>
>> I checked for in-kernel device trees. Unless I miss something, there are
>> no
>> leftovers from this list. (83xx provided no simple-bus property for
>> localbus,
>> so your argument is valid there). If we should care about strange cases,
>> not even blessed by kernel trees, I can add some of the old probes back.
>
> I see simple-bus missing in sbc8560 and ksi8560 -- were these included in
> the consolidation? Plus some of the others may have had simple-bus added
> after their initial version.
Patches for those are included in the patch serie. Kumar has applied them
to next-3.2.
> As for out-of-tree trees (which could include trees dynamically
> generated/augmented by firmware, as well as device trees for custom boards
> that forked off of an old reference board tree), it's still nice to not
> break them as long as they stick to the binding.
I see your point. I just wasn't thinking too much about ot-of-tree trees.
My thought was that if someone updates the kernel, he can also update the dtb.
> While the localbus binding is deficient regarding the compatible property,
> IIRC localbus preceded the introduction of simple-bus, which appears to be
> defined only in ePAPR (not in Linux or on devicetree.org). The ePAPR
> language does not suggest that it's mandatory for all buses that don't need
> special handling -- in fact, the language could be read as suggesting that
> it's only applicable to the "internal I/O bus" on an SoC (whereas this
> is an external bus), though that wasn't the intent behind it.
Could you please update the lbc.txt suggesting the compatibility
with simple-bus for lbc? Or you thing that it would be wrong?
I think we should define compatibility list as "fsl,mpc-localbus",
"fsl,pqX-localbus", "simple-bus", noting that by default new
platforms/boards should only use "simple-bus" internally. Does this
look reasonable for you? I can then try to provide a patch.
> The notion of probing buses isn't really a part of the device tree specs;
> they're more concerned with binding the devices themselves. In theory
> Linux should probably be probing everything that a driver will match,
> regardless of where in the tree it is, except where an ancestor node is
> diasbled, has matched a driver that wants to do things differently, or is
> on a blacklist. Of course, that's somewhat of a philosophical question on
> whether it's better to risk probing someting that shouldn't be, or not
> probing something that should be. The former is often nastier but more
> obvious, the latter is more likely until simple-bus is more widely used,
> and either one results in something not working.
> Leaving the localbus in may help someone, and it shouldn't hurt anything.
What do you suggest/prefer? To add .name="localbus" to generic code
or to have board-specific hooks (like one for mpc834xemitx)?
So,
>
> -Scott
>
>
--
With best wishes
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 13/14] 85xx: consolidate of_platform_bus_probe calls
On Fri, 22 Jul 2011 23:44:01 +0400
Dmitry Eremin-Solenikov wrote:
> On 7/19/11, Scott Wood wrote:
> > On Tue, 19 Jul 2011 12:53:50 +0400
> > Dmitry Eremin-Solenikov wrote:
> >
> >> +static struct of_device_id __initdata mpc85xx_common_ids[] = {
> >> + { .type = "soc", },
> >> + { .compatible = "soc", },
> >> + { .compatible = "simple-bus", },
> >> + { .compatible = "gianfar", },
> >> + { .compatible = "fsl,qe", },
> >> + { .compatible = "fsl,cpm2", },
> >> + {},
> >> +};
> >
> > Same comment as for 83xx regarding localbus and compatibility with old
> > device trees.
>
> I checked for in-kernel device trees. Unless I miss something, there are no
> leftovers from this list. (83xx provided no simple-bus property for localbus,
> so your argument is valid there). If we should care about strange cases,
> not even blessed by kernel trees, I can add some of the old probes back.
I see simple-bus missing in sbc8560 and ksi8560 -- were these included in
the consolidation? Plus some of the others may have had simple-bus added
after their initial version.
As for out-of-tree trees (which could include trees dynamically
generated/augmented by firmware, as well as device trees for custom boards
that forked off of an old reference board tree), it's still nice to not
break them as long as they stick to the binding.
While the localbus binding is deficient regarding the compatible property,
IIRC localbus preceded the introduction of simple-bus, which appears to be
defined only in ePAPR (not in Linux or on devicetree.org). The ePAPR
language does not suggest that it's mandatory for all buses that don't need
special handling -- in fact, the language could be read as suggesting that
it's only applicable to the "internal I/O bus" on an SoC (whereas this
is an external bus), though that wasn't the intent behind it.
The notion of probing buses isn't really a part of the device tree specs;
they're more concerned with binding the devices themselves. In theory
Linux should probably be probing everything that a driver will match,
regardless of where in the tree it is, except where an ancestor node is
diasbled, has matched a driver that wants to do things differently, or is
on a blacklist. Of course, that's somewhat of a philosophical question on
whether it's better to risk probing someting that shouldn't be, or not
probing something that should be. The former is often nastier but more
obvious, the latter is more likely until simple-bus is more widely used,
and either one results in something not working.
Leaving the localbus in may help someone, and it shouldn't hurt anything.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 13/14] 85xx: consolidate of_platform_bus_probe calls
On 7/19/11, Scott Wood wrote:
> On Tue, 19 Jul 2011 12:53:50 +0400
> Dmitry Eremin-Solenikov wrote:
>
>> +static struct of_device_id __initdata mpc85xx_common_ids[] = {
>> +{ .type = "soc", },
>> +{ .compatible = "soc", },
>> +{ .compatible = "simple-bus", },
>> +{ .compatible = "gianfar", },
>> +{ .compatible = "fsl,qe", },
>> +{ .compatible = "fsl,cpm2", },
>> +{},
>> +};
>
> Same comment as for 83xx regarding localbus and compatibility with old
> device trees.
I checked for in-kernel device trees. Unless I miss something, there are no
leftovers from this list. (83xx provided no simple-bus property for localbus,
so your argument is valid there). If we should care about strange cases,
not even blessed by kernel trees, I can add some of the old probes back.
What do you thing?
--
With best wishes
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 13/14] 85xx: consolidate of_platform_bus_probe calls
On Tue, 19 Jul 2011 12:53:50 +0400
Dmitry Eremin-Solenikov wrote:
> +static struct of_device_id __initdata mpc85xx_common_ids[] = {
> + { .type = "soc", },
> + { .compatible = "soc", },
> + { .compatible = "simple-bus", },
> + { .compatible = "gianfar", },
> + { .compatible = "fsl,qe", },
> + { .compatible = "fsl,cpm2", },
> + {},
> +};
Same comment as for 83xx regarding localbus and compatibility with old
device trees.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 13/14] 85xx: consolidate of_platform_bus_probe calls
On Jul 19, 2011, at 3:53 AM, Dmitry Eremin-Solenikov wrote:
> 85xx board files have a lot of duplication in *_publish_devices()/
> *_declare_of_platform_devices() functions. Merge that into a single
> function common to most of the boards.
>
> Signed-off-by: Dmitry Eremin-Solenikov
> ---
> arch/powerpc/platforms/85xx/Makefile |2 +
> arch/powerpc/platforms/85xx/ksi8560.c| 18 +-
> arch/powerpc/platforms/85xx/mpc8536_ds.c | 16 ++---
> arch/powerpc/platforms/85xx/mpc85xx.h|4 ++
> arch/powerpc/platforms/85xx/mpc85xx_ads.c| 20 ++-
> arch/powerpc/platforms/85xx/mpc85xx_cds.c| 16 ++---
> arch/powerpc/platforms/85xx/mpc85xx_common.c | 26 ++
> arch/powerpc/platforms/85xx/mpc85xx_ds.c | 20 +++
> arch/powerpc/platforms/85xx/mpc85xx_mds.c| 46 ++---
> arch/powerpc/platforms/85xx/mpc85xx_rdb.c| 16 ++---
> arch/powerpc/platforms/85xx/p1022_ds.c |7 ++--
> arch/powerpc/platforms/85xx/sbc8548.c| 18 ++
> arch/powerpc/platforms/85xx/sbc8560.c| 20 ++-
> arch/powerpc/platforms/85xx/socrates.c | 13 +--
> arch/powerpc/platforms/85xx/stx_gp3.c| 16 ++---
> arch/powerpc/platforms/85xx/tqm85xx.c| 16 ++---
> arch/powerpc/platforms/85xx/xes_mpc85xx.c| 20 +++
> 17 files changed, 77 insertions(+), 217 deletions(-)
> create mode 100644 arch/powerpc/platforms/85xx/mpc85xx.h
> create mode 100644 arch/powerpc/platforms/85xx/mpc85xx_common.c
In general this looks ok, can you refactor so its earlier in the patch sequence
- k
>
> diff --git a/arch/powerpc/platforms/85xx/Makefile
> b/arch/powerpc/platforms/85xx/Makefile
> index 43b2162..ca4b1b9 100644
> --- a/arch/powerpc/platforms/85xx/Makefile
> +++ b/arch/powerpc/platforms/85xx/Makefile
> @@ -3,6 +3,8 @@
> #
> obj-$(CONFIG_SMP) += smp.o
>
> +obj-y += mpc85xx_common.o
> +
> obj-$(CONFIG_MPC8540_ADS) += mpc85xx_ads.o
> obj-$(CONFIG_MPC8560_ADS) += mpc85xx_ads.o
> obj-$(CONFIG_MPC85xx_CDS) += mpc85xx_cds.o
> diff --git a/arch/powerpc/platforms/85xx/ksi8560.c
> b/arch/powerpc/platforms/85xx/ksi8560.c
> index c46f935..7657e1a 100644
> --- a/arch/powerpc/platforms/85xx/ksi8560.c
> +++ b/arch/powerpc/platforms/85xx/ksi8560.c
> @@ -35,6 +35,7 @@
> #include
> #include
>
> +#include "mpc85xx.h"
>
> #define KSI8560_CPLD_HVR 0x04 /* Hardware Version Register */
> #define KSI8560_CPLD_PVR 0x08 /* PLD Version Register */
> @@ -215,22 +216,7 @@ static void ksi8560_show_cpuinfo(struct seq_file *m)
> seq_printf(m, "PLL setting\t: 0x%x\n", ((phid1 >> 24) & 0x3f));
> }
>
> -static struct of_device_id __initdata of_bus_ids[] = {
> - { .type = "soc", },
> - { .type = "simple-bus", },
> - { .name = "cpm", },
> - { .name = "localbus", },
> - { .compatible = "gianfar", },
> - {},
> -};
> -
> -static int __init declare_of_platform_devices(void)
> -{
> - of_platform_bus_probe(NULL, of_bus_ids, NULL);
> -
> - return 0;
> -}
> -machine_device_initcall(ksi8560, declare_of_platform_devices);
> +machine_device_initcall(ksi8560, mpc85xx_common_publish_devices);
>
> /*
> * Called very early, device-tree isn't unflattened
> diff --git a/arch/powerpc/platforms/85xx/mpc8536_ds.c
> b/arch/powerpc/platforms/85xx/mpc8536_ds.c
> index f79f2f1..9ee6455 100644
> --- a/arch/powerpc/platforms/85xx/mpc8536_ds.c
> +++ b/arch/powerpc/platforms/85xx/mpc8536_ds.c
> @@ -32,6 +32,8 @@
> #include
> #include
>
> +#include "mpc85xx.h"
> +
> void __init mpc8536_ds_pic_init(void)
> {
> struct mpic *mpic;
> @@ -104,19 +106,7 @@ static void __init mpc8536_ds_setup_arch(void)
> printk("MPC8536 DS board from Freescale Semiconductor\n");
> }
>
> -static struct of_device_id __initdata mpc8536_ds_ids[] = {
> - { .type = "soc", },
> - { .compatible = "soc", },
> - { .compatible = "simple-bus", },
> - { .compatible = "gianfar", },
> - {},
> -};
> -
> -static int __init mpc8536_ds_publish_devices(void)
> -{
> - return of_platform_bus_probe(NULL, mpc8536_ds_ids, NULL);
> -}
> -machine_device_initcall(mpc8536_ds, mpc8536_ds_publish_devices);
> +machine_device_initcall(mpc8536_ds, mpc85xx_common_publish_devices);
>
> machine_arch_initcall(mpc8536_ds, swiotlb_setup_bus_notifier);
>
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx.h
> b/arch/powerpc/platforms/85xx/mpc85xx.h
> new file mode 100644
> index 000..1a1b4eb
> --- /dev/null
> +++ b/arch/powerpc/platforms/85xx/mpc85xx.h
> @@ -0,0 +1,4 @@
> +#ifndef MPC85xx_H
> +#define MPC85xx_H
> +extern int mpc85xx_common_publish_devices(void);
> +#endif
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ads.c
> b/arch/powerpc/platforms/85xx/mpc85xx_ads.c
> index 2483929..3bc2acc 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_ads.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_ads.c
> @@ -35,6 +35,8 @@
> #include
> #endif
>
> +#include "mpc85xx.h"
>
[PATCH 13/14] 85xx: consolidate of_platform_bus_probe calls
85xx board files have a lot of duplication in *_publish_devices()/
*_declare_of_platform_devices() functions. Merge that into a single
function common to most of the boards.
Signed-off-by: Dmitry Eremin-Solenikov
---
arch/powerpc/platforms/85xx/Makefile |2 +
arch/powerpc/platforms/85xx/ksi8560.c| 18 +-
arch/powerpc/platforms/85xx/mpc8536_ds.c | 16 ++---
arch/powerpc/platforms/85xx/mpc85xx.h|4 ++
arch/powerpc/platforms/85xx/mpc85xx_ads.c| 20 ++-
arch/powerpc/platforms/85xx/mpc85xx_cds.c| 16 ++---
arch/powerpc/platforms/85xx/mpc85xx_common.c | 26 ++
arch/powerpc/platforms/85xx/mpc85xx_ds.c | 20 +++
arch/powerpc/platforms/85xx/mpc85xx_mds.c| 46 ++---
arch/powerpc/platforms/85xx/mpc85xx_rdb.c| 16 ++---
arch/powerpc/platforms/85xx/p1022_ds.c |7 ++--
arch/powerpc/platforms/85xx/sbc8548.c| 18 ++
arch/powerpc/platforms/85xx/sbc8560.c| 20 ++-
arch/powerpc/platforms/85xx/socrates.c | 13 +--
arch/powerpc/platforms/85xx/stx_gp3.c| 16 ++---
arch/powerpc/platforms/85xx/tqm85xx.c| 16 ++---
arch/powerpc/platforms/85xx/xes_mpc85xx.c| 20 +++
17 files changed, 77 insertions(+), 217 deletions(-)
create mode 100644 arch/powerpc/platforms/85xx/mpc85xx.h
create mode 100644 arch/powerpc/platforms/85xx/mpc85xx_common.c
diff --git a/arch/powerpc/platforms/85xx/Makefile
b/arch/powerpc/platforms/85xx/Makefile
index 43b2162..ca4b1b9 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -3,6 +3,8 @@
#
obj-$(CONFIG_SMP) += smp.o
+obj-y += mpc85xx_common.o
+
obj-$(CONFIG_MPC8540_ADS) += mpc85xx_ads.o
obj-$(CONFIG_MPC8560_ADS) += mpc85xx_ads.o
obj-$(CONFIG_MPC85xx_CDS) += mpc85xx_cds.o
diff --git a/arch/powerpc/platforms/85xx/ksi8560.c
b/arch/powerpc/platforms/85xx/ksi8560.c
index c46f935..7657e1a 100644
--- a/arch/powerpc/platforms/85xx/ksi8560.c
+++ b/arch/powerpc/platforms/85xx/ksi8560.c
@@ -35,6 +35,7 @@
#include
#include
+#include "mpc85xx.h"
#define KSI8560_CPLD_HVR 0x04 /* Hardware Version Register */
#define KSI8560_CPLD_PVR 0x08 /* PLD Version Register */
@@ -215,22 +216,7 @@ static void ksi8560_show_cpuinfo(struct seq_file *m)
seq_printf(m, "PLL setting\t: 0x%x\n", ((phid1 >> 24) & 0x3f));
}
-static struct of_device_id __initdata of_bus_ids[] = {
- { .type = "soc", },
- { .type = "simple-bus", },
- { .name = "cpm", },
- { .name = "localbus", },
- { .compatible = "gianfar", },
- {},
-};
-
-static int __init declare_of_platform_devices(void)
-{
- of_platform_bus_probe(NULL, of_bus_ids, NULL);
-
- return 0;
-}
-machine_device_initcall(ksi8560, declare_of_platform_devices);
+machine_device_initcall(ksi8560, mpc85xx_common_publish_devices);
/*
* Called very early, device-tree isn't unflattened
diff --git a/arch/powerpc/platforms/85xx/mpc8536_ds.c
b/arch/powerpc/platforms/85xx/mpc8536_ds.c
index f79f2f1..9ee6455 100644
--- a/arch/powerpc/platforms/85xx/mpc8536_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc8536_ds.c
@@ -32,6 +32,8 @@
#include
#include
+#include "mpc85xx.h"
+
void __init mpc8536_ds_pic_init(void)
{
struct mpic *mpic;
@@ -104,19 +106,7 @@ static void __init mpc8536_ds_setup_arch(void)
printk("MPC8536 DS board from Freescale Semiconductor\n");
}
-static struct of_device_id __initdata mpc8536_ds_ids[] = {
- { .type = "soc", },
- { .compatible = "soc", },
- { .compatible = "simple-bus", },
- { .compatible = "gianfar", },
- {},
-};
-
-static int __init mpc8536_ds_publish_devices(void)
-{
- return of_platform_bus_probe(NULL, mpc8536_ds_ids, NULL);
-}
-machine_device_initcall(mpc8536_ds, mpc8536_ds_publish_devices);
+machine_device_initcall(mpc8536_ds, mpc85xx_common_publish_devices);
machine_arch_initcall(mpc8536_ds, swiotlb_setup_bus_notifier);
diff --git a/arch/powerpc/platforms/85xx/mpc85xx.h
b/arch/powerpc/platforms/85xx/mpc85xx.h
new file mode 100644
index 000..1a1b4eb
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/mpc85xx.h
@@ -0,0 +1,4 @@
+#ifndef MPC85xx_H
+#define MPC85xx_H
+extern int mpc85xx_common_publish_devices(void);
+#endif
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ads.c
b/arch/powerpc/platforms/85xx/mpc85xx_ads.c
index 2483929..3bc2acc 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ads.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ads.c
@@ -35,6 +35,8 @@
#include
#endif
+#include "mpc85xx.h"
+
#ifdef CONFIG_PCI
static int mpc85xx_exclude_device(struct pci_controller *hose,
u_char bus, u_char devfn)
@@ -219,23 +221,7 @@ static void mpc85xx_ads_show_cpuinfo(struct seq_file *m)
seq_printf(m, "PLL setting\t: 0x%x\n", ((phid1 >> 24) & 0x3f));
}
-static struct of_device_id __in
