On 04/19/2016 07:05 PM, 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 is not great. Since the hardware needs to toggle the reset
> before calling stmmac_dvr_probe() to perform mandatory preconfiguration,
> this patch splits socfpga_dwmac_init_probe() from socfpga_dwmac_init().
>
> The socfpga_dwmac_init_probe() temporarily registers the reset controller,
> performs the pre-configuration and unregisters the reset controller again.
> This function is only called from the socfpga_dwmac_probe().
>
> 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.
>
> 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 <ma...@denx.de>
> Cc: Matthew Gerlach <mgerl...@opensource.altera.com>
> Cc: Dinh Nguyen <dingu...@opensource.altera.com>
> Cc: David S. Miller <da...@davemloft.net>
> ---
> V2: Add missing stmmac_rst = NULL; into socfpga_dwmac_init_probe()
> ---
> .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 70
> ++++++++++++----------
> 1 file changed, 39 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index 76d671e..5885a2e 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,12 +228,38 @@ 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;
> }
>
> +static int socfpga_dwmac_init_probe(struct socfpga_dwmac *dwmac)
> +{
> + struct reset_control *stmmac_rst;
> + int ret;
> +
> + stmmac_rst = reset_control_get(dwmac->dev, STMMAC_RESOURCE_NAME);
> + if (IS_ERR(stmmac_rst)) {
> + dev_info(dwmac->dev, "Could not get reset control!\n");
> + if (PTR_ERR(stmmac_rst) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + stmmac_rst = NULL;
> + }
> +
> + if (stmmac_rst)
> + reset_control_assert(stmmac_rst);
> +
> + ret = socfpga_dwmac_setup(dwmac);
> +
> + if (stmmac_rst) {
> + reset_control_deassert(stmmac_rst);
> + reset_control_put(stmmac_rst);
> + }
> +
> + return ret;
> +}
I don't think you this function because...
> +
> static int socfpga_dwmac_probe(struct platform_device *pdev)
> {
> struct plat_stmmacenet_data *plat_dat;
> @@ -279,10 +288,9 @@ 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);
> + ret = socfpga_dwmac_init_probe(dwmac);
> if (ret)
> return ret;
>
>
if you modify the patch to call stmmac_dvr_probe() before calling
socfpga_dwmac_init(), then you would already have the reset control
information.
Something like this:
---------------------------------8<--------------------------------
@@ -269,14 +252,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);
- return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+ if (!ret)
+ ret = socfpga_dwmac_init(pdev, plat_dat->bsp_priv);
+ return ret;
}
What do you think?
Dinh
