FWD: [PATCH v2] Marvell phy: add fiber status check for some components

FWD: [PATCH v2] Marvell phy: add fiber status check for some components

[Andrew Lunn]

> >From 564b767163d19355a3b5efaad195e93796570c71 Mon Sep 17 00:00:00 2001
> From: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
> Date: Fri, 1 Apr 2016 16:16:35 +0200
> Subject: [PATCH] Marvell phy: add fiber status check for some components
> 
> Marvell's phy could have two modes: fiber and copper. Currently, the driver
> checks only the copper mode registers to get the status link which could be
> wrong.
> 
> This commit add a handler to check fiber then copper status link.
> If the fiber link is activated, the driver would use this information.
> Else, it would use the copper status.

Hi Florian

What do you think about this?

This works for basic status information. But what about other ethtool
options? Setting the speed and duplex, turning pause on/off, etc.

Do we actually need to stay on page 1 if fibre is in use? How do we
initially change to page 1 when the fibre link is still down?

Should we be using the old mechanism to swap between TP, BNC and AUI
to swap between copper and fibre?

   Andrew

> 
> This patch is not tested with all Marvell's phy.
> The new function is actived only for tested phys.
> 
> Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
> ---
>  drivers/net/phy/marvell.c | 43 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index ab1d0fc..22552ab 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -890,6 +890,45 @@ static int marvell_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/* marvell_read_fiber_status
> + *
> + * Some Marvell's phys have two modes: fiber and copper.
> + * Both need status checked.
> + * Description:
> + *   First, check the fiber link and status.
> + *   If the fiber link is down, check the copper link and status which
> + *   will be the default value if both link are down.
> + */
> +static int marvell_read_fiber_status(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	/* Check the fiber mode first */
> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> +	if (err < 0)
> +		goto error;
> +
> +	err = marvell_read_status(phydev);
> +	if (err < 0)
> +		goto error;
> +
> +	if (phydev->link) {
> +		phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +		return 0;
> +	}
> +
> +	/* If fiber link is down, check and save copper mode state */
> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +	if (err < 0)
> +		goto error;
> +
> +	return marvell_read_status(phydev);
> +
> +error:
> +	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +	return err;
> +}
> +
>  static int marvell_aneg_done(struct phy_device *phydev)
>  {
>  	int retval = phy_read(phydev, MII_M1011_PHY_STATUS);
> @@ -1122,7 +1161,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.probe = marvell_probe,
>  		.config_init = &m88e1111_config_init,
>  		.config_aneg = &marvell_config_aneg,
> -		.read_status = &marvell_read_status,
> +		.read_status = &marvell_read_fiber_status,
>  		.ack_interrupt = &marvell_ack_interrupt,
>  		.config_intr = &marvell_config_intr,
>  		.resume = &genphy_resume,
> @@ -1270,7 +1309,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.probe = marvell_probe,
>  		.config_init = &marvell_config_init,
>  		.config_aneg = &m88e1510_config_aneg,
> -		.read_status = &marvell_read_status,
> +		.read_status = &marvell_read_fiber_status,
>  		.ack_interrupt = &marvell_ack_interrupt,
>  		.config_intr = &marvell_config_intr,
>  		.did_interrupt = &m88e1121_did_interrupt,
> -- 
> 2.5.5
> 
> 
> 
> ----- End forwarded message -----
> 

Re: FWD: [PATCH v2] Marvell phy: add fiber status check for some components

[Charles-Antoine Couret]

Le 04/04/2016 15:25, Andrew Lunn a écrit :
> Should we be using the old mechanism to swap between TP, BNC and AUI
> to swap between copper and fibre?
> 
>    Andrew

What is this method ?
A specific ioctl ?

Regards,
Charles-Antoine

Re: FWD: [PATCH v2] Marvell phy: add fiber status check for some components

[Florian Fainelli]

On 04/04/16 06:25, Andrew Lunn wrote:
>> >From 564b767163d19355a3b5efaad195e93796570c71 Mon Sep 17 00:00:00 2001
>> From: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
>> Date: Fri, 1 Apr 2016 16:16:35 +0200
>> Subject: [PATCH] Marvell phy: add fiber status check for some components
>>
>> Marvell's phy could have two modes: fiber and copper. Currently, the driver
>> checks only the copper mode registers to get the status link which could be
>> wrong.
>>
>> This commit add a handler to check fiber then copper status link.
>> If the fiber link is activated, the driver would use this information.
>> Else, it would use the copper status.
> 
> Hi Florian
> 
> What do you think about this?
> 
> This works for basic status information. But what about other ethtool
> options? Setting the speed and duplex, turning pause on/off, etc.

Agreed, it seems like a PHY configured for fiber will need to provide
these informations differently, or does the standard BMSR register
provide accurate information already?

> 
> Do we actually need to stay on page 1 if fibre is in use? How do we
> initially change to page 1 when the fibre link is still down?

I also do not feel very comfortable with reading the fiber status first,
and then copper and then combine these two. At the very best, could we
do something like:

- identify if the PHY is configured for fiber in drv->probe or
drv->config_init, retain that information
- have two paths in drv->read_status which take care of reading one
status or the other?

Are there other side effects for other register accesses (say,
statistics, or auto-negotiation) that need to be fiber vs. copper aware?

> 
> Should we be using the old mechanism to swap between TP, BNC and AUI
> to swap between copper and fibre?

Did you mean using ethtool -s <iface> port fibre for instance?
-- 
Florian

Re: FWD: [PATCH v2] Marvell phy: add fiber status check for some components

[Charles-Antoine Couret]

Le 11/04/2016 21:47, Florian Fainelli a écrit :
> On 04/04/16 06:25, Andrew Lunn wrote:
>>> >From 564b767163d19355a3b5efaad195e93796570c71 Mon Sep 17 00:00:00 2001
>>> From: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
>>> Date: Fri, 1 Apr 2016 16:16:35 +0200
>>> Subject: [PATCH] Marvell phy: add fiber status check for some components
>>>
>>> Marvell's phy could have two modes: fiber and copper. Currently, the driver
>>> checks only the copper mode registers to get the status link which could be
>>> wrong.
>>>
>>> This commit add a handler to check fiber then copper status link.
>>> If the fiber link is activated, the driver would use this information.
>>> Else, it would use the copper status.
>>
>> Hi Florian
>>
>> What do you think about this?
>>
>> This works for basic status information. But what about other ethtool
>> options? Setting the speed and duplex, turning pause on/off, etc.
> 
> Agreed, it seems like a PHY configured for fiber will need to provide
> these informations differently, or does the standard BMSR register
> provide accurate information already?


The BSMR is similar between fiber and copper mode (but, it's the same values for duplex, speed...) in register 17.
To force speed, duplex, etc. it's the same way too (register 0).

The register's numbers are the same, only the page change.

>> Do we actually need to stay on page 1 if fibre is in use? How do we
>> initially change to page 1 when the fibre link is still down?
> 
> I also do not feel very comfortable with reading the fiber status first,
> and then copper and then combine these two. At the very best, could we
> do something like:
> 
> - identify if the PHY is configured for fiber in drv->probe or
> drv->config_init, retain that information
> - have two paths in drv->read_status which take care of reading one
> status or the other?

It should be a great idea.
Because, we could select the preferred link (copper or fiber). This option could select that during the init process.

I could save the state into marvell_priv structure, but how configure this choice ? Set copper by default and the user change that
by ethtool, driver loading option or the driver code ? I don't know what is the correct way to configure that properly. 


> Are there other side effects for other register accesses (say,
> statistics, or auto-negotiation) that need to be fiber vs. copper aware?

Auto-negociation are separated. But the statistics are globally common, there are some specific registers about that too. But I think these registers are not relevant.

I will improve my patch.
Thanks.

Regards.
Charles-Antoine Couret

Re: FWD: [PATCH v2] Marvell phy: add fiber status check for some components

[Charles-Antoine Couret]

Le 11/04/2016 à 21:47, Florian Fainelli a écrit :
>> Do we actually need to stay on page 1 if fibre is in use? How do we
>> initially change to page 1 when the fibre link is still down?
> 
> I also do not feel very comfortable with reading the fiber status first,
> and then copper and then combine these two. At the very best, could we
> do something like:
> 
> - identify if the PHY is configured for fiber in drv->probe or
> drv->config_init, retain that information

But, how configure in runtime the user choice? Add a driver option?
Else, a default choice should be added in init/probe function and the user should change by ethtool or driver recompilation.

> - have two paths in drv->read_status which take care of reading one
> status or the other?

I worked for a solution around that.
But the Marvell's datasheet seems to agree with my previous method:

Extract:
"Notes on Determining which Media Linked Up

Since there are two sets of IEEE registers (one for copper and the other for fiber) the software needs
to be aware of register 22.7:0 so that the correct set of registers are selected. In general the
sequence is as follows.

1-Set the Auto-Negotiation registers of the copper medium. (This step may not be necessary if the
hardware configuration defaults are acceptable).

2-Set the Auto-Negotiation registers of the fiber medium. (This step may not be necessary if the
hardware configuration defaults are acceptable).

3-Poll for link status.Go to step 4 if there is link.

4-Once there is link determine whether the link is copper or fiber medium.

5-Look at the Auto-Negotiation results for the medium that established link.

6-Poll for link status. If link status goes down then go back to step 3."

By default, the phy is configured to be connected on both interfaces with any preference. The first connected was selected.
A preference could be added, but it is not a force mode.

Extract:
"Preferred Media

The device can be programmed to give one media priority over the other. In other words if the
non-preferred media establishes link first and subsequently energy is detected on the preferred
media, the PHY will drop link on the non-preferred media for 4 seconds and give the preferred media
a chance to establish link."

We don't have registers to know really if Copper or Fiber is selected. We can know only by checking status registers as in my commit.


>> Should we be using the old mechanism to swap between TP, BNC and AUI
>> to swap between copper and fibre?
> 
> Did you mean using ethtool -s <iface> port fibre for instance?

So, to implement that I have to renable the port change in phy_ethtool_sset function.
But, the datasheet seems to disagree with this method.

Finally, what do I have to do? I continue my previous way or your suggestion?
I prefer to respect the datasheet, but if it's better for you to follow the other way, I will implement that.

Thank you in advance and have a nice day.
Regards,
Charles-Antoine Couret

Re: FWD: [PATCH v2] Marvell phy: add fiber status check for some components

[Charles-Antoine Couret]

Hello,
I'm sorry to repost that, but after one month, I need a answer to continue to imrpove my patch in the right direction. :)

Thank you in advance.
Regards.
Charles-Antoine Couret

Le 29/04/2016 à 10:28, Charles-Antoine Couret a écrit :
> Le 11/04/2016 à 21:47, Florian Fainelli a écrit :
>>> Do we actually need to stay on page 1 if fibre is in use? How do we
>>> initially change to page 1 when the fibre link is still down?
>>
>> I also do not feel very comfortable with reading the fiber status first,
>> and then copper and then combine these two. At the very best, could we
>> do something like:
>>
>> - identify if the PHY is configured for fiber in drv->probe or
>> drv->config_init, retain that information
> 
> But, how configure in runtime the user choice? Add a driver option?
> Else, a default choice should be added in init/probe function and the user should change by ethtool or driver recompilation.
> 
>> - have two paths in drv->read_status which take care of reading one
>> status or the other?
> 
> I worked for a solution around that.
> But the Marvell's datasheet seems to agree with my previous method:
> 
> Extract:
> "Notes on Determining which Media Linked Up
> 
> Since there are two sets of IEEE registers (one for copper and the other for fiber) the software needs
> to be aware of register 22.7:0 so that the correct set of registers are selected. In general the
> sequence is as follows.
> 
> 1-Set the Auto-Negotiation registers of the copper medium. (This step may not be necessary if the
> hardware configuration defaults are acceptable).
> 
> 2-Set the Auto-Negotiation registers of the fiber medium. (This step may not be necessary if the
> hardware configuration defaults are acceptable).
> 
> 3-Poll for link status.Go to step 4 if there is link.
> 
> 4-Once there is link determine whether the link is copper or fiber medium.
> 
> 5-Look at the Auto-Negotiation results for the medium that established link.
> 
> 6-Poll for link status. If link status goes down then go back to step 3."
> 
> By default, the phy is configured to be connected on both interfaces with any preference. The first connected was selected.
> A preference could be added, but it is not a force mode.
> 
> Extract:
> "Preferred Media
> 
> The device can be programmed to give one media priority over the other. In other words if the
> non-preferred media establishes link first and subsequently energy is detected on the preferred
> media, the PHY will drop link on the non-preferred media for 4 seconds and give the preferred media
> a chance to establish link."
> 
> We don't have registers to know really if Copper or Fiber is selected. We can know only by checking status registers as in my commit.
> 
> 
>>> Should we be using the old mechanism to swap between TP, BNC and AUI
>>> to swap between copper and fibre?
>>
>> Did you mean using ethtool -s <iface> port fibre for instance?
> 
> So, to implement that I have to renable the port change in phy_ethtool_sset function.
> But, the datasheet seems to disagree with this method.
> 
> Finally, what do I have to do? I continue my previous way or your suggestion?
> I prefer to respect the datasheet, but if it's better for you to follow the other way, I will implement that.
> 
> Thank you in advance and have a nice day.
> Regards,
> Charles-Antoine Couret
> 

Re: FWD: [PATCH v2] Marvell phy: add fiber status check for some components

[Andrew Lunn]

On Fri, May 27, 2016 at 11:23:22AM +0200, Charles-Antoine Couret wrote:
> 
> Hello,
> I'm sorry to repost that, but after one month, I need a answer to continue to imrpove my patch in the right direction. :)

Hi Charles-Aontine

Florian and I had a quick discussion. We think going with the Marvell
datasheet documentation is best.

Please just be careful to ensure the generic code does not try to
unconditionally read the registers, only the Marvell driver. I'm
talking about functions like genphy_config_advert(),
genphy_setup_forced(), genphy_restart_aneg() etc. We need to ensure we
are consistently using the correct page.

      Andrew

[PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Charles-Antoine Couret]

Hello,
I'm back with another patch evrsion about Marvell phys with a fiber interface.
>From the previous release, I fixed some issues reported by yours and I added some functions around the fiber interface to get statistics, to configure the aneg, etc.
Yes, to implement that, copper and fiber side are closed, but the fiber link needs some custom registers or bit values. I'm based on some genphy functions to adapt to fiber link.

With that, the 88E1512 for example could have the same features for the copper and fiber links.
I'm interested by your feedback about this patch.

Thank you in advance.
Regards.
Charles-Antoine Couret

>From 4ca5935f8aa97c3ba02cb27e970e1bcf248aed18 Mon Sep 17 00:00:00 2001
From: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
Date: Fri, 1 Apr 2016 16:16:35 +0200
Subject: [PATCH] Marvell phy: add fiber status check for some components

Some Marvell's phys have two modes: fiber and copper. Currently, the driver
configures and checks only the copper mode registers
which could be the wrong link.

This commit add handlers to check fiber then copper status link
or to configure the fiber interface.

This patch is not tested with all Marvell's phys.
The new functions are enabled only for tested phys.
Some of them functions are based on genphy functions.

Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
---
 drivers/net/phy/marvell.c | 323 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 307 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index ec2c1ee..3cd8d94 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -138,6 +138,20 @@
 #define MII_88E1510_GEN_CTRL_REG_1_MODE_SGMII	0x1	/* SGMII to copper */
 #define MII_88E1510_GEN_CTRL_REG_1_RESET	0x8000	/* Soft reset */
 
+#define LPA_FIBER_1000HALF	0x40
+#define LPA_FIBER_1000FULL	0x20
+
+#define LPA_PAUSE_FIBER		0x180
+#define LPA_PAUSE_ASYM_FIBER	0x100
+
+#define ADVERTISE_FIBER_1000HALF	0x40
+#define ADVERTISE_FIBER_1000FULL	0x20
+
+#define ADVERTISE_PAUSE_FIBER		0x180
+#define ADVERTISE_PAUSE_ASYM_FIBER	0x100
+
+#define REGISTER_LINK_STATUS	0x400
+
 MODULE_DESCRIPTION("Marvell PHY driver");
 MODULE_AUTHOR("Andy Fleming");
 MODULE_LICENSE("GPL");
@@ -151,6 +165,7 @@ struct marvell_hw_stat {
 
 static struct marvell_hw_stat marvell_hw_stats[] = {
 	{ "phy_receive_errors", 0, 21, 16},
+	{ "phy_receive_errors_fiber", 1, 21, 16},
 	{ "phy_idle_errors", 0, 10, 8 },
 };
 
@@ -477,15 +492,122 @@ static int m88e1318_config_aneg(struct phy_device *phydev)
 	return m88e1121_config_aneg(phydev);
 }
 
+/**
+ * ethtool_adv_to_fiber_adv_t
+ * @ethadv: the ethtool advertisement settings
+ *
+ * A small helper function that translates ethtool advertisement
+ * settings to phy autonegotiation advertisements for the
+ * MII_ADV register for fiber link.
+ */
+static inline u32 ethtool_adv_to_fiber_adv_t(u32 ethadv)
+{
+	u32 result = 0;
+
+	if (ethadv & ADVERTISED_1000baseT_Half)
+		result |= ADVERTISE_FIBER_1000HALF;
+	if (ethadv & ADVERTISED_1000baseT_Full)
+		result |= ADVERTISE_FIBER_1000FULL;
+
+	if ((ethadv & ADVERTISE_PAUSE_ASYM) && (ethadv & ADVERTISE_PAUSE_CAP))
+		result |= LPA_PAUSE_ASYM_FIBER;
+	else if (ethadv & ADVERTISE_PAUSE_CAP)
+		result |= (ADVERTISE_PAUSE_FIBER
+			   & (~ADVERTISE_PAUSE_ASYM_FIBER));
+
+	return result;
+}
+
+/**
+ * marvell_config_aneg_fiber - restart auto-negotiation or write BMCR
+ * @phydev: target phy_device struct
+ *
+ * Description: If auto-negotiation is enabled, we configure the
+ *   advertising, and then restart auto-negotiation.  If it is not
+ *   enabled, then we write the BMCR. Adapted for fiber link in
+ *   some Marvell's devices.
+ */
+static int marvell_config_aneg_fiber(struct phy_device *phydev)
+{
+	int changed = 0;
+	int err;
+	int adv, oldadv;
+	u32 advertise;
+
+	if (phydev->autoneg != AUTONEG_ENABLE)
+		return genphy_setup_forced(phydev);
+
+	/* Only allow advertising what this PHY supports */
+	phydev->advertising &= phydev->supported;
+	advertise = phydev->advertising;
+
+	/* Setup fiber advertisement */
+	adv = phy_read(phydev, MII_ADVERTISE);
+	if (adv < 0)
+		return adv;
+
+	oldadv = adv;
+	adv &= ~(ADVERTISE_FIBER_1000HALF | ADVERTISE_FIBER_1000FULL
+		| LPA_PAUSE_FIBER);
+	adv |= ethtool_adv_to_fiber_adv_t(advertise);
+
+	if (adv != oldadv) {
+		err = phy_write(phydev, MII_ADVERTISE, adv);
+		if (err < 0)
+			return err;
+
+		changed = 1;
+	}
+
+	if (changed == 0) {
+		/* Advertisement hasn't changed, but maybe aneg was never on to
+		 * begin with?  Or maybe phy was isolated?
+		 */
+		int ctl = phy_read(phydev, MII_BMCR);
+
+		if (ctl < 0)
+			return ctl;
+
+		if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
+			changed = 1; /* do restart aneg */
+	}
+
+	/* Only restart aneg if we are advertising something different
+	 * than we were before.
+	 */
+	if (changed > 0)
+		changed = genphy_restart_aneg(phydev);
+
+	return changed;
+}
+
 static int m88e1510_config_aneg(struct phy_device *phydev)
 {
 	int err;
 
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+	if (err < 0)
+		goto error;
+
+	/* Configure the copper link first */
 	err = m88e1318_config_aneg(phydev);
 	if (err < 0)
-		return err;
+		goto error;
 
-	return 0;
+	/* Then the fiber link */
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
+	if (err < 0)
+		goto error;
+
+	err = marvell_config_aneg_fiber(phydev);
+	if (err < 0)
+		goto error;
+
+	return phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+
+error:
+	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+	return err;
 }
 
 static int marvell_config_init(struct phy_device *phydev)
@@ -890,6 +1012,48 @@ static int m88e1145_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+/**
+ * fiber_lpa_to_ethtool_lpa_t
+ * @lpa: value of the MII_LPA register for fiber link
+ *
+ * A small helper function that translates MII_LPA
+ * bits to ethtool LP advertisement settings.
+ */
+static u32 fiber_lpa_to_ethtool_lpa_t(u32 lpa)
+{
+	u32 result = 0;
+
+	if (lpa & LPA_FIBER_1000HALF)
+		result |= ADVERTISED_1000baseT_Half;
+	if (lpa & LPA_FIBER_1000FULL)
+		result |= ADVERTISED_1000baseT_Full;
+
+	return result;
+}
+
+/**
+ * marvell_update_link - update link status in real time in @phydev
+ * @phydev: target phy_device struct
+ *
+ * Description: Update the value in phydev->link to reflect the
+ *   current link value.
+ */
+static int marvell_update_link(struct phy_device *phydev)
+{
+	int status;
+
+	status = phy_read(phydev, MII_M1011_PHY_STATUS);
+	if (status < 0)
+		return status;
+
+	if ((status & REGISTER_LINK_STATUS) == 0)
+		phydev->link = 0;
+	else
+		phydev->link = 1;
+
+	return 0;
+}
+
 /* marvell_read_status
  *
  * Generic status code does not detect Fiber correctly!
@@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device *phydev)
 	int lpa;
 	int lpagb;
 	int status = 0;
+	int page, fiber;
 
-	/* Update the link, but return if there
+	/* Detect and update the link, but return if there
 	 * was an error */
-	err = genphy_update_link(phydev);
-	if (err)
-		return err;
+	page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
+	if (page == MII_M1111_FIBER)
+		fiber = 1;
+	else
+		fiber = 0;
+
+	err = marvell_update_link(phydev);
 
 	if (AUTONEG_ENABLE == phydev->autoneg) {
 		status = phy_read(phydev, MII_M1011_PHY_STATUS);
@@ -930,9 +1099,6 @@ static int marvell_read_status(struct phy_device *phydev)
 		if (adv < 0)
 			return adv;
 
-		phydev->lp_advertising = mii_stat1000_to_ethtool_lpa_t(lpagb) |
-					 mii_lpa_to_ethtool_lpa_t(lpa);
-
 		lpa &= adv;
 
 		if (status & MII_M1011_PHY_STATUS_FULLDUPLEX)
@@ -957,9 +1123,30 @@ static int marvell_read_status(struct phy_device *phydev)
 			break;
 		}
 
-		if (phydev->duplex == DUPLEX_FULL) {
-			phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0;
-			phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0;
+		if (!fiber) {
+			phydev->lp_advertising = mii_stat1000_to_ethtool_lpa_t(lpagb) |
+					 mii_lpa_to_ethtool_lpa_t(lpa);
+
+			if (phydev->duplex == DUPLEX_FULL) {
+				phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0;
+				phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0;
+			}
+		} else {
+			/* The fiber link is only 1000M capable */
+			phydev->lp_advertising = fiber_lpa_to_ethtool_lpa_t(lpa);
+
+			if (phydev->duplex == DUPLEX_FULL) {
+				if (!(lpa & LPA_PAUSE_FIBER)) {
+					phydev->pause = 0;
+					phydev->asym_pause = 0;
+				} else if ((lpa & LPA_PAUSE_ASYM_FIBER)) {
+					phydev->pause = 1;
+					phydev->asym_pause = 1;
+				} else {
+					phydev->pause = 1;
+					phydev->asym_pause = 0;
+				}
+			}
 		}
 	} else {
 		int bmcr = phy_read(phydev, MII_BMCR);
@@ -986,6 +1173,105 @@ static int marvell_read_status(struct phy_device *phydev)
 	return 0;
 }
 
+/* marvell_read_fiber_status
+ *
+ * Some Marvell's phys have two modes: fiber and copper.
+ * Both need status checked.
+ * Description:
+ *   First, check the fiber link and status.
+ *   If the fiber link is down, check the copper link and status which
+ *   will be the default value if both link are down.
+ */
+static int marvell_read_fiber_status(struct phy_device *phydev)
+{
+	int err;
+
+	/* Check the fiber mode first */
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
+	if (err < 0)
+		goto error;
+
+	err = marvell_read_status(phydev);
+	if (err < 0)
+		goto error;
+
+	if (phydev->link) {
+		phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+		return 0;
+	}
+
+	/* If fiber link is down, check and save copper mode state */
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+	if (err < 0)
+		goto error;
+
+	return marvell_read_status(phydev);
+
+error:
+	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+	return err;
+}
+
+/* marvell_suspend_fiber
+ *
+ * Some Marvell's phys have two modes: fiber and copper.
+ * Both need to be suspended
+ */
+static int marvell_suspend_fiber(struct phy_device *phydev)
+{
+	int err;
+
+	/* Suspend the fiber mode first */
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
+	if (err < 0)
+		goto error;
+
+	err = genphy_suspend(phydev);
+	if (err < 0)
+		goto error;
+
+	/* Then, the copper link */
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+	if (err < 0)
+		goto error;
+
+	return genphy_suspend(phydev);
+
+error:
+	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+	return err;
+}
+
+/* marvell_resume_fiber
+ *
+ * Some Marvell's phys have two modes: fiber and copper.
+ * Both need to be resumed
+ */
+static int marvell_resume_fiber(struct phy_device *phydev)
+{
+	int err;
+
+	/* Resume the fiber mode first */
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
+	if (err < 0)
+		goto error;
+
+	err = genphy_resume(phydev);
+	if (err < 0)
+		goto error;
+
+	/* Then, the copper link */
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+	if (err < 0)
+		goto error;
+
+	return genphy_resume(phydev);
+
+error:
+	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
+	return err;
+}
+
 static int marvell_aneg_done(struct phy_device *phydev)
 {
 	int retval = phy_read(phydev, MII_M1011_PHY_STATUS);
@@ -1130,6 +1416,11 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
 	int err, oldpage, val;
 	u64 ret;
 
+	if (!(phydev->supported & SUPPORTED_FIBRE)) {
+		if (strstr(marvell_hw_stats[i].string, "fiber"))
+			return 0;
+	}
+
 	oldpage = phy_read(phydev, MII_MARVELL_PHY_PAGE);
 	err = phy_write(phydev, MII_MARVELL_PHY_PAGE,
 			stat.page);
@@ -1361,17 +1652,17 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E1510,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1510",
-		.features = PHY_GBIT_FEATURES,
+		.features = PHY_GBIT_FEATURES | SUPPORTED_FIBRE,
 		.flags = PHY_HAS_INTERRUPT,
 		.probe = marvell_probe,
 		.config_init = &m88e1510_config_init,
 		.config_aneg = &m88e1510_config_aneg,
-		.read_status = &marvell_read_status,
+		.read_status = &marvell_read_fiber_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
-		.resume = &genphy_resume,
-		.suspend = &genphy_suspend,
+		.resume = &marvell_resume_fiber,
+		.suspend = &marvell_suspend_fiber,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
-- 
2.7.4

Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Andrew Lunn]

On Tue, Jul 12, 2016 at 05:00:52PM +0200, Charles-Antoine Couret wrote:
> Hello,
> I'm back with another patch evrsion about Marvell phys with a fiber interface.
> >From the previous release, I fixed some issues reported by yours and I added some functions around the fiber interface to get statistics, to configure the aneg, etc.

Hi Charles

It is best to submit a number of smaller patches, each doing one
thing, than a single big patch. It makes review and discussion much
simpler.

So for example, this should be a patch of its own:

> @@ -151,6 +165,7 @@ struct marvell_hw_stat {
>  
>  static struct marvell_hw_stat marvell_hw_stats[] = {
>  	{ "phy_receive_errors", 0, 21, 16},
> +	{ "phy_receive_errors_fiber", 1, 21, 16},
>  	{ "phy_idle_errors", 0, 10, 8 },
>  };

I think we should also rename phy_receive_errors to
phy_receive_errors_copper.

	Andrew

Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Charles-Antoine Couret]

Le 12/07/2016 à 17:18, Andrew Lunn a écrit :
> Hi Charles
It's Charles-Antoine. ;)

> 
> It is best to submit a number of smaller patches, each doing one
> thing, than a single big patch. It makes review and discussion much
> simpler.
I'm sorry, I will fix that.


> So for example, this should be a patch of its own:
> 
>> @@ -151,6 +165,7 @@ struct marvell_hw_stat {
>>  
>>  static struct marvell_hw_stat marvell_hw_stats[] = {
>>  	{ "phy_receive_errors", 0, 21, 16},
>> +	{ "phy_receive_errors_fiber", 1, 21, 16},
>>  	{ "phy_idle_errors", 0, 10, 8 },
>>  };
> 
> I think we should also rename phy_receive_errors to
> phy_receive_errors_copper.

I agree with you. I will fix that too.
Thanks.
Charles-Antoine Couret

Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Andrew Lunn]

> +#define LPA_FIBER_1000HALF	0x40
> +#define LPA_FIBER_1000FULL	0x20
> +
> +#define LPA_PAUSE_FIBER		0x180
> +#define LPA_PAUSE_ASYM_FIBER	0x100
> +
> +#define ADVERTISE_FIBER_1000HALF	0x40
> +#define ADVERTISE_FIBER_1000FULL	0x20
> +
> +#define ADVERTISE_PAUSE_FIBER		0x180
> +#define ADVERTISE_PAUSE_ASYM_FIBER	0x100

Are these standardised anywhere? If they are following a standard,
they should be put into include/uapi/linux/mii.h.

> +static inline u32 ethtool_adv_to_fiber_adv_t(u32 ethadv)
> +{
> +	u32 result = 0;
> +
> +	if (ethadv & ADVERTISED_1000baseT_Half)
> +		result |= ADVERTISE_FIBER_1000HALF;

Dumb question: Does 1000baseT_Half even make sense for fibre? Can you
do half duplex?  Would that not mean you have a single fibre, both
ends are using the same laser frequency, and you are doing some form
of CSMA/CD?

> +	if (ethadv & ADVERTISED_1000baseT_Full)
> +		result |= ADVERTISE_FIBER_1000FULL;
> +
> +	if ((ethadv & ADVERTISE_PAUSE_ASYM) && (ethadv & ADVERTISE_PAUSE_CAP))
> +		result |= LPA_PAUSE_ASYM_FIBER;
> +	else if (ethadv & ADVERTISE_PAUSE_CAP)
> +		result |= (ADVERTISE_PAUSE_FIBER
> +			   & (~ADVERTISE_PAUSE_ASYM_FIBER));
> +
> +	return result;
> +}

If these values are standardised, i think this function should be
moved into the generic code. If however, this is Marvell specific,
keep it here.

>   *
>   * Generic status code does not detect Fiber correctly!
> @@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device *phydev)
>  	int lpa;
>  	int lpagb;
>  	int status = 0;
> +	int page, fiber;
>  
> -	/* Update the link, but return if there
> +	/* Detect and update the link, but return if there
>  	 * was an error */
> -	err = genphy_update_link(phydev);
> -	if (err)
> -		return err;
> +	page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> +	if (page == MII_M1111_FIBER)
> +		fiber = 1;
> +	else
> +		fiber = 0;

This read is expensive, since the MDIO bus is slow. It would be better
just to pass fibre as a parameter.

> +/* marvell_read_fiber_status
> + *
> + * Some Marvell's phys have two modes: fiber and copper.
> + * Both need status checked.
> + * Description:
> + *   First, check the fiber link and status.
> + *   If the fiber link is down, check the copper link and status which
> + *   will be the default value if both link are down.
> + */
> +static int marvell_read_fiber_status(struct phy_device *phydev)

The name is a bit confusing. I would probably use
marvell_read_copper_fiber_status() making it clear it reads both.

> +{
> +	int err;
> +
> +	/* Check the fiber mode first */
> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> +	if (err < 0)
> +		goto error;
> +
> +	err = marvell_read_status(phydev);
> +	if (err < 0)
> +		goto error;
> +
> +	if (phydev->link) {
> +		phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +		return 0;
> +	}
> +
> +	/* If fiber link is down, check and save copper mode state */
> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +	if (err < 0)
> +		goto error;

There should be a big fat comment somewhere that after this function
the copper or fibre page is left selected, depending on which has
link.  There is a danger somebody misses this assumption and breaks
the code by unconditionally restoring to copper.

> +
> +	return marvell_read_status(phydev);
> +
> +error:
> +	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +	return err;
> +}
> +
> +/* marvell_suspend_fiber
> + *
> + * Some Marvell's phys have two modes: fiber and copper.
> + * Both need to be suspended
> + */
> +static int marvell_suspend_fiber(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	/* Suspend the fiber mode first */
> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> +	if (err < 0)
> +		goto error;
> +
> +	err = genphy_suspend(phydev);
> +	if (err < 0)
> +		goto error;
> +
> +	/* Then, the copper link */
> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +	if (err < 0)
> +		goto error;
> +
> +	return genphy_suspend(phydev);
> +
> +error:
> +	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +	return err;
> +}

I think it would be better to look for SUPPORTED_FIBRE in
drv->features, rather than have two different functions.

In fact, i would do that in general, rather than add your _fibre()
functions.

> +
> +/* marvell_resume_fiber
> + *
> + * Some Marvell's phys have two modes: fiber and copper.
> + * Both need to be resumed
> + */
> +static int marvell_resume_fiber(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	/* Resume the fiber mode first */
> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> +	if (err < 0)
> +		goto error;
> +
> +	err = genphy_resume(phydev);
> +	if (err < 0)
> +		goto error;
> +
> +	/* Then, the copper link */
> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> +	if (err < 0)
> +		goto error;
> +
> +	return genphy_resume(phydev);

Should it be resumed twice? Or just once at the end?  Same question
for suspend.

> @@ -1130,6 +1416,11 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
>  	int err, oldpage, val;
>  	u64 ret;
>  
> +	if (!(phydev->supported & SUPPORTED_FIBRE)) {
> +		if (strstr(marvell_hw_stats[i].string, "fiber"))
> +			return 0;

I think a better solution is for marvell_get_sset_count() to return 2
or 3 depending on phydev->supported & SUPPORTED_FIBRE.

   Andrew

Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Charles-Antoine Couret]

Le 12/07/2016 à 17:57, Andrew Lunn a écrit :
>> +#define LPA_FIBER_1000HALF	0x40
>> +#define LPA_FIBER_1000FULL	0x20
>> +
>> +#define LPA_PAUSE_FIBER		0x180
>> +#define LPA_PAUSE_ASYM_FIBER	0x100
>> +
>> +#define ADVERTISE_FIBER_1000HALF	0x40
>> +#define ADVERTISE_FIBER_1000FULL	0x20
>> +
>> +#define ADVERTISE_PAUSE_FIBER		0x180
>> +#define ADVERTISE_PAUSE_ASYM_FIBER	0x100
> 
> Are these standardised anywhere? If they are following a standard,
> they should be put into include/uapi/linux/mii.h.

 [snip]

>> +	if (ethadv & ADVERTISED_1000baseT_Full)
>> +		result |= ADVERTISE_FIBER_1000FULL;
>> +
>> +	if ((ethadv & ADVERTISE_PAUSE_ASYM) && (ethadv & ADVERTISE_PAUSE_CAP))
>> +		result |= LPA_PAUSE_ASYM_FIBER;
>> +	else if (ethadv & ADVERTISE_PAUSE_CAP)
>> +		result |= (ADVERTISE_PAUSE_FIBER
>> +			   & (~ADVERTISE_PAUSE_ASYM_FIBER));
>> +
>> +	return result;
>> +}
> 
> If these values are standardised, i think this function should be
> moved into the generic code. If however, this is Marvell specific,
> keep it here.
> 

I don't find any standard about this, I think it should be Marvell specific.

>> +static inline u32 ethtool_adv_to_fiber_adv_t(u32 ethadv)
>> +{
>> +	u32 result = 0;
>> +
>> +	if (ethadv & ADVERTISED_1000baseT_Half)
>> +		result |= ADVERTISE_FIBER_1000HALF;
> 
> Dumb question: Does 1000baseT_Half even make sense for fibre? Can you
> do half duplex?  Would that not mean you have a single fibre, both
> ends are using the same laser frequency, and you are doing some form
> of CSMA/CD?

It's strange, I agree, but the register about that exists in the datasheet and the value is not fixed.
In practice, I don't have a component to test this case correctly.
 

>>   *
>>   * Generic status code does not detect Fiber correctly!
>> @@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device *phydev)
>>  	int lpa;
>>  	int lpagb;
>>  	int status = 0;
>> +	int page, fiber;
>>  
>> -	/* Update the link, but return if there
>> +	/* Detect and update the link, but return if there
>>  	 * was an error */
>> -	err = genphy_update_link(phydev);
>> -	if (err)
>> -		return err;
>> +	page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
>> +	if (page == MII_M1111_FIBER)
>> +		fiber = 1;
>> +	else
>> +		fiber = 0;
> 
> This read is expensive, since the MDIO bus is slow. It would be better
> just to pass fibre as a parameter.

But this function is used for other Marvell's phy, without fiber link for example.
And this function should has only the struct phy_device as parameter.

I don't have idea to avoid that, without create a custom function for that which would be very similar to this function.
Or used a phy_device field for that? I think it's awful idea...

>> +/* marvell_read_fiber_status
>> + *
>> + * Some Marvell's phys have two modes: fiber and copper.
>> + * Both need status checked.
>> + * Description:
>> + *   First, check the fiber link and status.
>> + *   If the fiber link is down, check the copper link and status which
>> + *   will be the default value if both link are down.
>> + */
>> +static int marvell_read_fiber_status(struct phy_device *phydev)
> 
> The name is a bit confusing. I would probably use
> marvell_read_copper_fiber_status() making it clear it reads both.

You're right.

>> +
>> +	return marvell_read_status(phydev);
>> +
>> +error:
>> +	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
>> +	return err;
>> +}
>> +
>> +/* marvell_suspend_fiber
>> + *
>> + * Some Marvell's phys have two modes: fiber and copper.
>> + * Both need to be suspended
>> + */
>> +static int marvell_suspend_fiber(struct phy_device *phydev)
>> +{
>> +	int err;
>> +
>> +	/* Suspend the fiber mode first */
>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
>> +	if (err < 0)
>> +		goto error;
>> +
>> +	err = genphy_suspend(phydev);
>> +	if (err < 0)
>> +		goto error;
>> +
>> +	/* Then, the copper link */
>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
>> +	if (err < 0)
>> +		goto error;
>> +
>> +	return genphy_suspend(phydev);
>> +
>> +error:
>> +	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
>> +	return err;
>> +}
> 
> I think it would be better to look for SUPPORTED_FIBRE in
> drv->features, rather than have two different functions.
> 
> In fact, i would do that in general, rather than add your _fibre()
> functions.

So, you suggest to do that in genphy_* functions or create marvell_* functions with this condition?
I'm agree with the second suggestion.

>> +
>> +/* marvell_resume_fiber
>> + *
>> + * Some Marvell's phys have two modes: fiber and copper.
>> + * Both need to be resumed
>> + */
>> +static int marvell_resume_fiber(struct phy_device *phydev)
>> +{
>> +	int err;
>> +
>> +	/* Resume the fiber mode first */
>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
>> +	if (err < 0)
>> +		goto error;
>> +
>> +	err = genphy_resume(phydev);
>> +	if (err < 0)
>> +		goto error;
>> +
>> +	/* Then, the copper link */
>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
>> +	if (err < 0)
>> +		goto error;
>> +
>> +	return genphy_resume(phydev);
> 
> Should it be resumed twice? Or just once at the end?  Same question
> for suspend.

I don't understand your question.
Each interface are resumed / suspended once by these functions.

> 
>> @@ -1130,6 +1416,11 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
>>  	int err, oldpage, val;
>>  	u64 ret;
>>  
>> +	if (!(phydev->supported & SUPPORTED_FIBRE)) {
>> +		if (strstr(marvell_hw_stats[i].string, "fiber"))
>> +			return 0;
> 
> I think a better solution is for marvell_get_sset_count() to return 2
> or 3 depending on phydev->supported & SUPPORTED_FIBRE.

Ok.

Thanks for all of your comments, I will fix that. However, I need some additional answers to do that entirely. :)

Regards.
Charles-Antoine Couret

Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Andrew Lunn]

