Re: [flashrom] [PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Wed, 08 Feb 2012 01:28:20 +0100
Carl-Daniel Hailfinger  wrote:
> Am 08.02.2012 00:38 schrieb Stefan Tauner:
> > On Fri, 03 Feb 2012 02:13:56 +0100
> > Carl-Daniel Hailfinger  wrote:
> > > Am 31.01.2012 06:59 schrieb Stefan Tauner:
> > > > /* If there is a valid 4k value in the last double words,
> > > >  * we want to override the value from double word 1, hence force
> > > >  * skipping its processing: */
> > > > if (bsize == 4 * 1024)
> > > > 	opcode_4k = 0xFF;
> > >
> > > Not really. What happens if there are multiple valid opcodes for 4k
> > > erase? Such chips do exist IIRC.
> >
> > hm... the standard with its wording "the 4k opcode" and my previous
> > experience led to this...
> > > What about
> > > if (bsize == 4 * 1024) {
> > >   if (tmp8 == opcode_4k)
> > >     opcode_4k == 0xFF;
> > >   else
> > >     msg_cdbg("More than one 4kB eraser opcode found: 0x%02x and
> > > 0x%02x.", tmp8, opcode_4k);
> > > }
> >
> > if multiple (different) 4k eraser opcodes are ok why should we log it
> > explicitly then?
>
> Because it contradicts your reading of the standard and your experience?

contradicting my previous (and now falsified) reading and my apparently
lacking experience... ;)
> > hm. and it would probably be better to enhance sfdp_add_uniform_eraser
> > to check for duplicates before adding a new one, and maybe even
> > introducing a distinct return value for this. if the sfdp table
> > specifies duplicates it would be justified to abort imo.
> > it would of course also make the 4k opcode handling of the first double
> > word easier because we could just add it immediately.
>
> It would just move the 4k special case handling to a different function,
> though.

there were two reasons why i implemented the 4k opcode of dw#1 in such
a manner:
1.: because the data structure is different, hence it needs to be
different to the loop for dw#8+.
2.: because i never thought of two *different* 4k opcodes to be both
valid and possibly useful.

1. can not be circumvented entirely of course, but checking for
duplicate erasers would limit the speciality to the different SFDP
address.
2.: was just wrong as you convinced me...

so... i no longer see a reason to handle it that differently. how did
we end up on reversed sides in this discussion? :)
> > > > +static int sfdp_fetch_pt(struct flashctx *flash, uint32_t addr, uint8_t *buf, uint16_t len)
> > > > +{
> > > > uint16_t i;
> > > > if (spi_sfdp(flash, addr, buf, len)) {
> > > > msg_cerr("Receiving SFDP parameter table failed.\n");
> > > > return 1;
> > > > }
> > > > msg_cspew("  Parameter table contents:\n");
> > > > for(i = 0; i < len; i++) {
> > > > if ((i % 8) == 0) {
> > > > 	msg_cspew("    0x%03x: ", i);
> > > > }
> > > > msg_cspew(" 0x%02x", buf[i]);
> > > > if ((i % 8) == 7) {
> > > > 	msg_cspew("\n");
> > > > 	continue;
> > > > }
> > > > if ((i % 8) == 3) {
> > > > 	msg_cspew(" ");
> > > > 	continue;
> > > > }
> > > > }
> > > > msg_cspew("\n");
> > >
> > > Do we have some generic hexdump() function? I agree that dumping the
> > > parameter table contents may make sense, but open-coding your own
> > > hexdump is probably not the best idea. Do we want this hexdump
> > > functionality at all, and if yes, should it be factored out?
> >
> > not that i know of... OTOH it is just a few lines. if we generalize the
> > function the way i imagine it right now, every call would also need a
> > few lines due to the number of parameter to customize the output ;)
>
> Point taken. But please don't prefix 0x for individual values. That is
> really superfluous.

gone.