On 2/20/2024 10:47 PM, Maxime Chevallier wrote:
Hello,
On Tue, 20 Feb 2024 03:57:38 +0800
Yang Xiwen via B4 Relay <devnull+forbidden405.outlook.com@xxxxxxxxxx>
wrote:
From: Yang Xiwen <forbidden405@xxxxxxxxxxx>
Register the sub MDIO bus if it is found. Also implement the internal
PHY reset procedure as needed.
Note it's unable to put the MDIO bus node outside of MAC controller
(i.e. at the same level in the parent bus node). Because we need to
control all clocks and resets in FEMAC driver due to the phy reset
procedure. So the clocks can't be assigned to MDIO bus device, which is
an essential resource for the MDIO bus to work.
No backward compatibility is maintained since the only existing
user(Hi3516DV300) has not received any updates from HiSilicon for about
8 years already. And till today, no mainline dts for this SoC is found.
It seems unlikely that there are still existing mainline Hi3516DV300
users. The effort to maintain the old binding seems gain a little.
Signed-off-by: Yang Xiwen <forbidden405@xxxxxxxxxxx>
Besides what Andrew and Simon already mentionned, I have a few other
small comments :
[...]
@@ -797,23 +816,22 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
goto out_free_netdev;
}
- priv->clk = devm_clk_get(&pdev->dev, NULL);
- if (IS_ERR(priv->clk)) {
- dev_err(dev, "failed to get clk\n");
- ret = -ENODEV;
+ ret = devm_clk_bulk_get_all(&pdev->dev, &priv->clks);
+ if (ret < 0 || ret != CLK_NUM) {
+ dev_err(dev, "failed to get clk bulk: %d\n", ret);
goto out_free_netdev;
}
- ret = clk_prepare_enable(priv->clk);
+ ret = clk_bulk_prepare_enable(CLK_NUM, priv->clks);
if (ret) {
- dev_err(dev, "failed to enable clk %d\n", ret);
+ dev_err(dev, "failed to enable clk bulk: %d\n", ret);
goto out_free_netdev;
}
priv->mac_rst = devm_reset_control_get(dev, "mac");
if (IS_ERR(priv->mac_rst)) {
ret = PTR_ERR(priv->mac_rst);
- goto out_disable_clk;
+ goto out_free_netdev;
When you return here (or even later), you are missing a call to
"clk_bulk_disable_unprepare" in the cleanup path
I didn't notice that. Originally i'm using devm_clk_get_enabled() so
it's not needed.
Maybe we can add a devm_clk_bulk_get_prepared_enabled() API to clk
framework too.
}
hisi_femac_core_reset(priv);
@@ -826,15 +844,34 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
priv->phy_reset_delays,
DELAYS_NUM);
if (ret)
- goto out_disable_clk;
+ goto out_free_netdev;
hisi_femac_phy_reset(priv);
}
+ // Register the optional MDIO bus
I think this comment style should be avoided, in favor of C-style
comments ( /* blabla */ )
Will fix in next release.
+ for_each_available_child_of_node(node, mdio_np) {
+ if (of_node_name_prefix(mdio_np, "mdio")) {
+ priv->mdio_pdev = of_platform_device_create(mdio_np, NULL, dev);
+ of_node_put(mdio_np);
+ if (!priv->mdio_pdev) {
+ dev_err(dev, "failed to register MDIO bus device\n");
+ ret = -ENODEV;
+ goto out_free_netdev;
+ }
+ mdio_registered = true;
+ break;
+ }
+ of_node_put(mdio_np);
+ }
+
+ if (!mdio_registered)
+ dev_warn(dev, "MDIO subnode not found. This is usually a bug.\n");
+
phy = of_phy_get_and_connect(ndev, node, hisi_femac_adjust_link);
if (!phy) {
dev_err(dev, "connect to PHY failed!\n");
ret = -ENODEV;
- goto out_disable_clk;
+ goto out_unregister_mdio_bus;
}
phy_attached_print(phy, "phy_id=0x%.8lx, phy_mode=%s\n",
Thanks,
Maxime
--
Regards,
Yang Xiwen
