| Message ID | trinity-e71bfb76-697e-4f08-a106-40cb6672054f-1726083287252@3c-app-gmx-bs04 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: tn40xx: add support for AQR105 based cards (was: net: phy: aquantia: emable firmware loading for aqr105) | expand |
Hi, Thanks a lot for the work! On Wed, 11 Sep 2024 21:34:47 +0200 Hans-Frieder Vogt <hfdevel@gmx.net> wrote: > diff --git a/drivers/net/ethernet/tehuti/tn40.c b/drivers/net/ethernet/tehuti/tn40.c > index 4e6f2f781ffc..240a79a08d58 100644 > --- a/drivers/net/ethernet/tehuti/tn40.c > +++ b/drivers/net/ethernet/tehuti/tn40.c > @@ -1781,7 +1781,7 @@ static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > ret = tn40_phy_register(priv); > if (ret) { > dev_err(&pdev->dev, "failed to set up PHY.\n"); > - goto err_free_irq; > + goto err_unregister_swnodes; > } > > ret = tn40_priv_init(priv); > @@ -1798,6 +1798,10 @@ static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > return 0; > err_unregister_phydev: > tn40_phy_unregister(priv); > +err_unregister_swnodes: > + device_remove_software_node(&priv->mdio->dev); > + software_node_unregister_node_group(priv->nodes.group); > + software_node_unregister(priv->nodes.group[SWNODE_MDIO]); Why this workaround is necessary? The problem lies on software node side? > diff --git a/drivers/net/ethernet/tehuti/tn40_mdio.c b/drivers/net/ethernet/tehuti/tn40_mdio.c > index af18615d64a8..bbd95fabbea0 100644 > --- a/drivers/net/ethernet/tehuti/tn40_mdio.c > +++ b/drivers/net/ethernet/tehuti/tn40_mdio.c > @@ -14,6 +14,8 @@ > (FIELD_PREP(TN40_MDIO_PRTAD_MASK, (port)))) > #define TN40_MDIO_CMD_READ BIT(15) > > +#define AQR105_FIRMWARE "tehuti/aqr105-tn40xx.cld" This firmware is for AQR PHY so aquantia directory is the better place? > +static int tn40_swnodes_register(struct tn40_priv *priv) > +{ > + struct tn40_nodes *nodes = &priv->nodes; > + struct pci_dev *pdev = priv->pdev; > + struct software_node *swnodes; > + u32 id; > + > + id = pci_dev_id(pdev); > + > + snprintf(nodes->phy_name, sizeof(nodes->phy_name), "tn40_aqr105_phy"); This doesn't work on a system having multiple TN40 cards because it tries create duplicated sysfs directory. I uses a machine with TN9310 (QT2025 PHY) and TN9510 (AQR105 PHY). > +MODULE_FIRMWARE(AQR105_FIRMWARE); AQR PHY driver better to have the above? Otherwise, how about adding it to tn40.c? It already has MODULE_FIRMWARE(TN40_FIRMWARE_NAME).
> > #define TN40_MDIO_CMD_READ BIT(15) > > > > +#define AQR105_FIRMWARE "tehuti/aqr105-tn40xx.cld" > > This firmware is for AQR PHY so aquantia directory is the better > place? I don't know the correct answer to this. One thing to consider is how it gets packaged. on my Debian system: p firmware-adi - Binary firmware for Analog Devices Inc. DSL modem chips (dummmy pac i firmware-amd-graphics - Binary firmware for AMD/ATI graphics chips p firmware-ast - Binary firmware for ASpeed Technologies graphics chips p firmware-ath9k-htc - firmware for AR7010 and AR9271 USB wireless adapters p firmware-ath9k-htc-dbgsym - QCA ath9k-htc Firmware ELF file p firmware-atheros - Binary firmware for Qualcomm Atheros wireless cards p firmware-b43-installer - firmware installer for the b43 driver p firmware-b43legacy-installer - firmware installer for the b43legacy driver p firmware-bnx2 - Binary firmware for Broadcom NetXtremeII p firmware-bnx2x - Binary firmware for Broadcom NetXtreme II 10Gb p firmware-brcm80211 - Binary firmware for Broadcom/Cypress 802.11 wireless cards It seems to get packaged by vendor. Given the mess aquantia firmware is, we are going to end up with lots of firmwares in firmware-aquantia which are never needed. If the firmware is placed into tehuti, installing firmware-tehuti gives you just what you need. So i can see the logic of tehuti/aqr105-tn40xx.cld Andrew
On Fri, 13 Sep 2024 14:46:06 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> > +#define AQR105_FIRMWARE "tehuti/aqr105-tn40xx.cld" >> >> This firmware is for AQR PHY so aquantia directory is the better >> place? > > I don't know the correct answer to this. One thing to consider is how > it gets packaged. > > on my Debian system: > > p firmware-adi - Binary firmware for Analog Devices Inc. DSL modem chips (dummmy pac > i firmware-amd-graphics - Binary firmware for AMD/ATI graphics chips > p firmware-ast - Binary firmware for ASpeed Technologies graphics chips > p firmware-ath9k-htc - firmware for AR7010 and AR9271 USB wireless adapters > p firmware-ath9k-htc-dbgsym - QCA ath9k-htc Firmware ELF file > p firmware-atheros - Binary firmware for Qualcomm Atheros wireless cards > p firmware-b43-installer - firmware installer for the b43 driver > p firmware-b43legacy-installer - firmware installer for the b43legacy driver > p firmware-bnx2 - Binary firmware for Broadcom NetXtremeII > p firmware-bnx2x - Binary firmware for Broadcom NetXtreme II 10Gb > p firmware-brcm80211 - Binary firmware for Broadcom/Cypress 802.11 wireless cards > > It seems to get packaged by vendor. Given the mess aquantia firmware > is, we are going to end up with lots of firmwares in firmware-aquantia > which are never needed. If the firmware is placed into tehuti, > installing firmware-tehuti gives you just what you need. Understood. Looks like "tehuti" directory makes things easier if firmware could be get packaged in the way. On my Ubuntu system, one linux-firmware package installs many firmware.
On 13.09.2024 08.55, FUJITA Tomonori wrote: > Hi, > Thanks a lot for the work! Thank you! You did the biggest part of bringing tn40xx into mainline! > > On Wed, 11 Sep 2024 21:34:47 +0200 > Hans-Frieder Vogt <hfdevel@gmx.net> wrote: > >> diff --git a/drivers/net/ethernet/tehuti/tn40.c b/drivers/net/ethernet/tehuti/tn40.c >> index 4e6f2f781ffc..240a79a08d58 100644 >> --- a/drivers/net/ethernet/tehuti/tn40.c >> +++ b/drivers/net/ethernet/tehuti/tn40.c >> @@ -1781,7 +1781,7 @@ static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent) >> ret = tn40_phy_register(priv); >> if (ret) { >> dev_err(&pdev->dev, "failed to set up PHY.\n"); >> - goto err_free_irq; >> + goto err_unregister_swnodes; >> } >> >> ret = tn40_priv_init(priv); >> @@ -1798,6 +1798,10 @@ static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent) >> return 0; >> err_unregister_phydev: >> tn40_phy_unregister(priv); >> +err_unregister_swnodes: >> + device_remove_software_node(&priv->mdio->dev); >> + software_node_unregister_node_group(priv->nodes.group); >> + software_node_unregister(priv->nodes.group[SWNODE_MDIO]); > Why this workaround is necessary? The problem lies on software node > side? I don't understand it either. Need to further dig into, where the usage counters are incremented and how to get them decremented again. > >> diff --git a/drivers/net/ethernet/tehuti/tn40_mdio.c b/drivers/net/ethernet/tehuti/tn40_mdio.c >> index af18615d64a8..bbd95fabbea0 100644 >> --- a/drivers/net/ethernet/tehuti/tn40_mdio.c >> +++ b/drivers/net/ethernet/tehuti/tn40_mdio.c >> @@ -14,6 +14,8 @@ >> (FIELD_PREP(TN40_MDIO_PRTAD_MASK, (port)))) >> #define TN40_MDIO_CMD_READ BIT(15) >> >> +#define AQR105_FIRMWARE "tehuti/aqr105-tn40xx.cld" > This firmware is for AQR PHY so aquantia directory is the better > place? this has been addressed by Andrew already. > >> +static int tn40_swnodes_register(struct tn40_priv *priv) >> +{ >> + struct tn40_nodes *nodes = &priv->nodes; >> + struct pci_dev *pdev = priv->pdev; >> + struct software_node *swnodes; >> + u32 id; >> + >> + id = pci_dev_id(pdev); >> + >> + snprintf(nodes->phy_name, sizeof(nodes->phy_name), "tn40_aqr105_phy"); > This doesn't work on a system having multiple TN40 cards because it > tries create duplicated sysfs directory. > > I uses a machine with TN9310 (QT2025 PHY) and TN9510 (AQR105 PHY). Thanks, I didn't think of this problem. I have a solution though, and will correct it in the next version of the patch. > >> +MODULE_FIRMWARE(AQR105_FIRMWARE); > AQR PHY driver better to have the above? Otherwise, how about adding > it to tn40.c? It already has MODULE_FIRMWARE(TN40_FIRMWARE_NAME). I understand your rationale, but moving MODULE_FIRMWARE to tn40.c would require moving the definition of AQR105_FIRMWARE to tn40.h. And I would prefer to keep definitions as local as possible. Thanks! Hans
diff --git a/drivers/net/ethernet/tehuti/tn40.c b/drivers/net/ethernet/tehuti/tn40.c index 4e6f2f781ffc..240a79a08d58 100644 --- a/drivers/net/ethernet/tehuti/tn40.c +++ b/drivers/net/ethernet/tehuti/tn40.c @@ -1781,7 +1781,7 @@ static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent) ret = tn40_phy_register(priv); if (ret) { dev_err(&pdev->dev, "failed to set up PHY.\n"); - goto err_free_irq; + goto err_unregister_swnodes; } ret = tn40_priv_init(priv); @@ -1798,6 +1798,10 @@ static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent) return 0; err_unregister_phydev: tn40_phy_unregister(priv); +err_unregister_swnodes: + device_remove_software_node(&priv->mdio->dev); + software_node_unregister_node_group(priv->nodes.group); + software_node_unregister(priv->nodes.group[SWNODE_MDIO]); err_free_irq: pci_free_irq_vectors(pdev); err_unset_drvdata: @@ -1819,6 +1823,10 @@ static void tn40_remove(struct pci_dev *pdev) unregister_netdev(ndev); tn40_phy_unregister(priv); + /* cleanup software nodes */ + device_remove_software_node(&priv->mdio->dev); + software_node_unregister_node_group(priv->nodes.group); + software_node_unregister(priv->nodes.group[SWNODE_MDIO]); pci_free_irq_vectors(priv->pdev); pci_set_drvdata(pdev, NULL); iounmap(priv->regs); diff --git a/drivers/net/ethernet/tehuti/tn40.h b/drivers/net/ethernet/tehuti/tn40.h index 490781fe5120..1897c79333f8 100644 --- a/drivers/net/ethernet/tehuti/tn40.h +++ b/drivers/net/ethernet/tehuti/tn40.h @@ -4,6 +4,7 @@ #ifndef _TN40_H_ #define _TN40_H_ +#include <linux/property.h> #include "tn40_regs.h" #define TN40_DRV_NAME "tn40xx" @@ -102,10 +103,34 @@ struct tn40_txdb { int size; /* Number of elements in the db */ }; +#define NODE_PROP(_NAME, _PROP) ( \ + (const struct software_node) { \ + .name = _NAME, \ + .properties = _PROP, \ + }) + +enum tn40_swnodes { + SWNODE_MDIO, + SWNODE_PHY, + SWNODE_MAX +}; + +struct tn40_nodes { + char phy_name[32]; + char mdio_name[32]; + struct property_entry phy_props[2]; + struct property_entry mdio_props[1]; + struct software_node_ref_args phy_ref[1]; + struct software_node swnodes[SWNODE_MAX]; + const struct software_node *group[SWNODE_MAX + 1]; +}; + struct tn40_priv { struct net_device *ndev; struct pci_dev *pdev; + struct tn40_nodes nodes; + struct napi_struct napi; /* RX FIFOs: 1 for data (full) descs, and 2 for free descs */ struct tn40_rxd_fifo rxd_fifo0; diff --git a/drivers/net/ethernet/tehuti/tn40_mdio.c b/drivers/net/ethernet/tehuti/tn40_mdio.c index af18615d64a8..bbd95fabbea0 100644 --- a/drivers/net/ethernet/tehuti/tn40_mdio.c +++ b/drivers/net/ethernet/tehuti/tn40_mdio.c @@ -14,6 +14,8 @@ (FIELD_PREP(TN40_MDIO_PRTAD_MASK, (port)))) #define TN40_MDIO_CMD_READ BIT(15) +#define AQR105_FIRMWARE "tehuti/aqr105-tn40xx.cld" + static void tn40_mdio_set_speed(struct tn40_priv *priv, u32 speed) { void __iomem *regs = priv->regs; @@ -111,6 +113,44 @@ static int tn40_mdio_write_c45(struct mii_bus *mii_bus, int addr, int devnum, return tn40_mdio_write(mii_bus->priv, addr, devnum, regnum, val); } +/* registers an mdio handle and an aqr105 PHY + * tn40_mdio-%id { + * phy-handle = <&tn40_aqr105_phy>; + * }; + * tn40_aqr105_phy { + * compatible = "ethernet-phy-id03a1.b4a3"; + * firmware-name = AQR105_FIRMWARE; + * }; + */ +static int tn40_swnodes_register(struct tn40_priv *priv) +{ + struct tn40_nodes *nodes = &priv->nodes; + struct pci_dev *pdev = priv->pdev; + struct software_node *swnodes; + u32 id; + + id = pci_dev_id(pdev); + + snprintf(nodes->phy_name, sizeof(nodes->phy_name), "tn40_aqr105_phy"); + snprintf(nodes->mdio_name, sizeof(nodes->mdio_name), "tn40_mdio-%x", + id); + + swnodes = nodes->swnodes; + + nodes->phy_props[0] = PROPERTY_ENTRY_STRING("compatible", + "ethernet-phy-id03a1.b4a3"); + nodes->phy_props[1] = PROPERTY_ENTRY_STRING("firmware-name", + AQR105_FIRMWARE); + swnodes[SWNODE_PHY] = NODE_PROP(nodes->phy_name, nodes->phy_props); + nodes->phy_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_PHY]); + + nodes->mdio_props[0] = PROPERTY_ENTRY_REF_ARRAY("phy", nodes->phy_ref); + swnodes[SWNODE_MDIO] = NODE_PROP(nodes->mdio_name, nodes->mdio_props); + nodes->group[SWNODE_PHY] = &swnodes[SWNODE_PHY]; + nodes->group[SWNODE_MDIO] = &swnodes[SWNODE_MDIO]; + return software_node_register_node_group(nodes->group); +} + int tn40_mdiobus_init(struct tn40_priv *priv) { struct pci_dev *pdev = priv->pdev; @@ -130,13 +170,34 @@ int tn40_mdiobus_init(struct tn40_priv *priv) bus->read_c45 = tn40_mdio_read_c45; bus->write_c45 = tn40_mdio_write_c45; + ret = tn40_swnodes_register(priv); + if (ret) { + pr_err("swnodes failed\n"); + return ret; + } + + ret = device_add_software_node(&bus->dev, + priv->nodes.group[SWNODE_MDIO]); + if (ret) { + dev_err(&pdev->dev, "device_add_software_node failed: %d\n", + ret); + } + ret = devm_mdiobus_register(&pdev->dev, bus); if (ret) { dev_err(&pdev->dev, "failed to register mdiobus %d %u %u\n", ret, bus->state, MDIOBUS_UNREGISTERED); - return ret; + goto err_swnodes_unregister; } tn40_mdio_set_speed(priv, TN40_MDIO_SPEED_6MHZ); priv->mdio = bus; return 0; + +err_swnodes_unregister: + device_remove_software_node(&bus->dev); + software_node_unregister_node_group(priv->nodes.group); + software_node_unregister(priv->nodes.group[SWNODE_MDIO]); + return ret; } + +MODULE_FIRMWARE(AQR105_FIRMWARE);
Create a software node (or fwnode) for the mdio function and a linked node for the Aquantia AQR105 PHY, providing a firmware-name to allow the PHY to load a MAC/MDIO specific firmware. I suggest to place the firmware in the directory, where there is already the firmware for the TN4010 MAC, so avoid pollution of the firmware directory. There is currently a little problem in the code that leads to an extra increment of the usage counter of the software node priv->nodes.group[SWNODE_MDIO] somewhere. Therefore, I need to unregister it twice. This is hopefully only a temporary workaround. Signed-off-by: Hans-Frieder Vogt <hfdevel@gmx.net> --- drivers/net/ethernet/tehuti/tn40.c | 10 +++- drivers/net/ethernet/tehuti/tn40.h | 25 ++++++++++ drivers/net/ethernet/tehuti/tn40_mdio.c | 63 ++++++++++++++++++++++++- 3 files changed, 96 insertions(+), 2 deletions(-) -- 2.45.2