mbox series

[net-next,v6,00/11] net: hisi-femac: add support for Hi3798MV200, remove unmaintained compatibles

Message ID 20240228-net-v6-0-6d78d3d598c1@outlook.com
Headers show
Series net: hisi-femac: add support for Hi3798MV200, remove unmaintained compatibles | expand

Message

Yang Xiwen via B4 Relay Feb. 28, 2024, 9:02 a.m. UTC 
Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Changes in v6:
- add missing "not" in commit logs (Andrew)
- rework binding changes, split it into several commits (Krzysztof Kozlowski)
- Link to v5: https://lore.kernel.org/r/20240223-net-v5-0-43b22d39c013@outlook.com

Changes in v5:
- hisi-femac-mdio: remove clock completely (Krzysztof Kozlowski)
- dt-bindings: mdio: apply comments from Krzysztof Kozlowski

Changes in v4:
- edit commit log to show why mdio bus clk is optional (Krzysztof Kozlowski)
- add clk_bulk_disable_unprepare() during error path (Maxime Chevallier)
- remove of_node_put() (Simon Horman)
- remove histb-clock.h header in binding example as it's goign to be deprecated.
- rearrange patches so binding comes before driver
- Link to v3: https://lore.kernel.org/r/20240220-net-v3-0-b68e5b75e765@outlook.com

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/20240216-net-v2-0-89bd4b7065c2@outlook.com

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/20240216-net-v1-0-e0ad972cda99@outlook.com

---
Yang Xiwen (11):
      dt-bindings: net: hisilicon-femac-mdio: convert to YAML
      dt-bindings: net: hisilicon,hisi-femac-mdio: remove clocks
      net: mdio: hisi-femac: remove clock
      dt-bindings: net: convert hisi-femac.txt to YAML
      dt-bindings: net: hisi-femac: remove core specific versioning
      dt-bindings: net: hisi-femac: add missing clocks
      dt-bindings: net: hisi-femac: replace deprecated phy-mode with recommended phy-connection-type
      dt-bindings: net: hisi-femac: add mandatory MDIO bus subnode
      dt-bindings: net: hisi-femac: add binding for Hi3798MV200 FEMAC core
      net: hisi_femac: remove unused compatible strings
      net: hisi_femac: add support for hisi_femac core on Hi3798MV200

 .../bindings/net/hisilicon,hisi-femac-mdio.yaml    |  39 +++++++
 .../bindings/net/hisilicon,hisi-femac.yaml         | 120 +++++++++++++++++++++
 .../bindings/net/hisilicon-femac-mdio.txt          |  22 ----
 .../devicetree/bindings/net/hisilicon-femac.txt    |  41 -------
 drivers/net/ethernet/hisilicon/hisi_femac.c        |  77 ++++++++++---
 drivers/net/mdio/mdio-hisi-femac.c                 |  18 +---
 6 files changed, 221 insertions(+), 96 deletions(-)
---
base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b
change-id: 20240216-net-9a208e17c40f

Best regards,

Comments

Yang Xiwen Feb. 28, 2024, 9:05 a.m. UTC | #1 
On 2/28/2024 5:02 PM, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> 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.

Will rewrite commit log in next version

> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  drivers/net/ethernet/hisilicon/hisi_femac.c | 74 +++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
> index 3c5c095dad05..be2bfd06c8d2 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,17 +816,16 @@ 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);
> -		goto out_free_netdev;
> +		dev_err(dev, "failed to enable clk bulk: %d\n", ret);
> +		goto out_disable_clk;
>  	}
>  
>  	priv->mac_rst = devm_reset_control_get(dev, "mac");
> @@ -830,11 +848,29 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
>  		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_disable_clk;
> +			}
> +			mdio_registered = true;
> +			break;
> +		}
> +	}
> +
> +	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 +921,10 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
>  out_disconnect_phy:
>  	netif_napi_del(&priv->napi);
>  	phy_disconnect(phy);
> +out_unregister_mdio_bus:
> +	platform_device_unregister(priv->mdio_pdev);
>  out_disable_clk:
> -	clk_disable_unprepare(priv->clk);
> +	clk_bulk_disable_unprepare(CLK_NUM, priv->clks);
>  out_free_netdev:
>  	free_netdev(ndev);
>  
> @@ -902,7 +940,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 +958,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 +967,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);
>  
> @@ -947,6 +992,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>  static const struct of_device_id hisi_femac_match[] = {
>  	{.compatible = "hisilicon,hisi-femac",},
>  	{.compatible = "hisilicon,hi3516cv300-femac",},
> +	{.compatible = "hisilicon,hi3798mv200-femac",},
>  	{},
>  };
>  
>
Krzysztof Kozlowski Feb. 29, 2024, 8:07 a.m. UTC | #2 
On 28/02/2024 10:02, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> It's hard to get the version number for each FEMAC core and it's unknown
> how the version can be used. Remove them until it's really needed.
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  drivers/net/ethernet/hisilicon/hisi_femac.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
> index 2406263c9dd3..3c5c095dad05 100644
> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c
> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
> @@ -945,8 +945,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,hisi-femac",},

