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

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

[Jens Scharsig]

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)



-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch_mix_bdflash_cfi.diff
Url: http://lists.denx.de/pipermail/u-boot/attachments/20061130/9f0805c4/attachment.txt 

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

[Timur Tabi]

Jens Scharsig wrote:

> +#ifdef CFG_FLASH_BOARD_DRIVER
> +#define flash_print_info cfi_flash_print_info
> +#define flash_erase cfi_flash_erase
> +#define write_buff  cfi_write_buff
> +#define flash_real_protect cfi_flash_real_protect
> +#endif

Ugh, I really dislike code like this.  I know Wolfgang has the final say 
on these things, but I would rather you did something else.  For 
instance, if CFG_FLASH_BOARD_DRIVER is not defined, then the cfi_xxx 
functions are defined here.  Otherwise, they are declared as externs and 
they need to be defined in the board file.  Like this:

#ifdef CFG_FLASH_BOARD_DRIVER
extern ... cfi_flash_print_info(...);
extern ... cfi_flash_erase(...);
extern ... cfi_write_buff(...);
extern ... cfi_flash_real_protect(...);
#else
... cfi_flash_print_info(...)
{
	// original cfi_flash_print_info code here
}

// and so on

#endif


> +#ifdef CFG_FLASH_BOARD_DRIVER
> +		size += flash_info[i].size = board_flash_get_size (bank_base[i], i);
> +#else
>  		size += flash_info[i].size = flash_get_size (bank_base[i], i);
> +#endif

You could do the same thing here.  Rename flash_get_size() to 
cfi_flash_get_size() and treat it like the other cfi_xxx functions above.

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

[Jens Scharsig]

Timur Tabi schrieb:

 >Ugh, I really dislike code like this.

I know,this code is dirty. But it's very simple and works for me.

> on these things, but I would rather you did something else.  For 
> instance, if CFG_FLASH_BOARD_DRIVER is not defined, then the cfi_xxx 
> functions are defined here.  Otherwise, they are declared as externs and 

No, if CFG_FLASH_BOARD_DRIVER is not defined cfi_xxx  functions are not 
defined in orginal cfi_flash.c. In this case all other uboot moduls can 
call functions under orginal names.

> You could do the same thing here.  Rename flash_get_size() to 
> cfi_flash_get_size() and treat it like the other cfi_xxx functions above.

A simple rename doesn't work.

flash_init calls board_flash_get_size
and
board_flash_getsize calls the orginal flash_get_size, if selected bank 
is suppored by cfi driver

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

[Timur Tabi]

Jens Scharsig wrote:

> No, if CFG_FLASH_BOARD_DRIVER is not defined cfi_xxx  functions are not 
> defined in orginal cfi_flash.c. In this case all other uboot moduls can 
> call functions under orginal names.

Ok, I get it now.  But all you're basically doing is hiding the CFI flash 
routines.  The flash commands in cmd_flash.c are only going to call the 
board-specific routines and not the CFI routines.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

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

[Jens Scharsig]

Timur Tabi  wrote:
> 
> Ok, I get it now.  But all you're basically doing is hiding the CFI flash 
> routines.  The flash commands in cmd_flash.c are only going to call the 
> board-specific routines and not the CFI routines.
> 
That's my inspiration. The flash commands call board-specific routines 
and board-specific routines call back the orginal cfi routines, if i 
want this.

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

[Tolunay Orkun]

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...

Tolunay

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

[Tolunay Orkun]

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

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

[Tolunay Orkun]

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

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

[Yotam Admon]

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 at 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 at 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

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

[Matvejchikov Ilya]

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.

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

[Tolunay Orkun]

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