[PATCH] Disallow the internal programmer on non-x86 for now

[Carl-Daniel Hailfinger]

The internal programmer needs correct information about flash_base and
chip window top/bottom alignment on non-x86 before it can be used. Abort
any internal programmer action for now until the code is fixed.

Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Index: flashrom-disable_internal_nonx86/internal.c
===================================================================
--- flashrom-disable_internal_nonx86/internal.c	(Revision 1013)
+++ flashrom-disable_internal_nonx86/internal.c	(Arbeitskopie)
@@ -165,16 +165,22 @@
 	pci_init(pacc);		/* Initialize the PCI library */
 	pci_scan_bus(pacc);	/* We want to get the list of devices */
 
-	/* We look at the lbtable first to see if we need a
+#if defined(__i386__) || defined(__x86_64__)
+	/* We look at the cbtable first to see if we need a
 	 * mainboard specific flash enable sequence.
 	 */
 	coreboot_init();
 
-#if defined(__i386__) || defined(__x86_64__)
 	dmi_init();
 
 	/* Probe for the Super I/O chip and fill global struct superio. */
 	probe_superio();
+#else
+	/* FIXME: Enable cbtable searching on all non-x86 platforms supported
+	 *        by coreboot.
+	 * FIXME: Find a replacement for DMI on non-x86.
+	 * FIXME: Enable SuperI/O probing once port I/O is possible.
+	 */
 #endif
 
 	/* Warn if a laptop is detected. */
@@ -200,6 +206,7 @@
 		}
 	}
 
+#if __FLASHROM_LITTLE_ENDIAN__
 	/* try to enable it. Failure IS an option, since not all motherboards
 	 * really need this to be done, etc., etc.
 	 */
@@ -220,7 +227,25 @@
 	 * The error code might have been a warning only.
 	 * Besides that, we don't check the board enable return code either.
 	 */
+#if defined(__i386__) || defined(__x86_64__)
 	return 0;
+#else
+	msg_perr("Your platform is not supported yet for the internal "
+		 "programmer due to missing flash_base and top/bottom "
+		 "alignment information.\n"
+		 "Aborting.\n");
+	return 1;
+#endif
+#else
+	/* FIXME: Remove this unconditional abort once all PCI drivers are
+	 * converted to use little-endian accesses for memory BARs.
+	 */
+	msg_perr("Your platform is not supported yet for the internal "
+		 "programmer because it has not been converted from native "
+		 "endian to little endian access yet.\n"
+		 "Aborting.\n");
+	return 1;
+#endif
 }
 
 int internal_shutdown(void)


-- 
http://www.hailfinger.org/

[Vladimir 'φ-coder/phcoder' Serbinenko]

Carl-Daniel Hailfinger wrote:
> The internal programmer needs correct information about flash_base and
> chip window top/bottom alignment on non-x86 before it can be used. Abort
> any internal programmer action for now until the code is fixed.
>
> Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
>
> +#else
> +	/* FIXME: Remove this unconditional abort once all PCI drivers are
> +	 * converted to use little-endian accesses for memory BARs.
> +	 */
> +	msg_perr("Your platform is not supported yet for the internal "
> +		 "programmer because it has not been converted from native "
> +		 "endian to little endian access yet.\n"
> +		 "Aborting.\n");

Should have been "external".


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

[Carl-Daniel Hailfinger]

On 26.05.2010 08:22, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Carl-Daniel Hailfinger wrote:
> > The internal programmer needs correct information about flash_base and
> > chip window top/bottom alignment on non-x86 before it can be used. Abort
> > any internal programmer action for now until the code is fixed.
> >
> > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
> >
> > +#else
> > +	/* FIXME: Remove this unconditional abort once all PCI drivers are
> > +	 * converted to use little-endian accesses for memory BARs.
> > +	 */
> > +	msg_perr("Your platform is not supported yet for the internal "
> > +		 "programmer because it has not been converted from native "
> > +		 "endian to little endian access yet.\n"
> > +		 "Aborting.\n");
>
> Should have been "external".

Are you sure? This warning is part of the internal (main onboard flash)
programmer.
Although this warning should be added for external PCI-based
programmers, such a change is not part of this patch because I want to
fix them instead of marking them as broken.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[Vladimir 'φ-coder/phcoder' Serbinenko]

