[PATCH 0/6] net: hisi-femac: add support for Hi3798MV200, remove unmaintained compatibles

[PATCH 0/6] net: hisi-femac: add support for Hi3798MV200, remove unmaintained compatibles

[Yang Xiwen via B4 Relay]

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Yang Xiwen (6):
      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-femac
      net: mdio: hisi-femac: make clock optional
      dt-bindings: net: hisilicon-femac-mdio: make clock optional

 .../bindings/net/hisilicon-femac-mdio.txt          |   2 +
 .../devicetree/bindings/net/hisilicon-femac.txt    |  41 -------
 .../devicetree/bindings/net/hisilicon-femac.yaml   | 125 +++++++++++++++++++++
 drivers/net/ethernet/hisilicon/hisi_femac.c        |  93 ++++++++++-----
 drivers/net/mdio/mdio-hisi-femac.c                 |   2 +-
 5 files changed, 196 insertions(+), 67 deletions(-)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240216-net-9a208e17c40f

Best regards,
-- 
Yang Xiwen <forbidden405@outlook.com>

[PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Considering that no users is found in the kernel, no backward
compatibility is maintained.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/net/ethernet/hisilicon/hisi_femac.c | 90 ++++++++++++++++++++++-------
 1 file changed, 68 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index 2406263c9dd3..d72160efff9a 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_BUS,
+	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 *clks[CLK_NUM];
 	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_disable(priv->clks[CLK_BUS]);
 	/* 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_enable(priv->clks[CLK_BUS]);
 }
 
 static void hisi_femac_port_init(struct hisi_femac_priv *priv)
@@ -768,11 +786,17 @@ 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;
+	int ret, i;
+	bool mdio_registered = false;
+	static const char * const clk_strs[] = {
+		[CLK_MAC] = "mac",
+		[CLK_BUS] = "bus",
+		[CLK_PHY] = "phy",
+	};
 
 	ndev = alloc_etherdev(sizeof(*priv));
 	if (!ndev)
@@ -797,23 +821,20 @@ 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;
-		goto out_free_netdev;
-	}
-
-	ret = clk_prepare_enable(priv->clk);
-	if (ret) {
-		dev_err(dev, "failed to enable clk %d\n", ret);
-		goto out_free_netdev;
+	for (i = 0; i < CLK_NUM; i++) {
+		priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]);
+		if (IS_ERR(priv->clks[i])) {
+			dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i],
+				PTR_ERR(priv->clks[i]));
+			ret = -ENODEV;
+			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 +847,32 @@ 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");
+				goto out_free_netdev;
+			}
+			mdio_registered = true;
+			break;
+		}
+	}
+
+	if (!mdio_registered)
+		dev_warn(dev, "MDIO subnode notfound. 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 +923,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);
 
@@ -897,12 +935,15 @@ static void hisi_femac_drv_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct hisi_femac_priv *priv = netdev_priv(ndev);
+	int i;
 
 	netif_napi_del(&priv->napi);
 	unregister_netdev(ndev);
 
 	phy_disconnect(ndev->phydev);
-	clk_disable_unprepare(priv->clk);
+	platform_device_unregister(priv->mdio_pdev);
+	for (i = 0; i < CLK_NUM; i++)
+		clk_disable_unprepare(priv->clks[i]);
 	free_netdev(ndev);
 }
 
@@ -912,6 +953,7 @@ static int hisi_femac_drv_suspend(struct platform_device *pdev,
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct hisi_femac_priv *priv = netdev_priv(ndev);
+	int i;
 
 	disable_irq(ndev->irq);
 	if (netif_running(ndev)) {
@@ -919,7 +961,8 @@ static int hisi_femac_drv_suspend(struct platform_device *pdev,
 		netif_device_detach(ndev);
 	}
 
-	clk_disable_unprepare(priv->clk);
+	for (i = 0; i < CLK_NUM; i++)
+		clk_disable_unprepare(priv->clks[i]);
 
 	return 0;
 }
@@ -928,8 +971,10 @@ 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 i;
 
-	clk_prepare_enable(priv->clk);
+	for (i = 0; i < CLK_NUM; i++)
+		clk_prepare_enable(priv->clks[i]);
 	if (priv->phy_rst)
 		hisi_femac_phy_reset(priv);
 
@@ -948,6 +993,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

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Andrew Lunn]

> +	for (i = 0; i < CLK_NUM; i++) {
> +		priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]);
> +		if (IS_ERR(priv->clks[i])) {
> +			dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i],
> +				PTR_ERR(priv->clks[i]));
> +			ret = -ENODEV;
> +			goto out_free_netdev;
> +		}

The clk API has devm_clk_bulk_ versions. Please take a look at them, and see
if it will simplify the code.

	Andrew

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Yang Xiwen]

On 2/16/2024 7:57 AM, Andrew Lunn wrote:
>> +	for (i = 0; i < CLK_NUM; i++) {
>> +		priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]);
>> +		if (IS_ERR(priv->clks[i])) {
>> +			dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i],
>> +				PTR_ERR(priv->clks[i]));
>> +			ret = -ENODEV;
>> +			goto out_free_netdev;
>> +		}
> The clk API has devm_clk_bulk_ versions. Please take a look at them, and see
> if it will simplify the code.
I know this API, but it can't be used. We need to control clocks 
individually in reset procedure.
> 	Andrew


-- 
Regards,
Yang Xiwen

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Andrew Lunn]

On Fri, Feb 16, 2024 at 07:59:19AM +0800, Yang Xiwen wrote:
> On 2/16/2024 7:57 AM, Andrew Lunn wrote:
> > > +	for (i = 0; i < CLK_NUM; i++) {
> > > +		priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]);
> > > +		if (IS_ERR(priv->clks[i])) {
> > > +			dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i],
> > > +				PTR_ERR(priv->clks[i]));
> > > +			ret = -ENODEV;
> > > +			goto out_free_netdev;
> > > +		}
> > The clk API has devm_clk_bulk_ versions. Please take a look at them, and see
> > if it will simplify the code.
> I know this API, but it can't be used. We need to control clocks
> individually in reset procedure.

/**
 * struct clk_bulk_data - Data used for bulk clk operations.
 *
 * @id: clock consumer ID
 * @clk: struct clk * to store the associated clock
 *
 * The CLK APIs provide a series of clk_bulk_() API calls as
 * a convenience to consumers which require multiple clks.  This
 * structure is used to manage data for these calls.
 */
struct clk_bulk_data {
	const char		*id;
	struct clk		*clk;
};

You pass the bulk API calls an array of this structure. After the get
has completed, you can access the individual clocks via the clk
pointer. So you can use the bulk API on probe and remove when you need
to operate on all three, and the single clk API for your reset handler
etc.

       Andrew

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Yang Xiwen]

On 2/16/2024 9:49 PM, Andrew Lunn wrote:
> On Fri, Feb 16, 2024 at 07:59:19AM +0800, Yang Xiwen wrote:
>> On 2/16/2024 7:57 AM, Andrew Lunn wrote:
>>>> +	for (i = 0; i < CLK_NUM; i++) {
>>>> +		priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]);
>>>> +		if (IS_ERR(priv->clks[i])) {
>>>> +			dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i],
>>>> +				PTR_ERR(priv->clks[i]));
>>>> +			ret = -ENODEV;
>>>> +			goto out_free_netdev;
>>>> +		}
>>> The clk API has devm_clk_bulk_ versions. Please take a look at them, and see
>>> if it will simplify the code.
>> I know this API, but it can't be used. We need to control clocks
>> individually in reset procedure.
> /**
>   * struct clk_bulk_data - Data used for bulk clk operations.
>   *
>   * @id: clock consumer ID
>   * @clk: struct clk * to store the associated clock
>   *
>   * The CLK APIs provide a series of clk_bulk_() API calls as
>   * a convenience to consumers which require multiple clks.  This
>   * structure is used to manage data for these calls.
>   */
> struct clk_bulk_data {
> 	const char		*id;
> 	struct clk		*clk;
> };
>
> You pass the bulk API calls an array of this structure. After the get
> has completed, you can access the individual clocks via the clk
> pointer. So you can use the bulk API on probe and remove when you need
> to operate on all three, and the single clk API for your reset handler
> etc.
seems okay. I'll implement this in next version.
>
>         Andrew


