[PATCHv2 00/11] soc: ti: add OMAP PRM driver (for reset)

[PATCHv2 00/11] soc: ti: add OMAP PRM driver (for reset)

[Tero Kristo]

Hi,

V2 of the series mostly has comments fixed from Suman.
- Added a link between reset + clock drivers to sync up the state between
  these; this is to avoid facing any timeout issues on either end due to
  sequencing of events (Patch #5.) This has been implemented via TI only
  private driver APIs, as at least I am not aware of anybody else needing
  similar mechanism and it is pretty SoC architecture specific.
- Dropped any powerdomain related data for now as it is not used for
  anything yet.
- Added checks against illegal reset IDs.
- Added checks for pdata validity during probe.
- Reset data is added for am4/omap5 SoCs.
- Some other minor tweaks.

This series depends on the clock driver changes [1] due to patch #5,
otherwise there will be build breakage.

Also, just as a background note, this driver has been implemented
under drivers/soc/ti due to the fact that I did not figure out any
better home for it. In its current form it would be suitable to
reside under drivers/reset, but there is a plan to extend this to
support powerdomain handling also (PRM stands for Power and Reset
Management.)

-Tero

[1] https://marc.info/?l=linux-clk&m=156697558331203&w=2


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 01/11] dt-bindings: omap: add new binding for PRM instances

[Tero Kristo]

Add new binding for OMAP PRM (Power and Reset Manager) instances. Each
of these will act as a power domain controller and potentially as a reset
provider.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 .../devicetree/bindings/arm/omap/prm-inst.txt | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/omap/prm-inst.txt

diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
new file mode 100644
index 000000000000..7c7527c37734
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
@@ -0,0 +1,31 @@
+OMAP PRM instance bindings
+
+Power and Reset Manager is an IP block on OMAP family of devices which
+handle the power domains and their current state, and provide reset
+handling for the domains and/or separate IP blocks under the power domain
+hierarchy.
+
+Required properties:
+- compatible:	Must be one of:
+		"ti,am3-prm-inst"
+		"ti,am4-prm-inst"
+		"ti,omap4-prm-inst"
+		"ti,omap5-prm-inst"
+		"ti,dra7-prm-inst"
+- reg:		Contains PRM instance register address range
+		(base address and length)
+
+Optional properties:
+- #reset-cells:	Should be 1 if the PRM instance in question supports resets.
+- clocks:	Associated clocks for the reset signals if any. Certain reset
+		signals can't be toggled properly without functional clock
+		being active for them.
+
+Example:
+
+prm_dsp2: prm@1b00 {
+	compatible = "ti,dra7-prm-inst";
+	reg = <0x1b00 0x40>;
+	#reset-cells = <1>;
+	clocks = <&dsp2_clkctrl DRA7_DSP2_MMU0_DSP2_CLKCTRL 0>;
+};
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCHv2 01/11] dt-bindings: omap: add new binding for PRM instances

[Tero Kristo]

Hi Rob,

Quick question below based on some discussion on the implementation 
details of the PRM support.

On 28/08/2019 10:19, Tero Kristo wrote:
> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each
> of these will act as a power domain controller and potentially as a reset
> provider.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>   .../devicetree/bindings/arm/omap/prm-inst.txt | 31 +++++++++++++++++++
>   1 file changed, 31 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/arm/omap/prm-inst.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
> new file mode 100644
> index 000000000000..7c7527c37734
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
> @@ -0,0 +1,31 @@
> +OMAP PRM instance bindings
> +
> +Power and Reset Manager is an IP block on OMAP family of devices which
> +handle the power domains and their current state, and provide reset
> +handling for the domains and/or separate IP blocks under the power domain
> +hierarchy.
> +
> +Required properties:
> +- compatible:	Must be one of:
> +		"ti,am3-prm-inst"
> +		"ti,am4-prm-inst"
> +		"ti,omap4-prm-inst"
> +		"ti,omap5-prm-inst"
> +		"ti,dra7-prm-inst"

Each of these bindings describes multiple PRM instances, like 
ti,dra7-prm-inst maps eventually into 21 different PRM instances. I end 
up matching these in the kernel based on base address to map additional 
details, like support for reset handling, supported reset bits, etc.

What is your take on this, should I just provide individual compatible 
strings for these all, the total amount of them would end up being like 
70+, or keep them like this? I find it rather cumbersome if I would have 
to deal with 70+ different compatibles... Also, I would need to keep 
updating the bindings doc once I add new instances on the supported list.

-Tero

> +- reg:		Contains PRM instance register address range
> +		(base address and length)
> +
> +Optional properties:
> +- #reset-cells:	Should be 1 if the PRM instance in question supports resets.
> +- clocks:	Associated clocks for the reset signals if any. Certain reset
> +		signals can't be toggled properly without functional clock
> +		being active for them.
> +
> +Example:
> +
> +prm_dsp2: prm@1b00 {
> +	compatible = "ti,dra7-prm-inst";
> +	reg = <0x1b00 0x40>;
> +	#reset-cells = <1>;
> +	clocks = <&dsp2_clkctrl DRA7_DSP2_MMU0_DSP2_CLKCTRL 0>;
> +};
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 02/11] soc: ti: add initial PRM driver with reset control support

[Tero Kristo]

Add initial PRM (Power and Reset Management) driver for TI OMAP class
SoCs. Initially this driver only supports reset control, but can be
extended to support rest of the functionality, like powerdomain
control, PRCM irq support etc.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/Kconfig |   1 +
 drivers/soc/ti/Makefile     |   1 +
 drivers/soc/ti/omap_prm.c   | 235 ++++++++++++++++++++++++++++++++++++
 3 files changed, 237 insertions(+)
 create mode 100644 drivers/soc/ti/omap_prm.c

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index fdb6743760a2..ad08d470a2ca 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
 	select TI_SYSC
 	select OMAP_IRQCHIP
 	select CLKSRC_TI_32K
+	select ARCH_HAS_RESET_CONTROLLER
 	help
 	  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
 
diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
index b3868d392d4f..788b5cd1e180 100644
--- a/drivers/soc/ti/Makefile
+++ b/drivers/soc/ti/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
 knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
 obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
 obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
+obj-$(CONFIG_ARCH_OMAP2PLUS)		+= omap_prm.o
 obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
 obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
 obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
