[PATCH 0/2] clk: imx: pll14xx: io relaxed fix

[PATCH 0/2] clk: imx: pll14xx: io relaxed fix

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

This patchset is insipred from Will Deacon's slide/video:
https://elinux.org/images/a/a8/Uh-oh-Its-IO-Ordering-Will-Deacon-Arm.pdf
https://www.youtube.com/watch?v=i6DayghhA8Q

Peng Fan (2):
  clk: imx: pll14xx: use writel_relaxed
  clk: imx: pll14xx: use readl to force write completed

 drivers/clk/imx/clk-pll14xx.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.16.4

[PATCH 1/2] clk: imx: pll14xx: use writel_relaxed

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

It not make sense to use writel, use relaxed variant.

Cc: Will Deacon <will@kernel.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/clk-pll14xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index 5682fce9f5e5..e34813904023 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -199,7 +199,7 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	/* Enable BYPASS */
 	tmp |= BYPASS_MASK;
-	writel(tmp, pll->base);
+	writel_relaxed(tmp, pll->base);
 
 	div_val = (rate->mdiv << MDIV_SHIFT) | (rate->pdiv << PDIV_SHIFT) |
 		(rate->sdiv << SDIV_SHIFT);
-- 
2.16.4

Re: [PATCH 1/2] clk: imx: pll14xx: use writel_relaxed

[Daniel Baluta]

On Wed, 2019-11-13 at 07:24 +0000, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> It not make sense to use writel, use relaxed variant.
> 

Hi Peng,

Please explain why this change is needed. 

RE: [PATCH 1/2] clk: imx: pll14xx: use writel_relaxed

[Peng Fan]

Hi Daniel,

> Subject: Re: [PATCH 1/2] clk: imx: pll14xx: use writel_relaxed
> 
> 
> On Wed, 2019-11-13 at 07:24 +0000, Peng Fan wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > It not make sense to use writel, use relaxed variant.
> >
> 
> Hi Peng,
> 
> Please explain why this change is needed.

writel has a barrier, however that barrier is not needed,
because device memory access is in order and clk driver
has spin_lock or other lock to make sure write finished.

I would hear more comments before I post V2 about
the change and other similar patches to switch to
use relaxed API.

Thanks,
Peng.

Re: [PATCH 1/2] clk: imx: pll14xx: use writel_relaxed

[Daniel Baluta]

On Wed, 2019-11-13 at 12:15 +0000, Peng Fan wrote:
> Hi Daniel,
> 
> > Subject: Re: [PATCH 1/2] clk: imx: pll14xx: use writel_relaxed
> > 
> > 
> > On Wed, 2019-11-13 at 07:24 +0000, Peng Fan wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > > 
> > > It not make sense to use writel, use relaxed variant.
> > > 
> > 
> > Hi Peng,
> > 
> > Please explain why this change is needed.
> 
> writel has a barrier, however that barrier is not needed,
> because device memory access is in order and clk driver
> has spin_lock or other lock to make sure write finished.
> 

Make sure you add this in the commit message for v2 :).

[PATCH 2/2] clk: imx: pll14xx: use readl to force write completed

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

To ensure writes to clock registers have properly completed,
add a readl after writel_relaxed. Then we could make sure
when udelay, write has been completed.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Will Deacon <will@kernel.org>
---
 drivers/clk/imx/clk-pll14xx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index e34813904023..2bbcfbf8081a 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -205,6 +205,12 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 		(rate->sdiv << SDIV_SHIFT);
 	writel_relaxed(div_val, pll->base + 0x4);
 
+	/*
+	 * readl will force write completed. There is a udelay below,
+	 * we need make sure before udelay, write has been completed
+	 */
+	readl(pll->base + 0x4);
+
 	/*
 	 * According to SPEC, t3 - t2 need to be greater than
 	 * 1us and 1/FREF, respectively.
-- 
2.16.4