net: mdio: an MDIO device driver for the Airoha EN8811H
Subsystems (4)
cover net: mdio: an MDIO device driver for the Airoha EN8811H
The Airoha EN8811H answers its PHY ID from power-on, but it is an MD32 microcontroller until the host loads firmware into its volatile RAM. Today one driver owns both roles: the PHY driver downloads the firmware from .probe(), which works only when the files are readable by then and leaves the reset line in the hands of a PHY node whose detach wipes what was loaded.
Describe the chip instead as an MDIO device that owns the download and the reset, and publish the PHY on a child pass-through bus once the firmware runs. Patch 1 adds the binding. Patch 2 adds mdiodev_lock(), mdiodev_lock_nested() and mdiodev_unlock(), the names Andrew asked for in [1], for code that holds the bus lock across a burst without a phy_device. Patches 3-5 move the download into the shared library, typed on the MDIO device, and teach it to skip a download when the MD32 already runs firmware - which is what lets the two drivers coexist whichever runs first. Patches 6-7 add the driver and its bus.
This is one half of an RFC last posted whole as v2 [2]. The other half teaches phylink to keep a port whose PHY is expected to probe late,
under a new phy-needs-host-firmware property; it is posted alongside
this one at https://lore.kernel.org/r/[email protected]/ . The halves touch no common file and go to different reviewers, so they are apart. They are
not alternatives: the phylink half alone carries a board whose chip
answers its ID before firmware and whose PHY driver is a module, and
this half buys the cases that are not that - chips mute before
firmware, a built-in PHY driver whose probe fails once on missing files
and is never retried, and reset ownership. The shape also admits a quad package, one MCU serving four PHYs; this driver passes one address
through and does not try.
There is a smaller way to do the EN8811H alone, and I did not take it:
keep the PHY driver, have it poll for its firmware from a workqueue, and hold the link down until the download is done. It fails on the cases above. A chip that is mute before firmware gets no phy_device at scan, so no PHY driver ever probes it. A PHY node that owns reset-gpios has that line asserted by phy_detach(), which erases the firmware, and the driver cannot refuse that from inside phylib. And once the firmware is up, nothing in phylib re-runs the initialisation the driver skipped at probe; the driver would be doing that itself, from a workqueue, under locks phylib expects to own.
Publishing the PHY on a child bus splits the two agents that drive this chip onto two mutexes, so the driver holds the child bus lock around its own access and the library beneath takes the parent one level down. Patch 7 has the mechanism. Two choices in it are worth your opinion:
the level is a bool and not an enum mdio_mutex_lock_class, because the helpers it picks between are the names Andrew asked for in [1], and the parameter arrives with the patch that creates the second level so it never sits unused. I can go the other way on either.
One shape the series does not support, though nothing rejects it: the pass-through bus reads its parent at MDIO_MUTEX_NESTED, the same subclass a DSA user bus takes for the switch's own parent. An MCU sitting on such a bus gives lockdep three nesting levels of one class across two subclasses, and a recursive-locking report. The mutexes are distinct, so that is a complaint and not a demonstrated deadlock, and the driver neither detects nor refuses the topology. The enum's remaining value, MDIO_MUTEX_MUX, belongs to mdio-mux, so a third level wants a new one - not a decision this series should make on its own.
Andrew asked whether any board with this chip sits on a bus without direct Clause 45. I found none. The two in-tree device trees that carry it - mt7981b-openwrt-one by compatible, mt7986a-bananapi-bpi-r3-mini by bare PHY nodes at the chip's addresses - put it on mtk_eth_soc, and out of tree it hangs off one of four controllers: mtk_eth_soc, the MT7530 DSA bus on EN7581, mdio-airoha on AN7583, or the SiFlower xgmac. All four set read_c45, and none of those boards reaches the chip through an mdio-mux or a bitbanged bus.
The status poll keeps the Clause 22 indirection, which patch 4 explains. It is a preference and not a necessity: the chip does answer Clause 45 before its firmware, and the board readings below say what it answered.
System sleep is handled but untested, since this board gives no way to cut power to the PHY. A power-cutting suspend wipes the MD32, so .resume repeats the reset decision and the download in line, because the child PHY's own resume calls phy_init_hw() immediately after and needs the firmware by then. request_firmware_direct() already caches what it loaded, so the explicit registration exists for the adopt path, where no file was ever read and the cache would otherwise hold nothing. A resume that cannot reload restores the chip and not the port; question 3 says why.
Tearing the MDIO device down while its child PHY is attached crashes:mdiobus_unregister() removes the PHY's driver without freeing the interrupt its consumer requested, so the next interrupt dereferences phydev->drv after phy_remove() cleared it. No open or close is needed. Module unload cannot reach it, because attaching a PHY takes a reference on the bus owner, and sysfs unbind cannot, because the attributes are suppressed. Removing the parent MDIO controller still can; question 3 again.
Tested on an MT7981B board (MT7531 switch, EN8811H on a 2500base-x port), warm boots only - I have no remote way to cut power. The board runs OpenWrt, so what booted is these patches backported onto its 6.18 tree, not the mailed text byte-for-byte. What the board showed:
- download and handover on the normal boot path: U-Boot leaves the MD32 in its bootloader on every reboot here, so each boot runs the driver's pulse-reset and download; ~144KB lands in about two seconds, which is what 73728 Clause 22 frames cost at the 2.5 MHz MDC these boards default to; the driver reports the firmware it loaded at 6.33 s, and the PHY driver finds that firmware already running at 6.39 s and skips its own download through the shared check
- the PHY is attached at 7.10 s through the child bus with irq=15 from DT and not PHY_POLL; the port reaches forwarding and the link comes up at 20.1 s, and the interrupt line counts link events across forced renegotiations
- Clause 45 before firmware: with the MD32 never programmed, read on the parent bus while the child bus does not exist, the PMA/PMD identifier is 0x03a2, matching the Clause 22 pair 0x03a2/0xa411 at the same address, and the vendor status reads 0x0000 where a silent chip would float to 0xffff
Two of the v1 review points [3] went a different way than asked, and each gets a sentence rather than silence. The shared header went to include/linux/mdio/ beside the other MDIO driver headers: include/net/phy/ does not exist, and include/linux/phy/ is the generic PHY framework. And the copy of parent->irq[addr] onto the child bus was in v2 and is gone from v3. A bus's irq[] starts as PHY_POLL from mdiobus_register(), and the DT path fills an entry only when it registers a PHY at that address, from the PHY node's own interrupts property. The MCU node is not a PHY, so the parent's entry at its address stays PHY_POLL, while the child bus gets its own entry filled from the child PHY node when that bus is registered. The copy carried PHY_POLL over a value the child fills correctly on its own, and the board's irq=15 comes from the child node either way.
The base matters for patch 5: it makes the MCU restart on the adoption path, and commit 03b4702fc5e3 ("net: phy: air_en8811h: move LED GPIO configuration to config_init") with commit 3498acda6b68 ("net: phy:air_en8811h: restore AN8811HB LED GPIO after MCU restart") moved the LED GPIO enable to the end of config_init, after that restart. On an older base the restart would have cleared the buckpbus-mapped GPIO state with nothing to re-enable it. The firmware size check landed as commit daf0972d38e9 ("net: phy: air_en8811h: refuse a firmware blob that is not a multiple of 4"), and patch 4 carries it into the library with the write loop it guards.
What I am asking:
1. The binding lets software timing pick the topology: the same chip on the same board is a plain PHY node when its firmware is in the bootloader, and an MCU with a child bus when the firmware arrives with the rootfs. Move the files into an initramfs and the recommended description changes without a wire moving. The hardware argument that does hold is narrower - the reset line belongs to the MD32 core rather than to the PHY the firmware creates, and a PHY node that owns it wipes the firmware on detach - and it argues for the MCU node, not for the child bus. If the child bus has to be justified by hardware too, I do not have that argument, and the honest options are to describe the chip one way always, or to say plainly in the binding that this is a driver model choice. Which would you rather see?
2. A third nesting level has no subclass, as above. Should this series add one?
3. Removing the parent MDIO controller reaches this driver's remove with the child PHY attached, and nothing pins the parent's owner. Closing it wants a way for a bus to tell its PHYs' consumers to let go, which is a phy-core change. The failed-resume path above wants the same channel for "re-initialise" rather than "let go", so that is two uses for one mechanism. Is it worth doing here?
4. MDIO_AIROHA_EN8811H sits in the "MDIO controller drivers" menu, though it drives a device on a bus and not a controller. The mdio-mux entries set the precedent, but say if you want it elsewhere.
5. The child bus accepts exactly one address, the one this node occupies on the parent, and I do not think the schema can say so. What it does say now is that a child named ethernet-phy@N is validated as one, and that it must not carry reset-gpios or its timings - the whole reason this node exists is that a PHY-node reset is what phy_detach() asserts, and it erases the firmware. What it cannot say is that a differently named child is refused:
mdio.yaml's own patternProperties evaluates any node@hex and its
additionalProperties: true admits the rest, so unevaluatedProperties has nothing left to reject. Is prose enough for the address, and is there an idiom I am missing for the rest?
6. include/linux/mdio/mdio-airoha-en8811h.h is named for the driver that consumes it, while its four neighbours in that directory name the driver that implements them - here the implementation is drivers/net/phy/air_phy_lib.c. Where should this header live, and should the MAINTAINERS entry that now claims it also claim the library files it declares?
[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/r/[email protected]/
[3] https://lore.kernel.org/netdev/[email protected]/
Aleksei Sviridkin (7):
dt-bindings: net: add Airoha EN8811H PHY MCU
net: phy: add mdiodev_lock(), mdiodev_lock_nested() and
mdiodev_unlock()
net: phy: air: type the buckpbus core on the mdio device
net: phy: air: move the EN8811H firmware download into the library
net: phy: air: skip the download when the MD32 is already running
net: mdio: add Airoha EN8811H MDIO device driver
net: mdio: en8811h: add the nested bus
.../bindings/net/airoha,en8811h-mcu.yaml | 117 +++++ MAINTAINERS | 8 + drivers/net/mdio/Kconfig | 13 + drivers/net/mdio/Makefile | 1 + drivers/net/mdio/mdio-airoha-en8811h.c | 382 ++++++++++++++++ drivers/net/phy/air_en8811h.c | 151 +------ drivers/net/phy/air_phy_lib.c | 421 ++++++++++++++++-- drivers/net/phy/air_phy_lib.h | 27 ++ include/linux/mdio/mdio-airoha-en8811h.h | 25 ++ include/linux/phy.h | 16 + 10 files changed, 982 insertions(+), 179 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml create mode 100644 drivers/net/mdio/mdio-airoha-en8811h.c create mode 100644 include/linux/mdio/mdio-airoha-en8811h.h
base-commit: 761ae184f850f33d1bbf6c4530c7f237be780d21
Signature · 1 line
2.53.0
RFC PATCH 1/7 dt-bindings: net: add Airoha EN8811H PHY MCU
The EN8811H answers its PHY ID from power-on, but a link is only possible once its MD32 MCU runs firmware that the host loads over MDIO into volatile RAM. On systems that keep the firmware files in a filesystem, the files arrive long after the MDIO bus was scanned, and the PHY node alone cannot express that gap.
Describe the MCU as an MDIO device of its own, with the PHY node on a child bus underneath it. The device downloads the firmware when the files appear, or adopts firmware the bootloader left running, and registers the child bus only then, so the PHY never becomes visible before the chip can serve it.
The reset line belongs to this node, not to the PHY node: firmware lives in volatile RAM, so reset may be asserted only while the MCU is not executing it. On a PHY node the line would be pulsed by every phy_detach() and wipe the running firmware.
--- .../bindings/net/airoha,en8811h-mcu.yaml | 117 ++++++++++++++++++ MAINTAINERS | 6 + 2 files changed, 123 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml
Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml new file +117 -0
@@ -0,0 +1,117 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/airoha,en8811h-mcu.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Airoha EN8811H PHY MCU + +maintainers: + - Aleksei Sviridkin <[email protected]> + +description: | + The Airoha EN8811H 2.5G PHY is built around an MD32 core that executes + firmware from volatile RAM. The chip answers an MDIO address from + power-on, but the PHY function does not exist until firmware has been + loaded over MDIO, and it disappears again if that RAM is cleared. + + This node describes the MD32 core. The PHY function answers on a child + MDIO bus below it, at the same address the core occupies on the parent + bus, and only while firmware is running. + + The reset line is wired to the core, and asserting it clears the + firmware RAM. It therefore belongs to this node: a reset described on + the PHY node would be pulsed by that node's own lifecycle, whatever + the core is doing. + + A board that has firmware in hand while its MDIO bus is enumerated can + describe the PHY on its own, as airoha,en8811h.yaml does, and that form + stays valid. This binding is for the boards where firmware arrives + later, so that there is no PHY to describe until the core has it. + +properties: + $nodename: + pattern: "^mcu@[0-9a-f]+$" + + compatible: + const: airoha,en8811h-mcu + + reg: + maxItems: 1 + + reset-gpios: + maxItems: 1 + description: + The GPIO phandle and specifier for the reset signal wired to the + MD32 core. + + reset-assert-us: + description: + Delay after the reset was asserted in microseconds. If this + property is missing the delay will be skipped. + + reset-deassert-us: + description: + Delay after the reset was deasserted in microseconds. If this + property is missing the delay will be skipped. + + mdio: + $ref: mdio.yaml# + description: + The bus the PHY function answers on while firmware is running, at + the same address this node occupies on the parent bus. Only that + one address is passed through. + + patternProperties: + '^ethernet-phy@[0-9a-f]+$': + type: object + $ref: ethernet-phy.yaml# + unevaluatedProperties: false + + properties: + reset-gpios: false + reset-assert-us: false + reset-deassert-us: false + + description: + The reset line belongs to the MCU node above. A reset described + here would be pulsed by this node's own lifecycle, and that erases + the firmware that created this PHY. + + unevaluatedProperties: false + +required: + - compatible + - reg + - mdio + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + mcu@d { + compatible = "airoha,en8811h-mcu"; + reg = <0xd>; + reset-gpios = <&pio 14 GPIO_ACTIVE_LOW>; + reset-assert-us = <10000>; + reset-deassert-us = <20000>; + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + ethernet-phy@d { + compatible = "ethernet-phy-id03a2.a411"; + reg = <0xd>; + interrupts-extended = <&pio 15 IRQ_TYPE_LEVEL_LOW>; + }; + }; + }; + };
MAINTAINERS +6 -0
@@ -750,6 +750,12 @@ S: Supported F: fs/aio.c F: include/linux/*aio*.h +AIROHA EN8811H MCU MDIO DRIVER +M: Aleksei Sviridkin <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml + AIROHA ETHERNET DRIVER M: Lorenzo Bianconi <[email protected]> L: [email protected] (moderated for non-subscribers)
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [RFC PATCH net-next 1/7] dt-bindings: net: add Airoha EN8811H PHY MCU
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The schema disables `reset-gpios` on the PHY node to prevent firmware erasure but fails to…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The schema disables reset-gpios on the PHY node to prevent firmware erasure but fails to disable the resets and reset-names properties.
- [Medium] The strict unevaluatedProperties: false on the child PHY node rejects valid EN8811H-specific PHY properties.
--
commit 6002751a738ff01594839da605cd448738f78458
Author: Aleksei Sviridkin <[email protected]>
Subject: dt-bindings: net: add Airoha EN8811H PHY MCU
This commit adds a device tree binding for the Airoha EN8811H PHY's MD32 MCU. It describes the MCU as an MDIO device that owns the reset line, with the PHY function operating on a child bus, ensuring the PHY is only visible when firmware is running.
Quoted 5 lines
Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml new file +0
-0
[ ... ]
Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml line 69
Quoted 12 lines
Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml new file +12
-0+ mdio: + $ref: mdio.yaml# + description: + The bus the PHY function answers on while firmware is running, at + the same address this node occupies on the parent bus. Only that + one address is passed through. + + patternProperties: + '^ethernet-phy@[0-9a-f]+$': + type: object + $ref: ethernet-phy.yaml# + unevaluatedProperties: false
[Severity: Medium]
Does this strict evaluation constraint inadvertently block valid PHY-specific properties?
By restricting the properties exclusively to those evaluated in ethernet-phy.yaml#, the schema rejects specific properties defined in the Airoha EN8811H PHY schema, such as rx-polarity from phy-common-props.yaml or airoha,pnswap-rx.
Could this cause dt_binding_check to fail when a valid device tree includes these EN8811H-specific PHY properties on the child node?
Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml line 74
Quoted 5 lines
Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml new file +5
-0+ + properties: + reset-gpios: false + reset-assert-us: false + reset-deassert-us: false
[Severity: Medium]
The schema disables reset-gpios to prevent firmware erasure on the PHY node, but does it also need to disable the standard resets and reset-names properties?
If a device tree configures a hardware reset controller for the child ethernet-phy node using the standard resets property, it will silently pass validation. At runtime, the PHY framework might pulse this reset during detachment, wiping the MCU's volatile firmware.
Should resets: false and reset-names: false be added here to fully close this validation loophole?
Quoted 5 lines
Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml new file +5
-0+ + description: + The reset line belongs to the MCU node above. A reset described + here would be pulsed by this node's own lifecycle, and that erases + the firmware that created this PHY.
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
RFC PATCH 2/7 net: phy: add mdiodev_lock(), mdiodev_lock_nested() and mdiodev_unlock()
In reply to Aleksei SviridkinCode that holds the MDIO bus across a burst of transactions has phy_lock_mdio_bus(), which needs a phy_device. A library serving a chip before its phy_device exists has to open-code the same mutex.
Add the mdio_device spelling beside it. The nested form is for a library whose caller already holds the bus one level up, which is what the EN8811H MDIO driver later in this series does.
--- include/linux/phy.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
include/linux/phy.h +16 -0
@@ -2284,6 +2284,22 @@ static inline void phy_unlock_mdio_bus(struct phy_device *phydev)mutex_unlock (&phydev->mdio.bus->mdio_lock); } +static inline void mdiodev_lock (struct mdio_device *mdiodev) +{ +mutex_lock (&mdiodev->bus->mdio_lock); +} + +/* For a bus reached through another one, as mdiobus_read_nested() is. */ +static inline void mdiodev_lock_nested (struct mdio_device *mdiodev) +{ +mutex_lock_nested (&mdiodev->bus->mdio_lock,MDIO_MUTEX_NESTED ); +} + +static inline void mdiodev_unlock (struct mdio_device *mdiodev) +{ +mutex_unlock (&mdiodev->bus->mdio_lock); +} + void phy_attached_print(struct phy_device *phydev, const char *fmt, ...) __printf(2, 3);char *phy_attached_info_irq (struct phy_device *phydev)
Signature · 1 line
2.53.0
RFC PATCH 3/7 net: phy: air: type the buckpbus core on the mdio device
In reply to Aleksei SviridkinThe buckpbus accessors only need an MDIO device, but they take a phy_device, which ties them to a probed PHY. An upcoming MDIO device driver needs the same register access before any phy_device exists, since it runs precisely to make the PHY presentable.
Retype the internal helpers onto an mdio_device and keep the exported phy_device API as page-selecting wrappers that pass &phydev->mdio. The EN8811H driver already reaches the AN8811HB pbus through an mdio_device, so this follows an existing direction rather than opening a new one.
No functional change.
--- drivers/net/phy/air_phy_lib.c | 80 ++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 38 deletions(-)
drivers/net/phy/air_phy_lib.c +42 -38
@@ -14,31 +14,32 @@#include "air_phy_lib.h" -static int __air_buckpbus_reg_read (struct phy_device *phydev, +static int __air_buckpbus_reg_read (struct mdio_device *mdiodev,u32 pbus_address,u32 *pbus_data) {int pbus_data_low, pbus_data_high;int ret; - ret =__phy_write (phydev,AIR_BPBUS_MODE ,AIR_BPBUS_MODE_ADDR_FIXED ); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_MODE , +AIR_BPBUS_MODE_ADDR_FIXED );if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_RD_ADDR_HIGH , -upper_16_bits (pbus_address)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_RD_ADDR_HIGH , +upper_16_bits (pbus_address));if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_RD_ADDR_LOW , -lower_16_bits (pbus_address)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_RD_ADDR_LOW , +lower_16_bits (pbus_address));if (ret <0 )return ret; - pbus_data_high =__phy_read (phydev,AIR_BPBUS_RD_DATA_HIGH ); + pbus_data_high =__mdiodev_read (mdiodev,AIR_BPBUS_RD_DATA_HIGH );if (pbus_data_high <0 )return pbus_data_high; - pbus_data_low =__phy_read (phydev,AIR_BPBUS_RD_DATA_LOW ); + pbus_data_low =__mdiodev_read (mdiodev,AIR_BPBUS_RD_DATA_LOW );if (pbus_data_low <0 )return pbus_data_low;
@@ -46,64 +47,66 @@ static int __air_buckpbus_reg_read(struct phy_device *phydev,return 0 ; } -static int __air_buckpbus_reg_write (struct phy_device *phydev, +static int __air_buckpbus_reg_write (struct mdio_device *mdiodev,u32 pbus_address,u32 pbus_data) {int ret; - ret =__phy_write (phydev,AIR_BPBUS_MODE ,AIR_BPBUS_MODE_ADDR_FIXED ); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_MODE , +AIR_BPBUS_MODE_ADDR_FIXED );if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_WR_ADDR_HIGH , -upper_16_bits (pbus_address)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_WR_ADDR_HIGH , +upper_16_bits (pbus_address));if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_WR_ADDR_LOW , -lower_16_bits (pbus_address)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_WR_ADDR_LOW , +lower_16_bits (pbus_address));if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_WR_DATA_HIGH , -upper_16_bits (pbus_data)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_WR_DATA_HIGH , +upper_16_bits (pbus_data));if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_WR_DATA_LOW , -lower_16_bits (pbus_data)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_WR_DATA_LOW , +lower_16_bits (pbus_data));if (ret <0 )return ret;return 0 ; } -static int __air_buckpbus_reg_modify (struct phy_device *phydev, +static int __air_buckpbus_reg_modify (struct mdio_device *mdiodev,u32 pbus_address,u32 mask,u32 set) {int pbus_data_low, pbus_data_high;u32 pbus_data_old, pbus_data_new;int ret; - ret =__phy_write (phydev,AIR_BPBUS_MODE ,AIR_BPBUS_MODE_ADDR_FIXED ); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_MODE , +AIR_BPBUS_MODE_ADDR_FIXED );if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_RD_ADDR_HIGH , -upper_16_bits (pbus_address)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_RD_ADDR_HIGH , +upper_16_bits (pbus_address));if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_RD_ADDR_LOW , -lower_16_bits (pbus_address)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_RD_ADDR_LOW , +lower_16_bits (pbus_address));if (ret <0 )return ret; - pbus_data_high =__phy_read (phydev,AIR_BPBUS_RD_DATA_HIGH ); + pbus_data_high =__mdiodev_read (mdiodev,AIR_BPBUS_RD_DATA_HIGH );if (pbus_data_high <0 )return pbus_data_high; - pbus_data_low =__phy_read (phydev,AIR_BPBUS_RD_DATA_LOW ); + pbus_data_low =__mdiodev_read (mdiodev,AIR_BPBUS_RD_DATA_LOW );if (pbus_data_low <0 )return pbus_data_low;
@@ -112,23 +115,23 @@ static int __air_buckpbus_reg_modify(struct phy_device *phydev,if (pbus_data_new == pbus_data_old)return 0 ; - ret =__phy_write (phydev,AIR_BPBUS_WR_ADDR_HIGH , -upper_16_bits (pbus_address)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_WR_ADDR_HIGH , +upper_16_bits (pbus_address));if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_WR_ADDR_LOW , -lower_16_bits (pbus_address)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_WR_ADDR_LOW , +lower_16_bits (pbus_address));if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_WR_DATA_HIGH , -upper_16_bits (pbus_data_new)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_WR_DATA_HIGH , +upper_16_bits (pbus_data_new));if (ret <0 )return ret; - ret =__phy_write (phydev,AIR_BPBUS_WR_DATA_LOW , -lower_16_bits (pbus_data_new)); + ret =__mdiodev_write (mdiodev,AIR_BPBUS_WR_DATA_LOW , +lower_16_bits (pbus_data_new));if (ret <0 )return ret;
@@ -144,7 +147,8 @@ int air_phy_buckpbus_reg_read(struct phy_device *phydev, u32 pbus_address, saved_page =phy_select_page (phydev,AIR_PHY_PAGE_EXTENDED_4 );if (saved_page >=0 ) { - ret =__air_buckpbus_reg_read (phydev, pbus_address, pbus_data); + ret =__air_buckpbus_reg_read (&phydev->mdio, pbus_address, + pbus_data);if (ret <0 )phydev_err (phydev,"%s 0x%08x failed: %d\n" , __func__, pbus_address, ret);
@@ -163,7 +167,7 @@ int air_phy_buckpbus_reg_write(struct phy_device *phydev, u32 pbus_address, saved_page =phy_select_page (phydev,AIR_PHY_PAGE_EXTENDED_4 );if (saved_page >=0 ) { - ret =__air_buckpbus_reg_write (phydev, pbus_address, + ret =__air_buckpbus_reg_write (&phydev->mdio, pbus_address, pbus_data);if (ret <0 ) phydev_err(phydev, "%s 0x%08x failed: %d\n", __func__,
@@ -183,8 +187,8 @@ int air_phy_buckpbus_reg_modify(struct phy_device *phydev, u32 pbus_address, saved_page =phy_select_page (phydev,AIR_PHY_PAGE_EXTENDED_4 );if (saved_page >=0 ) { - ret =__air_buckpbus_reg_modify (phydev, pbus_address, mask, - set); + ret =__air_buckpbus_reg_modify (&phydev->mdio, pbus_address, + mask, set);if (ret <0 )phydev_err (phydev,"%s 0x%08x failed: %d\n" , __func__, pbus_address, ret);
Signature · 1 line
2.53.0
RFC PATCH 4/7 net: phy: air: move the EN8811H firmware download into the library
In reply to Aleksei SviridkinThe EN8811H firmware loader is welded to the PHY driver, but the next patch adds an MDIO device driver that must perform the same download before any phy_device exists.
Move the download engine into the shared library, typed on the MDIO device like the buckpbus core. The moved helpers take the bus lock and save and restore the page register themselves, since phy_select_page() needs a phy_device.
The write loop is broken into chunks that each take the bus lock on their own, because the larger of the two blobs held it for 65536 register writes at a stretch and stalled every other PHY on the bus. It did not matter while this only ran from phy_probe() on a quiet bus at boot; the next patches run it on a live system.
The status poll asks for the MMD register through the Clause 22 indirection unconditionally. phy_read_mmd() resolved that from phydev->is_c45, which the Clause 22 probe path leaves false, so the constant records what the code already did rather than choosing it. It does the three selection writes itself rather than through mmd_phy_read(), which discards their errors and would let a failed selection read a different register back as a success.
The readiness poll's timeout message moves out to the PHY driver's wrapper, which is where the errno was already being reported to a human. The poll is about to gain a caller that retries forever, and an unconditional message there would repeat with every retry; the status value it used to name stays behind at debug level.
Two return values change with the move. The version read at the end of the download is now checked, so a bus error there fails the load instead of leaving the version at zero; en8811h_print_fw_version() still discards it on the AN8811HB path, which does not go through this download. And the readiness poll hands back the errno from a failed status read rather than -ENODEV, which reaches the AN8811HB probe too, since it shares the wait.
--- drivers/net/phy/air_en8811h.c | 146 +---------------- drivers/net/phy/air_phy_lib.c | 301 ++++++++++++++++++++++++++++++++++ drivers/net/phy/air_phy_lib.h | 29 ++++ 3 files changed, 337 insertions(+), 139 deletions(-)
drivers/net/phy/air_en8811h.c +7 -139
@@ -20,21 +20,15 @@#include <linux/bitfield.h> #include <linux/property.h> #include <linux/wordpart.h> -#include <linux/unaligned.h> #include "air_phy_lib.h" #define EN8811H_PHY_ID 0x03a2a411#define AN8811HB_PHY_ID 0xc0ff04a0 -#define EN8811H_MD32_DM "airoha/EthMD32.dm.bin" -#define EN8811H_MD32_DSP "airoha/EthMD32.DSP.bin"#define AN8811HB_MD32_DM "airoha/an8811hb/EthMD32_CRC.DM.bin"#define AN8811HB_MD32_DSP "airoha/an8811hb/EthMD32_CRC.DSP.bin" -#define AIR_FW_ADDR_DM 0x00000000 -#define AIR_FW_ADDR_DSP 0x00100000 -/* MII Registers */ #define AIR_AUX_CTRL_STATUS 0x1d#define AIR_AUX_CTRL_STATUS_SPEED_MASK GENMASK(4, 2)
@@ -44,8 +38,6 @@#define AIR_AUX_CTRL_STATUS_SPEED_2500 0xc/* Registers on MDIO_MMD_VEND1 */ -#define EN8811H_PHY_FW_STATUS 0x8009 -#define EN8811H_PHY_READY 0x02#define AIR_PHY_MCU_CMD_0 0x800b#define AIR_PHY_MCU_CMD_1 0x800c
@@ -108,8 +100,6 @@#define EN8811H_2P5G_LPA 0x3b30#define EN8811H_2P5G_LPA_2P5G BIT(0) -#define EN8811H_FW_VERSION 0x3b3c -#define EN8811H_POLARITY 0xca0f8#define EN8811H_POLARITY_TX_NORMAL BIT(0)#define EN8811H_POLARITY_RX_REVERSE BIT(1)
@@ -122,12 +112,6 @@#define EN8811H_CLK_CGM 0xcf958#define EN8811H_CLK_CGM_CKO BIT(26) -#define EN8811H_FW_CTRL_1 0x0f0018 -#define EN8811H_FW_CTRL_1_START 0x0 -#define EN8811H_FW_CTRL_1_FINISH 0x1 -#define EN8811H_FW_CTRL_2 0x800000 -#define EN8811H_FW_CTRL_2_LOADING BIT(11) -#define AN8811HB_CRC_PM_SET1 0xf020c#define AN8811HB_CRC_PM_MON2 0xf0218#define AN8811HB_CRC_PM_MON3 0xf021c
@@ -270,86 +254,14 @@ static int __air_pbus_reg_write(struct mdio_device *mdiodev, upper_16_bits(pbus_data)); } -static int __air_write_buf (struct phy_device *phydev,u32 address, -const struct firmware *fw) -{ -unsigned int offset; -int ret; -u16 val; - - ret =__phy_write (phydev,AIR_BPBUS_MODE ,AIR_BPBUS_MODE_ADDR_INCR ); -if (ret <0 ) -return ret; - - ret =__phy_write (phydev,AIR_BPBUS_WR_ADDR_HIGH , -upper_16_bits (address)); -if (ret <0 ) -return ret; - - ret =__phy_write (phydev,AIR_BPBUS_WR_ADDR_LOW , -lower_16_bits (address)); -if (ret <0 ) -return ret; - -for (offset =0 ; offset < fw->size; offset +=4 ) { - val =get_unaligned_le16 (&fw->data[offset +2 ]); - ret =__phy_write (phydev,AIR_BPBUS_WR_DATA_HIGH , val); -if (ret <0 ) -return ret; - - val =get_unaligned_le16 (&fw->data[offset]); - ret =__phy_write (phydev,AIR_BPBUS_WR_DATA_LOW , val); -if (ret <0 ) -return ret; - } - -return 0 ; -} - -static int air_write_buf (struct phy_device *phydev,u32 address, -const struct firmware *fw) -{ -int saved_page; -int ret =0 ; - -if (fw->size %4 ) { -phydev_err (phydev,"firmware size %zu is not a multiple of 4\n" , - fw->size); -return -EINVAL ; - } - - saved_page =phy_select_page (phydev,AIR_PHY_PAGE_EXTENDED_4 ); - -if (saved_page >=0 ) { - ret =__air_write_buf (phydev, address, fw); -if (ret <0 ) -phydev_err (phydev,"%s 0x%08x failed: %d\n" , __func__, - address, ret); - } - -return phy_restore_page (phydev, saved_page, ret); -} -static int en8811h_wait_mcu_ready (struct phy_device *phydev) { -int ret, reg_value; +int ret =air_en8811h_wait_mcu_ready (&phydev->mdio); - ret =air_phy_buckpbus_reg_write (phydev,EN8811H_FW_CTRL_1 , -EN8811H_FW_CTRL_1_FINISH ); -if (ret) -return ret; +if (ret <0 ) +phydev_err (phydev,"MCU not ready: %pe\n" ,ERR_PTR (ret)); -/* Because of mdio-lock, may have to wait for multiple loads */ - ret =phy_read_mmd_poll_timeout (phydev,MDIO_MMD_VEND1 , -EN8811H_PHY_FW_STATUS , reg_value, - reg_value ==EN8811H_PHY_READY , -20000 ,7500000 , true); -if (ret) { -phydev_err (phydev,"MCU not ready: 0x%x\n" , reg_value); -return -ENODEV ; - } - -return 0 ; +return ret; } static int an8811hb_check_crc(struct phy_device *phydev, u32 set1,
@@ -411,7 +323,7 @@ static int an8811hb_load_file(struct phy_device *phydev, const char *name,if (ret <0 )return ret; - ret =air_write_buf (phydev, address, fw); + ret =air_fw_write_buf (&phydev->mdio, address, fw);release_firmware (fw);return ret; }
@@ -507,54 +419,10 @@ static int an8811hb_load_firmware(struct phy_device *phydev)static int en8811h_load_firmware (struct phy_device *phydev) { -struct device *dev = &phydev->mdio.dev; -const struct firmware *fw1, *fw2; +struct en8811h_priv *priv = phydev->priv;int ret; - ret =request_firmware_direct (&fw1,EN8811H_MD32_DM , dev); -if (ret <0 ) -return ret; - - ret =request_firmware_direct (&fw2,EN8811H_MD32_DSP , dev); -if (ret <0 ) - goto en8811h_load_firmware_rel1; - - ret =air_phy_buckpbus_reg_write (phydev,EN8811H_FW_CTRL_1 , -EN8811H_FW_CTRL_1_START ); -if (ret <0 ) - goto en8811h_load_firmware_out; - - ret =air_phy_buckpbus_reg_modify (phydev,EN8811H_FW_CTRL_2 , -EN8811H_FW_CTRL_2_LOADING , -EN8811H_FW_CTRL_2_LOADING ); -if (ret <0 ) - goto en8811h_load_firmware_out; - - ret =air_write_buf (phydev,AIR_FW_ADDR_DM , fw1); -if (ret <0 ) - goto en8811h_load_firmware_out; - - ret =air_write_buf (phydev,AIR_FW_ADDR_DSP , fw2); -if (ret <0 ) - goto en8811h_load_firmware_out; - - ret =air_phy_buckpbus_reg_modify (phydev,EN8811H_FW_CTRL_2 , -EN8811H_FW_CTRL_2_LOADING ,0 ); -if (ret <0 ) - goto en8811h_load_firmware_out; - - ret =en8811h_wait_mcu_ready (phydev); -if (ret <0 ) - goto en8811h_load_firmware_out; - -en8811h_print_fw_version (phydev); - -en8811h_load_firmware_out: -release_firmware (fw2); - -en8811h_load_firmware_rel1: -release_firmware (fw1); - + ret =air_en8811h_fw_download (&phydev->mdio, &priv->firmware_version);if (ret <0 )phydev_err (phydev,"Load firmware failed: %d\n" , ret);
drivers/net/phy/air_phy_lib.c +301 -0
315 lines not on this page · raw message
drivers/net/phy/air_phy_lib.h +29 -0
@@ -16,6 +16,11 @@#define AIR_PHY_PAGE_EXTENDED_1 0x0001#define AIR_PHY_PAGE_EXTENDED_4 0x0004 +/* Bytes written between bus releases, so the other PHYs on it get a turn. +* At a 2.5 MHz MDC this size holds the bus for about 30 ms. +*/ +#define AIR_FW_CHUNK_BYTES 2304 +/* MII Registers Page 4*/ #define AIR_BPBUS_MODE 0x10#define AIR_BPBUS_MODE_ADDR_FIXED 0x0000
@@ -29,6 +34,23 @@#define AIR_BPBUS_RD_DATA_HIGH 0x17#define AIR_BPBUS_RD_DATA_LOW 0x18 +#define EN8811H_MD32_DM "airoha/EthMD32.dm.bin" +#define EN8811H_MD32_DSP "airoha/EthMD32.DSP.bin" + +#define AIR_FW_ADDR_DM 0x00000000 +#define AIR_FW_ADDR_DSP 0x00100000 + +#define EN8811H_FW_CTRL_1 0x0f0018 +#define EN8811H_FW_CTRL_1_START 0x0 +#define EN8811H_FW_CTRL_1_FINISH 0x1 +#define EN8811H_FW_CTRL_2 0x800000 +#define EN8811H_FW_CTRL_2_LOADING BIT(11) + +#define EN8811H_PHY_FW_STATUS 0x8009 +#define EN8811H_PHY_READY 0x02 + +#define EN8811H_FW_VERSION 0x3b3c +int air_phy_buckpbus_reg_modify (struct phy_device *phydev,u32 pbus_address,u32 mask,u32 set); int air_phy_buckpbus_reg_read(struct phy_device *phydev, u32 pbus_address,
@@ -38,4 +60,11 @@ int air_phy_buckpbus_reg_write(struct phy_device *phydev, u32 pbus_address,int air_phy_read_page (struct phy_device *phydev);int air_phy_write_page (struct phy_device *phydev,int page); +struct firmware ; + +int air_fw_write_buf (struct mdio_device *mdiodev,u32 address, +const struct firmware *fw); +int air_en8811h_wait_mcu_ready (struct mdio_device *mdiodev); +int air_en8811h_fw_download (struct mdio_device *mdiodev,u32 *fw_version); +#endif /* __AIR_PHY_LIB_H */
Signature · 1 line
2.53.0
RFC PATCH 5/7 net: phy: air: skip the download when the MD32 is already running
In reply to Aleksei SviridkinThe download is unconditional, so a chip whose firmware was loaded by something else - a bootloader, an earlier bind of the PHY driver, or an MDIO device serving the chip - is reprogrammed at 144KB per probe.
Read the status register the loader already polls for readiness and skip the download when it reports ready, only picking up the running firmware's version. Adopting is reported back distinctly from loading, because nothing on that path touches FW_CTRL_1: the PHY driver must leave the MCU restart to .config_init() rather than assume probe already did it.
The image in RAM now wins over the files on disk. Where the PHY node carries no reset-gpios nothing clears that RAM on unbind, so writing new firmware files and rebinding keeps the old image running and reports its version as the current one. A power cycle, or a reset line on the PHY node for phy_detach() to assert, brings the reload back.
--- drivers/net/phy/air_en8811h.c | 4 ++-- drivers/net/phy/air_phy_lib.c | 26 ++++++++++++++++++++++++++ drivers/net/phy/air_phy_lib.h | 3 +++ 3 files changed, 31 insertions(+), 2 deletions(-)
drivers/net/phy/air_en8811h.c +2 -2
@@ -1024,8 +1024,8 @@ static int en8811h_probe(struct phy_device *phydev)if (ret <0 )return ret; -/* mcu has just restarted after firmware load */ - priv->mcu_needs_restart = false; +/* Adopted firmware was never restarted here. */ + priv->mcu_needs_restart = (ret ==1 );/* MDIO_DEVS1/2 empty, so set mmds_present bits here */ phydev->c45_ids.mmds_present |=MDIO_DEVS_PMAPMD |MDIO_DEVS_AN ;
drivers/net/phy/air_phy_lib.c +26 -0
@@ -410,6 +410,17 @@ static int air_mmd_status_read(struct mdio_device *mdiodev)return ret; } +int air_en8811h_mcu_running (struct mdio_device *mdiodev) +{ +int ret =air_mmd_status_read (mdiodev); + +if (ret <0 ) +return ret; + +return ret ==EN8811H_PHY_READY ; +} +EXPORT_SYMBOL_GPL (air_en8811h_mcu_running); +int air_en8811h_wait_mcu_ready (struct mdio_device *mdiodev) {int ret, reg_value;
@@ -443,6 +454,21 @@ int air_en8811h_fw_download(struct mdio_device *mdiodev, u32 *fw_version)struct device *dev = &mdiodev->dev;int ret; + ret =air_en8811h_mcu_running (mdiodev); +if (ret <0 ) +return ret; + +if (ret) { + ret =air_mdio_buckpbus_reg_read (mdiodev,EN8811H_FW_VERSION , + fw_version); +if (ret <0 ) +return ret; + +dev_info (dev,"MD32 already running, firmware %08x\n" , + *fw_version); +return 1 ; + } + ret =request_firmware_direct (&fw1,EN8811H_MD32_DM , dev);if (ret <0 )return ret;
drivers/net/phy/air_phy_lib.h +3 -0
@@ -64,7 +64,10 @@ struct firmware;int air_fw_write_buf (struct mdio_device *mdiodev,u32 address,const struct firmware *fw); +/* Returns 1 running, 0 dormant, negative on a failed status read. */ +int air_en8811h_mcu_running (struct mdio_device *mdiodev);int air_en8811h_wait_mcu_ready (struct mdio_device *mdiodev); +/* Returns 1 when it adopted firmware that was already running. */ int air_en8811h_fw_download (struct mdio_device *mdiodev,u32 *fw_version);#endif /* __AIR_PHY_LIB_H */
Signature · 1 line
2.53.0
RFC PATCH 6/7 net: mdio: add Airoha EN8811H MDIO device driver
In reply to Aleksei SviridkinUntil its firmware has been downloaded the EN8811H is not an Ethernet PHY, it is an MD32 microcontroller waiting in its bootloader. On systems that keep the firmware files in a filesystem, the files become readable long after the MDIO bus was scanned, and the PHY driver's probe-time download then cannot work at boot.
Describe the chip as an MDIO device that polls for the files and downloads through the shared library helper once they can be read; a chip left running by the bootloader is adopted as-is. Polling rather than deferred probing because request_firmware_direct() has no usermode-helper fallback: an unmounted rootfs fails at once, and a deferral only parks the device until something else triggers the pending list, which need not happen when the files appear. There is no give-up path, since installing the firmware package on a running system is a normal thing to do.
The reset line is claimed here rather than on the PHY node, and it is cycled only while the MD32 does not report a running firmware: that firmware lives in volatile RAM, so an assert on a running chip - such as the one phy_detach() performs on a PHY-node reset - would wipe it. A status read that fails says nothing about the firmware, so the line is left alone unless it is already asserted, which is reason enough for the chip not to have answered.
Resume repeats the reset decision and the download in line, since a suspend that cut power leaves the MD32 back in its bootloader.
Nothing yet publishes a PHY: this driver takes the chip over, and the bus that exposes it to the device tree is the next patch. Splitting there keeps the download and its retry policy separate from what registering a bus costs, which is its own failure mode with its own backoff.
--- MAINTAINERS | 2 + drivers/net/mdio/Kconfig | 13 ++ drivers/net/mdio/Makefile | 1 + drivers/net/mdio/mdio-airoha-en8811h.c | 199 +++++++++++++++++++++++ drivers/net/phy/air_phy_lib.h | 9 +- include/linux/mdio/mdio-airoha-en8811h.h | 24 +++ 6 files changed, 241 insertions(+), 7 deletions(-) create mode 100644 drivers/net/mdio/mdio-airoha-en8811h.c create mode 100644 include/linux/mdio/mdio-airoha-en8811h.h
MAINTAINERS +2 -0
@@ -755,6 +755,8 @@ M: Aleksei Sviridkin <[email protected]> L: [email protected] S: Maintained F: Documentation/devicetree/bindings/net/airoha,en8811h-mcu.yaml +F: drivers/net/mdio/mdio-airoha-en8811h.c +F: include/linux/mdio/mdio-airoha-en8811h.h AIROHA ETHERNET DRIVER M: Lorenzo Bianconi <[email protected]>
drivers/net/mdio/Kconfig +13 -0
@@ -29,6 +29,19 @@ config MDIO_AIROHA This module provides a driver for the MDIO busses found in the Airoha AN7583 SoC's. +config MDIO_AIROHA_EN8811H + tristate "Airoha EN8811H MDIO device support" + depends on OF_MDIO + depends on FW_LOADER + select AIR_NET_PHYLIB + imply AIR_EN8811H_PHY + help + This module provides a driver for the Airoha EN8811H, which is an + MD32 microcontroller until firmware is downloaded into it and only + becomes an Ethernet PHY afterwards. The driver downloads that + firmware once it becomes readable, or adopts firmware a bootloader + left running. + config MDIO_SUN4I tristate "Allwinner sun4i MDIO interface support" depends on ARCH_SUNXI || COMPILE_TEST
drivers/net/mdio/Makefile +1 -0
@@ -6,6 +6,7 @@ obj-$(CONFIG_FWNODE_MDIO) += fwnode_mdio.o obj-$(CONFIG_OF_MDIO) += of_mdio.o obj-$(CONFIG_MDIO_AIROHA) += mdio-airoha.o +obj-$(CONFIG_MDIO_AIROHA_EN8811H) += mdio-airoha-en8811h.o obj-$(CONFIG_MDIO_ASPEED) += mdio-aspeed.o obj-$(CONFIG_MDIO_BCM_IPROC) += mdio-bcm-iproc.o obj-$(CONFIG_MDIO_BCM_UNIMAC) += mdio-bcm-unimac.o
drivers/net/mdio/mdio-airoha-en8811h.c new file +199 -0
199 lines not on this page · raw message
drivers/net/phy/air_phy_lib.h +2 -7
@@ -10,6 +10,8 @@#include <linux/phy.h> +#include <linux/mdio/mdio-airoha-en8811h.h> +#define AIR_EXT_PAGE_ACCESS 0x1f#define AIR_PHY_PAGE_STANDARD 0x0000
@@ -34,9 +36,6 @@#define AIR_BPBUS_RD_DATA_HIGH 0x17#define AIR_BPBUS_RD_DATA_LOW 0x18 -#define EN8811H_MD32_DM "airoha/EthMD32.dm.bin" -#define EN8811H_MD32_DSP "airoha/EthMD32.DSP.bin" -#define AIR_FW_ADDR_DM 0x00000000#define AIR_FW_ADDR_DSP 0x00100000
@@ -64,10 +63,6 @@ struct firmware;int air_fw_write_buf (struct mdio_device *mdiodev,u32 address,const struct firmware *fw); -/* Returns 1 running, 0 dormant, negative on a failed status read. */ -int air_en8811h_mcu_running (struct mdio_device *mdiodev);int air_en8811h_wait_mcu_ready (struct mdio_device *mdiodev); -/* Returns 1 when it adopted firmware that was already running. */ -int air_en8811h_fw_download (struct mdio_device *mdiodev,u32 *fw_version);#endif /* __AIR_PHY_LIB_H */
include/linux/mdio/mdio-airoha-en8811h.h new file +24 -0
@@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* +* Copyright (C) 2026 Airoha Technology Corp. +* Copyright (C) 2026 Collabora Ltd. +* Louis-Alexis Eyraud <[email protected]> +* Copyright (C) 2026 Aleksei Sviridkin <[email protected]> +*/ + +#ifndef __LINUX_MDIO_AIROHA_EN8811H_H +#define __LINUX_MDIO_AIROHA_EN8811H_H + +#include <linux/types.h> + +struct mdio_device ; + +#define EN8811H_MD32_DM "airoha/EthMD32.dm.bin" +#define EN8811H_MD32_DSP "airoha/EthMD32.DSP.bin" + +/* Returns 1 running, 0 dormant, negative on a failed status read. */ +int air_en8811h_mcu_running (struct mdio_device *mdiodev); +/* Returns 1 when it adopted firmware that was already running. */ +int air_en8811h_fw_download (struct mdio_device *mdiodev,u32 *fw_version); + +#endif /* __LINUX_MDIO_AIROHA_EN8811H_H */
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [RFC PATCH net-next 6/7] net: mdio: add Airoha EN8811H MDIO device driver
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Incorrect PM wrapper used for dev_pm_ops structure pointer. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Incorrect PM wrapper used for dev_pm_ops structure pointer.
--
commit 23bfd1bd681c899f8cb9185a35af023d5e062675
Author: Aleksei Sviridkin <[email protected]>
net: mdio: add Airoha EN8811H MDIO device driver
This commit introduces the EN8811H MDIO device driver. The driver polls for and downloads firmware to the MD32 microcontroller before it functions as an Ethernet PHY.
Quoted 5 lines
drivers/net/mdio/mdio-airoha-en8811h.c new file +0
-0
[ ... ]
Quoted 5 lines
drivers/net/mdio/mdio-airoha-en8811h.c new file +5
-0+static DEFINE_SIMPLE_DEV_PM_OPS(en8811h_mcu_pm_ops, NULL, en8811h_mcu_resume); + +static const struct of_device_id en8811h_mcu_of_match[] = { + { .compatible ="airoha,en8811h-mcu" }, + { }
drivers/net/mdio/mdio-airoha-en8811h.c line 188
Quoted 10 lines
drivers/net/mdio/mdio-airoha-en8811h.c new file +10
-0+}; +MODULE_DEVICE_TABLE (of, en8811h_mcu_of_match); + +static struct mdio_driver en8811h_mcu_driver = { + .probe = en8811h_mcu_probe, + .remove = en8811h_mcu_remove, + .mdiodrv.driver = { + .name ="airoha-en8811h-mcu" , + .of_match_table = en8811h_mcu_of_match, + .pm =pm_sleep_ptr (&en8811h_mcu_pm_ops),
[Severity: Low]
Should this use pm_ptr() instead of pm_sleep_ptr() for the dev_pm_ops structure pointer?
The power management subsystem guidelines indicate that pm_sleep_ptr() is reserved for individual sleep callbacks inside the dev_pm_ops structure itself, while pm_ptr() must be used when assigning the structure pointer to the driver declaration in en8811h_mcu_driver.
Quoted 4 lines
drivers/net/mdio/mdio-airoha-en8811h.c new file +4
-0+ }, +}; + +mdio_module_driver (en8811h_mcu_driver);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
RFC PATCH 7/7 net: mdio: en8811h: add the nested bus
In reply to Aleksei SviridkinRegistering the bus is what publishes the PHY, so it must happen only once the MD32 is running its firmware. Put the PHY on a bus of its own rather than on the parent so that the device tree can describe it normally, interrupts included, and so that the MCU keeps ownership of the reset line the PHY must not touch. Only the address the MD32 answers on is passed through.
Publishing the PHY there splits the two agents that drive this chip onto two mutexes: the PHY holds the child bus lock across a whole paged sequence while each frame under it takes the parent alone, so a download holding only the parent would land between two of those frames and leave the PHY writing to a page it did not select. This driver therefore takes the child bus lock around its own chip access once the bus exists, and the library beneath it takes the parent one level down. The level cannot be baked into the library: reached from here the parent sits below a lock already held, while reached from the PHY driver on this same bus the library takes the child itself. The caller passes it down. MDIO_MUTEX_NESTED is what the child bus's own accessors already use for the parent, so this adds no edge lockdep did not have; the enum's remaining value belongs to mdio-mux, so a bus stacked under another nested bus would want a new one.
The download becomes the first of two phases, since registering the bus can fail on its own and wants its own backoff and warning. That second phase gives up once its minute is spent, where the first does not:
firmware files can be installed at any time, but a registration that keeps failing repeats a bus creation, a message from the MDIO core and a pair of uevents on every retry, and only a deferred probe resolves by itself. A driver that stopped there needs to be unbound and bound again. Resume now
puts a failed reload back to the poller rather than leaving it, but that restores the chip and not the port: the PHY below has run its own resume by then and failed, and nothing calls phy_init_hw() a second time, so it stays unconfigured until its driver is rebound. Telling a consumer to re-initialise is the same mechanism this driver lacks for telling one to let go, and it is not invented here.
The sysfs bind attributes are suppressed, because unregistering this bus while its PHY is attached leaves the consumer holding a phy_device whose driver is gone, and the driver cannot disconnect a consumer it does not know. Module unload was already refused, since attaching a PHY takes a reference on the bus owner. Removing the parent MDIO bus still reaches this driver's remove and is not covered; that wants a way for a bus to tell its PHYs' consumers to let go.
--- drivers/net/mdio/mdio-airoha-en8811h.c | 223 +++++++++++++++++++++-- drivers/net/phy/air_en8811h.c | 7 +- drivers/net/phy/air_phy_lib.c | 64 ++++--- drivers/net/phy/air_phy_lib.h | 4 +- include/linux/mdio/mdio-airoha-en8811h.h | 5 +- 5 files changed, 251 insertions(+), 52 deletions(-)
drivers/net/mdio/mdio-airoha-en8811h.c +203 -20
@@ -14,6 +14,8 @@#include <linux/mdio.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_mdio.h> +#include <linux/phy.h> #include <linux/pm.h> #include <linux/property.h> #include <linux/workqueue.h>
@@ -28,21 +30,144 @@ struct en8811h_mcu {struct mdio_device *mdiodev;struct gpio_desc *reset_gpio;struct delayed_work fw_poll; +struct mii_bus *bus;unsigned int poll_ms;unsigned int waited_ms;u32 fw_version;bool warned; +bool fw_running; }; +static int en8811h_mcu_read (struct mii_bus *bus,int addr,int regnum) +{ +struct en8811h_mcu *mcu = bus->priv; + +if (addr != mcu->mdiodev->addr) +return -ENODEV ; + +return mdiobus_read_nested (mcu->mdiodev->bus, addr, regnum); +} + +static int en8811h_mcu_write (struct mii_bus *bus,int addr,int regnum,u16 val) +{ +struct en8811h_mcu *mcu = bus->priv; + +if (addr != mcu->mdiodev->addr) +return -ENODEV ; + +return mdiobus_write_nested (mcu->mdiodev->bus, addr, regnum, val); +} + +static int en8811h_mcu_read_c45 (struct mii_bus *bus,int addr,int devad, +int regnum) +{ +struct en8811h_mcu *mcu = bus->priv; + +if (addr != mcu->mdiodev->addr) +return -ENODEV ; + +return mdiobus_c45_read_nested (mcu->mdiodev->bus, addr, devad, regnum); +} + +static int en8811h_mcu_write_c45 (struct mii_bus *bus,int addr,int devad, +int regnum,u16 val) +{ +struct en8811h_mcu *mcu = bus->priv; + +if (addr != mcu->mdiodev->addr) +return -ENODEV ; + +return mdiobus_c45_write_nested (mcu->mdiodev->bus, addr, devad, regnum, + val); +} + +static int en8811h_mcu_bus_register (struct en8811h_mcu *mcu) +{ +struct mii_bus *parent = mcu->mdiodev->bus; +struct device *dev = &mcu->mdiodev->dev; +struct device_node *np; +struct mii_bus *bus; +int ret; + + np =of_get_child_by_name (dev->of_node,"mdio" ); +if (!np) +return -ENODEV ; + +/* Not devm: this is retried, and a devm bus would only be freed at +* detach. +*/ + bus =mdiobus_alloc (); +if (!bus) { +of_node_put (np); +return -ENOMEM ; + } + + bus->name ="airoha-en8811h" ; +snprintf (bus->id,MII_BUS_ID_SIZE ,"%s" ,dev_name (dev)); + bus->priv = mcu; + bus->parent = dev; +if (parent->read) { + bus->read = en8811h_mcu_read; + bus->write = en8811h_mcu_write; + } +if (parent->read_c45) { + bus->read_c45 = en8811h_mcu_read_c45; + bus->write_c45 = en8811h_mcu_write_c45; + } + + ret =of_mdiobus_register (bus, np); +of_node_put (np); +if (!ret && !mdiobus_get_phy (bus, mcu->mdiodev->addr)) { +/* An ID read that failed leaves the bus registered and the +* PHY absent; of_mdiobus_register() returns 0 either way. +*/ +mdiobus_unregister (bus); + ret = -ENODEV ; + } +if (ret) { +mdiobus_free (bus); +return ret; + } + + mcu->bus = bus; +return 0 ; +} + +/* Serialise with the PHY below, which reaches the same registers under +* this lock through phy_select_page(). +*/ +static struct mii_bus *en8811h_mcu_chip_lock (struct en8811h_mcu *mcu) +{ +struct mii_bus *bus = mcu->bus; + +if (bus) +mutex_lock (&bus->mdio_lock); + +return bus; +} + +static void en8811h_mcu_chip_unlock (struct mii_bus *bus) +{ +if (bus) +mutex_unlock (&bus->mdio_lock); +} +static void en8811h_mcu_fw_poll (struct work_struct *work) {struct en8811h_mcu *mcu =container_of (to_delayed_work (work), struct en8811h_mcu, fw_poll);struct device *dev = &mcu->mdiodev->dev; +struct mii_bus *chip;int ret; - ret =air_en8811h_fw_download (mcu->mdiodev, &mcu->fw_version); -if (ret >=0 ) { +if (!mcu->fw_running) { + chip =en8811h_mcu_chip_lock (mcu); + ret =air_en8811h_fw_download (mcu->mdiodev, &mcu->fw_version, + !!chip); +en8811h_mcu_chip_unlock (chip); +if (ret <0 ) + goto retry; +dev_dbg (dev,"firmware %08x running after %ums\n" , mcu->fw_version, mcu->waited_ms); ret = firmware_request_cache(dev, EN8811H_MD32_DM) ?:
@@ -50,34 +175,58 @@ static void en8811h_mcu_fw_poll(struct work_struct *work)if (ret)dev_dbg (dev,"not cached, resume will want the files: %pe\n" ,ERR_PTR (ret)); -return ; + mcu->fw_running = true; + mcu->poll_ms =EN8811H_FW_POLL_MIN_MS ; + mcu->waited_ms =0 ; + mcu->warned = false; } - mcu->waited_ms += mcu->poll_ms; +/* Resume re-runs the download, so the bus can already be here, and +* fwnode_mdio defers while the PHY node's interrupt controller is +* missing, so a failure is not necessarily permanent. +*/ + ret = mcu->bus ?0 :en8811h_mcu_bus_register (mcu); +if (!ret) +return ; + +retry:if (!mcu->warned && mcu->waited_ms >=EN8811H_FW_WARN_MS ) { -if (ret == -ENOENT ) -dev_warn (dev,"still waiting for %s and %s\n" , -EN8811H_MD32_DM ,EN8811H_MD32_DSP ); +if (!mcu->fw_running) +dev_warn (dev,"no firmware after %ums of waiting for %s and %s: %pe\n" , + mcu->waited_ms,EN8811H_MD32_DM , +EN8811H_MD32_DSP ,ERR_PTR (ret));else -dev_warn (dev,"firmware download keeps failing: %pe\n" , +dev_warn (dev,"no PHY at address %d, %ums after the firmware started: %pe\n" , + mcu->mdiodev->addr, mcu->waited_ms,ERR_PTR (ret)); mcu->warned = true; } +/* Past its budget, only a deferral will change on its own: every +* other error repeats a bus registration, its message from the MDIO +* core and its uevents for the uptime. Firmware files are the +* exception, since they can be installed at any time. +*/ +if (mcu->fw_running && ret != -EPROBE_DEFER && + mcu->waited_ms >=EN8811H_FW_WARN_MS ) +return ; + mcu->poll_ms =min (mcu->poll_ms *2 ,EN8811H_FW_POLL_MAX_MS ); +/* Count the sleep ahead: the first run was immediate. */ + mcu->waited_ms += mcu->poll_ms;queue_delayed_work (system_freezable_wq, &mcu->fw_poll,msecs_to_jiffies (mcu->poll_ms)); }/* The firmware lives in volatile RAM: no reset while the MD32 reports ready. */ -static void en8811h_mcu_reset_if_dormant (struct en8811h_mcu *mcu) +static void en8811h_mcu_reset_if_dormant (struct en8811h_mcu *mcu,bool nested) {struct mdio_device *mdiodev = mcu->mdiodev;struct device *dev = &mdiodev->dev;u32 assert_us =0 , deassert_us =0 ;int ret; - ret =air_en8811h_mcu_running (mdiodev); + ret =air_en8811h_mcu_running (mdiodev, nested);if (ret >0 ) {dev_dbg (dev,"MD32 already running, leaving reset alone\n" );return ;
@@ -116,6 +265,7 @@ static int en8811h_mcu_probe(struct mdio_device *mdiodev) {struct device *dev = &mdiodev->dev;struct en8811h_mcu *mcu; +struct device_node *np; mcu =devm_kzalloc (dev,sizeof (*mcu),GFP_KERNEL );if (!mcu)
@@ -124,6 +274,15 @@ static int en8811h_mcu_probe(struct mdio_device *mdiodev) mcu->mdiodev = mdiodev;mdiodev_set_drvdata (mdiodev, mcu); +/* Registration needs this only once the firmware runs, but a DT +* hole should fail the bind now, not as a work-item error later. +*/ + np =of_get_child_by_name (dev->of_node,"mdio" ); +if (!np) +return dev_err_probe (dev, -ENODEV , +"no mdio node describing the PHY\n" ); +of_node_put (np); +/* The core claims reset-gpios only for devices flagged as PHYs. */ mcu->reset_gpio =devm_gpiod_get_optional (dev,"reset" ,GPIOD_ASIS );if (IS_ERR (mcu->reset_gpio))
@@ -133,7 +292,7 @@ static int en8811h_mcu_probe(struct mdio_device *mdiodev)if (mcu->reset_gpio)gpiod_set_consumer_name (mcu->reset_gpio,"EN8811H reset" ); -en8811h_mcu_reset_if_dormant (mcu); +en8811h_mcu_reset_if_dormant (mcu, false); mcu->poll_ms =EN8811H_FW_POLL_MIN_MS ;INIT_DELAYED_WORK (&mcu->fw_poll, en8811h_mcu_fw_poll);
@@ -150,23 +309,43 @@ static void en8811h_mcu_remove(struct mdio_device *mdiodev)struct en8811h_mcu *mcu =mdiodev_get_drvdata (mdiodev);cancel_delayed_work_sync (&mcu->fw_poll); +if (mcu->bus) { +mdiobus_unregister (mcu->bus); +mdiobus_free (mcu->bus); + mcu->bus =NULL ; + } }static int en8811h_mcu_resume (struct device *dev) {struct en8811h_mcu *mcu =dev_get_drvdata (dev); - +struct mii_bus *chip;int ret; -/* Not on the workqueue: the child PHY's own resume calls -* phy_init_hw() straight after this one and needs the firmware by -* then. request_firmware() is answered from the cache the download -* registered, so it does not wait for a filesystem. +/* Nothing to redo: the poll is armed and thaws with everything else. */ +if (!mcu->fw_running) +return 0 ; + +/* Not on the workqueue: a DSA port's PHY resumes right after this +* one and calls phy_init_hw(), which needs the firmware by then. */ -en8811h_mcu_reset_if_dormant (mcu); - ret =air_en8811h_fw_download (mcu->mdiodev, &mcu->fw_version); -if (ret <0 ) -dev_err (dev,"firmware not restored: %pe\n" ,ERR_PTR (ret)); + chip =en8811h_mcu_chip_lock (mcu); +en8811h_mcu_reset_if_dormant (mcu, !!chip); + ret =air_en8811h_fw_download (mcu->mdiodev, &mcu->fw_version, !!chip); +en8811h_mcu_chip_unlock (chip); +if (ret <0 ) { +/* The reload restores this chip, not the PHY below it: its +* own resume has already failed by then and nothing calls +* phy_init_hw() twice. +*/ +dev_err (dev,"firmware not restored, reloading: %pe\n" , +ERR_PTR (ret)); + mcu->fw_running = false; + mcu->poll_ms =EN8811H_FW_POLL_MIN_MS ; + mcu->waited_ms =0 ; + mcu->warned = false; +queue_delayed_work (system_freezable_wq, &mcu->fw_poll,0 ); + }return 0 ; }
@@ -186,6 +365,10 @@ static struct mdio_driver en8811h_mcu_driver = { .name = "airoha-en8811h-mcu", .of_match_table = en8811h_mcu_of_match, .pm = pm_sleep_ptr(&en8811h_mcu_pm_ops), +/* Tearing the child bus down under an attached PHY is not +* something this driver can make safe on its own. +*/ + .suppress_bind_attrs = true, }, };
drivers/net/phy/air_en8811h.c +4 -3
@@ -256,7 +256,7 @@ static int __air_pbus_reg_write(struct mdio_device *mdiodev,static int en8811h_wait_mcu_ready (struct phy_device *phydev) { -int ret =air_en8811h_wait_mcu_ready (&phydev->mdio); +int ret =air_en8811h_wait_mcu_ready (&phydev->mdio, false);if (ret <0 )phydev_err (phydev,"MCU not ready: %pe\n" ,ERR_PTR (ret));
@@ -323,7 +323,7 @@ static int an8811hb_load_file(struct phy_device *phydev, const char *name,if (ret <0 )return ret; - ret =air_fw_write_buf (&phydev->mdio, address, fw); + ret =air_fw_write_buf (&phydev->mdio, address, fw, false);release_firmware (fw);return ret; }
@@ -422,7 +422,8 @@ static int en8811h_load_firmware(struct phy_device *phydev)struct en8811h_priv *priv = phydev->priv;int ret; - ret =air_en8811h_fw_download (&phydev->mdio, &priv->firmware_version); + ret =air_en8811h_fw_download (&phydev->mdio, &priv->firmware_version, + false);if (ret <0 )phydev_err (phydev,"Load firmware failed: %d\n" , ret);
drivers/net/phy/air_phy_lib.c +39 -25
180 lines not on this page · raw message
drivers/net/phy/air_phy_lib.h +2 -2
@@ -62,7 +62,7 @@ int air_phy_write_page(struct phy_device *phydev, int page);struct firmware ;int air_fw_write_buf (struct mdio_device *mdiodev,u32 address, -const struct firmware *fw); -int air_en8811h_wait_mcu_ready (struct mdio_device *mdiodev); +const struct firmware *fw,bool nested); +int air_en8811h_wait_mcu_ready (struct mdio_device *mdiodev,bool nested);#endif /* __AIR_PHY_LIB_H */
include/linux/mdio/mdio-airoha-en8811h.h +3 -2
@@ -17,8 +17,9 @@ struct mdio_device;#define EN8811H_MD32_DSP "airoha/EthMD32.DSP.bin"/* Returns 1 running, 0 dormant, negative on a failed status read. */ -int air_en8811h_mcu_running (struct mdio_device *mdiodev); +int air_en8811h_mcu_running (struct mdio_device *mdiodev,bool nested);/* Returns 1 when it adopted firmware that was already running. */ -int air_en8811h_fw_download (struct mdio_device *mdiodev,u32 *fw_version); +int air_en8811h_fw_download (struct mdio_device *mdiodev,u32 *fw_version, +bool nested);#endif /* __LINUX_MDIO_AIROHA_EN8811H_H */
Signature · 1 line
2.53.0