Re: [PATCH v4 1/3] clk: samsung: Implement manual PLL control for ARM64 SoCs

[Sam Protsenko <semen.protsenko@linaro.org>]

On Thu, Feb 29, 2024 at 7:51 PM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> Some ARM64 Exynos chips are capable to control PLL clocks automatically.
> For those chips, whether the PLL is controlled automatically or manually
> is chosen in PLL_CON1 register with next bits:
>
>     [28]  ENABLE_AUTOMATIC_CLKGATING
>     [1]   MANUAL_PLL_CTRL
>     [0]   AUTO_PLL_CTRL
>
> The bl2 bootloader sets 0x10000001 value for some PLL_CON1 registers,
> which means any attempt to control those PLLs manually (e.g.
> disabling/enabling those PLLs or changing MUX parent clocks) would lead
> to PLL lock timeout with error message like this:
>
>     Could not lock PLL ...
>
> At the moment, all Samsung clock drivers implement manual clock control.
> So in order to make it possible to control PLLs, corresponding PLL_CON1
> registers should be set to 0x2 first.
>
> Some older ARM64 chips don't implement the automatic clock control
> though. It also might be desirable to configure some PLLs for manual
> control, while keeping the default configuration for the rest. So it'd
> convenient to choose this PLL mode for each CMU separately. Introduce
> .manual_plls field to CMU structure to choose the PLL control mode.
> Because it'll be initialized with "false" in all existing CMU
> structures by default, it won't affect any existing clock drivers,
> allowing for this feature to be enabled gradually when it's needed with
> no change for the rest of users. In case .manual_plls is set, set
> PLL_CON1 registers to manual control, akin to what's already done for
> gate clocks in exynos_arm64_init_clocks(). Of course, PLL_CON1 registers
> should be added to corresponding struct samsung_cmu_info::clk_regs array
> to make sure they get initialized.
>
> No functional change. This patch adds a feature, but doesn't enable it
> for any users.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---

Hi Krzysztof,

If it looks ok to you, can you please apply this series?

    [PATCH 1/3] clk: samsung: Implement manual PLL control for ARM64 SoCs
    [PATCH 2/3] clk: samsung: exynos850: Add CMU_CPUCL0 and CMU_CPUCL1
    [PATCH 3/3] arm64: dts: exynos: Add CPU clocks for Exynos850

That concludes my efforts on CPU clock enablement in Exynos850.

Thanks!

> Changes in v4:
>   - Turned register checking macros into static functions
>
> Changes in v3:
>   - none
>
> Changes in v2:
>   - none
>
>  drivers/clk/samsung/clk-exynos-arm64.c | 56 +++++++++++++++++++-------
>  drivers/clk/samsung/clk.h              |  4 ++
>  2 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-exynos-arm64.c b/drivers/clk/samsung/clk-exynos-arm64.c
> index 6fb7194df7ab..bf7de21f329e 100644
> --- a/drivers/clk/samsung/clk-exynos-arm64.c
> +++ b/drivers/clk/samsung/clk-exynos-arm64.c
> @@ -17,10 +17,17 @@
>
>  #include "clk-exynos-arm64.h"
>
> +/* PLL register bits */
> +#define PLL_CON1_MANUAL                BIT(1)
> +
>  /* Gate register bits */
>  #define GATE_MANUAL            BIT(20)
>  #define GATE_ENABLE_HWACG      BIT(28)
>
> +/* PLL_CONx_PLL register offsets range */
> +#define PLL_CON_OFF_START      0x100
> +#define PLL_CON_OFF_END                0x600
> +
>  /* Gate register offsets range */
>  #define GATE_OFF_START         0x2000
>  #define GATE_OFF_END           0x2fff
> @@ -38,17 +45,36 @@ struct exynos_arm64_cmu_data {
>         struct samsung_clk_provider *ctx;
>  };
>
> +/* Check if the register offset is a GATE register */
> +static bool is_gate_reg(unsigned long off)
> +{
> +       return off >= GATE_OFF_START && off <= GATE_OFF_END;
> +}
> +
> +/* Check if the register offset is a PLL_CONx register */
> +static bool is_pll_conx_reg(unsigned long off)
> +{
> +       return off >= PLL_CON_OFF_START && off <= PLL_CON_OFF_END;
> +}
> +
> +/* Check if the register offset is a PLL_CON1 register */
> +static bool is_pll_con1_reg(unsigned long off)
> +{
> +       return is_pll_conx_reg(off) && (off & 0xf) == 0x4 && !(off & 0x10);
> +}
> +
>  /**
>   * exynos_arm64_init_clocks - Set clocks initial configuration
> - * @np:                        CMU device tree node with "reg" property (CMU addr)
> - * @reg_offs:          Register offsets array for clocks to init
> - * @reg_offs_len:      Number of register offsets in reg_offs array
> + * @np:                CMU device tree node with "reg" property (CMU addr)
> + * @cmu:       CMU data
>   *
> - * Set manual control mode for all gate clocks.
> + * Set manual control mode for all gate and PLL clocks.
>   */
>  static void __init exynos_arm64_init_clocks(struct device_node *np,
> -               const unsigned long *reg_offs, size_t reg_offs_len)
> +                                           const struct samsung_cmu_info *cmu)
>  {
> +       const unsigned long *reg_offs = cmu->clk_regs;
> +       size_t reg_offs_len = cmu->nr_clk_regs;
>         void __iomem *reg_base;
>         size_t i;
>
> @@ -60,14 +86,14 @@ static void __init exynos_arm64_init_clocks(struct device_node *np,
>                 void __iomem *reg = reg_base + reg_offs[i];
>                 u32 val;
>
> -               /* Modify only gate clock registers */
> -               if (reg_offs[i] < GATE_OFF_START || reg_offs[i] > GATE_OFF_END)
> -                       continue;
> -
> -               val = readl(reg);
> -               val |= GATE_MANUAL;
> -               val &= ~GATE_ENABLE_HWACG;
> -               writel(val, reg);
> +               if (cmu->manual_plls && is_pll_con1_reg(reg_offs[i])) {
> +                       writel(PLL_CON1_MANUAL, reg);
> +               } else if (is_gate_reg(reg_offs[i])) {
> +                       val = readl(reg);
> +                       val |= GATE_MANUAL;
> +                       val &= ~GATE_ENABLE_HWACG;
> +                       writel(val, reg);
> +               }
>         }
>
>         iounmap(reg_base);
> @@ -177,7 +203,7 @@ void __init exynos_arm64_register_cmu(struct device *dev,
>                 pr_err("%s: could not enable bus clock %s; err = %d\n",
>                        __func__, cmu->clk_name, err);
>
> -       exynos_arm64_init_clocks(np, cmu->clk_regs, cmu->nr_clk_regs);
> +       exynos_arm64_init_clocks(np, cmu);
>         samsung_cmu_register_one(np, cmu);
>  }
>
> @@ -224,7 +250,7 @@ int __init exynos_arm64_register_cmu_pm(struct platform_device *pdev,
>                        __func__, cmu->clk_name, ret);
>
>         if (set_manual)
> -               exynos_arm64_init_clocks(np, cmu->clk_regs, cmu->nr_clk_regs);
> +               exynos_arm64_init_clocks(np, cmu);
>
>         reg_base = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(reg_base))
> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
> index a763309e6f12..a70bd7cce39f 100644
> --- a/drivers/clk/samsung/clk.h
> +++ b/drivers/clk/samsung/clk.h
> @@ -330,6 +330,7 @@ struct samsung_clock_reg_cache {
>   * @suspend_regs: list of clock registers to set before suspend
>   * @nr_suspend_regs: count of clock registers in @suspend_regs
>   * @clk_name: name of the parent clock needed for CMU register access
> + * @manual_plls: Enable manual control for PLL clocks
>   */
>  struct samsung_cmu_info {
>         const struct samsung_pll_clock *pll_clks;
> @@ -354,6 +355,9 @@ struct samsung_cmu_info {
>         const struct samsung_clk_reg_dump *suspend_regs;
>         unsigned int nr_suspend_regs;
>         const char *clk_name;
> +
> +       /* ARM64 Exynos CMUs */
> +       bool manual_plls;
>  };
>
>  struct samsung_clk_provider *samsung_clk_init(struct device *dev,
> --
> 2.39.2
>