| Message ID | 20200122150147.25709-4-wolfgang.wallner@br-automation.com |
|---|---|
| State | Accepted |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | x86: Move ITSS from Apollo Lake to a more generic location | expand |
On Wed, 22 Jan 2020 at 08:01, Wolfgang Wallner <wolfgang.wallner@br-automation.com> wrote: > > The Interrupt Timer Subsystem (ITSS) is not specific to Apollo Lake, so > move it to a common location within arch/x86. > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com> > --- > At the moment, this commit enables building of itss.o unconditionally. > which is a bad idea I guess. > What is the preferred way to handle this? > Should I add a kconfig option e.g. in arch/x86/Kconfig? > > arch/x86/cpu/apollolake/Makefile | 1 - > arch/x86/cpu/intel_common/Makefile | 1 + > arch/x86/cpu/{apollolake => intel_common}/itss.c | 0 > 3 files changed, 1 insertion(+), 1 deletion(-) > rename arch/x86/cpu/{apollolake => intel_common}/itss.c (100%) Reviewed-by: Simon Glass <sjg@chromium.org>
Hello Simon, -----"Simon Glass" <sjg@chromium.org> schrieb: ----- > On Wed, 22 Jan 2020 at 08:01, Wolfgang Wallner > <wolfgang.wallner@br-automation.com> wrote: > > > > The Interrupt Timer Subsystem (ITSS) is not specific to Apollo Lake, so > > move it to a common location within arch/x86. > > > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com> > > --- > > At the moment, this commit enables building of itss.o unconditionally. > > which is a bad idea I guess. > > What is the preferred way to handle this? > > Should I add a kconfig option e.g. in arch/x86/Kconfig? Thank you for reviewing. But I'm still not sure how to handle the question mentioned above, and would like to ask for further feedback. My current idea would be to add a new kconfig option CONFIG_ITSS in arch/x86/Kconfig to control building of this driver (which would be automatically implied via 'select' when the build target is Apollo Lake). Does that make sense? If so, I would include the necessary modifications to kconfig in the next version of this patch. > > > > arch/x86/cpu/apollolake/Makefile | 1 - > > arch/x86/cpu/intel_common/Makefile | 1 + > > arch/x86/cpu/{apollolake => intel_common}/itss.c | 0 > > 3 files changed, 1 insertion(+), 1 deletion(-) > > rename arch/x86/cpu/{apollolake => intel_common}/itss.c (100%) > > Reviewed-by: Simon Glass <sjg@chromium.org> regards, Wolfgang
Hi Wolfgang, On Fri, 31 Jan 2020 at 00:47, Wolfgang Wallner <wolfgang.wallner@br-automation.com> wrote: > > Hello Simon, > > -----"Simon Glass" <sjg@chromium.org> schrieb: ----- > > On Wed, 22 Jan 2020 at 08:01, Wolfgang Wallner > > <wolfgang.wallner@br-automation.com> wrote: > > > > > > The Interrupt Timer Subsystem (ITSS) is not specific to Apollo Lake, so > > > move it to a common location within arch/x86. > > > > > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com> > > > --- > > > At the moment, this commit enables building of itss.o unconditionally. > > > which is a bad idea I guess. > > > What is the preferred way to handle this? > > > Should I add a kconfig option e.g. in arch/x86/Kconfig? > > Thank you for reviewing. > > But I'm still not sure how to handle the question mentioned above, and > would like to ask for further feedback. > > My current idea would be to add a new kconfig option CONFIG_ITSS in > arch/x86/Kconfig to control building of this driver (which would be > automatically implied via 'select' when the build target is Apollo Lake). > > Does that make sense? If so, I would include the necessary modifications > to kconfig in the next version of this patch. Yes that sounds fine to me. > > > > > > > arch/x86/cpu/apollolake/Makefile | 1 - > > > arch/x86/cpu/intel_common/Makefile | 1 + > > > arch/x86/cpu/{apollolake => intel_common}/itss.c | 0 > > > 3 files changed, 1 insertion(+), 1 deletion(-) > > > rename arch/x86/cpu/{apollolake => intel_common}/itss.c (100%) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > regards, Wolfgang Regards, Simon
On Wed, Jan 22, 2020 at 11:01 PM Wolfgang Wallner <wolfgang.wallner@br-automation.com> wrote: > > The Interrupt Timer Subsystem (ITSS) is not specific to Apollo Lake, so > move it to a common location within arch/x86. > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com> > --- > At the moment, this commit enables building of itss.o unconditionally. > which is a bad idea I guess. > What is the preferred way to handle this? > Should I add a kconfig option e.g. in arch/x86/Kconfig? > > arch/x86/cpu/apollolake/Makefile | 1 - > arch/x86/cpu/intel_common/Makefile | 1 + > arch/x86/cpu/{apollolake => intel_common}/itss.c | 0 > 3 files changed, 1 insertion(+), 1 deletion(-) > rename arch/x86/cpu/{apollolake => intel_common}/itss.c (100%) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
On Mon, Feb 3, 2020 at 5:33 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Wed, Jan 22, 2020 at 11:01 PM Wolfgang Wallner > <wolfgang.wallner@br-automation.com> wrote: > > > > The Interrupt Timer Subsystem (ITSS) is not specific to Apollo Lake, so > > move it to a common location within arch/x86. > > > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com> > > --- > > At the moment, this commit enables building of itss.o unconditionally. > > which is a bad idea I guess. > > What is the preferred way to handle this? > > Should I add a kconfig option e.g. in arch/x86/Kconfig? > > > > arch/x86/cpu/apollolake/Makefile | 1 - > > arch/x86/cpu/intel_common/Makefile | 1 + > > arch/x86/cpu/{apollolake => intel_common}/itss.c | 0 > > 3 files changed, 1 insertion(+), 1 deletion(-) > > rename arch/x86/cpu/{apollolake => intel_common}/itss.c (100%) > > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> applied to u-boot-x86, thanks!
Hi Bin, > -----"Bin Meng" <bmeng.cn@gmail.com> schrieb: ----- > applied to u-boot-x86, thanks! I intended to send a V2 of this patch, see discussion at [1]. As you have pulled that patch now, should I send the proposed modifications to kconfig as a separate patch instead of including it in a V2 of this one? regards, Wolfgang [1] https://lists.denx.de/pipermail/u-boot/2020-January/398711.html
Hi Wolfgang, On Mon, Feb 3, 2020 at 6:05 PM Wolfgang Wallner <wolfgang.wallner@br-automation.com> wrote: > > Hi Bin, > > > -----"Bin Meng" <bmeng.cn@gmail.com> schrieb: ----- > > applied to u-boot-x86, thanks! > > I intended to send a V2 of this patch, see discussion at [1]. > As you have pulled that patch now, should I send the proposed modifications > to kconfig as a separate patch instead of including it in a V2 of this one? > It's OK to send a separate patch for the Kconfig option. Thanks! Regards, Bin
diff --git a/arch/x86/cpu/apollolake/Makefile b/arch/x86/cpu/apollolake/Makefile index 1760df54d8..f99f2c6473 100644 --- a/arch/x86/cpu/apollolake/Makefile +++ b/arch/x86/cpu/apollolake/Makefile @@ -19,7 +19,6 @@ obj-y += fsp_s.o endif obj-y += hostbridge.o -obj-y += itss.o obj-y += lpc.o obj-y += p2sb.o obj-y += pch.o diff --git a/arch/x86/cpu/intel_common/Makefile b/arch/x86/cpu/intel_common/Makefile index cc4e1c962b..266e6e26fa 100644 --- a/arch/x86/cpu/intel_common/Makefile +++ b/arch/x86/cpu/intel_common/Makefile @@ -27,6 +27,7 @@ obj-y += microcode.o endif endif obj-y += pch.o +obj-y += itss.o ifdef CONFIG_SPL ifndef CONFIG_SPL_BUILD diff --git a/arch/x86/cpu/apollolake/itss.c b/arch/x86/cpu/intel_common/itss.c similarity index 100% rename from arch/x86/cpu/apollolake/itss.c rename to arch/x86/cpu/intel_common/itss.c
The Interrupt Timer Subsystem (ITSS) is not specific to Apollo Lake, so move it to a common location within arch/x86. Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com> --- At the moment, this commit enables building of itss.o unconditionally. which is a bad idea I guess. What is the preferred way to handle this? Should I add a kconfig option e.g. in arch/x86/Kconfig? arch/x86/cpu/apollolake/Makefile | 1 - arch/x86/cpu/intel_common/Makefile | 1 + arch/x86/cpu/{apollolake => intel_common}/itss.c | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename arch/x86/cpu/{apollolake => intel_common}/itss.c (100%)