Skip to contentSkip to conversation

phy: samsung: Use readl_poll_timeout function

PATCH Superseded Anand Moon 2 participants 4 messages Jul 7, 2020 – Jul 8, 2020

Subsystems (2)
Arm Samsung Exynos Generic PHY framework

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Anand Moon

author
#0 lore raw

PATCH v3 1/1 phy: samsung: Use readl_poll_timeout function

Instead of a busy waiting loop while loop using udelay use readl_poll_timeout function to check the condition is met or timeout occurs in crport_handshake function.

Fixes d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
Signed-off-by Anand Moon <[email protected]>

---
Changes v3:
--Fix the commit message.
--Drop the variable, used the value directly.
Changes v2:
--used the default timeout values.
--Added missing Fixed tags.

---
 drivers/phy/samsung/phy-exynos5-usbdrd.c | 35 +++++++-----------------
 1 file changed, 10 insertions(+), 25 deletions(-)
drivers/phy/samsung/phy-exynos5-usbdrd.c +10 -25
index e510732afb8b..fa75fa88da33 100644
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/iopoll.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>

@@ -556,40 +557,24 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
 static int crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
 			    u32 val, u32 cmd)
 {
-	u32 usec = 100;
 	unsigned int result;
+	int err;
 
 	writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
 
-	do {
-		result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
-		if (result & PHYREG1_CR_ACK)
-			break;
-
-		udelay(1);
-	} while (usec-- > 0);
-
-	if (!usec) {
-		dev_err(phy_drd->dev,
-			"CRPORT handshake timeout1 (0x%08x)\n", val);
+	err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+			result,	(result & PHYREG1_CR_ACK), 1, 100);
+	if (err) {
+		dev_err(phy_drd->dev, "CRPORT handshake timeout1 (0x%08x)\n", val);
 		return -ETIME;
 	}
 
-	usec = 100;
-
 	writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
 
-	do {
-		result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
-		if (!(result & PHYREG1_CR_ACK))
-			break;
-
-		udelay(1);
-	} while (usec-- > 0);
-
-	if (!usec) {
-		dev_err(phy_drd->dev,
-			"CRPORT handshake timeout2 (0x%08x)\n", val);
+	err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+			result,	!(result & PHYREG1_CR_ACK), 1, 100);
+	if (err) {
+		dev_err(phy_drd->dev, "CRPORT handshake timeout2 (0x%08x)\n", val);
 		return -ETIME;
 	}
Signature · 1 line
2.27.0
maintainer
#1 lore raw
Quoted 1 line · Anand Moon

Instead of a busy waiting loop while loop using udelay

You doubled "loop".

Quoted 2 lines

use readl_poll_timeout function to check the condition is met or timeout occurs in crport_handshake function.

Still you did not mention that you convert the function to use sleeping primitive. You also did not mention whether it is actually allowed in this context and I am not sure if you investigated it.

Best regards,
Krzysztof

Quoted 79 lines
Fixes d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
Signed-off-by Anand Moon <[email protected]>

---
Changes v3:
--Fix the commit message.
--Drop the variable, used the value directly.
Changes v2:
--used the default timeout values.
--Added missing Fixed tags.

---
 drivers/phy/samsung/phy-exynos5-usbdrd.c | 35 +++++++-----------------
 1 file changed, 10 insertions(+), 25 deletions(-)
drivers/phy/samsung/phy-exynos5-usbdrd.c +10 -25
index e510732afb8b..fa75fa88da33 100644
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/iopoll.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>

@@ -556,40 +557,24 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
 static int crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
 			    u32 val, u32 cmd)
 {
-	u32 usec = 100;
 	unsigned int result;
+	int err;
 
 	writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
 
-	do {
-		result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
-		if (result & PHYREG1_CR_ACK)
-			break;
-
-		udelay(1);
-	} while (usec-- > 0);
-
-	if (!usec) {
-		dev_err(phy_drd->dev,
-			"CRPORT handshake timeout1 (0x%08x)\n", val);
+	err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+			result,	(result & PHYREG1_CR_ACK), 1, 100);
+	if (err) {
+		dev_err(phy_drd->dev, "CRPORT handshake timeout1 (0x%08x)\n", val);
 		return -ETIME;
 	}
 
-	usec = 100;
-
 	writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
 
-	do {
-		result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
-		if (!(result & PHYREG1_CR_ACK))
-			break;
-
-		udelay(1);
-	} while (usec-- > 0);
-
-	if (!usec) {
-		dev_err(phy_drd->dev,
-			"CRPORT handshake timeout2 (0x%08x)\n", val);
+	err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+			result,	!(result & PHYREG1_CR_ACK), 1, 100);
+	if (err) {
+		dev_err(phy_drd->dev, "CRPORT handshake timeout2 (0x%08x)\n", val);
 		return -ETIME;
 	}
Signature · 1 line
2.27.0
author
#2 lore raw

Hi Krzysztof,

Quoted 6 lines · Krzysztof Kozlowski
Quoted 1 line · Anand Moon

Instead of a busy waiting loop while loop using udelay

You doubled "loop".

I will fix this in the next version.

Quoted 7 lines
Quoted 2 lines

use readl_poll_timeout function to check the condition is met or timeout occurs in crport_handshake function.

Still you did not mention that you convert the function to use sleeping primitive. You also did not mention whether it is actually allowed in this context and I am not sure if you investigated it.

OK, I am not sure how to resolve your query.
I learned some new things.

So here are some points.
-- Yes read_poll_timeout internally used might_sleep if sleep_us != 0 under some condition.
-- None of the code in phy-exynos5-usbdrd.c is called using kernel synchronization
methods like spinlock / mutex.
-- I have checked this function is called non atomic context. see below.

[    6.006395] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.013042] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.019646] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.026174] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.032699] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.039246] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.045707] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.052276] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.058760] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.065189] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.071631] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.078033] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.084433] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.090812] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.097102] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.103413] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.109670] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[    6.115871] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
Quoted 39 lines

