[PATCH] split spi.c and fix include headers

[Sean Nelson]

Split spi.c into programmer and chip drivers.
Make it so flash.h doesn't include any other flashrom-related headers.
Fix includes in files.

Signed-off-by: Sean Nelson

---

Made as minimal changes as I could, and split sli.c where it was clearly 
programmer code where the rest was moved to spi25.c. Names and Ideas as 
suggested by carldani.

[Carl-Daniel Hailfinger]

On 25.02.2010 08:55, Sean Nelson wrote:
> Split spi.c into programmer and chip drivers.
> Make it so flash.h doesn't include any other flashrom-related headers.
> Fix includes in files.
>
> Signed-off-by: Sean Nelson
>
> Made as minimal changes as I could, and split sli.c where it was
> clearly programmer code where the rest was moved to spi25.c. Names and
> Ideas as suggested by carldani.

Thanks for preparing this patch. It brings us a step closer to a good
separation and abstraction.
> --- a/flash.h
> +++ b/flash.h
> @@ -17,32 +17,34 @@
> 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
>
> */
>
> #ifndef __FLASH_H__
>  #define __FLASH_H__ 1
>
> #include <unistd.h>
>  #include <stdint.h>
>  #include <stdio.h>
> -#include "hwaccess.h"
>  #ifdef _WIN32
>  #include <windows.h>
>  #undef min
>  #undef max
>  #endif
> +#if NEED_PCI == 1
> +#include <pci/pci.h>
> +#endif

Hmmm... PCI accesses are hardware as well. Can you move them to
hwaccess.h or include them where needed?

Side note: In a perfect abstraction, the SPI programmer drivers should
not know anything about flash chips. Unfortunately, this still needs a
lot of work. I have a preliminary patch, but it's not ready for
consumption yet. It's not something you need to address, though.
> --- a/spi.c
> +++ b/spi.c
> @@ -139,191 +139,26 @@ int spi_send_command(unsigned int writecnt, unsigned int readcnt,
>  }
>
> int spi_send_multicommand(struct spi_command *cmds)
>  {
>    if (!spi_programmer[spi_controller].multicommand) {
>    	fprintf(stderr, "%s called, but SPI is unsupported on this "
>    		"hardware. Please report a bug.\n", __func__);
>    	return 1;
>    }
>
> return spi_programmer[spi_controller].multicommand(cmds);
>  }
>
> -int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> 	     const unsigned char *writearr, unsigned char *readarr)
>
> -{
> struct spi_command cmd[] = {
> {
> .writecnt = writecnt,
> .readcnt = readcnt,
> .writearr = writearr,
> .readarr = readarr,
> }, {
> .writecnt = 0,
> .writearr = NULL,
> .readcnt = 0,
> .readarr = NULL,
> }};
> return spi_send_multicommand(cmd);
>
> -}
>
> -int default_spi_send_multicommand(struct spi_command *cmds)
> -{
> int result = 0;
> for (; (cmds->writecnt || cmds->readcnt) && !result; cmds++) {
> result = spi_send_command(cmds->writecnt, cmds->readcnt,
> 			  cmds->writearr, cmds->readarr);
> }
> return result;
>
> -}

The two functions above need to stay in spi.c because they are generic
programmer code, not chip driver code.
> /* support 4 bytes flash ID */
>  int probe_spi_rdid4(struct flashchip *flash)
>  {
>    /* only some SPI chipsets support 4 bytes commands */
>    switch (spi_controller) {
>  #if INTERNAL_SUPPORT == 1
>    case SPI_CONTROLLER_ICH7:
>    case SPI_CONTROLLER_ICH9:
>    case SPI_CONTROLLER_VIA:
>    case SPI_CONTROLLER_SB600:
>    case SPI_CONTROLLER_WBSIO:
>  #endif
>  #if FT2232_SPI_SUPPORT == 1
> @@ -336,811 +171,46 @@ int probe_spi_rdid4(struct flashchip *flash)
>    case SPI_CONTROLLER_BUSPIRATE:
>  #endif
>  #if DEDIPROG_SUPPORT == 1
>    case SPI_CONTROLLER_DEDIPROG:
>  #endif
>    	return probe_spi_rdid_generic(flash, 4);
>    default:
>    	printf_debug("4b ID not supported on this SPI controller\n");
>    }
>
> return 0;
>  }

And even if this function looks programmer specific, it is chip driver
code. The only reason we perform any programmer differentiation is that
we don't have a generic can_run_this_command() function which would
check if the hardware supports the command in question. I have some code
to restructure this function from back when I attempted to extend the
SPI abstraction, but the code has bitrotted quite a bit, so it might be
faster to just rewrite it.
> int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
>    if (!spi_programmer[spi_controller].read) {
>    	fprintf(stderr, "%s called, but SPI read is unsupported on this"
>    		" hardware. Please report a bug.\n", __func__);
>    	return 1;
>    }
>
> return spi_programmer[spi_controller].read(flash, buf, start, len);
>  }

Hm yes, this one is very special. I can see arguments for declaring it
both as programmer code and as chip code. IMHO it is chip code because
it implies a certain read command. OTOH, it is a very specific
programmer operation which programmers can short-circuit. It would
definitely benefit from a can_run_this_command() function.
> +uint32_t spi_get_valid_read_addr(void)
> +{
> /* Need to return BBAR for ICH chipsets. */
> return 0;
>
> }

Why did you move this function a few lines up? Just a cosmetic question
because the move makes tracking code history harder.
> new file mode 100644
> index 0000000..624e22d
> --- /dev/null
> +++ b/spi25.c

