diff mbox series

[v2,11/17] clk: imx: Add blk_ctrl combo driver

Message ID 1597406966-13740-12-git-send-email-abel.vesa@nxp.com (mailing list archive)
State New, archived
Headers show
Series Add BLK_CTRL support for i.MX8MP | expand

Commit Message

Abel Vesa Aug. 14, 2020, 12:09 p.m. UTC 
On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
RM and usually is comprised of some GPRs that are considered too
generic to be part of any dedicated IP from that specific subsystem.

In general, some of the GPRs have some clock bits, some have reset bits,
so in order to be able to use the imx clock API, this needs to be
in a clock driver. From there it can use the reset controller API and
leave the rest to the syscon.

This driver is intended to work with the following BLK_CTRL IPs found in
i.MX8MP (but it might be reused by the future i.MX platforms that
have this kind of IP in their design):
 - Audio
 - Media
 - HDMI

Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
---
 drivers/clk/imx/Makefile       |   2 +-
 drivers/clk/imx/clk-blk-ctrl.c | 327 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/imx/clk-blk-ctrl.h |  81 ++++++++++
 3 files changed, 409 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/imx/clk-blk-ctrl.c
 create mode 100644 drivers/clk/imx/clk-blk-ctrl.h

Comments

Dong Aisheng Aug. 18, 2020, 11:27 a.m. UTC | #1 
On Fri, Aug 14, 2020 at 8:12 PM Abel Vesa <abel.vesa@nxp.com> wrote:
>
> On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
> RM and usually is comprised of some GPRs that are considered too
> generic to be part of any dedicated IP from that specific subsystem.
>
> In general, some of the GPRs have some clock bits, some have reset bits,
> so in order to be able to use the imx clock API, this needs to be
> in a clock driver. From there it can use the reset controller API and
> leave the rest to the syscon.
>
> This driver is intended to work with the following BLK_CTRL IPs found in
> i.MX8MP (but it might be reused by the future i.MX platforms that
> have this kind of IP in their design):
>  - Audio
>  - Media
>  - HDMI
>
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>

The code mostly looks good to me.
Only a few minor comments.

> ---
>  drivers/clk/imx/Makefile       |   2 +-
>  drivers/clk/imx/clk-blk-ctrl.c | 327 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/imx/clk-blk-ctrl.h |  81 ++++++++++
>  3 files changed, 409 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/imx/clk-blk-ctrl.c
>  create mode 100644 drivers/clk/imx/clk-blk-ctrl.h
>
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 928f874c..7afe1df 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -27,7 +27,7 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
>
>  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
>  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o
>  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
>  obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
>
> diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
> new file mode 100644
> index 00000000..1672646
> --- /dev/null
> +++ b/drivers/clk/imx/clk-blk-ctrl.c
> @@ -0,0 +1,327 @@
> +// 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-ctrl.h"
> +
> +struct reset_hw {

s/reset_hw/imx_reset_hw
It helps the reader to quickly understand it's not core structure

> +       u32 offset;
> +       u32 shift;
> +       u32 mask;
> +       bool asserted;
> +};
> +
> +struct pm_safekeep_info {

imx_pm_safekeep_info

> +       uint32_t *regs_values;
> +       uint32_t *regs_offsets;
> +       uint32_t regs_num;
> +};
> +
> +struct imx_blk_ctrl_drvdata {
> +       void __iomem *base;
> +       struct reset_controller_dev rcdev;
> +       struct reset_hw *rst_hws;
> +       struct pm_safekeep_info pm_info;
> +
> +       spinlock_t lock;
> +};
> +
> +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> +                                 unsigned long id, bool assert)
> +{
> +       struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> +                       struct imx_blk_ctrl_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;
> +       unsigned int asserted_before = 0, asserted_after = 0;

swap above two lines from long to short

> +       u32 reg;
> +       int i;
> +
> +       spin_lock_irqsave(&drvdata->lock, flags);
> +
> +       for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> +               if (drvdata->rst_hws[i].asserted)
> +                       asserted_before++;
> +
> +       if (asserted_before == 0 && assert)
> +               pm_runtime_get(rcdev->dev);
> +
> +       if (assert) {
> +               reg = readl(reg_addr);
> +               writel(reg & ~(mask << shift), reg_addr);
> +               drvdata->rst_hws[id].asserted = true;
> +       } else {
> +               reg = readl(reg_addr);
> +               writel(reg | (mask << shift), reg_addr);
> +               drvdata->rst_hws[id].asserted = false;
> +       }
> +
> +       for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> +               if (drvdata->rst_hws[i].asserted)
> +                       asserted_after++;

I guess the logic may be able to be simplified.
For example, put assert ref count in the private structure.
Then
call pm_runtime_get when ref count is 0 and assert is true.
call pm_runtime_put when ref ount is 0 and assert is false.
No need to go through twice for loop each time.

> +
> +       if (asserted_before == 1 && asserted_after == 0)
> +               pm_runtime_put(rcdev->dev);
> +
> +       spin_unlock_irqrestore(&drvdata->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int imx_blk_ctrl_reset_assert(struct reset_controller_dev *rcdev,
> +                                          unsigned long id)
> +{
> +       return imx_blk_ctrl_reset_set(rcdev, id, true);
> +}
> +
> +static int imx_blk_ctrl_reset_deassert(struct reset_controller_dev *rcdev,
> +                                            unsigned long id)
> +{
> +       return imx_blk_ctrl_reset_set(rcdev, id, false);
> +}
> +
> +static const struct reset_control_ops imx_blk_ctrl_reset_ops = {
> +       .assert         = imx_blk_ctrl_reset_assert,
> +       .deassert       = imx_blk_ctrl_reset_deassert,
> +};
> +
> +static int imx_blk_ctrl_register_reset_controller(struct device *dev)
> +{
> +       struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> +       const struct imx_blk_ctrl_dev_data *dev_data = of_device_get_match_data(dev);
> +       struct reset_hw *hws;
> +       int max = dev_data->resets_max;

sort from long to short

> +       int i;
> +
> +       spin_lock_init(&drvdata->lock);
> +
> +       drvdata->rcdev.owner     = THIS_MODULE;
> +       drvdata->rcdev.nr_resets = max;
> +       drvdata->rcdev.ops       = &imx_blk_ctrl_reset_ops;
> +       drvdata->rcdev.of_node   = dev->of_node;
> +       drvdata->rcdev.dev       = dev;
> +
> +       drvdata->rst_hws = devm_kcalloc(dev, max, sizeof(struct reset_hw),
> +                                       GFP_KERNEL);
> +       hws = drvdata->rst_hws;
> +
> +       for (i = 0; i < dev_data->hws_num; i++) {
> +               struct imx_blk_ctrl_hw *hw = &dev_data->hws[i];
> +
> +               if (hw->type != BLK_CTRL_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);
> +}
> +static struct clk_hw *imx_blk_ctrl_register_one_clock(struct device *dev,
> +                                               struct imx_blk_ctrl_hw *hw)
> +{
> +       struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> +       void __iomem *base = drvdata->base;
> +       struct clk_hw *clk_hw;
> +
> +       switch (hw->type) {
> +       case BLK_CTRL_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_CTRL_CLK_GATE:
> +               clk_hw = imx_dev_clk_hw_gate(dev, hw->name, hw->parents,
> +                                            base + hw->offset, hw->shift);
> +               break;
> +       case BLK_CTRL_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_CTRL_CLK_PLL14XX:
> +               clk_hw = imx_dev_clk_hw_pll14xx(dev, hw->name, hw->parents,
> +                                               base + hw->offset, hw->pll_tbl);
> +               break;
> +       default:
> +               clk_hw = NULL;

A better way may be assign clk_hw default to NULL.
Then instead, we can add a WARN here in case the clk hw data is insane.

> +       };
> +
> +       return clk_hw;
> +}
> +
> +static int imx_blk_ctrl_register_clock_controller(struct device *dev)
> +{
> +       const struct imx_blk_ctrl_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))
> +               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_ctrl_hw *hw = &dev_data->hws[i];
> +               struct clk_hw *tmp = imx_blk_ctrl_register_one_clock(dev, hw);
> +
> +               if (!tmp)
> +                       continue;
> +               hws[hw->id] = tmp;

tmp could be a non NULL error pointer.
Maybe here could be simplied as:
hws[hw->id] = imx_blk_ctrl_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_ctrl_init_runtime_pm_safekeeping(struct device *dev)
> +{
> +       const struct imx_blk_ctrl_dev_data *dev_data = of_device_get_match_data(dev);
> +       struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> +       struct 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)))
> +               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)))
> +               return PTR_ERR(pm_info->regs_offsets);
> +
> +       pm_info->regs_num = regs_num;
> +
> +       return 0;
> +}
> +
> +static int imx_blk_ctrl_probe(struct platform_device *pdev)
> +{
> +       struct imx_blk_ctrl_drvdata *drvdata;
> +       struct device *dev = &pdev->dev;
> +       int ret;
> +
> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +       if (WARN_ON(!drvdata))
> +               return -ENOMEM;
> +
> +       drvdata->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (WARN_ON(IS_ERR(drvdata->base)))
> +               return PTR_ERR(drvdata->base);
> +
> +       dev_set_drvdata(dev, drvdata);
> +
> +       ret = imx_blk_ctrl_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_ctrl_register_clock_controller(dev);
> +       if (ret) {
> +               pm_runtime_put(dev);
> +               return ret;
> +       }
> +
> +       ret = imx_blk_ctrl_register_reset_controller(dev);
> +
> +       pm_runtime_put(dev);
> +
> +       return ret;
> +}
> +
> +static void imx_blk_ctrl_read_write(struct device *dev, bool write)

