| Message ID | 1461110753-7641-1-git-send-email-marex@denx.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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 ]--- > Odd, I hadn't seen this error before. I would have noticed it, but I haven't ran linux-next for a few days. I see that this error was introduced on linux-next/next-20160414. The error was not there prior to that. While I agree that the extra call to get the reset control is not needed, I wonder what commit between next-20160413 and next-20160414 exposed this error. I'll try to dig this some more. Dinh
On 04/20/2016 04:04 AM, Dinh Nguyen wrote: > > > 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 ]--- >> > > Odd, I hadn't seen this error before. I would have noticed it, but I > haven't ran linux-next for a few days. > > I see that this error was introduced on linux-next/next-20160414. The > error was not there prior to that. While I agree that the extra call to > get the reset control is not needed, I wonder what commit between > next-20160413 and next-20160414 exposed this error. I'll try to dig this > some more. I'll save you the work, it's this one :) commit 0b52297f2288ca239e598afe6c92db83d8d2bfcd Author: Hans de Goede <hdegoede@redhat.com> Date: Tue Feb 23 18:46:26 2016 +0100 reset: Add support for shared reset controls In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new:
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 <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() > --- > .../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
On 04/20/2016 11:17 PM, Dinh Nguyen wrote: > 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 <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() >> --- >> .../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. I was under the impression that you must call socfpga_dwmac_init() before stmmac_dvr_probe() for whatever hardware-related reason. If you are absolutely certain this is not necessary, then that's just perfect and the patch can be simplified even further -- just remove the call to socfpga_dwmac_init() from probe altogether , the dwmac core code will call plat_dat->init at the end of probe . So shall we do that ? I am happy to spin V3 like that if you confirm that it's legal to do things in the aforementioned order. > 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 >
On 04/20/2016 05:27 PM, Marek Vasut wrote: > On 04/20/2016 11:17 PM, Dinh Nguyen wrote: >> 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 <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() >>> --- >>> .../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. > > I was under the impression that you must call socfpga_dwmac_init() > before stmmac_dvr_probe() for whatever hardware-related reason. If > you are absolutely certain this is not necessary, then that's just > perfect and the patch can be simplified even further -- just remove > the call to socfpga_dwmac_init() from probe altogether , the dwmac > core code will call plat_dat->init at the end of probe . > > So shall we do that ? I am happy to spin V3 like that if you confirm > that it's legal to do things in the aforementioned order. > AFAICT, I don't see any reason why the socfpga_dwmac_init() has to go before the stmmac_dvr_probe(). I tested this by using U-Boot to put the PHY modes in the system manager into a different mode, and put the ethernet IP into reset. Linux was able to use ethernet just fine with the aformentioned order. So yes, please spin V3. Thanks, Dinh
On 04/21/2016 04:44 AM, Dinh Nguyen wrote: > > > On 04/20/2016 05:27 PM, Marek Vasut wrote: >> On 04/20/2016 11:17 PM, Dinh Nguyen wrote: >>> 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 <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() >>>> --- >>>> .../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. >> >> I was under the impression that you must call socfpga_dwmac_init() >> before stmmac_dvr_probe() for whatever hardware-related reason. If >> you are absolutely certain this is not necessary, then that's just >> perfect and the patch can be simplified even further -- just remove >> the call to socfpga_dwmac_init() from probe altogether , the dwmac >> core code will call plat_dat->init at the end of probe . >> >> So shall we do that ? I am happy to spin V3 like that if you confirm >> that it's legal to do things in the aforementioned order. >> > > AFAICT, I don't see any reason why the socfpga_dwmac_init() has to go > before the stmmac_dvr_probe(). I tested this by using U-Boot to put the > PHY modes in the system manager into a different mode, and put the > ethernet IP into reset. Linux was able to use ethernet just fine with > the aformentioned order. Ha, the dwmac code does not call the ->init, so you have to call it from the socfpga-dwmac probe afterall. I will send a V3 now and do it just like you suggested. I will send a RFC for calling ->init and ->exit from the stmmac common code too. > So yes, please spin V3. > > Thanks, > Dinh >
On 04/21/2016 06:51 AM, Marek Vasut wrote: >>>> >>>> if you modify the patch to call stmmac_dvr_probe() before calling >>>> socfpga_dwmac_init(), then you would already have the reset control >>>> information. >>> >>> I was under the impression that you must call socfpga_dwmac_init() >>> before stmmac_dvr_probe() for whatever hardware-related reason. If >>> you are absolutely certain this is not necessary, then that's just >>> perfect and the patch can be simplified even further -- just remove >>> the call to socfpga_dwmac_init() from probe altogether , the dwmac >>> core code will call plat_dat->init at the end of probe . >>> >>> So shall we do that ? I am happy to spin V3 like that if you confirm >>> that it's legal to do things in the aforementioned order. >>> >> >> AFAICT, I don't see any reason why the socfpga_dwmac_init() has to go >> before the stmmac_dvr_probe(). I tested this by using U-Boot to put the >> PHY modes in the system manager into a different mode, and put the >> ethernet IP into reset. Linux was able to use ethernet just fine with >> the aformentioned order. > > Ha, the dwmac code does not call the ->init, so you have to call it from > the socfpga-dwmac probe afterall. I will send a V3 now and do > it just like you suggested. I will send a RFC for calling ->init and > ->exit from the stmmac common code too. > Yes, I saw this too. Was going to wait until after this patch to follow-up. Looks like you already did! Thanks! Dinh
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; +} + 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;
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 <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() --- .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 70 ++++++++++++---------- 1 file changed, 39 insertions(+), 31 deletions(-)