new file mode 100644
index 000000000000..fd5c431f8736
--- /dev/null
+++ b/drivers/soc/ti/omap_prm.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * OMAP2+ PRM driver
+ *
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ *	Tero Kristo <t-kristo@ti.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/delay.h>
+
+struct omap_rst_map {
+	s8 rst;
+	s8 st;
+};
+
+struct omap_prm_data {
+	u32 base;
+	const char *name;
+	u16 rstctrl;
+	u16 rstst;
+	const struct omap_rst_map *rstmap;
+	u8 flags;
+};
+
+struct omap_prm {
+	const struct omap_prm_data *data;
+	void __iomem *base;
+};
+
+struct omap_reset_data {
+	struct reset_controller_dev rcdev;
+	struct omap_prm *prm;
+};
+
+#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
+
+#define OMAP_MAX_RESETS		8
+#define OMAP_RESET_MAX_WAIT	10000
+
+#define OMAP_PRM_HAS_RSTCTRL	BIT(0)
+#define OMAP_PRM_HAS_RSTST	BIT(1)
+
+#define OMAP_PRM_HAS_RESETS	(OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_RSTST)
+
+static const struct of_device_id omap_prm_id_table[] = {
+	{ },
+};
+
+static bool _is_valid_reset(struct omap_reset_data *reset, unsigned long id)
+{
+	const struct omap_rst_map *map = reset->prm->data->rstmap;
+
+	while (map && map->rst >= 0) {
+		if (map->rst == id)
+			return true;
+		map++;
+	}
+
+	return false;
+}
+
+static int omap_reset_status(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+
+	if (!_is_valid_reset(reset, id))
+		return -EINVAL;
+
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
+	v &= 1 << id;
+	v >>= id;
+
+	return v;
+}
+
+static int omap_reset_assert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+
+	if (!_is_valid_reset(reset, id))
+		return -EINVAL;
+
+	/* assert the reset control line */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
+	v |= 1 << id;
+	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
+
+	return 0;
+}
+
+static int omap_reset_get_st_bit(struct omap_reset_data *reset,
+				 unsigned long id)
+{
+	const struct omap_rst_map *map = reset->prm->data->rstmap;
+
+	while (map && map->rst >= 0) {
+		if (map->rst == id)
+			return map->st;
+
+		map++;
+	}
+
+	return id;
+}
+
+/*
+ * Note that status will not change until clocks are on, and clocks cannot be
+ * enabled until reset is deasserted. Consumer drivers must check status
+ * separately after enabling clocks.
+ */
+static int omap_reset_deassert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+	int st_bit;
+	bool has_rstst;
+
+	if (!_is_valid_reset(reset, id))
+		return -EINVAL;
+
+	/* check the current status to avoid de-asserting the line twice */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
+	if (!(v & BIT(id)))
+		return -EEXIST;
+
+	has_rstst = reset->prm->data->rstst ||
+		(reset->prm->data->flags & OMAP_PRM_HAS_RSTST);
+
+	if (has_rstst) {
+		st_bit = omap_reset_get_st_bit(reset, id);
+
+		/* Clear the reset status by writing 1 to the status bit */
+		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
+		v |= 1 << st_bit;
+		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
+	}
+
+	/* de-assert the reset control line */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
+	v &= ~(1 << id);
+	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
+
+	return 0;
+}
+
+static const struct reset_control_ops omap_reset_ops = {
+	.assert		= omap_reset_assert,
+	.deassert	= omap_reset_deassert,
+	.status		= omap_reset_status,
+};
+
+static int omap_prm_reset_init(struct platform_device *pdev,
+			       struct omap_prm *prm)
+{
+	struct omap_reset_data *reset;
+
+	/*
+	 * Check if we have controllable resets. If either rstctrl is non-zero
+	 * or OMAP_PRM_HAS_RSTCTRL flag is set, we have reset control register
+	 * for the domain.
+	 */
+	if (!prm->data->rstctrl && !(prm->data->flags & OMAP_PRM_HAS_RSTCTRL))
+		return 0;
+
+	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
+	if (!reset)
+		return -ENOMEM;
+
+	reset->rcdev.owner = THIS_MODULE;
+	reset->rcdev.ops = &omap_reset_ops;
+	reset->rcdev.of_node = pdev->dev.of_node;
+	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
+
+	reset->prm = prm;
+
+	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
+}
+
+static int omap_prm_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	const struct omap_prm_data *data;
+	struct omap_prm *prm;
+	const struct of_device_id *match;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	match = of_match_device(omap_prm_id_table, &pdev->dev);
+	if (!match)
+		return -ENOTSUPP;
+
+	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
+	if (!prm)
+		return -ENOMEM;
+
+	data = match->data;
+
+	while (data->base != res->start) {
+		if (!data->base)
+			return -EINVAL;
+		data++;
+	}
+
+	prm->data = data;
+
+	prm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!prm->base)
+		return -ENOMEM;
+
+	return omap_prm_reset_init(pdev, prm);
+}
+
+static struct platform_driver omap_prm_driver = {
+	.probe = omap_prm_probe,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= omap_prm_id_table,
+	},
+};
+builtin_platform_driver(omap_prm_driver);
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCHv2 02/11] soc: ti: add initial PRM driver with reset control support

[Philipp Zabel]

On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:
> Add initial PRM (Power and Reset Management) driver for TI OMAP class
> SoCs. Initially this driver only supports reset control, but can be
> extended to support rest of the functionality, like powerdomain
> control, PRCM irq support etc.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  arch/arm/mach-omap2/Kconfig |   1 +
>  drivers/soc/ti/Makefile     |   1 +
>  drivers/soc/ti/omap_prm.c   | 235 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 237 insertions(+)
>  create mode 100644 drivers/soc/ti/omap_prm.c
> 
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index fdb6743760a2..ad08d470a2ca 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>  	select TI_SYSC
>  	select OMAP_IRQCHIP
>  	select CLKSRC_TI_32K
> +	select ARCH_HAS_RESET_CONTROLLER
>  	help
>  	  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>  
> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
> index b3868d392d4f..788b5cd1e180 100644
> --- a/drivers/soc/ti/Makefile
> +++ b/drivers/soc/ti/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
>  knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>  obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
>  obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
> +obj-$(CONFIG_ARCH_OMAP2PLUS)		+= omap_prm.o
>  obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
>  obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
>  obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> new file mode 100644
> index 000000000000..fd5c431f8736
> --- /dev/null
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * OMAP2+ PRM driver
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + *	Tero Kristo <t-kristo@ti.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>

Why <linux/module.h>? This is a builtin driver.

> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/delay.h>
> +
> +struct omap_rst_map {
> +	s8 rst;
> +	s8 st;
> +};
> +
> +struct omap_prm_data {
> +	u32 base;
> +	const char *name;
> +	u16 rstctrl;
> +	u16 rstst;
> +	const struct omap_rst_map *rstmap;
> +	u8 flags;
> +};

I wonder if splitting rstctrl/rstst/rstmap out into a separate structure
would make sense. That could be linked from omap_reset_data directly.
That only makes sense if there'd be enough cases where it can be reused
for multiple PRMs instances.

> +
> +struct omap_prm {
> +	const struct omap_prm_data *data;
> +	void __iomem *base;
> +};
> +
> +struct omap_reset_data {
> +	struct reset_controller_dev rcdev;
> +	struct omap_prm *prm;
> +};
> +
> +#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
> +
> +#define OMAP_MAX_RESETS		8
> +#define OMAP_RESET_MAX_WAIT	10000
> +
> +#define OMAP_PRM_HAS_RSTCTRL	BIT(0)
> +#define OMAP_PRM_HAS_RSTST	BIT(1)
> +
> +#define OMAP_PRM_HAS_RESETS	(OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_RSTST)
> +
> +static const struct of_device_id omap_prm_id_table[] = {
> +	{ },
> +};
> +
> +static bool _is_valid_reset(struct omap_reset_data *reset, unsigned long id)
> +{
> +	const struct omap_rst_map *map = reset->prm->data->rstmap;
> +
> +	while (map && map->rst >= 0) {

If rstmap is never NULL,

	while (map->rst >= 0) {

should be enough.

> +		if (map->rst == id)
> +			return true;
> +		map++;
> +	}
> +
> +	return false;
> +}
> +
> +static int omap_reset_status(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +
> +	if (!_is_valid_reset(reset, id))
> +		return -EINVAL;

Don't check this in the status/(de)assert/reset callbacks. Instead,
implement rcdev.of_xlate and return -EINVAL there, so that invalid ids
can never be requested.

> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +	v &= 1 << id;
> +	v >>= id;

omap_get_st_bit below makes it look like the status bit can be in a
different place than the reset control bit, should that be used here as
well?

> +
> +	return v;
> +}
> +
> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +
> +	if (!_is_valid_reset(reset, id))
> +		return -EINVAL;

Same as above.

> +	/* assert the reset control line */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
> +	v |= 1 << id;
> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);

This read-modify-write should be protected with a lock.

> +
> +	return 0;
> +}
> +
> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
> +				 unsigned long id)
> +{
> +	const struct omap_rst_map *map = reset->prm->data->rstmap;
> +
> +	while (map && map->rst >= 0) {

Same as above.

> +		if (map->rst == id)
> +			return map->st;
> +
> +		map++;
> +	}
> +
> +	return id;
> +}
> +
> +/*
> + * Note that status will not change until clocks are on, and clocks cannot be
> + * enabled until reset is deasserted. Consumer drivers must check status
> + * separately after enabling clocks.
> + */
> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +	int st_bit;
> +	bool has_rstst;
> +
> +	if (!_is_valid_reset(reset, id))
> +		return -EINVAL;

Same as above.

> +	/* check the current status to avoid de-asserting the line twice */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
> +	if (!(v & BIT(id)))
> +		return -EEXIST;

What is the purpose of this? For shared consumers the core already does
refcounting, and I expect exclusive consumers won't deassert twice.
Since the reset signal is deasserted after this call, this should not
return an error.

> +
> +	has_rstst = reset->prm->data->rstst ||
> +		(reset->prm->data->flags & OMAP_PRM_HAS_RSTST);
> +
> +	if (has_rstst) {
> +		st_bit = omap_reset_get_st_bit(reset, id);
> +
> +		/* Clear the reset status by writing 1 to the status bit */
> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +		v |= 1 << st_bit;
> +		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);

What does the value read from the rstst register indicate? Is it the
current state of the reset line? Is it 0 until deassertion is completed,
and then it turns to 1?

> +	}
> +
> +	/* de-assert the reset control line */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);

Reading the register again seems unnecessary.

