From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dinh Nguyen Subject: Re: [PATCH V2] net: stmmac: socfpga: Remove re-registration of reset controller Date: Wed, 20 Apr 2016 21:44:11 -0500 Message-ID: <57183E7B.5050804@opensource.altera.com> References: <1461110753-7641-1-git-send-email-marex@denx.de> <5717F1DF.1020004@opensource.altera.com> <5718023F.1060707@denx.de> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Cc: , , Matthew Gerlach , "David S . Miller" To: Marek Vasut , Return-path: Received: from mail-bn1on0088.outbound.protection.outlook.com ([157.56.110.88]:10720 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751001AbcDUCo2 (ORCPT ); Wed, 20 Apr 2016 22:44:28 -0400 In-Reply-To: <5718023F.1060707@denx.de> Sender: netdev-owner@vger.kernel.org List-ID: 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 >>> [] (unwind_backtrace) from [] (show_stack+0x10/0x14) >>> [] (show_stack) from [] (dump_stack+0x94/0xa8) >>> [] (dump_stack) from [] (__warn+0xec/0x104) >>> [] (__warn) from [] (warn_slowpath_null+0x20/0x28) >>> [] (warn_slowpath_null) from [] (__of_reset_control_get+0x218/0x270) >>> [] (__of_reset_control_get) from [] (__devm_reset_control_get+0x54/0x90) >>> [] (__devm_reset_control_get) from [] (stmmac_dvr_probe+0x1b4/0x8e8) >>> [] (stmmac_dvr_probe) from [] (socfpga_dwmac_probe+0x1b8/0x28c) >>> [] (socfpga_dwmac_probe) from [] (platform_drv_probe+0x4c/0xb0) >>> [] (platform_drv_probe) from [] (driver_probe_device+0x224/0x2bc) >>> [] (driver_probe_device) from [] (__driver_attach+0xac/0xb0) >>> [] (__driver_attach) from [] (bus_for_each_dev+0x6c/0xa0) >>> [] (bus_for_each_dev) from [] (bus_add_driver+0x1a4/0x21c) >>> [] (bus_add_driver) from [] (driver_register+0x78/0xf8) >>> [] (driver_register) from [] (do_one_initcall+0x40/0x170) >>> [] (do_one_initcall) from [] (kernel_init_freeable+0x1dc/0x27c) >>> [] (kernel_init_freeable) from [] (kernel_init+0x8/0x114) >>> [] (kernel_init) from [] (ret_from_fork+0x14/0x3c) >>> ---[ end trace 059d2fbe87608fa9 ]--- >>> >>> Signed-off-by: Marek Vasut >>> Cc: Matthew Gerlach >>> Cc: Dinh Nguyen >>> Cc: David S. Miller >>> --- >>> 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