-- 
Regards,
Yang Xiwen

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Andrew Lunn]

> +	// 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");
> +				goto out_free_netdev;
> +			}
> +			mdio_registered = true;
> +			break;
> +		}
> +	}
> +
> +	if (!mdio_registered)
> +		dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n");

I don't understand the architecture of this device yet...

It seems like you have an integrated PHY? In the example, you used a
phy-handle to bind the MAC to the PHY. So why is the MDIO bus
optional?

Do the MII signals from the MAC also go to SoC pins, so you could use
an external PHY? Is there a SERDES so you could connect to an SFP
cage?

Also, do the MDIO pins go to SoC pins? Can the MDIO bus master be used
to control external PHYs?

If everything is internal, fixed in silicon, no variation possible,
you don't need to describe the MDIO bus in DT. The MAC driver can
register it, and then get the PHY at the hard coded address it uses.

	 Andrew

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Yang Xiwen]

On 2/16/2024 9:23 PM, Andrew Lunn wrote:
>> +	// 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");
>> +				goto out_free_netdev;
>> +			}
>> +			mdio_registered = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!mdio_registered)
>> +		dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n");
> I don't understand the architecture of this device yet...
>
> It seems like you have an integrated PHY? In the example, you used a
> phy-handle to bind the MAC to the PHY. So why is the MDIO bus
> optional?
Because the MAC can also support external PHY according to the 
datasheet. Maybe some other SoCs didn't implement this internal PHY and 
used an external PHY instead.
>
> Do the MII signals from the MAC also go to SoC pins, so you could use
> an external PHY? Is there a SERDES so you could connect to an SFP
> cage?
No. MII signals is not accessible outside of the SoC. The SoC only 
exports FEPHY pins (i.e. RXN(P) and TXN(P)).
>
> Also, do the MDIO pins go to SoC pins? Can the MDIO bus master be used
> to control external PHYs?
It can, but not for Hi3798MV200. The datasheet said it can use both 
internal phy or external phy. But for Hi3798MV200, seems impossible.
>
> If everything is internal, fixed in silicon, no variation possible,
> you don't need to describe the MDIO bus in DT. The MAC driver can
> register it, and then get the PHY at the hard coded address it uses.
>
> 	 Andrew


-- 
Regards,
Yang Xiwen

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Andrew Lunn]

On Fri, Feb 16, 2024 at 09:41:29PM +0800, Yang Xiwen wrote:
> On 2/16/2024 9:23 PM, Andrew Lunn wrote:
> > > +	// 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");
> > > +				goto out_free_netdev;
> > > +			}
> > > +			mdio_registered = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (!mdio_registered)
> > > +		dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n");
> > I don't understand the architecture of this device yet...
> > 
> > It seems like you have an integrated PHY? In the example, you used a
> > phy-handle to bind the MAC to the PHY. So why is the MDIO bus
> > optional?
> Because the MAC can also support external PHY according to the datasheet.
> Maybe some other SoCs didn't implement this internal PHY and used an
> external PHY instead.
> > 
> > Do the MII signals from the MAC also go to SoC pins, so you could use
> > an external PHY? Is there a SERDES so you could connect to an SFP
> > cage?
> No. MII signals is not accessible outside of the SoC. The SoC only exports
> FEPHY pins (i.e. RXN(P) and TXN(P)).
> > 
> > Also, do the MDIO pins go to SoC pins? Can the MDIO bus master be used
> > to control external PHYs?
> It can, but not for Hi3798MV200. The datasheet said it can use both internal
> phy or external phy. But for Hi3798MV200, seems impossible.

So for the Hi3798MV200 this is not optional, the MDIO bus is
mandatory.

Also, it sounds like it exists in the silicon. So it is better to
always describe it in the .dtsi file.

And i took a quick look at mdio-hisi-femac.c. It has a probe function
which does:

        data->membase = devm_platform_ioremap_resource(pdev, 0);

meaning it expects to have its own address range. It is a device of
its own. That also explains the compatible. So please move the MDIO
bus to a node of its own, rather than embedding it within the MAC
node.

	Andrew

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Yang Xiwen]

On 2/16/2024 9:58 PM, Andrew Lunn wrote:
> On Fri, Feb 16, 2024 at 09:41:29PM +0800, Yang Xiwen wrote:
>> On 2/16/2024 9:23 PM, Andrew Lunn wrote:
>>>> +	// 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");
>>>> +				goto out_free_netdev;
>>>> +			}
>>>> +			mdio_registered = true;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (!mdio_registered)
>>>> +		dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n");
>>> I don't understand the architecture of this device yet...
>>>
>>> It seems like you have an integrated PHY? In the example, you used a
>>> phy-handle to bind the MAC to the PHY. So why is the MDIO bus
>>> optional?
>> Because the MAC can also support external PHY according to the datasheet.
>> Maybe some other SoCs didn't implement this internal PHY and used an
>> external PHY instead.
>>> Do the MII signals from the MAC also go to SoC pins, so you could use
>>> an external PHY? Is there a SERDES so you could connect to an SFP
>>> cage?
>> No. MII signals is not accessible outside of the SoC. The SoC only exports
>> FEPHY pins (i.e. RXN(P) and TXN(P)).
>>> Also, do the MDIO pins go to SoC pins? Can the MDIO bus master be used
>>> to control external PHYs?
>> It can, but not for Hi3798MV200. The datasheet said it can use both internal
>> phy or external phy. But for Hi3798MV200, seems impossible.
> So for the Hi3798MV200 this is not optional, the MDIO bus is
> mandatory.
>
> Also, it sounds like it exists in the silicon. So it is better to
> always describe it in the .dtsi file.
>
> And i took a quick look at mdio-hisi-femac.c. It has a probe function
> which does:
>
>          data->membase = devm_platform_ioremap_resource(pdev, 0);
>
> meaning it expects to have its own address range. It is a device of
> its own. That also explains the compatible. So please move the MDIO
> bus to a node of its own, rather than embedding it within the MAC
> node.

It won't work. Hi3798MV200 does not have a dedicated MDIO bus clock. 
this clock is merged to MAC clock for this SoC. We need to enable 
CLK_BUS and CLK_MAC before MDIO bus access.

Assigning CLK_MAC and CLK_BUS to MDIO bus device does not work either. 
Because the clocks will be enabled twice, the MAC controller will be 
unable to disable the clocks during PHY reset.

Note that the source is contributed by HiSilicon almost 8 yrs ago. And 
they had never proved that this driver  works. They are not doing things 
like this in downstream.

>
> 	Andrew


-- 
Regards,
Yang Xiwen

Re: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Dan Carpenter]

