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

[Stefan Tauner]

On Sat, 17 Sep 2011 20:58:41 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > todo:
> >  - revise hwseq activation: when should hwseq be used, when swseq?
> >    in the current version i have added a programmer parameter to tell flashrom when one
> >    wants to use hwseq _if it is available_ (i.e. descriptor valid), example:
> > flashrom -p internal:laptop=force_I_want_a_brick,hwseq=yes -VV
> >    this needs to be documented in the man page if it gets merged like that.
> >  - commit message (is a link to my blog post a good idea (additionally)?)
> 
> We have no limits for the commit message size, but maybe we should start
> to package additional documentation with flashrom?

hard decision to make. my view about smaller bits of information
(about 1-2 paragraphs):
1. just make commit logs verbose
++ context
+ in the repository
~ ok-ish accessibility via trac et al.
- very low visibility
-- not editable

2. blog posts/mails/wiki entries
++ high visibility
+ accessibility
~ editable by authors (except for mails)
-- not in the repository/easily deprecated
-- usually no code context

3. Documentation folder
++ full VCS support
+ keeps the code from getting cluttered
~ accessibility
-- no context/not navigable because it is unstructured

4. documentation directly in the source
++ full VCS support
+ context: important code is in direct vicinity, but unrelated stuff too
- accessibility because the big picture is spread
- less code in view

for bigger stuff like the complete description of descriptors, hardware
sequencing or ME unlocking the values change a lot.
context becomes less important, versioning and changeability to,
because the big picture is not hurt by minor improvements of
understanding. also, long paragraphs inside the code might by frown
upon by readers/devs.

in my other/private projects i usually have long(er) descriptions at
the top of each file that explains the bigger picture and this is also
what i miss most when i delve into a foreign code base. this can also
be done in a Documentations folder, although i do not seem an apparent
need to split it that way. the only advantage is smaller source files.
having those big chunks in the wiki or in blog posts is a viable
alternative imho, because it has a better visibility (also attracts
search engines). the disadvantage is that those entries are easily
forgotten to be kept in sync. but this is less of an issue for "the big
picture" information. i am undecided on where to put bigger chunks. but
it should probably be coherently in one medium and not spread over
mails, wiki and blog entries etc. (like it is now).

> >  - revise (levels of) prints
> >  - revise timeout values (for optimal performance)
> >  - build upon the upcoming opaque interface framework instead
> 
> I see all those TODOs, and most of them should be addressed before
> enabling the code. That's why I'd like to get most of this patch merged
> really soon except for the few places where the new code is hooked up.
> That way we get a much smaller remaining patch and the discussion about
> user interfaces and infrastructure are not attached to a megapatch, but
> rather to a small "hook up hwseq" patch.

i see your point, but i am a bit skeptical still. let's see how this
works out.

> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> > ---
> > diff --git a/ich_descriptors.c b/ich_descriptors.c
> > index ce40d9f..8076b09 100644
> > --- a/ich_descriptors.c
> > +++ b/ich_descriptors.c
> > @@ -214,6 +214,41 @@ void prettyprint_ich_descriptor_master(const struct ich_desc_master *mstr)
> >  	msg_pdbg2("\n");
> >  }
> >  
> > +/** Returns the integer representation of the component density with index
> > +idx in bytes or 0 if a correct size can not be determined. */
> > +int getFCBA_component_density(uint8_t idx, const struct ich_descriptors *desc)
> 
> I'd prefer to have the order (desc, idx) for the parameters here.

done

> > +{
> > +	uint8_t size_enc;
> > +	static const int const dec_mem[6] = {
> > +		      512 * 1024,
> > +		 1 * 1024 * 1024,
> > +		 2 * 1024 * 1024,
> > +		 4 * 1024 * 1024,
> > +		 8 * 1024 * 1024,
> > +		16 * 1024 * 1024,
> > +	};
> 
> Kill dec_mem and replace dec_mem[size_enc] with (1<<(19+size_enc)).

oh :)
good point

