Skip to contentSkip to conversation

new spiflash subsystem

RFC Mike Frysinger 8 participants 27 messages Jan 28, 2008 – Jan 30, 2008

lore
Share & tools
lore
#0 lore raw

this isnt against u-boot mainline, so there will be a few things that are out of date (like the CFG handling), so over look that part. what's up for comments here is the general architecture. ive omitted the env_spiflash.c portion as i think that'll be pretty cheesy.

basically ive laid it out like so:
- common/cmd_spiflash.c:
provides the user interface of the spiflash subsystem. depends on these functions being implemented elsewhere:
* spiflash_info() - ask the spiflash driver for info
* spiflash_read() - ask the spiflash driver to read
* spiflash_write() - ask the spiflash driver to write
* spiflash_erase() - ask the spiflash driver to erase
no validation occurs in the common code since it has no idea about sector sizes and such. i could add a spiflash_query() function and have it return a struct describing the spiflash and use that to validate, but i dont think it's really worth the effort.
relevant defines:
* CFG_CMD_SPIFLASH - include the "spiflash" command
* CFG_SPIFLASH_MULTI - support multiple parts (via "cs" parameter)

- drivers/mtd/spiflash_jedec.c:
provides the spiflash driver functions needed by the common layer. detects and works with Spansion/ST/Atmel/Winbond parts. i've personally verified at least one part from each family, but obviously not every part ;). it depends on these functions being implemented elsewhere:
* spiflash_on() - turn on the SPI
* spiflash_off() - turn off the SPI
* spiflash_cs_set() - assert/deassert the specified chip select
* spiflash_exchange_byte() - send specified byte and return received byte
relevant defines:
* CFG_SPIFLASH_JEDEC_DRIVER - enable this driver
* CFG_SPIFLASH_MULTI - disable a small runtime opt to avoid redetection

- board/$BOARD/spiflash.c:
provides the board / cpu specific spiflash functions. ive included the Blackfin one here as an example.

-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080128/13afe397/attachment.pgp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cmd_spiflash.c
Type: text/x-csrc
Size: 2389 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080128/13afe397/attachment.c
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spiflash_jedec.c
Type: text/x-csrc
Size: 11469 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080128/13afe397/attachment-0001.c
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spiflash.c
Type: text/x-csrc
Size: 2052 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080128/13afe397/attachment-0002.c

#1 lore raw

Unfortunately, this code seems useless, at least for the combination AT91 + SPI flash. Some issues:

I believe that the AT45 is not using the same command set as other SPI flash memories. I think the commands need to be separated. AT45 is much more advanced than other SPI flash.
Did you really test your code on the AT45 series?

You assume, incorrectly, that all sector sizes are the same size.

How do you do "byte writes" which is an important feature of the AT45?

Your code does not support DMA transfers, while the current dataflash code runs DMA up to 50 Mbps.

Erasing the entire SPI flash is generally stupid, since you store the environment there. You typically also store the initial bootloader and U-Boot.
Very rarely you want to erase the complete flash ,and a protection mechanism is needed to avoid accidental overwrites.
The current solution allows dataflash pages to be protected.

Typically you want to store data with a checksum,since relying
on the boot of the linux kernel to produce the error, will in my experience make people confused and they will spend a lot of time barking up the wrong tree.

There is a general problem with U-Boot which seems to assume
that there is more RAM than flash in the system.
How do you easily copy 256 MB from an SD-Card to an onboard 256 MB NAND flash when the SDRAM is 64 MB?

Today, you have to use 10 lines (U-Boot occupies 1 MB) and that is really bad.

The vanilla way of supporting storage devices is really wasteful in resources, and you cannot compare two memory areas if the memory area is larger than half the SDRAM size.

Best Regards
Ulf Samuelsson

Quoted 104 lines

/*
* SPI flash driver user interface
*
* Copyright (c) 2005-2008 Analog Devices Inc.
*
* Licensed under the GPL-2 or later.
*/

#include <common.h>
#include <config.h>
#include <command.h>

#if 1 //(CONFIG_COMMANDS & CFG_CMD_SPIFLASH)

int do_spiflash(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) {
{
int i = 0;
while (i < argc)
printf("[%s] ", argv[i++]);
printf("\n");
}

if (argc == 1) {
usage:
printf("Usage:\n%s\n", cmdtp->usage);
return 1;
}

ulong cs = CFG_SPIFLASH_CS_DEFAULT;
#ifdef CFG_SPIFLASH_MULTI
cs = simple_strtoul(argv[1], NULL, 10);
--argc;
++argv;
#endif

if (!strcmp(argv[1], "info")) {
if (argc != 2)
goto usage;
return spiflash_info(cs);
}

if (!strcmp(argv[1], "erase")) {
size_t off, cnt;

if (argc == 4) {
off = simple_strtoul(argv[2], NULL, 16);
cnt = simple_strtoul(argv[3], NULL, 16); printf("Erasing SPI flash @CS%lu: off 0x%lx count 0x%lx ... ", cs, off, cnt);
} else if (argc == 2) {
off = cnt = -1;
printf("Erasing entire SPI flash @CS%lu ... ", cs);
} else
goto usage;

int ret = spiflash_erase(cs, off, cnt);
puts(" done\n");
return ret;
}

if (argc != 5)
goto usage;

size_t (*spiflash_func)(int cs, size_t off, size_t cnt, uchar *buffer); if (!strcmp(argv[1], "read"))
spiflash_func = spiflash_read;
else if (!strcmp(argv[1], "write"))
spiflash_func = spiflash_write;
else
goto usage;

uchar *addr = (uchar *)simple_strtoul(argv[2], NULL, 16); size_t off = simple_strtoul(argv[3], NULL, 16); size_t cnt = simple_strtoul(argv[4], NULL, 16);

printf("SPI flash @CS%lu %s: addr 0x%p off 0x%lx count 0x%lx ... ", cs, argv[1], addr, off, cnt);
size_t ret = spiflash_func(cs, off, cnt, addr);
puts(" done\n");

return (ret != 0 ? 0 : 1);
}

#ifdef CFG_SPIFLASH_MULTI
U_BOOT_CMD(spiflash, 6, 1, do_spiflash,
"spiflash - SPI flash sub-system\n",
"read cs addr off cnt\n"
"spiflash write cs addr off cnt\n"
"spiflash erase [off cnt]\n"
"spiflash info cs\n"
" - read/write 'cnt' bytes from SPI flash with chip-select 'cs' at offset 'off' into/from 'addr'\n" );
#else
U_BOOT_CMD(spiflash, 5, 1, do_spiflash,
"spiflash - SPI flash sub-system\n",
"read addr off cnt\n"
"spiflash write addr off cnt\n"
"spiflash erase [off cnt]\n"
"spiflash info\n"
" - read/write 'cnt' bytes from SPI flash at offset 'off' into/from 'addr'\n"
);
#endif

Quoted 2 lines

#endif

--------------------------------------------------------------------------------

Quoted 503 lines

/*
* SPI flash driver for JEDEC compatible parts
*
* Copyright (c) 2005-2008 Analog Devices Inc.
*
* Licensed under the GPL-2 or later.
*/

#include <common.h>
#include <config.h>

#if defined(CFG_SPIFLASH_JEDEC_DRIVER)

struct flash_info {
char *name;
uint16_t id;
unsigned sector_size;
unsigned num_sectors;
};

/* SPI Speeds: 50 MHz / 33 MHz */
static struct flash_info flash_spansion_serial_flash[] = { { "S25FL016", 0x0215, 64 * 1024, 32 },
{ "S25FL032", 0x0216, 64 * 1024, 64 },
{ "S25FL064", 0x0217, 64 * 1024, 128 },
{ "S25FL0128", 0x0218, 256 * 1024, 64 },
{ NULL, 0, 0, 0 }
};