Best regards,
Krzysztof

Quoted 36 lines
Fixes d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
Signed-off-by Anand Moon <[email protected]>

---
Changes v3:
--Fix the commit message.
--Drop the variable, used the value directly.
Changes v2:
--used the default timeout values.
--Added missing Fixed tags.

---
 drivers/phy/samsung/phy-exynos5-usbdrd.c | 35 +++++++-----------------
 1 file changed, 10 insertions(+), 25 deletions(-)
drivers/phy/samsung/phy-exynos5-usbdrd.c +2 -1
index e510732afb8b..fa75fa88da33 100644
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/iopoll.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>

@@ -556,40 +557,24 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
 static int crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
                          u32 val, u32 cmd)
 {
-     u32 usec = 100;
      unsigned int result;
+     int err;
 
      writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
Quoted 16 lines
Quoted 6 lines
drivers/phy/samsung/phy-exynos5-usbdrd.c +0 -6
-     do {
-             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
-             if (result & PHYREG1_CR_ACK)
-                     break;
-
-             udelay(1);

drivers/phy/samsung/phy-exynos5-usbdrd.c static int exynos5_usbdrd_phy_power_off(struct phy *phy) · line 569

Quoted 10 lines
drivers/phy/samsung/phy-exynos5-usbdrd.c +4 -5
-     } while (usec-- > 0);
-
-     if (!usec) {
-             dev_err(phy_drd->dev,
-                     "CRPORT handshake timeout1 (0x%08x)\n", val);
+     err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+                     result, (result & PHYREG1_CR_ACK), 1, 100);
+     if (err) {
+             dev_err(phy_drd->dev, "CRPORT handshake timeout1 (0x%08x)\n", val);
              return -ETIME;

Here return should be -ETIMEDOUT;

Quoted 16 lines
Quoted 16 lines
drivers/phy/samsung/phy-exynos5-usbdrd.c +0 -12
      }
 
-     usec = 100;
-
      writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
 
-     do {
-             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
-             if (!(result & PHYREG1_CR_ACK))
-                     break;
-
-             udelay(1);
-     } while (usec-- > 0);
-
-     if (!usec) {
-             dev_err(phy_drd->dev,
Quoted 8 lines

drivers/phy/samsung/phy-exynos5-usbdrd.c static int exynos5_usbdrd_phy_power_off(struct phy *phy) · line 580

Quoted 8 lines
drivers/phy/samsung/phy-exynos5-usbdrd.c +4 -1
-                     "CRPORT handshake timeout2 (0x%08x)\n", val);
+     err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+                     result, !(result & PHYREG1_CR_ACK), 1, 100);
+     if (err) {
+             dev_err(phy_drd->dev, "CRPORT handshake timeout2 (0x%08x)\n", val);
              return -ETIME;
      }

Best Regards
-Anand

maintainer
#3 lore raw
Quoted 7 lines · Anand Moon
Quoted 4 lines

Still you did not mention that you convert the function to use sleeping primitive. You also did not mention whether it is actually allowed in this context and I am not sure if you investigated it.

OK, I am not sure how to resolve your query.
I learned some new things.

Quoted 6 lines

So here are some points.
-- Yes read_poll_timeout internally used might_sleep if sleep_us != 0 under some condition.
-- None of the code in phy-exynos5-usbdrd.c is called using kernel synchronization
methods like spinlock / mutex.

More important is rather the call to calibrare() as this is the place where affected code is used.

It is not only about synchronisation primitives used in the driver but also in the phy core. I guess there should not be a problem. I just stated the fact that you did not mention anything about it.

Quoted 1 line

-- I have checked this function is called non atomic context.

Great!

Best regards,
Krzysztof