Re: [flashrom] [PATCH 3/3] kill central list of SPI programmers

[Carl-Daniel Hailfinger]

Am 05.05.2011 03:04 schrieb Michael Karcher:
> Remove the array spi_programmer, replace it by dynamic registration
> instead. Also initially start with no busses supported, and switch to
> the default non-SPI only for the internal programmer.
>
> Signed-off-by: Michael Karcher

Thanks for your patch!
I have a few minor nitpicks and design questions, but the code changes 
seem to be bug-free, so you get an
Acked-by: Carl-Daniel Hailfinger

Your patch is very readable and got me thinking about future directions 
of that code, especially multiple registrations... so take the review 
with a grain of salt.
> diff --git a/dummyflasher.c b/dummyflasher.c
> index c4c9c4e..fdaa5f2 100644
> --- a/dummyflasher.c
> +++ b/dummyflasher.c
> @@ -339,7 +339,7 @@ static int emulate_spi_chip_response(unsigned int writecnt, unsigned int readcnt
>   }
>   #endif
>
> -int dummy_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> +static int dummy_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    	      const unsigned char *writearr, unsigned char *readarr)
>   {
>    int i;
> @@ -375,12 +375,22 @@ int dummy_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    return 0;
>   }
>
> -int dummy_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
> +static int dummy_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
>   {
>    return spi_write_chunked(flash, buf, start, len,
>    			 spi_write_256_chunksize);
>   }
>
> +static const struct spi_programmer spi_programmer_dummyflasher = {
>    .type = SPI_CONTROLLER_DUMMY,
>    .max_data_read = MAX_DATA_READ_UNLIMITED,
>    .max_data_write = MAX_DATA_UNSPECIFIED,

256 instead?
>    .command = dummy_spi_send_command,
>    .multicommand = default_spi_send_multicommand,
>    .read = default_spi_read,
>    .write_256 = dummy_spi_write_256,

Same comment as for patch 1 of the series. AFAICS dummy_spi_write_256 
should be replaced by the default one.
> +};
> int dummy_init(void)
> {
>  char *bustext = NULL;

Please fix the whitespace (spaces instead of tabs) in it87spi.c and 
dummyflasher.c.
Apparently a newline is missing at the end of spi.c.
> diff --git a/bitbang_spi.c b/bitbang_spi.c
> index be30944..ca28434 100644
> --- a/bitbang_spi.c
> +++ b/bitbang_spi.c
> @@ -81,7 +81,7 @@ static uint8_t bitbang_spi_readwrite_byte(uint8_t val)
>    return ret;
>   }
>
> -int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> +static int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    	const unsigned char *writearr, unsigned char *readarr)
>   {
>    int i;
> @@ -106,6 +106,16 @@ int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    return 0;
>   }
>
> +static const struct spi_programmer spi_programmer_bitbang = {

Can you make this one non-const to allow changing .type?
> .type = SPI_CONTROLLER_BITBANG,

And leave type empty, to be filled by bitbang_spi_init()?
> .max_data_read = MAX_DATA_READ_UNLIMITED,
> .max_data_write = MAX_DATA_WRITE_UNLIMITED,
> .command = bitbang_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = default_spi_read,
> .write_256 = default_spi_write_256,
>
> +};
> int bitbang_spi_init(const struct bitbang_spi_master *master, int halfperiod)

Could you extend this function signature to have an additional enum 
spi_controller parameter? If each bitbang-using programmer calls 
bitbang_spi_init(spi_controller, master, halfperiod) we can keep the 
spi_controller enums unchanged for now which would help us keep things 
consistent (for example, ichspi.c uses 3 different spi_controller enums 
for the same functions).
> {
>    /* BITBANG_SPI_INVALID is 0, so if someone forgot to initialize ->type,

Insert in this function:
spi_programmer_bitbang.type=spi_controller;
> diff --git a/programmer.h b/programmer.h
> index fefb9cd..53a50f6 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -527,7 +518,6 @@ enum spi_controller {
>    SPI_CONTROLLER_SB600,
>    SPI_CONTROLLER_VIA,
>    SPI_CONTROLLER_WBSIO,
> SPI_CONTROLLER_MCP6X_BITBANG,
> #endif
> #endif
> #if CONFIG_FT2232_SPI == 1
>
> @@ -542,16 +532,9 @@ enum spi_controller {
>   #if CONFIG_DEDIPROG == 1
>    SPI_CONTROLLER_DEDIPROG,
>   #endif
> -#if CONFIG_RAYER_SPI == 1
> SPI_CONTROLLER_RAYER,
>
> -#endif
> -#if CONFIG_NICINTEL_SPI == 1
> SPI_CONTROLLER_NICINTEL,
>
> -#endif
> -#if CONFIG_OGP_SPI == 1
> SPI_CONTROLLER_OGP,
>
> +#if CONFIG_OGP_SPI == 1 || CONFIG_NICINTEL_SPI == 1 || CONFIG_RAYER_SPI == 1 || (CONFIG_INTERNAL == 1&&  (defined(__i386__) || defined(__x86_64__)))
> SPI_CONTROLLER_BITBANG,
> #endif
> SPI_CONTROLLER_INVALID /* This must always be the last entry. */
> };
> extern const int spi_programmer_count;

