Re: [flashrom] [PATCH] split spi.c and fix include headers

[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