Re: [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Conor, Daire,

On Tue, Nov 30, 2021 at 3:06 PM <conor.dooley@microchip.com> 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!

> --- /dev/null
> +++ b/drivers/clk/microchip/clk-mpfs.c
> @@ -0,0 +1,444 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Daire McNamara,<daire.mcnamara@microchip.com>
> + * Copyright (C) 2020 Microchip Technology Inc.  All rights reserved.
> + */
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <dt-bindings/clock/microchip,mpfs-clock.h>
> +
> +/* address offset of control registers */
> +#define REG_CLOCK_CONFIG_CR    0x08u
> +#define REG_SUBBLK_CLOCK_CR    0x84u
> +#define REG_SUBBLK_RESET_CR    0x88u
> +
> +struct mpfs_clock_data {
> +       void __iomem *base;
> +       struct clk_hw_onecell_data hw_data;
> +};
> +
> +struct mpfs_cfg_clock {
> +       unsigned int id;

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