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

Add EMAC driver for CM3DS #8444

Merged
merged 1 commit into from Nov 8, 2018

Conversation

Projects
None yet
7 participants
@kapi90
Contributor

kapi90 commented Oct 16, 2018

Description

This commit adds EMAC driver for CM3DS that uses an SMSC LAN 9220
Ethernet controller. To ensure proper operation, some methods
needed to be updated in the SMSC9220's native driver as well.
It passes all related Greentea tests, however when supervised by
the Python environment it tends to fail because of Timeout.

The current timeout is set to 1200s that seems to be a little bit short
to finish all test cases, the timeout happens towards the end of the
last test case.

Pull request type

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

@ashok-rao @SeppoTakalo @tamasolaszi @tkaman

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 16, 2018

Looks neat. A few thoughts:

  • The "ARM_EMAC" is a bit vague. This is an SMC9220 or whatever EMAC, so I'd suggest the thing is labelled as that.
  • In the same vein, it's not clear why a chunk of the driver code is inside the CM3DS directory - most of that is SMC9220 code, so should be under an SMC9220 label only.
  • I would imagine the only thing that is CM3DS/board specific is locating the device - IRQ pin assignments and static memory address. Can you set up a simple scheme so the "SMC9220_EMAC" picks that up from the specific target? Via defines maybe?
  • Or for ultimate flexibility, the EMAC would allow the pin/address info to be passed to its constructor, just as you pass pins to an ESP8266 or whatever. Then it can only be your EMAC::get_default_instance for the CM3DS that passed the board-specific info to the constructor.
  • The ethernet_api stuff is totally obsolete - I suggest dropping it.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-ipcore Oct 16, 2018

@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 16, 2018

Hi @kjbracey-arm

Thanks for your suggestions, I just wanted to reflect on them before doing any changes. Let me start backwards, Point 5 is pretty straightforward, I'll just drop ethernet_api.

Regarding point 1, it seems to me that the convention has been to put the target's manufacturer's name right under the emac-drivers directory. Therefore I would just create a directory named "TARGET_SMSC9220" under "TARGET_ARM_EMAC" and move the driver files there.

As for points 2,3&4: We have a separate abstraction layer within our targets in which we collect and configure their peripherals. Their native drivers live under the specific target/device/drivers directory, it's been a pattern for a while that we don't wanna break. So I would just leave the native driver related code as it is now, to keep our targets consistent.

What do you think ?

@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 16, 2018

Updated the PR based on @kjbracey-arm's comments. There has been an issue raised on the timeout, however, since then the problem has changed slightly and it's not the 600s constraint for the "EMAC unicast long" case, but rather the 1200s constraint on the whole test.

Link to the issue: #8200

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 17, 2018

I don't think you need the "TARGET_ARM_EMAC" directory at all. Or at least, the SMC9220 driver shouldn't be inside it. It's not an ARM part, and anyone could mount an SMSC9220 onto any board, if the SoC has a static peripheral interface.

Just emac-drivers/TARGET_SMSC9220 would do. Or possibly that should be emac-drivers/COMPONENT_SMSC9220. Since the rest of the EMAC drivers were put in, the "component" mechanism has appeared, and I'm currently confirming whether that should be being used for new EMACs.

The other ones are under manufacturer names because they're on-chip EMACs for that family of target SoCs, and are hence generic drivers for a large chunk of that family. And the "_EMAC" suffix is really just to distinguish them - if it was "TARGET_Freescale" it would attempt compilation on all Freescale targets, but not all Freescale targets actually have the necessary EMAC and hence EMAC SDK headers in their HAL.

As to the location of the rest of the code - my primary objection is to it being inside TARGET_ARM_SSG/TARGET_CM3DS_MPS2. If anyone wants to use the part on another board, they'd have to copy-and-paste it over. I don't know that that's likely here, but it's a bad pattern. We have hit that problem, eg with Ublox having modem code inside a target, then finding they suddenly needed it in a different target when they changed SoC. At the time we put it in targets/TARGET_Ublox/TARGET_Ublox_modemname, and that was added as an extra_label, but I think the modern mechanism would be to put it in components/ethernet/COMPONENT_SMSC9220 or similar. (@sg- ?)

