Re: [flashrom] [PATCH 1/6] sbxxx: Add detection for the remaining AMD chipset families.

[Carl-Daniel Hailfinger]

Hello everyone,

we have stumbled upon something in the AMD southbridge docs which we
don't understand:
SPI controller register SPI_Cntrl0 (address spibar + 0x0), bit 28. In
various datasheets it is called either SpiClkGate or DropOneClkOnRd.

What exactly does that bit do?
1. Does it stop the SPI clock for the duration of one clock cycle (i.e.
one bit) after sending the last bit on the MOSI line (thereby
effectively doubling the time between last bit sent to the flash chip
and the first bit from the flash chip)?
2. Does it stop the SPI clock for the duration of 8 clock cycles (i.e.
one byte)?
3. Does it send out a dummy byte between last data bit on MOSI and first
data bit on MISO to allow easier implementation of SPI FAST READ commands?
4. Does it do something different entirely?

The code in flashrom inside sb600spi.c which we're working on is
reproduced below, together with some context:

Am 11.09.2013 15:51 schrieb Stefan Tauner:
> On Tue, 10 Sep 2013 09:31:13 +0200 (CEST)
> "Carl-Daniel U. Hailfinger"  wrote:
> > [Sorry for breaking the threading. This is meant to be a reply to
> > http://patchwork.coreboot.org/patch/4016/ ]
> > Stefan Tauner wrote on 2013-08-14 17:35:13
> > > On Wed, 14 Aug 2013 08:42:15 +0200
> > > Carl-Daniel Hailfinger  wrote:
> > > > Am 10.08.2013 03:45 schrieb Stefan Tauner:
> > > > > tmp = mmio_readl(sb600_spibar + 0x00);
> > > > > msg_pdbg("(0x%08" PRIx32 ") SpiArbEnable=%i", tmp, (tmp >> 19) & 0x1);
> > > > > msg_pdbg(", SpiAccessMacRomEn=%i, SpiHostAccessRomEn=%i, ArbWaitCount=%i",
> > > > >      (tmp >> 22) & 0x1, (tmp >> 23) & 0x1, (tmp >> 24) & 0x7);
> > > > > if (amd_gen != CHIPSET_YANGTZE)
> > > > >     msg_pdbg(", SpiBridgeDisable=%i", (tmp >> 27) & 0x1);
> > > > > switch (amd_gen) {
> > > > > case CHIPSET_SB7XX:
> > > > >     msg_pdbg(", DropOneClkOnRd/SpiClkGate=%i", (tmp >> 28) & 0x1);
> > > >
> > > > Should we check that bit and set it to 0? I haven't yet been able to
> > > > find a good explanation of what it does, but it seems to be a trick to
> > > > allow normal read commands at higher speed. I could be mistaken, though.
> > >
> > > I would refrain from changing it if we don't really know what it does
> > > (and we probably will never do so...). I have read the description a
> > > few times... and it does not make a lot of sense to me.
> >
> > Maybe our friends at AMD/Sage can explain this?
>
> One would have to ask them ;)

Any insights are appreciated.

Regards,
Carl-Daniel