Drop and just use SoC compatibles.

>  	{.compatible = "hisilicon,hi3516cv300-femac",},



Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 29, 2024, 8:10 a.m. UTC | #3 
On 28/02/2024 10:02, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> Register the sub MDIO bus if it is found. Also implement the internal
> PHY reset procedure as needed.
> 


>  
> @@ -947,6 +992,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>  static const struct of_device_id hisi_femac_match[] = {
>  	{.compatible = "hisilicon,hisi-femac",},
>  	{.compatible = "hisilicon,hi3516cv300-femac",},
> +	{.compatible = "hisilicon,hi3798mv200-femac",},

No, it does not make any sense. Why do you keep growing this table?

Best regards,
Krzysztof
Yang Xiwen Feb. 29, 2024, 8:14 a.m. UTC | #4 
On 2/29/2024 4:10 PM, Krzysztof Kozlowski wrote:
> On 28/02/2024 10:02, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> Register the sub MDIO bus if it is found. Also implement the internal
>> PHY reset procedure as needed.
>>
> 
> 
>>  
>> @@ -947,6 +992,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>>  static const struct of_device_id hisi_femac_match[] = {
>>  	{.compatible = "hisilicon,hisi-femac",},
>>  	{.compatible = "hisilicon,hi3516cv300-femac",},
>> +	{.compatible = "hisilicon,hi3798mv200-femac",},
> 
> No, it does not make any sense. Why do you keep growing this table?

So do I only have to keep the generic compatible "hisilicon,hisi-femac"
or the two SoC compatibles?

> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Feb. 29, 2024, 10:17 a.m. UTC | #5 
On 29/02/2024 09:14, Yang Xiwen wrote:
> On 2/29/2024 4:10 PM, Krzysztof Kozlowski wrote:
>> On 28/02/2024 10:02, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> Register the sub MDIO bus if it is found. Also implement the internal
>>> PHY reset procedure as needed.
>>>
>>
>>
>>>  
>>> @@ -947,6 +992,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>>>  static const struct of_device_id hisi_femac_match[] = {
>>>  	{.compatible = "hisilicon,hisi-femac",},
>>>  	{.compatible = "hisilicon,hi3516cv300-femac",},
>>> +	{.compatible = "hisilicon,hi3798mv200-femac",},
>>
>> No, it does not make any sense. Why do you keep growing this table?
> 
> So do I only have to keep the generic compatible "hisilicon,hisi-femac"
> or the two SoC compatibles?

You only need to keep here fallback compatible.

Best regards,
Krzysztof
Yang Xiwen March 5, 2024, 7:48 a.m. UTC | #6 
On 2/29/2024 4:07 PM, Krzysztof Kozlowski wrote:
> On 28/02/2024 10:02, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> It's hard to get the version number for each FEMAC core and it's unknown
>> how the version can be used. Remove them until it's really needed.
>>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hisi_femac.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
>> index 2406263c9dd3..3c5c095dad05 100644
>> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c
>> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
>> @@ -945,8 +945,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,hisi-femac",},
> Drop and just use SoC compatibles.


Will be fixed in v8.


>
>>   	{.compatible = "hisilicon,hi3516cv300-femac",},
>
>
> Best regards,
> Krzysztof
>