[PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[Marek Vasut]

Both socfpga_dwmac_parse_data() in dwmac-socfpga.c and stmmac_dvr_probe()
in stmmac_main.c functions call devm_reset_control_get() to register an
reset controller for the stmmac. This results in an attempt to register
two reset controllers for the same non-shared reset line.

The first attempt to register the reset controller works fine. The second
attempt fails with warning from the reset controller core, see below.
The warning is produced because the reset line is non-shared and thus
it is allowed to have only up-to one reset controller associated with
that reset line, not two or more.

The solution has multiple parts. First, the original socfpga_dwmac_init()
is tweaked to use reset controller pointer from the stmmac_priv (private
data of the stmmac core) instead of the local instance, which was used
before. The local re-registration of the reset controller is removed.

Next, the socfpga_dwmac_init() is moved after stmmac_dvr_probe() in the
probe function. This order is legal according to Altera and it makes the
code much easier, since there is no need to temporarily register and
unregister the reset controller ; the reset controller is already registered
by the stmmac_dvr_probe().

Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
since the functionality is already performed by the stmmac core.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x218/0x270
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc4-next-20160419-00015-gabb2477-dirty #4
Hardware name: Altera SOCFPGA
[<c010f290>] (unwind_backtrace) from [<c010b82c>] (show_stack+0x10/0x14)
[<c010b82c>] (show_stack) from [<c0373da4>] (dump_stack+0x94/0xa8)
[<c0373da4>] (dump_stack) from [<c011bcc0>] (__warn+0xec/0x104)
[<c011bcc0>] (__warn) from [<c011bd88>] (warn_slowpath_null+0x20/0x28)
[<c011bd88>] (warn_slowpath_null) from [<c03a6eb4>] (__of_reset_control_get+0x218/0x270)
[<c03a6eb4>] (__of_reset_control_get) from [<c03a701c>] (__devm_reset_control_get+0x54/0x90)
[<c03a701c>] (__devm_reset_control_get) from [<c041fa30>] (stmmac_dvr_probe+0x1b4/0x8e8)
[<c041fa30>] (stmmac_dvr_probe) from [<c04298c8>] (socfpga_dwmac_probe+0x1b8/0x28c)
[<c04298c8>] (socfpga_dwmac_probe) from [<c03d6ffc>] (platform_drv_probe+0x4c/0xb0)
[<c03d6ffc>] (platform_drv_probe) from [<c03d54ec>] (driver_probe_device+0x224/0x2bc)
[<c03d54ec>] (driver_probe_device) from [<c03d5630>] (__driver_attach+0xac/0xb0)
[<c03d5630>] (__driver_attach) from [<c03d382c>] (bus_for_each_dev+0x6c/0xa0)
[<c03d382c>] (bus_for_each_dev) from [<c03d4ad4>] (bus_add_driver+0x1a4/0x21c)
[<c03d4ad4>] (bus_add_driver) from [<c03d60ac>] (driver_register+0x78/0xf8)
[<c03d60ac>] (driver_register) from [<c0101760>] (do_one_initcall+0x40/0x170)
[<c0101760>] (do_one_initcall) from [<c0800e38>] (kernel_init_freeable+0x1dc/0x27c)
[<c0800e38>] (kernel_init_freeable) from [<c05d1bd4>] (kernel_init+0x8/0x114)
[<c05d1bd4>] (kernel_init) from [<c01076f8>] (ret_from_fork+0x14/0x3c)
---[ end trace 059d2fbe87608fa9 ]---

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Matthew Gerlach <mgerlach@opensource.altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: David S. Miller <davem@davemloft.net>
---
V2: Add missing stmmac_rst = NULL; into socfpga_dwmac_init_probe()
V3: Greatly simplify the code by moving socfpga_dwmac_init() after
    stmmac_dvr_probe(), which is legal. This removes the need for
    temporary registration of the reset controller, which is super.
---
 .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c    | 50 +++++++---------------
 1 file changed, 16 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 76d671e..784eb53 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -49,7 +49,6 @@ struct socfpga_dwmac {
 	u32	reg_shift;
 	struct	device *dev;
 	struct regmap *sys_mgr_base_addr;
-	struct reset_control *stmmac_rst;
 	void __iomem *splitter_base;
 	bool f2h_ptp_ref_clk;
 };
@@ -92,15 +91,6 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
 	struct device_node *np_splitter;
 	struct resource res_splitter;
 
-	dwmac->stmmac_rst = devm_reset_control_get(dev,
-						  STMMAC_RESOURCE_NAME);
-	if (IS_ERR(dwmac->stmmac_rst)) {
-		dev_info(dev, "Could not get reset control!\n");
-		if (PTR_ERR(dwmac->stmmac_rst) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		dwmac->stmmac_rst = NULL;
-	}
-
 	dwmac->interface = of_get_phy_mode(np);
 
 	sys_mgr_base_addr = syscon_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon");
@@ -194,30 +184,23 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac)
 	return 0;
 }
 
