lkml.org 
[lkml]   [2020]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH v3] phy: samsung: Use readl_poll_timeout function
    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

    \
     
     \ /
      Last update: 2020-07-08 10:30    [W:3.564 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site