You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/02/27 10:51:22 UTC

[GitHub] [incubator-nuttx] bjbrandt opened a new pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

bjbrandt opened a new pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391
 
 
   Add progmen flash block mapping for the STM32[F2/F4] family.
   
   This is based on the STM32F7 flash driver (commit 29164c5706490b74a84d44ba0f79cd588fd3e910).

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] bjbrandt closed pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

bjbrandt closed pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

davids5 commented on a change in pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#discussion_r385136773
 
 

 ##########
 File path: arch/arm/src/stm32/stm32f20xxf40xx_flash.c
 ##########
 @@ -226,83 +279,65 @@ int stm32_flash_writeprotect(size_t page, bool enabled)
       up_waste();
     }
 
-  /* Relock options */
+  /* Re-lock options */
 
   modifyreg32(STM32_FLASH_OPTCR, 0, FLASH_OPTCR_OPTLOCK);
   return 0;
 }
 
 size_t up_progmem_pagesize(size_t page)
 {
-  static const size_t page_sizes[STM32_FLASH_NPAGES] = STM32_FLASH_SIZES;
-
-  if (page >= sizeof(page_sizes) / sizeof(*page_sizes))
-    {
-      return 0;
-    }
-  else
-    {
-      return page_sizes[page];
-    }
+  return PAGESIZE;
 
 Review comment:
   @bjbrandt Maybe I should ask how this is used?  What happens if I call up_progmem_ispageerased(0), it use to tell me that the sector was erased.  Then it would be used as
   
   ```
   	/* blank-check the sector */
   
   	bool blank = up_progmem_ispageerased(sector) == 0;
   
   	/* erase the sector if it failed the blank check */
   	if (!blank) {
   		up_progmem_eraseblock(sector);
   ```
   How would this be done wit these changes?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] davids5 commented on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

davids5 commented on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#issuecomment-592470656

@bjbrandt -

I think we need to step back and look at the API. It seems to have become very corrupted.
Blocks, pages and Sectors have been muddled. The only thing that matters in the STM32 Soc is the Sector size and sizes.

For a flash driver to have Random Read/Write access we need 1 or ideally 2 backing RAM buffers.
This is because erase sizes are on Sector boundaries. This works fine on Soc's with Lots of RAM and/or SMALL Sector sizes. (Like 2K) It does not work on parts Like the F4, F7, H7

![image](https://user-images.githubusercontent.com/1945821/75544016-f0bd4d00-59d7-11ea-9eee-064e7dceb660.png)

It appears to me that the changes in 29164c5 and herein are not correct. But I may just be confused by the API.

Consider the use case of a bootloader: How does one implement a simple Erase From Sector 2 to N (23 on the F4) and program from Sector 2 - n?

With the changes in 29164c5 and herein what are the calls to do this? Can it even be done?

@patacongo Would you be able to add the history on the `up_progmem_.*` API and what it shuld be able to support?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

davids5 commented on a change in pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#discussion_r385117465
 
 

 ##########
 File path: arch/arm/src/stm32/stm32f20xxf40xx_flash.c
 ##########
 @@ -226,83 +279,65 @@ int stm32_flash_writeprotect(size_t page, bool enabled)
       up_waste();
     }
 
