[flashrom] [PATCH] nicintel_eeprom: New programmer for Intel Nics eeproms

Stefan Tauner stefan.tauner at alumni.tuwien.ac.at
Sun May 11 23:14:29 CEST 2014


On Thu,  5 Sep 2013 15:45:07 +0200
Ricardo Ribalda Delgado <ricardo.ribalda at gmail.com> wrote:

> This patch let you read and write the eeprom on your gigagit nic card.
> So far it has been tested on the 82580 NIC, but there should be other
> boards also compatibles. It is a nice substitution for the eeupdate tool.

Thank you very much for your patch and sorry for the long silence. And
no, pinging usually does not help if I don't have time and there is
simply nothing new to report ;)
I have now looked at your patch a bit more closely and it looks quite
decent. I have corrected/changed a few things while reviewing. My
version is attached.

I have added a few notes and questions regarding the implementation in
the old diff below which are the base for my modifications. I hope we
can merge this as soon as you answer my questions.

> […]
> diff --git a/nicintel_eeprom.c b/nicintel_eeprom.c
> new file mode 100644
> index 0000000..9719c24
> --- /dev/null
> +++ b/nicintel_eeprom.c
> @@ -0,0 +1,329 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (C) 2013 Ricardo Ribalda - Qtechnology A/S
> + *
> + * Based on nicinctel_spi.c and ichspi.c

This probably means that we want to copy parts of the copyright lines
from those files here. I don't care too much about this, but
Carl-Daniel probably does and it would be the right thing™ to do. The
least effort would be to copy them all... Carl-Daniel, what do you
think? For now I have only added myself for the opaque programmer
implementation in ichspi.c and my changes today.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> + */
> +
> +/*
> + * Datasheet: Intel 82580 Quad/Dual Gigabit Ethernet LAN Controller Datasheet
> + */
> +
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include "flash.h"
> +#include "programmer.h"
> +#include "hwaccess.h"
> +
> +#define PCI_VENDOR_ID_INTEL 0x8086
> +#define MEMMAP_SIZE getpagesize()

Since we automatically round mappings since a while ago, it is enough
to exactly specify what is needed, hence I have changed that line to
#define MEMMAP_SIZE (0x14 + 3) /* Only EEC and EERD are needed. */

> +
> +/* EEPROM/Flash Control & Data Register */
> +#define EECD	0x10

Where does the D in EECD comes from? My datasheet denotes it EEC only.

> +#define EERD	0x14
> +
> +/* Eeprom Access register bits
> +* Section 8.4.1
> +*/
> +#define EE_SCK	0
> +#define EE_CS	1
> +#define EE_SI	2
> +#define EE_SO	3
> +#define EE_REQ	6
> +#define EE_GNT	7
> +#define EE_PRES	8
> +#define EE_SIZE 11
> +#define EE_SIZE_MASK 0xf
> +
> +#define EERD_START 0
> +#define EERD_DONE 1
> +#define EERD_ADDR 2
> +#define EERD_DATA 16
> +
> +#define BIT(x) (1<<x)
> +#define PAGE_MASK 0x3f

It is probably more readable to define PAGE_SIZE (as decimal) and
derive the mask from that, but I have to admit that's a nitpick. :)

> +
> +uint8_t *nicintel_eebar;
> +struct pci_dev *nicintel_pci;
> +
> +#define UNPROG_DEVICE 0x1509
> +
> +const struct dev_entry nics_intel_ee[] = {
> +	{PCI_VENDOR_ID_INTEL, 0x150e, OK, "Intel", "82580 Quad/Dual Gigabit Ethernet LAN Controller"},
> +	{PCI_VENDOR_ID_INTEL, UNPROG_DEVICE, OK, "Intel", "Unprogrammed 82580 Quad/Dual Gigabit Ethernet LAN Controller"},

Should we include more than this? First of all the devices that were
tested by others, but also untested devices that should actually work.

> +	{0},
> +};
> +
> +static int nicintel_ee_probe(struct flashctx *flash)
> +{
> +	if (nicintel_pci->device_id == UNPROG_DEVICE)
> +		flash->chip->total_size = 16;

Why is this a special case? AFAICS this default ID is given if no
EEPROM is attached although I guess one could store exactly this ID in
the EEPROM too? UNPROG seems to indicate that this is also set for
empty EEPROMs. Isn't this dead code in case no EEPROM is attached due o
the check for EE_PRES in the init function? And why doesn't the probe
fail but defaults to 16kB instead? I seem to neither understand Intel's
auto-detection of the EEPROM size nor your interpretation.

> +	else{
> +		uint32_t tmp;
> +		tmp = pci_mmio_readl(nicintel_eebar + EECD);
> +		tmp = ((tmp >> EE_SIZE) & EE_SIZE_MASK);
> +		switch (tmp) {
> +			case 7:
> +				flash->chip->total_size = 16;
> +				break;
> +			case 8:
> +				flash->chip->total_size = 32;
> +				break;
> +			default:
> +				msg_cerr("Non supported chip size 0x%x\n", tmp);
> +				return 0;
> +		}
> +	}
> +
> +	flash->chip->page_size = PAGE_MASK + 1;
> +	flash->chip->tested = TEST_OK_PREW;
> +	flash->chip->gran = write_gran_1byte;

This is rather nasty, because it is mandatory. We should change this in
the long term. If a memory device does not need explicit erases before
writes then we should take advantage of this feature and not do dummy
writes like this. We need a new value in write_granularity for this.

> +	flash->chip->block_erasers->eraseblocks[0].size = (PAGE_MASK + 1);
> +	flash->chip->block_erasers->eraseblocks[0].count =
> +	    (flash->chip->total_size * 1024) / (PAGE_MASK + 1);
> +
> +	return 1;
> +}
> +
> +#define RETRIES_EERD 10000000