If you use enum spi_controller as parameter for bitbang_spi_init(), we 
don't need an extra SPI_CONTROLLER_BITBANG and can keep the 
programmer-specific ones.

The calls to bitbang_spi_init() in mcp6x_spi.c, nicintel_spi.c, 
ogp_spi.c and rayer_spi.c would need to be adjusted as well.

I plan to add a union private_data{} to struct spi_programmer to allow 
storing stuff like struct bitbang_spi_master, avoiding the need for 
programmer-internal static configuration variables.

Regards,
Carl-Daniel

[PATCH 3/3] kill central list of SPI programmers

[Michael Karcher]

Am Freitag, den 06.05.2011, 00:40 +0200 schrieb Carl-Daniel Hailfinger:
> Thanks for your patch!
> I have a few minor nitpicks and design questions, but the code changes 
> seem to be bug-free, so you get an
> Acked-by: Carl-Daniel Hailfinger

Thanks for the Ack, but I think we need one iteration of discussion.
> Your patch is very readable and got me thinking about future directions 
> of that code, especially multiple registrations... so take the review 
> with a grain of salt.

OK, I keep thinking of multiple registrations...
> > +static const struct spi_programmer spi_programmer_dummyflasher = {
> >    .type = SPI_CONTROLLER_DUMMY,
> >    .max_data_read = MAX_DATA_READ_UNLIMITED,
> >    .max_data_write = MAX_DATA_UNSPECIFIED,
>
> 256 instead?

I used UNSPECIFIED because I didn't use the generic write function (for
whatever reason). I am inclined to use "MAX_DATA_WRITE_UNLIMITED"
though, as the dummy flasher does not have any hardware limits on
maximal write size.
> >    .command = dummy_spi_send_command,
> >    .multicommand = default_spi_send_multicommand,
> >    .read = default_spi_read,
> >    .write_256 = dummy_spi_write_256,
>
> Same comment as for patch 1 of the series. AFAICS dummy_spi_write_256 
> should be replaced by the default one.

If we patch the structure to avoid the global variable
"spi_write_256_chunksize", you are right.
> Please fix the whitespace (spaces instead of tabs) in it87spi.c and 
> dummyflasher.c.

