[PATCH net 0/2] Use C45 Helpers when possible

[PATCH net 0/2] Use C45 Helpers when possible

[Jose Abreu]

For -net. Please review!

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>

Jose Abreu (2):
  net: phy: Use C45 Helpers in phy_read_status()
  net: phy: Use C45 Helpers in PHY_FORCING state

 drivers/net/phy/phy.c |  2 +-
 include/linux/phy.h   | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH net 1/2] net: phy: Use C45 Helpers in phy_read_status()

[Jose Abreu]

Currently phy_read_status() considers that either the PHY driver has the
read_status() callback or uses the generic callback.

For C45 PHYs we need to use the gen10g_read_status() callback.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>
---
 include/linux/phy.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 333b56d8f746..872899136fdc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1030,6 +1030,8 @@ static inline int phy_read_status(struct phy_device *phydev)
 
 	if (phydev->drv->read_status)
 		return phydev->drv->read_status(phydev);
+	else if (phydev->is_c45)
+		return gen10g_read_status(phydev);
 	else
 		return genphy_read_status(phydev);
 }
-- 
2.7.4

Re: [PATCH net 1/2] net: phy: Use C45 Helpers in phy_read_status()

[Florian Fainelli]

On 3/1/2019 2:54 AM, Jose Abreu wrote:
> Currently phy_read_status() considers that either the PHY driver has the
> read_status() callback or uses the generic callback.
> 
> For C45 PHYs we need to use the gen10g_read_status() callback.

Right, so we could expect your C45 PHY driver to assign the read_status
callback to gen10g_read_status() if it is appropriate. So far most of
the 10g PHY drivers (cortina, marvell10g, aquantia) have to define their
own read_status() callback to be feature complete. Unlike C22 PHYs that
can really be driven with a simple generic PHY driver model for standard
features, C45 PHYs seem to be quirky enough this does not work anymore.

> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Joao Pinto <joao.pinto@synopsys.com>
> ---
>  include/linux/phy.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 333b56d8f746..872899136fdc 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1030,6 +1030,8 @@ static inline int phy_read_status(struct phy_device *phydev)
>  
>  	if (phydev->drv->read_status)
>  		return phydev->drv->read_status(phydev);
> +	else if (phydev->is_c45)
> +		return gen10g_read_status(phydev);
>  	else
>  		return genphy_read_status(phydev);
>  }
> 

-- 
Florian

Re: [PATCH net 1/2] net: phy: Use C45 Helpers in phy_read_status()

[Andrew Lunn]

On Fri, Mar 01, 2019 at 07:14:20PM -0800, Florian Fainelli wrote:
> 
> 
> On 3/1/2019 2:54 AM, Jose Abreu wrote:
> > Currently phy_read_status() considers that either the PHY driver has the
> > read_status() callback or uses the generic callback.
> > 
> > For C45 PHYs we need to use the gen10g_read_status() callback.
> 
> Right, so we could expect your C45 PHY driver to assign the read_status
> callback to gen10g_read_status() if it is appropriate. So far most of
> the 10g PHY drivers (cortina, marvell10g, aquantia) have to define their
> own read_status() callback to be feature complete. Unlike C22 PHYs that
> can really be driven with a simple generic PHY driver model for standard
> features, C45 PHYs seem to be quirky enough this does not work anymore.

Hi Jose

Does your PHY support 1000Base-T? If so you need read_status() because
the registers for that link mode don't appear to be standardized.

    Andrew

Re: [PATCH net 1/2] net: phy: Use C45 Helpers in phy_read_status()

[Heiner Kallweit]

On 02.03.2019 15:11, Andrew Lunn wrote:
> On Fri, Mar 01, 2019 at 07:14:20PM -0800, Florian Fainelli wrote:
>>
>>
>> On 3/1/2019 2:54 AM, Jose Abreu wrote:
>>> Currently phy_read_status() considers that either the PHY driver has the
>>> read_status() callback or uses the generic callback.
>>>
>>> For C45 PHYs we need to use the gen10g_read_status() callback.
>>
The gen10g_ functions are deprecated and shouldn't be used in new code.
Consider the (partially brand-new) genphy_c45_ functions instead.
This should be ok because I think the two changes are material for
net-next.

