Hi Conor, Daire,

On Tue, Nov 30, 2021 at 3:06 PM [off-list ref] wrote:

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

Add support for clock configuration on Microchip PolarFire SoC

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Thanks for your patch!

implicit gap of 4 bytes

+       const char *name;
+       u8 shift;
+       u8 width;

implicit gap of 6 bytes

+       const struct clk_div_table *table;
+       unsigned long flags;

Hence you better move id, shift, and width to the end.

+};
+
+struct mpfs_cfg_hw_clock {
+       struct mpfs_cfg_clock cfg;
+       void __iomem *sys_base;
+       /* lock is used to prevent multiple writes */
+       spinlock_t *lock;
+       struct clk_hw hw;
+       struct clk_init_data init;
+};
+
+#define to_mpfs_cfg_clk(_hw) container_of(_hw, struct mpfs_cfg_hw_clock, hw)
+
+struct mpfs_periph_clock {
+       unsigned int id;

implicit gap of 4 bytes

+       const char *name;
+       u8 shift;

implicit gap of 7 bytes

+       unsigned long flags;
+};
+
+struct mpfs_periph_hw_clock {
+       struct mpfs_periph_clock periph;
+       void __iomem *sys_base;
+       /* lock is used to prevent multiple writes */
+       spinlock_t *lock;

This is not used?

+       struct clk_hw hw;
+};
+
+#define to_mpfs_periph_clk(_hw) container_of(_hw, struct mpfs_periph_hw_clock, hw)
+
+/*
+ * mpfs_clk_lock prevents anything else from writing to the
+ * mpfs clk block while a software locked register is being written.
+ */
+static DEFINE_SPINLOCK(mpfs_clk_lock);
+
+static struct clk_parent_data mpfs_cfg_parent[] = {
+       { .fw_name = "msspllclk", .name = "msspllclk" },

So you rely on the name of the fixed-clock reference clock? That is
very fragile. Please instead use devm_clk_get() to get the reference
clock from the "clocks" property of the clock-controller node.

+static unsigned long mpfs_cfg_clk_recalc_rate(struct clk_hw *hw, unsigned long prate)
+{
+       struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
+       struct mpfs_cfg_clock *cfg = &cfg_hw->cfg;
+       void __iomem *base_addr = cfg_hw->sys_base;
+       unsigned long rate;
+       u32 val;
+
+       val = readl_relaxed(base_addr + REG_CLOCK_CONFIG_CR) >> cfg->shift;
+       val &= clk_div_mask(cfg->width);
+       rate = prate / (1u << val);
+
+       return rate;

return prate / (1u << val);

+}

+static int mpfs_cfg_clk_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long prate)
+{
+       struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
+       struct mpfs_cfg_clock *cfg = &cfg_hw->cfg;
+       void __iomem *base_addr = cfg_hw->sys_base;
+       unsigned long flags = 0;
+       u32 val;
+       int divider_setting;
+
+       divider_setting = divider_get_val(rate, prate, cfg->table, cfg->width, cfg_hw->cfg.flags);
+
+       if (divider_setting < 0)
+               return divider_setting;
+
+       if (cfg_hw->lock)

Isn't this branch always taken?

+               spin_lock_irqsave(cfg_hw->lock, flags);
+       else
+               __acquire(cfg_hw->lock);
+
+       val = readl_relaxed(base_addr + REG_CLOCK_CONFIG_CR);
+       val &= ~(clk_div_mask(cfg->width) << cfg_hw->cfg.shift);
+       val |= divider_setting << cfg->shift;
+       writel_relaxed(val, base_addr + REG_CLOCK_CONFIG_CR);
+
+       if (cfg_hw->lock)
+               spin_unlock_irqrestore(cfg_hw->lock, flags);
+       else
+               __release(cfg_hw->lock);
+
+       return 0;
+}

