[PATCH 3/9] x86/moorestown: add moorestown platform flags

[PATCH 3/9] x86/moorestown: add moorestown platform flags

[Pan, Jacob jun]

>From 365e3a59c34055490f137af2da281d4305535237 Mon Sep 17 00:00:00 2001
From: Jacob Pan <jacob.jun.pan@intel.com>
Date: Thu, 11 Jun 2009 09:42:42 -0700
Subject: [PATCH] x86/moorestown: add moorestown platform flags

This patch adds some new features to the x86 platform feature flags.

These new features are first introduced in Intel Moorestown platform but
may continue to be present in the next generations.

Feature flags are initialized based on HW subarchitecture ID provided
by the FW via boot protocol.

Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
---
 arch/x86/Kconfig                        |   13 ++++++++++++-
 arch/x86/include/asm/platform_feature.h |    2 ++
 arch/x86/kernel/platform_info.c         |   12 +++++++++++-
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index d1430ef..c48c291 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1658,7 +1658,18 @@ config CMDLINE_OVERRIDE
 
 	  This is used to work around broken boot loaders.  This should
 	  be set to 'N' under normal conditions.
-
+config MRST
+	bool "Moorestown MID platform"
+	default n
+	depends on X86_32
+	select SFI
+	help
+	  Moorestown is Intel's Low Power Intel Architecture (LPIA) based Moblin
+	  Internet Device(MID) platform. Moorestown consists of two chips:
+	  Lincroft (CPU core, graphics, and memory controller) and Langwell IOH.
+	  Unlike standard x86 PCs, Moorestown does not have many legacy devices
+	  nor standard legacy replacement devices/features. e.g. Moorestown does
+	  not contain i8259, i8254, HPET, legacy BIOS, most of the io ports.
 endmenu
 
 config ARCH_ENABLE_MEMORY_HOTPLUG
diff --git a/arch/x86/include/asm/platform_feature.h b/arch/x86/include/asm/platform_feature.h
index bcadda5..026bdc7 100644
--- a/arch/x86/include/asm/platform_feature.h
+++ b/arch/x86/include/asm/platform_feature.h
@@ -45,6 +45,8 @@
 #define X86_PLATFORM_FEATURE_ACPI		(0*32+8) /* has ACPI support */
 #define X86_PLATFORM_FEATURE_SFI		(0*32+9) /* has SFI support */
 #define X86_PLATFORM_FEATURE_8042		(0*32+10) /* i8042 KBC */
+#define X86_PLATFORM_FEATURE_APBT		(0*32+11) /* APB timer */
+#define X86_PLATFORM_FEATURE_VRTC		(0*32+12) /* virtual RTC */
 
 extern __u32 platform_feature[N_PLATFORM_CAPINTS];
 extern const char *const
diff --git a/arch/x86/kernel/platform_info.c b/arch/x86/kernel/platform_info.c
index b39898a..3021ea9 100644
--- a/arch/x86/kernel/platform_info.c
+++ b/arch/x86/kernel/platform_info.c
@@ -105,7 +105,17 @@ static int __init sysfs_platforminfo_init(void)
 }
 arch_initcall(sysfs_platforminfo_init);
 
+/* Initialize a set of default feature flags based on subarch IDs
+ * Currently, only MRST platform has non-X86 PC standard feature set.
+ */
 void platform_feature_init_default(int subarch_id)
 {
-	printk(KERN_INFO "Use default X86 platform feature set\n");
+	if ((subarch_id >= 0) && (subarch_id < N_X86_SUBARCHS)) {
+		if (subarch_id == X86_SUBARCH_MRST) {
+			setup_mrst_default_feature();
+			return;
+		}
+	} else {
+		printk(KERN_INFO "Use default X86 platform feature set\n");
+	}
 }
-- 
1.5.6.5

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Pan, Jacob jun <jacob.jun.pan@intel.com> wrote:

