Patchwork Enable maximum decoded range for VT8237*

login
register
about
Submitter Rudolf Marek
Date 2010-04-20 21:47:25
Message ID <4BCE20ED.205@assembler.cz>
Download mbox | patch
Permalink /patch/1248/
State Superseded
Headers show

Comments

Rudolf Marek - 2010-04-20 21:47:25
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

Attached patch adds support for tinybootblock on VT8237* to decode whole flash
independent of strapping, making larger flashes work again (as it was in
pre-tiny bootblock era)

Signed-off-by: Rudolf Marek <r.marek@assembler.cz>

I had a strange problem:

#include <arch/io.h>
#include <arch/romcc_io.h>
#include <device/pci_ids.h>

static void bootblock_southbridge_init(void) {

device_t dev;

/* Power management controller */
dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
				      PCI_DEVICE_ID_VIA_VT8237R_LPC), 0);

if (dev == PCI_DEV_INVALID) {
/* Power management controller */
	dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA, 		
					PCI_DEVICE_ID_VIA_VT8237S_LPC), 0);

	if (dev == PCI_DEV_INVALID)
	return;
}
	pci_write_config8(dev, 0x41, 0x7f);
}

did not reach the pci write for some reason. This code is a copy from
enable_rom_decode() so it should work.

Thanks,
Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkvOIO0ACgkQ3J9wPJqZRNVqFwCgz6+rJ6QmDVoFLHjZ3MS668jY
uW4An2kWok6aukzhZGC1Fue5yJkN0Eef
=LGC8
-----END PGP SIGNATURE-----
Stefan Reinauer - 2010-04-20 21:59:40
On 4/20/10 11:47 PM, Rudolf Marek wrote:

Does it work better like this?
> /* Power management controller */
> dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
>                       PCI_DEVICE_ID_VIA_VT8237R_LPC), 0);
>
> if (dev == PCI_DEV_INVALID) {
> /* Power management controller */
>     dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
>                     PCI_DEVICE_ID_VIA_VT8237S_LPC), 0);
} // close scope early
>
>     if (dev == PCI_DEV_INVALID)
>     return;
>     pci_write_config8(dev, 0x41, 0x7f);

If not, we should make a test case and send it to eric.


....
> +static void bootblock_southbridge_init(void) {
>   
functions should have their opening bracket on the next line
> +
> +	/* ROM decode last 8MB FF800000 - FFFFFFFF on VT8237S/VT8237A */
> +	/* ROM decode last 4MB FFC00000 - FFFFFFFF on VT8237R */
> +
> +	/* get SB on 0:11.0, because pci_locate_device construct similar in
> +	   enable_rom_decode() did not worked for some reason */
> +	pci_write_config8(PCI_DEV(0,0x11,0), 0x41, 0x7f);
> +}
>   
Is the device always on 0, 0x11, 0? If not, we should add a more
explicit comment saying TODO and explaining why the broken code was
checked in.
Rudolf Marek - 2010-04-20 22:12:47
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> If not, we should make a test case and send it to eric.

It does not. I tried what you suggested. What now?


> 
> 
> ....
>> +static void bootblock_southbridge_init(void) {
>>   
> functions should have their opening bracket on the next line

Aha ok will fix it for a final patch.

>> +
>> +	/* ROM decode last 8MB FF800000 - FFFFFFFF on VT8237S/VT8237A */
>> +	/* ROM decode last 4MB FFC00000 - FFFFFFFF on VT8237R */
>> +
>> +	/* get SB on 0:11.0, because pci_locate_device construct similar in
>> +	   enable_rom_decode() did not worked for some reason */
>> +	pci_write_config8(PCI_DEV(0,0x11,0), 0x41, 0x7f);
>> +}
>>   
> Is the device always on 0, 0x11, 0? If not, we should add a more
> explicit comment saying TODO and explaining why the broken code was
> checked in.

Yes it it should although VT8237S has programmable SB IDSEL never seen any
VT8237 on something else than 11.0, and I did not found any straps for it just
a sw reg.

Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkvOJt8ACgkQ3J9wPJqZRNVA4QCgpApwVfH4rnmlbsuiVkgCKbMW
ijMAn0K26Fuax7CWJJ6J0HCjcE5Ua7S8
=DzHk
-----END PGP SIGNATURE-----
Stefan Reinauer - 2010-04-20 22:54:18
On 4/20/10 11:47 PM, Rudolf Marek wrote:
> Hello,
>
> Attached patch adds support for tinybootblock on VT8237* to decode
> whole flash
> independent of strapping, making larger flashes work again (as it was in
> pre-tiny bootblock era)
>
> Signed-off-by: Rudolf Marek <r.marek@assembler.cz>
>
> I had a strange problem:
>
> #include <arch/io.h>
> #include <arch/romcc_io.h>
> #include <device/pci_ids.h>
>
> static void bootblock_southbridge_init(void) {
>
> device_t dev;
>
> /* Power management controller */
> dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
> PCI_DEVICE_ID_VIA_VT8237R_LPC), 0);
>
> if (dev == PCI_DEV_INVALID) {
> /* Power management controller */
> dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
> PCI_DEVICE_ID_VIA_VT8237S_LPC), 0);
>
> if (dev == PCI_DEV_INVALID)
> return;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^