> +	v &= ~(1 << id);
> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);

As above, the read-modify-write should be locked.

> +
> +	return 0;
> +}
> +
> +static const struct reset_control_ops omap_reset_ops = {
> +	.assert		= omap_reset_assert,
> +	.deassert	= omap_reset_deassert,
> +	.status		= omap_reset_status,
> +};
> +
> +static int omap_prm_reset_init(struct platform_device *pdev,
> +			       struct omap_prm *prm)
> +{
> +	struct omap_reset_data *reset;
> +
> +	/*
> +	 * Check if we have controllable resets. If either rstctrl is non-zero
> +	 * or OMAP_PRM_HAS_RSTCTRL flag is set, we have reset control register
> +	 * for the domain.
> +	 */
> +	if (!prm->data->rstctrl && !(prm->data->flags & OMAP_PRM_HAS_RSTCTRL))
> +		return 0;
> +
> +	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
> +	if (!reset)
> +		return -ENOMEM;
> +
> +	reset->rcdev.owner = THIS_MODULE;
> +	reset->rcdev.ops = &omap_reset_ops;
> +	reset->rcdev.of_node = pdev->dev.of_node;
> +	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> +
> +	reset->prm = prm;
> +
> +	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
> +}
> +
> +static int omap_prm_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	const struct omap_prm_data *data;
> +	struct omap_prm *prm;
> +	const struct of_device_id *match;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;

This can be merged with devm_ioremap_resource below.

> +	match = of_match_device(omap_prm_id_table, &pdev->dev);
> +	if (!match)
> +		return -ENOTSUPP;
> +
> +	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
> +	if (!prm)
> +		return -ENOMEM;
> +
> +	data = match->data;
> +
> +	while (data->base != res->start) {
> +		if (!data->base)
> +			return -EINVAL;
> +		data++;
> +	}

Is this not something that you want to have encoded in the compatible
string? They all have a different register layout.

> +
> +	prm->data = data;
> +
> +	prm->base = devm_ioremap_resource(&pdev->dev, res);

	prm->base = devm_platform_ioremap_resource(pdev, 0);

> +	if (!prm->base)
> +		return -ENOMEM;
> +
> +	return omap_prm_reset_init(pdev, prm);
> +}
> +
> +static struct platform_driver omap_prm_driver = {
> +	.probe = omap_prm_probe,
> +	.driver = {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= omap_prm_id_table,
> +	},
> +};
> +builtin_platform_driver(omap_prm_driver);

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 02/11] soc: ti: add initial PRM driver with reset control support

[Tero Kristo]

On 29/08/2019 16:12, Philipp Zabel wrote:
> On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:
>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>> SoCs. Initially this driver only supports reset control, but can be
>> extended to support rest of the functionality, like powerdomain
>> control, PRCM irq support etc.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   arch/arm/mach-omap2/Kconfig |   1 +
>>   drivers/soc/ti/Makefile     |   1 +
>>   drivers/soc/ti/omap_prm.c   | 235 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 237 insertions(+)
>>   create mode 100644 drivers/soc/ti/omap_prm.c
>>
>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>> index fdb6743760a2..ad08d470a2ca 100644
>> --- a/arch/arm/mach-omap2/Kconfig
>> +++ b/arch/arm/mach-omap2/Kconfig
>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>   	select TI_SYSC
>>   	select OMAP_IRQCHIP
>>   	select CLKSRC_TI_32K
>> +	select ARCH_HAS_RESET_CONTROLLER
>>   	help
>>   	  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>>   
>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>> index b3868d392d4f..788b5cd1e180 100644
>> --- a/drivers/soc/ti/Makefile
>> +++ b/drivers/soc/ti/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
>>   knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>   obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
>>   obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
>> +obj-$(CONFIG_ARCH_OMAP2PLUS)		+= omap_prm.o
>>   obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
>>   obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
>>   obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> new file mode 100644
>> index 000000000000..fd5c431f8736
>> --- /dev/null
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -0,0 +1,235 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * OMAP2+ PRM driver
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> + *	Tero Kristo <t-kristo@ti.com>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
> 
> Why <linux/module.h>? This is a builtin driver.

Yeah, not anymore. Let me ditch this.

> 
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/delay.h>
>> +
>> +struct omap_rst_map {
>> +	s8 rst;
>> +	s8 st;
>> +};
>> +
>> +struct omap_prm_data {
>> +	u32 base;
>> +	const char *name;
>> +	u16 rstctrl;
>> +	u16 rstst;
>> +	const struct omap_rst_map *rstmap;
>> +	u8 flags;
>> +};
> 
> I wonder if splitting rstctrl/rstst/rstmap out into a separate structure
> would make sense. That could be linked from omap_reset_data directly.
> That only makes sense if there'd be enough cases where it can be reused
> for multiple PRMs instances.

Hmm, splitting these out would make it possible to share the bits for 
ipu:s across devices, same for dsp:s and eve:s.

However, adding too many levels of abstraction makes it kind of 
difficult to follow what is happening with the code, and it would only 
save maybe ~100 bytes of memory at the moment.

> 
>> +
>> +struct omap_prm {
>> +	const struct omap_prm_data *data;
>> +	void __iomem *base;
>> +};
>> +
>> +struct omap_reset_data {
>> +	struct reset_controller_dev rcdev;
>> +	struct omap_prm *prm;
>> +};
>> +
>> +#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
>> +
>> +#define OMAP_MAX_RESETS		8
>> +#define OMAP_RESET_MAX_WAIT	10000
>> +
>> +#define OMAP_PRM_HAS_RSTCTRL	BIT(0)
>> +#define OMAP_PRM_HAS_RSTST	BIT(1)
>> +
>> +#define OMAP_PRM_HAS_RESETS	(OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_RSTST)
>> +
>> +static const struct of_device_id omap_prm_id_table[] = {
>> +	{ },
>> +};
>> +
>> +static bool _is_valid_reset(struct omap_reset_data *reset, unsigned long id)
>> +{
>> +	const struct omap_rst_map *map = reset->prm->data->rstmap;
>> +
>> +	while (map && map->rst >= 0) {
> 
> If rstmap is never NULL,
> 
> 	while (map->rst >= 0) {
> 
> should be enough.

I'll actually re-write this to use the reset mask.

> 
>> +		if (map->rst == id)
>> +			return true;
>> +		map++;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>> +			     unsigned long id)
>> +{
>> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +	u32 v;
>> +
>> +	if (!_is_valid_reset(reset, id))
>> +		return -EINVAL;
> 
> Don't check this in the status/(de)assert/reset callbacks. Instead,
> implement rcdev.of_xlate and return -EINVAL there, so that invalid ids
> can never be requested.

Yeah, let me do that.

> 
>> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>> +	v &= 1 << id;
>> +	v >>= id;
> 
> omap_get_st_bit below makes it look like the status bit can be in a
> different place than the reset control bit, should that be used here as
> well?

True, this is a bug.

> 
>> +
>> +	return v;
>> +}
>> +
>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>> +			     unsigned long id)
>> +{
>> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +	u32 v;
>> +
>> +	if (!_is_valid_reset(reset, id))
>> +		return -EINVAL;
> 
> Same as above.

Will drop this one.

> 
>> +	/* assert the reset control line */
>> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
>> +	v |= 1 << id;
>> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
> 
> This read-modify-write should be protected with a lock.

Ok, will add a spinlock. I did think about this before but all the cases 
where we are sharing a reset register are to be controlled from the same 
process / driver, and the events are effectively serialized. Doesn't 
hurt adding it for possible future need though.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>> +				 unsigned long id)
>> +{
>> +	const struct omap_rst_map *map = reset->prm->data->rstmap;
>> +
>> +	while (map && map->rst >= 0) {
> 
> Same as above.

Yeah, usage of rstmap is now enforced, so no need to check it here.

> 
>> +		if (map->rst == id)
>> +			return map->st;
>> +
>> +		map++;
>> +	}
>> +
>> +	return id;
>> +}
>> +
>> +/*
>> + * Note that status will not change until clocks are on, and clocks cannot be
>> + * enabled until reset is deasserted. Consumer drivers must check status
>> + * separately after enabling clocks.
>> + */
>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>> +			       unsigned long id)
>> +{
>> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +	u32 v;
>> +	int st_bit;
>> +	bool has_rstst;
>> +
>> +	if (!_is_valid_reset(reset, id))
>> +		return -EINVAL;
> 
> Same as above.

Will drop.

> 
>> +	/* check the current status to avoid de-asserting the line twice */
>> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
>> +	if (!(v & BIT(id)))
>> +		return -EEXIST;
> 
> What is the purpose of this? For shared consumers the core already does
> refcounting, and I expect exclusive consumers won't deassert twice.
> Since the reset signal is deasserted after this call, this should not
> return an error.

This is actually a leftover from legacy code; this driver is mostly a 
move of the reset handling from platform codebase to be an actual driver 
of its own. But yes, I believe this can be dropped.

> 
>> +
>> +	has_rstst = reset->prm->data->rstst ||
>> +		(reset->prm->data->flags & OMAP_PRM_HAS_RSTST);
>> +
>> +	if (has_rstst) {
>> +		st_bit = omap_reset_get_st_bit(reset, id);
>> +
>> +		/* Clear the reset status by writing 1 to the status bit */
>> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>> +		v |= 1 << st_bit;
>> +		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
> 
> What does the value read from the rstst register indicate? Is it the
> current state of the reset line? Is it 0 until deassertion is completed,
> and then it turns to 1?

Value of 1 indicates that the corresponding IP has been reset 
successfully. Writing back 1 to the same bit clears it out, so the 
status can be polled later on.

> 
>> +	}
>> +
>> +	/* de-assert the reset control line */
>> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
> 
> Reading the register again seems unnecessary.

I dropped the earlier read for now, so this is again needed.

> 
>> +	v &= ~(1 << id);
>> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
> 
> As above, the read-modify-write should be locked.

Yep, will protect this.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct reset_control_ops omap_reset_ops = {
>> +	.assert		= omap_reset_assert,
>> +	.deassert	= omap_reset_deassert,
>> +	.status		= omap_reset_status,
>> +};
>> +
>> +static int omap_prm_reset_init(struct platform_device *pdev,
>> +			       struct omap_prm *prm)
>> +{
>> +	struct omap_reset_data *reset;
>> +
>> +	/*
>> +	 * Check if we have controllable resets. If either rstctrl is non-zero
>> +	 * or OMAP_PRM_HAS_RSTCTRL flag is set, we have reset control register
>> +	 * for the domain.
>> +	 */
>> +	if (!prm->data->rstctrl && !(prm->data->flags & OMAP_PRM_HAS_RSTCTRL))
>> +		return 0;
>> +
>> +	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>> +	if (!reset)
>> +		return -ENOMEM;
>> +
>> +	reset->rcdev.owner = THIS_MODULE;
>> +	reset->rcdev.ops = &omap_reset_ops;
>> +	reset->rcdev.of_node = pdev->dev.of_node;
>> +	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>> +
>> +	reset->prm = prm;
>> +
>> +	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>> +}
>> +
>> +static int omap_prm_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	const struct omap_prm_data *data;
>> +	struct omap_prm *prm;
>> +	const struct of_device_id *match;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res)
>> +		return -ENODEV;
> 
> This can be merged with devm_ioremap_resource below.

