Re: [PATCH 1/4] clk: qcom: clk-rcg2: Fail Duty-Cycle configuration if MND divider is not enabled.

[Nikita Travkin <nikita@trvn.ru>]

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