[PATCH]Remove fallback/normal decision from mainboards' romstage.c

[Patrick Georgi]

Hi,

this is a follow up patch to my previous patch. While the previous one
handled the boards with support for old-style failover, this one take
care of the other boards (that "only" did normal/fallback)

It's abuild tested and boot-tested with kontron/986lcd-m (which is
affected due to the changes in src/cpu/intel)

Signed-off-by: Patrick Georgi

[PATCH]Remove fallback/normal decision from mainboards'romstage.c

[Myles Watson]

> It's abuild tested and boot-tested with kontron/986lcd-m (which is
> affected due to the changes in src/cpu/intel)
> if (!((cpu_init_detectedx) || (!boot_cpu()))) {

Maybe in a follow-up patch this could become:
+	if (!cpu_init_detectedx && boot_cpu()) {
> Signed-off-by: Patrick Georgi

Acked-by: Myles Watson

Thanks,
Myles

[PATCH]Remove fallback/normal decision from mainboards'romstage.c

[Patrick Georgi]

Am 18.03.2010 17:09, schrieb Myles Watson:
> > It's abuild tested and boot-tested with kontron/986lcd-m (which is
> > affected due to the changes in src/cpu/intel)
> > if (!((cpu_init_detectedx) || (!boot_cpu()))) {
>
> Maybe in a follow-up patch this could become:
> if (!cpu_init_detectedx && boot_cpu()) {

Like this?
I also removed the failover.c files, which aren't used anymore (and were
pretty useless after the cleanup anyway)

Signed-off-by: Patrick Georgi

[PATCH]Remove fallback/normal decision from mainboards'romstage.c

[Peter Stuge]

Patrick Georgi wrote:
> > > It's abuild tested and boot-tested with kontron/986lcd-m (which is
> > > affected due to the changes in src/cpu/intel)
> > > if (!((cpu_init_detectedx) || (!boot_cpu()))) {
> >
> > Maybe in a follow-up patch this could become:
> > if (!cpu_init_detectedx && boot_cpu()) {
>
> Like this?
> I also removed the failover.c files, which aren't used anymore (and were
> pretty useless after the cleanup anyway)
>
> Signed-off-by: Patrick Georgi

Acked-by: Peter Stuge

[PATCH]Remove fallback/normal decision from mainboards'romstage.c

[Myles Watson]

> > > if (!((cpu_init_detectedx) || (!boot_cpu()))) {
> >
> > Maybe in a follow-up patch this could become:
> > if (!cpu_init_detectedx && boot_cpu()) {
>
> Like this?
> I also removed the failover.c files, which aren't used anymore (and were
> pretty useless after the cleanup anyway)

Much easier to read.  Thanks.
> Signed-off-by: Patrick Georgi

Acked-by: Myles Watson

Thanks,
Myles

[PATCH]Remove fallback/normal decision from mainboards' romstage.c

[Peter Stuge]

Patrick Georgi wrote:
> this is a follow up patch to my previous patch. While the previous one
> handled the boards with support for old-style failover, this one take
> care of the other boards (that "only" did normal/fallback)

..
> +++ src/mainboard/kontron/kt690/romstage.c	(working copy)

..
> @@ -163,7 +113,15 @@
>    struct cpuid_result cpuid1;
>    struct sys_info *sysinfo = (struct sys_info *)(CONFIG_DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE - CONFIG_DCACHE_RAM_GLOBAL_VAR_SIZE);
>
> if (!((cpu_init_detectedx) || (!boot_cpu()))) {
> /* Nothing special needs to be done to find bus 0 */
> /* Allow the HT devices to be found */
> enumerate_ht_chain();
>
> /* sb600_lpc_port80(); */
> sb600_pci_port80();

This is a recurring pattern with both sb600 and sb700. Is there a
reason to duplicate that call in a comment or could the comment just
be removed?
> --- src/mainboard/amd/mahogany/cache_as_ram_auto.c	(revision 5256)

..
> -void real_main(unsigned long bist, unsigned long cpu_init_detectedx)
> -{
> static const u16 spd_addr[] = { DIMM0, 0, 0, 0, DIMM1, 0, 0, 0, };
> int needs_reset = 0;
> u32 bsp_apicid = 0;
> msr_t msr;
> struct cpuid_result cpuid1;
> struct sys_info *sysinfo = (struct sys_info *)(CONFIG_DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE - CONFIG_DCACHE_RAM_GLOBAL_VAR_SIZE);
> if (bist == 0) {
> bsp_apicid = init_cpus(cpu_init_detectedx, sysinfo);
> }
> enable_rs780_dev8();
> sb700_lpc_init();
> it8718f_enable_serial(0, CONFIG_TTYS0_BASE);
> uart_init();
> console_init();
> /* Halt if there was a built in self test failure */
> report_bist_failure(bist);
> printk_debug("bsp_apicid=0x%x\n", bsp_apicid);
> setup_mahogany_resource_map();
> setup_coherent_ht_domain();
>
> -#if CONFIG_LOGICAL_CPUS==1
> /* It is said that we should start core1 after all core0 launched */
> wait_all_core0_started();
> start_other_cores();
>
> -#endif
> wait_all_aps_started(bsp_apicid);
> ht_setup_chains_x(sysinfo);
> /* run _early_setup before soft-reset. */
> rs780_early_setup();
> sb700_early_setup();
>
> -/* Check to see if processor is capable of changing FIDVID  */
> /* otherwise it will throw a GP# when reading FIDVID_STATUS */
> cpuid1 = cpuid(0x80000007);
> if( (cpuid1.edx & 0x6) == 0x6 ) {
> /* Read FIDVID_STATUS */
> msr=rdmsr(0xc0010042);
> printk_debug("begin msr fid, vid: hi=0x%x, lo=0x%x\n", msr.hi, msr.lo);
> enable_fid_change();
> enable_fid_change_on_sb(sysinfo->sbbusn, sysinfo->sbdn);
> init_fidvid_bsp(bsp_apicid);
> /* show final fid and vid */
> msr=rdmsr(0xc0010042);
> printk_debug("end msr fid, vid: hi=0x%x, lo=0x%x\n", msr.hi, msr.lo);
> } else {
> printk_debug("Changing FIDVID not supported\n");
> }
> needs_reset = optimize_link_coherent_ht();
> needs_reset |= optimize_link_incoherent_ht(sysinfo);
> rs780_htinit();
> printk_debug("needs_reset=0x%x\n", needs_reset);
> if (needs_reset) {
> print_info("ht reset -\r\n");
> soft_reset();
> }
> allow_all_aps_stop(bsp_apicid);
> /* It's the time to set ctrl now; */
> printk_debug("sysinfo->nodes: %2x  sysinfo->ctrl: %2x  spd_addr: %2x\n",
>      sysinfo->nodes, sysinfo->ctrl, spd_addr);
> fill_mem_ctrl(sysinfo->nodes, sysinfo->ctrl, spd_addr);
> sdram_initialize(sysinfo->nodes, sysinfo->ctrl, sysinfo);
> rs780_before_pci_init();
> sb700_before_pci_init();
> post_cache_as_ram();
>
> -}

Where did this code go?

//Peter

[PATCH]Remove fallback/normal decision from mainboards' romstage.c

[Stefan Reinauer]

On 3/18/10 5:23 PM, Peter Stuge wrote:
> > > /* sb600_lpc_port80(); */
> > > sb600_pci_port80();
>
> This is a recurring pattern with both sb600 and sb700. Is there a
> reason to duplicate that call in a comment or could the comment just
> be removed?

It should stay. One command is routing port 80 to LPC, the other to PCI.

[PATCH]Remove fallback/normal decision from mainboards' romstage.c

[Patrick Georgi]

Am 18.03.2010 17:23, schrieb Peter Stuge:
> > if (!((cpu_init_detectedx) || (!boot_cpu()))) {
> > /* Nothing special needs to be done to find bus 0 */
> > /* Allow the HT devices to be found */
> > enumerate_ht_chain();
> >
> > /* sb600_lpc_port80(); */
> > sb600_pci_port80();
>
> This is a recurring pattern with both sb600 and sb700. Is there a
> reason to duplicate that call in a comment or could the comment just
> be removed?

This was a mechanical refactoring without looking in-depth at such
issues. (all that code existed before)
I guess these two calls configure where port 80 writes go, and the
comment is basically documentation how to move port 80 to the LPC bus.

There might be ways to refactor such code, but that requires a more
global view.
> > --- src/mainboard/amd/mahogany/cache_as_ram_auto.c	(revision 5256)
>
> Where did this code go?

src/mainboard/amd/mahogany/romstage.c already exists and is nearly
identical, except for an additional bugfix.

Patrick

[PATCH]Remove fallback/normal decision from mainboards' romstage.c

[Peter Stuge]

Patrick Georgi wrote:
> > > /* sb600_lpc_port80(); */
> > > sb600_pci_port80();
> >
> > This is a recurring pattern with both sb600 and sb700. Is there a
> > reason to duplicate that call in a comment or could the comment just
> > be removed?
>
> This was a mechanical refactoring without looking in-depth at such
> issues. (all that code existed before)

Nod, yeah, I noticed it was there before.
> There might be ways to refactor such code, but that requires a more
> global view.

Fair enough.
> > > --- src/mainboard/amd/mahogany/cache_as_ram_auto.c	(revision 5256)
> >
> > Where did this code go?
>
> src/mainboard/amd/mahogany/romstage.c already exists and is nearly
> identical, except for an additional bugfix.

Brilliant.

Acked-by: Peter Stuge