> > +
> > +	switch(idx) {
> > +	case 0:
> > +		size_enc = desc->component.comp1_density;
> > +		break;
> > +	case 1:
> > +		if (desc->content.NC == 0)
> 
> If NC is shorthand for "not connected", this looks like inverted logic.

one click away in any sane editor is:
				 NC	:2, /* Number Of Components */
;)
which is 0-based btw, hence NC == 0 -> only one device -> density of
component with index 1: 0

> 
> > +			return 0;
> > +		size_enc = desc->component.comp2_density;
> > +		break;
> > +	default:
> > +		msg_perr("Only component index 0 or 1 are supported yet.\n");
> 
> More specific error message: "Only ICH SPI component..."
> 
> 
> > +		return 0;
> > +	}
> > +	if (size_enc > 5) {
> > +		msg_perr("Density of component with index %d is invalid. "
> 
> Same error message change here.

since we are no longer limited by the deleted array and the future
development of those bits (if any) is pretty obvious, should we drop
the return 0 and change the message to a warning? the densities have 3
bits, so value 6 and 7 (32 and 64 MB) are reserved atm.
> 
>  
> > +			 "Encoded density is 0x%x.\n", idx, size_enc);
> > +		return 0;
> > +	}
> > +	return dec_mem[size_enc];
> > +}
> > +
> >  static uint32_t read_descriptor_reg(uint8_t section, uint16_t offset, void *spibar)
> >  {
> >  	uint32_t control = 0;
> > diff --git a/ich_descriptors.h b/ich_descriptors.h
> > index b1da914..8ab798f 100644
> > --- a/ich_descriptors.h
> > +++ b/ich_descriptors.h
> > @@ -255,7 +255,8 @@ void prettyprint_ich_descriptor_region(const struct ich_descriptors *desc);
> >  void prettyprint_ich_descriptor_master(const struct ich_desc_master *master);
> >  
> >  int read_ich_descriptors_via_fdo(void *spibar, struct ich_descriptors *desc);
> > -
> > +int getFCBA_component_density(uint8_t idx, const struct ich_descriptors *desc);
> > + 
> >  #endif /* BITFIELDS == 1 */
> >  #endif /* __ICH_DESCRIPTORS_H__ */
> >  #endif /* defined(__i386__) || defined(__x86_64__) */
> > diff --git a/ichspi.c b/ichspi.c
> > index 2e25d34..61c0825 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -26,7 +26,10 @@
> >  #if defined(__i386__) || defined(__x86_64__)
> >  
> >  #include <string.h>
> > +#include <stdlib.h>
> >  #include "flash.h"
> > +#include "flashchips.h"
> > +#include "chipdrivers.h"
> 
> Including flashchips.h or chipdrivers.h from a programmer driver is a
> sign that something went wrong during the design phase.

that's a bit oversimplified imo. there is no way to implement this
without some major changes to flashrom and this is exactly what i tried
to accomplish. and i don't have a problem with changing this if the
opaque framework is ready/available.

> >  #include "programmer.h"
> >  #include "spi.h"
> >  #include "ich_descriptors.h"
> > @@ -1084,6 +1087,298 @@ static int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> >  	return result;
> >  }
> >  
> > +static struct hwseq {
> > +	uint32_t size_comp0;
> > +	uint32_t size_comp1;
> > +} hwseq;
> > +
i have postponed the struct for now. it is used to communicate the
component sizes from the init function to the probing function. both
parts are not in the first patch.