> >From 365e3a59c34055490f137af2da281d4305535237 Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@intel.com>
> Date: Thu, 11 Jun 2009 09:42:42 -0700
> Subject: [PATCH] x86/moorestown: add moorestown platform flags
> 
> This patch adds some new features to the x86 platform feature flags.
> 
> These new features are first introduced in Intel Moorestown platform but
> may continue to be present in the next generations.
> 
> Feature flags are initialized based on HW subarchitecture ID provided
> by the FW via boot protocol.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
> ---
>  arch/x86/Kconfig                        |   13 ++++++++++++-
>  arch/x86/include/asm/platform_feature.h |    2 ++
>  arch/x86/kernel/platform_info.c         |   12 +++++++++++-
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index d1430ef..c48c291 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1658,7 +1658,18 @@ config CMDLINE_OVERRIDE
>  
>  	  This is used to work around broken boot loaders.  This should
>  	  be set to 'N' under normal conditions.
> -
> +config MRST
> +	bool "Moorestown MID platform"
> +	default n
> +	depends on X86_32
> +	select SFI
> +	help
> +	  Moorestown is Intel's Low Power Intel Architecture (LPIA) based Moblin
> +	  Internet Device(MID) platform. Moorestown consists of two chips:
> +	  Lincroft (CPU core, graphics, and memory controller) and Langwell IOH.
> +	  Unlike standard x86 PCs, Moorestown does not have many legacy devices
> +	  nor standard legacy replacement devices/features. e.g. Moorestown does
> +	  not contain i8259, i8254, HPET, legacy BIOS, most of the io ports.
>  endmenu
>  
>  config ARCH_ENABLE_MEMORY_HOTPLUG
> diff --git a/arch/x86/include/asm/platform_feature.h b/arch/x86/include/asm/platform_feature.h
> index bcadda5..026bdc7 100644
> --- a/arch/x86/include/asm/platform_feature.h
> +++ b/arch/x86/include/asm/platform_feature.h
> @@ -45,6 +45,8 @@
>  #define X86_PLATFORM_FEATURE_ACPI		(0*32+8) /* has ACPI support */
>  #define X86_PLATFORM_FEATURE_SFI		(0*32+9) /* has SFI support */
>  #define X86_PLATFORM_FEATURE_8042		(0*32+10) /* i8042 KBC */
> +#define X86_PLATFORM_FEATURE_APBT		(0*32+11) /* APB timer */
> +#define X86_PLATFORM_FEATURE_VRTC		(0*32+12) /* virtual RTC */
>  
>  extern __u32 platform_feature[N_PLATFORM_CAPINTS];
>  extern const char *const
> diff --git a/arch/x86/kernel/platform_info.c b/arch/x86/kernel/platform_info.c
> index b39898a..3021ea9 100644
> --- a/arch/x86/kernel/platform_info.c
> +++ b/arch/x86/kernel/platform_info.c
> @@ -105,7 +105,17 @@ static int __init sysfs_platforminfo_init(void)
>  }
>  arch_initcall(sysfs_platforminfo_init);
>  
> +/* Initialize a set of default feature flags based on subarch IDs
> + * Currently, only MRST platform has non-X86 PC standard feature set.
> + */

please use the customary comment style:

  /*
   * Comment .....
   * ...... goes here:
   */

specified in Documentation/CodingStyle.

>  void platform_feature_init_default(int subarch_id)
>  {
> -	printk(KERN_INFO "Use default X86 platform feature set\n");
> +	if ((subarch_id >= 0) && (subarch_id < N_X86_SUBARCHS)) {
> +		if (subarch_id == X86_SUBARCH_MRST) {
> +			setup_mrst_default_feature();
> +			return;
> +		}
> +	} else {
> +		printk(KERN_INFO "Use default X86 platform feature set\n");
> +	}
>  }

Why dont we have some clean and robust PCI config space based 
enumeration instead of this boot ID based thing?

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Alan Cox]

> Why dont we have some clean and robust PCI config space based 
> enumeration instead of this boot ID based thing?

That strikes me as a rather dumb question given that 

- Embedded x86 like devices are going to regularly occur without any PCI
- You need to know the platform in order to know how to access any PCI
  bus that may or may not hypothetically exist.

Take a look at how ARM and PPC address this. They do it via platform
features and descriptors for good reason.

One thing PPC does nice is to encapsulate pretty much all of it into a
struct of function pointers for each platform to keep the if conditionals
down. The ARM file layout of arch/foo/platform/bar might also be a good
idea before mrst and olpc and other stuff like this starts to make the
main x86 directory hard to read ?

Alan

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Why dont we have some clean and robust PCI config space based 
> > enumeration instead of this boot ID based thing?
> 
> That strikes me as a rather dumb question given that

It's an entirely legitimate question, given that many other current 
modern subarchitectures detect themselves based on PCI config space 
early accesses or (sometimes) BIOS data structures - and that both 
of those methods are better than using boot flags.

> - Embedded x86 like devices are going to regularly occur without 
> any PCI

This proposed Intel subarchitecture comes with PCI support, 
obviously.

> - You need to know the platform in order to know how to access any
>   PCI bus that may or may not hypothetically exist.

Uhm, not really.

Have a look at arch/x86/kernel/early-quirks.c. All you generally 
need to know is a PCI ID that sits on the root bus.

Early PCI ID checks are typical and robust way to identify 'weird' 
subarchitectures. Sometimes we do it via BIOS data structures. Only 
as a last option do we want to use boot loader mechanisms - it's the 
most inflexible one really.

Furtherore, Moorestown comes with SFI and there sure can be a BIOS 
table that describes the platform properly. We can read such tables 
very early during bootup, well before platform devices are set up.

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Alan Cox]

> It's an entirely legitimate question, given that many other current 
> modern subarchitectures detect themselves based on PCI config space 
> early accesses or (sometimes) BIOS data structures - and that both 
> of those methods are better than using boot flags.