Hi Yang,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Yang-Xiwen-via-B4-Relay/net-hisilicon-add-support-for-hisi_femac-core-on-Hi3798MV200/20240216-075015
base:   8d3dea210042f54b952b481838c1e7dfc4ec751d
patch link:    https://lore.kernel.org/r/20240216-net-v1-1-e0ad972cda99%40outlook.com
patch subject: [PATCH 1/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200
config: arm-randconfig-r081-20240223 (https://download.01.org/0day-ci/archive/20240224/202402242354.leEII4M9-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project edd4aee4dd9b5b98b2576a6f783e4086173d902a)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202402242354.leEII4M9-lkp@intel.com/

smatch warnings:
drivers/net/ethernet/hisilicon/hisi_femac.c:931 hisi_femac_drv_probe() error: uninitialized symbol 'ret'.

vim +/ret +931 drivers/net/ethernet/hisilicon/hisi_femac.c

542ae60af24f02 Dongpo Li      2016-07-15  786  static int hisi_femac_drv_probe(struct platform_device *pdev)
542ae60af24f02 Dongpo Li      2016-07-15  787  {
542ae60af24f02 Dongpo Li      2016-07-15  788  	struct device *dev = &pdev->dev;
3c2c758a1607ea Yang Xiwen     2024-02-16  789  	struct device_node *node = dev->of_node, *mdio_np;
542ae60af24f02 Dongpo Li      2016-07-15  790  	struct net_device *ndev;
542ae60af24f02 Dongpo Li      2016-07-15  791  	struct hisi_femac_priv *priv;
542ae60af24f02 Dongpo Li      2016-07-15  792  	struct phy_device *phy;
3c2c758a1607ea Yang Xiwen     2024-02-16  793  	int ret, i;
3c2c758a1607ea Yang Xiwen     2024-02-16  794  	bool mdio_registered = false;
3c2c758a1607ea Yang Xiwen     2024-02-16  795  	static const char * const clk_strs[] = {
3c2c758a1607ea Yang Xiwen     2024-02-16  796  		[CLK_MAC] = "mac",
3c2c758a1607ea Yang Xiwen     2024-02-16  797  		[CLK_BUS] = "bus",
3c2c758a1607ea Yang Xiwen     2024-02-16  798  		[CLK_PHY] = "phy",
3c2c758a1607ea Yang Xiwen     2024-02-16  799  	};
542ae60af24f02 Dongpo Li      2016-07-15  800  
542ae60af24f02 Dongpo Li      2016-07-15  801  	ndev = alloc_etherdev(sizeof(*priv));
542ae60af24f02 Dongpo Li      2016-07-15  802  	if (!ndev)
542ae60af24f02 Dongpo Li      2016-07-15  803  		return -ENOMEM;
542ae60af24f02 Dongpo Li      2016-07-15  804  
542ae60af24f02 Dongpo Li      2016-07-15  805  	platform_set_drvdata(pdev, ndev);
2087d421a5a1af Dongpo Li      2016-12-12  806  	SET_NETDEV_DEV(ndev, &pdev->dev);
542ae60af24f02 Dongpo Li      2016-07-15  807  
542ae60af24f02 Dongpo Li      2016-07-15  808  	priv = netdev_priv(ndev);
542ae60af24f02 Dongpo Li      2016-07-15  809  	priv->dev = dev;
542ae60af24f02 Dongpo Li      2016-07-15  810  	priv->ndev = ndev;
542ae60af24f02 Dongpo Li      2016-07-15  811  
56170ba3bd9098 Jiangfeng Xiao 2019-07-12  812  	priv->port_base = devm_platform_ioremap_resource(pdev, 0);
542ae60af24f02 Dongpo Li      2016-07-15  813  	if (IS_ERR(priv->port_base)) {
542ae60af24f02 Dongpo Li      2016-07-15  814  		ret = PTR_ERR(priv->port_base);
542ae60af24f02 Dongpo Li      2016-07-15  815  		goto out_free_netdev;
542ae60af24f02 Dongpo Li      2016-07-15  816  	}
542ae60af24f02 Dongpo Li      2016-07-15  817  
56170ba3bd9098 Jiangfeng Xiao 2019-07-12  818  	priv->glb_base = devm_platform_ioremap_resource(pdev, 1);
542ae60af24f02 Dongpo Li      2016-07-15  819  	if (IS_ERR(priv->glb_base)) {
542ae60af24f02 Dongpo Li      2016-07-15  820  		ret = PTR_ERR(priv->glb_base);
542ae60af24f02 Dongpo Li      2016-07-15  821  		goto out_free_netdev;
542ae60af24f02 Dongpo Li      2016-07-15  822  	}
542ae60af24f02 Dongpo Li      2016-07-15  823  
3c2c758a1607ea Yang Xiwen     2024-02-16  824  	for (i = 0; i < CLK_NUM; i++) {
3c2c758a1607ea Yang Xiwen     2024-02-16  825  		priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]);
3c2c758a1607ea Yang Xiwen     2024-02-16  826  		if (IS_ERR(priv->clks[i])) {
3c2c758a1607ea Yang Xiwen     2024-02-16  827  			dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i],
3c2c758a1607ea Yang Xiwen     2024-02-16  828  				PTR_ERR(priv->clks[i]));
542ae60af24f02 Dongpo Li      2016-07-15  829  			ret = -ENODEV;
542ae60af24f02 Dongpo Li      2016-07-15  830  			goto out_free_netdev;
542ae60af24f02 Dongpo Li      2016-07-15  831  		}
542ae60af24f02 Dongpo Li      2016-07-15  832  	}
542ae60af24f02 Dongpo Li      2016-07-15  833  
542ae60af24f02 Dongpo Li      2016-07-15  834  	priv->mac_rst = devm_reset_control_get(dev, "mac");
542ae60af24f02 Dongpo Li      2016-07-15  835  	if (IS_ERR(priv->mac_rst)) {
542ae60af24f02 Dongpo Li      2016-07-15  836  		ret = PTR_ERR(priv->mac_rst);
3c2c758a1607ea Yang Xiwen     2024-02-16  837  		goto out_free_netdev;
542ae60af24f02 Dongpo Li      2016-07-15  838  	}
542ae60af24f02 Dongpo Li      2016-07-15  839  	hisi_femac_core_reset(priv);
542ae60af24f02 Dongpo Li      2016-07-15  840  
542ae60af24f02 Dongpo Li      2016-07-15  841  	priv->phy_rst = devm_reset_control_get(dev, "phy");
542ae60af24f02 Dongpo Li      2016-07-15  842  	if (IS_ERR(priv->phy_rst)) {
542ae60af24f02 Dongpo Li      2016-07-15  843  		priv->phy_rst = NULL;
542ae60af24f02 Dongpo Li      2016-07-15  844  	} else {
542ae60af24f02 Dongpo Li      2016-07-15  845  		ret = of_property_read_u32_array(node,
542ae60af24f02 Dongpo Li      2016-07-15  846  						 PHY_RESET_DELAYS_PROPERTY,
542ae60af24f02 Dongpo Li      2016-07-15  847  						 priv->phy_reset_delays,
542ae60af24f02 Dongpo Li      2016-07-15  848  						 DELAYS_NUM);
542ae60af24f02 Dongpo Li      2016-07-15  849  		if (ret)
3c2c758a1607ea Yang Xiwen     2024-02-16  850  			goto out_free_netdev;
542ae60af24f02 Dongpo Li      2016-07-15  851  		hisi_femac_phy_reset(priv);
542ae60af24f02 Dongpo Li      2016-07-15  852  	}
542ae60af24f02 Dongpo Li      2016-07-15  853  
3c2c758a1607ea Yang Xiwen     2024-02-16  854  	// Register the optional MDIO bus
3c2c758a1607ea Yang Xiwen     2024-02-16  855  	for_each_available_child_of_node(node, mdio_np) {
3c2c758a1607ea Yang Xiwen     2024-02-16  856  		if (of_node_name_prefix(mdio_np, "mdio")) {
3c2c758a1607ea Yang Xiwen     2024-02-16  857  			priv->mdio_pdev = of_platform_device_create(mdio_np, NULL, dev);
3c2c758a1607ea Yang Xiwen     2024-02-16  858  			of_node_put(mdio_np);
3c2c758a1607ea Yang Xiwen     2024-02-16  859  			if (!priv->mdio_pdev) {
3c2c758a1607ea Yang Xiwen     2024-02-16  860  				dev_err(dev, "failed to register MDIO bus device\n");

ret = -ENOMEM;

3c2c758a1607ea Yang Xiwen     2024-02-16  861  				goto out_free_netdev;
3c2c758a1607ea Yang Xiwen     2024-02-16  862  			}
3c2c758a1607ea Yang Xiwen     2024-02-16  863  			mdio_registered = true;
3c2c758a1607ea Yang Xiwen     2024-02-16  864  			break;
3c2c758a1607ea Yang Xiwen     2024-02-16  865  		}
3c2c758a1607ea Yang Xiwen     2024-02-16  866  	}
3c2c758a1607ea Yang Xiwen     2024-02-16  867  
3c2c758a1607ea Yang Xiwen     2024-02-16  868  	if (!mdio_registered)
3c2c758a1607ea Yang Xiwen     2024-02-16  869  		dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n");
3c2c758a1607ea Yang Xiwen     2024-02-16  870  
542ae60af24f02 Dongpo Li      2016-07-15  871  	phy = of_phy_get_and_connect(ndev, node, hisi_femac_adjust_link);
542ae60af24f02 Dongpo Li      2016-07-15  872  	if (!phy) {
542ae60af24f02 Dongpo Li      2016-07-15  873  		dev_err(dev, "connect to PHY failed!\n");
542ae60af24f02 Dongpo Li      2016-07-15  874  		ret = -ENODEV;
3c2c758a1607ea Yang Xiwen     2024-02-16  875  		goto out_unregister_mdio_bus;
542ae60af24f02 Dongpo Li      2016-07-15  876  	}
542ae60af24f02 Dongpo Li      2016-07-15  877  
542ae60af24f02 Dongpo Li      2016-07-15  878  	phy_attached_print(phy, "phy_id=0x%.8lx, phy_mode=%s\n",
542ae60af24f02 Dongpo Li      2016-07-15  879  			   (unsigned long)phy->phy_id,
542ae60af24f02 Dongpo Li      2016-07-15  880  			   phy_modes(phy->interface));
542ae60af24f02 Dongpo Li      2016-07-15  881  
9ca01b25dffffe Jakub Kicinski 2021-10-06  882  	ret = of_get_ethdev_address(node, ndev);
83216e3988cd19 Michael Walle  2021-04-12  883  	if (ret) {
542ae60af24f02 Dongpo Li      2016-07-15  884  		eth_hw_addr_random(ndev);
542ae60af24f02 Dongpo Li      2016-07-15  885  		dev_warn(dev, "using random MAC address %pM\n",
542ae60af24f02 Dongpo Li      2016-07-15  886  			 ndev->dev_addr);
542ae60af24f02 Dongpo Li      2016-07-15  887  	}
542ae60af24f02 Dongpo Li      2016-07-15  888  
542ae60af24f02 Dongpo Li      2016-07-15  889  	ndev->watchdog_timeo = 6 * HZ;
542ae60af24f02 Dongpo Li      2016-07-15  890  	ndev->priv_flags |= IFF_UNICAST_FLT;
542ae60af24f02 Dongpo Li      2016-07-15  891  	ndev->netdev_ops = &hisi_femac_netdev_ops;
542ae60af24f02 Dongpo Li      2016-07-15  892  	ndev->ethtool_ops = &hisi_femac_ethtools_ops;
b707b89f7be361 Jakub Kicinski 2022-05-06  893  	netif_napi_add_weight(ndev, &priv->napi, hisi_femac_poll,
b707b89f7be361 Jakub Kicinski 2022-05-06  894  			      FEMAC_POLL_WEIGHT);
542ae60af24f02 Dongpo Li      2016-07-15  895  
542ae60af24f02 Dongpo Li      2016-07-15  896  	hisi_femac_port_init(priv);
542ae60af24f02 Dongpo Li      2016-07-15  897  
542ae60af24f02 Dongpo Li      2016-07-15  898  	ret = hisi_femac_init_tx_and_rx_queues(priv);
542ae60af24f02 Dongpo Li      2016-07-15  899  	if (ret)
542ae60af24f02 Dongpo Li      2016-07-15  900  		goto out_disconnect_phy;
542ae60af24f02 Dongpo Li      2016-07-15  901  
542ae60af24f02 Dongpo Li      2016-07-15  902  	ndev->irq = platform_get_irq(pdev, 0);
ae1d60c41e581b Ruan Jinjie    2023-07-31  903  	if (ndev->irq < 0) {
ae1d60c41e581b Ruan Jinjie    2023-07-31  904  		ret = ndev->irq;
542ae60af24f02 Dongpo Li      2016-07-15  905  		goto out_disconnect_phy;
542ae60af24f02 Dongpo Li      2016-07-15  906  	}
542ae60af24f02 Dongpo Li      2016-07-15  907  
542ae60af24f02 Dongpo Li      2016-07-15  908  	ret = devm_request_irq(dev, ndev->irq, hisi_femac_interrupt,
542ae60af24f02 Dongpo Li      2016-07-15  909  			       IRQF_SHARED, pdev->name, ndev);
542ae60af24f02 Dongpo Li      2016-07-15  910  	if (ret) {
542ae60af24f02 Dongpo Li      2016-07-15  911  		dev_err(dev, "devm_request_irq %d failed!\n", ndev->irq);
542ae60af24f02 Dongpo Li      2016-07-15  912  		goto out_disconnect_phy;
542ae60af24f02 Dongpo Li      2016-07-15  913  	}
542ae60af24f02 Dongpo Li      2016-07-15  914  
542ae60af24f02 Dongpo Li      2016-07-15  915  	ret = register_netdev(ndev);
542ae60af24f02 Dongpo Li      2016-07-15  916  	if (ret) {
542ae60af24f02 Dongpo Li      2016-07-15  917  		dev_err(dev, "register_netdev failed!\n");
542ae60af24f02 Dongpo Li      2016-07-15  918  		goto out_disconnect_phy;
542ae60af24f02 Dongpo Li      2016-07-15  919  	}
542ae60af24f02 Dongpo Li      2016-07-15  920  
542ae60af24f02 Dongpo Li      2016-07-15  921  	return ret;
542ae60af24f02 Dongpo Li      2016-07-15  922  
542ae60af24f02 Dongpo Li      2016-07-15  923  out_disconnect_phy:
542ae60af24f02 Dongpo Li      2016-07-15  924  	netif_napi_del(&priv->napi);
542ae60af24f02 Dongpo Li      2016-07-15  925  	phy_disconnect(phy);
3c2c758a1607ea Yang Xiwen     2024-02-16  926  out_unregister_mdio_bus:
3c2c758a1607ea Yang Xiwen     2024-02-16  927  	platform_device_unregister(priv->mdio_pdev);
542ae60af24f02 Dongpo Li      2016-07-15  928  out_free_netdev:
542ae60af24f02 Dongpo Li      2016-07-15  929  	free_netdev(ndev);
542ae60af24f02 Dongpo Li      2016-07-15  930  
542ae60af24f02 Dongpo Li      2016-07-15 @931  	return ret;
542ae60af24f02 Dongpo Li      2016-07-15  932  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/6] net: hisi_femac: remove unused compatible strings

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