/* SPI Speeds: 50 MHz / 20 MHz */
static struct flash_info flash_st_serial_flash[] = { { "m25p05", 0x2010, 32 * 1024, 2 },
{ "m25p10", 0x2011, 32 * 1024, 4 },
{ "m25p20", 0x2012, 64 * 1024, 4 },
{ "m25p40", 0x2013, 64 * 1024, 8 },
{ "m25p16", 0x2015, 64 * 1024, 32 },
{ "m25p32", 0x2016, 64 * 1024, 64 },
{ "m25p64", 0x2017, 64 * 1024, 128 },
{ "m25p128", 0x2018, 256 * 1024, 64 },
{ NULL, 0, 0, 0 }
};

/* SPI Speeds: 66 MHz / 33 MHz */
static struct flash_info flash_atmel_dataflash[] = { { "AT45DB011x", 0x0c, 264, 512 },
{ "AT45DB021x", 0x14, 264, 1025 },
{ "AT45DB041x", 0x1c, 264, 2048 },
{ "AT45DB081x", 0x24, 264, 4096 },
{ "AT45DB161x", 0x2c, 528, 4096 },
{ "AT45DB321x", 0x34, 528, 8192 },
{ "AT45DB642x", 0x3c, 1056, 8192 },
{ NULL, 0, 0, 0 }
};

/* SPI Speed: 50 MHz / 25 MHz or 40 MHz / 20 MHz */ static struct flash_info flash_winbond_serial_flash[] = { { "W25X10", 0x3011, 16 * 256, 32 },
{ "W25X20", 0x3012, 16 * 256, 64 },
{ "W25X40", 0x3013, 16 * 256, 128 },
{ "W25X80", 0x3014, 16 * 256, 256 },
{ "W25P80", 0x2014, 256 * 256, 16 },
{ "W25P16", 0x2015, 256 * 256, 32 },
{ NULL, 0, 0, 0 }
};

struct flash_ops {
uint8_t read, write, erase, status;
};

#define OP_READ_SLOW 0x03
#define OP_READ_FAST 0x0B
#ifdef CFG_SPIFLASH_SLOW_READ

# define OP_READ OP_READ_SLOW
#else
# define OP_READ OP_READ_FAST
#endif
static struct flash_ops flash_st_ops = {
.read = OP_READ,
.write = 0x02,
.erase = 0xD8,
.status = 0x05,
};

static struct flash_ops flash_atmel_ops = {
.read = OP_READ,
.write = 0x82,
.erase = 0x81,
.status = 0xD7,
};

static struct flash_ops flash_winbond_ops = {
.read = OP_READ,
.write = 0x02,
.erase = 0x20,
.status = 0x05,
};

struct manufacturer_info {
const char *name;
uint8_t id;
struct flash_info *flashes;
struct flash_ops *ops;
};

static struct {
struct manufacturer_info *manufacturer;
struct flash_info *flash;
struct flash_ops *ops;
uint8_t manufacturer_id, device_id1, device_id2;
unsigned int write_length;
unsigned long sector_size, num_sectors;
} flash;

enum {
JED_MANU_SPANSION = 0x01,

JED_MANU_ST       = 0x20,
JED_MANU_ATMEL    = 0x1F,
JED_MANU_WINBOND  = 0xEF,

};

static struct manufacturer_info flash_manufacturers[] = { {
.name = "Spansion",
.id = JED_MANU_SPANSION,
.flashes = flash_spansion_serial_flash,
.ops = &flash_st_ops,
},
{
.name = "ST",
.id = JED_MANU_ST,
.flashes = flash_st_serial_flash,
.ops = &flash_st_ops,
},
{
.name = "Atmel",
.id = JED_MANU_ATMEL,
.flashes = flash_atmel_dataflash,
.ops = &flash_atmel_ops,
},
{
.name = "Winbond",
.id = JED_MANU_WINBOND,
.flashes = flash_winbond_serial_flash,
.ops = &flash_winbond_ops,
},
};

#define TIMEOUT 5000 /* timeout of 5 seconds */

static uint8_t read_status_register(int cs)
{
uint8_t status_register;

/* send instruction to read status register */
spiflash_cs_set(cs, 1);
spiflash_exchange_byte(flash.ops->status);
/* send dummy to receive the status register */
status_register = spiflash_exchange_byte(0);
spiflash_cs_set(cs, 0);

return status_register;
}

/* Request and read the manufacturer and device id of parts which
* are compatible with the JEDEC standard (JEP106) and use that to
* setup other operating conditions.
*/
static int spiflash_detect_part(int cs)
{
uint16_t dev_id;
size_t i;

#ifndef CFG_SPIFLASH_MULTI
static char called_init;
if (called_init)
return 0;
#endif

spiflash_cs_set(cs, 1);

/* Send the request for the part identification */ spiflash_exchange_byte(0x9F);

/* Now read in the manufacturer id bytes */
do {
flash.manufacturer_id = spiflash_exchange_byte(0); if (flash.manufacturer_id == 0x7F)
puts("Warning: unhandled manufacturer continuation byte!\n"); } while (flash.manufacturer_id == 0x7F);

/* Now read in the first device id byte */
flash.device_id1 = spiflash_exchange_byte(0);

/* Now read in the second device id byte */
flash.device_id2 = spiflash_exchange_byte(0);

spiflash_cs_set(cs, 0);

dev_id = (flash.device_id1 << 8) | flash.device_id2;

for (i = 0; i < ARRAY_SIZE(flash_manufacturers); ++i) { if (flash.manufacturer_id == flash_manufacturers[i].id) break;
}
if (i == ARRAY_SIZE(flash_manufacturers))
goto unknown;

flash.manufacturer = &flash_manufacturers[i];
flash.ops = flash_manufacturers[i].ops;

switch (flash.manufacturer_id) {
case JED_MANU_SPANSION:
case JED_MANU_ST:
case JED_MANU_WINBOND:
for (i = 0; flash.manufacturer->flashes[i].name; ++i) { if (dev_id == flash.manufacturer->flashes[i].id) break;
}
if (!flash.manufacturer->flashes[i].name)
goto unknown;

flash.flash = &flash.manufacturer->flashes[i];
flash.sector_size = flash.flash->sector_size;
flash.num_sectors = flash.flash->num_sectors;
flash.write_length = 256;
break;

case JED_MANU_ATMEL: {
uint8_t status = read_status_register(cs);

for (i = 0; flash.manufacturer->flashes[i].name; ++i) { if ((status & 0x3c) == flash.manufacturer->flashes[i].id) break;
}
if (!flash.manufacturer->flashes[i].name)
goto unknown;

flash.flash = &flash.manufacturer->flashes[i];
flash.sector_size = flash.flash->sector_size;
flash.num_sectors = flash.flash->num_sectors;

/* see if flash is in "power of 2" mode */
if (status & 0x1)
flash.sector_size &= ~(1 << (ffs(flash.sector_size) - 1));

flash.write_length = flash.sector_size;
break;
}
}

#ifndef CFG_SPIFLASH_MULTI
called_init = 1;
#endif
return 0;

unknown:
printf("Unknown SPI device: 0x%02X 0x%02X 0x%02X\n", flash.manufacturer_id, flash.device_id1, flash.device_id2); return 1;
}

static int wait_for_ready_status(int cs)
{
ulong start = get_timer(0);

while (get_timer(0) - start < TIMEOUT) {
switch (flash.manufacturer_id) {
case JED_MANU_SPANSION:
case JED_MANU_ST:
case JED_MANU_WINBOND:
if (!(read_status_register(cs) & 0x01))
return 0;
break;

case JED_MANU_ATMEL:
if (read_status_register(cs) & 0x80)
return 0;
break;
}

if (ctrlc()) {
puts("\nAbort\n");
return -1;
}
}

puts("Timeout\n");
return -1;
}

static void transmit_address(size_t addr)
{
/* Send the highest byte of the 24 bit address at first */ spiflash_exchange_byte(addr >> 16);
/* Send the middle byte of the 24 bit address at second */ spiflash_exchange_byte(addr >> 8);
/* Send the lowest byte of the 24 bit address finally */ spiflash_exchange_byte(addr);
}

static int enable_writing(int cs)
{
ulong start;

if (flash.manufacturer_id == JED_MANU_ATMEL)
return 0;

/* A write enable instruction must previously have been executed */ spiflash_cs_set(cs, 1);
spiflash_exchange_byte(0x06);
spiflash_cs_set(cs, 0);

/* The status register will be polled to check the write enable latch "WREN" */ start = get_timer(0);
while (get_timer(0) - start < TIMEOUT) {
if (read_status_register(cs) & 0x02)
return 0;

if (ctrlc()) {
puts("\nAbort\n");
return -1;
}
}

puts("Timeout\n");
return -1;
}

size_t spiflash_read(int cs, size_t off, size_t cnt, uchar *buffer) {
size_t ret = cnt;

spiflash_on(cs);

if (spiflash_detect_part(cs))
ret = 0;
else {
spiflash_cs_set(cs, 1);

/* Tell the flash we want to read */
spiflash_exchange_byte(flash.ops->read);

/* Tell the flash where to read */
transmit_address(off);

/* Send dummy byte when doing SPI fast reads */
if (flash.ops->read == OP_READ_FAST)
spiflash_exchange_byte(0);

/* Now grab the stream of bytes coming back */
size_t i;
for (i = 1; i <= cnt; ++i) {
*buffer++ = spiflash_exchange_byte(0);
if (i % flash.sector_size == 0)
puts(".");
}

spiflash_cs_set(cs, 0);
}

spiflash_off(cs);

return ret;
}

static size_t write_flash(int cs, size_t address, size_t cnt, uchar *buffer) {
size_t i, write_buffer_size;

if (enable_writing(cs))
return -1;

/* Send write command followed by the 24 bit address */ spiflash_cs_set(cs, 1); spiflash_exchange_byte(flash.ops->write); transmit_address(address);

/* Shoot out a single write buffer */
write_buffer_size = min(cnt, flash.write_length);
for (i = 0; i < write_buffer_size; ++i)
spiflash_exchange_byte(buffer[i]);

spiflash_cs_set(cs, 0);

/* Wait for the flash to do its thing */
if (wait_for_ready_status(cs))
return -1;
else
return i;
}

static int write_sector(int cs, size_t address, size_t cnt, uchar *buffer) {
size_t write_cnt;

while (cnt != 0) {
write_cnt = write_flash(cs, address, cnt, buffer); if (write_cnt == -1)
return -1;

/* Now that we've sent some bytes out to the flash, update
* our counters a bit
*/
cnt -= write_cnt;
address += write_cnt;
buffer += write_cnt;
}

return 0;
}

static int erase_sector(int cs, size_t address)
{
/* sector gets checked in higher function, so assume it's valid
* here and figure out the offset of the sector in flash */
if (enable_writing(cs))
return -1;

/*
* Send the erase block command to the flash followed by the 24 address
* to point to the start of a sector
*/
spiflash_cs_set(cs, 1); spiflash_exchange_byte(flash.ops->erase); transmit_address(address);
spiflash_cs_set(cs, 0);

return wait_for_ready_status(cs);
}

static size_t write_or_erase(int cs, size_t off, size_t cnt, uchar *buffer, int w_o_e) {
int ret = cnt;

spiflash_on(cs);

if (spiflash_detect_part(cs))
ret = 0;
else if (off % flash.sector_size || cnt % flash.sector_size) { ret = 0;
printf("\n%s: off/cnt not aligned to sector size 0x%x\n", __func__, flash.sector_size);
} else {
while (off < cnt) {
if ((w_o_e == 'w' && write_sector(cs, off, cnt, buffer)) ||

    (w_o_e == 'e' && erase_sector(cs, off)))

{
ret = 0;
break;
}
off += flash.sector_size;
puts(".");
}
}

spiflash_off(cs);

return ret;
}

size_t spiflash_write(int cs, size_t off, size_t cnt, uchar *buffer) {
return write_or_erase(cs, off, cnt, buffer, 'w');
}

size_t spiflash_erase(int cs, size_t off, size_t cnt) {
return write_or_erase(cs, off, cnt, NULL, 'e'); }

int spiflash_info(int cs)
{
int ret = 0;

spiflash_on(cs);

if (spiflash_detect_part(cs))
ret = 1;
else {
printf("SPI Device: %s 0x%02X (%s) 0x%02X 0x%02X\n" "Parameters: num sectors = %i, sector size = %i, write size = %i\n" "Flash Size: %i mbit (%i mbyte)\n"
"Status: 0x%02X\n",
flash.flash->name, flash.manufacturer_id, flash.manufacturer->name, flash.device_id1, flash.device_id2, flash.num_sectors, flash.sector_size, flash.write_length, (flash.num_sectors * flash.sector_size) >> 17, (flash.num_sectors * flash.sector_size) >> 20, read_status_register(cs));

printf(" Sector Start Addresses:");
unsigned i, off = 0;
for (i = 0; i < flash.num_sectors; ++i) {
if (i % 5 == 0)
puts("\n ");
printf("%08x ", off);
off += flash.sector_size;
}
if (i % 5)
puts("\n");
}

Quoted 7 lines

spiflash_off(cs);

return ret;
}

#endif

--------------------------------------------------------------------------------

Quoted 85 lines

/*
* SPI flash driver
*
* Enter bugs at http://blackfin.uclinux.org/
*
* Copyright (c) 2005-2007 Analog Devices Inc.
*
* Licensed under the GPL-2 or later.
*/

/* Configuration options:
* CONFIG_SPI_BAUD - value to load into SPI_BAUD (divisor of SCLK to get SPI CLK)
* CONFIG_SPI_FLASH_SLOW_READ - force usage of the slower read
* WARNING: make sure your SCLK + SPI_BAUD is slow enough
*/

#include <common.h>
#include <malloc.h>
#include <asm/io.h>
#include <asm/mach-common/bits/spi.h>

#if defined(CFG_SPIFLASH)

void spiflash_on(int cs)
{
/* [#3541] This delay appears to be necessary, but not sure
* exactly why as the history behind it is non-existant. */
udelay(CONFIG_CCLK_HZ / 25000000);

/* enable SPI pins: SSEL, MOSI, MISO, SCK */
#ifdef __ADSPBF54x__
*pPORTE_FER |= (SPI0_SCK | SPI0_MOSI | SPI0_MISO | SPI0_SEL1);
#elif defined(__ADSPBF534__) || defined(__ADSPBF536__) || defined(__ADSPBF537__) *pPORTF_FER |= (PF10 | PF11 | PF12 | PF13);
#elif defined(__ADSPBF52x__)
bfin_write_PORTG_MUX((bfin_read_PORTG_MUX() & ~PORT_x_MUX_0_MASK) | PORT_x_MUX_0_FUNC_3); bfin_write_PORTG_FER(bfin_read_PORTG_FER() | PG1 | PG2 | PG3 | PG4);
#endif

/* initate communication upon write of TDBR */
*pSPI_CTL = (SPE|MSTR|CPHA|CPOL|0x01);
*pSPI_BAUD = CONFIG_SPI_BAUD;
}

void spiflash_off(int cs)
{
/* put SPI settings back to reset state */
*pSPI_CTL = 0x0400;
*pSPI_BAUD = 0;
SSYNC();
}

void spiflash_cs_set(int cs, uint on)
{
if (on) {
cs = 1 << cs;

/* toggle SSEL to reset the device so it'll take a new command */ *pSPI_FLG = 0xFF00 | cs;
SSYNC();

*pSPI_FLG = ((0xFF & ~cs) << 8) | cs;
} else {
/* put SPI settings back to reset state */
*pSPI_FLG = 0xFF00;
}
SSYNC();
}