Well, I actually use the "res" later on to map the DT node to the 
corresponding prm_data based on address.

> 
>> +	match = of_match_device(omap_prm_id_table, &pdev->dev);
>> +	if (!match)
>> +		return -ENOTSUPP;
>> +
>> +	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>> +	if (!prm)
>> +		return -ENOMEM;
>> +
>> +	data = match->data;
>> +
>> +	while (data->base != res->start) {
>> +		if (!data->base)
>> +			return -EINVAL;
>> +		data++;
>> +	}
> 
> Is this not something that you want to have encoded in the compatible
> string? They all have a different register layout.

With the addition of all the prm instances later on, this changes. Most 
of the prm instances will have same register layout then. See v1 data 
that was posted earlier [1], but which I dropped for now to keep this 
series isolated for reset handling only. In this patch, you see that for 
DRA7, all the power domain handling related PRM instances have identical 
register layout, they just differ based on base address.

[1] https://www.spinics.net/lists/linux-omap/msg149872.html

It would be possible to encode all of this based on different 
compatibles, but then the amount of different compatible strings would 
explode... DRA7 is just one SoC.

> 
>> +
>> +	prm->data = data;
>> +
>> +	prm->base = devm_ioremap_resource(&pdev->dev, res);
> 
> 	prm->base = devm_platform_ioremap_resource(pdev, 0);

I still need the "res" pointer as indicated above.

-Tero

> 
>> +	if (!prm->base)
>> +		return -ENOMEM;
>> +
>> +	return omap_prm_reset_init(pdev, prm);
>> +}
>> +
>> +static struct platform_driver omap_prm_driver = {
>> +	.probe = omap_prm_probe,
>> +	.driver = {
>> +		.name		= KBUILD_MODNAME,
>> +		.of_match_table	= omap_prm_id_table,
>> +	},
>> +};
>> +builtin_platform_driver(omap_prm_driver);
> 
> regards
> Philipp
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCHv2 02/11] soc: ti: add initial PRM driver with reset control support

[Philipp Zabel]

Hi Tero,

On Fri, 2019-08-30 at 10:28 +0300, Tero Kristo wrote:
> On 29/08/2019 16:12, Philipp Zabel wrote:
[...]
> > I wonder if splitting rstctrl/rstst/rstmap out into a separate structure
> > would make sense. That could be linked from omap_reset_data directly.
> > That only makes sense if there'd be enough cases where it can be reused
> > for multiple PRMs instances.
> 
> Hmm, splitting these out would make it possible to share the bits for 
> ipu:s across devices, same for dsp:s and eve:s.
> 
> However, adding too many levels of abstraction makes it kind of 
> difficult to follow what is happening with the code, and it would only 
> save maybe ~100 bytes of memory at the moment.

Good point, that is likely not worth the additional complexity.

[...]
> > What does the value read from the rstst register indicate? Is it the
> > current state of the reset line? Is it 0 until deassertion is completed,
> > and then it turns to 1?
> 
> Value of 1 indicates that the corresponding IP has been reset 
> successfully. Writing back 1 to the same bit clears it out, so the 
> status can be polled later on.

Then this should not be returned directly by reset_control_status:

/**
 * reset_control_status - returns a negative errno if not supported, a                                                                        
 * positive value if the reset line is asserted, or zero if the reset
 * line is not asserted or if the desc is NULL (optional reset).
 */

This should return 0 only if the control bit is deasserted and the
status bit is already set.

If either the control bit is asserted, or if the status bit is still
cleared, this should return 1.

[...]
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res)
> > > +		return -ENODEV;
> > 
> > This can be merged with devm_ioremap_resource below.
> 
> Well, I actually use the "res" later on to map the DT node to the 
> corresponding prm_data based on address.

Ok. I glanced over the data->base loop below after questioning whether
it is needed at all.

> > 
> > > +	match = of_match_device(omap_prm_id_table, &pdev->dev);
> > > +	if (!match)
> > > +		return -ENOTSUPP;
> > > +
> > > +	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
> > > +	if (!prm)
> > > +		return -ENOMEM;
> > > +
> > > +	data = match->data;
> > > +
> > > +	while (data->base != res->start) {
> > > +		if (!data->base)
> > > +			return -EINVAL;
> > > +		data++;
> > > +	}
> > 
> > Is this not something that you want to have encoded in the compatible
> > string? They all have a different register layout.
> 
> With the addition of all the prm instances later on, this changes. Most 
> of the prm instances will have same register layout then. See v1 data 
> that was posted earlier [1], but which I dropped for now to keep this 
> series isolated for reset handling only. In this patch, you see that for 
> DRA7, all the power domain handling related PRM instances have identical 
> register layout, they just differ based on base address.
>
> [1] https://www.spinics.net/lists/linux-omap/msg149872.html
> 
> It would be possible to encode all of this based on different 
> compatibles, but then the amount of different compatible strings would 
> explode... DRA7 is just one SoC.

I see only 3 different layouts in that patch. All instances with
identical layout could share the same compatible.

Either way, that is DT bindings territory, and not for me to decide. I
just found it unusual to encode the device type by its base address in
the driver.

> > 
> > > +
> > > +	prm->data = data;
> > > +
> > > +	prm->base = devm_ioremap_resource(&pdev->dev, res);
> > 
> > 	prm->base = devm_platform_ioremap_resource(pdev, 0);
> 
> I still need the "res" pointer as indicated above.

Got it. If the lookup by base address is needed, this has to stay split.

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 02/11] soc: ti: add initial PRM driver with reset control support

[Tero Kristo]

On 30/08/2019 12:02, Philipp Zabel wrote:
> Hi Tero,
> 
> On Fri, 2019-08-30 at 10:28 +0300, Tero Kristo wrote:
>> On 29/08/2019 16:12, Philipp Zabel wrote:
> [...]
>>> I wonder if splitting rstctrl/rstst/rstmap out into a separate structure
>>> would make sense. That could be linked from omap_reset_data directly.
>>> That only makes sense if there'd be enough cases where it can be reused
>>> for multiple PRMs instances.
>>
>> Hmm, splitting these out would make it possible to share the bits for
>> ipu:s across devices, same for dsp:s and eve:s.
>>
>> However, adding too many levels of abstraction makes it kind of
>> difficult to follow what is happening with the code, and it would only
>> save maybe ~100 bytes of memory at the moment.
> 
> Good point, that is likely not worth the additional complexity.
> 
> [...]
>>> What does the value read from the rstst register indicate? Is it the
>>> current state of the reset line? Is it 0 until deassertion is completed,
>>> and then it turns to 1?
>>
>> Value of 1 indicates that the corresponding IP has been reset
>> successfully. Writing back 1 to the same bit clears it out, so the
>> status can be polled later on.
> 
> Then this should not be returned directly by reset_control_status:
> 
> /**
>   * reset_control_status - returns a negative errno if not supported, a
>   * positive value if the reset line is asserted, or zero if the reset
>   * line is not asserted or if the desc is NULL (optional reset).
>   */
> 
> This should return 0 only if the control bit is deasserted and the
> status bit is already set.
> 
> If either the control bit is asserted, or if the status bit is still
> cleared, this should return 1.

Hmm ok, let me fix that a bit also.

> 
> [...]
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!res)
>>>> +		return -ENODEV;
>>>
>>> This can be merged with devm_ioremap_resource below.
>>
>> Well, I actually use the "res" later on to map the DT node to the
>> corresponding prm_data based on address.
> 
> Ok. I glanced over the data->base loop below after questioning whether
> it is needed at all.
> 
>>>
>>>> +	match = of_match_device(omap_prm_id_table, &pdev->dev);
>>>> +	if (!match)
>>>> +		return -ENOTSUPP;
>>>> +
>>>> +	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>>>> +	if (!prm)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	data = match->data;
>>>> +
>>>> +	while (data->base != res->start) {
>>>> +		if (!data->base)
>>>> +			return -EINVAL;
>>>> +		data++;
>>>> +	}
>>>
>>> Is this not something that you want to have encoded in the compatible
>>> string? They all have a different register layout.
>>
>> With the addition of all the prm instances later on, this changes. Most
>> of the prm instances will have same register layout then. See v1 data
>> that was posted earlier [1], but which I dropped for now to keep this
>> series isolated for reset handling only. In this patch, you see that for
>> DRA7, all the power domain handling related PRM instances have identical
>> register layout, they just differ based on base address.
>>
>> [1] https://www.spinics.net/lists/linux-omap/msg149872.html
>>
>> It would be possible to encode all of this based on different
>> compatibles, but then the amount of different compatible strings would
>> explode... DRA7 is just one SoC.
> 
> I see only 3 different layouts in that patch. All instances with
> identical layout could share the same compatible.
> 
> Either way, that is DT bindings territory, and not for me to decide. I
> just found it unusual to encode the device type by its base address in
> the driver.

I'll try to poke Rob for comment on that on the bindings patch.

> 
>>>
>>>> +
>>>> +	prm->data = data;
>>>> +
>>>> +	prm->base = devm_ioremap_resource(&pdev->dev, res);
>>>
>>> 	prm->base = devm_platform_ioremap_resource(pdev, 0);
>>
>> I still need the "res" pointer as indicated above.
> 
> Got it. If the lookup by base address is needed, this has to stay split.

Ok thanks for the reviews, much appreciated. I'll see if I can get reply 
from Rob on the binding item, otherwise I might just send v2 with the 
existing bindings out and send a v3 with updated bindings if necessary.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 03/11] soc: ti: omap-prm: poll for reset complete during de-assert

[Tero Kristo]

Poll for reset completion status during de-assertion of reset, otherwise
the IP in question might be accessed before it has left reset properly.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index fd5c431f8736..afeb70761b27 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -127,6 +127,7 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 	u32 v;
 	int st_bit;
 	bool has_rstst;
+	int timeout = 0;
 
 	if (!_is_valid_reset(reset, id))
 		return -EINVAL;
@@ -153,6 +154,25 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 	v &= ~(1 << id);
 	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
 
+	if (!has_rstst)
+		return 0;
+
+	/* wait for the status to be set */
+	while (1) {
+		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
+		v &= 1 << st_bit;
+		if (v)
+			break;
+		timeout++;
+		if (timeout > OMAP_RESET_MAX_WAIT) {
+			pr_err("%s: timedout waiting for %s:%lu\n", __func__,
+			       dev_name(rcdev->dev), id);
+			return -EBUSY;
+		}
+
+		udelay(1);
+	}
+
 	return 0;
 }
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCHv2 03/11] soc: ti: omap-prm: poll for reset complete during de-assert

[Philipp Zabel]

Hi Tero,

On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:
> Poll for reset completion status during de-assertion of reset, otherwise
> the IP in question might be accessed before it has left reset properly.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index fd5c431f8736..afeb70761b27 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -127,6 +127,7 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>  	u32 v;
>  	int st_bit;
>  	bool has_rstst;
> +	int timeout = 0;
>  
>  	if (!_is_valid_reset(reset, id))
>  		return -EINVAL;
> @@ -153,6 +154,25 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>  	v &= ~(1 << id);
>  	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
>  
> +	if (!has_rstst)
> +		return 0;
> +
> +	/* wait for the status to be set */
> +	while (1) {
> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +		v &= 1 << st_bit;
> +		if (v)
> +			break;
> +		timeout++;
> +		if (timeout > OMAP_RESET_MAX_WAIT) {
> +			pr_err("%s: timedout waiting for %s:%lu\n", __func__,
> +			       dev_name(rcdev->dev), id);
> +			return -EBUSY;
> +		}
> +
> +		udelay(1);
> +	}

This looks like you could use

	readl_relaxed_poll_timeout(_atomic)

regards
Philipp

Re: [PATCHv2 03/11] soc: ti: omap-prm: poll for reset complete during de-assert

[Tero Kristo]

On 29/08/2019 16:14, Philipp Zabel wrote:
> Hi Tero,
> 
> On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:
>> Poll for reset completion status during de-assertion of reset, otherwise
>> the IP in question might be accessed before it has left reset properly.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> index fd5c431f8736..afeb70761b27 100644
>> --- a/drivers/soc/ti/omap_prm.c
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -127,6 +127,7 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>   	u32 v;
>>   	int st_bit;
>>   	bool has_rstst;
>> +	int timeout = 0;
>>   
>>   	if (!_is_valid_reset(reset, id))
>>   		return -EINVAL;
>> @@ -153,6 +154,25 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>   	v &= ~(1 << id);
>>   	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
>>   
>> +	if (!has_rstst)
>> +		return 0;
>> +
>> +	/* wait for the status to be set */
>> +	while (1) {
>> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>> +		v &= 1 << st_bit;
>> +		if (v)
>> +			break;
>> +		timeout++;
>> +		if (timeout > OMAP_RESET_MAX_WAIT) {
>> +			pr_err("%s: timedout waiting for %s:%lu\n", __func__,
>> +			       dev_name(rcdev->dev), id);
>> +			return -EBUSY;
>> +		}
>> +
>> +		udelay(1);
>> +	}
> 
> This looks like you could use
> 
> 	readl_relaxed_poll_timeout(_atomic)

Yeah true, let me change that.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 04/11] soc: ti: omap-prm: add support for denying idle for reset clockdomain

[Tero Kristo]

TI SoCs hardware reset signals require the parent clockdomain to be
in force wakeup mode while de-asserting the reset, otherwise it may
never complete. To support this, add pdata hooks to control the
clockdomain directly.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c            | 34 +++++++++++++++++++++++++---
 include/linux/platform_data/ti-prm.h | 21 +++++++++++++++++
 2 files changed, 52 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/platform_data/ti-prm.h

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index afeb70761b27..38998ce19c71 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -16,6 +16,8 @@
 #include <linux/reset-controller.h>
 #include <linux/delay.h>
 
