Skip to contentSkip to conversation

phy: samsung: Use readl_poll_timeout function

PATCH Superseded Anand Moon 3 participants 5 messages Jul 13, 2020 – Jul 20, 2020

Subsystems (3)
USB Arm Samsung Exynos Generic PHY framework

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Anand Moon

author
#0 lore raw

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

Instead of a busy waiting while loop using udelay use readl_poll_timeout function to check the condition is met or timeout occurs in crport_handshake function. readl_poll_timeout is called in non atomic context so it safe to sleep until the condition is met.

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

---
Changes v4:
Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
--Fix the commit message.
--Fix the error timeout condition for -ETIMEDOUT
---
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 | 39 ++++++++----------------
 1 file changed, 12 insertions(+), 27 deletions(-)
drivers/phy/samsung/phy-exynos5-usbdrd.c +12 -27
index 7f6279fb4f8f..ad81aa65cdff 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,41 +557,25 @@ 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);
-		return -ETIME;
+	err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+			result,	(result & PHYREG1_CR_ACK), 1, 100);
+	if (err == -ETIMEDOUT) {
+		dev_err(phy_drd->dev, "CRPORT handshake timeout1 (0x%08x)\n", val);
+		return err;
 	}
 
-	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);
-		return -ETIME;
+	err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+			result,	!(result & PHYREG1_CR_ACK), 1, 100);
+	if (err == -ETIMEDOUT) {
+		dev_err(phy_drd->dev, "CRPORT handshake timeout2 (0x%08x)\n", val);
+		return err;
 	}
 
 	return 0;
Signature · 1 line
2.27.0
maintainer
#1 lore raw
Quoted 50 lines · Anand Moon

Instead of a busy waiting while loop using udelay use readl_poll_timeout function to check the condition is met or timeout occurs in crport_handshake function. readl_poll_timeout is called in non atomic context so it safe to sleep until the condition is met.

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

---
Changes v4:
Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
--Fix the commit message.
--Fix the error timeout condition for -ETIMEDOUT
---
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 | 39 ++++++++----------------
 1 file changed, 12 insertions(+), 27 deletions(-)
drivers/phy/samsung/phy-exynos5-usbdrd.c +2 -5
index 7f6279fb4f8f..ad81aa65cdff 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,41 +557,25 @@ 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;

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

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

pls align this line to opening brace of preceding line:

        err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
                                 result, (result & PHYREG1_CR_ACK), 1, 100);

This is recommended way of splitting lines, see Documentation/process/coding-style.rst and run checkpatch.pl with --strict option

thanks

Signature · 1 line
~Vinod
author
#2 lore raw

Hi Vinod,

Quoted 65 lines · Vinod Koul
Quoted 50 lines · Anand Moon

Instead of a busy waiting while loop using udelay use readl_poll_timeout function to check the condition is met or timeout occurs in crport_handshake function. readl_poll_timeout is called in non atomic context so it safe to sleep until the condition is met.

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

---
Changes v4:
Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
--Fix the commit message.
--Fix the error timeout condition for -ETIMEDOUT
---
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 | 39 ++++++++----------------
 1 file changed, 12 insertions(+), 27 deletions(-)
drivers/phy/samsung/phy-exynos5-usbdrd.c +2 -5
index 7f6279fb4f8f..ad81aa65cdff 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,41 +557,25 @@ 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;

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

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

pls align this line to opening brace of preceding line:

Quoted 6 lines
        err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
                                 result, (result & PHYREG1_CR_ACK), 1, 100);

This is recommended way of splitting lines, see Documentation/process/coding-style.rst and run checkpatch.pl with --strict option

Ok, I will do this, just waiting for some more feedback on these changes.

Quoted 4 lines

thanks
--
~Vinod

-Anand

maintainer
#3 lore raw
In reply to Anand Moon
Quoted 7 lines · Anand Moon

Instead of a busy waiting while loop using udelay use readl_poll_timeout function to check the condition is met or timeout occurs in crport_handshake function. readl_poll_timeout is called in non atomic context so it safe to sleep until the condition is met.

Fixes d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")

There is no bug in original code so Fixes tag is not appropriate. Remove it please.

Best regards,
Krzysztof

Quoted 17 lines
Signed-off-by Anand Moon <[email protected]>

---
Changes v4:
Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
--Fix the commit message.
--Fix the error timeout condition for -ETIMEDOUT
---
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 | 39 ++++++++----------------
 1 file changed, 12 insertions(+), 27 deletions(-)
author
#4 lore raw

Hi Krzysztof,

Quoted 8 lines · Krzysztof Kozlowski
Quoted 6 lines · Anand Moon

Instead of a busy waiting while loop using udelay use readl_poll_timeout function to check the condition is met or timeout occurs in crport_handshake function. readl_poll_timeout is called in non atomic context so it safe to sleep until the condition is met.

Quoted 5 lines
Quoted 1 line
Fixes d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")

There is no bug in original code so Fixes tag is not appropriate. Remove it please.

Thanks for your review. Ok I will do that.

Quoted 3 lines

Best regards,
Krzysztof

-Anand

Quoted 17 lines
Quoted 17 lines
Signed-off-by Anand Moon <[email protected]>

---
Changes v4:
Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
--Fix the commit message.
--Fix the error timeout condition for -ETIMEDOUT
---
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 | 39 ++++++++----------------
 1 file changed, 12 insertions(+), 27 deletions(-)