Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: Philipp Zabel <p.zabel@pengutronix.de>
To: Abel Vesa <abel.vesa@nxp.com>,
	Mike Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, 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>,
	Dong Aisheng <aisheng.dong@nxp.com>, Peng Fan <peng.fan@nxp.com>,
	Fugang Duan <fugang.duan@nxp.com>
Cc: NXP Linux Team <linux-imx@nxp.com>,
	linux-arm-kernel@lists.infradead.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 11/17] clk: imx: Add blk_ctrl combo driver
Date: Wed, 29 Jul 2020 14:46:28 +0200
Message-ID: <d44e88a1408add6491897a8793b57ee0090fa4c6.camel@pengutronix.de> (raw)
In-Reply-To: <1596024483-21482-12-git-send-email-abel.vesa@nxp.com>

Hi Abel,

On Wed, 2020-07-29 at 15:07 +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 | 318 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/imx/clk-blk-ctrl.h |  81 +++++++++++
>  3 files changed, 400 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..a46e674
> --- /dev/null
> +++ b/drivers/clk/imx/clk-blk-ctrl.c
> @@ -0,0 +1,318 @@
> +// 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;
> +};
> +
> +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;
> +	u32 reg;
> +
> +	if (assert) {
> +		pm_runtime_get_sync(rcdev->dev);
> +		spin_lock_irqsave(&drvdata->lock, flags);
> +		reg = readl(reg_addr);
> +		writel(reg & ~(mask << shift), reg_addr);
> +		spin_unlock_irqrestore(&drvdata->lock, flags);
> +	} else {
> +		spin_lock_irqsave(&drvdata->lock, flags);
> +		reg = readl(reg_addr);
> +		writel(reg | (mask << shift), reg_addr);
> +		spin_unlock_irqrestore(&drvdata->lock, flags);
> +		pm_runtime_put(rcdev->dev);

This still has the issue of potentially letting exclusive reset control
users break the device usage counter.

Also shared reset control users start with deassert(), and you end probe
with pm_runtime_put(), so the first shared reset control user that
deasserts its reset will decrement the dev->power.usage_count to -1 ?
For multiple resets being initially deasserted this would decrement
multiple times.

I think you'll have to track the (number of) asserted reset bits in this
reset controller and limit when to call pm_runtime_get/put_sync().

> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_blk_ctrl_reset_reset(struct reset_controller_dev *rcdev,
> +					   unsigned long id)
> +{
> +	imx_blk_ctrl_reset_set(rcdev, id, true);
> +	return imx_blk_ctrl_reset_set(rcdev, id, false);

Does this work for all peripherals? Are there none that require the
reset line to be asserted for a certain number of bus clocks or similar?

> +}
> +
> +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 = {
> +	.reset		= imx_blk_ctrl_reset_reset,
> +	.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_kzalloc(dev, sizeof(struct reset_hw) * max,
> +					GFP_KERNEL);

I'd use devm_kcalloc() here.

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

regards
Philipp

  reply index

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-29 12:07 [PATCH 00/17] Add BLK_CTRL support for i.MX8MP Abel Vesa
2020-07-29 12:07 ` [PATCH 01/17] dt-bindings: clocks: imx8mp: Rename audiomix ids clocks to audio_blk_ctrl Abel Vesa
2020-07-29 19:47   ` Stephen Boyd
2020-07-30  7:29     ` Abel Vesa
2020-07-29 12:07 ` [PATCH 02/17] dt-bindings: reset: imx8mp: Add audio blk_ctrl reset IDs Abel Vesa
2020-07-29 12:07 ` [PATCH 03/17] dt-bindings: clock: imx8mp: Add ids for the audio shared gate Abel Vesa
2020-07-31 22:22   ` Rob Herring
2020-07-29 12:07 ` [PATCH 04/17] dt-bindings: clock: imx8mp: Add media blk_ctrl clock IDs Abel Vesa
2020-07-31 22:22   ` Rob Herring
2020-07-29 12:07 ` [PATCH 05/17] dt-bindings: reset: imx8mp: Add media blk_ctrl reset IDs Abel Vesa
2020-07-31 22:23   ` Rob Herring
2020-07-29 12:07 ` [PATCH 06/17] dt-bindings: clock: imx8mp: Add hdmi blk_ctrl clock IDs Abel Vesa
2020-07-31 22:23   ` Rob Herring
2020-07-29 12:07 ` [PATCH 07/17] dt-bindings: reset: imx8mp: Add hdmi blk_ctrl reset IDs Abel Vesa
2020-07-31 22:24   ` Rob Herring
2020-07-29 12:07 ` [PATCH 08/17] clk: imx8mp: Add audio shared gate Abel Vesa
2020-07-29 12:07 ` [PATCH 09/17] arm64: dts: Remove imx-hdmimix-reset header file Abel Vesa
2020-07-29 12:07 ` [PATCH 10/17] Documentation: bindings: clk: Add bindings for i.MX BLK_CTRL Abel Vesa
2020-07-29 19:49   ` Stephen Boyd
2020-07-30  7:30     ` Abel Vesa
2020-07-31 18:20   ` Rob Herring
2020-07-29 12:07 ` [PATCH 11/17] clk: imx: Add blk_ctrl combo driver Abel Vesa
2020-07-29 12:46   ` Philipp Zabel [this message]
2020-07-30  8:55     ` Abel Vesa
2020-07-30  9:39       ` Philipp Zabel
2020-08-12  7:28         ` Abel Vesa
2020-07-29 12:07 ` [PATCH 12/17] clk: imx8mp: Add audio blk_ctrl clocks and resets Abel Vesa
2020-07-29 12:07 ` [PATCH 13/17] clk: imx8mp: Add hdmi " Abel Vesa
2020-07-29 12:08 ` [PATCH 14/17] clk: imx8mp: Add media " Abel Vesa
2020-07-29 12:08 ` [PATCH 15/17] arm64: dts: imx8mp: Add audio_blk_ctrl node Abel Vesa
2020-07-29 12:16   ` Abel Vesa
2020-07-29 12:08 ` [PATCH 16/17] arm64: dts: imx8mp: Add media_blk_ctrl node Abel Vesa
2020-07-29 12:17   ` Abel Vesa
2020-07-29 12:08 ` [PATCH 17/17] arm64: dts: imx8mp: Add hdmi_blk_ctrl node Abel Vesa
2020-07-29 12:17   ` Abel Vesa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d44e88a1408add6491897a8793b57ee0090fa4c6.camel@pengutronix.de \
    --to=p.zabel@pengutronix.de \
    --cc=abel.vesa@nxp.com \
    --cc=aisheng.dong@nxp.com \
    --cc=anson.huang@nxp.com \
    --cc=devicetree@vger.kernel.org \
    --cc=fabio.estevam@nxp.com \
    --cc=fugang.duan@nxp.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=peng.fan@nxp.com \
    --cc=robh@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=shawnguo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git