[PATCH v3] ARM: Samsung SoC: clksrc-clk: wait for the stable SRC/DIV status.
MyungJoo Ham
myungjoo.ham at samsung.com
Tue Aug 3 05:29:34 EDT 2010
On Tue, Aug 3, 2010 at 9:19 AM, Kukjin Kim <kgene.kim at samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> On Mon, Aug 2, 2010 at 2:30 PM, Kukjin Kim <kgene.kim at 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 at samsung.com>
>> >> Signed-off-by: Kyungmin Park <kyungmin.park at 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 at 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
More information about the linux-arm-kernel
mailing list