| Message ID | alpine.DEB.2.10.1411181233160.902@chrisp-dl.ws.atlnz.lc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/18/2014 12:34 PM, Chris Packham wrote: > Hi Thomas, Maxime > > On Mon, 17 Nov 2014, Thomas Petazzoni wrote: > >> Dear Chris Packham, >> >> On Fri, 7 Nov 2014 15:33:46 +1300, Chris Packham wrote: >>> The machine specific SMP operations can be configured either via >>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices >>> both of these are called and setup_arch wins because it is called last. >>> This means that it is not possible to substitute a different set of SMP >>> operations via the device-tree. >>> >>> For the ARMADA_370_XP_DT compatible devices add a smp_init function that >>> detects if the device tree has an enable-method defined. If it does >>> return true to indicate that the smp_ops have already been set which >>> will prevent setup_arch from overriding them. >>> >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> >> My colleague Maxime Ripard (in Cc) rightfully suggests exploring a >> different option: what about getting rid completely of the .smp field >> of the DT_MACHINE structure, and instead have some code run early >> enough that looks if an enable-method is defined, and if not, defines >> it to the default value. This way, we continue to be backward >> compatible in terms of DT, but we always use the enable-method from the >> DT, and not sometimes from DT, sometimes from the DT_MACHINE structure. >> >> Unfortunately, it will have to be done on the flattened DT, because the >> DT is unflattened right before the enable-method properties are looked >> up: >> >> unflatten_device_tree(); >> >> arm_dt_init_cpu_maps(); >> >> And manipulating the DT in its flattened format, while possible in >> ->dt_fixup(), is a pain, and probably doesn't allow adding new >> properties anyway. >> >> So, in the end, maybe this idea doesn't work, I haven't checked >> completely. >> > > So yeah it's tricky to work with the flattened dt. Not impossible the > powerpc arch code does quite a lot with it. Arm does less but still uses > it in atags_to_fdt.c. Below is a rough attempt at an implementation that > seems to work. Because of the flattened dt it's harder to iterate > through the cpu nodes so I haven't implemented anything to look for an > enable-method attached to them. (I really need to figure out how to tell Thunderbird how to wrap some parts but not others) > > ---- 8< ---- > Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for > enable-method > > When the device tree doesn't define an enable-method insert a property > into the flattened device tree. arm_dt_init_cpu_maps() will then parse > this an set smp_ops appropriately. Now that we have this fallback it is > no longer necessary to set .smp in the DT_MACHINE definition. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > arch/arm/mach-mvebu/Makefile | 2 ++ > arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++- > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile > index 1636cdb..f818eaf 100644 > --- a/arch/arm/mach-mvebu/Makefile > +++ b/arch/arm/mach-mvebu/Makefile > @@ -14,3 +14,5 @@ endif > > obj-$(CONFIG_MACH_DOVE) += dove.o > obj-$(CONFIG_MACH_KIRKWOOD) += kirkwood.o kirkwood-pm.o > + > +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt > \ No newline at end of file > diff --git a/arch/arm/mach-mvebu/board-v7.c > b/arch/arm/mach-mvebu/board-v7.c > index b2524d6..45851a2 100644 > --- a/arch/arm/mach-mvebu/board-v7.c > +++ b/arch/arm/mach-mvebu/board-v7.c > @@ -17,6 +17,8 @@ > #include <linux/clk-provider.h> > #include <linux/of_address.h> > #include <linux/of_platform.h> > +#include <linux/of_fdt.h> > +#include <linux/libfdt.h> > #include <linux/io.h> > #include <linux/clocksource.h> > #include <linux/dma-mapping.h> > @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void) > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > } > > +static void __init armada_370_xp_dt_fixup(void) > +{ > + void *prop; > + int offset; > + int len; > + > + offset = fdt_path_offset(initial_boot_params, "/cpus"); > + prop = fdt_getprop(initial_boot_params, offset, "enable-method", > &len); > + > + if (!prop) { > + pr_info("No enable-method defined. " > + "Falling back to \"marvell,armada-xp-smp\"\n"); > + > + fdt_appendprop(initial_boot_params, offset, "enable-method", > + "marvell,armada-xp-smp", > + sizeof("marvell,armada-xp-smp")); Instead of inserting something into the device tree I could just call set_smp_ops here. That might be safer than trying to insert something into the device tree. > + } > +} > + > static const char * const armada_370_xp_dt_compat[] = { > "marvell,armada-370-xp", > NULL, > @@ -192,11 +213,11 @@ static const char * const > armada_370_xp_dt_compat[] = { > DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)") > .l2c_aux_val = 0, > .l2c_aux_mask = ~0, > - .smp = smp_ops(armada_xp_smp_ops), > .init_machine = mvebu_dt_init, > .init_irq = mvebu_init_irq, > .restart = mvebu_restart, > .dt_compat = armada_370_xp_dt_compat, > + .dt_fixup = armada_370_xp_dt_fixup, > MACHINE_END > > static const char * const armada_375_dt_compat[] = {
Hi, On Tue, Nov 18, 2014 at 12:34:41PM +1300, Chris Packham wrote: > Hi Thomas, Maxime > > On Mon, 17 Nov 2014, Thomas Petazzoni wrote: > > >Dear Chris Packham, > > > >On Fri, 7 Nov 2014 15:33:46 +1300, Chris Packham wrote: > >>The machine specific SMP operations can be configured either via > >>setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices > >>both of these are called and setup_arch wins because it is called last. > >>This means that it is not possible to substitute a different set of SMP > >>operations via the device-tree. > >> > >>For the ARMADA_370_XP_DT compatible devices add a smp_init function that > >>detects if the device tree has an enable-method defined. If it does > >>return true to indicate that the smp_ops have already been set which > >>will prevent setup_arch from overriding them. > >> > >>Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > > >My colleague Maxime Ripard (in Cc) rightfully suggests exploring a > >different option: what about getting rid completely of the .smp field > >of the DT_MACHINE structure, and instead have some code run early > >enough that looks if an enable-method is defined, and if not, defines > >it to the default value. This way, we continue to be backward > >compatible in terms of DT, but we always use the enable-method from the > >DT, and not sometimes from DT, sometimes from the DT_MACHINE structure. > > > >Unfortunately, it will have to be done on the flattened DT, because the > >DT is unflattened right before the enable-method properties are looked > >up: > > > > unflatten_device_tree(); > > > > arm_dt_init_cpu_maps(); > > > >And manipulating the DT in its flattened format, while possible in > >->dt_fixup(), is a pain, and probably doesn't allow adding new > >properties anyway. > > > >So, in the end, maybe this idea doesn't work, I haven't checked > >completely. > > > > So yeah it's tricky to work with the flattened dt. Not impossible the > powerpc arch code does quite a lot with it. Arm does less but still uses it > in atags_to_fdt.c. Below is a rough attempt at an implementation that seems > to work. Because of the flattened dt it's harder to iterate through the cpu > nodes so I haven't implemented anything to look for an enable-method > attached to them. > > ---- 8< ---- > Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for > enable-method > > When the device tree doesn't define an enable-method insert a property > into the flattened device tree. arm_dt_init_cpu_maps() will then parse > this an set smp_ops appropriately. Now that we have this fallback it is > no longer necessary to set .smp in the DT_MACHINE definition. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > arch/arm/mach-mvebu/Makefile | 2 ++ > arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++- > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile > index 1636cdb..f818eaf 100644 > --- a/arch/arm/mach-mvebu/Makefile > +++ b/arch/arm/mach-mvebu/Makefile > @@ -14,3 +14,5 @@ endif > > obj-$(CONFIG_MACH_DOVE) += dove.o > obj-$(CONFIG_MACH_KIRKWOOD) += kirkwood.o kirkwood-pm.o > + > +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt > \ No newline at end of file > diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c > index b2524d6..45851a2 100644 > --- a/arch/arm/mach-mvebu/board-v7.c > +++ b/arch/arm/mach-mvebu/board-v7.c > @@ -17,6 +17,8 @@ > #include <linux/clk-provider.h> > #include <linux/of_address.h> > #include <linux/of_platform.h> > +#include <linux/of_fdt.h> > +#include <linux/libfdt.h> > #include <linux/io.h> > #include <linux/clocksource.h> > #include <linux/dma-mapping.h> > @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void) > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > } > > +static void __init armada_370_xp_dt_fixup(void) > +{ > + void *prop; > + int offset; > + int len; > + > + offset = fdt_path_offset(initial_boot_params, "/cpus"); > + prop = fdt_getprop(initial_boot_params, offset, "enable-method", &len); > + > + if (!prop) { > + pr_info("No enable-method defined. " > + "Falling back to \"marvell,armada-xp-smp\"\n"); > + > + fdt_appendprop(initial_boot_params, offset, "enable-method", > + "marvell,armada-xp-smp", > + sizeof("marvell,armada-xp-smp")); > + } > +} > + It's what I had in mind yes :) I don't think you need to use fdt_appendprop though, fdt_setprop would be enough. Maxime
On Tue, Nov 18, 2014 at 12:31:52AM +0000, Chris Packham wrote: > > > On 11/18/2014 12:34 PM, Chris Packham wrote: > > Hi Thomas, Maxime > > > > On Mon, 17 Nov 2014, Thomas Petazzoni wrote: > > > >> Dear Chris Packham, > >> > >> On Fri, 7 Nov 2014 15:33:46 +1300, Chris Packham wrote: > >>> The machine specific SMP operations can be configured either via > >>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices > >>> both of these are called and setup_arch wins because it is called last. > >>> This means that it is not possible to substitute a different set of SMP > >>> operations via the device-tree. > >>> > >>> For the ARMADA_370_XP_DT compatible devices add a smp_init function that > >>> detects if the device tree has an enable-method defined. If it does > >>> return true to indicate that the smp_ops have already been set which > >>> will prevent setup_arch from overriding them. > >>> > >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > >> > >> My colleague Maxime Ripard (in Cc) rightfully suggests exploring a > >> different option: what about getting rid completely of the .smp field > >> of the DT_MACHINE structure, and instead have some code run early > >> enough that looks if an enable-method is defined, and if not, defines > >> it to the default value. This way, we continue to be backward > >> compatible in terms of DT, but we always use the enable-method from the > >> DT, and not sometimes from DT, sometimes from the DT_MACHINE structure. > >> > >> Unfortunately, it will have to be done on the flattened DT, because the > >> DT is unflattened right before the enable-method properties are looked > >> up: > >> > >> unflatten_device_tree(); > >> > >> arm_dt_init_cpu_maps(); > >> > >> And manipulating the DT in its flattened format, while possible in > >> ->dt_fixup(), is a pain, and probably doesn't allow adding new > >> properties anyway. > >> > >> So, in the end, maybe this idea doesn't work, I haven't checked > >> completely. > >> > > > > So yeah it's tricky to work with the flattened dt. Not impossible the > > powerpc arch code does quite a lot with it. Arm does less but still uses > > it in atags_to_fdt.c. Below is a rough attempt at an implementation that > > seems to work. Because of the flattened dt it's harder to iterate > > through the cpu nodes so I haven't implemented anything to look for an > > enable-method attached to them. > > (I really need to figure out how to tell Thunderbird how to wrap some > parts but not others) > > > > > ---- 8< ---- > > Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for > > enable-method > > > > When the device tree doesn't define an enable-method insert a property > > into the flattened device tree. arm_dt_init_cpu_maps() will then parse > > this an set smp_ops appropriately. Now that we have this fallback it is > > no longer necessary to set .smp in the DT_MACHINE definition. > > > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > --- > > arch/arm/mach-mvebu/Makefile | 2 ++ > > arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++- > > 2 files changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile > > index 1636cdb..f818eaf 100644 > > --- a/arch/arm/mach-mvebu/Makefile > > +++ b/arch/arm/mach-mvebu/Makefile > > @@ -14,3 +14,5 @@ endif > > > > obj-$(CONFIG_MACH_DOVE) += dove.o > > obj-$(CONFIG_MACH_KIRKWOOD) += kirkwood.o kirkwood-pm.o > > + > > +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt > > \ No newline at end of file > > diff --git a/arch/arm/mach-mvebu/board-v7.c > > b/arch/arm/mach-mvebu/board-v7.c > > index b2524d6..45851a2 100644 > > --- a/arch/arm/mach-mvebu/board-v7.c > > +++ b/arch/arm/mach-mvebu/board-v7.c > > @@ -17,6 +17,8 @@ > > #include <linux/clk-provider.h> > > #include <linux/of_address.h> > > #include <linux/of_platform.h> > > +#include <linux/of_fdt.h> > > +#include <linux/libfdt.h> > > #include <linux/io.h> > > #include <linux/clocksource.h> > > #include <linux/dma-mapping.h> > > @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void) > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > } > > > > +static void __init armada_370_xp_dt_fixup(void) > > +{ > > + void *prop; > > + int offset; > > + int len; > > + > > + offset = fdt_path_offset(initial_boot_params, "/cpus"); > > + prop = fdt_getprop(initial_boot_params, offset, "enable-method", > > &len); > > + > > + if (!prop) { > > + pr_info("No enable-method defined. " > > + "Falling back to \"marvell,armada-xp-smp\"\n"); > > + > > + fdt_appendprop(initial_boot_params, offset, "enable-method", > > + "marvell,armada-xp-smp", > > + sizeof("marvell,armada-xp-smp")); > > Instead of inserting something into the device tree I could just call > set_smp_ops here. That might be safer than trying to insert something > into the device tree. I don't think this is necessary. Injecting something in the DT is safe, u-boot does that at every boot :) Maxime
Hi Maxime, On 11/18/2014 09:21 PM, Maxime Ripard wrote: > On Tue, Nov 18, 2014 at 12:31:52AM +0000, Chris Packham wrote: >> >> >> On 11/18/2014 12:34 PM, Chris Packham wrote: >>> Hi Thomas, Maxime >>> >>> On Mon, 17 Nov 2014, Thomas Petazzoni wrote: >>> >>>> Dear Chris Packham, >>>> >>>> On Fri, 7 Nov 2014 15:33:46 +1300, Chris Packham wrote: >>>>> The machine specific SMP operations can be configured either via >>>>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices >>>>> both of these are called and setup_arch wins because it is called last. >>>>> This means that it is not possible to substitute a different set of SMP >>>>> operations via the device-tree. >>>>> >>>>> For the ARMADA_370_XP_DT compatible devices add a smp_init function that >>>>> detects if the device tree has an enable-method defined. If it does >>>>> return true to indicate that the smp_ops have already been set which >>>>> will prevent setup_arch from overriding them. >>>>> >>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>>> >>>> My colleague Maxime Ripard (in Cc) rightfully suggests exploring a >>>> different option: what about getting rid completely of the .smp field >>>> of the DT_MACHINE structure, and instead have some code run early >>>> enough that looks if an enable-method is defined, and if not, defines >>>> it to the default value. This way, we continue to be backward >>>> compatible in terms of DT, but we always use the enable-method from the >>>> DT, and not sometimes from DT, sometimes from the DT_MACHINE structure. >>>> >>>> Unfortunately, it will have to be done on the flattened DT, because the >>>> DT is unflattened right before the enable-method properties are looked >>>> up: >>>> >>>> unflatten_device_tree(); >>>> >>>> arm_dt_init_cpu_maps(); >>>> >>>> And manipulating the DT in its flattened format, while possible in >>>> ->dt_fixup(), is a pain, and probably doesn't allow adding new >>>> properties anyway. >>>> >>>> So, in the end, maybe this idea doesn't work, I haven't checked >>>> completely. >>>> >>> >>> So yeah it's tricky to work with the flattened dt. Not impossible the >>> powerpc arch code does quite a lot with it. Arm does less but still uses >>> it in atags_to_fdt.c. Below is a rough attempt at an implementation that >>> seems to work. Because of the flattened dt it's harder to iterate >>> through the cpu nodes so I haven't implemented anything to look for an >>> enable-method attached to them. >> >> (I really need to figure out how to tell Thunderbird how to wrap some >> parts but not others) >> >>> >>> ---- 8< ---- >>> Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for >>> enable-method >>> >>> When the device tree doesn't define an enable-method insert a property >>> into the flattened device tree. arm_dt_init_cpu_maps() will then parse >>> this an set smp_ops appropriately. Now that we have this fallback it is >>> no longer necessary to set .smp in the DT_MACHINE definition. >>> >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>> --- >>> arch/arm/mach-mvebu/Makefile | 2 ++ >>> arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++- >>> 2 files changed, 24 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile >>> index 1636cdb..f818eaf 100644 >>> --- a/arch/arm/mach-mvebu/Makefile >>> +++ b/arch/arm/mach-mvebu/Makefile >>> @@ -14,3 +14,5 @@ endif >>> >>> obj-$(CONFIG_MACH_DOVE) += dove.o >>> obj-$(CONFIG_MACH_KIRKWOOD) += kirkwood.o kirkwood-pm.o >>> + >>> +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt >>> \ No newline at end of file >>> diff --git a/arch/arm/mach-mvebu/board-v7.c >>> b/arch/arm/mach-mvebu/board-v7.c >>> index b2524d6..45851a2 100644 >>> --- a/arch/arm/mach-mvebu/board-v7.c >>> +++ b/arch/arm/mach-mvebu/board-v7.c >>> @@ -17,6 +17,8 @@ >>> #include <linux/clk-provider.h> >>> #include <linux/of_address.h> >>> #include <linux/of_platform.h> >>> +#include <linux/of_fdt.h> >>> +#include <linux/libfdt.h> >>> #include <linux/io.h> >>> #include <linux/clocksource.h> >>> #include <linux/dma-mapping.h> >>> @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void) >>> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); >>> } >>> >>> +static void __init armada_370_xp_dt_fixup(void) >>> +{ >>> + void *prop; >>> + int offset; >>> + int len; >>> + >>> + offset = fdt_path_offset(initial_boot_params, "/cpus"); >>> + prop = fdt_getprop(initial_boot_params, offset, "enable-method", >>> &len); >>> + >>> + if (!prop) { >>> + pr_info("No enable-method defined. " >>> + "Falling back to \"marvell,armada-xp-smp\"\n"); >>> + >>> + fdt_appendprop(initial_boot_params, offset, "enable-method", >>> + "marvell,armada-xp-smp", >>> + sizeof("marvell,armada-xp-smp")); >> >> Instead of inserting something into the device tree I could just call >> set_smp_ops here. That might be safer than trying to insert something >> into the device tree. > > I don't think this is necessary. Injecting something in the DT is > safe, u-boot does that at every boot :) > Actually I thought a bit more about this option this morning. What I'm trying to do is provide a fallback that defines smp_ops when there isn't an enable-method in the device tree. I don't actually need do do anything to the incoming device tree, I don't even need to look at it. I can unconditionally call set_smp_ops() and if the device tree has an enable-method it will override whatever has been configured. If the device tree doesn't define an enable-method it will use the default that I've configured here. That's actually very little code and can all be contained in board-v7.c.
diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile index 1636cdb..f818eaf 100644 --- a/arch/arm/mach-mvebu/Makefile +++ b/arch/arm/mach-mvebu/Makefile @@ -14,3 +14,5 @@ endif obj-$(CONFIG_MACH_DOVE) += dove.o obj-$(CONFIG_MACH_KIRKWOOD) += kirkwood.o kirkwood-pm.o + +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt \ No newline at end of file diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c index b2524d6..45851a2 100644 --- a/arch/arm/mach-mvebu/board-v7.c +++ b/arch/arm/mach-mvebu/board-v7.c @@ -17,6 +17,8 @@ #include <linux/clk-provider.h> #include <linux/of_address.h> #include <linux/of_platform.h> +#include <linux/of_fdt.h> +#include <linux/libfdt.h> #include <linux/io.h> #include <linux/clocksource.h> #include <linux/dma-mapping.h> @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void) of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); } +static void __init armada_370_xp_dt_fixup(void) +{ + void *prop; + int offset; + int len; + + offset = fdt_path_offset(initial_boot_params, "/cpus"); + prop = fdt_getprop(initial_boot_params, offset, "enable-method", &len); + + if (!prop) { + pr_info("No enable-method defined. " + "Falling back to \"marvell,armada-xp-smp\"\n"); + + fdt_appendprop(initial_boot_params, offset, "enable-method", + "marvell,armada-xp-smp", + sizeof("marvell,armada-xp-smp")); + } +} +