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

[Peter Stuge]

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

I still have a few thoughts..

Harrison, Jon (SELEX GALILEO, UK) wrote:
> 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.

Unrelated, but maybe talk to your systems administrator about this,
it doesn't make much sense on a public mailing list - and who sends
confidential email without encryption anyway? ;)

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?
> +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.
> 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?
> 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.

Thanks

//Peter