Feature emac lpc546xx #6810

mmahadevan108 · 2018-05-03T21:13:57Z

ENET support for LPC546XX boards

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

mmahadevan108 · 2018-05-03T21:16:12Z

cc @maclobdell @0xc0170 @kjbracey-arm

TuomoHautamaki · 2018-05-07T11:06:42Z

Hi @mmahadevan108 , great to see this PR coming. Did you run netsocket and emac greentea tests already?

Regards,
Tuomo

mmahadevan108 · 2018-05-07T20:11:13Z

I have rebased this PR. Yes, I ran the netsocket and network-emac tests.

cmonr · 2018-05-07T23:42:06Z

@mmahadevan108 Would you mind posting those logs?

/morph build

mbed-ci · 2018-05-08T00:03:41Z

Build : SUCCESS

Build number : 1939
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6810/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-05-08T07:32:48Z

Exporter Build : SUCCESS

Build number : 1586
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6810/

mbed-ci · 2018-05-08T08:05:47Z

Test : FAILURE

Build number : 1761
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6810/1761

0xc0170 · 2018-05-08T08:41:30Z

@mmahadevan108 Please review the failures, related to the changes

mmahadevan108 · 2018-05-08T14:38:58Z

I am unclear why the failures are seen. All tests pass with ARM tool chain. One test failure seen with GCC_ARM and more with IAR.

Is there a toolchain specific configuration I need to pay attention to in the new ENET frameowork?

mmahadevan108 · 2018-05-08T19:47:20Z

I was debugging the failure with IAR and see the it is failing to get the below semaphore during bringup
https://github.com/ARMmbed/mbed-os/blob/feature-emac/features/FEATURE_LWIP/lwip-interface/LWIPInterface.cpp#L522

Any idea why?

maclobdell · 2018-05-08T20:19:06Z

@mmahadevan108 did you run the tests with an echo server on the network? I just noticed that the tests might pass if an echo server is not running, but will fail otherwise.

mmahadevan108 · 2018-05-08T20:23:09Z

I was debugging the gethostbyname test under netsocket. This is failing in the mbed-ci for IAR and GCC_ARM but passes for ARM. All tests pass in mbed-ci for the ARM toolchain.

maclobdell · 2018-05-09T18:24:01Z

@mmahadevan108

I rebased your LPC546XX commits on top of Mika’s branch referenced in #6851. I had to add an additional commit to add “EMAC” and “ETHERNET” to the device_has list in targets.json.

I ran the tests and the broadcast test is failing.

At your earliest convenience, please test it out.

If you have any questions or comments please contact me.

Initial results:

+------------------+---------------+--------------------+-----------------+--------+--------+--------+--------------------+
| target           | platform_name | test suite         | test case       | passed | failed | result | elapsed_time (sec) |
+------------------+---------------+--------------------+-----------------+--------+--------+--------+--------------------+
| LPC546XX-GCC_ARM | LPC546XX      | tests-network-emac | EMAC broadcast  | 0      | 1      | FAIL   | 10.37              |
| LPC546XX-GCC_ARM | LPC546XX      | tests-network-emac | EMAC initialize | 1      | 0      | OK     | 1.88               |
+------------------+---------------+--------------------+-----------------+--------+--------+--------+--------------------+

mmahadevan108 · 2018-05-09T20:56:16Z

@maclobdell I am seeing issues acquiring a semaphore. I am not clear if I am missing something in my code. I have highlighted this above, any suggestions.

maclobdell · 2018-05-10T15:46:01Z

@mikaleppanen Please see issue acquiring semaphore that Mahesh highlighted. Let us know if you have any suggestions.

mikaleppanen · 2018-05-11T06:30:00Z

The semaphore here:

https://github.com/ARMmbed/mbed-os/blob/feature-emac/features/FEATURE_LWIP/lwip-interface/LWIPInterface.cpp#L522

waits for the LWIP stack DHCP negotiation to complete, so failing to acquire the semaphore indicates that there could be any problem in IP message transfer. Can you acquire logs from wire to see in what phase the negotiation fails.

mikaleppanen

Added two comments based on greentea testing.

mikaleppanen · 2018-05-11T11:23:40Z

features/netsocket/emac-drivers/TARGET_NXP_EMAC/TARGET_LPC546XX/lpc546xx_emac.cpp

+        ENET_ALIGN(ENET_ETH_MAX_FLEN, ENET_BUFF_ALIGNMENT),
+    };
+
+    ENET_Init(ENET, &config, hwaddr, refClock);


On the new emac interface power_up() mac address is not yet set at this point so it contains null string. At old lwip driver interface it was valid already at this point. On new emac, mac is written by network stack to emac after the power_up() with void set_hwaddr() call.

We are having test case failures (at least with IAR) where the device does not receive messages sent to its mac address.

Could you check that mac address configuration works as expected. Just was testing purposes, we changed code so that mac is valid already here and the driver worked. But that cannot be a solution in new emac since mac written later using the set_hwaddr().

Yes, it seems like you should be passing NULL here rather than hwaddr, so ENET_Init doesn't try programming the MAC address yet.

Not sure what the other test failures were.

One option is to use the mbed_mac_address() call to set the address to hwaddr before the ENET_Init call.

mikaleppanen · 2018-05-11T11:35:49Z

features/netsocket/emac-drivers/TARGET_NXP_EMAC/TARGET_LPC546XX/lpc546xx_emac.cpp