-  /* Relock options */
+  /* Re-lock options */
 
   modifyreg32(STM32_FLASH_OPTCR, 0, FLASH_OPTCR_OPTLOCK);
   return 0;
 }
 
 size_t up_progmem_pagesize(size_t page)
 {
-  static const size_t page_sizes[STM32_FLASH_NPAGES] = STM32_FLASH_SIZES;
-
-  if (page >= sizeof(page_sizes) / sizeof(*page_sizes))
-    {
-      return 0;
-    }
-  else
-    {
-      return page_sizes[page];
-    }
+  return PAGESIZE;
 
 Review comment:
   This will not work on the F4 the page (sector) size is not constant. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

davids5 commented on a change in pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#discussion_r389250669
 
 

 ##########
 File path: arch/arm/src/stm32/stm32f20xxf40xx_flash.c
 ##########
 @@ -332,7 +367,7 @@ ssize_t up_progmem_ispageerased(size_t page)
 
 ssize_t up_progmem_eraseblock(size_t block)
 {
-  if (block >= STM32_FLASH_NPAGES)
+  if (block >= PROGMEM_NBLOCKS)
 
 Review comment:
   Referring to https://github.com/messBUS/incubator-nuttx/blob/export/include/nuttx/progmem.h#L142-L162
   ```
    * Returned Value:
    *   block size or negative value on error.  The following errors are reported
   ``
   
   This function should return the number of byte (sector size) of the erased sector.
   
   This PR violated that API - and will cause breaking changes to all  calles. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] bjbrandt commented on a change in pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

bjbrandt commented on a change in pull request #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#discussion_r385395911
 
 

 ##########
 File path: arch/arm/src/stm32/stm32f20xxf40xx_flash.c
 ##########
 @@ -226,83 +279,65 @@ int stm32_flash_writeprotect(size_t page, bool enabled)
       up_waste();
     }
 
-  /* Relock options */
+  /* Re-lock options */
 
   modifyreg32(STM32_FLASH_OPTCR, 0, FLASH_OPTCR_OPTLOCK);
   return 0;
 }
 
 size_t up_progmem_pagesize(size_t page)
 {
-  static const size_t page_sizes[STM32_FLASH_NPAGES] = STM32_FLASH_SIZES;
-
-  if (page >= sizeof(page_sizes) / sizeof(*page_sizes))
-    {
-      return 0;
-    }
-  else
-    {
-      return page_sizes[page];
-    }
+  return PAGESIZE;
 
 Review comment:
   @davids5 For this purpose it would be helpful to have a function like up_progmem_iseraseblockerased() or up_progmem_issectorerased(), I guess. As long as there is no such function defined in progmem.h you need to iterate over all pages of the desired sector instead. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] bjbrandt commented on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

bjbrandt commented on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#issuecomment-598662925
 
 
   @davids5 - Thanks for reviewing this. Of course you are right with your API concerns. I trusted in the former commit and hence did not check the compability - just tested both with the MTD driver and the SMARTFS file system and it worked (as just some functions are used by this drivers). 
   
   I gonna close this PR and open a new one when i find the time to implement the requested changes.  

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] bjbrandt commented on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

bjbrandt commented on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#issuecomment-592500639
 
 
   > I think we need to step back and look at the API. It seems to have become very corrupted.
   Blocks, pages and Sectors have been muddled. The only thing that matters in the STM32 Soc is the Sector size and sizes.
   
   I think the use of terms like 'pages' and ' erase blocks' is just a try of generalization. Regarding the STM32 family an 'erase block' is just an alias for 'sector' while 'page' means the smaller r/w hunks (that do not have a name in the STM32 Reference manual). It is defined that an erase block is always a multiple of a single r/w page.
   
   
   > Consider the use case of a bootloader: How does one implement a simple Erase From Sector 2 to N (23 on the F4) and program from Sector 2 - n?
   > 
   > With the changes in [29164c5](https://github.com/apache/incubator-nuttx/commit/29164c5706490b74a84d44ba0f79cd588fd3e910) and herein what are the calls to do this? Can it even be done?
   
   If I got the API right, it should be done like this:
   
   ```
   /* erase sector 2 - N */
   
   for (int block = 1; block < N; ++i)
   {
      up_progmem_eraseblock(block);
   }
   
   /* get the first write page */
   
   size_t page = up_progmem_erasesize(0) / up_progmem_pagesize(0); 
   
   /* get the address off the desired page */
   
   size_t addr = up_progmem_getaddress(page);
   
   /* finally write the hunk */
   
   up_progmem_write(addr, buf, count);
   ```
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] davids5 commented on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

davids5 commented on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#issuecomment-596083672
 
 
   @bjbrandt - My apologies for it taking me so long to get to this. I ran it (with the change I suggested) on production units that requier this API to replace  bootloaders. It fails with `ERROR [bl_update] erase error at 0x8000080` hence the review request. I would reject this as it is and will raise the issue with @patacongo of revering the  29164c5 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-nuttx] bjbrandt edited a comment on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.

Posted by GitBox <gi...@apache.org>.

bjbrandt edited a comment on issue #391: arch/arm/src/stm32/stm32f20xxf40xx_flash.c: Add progmen flash block mapping.
URL: https://github.com/apache/incubator-nuttx/pull/391#issuecomment-592500639
 
 
   @davids5 
   > I think we need to step back and look at the API. It seems to have become very corrupted.
   Blocks, pages and Sectors have been muddled. The only thing that matters in the STM32 Soc is the Sector size and sizes.
   
   I think the use of terms like 'pages' and ' erase blocks' is just a try of generalization. Regarding the STM32 family an 'erase block' is just an alias for 'sector' while 'page' means the smaller r/w hunks (that do not have a name in the STM32 Reference manual). It is defined that an erase block is always a multiple of a single r/w page.
   
   
   > Consider the use case of a bootloader: How does one implement a simple Erase From Sector 2 to N (23 on the F4) and program from Sector 2 - n?
   > 
   > With the changes in [29164c5](https://github.com/apache/incubator-nuttx/commit/29164c5706490b74a84d44ba0f79cd588fd3e910) and herein what are the calls to do this? Can it even be done?
   
   If I got the API right, it should be done like this:
   
   ```
   /* erase sector 2 - N */
   
   for (int block = 1; block < N; ++i)
   {
      up_progmem_eraseblock(block);
   }
   
   /* get the first write page */
   
   size_t page = up_progmem_erasesize(0) / up_progmem_pagesize(0); 
   
   /* get the address off the desired page */
   
   size_t addr = up_progmem_getaddress(page);
   
   /* finally write the hunk */
   
   up_progmem_write(addr, buf, count);
   ```
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services