Signed-off-by: Yang Xiwen <[email protected]>
---
Changes in v3:
- rearrange patches to fix bot error. (Rob Herring)
- rewrite commit logs (Andrew Lunn)
- use clk_bulk_ APIs (Andrew Lunn)
- fix uninitialization use of ret (assign to -ENODEV before goto) (Simon Horman)
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- replace email.
- hisi-femac: s/BUS/MACIF (Andrew Lunn)
- hisi-femac: add "hisilicon,hisi-femac" compatible since the driver
seems generic enough for various SoCs
- hisi-femac-mdio: convert binding to YAML (Krzysztof Kozlowski)
- rewrite commit logs (Krzysztof Kozlowski)
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Yang Xiwen (6):
net: mdio: hisi-femac: make clock optional
dt-bindings: net: hisilicon-femac-mdio: convert to YAML
net: hisilicon: add support for hisi_femac core on Hi3798MV200
net: hisi_femac: remove unused compatible strings
dt-bindings: net: remove outdated hisilicon-femac
dt-bindings: net: add hisilicon,hisi-femac
.../bindings/net/hisilicon,hisi-femac-mdio.yaml | 48 +++++++++
.../bindings/net/hisilicon,hisi-femac.yaml | 117 +++++++++++++++++++++
.../bindings/net/hisilicon-femac-mdio.txt | 22 ----
.../devicetree/bindings/net/hisilicon-femac.txt | 41 --------
drivers/net/ethernet/hisilicon/hisi_femac.c | 81 ++++++++++----
drivers/net/mdio/mdio-hisi-femac.c | 2 +-
6 files changed, 228 insertions(+), 83 deletions(-)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240216-net-9a208e17c40f
Best regards,
--
Yang Xiwen <[email protected]>
From: Yang Xiwen <[email protected]>
Also mark the clock optional
Signed-off-by: Yang Xiwen <[email protected]>
---
.../bindings/net/hisilicon,hisi-femac-mdio.yaml | 48 ++++++++++++++++++++++
.../bindings/net/hisilicon-femac-mdio.txt | 22 ----------
2 files changed, 48 insertions(+), 22 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/hisilicon,hisi-femac-mdio.yaml b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac-mdio.yaml
new file mode 100644
index 000000000000..1c85e6e7f8f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac-mdio.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hisilicon,hisi-femac-mdio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon FEMAC MDIO bus
+
+maintainers:
+ - Yang Xiwen <[email protected]>
+
+allOf:
+ - $ref: mdio.yaml#
+
+properties:
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ compatible:
+ const: hisilicon,hisi-femac-mdio
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ mdio@10091100 {
+ compatible = "hisilicon,hisi-femac-mdio";
+ reg = <0x10091100 0x20>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy@1 {
+ reg = <1>;
+ };
+ };
diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt b/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
deleted file mode 100644
index 23a39a309d17..000000000000
--- a/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
+++ /dev/null
@@ -1,22 +0,0 @@
-Hisilicon Fast Ethernet MDIO Controller interface
-
-Required properties:
-- compatible: should be "hisilicon,hisi-femac-mdio".
-- reg: address and length of the register set for the device.
-- clocks: A phandle to the reference clock for this device.
-
-- PHY subnode: inherits from phy binding [1]
-[1] Documentation/devicetree/bindings/net/phy.txt
-
-Example:
-mdio: mdio@10091100 {
- compatible = "hisilicon,hisi-femac-mdio";
- reg = <0x10091100 0x10>;
- clocks = <&crg HI3516CV300_MDIO_CLK>;
- #address-cells = <1>;
- #size-cells = <0>;
-
- phy0: phy@1 {
- reg = <1>;
- };
-};
--
2.43.0
From: Yang Xiwen <[email protected]>
The only documented SoC Hi3516DV300 does not receive any updates from 8
years ago. With the recent driver changes, it unlikely works for this
SoC anymore. Remove the binding for this SoC.
Also it's hard to get the version number and it's unknown how the
version can be used. Remove them until it's really needed.
Signed-off-by: Yang Xiwen <[email protected]>
---
drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index fedfc7219fad..a5a7e0f5eb59 100644
--- a/drivers/net/ethernet/hisilicon/hisi_femac.c
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -989,9 +989,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
#endif
static const struct of_device_id hisi_femac_match[] = {
- {.compatible = "hisilicon,hisi-femac-v1",},
- {.compatible = "hisilicon,hisi-femac-v2",},
- {.compatible = "hisilicon,hi3516cv300-femac",},
+ {.compatible = "hisilicon,hisi-femac",},
{.compatible = "hisilicon,hi3798mv200-femac",},
{},
};
--
2.43.0
From: Yang Xiwen <[email protected]>
The user documented(Hi3516) is not found in current kernel anymore. And
it seems this SoC has been EOL for a long time. While at it, Remove this
binding entirely due to recent driver changes.
Signed-off-by: Yang Xiwen <[email protected]>
---
.../devicetree/bindings/net/hisilicon-femac.txt | 41 ----------------------
1 file changed, 41 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.txt b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
deleted file mode 100644
index 5f96976f3cea..000000000000
--- a/Documentation/devicetree/bindings/net/hisilicon-femac.txt
+++ /dev/null
@@ -1,41 +0,0 @@
-Hisilicon Fast Ethernet MAC controller
-
-Required properties:
-- compatible: should contain one of the following version strings:
- * "hisilicon,hisi-femac-v1"
- * "hisilicon,hisi-femac-v2"
- and the soc string "hisilicon,hi3516cv300-femac".
-- reg: specifies base physical address(s) and size of the device registers.
- The first region is the MAC core register base and size.
- The second region is the global MAC control register.
-- interrupts: should contain the MAC interrupt.
-- clocks: A phandle to the MAC main clock.
-- resets: should contain the phandle to the MAC reset signal(required) and
- the PHY reset signal(optional).
-- reset-names: should contain the reset signal name "mac"(required)
- and "phy"(optional).
-- phy-mode: see ethernet.txt [1].
-- phy-handle: see ethernet.txt [1].
-- hisilicon,phy-reset-delays-us: triplet of delays if PHY reset signal given.
- The 1st cell is reset pre-delay in micro seconds.
- The 2nd cell is reset pulse in micro seconds.
- The 3rd cell is reset post-delay in micro seconds.
-
-The MAC address will be determined using the optional properties
-defined in ethernet.txt[1].
-
-[1] Documentation/devicetree/bindings/net/ethernet.txt
-
-Example:
- hisi_femac: ethernet@10090000 {
- compatible = "hisilicon,hi3516cv300-femac","hisilicon,hisi-femac-v2";
- reg = <0x10090000 0x1000>,<0x10091300 0x200>;
- interrupts = <12>;
- clocks = <&crg HI3518EV200_ETH_CLK>;
- resets = <&crg 0xec 0>,<&crg 0xec 3>;
- reset-names = "mac","phy";
- mac-address = [00 00 00 00 00 00];
- phy-mode = "mii";
- phy-handle = <&phy0>;
- hisilicon,phy-reset-delays-us = <10000 20000 20000>;
- };
--
2.43.0
From: Yang Xiwen <[email protected]>
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 <[email protected]>
---
drivers/net/ethernet/hisilicon/hisi_femac.c | 77 +++++++++++++++++++++++------
1 file changed, 61 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index 2406263c9dd3..fedfc7219fad 100644
--- a/drivers/net/ethernet/hisilicon/hisi_femac.c
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -10,8 +10,10 @@
#include <linux/etherdevice.h>
#include <linux/interrupt.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/of_mdio.h>
#include <linux/of_net.h>
+#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/reset.h>
@@ -97,6 +99,13 @@ enum phy_reset_delays {
DELAYS_NUM,
};
+enum clk_type {
+ CLK_MAC,
+ CLK_MACIF,
+ CLK_PHY,
+ CLK_NUM,
+};
+
struct hisi_femac_queue {
struct sk_buff **skb;
dma_addr_t *dma_phys;
@@ -108,7 +117,7 @@ struct hisi_femac_queue {
struct hisi_femac_priv {
void __iomem *port_base;
void __iomem *glb_base;
- struct clk *clk;
+ struct clk_bulk_data *clks;
struct reset_control *mac_rst;
struct reset_control *phy_rst;
u32 phy_reset_delays[DELAYS_NUM];
@@ -116,6 +125,7 @@ struct hisi_femac_priv {
struct device *dev;
struct net_device *ndev;
+ struct platform_device *mdio_pdev;
struct hisi_femac_queue txq;
struct hisi_femac_queue rxq;
@@ -693,6 +703,7 @@ static const struct net_device_ops hisi_femac_netdev_ops = {
static void hisi_femac_core_reset(struct hisi_femac_priv *priv)
{
reset_control_assert(priv->mac_rst);
+ usleep_range(200, 300);
reset_control_deassert(priv->mac_rst);
}
@@ -712,6 +723,10 @@ static void hisi_femac_sleep_us(u32 time_us)
static void hisi_femac_phy_reset(struct hisi_femac_priv *priv)
{
+ /* MAC clock must be disabled before PHY reset
+ */
+ clk_disable(priv->clks[CLK_MAC].clk);
+ clk_disable(priv->clks[CLK_MACIF].clk);
/* To make sure PHY hardware reset success,
* we must keep PHY in deassert state first and
* then complete the hardware reset operation
@@ -727,6 +742,9 @@ static void hisi_femac_phy_reset(struct hisi_femac_priv *priv)
reset_control_deassert(priv->phy_rst);
/* delay some time to ensure later MDIO access */
hisi_femac_sleep_us(priv->phy_reset_delays[POST_DELAY]);
+
+ clk_enable(priv->clks[CLK_MAC].clk);
+ clk_enable(priv->clks[CLK_MACIF].clk);
}
static void hisi_femac_port_init(struct hisi_femac_priv *priv)
@@ -768,11 +786,12 @@ static void hisi_femac_port_init(struct hisi_femac_priv *priv)
static int hisi_femac_drv_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct device_node *node = dev->of_node;
+ struct device_node *node = dev->of_node, *mdio_np;
struct net_device *ndev;
struct hisi_femac_priv *priv;
struct phy_device *phy;
int ret;
+ bool mdio_registered = false;
ndev = alloc_etherdev(sizeof(*priv));
if (!ndev)
@@ -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;
}
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
+ 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",
@@ -885,8 +922,8 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
out_disconnect_phy:
netif_napi_del(&priv->napi);
phy_disconnect(phy);
-out_disable_clk:
- clk_disable_unprepare(priv->clk);
+out_unregister_mdio_bus:
+ platform_device_unregister(priv->mdio_pdev);
out_free_netdev:
free_netdev(ndev);
@@ -902,7 +939,8 @@ static void hisi_femac_drv_remove(struct platform_device *pdev)
unregister_netdev(ndev);
phy_disconnect(ndev->phydev);
- clk_disable_unprepare(priv->clk);
+ platform_device_unregister(priv->mdio_pdev);
+ clk_bulk_disable_unprepare(CLK_NUM, priv->clks);
free_netdev(ndev);
}
@@ -919,7 +957,7 @@ static int hisi_femac_drv_suspend(struct platform_device *pdev,
netif_device_detach(ndev);
}
- clk_disable_unprepare(priv->clk);
+ clk_bulk_disable_unprepare(CLK_NUM, priv->clks);
return 0;
}
@@ -928,8 +966,14 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
{
struct net_device *ndev = platform_get_drvdata(pdev);
struct hisi_femac_priv *priv = netdev_priv(ndev);
+ int ret;
+
+ ret = clk_bulk_prepare_enable(CLK_NUM, priv->clks);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to enable clk bulk: %d\n", ret);
+ return ret;
+ }
- clk_prepare_enable(priv->clk);
if (priv->phy_rst)
hisi_femac_phy_reset(priv);
@@ -948,6 +992,7 @@ static const struct of_device_id hisi_femac_match[] = {
{.compatible = "hisilicon,hisi-femac-v1",},
{.compatible = "hisilicon,hisi-femac-v2",},
{.compatible = "hisilicon,hi3516cv300-femac",},
+ {.compatible = "hisilicon,hi3798mv200-femac",},
{},
};
--
2.43.0
From: Yang Xiwen <[email protected]>
The clocks are optional.
Signed-off-by: Yang Xiwen <[email protected]>
---
drivers/net/mdio/mdio-hisi-femac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/mdio/mdio-hisi-femac.c b/drivers/net/mdio/mdio-hisi-femac.c
index 6703f626ee83..a2620b200381 100644
--- a/drivers/net/mdio/mdio-hisi-femac.c
+++ b/drivers/net/mdio/mdio-hisi-femac.c
@@ -93,7 +93,7 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
goto err_out_free_mdiobus;
}
- data->clk = devm_clk_get(&pdev->dev, NULL);
+ data->clk = devm_clk_get_optional(&pdev->dev, NULL);
if (IS_ERR(data->clk)) {
ret = PTR_ERR(data->clk);
goto err_out_free_mdiobus;
--
2.43.0
From: Yang Xiwen <[email protected]>
Compared to previous txt based binding doc, the following changes are
made according to the TRM:
- Remove unmaintained Hi3516 SoC, add Hi3798MV200
- add MDIO subnode, because MDIO bus is integrated
- add ahb bus clock, phy clock and reset
Also remove "hisi-femac-v1/2" binding.
The difference between versions is unknown and not documented anywhere.
Nor is it used in driver. Remove it until it's needed in the future.
Signed-off-by: Yang Xiwen <[email protected]>
---
.../bindings/net/hisilicon,hisi-femac.yaml | 117 +++++++++++++++++++++
1 file changed, 117 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
new file mode 100644
index 000000000000..08158118c9c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hisilicon,hisi-femac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon Fast Ethernet MAC controller
+
+maintainers:
+ - Yang Xiwen <[email protected]>
+
+allOf:
+ - $ref: ethernet-controller.yaml
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - hisilicon,hi3798mv200-femac
+ - const: hisilicon,hisi-femac
+
+ reg:
+ items:
+ - description: The first region is the MAC core register base and size.
+ - description: The second region is the global MAC control register.
+
+ ranges:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 3
+
+ clock-names:
+ items:
+ - const: mac
+ - const: macif
+ - const: phy
+
+ resets:
+ maxItems: 2
+
+ reset-names:
+ items:
+ - const: mac
+ - const: phy
+
+ hisilicon,phy-reset-delays-us:
+ items:
+ - description: The 1st cell is reset pre-delay in micro seconds.
+ - description: The 2nd cell is reset pulse in micro seconds.
+ - description: The 3rd cell is reset post-delay in micro seconds.
+
+patternProperties:
+ '^mdio@[0-9a-f]+$':
+ $ref: hisilicon,hisi-femac-mdio.yaml#
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - resets
+ - reset-names
+ - phy-connection-type
+ - phy-handle
+ - hisilicon,phy-reset-delays-us
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/histb-clock.h>
+
+ ethernet@9c30000 {
+ compatible = "hisilicon,hi3798mv200-femac", "hisilicon,hisi-femac";
+ reg = <0x9c30000 0x1000>, <0x9c31300 0x200>;
+ ranges = <0x0 0x9c30000 0x10000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&crg HISTB_ETH0_MAC_CLK>,
+ <&crg HISTB_ETH0_MACIF_CLK>,
+ <&crg 62>;
+ clock-names = "mac", "macif", "phy";
+ resets = <&crg 0xd0 3>, <&crg 0x388 4>;
+ reset-names = "mac", "phy";
+ phy-handle = <&fephy>;
+ phy-connection-type = "mii";
+ // To be filled by bootloader
+ mac-address = [00 00 00 00 00 00];
+ hisilicon,phy-reset-delays-us = <10000 10000 500000>;
+ status = "okay";
+
+ mdio@1100 {
+ compatible = "hisilicon,hisi-femac-mdio";
+ reg = <0x1100 0x20>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "okay";
+
+ ethernet-phy@1 {
+ reg = <1>;
+ #phy-cells = <0>;
+ };
+ };
+ };
--
2.43.0
> 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.
What PHY driver is being used? If there a specific PHY driver for this
hardware? Does it implement soft reset?
I'm wondering if you can skip hardware reset of the PHY and only do a
software reset.
Andrew
On 2/20/2024 4:03 AM, Andrew Lunn wrote:
>> 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.
> What PHY driver is being used? If there a specific PHY driver for this
> hardware? Does it implement soft reset?
I'm using generic PHY driver.
It implements IEEE C22 standard. So there is a soft reset in BMCR register.
>
> I'm wondering if you can skip hardware reset of the PHY and only do a
> software reset.
There must be someone to deassert the hardware reset control signal for
the PHY. We can't rely on the boot loader to do that. And here even we
choose to skip the hardware reset procedure, the sequence of deasserting
the reset signals is also very important. (i.e. first PHY, then MAC and
MACIF). Opposite to the normal sequence. (we normally first register MAC
driver, and then PHY).
And it might be possible that boot loaders screw all the things up and
we are forced to do the hardware reset procedure in kernel.
>
> Andrew
--
Regards,
Yang Xiwen
On 2/20/2024 4:03 AM, Andrew Lunn wrote:
>> 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.
> What PHY driver is being used? If there a specific PHY driver for this
> hardware? Does it implement soft reset?
>
> I'm wondering if you can skip hardware reset of the PHY and only do a
> software reset.
Can we ask ethernet PHY framework to notify us (the MAC driver) when it
is going to do the hardware reset? In that way we can add clock
disabling/enabling code to the callback and let the PHY framework do the
reset procedure. In this way, we can benefit a lot from PHY framework.
E.g. make use of dt props like `reset-(de)assert-us`, rather than
encoding these value in the MAC driver with a custom vendor property.
>
> Andrew
--
Regards,
Yang Xiwen
On Tue, Feb 20, 2024 at 04:14:36AM +0800, Yang Xiwen wrote:
> On 2/20/2024 4:03 AM, Andrew Lunn wrote:
> > > 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.
> > What PHY driver is being used? If there a specific PHY driver for this
> > hardware? Does it implement soft reset?
>
> I'm using generic PHY driver.
>
> It implements IEEE C22 standard. So there is a soft reset in BMCR register.
>
> >
> > I'm wondering if you can skip hardware reset of the PHY and only do a
> > software reset.
>
> There must be someone to deassert the hardware reset control signal for the
> PHY. We can't rely on the boot loader to do that. And here even we choose to
> skip the hardware reset procedure, the sequence of deasserting the reset
> signals is also very important. (i.e. first PHY, then MAC and MACIF).
> Opposite to the normal sequence. (we normally first register MAC driver, and
> then PHY).
There are a few MACs which require the PHY to provide a clock to the
MAC before they can use their DMA engine. The PHY provides typically a
25MHz clock, which is used to driver the DMA. So long as you don't
touch the DMA, you can access other parts of the MAC before the PHY is
generating the clock.
So it might be possible to take the MAC and MACIF out of reset, then
create the MDIO bus, probe the PHY, take it out of reset so its
generating the clock, and then complete the rest of the MAC setup.
Andrew
On 2/20/2024 5:05 AM, Andrew Lunn wrote:
> On Tue, Feb 20, 2024 at 04:14:36AM +0800, Yang Xiwen wrote:
>> On 2/20/2024 4:03 AM, Andrew Lunn wrote:
>>>> 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.
>>> What PHY driver is being used? If there a specific PHY driver for this
>>> hardware? Does it implement soft reset?
>> I'm using generic PHY driver.
>>
>> It implements IEEE C22 standard. So there is a soft reset in BMCR register.
>>
>>> I'm wondering if you can skip hardware reset of the PHY and only do a
>>> software reset.
>> There must be someone to deassert the hardware reset control signal for the
>> PHY. We can't rely on the boot loader to do that. And here even we choose to
>> skip the hardware reset procedure, the sequence of deasserting the reset
>> signals is also very important. (i.e. first PHY, then MAC and MACIF).
>> Opposite to the normal sequence. (we normally first register MAC driver, and
>> then PHY).
> There are a few MACs which require the PHY to provide a clock to the
> MAC before they can use their DMA engine. The PHY provides typically a
> 25MHz clock, which is used to driver the DMA. So long as you don't
> touch the DMA, you can access other parts of the MAC before the PHY is
> generating the clock.
>
> So it might be possible to take the MAC and MACIF out of reset, then
> create the MDIO bus, probe the PHY, take it out of reset so its
> generating the clock, and then complete the rest of the MAC setup.
It's not MAC which behaves wrongly, it's the MDIO bus. If we don't
follow the reset procedure properly. The MDIO bus fails to respond to
any write/read commands. But i believe MAC controller and PHY are still
working. I recalled that it can still transfer network packets, though
it fails to read PHY registers from MDIO bus so only 10Mbps is available
(And the phy id read out is always 0x0, normally it's 0x20669853).
Maybe during initialization, PHY sent some garbage to MDIO bus and
killed it.
>
> Andrew
>
--
Regards,
Yang Xiwen
> It's not MAC which behaves wrongly, it's the MDIO bus. If we don't follow
> the reset procedure properly. The MDIO bus fails to respond to any
> write/read commands. But i believe MAC controller and PHY are still working.
> I recalled that it can still transfer network packets, though it fails to
> read PHY registers from MDIO bus so only 10Mbps is available (And the phy id
> read out is always 0x0, normally it's 0x20669853).
>
> Maybe during initialization, PHY sent some garbage to MDIO bus and killed
> it.
MDIO bus masters are really simple things, not much more than a shift
register. I find it hard to believe the MDIO bus master breaks because
of reset order. If the MDIO pins went to SoC pins, it would be simple
to prove, a bus-pirate or similar can capture the signals and sigrok
can decode MDIO.
To me, its more likely the PHY side of the MDIO bus is broken somehow.
Andrew
On 2/20/2024 6:05 AM, Andrew Lunn wrote:
>> It's not MAC which behaves wrongly, it's the MDIO bus. If we don't follow
>> the reset procedure properly. The MDIO bus fails to respond to any
>> write/read commands. But i believe MAC controller and PHY are still working.
>> I recalled that it can still transfer network packets, though it fails to
>> read PHY registers from MDIO bus so only 10Mbps is available (And the phy id
>> read out is always 0x0, normally it's 0x20669853).
>>
>> Maybe during initialization, PHY sent some garbage to MDIO bus and killed
>> it.
> MDIO bus masters are really simple things, not much more than a shift
> register. I find it hard to believe the MDIO bus master breaks because
> of reset order. If the MDIO pins went to SoC pins, it would be simple
> to prove, a bus-pirate or similar can capture the signals and sigrok
> can decode MDIO.
>
> To me, its more likely the PHY side of the MDIO bus is broken somehow.
The MDIO pins are not accessible from outside, only PHY pins are exported.
Maybe. I've tried many different approaches before i sent this patch.
This is almost the only simple way i can implement to make it work. The
downstream is also not telling us why it is needed to disable MAC
controller before PHY reset. But it is mandatory. All i can do is to
guess, unless HiSilicon people can join this conversion and tell us why.
So the conclusion i got from trial and error is that MAC controller MUST
be disabled during PHY reset, no matter what the reason is. I hope we
can have the framework providing utilities for me to resolve this (e.g.
provide a custom callback for PHY reset, in mdio_device_reset() function).
Also it's very strange that, if the PHY is reset in a wrong order, (i.e.
Now we can not operate MDIO bus), we have to do the PHY reset procedure
once again, simply resetting MAC does not work. So it might also be the
PHY which is broken (partially).
>
> Andrew
--
Regards,
Yang Xiwen
On 2/20/2024 6:05 AM, Andrew Lunn wrote:
>> It's not MAC which behaves wrongly, it's the MDIO bus. If we don't follow
>> the reset procedure properly. The MDIO bus fails to respond to any
>> write/read commands. But i believe MAC controller and PHY are still working.
>> I recalled that it can still transfer network packets, though it fails to
>> read PHY registers from MDIO bus so only 10Mbps is available (And the phy id
>> read out is always 0x0, normally it's 0x20669853).
>>
>> Maybe during initialization, PHY sent some garbage to MDIO bus and killed
>> it.
> MDIO bus masters are really simple things, not much more than a shift
> register. I find it hard to believe the MDIO bus master breaks because
I conclude that master side is working. Because when i tried to read PHY
registers manually. It does say it completes reading(the finish bit is
set). Though the data read out is invalid.
> of reset order. If the MDIO pins went to SoC pins, it would be simple
> to prove, a bus-pirate or similar can capture the signals and sigrok
> can decode MDIO.
>
> To me, its more likely the PHY side of the MDIO bus is broken somehow.
>
> Andrew
--
Regards,
Yang Xiwen
On Tue, Feb 20, 2024 at 03:57:38AM +0800, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/net/ethernet/hisilicon/hisi_femac.c | 77 +++++++++++++++++++++++------
> 1 file changed, 61 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
..
> @@ -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
> + 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);
Sorry for not noticing this earlier.
I think that of_node_put() only needs to be called in the
case of terminating the loop (via break, goto, return, etc...).
But should not be called otherwise (when the loop cycles) as
for_each_available_child_of_node() calls of_node_put().
Flagged by Coccinelle.
> + }
> +
> + 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",
..
Hello,
On Tue, 20 Feb 2024 03:57:38 +0800
Yang Xiwen via B4 Relay <[email protected]>
wrote:
> From: Yang Xiwen <[email protected]>
>
> 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 <[email protected]>
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
> }
> 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 */ )
> + 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
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 <[email protected]>
> wrote:
>
>> From: Yang Xiwen <[email protected]>
>>
>> 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 <[email protected]>
> 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
On 19/02/2024 20:57, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
>
> Also mark the clock optional
>
> +allOf:
> + - $ref: mdio.yaml#
> +
> +properties:
> + "#address-cells":
> + const: 1
Drop property.
> +
> + "#size-cells":
> + const: 0
Drop property.
> +
> + compatible:
> + const: hisilicon,hisi-femac-mdio
Put compatible as the first.
> +
> + reg:
> + maxItems: 1
Reg the second.
> +
> + clocks:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + mdio@10091100 {
> + compatible = "hisilicon,hisi-femac-mdio";
> + reg = <0x10091100 0x20>;
> + #address-cells = <1>;
> + #size-cells = <0>;
Add clocks to the example.
Best regards,
Krzysztof
On 19/02/2024 20:57, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
>
> Also mark the clock optional
Why? Your commit msg must provide rationale. Clocks are rarely optional
signals, so this looks wrong.
Best regards,
Krzysztof
On 2/21/2024 4:12 PM, Krzysztof Kozlowski wrote:
> On 19/02/2024 20:57, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <[email protected]>
>>
>> Also mark the clock optional
> Why? Your commit msg must provide rationale. Clocks are rarely optional
> signals, so this looks wrong.
Well. I don't know too much about the hardware either. I think the clock
can be either shared with MAC controller or completely non-existent in
hardware.
But from the driver side, the clock must be stripped from MDIO bus node.
The clocks must be controlled entirely within MAC controller driver. Do
I need to mention it in this commit too?
Actually I've never seen anything like "MDIO bus clock" in the
datasheet. The MDIO bus and MAC controller are activated simultaneously.
So I think removing this clock entirely is also acceptable.
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 21/02/2024 12:24, Yang Xiwen wrote:
> On 2/21/2024 4:12 PM, Krzysztof Kozlowski wrote:
>> On 19/02/2024 20:57, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <[email protected]>
>>>
>>> Also mark the clock optional
>> Why? Your commit msg must provide rationale. Clocks are rarely optional
>> signals, so this looks wrong.
>
>
> Well. I don't know too much about the hardware either. I think the clock
> can be either shared with MAC controller or completely non-existent in
> hardware.
>
>
> But from the driver side, the clock must be stripped from MDIO bus node.
> The clocks must be controlled entirely within MAC controller driver. Do
> I need to mention it in this commit too?
>
>
> Actually I've never seen anything like "MDIO bus clock" in the
> datasheet. The MDIO bus and MAC controller are activated simultaneously.
> So I think removing this clock entirely is also acceptable.
None of these are arguments to make the clock optional. The last one is
an argument to drop clock entirely.
Best regards,
Krzysztof