+static void mpfs_clk_unregister_cfg(struct device *dev, struct clk_hw *hw)
+{
+       struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
+
+       devm_clk_hw_unregister(dev, hw);
+       kfree(cfg_hw);

This is freeing a part of the big array allocated with devm_kzalloc()?

+}
+
+static struct clk_hw *mpfs_clk_register_cfg(struct device *dev,
+                                           struct mpfs_cfg_hw_clock *cfg_hw,
+                                           void __iomem *sys_base)
+{
+       struct clk_hw *hw;
+       int err;
+
+       cfg_hw->sys_base = sys_base;
+       cfg_hw->lock = &mpfs_clk_lock;
+
+       hw = &cfg_hw->hw;
+       err = devm_clk_hw_register(dev, hw);
+       if (err)
+               return ERR_PTR(err);
+
+       return hw;
+}
+
+static int mpfs_clk_register_cfgs(struct device *dev, struct mpfs_cfg_hw_clock *cfg_hws,
+                                 int num_clks, struct mpfs_clock_data *data)

unsigned int num_clks

+{
+       struct clk_hw *hw;
+       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];
+
+               hw = mpfs_clk_register_cfg(dev, cfg_hw, sys_base);
+               if (IS_ERR(hw)) {
+                       dev_err(dev, "%s: failed to register clock %s\n", __func__,

I guess the __func__ can be dropped, as the clock name is unique?

+                               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 void mpfs_clk_unregister_periph(struct device *dev, struct clk_hw *hw)
+{
+       struct mpfs_periph_hw_clock *periph_hw = to_mpfs_periph_clk(hw);
+
+       devm_clk_hw_unregister(dev, hw);
+       kfree(periph_hw);'

This is freeing a part of the big array allocated with devm_kzalloc()?

+}

+static int mpfs_clk_register_periphs(struct device *dev, struct mpfs_periph_hw_clock *periph_hws,
+                                    int num_clks, struct mpfs_clock_data *data)
+{
+       struct clk_hw *hw;
+       void __iomem *sys_base = data->base;
+       unsigned int i, id;
+
+       for (i = 0; i < num_clks; i++) {
+               struct mpfs_periph_hw_clock *periph_hw = &periph_hws[i];
+
+               hw = mpfs_clk_register_periph(dev, periph_hw, sys_base);
+               if (IS_ERR(hw)) {
+                       dev_err(dev, "%s: failed to register clock %s\n", __func__,

I guess the __func__ can be dropped, as the clock name is unique?

+                               periph_hw->periph.name);
+                       goto err_clk;
+               }
+
+               id = periph_hws[i].periph.id;
+               data->hw_data.hws[id] = hw;
+       }
+
+       return 0;
+
+err_clk:
+       while (i--)
+               mpfs_clk_unregister_periph(dev, data->hw_data.hws[periph_hws[i].periph.id]);
+
+       return PTR_ERR(hw);
+}
+
+static int mpfs_clk_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct mpfs_clock_data *clk_data;
+       struct resource *res;
+       int num_clks;

unsigned int

+       int ret;
+
+       num_clks = ARRAY_SIZE(mpfs_cfg_clks) + ARRAY_SIZE(mpfs_periph_clks);

As mpfs_periph_clks[] lacks an entry for CLK_RESERVED, num_clks is
one too small...

+
+       clk_data = devm_kzalloc(dev, struct_size(clk_data, hw_data.hws, num_clks), GFP_KERNEL);
+       if (!clk_data)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       clk_data->base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(clk_data->base))
+               return PTR_ERR(clk_data->base);
+
+       clk_data->hw_data.num = num_clks;
+
+       ret = mpfs_clk_register_cfgs(dev, mpfs_cfg_clks, ARRAY_SIZE(mpfs_cfg_clks), clk_data);
+       if (ret)
+               goto err_clk;
+
+       ret = mpfs_clk_register_periphs(dev, mpfs_periph_clks, ARRAY_SIZE(mpfs_periph_clks),
+                                       clk_data);

... and initializing CLK_CFM will write beyond the end of clk_data[].

+       if (ret)
+               goto err_clk;
+
+       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, &clk_data->hw_data);
+       if (ret)
+               goto err_clk;
+
+       dev_info(dev, "registered MPFS core clocks\n");
+       return ret;
+
+err_clk:
+       dev_err(dev, "failed to register MPFS core clocks\n");
+       return ret;
+}

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