[v5,00/14] Add BLK_CTL support for i.MX8MP

The BLK_CTL according to HW design is basically the wrapper of the entire
function specific group of IPs and holds GPRs that usually cannot be placed
into one specific IP from that group. Some of these GPRs are used to control
some clocks, other some resets, others some very specific function that does
not fit into clocks or resets. Since the clocks are registered using the i.MX
clock subsystem API, the driver is placed into the clock subsystem, but it
also registers the resets. For the other functionalities that other GPRs might
have, the syscon is used.

Changes since v4:
 * added back the bus_blk_clk in the imx8mp blk_ctl driver (media_blk_ctl)
 * added the R-b tag from Rob to the documentation patch

Abel Vesa (14):
  dt-bindings: clocks: imx8mp: Rename audiomix ids clocks to
    audio_blk_ctl
  dt-bindings: reset: imx8mp: Add audio blk_ctl reset IDs
  dt-bindings: clock: imx8mp: Add ids for the audio shared gate
  dt-bindings: clock: imx8mp: Add media blk_ctl clock IDs
  dt-bindings: reset: imx8mp: Add media blk_ctl reset IDs
  dt-bindings: clock: imx8mp: Add hdmi blk_ctl clock IDs
  dt-bindings: reset: imx8mp: Add hdmi blk_ctl reset IDs
  clk: imx8mp: Add audio shared gate
  Documentation: bindings: clk: Add bindings for i.MX BLK_CTL
  clk: imx: Add generic blk-ctl driver
  clk: imx: Add blk-ctl driver for i.MX8MP
  arm64: dts: imx8mp: Add audio_blk_ctl node
  arm64: dts: imx8mp: Add media_blk_ctl node
  arm64: dts: imx8mp: Add hdmi_blk_ctl node

 .../devicetree/bindings/clock/fsl,imx-blk-ctl.yaml |  60 ++++
 arch/arm64/boot/dts/freescale/imx8mp.dtsi          |  37 +++
 drivers/clk/imx/Makefile                           |   2 +-
 drivers/clk/imx/clk-blk-ctl-imx8mp.c               | 317 +++++++++++++++++++++
 drivers/clk/imx/clk-blk-ctl.c                      | 302 ++++++++++++++++++++
 drivers/clk/imx/clk-blk-ctl.h                      |  80 ++++++
 drivers/clk/imx/clk-imx8mp.c                       |  12 +-
 include/dt-bindings/clock/imx8mp-clock.h           | 200 +++++++++----
 include/dt-bindings/reset/imx8mp-reset.h           |  45 +++
 9 files changed, 992 insertions(+), 63 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/fsl,imx-blk-ctl.yaml
 create mode 100644 drivers/clk/imx/clk-blk-ctl-imx8mp.c
 create mode 100644 drivers/clk/imx/clk-blk-ctl.c
 create mode 100644 drivers/clk/imx/clk-blk-ctl.h

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
>

Quoting Abel Vesa (2020-11-03 03:18:16)
> These will be used by the imx8mp for blk_ctl driver.
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Quoting Abel Vesa (2020-11-03 03:18:18)
> These will be used by the imx8mp for blk_ctl driver.
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Quoting Abel Vesa (2020-11-03 03:18:19)
> These will be used imx8mp for blk_ctl driver.
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Quoting Abel Vesa (2020-11-03 03:18:20)
> According to the RM, the CCGR101 is shared for the following root clocks:
> - AUDIO_AHB_CLK_ROOT
> - AUDIO_AXI_CLK_ROOT
> - SAI2_CLK_ROOT
> - SAI3_CLK_ROOT
> - SAI5_CLK_ROOT
> - SAI6_CLK_ROOT
> - SAI7_CLK_ROOT
> - PDM_CLK_ROOT
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Quoting Abel Vesa (2020-11-03 03:18:23)
> This driver is intended to work with the following BLK_CTL IPs found in
> i.MX8MP:
>  - Audio
>  - Media
>  - HDMI
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

> -----Original Message-----

> From: Abel Vesa [mailto:abel.vesa@nxp.com]

> Sent: Tuesday, November 3, 2020 7:18 PM

> To: Mike Turquette <mturquette@baylibre.com>; Stephen Boyd

> <sboyd@kernel.org>; Adam Ford <aford173@gmail.com>; Marek Vasut

> <marek.vasut@gmail.com>; Lucas Stach <l.stach@pengutronix.de>; Rob

> Herring <robh@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha

> Hauer <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>;

> Anson Huang <anson.huang@nxp.com>; Jacky Bai <ping.bai@nxp.com>; Peng

> Fan <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>

> Cc: dl-linux-imx <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: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver

> 

> The i.MX8MP platform introduces a new type of IP which is called BLK_CTL 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.

> 


