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
Add a binding document for the Broadcom STB reset controller, also known
as SW_INIT-style reset controller.
Signed-off-by: Florian Fainelli <[email protected]>
---
.../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>;
+ };
+
+ ðernet_switch {
+ resets = <&reset>;
+ reset-names = "switch";
+ };
--
2.17.1
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> .../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>;
> + };
> +
> + ðernet_switch {
> + resets = <&reset>;
> + reset-names = "switch";
> + };
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 <[email protected]>
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
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 <[email protected]>
>
> 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
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 <[email protected]>
>> ---
>> .../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
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
> >> ---
> >> .../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
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 <[email protected]>
>> ---
>> .../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
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 <[email protected]>
>>>> ---
>>>> .../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
On Thu, Jan 3, 2019 at 1:31 PM Florian Fainelli <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> .../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
On 1/3/19 2:54 PM, Rob Herring wrote:
> On Thu, Jan 3, 2019 at 1:31 PM Florian Fainelli <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> .../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