+        update_read_buffer(bdPtr, NULL);
+    } else {
+        if (bdPtr->control & ENET_RXDESCRIP_WR_LD_MASK) {
+            length = (bdPtr->control & ENET_RXDESCRIP_WR_PACKETLEN_MASK);


When testing, we are seeing here that the length here is 4 bytes more than expected. Also with incoming message with 1500 bytes long payload there is a buffer tail overwrite of the received emac_mem_buf_t. Could you check the length handling.

I can't see an issue with this code. I need to debug more why this is happening.

The datasheet says that the packet written to the buffer includes the CRC, and the length in this descriptor reflects that.

You just need to subtract 4, as far as I can see, as we want the packet length including MAC header, but excluding CRC. (You or the hardware are assumed to have checked and stripped the CRC - the hardware here has checked it but not stripped it.)

mikaleppanen · 2018-05-11T12:01:15Z

features/netsocket/emac-drivers/TARGET_NXP_EMAC/TARGET_LPC546XX/lpc546xx_emac.cpp

+
+void LPC546XX_EMAC::add_multicast_group(const uint8_t *addr)
+{
+    /* No-op at this stage */


When testing with greentea, driver did not pass multicast packets to network stack. You should add set_all_multicast(true) call here, so in case network stack configures multicast (i.e. is ipv6 capable) multicast filtering is disabled.

kjbracey · 2018-05-15T13:49:35Z

Any progress here?

maclobdell · 2018-05-16T03:46:26Z

@mmahadevan108 - please let us know if these suggested updates work for you.

Signed-off-by: Mahesh Mahadevan <[email protected]>

cmonr · 2018-05-17T15:34:46Z

@mmahadevan108 ^^^

mmahadevan108 · 2018-05-17T16:21:20Z

I have updated the PR to include the call for set_all_multicast(true); inside add_multicast_group() function. I am not clear why we are seeing the length issue.

kjbracey · 2018-05-18T07:53:33Z

On the MAC address, the problem is due to an error in the driver code for ENET_SetMacAddr.

mbed-os/targets/TARGET_NXP/TARGET_MCUXpresso_MCUS/TARGET_LPC546XX/drivers/fsl_enet.h

Line 678 in 4b72158

    
           base->MAC_ADDR_LOW = ((uint32_t)macAddr[3] << 24) | ((uint32_t)macAddr[2] << 16) | ((uint32_t)macAddr[1] << 8) |

The MAC_ADDR_LOW register must be written last. The user manual says:

If the MAC address registers are configured to be double-synchronized to the MII clock
domains, then the synchronization is triggered only when bits 31:24 (in little-endian mode)
or bits 7:0 (in Big-Endian mode) of the MAC address low register are written to.

Swapping the order has made it work here.

kjbracey · 2018-05-18T08:23:31Z

The remaining issue is the memory buffer tail overwrite. We've not run the tests yet, but I believe the issue is that you are configuring the hardware buffer size to ENET_ALIGN(ENET_ETH_MAX_FLEN, ENET_BUFF_ALIGNMENT) - this is 4-byte aligned, as the hardware requires (1520)

But you're only actually allocating ENET_ETH_MAX_FLEN (1518) from the memory pool, so the hardware is overwriting by 2 bytes. Increase the pool allocation request to the aligned number.

kjbracey · 2018-05-18T09:34:01Z

I've applied the 3 fixes I describe to the combined PR here #6936

kjbracey

Approved, with the 3 changes I've applied on the combined test PR. Passing tests here.

kjbracey · 2018-05-21T09:24:29Z

Merged via #6936

0xc0170 added the needs: review label May 4, 2018

0xc0170 requested review from kjbracey and mikaleppanen May 4, 2018 08:49

mmahadevan108 force-pushed the feature-emac-lpc546xx branch from 7821aaa to 86d4f33 Compare May 7, 2018 20:09

0xc0170 added needs: work and removed needs: review labels May 8, 2018

kjbracey force-pushed the feature-emac branch from ea912d5 to 942209e Compare May 9, 2018 07:09

mmahadevan108 force-pushed the feature-emac-lpc546xx branch from 86d4f33 to 9660393 Compare May 9, 2018 15:04

mikaleppanen reviewed May 11, 2018

View reviewed changes

mmahadevan108 added 2 commits May 17, 2018 07:32

LPC546XX: Add ENET support

ffb5ce5

Signed-off-by: Mahesh Mahadevan <[email protected]>

Add memory configuration for LPC546XX

fa72cde

Signed-off-by: Mahesh Mahadevan <[email protected]>

mmahadevan108 force-pushed the feature-emac-lpc546xx branch from 9660393 to fa72cde Compare May 17, 2018 16:15

kjbracey mentioned this pull request May 18, 2018

Feature-emac combined merge #6936

Merged

kjbracey added the do not merge label May 18, 2018

kjbracey approved these changes May 18, 2018

View reviewed changes

kjbracey closed this May 21, 2018

kjbracey removed do not merge needs: work labels May 21, 2018

mmahadevan108 deleted the feature-emac-lpc546xx branch July 4, 2018 13:18

Feature emac lpc546xx #6810

Feature emac lpc546xx #6810

Uh oh!

Conversation

mmahadevan108 commented May 3, 2018

Uh oh!

mmahadevan108 commented May 3, 2018

Uh oh!

TuomoHautamaki commented May 7, 2018

Uh oh!

mmahadevan108 commented May 7, 2018

Uh oh!

cmonr commented May 7, 2018

Uh oh!

mbed-ci commented May 8, 2018

Triggering tests

Uh oh!

mbed-ci commented May 8, 2018

Uh oh!

mbed-ci commented May 8, 2018

Uh oh!

0xc0170 commented May 8, 2018

Uh oh!

mmahadevan108 commented May 8, 2018

Uh oh!

mmahadevan108 commented May 8, 2018

Uh oh!

maclobdell commented May 8, 2018

Uh oh!

mmahadevan108 commented May 8, 2018

Uh oh!

maclobdell commented May 9, 2018

Uh oh!

mmahadevan108 commented May 9, 2018

Uh oh!

maclobdell commented May 10, 2018

Uh oh!

mikaleppanen commented May 11, 2018

Uh oh!

mikaleppanen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey commented May 15, 2018

Uh oh!

maclobdell commented May 16, 2018

Uh oh!

cmonr commented May 17, 2018

Uh oh!

mmahadevan108 commented May 17, 2018

Uh oh!

kjbracey commented May 18, 2018

Uh oh!

kjbracey commented May 18, 2018

Uh oh!

kjbracey commented May 18, 2018

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

kjbracey commented May 21, 2018

Uh oh!

Uh oh!