On 12/5/20 2:43 AM, Damien Le Moal wrote:
Hi Stephen,
Thank you for the review. I will address all your comments.
I just have a few questions below.
On Fri, 2020-12-04 at 22:19 -0800, Stephen Boyd wrote:
Quoting Damien Le Moal (2020-12-01 19:24:50)
diff --git a/MAINTAINERS b/MAINTAINERS
index 2daa6ee673f7..3da9a7a02f61 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3822,6 +3822,22 @@ W: https://github.com/Cascoda/ca8210-linux.git
F: Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
F: drivers/net/ieee802154/ca8210.c
+CANAAN/KENDRYTE K210 SOC CLOCK DRIVER
+M: Damien Le Moal <damien.lemoal@xxxxxxx>
+L: linux-riscv@xxxxxxxxxxxxxxxxxxx
+L: linux-clk@xxxxxxxxxxxxxxx (clock driver)
Is this needed? I think we cover all of drivers/clk/ and bindings/clock
already.
I was not sure about that so I added the entry. Will remove it.
+S: Maintained
+F: Documentation/devicetree/bindings/clock/canaan,k210-clk.yaml
+F: drivers/clk/clk-k210.c
diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index 88ac0d1a5da4..f2f9633087d1 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -29,6 +29,8 @@ config SOC_CANAAN
select SERIAL_SIFIVE if TTY
select SERIAL_SIFIVE_CONSOLE if TTY
select SIFIVE_PLIC
+ select SOC_K210_SYSCTL
+ select CLK_K210
Any reason to do this vs. just make it the default?
I do not understand here... Just selecting the drivers needed for the SoC here.
Is there any other way of doing this ?
[...]
+
+ while (true) {
+ reg = readl(pll->lock);
+ if ((reg & mask) == mask)
+ break;
+
+ reg |= BIT(pll->lock_shift + K210_PLL_CLEAR_SLIP);
+ writel(reg, pll->lock);
Is this readl_poll_timeout?
Oh. Yes, it is. I did not know about this function. Will change the code to use
it.
FWIW the timeout could be incorrect since we might be configuring a
parent of ACLK. And realistically the only way this fails is if a user
has edited this file and put in invalid PLL parameters. I don't think
you gain much by adding a timeout.
+ }
+}
+
+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;
+
+ if (pll->id == K210_PLL0) {
+ /* Re-parent aclk to IN0 to keep the CPUs running */
+ k210_aclk_set_selector(0);
+ }
+
+ /* Set 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);
+
+ /* Ensure reset is low before asserting it */
+ reg &= ~K210_PLL_RESET;
+ writel(reg, pll->reg);
+ reg |= K210_PLL_RESET;
+ writel(reg, pll->reg);
+ nop();
+ nop();
Are these nops needed for some reason? Any comment to add here? It's
basically non-portable code and hopefully nothing is inserted into that
writel function that shouldn't be there.
No clue... They are "magic" nops that are present in the K210 SDK from
Kendryte. I copied that, but do not actually know if they are really needed. I
am working without any specs for the hardware: the Kendryte SDK is my main
source of information here. I will try to remove them or just replace this with
a delay() call a nd see what happens.
Basically any delay should work as long as it takes more than 2
instructions ;) Of course, anything longer than that just delays startup
for no reason.
--Sean
[...]
+static int k210_aclk_set_parent(struct clk_hw *hw, u8 index)
+{
+ if (WARN_ON(index > 1))
Is this possible? What am I going to do as a user if this happens?
No, it is not possible. Will remove this. I could put a BUG_ON(), but I am not
a fan this extreme.
[...]
+ /*
+ * in0 is the system base fixed-rate 26MHz oscillator which
+ * should already be defined by the device tree. If it is not,
+ * create it here.
Are there old DTBs that don't have this? Sadface.
No, not any old DTB. Will remove that.
+ */
+ in0_clk = of_clk_get(np, 0);
+ if (IS_ERR(in0_clk)) {
+ pr_warn("%pOFP: in0 oscillator not found\n", np);
+ hws[K210_CLK_IN0] =
+ clk_hw_register_fixed_rate(NULL, "in0", NULL,
+ 0, K210_IN0_RATE);
+ } else {
+ hws[K210_CLK_IN0] = __clk_get_hw(in0_clk);
+ }
+ if (IS_ERR(hws[K210_CLK_IN0])) {
+ pr_err("%pOFP: failed to get base oscillator\n", np);
+ goto err;
+ }
+
+ in0 = clk_hw_get_name(hws[K210_CLK_IN0]);
+ aclk_parents[0] = in0;
+ pll_parents[0] = in0;
+ mux_parents[0] = in0;
Can we use the new way of specifying clk parents so that we don't have
to use __clk_get_hw(), of_clk_get(), and clk_hw_get_name()? Hopefully
the core can handl that all instead of this driver.
Not sure what new way of specifying the parent you are referring to here.
clk_hw_set_parent() ?
+
+ /* PLLs */
+ hws[K210_CLK_PLL0] =
+ k210_register_pll(K210_PLL0, "pll0", pll_parents, 1, 0);
+ hws[K210_CLK_PLL1] =
+ k210_register_pll(K210_PLL1, "pll1", pll_parents, 1, 0);
+ hws[K210_CLK_PLL2] =
+ k210_register_pll(K210_PLL2, "pll2", pll_parents, 3, 0);
+
+ /* aclk: muxed of in0 and pll0_d, no gate */
+ hws[K210_CLK_ACLK] = k210_register_aclk();
+
+ /*
+ * Clocks with aclk as source: the CPU clock is obviously critical.
+ * So is the CLINT clock as the scheduler clocksource.
+ */
+ hws[K210_CLK_CPU] =
+ k210_register_clk(K210_CLK_CPU, "cpu", "aclk", CLK_IS_CRITICAL);
+ hws[K210_CLK_CLINT] =
+ clk_hw_register_fixed_factor(NULL, "clint", "aclk",
+ CLK_IS_CRITICAL, 1, 50);
Is anyone getting these clks? It's nice and all to model things in the
clk framework but if they never have a consumer then it is sort of
useless and just wastes memory and causes more overhead.
The CPU and SRAM clocks do not have any consumer, so I could remove them (just
enable the HW but not represent them as clocks in the driver). There is no
direct consumer of ACLK but it is the parent of multiple clocks, including the
SRAM clocks. So it needs to be represented as a clock and kept alive even if
all the peripheral drivers needing it are disabled. Otherwise, the system just
stops (SRAM accesses hang).
[...]
+ 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);
+ else
+ pr_info("%pOFP: CPU running at %lu MHz\n",
Is this important? Is there a CPUfreq driver that runs and tells us the
boot CPU frequency instead? Doesn't feel like we care in the clk driver
about this.
There is no CPU freq driver that gives this frequency that I know of. That is
why I added the message since the driver basically just comes up using the
default HW settings for the SoC. CPU freq speed can be changed though by
increasing the PLL freq. Just not supporting this for now as it is tricky to
do: the SRAM clocks depend on aclk and PLL1 and if these are not the same
value, the system hangs (most likely because we end up with the sram banks
running at different speeds, which the SoC cache does not like).
[...]
+CLK_OF_DECLARE_DRIVER(k210_clk, "canaan,k210-clk", k210_clk_init);
Is this needed or can this just be a plain platform driver? If something
is needed early for a clocksource or clockevent then the driver can be
split to register those few clks early from this hook and then register
the rest later when the platform device probes. That's what
CLK_OF_DECLARE_DRIVER is for. A DECLARE_DRIVER without a platform driver
is incorrect.
I think I can clean this up: aclk and clint clocks are needed early but others
can likely be deferred. Will fix this up.
Thanks !