The flags are passed by the boot loader which is the one thing that knows
what the platform is only deeply embedded hardware. See the ARM and PPC
ports.

> > - Embedded x86 like devices are going to regularly occur without 
> > any PCI
> 
> This proposed Intel subarchitecture comes with PCI support, 
> obviously.

And this is relevant to the fact there will be systems without PCI how ?

> > - You need to know the platform in order to know how to access any
> >   PCI bus that may or may not hypothetically exist.
> 
> Uhm, not really.

Uhm yes really

> Have a look at arch/x86/kernel/early-quirks.c. All you generally 
> need to know is a PCI ID that sits on the root bus.

How will you probe the root bus without knowing what the PCI
configuration mechanism is for your platform

> Early PCI ID checks are typical and robust way to identify 'weird' 
> subarchitectures. Sometimes we do it via BIOS data structures. Only 
> as a last option do we want to use boot loader mechanisms - it's the 
> most inflexible one really.

SFI doesn't indicate MRST
> 
> Furtherore, Moorestown comes with SFI and there sure can be a BIOS 
> table that describes the platform properly. We can read such tables 
> very early during bootup, well before platform devices are set up.

But they don't tell you what the platform is. Again see every platform we
currently support which has deeply embedded forms. There is a reason they
went that way and it includes fifteen years of finding out which ways
don't work for all cases. For example if you don't have standard PC like
firmware and you go looking for SFI or ACPI will your box stay up ?

It's also kind of irrelevant to the flags anyway - you need the boot
flags whether the platform comes from the bootloader or divine
intervention somewhere early in probing.

There is a more general question which is whether the flags are better
than the PPC style approach of having a machine vector and putting them
in that so you just do

	platform_ops = platform_ops_table[our_platform]

and use
	platform_ops->

for the various methods such as EBDA and which timer methods to use

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > It's an entirely legitimate question, given that many other 
> > current modern subarchitectures detect themselves based on PCI 
> > config space early accesses or (sometimes) BIOS data structures 
> > - and that both of those methods are better than using boot 
> > flags.
> 
> The flags are passed by the boot loader which is the one thing 
> that knows what the platform is only deeply embedded hardware. See 
> the ARM and PPC ports.

And? There's an obvious quality difference between various platform 
enumeration methods - and we strive for the highest quality methods.

Using boot flags is one of the lowest quality enumeration methods 
and the fact that there's precedence for it in other architectures 
is not a technical reason to make the same mistakes on x86 too.

Especially here where there's two other enumeration methods easily 
available: SFI and PCI. Any of those suffices.

> > > - Embedded x86 like devices are going to regularly occur 
> > > without any PCI
> > 
> > This proposed Intel subarchitecture comes with PCI support, 
> > obviously.
> 
> And this is relevant to the fact there will be systems without PCI 
> how ?

It is obviously relevant to the fact that the proposed patches here 
(on which i commented) come for a platform with PCI support. Or do 
you claim that Intel submitted this patch-set for PCI-less MIDs? 
(this was not mentioned anywhere in the patches)

> > > - You need to know the platform in order to know how to access 
> > >   any PCI bus that may or may not hypothetically exist.
> > 
> > Uhm, not really.
> 
> Uhm yes really
> 
> > Have a look at arch/x86/kernel/early-quirks.c. All you generally 
> > need to know is a PCI ID that sits on the root bus.
> 
> How will you probe the root bus without knowing what the PCI 
> configuration mechanism is for your platform

That's very simple really - there's basically a very low number of 
PCI configuration mechanisms/ports on x86, and the MIDs have no 
reason to depart from that standard. There's two that matter in 
practice, and it can all be safely probed.

PCI is really essential to any modern architecture (precisely 
because it's standard and well-known) and i doubt Intel will try to 
engineer out PCI from its systems. If it happens i will comment on 
that kind of braindamage in due course.

But, PCI IDs dont _have_ to be used - there's ample other 
environmental space available via SFI, ACPI or EFI or old-and-proven 
EBDA.

> > Early PCI ID checks are typical and robust way to identify 
> > 'weird' subarchitectures. Sometimes we do it via BIOS data 
> > structures. Only as a last option do we want to use boot loader 
> > mechanisms - it's the most inflexible one really.
> 
> SFI doesn't indicate MRST

So do you claim that this particular patchset supports systems with 
no SFI, no ACPI, just plain bootloader flags?

> > Furtherore, Moorestown comes with SFI and there sure can be a 
> > BIOS table that describes the platform properly. We can read 
> > such tables very early during bootup, well before platform 
> > devices are set up.
> 
> But they don't tell you what the platform is. Again see every 
> platform we currently support which has deeply embedded forms. 
> There is a reason they went that way and it includes fifteen years 
> of finding out which ways don't work for all cases. For example if 
> you don't have standard PC like firmware and you go looking for 
> SFI or ACPI will your box stay up ?

Do you claim that the Intel patchset here deals with systems that 
have no SFI and no ACPI? My question was very specific to this 
patch-set. Bootloader flags suck because they add a needless 
dimension to the already complex boot protocol. Use existing 
mechanisms instead - it's really easy and has other advantages as 
well.

Non-x86 ultra-embedded might use crappier techniques but i'd expect 
Intel has the resources to do better here. Using standard hardware 
or firmware interfaces for that is far better in the x86 space and 
we have no reason to depart from that really.

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Alan Cox]

> > The flags are passed by the boot loader which is the one thing 
> > that knows what the platform is only deeply embedded hardware. See 
> > the ARM and PPC ports.
> 
> And? There's an obvious quality difference between various platform 
> enumeration methods - and we strive for the highest quality methods.

Good the boot loader knows precisely what it is running on

> Using boot flags is one of the lowest quality enumeration methods 

It's probably the most reliable. If you don't believe so then provide
data to back your assertion

> and the fact that there's precedence for it in other architectures 
> is not a technical reason to make the same mistakes on x86 too.

How about "they tried other methods and they didn't work"

> It is obviously relevant to the fact that the proposed patches here 
> (on which i commented) come for a platform with PCI support. Or do 
> you claim that Intel submitted this patch-set for PCI-less MIDs? 
> (this was not mentioned anywhere in the patches)

I claimed nothing of the sort Ingo. I pointed out your approach doesn't
actually work except for the PCI case. WHich means it doesn't even work
on a generic 486 ISA bus PC. Is that a single chip embedded device using
a licenced core from one of the 486 vendors or an ISA PC .. how do you
tell.

> That's very simple really - there's basically a very low number of 
> PCI configuration mechanisms/ports on x86, and the MIDs have no 
> reason to depart from that standard. There's two that matter in 
> practice, and it can all be safely probed.

You try one, that I/O address isn't mapped and you get a fatal machine
check. Exactly why is that reliable ?

> PCI is really essential to any modern architecture (precisely 
> because it's standard and well-known) and i doubt Intel will try to 
> engineer out PCI from its systems. If it happens i will comment on 
> that kind of braindamage in due course.

PCI is already obsolete.

> But, PCI IDs dont _have_ to be used - there's ample other 
> environmental space available via SFI, ACPI or EFI or old-and-proven 
> EBDA.

You only have an EBDA on a PC type machine. You only have ACPI on a PC
type machien. You only have EFI ona PCI type machine. So how will you
look for them when the generic whacky BIOS windows for 16bit DOS
compatibility that hold these things might be ROM or random chunks of
main memory ?

> > SFI doesn't indicate MRST
> 
> So do you claim that this particular patchset supports systems with 
> no SFI, no ACPI, just plain bootloader flags?

If you have the bootloader pass the information then that is one source
of it. It doesn't need to be the only one.

You are also pointlessly entangling MRST and platform identification.

Please separate out the following in your thinking

- How you implement   int platform = gee_what_the_hell_am_i_running_on()

and "ok so I'm a voyager, now what"

> Do you claim that the Intel patchset here deals with systems that 
> have no SFI and no ACPI? My question was very specific to this 
> patch-set. 

I'm trying to sort out the broader question.

It's called architecture - planning for the future, design. If you want
to be utterly narrow minded you can NIH about every platform but x86 and
pretend hardware won't exist that doesn't fit your predetermined vision.
If you don't plan for the future you get to write the arch code every new
hardware platform. Which is fun and you really want to spend your life
re-inventing this every six months no ?

> Non-x86 ultra-embedded might use crappier techniques but i'd expect 
> Intel has the resources to do better here. Using standard hardware 

What makes you think x86 ultra-embedded is solely an Intel thing. OLPC
isn't Intel for one although its still quite PCish.

> or firmware interfaces for that is far better in the x86 space and 
> we have no reason to depart from that really.

It isn't about departing from that, it's about supporting the future. If
you untangle this into two questions

- How do I identify this platform

- How do I run on lots of platforms without the code becoming a
  mess

it might make more progress.

The important one right now is the second question. Whether you set the
platform on dip switches or by PCI bus probing the interface doesn't
change. Its a function that returns a number.

The hardware interfaces don't exist in x86 unlike most other
architectures so that does need addressing properly and can't be deferred.

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > > The flags are passed by the boot loader which is the one thing 
> > > that knows what the platform is only deeply embedded hardware. See 
> > > the ARM and PPC ports.
> > 
> > And? There's an obvious quality difference between various 
> > platform enumeration methods - and we strive for the highest 
> > quality methods.
> 
> Good the boot loader knows precisely what it is running on

That's a pretty bogus claim - on x86 a bootloader generally knows 
very little about 'what it is running on'. We do most of the 
enumeration in early platform code and retrieve information via 
standard BIOS interfaces.

> > Using boot flags is one of the lowest quality enumeration 
> > methods
> 
> It's probably the most reliable. If you don't believe so then 
> provide data to back your assertion

You are the one who is trying to do a change here really, so you 
should provide data to back your assertion that all existing x86 
enumeration methods are wrong and that modern x86 platforms from
now on should freely splinter into non-standard platforms along
boot flags.

The success of the PC platform was based on standardization and 
standard interfaces. Do you need data for that fact? ;-)

