You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@mynewt.apache.org by "m-gorecki (via GitHub)" <gi...@apache.org> on 2025/03/03 08:59:03 UTC
[PR] [WIP] STM32 Ethernet driver migration to new version [mynewt-core]
m-gorecki opened a new pull request, #3380:
URL: https://github.com/apache/mynewt-core/pull/3380
(no comment)
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STM32 Ethernet driver migration to new version [mynewt-core]
Posted by "m-gorecki (via GitHub)" <gi...@apache.org>.
m-gorecki merged PR #3380:
URL: https://github.com/apache/mynewt-core/pull/3380
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STM32 Ethernet driver migration to new version [mynewt-core]
Posted by "kasjer (via GitHub)" <gi...@apache.org>.
kasjer commented on code in PR #3380:
URL: https://github.com/apache/mynewt-core/pull/3380#discussion_r1991468530
##########
hw/drivers/lwip/stm32_eth/src/stm32_eth.c:
##########
@@ -59,129 +74,108 @@
#define SMSC_8710_ISR_AUTO_DONE 0x40
#define SMSC_8710_ISR_LINK_DOWN 0x10
-#define STM32_ETH_RX_DESC_SZ 3
-#define STM32_ETH_TX_DESC_SZ 4
-
-struct stm32_eth_desc {
- volatile ETH_DMADescTypeDef desc;
- struct pbuf *p;
+struct rx_buff {
+ struct pbuf_custom pbuf_custom;
+ uint8_t buff[(ETH_RX_BUF_SIZE + 31) & ~31];
};
struct stm32_eth_state {
struct netif st_nif;
ETH_HandleTypeDef st_eth;
- struct stm32_eth_desc st_rx_descs[STM32_ETH_RX_DESC_SZ];
- struct stm32_eth_desc st_tx_descs[STM32_ETH_TX_DESC_SZ];
- uint8_t st_rx_head;
- uint8_t st_rx_tail;
- uint8_t st_tx_head;
- uint8_t st_tx_tail;
+ ETH_DMADescTypeDef st_rx_descs[ETH_RX_DESC_CNT];
+ ETH_DMADescTypeDef st_tx_descs[ETH_TX_DESC_CNT];
+ ETH_TxPacketConfig st_tx_cfg;
+ ETH_MACConfigTypeDef st_mac_cfg;
struct hal_timer st_phy_tmr;
const struct stm32_eth_cfg *cfg;
};
-struct stm32_eth_stats {
- uint32_t oframe;
- uint32_t odone;
- uint32_t oerr;
- uint32_t iframe;
- uint32_t imem;
-} stm32_eth_stats;
-
static struct stm32_eth_state stm32_eth_state;
+static uint8_t rx_alloc_failed;
Review Comment:
maybe `bool` ?
##########
hw/drivers/lwip/stm32_eth/src/stm32_eth.c:
##########
@@ -215,99 +209,42 @@ stm32_mld_mac_filter(struct netif *nif, const ip6_addr_t *group,
}
#endif
-static void
-stm32_eth_output_done(struct stm32_eth_state *ses)
-{
- struct stm32_eth_desc *sed;
-
- while (1) {
- sed = &ses->st_tx_descs[ses->st_tx_tail];
- if (!sed->p) {
- break;
- }
- if (sed->desc.Status & ETH_DMATXDESC_OWN) {
- /*
- * Belongs to board
- */
- break;
- }
- if (sed->desc.Status & ETH_DMATXDESC_ES) {
- ++stm32_eth_stats.oerr;
- } else {
- ++stm32_eth_stats.odone;
- }
- pbuf_free(sed->p);
- sed->p = NULL;
- ses->st_tx_tail++;
- if (ses->st_tx_tail >= STM32_ETH_TX_DESC_SZ) {
- ses->st_tx_tail = 0;
- }
- }
-}
-
static err_t
stm32_eth_output(struct netif *nif, struct pbuf *p)
{
- struct stm32_eth_state *ses = (struct stm32_eth_state *)nif;
- struct stm32_eth_desc *sed;
- uint32_t reg;
- struct pbuf *q;
- err_t errval;
+ int i = 0;
+ struct pbuf *q = NULL;
Review Comment:
initialization not really needed here
##########
hw/drivers/lwip/stm32_eth/src/stm32_eth.c:
##########
@@ -59,129 +74,108 @@
#define SMSC_8710_ISR_AUTO_DONE 0x40
#define SMSC_8710_ISR_LINK_DOWN 0x10
-#define STM32_ETH_RX_DESC_SZ 3
-#define STM32_ETH_TX_DESC_SZ 4
-
-struct stm32_eth_desc {
- volatile ETH_DMADescTypeDef desc;
- struct pbuf *p;
+struct rx_buff {
+ struct pbuf_custom pbuf_custom;
+ uint8_t buff[(ETH_RX_BUF_SIZE + 31) & ~31];
};
struct stm32_eth_state {
struct netif st_nif;
ETH_HandleTypeDef st_eth;
- struct stm32_eth_desc st_rx_descs[STM32_ETH_RX_DESC_SZ];
- struct stm32_eth_desc st_tx_descs[STM32_ETH_TX_DESC_SZ];
- uint8_t st_rx_head;
- uint8_t st_rx_tail;
- uint8_t st_tx_head;
- uint8_t st_tx_tail;
+ ETH_DMADescTypeDef st_rx_descs[ETH_RX_DESC_CNT];
+ ETH_DMADescTypeDef st_tx_descs[ETH_TX_DESC_CNT];
+ ETH_TxPacketConfig st_tx_cfg;
+ ETH_MACConfigTypeDef st_mac_cfg;
struct hal_timer st_phy_tmr;
const struct stm32_eth_cfg *cfg;
};
-struct stm32_eth_stats {
- uint32_t oframe;
- uint32_t odone;
- uint32_t oerr;
- uint32_t iframe;
- uint32_t imem;
-} stm32_eth_stats;
-
static struct stm32_eth_state stm32_eth_state;
+static uint8_t rx_alloc_failed;
+
+LWIP_MEMPOOL_DECLARE(RX_POOL, ETH_RX_BUFFER_CNT, sizeof(struct rx_buff), "Zero-copy RX PBUF pool");
static void
-stm32_eth_setup_descs(struct stm32_eth_desc *descs, int cnt)
+stm32_eth_input(struct stm32_eth_state *ses)
{
- int i;
+ struct pbuf *p = NULL;
- for (i = 0; i < cnt - 1; i++) {
- descs[i].desc.Status = 0;
- descs[i].desc.Buffer2NextDescAddr = (uint32_t)&descs[i + 1].desc;
+ if (rx_alloc_failed) {
+ return;
}
- descs[cnt - 1].desc.Status = 0;
- descs[cnt - 1].desc.Buffer2NextDescAddr = (uint32_t)&descs[0].desc;
-}
-
-static void
-stm32_eth_fill_rx(struct stm32_eth_state *ses)
-{
- struct stm32_eth_desc *sed;
- struct pbuf *p;
- while (1) {
- sed = &ses->st_rx_descs[ses->st_rx_tail];
- if (sed->p) {
- break;
- }
- p = pbuf_alloc(PBUF_RAW, ETH_MAX_PACKET_SIZE, PBUF_POOL);
- if (!p) {
- ++stm32_eth_stats.imem;
- break;
- }
- sed->p = p;
- sed->desc.Status = 0;
- sed->desc.ControlBufferSize = ETH_DMARXDESC_RCH | ETH_MAX_PACKET_SIZE;
- sed->desc.Buffer1Addr = (uint32_t)p->payload;
- sed->desc.Status = ETH_DMARXDESC_OWN;
-
- ses->st_rx_tail++;
- if (ses->st_rx_tail >= STM32_ETH_RX_DESC_SZ) {
- ses->st_rx_tail = 0;
+ HAL_ETH_ReadData(&ses->st_eth, (void **)&p);
+ if (p != NULL) {
+ if (ses->st_nif.input(p, &ses->st_nif) != ERR_OK) {
+ pbuf_free(p);
}
}
}
-static void
-stm32_eth_input(struct stm32_eth_state *ses)
+void
+HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *heth)
{
- struct stm32_eth_desc *sed;
- struct pbuf *p;
- struct netif *nif;
+ struct stm32_eth_state *ses = &stm32_eth_state;
- nif = &ses->st_nif;
+ stm32_eth_input(ses);
+}
- while (1) {
- sed = &ses->st_rx_descs[ses->st_rx_head];
- if (!sed->p) {
- break;
- }
- if (sed->desc.Status & ETH_DMARXDESC_OWN) {
- break;
- }
- p = sed->p;
- sed->p = NULL;
- if (!(sed->desc.Status & ETH_DMARXDESC_LS)) {
- /*
- * Incoming data spans multiple pbufs. XXX support later
- */
- pbuf_free(p);
- continue;
- }
- p->len = p->tot_len = (sed->desc.Status & ETH_DMARXDESC_FL) >> 16;
- ++stm32_eth_stats.iframe;
- nif->input(p, nif);
- ses->st_rx_head++;
- if (ses->st_rx_head >= STM32_ETH_RX_DESC_SZ) {
- ses->st_rx_head = 0;
- }
+void
+pbuf_free_custom(struct pbuf *p)
+{
+ struct pbuf_custom * custom_pbuf = (struct pbuf_custom *) p;
+ struct stm32_eth_state *ses = &stm32_eth_state;
+ LWIP_MEMPOOL_FREE(RX_POOL, custom_pbuf);
+
+ if (rx_alloc_failed) {
+ rx_alloc_failed = 0;
+ stm32_eth_input(ses);
}
+}
- stm32_eth_fill_rx(ses);
+void
+HAL_ETH_RxAllocateCallback(uint8_t **buff)
+{
+ struct pbuf_custom *p;
- if (ses->st_eth.Instance->DMASR & ETH_DMASR_RBUS) {
- /*
- * Resume DMA reception
- */
- ses->st_eth.Instance->DMASR = ETH_DMASR_RBUS;
- ses->st_eth.Instance->DMARPDR = 0;
+ p = LWIP_MEMPOOL_ALLOC(RX_POOL);
+ if (p != NULL) {
+ *buff = (uint8_t *)p + offsetof(struct rx_buff, buff);
Review Comment:
This line could be less confusing to me like that:
```c
*buff = ((struct rx_buff *)p)->buff;
```
On the first glance it's not obvious that `rx_buff` encapsulates `pbuf_custom`
Especially since function argument name is `buff` and in confusing line `buff` has two different meanings
##########
hw/drivers/lwip/stm32_eth/src/stm32_eth.c:
##########
@@ -49,6 +63,7 @@
* This shows up as MMCRFCECR going up, and no valid RX happening.
*/
#define STM32_PHY_POLL_FREQ os_cputime_usecs_to_ticks(1500000) /* 1.5 secs */
+#define ETH_RX_BUFFER_CNT 10
Review Comment:
This could be configurable in syscfg.
I don't know if it was previously like that.
##########
hw/drivers/lwip/stm32_eth/src/stm32_eth.c:
##########
@@ -59,129 +74,108 @@
#define SMSC_8710_ISR_AUTO_DONE 0x40
#define SMSC_8710_ISR_LINK_DOWN 0x10
-#define STM32_ETH_RX_DESC_SZ 3
-#define STM32_ETH_TX_DESC_SZ 4
-
-struct stm32_eth_desc {
- volatile ETH_DMADescTypeDef desc;
- struct pbuf *p;
+struct rx_buff {
+ struct pbuf_custom pbuf_custom;
+ uint8_t buff[(ETH_RX_BUF_SIZE + 31) & ~31];
};
struct stm32_eth_state {
struct netif st_nif;
ETH_HandleTypeDef st_eth;
- struct stm32_eth_desc st_rx_descs[STM32_ETH_RX_DESC_SZ];
- struct stm32_eth_desc st_tx_descs[STM32_ETH_TX_DESC_SZ];
- uint8_t st_rx_head;
- uint8_t st_rx_tail;
- uint8_t st_tx_head;
- uint8_t st_tx_tail;
+ ETH_DMADescTypeDef st_rx_descs[ETH_RX_DESC_CNT];
+ ETH_DMADescTypeDef st_tx_descs[ETH_TX_DESC_CNT];
+ ETH_TxPacketConfig st_tx_cfg;
+ ETH_MACConfigTypeDef st_mac_cfg;
struct hal_timer st_phy_tmr;
const struct stm32_eth_cfg *cfg;
};
-struct stm32_eth_stats {
- uint32_t oframe;
- uint32_t odone;
- uint32_t oerr;
- uint32_t iframe;
- uint32_t imem;
-} stm32_eth_stats;
-
static struct stm32_eth_state stm32_eth_state;
+static uint8_t rx_alloc_failed;
+
+LWIP_MEMPOOL_DECLARE(RX_POOL, ETH_RX_BUFFER_CNT, sizeof(struct rx_buff), "Zero-copy RX PBUF pool");
static void
-stm32_eth_setup_descs(struct stm32_eth_desc *descs, int cnt)
+stm32_eth_input(struct stm32_eth_state *ses)
{
- int i;
+ struct pbuf *p = NULL;
- for (i = 0; i < cnt - 1; i++) {
- descs[i].desc.Status = 0;
- descs[i].desc.Buffer2NextDescAddr = (uint32_t)&descs[i + 1].desc;
+ if (rx_alloc_failed) {
+ return;
}
- descs[cnt - 1].desc.Status = 0;
- descs[cnt - 1].desc.Buffer2NextDescAddr = (uint32_t)&descs[0].desc;
-}
-
-static void
-stm32_eth_fill_rx(struct stm32_eth_state *ses)
-{
- struct stm32_eth_desc *sed;
- struct pbuf *p;
- while (1) {
- sed = &ses->st_rx_descs[ses->st_rx_tail];
- if (sed->p) {
- break;
- }
- p = pbuf_alloc(PBUF_RAW, ETH_MAX_PACKET_SIZE, PBUF_POOL);
- if (!p) {
- ++stm32_eth_stats.imem;
- break;
- }
- sed->p = p;
- sed->desc.Status = 0;
- sed->desc.ControlBufferSize = ETH_DMARXDESC_RCH | ETH_MAX_PACKET_SIZE;
- sed->desc.Buffer1Addr = (uint32_t)p->payload;
- sed->desc.Status = ETH_DMARXDESC_OWN;
-
- ses->st_rx_tail++;
- if (ses->st_rx_tail >= STM32_ETH_RX_DESC_SZ) {
- ses->st_rx_tail = 0;
+ HAL_ETH_ReadData(&ses->st_eth, (void **)&p);
+ if (p != NULL) {
+ if (ses->st_nif.input(p, &ses->st_nif) != ERR_OK) {
+ pbuf_free(p);
}
}
}
-static void
-stm32_eth_input(struct stm32_eth_state *ses)
+void
+HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *heth)
{
- struct stm32_eth_desc *sed;
- struct pbuf *p;
- struct netif *nif;
+ struct stm32_eth_state *ses = &stm32_eth_state;
- nif = &ses->st_nif;
+ stm32_eth_input(ses);
+}
- while (1) {
- sed = &ses->st_rx_descs[ses->st_rx_head];
- if (!sed->p) {
- break;
- }
- if (sed->desc.Status & ETH_DMARXDESC_OWN) {
- break;
- }
- p = sed->p;
- sed->p = NULL;
- if (!(sed->desc.Status & ETH_DMARXDESC_LS)) {
- /*
- * Incoming data spans multiple pbufs. XXX support later
- */
- pbuf_free(p);
- continue;
- }
- p->len = p->tot_len = (sed->desc.Status & ETH_DMARXDESC_FL) >> 16;
- ++stm32_eth_stats.iframe;
- nif->input(p, nif);
- ses->st_rx_head++;
- if (ses->st_rx_head >= STM32_ETH_RX_DESC_SZ) {
- ses->st_rx_head = 0;
- }
+void
+pbuf_free_custom(struct pbuf *p)
+{
+ struct pbuf_custom * custom_pbuf = (struct pbuf_custom *) p;
Review Comment:
two extra spaces
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org