LKML Archive on lore.kernel.org
 help / Atom feed
From: Matthias Kaehlcke <mka@chromium.org>
To: Sibi Sankar <sibis@codeaurora.org>
Cc: bjorn.andersson@linaro.org, p.zabel@pengutronix.de,
	robh+dt@kernel.org, linux-remoteproc@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	ohad@wizery.com, mark.rutland@arm.com, sricharan@codeaurora.org,
	akdwived@codeaurora.org, linux-arm-msm@vger.kernel.org,
	tsoni@codeaurora.org
Subject: Re: [PATCH v2 2/6] reset: qcom: PDC Global (Power Domain Controller) reset controller
Date: Mon, 27 Aug 2018 17:22:36 -0700
Message-ID: <20180828002236.GX160295@google.com> (raw)
In-Reply-To: <20180824131900.5353-3-sibis@codeaurora.org>

Hi Sibi,

On Fri, Aug 24, 2018 at 06:48:56PM +0530, Sibi Sankar wrote:
> Add reset controller for SDM845 SoCs to control reset signals provided
> by PDC Global for Modem, Compute, Display, GPU, Debug, AOP, Sensors,
> Audio, SP and APPS
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/reset/Kconfig          |   9 +++
>  drivers/reset/Makefile         |   1 +
>  drivers/reset/reset-qcom-pdc.c | 142 +++++++++++++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/reset/reset-qcom-pdc.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 13d28fdbdbb5..c21da9fe51ec 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -98,6 +98,15 @@ config RESET_QCOM_AOSS
>  	  reset signals provided by AOSS for Modem, Venus, ADSP,
>  	  GPU, Camera, Wireless, Display subsystem. Otherwise, say N.
>  
> +config RESET_QCOM_PDC
> +	tristate "Qualcomm PDC Reset Driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This enables the PDC (Power Domain Controller) reset driver
> +	  for Qualcomm Technologies Inc SDM845 SoCs. Say Y if you want
> +	  to control reset signals provided by PDC for Modem, Compute,
> +	  Display, GPU, Debug, AOP, Sensors, Audio, SP and APPS.

What exactly does APPS mean? The AP cores, the entire SoC, something
else?

> +
>  config RESET_SIMPLE
>  	bool "Simple Reset Controller Driver" if COMPILE_TEST
>  	default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARCH_ASPEED
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4243c38228e2..d08e8b90046a 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
>  obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
>  obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
>  obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> +obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
>  obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
>  obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
>  obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> diff --git a/drivers/reset/reset-qcom-pdc.c b/drivers/reset/reset-qcom-pdc.c
> new file mode 100644
> index 000000000000..bb6a5e5ee0f8
> --- /dev/null
> +++ b/drivers/reset/reset-qcom-pdc.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/of_device.h>
> +#include <dt-bindings/reset/qcom,sdm845-pdc.h>

Headers should be sorted in alphabetical order.

> +
> +#define RPMH_PDC_SYNC_RESET	0x100
> +
> +struct qcom_pdc_reset_map {
> +	u8 bit;
> +};
> +
> +struct qcom_pdc_desc {
> +	const struct regmap_config *config;
> +	const struct qcom_pdc_reset_map *resets;
> +	size_t num_resets;
> +};

Not sure if this structure adds much value or just a layer of
indirection:

- .config is only accessed in _probe(), sdm845_pdc_regmap_config could
  be used directly
- .resets is used in _(de)assert(), sdm845_pdc_resets could be used
  directly
- .num_resets is only accessed in _probe(),
  ARRAY_SIZE(sdm845_pdc_resets) could be used instead

It probably makes sense if it is planned to support reset controllers
of other SoCs with this driver.

