Patchwork clk: mvebu: armada-37xx-periph: Fix initialization for cpu clocks

login
register
mail settings
Submitter Gregory CLEMENT
Date March 14, 2019, 2:20 p.m.
Message ID <874l85v8p6.fsf@FE-laptop>
Download mbox | patch
Permalink /patch/748861/
State New
Headers show

Comments

Gregory CLEMENT - March 14, 2019, 2:20 p.m.
Hi Ilias,
 
 On jeu., mars 14 2019, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:

> Hello Christian,
>> Hi,
>> 
>> I assume you use the 1000 MHz firmware. This does also not work on my Rev 7
>> board. But I'm pretty sure this is not a problem of the patches, because if
>> I take a newer kernel (4.19.20/27) without the patches it also does not
>> work. A kernel 4.19.17 does work for me. My opinion on that is that this is
>> another problem which does just occure now because now the cpu frequency
>> scaling is working with the right frequencies.
> I am not sure which firmware i am running, i did all my tests on 5.0.0 and
> changing between governors worked fine without the patches

Curently my espressobin is broken so I tested the patches on the Armada
3700 DB and I didn't observe the issue you had.

The 3700 DB I used is configured to run at 800MHz.

Could you apply the following patch and sent me the boot log?




>
> Regards
> /Ilias
>> 
>> Ilias Apalodimas <ilias.apalodimas@linaro.org> schrieb am Do., 14. März
>> 2019, 13:15:
>> 
>> > Hi Gregory,
>> > > The clock parenting was not setup properly when DVFS was enabled. It was
>> > > expected that the same clock source was used with and without DVFS which
>> > > was not the case.
>> > >
>> > > This patch fixes this issue, allowing to make the cpufreq support work
>> > > when the CPU clocks source are not the default ones.
>> > >
>> > > Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")
>> > > Cc: <stable@vger.kernel.org>
>> > > Reported-by: Christian Neubert <christian.neubert.86@gmail.com>
>> > > Reported-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> > > Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> > > ---
>> > >  drivers/clk/mvebu/armada-37xx-periph.c | 11 +++++++++++
>> > >  1 file changed, 11 insertions(+)
>> > >
>> > > diff --git a/drivers/clk/mvebu/armada-37xx-periph.c
>> > b/drivers/clk/mvebu/armada-37xx-periph.c
>> > > index 1f1cff428d78..26ed3c18a239 100644
>> > > --- a/drivers/clk/mvebu/armada-37xx-periph.c
>> > > +++ b/drivers/clk/mvebu/armada-37xx-periph.c
>> > > @@ -671,6 +671,17 @@ static int armada_3700_add_composite_clk(const
>> > struct clk_periph_data *data,
>> > >               map = syscon_regmap_lookup_by_compatible(
>> > >                               "marvell,armada-3700-nb-pm");
>> > >               pmcpu_clk->nb_pm_base = map;
>> > > +
>> > > +             /*
>> > > +              * Use the same parent when DVFS is enabled that the
>> > > +              * default parent received at boot time. When this
>> > > +              * function is called, DVFS is not enabled yet, so we
>> > > +              * get the default parent and we can set the parent
>> > > +              * for DVFS.
>> > > +              */
>> > > +             if (clk_pm_cpu_set_parent(muxrate_hw,
>> > > +
>> >  clk_pm_cpu_get_parent(muxrate_hw)))
>> > > +                     dev_warn(dev, "Failed to setup default parent
>> > clock for DVFS\n");
>> > >       }
>> > >
>> > >       *hw = clk_hw_register_composite(dev, data->name,
>> > data->parent_names,
>> > > --
>> > > 2.20.1
>> > >
>> > Applied this and selected only
>> >
>> > CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
>> > CONFIG_CPU_FREQ_GOV_PERFORMANCE=y
>> > CONFIG_CPU_FREQ_GOV_POWERSAVE=y
>> >
>> > After changing the governor from 'powersave' to 'performance' the board
>> > completely froze (i even lost access to the serial port)
>> >
>> > Cheers
>> > /Ilias
>> >
Ilias Apalodimas - March 18, 2019, 11:28 a.m.
Hi Gregory, 
> Hi Ilias,
>  
>  On jeu., mars 14 2019, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> 
> > Hello Christian,
> >> Hi,
> >> 
> >> I assume you use the 1000 MHz firmware. This does also not work on my Rev 7
> >> board. But I'm pretty sure this is not a problem of the patches, because if
> >> I take a newer kernel (4.19.20/27) without the patches it also does not
> >> work. A kernel 4.19.17 does work for me. My opinion on that is that this is
> >> another problem which does just occure now because now the cpu frequency
> >> scaling is working with the right frequencies.
> > I am not sure which firmware i am running, i did all my tests on 5.0.0 and
> > changing between governors worked fine without the patches
> 
> Curently my espressobin is broken so I tested the patches on the Armada
> 3700 DB and I didn't observe the issue you had.
> 
> The 3700 DB I used is configured to run at 800MHz.
> 
> Could you apply the following patch and sent me the boot log?
> 
> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> index 26ed3c18a239..f814ade5cd80 100644
> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> @@ -452,14 +452,17 @@ static int clk_pm_cpu_set_parent(struct clk_hw *hw, u8 index)
>  
>         /* Set the parent clock for all the load level */
>         for (load_level = 0; load_level < LOAD_LEVEL_NR; load_level++) {
> -               unsigned int reg, mask,  val,
> +               unsigned int reg, mask,  val, old,
>                         offset = ARMADA_37XX_NB_TBG_SEL_OFF;
>  
>                 armada_3700_pm_dvfs_update_regs(load_level, &reg, &offset);
>  
>                 val = index << offset;
>                 mask = ARMADA_37XX_NB_TBG_SEL_MASK << offset;
> +               regmap_read(base, reg, &old);
>                 regmap_update_bits(base, reg, mask, val);
> +               pr_err("%s old=%X -> val=0x%X load_level=%d\n",
> +                      __func__, old,  val, load_level);
>         }
>         return 0;
>  }
> 
> 
> 
[   14.909524] clk_pm_cpu_set_parent old=28004840 -> val=0x6000000 load_level=0
[   14.916424] clk_pm_cpu_set_parent old=2E004840 -> val=0x600 load_level=1
[   14.923315] clk_pm_cpu_set_parent old=8880A8C0 -> val=0x6000000 load_level=2
[   14.930572] clk_pm_cpu_set_parent old=8E80A8C0 -> val=0x600 load_level=3


Let me know if you need anything else

Regards
/Ilias
Gregory CLEMENT - March 18, 2019, 11:40 a.m.
Hi Ilias,
 
 On lun., mars 18 2019, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:

> Hi Gregory, 
>> Hi Ilias,
>>  
>>  On jeu., mars 14 2019, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
>> 
>> > Hello Christian,
>> >> Hi,
>> >> 
>> >> I assume you use the 1000 MHz firmware. This does also not work on my Rev 7
>> >> board. But I'm pretty sure this is not a problem of the patches, because if
>> >> I take a newer kernel (4.19.20/27) without the patches it also does not
>> >> work. A kernel 4.19.17 does work for me. My opinion on that is that this is
>> >> another problem which does just occure now because now the cpu frequency
>> >> scaling is working with the right frequencies.
>> > I am not sure which firmware i am running, i did all my tests on 5.0.0 and
>> > changing between governors worked fine without the patches
>> 
>> Curently my espressobin is broken so I tested the patches on the Armada
>> 3700 DB and I didn't observe the issue you had.
>> 
>> The 3700 DB I used is configured to run at 800MHz.
>> 
>> Could you apply the following patch and sent me the boot log?
>> 
>> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
>> index 26ed3c18a239..f814ade5cd80 100644
>> --- a/drivers/clk/mvebu/armada-37xx-periph.c
>> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
>> @@ -452,14 +452,17 @@ static int clk_pm_cpu_set_parent(struct clk_hw *hw, u8 index)
>>  
>>         /* Set the parent clock for all the load level */
>>         for (load_level = 0; load_level < LOAD_LEVEL_NR; load_level++) {
>> -               unsigned int reg, mask,  val,
>> +               unsigned int reg, mask,  val, old,
>>                         offset = ARMADA_37XX_NB_TBG_SEL_OFF;
>>  
>>                 armada_3700_pm_dvfs_update_regs(load_level, &reg, &offset);
>>  
>>                 val = index << offset;
>>                 mask = ARMADA_37XX_NB_TBG_SEL_MASK << offset;
>> +               regmap_read(base, reg, &old);
>>                 regmap_update_bits(base, reg, mask, val);
>> +               pr_err("%s old=%X -> val=0x%X load_level=%d\n",
>> +                      __func__, old,  val, load_level);
>>         }
>>         return 0;
>>  }
>> 
>> 
>> 
> [   14.909524] clk_pm_cpu_set_parent old=28004840 -> val=0x6000000 load_level=0
> [   14.916424] clk_pm_cpu_set_parent old=2E004840 -> val=0x600 load_level=1
> [   14.923315] clk_pm_cpu_set_parent old=8880A8C0 -> val=0x6000000 load_level=2
> [   14.930572] clk_pm_cpu_set_parent old=8E80A8C0 -> val=0x600 load_level=3
>
>
> Let me know if you need anything else