How did you obtain all retry values?

> +static int nicintel_ee_read_word(unsigned int daddr, uint16_t * word)

I wonder why this interface uses 16b words since all the EEPROMs output
8 bits normally. Probably the controller's core is a 16b CPU?

> +{
> +	uint32_t tmp;
> +	int i;
> +
> +	tmp = BIT(EERD_START) + (daddr << EERD_ADDR);
> +	pci_mmio_writel(tmp, nicintel_eebar + EERD);
> +
> +	for (i = 0; i < RETRIES_EERD; i++) {
> +		tmp = pci_mmio_readl(nicintel_eebar + EERD);
> +		if (tmp & BIT(EERD_DONE)) {
> +			*word = (tmp >> EERD_DATA) & 0xffff;
> +			return 0;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
> +static int nicintel_ee_read(struct flashctx *flash, uint8_t * buf,
> +			    unsigned int addr, unsigned int len)
> +{
> +	uint16_t word;
> +
> +	if (addr & 1) {
> +		if (nicintel_ee_read_word(addr / 2, &word))
> +			return -1;
> +		*buf++ = word & 0xff;
> +		addr++;
> +		len--;
> +	}
> +
> +	while (len) {
> +		if (nicintel_ee_read_word(addr / 2, &word))
> +			return -1;
> +		*buf++ = word & 0xff;
> +		addr++;
> +		len--;
> +		if (len) {
> +			*buf++ = (word >> 8) & 0xff;
> +			addr++;
> +			len--;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int nicintel_ee_bitset(int reg, int bit, int val)
> +{
> +	uint32_t tmp;
> +
> +	tmp = pci_mmio_readl(nicintel_eebar + reg);
> +	if (val)
> +		tmp |= BIT(bit);
> +	else
> +		tmp &= ~BIT(bit);
> +	pci_mmio_writel(tmp, nicintel_eebar + reg);
> +
> +	return -1;
> +}
> +
> +static int nicintel_ee_byte(uint8_t mosi, uint8_t * miso)
> +{
> +	int i;
> +	uint8_t out = 0x0;
> +
> +	for (i = 7; i >= 0; i--) {
> +		nicintel_ee_bitset(EECD, EE_SI, mosi & BIT(i));
> +		nicintel_ee_bitset(EECD, EE_SCK, 1);
> +		if (miso) {
> +			uint32_t tmp = pci_mmio_readl(nicintel_eebar + EECD);
> +			if (tmp & BIT(EE_SO))
> +				out |= BIT(i);
> +		}
> +		nicintel_ee_bitset(EECD, EE_SCK, 0);
> +	}
> +
> +	if (miso)
> +		*miso = out;
> +
> +	return 0;
> +}
> +
> +#define OPCODE_RDSR  0x5
> +#define RETRIES_RDSR 100
> +#define RDSR_WIP 0
> +static int nicintel_ee_ready()
> +{
> +	int i;
> +	uint8_t rdsr;
> +
> +	nicintel_ee_bitset(EECD, EE_CS, 0);
> +	nicintel_ee_byte(OPCODE_RDSR, NULL);
> +	for (i = 0; i < RETRIES_RDSR; i++) {
> +		nicintel_ee_byte(0x00, &rdsr);

You are relying on continuous status register reads here. I am not sure
if this works for all devices. Only the ST chips explicitly specify
this. I think we should do complete CS on, write RDSR, read value, CS
off cycles here, c.f. my patch.

> +		if (!(rdsr & BIT(RDSR_WIP))) {
> +			nicintel_ee_bitset(EECD, EE_CS, 1);
> +			programmer_delay(1);
> +			return 0;
> +		}
> +	}
> +
> +	nicintel_ee_bitset(EECD, EE_CS, 1);
> +	programmer_delay(1);
> +
> +	return -1;
> +}
> +
> +static int nicintel_ee_req()
> +{
> +	uint32_t tmp;
> +	nicintel_ee_bitset(EECD, EE_REQ, 1);
> +
> +	tmp = pci_mmio_readl(nicintel_eebar + EECD);
> +	if (!(tmp & BIT(EE_GNT))) {
> +		msg_perr("Enabling eeprom access failed.\n");
> +		return 1;
> +	}
> +
> +	nicintel_ee_bitset(EECD, EE_SCK, 0);
> +	return 0;
> +}
> +
> +#define DELAY_WRITE 1000

Never used, because _write() relies on _ready().

> +#define OPCODE_WRITE 0x2
> +#define OPCODE_WREN  0x6

These opcodes (and RDSR above too) are standard JEDEC opcodes that are
also used in flash chips, c.f. spi.h.
Likewise, the status register bit indicating a busy chip (the whole
status register layout actually) is also equal to most flash chips:
RDSR_WIP -> SPI_SR_WIP

> +static int nicintel_ee_write(struct flashctx *flash, uint8_t * buf,
> +			     unsigned int addr, unsigned int len)
> +{
> +	if (nicintel_ee_req())
> +		return -1;
> +
> +	if (nicintel_ee_ready()) {
> +		nicintel_ee_bitset(EECD, EE_REQ, 0);
> +		return -1;
> +	}
> +
> +	while (len) {
> +		//wren
> +		nicintel_ee_bitset(EECD, EE_CS, 0);
> +		nicintel_ee_byte(OPCODE_WREN, NULL);
> +		nicintel_ee_bitset(EECD, EE_CS, 1);
> +		programmer_delay(1);
> +
> +		//data
> +		nicintel_ee_bitset(EECD, EE_CS, 0);
> +		nicintel_ee_byte(OPCODE_WRITE, NULL);
> +		nicintel_ee_byte((addr >> 8) & 0xff, NULL);
> +		nicintel_ee_byte(addr & 0xff, NULL);
> +		while (len) {
> +			nicintel_ee_byte((buf) ? *buf++ : 0xff, NULL);
> +			len--;
> +			addr++;
> +			if (!(addr & PAGE_MASK))
> +				break;
> +		}
> +		nicintel_ee_bitset(EECD, EE_CS, 1);
> +		programmer_delay(1);
> +		if (nicintel_ee_ready()) {
> +			nicintel_ee_bitset(EECD, EE_REQ, 0);
> +			return -1;
> +		}
> +	}
> +
> +	nicintel_ee_bitset(EECD, EE_REQ, 0);
> +	return 0;
> +}
> +
> +static int nicintel_ee_erase(struct flashctx *flash, unsigned int addr,
> +			     unsigned int len)
> +{
> +	return nicintel_ee_write(flash, NULL, addr, len);
> +}
> +
> +static const struct opaque_programmer opaque_programmer_nicintel_ee = {
> +	.probe = nicintel_ee_probe,
> +	.read = nicintel_ee_read,
> +	.write = nicintel_ee_write,
> +	.erase = nicintel_ee_erase,
> +};
> +
> +static int nicintel_spi_shutdown(void *data)

This is pointless but for documentation purposes...

> +{
> +
> +	/* See the comment about EECD in nicintel_ee_init() for details. */
> +
> +	return 0;
> +}
> +
> +int nicintel_ee_init(void)
> +{
> +	struct pci_dev *dev = NULL;
> +	uint32_t tmp;
> +
> +	if (rget_io_perms())
> +		return 1;
> +
> +	dev = pcidev_init(nics_intel_ee, PCI_BASE_ADDRESS_0);
> +	if (!dev)
> +		return 1;
> +
> +	io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
> +	if (!io_base_addr)
> +		return 1;
> +
> +	nicintel_eebar = rphysmap("Intel Gigabit NIC w/ SPI EEPROM",
> +						io_base_addr, MEMMAP_SIZE);
> +	nicintel_pci=dev;
> +	/* Automatic restore of EECD on shutdown is not possible because EECD
> +	 * does not only contain EE_REQ but other bits with side effects as well.
> +	 * Those other bits must be left untouched.
> +	 */

This is actually not true. We can of course store the state of the bits
we want to restore and use that in a shutdown function that only
restores these bits. I have implemented this in my patch for the bit
banging output pins/bits and EE_REQ.

> +	if (dev->device_id != UNPROG_DEVICE) {

And why this distinction again?

> +		tmp = pci_mmio_readl(nicintel_eebar + EECD);
> +
> +		if (!(tmp & BIT(EE_PRES))) {
> +			msg_perr("EEprom not present.\n");
> +			return 1;
> +		}
> +	}
> +
> +	if (register_shutdown(nicintel_spi_shutdown, NULL))
> +		return 1;
> +
> +	return register_opaque_programmer(&opaque_programmer_nicintel_ee);
> +}

I hope I have not broken anything. Testing the patch again would be
much appreciated though (maybe only after further refining the patch if
need be). Big fat TODO: manpage documentation! Please state if you
want to write it or not. To sum up, the patch is pretty much good to go
in IMHO. The only real blocker for me is the missing manpage entry.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-new-programmer-for-Intel-NICs-EEPROMs.patch
Type: text/x-patch
Size: 12062 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20140511/d380958a/attachment.patch>


More information about the flashrom mailing list