Argh, I am going to kill my "smart" editor. It tries to guess
white-space type from the file contents, and usually fails.
> > +static const struct spi_programmer spi_programmer_bitbang = {
>
> Can you make this one non-const to allow changing .type?

No, I can't. If I think of multiple registrations, I also think of
multiple bit-banging adapter. I can of course have bitbang_spi_init()
make a copy and patch type in that.
> Could you extend this function signature to have an additional enum 
> spi_controller parameter? If each bitbang-using programmer calls 
> bitbang_spi_init(spi_controller, master, halfperiod) we can keep the 
> spi_controller enums unchanged for now which would help us keep things 
> consistent (for example, ichspi.c uses 3 different spi_controller enums 
> for the same functions).

The controller type enum is used as far as I can see to take care of
limits of certains SPI hosts. As the limits (number of read/written
bytes, whether there is a lowest accessible address and so on) are the
same for all bitbang programmers, so I consider having one type for all
bitbang programmers to be superior to having different types.

In the long run, I suggest to get rid of the type alltogether. Better to
have feature flags instead like "SPI_MASTER_CAN_DO_SPI".
> I plan to add a union private_data{} to struct spi_programmer to allow 
> storing stuff like struct bitbang_spi_master, avoiding the need for 
> programmer-internal static configuration variables.

I thought of either "void *private_data", so the generic does not have
to know about the types of private_data, or having derived structures
that start with a struct spi_programmer and cast them in the programmer
implementation to the derived type while registering them as the first
object, like this:

struct ich_spi_programmer {
    struct spi_programmer spipgm;
    unsigned int spi_bar;
}

int ich_spi_init()
{
    [...]
    register_spi_programmer(&ich_spi_programmer.spipgm);
    [...]
}

int ich_spi_send_command(struct spi_programmer * spipgm, ...)
{
    struct ich_spi_programmer * pgm = (struct ich_spi_programmer*)spipgm;
    [...]
}

Of course your idea gets rid of the cast, and the size of the flashrom
project is small enough that we don't have to care about the generic
code depend on all the specific programmer types. While I still prefer
the "more OO like" approach I pointed out, I have no problem with the
union approach, too.

Thanks for your review,
  Michael Karcher

[PATCH 3/3] kill central list of SPI programmers

[Carl-Daniel Hailfinger]

Am 06.05.2011 13:30 schrieb Michael Karcher:
> Am Freitag, den 06.05.2011, 00:40 +0200 schrieb Carl-Daniel Hailfinger:
> > Thanks for your patch!
> > I have a few minor nitpicks and design questions, but the code changes
> > seem to be bug-free, so you get an
> > Acked-by: Carl-Daniel Hailfinger
>
> Thanks for the Ack, but I think we need one iteration of discussion.
> > Your patch is very readable and got me thinking about future directions
> > of that code, especially multiple registrations... so take the review
> > with a grain of salt.
>
> OK, I keep thinking of multiple registrations...
> > > +static const struct spi_programmer spi_programmer_dummyflasher = {
> > >    .type = SPI_CONTROLLER_DUMMY,
> > >    .max_data_read = MAX_DATA_READ_UNLIMITED,
> > >    .max_data_write = MAX_DATA_UNSPECIFIED,
> >
> > 256 instead?
>
> I used UNSPECIFIED because I didn't use the generic write function (for
> whatever reason). I am inclined to use "MAX_DATA_WRITE_UNLIMITED"
> though, as the dummy flasher does not have any hardware limits on
> maximal write size.

Right. However, generic write calls spi_write_chunked which calls 
spi_nbyte_program which will cause a stack overflow for writes larger 
than 256 bytes. That's why I'd like to keep the 256 byte limit for now.
> > >    .command = dummy_spi_send_command,
> > >    .multicommand = default_spi_send_multicommand,
> > >    .read = default_spi_read,
> > >    .write_256 = dummy_spi_write_256,
> >
> > Same comment as for patch 1 of the series. AFAICS dummy_spi_write_256
> > should be replaced by the default one.
>
> If we patch the structure to avoid the global variable
> "spi_write_256_chunksize", you are right.

The chunk size accepted by the simulated flash chip and the chunk size 
accepted by the dummy programmer are identical anyway, so killing that 
variable indeed makes sense.
> > > +static const struct spi_programmer spi_programmer_bitbang = {
> >
> > Can you make this one non-const to allow changing .type?
>
> No, I can't. If I think of multiple registrations, I also think of
> multiple bit-banging adapter. I can of course have bitbang_spi_init()
> make a copy and patch type in that.

And that's the interesting question. Should the bitbanging core register 
a SPI programmer or should each individual driver do it? The first 
variant needs copying/patching, the second variant creates "duplicated" 
(copy-pasted-modified) code. I don't have a strong preference in either 
direction.
> > Could you extend this function signature to have an additional enum
> > spi_controller parameter? If each bitbang-using programmer calls
> > bitbang_spi_init(spi_controller, master, halfperiod) we can keep the
> > spi_controller enums unchanged for now which would help us keep things
> > consistent (for example, ichspi.c uses 3 different spi_controller enums
> > for the same functions).
>
> The controller type enum is used as far as I can see to take care of
> limits of certains SPI hosts.

Limits, but also stuff like which MMIO address to use. It's controller 
private data and should be opaque to the spi registration core.
> As the limits (number of read/written
> bytes, whether there is a lowest accessible address and so on) are the
> same for all bitbang programmers, so I consider having one type for all
> bitbang programmers to be superior to having different types.
>
> In the long run, I suggest to get rid of the type alltogether.

Fully agreed.
> Better to have feature flags instead like "SPI_MASTER_CAN_DO_SPI".

Sorry, -ENOPARSE.
> > I plan to add a union private_data{} to struct spi_programmer to allow
> > storing stuff like struct bitbang_spi_master, avoiding the need for
> > programmer-internal static configuration variables.
>
> I thought of either "void *private_data", so the generic does not have
> to know about the types of private_data,

void *private_data is the best choice. I retract my union suggestion.
> or having derived structures
> that start with a struct spi_programmer and cast them in the programmer
> implementation to the derived type while registering them as the first
> object, like this:
>
> struct ich_spi_programmer {
>    struct spi_programmer spipgm;
>    unsigned int spi_bar;
> }
>
> int ich_spi_init()
> {
>    [...]
>    register_spi_programmer(&ich_spi_programmer.spipgm);
>    [...]
> }
>
> int ich_spi_send_command(struct spi_programmer * spipgm, ...)
> {
>    struct ich_spi_programmer * pgm = (struct ich_spi_programmer*)spipgm;
>    [...]
> }
>
> Of course your idea gets rid of the cast, and the size of the flashrom
> project is small enough that we don't have to care about the generic
> code depend on all the specific programmer types. While I still prefer
> the "more OO like" approach I pointed out, I have no problem with the
> union approach, too.

Which one is the "more OO like approach"? Pointer or derived struct 
casting? I prefer the pointer variant.  And yes, union was a bad idea.
> Thanks for your review,

Thank you for the patch and for the excellent response.

Regards,
Carl-Daniel

[PATCH 3/3] kill central list of SPI programmers

[Michael Karcher]

Am Freitag, den 06.05.2011, 22:13 +0200 schrieb Carl-Daniel Hailfinger:
> > I used UNSPECIFIED because I didn't use the generic write function (for
> > whatever reason). I am inclined to use "MAX_DATA_WRITE_UNLIMITED"
> > though, as the dummy flasher does not have any hardware limits on
> > maximal write size.
>
> Right. However, generic write calls spi_write_chunked which calls 
> spi_nbyte_program which will cause a stack overflow for writes larger 
> than 256 bytes. That's why I'd like to keep the 256 byte limit for now.

MAX_DATA_WRITE_UNLIMITED happens to be 256 to "keep the 256 byte limit
for now". So don't worry about that.
> > If we patch the structure to avoid the global variable
> > "spi_write_256_chunksize", you are right.
>
> The chunk size accepted by the simulated flash chip and the chunk size 
> accepted by the dummy programmer are identical anyway, so killing that 
> variable indeed makes sense.

This is wrong. The simulated flash chip accepts emu_max_byteprogram_size
byte chunks, while the programmer accepts spi_write_256_chunksize. But
this variable is indeed redundant, when we use a patched programmer info
structure that contains the maximum write chunk size.
> > > > +static const struct spi_programmer spi_programmer_bitbang = {
> > >
> > > Can you make this one non-const to allow changing .type?
> >
> > No, I can't. If I think of multiple registrations, I also think of
> > multiple bit-banging adapter. I can of course have bitbang_spi_init()
> > make a copy and patch type in that.
>
> And that's the interesting question. Should the bitbanging core register 
> a SPI programmer or should each individual driver do it?

My current patch lets the bitbanging core register the SPI driver. I
consider this advantageous, because this less redundant code and a more
clean layer separation (spi core <-> bitbang core <-> bitbang driver)
where each layer only communicates with the adjacent layers.
> The first variant needs copying/patching,

Only if we really need to have the different bitbang adapter types in
the spi_controller structure which I am still not convinced in.
> > The controller type enum is used as far as I can see to take care of
> > limits of certains SPI hosts.
>
> Limits, but also stuff like which MMIO address to use. It's controller 
> private data and should be opaque to the spi registration core.

As we are planning to provide a nice "private data" element soon, and
controllers are going to use the private data structure for MMIO base
address and the like, an opaque "controller type" in the public
structure seems to make no sense. Probably we look at it from different
points of views. From my point of view, namely the lower-level SPI
drivers, I am envisioning the end of controller_type quite soon. In the
low-level drivers, it seems to be used in ichspi only, so the discussion
of the use of this field as private ingo for other low-level drivers
(e.g. for deciding whether we need a single "bitbang" or many different
types seems pointless. I am still tending towards a single "BITBANG"
type. Low-level doesn't need more specific info and high-level probably
doesn't care what type of bitbang host it is.
> > In the long run, I suggest to get rid of the type alltogether.
>
> Fully agreed.
> > Better to have feature flags instead like "SPI_MASTER_CAN_DO_SPI".
>
> Sorry, -ENOPARSE.

Of course. I hope it parses with SPI_MASTER_CAN_DO_AAI.
> void *private_data is the best choice. I retract my union suggestion.

Fine. Will not go into this patch, though, put in a later patch.

Regards,
  Michael Karcher

[PATCH 3/3] kill central list of SPI programmers

[Carl-Daniel Hailfinger]

Am 11.05.2011 10:49 schrieb Michael Karcher:
> Am Freitag, den 06.05.2011, 22:13 +0200 schrieb Carl-Daniel Hailfinger:
> > > I used UNSPECIFIED because I didn't use the generic write function (for
> > > whatever reason). I am inclined to use "MAX_DATA_WRITE_UNLIMITED"
> > > though, as the dummy flasher does not have any hardware limits on
> > > maximal write size.
> >
> > Right. However, generic write calls spi_write_chunked which calls
> > spi_nbyte_program which will cause a stack overflow for writes larger
> > than 256 bytes. That's why I'd like to keep the 256 byte limit for now.
>
> MAX_DATA_WRITE_UNLIMITED happens to be 256 to "keep the 256 byte limit
> for now". So don't worry about that.

OK.
> > > If we patch the structure to avoid the global variable
> > > "spi_write_256_chunksize", you are right.
> >
> > The chunk size accepted by the simulated flash chip and the chunk size
> > accepted by the dummy programmer are identical anyway, so killing that
> > variable indeed makes sense.
>
> This is wrong. The simulated flash chip accepts emu_max_byteprogram_size
> byte chunks, while the programmer accepts spi_write_256_chunksize. But
> this variable is indeed redundant, when we use a patched programmer info
> structure that contains the maximum write chunk size.

Right, I incorrectly remembered this part.
> > > > > +static const struct spi_programmer spi_programmer_bitbang = {
> > > >
> > > > Can you make this one non-const to allow changing .type?
> > >
> > > No, I can't. If I think of multiple registrations, I also think of
> > > multiple bit-banging adapter. I can of course have bitbang_spi_init()
> > > make a copy and patch type in that.
> >
> > And that's the interesting question. Should the bitbanging core register
> > a SPI programmer or should each individual driver do it?
>
> My current patch lets the bitbanging core register the SPI driver. I
> consider this advantageous, because this less redundant code and a more
> clean layer separation (spi core<->  bitbang core<->  bitbang driver)
> where each layer only communicates with the adjacent layers.

Given that we'll kill the programmer type variable anyway and all 
programmer instance settings will be handled with a private pointer in 
struct spi_programmer, this is pretty much irrelevant. Just go ahead 
with your version.
> > The first variant needs copying/patching,
>
> Only if we really need to have the different bitbang adapter types in
> the spi_controller structure which I am still not convinced in.

Postponed this decision.
> > > The controller type enum is used as far as I can see to take care of
> > > limits of certains SPI hosts.
> >
> > Limits, but also stuff like which MMIO address to use. It's controller
> > private data and should be opaque to the spi registration core.
>
> As we are planning to provide a nice "private data" element soon, and
> controllers are going to use the private data structure for MMIO base
> address and the like,

Right, but I'd like to keep some data in local static variables if the 
programmer can't be instantiated more than once. That said, we'll decide 
about this issue in the next patch.
> an opaque "controller type" in the public
> structure seems to make no sense. Probably we look at it from different
> points of views. From my point of view, namely the lower-level SPI
> drivers, I am envisioning the end of controller_type quite soon. In the
> low-level drivers, it seems to be used in ichspi only, so the discussion
> of the use of this field as private ingo for other low-level drivers
> (e.g. for deciding whether we need a single "bitbang" or many different
> types seems pointless. I am still tending towards a single "BITBANG"
> type. Low-level doesn't need more specific info and high-level probably
> doesn't care what type of bitbang host it is.

Agreed. A private discussion cleared this up.
> Better to have feature flags instead like "SPI_MASTER_CAN_DO_SPI".
> > Sorry, -ENOPARSE.
>
> Of course. I hope it parses with SPI_MASTER_CAN_DO_AAI.

Right.
> > void *private_data is the best choice. I retract my union suggestion.
>
> Fine. Will not go into this patch, though, put in a later patch.

OK.

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[PATCH 3/3] kill central list of SPI programmers

[Michael Karcher]

Am Mittwoch, den 11.05.2011, 17:27 +0200 schrieb Carl-Daniel Hailfinger:
> Acked-by: Carl-Daniel Hailfinger

Thanks for the review and the discussion. This is r1299.

Regards,
  Michael Karcher