From: Stephen Boyd <sboyd@kernel.org> To: Abel Vesa <abel.vesa@nxp.com>, Adam Ford <aford173@gmail.com>, Anson Huang <anson.huang@nxp.com>, Dong Aisheng <aisheng.dong@nxp.com>, Fabio Estevam <fabio.estevam@nxp.com>, Jacky Bai <ping.bai@nxp.com>, Lucas Stach <l.stach@pengutronix.de>, Marek Vasut <marek.vasut@gmail.com>, Mike Turquette <mturquette@baylibre.com>, Peng Fan <peng.fan@nxp.com>, Rob Herring <robh@kernel.org>, Sascha Hauer <kernel@pengutronix.de>, Shawn Guo <shawnguo@kernel.org> Cc: NXP Linux Team <linux-imx@nxp.com>, linux-arm-kernel@lists.infradead.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-clk@vger.kernel.org, devicetree@vger.kernel.org, Abel Vesa <abel.vesa@nxp.com> Subject: Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver Date: Wed, 04 Nov 2020 16:59:55 -0800 [thread overview] Message-ID: <160453799538.3965362.16773121624590810076@swboyd.mtv.corp.google.com> (raw) In-Reply-To: <1604402306-5348-11-git-send-email-abel.vesa@nxp.com> Quoting Abel Vesa (2020-11-03 03:18:22) > diff --git a/drivers/clk/imx/clk-blk-ctl.c b/drivers/clk/imx/clk-blk-ctl.c > new file mode 100644 > index 00000000..9ac0ed0 > --- /dev/null > +++ b/drivers/clk/imx/clk-blk-ctl.c > @@ -0,0 +1,302 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright 2020 NXP. > + */ > + > +#include <linux/clk.h> > +#include <linux/reset-controller.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/slab.h> > +#include <linux/string.h> > +#include <linux/types.h> > + > +#include "clk.h" > +#include "clk-blk-ctl.h" > + > +struct imx_reset_hw { > + u32 offset; > + u32 shift; > + u32 mask; > + unsigned long asserted; > +}; > + > +struct imx_pm_safekeep_info { > + uint32_t *regs_values; > + uint32_t *regs_offsets; Please use u32, it's shorter and kernel style. > + uint32_t regs_num; > +}; > + > +struct imx_blk_ctl_drvdata { > + void __iomem *base; > + struct reset_controller_dev rcdev; > + struct imx_reset_hw *rst_hws; > + struct imx_pm_safekeep_info pm_info; > + > + spinlock_t lock; > +}; > + > +static void __maybe_unused imx_blk_ctl_read_write(struct device *dev, > + bool write) > +{ > + struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev); > + struct imx_pm_safekeep_info *pm_info = &drvdata->pm_info; > + void __iomem *base = drvdata->base; > + int i; > + > + if (!pm_info->regs_num) > + return; > + > + for (i = 0; i < pm_info->regs_num; i++) { > + u32 offset = pm_info->regs_offsets[i]; > + > + if (write) > + writel(pm_info->regs_values[i], base + offset); > + else > + pm_info->regs_values[i] = readl(base + offset); > + } > + > +} > + > +static int __maybe_unused imx_blk_ctl_runtime_suspend(struct device *dev) > +{ > + imx_blk_ctl_read_write(dev, false); > + > + return 0; > +} > + > +static int __maybe_unused imx_blk_ctl_runtime_resume(struct device *dev) > +{ > + imx_blk_ctl_read_write(dev, true); > + > + return 0; > +} > + > +const struct dev_pm_ops imx_blk_ctl_pm_ops = { > + SET_RUNTIME_PM_OPS(imx_blk_ctl_runtime_suspend, > + imx_blk_ctl_runtime_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > +}; > +EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops); > + > +static int imx_blk_ctl_reset_set(struct reset_controller_dev *rcdev, > + unsigned long id, bool assert) > +{ > + struct imx_blk_ctl_drvdata *drvdata = container_of(rcdev, > + struct imx_blk_ctl_drvdata, rcdev); > + unsigned int offset = drvdata->rst_hws[id].offset; > + unsigned int shift = drvdata->rst_hws[id].shift; > + unsigned int mask = drvdata->rst_hws[id].mask; > + void __iomem *reg_addr = drvdata->base + offset; > + unsigned long flags; > + u32 reg; > + > + if (!assert && !test_bit(1, &drvdata->rst_hws[id].asserted)) > + return -ENODEV; > + > + if (assert && !test_and_set_bit(1, &drvdata->rst_hws[id].asserted)) We are testing the same bit atomically, twice. At a quick glance this code looks broken. But then looking closer it's being multiplexed. Can we move these assert/!assert checks to the caller? It will make it easier to reason about. Is the atomic-ness of this code important? If not, it should use __test_bit() and __test_and_set_bit() because locking is being handled somewhere higher up. Furthermore, why are we using a per-reset bitmap to set one bit? Either combine them into one bitmap or have a simple bool please. > + pm_runtime_get_sync(rcdev->dev); > + > + spin_lock_irqsave(&drvdata->lock, flags); > + > + reg = readl(reg_addr); > + if (assert) > + writel(reg & ~(mask << shift), reg_addr); > + else > + writel(reg | (mask << shift), reg_addr); > + > + spin_unlock_irqrestore(&drvdata->lock, flags); > + > + if (!assert && test_and_clear_bit(1, &drvdata->rst_hws[id].asserted)) > + pm_runtime_put(rcdev->dev); > + Same comment here. > + return 0; > +} > + > +static int imx_blk_ctl_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return imx_blk_ctl_reset_set(rcdev, id, true); > +} > + > +static int imx_blk_ctl_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return imx_blk_ctl_reset_set(rcdev, id, false); > +} > + > +static const struct reset_control_ops imx_blk_ctl_reset_ops = { > + .assert = imx_blk_ctl_reset_assert, > + .deassert = imx_blk_ctl_reset_deassert, > +}; > + > +static int imx_blk_ctl_register_reset_controller(struct device *dev) > +{ > + const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev); > + struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev); > + int max = dev_data->resets_max; > + struct imx_reset_hw *hws; > + int i; > + > + spin_lock_init(&drvdata->lock); > + > + drvdata->rcdev.owner = THIS_MODULE; > + drvdata->rcdev.nr_resets = max; > + drvdata->rcdev.ops = &imx_blk_ctl_reset_ops; > + drvdata->rcdev.of_node = dev->of_node; > + drvdata->rcdev.dev = dev; > + > + drvdata->rst_hws = devm_kcalloc(dev, max, sizeof(struct imx_reset_hw), > + GFP_KERNEL); > + hws = drvdata->rst_hws; > + > + for (i = 0; i < dev_data->hws_num; i++) { > + struct imx_blk_ctl_hw *hw = &dev_data->hws[i]; > + > + if (hw->type != BLK_CTL_RESET) > + continue; > + > + hws[hw->id].offset = hw->offset; > + hws[hw->id].shift = hw->shift; > + hws[hw->id].mask = hw->mask; > + } > + > + return devm_reset_controller_register(dev, &drvdata->rcdev); > +} Nitpick: Add a newline here please > +static struct clk_hw *imx_blk_ctl_register_one_clock(struct device *dev, > + struct imx_blk_ctl_hw *hw) > +{ > + struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev); > + void __iomem *base = drvdata->base; > + struct clk_hw *clk_hw = NULL; Is this necessary? Please add a default to the case if so and drop this assignment. But if all hw->type are covered in the case statement then this assignment can be dropped and nothing else. The compiler should tell us we forgot to handle a case. > + > + switch (hw->type) { > + case BLK_CTL_CLK_MUX: > + clk_hw = imx_dev_clk_hw_mux_flags(dev, hw->name, > + base + hw->offset, > + hw->shift, hw->width, > + hw->parents, > + hw->parents_count, > + hw->flags); > + break; > + case BLK_CTL_CLK_GATE: > + clk_hw = imx_dev_clk_hw_gate(dev, hw->name, hw->parents, > + base + hw->offset, hw->shift); > + break; > + case BLK_CTL_CLK_SHARED_GATE: > + clk_hw = imx_dev_clk_hw_gate_shared(dev, hw->name, > + hw->parents, > + base + hw->offset, > + hw->shift, > + hw->shared_count); > + break; > + case BLK_CTL_CLK_PLL14XX: > + clk_hw = imx_dev_clk_hw_pll14xx(dev, hw->name, hw->parents, > + base + hw->offset, hw->pll_tbl); > + break; > + }; > + > + return clk_hw; > +} > + > +static int imx_blk_ctl_register_clock_controller(struct device *dev) > +{ > + const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev); > + struct clk_hw_onecell_data *clk_hw_data; > + struct clk_hw **hws; > + int i; > + > + clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws, > + dev_data->hws_num), GFP_KERNEL); > + if (WARN_ON(!clk_hw_data)) We can drop the WARN_ON() call, slab allocations already print a stacktrace. > + return -ENOMEM; > + > + clk_hw_data->num = dev_data->clocks_max; > + hws = clk_hw_data->hws; > + > + for (i = 0; i < dev_data->hws_num; i++) { > + struct imx_blk_ctl_hw *hw = &dev_data->hws[i]; > + Add a comment like /* Resets aren't a clk */ > + if (hw->type == BLK_CTL_RESET) > + continue; > + > + hws[hw->id] = imx_blk_ctl_register_one_clock(dev, hw); > + } > + > + imx_check_clk_hws(hws, dev_data->clocks_max); > + > + return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, > + clk_hw_data); > +} > + > +static int imx_blk_ctl_init_runtime_pm_safekeeping(struct device *dev) > +{ > + const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev); > + struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev); > + struct imx_pm_safekeep_info *pm_info = &drvdata->pm_info; > + u32 regs_num = dev_data->pm_runtime_saved_regs_num; > + const u32 *regs_offsets = dev_data->pm_runtime_saved_regs; > + > + if (!dev_data->pm_runtime_saved_regs_num) > + return 0; > + > + pm_info->regs_values = devm_kzalloc(dev, > + sizeof(u32) * regs_num, > + GFP_KERNEL); > + if (WARN_ON(IS_ERR(pm_info->regs_values))) Please drop WARN_ON > + return PTR_ERR(pm_info->regs_values); > + > + pm_info->regs_offsets = kmemdup(regs_offsets, > + regs_num * sizeof(u32), GFP_KERNEL); > + if (WARN_ON(IS_ERR(pm_info->regs_offsets))) Please drop WARN_ON > + return PTR_ERR(pm_info->regs_offsets); > + > + pm_info->regs_num = regs_num; > + > + return 0; > +} > + > +int imx_blk_ctl_register(struct platform_device *pdev) > +{ > + struct imx_blk_ctl_drvdata *drvdata; > + struct device *dev = &pdev->dev; > + int ret; > + > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > + if (WARN_ON(!drvdata)) Please drop WARN_ON > + return -ENOMEM; > + > + drvdata->base = devm_platform_ioremap_resource(pdev, 0); > + if (WARN_ON(IS_ERR(drvdata->base))) Please drop WARN_ON > + return PTR_ERR(drvdata->base); > + > + dev_set_drvdata(dev, drvdata); > + > + ret = imx_blk_ctl_init_runtime_pm_safekeeping(dev); > + if (ret) > + return ret; > + > + pm_runtime_get_noresume(dev); > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + > + ret = imx_blk_ctl_register_clock_controller(dev); > + if (ret) { > + pm_runtime_put(dev); > + return ret; > + } > + > + ret = imx_blk_ctl_register_reset_controller(dev); > + > + pm_runtime_put(dev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(imx_blk_ctl_register); > diff --git a/drivers/clk/imx/clk-blk-ctl.h b/drivers/clk/imx/clk-blk-ctl.h > new file mode 100644 > index 00000000..3f14a47 > --- /dev/null > +++ b/drivers/clk/imx/clk-blk-ctl.h > @@ -0,0 +1,80 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __MACH_IMX_CLK_BLK_CTL_H > +#define __MACH_IMX_CLK_BLK_CTL_H > + > +enum imx_blk_ctl_hw_type { > + BLK_CTL_CLK_MUX, > + BLK_CTL_CLK_GATE, > + BLK_CTL_CLK_SHARED_GATE, > + BLK_CTL_CLK_PLL14XX, > + BLK_CTL_RESET, > +}; > + > +struct imx_blk_ctl_hw { > + int type; > + char *name; > + u32 offset; > + u32 shift; > + u32 mask; > + u32 width; > + u32 flags; > + u32 id; > + const void *parents; > + u32 parents_count; > + int *shared_count; > + const struct imx_pll14xx_clk *pll_tbl; Please forward declare pointer types. > +}; > + > +struct imx_blk_ctl_dev_data { > + struct imx_blk_ctl_hw *hws; Please forward declare pointer types. > + u32 hws_num; > + > + u32 clocks_max; > + u32 resets_max; > + > + u32 pm_runtime_saved_regs_num; > + u32 pm_runtime_saved_regs[]; > +}; > + > +#define IMX_BLK_CTL(_type, _name, _id, _offset, _shift, _width, _mask, _parents, _parents_count, _flags, sh_count, _pll_tbl) \ > + { \ > + .type = _type, \ > + .name = _name, \ > + .id = _id, \ > + .offset = _offset, \ > + .shift = _shift, \ > + .width = _width, \ > + .mask = _mask, \ > + .parents = _parents, \ > + .parents_count = _parents_count, \ > + .flags = _flags, \ > + .shared_count = sh_count, \ > + .pll_tbl = _pll_tbl, \ > + } > + > +#define IMX_BLK_CTL_CLK_MUX(_name, _id, _offset, _shift, _width, _parents) \ > + IMX_BLK_CTL(BLK_CTL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), 0, NULL, NULL) > + > +#define IMX_BLK_CTL_CLK_MUX_FLAGS(_name, _id, _offset, _shift, _width, _parents, _flags) \ > + IMX_BLK_CTL(BLK_CTL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), _flags, NULL, NULL) > + > +#define IMX_BLK_CTL_CLK_GATE(_name, _id, _offset, _shift, _parents) \ > + IMX_BLK_CTL(BLK_CTL_CLK_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, NULL, NULL) > + > +#define IMX_BLK_CTL_CLK_SHARED_GATE(_name, _id, _offset, _shift, _parents, sh_count) \ > + IMX_BLK_CTL(BLK_CTL_CLK_SHARED_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, sh_count, NULL) > + > +#define IMX_BLK_CTL_CLK_PLL14XX(_name, _id, _offset, _parents, _pll_tbl) \ > + IMX_BLK_CTL(BLK_CTL_CLK_PLL14XX, _name, _id, _offset, 0, 0, 0, _parents, 1, 0, NULL, _pll_tbl) > + > +#define IMX_BLK_CTL_RESET(_id, _offset, _shift) \ > + IMX_BLK_CTL(BLK_CTL_RESET, NULL, _id, _offset, _shift, 0, 1, NULL, 0, 0, NULL, NULL) > + > +#define IMX_BLK_CTL_RESET_MASK(_id, _offset, _shift, mask) \ > + IMX_BLK_CTL(BLK_CTL_RESET, NULL, _id, _offset, _shift, 0, mask, NULL, 0, 0, NULL, NULL) > + > +extern const struct dev_pm_ops imx_blk_ctl_pm_ops; > + > +int imx_blk_ctl_register(struct platform_device *pdev); Please forward declare pointer types. > + > +#endif > -- > 2.7.4 >
WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <sboyd@kernel.org> To: Abel Vesa <abel.vesa@nxp.com>, Adam Ford <aford173@gmail.com>, Anson Huang <anson.huang@nxp.com>, Dong Aisheng <aisheng.dong@nxp.com>, Fabio Estevam <fabio.estevam@nxp.com>, Jacky Bai <ping.bai@nxp.com>, Lucas Stach <l.stach@pengutronix.de>, Marek Vasut <marek.vasut@gmail.com>, Mike Turquette <mturquette@baylibre.com>, Peng Fan <peng.fan@nxp.com>, Rob Herring <robh@kernel.org>, Sascha Hauer <kernel@pengutronix.de>, Shawn Guo <shawnguo@kernel.org> Cc: devicetree@vger.kernel.org, Abel Vesa <abel.vesa@nxp.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, NXP Linux Team <linux-imx@nxp.com>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver Date: Wed, 04 Nov 2020 16:59:55 -0800 [thread overview] Message-ID: <160453799538.3965362.16773121624590810076@swboyd.mtv.corp.google.com> (raw) In-Reply-To: <1604402306-5348-11-git-send-email-abel.vesa@nxp.com> Quoting Abel Vesa (2020-11-03 03:18:22) > diff --git a/drivers/clk/imx/clk-blk-ctl.c b/drivers/clk/imx/clk-blk-ctl.c > new file mode 100644 > index 00000000..9ac0ed0 > --- /dev/null > +++ b/drivers/clk/imx/clk-blk-ctl.c > @@ -0,0 +1,302 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright 2020 NXP. > + */ > + > +#include <linux/clk.h> > +#include <linux/reset-controller.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/slab.h> > +#include <linux/string.h> > +#include <linux/types.h> > + > +#include "clk.h" > +#include "clk-blk-ctl.h" > + > +struct imx_reset_hw { > + u32 offset; > + u32 shift; > + u32 mask; > + unsigned long asserted; > +}; > + > +struct imx_pm_safekeep_info { > + uint32_t *regs_values; > + uint32_t *regs_offsets; Please use u32, it's shorter and kernel style. > + uint32_t regs_num; > +}; > + > +struct imx_blk_ctl_drvdata { > + void __iomem *base; > + struct reset_controller_dev rcdev; > + struct imx_reset_hw *rst_hws; > + struct imx_pm_safekeep_info pm_info; > + > + spinlock_t lock; > +}; > + > +static void __maybe_unused imx_blk_ctl_read_write(struct device *dev, > + bool write) > +{ > + struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev); > + struct imx_pm_safekeep_info *pm_info = &drvdata->pm_info; > + void __iomem *base = drvdata->base; > + int i; > + > + if (!pm_info->regs_num) > + return; > + > + for (i = 0; i < pm_info->regs_num; i++) { > + u32 offset = pm_info->regs_offsets[i]; > + > + if (write) > + writel(pm_info->regs_values[i], base + offset); > + else > + pm_info->regs_values[i] = readl(base + offset); > + } > + > +} > + > +static int __maybe_unused imx_blk_ctl_runtime_suspend(struct device *dev) > +{ > + imx_blk_ctl_read_write(dev, false); > + > + return 0; > +} > + > +static int __maybe_unused imx_blk_ctl_runtime_resume(struct device *dev) > +{ > + imx_blk_ctl_read_write(dev, true); > + > + return 0; > +} > + > +const struct dev_pm_ops imx_blk_ctl_pm_ops = { > + SET_RUNTIME_PM_OPS(imx_blk_ctl_runtime_suspend, > + imx_blk_ctl_runtime_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > +}; > +EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops); > + > +static int imx_blk_ctl_reset_set(struct reset_controller_dev *rcdev, > + unsigned long id, bool assert) > +{ > + struct imx_blk_ctl_drvdata *drvdata = container_of(rcdev, > + struct imx_blk_ctl_drvdata, rcdev); > + unsigned int offset = drvdata->rst_hws[id].offset; > + unsigned int shift = drvdata->rst_hws[id].shift; > + unsigned int mask = drvdata->rst_hws[id].mask; > + void __iomem *reg_addr = drvdata->base + offset; > + unsigned long flags; > + u32 reg; > + > + if (!assert && !test_bit(1, &drvdata->rst_hws[id].asserted)) > + return -ENODEV; > + > + if (assert && !test_and_set_bit(1, &drvdata->rst_hws[id].asserted)) We are testing the same bit atomically, twice. At a quick glance this code looks broken. But then looking closer it's being multiplexed. Can we move these assert/!assert checks to the caller? It will make it easier to reason about. Is the atomic-ness of this code important? If not, it should use __test_bit() and __test_and_set_bit() because locking is being handled somewhere higher up. Furthermore, why are we using a per-reset bitmap to set one bit? Either combine them into one bitmap or have a simple bool please. > + pm_runtime_get_sync(rcdev->dev); > + > + spin_lock_irqsave(&drvdata->lock, flags); > + > + reg = readl(reg_addr); > + if (assert) > + writel(reg & ~(mask << shift), reg_addr); > + else > + writel(reg | (mask << shift), reg_addr); > + > + spin_unlock_irqrestore(&drvdata->lock, flags); > + > + if (!assert && test_and_clear_bit(1, &drvdata->rst_hws[id].asserted)) > + pm_runtime_put(rcdev->dev); > + Same comment here. > + return 0; > +} > + > +static int imx_blk_ctl_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return imx_blk_ctl_reset_set(rcdev, id, true); > +} > + > +static int imx_blk_ctl_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return imx_blk_ctl_reset_set(rcdev, id, false); > +} > + > +static const struct reset_control_ops imx_blk_ctl_reset_ops = { > + .assert = imx_blk_ctl_reset_assert, > + .deassert = imx_blk_ctl_reset_deassert, > +}; > + > +static int imx_blk_ctl_register_reset_controller(struct device *dev) > +{ > + const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev); > + struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev); > + int max = dev_data->resets_max; > + struct imx_reset_hw *hws; > + int i; > + > + spin_lock_init(&drvdata->lock); > + > + drvdata->rcdev.owner = THIS_MODULE; > + drvdata->rcdev.nr_resets = max; > + drvdata->rcdev.ops = &imx_blk_ctl_reset_ops; > + drvdata->rcdev.of_node = dev->of_node; > + drvdata->rcdev.dev = dev; > + > + drvdata->rst_hws = devm_kcalloc(dev, max, sizeof(struct imx_reset_hw), > + GFP_KERNEL); > + hws = drvdata->rst_hws; > + > + for (i = 0; i < dev_data->hws_num; i++) { > + struct imx_blk_ctl_hw *hw = &dev_data->hws[i]; > + > + if (hw->type != BLK_CTL_RESET) > + continue; > + > + hws[hw->id].offset = hw->offset; > + hws[hw->id].shift = hw->shift; > + hws[hw->id].mask = hw->mask; > + } > + > + return devm_reset_controller_register(dev, &drvdata->rcdev); > +} Nitpick: Add a newline here please > +static struct clk_hw *imx_blk_ctl_register_one_clock(struct device *dev, > + struct imx_blk_ctl_hw *hw) > +{ > + struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev); > + void __iomem *base = drvdata->base; > + struct clk_hw *clk_hw = NULL; Is this necessary? Please add a default to the case if so and drop this assignment. But if all hw->type are covered in the case statement then this assignment can be dropped and nothing else. The compiler should tell us we forgot to handle a case. > + > + switch (hw->type) { > + case BLK_CTL_CLK_MUX: > + clk_hw = imx_dev_clk_hw_mux_flags(dev, hw->name, > + base + hw->offset, > + hw->shift, hw->width, > + hw->parents, > + hw->parents_count, > + hw->flags); > + break; > + case BLK_CTL_CLK_GATE: > + clk_hw = imx_dev_clk_hw_gate(dev, hw->name, hw->parents, > + base + hw->offset, hw->shift); > + break; > + case BLK_CTL_CLK_SHARED_GATE: > + clk_hw = imx_dev_clk_hw_gate_shared(dev, hw->name, > + hw->parents, > + base + hw->offset, > + hw->shift, > + hw->shared_count); > + break; > + case BLK_CTL_CLK_PLL14XX: > + clk_hw = imx_dev_clk_hw_pll14xx(dev, hw->name, hw->parents, > + base + hw->offset, hw->pll_tbl); > + break; > + }; > + > + return clk_hw; > +} > + > +static int imx_blk_ctl_register_clock_controller(struct device *dev) > +{ > + const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev); > + struct clk_hw_onecell_data *clk_hw_data; > + struct clk_hw **hws; > + int i; > + > + clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws, > + dev_data->hws_num), GFP_KERNEL); > + if (WARN_ON(!clk_hw_data)) We can drop the WARN_ON() call, slab allocations already print a stacktrace. > + return -ENOMEM; > + > + clk_hw_data->num = dev_data->clocks_max; > + hws = clk_hw_data->hws; > + > + for (i = 0; i < dev_data->hws_num; i++) { > + struct imx_blk_ctl_hw *hw = &dev_data->hws[i]; > + Add a comment like /* Resets aren't a clk */ > + if (hw->type == BLK_CTL_RESET) > + continue; > + > + hws[hw->id] = imx_blk_ctl_register_one_clock(dev, hw); > + } > + > + imx_check_clk_hws(hws, dev_data->clocks_max); > + > + return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, > + clk_hw_data); > +} > + > +static int imx_blk_ctl_init_runtime_pm_safekeeping(struct device *dev) > +{ > + const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev); > + struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev); > + struct imx_pm_safekeep_info *pm_info = &drvdata->pm_info; > + u32 regs_num = dev_data->pm_runtime_saved_regs_num; > + const u32 *regs_offsets = dev_data->pm_runtime_saved_regs; > + > + if (!dev_data->pm_runtime_saved_regs_num) > + return 0; > + > + pm_info->regs_values = devm_kzalloc(dev, > + sizeof(u32) * regs_num, > + GFP_KERNEL); > + if (WARN_ON(IS_ERR(pm_info->regs_values))) Please drop WARN_ON > + return PTR_ERR(pm_info->regs_values); > + > + pm_info->regs_offsets = kmemdup(regs_offsets, > + regs_num * sizeof(u32), GFP_KERNEL); > + if (WARN_ON(IS_ERR(pm_info->regs_offsets))) Please drop WARN_ON > + return PTR_ERR(pm_info->regs_offsets); > + > + pm_info->regs_num = regs_num; > + > + return 0; > +} > + > +int imx_blk_ctl_register(struct platform_device *pdev) > +{ > + struct imx_blk_ctl_drvdata *drvdata; > + struct device *dev = &pdev->dev; > + int ret; > + > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > + if (WARN_ON(!drvdata)) Please drop WARN_ON > + return -ENOMEM; > + > + drvdata->base = devm_platform_ioremap_resource(pdev, 0); > + if (WARN_ON(IS_ERR(drvdata->base))) Please drop WARN_ON > + return PTR_ERR(drvdata->base); > + > + dev_set_drvdata(dev, drvdata); > + > + ret = imx_blk_ctl_init_runtime_pm_safekeeping(dev); > + if (ret) > + return ret; > + > + pm_runtime_get_noresume(dev); > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + > + ret = imx_blk_ctl_register_clock_controller(dev); > + if (ret) { > + pm_runtime_put(dev); > + return ret; > + } > + > + ret = imx_blk_ctl_register_reset_controller(dev); > + > + pm_runtime_put(dev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(imx_blk_ctl_register); > diff --git a/drivers/clk/imx/clk-blk-ctl.h b/drivers/clk/imx/clk-blk-ctl.h > new file mode 100644 > index 00000000..3f14a47 > --- /dev/null > +++ b/drivers/clk/imx/clk-blk-ctl.h > @@ -0,0 +1,80 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __MACH_IMX_CLK_BLK_CTL_H > +#define __MACH_IMX_CLK_BLK_CTL_H > + > +enum imx_blk_ctl_hw_type { > + BLK_CTL_CLK_MUX, > + BLK_CTL_CLK_GATE, > + BLK_CTL_CLK_SHARED_GATE, > + BLK_CTL_CLK_PLL14XX, > + BLK_CTL_RESET, > +}; > + > +struct imx_blk_ctl_hw { > + int type; > + char *name; > + u32 offset; > + u32 shift; > + u32 mask; > + u32 width; > + u32 flags; > + u32 id; > + const void *parents; > + u32 parents_count; > + int *shared_count; > + const struct imx_pll14xx_clk *pll_tbl; Please forward declare pointer types. > +}; > + > +struct imx_blk_ctl_dev_data { > + struct imx_blk_ctl_hw *hws; Please forward declare pointer types. > + u32 hws_num; > + > + u32 clocks_max; > + u32 resets_max; > + > + u32 pm_runtime_saved_regs_num; > + u32 pm_runtime_saved_regs[]; > +}; > + > +#define IMX_BLK_CTL(_type, _name, _id, _offset, _shift, _width, _mask, _parents, _parents_count, _flags, sh_count, _pll_tbl) \ > + { \ > + .type = _type, \ > + .name = _name, \ > + .id = _id, \ > + .offset = _offset, \ > + .shift = _shift, \ > + .width = _width, \ > + .mask = _mask, \ > + .parents = _parents, \ > + .parents_count = _parents_count, \ > + .flags = _flags, \ > + .shared_count = sh_count, \ > + .pll_tbl = _pll_tbl, \ > + } > + > +#define IMX_BLK_CTL_CLK_MUX(_name, _id, _offset, _shift, _width, _parents) \ > + IMX_BLK_CTL(BLK_CTL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), 0, NULL, NULL) > + > +#define IMX_BLK_CTL_CLK_MUX_FLAGS(_name, _id, _offset, _shift, _width, _parents, _flags) \ > + IMX_BLK_CTL(BLK_CTL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), _flags, NULL, NULL) > + > +#define IMX_BLK_CTL_CLK_GATE(_name, _id, _offset, _shift, _parents) \ > + IMX_BLK_CTL(BLK_CTL_CLK_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, NULL, NULL) > + > +#define IMX_BLK_CTL_CLK_SHARED_GATE(_name, _id, _offset, _shift, _parents, sh_count) \ > + IMX_BLK_CTL(BLK_CTL_CLK_SHARED_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, sh_count, NULL) > + > +#define IMX_BLK_CTL_CLK_PLL14XX(_name, _id, _offset, _parents, _pll_tbl) \ > + IMX_BLK_CTL(BLK_CTL_CLK_PLL14XX, _name, _id, _offset, 0, 0, 0, _parents, 1, 0, NULL, _pll_tbl) > + > +#define IMX_BLK_CTL_RESET(_id, _offset, _shift) \ > + IMX_BLK_CTL(BLK_CTL_RESET, NULL, _id, _offset, _shift, 0, 1, NULL, 0, 0, NULL, NULL) > + > +#define IMX_BLK_CTL_RESET_MASK(_id, _offset, _shift, mask) \ > + IMX_BLK_CTL(BLK_CTL_RESET, NULL, _id, _offset, _shift, 0, mask, NULL, 0, 0, NULL, NULL) > + > +extern const struct dev_pm_ops imx_blk_ctl_pm_ops; > + > +int imx_blk_ctl_register(struct platform_device *pdev); Please forward declare pointer types. > + > +#endif > -- > 2.7.4 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-11-05 1:00 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-03 11:18 [PATCH v5 00/14] Add BLK_CTL support for i.MX8MP Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-03 11:18 ` [PATCH v5 01/14] dt-bindings: clocks: imx8mp: Rename audiomix ids clocks to audio_blk_ctl Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:05 ` Stephen Boyd 2020-11-05 1:05 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 02/14] dt-bindings: reset: imx8mp: Add audio blk_ctl reset IDs Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:05 ` Stephen Boyd 2020-11-05 1:05 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 03/14] dt-bindings: clock: imx8mp: Add ids for the audio shared gate Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:05 ` Stephen Boyd 2020-11-05 1:05 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 04/14] dt-bindings: clock: imx8mp: Add media blk_ctl clock IDs Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:05 ` Stephen Boyd 2020-11-05 1:05 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 05/14] dt-bindings: reset: imx8mp: Add media blk_ctl reset IDs Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:06 ` Stephen Boyd 2020-11-05 1:06 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 06/14] dt-bindings: clock: imx8mp: Add hdmi blk_ctl clock IDs Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:06 ` Stephen Boyd 2020-11-05 1:06 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 07/14] dt-bindings: reset: imx8mp: Add hdmi blk_ctl reset IDs Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:06 ` Stephen Boyd 2020-11-05 1:06 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 08/14] clk: imx8mp: Add audio shared gate Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:07 ` Stephen Boyd 2020-11-05 1:07 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 09/14] Documentation: bindings: clk: Add bindings for i.MX BLK_CTL Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-04 19:01 ` Rob Herring 2020-11-04 19:01 ` Rob Herring 2020-11-05 1:08 ` Stephen Boyd 2020-11-05 1:08 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 0:59 ` Stephen Boyd [this message] 2020-11-05 0:59 ` Stephen Boyd 2020-11-09 5:45 ` Jacky Bai 2020-11-09 5:45 ` Jacky Bai 2020-11-11 9:13 ` Dong Aisheng 2020-11-11 9:13 ` Dong Aisheng 2020-11-17 14:48 ` Abel Vesa 2020-11-17 14:48 ` Abel Vesa 2021-02-25 8:23 ` Frieder Schrempf 2021-02-25 8:23 ` Frieder Schrempf 2021-02-25 8:27 ` Jacky Bai 2021-02-25 8:27 ` Jacky Bai 2021-03-18 19:58 ` Tim Harvey 2021-03-18 19:58 ` Tim Harvey 2020-11-03 11:18 ` [PATCH v5 11/14] clk: imx: Add blk-ctl driver for i.MX8MP Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-05 1:09 ` Stephen Boyd 2020-11-05 1:09 ` Stephen Boyd 2020-11-03 11:18 ` [PATCH v5 12/14] arm64: dts: imx8mp: Add audio_blk_ctl node Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-03 11:18 ` [PATCH v5 13/14] arm64: dts: imx8mp: Add media_blk_ctl node Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2020-11-03 11:18 ` [PATCH v5 14/14] arm64: dts: imx8mp: Add hdmi_blk_ctl node Abel Vesa 2020-11-03 11:18 ` Abel Vesa 2021-03-03 10:47 ` [PATCH v5 00/14] Add BLK_CTL support for i.MX8MP Abel Vesa 2021-03-03 10:47 ` Abel Vesa 2021-03-03 10:54 ` Marek Vasut 2021-03-03 10:54 ` Marek Vasut 2021-03-22 23:49 ` Adam Ford 2021-03-22 23:49 ` Adam Ford
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=160453799538.3965362.16773121624590810076@swboyd.mtv.corp.google.com \ --to=sboyd@kernel.org \ --cc=abel.vesa@nxp.com \ --cc=aford173@gmail.com \ --cc=aisheng.dong@nxp.com \ --cc=anson.huang@nxp.com \ --cc=devicetree@vger.kernel.org \ --cc=fabio.estevam@nxp.com \ --cc=kernel@pengutronix.de \ --cc=l.stach@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=marek.vasut@gmail.com \ --cc=mturquette@baylibre.com \ --cc=peng.fan@nxp.com \ --cc=ping.bai@nxp.com \ --cc=robh@kernel.org \ --cc=shawnguo@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.