Re: [flashrom] [Patch] NVIDIA Tegra2 SPI controller support

[Carl-Daniel Hailfinger]

Am 28.06.2011 01:52 schrieb David Hendricks:
> This patch set makes Flashrom a bit more ARM-friendly and adds raw Tegra2
> SPI controller support. It isolates a few PCI-isms, adds some low-level
> hardware access stubs, adds Tegra2 processor detection, and adds Tegra2 raw
> SPI controller code.

Nice!
> I've attached some logfiles for read/erase/write operations. This particular
> patch set was tested using a board based off NVIDIA's Tegra2 reference board
> ("Seaboard"). Earlier revisions were tested on a Seaboard.
>
> All three attached patches are:
> Signed-off-by: David Hendricks
>
> Much of the original U-Boot --> Flashrom porting work was done by Stefan
> Reinauer , and the FIFO code updated / fixed up by
> Louis Lo .
>
> A few notes:
> A few programmers must be disabled due to reliance on PCI. You can hack
>
> up your Makefile or compile with "CONFIG_RAYER_SPI=no CONFIG_NIC3COM=no
> CONFIG_NICREALTEK=no CONFIG_SATAMV=no".

Actually, it is a requirement for PCI port I/O space (well, the
non-memmapped space specified in the PCI standard, usually accesses with
in[bwl]/out[bwl] functions). We ought to detect unavailability of that
at compile time, preferably in the Makefile.
> Due to issues with libpci, you can't use the same binary on machines with
>
> PCI and without PCI. So for example, you can't program a SPI ROM onboard a
> PCI card that is connected to your ARM SoC.

Could you explain more? IIRC some people were using flashrom on ARM
(independent patch) with PCI support. Not calling any PCI functions
should be enough to avoid any and all interactions between libpci and
the hardware.
> The tegra2_spi code was originally contributed by NVIDIA to the Chromium
>
> OS U-Boothttp://git.chromium.org/gitweb/?p=chromiumos/third_party/u-boot.git;a=blob;f=drivers/spi/tegra2_spi.c;h=15a23124027ecdad5fcec23206a0fc67b968872d;hb=HEAD
> branch
> and was later grafted into Flashrom so we could more easily perform SPI ROM
> updates from userspace. Please excuse minor style inconsistencies as we are
> trying to stay reasonably close to the code checked into the U-Boot tree ;-)

Ah, that explains some of the quirks I saw.

Could you explain the Tegra2 hardware a bit more? The code suggests that
it is more or less a slightly intelligent bitbanging controller with a
"special" programing interface.

At a first glance, the following issues stick out:
1-3-isolate_pci_isms.patch
This essentially declares ARM a no-PCI zone. That's OK for testing, but
not for merge. What exactly is the problem with PCI probing here? Does
it return errors? Does it crash or terminate the application? Does it
mess with the machine in ways which might cause breakage?

2-3-add_arm_isms.patch
Have you checked with an architecture guru that sync_primitive() can
really be empty, not only for that arch in general, but also for
accesses to a mmapped region the way flashrom does it?

3-3-add_tegra2_spi_controller.patch
Not your fault, but we really need a way to detect the target
architecture in the Makefile. The issues of which SPI masters to include
for internal exists on all architectures. Any ideas? I experimented with
uname calls in the past, but that checks the host arch, not the target
arch. Grepping the preprocessor output for the compiler might be an option.
echo|gcc -dM -E -|grep "whatever|arch|defines"
Or you try this pretty straightforward solution:

Create a file arch.h with these contents:
#if defined (__i386__)
#define __FLASHROM_ARCH__ "x86"
#elif defined (__x86_64__)
#define __FLASHROM_ARCH__ "x86_64"
#elif defined (__mips) || defined (__mips__) || defined (_mips) ||
defined (mips)
#define __FLASHROM_ARCH__ "mips"
#elif defined(__powerpc__) || defined(__powerpc64__) || defined(__ppc__)
|| defined(__ppc64__)
#define __FLASHROM_ARCH__ "ppc"
#endif
__FLASHROM_ARCH__

Then run
gcc -E arch.h|grep -v ^#
The result on my machine will be
"x86"
Side note: Such an arch.h file (with some extensions) might save us from
having to check every possible arch define everywhere we need to
differentiate between architectures.

I will comment on the Tegra2 SPI driver later, we should get the
infrastructure questions addressed first.

Regards,
Carl-Daniel

[Patch] NVIDIA Tegra2 SPI controller support

[Stefan Reinauer]

On Tue, Jun 28, 2011 at 5:54 PM, Carl-Daniel Hailfinger
 wrote:
> > Due to issues with libpci, you can't use the same binary on machines with
> >
> > PCI and without PCI. So for example, you can't program a SPI ROM onboard a
> > PCI card that is connected to your ARM SoC.
>
> Could you explain more? IIRC some people were using flashrom on ARM
> (independent patch) with PCI support. Not calling any PCI functions
> should be enough to avoid any and all interactions between libpci and
> the hardware.

One of the problems is that if you call libpci's init function on a
system with no PCI support, libpci will not return an error but just
end the program right away. This makes it hard to have one binary that
works on PCI and non-PCI ARM systems.

Stefan

[Patch] NVIDIA Tegra2 SPI controller support

[David Hendricks]

On Tue, Jun 28, 2011 at 5:54 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> 2-3-add_arm_isms.patch

Have you checked with an architecture guru that sync_primitive() can
> really be empty, not only for that arch in general, but also for
> accesses to a mmapped region the way flashrom does it?

Nope. Scattered resources/forum posts about ARMv7 suggest that peripheral
memory is strongly ordered. Also, the U-Boot code doesn't have special
handling for sync primitives AFAICT.

I'm pretty new to ARM... Any advice from experts on this list would be
appreciated.

On Tue, Jun 28, 2011 at 5:54 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> 3-3-add_tegra2_spi_controller.patch
> Not your fault, but we really need a way to detect the target
> architecture in the Makefile. The issues of which SPI masters to include
> for internal exists on all architectures. Any ideas? I experimented with
> uname calls in the past, but that checks the host arch, not the target
> arch. Grepping the preprocessor output for the compiler might be an option.
> echo|gcc -dM -E -|grep "whatever|arch|defines"
> Or you try this pretty straightforward solution:
>
> Create a file arch.h with these contents:
> #if defined (__i386__)
> #define __FLASHROM_ARCH__ "x86"
> #elif defined (__x86_64__)
> #define __FLASHROM_ARCH__ "x86_64"
> #elif defined (__mips) || defined (__mips__) || defined (_mips) ||
> defined (mips)
> #define __FLASHROM_ARCH__ "mips"
> #elif defined(__powerpc__) || defined(__powerpc64__) || defined(__ppc__)
> || defined(__ppc64__)
> #define __FLASHROM_ARCH__ "ppc"
> #endif
> __FLASHROM_ARCH__
>
> Then run
> gcc -E arch.h|grep -v ^#
> The result on my machine will be
> "x86"
> Side note: Such an arch.h file (with some extensions) might save us from
> having to check every possible arch define everywhere we need to
> differentiate between architectures.
>
> I will comment on the Tegra2 SPI driver later, we should get the
> infrastructure questions addressed first.

I've updated this patch set on top of the other patches you've been working
on (autodetect target processor http://patchwork.coreboot.org/patch/3256/
 and the libpci hack http://patchwork.coreboot.org/patch/3255/).

The difference between this patchset and the previous one is pretty minor.
We can now discard the first patch which #ifdef'd out PCI code for ARM
targets. The remaining differences are mostly centered around the Makefile
and arch.h

New patches are:
Signed-off-by: David Hendricks