Re: [PATCH v3] phy: samsung: Use readl_poll_timeout function

[Anand Moon]

Hi Krzysztof,

On Tue, 7 Jul 2020 at 17:06, Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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