> > +static void ich_hwseq_set_addr(uint32_t addr)
> > +{
> > +	uint32_t addr_old = REGREAD32(ICH9_REG_FADDR) & ~0x01FFFFFF;
> > +	REGWRITE32(ICH9_REG_FADDR, (addr & 0x01FFFFFF) | addr_old);
> 
> Can this write fail somehow?

not really, but the address could be truncated, if it is > 0x01FFFFFF.
and of course setting it to some address > total size of the chips
might have funny effects. the question is where to deal with this. imho
not in this function but its callers.
in the first patch this is only ich_hwseq_get_erase_block_size which
has no persistence effects. i have expanded the comment of
ich_hwseq_get_erase_block to indicate that the address must be valid
and added a description of ich_hwseq_set_addr.

> > +}
> > +
> > +/* Sets FADDR.FLA to 'addr' and returns the erase block size in bytes
> > + * of the block containing this address. */
> 
> Is it really possible to encode per-region block sizes in a descriptor?

not per FREG-defined "region" but per FPB-defined flash partition.
i have added an explanation of this.

> > +static uint32_t ich_hwseq_get_erase_block_size(unsigned int addr)
> > +{
> > +	uint8_t enc_berase;
> > +	static const uint32_t const dec_berase[4] = {
> 
> decode_ instead of dec_. dec_ sounds too much like decrement. Or you
> pick a completely different name, e.g. eraseblock_sizes.

really? together with another variable that has the prefix enc_? hm.
what about "encoded" and "decoded". it is pretty clear that we are
doing some stuff with erase block(s/sizes) so that is redundant anyway.

> 
> > +		256,
> > +		4 * 1024,
> > +		8 * 1024,
> > +		64 * 1024
> > +	};
> > +
> > +	ich_hwseq_set_addr(addr);
> > +	enc_berase = (REGREAD16(ICH9_REG_HSFS) & HSFS_BERASE) >>
> 
> HSFS_BERASE only has two adjacent bits set, correct?

jup:
#define HSFS_BERASE		(0x3 << HSFS_BERASE_OFF)

> 
> > +		     HSFS_BERASE_OFF;
> > +	return dec_berase[enc_berase];
> > +}
> > +
> > +/* Polls for Cycle Done Status, Flash Cycle Error or timeout in 10 us intervals.
> 
> Not something to be addressed in this patch, but we should really think
> about exponential backoff with maximum wait for such wait-for-hardware
> calls.

have we talked about this already? i can remember thinking about this
already too. at least specifying one (big) initial delay and then some
finer grained querying intervals would be a good idea.
hm actually both is needed. a initial delay and a exponential backing
off delay.

> > +   Resets all error flags in HSFS.
> > +   Returns 0 if the cycle completes successfully without errors within
> > +   timeout us, 1 on errors. */
> > +static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout,
> 
> Can we specify the timeout in usecs instead and use programmer_delay(8)
> to avoid division overhead (use shift instead)?

please explain, which division?
hm probably the non-existent one that should be there to really do what
the comment says :)

i have added this (and changed the 10 to 8 everywhere):
timeout >>= 3; /* equals /= 8; scale timeout duration to counter */
should i drop the shifting because the compiler will do it anyway and
it is better readable?

> > +					     unsigned int len)
> > +{
> > +	uint16_t hsfs;
> > +	uint32_t addr;
> > +
> > +	while ((((hsfs = REGREAD16(ICH9_REG_HSFS)) &
> > +		 (HSFS_FDONE | HSFS_FCERR)) == 0) &&
> 
> Can we really ignore AEL here?

we never care for AEL specifically, because it is just an additional
flag that indicates the reason for a set FCERR afaik.

> 
> > +	       --timeout) {
> > +		programmer_delay(10);
> > +	}
> > +	REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
> 
> This clears all errors. OK.
jup

> > +	if (!timeout) {
> > +		addr = REGREAD32(ICH9_REG_FADDR) & 0x01FFFFFF;
> > +		msg_perr("Timeout error between offset 0x%08x and "
> > +			 "0x%08x + %d (=0x%08x)!\n",
> 
> Please don't mix hex and decimal encoding.

please think about this twice. imho this form gives the most readable
output. addresses are easier to read as hex, but lengths are easier to
grasp in decimal (at least for me). the base is clearly visible for all
values too.

if you insist on this i would suggest removing the length value
completely and only use "Timeout error between offset 0x%08x and
0x%08x!\n". that's less information though and will not increase
readability imo.

> > +			 addr, addr, len - 1, addr + len - 1);
> > +		prettyprint_ich9_reg_hsfs(hsfs);
> > +		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC));
> > +		return 1;
> > +	}
> > +
> > +	if (hsfs & HSFS_FCERR) {
> > +		addr = REGREAD32(ICH9_REG_FADDR) & 0x01FFFFFF;
> > +		msg_perr("Transaction error between offset 0x%08x and "
> > +			 "0x%08x (=0x%08x + %d)!\n",
> 
> hex/decimal mixture.

ditto

> > +			 addr, addr + len - 1, addr, len - 1);
> > +		prettyprint_ich9_reg_hsfs(hsfs);
> > +		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC));
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> > +
> 
> ich_hwseq_probe, ich_hwseq_send_command, ich_hwseq_block_erase,
> ich_hwseq_read and ich_hwseq_write256 use an interface which does not
> fit well with flashrom. Could you cut out those functions and post them
> separately, preferably as part of the hookup patch I mentioned above?

done

> > +static const struct spi_programmer spi_programmer_ich_hwseq = {
> > +	.type = SPI_CONTROLLER_ICH_HWSEQ,
> > +	.max_data_read = 64,
> > +	.max_data_write = 64,
> > +	.command = ich_hwseq_send_command,
> > +	.multicommand = default_spi_send_multicommand,
> > +	.read = ich_hwseq_read,
> > +	.write_256 = ich_hwseq_write_256,
> > +};
> 
> Move the hunk above to the hookup patch as well.

likewise

> > +
> >  int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >  			int ich_generation)
> >  {
> > @@ -1214,21 +1519,20 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >  	uint8_t old, new;
> >  	uint16_t spibar_offset, tmp2;
> >  	uint32_t tmp;
> > +	char *arg;
> >  	int ichspi_desc = 0;
> > +	int hwseq_en = 0;
> >  
> >  	switch (ich_generation) {
> >  	case 7:
> > -		register_spi_programmer(&spi_programmer_ich7);
> >  		spibar_offset = 0x3020;
> >  		break;
> >  	case 8:
> > -		register_spi_programmer(&spi_programmer_ich9);
> >  		spibar_offset = 0x3020;
> >  		break;
> >  	case 9:
> >  	case 10:
> >  	default:		/* Future version might behave the same */
> > -		register_spi_programmer(&spi_programmer_ich9);
> >  		spibar_offset = 0x3800;
> >  		break;
> >  	}
> > @@ -1239,8 +1543,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >  	/* Assign Virtual Address */
> >  	ich_spibar = rcrb + spibar_offset;
> >  
> > -	switch (spi_programmer->type) {
> > -	case SPI_CONTROLLER_ICH7:
> > +	switch (ich_generation) {
> > +	case 7:
> 
> Please make sure the VIA driver doesn't explode with this.

afaics VIA driver directly calls via_init_spi and never ich_spi_init.
it registers its own programmer there. i can't see how it would be
affected.

from a quick look i think we should extract a ich_init_spi_7 (or so)
from ich_init_spi and combine it with via_init_spi. this should be done
together with the ich7 cleanup/macro definition patch.

> >  		msg_pdbg("0x00: 0x%04x     (SPIS)\n",
> >  			     mmio_readw(ich_spibar + 0));
> >  		msg_pdbg("0x02: 0x%04x     (SPIC)\n",
> > @@ -1277,9 +1581,21 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >  			ichspi_lock = 1;
> >  		}
> >  		ich_set_bbar(ich_generation, 0);
> > +		register_spi_programmer(&spi_programmer_ich7);
> >  		ich_init_opcodes();
> >  		break;
> > -	case SPI_CONTROLLER_ICH9:
> > +	case 8:
> > +	case 9:
> > +	case 10:
> > +	default:		/* Future version might behave the same */
> 
> OK until here.
> 
> 
> > +		arg = extract_programmer_param("hwseq");
> > +		if (arg && !strcmp(arg, "yes"))
> > +			hwseq_en = 1;
> > +		else if (arg && !strlen(arg))
> > +			msg_pinfo("Missing argument for hwseq.\n");
> > +		else if (arg)
> > +			msg_pinfo("Unknown argument for hwseq: %s\n", arg);
> > +		free(arg);
> 
> Please move the programmer parameter evaluation to the hookup patch.

gone

> >  		tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS);
> >  		msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
> >  		prettyprint_ich9_reg_hsfs(tmp2);
> > @@ -1363,18 +1679,37 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >  
> >  		msg_pdbg("\n");
> >  #if BITFIELDS == 1
> 
> #if BITFIELDS needs to die.

it is already dead, but you wanted me to post the first patch of the
series only...

> > +		{ /* to be able to declare desc here and avoid another
> > +		   * "#if BITFIELDS == 1" on top
> > +		   */
> > +		struct ich_descriptors desc = {{ 0 }};
> >  		if (ichspi_desc) {
> 
> Maybe rename ichspi_desc to ichspi_desc_active or ichspi_desc_enabled.

if at all it should be _valid because this is used all over the
datasheet.
and the ichspi_ prefix is pretty useless since it is a local variable.
have changed this to desc_valid. is that ok with you?
>  
> > -			struct ich_descriptors desc = {{ 0 }};
> >  			if (read_ich_descriptors_via_fdo(ich_spibar, &desc) ==
> >  			    RET_OK)
> >  				prettyprint_ich_descriptors(CHIPSET_ICH_UNKNOWN,
> >  							    &desc);
> > +			/* If the descriptor is valid and indicates multiple
> > +			 * flash devices we need to use hwseq to be able to
> > +			 * access the second flash device.
> > +			 */
> > +			if (desc.content.NC != 0)
> > +				hwseq_en = 1;
> > +		} else
> > +			hwseq_en = 0; /* disable it even if the user said yes */
> 
> This case needs at least a msg_pinfo, maybe even msg_perr. We act
> contrary to the wishes of the user.

true.

> > +
> > +		if (hwseq_en > 0) {
> > +			hwseq.size_comp0 = getFCBA_component_density(0, &desc);
> > +			hwseq.size_comp1 = getFCBA_component_density(1, &desc);
> > +			register_spi_programmer(&spi_programmer_ich_hwseq);
> 
> Please move register_spi_programmer to the hookup patch. The rest of the
> code of this hunk should stay in this patch.

hm.. i have removed hwseq_en and the struct already... and i think it
would be better off in the hookup patch anyway. this actually IS the
hookup code :)

> > +		register_spi_programmer(&spi_programmer_ich9);
> >  		ich_init_opcodes();
> > -		break;
> > -	default:
> > -		/* Nothing */
> > +#endif /* BITFIELDS == 1 */
> >  		break;
> >  	}
> >  
> > diff --git a/programmer.h b/programmer.h
> > index 6a28dbe..6e316d1 100644
> > --- a/programmer.h
> > +++ b/programmer.h
> > @@ -517,6 +517,7 @@ enum spi_controller {
> >  #if defined(__i386__) || defined(__x86_64__)
> >  	SPI_CONTROLLER_ICH7,
> >  	SPI_CONTROLLER_ICH9,
> > +	SPI_CONTROLLER_ICH_HWSEQ,
> 
> Please move this to the hookup patch.
gone.

i had to add #if 0 around the unreachable code to not break compilation.
i would rather keep the patch out of the repository until we can merge
the hookup code too. if i wont change it, there is no need to review it
again then and since it only touches ich-specific stuff and files it
will be easy to keep it rebased to HEAD even outside subversion. do you
want me to mail in the new patch set (which also includes an unchanged
ich_descriptor_tool patch in addition to the two hwseq patches) and
how do we continue from here?
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner