clk: bcm: prerequisite and bus clock support
Subsystems (5)
v4 cover clk: bcm: prerequisite and bus clock support
Currently only peripheral clocks are supported for Broadcom platforms that use Kona style CCUs for clocking. This series adds support for bus clocks as well.
One motivation for doing this is that there exist peripheral clocks that cannot be configured without having first enabled a related bus clock. Adding bus clock support allows such peripheral clocks to be usable.
This also imposes a new requirement, however--that the bus clock be enabled *before* operating on the clock that depends on it. For this, we define the notion of a "prerequisite" clock. If a clock has a prerequisite specified, that prequisite clock will be prepared and enabled at the beginning of the dependent clock's ->prepare method. The prerequisite clock is similarly disabled and unprepared at the end of the dependent clock's ->unprepare method.
These patches are based on Mike Turquette's current "clk-next" branch.
9ec2749 clk: qcom: Return error pointers for unimplemented clocks
They are available here:
http://git.linaro.org/landing-teams/working/broadcom/kernel.git
Branch review/bcm-bus-clk-v4
-Alex
Version history:
v4 - Remove boot-time initialization of clocks, opting instead to
do this work when needed, via clock ->prepare method.
- Prerequisite clocks are now prepared and enabled as in a dependent clock's ->prepare method.
- Use __clk_lookup() to get the prerequisite clock pointer.
- ccu_write_enable() calls can now be nested
v3: - Deleted clk_lookup field from struct kona_clk.
- Now use term "initialized" rather than "enabled".
v2: - Added field "p" to the previously unnamed prereq union.
Alex Elder (7):
clk: kona: allow nested ccu_write_enable() requests
clk: kona: move some code
clk: kona: don't init clocks at startup time
clk: bcm281xx: implement prerequisite clocks
clk: bcm281xx: add bus clock support
clk: bcm281xx: define a bus clock
ARM: dts: add bus clock bsc3_apb for bcm281xx
arch/arm/boot/dts/bcm11351.dtsi | 3 +- drivers/clk/bcm/clk-bcm281xx.c | 13 +- drivers/clk/bcm/clk-kona-setup.c | 123 ++++++++++++- drivers/clk/bcm/clk-kona.c | 325 ++++++++++++++++++++++++----------- drivers/clk/bcm/clk-kona.h | 31 +++- include/dt-bindings/clock/bcm281xx.h | 3 +- 6 files changed, 387 insertions(+), 111 deletions(-)
Signature · 1 line
1.9.1
PATCH v4 1/7 clk: kona: allow nested ccu_write_enable() requests
Use a counter rather than a Boolean to track whether write access to a CCU has been enabled or not. This will allow more than one of these requests to be nested.
Note that __ccu_write_enable() and __ccu_write_disable() calls all come in pairs, and they are always surrounded immediately by calls to ccu_lock() and ccu_unlock().
--- drivers/clk/bcm/clk-kona.c | 14 ++++---------- drivers/clk/bcm/clk-kona.h | 2 +- 2 files changed, 5 insertions(+), 11 deletions(-)
drivers/clk/bcm/clk-kona.c +4 -10
@@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)*/ static inline void __ccu_write_enable (struct ccu_data *ccu) { -if (ccu->write_enabled) { -pr_err ("%s: access already enabled for %s\n" , __func__, - ccu->name); -return ; - } - ccu->write_enabled = true; -__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD |1 ); +if (!ccu->write_enabled++) +__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD |1 ); }static inline void __ccu_write_disable (struct ccu_data *ccu)
@@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu) ccu->name);return ; } - -__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD ); - ccu->write_enabled = false; +if (!--ccu->write_enabled) +__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD ); }/*
drivers/clk/bcm/clk-kona.h +1 -1
@@ -478,7 +478,7 @@ struct ccu_policy {struct ccu_data {void __iomem *base;/* base of mapped address space */ spinlock_t lock;/* serialization lock */ -bool write_enabled;/* write access is currently enabled */ +u32 write_enabled;/* write access enable count */ struct ccu_policy policy;struct list_head links;/* for ccu_list */ struct device_node *node;
Signature · 1 line
1.9.1
Re: [PATCH v4 1/7] clk: kona: allow nested ccu_write_enable() requests
Quoting Alex Elder (2014-05-30 13:53:02)
Quoted 19 lines
Use a counter rather than a Boolean to track whether write access to a CCU has been enabled or not. This will allow more than one of these requests to be nested.
Note that
__ccu_write_enable()and__ccu_write_disable()calls all come in pairs, and they are always surrounded immediately by calls toccu_lock()andccu_unlock().Signed-off-by Alex Elder <[email protected]>--- drivers/clk/bcm/clk-kona.c | 14 ++++---------- drivers/clk/bcm/clk-kona.h | 2 +- 2 files changed, 5 insertions(+), 11 deletions(-)drivers/clk/bcm/clk-kona.c +0
-0
drivers/clk/bcm/clk-kona.c static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags) · line 171
Quoted 2 lines
drivers/clk/bcm/clk-kona.c +0
-0@@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)*/ static inline void __ccu_write_enable (struct ccu_data *ccu)
Per Documentation/CodingStyle, chapter 15, "the inline disease", it might be best to not inline these functions.
Quoted 14 lines
drivers/clk/bcm/clk-kona.c +2
-7{ -if (ccu->write_enabled) { -pr_err ("%s: access already enabled for %s\n" , __func__, - ccu->name); -return ; - } - ccu->write_enabled = true; -__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD |1 ); +if (!ccu->write_enabled++) +__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD |1 ); }static inline void __ccu_write_disable (struct ccu_data *ccu)
drivers/clk/bcm/clk-kona.c static inline void __ccu_write_disable(struct ccu_data *ccu) · line 185
Quoted 8 lines
drivers/clk/bcm/clk-kona.c +2
-3@@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu) ccu->name);return ; } - -__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD ); - ccu->write_enabled = false; +if (!--ccu->write_enabled) +__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD );
What happens if calls to __ccu_write_enable and __ccu_write_disable are unbalanced? It would be better to catch that case and throw a WARN:
if (WARN_ON(ccu->write_enabled == 0)) return; if (--ccu->write_enabled > 0) return; __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
drivers/clk/bcm/clk-kona.h struct ccu_policy { · line 481
Quoted 13 lines
drivers/clk/bcm/clk-kona.c +0
-0}/* drivers/clk/bcm/clk-kona.h +1
-1@@ -478,7 +478,7 @@ struct ccu_policy {struct ccu_data {void __iomem *base;/* base of mapped address space */ spinlock_t lock;/* serialization lock */ -bool write_enabled;/* write access is currently enabled */ +u32 write_enabled;/* write access enable count */
Why u32? An unsigned int will do just nicely here.
Regards,
Mike
Quoted 6 lines
drivers/clk/bcm/clk-kona.h +0
-0struct ccu_policy policy;struct list_head links;/* for ccu_list */ struct device_node *node;Signature · 1 line
1.9.1
Re: [PATCH v4 1/7] clk: kona: allow nested ccu_write_enable() requests
Quoted 15 lines · Mike Turquette
Quoting Alex Elder (2014-05-30 13:53:02)
Quoted 14 lines
Use a counter rather than a Boolean to track whether write access to a CCU has been enabled or not. This will allow more than one of these requests to be nested.
Note that
__ccu_write_enable()and__ccu_write_disable()calls all come in pairs, and they are always surrounded immediately by calls toccu_lock()andccu_unlock().Signed-off-by Alex Elder <[email protected]>--- drivers/clk/bcm/clk-kona.c | 14 ++++---------- drivers/clk/bcm/clk-kona.h | 2 +- 2 files changed, 5 insertions(+), 11 deletions(-)
Quoted 10 lines
drivers/clk/bcm/clk-kona.c static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags) · line 171
Quoted 7 lines
drivers/clk/bcm/clk-kona.c +0
-0@@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)*/ static inline void __ccu_write_enable (struct ccu_data *ccu)Per
Documentation/CodingStyle, chapter 15, "the inline disease", it might be best to not inline these functions.
This was not intentional. I normally only inline things defined in header files, and maybe this is an artifact of having been in a header at one time. I don't know, I'll get rid of the inline.
Quoted 13 lines
Quoted 13 lines
drivers/clk/bcm/clk-kona.c +2
-7{ -if (ccu->write_enabled) { -pr_err ("%s: access already enabled for %s\n" , __func__, - ccu->name); -return ; - } - ccu->write_enabled = true; -__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD |1 ); +if (!ccu->write_enabled++) +__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD |1 ); }
Quoted 13 lines
drivers/clk/bcm/clk-kona.c static inline void __ccu_write_disable(struct ccu_data *ccu) · line 185
Quoted 10 lines
drivers/clk/bcm/clk-kona.c +2
-3static inline void __ccu_write_disable (struct ccu_data *ccu)@@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu) ccu->name);return ; } - -__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD ); - ccu->write_enabled = false; +if (!--ccu->write_enabled) +__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD );What happens if calls to
__ccu_write_enableand__ccu_write_disableare unbalanced? It would be better to catch that case and throw a WARN:
You can't see it in the diff, but that's what happens
(well, it's a pr_err(), not a WARN()). I think a WARN()
is probably right in this case though.
Quoted 10 lines
if (WARN_ON(ccu->write_enabled == 0)) return; if (--ccu->write_enabled > 0) return; __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);Quoted 2 lines
drivers/clk/bcm/clk-kona.c +0
-0}
Quoted 13 lines
drivers/clk/bcm/clk-kona.h struct ccu_policy { · line 481
Quoted 11 lines
drivers/clk/bcm/clk-kona.c +0
-0/* drivers/clk/bcm/clk-kona.h +1
-1@@ -478,7 +478,7 @@ struct ccu_policy {struct ccu_data {void __iomem *base;/* base of mapped address space */ spinlock_t lock;/* serialization lock */ -bool write_enabled;/* write access is currently enabled */ +u32 write_enabled;/* write access enable count */ Why u32? An unsigned int will do just nicely here.
That's a preference of mine. I almost always favor using u32, etc. because they are compact, and explicit about the size and signedness. I "know" an int is 32 bits, but I still prefer being explicit.
I'll interpret this as a preference on your part for unsigned int, and I have no problem making that change.
-Alex
Quoted 9 lines
Regards,
MikeQuoted 6 lines
drivers/clk/bcm/clk-kona.h +0
-0struct ccu_policy policy;struct list_head links;/* for ccu_list */ struct device_node *node;Signature · 1 line
1.9.1
Re: [PATCH v4 1/7] clk: kona: allow nested ccu_write_enable() requests
Quoting Alex Elder (2014-05-30 20:46:46)
Quoted 87 lines · Mike Turquette
Quoted 15 lines
Quoting Alex Elder (2014-05-30 13:53:02)
Quoted 14 lines
Use a counter rather than a Boolean to track whether write access to a CCU has been enabled or not. This will allow more than one of these requests to be nested.
Note that
__ccu_write_enable()and__ccu_write_disable()calls all come in pairs, and they are always surrounded immediately by calls toccu_lock()andccu_unlock().Signed-off-by Alex Elder <[email protected]>--- drivers/clk/bcm/clk-kona.c | 14 ++++---------- drivers/clk/bcm/clk-kona.h | 2 +- 2 files changed, 5 insertions(+), 11 deletions(-)Quoted 10 lines
drivers/clk/bcm/clk-kona.c static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags) · line 171
Quoted 7 lines
drivers/clk/bcm/clk-kona.c +0
-0@@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)*/ static inline void __ccu_write_enable (struct ccu_data *ccu)Per
Documentation/CodingStyle, chapter 15, "the inline disease", it might be best to not inline these functions.This was not intentional. I normally only inline things defined in header files, and maybe this is an artifact of having been in a header at one time. I don't know, I'll get rid of the inline.
Quoted 13 lines
Quoted 13 lines
drivers/clk/bcm/clk-kona.c +2
-7{ -if (ccu->write_enabled) { -pr_err ("%s: access already enabled for %s\n" , __func__, - ccu->name); -return ; - } - ccu->write_enabled = true; -__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD |1 ); +if (!ccu->write_enabled++) +__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD |1 ); }Quoted 13 lines
drivers/clk/bcm/clk-kona.c static inline void __ccu_write_disable(struct ccu_data *ccu) · line 185
Quoted 10 lines
drivers/clk/bcm/clk-kona.c +2
-3static inline void __ccu_write_disable (struct ccu_data *ccu)@@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu) ccu->name);return ; } - -__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD ); - ccu->write_enabled = false; +if (!--ccu->write_enabled) +__ccu_write (ccu,0 ,CCU_ACCESS_PASSWORD );What happens if calls to
__ccu_write_enableand__ccu_write_disableare unbalanced? It would be better to catch that case and throw a WARN:You can't see it in the diff, but that's what happens
(well, it's apr_err(), not aWARN()). I think aWARN()
is probably right in this case though.Quoted 10 lines
if (WARN_ON(ccu->write_enabled == 0)) return; if (--ccu->write_enabled > 0) return; __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);Quoted 2 lines
drivers/clk/bcm/clk-kona.c +0
-0}Quoted 13 lines
drivers/clk/bcm/clk-kona.h struct ccu_policy { · line 481
Quoted 11 lines
drivers/clk/bcm/clk-kona.c +0
-0/* drivers/clk/bcm/clk-kona.h +1
-1@@ -478,7 +478,7 @@ struct ccu_policy {struct ccu_data {void __iomem *base;/* base of mapped address space */ spinlock_t lock;/* serialization lock */ -bool write_enabled;/* write access is currently enabled */ +u32 write_enabled;/* write access enable count */ Why u32? An unsigned int will do just nicely here.
Quoted 7 lines
That's a preference of mine. I almost always favor using u32, etc. because they are compact, and explicit about the size and signedness. I "know" an int is 32 bits, but I still prefer being explicit.
I'll interpret this as a preference on your part for unsigned int, and I have no problem making that change.
It's not a big deal, I was just curious why. Feel free to use whatever solution you prefer here.
Regards,
Mike
Quoted 13 lines
-Alex
Quoted 9 lines
Regards,
MikeQuoted 6 lines
drivers/clk/bcm/clk-kona.h +0
-0struct ccu_policy policy;struct list_head links;/* for ccu_list */ struct device_node *node;Signature · 1 line
1.9.1
PATCH v4 2/7 clk: kona: move some code
In reply to Alex ElderMove the definition of __peri_clk_init() up in "clk-kona.c", in preparation for the next patch (so it's defined before it's needed). Move kona_ccu_init() and __kona_clk_init() as well.
Done as a separate patch so doing so doesn't obscure other changes.
--- drivers/clk/bcm/clk-kona.c | 174 ++++++++++++++++++++++----------------------- 1 file changed, 87 insertions(+), 87 deletions(-)
drivers/clk/bcm/clk-kona.c +87 -87
@@ -975,6 +975,93 @@ static int selector_write(struct ccu_data *ccu, struct bcm_clk_gate *gate,return ret; } +/* Put a peripheral clock into its initial state */ +static bool __peri_clk_init (struct kona_clk *bcm_clk) +{ +struct ccu_data *ccu = bcm_clk->ccu; +struct peri_clk_data *peri = bcm_clk->u.peri; +const char *name = bcm_clk->init_data.name; +struct bcm_clk_trig *trig; + +BUG_ON (bcm_clk->type != bcm_clk_peri); + +if (!policy_init (ccu, &peri->policy)) { +pr_err ("%s: error initializing policy for %s\n" , + __func__, name); +return false; + } +if (!gate_init (ccu, &peri->gate)) { +pr_err ("%s: error initializing gate for %s\n" , __func__, name); +return false; + } +if (!hyst_init (ccu, &peri->hyst)) { +pr_err ("%s: error initializing hyst for %s\n" , __func__, name); +return false; + } +if (!div_init (ccu, &peri->gate, &peri->div, &peri->trig)) { +pr_err ("%s: error initializing divider for %s\n" , __func__, + name); +return false; + } + +/* +* For the pre-divider and selector, the pre-trigger is used +* if it's present, otherwise we just use the regular trigger. +*/ + trig =trigger_exists (&peri->pre_trig) ? &peri->pre_trig + : &peri->trig; + +if (!div_init (ccu, &peri->gate, &peri->pre_div, trig)) { +pr_err ("%s: error initializing pre-divider for %s\n" , __func__, + name); +return false; + } + +if (!sel_init (ccu, &peri->gate, &peri->sel, trig)) { +pr_err ("%s: error initializing selector for %s\n" , __func__, + name); +return false; + } + +return true; +} + +static bool __kona_clk_init (struct kona_clk *bcm_clk) +{ +switch (bcm_clk->type) { +case bcm_clk_peri: +return __peri_clk_init (bcm_clk); +default : +BUG (); + } +return -EINVAL ; +} + +/* Set a CCU and all its clocks into their desired initial state */ +bool __init kona_ccu_init (struct ccu_data *ccu) +{ +unsigned long flags; +unsigned int which; +struct clk **clks = ccu->clk_data.clks; +bool success = true; + + flags =ccu_lock (ccu); +__ccu_write_enable (ccu); + +for (which =0 ; which < ccu->clk_data.clk_num; which++) { +struct kona_clk *bcm_clk; + +if (!clks[which]) +continue ; + bcm_clk =to_kona_clk (__clk_get_hw (clks[which])); + success &=__kona_clk_init (bcm_clk); + } + +__ccu_write_disable (ccu); +ccu_unlock (ccu, flags); +return success; +} +/* Clock operations */ static int kona_peri_clk_enable (struct clk_hw *hw)
@@ -1186,90 +1273,3 @@ struct clk_ops kona_peri_clk_ops = { .get_parent = kona_peri_clk_get_parent, .set_rate = kona_peri_clk_set_rate, }; - -/* Put a peripheral clock into its initial state */ -static bool __peri_clk_init (struct kona_clk *bcm_clk) -{ -struct ccu_data *ccu = bcm_clk->ccu; -struct peri_clk_data *peri = bcm_clk->u.peri; -const char *name = bcm_clk->init_data.name; -struct bcm_clk_trig *trig; - -BUG_ON (bcm_clk->type != bcm_clk_peri); - -if (!policy_init (ccu, &peri->policy)) { -pr_err ("%s: error initializing policy for %s\n" , - __func__, name); -return false; - } -if (!gate_init (ccu, &peri->gate)) { -pr_err ("%s: error initializing gate for %s\n" , __func__, name); -return false; - } -if (!hyst_init (ccu, &peri->hyst)) { -pr_err ("%s: error initializing hyst for %s\n" , __func__, name); -return false; - } -if (!div_init (ccu, &peri->gate, &peri->div, &peri->trig)) { -pr_err ("%s: error initializing divider for %s\n" , __func__, - name); -return false; - } - -/* -* For the pre-divider and selector, the pre-trigger is used -* if it's present, otherwise we just use the regular trigger. -*/ - trig =trigger_exists (&peri->pre_trig) ? &peri->pre_trig - : &peri->trig; - -if (!div_init (ccu, &peri->gate, &peri->pre_div, trig)) { -pr_err ("%s: error initializing pre-divider for %s\n" , __func__, - name); -return false; - } - -if (!sel_init (ccu, &peri->gate, &peri->sel, trig)) { -pr_err ("%s: error initializing selector for %s\n" , __func__, - name); -return false; - } - -return true; -} - -static bool __kona_clk_init (struct kona_clk *bcm_clk) -{ -switch (bcm_clk->type) { -case bcm_clk_peri: -return __peri_clk_init (bcm_clk); -default : -BUG (); - } -return -EINVAL ; -} - -/* Set a CCU and all its clocks into their desired initial state */ -bool __init kona_ccu_init (struct ccu_data *ccu) -{ -unsigned long flags; -unsigned int which; -struct clk **clks = ccu->clk_data.clks; -bool success = true; - - flags =ccu_lock (ccu); -__ccu_write_enable (ccu); - -for (which =0 ; which < ccu->clk_data.clk_num; which++) { -struct kona_clk *bcm_clk; - -if (!clks[which]) -continue ; - bcm_clk =to_kona_clk (__clk_get_hw (clks[which])); - success &=__kona_clk_init (bcm_clk); - } - -__ccu_write_disable (ccu); -ccu_unlock (ccu, flags); -return success; -}
Signature · 1 line
1.9.1
PATCH v4 3/7 clk: kona: don't init clocks at startup time
In reply to Alex ElderThe last thing done for CCU setup is a call to kona_ccu_init(). This locks and enables write access on the CCU, then calls __kona_clk_init() for all of the clocks it provides. The purpose of this was to get or set the initial state of all the registers related to each clock.
There's no reason to do this though. The common clock framework assumes nothing about the state of the hardware, and if a driver requires its clock to be in a particular state it can set that state up itself.
Instead of initializing clocks at startup time, define a prepare method that performs that step when a clock is first needed.
--- drivers/clk/bcm/clk-kona-setup.c | 3 --- drivers/clk/bcm/clk-kona.c | 48 ++++++++++++++++++---------------------- drivers/clk/bcm/clk-kona.h | 1 - 3 files changed, 22 insertions(+), 30 deletions(-)
drivers/clk/bcm/clk-kona-setup.c +0 -3
@@ -867,9 +867,6 @@ void __init kona_dt_ccu_setup(struct ccu_data *ccu, goto out_err; } -if (!kona_ccu_init (ccu)) -pr_err ("Broadcom %s initialization had errors\n" , node->name); -return ; out_err:kona_ccu_teardown (ccu);
drivers/clk/bcm/clk-kona.c +22 -26
71 lines not on this page · raw message
drivers/clk/bcm/clk-kona.h +0 -1
@@ -511,6 +511,5 @@ extern u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value,extern struct clk *kona_clk_setup (struct kona_clk *bcm_clk);extern void __init kona_dt_ccu_setup (struct ccu_data *ccu,struct device_node *node); -extern bool __init kona_ccu_init (struct ccu_data *ccu);#endif /* _CLK_KONA_H */
Signature · 1 line
1.9.1
Re: [PATCH v4 3/7] clk: kona: don't init clocks at startup time
Quoting Alex Elder (2014-05-30 13:53:04)
Quoted 9 lines
drivers/clk/bcm/clk-kona.c +8
-0+static int kona_clk_prepare (struct clk_hw *hw) { +struct kona_clk *bcm_clk =to_kona_clk (hw); +struct ccu_data *ccu = bcm_clk->ccu; +unsigned long flags; +int ret =0 ; + + flags =ccu_lock (ccu); +__ccu_write_enable (ccu);
drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1048
Quoted 10 lines
drivers/clk/bcm/clk-kona.c +4
-1+switch (bcm_clk->type) {case bcm_clk_peri: -return __peri_clk_init (bcm_clk); +if (!__peri_clk_init (bcm_clk)) + ret = -EINVAL ; +break ;default :BUG (); }
The switch-case only has one match, plus a default. Will there be others in the future? Otherwise it can be replaced with an if-statement.
Quoted 19 lines
drivers/clk/bcm/clk-kona.c +0
-19-return -EINVAL ; -} - -/* Set a CCU and all its clocks into their desired initial state */ -bool __init kona_ccu_init (struct ccu_data *ccu) -{ -unsigned long flags; -unsigned int which; -struct clk **clks = ccu->clk_data.clks; -bool success = true; - - flags =ccu_lock (ccu); -__ccu_write_enable (ccu); - -for (which =0 ; which < ccu->clk_data.clk_num; which++) { -struct kona_clk *bcm_clk; - -if (!clks[which]) -continue ;
drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1054
Quoted 10 lines
drivers/clk/bcm/clk-kona.c +2
-4- bcm_clk =to_kona_clk (__clk_get_hw (clks[which])); - success &=__kona_clk_init (bcm_clk); - }__ccu_write_disable (ccu);ccu_unlock (ccu, flags); -return success; + +return ret; }
Does this prepare callback "enable" a clock? E.g does a line NOT toggle at a rate prior to this call, and then after this call completes that same line is now toggling at a rate?
drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1059
Quoted 6 lines
drivers/clk/bcm/clk-kona.c +4
-1-/* Clock operations */ +static void kona_clk_unprepare (struct clk_hw *hw) +{ +/* Nothing to do. */ +}
Is doing nothing the right thing to do? Could power be saved somehow if the .unprepare callback really gets called? Remember that if .unprepare actually runs (because struct clk->prepare_count == 0) then the next call to clk_prepare will actually call your .prepare callback and set up the prereq clocks again. So the prereq clock initialization is no longer a one-time thing, which might afford you some optimizations.
Regards,
Mike
Quoted 26 lines
static int
kona_peri_clk_enable(struct clk_hw *hw){drivers/clk/bcm/clk-kona.c +2
-0@@ -1264,6 +1258,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate, }struct clk_ops kona_peri_clk_ops = { + .prepare = kona_clk_prepare, + .unprepare = kona_clk_unprepare, .enable = kona_peri_clk_enable, .disable = kona_peri_clk_disable, .is_enabled = kona_peri_clk_is_enabled,drivers/clk/bcm/clk-kona.h +0
-1@@ -511,6 +511,5 @@ extern u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value,extern struct clk *kona_clk_setup (struct kona_clk *bcm_clk);extern void __init kona_dt_ccu_setup (struct ccu_data *ccu,struct device_node *node); -extern bool __init kona_ccu_init (struct ccu_data *ccu);#endif /* _CLK_KONA_H */ Signature · 1 line
1.9.1
Re: [PATCH v4 3/7] clk: kona: don't init clocks at startup time
Quoted 15 lines · Mike Turquette
Quoting Alex Elder (2014-05-30 13:53:04)
Quoted 14 lines
drivers/clk/bcm/clk-kona.c +10
-1+static int kona_clk_prepare (struct clk_hw *hw) { +struct kona_clk *bcm_clk =to_kona_clk (hw); +struct ccu_data *ccu = bcm_clk->ccu; +unsigned long flags; +int ret =0 ; + + flags =ccu_lock (ccu); +__ccu_write_enable (ccu); +switch (bcm_clk->type) {case bcm_clk_peri: -return __peri_clk_init (bcm_clk); +if (!__peri_clk_init (bcm_clk))
Quoted 8 lines
drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1048
Quoted 5 lines
drivers/clk/bcm/clk-kona.c +2
-0+ ret = -EINVAL ; +break ;default :BUG (); }The switch-case only has one match, plus a default. Will there be others in the future? Otherwise it can be replaced with an if-statement.
Yes, bus clocks in patch 5/7. Maybe another type someday but at least these two. All the code is structured this way in anticipation of other clock types.
Quoted 23 lines
Quoted 23 lines
drivers/clk/bcm/clk-kona.c +0
-22-return -EINVAL ; -} - -/* Set a CCU and all its clocks into their desired initial state */ -bool __init kona_ccu_init (struct ccu_data *ccu) -{ -unsigned long flags; -unsigned int which; -struct clk **clks = ccu->clk_data.clks; -bool success = true; - - flags =ccu_lock (ccu); -__ccu_write_enable (ccu); - -for (which =0 ; which < ccu->clk_data.clk_num; which++) { -struct kona_clk *bcm_clk; - -if (!clks[which]) -continue ; - bcm_clk =to_kona_clk (__clk_get_hw (clks[which])); - success &=__kona_clk_init (bcm_clk); - }
Quoted 10 lines
drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1054
Quoted 6 lines
__ccu_write_disable(ccu); ccu_unlock(ccu, flags);drivers/clk/bcm/clk-kona.c +2
-1-return success; + +return ret; }Does this prepare callback "enable" a clock? E.g does a line NOT toggle at a rate prior to this call, and then after this call completes that same line is now toggling at a rate?
Hmmm. Good question.
The state of the gate (if present) will not change, so it'll continue toggling if it was before, and will not start if it was not. But once this is done we'll know whether the hardware is enabled or not because we'll have read its value.
Quoted 9 lines
drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1059
Quoted 6 lines
drivers/clk/bcm/clk-kona.c +4
-1-/* Clock operations */ +static void kona_clk_unprepare (struct clk_hw *hw) +{ +/* Nothing to do. */ +}Is doing nothing the right thing to do? Could power be saved somehow if the .unprepare callback really gets called? Remember that if .unprepare
This is again a skeleton in place now because the prerequisite code will actually fill this in with something to do. But in any case I use the enable/disable methods for gate control; the main purpose for these prepare methods will be to prepare and enable the prerequisite clock. I may need to think a bit more about what belongs in the prepare function
Quoted 4 lines
actually runs (because struct
clk->prepare_count== 0) then the next call toclk_preparewill actually call your .prepare callback and set up the prereq clocks again. So the prereq clock initialization is no longer a one-time thing, which might afford you some optimizations.
I'll think about this. Already the code caches the current state of the hardware and doesn't update the hardware unless it changes that state. If I maintained an initialized flag (which I just got rid of) I could probably avoid taking the spin lock, etc., for some savings.
This switch over to using the prepare method to do the initialization may not have been as well thought out as it should have been. It changes from "initialize all clocks, once, all at once" to "initialize each clock only when it's needed (but every time it's needed)."
-Alex
Quoted 29 lines
Regards,
MikeQuoted 26 lines
static int
kona_peri_clk_enable(struct clk_hw *hw){drivers/clk/bcm/clk-kona.c +2
-0@@ -1264,6 +1258,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate, }struct clk_ops kona_peri_clk_ops = { + .prepare = kona_clk_prepare, + .unprepare = kona_clk_unprepare, .enable = kona_peri_clk_enable, .disable = kona_peri_clk_disable, .is_enabled = kona_peri_clk_is_enabled,drivers/clk/bcm/clk-kona.h +0
-1@@ -511,6 +511,5 @@ extern u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value,extern struct clk *kona_clk_setup (struct kona_clk *bcm_clk);extern void __init kona_dt_ccu_setup (struct ccu_data *ccu,struct device_node *node); -extern bool __init kona_ccu_init (struct ccu_data *ccu);#endif /* _CLK_KONA_H */ Signature · 1 line
1.9.1
PATCH v4 4/7 clk: bcm281xx: implement prerequisite clocks
In reply to Alex ElderAllow a clock to specify a "prerequisite" clock, identified by its name. The prerequisite clock must be prepared and enabled before a clock that depends on it is used. In order to simplify locking, we require a clock and its prerequisite to be associated with the same CCU. (We'll just trust--but not verify--that nobody defines a cycle of prerequisite clocks.)
Rework the KONA_CLK() macro, and define a new KONA_CLK_PREREQ() variant that allows a prerequisite clock to be specified.
--- drivers/clk/bcm/clk-kona-setup.c | 24 ++++++++++++ drivers/clk/bcm/clk-kona.c | 84 +++++++++++++++++++++++++++++++++++++++- drivers/clk/bcm/clk-kona.h | 20 ++++++++-- 3 files changed, 123 insertions(+), 5 deletions(-)
drivers/clk/bcm/clk-kona-setup.c +24 -0
@@ -796,6 +796,28 @@ static bool ccu_data_valid(struct ccu_data *ccu) }/* +* If a clock specifies a prerequisite clock, they must both be on +* the same CCU. Check if the named prerequisite clock matches one +* of the ones provided by CCU. A null name means no prerequisite. +*/ +static bool kona_prereq_valid (struct ccu_data *ccu,const char *prereq_name) +{ +unsigned int i; + +if (!prereq_name) +return true; + +for (i =0 ; i < ccu->clk_data.clk_num; i++) +if (!strcmp (prereq_name, ccu->kona_clks[i].init_data.name)) +return true; + +pr_err ("%s: prereq clock %s not defined for ccu %s\n" , __func__, + prereq_name, ccu->name); + +return false; +} + +/* * Set up a CCU. Call the provided ccu_clks_setup callback to * initialize the array of clocks provided by the CCU. */
@@ -857,6 +879,8 @@ void __init kona_dt_ccu_setup(struct ccu_data *ccu,for (i =0 ; i < ccu->clk_data.clk_num; i++) {if (!ccu->kona_clks[i].ccu)continue ; +if (!kona_prereq_valid (ccu, ccu->kona_clks[i].prereq.name)) +continue ; ccu->clk_data.clks[i] =kona_clk_setup (&ccu->kona_clks[i]); }
drivers/clk/bcm/clk-kona.c +82 -2
110 lines not on this page · raw message
drivers/clk/bcm/clk-kona.h +17 -3
@@ -406,6 +406,10 @@ struct kona_clk {struct clk_init_data init_data;/* includes name of this clock */ struct ccu_data *ccu;/* ccu this clock is associated with */ enum bcm_clk_type type; +struct { +const char *name; +struct clk *clk; + } prereq;union {void *data;struct peri_clk_data *peri;
@@ -415,16 +419,26 @@ struct kona_clk { container_of(_hw, struct kona_clk, hw) /* Initialization macro for an entry in a CCU's kona_clks[] array. */ -#define KONA_CLK (_ccu_name, _clk_name, _type) \ - { \ +#define ___KONA_CLK_COMMON (_ccu_name, _clk_name, _type) \ .init_data = { \ .name = #_clk_name, \ .ops = &kona_ ## _type ## _clk_ops, \ }, \ .ccu = &_ccu_name ## _ccu_data, \ .type = bcm_clk_ ## _type, \ - .u.data = &_clk_name ## _data, \ + .u.data = &_clk_name ## _data + +#define KONA_CLK_PREREQ (_ccu_name, _clk_name, _type, _prereq) \ + { \ + .prereq.name = #_prereq, \ + ___KONA_CLK_COMMON(_ccu_name, _clk_name, _type), \ } + +#define KONA_CLK (_ccu_name, _clk_name, _type) \ + { \ + ___KONA_CLK_COMMON(_ccu_name, _clk_name, _type), \ + } +#define LAST_KONA_CLK { .type = bcm_clk_none }/*
Signature · 1 line
1.9.1
PATCH v4 5/7 clk: bcm281xx: add bus clock support
In reply to Alex ElderAdd bus clock support. A bus clock has a subset of the components present in a peripheral clock (again, all optional): a gate; CCU policy management bits; and if needed, bits to control hysteresis.
--- drivers/clk/bcm/clk-kona-setup.c | 96 ++++++++++++++++++++++++++++++++++++++-- drivers/clk/bcm/clk-kona.c | 61 +++++++++++++++++++++++++ drivers/clk/bcm/clk-kona.h | 8 ++++ 3 files changed, 162 insertions(+), 3 deletions(-)
drivers/clk/bcm/clk-kona-setup.c +93 -3
@@ -76,6 +76,56 @@ static bool clk_requires_trigger(struct kona_clk *bcm_clk)return divider_exists (div) && !divider_is_fixed (div); } +static bool bus_clk_data_offsets_valid (struct kona_clk *bcm_clk) +{ +struct bus_clk_data *bus; +struct bcm_clk_policy *policy; +struct bcm_clk_gate *gate; +struct bcm_clk_hyst *hyst; +const char *name; +u32 limit; + +BUG_ON (bcm_clk->type != bcm_clk_bus); + bus = bcm_clk->u.bus; + name = bcm_clk->init_data.name; + + limit = bcm_clk->ccu->range -sizeof (u32); + limit =round_down (limit,sizeof (u32)); + + policy = &bus->policy; +if (policy_exists (policy)) { +if (policy->offset > limit) { +pr_err ("%s: bad policy offset for %s (%u > %u)\n" , + __func__, name, policy->offset, limit); +return false; + } + } + + gate = &bus->gate; + hyst = &bus->hyst; +if (gate_exists (gate)) { +if (gate->offset > limit) { +pr_err ("%s: bad gate offset for %s (%u > %u)\n" , + __func__, name, gate->offset, limit); +return false; + } +if (hyst_exists (hyst)) { +if (hyst->offset > limit) { +pr_err ("%s: bad hysteresis offset for %s " +"(%u > %u)\n" , __func__, + name, hyst->offset, limit); +return false; + } + } + }else if (hyst_exists (hyst)) { +pr_err ("%s: hysteresis but no gate for %s\n" , __func__, name); +return false; + } + + +return true; +} +static bool peri_clk_data_offsets_valid (struct kona_clk *bcm_clk) {struct peri_clk_data *peri;
@@ -86,15 +136,13 @@ static bool peri_clk_data_offsets_valid(struct kona_clk *bcm_clk)struct bcm_clk_sel *sel;struct bcm_clk_trig *trig;const char *name; -u32 range;u32 limit;BUG_ON (bcm_clk->type != bcm_clk_peri); peri = bcm_clk->u.peri; name = bcm_clk->init_data.name; - range = bcm_clk->ccu->range; - limit = range -sizeof (u32); + limit = bcm_clk->ccu->range -sizeof (u32); limit =round_down (limit,sizeof (u32)); policy = &peri->policy;
@@ -397,6 +445,23 @@ static bool trig_valid(struct bcm_clk_trig *trig, const char *field_name,return bit_posn_valid (trig->bit, field_name, clock_name); } +/* Determine whether the set of bus clock registers are valid. */ +static bool +bus_clk_data_valid (struct kona_clk *bcm_clk) +{ +struct bcm_clk_gate *gate; + +BUG_ON (bcm_clk->type != bcm_clk_bus); +if (!bus_clk_data_offsets_valid (bcm_clk)) +return false; + + gate = &bcm_clk->u.bus->gate; +if (!gate_exists (gate)) +return true; + +return gate_valid (gate,"gate" , bcm_clk->init_data.name); +} +/* Determine whether the set of peripheral clock registers are valid. */ static bool peri_clk_data_valid (struct kona_clk *bcm_clk)
@@ -494,6 +559,10 @@ peri_clk_data_valid(struct kona_clk *bcm_clk)static bool kona_clk_valid (struct kona_clk *bcm_clk) {switch (bcm_clk->type) { +case bcm_clk_bus: +if (!bus_clk_data_valid (bcm_clk)) +return false; +break ;case bcm_clk_peri:if (!peri_clk_data_valid (bcm_clk))return false;
@@ -664,6 +733,20 @@ static void clk_sel_teardown(struct bcm_clk_sel *sel, init_data->parent_names =NULL ; } +static void bus_clk_teardown (struct bus_clk_data *data, +struct clk_init_data *init_data) +{ +/* Nothing to do */ +} + +static int +bus_clk_setup (struct bus_clk_data *data,struct clk_init_data *init_data) +{ + init_data->flags =CLK_IGNORE_UNUSED ; + +return 0 ; +} +static void peri_clk_teardown (struct peri_clk_data *data,struct clk_init_data *init_data) {
@@ -687,6 +770,9 @@ peri_clk_setup(struct peri_clk_data *data, struct clk_init_data *init_data)static void bcm_clk_teardown (struct kona_clk *bcm_clk) {switch (bcm_clk->type) { +case bcm_clk_bus: +bus_clk_teardown (bcm_clk->u.data, &bcm_clk->init_data); +break ;case bcm_clk_peri:peri_clk_teardown (bcm_clk->u.data, &bcm_clk->init_data);break ;
@@ -722,6 +808,10 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk)struct clk *clk =NULL ;switch (bcm_clk->type) { +case bcm_clk_bus: +if (bus_clk_setup (bcm_clk->u.data, init_data)) +return NULL ; +break ;case bcm_clk_peri:if (peri_clk_setup (bcm_clk->u.data, init_data))return NULL ;
drivers/clk/bcm/clk-kona.c +61 -0
79 lines not on this page · raw message
drivers/clk/bcm/clk-kona.h +8 -0
@@ -387,6 +387,12 @@ struct bcm_clk_trig { .flags = FLAG(TRIG, EXISTS), \ } +struct bus_clk_data { +struct bcm_clk_policy policy; +struct bcm_clk_gate gate; +struct bcm_clk_hyst hyst; +}; +struct peri_clk_data {struct bcm_clk_policy policy;struct bcm_clk_gate gate;
@@ -412,6 +418,7 @@ struct kona_clk { } prereq;union {void *data; +struct bus_clk_data *bus;struct peri_clk_data *peri; } u; };
@@ -513,6 +520,7 @@ struct ccu_data {/* Exported globals */ +extern struct clk_ops kona_bus_clk_ops;extern struct clk_ops kona_peri_clk_ops;/* Externally visible functions */
Signature · 1 line
1.9.1
PATCH v4 6/7 clk: bcm281xx: define a bus clock
In reply to Alex ElderDefine the bus clock "bsc3_apb". This bus clock has to be managed using the CCU policy mechanism, so add the definitions required for that to the clock and its CCU.
This one bus clock in particular is defined because it is needed by peripheral clock "bsc3". Our boot loader does not properly activate "bsc3_apb", and as a result, "bsc3" isn't able to function properly. With "bsc3_apb" specified as a prerequisite clock for "bsc3", the latter works correctly.
For now only this one bus clock is defined, because it allows correct operation of "bsc3". Others can be added later as needed (and this patch serves to show how that's done).
--- drivers/clk/bcm/clk-bcm281xx.c | 13 ++++++++++++- include/dt-bindings/clock/bcm281xx.h | 3 ++- 2 files changed, 14 insertions(+), 2 deletions(-)
drivers/clk/bcm/clk-bcm281xx.c +12 -1
@@ -309,8 +309,17 @@ static struct peri_clk_data pwm_data = { .trig = TRIGGER(0x0afc, 15), }; +static struct bus_clk_data bsc3_apb_data = { + .policy =POLICY (0x0048 ,4 ), + .gate =HW_SW_GATE (0x0484 ,16 ,0 ,1 ), +}; +static struct ccu_data slave_ccu_data = {BCM281XX_CCU_COMMON (slave,SLAVE ), + .policy = { + .enable =CCU_LVM_EN (0x0034 ,0 ), + .control =CCU_POLICY_CTL (0x000c ,0 ,1 ,2 ), + }, .kona_clks = { [BCM281XX_SLAVE_CCU_UARTB ] =KONA_CLK (slave, uartb, peri),
@@ -329,9 +338,11 @@ static struct ccu_data slave_ccu_data = { [BCM281XX_SLAVE_CCU_BSC2] = KONA_CLK(slave, bsc2, peri), [BCM281XX_SLAVE_CCU_BSC3 ] = - KONA_CLK(slave, bsc3, peri), + KONA_CLK_PREREQ(slave, bsc3, peri, bsc3_apb), [BCM281XX_SLAVE_CCU_PWM ] = KONA_CLK(slave, pwm, peri), + [BCM281XX_SLAVE_CCU_BSC3_APB ] = + KONA_CLK(slave, bsc3_apb, bus), [BCM281XX_SLAVE_CCU_CLOCK_COUNT] = LAST_KONA_CLK, }, };
include/dt-bindings/clock/bcm281xx.h +2 -1
@@ -72,6 +72,7 @@#define BCM281XX_SLAVE_CCU_BSC2 7#define BCM281XX_SLAVE_CCU_BSC3 8#define BCM281XX_SLAVE_CCU_PWM 9 -#define BCM281XX_SLAVE_CCU_CLOCK_COUNT 10 +#define BCM281XX_SLAVE_CCU_BSC3_APB 10 +#define BCM281XX_SLAVE_CCU_CLOCK_COUNT 11#endif /* _CLOCK_BCM281XX_H */
Signature · 1 line
1.9.1
PATCH v4 7/7 ARM: dts: add bus clock bsc3_apb for bcm281xx
In reply to Alex ElderAdd the bus clock named "bsc3_apb" to the list of those provided by the slave CCU.
--- arch/arm/boot/dts/bcm11351.dtsi | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/boot/dts/bcm11351.dtsi +2 -1
@@ -247,7 +247,8 @@ "bsc1", "bsc2", "bsc3", - "pwm"; + "pwm", + "bsc3_apb"; }; ref_1m_clk: ref_1m {
Signature · 1 line
1.9.1