Skip to content
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

dts: ethernet: rework how we deal with mac-address setting #24651

Merged
merged 5 commits into from
May 9, 2020

Conversation

galak
Copy link
Collaborator

@galak galak commented Apr 23, 2020

Rework the ethernet drivers to determine how they set the mac-address. In the majority of cases with just utilize devicetree and a combination of 'local-mac-address' property and 'zephyr,random-mac-address' property.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 23, 2020

All checks passed.

checkpatch (informational only, not a failure)

-:457: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#457: FILE: drivers/ethernet/eth_mcux.c:882:
+    DT_INST_PROP(1, zephyr_random_mac_address)$

-:477: WARNING:LONG_LINE: line over 80 characters
#477: FILE: drivers/ethernet/eth_mcux.c:891:
+    DT_HAS_NODE_STATUS_OKAY(DT_DRV_INST(1)) && !DT_INST_NODE_HAS_PROP(1, local_mac_address)

-:477: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#477: FILE: drivers/ethernet/eth_mcux.c:891:
+    DT_HAS_NODE_STATUS_OKAY(DT_DRV_INST(1)) && !DT_INST_NODE_HAS_PROP(1, local_mac_address)$

-:486: WARNING:LONG_LINE: line over 80 characters
#486: FILE: drivers/ethernet/eth_mcux.c:910:
+#if DT_HAS_NODE_STATUS_OKAY(DT_DRV_INST(1)) && !DT_INST_NODE_HAS_PROP(1, local_mac_address)

- total: 0 errors, 4 warnings, 578 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

LGTM

mac_addr[3] = entropy >> 8;
mac_addr[4] = entropy >> 16;
/* Locally administered, unicast */
mac_addr[5] = ((entropy >> 0) & 0xfc) | 0x02;
Copy link
Member

Choose a reason for hiding this comment

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

The liteeth driver seems to have set the LAA bit to wrong byte earlier.

#if defined(CONFIG_ETH_MCUX_0_UNIQUE_MAC) || \
defined(CONFIG_ETH_MCUX_1_UNIQUE_MAC)
#if !DT_INST_NODE_HAS_PROP(0, local_mac_address) || \
DT_HAS_NODE(DT_DRV_INST(1)) && !DT_INST_NODE_HAS_PROP(1, local_mac_address)
Copy link
Member

Choose a reason for hiding this comment

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

Why does generating eth0 depend on instance 1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because generate_eth1_unique_mac calls generate_eth0_unique_mac. And we technically could have a case in which local-mac-address is set on inst0 and not on inst1.

@pabigot pabigot self-requested a review April 25, 2020 11:31
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

A few comments that I'd like considered. If they don't make sense I'll remove the request changes, though I am specifically concerned about a change in behavior to local-mac-address = [000000000000] in existing overlays.

dts/bindings/ethernet/ethernet.yaml Outdated Show resolved Hide resolved
drivers/ethernet/eth.h Show resolved Hide resolved
dts/arm/nxp/nxp_k6x.dtsi Show resolved Hide resolved
drivers/ethernet/eth_mcux.c Outdated Show resolved Hide resolved
@nandojve nandojve mentioned this pull request May 7, 2020
61 tasks
Rather than having each driver have its own slightly different way of
generating a random mac address, add a helper function that they all can
call so we do it one way.

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
dts/bindings/ethernet/ethernet.yaml Outdated Show resolved Hide resolved
galak added 4 commits May 8, 2020 11:20
Add definition of zephyr,random-mac-address property that conveys to a
driver to utilize a random MAC address if the driver supports this
feature.

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
Utilize the devicetree property "zephyr,random-mac-address" to determine
if a driver should use a random mac address and remove the associated
Kconfig options that enabled this feature.

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
Move from a Kconfig to select/initialize the MAC address to using the
"local-mac-address" property in devicetree.  If the property is set the
drivers will initialize the mac-address from the devicetree (unless the
mac address is all 0's).  The MAC address might get overwritten by
either a driver specific means or by the setting of
"zephyr,random-mac-address" in the devicetree.

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
Instead of having a Kconfig property, if there is no local-mac-address
property in the devicetree than we'll generate a unique MAC address
based on unique ID registers on the SoC.

We remove the local-mac-address properties in the SoC dtsi files to
match the default behavior that existed before (ie, unique MAC address)

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
@carlescufi carlescufi merged commit df56ce3 into zephyrproject-rtos:master May 9, 2020
@galak galak deleted the eth-mac branch May 9, 2020 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants