[PATCH v11 01/10] clk: Add RISC-V Canaan Kendryte K210 clock driver
Damien Le Moal
Damien.LeMoal at wdc.com
Wed Jan 13 04:14:56 EST 2021
On 2021/01/13 17:25, Stephen Boyd wrote:
> Quoting Damien Le Moal (2021-01-11 17:02:38)
>> Add a clock provider driver for the Canaan Kendryte K210 RISC-V SoC.
>> This new driver with the compatible string "canaan,k210-clk" implements
>> support for the full clock structure of the K210 SoC. Since it is
>> required for the correct operation of the SoC, this driver is
>> selected by default for compilation when the SOC_CANAAN option is
>> selected.
>>
>> With this change, the k210-sysctl driver is turned into a simple
>> platform driver which enables its power bus clock and triggers
>> populating its child nodes. This driver is also automatically selected
>> for compilation with the selection of SOC_CANAAN.
>
> That's stated twice.
>
>> The sysctl driver
>> retains the SOC early initialization code, but the implementation now
>> relies on the new function k210_clk_early_init() provided by the new
>> clk-k210 driver.
>>
>> The clock structure implemented and many of the coding ideas for the
>> driver come from the work by Sean Anderson on the K210 support for the
>> U-Boot project.
>>
>> Cc: Stephen Boyd <sboyd at kernel.org>
>> Cc: Michael Turquette <mturquette at baylibre.com>
>> Cc: linux-clk at vger.kernel.org
>> Signed-off-by: Damien Le Moal <damien.lemoal at wdc.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/clk/Kconfig | 8 +
>> drivers/clk/Makefile | 1 +
>> drivers/clk/clk-k210.c | 1005 ++++++++++++++++++++++++++++++
>> drivers/soc/canaan/Kconfig | 18 +-
>> drivers/soc/canaan/Makefile | 2 +-
>> drivers/soc/canaan/k210-sysctl.c | 205 ++----
>> include/soc/canaan/k210-sysctl.h | 2 +
>> 8 files changed, 1064 insertions(+), 178 deletions(-)
>> create mode 100644 drivers/clk/clk-k210.c
>
> I'm not sure how/if this can be merged through clk tree given that it
> deletes code from drivers/soc/
>
> What's the merge strategy?
There is a patch preceding this one that renames drivers/soc/kendryte to
drivers/soc/canaan, so I don't think the patch will apply to your clock tree. Is
it OK if Palmer takes this one in the riscv tree ?
[...]
>> +/**
>> + * struct k210_clk - Driver data
>> + * @regs: system controller registers start address
>> + * @clk_lock: clock setting spinlock
>> + * @plls: SoC PLLs descriptors
>> + * @aclk: ACLK clock
>> + * @clks: All other clocks
>> + * @parents: array of pointers to clocks used as parents for muxed clocks.
>> + * @clk_data: clock specifier translation for all clocks
>> + */
>> +struct k210_clk {
>> + void __iomem *regs;
>> + spinlock_t clk_lock;
>> + struct k210_pll plls[K210_PLL_NUM];
>> + struct clk_hw aclk;
>> + struct clk_hw clks[K210_NUM_CLKS];
>> + const struct clk_hw *parents[K210_PARENT_NUM_CLKS];
>> + struct clk_hw_onecell_data *clk_data;
>> +};
>> +
>> +static struct k210_clk *kcl;
>
> Please remove this and get to it from the clk hw structure.
OK.
>
>> +
>> +/*
>> + * Set ACLK parent selector: 0 for IN0, 1 for PLL0.
>> + */
>> +static void k210_aclk_set_selector(u8 sel)
>> +{
>> + u32 reg = readl(kcl->regs + K210_SYSCTL_SEL0);
>> +
>> + if (sel)
>> + reg |= K210_ACLK_SEL;
>> + else
>> + reg &= K210_ACLK_SEL;
>> + writel(reg, kcl->regs + K210_SYSCTL_SEL0);
>> +}
>> +
>> +static void k210_init_pll(struct k210_pll *pll, enum k210_pll_id id,
>> + void __iomem *base)
>> +{
>> + pll->id = id;
>> + pll->lock = base + K210_SYSCTL_PLL_LOCK;
>> + pll->reg = base + k210_plls_cfg[id].reg;
>> + pll->lock_shift = k210_plls_cfg[id].lock_shift;
>> + pll->lock_width = k210_plls_cfg[id].lock_width;
>> +}
>> +
>> +static void k210_pll_wait_for_lock(struct k210_pll *pll)
>> +{
>> + u32 reg, mask = GENMASK(pll->lock_shift + pll->lock_width - 1,
>> + pll->lock_shift);
>> +
>> + while (true) {
>
> Any timeout here? Otherwise this is an infinite loop if the hardware
> misbehaves.
No timeout from the hardware for sure. I can add one in the loop here. However,
since this is used super early in the code when there is no possibility yet of
getting real time, the timeout needs to be a limit on the number of iteration
for the loop. That limit will be super arbitrary at best... But definitely, that
will avoid an infinite loop.
>
>> + reg = readl(pll->lock);
>> + if ((reg & mask) == mask)
>> + break;
>> +
>> + reg |= BIT(pll->lock_shift + K210_PLL_CLEAR_SLIP);
>> + writel(reg, pll->lock);
>> + }
>> +}
>> +
>> +static bool k210_pll_hw_is_enabled(struct k210_pll *pll)
>> +{
>> + u32 reg = readl(pll->reg);
>> + u32 mask = K210_PLL_PWRD | K210_PLL_EN;
>> +
>> + if (reg & K210_PLL_RESET)
>> + return false;
>> +
>> + return (reg & mask) == mask;
>> +}
>> +
>> +static void k210_pll_enable_hw(struct k210_pll *pll)
>> +{
>> + struct k210_pll_cfg *pll_cfg = &k210_plls_cfg[pll->id];
>> + unsigned long flags;
>> + u32 reg;
>> +
>> + spin_lock_irqsave(&kcl->clk_lock, flags);
>> +
>> + if (k210_pll_hw_is_enabled(pll))
>> + goto unlock;
>> +
>> + /*
>> + * For PLL0, we need to re-parent ACLK to IN0 to keep the CPU cores and
>> + * SRAM running.
>> + */
>> + if (pll->id == K210_PLL0)
>> + k210_aclk_set_selector(0);
>> +
>> + /* Set PLL factors */
>> + reg = readl(pll->reg);
>> + reg &= ~GENMASK(19, 0);
>> + reg |= FIELD_PREP(K210_PLL_CLKR, pll_cfg->r);
>> + reg |= FIELD_PREP(K210_PLL_CLKF, pll_cfg->f);
>> + reg |= FIELD_PREP(K210_PLL_CLKOD, pll_cfg->od);
>> + reg |= FIELD_PREP(K210_PLL_BWADJ, pll_cfg->bwadj);
>> + reg |= K210_PLL_PWRD;
>> + writel(reg, pll->reg);
>> +
>> + /*
>> + * Reset the PLL: ensure reset is low before asserting it.
>> + * The magic NOPs come from the Kendryte reference SDK.
>> + */
>> + reg &= ~K210_PLL_RESET;
>> + writel(reg, pll->reg);
>> + reg |= K210_PLL_RESET;
>> + writel(reg, pll->reg);
>> + nop();
>> + nop();
>> + reg &= ~K210_PLL_RESET;
>> + writel(reg, pll->reg);
>> +
>> + k210_pll_wait_for_lock(pll);
>> +
>> + reg &= ~K210_PLL_BYPASS;
>> + reg |= K210_PLL_EN;
>> + writel(reg, pll->reg);
>> +
>> + if (pll->id == K210_PLL0)
>> + k210_aclk_set_selector(1);
>> +
>> +unlock:
>> + spin_unlock_irqrestore(&kcl->clk_lock, flags);
>> +}
>> +
>> +static void k210_pll_disable_hw(struct k210_pll *pll)
>> +{
>> + unsigned long flags;
>> + u32 reg;
>> +
>> + /*
>> + * Bypassing before powering off is important so child clocks do not
>> + * stop working. This is especially important for pll0, the indirect
>> + * parent of the cpu clock.
>> + */
>> + spin_lock_irqsave(&kcl->clk_lock, flags);
>> + reg = readl(pll->reg);
>> + reg |= K210_PLL_BYPASS;
>> + writel(reg, pll->reg);
>> +
>> + reg &= ~K210_PLL_PWRD;
>> + reg &= ~K210_PLL_EN;
>> + writel(reg, pll->reg);
>> + spin_unlock_irqrestore(&kcl->clk_lock, flags);
>> +}
>> +
>> +static int k210_pll_enable(struct clk_hw *hw)
>> +{
>> + k210_pll_enable_hw(to_k210_pll(hw));
>> +
>> + return 0;
>> +}
>> +
>> +static void k210_pll_disable(struct clk_hw *hw)
>> +{
>> + k210_pll_disable_hw(to_k210_pll(hw));
>
> Why the extra wrapper? Inline k210_pll_disable_hw() here? Same for
> enable.
k210_pll_disable_hw() is used in k210_clk_early_init() when there is not yet any
clk_hw struct available (only the reg iomap). Hence this lower level function to
avoid repeating the code.
>
>> +}
>> +
>> +static int k210_pll_is_enabled(struct clk_hw *hw)
>> +{
>> + return k210_pll_hw_is_enabled(to_k210_pll(hw));
>
> Same comment.
k210_pll_hw_is_enabled() is also used in k210_pll_enable_hw(). I can remove
k210_pll_enable_hw() and code it as k210_pll_enable(). I wanted to be symmetric
with the enable/disable/is_enabled ops though, so I added k210_pll_enable_hw()
despite it not being really needed. I can remove it.
[...]
>> +static int k210_clk_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> + struct k210_clk_cfg *kclk = to_k210_clk_cfg(hw);
>> + unsigned long flags;
>> + u32 reg;
>> +
>> + if (!kclk->mux_reg) {
>> + if (WARN_ON(index != 0))
>
> Why would this ever happen? Seems like we don't want to allow different
> parents? But then why allow set_parent to be called?
Arg. Yes. If mux_reg is not set, then this is not a muxed clock. So we don't
need set_parent(). Will fix that.
[...]
>> +static inline struct clk_hw *k210_register_critical_clk(int id,
>> + enum k210_parent_idx parent_idx)
>> +{
>> + return k210_register_clk(id, parent_idx, 1, CLK_IS_CRITICAL);
>
> Why critical? Please add a comment. Or that is lower down? Maybe move
> that comment up to this inline function instead.
There is a comment in k210_clk_init() where all calls to
k210_register_critical_clk() are grouped together. It says:
/*
* Critical clocks: there are no consumers of the SRAM clocks,
* including the AI clock for the third SRAM bank. The CPU clock
* is only referenced by the uarths serial device and so would be
* disabled if the serial console is disabled. Mark all these clocks
* as critical so that they are never disabled by the core clock
* management.
*/
I could remove the SRAM and AI clocks controlling the 3 memory banks, however,
they are handy to have around since the CPU clock can be changed on this board
but doing so requires that the SRAM and AI clocks also be reconfigured to have a
frequency that is a divisor of the CPU clock. Otherwise, the board hangs...
The CPU clock frequency adjustment is not in yet and is for later.
>
>> +}
>> +
>> +static inline struct clk_hw *k210_register_child(int id,
>> + enum k210_parent_idx parent_idx)
>> +{
>> + return k210_register_clk(id, parent_idx, 1, 0);
>> +}
>> +
>> +static inline struct clk_hw *k210_register_in0_child(int id)
>> +{
>> + return k210_register_child(id, K210_PARENT_IN0);
>> +}
>> +
>> +static inline struct clk_hw *k210_register_pll0_child(int id)
>> +{
>> + return k210_register_child(id, K210_PARENT_PLL0);
>> +}
>> +
>> +static inline struct clk_hw *k210_register_pll2_child(int id)
>> +{
>> + return k210_register_child(id, K210_PARENT_PLL2);
>> +}
>> +
>> +static inline struct clk_hw *k210_register_aclk_child(int id)
>> +{
>> + return k210_register_child(id, K210_PARENT_ACLK);
>> +}
>> +
>> +static inline struct clk_hw *k210_register_apb0_child(int id)
>> +{
>> + return k210_register_child(id, K210_PARENT_APB0);
>> +}
>> +
>> +static inline struct clk_hw *k210_register_apb1_child(int id)
>> +{
>> + return k210_register_child(id, K210_PARENT_APB1);
>> +}
>> +
>> +static void __init k210_clk_init(struct device_node *np)
>> +{
>> + struct device_node *sysctl_np;
>> + struct clk_hw **hws;
>> + struct clk *in0;
>> + int i, ret;
>> +
>> + kcl = kzalloc(sizeof(*kcl), GFP_KERNEL);
>> + if (!kcl)
>> + return;
>> +
>> + kcl->clk_data = kzalloc(struct_size(kcl->clk_data, hws, K210_NUM_CLKS),
>> + GFP_KERNEL);
>> + if (!kcl->clk_data)
>> + return;
>> +
>> + sysctl_np = of_get_parent(np);
>> + kcl->regs = of_iomap(sysctl_np, 0);
>> + of_node_put(sysctl_np);
>> + if (!kcl->regs) {
>> + pr_err("%pOFP: failed to map registers\n", np);
>> + return;
>> + }
>> +
>> + spin_lock_init(&kcl->clk_lock);
>> + kcl->clk_data->num = K210_NUM_CLKS;
>> + hws = kcl->clk_data->hws;
>> + for (i = 0; i < K210_NUM_CLKS; i++)
>> + hws[i] = ERR_PTR(-EPROBE_DEFER);
>> +
>> + /* Get the system base fixed-rate 26MHz oscillator clock */
>> + in0 = of_clk_get(np, 0);
>> + if (IS_ERR(in0)) {
>> + pr_err("%pOFP: failed to get base fixed-rate oscillator\n", np);
>> + return;
>> + }
>> + kcl->parents[K210_PARENT_IN0] = __clk_get_hw(in0);
>
> This shouldn't be necessary. Specify the DT node index in the
> parent_data structure instead.
I am not sure I understand what you mean here. There are multiple possible
parents. And only IN0 has a DT node. The parents array is used for all muxed
clocks since they all have the same first 2 parents (IN0 and PLL0). See the
comment in k210_register_mux_clk(). I thought this was a clean way of setting
things up without a parent array for each mux clock.
>
>> +
>> + ret = k210_register_plls(np);
>> + if (ret)
>> + return;
>> +
>> + ret = k210_register_aclk(np);
>> + if (ret)
>> + return;
>> +
>> + /*
>> + * Critical clocks: there are no consumers of the SRAM clocks,
>> + * including the AI clock for the third SRAM bank. The CPU clock
>> + * is only referenced by the uarths serial device and so would be
>> + * disabled if the serial console is disabled. Mark all these clocks
>> + * as critical so that they are never disabled by the core clock
>> + * management.
>> + */
>> + hws[K210_CLK_CPU] =
>> + k210_register_critical_clk(K210_CLK_CPU, K210_PARENT_ACLK);
>> + hws[K210_CLK_SRAM0] =
>> + k210_register_critical_clk(K210_CLK_SRAM0, K210_PARENT_ACLK);
>> + hws[K210_CLK_SRAM1] =
>> + k210_register_critical_clk(K210_CLK_SRAM1, K210_PARENT_ACLK);
>> + hws[K210_CLK_AI] =
>> + k210_register_critical_clk(K210_CLK_AI, K210_PARENT_PLL1);
>> +
>> + /* Clocks with aclk as source */
>> + hws[K210_CLK_DMA] = k210_register_aclk_child(K210_CLK_DMA);
>> + hws[K210_CLK_FFT] = k210_register_aclk_child(K210_CLK_FFT);
>> + hws[K210_CLK_ROM] = k210_register_aclk_child(K210_CLK_ROM);
>> + hws[K210_CLK_DVP] = k210_register_aclk_child(K210_CLK_DVP);
>> + hws[K210_CLK_APB0] = k210_register_aclk_child(K210_CLK_APB0);
>> + hws[K210_CLK_APB1] = k210_register_aclk_child(K210_CLK_APB1);
>> + hws[K210_CLK_APB2] = k210_register_aclk_child(K210_CLK_APB2);
>> +
>> + /* Clocks with PLL0 as source */
>> + hws[K210_CLK_SPI0] = k210_register_pll0_child(K210_CLK_SPI0);
>> + hws[K210_CLK_SPI1] = k210_register_pll0_child(K210_CLK_SPI1);
>> + hws[K210_CLK_SPI2] = k210_register_pll0_child(K210_CLK_SPI2);
>> + hws[K210_CLK_I2C0] = k210_register_pll0_child(K210_CLK_I2C0);
>> + hws[K210_CLK_I2C1] = k210_register_pll0_child(K210_CLK_I2C1);
>> + hws[K210_CLK_I2C2] = k210_register_pll0_child(K210_CLK_I2C2);
>> +
>> + /* Clocks with PLL2 as source */
>> + hws[K210_CLK_I2S0] = k210_register_pll2_child(K210_CLK_I2S0);
>> + hws[K210_CLK_I2S1] = k210_register_pll2_child(K210_CLK_I2S1);
>> + hws[K210_CLK_I2S2] = k210_register_pll2_child(K210_CLK_I2S2);
>> + hws[K210_CLK_I2S0_M] = k210_register_pll2_child(K210_CLK_I2S0_M);
>> + hws[K210_CLK_I2S1_M] = k210_register_pll2_child(K210_CLK_I2S1_M);
>> + hws[K210_CLK_I2S2_M] = k210_register_pll2_child(K210_CLK_I2S2_M);
>> +
>> + /* Clocks with IN0 as source */
>> + hws[K210_CLK_WDT0] = k210_register_in0_child(K210_CLK_WDT0);
>> + hws[K210_CLK_WDT1] = k210_register_in0_child(K210_CLK_WDT1);
>> + hws[K210_CLK_RTC] = k210_register_in0_child(K210_CLK_RTC);
>> +
>> + /* Clocks with APB0 as source */
>> + kcl->parents[K210_PARENT_APB0] = hws[K210_CLK_APB0];
>> + hws[K210_CLK_GPIO] = k210_register_apb0_child(K210_CLK_GPIO);
>> + hws[K210_CLK_UART1] = k210_register_apb0_child(K210_CLK_UART1);
>> + hws[K210_CLK_UART2] = k210_register_apb0_child(K210_CLK_UART2);
>> + hws[K210_CLK_UART3] = k210_register_apb0_child(K210_CLK_UART3);
>> + hws[K210_CLK_FPIOA] = k210_register_apb0_child(K210_CLK_FPIOA);
>> + hws[K210_CLK_SHA] = k210_register_apb0_child(K210_CLK_SHA);
>> +
>> + /* Clocks with APB1 as source */
>> + kcl->parents[K210_PARENT_APB1] = hws[K210_CLK_APB1];
>> + hws[K210_CLK_AES] = k210_register_apb1_child(K210_CLK_AES);
>> + hws[K210_CLK_OTP] = k210_register_apb1_child(K210_CLK_OTP);
>> +
>> + /* Mux clocks with in0 or pll0 as source */
>> + hws[K210_CLK_SPI3] = k210_register_mux_clk(K210_CLK_SPI3);
>> + hws[K210_CLK_TIMER0] = k210_register_mux_clk(K210_CLK_TIMER0);
>> + hws[K210_CLK_TIMER1] = k210_register_mux_clk(K210_CLK_TIMER1);
>> + hws[K210_CLK_TIMER2] = k210_register_mux_clk(K210_CLK_TIMER2);
>> +
>> + for (i = 0; i < K210_NUM_CLKS; i++) {
>> + if (IS_ERR(hws[i])) {
>> + pr_err("%pOFP: register clock %s failed %ld\n",
>> + np, k210_clks[i].name, PTR_ERR(hws[i]));
>> + return;
>> + }
>> + }
>> +
>> + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, kcl->clk_data);
>> + if (ret) {
>> + pr_err("%pOFP: add clock provider failed %d\n", np, ret);
>> + return;
>> + }
>> +
>> + pr_info("%pOFP: CPU running at %lu MHz\n",
>> + np, clk_hw_get_rate(hws[K210_CLK_CPU]) / 1000000);
>> +}
>> +
>> +CLK_OF_DECLARE(k210_clk, "canaan,k210-clk", k210_clk_init);
>> +
>> +/*
>> + * Enable PLL1 to be able to use the AI SRAM.
>> + */
>> +void __init k210_clk_early_init(void __iomem *regs)
>
> Is it impossible to put this into the bootloader? Does riscv have some
> boot constraints or requirements that say memory should be working
> before the kernel starts? Otherwise this is depressing that we have to
> enable clks to get memory to work early like this.
Given the very low amount of memory on this board (8MB total), booting it
without a boot loader, with the board startup directly jumping to kernel start
at address 0x800000000, is more efficient. Sean added U-boot support but using
the board without it with a builtin DTB is far easier and does not mandate using
the SD card.
For this particular phase of the initialization, this has nothing to do with
RISC-V. This is specific to this board where the third 2MB SRAM bank is by
default off when the board powers-up. This bank is by default reserved for the
SoC KPU which is an AI acceleration engine. This memory can however be
referenced and used as general purpose by the CPU, but it has to be turned on
first using PLL1. Turning it on super early is needed as the DT is moved toward
the end of the address space when it is parsed early in the startup code.
>
>> +{
>> + struct k210_pll pll1;
>> +
>> + /* Make sure ACLK selector is set to PLL0 */
>> + k210_aclk_set_selector(1);
>> +
>> + /* Startup PLL1 to enable the aisram bank for general memory use */
>> + k210_init_pll(&pll1, K210_PLL1, regs);
>> + k210_pll_enable_hw(&pll1);
>> +}
>
--
Damien Le Moal
Western Digital Research
More information about the linux-riscv
mailing list