MCUXpresso: Fix ENET driver to enable interrupts after interrupt handler is set #3519

mmahadevan108 · 2017-01-03T19:11:10Z

Description

This PR is a fix for issue 3358
#3358

Status

**READY

[ ] Tests
Ran the NET tests on K64 and K66 FRDM boards.

mmahadevan108 · 2017-01-03T19:11:48Z

cc @geky @0xc0170 @kuggenhoffen

bridadan · 2017-01-03T19:51:53Z

/morph test-nightly

mbed-bot · 2017-01-03T22:08:37Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1348

All builds and test passed!

tommikas · 2017-01-04T14:50:53Z

Seems like this might be incompatible with https://github.com/ARMmbed/sal-nanostack-driver-k64f-eth. I'm not sure if that should prevent merging though.

@SeppoTakalo Could you have a look please?

0xc0170 · 2017-01-04T14:55:29Z

@tommikas Please provide details. This patch contains a patch in enet driver. I believe the repository referenced here is based on older driver (even prior the rename). Worth testing anyway. Let us know

tommikas · 2017-01-05T16:45:30Z

The sal-nanostack-driver-k64f-eth driver is a nanostack compatible ethernet driver for the FRDM-K64F board. Before this PR it worked fine, but with the changes in this PR the connect() call consistently fails (freezes, it seems). I haven't debugged further than that.

As I said, I'm not at all certain that warrants holding this PR from being merged, though. I was hoping @SeppoTakalo could chime in with a better answer.

SeppoTakalo · 2017-01-09T09:14:57Z

@hasnainvirk As you have the knowledge of the nanostack port of this driver, can you study why it does not work anymore with these changes merged in?

0xc0170 · 2017-01-09T14:08:44Z

@hasnainvirk As you have the knowledge of the nanostack port of this driver, can you study why it does not work anymore with these changes merged in?

please do, we can set this to "Do not merge" but we should know a reason prior that. I'll set it for now needs: review. Waiting for more specific information

hasnainvirk

@mmahadevan108 @0xc0170 @SeppoTakalo
If the purpose was to enable interrupts after the handle is set, i.e., to avoid setting up ISR with null s_ENETHandle, then this PR doesn't seem to solve that problem. Because, ENET_SetMacController() enables interrupts before you call ENET_SetHandler().
In my opinion, the solution could have been a one liner. I mean before calling ENET_SetMacController() in ENET_Init(), just add this line s_ENETHandle[instance] = handle;
In the current state, your PR is breaking our implementation of eth driver. I didn't debugged in detail, so I can't be sure what exactly is breaking.

hasnainvirk

Okay, I debugged and found out the issue with this PR. @0xc0170 @SeppoTakalo
@mmahadevan108 Mahesh you forgot to add handle->txBdDirty = bufferConfig->txBdStartAddrAlign;
while setting up handler. After adding this I could start enet module relieably. Please fix.

hasnainvirk · 2017-01-10T12:09:52Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_K66F/drivers/fsl_enet.c

+    handle->rxBdBase = bufferConfig->rxBdStartAddrAlign;
+    handle->rxBdCurrent = bufferConfig->rxBdStartAddrAlign;
+    handle->txBdBase = bufferConfig->txBdStartAddrAlign;
+    handle->txBdCurrent = bufferConfig->txBdStartAddrAlign;


handle->txBdDirty = bufferConfig->txBdStartAddrAlign;
Adding this will fix your PR.

Same to the K64F files as well

Thanks for looking into this and finding the issue. I have update the PR.

hasnainvirk

@mmahadevan108 Hi Mahesh,
Sorry to bother you again mate. Can you please refactor the code a little bit more . IMO this will ensure what was your initial intention. I caught another minor glitch (its not fatal though). In 'ENET_SetTxBufferDescriptors()', there exists a null check upon an integer which is ofcourse wrong. The code looks like that: if (txBuffSizeAlign != NULL) where txBuffSizeAlign is an integer. I think its a typo, as it should be txBuffStartAlign != NULL where txBufferStartAlign is the pointer to an aligned data buffer. While you are at it, I think its a good opportunity to fix all which is obvious. I am sorry again to stretch your work a bit more. Other than that, it looks pretty good to me. Please make sure that all the requested changes go in for both K66F and K64F. Thanks in advance

