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

[Carl-Daniel Hailfinger]

Am 12.09.2013 17:18 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

Thanks! One nitpick, otherwise
Acked-by: Carl-Daniel Hailfinger
> diff --git a/sb600spi.c b/sb600spi.c
> index febeabb..91268c0 100644
> --- a/sb600spi.c
> +++ b/sb600spi.c
> @@ -272,6 +272,62 @@ static int sb600_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>    return 0;
>  }
>
> +struct spispeed {
> const char *const name;
> const int8_t speed;

uint8_t instead? That would avoid a type conversion in set_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;
>
> +}

Regards,
Carl-Daniel

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

[Stefan Tauner]

On Sun, 15 Sep 2013 03:07:54 +0200
Carl-Daniel Hailfinger  wrote:
> Am 12.09.2013 17:18 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
>
> Thanks! One nitpick, otherwise
> Acked-by: Carl-Daniel Hailfinger

Thanks, r1750.
> > diff --git a/sb600spi.c b/sb600spi.c
> > index febeabb..91268c0 100644
> > --- a/sb600spi.c
> > +++ b/sb600spi.c
> > @@ -272,6 +272,62 @@ static int sb600_spi_send_command(struct flashctx *flash, unsigned int writecnt,
> >    return 0;
> >  }
> >
> > +struct spispeed {
> > const char *const name;
> > const int8_t speed;
>
> uint8_t instead? That would avoid a type conversion in set_speed().

Yes, probably a typo - at least I cannot remember any rationale :)