On Wed, Jul 13, 2016 at 11:14:21AM +0200, Charles-Antoine Couret wrote:


Hi Charles-Antoine
 
> >> +#define LPA_FIBER_1000HALF	0x40
> >> +#define LPA_FIBER_1000FULL	0x20
> >> +
> >> +#define LPA_PAUSE_FIBER		0x180
> >> +#define LPA_PAUSE_ASYM_FIBER	0x100
> >> +
> >> +#define ADVERTISE_FIBER_1000HALF	0x40
> >> +#define ADVERTISE_FIBER_1000FULL	0x20
> >> +
> >> +#define ADVERTISE_PAUSE_FIBER		0x180
> >> +#define ADVERTISE_PAUSE_ASYM_FIBER	0x100
> > 
> > Are these standardised anywhere? If they are following a standard,
> > they should be put into include/uapi/linux/mii.h.

> I don't find any standard about this, I think it should be Marvell specific.

O.K.

> >> +static inline u32 ethtool_adv_to_fiber_adv_t(u32 ethadv)
> >> +{
> >> +	u32 result = 0;
> >> +
> >> +	if (ethadv & ADVERTISED_1000baseT_Half)
> >> +		result |= ADVERTISE_FIBER_1000HALF;
> > 
> > Dumb question: Does 1000baseT_Half even make sense for fibre? Can you
> > do half duplex?  Would that not mean you have a single fibre, both
> > ends are using the same laser frequency, and you are doing some form
> > of CSMA/CD?
> 
> It's strange, I agree, but the register about that exists in the datasheet and the value is not fixed.
> In practice, I don't have a component to test this case correctly.

O.K, just implement it according to the data sheet.
 
> >>   *
> >>   * Generic status code does not detect Fiber correctly!
> >> @@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device *phydev)
> >>  	int lpa;
> >>  	int lpagb;
> >>  	int status = 0;
> >> +	int page, fiber;
> >>  
> >> -	/* Update the link, but return if there
> >> +	/* Detect and update the link, but return if there
> >>  	 * was an error */
> >> -	err = genphy_update_link(phydev);
> >> -	if (err)
> >> -		return err;
> >> +	page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> >> +	if (page == MII_M1111_FIBER)
> >> +		fiber = 1;
> >> +	else
> >> +		fiber = 0;
> > 
> > This read is expensive, since the MDIO bus is slow. It would be better
> > just to pass fibre as a parameter.
> 
> But this function is used for other Marvell's phy, without fiber link for example.
> And this function should has only the struct phy_device as parameter.
> 
> I don't have idea to avoid that, without create a custom function for that which would be very similar to this function.
> Or used a phy_device field for that? I think it's awful idea...