hasnainvirk · 2017-01-10T14:54:44Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_K66F/drivers/fsl_enet.c

-    /* Save the handle pointer in the global variables. */
-    s_ENETHandle[instance] = handle;
+    /* Set all buffers or data in handler for data transmit/receive process. */
+    ENET_SetHandler(base, handle, config, bufferConfig);


Step 1. Please move ENET_SetHandler() above ENET_setMacController()

hasnainvirk · 2017-01-10T14:58:54Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_K66F/drivers/fsl_enet.c

+    s_ENETHandle[instance] = handle;
+
+    /* Set the IRQ handler when the interrupt is enabled. */
+    if (config->interrupt & ENET_TX_INTERRUPT)


Step 2. Please move this if-else section from here to ENET_SetMacController() , paste it after "ENET_EnableInterrupts(base, config->interrupt)" but before "ecr = base->ECR;"

Thanks, you are correct, I have changed txBuffSizeAlign to txBuffStartAlign.

With regards to the remaining, I don't see why those changes are required. The code enables the interrupts in the NVIC inside ENET_SetHandler() which is the done after the handle is saved in the global variable. This should address the issue reported.

PR has been updated.

Okay, so you mean that ENET_EnableInterrupts(base, config->interrupt enable interrupts for the enet module and NVIC still at that particular time gates the interrupts to processor ? Alrigh, if that's the case. I think I got your point. I was of the view that when you write to base->EIMR, it enables interrupts globally.

…lers are set Signed-off-by: Mahadevan Mahesh <[email protected]>

tommikas · 2017-01-11T15:55:13Z

Just a heads up - continuous-integration/jenkins/pr-head showing red at the moment is unrelated to this PR. The tests that were previously failing are now passing since the latest commits.

bridadan · 2017-01-11T16:38:59Z

@tommikas Do you know if the stability issues with continuous-integration/jenkins/pr-head will be fixed soon? It's been failing quite a bit more recently.

tommikas · 2017-01-12T14:16:01Z

@bridadan The latest (and greatest) issue causing huge runtimes and even more failures has been resolved and it's looking a lot better today. The core issue behind the persistent problems still exists though, and cannot be fully resolved for at least a few weeks for reasons outside our control. In the meantime we have been doing and continue to do what we can to mitigate the problem.

0xc0170 · 2017-01-12T14:45:21Z

Thanks for the heads-up @tommikas

This is now green , should be good to go.

bridadan · 2017-01-12T15:50:18Z

Thanks for following up @tommikas!

Ports for Upcoming Targets Fixes and Changes 3488: Dev stm i2c v2 unitary functions ARMmbed/mbed-os#3488 3492: Fix #3463 CAN read() return value ARMmbed/mbed-os#3492 3503: [LPC15xx] Ensure that PWM=1 is resolved correctly ARMmbed/mbed-os#3503 3504: [LPC15xx] CAN implementation improvements ARMmbed/mbed-os#3504 3539: NUCLEO_F412ZG - Add support of TRNG peripheral ARMmbed/mbed-os#3539 3540: STM: SPI: Initialize Rx in spi_master_write ARMmbed/mbed-os#3540 3438: K64F: Add support for SERIAL ASYNCH API ARMmbed/mbed-os#3438 3519: MCUXpresso: Fix ENET driver to enable interrupts after interrupt handler is set ARMmbed/mbed-os#3519 3544: STM32L4 deepsleep improvement ARMmbed/mbed-os#3544 3546: NUCLEO-F412ZG - Add CAN peripheral ARMmbed/mbed-os#3546 3551: Fix I2C driver for RZ/A1H ARMmbed/mbed-os#3551 3558: K64F UART Asynch API: Fix synchronization issue ARMmbed/mbed-os#3558 3563: LPC4088 - Fix vector checksum ARMmbed/mbed-os#3563 3567: Dev stm32 F0 v1.7.0 ARMmbed/mbed-os#3567 3577: Fixes linking errors when building with debug profile ARMmbed/mbed-os#3577

bridadan added needs: CI needs: review labels Jan 3, 2017

0xc0170 removed the needs: CI label Jan 4, 2017

0xc0170 approved these changes Jan 4, 2017

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Jan 4, 2017

geky approved these changes Jan 5, 2017

View reviewed changes

0xc0170 added needs: review and removed ready for merge labels Jan 9, 2017

hasnainvirk suggested changes Jan 10, 2017

View reviewed changes

hasnainvirk reviewed Jan 10, 2017

View reviewed changes

mmahadevan108 force-pushed the ENET_Driver_Interrupt_Enable_Issue branch from cbb747b to 298b5a6 Compare January 10, 2017 14:21

hasnainvirk suggested changes Jan 10, 2017

View reviewed changes

MCUXpresso: Fix ENET driver to enable interrupts after interrupt hand…

89f8fe4

…lers are set Signed-off-by: Mahadevan Mahesh <[email protected]>

mmahadevan108 force-pushed the ENET_Driver_Interrupt_Enable_Issue branch from 298b5a6 to 89f8fe4 Compare January 10, 2017 20:53

hasnainvirk approved these changes Jan 11, 2017

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Jan 12, 2017

adbridge added release-version: 5.3.3 and removed ready for merge labels Jan 12, 2017

adbridge merged commit fd52695 into ARMmbed:master Jan 12, 2017

mmahadevan108 deleted the ENET_Driver_Interrupt_Enable_Issue branch January 12, 2017 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MCUXpresso: Fix ENET driver to enable interrupts after interrupt handler is set #3519

MCUXpresso: Fix ENET driver to enable interrupts after interrupt handler is set #3519

mmahadevan108 commented Jan 3, 2017

mmahadevan108 commented Jan 3, 2017

bridadan commented Jan 3, 2017

mbed-bot commented Jan 3, 2017

tommikas commented Jan 4, 2017

0xc0170 commented Jan 4, 2017

tommikas commented Jan 5, 2017 •

edited

Loading

SeppoTakalo commented Jan 9, 2017

0xc0170 commented Jan 9, 2017 •

edited

Loading

hasnainvirk left a comment •

edited

Loading

hasnainvirk left a comment •

edited

Loading

hasnainvirk Jan 10, 2017

hasnainvirk Jan 10, 2017

mmahadevan108 Jan 10, 2017

hasnainvirk left a comment •

edited

Loading

hasnainvirk Jan 10, 2017

hasnainvirk Jan 10, 2017

mmahadevan108 Jan 10, 2017

mmahadevan108 Jan 10, 2017

hasnainvirk Jan 11, 2017

tommikas commented Jan 11, 2017

bridadan commented Jan 11, 2017

tommikas commented Jan 12, 2017

0xc0170 commented Jan 12, 2017

bridadan commented Jan 12, 2017

MCUXpresso: Fix ENET driver to enable interrupts after interrupt handler is set #3519

MCUXpresso: Fix ENET driver to enable interrupts after interrupt handler is set #3519

Conversation

mmahadevan108 commented Jan 3, 2017

Description

Status

mmahadevan108 commented Jan 3, 2017

bridadan commented Jan 3, 2017

mbed-bot commented Jan 3, 2017

Output

tommikas commented Jan 4, 2017

0xc0170 commented Jan 4, 2017

tommikas commented Jan 5, 2017 • edited Loading

SeppoTakalo commented Jan 9, 2017

0xc0170 commented Jan 9, 2017 • edited Loading

hasnainvirk left a comment • edited Loading

Choose a reason for hiding this comment

hasnainvirk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasnainvirk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommikas commented Jan 11, 2017

bridadan commented Jan 11, 2017

tommikas commented Jan 12, 2017

0xc0170 commented Jan 12, 2017

bridadan commented Jan 12, 2017

tommikas commented Jan 5, 2017 •

edited

Loading

0xc0170 commented Jan 9, 2017 •

edited

Loading

hasnainvirk left a comment •

edited

Loading

hasnainvirk left a comment •

edited

Loading

hasnainvirk left a comment •

edited

Loading