__maybe_unused

> +{
> +       struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> +       struct 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 imx_blk_ctrl_runtime_suspend(struct device *dev)

__maybe_unused

> +{
> +       imx_blk_ctrl_read_write(dev, false);
> +
> +       return 0;
> +}
> +
> +static int imx_blk_ctrl_runtime_resume(struct device *dev)

__maybe_unused

> +{
> +       imx_blk_ctrl_read_write(dev, true);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops imx_blk_ctrl_pm_ops = {
> +       SET_RUNTIME_PM_OPS(imx_blk_ctrl_runtime_suspend,
> +                          imx_blk_ctrl_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                          pm_runtime_force_resume)
> +};
> +
> +static const struct of_device_id imx_blk_ctrl_of_match[] = {
> +       { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_blk_ctrl_of_match);
> +
> +static struct platform_driver imx_blk_ctrl_driver = {
> +       .probe = imx_blk_ctrl_probe,
> +       .driver = {
> +               .name = "imx-blk-ctrl",
> +               .of_match_table = of_match_ptr(imx_blk_ctrl_of_match),
> +               .pm = &imx_blk_ctrl_pm_ops,
> +       },
> +};
> +module_platform_driver(imx_blk_ctrl_driver);
> diff --git a/drivers/clk/imx/clk-blk-ctrl.h b/drivers/clk/imx/clk-blk-ctrl.h
> new file mode 100644
> index 00000000..b3b7fc37
> --- /dev/null
> +++ b/drivers/clk/imx/clk-blk-ctrl.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __MACH_IMX_CLK_BLK_CTRL_H
> +#define __MACH_IMX_CLK_BLK_CTRL_H
> +
> +enum imx_blk_ctrl_hw_type {
> +       BLK_CTRL_CLK_MUX,
> +       BLK_CTRL_CLK_GATE,
> +       BLK_CTRL_CLK_SHARED_GATE,
> +       BLK_CTRL_CLK_PLL14XX,
> +       BLK_CTRL_RESET,
> +};
> +
> +struct imx_blk_ctrl_hw {
> +       int type;
> +       char *name;
> +       u32 offset;
> +       u32 shift;
> +       u32 mask;
> +       u32 width;
> +       u32 flags;
> +       u32 id;
> +       void *parents;
> +       u32 parents_count;
> +       int *shared_count;
> +       struct imx_pll14xx_clk *pll_tbl;
> +};
> +
> +struct imx_blk_ctrl_dev_data {
> +       struct imx_blk_ctrl_hw *hws;
> +       u32 hws_num;
> +
> +       u32 clocks_max;
> +       u32 resets_max;
> +
> +       u32 pm_runtime_saved_regs_num;
> +       u32 pm_runtime_saved_regs[];
> +};
> +
> +#define IMX_BLK_CTRL(_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_CTRL_CLK_MUX(_name, _id, _offset, _shift, _width, _parents) \
> +       IMX_BLK_CTRL(BLK_CTRL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), 0, NULL, NULL)
> +
> +#define IMX_BLK_CTRL_CLK_MUX_FLAGS(_name, _id, _offset, _shift, _width, _parents, _flags) \
> +       IMX_BLK_CTRL(BLK_CTRL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), _flags, NULL, NULL)
> +
> +#define IMX_BLK_CTRL_CLK_GATE(_name, _id, _offset, _shift, _parents) \
> +       IMX_BLK_CTRL(BLK_CTRL_CLK_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, NULL, NULL)
> +
> +#define IMX_BLK_CTRL_CLK_SHARED_GATE(_name, _id, _offset, _shift, _parents, sh_count) \
> +       IMX_BLK_CTRL(BLK_CTRL_CLK_SHARED_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, sh_count, NULL)
> +
> +#define IMX_BLK_CTRL_CLK_PLL14XX(_name, _id, _offset, _parents, _pll_tbl) \
> +       IMX_BLK_CTRL(BLK_CTRL_CLK_PLL14XX, _name, _id, _offset, 0, 0, 0, _parents, 1, 0, NULL, _pll_tbl)
> +
> +#define IMX_BLK_CTRL_RESET(_id, _offset, _shift) \
> +       IMX_BLK_CTRL(BLK_CTRL_RESET, NULL, _id, _offset, _shift, 0, 1, NULL, 0, 0, NULL, NULL)
> +
> +#define IMX_BLK_CTRL_RESET_MASK(_id, _offset, _shift, mask) \
> +       IMX_BLK_CTRL(BLK_CTRL_RESET, NULL, _id, _offset, _shift, 0, mask, NULL, 0, 0, NULL, NULL)
> +
> +extern const struct imx_blk_ctrl_dev_data imx8mp_audio_blk_ctrl_dev_data __initconst;
> +extern const struct imx_blk_ctrl_dev_data imx8mp_media_blk_ctrl_dev_data __initconst;
> +extern const struct imx_blk_ctrl_dev_data imx8mp_hdmi_blk_ctrl_dev_data __initconst;
> +

If no special reasons, i may prefer to put those data in either
clk-blk-ctrl.c or separate clk-blk-ctrl-data.c
because there seems to be no code level dependency on the CCM
driver(clk-imx8mq.c) for clk_blk_ctrl drivers.
Then we can save those extern variables.

Regards
Aisheng

> +#endif
> +
> --
> 2.7.4
>
Abel Vesa Aug. 19, 2020, 8:31 p.m. UTC | #2 
On 20-08-18 19:27:03, Dong Aisheng wrote:
> On Fri, Aug 14, 2020 at 8:12 PM Abel Vesa <abel.vesa@nxp.com> wrote:
> >
> > On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
> > RM and usually is comprised of some GPRs that are considered too
> > generic to be part of any dedicated IP from that specific subsystem.
> >
> > In general, some of the GPRs have some clock bits, some have reset bits,
> > so in order to be able to use the imx clock API, this needs to be
> > in a clock driver. From there it can use the reset controller API and
> > leave the rest to the syscon.
> >
> > This driver is intended to work with the following BLK_CTRL IPs found in
> > i.MX8MP (but it might be reused by the future i.MX platforms that
> > have this kind of IP in their design):
> >  - Audio
> >  - Media
> >  - HDMI
> >
> > Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> 
> The code mostly looks good to me.
> Only a few minor comments.
> 
> > ---
> >  drivers/clk/imx/Makefile       |   2 +-
> >  drivers/clk/imx/clk-blk-ctrl.c | 327 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/imx/clk-blk-ctrl.h |  81 ++++++++++
> >  3 files changed, 409 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/clk/imx/clk-blk-ctrl.c
> >  create mode 100644 drivers/clk/imx/clk-blk-ctrl.h
> >
> > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> > index 928f874c..7afe1df 100644
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -27,7 +27,7 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
> >
> >  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> >  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> > -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> > +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o
> >  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> >  obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> >
> > diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
> > new file mode 100644
> > index 00000000..1672646
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-blk-ctrl.c
> > @@ -0,0 +1,327 @@
> > +// 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-ctrl.h"
> > +
> > +struct reset_hw {
> 
> s/reset_hw/imx_reset_hw
> It helps the reader to quickly understand it's not core structure
> 
> > +       u32 offset;
> > +       u32 shift;
> > +       u32 mask;
> > +       bool asserted;
> > +};
> > +
> > +struct pm_safekeep_info {
> 
> imx_pm_safekeep_info
> 

Will do in the next version.

> > +       uint32_t *regs_values;
> > +       uint32_t *regs_offsets;
> > +       uint32_t regs_num;
> > +};
> > +
> > +struct imx_blk_ctrl_drvdata {
> > +       void __iomem *base;
> > +       struct reset_controller_dev rcdev;
> > +       struct reset_hw *rst_hws;
> > +       struct pm_safekeep_info pm_info;
> > +
> > +       spinlock_t lock;
> > +};
> > +
> > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> > +                                 unsigned long id, bool assert)
> > +{
> > +       struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> > +                       struct imx_blk_ctrl_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;
> > +       unsigned int asserted_before = 0, asserted_after = 0;
> 
> swap above two lines from long to short

Will do.

> 
> > +       u32 reg;
> > +       int i;
> > +
> > +       spin_lock_irqsave(&drvdata->lock, flags);
> > +
> > +       for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > +               if (drvdata->rst_hws[i].asserted)
> > +                       asserted_before++;
> > +
> > +       if (asserted_before == 0 && assert)
> > +               pm_runtime_get(rcdev->dev);
> > +
> > +       if (assert) {
> > +               reg = readl(reg_addr);
> > +               writel(reg & ~(mask << shift), reg_addr);
> > +               drvdata->rst_hws[id].asserted = true;
> > +       } else {
> > +               reg = readl(reg_addr);
> > +               writel(reg | (mask << shift), reg_addr);
> > +               drvdata->rst_hws[id].asserted = false;
> > +       }
> > +
> > +       for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > +               if (drvdata->rst_hws[i].asserted)
> > +                       asserted_after++;
> 
> I guess the logic may be able to be simplified.
> For example, put assert ref count in the private structure.
> Then
> call pm_runtime_get when ref count is 0 and assert is true.
> call pm_runtime_put when ref ount is 0 and assert is false.
> No need to go through twice for loop each time.
> 

Not really. For example, nothing stops a user to assert the same reset twice
before deasserting. The refcount will increase twice while decreasing once.
So we need to keep track of all the assertions and deassertions for each bit.

If the assert is requested twice, the asserted member is already set so no change.
Same logic goes for the deassertion. 

In the end we need to know when the last asserted bit is deasserted in order
to call the pm_runtime_put. Also when the there was no bit asserted and an
assertion is happening in order to do the pm_runtime_get. 

The alternative to the current implementation would be a mask, but the number
of bits could be virtually unlimited for a BLK_CTL IP, therefore, the two
loops and the asserted member seem a cleaner approach.

> > +
> > +       if (asserted_before == 1 && asserted_after == 0)
> > +               pm_runtime_put(rcdev->dev);
> > +
> > +       spin_unlock_irqrestore(&drvdata->lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx_blk_ctrl_reset_assert(struct reset_controller_dev *rcdev,
> > +                                          unsigned long id)
> > +{
> > +       return imx_blk_ctrl_reset_set(rcdev, id, true);
> > +}
> > +
> > +static int imx_blk_ctrl_reset_deassert(struct reset_controller_dev *rcdev,
> > +                                            unsigned long id)
> > +{
> > +       return imx_blk_ctrl_reset_set(rcdev, id, false);
> > +}
> > +
> > +static const struct reset_control_ops imx_blk_ctrl_reset_ops = {
> > +       .assert         = imx_blk_ctrl_reset_assert,
> > +       .deassert       = imx_blk_ctrl_reset_deassert,
> > +};
> > +
> > +static int imx_blk_ctrl_register_reset_controller(struct device *dev)
> > +{
> > +       struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> > +       const struct imx_blk_ctrl_dev_data *dev_data = of_device_get_match_data(dev);
> > +       struct reset_hw *hws;
> > +       int max = dev_data->resets_max;
> 
> sort from long to short

Will do.

> 
> > +       int i;
> > +
> > +       spin_lock_init(&drvdata->lock);
> > +
> > +       drvdata->rcdev.owner     = THIS_MODULE;
> > +       drvdata->rcdev.nr_resets = max;
> > +       drvdata->rcdev.ops       = &imx_blk_ctrl_reset_ops;
> > +       drvdata->rcdev.of_node   = dev->of_node;
> > +       drvdata->rcdev.dev       = dev;
> > +
> > +       drvdata->rst_hws = devm_kcalloc(dev, max, sizeof(struct reset_hw),
> > +                                       GFP_KERNEL);
> > +       hws = drvdata->rst_hws;
> > +
> > +       for (i = 0; i < dev_data->hws_num; i++) {
> > +               struct imx_blk_ctrl_hw *hw = &dev_data->hws[i];
> > +
> > +               if (hw->type != BLK_CTRL_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);
> > +}
> > +static struct clk_hw *imx_blk_ctrl_register_one_clock(struct device *dev,
> > +                                               struct imx_blk_ctrl_hw *hw)
> > +{
> > +       struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> > +       void __iomem *base = drvdata->base;
> > +       struct clk_hw *clk_hw;
> > +
> > +       switch (hw->type) {
> > +       case BLK_CTRL_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_CTRL_CLK_GATE:
> > +               clk_hw = imx_dev_clk_hw_gate(dev, hw->name, hw->parents,
> > +                                            base + hw->offset, hw->shift);
> > +               break;
> > +       case BLK_CTRL_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_CTRL_CLK_PLL14XX:
> > +               clk_hw = imx_dev_clk_hw_pll14xx(dev, hw->name, hw->parents,
> > +                                               base + hw->offset, hw->pll_tbl);
> > +               break;
> > +       default:
> > +               clk_hw = NULL;
> 
> A better way may be assign clk_hw default to NULL.
> Then instead, we can add a WARN here in case the clk hw data is insane.
> 

Will do.

> > +       };
> > +
> > +       return clk_hw;
> > +}
> > +
> > +static int imx_blk_ctrl_register_clock_controller(struct device *dev)
> > +{
> > +       const struct imx_blk_ctrl_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))
> > +               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_ctrl_hw *hw = &dev_data->hws[i];
> > +               struct clk_hw *tmp = imx_blk_ctrl_register_one_clock(dev, hw);
> > +
> > +               if (!tmp)
> > +                       continue;
> > +               hws[hw->id] = tmp;
> 
> tmp could be a non NULL error pointer.
> Maybe here could be simplied as:
> hws[hw->id] = imx_blk_ctrl_register_one_clock(dev, hw);
> 

Will do.

> > +       }
> > +
> > +       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_ctrl_init_runtime_pm_safekeeping(struct device *dev)
> > +{
> > +       const struct imx_blk_ctrl_dev_data *dev_data = of_device_get_match_data(dev);
> > +       struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> > +       struct 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)))
> > +               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)))
> > +               return PTR_ERR(pm_info->regs_offsets);
> > +
> > +       pm_info->regs_num = regs_num;
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx_blk_ctrl_probe(struct platform_device *pdev)
> > +{
> > +       struct imx_blk_ctrl_drvdata *drvdata;
> > +       struct device *dev = &pdev->dev;
> > +       int ret;
> > +
> > +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +       if (WARN_ON(!drvdata))
> > +               return -ENOMEM;
> > +
> > +       drvdata->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (WARN_ON(IS_ERR(drvdata->base)))
> > +               return PTR_ERR(drvdata->base);
> > +
> > +       dev_set_drvdata(dev, drvdata);
> > +
> > +       ret = imx_blk_ctrl_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_ctrl_register_clock_controller(dev);
> > +       if (ret) {
> > +               pm_runtime_put(dev);
> > +               return ret;
> > +       }
> > +
> > +       ret = imx_blk_ctrl_register_reset_controller(dev);
> > +
> > +       pm_runtime_put(dev);
> > +
> > +       return ret;
> > +}
> > +
> > +static void imx_blk_ctrl_read_write(struct device *dev, bool write)
> 
> __maybe_unused
> 

Will do.

> > +{
> > +       struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> > +       struct 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 imx_blk_ctrl_runtime_suspend(struct device *dev)
> 
> __maybe_unused
> 

Will do.

> > +{
> > +       imx_blk_ctrl_read_write(dev, false);
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx_blk_ctrl_runtime_resume(struct device *dev)
> 
> __maybe_unused
> 

Will do.

> > +{
> > +       imx_blk_ctrl_read_write(dev, true);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops imx_blk_ctrl_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(imx_blk_ctrl_runtime_suspend,
> > +                          imx_blk_ctrl_runtime_resume, NULL)
> > +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +                          pm_runtime_force_resume)
> > +};
> > +
> > +static const struct of_device_id imx_blk_ctrl_of_match[] = {
> > +       { /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx_blk_ctrl_of_match);
> > +
> > +static struct platform_driver imx_blk_ctrl_driver = {
> > +       .probe = imx_blk_ctrl_probe,
> > +       .driver = {
> > +               .name = "imx-blk-ctrl",
> > +               .of_match_table = of_match_ptr(imx_blk_ctrl_of_match),
> > +               .pm = &imx_blk_ctrl_pm_ops,
> > +       },
> > +};
> > +module_platform_driver(imx_blk_ctrl_driver);
> > diff --git a/drivers/clk/imx/clk-blk-ctrl.h b/drivers/clk/imx/clk-blk-ctrl.h
> > new file mode 100644
> > index 00000000..b3b7fc37
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-blk-ctrl.h
> > @@ -0,0 +1,81 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __MACH_IMX_CLK_BLK_CTRL_H
> > +#define __MACH_IMX_CLK_BLK_CTRL_H
> > +
> > +enum imx_blk_ctrl_hw_type {
> > +       BLK_CTRL_CLK_MUX,
> > +       BLK_CTRL_CLK_GATE,
> > +       BLK_CTRL_CLK_SHARED_GATE,
> > +       BLK_CTRL_CLK_PLL14XX,
> > +       BLK_CTRL_RESET,
> > +};
> > +
> > +struct imx_blk_ctrl_hw {
> > +       int type;
> > +       char *name;
> > +       u32 offset;
> > +       u32 shift;
> > +       u32 mask;
> > +       u32 width;
> > +       u32 flags;
> > +       u32 id;
> > +       void *parents;
> > +       u32 parents_count;
> > +       int *shared_count;
> > +       struct imx_pll14xx_clk *pll_tbl;
> > +};
> > +
> > +struct imx_blk_ctrl_dev_data {
> > +       struct imx_blk_ctrl_hw *hws;
> > +       u32 hws_num;
> > +
> > +       u32 clocks_max;
> > +       u32 resets_max;
> > +
> > +       u32 pm_runtime_saved_regs_num;
> > +       u32 pm_runtime_saved_regs[];
> > +};
> > +
> > +#define IMX_BLK_CTRL(_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_CTRL_CLK_MUX(_name, _id, _offset, _shift, _width, _parents) \
> > +       IMX_BLK_CTRL(BLK_CTRL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), 0, NULL, NULL)
> > +
> > +#define IMX_BLK_CTRL_CLK_MUX_FLAGS(_name, _id, _offset, _shift, _width, _parents, _flags) \
> > +       IMX_BLK_CTRL(BLK_CTRL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), _flags, NULL, NULL)
> > +
> > +#define IMX_BLK_CTRL_CLK_GATE(_name, _id, _offset, _shift, _parents) \
> > +       IMX_BLK_CTRL(BLK_CTRL_CLK_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, NULL, NULL)
> > +
> > +#define IMX_BLK_CTRL_CLK_SHARED_GATE(_name, _id, _offset, _shift, _parents, sh_count) \
> > +       IMX_BLK_CTRL(BLK_CTRL_CLK_SHARED_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, sh_count, NULL)
> > +
> > +#define IMX_BLK_CTRL_CLK_PLL14XX(_name, _id, _offset, _parents, _pll_tbl) \
> > +       IMX_BLK_CTRL(BLK_CTRL_CLK_PLL14XX, _name, _id, _offset, 0, 0, 0, _parents, 1, 0, NULL, _pll_tbl)
> > +
> > +#define IMX_BLK_CTRL_RESET(_id, _offset, _shift) \
> > +       IMX_BLK_CTRL(BLK_CTRL_RESET, NULL, _id, _offset, _shift, 0, 1, NULL, 0, 0, NULL, NULL)
> > +
> > +#define IMX_BLK_CTRL_RESET_MASK(_id, _offset, _shift, mask) \
> > +       IMX_BLK_CTRL(BLK_CTRL_RESET, NULL, _id, _offset, _shift, 0, mask, NULL, 0, 0, NULL, NULL)
> > +
> > +extern const struct imx_blk_ctrl_dev_data imx8mp_audio_blk_ctrl_dev_data __initconst;
> > +extern const struct imx_blk_ctrl_dev_data imx8mp_media_blk_ctrl_dev_data __initconst;
> > +extern const struct imx_blk_ctrl_dev_data imx8mp_hdmi_blk_ctrl_dev_data __initconst;
> > +
> 
> If no special reasons, i may prefer to put those data in either
> clk-blk-ctrl.c or separate clk-blk-ctrl-data.c
> because there seems to be no code level dependency on the CCM
> driver(clk-imx8mq.c) for clk_blk_ctrl drivers.
> Then we can save those extern variables.
> 

The rationale here is to have the SoC specific definitions in the SoC specific
clock provider driver. Otherwise with every new SoC that will use the blk_ctl
IPs we will increase the size of clk-blk-ctrl driver. Plus the kernel names of
the clocks used by each blk_ctl IP as parents are also defined in the SoC
specific clock provider driver.

I'll find a way, though, to move those so that they would not be shared between
all the clock drivers that needs a blk_ctl implementation.

> Regards
> Aisheng
> 
> > +#endif
> > +
> > --
> > 2.7.4
> >
Dong Aisheng Aug. 20, 2020, 1:40 a.m. UTC | #3 
On Thu, Aug 20, 2020 at 4:31 AM Abel Vesa <abel.vesa@nxp.com> wrote:
....
> > > +extern const struct imx_blk_ctrl_dev_data imx8mp_audio_blk_ctrl_dev_data __initconst;
> > > +extern const struct imx_blk_ctrl_dev_data imx8mp_media_blk_ctrl_dev_data __initconst;
> > > +extern const struct imx_blk_ctrl_dev_data imx8mp_hdmi_blk_ctrl_dev_data __initconst;
> > > +
> >
> > If no special reasons, i may prefer to put those data in either
> > clk-blk-ctrl.c or separate clk-blk-ctrl-data.c
> > because there seems to be no code level dependency on the CCM
> > driver(clk-imx8mq.c) for clk_blk_ctrl drivers.
> > Then we can save those extern variables.
> >
>
> The rationale here is to have the SoC specific definitions in the SoC specific
> clock provider driver. Otherwise with every new SoC that will use the blk_ctl
> IPs we will increase the size of clk-blk-ctrl driver. Plus the kernel names of
> the clocks used by each blk_ctl IP as parents are also defined in the SoC
> specific clock provider driver.
>
> I'll find a way, though, to move those so that they would not be shared between
> all the clock drivers that needs a blk_ctl implementation.
>

Please refer to pinctrl-imx.c to see if we can do similar things for blk-ctrl.

Regards
Aisheng

> > Regards
> > Aisheng
> >
> > > +#endif
> > > +
> > > --
> > > 2.7.4
> > >
Philipp Zabel Aug. 25, 2020, 10:48 a.m. UTC | #4 
On Fri, 2020-08-14 at 15:09 +0300, Abel Vesa wrote:
> On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
> RM and usually is comprised of some GPRs that are considered too
> generic to be part of any dedicated IP from that specific subsystem.
> 
> In general, some of the GPRs have some clock bits, some have reset bits,
> so in order to be able to use the imx clock API, this needs to be
> in a clock driver. From there it can use the reset controller API and
> leave the rest to the syscon.
> 
> This driver is intended to work with the following BLK_CTRL IPs found in
> i.MX8MP (but it might be reused by the future i.MX platforms that
> have this kind of IP in their design):
>  - Audio
>  - Media
>  - HDMI
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> ---
>  drivers/clk/imx/Makefile       |   2 +-
>  drivers/clk/imx/clk-blk-ctrl.c | 327 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/imx/clk-blk-ctrl.h |  81 ++++++++++
>  3 files changed, 409 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/imx/clk-blk-ctrl.c
>  create mode 100644 drivers/clk/imx/clk-blk-ctrl.h
> 
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 928f874c..7afe1df 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -27,7 +27,7 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
>  
>  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
>  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o
>  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
>  obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
>  
> diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
> new file mode 100644
> index 00000000..1672646
> --- /dev/null
> +++ b/drivers/clk/imx/clk-blk-ctrl.c
> @@ -0,0 +1,327 @@
> +// 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-ctrl.h"
> +
> +struct reset_hw {
> +	u32 offset;
> +	u32 shift;
> +	u32 mask;
> +	bool asserted;
> +};
> +
> +struct pm_safekeep_info {
> +	uint32_t *regs_values;
> +	uint32_t *regs_offsets;
> +	uint32_t regs_num;
> +};
> +
> +struct imx_blk_ctrl_drvdata {
> +	void __iomem *base;
> +	struct reset_controller_dev rcdev;
> +	struct reset_hw *rst_hws;
> +	struct pm_safekeep_info pm_info;
> +
> +	spinlock_t lock;
> +};
> +
> +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> +				  unsigned long id, bool assert)
> +{
> +	struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> +			struct imx_blk_ctrl_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;
> +	unsigned int asserted_before = 0, asserted_after = 0;
> +	u32 reg;
> +	int i;
> +
> +	spin_lock_irqsave(&drvdata->lock, flags);
> +
> +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> +		if (drvdata->rst_hws[i].asserted)
> +			asserted_before++;
> +
> +	if (asserted_before == 0 && assert)
> +		pm_runtime_get(rcdev->dev);

Shouldn't that be pm_runtime_get_sync() ?

I would do that unconditionally before locking drvdata->lock and then
drop unnecessary refcounts afterwards.

> +
> +	if (assert) {
> +		reg = readl(reg_addr);
> +		writel(reg & ~(mask << shift), reg_addr);
> +		drvdata->rst_hws[id].asserted = true;
> +	} else {
> +		reg = readl(reg_addr);
> +		writel(reg | (mask << shift), reg_addr);
> +		drvdata->rst_hws[id].asserted = false;
> +	}
> +
> +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> +		if (drvdata->rst_hws[i].asserted)
> +			asserted_after++;
> +
> +	if (asserted_before == 1 && asserted_after == 0)
> +		pm_runtime_put(rcdev->dev);
> +
> +	spin_unlock_irqrestore(&drvdata->lock, flags);
> +
> +	return 0;
> +}

regards
Philipp
Abel Vesa Aug. 25, 2020, 11:24 a.m. UTC | #5 
On 20-08-25 12:48:41, Philipp Zabel wrote:
> On Fri, 2020-08-14 at 15:09 +0300, Abel Vesa wrote:
> > On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
> > RM and usually is comprised of some GPRs that are considered too
> > generic to be part of any dedicated IP from that specific subsystem.
> > 
> > In general, some of the GPRs have some clock bits, some have reset bits,
> > so in order to be able to use the imx clock API, this needs to be
> > in a clock driver. From there it can use the reset controller API and
> > leave the rest to the syscon.
> > 
> > This driver is intended to work with the following BLK_CTRL IPs found in
> > i.MX8MP (but it might be reused by the future i.MX platforms that
> > have this kind of IP in their design):
> >  - Audio
> >  - Media
> >  - HDMI
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> > ---
> >  drivers/clk/imx/Makefile       |   2 +-
> >  drivers/clk/imx/clk-blk-ctrl.c | 327 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/imx/clk-blk-ctrl.h |  81 ++++++++++
> >  3 files changed, 409 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/clk/imx/clk-blk-ctrl.c
> >  create mode 100644 drivers/clk/imx/clk-blk-ctrl.h
> > 
> > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> > index 928f874c..7afe1df 100644
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -27,7 +27,7 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
> >  
> >  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> >  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> > -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> > +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o
> >  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> >  obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> >  
> > diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
> > new file mode 100644
> > index 00000000..1672646
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-blk-ctrl.c
> > @@ -0,0 +1,327 @@
> > +// 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-ctrl.h"
> > +
> > +struct reset_hw {
> > +	u32 offset;
> > +	u32 shift;
> > +	u32 mask;
> > +	bool asserted;
> > +};
> > +
> > +struct pm_safekeep_info {
> > +	uint32_t *regs_values;
> > +	uint32_t *regs_offsets;
> > +	uint32_t regs_num;
> > +};
> > +
> > +struct imx_blk_ctrl_drvdata {
> > +	void __iomem *base;
> > +	struct reset_controller_dev rcdev;
> > +	struct reset_hw *rst_hws;
> > +	struct pm_safekeep_info pm_info;
> > +
> > +	spinlock_t lock;
> > +};
> > +
> > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> > +				  unsigned long id, bool assert)
> > +{
> > +	struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> > +			struct imx_blk_ctrl_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;
> > +	unsigned int asserted_before = 0, asserted_after = 0;
> > +	u32 reg;
> > +	int i;
> > +
> > +	spin_lock_irqsave(&drvdata->lock, flags);
> > +
> > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > +		if (drvdata->rst_hws[i].asserted)
> > +			asserted_before++;
> > +
> > +	if (asserted_before == 0 && assert)
> > +		pm_runtime_get(rcdev->dev);
> 
> Shouldn't that be pm_runtime_get_sync() ?
> 
> I would do that unconditionally before locking drvdata->lock and then
> drop unnecessary refcounts afterwards.
> 

I thought we already discussed this on the last's version thread.

The assert and deassert do not necessary get called in pairs,
leading to the device power domain staying always on because
some use called assert multiple times without doig the same number
of deasserts.

> > +
> > +	if (assert) {
> > +		reg = readl(reg_addr);
> > +		writel(reg & ~(mask << shift), reg_addr);
> > +		drvdata->rst_hws[id].asserted = true;
> > +	} else {
> > +		reg = readl(reg_addr);
> > +		writel(reg | (mask << shift), reg_addr);
> > +		drvdata->rst_hws[id].asserted = false;
> > +	}
> > +
> > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > +		if (drvdata->rst_hws[i].asserted)
> > +			asserted_after++;
> > +
> > +	if (asserted_before == 1 && asserted_after == 0)
> > +		pm_runtime_put(rcdev->dev);
> > +
> > +	spin_unlock_irqrestore(&drvdata->lock, flags);
> > +
> > +	return 0;
> > +}
> 
> regards
> Philipp
Philipp Zabel Aug. 25, 2020, 12:07 p.m. UTC | #6 
On Tue, 2020-08-25 at 14:24 +0300, Abel Vesa wrote:
[...]
> > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> > > +				  unsigned long id, bool assert)
> > > +{
> > > +	struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> > > +			struct imx_blk_ctrl_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;
> > > +	unsigned int asserted_before = 0, asserted_after = 0;
> > > +	u32 reg;
> > > +	int i;
> > > +
> > > +	spin_lock_irqsave(&drvdata->lock, flags);
> > > +
> > > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > +		if (drvdata->rst_hws[i].asserted)
> > > +			asserted_before++;
> > > +
> > > +	if (asserted_before == 0 && assert)
> > > +		pm_runtime_get(rcdev->dev);
> > 
> > Shouldn't that be pm_runtime_get_sync() ?
> > 
> > I would do that unconditionally before locking drvdata->lock and then
> > drop unnecessary refcounts afterwards.
> > 
> 
> I thought we already discussed this on the last's version thread.

This is about something different. pm_runtime_get() just queues the
device to be enabled at a later point, but I presume you want to have it
enabled before writing to its registers. (The question here is can you
write to the registers, and have the device update its internal state,
while the power domain is disabled?)
Either way, if you want the reset to be asserted after the function
returns (as is required by the reset API), as I understand it, you have
to make sure that the power domain is activated before the function
returns.
Therefore pm_runtime_get_sync() is required instead of pm_runtime_get(),
and that must be called outside of the spin locked section. My
suggestion would be:

	if (assert)
		pm_runtime_get_sync();
	spin_lock_irqsave();
	/* ... */
	spin_unlock_irqrestore();
	if (assert && asserted_before)
		pm_runtime_put();

unless the following might be an issue:

> > > +
> > > +	if (assert) {
> > > +		reg = readl(reg_addr);
> > > +		writel(reg & ~(mask << shift), reg_addr);
> > > +		drvdata->rst_hws[id].asserted = true;
> > > +	} else {
> > > +		reg = readl(reg_addr);
> > > +		writel(reg | (mask << shift), reg_addr);

Could this cause problems if the power domain is already disabled? If
so, it would be best to either temporarily enable power, or to skip the
register writes if asserted_before == 0 && !assert.

> > > +		drvdata->rst_hws[id].asserted = false;
> > > +	}
> > > +
> > > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > +		if (drvdata->rst_hws[i].asserted)
> > > +			asserted_after++;
> > > +
> > > +	if (asserted_before == 1 && asserted_after == 0)
> > > +		pm_runtime_put(rcdev->dev);
> > > +
> > > +	spin_unlock_irqrestore(&drvdata->lock, flags);
> > > +
> > > +	return 0;
> > > +}

regards
Philipp
Abel Vesa Aug. 25, 2020, 2:11 p.m. UTC | #7 
On 20-08-25 14:07:29, Philipp Zabel wrote:
> On Tue, 2020-08-25 at 14:24 +0300, Abel Vesa wrote:
> [...]
> > > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> > > > +				  unsigned long id, bool assert)
> > > > +{
> > > > +	struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> > > > +			struct imx_blk_ctrl_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;
> > > > +	unsigned int asserted_before = 0, asserted_after = 0;
> > > > +	u32 reg;
> > > > +	int i;
> > > > +
> > > > +	spin_lock_irqsave(&drvdata->lock, flags);
> > > > +
> > > > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > > +		if (drvdata->rst_hws[i].asserted)
> > > > +			asserted_before++;
> > > > +
> > > > +	if (asserted_before == 0 && assert)
> > > > +		pm_runtime_get(rcdev->dev);
> > > 
> > > Shouldn't that be pm_runtime_get_sync() ?
> > > 
> > > I would do that unconditionally before locking drvdata->lock and then
> > > drop unnecessary refcounts afterwards.
> > > 
> > 
> > I thought we already discussed this on the last's version thread.
> 
> This is about something different. pm_runtime_get() just queues the
> device to be enabled at a later point, but I presume you want to have it
> enabled before writing to its registers. (The question here is can you
> write to the registers, and have the device update its internal state,
> while the power domain is disabled?)
> Either way, if you want the reset to be asserted after the function
> returns (as is required by the reset API), as I understand it, you have
> to make sure that the power domain is activated before the function
> returns.
> Therefore pm_runtime_get_sync() is required instead of pm_runtime_get(),
> and that must be called outside of the spin locked section. My
> suggestion would be:
> 
> 	if (assert)
> 		pm_runtime_get_sync();
> 	spin_lock_irqsave();
> 	/* ... */
> 	spin_unlock_irqrestore();
> 	if (assert && asserted_before)
> 		pm_runtime_put();
> 

You're right this makes more sense.

> unless the following might be an issue:
> 
> > > > +
> > > > +	if (assert) {
> > > > +		reg = readl(reg_addr);
> > > > +		writel(reg & ~(mask << shift), reg_addr);
> > > > +		drvdata->rst_hws[id].asserted = true;
> > > > +	} else {
> > > > +		reg = readl(reg_addr);
> > > > +		writel(reg | (mask << shift), reg_addr);
> 
> Could this cause problems if the power domain is already disabled? If
> so, it would be best to either temporarily enable power, or to skip the
> register writes if asserted_before == 0 && !assert.

I'll go with the latter one since it leaves the PD off.

> 
> > > > +		drvdata->rst_hws[id].asserted = false;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > > +		if (drvdata->rst_hws[i].asserted)
> > > > +			asserted_after++;
> > > > +
> > > > +	if (asserted_before == 1 && asserted_after == 0)
> > > > +		pm_runtime_put(rcdev->dev);
> > > > +
> > > > +	spin_unlock_irqrestore(&drvdata->lock, flags);
> > > > +
> > > > +	return 0;
> > > > +}
> 
> regards
> Philipp
Abel Vesa Aug. 25, 2020, 6:30 p.m. UTC | #8 
On 20-08-25 14:07:29, Philipp Zabel wrote:
> On Tue, 2020-08-25 at 14:24 +0300, Abel Vesa wrote:
> [...]
> > > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> > > > +				  unsigned long id, bool assert)
> > > > +{
> > > > +	struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> > > > +			struct imx_blk_ctrl_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;
> > > > +	unsigned int asserted_before = 0, asserted_after = 0;
> > > > +	u32 reg;
> > > > +	int i;
> > > > +
> > > > +	spin_lock_irqsave(&drvdata->lock, flags);
> > > > +
> > > > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > > +		if (drvdata->rst_hws[i].asserted)
> > > > +			asserted_before++;
> > > > +
> > > > +	if (asserted_before == 0 && assert)
> > > > +		pm_runtime_get(rcdev->dev);
> > > 
> > > Shouldn't that be pm_runtime_get_sync() ?
> > > 
> > > I would do that unconditionally before locking drvdata->lock and then
> > > drop unnecessary refcounts afterwards.
> > > 
> > 
> > I thought we already discussed this on the last's version thread.
> 
> This is about something different. pm_runtime_get() just queues the
> device to be enabled at a later point, but I presume you want to have it
> enabled before writing to its registers. (The question here is can you
> write to the registers, and have the device update its internal state,
> while the power domain is disabled?)
> Either way, if you want the reset to be asserted after the function
> returns (as is required by the reset API), as I understand it, you have
> to make sure that the power domain is activated before the function
> returns.
> Therefore pm_runtime_get_sync() is required instead of pm_runtime_get(),
> and that must be called outside of the spin locked section. My
> suggestion would be:
> 
> 	if (assert)
> 		pm_runtime_get_sync();
> 	spin_lock_irqsave();
> 	/* ... */
> 	spin_unlock_irqrestore();
> 	if (assert && asserted_before)
> 		pm_runtime_put();
> 

On a second thought this doesn't work because, for the first assertion,
the runtime put will never be called, if the asserted_before does not count
the current assertion. If it counts the current assertion, then every assertion
will end with runtime put. None of these options work here.

How about the following:

	if (assert && !test_and_set_bit(1, &drvdata->rst_hws[id].asserted))     
		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);                                     
                                                                        
