Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

STM32Hxx porting fixes and suggestions #960

Merged
merged 11 commits into from
Jul 21, 2023
Merged

STM32Hxx porting fixes and suggestions #960

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3e48c7d
typos and wrong function name fixes
miguelfreitas Jul 10, 2023
84dcdef
compilation warning fixes
miguelfreitas Jul 10, 2023
9c29975
port notes updates and suggestions on how to avoid clashing with orig…
miguelfreitas Jul 10, 2023
389ff69
implement hacks described in readme.md to avoid conflict with origina…
miguelfreitas Jul 10, 2023
e0871ca
spacing/identation fixes (uncrustify)
miguelfreitas Jul 10, 2023
172733b
revert cube IDE auto generated code related changes
tony-josi-aws Jul 20, 2023
c64bef1
Merge branch 'main' into miguel_main
tony-josi-aws Jul 20, 2023
6d6ef3c
revert cube IDE auto generated code related changes
tony-josi-aws Jul 20, 2023
7fb47c9
fix formatting
tony-josi-aws Jul 20, 2023
4cfdf89
remove trailing whitespaces
tony-josi-aws Jul 20, 2023
245d587
Merge branch 'main' into main
tony-josi-aws Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 44 additions & 28 deletions source/portable/NetworkInterface/STM32Hxx/NetworkInterface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ SemaphoreHandle_t xTXDescriptorSemaphore = NULL;
static SemaphoreHandle_t xTransmissionMutex;

/* Global Ethernet handle */
static ETH_HandleTypeDef xEthHandle;
ETH_HandleTypeDef xEthHandle;
static ETH_TxPacketConfig xTxConfig;

