Message-ID: <e59a1c94-e6b3-49c8-aa78-78d031e8a6ec@microchip.com>
Date: Fri, 11 Jul 2025 07:38:22 +0000
From: <Varshini.Rajendran@...rochip.com>
To: <claudiu.beznea@...on.dev>, <mturquette@...libre.com>, <sboyd@...nel.org>,
	<Nicolas.Ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
	<linux-clk@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
CC: <Patrice.Vilchez@...rochip.com>
Subject: Re: [PATCH] clk: at91: sam9x7: update pll clk ranges

Hi Claudiu,

On 10/07/25 1:48 pm, Claudiu Beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi, Varshini,
> 
> On 24.06.2025 12:09, Varshini.Rajendran@...rochip.com wrote:
>> Hi Claudiu,
>>
>> On 24/06/25 12:34 pm, Claudiu Beznea wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi, Varshini,
>>>
>>> On 10.06.2025 11:45, Varshini Rajendran wrote:
>>>> Update the min, max ranges of the PLL clocks according to the latest
>>>> datasheet to be coherent in the driver. This patch apparently solves
>>>> issues in obtaining the right sdio frequency.
>>>>
>>>> Fixes: 33013b43e271 ("clk: at91: sam9x7: add sam9x7 pmc driver")
>>>> Suggested-by: Patrice Vilchez <Patrice.Vilchez@...rochip.com>
>>>> Signed-off-by: Varshini Rajendran <varshini.rajendran@...rochip.com>
>>>> ---
>>>>    drivers/clk/at91/sam9x7.c | 20 ++++++++++----------
>>>>    1 file changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/at91/sam9x7.c b/drivers/clk/at91/sam9x7.c
>>>> index cbb8b220f16b..ffab32b047a0 100644
>>>> --- a/drivers/clk/at91/sam9x7.c
>>>> +++ b/drivers/clk/at91/sam9x7.c
>>>> @@ -61,44 +61,44 @@ static const struct clk_master_layout sam9x7_master_layout = {
>>>>
>>>>    /* Fractional PLL core output range. */
>>>>    static const struct clk_range plla_core_outputs[] = {
>>>> -     { .min = 375000000, .max = 1600000000 },
>>>> +     { .min = 800000000, .max = 1600000000 },
>>>>    };
>>>>
>>>>    static const struct clk_range upll_core_outputs[] = {
>>>> -     { .min = 600000000, .max = 1200000000 },
>>>> +     { .min = 600000000, .max = 960000000 },
>>>>    };
>>>>
>>>>    static const struct clk_range lvdspll_core_outputs[] = {
>>>> -     { .min = 400000000, .max = 800000000 },
>>>> +     { .min = 600000000, .max = 1200000000 },
>>>>    };
>>>>
>>>>    static const struct clk_range audiopll_core_outputs[] = {
>>>> -     { .min = 400000000, .max = 800000000 },
>>>> +     { .min = 600000000, .max = 1200000000 },
>>>>    };
>>>>
>>>>    static const struct clk_range plladiv2_core_outputs[] = {
>>>> -     { .min = 375000000, .max = 1600000000 },
>>>> +     { .min = 800000000, .max = 1600000000 },
>>>>    };
>>>>
>>>>    /* Fractional PLL output range. */
>>>>    static const struct clk_range plla_outputs[] = {
>>>> -     { .min = 732421, .max = 800000000 },
>>>> +     { .min = 400000000, .max = 800000000 },
>>>>    };
>>>>
>>>>    static const struct clk_range upll_outputs[] = {
>>>> -     { .min = 300000000, .max = 600000000 },
>>>> +     { .min = 300000000, .max = 480000000 },
>>>>    };
>>>>
>>>>    static const struct clk_range lvdspll_outputs[] = {
>>>> -     { .min = 10000000, .max = 800000000 },
>>>> +     { .min = 175000000, .max = 550000000 },
>>>>    };
>>>>
>>>>    static const struct clk_range audiopll_outputs[] = {
>>>> -     { .min = 10000000, .max = 800000000 },
>>>> +     { .min = 0, .max = 300000000 },
>>>
>>> Is this min value something valid?
>>
>> Yes. This is a valid value mentioned in the datasheet. This is the range
>> that fixes the issue of not being able to set low frequency values for
>> the SDIO interface for instance.
> 
> Sorry for the late reply. This range is for audiopll. Can you please
> explain in the commit message why this fixes issues with the SDIO interface.

I can do that, while doing so, I can rephrase it as well. SDIO is just 
an example, It can potentially fix any range issues in any clock 
associated with peripherals can have.

> 
> Also, having zero here as min rate would involve, as of my code inspection,
> setting frac->mul = -1 for disabled PLLs on probe (I suppose audio PLLs are
> among them). Please check:
> 
> https://elixir.bootlin.com/linux/v6.15.5/source/drivers/clk/at91/clk-sam9x60-pll.c#L693
> 
> Having frac->mul = -1 means frac->mul = 0xff meaning the HW registers will
> be set with the maximum multiplier. If my assumption is right, for a
> reference clock of 24MHz this will mean the Fcorepll will be 384MHz which
> is not in range for Fcorepll specifications (described by
> audiopll_core_outputs[]).
> 
> This may be harmless if the PLL is unused at boot time, or reconfigured
> later, but if the PLL is just enabled later (w/o setting again the
> frequency) and the core output frequency is not in range (described by
> audiopll_core_outputs[]) it will lead to PLL lock to take forever and the
> execution to stuck.
> 
> Can you please check this?

I inspected the code on my side and did the calculations and I didn't 
quite get the part how you arrived at the value 384MHz, my bad. But I 
did get your point.

What if I set the minimum AudioPLL output that can be obtained from the 
coreclk output range which is 600MHz minimum, which gives us 2343750Hz 
as the minimum AudioPLL frequency achievable from the divider part. If 
that is okay I can send another version of this patch.

Thank you for spending the time to review my patch and for the detailed 
explanations.

> 
> Thank you,
> Claudiu
> 
> 
> 


-- 
Thanks,
Varshini Rajendran.

Powered by blists - more mailing lists