uint8_t spiflash_exchange_byte(uint8_t transmit)
{
*pSPI_TDBR = transmit;
SSYNC();

while ((*pSPI_STAT & TXS))
if (ctrlc())
break;
while (!(*pSPI_STAT & SPIF))
if (ctrlc())
break;
while (!(*pSPI_STAT & RXS))
if (ctrlc())
break;

Quoted 6 lines

/* Read dummy to empty the receive register */
return *pSPI_RDBR;
}

#endif

--------------------------------------------------------------------------------

Quoted 4 lines

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

--------------------------------------------------------------------------------

Quoted 5 lines

_______________________________________________
U-Boot-Users mailing list
U-Boot-Users at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users

#2 lore raw
Quoted 5 lines · Ulf Samuelsson

Unfortunately, this code seems useless, at least for the combination AT91 + SPI flash. Some issues:

I believe that the AT45 is not using the same command set as other SPI flash memories. I think the commands need to be separated.

i already accounted for different command sets based on part family.

Quoted 1 line

AT45 is much more advanced than other SPI flash.

yes, but it still has a basic subset which can be used.

Quoted 1 line

Did you really test your code on the AT45 series?

it was the part i developed the code on originally (the 64mbit one).

Quoted 1 line

You assume, incorrectly, that all sector sizes are the same size.

that depends on what level you look at it. sector 0 can be accessed in pieces, but it can also be treated as one big sector the same as all the others. this is how i treated it.

Quoted 1 line

How do you do "byte writes" which is an important feature of the AT45?

simple: i dont. spi flash writing isnt something to be done constantly nor is it fast, so i dont sweat getting maximum performance.

Quoted 2 lines

Your code does not support DMA transfers, while the current dataflash code runs DMA up to 50 Mbps.

so ? the point of u-boot is to do everything in PIO mode.

Quoted 3 lines

Erasing the entire SPI flash is generally stupid, since you store the environment there. You typically also store the initial bootloader and U-Boot.

so the user is stupid if they erase the entire flash ? you could say the same thing about any flash type.

Quoted 3 lines

Very rarely you want to erase the complete flash ,and a protection mechanism is needed to avoid accidental overwrites.
The current solution allows dataflash pages to be protected.

yes, protection of pages (hardware and software) is missing.

Quoted 4 lines

Typically you want to store data with a checksum,since relying on the boot of the linux kernel to produce the error, will in my experience make people confused and they will spend a lot of time barking up the wrong tree.

ok ? not sure how this is relevant to the topic at hand.

Quoted 11 lines

There is a general problem with U-Boot which seems to assume that there is more RAM than flash in the system.
How do you easily copy 256 MB from an SD-Card to an onboard 256 MB NAND flash when the SDRAM is 64 MB?

Today, you have to use 10 lines (U-Boot occupies 1 MB) and that is really bad.

The vanilla way of supporting storage devices is really wasteful in resources, and you cannot compare two memory areas if the memory area is larger than half the SDRAM size.

ok ? u-boot is designed as a monitor to get the system bootstrapped and execute something else. it isnt an operating system, it doesnt get maximal performance, and it isnt supposed to support all sorts of extended features. what you describe as deficiency doesnt apply to the topic at hand and really sounds like a basic design decision for u-boot. if you want optimal performance, use Linux.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080128/df5c772b/attachment.pgp

#3 lore raw
Quoted 5 lines · Ulf Samuelsson

Unfortunately, this code seems useless, at least for the combination AT91 + SPI flash. Some issues:

I believe that the AT45 is not using the same command set as other SPI flash memories. I think the commands need to be separated.

i already accounted for different command sets based on part family.

Quoted 1 line

AT45 is much more advanced than other SPI flash.

yes, but it still has a basic subset which can be used.

Quoted 1 line

Did you really test your code on the AT45 series?

it was the part i developed the code on originally (the 64mbit one).

Quoted 1 line

You assume, incorrectly, that all sector sizes are the same size.

that depends on what level you look at it. sector 0 can be accessed in pieces, but it can also be treated as one big sector the same as all the others. this is how i treated it.

==> And this is not a good approach, you want the S/W to use the
first 256 kB to store initial boot, U-boot and environment.
The AT91SAM9 chips will use the first 8 kB sector
for the AT91bootstrap, and that should have protection separate from U-Boot code. AVR32 boot ROM will boot from a higher address and use the
8 kB sector at address zero as environment.
Both will need to have a S/W partitioning scheme added with separate protection for each partition.
This partioning scheme exists today.

Quoted 1 line

How do you do "byte writes" which is an important feature of the AT45?

simple: i dont. spi flash writing isnt something to be done constantly nor is it fast, so i dont sweat getting maximum performance.

==> Which means that you want to impose an arbitary limit which
currently does not exist in U-Boot.
You also increase startup time, which is a critical parameter for many customers.

Quoted 2 lines

Your code does not support DMA transfers, while the current dataflash code runs DMA up to 50 Mbps.

so ? the point of u-boot is to do everything in PIO mode.

==> This is not how the dataflash support is implemented in U-Boot today.
Current implementation is using DMA.

Quoted 3 lines

Erasing the entire SPI flash is generally stupid, since you store the environment there. You typically also store the initial bootloader and U-Boot.

so the user is stupid if they erase the entire flash ? you could say the same thing about any flash type.

==>
No, but U-Boot should make them aware of consequences, so they do not do this by mistake.
You normally can't remove the bootloader from a parallel flash without unprotecting it first.

Quoted 3 lines

Very rarely you want to erase the complete flash ,and a protection mechanism is needed to avoid accidental overwrites.
The current solution allows dataflash pages to be protected.

yes, protection of pages (hardware and software) is missing.

==> Exactly my point

Quoted 4 lines

Typically you want to store data with a checksum,since relying on the boot of the linux kernel to produce the error, will in my experience make people confused and they will spend a lot of time barking up the wrong tree.

ok ? not sure how this is relevant to the topic at hand.

==> Just pointing out that the user interface is missing features.

Quoted 11 lines

There is a general problem with U-Boot which seems to assume that there is more RAM than flash in the system.
How do you easily copy 256 MB from an SD-Card to an onboard 256 MB NAND flash when the SDRAM is 64 MB?

Today, you have to use 10 lines (U-Boot occupies 1 MB) and that is really bad.

The vanilla way of supporting storage devices is really wasteful in resources, and you cannot compare two memory areas if the memory area is larger than half the SDRAM size.

ok ? u-boot is designed as a monitor to get the system bootstrapped and execute something else. it isnt an operating system, it doesnt get maximal performance, and it isnt supposed to support all sorts of extended features. what you describe as deficiency doesnt apply to the topic at hand and really sounds like a basic design decision for u-boot. if you want optimal performance, use Linux.
-mike

==> Yes, it is a basic design decision, and with huge flash memories,
it is becoming flawed. Thats why I think we should rethink how we do things, before we rewrite the code.

Programming the internal flash memory in an efficient way is really the core of a bootloader, not an extended feature. Programming time is a critical parameter for large volume production so if you can avoid booting Linux, this is probably a good thing.

==> My conclusion remains, that I do not see why anyone using
dataflash in the current U-boot would want to switch to the proposed implementation, which will reduce performance and functionality, and increase risk of errors.

Best Regards
Ulf Samuelsson ulf at atmel.com
Atmel Nordic AB

Mail:  Box 2033, 174 02 Sundbyberg, Sweden
Visit:  Kavalleriv?gen 24, 174 58 Sundbyberg, Sweden

Phone +46 (8) 441 54 22 Fax +46 (8) 441 54 29
GSM +46 (706) 22 44 57

#4 lore raw
Quoted 7 lines · On Tue, 29 Jan 2008 08:35:50 +0100 "Ulf Samuelsson" <[email protected]>…
Quoted 2 lines