And you are sure that dev is not PCI_DEV_INVALID at this point?

> }
> pci_write_config8(dev, 0x41, 0x7f);
> }
Rudolf Marek - 2010-04-22 21:04:44
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Following code does work:

static void bootblock_southbridge_init(void) {

/* ROM decode last 8MB FF800000 - FFFFFFFF on VT8237S/VT8237A */
/* ROM decode last 4MB FFC00000 - FFFFFFFF on VT8237R */

device_t dev;

/* Power management controller */
//dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
//		    PCI_DEVICE_ID_VIA_VT8237R_LPC), 0);

//if (dev == PCI_DEV_INVALID) {
/* Power management controller */
dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
               PCI_DEVICE_ID_VIA_VT8237S_LPC), 0);
//}

if (dev == PCI_DEV_INVALID)
   return;

  pci_write_config8(dev, 0x41, 0x7f);
}

If one uncomment the commented out parts. It stops working. Nothing is changed
if the if (dev == PCI_DEV_INVALID) return; is moved to the if or not.

Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkvQuesACgkQ3J9wPJqZRNUikwCglEm7Upbqg8SlTNnyUaRoKObi
IKgAoNaottXQGBnJaYsCRFRgTzkafhuV
=/HGI
-----END PGP SIGNATURE-----
Rudolf Marek - 2010-04-22 21:22:02
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi again,

dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
				    PCI_DEVICE_ID_VIA_VT8237R_LPC), 0);

pci_write_config8(PCI_DEV(0,0x11,0), 0x41, 0x7f);

This code won't make it either. So I suspect it is not good idea to walk whole
PCI bus so early. Maybe HT needs to be setup?

Thanks,
Rudolf


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkvQvfoACgkQ3J9wPJqZRNV/vACfUDVMP5rnCPFbYgXe8eNdGq8c
AmoAoJo7TzcKesMm26tXGY++crtZJm2l
=m4HL
-----END PGP SIGNATURE-----
Myles Watson - 2010-04-22 21:29:49
> Hi again,
>
> dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
>                                    PCI_DEVICE_ID_VIA_VT8237R_LPC), 0);
>
> pci_write_config8(PCI_DEV(0,0x11,0), 0x41, 0x7f);
>
> This code won't make it either. So I suspect it is not good idea to walk whole
> PCI bus so early. Maybe HT needs to be setup?

You could use pci_locate_device_on_bus() with bus 0 to test that theory.

Thanks,
Myles

Patch

Index: src/southbridge/via/vt8237r/bootblock.c
===================================================================
--- src/southbridge/via/vt8237r/bootblock.c	(revision 0)
+++ src/southbridge/via/vt8237r/bootblock.c	(revision 0)
@@ -0,0 +1,32 @@ 
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright (C) 2010 Rudolf Marek <r.marek@assembler.cz>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+
+#include <arch/io.h>
+#include <arch/romcc_io.h>
+#include <device/pci_ids.h>
+
+static void bootblock_southbridge_init(void) {
+
+	/* ROM decode last 8MB FF800000 - FFFFFFFF on VT8237S/VT8237A */
+	/* ROM decode last 4MB FFC00000 - FFFFFFFF on VT8237R */
+
+	/* get SB on 0:11.0, because pci_locate_device construct similar in
+	   enable_rom_decode() did not worked for some reason */
+	pci_write_config8(PCI_DEV(0,0x11,0), 0x41, 0x7f);
+}
Index: src/southbridge/via/vt8237r/Kconfig
===================================================================
--- src/southbridge/via/vt8237r/Kconfig	(revision 5467)
+++ src/southbridge/via/vt8237r/Kconfig	(working copy)
@@ -25,3 +25,8 @@ 
 	bool
 	default n
 	depends on SOUTHBRIDGE_VIA_VT8237R
+
+config BOOTBLOCK_SOUTHBRIDGE_INIT
+	string
+	default "southbridge/via/vt8237r/bootblock.c"
+	depends on SOUTHBRIDGE_VIA_VT8237R