Hi Conor,

On Thu, Dec 16, 2021 at 10:41 AM [off-list ref] wrote:

From: Daire McNamara <daire.mcnamara@microchip.com>

Add support for clock configuration on Microchip PolarFire SoC

Co-developed-by: Padmarao Bengari <redacted>
Signed-off-by: Padmarao Bengari <redacted>
Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Thanks for the update!

+static void mpfs_clk_unregister_cfg(struct device *dev, struct clk_hw *hw)
+{
+       devm_clk_hw_unregister(dev, hw);

Just call devm_clk_hw_unregister() directly from the caller?

+}

+static int mpfs_clk_register_cfgs(struct device *dev, struct mpfs_cfg_hw_clock *cfg_hws,
+                                 unsigned int num_clks, struct mpfs_clock_data *data)
+{
+       struct clk_hw *hw;
+       struct clk *clk_parent;
+       void __iomem *sys_base = data->base;
+       unsigned int i, id;
+
+       for (i = 0; i < num_clks; i++) {
+               struct mpfs_cfg_hw_clock *cfg_hw = &cfg_hws[i];
+
+               clk_parent = devm_clk_get(dev, NULL);

Please get the clock once, in mpfs_clk_probe()...

+               if (IS_ERR(clk_parent))
+                       return -EPROBE_DEFER;

... and propagate the actual error.

+
+               cfg_hw->cfg.parent = __clk_get_hw(clk_parent);
+               cfg_hw->hw.init = CLK_HW_INIT_HW(cfg_hw->cfg.name, cfg_hw->cfg.parent,
+                                                &mpfs_clk_cfg_ops, cfg_hw->cfg.flags);
+               hw = mpfs_clk_register_cfg(dev, cfg_hw, sys_base);
+               if (IS_ERR(hw)) {
+                       dev_err(dev, "failed to register clock %s\n", cfg_hw->cfg.name);
+                       goto err_clk;
+               }
+
+               id = cfg_hws[i].cfg.id;
+               data->hw_data.hws[id] = hw;
+       }
+
+       return 0;
+
+err_clk:
+       while (i--)
+               mpfs_clk_unregister_cfg(dev, data->hw_data.hws[cfg_hws[i].cfg.id]);
+
+       return PTR_ERR(hw);
+}

+static struct mpfs_periph_hw_clock mpfs_periph_clks[] = {
+       CLK_PERIPH(CLK_ENVM, "clk_periph_envm", PARENT_CLK(AHB), 0, CLK_IS_CRITICAL),
+       CLK_PERIPH(CLK_MAC0, "clk_periph_mac0", PARENT_CLK(AHB), 1, 0),
+       CLK_PERIPH(CLK_MAC1, "clk_periph_mac1", PARENT_CLK(AHB), 2, 0),
+       CLK_PERIPH(CLK_MMC, "clk_periph_mmc", PARENT_CLK(AHB), 3, 0),
+       CLK_PERIPH(CLK_TIMER, "clk_periph_timer", PARENT_CLK(AHB), 4, 0),
+       CLK_PERIPH(CLK_MMUART0, "clk_periph_mmuart0", PARENT_CLK(AHB), 5, CLK_IS_CRITICAL),
+       CLK_PERIPH(CLK_MMUART1, "clk_periph_mmuart1", PARENT_CLK(AHB), 6, 0),
+       CLK_PERIPH(CLK_MMUART2, "clk_periph_mmuart2", PARENT_CLK(AHB), 7, 0),
+       CLK_PERIPH(CLK_MMUART3, "clk_periph_mmuart3", PARENT_CLK(AHB), 8, 0),
+       CLK_PERIPH(CLK_MMUART4, "clk_periph_mmuart4", PARENT_CLK(AHB), 9, 0),
+       CLK_PERIPH(CLK_SPI0, "clk_periph_spi0", PARENT_CLK(AHB), 10, 0),
+       CLK_PERIPH(CLK_SPI1, "clk_periph_spi1", PARENT_CLK(AHB), 11, 0),
+       CLK_PERIPH(CLK_I2C0, "clk_periph_i2c0", PARENT_CLK(AHB), 12, 0),
+       CLK_PERIPH(CLK_I2C1, "clk_periph_i2c1", PARENT_CLK(AHB), 13, 0),
+       CLK_PERIPH(CLK_CAN0, "clk_periph_can0", PARENT_CLK(AHB), 14, 0),
+       CLK_PERIPH(CLK_CAN1, "clk_periph_can1", PARENT_CLK(AHB), 15, 0),
+       CLK_PERIPH(CLK_USB, "clk_periph_usb", PARENT_CLK(AHB), 16, 0),
+       CLK_PERIPH(CLK_RTC, "clk_periph_rtc", PARENT_CLK(AHB), 18, 0),
+       CLK_PERIPH(CLK_QSPI, "clk_periph_qspi", PARENT_CLK(AHB), 19, 0),
+       CLK_PERIPH(CLK_GPIO0, "clk_periph_gpio0", PARENT_CLK(AHB), 20, 0),
+       CLK_PERIPH(CLK_GPIO1, "clk_periph_gpio1", PARENT_CLK(AHB), 21, 0),
+       CLK_PERIPH(CLK_GPIO2, "clk_periph_gpio2", PARENT_CLK(AHB), 22, 0),
+       CLK_PERIPH(CLK_DDRC, "clk_periph_ddrc", PARENT_CLK(AHB), 23, CLK_IS_CRITICAL),
+       CLK_PERIPH(CLK_FIC0, "clk_periph_fic0", PARENT_CLK(AHB), 24, CLK_IS_CRITICAL),
+       CLK_PERIPH(CLK_FIC1, "clk_periph_fic1", PARENT_CLK(AHB), 25, CLK_IS_CRITICAL),
+       CLK_PERIPH(CLK_FIC2, "clk_periph_fic2", PARENT_CLK(AHB), 26, CLK_IS_CRITICAL),
+       CLK_PERIPH(CLK_FIC3, "clk_periph_fic3", PARENT_CLK(AHB), 27, CLK_IS_CRITICAL),

Why were the ficN clocks changed to critical clocks?
it seemed to work fine without that before?

+       CLK_PERIPH(CLK_ATHENA, "clk_periph_athena", PARENT_CLK(AHB), 28, 0),
+       CLK_PERIPH(CLK_CFM, "clk_periph_cfm", PARENT_CLK(AHB), 29, 0),
+};
+
+static void mpfs_clk_unregister_periph(struct device *dev, struct clk_hw *hw)
+{
+       devm_clk_hw_unregister(dev, hw);

Just call devm_clk_hw_unregister() directly from the caller?

+}

The above are minor comments, so
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds