Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <e8d3f59a-fa1d-9993-6e84-99d976eb05fa@tuxon.dev>
Date:   Mon, 18 Sep 2023 09:20:53 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 22/37] clk: renesas: add minimal boot support for RZ/G3S
 SoC
Content-Language: en-US
To:     Geert Uytterhoeven <geert@linux-m68k.org>
Cc:     geert+renesas@glider.be, mturquette@baylibre.com, sboyd@kernel.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        conor+dt@kernel.org, ulf.hansson@linaro.org,
        linus.walleij@linaro.org, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, magnus.damm@gmail.com,
        catalin.marinas@arm.com, will@kernel.org,
        prabhakar.mahadev-lad.rj@bp.renesas.com,
        biju.das.jz@bp.renesas.com, quic_bjorande@quicinc.com,
        arnd@arndb.de, konrad.dybcio@linaro.org, neil.armstrong@linaro.org,
        nfraprado@collabora.com, rafal@milecki.pl,
        wsa+renesas@sang-engineering.com,
        linux-renesas-soc@vger.kernel.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mmc@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
References: <20230912045157.177966-1-claudiu.beznea.uj@bp.renesas.com>
 <20230912045157.177966-23-claudiu.beznea.uj@bp.renesas.com>
 <CAMuHMdUCpbPA3cDjNAq1irvr6z4Jux-5-tVDBuyr6nx_qOQGpg@mail.gmail.com>
From:   claudiu beznea <claudiu.beznea@tuxon.dev>
In-Reply-To: <CAMuHMdUCpbPA3cDjNAq1irvr6z4Jux-5-tVDBuyr6nx_qOQGpg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Geert,

On 15.09.2023 15:52, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Add minimal clock and reset support for RZ/G3S SoC to be able to boot
>> Linux from SD Card/eMMC. This includes necessary core clocks for booting
>> and GIC, SCIF, GPIO, SD0 mod clocks and resets.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- /dev/null
>> +++ b/drivers/clk/renesas/r9a08g045-cpg.c
>> @@ -0,0 +1,217 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * RZ/G3S CPG driver
>> + *
>> + * Copyright (C) 2023 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/device.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +
>> +#include <dt-bindings/clock/r9a08g045-cpg.h>
>> +
>> +#include "rzg2l-cpg.h"
>> +
>> +/* RZ/G3S Specific registers. */
>> +#define G3S_CPG_PL2_DDIV               (0x204)
>> +#define G3S_CPG_SDHI_DDIV              (0x218)
>> +#define G3S_CPG_PLL_DSEL               (0x240)
>> +#define G3S_CPG_SDHI_DSEL              (0x244)
>> +#define G3S_CLKSELSTATUS               (0x284)
>> +
>> +/* RZ/G3S Specific division configuration.  */
>> +#define G3S_DIVPL2B            DDIV_PACK(G3S_CPG_PL2_DDIV, 4, 3)
>> +#define G3S_DIV_SDHI0          DDIV_PACK(G3S_CPG_SDHI_DDIV, 0, 1)
>> +
>> +/* RZ/G3S Clock status configuration. */
>> +#define G3S_DIVPL1A_STS                DDIV_PACK(CPG_CLKSTATUS, 0, 1)
>> +#define G3S_DIVPL2B_STS                DDIV_PACK(CPG_CLKSTATUS, 5, 1)
>> +#define G3S_DIVPL3A_STS                DDIV_PACK(CPG_CLKSTATUS, 8, 1)
>> +#define G3S_DIVPL3B_STS                DDIV_PACK(CPG_CLKSTATUS, 9, 1)
>> +#define G3S_DIVPL3C_STS                DDIV_PACK(CPG_CLKSTATUS, 10, 1)
>> +#define G3S_DIV_SDHI0_STS      DDIV_PACK(CPG_CLKSTATUS, 24, 1)
> 
> The register at offset 0x280 is called CPG_CLKDIVSTATUS, so
> you probably want to add and use a G3S-specific definition.

I just used the already definition as there is no conflict at the moment,
it points to the same offset and is almost identical in name. With this
would you still prefer to have it separately ?

> 
>> +#define G3S_SEL_PLL4_STS       SEL_PLL_PACK(G3S_CLKSELSTATUS, 6, 1)
>> +#define G3S_SEL_SDHI0_STS      SEL_PLL_PACK(G3S_CLKSELSTATUS, 16, 1)
>> +
>> +/* RZ/G3S Specific clocks select. */
>> +#define G3S_SEL_PLL4           SEL_PLL_PACK(G3S_CPG_PLL_DSEL, 6, 1)
>> +#define G3S_SEL_SDHI0          SEL_PLL_PACK(G3S_CPG_SDHI_DSEL, 0, 2)
>> +
>> +/* PLL 1/4/6 configuration registers macro. */
>> +#define G3S_PLL146_CONF(clk1, clk2)    ((clk1) << 22 | (clk2) << 12)
>> +
>> +#define DEF_G3S_MUX(_name, _id, _conf, _parent_names, _mux_flags, _clk_flags) \
>> +       DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = (_conf), \
>> +                .parent_names = (_parent_names), \
>> +                .num_parents = ARRAY_SIZE((_parent_names)), \
>> +                .mux_flags = CLK_MUX_HIWORD_MASK | (_mux_flags), \
>> +                .flag = (_clk_flags))
>> +
>> +enum clk_ids {
>> +       /* Core Clock Outputs exported to DT */
>> +       LAST_DT_CORE_CLK = R9A08G045_SWD,
>> +
>> +       /* External Input Clocks */
>> +       CLK_EXTAL,
>> +
>> +       /* Internal Core Clocks */
>> +       CLK_OSC_DIV1000,
>> +       CLK_PLL1,
>> +       CLK_PLL2,
>> +       CLK_PLL2_DIV2,
>> +       CLK_PLL2_DIV2_8,
>> +       CLK_PLL2_DIV6,
>> +       CLK_PLL3,
>> +       CLK_PLL3_DIV2,
>> +       CLK_PLL3_DIV2_2,
> 
> Do you need CLK_PLL3_DIV2_2?
> When adding support for R9A07G043_CLK_AT later, you can define it
> as CLK_PLL3_DIV2 / 2.

That's true. I kept it here as I saw it as a core clock. I can remove it.

> 
>> +       CLK_PLL3_DIV2_4,
>> +       CLK_PLL3_DIV2_8,
>> +       CLK_PLL3_DIV6,
>> +       CLK_PLL4,
>> +       CLK_PLL6,
>> +       CLK_PLL6_DIV2,
>> +       CLK_SEL_SDHI0,
>> +       CLK_SEL_PLL4,
>> +       CLK_P1_DIV2,
>> +       CLK_P3_DIV2,
> 
> Do you need CLK_P1_DIV2 and CLK_P3_DIV2?
> I don't see them in Figure 7.3 ("Clock System Diagram (2)").
> 
>> +       CLK_SD0_DIV,
> 
> CLK_SD0_DIV is unused.

Ok, I'll remove it.

> 
>> +       CLK_SD0_DIV4,
>> +       CLK_S0_DIV2,
> 
> CLK_S0_DIV2 is unused.

ok.

> 
>> +
>> +       /* Module Clocks */
>> +       MOD_CLK_BASE,
>> +};
>> +
>> +/* Divider tables */
>> +static const struct clk_div_table dtable_1_2[] = {
>> +       {0, 1},
> 
> "{ 0, 1 }," etc...

ok.

> 
>> +       {1, 2},
>> +       {0, 0},
>> +};
>> +
>> +static const struct clk_div_table dtable_1_8[] = {
>> +       {0, 1},
>> +       {1, 2},
>> +       {2, 4},
>> +       {3, 8},
>> +       {0, 0},
>> +};
>> +
>> +static const struct clk_div_table dtable_1_32[] = {
>> +       {0, 1},
>> +       {1, 2},
>> +       {2, 4},
>> +       {3, 8},
>> +       {4, 32},
>> +       {0, 0},
>> +};
>> +
>> +/* Mux clock names tables. */
>> +static const char * const sel_sdhi[] = { ".pll2_div2", ".pll6", ".pll2_div6" };
>> +static const char * const sel_pll4[] = { ".osc_div1000", ".pll4" };
>> +
>> +/* Mux clock indexes tables. */
> 
> indices
> 
>> +static const u32 mtable_sd[] = { 0, 2, 3 };
>> +static const u32 mtable_pll4[] = { 0, 1 };
>> +
>> +static const struct cpg_core_clk r9a08g045_core_clks[] __initconst = {
>> +       /* External Clock Inputs */
>> +       DEF_INPUT("extal", CLK_EXTAL),
>> +
>> +       /* Internal Core Clocks */
>> +       DEF_FIXED(".osc", R9A08G045_OSCCLK, CLK_EXTAL, 1, 1),
> 
> "OSC", as this is not an internal core clock.

ok, I wasn't aware of this convention.

> 
>> +       DEF_FIXED(".osc2", R9A08G045_OSCCLK2, CLK_EXTAL, 1, 3),
> 
> "OSC2"

ok.

> 
>> +       DEF_FIXED(".osc_div1000", CLK_OSC_DIV1000, CLK_EXTAL, 1, 1000),
>> +       DEF_G3S_SAMPLL(".pll1", CLK_PLL1, CLK_EXTAL, G3S_PLL146_CONF(0x4, 0x8)),
>> +       DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 200, 3),
>> +       DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 200, 3),
>> +       DEF_FIXED(".pll4", CLK_PLL4, CLK_EXTAL, 100, 3),
>> +       DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6),
>> +       DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
>> +       DEF_FIXED(".pll2_div2_8", CLK_PLL2_DIV2_8, CLK_PLL2_DIV2, 1, 8),
>> +       DEF_FIXED(".pll2_div6", CLK_PLL2_DIV6, CLK_PLL2, 1, 6),
>> +       DEF_FIXED(".pll3_div2", CLK_PLL3_DIV2, CLK_PLL3, 1, 2),
>> +       DEF_FIXED(".pll3_div2_2", CLK_PLL3_DIV2_2, CLK_PLL3_DIV2, 1, 2),
>> +       DEF_FIXED(".pll3_div2_4", CLK_PLL3_DIV2_4, CLK_PLL3_DIV2, 1, 4),
>> +       DEF_FIXED(".pll3_div2_8", CLK_PLL3_DIV2_8, CLK_PLL3_DIV2, 1, 8),
>> +       DEF_FIXED(".pll3_div6", CLK_PLL3_DIV6, CLK_PLL3, 1, 6),
>> +       DEF_FIXED(".pll6_div2", CLK_PLL6_DIV2, CLK_PLL6, 1, 2),
>> +       DEF_SD_MUX(".sel_sd0", CLK_SEL_SDHI0, G3S_SEL_SDHI0, G3S_SEL_SDHI0_STS, sel_sdhi,
>> +                  mtable_sd, 0, NULL),
>> +       DEF_SD_MUX(".sel_pll4", CLK_SEL_PLL4, G3S_SEL_PLL4, G3S_SEL_PLL4_STS, sel_pll4,
>> +                  mtable_pll4, CLK_SET_PARENT_GATE, NULL),
>> +
>> +       /* Core output clk */
>> +       DEF_G3S_DIV("I", R9A08G045_CLK_I, CLK_PLL1, DIVPL1A, G3S_DIVPL1A_STS, dtable_1_8,
>> +                   0, 0, NULL),
>> +       DEF_G3S_DIV("P0", R9A08G045_CLK_P0, CLK_PLL2_DIV2_8, G3S_DIVPL2B, G3S_DIVPL2B_STS,
>> +                   dtable_1_32, 0, 0, NULL),
>> +       DEF_G3S_DIV("SD0", R9A08G045_CLK_SD0, CLK_SEL_SDHI0, G3S_DIV_SDHI0, G3S_DIV_SDHI0_STS,
>> +                   dtable_1_2, 800000000UL, CLK_SET_RATE_PARENT, DIV_NOTIF),
>> +       DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A08G045_CLK_SD0, 1, 4),
> 
> ".sd0_div4", as this is not a public core clock.

ok.

> 
>> +       DEF_FIXED("M0", R9A08G045_CLK_M0, CLK_PLL3_DIV2_4, 1, 1),
>> +       DEF_G3S_DIV("P1", R9A08G045_CLK_P1, CLK_PLL3_DIV2_4, DIVPL3A, G3S_DIVPL3A_STS,
>> +                   dtable_1_32, 0, 0, NULL),
>> +       DEF_FIXED("P1_DIV2", CLK_P1_DIV2, R9A08G045_CLK_P1, 1, 2),
>> +       DEF_G3S_DIV("P2", R9A08G045_CLK_P2, CLK_PLL3_DIV2_8, DIVPL3B, G3S_DIVPL3B_STS,
>> +                   dtable_1_32, 0, 0, NULL),
>> +       DEF_G3S_DIV("P3", R9A08G045_CLK_P3, CLK_PLL3_DIV2_4, DIVPL3C, G3S_DIVPL3C_STS,
>> +                   dtable_1_32, 0, 0, NULL),
>> +       DEF_FIXED("P3_DIV2", CLK_P3_DIV2, R9A08G045_CLK_P3, 1, 2),
>> +       DEF_FIXED("S0", R9A08G045_CLK_S0, CLK_SEL_PLL4, 1, 2),
>> +       DEF_FIXED("S0_DIV2", CLK_S0_DIV2, R9A08G045_CLK_S0, 1, 2),
>> +};
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>