Carl-Daniel Hailfinger wrote:
> On 26.05.2010 08:22, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > Carl-Daniel Hailfinger wrote:
> > > The internal programmer needs correct information about flash_base and
> > > chip window top/bottom alignment on non-x86 before it can be used. Abort
> > > any internal programmer action for now until the code is fixed.
> > >
> > > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
> > >
> > > +#else
> > > +	/* FIXME: Remove this unconditional abort once all PCI drivers are
> > > +	 * converted to use little-endian accesses for memory BARs.
> > > +	 */
> > > +	msg_perr("Your platform is not supported yet for the internal "
> > > +		 "programmer because it has not been converted from native "
> > > +		 "endian to little endian access yet.\n"
> > > +		 "Aborting.\n");
> >
> > Should have been "external".
>
> Are you sure? This warning is part of the internal (main onboard flash)
> programmer.
> Although this warning should be added for external PCI-based
> programmers, such a change is not part of this patch because I want to
> fix them instead of marking them as broken.

Then I misread the code. Sorry for noise.
> Regards,
> Carl-Daniel

[PATCH] Disallow the internal programmer on non-x86 for now

[Uwe Hermann]

On Wed, May 26, 2010 at 05:07:25AM +0200, Carl-Daniel Hailfinger wrote:
> The internal programmer needs correct information about flash_base and
> chip window top/bottom alignment on non-x86 before it can be used. Abort
> any internal programmer action for now until the code is fixed.
>
> Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
> +#if defined(__i386__) || defined(__x86_64__)
>   return 0;
> +#else
> +	msg_perr("Your platform is not supported yet for the internal "
> +		 "programmer due to missing flash_base and top/bottom "
>                                            ^
> Add an \n here please, to make the output look nice in 80x25 terminals/xterms.
> +	/* FIXME: Remove this unconditional abort once all PCI drivers are
> +	 * converted to use little-endian accesses for memory BARs.
> +	 */
> +	msg_perr("Your platform is not supported yet for the internal "
> +		 "programmer because it has not been converted from native "
>                                            ^
>                                       Ditto here.

Otherwise it looks fine and compiles on x86.

Acked-by: Uwe Hermann <[e-mail]>


Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org

[PATCH] Disallow the internal programmer on non-x86 for now

[Carl-Daniel Hailfinger]

On 04.06.2010 18:52, Uwe Hermann wrote:
> On Wed, May 26, 2010 at 05:07:25AM +0200, Carl-Daniel Hailfinger wrote:
> > The internal programmer needs correct information about flash_base and
> > chip window top/bottom alignment on non-x86 before it can be used. Abort
> > any internal programmer action for now until the code is fixed.
> >
> > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
>
> Otherwise it looks fine and compiles on x86.
>
> Acked-by: Uwe Hermann <[e-mail]>

Thanks for the review. I changed the output formatting as requested.
Unfortunately it seems that the patch no longer compiles for me, and
apparently half of the patch was somehow left out of the diff.

New patch follows. Could you please take a look again?

Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Index: flashrom-disable_internal_nonx86/flash.h
===================================================================
--- flashrom-disable_internal_nonx86/flash.h	(Revision 1028)
+++ flashrom-disable_internal_nonx86/flash.h	(Arbeitskopie)
@@ -361,6 +361,9 @@
 /* chipset_enable.c */
 int chipset_flash_enable(void);
 
+/* processor_enable.c */
+int processor_flash_enable(void);
+
 /* physmap.c */
 void *physmap(const char *descr, unsigned long phys_addr, size_t len);
 void *physmap_try_ro(const char *descr, unsigned long phys_addr, size_t len);
Index: flashrom-disable_internal_nonx86/processor_enable.c
===================================================================
--- flashrom-disable_internal_nonx86/processor_enable.c	(Revision 0)
+++ flashrom-disable_internal_nonx86/processor_enable.c	(Revision 0)
@@ -0,0 +1,45 @@
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright (C) 2010 Carl-Daniel Hailfinger
+ *
+ * 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
+ */
+
+/*
+ * Contains the processor specific flash enables and system settings.
+ */
+
+#include "flash.h"
+
+#if defined(__i386__) || defined(__x86_64__)
+
+int processor_flash_enable(void)
+{
+	/* On x86, flash access is not processor specific except on
+	 * AMD Elan SC520, AMD Geode and maybe other SoC-style CPUs.
+	 * FIXME: Move enable_flash_cs5536 and get_flashbase_sc520 here.
+	 */
+	return 0;
+}
+
+#else
+
+int processor_flash_enable(void)
+{
+	/* Not implemented yet. Oh well. */
+	return 1;
+}
+
+#endif
Index: flashrom-disable_internal_nonx86/Makefile
===================================================================
--- flashrom-disable_internal_nonx86/Makefile	(Revision 1028)
+++ flashrom-disable_internal_nonx86/Makefile	(Arbeitskopie)
@@ -124,7 +124,7 @@
 
 ifeq ($(CONFIG_INTERNAL), yes)
 FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
-PROGRAMMER_OBJS += chipset_enable.o board_enable.o cbtable.o dmi.o internal.o
+PROGRAMMER_OBJS += processor_enable.o chipset_enable.o board_enable.o cbtable.o dmi.o internal.o
 # FIXME: The PROGRAMMER_OBJS below should only be included on x86.
 PROGRAMMER_OBJS += it87spi.o ichspi.o sb600spi.o wbsio_spi.o
 NEED_PCI := yes
Index: flashrom-disable_internal_nonx86/internal.c
===================================================================
--- flashrom-disable_internal_nonx86/internal.c	(Revision 1028)
+++ flashrom-disable_internal_nonx86/internal.c	(Arbeitskopie)
@@ -165,16 +165,27 @@
 	pci_init(pacc);		/* Initialize the PCI library */
 	pci_scan_bus(pacc);	/* We want to get the list of devices */
 
-	/* We look at the lbtable first to see if we need a
+	if (processor_flash_enable()) {
+		msg_perr("Processor detection/init failed, aborting.");
+		return 1;
+	}
+
+#if defined(__i386__) || defined(__x86_64__)
+	/* We look at the cbtable first to see if we need a
 	 * mainboard specific flash enable sequence.
 	 */
 	coreboot_init();
 
-#if defined(__i386__) || defined(__x86_64__)
 	dmi_init();
 
 	/* Probe for the Super I/O chip and fill global struct superio. */
 	probe_superio();
+#else
+	/* FIXME: Enable cbtable searching on all non-x86 platforms supported
+	 *        by coreboot.
+	 * FIXME: Find a replacement for DMI on non-x86.
+	 * FIXME: Enable SuperI/O probing once port I/O is possible.
+	 */
 #endif
 
 	/* Warn if a laptop is detected. */
@@ -200,6 +211,7 @@
 		}
 	}
 