Could you show me the output of "cat /sys/kernel/debug/clk/clk_summary"

Also, during this week-end, Christian suggested that the issue might
come from the AVS support.

Could you disable it and check you still have the issue?

For this, you just have to remove the avs node in
arch/arm64/boot/dts/marvell/armada-37xx.dtsi and rebuild the dtb.

Thanks,

Gregory

>
> Regards
> /Ilias
Ilias Apalodimas - March 18, 2019, 12:21 p.m.
Hi Gregory, 

> >> 
> > [   14.909524] clk_pm_cpu_set_parent old=28004840 -> val=0x6000000 load_level=0
> > [   14.916424] clk_pm_cpu_set_parent old=2E004840 -> val=0x600 load_level=1
> > [   14.923315] clk_pm_cpu_set_parent old=8880A8C0 -> val=0x6000000 load_level=2
> > [   14.930572] clk_pm_cpu_set_parent old=8E80A8C0 -> val=0x600 load_level=3
> >
> >
> > Let me know if you need anything else
> 
> Could you show me the output of "cat /sys/kernel/debug/clk/clk_summary"
> 
root@localhost ~ # cat /sys/kernel/debug/clk/clk_summary
                                 enable  prepare  protect                                duty
   clock                          count    count    count        rate   accuracy phase  cycle
