[PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth

[PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth

[Alexey Brodkin]

Given there're default values mentioned in the PHY datasheet
fall-back gracefully to them instead of silently return an error
through the whole call-chain.

This allows to use minimalistic description in DT if no special
features are required.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Murali Karicheri <m-karicheri2@ti.com>
Cc: Sekhar Nori <nsekhar@ti.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/dp83867.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index ca1b462..5d8c5ec 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -135,17 +135,20 @@ static int dp83867_of_init(struct phy_device *phydev)
 	if (ret &&
 	    (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
 	     phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID))
-		return ret;
+		dp83867->rx_id_delay = DP83867_RGMIIDCTL_2_00_NS;
 
 	ret = of_property_read_u32(of_node, "ti,tx-internal-delay",
 				   &dp83867->tx_id_delay);
 	if (ret &&
 	    (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
 	     phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID))
-		return ret;
+		dp83867->tx_id_delay = DP83867_RGMIIDCTL_2_00_NS;
 
-	return of_property_read_u32(of_node, "ti,fifo-depth",
-				   &dp83867->fifo_depth);
+	ret = of_property_read_u32(of_node, "ti,fifo-depth", &dp83867->fifo_depth);
+	if (ret)
+		dp83867->fifo_depth = DP83867_PHYCR_FIFO_DEPTH_4_B_NIB;
+
+	return 0;
 }
 #else
 static int dp83867_of_init(struct phy_device *phydev)
-- 
2.10.2

Re: [PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth

[David Miller]

From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
Date: Mon,  6 Feb 2017 22:24:45 +0300

> Given there're default values mentioned in the PHY datasheet
> fall-back gracefully to them instead of silently return an error
> through the whole call-chain.
> 
> This allows to use minimalistic description in DT if no special
> features are required.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

If these defaults are legitimate to use, then you should probably also
set them in the non-CONFIG_OF_MDIO case implementation of
dp83867_of_init().

But I'd like to see feedback and reviews from other PHY area experts
before I even consider that updated version of this change.

Thanks.

Re: [PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth

[Florian Fainelli]

On 02/08/2017 10:15 AM, David Miller wrote:
> From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
> Date: Mon,  6 Feb 2017 22:24:45 +0300
> 
>> Given there're default values mentioned in the PHY datasheet
>> fall-back gracefully to them instead of silently return an error
>> through the whole call-chain.
>>
>> This allows to use minimalistic description in DT if no special
>> features are required.
>>
>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> If these defaults are legitimate to use, then you should probably also
> set them in the non-CONFIG_OF_MDIO case implementation of
> dp83867_of_init().

IIRC this is what Alexey is doing already, in case of errors, instead of
returning the error code, he changes the default values and does not
propagate the error code.

Alexey, you are essentially making dp83867_of_init() impossible to fail
(or nearly) now, even with invalid properties, so I agree with David
here, you should probably have a function that runs after
dp83867_of_init() whose job is to set default values, if none have been
previously set through Device Tree.

> 
> But I'd like to see feedback and reviews from other PHY area experts
> before I even consider that updated version of this change.
> 
> Thanks.
> 


-- 
Florian

Re: [PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth

[Alexey Brodkin]

Hi Florian,

On Wed, 2017-02-08 at 10:20 -0800, Florian Fainelli wrote:
> On 02/08/2017 10:15 AM, David Miller wrote:
> > 
> > From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
> > Date: Mon,  6 Feb 2017 22:24:45 +0300
> > 
> > > 
> > > Given there're default values mentioned in the PHY datasheet
> > > fall-back gracefully to them instead of silently return an error
> > > through the whole call-chain.
> > > 
> > > This allows to use minimalistic description in DT if no special
> > > features are required.
> > > 
> > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > 
> > If these defaults are legitimate to use, then you should probably also
> > set them in the non-CONFIG_OF_MDIO case implementation of
> > dp83867_of_init().
> 
> IIRC this is what Alexey is doing already, in case of errors, instead of
> returning the error code, he changes the default values and does not
> propagate the error code.
> 
> Alexey, you are essentially making dp83867_of_init() impossible to fail
> (or nearly) now, even with invalid properties, so I agree with David
> here, you should probably have a function that runs after
> dp83867_of_init() whose job is to set default values, if none have been
> previously set through Device Tree.

But why do we need to return error code from dp83867_of_init()?
The point is this function doesn't do any hardware setup as well,
in fact it doesn't even reads anything from real hardware.

So we may indeed report an error to above callers (which is not super convenient
because the error comes many levels above without any error message making
understanding of the failure pretty complicated - go trace where it came from)
but what would it mean to a user? What do we want our user to do?
Set at least something in required properties? Then why don't we do that ourselves
right in the driver? Moreover since we're talking about defaults mentioned by
the PHY manufacturer we will just use "factory settings". Still allowing others to
override defaults with their .dts.

I agree with David that it really worth to do the same settings for non_CONFIG_OF_MDIO
case.

-Alexey