> +struct qcom_pdc_reset_data {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +	const struct qcom_pdc_desc *desc;
> +};
> +
> +static const struct regmap_config sdm845_pdc_regmap_config = {
> +	.name		= "pdc-reset",
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x20000,
> +	.fast_io	= true,
> +};
> +
> +static const struct qcom_pdc_reset_map sdm845_pdc_resets[] = {
> +	[PDC_APPS_SYNC_RESET] = {0},
> +	[PDC_SP_SYNC_RESET] = {1},
> +	[PDC_AUDIO_SYNC_RESET] = {2},
> +	[PDC_SENSORS_SYNC_RESET] = {3},
> +	[PDC_AOP_SYNC_RESET] = {4},
> +	[PDC_DEBUG_SYNC_RESET] = {5},
> +	[PDC_GPU_SYNC_RESET] = {6},
> +	[PDC_DISPLAY_SYNC_RESET] = {7},
> +	[PDC_COMPUTE_SYNC_RESET] = {8},
> +	[PDC_MODEM_SYNC_RESET] = {9},
> +};
> +
> +static const struct qcom_pdc_desc sdm845_pdc_desc = {
> +	.config = &sdm845_pdc_regmap_config,
> +	.resets = sdm845_pdc_resets,
> +	.num_resets = ARRAY_SIZE(sdm845_pdc_resets),
> +};
> +
> +static inline struct qcom_pdc_reset_data *to_qcom_pdc_reset_data(
> +				struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct qcom_pdc_reset_data, rcdev);
> +}
> +
> +static int qcom_pdc_control_assert(struct reset_controller_dev *rcdev,
> +					unsigned long idx)
> +{
> +	struct qcom_pdc_reset_data *data = to_qcom_pdc_reset_data(rcdev);
> +	const struct qcom_pdc_reset_map *map = &data->desc->resets[idx];
> +
> +	return regmap_update_bits(data->regmap, RPMH_PDC_SYNC_RESET,
> +					BIT(map->bit), BIT(map->bit));
> +}
> +
> +static int qcom_pdc_control_deassert(struct reset_controller_dev *rcdev,
> +					unsigned long idx)
> +{
> +	struct qcom_pdc_reset_data *data = to_qcom_pdc_reset_data(rcdev);
> +	const struct qcom_pdc_reset_map *map = &data->desc->resets[idx];
> +
> +	return regmap_update_bits(data->regmap, RPMH_PDC_SYNC_RESET,
> +					BIT(map->bit), 0);
> +}
> +
> +static const struct reset_control_ops qcom_pdc_reset_ops = {
> +	.assert = qcom_pdc_control_assert,
> +	.deassert = qcom_pdc_control_deassert,
> +};
> +
> +static int qcom_pdc_reset_probe(struct platform_device *pdev)
> +{
> +	struct qcom_pdc_reset_data *data;
> +	struct device *dev = &pdev->dev;
> +	const struct qcom_pdc_desc *desc;
> +	void __iomem *base;
> +	struct resource *res;
> +
> +	desc = of_device_get_match_data(dev);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->desc = desc;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	data->regmap = devm_regmap_init_mmio(dev, base, desc->config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(dev, "Unable to get pdc-global regmap");

Add missing '\n'

Say 'pdc-reset' instead of 'pdc-global'? (see also my other comment
below).

> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	data->rcdev.owner = THIS_MODULE;
> +	data->rcdev.ops = &qcom_pdc_reset_ops;
> +	data->rcdev.nr_resets = desc->num_resets;
> +	data->rcdev.of_node = dev->of_node;
> +
> +	return devm_reset_controller_register(dev, &data->rcdev);
> +}
> +
> +static const struct of_device_id qcom_pdc_reset_of_match[] = {
> +	{ .compatible = "qcom,sdm845-pdc-global", .data = &sdm845_pdc_desc },

Should this be 'qcom,sdm845-pdc-reset' which is more specific than
'global' and in line with the name and purpose of the driver?
Obviously this would require to adjust the bindings doc too.

Cheers

Matthias

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-24 13:18 [PATCH v2 0/6] Add support for PDC Global on SDM845 SoCs Sibi Sankar
2018-08-24 13:18 ` [PATCH v2 1/6] dt-bindings: reset: Add PDC Global binding for " Sibi Sankar
2018-08-28  0:33   ` Bjorn Andersson
2018-08-28  0:38   ` Matthias Kaehlcke
2018-08-28  6:08     ` sibis
2018-08-28 17:11       ` Matthias Kaehlcke
2018-08-29  0:43   ` Rob Herring
2018-08-24 13:18 ` [PATCH v2 2/6] reset: qcom: PDC Global (Power Domain Controller) reset controller Sibi Sankar
2018-08-28  0:22   ` Matthias Kaehlcke [this message]
2018-08-28  3:02     ` Bjorn Andersson
2018-08-28  7:11       ` Sibi Sankar
2018-08-28 16:56       ` Matthias Kaehlcke
2018-08-24 13:18 ` [PATCH v2 3/6] dt-bindings: remoteproc: Remove additional definition tag Sibi Sankar
2018-08-28  0:44   ` Matthias Kaehlcke
2018-08-29 16:38     ` Sibi Sankar
2018-08-24 13:18 ` [PATCH v2 4/6] dt-bindings: remoteproc: Add PDC reset binding for Q6V5 PIL Sibi Sankar
2018-08-29  0:44   ` Rob Herring
2018-08-24 13:18 ` [PATCH v2 5/6] remoteproc: qcom: q6v5-pil: Explicitly get mss_restart line Sibi Sankar
2018-08-24 13:19 ` [PATCH v2 6/6] remoteproc: qcom: q6v5-pil: Add PDC reset for modem on SDM845 SoCs Sibi Sankar

Reply instructions:

You may reply publically 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=20180828002236.GX160295@google.com \
    --to=mka@chromium.org \
    --cc=akdwived@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=ohad@wizery.com \
    --cc=p.zabel@pengutronix.de \
    --cc=robh+dt@kernel.org \
    --cc=sibis@codeaurora.org \
    --cc=sricharan@codeaurora.org \
    --cc=tsoni@codeaurora.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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

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


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


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