Hi Ilpo, Thanks for your comments. the patch will modify according to your comments and will be sent in the next kernel revision 5.19.rc1 On Mon, 23 May 2022 at 10:07, Ilpo Järvinen wrote: > On Sun, 22 May 2022, Tomer Maimon wrote: > > > Nuvoton Arbel BMC NPCM7XX contains an integrated clock controller, which > > generates and supplies clocks to all modules within the BMC. > > > > Signed-off-by: Tomer Maimon > > > +static struct clk_hw * > > +npcm8xx_clk_register_pll(void __iomem *pllcon, const char *name, > > + const char *parent_name, unsigned long flags) > > +{ > > + struct npcm8xx_clk_pll *pll; > > + struct clk_init_data init; > > + struct clk_hw *hw; > > + int ret; > > + > > + pll = kzalloc(sizeof(*pll), GFP_KERNEL); > > + if (!pll) > > + return ERR_PTR(-ENOMEM); > > + > > + pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name); > > + > > + init.name = name; > > + init.ops = &npcm8xx_clk_pll_ops; > > + init.parent_names = &parent_name; > > + init.num_parents = 1; > > + init.flags = flags; > > + > > + pll->pllcon = pllcon; > > + pll->hw.init = &init; > > + > > + hw = &pll->hw; > > + > > + ret = clk_hw_register(NULL, hw); > > + if (ret) { > > + kfree(pll); > > + hw = ERR_PTR(ret); > > + } > > + > > + return hw; > > +} > > + > > +#define NPCM8XX_CLKEN1 (0x00) > > +#define NPCM8XX_CLKEN2 (0x28) > > +#define NPCM8XX_CLKEN3 (0x30) > > +#define NPCM8XX_CLKEN4 (0x70) > > +#define NPCM8XX_CLKSEL (0x04) > > +#define NPCM8XX_CLKDIV1 (0x08) > > +#define NPCM8XX_CLKDIV2 (0x2C) > > +#define NPCM8XX_CLKDIV3 (0x58) > > +#define NPCM8XX_CLKDIV4 (0x7C) > > +#define NPCM8XX_PLLCON0 (0x0C) > > +#define NPCM8XX_PLLCON1 (0x10) > > +#define NPCM8XX_PLLCON2 (0x54) > > +#define NPCM8XX_SWRSTR (0x14) > > +#define NPCM8XX_IRQWAKECON (0x18) > > +#define NPCM8XX_IRQWAKEFLAG (0x1C) > > +#define NPCM8XX_IPSRST1 (0x20) > > +#define NPCM8XX_IPSRST2 (0x24) > > +#define NPCM8XX_IPSRST3 (0x34) > > +#define NPCM8XX_WD0RCR (0x38) > > +#define NPCM8XX_WD1RCR (0x3C) > > +#define NPCM8XX_WD2RCR (0x40) > > +#define NPCM8XX_SWRSTC1 (0x44) > > +#define NPCM8XX_SWRSTC2 (0x48) > > +#define NPCM8XX_SWRSTC3 (0x4C) > > +#define NPCM8XX_SWRSTC4 (0x50) > > +#define NPCM8XX_CORSTC (0x5C) > > +#define NPCM8XX_PLLCONG (0x60) > > +#define NPCM8XX_AHBCKFI (0x64) > > +#define NPCM8XX_SECCNT (0x68) > > +#define NPCM8XX_CNTR25M (0x6C) > > +#define NPCM8XX_THRTL_CNT (0xC0) > > + > > +struct npcm8xx_clk_gate_data { > > + u32 reg; > > + u8 bit_idx; > > + const char *name; > > + const char *parent_name; > > + unsigned long flags; > > + /* > > + * If this clock is exported via DT, set onecell_idx to constant > > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h > for > > + * this specific clock. Otherwise, set to -1. > > + */ > > + int onecell_idx; > > +}; > > + > > +struct npcm8xx_clk_mux_data { > > + u8 shift; > > + u8 mask; > > + u32 *table; > > + const char *name; > > + const char * const *parent_names; > > + u8 num_parents; > > + unsigned long flags; > > + /* > > + * If this clock is exported via DT, set onecell_idx to constant > > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h > for > > + * this specific clock. Otherwise, set to -1. > > + */ > > + int onecell_idx; > > + > > +}; > > + > > +struct npcm8xx_clk_div_fixed_data { > > + u8 mult; > > + u8 div; > > + const char *name; > > + const char *parent_name; > > + u8 clk_divider_flags; > > + /* > > + * If this clock is exported via DT, set onecell_idx to constant > > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h > for > > + * this specific clock. Otherwise, set to -1. > > + */ > > + int onecell_idx; > > +}; > > + > > +struct npcm8xx_clk_div_data { > > + u32 reg; > > + u8 shift; > > + u8 width; > > + const char *name; > > + const char *parent_name; > > + u8 clk_divider_flags; > > + unsigned long flags; > > + /* > > + * If this clock is exported via DT, set onecell_idx to constant > > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h > for > > + * this specific clock. Otherwise, set to -1. > > + */ > > + int onecell_idx; > > +}; > > + > > +struct npcm8xx_clk_pll_data { > > + u32 reg; > > + const char *name; > > + const char *parent_name; > > + unsigned long flags; > > + /* > > + * If this clock is exported via DT, set onecell_idx to constant > > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h > for > > + * this specific clock. Otherwise, set to -1. > > + */ > > + int onecell_idx; > > +}; > > + > > > > +/* > > + * Single copy of strings used to refer to clocks within this driver > indexed by > > + * above enum. > > + */ > > +#define NPCM8XX_CLK_S_REFCLK "refclk" > > +#define NPCM8XX_CLK_S_SYSBYPCK "sysbypck" > > +#define NPCM8XX_CLK_S_MCBYPCK "mcbypck" > > +#define NPCM8XX_CLK_S_GFXBYPCK "gfxbypck" > > +#define NPCM8XX_CLK_S_PLL0 "pll0" > > +#define NPCM8XX_CLK_S_PLL1 "pll1" > > +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2" > > +#define NPCM8XX_CLK_S_PLL2 "pll2" > > +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx" > > +#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2" > > +#define NPCM8XX_CLK_S_PIX_MUX "gfx_pixel" > > +#define NPCM8XX_CLK_S_GPRFSEL_MUX "gprfsel_mux" > > +#define NPCM8XX_CLK_S_MC_MUX "mc_phy" > > +#define NPCM8XX_CLK_S_CPU_MUX "cpu" /*AKA system clock.*/ > > Add spaces around comment. > > > +#define NPCM8XX_CLK_S_MC "mc" > > +#define NPCM8XX_CLK_S_AXI "axi" /*AKA CLK2*/ > > +#define NPCM8XX_CLK_S_AHB "ahb" /*AKA CLK4*/ > > Ditto. > > > +static void __init npcm8xx_clk_init(struct device_node *clk_np) > > +{ > > + struct clk_hw_onecell_data *npcm8xx_clk_data; > > + void __iomem *clk_base; > > + struct resource res; > > + struct clk_hw *hw; > > + int ret; > > + int i; > > + > > + ret = of_address_to_resource(clk_np, 0, &res); > > + if (ret) { > > + pr_err("%pOFn: failed to get resource, ret %d\n", clk_np, > ret); > > + return; > > + } > > + > > + clk_base = ioremap(res.start, resource_size(&res)); > > + if (!clk_base) > > + goto npcm8xx_init_error; > > + > > + npcm8xx_clk_data = kzalloc(struct_size(npcm8xx_clk_data, hws, > > + NPCM8XX_NUM_CLOCKS), > GFP_KERNEL); > > + if (!npcm8xx_clk_data) > > + goto npcm8xx_init_np_err; > > + > > + npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS; > > + > > + for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++) > > + npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER); > > + > > + /* Register plls */ > > + for (i = 0; i < ARRAY_SIZE(npcm8xx_plls); i++) { > > + const struct npcm8xx_clk_pll_data *pll_data = > &npcm8xx_plls[i]; > > + > > + hw = npcm8xx_clk_register_pll(clk_base + pll_data->reg, > > + pll_data->name, > > + pll_data->parent_name, > > + pll_data->flags); > > + if (IS_ERR(hw)) { > > Who deregisters the already registered plls on error paths? > > You might want to consider devm_ variants in npcm8xx_clk_register_pll() to > make the cleanup simpler. > > Please check the other error path rollbacks from this point onward too. > > + pr_err("npcm8xx_clk: Can't register pll\n"); > > + goto npcm8xx_init_fail; > > + } > > + > > + if (pll_data->onecell_idx >= 0) > > + npcm8xx_clk_data->hws[pll_data->onecell_idx] = hw; > > + } > > + > > + /* Register fixed dividers */ > > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_PLL1_DIV2, > > + NPCM8XX_CLK_S_PLL1, 0, 1, 2); > > + if (IS_ERR(hw)) { > > + pr_err("npcm8xx_clk: Can't register fixed div\n"); > > + goto npcm8xx_init_fail; > > + } > > + > > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_PLL2_DIV2, > > + NPCM8XX_CLK_S_PLL2, 0, 1, 2); > > + if (IS_ERR(hw)) { > > + pr_err("npcm8xx_clk: Can't register pll div2\n"); > > + goto npcm8xx_init_fail; > > + } > > + > > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_PRE_CLK, > > + NPCM8XX_CLK_S_CPU_MUX, 0, 1, 2); > > + if (IS_ERR(hw)) { > > + pr_err("npcm8xx_clk: Can't register ckclk div2\n"); > > + goto npcm8xx_init_fail; > > + } > > + > > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_AXI, > > + NPCM8XX_CLK_S_TH, 0, 1, 2); > > + if (IS_ERR(hw)) { > > + pr_err("npcm8xx_clk: Can't register axi div2\n"); > > + goto npcm8xx_init_fail; > > + } > > + > > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_ATB, > > + NPCM8XX_CLK_S_AXI, 0, 1, 2); > > + if (IS_ERR(hw)) { > > + pr_err("npcm8xx_clk: Can't register atb div2\n"); > > + goto npcm8xx_init_fail; > > + } > > + > > + /* Register muxes */ > > + for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) { > > + const struct npcm8xx_clk_mux_data *mux_data = > &npcm8xx_muxes[i]; > > + > > + hw = clk_hw_register_mux_table(NULL, mux_data->name, > > + mux_data->parent_names, > > + mux_data->num_parents, > > + mux_data->flags, > > + clk_base + NPCM8XX_CLKSEL, > > + mux_data->shift, > > + mux_data->mask, 0, > > + mux_data->table, > > + &npcm8xx_clk_lock); > > + > > + if (IS_ERR(hw)) { > > + pr_err("npcm8xx_clk: Can't register mux\n"); > > + goto npcm8xx_init_fail; > > + } > > + > > + if (mux_data->onecell_idx >= 0) > > + npcm8xx_clk_data->hws[mux_data->onecell_idx] = hw; > > + } > > + > > + /* Register clock dividers specified in npcm8xx_divs */ > > + for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) { > > + const struct npcm8xx_clk_div_data *div_data = > &npcm8xx_divs[i]; > > + > > + hw = clk_hw_register_divider(NULL, div_data->name, > > + div_data->parent_name, > > + div_data->flags, > > + clk_base + div_data->reg, > > + div_data->shift, > div_data->width, > > + div_data->clk_divider_flags, > > + &npcm8xx_clk_lock); > > + if (IS_ERR(hw)) { > > + pr_err("npcm8xx_clk: Can't register div table\n"); > > + goto npcm8xx_init_fail; > > + } > > + > > + if (div_data->onecell_idx >= 0) > > + npcm8xx_clk_data->hws[div_data->onecell_idx] = hw; > > + } > > + > > + ret = of_clk_add_hw_provider(clk_np, of_clk_hw_onecell_get, > > + npcm8xx_clk_data); > > + if (ret) > > + pr_err("failed to add DT provider: %d\n", ret); > > + > > + of_node_put(clk_np); > > + > > + return; > > + > > +npcm8xx_init_fail: > > + kfree(npcm8xx_clk_data->hws); > > +npcm8xx_init_np_err: > > + iounmap(clk_base); > > +npcm8xx_init_error: > > + of_node_put(clk_np); > > +} > > + > > +CLK_OF_DECLARE(npcm8xx_clk_init, "nuvoton,npcm845-clk", > npcm8xx_clk_init); > > > > -- > i. > > Best regards, Tomer