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

[Carl-Daniel Hailfinger]

Am 10.08.2013 03:45 schrieb Stefan Tauner:
> Do not rely on broken firmware to set up the SPI configuration correctly.
> Some boards fail with flashrom because the firmware chose too high speeds
> for the alternate SPI mode which flashrom uses. Temporarily change the
> clock to the lowest common value of 16.5 MHz.
>
> Also, disable fast reads just to be safe.
>
> Signed-off-by: Stefan Tauner

General comments:
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?

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.
> 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 },
>
> +};
>
> +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));
> }
> if (!success) {
> msg_perr("Setting SPI clock failed.\n");
> return 1;
> }
> return 0;
>
> +}
>
> +static int handle_speed(struct pci_dev *dev)
> +{
> uint32_t tmp;
> int8_t spispeed_idx = 3; /* Default to 16.5 MHz */
> /* bit   6xx   7xx/SP5100  8xx             9xx  hudson1  hudson234  yangtze
> * 18    rsvd  <-          fastReadEnable  ?    <-       ?          SpiReadMode[0]
> * 29:30 rsvd  <-          <-              ?    <-       ?          SpiReadMode[2:1]
> */
> if (amd_gen != CHIPSET_YANGTZE) {
> if (amd_gen >= CHIPSET_SB89XX && amd_gen <= CHIPSET_HUDSON234) {
> 	bool fast_read = (mmio_readl(sb600_spibar + 0x00) >> 18) & 0x1;
> 	msg_pdbg("Fast Reads are %sabled\n", fast_read ? "en" : "dis");
> 	if (fast_read) {
> 		msg_pdbg("Disabling them temporarily.\n");
> 		rmmio_writel(mmio_readl(sb600_spibar + 0x00) & ~(0x1 << 18),
> 			     sb600_spibar + 0x00);
> 	}
> }
> tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3;
> msg_pdbg("NormSpeed is %s\n", spispeeds[tmp].name);
> }
> return set_speed(dev, &spispeeds[spispeed_idx]);
>
> +}
>
> static int sb600_handle_imc(struct pci_dev *dev, bool amd_imc_force)
>  {
>    /* Handle IMC everywhere but sb600 which does not have one. */
> @@ -322,9 +377,6 @@ int sb600_probe_spi(struct pci_dev *dev)
>    uint32_t tmp;
>    uint8_t reg;
>    bool amd_imc_force = false;
>
> static const char *const speed_names[4] = {
> "66/reserved", "33", "22", "16.5"
>
> };
>
> char *arg = extract_programmer_param("amd_imc_force");
> if (arg && !strcmp(arg, "yes")) {
>
> @@ -441,9 +493,6 @@ int sb600_probe_spi(struct pci_dev *dev)
>    	return ERROR_NONFATAL;
>    }
> tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3;
> msg_pdbg("NormSpeed is %s MHz\n", speed_names[tmp]);
> if (amd_gen >= CHIPSET_SB89XX) {
>   tmp = mmio_readb(sb600_spibar + 0x1D);
>   msg_pdbg("Using SPI_CS%d\n", tmp & 0x3);
>
> @@ -488,6 +537,9 @@ int sb600_probe_spi(struct pci_dev *dev)
>    	return 0;
>    }
> if (handle_speed(dev) != 0)
> return ERROR_FATAL;
> if (sb600_handle_imc(dev, amd_imc_force) != 0)
>   return ERROR_FATAL;

Regards,
Carl-Daniel

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