[flashrom] [PATCH 2/6] sbxxx: Set SPI clock to 16.5 MHz and disable fast reads.

[Stefan Tauner]

On Wed, 14 Aug 2013 08:56:27 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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.

> > +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?

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner