[flashrom] [PATCH] Fix VIA VX*** support.

[Stefan Tauner]

On Wed, 01 Aug 2012 00:16:05 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 31.07.2012 20:52 schrieb Stefan Tauner:
> >  
> > +	internal_buses_supported = BUS_LPC | BUS_FWH;
> 
> It's not that easy. Some older VIA chipsets do not support ROMs on
> LPC/FWH, or LPC/FWH at all. And unfortunately, not all VIA chipsets
> allow you to read the ROM type straps. Please kill this hunk for now.

killed.

> VT8231 can speak Parallel/LPC
> VT8233 can speak Parallel/LPC
> VT8233A ???
> VT8235 can speak Parallel/LPC/FWH
> VT8237A ???
> VT8237R can speak LPC/FWH (and is identical to VT8237)
> VT8237S can speak LPC/FWH/SPI
> CX700 can speak LPC/FWH
> VX[89]* can speak LPC/FWH/SPI
> 
> Adding that information to the chipset enable functions should be done
> in a separate patch. I can do that if you want... it's pretty tricky.

if you have a good idea how to do it, please go ahead!

> > +			/* Offset 4 - Bit 0 holds SPI Bus1 Enable Bit. */
> > +			spi_cntl = mmio_readl(mmio_base_physmapped) + 0x04;
> > +			if ((spi_cntl & 0x01) == 1)
> > +				msg_pdbg2("SPI Bus1 is enabled too.\n");
> 
> We don't handle that anywhere, so it should probably spit out some
> really loud warning.

why? afaik it can not boot from it and is probably used as simple data
storage (if at all :)
maybe we should use dbg instead of dbg2 so that we notice it easier,
but the only reason why i have added it, was to know if supporting this
programmer makes any sense at all.

> > +
> > +			physunmap(mmio_base_physmapped, SPI_CNTL_LEN);
> > +			break;
> > +		default:
> > +			msg_perr("%s: Unsupported chipset %x:%x!\n", __func__, dev->vendor_id, dev->device_id);
> > +			return ERROR_FATAL;
> > +	}
> > +
> > +	return via_init_spi(dev, spi0_mm_base);
> > +}
> > +
> > +static int enable_flash_vt8237s_spi(struct pci_dev *dev, const char *name)
> 
> This function needs to die.

i didnt/dont touch it because i dont know where the straps are (as
noted in the wiki)...

> > +{
> > +	return via_init_spi(dev, pci_read_long(dev, 0xbc) << 8);
> > +}
> > +
> >  static int enable_flash_cs5530(struct pci_dev *dev, const char *name)
> >  {
> >  	uint8_t reg8;
> > @@ -1263,7 +1322,7 @@ const struct penable chipset_enables[] = {
> >  	{0x1106, 0x0586, OK, "VIA", "VT82C586A/B",	enable_flash_amd8111},
> >  	{0x1106, 0x0596, OK, "VIA", "VT82C596",		enable_flash_amd8111},
> >  	{0x1106, 0x0686, OK, "VIA", "VT82C686A/B",	enable_flash_amd8111},
> > -	{0x1106, 0x3074, OK, "VIA", "VT8233",		enable_flash_vt823x},
> > +	{0x1106, 0x3074, OK, "VIA", "VT8233/VT8237R",	enable_flash_vt823x},
> 
> No, that's very likely just a datasheet bug (compare table 4 in VT8237R
> datasheet 2.06 to the values mentioned in the detailed register
> description for the Bus Control device). Please undo this hunk.

*shrug* i have changed the *37 name instead:
	{0x1106, 0x3227, OK, "VIA", "VT8237(R)",	enable_flash_vt823x},

> 
> >  	{0x1106, 0x3147, OK, "VIA", "VT8233A",		enable_flash_vt823x},
> >  	{0x1106, 0x3177, OK, "VIA", "VT8235",		enable_flash_vt823x},
> >  	{0x1106, 0x3227, OK, "VIA", "VT8237",		enable_flash_vt823x},
> > @@ -1271,8 +1330,9 @@ const struct penable chipset_enables[] = {
> >  	{0x1106, 0x3372, OK, "VIA", "VT8237S",		enable_flash_vt8237s_spi},
> >  	{0x1106, 0x8231, NT, "VIA", "VT8231",		enable_flash_vt823x},
> >  	{0x1106, 0x8324, OK, "VIA", "CX700",		enable_flash_vt823x},
> > -	{0x1106, 0x8353, OK, "VIA", "VX800/VX820",	enable_flash_vt8237s_spi},
> > -	{0x1106, 0x8409, OK, "VIA", "VX855/VX875",	enable_flash_vt823x},
> > +	{0x1106, 0x8353, NT, "VIA", "VX800/VX820",	enable_flash_vt_vx},
> > +	{0x1106, 0x8409, NT, "VIA", "VX855/VX875",	enable_flash_vt_vx},
> > +	{0x1106, 0x8410, NT, "VIA", "VX900",		enable_flash_vt_vx},
> >  	{0x1166, 0x0200, OK, "Broadcom", "OSB4",	enable_flash_osb4},
> >  	{0x1166, 0x0205, OK, "Broadcom", "HT-1000",	enable_flash_ht1000},
> >  	{0x17f3, 0x6030, OK, "RDC", "R8610/R3210",	enable_flash_rdc_r8610},
> > diff --git a/ichspi.c b/ichspi.c
> > index 0223ae3..44d659b 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -1844,19 +1844,16 @@ static const struct spi_programmer spi_programmer_via = {
> >  	.write_aai = default_spi_write_aai,
> >  };
> >  
> > -int via_init_spi(struct pci_dev *dev)
> > +int via_init_spi(struct pci_dev *dev, uint32_t mmio_base)
> >  {
> > -	uint32_t mmio_base;
> >  	int i;
> >  
> > -	mmio_base = (pci_read_long(dev, 0xbc)) << 8;
> > -	msg_pdbg("MMIO base at = 0x%x\n", mmio_base);
> > -	ich_spibar = physmap("VT8237S MMIO registers", mmio_base, 0x70);
> > +	ich_spibar = physmap("VIA SPI MMIO registers", mmio_base, 0x70);
> > +	/* Do we really need no write enable? Like the LPC one at D17F0 0x40 */
> >  
> > -	/* Not sure if it speaks all these bus protocols. */
> > -	internal_buses_supported = BUS_LPC | BUS_FWH;
> >  	ich_generation = CHIPSET_ICH7;
> >  	register_spi_programmer(&spi_programmer_via);
> > +	internal_buses_supported = BUS_SPI;
> 
> Please undo the internal_buses_supported change here.
> So far, all VIA chipsets with SPI also support LPC/FWH. We could either
> call enable_flash_vt823x() on all SPI-capable chipsets as well, and have
> LPC/FWH/SPI sorted out automatically by flashrom, or we could try to
> read the ROM straps, which may or may not be readable or documented.

the latter was my goal...
i have reverted all internal_buses_supported changes.
new patch attached.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-VIA-VX-support.patch
Type: text/x-patch
Size: 7383 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20120815/09073940/attachment.patch>