+#if __FLASHROM_LITTLE_ENDIAN__
 	/* try to enable it. Failure IS an option, since not all motherboards
 	 * really need this to be done, etc., etc.
 	 */
@@ -220,7 +232,26 @@
 	 * The error code might have been a warning only.
 	 * Besides that, we don't check the board enable return code either.
 	 */
+#if defined(__i386__) || defined(__x86_64__)
 	return 0;
+#else
+	msg_perr("Your platform is not supported yet for the internal "
+		 "programmer due to missing\n"
+		 "flash_base and top/bottom alignment information.\n"
+		 "Aborting.\n");
+	return 1;
+#endif
+#else
+	/* FIXME: Remove this unconditional abort once all PCI drivers are
+	 * converted to use little-endian accesses for memory BARs.
+	 */
+	msg_perr("Your platform is not supported yet for the internal "
+		 "programmer because it has\n"
+		 "not been converted from native endian to little endian "
+		 "access yet.\n"
+		 "Aborting.\n");
+	return 1;
+#endif
 }
 
 int internal_shutdown(void)


-- 
http://www.hailfinger.org/

[PATCH] Disallow the internal programmer on non-x86 for now

[Uwe Hermann]

On Fri, Jun 04, 2010 at 08:01:21PM +0200, Carl-Daniel Hailfinger wrote:
> Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Acked-by: Uwe Hermann <[e-mail]>

Re-tested, looks good.
> Index: flashrom-disable_internal_nonx86/internal.c
> ===================================================================
> --- flashrom-disable_internal_nonx86/internal.c	(Revision 1028)
> +++ flashrom-disable_internal_nonx86/internal.c	(Arbeitskopie)
> @@ -165,16 +165,27 @@
>   pci_init(pacc);		/* Initialize the PCI library */
>   pci_scan_bus(pacc);	/* We want to get the list of devices */
>
> -	/* We look at the lbtable first to see if we need a
> +	if (processor_flash_enable()) {
> +		msg_perr("Processor detection/init failed, aborting.");

Missing \n in this message.
> +	 * FIXME: Enable SuperI/O probing once port I/O is possible.

"Super I/O" here for consistency please, forgot to mention last time.


Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org

[PATCH] Disallow the internal programmer on non-x86 for now

[Carl-Daniel Hailfinger]

On 04.06.2010 20:27, Uwe Hermann wrote:
> On Fri, Jun 04, 2010 at 08:01:21PM +0200, Carl-Daniel Hailfinger wrote:
> > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
>
> Acked-by: Uwe Hermann <[e-mail]>
>
> Re-tested, looks good.

Thanks!
Committed in r1031 with the changes you requested.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/