| Message ID | 1604655988-353-1-git-send-email-claudiu.beznea@microchip.com |
|---|---|
| Headers | show |
| Series | clk: at91: adapt for dvfs | expand |
Quoting Claudiu Beznea (2020-11-06 01:46:23) > diff --git a/drivers/clk/at91/clk-sam9x60-pll.c b/drivers/clk/at91/clk-sam9x60-pll.c > index 78f458a7b2ef..6fe5d8530a0c 100644 > --- a/drivers/clk/at91/clk-sam9x60-pll.c > +++ b/drivers/clk/at91/clk-sam9x60-pll.c > @@ -225,8 +225,51 @@ static int sam9x60_frac_pll_set_rate(struct clk_hw *hw, unsigned long rate, > unsigned long parent_rate) > { > struct sam9x60_pll_core *core = to_sam9x60_pll_core(hw); > + struct sam9x60_frac *frac = to_sam9x60_frac(core); > + struct regmap *regmap = core->regmap; > + unsigned long irqflags, clkflags = clk_hw_get_flags(hw); > + unsigned int val, cfrac, cmul; > + long ret; > + > + ret = sam9x60_frac_pll_compute_mul_frac(core, rate, parent_rate, true); > + if (ret <= 0 || (clkflags & CLK_SET_RATE_GATE)) Is this function being called when the clk is enabled and it has the CLK_SET_RATE_GATE flag set? I'm confused why this driver needs to check this flag. > + return ret; > + > + spin_lock_irqsave(core->lock, irqflags); > + > + regmap_update_bits(regmap, AT91_PMC_PLL_UPDT, AT91_PMC_PLL_UPDT_ID_MSK, > + core->id); > + regmap_read(regmap, AT91_PMC_PLL_CTRL1, &val); > + cmul = (val & core->layout->mul_mask) >> core->layout->mul_shift; > + cfrac = (val & core->layout->frac_mask) >> core->layout->frac_shift; > + > + if (cmul == frac->mul && cfrac == frac->frac) > + goto unlock; > + > + regmap_write(regmap, AT91_PMC_PLL_CTRL1, > + (frac->mul << core->layout->mul_shift) | > + (frac->frac << core->layout->frac_shift)); > + > + regmap_update_bits(regmap, AT91_PMC_PLL_UPDT, > + AT91_PMC_PLL_UPDT_UPDATE | AT91_PMC_PLL_UPDT_ID_MSK, > + AT91_PMC_PLL_UPDT_UPDATE | core->id); > + > + regmap_update_bits(regmap, AT91_PMC_PLL_CTRL0, > + AT91_PMC_PLL_CTRL0_ENLOCK | AT91_PMC_PLL_CTRL0_ENPLL, > + AT91_PMC_PLL_CTRL0_ENLOCK | > + AT91_PMC_PLL_CTRL0_ENPLL); > + > + regmap_update_bits(regmap, AT91_PMC_PLL_UPDT, > + AT91_PMC_PLL_UPDT_UPDATE | AT91_PMC_PLL_UPDT_ID_MSK, > + AT91_PMC_PLL_UPDT_UPDATE | core->id); > > - return sam9x60_frac_pll_compute_mul_frac(core, rate, parent_rate, true); > + while (!sam9x60_pll_ready(regmap, core->id)) > + cpu_relax(); > + > +unlock: > + spin_unlock_irqrestore(core->lock, irqflags); > + > + return ret; > } > > static const struct clk_ops sam9x60_frac_pll_ops = { > @@ -378,9 +421,39 @@ static int sam9x60_div_pll_set_rate(struct clk_hw *hw, unsigned long rate, > { > struct sam9x60_pll_core *core = to_sam9x60_pll_core(hw); > struct sam9x60_div *div = to_sam9x60_div(core); > + struct regmap *regmap = core->regmap; > + unsigned long irqflags, clkflags = clk_hw_get_flags(hw); > + unsigned int val, cdiv; > > div->div = DIV_ROUND_CLOSEST(parent_rate, rate) - 1; > > + if (clkflags & CLK_SET_RATE_GATE) Same comment. > diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c > index d685e22b2014..33faf7c6d9fb 100644 > --- a/drivers/clk/at91/sama7g5.c > +++ b/drivers/clk/at91/sama7g5.c > @@ -95,15 +95,15 @@ static const struct clk_pll_layout pll_layout_divio = { > * @p: clock parent > * @l: clock layout > * @t: clock type > - * @f: true if clock is critical and cannot be disabled > + * @f: clock flags > * @eid: export index in sama7g5->chws[] array > */ > static const struct { > const char *n; > const char *p; > const struct clk_pll_layout *l; > + u32 f; Why not unsigned long? > u8 t; > - u8 c; > u8 eid; > } sama7g5_plls[][PLL_ID_MAX] = { > [PLL_ID_CPU] = { > @@ -111,13 +111,13 @@ static const struct { > .p = "mainck", > .l = &pll_layout_frac, > .t = PLL_TYPE_FRAC, > - .c = 1, }, > + .f = CLK_IS_CRITICAL, }, > > { .n = "cpupll_divpmcck", > .p = "cpupll_fracck", > .l = &pll_layout_divpmc, > .t = PLL_TYPE_DIV, > - .c = 1, > + .f = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, > .eid = PMC_CPUPLL, }, > }, > > @@ -126,13 +126,13 @@ static const struct { > .p = "mainck", > .l = &pll_layout_frac, > .t = PLL_TYPE_FRAC, > - .c = 1, }, > + .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE, }, > > { .n = "syspll_divpmcck", > .p = "syspll_fracck", > .l = &pll_layout_divpmc, > .t = PLL_TYPE_DIV, > - .c = 1, > + .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE, Please indicate why clks are critical. Whenever the CLK_IS_CRITICAL flag is used we should have a comment indicating why. > .eid = PMC_SYSPLL, }, > }, >
On 14.11.2020 23:14, Stephen Boyd wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Quoting Claudiu Beznea (2020-11-06 01:46:23) >> diff --git a/drivers/clk/at91/clk-sam9x60-pll.c b/drivers/clk/at91/clk-sam9x60-pll.c >> index 78f458a7b2ef..6fe5d8530a0c 100644 >> --- a/drivers/clk/at91/clk-sam9x60-pll.c >> +++ b/drivers/clk/at91/clk-sam9x60-pll.c >> @@ -225,8 +225,51 @@ static int sam9x60_frac_pll_set_rate(struct clk_hw *hw, unsigned long rate, >> unsigned long parent_rate) >> { >> struct sam9x60_pll_core *core = to_sam9x60_pll_core(hw); >> + struct sam9x60_frac *frac = to_sam9x60_frac(core); >> + struct regmap *regmap = core->regmap; >> + unsigned long irqflags, clkflags = clk_hw_get_flags(hw); >> + unsigned int val, cfrac, cmul; >> + long ret; >> + >> + ret = sam9x60_frac_pll_compute_mul_frac(core, rate, parent_rate, true); >> + if (ret <= 0 || (clkflags & CLK_SET_RATE_GATE)) > > Is this function being called when the clk is enabled and it has the > CLK_SET_RATE_GATE flag set? Yes, this function could be called when CLK_SET_RATE_GATE is set. On SAMA7G5 there are multiple PLL blocks of the same type. All these PLLs are controlled by clk-sam9x60-pll.c driver. One of this PLL block fed the CPU who's frequency could be changed at run time. At the same time there are PLLs that fed hardware block not glitch free aware or that we don't want to allow the rate change (this is the case of SAM9X60's CPU PLL, or the DDR PLL on SAMA7G5). I'm confused why this driver needs to check > this flag. Because we have multiple PLLs of the same type, some of them feed hardware blocks that are glitch free aware of these PLLs' frequencies changes, some feed hardware blocks that are not glitch free aware of PLLs' frequencies changes or for some of them we don't want the frequency changes at all. > >> + return ret; >> + >> + spin_lock_irqsave(core->lock, irqflags); >> + >> + regmap_update_bits(regmap, AT91_PMC_PLL_UPDT, AT91_PMC_PLL_UPDT_ID_MSK, >> + core->id); >> + regmap_read(regmap, AT91_PMC_PLL_CTRL1, &val); >> + cmul = (val & core->layout->mul_mask) >> core->layout->mul_shift; >> + cfrac = (val & core->layout->frac_mask) >> core->layout->frac_shift; >> + >> + if (cmul == frac->mul && cfrac == frac->frac) >> + goto unlock; >> + >> + regmap_write(regmap, AT91_PMC_PLL_CTRL1, >> + (frac->mul << core->layout->mul_shift) | >> + (frac->frac << core->layout->frac_shift)); >> + >> + regmap_update_bits(regmap, AT91_PMC_PLL_UPDT, >> + AT91_PMC_PLL_UPDT_UPDATE | AT91_PMC_PLL_UPDT_ID_MSK, >> + AT91_PMC_PLL_UPDT_UPDATE | core->id); >> + >> + regmap_update_bits(regmap, AT91_PMC_PLL_CTRL0, >> + AT91_PMC_PLL_CTRL0_ENLOCK | AT91_PMC_PLL_CTRL0_ENPLL, >> + AT91_PMC_PLL_CTRL0_ENLOCK | >> + AT91_PMC_PLL_CTRL0_ENPLL); >> + >> + regmap_update_bits(regmap, AT91_PMC_PLL_UPDT, >> + AT91_PMC_PLL_UPDT_UPDATE | AT91_PMC_PLL_UPDT_ID_MSK, >> + AT91_PMC_PLL_UPDT_UPDATE | core->id); >> >> - return sam9x60_frac_pll_compute_mul_frac(core, rate, parent_rate, true); >> + while (!sam9x60_pll_ready(regmap, core->id)) >> + cpu_relax(); >> + >> +unlock: >> + spin_unlock_irqrestore(core->lock, irqflags); >> + >> + return ret; >> } >> >> static const struct clk_ops sam9x60_frac_pll_ops = { >> @@ -378,9 +421,39 @@ static int sam9x60_div_pll_set_rate(struct clk_hw *hw, unsigned long rate, >> { >> struct sam9x60_pll_core *core = to_sam9x60_pll_core(hw); >> struct sam9x60_div *div = to_sam9x60_div(core); >> + struct regmap *regmap = core->regmap; >> + unsigned long irqflags, clkflags = clk_hw_get_flags(hw); >> + unsigned int val, cdiv; >> >> div->div = DIV_ROUND_CLOSEST(parent_rate, rate) - 1; >> >> + if (clkflags & CLK_SET_RATE_GATE) > > Same comment. > >> diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c >> index d685e22b2014..33faf7c6d9fb 100644 >> --- a/drivers/clk/at91/sama7g5.c >> +++ b/drivers/clk/at91/sama7g5.c >> @@ -95,15 +95,15 @@ static const struct clk_pll_layout pll_layout_divio = { >> * @p: clock parent >> * @l: clock layout >> * @t: clock type >> - * @f: true if clock is critical and cannot be disabled >> + * @f: clock flags >> * @eid: export index in sama7g5->chws[] array >> */ >> static const struct { >> const char *n; >> const char *p; >> const struct clk_pll_layout *l; >> + u32 f; > > Why not unsigned long? I'll switch to unsigned long. > >> u8 t; >> - u8 c; >> u8 eid; >> } sama7g5_plls[][PLL_ID_MAX] = { >> [PLL_ID_CPU] = { >> @@ -111,13 +111,13 @@ static const struct { >> .p = "mainck", >> .l = &pll_layout_frac, >> .t = PLL_TYPE_FRAC, >> - .c = 1, }, >> + .f = CLK_IS_CRITICAL, }, >> >> { .n = "cpupll_divpmcck", >> .p = "cpupll_fracck", >> .l = &pll_layout_divpmc, >> .t = PLL_TYPE_DIV, >> - .c = 1, >> + .f = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, >> .eid = PMC_CPUPLL, }, >> }, >> >> @@ -126,13 +126,13 @@ static const struct { >> .p = "mainck", >> .l = &pll_layout_frac, >> .t = PLL_TYPE_FRAC, >> - .c = 1, }, >> + .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE, }, >> >> { .n = "syspll_divpmcck", >> .p = "syspll_fracck", >> .l = &pll_layout_divpmc, >> .t = PLL_TYPE_DIV, >> - .c = 1, >> + .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE, > > Please indicate why clks are critical. Sure! I'll do it in next version. I chose it like this because they are feeding critical parts of the system like CPU or memory. > Whenever the CLK_IS_CRITICAL flag > is used we should have a comment indicating why. I was not aware of this rule. I'll update the code accordingly. Thank you, Claudiu Beznea > >> .eid = PMC_SYSPLL, }, >> }, >>
Quoting Claudiu.Beznea@microchip.com (2020-11-16 03:24:54) > > > On 14.11.2020 23:14, Stephen Boyd wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Quoting Claudiu Beznea (2020-11-06 01:46:23) > >> diff --git a/drivers/clk/at91/clk-sam9x60-pll.c b/drivers/clk/at91/clk-sam9x60-pll.c > >> index 78f458a7b2ef..6fe5d8530a0c 100644 > >> --- a/drivers/clk/at91/clk-sam9x60-pll.c > >> +++ b/drivers/clk/at91/clk-sam9x60-pll.c > >> @@ -225,8 +225,51 @@ static int sam9x60_frac_pll_set_rate(struct clk_hw *hw, unsigned long rate, > >> unsigned long parent_rate) > >> { > >> struct sam9x60_pll_core *core = to_sam9x60_pll_core(hw); > >> + struct sam9x60_frac *frac = to_sam9x60_frac(core); > >> + struct regmap *regmap = core->regmap; > >> + unsigned long irqflags, clkflags = clk_hw_get_flags(hw); > >> + unsigned int val, cfrac, cmul; > >> + long ret; > >> + > >> + ret = sam9x60_frac_pll_compute_mul_frac(core, rate, parent_rate, true); > >> + if (ret <= 0 || (clkflags & CLK_SET_RATE_GATE)) > > > > Is this function being called when the clk is enabled and it has the > > CLK_SET_RATE_GATE flag set? > > Yes, this function could be called when CLK_SET_RATE_GATE is set. > On SAMA7G5 there are multiple PLL blocks of the same type. All these PLLs > are controlled by clk-sam9x60-pll.c driver. One of this PLL block fed the > CPU who's frequency could be changed at run time. At the same time there > are PLLs that fed hardware block not glitch free aware or that we don't > want to allow the rate change (this is the case of SAM9X60's CPU PLL, or > the DDR PLL on SAMA7G5). > > I'm confused why this driver needs to check > > this flag. > > Because we have multiple PLLs of the same type, some of them feed hardware > blocks that are glitch free aware of these PLLs' frequencies changes, some > feed hardware blocks that are not glitch free aware of PLLs' frequencies > changes or for some of them we don't want the frequency changes at all. Can we have different clk_ops for the different types of PLLs? It looks like the flag is being used to overload this function to do different things depending on how the flags are set. What happens if we decide to change the semantics of this clk flag? How does it map to this driver? Ideally this driver shouldn't need to worry about this flag, at least not in this function, except to figure out if it should do something different like not write the value to the hardware or something like that. The flag indicates to the clk framework that this clk should be gated when clk_set_rate() is called on it. The driver should be able to figure out that the clk is disabled by reading the hardware here and checking the enable state, or it could just have different clk_ops for the different type of PLL and do something different without checking the flag. Either way, checking the flag looks wrong. > >> - .c = 1, > >> + .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE, > > > > Please indicate why clks are critical. > > Sure! I'll do it in next version. I chose it like this because they are > feeding critical parts of the system like CPU or memory. > > > Whenever the CLK_IS_CRITICAL flag > > is used we should have a comment indicating why. > > I was not aware of this rule. I'll update the code accordingly. Sorry. I should put a document comment next to the flag.
On 18.11.2020 03:49, Stephen Boyd wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Quoting Claudiu.Beznea@microchip.com (2020-11-16 03:24:54) >> >> >> On 14.11.2020 23:14, Stephen Boyd wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> Quoting Claudiu Beznea (2020-11-06 01:46:23) >>>> diff --git a/drivers/clk/at91/clk-sam9x60-pll.c b/drivers/clk/at91/clk-sam9x60-pll.c >>>> index 78f458a7b2ef..6fe5d8530a0c 100644 >>>> --- a/drivers/clk/at91/clk-sam9x60-pll.c >>>> +++ b/drivers/clk/at91/clk-sam9x60-pll.c >>>> @@ -225,8 +225,51 @@ static int sam9x60_frac_pll_set_rate(struct clk_hw *hw, unsigned long rate, >>>> unsigned long parent_rate) >>>> { >>>> struct sam9x60_pll_core *core = to_sam9x60_pll_core(hw); >>>> + struct sam9x60_frac *frac = to_sam9x60_frac(core); >>>> + struct regmap *regmap = core->regmap; >>>> + unsigned long irqflags, clkflags = clk_hw_get_flags(hw); >>>> + unsigned int val, cfrac, cmul; >>>> + long ret; >>>> + >>>> + ret = sam9x60_frac_pll_compute_mul_frac(core, rate, parent_rate, true); >>>> + if (ret <= 0 || (clkflags & CLK_SET_RATE_GATE)) >>> >>> Is this function being called when the clk is enabled and it has the >>> CLK_SET_RATE_GATE flag set? >> >> Yes, this function could be called when CLK_SET_RATE_GATE is set. >> On SAMA7G5 there are multiple PLL blocks of the same type. All these PLLs >> are controlled by clk-sam9x60-pll.c driver. One of this PLL block fed the >> CPU who's frequency could be changed at run time. At the same time there >> are PLLs that fed hardware block not glitch free aware or that we don't >> want to allow the rate change (this is the case of SAM9X60's CPU PLL, or >> the DDR PLL on SAMA7G5). >> >> I'm confused why this driver needs to check >>> this flag. >> >> Because we have multiple PLLs of the same type, some of them feed hardware >> blocks that are glitch free aware of these PLLs' frequencies changes, some >> feed hardware blocks that are not glitch free aware of PLLs' frequencies >> changes or for some of them we don't want the frequency changes at all. > > Can we have different clk_ops for the different types of PLLs? Sure! I'll switch to this way. Thank you for your feedback, Claudiu Beznea > It looks > like the flag is being used to overload this function to do different > things depending on how the flags are set. What happens if we decide to > change the semantics of this clk flag? How does it map to this driver? > Ideally this driver shouldn't need to worry about this flag, at least > not in this function, except to figure out if it should do something > different like not write the value to the hardware or something like > that. > > The flag indicates to the clk framework that this clk should be gated > when clk_set_rate() is called on it. The driver should be able to figure > out that the clk is disabled by reading the hardware here and checking > the enable state, or it could just have different clk_ops for the > different type of PLL and do something different without checking the > flag. Either way, checking the flag looks wrong. > >>>> - .c = 1, >>>> + .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE, >>> >>> Please indicate why clks are critical. >> >> Sure! I'll do it in next version. I chose it like this because they are >> feeding critical parts of the system like CPU or memory. >> >>> Whenever the CLK_IS_CRITICAL flag >>> is used we should have a comment indicating why. >> >> I was not aware of this rule. I'll update the code accordingly. > > Sorry. I should put a document comment next to the flag. >
Hi, SAMA7G5 is capable of DVFS. The supported CPU clock frequencies could be obtained from CPU PLL. The hardware block diagram for clock feeding the CPU is as follows: +--------+ +-->|divider1|--> CPU clock | +--------+ +--------+ +----------+ | +--------+ |CPU PLL |-->|prescaller|--+-->|divider0|--> MCK0 clock +--------+ +----------+ +--------+ When switching CPU clock frequencies the MCK0 is also changed by DVFS driver to avoid its over/under clocking (depending on CPU clock frequency requested by DVFS algorithms). Some of IPs feed by MCK0 are MCK0 glich aware, some are not. For this MCK0 was removed from the parents list of the IPs which are not MCK0 glitch aware (patch 7/11). This series adapt AT91 clocks (mostly sam9x60-pll and master clock drivers) so that runtime changes of these clocks to be allowed. The CPU clock was registered from prescaller clock (see above diagram) and no software control has been added for divider1 because the frequencies supported by SAMA7G5's CPU could be directly obtained from CPU PLL + prescaller. On top of this series I also added a fix for sama7g5.c code (patch 1/11). Please let me know if you would like me to send this one separtely (it would be nice if this fix could be integrated in 5.10). Changes were tested on SAMA5D2, SAMA5D3, SAM9X60 and SAMA7G5. Thank you, Claudiu Beznea Changes in v4: - added Reviewed-by, Tested-by tags forgoten in v3 Changes in v3: - collected Reviewed-by, Tested-by tags - add patches 4/11, 5/11, 9/11 - in patch 10/11 use CLK_SET_RATE_GATE on MCK div and MCK pres for all the platforms except sama7g5 Changes in v2: - s/at91rm9200_mck_lock/at91sam9260_mck_lock in patch 7/8 Claudiu Beznea (7): clk: at91: sama7g5: fix compilation error clk: at91: clk-sam9x60-pll: allow runtime changes for pll clk: at91: sama7g5: remove mck0 from parent list of other clocks clk: at91: sama7g5: decrease lower limit for MCK0 rate clk: at91: sama7g5: do not allow cpu pll to go higher than 1GHz clk: at91: clk-master: re-factor master clock clk: at91: sama7g5: register cpu clock Eugen Hristev (4): dt-bindings: clock: at91: add sama7g5 pll defines clk: at91: sama7g5: allow SYS and CPU PLLs to be exported and referenced in DT clk: at91: clk-master: add 5th divisor for mck master clk: at91: sama7g5: add 5th divisor for mck0 layout and characteristics drivers/clk/at91/at91rm9200.c | 21 ++- drivers/clk/at91/at91sam9260.c | 26 ++- drivers/clk/at91/at91sam9g45.c | 32 +++- drivers/clk/at91/at91sam9n12.c | 36 ++-- drivers/clk/at91/at91sam9rl.c | 23 ++- drivers/clk/at91/at91sam9x5.c | 28 +++- drivers/clk/at91/clk-master.c | 328 +++++++++++++++++++++++++++++++------ drivers/clk/at91/clk-sam9x60-pll.c | 102 ++++++++++-- drivers/clk/at91/dt-compat.c | 15 +- drivers/clk/at91/pmc.h | 22 ++- drivers/clk/at91/sam9x60.c | 36 ++-- drivers/clk/at91/sama5d2.c | 42 +++-- drivers/clk/at91/sama5d3.c | 38 +++-- drivers/clk/at91/sama5d4.c | 40 +++-- drivers/clk/at91/sama7g5.c | 204 ++++++++++++++--------- include/dt-bindings/clock/at91.h | 11 ++ 16 files changed, 759 insertions(+), 245 deletions(-)