| Message ID | 1445252572.4143.12.camel@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
On 19-10-15 13:02, Kinsella, Ray wrote: > The Intel X1000 is the Pentium class microprocessor that ships with Galileo > Gen 1/2. This patch adds changes to arch and toolchain-wrapper to omit the lock > prefix for the X1000. > > Signed-off-by: Ray Kinsella <ray.kinsella@intel.com> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Stylistic comments below. > --- > arch/Config.in.x86 | 10 ++++++++++ > toolchain/toolchain-wrapper.c | 3 +++ > toolchain/toolchain-wrapper.mk | 4 ++++ > 3 files changed, 17 insertions(+) > > diff --git a/arch/Config.in.x86 b/arch/Config.in.x86 > index 43f6abc..28b8adf 100644 > --- a/arch/Config.in.x86 > +++ b/arch/Config.in.x86 > @@ -34,6 +34,14 @@ config BR2_x86_i486 > config BR2_x86_i586 > bool "i586" > depends on !BR2_x86_64 > +config BR2_x86_x1000 > + bool "x1000" > + depends on !BR2_x86_64 > + help > + The Intel X1000 is a Pentium class microprocessor in the Quark > + (sub-Atom) Product Line. The X1000 has a bug on the lock prefix > + requiring that prefix must be stripped at build time. See > + https://en.wikipedia.org/wiki/Intel_Quark Now that it has become a full config option including help text, perhaps we should empty lines around it like we do elsewhere. OTOH that breaks the current flow of the file... Also, perhaps the URL should be on a separate line like we do for packages. Regards, Arnout > config BR2_x86_i686 > bool "i686" > depends on !BR2_x86_64 [snip]
Ray, On Mon, 19 Oct 2015 11:02:52 +0000, Kinsella, Ray wrote: > The Intel X1000 is the Pentium class microprocessor that ships with Galileo > Gen 1/2. This patch adds changes to arch and toolchain-wrapper to omit the lock > prefix for the X1000. > > Signed-off-by: Ray Kinsella <ray.kinsella@intel.com> I've applied your patch, after doing some small tweaks (see below). > --- > arch/Config.in.x86 | 10 ++++++++++ > toolchain/toolchain-wrapper.c | 3 +++ > toolchain/toolchain-wrapper.mk | 4 ++++ > 3 files changed, 17 insertions(+) > > diff --git a/arch/Config.in.x86 b/arch/Config.in.x86 > index 43f6abc..28b8adf 100644 > --- a/arch/Config.in.x86 > +++ b/arch/Config.in.x86 > @@ -34,6 +34,14 @@ config BR2_x86_i486 > config BR2_x86_i586 > bool "i586" > depends on !BR2_x86_64 > +config BR2_x86_x1000 > + bool "x1000" > + depends on !BR2_x86_64 > + help > + The Intel X1000 is a Pentium class microprocessor in the Quark Indentation for help text is one tab + two spaces. Also, there was a trailing space on this line. > + (sub-Atom) Product Line. The X1000 has a bug on the lock prefix > + requiring that prefix must be stripped at build time. See > + https://en.wikipedia.org/wiki/Intel_Quark And the lines were a bit too long, so I rewrapped them. > +#ifdef BR_OMIT_LOCK_PREFIX > + "-Wa,-momit-lock-prefix=yes", There was also a trailing space here. And finally, I've changed the commit title to: arch/x86: add support for Intel x1000 We indeed like to have some sort of "category" prefix for all patches. Generally it's just the package name (when the patch is touching a package), but we also try to do the same for patches touching other things as well. Thanks! Thomas
Hi Thomas, Thanks for that. I took a quick check of my board/galileo patchset. It is free for errant trailing spaces and has the required "category" prefix. However, I had a question on tabbing, for files like grub.cfg or genimage.cfg. Is there a convention? are tabs _or_ spaces fine? Thanks, Ray K On Tue, 2015-10-20 at 10:06 +0200, Thomas Petazzoni wrote: > Ray, > > On Mon, 19 Oct 2015 11:02:52 +0000, Kinsella, Ray wrote: > > The Intel X1000 is the Pentium class microprocessor that ships with Galileo > > Gen 1/2. This patch adds changes to arch and toolchain-wrapper to omit the lock > > prefix for the X1000. > > > > Signed-off-by: Ray Kinsella <ray.kinsella@intel.com> > > I've applied your patch, after doing some small tweaks (see below). > > > --- > > arch/Config.in.x86 | 10 ++++++++++ > > toolchain/toolchain-wrapper.c | 3 +++ > > toolchain/toolchain-wrapper.mk | 4 ++++ > > 3 files changed, 17 insertions(+) > > > > diff --git a/arch/Config.in.x86 b/arch/Config.in.x86 > > index 43f6abc..28b8adf 100644 > > --- a/arch/Config.in.x86 > > +++ b/arch/Config.in.x86 > > @@ -34,6 +34,14 @@ config BR2_x86_i486 > > config BR2_x86_i586 > > bool "i586" > > depends on !BR2_x86_64 > > +config BR2_x86_x1000 > > + bool "x1000" > > + depends on !BR2_x86_64 > > + help > > + The Intel X1000 is a Pentium class microprocessor in the Quark > > Indentation for help text is one tab + two spaces. Also, there was a > trailing space on this line. > > > + (sub-Atom) Product Line. The X1000 has a bug on the lock prefix > > + requiring that prefix must be stripped at build time. See > > + https://en.wikipedia.org/wiki/Intel_Quark > > And the lines were a bit too long, so I rewrapped them. > > > +#ifdef BR_OMIT_LOCK_PREFIX > > + "-Wa,-momit-lock-prefix=yes", > > There was also a trailing space here. > > And finally, I've changed the commit title to: > > arch/x86: add support for Intel x1000 > > We indeed like to have some sort of "category" prefix for all patches. > Generally it's just the package name (when the patch is touching a > package), but we also try to do the same for patches touching other > things as well. > > Thanks! > > Thomas
Dear Kinsella, Ray, On Tue, 20 Oct 2015 09:28:27 +0000, Kinsella, Ray wrote: > I took a quick check of my board/galileo patchset. > It is free for errant trailing spaces and has the required "category" > prefix. Thanks for checking. > However, I had a question on tabbing, for files like grub.cfg or > genimage.cfg. > Is there a convention? are tabs _or_ spaces fine? I think we more commonly use tabs. Thanks, Thomas
diff --git a/arch/Config.in.x86 b/arch/Config.in.x86 index 43f6abc..28b8adf 100644 --- a/arch/Config.in.x86 +++ b/arch/Config.in.x86 @@ -34,6 +34,14 @@ config BR2_x86_i486 config BR2_x86_i586 bool "i586" depends on !BR2_x86_64 +config BR2_x86_x1000 + bool "x1000" + depends on !BR2_x86_64 + help + The Intel X1000 is a Pentium class microprocessor in the Quark + (sub-Atom) Product Line. The X1000 has a bug on the lock prefix + requiring that prefix must be stripped at build time. See + https://en.wikipedia.org/wiki/Intel_Quark config BR2_x86_i686 bool "i686" depends on !BR2_x86_64 @@ -202,6 +210,7 @@ config BR2_ARCH default "i386" if BR2_x86_i386 default "i486" if BR2_x86_i486 default "i586" if BR2_x86_i586 + default "i586" if BR2_x86_x1000 default "i586" if BR2_x86_pentium_mmx default "i586" if BR2_x86_geode default "i586" if BR2_x86_c3 @@ -240,6 +249,7 @@ config BR2_GCC_TARGET_ARCH default "i386" if BR2_x86_i386 default "i486" if BR2_x86_i486 default "i586" if BR2_x86_i586 + default "i586" if BR2_x86_x1000 default "pentium-mmx" if BR2_x86_pentium_mmx default "i686" if BR2_x86_i686 default "pentiumpro" if BR2_x86_pentiumpro diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c index 16a3d78..f84bbe5 100644 --- a/toolchain/toolchain-wrapper.c +++ b/toolchain/toolchain-wrapper.c @@ -63,6 +63,9 @@ static char *predef_args[] = { #ifdef BR_64 "-m64", #endif +#ifdef BR_OMIT_LOCK_PREFIX + "-Wa,-momit-lock-prefix=yes", +#endif #ifdef BR_BINFMT_FLAT "-Wl,-elf2flt", #endif diff --git a/toolchain/toolchain-wrapper.mk b/toolchain/toolchain-wrapper.mk index c78363a..eba2b38 100644 --- a/toolchain/toolchain-wrapper.mk +++ b/toolchain/toolchain-wrapper.mk @@ -20,6 +20,10 @@ ifeq ($(BR2_CCACHE),y) TOOLCHAIN_WRAPPER_ARGS += -DBR_CCACHE endif +ifeq ($(BR2_x86_x1000),y) +TOOLCHAIN_WRAPPER_ARGS += -DBR_OMIT_LOCK_PREFIX +endif + ifeq ($(BR2_CCACHE_USE_BASEDIR),y) TOOLCHAIN_WRAPPER_ARGS += -DBR_CCACHE_BASEDIR='"$(BASE_DIR)"' endif
The Intel X1000 is the Pentium class microprocessor that ships with Galileo Gen 1/2. This patch adds changes to arch and toolchain-wrapper to omit the lock prefix for the X1000. Signed-off-by: Ray Kinsella <ray.kinsella@intel.com> --- arch/Config.in.x86 | 10 ++++++++++ toolchain/toolchain-wrapper.c | 3 +++ toolchain/toolchain-wrapper.mk | 4 ++++ 3 files changed, 17 insertions(+)