-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Enable Fast Models emac drivers #8668
Conversation
Greentea test result for netsocket on all toolchains
|
Some commonality in concept and review comments with the ongoing #8444 here - could you have a read through the discussion on that, and check its final code for style preference? In particular:
|
network-emac and network-interface test resultsbroadcast test got skipped due to not able to put any echo server within the same subnet to the target
|
@jamesbeyond Run the netsocket tests as well and provide the results. And when running, enable the extented tests macro as instructed in https://github.com/ARMmbed/mbed-os/blob/master/TESTS/netsocket/README.md#building-test-binaries |
Hi @kjbracey-arm, thanks for the review, your comments been addressed, please check again. I had read #8444, your comments totally make sense. However, I can't see those points in the Porting Guide. also seems the existing So are there plans to put these points in the Porting Gude? |
@@ -246,7 +232,7 @@ bool fvp_EMAC::power_up() | |||
mbed::mbed_event_queue()->call(mbed::callback(this, &fvp_EMAC::phy_task)); | |||
|
|||
/* Allow the PHY task to detect the initial link state and set up the proper flags */ | |||
osDelay(10); | |||
wait_ms(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because I know wait
APIs are being a pain, I'd actually prefer going straight to ThisThread::sleep_for(10)
here, given that you're relying on lots of other rtos
APIs anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait_ms()
been updated to ThisThread::sleep_for()
Hi @SeppoTakalo,
|
Good suggestion for the porting guide. #8444 and this are both exceptions to everything that's gone before - all the other ones are drivers for an EMAC built-in to the SoC, thus really are "TARGET_STM_EMAC" drivers or whatever - that's a generic driver that works on a subset of TARGET_STM platforms. I'd still like a truly freestanding component EMAC driver as an example, but both of these would need a bit more polish for configurability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As technically deep as I can get with this PR, LGTM
@kjbracey-arm Looks like your comments were addressed. Re-review? All good? |
/morph build |
Build : SUCCESSBuild number : 3604 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 3209 |
Test : FAILUREBuild number : 3381 |
In attempting to bundle this PR into the current rollup PR, this was not able to be automatically merged, and will require a rebase once the rollup PR, (primarily #8717) comes in. Once #8717 is in, all future PRs will require considerably less rebasing due to how a majority of the merge conficts have occurred Tl;dr: Once #8733 is in, this will need to be rebased. |
@jamesbeyond This needs a rebase. |
* move emac driver to COMPONENT folder * use mbed rtos C++ API instead of CMSIS API
Hi @cmonr, rebase finished, cheers! |
/morph build |
Build : SUCCESSBuild number : 3643 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 3252 |
Test : SUCCESSBuild number : 3424 |
Description
Enabled EMAC drivers for all Fast Models FVP MPS2 platforms
Pull request type
@SeppoTakalo