> > and the fact that there's precedence for it in other 
> > architectures is not a technical reason to make the same 
> > mistakes on x86 too.
> 
> How about "they tried other methods and they didn't work"

The thing is, you are trying to defend a v1 patch-set here that is 
really indefensible: it's ugly and deficient in numerous smaller and 
larger details. I outlined numerous deficiencies already - and i'll 
review v2 too to see what else is there to fix.

You outlined here various claims about how x86 should suddenly 
change into ARM or PPC to become 'successful' - but if this current 
patch-set is your attempt at that then it has failed spectacularly. 

Thanks,

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Alan Cox]

> That's a pretty bogus claim - on x86 a bootloader generally knows 
> very little about 'what it is running on'. We do most of the 
> enumeration in early platform code and retrieve information via 
> standard BIOS interfaces.

Stop thinking about existing x86 PC systems running grub for a bit.
Highly embedded systems usually boot from ROM firmware direct into the
kernel. They have a very good idea what they are running on because they
were flashed specifically for the device.

I'm not worried about PC/ISA v PC/MCA v Voyager v CBus v CompaqSMP v
Intel MP 1.4 SMP v PCI v PCI/X v PCI Express v OLPC v Voyager etc, we can
clearly tell all those apart via bus probing. But those are all basically
a PC.

> > How about "they tried other methods and they didn't work"
> 
> The thing is, you are trying to defend a v1 patch-set here that is 
> really indefensible: it's ugly and deficient in numerous smaller and 
> larger details. I outlined numerous deficiencies already - and i'll 
> review v2 too to see what else is there to fix.

No I'm trying to understand what you actually want the thing to look like.

Are we talking

/* Fixed struct not pointer for speed */
struct platform_ops platform_ops;
memcpy(platform_ops, platform_op_list[detect_platform_type()],
					sizeof(struct platform_ops));

platform->add_private_resources();
platform->timer_foo();
platfomr->timer_bar();

or

platform->init();
timer = platform->timer;

timer->begin()

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > That's a pretty bogus claim - on x86 a bootloader generally 
> > knows very little about 'what it is running on'. We do most of 
> > the enumeration in early platform code and retrieve information 
> > via standard BIOS interfaces.
> 
> Stop thinking about existing x86 PC systems running grub for a 
> bit. [...]

You are talking to the wrong person then i guess, i'm not going to 
ignore 95% of our installed base. ;-)

We will gladly take clean x86 patches that abstract away lowlevel 
details of x86 platforms, and have been taking them and have been 
writing them for a long time. If this patch-set can shape itself in 
such a way (as i requested), without hindering the common case, it 
is certainly welcome.

Generally, if you try to deviate from de facto standards then you 
better show a very good reason and much cleaner code than this 
deficient v1 patch-set.

Yes, platform abstraction is good if it's necessary and if it's done 
cleanly, and x86 certainly has a few things to learn in that area 
(it still has way too much platform spaghetti), but i'm not 
convinced here at all that it's necessary, and it's certainly not 
cleanly done at all. As i pointed it out in my review in detail, 
it's full of 'if (feature)' crap invading core platform code for 
example.

There's good examples as well, from the same quarters of Intel: for 
example i reviewed and commented on the related SFI patches a few 
days ago, and they looked distinctly clean and desirable.

Thanks,

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Alan Cox]

> We will gladly take clean x86 patches that abstract away lowlevel 
> details of x86 platforms, and have been taking them and have been 
> writing them for a long time. If this patch-set can shape itself in 
> such a way (as i requested), without hindering the common case, it 
> is certainly welcome.

Lets try again shall we. I'l repeat the relevant bits of the mail you
quoted bits from and ignored almost all of

> The thing is, you are trying to defend a v1 patch-set here that is 
> really indefensible: it's ugly and deficient in numerous smaller and 
> larger details. I outlined numerous deficiencies already - and i'll 
> review v2 too to see what else is there to fix.  

No I'm trying to understand what you actually want the thing to look like.
Are we talking

/* Fixed struct not pointer for speed */
struct platform_ops platform_ops;
memcpy(platform_ops, platform_op_list[detect_platform_type()],
					sizeof(struct platform_ops));

platform->add_private_resources();
platform->timer_foo();
platfomr->timer_bar();

or

platform->init();
timer = platform->timer;

timer->begin()

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > The thing is, you are trying to defend a v1 patch-set here that 
> > is really indefensible: it's ugly and deficient in numerous 
> > smaller and larger details. I outlined numerous deficiencies 
> > already - and i'll review v2 too to see what else is there to 
> > fix.
> 
> No I'm trying to understand what you actually want the thing to 
> look like.

It's a case by case thing and i pointed out a few specific 
directions in the review. The IO-APIC changes should probably go on 
top of Jeremy's IO-APIC driver-ization patches. They dont 
necessarily need their own IO-APIC driver (if the resulting line 
count increase is too much), but they should not wreck Jeremy's 
IO-APIC patches.

Bootup quirks that are small modificatons to existing PC 
initialization sequences should go into x86_quirks.

Timer related changes (the APB system timer) are mostly modular 
already by virtue of half of it being a clocksource and clockevents 
driver. The remaining bit of system timer handling should be 
abstracted out as a 'struct x86_system_timer' kind of structure, 
with ->init, ->timer_irq and ->shutdown functions.

[ Although it is beyond me why ABP was done - why wasnt HPET good
  enough? HPET can do per CPU clockevents too and it's just as 
  off-chip (and hence fundamentally slow) as ABP. ]

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > We will gladly take clean x86 patches that abstract away 
> > lowlevel details of x86 platforms, and have been taking them and 
> > have been writing them for a long time. If this patch-set can 
> > shape itself in such a way (as i requested), without hindering 
> > the common case, it is certainly welcome.
> 
> Lets try again shall we. I'l repeat the relevant bits of the mail 
> you quoted bits from and ignored almost all of

Below is my reply to your above points (already sent in another 
mail) - which is really just mostly re-hashing the (rather obvious 
looking) things that have been said before.

	Ingo

---------------------->

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > The thing is, you are trying to defend a v1 patch-set here that 
> > is really indefensible: it's ugly and deficient in numerous 
> > smaller and larger details. I outlined numerous deficiencies 
> > already - and i'll review v2 too to see what else is there to 
> > fix.
> 
> No I'm trying to understand what you actually want the thing to 
> look like.

It's a case by case thing and i pointed out a few specific 
directions in the review. The IO-APIC changes should probably go on 
top of Jeremy's IO-APIC driver-ization patches. They dont 
necessarily need their own IO-APIC driver (if the resulting line 
count increase is too much), but they should not wreck Jeremy's 
IO-APIC patches.

Bootup quirks that are small modificatons to existing PC 
initialization sequences should go into x86_quirks.

Timer related changes (the APB system timer) are mostly modular 
already by virtue of half of it being a clocksource and clockevents 
driver. The remaining bit of system timer handling should be 
abstracted out as a 'struct x86_system_timer' kind of structure, 
with ->init, ->timer_irq and ->shutdown functions.

[ Although it is beyond me why ABP was done - why wasnt HPET good
  enough? HPET can do per CPU clockevents too and it's just as 
  off-chip (and hence fundamentally slow) as ABP. ]

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> And? There's an obvious quality difference between various 
> platform enumeration methods - and we strive for the highest 
> quality methods.
> 
> Using boot flags is one of the lowest quality enumeration methods 
> and the fact that there's precedence for it in other architectures 
> is not a technical reason to make the same mistakes on x86 too.
> 
> Especially here where there's two other enumeration methods easily 
> available: SFI and PCI. Any of those suffices.

btw., the "I am MRST" bootstrap info is certainly doable via a 
bootloader flag as well, if this platform is _so_ deprived of basic 
PC features that it has no other channel of information. Especially 
if it has no BIOS and if the bootloader provides the memory map as 
well - which seems to be the case here.

I can understand the PCI ID space being potentially awkward (if many 
models with mismatching PCI IDs are planned) - so it's certainly 
possible that the only thing that remains in the end is the 
bootloader provided flag - no matter how sucky that may be.

So i am not trying to make a bigger deal out of this than it really 
deserves and this is not a showstopper in my eyes - but this is 
really an exceptional case and shouldnt be the glory model for how 
ultra embedded should be done on x86 ...

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Thomas Gleixner]

On Fri, 26 Jun 2009, Ingo Molnar wrote:
> [ Although it is beyond me why ABP was done - why wasnt HPET good
>   enough? HPET can do per CPU clockevents too and it's just as 
>   off-chip (and hence fundamentally slow) as ABP. ]

Welcome to the wonderful world of embedded systems. Just have a peek
into arch/[arm/powerpc/mips] to see what's coming up to us with full
force. I would not be surprised when we see an x86 system sharing the
device driver for i2c or whatever with an ARM SoC in the foreseable
future.

Thanks,

	tglx

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Jesse Barnes]

On Fri, 26 Jun 2009 09:19:55 +0200
Ingo Molnar <mingo@elte.hu> wrote:
> >  void platform_feature_init_default(int subarch_id)
> >  {
> > -	printk(KERN_INFO "Use default X86 platform feature set\n");
> > +	if ((subarch_id >= 0) && (subarch_id < N_X86_SUBARCHS)) {
> > +		if (subarch_id == X86_SUBARCH_MRST) {
> > +			setup_mrst_default_feature();
> > +			return;
> > +		}
> > +	} else {
> > +		printk(KERN_INFO "Use default X86 platform feature
> > set\n");
> > +	}
> >  }
> 
> Why dont we have some clean and robust PCI config space based 
> enumeration instead of this boot ID based thing?

MRST has special PCI support that depends on having the platform
identified already, so if you want to use something other than a flag
here, maybe we could use an SFI string instead.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Jesse Barnes]

On Fri, 26 Jun 2009 18:32:42 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 26 Jun 2009, Ingo Molnar wrote:
> > [ Although it is beyond me why ABP was done - why wasnt HPET good
> >   enough? HPET can do per CPU clockevents too and it's just as 
> >   off-chip (and hence fundamentally slow) as ABP. ]
> 
> Welcome to the wonderful world of embedded systems. Just have a peek
> into arch/[arm/powerpc/mips] to see what's coming up to us with full
> force. I would not be surprised when we see an x86 system sharing the
> device driver for i2c or whatever with an ARM SoC in the foreseable
> future.

Ha, yeah I was just going to say "think embedded".  ABP is a much
simpler spec and programming interface than HPET, and since we were
designing new custom silicon, it made sense to just do the simple
thing, rather than butchering an existing spec, then making a partial
HPET that looks like ABP anyway and forcing any future HPET updates to
conform to the new standard (very similar reasoning to the ACPI vs SFI
discussion btw).  Hopefully the technologies we've come up with for
MRST will be flexible enough to be used in future platforms as well;
we'll see.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 26 Jun 2009 09:19:55 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> > >  void platform_feature_init_default(int subarch_id)
> > >  {
> > > -	printk(KERN_INFO "Use default X86 platform feature set\n");
> > > +	if ((subarch_id >= 0) && (subarch_id < N_X86_SUBARCHS)) {
> > > +		if (subarch_id == X86_SUBARCH_MRST) {
> > > +			setup_mrst_default_feature();
> > > +			return;
> > > +		}
> > > +	} else {
> > > +		printk(KERN_INFO "Use default X86 platform feature
> > > set\n");
> > > +	}
> > >  }
> > 
> > Why dont we have some clean and robust PCI config space based 
> > enumeration instead of this boot ID based thing?
> 
> MRST has special PCI support that depends on having the platform 
> identified already, so if you want to use something other than a 
> flag here, maybe we could use an SFI string instead.

An SFI flag would be nicer than any PCI space method i guess - if it 
can be accessed early enough.

	Ingo

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Pavel Machek]

On Fri 2009-06-26 09:54:54, Jesse Barnes wrote:
> On Fri, 26 Jun 2009 18:32:42 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Fri, 26 Jun 2009, Ingo Molnar wrote:
> > > [ Although it is beyond me why ABP was done - why wasnt HPET good
> > >   enough? HPET can do per CPU clockevents too and it's just as 
> > >   off-chip (and hence fundamentally slow) as ABP. ]
> > 
> > Welcome to the wonderful world of embedded systems. Just have a peek
> > into arch/[arm/powerpc/mips] to see what's coming up to us with full
> > force. I would not be surprised when we see an x86 system sharing the
> > device driver for i2c or whatever with an ARM SoC in the foreseable
> > future.
> 
> Ha, yeah I was just going to say "think embedded".  ABP is a much
> simpler spec and programming interface than HPET, and since we were
> designing new custom silicon, it made sense to just do the simple
> thing, rather than butchering an existing spec, then making a partial
> HPET that looks like ABP anyway and forcing any future HPET updates to
> conform to the new standard (very similar reasoning to the ACPI vs SFI
> discussion btw).  Hopefully the technologies we've come up with for

Very similary wrong, I'd say :-(. While you could have created
hpet-lite, where hpet-lite driver would work on hpet system, you went
and created something new.

And yes, SFI is similar disaster, you should just define subset of
acpi ('acpi-lite').

In the end, you are willing to use silicon for compatibility (arm
 instruction set needs less transistors, right?) and wasting millions
 of transistors, then try to save thousands with non-compatible
 devices :-(.
 
								Pavel
 


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Jesse Barnes]