Your code does not support DMA transfers, while the current dataflash code runs DMA up to 50 Mbps.

so ? the point of u-boot is to do everything in PIO mode.

==> This is not how the dataflash support is implemented in U-Boot today.
Current implementation is using DMA.

Current implementation is also AT91-only, which is a design flaw that is much worse than anything in Mike's approach.

That said, I think the low-level SPI API should make it possible for a low-level driver to do DMA. The proposed byte-at-a-time API does not allow this, while the existing spi_xfer() interface in include/spi.h does, assuming the buffers are properly aligned.

Haavard

#5 lore raw
In reply to Ulf Samuelsson

please quote properly in your replies

Quoted 6 lines · Ulf Samuelsson
Quoted 1 line

You assume, incorrectly, that all sector sizes are the same size.

that depends on what level you look at it. sector 0 can be accessed in pieces, but it can also be treated as one big sector the same as all the others. this is how i treated it.

Quoted 9 lines

==> And this is not a good approach, you want the S/W to use the
first 256 kB to store initial boot, U-boot and environment. The AT91SAM9 chips will use the first 8 kB sector for the AT91bootstrap, and that should have protection separate
from U-Boot code. AVR32 boot ROM will boot from a higher address and use the 8 kB sector at address zero as environment.
Both will need to have a S/W partitioning scheme added with separate protection for each partition.
This partioning scheme exists today.

sorry, i didnt mean to imply i thought this limitation was something to be written in stone. i understand perfectly why you would want this flexibility, so i can take a look. rather than making general statements, can you please provide a concrete list of all relevant examples you know of.

Quoted 9 lines
Quoted 1 line

How do you do "byte writes" which is an important feature of the AT45?

simple: i dont. spi flash writing isnt something to be done constantly nor is it fast, so i dont sweat getting maximum performance.

==> Which means that you want to impose an arbitary limit which
currently does not exist in U-Boot.
You also increase startup time, which is a critical parameter for
many customers.

uhh, what ? you're telling me that customers do a SPI write in u-boot everytime they boot up ? that just sounds stupid and your customers need to rethink their system. "critical boot time" and "spi flash writing" dont belong in the same sentence :P

Quoted 7 lines
Quoted 2 lines

Your code does not support DMA transfers, while the current dataflash code runs DMA up to 50 Mbps.

so ? the point of u-boot is to do everything in PIO mode.

==> This is not how the dataflash support is implemented in U-Boot today.
Current implementation is using DMA.

then i'm afraid your implementation is wrong. DMA is board/cpu-specific and there's no way that can be represented in a general framework. in fact, when i was researching how to support SPI dataflash originally, i saw the dataflash code in u-boot and found it to be worthless in terms of re-use. as Haavard points out though, by using a more general SPI transfer layer, you can still (wrongly imo) do DMA transfers.

Quoted 4 lines
Quoted 3 lines

Erasing the entire SPI flash is generally stupid, since you store the environment there. You typically also store the initial bootloader and U-Boot.

Quoted 8 lines

so the user is stupid if they erase the entire flash ? you could say the same thing about any flash type.

==>
No, but U-Boot should make them aware of consequences, so they do not do this by mistake.
You normally can't remove the bootloader from a parallel flash without unprotecting it first.

providing sector protection is a different topic from removing functionality. nand flash provides the ability to erase the whole flash, so are you going to lobby them to drop that too ?

Quoted 8 lines
Quoted 4 lines

Typically you want to store data with a checksum,since relying on the boot of the linux kernel to produce the error, will in my experience make people confused and they will spend a lot of time barking up the wrong tree.

ok ? not sure how this is relevant to the topic at hand.

==> Just pointing out that the user interface is missing features.

what features ? checksums is not the domain of the flash layer unless the flash media itself requires it (like NAND and ECC/bad-blocks). parallel/SPI flash provide no such thing, so adding a completely software checksum layer is inappropriate. if you want checksums, add them yourself in your board code.

Quoted 23 lines
Quoted 11 lines

There is a general problem with U-Boot which seems to assume that there is more RAM than flash in the system.
How do you easily copy 256 MB from an SD-Card to an onboard 256 MB NAND flash when the SDRAM is 64 MB?

Today, you have to use 10 lines (U-Boot occupies 1 MB) and that is really bad.

The vanilla way of supporting storage devices is really wasteful in resources, and you cannot compare two memory areas if the memory area is larger than half the SDRAM size.

ok ? u-boot is designed as a monitor to get the system bootstrapped and execute something else. it isnt an operating system, it doesnt get maximal performance, and it isnt supposed to support all sorts of extended features. what you describe as deficiency doesnt apply to the topic at hand and really sounds like a basic design decision for u-boot. if you want optimal performance, use Linux.

==> Yes, it is a basic design decision, and with huge flash memories,
it is becoming flawed. Thats why I think we should rethink how we do things, before we rewrite the code.

Quoted 5 lines

Programming the internal flash memory in an efficient way is really the core of a bootloader, not an extended feature. Programming time is a critical parameter for large volume
production so if you can avoid booting Linux, this is probably a good thing.

not relevant to the thread. please drop any further comments on the topic. if you want to re-architect u-boot, start a new thread.

Quoted 4 lines

==> My conclusion remains, that I do not see why anyone using
dataflash in the current U-boot would want to switch to the
proposed implementation, which will reduce performance and functionality, and increase risk of errors.

do you not understand how the software development process works ? i post a new framework, asking for feedback. that doesnt mean it is perfect the first time through and that everyone should start using it RIGHT NOW. please exercise some patience.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080129/bd8300f9/attachment.pgp

#6 lore raw

please quote properly in your replies

==> Mailers can't quote properly when mails are sent as a text attachement.
Also quoting does not work when people are sending non-compliant mails allowing long lines.

Quoted 6 lines · Ulf Samuelsson
Quoted 1 line

You assume, incorrectly, that all sector sizes are the same size.

that depends on what level you look at it. sector 0 can be accessed in pieces, but it can also be treated as one big sector the same as all the others. this is how i treated it.

Quoted 9 lines

==> And this is not a good approach, you want the S/W to use the
first 256 kB to store initial boot, U-boot and environment. The AT91SAM9 chips will use the first 8 kB sector for the AT91bootstrap, and that should have protection separate
from U-Boot code. AVR32 boot ROM will boot from a higher address and use the 8 kB sector at address zero as environment.
Both will need to have a S/W partitioning scheme added with separate protection for each partition.
This partioning scheme exists today.

sorry, i didnt mean to imply i thought this limitation was something to be written in stone. i understand perfectly why you would want this flexibility, so i can take a look. rather than making general statements, can you please provide a concrete list of all relevant examples you know of.

Quoted 9 lines
Quoted 1 line

How do you do "byte writes" which is an important feature of the AT45?

simple: i dont. spi flash writing isnt something to be done constantly nor is it fast, so i dont sweat getting maximum performance.

==> Which means that you want to impose an arbitary limit which
currently does not exist in U-Boot.
You also increase startup time, which is a critical parameter for
many customers.

uhh, what ? you're telling me that customers do a SPI write in u-boot everytime they boot up ? that just sounds stupid and your customers need to rethink their system. "critical boot time" and "spi flash writing" dont belong in the same sentence :P

==> Should have been a line break there, the sentence belongs to the
statement below.

Quoted 7 lines
Quoted 2 lines

Your code does not support DMA transfers, while the current dataflash code runs DMA up to 50 Mbps.

so ? the point of u-boot is to do everything in PIO mode.

==> This is not how the dataflash support is implemented in U-Boot today.
Current implementation is using DMA.

then i'm afraid your implementation is wrong. DMA is board/cpu-specific and there's no way that can be represented in a general framework. in fact, when i was researching how to support SPI dataflash originally, i saw the dataflash code in u-boot and found it to be worthless in terms of re-use. as Haavard points out though, by using a more general SPI transfer layer, you can still (wrongly imo) do DMA transfers.