From the test I did internally, this blk ctrl implementation will have system deadlock risk due to the clock prepare mutex.
Please make sure it can works without deadlock.

BR
Jacky Bai
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>

> ---

>  drivers/clk/imx/Makefile      |   2 +-

>  drivers/clk/imx/clk-blk-ctl.c | 302

> ++++++++++++++++++++++++++++++++++++++++++

>  drivers/clk/imx/clk-blk-ctl.h |  80 +++++++++++

>  3 files changed, 383 insertions(+), 1 deletion(-)  create mode 100644

> drivers/clk/imx/clk-blk-ctl.c  create mode 100644

> drivers/clk/imx/clk-blk-ctl.h

> 

> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index

> dd6a737..3d6d9cb 100644

> --- a/drivers/clk/imx/Makefile

> +++ b/drivers/clk/imx/Makefile

> @@ -23,7 +23,7 @@ obj-$(CONFIG_MXC_CLK) += mxc-clk.o

> 

>  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-ctl.o

>  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o

> 

>  obj-$(CONFIG_MXC_CLK_SCU) += clk-imx-scu.o clk-imx-lpcg-scu.o 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;

> +	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))

> +		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);

> +

> +	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); } 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;

> +

> +	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))

> +		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];

> +

> +		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)))

> +		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;

> +}

> +

> +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))

> +		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_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; };

> +

> +struct imx_blk_ctl_dev_data {

> +	struct imx_blk_ctl_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_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);

> +

> +#endif

> --

> 2.7.4

On Tue, Nov 3, 2020 at 7:22 PM Abel Vesa <abel.vesa@nxp.com> wrote:
...
> +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;


What if consumers call deassert first in probe which seems common in kernel?
It seems will fail.
e.g.
probe() {
    reset_control_get()
    reset_control_deassert()
}

Regards
Aisheng

> +

> +       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)

> +

> +       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);

> +}

> +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;

> +

> +       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))

> +               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];

> +

> +               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)))

> +               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;

> +}

> +

> +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))

> +               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_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;

> +};

> +

> +struct imx_blk_ctl_dev_data {

> +       struct imx_blk_ctl_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_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);

> +

> +#endif

> --

> 2.7.4

>

On 20-11-11 17:13:25, Dong Aisheng wrote:
> On Tue, Nov 3, 2020 at 7:22 PM Abel Vesa <abel.vesa@nxp.com> wrote:

> ...

> > +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;

> 

> What if consumers call deassert first in probe which seems common in kernel?

> It seems will fail.

> e.g.

> probe() {

>     reset_control_get()

>     reset_control_deassert()

> }

> 

> Regards

> Aisheng

> 


OK, I'm trying to explain here how I know the resets are supposed to be working
and how the BLK_CTL IP is working.


First of, the BLK_CTL bits (resets and clocks) all have the HW init (default) values
as 0. Basically, after the blk_ctl PD is powered on, the resets are deasserted and
clocks are gated by default. Since the blk_ctl is not the parent of any of the
consumers in devicetree (the reg maps are entirely different anyway), there is no
way of ordering the runtime callbacks between the consumer and the blk_ctl. So we
might end up having the runtime resume callback after the one from EARC (consumer),
for example, which will basically overwrite the value written by EARC driver with
whatever was saved on suspend.

Now, about the usage of the reset bits. AFAICT, it would make more sense to assert
the reset, then enable the clock, then deassert. This way, you're keeping the
EARC (consumer) in reset (with the clocks on) until you eventually release it out of
reset by deasserting. This is how the runtime resume should deal with the reset
and the clock. As for the runtime suspend, the reset can be entirely ignored as long
as you're disabling the clock.

This last part will allow the blk_ctl to make the following assumption:
if all the clocks are disabled and none of the reset bits are asserted, I can power off.

Now, I know there are drivers outthere that do assert on suspend, but as long as the 
clocks are disabled, the assert will have no impact. But maybe in their case the reset
controller cannot power down itself.

As for the safekeeping of the register, I'll just drop it due to the following arguments:
1. all the clocks are gated by default
2. all resets are deasserted by default
3. when blk_ctl goes down, all the consumers go down. (all have the same PD)

From 1 and 2 results the IP will not be running and from 3 results the HW state
of every IP becomes HW init state.

> > +

> > +       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)

> > +

> > +       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);

> > +}

> > +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;

> > +

> > +       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))

> > +               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];

> > +

> > +               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)))

> > +               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;

> > +}

> > +

> > +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))

> > +               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_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;

> > +};

> > +

> > +struct imx_blk_ctl_dev_data {

> > +       struct imx_blk_ctl_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_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);

> > +

> > +#endif

> > --

> > 2.7.4

> >

Hi Abel,

On 17.11.20 15:48, Abel Vesa wrote:
> On 20-11-11 17:13:25, Dong Aisheng wrote:

>> On Tue, Nov 3, 2020 at 7:22 PM Abel Vesa <abel.vesa@nxp.com> wrote:

>> ...

>>> +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;

>>

>> What if consumers call deassert first in probe which seems common in kernel?

>> It seems will fail.

>> e.g.

>> probe() {

>>      reset_control_get()

>>      reset_control_deassert()

>> }

>>

>> Regards

>> Aisheng

>>

> 

> OK, I'm trying to explain here how I know the resets are supposed to be working

> and how the BLK_CTL IP is working.

> 

> 

> First of, the BLK_CTL bits (resets and clocks) all have the HW init (default) values

> as 0. Basically, after the blk_ctl PD is powered on, the resets are deasserted and

> clocks are gated by default. Since the blk_ctl is not the parent of any of the

> consumers in devicetree (the reg maps are entirely different anyway), there is no

> way of ordering the runtime callbacks between the consumer and the blk_ctl. So we

> might end up having the runtime resume callback after the one from EARC (consumer),

> for example, which will basically overwrite the value written by EARC driver with

> whatever was saved on suspend.

> 

> Now, about the usage of the reset bits. AFAICT, it would make more sense to assert

> the reset, then enable the clock, then deassert. This way, you're keeping the

> EARC (consumer) in reset (with the clocks on) until you eventually release it out of

> reset by deasserting. This is how the runtime resume should deal with the reset

> and the clock. As for the runtime suspend, the reset can be entirely ignored as long

> as you're disabling the clock.

> 

> This last part will allow the blk_ctl to make the following assumption:

> if all the clocks are disabled and none of the reset bits are asserted, I can power off.

> 

> Now, I know there are drivers outthere that do assert on suspend, but as long as the

> clocks are disabled, the assert will have no impact. But maybe in their case the reset

> controller cannot power down itself.

> 

> As for the safekeeping of the register, I'll just drop it due to the following arguments:

> 1. all the clocks are gated by default

> 2. all resets are deasserted by default

> 3. when blk_ctl goes down, all the consumers go down. (all have the same PD)

> 

>  From 1 and 2 results the IP will not be running and from 3 results the HW state

> of every IP becomes HW init state.


Are there any plans to continue this work? As BLK-CTL it is not only 
relevant for the i.MX8MP, but also for i.MX8MM and i.MX8MN, it would be 
nice to get this ready in order to prepare for proper graphics/display 
support.

Thanks
Frieder

> -----Original Message-----

> From: Frieder Schrempf [mailto:frieder.schrempf@kontron.de]

> Sent: Thursday, February 25, 2021 4:23 PM

> To: Abel Vesa <abel.vesa@nxp.com>; Dong Aisheng <dongas86@gmail.com>

> Cc: Aisheng Dong <aisheng.dong@nxp.com>; Rob Herring <robh@kernel.org>;

> Peng Fan <peng.fan@nxp.com>; Jacky Bai <ping.bai@nxp.com>; Anson Huang

> <anson.huang@nxp.com>; devicetree <devicetree@vger.kernel.org>;

> Stephen Boyd <sboyd@kernel.org>; Shawn Guo <shawnguo@kernel.org>;

> Mike Turquette <mturquette@baylibre.com>; Linux Kernel Mailing List

> <linux-kernel@vger.kernel.org>; Marek Vasut <marek.vasut@gmail.com>;

> dl-linux-imx <linux-imx@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;

> Fabio Estevam <fabio.estevam@nxp.com>; Philipp Zabel

> <p.zabel@pengutronix.de>; Adam Ford <aford173@gmail.com>; linux-clk

> <linux-clk@vger.kernel.org>; moderated list:ARM/FREESCALE IMX / MXC

> ARM ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; Lucas Stach

> <l.stach@pengutronix.de>

> Subject: Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver

> 

> Hi Abel,

> 

> On 17.11.20 15:48, Abel Vesa wrote:

> > On 20-11-11 17:13:25, Dong Aisheng wrote:

> >> On Tue, Nov 3, 2020 at 7:22 PM Abel Vesa <abel.vesa@nxp.com> wrote:

> >> ...

> >>> +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;

> >>

> >> What if consumers call deassert first in probe which seems common in

> kernel?

> >> It seems will fail.

> >> e.g.

> >> probe() {

> >>      reset_control_get()

> >>      reset_control_deassert()

> >> }

> >>

> >> Regards

> >> Aisheng

> >>

> >

> > OK, I'm trying to explain here how I know the resets are supposed to

> > be working and how the BLK_CTL IP is working.

> >

> >

> > First of, the BLK_CTL bits (resets and clocks) all have the HW init

> > (default) values as 0. Basically, after the blk_ctl PD is powered on,

> > the resets are deasserted and clocks are gated by default. Since the

> > blk_ctl is not the parent of any of the consumers in devicetree (the

> > reg maps are entirely different anyway), there is no way of ordering

> > the runtime callbacks between the consumer and the blk_ctl. So we

> > might end up having the runtime resume callback after the one from

> > EARC (consumer), for example, which will basically overwrite the value

> written by EARC driver with whatever was saved on suspend.

> >

> > Now, about the usage of the reset bits. AFAICT, it would make more

> > sense to assert the reset, then enable the clock, then deassert. This

> > way, you're keeping the EARC (consumer) in reset (with the clocks on)

> > until you eventually release it out of reset by deasserting. This is

> > how the runtime resume should deal with the reset and the clock. As

> > for the runtime suspend, the reset can be entirely ignored as long as you're

> disabling the clock.

> >

> > This last part will allow the blk_ctl to make the following assumption:

> > if all the clocks are disabled and none of the reset bits are asserted, I can

> power off.

> >

> > Now, I know there are drivers outthere that do assert on suspend, but

> > as long as the clocks are disabled, the assert will have no impact.

> > But maybe in their case the reset controller cannot power down itself.

> >

> > As for the safekeeping of the register, I'll just drop it due to the following

> arguments:

> > 1. all the clocks are gated by default 2. all resets are deasserted by

> > default 3. when blk_ctl goes down, all the consumers go down. (all

> > have the same PD)

> >

> >  From 1 and 2 results the IP will not be running and from 3 results

> > the HW state of every IP becomes HW init state.

> 

> Are there any plans to continue this work? As BLK-CTL it is not only relevant

> for the i.MX8MP, but also for i.MX8MM and i.MX8MN, it would be nice to get

> this ready in order to prepare for proper graphics/display support.

> 


Before continuing this work, we need to find out a way to resolve the cycling dependency issue between power domain and blk-ctrl.
it is indeed introduced some troubles in NXP latest internal release when the blk-ctrl driver is added.

BR
Jacky Bai

> Thanks

> Frieder

On 20-11-03 13:18:12, Abel Vesa wrote:
> The BLK_CTL according to HW design is basically the wrapper of the entire

> function specific group of IPs and holds GPRs that usually cannot be placed

> into one specific IP from that group. Some of these GPRs are used to control

> some clocks, other some resets, others some very specific function that does

> not fit into clocks or resets. Since the clocks are registered using the i.MX

> clock subsystem API, the driver is placed into the clock subsystem, but it

> also registers the resets. For the other functionalities that other GPRs might

> have, the syscon is used.

> 


This approach seems to be introducing a possible ABBA deadlock due to
the core clock and genpd locking. Here is a backtrace I got from Pete
Zhang (he reported the issue on the internal mailing list):

[   11.667711][  T108] -> #1 (&genpd->mlock){+.+.}-{3:3}:
[   11.675041][  T108]        __lock_acquire+0xae4/0xef8
[   11.680093][  T108]        lock_acquire+0xfc/0x2f8
[   11.684888][  T108]        __mutex_lock+0x90/0x870
[   11.689685][  T108]        mutex_lock_nested+0x44/0x50
[   11.694826][  T108]        genpd_lock_mtx+0x18/0x24
[   11.699706][  T108]        genpd_runtime_resume+0x90/0x214 (hold genpd->mlock)
[   11.705194][  T108]        __rpm_callback+0x80/0x2c0
[   11.710160][  T108]        rpm_resume+0x468/0x650
[   11.714866][  T108]        __pm_runtime_resume+0x60/0x88
[   11.720180][  T108]        clk_pm_runtime_get+0x28/0x9c
[   11.725410][  T108]        clk_disable_unused_subtree+0x8c/0x144
[   11.731420][  T108]        clk_disable_unused_subtree+0x124/0x144
[   11.737518][  T108]        clk_disable_unused+0xa4/0x11c (hold prepare_lock)
[   11.742833][  T108]        do_one_initcall+0x98/0x178
[   11.747888][  T108]        do_initcall_level+0x9c/0xb8
[   11.753028][  T108]        do_initcalls+0x54/0x94
[   11.757736][  T108]        do_basic_setup+0x24/0x30
[   11.762614][  T108]        kernel_init_freeable+0x70/0xa4
[   11.768014][  T108]        kernel_init+0x14/0x18c
[   11.772722][  T108]        ret_from_fork+0x10/0x18

