[RFC] Adding struct flashchip * to all chip functions

[Carl-Daniel Hailfinger]

Short summary: We need to add struct flashchip * to all chip functions
to avoid a big mess once we support multiprotocol capable external
programmers.

Flashrom has really nice generic routines like probe_jedec. They work
perfectly as long as flashrom does not have to care about the bus
protocol it should use to talk to the chip.
Even current external flashers (nic3com, satasii) work fine because
flashrom does not have to care about the bus protocol there either.
However, any external flasher supporting multiple bus protocols (and
such flashers are being worked on) really wants to know which protocol
to use. Unfortunately, chip_read* and chip_write* don't know about the
bus protocol.

Adding struct flashchip * as parameter to chip_read* and chip_write*
would solve the issue quite nicely and improve our abstraction at the
same time.
Of course, this means every function calling chip_read* and chip_write*
has to pass struct flashchip * to them. Ergo, we need struct flashchip *
as parameter in the whole call chain.

An alternative would be using global variables for current flash chip
properties, but that is madness during probe where we want to check for
multiple chips.

Opinions? Thoughts? Want explanations? Tell me!

Regards,
Carl-Daniel

[ron minnich]

I think you argument makes some sense Another option is the struct
flashchip *chip global variable. There are lots of arguments to go
either way.

ron

[Urja Rannikko]

> Flashrom has really nice generic routines like probe_jedec. They work
> perfectly as long as flashrom does not have to care about the bus
> protocol it should use to talk to the chip.
> Even current external flashers (nic3com, satasii) work fine because
> flashrom does not have to care about the bus protocol there either.
> However, any external flasher supporting multiple bus protocols (and
> such flashers are being worked on) really wants to know which protocol
> to use. Unfortunately, chip_read* and chip_write* don't know about the
> bus protocol.

If you're referring to my work, yes the protocol is designed so to be
able to support
many bustypes, but i have no real hardware for anything else than parallel.

And then, i realized a missing feature in my protocol, it cannot tell
the flasher of the bustype to
be used. It's only an optional command away though.
> Adding struct flashchip * as parameter to chip_read* and chip_write*
> would solve the issue quite nicely and improve our abstraction at the
> same time.
> Of course, this means every function calling chip_read* and chip_write*
> has to pass struct flashchip * to them. Ergo, we need struct flashchip *
> as parameter in the whole call chain.
>
> An alternative would be using global variables for current flash chip
> properties, but that is madness during probe where we want to check for
> multiple chips.

I think that having a simple global current_flashchip pointer would
be simpler in many ways:
- it would only need a single assign to operation to the loop at
probe_flash to support it
- it wouldnt be necessary to pass unnecessary parameters to chip_*
functions ( most dont need this info - atleast atm ;P ),
making for a smaller binary

[Carl-Daniel Hailfinger]

On 08.06.2009 19:07, Urja Rannikko wrote:
> > However, any external flasher supporting multiple bus protocols (and
> > such flashers are being worked on) really wants to know which protocol
> > to use. Unfortunately, chip_read* and chip_write* don't know about the
> > bus protocol.
>
> If you're referring to my work, yes the protocol is designed so to be
> able to support
> many bustypes, but i have no real hardware for anything else than parallel.

I was thinking of Paraflasher.
> And then, i realized a missing feature in my protocol, it cannot tell
> the flasher of the bustype to
> be used. It's only an optional command away though.

I think we have too many words for too many things.
"bus protocol" in my mail above refers to the bus tye/protocol between
flasher and flash chip.
> > struct flashchip * as parameter to chip_read* and chip_write*
> > [...]
> > using global variables for current flash chip [...]
>
> I think that having a simple global current_flashchip pointer would
> be simpler in many ways:
> it would only need a single assign to operation to the loop at
>
> probe_flash to support it

IMHO the loop would get less readable because it tries to handle
detection of multiple chips. You'd have to keep an array of found chips
which would be local, whereas the currently probed chip would be global.
After probing, the global variable would have to be overwritten by a
local variable... Not exactly my idea of fun.
> it wouldnt be necessary to pass unnecessary parameters to chip_*
>
> functions ( most dont need this info - atleast atm ;P ),
> making for a smaller binary

I doubt the binary is going to be smaller, but this suggestion does
warrant an investigation. I'll try to cook up a patch which adds struct
flashchip everywhere and measure the size differences.

Regards,
Carl-Daniel

[Urja Rannikko]

> I think we have too many words for too many things.
> "bus protocol" in my mail above refers to the bus tye/protocol between
> flasher and flash chip.

I know - i was meaning that the serial (as in RS-232) protocol i have
doesnt currently have a way to say to the flasher/programmer which
bustype (parallel/fwh/lpc/spi) it is to use - in case somebody makes a
programmer that has sockets/support for more than one of the 4
bustypes.
> > > struct flashchip * as parameter to chip_read* and chip_write*
> > > [...]
> > > using global variables for current flash chip [...]
> >
> > I think that having a simple global current_flashchip pointer would
> > be simpler in many ways:
> > it would only need a single assign to operation to the loop at
> >
> > probe_flash to support it
>
> IMHO the loop would get less readable because it tries to handle
> detection of multiple chips. You'd have to keep an array of found chips
> which would be local, whereas the currently probed chip would be global.
> After probing, the global variable would have to be overwritten by a
> local variable... Not exactly my idea of fun.

This support is exactly 3 added and 1 modified line - see my patch here.
> I doubt the binary is going to be smaller, but this suggestion does
> warrant an investigation. I'll try to cook up a patch which adds struct
> flashchip everywhere and measure the size differences.

What i was saying that the support in my patch will be smaller than
the way of adding
parameters to each call (which nobody atm uses) - i think (testing appreciated).

-- the patch (also attached) --
Add a global current_flashchip pointer.
Signed-off-by: Urja Rannikko

---
Index: flash.h
===================================================================
--- flash.h	(revision 580)
+++ flash.h	(working copy)
@@ -713,6 +713,7 @@
 void map_flash_registers(struct flashchip *flash);
 int read_memmapped(struct flashchip *flash, uint8_t *buf);
 extern char *pcidev_bdf;
+struct flashchip *current_flashchip;

/* layout.c */
 int show_id(uint8_t *bios, int size, int force);
Index: flashrom.c
===================================================================
--- flashrom.c	(revision 580)
+++ flashrom.c	(working copy)
@@ -33,6 +33,7 @@
 int exclude_start_page, exclude_end_page;
 int verbose = 0;
 int programmer = PROGRAMMER_INTERNAL;
+struct flashchip* current_flashchip;

const struct programmer_entry programmer_table[] = {
    {
@@ -247,6 +248,7 @@
    char *tmp;

for (flash = first_flash; flash && flash->name; flash++) {
+		current_flashchip = flash;
    	if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0)
    		continue;
    	printf_debug("Probing for %s %s, %d KB: ",
@@ -707,7 +709,7 @@
    	exit(1);
    }

-	flash = flashes[0];
+	current_flashchip = flash = flashes[0];

if (TEST_OK_MASK != (flash->tested & TEST_OK_MASK)) {
    	printf("===\n");