Fix phy_init for Marvell network eth driver

Fix phy_init for Marvell network eth driver

[Kosta Zertsekel]

Hi all,
resubmitting the patch for Marvell mv643xx driver (patch 1/2) adding the below explanation (thanks to Florian F.):
The D-Link DNS323_REV_C1 board has a specific PHY device fixup using the flag MARVELL_PHY_M1118_DNS323_LEDS which is set by the architecture code in arch/arm/mach-orion5x/dns323-setup.c. This flag is unfortunately lost during the call to phy_attach() in mv643xx_eth and therefore, the underlying Marvell PHY driver cannot make use of this flag to fixup the PHY device LEDs. This patch ensures the phy->dev_flags value is passed to the phy_attach() function such that the PHY device driver can
actually use it.

Also submitting new patch 2/2 using the same (or at least very close in meaning) explanation.

[PATCH 1/2] Fix phy_init for Marvell network eth driver

[Kosta Zertsekel]

At least it fixes DNS323_REV_C1 board case (mach-orion5x/dsn323-setup.c file).

Change-Id: I37d64795f79c358f66a211a150cec2263e40e295
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 84c1326..7b8c1ac 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2789,7 +2789,7 @@ static void phy_init(struct mv643xx_eth_private *mp, int speed, int duplex)
 
 	phy_reset(mp);
 