[   11.777512][  T108] -> #0 (prepare_lock){+.+.}-{3:3}:
[   11.784749][  T108]        check_noncircular+0x134/0x13c
[   11.790064][  T108]        validate_chain+0x590/0x2a04
[   11.795204][  T108]        __lock_acquire+0xae4/0xef8
[   11.800258][  T108]        lock_acquire+0xfc/0x2f8
[   11.805050][  T108]        __mutex_lock+0x90/0x870
[   11.809841][  T108]        mutex_lock_nested+0x44/0x50
[   11.814983][  T108]        clk_unprepare+0x5c/0x100 ((hold prepare_lock))
[   11.819864][  T108]        imx8m_pd_power_off+0xac/0x110
[   11.825179][  T108]        genpd_power_off+0x1b4/0x2dc
[   11.830318][  T108]        genpd_power_off_work_fn+0x38/0x58 (hold genpd->mlock)
[   11.835981][  T108]        process_one_work+0x270/0x444
[   11.841208][  T108]        worker_thread+0x280/0x4e4
[   11.846176][  T108]        kthread+0x13c/0x14
[   11.850621][  T108]        ret_from_fork+0x10/0x18

Now, this has been reproduced only on the NXP internal tree, but I think
it is pretty obvious this could happen in upstream too, with this
patchset applied.

First, my thought was to change the prepare_lock/enable_lock in clock
core, from a global approach to a per clock basis. But that doesn't
actually fix the issue.

The usecase seen above is due to clk_disable_unused, but the same could
happen when a clock consumer calls prepare/unprepare on a clock.

I guess the conclusion is that the current state of the clock core and
genpd implementation does not support a usecase where a clock controller
has a PD which in turn uses another clock (from another clock controller).

Jacky, Pete, did I miss anything here ?

> Changes since v4:

>  * added back the bus_blk_clk in the imx8mp blk_ctl driver (media_blk_ctl)

>  * added the R-b tag from Rob to the documentation patch

> 

> Abel Vesa (14):

>   dt-bindings: clocks: imx8mp: Rename audiomix ids clocks to

>     audio_blk_ctl

>   dt-bindings: reset: imx8mp: Add audio blk_ctl reset IDs

>   dt-bindings: clock: imx8mp: Add ids for the audio shared gate

>   dt-bindings: clock: imx8mp: Add media blk_ctl clock IDs

>   dt-bindings: reset: imx8mp: Add media blk_ctl reset IDs

>   dt-bindings: clock: imx8mp: Add hdmi blk_ctl clock IDs

>   dt-bindings: reset: imx8mp: Add hdmi blk_ctl reset IDs

>   clk: imx8mp: Add audio shared gate

>   Documentation: bindings: clk: Add bindings for i.MX BLK_CTL

>   clk: imx: Add generic blk-ctl driver

>   clk: imx: Add blk-ctl driver for i.MX8MP

>   arm64: dts: imx8mp: Add audio_blk_ctl node

>   arm64: dts: imx8mp: Add media_blk_ctl node

>   arm64: dts: imx8mp: Add hdmi_blk_ctl node

> 

>  .../devicetree/bindings/clock/fsl,imx-blk-ctl.yaml |  60 ++++

>  arch/arm64/boot/dts/freescale/imx8mp.dtsi          |  37 +++

>  drivers/clk/imx/Makefile                           |   2 +-

>  drivers/clk/imx/clk-blk-ctl-imx8mp.c               | 317 +++++++++++++++++++++

>  drivers/clk/imx/clk-blk-ctl.c                      | 302 ++++++++++++++++++++

>  drivers/clk/imx/clk-blk-ctl.h                      |  80 ++++++

>  drivers/clk/imx/clk-imx8mp.c                       |  12 +-

>  include/dt-bindings/clock/imx8mp-clock.h           | 200 +++++++++----

>  include/dt-bindings/reset/imx8mp-reset.h           |  45 +++

>  9 files changed, 992 insertions(+), 63 deletions(-)

>  create mode 100644 Documentation/devicetree/bindings/clock/fsl,imx-blk-ctl.yaml

>  create mode 100644 drivers/clk/imx/clk-blk-ctl-imx8mp.c

>  create mode 100644 drivers/clk/imx/clk-blk-ctl.c

>  create mode 100644 drivers/clk/imx/clk-blk-ctl.h

> 

> -- 

> 2.7.4

>

