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 2021/05/01 18:32:07 UTC
[GitHub] [incubator-nuttx] saramonteiro opened a new pull request #3642: xtensa/esp32: several uart fixes
saramonteiro opened a new pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642
## Summary
This PR intends to do some refactoring and apply some workarounds to know issues from https://www.espressif.com/sites/default/files/documentation/eco_and_workarounds_for_bugs_in_esp32_en.pdf.
- Refactoring Work:
It was created the esp32_lowsetup that was being mentioned in comments but didn't exist.
Initial pin configuration was moved to esp32_lowsetup. This way, the fisrt step on serial
configuration is the pins one.
It was removed the end pin configuration. NuttX nor termios interface support remapping pins
in runtime, therefore, there's no need to detach signals from that pins or reconfigure it.
Actually, this approach can cause unpredictable behavior on these pins as has already been seen
in ESP32-C3 experiences.
- Fixing Work:
It was changed the way we access RX FIFO for reading and for writing. Now we use an AHB address for writing.
This and others workarounds are at https://www.espressif.com/sites/default/files/documentation/eco_and_workarounds_for_bugs_in_esp32_en.pdf.
It was changed the way we count RX FIFO bytes, since the standard way was not reliable.
It was added a reset step to reset both RX and TX FIFOs.
What was not solved:
Unfortunately, after all this reorganization and fixes, the garbage between bootloader and NuttX initialization is still there.
## Impact
This PR may impact ESP32 serial users or drivers that rely on serial driver.
## Testing
Nsh defconfig.
--
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
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642#discussion_r625983392
##########
File path: arch/risc-v/src/esp32c3/hardware/esp32c3_soc.h
##########
@@ -257,4 +257,12 @@
#define BIT(nr) (1UL << (nr))
+/* Extract the field from the register and shift it to avoid wrong reading */
+
+#define REG_MASK(_reg, _field) ((_reg & (_field##_M)) >> (_field##_S))
Review comment:
done
--
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
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642#discussion_r625983556
##########
File path: arch/risc-v/src/esp32c3/hardware/esp32c3_soc.h
##########
@@ -257,4 +257,12 @@
#define BIT(nr) (1UL << (nr))
+/* Extract the field from the register and shift it to avoid wrong reading */
+
+#define REG_MASK(_reg, _field) ((_reg & (_field##_M)) >> (_field##_S))
+
+/* Helper to place a value in a field */
+
+#define VALUE_TO_FIELD(_value, _field) ((_value << (_field##_S)) & (_field##_M))
Review comment:
done
##########
File path: arch/xtensa/src/esp32/hardware/esp32_soc.h
##########
@@ -183,6 +183,14 @@
#define GET_PERI_REG_BITS2(reg, mask,shift) ((READ_PERI_REG(reg)>>(shift))&(mask))
+/* Extract the field from the register and shift it to avoid wrong reading */
+
+#define REG_MASK(_reg, _field) ((_reg & (_field##_M)) >> (_field##_S))
Review comment:
done
##########
File path: arch/xtensa/src/esp32/hardware/esp32_soc.h
##########
@@ -183,6 +183,14 @@
#define GET_PERI_REG_BITS2(reg, mask,shift) ((READ_PERI_REG(reg)>>(shift))&(mask))
+/* Extract the field from the register and shift it to avoid wrong reading */
+
+#define REG_MASK(_reg, _field) ((_reg & (_field##_M)) >> (_field##_S))
+
+/* Helper to place a value in a field */
+
+#define VALUE_TO_FIELD(_value, _field) ((_value << (_field##_S)) & (_field##_M))
Review comment:
done
--
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642#discussion_r625875411
##########
File path: arch/xtensa/src/esp32/hardware/esp32_soc.h
##########
@@ -183,6 +183,14 @@
#define GET_PERI_REG_BITS2(reg, mask,shift) ((READ_PERI_REG(reg)>>(shift))&(mask))
+/* Extract the field from the register and shift it to avoid wrong reading */
+
+#define REG_MASK(_reg, _field) ((_reg & (_field##_M)) >> (_field##_S))
+
+/* Helper to place a value in a field */
+
+#define VALUE_TO_FIELD(_value, _field) ((_value << (_field##_S)) & (_field##_M))
Review comment:
```suggestion
#define VALUE_TO_FIELD(_value, _field) (((_value) << (_field##_S)) & (_field##_M))
```
Wrap macro argument in parentheses to avoid surprises with operator precedence
--
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642#discussion_r625863710
##########
File path: arch/risc-v/src/esp32c3/hardware/esp32c3_soc.h
##########
@@ -257,4 +257,12 @@
#define BIT(nr) (1UL << (nr))
+/* Extract the field from the register and shift it to avoid wrong reading */
+
+#define REG_MASK(_reg, _field) ((_reg & (_field##_M)) >> (_field##_S))
Review comment:
```suggestion
#define REG_MASK(_reg, _field) (((_reg) & (_field##_M)) >> (_field##_S))
```
Wrap macro argument in parentheses to avoid surprises with operator precedence
--
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642#discussion_r625873350
##########
File path: arch/xtensa/src/esp32/esp32_serial.c
##########
@@ -358,24 +362,112 @@ static uart_dev_t g_uart2port =
/****************************************************************************
* Private Functions
****************************************************************************/
+#ifndef CONFIG_SUPPRESS_UART_CONFIG
+/****************************************************************************
+ * Name: esp32_reset_rx_fifo
+ *
+ * Description:
+ * Resets the RX FIFO.
+ * NOTE: We can not use rxfifo_rst to reset the hardware RX FIFO.
+ *
+ * Parameters:
+ * priv - Pointer to the serial driver struct.
+ *
+ ****************************************************************************/
+
+static void esp32_reset_rx_fifo(struct esp32_dev_s *priv)
+{
+ uint32_t rx_status_reg;
+ uint32_t fifo_cnt;
+ uint32_t mem_rx_status_reg;
+ uint32_t rd_address;
+ uint32_t wr_address;
+ do
+ {
+ rx_status_reg = getreg32(UART_STATUS_REG(priv->config->id));
+ fifo_cnt = REG_MASK(rx_status_reg, UART_RXFIFO_CNT);
+ mem_rx_status_reg = getreg32(UART_MEM_RX_STATUS_REG(priv->config->id));
+ rd_address = REG_MASK(mem_rx_status_reg, UART_RD_ADDRESS);
+ wr_address = REG_MASK(mem_rx_status_reg, UART_WR_ADDRESS);
+
+ if ((fifo_cnt != 0) || (rd_address != wr_address))
+ {
+ getreg32(DR_UART_FIFO_REG(priv->config->id));
+ }
+ else
+ {
+ break;
+ }
+ }
+ while (true);
Review comment:
```suggestion
uint32_t rx_status_reg = getreg32(UART_STATUS_REG(priv->config->id));
uint32_t fifo_cnt = REG_MASK(rx_status_reg, UART_RXFIFO_CNT);
uint32_t mem_rx_status_reg = getreg32(UART_MEM_RX_STATUS_REG(priv->config->id));
uint32_t rd_address = REG_MASK(mem_rx_status_reg, UART_RD_ADDRESS);
uint32_t wr_address = REG_MASK(mem_rx_status_reg, UART_WR_ADDRESS);
while ((fifo_cnt != 0) || (rd_address != wr_address))
{
getreg32(DR_UART_FIFO_REG(priv->config->id));
rx_status_reg = getreg32(UART_STATUS_REG(priv->config->id));
fifo_cnt = REG_MASK(rx_status_reg, UART_RXFIFO_CNT);
mem_rx_status_reg = getreg32(UART_MEM_RX_STATUS_REG(priv->config->id));
rd_address = REG_MASK(mem_rx_status_reg, UART_RD_ADDRESS);
wr_address = REG_MASK(mem_rx_status_reg, UART_WR_ADDRESS);
}
```
This suggestion is functionally equivalent, but I believe it makes the while condition for termination more explicit.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642
--
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642#discussion_r625873350
##########
File path: arch/xtensa/src/esp32/esp32_serial.c
##########
@@ -358,24 +362,112 @@ static uart_dev_t g_uart2port =
/****************************************************************************
* Private Functions
****************************************************************************/
+#ifndef CONFIG_SUPPRESS_UART_CONFIG
+/****************************************************************************
+ * Name: esp32_reset_rx_fifo
+ *
+ * Description:
+ * Resets the RX FIFO.
+ * NOTE: We can not use rxfifo_rst to reset the hardware RX FIFO.
+ *
+ * Parameters:
+ * priv - Pointer to the serial driver struct.
+ *
+ ****************************************************************************/
+
+static void esp32_reset_rx_fifo(struct esp32_dev_s *priv)
+{
+ uint32_t rx_status_reg;
+ uint32_t fifo_cnt;
+ uint32_t mem_rx_status_reg;
+ uint32_t rd_address;
+ uint32_t wr_address;
+ do
+ {
+ rx_status_reg = getreg32(UART_STATUS_REG(priv->config->id));
+ fifo_cnt = REG_MASK(rx_status_reg, UART_RXFIFO_CNT);
+ mem_rx_status_reg = getreg32(UART_MEM_RX_STATUS_REG(priv->config->id));
+ rd_address = REG_MASK(mem_rx_status_reg, UART_RD_ADDRESS);
+ wr_address = REG_MASK(mem_rx_status_reg, UART_WR_ADDRESS);
+
+ if ((fifo_cnt != 0) || (rd_address != wr_address))
+ {
+ getreg32(DR_UART_FIFO_REG(priv->config->id));
+ }
+ else
+ {
+ break;
+ }
+ }
+ while (true);
Review comment:
```suggestion
uint32_t rx_status_reg = getreg32(UART_STATUS_REG(priv->config->id));
uint32_t fifo_cnt = REG_MASK(rx_status_reg, UART_RXFIFO_CNT);
uint32_t mem_rx_status_reg = getreg32(UART_MEM_RX_STATUS_REG(priv->config->id));
uint32_t rd_address = REG_MASK(mem_rx_status_reg, UART_RD_ADDRESS);
uint32_t wr_address = REG_MASK(mem_rx_status_reg, UART_WR_ADDRESS);
while ((fifo_cnt != 0) || (rd_address != wr_address))
{
getreg32(DR_UART_FIFO_REG(priv->config->id));
rx_status_reg = getreg32(UART_STATUS_REG(priv->config->id));
fifo_cnt = REG_MASK(rx_status_reg, UART_RXFIFO_CNT);
mem_rx_status_reg = getreg32(UART_MEM_RX_STATUS_REG(priv->config->id));
rd_address = REG_MASK(mem_rx_status_reg, UART_RD_ADDRESS);
wr_address = REG_MASK(mem_rx_status_reg, UART_WR_ADDRESS);
}
```
nit: This suggestion is functionally equivalent, but I believe it makes the while condition for termination more explicit.
--
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642#discussion_r625875267
##########
File path: arch/xtensa/src/esp32/hardware/esp32_soc.h
##########
@@ -183,6 +183,14 @@
#define GET_PERI_REG_BITS2(reg, mask,shift) ((READ_PERI_REG(reg)>>(shift))&(mask))
+/* Extract the field from the register and shift it to avoid wrong reading */
+
+#define REG_MASK(_reg, _field) ((_reg & (_field##_M)) >> (_field##_S))
Review comment:
```suggestion
#define REG_MASK(_reg, _field) (((_reg) & (_field##_M)) >> (_field##_S))
```
Wrap macro argument in parentheses to avoid surprises with operator precedence
--
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3642: xtensa/esp32: several uart fixes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3642:
URL: https://github.com/apache/incubator-nuttx/pull/3642#discussion_r625863888
##########
File path: arch/risc-v/src/esp32c3/hardware/esp32c3_soc.h
##########
@@ -257,4 +257,12 @@
#define BIT(nr) (1UL << (nr))
+/* Extract the field from the register and shift it to avoid wrong reading */
+
+#define REG_MASK(_reg, _field) ((_reg & (_field##_M)) >> (_field##_S))
+
+/* Helper to place a value in a field */
+
+#define VALUE_TO_FIELD(_value, _field) ((_value << (_field##_S)) & (_field##_M))
Review comment:
```suggestion
#define VALUE_TO_FIELD(_value, _field) (((_value) << (_field##_S)) & (_field##_M))
```
Wrap macro argument in parentheses to avoid surprises with operator precedence
--
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