Received-SPF: pass (google.com: domain of linux-kernel+bounces-25836-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
In-Reply-To: <20240110-clk-th1520-v1-3-8b0682567984@tenstorrent.com>
References: <20240110-clk-th1520-v1-0-8b0682567984@tenstorrent.com> <20240110-clk-th1520-v1-3-8b0682567984@tenstorrent.com>
Precedence: bulk
Mime-Version: 1.0
Date: Mon, 15 Jan 2024 04:52:08 -0500
Message-ID: <CAJM55Z8_nTu__iSvSTbo719SPvtGcd6jKrK=UJ6YwLj1jCk2_w@mail.gmail.com>
Subject: Re: [PATCH RFC 3/3] clk: thead: add support for T-HEAD TH1520 AP clocks
To: Drew Fustini <dfustini@tenstorrent.com>, Jisheng Zhang <jszhang@kernel.org>, 
	Guo Ren <guoren@kernel.org>, Fu Wei <wefu@redhat.com>, 
	Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, 
	Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Yangtao Li <frank.li@vivo.com>
Cc: linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, 
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org, 
	Emil Renner Berthing <emil.renner.berthing@canonical.com>, Han Gao <gaohan@iscas.ac.cn>, 
	Xi Ruoyao <xry111@xry111.site>, Robert Nelson <robertcnelson@beagleboard.org>, 
	Jason Kridner <jkridner@beagleboard.org>, Drew Fustini <drew@tenstorrent.org>
Content-Type: text/plain; charset="UTF-8"

Drew Fustini wrote:
> From: Jisheng Zhang <jszhang@kernel.org>
>
> Add support for the AP sub system clock controller in the T-HEAD TH1520.
> This include CPU, DPU, GMAC and TEE PLLs.
>
> Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
> Co-developed-by: Yangtao Li <frank.li@vivo.com>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> [rebased on linux-next-20240110]
> [fixed checkpatch warnings]
> [corrected npu_clk enable bit and c910_i0_clk reg]
> [revised commit description]
> Signed-off-by: Drew Fustini <drew@tenstorrent.org>
> ---
>  MAINTAINERS                       |    1 +
>  drivers/clk/Kconfig               |    1 +
>  drivers/clk/Makefile              |    1 +
>  drivers/clk/thead/Kconfig         |   12 +
>  drivers/clk/thead/Makefile        |    2 +
>  drivers/clk/thead/clk-th1520-ap.c | 1018 +++++++++++++++++++++++++++++++++++++
>  6 files changed, 1035 insertions(+)
>
..
> --- /dev/null
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -0,0 +1,1018 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> + *  Authors: Yangtao Li <frank.li@vivo.com>
> + */
> +
> +#include <dt-bindings/clock/thead,th1520-clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ccu_internal {
> +	u8	shift;
> +	u8	width;
> +};
> +
> +struct ccu_div_internal {
> +	u8	shift;
> +	u8	width;
> +	u32	flags;
> +};
> +
> +struct ccu_common {
> +	struct regmap	*map;
> +	u16		reg;
> +	struct clk_hw	hw;
> +};
> +
> +struct ccu_mux {
> +	struct ccu_internal	mux;
> +	struct ccu_common	common;
> +};
> +
> +struct ccu_gate {
> +	u32			enable;
> +	struct ccu_common	common;
> +};
> +
> +struct ccu_div {
> +	u32			enable;
> +	struct ccu_div_internal	div;
> +	struct ccu_internal	mux;
> +	struct ccu_common	common;
> +};
> +
> +/*
> + * struct ccu_mdiv - Definition of an M-D-I-V clock
> + *
> + * Clocks based on the formula (parent * M) / (D * I * V)
> + */
> +struct ccu_mdiv {
> +	struct ccu_internal	m;
> +	struct ccu_internal	d;
> +	struct ccu_internal	i;
> +	struct ccu_internal	v;
> +	struct ccu_common	common;
> +};
> [...]
> +static unsigned long ccu_mdiv_recalc_rate(struct clk_hw *hw,
> +					  unsigned long parent_rate)
> +{
> +	struct ccu_mdiv *mdiv = hw_to_ccu_mdiv(hw);
> +	unsigned long div, rate = parent_rate;
> +	unsigned int m, d, i, v, val;
> +
> +	regmap_read(mdiv->common.map, mdiv->common.reg, &val);
> +
> +	m = val >> mdiv->m.shift;
> +	m &= GENMASK(mdiv->m.width - 1, 0);
> +
> +	d = val >> mdiv->d.shift;
> +	d &= GENMASK(mdiv->d.width - 1, 0);
> +
> +	i = val >> mdiv->i.shift;
> +	i &= GENMASK(mdiv->i.width - 1, 0);
> +
> +	v = val >> mdiv->v.shift;
> +	v &= GENMASK(mdiv->v.width - 1, 0);
> +
> +	rate = parent_rate * m;
> +	div = d * i * v;
> +	do_div(rate, div);
> +
> +	return rate;
> +}

Hi Drew,

I don't think this is right. There is an input predivider that's not handled
here, and then the PLL multiplies the input frequency and outputs "Foutvco".
Then this is followed by a post divider to produce "Foutpostdiv".
Some clocks derive directly from the "Foutvco" so this should really be
modelled as two different clocks. Also what's called D and I are the
postdivider but V is an optional fractional divider.
All in all I think it should be something like this:

#define TH1520_PLL_CFG0         0x0
#define TH1520_PLL_POSTDIV2     GENMASK(26, 24)
#define TH1520_PLL_POSTDIV1     GENMASK(22, 20)
#define TH1520_PLL_FBDIV        GENMASK(19, 8)
#define TH1520_PLL_REFDIV       GENMASK(5, 0)
#define TH1520_PLL_CFG1         0x4
#define TH1520_PLL_BYPASS       BIT(30)
#define TH1520_PLL_RST          BIT(29)
#define TH1520_PLL_POSTDIVPD    BIT(28)
#define TH1520_PLL_4PHASEPD     BIT(27)
#define TH1520_PLL_DACPD        BIT(25)
#define TH1520_PLL_DSMPD        BIT(24)
#define TH1520_PLL_FRAC         GENMASK(23, 0)
#define TH1520_PLL_FRAC_BITS    24
#define TH1520_PLL_CFG2         0x8
#define TH1520_PLL_CFG3         0xc

static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
                                                unsigned long parent_rate)
{
        struct th1520_pll *pll = th1520_pll_from_vco(hw);
        void __iomem *cfg0reg = pll->base + TH1520_PLL_CFG0;
        void __iomem *cfg1reg = pll->base + TH1520_PLL_CFG1;
        unsigned long rate, mul;
        u32 cfg0, cfg1, div;

        scoped_guard(spinlock_irqsave, pll->lock) {
                cfg0 = readl_relaxed(cfg0reg);
                cfg1 = readl_relaxed(cfg1reg);
        }

        mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
        div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
        if (!(cfg1 & TH1520_PLL_DSMPD)) {
                mul <<= TH1520_PLL_FRAC_BITS;
                mul += FIELD_GET(TH1520_PLL_FRAC, cfg1);
                div <<= TH1520_PLL_FRAC_BITS;
        }
        rate = parent_rate * mul;
        do_div(rate, div);
        return rate;
}

static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
                                                    unsigned long parent_rate)
{
        struct th1520_pll *pll = th1520_pll_from_postdiv(hw);
        void __iomem *cfg0reg = pll->base + TH1520_PLL_CFG0;
        void __iomem *cfg1reg = pll->base + TH1520_PLL_CFG1;
        unsigned long rate = parent_rate;
        u32 cfg0, cfg1;

        scoped_guard(spinlock_irqsave, pll->lock) {
                cfg0 = readl_relaxed(cfg0reg);
                cfg1 = readl_relaxed(cfg1reg);
        }

        if (cfg1 & TH1520_PLL_BYPASS)
                return rate;

        do_div(rate,
               FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
               FIELD_GET(TH1520_PLL_POSTDIV2, cfg0));
        return rate;
}

(Here D = POSTDIV1, I = POSTDIV2 and V = FRAC)

However, have a look at Chen Wang's series at

https://lore.kernel.org/linux-riscv/cdb7aed766aa6411e61ec25a6f1cb22a1aef4a21.1704694903.git.unicorn_wang@outlook.com/

The PLL implementation there is very similar to the TH1520. At first I thought
it was exactly the same, but on closer inspection it seems like the bitfields
are arranged a little different unfortunately.

The rest of the clocks in this driver seem to be generic gate and mux
implementations that should probably be replaced by the versions in Linux
already. Eg. devm_clk_hw_register_gate*() and devm_clk_hw_register_mux*().

Lastly this only implements the clocks in the AP_SUBSYS memory range, but there
are also AON_SUBSYS, DDR_SUBSYS, MISC_SUBSYS, VI_SUBSYS, VO_SUBSYS, VP_SUBSYS,
DSP_SUBSYS and AUDIO_SUBSYS. Upstreaming one of them first in fine, but make
sure to consider how the others should be added.

/Emil