| Message ID | 20211209163720.106185-2-nikita@trvn.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Prepare general purpose clocks on msm8916 | expand |
Quoting Nikita Travkin (2021-12-09 08:37:17) > In cases when MND is not enabled (e.g. when only Half Integer Divider is > used), setting D registers makes no effect. Fail instead of making > ineffective write. > > Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG") > Signed-off-by: Nikita Travkin <nikita@trvn.ru> > --- > drivers/clk/qcom/clk-rcg2.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c > index e1b1b426fae4..6964cf914b60 100644 > --- a/drivers/clk/qcom/clk-rcg2.c > +++ b/drivers/clk/qcom/clk-rcg2.c > @@ -396,7 +396,7 @@ static int clk_rcg2_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) > static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) > { > struct clk_rcg2 *rcg = to_clk_rcg2(hw); > - u32 notn_m, n, m, d, not2d, mask, duty_per; > + u32 notn_m, n, m, d, not2d, mask, duty_per, cfg; > int ret; > > /* Duty-cycle cannot be modified for non-MND RCGs */ > @@ -407,6 +407,11 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) > > regmap_read(rcg->clkr.regmap, RCG_N_OFFSET(rcg), ¬n_m); > regmap_read(rcg->clkr.regmap, RCG_M_OFFSET(rcg), &m); > + regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); > + > + /* Duty-cycle cannot be modified if MND divider is in bypass mode. */ > + if (!(cfg & CFG_MODE_MASK)) > + return -EINVAL; Should we still allow 50% duty cycle to succeed?
Hi, Stephen Boyd писал(а) 08.01.2022 05:52: > Quoting Nikita Travkin (2021-12-09 08:37:17) >> In cases when MND is not enabled (e.g. when only Half Integer Divider is >> used), setting D registers makes no effect. Fail instead of making >> ineffective write. >> >> Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG") >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >> --- >> drivers/clk/qcom/clk-rcg2.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c >> index e1b1b426fae4..6964cf914b60 100644 >> --- a/drivers/clk/qcom/clk-rcg2.c >> +++ b/drivers/clk/qcom/clk-rcg2.c >> @@ -396,7 +396,7 @@ static int clk_rcg2_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) >> static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) >> { >> struct clk_rcg2 *rcg = to_clk_rcg2(hw); >> - u32 notn_m, n, m, d, not2d, mask, duty_per; >> + u32 notn_m, n, m, d, not2d, mask, duty_per, cfg; >> int ret; >> >> /* Duty-cycle cannot be modified for non-MND RCGs */ >> @@ -407,6 +407,11 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) >> >> regmap_read(rcg->clkr.regmap, RCG_N_OFFSET(rcg), ¬n_m); >> regmap_read(rcg->clkr.regmap, RCG_M_OFFSET(rcg), &m); >> + regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); >> + >> + /* Duty-cycle cannot be modified if MND divider is in bypass mode. */ >> + if (!(cfg & CFG_MODE_MASK)) >> + return -EINVAL; > > Should we still allow 50% duty cycle to succeed? *Technically* setting 50% duty cycle works since it's the default, but how I understand it, the main way to get there is to call clk_set_duty_cycle() which implies that it's caller intends to control duty cycle specifically but the call doesn't actually control anything as the hardware block is disabled. I'm adding this error here primarily to bring attention of the user (e.g. developer enabling some peripheral that needs duty cycle control) who might have to change their clock tree to make this control effective. So, assuming that if someone sets the duty cycle to 50% then they might set it to some other value later, it makes sense to fail the first call anyway. If you think there are some other possibilities for this call to happen specifically with 50% duty cycle (e.g. some preparations or cleanups in the clk subsystem or some drivers that I'm not aware of) then I can make an exemption in the check for that. Thanks, Nikita
Quoting Nikita Travkin (2022-01-07 23:25:19) > Hi, > > Stephen Boyd писал(а) 08.01.2022 05:52: > > Quoting Nikita Travkin (2021-12-09 08:37:17) > I'm adding this error here primarily to bring attention of the > user (e.g. developer enabling some peripheral that needs > duty cycle control) who might have to change their clock tree > to make this control effective. So, assuming that if someone > sets the duty cycle to 50% then they might set it to some other > value later, it makes sense to fail the first call anyway. > > If you think there are some other possibilities for this call > to happen specifically with 50% duty cycle (e.g. some > preparations or cleanups in the clk subsystem or some drivers > that I'm not aware of) then I can make an exemption in the check > for that. > I don't see anywhere in clk_set_duty_cycle() where it would bail out early if the duty cycle was set to what it already is. The default for these clks is 50%, so I worry that some driver may try to set the duty cycle to 50% and then fail now. Either we need to check the duty cycle in the core before calling down into the driver or we need to check it here in the driver. Can you send a patch to check the current duty cycle in the core before calling down into the clk ops?
Stephen Boyd писал(а) 11.01.2022 01:14: > Quoting Nikita Travkin (2022-01-07 23:25:19) >> Hi, >> >> Stephen Boyd писал(а) 08.01.2022 05:52: >> > Quoting Nikita Travkin (2021-12-09 08:37:17) >> I'm adding this error here primarily to bring attention of the >> user (e.g. developer enabling some peripheral that needs >> duty cycle control) who might have to change their clock tree >> to make this control effective. So, assuming that if someone >> sets the duty cycle to 50% then they might set it to some other >> value later, it makes sense to fail the first call anyway. >> >> If you think there are some other possibilities for this call >> to happen specifically with 50% duty cycle (e.g. some >> preparations or cleanups in the clk subsystem or some drivers >> that I'm not aware of) then I can make an exemption in the check >> for that. >> > > I don't see anywhere in clk_set_duty_cycle() where it would bail out > early if the duty cycle was set to what it already is. The default for > these clks is 50%, so I worry that some driver may try to set the duty > cycle to 50% and then fail now. Either we need to check the duty cycle > in the core before calling down into the driver or we need to check it > here in the driver. Can you send a patch to check the current duty cycle > in the core before calling down into the clk ops? Hi, sorry for a rather delayed response, I spent a bit of time looking at how to make the clk core be careful with ineffective duty-cycle calls and I can't find a nice way to do this... My idea was something like this: static int clk_core_set_duty_cycle_nolock(struct clk_core *core, struct clk_duty *duty) { /* ... */ /* Update core->duty values */ clk_core_update_duty_cycle_nolock(core); if ( /* duty doesn't match core->duty */ ) { ret = core->ops->set_duty_cycle(core->hw, duty); /* ... */ } However there seem to be drawbacks to any variant of the comparison that I could come up with: Naive one would be to do if (duty->num != core->duty->num || duty->den != core->duty->den) but it won't correctly compare e.g. 1/2 and 10/20. Other idea was to do if (duty->den / duty->num != core->duty->den / core->duty->num) but it will likely fail with very close values (e.g. 100/500 and 101/500) I briefly thought of some more sophisticated math but I don't like the idea of complicating this too far. I briefly grepped the kernel sources for duty-cycle related methods and I saw only one user of the clk_set_duty_cycle: sound/soc/meson/axg-tdm-interface.c Notably it sets the cycle to 1/2 in some cases, though it seems to be tied to the drivers/clk/meson/sclk-div.c clock driver by being the blocks of the same SoC. Thinking of it a bit more, I saw another approach to the problem I want to solve: Since I just want to make developers aware of the hardware quirk, maybe I don't need to fail the set but just put a WARN or even WARN_ONCE there? This way the behavior will be unchanged. Thanks, Nikita
Quoting Nikita Travkin (2022-01-26 07:14:21) > Stephen Boyd писал(а) 11.01.2022 01:14: > > Quoting Nikita Travkin (2022-01-07 23:25:19) > >> Hi, > >> > >> Stephen Boyd писал(а) 08.01.2022 05:52: > >> > Quoting Nikita Travkin (2021-12-09 08:37:17) > >> I'm adding this error here primarily to bring attention of the > >> user (e.g. developer enabling some peripheral that needs > >> duty cycle control) who might have to change their clock tree > >> to make this control effective. So, assuming that if someone > >> sets the duty cycle to 50% then they might set it to some other > >> value later, it makes sense to fail the first call anyway. > >> > >> If you think there are some other possibilities for this call > >> to happen specifically with 50% duty cycle (e.g. some > >> preparations or cleanups in the clk subsystem or some drivers > >> that I'm not aware of) then I can make an exemption in the check > >> for that. > >> > > > > I don't see anywhere in clk_set_duty_cycle() where it would bail out > > early if the duty cycle was set to what it already is. The default for > > these clks is 50%, so I worry that some driver may try to set the duty > > cycle to 50% and then fail now. Either we need to check the duty cycle > > in the core before calling down into the driver or we need to check it > > here in the driver. Can you send a patch to check the current duty cycle > > in the core before calling down into the clk ops? > > Hi, sorry for a rather delayed response, > I spent a bit of time looking at how to make the clk core be > careful with ineffective duty-cycle calls and I can't find a > nice way to do this... My idea was something like this: > > static int clk_core_set_duty_cycle_nolock(struct clk_core *core, > struct clk_duty *duty) > { /* ... */ > > /* Update core->duty values */ > clk_core_update_duty_cycle_nolock(core); > > if ( /* duty doesn't match core->duty */ ) { > ret = core->ops->set_duty_cycle(core->hw, duty); > /* ... */ > } > > However there seem to be drawbacks to any variant of the > comparison that I could come up with: > > Naive one would be to do > if (duty->num != core->duty->num || duty->den != core->duty->den) > but it won't correctly compare e.g. 1/2 and 10/20. > > Other idea was to do > if (duty->den / duty->num != core->duty->den / core->duty->num) > but it will likely fail with very close values (e.g. 100/500 and 101/500) > > I briefly thought of some more sophisticated math but I don't > like the idea of complicating this too far. > > I briefly grepped the kernel sources for duty-cycle related methods > and I saw only one user of the clk_set_duty_cycle: > sound/soc/meson/axg-tdm-interface.c > Notably it sets the cycle to 1/2 in some cases, though it seems to > be tied to the drivers/clk/meson/sclk-div.c clock driver by being > the blocks of the same SoC. Indeed, so this patch is untested? I doubt the qcom driver is being used with the one caller of clk_set_duty_cycle() in the kernel. > > Thinking of it a bit more, I saw another approach to the problem > I want to solve: Since I just want to make developers aware of the > hardware quirk, maybe I don't need to fail the set but just put a > WARN or even WARN_ONCE there? This way the behavior will be unchanged. > I don't like the idea of a WARN or a WARN_ONCE as most likely nobody is going to read it or do anything about it. Returning an error should be fine then. If the duty cycle call fails for 50% then that's something we have to live with.
Hi, Stephen Boyd писал(а) 18.02.2022 03:37: > Quoting Nikita Travkin (2022-01-26 07:14:21) >> Stephen Boyd писал(а) 11.01.2022 01:14: >> > Quoting Nikita Travkin (2022-01-07 23:25:19) >> >> Hi, >> >> >> >> Stephen Boyd писал(а) 08.01.2022 05:52: >> >> > Quoting Nikita Travkin (2021-12-09 08:37:17) >> >> I'm adding this error here primarily to bring attention of the >> >> user (e.g. developer enabling some peripheral that needs >> >> duty cycle control) who might have to change their clock tree >> >> to make this control effective. So, assuming that if someone >> >> sets the duty cycle to 50% then they might set it to some other >> >> value later, it makes sense to fail the first call anyway. >> >> >> >> If you think there are some other possibilities for this call >> >> to happen specifically with 50% duty cycle (e.g. some >> >> preparations or cleanups in the clk subsystem or some drivers >> >> that I'm not aware of) then I can make an exemption in the check >> >> for that. >> >> >> > >> > I don't see anywhere in clk_set_duty_cycle() where it would bail out >> > early if the duty cycle was set to what it already is. The default for >> > these clks is 50%, so I worry that some driver may try to set the duty >> > cycle to 50% and then fail now. Either we need to check the duty cycle >> > in the core before calling down into the driver or we need to check it >> > here in the driver. Can you send a patch to check the current duty cycle >> > in the core before calling down into the clk ops? >> >> Hi, sorry for a rather delayed response, >> I spent a bit of time looking at how to make the clk core be >> careful with ineffective duty-cycle calls and I can't find a >> nice way to do this... My idea was something like this: >> >> static int clk_core_set_duty_cycle_nolock(struct clk_core *core, >> struct clk_duty *duty) >> { /* ... */ >> >> /* Update core->duty values */ >> clk_core_update_duty_cycle_nolock(core); >> >> if ( /* duty doesn't match core->duty */ ) { >> ret = core->ops->set_duty_cycle(core->hw, duty); >> /* ... */ >> } >> >> However there seem to be drawbacks to any variant of the >> comparison that I could come up with: >> >> Naive one would be to do >> if (duty->num != core->duty->num || duty->den != core->duty->den) >> but it won't correctly compare e.g. 1/2 and 10/20. >> >> Other idea was to do >> if (duty->den / duty->num != core->duty->den / core->duty->num) >> but it will likely fail with very close values (e.g. 100/500 and 101/500) >> >> I briefly thought of some more sophisticated math but I don't >> like the idea of complicating this too far. >> >> I briefly grepped the kernel sources for duty-cycle related methods >> and I saw only one user of the clk_set_duty_cycle: >> sound/soc/meson/axg-tdm-interface.c >> Notably it sets the cycle to 1/2 in some cases, though it seems to >> be tied to the drivers/clk/meson/sclk-div.c clock driver by being >> the blocks of the same SoC. > > Indeed, so this patch is untested? I doubt the qcom driver is being used > with the one caller of clk_set_duty_cycle() in the kernel. > While right now, to my knowledge, there is no users of the duty cycle control, I'm adding a generic driver that uses it in another series [1] with an intention to use it across multiple qcom based devices. While making it I spent quite a bit of time staring at the oscilloscope to figure out that I need changes from patch 4/4 of this series and I'd like to make this quirk a bit more obvious to others. [1] https://lore.kernel.org/linux-pwm/20220212162342.72646-1-nikita@trvn.ru/ >> >> Thinking of it a bit more, I saw another approach to the problem >> I want to solve: Since I just want to make developers aware of the >> hardware quirk, maybe I don't need to fail the set but just put a >> WARN or even WARN_ONCE there? This way the behavior will be unchanged. >> > > I don't like the idea of a WARN or a WARN_ONCE as most likely nobody is > going to read it or do anything about it. Returning an error should be > fine then. If the duty cycle call fails for 50% then that's something we > have to live with. I intend this WARN or error to be hit by a person bringing up something new, user should never see it. For example a possible story could be: - Backlight control is connected to the clock on device X - Developer adds (future) pwm-clk adapter and pwm-backlight to the DT - Backlight slider in UI doesn't work anyway. (don't think UIs show errors here) - Developer troubleshoots the thing and either finds WARN in dmesg or that the sysfs write errors out. In my experience, people bringing devices up pay a very close attention to dmesg so I think giving a WARN is fine, but I'm fine with whichever approach you prefer. Nikita
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c index e1b1b426fae4..6964cf914b60 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -396,7 +396,7 @@ static int clk_rcg2_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) { struct clk_rcg2 *rcg = to_clk_rcg2(hw); - u32 notn_m, n, m, d, not2d, mask, duty_per; + u32 notn_m, n, m, d, not2d, mask, duty_per, cfg; int ret; /* Duty-cycle cannot be modified for non-MND RCGs */ @@ -407,6 +407,11 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty) regmap_read(rcg->clkr.regmap, RCG_N_OFFSET(rcg), ¬n_m); regmap_read(rcg->clkr.regmap, RCG_M_OFFSET(rcg), &m); + regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg); + + /* Duty-cycle cannot be modified if MND divider is in bypass mode. */ + if (!(cfg & CFG_MODE_MASK)) + return -EINVAL; n = (~(notn_m) + m) & mask;
In cases when MND is not enabled (e.g. when only Half Integer Divider is used), setting D registers makes no effect. Fail instead of making ineffective write. Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG") Signed-off-by: Nikita Travkin <nikita@trvn.ru> --- drivers/clk/qcom/clk-rcg2.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)