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.

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

MIMXRT1050: Fix ENET issues #10299

Merged
merged 1 commit into from Apr 4, 2019

Conversation

Projects
None yet
7 participants
@mmahadevan108
Copy link
Contributor

commented Apr 2, 2019

Description

This is a fix for Issue#10239. The change aligns the receive buffer lengths

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Apr 2, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@mmahadevan108, thank you for your changes.
@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-ipcore please review.

@mmahadevan108 mmahadevan108 force-pushed the NXPmicro:Fix_MXRT1050_ENET branch from 5c5b6b4 to 3ffb9b4 Apr 2, 2019

@kjbracey-arm
Copy link
Contributor

left a comment

This leaves some things undone:

  • Alignment needs to be 32 bytes
  • You also need to invalidate the cache for a buffer before assigning it to a RX descriptor (in update _read_buffer?). This prevents writeback into the region during reception.
  • You need to clean the cache for a buffer when adding a TX descriptor.

I still don't really understand the original issue - it seems to me like it should be performing much worse due to the lack of invalidates and cleans, and the absence of them doesn't explain the crash - it would just lead to data corruption.

I'm suspicious that the caches have just been disabled somewhere (like other M7 platforms have done when turning on Ethernet...)

@@ -189,7 +192,8 @@ bool Kinetis_EMAC::low_level_init_successful()

/* Create buffers for each receive BD */
for (i = 0; i < ENET_RX_RING_LEN; i++) {
rx_buff[i] = memory_manager->alloc_heap(ENET_ETH_MAX_FLEN, ENET_BUFF_ALIGNMENT);
rx_buff[i] = memory_manager->alloc_heap(ENET_ALIGN(ENET_ETH_MAX_FLEN, ENET_BUFF_ALIGNMENT),

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 3, 2019

Contributor

When I was inspecting it, I didn't think an extra ENET_ALIGN was necessary, as you're already passing in ENET_BUFF_ALIGNMENT, which should mean alloc_heap rounds up itself.

But because of the implementation, this will end up adding a couple more bytes, and maybe that makes the difference if we're overrunning?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 3, 2019

Contributor

This analysis was incorrect, as per comment on #10239 - alloc_heap does not in fact round up the size.

@@ -278,8 +282,14 @@ emac_mem_buf_t *Kinetis_EMAC::low_level_input(int idx)
p = rx_buff[idx];
memory_manager->set_len(p, length);

#if defined(FSL_SDK_ENABLE_DRIVER_CACHE_CONTROL) && FSL_SDK_ENABLE_DRIVER_CACHE_CONTROL
/* Add the cache invalidate maintain. */
DCACHE_InvalidateByRange((uint32_t)bdPtr->buffer, g_handle.rxBuffSizeAlign[0]);

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 3, 2019

Contributor

If you are going to be cache invalidating, I believe the RX buffer alignment will need to be increased from 4 to 32 (FSL_FEATURE_L1DCACHE_LINESIZE_BYTE)

@@ -1917,6 +1917,7 @@
"SKIP_SYSCLK_INIT",
"FSL_FEATURE_PHYKSZ8081_USE_RMII50M_MODE",
"SDRAM_IS_SHAREABLE",
"FSL_SDK_ENABLE_DRIVER_CACHE_CONTROL=1",

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 3, 2019

Contributor

Not sure about turning this on. This activates code in fsl_enet.c which mostly isn't used, but I can see one incorrect one at line 812:

       DCACHE_InvalidateByRange((uint32_t)rxBuffer,  (buffCfg->rxBdNumber * rxBuffSizeAlign));

That's assuming rxBuffer is a contiguous array of packets (as its pointer type suggests), when really it's pointing to an array of pointers to packets, and accessed via insane casting:

       for (count = 0; count < buffCfg->rxBdNumber; count++)
        {
            /* Set data buffer and the length. */
            curBuffDescrip->buffer = (uint8_t *)(*((uint32_t *)(rxBuffer + count * 4)));

So that invalidate could be trashing quite a lot of system workspace.

You actually want

       DCACHE_InvalidateByRange((uint32_t)curBuffDescrip->buffer,  rxBuffSizeAlign);

inside the loop.

This comment has been minimized.

Copy link
@mmahadevan108

mmahadevan108 Apr 3, 2019

Author Contributor

Thanks for highlighting this, I will raise a ticket against the SDK driver.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 3, 2019

MIMXRT1050: Fix ENET issues
This is a fix for Issue 10239. The change aligns
the receive buffer lengths

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>

@mmahadevan108 mmahadevan108 force-pushed the NXPmicro:Fix_MXRT1050_ENET branch from 3ffb9b4 to 65942ba Apr 3, 2019

@kjbracey-arm
Copy link
Contributor

left a comment

Looks correct to me now.

Follow-up tasks would be:

  1. check other drivers for the same issue - several have the same logic flaw but I guess get away with it because their alignment is only 4, which is the same as the heap.
  2. Put in the data cache cleans and invalidates and turn that shareable flag off again so you get your data cache back into use. The i.MX RT1050's data cache has been effectively disabled since the Ethernet driver was added to the tree.
@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 4, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I restarted client error - k64f target error - not related

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 4, 2019

@0xc0170 0xc0170 merged commit 71c84e8 into ARMmbed:master Apr 4, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9068 cycles (-149 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
@unsignedint

This comment has been minimized.

Copy link

commented Apr 5, 2019

@0xc0170 I'm not sure what your release plan is... but it would be ideal if this and #10314 could go into 5.11 as a maintenance release. For our project we already have custom hardware and to upgrade to 5.12 at this point is non-trivial. Of course we can take it as an independent patch, but potentially others may also benefit from a 5.11 point update at some point? :)

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Please take them as independent patches. Only patch releases for 5.12 at the moment.

Note, besides our release plan, 5.12 introduced non-backward compatible changes (toolchain updates), testing older releases would take quite a time (CI switch and so on). We will do some improvements there soon, don't have anything solid to provide.

@mmahadevan108 mmahadevan108 deleted the NXPmicro:Fix_MXRT1050_ENET branch Apr 5, 2019

@cmonr cmonr removed the ready for merge label Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.