Some devices make use of general purpose clocks as PWM outputs by
controlling their duty cycle.
Notably, many devices (e.g. Samsung A3/A5, LG G Watch R and probably
many others) use clock based PWM to control the haptic feedback,
some other can control backlight or flash/torch LED brightness.
As a follow-up to a proposed clock based PWM output driver [1],
this series contains various fixes to make it useful on msm8916
based devices.
[1] - https://lore.kernel.org/lkml/[email protected]/T/
Nikita Travkin (4):
clk: qcom: clk-rcg2: Fail Duty-Cycle configuration if MND divider is
not enabled.
clk: qcom: clk-rcg2: Make sure to not write d=0 to the NMD register
pinctrl: qcom: msm8916: Allow CAMSS GP clocks to be muxed
clk: qcom: gcc-msm8916: Add rates to the GP clocks
drivers/clk/qcom/clk-rcg2.c | 11 +++++++-
drivers/clk/qcom/gcc-msm8916.c | 35 ++++++++++++++++++++++++++
drivers/pinctrl/qcom/pinctrl-msm8916.c | 4 +--
3 files changed, 47 insertions(+), 3 deletions(-)
--
2.30.2
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 <[email protected]>
---
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;
n = (~(notn_m) + m) & mask;
--
2.30.2
Sometimes calculation of d value may result in 0 because of the
rounding after integer division. This causes the following error:
[ 113.969689] camss_gp1_clk_src: rcg didn't update its configuration.
[ 113.969754] WARNING: CPU: 3 PID: 35 at drivers/clk/qcom/clk-rcg2.c:122 update_config+0xc8/0xdc
Make sure that D value is never zero.
Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG")
Signed-off-by: Nikita Travkin <[email protected]>
---
drivers/clk/qcom/clk-rcg2.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 6964cf914b60..fdfd43e2a01b 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -424,6 +424,10 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
if (d > mask)
d = mask;
+ /* Hardware can't handle d=0, make sure it's at least 1 */
+ if (!d)
+ d = 1;
+
if ((d / 2) > (n - m))
d = (n - m) * 2;
else if ((d / 2) < (m / 2))
--
2.30.2
msm8916 has (at least) 6 "General Purpose" clocks that can be muxed to
SoC pins. These clocks are:
GP_CLK{0, 1} : GPIO_{31, 32} (Belongs to CAMSS according to Linux)
GP_CLK_{1-3}{A, B} : GPIO_{49-51, 97, 12, 13} (Belongs to GCC itself)
GP_MN : GPIO_110 (Doesn't seem to be described in gcc,
ignored in this patch)
Those clocks may be used as e.g. PWM sources for external peripherals.
Add more frequencies to the table for those clocks so it's possible
for arbitrary peripherals to make use of them.
Signed-off-by: Nikita Travkin <[email protected]>
---
drivers/clk/qcom/gcc-msm8916.c | 35 ++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/clk/qcom/gcc-msm8916.c b/drivers/clk/qcom/gcc-msm8916.c
index 17e4a5a2a9fd..9a46794f6eb8 100644
--- a/drivers/clk/qcom/gcc-msm8916.c
+++ b/drivers/clk/qcom/gcc-msm8916.c
@@ -765,7 +765,20 @@ static struct clk_rcg2 cci_clk_src = {
},
};
+/*
+ * This is a frequency table for "General Purpose" clocks.
+ * These clocks can be muxed to the SoC pins and may be used by
+ * external devices. They're often used as PWM source.
+ *
+ * See comment at ftbl_gcc_gp1_3_clk.
+ */
static const struct freq_tbl ftbl_gcc_camss_gp0_1_clk[] = {
+ F(10000, P_XO, 16, 1, 120),
+ F(100000, P_XO, 16, 1, 12),
+ F(500000, P_GPLL0, 16, 1, 100),
+ F(1000000, P_GPLL0, 16, 1, 50),
+ F(2500000, P_GPLL0, 16, 1, 20),
+ F(5000000, P_GPLL0, 16, 1, 10),
F(100000000, P_GPLL0, 8, 0, 0),
F(200000000, P_GPLL0, 4, 0, 0),
{ }
@@ -927,7 +940,29 @@ static struct clk_rcg2 crypto_clk_src = {
},
};
+/*
+ * This is a frequency table for "General Purpose" clocks.
+ * These clocks can be muxed to the SoC pins and may be used by
+ * external devices. They're often used as PWM source.
+ *
+ * Please note that MND divider must be enabled for duty-cycle
+ * control to be possible. (M != N) Also since D register is configured
+ * with a value multiplied by 2, and duty cycle is calculated as
+ * (2 * D) % 2^W
+ * DutyCycle = ----------------
+ * 2 * (N % 2^W)
+ * (where W = .mnd_width)
+ * N must be half or less than maximum value for the register.
+ * Otherwise duty-cycle control would be limited.
+ * (e.g. for 8-bit NMD N should be less than 128)
+ */
static const struct freq_tbl ftbl_gcc_gp1_3_clk[] = {
+ F(10000, P_XO, 16, 1, 120),
+ F(100000, P_XO, 16, 1, 12),
+ F(500000, P_GPLL0, 16, 1, 100),
+ F(1000000, P_GPLL0, 16, 1, 50),
+ F(2500000, P_GPLL0, 16, 1, 20),
+ F(5000000, P_GPLL0, 16, 1, 10),
F(19200000, P_XO, 1, 0, 0),
{ }
};
--
2.30.2
GPIO 31, 32 can be muxed to GCC_CAMSS_GP(1,2)_CLK respectively but the
function was never assigned to the pingroup (even though the function
exists already).
Add this mode to the related pins.
Fixes: 5373a2c5abb6 ("pinctrl: qcom: Add msm8916 pinctrl driver")
Signed-off-by: Nikita Travkin <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm8916.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm8916.c b/drivers/pinctrl/qcom/pinctrl-msm8916.c
index 396db12ae904..bf68913ba821 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm8916.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm8916.c
@@ -844,8 +844,8 @@ static const struct msm_pingroup msm8916_groups[] = {
PINGROUP(28, pwr_modem_enabled_a, NA, NA, NA, NA, NA, qdss_tracedata_b, NA, atest_combodac),
PINGROUP(29, cci_i2c, NA, NA, NA, NA, NA, qdss_tracedata_b, NA, atest_combodac),
PINGROUP(30, cci_i2c, NA, NA, NA, NA, NA, NA, NA, qdss_tracedata_b),
- PINGROUP(31, cci_timer0, NA, NA, NA, NA, NA, NA, NA, NA),
- PINGROUP(32, cci_timer1, NA, NA, NA, NA, NA, NA, NA, NA),
+ PINGROUP(31, cci_timer0, flash_strobe, NA, NA, NA, NA, NA, NA, NA),
+ PINGROUP(32, cci_timer1, flash_strobe, NA, NA, NA, NA, NA, NA, NA),
PINGROUP(33, cci_async, NA, NA, NA, NA, NA, NA, NA, qdss_tracedata_b),
PINGROUP(34, pwr_nav_enabled_a, NA, NA, NA, NA, NA, NA, NA, qdss_tracedata_b),
PINGROUP(35, pwr_crypto_enabled_a, NA, NA, NA, NA, NA, NA, NA, qdss_tracedata_b),
--
2.30.2
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 <[email protected]>
> ---
> 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?
Quoting Nikita Travkin (2021-12-09 08:37:20)
> msm8916 has (at least) 6 "General Purpose" clocks that can be muxed to
> SoC pins. These clocks are:
>
> GP_CLK{0, 1} : GPIO_{31, 32} (Belongs to CAMSS according to Linux)
> GP_CLK_{1-3}{A, B} : GPIO_{49-51, 97, 12, 13} (Belongs to GCC itself)
> GP_MN : GPIO_110 (Doesn't seem to be described in gcc,
> ignored in this patch)
>
> Those clocks may be used as e.g. PWM sources for external peripherals.
> Add more frequencies to the table for those clocks so it's possible
> for arbitrary peripherals to make use of them.
>
> Signed-off-by: Nikita Travkin <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
Quoting Nikita Travkin (2021-12-09 08:37:18)
> Sometimes calculation of d value may result in 0 because of the
> rounding after integer division. This causes the following error:
>
> [ 113.969689] camss_gp1_clk_src: rcg didn't update its configuration.
> [ 113.969754] WARNING: CPU: 3 PID: 35 at drivers/clk/qcom/clk-rcg2.c:122 update_config+0xc8/0xdc
>
> Make sure that D value is never zero.
>
> Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG")
> Signed-off-by: Nikita Travkin <[email protected]>
> ---
> drivers/clk/qcom/clk-rcg2.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 6964cf914b60..fdfd43e2a01b 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -424,6 +424,10 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
> if (d > mask)
> d = mask;
>
> + /* Hardware can't handle d=0, make sure it's at least 1 */
> + if (!d)
> + d = 1;
Maybe clamp() would be better
/*
* Check bit widths of 2d. If D is too big reduce duty cycle and
* ensure it is non-zero.
*/
clamp(d, 1, mask);
> +
> if ((d / 2) > (n - m))
> d = (n - m) * 2;
> else if ((d / 2) < (m / 2))
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 <[email protected]>
>> ---
>> 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
Hi,
Stephen Boyd писал(а) 08.01.2022 05:56:
> Quoting Nikita Travkin (2021-12-09 08:37:18)
>> Sometimes calculation of d value may result in 0 because of the
>> rounding after integer division. This causes the following error:
>>
>> [ 113.969689] camss_gp1_clk_src: rcg didn't update its configuration.
>> [ 113.969754] WARNING: CPU: 3 PID: 35 at drivers/clk/qcom/clk-rcg2.c:122 update_config+0xc8/0xdc
>>
>> Make sure that D value is never zero.
>>
>> Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG")
>> Signed-off-by: Nikita Travkin <[email protected]>
>> ---
>> drivers/clk/qcom/clk-rcg2.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
>> index 6964cf914b60..fdfd43e2a01b 100644
>> --- a/drivers/clk/qcom/clk-rcg2.c
>> +++ b/drivers/clk/qcom/clk-rcg2.c
>> @@ -424,6 +424,10 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
>> if (d > mask)
>> d = mask;
>>
>> + /* Hardware can't handle d=0, make sure it's at least 1 */
>> + if (!d)
>> + d = 1;
>
> Maybe clamp() would be better
>
> /*
> * Check bit widths of 2d. If D is too big reduce duty cycle and
> * ensure it is non-zero.
> */
> clamp(d, 1, mask);
>
Yes, this looks nicer, will use it in v2. Thanks for the suggestion!
Nikita
>> +
>> if ((d / 2) > (n - m))
>> d = (n - m) * 2;
>> else if ((d / 2) < (m / 2))
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/[email protected]/
>>
>> 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