On 3/3/21 11:47 AM, Abel Vesa wrote:
> On 20-11-03 13:18:12, Abel Vesa wrote:
>> The BLK_CTL according to HW design is basically the wrapper of the entire
>> function specific group of IPs and holds GPRs that usually cannot be placed
>> into one specific IP from that group. Some of these GPRs are used to control
>> some clocks, other some resets, others some very specific function that does
>> not fit into clocks or resets. Since the clocks are registered using the i.MX
>> clock subsystem API, the driver is placed into the clock subsystem, but it
>> also registers the resets. For the other functionalities that other GPRs might
>> have, the syscon is used.
>>
> 
> This approach seems to be introducing a possible ABBA deadlock due to
> the core clock and genpd locking. Here is a backtrace I got from Pete
> Zhang (he reported the issue on the internal mailing list):
> 
> [   11.667711][  T108] -> #1 (&genpd->mlock){+.+.}-{3:3}:
> [   11.675041][  T108]        __lock_acquire+0xae4/0xef8
> [   11.680093][  T108]        lock_acquire+0xfc/0x2f8
> [   11.684888][  T108]        __mutex_lock+0x90/0x870
> [   11.689685][  T108]        mutex_lock_nested+0x44/0x50
> [   11.694826][  T108]        genpd_lock_mtx+0x18/0x24
> [   11.699706][  T108]        genpd_runtime_resume+0x90/0x214 (hold genpd->mlock)
> [   11.705194][  T108]        __rpm_callback+0x80/0x2c0
> [   11.710160][  T108]        rpm_resume+0x468/0x650
> [   11.714866][  T108]        __pm_runtime_resume+0x60/0x88
> [   11.720180][  T108]        clk_pm_runtime_get+0x28/0x9c
> [   11.725410][  T108]        clk_disable_unused_subtree+0x8c/0x144
> [   11.731420][  T108]        clk_disable_unused_subtree+0x124/0x144
> [   11.737518][  T108]        clk_disable_unused+0xa4/0x11c (hold prepare_lock)
> [   11.742833][  T108]        do_one_initcall+0x98/0x178
> [   11.747888][  T108]        do_initcall_level+0x9c/0xb8
> [   11.753028][  T108]        do_initcalls+0x54/0x94
> [   11.757736][  T108]        do_basic_setup+0x24/0x30
> [   11.762614][  T108]        kernel_init_freeable+0x70/0xa4
> [   11.768014][  T108]        kernel_init+0x14/0x18c
> [   11.772722][  T108]        ret_from_fork+0x10/0x18
> 
> [   11.777512][  T108] -> #0 (prepare_lock){+.+.}-{3:3}:
> [   11.784749][  T108]        check_noncircular+0x134/0x13c
> [   11.790064][  T108]        validate_chain+0x590/0x2a04
> [   11.795204][  T108]        __lock_acquire+0xae4/0xef8
> [   11.800258][  T108]        lock_acquire+0xfc/0x2f8
> [   11.805050][  T108]        __mutex_lock+0x90/0x870
> [   11.809841][  T108]        mutex_lock_nested+0x44/0x50
> [   11.814983][  T108]        clk_unprepare+0x5c/0x100 ((hold prepare_lock))
> [   11.819864][  T108]        imx8m_pd_power_off+0xac/0x110
> [   11.825179][  T108]        genpd_power_off+0x1b4/0x2dc
> [   11.830318][  T108]        genpd_power_off_work_fn+0x38/0x58 (hold genpd->mlock)
> [   11.835981][  T108]        process_one_work+0x270/0x444
> [   11.841208][  T108]        worker_thread+0x280/0x4e4
> [   11.846176][  T108]        kthread+0x13c/0x14
> [   11.850621][  T108]        ret_from_fork+0x10/0x18
> 
> Now, this has been reproduced only on the NXP internal tree, but I think
> it is pretty obvious this could happen in upstream too, with this
> patchset applied.
> 
> First, my thought was to change the prepare_lock/enable_lock in clock
> core, from a global approach to a per clock basis. But that doesn't
> actually fix the issue.
> 
> The usecase seen above is due to clk_disable_unused, but the same could
> happen when a clock consumer calls prepare/unprepare on a clock.
> 
> I guess the conclusion is that the current state of the clock core and
> genpd implementation does not support a usecase where a clock controller
> has a PD which in turn uses another clock (from another clock controller).
> 
> Jacky, Pete, did I miss anything here ?

Just for completeness, I have a feeling I already managed to trigger 
this and discussed this with Lucas before, so this concern is certainly 
valid.

On Thu, Feb 25, 2021 at 12:28 AM Jacky Bai <ping.bai@nxp.com> wrote:
>

>

>

> > -----Original Message-----

> > From: Frieder Schrempf [mailto:frieder.schrempf@kontron.de]

> > Sent: Thursday, February 25, 2021 4:23 PM

> > To: Abel Vesa <abel.vesa@nxp.com>; Dong Aisheng <dongas86@gmail.com>

> > Cc: Aisheng Dong <aisheng.dong@nxp.com>; Rob Herring <robh@kernel.org>;

> > Peng Fan <peng.fan@nxp.com>; Jacky Bai <ping.bai@nxp.com>; Anson Huang

> > <anson.huang@nxp.com>; devicetree <devicetree@vger.kernel.org>;

> > Stephen Boyd <sboyd@kernel.org>; Shawn Guo <shawnguo@kernel.org>;

> > Mike Turquette <mturquette@baylibre.com>; Linux Kernel Mailing List

