Frank Oltmanns <frank@xxxxxxxxxxxx> writes:
> Hi Måns,
>
> 21.05.2024 15:43:10 Måns Rullgård <mans@xxxxxxxxx>:
>
>> Frank Oltmanns <frank@xxxxxxxxxxxx> writes:
>>
>>> The Allwinner SoC's typically have an upper and lower limit for their
>>> clocks' rates. Up until now, support for that has been implemented
>>> separately for each clock type.
>>>
>>> Implement that functionality in the sunxi-ng's common part making use of
>>> the CCF rate liming capabilities, so that it is available for all clock
>>> types.
>>>
>>> Suggested-by: Maxime Ripard <mripard@xxxxxxxxxx>
>>> Signed-off-by: Frank Oltmanns <frank@xxxxxxxxxxxx>
>>> Cc: stable@xxxxxxxxxxxxxxx
>>> ---
>>> drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>> drivers/clk/sunxi-ng/ccu_common.h | 3 +++
>>> 2 files changed, 22 insertions(+)
>>
>> This just landed in 6.6 stable, and it broke HDMI output on an A20 based
>> device, the clocks ending up all wrong as seen in this diff of
>> /sys/kernel/debug/clk/clk_summary:
>>
>> @@ -70,16 +71,14 @@
>> apb1-i2c0 0 0 0 24000000 0
>>
>> pll-gpu 0 0 0 1200000000 0
>> - pll-video1 3 3 1 159000000 0
>> + pll-video1 2 2 1 159000000 0
>> hdmi 1 1 0 39750000 0
>>
>> tcon0-ch1-sclk2 1 1 1 39750000 0
>> tcon0-ch1-sclk1 1 1 1 39750000 0
>>
>> - pll-video1-2x 1 1 0 318000000 0
>> + pll-video1-2x 0 0 0 318000000 0
>>
>> - hdmi-tmds 2 2 0 39750000 0
>> - hdmi-ddc 1 1 0 1987500 0
>> pll-periph-base 2 2 0 1200000000 0
>> mbus 1 1 0 300000000 0
>> pll-periph-sata 0 0 0 100000000 0
>> @@ -199,7 +198,7 @@
>>
>> ace 0 0 0 384000000 0
>> ve 0 0 0 384000000 0
>> - pll-video0 4 4 2 297000000 0
>> + pll-video0 5 5 2 297000000 0
>> hdmi1 0 0 0 297000000 0
>> tcon1-ch1-sclk2 0 0 0 297000000 0
>> tcon1-ch1-sclk1 0 0 0 297000000 0
>> @@ -222,8 +221,10 @@
>>
>> de-be0 1 1 1 297000000 0
>>
>> - pll-video0-2x 0 0 0 594000000 0
>> + pll-video0-2x 1 1 0 594000000 0
>>
>> + hdmi-tmds 2 2 0 594000000 0
>> + hdmi-ddc 1 1 0 29700000 0
>> pll-audio-base 0 0 0 1500000 0
>> pll-audio-8x 0 0 0 3000000 0
>> i2s2 0 0 0 3000000 0
>>
>> Reverting this commit makes it work again.
>
> Thank you for your detailed report!
>
> I've had a first look at hdmi-tmds and hdmi-ddc, and neither seems to
> be calling ccu_is_better_rate() in their determine_rate()
> functions. Their parents have the exact same rates in your diff, so,
> my current working assumption is that they can't be the cause either.
>
> I'll have a more detailed look over the weekend. Until then, if anyone
> has some ideas where I should have a look next, please share your
> thoughts.
In case it's relevant, this system doesn't use the HDMI DDC, the
physical DDC pins being connected to a different I2C adapter for
various reasons.
>From the clk_summary diff, I see a few things:
1. hdmi-tmds has changed parent from pll-video1-2x to pll-video0-2x.
2. The ratio of hdmi-tmds to its parent has changed from 1/8 to 1.
3. The resulting rate bears no relation to the pixel clock from EDID.
I tried kernel 6.9.1 as well, and that doesn't work either. I'll keep
digging and try to narrow it down.
>>> diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
>>> index 8babce55302f..ac0091b4ce24 100644
>>> --- a/drivers/clk/sunxi-ng/ccu_common.c
>>> +++ b/drivers/clk/sunxi-ng/ccu_common.c
>>> @@ -44,6 +44,16 @@ bool ccu_is_better_rate(struct ccu_common *common,
>>> unsigned long current_rate,
>>> unsigned long best_rate)
>>> {
>>> + unsigned long min_rate, max_rate;
>>> +
>>> + clk_hw_get_rate_range(&common->hw, &min_rate, &max_rate);
>>> +
>>> + if (current_rate > max_rate)
>>> + return false;
>>> +
>>> + if (current_rate < min_rate)
>>> + return false;
>>> +
>>> if (common->features & CCU_FEATURE_CLOSEST_RATE)
>>> return abs(current_rate - target_rate) < abs(best_rate - target_rate);
>>>
>>> @@ -122,6 +132,7 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>>>
>>> for (i = 0; i < desc->hw_clks->num ; i++) {
>>> struct clk_hw *hw = desc->hw_clks->hws[i];
>>> + struct ccu_common *common = hw_to_ccu_common(hw);
>>> const char *name;
>>>
>>> if (!hw)
>>> @@ -136,6 +147,14 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>>> pr_err("Couldn't register clock %d - %s\n", i, name);
>>> goto err_clk_unreg;
>>> }
>>> +
>>> + if (common->max_rate)
>>> + clk_hw_set_rate_range(hw, common->min_rate,
>>> + common->max_rate);
>>> + else
>>> + WARN(common->min_rate,
>>> + "No max_rate, ignoring min_rate of clock %d - %s\n",
>>> + i, name);
>>> }
>>>
>>> ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
>>> diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
>>> index 942a72c09437..329734f8cf42 100644
>>> --- a/drivers/clk/sunxi-ng/ccu_common.h
>>> +++ b/drivers/clk/sunxi-ng/ccu_common.h
>>> @@ -31,6 +31,9 @@ struct ccu_common {
>>> u16 lock_reg;
>>> u32 prediv;
>>>
>>> + unsigned long min_rate;
>>> + unsigned long max_rate;
>>> +
>>> unsigned long features;
>>> spinlock_t *lock;
>>> struct clk_hw hw;
>>>
>>> --
>>>
>>> 2.44.0
>>>
>>
>> --
>> Måns Rullgård
>
--
Måns Rullgård
