Hi Conor, Daire,
On Tue, Nov 30, 2021 at 3:06 PM <conor.dooley@xxxxxxxxxxxxx> wrote:
> From: Daire McNamara <daire.mcnamara@xxxxxxxxxxxxx>
>
> Add support for clock configuration on Microchip PolarFire SoC
>
> Signed-off-by: Daire McNamara <daire.mcnamara@xxxxxxxxxxxxx>
> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
> + * 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@xxxxxxxxxxxxxx
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
