You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2022/09/26 23:22:38 UTC
[GitHub] [mynewt-core] agross-korg opened a new pull request, #2884: Flash autoconfigure updates
agross-korg opened a new pull request, #2884:
URL: https://github.com/apache/mynewt-core/pull/2884
This set of patches fixes an issue with Gigadevice flash when issuing rdid reads. The second patch adds an export of the RDID information so that it can be used by other application code. This is especially useful if special actions need to be taken based on the detected flash identification.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] andrzej-kaczmarek commented on pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#issuecomment-1276831554
@agross-korg why this was merged with unresolved comments?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] agross-korg commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
agross-korg commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r994028463
##########
hw/mcu/dialog/da1469x/src/hal_flash.c:
##########
@@ -430,6 +430,9 @@ qspi_read_rdid(const struct hal_flash *dev)
int i;
uint32_t result;
+ da1469x_qspi_mode_manual(dev);
+ da1469x_qspi_wait_busy(dev);
Review Comment:
I disagree on this. I think you need to assure a function can do what it needs to do without requiring something being done outside as a prerequisite.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] agross-korg commented on pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
agross-korg commented on PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#issuecomment-1276912080
I thought they were all resolved with the push I did. Let me double check that the changes hit all of the points. I thought they had.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] andrzej-kaczmarek commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r981614827
##########
hw/mcu/dialog/da1469x/src/hal_flash.c:
##########
@@ -430,6 +430,9 @@ qspi_read_rdid(const struct hal_flash *dev)
int i;
uint32_t result;
+ da1469x_qspi_mode_manual(dev);
+ da1469x_qspi_wait_busy(dev);
Review Comment:
`qspi_read_rdid` is static here so not sure how you call it in other places, but if you somehow do then fixed version has rather unexpected side-effect of switching qspi controller to manual mode. in `hal_flash` it's used as a helper and caller takes care of switching qspi between manual and auto mode.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] agross-korg commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
agross-korg commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r981492275
##########
hw/mcu/dialog/da1469x/src/hal_flash.c:
##########
@@ -430,6 +430,9 @@ qspi_read_rdid(const struct hal_flash *dev)
int i;
uint32_t result;
+ da1469x_qspi_mode_manual(dev);
+ da1469x_qspi_wait_busy(dev);
Review Comment:
The main reason is that we had been using the read_rdid call directly in other places and I found that it made more sense to make function calls independent from people who call them, especially if you are issuing commands which may leave the chip in an odd state.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] andrzej-kaczmarek commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r980894944
##########
hw/mcu/dialog/da1469x/include/mcu/da1469x_hal.h:
##########
@@ -85,6 +85,8 @@ struct qspi_flash_config {
extern const struct qspi_flash_config rdids[];
extern const int qspi_flash_config_array_size;
+extern const struct qspi_flash_config *rdid_detected;
Review Comment:
I'd prefer to have getter for this so apps don't rely on variable name
##########
hw/mcu/dialog/da1469x/src/hal_flash.c:
##########
@@ -430,6 +430,9 @@ qspi_read_rdid(const struct hal_flash *dev)
int i;
uint32_t result;
+ da1469x_qspi_mode_manual(dev);
+ da1469x_qspi_wait_busy(dev);
Review Comment:
why not just add wait_busy to init? this way auto mode is disabled and then re-enabled by the same function
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] andrzej-kaczmarek commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r981663328
##########
hw/mcu/dialog/da1469x/src/hal_flash.c:
##########
@@ -430,6 +430,9 @@ qspi_read_rdid(const struct hal_flash *dev)
int i;
uint32_t result;
+ da1469x_qspi_mode_manual(dev);
+ da1469x_qspi_wait_busy(dev);
Review Comment:
`qspi_read_rdid` is just a helper, it does not need to handle controller mode - this is done by caller which is `da1469x_hff_mcu_custom_init` and does everything just as two other mentioned functions so basically it makes sense to also add wait busy to init func.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] agross-korg merged pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
agross-korg merged PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#issuecomment-1258777883
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### hw/mcu/dialog/da1469x/src/hal_flash.c
<details>
```diff
@@ -31,7 +31,7 @@
union da1469x_qspi_data_reg {
uint32_t d32;
uint16_t d16;
- uint8_t d8;
+ uint8_t d8;
};
const struct qspi_flash_config *rdid_detected = NULL;
```
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] agross-korg commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
agross-korg commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r981494416
##########
hw/mcu/dialog/da1469x/include/mcu/da1469x_hal.h:
##########
@@ -85,6 +85,8 @@ struct qspi_flash_config {
extern const struct qspi_flash_config rdids[];
extern const int qspi_flash_config_array_size;
+extern const struct qspi_flash_config *rdid_detected;
Review Comment:
That's reasonable. I can add a function like:
const struct *qspi_flash_get_config(void)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] agross-korg commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
agross-korg commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r981619657
##########
hw/mcu/dialog/da1469x/src/hal_flash.c:
##########
@@ -430,6 +430,9 @@ qspi_read_rdid(const struct hal_flash *dev)
int i;
uint32_t result;
+ da1469x_qspi_mode_manual(dev);
+ da1469x_qspi_wait_busy(dev);
Review Comment:
It was now, but I found this issue when I was experimenting with having access to the rdid function. Personally, I'd rather functions that do something specific do all of the things that need to be done to be successful. There are other functions where we do this set manual mode, wait for busy. This makes this consistent with those other functions. Examples: da1469x_qspi_write and da1469x_qspi_erase_sector
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#issuecomment-1259954684
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### hw/mcu/dialog/da1469x/src/hal_flash.c
<details>
```diff
@@ -63,7 +63,8 @@
.hf_erased_val = 0xff,
};
-const struct qspi_flash_config *da1469x_qspi_get_config(void)
+const struct qspi_flash_config *
+da1469x_qspi_get_config(void)
{
return rdid_detected;
}
```
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] andrzej-kaczmarek commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r981663328
##########
hw/mcu/dialog/da1469x/src/hal_flash.c:
##########
@@ -430,6 +430,9 @@ qspi_read_rdid(const struct hal_flash *dev)
int i;
uint32_t result;
+ da1469x_qspi_mode_manual(dev);
+ da1469x_qspi_wait_busy(dev);
Review Comment:
`qspi_read_rdid` is just a helper, it does not need to handle controller mode - this is done by caller which is `da1469x_hff_mcu_custom_init` and does everything just as two other mentioned functions so basically it makes sense to also add wait busy to init func. it's just like a critical section - you don't expect helpers to enter critical section, they can just assume they are called inside a critical section.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#issuecomment-1259972404
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] agross-korg commented on a diff in pull request #2884: Flash autoconfigure updates
Posted by GitBox <gi...@apache.org>.
agross-korg commented on code in PR #2884:
URL: https://github.com/apache/mynewt-core/pull/2884#discussion_r994027924
##########
hw/mcu/dialog/da1469x/include/mcu/da1469x_hal.h:
##########
@@ -85,6 +85,8 @@ struct qspi_flash_config {
extern const struct qspi_flash_config rdids[];
extern const int qspi_flash_config_array_size;
+extern const struct qspi_flash_config *rdid_detected;
Review Comment:
I added the accessor function in the 5cfcdee commit below.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org