| Message ID | 20210126090120.19900-3-gabriel.fernandez@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce STM32MP1 RCC in secured mode | expand |
Quoting gabriel.fernandez@foss.st.com (2021-01-26 01:01:08) > From: Gabriel Fernandez <gabriel.fernandez@foss.st.com> > > 'ck_rtc' has multiple clocks as input (ck_hsi, ck_lsi, and ck_hse). > A divider is available only on the specific rtc input for ck_hse. > This Merge will facilitate to have a more coherent clock tree > in no trusted / trusted world. > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@foss.st.com> > --- > drivers/clk/clk-stm32mp1.c | 49 +++++++++++++++++++++++++++++++++----- > 1 file changed, 43 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c > index 35d5aee8f9b0..0e1d4427a8df 100644 > --- a/drivers/clk/clk-stm32mp1.c > +++ b/drivers/clk/clk-stm32mp1.c > @@ -245,7 +245,7 @@ static const char * const dsi_src[] = { > }; > > static const char * const rtc_src[] = { > - "off", "ck_lse", "ck_lsi", "ck_hse_rtc" > + "off", "ck_lse", "ck_lsi", "ck_hse" > }; > > static const char * const mco1_src[] = { > @@ -1031,6 +1031,42 @@ static struct clk_hw *clk_register_cktim(struct device *dev, const char *name, > return hw; > } > > +/* The divider of RTC clock concerns only ck_hse clock */ > +#define HSE_RTC 3 > + > +static unsigned long clk_divider_rtc_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) > + return clk_divider_ops.recalc_rate(hw, parent_rate); > + > + return parent_rate; > +} > + > +static long clk_divider_rtc_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *prate) > +{ > + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) This clk op can be called at basically any time. Maybe this should use the determine rate op and then look to see what the parent is that comes in via the rate request structure? Or is the intention to keep this pinned to one particular parent? Looking at this right now it doesn't really make much sense why the current parent state should play into what rate the clk can round to, unless there is some more clk flags going on that constrain the ability to change this clk's parent. > + return clk_divider_ops.round_rate(hw, rate, prate); > + > + return *prate; > +} > + > +static int clk_divider_rtc_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) > + return clk_divider_ops.set_rate(hw, rate, parent_rate); > + > + return parent_rate; > +} > +
On 2/9/21 9:00 AM, Stephen Boyd wrote: > Quoting gabriel.fernandez@foss.st.com (2021-01-26 01:01:08) >> From: Gabriel Fernandez <gabriel.fernandez@foss.st.com> >> >> 'ck_rtc' has multiple clocks as input (ck_hsi, ck_lsi, and ck_hse). >> A divider is available only on the specific rtc input for ck_hse. >> This Merge will facilitate to have a more coherent clock tree >> in no trusted / trusted world. >> >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@foss.st.com> >> --- >> drivers/clk/clk-stm32mp1.c | 49 +++++++++++++++++++++++++++++++++----- >> 1 file changed, 43 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c >> index 35d5aee8f9b0..0e1d4427a8df 100644 >> --- a/drivers/clk/clk-stm32mp1.c >> +++ b/drivers/clk/clk-stm32mp1.c >> @@ -245,7 +245,7 @@ static const char * const dsi_src[] = { >> }; >> >> static const char * const rtc_src[] = { >> - "off", "ck_lse", "ck_lsi", "ck_hse_rtc" >> + "off", "ck_lse", "ck_lsi", "ck_hse" >> }; >> >> static const char * const mco1_src[] = { >> @@ -1031,6 +1031,42 @@ static struct clk_hw *clk_register_cktim(struct device *dev, const char *name, >> return hw; >> } >> >> +/* The divider of RTC clock concerns only ck_hse clock */ >> +#define HSE_RTC 3 >> + >> +static unsigned long clk_divider_rtc_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) >> + return clk_divider_ops.recalc_rate(hw, parent_rate); >> + >> + return parent_rate; >> +} >> + >> +static long clk_divider_rtc_round_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long *prate) >> +{ >> + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) > This clk op can be called at basically any time. Maybe this should use > the determine rate op and then look to see what the parent is that comes > in via the rate request structure? Or is the intention to keep this > pinned to one particular parent? Looking at this right now it doesn't > really make much sense why the current parent state should play into > what rate the clk can round to, unless there is some more clk flags > going on that constrain the ability to change this clk's parent. Yes the intention is to keep this pinned for one particular parent. This divider is only applied on the 4th input of the MUX of the RTC and doesn't affect the HSE frequency for all the system. Oscillators ----- | lse |----------------+----------------> ck_lse ----- | ----- | | lsi |------------+--------------------> ck_lsi ----- | | | | ----- | | | hse |----+-------|---|----------------> ck_hse ----- | | | | | | |\ mux | | | OFF -->| \ | | | | \ gate | | --------->| | --- | | | |--->| |--> ck_rtc | ------------->| | --- | ----------- | | ----| % 1 to 64 |--->| / ----------- |/ divider I manage the RTC with a clock composite with a gate a mux and a specific rate ops for hse input. That why i need to the parent state. Best Regards Gabriel >> + return clk_divider_ops.round_rate(hw, rate, prate); >> + >> + return *prate; >> +} >> + >> +static int clk_divider_rtc_set_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) >> + return clk_divider_ops.set_rate(hw, rate, parent_rate); >> + >> + return parent_rate; >> +} >> +
On 2/19/21 2:27 AM, Stephen Boyd wrote: > Quoting gabriel.fernandez@foss.st.com (2021-02-12 00:08:40) >> On 2/9/21 9:00 AM, Stephen Boyd wrote: >>> Quoting gabriel.fernandez@foss.st.com (2021-01-26 01:01:08) >>>> From: Gabriel Fernandez <gabriel.fernandez@foss.st.com> >>>> >>>> 'ck_rtc' has multiple clocks as input (ck_hsi, ck_lsi, and ck_hse). >>>> A divider is available only on the specific rtc input for ck_hse. >>>> This Merge will facilitate to have a more coherent clock tree >>>> in no trusted / trusted world. >>>> >>>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@foss.st.com> >>>> --- >>>> drivers/clk/clk-stm32mp1.c | 49 +++++++++++++++++++++++++++++++++----- >>>> 1 file changed, 43 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c >>>> index 35d5aee8f9b0..0e1d4427a8df 100644 >>>> --- a/drivers/clk/clk-stm32mp1.c >>>> +++ b/drivers/clk/clk-stm32mp1.c >>>> @@ -245,7 +245,7 @@ static const char * const dsi_src[] = { >>>> }; >>>> >>>> static const char * const rtc_src[] = { >>>> - "off", "ck_lse", "ck_lsi", "ck_hse_rtc" >>>> + "off", "ck_lse", "ck_lsi", "ck_hse" >>>> }; >>>> >>>> static const char * const mco1_src[] = { >>>> @@ -1031,6 +1031,42 @@ static struct clk_hw *clk_register_cktim(struct device *dev, const char *name, >>>> return hw; >>>> } >>>> >>>> +/* The divider of RTC clock concerns only ck_hse clock */ >>>> +#define HSE_RTC 3 >>>> + >>>> +static unsigned long clk_divider_rtc_recalc_rate(struct clk_hw *hw, >>>> + unsigned long parent_rate) >>>> +{ >>>> + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) >>>> + return clk_divider_ops.recalc_rate(hw, parent_rate); >>>> + >>>> + return parent_rate; >>>> +} >>>> + >>>> +static long clk_divider_rtc_round_rate(struct clk_hw *hw, unsigned long rate, >>>> + unsigned long *prate) >>>> +{ >>>> + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) >>> This clk op can be called at basically any time. Maybe this should use >>> the determine rate op and then look to see what the parent is that comes >>> in via the rate request structure? Or is the intention to keep this >>> pinned to one particular parent? Looking at this right now it doesn't >>> really make much sense why the current parent state should play into >>> what rate the clk can round to, unless there is some more clk flags >>> going on that constrain the ability to change this clk's parent. >> Yes the intention is to keep this pinned for one particular parent. >> >> This divider is only applied on the 4th input of the MUX of the RTC and >> >> doesn't affect the HSE frequency for all the system. >> >> >> Oscillators >> ----- >> | lse |----------------+----------------> ck_lse >> ----- | >> ----- | >> | lsi |------------+--------------------> ck_lsi >> ----- | | >> | | >> ----- | | >> | hse |----+-------|---|----------------> ck_hse >> ----- | | | >> | | | |\ mux >> | | | OFF -->| \ >> | | | | \ gate >> | | --------->| | --- >> | | | |--->| |--> ck_rtc >> | ------------->| | --- >> | ----------- | | >> ----| % 1 to 64 |--->| / >> ----------- |/ >> divider >> >> I manage the RTC with a clock composite with a gate a mux and a specific >> rate ops for hse input. >> >> That why i need to the parent state. >> > So would using determine_rate op instead of round_rate op help here? That > will provide the current parent rate and hw pointer in the rate request > structure. yes u understand what you mean, i will send you a v3. Many Thanks.
diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c index 35d5aee8f9b0..0e1d4427a8df 100644 --- a/drivers/clk/clk-stm32mp1.c +++ b/drivers/clk/clk-stm32mp1.c @@ -245,7 +245,7 @@ static const char * const dsi_src[] = { }; static const char * const rtc_src[] = { - "off", "ck_lse", "ck_lsi", "ck_hse_rtc" + "off", "ck_lse", "ck_lsi", "ck_hse" }; static const char * const mco1_src[] = { @@ -1031,6 +1031,42 @@ static struct clk_hw *clk_register_cktim(struct device *dev, const char *name, return hw; } +/* The divider of RTC clock concerns only ck_hse clock */ +#define HSE_RTC 3 + +static unsigned long clk_divider_rtc_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) + return clk_divider_ops.recalc_rate(hw, parent_rate); + + return parent_rate; +} + +static long clk_divider_rtc_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *prate) +{ + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) + return clk_divider_ops.round_rate(hw, rate, prate); + + return *prate; +} + +static int clk_divider_rtc_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + if (clk_hw_get_parent(hw) == clk_hw_get_parent_by_index(hw, HSE_RTC)) + return clk_divider_ops.set_rate(hw, rate, parent_rate); + + return parent_rate; +} + +static const struct clk_ops rtc_div_clk_ops = { + .recalc_rate = clk_divider_rtc_recalc_rate, + .round_rate = clk_divider_rtc_round_rate, + .set_rate = clk_divider_rtc_set_rate, +}; + struct stm32_pll_cfg { u32 offset; }; @@ -1243,6 +1279,10 @@ _clk_stm32_register_composite(struct device *dev, _STM32_DIV(_div_offset, _div_shift, _div_width,\ _div_flags, _div_table, NULL)\ +#define _DIV_RTC(_div_offset, _div_shift, _div_width, _div_flags, _div_table)\ + _STM32_DIV(_div_offset, _div_shift, _div_width,\ + _div_flags, _div_table, &rtc_div_clk_ops) + #define _STM32_MUX(_offset, _shift, _width, _mux_flags, _mmux, _ops)\ .mux = &(struct stm32_mux_cfg) {\ &(struct mux_cfg) {\ @@ -1965,13 +2005,10 @@ static const struct clock_config stm32mp1_clock_cfg[] = { _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)), /* RTC clock */ - DIV(NO_ID, "ck_hse_rtc", "ck_hse", 0, RCC_RTCDIVR, 0, 6, 0), - - COMPOSITE(RTC, "ck_rtc", rtc_src, CLK_OPS_PARENT_ENABLE | - CLK_SET_RATE_PARENT, + COMPOSITE(RTC, "ck_rtc", rtc_src, CLK_OPS_PARENT_ENABLE, _GATE(RCC_BDCR, 20, 0), _MUX(RCC_BDCR, 16, 2, 0), - _NO_DIV), + _DIV_RTC(RCC_RTCDIVR, 0, 6, 0, NULL)), /* MCO clocks */ COMPOSITE(CK_MCO1, "ck_mco1", mco1_src, CLK_OPS_PARENT_ENABLE |