| Message ID | 1460459243-27120-1-git-send-email-alim.akhtar@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/12/2016 01:07 PM, Alim Akhtar wrote: > This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is > required before accessing registers of these blocks. Patch applied, thanks!
2016-04-12 14:07 GMT+03:00 Alim Akhtar <alim.akhtar@samsung.com>: > This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is > required before accessing registers of these blocks. > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > --- > drivers/clk/samsung/clk-exynos7.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c > index ad68d46..7013aa7 100644 > --- a/drivers/clk/samsung/clk-exynos7.c > +++ b/drivers/clk/samsung/clk-exynos7.c > @@ -8,6 +8,7 @@ > * > */ > > +#include <linux/clk.h> > #include <linux/clk-provider.h> > #include <linux/of.h> > > @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = { > > static void __init exynos7_clk_topc_init(struct device_node *np) > { > + struct clk *clk; > + > samsung_cmu_register_one(np, &topc_cmu_info); > + clk = __clk_lookup("aclk_ccore_133"); > + clk_prepare_enable(clk); Shouldn't this be rather done before calling samsung_cmu_register_one()? I don't remember exactly, but wouldn't clock registration trigger reading back current (mux, div) values from registers? Also, do we have any guarantees on order of initialization of particular CMUs? I believe this will happen in order of DT nodes and so would be not any kind of guarantee at all. Best regards, Tomasz
On 04/13/2016 08:26 AM, Tomasz Figa wrote: >> > @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = { >> > >> > static void __init exynos7_clk_topc_init(struct device_node *np) >> > { >> > + struct clk *clk; >> > + >> > samsung_cmu_register_one(np, &topc_cmu_info); >> > + clk = __clk_lookup("aclk_ccore_133"); >> > + clk_prepare_enable(clk); > > Shouldn't this be rather done before calling > samsung_cmu_register_one()? I don't remember exactly, but wouldn't > clock registration trigger reading back current (mux, div) values from > registers? > > Also, do we have any guarantees on order of initialization of > particular CMUs? I believe this will happen in order of DT nodes and > so would be not any kind of guarantee at all. If these clocks need to be kept enabled perhaps it's better to just set CLK_IS_CRITICAL flag for them? Patches adding this flag are already in clk-next branch in the clk git tree. This way the clocks would get enabled within the clk_register() call. The CMU registration order is enforced by listing input clocks to each CMU in DT. However, I wouldn't be concerned much about it in context of this patch. We are enabling here clocks which belong to same CMU. samsung_cmu_register_one() needs to be called first for subsequent __clk_lookup() calls to work. Perhaps related bits need to be set manually in CMU registers before registering a clock provider for the CMU, to fulfil the requirements. Anyway, summary of the $subject patch seems not precise enough: "This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is required before accessing registers of these blocks." We need to enable selected clocks (i.e. access the CMU's registers) before accessing this CMU's registers? -- Regards, Sylwester
2016-04-13 13:36 GMT+03:00 Sylwester Nawrocki <s.nawrocki@samsung.com>: > On 04/13/2016 08:26 AM, Tomasz Figa wrote: >>> > @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = { >>> > >>> > static void __init exynos7_clk_topc_init(struct device_node *np) >>> > { >>> > + struct clk *clk; >>> > + >>> > samsung_cmu_register_one(np, &topc_cmu_info); >>> > + clk = __clk_lookup("aclk_ccore_133"); >>> > + clk_prepare_enable(clk); >> >> Shouldn't this be rather done before calling >> samsung_cmu_register_one()? I don't remember exactly, but wouldn't >> clock registration trigger reading back current (mux, div) values from >> registers? >> >> Also, do we have any guarantees on order of initialization of >> particular CMUs? I believe this will happen in order of DT nodes and >> so would be not any kind of guarantee at all. > > If these clocks need to be kept enabled perhaps it's better to just set > CLK_IS_CRITICAL flag for them? Patches adding this flag are already in > clk-next branch in the clk git tree. This way the clocks would get > enabled within the clk_register() call. > Oh, I didn't know about this. Sounds like a good idea. :) > The CMU registration order is enforced by listing input clocks to each > CMU in DT. Ah, right, I recall now, I even reviewed relevant patch. Thanks for refreshing my memory. > However, I wouldn't be concerned much about it in context > of this patch. We are enabling here clocks which belong to same CMU. > samsung_cmu_register_one() needs to be called first for subsequent > __clk_lookup() calls to work. I thought a clock from another CMU has to be enabled for the CPU to be able to access the CMU being registered. > > Perhaps related bits need to be set manually in CMU registers before > registering a clock provider for the CMU, to fulfil the requirements. > > Anyway, summary of the $subject patch seems not precise enough: > > "This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is > required before accessing registers of these blocks." > > We need to enable selected clocks (i.e. access the CMU's registers) > before accessing this CMU's registers? Yeah, maybe the explanation could be a bit more precise. Best regards, Tomasz
Hi On 04/13/2016 05:01 PM, Tomasz Figa wrote: > 2016-04-13 13:36 GMT+03:00 Sylwester Nawrocki <s.nawrocki@samsung.com>: >> On 04/13/2016 08:26 AM, Tomasz Figa wrote: >>>>> @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = { >>>>> >>>>> static void __init exynos7_clk_topc_init(struct device_node *np) >>>>> { >>>>> + struct clk *clk; >>>>> + >>>>> samsung_cmu_register_one(np, &topc_cmu_info); >>>>> + clk = __clk_lookup("aclk_ccore_133"); >>>>> + clk_prepare_enable(clk); >>> >>> Shouldn't this be rather done before calling >>> samsung_cmu_register_one()? I don't remember exactly, but wouldn't >>> clock registration trigger reading back current (mux, div) values from >>> registers? >>> >>> Also, do we have any guarantees on order of initialization of >>> particular CMUs? I believe this will happen in order of DT nodes and >>> so would be not any kind of guarantee at all. >> >> If these clocks need to be kept enabled perhaps it's better to just set >> CLK_IS_CRITICAL flag for them? Patches adding this flag are already in >> clk-next branch in the clk git tree. This way the clocks would get >> enabled within the clk_register() call. >> > > Oh, I didn't know about this. Sounds like a good idea. :) > I was not aware of this flag, Thanks for pointing this out. This flag indeed solves my problem. These clocks should not be gated since the CMU block using this clock for SFR access. Please ignore the $SUBJECT patch, I will send a new patch which uses CLK_IS_CRITICAL flag instead. >> The CMU registration order is enforced by listing input clocks to each >> CMU in DT. > > Ah, right, I recall now, I even reviewed relevant patch. Thanks for > refreshing my memory. > >> However, I wouldn't be concerned much about it in context >> of this patch. We are enabling here clocks which belong to same CMU. >> samsung_cmu_register_one() needs to be called first for subsequent >> __clk_lookup() calls to work. > > I thought a clock from another CMU has to be enabled for the CPU to be > able to access the CMU being registered. > >> >> Perhaps related bits need to be set manually in CMU registers before >> registering a clock provider for the CMU, to fulfil the requirements. >> >> Anyway, summary of the $subject patch seems not precise enough: >> >> "This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is >> required before accessing registers of these blocks." >> >> We need to enable selected clocks (i.e. access the CMU's registers) >> before accessing this CMU's registers? > > Yeah, maybe the explanation could be a bit more precise. > > Best regards, > Tomasz > >
diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c index ad68d46..7013aa7 100644 --- a/drivers/clk/samsung/clk-exynos7.c +++ b/drivers/clk/samsung/clk-exynos7.c @@ -8,6 +8,7 @@ * */ +#include <linux/clk.h> #include <linux/clk-provider.h> #include <linux/of.h> @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = { static void __init exynos7_clk_topc_init(struct device_node *np) { + struct clk *clk; + samsung_cmu_register_one(np, &topc_cmu_info); + clk = __clk_lookup("aclk_ccore_133"); + clk_prepare_enable(clk); } CLK_OF_DECLARE(exynos7_clk_topc, "samsung,exynos7-clock-topc", @@ -573,7 +578,11 @@ static struct samsung_cmu_info top1_cmu_info __initdata = { static void __init exynos7_clk_top1_init(struct device_node *np) { + struct clk *clk; + samsung_cmu_register_one(np, &top1_cmu_info); + clk = __clk_lookup("aclk_fsys0_200"); + clk_prepare_enable(clk); } CLK_OF_DECLARE(exynos7_clk_top1, "samsung,exynos7-clock-top1",
This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is required before accessing registers of these blocks. Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> --- drivers/clk/samsung/clk-exynos7.c | 9 +++++++++ 1 file changed, 9 insertions(+)