Add SDM845 PDC (Power Domain Controller) reset controller binding Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- .../bindings/reset/qcom,pdc-reset.txt | 52 +++++++++++++++++++ include/dt-bindings/reset/qcom,sdm845-pdc.h | 20 +++++++ 2 files changed, 72 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt new file mode 100644 index 000000000000..85e159962e08 --- /dev/null +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt @@ -0,0 +1,52 @@ +PDC Reset Controller +====================================== + +This binding describes a reset-controller found on PDC-Global(Power Domain +Controller) block for Qualcomm Technologies Inc SDM845 SoCs. + +Required properties: +- compatible: + Usage: required + Value type: <string> + Definition: must be: + "qcom,sdm845-pdc-global" + +- reg: + Usage: required + Value type: <prop-encoded-array> + Definition: must specify the base address and size of the register + space. + +- #reset-cells: + Usage: required + Value type: <uint> + Definition: must be 1; cell entry represents the reset index. + +Example: + +pdc_reset: reset-controller@b2e0000 { + compatible = "qcom,sdm845-pdc-global"; + reg = <0xb2e0000 0x20000>; + #reset-cells = <1>; +}; + +PDC reset clients +====================================== + +Device nodes that need access to reset lines should +specify them as a reset phandle in their corresponding node as +specified in reset.txt. + +For list of all valid reset indicies see +<dt-bindings/reset/qcom,sdm845-pdc.h> + +Example: + +modem-pil@4080000 { + ... + + resets = <&pdc_reset PDC_MODEM_SYNC_RESET>; + reset-names = "pdc_restart"; + + ... +}; diff --git a/include/dt-bindings/reset/qcom,sdm845-pdc.h b/include/dt-bindings/reset/qcom,sdm845-pdc.h new file mode 100644 index 000000000000..53c37f9c319a --- /dev/null +++ b/include/dt-bindings/reset/qcom,sdm845-pdc.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2018 The Linux Foundation. All rights reserved. + */ + +#ifndef _DT_BINDINGS_RESET_PDC_SDM_845_H +#define _DT_BINDINGS_RESET_PDC_SDM_845_H + +#define PDC_APPS_SYNC_RESET 0 +#define PDC_SP_SYNC_RESET 1 +#define PDC_AUDIO_SYNC_RESET 2 +#define PDC_SENSORS_SYNC_RESET 3 +#define PDC_AOP_SYNC_RESET 4 +#define PDC_DEBUG_SYNC_RESET 5 +#define PDC_GPU_SYNC_RESET 6 +#define PDC_DISPLAY_SYNC_RESET 7 +#define PDC_COMPUTE_SYNC_RESET 8 +#define PDC_MODEM_SYNC_RESET 9 + +#endif -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Add reset controller for SDM845 SoC to control reset signals provided by PDC 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 | 139 +++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+) create mode 100644 drivers/reset/reset-qcom-pdc.c diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 13d28fdbdbb5..5344e202a630 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 + bool "Qcom 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. + 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..64a0041e3452 --- /dev/null +++ b/drivers/reset/reset-qcom-pdc.c @@ -0,0 +1,139 @@ +// 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> + +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; +}; + +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, 0x100, + 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, 0x100, 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"); + 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 }, + {} +}; + +static struct platform_driver qcom_pdc_reset_driver = { + .probe = qcom_pdc_reset_probe, + .driver = { + .name = "qcom_pdc_reset", + .of_match_table = qcom_pdc_reset_of_match, + }, +}; + +builtin_platform_driver(qcom_pdc_reset_driver); + +MODULE_DESCRIPTION("Qualcomm PDC Reset Driver"); +MODULE_LICENSE("GPL v2"); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Explicitly get mss_restart to facilitate adding PDC restart line for modem on SDM845 SoCs Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- drivers/remoteproc/qcom_q6v5_pil.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index b1296d614b8b..d57fdb34e3dd 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -1176,8 +1176,7 @@ static int q6v5_init_clocks(struct device *dev, struct clk **clks, static int q6v5_init_reset(struct q6v5 *qproc) { - qproc->mss_restart = devm_reset_control_get_exclusive(qproc->dev, - NULL); + qproc->mss_restart = devm_reset_control_get(qproc->dev, "mss_restart"); if (IS_ERR(qproc->mss_restart)) { dev_err(qproc->dev, "failed to acquire mss restart\n"); return PTR_ERR(qproc->mss_restart); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
In the presence of a PDC block working with subsystem RSC, assert/deassert PDC restart in modem start/stop path. Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- .../bindings/remoteproc/qcom,q6v5.txt | 4 +++ drivers/remoteproc/qcom_q6v5_pil.c | 27 ++++++++++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt index 601dd9f389aa..124fb1dc6fb8 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt @@ -51,6 +51,8 @@ on the Qualcomm Hexagon core. Usage: required Value type: <phandle> Definition: reference to the reset-controller for the modem sub-system + reference to the list of 2 reset-controllers for the modem + sub-system on SDM845 SoCs reference to the list of 3 reset-controllers for the wcss sub-system @@ -58,6 +60,8 @@ on the Qualcomm Hexagon core. Usage: required Value type: <stringlist> Definition: must be "mss_restart" for the modem sub-system + Definition: must be "mss_restart", "pdc_restart" for the modem + sub-system on SDM845 SoCs Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset" for the wcss syb-system diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index d57fdb34e3dd..5be794639fc3 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -149,6 +149,7 @@ struct q6v5 { u32 halt_nc; struct reset_control *mss_restart; + struct reset_control *pdc_restart; struct qcom_q6v5 q6v5; @@ -349,10 +350,17 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw) static int q6v5_reset_assert(struct q6v5 *qproc) { - if (qproc->has_alt_reset) - return reset_control_reset(qproc->mss_restart); - else - return reset_control_assert(qproc->mss_restart); + int ret; + + if (qproc->has_alt_reset) { + reset_control_assert(qproc->pdc_restart); + ret = reset_control_reset(qproc->mss_restart); + reset_control_deassert(qproc->pdc_restart); + } else { + ret = reset_control_assert(qproc->mss_restart); + } + + return ret; } static int q6v5_reset_deassert(struct q6v5 *qproc) @@ -360,9 +368,11 @@ static int q6v5_reset_deassert(struct q6v5 *qproc) int ret; if (qproc->has_alt_reset) { + reset_control_assert(qproc->pdc_restart); writel(1, qproc->rmb_base + RMB_MBA_ALT_RESET); ret = reset_control_reset(qproc->mss_restart); writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET); + reset_control_deassert(qproc->pdc_restart); } else { ret = reset_control_deassert(qproc->mss_restart); } @@ -1182,6 +1192,15 @@ static int q6v5_init_reset(struct q6v5 *qproc) return PTR_ERR(qproc->mss_restart); } + if (qproc->has_alt_reset) { + qproc->pdc_restart = devm_reset_control_get(qproc->dev, + "pdc_restart"); + if (IS_ERR(qproc->pdc_restart)) { + dev_err(qproc->dev, "failed to acquire pdc restart\n"); + return PTR_ERR(qproc->pdc_restart); + } + } + return 0; } -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Sibi, On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: > Add SDM845 PDC (Power Domain Controller) reset controller binding > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > .../bindings/reset/qcom,pdc-reset.txt | 52 +++++++++++++++++++ > include/dt-bindings/reset/qcom,sdm845-pdc.h | 20 +++++++ > 2 files changed, 72 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h > > diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > new file mode 100644 > index 000000000000..85e159962e08 > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > @@ -0,0 +1,52 @@ > +PDC Reset Controller > +====================================== > + > +This binding describes a reset-controller found on PDC-Global(Power Domain > +Controller) block for Qualcomm Technologies Inc SDM845 SoCs. > + > +Required properties: > +- compatible: > + Usage: required > + Value type: <string> > + Definition: must be: > + "qcom,sdm845-pdc-global" > + > +- reg: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: must specify the base address and size of the register > + space. > + > +- #reset-cells: > + Usage: required > + Value type: <uint> > + Definition: must be 1; cell entry represents the reset index. > + > +Example: > + > +pdc_reset: reset-controller@b2e0000 { Is this really just a reset controller? The name makes it sound like a driver binding to this should also provide pm_genpd and the binding should probably call this a power- controller: Documentation/devicetree/bindings/power/power_domain.txt. > + compatible = "qcom,sdm845-pdc-global"; > + reg = <0xb2e0000 0x20000>; This looks like this is the register space of the complete PDC, not just the reset register? > + #reset-cells = <1>; > +}; > + > +PDC reset clients > +====================================== > + > +Device nodes that need access to reset lines should > +specify them as a reset phandle in their corresponding node as > +specified in reset.txt. > + > +For list of all valid reset indicies see > +<dt-bindings/reset/qcom,sdm845-pdc.h> > + > +Example: > + > +modem-pil@4080000 { > + ... > + > + resets = <&pdc_reset PDC_MODEM_SYNC_RESET>; > + reset-names = "pdc_restart"; > + > + ... > +}; > diff --git a/include/dt-bindings/reset/qcom,sdm845-pdc.h b/include/dt-bindings/reset/qcom,sdm845-pdc.h > new file mode 100644 > index 000000000000..53c37f9c319a > --- /dev/null > +++ b/include/dt-bindings/reset/qcom,sdm845-pdc.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 The Linux Foundation. All rights reserved. > + */ > + > +#ifndef _DT_BINDINGS_RESET_PDC_SDM_845_H > +#define _DT_BINDINGS_RESET_PDC_SDM_845_H > + > +#define PDC_APPS_SYNC_RESET 0 > +#define PDC_SP_SYNC_RESET 1 > +#define PDC_AUDIO_SYNC_RESET 2 > +#define PDC_SENSORS_SYNC_RESET 3 > +#define PDC_AOP_SYNC_RESET 4 > +#define PDC_DEBUG_SYNC_RESET 5 > +#define PDC_GPU_SYNC_RESET 6 > +#define PDC_DISPLAY_SYNC_RESET 7 > +#define PDC_COMPUTE_SYNC_RESET 8 > +#define PDC_MODEM_SYNC_RESET 9 > + > +#endif regards Philipp
On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: > Add reset controller for SDM845 SoC to control reset signals > provided by PDC 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 | 139 +++++++++++++++++++++++++++++++++ > 3 files changed, 149 insertions(+) > create mode 100644 drivers/reset/reset-qcom-pdc.c > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 13d28fdbdbb5..5344e202a630 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 > + bool "Qcom 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. > + > 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..64a0041e3452 > --- /dev/null > +++ b/drivers/reset/reset-qcom-pdc.c > @@ -0,0 +1,139 @@ > +// 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> > + > +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; > +}; > + > +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, 0x100, Does this register have a name? If so, a #define would be preferable. Otherwise this driver looks fine to me. > + 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, 0x100, 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"); > + 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 }, > + {} > +}; > + > +static struct platform_driver qcom_pdc_reset_driver = { > + .probe = qcom_pdc_reset_probe, > + .driver = { > + .name = "qcom_pdc_reset", > + .of_match_table = qcom_pdc_reset_of_match, > + }, > +}; > + > +builtin_platform_driver(qcom_pdc_reset_driver); > + > +MODULE_DESCRIPTION("Qualcomm PDC Reset Driver"); > +MODULE_LICENSE("GPL v2"); regards Philipp
On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: > In the presence of a PDC block working with subsystem RSC, > assert/deassert PDC restart in modem start/stop path. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > .../bindings/remoteproc/qcom,q6v5.txt | 4 +++ > drivers/remoteproc/qcom_q6v5_pil.c | 27 ++++++++++++++++--- > 2 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > index 601dd9f389aa..124fb1dc6fb8 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt [...] > @@ -1182,6 +1192,15 @@ static int q6v5_init_reset(struct q6v5 *qproc) > return PTR_ERR(qproc->mss_restart); > } > > + if (qproc->has_alt_reset) { > + qproc->pdc_restart = devm_reset_control_get(qproc->dev, > + "pdc_restart"); Please use devm_reset_control_get_exclusive() instead. > + if (IS_ERR(qproc->pdc_restart)) { > + dev_err(qproc->dev, "failed to acquire pdc restart\n"); > + return PTR_ERR(qproc->pdc_restart); > + } > + } > + > return 0; > } regards Philipp
On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> Explicitly get mss_restart to facilitate adding PDC
> restart line for modem on SDM845 SoCs
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
> drivers/remoteproc/qcom_q6v5_pil.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index b1296d614b8b..d57fdb34e3dd 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -1176,8 +1176,7 @@ static int q6v5_init_clocks(struct device *dev, struct clk **clks,
>
> static int q6v5_init_reset(struct q6v5 *qproc)
> {
> - qproc->mss_restart = devm_reset_control_get_exclusive(qproc->dev,
> - NULL);
> + qproc->mss_restart = devm_reset_control_get(qproc->dev, "mss_restart");
Please keep using devm_reset_control_get_exclusive.
regards
Philipp
Hi Philipp, Thanks for the review! On 07/31/2018 02:12 PM, Philipp Zabel wrote: > Hi Sibi, > > On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: >> Add SDM845 PDC (Power Domain Controller) reset controller binding >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> .../bindings/reset/qcom,pdc-reset.txt | 52 +++++++++++++++++++ >> include/dt-bindings/reset/qcom,sdm845-pdc.h | 20 +++++++ >> 2 files changed, 72 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt >> create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h >> >> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt >> new file mode 100644 >> index 000000000000..85e159962e08 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt >> @@ -0,0 +1,52 @@ >> +PDC Reset Controller >> +====================================== >> + >> +This binding describes a reset-controller found on PDC-Global(Power Domain >> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs. >> + >> +Required properties: >> +- compatible: >> + Usage: required >> + Value type: <string> >> + Definition: must be: >> + "qcom,sdm845-pdc-global" >> + >> +- reg: >> + Usage: required >> + Value type: <prop-encoded-array> >> + Definition: must specify the base address and size of the register >> + space. >> + >> +- #reset-cells: >> + Usage: required >> + Value type: <uint> >> + Definition: must be 1; cell entry represents the reset index. >> + >> +Example: >> + >> +pdc_reset: reset-controller@b2e0000 { > > Is this really just a reset controller? > > The name makes it sound like a driver binding to this should also > provide pm_genpd and the binding should probably call this a power- > controller: Documentation/devicetree/bindings/power/power_domain.txt. > The PDC-global reg space which is a part of PDC-wrapper reg space seems to be only used for the reset lines. Couple of other drivers use other parts of the PDC-wrapper reg space: https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller) https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries to occupy the entire pdc-wrapper reg space) since it couldn't be logically mapped into pdc-interrupt driver, it had to be included as a separate reset driver. >> + compatible = "qcom,sdm845-pdc-global"; >> + reg = <0xb2e0000 0x20000>; > > This looks like this is the register space of the complete PDC, not just > the reset register? > The entire register space was chosen because it is only used for its reset lines (had a good look at the downstream kernel and had a conversation with Lina) and to ensure break backward compatibility for the for the dt entry if the reg-space was used for other purposes in the future. >> + #reset-cells = <1>; >> +}; >> + >> +PDC reset clients >> +====================================== >> + >> +Device nodes that need access to reset lines should >> +specify them as a reset phandle in their corresponding node as >> +specified in reset.txt. >> + >> +For list of all valid reset indicies see >> +<dt-bindings/reset/qcom,sdm845-pdc.h> >> + >> +Example: >> + >> +modem-pil@4080000 { >> + ... >> + >> + resets = <&pdc_reset PDC_MODEM_SYNC_RESET>; >> + reset-names = "pdc_restart"; >> + >> + ... >> +}; >> diff --git a/include/dt-bindings/reset/qcom,sdm845-pdc.h b/include/dt-bindings/reset/qcom,sdm845-pdc.h >> new file mode 100644 >> index 000000000000..53c37f9c319a >> --- /dev/null >> +++ b/include/dt-bindings/reset/qcom,sdm845-pdc.h >> @@ -0,0 +1,20 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright (C) 2018 The Linux Foundation. All rights reserved. >> + */ >> + >> +#ifndef _DT_BINDINGS_RESET_PDC_SDM_845_H >> +#define _DT_BINDINGS_RESET_PDC_SDM_845_H >> + >> +#define PDC_APPS_SYNC_RESET 0 >> +#define PDC_SP_SYNC_RESET 1 >> +#define PDC_AUDIO_SYNC_RESET 2 >> +#define PDC_SENSORS_SYNC_RESET 3 >> +#define PDC_AOP_SYNC_RESET 4 >> +#define PDC_DEBUG_SYNC_RESET 5 >> +#define PDC_GPU_SYNC_RESET 6 >> +#define PDC_DISPLAY_SYNC_RESET 7 >> +#define PDC_COMPUTE_SYNC_RESET 8 >> +#define PDC_MODEM_SYNC_RESET 9 >> + >> +#endif > > regards > Philipp > -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Philipp, Thanks for the review! On 07/31/2018 02:21 PM, Philipp Zabel wrote: > On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: >> Add reset controller for SDM845 SoC to control reset signals >> provided by PDC 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 | 139 +++++++++++++++++++++++++++++++++ >> 3 files changed, 149 insertions(+) >> create mode 100644 drivers/reset/reset-qcom-pdc.c >> >> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig >> index 13d28fdbdbb5..5344e202a630 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 >> + bool "Qcom 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. >> + >> 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..64a0041e3452 >> --- /dev/null >> +++ b/drivers/reset/reset-qcom-pdc.c >> @@ -0,0 +1,139 @@ >> +// 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> >> + >> +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; >> +}; >> + >> +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, 0x100, > > Does this register have a name? If so, a #define would be preferable. > Otherwise this driver looks fine to me. > Yes will had a separate #define for it. >> + 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, 0x100, 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"); >> + 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 }, >> + {} >> +}; >> + >> +static struct platform_driver qcom_pdc_reset_driver = { >> + .probe = qcom_pdc_reset_probe, >> + .driver = { >> + .name = "qcom_pdc_reset", >> + .of_match_table = qcom_pdc_reset_of_match, >> + }, >> +}; >> + >> +builtin_platform_driver(qcom_pdc_reset_driver); >> + >> +MODULE_DESCRIPTION("Qualcomm PDC Reset Driver"); >> +MODULE_LICENSE("GPL v2"); > > regards > Philipp > -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Philipp, Thanks for the review! On 07/31/2018 02:27 PM, Philipp Zabel wrote: > On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: >> Explicitly get mss_restart to facilitate adding PDC >> restart line for modem on SDM845 SoCs >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> drivers/remoteproc/qcom_q6v5_pil.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index b1296d614b8b..d57fdb34e3dd 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -1176,8 +1176,7 @@ static int q6v5_init_clocks(struct device *dev, struct clk **clks, >> >> static int q6v5_init_reset(struct q6v5 *qproc) >> { >> - qproc->mss_restart = devm_reset_control_get_exclusive(qproc->dev, >> - NULL); >> + qproc->mss_restart = devm_reset_control_get(qproc->dev, "mss_restart"); > > Please keep using devm_reset_control_get_exclusive. > got misled when I saw it being used in a recent driver :), will revert it back. > regards > Philipp > -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Philipp, Thanks for the review! On 07/31/2018 02:24 PM, Philipp Zabel wrote: > On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: >> In the presence of a PDC block working with subsystem RSC, >> assert/deassert PDC restart in modem start/stop path. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> .../bindings/remoteproc/qcom,q6v5.txt | 4 +++ >> drivers/remoteproc/qcom_q6v5_pil.c | 27 ++++++++++++++++--- >> 2 files changed, 27 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt >> index 601dd9f389aa..124fb1dc6fb8 100644 >> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > [...] >> @@ -1182,6 +1192,15 @@ static int q6v5_init_reset(struct q6v5 *qproc) >> return PTR_ERR(qproc->mss_restart); >> } >> >> + if (qproc->has_alt_reset) { >> + qproc->pdc_restart = devm_reset_control_get(qproc->dev, >> + "pdc_restart"); > > Please use devm_reset_control_get_exclusive() instead. > will replace it in the next re-spin. >> + if (IS_ERR(qproc->pdc_restart)) { >> + dev_err(qproc->dev, "failed to acquire pdc restart\n"); >> + return PTR_ERR(qproc->pdc_restart); >> + } >> + } >> + >> return 0; >> } > > regards > Philipp > -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote: > Hi Philipp, > Thanks for the review! > > On 07/31/2018 02:12 PM, Philipp Zabel wrote: > > Hi Sibi, > > > > On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: > > > Add SDM845 PDC (Power Domain Controller) reset controller binding > > > > > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > > > --- > > > .../bindings/reset/qcom,pdc-reset.txt | 52 +++++++++++++++++++ > > > include/dt-bindings/reset/qcom,sdm845-pdc.h | 20 +++++++ > > > 2 files changed, 72 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > > > create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h > > > > > > diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > > > new file mode 100644 > > > index 000000000000..85e159962e08 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > > > @@ -0,0 +1,52 @@ > > > +PDC Reset Controller > > > +====================================== > > > + > > > +This binding describes a reset-controller found on PDC-Global(Power Domain > > > +Controller) block for Qualcomm Technologies Inc SDM845 SoCs. > > > + > > > +Required properties: > > > +- compatible: > > > + Usage: required > > > + Value type: <string> > > > + Definition: must be: > > > + "qcom,sdm845-pdc-global" > > > + > > > +- reg: > > > + Usage: required > > > + Value type: <prop-encoded-array> > > > + Definition: must specify the base address and size of the register > > > + space. > > > + > > > +- #reset-cells: > > > + Usage: required > > > + Value type: <uint> > > > + Definition: must be 1; cell entry represents the reset index. > > > + > > > +Example: > > > + > > > +pdc_reset: reset-controller@b2e0000 { > > > > Is this really just a reset controller? > > > > The name makes it sound like a driver binding to this should also > > provide pm_genpd and the binding should probably call this a power- > > controller: Documentation/devicetree/bindings/power/power_domain.txt. > > > > The PDC-global reg space which is a part of PDC-wrapper reg space seems > to be only used for the reset lines. > > Couple of other drivers use other parts of the PDC-wrapper reg space: > https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller) > https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries > to occupy the entire pdc-wrapper reg space) > > since it couldn't be logically mapped into pdc-interrupt driver, it had > to be included as a separate reset driver. You can't have overlapping regions in DT (well, you can because we have to work-around existing DTs that do, but you shouldn't). A single node can be multiple providers such as interrupt controller and reset controller. It's an OS problem to split that into multiple drivers. > > > + compatible = "qcom,sdm845-pdc-global"; > > > + reg = <0xb2e0000 0x20000>; > > > > This looks like this is the register space of the complete PDC, not just > > the reset register? > > > > The entire register space was chosen because it is only used for its > reset lines (had a good look at the downstream kernel and had a conversation > with Lina) and to ensure break backward compatibility for > the for the dt entry if the reg-space was used for other purposes in > the future. Why do you want to ensure breaking backwards compatibility? Rob
On Fri, Jul 27, 2018 at 08:58:11PM +0530, Sibi Sankar wrote: > In the presence of a PDC block working with subsystem RSC, > assert/deassert PDC restart in modem start/stop path. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > .../bindings/remoteproc/qcom,q6v5.txt | 4 +++ Please split bindings to separate patch. > drivers/remoteproc/qcom_q6v5_pil.c | 27 ++++++++++++++++--- > 2 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > index 601dd9f389aa..124fb1dc6fb8 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > @@ -51,6 +51,8 @@ on the Qualcomm Hexagon core. > Usage: required > Value type: <phandle> > Definition: reference to the reset-controller for the modem sub-system > + reference to the list of 2 reset-controllers for the modem > + sub-system on SDM845 SoCs > reference to the list of 3 reset-controllers for the > wcss sub-system > > @@ -58,6 +60,8 @@ on the Qualcomm Hexagon core. > Usage: required > Value type: <stringlist> > Definition: must be "mss_restart" for the modem sub-system > + Definition: must be "mss_restart", "pdc_restart" for the modem > + sub-system on SDM845 SoCs > Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset" > for the wcss syb-system >
Hi Rob, Thanks for the review On 08/07/2018 11:46 PM, Rob Herring wrote: > On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote: >> Hi Philipp, >> Thanks for the review! >> >> On 07/31/2018 02:12 PM, Philipp Zabel wrote: >>> Hi Sibi, >>> >>> On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: >>>> Add SDM845 PDC (Power Domain Controller) reset controller binding >>>> >>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >>>> --- >>>> .../bindings/reset/qcom,pdc-reset.txt | 52 +++++++++++++++++++ >>>> include/dt-bindings/reset/qcom,sdm845-pdc.h | 20 +++++++ >>>> 2 files changed, 72 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt >>>> create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h >>>> >>>> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt >>>> new file mode 100644 >>>> index 000000000000..85e159962e08 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt >>>> @@ -0,0 +1,52 @@ >>>> +PDC Reset Controller >>>> +====================================== >>>> + >>>> +This binding describes a reset-controller found on PDC-Global(Power Domain >>>> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs. >>>> + >>>> +Required properties: >>>> +- compatible: >>>> + Usage: required >>>> + Value type: <string> >>>> + Definition: must be: >>>> + "qcom,sdm845-pdc-global" >>>> + >>>> +- reg: >>>> + Usage: required >>>> + Value type: <prop-encoded-array> >>>> + Definition: must specify the base address and size of the register >>>> + space. >>>> + >>>> +- #reset-cells: >>>> + Usage: required >>>> + Value type: <uint> >>>> + Definition: must be 1; cell entry represents the reset index. >>>> + >>>> +Example: >>>> + >>>> +pdc_reset: reset-controller@b2e0000 { >>> >>> Is this really just a reset controller? >>> >>> The name makes it sound like a driver binding to this should also >>> provide pm_genpd and the binding should probably call this a power- >>> controller: Documentation/devicetree/bindings/power/power_domain.txt. >>> >> >> The PDC-global reg space which is a part of PDC-wrapper reg space seems >> to be only used for the reset lines. >> >> Couple of other drivers use other parts of the PDC-wrapper reg space: >> https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller) >> https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries >> to occupy the entire pdc-wrapper reg space) >> >> since it couldn't be logically mapped into pdc-interrupt driver, it had >> to be included as a separate reset driver. > > You can't have overlapping regions in DT (well, you can because we have > to work-around existing DTs that do, but you shouldn't). > > A single node can be multiple providers such as interrupt controller and > reset controller. It's an OS problem to split that into multiple > drivers. > There will be no overlaps. Jordan will be changing the dt binding of gmu_pdc so that there is no overlap I guess. What I meant to say is that pdc-global is a separate reg-space and currently has no other functionality other than exposing the reset lines. >>>> + compatible = "qcom,sdm845-pdc-global"; >>>> + reg = <0xb2e0000 0x20000>; >>> >>> This looks like this is the register space of the complete PDC, not just >>> the reset register? >>> >> >> The entire register space was chosen because it is only used for its >> reset lines (had a good look at the downstream kernel and had a conversation >> with Lina) and to ensure break backward compatibility for >> the for the dt entry if the reg-space was used for other purposes in >> the future. > > Why do you want to ensure breaking backwards compatibility? > Similar to the AOSS reset driver which had a unused clock part, this driver also exposes a reg space of which only reset lines are used. > Rob > -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On 08/07/2018 11:48 PM, Rob Herring wrote: > On Fri, Jul 27, 2018 at 08:58:11PM +0530, Sibi Sankar wrote: >> In the presence of a PDC block working with subsystem RSC, >> assert/deassert PDC restart in modem start/stop path. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> .../bindings/remoteproc/qcom,q6v5.txt | 4 +++ > > Please split bindings to separate patch. > Okay will split them. >> drivers/remoteproc/qcom_q6v5_pil.c | 27 ++++++++++++++++--- >> 2 files changed, 27 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt >> index 601dd9f389aa..124fb1dc6fb8 100644 >> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt >> @@ -51,6 +51,8 @@ on the Qualcomm Hexagon core. >> Usage: required >> Value type: <phandle> >> Definition: reference to the reset-controller for the modem sub-system >> + reference to the list of 2 reset-controllers for the modem >> + sub-system on SDM845 SoCs >> reference to the list of 3 reset-controllers for the >> wcss sub-system >> >> @@ -58,6 +60,8 @@ on the Qualcomm Hexagon core. >> Usage: required >> Value type: <stringlist> >> Definition: must be "mss_restart" for the modem sub-system >> + Definition: must be "mss_restart", "pdc_restart" for the modem >> + sub-system on SDM845 SoCs >> Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset" >> for the wcss syb-system >> -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On Wed, Aug 08, 2018 at 09:14:05PM +0530, Sibi Sankar wrote:
> Hi Rob,
> Thanks for the review
>
> On 08/07/2018 11:46 PM, Rob Herring wrote:
> >On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
> >>Hi Philipp,
> >>Thanks for the review!
> >>
> >>On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> >>>Hi Sibi,
> >>>
> >>>On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> >>>>Add SDM845 PDC (Power Domain Controller) reset controller binding
> >>>>
> >>>>Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> >>>>---
> >>>> .../bindings/reset/qcom,pdc-reset.txt | 52 +++++++++++++++++++
> >>>> include/dt-bindings/reset/qcom,sdm845-pdc.h | 20 +++++++
> >>>> 2 files changed, 72 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> >>>> create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> >>>>
> >>>>diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> >>>>new file mode 100644
> >>>>index 000000000000..85e159962e08
> >>>>--- /dev/null
> >>>>+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> >>>>@@ -0,0 +1,52 @@
> >>>>+PDC Reset Controller
> >>>>+======================================
> >>>>+
> >>>>+This binding describes a reset-controller found on PDC-Global(Power Domain
> >>>>+Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
> >>>>+
> >>>>+Required properties:
> >>>>+- compatible:
> >>>>+ Usage: required
> >>>>+ Value type: <string>
> >>>>+ Definition: must be:
> >>>>+ "qcom,sdm845-pdc-global"
> >>>>+
> >>>>+- reg:
> >>>>+ Usage: required
> >>>>+ Value type: <prop-encoded-array>
> >>>>+ Definition: must specify the base address and size of the register
> >>>>+ space.
> >>>>+
> >>>>+- #reset-cells:
> >>>>+ Usage: required
> >>>>+ Value type: <uint>
> >>>>+ Definition: must be 1; cell entry represents the reset index.
> >>>>+
> >>>>+Example:
> >>>>+
> >>>>+pdc_reset: reset-controller@b2e0000 {
> >>>
> >>>Is this really just a reset controller?
> >>>
> >>>The name makes it sound like a driver binding to this should also
> >>>provide pm_genpd and the binding should probably call this a power-
> >>>controller: Documentation/devicetree/bindings/power/power_domain.txt.
> >>>
> >>
> >>The PDC-global reg space which is a part of PDC-wrapper reg space seems
> >>to be only used for the reset lines.
> >>
> >>Couple of other drivers use other parts of the PDC-wrapper reg space:
> >>https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
> >>https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
> >>to occupy the entire pdc-wrapper reg space)
> >>
> >>since it couldn't be logically mapped into pdc-interrupt driver, it had
> >>to be included as a separate reset driver.
> >
> >You can't have overlapping regions in DT (well, you can because we have
> >to work-around existing DTs that do, but you shouldn't).
> >
> >A single node can be multiple providers such as interrupt controller and
> >reset controller. It's an OS problem to split that into multiple
> >drivers.
>
> There will be no overlaps. Jordan will be changing the dt binding of
> gmu_pdc so that there is no overlap I guess. What I meant to say is that
> pdc-global is a separate reg-space and currently has no other
> functionality other than exposing the reset lines.
Correct - the updated GPU DT will be:
reg = <0x506a000 0x30000>,
<0xb280000 0x10000>,
<0xb480000, 0x10000>;
reg-names = "gmu", "gmu_pdc", "gmu_pdc_seq";
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed, Aug 08, 2018 at 03:37:24PM -0600, Jordan Crouse wrote: > On Wed, Aug 08, 2018 at 09:14:05PM +0530, Sibi Sankar wrote: > > Hi Rob, > > Thanks for the review > > > > On 08/07/2018 11:46 PM, Rob Herring wrote: > > >On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote: > > >>Hi Philipp, > > >>Thanks for the review! > > >> > > >>On 07/31/2018 02:12 PM, Philipp Zabel wrote: > > >>>Hi Sibi, > > >>> > > >>>On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote: > > >>>>Add SDM845 PDC (Power Domain Controller) reset controller binding > > >>>> > > >>>>Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > > >>>>--- > > >>>> .../bindings/reset/qcom,pdc-reset.txt | 52 +++++++++++++++++++ > > >>>> include/dt-bindings/reset/qcom,sdm845-pdc.h | 20 +++++++ > > >>>> 2 files changed, 72 insertions(+) > > >>>> create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > > >>>> create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h > > >>>> > > >>>>diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > > >>>>new file mode 100644 > > >>>>index 000000000000..85e159962e08 > > >>>>--- /dev/null > > >>>>+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > > >>>>@@ -0,0 +1,52 @@ > > >>>>+PDC Reset Controller > > >>>>+====================================== > > >>>>+ > > >>>>+This binding describes a reset-controller found on PDC-Global(Power Domain > > >>>>+Controller) block for Qualcomm Technologies Inc SDM845 SoCs. > > >>>>+ > > >>>>+Required properties: > > >>>>+- compatible: > > >>>>+ Usage: required > > >>>>+ Value type: <string> > > >>>>+ Definition: must be: > > >>>>+ "qcom,sdm845-pdc-global" > > >>>>+ > > >>>>+- reg: > > >>>>+ Usage: required > > >>>>+ Value type: <prop-encoded-array> > > >>>>+ Definition: must specify the base address and size of the register > > >>>>+ space. > > >>>>+ > > >>>>+- #reset-cells: > > >>>>+ Usage: required > > >>>>+ Value type: <uint> > > >>>>+ Definition: must be 1; cell entry represents the reset index. > > >>>>+ > > >>>>+Example: > > >>>>+ > > >>>>+pdc_reset: reset-controller@b2e0000 { > > >>> > > >>>Is this really just a reset controller? > > >>> > > >>>The name makes it sound like a driver binding to this should also > > >>>provide pm_genpd and the binding should probably call this a power- > > >>>controller: Documentation/devicetree/bindings/power/power_domain.txt. > > >>> > > >> > > >>The PDC-global reg space which is a part of PDC-wrapper reg space seems > > >>to be only used for the reset lines. > > >> > > >>Couple of other drivers use other parts of the PDC-wrapper reg space: > > >>https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller) > > >>https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries > > >>to occupy the entire pdc-wrapper reg space) > > >> > > >>since it couldn't be logically mapped into pdc-interrupt driver, it had > > >>to be included as a separate reset driver. > > > > > >You can't have overlapping regions in DT (well, you can because we have > > >to work-around existing DTs that do, but you shouldn't). > > > > > >A single node can be multiple providers such as interrupt controller and > > >reset controller. It's an OS problem to split that into multiple > > >drivers. > > > > There will be no overlaps. Jordan will be changing the dt binding of > > gmu_pdc so that there is no overlap I guess. What I meant to say is that > > pdc-global is a separate reg-space and currently has no other > > functionality other than exposing the reset lines. > > Correct - the updated GPU DT will be: > > reg = <0x506a000 0x30000>, > <0xb280000 0x10000>, > <0xb480000, 0x10000>; > reg-names = "gmu", "gmu_pdc", "gmu_pdc_seq"; > > Jordan Code change: https://patchwork.freedesktop.org/patch/243513/ DT change: https://patchwork.freedesktop.org/patch/243539/ Jordan -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On Fri 27 Jul 08:28 PDT 2018, Sibi Sankar wrote: > Add SDM845 PDC (Power Domain Controller) reset controller binding > Even though this is currently describing only a reset controller I think this binding better be talking about the "PDC Global" hardware. > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > .../bindings/reset/qcom,pdc-reset.txt | 52 +++++++++++++++++++ > include/dt-bindings/reset/qcom,sdm845-pdc.h | 20 +++++++ > 2 files changed, 72 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h > > diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt Rename this qcom,pdc-global to match the compatible, and hardware name. > new file mode 100644 > index 000000000000..85e159962e08 > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt > @@ -0,0 +1,52 @@ > +PDC Reset Controller PDC Global > +====================================== > + > +This binding describes a reset-controller found on PDC-Global(Power Domain > +Controller) block for Qualcomm Technologies Inc SDM845 SoCs. This looks good. > + > +Required properties: > +- compatible: > + Usage: required > + Value type: <string> > + Definition: must be: > + "qcom,sdm845-pdc-global" > + > +- reg: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: must specify the base address and size of the register > + space. > + > +- #reset-cells: > + Usage: required > + Value type: <uint> > + Definition: must be 1; cell entry represents the reset index. > + > +Example: > + > +pdc_reset: reset-controller@b2e0000 { This is perfectly fine, in its current form it is a reset-controller. > + compatible = "qcom,sdm845-pdc-global"; > + reg = <0xb2e0000 0x20000>; > + #reset-cells = <1>; > +}; > + Apart from this, the binding looks good! Regards, Bjorn
On Fri 27 Jul 08:28 PDT 2018, Sibi Sankar wrote: > Add reset controller for SDM845 SoC to control reset signals > provided by PDC 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 | 139 +++++++++++++++++++++++++++++++++ > 3 files changed, 149 insertions(+) > create mode 100644 drivers/reset/reset-qcom-pdc.c > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 13d28fdbdbb5..5344e202a630 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 > + bool "Qcom PDC Reset Driver" "Qualcomm" And I do not see a strong reason for this not being tristate, the consumers we've talked about so far can all be modules. > + 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. > + Implementation looks fine. Regards, Bjorn
On Fri 27 Jul 08:28 PDT 2018, Sibi Sankar wrote:
> In the presence of a PDC block working with subsystem RSC,
> assert/deassert PDC restart in modem start/stop path.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
> .../bindings/remoteproc/qcom,q6v5.txt | 4 +++
> drivers/remoteproc/qcom_q6v5_pil.c | 27 ++++++++++++++++---
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 601dd9f389aa..124fb1dc6fb8 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -51,6 +51,8 @@ on the Qualcomm Hexagon core.
> Usage: required
> Value type: <phandle>
> Definition: reference to the reset-controller for the modem sub-system
> + reference to the list of 2 reset-controllers for the modem
> + sub-system on SDM845 SoCs
> reference to the list of 3 reset-controllers for the
> wcss sub-system
>
> @@ -58,6 +60,8 @@ on the Qualcomm Hexagon core.
> Usage: required
> Value type: <stringlist>
> Definition: must be "mss_restart" for the modem sub-system
> + Definition: must be "mss_restart", "pdc_restart" for the modem
> + sub-system on SDM845 SoCs
> Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
> for the wcss syb-system
Seems like we got an additional "Definition:" added here as we added the
wcss. Please don't add another one.
The rest looks good.
Regards,
Bjorn