Skip to content

cpu/stm32: Fix & cleanup periph_eth#15193

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
maribu:stm32_eth_fix
Oct 11, 2020
Merged

cpu/stm32: Fix & cleanup periph_eth#15193
benpicco merged 2 commits intoRIOT-OS:masterfrom
maribu:stm32_eth_fix

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Oct 9, 2020

Contribution description

The function to check the link status of the STM32 Ethernet peripheral used the hard coded PHY address 0, rather than the configured address. This made me realize that the API of the MII register access functions sucks, as only one PHY is supported anyway.

  • Dropped the PHY address parameter in the MII register read and write functions
    • Instead, the configured PHY address is used
  • Updated all calls to it
  • Fixed the PHY address of the Nucleos
    • The reason why address 0 worked for reading the link status is that apparently 0 is the correct PHY address
    • The Ethernet on Nucleo-F767ZI works as good as before with PHY address 0 (with occasionally the PHY getting configured to 10 Mbps half-duplex while the MAC is configured at 100 Mbps full-duplex, resulting in cpu/stm32/eth, board/nucleo-f767z1: ethernet initialisation fails sometimes #13490 that is already present in master)

Testing procedure

E.g. flash examples/gnrc_networking

Issues/PRs references

None

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports labels Oct 9, 2020
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 9, 2020

@JannesVolkens: You have a Nucleo-F207ZG if I recall correctly. Could you test this PR on this board?

@bergzand: You have a Nucleo-F746ZG if I recall correctly. Could you test this PR?

@aabadie: Care to confirm my test results on the Nucleo-F767ZI?

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 9, 2020

BTW: Now that I know what is causing #13490 I should come up with a fix swiftly. But first let me split out the MII definitions from cpu/stm32/include/periph_cpu.h to a common utility header. The MII register specification is a standard used by most (all?) Ethernet network interfaces - we should have this as common header. E.g. Linux also has a common utility header for this, rather than reinventing the wheel for every Ethernet adapter.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 9, 2020

Fixed typo in commit message

@JannesVolkens
Copy link
Copy Markdown
Contributor

JannesVolkens commented Oct 9, 2020

Everything is working correctly on the nucleo-f207zg!

Output:
ifconfig with with an active link:

ifconfig
2020-10-09 12:36:11,367 #  ifconfig
2020-10-09 12:36:11,371 # Iface  5  HWaddr: 26:1B:10:41:03:29  Link: up 
2020-10-09 12:36:11,375 #           L2-PDU:1500  MTU:1500  HL:64  RTR  
2020-10-09 12:36:11,377 #           RTR_ADV  
2020-10-09 12:36:11,380 #           Source address length: 6
2020-10-09 12:36:11,383 #           Link type: wired
2020-10-09 12:36:11,388 #           inet6 addr: fe80::241b:10ff:fe41:329  scope: link  VAL
2020-10-09 12:36:11,391 #           inet6 group: ff02::2
2020-10-09 12:36:11,394 #           inet6 group: ff02::1
2020-10-09 12:36:11,397 #           inet6 group: ff02::1:ff41:329
2020-10-09 12:36:11,398 #           
2020-10-09 12:36:11,401 #           Statistics for Layer 2
2020-10-09 12:36:11,404 #             RX packets 7  bytes 1084
2020-10-09 12:36:11,409 #             TX packets 1 (Multicast: 1)  bytes 78
2020-10-09 12:36:11,412 #             TX succeeded 1 errors 0
2020-10-09 12:36:11,415 #           Statistics for IPv6
2020-10-09 12:36:11,418 #             RX packets 2  bytes 192
2020-10-09 12:36:11,422 #             TX packets 1 (Multicast: 1)  bytes 64
2020-10-09 12:36:11,425 #             TX succeeded 1 errors 0
2020-10-09 12:36:11,425 #

ifconfig without an active link:

ifconfig
2020-10-09 12:36:18,187 #  ifconfig
2020-10-09 12:36:18,191 # Iface  5  HWaddr: 26:1B:10:41:03:29  Link: down 
2020-10-09 12:36:18,195 #           L2-PDU:1500  MTU:1500  HL:64  RTR  
2020-10-09 12:36:18,197 #           RTR_ADV  
2020-10-09 12:36:18,200 #           Source address length: 6
2020-10-09 12:36:18,203 #           Link type: wired
2020-10-09 12:36:18,208 #           inet6 addr: fe80::241b:10ff:fe41:329  scope: link  VAL
2020-10-09 12:36:18,211 #           inet6 group: ff02::2
2020-10-09 12:36:18,214 #           inet6 group: ff02::1
2020-10-09 12:36:18,217 #           inet6 group: ff02::1:ff41:329
2020-10-09 12:36:18,218 #           
2020-10-09 12:36:18,221 #           Statistics for Layer 2
2020-10-09 12:36:18,224 #             RX packets 45  bytes 8782
2020-10-09 12:36:18,229 #             TX packets 4 (Multicast: 2)  bytes 328
2020-10-09 12:36:18,232 #             TX succeeded 4 errors 0
2020-10-09 12:36:18,235 #           Statistics for IPv6
2020-10-09 12:36:18,238 #             RX packets 39  bytes 7030
2020-10-09 12:36:18,242 #             TX packets 4 (Multicast: 2)  bytes 272
2020-10-09 12:36:18,245 #             TX succeeded 4 errors 0
2020-10-09 12:36:18,246 #

ping6 with an active link:

ping6 fe80::d941:e3fd:1e26:ad1b
2020-10-09 12:36:24,712 #  ping6 fe80::d941:e3fd:1e26:ad1b
2020-10-09 12:36:24,719 # 12 bytes from fe80::d941:e3fd:1e26:ad1b%5: icmp_seq=0 ttl=64 time=0.247 ms
2020-10-09 12:36:25,721 # 12 bytes from fe80::d941:e3fd:1e26:ad1b%5: icmp_seq=1 ttl=64 time=1.447 ms
2020-10-09 12:36:26,722 # 12 bytes from fe80::d941:e3fd:1e26:ad1b%5: icmp_seq=2 ttl=64 time=1.474 ms
2020-10-09 12:36:26,722 # 
2020-10-09 12:36:26,725 # --- fe80::d941:e3fd:1e26:ad1b PING statistics ---
2020-10-09 12:36:26,730 # 3 packets transmitted, 3 packets received, 0% packet loss
2020-10-09 12:36:26,734 # round-trip min/avg/max = 0.247/1.056/1.474 ms

ping6 without an active link:

ping6 fe80::d941:e3fd:1e26:ad1b
2020-10-09 12:36:29,184 #  ping6 fe80::d941:e3fd:1e26:ad1b
2020-10-09 12:36:32,184 # 
2020-10-09 12:36:32,188 # --- fe80::d941:e3fd:1e26:ad1b PING statistics ---
2020-10-09 12:36:32,194 # 3 packets transmitted, 0 packets received, 100% packet loss

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 9, 2020
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 9, 2020

Will this need a backport?

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 9, 2020

Will this need a backport?

Wouldn't hurt :-)

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Oct 9, 2020

Wouldn't hurt :-)

I agree that this fixes a bug. I'm fine with a backport for this. Thanks!

@benpicco benpicco added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 9, 2020
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 9, 2020 •
edited
Loading

Hmm, I think I should use uint16_t for the phy addr - even though 0-31 comfortably fits into uint8_t. Bit shift and integers of different width is a can of worms I might better not open

maribu added 2 commits October 9, 2020 20:20
@maribu
cpu/stm32: Fix/cleanup periph_eth
0e43c92 
The methods to read from / write to MII registers had an address argument to
allow specifying the PHY to communicate with. However, only a single PHY is
available on all boards supported and the driver is not able to operate with
multiple PHYs anyway - thus, drop this parameter for ease of use.

This fixes a bug in the _get_link_status() function, which used hard coded the
address 0; which might not be correct for all boards.
@maribu
boards/nucleo-{f207zg,f746zg,f767zi}: periph_conf
a30872b 
Fix PHY address in eth_config. It should be 0 for these boards, not 1. This is
why previously the link status read out worked with an hard code PHY address
0 before.

Some dubious references for 0 being the correct PHY address and not 1 (in
absence of proper references):

https://www.carminenoviello.com/2016/01/22/getting-started-stm32-nucleo-f746zg/
https://community.st.com/s/question/0D50X00009XkgfISAR/stm32f767-nucleo-ethernet-not-working
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 9, 2020

Hmm, I think I should use uint16_t for the phy addr - even though 0-31 comfortably fits into uint8_t. Bit shift and integers of different width is a can of worms I might better not open

Fixed. Test results remain valid: Due to integer promotion of uint8_t the result of the operations is still identical. But with uint16_t used, I do no longer need to recall details of the C specification to explain why the code is correct :-)

I agree that this fixes a bug. I'm fine with a backport for this. Thanks!

I propose to only backport the second commit and keep the hard coded phy address of 0 for the link status. All Nucleos supported by that branch use 0 as phy address (so the hard coded value is correct for all of them) and no new board support is going to be back-ported. Otherwise I would have to decide on whether to also backport the clean-up, or fix it completely differently in the stable branch.

This was referenced Oct 9, 2020
Merged
Merged
benpicco
benpicco approved these changes Oct 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty straightforward and @JannesVolkens confirmed it's working.

@benpicco benpicco merged commit 94e78cd into RIOT-OS:master Oct 11, 2020
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 12, 2020

Thanks!

@maribu maribu deleted the stm32_eth_fix branch October 12, 2020 06:43
@maribu maribu mentioned this pull request Oct 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@bergzand bergzand Awaiting requested review from bergzand

@JannesVolkens JannesVolkens Awaiting requested review from JannesVolkens

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

Assignees

No one assigned

Labels

Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maribu @JannesVolkens @benpicco @bergzand