On 26 April 2016 at 14:47, Marek Vasut <ma...@denx.de> wrote:
> On 04/26/2016 02:26 PM, Joachim Eastwood wrote:
>> On 26 April 2016 at 00:55, Marek Vasut <ma...@denx.de> wrote:
>>> On 04/25/2016 08:11 PM, Joachim Eastwood wrote:
>>>> On 21 April 2016 at 14:11, Marek Vasut <ma...@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 <ma...@denx.de>
>
> to those patches
Excellent. Thanks Marek.
btw, did you also test suspend/resume?
regards,
Joachim Eastwood
