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

[Marek Vasut <marex@denx.de>]

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