-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable the RWW function of Macronix Flash MX25LW51245G in OSPI block device driver #14221
Conversation
@rogeryou, thank you for your changes. |
@rogeryou, thank you for your changes. |
Please fix the styling |
#define MX25LM51245G_CHUNK_SIZE 0x10 /* fred: 16 bytes */ | ||
|
||
#ifdef MX_FLASH_SUPPORT_RWW | ||
#define MX25LM51245G_BANK_SIZE 0x01000000 /* fred: 16 MBytes */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you don't create a new file MX25LW51245G_config.h ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created a new file MX25LW51245G_config.h .
@@ -62,6 +62,8 @@ | |||
#define MBED_CONF_OSPIF_OSPI_FREQ 40000000 | |||
#endif | |||
|
|||
#define MX_FLASH_SUPPORT_RWW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not related to the memory ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is.
I have moved it to MX25LW51245G_config.h.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DISCO_L4R9I doesn't compile any more :-(
OSPIF_CR2_OPI_EN_ADDR is missing
I have fixed this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non regression tests OK
@@ -34,7 +34,7 @@ | |||
|
|||
/* Max amount of flash size is 4Gbytes */ | |||
/* hence 2^(31+1), then FLASH_SIZE_DEFAULT = 1<<31 */ | |||
#define OSPI_FLASH_SIZE_DEFAULT 0x80000000 | |||
#define OSPI_FLASH_SIZE_DEFAULT 0x4000000 //512Mbits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't understand why driver is using a "default" flash size...?
All octo SPI are the same?
(maybe comment above is wrong now?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is copy from qspi_api.c.
I found a issue about this value.
I modified the code below.
obj->handle.Init.DeviceSize = POSITION_VAL(OSPI_FLASH_SIZE_DEFAULT) - 1;
obj->handle.Init.DeviceSize = 32;
The driver need to execute command RDCR2 to read the bank status of flash. This command need to send address(0xC0000000> OSPI_FLASH_SIZE_DEFAULT). The controller will report error when i execute RDCR2.
SO i need to change the value of DeviceSize.
#define MX_FLASH_BLOCK_SIZE 0x10000 /* 1024 blocks of 64 KBytes */ | ||
#define MX_FLASH_SECTOR_SIZE 0x1000 /* 16384 sectors of 4 kBytes */ | ||
#define MX_FLASH_PAGE_SIZE 0x100 /* 262144 pages of 256 bytes */ | ||
#define MX_FLASH_CHUNK_SIZE 0x10 /* 16 bytes */ | ||
#define MX_FLASH_BANK_SIZE 0x01000000 /* 16 MBytes */ | ||
#define MX_FLASH_BANK_SIZE_MASK ~(MX_FLASH_BANK_SIZE - 1) /* 0xFF000000 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Macro not used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes , these Macro are not used in this version. They will be used in VEEBlockDevice driver which I will push to github later.
#ifdef NEED_DEFINE_SFDP_PARA | ||
uint8_t _sfdp_head_table[32] = {0x53, 0x46, 0x44, 0x50, 0x06, 0x01, 0x02, 0xFF, 0x00, 0x06, 0x01, | ||
0x10, 0x30, 0x00, 0x00, 0xFF, 0xC2, 0x00, 0x01, 0x04, 0x10, 0x01, | ||
0x00, 0xFF, 0x84, 0x00, 0x01, 0x02, 0xC0, 0x00, 0x00, 0xFF | ||
}; | ||
uint8_t _sfdp_basic_param_table[64] = {0x30, 0xFF, 0xFB, 0xFF, 0xFF, 0xFF, 0xFF, 0x1F, 0xFF, 0xFF, | ||
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x01, 0x14, 0xEC, | ||
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x0C, 0x20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that values for this workaround is specific for each OSPI,
so this should be kept in this config file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. But it will report some errors when compiled if more than one file include this config file. _sfdp_head_table will be redefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as these should be rather const and static if in header file to avoid that.
Moving to code file if they are not reused anywhere is fine.
This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, @ARMmbed/mbed-os-core, please complete review of the changes to move the PR forward. Thank you for your contributions. |
#ifdef NEED_DEFINE_SFDP_PARA | ||
uint8_t _sfdp_head_table[32] = {0x53, 0x46, 0x44, 0x50, 0x06, 0x01, 0x02, 0xFF, 0x00, 0x06, 0x01, | ||
0x10, 0x30, 0x00, 0x00, 0xFF, 0xC2, 0x00, 0x01, 0x04, 0x10, 0x01, | ||
0x00, 0xFF, 0x84, 0x00, 0x01, 0x02, 0xC0, 0x00, 0x00, 0xFF | ||
}; | ||
uint8_t _sfdp_basic_param_table[64] = {0x30, 0xFF, 0xFB, 0xFF, 0xFF, 0xFF, 0xFF, 0x1F, 0xFF, 0xFF, | ||
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x01, 0x14, 0xEC, | ||
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x0C, 0x20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as these should be rather const and static if in header file to avoid that.
Moving to code file if they are not reused anywhere is fine.
CI restarted |
Jenkins CI Test : ❌ FAILEDBuild Number: 5 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
CI restarted |
Jenkins CI Test : ❌ FAILEDBuild Number: 6 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
One more restart |
Jenkins CI Test : ❌ FAILEDBuild Number: 7 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Jenkins CI Test : ✔️ SUCCESSBuild Number: 8 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Summary of changes
Update PR #12644.
Add source code about RWW(read while write) in OSPI block device driver for using the RWW function Macronix octaflash MX25LW51245G to improve performance.
Impact of changes
Enable the RWW function of MX25LW51245G.
Migration actions required
Documentation
N/A
N/A
Pull request type
Test results
This driver is tested on DISCO_L4R9I. The flash on this board is MX25LM51245G. You need to replace it with MX25LW51245G.
The test case for RWW is "Testing read while write and read while erase". You also need to replace MX25LM51245G with MX25LW51245G about DISCO_L4R9I in target.json.
Reviewers
@jeromecoutant Please review.