[Sorry for breaking the threading. This is a reply to http://patchwork.coreboot.org/patch/4011/ ]
Stefan Tauner wrote on 2013-08-14 17:44:19 > > On Wed, 14 Aug 2013 08:56:27 +0200 > Carl-Daniel Hailfinger <[email protected]> wrote: > > > Am 10.08.2013 03:45 schrieb Stefan Tauner: > > > diff --git a/sb600spi.c b/sb600spi.c > > > index c9be44c..2c1d400 100644 > > > --- a/sb600spi.c > > > +++ b/sb600spi.c > > > @@ -272,6 +272,61 @@ static int sb600_spi_send_command(struct flashctx > > > *flash, unsigned int writecnt, > > > return 0; > > > } > > > > > > +struct spispeed { > > > + const char *const name; > > > + const int8_t speed; > > > +}; > > > + > > > +static const struct spispeed spispeeds[] = { > > > + { "66 MHz", 0x00 }, > > > + { "33 MHz", 0x01 }, > > > + { "22 MHz", 0x02 }, > > > + { "16.5 MHz", 0x03 }, > > > +}; > > > I see struct spispeed and think it is a somewhat odd construct. The > > speed member is just the array index stored in the corresponding array > > member. Do you envision speed values which are not an array index? > > I want to merge the various frequency setting functions eventually and > Dediprog for example uses non-linear values. Hence I'd rather use this > unnecessary scheme for easier refactoring later. OK. > > > +static int set_speed(struct pci_dev *dev, const struct spispeed > > > *spispeed) > > > +{ > > > + bool success = false; > > > + uint8_t speed = spispeed->speed; > > > + > > > + msg_pdbg("Setting SPI clock to %s (0x%x).\n", spispeed->name, speed); > > > + if (amd_gen != CHIPSET_YANGTZE) { > > > + rmmio_writeb((mmio_readb(sb600_spibar + 0xd) & ~(0x3 << 4)) | > > > (speed << 4), sb600_spibar + 0xd); > > > + success = (speed == ((mmio_readb(sb600_spibar + 0xd) >> 4) & > > > 0x3)); > > > The code reads spispeed and then sets 16 MHz regardless of whether it's > > already 16 MHz. I also seem to remember that you were opposed to setting > > this speed permanently and advocated to restore it on shutdown. > > This should not be done unconditionally, right, I will change this. > But it is done (like the (un)setting of fast speed also) with the > r*-mmio functions so it should be reverted on shutdown AFAIK? Indeed, I overlooked the r* prefix. Given that a repost of part 1 of this series would be nice, I'll go out on a limb and ask you to repost this one after the changes as well. With the change you mentioned, this is Acked-by: Carl-Daniel Hailfinger <[email protected]> Regards, Carl-Daniel _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
