Re: [flashrom] [PATCH] Generic programmer registration: final part

[Carl-Daniel Hailfinger]

Am 19.12.2011 22:57 schrieb Michael Karcher:
> Am Montag, den 19.12.2011, 02:38 +0100 schrieb Carl-Daniel Hailfinger:
> > > I suggest to drop noop_chip_readb/noop_chip_writeb at the same time as
> > > we drop par_programmer_none, as these functions are no longer used.
> >
> > noop_chip_readb dropped.
> > noop_chip_writeb may be useful if we merge a programmer driver which
> > does not support write.
> > What do you think?
>
> If we find a way to revive the "blind read via force", good idea. On the
> other hand, I think a opaque mmap dumper would be more apt for the blind
> read. It should just get the chip size and the optionally start or end
> address of the mapping as parameter.

I'll leave the #if 0 forced read code in cli_classic.c until we have a
blind read solution again.
> > > > msg_pdbg("This programmer supports the following protocols: %s.\n",
> > > >    	 tempstr);
> > >
> > > It's definitely not "this programmer", as should be obvious if you "kept
> > > in mind" as I told you that get_buses_supportet returns the union of all
> > > programmers.
> >
> > I beg to differ. This is a message to the end user, and the end user
> > does not think of a mainboard as multiple programmers.
>
> As on IRC: "The following protocols are supported".

Done.
> > ichspi.c:
> > Use ich_generation instead of spi_programmer->type (or
> > flash->pgm->spi.type) in run_opcode to determine if the programmer was
> > initialized correctly.
> > This change is debatable because it uses a local static variable instead
> > of the info available in flash->pgm, but OTOH now the internal usage is
> > consistent.
>
> consistency first - we can eliminate the local variable later anyway
> (and we are going to, I expect)

OK.
> > Another question is whether we should really check for
> > correct initialization at this point.
>
> I guess the checking makes no sense anymore, as there is no way to enter
> this function without going through the init function.

Hm yes, it would only serve as a check against buggy init functions.
Looking at one of the recent ICH init additions in r1430 where new code
was committed because it worked, but it was not really integrated well
with existing ICH init code.
> > The programmer registration functions now return error/success instead
> > of void, but their callers don't care. Fix now or later?
>
> Fix later. We already inform the user if we are losing programmers,
> that's enough for now.

OK.
> > TODO?
> > max_decode is a programmer property, add it to the
> >
> > register_*_programmer parameters
> > programmer_may_write is a programmer property, add it to the
> >
> > register_*_programmer parameters
>
> Yes, both of them should be added to the programmer structure. I am
> unsure whether we want rarely used options (like may-not-write) as
> parameters though. Having the size there is good - it makes the
> programmer think about the maximum supported size.

Postponed.
> > All programmer types (Parallel, SPI, Opaque) now register themselves
> > into a generic programmer list and probing is now programmer-centric
> > instead of chip-centric.
>
> TODO: split the flash chip list into SPI and non-SPI lists. No need to
> iterate over the other type and skip those chips because of bus
> mismatches. Do it later.

Postponed, but given that flash chips may support multiple buses (some
weird currently unsupported models even speak SPI and parallel), the
reordering is not something I want to do.
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Michael Karcher

Thanks a lot for the review, committed in r1475 with the changes
mentioned during review.
> > int bitbang_spi_shutdown(const struct bitbang_spi_master *master)
> >  {
> > if (!bitbang_spi_master) {
> > if (!master) {
> > msg_perr("Shutting down an uninitialized SPI bitbang master!\n"
> > 	 "Please report a bug at [e-mail]\n");
> > return 1;
>
> As I just found out, bitbang_spi_shutdown is never called at all, but if
> it will be called, it will be a callback registered in bitbang_spi_init
> which already checked master for nullness. Also the function could be
> made static in that case.

Unfortunately, register_shutdown() takes a non-const void *parameter,
and that causes warnings if it is called from bitbang_spi_init with
const struct bitbang_spi_master *. Made static inside #if 0 for now
until register_shutdown is changed.
> > msg_pdbg("This programmer supports the following protocols: %s.\n",
> >    	 tempstr);
>
> Message change: see intro.
> > if (!pgm->probe || !pgm->read || !pgm->write || !pgm->erase) {
> >    	msg_perr("%s called with one of probe/read/write/erase being "
> >    		 "NULL. Please report a bug at [e-mail]\n",
> >    		 __func__);
> > return;
> > return ERROR_FLASHROM_BUG;
>
> Later, we could add a feature that allows read-only "programmers" that
> have write and erase NULL, setting programmer_may_write to FALSE.

Or we check for programmer_may_write==FALSE and then skip the
erase/write member check.
> > -void register_spi_programmer(const struct spi_programmer *pgm)
> > +int register_spi_programmer(const struct spi_programmer *pgm)
> >  {
> > spi_programmer = pgm;
> > buses_supported |= BUS_SPI;
> > struct registered_programmer rpgm;
> > if (!pgm->write_256 || !pgm->read || !pgm->command ||
> >    !pgm->multicommand ||
> >    ((pgm->command == default_spi_send_command) &&
> >     (pgm->multicommand == default_spi_send_multicommand))) {
> > msg_perr("%s called with inconsistent settings. "
>
> s/inconsistent settings/incomplete programmer definition/
> > if (!chip_writeb || !chip_writew || !chip_writel || !chip_writen ||
> >    !chip_readb || !chip_readw || !chip_readl || !chip_readn) {
> > msg_perr("%s called with one of probe/read/write/erase being "
>
> dito.

Done.