From: Marek Vasut <marex@denx•de>
To: Joachim Eastwood <manabian@gmail•com>
Cc: netdev <netdev@vger•kernel.org>,
"peppe.cavallaro" <peppe.cavallaro@st•com>,
alexandre.torgue@st•com,
Matthew Gerlach <mgerlach@opensource•altera.com>,
Dinh Nguyen <dinguyen@opensource•altera.com>,
"David S . Miller" <davem@davemloft•net>
Subject: Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller
Date: Tue, 26 Apr 2016 14:47:40 +0200 [thread overview]
Message-ID: <571F636C.9040503@denx.de> (raw)
In-Reply-To: <CAGhQ9Vz568hdSk+HWGHXuSNn4DU7HZkUBfVbajQyRkq+rBYY7Q@mail.gmail.com>
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
> regards,
> Joachim Eastwood
>
--
Best regards,
Marek Vasut
next prev parent reply other threads:[~2016-04-26 12:47 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-21 12:11 [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller Marek Vasut
2016-04-21 14:39 ` Dinh Nguyen
2016-04-24 18:40 ` David Miller
2016-04-25 18:11 ` Joachim Eastwood
2016-04-25 22:55 ` Marek Vasut
2016-04-26 12:26 ` Joachim Eastwood
2016-04-26 12:47 ` Marek Vasut [this message]
2016-04-26 21:22 ` Joachim Eastwood
2016-04-26 22:33 ` Marek Vasut
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=571F636C.9040503@denx.de \
--to=marex@denx$(echo .)de \
--cc=alexandre.torgue@st$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dinguyen@opensource$(echo .)altera.com \
--cc=manabian@gmail$(echo .)com \
--cc=mgerlach@opensource$(echo .)altera.com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=peppe.cavallaro@st$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox