diff mbox series

[v4,8/9] mips: ralink: get cpu rate from new driver code

Message ID 20230617052435.359177-9-sergio.paracuellos@gmail.com
State New
Headers
Series mips: ralink: add complete clock and reset driver for mtmips SoCs |

Commit Message

Sergio Paracuellos June 17, 2023, 5:24 a.m. UTC 
  At very early stage on boot, there is a need to set 'mips_hpt_frequency'.
This timer frequency is a half of the CPU frequency. To get clocks properly
set we need to call to 'of_clk_init()' and properly get cpu clock frequency
afterwards. Depending on the SoC, CPU clock index in the clock provider is
different being two for MT7620 SoC and one for the rest. Hence, adapt code
to be aligned with new clock driver.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/ralink/clk.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

Comments

Krzysztof Kozlowski June 17, 2023, 2:48 p.m. UTC | #1 
On 17/06/2023 07:24, Sergio Paracuellos wrote:
> At very early stage on boot, there is a need to set 'mips_hpt_frequency'.
> This timer frequency is a half of the CPU frequency. To get clocks properly
> set we need to call to 'of_clk_init()' and properly get cpu clock frequency
> afterwards. Depending on the SoC, CPU clock index in the clock provider is
> different being two for MT7620 SoC and one for the rest. Hence, adapt code
> to be aligned with new clock driver.