On Tue, 30 Jun 2009 08:35:13 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Fri 2009-06-26 09:54:54, Jesse Barnes wrote:
> > On Fri, 26 Jun 2009 18:32:42 +0200 (CEST)
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > On Fri, 26 Jun 2009, Ingo Molnar wrote:
> > > > [ Although it is beyond me why ABP was done - why wasnt HPET
> > > > good enough? HPET can do per CPU clockevents too and it's just
> > > > as off-chip (and hence fundamentally slow) as ABP. ]
> > > 
> > > Welcome to the wonderful world of embedded systems. Just have a
> > > peek into arch/[arm/powerpc/mips] to see what's coming up to us
> > > with full force. I would not be surprised when we see an x86
> > > system sharing the device driver for i2c or whatever with an ARM
> > > SoC in the foreseable future.
> > 
> > Ha, yeah I was just going to say "think embedded".  ABP is a much
> > simpler spec and programming interface than HPET, and since we were
> > designing new custom silicon, it made sense to just do the simple
> > thing, rather than butchering an existing spec, then making a
> > partial HPET that looks like ABP anyway and forcing any future HPET
> > updates to conform to the new standard (very similar reasoning to
> > the ACPI vs SFI discussion btw).  Hopefully the technologies we've
> > come up with for
> 
> Very similary wrong, I'd say :-(. While you could have created
> hpet-lite, where hpet-lite driver would work on hpet system, you went
> and created something new.
> 
> And yes, SFI is similar disaster, you should just define subset of
> acpi ('acpi-lite').
> 
> In the end, you are willing to use silicon for compatibility (arm
>  instruction set needs less transistors, right?) and wasting millions
>  of transistors, then try to save thousands with non-compatible
>  devices :-(.

You didn't address the essence of either argument; butchering an
existing spec and placing an extra burden on all future implementations
is a high price to pay, both in terms of complexity and cost.

But really these are moot points; this is how the platform works.  I'd
like to see Linux run on it "out of the box" which means integrating
support for these features in a maintainable way.  Hopefully no one
disagrees with that; after all Linux runs on much uglier platforms than
this.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/9] x86/moorestown: add moorestown platform flags

[Ingo Molnar]

* Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 30 Jun 2009 08:35:13 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > On Fri 2009-06-26 09:54:54, Jesse Barnes wrote:
> > > On Fri, 26 Jun 2009 18:32:42 +0200 (CEST)
> > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > 
> > > > On Fri, 26 Jun 2009, Ingo Molnar wrote:
> > > > > [ Although it is beyond me why ABP was done - why wasnt HPET
> > > > > good enough? HPET can do per CPU clockevents too and it's just
> > > > > as off-chip (and hence fundamentally slow) as ABP. ]
> > > > 
> > > > Welcome to the wonderful world of embedded systems. Just have a
> > > > peek into arch/[arm/powerpc/mips] to see what's coming up to us
> > > > with full force. I would not be surprised when we see an x86
> > > > system sharing the device driver for i2c or whatever with an ARM
> > > > SoC in the foreseable future.
> > > 
> > > Ha, yeah I was just going to say "think embedded".  ABP is a much
> > > simpler spec and programming interface than HPET, and since we were
> > > designing new custom silicon, it made sense to just do the simple
> > > thing, rather than butchering an existing spec, then making a
> > > partial HPET that looks like ABP anyway and forcing any future HPET
> > > updates to conform to the new standard (very similar reasoning to
> > > the ACPI vs SFI discussion btw).  Hopefully the technologies we've
> > > come up with for
> > 
> > Very similary wrong, I'd say :-(. While you could have created
> > hpet-lite, where hpet-lite driver would work on hpet system, you went
> > and created something new.
> > 
> > And yes, SFI is similar disaster, you should just define subset of
> > acpi ('acpi-lite').
> > 
> > In the end, you are willing to use silicon for compatibility (arm
> >  instruction set needs less transistors, right?) and wasting millions
> >  of transistors, then try to save thousands with non-compatible
> >  devices :-(.
> 
> You didn't address the essence of either argument; butchering an 
> existing spec and placing an extra burden on all future 
> implementations is a high price to pay, both in terms of 
> complexity and cost.
> 
> But really these are moot points; this is how the platform works.  
> I'd like to see Linux run on it "out of the box" which means 
> integrating support for these features in a maintainable way.  
> Hopefully no one disagrees with that; after all Linux runs on much 
> uglier platforms than this.

I agree and that's fine.

Note that obviously ugliness increases the integration costs and 
latency for Intel: all ugly bits have to be factored out cleanly, 
have to be turned into isolated components and tucked away, so that 
the ugliness does not spread.

If something is just plain 'standard', that makes it really easy to 
merge. The less standard something is, the more trouble integration 
becomes.

Also, i genuinely think that it's perfectly fine to iterate hardware 
specs - to simplify and enhance it. And this is an area where Linux 
is the strongest - we can support just about anything and can do it 
quickly. I just wish that those variations were something to be 
unconditionally proud of from an engineering POV ;-)

So for similar projects in the future - it's OK to be non-standard, 
but the preferred direction should be something genuinely better 
designed, or something that is just a _sub-set_ of the standard. 
Supporting a sub-set is way easier than supporting something that is 
different in some unanticipated way and needs an extension of 
infrastructure.

The IO-APIC bits seems to be the latter in this case - i.e. they are 
an "IO-APIC light" implementation in essence. And that shows 
immediately: the patches arent all that horrible.

And SFI seems useful to me as well, not because it's a sub-set of 
ACPI, but because i find it cleaner than ACPI.

The ABP timer is another matter - it's a completely separate 
from-scratch thing that is dissimilar to everything that exists. 
OTOH, timers are pretty separate entities just (still) not 
abstracted out well enough on x86 yet.

	Ingo