I don't think there's a pressing need to split the "C++ EMAC" stuff totally away from the "low level" stuff - I'd be happy with the low-level stuff living in a subdirectory of netsocket/emac-drivers/COMPONENT_SMSC9220 - in mbed OS its only user would be the EMAC implementation. But I'm fine with it as long as it doesn't end up in a target or board directory.

Then board-specific config for EMAC+lower-level driver can be done in one of two ways:

  • EMAC code picks up board-specific info from defines. Any target adding COMPONENT_SMSC9220 must provide those defines.
  • EMAC constructor takes info on address+IRQ pins in its constructor. Boards with SMSC9220 provide a EMAC::get_default_instance that pass necessary info to the constructor.

I and I believe @sg- favour the latter, as this is a distinct component, not something inside an SoC.

@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 17, 2018

Hi @kjbracey-arm

I understand that you are striving for reusability and less duplication and I really do appreciate your comments. However, our concern with your proposal is that this is not a canonical solution for this IP, it does not come from the manufacturer, this is only our own native driver implementation for the SMSC LAN9220 Ethernet controller. This means that there could be more than one implementation, e.g. some party wants to have a slightly different mode operation, different subset of functions, or Microchip decides to release an official driver.

By having the native driver under our target's directory we have full control over its version, that we would lose if it was stored somewhere in a central directory, possibly breaking our code after modifications. So this is an "ARM implementation" of the ethernet driver, and I have to go further here, since the EMAC driver is built on the native driver, it is implementation dependent as well, which I think justifies its placement under TARGET_ARM_EMAC.

As for copying sources between targets, we already have an internal repository provisioning solution, which builds our repos based on its JSON descriptor, checking out and copying the specified file/driver versions from our internal repos.

I hope I could share somewhat our perspective on this matter, please let me know if I can be your further assistance.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 17, 2018

We already have quite a few drivers already meeting the same description - developed by ARM for a third-party device (eg Atmel RF233 802.15.4, ESP8266 Wi-Fi), so this is really not a special case.

But I see your point that if there is no plan or intent to actually support the device for any purpose other than this CM3DS at present in mbed OS, putting it in that target directory and treating it as part of the target HAL stops anyone from attempting to use it, and makes clear the limited scope of support.

So I guess this can stay as-is, but we would need to revisit in the unlikely event someone came along asking for mbed OS support for the chip on another board.

In this model, I don't think you need the "TARGET_ARM_EMAC" extra label - you can just have TARGET_ARM_SSG/COMPONENT_SMSC9220 (or TARGET_SMSC9220). The _EMAC labels were to activate generic drivers for a limited set of targets; as this doesn't have a generic driver and you have a separate specific label, can just use the company name.

kapi90 added a commit to kapi90/mbed-os that referenced this pull request Oct 17, 2018

Add EMAC driver to CM3DS platform
Final iteration based on mbed team's PR comments.
PR link: ARMmbed#8444

Change-Id: Ifed46c7578f591e107311d541cca877eb38b9518
Signed-off-by: Bence Kaposzta <bence.kaposzta@arm.com>
@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 17, 2018

@kjbracey-arm thanks again for your comments, I've applied the modifications based on your latest suggestion.

@@ -58,4 +58,8 @@
#define ARM_UART3
#define ARM_UART4
/* SMSC9220 Ethernet */
#define SMSC9220_ETH
#define SMSC9220_Ethernet_Interrupt_Handler ETHERNET_IRQHandler

This comment has been minimized.

@0xc0170

0xc0170 Oct 17, 2018

Member

Why is this not using dynamic vectors (NVIC Set/Get vector) ?

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 18, 2018

Contributor

Yes, this will pull in a chunk of Ethernet driver code, even when the application isn't using networking. Even if ROM is not a concern for the CM3DS, it's a bad example for in-tree.