---------------------------------------------------------------------------------------------
 xtal                                 2        3        0    25000000          0     0  50000
    i2c_1                             0        0        0    25000000          0     0  50000
    i2c_2                             0        0        0    25000000          0     0  50000
    avs                               0        0        0    25000000          0     0  50000
    TBG-B-S                           2        2        0  1000000000          0     0  50000
       cpu                            0        0        0   200000000          0     0  50000
       usb32_ss_sys                   1        1        0   125000000          0     0  50000
       usb32_usb2_sys                 0        0        0   100000000          0     0  50000
       gbe_125                        0        0        0   125000000          0     0  50000
          gbe0_125                    0        0        0   125000000          0     0  50000
          gbe1_125                    0        0        0   125000000          0     0  50000
       gbe_core                       1        1        0   250000000          0     0  50000
          gbe_bm                      0        0        0   250000000          0     0  50000
          gbe0_core                   1        1        0   250000000          0     0  50000
          gbe1_core                   0        0        0   250000000          0     0  50000
       eip97                          0        0        0   500000000          0     0  50000
       trace                          0        0        0  1000000000          0     0  50000
       ddr_fclk                       0        0        0   125000000          0     0  50000
       pwm                            0        0        0    50000000          0     0  50000
       tscem_tmx                      0        0        0  1000000000          0     0  50000
       sec_dap                        0        0        0   100000000          0     0  50000
       sec_at                         0        0        0   200000000          0     0  50000
    TBG-A-S                           0        0        0  1600000000          0     0  50000
       sdio                           0        0        0   400000000          0     0  50000
       ddr_phy                        0        0        0   400000000          0     0  50000
       mmc                            0        0        0   400000000          0     0  50000
    TBG-B-P                           0        0        0   500000000          0     0  50000
       gbe_50                         0        0        0    50000000          0     0  50000
          gbe0_50                     0        0        0    50000000          0     0  50000
          gbe1_50                     0        0        0    50000000          0     0  50000
    TBG-A-P                           0        1        0   800000000          0     0  50000
       counter                        0        0        0   160000000          0     0  50000
       sqf                            0        1        0   200000000          0     0  50000
       tscem                          0        0        0   200000000          0     0  50000
       sata_host                      0        0        0   200000000          0     0  50000
root@localhost ~ # 

> Also, during this week-end, Christian suggested that the issue might
> come from the AVS support.
> 
> Could you disable it and check you still have the issue?
> 
> For this, you just have to remove the avs node in
> arch/arm64/boot/dts/marvell/armada-37xx.dtsi and rebuild the dtb.
Sure. You'll have to wait for a week though. Currently on a trip. I'll run that
 once i return


Cheers
/Ilias

Patch

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 26ed3c18a239..f814ade5cd80 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -452,14 +452,17 @@  static int clk_pm_cpu_set_parent(struct clk_hw *hw, u8 index)
 
        /* Set the parent clock for all the load level */
        for (load_level = 0; load_level < LOAD_LEVEL_NR; load_level++) {
-               unsigned int reg, mask,  val,
+               unsigned int reg, mask,  val, old,
                        offset = ARMADA_37XX_NB_TBG_SEL_OFF;
 
                armada_3700_pm_dvfs_update_regs(load_level, &reg, &offset);
 
                val = index << offset;
                mask = ARMADA_37XX_NB_TBG_SEL_MASK << offset;
+               regmap_read(base, reg, &old);
                regmap_update_bits(base, reg, mask, val);
+               pr_err("%s old=%X -> val=0x%X load_level=%d\n",
+                      __func__, old,  val, load_level);
        }
        return 0;
 }