Re: [PATCH 05/10] clk: iproc: Add PLL base write function

2015-10-12 Thread Jon Mason 
On Fri, Oct 09, 2015 at 05:21:06PM -0700, Stephen Boyd wrote:
> On 10/02, Jon Mason wrote:
> > diff --git a/drivers/clk/bcm/clk-iproc-pll.c 
> > b/drivers/clk/bcm/clk-iproc-pll.c
> > index e029ab3..a4602aa 100644
> > --- a/drivers/clk/bcm/clk-iproc-pll.c
> > +++ b/drivers/clk/bcm/clk-iproc-pll.c
> > @@ -137,6 +137,18 @@ static int pll_wait_for_lock(struct iproc_pll *pll)
> > return -EIO;
> >  }
> >  
> > +static void iproc_pll_write(struct iproc_pll *pll, void __iomem *base,
> > +   u32 offset, u32 val)
> > +{
> > +   const struct iproc_pll_ctrl *ctrl = pll->ctrl;
> > +
> > +   writel(val, base + offset);
> > +
> > +   if (unlikely(ctrl->flags & IPROC_CLK_NEEDS_READ_BACK &&
> > +base == pll->pll_base))
> > +   val = readl(base + offset);
> 
> Is there any point to assign val here?

It is a flush.  The val assignment could be excluded (and thus made
const) if so desired.

Thanks,
Jon

> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] clk: iproc: Add PLL base write function

2015-10-09 Thread Stephen Boyd 
On 10/02, Jon Mason wrote:
> diff --git a/drivers/clk/bcm/clk-iproc-pll.c b/drivers/clk/bcm/clk-iproc-pll.c
> index e029ab3..a4602aa 100644
> --- a/drivers/clk/bcm/clk-iproc-pll.c
> +++ b/drivers/clk/bcm/clk-iproc-pll.c
> @@ -137,6 +137,18 @@ static int pll_wait_for_lock(struct iproc_pll *pll)
>   return -EIO;
>  }
>  
> +static void iproc_pll_write(struct iproc_pll *pll, void __iomem *base,
> + u32 offset, u32 val)
> +{
> + const struct iproc_pll_ctrl *ctrl = pll->ctrl;
> +
> + writel(val, base + offset);
> +
> + if (unlikely(ctrl->flags & IPROC_CLK_NEEDS_READ_BACK &&
> +  base == pll->pll_base))
> + val = readl(base + offset);

Is there any point to assign val here?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] clk: iproc: Add PLL base write function

2015-10-09 Thread Jon Mason 
On Fri, Oct 09, 2015 at 12:03:57AM -0700, Stephen Boyd wrote:
> On 10/02, Jon Mason wrote:
> > diff --git a/drivers/clk/bcm/clk-iproc-pll.c 
> > b/drivers/clk/bcm/clk-iproc-pll.c
> > index e029ab3..a4602aa 100644
> > --- a/drivers/clk/bcm/clk-iproc-pll.c
> > +++ b/drivers/clk/bcm/clk-iproc-pll.c
> > @@ -137,6 +137,18 @@ static int pll_wait_for_lock(struct iproc_pll *pll)
> > return -EIO;
> >  }
> >  
> > +static void iproc_pll_write(struct iproc_pll *pll, void __iomem *base,
> 
> Seems that pll could be const too?

Yes, and offset can be const too.  I'll make these changes and
resubmit.

Thanks,
Jon

> 
> > +   u32 offset, u32 val)
> > +{
> > +   const struct iproc_pll_ctrl *ctrl = pll->ctrl;
> > +
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] clk: iproc: Add PLL base write function

2015-10-09 Thread Stephen Boyd 
On 10/02, Jon Mason wrote:
> diff --git a/drivers/clk/bcm/clk-iproc-pll.c b/drivers/clk/bcm/clk-iproc-pll.c
> index e029ab3..a4602aa 100644
> --- a/drivers/clk/bcm/clk-iproc-pll.c
> +++ b/drivers/clk/bcm/clk-iproc-pll.c
> @@ -137,6 +137,18 @@ static int pll_wait_for_lock(struct iproc_pll *pll)
>   return -EIO;
>  }
>  
> +static void iproc_pll_write(struct iproc_pll *pll, void __iomem *base,

Seems that pll could be const too?

> + u32 offset, u32 val)
> +{
> + const struct iproc_pll_ctrl *ctrl = pll->ctrl;
> +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/10] clk: iproc: Add PLL base write function

2015-10-02 Thread Jon Mason 
All writes to the PLL base address must be flushed if the
IPROC_CLK_NEEDS_READ_BACK flag is set.  If we add a function to make the
necessary write and reads, we can make sure that any future code which
makes PLL base writes will do the correct thing.

Signed-off-by: Jon Mason 
---
 drivers/clk/bcm/clk-iproc-pll.c | 80 +
 1 file changed, 33 insertions(+), 47 deletions(-)

diff --git a/drivers/clk/bcm/clk-iproc-pll.c b/drivers/clk/bcm/clk-iproc-pll.c
index e029ab3..a4602aa 100644
--- a/drivers/clk/bcm/clk-iproc-pll.c
+++ b/drivers/clk/bcm/clk-iproc-pll.c
@@ -137,6 +137,18 @@ static int pll_wait_for_lock(struct iproc_pll *pll)
return -EIO;
 }
 
+static void iproc_pll_write(struct iproc_pll *pll, void __iomem *base,
+   u32 offset, u32 val)
+{
+   const struct iproc_pll_ctrl *ctrl = pll->ctrl;
+
+   writel(val, base + offset);
+
+   if (unlikely(ctrl->flags & IPROC_CLK_NEEDS_READ_BACK &&
+base == pll->pll_base))
+   val = readl(base + offset);
+}
+
 static void __pll_disable(struct iproc_pll *pll)
 {
const struct iproc_pll_ctrl *ctrl = pll->ctrl;
@@ -145,27 +157,24 @@ static void __pll_disable(struct iproc_pll *pll)
if (ctrl->flags & IPROC_CLK_PLL_ASIU) {
val = readl(pll->asiu_base + ctrl->asiu.offset);
val &= ~(1 << ctrl->asiu.en_shift);
-   writel(val, pll->asiu_base + ctrl->asiu.offset);
+   iproc_pll_write(pll, pll->asiu_base, ctrl->asiu.offset, val);
}
 
if (ctrl->flags & IPROC_CLK_EMBED_PWRCTRL) {
val = readl(pll->pll_base + ctrl->aon.offset);
val |= (bit_mask(ctrl->aon.pwr_width) << ctrl->aon.pwr_shift);
-   writel(val, pll->pll_base + ctrl->aon.offset);
-
-   if (unlikely(ctrl->flags & IPROC_CLK_NEEDS_READ_BACK))
-   readl(pll->pll_base + ctrl->aon.offset);
+   iproc_pll_write(pll, pll->pll_base, ctrl->aon.offset, val);
}
 
if (pll->pwr_base) {
/* latch input value so core power can be shut down */
val = readl(pll->pwr_base + ctrl->aon.offset);
val |= (1 << ctrl->aon.iso_shift);
-   writel(val, pll->pwr_base + ctrl->aon.offset);
+   iproc_pll_write(pll, pll->pwr_base, ctrl->aon.offset, val);
 
/* power down the core */
val &= ~(bit_mask(ctrl->aon.pwr_width) << ctrl->aon.pwr_shift);
-   writel(val, pll->pwr_base + ctrl->aon.offset);
+   iproc_pll_write(pll, pll->pwr_base, ctrl->aon.offset, val);
}
 }
 
@@ -177,10 +186,7 @@ static int __pll_enable(struct iproc_pll *pll)
if (ctrl->flags & IPROC_CLK_EMBED_PWRCTRL) {
val = readl(pll->pll_base + ctrl->aon.offset);
val &= ~(bit_mask(ctrl->aon.pwr_width) << ctrl->aon.pwr_shift);
-   writel(val, pll->pll_base + ctrl->aon.offset);
-
-   if (unlikely(ctrl->flags & IPROC_CLK_NEEDS_READ_BACK))
-   readl(pll->pll_base + ctrl->aon.offset);
+   iproc_pll_write(pll, pll->pll_base, ctrl->aon.offset, val);
}
 
if (pll->pwr_base) {
@@ -188,14 +194,14 @@ static int __pll_enable(struct iproc_pll *pll)
val = readl(pll->pwr_base + ctrl->aon.offset);
val |= bit_mask(ctrl->aon.pwr_width) << ctrl->aon.pwr_shift;
val &= ~(1 << ctrl->aon.iso_shift);
-   writel(val, pll->pwr_base + ctrl->aon.offset);
+   iproc_pll_write(pll, pll->pwr_base, ctrl->aon.offset, val);
}
 
/* certain PLLs also need to be ungated from the ASIU top level */
if (ctrl->flags & IPROC_CLK_PLL_ASIU) {
val = readl(pll->asiu_base + ctrl->asiu.offset);
val |= (1 << ctrl->asiu.en_shift);
-   writel(val, pll->asiu_base + ctrl->asiu.offset);
+   iproc_pll_write(pll, pll->asiu_base, ctrl->asiu.offset, val);
}
 
return 0;
@@ -209,9 +215,7 @@ static void __pll_put_in_reset(struct iproc_pll *pll)
 
val = readl(pll->pll_base + reset->offset);
val &= ~(1 << reset->reset_shift | 1 << reset->p_reset_shift);
-   writel(val, pll->pll_base + reset->offset);
-   if (unlikely(ctrl->flags & IPROC_CLK_NEEDS_READ_BACK))
-   readl(pll->pll_base + reset->offset);
+   iproc_pll_write(pll, pll->pll_base, reset->offset, val);
 }
 
 static void __pll_bring_out_reset(struct iproc_pll *pll, unsigned int kp,
@@ -228,9 +232,7 @@ static void __pll_bring_out_reset(struct iproc_pll *pll, 
unsigned int kp,
val |=  ki << reset->ki_shift | kp << reset->kp_shift |
ka << reset->ka_shift;
val |= 1 << reset->reset_shift | 1 << reset->p_reset_shift;
-   writel(val, pll->pll_base + reset->offset);
-   if (unlikely(ctrl->flags & IPROC_CLK_NEEDS_READ_BACK))
-