These compatible strings are not found in any mainline dts, remove them.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/net/ethernet/hisilicon/hisi_femac.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index d72160efff9a..6dabc62a00b7 100644
--- a/drivers/net/ethernet/hisilicon/hisi_femac.c
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -990,9 +990,6 @@ 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,hi3798mv200-femac",},
 	{},
 };

-- 
2.43.0

Re: [PATCH 2/6] net: hisi_femac: remove unused compatible strings

[Krzysztof Kozlowski]

On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> These compatible strings are not found in any mainline dts, remove them.

That's not a real reason. What about all other users?

Best regards,
Krzysztof

Re: [PATCH 2/6] net: hisi_femac: remove unused compatible strings

[Yang Xiwen]

On 2/16/2024 3:20 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> These compatible strings are not found in any mainline dts, remove them.
> That's not a real reason. What about all other users?
The people who want their devices being supported should post a working 
dts first. Having found the dts missing is strongly telling me that this 
SoC(Hi3516) is orphan and EOL already. I can't even find it in git 
commit logs. I'll argue that the old binding is simply wrong, and does 
not describe the hardware properly. Who knows? Could anyone tell me if 
the driver is still working for Hi3516 or not? I'm very willing to keep 
the backward compatibility if someone can tell me the effort i paid to 
maintain the old binding really makes sense. But the only things i found 
in mainline kernel about Hi3516 is an CRG(clock) driver and this femac 
driver. And it's been 8 years since last update for this SoC.
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Re: [PATCH 2/6] net: hisi_femac: remove unused compatible strings

