| Message ID | 1341829866-26091-5-git-send-email-shiraz.hashim@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 9, 2012 at 11:31 AM, Shiraz Hashim <shiraz.hashim@st.com> wrote: > From: Vipul Kumar Samar <vipulkumar.samar@st.com> > > sys_clk have multiple parents and selection of parent is depends on s/ is// > sys_clk_ctrl register (bit no. 23:25) with possible values, > > 0XX: pll1_clk > 10X: sys_synth_clk > 110: pll2_clk > 111: pll3_clk > > Update sys_clk parent array accordingly (ex. 0:3-pll1_clk) and > fix mask value to 7. > > Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> > --- > drivers/clk/spear/spear1340_clock.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/spear/spear1340_clock.c b/drivers/clk/spear/spear1340_clock.c > index e69c542..b3b56de 100644 > --- a/drivers/clk/spear/spear1340_clock.c > +++ b/drivers/clk/spear/spear1340_clock.c > @@ -25,7 +25,7 @@ > #define SPEAR1340_HCLK_SRC_SEL_SHIFT 27 > #define SPEAR1340_HCLK_SRC_SEL_MASK 1 > #define SPEAR1340_SCLK_SRC_SEL_SHIFT 23 > - #define SPEAR1340_SCLK_SRC_SEL_MASK 3 > + #define SPEAR1340_SCLK_SRC_SEL_MASK 7 AFAICR, Mask represents number of bits and not the actual mask. Can you check that again? > /* PLL related registers and bit values */ > #define SPEAR1340_PLL_CFG (VA_MISC_BASE + 0x210) > @@ -369,8 +369,8 @@ static struct frac_rate_tbl gen_rtbl[] = { > > /* clock parents */ > static const char *vco_parents[] = { "osc_24m_clk", "osc_25m_clk", }; > -static const char *sys_parents[] = { "none", "pll1_clk", "none", "none", > - "sys_synth_clk", "none", "pll2_clk", "pll3_clk", }; > +static const char *sys_parents[] = { "pll1_clk", "pll1_clk", "pll1_clk", > + "pll1_clk", "sys_synth_clk", "sys_synth_clk", "pll2_clk", "pll3_clk", }; Don't know what would be the implication of this? @Mike: Can you please tell us what should we do in such cases? -- viresh
On 7/9/2012 4:34 PM, viresh kumar wrote: > On Mon, Jul 9, 2012 at 11:31 AM, Shiraz Hashim<shiraz.hashim@st.com> wrote: >> From: Vipul Kumar Samar<vipulkumar.samar@st.com> >> >> sys_clk have multiple parents and selection of parent is depends on > > s/ is// > >> sys_clk_ctrl register (bit no. 23:25) with possible values, >> >> 0XX: pll1_clk >> 10X: sys_synth_clk >> 110: pll2_clk >> 111: pll3_clk >> >> Update sys_clk parent array accordingly (ex. 0:3-pll1_clk) and >> fix mask value to 7. >> >> Signed-off-by: Vipul Kumar Samar<vipulkumar.samar@st.com> >> --- >> drivers/clk/spear/spear1340_clock.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clk/spear/spear1340_clock.c b/drivers/clk/spear/spear1340_clock.c >> index e69c542..b3b56de 100644 >> --- a/drivers/clk/spear/spear1340_clock.c >> +++ b/drivers/clk/spear/spear1340_clock.c >> @@ -25,7 +25,7 @@ >> #define SPEAR1340_HCLK_SRC_SEL_SHIFT 27 >> #define SPEAR1340_HCLK_SRC_SEL_MASK 1 >> #define SPEAR1340_SCLK_SRC_SEL_SHIFT 23 >> - #define SPEAR1340_SCLK_SRC_SEL_MASK 3 >> + #define SPEAR1340_SCLK_SRC_SEL_MASK 7 > > AFAICR, Mask represents number of bits and not the actual mask. > Can you check that again? Sorry, i'll correct it. > >> /* PLL related registers and bit values */ >> #define SPEAR1340_PLL_CFG (VA_MISC_BASE + 0x210) >> @@ -369,8 +369,8 @@ static struct frac_rate_tbl gen_rtbl[] = { >> >> /* clock parents */ >> static const char *vco_parents[] = { "osc_24m_clk", "osc_25m_clk", }; >> -static const char *sys_parents[] = { "none", "pll1_clk", "none", "none", >> - "sys_synth_clk", "none", "pll2_clk", "pll3_clk", }; >> +static const char *sys_parents[] = { "pll1_clk", "pll1_clk", "pll1_clk", >> + "pll1_clk", "sys_synth_clk", "sys_synth_clk", "pll2_clk", "pll3_clk", }; > > Don't know what would be the implication of this? > > @Mike: Can you please tell us what should we do in such cases? > Is there any other solution for such cases ??? Regards Vipul Samar
On Mon, Jul 9, 2012 at 1:04 PM, vipul kumar samar <vipulkumar.samar@st.com> wrote: > On 7/9/2012 4:34 PM, viresh kumar wrote: >> >> On Mon, Jul 9, 2012 at 11:31 AM, Shiraz Hashim<shiraz.hashim@st.com> >> wrote: >>> >>> From: Vipul Kumar Samar<vipulkumar.samar@st.com> >>> >>> sys_clk have multiple parents and selection of parent is depends on >> >> >> s/ is// I hope you haven't missed this comment :) >>> static const char *vco_parents[] = { "osc_24m_clk", "osc_25m_clk", }; >>> -static const char *sys_parents[] = { "none", "pll1_clk", "none", "none", >>> - "sys_synth_clk", "none", "pll2_clk", "pll3_clk", }; >>> +static const char *sys_parents[] = { "pll1_clk", "pll1_clk", "pll1_clk", >>> + "pll1_clk", "sys_synth_clk", "sys_synth_clk", "pll2_clk", >>> "pll3_clk", }; >> >> >> Don't know what would be the implication of this? >> >> @Mike: Can you please tell us what should we do in such cases? >> > > Is there any other solution for such cases ??? That's what i have asked mike for :) Probably you can go through the clock framework code and check how these names are used. Shouldn't be too complex to understand. -- viresh
On 20120709-13:34, viresh kumar wrote: > On Mon, Jul 9, 2012 at 1:04 PM, vipul kumar samar > <vipulkumar.samar@st.com> wrote: > > On 7/9/2012 4:34 PM, viresh kumar wrote: > >> > >> On Mon, Jul 9, 2012 at 11:31 AM, Shiraz Hashim<shiraz.hashim@st.com> > >> wrote: > >>> > >>> From: Vipul Kumar Samar<vipulkumar.samar@st.com> > >>> > >>> sys_clk have multiple parents and selection of parent is depends on > >> > >> > >> s/ is// > > I hope you haven't missed this comment :) > > >>> static const char *vco_parents[] = { "osc_24m_clk", "osc_25m_clk", }; > >>> -static const char *sys_parents[] = { "none", "pll1_clk", "none", "none", > >>> - "sys_synth_clk", "none", "pll2_clk", "pll3_clk", }; > >>> +static const char *sys_parents[] = { "pll1_clk", "pll1_clk", "pll1_clk", > >>> + "pll1_clk", "sys_synth_clk", "sys_synth_clk", "pll2_clk", > >>> "pll3_clk", }; > >> > >> > >> Don't know what would be the implication of this? > >> > >> @Mike: Can you please tell us what should we do in such cases? > >> > > > > Is there any other solution for such cases ??? > > That's what i have asked mike for :) > Probably you can go through the clock framework code and check how these > names are used. Shouldn't be too complex to understand. > I assume this change has been tested and wouldn't be posted if modifying the parent names broke things. That said, as long as the parent names are valid strings then the clk framework should handle them. When calling __clk_lookup for parent name "none", __clk_lookup will return NULL (of course assuming no one else in the system registered a clock named "none", which would be silly). This is handled gracefully by the clk framework by re-parenting your "sys" clk from $old_parent to the "orphan" list. At the top level, there are basically two clock trees. The first tree is "real" clock tree which starts as a list of clocks that set the CLK_IS_ROOT flag. The second tree is a tree of "orphans" for clocks which are defined but "disconnected" from any real root clock (which might be caused by missing data, etc). In general it is OK to declare parent names which might result in your clock being orphaned. In practice it is more likely that your data matches your code: e.g. if you don't support a parent clock in the data then you likely never try use that missing parent clock in your code. The OMAP port does in fact make use of the orphan tree for some clocks, so it is tested. However we haven't had any users of the clock tree which made a lot of use of "dynamic" reparenting to and from the orphan tree. I did unit test this back during the 3.4 cycle, but I haven't since. Let me know if you have any problems with it. Regards, Mike > -- > viresh > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 9 July 2012 23:57, Mike Turquette <mturquette@ti.com> wrote: > I assume this change has been tested and wouldn't be posted if modifying > the parent names broke things. > > That said, as long as the parent names are valid strings then the clk > framework should handle them. When calling __clk_lookup for parent name > "none", __clk_lookup will return NULL (of course assuming no one else in > the system registered a clock named "none", which would be silly). This > is handled gracefully by the clk framework by re-parenting your "sys" > clk from $old_parent to the "orphan" list. > > At the top level, there are basically two clock trees. The first tree > is "real" clock tree which starts as a list of clocks that set the > CLK_IS_ROOT flag. The second tree is a tree of "orphans" for clocks > which are defined but "disconnected" from any real root clock (which > might be caused by missing data, etc). > > In general it is OK to declare parent names which might result in your > clock being orphaned. In practice it is more likely that your data > matches your code: e.g. if you don't support a parent clock in the data > then you likely never try use that missing parent clock in your code. > > The OMAP port does in fact make use of the orphan tree for some clocks, > so it is tested. However we haven't had any users of the clock tree > which made a lot of use of "dynamic" reparenting to and from the orphan > tree. I did unit test this back during the 3.4 cycle, but I haven't > since. Let me know if you have any problems with it. > Hi Mike, For example, clk1 have parents pclk1 and pclk2. Register values for pclk1 is 0X and for pclk2 is 1X. i.e. pclk1 is selected for both 00 and 01. And pclk2 is selected for 10 and 11. so, its better to take care of all these situations, otherwise there can be corner cases like: In linux we decide to use 00 for pclk1, 10 for pclk2 and other two for "none". But bootloader, enabled the clock with 01 for pclk1. This clock isn't a orphan but it will look like that in our clock tree. So, is this OK in the current implementation of clk framework to have parents like "pclk1", "pclk1", "pclk2", "pclk2"? -- viresh
Hi Mike, On Mon, Jul 09, 2012 at 03:57:00PM -0700, Mike Turquette wrote: > On 20120709-13:34, viresh kumar wrote: > > On Mon, Jul 9, 2012 at 1:04 PM, vipul kumar samar > > <vipulkumar.samar@st.com> wrote: > > > On 7/9/2012 4:34 PM, viresh kumar wrote: > > >> > > >> On Mon, Jul 9, 2012 at 11:31 AM, Shiraz Hashim<shiraz.hashim@st.com> > > >> wrote: > > >>> > > >>> From: Vipul Kumar Samar<vipulkumar.samar@st.com> > > >>> > > >>> sys_clk have multiple parents and selection of parent is depends on > > >> > > >> > > >> s/ is// > > > > I hope you haven't missed this comment :) > > > > >>> static const char *vco_parents[] = { "osc_24m_clk", "osc_25m_clk", }; > > >>> -static const char *sys_parents[] = { "none", "pll1_clk", "none", "none", > > >>> - "sys_synth_clk", "none", "pll2_clk", "pll3_clk", }; > > >>> +static const char *sys_parents[] = { "pll1_clk", "pll1_clk", "pll1_clk", > > >>> + "pll1_clk", "sys_synth_clk", "sys_synth_clk", "pll2_clk", > > >>> "pll3_clk", }; > > >> > > >> > > >> Don't know what would be the implication of this? > > >> > > >> @Mike: Can you please tell us what should we do in such cases? > > >> > > > > > > Is there any other solution for such cases ??? > > > > That's what i have asked mike for :) > > Probably you can go through the clock framework code and check how these > > names are used. Shouldn't be too complex to understand. > > > > I assume this change has been tested and wouldn't be posted if modifying > the parent names broke things. Yes, it is working fine for us. -- regards Shiraz
On 20120710-09:21, Viresh Kumar wrote: > On 9 July 2012 23:57, Mike Turquette <mturquette@ti.com> wrote: > > > I assume this change has been tested and wouldn't be posted if modifying > > the parent names broke things. > > > > That said, as long as the parent names are valid strings then the clk > > framework should handle them. When calling __clk_lookup for parent name > > "none", __clk_lookup will return NULL (of course assuming no one else in > > the system registered a clock named "none", which would be silly). This > > is handled gracefully by the clk framework by re-parenting your "sys" > > clk from $old_parent to the "orphan" list. > > > > At the top level, there are basically two clock trees. The first tree > > is "real" clock tree which starts as a list of clocks that set the > > CLK_IS_ROOT flag. The second tree is a tree of "orphans" for clocks > > which are defined but "disconnected" from any real root clock (which > > might be caused by missing data, etc). > > > > In general it is OK to declare parent names which might result in your > > clock being orphaned. In practice it is more likely that your data > > matches your code: e.g. if you don't support a parent clock in the data > > then you likely never try use that missing parent clock in your code. > > > > The OMAP port does in fact make use of the orphan tree for some clocks, > > so it is tested. However we haven't had any users of the clock tree > > which made a lot of use of "dynamic" reparenting to and from the orphan > > tree. I did unit test this back during the 3.4 cycle, but I haven't > > since. Let me know if you have any problems with it. > > > > Hi Mike, > > For example, clk1 have parents pclk1 and pclk2. Register values > for pclk1 is 0X and for pclk2 is 1X. i.e. pclk1 is selected for both 00 and > 01. > And pclk2 is selected for 10 and 11. so, its better to take care of all > these situations, > otherwise there can be corner cases like: In linux we decide to use 00 for > pclk1, 10 for > pclk2 and other two for "none". But bootloader, enabled the clock with 01 > for pclk1. > This clock isn't a orphan but it will look like that in our clock tree. > > So, is this OK in the current implementation of clk framework to have > parents like > "pclk1", "pclk1", "pclk2", "pclk2"? > Hello Viresh, I believe it will be OK, but that situation definitely needs testing. There will not be duplicate clocks in the tree (which is good), but the parent selection logic for clk1 will be a bit silly. All in all duplicating parent string names is a hack. If this case is very common then a better way to handle it would be a clk_mux-specific flag which handles this case gracefully. Mark all of your mux data with this flag for the affected clocks and then don't worry about it anymore. Regards, Mike > -- > viresh
diff --git a/drivers/clk/spear/spear1340_clock.c b/drivers/clk/spear/spear1340_clock.c index e69c542..b3b56de 100644 --- a/drivers/clk/spear/spear1340_clock.c +++ b/drivers/clk/spear/spear1340_clock.c @@ -25,7 +25,7 @@ #define SPEAR1340_HCLK_SRC_SEL_SHIFT 27 #define SPEAR1340_HCLK_SRC_SEL_MASK 1 #define SPEAR1340_SCLK_SRC_SEL_SHIFT 23 - #define SPEAR1340_SCLK_SRC_SEL_MASK 3 + #define SPEAR1340_SCLK_SRC_SEL_MASK 7 /* PLL related registers and bit values */ #define SPEAR1340_PLL_CFG (VA_MISC_BASE + 0x210) @@ -369,8 +369,8 @@ static struct frac_rate_tbl gen_rtbl[] = { /* clock parents */ static const char *vco_parents[] = { "osc_24m_clk", "osc_25m_clk", }; -static const char *sys_parents[] = { "none", "pll1_clk", "none", "none", - "sys_synth_clk", "none", "pll2_clk", "pll3_clk", }; +static const char *sys_parents[] = { "pll1_clk", "pll1_clk", "pll1_clk", + "pll1_clk", "sys_synth_clk", "sys_synth_clk", "pll2_clk", "pll3_clk", }; static const char *ahb_parents[] = { "cpu_div3_clk", "amba_synth_clk", }; static const char *gpt_parents[] = { "osc_24m_clk", "apb_clk", }; static const char *uart0_parents[] = { "pll5_clk", "osc_24m_clk",