| Message ID | 1415249396-2985-1-git-send-email-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 06, 2014 at 05:49:56PM +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. > > All of the ARMADA_370_XP_DT compatible devices are either single core or > declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi, > armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp > assignment from board-v7.c so that the SMP operations set via the > device-tree aren't discarded. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > Hi, > > (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness) Hi Chris Congratulations on a good looking patch. Nice changelog, has a signed-off-by, comments are bellow the ---. All good. It is however a good idea to Cc: the mvebu maintainers. I added a few people to Cc:. > I'm working on a new board that uses a integrated CPU that according to the > vendor is "based on the ARMADA-XP", more on that to come in the future > hopefully. A packet processor chip with an embedded CPU? Next generation Prestera? No need to answer that if you don't want to. > For the most part things just work if I use a .dts based on the > mv78260. > This is my attempt at fixing the problem by not setting mdesc.smp for the > ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has > been setup by arm_dt_init_cpu_maps. You really need Thomas or Gregory to Ack this patch, but to me it looks correct. Interestingly, this machine descriptor is used by 370, which is a single processor. The smp operations are useless, so your patch is also partly a cleanup. Andrew
Dear Chris Packham, On Thu, 6 Nov 2014 17:49:56 +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. > > All of the ARMADA_370_XP_DT compatible devices are either single core or > declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi, > armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp > assignment from board-v7.c so that the SMP operations set via the > device-tree aren't discarded. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > Hi, > > (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness) > > I'm working on a new board that uses a integrated CPU that according to the > vendor is "based on the ARMADA-XP", more on that to come in the future > hopefully. For the most part things just work if I use a .dts based on the > mv78260. > > One difference I've encountered is in the way the secondary CPU is initialised, > which requires me to supply a slightly different set of machine specific SMP > operations so I can bring up the 2nd core. It looks like I should be able to > supply a different /cpus/enable-method in the .dts and once I define the > corresponding set of operations it should be picked up. > > Which leads me to what I think could be a bug (or just a lack of clear > specification). The smp ops are set by both arm_dt_init_cpu_maps (from the > enable-method) and setup_arch (from mdesc). The latter always wins because it > is called last and doesn't check to see if smp_ops has already been set. > > This is my attempt at fixing the problem by not setting mdesc.smp for the > ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has > been setup by arm_dt_init_cpu_maps. > > Thanks, > Chris > > arch/arm/mach-mvebu/board-v7.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c > index 6478626..894974b 100644 > --- a/arch/arm/mach-mvebu/board-v7.c > +++ b/arch/arm/mach-mvebu/board-v7.c > @@ -206,7 +206,6 @@ 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, There is a very good reason to keep this .smp initialization. The Device Tree for Armada XP used to *not* have the enable-method property, since the SMP enable-method binding did not exist at the time we introduced the Armada XP SMP support. Therefore, if we want to keep backward compatibility with the old Armada XP DTs and continue to have SMP support working with those, we need to keep this .smp initialization essentially forever. Yes, backward compatibility sometimes sucks :-/ Best regards, Thomas
> > diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c > > index 6478626..894974b 100644 > > --- a/arch/arm/mach-mvebu/board-v7.c > > +++ b/arch/arm/mach-mvebu/board-v7.c > > @@ -206,7 +206,6 @@ 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, > > There is a very good reason to keep this .smp initialization. The > Device Tree for Armada XP used to *not* have the enable-method > property, since the SMP enable-method binding did not exist at the > time we introduced the Armada XP SMP support. Therefore, if we want to > keep backward compatibility with the old Armada XP DTs and continue to > have SMP support working with those, we need to keep this .smp > initialization essentially forever. Hi Thomas Any idea what order things are done? Would it be possible to remove the .smp entry, and then in mvebu_dt_init() check if there are smp operations set. If not, then set them to armada_xp_smp_ops? Andrew
Dear Andrew Lunn, On Thu, 6 Nov 2014 16:21:35 +0100, Andrew Lunn wrote: > Any idea what order things are done? > > Would it be possible to remove the .smp entry, and then in > mvebu_dt_init() check if there are smp operations set. If not, then > set them to armada_xp_smp_ops? ->init_machine() is *way* too late for SMP. ->init_early() is also too late as far as I remember. ->dt_fixup() would work, though I believe. Best regards, Thomas
On 11/07/2014 03:49 AM, Andrew Lunn wrote: > On Thu, Nov 06, 2014 at 05:49:56PM +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. >> >> All of the ARMADA_370_XP_DT compatible devices are either single core or >> declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi, >> armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp >> assignment from board-v7.c so that the SMP operations set via the >> device-tree aren't discarded. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- >> Hi, >> >> (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness) > Hi Chris > > Congratulations on a good looking patch. Nice changelog, has a > signed-off-by, comments are bellow the ---. All good. > > It is however a good idea to Cc: the mvebu maintainers. I added a few > people to Cc:. Thanks for the warm welcome. I actually thought the same thing after I hit send, thanks for filling in the Cc list for me. >> I'm working on a new board that uses a integrated CPU that according to the >> vendor is "based on the ARMADA-XP", more on that to come in the future >> hopefully. > A packet processor chip with an embedded CPU? Next generation > Prestera? No need to answer that if you don't want to. You guess correctly. Not sure how much I can say without falling foul of our NDA but they know I intend on trying to get this stuff upstream rather than being locked into their particular SDK. >> For the most part things just work if I use a .dts based on the >> mv78260. > >> This is my attempt at fixing the problem by not setting mdesc.smp for the >> ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has >> been setup by arm_dt_init_cpu_maps. > You really need Thomas or Gregory to Ack this patch, but to me it > looks correct. Interestingly, this machine descriptor is used by 370, > which is a single processor. The smp operations are useless, so your > patch is also partly a cleanup. > > Andrew
Hi Thomas, On 11/07/2014 03:58 AM, Thomas Petazzoni wrote: > Dear Chris Packham, > > On Thu, 6 Nov 2014 17:49:56 +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. >> >> All of the ARMADA_370_XP_DT compatible devices are either single core or >> declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi, >> armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp >> assignment from board-v7.c so that the SMP operations set via the >> device-tree aren't discarded. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- >> Hi, >> >> (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness) >> >> I'm working on a new board that uses a integrated CPU that according to the >> vendor is "based on the ARMADA-XP", more on that to come in the future >> hopefully. For the most part things just work if I use a .dts based on the >> mv78260. >> >> One difference I've encountered is in the way the secondary CPU is initialised, >> which requires me to supply a slightly different set of machine specific SMP >> operations so I can bring up the 2nd core. It looks like I should be able to >> supply a different /cpus/enable-method in the .dts and once I define the >> corresponding set of operations it should be picked up. >> >> Which leads me to what I think could be a bug (or just a lack of clear >> specification). The smp ops are set by both arm_dt_init_cpu_maps (from the >> enable-method) and setup_arch (from mdesc). The latter always wins because it >> is called last and doesn't check to see if smp_ops has already been set. >> >> This is my attempt at fixing the problem by not setting mdesc.smp for the >> ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has >> been setup by arm_dt_init_cpu_maps. >> >> Thanks, >> Chris >> >> arch/arm/mach-mvebu/board-v7.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c >> index 6478626..894974b 100644 >> --- a/arch/arm/mach-mvebu/board-v7.c >> +++ b/arch/arm/mach-mvebu/board-v7.c >> @@ -206,7 +206,6 @@ 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, > There is a very good reason to keep this .smp initialization. The > Device Tree for Armada XP used to *not* have the enable-method > property, since the SMP enable-method binding did not exist at the > time we introduced the Armada XP SMP support. Therefore, if we want to > keep backward compatibility with the old Armada XP DTs and continue to > have SMP support working with those, we need to keep this .smp > initialization essentially forever. > > Yes, backward compatibility sometimes sucks :-/ Darn. I thought that might be the case but I wanted to keep the scope of my change small. How about making setup_arch check if smp_ops is already set? I didn't do that initially because I thought that might affect too many platforms. Alternatively if we gave ARMADA_370_XP_DT a smp_init function that returned non-zero if smp_ops is already set then I think it'd be backwards compatible and keep the scope of the change restricted to the 370-XP platforms.
> >> I'm working on a new board that uses a integrated CPU that according to the > >> vendor is "based on the ARMADA-XP", more on that to come in the future > >> hopefully. > > A packet processor chip with an embedded CPU? Next generation > > Prestera? No need to answer that if you don't want to. > You guess correctly. Not sure how much I can say without falling foul of > our NDA but they know I intend on trying to get this stuff upstream > rather than being locked into their particular SDK. Hi Chris Nice to hear you want to upstream it. There are some interesting chips, even in consumer level devices like the TPE-1620WS, which i would love to play with. But the Switching part of Marvell don't seem interested in the community, unlike the processors part of Marvell which is engaging with the community. Andrew
> Darn. I thought that might be the case but I wanted to keep the scope of > my change small. How about making setup_arch check if smp_ops is already > set? I think that would be last resort solution. Try not to touch core code unless you really have to. > Alternatively if we gave ARMADA_370_XP_DT a smp_init function that > returned non-zero if smp_ops is already set then I think it'd be > backwards compatible and keep the scope of the change restricted to > the 370-XP platforms. This sounds more reasonable. Andrew
diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c index 6478626..894974b 100644 --- a/arch/arm/mach-mvebu/board-v7.c +++ b/arch/arm/mach-mvebu/board-v7.c @@ -206,7 +206,6 @@ 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,
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. All of the ARMADA_370_XP_DT compatible devices are either single core or declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi, armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp assignment from board-v7.c so that the SMP operations set via the device-tree aren't discarded. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- Hi, (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness) I'm working on a new board that uses a integrated CPU that according to the vendor is "based on the ARMADA-XP", more on that to come in the future hopefully. For the most part things just work if I use a .dts based on the mv78260. One difference I've encountered is in the way the secondary CPU is initialised, which requires me to supply a slightly different set of machine specific SMP operations so I can bring up the 2nd core. It looks like I should be able to supply a different /cpus/enable-method in the .dts and once I define the corresponding set of operations it should be picked up. Which leads me to what I think could be a bug (or just a lack of clear specification). The smp ops are set by both arm_dt_init_cpu_maps (from the enable-method) and setup_arch (from mdesc). The latter always wins because it is called last and doesn't check to see if smp_ops has already been set. This is my attempt at fixing the problem by not setting mdesc.smp for the ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has been setup by arm_dt_init_cpu_maps. Thanks, Chris arch/arm/mach-mvebu/board-v7.c | 1 - 1 file changed, 1 deletion(-)