So i would have

static int marvell_read_status_page(struct phy_device *phydev, int page)
{}

basically doing what you have above, but without the read.

static int marvell_read_status(struct phy_device *phydev)
{
	if (phydev->supported & SUPPORTED_FIBRE) {
		marvell_read_status_page(phydev, MII_M1111_FIBER);
	   	if (phydev->link)
	      	return;

	return marvell_read_status_page(phydev, MII_M1111_COPPER);
}

> > I think it would be better to look for SUPPORTED_FIBRE in
> > drv->features, rather than have two different functions.
> > 
> > In fact, i would do that in general, rather than add your _fibre()
> > functions.
> 
> So, you suggest to do that in genphy_* functions or create marvell_* functions with this condition?
> I'm agree with the second suggestion.

The second.

> 
> >> +
> >> +/* marvell_resume_fiber
> >> + *
> >> + * Some Marvell's phys have two modes: fiber and copper.
> >> + * Both need to be resumed
> >> + */
> >> +static int marvell_resume_fiber(struct phy_device *phydev)
> >> +{
> >> +	int err;
> >> +
> >> +	/* Resume the fiber mode first */
> >> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> >> +	if (err < 0)
> >> +		goto error;
> >> +
> >> +	err = genphy_resume(phydev);
> >> +	if (err < 0)
> >> +		goto error;
> >> +
> >> +	/* Then, the copper link */
> >> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> >> +	if (err < 0)
> >> +		goto error;
> >> +
> >> +	return genphy_resume(phydev);
> > 
> > Should it be resumed twice? Or just once at the end?  Same question
> > for suspend.
> 
> I don't understand your question.

You call genphy_resume(phydev) twice. Once is sufficient.

    Andrew

Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Charles-Antoine Couret]

Hi Andrew,

Le 13/07/2016 à 15:26, Andrew Lunn a écrit :
>>>>   *
>>>>   * Generic status code does not detect Fiber correctly!
>>>> @@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device *phydev)
>>>>  	int lpa;
>>>>  	int lpagb;
>>>>  	int status = 0;
>>>> +	int page, fiber;
>>>>  
>>>> -	/* Update the link, but return if there
>>>> +	/* Detect and update the link, but return if there
>>>>  	 * was an error */
>>>> -	err = genphy_update_link(phydev);
>>>> -	if (err)
>>>> -		return err;
>>>> +	page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
>>>> +	if (page == MII_M1111_FIBER)
>>>> +		fiber = 1;
>>>> +	else
>>>> +		fiber = 0;
>>>
>>> This read is expensive, since the MDIO bus is slow. It would be better
>>> just to pass fibre as a parameter.
>>
>> But this function is used for other Marvell's phy, without fiber link for example.
>> And this function should has only the struct phy_device as parameter.
>>
>> I don't have idea to avoid that, without create a custom function for that which would be very similar to this function.
>> Or used a phy_device field for that? I think it's awful idea...
> 
> So i would have
> 
> static int marvell_read_status_page(struct phy_device *phydev, int page)
> {}
> 
> basically doing what you have above, but without the read.
> 
> static int marvell_read_status(struct phy_device *phydev)
> {
> 	if (phydev->supported & SUPPORTED_FIBRE) {
> 		marvell_read_status_page(phydev, MII_M1111_FIBER);
> 	   	if (phydev->link)
> 	      	return;
> 
> 	return marvell_read_status_page(phydev, MII_M1111_COPPER);
> }

Oh I see. Thank you!

> 
>>> I think it would be better to look for SUPPORTED_FIBRE in
>>> drv->features, rather than have two different functions.
>>>
>>> In fact, i would do that in general, rather than add your _fibre()
>>> functions.
>>
>> So, you suggest to do that in genphy_* functions or create marvell_* functions with this condition?
>> I'm agree with the second suggestion.
> 
> The second.

I'm working on this.
It's done for _resume and _suspend. It will be done for _status.

But, for aneg or ethtool concerned, I think adding these functions is better.

>>>> +
>>>> +/* marvell_resume_fiber
>>>> + *
>>>> + * Some Marvell's phys have two modes: fiber and copper.
>>>> + * Both need to be resumed
>>>> + */
>>>> +static int marvell_resume_fiber(struct phy_device *phydev)
>>>> +{
>>>> +	int err;
>>>> +
>>>> +	/* Resume the fiber mode first */
>>>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
>>>> +	if (err < 0)
>>>> +		goto error;
>>>> +
>>>> +	err = genphy_resume(phydev);
>>>> +	if (err < 0)
>>>> +		goto error;
>>>> +
>>>> +	/* Then, the copper link */
>>>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
>>>> +	if (err < 0)
>>>> +		goto error;
>>>> +
>>>> +	return genphy_resume(phydev);
>>>
>>> Should it be resumed twice? Or just once at the end?  Same question
>>> for suspend.
>>
>> I don't understand your question.
> 
> You call genphy_resume(phydev) twice. Once is sufficient.

Yes, but it's normal because each interface could be suspended or resumed independently.
genphy_* functions use BMCR register which are identical between fiber and copper link. But each link has its own register to change.

Thank you.
Regards.

Charles-Antoine Couret

Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Andrew Lunn]

> >>>> +static int marvell_resume_fiber(struct phy_device *phydev)
> >>>> +{
> >>>> +	int err;
> >>>> +
> >>>> +	/* Resume the fiber mode first */
> >>>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> >>>> +	if (err < 0)
> >>>> +		goto error;
> >>>> +
> >>>> +	err = genphy_resume(phydev);
> >>>> +	if (err < 0)
> >>>> +		goto error;
> >>>> +
> >>>> +	/* Then, the copper link */
> >>>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> >>>> +	if (err < 0)
> >>>> +		goto error;
> >>>> +
> >>>> +	return genphy_resume(phydev);
> >>>
> >>> Should it be resumed twice? Or just once at the end?  Same question
> >>> for suspend.
> >>
> >> I don't understand your question.
> > 
> > You call genphy_resume(phydev) twice. Once is sufficient.
> 
> Yes, but it's normal because each interface could be suspended or resumed independently.

> genphy_* functions use BMCR register which are identical between
> fiber and copper link. But each link has its own register to change.

Ah! Now i get it. I think you need a comment here. Something like:

    /* With the page set, use the generic resume */

What i was worried about is that there is some reference counting
going on inside these functions. And so suspending the same phydev
multiple times will mess up the reference counts. But no, it just
twiddles a register bit, so that is O.K.

	 Andrew