[Krzysztof Kozlowski]

On 16/02/2024 09:21, Yang Xiwen wrote:
> On 2/16/2024 3:20 PM, Krzysztof Kozlowski wrote:
>> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> These compatible strings are not found in any mainline dts, remove them.
>> That's not a real reason. What about all other users?
> The people who want their devices being supported should post a working 
> dts first. Having found the dts missing is strongly telling me that this 

Considering how poor HiSilicon contributions were - in numbers and
quality - that's kind of expected. :(


> SoC(Hi3516) is orphan and EOL already. I can't even find it in git 
> commit logs. I'll argue that the old binding is simply wrong, and does 
> not describe the hardware properly. Who knows? Could anyone tell me if 
> the driver is still working for Hi3516 or not? I'm very willing to keep 
> the backward compatibility if someone can tell me the effort i paid to 
> maintain the old binding really makes sense. But the only things i found 
> in mainline kernel about Hi3516 is an CRG(clock) driver and this femac 
> driver. And it's been 8 years since last update for this SoC.

OK, that's fine with me, but please add parts of this explanation to the
commit msg (SoC is EOL, driver looks buggy and might not even work,
platform was upstreamed 8 years ago and no maintenance work happened on
it, thus it looks abandoned etc.).

Best regards,
Krzysztof

Re: [PATCH 2/6] net: hisi_femac: remove unused compatible strings

[Yang Xiwen]

On 2/16/2024 4:26 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 09:21, Yang Xiwen wrote:
>> On 2/16/2024 3:20 PM, Krzysztof Kozlowski wrote:
>>> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
>>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>>
>>>> These compatible strings are not found in any mainline dts, remove them.
>>> That's not a real reason. What about all other users?
>> The people who want their devices being supported should post a working
>> dts first. Having found the dts missing is strongly telling me that this
> Considering how poor HiSilicon contributions were - in numbers and
> quality - that's kind of expected. :(
>
>
>> SoC(Hi3516) is orphan and EOL already. I can't even find it in git
>> commit logs. I'll argue that the old binding is simply wrong, and does
>> not describe the hardware properly. Who knows? Could anyone tell me if
>> the driver is still working for Hi3516 or not? I'm very willing to keep
>> the backward compatibility if someone can tell me the effort i paid to
>> maintain the old binding really makes sense. But the only things i found
>> in mainline kernel about Hi3516 is an CRG(clock) driver and this femac
>> driver. And it's been 8 years since last update for this SoC.
> OK, that's fine with me, but please add parts of this explanation to the
> commit msg (SoC is EOL, driver looks buggy and might not even work,
> platform was upstreamed 8 years ago and no maintenance work happened on
> it, thus it looks abandoned etc.).

For me, it's a bit lucky to find a (partially) working driver in 
mainline. It'll take me even more time if no mainline driver is 
available. In fact, i wrote the driver for mainline u-boot from scratch 
and it has been merged. So it's good to have this binding accepted 
unmodified, or i'll have to modify u-boot side driver code to keep them 
sync.

>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Re: [PATCH 2/6] net: hisi_femac: remove unused compatible strings

[Andrew Lunn]

> For me, it's a bit lucky to find a (partially) working driver in mainline.
> It'll take me even more time if no mainline driver is available. In fact, i
> wrote the driver for mainline u-boot from scratch and it has been merged. So
> it's good to have this binding accepted unmodified, or i'll have to modify
> u-boot side driver code to keep them sync.

Sorry, but that is not how it works. If during review we decided it
needs to be modified, you will need to modify it.

I would suggest you first mainstream bindings to the kernel, because
it has active DT maintainers how really care about bindings. Then get
is merged to u-boot.

   Andrew

Re: [PATCH 2/6] net: hisi_femac: remove unused compatible strings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On Fri, Feb 16, 2024 at 02:01:08PM +0100, Andrew Lunn wrote:
> > For me, it's a bit lucky to find a (partially) working driver in mainline.
> > It'll take me even more time if no mainline driver is available. In fact, i
> > wrote the driver for mainline u-boot from scratch and it has been merged. So
> > it's good to have this binding accepted unmodified, or i'll have to modify
> > u-boot side driver code to keep them sync.
> 
> Sorry, but that is not how it works. If during review we decided it
> needs to be modified, you will need to modify it.
> 
> I would suggest you first mainstream bindings to the kernel, because
> it has active DT maintainers how really care about bindings. Then get
> is merged to u-boot.

Just to note, the U-Boot folk are currently working on a model where
they will be importing the kernel's dts files directly into their tree
along with the bindings. I think they're adding dtbs_check too.
Although that will be opt-in per board, it does point to an increased
desire for compliance there too, which is great.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/6] net: hisi_femac: remove unused compatible strings

[Yang Xiwen]

On 2/17/2024 4:05 AM, Conor Dooley wrote:
> On Fri, Feb 16, 2024 at 02:01:08PM +0100, Andrew Lunn wrote:
>>> For me, it's a bit lucky to find a (partially) working driver in mainline.
>>> It'll take me even more time if no mainline driver is available. In fact, i
>>> wrote the driver for mainline u-boot from scratch and it has been merged. So
>>> it's good to have this binding accepted unmodified, or i'll have to modify
>>> u-boot side driver code to keep them sync.
>> Sorry, but that is not how it works. If during review we decided it
>> needs to be modified, you will need to modify it.
>>
>> I would suggest you first mainstream bindings to the kernel, because
>> it has active DT maintainers how really care about bindings. Then get
>> is merged to u-boot.
> Just to note, the U-Boot folk are currently working on a model where
> they will be importing the kernel's dts files directly into their tree
> along with the bindings. I think they're adding dtbs_check too.
> Although that will be opt-in per board, it does point to an increased
> desire for compliance there too, which is great.

Of course. I'll sync this stuff back to u-boot once this gets accepted 
and merged. I begin working from u-boot simply because the Driver Model 
of U-Boot is much simpler than Linux's. I wrote the driver for U-Boot 
first to figure out how the hardware is working, then port it to Linux.

-- 
Regards,
Yang Xiwen

[PATCH 3/6] dt-bindings: net: remove outdated hisilicon-femac

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

The user documented(Hi3516) is not found in current kernel anymore.
Remove this binding entirely due to recent driver changes.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../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

Re: [PATCH 3/6] dt-bindings: net: remove outdated hisilicon-femac

[Krzysztof Kozlowski]

On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> The user documented(Hi3516) is not found in current kernel anymore.
> Remove this binding entirely due to recent driver changes.

Hardware does not change because you decided to re-implement driver.


Best regards,
Krzysztof

Re: [PATCH 3/6] dt-bindings: net: remove outdated hisilicon-femac

[Yang Xiwen]

On 2/16/2024 3:21 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> The user documented(Hi3516) is not found in current kernel anymore.
>> Remove this binding entirely due to recent driver changes.
> Hardware does not change because you decided to re-implement driver.

The only hardware i have is the hi3798mv200. According to downstream 
driver name, this is supposed to be a hisi-femac-v3 actually. I don't 
know much about Hi3516, but it confuses me a lot. According to the 
device tree node example in the text binding file, the MDIO bus is 
supposed to be inside the femac core (femac core is at 0x0-0x1000 & 
0x1100-0x1300 and mdio bus is at 0x1100-0x1120). So i think it's highly 
possible they are the same hardware. But according to the TRM and my 
tests, there are 3 clocks in total for femac core in hi3798mv200, one 
for mac ctrl, one for ahb bus(I'm sure this "bus" clock is not MDIO bus 
clock), and one for phy, which is very similar to the hisi-gmac driver.