+#include <linux/platform_data/ti-prm.h>
+
 struct omap_rst_map {
 	s8 rst;
 	s8 st;
@@ -24,6 +26,7 @@ struct omap_rst_map {
 struct omap_prm_data {
 	u32 base;
 	const char *name;
+	const char *clkdm_name;
 	u16 rstctrl;
 	u16 rstst;
 	const struct omap_rst_map *rstmap;
@@ -38,6 +41,8 @@ struct omap_prm {
 struct omap_reset_data {
 	struct reset_controller_dev rcdev;
 	struct omap_prm *prm;
+	struct clockdomain *clkdm;
+	struct device *dev;
 };
 
 #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
@@ -128,6 +133,8 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 	int st_bit;
 	bool has_rstst;
 	int timeout = 0;
+	struct ti_prm_platform_data *pdata = dev_get_platdata(reset->dev);
+	int ret = 0;
 
 	if (!_is_valid_reset(reset, id))
 		return -EINVAL;
@@ -149,13 +156,15 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
 	}
 
+	pdata->clkdm_deny_idle(reset->clkdm);
+
 	/* de-assert the reset control line */
 	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
 	v &= ~(1 << id);
 	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
 
 	if (!has_rstst)
-		return 0;
+		goto exit;
 
 	/* wait for the status to be set */
 	while (1) {
@@ -167,13 +176,17 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 		if (timeout > OMAP_RESET_MAX_WAIT) {
 			pr_err("%s: timedout waiting for %s:%lu\n", __func__,
 			       dev_name(rcdev->dev), id);
-			return -EBUSY;
+			ret = -EBUSY;
+			goto exit;
 		}
 
 		udelay(1);
 	}
 
-	return 0;
+exit:
+	pdata->clkdm_allow_idle(reset->clkdm);
+
+	return ret;
 }
 
 static const struct reset_control_ops omap_reset_ops = {
@@ -186,6 +199,8 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 			       struct omap_prm *prm)
 {
 	struct omap_reset_data *reset;
+	struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	char buf[32];
 
 	/*
 	 * Check if we have controllable resets. If either rstctrl is non-zero
@@ -195,6 +210,11 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 	if (!prm->data->rstctrl && !(prm->data->flags & OMAP_PRM_HAS_RSTCTRL))
 		return 0;
 
+	/* Check if we have the pdata callbacks in place */
+	if (!pdata->clkdm_lookup || !pdata->clkdm_deny_idle ||
+	    !pdata->clkdm_allow_idle)
+		return -EINVAL;
+
 	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
 	if (!reset)
 		return -ENOMEM;
@@ -203,9 +223,17 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 	reset->rcdev.ops = &omap_reset_ops;
 	reset->rcdev.of_node = pdev->dev.of_node;
 	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
+	reset->dev = &pdev->dev;
 
 	reset->prm = prm;
 
+	sprintf(buf, "%s_clkdm", prm->data->clkdm_name ? prm->data->clkdm_name :
+		prm->data->name);
+
+	reset->clkdm = pdata->clkdm_lookup(buf);
+	if (!reset->clkdm)
+		return -EINVAL;
+
 	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
 }
 
diff --git a/include/linux/platform_data/ti-prm.h b/include/linux/platform_data/ti-prm.h
new file mode 100644
index 000000000000..28154c3226c2
--- /dev/null
+++ b/include/linux/platform_data/ti-prm.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * TI PRM (Power & Reset Manager) platform data
+ *
+ * Copyright (C) 2019 Texas Instruments, Inc.
+ *
+ * Tero Kristo <t-kristo@ti.com>
+ */
+
+#ifndef _LINUX_PLATFORM_DATA_TI_PRM_H
+#define _LINUX_PLATFORM_DATA_TI_PRM_H
+
+struct clockdomain;
+
+struct ti_prm_platform_data {
+	void (*clkdm_deny_idle)(struct clockdomain *clkdm);
+	void (*clkdm_allow_idle)(struct clockdomain *clkdm);
+	struct clockdomain * (*clkdm_lookup)(const char *name);
+};
+
+#endif /* _LINUX_PLATFORM_DATA_TI_PRM_H */
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 05/11] soc: ti: omap-prm: sync func clock status with resets

[Tero Kristo]

Hardware reset signals are tightly coupled with associated clocks, and
basically de-asserting a reset won't succeed properly if the clock is
not enabled, and vice-versa. Also, disabling a clock won't fully succeed
if the associated hardware resets are not asserted. Add status sync
functionality between these two for TI drivers so that the situations
can be handled properly without generating any timeouts.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index 38998ce19c71..e876bad8f8d5 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -15,6 +15,8 @@
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
 #include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/clk/ti.h>
 
 #include <linux/platform_data/ti-prm.h>
 
@@ -42,7 +44,9 @@ struct omap_reset_data {
 	struct reset_controller_dev rcdev;
 	struct omap_prm *prm;
 	struct clockdomain *clkdm;
+	struct clk *clk;
 	struct device *dev;
+	u32 mask;
 };
 
 #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
@@ -102,6 +106,8 @@ static int omap_reset_assert(struct reset_controller_dev *rcdev,
 	v |= 1 << id;
 	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
 
+	ti_clk_notify_resets(reset->clk, v == reset->mask);
+
 	return 0;
 }
 
@@ -163,9 +169,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 	v &= ~(1 << id);
 	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
 
+	ti_clk_notify_resets(reset->clk, v == reset->mask);
+
 	if (!has_rstst)
 		goto exit;
 
+	/* If associated clock is disabled, we can't poll completion status */
+	if (reset->clk) {
+		struct clk_hw *hw = __clk_get_hw(reset->clk);
+
+		if (!clk_hw_is_enabled(hw))
+			return ret;
+	}
+
 	/* wait for the status to be set */
 	while (1) {
 		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
@@ -199,8 +215,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 			       struct omap_prm *prm)
 {
 	struct omap_reset_data *reset;
+	const struct omap_rst_map *map;
 	struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	char buf[32];
+	u32 v;
 
 	/*
 	 * Check if we have controllable resets. If either rstctrl is non-zero
@@ -215,6 +233,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 	    !pdata->clkdm_allow_idle)
 		return -EINVAL;
 
+	map = prm->data->rstmap;
+	if (!map)
+		return -EINVAL;
+
 	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
 	if (!reset)
 		return -ENOMEM;
@@ -224,6 +246,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 	reset->rcdev.of_node = pdev->dev.of_node;
 	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
 	reset->dev = &pdev->dev;
+	reset->clk = of_clk_get(pdev->dev.of_node, 0);
+
+	if (IS_ERR(reset->clk))
+		reset->clk = NULL;
 
 	reset->prm = prm;
 
@@ -234,6 +260,16 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 	if (!reset->clkdm)
 		return -EINVAL;
 
+	while (map->rst >= 0) {
+		reset->mask |= BIT(map->rst);
+		map++;
+	}
+
+	if (reset->clk) {
+		v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
+		ti_clk_notify_resets(reset->clk, v == reset->mask);
+	}
+
 	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
 }
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCHv2 05/11] soc: ti: omap-prm: sync func clock status with resets

[Philipp Zabel]

On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:
> Hardware reset signals are tightly coupled with associated clocks, and
> basically de-asserting a reset won't succeed properly if the clock is
> not enabled, and vice-versa. Also, disabling a clock won't fully succeed
> if the associated hardware resets are not asserted. Add status sync
> functionality between these two for TI drivers so that the situations
> can be handled properly without generating any timeouts.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/soc/ti/omap_prm.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index 38998ce19c71..e876bad8f8d5 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -15,6 +15,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
>  #include <linux/delay.h>
> +#include <linux/clk.h>
> +#include <linux/clk/ti.h>
>  
>  #include <linux/platform_data/ti-prm.h>
>  
> @@ -42,7 +44,9 @@ struct omap_reset_data {
>  	struct reset_controller_dev rcdev;
>  	struct omap_prm *prm;
>  	struct clockdomain *clkdm;
> +	struct clk *clk;
>  	struct device *dev;
> +	u32 mask;
>  };
>  
>  #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
> @@ -102,6 +106,8 @@ static int omap_reset_assert(struct reset_controller_dev *rcdev,
>  	v |= 1 << id;
>  	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
>  
> +	ti_clk_notify_resets(reset->clk, v == reset->mask);
> +
>  	return 0;
>  }
>  
> @@ -163,9 +169,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>  	v &= ~(1 << id);
>  	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
>  
> +	ti_clk_notify_resets(reset->clk, v == reset->mask);
> +
>  	if (!has_rstst)
>  		goto exit;
>  
> +	/* If associated clock is disabled, we can't poll completion status */
> +	if (reset->clk) {
> +		struct clk_hw *hw = __clk_get_hw(reset->clk);
> +
> +		if (!clk_hw_is_enabled(hw))
> +			return ret;
> +	}
> +
>  	/* wait for the status to be set */
>  	while (1) {
>  		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> @@ -199,8 +215,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
>  			       struct omap_prm *prm)
>  {
>  	struct omap_reset_data *reset;
> +	const struct omap_rst_map *map;
>  	struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev);
>  	char buf[32];
> +	u32 v;
>  
>  	/*
>  	 * Check if we have controllable resets. If either rstctrl is non-zero
> @@ -215,6 +233,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
>  	    !pdata->clkdm_allow_idle)
>  		return -EINVAL;
>  
> +	map = prm->data->rstmap;
> +	if (!map)
> +		return -EINVAL;

Can this actually happen?

> +
>  	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>  	if (!reset)
>  		return -ENOMEM;
> @@ -224,6 +246,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
>  	reset->rcdev.of_node = pdev->dev.of_node;
>  	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>  	reset->dev = &pdev->dev;
> +	reset->clk = of_clk_get(pdev->dev.of_node, 0);
> +
> +	if (IS_ERR(reset->clk))
> +		reset->clk = NULL;

Maybe only ignore -ENOENT?

>  	reset->prm = prm;
>  
> @@ -234,6 +260,16 @@ static int omap_prm_reset_init(struct platform_device *pdev,
>  	if (!reset->clkdm)
>  		return -EINVAL;
>  
> +	while (map->rst >= 0) {
> +		reset->mask |= BIT(map->rst);
> +		map++;
> +	}

With this, you could use reset->mask to simplify _is_valid_reset.

regards
Philipp

Re: [PATCHv2 05/11] soc: ti: omap-prm: sync func clock status with resets

[Tero Kristo]

On 29/08/2019 16:25, Philipp Zabel wrote:
> On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:
>> Hardware reset signals are tightly coupled with associated clocks, and
>> basically de-asserting a reset won't succeed properly if the clock is
>> not enabled, and vice-versa. Also, disabling a clock won't fully succeed
>> if the associated hardware resets are not asserted. Add status sync
>> functionality between these two for TI drivers so that the situations
>> can be handled properly without generating any timeouts.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/soc/ti/omap_prm.c | 36 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> index 38998ce19c71..e876bad8f8d5 100644
>> --- a/drivers/soc/ti/omap_prm.c
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -15,6 +15,8 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/reset-controller.h>
>>   #include <linux/delay.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk/ti.h>
>>   
>>   #include <linux/platform_data/ti-prm.h>
>>   
>> @@ -42,7 +44,9 @@ struct omap_reset_data {
>>   	struct reset_controller_dev rcdev;
>>   	struct omap_prm *prm;
>>   	struct clockdomain *clkdm;
>> +	struct clk *clk;
>>   	struct device *dev;
>> +	u32 mask;
>>   };
>>   
>>   #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
>> @@ -102,6 +106,8 @@ static int omap_reset_assert(struct reset_controller_dev *rcdev,
>>   	v |= 1 << id;
>>   	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
>>   
>> +	ti_clk_notify_resets(reset->clk, v == reset->mask);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -163,9 +169,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>   	v &= ~(1 << id);
>>   	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
>>   
>> +	ti_clk_notify_resets(reset->clk, v == reset->mask);
>> +
>>   	if (!has_rstst)
>>   		goto exit;
>>   
>> +	/* If associated clock is disabled, we can't poll completion status */
>> +	if (reset->clk) {
>> +		struct clk_hw *hw = __clk_get_hw(reset->clk);
>> +
>> +		if (!clk_hw_is_enabled(hw))
>> +			return ret;
>> +	}
>> +
>>   	/* wait for the status to be set */
>>   	while (1) {
>>   		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>> @@ -199,8 +215,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
>>   			       struct omap_prm *prm)
>>   {
>>   	struct omap_reset_data *reset;
>> +	const struct omap_rst_map *map;
>>   	struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev);
>>   	char buf[32];
>> +	u32 v;
>>   
>>   	/*
>>   	 * Check if we have controllable resets. If either rstctrl is non-zero
>> @@ -215,6 +233,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
>>   	    !pdata->clkdm_allow_idle)
>>   		return -EINVAL;
>>   
>> +	map = prm->data->rstmap;
>> +	if (!map)
>> +		return -EINVAL;
> 
> Can this actually happen?

It can, if user writes bad data to the omap_prm_data structs. Without 
this check it would make the probe crash which is bad.

> 
>> +
>>   	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>   	if (!reset)
>>   		return -ENOMEM;
>> @@ -224,6 +246,10 @@ static int omap_prm_reset_init(struct platform_device *pdev,
>>   	reset->rcdev.of_node = pdev->dev.of_node;
>>   	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>>   	reset->dev = &pdev->dev;
>> +	reset->clk = of_clk_get(pdev->dev.of_node, 0);
>> +
>> +	if (IS_ERR(reset->clk))
>> +		reset->clk = NULL;
> 
> Maybe only ignore -ENOENT?

Yeah, I can modify this.

> 
>>   	reset->prm = prm;
>>   
>> @@ -234,6 +260,16 @@ static int omap_prm_reset_init(struct platform_device *pdev,
>>   	if (!reset->clkdm)
>>   		return -EINVAL;
>>   
>> +	while (map->rst >= 0) {
>> +		reset->mask |= BIT(map->rst);
>> +		map++;
>> +	}
> 
> With this, you could use reset->mask to simplify _is_valid_reset.

True, let me re-write it.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 06/11] soc: ti: omap-prm: support resets with no associated clockdomain

[Tero Kristo]

Typically hardware resets on TI SoCs are associated with a clockdomain
which must be forced to be active while the reset is being de-asserted.
Otherwise the reset may not de-assert properly leaving the IP in some
weird metastate. However, some hardware reset lines don't have this
association in place, so add support for a new PRM flag for this
purpose.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index e876bad8f8d5..d7a29e282788 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -56,6 +56,7 @@ struct omap_reset_data {
 
 #define OMAP_PRM_HAS_RSTCTRL	BIT(0)
 #define OMAP_PRM_HAS_RSTST	BIT(1)
+#define OMAP_PRM_HAS_NO_CLKDM	BIT(2)
 
 #define OMAP_PRM_HAS_RESETS	(OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_RSTST)
 
@@ -162,7 +163,8 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
 	}
 
-	pdata->clkdm_deny_idle(reset->clkdm);
+	if (reset->clkdm)
+		pdata->clkdm_deny_idle(reset->clkdm);
 
 	/* de-assert the reset control line */
 	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
@@ -200,7 +202,8 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 	}
 
 exit:
-	pdata->clkdm_allow_idle(reset->clkdm);
+	if (reset->clkdm)
+		pdata->clkdm_allow_idle(reset->clkdm);
 
 	return ret;
 }
