Re: [U-Boot-Users] [PATCH] use CFI-Flash and board depended driver together

Hi, The callback function which obtains the portwidth, chipwidth is the prefer option for a more generic solution. The driver will then support the none CFI devices as well. Call for a board specific function to setup the flash_info_t for that bank will cause a code duplication for auto detect of none CFI devices. Thanks, Yotam
-----Original Message----- From: Tolunay Orkun [mailto:listmember@orkun.us] Sent: Tuesday, December 05, 2006 2:56 AM To: U-Boot Cc: Stefan Roese; Jens Scharsig; Andrew Dyer; Rui Sousa; Timur Tabi; Yotam Admon; matvejchikov@gmail.com Subject: Re: [U-Boot-Users] [PATCH] use CFI-Flash and board depended driver together
Tolunay Orkun wrote:
Jens Scharsig wrote:
Hello,
i need to use cfi and board flash driver together. This Patch
contains a
simple way to do this with a minimun changes at cfi_flash.c. All
changes
will only enable if CFG_FLASH_BOARD_DRIVER defined.
CHANGELOG
- Rename flash_print_info, flash_erase, write_buff and flash_real_protect to cfi_xxxx, if CFG_FLASH_BOARD_DRIVER is defined. This will allow use cfi driver together with board a dependent flash driver (see README.board-dependent-flash for an example)
I have to object to this patch. I will be providing a testing patch tonight or tomorrow that will address this issue properly which is teaching cfi_flash driver to deal with non-cfi flash. There is no need
to have two flash drivers...
I've started with this patch but ran into some roadblocks so it did not happen last Friday as promised. I have been thinking better solutions that avoids two sets of flash drivers.
Basically, I was trying to detect the port and chip width by using a technique similar to flash_detect_cfi() but instead of checking for combinations of Q R Y etc., I was trying to see there was a change in the first 16 bytes of flash. Well that seemed workable until I actually tried the modified code :( The probe this way failed miserably with Intel flash responding to 0x90 in either in upper or lower byte even in 16-bit mode!
Thus, if the flash is not detected by CFI method the portwidth and chipwidth will have to be set manually for even jedec ids to be read properly. If the jedec probe worked, the jedec ids could be used to lookup a static list for proper configuration of info structure.
I have not given up yet. I would like to start over and take a simpler approach. Here is the idea:
The CFI driver will do CFI probe. If the probe is not successful and board configuration has defined a macro, CFI driver will call a board specific
function to setup the flash_info_t for that bank.
I will pass the bank number to the function as argument. If the function
returns non-zero, I will assume that board has setup the info structure successfully (hopefully) and move on. This still avoids driver code duplication by only passing the responsibility of setting up the info structure to the board.
A variation of this could be the callback function obtains the portwidth, chipwidth (and possibly the command set) for the specific bank and do flash_read_jedec_ids to get the ids and lookup the values from a statically compiled list like I originally intended. This list would be compiled in
compiled only if this feature is enabled, say CFG_FLASH_CFI_JEDEC. Instead of a callback we could use the flash_base address list and augment the array to an array of structures which include base, portwidth and chipwidth but that would require a change in all boards that uses CFI driver. A callback to the board supplied function is the least invasive way in my opinion.
In either method, to cut the compiled image size a bit if there is definitely no cfi compliant flash on any bank we can omit the cfi detection and other cfi related setup code using a macro like CFG_FLASH_CFI_DISABLE (or something like that).
I would like to hear opinions regarding which choices looks more attractive...
Tolunay

Hi all,
While I try to examine cfi_flash.c file I have found that flash_detect_cfi() function used uninitialized value of info->cmd_reset:
flash_get_size() { .... if (flash_detect_cfi(info)) { .... switch (info->vendor) { .... info->cmd_reset = x_CMD_RESET; .... } } }
flash_detect_cfi(info) { .... flash_write_cmd (info, 0, 0, info->cmd_reset); .... }
Am I right? Or may be - 'Is it true?' :)
Best regards, Matvejchikov Ilya, Russia, Moscow.

Matvejchikov Ilya wrote:
Hi all,
While I try to examine cfi_flash.c file I have found that flash_detect_cfi() function used uninitialized value of info->cmd_reset:
flash_get_size() { .... if (flash_detect_cfi(info)) { .... switch (info->vendor) { .... info->cmd_reset = x_CMD_RESET; .... } } }
flash_detect_cfi(info) { .... flash_write_cmd (info, 0, 0, info->cmd_reset); .... }
Am I right? Or may be - 'Is it true?' :)
Good obvervation... It is true but mostly harmless (if the garbage in cmd_reset does not happen to be a valid command). The reset command is issued if the attempt to put flash in CFI mode has failed. The line could be removed and it would still work. When I designed my last patch flash_read_jedec_ids(), I've specifically avoided the info->cmd_reset for this reason.
Actually, AMD_CMD_RESET should be applicable to most Intel parts as well per CFI spec documents. I think the Linux MTD driver choose to send the Intel sequence followed by AMD sequence back to back during the probe.
BTW, Were you able to read my proposals to extend the CFI flash driver to handle non-cfi chips. I've only received one comment so far. I am waiting to get more comments before I go ahead with coding effort....
Best regards, Tolunay
participants (3)
-
Matvejchikov Ilya
-
Tolunay Orkun
-
Yotam Admon