an8855: add MFD driver
Subsystems (2)
v2 cover an8855: add MFD driver
This micro series is the AN8855 MFD driver that can go parallel to the DSA driver.
Changes v2:
- Rebase on top of linux-next
- Drop switch part from DT patch
Christian Marangi (2):
dt-bindings: mfd: Document support for Airoha AN8855 Switch SoC
mfd: an8855: Add support for Airoha AN8855 Switch
.../bindings/mfd/airoha,an8855.yaml | 134 ++++ drivers/mfd/Kconfig | 14 + drivers/mfd/Makefile | 1 + drivers/mfd/airoha-an8855.c | 579 ++++++++++++++++++ 4 files changed, 728 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/airoha,an8855.yaml create mode 100644 drivers/mfd/airoha-an8855.c
Signature · 1 line
2.55.0
PATCH v2 1/2 dt-bindings: mfd: Document support for Airoha AN8855 Switch SoC
Document support for Airoha AN8855 Switch SoC. This SoC expose various peripherals like an Ethernet Switch, a NVMEM provider and Ethernet PHYs.
It does also support i2c and timers but those are not currently supported/used.
---
This was already approved by Rob but I dropped since I dropped the DSA switch section and reference (since it will be handled by the other series)
I have a question about this... Can I send a follow-up patch to this schema to add the ethernet-switch and make it required by this or will be an ABI violation?
It's not clear how to handle this detail. Would really love some guidance on this.
(or maybe the DSA schema should be pushed with this series?)
.../bindings/mfd/airoha,an8855.yaml | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/airoha,an8855.yaml
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml new file +134 -0
@@ -0,0 +1,134 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/airoha,an8855.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Airoha AN8855 Switch SoC + +maintainers: + - Christian Marangi <[email protected]> + +description: > + Airoha AN8855 Switch is a SoC that expose various peripherals like an + Ethernet Switch, a NVMEM provider and Ethernet PHYs. + + It does also support i2c and timers but those are not currently + supported/used. + +properties: + compatible: + const: airoha,an8855 + + reg: + maxItems: 1 + + reset-gpios: + maxItems: 1 + + efuse: + type: object + $ref: /schemas/nvmem/airoha,an8855-efuse.yaml + description: EFUSE exposed by the Airoha AN8855 SoC + + mdio: + type: object + $ref: /schemas/net/mdio.yaml# + description: MDIO exposed by the Airoha AN8855 SoC + unevaluatedProperties: false + +required: + - compatible + - reg + - mdio + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + soc@1 { + compatible = "airoha,an8855"; + reg = <1>; + + reset-gpios = <&pio 39 GPIO_ACTIVE_HIGH>; + + efuse { + compatible = "airoha,an8855-efuse"; + + #nvmem-cell-cells = <0>; + + nvmem-layout { + compatible = "fixed-layout"; + #address-cells = <1>; + #size-cells = <1>; + + shift_sel_port0_tx_a: shift-sel-port0-tx-a@c { + reg = <0xc 0x4>; + }; + + shift_sel_port0_tx_b: shift-sel-port0-tx-b@10 { + reg = <0x10 0x4>; + }; + + shift_sel_port0_tx_c: shift-sel-port0-tx-c@14 { + reg = <0x14 0x4>; + }; + + shift_sel_port0_tx_d: shift-sel-port0-tx-d@18 { + reg = <0x18 0x4>; + }; + + shift_sel_port1_tx_a: shift-sel-port1-tx-a@1c { + reg = <0x1c 0x4>; + }; + + shift_sel_port1_tx_b: shift-sel-port1-tx-b@20 { + reg = <0x20 0x4>; + }; + + shift_sel_port1_tx_c: shift-sel-port1-tx-c@24 { + reg = <0x24 0x4>; + }; + + shift_sel_port1_tx_d: shift-sel-port1-tx-d@28 { + reg = <0x28 0x4>; + }; + }; + }; + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + internal_phy1: ethernet-phy@1 { + compatible = "ethernet-phy-idc0ff.0410", + "ethernet-phy-ieee802.3-c22"; + reg = <1>; + + nvmem-cells = <&shift_sel_port0_tx_a>, + <&shift_sel_port0_tx_b>, + <&shift_sel_port0_tx_c>, + <&shift_sel_port0_tx_d>; + nvmem-cell-names = "tx_a", "tx_b", "tx_c", "tx_d"; + }; + + internal_phy2: ethernet-phy@2 { + compatible = "ethernet-phy-idc0ff.0410", + "ethernet-phy-ieee802.3-c22"; + reg = <2>; + + nvmem-cells = <&shift_sel_port1_tx_a>, + <&shift_sel_port1_tx_b>, + <&shift_sel_port1_tx_c>, + <&shift_sel_port1_tx_d>; + nvmem-cell-names = "tx_a", "tx_b", "tx_c", "tx_d"; + }; + }; + }; + };
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v2 1/2] dt-bindings: mfd: Document support for Airoha AN8855 Switch SoC
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] dt-bindings: mfd: airoha,an8855: missing ethernet-switch node breaks DT ABI - [Low] dt-bindings…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: mfd: airoha,an8855: missing ethernet-switch node breaks DT ABI
- [Low] dt-bindings: mfd: airoha,an8855: undocumented PHY nvmem-cells in example --
commit 5c22f417efc026df5dc02aca005efec24fec3d06
Author: Christian Marangi <[email protected]>
dt-bindings: mfd: Document support for Airoha AN8855 Switch SoC
This commit adds the MFD device tree binding schema for the Airoha AN8855 Switch SoC, documenting its efuse NVMEM provider and MDIO bus child nodes.
Quoted 6 lines
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml new file +0
-0@@ -0,0 +1,134 @@
[ ... ]
Quoted 17 lines
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml new file +17
-0+properties: + compatible: + const: airoha,an8855 + + reg: + maxItems: 1 + + reset-gpios: + maxItems: 1 + + efuse: + type: object + $ref: /schemas/nvmem/airoha,an8855-efuse.yaml + description: EFUSE exposed by the Airoha AN8855 SoC + + mdio: + type: object
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml line 45
Quoted 10 lines
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml new file +10
-0+ $ref: /schemas/net/mdio.yaml# + description: MDIO exposed by the Airoha AN8855 SoC + unevaluatedProperties: false + +required: + - compatible + - reg + - mdio + +additionalProperties: false
[Severity: Medium]
The description states that the Airoha AN8855 Switch SoC exposes an Ethernet Switch, an NVMEM provider, and Ethernet PHYs, and the accompanying MFD driver registers MFD cells for both airoha,an8855-efuse and airoha,an8855-switch.
However, properties in airoha,an8855.yaml omits the ethernet-switch child node while setting additionalProperties: false, and leaves efuse out of the required list.
In Device Tree bindings, adding a child node to required after the binding is merged breaks DT ABI compatibility because existing DTBs without that child node become invalid, and bindings are expected to describe the complete hardware up front rather than staging child nodes across driver series.
Should the DSA switch schema be included with this series so that ethernet-switch and efuse can both be documented and marked required from the start?
[ ... ]
Quoted 5 lines
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml new file +5
-0+ mdio { + #address-cells = <1>; + #size-cells = <0>; + + internal_phy1: ethernet-phy@1 {
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml line 119
Quoted 10 lines
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml new file +10
-0+ compatible = "ethernet-phy-idc0ff.0410", + "ethernet-phy-ieee802.3-c22"; + reg = <1>; + + nvmem-cells = <&shift_sel_port0_tx_a>, + <&shift_sel_port0_tx_b>, + <&shift_sel_port0_tx_c>, + <&shift_sel_port0_tx_d>; + nvmem-cell-names = "tx_a", "tx_b", "tx_c", "tx_d"; + };
[Severity: Low]
In examples, internal_phy1 and internal_phy2 under the mdio node specify compatible = "ethernet-phy-idc0ff.0410", "ethernet-phy-ieee802.3-c22" along with nvmem-cells and nvmem-cell-names = "tx_a", "tx_b", "tx_c", "tx_d".
Documentation/devicetree/bindings/net/ethernet-phy.yaml does not define nvmem-cells or nvmem-cell-names, and no binding schema under Documentation/devicetree/bindings/net/ documents ethernet-phy-idc0ff.0410 or these calibration cell names. The example only passes dt_binding_check because ethernet-phy.yaml sets additionalProperties: true.
Does this need a dedicated PHY binding schema documenting those nvmem calibration properties before using them in the example?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v2 2/2 mfd: an8855: Add support for Airoha AN8855 Switch
In reply to Christian MarangiAdd support for Airoha AN8855 Switch that provide support for a DSA switch and a NVMEM provider.
Also make use of the mdio-regmap driver and register a regmap for each internal PHY of the switch.
This is needed to handle the double usage of the PHYs as both PHY and Switch accessor.
--- drivers/mfd/Kconfig | 14 + drivers/mfd/Makefile | 1 + drivers/mfd/airoha-an8855.c | 579 ++++++++++++++++++++++++++++++++++++ 3 files changed, 594 insertions(+) create mode 100644 drivers/mfd/airoha-an8855.c
drivers/mfd/Kconfig +14 -0
@@ -54,6 +54,20 @@ config MFD_ALTERA_SYSMGR using regmap_mmio accesses for ARM32 parts and SMC calls to EL3 for ARM64 parts. +config MFD_AIROHA_AN8855 + tristate "Airoha AN8855 Switch Core" + select MFD_CORE + select REGMAP + select PHYLIB + select MDIO_REGMAP + depends on NETDEVICES && OF + help + Support for the Airoha AN8855 Switch Core. This is an SoC + that provides various peripherals, to count, i2c, an Ethrnet + Switch, a CPU timer, GPIO, eFUSE. + + Currently it provides a DSA switch and a NVMEM provider. + config MFD_ACT8945A tristate "Active-semi ACT8945A" select MFD_CORE
drivers/mfd/Makefile +1 -0
@@ -8,6 +8,7 @@ obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o obj-$(CONFIG_MFD_88PM800) += 88pm800.o 88pm80x.o obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o +obj-$(CONFIG_MFD_AIROHA_AN8855) += airoha-an8855.o obj-$(CONFIG_MFD_ACT8945A) += act8945a.o obj-$(CONFIG_MFD_SM501) += sm501.o obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
drivers/mfd/airoha-an8855.c new file +579 -0
579 lines not on this page · raw message
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v2 2/2] mfd: an8855: Add support for Airoha AN8855 Switch
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mfd: an8855: missing reset-gpios handling in an8855_probe() - [High] mfd: an8855: missing…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mfd: an8855: missing reset-gpios handling in an8855_probe()
- [High] mfd: an8855: missing PHY_MAX_ADDR check in an8855_phy_bus_register()
- [Medium] mfd: an8855: register internal PHYs after an8855-efuse to avoid genphy fallback --
commit b751ed12fefc10d7bebaac0f5731b709d5c0d2bd
Author: Christian Marangi <[email protected]>
mfd: an8855: Add support for Airoha AN8855 Switch
This commit adds an MFD core driver for the Airoha AN8855 switch, registering virtual MDIO buses via mdio-regmap for the internal PHYs and MFD cells for the DSA switch and NVMEM provider.
Quoted 6 lines
drivers/mfd/airoha-an8855.c new file +0
-0@@ -0,0 +1,579 @@
[ ... ]
Quoted 4 lines
drivers/mfd/airoha-an8855.c new file +4
-0+#include <linux/bitfield.h> +#include <linux/fwnode_mdio.h> +#include <linux/gpio/consumer.h> +#include <linux/mfd/core.h>
[ ... ]
Quoted 8 lines
drivers/mfd/airoha-an8855.c new file +8
-0+static int an8855_phy_bus_register (struct device *dev,struct an8855 *ddata, +struct device_node *phy_np, +struct an8855_phy_bus *phy_bus_info) +{ +struct mdio_regmap_config mrc = { }; +struct an8855_phy *phy_ddata; +struct regmap *regmap; +u32 addr, phy_offset;
drivers/mfd/airoha-an8855.c line 420
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+struct mii_bus *bus; +int ret; + + ret =of_property_read_u32 (phy_np,"reg" , &addr); +if (ret) +return ret; + + phy_offset = addr - ddata->switch_addr; +if (phy_offset >=AN8855_MAX_PHY_PORT ) +return -EINVAL ;
[Severity: High]
Can this cause out-of-bounds accesses on bus->irq[] and bus->mdio_map[] if ddata->switch_addr is between 28 and 31 and a child PHY node specifies a reg value >= 32 (PHY_MAX_ADDR)?
In drivers/mfd/airoha-an8855.c, an8855_phy_bus_register() reads addr via of_property_read_u32() and only checks that addr - ddata->switch_addr is less than AN8855_MAX_PHY_PORT (5), without checking that addr < PHY_MAX_ADDR (32) or using of_mdio_parse_addr().
When an8855_probe() subsequently passes phy_bus_infos[i].addr to fwnode_mdiobus_register_phy() for a node with an ethernet-phy-id compatible string, the call chain:
an8855_probe()
fwnode_mdiobus_register_phy()
fwnode_mdiobus_phy_device_register()
phy_device_register()
mdiobus_register_device()
indexes mdio->irq[addr] and mdiodev->bus->mdio_map[mdiodev->addr] (both sized PHY_MAX_ADDR) and evaluates 1 << addr without bounds checking:
drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_phy_device_register() {
...
if (rc > 0) {
phy->irq = rc;
mdio->irq[addr] = rc;
} else {
phy->irq = mdio->irq[addr];
} if (fwnode_property_read_bool(child, "broken-turn-around"))
mdio->phy_ignore_ta_mask |= 1 << addr;
...}
drivers/net/phy/mdio_device.c:mdiobus_register_device() {
if (mdiodev->bus->mdio_map[mdiodev->addr])
return -EBUSY;
...
}Should an8855_phy_bus_register() also verify that addr < PHY_MAX_ADDR (or use of_mdio_parse_addr()) before accepting the PHY address?
[ ... ]
Quoted 8 lines
drivers/mfd/airoha-an8855.c new file +8
-0+static int an8855_probe (struct mdio_device *mdiodev) +{ +struct an8855_phy_bus phy_bus_infos[AN8855_MAX_PHY_PORT ] = { }; +struct device *dev = &mdiodev->dev; +struct regmap *regmap; +struct an8855 *ddata; +int num_phys; +int i, ret;
[ ... ]
Quoted 6 lines
drivers/mfd/airoha-an8855.c new file +6
-0+/* Reset PHY page for switch address */ +mutex_lock_nested (&ddata->bus->mdio_lock,MDIO_MUTEX_NESTED ); + ret =an8855_mii_set_page (ddata, ddata->switch_addr, +AN8855_PHY_PAGE_STANDARD ); +mutex_unlock (&ddata->bus->mdio_lock); +if (ret)
drivers/mfd/airoha-an8855.c line 536
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+ goto err_put_fwnodes; + + ret =an8855_read_switch_id (dev, regmap); +if (ret) + goto err_put_fwnodes; + +/* Release global PHY power down */ + ret =regmap_write (regmap,AN8855_RG_GPHY_AFE_PWD ,0x0 ); +if (ret) + goto err_put_fwnodes;
[Severity: High]
Does an8855_probe() need to request and deassert reset-gpios before accessing the switch registers over MDIO?
Documentation/devicetree/bindings/mfd/airoha,an8855.yaml defines reset-gpios on the airoha,an8855 node, and drivers/mfd/airoha-an8855.c includes linux/gpio/consumer.h, but never requests or deasserts the GPIO.
Because airoha,an8855 is a generic MDIO device rather than a PHY device, __of_mdiobus_parse_phys() registers it via of_mdiobus_register_device() without MDIO_DEVICE_FLAG_PHY set. In drivers/net/phy/mdio_device.c, mdiobus_register_device() only registers reset lines for PHY devices:
if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY) {
err = mdio_device_register_reset(mdiodev);
if (err)
return err; /* Assert the reset signal */
mdio_device_reset(mdiodev, 1);}
As a result, the MDIO core does not manage reset-gpios for this device. If the reset line is asserted when an8855_probe() runs, the MDIO accesses in an8855_mii_set_page() and an8855_read_switch_id() will fail; conversely, if a child MFD driver toggles reset-gpios later during devm_mfd_add_devices(), it would reset the switch and wipe out the AN8855_RG_GPHY_AFE_PWD write and internal PHY state initialized here in an8855_probe().
Quoted 6 lines
drivers/mfd/airoha-an8855.c new file +6
-0+ +/* Register PHYs */ +for (i =0 ; i < num_phys; i++) { + ret =fwnode_mdiobus_register_phy (phy_bus_infos[i].bus, + phy_bus_infos[i].node, + phy_bus_infos[i].addr);
drivers/mfd/airoha-an8855.c line 552
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+fwnode_handle_put (phy_bus_infos[i].node); + phy_bus_infos[i].node =NULL ; + +if (ret) + goto err_put_fwnodes; + } + +return devm_mfd_add_devices (dev,PLATFORM_DEVID_AUTO , an8855_cells, +ARRAY_SIZE (an8855_cells),NULL ,0 , +NULL );
[Severity: Medium]
Can registering the internal PHYs before the an8855-efuse MFD cell cause phy_attach_direct() to bind the fallback genphy_driver instead of the dedicated PHY driver?
In Documentation/devicetree/bindings/mfd/airoha,an8855.yaml, each internal ethernet-phy node consumes nvmem-cells from the an8855-efuse child node, which causes fw_devlink to create supplier links from the efuse node to the PHY nodes.
In an8855_probe(), fwnode_mdiobus_register_phy() is called for all internal PHYs before devm_mfd_add_devices() creates the an8855-efuse and an8855-switch devices. During fwnode_mdiobus_register_phy() -> phy_device_register() -> device_add(), each PHY defers probe with -EPROBE_DEFER because its nvmem supplier (an8855-efuse) has not been registered yet, leaving phydev->mdio.dev.driver as NULL on deferred_probe_pending_list.
Immediately afterwards, devm_mfd_add_devices() registers an8855-efuse (which queues deferred_probe_work asynchronously) and then synchronously registers an8855-switch. Because phy-handle is not tracked by fw_devlink, an8855-switch can probe and call dsa_register_switch() -> phylink_of_phy_connect() -> phy_attach_direct() before deferred_probe_work runs. When phy_attach_direct() sees that d->driver is NULL, it forcibly binds genphy_driver and removes the PHY from the deferred probe list:
drivers/net/phy/phy_device.c:phy_attach_direct() {
...
if (!d->driver) {
if (phydev->is_c45)
d->driver = &genphy_c45_driver.mdiodrv.driver;
else
d->driver = &genphy_driver.mdiodrv.driver; phydev->is_genphy_driven = 1;
}
...}
Should an8855-efuse be registered before fwnode_mdiobus_register_phy() (or the PHYs registered before an8855-switch after an8855-efuse is available) so the internal PHYs can bind to their dedicated driver without deferring?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Lee Jones
maintainerRe: [PATCH v2 2/2] mfd: an8855: Add support for Airoha AN8855 Switch
In reply to Christian MarangiPlease review, explain or fix the Sashiko review(s). If this is a set, this request includes the other patches too.
Quoted 62 lines
Add support for Airoha AN8855 Switch that provide support for a DSA switch and a NVMEM provider.
Also make use of the mdio-regmap driver and register a regmap for each internal PHY of the switch.
This is needed to handle the double usage of the PHYs as both PHY and Switch accessor.Signed-off-by Christian Marangi <[email protected]>--- drivers/mfd/Kconfig | 14 + drivers/mfd/Makefile | 1 + drivers/mfd/airoha-an8855.c | 579 ++++++++++++++++++++++++++++++++++++ 3 files changed, 594 insertions(+) create mode 100644 drivers/mfd/airoha-an8855.cdrivers/mfd/Kconfig +14
-0@@ -54,6 +54,20 @@ config MFD_ALTERA_SYSMGR using regmap_mmio accesses for ARM32 parts and SMC calls to EL3 for ARM64 parts. +config MFD_AIROHA_AN8855 + tristate "Airoha AN8855 Switch Core" + select MFD_CORE + select REGMAP + select PHYLIB + select MDIO_REGMAP + depends on NETDEVICES && OF + help + Support for the Airoha AN8855 Switch Core. This is an SoC + that provides various peripherals, to count, i2c, an Ethrnet + Switch, a CPU timer, GPIO, eFUSE. + + Currently it provides a DSA switch and a NVMEM provider. + config MFD_ACT8945A tristate "Active-semi ACT8945A" select MFD_COREdrivers/mfd/Makefile +1
-0@@ -8,6 +8,7 @@ obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o obj-$(CONFIG_MFD_88PM800) += 88pm800.o 88pm80x.o obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o +obj-$(CONFIG_MFD_AIROHA_AN8855) += airoha-an8855.o obj-$(CONFIG_MFD_ACT8945A) += act8945a.o obj-$(CONFIG_MFD_SM501) += sm501.o obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.odrivers/mfd/airoha-an8855.c new file +3
-0@@ -0,0 +1,579 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Core driver for Airoha AN8855 Switch
drivers/mfd/airoha-an8855.c line 13
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+* +* Copyright (C) 2024 Christian Marangi <[email protected]> +*/ + +#include <linux/bitfield.h> +#include <linux/fwnode_mdio.h> +#include <linux/gpio/consumer.h> +#include <linux/mfd/core.h> +#include <linux/mdio.h> +#include <linux/mdio/mdio-regmap.h>
Alphabetical.
Quoted 8 lines
drivers/mfd/airoha-an8855.c new file +8
-0+#include <linux/module.h> +#include <linux/of.h> +#include <linux/phy.h> +#include <linux/regmap.h> + +/* Register for HW trap status */ +#define AN8855_HWTRAP 0x1000009c +
drivers/mfd/airoha-an8855.c line 31
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+/* +* Register of the Switch ID +* (called Project ID in Documentation) +*/ +#define AN8855_CREV 0x10005000 +#define AN8855_ID 0x8855/* Switch ID */ + +/* Register for GPHY Power Down +* Used to Toggle the Gigabit PHY power and enable them. +*/
This is an incorrectly formatted multi-line comment.
Quoted 37 lines
drivers/mfd/airoha-an8855.c new file +37
-0+#define AN8855_RG_GPHY_AFE_PWD 0x1028c840 + +/* MII Registers */ +#define AN8855_PHY_SELECT_PAGE 0x1f +#define AN8855_PHY_PAGE GENMASK(2, 0) +#define AN8855_PHY_PAGE_STANDARD FIELD_PREP_CONST(AN8855_PHY_PAGE, 0x0) +#define AN8855_PHY_PAGE_EXTENDED_1 FIELD_PREP_CONST(AN8855_PHY_PAGE, 0x1) +#define AN8855_PHY_PAGE_EXTENDED_4 FIELD_PREP_CONST(AN8855_PHY_PAGE, 0x4) + +/* MII Registers Page 4 */ +#define AN8855_PBUS_MODE 0x10 +#define AN8855_PBUS_MODE_ADDR_FIXED 0x0 +#define AN8855_PBUS_MODE_ADDR_INCR BIT(15) +#define AN8855_PBUS_WR_ADDR_HIGH 0x11 +#define AN8855_PBUS_WR_ADDR_LOW 0x12 +#define AN8855_PBUS_WR_DATA_HIGH 0x13 +#define AN8855_PBUS_WR_DATA_LOW 0x14 +#define AN8855_PBUS_RD_ADDR_HIGH 0x15 +#define AN8855_PBUS_RD_ADDR_LOW 0x16 +#define AN8855_PBUS_RD_DATA_HIGH 0x17 +#define AN8855_PBUS_RD_DATA_LOW 0x18 + +#define AN8855_MAX_PHY_PORT 5 + +struct an8855 { +struct mii_bus *bus; +struct mii_bus *phy_bus; +unsigned int switch_addr; +u16 current_page; +}; + +struct an8855_phy { +u8 addr; +struct an8855 *core; +}; + +struct an8855_phy_bus {
drivers/mfd/airoha-an8855.c line 78
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+struct mii_bus *bus; +struct fwnode_handle *node; +u8 addr; +}; + +static const struct mfd_cell an8855_cells[] = { +MFD_CELL_OF ("an8855-efuse" ,NULL ,NULL ,0 ,0 , +"airoha,an8855-efuse" ), +MFD_CELL_OF ("an8855-switch" ,NULL ,NULL ,0 ,0 , +"airoha,an8855-switch" ),
Use 100-chars to avoid these pointless wraps.
They also appear to be shorter than some following lines.
drivers/mfd/airoha-an8855.c line 82
Quoted 4 lines
drivers/mfd/airoha-an8855.c new file +4
-0+}; + +static int an8855_mii_set_page(struct an8855 *ddata, u8 addr, + u8 page) __must_hold(&ddata->bus->mdio_lock)
This should line-up with the ( and is that 8 spaces?
drivers/mfd/airoha-an8855.c line 89
Quoted 7 lines
drivers/mfd/airoha-an8855.c new file +7
-0+{ +struct mii_bus *bus = ddata->bus; +int ret; + + ret =__mdiobus_write (bus, addr,AN8855_PHY_SELECT_PAGE , page); +if (ret) { +dev_err_ratelimited (&bus->dev,"failed to set mii page\n" );
Can we start sentences with an upper-case char?
Quoted 31 lines
drivers/mfd/airoha-an8855.c new file +31
-0+return ret; + } + +/* Cache current page if next MII read/write is for Switch page */ + ddata->current_page = page; +return 0 ; +} + +static int an8855_mii_read32 (struct mii_bus *bus,u8 phy_id,u32 reg, + u32 *val) __must_hold(&bus->mdio_lock) +{ +int lo, hi, ret; + + ret =__mdiobus_write (bus, phy_id,AN8855_PBUS_MODE , +AN8855_PBUS_MODE_ADDR_FIXED ); +if (ret) + goto err; + + ret =__mdiobus_write (bus, phy_id,AN8855_PBUS_RD_ADDR_HIGH , +upper_16_bits (reg)); +if (ret) + goto err; + + ret =__mdiobus_write (bus, phy_id,AN8855_PBUS_RD_ADDR_LOW , +lower_16_bits (reg)); +if (ret) + goto err; + + hi =__mdiobus_read (bus, phy_id,AN8855_PBUS_RD_DATA_HIGH ); +if (hi <0 ) { + ret = hi;
drivers/mfd/airoha-an8855.c line 130
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+ goto err; + } + + lo =__mdiobus_read (bus, phy_id,AN8855_PBUS_RD_DATA_LOW ); +if (lo <0 ) { + ret = lo; + goto err; + } + + *val = ((u32 )hi <<16 ) | lo;
No magic numbers please.
Quoted 4 lines
drivers/mfd/airoha-an8855.c new file +4
-0+return 0 ; +err: +dev_err_ratelimited (&bus->dev,"failed to read register\n" ); +return ret;
drivers/mfd/airoha-an8855.c line 145
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+} + +/* +* Protect concurrent access to phy page if switch address also +* expose a PHY. +*/ +static void an8855_regmap_lock (struct an8855 *ddata) +{ +if (ddata->phy_bus) +mutex_lock (&ddata->phy_bus->mdio_lock);
Nit: \n
drivers/mfd/airoha-an8855.c line 151
Quoted 6 lines
drivers/mfd/airoha-an8855.c new file +6
-0+mutex_lock_nested (&ddata->bus->mdio_lock,MDIO_MUTEX_NESTED ); +} + +static void an8855_regmap_unlock (struct an8855 *ddata) +{ +mutex_unlock (&ddata->bus->mdio_lock);
Nit: \n
drivers/mfd/airoha-an8855.c line 163
Quoted 12 lines
drivers/mfd/airoha-an8855.c new file +12
-0+if (ddata->phy_bus) +mutex_unlock (&ddata->phy_bus->mdio_lock); +} + +static int an8855_regmap_read (void *ctx,uint32_t reg,uint32_t *val) +{ +struct an8855 *ddata = ctx; +struct mii_bus *bus = ddata->bus; +u16 addr = ddata->switch_addr; +int ret; + +an8855_regmap_lock (ddata);
Nit: \n
drivers/mfd/airoha-an8855.c line 164
Quoted 2 lines
drivers/mfd/airoha-an8855.c new file +2
-0+ ret =an8855_mii_set_page (ddata, addr,AN8855_PHY_PAGE_EXTENDED_4 ); +if (ret <0 )
Can an8855_mii_set_page() return positive values?
If not, please use 'if (ret)'.
drivers/mfd/airoha-an8855.c line 173
Quoted 8 lines
drivers/mfd/airoha-an8855.c new file +8
-0+ goto exit; + + ret =an8855_mii_read32 (bus, addr, reg, val); + +exit: +an8855_regmap_unlock (ddata); + +return ret <0 ? ret :0 ;
Both an8855_mii_set_page() and an8855_mii_read32() only return 0 or a negative error code. Can't you just 'return ret;' here and in an8855_regmap_write() and an8855_regmap_update_bits()?
Quoted 18 lines
drivers/mfd/airoha-an8855.c new file +18
-0+} + +static int an8855_mii_write32 (struct mii_bus *bus,u8 phy_id,u32 reg, + u32 val) __must_hold(&bus->mdio_lock) +{ +int ret; + + ret =__mdiobus_write (bus, phy_id,AN8855_PBUS_MODE , +AN8855_PBUS_MODE_ADDR_FIXED ); +if (ret) + goto err; + + ret =__mdiobus_write (bus, phy_id,AN8855_PBUS_WR_ADDR_HIGH , +upper_16_bits (reg)); +if (ret) + goto err; + ret =__mdiobus_write (bus, phy_id,AN8855_PBUS_WR_ADDR_LOW , +lower_16_bits (reg));
drivers/mfd/airoha-an8855.c line 201
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+if (ret) + goto err; + + ret =__mdiobus_write (bus, phy_id,AN8855_PBUS_WR_DATA_HIGH , +upper_16_bits (val)); +if (ret) + goto err; + + ret =__mdiobus_write (bus, phy_id,AN8855_PBUS_WR_DATA_LOW , +lower_16_bits (val));
100-chars throughout.
Quoted 36 lines
drivers/mfd/airoha-an8855.c new file +36
-0+if (ret) + goto err; + +return 0 ; +err: +dev_err_ratelimited (&bus->dev, +"failed to write an8855 register\n" ); +return ret; +} + +static int an8855_regmap_write (void *ctx,uint32_t reg,uint32_t val) +{ +struct an8855 *ddata = ctx; +struct mii_bus *bus = ddata->bus; +u16 addr = ddata->switch_addr; +int ret; + +an8855_regmap_lock (ddata); + ret =an8855_mii_set_page (ddata, addr,AN8855_PHY_PAGE_EXTENDED_4 ); +if (ret) + goto exit; + + ret =an8855_mii_write32 (bus, addr, reg, val); + +exit: +an8855_regmap_unlock (ddata); + +return ret <0 ? ret :0 ; +} + +static int an8855_regmap_update_bits (void *ctx,uint32_t reg,uint32_t mask, +uint32_t write_val) +{ +struct an8855 *ddata = ctx; +struct mii_bus *bus = ddata->bus; +u16 addr = ddata->switch_addr;
drivers/mfd/airoha-an8855.c line 247
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+u32 val; +int ret; + +an8855_regmap_lock (ddata); + ret =an8855_mii_set_page (ddata, addr,AN8855_PHY_PAGE_EXTENDED_4 ); +if (ret) + goto exit; + + ret =an8855_mii_read32 (bus, addr, reg, &val); +if (ret <0 )
Same here - 'if (ret)'.
Quoted 78 lines
drivers/mfd/airoha-an8855.c new file +78
-0+ goto exit; + + val &= ~mask; + val |= write_val & mask; + ret =an8855_mii_write32 (bus, addr, reg, val); + +exit: +an8855_regmap_unlock (ddata); + +return ret <0 ? ret :0 ; +} + +static const struct regmap_range an8855_readable_ranges[] = { +regmap_reg_range (0x10000000 ,0x10000fff ),/* SCU */ +regmap_reg_range (0x10001000 ,0x10001fff ),/* RBUS */ +regmap_reg_range (0x10002000 ,0x10002fff ),/* MCU */ +regmap_reg_range (0x10005000 ,0x10005fff ),/* SYS SCU */ +regmap_reg_range (0x10007000 ,0x10007fff ),/* I2C Slave */ +regmap_reg_range (0x10008000 ,0x10008fff ),/* I2C Master */ +regmap_reg_range (0x10009000 ,0x10009fff ),/* PDMA */ +regmap_reg_range (0x1000a100 ,0x1000a1ff ),/* General Purpose Timer */ +regmap_reg_range (0x1000a200 ,0x1000a2ff ),/* GPU timer */ +regmap_reg_range (0x1000a300 ,0x1000a3ff ),/* GPIO */ +regmap_reg_range (0x1000a400 ,0x1000a5ff ),/* EFUSE */ +regmap_reg_range (0x1000c000 ,0x1000cfff ),/* GDMP CSR */ +regmap_reg_range (0x10010000 ,0x1001ffff ),/* GDMP SRAM */ +regmap_reg_range (0x10200000 ,0x10203fff ),/* Switch - ARL Global */ +regmap_reg_range (0x10204000 ,0x10207fff ),/* Switch - BMU */ +regmap_reg_range (0x10208000 ,0x1020bfff ),/* Switch - ARL Port */ +regmap_reg_range (0x1020c000 ,0x1020cfff ),/* Switch - SCH */ +regmap_reg_range (0x10210000 ,0x10213fff ),/* Switch - MAC */ +regmap_reg_range (0x10214000 ,0x10217fff ),/* Switch - MIB */ +regmap_reg_range (0x10218000 ,0x1021bfff ),/* Switch - Port Control */ +regmap_reg_range (0x1021c000 ,0x1021ffff ),/* Switch - TOP */ +regmap_reg_range (0x10220000 ,0x1022ffff ),/* SerDes */ +regmap_reg_range (0x10286000 ,0x10286fff ),/* RG Batcher */ +regmap_reg_range (0x1028c000 ,0x1028ffff ),/* ETHER_SYS */ +regmap_reg_range (0x30000000 ,0x37ffffff ),/* I2C EEPROM */ +regmap_reg_range (0x38000000 ,0x3fffffff ),/* BOOT_ROM */ +regmap_reg_range (0xa0000000 ,0xbfffffff ),/* GPHY */ +}; + +static const struct regmap_access_table an8855_readable_table = { + .yes_ranges = an8855_readable_ranges, + .n_yes_ranges =ARRAY_SIZE (an8855_readable_ranges), +}; + +static const struct regmap_config an8855_regmap_config = { + .name ="switch" , + .reg_bits =32 , + .val_bits =32 , + .reg_stride =4 , + .max_register =0xbfffffff , + .reg_read = an8855_regmap_read, + .reg_write = an8855_regmap_write, + .reg_update_bits = an8855_regmap_update_bits, + .disable_locking = true, + .rd_table = &an8855_readable_table, +}; + +static int an8855_regmap_phy_reset_page (struct an8855 *ddata, + int phy) __must_hold(&ddata->bus->mdio_lock) +{ +/* Check PHY page only for addr shared with switch */ +if (phy != ddata->switch_addr) +return 0 ; + +/* Don't restore page if it's not set to Switch page */ +if (ddata->current_page !=AN8855_PHY_PAGE_EXTENDED_4 ) +return 0 ; + +/* +* Restore page to 0, PHY might change page right after but that +* will be ignored as it won't be a switch page. +*/ +return an8855_mii_set_page (ddata, phy,AN8855_PHY_PAGE_STANDARD ); +} +
drivers/mfd/airoha-an8855.c line 335
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+static int an8855_regmap_phy_read (void *ctx,uint32_t reg,uint32_t *val) +{ +struct an8855_phy *ddata = ctx; +struct an8855 *core_ddata; +u32 addr = ddata->addr; +struct mii_bus *bus; +int ret; + + core_ddata = ddata->core; + bus = core_ddata->bus;
Why not initialise 'core_ddata' and 'bus' when declaring them?
Quoted 16 lines
drivers/mfd/airoha-an8855.c new file +16
-0+ +mutex_lock_nested (&bus->mdio_lock,MDIO_MUTEX_NESTED ); + ret =an8855_regmap_phy_reset_page (core_ddata, addr); +if (ret) + goto exit; + + ret =__mdiobus_read (bus, addr, reg); +if (ret >=0 ) + *val = ret; + +exit: +mutex_unlock (&bus->mdio_lock); + +return ret <0 ? ret :0 ; +} +
drivers/mfd/airoha-an8855.c line 361
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+static int an8855_regmap_phy_write (void *ctx,uint32_t reg,uint32_t val) +{ +struct an8855_phy *ddata = ctx; +struct an8855 *core_ddata; +u32 addr = ddata->addr; +struct mii_bus *bus; +int ret; + + core_ddata = ddata->core; + bus = core_ddata->bus;
Same here.
Quoted 34 lines
drivers/mfd/airoha-an8855.c new file +34
-0+ +mutex_lock_nested (&bus->mdio_lock,MDIO_MUTEX_NESTED ); + ret =an8855_regmap_phy_reset_page (core_ddata, addr); +if (ret) + goto exit; + + ret =__mdiobus_write (bus, addr, reg, val); + +exit: +mutex_unlock (&bus->mdio_lock); + +return ret; +} + +static const struct regmap_config an8855_phy_regmap_config = { + .reg_bits =16 , + .val_bits =16 , + .reg_read = an8855_regmap_phy_read, + .reg_write = an8855_regmap_phy_write, + .disable_locking = true, + .max_register =0x1f , +}; + +static int an8855_read_switch_id (struct device *dev,struct regmap *regmap) +{ +u32 id; +int ret; + + ret =regmap_read (regmap,AN8855_CREV , &id); +if (ret) +return ret; + +if (id !=AN8855_ID ) { + dev_err(dev, "Detected Switch ID %x but %x was expected\n",
drivers/mfd/airoha-an8855.c line 405
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+ id, AN8855_ID); +return -ENODEV ; + } + +return 0 ; +} + +static int an8855_phy_bus_register (struct device *dev,struct an8855 *ddata, +struct device_node *phy_np, +struct an8855_phy_bus *phy_bus_info)
Why can't we do this in the phy driver?
Quoted 12 lines
drivers/mfd/airoha-an8855.c new file +12
-0+{ +struct mdio_regmap_config mrc = { }; +struct an8855_phy *phy_ddata; +struct regmap *regmap; +u32 addr, phy_offset; +struct mii_bus *bus; +int ret; + + ret =of_property_read_u32 (phy_np,"reg" , &addr); +if (ret) +return ret; +
drivers/mfd/airoha-an8855.c line 427
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+ phy_offset = addr - ddata->switch_addr; +if (phy_offset >=AN8855_MAX_PHY_PORT ) +return -EINVAL ; + + phy_ddata =devm_kzalloc (dev,sizeof (*phy_ddata),GFP_KERNEL ); +if (!phy_ddata) +return -ENOMEM ; + + phy_ddata->addr = addr; + phy_ddata->core = ddata;
Nested ddatas!
Quoted 27 lines
drivers/mfd/airoha-an8855.c new file +27
-0+ regmap =devm_regmap_init (dev,NULL , phy_ddata, &an8855_phy_regmap_config); +if (IS_ERR (regmap)) +return dev_err_probe (dev,PTR_ERR (regmap), +"phy%d regmap initialization failed\n" , + addr); + + mrc.regmap = regmap; + mrc.parent = dev; + mrc.valid_addr = addr; +snprintf (mrc.name,MII_BUS_ID_SIZE ,"%s-phy%u" ,dev_name (dev), addr); + + bus =devm_mdio_regmap_register (dev, &mrc); +if (IS_ERR (bus)) +return PTR_ERR (bus); + +if (addr == ddata->switch_addr) + ddata->phy_bus = bus; + + phy_bus_info->bus = bus; + phy_bus_info->addr = addr; + phy_bus_info->node =fwnode_handle_get (of_fwnode_handle (phy_np)); + +return 0 ; +} + +static int an8855_mdio_register (struct device *dev,struct an8855 *ddata, +struct an8855_phy_bus *phy_bus_infos)
drivers/mfd/airoha-an8855.c line 465
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+{ +struct device_node *mdio_np; +int ret =0 ; +int i =0 ; + + mdio_np =of_get_child_by_name (dev->of_node,"mdio" ); +if (!mdio_np) +return -ENODEV ; + +for_each_available_child_of_node_scoped (mdio_np, phy_np) {
Can you use __free(device_node) for 'mdio_np' too, or use the fwnode API consistently throughout instead of mixing 'of_*' and 'fwnode_*'?
drivers/mfd/airoha-an8855.c line 476
Quoted 11 lines
drivers/mfd/airoha-an8855.c new file +11
-0+if (i >=AN8855_MAX_PHY_PORT ) { + ret = -EINVAL ; + goto exit; + } + + ret =an8855_phy_bus_register (dev, ddata, phy_np, + &phy_bus_infos[i]); +if (ret) + goto exit; + + i++;
This isn't a meaningless iterator. Can we name it? 'phy'?
Quoted 57 lines
drivers/mfd/airoha-an8855.c new file +57
-0+ } + +exit: +of_node_put (mdio_np); +if (ret) { +while (i--) +fwnode_handle_put (phy_bus_infos[i].node); + +return ret; + } + +return i; +} + +static int an8855_probe (struct mdio_device *mdiodev) +{ +struct an8855_phy_bus phy_bus_infos[AN8855_MAX_PHY_PORT ] = { }; +struct device *dev = &mdiodev->dev; +struct regmap *regmap; +struct an8855 *ddata; +int num_phys; +int i, ret; + + ddata =devm_kzalloc (dev,sizeof (*ddata),GFP_KERNEL ); +if (!ddata) +return -ENOMEM ; + + ddata->bus = mdiodev->bus; + ddata->switch_addr = mdiodev->addr; +/* No DMA for mdiobus, mute warning for DMA mask not set */ + dev->dma_mask = &dev->coherent_dma_mask; + +/* Register virtual MDIO for each PHY */ + num_phys =an8855_mdio_register (dev, ddata, phy_bus_infos); +if (num_phys <0 ) +return num_phys; + + regmap =devm_regmap_init (dev,NULL , ddata, &an8855_regmap_config); +if (IS_ERR (regmap)) { + ret =PTR_ERR (regmap); +dev_err_probe (dev, ret,"regmap initialization failed\n" ); + goto err_put_fwnodes; + } + +/* Reset PHY page for switch address */ +mutex_lock_nested (&ddata->bus->mdio_lock,MDIO_MUTEX_NESTED ); + ret =an8855_mii_set_page (ddata, ddata->switch_addr, +AN8855_PHY_PAGE_STANDARD ); +mutex_unlock (&ddata->bus->mdio_lock); +if (ret) + goto err_put_fwnodes; + + ret =an8855_read_switch_id (dev, regmap); +if (ret) + goto err_put_fwnodes; + +/* Release global PHY power down */
drivers/mfd/airoha-an8855.c line 543
Quoted 10 lines
drivers/mfd/airoha-an8855.c new file +10
-0+ ret =regmap_write (regmap,AN8855_RG_GPHY_AFE_PWD ,0x0 ); +if (ret) + goto err_put_fwnodes; + +/* Register PHYs */ +for (i =0 ; i < num_phys; i++) { + ret =fwnode_mdiobus_register_phy (phy_bus_infos[i].bus, + phy_bus_infos[i].node, + phy_bus_infos[i].addr); +fwnode_handle_put (phy_bus_infos[i].node);
Why do we need 'an8855_phy_bus' and two separate loops at all? If you initialise the switch regmap, verify the Switch ID, and release 'AN8855_RG_GPHY_AFE_PWD' before calling an8855_mdio_register(), can't an8855_phy_bus_register() call fwnode_mdiobus_register_phy() directly? That would allow you to drop 'struct an8855_phy_bus', the 'phy_bus_infos' array, and all of the manual fwnode_handle_get() / fwnode_handle_put() error-unwinding.
Or, better yet, move all of this phy stuff out of MFD, which shouldn't have to know all of this stuff.
drivers/mfd/airoha-an8855.c line 555
Quoted 12 lines
drivers/mfd/airoha-an8855.c new file +12
-0+ phy_bus_infos[i].node =NULL ; + +if (ret) + goto err_put_fwnodes; + } + +return devm_mfd_add_devices (dev,PLATFORM_DEVID_AUTO , an8855_cells, +ARRAY_SIZE (an8855_cells),NULL ,0 , +NULL ); + +err_put_fwnodes: +for (i =0 ; i < num_phys; i++)
'phy' and 'for (int phy = 0; ...'
drivers/mfd/airoha-an8855.c line 568
Quoted 13 lines
drivers/mfd/airoha-an8855.c new file +13
-0+if (phy_bus_infos[i].node) +fwnode_handle_put (phy_bus_infos[i].node); + +return ret; +} + +static const struct of_device_id an8855_of_match[] = { + { .compatible ="airoha,an8855" }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, an8855_of_match); + +static struct mdio_driver an8855_driver = {
So this isn't a platform driver?
Not sure how we feel about splitting an mdio driver into various platform drivers?
Quoted 14 lines
drivers/mfd/airoha-an8855.c new file +11
-0+ .probe = an8855_probe, + .mdiodrv.driver = { + .name = "an8855", + .of_match_table = an8855_of_match, + }, +}; +mdio_module_driver (an8855_driver); + +MODULE_AUTHOR ("Christian Marangi <[email protected]>" ); +MODULE_DESCRIPTION ("Driver for Airoha AN8855" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.55.0
Signature · 1 line
Lee Jones