Which complicates things a lot is the complex clock enabling timing 
requirements here. at least for hi3798mv200(and all SoCs with 
hisi-femac-v3 core i think according to the downstream kernel source), 
It must strictly follow the sequence in hisi_femac_phy_reset() (disable 
MAC clk and BUS clk first before asserting PHY reset), or the PHY would 
fail to work. So as said in previous reply, the simplest way is to do 
all resets and clocks management in the MAC driver, or else it'll be 
very hard to implement. I can't find an easy way to "tell" a driver to 
kindly disable its clocks remotely.

>
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Re: [PATCH 3/6] dt-bindings: net: remove outdated hisilicon-femac

[Krzysztof Kozlowski]

On 16/02/2024 09:03, Yang Xiwen wrote:
> On 2/16/2024 3:21 PM, Krzysztof Kozlowski wrote:
>> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> The user documented(Hi3516) is not found in current kernel anymore.
>>> Remove this binding entirely due to recent driver changes.
>> Hardware does not change because you decided to re-implement driver.
> 
> The only hardware i have is the hi3798mv200. According to downstream 
> driver name, this is supposed to be a hisi-femac-v3 actually. I don't 
> know much about Hi3516, but it confuses me a lot. According to the 
> device tree node example in the text binding file, the MDIO bus is 
> supposed to be inside the femac core (femac core is at 0x0-0x1000 & 
> 0x1100-0x1300 and mdio bus is at 0x1100-0x1120). So i think it's highly 
> possible they are the same hardware. But according to the TRM and my 
> tests, there are 3 clocks in total for femac core in hi3798mv200, one 
> for mac ctrl, one for ahb bus(I'm sure this "bus" clock is not MDIO bus 
> clock), and one for phy, which is very similar to the hisi-gmac driver.
> 
> Which complicates things a lot is the complex clock enabling timing 
> requirements here. at least for hi3798mv200(and all SoCs with 
> hisi-femac-v3 core i think according to the downstream kernel source), 
> It must strictly follow the sequence in hisi_femac_phy_reset() (disable 
> MAC clk and BUS clk first before asserting PHY reset), or the PHY would 
> fail to work. So as said in previous reply, the simplest way is to do 
> all resets and clocks management in the MAC driver, or else it'll be 
> very hard to implement. I can't find an easy way to "tell" a driver to 
> kindly disable its clocks remotely.
> 

None of these explain why support for these devices should be dropped...

Best regards,
Krzysztof

[PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

This binding gets rewritten. Compared to previous txt based binding doc,
the following changes are made:

- No "hisi-femac-v1/2" binding anymore
- Remove unused Hi3516 SoC, add Hi3798MV200
- add MDIO subnode
- add phy clock and reset

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../devicetree/bindings/net/hisilicon-femac.yaml   | 125 +++++++++++++++++++++
 1 file changed, 125 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon-femac.yaml
new file mode 100644
index 000000000000..008127e148aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon-femac.yaml
@@ -0,0 +1,125 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hisilicon-femac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon Fast Ethernet MAC controller
+
+maintainers:
+  - Yang Xiwen <forbidden405@foxmail.com>
+
+allOf:
+  - $ref: ethernet-controller.yaml
+
+properties:
+  compatible:
+    enum:
+      - hisilicon,hi3798mv200-femac
+
+  reg:
+    minItems: 2
+    maxItems: 2
+    description: |
+      The first region is the MAC core register base and size.
+      The second region is the global MAC control register.
+
+  ranges:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    minItems: 3
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: mac
+      - const: macif
+      - const: phy
+
+  resets:
+    minItems: 2
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: mac
+      - const: phy
+
+  hisilicon,phy-reset-delays-us:
+    minItems: 3
+    maxItems: 3
+    description: |
+      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.
+
+patternProperties:
+  '^mdio@[0-9a-f]+$':
+    type: object
+    description: See ./hisi-femac-mdio.txt
+
+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>
+
+    #ifndef HISTB_ETH0_PHY_CLK
+    #define HISTB_ETH0_PHY_CLK 0
+    #endif
+    femac: ethernet@9c30000 {
+        compatible = "hisilicon,hi3798mv200-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 HISTB_ETH0_PHY_CLK>;
+        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: mdio@1100 {
+            compatible = "hisilicon,hisi-femac-mdio";
+            reg = <0x1100 0x20>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            status = "okay";
+
+            fephy: ethernet-phy@1 {
+                reg = <1>;
+                #phy-cells = <0>;
+            };
+        };
+    };

-- 
2.43.0

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Andrew Lunn]

> +  clocks:
> +    minItems: 3
> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: mac
> +      - const: macif
> +      - const: phy

The C code has:

+enum clk_type {
+       CLK_MAC,
+       CLK_BUS,
+       CLK_PHY,
+       CLK_NUM,
+};

Could you explain BUS vs macif?

Also, what exactly is the PHY clock?

      Andrew

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Yang Xiwen]

On 2/16/2024 8:06 AM, Andrew Lunn wrote:
>> +  clocks:
>> +    minItems: 3
>> +    maxItems: 3
>> +
>> +  clock-names:
>> +    items:
>> +      - const: mac
>> +      - const: macif
>> +      - const: phy
> The C code has:
>
> +enum clk_type {
> +       CLK_MAC,
> +       CLK_BUS,
> +       CLK_PHY,
> +       CLK_NUM,
> +};
>
> Could you explain BUS vs macif?
To be honest, I don't know. "macif" is used by hisi-gmac driver, but in 
the TRM it's called "bus". So I guess it's just an alias? As you 
mentioned, I'll stick to macif everywhere, to keep sync with hisi-gmac 
driver.
>
> Also, what exactly is the PHY clock?
As the name suggests, it's not part of the mac controller actually, 
rather it's the clock of the internal PHY. The SoC (or the PHY/MAC, I 
don't know exactly which) is quirky that it is mandatory to disable PHY 
CLK before MAC reset or else the PHY won't work(see 
hisi_femac_phy_reset()). I can't find a better solution. Letting the 
ethernet controller manage all clocks and resets seems the easiest way 
to handle this quirk.
>
>        Andrew


-- 
Regards,
Yang Xiwen

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Rob Herring]

On Fri, 16 Feb 2024 07:48:56 +0800, Yang Xiwen wrote:
> This binding gets rewritten. Compared to previous txt based binding doc,
> the following changes are made:
> 
> - No "hisi-femac-v1/2" binding anymore
> - Remove unused Hi3516 SoC, add Hi3798MV200
> - add MDIO subnode
> - add phy clock and reset
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  .../devicetree/bindings/net/hisilicon-femac.yaml   | 125 +++++++++++++++++++++
>  1 file changed, 125 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/net/hisilicon-femac.example.dtb: /example-0/ethernet@9c30000/mdio@1100: failed to match any schema with compatible: ['hisilicon,hisi-femac-mdio']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240216-net-v1-4-e0ad972cda99@outlook.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Yang Xiwen]

On 2/16/2024 10:09 AM, Rob Herring wrote:
> On Fri, 16 Feb 2024 07:48:56 +0800, Yang Xiwen wrote:
>> This binding gets rewritten. Compared to previous txt based binding doc,
>> the following changes are made:
>>
>> - No "hisi-femac-v1/2" binding anymore
>> - Remove unused Hi3516 SoC, add Hi3798MV200
>> - add MDIO subnode
>> - add phy clock and reset
>>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   .../devicetree/bindings/net/hisilicon-femac.yaml   | 125 +++++++++++++++++++++
>>   1 file changed, 125 insertions(+)
>>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/net/hisilicon-femac.example.dtb: /example-0/ethernet@9c30000/mdio@1100: failed to match any schema with compatible: ['hisilicon,hisi-femac-mdio']
it's fine. This compatible is documented in a plain text file 
`./hisi-femac-mdio.txt`.
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240216-net-v1-4-e0ad972cda99@outlook.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>

-- 
Regards,
Yang Xiwen

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Krzysztof Kozlowski]