-static void socfpga_dwmac_exit(struct platform_device *pdev, void *priv)
-{
-	struct socfpga_dwmac	*dwmac = priv;
-
-	/* On socfpga platform exit, assert and hold reset to the
-	 * enet controller - the default state after a hard reset.
-	 */
-	if (dwmac->stmmac_rst)
-		reset_control_assert(dwmac->stmmac_rst);
-}
-
 static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
 {
-	struct socfpga_dwmac	*dwmac = priv;
+	struct socfpga_dwmac *dwmac = priv;
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct stmmac_priv *stpriv = NULL;
 	int ret = 0;
 
-	if (ndev)
-		stpriv = netdev_priv(ndev);
+	if (!ndev)
+		return -EINVAL;
+
+	stpriv = netdev_priv(ndev);
+	if (!stpriv)
+		return -EINVAL;
 
 	/* Assert reset to the enet controller before changing the phy mode */
-	if (dwmac->stmmac_rst)
-		reset_control_assert(dwmac->stmmac_rst);
+	if (stpriv->stmmac_rst)
+		reset_control_assert(stpriv->stmmac_rst);
 
 	/* Setup the phy mode in the system manager registers according to
 	 * devicetree configuration
@@ -227,8 +210,8 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
 	/* Deassert reset for the phy configuration to be sampled by
 	 * the enet controller, and operation to start in requested mode
 	 */
-	if (dwmac->stmmac_rst)
-		reset_control_deassert(dwmac->stmmac_rst);
+	if (stpriv->stmmac_rst)
+		reset_control_deassert(stpriv->stmmac_rst);
 
 	/* Before the enet controller is suspended, the phy is suspended.
 	 * This causes the phy clock to be gated. The enet controller is
@@ -245,7 +228,7 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
 	 * control register 0, and can be modified by the phy driver
 	 * framework.
 	 */
-	if (stpriv && stpriv->phydev)
+	if (stpriv->phydev)
 		phy_resume(stpriv->phydev);
 
 	return ret;
@@ -279,14 +262,13 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 
 	plat_dat->bsp_priv = dwmac;
 	plat_dat->init = socfpga_dwmac_init;
-	plat_dat->exit = socfpga_dwmac_exit;
 	plat_dat->fix_mac_speed = socfpga_dwmac_fix_mac_speed;
 
-	ret = socfpga_dwmac_init(pdev, plat_dat->bsp_priv);
-	if (ret)
-		return ret;
+	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	if (!ret)
+		ret = socfpga_dwmac_init(pdev, dwmac);
 
-	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	return ret;
 }
 
 static const struct of_device_id socfpga_dwmac_match[] = {
-- 
2.7.0

Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[Dinh Nguyen]

On 04/21/2016 07:11 AM, Marek Vasut wrote:
> Both socfpga_dwmac_parse_data() in dwmac-socfpga.c and stmmac_dvr_probe()
> in stmmac_main.c functions call devm_reset_control_get() to register an
> reset controller for the stmmac. This results in an attempt to register
> two reset controllers for the same non-shared reset line.
> 
> The first attempt to register the reset controller works fine. The second
> attempt fails with warning from the reset controller core, see below.
> The warning is produced because the reset line is non-shared and thus
> it is allowed to have only up-to one reset controller associated with
> that reset line, not two or more.
> 
> The solution has multiple parts. First, the original socfpga_dwmac_init()
> is tweaked to use reset controller pointer from the stmmac_priv (private
> data of the stmmac core) instead of the local instance, which was used
> before. The local re-registration of the reset controller is removed.
> 
> Next, the socfpga_dwmac_init() is moved after stmmac_dvr_probe() in the
> probe function. This order is legal according to Altera and it makes the
> code much easier, since there is no need to temporarily register and
> unregister the reset controller ; the reset controller is already registered
> by the stmmac_dvr_probe().
> 
> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
> since the functionality is already performed by the stmmac core.
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x218/0x270
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc4-next-20160419-00015-gabb2477-dirty #4
> Hardware name: Altera SOCFPGA
> [<c010f290>] (unwind_backtrace) from [<c010b82c>] (show_stack+0x10/0x14)
> [<c010b82c>] (show_stack) from [<c0373da4>] (dump_stack+0x94/0xa8)
> [<c0373da4>] (dump_stack) from [<c011bcc0>] (__warn+0xec/0x104)
> [<c011bcc0>] (__warn) from [<c011bd88>] (warn_slowpath_null+0x20/0x28)
> [<c011bd88>] (warn_slowpath_null) from [<c03a6eb4>] (__of_reset_control_get+0x218/0x270)
> [<c03a6eb4>] (__of_reset_control_get) from [<c03a701c>] (__devm_reset_control_get+0x54/0x90)
> [<c03a701c>] (__devm_reset_control_get) from [<c041fa30>] (stmmac_dvr_probe+0x1b4/0x8e8)
> [<c041fa30>] (stmmac_dvr_probe) from [<c04298c8>] (socfpga_dwmac_probe+0x1b8/0x28c)
> [<c04298c8>] (socfpga_dwmac_probe) from [<c03d6ffc>] (platform_drv_probe+0x4c/0xb0)
> [<c03d6ffc>] (platform_drv_probe) from [<c03d54ec>] (driver_probe_device+0x224/0x2bc)
> [<c03d54ec>] (driver_probe_device) from [<c03d5630>] (__driver_attach+0xac/0xb0)
> [<c03d5630>] (__driver_attach) from [<c03d382c>] (bus_for_each_dev+0x6c/0xa0)
> [<c03d382c>] (bus_for_each_dev) from [<c03d4ad4>] (bus_add_driver+0x1a4/0x21c)
> [<c03d4ad4>] (bus_add_driver) from [<c03d60ac>] (driver_register+0x78/0xf8)
> [<c03d60ac>] (driver_register) from [<c0101760>] (do_one_initcall+0x40/0x170)
> [<c0101760>] (do_one_initcall) from [<c0800e38>] (kernel_init_freeable+0x1dc/0x27c)
> [<c0800e38>] (kernel_init_freeable) from [<c05d1bd4>] (kernel_init+0x8/0x114)
> [<c05d1bd4>] (kernel_init) from [<c01076f8>] (ret_from_fork+0x14/0x3c)
> ---[ end trace 059d2fbe87608fa9 ]---
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Matthew Gerlach <mgerlach@opensource.altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: David S. Miller <davem@davemloft.net>
> ---
> V2: Add missing stmmac_rst = NULL; into socfpga_dwmac_init_probe()
> V3: Greatly simplify the code by moving socfpga_dwmac_init() after
>     stmmac_dvr_probe(), which is legal. This removes the need for
>     temporary registration of the reset controller, which is super.
>

Tested-by: Dinh Nguyen <dinguyen@opensource.altera.com>

Thanks,
Dinh

Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[David Miller]

From: Marek Vasut <marex@denx.de>
Date: Thu, 21 Apr 2016 14:11:50 +0200

> Both socfpga_dwmac_parse_data() in dwmac-socfpga.c and stmmac_dvr_probe()
> in stmmac_main.c functions call devm_reset_control_get() to register an
> reset controller for the stmmac. This results in an attempt to register
> two reset controllers for the same non-shared reset line.
> 
> The first attempt to register the reset controller works fine. The second
> attempt fails with warning from the reset controller core, see below.
> The warning is produced because the reset line is non-shared and thus
> it is allowed to have only up-to one reset controller associated with
> that reset line, not two or more.
> 
> The solution has multiple parts. First, the original socfpga_dwmac_init()
> is tweaked to use reset controller pointer from the stmmac_priv (private
> data of the stmmac core) instead of the local instance, which was used
> before. The local re-registration of the reset controller is removed.
> 
> Next, the socfpga_dwmac_init() is moved after stmmac_dvr_probe() in the
> probe function. This order is legal according to Altera and it makes the
> code much easier, since there is no need to temporarily register and
> unregister the reset controller ; the reset controller is already registered
> by the stmmac_dvr_probe().
> 
> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
> since the functionality is already performed by the stmmac core.
 ...
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Matthew Gerlach <mgerlach@opensource.altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: David S. Miller <davem@davemloft.net>

Applied, thanks.

Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[Joachim Eastwood]

Hi Marek,

On 21 April 2016 at 14:11, Marek Vasut <marex@denx.de> wrote:
> Both socfpga_dwmac_parse_data() in dwmac-socfpga.c and stmmac_dvr_probe()
> in stmmac_main.c functions call devm_reset_control_get() to register an
> reset controller for the stmmac. This results in an attempt to register
> two reset controllers for the same non-shared reset line.
>
> The first attempt to register the reset controller works fine. The second
> attempt fails with warning from the reset controller core, see below.
> The warning is produced because the reset line is non-shared and thus
> it is allowed to have only up-to one reset controller associated with
> that reset line, not two or more.
>
> The solution has multiple parts. First, the original socfpga_dwmac_init()
> is tweaked to use reset controller pointer from the stmmac_priv (private
> data of the stmmac core) instead of the local instance, which was used
> before. The local re-registration of the reset controller is removed.
>
> Next, the socfpga_dwmac_init() is moved after stmmac_dvr_probe() in the
> probe function. This order is legal according to Altera and it makes the
> code much easier, since there is no need to temporarily register and
> unregister the reset controller ; the reset controller is already registered
> by the stmmac_dvr_probe().
>
> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
> since the functionality is already performed by the stmmac core.

I am trying to rebase my changes on top of your two patches and
noticed a couple of things.

>  static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>  {
> -       struct socfpga_dwmac    *dwmac = priv;
> +       struct socfpga_dwmac *dwmac = priv;
>         struct net_device *ndev = platform_get_drvdata(pdev);
>         struct stmmac_priv *stpriv = NULL;
>         int ret = 0;
>
> -       if (ndev)
> -               stpriv = netdev_priv(ndev);
> +       if (!ndev)
> +               return -EINVAL;

ndev can never be NULL here. socfpga_dwmac_init() is only called if
stmmac_dvr_probe() succeeds or we are running the resume callback. So
I don't see how this could ever be NULL.


> +
> +       stpriv = netdev_priv(ndev);

It's not really nice to access 'stmmac_priv' as it should be private
to the core driver, but I don't see any other good solution right now.


> +       if (!stpriv)
> +               return -EINVAL;
>
>         /* Assert reset to the enet controller before changing the phy mode */
> -       if (dwmac->stmmac_rst)
> -               reset_control_assert(dwmac->stmmac_rst);
> +       if (stpriv->stmmac_rst)
> +               reset_control_assert(stpriv->stmmac_rst);
>
>         /* Setup the phy mode in the system manager registers according to
>          * devicetree configuration
> @@ -227,8 +210,8 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>         /* Deassert reset for the phy configuration to be sampled by
>          * the enet controller, and operation to start in requested mode
>          */
> -       if (dwmac->stmmac_rst)
> -               reset_control_deassert(dwmac->stmmac_rst);
> +       if (stpriv->stmmac_rst)
> +               reset_control_deassert(stpriv->stmmac_rst);
>
>         /* Before the enet controller is suspended, the phy is suspended.
>          * This causes the phy clock to be gated. The enet controller is
> @@ -245,7 +228,7 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>          * control register 0, and can be modified by the phy driver
>          * framework.
>          */
> -       if (stpriv && stpriv->phydev)
> +       if (stpriv->phydev)
>                 phy_resume(stpriv->phydev);

Before this change phy_resume() was only called during driver resume
when , but your patches cause phy_resume() to called at probe time as
well. Is this okey?


regards,
Joachim Eastwood

Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[Marek Vasut]

On 04/25/2016 08:11 PM, Joachim Eastwood wrote:
> Hi Marek,

Hi!

> On 21 April 2016 at 14:11, Marek Vasut <marex@denx.de> wrote:
>> Both socfpga_dwmac_parse_data() in dwmac-socfpga.c and stmmac_dvr_probe()
>> in stmmac_main.c functions call devm_reset_control_get() to register an
>> reset controller for the stmmac. This results in an attempt to register
>> two reset controllers for the same non-shared reset line.
>>
>> The first attempt to register the reset controller works fine. The second
>> attempt fails with warning from the reset controller core, see below.
>> The warning is produced because the reset line is non-shared and thus
>> it is allowed to have only up-to one reset controller associated with
>> that reset line, not two or more.
>>
>> The solution has multiple parts. First, the original socfpga_dwmac_init()
>> is tweaked to use reset controller pointer from the stmmac_priv (private
>> data of the stmmac core) instead of the local instance, which was used
>> before. The local re-registration of the reset controller is removed.
>>
>> Next, the socfpga_dwmac_init() is moved after stmmac_dvr_probe() in the
>> probe function. This order is legal according to Altera and it makes the
>> code much easier, since there is no need to temporarily register and
>> unregister the reset controller ; the reset controller is already registered
>> by the stmmac_dvr_probe().
>>
>> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
>> since the functionality is already performed by the stmmac core.
> 
> I am trying to rebase my changes on top of your two patches and
> noticed a couple of things.
> 
>>  static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>  {
>> -       struct socfpga_dwmac    *dwmac = priv;
>> +       struct socfpga_dwmac *dwmac = priv;
>>         struct net_device *ndev = platform_get_drvdata(pdev);
>>         struct stmmac_priv *stpriv = NULL;
>>         int ret = 0;
>>
>> -       if (ndev)
>> -               stpriv = netdev_priv(ndev);
>> +       if (!ndev)
>> +               return -EINVAL;
> 
> ndev can never be NULL here. socfpga_dwmac_init() is only called if
> stmmac_dvr_probe() succeeds or we are running the resume callback. So
> I don't see how this could ever be NULL.

That's a good point, this check can indeed be removed. While you're at
the patching, can you remove this one ?

>> +
>> +       stpriv = netdev_priv(ndev);
> 
> It's not really nice to access 'stmmac_priv' as it should be private
> to the core driver, but I don't see any other good solution right now.

I guess some stmmac_reset_assert() wrapper would be nicer, yes. What do
you think ?

>> +       if (!stpriv)
>> +               return -EINVAL;
>>
>>         /* Assert reset to the enet controller before changing the phy mode */
>> -       if (dwmac->stmmac_rst)
>> -               reset_control_assert(dwmac->stmmac_rst);
>> +       if (stpriv->stmmac_rst)
>> +               reset_control_assert(stpriv->stmmac_rst);
>>
>>         /* Setup the phy mode in the system manager registers according to
>>          * devicetree configuration
>> @@ -227,8 +210,8 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>         /* Deassert reset for the phy configuration to be sampled by
>>          * the enet controller, and operation to start in requested mode
>>          */
>> -       if (dwmac->stmmac_rst)
>> -               reset_control_deassert(dwmac->stmmac_rst);
>> +       if (stpriv->stmmac_rst)
>> +               reset_control_deassert(stpriv->stmmac_rst);
>>
>>         /* Before the enet controller is suspended, the phy is suspended.
>>          * This causes the phy clock to be gated. The enet controller is
>> @@ -245,7 +228,7 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>          * control register 0, and can be modified by the phy driver
>>          * framework.
>>          */
>> -       if (stpriv && stpriv->phydev)
>> +       if (stpriv->phydev)
>>                 phy_resume(stpriv->phydev);
> 
> Before this change phy_resume() was only called during driver resume
> when , but your patches cause phy_resume() to called at probe time as
> well. Is this okey?

I _hope_ it's OK. The cryptic comment above is not very helpful in this
aspect. Dinh ? :)

> regards,
> Joachim Eastwood
> 

btw I wish you reviewed my patch a bit earlier to catch these bits.

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[Joachim Eastwood]

On 26 April 2016 at 00:55, Marek Vasut <marex@denx.de> wrote:
> On 04/25/2016 08:11 PM, Joachim Eastwood wrote:
>> On 21 April 2016 at 14:11, Marek Vasut <marex@denx.de> wrote:
>>>
>>> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
>>> since the functionality is already performed by the stmmac core.
>>
>> I am trying to rebase my changes on top of your two patches and
>> noticed a couple of things.
>>
>>>  static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>  {
>>> -       struct socfpga_dwmac    *dwmac = priv;
>>> +       struct socfpga_dwmac *dwmac = priv;
>>>         struct net_device *ndev = platform_get_drvdata(pdev);
>>>         struct stmmac_priv *stpriv = NULL;
>>>         int ret = 0;
>>>
>>> -       if (ndev)
>>> -               stpriv = netdev_priv(ndev);
>>> +       if (!ndev)
>>> +               return -EINVAL;
>>
>> ndev can never be NULL here. socfpga_dwmac_init() is only called if
>> stmmac_dvr_probe() succeeds or we are running the resume callback. So
>> I don't see how this could ever be NULL.
>
> That's a good point, this check can indeed be removed. While you're at
> the patching, can you remove this one ?

Yes, my patch will refactor the init() function so this will go away.


>>> +
>>> +       stpriv = netdev_priv(ndev);
>>
>> It's not really nice to access 'stmmac_priv' as it should be private
>> to the core driver, but I don't see any other good solution right now.
>
> I guess some stmmac_reset_assert() wrapper would be nicer, yes. What do
> you think ?
>
>>> +       if (!stpriv)
>>> +               return -EINVAL;
>>>
>>>         /* Assert reset to the enet controller before changing the phy mode */
>>> -       if (dwmac->stmmac_rst)
>>> -               reset_control_assert(dwmac->stmmac_rst);
>>> +       if (stpriv->stmmac_rst)
>>> +               reset_control_assert(stpriv->stmmac_rst);
>>>
>>>         /* Setup the phy mode in the system manager registers according to
>>>          * devicetree configuration
>>> @@ -227,8 +210,8 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>         /* Deassert reset for the phy configuration to be sampled by
>>>          * the enet controller, and operation to start in requested mode
>>>          */
>>> -       if (dwmac->stmmac_rst)
>>> -               reset_control_deassert(dwmac->stmmac_rst);
>>> +       if (stpriv->stmmac_rst)
>>> +               reset_control_deassert(stpriv->stmmac_rst);
>>>
>>>         /* Before the enet controller is suspended, the phy is suspended.
>>>          * This causes the phy clock to be gated. The enet controller is
>>> @@ -245,7 +228,7 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>          * control register 0, and can be modified by the phy driver
>>>          * framework.
>>>          */
>>> -       if (stpriv && stpriv->phydev)
>>> +       if (stpriv->phydev)
>>>                 phy_resume(stpriv->phydev);
>>
>> Before this change phy_resume() was only called during driver resume
>> when , but your patches cause phy_resume() to called at probe time as
>> well. Is this okey?
>
> I _hope_ it's OK. The cryptic comment above is not very helpful in this
> aspect. Dinh ? :)

My patches will move phy_resume() to the resume callback so it
preserves the previous behavior. But if someone knows more about this
that would be useful.


> btw I wish you reviewed my patch a bit earlier to catch these bits.

Sorry, about that. I have been really busy with other things lately.


My patches based on next from Friday can be found here now:
https://github.com/manabian/linux-lpc/tree/net-socfpga-dwmac-on-next

I had to add your latest patch as well since the next version I used
didn't have it. I'll post the patches on netdev later today or
tomorrow.



regards,
Joachim Eastwood

Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[Marek Vasut]

On 04/26/2016 02:26 PM, Joachim Eastwood wrote:
> On 26 April 2016 at 00:55, Marek Vasut <marex@denx.de> wrote:
>> On 04/25/2016 08:11 PM, Joachim Eastwood wrote:
>>> On 21 April 2016 at 14:11, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
>>>> since the functionality is already performed by the stmmac core.
>>>
>>> I am trying to rebase my changes on top of your two patches and
>>> noticed a couple of things.
>>>
>>>>  static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>  {
>>>> -       struct socfpga_dwmac    *dwmac = priv;
>>>> +       struct socfpga_dwmac *dwmac = priv;
>>>>         struct net_device *ndev = platform_get_drvdata(pdev);
>>>>         struct stmmac_priv *stpriv = NULL;
>>>>         int ret = 0;
>>>>
>>>> -       if (ndev)
>>>> -               stpriv = netdev_priv(ndev);
>>>> +       if (!ndev)
>>>> +               return -EINVAL;
>>>
>>> ndev can never be NULL here. socfpga_dwmac_init() is only called if
>>> stmmac_dvr_probe() succeeds or we are running the resume callback. So
>>> I don't see how this could ever be NULL.
>>
>> That's a good point, this check can indeed be removed. While you're at
>> the patching, can you remove this one ?
> 
> Yes, my patch will refactor the init() function so this will go away.

Thanks!

>>>> +
>>>> +       stpriv = netdev_priv(ndev);
>>>
>>> It's not really nice to access 'stmmac_priv' as it should be private
>>> to the core driver, but I don't see any other good solution right now.
>>
>> I guess some stmmac_reset_assert() wrapper would be nicer, yes. What do
>> you think ?
>>
>>>> +       if (!stpriv)
>>>> +               return -EINVAL;
>>>>
>>>>         /* Assert reset to the enet controller before changing the phy mode */
>>>> -       if (dwmac->stmmac_rst)
>>>> -               reset_control_assert(dwmac->stmmac_rst);
>>>> +       if (stpriv->stmmac_rst)
>>>> +               reset_control_assert(stpriv->stmmac_rst);
>>>>
>>>>         /* Setup the phy mode in the system manager registers according to
>>>>          * devicetree configuration
>>>> @@ -227,8 +210,8 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>         /* Deassert reset for the phy configuration to be sampled by
>>>>          * the enet controller, and operation to start in requested mode
>>>>          */
>>>> -       if (dwmac->stmmac_rst)
>>>> -               reset_control_deassert(dwmac->stmmac_rst);
>>>> +       if (stpriv->stmmac_rst)
>>>> +               reset_control_deassert(stpriv->stmmac_rst);
>>>>
>>>>         /* Before the enet controller is suspended, the phy is suspended.
>>>>          * This causes the phy clock to be gated. The enet controller is
>>>> @@ -245,7 +228,7 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>          * control register 0, and can be modified by the phy driver
>>>>          * framework.
>>>>          */
>>>> -       if (stpriv && stpriv->phydev)
>>>> +       if (stpriv->phydev)
>>>>                 phy_resume(stpriv->phydev);
>>>
>>> Before this change phy_resume() was only called during driver resume
>>> when , but your patches cause phy_resume() to called at probe time as
>>> well. Is this okey?
>>
>> I _hope_ it's OK. The cryptic comment above is not very helpful in this
>> aspect. Dinh ? :)
> 
> My patches will move phy_resume() to the resume callback so it
> preserves the previous behavior. But if someone knows more about this
> that would be useful.
> 
> 
>> btw I wish you reviewed my patch a bit earlier to catch these bits.
> 
> Sorry, about that. I have been really busy with other things lately.

Oh I'm real happy someone is doing the refactoring :) I appreciate your
work, sorry if that was unclear.

> My patches based on next from Friday can be found here now:
> https://github.com/manabian/linux-lpc/tree/net-socfpga-dwmac-on-next
> 
> I had to add your latest patch as well since the next version I used
> didn't have it. I'll post the patches on netdev later today or
> tomorrow.

Looks like next wasn't synced for a few days, yeah.

You can add my:

On SoCFPGA Cyclone V SoC (DENX MCVEVK):
Tested-by: Marek Vasut <marex@denx.de>

to those patches

> regards,
> Joachim Eastwood
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[Joachim Eastwood]

On 26 April 2016 at 14:47, Marek Vasut <marex@denx.de> wrote:
> On 04/26/2016 02:26 PM, Joachim Eastwood wrote:
>> On 26 April 2016 at 00:55, Marek Vasut <marex@denx.de> wrote:
>>> On 04/25/2016 08:11 PM, Joachim Eastwood wrote:
>>>> On 21 April 2016 at 14:11, Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
>>>>> since the functionality is already performed by the stmmac core.
>>>>
>>>> I am trying to rebase my changes on top of your two patches and
>>>> noticed a couple of things.
>>>>
>>>>>  static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>>  {
>>>>> -       struct socfpga_dwmac    *dwmac = priv;
>>>>> +       struct socfpga_dwmac *dwmac = priv;
>>>>>         struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>         struct stmmac_priv *stpriv = NULL;
>>>>>         int ret = 0;
>>>>>
>>>>> -       if (ndev)
>>>>> -               stpriv = netdev_priv(ndev);
>>>>> +       if (!ndev)
>>>>> +               return -EINVAL;
>>>>
>>>> ndev can never be NULL here. socfpga_dwmac_init() is only called if
>>>> stmmac_dvr_probe() succeeds or we are running the resume callback. So
>>>> I don't see how this could ever be NULL.
>>>
>>> That's a good point, this check can indeed be removed. While you're at
>>> the patching, can you remove this one ?
>>
>> Yes, my patch will refactor the init() function so this will go away.
>
> Thanks!
>
>>>>> +
>>>>> +       stpriv = netdev_priv(ndev);
>>>>
>>>> It's not really nice to access 'stmmac_priv' as it should be private
>>>> to the core driver, but I don't see any other good solution right now.
>>>
>>> I guess some stmmac_reset_assert() wrapper would be nicer, yes. What do
>>> you think ?
>>>
>>>>> +       if (!stpriv)
>>>>> +               return -EINVAL;
>>>>>
>>>>>         /* Assert reset to the enet controller before changing the phy mode */
>>>>> -       if (dwmac->stmmac_rst)
>>>>> -               reset_control_assert(dwmac->stmmac_rst);
>>>>> +       if (stpriv->stmmac_rst)
>>>>> +               reset_control_assert(stpriv->stmmac_rst);
>>>>>
>>>>>         /* Setup the phy mode in the system manager registers according to
>>>>>          * devicetree configuration
>>>>> @@ -227,8 +210,8 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>>         /* Deassert reset for the phy configuration to be sampled by
>>>>>          * the enet controller, and operation to start in requested mode
>>>>>          */
>>>>> -       if (dwmac->stmmac_rst)
>>>>> -               reset_control_deassert(dwmac->stmmac_rst);
>>>>> +       if (stpriv->stmmac_rst)
>>>>> +               reset_control_deassert(stpriv->stmmac_rst);
>>>>>
>>>>>         /* Before the enet controller is suspended, the phy is suspended.
>>>>>          * This causes the phy clock to be gated. The enet controller is
>>>>> @@ -245,7 +228,7 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>>          * control register 0, and can be modified by the phy driver
>>>>>          * framework.
>>>>>          */
>>>>> -       if (stpriv && stpriv->phydev)
>>>>> +       if (stpriv->phydev)
>>>>>                 phy_resume(stpriv->phydev);
>>>>
>>>> Before this change phy_resume() was only called during driver resume
>>>> when , but your patches cause phy_resume() to called at probe time as
>>>> well. Is this okey?
>>>
>>> I _hope_ it's OK. The cryptic comment above is not very helpful in this
>>> aspect. Dinh ? :)
>>
>> My patches will move phy_resume() to the resume callback so it
>> preserves the previous behavior. But if someone knows more about this
>> that would be useful.
>>
>>
>>> btw I wish you reviewed my patch a bit earlier to catch these bits.
>>
>> Sorry, about that. I have been really busy with other things lately.
>
> Oh I'm real happy someone is doing the refactoring :) I appreciate your
> work, sorry if that was unclear.
>
>> My patches based on next from Friday can be found here now:
>> https://github.com/manabian/linux-lpc/tree/net-socfpga-dwmac-on-next
>>
>> I had to add your latest patch as well since the next version I used
>> didn't have it. I'll post the patches on netdev later today or
>> tomorrow.
>
> Looks like next wasn't synced for a few days, yeah.
>
> You can add my:
>
> On SoCFPGA Cyclone V SoC (DENX MCVEVK):
> Tested-by: Marek Vasut <marex@denx.de>
>
> to those patches

Excellent. Thanks Marek.

btw, did you also test suspend/resume?


regards,
Joachim Eastwood

Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

[Marek Vasut]

On 04/26/2016 11:22 PM, Joachim Eastwood wrote:
> On 26 April 2016 at 14:47, Marek Vasut <marex@denx.de> wrote:
>> On 04/26/2016 02:26 PM, Joachim Eastwood wrote:
>>> On 26 April 2016 at 00:55, Marek Vasut <marex@denx.de> wrote:
>>>> On 04/25/2016 08:11 PM, Joachim Eastwood wrote:
>>>>> On 21 April 2016 at 14:11, Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
>>>>>> since the functionality is already performed by the stmmac core.
>>>>>
>>>>> I am trying to rebase my changes on top of your two patches and
>>>>> noticed a couple of things.
>>>>>
>>>>>>  static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>>>  {
>>>>>> -       struct socfpga_dwmac    *dwmac = priv;
>>>>>> +       struct socfpga_dwmac *dwmac = priv;
>>>>>>         struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>>         struct stmmac_priv *stpriv = NULL;
>>>>>>         int ret = 0;
>>>>>>
>>>>>> -       if (ndev)
>>>>>> -               stpriv = netdev_priv(ndev);
>>>>>> +       if (!ndev)
>>>>>> +               return -EINVAL;
>>>>>
>>>>> ndev can never be NULL here. socfpga_dwmac_init() is only called if
>>>>> stmmac_dvr_probe() succeeds or we are running the resume callback. So
>>>>> I don't see how this could ever be NULL.
>>>>
>>>> That's a good point, this check can indeed be removed. While you're at
>>>> the patching, can you remove this one ?
>>>
>>> Yes, my patch will refactor the init() function so this will go away.
>>
>> Thanks!
>>
>>>>>> +
>>>>>> +       stpriv = netdev_priv(ndev);
>>>>>
>>>>> It's not really nice to access 'stmmac_priv' as it should be private
>>>>> to the core driver, but I don't see any other good solution right now.
>>>>
>>>> I guess some stmmac_reset_assert() wrapper would be nicer, yes. What do
>>>> you think ?
>>>>
>>>>>> +       if (!stpriv)
>>>>>> +               return -EINVAL;
>>>>>>
>>>>>>         /* Assert reset to the enet controller before changing the phy mode */
>>>>>> -       if (dwmac->stmmac_rst)
>>>>>> -               reset_control_assert(dwmac->stmmac_rst);
>>>>>> +       if (stpriv->stmmac_rst)
>>>>>> +               reset_control_assert(stpriv->stmmac_rst);
>>>>>>
>>>>>>         /* Setup the phy mode in the system manager registers according to
>>>>>>          * devicetree configuration
>>>>>> @@ -227,8 +210,8 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>>>         /* Deassert reset for the phy configuration to be sampled by
>>>>>>          * the enet controller, and operation to start in requested mode
>>>>>>          */
>>>>>> -       if (dwmac->stmmac_rst)
>>>>>> -               reset_control_deassert(dwmac->stmmac_rst);
>>>>>> +       if (stpriv->stmmac_rst)
>>>>>> +               reset_control_deassert(stpriv->stmmac_rst);
>>>>>>
>>>>>>         /* Before the enet controller is suspended, the phy is suspended.
>>>>>>          * This causes the phy clock to be gated. The enet controller is
>>>>>> @@ -245,7 +228,7 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>>>          * control register 0, and can be modified by the phy driver
>>>>>>          * framework.
>>>>>>          */
>>>>>> -       if (stpriv && stpriv->phydev)
>>>>>> +       if (stpriv->phydev)
>>>>>>                 phy_resume(stpriv->phydev);
>>>>>
>>>>> Before this change phy_resume() was only called during driver resume
>>>>> when , but your patches cause phy_resume() to called at probe time as
>>>>> well. Is this okey?
>>>>
>>>> I _hope_ it's OK. The cryptic comment above is not very helpful in this
>>>> aspect. Dinh ? :)
>>>
>>> My patches will move phy_resume() to the resume callback so it
>>> preserves the previous behavior. But if someone knows more about this
>>> that would be useful.
>>>
>>>
>>>> btw I wish you reviewed my patch a bit earlier to catch these bits.
>>>
>>> Sorry, about that. I have been really busy with other things lately.
>>
>> Oh I'm real happy someone is doing the refactoring :) I appreciate your
>> work, sorry if that was unclear.
>>
>>> My patches based on next from Friday can be found here now:
>>> https://github.com/manabian/linux-lpc/tree/net-socfpga-dwmac-on-next
>>>
>>> I had to add your latest patch as well since the next version I used
>>> didn't have it. I'll post the patches on netdev later today or
>>> tomorrow.
>>
>> Looks like next wasn't synced for a few days, yeah.
>>
>> You can add my:
>>
>> On SoCFPGA Cyclone V SoC (DENX MCVEVK):
>> Tested-by: Marek Vasut <marex@denx.de>
>>
>> to those patches
> 
> Excellent. Thanks Marek.
> 
> btw, did you also test suspend/resume?

No, I believe that is unsupported on SoCFPGA Cyclone V in general.

-- 
Best regards,
Marek Vasut