[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?
From: Timur Tabi timur@freescale.com
This patch adds support for the reversed geometry data in some AMD flash chips.
I'm not proud of this patch, so I'm posting it for review only. I know it works on the my board that was the problem, but I have no idea if it will break any other board. I'm sure this code could be improved a lot.
Signed-off-by: Timur Tabi timur@freescale.com --- drivers/cfi_flash.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c index fd0a186..8cc8d60 100644 --- a/drivers/cfi_flash.c +++ b/drivers/cfi_flash.c @@ -1120,6 +1120,7 @@ ulong flash_get_size (ulong base, int ba uchar num_erase_regions; int erase_region_size; int erase_region_count; + int geometry_reversed = 0; #ifdef CFG_FLASH_PROTECTION int ext_addr; info->legacy_unlock = 0; @@ -1148,6 +1149,8 @@ #endif case CFI_CMDSET_AMD_STANDARD: case CFI_CMDSET_AMD_EXTENDED: info->cmd_reset = AMD_CMD_RESET; + if (flash_read_uchar(info, FLASH_OFFSET_CFI_RESP + (0x7E / info->portwidth)) == 3) + geometry_reversed = 1; break; }
@@ -1171,8 +1174,13 @@ #endif num_erase_regions, NUM_ERASE_REGIONS); break; } - tmp = flash_read_long (info, 0, + if (geometry_reversed) + tmp = flash_read_long (info, 0, FLASH_OFFSET_ERASE_REGIONS + + (num_erase_regions - 1 - i) * 4); + else + tmp = flash_read_long (info, 0, + FLASH_OFFSET_ERASE_REGIONS + i * 4); erase_region_size = (tmp & 0xffff) ? ((tmp & 0xffff) * 256) : 128; -- 1.4.2.1
timur@freescale.com wrote:
From: Timur Tabi timur@freescale.com
This patch adds support for the reversed geometry data in some AMD flash chips.
I'm not proud of this patch, so I'm posting it for review only. I know it works on the my board that was the problem, but I have no idea if it will break any other board. I'm sure this code could be improved a lot.
NAK. It will break any bottom boot AMD flash. I have reviewed Linux MTD driver and the coder had nice things to say about AMD regarding this matter (!).
Anyway, I will get proper patch to you to try tonight or tomorrow. I do not have a board with AMD flash myself.
Tolunay
-----Original Message----- From: u-boot-users-bounces@lists.sourceforge.net [mailto:u-boot-users-bounces@lists.sourceforge.net] On Behalf Of timur@freescale.com Sent: Monday, November 06, 2006 4:47 PM To: u-boot-users@lists.sourceforge.net Subject: [U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check fortop/bottom boot?
From: Timur Tabi timur@freescale.com
This patch adds support for the reversed geometry data in some AMD flash chips.
<snip> case CFI_CMDSET_AMD_STANDARD: case CFI_CMDSET_AMD_EXTENDED: info->cmd_reset = AMD_CMD_RESET; + if (flash_read_uchar(info, FLASH_OFFSET_CFI_RESP + (0x7E / info->portwidth)) == 3)
The code also needs to check: 1) that there is an extended query table 2) The query table is version 1.1 or later
Then (and only then) is it safe to use this byte from the query table.
Nick
Here is the patch (attached) that handles top boot geometry reversal case on AMD flash.
This version is only for testing and commenting purposes. I've basically ported the way Linux MTD driver handles this issue (so I think). If it seems to work properly, I will prepare an official patch for submission. I appreciate if you can test on your AMD flash command set parts using CFI driver.
The patch is against the top of the tree.
Tolunay
Tolunay Orkun wrote:
Here is the patch (attached) that handles top boot geometry reversal case on AMD flash.
This patch works for me, so I approve it. I tested it on the board that has this problem, as well as an 8360-based board that has only same-sized sectors.
Just one nit:
Adds trailing whitespace. .dotest/patch:35: man_id = flash_read_uchar (info, Adds trailing whitespace. .dotest/patch:37: dev_id = flash_read_uchar(info, warning: 2 lines add trailing whitespaces.
Timur Tabi wrote:
Tolunay Orkun wrote:
Here is the patch (attached) that handles top boot geometry reversal case on AMD flash.
This patch works for me, so I approve it. I tested it on the board that has this problem, as well as an 8360-based board that has only same-sized sectors.
Glad to hear. Hopefully I will get more positive feedback.
Just one nit:
Adds trailing whitespace. .dotest/patch:35: man_id = flash_read_uchar (info, Adds trailing whitespace. .dotest/patch:37: dev_id = flash_read_uchar(info, warning: 2 lines add trailing whitespaces.
I will fix that.
Questions for the maintainers:
1) I have some new variables for man_id, dev_id, cfi_version introduced by this patch (actually man_id is currently not used so could be removed). I can kept these local to the function for now or I can add them to the info structure along with ext_addr (to easily locate the Vendor Extended Query Structure in flash). What do you think?
In the minimum displaying man_id, dev_id, cfi_version when executing "flinfo" command could be useful for diagnostic purposes. We might need to use them to tweak the behavior of some other functionality in the future as well.
2) There are some outdated comments at the beginning of the cfi_flash.c. The modification history is not up to date and todo list stuff is mostly addressed over time. Since the actual history is already is the git, I propose to get rid of modification history and todo list from the comments.
Best regards, Tolunay
Hi Tolunay,
On Tuesday 07 November 2006 19:01, Tolunay Orkun wrote:
Questions for the maintainers:
- I have some new variables for man_id, dev_id, cfi_version introduced by
this patch (actually man_id is currently not used so could be removed). I can kept these local to the function for now or I can add them to the info structure along with ext_addr (to easily locate the Vendor Extended Query Structure in flash). What do you think?
In the minimum displaying man_id, dev_id, cfi_version when executing "flinfo" command could be useful for diagnostic purposes. We might need to use them to tweak the behavior of some other functionality in the future as well.
Yes, I definitely think it's useful to display this ID information (or even better a real manufacturer/device string) in the "flinfo" command. So my vote is to extend the struct.
- There are some outdated comments at the beginning of the cfi_flash.c.
The modification history is not up to date and todo list stuff is mostly addressed over time. Since the actual history is already is the git, I propose to get rid of modification history and todo list from the comments.
ACK.
Thanks for helping out here.
Best regards, Stefan
Stefan Roese wrote:
Hi Tolunay,
On Tuesday 07 November 2006 19:01, Tolunay Orkun wrote:
Questions for the maintainers:
- I have some new variables for man_id, dev_id, cfi_version introduced by
this patch (actually man_id is currently not used so could be removed). I can kept these local to the function for now or I can add them to the info structure along with ext_addr (to easily locate the Vendor Extended Query Structure in flash). What do you think?
In the minimum displaying man_id, dev_id, cfi_version when executing "flinfo" command could be useful for diagnostic purposes. We might need to use them to tweak the behavior of some other functionality in the future as well.
Yes, I definitely think it's useful to display this ID information (or even better a real manufacturer/device string) in the "flinfo" command. So my vote is to extend the struct.
I just noticed the info structure has a flash_id field which the comment says it is for combined manufacturer id (ms 16-bit) and device id (ls 16-bit).
However, for CFI driver the value stored there is FLASH_MAN_CFI. I can get rid of that and store the actual flash id but I will have to get rid of the checks in the driver that verifies the field to be FLASH_MAN_CFI.
Best regards, Tolunay
Stefan Roese wrote:
Hi Tolunay,
On Tuesday 07 November 2006 19:01, Tolunay Orkun wrote:
Questions for the maintainers:
- I have some new variables for man_id, dev_id, cfi_version introduced by
this patch (actually man_id is currently not used so could be removed). I can kept these local to the function for now or I can add them to the info structure along with ext_addr (to easily locate the Vendor Extended Query Structure in flash). What do you think?
In the minimum displaying man_id, dev_id, cfi_version when executing "flinfo" command could be useful for diagnostic purposes. We might need to use them to tweak the behavior of some other functionality in the future as well.
Yes, I definitely think it's useful to display this ID information (or even better a real manufacturer/device string) in the "flinfo" command. So my vote is to extend the struct.
I just noticed the info structure has a flash_id field which the comment says it is for combined manufacturer id (ms 16-bit) and device id (ls 16-bit).
However, for CFI driver the value stored there is FLASH_MAN_CFI. I can get rid of that and store the actual flash id but I will have to get rid of the checks in the driver that verifies the field to be FLASH_MAN_CFI.
Best regards, Tolunay
Tolunay Orkun wrote:
Subject: Re: [U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?
Here is the patch (attached) that handles top boot geometry reversal case on AMD flash.
You probably need to add a check that ext_addr is non-zero. If the extended query table is not present then you can't read the version number (otherwise you read the reserved section at 0h which is undefined)
Nick
Spence Nick-rxtd10 wrote:
Tolunay Orkun wrote:
Subject: Re: [U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?
Here is the patch (attached) that handles top boot geometry reversal case on AMD flash.
You probably need to add a check that ext_addr is non-zero. If the extended query table is not present then you can't read the version number (otherwise you read the reserved section at 0h which is undefined)
I thought about it and it is simple to do. That line that sets the ext_addr was already present for Intel case (to check legacy lock feature from extended query table) so I just relocated out of the case statement.
Still, I do not know of any actual CFI compliant flash that lacks an extended query table. Perhaps, it might have been relevant to old non-CFI JEDEC flash which we do not handle with this driver.
I do not want to introduce code that will not be applicable. What do you think? I guess I can put the check for robustness just in case there happens to be an odd one in the market.
Tolunay
Tolunay Orkun wrote:
I do not want to introduce code that will not be applicable. What do you think? I guess I can put the check for robustness just in case there happens to be an odd one in the market.
My vote is to add the check that Nick suggests, simply to limit the possibility of false positives.
On Tuesday 07 November 2006 19:38, Timur Tabi wrote:
I do not want to introduce code that will not be applicable. What do you think? I guess I can put the check for robustness just in case there happens to be an odd one in the market.
My vote is to add the check that Nick suggests, simply to limit the possibility of false positives.
ACK.
Best regards, Stefan
participants (5)
-
Spence Nick-rxtd10 -
Stefan Roese -
Timur Tabi -
timur＠freescale.com
-
Tolunay Orkun