On 16/02/2024 07:30, Yang Xiwen wrote:
> On 2/16/2024 10:09 AM, Rob Herring wrote:
>> On Fri, 16 Feb 2024 07:48:56 +0800, Yang Xiwen wrote:
>>> This binding gets rewritten. Compared to previous txt based binding doc,
>>> the following changes are made:
>>>
>>> - No "hisi-femac-v1/2" binding anymore
>>> - Remove unused Hi3516 SoC, add Hi3798MV200
>>> - add MDIO subnode
>>> - add phy clock and reset
>>>
>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>>> ---
>>>   .../devicetree/bindings/net/hisilicon-femac.yaml   | 125 +++++++++++++++++++++
>>>   1 file changed, 125 insertions(+)
>>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> Documentation/devicetree/bindings/net/hisilicon-femac.example.dtb: /example-0/ethernet@9c30000/mdio@1100: failed to match any schema with compatible: ['hisilicon,hisi-femac-mdio']
> it's fine. This compatible is documented in a plain text file 
> `./hisi-femac-mdio.txt`.

No, it is not fine. This must be fixed.

Best regards,
Krzysztof

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Krzysztof Kozlowski]

On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> This binding gets rewritten. Compared to previous txt based binding doc,
> the following changes are made:
> 
> - No "hisi-femac-v1/2" binding anymore
> - Remove unused Hi3516 SoC, add Hi3798MV200
> - add MDIO subnode
> - add phy clock and reset

I don't understand why conversion you make in two commits. Please
perform proper conversion and then propose changes to the binding.

> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  .../devicetree/bindings/net/hisilicon-femac.yaml   | 125 +++++++++++++++++++++
>  1 file changed, 125 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon-femac.yaml
> new file mode 100644
> index 000000000000..008127e148aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hisilicon-femac.yaml

Use compatible as filename.

> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/hisilicon-femac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hisilicon Fast Ethernet MAC controller
> +
> +maintainers:
> +  - Yang Xiwen <forbidden405@foxmail.com>
> +
> +allOf:
> +  - $ref: ethernet-controller.yaml
> +
> +properties:
> +  compatible:
> +    enum:
> +      - hisilicon,hi3798mv200-femac
> +
> +  reg:
> +    minItems: 2
> +    maxItems: 2
> +    description: |
> +      The first region is the MAC core register base and size.
> +      The second region is the global MAC control register.

Just items: - description: instead of all this.

> +
> +  ranges:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 3

Drop

> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: mac
> +      - const: macif
> +      - const: phy
> +
> +  resets:
> +    minItems: 2

Drop

> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: mac
> +      - const: phy
> +
> +  hisilicon,phy-reset-delays-us:
> +    minItems: 3
> +    maxItems: 3
> +    description: |
> +      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.

items:
 - description:

Anyway, isn't this property of the phy?


> +
> +patternProperties:
> +  '^mdio@[0-9a-f]+$':
> +    type: object
> +    description: See ./hisi-femac-mdio.txt

No, please first convert other file and then reference it here.

> +
> +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>
> +
> +    #ifndef HISTB_ETH0_PHY_CLK
> +    #define HISTB_ETH0_PHY_CLK 0
> +    #endif

Drop these defines.

> +    femac: ethernet@9c30000 {

Drop label.

> +        compatible = "hisilicon,hi3798mv200-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 HISTB_ETH0_PHY_CLK>;
> +        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";

Drop

> +
> +        mdio: mdio@1100 {

Drop label

> +            compatible = "hisilicon,hisi-femac-mdio";
> +            reg = <0x1100 0x20>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            status = "okay";

Drop

> +
> +            fephy: ethernet-phy@1 {

Drop label


> +                reg = <1>;
> +                #phy-cells = <0>;
> +            };
> +        };
> +    };
> 

Best regards,
Krzysztof

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Yang Xiwen]

On 2/16/2024 3:24 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> This binding gets rewritten. Compared to previous txt based binding doc,
>> the following changes are made:
>>
>> - No "hisi-femac-v1/2" binding anymore
>> - Remove unused Hi3516 SoC, add Hi3798MV200
>> - add MDIO subnode
>> - add phy clock and reset
> I don't understand why conversion you make in two commits. Please
> perform proper conversion and then propose changes to the binding.
>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   .../devicetree/bindings/net/hisilicon-femac.yaml   | 125 +++++++++++++++++++++
>>   1 file changed, 125 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon-femac.yaml
>> new file mode 100644
>> index 000000000000..008127e148aa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/hisilicon-femac.yaml
> Use compatible as filename.
>
>> @@ -0,0 +1,125 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/hisilicon-femac.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Hisilicon Fast Ethernet MAC controller
>> +
>> +maintainers:
>> +  - Yang Xiwen <forbidden405@foxmail.com>
>> +
>> +allOf:
>> +  - $ref: ethernet-controller.yaml
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - hisilicon,hi3798mv200-femac
>> +
>> +  reg:
>> +    minItems: 2
>> +    maxItems: 2
>> +    description: |
>> +      The first region is the MAC core register base and size.
>> +      The second region is the global MAC control register.
> Just items: - description: instead of all this.
>
>> +
>> +  ranges:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    minItems: 3
> Drop
>
>> +    maxItems: 3
>> +
>> +  clock-names:
>> +    items:
>> +      - const: mac
>> +      - const: macif
>> +      - const: phy
>> +
>> +  resets:
>> +    minItems: 2
> Drop
>
>> +    maxItems: 2
>> +
>> +  reset-names:
>> +    items:
>> +      - const: mac
>> +      - const: phy
>> +
>> +  hisilicon,phy-reset-delays-us:
>> +    minItems: 3
>> +    maxItems: 3
>> +    description: |
>> +      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.
> items:
>   - description:
>
> Anyway, isn't this property of the phy?
It ought to be. But it seems a bit hard to implement it like this.
>
>
>> +
>> +patternProperties:
>> +  '^mdio@[0-9a-f]+$':
>> +    type: object
>> +    description: See ./hisi-femac-mdio.txt
> No, please first convert other file and then reference it here.
>
>> +
>> +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>
>> +
>> +    #ifndef HISTB_ETH0_PHY_CLK
>> +    #define HISTB_ETH0_PHY_CLK 0
>> +    #endif
> Drop these defines.

It is depending on a local patch which update 
include/dt-bindings/clock/histb-clock.h. But currently i'm not going to 
sent it since i'm still tweaking clock drivers frequently.

Removing these defines now would probably leads to a compile error.

Or I can just change them to some magic numbers.

>
>> +    femac: ethernet@9c30000 {
> Drop label.
>
>> +        compatible = "hisilicon,hi3798mv200-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 HISTB_ETH0_PHY_CLK>;
>> +        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";
> Drop
>
>> +
>> +        mdio: mdio@1100 {
> Drop label
>
>> +            compatible = "hisilicon,hisi-femac-mdio";
>> +            reg = <0x1100 0x20>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +            status = "okay";
> Drop
>
>> +
>> +            fephy: ethernet-phy@1 {
> Drop label
>
>
>> +                reg = <1>;
>> +                #phy-cells = <0>;
>> +            };
>> +        };
>> +    };
>>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Krzysztof Kozlowski]

On 16/02/2024 10:36, Yang Xiwen wrote:
>>
>>> +    maxItems: 2
>>> +
>>> +  reset-names:
>>> +    items:
>>> +      - const: mac
>>> +      - const: phy
>>> +
>>> +  hisilicon,phy-reset-delays-us:
>>> +    minItems: 3
>>> +    maxItems: 3
>>> +    description: |
>>> +      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.
>> items:
>>   - description:
>>
>> Anyway, isn't this property of the phy?
> It ought to be. But it seems a bit hard to implement it like this.

Why? You have phy node, so phy should know what to do.



Best regards,
Krzysztof

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Yang Xiwen]

On 2/16/2024 5:41 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 10:36, Yang Xiwen wrote:
>>>> +    maxItems: 2
>>>> +
>>>> +  reset-names:
>>>> +    items:
>>>> +      - const: mac
>>>> +      - const: phy
>>>> +
>>>> +  hisilicon,phy-reset-delays-us:
>>>> +    minItems: 3
>>>> +    maxItems: 3
>>>> +    description: |
>>>> +      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.
>>> items:
>>>    - description:
>>>
>>> Anyway, isn't this property of the phy?
>> It ought to be. But it seems a bit hard to implement it like this.
> Why? You have phy node, so phy should know what to do.
It's not PHY which needs special treatment. It's the MAC. The PHY needs 
to be reset with MAC and BUS clocks disabled explicitly(see 
hisi_femac_phy_reset() in patch 1). I can't find a way to implement this 
easily except moving all clocks/resets management to MAC driver. 
Especially when the boot loader might has enabled the clocks already (so 
the clocks need to be disabled explicitly(i.e. call clk_prepare_enable() 
and clk_disable() for them, and ensure there is only one consumer).
>
>
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Andrew Lunn]

> +    femac: ethernet@9c30000 {
> +        compatible = "hisilicon,hi3798mv200-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 HISTB_ETH0_PHY_CLK>;
> +        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: mdio@1100 {
> +            compatible = "hisilicon,hisi-femac-mdio";

Is the MDIO bus master a device of its own? Is this compatible
actually used anywhere?

There are generally two different ways an MDIO bus master works. It is
an individual device, with its own register space and it gets probed
as an independent device. In this case, it needs a compatible to
indicate what driver should be used.

Or the MDIO bus master is embedded within a MAC driver. It shares the
register space with the MAC driver. It is not a device which gets
probed, and so it does not need a compatible. The MAC drivers
compatible is sufficient.

	Andrew

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Yang Xiwen]

On 2/16/2024 9:11 PM, Andrew Lunn wrote:
>> +    femac: ethernet@9c30000 {
>> +        compatible = "hisilicon,hi3798mv200-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 HISTB_ETH0_PHY_CLK>;
>> +        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: mdio@1100 {
>> +            compatible = "hisilicon,hisi-femac-mdio";
> Is the MDIO bus master a device of its own? Is this compatible
> actually used anywhere?
>
> There are generally two different ways an MDIO bus master works. It is
> an individual device, with its own register space and it gets probed
> as an independent device. In this case, it needs a compatible to
> indicate what driver should be used.
>
> Or the MDIO bus master is embedded within a MAC driver. It shares the
> register space with the MAC driver. It is not a device which gets
> probed, and so it does not need a compatible. The MAC drivers
> compatible is sufficient.

I guess this is the main difference between the old "hi3516dv300-femac" 
and the new "hi3798mv200-femac".

The old binding says there are individual clocks for MDIO bus and MAC 
for hi3516. But according to my test, it's not true for hi3798mv200.

I've tried accessing MDIO address space and MAC controller address space 
in u-boot with `md` and `mw` [1]. From the result, i guess the CLK_BUS 
is the System Bus clock (AHB Bus clock), and the CLK_MAC is the clock 
shared by both MDIO bus and MAC. The MAC has a internal clock divider to 
divide the input clock(54MHz in common) to a configurable variable rate.

Due to the fact that MDIO bus address space is inside MAC address space 
( MAC is at `f9c30000- f9c40000`, MDIO bus is at `f9c31100-f9c31120` ). 
The only thing i can tell it's inside the SoC, but i can't say it's 100% 
completely integrated to the MAC controller, though highly possible.

[1]: the result is summarized below, `md f9c31300` in u-boot is used to 
poke the register values:

CLK_BUS  |  CLK_MAC  | register values

Off | Off | All zero

Off | On | All zero

On | Off | System Hangs

On | On | All okay

>
> 	Andrew


-- 
Regards,
Yang Xiwen

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Andrew Lunn]

> I've tried accessing MDIO address space and MAC controller address space in
> u-boot with `md` and `mw` [1]. From the result, i guess the CLK_BUS is the
> System Bus clock (AHB Bus clock), and the CLK_MAC is the clock shared by
> both MDIO bus and MAC. The MAC has a internal clock divider to divide the
> input clock(54MHz in common) to a configurable variable rate.

In general, sharing a clock is not a problem. The clock API does
reference counting. So if two consumers enable the clock, it will not
be disabled until two consumes disable the clock. So it should not be
an issue for both the MAC and the MDIO driver to consume the clock.

However, the funny PHY reset code is going to be key here. We need to
understand that in more detail.

Talking about details, you commit messages need improving. The commit
message is your chance to answer all the reviewers questions before
they even ask them. Removing a binding was always going to need
justification, so you needed to have that in the commit message.  In
order to review a DT bindings, having an overview of what the hardware
actually looks like is needed. So that is something which you can add
to the commit messages.

Please take a look at your patches from the perspective of a reviewer,
somebody how knows nothing about this device. What information is
needed to understand the patches?

       Andrew

Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

[Yang Xiwen]

On 2/16/2024 10:10 PM, Andrew Lunn wrote:
>> I've tried accessing MDIO address space and MAC controller address space in
>> u-boot with `md` and `mw` [1]. From the result, i guess the CLK_BUS is the
>> System Bus clock (AHB Bus clock), and the CLK_MAC is the clock shared by
>> both MDIO bus and MAC. The MAC has a internal clock divider to divide the
>> input clock(54MHz in common) to a configurable variable rate.
> In general, sharing a clock is not a problem. The clock API does
> reference counting. So if two consumers enable the clock, it will not
> be disabled until two consumes disable the clock. So it should not be
> an issue for both the MAC and the MDIO driver to consume the clock.
>
> However, the funny PHY reset code is going to be key here. We need to
> understand that in more detail.

I don't know about details too much, either. All the resources i have is 
a brief TRM and the downstream linux kernel code[1], U-Boot code[2]. The 
idea of managing all clocks and resets in MAC driver is taken from 
hix5hd2_gmac.c(in the same directory).

>
> Talking about details, you commit messages need improving. The commit
> message is your chance to answer all the reviewers questions before
> they even ask them. Removing a binding was always going to need
> justification, so you needed to have that in the commit message.  In
> order to review a DT bindings, having an overview of what the hardware
> actually looks like is needed. So that is something which you can add
> to the commit messages.
Thanks for your comment. Also sorry for the inconvenience I brought.
>
> Please take a look at your patches from the perspective of a reviewer,
> somebody how knows nothing about this device. What information is
> needed to understand the patches?
Will rewrite commit logs in next version.
>
>         Andrew

[1]: 
https://github.com/JasonFreeLab/HiSTBLinuxV100R005C00SPC050/blob/fd20f78ab02934e71474dbb1d933c6ec911b01c9/source/kernel/linux-4.4.y/drivers/net/ethernet/hieth/hieth.c#L1281

[2]: 
https://github.com/JasonFreeLab/HiSTBLinuxV100R005C00SPC050/blob/fd20f78ab02934e71474dbb1d933c6ec911b01c9/source/boot/fastboot/drivers/net/hisfv300/sys.c#L50

-- 
Regards,
Yang Xiwen

[PATCH 5/6] net: mdio: hisi-femac: make clock optional

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@foxmail.com>

The clocks are optional.

Signed-off-by: Yang Xiwen <forbidden405@foxmail.com>
---
 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

[PATCH 6/6] dt-bindings: net: hisilicon-femac-mdio: make clock optional

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

The clock is optional.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt b/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
index 23a39a309d17..cd37f43abd45 100644
--- a/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
+++ b/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
@@ -3,6 +3,8 @@ 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.
+
+Optional properties:
 - clocks: A phandle to the reference clock for this device.
 
 - PHY subnode: inherits from phy binding [1]

-- 
2.43.0

Re: [PATCH 6/6] dt-bindings: net: hisilicon-femac-mdio: make clock optional

[Krzysztof Kozlowski]

On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> The clock is optional.

Convert the bindings to DT schema instead, please.

Best regards,
Krzysztof