| Message ID | 20220117002641.26773-2-ansuelsmth@gmail.com |
|---|---|
| State | Accepted |
| Commit | dc9d167737a5814c8dcbdc33a298dc4b9407bba5 |
| Headers | show |
| Series | [1/2] drivers: phy: qcom: ipq806x-usb: convert to BITFIELD macro | expand |
On 17-01-22, 01:26, Ansuel Smith wrote: > Convert latch function to readl pool macro to tidy things up. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > index 6788e0e8272a..ab2d1431546d 100644 > --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > @@ -112,6 +112,9 @@ > #define SS_CR_READ_REG BIT(0) > #define SS_CR_WRITE_REG BIT(0) > > +#define LATCH_SLEEP 40 > +#define LATCH_TIMEOUT 100 > + > struct usb_phy { > void __iomem *base; > struct device *dev; > @@ -156,19 +159,9 @@ static inline void usb_phy_write_readback(struct usb_phy *phy_dwc3, > > static int wait_for_latch(void __iomem *addr) > { > - u32 retry = 10; > - > - while (true) { > - if (!readl(addr)) > - break; we break if read returns non zero value... Do you know what is the value expected? > - > - if (--retry == 0) > - return -ETIMEDOUT; > - > - usleep_range(10, 20); > - } > + u32 val; Okay this contains garbage.. > > - return 0; > + return readl_poll_timeout(addr, val, !val, LATCH_SLEEP, LATCH_TIMEOUT); and we are waiting for it read a garbage value!
On Sun, Jan 23, 2022 at 05:22:18PM +0530, Vinod Koul wrote: > On 17-01-22, 01:26, Ansuel Smith wrote: > > Convert latch function to readl pool macro to tidy things up. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c | 17 +++++------------ > > 1 file changed, 5 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > > index 6788e0e8272a..ab2d1431546d 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > > +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > > @@ -112,6 +112,9 @@ > > #define SS_CR_READ_REG BIT(0) > > #define SS_CR_WRITE_REG BIT(0) > > > > +#define LATCH_SLEEP 40 > > +#define LATCH_TIMEOUT 100 > > + > > struct usb_phy { > > void __iomem *base; > > struct device *dev; > > @@ -156,19 +159,9 @@ static inline void usb_phy_write_readback(struct usb_phy *phy_dwc3, > > > > static int wait_for_latch(void __iomem *addr) > > { > > - u32 retry = 10; > > - > > - while (true) { > > - if (!readl(addr)) > > - break; > > we break if read returns non zero value... > > Do you know what is the value expected? > If I understand the logic here, we write a value and we wait for it to get applied. To confirm that we execute a writel and then we readl the same address until it does return a value. That is the way used to understand that the write process has finished and that the value has been applied/we can write again. > > - > > - if (--retry == 0) > > - return -ETIMEDOUT; > > - > > - usleep_range(10, 20); > > - } > > + u32 val; > > Okay this contains garbage.. I think I didn't understand, val value will get replaced by readl in the pool_timeout function. > > > > - return 0; > > + return readl_poll_timeout(addr, val, !val, LATCH_SLEEP, LATCH_TIMEOUT); > > and we are waiting for it read a garbage value! > Again could be very confused and wrong but the pool_timeout macro does the exact same thing of the wait_for_latch function with th only difference of handling the sleep differently. We put in val the return of readl and the break condition as !val. Or I didn't understand the concern about garbage value. > > -- > ~Vinod
On 23-01-22, 15:46, Ansuel Smith wrote: > On Sun, Jan 23, 2022 at 05:22:18PM +0530, Vinod Koul wrote: > > On 17-01-22, 01:26, Ansuel Smith wrote: > > > Convert latch function to readl pool macro to tidy things up. > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > --- > > > drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c | 17 +++++------------ > > > 1 file changed, 5 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > > > index 6788e0e8272a..ab2d1431546d 100644 > > > --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > > > +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c > > > @@ -112,6 +112,9 @@ > > > #define SS_CR_READ_REG BIT(0) > > > #define SS_CR_WRITE_REG BIT(0) > > > > > > +#define LATCH_SLEEP 40 > > > +#define LATCH_TIMEOUT 100 > > > + > > > struct usb_phy { > > > void __iomem *base; > > > struct device *dev; > > > @@ -156,19 +159,9 @@ static inline void usb_phy_write_readback(struct usb_phy *phy_dwc3, > > > > > > static int wait_for_latch(void __iomem *addr) > > > { > > > - u32 retry = 10; > > > - > > > - while (true) { > > > - if (!readl(addr)) > > > - break; > > > > we break if read returns non zero value... > > > > Do you know what is the value expected? > > > > If I understand the logic here, we write a value and we wait for it to > get applied. To confirm that we execute a writel and then we readl the > same address until it does return a value. That is the way used to > understand that the write process has finished and that the value has > been applied/we can write again. > > > > - > > > - if (--retry == 0) > > > - return -ETIMEDOUT; > > > - > > > - usleep_range(10, 20); > > > - } > > > + u32 val; > > > > Okay this contains garbage.. > > I think I didn't understand, val value will get replaced by readl in > the pool_timeout function. > > > > > > > - return 0; > > > + return readl_poll_timeout(addr, val, !val, LATCH_SLEEP, LATCH_TIMEOUT); > > > > and we are waiting for it read a garbage value! > > > > Again could be very confused and wrong but the pool_timeout macro does > the exact same thing of the wait_for_latch function with th only > difference of handling the sleep differently. We put in val the return > of readl and the break condition as !val. Or I didn't understand the > concern about garbage value. Sorry I read the readl_poll_timeout wrongly, this seems correct, I will pick these now.
diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c index 6788e0e8272a..ab2d1431546d 100644 --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c @@ -112,6 +112,9 @@ #define SS_CR_READ_REG BIT(0) #define SS_CR_WRITE_REG BIT(0) +#define LATCH_SLEEP 40 +#define LATCH_TIMEOUT 100 + struct usb_phy { void __iomem *base; struct device *dev; @@ -156,19 +159,9 @@ static inline void usb_phy_write_readback(struct usb_phy *phy_dwc3, static int wait_for_latch(void __iomem *addr) { - u32 retry = 10; - - while (true) { - if (!readl(addr)) - break; - - if (--retry == 0) - return -ETIMEDOUT; - - usleep_range(10, 20); - } + u32 val; - return 0; + return readl_poll_timeout(addr, val, !val, LATCH_SLEEP, LATCH_TIMEOUT); } /**
Convert latch function to readl pool macro to tidy things up. Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)