> > <linux-kernel@vger.kernel.org>; Marek Vasut <marek.vasut@gmail.com>;

> > dl-linux-imx <linux-imx@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;

> > Fabio Estevam <fabio.estevam@nxp.com>; Philipp Zabel

> > <p.zabel@pengutronix.de>; Adam Ford <aford173@gmail.com>; linux-clk

> > <linux-clk@vger.kernel.org>; moderated list:ARM/FREESCALE IMX / MXC

> > ARM ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; Lucas Stach

> > <l.stach@pengutronix.de>

> > Subject: Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver

> >

> > Hi Abel,

> >

> > On 17.11.20 15:48, Abel Vesa wrote:

> > > On 20-11-11 17:13:25, Dong Aisheng wrote:

> > >> On Tue, Nov 3, 2020 at 7:22 PM Abel Vesa <abel.vesa@nxp.com> wrote:

> > >> ...

> > >>> +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;

> > >>

> > >> What if consumers call deassert first in probe which seems common in

> > kernel?

> > >> It seems will fail.

> > >> e.g.

> > >> probe() {

> > >>      reset_control_get()

> > >>      reset_control_deassert()

> > >> }

> > >>

> > >> Regards

> > >> Aisheng

> > >>

> > >

> > > OK, I'm trying to explain here how I know the resets are supposed to

> > > be working and how the BLK_CTL IP is working.

> > >

> > >

> > > First of, the BLK_CTL bits (resets and clocks) all have the HW init

> > > (default) values as 0. Basically, after the blk_ctl PD is powered on,

> > > the resets are deasserted and clocks are gated by default. Since the

> > > blk_ctl is not the parent of any of the consumers in devicetree (the

> > > reg maps are entirely different anyway), there is no way of ordering

> > > the runtime callbacks between the consumer and the blk_ctl. So we

> > > might end up having the runtime resume callback after the one from

> > > EARC (consumer), for example, which will basically overwrite the value

> > written by EARC driver with whatever was saved on suspend.

> > >

> > > Now, about the usage of the reset bits. AFAICT, it would make more

> > > sense to assert the reset, then enable the clock, then deassert. This

> > > way, you're keeping the EARC (consumer) in reset (with the clocks on)

> > > until you eventually release it out of reset by deasserting. This is

> > > how the runtime resume should deal with the reset and the clock. As

> > > for the runtime suspend, the reset can be entirely ignored as long as you're

> > disabling the clock.

> > >

> > > This last part will allow the blk_ctl to make the following assumption:

> > > if all the clocks are disabled and none of the reset bits are asserted, I can

> > power off.

> > >

> > > Now, I know there are drivers outthere that do assert on suspend, but

> > > as long as the clocks are disabled, the assert will have no impact.

> > > But maybe in their case the reset controller cannot power down itself.

> > >

> > > As for the safekeeping of the register, I'll just drop it due to the following

> > arguments:

> > > 1. all the clocks are gated by default 2. all resets are deasserted by

> > > default 3. when blk_ctl goes down, all the consumers go down. (all

> > > have the same PD)

> > >

> > >  From 1 and 2 results the IP will not be running and from 3 results

> > > the HW state of every IP becomes HW init state.

> >

> > Are there any plans to continue this work? As BLK-CTL it is not only relevant

> > for the i.MX8MP, but also for i.MX8MM and i.MX8MN, it would be nice to get

> > this ready in order to prepare for proper graphics/display support.

> >

>

> Before continuing this work, we need to find out a way to resolve the cycling dependency issue between power domain and blk-ctrl.

> it is indeed introduced some troubles in NXP latest internal release when the blk-ctrl driver is added.

>


Jacky,

Any update on this? This is still blocking several drivers and major
functionality of the i.MX8 SoC's in mainline and I would hope this
would be a top priority for NXP.

Best regards,

Tim