Please make sure this file is created with "svn cp spi.c spi25.c" and
then modified as needed before committing. git-svn may or may not do
that. I just want to make sure the code history stays intact.
> @@ -0,0 +1,964 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2007, 2008, 2009 Carl-Daniel Hailfinger
> Copyright (C) 2008 coresystems GmbH
> 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 generic SPI framework

Contains the common SPI chip driver functions
> */
>
> +#include <string.h>
> +#include "flash.h"
> +#include "flashchips.h"
> +#include "chipdrivers.h"
> +#include "spi.h"
>
> +void spi_prettyprint_status_register(struct flashchip *flash);
>
> +int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> 	     const unsigned char *writearr, unsigned char *readarr)
>
> +{
> struct spi_command cmd[] = {
> {
> .writecnt = writecnt,
> .readcnt = readcnt,
> .writearr = writearr,
> .readarr = readarr,
> }, {
> .writecnt = 0,
> .writearr = NULL,
> .readcnt = 0,
> .readarr = NULL,
> }};
> return spi_send_multicommand(cmd);
>
> +}
>
> +int default_spi_send_multicommand(struct spi_command *cmds)
> +{
> int result = 0;
> for (; (cmds->writecnt || cmds->readcnt) && !result; cmds++) {
> result = spi_send_command(cmds->writecnt, cmds->readcnt,
> 			  cmds->writearr, cmds->readarr);
> }
> return result;
>
> +}

The two functions above belong in spi.c (see above).
> +/* support 4 bytes flash ID */
> +int probe_spi_rems(struct flashchip *flash)

Comment doesn't belong to function. It seems something got lost in the
shuffle.

What I could not check was function ordering. Did you keep the old order
while moving code from spi.c to spi25.c? If not, please do so because it
makes tracking history a lot easier.

Looks good otherwise.

Regards,
Carl-Daniel

[Sean Nelson]

Updated against svn with suggestions.

[Carl-Daniel Hailfinger]

On 25.02.2010 21:50, Sean Nelson wrote:
> Updated against svn with suggestions.

I really like this patch, but I noticed we're doing two things at once,
and some of my suggestions (which you followed) has unintended side
effects (which are note really desirable). This is my fault and I'll
take the blame.
One part of this patch splits the include files (including all the nasty
hwaccess.h stuff), and another part makes all chip drivers use
chipdrivers.h and splits spi.c into spi.c and spi25.c.

Could you split off the chipdriver stuff (including the spi.c split) and
send it as separate patch? All the chipdriver stuff is committable and
looks fine. Here's a file list you can supply to svn diff (svn diff
takes multiple files as arguments which makes splitting easy):
82802ab.c flashchips.c jedec.c m29f400bt.c Makefile pm49fl00x.c
sharplhf00l04.c spi25.c spi.c sst28sf040.c sst49lfxxxc.c sst_fwhub.c
stm50flw0x0x.c w29ee011.c w39v040c.c w39v080fa.c
I checked that the chipdriver stuff is standalone and compiles just
fine. That part is immediately ackable.

The programmer/hwaccess stuff turned out to be more complicated than I
had hoped. I'd like to postpone that part until we know how to keep all
programmer stuff in one header file... If you want to svn revert those
files (keep the diff around, I believe the work you did was valuable),
here's a list:
board_enable.c buspirate_spi.c chipset_enable.c dediprog.c drkaiser.c
flash.h flashrom.c hwaccess.c hwaccess.h ichspi.c internal.c it87spi.c
nic3com.c pcidev.c physmap.c print.c satasii.c sb600spi.c wbsio_spi.c

Sorry for not seeing the PCI splitoff mess before.

Regards,
Carl-Daniel

[Sean Nelson]

Chipdriver and spi split patch.
Some of the spi programmer drivers required chipdrivers.h (needs fixing 
later) :
   it87spi.c
   ichspi.c
   sb600spi.c
   wbsio_spi.c
   buspirate_spi.c

Signed-off-by: Sean Nelson

[Sean Nelson]

Chipdriver and spi split patch.
Some of the spi programmer drivers required chipdrivers.h (needs fixing 
later) :
   it87spi.c
   ichspi.c
   sb600spi.c
   wbsio_spi.c
   buspirate_spi.c

Signed-off-by: Sean Nelson

[Carl-Daniel Hailfinger]

On 26.02.2010 03:07, Sean Nelson wrote:
> Chipdriver and spi split patch.

Thanks for doing this work, it gets us closer to our goal.
> Some of the spi programmer drivers required chipdrivers.h (needs
> fixing later) :
>   it87spi.c
>   ichspi.c
>   sb600spi.c
>   wbsio_spi.c
>   buspirate_spi.c

You forgot ft2232spi.c and bitbang_spi.c and dediprog.c, probably
because they are not compiled by default.
> Signed-off-by: Sean Nelson

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[Sean Nelson]

> You forgot ft2232spi.c and bitbang_spi.c and dediprog.c, probably
> because they are not compiled by default.

Patch fixed for mailinglist.

---

Split spi.c into programmer and chip code
Remove chipdriver.h from flash.h
Some of the spi programmer drivers required chipdrivers.h, needs fixing 
later:
   it87spi.c
   ichspi.c
   sb600spi.c
   wbsio_spi.c
   buspirate_spi.c
   ft2232spi.c
   bitbang_spi.c
   dediprog.c

Signed-off-by: Sean Nelson 
Acked-by: Carl-Daniel Hailfinger

[Sean Nelson]

On 2/25/10 9:47 PM, Sean Nelson wrote:
> Signed-off-by: Sean Nelson 
> Acked-by: Carl-Daniel Hailfinger

Thanks for the Ack. Committed in r914.