drivers: ethernet: xlnx_gem: cleanup DT dependencies

[venodela]

Summary

The current ETH_XLNX_GEM_DEV_CONFIG macro in the Xilinx GEM Ethernet driver relies heavily on DeviceTree (DT_INST_PROP) for configuration values that should instead come from driver defaults, or Kconfig. This makes the driver harder to maintain, less portable, and prone to misconfiguration.
Keeping them in DT unnecessarily bloats the DTS files.

https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/ethernet/eth_xlnx_gem_priv.h#L709

https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/ethernet/eth_xlnx_gem_priv.h#L418

Describe the solution you'd like

Refactor the struct eth_xlnx_gem_dev_cfg initialization to minimize DT dependency, align defaults with Linux, and move board-agnostic values into driver defaults or Kconfig.

Keep from DT

.mdio_dev → ETH_XLNX_GEM_MDIO_DEV(port)

.phy_dev → ETH_XLNX_GEM_PHY_DEV(port)

.base_addr → DT_REG_ADDR_BY_IDX(DT_INST(port, xlnx_gem), 0)

.pll_clock_frequency → DT_INST_PROP(port, clock_frequency)

.clk_ctrl_reg_address → DT_REG_ADDR_BY_IDX(DT_INST(port, xlnx_gem), 1)

.enable_sgmii_mode → OK from DT but just remove and get this from phy-mode

Driver only (not DT)

Defaults or Kconfig options; DT properties should be removed:

.config_func → driver internal IRQ config (already handled)

.defer_rxp_to_queue

.defer_txd_to_queue

.amba_dbus_width → read from GEM DCFG1 register; or default = 64 from KCONFIG

.ahb_burst_length → default = 16; Kconfig option

.hw_rx_buffer_size →

.hw_rx_buffer_offset →

.rx_bd_count / .tx_bd_count → default = 32; Kconfig option.

.rx_buffer_size / .tx_buffer_size →

.ignore_ipg_rxer → default = 0; set to 1 only if RGMII half-duplex, no DT property

.enable_ipg_stretch → default = 0; Kconfig option if needed

.enable_rx_chksum_offload / .enable_tx_chksum_offload → default = 1; Kconfig

.discard_rx_fcs → default = 1; Kconfig option

.discard_rx_length_errors → default = 1; Kconfig option

.enable_pause → default = 0;

.enable_tbi → default = 0;

.ext_addr_match → default = 0

.enable_1536_frames → default = 1

.enable_ucast_hash → default = 0

.enable_mcast_hash → default = 0;

.disable_bcast → default = 0;

.copy_all_frames → default = 0; Kconfig option for promiscuous mode

.discard_non_vlan → default = 0; Kconfig option

.tx_buffer_size_full → default = 1; Kconfig option if needed

Drop entirely (debug/unsupported, should never be in driver)

.disable_reject_nsp

.disable_reject_fcs_crc_errors

.disable_pause_copy

.disc_rx_ahb_unavail

Endianness config

.enable_ahb_packet_endian_swap / .enable_ahb_md_endian_swap → set defaults based on CPU architecture

Alternatives

No response

Additional Context

No response

[venodela]

@HariniKatakamX @ibirnbaum

[venodela]

FYI: @michalsimek

[ibirnbaum]

Regarding the debug/unsupported items: fine by me, that's a quick fix. However, could you maybe shed some light on what those bits were supposed to do / under which circumstances they could have been useful / why they ended up in NWCFG?

Endianess config: admittedly, it's unlikely that we're ever going to see a big endian system using the GEM again ever since the PowerPC has disappeared along with the older generation Virtexes.
AMBA data bus width: fine by me, if that register is correctly implemented in the Zynq-7000 as well. Haven't checked the TRM yet.

.enable_rx_chksum_offload / .enable_tx_chksum_offload: have just been inverted to active-disable with #95435 with enabled now being the default.

Regarding all other points, in particular buffer count, buffer sizes, as mentioned above checksumming disable, enable 1536 byte frames etc.: please be aware that converting any item to a single Kconfig option makes this setting global for all GEM interfaces. Having separate Kconfig items for GEM[0..3] each for each configurable parameter moved there from the device tree

• just moves the bloat to the driver's Kconfig,
• would result in Kconfig options that are not required/valid for the Zynq (anything related to GEM2/3)

Also, this is an approach that doesn't work at all for IP-based peripherals. Take the AXI GPIO as the simplest supported example: the instance count can be [0..n]. This can't be represented in Kconfig as per-instance configuration.

I disagree when you call those items board agnostic. It probably applies to some extent to eval boards that are used to test all kinds of things, however, in my work context, we usually design a board for a specific product and a specific purpose, i.e., there's a direct relation with one particular firmware that runs on it and nothing else. We make a lot of products where one Ethernet interface is used for regular Ethernet stuff, providing access to a web server etc., while the other interface is used for an Ethernet-based industrial field bus such as EtherCAT or Profinet. Those have completely different requirements and behaviours which reflect directly into the hardware configuration (admittedly usually more to the PHY side of things, however, for example, Profinet has a maximum payload size of 1440 bytes - so why waste buffer memory?) and are sometimes relevant for product certification, so if one of those two interfaces requires a different approach to checksums, buffer sizes etc., I need those configurable per instance, and as those are hardware-related properties and not properties related to which code is executed in the driver or not, from my point of view, those settings belong into the DT.

You're making suggestions regarding default values, if those match the needs of 95% of all developers and users, then they won't need to tinker with them in their board DT and the defaults in the SoC DT will remain untouched and will remain the single place where they need to be maintained. I don't see how the number of attributes for which defaults are given in a single place make things more error prone or less maintainable. Still, those settings should always be modifiable per interface and this doesn't reflect well in Kconfig.

