[2/4] clk: tegra: add fence_delay for clock registers

Message ID 1510313868-24810-3-git-send-email-pdeschrijver@nvidia.com
State New
Headers show
Series
  • MBIST WAR for Tegra210
Related show

Commit Message

Peter De Schrijver Nov. 10, 2017, 11:37 a.m.
Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 drivers/clk/tegra/clk.h | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Stephen Boyd Nov. 15, 2017, 12:41 a.m. | #1
On 11/10, Peter De Schrijver wrote:
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>

Some commit text perhaps explaining why we add this?

> ---
>  drivers/clk/tegra/clk.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> index 872f118..3b8947e 100644
> --- a/drivers/clk/tegra/clk.h
> +++ b/drivers/clk/tegra/clk.h
> @@ -809,4 +809,11 @@ static inline struct clk *tegra_clk_register_emc(void __iomem *base,
>  u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate);
>  int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
>  
> +/* Combined read fence with delay */
> +#define fence_udelay(delay, reg)	\
> +	do {				\
> +		readl_relaxed(reg);	\
> +		udelay(delay);		\

Does the udelay() need to be after the readl()? As far as I
recall, the delay can start spinning before the read returns
because there isn't any sort of barrier between the two. One
approach would be to have an mb() between the read and the delay
so that the read is ordered with respect to the delay. If not, we
need a better comment than "combined read fence with delay".
Peter De Schrijver Nov. 16, 2017, 11:27 a.m. | #2
On Tue, Nov 14, 2017 at 04:41:32PM -0800, Stephen Boyd wrote:
> On 11/10, Peter De Schrijver wrote:
> > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> 
> Some commit text perhaps explaining why we add this?
> 
> > ---
> >  drivers/clk/tegra/clk.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> > index 872f118..3b8947e 100644
> > --- a/drivers/clk/tegra/clk.h
> > +++ b/drivers/clk/tegra/clk.h
> > @@ -809,4 +809,11 @@ static inline struct clk *tegra_clk_register_emc(void __iomem *base,
> >  u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate);
> >  int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
> >  
> > +/* Combined read fence with delay */
> > +#define fence_udelay(delay, reg)	\
> > +	do {				\
> > +		readl_relaxed(reg);	\
> > +		udelay(delay);		\
> 
> Does the udelay() need to be after the readl()? As far as I
> recall, the delay can start spinning before the read returns
> because there isn't any sort of barrier between the two. One
> approach would be to have an mb() between the read and the delay
> so that the read is ordered with respect to the delay. If not, we
> need a better comment than "combined read fence with delay".
> 

Yes. the udelay() needs to be after the readl, but it indeed needs to be
readl() so we're sure the read cycle has been completed.

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
index 872f118..3b8947e 100644
--- a/drivers/clk/tegra/clk.h
+++ b/drivers/clk/tegra/clk.h
@@ -809,4 +809,11 @@  static inline struct clk *tegra_clk_register_emc(void __iomem *base,
 u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate);
 int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
 
+/* Combined read fence with delay */
+#define fence_udelay(delay, reg)	\
+	do {				\
+		readl_relaxed(reg);	\
+		udelay(delay);		\
+	} while(0)
+
 #endif /* TEGRA_CLK_H */