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

[Marek Vasut <marex@denx.de>]

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