Re: [coreboot] [PATCH] CN400 / EPIA-NL [v2] patch

[Harrison, Jon (SELEX GALILEO, UK)]

Peter,

See below. If this patch can't go in as it is then it'll have to wait
until I've got the time to break the patches up, I have another patch
coming soon to add initial ACPI support, which seems to be just about
working.

Jon
-----Original Message-----
From: [e-mail]
[mailto:[e-mail]] On Behalf Of
[e-mail]
Sent: 13 July 2009 19:57
To: [e-mail]
Subject: coreboot Digest, Vol 53, Issue 56
------------------------------

Message: 3
Date: Mon, 13 Jul 2009 14:02:29 +0200
From: Peter Stuge 
To: [e-mail]
Subject: Re: [coreboot] [PATCH] CN400 / EPIA-NL [v2] patch
Message-ID: 
Content-Type: text/plain; charset=us-ascii

Hi Jon, I did have a look but didn't send out comments.

I still have a few thoughts..

Content-Description: cn400-patch-v6.patch
> Index: src/mainboard/via/epia-n/Config.lb

..
> -chip northbridge/via/cn400			# Northbridge
> device apic_cluster 0 on			# APIC cluster
> chip cpu/via/model_c3			# VIA C3
>  device apic 0 on end			# APIC
> end
> end
> This apic_cluster block is moved to be last within the cn400 block,
> does it matter if it comes first or last?

I don't know. It works this way round and it's now consistent with the
epia-cn
> +chip northbridge/via/cn400				# Northbridge

..
> device pci_domain 0 on			# PCI domain
> device pci_domain 0 on				# PCI domain
> The patch still has some whitespace changes in Config.lb.
> I know how things move around while working, but it would be so nice
> to not have whitespace changes in patches that are sent for review.
> Everyone, please review your patches yourself before sending out.

These whitespace fixes come for free with the re-jig of the Config.lb
order
> Index: src/northbridge/via/cn400/raminit.c

..
> //print_val("\r\nBank 0 (*32 Mb) ",c);

..
> //print_val("\r\nTotal Memory (*32 Mb) ",c);

..
> /* Graphics Control Basic Init. */
> //pci_write_config8(ctrl.d0f3, 0xb0, 0xFf);
> //pci_write_config8(ctrl.d0f3, 0xb1, 0xAA);
> //pci_write_config8(ctrl.d0f3, 0xb2, 0xAA);
> //pci_write_config8(ctrl.d0f3, 0xb3, 0x5A);
> //pci_write_config8(ctrl.d0f3, 0xb4, 0x0f);
> /* AGP Controller Interface Basic Init */
> //pci_write_config8(ctrl.d0f3, 0xc0, 0x3b);
> /* VGA device, Basic frame Buffer Init. */
> //pci_write_config8(ctrl.d0f3, 0xa0, 0x01);
> /* Bit 7 = Enable VGA When Set to 1 */
> //pci_write_config8(ctrl.d0f3, 0xa1, 0xef);
> //pci_write_config8(ctrl.d0f3, 0xa4, 0x00);
> Should these and all other removed comments be a separate patch? Are
> they related to the other changes in this patch?

Just part of the general tidy up before moving on.
> Index: src/northbridge/via/cn400/northbridge.c

..
> static u32 find_pci_tolm(struct bus *bus)
>  {
> struct resource *min;
> struct resource *min = NULL;
> u32 tolm;
> print_debug("Entering CN400 find_pci_tolm\n");
> printk_spew("Entering CN400 find_pci_tolm\n");
>
> min = 0;
> search_bus_resources(bus, IORESOURCE_MEM, IORESOURCE_MEM,
> 	     tolm_test, &min);
> tolm = 0xffffffffUL;
> if (min && tolm > min->base)
> tolm = min->base;
>
> print_debug("Leaving find_pci_tolm\n");
>
> printk_spew("Leaving CN400 find_pci_tolm\n");
>
> return tolm;
>
> }
> This hunk doesn't change functionality much, it seems to only change
> debugging, but if you prefer this way then I think that's fine.

Again this is just for consistency with the rest of the code.
> Thanks
> //Peter

SELEX Sensors and Airborne Systems Limited
Registered Office: Sigma House, Christopher Martin Road, Basildon, Essex SS14 3EL
A company registered in England & Wales.  Company no. 02426132
********************************************************************
This email and any attachments are confidential to the intended
recipient and may also be privileged. If you are not the intended
recipient please delete it from your system and notify the sender.
You should not copy it or use it for any purpose nor disclose or
distribute its contents to any other person.
********************************************************************