static NetworkInterface_t * pxMyInterface = NULL;
Expand Down Expand Up @@ -171,7 +171,9 @@ static int32_t ETH_PHY_IO_WriteReg( uint32_t DevAddr,
static void vClearOptionBit( volatile uint32_t * pulValue,
uint32_t ulValue );

static size_t uxGetOwnCount( ETH_HandleTypeDef * heth );
#if ( ipconfigHAS_PRINTF != 0 )
static size_t uxGetOwnCount( ETH_HandleTypeDef * heth );
#endif

/* FreeRTOS+TCP/multi :
* Each network device has 3 access functions:
Expand Down Expand Up @@ -374,7 +376,7 @@ static BaseType_t xSTM32H_GetPhyLinkStatus( NetworkInterface_t * pxInterface )
NetworkInterface_t * pxFillInterfaceDescriptor( BaseType_t xEMACIndex,
NetworkInterface_t * pxInterface )
{
pxSTM32Hxx_FillInterfaceDescriptor( xEMACIndex, pxInterface );
return pxSTM32H_FillInterfaceDescriptor( xEMACIndex, pxInterface );
}

#endif
Expand Down Expand Up @@ -412,7 +414,7 @@ static BaseType_t xSTM32H_NetworkInterfaceOutput( NetworkInterface_t * pxInterfa
TickType_t xBlockTimeTicks = pdMS_TO_TICKS( 100U );
uint8_t * pucTXBuffer;

if( xGetPhyLinkStatus( pxInterface ) == = pdPASS )
if( xSTM32H_GetPhyLinkStatus( pxInterface ) == pdPASS )
{
#if ( ipconfigZERO_COPY_TX_DRIVER != 0 )
/* Zero-copy method, pass the buffer. */
Expand Down Expand Up @@ -801,11 +803,20 @@ static int32_t ETH_PHY_IO_WriteReg( uint32_t ulDevAddr,
/*******************************************************************************
* Ethernet Handling Functions
*******************************************************************************/

void ETH_IRQHandler( void )
{
HAL_ETH_IRQHandler( &( xEthHandle ) );
}
#if 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very clear on the benefits of using the CubeIDE generated code for Ethernet driver and its initialization in this case. We are always defaulting to the ethernet driver present in the source/portable/NetworkInterface/STM32Hxx folder, even if the CubeIDE generates a different version. Also, there are couple of things that user needs to manually update in the project as you have listed in the readme.md to make it work with the autogenerated code, which I think defeats the purpose of the CubeIDE tool.

I believe there are a lot of users that doesn't rely on CubeIDE generated code (atleast for ethernet peripheral), removing ETH_IRQHandler from the NetworkInterface.c will break their code, unless they manually update it.

The only benefit that I see we get from the CubeIDE generated code are the two auto generated functions: HAL_ETH_MspInit() and HAL_ETH_MspDeInit(), which can be helpful in initializing/deinitializing the hardware (especially for the official ST development boards supported by the Cube IDE), but, again has to be manually tweaked for custom boards that runs on STM32Hxx. I think users can always refer the autogenerated version of these functions and implement their own version part of the application itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, @tony-josi-aws. I'm just sharing back the fixes plus the suggestions of how I have been able to integrate the lib to the project here. I didn't have any merge expectations specially on these hacks.

I totally agree with your comments, keeping ETH_IRQHandler at NetworkInterface.c seems pretty reasonable if one is able to remove that function from original driver to avoid conflict. This is not the case if these driver files keep getting overwritten by Cube IDE whenever I change a setting (so the conflict keeps coming back).

So in case of Cube IDE generated code I had to use such hacks to be able to disable parts of their code while keeping the project maintainable. I haven't thought of any other way... Unless maybe I could try to totally disable ETH from Cube IDE (which would also remove the define 'HAL_ETH_MODULE_ENABLED'). I don't know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miguelfreitas, yes, I usually disable code generation for ethernet from the Cube IDE and rely on the NetworkInterface.c. While disabling Cube IDE generated code for ethernet we have to manually define (uncomment) HAL_ETH_MODULE_ENABLED in Core\Inc\stm32h7xx_hal_conf.h.

STM32Hxx network interface was not fully updated to support the IPv6 related functionalities which got recently added to the main branch. The PRs: #970 and #965 (970 is an alternative approach for changes made in 965) should fix that. While updating those changes some of the build related fixes that you had made in your PR are already added in those PRs, which was required while testing. Will you be interested in updating your PR with respect to the latest changes or prefer us to do it on your behalf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tony-josi-aws, thanks for this new info. I'd prefer you to to proceed with updating this or the other PR with the latest changes as you prefer since I'll be away on vacations for the next two weeks. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @miguelfreitas, I will update the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miguelfreitas, merging this PR. Thanks for contibuting to FreeRTOS+TCP.


/* ETH_IRQHandler might be defined in the (auto-generated) stm32h7xx_it.c.
* In order to not clash with the other implementation it is possible to disable
* the code here and add the following define to an "USER CODE" section
* of stm32h7xx_it.c to trick it into using the right handle.
* #define heth xEthHandle
* (...) generated code there (can't edit): HAL_ETH_IRQHandler(&heth);
*/
void ETH_IRQHandler( void )
{
HAL_ETH_IRQHandler( &( xEthHandle ) );
}
#endif
/*-----------------------------------------------------------*/

static void prvSetFlagsAndNotify( uint32_t ulFlags )
Expand Down Expand Up @@ -870,7 +881,7 @@ void vNetworkInterfaceAllocateRAMToBuffers( NetworkBufferDescriptor_t pxNetworkB
}
/*-----------------------------------------------------------*/

#define __NOP() __ASM volatile ( "nop" )
/*#define __NOP() __ASM volatile ( "nop" ) */

static void vClearOptionBit( volatile uint32_t * pulValue,
uint32_t ulValue )
Expand All @@ -881,26 +892,28 @@ static void vClearOptionBit( volatile uint32_t * pulValue,
}
/*-----------------------------------------------------------*/

static size_t uxGetOwnCount( ETH_HandleTypeDef * heth )
{
BaseType_t xIndex;
BaseType_t xCount = 0;
ETH_RxDescListTypeDef * dmarxdesclist = &heth->RxDescList;

/* Count the number of RX descriptors that are owned by DMA. */
for( xIndex = 0; xIndex < ETH_RX_DESC_CNT; xIndex++ )
#if ( ipconfigHAS_PRINTF != 0 )
static size_t uxGetOwnCount( ETH_HandleTypeDef * heth )
{
__IO const ETH_DMADescTypeDef * dmarxdesc =
( __IO const ETH_DMADescTypeDef * )dmarxdesclist->RxDesc[ xIndex ];
BaseType_t xIndex;
BaseType_t xCount = 0;
ETH_RxDescListTypeDef * dmarxdesclist = &heth->RxDescList;

if( ( dmarxdesc->DESC3 & ETH_DMARXNDESCWBF_OWN ) != 0U )
/* Count the number of RX descriptors that are owned by DMA. */
for( xIndex = 0; xIndex < ETH_RX_DESC_CNT; xIndex++ )
{
xCount++;
__IO const ETH_DMADescTypeDef * dmarxdesc =
( __IO const ETH_DMADescTypeDef * )dmarxdesclist->RxDesc[ xIndex ];

if( ( dmarxdesc->DESC3 & ETH_DMARXNDESCWBF_OWN ) != 0U )
{
xCount++;
}
}
}

return xCount;
}
return xCount;
}
#endif /* if ( ipconfigHAS_PRINTF != 0 ) */
/*-----------------------------------------------------------*/

static void prvEMACHandlerTask( void * pvParameters )
Expand All @@ -909,8 +922,11 @@ static void prvEMACHandlerTask( void * pvParameters )
* be occupied. In stat case, the program will wait (block) for the counting
* semaphore. */
const TickType_t ulMaxBlockTime = pdMS_TO_TICKS( 100UL );
size_t uxTXDescriptorsUsed = 0U;
size_t uxRXDescriptorsUsed = ETH_RX_DESC_CNT;

#if ( ipconfigHAS_PRINTF != 0 )
size_t uxTXDescriptorsUsed = 0U;
size_t uxRXDescriptorsUsed = ETH_RX_DESC_CNT;
#endif

( void ) pvParameters;

Expand Down Expand Up @@ -994,7 +1010,7 @@ static void prvEMACHandlerTask( void * pvParameters )
* The function xPhyCheckLinkStatus() returns pdTRUE if the
* Link Status has changes since it was called the last time.
*/
if( xGetPhyLinkStatus( pxMyInterface ) == pdFALSE )
if( xSTM32H_GetPhyLinkStatus( pxMyInterface ) == pdFALSE )
{
/* Stop the DMA transfer. */
HAL_ETH_Stop_IT( &( xEthHandle ) );
Expand Down
35 changes: 31 additions & 4 deletions source/portable/NetworkInterface/STM32Hxx/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,22 @@ The following macro's are **not** used by the FreeRTOS driver:

All memory that is shared between the CPU and the DMA ETH peripheral, should be
located in special RAM area called ".ethernet_data". This shall be declared in
the linker file.
the linker file (.ld).

It is possible to use the AXI SRAM for this, but RAM{1,2,3} are also connected
to the Ethernet MAC.

Here is an example of the changes to the linker file:

AXI_RAM (xrw) : ORIGIN = 0x24000000, LENGTH = 512K /* .ethernet_data declared here. */
.ethernet_data :
RAM_D1 (xrw) : ORIGIN = 0x24000000, LENGTH = 512K /* should already exist in MEMORY section */

.ethernet_data : /* inside SECTIONS section, before /DISCARD/ */
{
PROVIDE_HIDDEN (__ethernet_data_start = .);
KEEP (*(SORT(.ethernet_data.*)))
KEEP (*(.ethernet_data*))
PROVIDE_HIDDEN (__ethernet_data_end = .);
} >AXI_RAM
} >RAM_D1

Here is a table of 3 types of STH32H7 :

Expand All @@ -69,6 +70,10 @@ Here is a table of 3 types of STH32H7 :
Please make sure that the addresses and lengths are correct for your model of STM32H7xx.
If you use a memory that is not supported, it will result in a DMA errors.

Don't redefine a new memory area (like AXI-SRAM, RAM_D1) if it already exists in the
MEMORY section, just take note of it's name for defining the section .ethernet_data later
in that same file.

In FreeRTOSIPConfig.h :

Define the total number of network buffer descriptors, e.g. 64:
Expand Down Expand Up @@ -120,3 +125,25 @@ The most important DMAC registers, along with their names which are used in the
As most EMAC's, the STM32H7 EMAC is able to put packets in multiple linked DMA segments.
FreeRTOS+TCP never uses this feature. Each packet is stored in a single buffer called
`NetworkBufferDescriptor_t`.

~~~

The provided NetworkInterface.c and stm32hxx_hal_eth.c may clash with the original
auto-generated files from STM32CubeIDE code generator. Some tricks may apply:

1) Undefining HAL_ETH_MODULE_ENABLED at the end of stm32hxx_hal_eth.h and having
"portable/NetworkInterface/STM32Hxx" included before "STM32H7xx_HAL_Driver/Inc" in
path order. This will disable STM32H7xx_HAL_Driver/stm32hxx_hal_eth.c entirely
(removing the link file within IDE project might not work since it keeps coming
back on reconfiguration).

2) Remove '#ifdef HAL_ETH_MODULE_ENABLED' check from our own stm32hxx_hal_eth.c
(so it will compile regardless of the #undef just added above).

3) Comment ETH_IRQHandler() from NetworkInterface.c and trick stm32h7xx_it.c's
version of the same function into using our handle xEthHandle instead of heth.

4) Remove DMARxDscrTab and DMATxDscrTab from auto-generated main.c. Since they are
inside a non-"USER CODE" section, one possible trick is to temporaly undefine __GNUC__
in main.c so these two variables are never compiled there.

Loading