[DEV] STM32H7xx support (new)

[mikeprotts]

No functional changes, just tidied up and unused code removed. Files attached. I'm happy to repackage as a patch if preferred.

I've not included the changes to the board file as I've not worked out how to use the config generator yet. The only changes are the board voltage (not needed for the MACv2 work) and a few pins changed to FLOATING which I think is the correct

Code: Select all

diff ./changed/ChibiOS_20.3.1/os/hal/boards/ST_NUCLEO144_H743ZI/board.h ./original/ChibiOS_20.3.1/os/hal/boards/ST_NUCLEO144_H743ZI/board.h
65,70d64
<  /*
<  * Board voltages.
<  * Required for performance limits calculation.
<  */
< #define STM32_VDD                   330U
417c411
<                                      PIN_PUPDR_FLOATING(GPIOA_RMII_MDIO) |    \
---
>                                      PIN_PUPDR_PULLUP(GPIOA_RMII_MDIO) |    \
422c416
<                                      PIN_PUPDR_FLOATING(GPIOA_RMII_CRS_DV) |  \
---
>                                      PIN_PUPDR_PULLUP(GPIOA_RMII_CRS_DV) |  \
545c539
<                                      PIN_PUPDR_FLOATING(GPIOB_RMII_TXD1) |    \
---
>                                      PIN_PUPDR_PULLUP(GPIOB_RMII_TXD1) |    \

Mike

[Giovanni]

Note that changes to board files have to be done in board.chcfg then generate the other files. I cannot commit just a modified board.h, it would be overwritten on next mass update.

Giovanni

[mikeprotts]

Note that changes to board files have to be done in board.chcfg then generate the other files. I cannot commit just a modified board.h, it would be overwritten on next mass update.

Giovanni

That's what I thought, and why I haven't included the board file in the archive. I've made the changes to the board.chcfg file:

Code: Select all

diff -c --exclude build --exclude .dep -rwb ./original/ChibiOS_20.3.1/os/hal/boards/ST_NUCLEO144_H743ZI/cfg/board.chcfg ./changed/ChibiOS_20.3.1/o
s/hal/boards/ST_NUCLEO144_H743ZI/cfg/board.chcfg
*** ./original/ChibiOS_20.3.1/os/hal/boards/ST_NUCLEO144_H743ZI/cfg/board.chcfg 2020-01-30 17:50:59.000000000 +0000
--- ./changed/ChibiOS_20.3.1/os/hal/boards/ST_NUCLEO144_H743ZI/cfg/board.chcfg  2020-07-21 12:03:18.045089019 +0100
***************
*** 22,28 ****
      HSEBypass="true"
      LSEFrequency="32768"
      LSEBypass="false"
!     VDD="300"
      LSEDrive="3 High Drive (default)" />
    <ports>
      <GPIOA>
--- 22,28 ----
      HSEBypass="true"
      LSEFrequency="32768"
      LSEBypass="false"
!     VDD="330"
      LSEDrive="3 High Drive (default)" />
    <ports>
      <GPIOA>
***************
*** 47,53 ****
          Type="PushPull"
          Level="High"
          Speed="Maximum"
!         Resistor="PullUp"
          Mode="Alternate"
          Alternate="11" />
        <pin3
--- 47,53 ----
          Type="PushPull"
          Level="High"
          Speed="Maximum"
!         Resistor="Floating"
          Mode="Alternate"
          Alternate="11" />
        <pin3
***************
*** 87,93 ****
          Type="PushPull"
          Level="High"
          Speed="Maximum"
!         Resistor="PullUp"
          Mode="Alternate"
          Alternate="11" />
        <pin8
--- 87,93 ----
          Type="PushPull"
          Level="High"
          Speed="Maximum"
!         Resistor="Floating"
          Mode="Alternate"
          Alternate="11" />
        <pin8
***************
*** 265,271 ****
          Type="PushPull"
          Level="High"
          Speed="Maximum"
!         Resistor="PullUp"
          Mode="Alternate"
          Alternate="11" />
        <pin14
--- 265,271 ----
          Type="PushPull"
          Level="High"
          Speed="Maximum"
!         Resistor="Floating"
          Mode="Alternate"
          Alternate="11" />
        <pin14

Mike

[geebee]

I think there is an issue with stalls/restarts, from compiler warnings. In hal_mac_lld, I believe you might want to do either

Code: Select all

--- a/os/hal/ports/STM32/LLD/MACv2/hal_mac_lld.c
+++ b/os/hal/ports/STM32/LLD/MACv2/hal_mac_lld.c
@@ -517,7 +517,7 @@ void mac_lld_release_transmit_descriptor(MACTransmitDescriptor *tdp) {
   __DSB();
 
   /* If the DMA engine is stalled then a restart request is issued.*/
-  if ((ETH->DMACSR & ETH_DMACSR_TPS) == ETH_DMADSR_TPS_SUSPENDED) {
+  if (ETH->DMACSR & ETH_DMACSR_TPS) {^M
     ETH->DMACSR   = ETH_DMACSR_TBU;
   }
   ETH->DMACTDTPR = 0;
@@ -594,7 +594,7 @@ void mac_lld_release_receive_descriptor(MACReceiveDescriptor *rdp) {
   __DSB();
 
   /* If the DMA engine is stalled then a restart request is issued.*/
-  if ((ETH->DMACSR & ETH_DMACSR_RPS) == ETH_DMADSR_RPS_SUSPENDED) {
+  if (ETH->DMACSR & ETH_DMACSR_RPS) {^M
     ETH->DMACSR   = ETH_DMACSR_RBU;
   }
   ETH->DMACRDTPR = 0;

or

Code: Select all

--- a/os/hal/ports/STM32/LLD/MACv2/hal_mac_lld.c
+++ b/os/hal/ports/STM32/LLD/MACv2/hal_mac_lld.c
@@ -517,7 +517,7 @@ void mac_lld_release_transmit_descriptor(MACTransmitDescriptor *tdp) {
   __DSB();
 
   /* If the DMA engine is stalled then a restart request is issued.*/
-  if ((ETH->DMACSR & ETH_DMACSR_TPS) == ETH_DMADSR_TPS_SUSPENDED) {
+  if ((ETH->DMADSR & ETH_DMADSR_TPS) == ETH_DMADSR_TPS_SUSPENDED) {^M
     ETH->DMACSR   = ETH_DMACSR_TBU;
   }
   ETH->DMACTDTPR = 0;
@@ -594,7 +594,7 @@ void mac_lld_release_receive_descriptor(MACReceiveDescriptor *rdp) {
   __DSB();
 
   /* If the DMA engine is stalled then a restart request is issued.*/
-  if ((ETH->DMACSR & ETH_DMACSR_RPS) == ETH_DMADSR_RPS_SUSPENDED) {
+  if ((ETH->DMADSR & ETH_DMADSR_RPS) == ETH_DMADSR_RPS_SUSPENDED) {^M
     ETH->DMACSR   = ETH_DMACSR_RBU;
   }
   ETH->DMACRDTPR = 0;

As it is, you are maksing DMACSR (single bit status) but comparing with the status constant for the debug register DMADSR (multiple values, at a different location), which is always false.

Aside from the compiler warnings I have not noticed any actual issue with it, so I don't think I ever put the MAC in a state to need that code. So far everything has worked just fine with your patch.

GB

[mikeprotts]

A good spot. I think it's the second option based on comparing with MACv1 code and the documentation.

STM32F76xx & STM32F77xx docs:

Ethernet DMA status register (ETH_DMASR)
Bits 22:20 TPS: Transmit process state
These bits indicate the Transmit DMA FSM state. This field does not generate an interrupt.
000: Stopped; Reset or Stop Transmit Command issued
001: Running; Fetching transmit transfer descriptor
010: Running; Waiting for status
011: Running; Reading Data from host memory buffer and queuing it to transmit buffer (TxFIFO)
100, 101: Reserved for future use
110: Suspended; Transmit descriptor unavailable or transmit buffer underflow <<<<<<<<<<<<<<<<<<<<<<<<<
111: Running; Closing transmit descriptor

STM32H742, STM32H743/753 & STM32H750 docs:

Debug status register (ETH_DMADSR)
TPS0[3:0]: DMA Channel Transmit Process State
This field indicates the Tx DMA FSM state for Channel:
000: Stopped (Reset or Stop Transmit Command issued)
001: Running (Fetching Tx Transfer Descriptor)
010: Running (Waiting for status)
011: Running (Reading Data from system memory buffer and queuing it to the Tx buffer (TxFIFO))
100: Timestamp write state
101: Reserved for future use
110: Suspended (Tx Descriptor Unavailable or Tx Buffer Underflow) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<
111: Running (Closing Tx Descriptor)
The MSB of this field always returns 0. This field does not generate an interrupt.

I don't think I've ever triggered this code, but I'll test with a breakpoint to see if it ever occurs.

Mike

[mikeprotts]

Putting in breakpoints tells me the code is being run for both transmit & receive. I'm now running a longer test to check there are no other consequences,

Mike

[twarge]

I'm pleased to report that this driver has been working continuously for a month and I think it's ready for inclusion!

Giovanni, can you please consider adding it for the next release?

[geebee]

I can confirm that it's looking really solid, and support merging.

I found another issue with checking the DMA status, this time it looks like a problem with the headers by ST. Specifically this line triggers a warning:

Code: Select all

if ((ETH->DMADSR & ETH_DMADSR_RPS) == ETH_DMADSR_RPS_SUSPENDED) {

Because of an issue with the definitions of all the ETH_DMADSR_RPS_xxxx constants. In this specific case

Code: Select all

#define ETH_DMADSR_RPS_Pos                            (8U)
#define ETH_DMADSR_RPS_Msk                            (0xFUL << ETH_DMADSR_RPS_Pos) /*!< 0x00000F00 */
#define ETH_DMADSR_RPS                                ETH_DMADSR_RPS_Msk       /* DMA Channel Receive Process State */
//...
#define ETH_DMADSR_RPS_SUSPENDED_Pos                  (14U)
#define ETH_DMADSR_RPS_SUSPENDED_Msk                  (0x1UL << ETH_DMADSR_RPS_SUSPENDED_Pos) /*!< 0x00004000 */
#define ETH_DMADSR_RPS_SUSPENDED                      ETH_DMADSR_RPS_SUSPENDED_Msk /* Suspended (Rx Descriptor Unavailable) */

RPS is bits 8-11 (as it should be), but ETH_DMADSR_RPS_SUSPENDED is incorrect (Pos should be 10 I think). This seems to still be the case in the current CubeH7 header.

GB

[mikeprotts]

Docs:

Code: Select all

Bits 11:8 RPS0[3:0]: DMA Channel Receive Process State
This field indicates the Rx DMA FSM state for Channel:
000: Stopped (Reset or Stop Receive Command issued)
001: Running (Fetching Rx Transfer Descriptor)
010: Reserved for future use
011: Running (Waiting for Rx packet)
100: Suspended (Rx Descriptor Unavailable)
101: Running (Closing the Rx Descriptor)
110: Timestamp write state
111: Running (Transferring the received packet data from the Rx buffer to the system memory)
The MSB of this field always returns 0. This field does not generate an interrupt.

ChibiOS_20.3.1/os/common/ext/ST/STM32F7xx/stm32f767xx.h:8258

Code: Select all

#define ETH_DMADSR_RPS_Pos                            (8U)
#define ETH_DMADSR_RPS_Msk                            (0xFUL << ETH_DMADSR_RPS_Pos) /*!< 0x00000F00 */
#define ETH_DMADSR_RPS                                ETH_DMADSR_RPS_Msk       /* DMA Channel Receive Process State */
#define ETH_DMADSR_RPS_STOPPED                        ((uint32_t)0x00000000)   /* Stopped (Reset or Stop Receive Command issued) */
#define ETH_DMADSR_RPS_FETCHING_Pos                   (12U)
#define ETH_DMADSR_RPS_FETCHING_Msk                   (0x1UL << ETH_DMADSR_RPS_FETCHING_Pos) /*!< 0x00001000 */
#define ETH_DMADSR_RPS_FETCHING                       ETH_DMADSR_RPS_FETCHING_Msk /* Running (Fetching Rx Transfer Descriptor) */
#define ETH_DMADSR_RPS_WAITING_Pos                    (12U)
#define ETH_DMADSR_RPS_WAITING_Msk                    (0x3UL << ETH_DMADSR_RPS_WAITING_Pos) /*!< 0x00003000 */
#define ETH_DMADSR_RPS_WAITING                        ETH_DMADSR_RPS_WAITING_Msk /* Running (Waiting for status) */
#define ETH_DMADSR_RPS_SUSPENDED_Pos                  (14U)
#define ETH_DMADSR_RPS_SUSPENDED_Msk                  (0x1UL << ETH_DMADSR_RPS_SUSPENDED_Pos) /*!< 0x00004000 */
#define ETH_DMADSR_RPS_SUSPENDED                      ETH_DMADSR_RPS_SUSPENDED_Msk /* Suspended (Rx Descriptor Unavailable) */
#define ETH_DMADSR_RPS_CLOSING_Pos                    (12U)
#define ETH_DMADSR_RPS_CLOSING_Msk                    (0x5UL << ETH_DMADSR_RPS_CLOSING_Pos) /*!< 0x00005000 */
#define ETH_DMADSR_RPS_CLOSING                        ETH_DMADSR_RPS_CLOSING_Msk /* Running (Closing the Rx Descriptor) */
#define ETH_DMADSR_RPS_TIMESTAMP_WR_Pos               (13U)
#define ETH_DMADSR_RPS_TIMESTAMP_WR_Msk               (0x3UL << ETH_DMADSR_RPS_TIMESTAMP_WR_Pos) /*!< 0x00006000 */
#define ETH_DMADSR_RPS_TIMESTAMP_WR                   ETH_DMADSR_RPS_TIMESTAMP_WR_Msk /* Timestamp write state */
#define ETH_DMADSR_RPS_TRANSFERRING_Pos               (12U)
#define ETH_DMADSR_RPS_TRANSFERRING_Msk               (0x7UL << ETH_DMADSR_RPS_TRANSFERRING_Pos) /*!< 0x00007000 */
#define ETH_DMADSR_RPS_TRANSFERRING                   ETH_DMADSR_RPS_TRANSFERRING_Msk /* Running (Transferring the received packet data from the Rx buffer to the system memory) */

I think should be:

Code: Select all

#define ETH_DMADSR_RPS_Pos                            (8U)
#define ETH_DMADSR_RPS_Msk                            (0xFUL << ETH_DMADSR_RPS_Pos) /*!< 0x00000F00 */
#define ETH_DMADSR_RPS                                ETH_DMADSR_RPS_Msk       /* DMA Channel Receive Process State */
#define ETH_DMADSR_RPS_STOPPED                        ((uint32_t)0x00000000)   /* Stopped (Reset or Stop Receive Command issued) */
#define ETH_DMADSR_RPS_FETCHING_Pos                   (8U)
#define ETH_DMADSR_RPS_FETCHING_Msk                   (0x1UL << ETH_DMADSR_RPS_FETCHING_Pos) /*!< 0x00001000 */
#define ETH_DMADSR_RPS_FETCHING                       ETH_DMADSR_RPS_FETCHING_Msk /* Running (Fetching Rx Transfer Descriptor) */
#define ETH_DMADSR_RPS_WAITING_Pos                    (9U)
#define ETH_DMADSR_RPS_WAITING_Msk                    (0x3UL << ETH_DMADSR_RPS_WAITING_Pos) /*!< 0x00003000 */
#define ETH_DMADSR_RPS_WAITING                        ETH_DMADSR_RPS_WAITING_Msk /* Running (Waiting for status) */
#define ETH_DMADSR_RPS_SUSPENDED_Pos                  (10U)
#define ETH_DMADSR_RPS_SUSPENDED_Msk                  (0x1UL << ETH_DMADSR_RPS_SUSPENDED_Pos) /*!< 0x00004000 */
#define ETH_DMADSR_RPS_SUSPENDED                      ETH_DMADSR_RPS_SUSPENDED_Msk /* Suspended (Rx Descriptor Unavailable) */
#define ETH_DMADSR_RPS_CLOSING_Pos                    (10U)
#define ETH_DMADSR_RPS_CLOSING_Msk                    (0x5UL << ETH_DMADSR_RPS_CLOSING_Pos) /*!< 0x00005000 */
#define ETH_DMADSR_RPS_CLOSING                        ETH_DMADSR_RPS_CLOSING_Msk /* Running (Closing the Rx Descriptor) */
#define ETH_DMADSR_RPS_TIMESTAMP_WR_Pos               (10U)
#define ETH_DMADSR_RPS_TIMESTAMP_WR_Msk               (0x6UL << ETH_DMADSR_RPS_TIMESTAMP_WR_Pos) /*!< 0x00006000 */
#define ETH_DMADSR_RPS_TIMESTAMP_WR                   ETH_DMADSR_RPS_TIMESTAMP_WR_Msk /* Timestamp write state */
#define ETH_DMADSR_RPS_TRANSFERRING_Pos               (10U)
#define ETH_DMADSR_RPS_TRANSFERRING_Msk               (0x7UL << ETH_DMADSR_RPS_TRANSFERRING_Pos) /*!< 0x00007000 */
#define ETH_DMADSR_RPS_TRANSFERRING                   ETH_DMADSR_RPS_TRANSFERRING_Msk /* Running (Transferring the received packet data from the Rx buffer to the system memory) */

That's correcting offsets and ETH_DMADSR_RPS_TIMESTAMP_WR_Msk

Mike

[Giovanni]

Hi,

MACv2 has been added in trunk, please give it a try.

Giovanni