It is actually taking away the flexibility of per-instance configuration of hardware parameters that makes the driver harder to maintain, less portable and more prone to misconfiguration when you want to use Zephyr mainline code as the basis, because when you need per-instance configuration that's no longer there, you start applying patches, forking the driver into a module with a different DT compatible, deleting the default nodes from the DT in order to replace them with your patched version - I've been there and it's a mess.

The current ETH_XLNX_GEM_DEV_CONFIG macro in the Xilinx GEM Ethernet driver relies heavily on DeviceTree (DT_INST_PROP)

That's how Zephyr drivers generally obtain their per-instance hardware configuration parameters.

for configuration values that should instead come from driver defaults, or Kconfig

In summary, I disagree with that statement as Kconfig is for the software parameterization side of things (feature inclusion = source code inclusion, conditional code execution in places like the kernel, the network stack etc.) and it's not fit for per-instance hardware configuration. Making things like buffer sizes, checksum offloading etc. global for all 2/4 interfaces at once is a no-go to me. From my point of view, if there's a set of defaults that will do for most people, there's one single well defined place to put those and to maintain those - the SoC DT -, but configurability must be provided as there's always someone out there whose needs you don't know about. To give you one really zany example I've worked on: AFDX, an Ethernet-based protocol from the aerospace sector. It's IPv4/UDP, but the Ethernet address tuple basically identifies a virtual wire, the IP addresses and the UDP port are abused for service and message ID and while the Ethernet frame and the IP header must be checksummed, the UDP checksum must always be 0. So no checksum offloading for the respective interface, still, I want checksum offloading on the management interface that's regular Ethernet.

align defaults with Linux

... and the point of doing so in a system that is not Linux is? In a lot of cases, relevant hardware configuration information can be transported using the same items and identifiers as in the Linux device tree, and I'll use those if they serve the intended purpose, but there's more than one area where Zephyr's DT structure has diverged and there's more coming. Have a look at #94679, for example. If the new clock management is implemened, clock configuration in the DT will likely not look like how Linux does it at all. The whole MDIO thing is another example: having an MDIO node nested within the MAC node as is standard on Linux only works in Zephyr by using a workaround for the device initialization priorities (and in one instance, NXP has found a really dirty way to bypass that issue, but I'm not telling on them...). While a separate MDIO node solves that problem, I'll have to make the nested approach work for multiple reasons, however, the result may well have been something that diverges from the Linux approach.

In summary, I'm in for using existing bindings if they suit everyone's needs because Zephyr doesn't offer any parameterization options on top. However, I wouldn't consider the existing Linux DT the gold standard for the reasons metioned above. Things like buffer count and buffer sizes are hardware properties that are relevant in systems where you're running on resource constrained embedded targets where resources like memory have to be budgeted. If Linux doesn't offer those configuration options, tough luck. If it's relevant to the kind of targets Zephyr runs on, and it's a per-instance hardware configuration item, then it belongs into the DT.

I'll have a look into the items mentioned above that serve no actual purpose or that can be obtained at compile time or run time. That should be a quick fix.

[HariniKatakamX]

@ibirnbaum Thanks for the response. I understand that allowing certain configurations per instance makes more sense than making them driver specific. If there are such parameters, we should keep them in the DT. We will also explore other options to make them user-configurable, when they are not HW dependent.
On buffer sizes or ring sizes on a resource constrained system, wouldn't it make sense to reduce the size across the board?
And we agree that speed, duplexity, flow control and certain other HW specific options need to be controlled from DT or PHY autonegotiation.
However, for options that are inside the IP (and may only differ from SoC to SoC), DT properties may be redundant. Defaults can be established for the device compatible string and also design_cfgX registers can be used to read capability. Defining these as DT puts the onus on user to define working defaults where, like you mentioned, a large % may not ever care to change the configuration.
Could you please advise if that's agreeable?

Yes, alignment with Linux DT may not always be possible. We do have a Baremetal driver for this from AMD where there's a smaller set of user configurable options that customers have used over the years that might make more sense.
cc @venodela to comment and suggest an updated list
cc @michalsimek

[michalsimek]

I also don't think that moving things to Kconfig is the good direction. If there are some options which can be read from HW please send that as separate patch and look at it on case by case basis.

[venodela]

Hi @ibirnbaum

Regarding the 'amba-ahb-dbus-width' property in zynqmp.dtsi:
amba-ahb-dbus-width = <XLNX_GEM_AMBA_AHB_DBUS_WIDTH_32BIT>;
https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/arm/xilinx/zynqmp.dtsi#L194

I’m planning to enhance this logic following the Linux driver’s approach. In Linux, the driver reads the Design Configuration Register (DCFG1) to determine the supported bus width, and then configures the data_bus_width[22:21] field in the network configuration register accordingly. https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/cadence/macb_main.c#L2737

If we take this approach, we wouldn’t need to keep the 'amba-ahb-dbus-width' property in the device tree.

Based on my understanding, in the ZCU102 default Vivado design where GEM3 is configured, the data_bus_width[22:21] field in the network configuration register should be set to XLNX_GEM_AMBA_AHB_DBUS_WIDTH_64BIT.

I’ll get back with clarifications on the other properties.

@HariniKatakamX

[ibirnbaum]

@venodela I prepared the acquisition of the data bus width from the design config register over the weekend. PR coming up in a few minutes.

[michalsimek]

Just for the record it is #96364 .