This would only call the get_sync/put once for each reset bit.

> unless the following might be an issue:
> 
> > > > +
> > > > +	if (assert) {
> > > > +		reg = readl(reg_addr);
> > > > +		writel(reg & ~(mask << shift), reg_addr);
> > > > +		drvdata->rst_hws[id].asserted = true;
> > > > +	} else {
> > > > +		reg = readl(reg_addr);
> > > > +		writel(reg | (mask << shift), reg_addr);
> 
> Could this cause problems if the power domain is already disabled? If
> so, it would be best to either temporarily enable power, or to skip the
> register writes if asserted_before == 0 && !assert.
> 
> > > > +		drvdata->rst_hws[id].asserted = false;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > > +		if (drvdata->rst_hws[i].asserted)
> > > > +			asserted_after++;
> > > > +
> > > > +	if (asserted_before == 1 && asserted_after == 0)
> > > > +		pm_runtime_put(rcdev->dev);
> > > > +
> > > > +	spin_unlock_irqrestore(&drvdata->lock, flags);
> > > > +
> > > > +	return 0;
> > > > +}
> 
> regards
> Philipp
Philipp Zabel Aug. 26, 2020, 7:46 a.m. UTC | #9 
On Tue, 2020-08-25 at 21:30 +0300, Abel Vesa wrote:
[...]
> > 	if (assert)
> > 		pm_runtime_get_sync();
> > 	spin_lock_irqsave();
> > 	/* ... */
> > 	spin_unlock_irqrestore();
> > 	if (assert && asserted_before)
> > 		pm_runtime_put();
> > 
> 
> On a second thought this doesn't work because, for the first assertion,
> the runtime put will never be called, if the asserted_before does not count
> the current assertion.

I'm not sure I follow. The first assert will increment device usage
0 -> 1, all others asserts will just temporarily increment and decrement
1 -> 2 -> 1. Isn't this just missing one
	if (!assert && !asserted_after)
		pm_runtime_put()
to do the last deassert 1 -> 0 transition?

> If it counts the current assertion, then every assertion
> will end with runtime put. None of these options work here.
>
> How about the following:
>
> 	if (assert && !test_and_set_bit(1, &drvdata->rst_hws[id].asserted))     
> 		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);                                     
>                                                                         
> This would only call the get_sync/put once for each reset bit.

Yes, that should work. I think it is a much better idea, no more looping
through the entire reset control array.

regards
Philipp
diff mbox series

Patch

diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index 928f874c..7afe1df 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -27,7 +27,7 @@  obj-$(CONFIG_MXC_CLK_SCU) += \
 
 obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
 obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
-obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
+obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o
 obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
 obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
 
diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
new file mode 100644
index 00000000..1672646
--- /dev/null
+++ b/drivers/clk/imx/clk-blk-ctrl.c
@@ -0,0 +1,327 @@ 
+// 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-ctrl.h"
+
+struct reset_hw {
+	u32 offset;
+	u32 shift;
+	u32 mask;
+	bool asserted;
+};
+
+struct pm_safekeep_info {
+	uint32_t *regs_values;
+	uint32_t *regs_offsets;
+	uint32_t regs_num;
+};
+
+struct imx_blk_ctrl_drvdata {
+	void __iomem *base;
+	struct reset_controller_dev rcdev;
+	struct reset_hw *rst_hws;
+	struct pm_safekeep_info pm_info;
+
+	spinlock_t lock;
+};
+
+static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
+				  unsigned long id, bool assert)
+{
+	struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
+			struct imx_blk_ctrl_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;
+	unsigned int asserted_before = 0, asserted_after = 0;
+	u32 reg;
+	int i;
+
+	spin_lock_irqsave(&drvdata->lock, flags);
+
+	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
+		if (drvdata->rst_hws[i].asserted)
+			asserted_before++;
+
+	if (asserted_before == 0 && assert)
+		pm_runtime_get(rcdev->dev);
+
+	if (assert) {
+		reg = readl(reg_addr);
+		writel(reg & ~(mask << shift), reg_addr);
+		drvdata->rst_hws[id].asserted = true;
+	} else {
+		reg = readl(reg_addr);
+		writel(reg | (mask << shift), reg_addr);
+		drvdata->rst_hws[id].asserted = false;
+	}
+
+	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
+		if (drvdata->rst_hws[i].asserted)
+			asserted_after++;
+
+	if (asserted_before == 1 && asserted_after == 0)
+		pm_runtime_put(rcdev->dev);
+
+	spin_unlock_irqrestore(&drvdata->lock, flags);
+
+	return 0;
+}
+
+static int imx_blk_ctrl_reset_assert(struct reset_controller_dev *rcdev,
+					   unsigned long id)
+{
+	return imx_blk_ctrl_reset_set(rcdev, id, true);
+}
+
+static int imx_blk_ctrl_reset_deassert(struct reset_controller_dev *rcdev,
+					     unsigned long id)
+{
+	return imx_blk_ctrl_reset_set(rcdev, id, false);
+}
+
+static const struct reset_control_ops imx_blk_ctrl_reset_ops = {
+	.assert		= imx_blk_ctrl_reset_assert,
+	.deassert	= imx_blk_ctrl_reset_deassert,
+};
+
+static int imx_blk_ctrl_register_reset_controller(struct device *dev)
+{
+	struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+	const struct imx_blk_ctrl_dev_data *dev_data = of_device_get_match_data(dev);
+	struct reset_hw *hws;
+	int max = dev_data->resets_max;
+	int i;
+
+	spin_lock_init(&drvdata->lock);
+
+	drvdata->rcdev.owner     = THIS_MODULE;
+	drvdata->rcdev.nr_resets = max;
+	drvdata->rcdev.ops       = &imx_blk_ctrl_reset_ops;
+	drvdata->rcdev.of_node   = dev->of_node;
+	drvdata->rcdev.dev	 = dev;
+
+	drvdata->rst_hws = devm_kcalloc(dev, max, sizeof(struct reset_hw),
+					GFP_KERNEL);
+	hws = drvdata->rst_hws;
+
+	for (i = 0; i < dev_data->hws_num; i++) {
+		struct imx_blk_ctrl_hw *hw = &dev_data->hws[i];
+
+		if (hw->type != BLK_CTRL_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);
+}
+static struct clk_hw *imx_blk_ctrl_register_one_clock(struct device *dev,
+						struct imx_blk_ctrl_hw *hw)
+{
+	struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+	void __iomem *base = drvdata->base;
+	struct clk_hw *clk_hw;
+
+	switch (hw->type) {
+	case BLK_CTRL_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_CTRL_CLK_GATE:
+		clk_hw = imx_dev_clk_hw_gate(dev, hw->name, hw->parents,
+					     base + hw->offset, hw->shift);
+		break;
+	case BLK_CTRL_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_CTRL_CLK_PLL14XX:
+		clk_hw = imx_dev_clk_hw_pll14xx(dev, hw->name, hw->parents,
+						base + hw->offset, hw->pll_tbl);
+		break;
+	default:
+		clk_hw = NULL;
+	};
+
+	return clk_hw;
+}
+
+static int imx_blk_ctrl_register_clock_controller(struct device *dev)
+{
+	const struct imx_blk_ctrl_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))
+		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_ctrl_hw *hw = &dev_data->hws[i];
+		struct clk_hw *tmp = imx_blk_ctrl_register_one_clock(dev, hw);
+
+		if (!tmp)
+			continue;
+		hws[hw->id] = tmp;
+	}
+
+	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_ctrl_init_runtime_pm_safekeeping(struct device *dev)
+{
+	const struct imx_blk_ctrl_dev_data *dev_data = of_device_get_match_data(dev);
+	struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+	struct 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)))
+		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)))
+		return PTR_ERR(pm_info->regs_offsets);
+
+	pm_info->regs_num = regs_num;
+
+	return 0;
+}
+
+static int imx_blk_ctrl_probe(struct platform_device *pdev)
+{
+	struct imx_blk_ctrl_drvdata *drvdata;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (WARN_ON(!drvdata))
+		return -ENOMEM;
+
+	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
+	if (WARN_ON(IS_ERR(drvdata->base)))
+		return PTR_ERR(drvdata->base);
+
+	dev_set_drvdata(dev, drvdata);
+
+	ret = imx_blk_ctrl_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_ctrl_register_clock_controller(dev);
+	if (ret) {
+		pm_runtime_put(dev);
+		return ret;
+	}
+
+	ret = imx_blk_ctrl_register_reset_controller(dev);
+
+	pm_runtime_put(dev);
+
+	return ret;
+}
+
+static void imx_blk_ctrl_read_write(struct device *dev, bool write)
+{
+	struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+	struct 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 imx_blk_ctrl_runtime_suspend(struct device *dev)
+{
+	imx_blk_ctrl_read_write(dev, false);
+
+	return 0;
+}
+
+static int imx_blk_ctrl_runtime_resume(struct device *dev)
+{
+	imx_blk_ctrl_read_write(dev, true);
+
+	return 0;
+}
+
+static const struct dev_pm_ops imx_blk_ctrl_pm_ops = {
+	SET_RUNTIME_PM_OPS(imx_blk_ctrl_runtime_suspend,
+			   imx_blk_ctrl_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+			   pm_runtime_force_resume)
+};
+
+static const struct of_device_id imx_blk_ctrl_of_match[] = {
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_blk_ctrl_of_match);
+
+static struct platform_driver imx_blk_ctrl_driver = {
+	.probe = imx_blk_ctrl_probe,
+	.driver = {
+		.name = "imx-blk-ctrl",
+		.of_match_table = of_match_ptr(imx_blk_ctrl_of_match),
+		.pm = &imx_blk_ctrl_pm_ops,
+	},
+};
+module_platform_driver(imx_blk_ctrl_driver);
diff --git a/drivers/clk/imx/clk-blk-ctrl.h b/drivers/clk/imx/clk-blk-ctrl.h
new file mode 100644
index 00000000..b3b7fc37
--- /dev/null
+++ b/drivers/clk/imx/clk-blk-ctrl.h
@@ -0,0 +1,81 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __MACH_IMX_CLK_BLK_CTRL_H
+#define __MACH_IMX_CLK_BLK_CTRL_H
+
+enum imx_blk_ctrl_hw_type {
+	BLK_CTRL_CLK_MUX,
+	BLK_CTRL_CLK_GATE,
+	BLK_CTRL_CLK_SHARED_GATE,
+	BLK_CTRL_CLK_PLL14XX,
+	BLK_CTRL_RESET,
+};
+
+struct imx_blk_ctrl_hw {
+	int type;
+	char *name;
+	u32 offset;
+	u32 shift;
+	u32 mask;
+	u32 width;
+	u32 flags;
+	u32 id;
+	void *parents;
+	u32 parents_count;
+	int *shared_count;
+	struct imx_pll14xx_clk *pll_tbl;
+};
+
+struct imx_blk_ctrl_dev_data {
+	struct imx_blk_ctrl_hw *hws;
+	u32 hws_num;
+
+	u32 clocks_max;
+	u32 resets_max;
+
+	u32 pm_runtime_saved_regs_num;
+	u32 pm_runtime_saved_regs[];
+};
+
+#define IMX_BLK_CTRL(_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_CTRL_CLK_MUX(_name, _id, _offset, _shift, _width, _parents) \
+	IMX_BLK_CTRL(BLK_CTRL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), 0, NULL, NULL)
+
+#define IMX_BLK_CTRL_CLK_MUX_FLAGS(_name, _id, _offset, _shift, _width, _parents, _flags) \
+	IMX_BLK_CTRL(BLK_CTRL_CLK_MUX, _name, _id, _offset, _shift, _width, 0, _parents, ARRAY_SIZE(_parents), _flags, NULL, NULL)
+
+#define IMX_BLK_CTRL_CLK_GATE(_name, _id, _offset, _shift, _parents) \
+	IMX_BLK_CTRL(BLK_CTRL_CLK_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, NULL, NULL)
+
+#define IMX_BLK_CTRL_CLK_SHARED_GATE(_name, _id, _offset, _shift, _parents, sh_count) \
+	IMX_BLK_CTRL(BLK_CTRL_CLK_SHARED_GATE, _name, _id, _offset, _shift, 1, 0, _parents, 1, 0, sh_count, NULL)
+
+#define IMX_BLK_CTRL_CLK_PLL14XX(_name, _id, _offset, _parents, _pll_tbl) \
+	IMX_BLK_CTRL(BLK_CTRL_CLK_PLL14XX, _name, _id, _offset, 0, 0, 0, _parents, 1, 0, NULL, _pll_tbl)
+
+#define IMX_BLK_CTRL_RESET(_id, _offset, _shift) \
+	IMX_BLK_CTRL(BLK_CTRL_RESET, NULL, _id, _offset, _shift, 0, 1, NULL, 0, 0, NULL, NULL)
+
+#define IMX_BLK_CTRL_RESET_MASK(_id, _offset, _shift, mask) \
+	IMX_BLK_CTRL(BLK_CTRL_RESET, NULL, _id, _offset, _shift, 0, mask, NULL, 0, 0, NULL, NULL)
+
+extern const struct imx_blk_ctrl_dev_data imx8mp_audio_blk_ctrl_dev_data __initconst;
+extern const struct imx_blk_ctrl_dev_data imx8mp_media_blk_ctrl_dev_data __initconst;
+extern const struct imx_blk_ctrl_dev_data imx8mp_hdmi_blk_ctrl_dev_data __initconst;
+
+#endif
+