Re: [PATCH 2/3] clk: renesas: r9a07g044: Add MSTOP for RZ/G2L

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Geert,

On 04.08.2025 13:00, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, 4 Jul 2025 at 15:43, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Add MSTOP configuration for all the module clocks on the RZ/G2L
>> based SoCs (RZ/G2L, RZ/G2LC).
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/clk/renesas/r9a07g044-cpg.c
>> +++ b/drivers/clk/renesas/r9a07g044-cpg.c
>> @@ -242,163 +242,163 @@ static const struct {
>>  } mod_clks = {
>>         .common = {
> 
>>                 DEF_MOD("gpu_clk",      R9A07G044_GPU_CLK, R9A07G044_CLK_G,
>> -                                       0x558, 0, 0),
>> +                                       0x558, 0, MSTOP(BUS_REG1, BIT(4))),
>>                 DEF_MOD("gpu_axi_clk",  R9A07G044_GPU_AXI_CLK, R9A07G044_CLK_P1,
>>                                         0x558, 1, 0),
>>                 DEF_MOD("gpu_ace_clk",  R9A07G044_GPU_ACE_CLK, R9A07G044_CLK_P1,
>>                                         0x558, 2, 0),
> 
> Perhaps these two should have "MSTOP(BUS_REG1, BIT(4))", too?

According to table "Table 42.3 Registers for Module Standby Mode" only bit
0 of CPG_CLKON_3DGE maps to bit 4 of CPG_BUS_REG1_MSTOP. There are no hints
in the description of CPG_BUS_REG1_MSTOP neither in the description of
CPG_CLKON_3DGE registers that leads to having bit 4 of CPG_BUS_REG1_MSTOP
for AXI or ACE clocks as well.

I tried to play with MSTOP and clocks at runtime to see if there is any
relation b/w them but can't establish something.

I tried the following steps:
a/ remove the panfrost module (at this point all clocks are disabled and
   mstop != 0)
b/ devmem2 0x11010858 w 0x130013 # de-assert the GPU resets
c/ devmem2 0x11010b80 w 0x100000 # switch the module to normal
d/ enable different clocks and read one register as follows:

1/ gpu_axi enabled                   -> devmem 0x11840000 fail
2/ gpu_axi, gpu_ace enabled          -> devmem 0x11840000 fail
3/ gpu_axi, gpu_ace, gpu_clk enabled -> devmem 0x11840000 OK
4/ gpu_clk enabled                   -> devmem2 0x11840000 fail
5/ gpu_clk, gpu_axi enabled          -> devmem2 0x11840000 OK
6/ gpu_clk, gpu_ace enabled          -> devmem2 0x11840000 fail
7/ gpu_ace enabled                   -> devmem2 0x11840000 fail
8/ gpu_ace, gpu_clk enabled          -> devmem2 0x11840000 fail

From software point of view I can add it, it doesn't harm. With this,
please let me know if you prefer to have it.


> 
>>                 DEF_MOD("canfd",        R9A07G044_CANFD_PCLK, R9A07G044_CLK_P0,
>> -                                       0x594, 0, 0),
>> +                                       0x594, 0, MSTOP(BUS_MCPU2, BIT(9))),
>>                 DEF_MOD("gpio",         R9A07G044_GPIO_HCLK, R9A07G044_OSCCLK,
>>                                         0x598, 0, 0),
> 
> "MSTOP(BUS_PERI_CPU, BIT(6))"?

That was a good catch, thank you!

I played with it at runtime and it seems it should be here. It is not
present in table "Table 42.3 Registers for Module Standby Mode" either. It
is also valid for RZ/G3S so I'll add it there, too.

Thank you for your review,
Claudiu

> 
>>                 DEF_MOD("adc_adclk",    R9A07G044_ADC_ADCLK, R9A07G044_CLK_TSU,
>> -                                       0x5a8, 0, 0),
>> +                                       0x5a8, 0, MSTOP(BUS_MCPU2, BIT(14))),
> 
> 
> The rest LGTM.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>