==> If a customer measures the boot speed, then slow non-DMA code is wrong
and fast code using DMA is right.
It may be that this should be in an Atmel specific driver location.

Quoted 4 lines
Quoted 3 lines

Erasing the entire SPI flash is generally stupid, since you store the environment there. You typically also store the initial bootloader and U-Boot.

Quoted 8 lines

so the user is stupid if they erase the entire flash ? you could say the same thing about any flash type.

==>
No, but U-Boot should make them aware of consequences, so they do not do this by mistake.
You normally can't remove the bootloader from a parallel flash without unprotecting it first.

providing sector protection is a different topic from removing functionality. nand flash provides the ability to erase the whole flash, so are you going to lobby them to drop that too ?

==> If you store the U-boot and environment in the NAND flash
then allowing users to destroy the functionality of the board with a simple command is stupid.
NAND flash need partitioning and protection as well.

Quoted 8 lines
Quoted 4 lines

Typically you want to store data with a checksum,since relying on the boot of the linux kernel to produce the error, will in my experience make people confused and they will spend a lot of time barking up the wrong tree.

ok ? not sure how this is relevant to the topic at hand.

==> Just pointing out that the user interface is missing features.

what features ? checksums is not the domain of the flash layer unless the flash media itself requires it (like NAND and ECC/bad-blocks). parallel/SPI flash provide no such thing, so adding a completely software checksum layer is inappropriate. if you want checksums, add them yourself in your board code.

==> It is your opinion that it is inappropriate, others may disagree.
I believe that this should be a user choice.
I have seen this problem numerous times and users invariably look in the wrong place to solve the problem, wasting their time and eventually my time as well.

Quoted 23 lines
Quoted 11 lines

There is a general problem with U-Boot which seems to assume that there is more RAM than flash in the system.
How do you easily copy 256 MB from an SD-Card to an onboard 256 MB NAND flash when the SDRAM is 64 MB?

Today, you have to use 10 lines (U-Boot occupies 1 MB) and that is really bad.

The vanilla way of supporting storage devices is really wasteful in resources, and you cannot compare two memory areas if the memory area is larger than half the SDRAM size.

ok ? u-boot is designed as a monitor to get the system bootstrapped and execute something else. it isnt an operating system, it doesnt get maximal performance, and it isnt supposed to support all sorts of extended features. what you describe as deficiency doesnt apply to the topic at hand and really sounds like a basic design decision for u-boot. if you want optimal performance, use Linux.

==> Yes, it is a basic design decision, and with huge flash memories,
it is becoming flawed. Thats why I think we should rethink how we do things, before we rewrite the code.

Quoted 5 lines

Programming the internal flash memory in an efficient way is really the core of a bootloader, not an extended feature. Programming time is a critical parameter for large volume
production so if you can avoid booting Linux, this is probably a good thing.

not relevant to the thread. please drop any further comments on the topic. if you want to re-architect u-boot, start a new thread.

Quoted 4 lines

==> My conclusion remains, that I do not see why anyone using
dataflash in the current U-boot would want to switch to the
proposed implementation, which will reduce performance and functionality, and increase risk of errors.

do you not understand how the software development process works ? i post a new framework, asking for feedback. that doesnt mean it is perfect the first time through and that everyone should start using it RIGHT NOW. please exercise some patience.

==> I *am* providing feedback, and the feedback is that the proposed code
will limit functionality compared to the current implementation, and introduce dangerous features which I prefer not having in any boards that I have to support.
It is moving u-boot in a direction which I belive is a dead end due to the emergence of huge flash memories.
I do not mind that code like this is added to u-boot as long as you do not build this in by default and that the current implemention is available until the deficiencies in the proposed implementation is fixed.

-mike

Best Regards
Ulf Samuelsson ulf at atmel.com
Atmel Nordic AB

Mail:  Box 2033, 174 02 Sundbyberg, Sweden
Visit:  Kavalleriv?gen 24, 174 58 Sundbyberg, Sweden

Phone +46 (8) 441 54 22 Fax +46 (8) 441 54 29
GSM +46 (706) 22 44 57

Technical support when I am not available:
AT90 AVR Applications Group: mailto:avr at atmel.com AT91 ARM Applications Group: mailto:at91support at atmel.com
AVR32 Applications Group mailto:avr32 at atmel.com
http://www.avrfreaks.net/; http://avr32linux.org/
http://www.at91.com/ ; ftp://at91dist:distrib at 81.80.104.162/
----- Original Message -----
From: "Mike Frysinger" <[email protected]>
To: "Ulf Samuelsson" <[email protected]>
Cc: <[email protected]>
Sent: Tuesday, January 29, 2008 3:07 PM
Subject: Re: [U-Boot-Users] [rfc] new spiflash subsystem

#7 lore raw
Quoted 5 lines · Ulf Samuelsson

please quote properly in your replies

==> Mailers can't quote properly when mails are sent as a text attachement.
Also quoting does not work when people are sending non-compliant
mails allowing long lines.

funny, no one else has a problem. the body of the original message was not an attachment (the additional files were) nor did it have long lines. the different portions were also properly mime quoted. the code looking for comments did contain long lines, but that is correct behavior when posting code/patches, and line wrapping is wrong. sane e-mail clients can quote perfectly fine in any of these cases. please fix your client.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080129/c7407044/attachment.pgp

#8 lore raw
In reply to Ulf Samuelsson
Quoted 1 line · On Wed, 30 Jan 2008 01:39:56 +0100 "Ulf Samuelsson" <[email protected]> wrote:

X-Mailer: Microsoft Outlook Express 6.00.2900.3138

Right...

Quoted 5 lines

please quote properly in your replies

==> Mailers can't quote properly when mails are sent as a text attachement.
Also quoting does not work when people are sending non-compliant mails allowing long lines.

That's because your mailer is a piece of crap. Get a better one.

Haavard

#9 lore raw
In reply to Mike Frysinger
Quoted 11 lines · Mike Frysinger
Quoted 9 lines · Ulf Samuelsson

==> And this is not a good approach, you want the S/W to use the
first 256 kB to store initial boot, U-boot and environment. The AT91SAM9 chips will use the first 8 kB sector for the AT91bootstrap, and that should have protection separate
from U-Boot code. AVR32 boot ROM will boot from a higher address and use the 8 kB sector at address zero as environment.
Both will need to have a S/W partitioning scheme added with separate protection for each partition.
This partioning scheme exists today.

Quoted 5 lines

sorry, i didnt mean to imply i thought this limitation was something to be written in stone. i understand perfectly why you would want this flexibility, so i can take a look. rather than making general statements, can you please provide a concrete list of all relevant examples you know of.

Ulf: can you at least follow up to this so i have a goal to work towards ? maybe i'll demand some sample parts if you want support ! :)
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080130/07f29032/attachment.pgp

#10 lore raw
In reply to Ulf Samuelsson

Ulf,

please get your quoting right!!!

Quoted 15 lines · In message <[email protected]> you wrote:
Quoted 5 lines · Ulf Samuelsson

Unfortunately, this code seems useless, at least for the combination AT91 + SPI flash. Some issues:

I believe that the AT45 is not using the same command set as other SPI flash memories. I think the commands need to be separated.

i already accounted for different command sets based on part family.

Quoted 1 line

AT45 is much more advanced than other SPI flash.

yes, but it still has a basic subset which can be used.

Quoted 1 line

Did you really test your code on the AT45 series?

Quoted 7 lines

it was the part i developed the code on originally (the 64mbit one).

Quoted 1 line

You assume, incorrectly, that all sector sizes are the same size.

that depends on what level you look at it. sector 0 can be accessed in pieces, but it can also be treated as one big sector the same as all the others. this is how i treated it.

This looks as if you were replying to your own postings, when you actualy reply to Mike's text!!!