@@ -229,7 +232,7 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 		return 0;
 
 	/* Check if we have the pdata callbacks in place */
-	if (!pdata->clkdm_lookup || !pdata->clkdm_deny_idle ||
+	if (!pdata || !pdata->clkdm_lookup || !pdata->clkdm_deny_idle ||
 	    !pdata->clkdm_allow_idle)
 		return -EINVAL;
 
@@ -256,9 +259,11 @@ static int omap_prm_reset_init(struct platform_device *pdev,
 	sprintf(buf, "%s_clkdm", prm->data->clkdm_name ? prm->data->clkdm_name :
 		prm->data->name);
 
-	reset->clkdm = pdata->clkdm_lookup(buf);
-	if (!reset->clkdm)
-		return -EINVAL;
+	if (!(prm->data->flags & OMAP_PRM_HAS_NO_CLKDM)) {
+		reset->clkdm = pdata->clkdm_lookup(buf);
+		if (!reset->clkdm)
+			return -EINVAL;
+	}
 
 	while (map->rst >= 0) {
 		reset->mask |= BIT(map->rst);
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 07/11] soc: ti: omap-prm: add omap4 PRM data

[Tero Kristo]

Add PRM data for omap4 family of SoCs. Initially this is just used to
provide reset support.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index d7a29e282788..192eeae67dfc 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -60,7 +60,29 @@ struct omap_reset_data {
 
 #define OMAP_PRM_HAS_RESETS	(OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_RSTST)
 
+static const struct omap_rst_map rst_map_01[] = {
+	{ .rst = 0, .st = 0 },
+	{ .rst = 1, .st = 1 },
+	{ .rst = -1 },
+};
+
+static const struct omap_rst_map rst_map_012[] = {
+	{ .rst = 0, .st = 0 },
+	{ .rst = 1, .st = 1 },
+	{ .rst = 2, .st = 2 },
+	{ .rst = -1 },
+};
+
+static const struct omap_prm_data omap4_prm_data[] = {
+	{ .name = "tesla", .base = 0x4a306400, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
+	{ .name = "core", .base = 0x4a306700, .rstctrl = 0x210, .rstst = 0x214, .clkdm_name = "ducati", .rstmap = rst_map_012 },
+	{ .name = "ivahd", .base = 0x4a306f00, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_012 },
+	{ .name = "device", .base = 0x4a307b00, .rstctrl = 0x0, .rstst = 0x4, .rstmap = rst_map_01, .flags = OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_NO_CLKDM },
+	{ },
+};
+
 static const struct of_device_id omap_prm_id_table[] = {
+	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
 	{ },
 };
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 08/11] soc: ti: omap-prm: add data for am33xx

[Tero Kristo]

Add PRM instance data for AM33xx SoC. Includes some basic register
definitions and reset data for now.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index 192eeae67dfc..c4b33214bce1 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -60,6 +60,11 @@ struct omap_reset_data {
 
 #define OMAP_PRM_HAS_RESETS	(OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_RSTST)
 
+static const struct omap_rst_map rst_map_0[] = {
+	{ .rst = 0, .st = 0 },
+	{ .rst = -1 },
+};
+
 static const struct omap_rst_map rst_map_01[] = {
 	{ .rst = 0, .st = 0 },
 	{ .rst = 1, .st = 1 },
@@ -81,8 +86,27 @@ static const struct omap_prm_data omap4_prm_data[] = {
 	{ },
 };
 
+static const struct omap_rst_map am3_per_rst_map[] = {
+	{ .rst = 1 },
+	{ .rst = -1 },
+};
+
+static const struct omap_rst_map am3_wkup_rst_map[] = {
+	{ .rst = 3, .st = 5 },
+	{ .rst = -1 },
+};
+
+static const struct omap_prm_data am3_prm_data[] = {
+	{ .name = "per", .base = 0x44e00c00, .rstctrl = 0x0, .rstmap = am3_per_rst_map, .flags = OMAP_PRM_HAS_RSTCTRL, .clkdm_name = "pruss_ocp" },
+	{ .name = "wkup", .base = 0x44e00d00, .rstctrl = 0x0, .rstst = 0xc, .rstmap = am3_wkup_rst_map, .flags = OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_NO_CLKDM },
+	{ .name = "device", .base = 0x44e00f00, .rstctrl = 0x0, .rstst = 0x8, .rstmap = rst_map_01, .flags = OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_NO_CLKDM },
+	{ .name = "gfx", .base = 0x44e01100, .rstctrl = 0x4, .rstst = 0x14, .rstmap = rst_map_0, .clkdm_name = "gfx_l3" },
+	{ },
+};
+
 static const struct of_device_id omap_prm_id_table[] = {
 	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
+	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
 	{ },
 };
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 09/11] soc: ti: omap-prm: add dra7 PRM data

[Tero Kristo]

Add PRM instance data for dra7 family of SoCs. Initially this is just
used to provide reset support.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index c4b33214bce1..a54c2e556b7a 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -86,6 +86,19 @@ static const struct omap_prm_data omap4_prm_data[] = {
 	{ },
 };
 
+static const struct omap_prm_data dra7_prm_data[] = {
+	{ .name = "dsp1", .base = 0x4ae06400, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
+	{ .name = "ipu", .base = 0x4ae06500, .rstctrl = 0x10, .rstst = 0x14, .clkdm_name = "ipu1", .rstmap = rst_map_012 },
+	{ .name = "core", .base = 0x4ae06700, .rstctrl = 0x210, .rstst = 0x214, .clkdm_name = "ipu2", .rstmap = rst_map_012 },
+	{ .name = "iva", .base = 0x4ae06f00, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_012 },
+	{ .name = "dsp2", .base = 0x4ae07b00, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
+	{ .name = "eve1", .base = 0x4ae07b40, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
+	{ .name = "eve2", .base = 0x4ae07b80, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
+	{ .name = "eve3", .base = 0x4ae07bc0, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
+	{ .name = "eve4", .base = 0x4ae07c00, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
+	{ },
+};
+
 static const struct omap_rst_map am3_per_rst_map[] = {
 	{ .rst = 1 },
 	{ .rst = -1 },
@@ -106,6 +119,7 @@ static const struct omap_prm_data am3_prm_data[] = {
 
 static const struct of_device_id omap_prm_id_table[] = {
 	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
+	{ .compatible = "ti,dra7-prm-inst", .data = dra7_prm_data },
 	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
 	{ },
 };
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 10/11] soc: ti: omap-prm: add am4 PRM data

[Tero Kristo]

Add PRM instance data for am4 family of SoCs. Initially this is just
used to provide reset support.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index a54c2e556b7a..fd11785637ff 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -117,10 +117,30 @@ static const struct omap_prm_data am3_prm_data[] = {
 	{ },
 };
 
+static const struct omap_rst_map am4_per_rst_map[] = {
+	{ .rst = 1, .st = 0 },
+	{ .rst = -1 },
+};
+
+static const struct omap_rst_map am4_device_rst_map[] = {
+	{ .rst = 0, .st = 1 },
+	{ .rst = 1, .st = 0 },
+	{ .rst = -1 },
+};
+
+static const struct omap_prm_data am4_prm_data[] = {
+	{ .name = "gfx", .base = 0x44df0400, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_0, .clkdm_name = "gfx_l3" },
+	{ .name = "per", .base = 0x44df0800, .rstctrl = 0x10, .rstst = 0x14, .rstmap = am4_per_rst_map, .clkdm_name = "pruss_ocp" },
+	{ .name = "wkup", .base = 0x44df2000, .rstctrl = 0x10, .rstst = 0x14, .rstmap = am3_wkup_rst_map, .flags = OMAP_PRM_HAS_NO_CLKDM },
+	{ .name = "device", .base = 0x44df4000, .rstctrl = 0x0, .rstst = 0x4, .rstmap = am4_device_rst_map, .flags = OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_NO_CLKDM },
+	{ },
+};
+
 static const struct of_device_id omap_prm_id_table[] = {
 	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
 	{ .compatible = "ti,dra7-prm-inst", .data = dra7_prm_data },
 	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
+	{ .compatible = "ti,am4-prm-inst", .data = am4_prm_data },
 	{ },
 };
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 11/11] soc: ti: omap-prm: add omap5 PRM data

[Tero Kristo]

Add PRM instance data for omap5 family of SoCs. Initially this is just
used to provide reset support.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index fd11785637ff..1a02e319fc63 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -86,6 +86,14 @@ static const struct omap_prm_data omap4_prm_data[] = {
 	{ },
 };
 
+static const struct omap_prm_data omap5_prm_data[] = {
+	{ .name = "dsp", .base = 0x4ae06400, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
+	{ .name = "core", .base = 0x4ae06700, .rstctrl = 0x210, .rstst = 0x214, .clkdm_name = "ipu", .rstmap = rst_map_012 },
+	{ .name = "iva", .base = 0x4ae07200, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_012 },
+	{ .name = "device", .base = 0x4ae07c00, .rstctrl = 0x0, .rstst = 0x4, .rstmap = rst_map_01, .flags = OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_NO_CLKDM },
+	{ },
+};
+
 static const struct omap_prm_data dra7_prm_data[] = {
 	{ .name = "dsp1", .base = 0x4ae06400, .rstctrl = 0x10, .rstst = 0x14, .rstmap = rst_map_01 },
 	{ .name = "ipu", .base = 0x4ae06500, .rstctrl = 0x10, .rstst = 0x14, .clkdm_name = "ipu1", .rstmap = rst_map_012 },
@@ -138,6 +146,7 @@ static const struct omap_prm_data am4_prm_data[] = {
 
 static const struct of_device_id omap_prm_id_table[] = {
 	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
+	{ .compatible = "ti,omap5-prm-inst", .data = omap5_prm_data },
 	{ .compatible = "ti,dra7-prm-inst", .data = dra7_prm_data },
 	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
 	{ .compatible = "ti,am4-prm-inst", .data = am4_prm_data },
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki