[PATCH v3] ARM: Samsung SoC: clksrc-clk: wait for the stable SRC/DIV status.

[PATCH v3] ARM: Samsung SoC: clksrc-clk: wait for the stable SRC/DIV status.

[MyungJoo Ham]

Many MUX and clock dividers have a status bit so that users can wait
until the status is stable. When corresponding registers are accessed
while a clock is not stable, we may suffer from unexpected errors.

Therefore, we introduce a mechanism to let the operations related with
updating SRC/DIV registers of clksrc-clk wait for the stabilization:
clk_set_parent, clk_set_rate.

In order to use this feature, the definition of clksrc_clk should
include reg_src_stable or reg_div_stable. With effective rec_src_stable
values, clk_set_parent returns with a stabilized SRC register and
with effective rec_div_stable values, clk_set_rate returns with a
stabilized DIV register. If .reg field is null, its (either SRC or DIV)
register's status is not checked and returned without waiting; i.e.,
some MUX/DIV may not need this feature.

When setting reg_*_stable, .size is used to tell the value of "stable".
If .size = 0, the stable status is 0 and if .size = 1, the stable status
is 1.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
--
v2:
	- Wait-for-stable loop is described at an inline function.
v3:
	- Stop using clksrc_reg for stable bits. Use "clkstat_reg" for
	  them.

---
 arch/arm/plat-samsung/clock-clksrc.c              |   14 +++++++++++++
 arch/arm/plat-samsung/include/plat/clock-clksrc.h |   22 +++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-samsung/clock-clksrc.c b/arch/arm/plat-samsung/clock-clksrc.c
index 46d204a..79ae92e 100644
--- a/arch/arm/plat-samsung/clock-clksrc.c
+++ b/arch/arm/plat-samsung/clock-clksrc.c
@@ -50,6 +50,15 @@ static unsigned long s3c_getrate_clksrc(struct clk *clk)
 	return rate;
 }
 
+static inline void s3c_wait_for_stable(struct clkstat_reg *stat)
+{
+	WARN_ON(stat == NULL);
+	if (stat->reg) {
+		do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
+				!= stat->stable);
+	}
+}
+
 static int s3c_setrate_clksrc(struct clk *clk, unsigned long rate)
 {
 	struct clksrc_clk *sclk = to_clksrc(clk);
@@ -68,6 +77,8 @@ static int s3c_setrate_clksrc(struct clk *clk, unsigned long rate)
 	val |= (div - 1) << sclk->reg_div.shift;
 	__raw_writel(val, reg);
 
+	s3c_wait_for_stable(&sclk->reg_div_stable);
+
 	return 0;
 }
 
@@ -93,6 +104,9 @@ static int s3c_setparent_clksrc(struct clk *clk, struct clk *parent)
 		clksrc |= src_nr << sclk->reg_src.shift;
 
 		__raw_writel(clksrc, sclk->reg_src.reg);
+
+		s3c_wait_for_stable(&sclk->reg_src_stable);
+
 		return 0;
 	}
 
diff --git a/arch/arm/plat-samsung/include/plat/clock-clksrc.h b/arch/arm/plat-samsung/include/plat/clock-clksrc.h
index 50a8ca7..f1d54da 100644
--- a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
+++ b/arch/arm/plat-samsung/include/plat/clock-clksrc.h
@@ -40,11 +40,30 @@ struct clksrc_reg {
 };
 
 /**
+ * struct clkstat_reg - register definition for clock stat bits
+ * @reg: pointer to the register in virtual memory.
+ * @shift: the shift in bits to where the bitfield is.
+ * @stable: the bit value of stable status.
+ */
+struct clkstat_reg {
+	void __iomem		*reg;
+	unsigned short		shift;
+	unsigned short		stable;
+};
+
+/**
  * struct clksrc_clk - class of clock for newer style samsung devices.
  * @clk: the standard clock representation
  * @sources: the sources for this clock
  * @reg_src: the register definition for selecting the clock's source
  * @reg_div: the register definition for the clock's output divisor
+ * @reg_src_stable: the register definition to probe if reg_src is
+ *    stabilized after the update of reg_src. It is "stabilized" if
+ *    reg[shift] == size. If reg == NULL, this stable reg is not looked
+ *    up. Thus, in S5PV210, size is usually 0.
+ * @reg_div_stable: the register definition to probe if reg_div is
+ *    stabilized after the update of reg_div. Same mechanism with
+ *    reg_src_stable.
  *
  * This clock implements the features required by the newer SoCs where
  * the standard clock block provides an input mux and a post-mux divisor
@@ -61,6 +80,9 @@ struct clksrc_clk {
 
 	struct clksrc_reg	reg_src;
 	struct clksrc_reg	reg_div;
+
+	struct clkstat_reg	reg_src_stable;
+	struct clkstat_reg	reg_div_stable;
 };
 
 /**
-- 
1.6.3.3

[PATCH v3] ARM: Samsung SoC: clksrc-clk: wait for the stable SRC/DIV status.

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> Many MUX and clock dividers have a status bit so that users can wait
> until the status is stable. When corresponding registers are accessed
> while a clock is not stable, we may suffer from unexpected errors.
> 
> Therefore, we introduce a mechanism to let the operations related with
> updating SRC/DIV registers of clksrc-clk wait for the stabilization:
> clk_set_parent, clk_set_rate.
> 
> In order to use this feature, the definition of clksrc_clk should
> include reg_src_stable or reg_div_stable. With effective rec_src_stable
> values, clk_set_parent returns with a stabilized SRC register and
> with effective rec_div_stable values, clk_set_rate returns with a
> stabilized DIV register. If .reg field is null, its (either SRC or DIV)
> register's status is not checked and returned without waiting; i.e.,
> some MUX/DIV may not need this feature.
> 
> When setting reg_*_stable, .size is used to tell the value of "stable".
> If .size = 0, the stable status is 0 and if .size = 1, the stable status
> is 1.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> --
> v2:
> 	- Wait-for-stable loop is described at an inline function.
> v3:
> 	- Stop using clksrc_reg for stable bits. Use "clkstat_reg" for
> 	  them.
> 
> ---
>  arch/arm/plat-samsung/clock-clksrc.c              |   14 +++++++++++++
>  arch/arm/plat-samsung/include/plat/clock-clksrc.h |   22
> +++++++++++++++++++++
>  2 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-samsung/clock-clksrc.c
b/arch/arm/plat-samsung/clock-
> clksrc.c
> index 46d204a..79ae92e 100644
> --- a/arch/arm/plat-samsung/clock-clksrc.c
> +++ b/arch/arm/plat-samsung/clock-clksrc.c
> @@ -50,6 +50,15 @@ static unsigned long s3c_getrate_clksrc(struct clk
*clk)
>  	return rate;
>  }
> 
> +static inline void s3c_wait_for_stable(struct clkstat_reg *stat)
> +{
> +	WARN_ON(stat == NULL);

if (stat == NULL)
	return;

If don't need to check status, have not become define relevant status
register.

> +	if (stat->reg) {

No need?...or need to check stat->reg and stat->shift?

> +		do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
> +				!= stat->stable);

I'm still wondering we _really_ need 'stable'...just comments is enough,
right now.

> +		do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
> +				!= stat->stable);

> +	}
> +}
> +
>  static int s3c_setrate_clksrc(struct clk *clk, unsigned long rate)
>  {
>  	struct clksrc_clk *sclk = to_clksrc(clk);
> @@ -68,6 +77,8 @@ static int s3c_setrate_clksrc(struct clk *clk, unsigned
long rate)
>  	val |= (div - 1) << sclk->reg_div.shift;
>  	__raw_writel(val, reg);
> 
> +	s3c_wait_for_stable(&sclk->reg_div_stable);
> +
>  	return 0;
>  }
> 
> @@ -93,6 +104,9 @@ static int s3c_setparent_clksrc(struct clk *clk, struct
clk
> *parent)
>  		clksrc |= src_nr << sclk->reg_src.shift;
> 
>  		__raw_writel(clksrc, sclk->reg_src.reg);
> +
> +		s3c_wait_for_stable(&sclk->reg_src_stable);
> +
>  		return 0;
>  	}
> 
> diff --git a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
b/arch/arm/plat-
> samsung/include/plat/clock-clksrc.h
> index 50a8ca7..f1d54da 100644
> --- a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
> +++ b/arch/arm/plat-samsung/include/plat/clock-clksrc.h
> @@ -40,11 +40,30 @@ struct clksrc_reg {
>  };
> 
>  /**
> + * struct clkstat_reg - register definition for clock stat bits
> + * @reg: pointer to the register in virtual memory.
> + * @shift: the shift in bits to where the bitfield is.
> + * @stable: the bit value of stable status.
> + */
> +struct clkstat_reg {
> +	void __iomem		*reg;
> +	unsigned short		shift;
> +	unsigned short		stable;

If you want to use this, just 'value' is better.
But...I think...we don't need this field now.

> +};
> +
> +/**
>   * struct clksrc_clk - class of clock for newer style samsung devices.
>   * @clk: the standard clock representation
>   * @sources: the sources for this clock
>   * @reg_src: the register definition for selecting the clock's source
>   * @reg_div: the register definition for the clock's output divisor
> + * @reg_src_stable: the register definition to probe if reg_src is
> + *    stabilized after the update of reg_src. It is "stabilized" if
> + *    reg[shift] == size. If reg == NULL, this stable reg is not looked
> + *    up. Thus, in S5PV210, size is usually 0.
> + * @reg_div_stable: the register definition to probe if reg_div is
> + *    stabilized after the update of reg_div. Same mechanism with
> + *    reg_src_stable.
>   *
>   * This clock implements the features required by the newer SoCs where
>   * the standard clock block provides an input mux and a post-mux divisor
> @@ -61,6 +80,9 @@ struct clksrc_clk {
> 
>  	struct clksrc_reg	reg_src;
>  	struct clksrc_reg	reg_div;
> +
> +	struct clkstat_reg	reg_src_stable;
> +	struct clkstat_reg	reg_div_stable;
>  };
> 
>  /**
> --


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH v3] ARM: Samsung SoC: clksrc-clk: wait for the stable SRC/DIV status.

[MyungJoo Ham]

On Mon, Aug 2, 2010 at 2:30 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> Many MUX and clock dividers have a status bit so that users can wait
>> until the status is stable. When corresponding registers are accessed
>> while a clock is not stable, we may suffer from unexpected errors.
>>
>> Therefore, we introduce a mechanism to let the operations related with
>> updating SRC/DIV registers of clksrc-clk wait for the stabilization:
>> clk_set_parent, clk_set_rate.
>>
>> In order to use this feature, the definition of clksrc_clk should
>> include reg_src_stable or reg_div_stable. With effective rec_src_stable
>> values, clk_set_parent returns with a stabilized SRC register and
>> with effective rec_div_stable values, clk_set_rate returns with a
>> stabilized DIV register. If .reg field is null, its (either SRC or DIV)
>> register's status is not checked and returned without waiting; i.e.,
>> some MUX/DIV may not need this feature.
>>
>> When setting reg_*_stable, .size is used to tell the value of "stable".
>> If .size = 0, the stable status is 0 and if .size = 1, the stable status
>> is 1.
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> --
>> v2:
>> ? ? ? - Wait-for-stable loop is described at an inline function.
>> v3:
>> ? ? ? - Stop using clksrc_reg for stable bits. Use "clkstat_reg" for
>> ? ? ? ? them.
>>
>> ---
>> ?arch/arm/plat-samsung/clock-clksrc.c ? ? ? ? ? ? ?| ? 14 +++++++++++++
>> ?arch/arm/plat-samsung/include/plat/clock-clksrc.h | ? 22
>> +++++++++++++++++++++
>> ?2 files changed, 36 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-samsung/clock-clksrc.c
> b/arch/arm/plat-samsung/clock-
>> clksrc.c
>> index 46d204a..79ae92e 100644
>> --- a/arch/arm/plat-samsung/clock-clksrc.c
>> +++ b/arch/arm/plat-samsung/clock-clksrc.c
>> @@ -50,6 +50,15 @@ static unsigned long s3c_getrate_clksrc(struct clk
> *clk)
>> ? ? ? return rate;
>> ?}
>>
>> +static inline void s3c_wait_for_stable(struct clkstat_reg *stat)
>> +{
>> + ? ? WARN_ON(stat == NULL);
>
> if (stat == NULL)
> ? ? ? ?return;

Because .reg_*_stable is included in struct clksrc_clk and
s3c_wait_for_stable is called by clk_set_parent and clk_set_rate,
which have struct clksrc_clk, stat is unlikely to be NULL and if it's
NULL, s3c_wait_for_stable is called from a wrong function and wrong
context; thus, I'd use WARN for it rather than silently doing return.

>
> If don't need to check status, have not become define relevant status
> register.
>
>> + ? ? if (stat->reg) {
>
> No need?...or need to check stat->reg and stat->shift?

If stat->reg is NULL (it can be intentionally NULL), it means that the
corresponding struct clksrc_clk does not have stable-stat bit; thus,
pass this checking routine. And, stat->shift may be both 0 and
positive effectively.

stat->reg = 0 : NO STAT REGISTER
stat->reg != 0 : STAT Register Exists. Use stat->shift to specify the
bit (stat->shift can be 0 or larger)

>
>> + ? ? ? ? ? ? do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? != stat->stable);
>
> I'm still wondering we _really_ need 'stable'...just comments is enough,
> right now.

S5PV210 is fine without stable because stat->stable is always 0 for S5PV210.

However, this is for "Samsung-SoC" not for "S5PV210" although only
S5PV210 uses this feature for now.

I've added stat->stable for the generality. Would it be better discard
such generality and assume that other Samsung-SoC systems will also
use the same stat bit scheme for every register (0 for stable, 1 for
changing for every STAT register) with S5PV210? If we can guarantee
such homogeneousity, getting rid of stable property would be perfectly
fine. It is added just because I couldn't sure about Samsung-SoC chips
other than S5PV210. (and I thought some chips may use different scheme
for each register; e.g., some registers use 0 for stable, some others
use 1 for stable)

>
>> + ? ? ? ? ? ? do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? != stat->stable);
>
>> + ? ? }
>> +}
>> +
>> ?static int s3c_setrate_clksrc(struct clk *clk, unsigned long rate)
>> ?{
>> ? ? ? struct clksrc_clk *sclk = to_clksrc(clk);
>> @@ -68,6 +77,8 @@ static int s3c_setrate_clksrc(struct clk *clk, unsigned
> long rate)
>> ? ? ? val |= (div - 1) << sclk->reg_div.shift;
>> ? ? ? __raw_writel(val, reg);
>>
>> + ? ? s3c_wait_for_stable(&sclk->reg_div_stable);
>> +
>> ? ? ? return 0;
>> ?}
>>
>> @@ -93,6 +104,9 @@ static int s3c_setparent_clksrc(struct clk *clk, struct
> clk
>> *parent)
>> ? ? ? ? ? ? ? clksrc |= src_nr << sclk->reg_src.shift;
>>
>> ? ? ? ? ? ? ? __raw_writel(clksrc, sclk->reg_src.reg);
>> +
>> + ? ? ? ? ? ? s3c_wait_for_stable(&sclk->reg_src_stable);
>> +
>> ? ? ? ? ? ? ? return 0;
>> ? ? ? }
>>
>> diff --git a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
> b/arch/arm/plat-
>> samsung/include/plat/clock-clksrc.h
>> index 50a8ca7..f1d54da 100644
>> --- a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
>> +++ b/arch/arm/plat-samsung/include/plat/clock-clksrc.h
>> @@ -40,11 +40,30 @@ struct clksrc_reg {
>> ?};
>>
>> ?/**
>> + * struct clkstat_reg - register definition for clock stat bits
>> + * @reg: pointer to the register in virtual memory.
>> + * @shift: the shift in bits to where the bitfield is.
>> + * @stable: the bit value of stable status.
>> + */
>> +struct clkstat_reg {
>> + ? ? void __iomem ? ? ? ? ? ?*reg;
>> + ? ? unsigned short ? ? ? ? ?shift;
>> + ? ? unsigned short ? ? ? ? ?stable;
>
> If you want to use this, just 'value' is better.
> But...I think...we don't need this field now.
>
>> +};
>> +
>> +/**
>> ? * struct clksrc_clk - class of clock for newer style samsung devices.
>> ? * @clk: the standard clock representation
>> ? * @sources: the sources for this clock
>> ? * @reg_src: the register definition for selecting the clock's source
>> ? * @reg_div: the register definition for the clock's output divisor
>> + * @reg_src_stable: the register definition to probe if reg_src is
>> + * ? ?stabilized after the update of reg_src. It is "stabilized" if
>> + * ? ?reg[shift] == size. If reg == NULL, this stable reg is not looked
>> + * ? ?up. Thus, in S5PV210, size is usually 0.
>> + * @reg_div_stable: the register definition to probe if reg_div is
>> + * ? ?stabilized after the update of reg_div. Same mechanism with
>> + * ? ?reg_src_stable.
>> ? *
>> ? * This clock implements the features required by the newer SoCs where
>> ? * the standard clock block provides an input mux and a post-mux divisor
>> @@ -61,6 +80,9 @@ struct clksrc_clk {
>>
>> ? ? ? struct clksrc_reg ? ? ? reg_src;
>> ? ? ? struct clksrc_reg ? ? ? reg_div;
>> +
>> + ? ? struct clkstat_reg ? ? ?reg_src_stable;
>> + ? ? struct clkstat_reg ? ? ?reg_div_stable;
>> ?};
>>
>> ?/**
>> --
>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>



-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[PATCH v3] ARM: Samsung SoC: clksrc-clk: wait for the stable SRC/DIV status.

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> On Mon, Aug 2, 2010 at 2:30 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > MyungJoo Ham wrote:
> >>
> >> Many MUX and clock dividers have a status bit so that users can wait
> >> until the status is stable. When corresponding registers are accessed
> >> while a clock is not stable, we may suffer from unexpected errors.
> >>
> >> Therefore, we introduce a mechanism to let the operations related with
> >> updating SRC/DIV registers of clksrc-clk wait for the stabilization:
> >> clk_set_parent, clk_set_rate.
> >>
> >> In order to use this feature, the definition of clksrc_clk should
> >> include reg_src_stable or reg_div_stable. With effective rec_src_stable
> >> values, clk_set_parent returns with a stabilized SRC register and
> >> with effective rec_div_stable values, clk_set_rate returns with a
> >> stabilized DIV register. If .reg field is null, its (either SRC or DIV)
> >> register's status is not checked and returned without waiting; i.e.,
> >> some MUX/DIV may not need this feature.
> >>
> >> When setting reg_*_stable, .size is used to tell the value of "stable".
> >> If .size = 0, the stable status is 0 and if .size = 1, the stable status
> >> is 1.
> >>
> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> --
> >> v2:
> >>       - Wait-for-stable loop is described at an inline function.
> >> v3:
> >>       - Stop using clksrc_reg for stable bits. Use "clkstat_reg" for
> >>         them.
> >>
> >> ---
> >>  arch/arm/plat-samsung/clock-clksrc.c              |   14 +++++++++++++
> >>  arch/arm/plat-samsung/include/plat/clock-clksrc.h |   22
> >> +++++++++++++++++++++
> >>  2 files changed, 36 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/plat-samsung/clock-clksrc.c
> > b/arch/arm/plat-samsung/clock-
> >> clksrc.c
> >> index 46d204a..79ae92e 100644
> >> --- a/arch/arm/plat-samsung/clock-clksrc.c
> >> +++ b/arch/arm/plat-samsung/clock-clksrc.c
> >> @@ -50,6 +50,15 @@ static unsigned long s3c_getrate_clksrc(struct clk
> > *clk)
> >>       return rate;
> >>  }
> >>
> >> +static inline void s3c_wait_for_stable(struct clkstat_reg *stat)
> >> +{
> >> +     WARN_ON(stat == NULL);
> >
> > if (stat == NULL)
> >        return;
> 
> Because .reg_*_stable is included in struct clksrc_clk and
> s3c_wait_for_stable is called by clk_set_parent and clk_set_rate,
> which have struct clksrc_clk, stat is unlikely to be NULL and if it's
> NULL, s3c_wait_for_stable is called from a wrong function and wrong
> context; thus, I'd use WARN for it rather than silently doing return.
> 
I don't think that need warning to it, because it is not wrong being NULL.

> >
> > If don't need to check status, have not become define relevant status
> > register.
> >
> >> +     if (stat->reg) {
> >
> > No need?...or need to check stat->reg and stat->shift?
> 
> If stat->reg is NULL (it can be intentionally NULL), it means that the
> corresponding struct clksrc_clk does not have stable-stat bit; thus,
> pass this checking routine. And, stat->shift may be both 0 and
> positive effectively.
> 
> stat->reg = 0 : NO STAT REGISTER
> stat->reg != 0 : STAT Register Exists. Use stat->shift to specify the
> bit (stat->shift can be 0 or larger)
> 
If stat != NULL, I think that it does not need...because should being reg.

> >
> >> +             do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
> >> +                             != stat->stable);
> >
> > I'm still wondering we _really_ need 'stable'...just comments is enough,
> > right now.
> 
> S5PV210 is fine without stable because stat->stable is always 0 for S5PV210.
> 

Generally, the meaning of stable value may not change....frankly I'm not sure...
but if change, then we can add that later...so right now, unnecessary stuff like that does not need.

Then...and no need to add 'struct clkstat_reg'

> However, this is for "Samsung-SoC" not for "S5PV210" although only
> S5PV210 uses this feature for now.
> 
> I've added stat->stable for the generality. Would it be better discard
> such generality and assume that other Samsung-SoC systems will also
> use the same stat bit scheme for every register (0 for stable, 1 for
> changing for every STAT register) with S5PV210? If we can guarantee
> such homogeneousity, getting rid of stable property would be perfectly
> fine. It is added just because I couldn't sure about Samsung-SoC chips
> other than S5PV210. (and I thought some chips may use different scheme
> for each register; e.g., some registers use 0 for stable, some others
> use 1 for stable)
> 
> >
> >> +             do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
> >> +                             != stat->stable);
> >
> >> +     }
> >> +}
> >> +
> >>  static int s3c_setrate_clksrc(struct clk *clk, unsigned long rate)
> >>  {
> >>       struct clksrc_clk *sclk = to_clksrc(clk);
> >> @@ -68,6 +77,8 @@ static int s3c_setrate_clksrc(struct clk *clk, unsigned
> > long rate)
> >>       val |= (div - 1) << sclk->reg_div.shift;
> >>       __raw_writel(val, reg);
> >>
> >> +     s3c_wait_for_stable(&sclk->reg_div_stable);
> >> +
> >>       return 0;
> >>  }
> >>
> >> @@ -93,6 +104,9 @@ static int s3c_setparent_clksrc(struct clk *clk, struct
> > clk
> >> *parent)
> >>               clksrc |= src_nr << sclk->reg_src.shift;
> >>
> >>               __raw_writel(clksrc, sclk->reg_src.reg);
> >> +
> >> +             s3c_wait_for_stable(&sclk->reg_src_stable);
> >> +
> >>               return 0;
> >>       }
> >>
> >> diff --git a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
> > b/arch/arm/plat-
> >> samsung/include/plat/clock-clksrc.h
> >> index 50a8ca7..f1d54da 100644
> >> --- a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
> >> +++ b/arch/arm/plat-samsung/include/plat/clock-clksrc.h
> >> @@ -40,11 +40,30 @@ struct clksrc_reg {
> >>  };
> >>
> >>  /**
> >> + * struct clkstat_reg - register definition for clock stat bits
> >> + * @reg: pointer to the register in virtual memory.
> >> + * @shift: the shift in bits to where the bitfield is.
> >> + * @stable: the bit value of stable status.
> >> + */
> >> +struct clkstat_reg {
> >> +     void __iomem            *reg;
> >> +     unsigned short          shift;
> >> +     unsigned short          stable;
> >
> > If you want to use this, just 'value' is better.
> > But...I think...we don't need this field now.
> >
> >> +};
> >> +
> >> +/**
> >>   * struct clksrc_clk - class of clock for newer style samsung devices.
> >>   * @clk: the standard clock representation
> >>   * @sources: the sources for this clock
> >>   * @reg_src: the register definition for selecting the clock's source
> >>   * @reg_div: the register definition for the clock's output divisor
> >> + * @reg_src_stable: the register definition to probe if reg_src is
> >> + *    stabilized after the update of reg_src. It is "stabilized" if
> >> + *    reg[shift] == size. If reg == NULL, this stable reg is not looked
> >> + *    up. Thus, in S5PV210, size is usually 0.
> >> + * @reg_div_stable: the register definition to probe if reg_div is
> >> + *    stabilized after the update of reg_div. Same mechanism with
> >> + *    reg_src_stable.
> >>   *
> >>   * This clock implements the features required by the newer SoCs where
> >>   * the standard clock block provides an input mux and a post-mux divisor
> >> @@ -61,6 +80,9 @@ struct clksrc_clk {
> >>
> >>       struct clksrc_reg       reg_src;
> >>       struct clksrc_reg       reg_div;
> >> +
> >> +     struct clkstat_reg      reg_src_stable;
> >> +     struct clkstat_reg      reg_div_stable;
> >>  };
> >>
> >>  /**
> >> --
> >
> >


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH v3] ARM: Samsung SoC: clksrc-clk: wait for the stable SRC/DIV status.

[MyungJoo Ham]

On Tue, Aug 3, 2010 at 9:19 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> On Mon, Aug 2, 2010 at 2:30 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > MyungJoo Ham wrote:
>> >>
>> >> Many MUX and clock dividers have a status bit so that users can wait
>> >> until the status is stable. When corresponding registers are accessed
>> >> while a clock is not stable, we may suffer from unexpected errors.
>> >>
>> >> Therefore, we introduce a mechanism to let the operations related with
>> >> updating SRC/DIV registers of clksrc-clk wait for the stabilization:
>> >> clk_set_parent, clk_set_rate.
>> >>
>> >> In order to use this feature, the definition of clksrc_clk should
>> >> include reg_src_stable or reg_div_stable. With effective rec_src_stable
>> >> values, clk_set_parent returns with a stabilized SRC register and
>> >> with effective rec_div_stable values, clk_set_rate returns with a
>> >> stabilized DIV register. If .reg field is null, its (either SRC or DIV)
>> >> register's status is not checked and returned without waiting; i.e.,
>> >> some MUX/DIV may not need this feature.
>> >>
>> >> When setting reg_*_stable, .size is used to tell the value of "stable".
>> >> If .size = 0, the stable status is 0 and if .size = 1, the stable status
>> >> is 1.
>> >>
>> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> >> --
>> >> v2:
>> >> ? ? ? - Wait-for-stable loop is described at an inline function.
>> >> v3:
>> >> ? ? ? - Stop using clksrc_reg for stable bits. Use "clkstat_reg" for
>> >> ? ? ? ? them.
>> >>
>> >> ---
>> >> ?arch/arm/plat-samsung/clock-clksrc.c ? ? ? ? ? ? ?| ? 14 +++++++++++++
>> >> ?arch/arm/plat-samsung/include/plat/clock-clksrc.h | ? 22
>> >> +++++++++++++++++++++
>> >> ?2 files changed, 36 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/arch/arm/plat-samsung/clock-clksrc.c
>> > b/arch/arm/plat-samsung/clock-
>> >> clksrc.c
>> >> index 46d204a..79ae92e 100644
>> >> --- a/arch/arm/plat-samsung/clock-clksrc.c
>> >> +++ b/arch/arm/plat-samsung/clock-clksrc.c
>> >> @@ -50,6 +50,15 @@ static unsigned long s3c_getrate_clksrc(struct clk
>> > *clk)
>> >> ? ? ? return rate;
>> >> ?}
>> >>
>> >> +static inline void s3c_wait_for_stable(struct clkstat_reg *stat)
>> >> +{
>> >> + ? ? WARN_ON(stat == NULL);
>> >
>> > if (stat == NULL)
>> > ? ? ? ?return;
>>
>> Because .reg_*_stable is included in struct clksrc_clk and
>> s3c_wait_for_stable is called by clk_set_parent and clk_set_rate,
>> which have struct clksrc_clk, stat is unlikely to be NULL and if it's
>> NULL, s3c_wait_for_stable is called from a wrong function and wrong
>> context; thus, I'd use WARN for it rather than silently doing return.
>>
> I don't think that need warning to it, because it is not wrong being NULL.

In this context (caller: s3c_setrate_clksrc, s3c_setparent_clksrc /
callee: s3c_wait_for_stable), it cannot be NULL as long as the callers
are these two. If it's NULL, it means that the function,
s3c_wait_for_stable, is called by someone who's not supposed to call.
In these two callers, &sclk->reg_xxx_stable cannot be NULL as long as
clk is not NULL and if clk is NULL, the thread cannot successfully
reach there.

>
>> >
>> > If don't need to check status, have not become define relevant status
>> > register.
>> >
>> >> + ? ? if (stat->reg) {
>> >
>> > No need?...or need to check stat->reg and stat->shift?
>>
>> If stat->reg is NULL (it can be intentionally NULL), it means that the
>> corresponding struct clksrc_clk does not have stable-stat bit; thus,
>> pass this checking routine. And, stat->shift may be both 0 and
>> positive effectively.
>>
>> stat->reg = 0 : NO STAT REGISTER
>> stat->reg != 0 : STAT Register Exists. Use stat->shift to specify the
>> bit (stat->shift can be 0 or larger)
>>
> If stat != NULL, I think that it does not need...because should being reg.

When stat != NULL, stat->reg may still be NULL intentionally as
mentioned in this paragraph because stat is not going to be NULL if
the owning clksrc_clk is present. Note that stat is a non-pointer
member of clksrc_clk.

>
>> >
>> >> + ? ? ? ? ? ? do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? != stat->stable);
>> >
>> > I'm still wondering we _really_ need 'stable'...just comments is enough,
>> > right now.
>>
>> S5PV210 is fine without stable because stat->stable is always 0 for S5PV210.
>>
>
> Generally, the meaning of stable value may not change....frankly I'm not sure...
> but if change, then we can add that later...so right now, unnecessary stuff like that does not need.
>
> Then...and no need to add 'struct clkstat_reg'

Yes, if the definition of stable value does not change, "stable" is
not needed and only shift and reg values are required.

>
>> However, this is for "Samsung-SoC" not for "S5PV210" although only
>> S5PV210 uses this feature for now.
>>
>> I've added stat->stable for the generality. Would it be better discard
>> such generality and assume that other Samsung-SoC systems will also
>> use the same stat bit scheme for every register (0 for stable, 1 for
>> changing for every STAT register) with S5PV210? If we can guarantee
>> such homogeneousity, getting rid of stable property would be perfectly
>> fine. It is added just because I couldn't sure about Samsung-SoC chips
>> other than S5PV210. (and I thought some chips may use different scheme
>> for each register; e.g., some registers use 0 for stable, some others
>> use 1 for stable)
>>
>> >
>> >> + ? ? ? ? ? ? do { } while (((__raw_readl(stat->reg) >> stat->shift) & 1)
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? != stat->stable);
>> >
>> >> + ? ? }
>> >> +}
>> >> +
>> >> ?static int s3c_setrate_clksrc(struct clk *clk, unsigned long rate)
>> >> ?{
>> >> ? ? ? struct clksrc_clk *sclk = to_clksrc(clk);
>> >> @@ -68,6 +77,8 @@ static int s3c_setrate_clksrc(struct clk *clk, unsigned
>> > long rate)
>> >> ? ? ? val |= (div - 1) << sclk->reg_div.shift;
>> >> ? ? ? __raw_writel(val, reg);
>> >>
>> >> + ? ? s3c_wait_for_stable(&sclk->reg_div_stable);
>> >> +
>> >> ? ? ? return 0;
>> >> ?}
>> >>
>> >> @@ -93,6 +104,9 @@ static int s3c_setparent_clksrc(struct clk *clk, struct
>> > clk
>> >> *parent)
>> >> ? ? ? ? ? ? ? clksrc |= src_nr << sclk->reg_src.shift;
>> >>
>> >> ? ? ? ? ? ? ? __raw_writel(clksrc, sclk->reg_src.reg);
>> >> +
>> >> + ? ? ? ? ? ? s3c_wait_for_stable(&sclk->reg_src_stable);
>> >> +
>> >> ? ? ? ? ? ? ? return 0;
>> >> ? ? ? }
>> >>
>> >> diff --git a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
>> > b/arch/arm/plat-
>> >> samsung/include/plat/clock-clksrc.h
>> >> index 50a8ca7..f1d54da 100644
>> >> --- a/arch/arm/plat-samsung/include/plat/clock-clksrc.h
>> >> +++ b/arch/arm/plat-samsung/include/plat/clock-clksrc.h
>> >> @@ -40,11 +40,30 @@ struct clksrc_reg {
>> >> ?};
>> >>
>> >> ?/**
>> >> + * struct clkstat_reg - register definition for clock stat bits
>> >> + * @reg: pointer to the register in virtual memory.
>> >> + * @shift: the shift in bits to where the bitfield is.
>> >> + * @stable: the bit value of stable status.
>> >> + */
>> >> +struct clkstat_reg {
>> >> + ? ? void __iomem ? ? ? ? ? ?*reg;
>> >> + ? ? unsigned short ? ? ? ? ?shift;
>> >> + ? ? unsigned short ? ? ? ? ?stable;
>> >
>> > If you want to use this, just 'value' is better.
>> > But...I think...we don't need this field now.
>> >
>> >> +};
>> >> +
>> >> +/**
>> >> ? * struct clksrc_clk - class of clock for newer style samsung devices.
>> >> ? * @clk: the standard clock representation
>> >> ? * @sources: the sources for this clock
>> >> ? * @reg_src: the register definition for selecting the clock's source
>> >> ? * @reg_div: the register definition for the clock's output divisor
>> >> + * @reg_src_stable: the register definition to probe if reg_src is
>> >> + * ? ?stabilized after the update of reg_src. It is "stabilized" if
>> >> + * ? ?reg[shift] == size. If reg == NULL, this stable reg is not looked
>> >> + * ? ?up. Thus, in S5PV210, size is usually 0.
>> >> + * @reg_div_stable: the register definition to probe if reg_div is
>> >> + * ? ?stabilized after the update of reg_div. Same mechanism with
>> >> + * ? ?reg_src_stable.
>> >> ? *
>> >> ? * This clock implements the features required by the newer SoCs where
>> >> ? * the standard clock block provides an input mux and a post-mux divisor
>> >> @@ -61,6 +80,9 @@ struct clksrc_clk {
>> >>
>> >> ? ? ? struct clksrc_reg ? ? ? reg_src;
>> >> ? ? ? struct clksrc_reg ? ? ? reg_div;
>> >> +
>> >> + ? ? struct clkstat_reg ? ? ?reg_src_stable;
>> >> + ? ? struct clkstat_reg ? ? ?reg_div_stable;
>> >> ?};
>> >>
>> >> ?/**
>> >> --
>> >
>> >
>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858