The gen10g functions belong to the old gen10g driver which knows about
10G only. I think sooner or later we'll replace it with a genc45
driver or similar.

>> Right, so we could expect your C45 PHY driver to assign the read_status
>> callback to gen10g_read_status() if it is appropriate. So far most of
>> the 10g PHY drivers (cortina, marvell10g, aquantia) have to define their
>> own read_status() callback to be feature complete. Unlike C22 PHYs that
>> can really be driven with a simple generic PHY driver model for standard
>> features, C45 PHYs seem to be quirky enough this does not work anymore.
> 
> Hi Jose
> 
> Does your PHY support 1000Base-T? If so you need read_status() because
> the registers for that link mode don't appear to be standardized.
> 
>     Andrew
> 
Heiner

[PATCH net 2/2] net: phy: Use C45 Helpers in PHY_FORCING state

[Jose Abreu]

When using a C45 PHY that is in PHY_FORCING state we are currently not
taking into account that this kind of PHY has different update_link
functions.

Use the C45 Helpers instead or the driver built-in read_status() helper,
if possible.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>
---
 drivers/net/phy/phy.c |  2 +-
 include/linux/phy.h   | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c5675df5fc6f..1ef5dbc384d0 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -940,7 +940,7 @@ void phy_state_machine(struct work_struct *work)
 		err = phy_check_link_status(phydev);
 		break;
 	case PHY_FORCING:
-		err = genphy_update_link(phydev);
+		err = phy_update_link(phydev);
 		if (err)
 			break;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 872899136fdc..67caa7eb93b1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1036,6 +1036,19 @@ static inline int phy_read_status(struct phy_device *phydev)
 		return genphy_read_status(phydev);
 }
 
+static inline int phy_update_link(struct phy_device *phydev)
+{
+	if (!phydev->drv)
+		return -EIO;
+
+	if (phydev->drv->read_status)
+		return phydev->drv->read_status(phydev);
+	else if (phydev->is_c45)
+		return gen10g_read_status(phydev);
+	else
+		return genphy_update_link(phydev);
+}
+
 void phy_driver_unregister(struct phy_driver *drv);
 void phy_drivers_unregister(struct phy_driver *drv, int n);
 int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
-- 
2.7.4

Re: [PATCH net 2/2] net: phy: Use C45 Helpers in PHY_FORCING state

[Andrew Lunn]

On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote:
> +static inline int phy_update_link(struct phy_device *phydev)
> +{
> +	if (!phydev->drv)
> +		return -EIO;
> +
> +	if (phydev->drv->read_status)
> +		return phydev->drv->read_status(phydev);
> +	else if (phydev->is_c45)
> +		return gen10g_read_status(phydev);
> +	else
> +		return genphy_update_link(phydev);
> +}

Hi Jose

The asymmetry here could be an issue.  We might fall into the trap
that a c45 PHY has the full state in phydev updated, were as a c22
only has the link updated. Somebody testing on C45 might miss a bug
for a C22 device.

Maybe this should be called phy_read_state(), and calls
genphy_read_status() not genphy_update_link().

     Andrew

Re: [PATCH net 2/2] net: phy: Use C45 Helpers in PHY_FORCING state

[Jose Abreu]

Hi Andrew,

On 3/1/2019 1:53 PM, Andrew Lunn wrote:
> On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote:
>> +static inline int phy_update_link(struct phy_device *phydev)
>> +{
>> +	if (!phydev->drv)
>> +		return -EIO;
>> +
>> +	if (phydev->drv->read_status)
>> +		return phydev->drv->read_status(phydev);
>> +	else if (phydev->is_c45)
>> +		return gen10g_read_status(phydev);
>> +	else
>> +		return genphy_update_link(phydev);
>> +}
> 
> Hi Jose
> 
> The asymmetry here could be an issue.  We might fall into the trap
> that a c45 PHY has the full state in phydev updated, were as a c22
> only has the link updated. Somebody testing on C45 might miss a bug
> for a C22 device.

Notice that this phy_update_link() is called from PHY_FORCING
state which in my case happens when autoneg is not enabled / is
not supported.

I think it makes sense, in this case, to only update link status,
no ?

Thanks,
Jose Miguel Abreu

> 
> Maybe this should be called phy_read_state(), and calls
> genphy_read_status() not genphy_update_link().
> 
>      Andrew
> 

Re: [PATCH net 2/2] net: phy: Use C45 Helpers in PHY_FORCING state

[Andrew Lunn]

On Mon, Mar 04, 2019 at 03:07:24PM +0000, Jose Abreu wrote:
> Hi Andrew,
> 
> On 3/1/2019 1:53 PM, Andrew Lunn wrote:
> > On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote:
> >> +static inline int phy_update_link(struct phy_device *phydev)
> >> +{
> >> +	if (!phydev->drv)
> >> +		return -EIO;
> >> +
> >> +	if (phydev->drv->read_status)
> >> +		return phydev->drv->read_status(phydev);
> >> +	else if (phydev->is_c45)
> >> +		return gen10g_read_status(phydev);
> >> +	else
> >> +		return genphy_update_link(phydev);
> >> +}
> > 
> > Hi Jose
> > 
> > The asymmetry here could be an issue.  We might fall into the trap
> > that a c45 PHY has the full state in phydev updated, were as a c22
> > only has the link updated. Somebody testing on C45 might miss a bug
> > for a C22 device.
> 
> Notice that this phy_update_link() is called from PHY_FORCING
> state which in my case happens when autoneg is not enabled / is
> not supported.
> 
> I think it makes sense, in this case, to only update link status,
> no ?
 
Hi Jose

It is actually quite difficult to determine when the link is up. I
personally would not trust gen10g_read_status() to get this right, and
would always implement the read_status callback.

Which PHY driver are you using, which does not support
read_status(). All the mainline PHY drivers do seem to have
read_status implemented.

    Andrew

Re: [PATCH net 2/2] net: phy: Use C45 Helpers in PHY_FORCING state

[Heiner Kallweit]

On 04.03.2019 16:44, Andrew Lunn wrote:
> On Mon, Mar 04, 2019 at 03:07:24PM +0000, Jose Abreu wrote:
>> Hi Andrew,
>>
>> On 3/1/2019 1:53 PM, Andrew Lunn wrote:
>>> On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote:
>>>> +static inline int phy_update_link(struct phy_device *phydev)
>>>> +{
>>>> +	if (!phydev->drv)
>>>> +		return -EIO;
>>>> +
>>>> +	if (phydev->drv->read_status)
>>>> +		return phydev->drv->read_status(phydev);
>>>> +	else if (phydev->is_c45)
>>>> +		return gen10g_read_status(phydev);
>>>> +	else
>>>> +		return genphy_update_link(phydev);
>>>> +}
>>>
>>> Hi Jose
>>>
>>> The asymmetry here could be an issue.  We might fall into the trap
>>> that a c45 PHY has the full state in phydev updated, were as a c22
>>> only has the link updated. Somebody testing on C45 might miss a bug
>>> for a C22 device.
>>
>> Notice that this phy_update_link() is called from PHY_FORCING
>> state which in my case happens when autoneg is not enabled / is
>> not supported.
>>
>> I think it makes sense, in this case, to only update link status,
>> no ?
>  
> Hi Jose
> 
> It is actually quite difficult to determine when the link is up. I
> personally would not trust gen10g_read_status() to get this right, and
> would always implement the read_status callback.
> 
Not to forget that we just stopped exporting gen10g_read_status().
genphy_c45_read_link() seems to be the right function to be used
in phy_update_link().

> Which PHY driver are you using, which does not support
> read_status(). All the mainline PHY drivers do seem to have
> read_status implemented.
> 
>     Andrew
> 
Heiner

Re: [PATCH net 0/2] Use C45 Helpers when possible

[Andrew Lunn]

On Fri, Mar 01, 2019 at 11:54:22AM +0100, Jose Abreu wrote:
> For -net. Please review!

Hi Jose

Patches for net should have fixes: tags, making it clear what they
fix, and how far back the patches should be ported.

Thanks
	Andrew