linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] reset: Add Broadcom STB SW_INIT reset controller driver
@ 2018-12-21  1:34 Florian Fainelli
  2018-12-21  1:34 ` [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller Florian Fainelli
  2018-12-21  1:34 ` [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
  0 siblings, 2 replies; 13+ messages in thread
From: Florian Fainelli @ 2018-12-21  1:34 UTC (permalink / raw)
  To: p.zabel, linux-kernel
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Florian Fainelli, Rob Herring,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

Hi Philipp,

This patch series adds support for the Broadcom STB reset controller
using SW_INIT-style registers which have one or more banks of 32
reset lines and separate CLEAR/SET/STATUS registers.

Thanks!

Florian Fainelli (2):
  dt-bindings: reset: Add document for Broadcom STB reset controller
  reset: Add Broadcom STB SW_INIT reset controller driver

 .../devicetree/bindings/reset/brcm,reset.txt  |  27 ++++
 drivers/reset/Kconfig                         |   7 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-brcmstb.c                 | 121 ++++++++++++++++++
 4 files changed, 156 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
 create mode 100644 drivers/reset/reset-brcmstb.c

-- 
2.17.1


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2018-12-21  1:34 [PATCH 0/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
@ 2018-12-21  1:34 ` Florian Fainelli
  2018-12-31 23:13   ` Scott Branden
  2019-01-03 17:41   ` Rob Herring
  2018-12-21  1:34 ` [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
  1 sibling, 2 replies; 13+ messages in thread
From: Florian Fainelli @ 2018-12-21  1:34 UTC (permalink / raw)
  To: p.zabel, linux-kernel
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Florian Fainelli, Rob Herring,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

Add a binding document for the Broadcom STB reset controller, also known
as SW_INIT-style reset controller.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt

diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
new file mode 100644
index 000000000000..6e5341b4f891
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
@@ -0,0 +1,27 @@
+Broadcom STB SW_INIT-style reset controller
+===========================================
+
+Broadcom STB SoCs have a SW_INIT-style reset controller with separate
+SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
+reset lines.
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+Required properties:
+- compatible: should be brcm,brcmstb-reset
+- reg: register base and length
+- #reset-cells: must be set to 1
+
+Example:
+
+	reset: reset-controller@8404318 {
+		compatible = "brcm,brcmstb-reset";
+		reg = <0x8404318 0x30>;
+		#reset-cells = <1>;
+	};
+
+	&ethernet_switch {
+		resets = <&reset>;
+		reset-names = "switch";
+	};
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver
  2018-12-21  1:34 [PATCH 0/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
  2018-12-21  1:34 ` [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller Florian Fainelli
@ 2018-12-21  1:34 ` Florian Fainelli
  2019-01-02 10:44   ` Philipp Zabel
  1 sibling, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2018-12-21  1:34 UTC (permalink / raw)
  To: p.zabel, linux-kernel
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Florian Fainelli, Rob Herring,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

Add support for resetting blocks through the Linux reset controller
subsystem when reset lines are provided through a SW_INIT-style reset
controller on Broadcom STB SoCs.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/reset/Kconfig         |   7 ++
 drivers/reset/Makefile        |   1 +
 drivers/reset/reset-brcmstb.c | 121 ++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drivers/reset/reset-brcmstb.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 2e01bd833ffd..1ca03c57e049 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -40,6 +40,13 @@ config RESET_BERLIN
 	help
 	  This enables the reset controller driver for Marvell Berlin SoCs.
 
+config RESET_BRCMSTB
+	bool "Broadcom STB reset controller" if COMPILE_TEST
+	default ARCH_BRCMSTB
+	help
+	  This enables the reset controller driver for Broadcom STB SoCs using
+	  a SUN_TOP_CTRL_SW_INIT style controller.
+
 config RESET_HSDK
 	bool "Synopsys HSDK Reset Driver"
 	depends on HAS_IOMEM
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index dc7874df78d9..7395db2cb1dd 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
 obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
 obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
+obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
 obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
 obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
 obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
diff --git a/drivers/reset/reset-brcmstb.c b/drivers/reset/reset-brcmstb.c
new file mode 100644
index 000000000000..17a0bcdd6c9a
--- /dev/null
+++ b/drivers/reset/reset-brcmstb.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Broadcom STB generic reset controller for SW_INIT style reset controller
+ *
+ * Copyright (C) 2018 Broadcom
+ *
+ */
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/types.h>
+
+struct brcmstb_reset {
+	void __iomem *base;
+	unsigned int n_words;
+	struct device *dev;
+	struct reset_controller_dev rcdev;
+};
+
+#define SW_INIT_SET		0x00
+#define SW_INIT_CLEAR		0x04
+#define SW_INIT_STATUS		0x08
+
+#define SW_INIT_BIT(id)		BIT((id) & 0x1f)
+#define SW_INIT_BANK(id)	(id >> 5)
+
+#define SW_INIT_BANK_SIZE	0x18
+
+static inline
+struct brcmstb_reset *to_brcmstb(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct brcmstb_reset, rcdev);
+}
+
+static int brcmstb_reset_assert(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
+	struct brcmstb_reset *priv = to_brcmstb(rcdev);
+
+	writel_relaxed(SW_INIT_BIT(id), priv->base + off + SW_INIT_SET);
+	msleep(10);
+
+	return 0;
+}
+
+static int brcmstb_reset_deassert(struct reset_controller_dev *rcdev,
+				  unsigned long id)
+{
+	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
+	struct brcmstb_reset *priv = to_brcmstb(rcdev);
+
+	writel_relaxed(SW_INIT_BIT(id), priv->base + off + SW_INIT_CLEAR);
+	msleep(10);
+
+	return 0;
+}
+
+static int brcmstb_reset_status(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
+	struct brcmstb_reset *priv = to_brcmstb(rcdev);
+
+	return readl_relaxed(priv->base + off + SW_INIT_STATUS);
+}
+
+static const struct reset_control_ops brcmstb_reset_ops = {
+	.assert	= brcmstb_reset_assert,
+	.deassert = brcmstb_reset_deassert,
+	.status = brcmstb_reset_status,
+};
+
+static int brcmstb_reset_probe(struct platform_device *pdev)
+{
+	struct device *kdev = &pdev->dev;
+	struct brcmstb_reset *priv;
+	struct resource *res;
+
+	priv = devm_kzalloc(kdev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(kdev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	dev_set_drvdata(kdev, priv);
+
+	priv->rcdev.owner = THIS_MODULE;
+	priv->rcdev.nr_resets = (resource_size(res) / SW_INIT_BANK_SIZE) * 32;
+	priv->rcdev.ops = &brcmstb_reset_ops;
+	priv->rcdev.of_node = kdev->of_node;
+	/* Use defaults: 1 cell and simple xlate function */
+	priv->dev = kdev;
+
+	return devm_reset_controller_register(kdev, &priv->rcdev);
+}
+
+static const struct of_device_id brcmstb_reset_of_match[] = {
+	{ .compatible = "brcm,brcmstb-reset" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver brcmstb_reset_driver = {
+	.probe	= brcmstb_reset_probe,
+	.driver	= {
+		.name = "brcmstb-reset",
+		.of_match_table = brcmstb_reset_of_match,
+	},
+};
+module_platform_driver(brcmstb_reset_driver);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Broadcom STB reset controller");
+MODULE_LICENSE("GPL");
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2018-12-21  1:34 ` [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller Florian Fainelli
@ 2018-12-31 23:13   ` Scott Branden
  2019-01-02 20:38     ` Florian Fainelli
  2019-01-03 17:41   ` Rob Herring
  1 sibling, 1 reply; 13+ messages in thread
From: Scott Branden @ 2018-12-31 23:13 UTC (permalink / raw)
  To: Florian Fainelli, p.zabel, linux-kernel
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	Gregory Fong, Brian Norris,
	moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE


On 2018-12-20 5:34 p.m., Florian Fainelli wrote:
> Add a binding document for the Broadcom STB reset controller, also known
> as SW_INIT-style reset controller.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
>   1 file changed, 27 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
>
> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> new file mode 100644
> index 000000000000..6e5341b4f891
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
This is brcmstb-reset specific.  File should be brcm, brcmstb-reset.txt
> @@ -0,0 +1,27 @@
> +Broadcom STB SW_INIT-style reset controller
> +===========================================
> +
> +Broadcom STB SoCs have a SW_INIT-style reset controller with separate
> +SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
> +reset lines.
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +Required properties:
> +- compatible: should be brcm,brcmstb-reset
> +- reg: register base and length
> +- #reset-cells: must be set to 1
> +
> +Example:
> +
> +	reset: reset-controller@8404318 {
> +		compatible = "brcm,brcmstb-reset";
> +		reg = <0x8404318 0x30>;
> +		#reset-cells = <1>;
> +	};
> +
> +	&ethernet_switch {
> +		resets = <&reset>;
> +		reset-names = "switch";
> +	};

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver
  2018-12-21  1:34 ` [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
@ 2019-01-02 10:44   ` Philipp Zabel
  2019-01-02 20:38     ` Florian Fainelli
  0 siblings, 1 reply; 13+ messages in thread
From: Philipp Zabel @ 2019-01-02 10:44 UTC (permalink / raw)
  To: Florian Fainelli, linux-kernel
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	Gregory Fong, Brian Norris,
	moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

Hi Florian,

On Thu, 2018-12-20 at 17:34 -0800, Florian Fainelli wrote:
> Add support for resetting blocks through the Linux reset controller
> subsystem when reset lines are provided through a SW_INIT-style reset
> controller on Broadcom STB SoCs.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Thank you, this looks mostly good to me. I just have a few small
nitpicks and I'm curious about the mdelays, see below.

> ---
>  drivers/reset/Kconfig         |   7 ++
>  drivers/reset/Makefile        |   1 +
>  drivers/reset/reset-brcmstb.c | 121 ++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
>  create mode 100644 drivers/reset/reset-brcmstb.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 2e01bd833ffd..1ca03c57e049 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -40,6 +40,13 @@ config RESET_BERLIN
>  	help
>  	  This enables the reset controller driver for Marvell Berlin SoCs.
>  
> +config RESET_BRCMSTB
> +	bool "Broadcom STB reset controller" if COMPILE_TEST
> +	default ARCH_BRCMSTB
> +	help
> +	  This enables the reset controller driver for Broadcom STB SoCs using
> +	  a SUN_TOP_CTRL_SW_INIT style controller.
> +
>  config RESET_HSDK
>  	bool "Synopsys HSDK Reset Driver"
>  	depends on HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index dc7874df78d9..7395db2cb1dd 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
>  obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
>  obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
>  obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
> +obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
>  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> diff --git a/drivers/reset/reset-brcmstb.c b/drivers/reset/reset-brcmstb.c
> new file mode 100644
> index 000000000000..17a0bcdd6c9a
> --- /dev/null
> +++ b/drivers/reset/reset-brcmstb.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Broadcom STB generic reset controller for SW_INIT style reset controller
> + *
> + * Copyright (C) 2018 Broadcom
> + *
> + */
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/types.h>
> +
> +struct brcmstb_reset {
> +	void __iomem *base;

> +	unsigned int n_words;
> +	struct device *dev;

These two variables are not used anywhere.

> +	struct reset_controller_dev rcdev;
> +};
> +
> +#define SW_INIT_SET		0x00
> +#define SW_INIT_CLEAR		0x04
> +#define SW_INIT_STATUS		0x08
> +
> +#define SW_INIT_BIT(id)		BIT((id) & 0x1f)
> +#define SW_INIT_BANK(id)	(id >> 5)

Checkpatch suggests to use ((id) >> 5) here.

> +
> +#define SW_INIT_BANK_SIZE	0x18
> +
> +static inline
> +struct brcmstb_reset *to_brcmstb(struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct brcmstb_reset, rcdev);
> +}
> +
> +static int brcmstb_reset_assert(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{
> +	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
> +	struct brcmstb_reset *priv = to_brcmstb(rcdev);
> +
> +	writel_relaxed(SW_INIT_BIT(id), priv->base + off + SW_INIT_SET);
> +	msleep(10);

What is the purpose of the msleep(10)? Is it guaranteed that the writel
takes effect before the msleep, or could it be lingering in some store
buffer for (a part of) the duration? Also, checkpatch warns about this
being < 20 ms. You could increase the delay or use usleep_range.

> +	return 0;
> +}
> +
> +static int brcmstb_reset_deassert(struct reset_controller_dev *rcdev,
> +				  unsigned long id)
> +{
> +	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
> +	struct brcmstb_reset *priv = to_brcmstb(rcdev);
> +
> +	writel_relaxed(SW_INIT_BIT(id), priv->base + off + SW_INIT_CLEAR);
> +	msleep(10);

Same as above, what has to be delayed for 10 ms after deasserting the
reset? Is this the same for all reset lines handled by this controller?

> +
> +	return 0;
> +}
> +
> +static int brcmstb_reset_status(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{
> +	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
> +	struct brcmstb_reset *priv = to_brcmstb(rcdev);
> +
> +	return readl_relaxed(priv->base + off + SW_INIT_STATUS);

Should this be

+	return readl_relaxed(priv->base + off + SW_INIT_STATUS) &
+	       SW_INIT_BANK(id);

i.e. do the SW_INIT_STATUS registers contain 32 status bits, one for
each reset line?

> +}
> +
> +static const struct reset_control_ops brcmstb_reset_ops = {
> +	.assert	= brcmstb_reset_assert,
> +	.deassert = brcmstb_reset_deassert,
> +	.status = brcmstb_reset_status,
> +};
> +
> +static int brcmstb_reset_probe(struct platform_device *pdev)
> +{
> +	struct device *kdev = &pdev->dev;
> +	struct brcmstb_reset *priv;
> +	struct resource *res;
> +
> +	priv = devm_kzalloc(kdev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(kdev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	dev_set_drvdata(kdev, priv);
> +
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.nr_resets = (resource_size(res) / SW_INIT_BANK_SIZE) * 32;
> +	priv->rcdev.ops = &brcmstb_reset_ops;
> +	priv->rcdev.of_node = kdev->of_node;
> +	/* Use defaults: 1 cell and simple xlate function */
> +	priv->dev = kdev;

priv->dev could be removed.

> +
> +	return devm_reset_controller_register(kdev, &priv->rcdev);
> +}
> +
> +static const struct of_device_id brcmstb_reset_of_match[] = {
> +	{ .compatible = "brcm,brcmstb-reset" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver brcmstb_reset_driver = {
> +	.probe	= brcmstb_reset_probe,
> +	.driver	= {
> +		.name = "brcmstb-reset",
> +		.of_match_table = brcmstb_reset_of_match,
> +	},
> +};
> +module_platform_driver(brcmstb_reset_driver);
> +
> +MODULE_AUTHOR("Broadcom");
> +MODULE_DESCRIPTION("Broadcom STB reset controller");
> +MODULE_LICENSE("GPL");

regards
Philipp

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver
  2019-01-02 10:44   ` Philipp Zabel
@ 2019-01-02 20:38     ` Florian Fainelli
  0 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2019-01-02 20:38 UTC (permalink / raw)
  To: Philipp Zabel, Florian Fainelli, linux-kernel
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	Gregory Fong, Brian Norris,
	moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

Hi Philipp,

On 1/2/19 2:44 AM, Philipp Zabel wrote:
> Hi Florian,
> 
> On Thu, 2018-12-20 at 17:34 -0800, Florian Fainelli wrote:
>> Add support for resetting blocks through the Linux reset controller
>> subsystem when reset lines are provided through a SW_INIT-style reset
>> controller on Broadcom STB SoCs.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Thank you, this looks mostly good to me. I just have a few small
> nitpicks and I'm curious about the mdelays, see below.

Thanks, answers inline.

[snip]

>> +struct brcmstb_reset {
>> +	void __iomem *base;
> 
>> +	unsigned int n_words;
>> +	struct device *dev;
> 
> These two variables are not used anywhere.

Indeed, the first one should actually be added as a check to make sure
we have a correct resource size being passed, I will add that back in.

> 
>> +	struct reset_controller_dev rcdev;
>> +};
>> +
>> +#define SW_INIT_SET		0x00
>> +#define SW_INIT_CLEAR		0x04
>> +#define SW_INIT_STATUS		0x08
>> +
>> +#define SW_INIT_BIT(id)		BIT((id) & 0x1f)
>> +#define SW_INIT_BANK(id)	(id >> 5)
> 
> Checkpatch suggests to use ((id) >> 5) here.
> 
>> +
>> +#define SW_INIT_BANK_SIZE	0x18
>> +
>> +static inline
>> +struct brcmstb_reset *to_brcmstb(struct reset_controller_dev *rcdev)
>> +{
>> +	return container_of(rcdev, struct brcmstb_reset, rcdev);
>> +}
>> +
>> +static int brcmstb_reset_assert(struct reset_controller_dev *rcdev,
>> +				unsigned long id)
>> +{
>> +	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
>> +	struct brcmstb_reset *priv = to_brcmstb(rcdev);
>> +
>> +	writel_relaxed(SW_INIT_BIT(id), priv->base + off + SW_INIT_SET);
>> +	msleep(10);
> 
> What is the purpose of the msleep(10)? Is it guaranteed that the writel
> takes effect before the msleep, or could it be lingering in some store
> buffer for (a part of) the duration? Also, checkpatch warns about this
> being < 20 ms. You could increase the delay or use usleep_range.

Good point, let me check what the real delay requirements are with the
design team, since I suspect they were just overly conservative in their
recommendations previously

> 
>> +	return 0;
>> +}
>> +
>> +static int brcmstb_reset_deassert(struct reset_controller_dev *rcdev,
>> +				  unsigned long id)
>> +{
>> +	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
>> +	struct brcmstb_reset *priv = to_brcmstb(rcdev);
>> +
>> +	writel_relaxed(SW_INIT_BIT(id), priv->base + off + SW_INIT_CLEAR);
>> +	msleep(10);
> 
> Same as above, what has to be delayed for 10 ms after deasserting the
> reset? Is this the same for all reset lines handled by this controller?

AFAICT, all lines behave the same way, as per our SoCs reset architecture.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int brcmstb_reset_status(struct reset_controller_dev *rcdev,
>> +				unsigned long id)
>> +{
>> +	unsigned int off = SW_INIT_BANK(id) * SW_INIT_BANK_SIZE;
>> +	struct brcmstb_reset *priv = to_brcmstb(rcdev);
>> +
>> +	return readl_relaxed(priv->base + off + SW_INIT_STATUS);
> 
> Should this be
> 
> +	return readl_relaxed(priv->base + off + SW_INIT_STATUS) &
> +	       SW_INIT_BANK(id);
> 
> i.e. do the SW_INIT_STATUS registers contain 32 status bits, one for
> each reset line?

Yes they do. The same bits are used for SET/CLEAR/STATUS so this should
be something along the lines of:

return readl_relaxed(priv->base + off + SW_INIT_STATUS) & BIT(id));

thanks for spotting that!

> 
>> +}
>> +
>> +static const struct reset_control_ops brcmstb_reset_ops = {
>> +	.assert	= brcmstb_reset_assert,
>> +	.deassert = brcmstb_reset_deassert,
>> +	.status = brcmstb_reset_status,
>> +};
>> +
>> +static int brcmstb_reset_probe(struct platform_device *pdev)
>> +{
>> +	struct device *kdev = &pdev->dev;
>> +	struct brcmstb_reset *priv;
>> +	struct resource *res;
>> +
>> +	priv = devm_kzalloc(kdev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	priv->base = devm_ioremap_resource(kdev, res);
>> +	if (IS_ERR(priv->base))
>> +		return PTR_ERR(priv->base);
>> +
>> +	dev_set_drvdata(kdev, priv);
>> +
>> +	priv->rcdev.owner = THIS_MODULE;
>> +	priv->rcdev.nr_resets = (resource_size(res) / SW_INIT_BANK_SIZE) * 32;
>> +	priv->rcdev.ops = &brcmstb_reset_ops;
>> +	priv->rcdev.of_node = kdev->of_node;
>> +	/* Use defaults: 1 cell and simple xlate function */
>> +	priv->dev = kdev;
> 
> priv->dev could be removed.

Indeed, I had sprinkled dev_*() messages during debug, which required
it, but this is no longer required indeed.
-- 
Florian

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2018-12-31 23:13   ` Scott Branden
@ 2019-01-02 20:38     ` Florian Fainelli
  0 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2019-01-02 20:38 UTC (permalink / raw)
  To: Scott Branden, Florian Fainelli, p.zabel, linux-kernel
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	Gregory Fong, Brian Norris,
	moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

On 12/31/18 3:13 PM, Scott Branden wrote:
> 
> On 2018-12-20 5:34 p.m., Florian Fainelli wrote:
>> Add a binding document for the Broadcom STB reset controller, also known
>> as SW_INIT-style reset controller.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/reset/brcm,reset.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt
>> b/Documentation/devicetree/bindings/reset/brcm,reset.txt
>> new file mode 100644
>> index 000000000000..6e5341b4f891
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> This is brcmstb-reset specific.  File should be brcm, brcmstb-reset.txt

Indeed, thanks!
-- 
Florian

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2018-12-21  1:34 ` [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller Florian Fainelli
  2018-12-31 23:13   ` Scott Branden
@ 2019-01-03 17:41   ` Rob Herring
  2019-01-03 18:53     ` Florian Fainelli
  1 sibling, 1 reply; 13+ messages in thread
From: Rob Herring @ 2019-01-03 17:41 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	p.zabel, linux-kernel,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

On Thu, Dec 20, 2018 at 05:34:08PM -0800, Florian Fainelli wrote:
> Add a binding document for the Broadcom STB reset controller, also known
> as SW_INIT-style reset controller.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> new file mode 100644
> index 000000000000..6e5341b4f891
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> @@ -0,0 +1,27 @@
> +Broadcom STB SW_INIT-style reset controller
> +===========================================
> +
> +Broadcom STB SoCs have a SW_INIT-style reset controller with separate
> +SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
> +reset lines.
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +Required properties:
> +- compatible: should be brcm,brcmstb-reset
> +- reg: register base and length
> +- #reset-cells: must be set to 1
> +
> +Example:
> +
> +	reset: reset-controller@8404318 {
> +		compatible = "brcm,brcmstb-reset";
> +		reg = <0x8404318 0x30>;

Based on this address, should this be a sub-node of something else? Or 
not even a sub-node and just make the parent be a reset provider?

Rob

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2019-01-03 17:41   ` Rob Herring
@ 2019-01-03 18:53     ` Florian Fainelli
  2019-01-03 19:19       ` Rob Herring
  0 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2019-01-03 18:53 UTC (permalink / raw)
  To: Rob Herring, Florian Fainelli
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	p.zabel, linux-kernel,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

On 1/3/19 9:41 AM, Rob Herring wrote:
> On Thu, Dec 20, 2018 at 05:34:08PM -0800, Florian Fainelli wrote:
>> Add a binding document for the Broadcom STB reset controller, also known
>> as SW_INIT-style reset controller.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
>> new file mode 100644
>> index 000000000000..6e5341b4f891
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
>> @@ -0,0 +1,27 @@
>> +Broadcom STB SW_INIT-style reset controller
>> +===========================================
>> +
>> +Broadcom STB SoCs have a SW_INIT-style reset controller with separate
>> +SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
>> +reset lines.
>> +
>> +Please also refer to reset.txt in this directory for common reset
>> +controller binding usage.
>> +
>> +Required properties:
>> +- compatible: should be brcm,brcmstb-reset
>> +- reg: register base and length
>> +- #reset-cells: must be set to 1
>> +
>> +Example:
>> +
>> +	reset: reset-controller@8404318 {
>> +		compatible = "brcm,brcmstb-reset";
>> +		reg = <0x8404318 0x30>;
> 
> Based on this address, should this be a sub-node of something else? Or 
> not even a sub-node and just make the parent be a reset provider?

The reset controller is part of a larger "sundry" node which has a
collection of functionality, from pinmux/pinctrl, reset controller,
spare bits, chicken bits, anything the designers forgot to put somewhere
else and decided to put there.

If there is one thing consistent though is that given a set of 32-bit
register groups, they have a self contained functionality such that you
can break up the larger "sundry" space into smaller sub-blocks which
have one an only one functionality. Do you think this warrants a
different representation in Device Tree?
-- 
Florian

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2019-01-03 18:53     ` Florian Fainelli
@ 2019-01-03 19:19       ` Rob Herring
  2019-01-03 19:31         ` Florian Fainelli
  0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2019-01-03 19:19 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	p.zabel, linux-kernel,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

On Thu, Jan 03, 2019 at 10:53:25AM -0800, Florian Fainelli wrote:
> On 1/3/19 9:41 AM, Rob Herring wrote:
> > On Thu, Dec 20, 2018 at 05:34:08PM -0800, Florian Fainelli wrote:
> >> Add a binding document for the Broadcom STB reset controller, also known
> >> as SW_INIT-style reset controller.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
> >>  1 file changed, 27 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> >> new file mode 100644
> >> index 000000000000..6e5341b4f891
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> >> @@ -0,0 +1,27 @@
> >> +Broadcom STB SW_INIT-style reset controller
> >> +===========================================
> >> +
> >> +Broadcom STB SoCs have a SW_INIT-style reset controller with separate
> >> +SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
> >> +reset lines.
> >> +
> >> +Please also refer to reset.txt in this directory for common reset
> >> +controller binding usage.
> >> +
> >> +Required properties:
> >> +- compatible: should be brcm,brcmstb-reset
> >> +- reg: register base and length
> >> +- #reset-cells: must be set to 1
> >> +
> >> +Example:
> >> +
> >> +	reset: reset-controller@8404318 {
> >> +		compatible = "brcm,brcmstb-reset";
> >> +		reg = <0x8404318 0x30>;
> > 
> > Based on this address, should this be a sub-node of something else? Or 
> > not even a sub-node and just make the parent be a reset provider?
> 
> The reset controller is part of a larger "sundry" node which has a
> collection of functionality, from pinmux/pinctrl, reset controller,
> spare bits, chicken bits, anything the designers forgot to put somewhere
> else and decided to put there.
> 
> If there is one thing consistent though is that given a set of 32-bit
> register groups, they have a self contained functionality such that you
> can break up the larger "sundry" space into smaller sub-blocks which
> have one an only one functionality. Do you think this warrants a
> different representation in Device Tree?

With pinctrl in the mix, you're going to need sub-nodes anyways. So just 
define what this is a sub-node of.

Also, I'd prefer to have complete example for the "sundry" node and 
child nodes than partial examples spread across the tree.

Rob

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2019-01-03 19:19       ` Rob Herring
@ 2019-01-03 19:31         ` Florian Fainelli
  2019-01-03 22:54           ` Rob Herring
  0 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2019-01-03 19:31 UTC (permalink / raw)
  To: Rob Herring, Florian Fainelli
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	p.zabel, linux-kernel,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

On 1/3/19 11:19 AM, Rob Herring wrote:
> On Thu, Jan 03, 2019 at 10:53:25AM -0800, Florian Fainelli wrote:
>> On 1/3/19 9:41 AM, Rob Herring wrote:
>>> On Thu, Dec 20, 2018 at 05:34:08PM -0800, Florian Fainelli wrote:
>>>> Add a binding document for the Broadcom STB reset controller, also known
>>>> as SW_INIT-style reset controller.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>  .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
>>>>  1 file changed, 27 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
>>>> new file mode 100644
>>>> index 000000000000..6e5341b4f891
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
>>>> @@ -0,0 +1,27 @@
>>>> +Broadcom STB SW_INIT-style reset controller
>>>> +===========================================
>>>> +
>>>> +Broadcom STB SoCs have a SW_INIT-style reset controller with separate
>>>> +SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
>>>> +reset lines.
>>>> +
>>>> +Please also refer to reset.txt in this directory for common reset
>>>> +controller binding usage.
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be brcm,brcmstb-reset
>>>> +- reg: register base and length
>>>> +- #reset-cells: must be set to 1
>>>> +
>>>> +Example:
>>>> +
>>>> +	reset: reset-controller@8404318 {
>>>> +		compatible = "brcm,brcmstb-reset";
>>>> +		reg = <0x8404318 0x30>;
>>>
>>> Based on this address, should this be a sub-node of something else? Or 
>>> not even a sub-node and just make the parent be a reset provider?
>>
>> The reset controller is part of a larger "sundry" node which has a
>> collection of functionality, from pinmux/pinctrl, reset controller,
>> spare bits, chicken bits, anything the designers forgot to put somewhere
>> else and decided to put there.
>>
>> If there is one thing consistent though is that given a set of 32-bit
>> register groups, they have a self contained functionality such that you
>> can break up the larger "sundry" space into smaller sub-blocks which
>> have one an only one functionality. Do you think this warrants a
>> different representation in Device Tree?
> 
> With pinctrl in the mix, you're going to need sub-nodes anyways. So just 
> define what this is a sub-node of.

pinctrl is not necessarily something I want the kernel to control, since
we have a high level scripting language without our bootloader that
makes sure pinctrl is properly configured from early boot on all the way
to the kernel, and preserved across suspend/resume states.
pinctrl-single does work, and was occasionally used. Everything else is
typically muxes that the kernel does not need to touch/see/be aware of.

> 
> Also, I'd prefer to have complete example for the "sundry" node and 
> child nodes than partial examples spread across the tree.

I am afraid I can't provide that example because the sundry node is
really changing from chip to chip, and there is a gazillion of things in
there that the kernel typically does not even touch, like
pinmuxing/pinctrl, various mux selections etc. I could provide the
following example if that is what you are requesting?:


sun-top-ctrl: simple-bus@8404000 {
	compatible = "brcm,brcmstb-sun-top-ctrl", "simple-bus";
	reg = <0x8404000 0x708>;

	reset: reset-controller@318 {
		compatible = "brcm,brcmstb-reset";
		reg = <0x318 0x30>;
		#reset-cells = <1>;
	};
};

Would that be what you expect to see?
-- 
Florian

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2019-01-03 19:31         ` Florian Fainelli
@ 2019-01-03 22:54           ` Rob Herring
  2019-01-11 18:51             ` Florian Fainelli
  0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2019-01-03 22:54 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Philipp Zabel, linux-kernel,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

On Thu, Jan 3, 2019 at 1:31 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 1/3/19 11:19 AM, Rob Herring wrote:
> > On Thu, Jan 03, 2019 at 10:53:25AM -0800, Florian Fainelli wrote:
> >> On 1/3/19 9:41 AM, Rob Herring wrote:
> >>> On Thu, Dec 20, 2018 at 05:34:08PM -0800, Florian Fainelli wrote:
> >>>> Add a binding document for the Broadcom STB reset controller, also known
> >>>> as SW_INIT-style reset controller.
> >>>>
> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >>>> ---
> >>>>  .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
> >>>>  1 file changed, 27 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> >>>> new file mode 100644
> >>>> index 000000000000..6e5341b4f891
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> >>>> @@ -0,0 +1,27 @@
> >>>> +Broadcom STB SW_INIT-style reset controller
> >>>> +===========================================
> >>>> +
> >>>> +Broadcom STB SoCs have a SW_INIT-style reset controller with separate
> >>>> +SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
> >>>> +reset lines.
> >>>> +
> >>>> +Please also refer to reset.txt in this directory for common reset
> >>>> +controller binding usage.
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible: should be brcm,brcmstb-reset
> >>>> +- reg: register base and length
> >>>> +- #reset-cells: must be set to 1
> >>>> +
> >>>> +Example:
> >>>> +
> >>>> +  reset: reset-controller@8404318 {
> >>>> +          compatible = "brcm,brcmstb-reset";
> >>>> +          reg = <0x8404318 0x30>;
> >>>
> >>> Based on this address, should this be a sub-node of something else? Or
> >>> not even a sub-node and just make the parent be a reset provider?
> >>
> >> The reset controller is part of a larger "sundry" node which has a
> >> collection of functionality, from pinmux/pinctrl, reset controller,
> >> spare bits, chicken bits, anything the designers forgot to put somewhere
> >> else and decided to put there.
> >>
> >> If there is one thing consistent though is that given a set of 32-bit
> >> register groups, they have a self contained functionality such that you
> >> can break up the larger "sundry" space into smaller sub-blocks which
> >> have one an only one functionality. Do you think this warrants a
> >> different representation in Device Tree?
> >
> > With pinctrl in the mix, you're going to need sub-nodes anyways. So just
> > define what this is a sub-node of.
>
> pinctrl is not necessarily something I want the kernel to control, since
> we have a high level scripting language without our bootloader that
> makes sure pinctrl is properly configured from early boot on all the way
> to the kernel, and preserved across suspend/resume states.
> pinctrl-single does work, and was occasionally used. Everything else is
> typically muxes that the kernel does not need to touch/see/be aware of.

That's good. I'd rather see more platforms do that rather than have
the kernel handle it. OTOH, bootloaders often use DT too, so maybe who
handles pin muxing is irrelevant.

> > Also, I'd prefer to have complete example for the "sundry" node and
> > child nodes than partial examples spread across the tree.
>
> I am afraid I can't provide that example because the sundry node is
> really changing from chip to chip, and there is a gazillion of things in
> there that the kernel typically does not even touch, like
> pinmuxing/pinctrl, various mux selections etc. I could provide the
> following example if that is what you are requesting?:
>
>
> sun-top-ctrl: simple-bus@8404000 {
>         compatible = "brcm,brcmstb-sun-top-ctrl", "simple-bus";
>         reg = <0x8404000 0x708>;
>
>         reset: reset-controller@318 {
>                 compatible = "brcm,brcmstb-reset";
>                 reg = <0x318 0x30>;
>                 #reset-cells = <1>;
>         };
> };
>
> Would that be what you expect to see?

The problem is with this alone, you should just move #reset-cells to
the parent and remove the child node. That's all you really need from
a DT perspective. But if this is really a separate block that's reused
from chip to chip, then a separate node is fine. Typically in these
situations, I just can't tell whether it's that or just the
convenience of creating nodes for every kernel driver.

Rob

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
  2019-01-03 22:54           ` Rob Herring
@ 2019-01-11 18:51             ` Florian Fainelli
  0 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2019-01-11 18:51 UTC (permalink / raw)
  To: Rob Herring, Florian Fainelli
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Philipp Zabel, linux-kernel,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Gregory Fong,
	Brian Norris, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE

On 1/3/19 2:54 PM, Rob Herring wrote:
> On Thu, Jan 3, 2019 at 1:31 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 1/3/19 11:19 AM, Rob Herring wrote:
>>> On Thu, Jan 03, 2019 at 10:53:25AM -0800, Florian Fainelli wrote:
>>>> On 1/3/19 9:41 AM, Rob Herring wrote:
>>>>> On Thu, Dec 20, 2018 at 05:34:08PM -0800, Florian Fainelli wrote:
>>>>>> Add a binding document for the Broadcom STB reset controller, also known
>>>>>> as SW_INIT-style reset controller.
>>>>>>
>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
>>>>>>  1 file changed, 27 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..6e5341b4f891
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
>>>>>> @@ -0,0 +1,27 @@
>>>>>> +Broadcom STB SW_INIT-style reset controller
>>>>>> +===========================================
>>>>>> +
>>>>>> +Broadcom STB SoCs have a SW_INIT-style reset controller with separate
>>>>>> +SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
>>>>>> +reset lines.
>>>>>> +
>>>>>> +Please also refer to reset.txt in this directory for common reset
>>>>>> +controller binding usage.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: should be brcm,brcmstb-reset
>>>>>> +- reg: register base and length
>>>>>> +- #reset-cells: must be set to 1
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +  reset: reset-controller@8404318 {
>>>>>> +          compatible = "brcm,brcmstb-reset";
>>>>>> +          reg = <0x8404318 0x30>;
>>>>>
>>>>> Based on this address, should this be a sub-node of something else? Or
>>>>> not even a sub-node and just make the parent be a reset provider?
>>>>
>>>> The reset controller is part of a larger "sundry" node which has a
>>>> collection of functionality, from pinmux/pinctrl, reset controller,
>>>> spare bits, chicken bits, anything the designers forgot to put somewhere
>>>> else and decided to put there.
>>>>
>>>> If there is one thing consistent though is that given a set of 32-bit
>>>> register groups, they have a self contained functionality such that you
>>>> can break up the larger "sundry" space into smaller sub-blocks which
>>>> have one an only one functionality. Do you think this warrants a
>>>> different representation in Device Tree?
>>>
>>> With pinctrl in the mix, you're going to need sub-nodes anyways. So just
>>> define what this is a sub-node of.
>>
>> pinctrl is not necessarily something I want the kernel to control, since
>> we have a high level scripting language without our bootloader that
>> makes sure pinctrl is properly configured from early boot on all the way
>> to the kernel, and preserved across suspend/resume states.
>> pinctrl-single does work, and was occasionally used. Everything else is
>> typically muxes that the kernel does not need to touch/see/be aware of.
> 
> That's good. I'd rather see more platforms do that rather than have
> the kernel handle it. OTOH, bootloaders often use DT too, so maybe who
> handles pin muxing is irrelevant.
> 
>>> Also, I'd prefer to have complete example for the "sundry" node and
>>> child nodes than partial examples spread across the tree.
>>
>> I am afraid I can't provide that example because the sundry node is
>> really changing from chip to chip, and there is a gazillion of things in
>> there that the kernel typically does not even touch, like
>> pinmuxing/pinctrl, various mux selections etc. I could provide the
>> following example if that is what you are requesting?:
>>
>>
>> sun-top-ctrl: simple-bus@8404000 {
>>         compatible = "brcm,brcmstb-sun-top-ctrl", "simple-bus";
>>         reg = <0x8404000 0x708>;
>>
>>         reset: reset-controller@318 {
>>                 compatible = "brcm,brcmstb-reset";
>>                 reg = <0x318 0x30>;
>>                 #reset-cells = <1>;
>>         };
>> };
>>
>> Would that be what you expect to see?
> 
> The problem is with this alone, you should just move #reset-cells to
> the parent and remove the child node. That's all you really need from
> a DT perspective. But if this is really a separate block that's reused
> from chip to chip, then a separate node is fine. Typically in these
> situations, I just can't tell whether it's that or just the
> convenience of creating nodes for every kernel driver.

I found a couple of occurrences where the same HW block is used outside
of this sundry register block and also got confirmation from the
designers that the same block gets re-used from chip to chip, and
happens to get "wired" into the bus address decoding logic as part of
this sundry block for convenience and principle of least surprise.
-- 
Florian

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-01-11 18:52 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-21  1:34 [PATCH 0/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
2018-12-21  1:34 ` [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller Florian Fainelli
2018-12-31 23:13   ` Scott Branden
2019-01-02 20:38     ` Florian Fainelli
2019-01-03 17:41   ` Rob Herring
2019-01-03 18:53     ` Florian Fainelli
2019-01-03 19:19       ` Rob Herring
2019-01-03 19:31         ` Florian Fainelli
2019-01-03 22:54           ` Rob Herring
2019-01-11 18:51             ` Florian Fainelli
2018-12-21  1:34 ` [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
2019-01-02 10:44   ` Philipp Zabel
2019-01-02 20:38     ` Florian Fainelli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).