Best regards,

Wolfgang Denk

Signature · 4 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Intel told us the Pentium would have "RISK" features...
#11 lore raw
In reply to Ulf Samuelsson
Quoted 3 lines · In message <[email protected]> you wrote:

==> This is not how the dataflash support is implemented in U-Boot today.
Current implementation is using DMA.

Note that you are referring to code that is not part of the public U-Boot tree, and that (as is) will never become a part of it.

Best regards,

Wolfgang Denk

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Without freedom of choice there is no creativity.
	-- Kirk, "The return of the Archons", stardate 3157.4
#12 lore raw
Quoted 8 lines
Quoted 3 lines · In message <[email protected]> you wrote:

==> This is not how the dataflash support is implemented in U-Boot today.
Current implementation is using DMA.

Note that you are referring to code that is not part of the public U-Boot tree, and that (as is) will never become a part of it.

No, I am referring to code which does exist in the U-Boot tree.

- Look how proper quoting can be when the sender of the original mail follows mail standards ...

Best Regards
Ulf Samuelsson

#13 lore raw
Quoted 8 lines · In message <[email protected]> you wrote:
Quoted 6 lines
Quoted 3 lines

==> This is not how the dataflash support is implemented in U-Boot today.
Current implementation is using DMA.

Note that you are referring to code that is not part of the public U-Boot tree, and that (as is) will never become a part of it.

No, I am referring to code which does exist in the U-Boot tree.

You mean the current dataflash implementation uses DMA? Which part of the code is this?

Best regards,

Wolfgang Denk

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
When the bosses talk about improving  productivity,  they  are  never
talking about themselves.
#14 lore raw
Quoted 8 lines
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 6 lines
Quoted 3 lines

==> This is not how the dataflash support is implemented in U-Boot today.
Current implementation is using DMA.

Note that you are referring to code that is not part of the public U-Boot tree, and that (as is) will never become a part of it.

Quoted 16 lines
Quoted 1 line

No, I am referring to code which does exist in the U-Boot tree.

You mean the current dataflash implementation uses DMA? Which part of the code is this?

Best regards,

Wolfgang Denk

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
When the bosses talk about improving  productivity,  they  are  never
talking about themselves.

Not sure as I do not have a source tree in front of me, but if I remember correctly, it was in the board specific "at45.c" which then was split in two files.
One generic "at45.c" and one CPU specific file "spi.c" which is located in the cpu/arm920t atmel specific directory, and the DMA functionality is in the CPU specific part. (Where it should be)

I think a good SPI implementation should (like today) create a command block in a buffer, and request the CPU specific driver to do a block transfer.

Having a generic driver do the small details of access will remove all possibilities for an efficient driver.

There can of course be a driver which sends bytes one by one, as a fallback , but I think this should be an option that can be overridden by a driver focusing on efficiency.

Best Regards
Ulf Samuelsson

#15 lore raw
Quoted 3 lines · In message <[email protected]> you wrote:
Quoted 3 lines

You mean the current dataflash implementation uses DMA? Which part of the code is this?

...

Quoted 8 lines

Not sure as I do not have a source tree in front of me, but if I remember correctly, it was in the board specific "at45.c" which then was split in two files.
One generic "at45.c" and one CPU specific file "spi.c" which is located in the cpu/arm920t atmel specific directory, and the DMA functionality is in the CPU specific part. (Where it should be)

Could you please look it up once you get access to the source code again? Thanks.

Best regards,

Wolfgang Denk

Signature · 6 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
He'd heard her use that sweet, innocent  tone  of  voice  before.  It
meant that, pretty soon, there was going to be trouble.
                                        - Terry Pratchett, _Truckers_
#16 lore raw
In reply to Mike Frysinger
Quoted 31 lines · Mike Frysinger
Quoted 5 lines · Ulf Samuelsson

Unfortunately, this code seems useless, at least for the combination AT91 + SPI flash. Some issues:

I believe that the AT45 is not using the same command set as other SPI flash memories. I think the commands need to be separated.

i already accounted for different command sets based on part family.

Quoted 1 line

AT45 is much more advanced than other SPI flash.

yes, but it still has a basic subset which can be used.

Quoted 1 line

Did you really test your code on the AT45 series?

it was the part i developed the code on originally (the 64mbit one).

Quoted 1 line

You assume, incorrectly, that all sector sizes are the same size.

that depends on what level you look at it. sector 0 can be accessed in pieces, but it can also be treated as one big sector the same as all the others. this is how i treated it.

Quoted 1 line

How do you do "byte writes" which is an important feature of the AT45?

simple: i dont. spi flash writing isnt something to be done constantly nor is it fast, so i dont sweat getting maximum performance.

Quoted 2 lines

Your code does not support DMA transfers, while the current dataflash code runs DMA up to 50 Mbps.

Quoted 12 lines

so ? the point of u-boot is to do everything in PIO mode.

Quoted 3 lines

Erasing the entire SPI flash is generally stupid, since you store the environment there. You typically also store the initial bootloader and U-Boot.

so the user is stupid if they erase the entire flash ? you could say the same thing about any flash type.

Quoted 3 lines

Very rarely you want to erase the complete flash ,and a protection mechanism is needed to avoid accidental overwrites.
The current solution allows dataflash pages to be protected.

I disagree on some product you use a spi flash to store other code and not nessarely store u-boot in it? (you can have 2 falsh)

Quoted 22 lines

yes, protection of pages (hardware and software) is missing.

Quoted 4 lines

Typically you want to store data with a checksum,since relying on the boot of the linux kernel to produce the error, will in my experience make people confused and they will spend a lot of time barking up the wrong tree.

ok ? not sure how this is relevant to the topic at hand.

Quoted 11 lines

There is a general problem with U-Boot which seems to assume that there is more RAM than flash in the system.
How do you easily copy 256 MB from an SD-Card to an onboard 256 MB NAND flash when the SDRAM is 64 MB?

Today, you have to use 10 lines (U-Boot occupies 1 MB) and that is really bad.

The vanilla way of supporting storage devices is really wasteful in resources, and you cannot compare two memory areas if the memory area is larger than half the SDRAM size.

Quoted 6 lines

ok ? u-boot is designed as a monitor to get the system bootstrapped and execute something else. it isnt an operating system, it doesnt get maximal performance, and it isnt supposed to support all sorts of extended features. what you describe as deficiency doesnt apply to the topic at hand and really sounds like a basic design decision for u-boot. if you want optimal performance, use Linux.

I also disagree you may need the optimal performance, in some project I need it.

Best Regards,
J.

Quoted 12 lines

-mike

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users

#17 lore raw
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

How do you do "byte writes" which is an important feature of the AT45?

simple: i dont. spi flash writing isnt something to be done constantly nor is it fast, so i dont sweat getting maximum performance.

This is an artificial limitation based on your _opinion_. How, why, or what someone chooses to write into spi flash should not be constrained by design.

Quoted 3 lines
Quoted 3 lines
Quoted 2 lines

Your code does not support DMA transfers, while the current dataflash code runs DMA up to 50 Mbps.

so ? the point of u-boot is to do everything in PIO mode.

This is also an opinion -- that I happen to disagree with.

Quoted 7 lines
Quoted 7 lines
Quoted 4 lines

Erasing the entire SPI flash is generally stupid, since you store the environment there. You typically also store the initial bootloader and U-Boot.

so the user is stupid if they erase the entire flash ? you could say the same thing about any flash type.

I have valid reasons, better _requirements_, for erasing an entire spi flash and have done so many times. I never realized that I was stupid ... and for all these years! ;-)

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

Very rarely you want to erase the complete flash ,and a protection mechanism is needed to avoid accidental overwrites.
The current solution allows dataflash pages to be protected.

I disagree on some product you use a spi flash to store other code and not nessarely store u-boot in it? (you can have 2 falsh)

Ditto -- I also disagree.