>  void __init plat_time_init(void)
>  {
> +	struct of_phandle_args clkspec;
>  	struct clk *clk;
> +	int cpu_clk_idx;
>  
>  	ralink_of_remap();
>  
> -	ralink_clk_init();
> -	clk = clk_get_sys("cpu", NULL);
> +	cpu_clk_idx = clk_cpu_index();
> +	if (cpu_clk_idx == -1)
> +		panic("unable to get CPU clock index");
> +
> +	of_clk_init(NULL);
> +	clkspec.np = of_find_node_by_name(NULL, "sysc");
> +	clkspec.args_count = 1;
> +	clkspec.args[0] = cpu_clk_idx;
> +	clk = of_clk_get_from_provider(&clkspec);

This is very obfuscated way of getting clock. Why can't you get it from
"clocks" property of "cpu", like every other recent platform?

Anyway, NAK for of_find_node_by_name(), because you now create ABI for
node name. It's broken approach.

Best regards,
Krzysztof
Sergio Paracuellos June 17, 2023, 3:35 p.m. UTC | #2 
On Sat, Jun 17, 2023 at 4:48 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/06/2023 07:24, Sergio Paracuellos wrote:
> > At very early stage on boot, there is a need to set 'mips_hpt_frequency'.
> > This timer frequency is a half of the CPU frequency. To get clocks properly
> > set we need to call to 'of_clk_init()' and properly get cpu clock frequency
> > afterwards. Depending on the SoC, CPU clock index in the clock provider is
> > different being two for MT7620 SoC and one for the rest. Hence, adapt code
> > to be aligned with new clock driver.
>
>
> >  void __init plat_time_init(void)
> >  {
> > +     struct of_phandle_args clkspec;
> >       struct clk *clk;
> > +     int cpu_clk_idx;
> >
> >       ralink_of_remap();
> >
> > -     ralink_clk_init();
> > -     clk = clk_get_sys("cpu", NULL);
> > +     cpu_clk_idx = clk_cpu_index();
> > +     if (cpu_clk_idx == -1)
> > +             panic("unable to get CPU clock index");
> > +
> > +     of_clk_init(NULL);
> > +     clkspec.np = of_find_node_by_name(NULL, "sysc");
> > +     clkspec.args_count = 1;
> > +     clkspec.args[0] = cpu_clk_idx;
> > +     clk = of_clk_get_from_provider(&clkspec);
>
> This is very obfuscated way of getting clock. Why can't you get it from
> "clocks" property of "cpu", like every other recent platform?

I did not find any other approach that works for me. So I ended up in this one.
Can you point me out in a sample of code doing the same so I can check
if it works for me then?

>
> Anyway, NAK for of_find_node_by_name(), because you now create ABI for
> node name. It's broken approach.

I will change whatever is needed to provide a valid approach. Please,
point me out in the right direction.

>
> Best regards,
> Krzysztof
>

Thanks in advance for your time.

Best regards,
    Sergio Paracuellos
Krzysztof Kozlowski June 17, 2023, 5:27 p.m. UTC | #3 
On 17/06/2023 17:35, Sergio Paracuellos wrote:
> On Sat, Jun 17, 2023 at 4:48 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/06/2023 07:24, Sergio Paracuellos wrote:
>>> At very early stage on boot, there is a need to set 'mips_hpt_frequency'.
>>> This timer frequency is a half of the CPU frequency. To get clocks properly
>>> set we need to call to 'of_clk_init()' and properly get cpu clock frequency
>>> afterwards. Depending on the SoC, CPU clock index in the clock provider is
>>> different being two for MT7620 SoC and one for the rest. Hence, adapt code
>>> to be aligned with new clock driver.
>>
>>
>>>  void __init plat_time_init(void)
>>>  {
>>> +     struct of_phandle_args clkspec;
>>>       struct clk *clk;
>>> +     int cpu_clk_idx;
>>>
>>>       ralink_of_remap();
>>>
>>> -     ralink_clk_init();
>>> -     clk = clk_get_sys("cpu", NULL);
>>> +     cpu_clk_idx = clk_cpu_index();
>>> +     if (cpu_clk_idx == -1)
>>> +             panic("unable to get CPU clock index");
>>> +
>>> +     of_clk_init(NULL);
>>> +     clkspec.np = of_find_node_by_name(NULL, "sysc");
>>> +     clkspec.args_count = 1;
>>> +     clkspec.args[0] = cpu_clk_idx;
>>> +     clk = of_clk_get_from_provider(&clkspec);
>>
>> This is very obfuscated way of getting clock. Why can't you get it from
>> "clocks" property of "cpu", like every other recent platform?
> 
> I did not find any other approach that works for me. So I ended up in this one.
> Can you point me out in a sample of code doing the same so I can check
> if it works for me then?

You mean bindings?
git grep cpus.yaml

Driver?
git grep clk_get_rate
clk_get
eventually of_clk_get

It all depends on the context.

Anyway, it is very easy to find existing solutions not using
of_find_node_by_name for your platform:

git grep mips_hpt_frequency

First result.


Best regards,
Krzysztof
Sergio Paracuellos June 17, 2023, 7:01 p.m. UTC | #4 
On Sat, Jun 17, 2023 at 7:27 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/06/2023 17:35, Sergio Paracuellos wrote:
> > On Sat, Jun 17, 2023 at 4:48 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 17/06/2023 07:24, Sergio Paracuellos wrote:
> >>> At very early stage on boot, there is a need to set 'mips_hpt_frequency'.
> >>> This timer frequency is a half of the CPU frequency. To get clocks properly
> >>> set we need to call to 'of_clk_init()' and properly get cpu clock frequency
> >>> afterwards. Depending on the SoC, CPU clock index in the clock provider is
> >>> different being two for MT7620 SoC and one for the rest. Hence, adapt code
> >>> to be aligned with new clock driver.
> >>
> >>
> >>>  void __init plat_time_init(void)
> >>>  {
> >>> +     struct of_phandle_args clkspec;
> >>>       struct clk *clk;
> >>> +     int cpu_clk_idx;
> >>>
> >>>       ralink_of_remap();
> >>>
> >>> -     ralink_clk_init();
> >>> -     clk = clk_get_sys("cpu", NULL);
> >>> +     cpu_clk_idx = clk_cpu_index();
> >>> +     if (cpu_clk_idx == -1)
> >>> +             panic("unable to get CPU clock index");
> >>> +
> >>> +     of_clk_init(NULL);
> >>> +     clkspec.np = of_find_node_by_name(NULL, "sysc");
> >>> +     clkspec.args_count = 1;
> >>> +     clkspec.args[0] = cpu_clk_idx;
> >>> +     clk = of_clk_get_from_provider(&clkspec);
> >>
> >> This is very obfuscated way of getting clock. Why can't you get it from
> >> "clocks" property of "cpu", like every other recent platform?
> >
> > I did not find any other approach that works for me. So I ended up in this one.
> > Can you point me out in a sample of code doing the same so I can check
> > if it works for me then?
>
> You mean bindings?
> git grep cpus.yaml
>
> Driver?
> git grep clk_get_rate
> clk_get
> eventually of_clk_get
>
> It all depends on the context.
>
> Anyway, it is very easy to find existing solutions not using
> of_find_node_by_name for your platform:
>
> git grep mips_hpt_frequency
>
> First result.

I have tested before all of these and got into panic because clock
cannot get retrieved:

For example first result is to make use of clk_get so change the code into:

void __init plat_time_init(void)
{
    struct clk *clk;

    of_clk_init(NULL);
    clk = clk_get(NULL, "cpu");
    if (IS_ERR(clk))
       panic("unable to get CPU clock, err=%ld", PTR_ERR(clk));

    pr_info("CPU Clock: %ldMHz\n", clk_get_rate(clk) / 1000000);
    mips_hpt_frequency = clk_get_rate(clk) / 2;
    clk_put(clk);
    timer_probe();
}

And I panic because I cannot get the cpu clock...

>
>
> Best regards,
> Krzysztof
>

Thanks,
    Sergio Paracuellos
diff mbox series

Patch

diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
index 5b02bb7e0829..3d29e956f785 100644
--- a/arch/mips/ralink/clk.c
+++ b/arch/mips/ralink/clk.c
@@ -11,29 +11,41 @@ 
 #include <linux/clkdev.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <asm/mach-ralink/ralink_regs.h>
 
 #include <asm/time.h>
 
 #include "common.h"
 
-void ralink_clk_add(const char *dev, unsigned long rate)
+static int clk_cpu_index(void)
 {
-	struct clk *clk = clk_register_fixed_rate(NULL, dev, NULL, 0, rate);
+	if (ralink_soc == RALINK_UNKNOWN)
+		return -1;
 
-	if (!clk)
-		panic("failed to add clock");
+	if (ralink_soc == MT762X_SOC_MT7620A ||
+	    ralink_soc == MT762X_SOC_MT7620N)
+		return 2;
 
-	clkdev_create(clk, NULL, "%s", dev);
+	return 1;
 }
 
 void __init plat_time_init(void)
 {
+	struct of_phandle_args clkspec;
 	struct clk *clk;
+	int cpu_clk_idx;
 
 	ralink_of_remap();
 
-	ralink_clk_init();
-	clk = clk_get_sys("cpu", NULL);
+	cpu_clk_idx = clk_cpu_index();
+	if (cpu_clk_idx == -1)
+		panic("unable to get CPU clock index");
+
+	of_clk_init(NULL);
+	clkspec.np = of_find_node_by_name(NULL, "sysc");
+	clkspec.args_count = 1;
+	clkspec.args[0] = cpu_clk_idx;
+	clk = of_clk_get_from_provider(&clkspec);
 	if (IS_ERR(clk))
 		panic("unable to get CPU clock, err=%ld", PTR_ERR(clk));
 	pr_info("CPU Clock: %ldMHz\n", clk_get_rate(clk) / 1000000);