Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

["Måns Rullgård" <mans@mansr.com>]

Frank Oltmanns <frank@oltmanns.dev> writes:

> Hi Måns,
>
> 21.05.2024 15:43:10 Måns Rullgård <mans@mansr.com>:
>
>> Frank Oltmanns <frank@oltmanns.dev> 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@kernel.org>
>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>>> Cc: stable@vger.kernel.org
>>> ---
>>> 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