On Wed, Mar 3, 2021 at 4:54 AM Marek Vasut <marex@denx.de> wrote:
>
> On 3/3/21 11:47 AM, Abel Vesa wrote:
> > On 20-11-03 13:18:12, Abel Vesa wrote:
> >> The BLK_CTL according to HW design is basically the wrapper of the entire
> >> function specific group of IPs and holds GPRs that usually cannot be placed
> >> into one specific IP from that group. Some of these GPRs are used to control
> >> some clocks, other some resets, others some very specific function that does
> >> not fit into clocks or resets. Since the clocks are registered using the i.MX
> >> clock subsystem API, the driver is placed into the clock subsystem, but it
> >> also registers the resets. For the other functionalities that other GPRs might
> >> have, the syscon is used.
> >>
> >
> > This approach seems to be introducing a possible ABBA deadlock due to
> > the core clock and genpd locking. Here is a backtrace I got from Pete
> > Zhang (he reported the issue on the internal mailing list):
> >
> > [   11.667711][  T108] -> #1 (&genpd->mlock){+.+.}-{3:3}:
> > [   11.675041][  T108]        __lock_acquire+0xae4/0xef8
> > [   11.680093][  T108]        lock_acquire+0xfc/0x2f8
> > [   11.684888][  T108]        __mutex_lock+0x90/0x870
> > [   11.689685][  T108]        mutex_lock_nested+0x44/0x50
> > [   11.694826][  T108]        genpd_lock_mtx+0x18/0x24
> > [   11.699706][  T108]        genpd_runtime_resume+0x90/0x214 (hold genpd->mlock)
> > [   11.705194][  T108]        __rpm_callback+0x80/0x2c0
> > [   11.710160][  T108]        rpm_resume+0x468/0x650
> > [   11.714866][  T108]        __pm_runtime_resume+0x60/0x88
> > [   11.720180][  T108]        clk_pm_runtime_get+0x28/0x9c
> > [   11.725410][  T108]        clk_disable_unused_subtree+0x8c/0x144
> > [   11.731420][  T108]        clk_disable_unused_subtree+0x124/0x144
> > [   11.737518][  T108]        clk_disable_unused+0xa4/0x11c (hold prepare_lock)
> > [   11.742833][  T108]        do_one_initcall+0x98/0x178
> > [   11.747888][  T108]        do_initcall_level+0x9c/0xb8
> > [   11.753028][  T108]        do_initcalls+0x54/0x94
> > [   11.757736][  T108]        do_basic_setup+0x24/0x30
> > [   11.762614][  T108]        kernel_init_freeable+0x70/0xa4
> > [   11.768014][  T108]        kernel_init+0x14/0x18c
> > [   11.772722][  T108]        ret_from_fork+0x10/0x18
> >
> > [   11.777512][  T108] -> #0 (prepare_lock){+.+.}-{3:3}:
> > [   11.784749][  T108]        check_noncircular+0x134/0x13c
> > [   11.790064][  T108]        validate_chain+0x590/0x2a04
> > [   11.795204][  T108]        __lock_acquire+0xae4/0xef8
> > [   11.800258][  T108]        lock_acquire+0xfc/0x2f8
> > [   11.805050][  T108]        __mutex_lock+0x90/0x870
> > [   11.809841][  T108]        mutex_lock_nested+0x44/0x50
> > [   11.814983][  T108]        clk_unprepare+0x5c/0x100 ((hold prepare_lock))
> > [   11.819864][  T108]        imx8m_pd_power_off+0xac/0x110
> > [   11.825179][  T108]        genpd_power_off+0x1b4/0x2dc
> > [   11.830318][  T108]        genpd_power_off_work_fn+0x38/0x58 (hold genpd->mlock)
> > [   11.835981][  T108]        process_one_work+0x270/0x444
> > [   11.841208][  T108]        worker_thread+0x280/0x4e4
> > [   11.846176][  T108]        kthread+0x13c/0x14
> > [   11.850621][  T108]        ret_from_fork+0x10/0x18
> >
> > Now, this has been reproduced only on the NXP internal tree, but I think
> > it is pretty obvious this could happen in upstream too, with this
> > patchset applied.
> >
> > First, my thought was to change the prepare_lock/enable_lock in clock
> > core, from a global approach to a per clock basis. But that doesn't
> > actually fix the issue.
> >
> > The usecase seen above is due to clk_disable_unused, but the same could
> > happen when a clock consumer calls prepare/unprepare on a clock.
> >
> > I guess the conclusion is that the current state of the clock core and
> > genpd implementation does not support a usecase where a clock controller
> > has a PD which in turn uses another clock (from another clock controller).
> >
> > Jacky, Pete, did I miss anything here ?
>
> Just for completeness, I have a feeling I already managed to trigger
> this and discussed this with Lucas before, so this concern is certainly
> valid.

I know it may not be ideal, someone tied a soft-reset and soft-enable
to the driver of the Hantro VPU on the IMXMQ [1], and I wonder if
something similar could be done for the drivers who are consumers of
the clocks.

For example:

lcdif could request the power domain.
The power domain soft-resets and enables bus clock (vis syscon)
After successful enabling of power-domain, the LCDIF requests the soft
reset and respective clock bits (also via syscon) similar to how [1]
and [2] do it for the Hantro VPU.

The syscon node could be a common node similar to what was done in
[2], and multiple consumers could control when each soft-reset and
clock-enable get activated.  I know it's probably more of an abuse of
the syscon system, but having the individual drivers control the order
of operations might be safer than trying to create a one-size-fits-all
driver.

adam
[1] - https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210318082046.51546-4-benjamin.gaignard@collabora.com/
[2] - https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210318082046.51546-14-benjamin.gaignard@collabora.com/