Quoted 2 lines
Quoted 2 lines

execute something else. it isnt an operating system, it doesnt get maximal performance, and it isnt supposed to support all sorts of extended features.

What it's _supposed_ to support can be debated. But I'm quite sure that preventing extended features is not a good thing to design into a subsystem from the outset.

Quoted 3 lines
Quoted 3 lines

what you describe as deficiency doesnt apply to the topic at hand and really sounds like a basic design decision for u-boot. if you want optimal performance, use Linux.

The pot is calling the kettle black here -- WRT basic design decisions. If I want optimal performance, I shouldn't have to find an alternative to u-boot simply because a subsystem prevents me from doing so by design.

I think many of the comments/suggestions in this email topic have been sound and raise some valid issues/concerns -- this is a good thing.

Regards,
--Scott

#18 lore raw
Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

Erasing the entire SPI flash is generally stupid, since you store the environment there. You typically also store the initial bootloader and U-Boot.

so the user is stupid if they erase the entire flash ? you could say the same thing about any flash type.

I have valid reasons, better _requirements_, for erasing an entire spi flash and have done so many times. I never realized that I was stupid ... and for all these years! ;-)

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

Very rarely you want to erase the complete flash ,and a protection mechanism is needed to avoid accidental overwrites.
The current solution allows dataflash pages to be protected.

I disagree on some product you use a spi flash to store other code and not nessarely store u-boot in it? (you can have 2 falsh)

OK, let me rephrase the statement:
It is stupid to allow the user to *accidently* erase the environment sector and the U-boot area The proposed user interface will do this, if you do not give any address parameters to the erase command.

Best Regards
Ulf Samuelsson

#19 lore raw
In reply to Ulf Samuelsson
Quoted 3 lines · In message <[email protected]> you wrote:

There is a general problem with U-Boot which seems to assume that there is more RAM than flash in the system.

U-Boot does not make any such assumption - at least none of the ports I know of.

Quoted 2 lines

How do you easily copy 256 MB from an SD-Card to an onboard 256 MB NAND flash when the SDRAM is 64 MB?

Please read the archives. We've had this discussion a long time before.

Quoted 3 lines

The vanilla way of supporting storage devices is really wasteful in resources, and you cannot compare two memory areas if the memory area is larger than half the SDRAM size.

You can compare *memory* areas of any size that is mapped into the processor's address space. I guess you are talking about storage devices.

Best regards,

Wolfgang Denk

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Committee, n.:  A group of men who individually can do nothing but as
a group decide that nothing can be done.                 - Fred Allen
#20 lore raw
In reply to Mike Frysinger
Quoted 1 line · On Mon, 28 Jan 2008 15:00:34 -0500 Mike Frysinger <[email protected]> wrote:

uint8_t spiflash_exchange_byte(uint8_t transmit)

Uh. Did you just propose SPI framework #6 for u-boot?

How about we try to modify one of the existing ones so that it can be useful for everything? That is, spi flash, spi eeproms, LCD panels, sensors, etc.?

In fact, I did post a patch that attempted this not so long ago, but I never got any response:

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33822/focus=33821

I'd really like to see a common layer for SPI flash though, so I don't mean to criticise your effort. But it would be nice if not every new SPI client came with its own low-level SPI functions...

Haavard

#21 lore raw
Quoted 10 lines · Haavard Skinnemoen
Quoted 1 line · On Mon, 28 Jan 2008 15:00:34 -0500 Mike Frysinger <[email protected]> wrote:

uint8_t spiflash_exchange_byte(uint8_t transmit)

Uh. Did you just propose SPI framework #6 for u-boot?

How about we try to modify one of the existing ones so that it can be useful for everything? That is, spi flash, spi eeproms, LCD panels, sensors, etc.?

Quoted 8 lines

In fact, I did post a patch that attempted this not so long ago, but I never got any response:

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33822/focus=33821

I'd really like to see a common layer for SPI flash though, so I don't mean to criticise your effort. But it would be nice if not every new SPI client came with its own low-level SPI functions...

i know exactly where you're coming from, so dont sweat it. my purpose was to get things integrated and then take a look at the existing pieces and unify them. easier i think to have known working code to throw at a common framework than code for hypothetical uses we cant actually break our teeth on.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080128/7ae3f643/attachment.pgp

#22 lore raw
Quoted 9 lines · On Mon, 28 Jan 2008 19:29:38 -0500 Mike Frysinger <[email protected]> wrote:
Quoted 3 lines

I'd really like to see a common layer for SPI flash though, so I don't mean to criticise your effort. But it would be nice if not every new SPI client came with its own low-level SPI functions...

i know exactly where you're coming from, so dont sweat it. my purpose was to get things integrated and then take a look at the existing pieces and unify them. easier i think to have known working code to throw at a common framework than code for hypothetical uses we cant actually break our teeth on.

Ok, don't worry about it then. I just wanted to make sure you investigate the API(s) we already have before you propose something new.

So let's just make "please use the include/spi.h API for low-level SPI access" be one of the comments on your RFC :-)

Haavard

#23 lore raw
Quoted 11 lines · Haavard Skinnemoen
Quoted 9 lines · Mike Frysinger
Quoted 3 lines

I'd really like to see a common layer for SPI flash though, so I don't mean to criticise your effort. But it would be nice if not every new SPI client came with its own low-level SPI functions...

i know exactly where you're coming from, so dont sweat it. my purpose was to get things integrated and then take a look at the existing pieces and unify them. easier i think to have known working code to throw at a common framework than code for hypothetical uses we cant actually break our teeth on.

Quoted 6 lines

Ok, don't worry about it then. I just wanted to make sure you investigate the API(s) we already have before you propose something new.

So let's just make "please use the include/spi.h API for low-level SPI access" be one of the comments on your RFC :-)

do you have a git tree for this so i can pull it down and test it for Blackfin ? i can see about posting an updated implementation then ... -mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080129/a4fc0fe7/attachment.pgp

#24 lore raw
Quoted 5 lines · On Tue, 29 Jan 2008 09:13:32 -0500 Mike Frysinger <[email protected]> wrote:
Quoted 2 lines

So let's just make "please use the include/spi.h API for low-level SPI access" be one of the comments on your RFC :-)

do you have a git tree for this so i can pull it down and test it for Blackfin ? i can see about posting an updated implementation then ...

No, it's really just the patch I mentioned, and a follow-up patch with a hardware spi driver that implements the new API. include/spi.h exists today, I just did a couple of tweaks to make it more useful with hardware SPI controllers.

The patch needs to be rebased and possibly updated to fix up more boards and drivers. I'm hoping to get it done this week, but I won't promise anything.

Haavard

#25 lore raw
In reply to Haavard Skinnemoen
Quoted 9 lines · Haavard Skinnemoen
Quoted 2 lines · On Mon, 28 Jan 2008 15:00:34 -0500 Mike Frysinger <[email protected]> wrote:

uint8_t spiflash_exchange_byte(uint8_t transmit)

Uh. Did you just propose SPI framework #6 for u-boot?

Quoted 10 lines

How about we try to modify one of the existing ones so that it can be useful for everything? That is, spi flash, spi eeproms, LCD panels, sensors, etc.?

In fact, I did post a patch that attempted this not so long ago, but I never got any response:

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33822/focus=33821

Huh, missed that one :-( . Nice work - getting rid of the table is a good thing. If you re-base and post again I can do some testing.

regards,
Ben

#26 lore raw
Quoted 5 lines · On Mon, 28 Jan 2008 19:44:55 -0500 Ben Warren <[email protected]> wrote:
Quoted 3 lines

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33822/focus=33821

Huh, missed that one :-( . Nice work - getting rid of the table is a good thing. If you re-base and post again I can do some testing.

That would be great. I'm currently a bit busy trying to extend the Linux DMA Engine framework, but I'll see if I can get it rebased before this week ends.

Haavard