-	phy_attach(mp->dev, dev_name(&phy->dev), 0, PHY_INTERFACE_MODE_GMII);
+	phy_attach(mp->dev, dev_name(&phy->dev), phy->dev_flags, PHY_INTERFACE_MODE_GMII);
 
 	if (speed == 0) {
 		phy->autoneg = AUTONEG_ENABLE;
-- 
1.8.1

Re: [PATCH 1/2] Fix phy_init for Marvell network eth driver

[Sergei Shtylyov]

Hello.

On 10-01-2013 16:00, Kosta Zertsekel wrote:

> At least it fixes DNS323_REV_C1 board case (mach-orion5x/dsn323-setup.c file).

> Change-Id: I37d64795f79c358f66a211a150cec2263e40e295

    This line has no place in the upstream commit, please remove it. Also, the 
patch misses your signoff, so  couldn't be applied anyway. Same about the next 
patch.

WBR, Sergei

[PATCH 2/2] Fix phy_attach - forward dev_flags for phy_attach

[Kosta Zertsekel]

Change-Id: Ie3191f95c36eada6d0c673460de5393641128182
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 2 +-
 net/dsa/slave.c                           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 10d678d..63baa3b 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -1391,7 +1391,7 @@ static void phy_init(struct pxa168_eth_private *pep, int speed, int duplex)
 	struct phy_device *phy = pep->phy;
 	ethernet_phy_reset(pep);
 
-	phy_attach(pep->dev, dev_name(&phy->dev), 0, PHY_INTERFACE_MODE_MII);
+	phy_attach(pep->dev, dev_name(&phy->dev), phy->dev_flags, PHY_INTERFACE_MODE_MII);
 
 	if (speed == 0) {
 		phy->autoneg = AUTONEG_ENABLE;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e32083d..bf09902 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -391,7 +391,7 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 
 	if (p->phy != NULL) {
 		phy_attach(slave_dev, dev_name(&p->phy->dev),
-			   0, PHY_INTERFACE_MODE_GMII);
+			   phy->dev_flags, PHY_INTERFACE_MODE_GMII);
 
 		p->phy->autoneg = AUTONEG_ENABLE;
 		p->phy->speed = 0;
-- 
1.8.1

Re: [PATCH 2/2] Fix phy_attach - forward dev_flags for phy_attach

[Florian Fainelli]

Le 01/10/13 13:00, Kosta Zertsekel a écrit :
> Change-Id: Ie3191f95c36eada6d0c673460de5393641128182
> ---
>   drivers/net/ethernet/marvell/pxa168_eth.c | 2 +-
>   net/dsa/slave.c                           | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

I think that you should have actually two patches, one for pxa168_eth 
and one for net/dsa/slave.c.

Also, please prefix your patches with what was usually used on these 
files before: "pxa168_eth:" and "net: dsa:" respectively.

By the way, most, if not all of the phy_connect() users in 
drivers/net/ethernet/ also do not ensure they pass the phy device flags, 
so you might want to fix this globally and not just for Marvell driver.

Thanks.
--
Florian

RE: [PATCH 2/2] Fix phy_attach - forward dev_flags for phy_attach

[Kosta Zertsekel]

> By the way, most, if not all of the phy_connect() users in drivers/net/ethernet/ also do not ensure they pass the phy device flags, so you might 

want to fix this globally and not just for Marvell driver.
Indeed, phy_connect() mostly just pass zero intead of phy_dev->dev_flags.
But, I think, the guy that calls phy_connect() in its driver should know what he does, and,
probably, we should rely on him knowing his stuff.
The only evidence of the bug is when phy_dev->dev_flags was actually changed by PHY fixup callback
(see dns323-setup.c for example) and was *not* propagated to phy_connect() or phy_attach() as in pxa168_eth.c phy_init().
The code conforming to the former should be fixed IMHO.

Thanks,
--- KostaZ

Re: [PATCH 2/2] Fix phy_attach - forward dev_flags for phy_attach

[Florian Fainelli]

Le 01/10/13 16:57, Kosta Zertsekel a écrit :
>> By the way, most, if not all of the phy_connect() users in drivers/net/ethernet/ also do not ensure they pass the phy device flags, so you might
> want to fix this globally and not just for Marvell driver.
> Indeed, phy_connect() mostly just pass zero intead of phy_dev->dev_flags.
> But, I think, the guy that calls phy_connect() in its driver should know what he does, and,
> probably, we should rely on him knowing his stuff.
> The only evidence of the bug is when phy_dev->dev_flags was actually changed by PHY fixup callback
> (see dns323-setup.c for example) and was *not* propagated to phy_connect() or phy_attach() as in pxa168_eth.c phy_init().
> The code conforming to the former should be fixed IMHO.

Actually, I wonder if we should not rather remove entirely the flags 
argument, let the phy_connect() or phy_attach() callers modify the phy 
device dev_flags like it does today (e.g: tg3) and modify phy_connect() 
and phy_attach() to pass phy->dev_flags to phy_attach_direct().
--
Florian

Re: Fix phy_init for Marvell network eth driver

[Jason Cooper]

On Thu, Jan 10, 2013 at 02:00:32PM +0200, Kosta Zertsekel wrote:
> Hi all,
> resubmitting the patch for Marvell mv643xx driver (patch 1/2) adding the below explanation (thanks to Florian F.):
> The D-Link DNS323_REV_C1 board has a specific PHY device fixup using the flag MARVELL_PHY_M1118_DNS323_LEDS which is set by the architecture code in arch/arm/mach-orion5x/dns323-setup.c. This flag is unfortunately lost during the call to phy_attach() in mv643xx_eth and therefore, the underlying Marvell PHY driver cannot make use of this flag to fixup the PHY device LEDs. This patch ensures the phy->dev_flags value is passed to the phy_attach() function such that the PHY device driver can
> actually use it.
> 
> Also submitting new patch 2/2 using the same (or at least very close in meaning) explanation.

Could you please put the above description in the patch commit log for
both patches?  Other than that,

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

Re: Fix phy_init for Marvell network eth driver

[Andrew Lunn]

On Thu, Jan 10, 2013 at 02:00:32PM +0200, Kosta Zertsekel wrote:
> Hi all,
> resubmitting the patch for Marvell mv643xx driver (patch 1/2) adding the below explanation (thanks to Florian F.):
> The D-Link DNS323_REV_C1 board has a specific PHY device fixup using the flag MARVELL_PHY_M1118_DNS323_LEDS which is set by the architecture code in arch/arm/mach-orion5x/dns323-setup.c. This flag is unfortunately lost during the call to phy_attach() in mv643xx_eth and therefore, the underlying Marvell PHY driver cannot make use of this flag to fixup the PHY device LEDs. This patch ensures the phy->dev_flags value is passed to the phy_attach() function such that the PHY device driver can
> actually use it.
> 
> Also submitting new patch 2/2 using the same (or at least very close in meaning) explanation.
> 

Hi Kosta

This comment is supposed to be in the header of the patch. 

Since this comment is about a bug for the D-Link DNS323_REV_C1, the
fix to DSA should be in a separate patch, again with a comment about
the patch, why its needed, what if fixes etc.

I suggest you take a look at Documentation/SubmittingPatches

Thanks
	Andrew

RE: Fix phy_init for Marvell network eth driver

[Kosta Zertsekel]

> This comment is supposed to be in the header of the patch. 
> Since this comment is about a bug for the D-Link DNS323_REV_C1, the fix to DSA should be in a separate patch, again with a comment about the patch, why its needed, what if fixes etc.
> I suggest you take a look at Documentation/SubmittingPatches

Oh, I know. I am really sorry for the mess with 'git sendemail' (forgot --compose flag or something).
I'll resend the patch "locally" with patch header email and then repost the both patches again on netdev and linux-arm-kernel.
Sorry again.

Thanks,
--- KostaZ