[U-Boot] [PATCH 04/12] Net: phy: ti: Fix fifo_depth register write

[U-Boot] [PATCH 04/12] Net: phy: ti: Fix fifo_depth register write

[Janine Hagemann]

The register was not read before the writing, so the
previous value was overwritten.

Signed-off-by: Janine Hagemann <j.hagemann@phytec.de>
---
 drivers/net/phy/ti.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/ti.c b/drivers/net/phy/ti.c
index 8f3ed8a..d7ae881 100644
--- a/drivers/net/phy/ti.c
+++ b/drivers/net/phy/ti.c
@@ -51,6 +51,8 @@ DECLARE_GLOBAL_DATA_PTR;
 
 /* PHY CTRL bits */
 #define DP83867_PHYCR_FIFO_DEPTH_SHIFT		14
+#define DP83867_PHYCR_FIFO_DEPTH_MASK	(3 << 14)
+#define DP83867_PHYCR_RESERVED_MASK	BIT(11)
 #define DP83867_MDI_CROSSOVER		5
 #define DP83867_MDI_CROSSOVER_AUTO	2
 #define DP83867_MDI_CROSSOVER_MDIX	2
@@ -233,9 +235,14 @@ static int dp83867_config(struct phy_device *phydev)
 		  val | DP83867_SW_RESTART);
 
 	if (phy_interface_is_rgmii(phydev)) {
+		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL);
+		if (val < 0)
+			return val;
+		val &= ~DP83867_PHYCR_FIFO_DEPTH_MASK;
+		val |= (dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT);
 		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL,
-			(DP83867_MDI_CROSSOVER_AUTO << DP83867_MDI_CROSSOVER) |
-			(dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
+				val);
+
 		if (ret)
 			goto err_out;
 	} else if (phy_interface_is_sgmii(phydev)) {
-- 
2.7.4

[U-Boot] [PATCH 04/12] Net: phy: ti: Fix fifo_depth register write

[Trent Piepho]

On Thu, 2018-06-14 at 10:53 +0000, u-boot-request at lists.denx.de wrote:
> Message: 52
> Date: Thu, 14 Jun 2018 11:48:48 +0200
> From: Janine Hagemann <j.hagemann@phytec.de>
> To: albert.u.boot at aribaud.net, sjg at chromium.org,
> 	philipp.tomsich at theobroma-systems.com, w.egorov at phytec.de,
> 	joe.hershberger at ni.com, u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH 04/12] Net: phy: ti: Fix fifo_depth register
> 	write
> Message-ID: <1528969736-44037-4-git-send-email-j.hagemann@phytec.de>
> 
> The register was not read before the writing, so the
> previous value was overwritten.
> 
> @@ -233,9 +235,14 @@ static int dp83867_config(struct phy_device *phydev)
>  		  val | DP83867_SW_RESTART);
>  
>  	if (phy_interface_is_rgmii(phydev)) {
> +		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL);
> +		if (val < 0)
> +			return val;
> +		val &= ~DP83867_PHYCR_FIFO_DEPTH_MASK;
> +		val |= (dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT);
>  		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL,
> -			(DP83867_MDI_CROSSOVER_AUTO << DP83867_MDI_CROSSOVER) |
> -			(dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
> +				val);
> +
>  		if (ret)
>  			goto err_out;
>  	} else if (phy_interface_is_sgmii(phydev)) {

If any of the bits that prevent the phy from working are set, like
DEEP_POWER_DOWN_EN, POWER_SAVE_MODE, and so on, they won't be reset
anymore.  I.e., put phy in power save mode, reboot, phy doesn't work
anymore.  I think the code here is suppose to be configuring the phy,
rather than changing a configuration that was done elsewhere.

[U-Boot] [PATCH 04/12] Net: phy: ti: Fix fifo_depth register write

[Joe Hershberger]

On Thu, Jun 14, 2018 at 12:09 PM, Trent Piepho <tpiepho@impinj.com> wrote:
> On Thu, 2018-06-14 at 10:53 +0000, u-boot-request at lists.denx.de wrote:
>> Message: 52
>> Date: Thu, 14 Jun 2018 11:48:48 +0200
>> From: Janine Hagemann <j.hagemann@phytec.de>
>> To: albert.u.boot at aribaud.net, sjg at chromium.org,
>>       philipp.tomsich at theobroma-systems.com, w.egorov at phytec.de,
>>       joe.hershberger at ni.com, u-boot at lists.denx.de
>> Subject: [U-Boot] [PATCH 04/12] Net: phy: ti: Fix fifo_depth register
>>       write
>> Message-ID: <1528969736-44037-4-git-send-email-j.hagemann@phytec.de>
>>
>> The register was not read before the writing, so the
>> previous value was overwritten.

Was this changed because of some problem? Please include more details
in the change log.

>> @@ -233,9 +235,14 @@ static int dp83867_config(struct phy_device *phydev)
>>                 val | DP83867_SW_RESTART);
>>
>>       if (phy_interface_is_rgmii(phydev)) {
>> +             val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL);
>> +             if (val < 0)
>> +                     return val;
>> +             val &= ~DP83867_PHYCR_FIFO_DEPTH_MASK;
>> +             val |= (dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT);
>>               ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL,
>> -                     (DP83867_MDI_CROSSOVER_AUTO << DP83867_MDI_CROSSOVER) |
>> -                     (dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
>> +                             val);
>> +
>>               if (ret)
>>                       goto err_out;
>>       } else if (phy_interface_is_sgmii(phydev)) {
>
> If any of the bits that prevent the phy from working are set, like
> DEEP_POWER_DOWN_EN, POWER_SAVE_MODE, and so on, they won't be reset
> anymore.  I.e., put phy in power save mode, reboot, phy doesn't work
> anymore.  I think the code here is suppose to be configuring the phy,
> rather than changing a configuration that was done elsewhere.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot