Linux-Samsung-soc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] phy: samsung: Use readl_poll_timeout function
@ 2020-07-07  9:59 Anand Moon
  2020-07-07 11:36 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 4+ messages in thread
From: Anand Moon @ 2020-07-07  9:59 UTC (permalink / raw)
  To: linux-arm-kernel, linux-samsung-soc, linux-kernel
  Cc: Kishon Vijay Abraham I, Vinod Koul, Kukjin Kim,
	Krzysztof Kozlowski, Marek Szyprowski

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 <linux.amoon@gmail.com>
---
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(-)

diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
index e510732afb8b..fa75fa88da33 100644
--- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
+++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
@@ -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;
 	}
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] phy: samsung: Use readl_poll_timeout function
  2020-07-07  9:59 [PATCH v3] phy: samsung: Use readl_poll_timeout function Anand Moon
@ 2020-07-07 11:36 ` Krzysztof Kozlowski
  2020-07-08  8:29   ` Anand Moon
  0 siblings, 1 reply; 4+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-07 11:36 UTC (permalink / raw)
  To: Anand Moon
  Cc: linux-arm-kernel, linux-samsung-soc, linux-kernel,
	Kishon Vijay Abraham I, Vinod Koul, Kukjin Kim, Marek Szyprowski

On Tue, Jul 07, 2020 at 09:59:08AM +0000, Anand Moon wrote:
> Instead of a busy waiting loop while loop using udelay

You doubled "loop".

> 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

> 
> Fixes: d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> 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(-)
> 
> diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> index e510732afb8b..fa75fa88da33 100644
> --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> @@ -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;
>  	}
>  
> -- 
> 2.27.0
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] phy: samsung: Use readl_poll_timeout function
  2020-07-07 11:36 ` Krzysztof Kozlowski
@ 2020-07-08  8:29   ` Anand Moon
  2020-07-08 12:31     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 4+ messages in thread
From: Anand Moon @ 2020-07-08  8:29 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-arm-kernel, linux-samsung-soc, Linux Kernel,
	Kishon Vijay Abraham I, Vinod Koul, Kukjin Kim, Marek Szyprowski

Hi Krzysztof,

On Tue, 7 Jul 2020 at 17:06, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, Jul 07, 2020 at 09:59:08AM +0000, Anand Moon wrote:
> > Instead of a busy waiting loop while loop using udelay
>
> You doubled "loop".
>
I will fix this in the next version.
> > 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

> Best regards,
> Krzysztof
>
> >
> > Fixes: d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > index e510732afb8b..fa75fa88da33 100644
> > --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > @@ -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;
Here return should be -ETIMEDOUT;
> >       }
> >
> > -     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;
> >       }
> >

Best Regards
-Anand

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] phy: samsung: Use readl_poll_timeout function
  2020-07-08  8:29   ` Anand Moon
@ 2020-07-08 12:31     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 4+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-08 12:31 UTC (permalink / raw)
  To: Anand Moon
  Cc: linux-arm-kernel, linux-samsung-soc, Linux Kernel,
	Kishon Vijay Abraham I, Vinod Koul, Kukjin Kim, Marek Szyprowski

On Wed, Jul 08, 2020 at 01:59:46PM +0530, Anand Moon wrote:
> > 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.

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.

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

Great!

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07  9:59 [PATCH v3] phy: samsung: Use readl_poll_timeout function Anand Moon
2020-07-07 11:36 ` Krzysztof Kozlowski
2020-07-08  8:29   ` Anand Moon
2020-07-08 12:31     ` Krzysztof Kozlowski

Linux-Samsung-soc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-samsung-soc/0 linux-samsung-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-samsung-soc linux-samsung-soc/ https://lore.kernel.org/linux-samsung-soc \
		linux-samsung-soc@vger.kernel.org
	public-inbox-index linux-samsung-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-samsung-soc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git