[flashrom] [PATCH 2/2] ichspi: add support for Intel Hardware Sequencing

[Stefan Tauner]

On Mon, 07 Nov 2011 00:15:53 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 26.10.2011 15:35 schrieb Stefan Tauner:
> > Based on the new opaque programmer framework this patch adds support
> > for Intel Hardware Sequencing on ICH8 and its successors.
> >
> > By default (or when setting the ich_spi_mode option to auto)
> > the module tries to use swseq and only activates hwseq if need be:
> > - if important opcodes are inaccessible due to lockdown
> > - if more than one flash chip is attached.
> > The other options (swseq, hwseq) select the respective mode (if possible).
> >
> > A general description of Hardware Sequencing can be found in this blog entry:
> > http://blogs.coreboot.org/blog/2011/06/11/gsoc-2011-flashrom-part-1/
> >
> > TODO: adding real documentation when we have a directory for it
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> 
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
thanks, but ignored for now.

> with a few small comments.
in contrast to those of course ;)

> > ---
> >  flashrom.8 |   20 +++++
> >  ichspi.c   |  268 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 283 insertions(+), 5 deletions(-)
> >
> > diff --git a/flashrom.8 b/flashrom.8
> > index a8f4660..66cde4f 100644
> > --- a/flashrom.8
> > +++ b/flashrom.8
> > @@ -303,6 +303,26 @@ is the I/O port number (must be a multiple of 8). In the unlikely case
> >  flashrom doesn't detect an active IT87 LPC<->SPI bridge, please send a bug
> >  report so we can diagnose the problem.
> >  .sp
> > +If you have an Intel chipset with an ICH8 or later southbridge with SPI flash
> > +attached, and if a valid descriptor was written to it (e.g. by the vendor), the
> > +chipset provides an alternative way to access the flash chip(s) named
> > +.BR "Hardware Sequencing" .
> > +It is much simpler than the normal access method (called
> > +.BR "Software Sequencing" "),"
> > +but does not allow the software to choose the SPI commands to be sent.
> > +You can use the
> > +.sp
> > +.B "  flashrom \-p internal:ich_spi_mode=value"
> > +.sp
> > +syntax where value can be
> > +.BR auto ", " swseq " or " hwseq .
> > +By default
> > +.RB "(or when setting " ich_spi_mode=auto )
> > +the module tries to use swseq and only activates hwseq if need be (e.g. if
> > +important opcodes are inaccessible due to lockdown; or if more than one flash
> > +chip is attached). The other options (swseq, hwseq) select the respective mode
> > +(if possible).
> > +.sp
> >  If you have an Intel chipset with an ICH6 or later southbridge and if you want
> >  to set specific IDSEL values for a non-default flash chip or an embedded
> >  controller (EC), you can use the
> > diff --git a/ichspi.c b/ichspi.c
> > index bc03554..1d5dd34 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -575,6 +575,25 @@ static int program_opcodes(int ich_generation, OPCODES *op, int enable_undo)
> >  	return 0;
> >  }
> >  
> > +/* Returns true if the most important opcodes are accessible. */
> 
> You assume that some erase opcode will be available if BYTE_PROGRAM is
> available. I think that assumption is reasonable, but it could be
> documented in this comment above the function.

this was just a basis for discussion actually. sorry for not explaining
that bit. there are more problems than just the missing check for *any*
available erase opcode.

i do not intend to fix this soon. OTOH it is not really a problem imho.
if this trigger would enable hwseq the bios has probably not only
locked down the opcodes, but also set up PR or FREG/FRAP protections,
which we can not deal with correctly yet, not even with hwseq. so *not*
selecting hwseq due to a too simple ich_check_opcodes method will not
hurt us, because it would not have saved us from failure anyway.

i have reduced the method to the absolute minimum (see patch) and
added the following comment:
 * FIXME: this should also check for
 *   - at least one probing opcode (RDID (incl. AT25F variants?), REMS, RES?)
 *   - at least one erasing opcode (lots.)
 *   - at least one program opcode (BYTE_PROGRAM, AAI_WORD_PROGRAM, ...?)
 *   - necessary preops? (EWSR, WREN, ...?)

> > […]
> > +static const struct opaque_programmer opaque_programmer_ich_hwseq = {
> > +	.max_data_read = 64,
> > +	.max_data_write = 64,
> > +	.probe = ich_hwseq_probe,
> > +	.read = ich_hwseq_read,
> > +	.write = ich_hwseq_write,
> 
> Please add ich_hwseq_block_erase here.

done

> > […]
> > +		arg = extract_programmer_param("ich_spi_mode");
> > +		if (arg && !strcmp(arg, "hwseq")) {
> > +			ich_spi_mode = ich_hwseq;
> > +			msg_pspew("user selected hwseq\n");
> > +		} else if (arg && !strcmp(arg, "swseq")) {
> > +			ich_spi_mode = ich_swseq;
> > +			msg_pspew("user selected swseq\n");
> > +		} else if (arg && !strcmp(arg, "auto")) {
> > +			msg_pspew("user selected auto\n");
> > +			ich_spi_mode = ich_auto;
> > +		} else if (arg && !strlen(arg))
> > +			msg_pinfo("Missing argument for ich_spi_mode.\n");
> > +		else if (arg)
> > +			msg_pinfo("Unknown argument for ich_spi_mode: %s\n", arg);
> 
> We should return an error all the way up to programmer init for both
> cases (and clean up everything). I know that this is a complicated code
> path, so if you decide not to fail programmer init here, please add a
> comment like the one below:
> /* FIXME: Return an error to programmer_init. */

fixing this was not that easy, because the ich code in chipset_enable.c
was not passing the fatal error further; see patch.

> > […]
> > +		if (ich_spi_mode == ich_hwseq) {
> > +			if (!desc_valid) {
> > +				msg_perr("Hardware sequencing was requested "
> > +					 "but the flash descriptor is not "
> > +					 "valid. Aborting.\n");
> > +				return 1;  
> 
> Can you check that this indeed causes flashrom to return an error from
> programmer_init? Chipset init IIRC ignores most errors unless they are
> somehow declared to be fatal.

done. and also if a bogus ich_gen is passed to ichspi_init at the
beginning of the function.

i am not sure if it was due to the changes to the previous patches, but
ich_init_opcodes() were missing from the ich8+ code path (again).

besides that i have also added null-pointer guards to find_opcode and
find_preop, because i think it matches the other opcode methods better
(curopcodes == NULL has some meaning and is actively used/checked in
other functions).

ps: you like to abort when the user gets the command line wrong for
safety. there is a case we do not handle in this manner. for example
with this patch set one can do:
flashrom -p internal:laptop=force_I_want_a_brick,ich_spi_mode
it prints "Unhandled programmer parameters: ich_spi_mode" but
continues, you may wanna look into this.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-squash-ichspi-add-support-for-Intel-Hardware-Sequenc.patch
Type: text/x-patch
Size: 5855 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20111107/e4878a14/attachment.patch>