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

[Carl-Daniel U. Hailfinger]

[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  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

Regards,
Carl-Daniel