| Message ID | 20191108174112.28183-2-vgupta@synopsys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARC buildroot fixes/updates | expand |
Hello, +Arnout for legacy handling. On Fri, 8 Nov 2019 09:41:10 -0800 Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote: > This corresponds to -mcu=hs38 with mpy-option=9 (64-bit multiplier) > > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > --- > arch/Config.in.arc | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/arch/Config.in.arc b/arch/Config.in.arc > index c65bb01f1f4f..284951b82cee 100644 > --- a/arch/Config.in.arc > +++ b/arch/Config.in.arc > @@ -11,13 +11,19 @@ config BR2_arc750d > config BR2_arc770d > bool "ARC 770D" > > -config BR2_archs38 > +config BR2_archs > bool "ARC HS38" > help > Generic ARC HS capable of running Linux, i.e. with MMU, > - caches and multiplier. Also it corresponds to the default > + caches and 32-bit multiplier. Also it corresponds to the default > configuration in older GNU toolchain versions. > > +config BR2_archs38 This re-use of an existing name is a bit annoying. Indeed, all existing users of Buildroot that have a configuration with BR2_archs38 will now be building for a ARC system with a 64-bit multiplier, while they were previously building for a 32-bit multiplier. I see that what you have done is to try to be consistent between the BR2_ options and the gcc options. I'm hesitating between keeping the consistency but making the migration a bit annoying for users, or breaking the consistency to make the migration smooth for users. Since I think the number of affected users will probably be quite small/limited, I think I would be fine with merging your patch as-is, but I'd like to hear from others. Thanks, Thomas
Vineet, Thomas, All, On 2019-11-09 14:46 +0100, Thomas Petazzoni spake thusly: > +Arnout for legacy handling. > > On Fri, 8 Nov 2019 09:41:10 -0800 > Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote: > > > This corresponds to -mcu=hs38 with mpy-option=9 (64-bit multiplier) > > > > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > > --- > > arch/Config.in.arc | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/arch/Config.in.arc b/arch/Config.in.arc > > index c65bb01f1f4f..284951b82cee 100644 > > --- a/arch/Config.in.arc > > +++ b/arch/Config.in.arc > > @@ -11,13 +11,19 @@ config BR2_arc750d > > config BR2_arc770d > > bool "ARC 770D" > > > > -config BR2_archs38 > > +config BR2_archs > > bool "ARC HS38" > > help > > Generic ARC HS capable of running Linux, i.e. with MMU, > > - caches and multiplier. Also it corresponds to the default > > + caches and 32-bit multiplier. Also it corresponds to the default > > configuration in older GNU toolchain versions. > > > > +config BR2_archs38 > > This re-use of an existing name is a bit annoying. Indeed, all existing > users of Buildroot that have a configuration with BR2_archs38 will now > be building for a ARC system with a 64-bit multiplier, while they were > previously building for a 32-bit multiplier. > > I see that what you have done is to try to be consistent between the > BR2_ options and the gcc options. I'm hesitating between keeping the > consistency but making the migration a bit annoying for users, or > breaking the consistency to make the migration smooth for users. > > Since I think the number of affected users will probably be quite > small/limited, I think I would be fine with merging your patch as-is, > but I'd like to hear from others. I would prefer if we kept the existing name as-is, and introduce the new variant under a different name, e.g.: config BR2_archs38 bool "ARC HS38" config BR2_archs68_64mpy bool "ARC HS38 w/ 64-bit mpy" Because, well, they both are HS38 cores, so we are not wrong in naming both options with hs38. It would have been good that the config names match the gcc option, of course, but that is a minor detail I believe... Regards, Yann E. MORIN. > Thanks, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Thomas, On 11/9/19 5:46 AM, Thomas Petazzoni wrote: > Hello, > > +Arnout for legacy handling. > > On Fri, 8 Nov 2019 09:41:10 -0800 > Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote: > >> This corresponds to -mcu=hs38 with mpy-option=9 (64-bit multiplier) >> >> Signed-off-by: Vineet Gupta <vgupta@synopsys.com> >> --- >> arch/Config.in.arc | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/arch/Config.in.arc b/arch/Config.in.arc >> index c65bb01f1f4f..284951b82cee 100644 >> --- a/arch/Config.in.arc >> +++ b/arch/Config.in.arc >> @@ -11,13 +11,19 @@ config BR2_arc750d >> config BR2_arc770d >> bool "ARC 770D" >> >> -config BR2_archs38 >> +config BR2_archs >> bool "ARC HS38" >> help >> Generic ARC HS capable of running Linux, i.e. with MMU, >> - caches and multiplier. Also it corresponds to the default >> + caches and 32-bit multiplier. Also it corresponds to the default >> configuration in older GNU toolchain versions. >> >> +config BR2_archs38 > This re-use of an existing name is a bit annoying. Indeed, all existing > users of Buildroot that have a configuration with BR2_archs38 will now > be building for a ARC system with a 64-bit multiplier, while they were > previously building for a 32-bit multiplier. > > I see that what you have done is to try to be consistent between the > BR2_ options and the gcc options. I'm hesitating between keeping the > consistency but making the migration a bit annoying for users, or > breaking the consistency to make the migration smooth for users. > > Since I think the number of affected users will probably be quite > small/limited, I think I would be fine with merging your patch as-is, > but I'd like to hear from others. I agree that this might cause potential breakage, but this is not totally uncharted territory for software build config. We've been building Linux kernel with this option as default since mid 2018. 2018-09-07 00a99339f0a3 ARCv2: build: use mcpu=hs38 iso generic mcpu=archs Granted that kernel build is just one part of puzzle and any latent codegen issues are more likely to surface with default applied to full software stack, I would still vote for switching default to -mcpu=hs38 Thx, -Vineet
Vineet, All, On 2019-11-11 18:47 +0000, Vineet Gupta spake thusly: > On 11/9/19 5:46 AM, Thomas Petazzoni wrote: > > On Fri, 8 Nov 2019 09:41:10 -0800 > > Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote: > >> Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > >> --- > >> arch/Config.in.arc | 21 ++++++++++++++------- > >> 1 file changed, 14 insertions(+), 7 deletions(-) > >> > >> diff --git a/arch/Config.in.arc b/arch/Config.in.arc > >> index c65bb01f1f4f..284951b82cee 100644 > >> --- a/arch/Config.in.arc > >> +++ b/arch/Config.in.arc > >> @@ -11,13 +11,19 @@ config BR2_arc750d > >> config BR2_arc770d > >> bool "ARC 770D" > >> > >> -config BR2_archs38 > >> +config BR2_archs [Note this change, above ^^^] > >> bool "ARC HS38" > >> help > >> Generic ARC HS capable of running Linux, i.e. with MMU, > >> - caches and multiplier. Also it corresponds to the default > >> + caches and 32-bit multiplier. Also it corresponds to the default > >> configuration in older GNU toolchain versions. > >> > >> +config BR2_archs38 > >> + bool "ARC HS38 with 64-bit mpy" [and note this, above^^^] > > This re-use of an existing name is a bit annoying. Indeed, all existing > > users of Buildroot that have a configuration with BR2_archs38 will now > > be building for a ARC system with a 64-bit multiplier, while they were > > previously building for a 32-bit multiplier. > > > > I see that what you have done is to try to be consistent between the > > BR2_ options and the gcc options. I'm hesitating between keeping the > > consistency but making the migration a bit annoying for users, or > > breaking the consistency to make the migration smooth for users. > > > > Since I think the number of affected users will probably be quite > > small/limited, I think I would be fine with merging your patch as-is, > > but I'd like to hear from others. > > I agree that this might cause potential breakage, but this is not totally > uncharted territory for software build config. We've been building Linux kernel > with this option as default since mid 2018. I think there is some misunderstanding. What Thomas and I argue on, is the way to change the meaning for the BR2_archs38 option. Before this patch, BR2_archs38 meant "ARC HS38". But with your patch, it now means "ARC HS38 with 64-bit mpy". So, a user that updates their Buildroot configurationi which previously had a "plain" HS38 setup, but with this patch, they get an "extended" HS38 with the 64-bit multiplier. That's why I suggested in my own reply, to keep BR2_archs38 as it was before, and introduce BR2_archs38_64mpy to mean the new HS38 w/ the 64-bit multiplier. Indeed, it does not match the gcc config options, but that would hardly be the only derogation we have in Buildroot... ;-) > 2018-09-07 00a99339f0a3 ARCv2: build: use mcpu=hs38 iso generic mcpu=archs > > Granted that kernel build is just one part of puzzle and any latent codegen issues > are more likely to surface with default applied to full software stack, I would > still vote for switching default to -mcpu=hs38 Changing the meaning of an option, and changing the default of a choice, are two different things. I'm OK with changing the default, but I'd rather that options keep their meaning. Regards, Yann E. MORIN.
Hi Yann, On 11/11/19 1:09 PM, Yann E. MORIN wrote: > >>> This re-use of an existing name is a bit annoying. Indeed, all existing >>> users of Buildroot that have a configuration with BR2_archs38 will now >>> be building for a ARC system with a 64-bit multiplier, while they were >>> previously building for a 32-bit multiplier. >>> >>> I see that what you have done is to try to be consistent between the >>> BR2_ options and the gcc options. I'm hesitating between keeping the >>> consistency but making the migration a bit annoying for users, or >>> breaking the consistency to make the migration smooth for users. >>> >>> Since I think the number of affected users will probably be quite >>> small/limited, I think I would be fine with merging your patch as-is, >>> but I'd like to hear from others. >> I agree that this might cause potential breakage, but this is not totally >> uncharted territory for software build config. We've been building Linux kernel >> with this option as default since mid 2018. > I think there is some misunderstanding. Not really, I understand what you and Thomas are asking for - its not a super complicated patch after all ;-) > What Thomas and I argue on, is the way to change the meaning for the > BR2_archs38 option. Yeah I understand that part. > Before this patch, BR2_archs38 meant "ARC HS38". But with your patch, it > now means "ARC HS38 with 64-bit mpy". > > So, a user that updates their Buildroot configurationi which previously > had a "plain" HS38 setup, but with this patch, they get an "extended" > HS38 with the 64-bit multiplier. > > That's why I suggested in my own reply, to keep BR2_archs38 as it was > before, and introduce BR2_archs38_64mpy to mean the new HS38 w/ the > 64-bit multiplier. > > Indeed, it does not match the gcc config options, but that would hardly > be the only derogation we have in Buildroot... ;-) Right that's not the point of doing this exercise anyways. >> 2018-09-07 00a99339f0a3 ARCv2: build: use mcpu=hs38 iso generic mcpu=archs >> >> Granted that kernel build is just one part of puzzle and any latent codegen issues >> are more likely to surface with default applied to full software stack, I would >> still vote for switching default to -mcpu=hs38 > Changing the meaning of an option, and changing the default of a choice, > are two different things. I'm OK with changing the default, but I'd > rather that options keep their meaning. Ok sounds good ! Thx, -Vineet
diff --git a/arch/Config.in.arc b/arch/Config.in.arc index c65bb01f1f4f..284951b82cee 100644 --- a/arch/Config.in.arc +++ b/arch/Config.in.arc @@ -11,13 +11,19 @@ config BR2_arc750d config BR2_arc770d bool "ARC 770D" -config BR2_archs38 +config BR2_archs bool "ARC HS38" help Generic ARC HS capable of running Linux, i.e. with MMU, - caches and multiplier. Also it corresponds to the default + caches and 32-bit multiplier. Also it corresponds to the default configuration in older GNU toolchain versions. +config BR2_archs38 + bool "ARC HS38 with 64-bit mpy" + help + Fully featured ARC HS capable of running Linux, i.e. with MMU, + caches and 64-bit multiplier. + If you're not sure which version of ARC HS core you build for keep this one. @@ -43,7 +49,7 @@ endchoice # Choice of atomic instructions presence config BR2_ARC_ATOMIC_EXT bool "Atomic extension (LLOCK/SCOND instructions)" - default y if BR2_arc770d || BR2_archs38 || BR2_archs38_full || BR2_archs4x_rel31 + default y if BR2_arc770d || BR2_archs || BR2_archs38 || BR2_archs38_full || BR2_archs4x_rel31 config BR2_ARCH default "arc" if BR2_arcle @@ -60,13 +66,14 @@ config BR2_ENDIAN config BR2_GCC_TARGET_CPU default "arc700" if BR2_arc750d default "arc700" if BR2_arc770d - default "archs" if BR2_archs38 + default "archs" if BR2_archs + default "hs38" if BR2_archs38 default "hs38_linux" if BR2_archs38_full default "hs4x_rel31" if BR2_archs4x_rel31 config BR2_READELF_ARCH_NAME default "ARCompact" if BR2_arc750d || BR2_arc770d - default "ARCv2" if BR2_archs38 || BR2_archs38_full || BR2_archs4x_rel31 + default "ARCv2" if BR2_archs || BR2_archs38 || BR2_archs38_full || BR2_archs4x_rel31 choice prompt "MMU Page Size" @@ -86,7 +93,7 @@ choice config BR2_ARC_PAGE_SIZE_4K bool "4KB" - depends on BR2_arc770d || BR2_archs38 || BR2_archs38_full || BR2_archs4x_rel31 + depends on BR2_arc770d || BR2_archs || BR2_archs38 || BR2_archs38_full || BR2_archs4x_rel31 config BR2_ARC_PAGE_SIZE_8K bool "8KB" @@ -96,7 +103,7 @@ config BR2_ARC_PAGE_SIZE_8K config BR2_ARC_PAGE_SIZE_16K bool "16KB" - depends on BR2_arc770d || BR2_archs38 || BR2_archs38_full || BR2_archs4x_rel31 + depends on BR2_arc770d || BR2_archs || BR2_archs38 || BR2_archs38_full || BR2_archs4x_rel31 endchoice
This corresponds to -mcu=hs38 with mpy-option=9 (64-bit multiplier) Signed-off-by: Vineet Gupta <vgupta@synopsys.com> --- arch/Config.in.arc | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)