#define SMSC9220_ETH_MTU_SIZE 1500U
#define SMSC9220_ETH_IF_NAME "smsc9220"
#endif /* SMSC9220_EMAC_CONFIG_H */

This comment has been minimized.

@0xc0170

0xc0170 Oct 17, 2018

Member

new line at the end of files (github shows its not here?)

bool prev_link_status;
int link_status_task_handle;
uint8_t hwaddr[SMSC9220_HWADDR_SIZE];
EMACMemoryManager *memory_manager;

This comment has been minimized.

@0xc0170

0xc0170 Oct 17, 2018

Member

private variable should start with _ prefix (see drivers/SPI.h for instance)

@kjbracey-arm

I'm pretty happy with this, but a few comments.

@@ -58,4 +58,8 @@
#define ARM_UART3
#define ARM_UART4
/* SMSC9220 Ethernet */
#define SMSC9220_ETH
#define SMSC9220_Ethernet_Interrupt_Handler ETHERNET_IRQHandler

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 18, 2018

Contributor

Yes, this will pull in a chunk of Ethernet driver code, even when the application isn't using networking. Even if ROM is not a concern for the CM3DS, it's a bad example for in-tree.

int stacksize, osPriority_t priority,
mbed_rtos_storage_thread_t *thread_cb)
{
osThreadAttr_t attr = {0};

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 18, 2018

Contributor

We prefer use of the C++ wrappers to direct CMSIS-RTOS use, so Thread class and ThisThread namespace rather than osThreadxxx calls.

@@ -2881,13 +2881,16 @@
"inherits": ["ARM_IOTSS_Target"],
"core": "Cortex-M3",
"supported_toolchains": ["ARM", "GCC_ARM", "IAR"],
"extra_labels": ["ARM_SSG", "CM3DS_MPS2"],
"extra_labels": ["ARM_SSG", "CM3DS_MPS2", "SMSC9220"],

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 18, 2018

Contributor

If using the component directory (which I've confirmed is correct thing to do), you should have this instead as

"components": ["SMSC9220"],
@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 19, 2018

Hi @kjbracey-arm

I've applied some changes again based on the latest comments, however at the Ethernet's interrupt handler I used a slightly different approach, and added guards using the COMPONENT_SMSC9220 define coming from the target.json

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 19, 2018

The COMPONENT test doesn't achieve what we were aiming for - excluding the Ethernet driver when the application isn't using it. The COMPONENT flag will always be on.

Is there a reason you can't do this dynamically? The sequence is something like this seen in the LPC driver

NVIC_SetVector(ENET_IRQn, (uint32_t)LPC17xxEthernetHandler);
NVIC_SetPriority(ENET_IRQn, ((0x01 << 3) | 0x01));
NVIC_EnableIRQ(ENET_IRQn);
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 19, 2018

Ah, does the CM3DS not have its vector table in RAM, so you can't NVIC_SetVector? Okay, that's a problem, but a system-wide one.

I suggest you leave that for now then.

emac_mem_buf_t *low_level_input();
static void receiver_thread_function(void* params);
mbed_rtos_storage_thread_t _thread_cb;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 19, 2018

Contributor

No longer needed. But Thread should be here?

#endif
static const struct smsc9220_eth_dev_t *dev = &SMSC9220_ETH_DEV;
/* Processing thread */
static Thread receiver_thread(LINK_STATUS_THREAD_PRIORITY,

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 19, 2018

Contributor

This would be constructed and included in image even when SMSC9220_EMAC is not not used. Can't you put it inside your EMAC object?

If it must be static, use SingletonPtr to avoid construction and inclusion.

uint32_t message_length = 0;
message_length = smsc9220_peek_next_packet_size(dev);
if (message_length == 0){

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 19, 2018

Contributor

Formatting - missing

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 24, 2018

Contributor

if (message_length == 0) {

bool SMSC9220_EMAC::link_out(emac_mem_buf_t *buf)
{
if(buf == NULL){

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 19, 2018

Contributor

Formatting

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 24, 2018

Contributor

if (buf == NULL) {

void SMSC9220_EMAC::set_hwaddr(const uint8_t *addr)
{
if(!addr) {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 19, 2018

Contributor

Formatting

This comment has been minimized.

@kapi90

kapi90 Oct 24, 2018

Contributor

not sure what to change here

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 24, 2018

Contributor

if (!addr) {

@cmonr cmonr added needs: work and removed needs: review labels Oct 25, 2018

Initial review comments addressed. Please re-review.

@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 29, 2018

Hi @0xc0170 I dropped the ethernet_api.c based on @kjbracey-arm's suggestion: "The ethernet_api stuff is totally obsolete - I suggest dropping it." and based on the error messages I suspect that this may have caused the build failures in this case.

I've copied out the part that I thought relevant, what do you think ?

Link: /usr/local/ARM_Compiler_5.06u3/bin/armlink --via /builds/ws/mbed-os-build-matrix/3491/ARM_CM3DS_MPS2_ARM/sources/mbed-os/BUILD/tests/ARM_CM3DS_MPS2/ARM/TESTS/mbed_platform/stats_thread/.link_options.txt
[DEBUG] Return: 1
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_address (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_free (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_init (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_link (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_read (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_receive (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_send (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_set_link (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Error: L6218E: Undefined symbol ethernet_write (referred from BUILD/tests/ARM_CM3DS_MPS2/ARM/drivers/Ethernet.o).
[DEBUG] Errors: Finished: 0 information, 0 warning and 9 error messages.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 29, 2018

You need to remove the "ETHERNET" representing the obsolete API from your target's "device_has".

This commit adds EMAC driver for CM3DS that uses an SMSC LAN 9220
Ethernet controller. To ensure proper operation, some methods
needed to be updated in the SMSC9220's native driver as well.
It passes all related Greentea tests, however when supervised by
the Python environment it tends to fail because of Timeout.

The current timeout is set to 1200s that seems to be a little bit short
to finish all test cases, the timeout happens towards the end of the
last test case.

Change-Id: I914608c34828b493a80e133cd132537a297bfc84
Signed-off-by: Bence Kaposzta <bence.kaposzta@arm.com>

@kapi90 kapi90 force-pushed the kapi90:cm3ds_emac branch from 6a6287b to 9e6b124 Oct 29, 2018

@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 29, 2018

Removed "ETHERNET" label from "device_has" section

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Oct 31, 2018

@kapi90

The current timeout is set to 1200s that seems to be a little bit short
to finish all test cases

So how long is enough then?

@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 31, 2018

@SeppoTakalo

I've measured 1362.85 seconds, so 1400 would sound legit I guess...

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Oct 31, 2018

@kapi90 Would this #8604 help then?

@kapi90

This comment has been minimized.

Contributor

kapi90 commented Oct 31, 2018

@SeppoTakalo Definitely, thanks!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 7, 2018

The extending timeout PR landed, this can proceed now

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 7, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Nov 7, 2018

More IOExceptions. Restarting.
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Nov 8, 2018

Test failure does not appear to be caused by glitch.
Will restart when able.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Nov 8, 2018

Note: This PR is now a part of a rollup PR (#8675).

Jenkins CI export nodes experienced many drops throughout the day causing false failures. In an attempt to get those PRs through CI, while keeping CI load low, several PRs have been bundled into a single rollup PR.

If any more commits are made in this PR, this PR will remain open and have to go through CI on its own.

@0xc0170 0xc0170 referenced this pull request Nov 8, 2018

Merged

Rollup PR #8676

@cmonr cmonr merged commit 9e6b124 into ARMmbed:master Nov 8, 2018

13 of 15 checks passed

ci-morph-exporter build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
ci-morph-build build completed
Details
ci-morph-mbed2-build build completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 547 files (+2 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9141 cycles (-783 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment