Skip to contentSkip to conversation

Amlogic: Meson: Add reset controller

PATCH Superseded Neil Armstrong 4 participants 12 messages May 25, 2016 – May 30, 2016

Subsystems (3)
Device tree bindings Arm/Amlogic Meson SoC support Reset controller framework

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Neil Armstrong

author
#0 lore raw

v2 cover Amlogic: Meson: Add reset controller

Patchset to add and enable the reset controller driver on Meson SoCs platforms.

This reset controller has up to 256 reset lines with reset pulse generation only, so the assert and deassert calls are not available.

Depends on the patch :
- "reset: add devm_reset_controller_register API" [1]

Changes since v1 at http://lkml.kernel.org/r/1463732875-23141-1-git-send-email-narmstrong at baylibre.com :
- Remove _gxbb_ in names
- Depends build on ARCH_MESON only
- Add meson8b compatible and bindings
- Re-indent dt-bindings headers
- Switch to devm_ reset controller register
- Remove platform "remove" callback
- Update Dual BSD/GPL file headers

Changes since the RFC at http://lkml.kernel.org/r/1463148012-25988-1-git-send-email-narmstrong at baylibre.com :
- Fix register mapping and bit defines in bindings header
- Remove assert and unassert calls
- Fix missing __iomem

[1] https://patchwork.kernel.org/patch/8988471/

Neil Armstrong (4):
reset: Add support for the Amlogic Meson SoC Reset Controller
dt-bindings: reset: Add bindings for the Meson SoC Reset Controller
ARM64: dts: amlogic: Enable Reset Controller on GXBB-based platforms
ARM: dts: amlogic: Enable Reset Controller on Meson8b platforms

 .../bindings/reset/amlogic,meson-reset.txt         |  18 ++
 arch/arm/boot/dts/meson8b.dtsi                     |   7 +
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |   7 +
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-meson.c                        | 137 ++++++++++++++
 .../dt-bindings/reset/amlogic,meson-gxbb-reset.h   | 210 +++++++++++++++++++++
 include/dt-bindings/reset/amlogic,meson8b-reset.h  | 175 +++++++++++++++++
 7 files changed, 555 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/amlogic,meson-reset.txt
 create mode 100644 drivers/reset/reset-meson.c
 create mode 100644 include/dt-bindings/reset/amlogic,meson-gxbb-reset.h
 create mode 100644 include/dt-bindings/reset/amlogic,meson8b-reset.h
Signature · 1 line
2.7.0
author
#1 lore raw

PATCH v2 1/4 reset: Add support for the Amlogic Meson SoC Reset Controller

This patch adds the platform driver for the Amlogic Meson SoC Reset Controller.

The Meson8b and GXBB SoCs are supported.

Signed-off-by Neil Armstrong <[email protected]>
---
 drivers/reset/Makefile      |   1 +
 drivers/reset/reset-meson.c | 137 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 138 insertions(+)
 create mode 100644 drivers/reset/reset-meson.c
drivers/reset/Makefile +1 -0
index f173fc3..03dc1bb 100644
@@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
 obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
 obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
 obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
+obj-$(CONFIG_ARCH_MESON) += reset-meson.o
 obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_ARCH_STI) += sti/
 obj-$(CONFIG_ARCH_HISI) += hisilicon/
drivers/reset/reset-meson.c new file +137 -0

137 lines not on this page · raw message

Signature · 1 line
2.7.0
maintainer
#2 lore raw

Re: [PATCH v2 1/4] reset: Add support for the Amlogic Meson SoC Reset Controller

Hi Neil,

looks fine, just two comments below.

Am Mittwoch, den 25.05.2016, 11:49 +0200 schrieb Neil Armstrong:

Quoted 12 lines

This patch adds the platform driver for the Amlogic Meson SoC Reset Controller.

The Meson8b and GXBB SoCs are supported.

Signed-off-by Neil Armstrong <[email protected]>
---
 drivers/reset/Makefile      |   1 +
 drivers/reset/reset-meson.c | 137 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 138 insertions(+)
 create mode 100644 drivers/reset/reset-meson.c

[...]

Quoted 5 lines
drivers/reset/reset-meson.c new file +0 -0
new file mode 100644
index 0000000..98087ae

[...]

drivers/reset/reset-meson.c line 79

Quoted 8 lines
drivers/reset/reset-meson.c new file +8 -0
+static int meson_reset_reset(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct meson_reset *data =
+		container_of(rcdev, struct meson_reset, rcdev);
+	unsigned int bank = id / BITS_PER_REG;
+	unsigned int offset = id % BITS_PER_REG;
+	void *reg_addr = data->reg_base + (bank << 2);

We lost the __iomem here.

Quoted 19 lines
drivers/reset/reset-meson.c new file +19 -0
+
+	if (bank >= REG_COUNT)
+		return -EINVAL;
+
+	writel(BIT(offset), reg_addr);
+
+	return 0;
+}
+
+static const struct reset_control_ops meson_reset_ops = {
+	.reset		= meson_reset_reset,
+};
+
+static const struct of_device_id meson_reset_dt_ids[] = {
+	 { .compatible = "amlogic,meson8b-reset", },
+	 { .compatible = "amlogic,meson-gxbb-reset", },
+	 { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);

[...]

Quoted 1 line

+MODULE_ALIAS("platform:meson_reset");

And I think you can drop the MODULE_ALIAS since of: modaliases will be generated from the MODULE_DEVICE_TABLE above. This driver is only ever going to be probed from device tree, isn't it?

drivers/reset/reset-meson.c line 137

Quoted 3 lines
drivers/reset/reset-meson.c new file +3 -0
+MODULE_AUTHOR("Neil Armstrong <[email protected]>");
+MODULE_DESCRIPTION("Amlogic Meson Reset Controller driver");
+MODULE_LICENSE("Dual BSD/GPL");

regards
Philipp

maintainer
#3 lore raw

PATCH v2 1/4 reset: Add support for the Amlogic Meson SoC Reset Controller

In reply to Neil Armstrong
Quoted 6 lines · Neil Armstrong

This patch adds the platform driver for the Amlogic Meson SoC Reset Controller.

The Meson8b and GXBB SoCs are supported.

Signed-off-by Neil Armstrong <[email protected]>

Maybe a question for Philipp, but when testog this version with the stmmac ethernet driver on the Amlogic boards, I noticed that ->reset was never getting called.

Turns out, the stmmac driver only uses reset_control_assert() and reset_control_deassert(), both of which return -ENOTSUPP since this driver doesn't provide ->assert or ->deassert, so the driver's ->reset() never gets called.

I haven't looked into the reset framework in detail, but if there's only a ->reset hook, I kind of expected that reset_control_deassert() would use that instead of returning -ENOTSUPP.

If not, what's the proper way of handling hardware that only supports a write-only reset pulse? Should users of reset_control_* be adapted to check if ->deassert returns -ENOTSUPP and call ->reset?

Kevin

maintainer
#4 lore raw

Re: [PATCH v2 1/4] reset: Add support for the Amlogic Meson SoC Reset Controller

Hi Kevin,

Am Mittwoch, den 25.05.2016, 19:42 -0700 schrieb Kevin Hilman:

Quoted 18 lines
Quoted 6 lines · Neil Armstrong

This patch adds the platform driver for the Amlogic Meson SoC Reset Controller.

The Meson8b and GXBB SoCs are supported.

Signed-off-by Neil Armstrong <[email protected]>

Maybe a question for Philipp, but when testog this version with the stmmac ethernet driver on the Amlogic boards, I noticed that ->reset was never getting called.

Turns out, the stmmac driver only uses reset_control_assert() and reset_control_deassert(), both of which return -ENOTSUPP since this driver doesn't provide ->assert or ->deassert, so the driver's ->reset() never gets called.

Quoted 7 lines

I haven't looked into the reset framework in detail, but if there's only a ->reset hook, I kind of expected that reset_control_deassert() would use that instead of returning -ENOTSUPP.

If not, what's the proper way of handling hardware that only supports a write-only reset pulse? Should users of reset_control_* be adapted to check if ->deassert returns -ENOTSUPP and call ->reset?

I initially came up with this for i.MX6, which has a reset controller that just like the Meson ones doesn't support manual assertion and deassertion of the reset line.
My assumption then was that reset() is the default for all devices that just need their internal state to be reset at some point. I didn't expect the clock-like usage of manual deassert()/assert() for power management to become so common.
So assert() and deassert() return -ENOTSUPP if the reset controller doesn't support manually asserting or deasserting the reset line.

Of course you can argue that after a reset() pulse the reset line is deasserted, and if you instead interpret the API in terms of expected outcome, that would be similar to the state after call to deassert() (iff the line was asserted before).
That would blur the line between reset() and deassert() somewhat, and in my opinion having a call to deassert() first doing the exact opposite isn't good nomenclature.
Personally, I'd prefer if the driver could switch to only using

	reset_control_reset(rstc);
or, if the reset line needs to stay asserted during powerdown where the
reset controller supports it, but doesn't matter whert not, use:
	ret = reset_control_deassert(rstc);
	if (ret == -ENOTSUPP)
		ret = reset_control_reset(rstc);

We could add a helper reset_control_deassert_or_reset() for that.

regards
Philipp

maintainer
#5 lore raw

Re: [PATCH v2 1/4] reset: Add support for the Amlogic Meson SoC Reset Controller

Quoted 45 lines · Philipp Zabel

Am Mittwoch, den 25.05.2016, 19:42 -0700 schrieb Kevin Hilman:

Quoted 18 lines
Quoted 6 lines · Neil Armstrong

This patch adds the platform driver for the Amlogic Meson SoC Reset Controller.

The Meson8b and GXBB SoCs are supported.

Signed-off-by Neil Armstrong <[email protected]>

Maybe a question for Philipp, but when testog this version with the stmmac ethernet driver on the Amlogic boards, I noticed that ->reset was never getting called.

Turns out, the stmmac driver only uses reset_control_assert() and reset_control_deassert(), both of which return -ENOTSUPP since this driver doesn't provide ->assert or ->deassert, so the driver's ->reset() never gets called.

Quoted 7 lines

I haven't looked into the reset framework in detail, but if there's only a ->reset hook, I kind of expected that reset_control_deassert() would use that instead of returning -ENOTSUPP.

If not, what's the proper way of handling hardware that only supports a write-only reset pulse? Should users of reset_control_* be adapted to check if ->deassert returns -ENOTSUPP and call ->reset?

I initially came up with this for i.MX6, which has a reset controller that just like the Meson ones doesn't support manual assertion and deassertion of the reset line.
My assumption then was that reset() is the default for all devices that just need their internal state to be reset at some point. I didn't expect the clock-like usage of manual deassert()/assert() for power management to become so common.
So assert() and deassert() return -ENOTSUPP if the reset controller doesn't support manually asserting or deasserting the reset line.

Of course you can argue that after a reset() pulse the reset line is deasserted, and if you instead interpret the API in terms of expected outcome, that would be similar to the state after call to deassert() (iff the line was asserted before).
That would blur the line between reset() and deassert() somewhat, and in my opinion having a call to deassert() first doing the exact opposite isn't good nomenclature.
Personally, I'd prefer if the driver could switch to only using

Quoted 1 line
	reset_control_reset(rstc);

Then what would happen if that same driver is used on platforms that have ->assert and ->deassert but no -reset?

The bind I'm in is that I'm using an exsting network driver (stmicro/stmmac) which is used on a bunch of platforms and I dont' know what all those reset controllers are actually doing, so going and changing the driver seems problematic.

Quoted 7 lines

or, if the reset line needs to stay asserted during powerdown where the reset controller supports it, but doesn't matter whert not, use:

	ret = reset_control_deassert(rstc);
	if (ret == -ENOTSUPP)
		ret = reset_control_reset(rstc);

We could add a helper reset_control_deassert_or_reset() for that.

IMO, that would be a useful helper function, but I still think that should be the default behavior of _deassert, otherwise it will still require changing all the drivers.

Kevin

maintainer
#6 lore raw

Re: [PATCH v2 1/4] reset: Add support for the Amlogic Meson SoC Reset Controller

Hi Kevin,

Am Donnerstag, den 26.05.2016, 11:42 -0700 schrieb Kevin Hilman:
[...]

Quoted 5 lines
Quoted 2 lines

Personally, I'd prefer if the driver could switch to only using

	reset_control_reset(rstc);

Then what would happen if that same driver is used on platforms that have ->assert and ->deassert but no -reset?

If the implementer of the reset controller driver doesn't have enough information about all connected devices to implement ->reset, then _reset() has to return -ENOTSUPP, so this is not an option in the general case.

Quoted 4 lines

The bind I'm in is that I'm using an exsting network driver (stmicro/stmmac) which is used on a bunch of platforms and I dont' know what all those reset controllers are actually doing, so going and changing the driver seems problematic.

I see. It's all the more important that the API clearly states the intended effects.

Quoted 11 lines
Quoted 7 lines

or, if the reset line needs to stay asserted during powerdown where the reset controller supports it, but doesn't matter whert not, use:

	ret = reset_control_deassert(rstc);
	if (ret == -ENOTSUPP)
		ret = reset_control_reset(rstc);

We could add a helper reset_control_deassert_or_reset() for that.

IMO, that would be a useful helper function, but I still think that should be the default behavior of _deassert, otherwise it will still require changing all the drivers.

"All the drivers" makes me reconsider. If all drivers eventually, reasonably could be changed to use reset_control_deassert_or_reset() instead of reset_control_deassert(), there would be no reason not to make _deassert() behave like you expect (well, except for the naming issue).

One thing that has to be considered is that in the case of synchronous resets there may be a significant difference in the behaviour of ->reset and ->deassert: With ->deassert you could prime the reset line before enabling clocks and letting the reset signal propagate. This is not possible with ->reset, which could hang or have no effect at all with the relevant clocks disabled. So it should be clearly documented that anybody intending to make use of this must call reset_control_assert() first and back off if that returns -ENOTSUPP.

regards
Philipp

author
#7 lore raw

PATCH v2 2/4 dt-bindings: reset: Add bindings for the Meson SoC Reset Controller

In reply to Neil Armstrong

Add DT bindings for the Meson SoC Reset Controller documentation and the associated include file.

Signed-off-by Neil Armstrong <[email protected]>
---
 .../bindings/reset/amlogic,meson-reset.txt         |  18 ++
 .../dt-bindings/reset/amlogic,meson-gxbb-reset.h   | 210 +++++++++++++++++++++
 include/dt-bindings/reset/amlogic,meson8b-reset.h  | 175 +++++++++++++++++
 3 files changed, 403 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/amlogic,meson-reset.txt
 create mode 100644 include/dt-bindings/reset/amlogic,meson-gxbb-reset.h
 create mode 100644 include/dt-bindings/reset/amlogic,meson8b-reset.h
Documentation/devicetree/bindings/reset/amlogic,meson-reset.txt new file +18 -0
new file mode 100644
index 0000000..e746b63
@@ -0,0 +1,18 @@
+Amlogic Meson SoC Reset Controller
+=======================================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+Required properties:
+- compatible: Should be "amlogic,meson8b-reset" or "amlogic,meson-gxbb-reset"
+- reg: should contain the register address base
+- #reset-cells: 1, see below
+
+example:
+
+reset: reset-controller {
+	compatible = "amlogic,meson-gxbb-reset";
+	reg = <0x0 0x04404 0x0 0x20>;
+	#reset-cells = <1>;
+};
include/dt-bindings/reset/amlogic,meson-gxbb-reset.h new file +210 -0

210 lines not on this page · raw message

include/dt-bindings/reset/amlogic,meson8b-reset.h new file +175 -0

175 lines not on this page · raw message

Signature · 1 line
2.7.0
maintainer
#8 lore raw

Re: [PATCH v2 2/4] dt-bindings: reset: Add bindings for the Meson SoC Reset Controller

Quoted 4 lines · Neil Armstrong

Add DT bindings for the Meson SoC Reset Controller documentation and the associated include file.

Signed-off-by Neil Armstrong <[email protected]>
Quoted 8 lines
---
 .../bindings/reset/amlogic,meson-reset.txt         |  18 ++
 .../dt-bindings/reset/amlogic,meson-gxbb-reset.h   | 210 +++++++++++++++++++++
 include/dt-bindings/reset/amlogic,meson8b-reset.h  | 175 +++++++++++++++++
 3 files changed, 403 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/amlogic,meson-reset.txt
 create mode 100644 include/dt-bindings/reset/amlogic,meson-gxbb-reset.h
 create mode 100644 include/dt-bindings/reset/amlogic,meson8b-reset.h

Please add acks when posting new versions.

Rob

author
#9 lore raw

Re: [PATCH v2 2/4] dt-bindings: reset: Add bindings for the Meson SoC Reset Controller

Quoted 14 lines · Rob Herring
Quoted 4 lines · Neil Armstrong

Add DT bindings for the Meson SoC Reset Controller documentation and the associated include file.

Signed-off-by Neil Armstrong <[email protected]>
Quoted 8 lines
---
 .../bindings/reset/amlogic,meson-reset.txt         |  18 ++
 .../dt-bindings/reset/amlogic,meson-gxbb-reset.h   | 210 +++++++++++++++++++++
 include/dt-bindings/reset/amlogic,meson8b-reset.h  | 175 +++++++++++++++++
 3 files changed, 403 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/amlogic,meson-reset.txt
 create mode 100644 include/dt-bindings/reset/amlogic,meson-gxbb-reset.h
 create mode 100644 include/dt-bindings/reset/amlogic,meson8b-reset.h
Quoted 4 lines

Please add acks when posting new versions.

Rob

Hi Rob,

Actually I made a lot of changes here since I added support for the meson8b. I asked myself if I could carry your ack actually...

If the ack is still valid, I'll won't forget it for v4.

Thanks,
Neil

author
#10 lore raw

PATCH v2 3/4 ARM64: dts: amlogic: Enable Reset Controller on GXBB-based platforms

In reply to Neil Armstrong

Update DTSI file to add the reset controller node.

Signed-off-by Neil Armstrong <[email protected]>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)
arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +7 -0
index 832815d..45e9c55 100644
@@ -43,6 +43,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/reset/amlogic,meson-gxbb-reset.h>
 
 / {
 	compatible = "amlogic,meson-gxbb";

@@ -129,6 +130,12 @@
 			#size-cells = <2>;
 			ranges = <0x0 0x0 0x0 0xc1100000 0x0 0x100000>;
 
+			reset: reset-controller at 4404 {
+				compatible = "amlogic,meson-gxbb-reset";
+				reg = <0x0 0x04404 0x0 0x20>;
+				#reset-cells = <1>;
+			};
+
 			uart_A: serial at 84c0 {
 				compatible = "amlogic,meson-uart";
 				reg = <0x0 0x084c0 0x0 0x14>;
Signature · 1 line
2.7.0
author
#11 lore raw

PATCH v2 4/4 ARM: dts: amlogic: Enable Reset Controller on Meson8b platforms

In reply to Neil Armstrong

Update DTSI file to add the reset controller node.

Signed-off-by Neil Armstrong <[email protected]>
---
 arch/arm/boot/dts/meson8b.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/boot/dts/meson8b.dtsi +7 -0
index 2bfe401..fc4080d 100644
@@ -46,6 +46,7 @@
 
 #include <dt-bindings/clock/meson8b-clkc.h>
 #include <dt-bindings/gpio/meson8b-gpio.h>
+#include <dt-bindings/reset/amlogic,meson8b-reset.h>
 #include "skeleton.dtsi"
 
 / {

@@ -105,6 +106,12 @@
 			#interrupt-cells = <3>;
 		};
 
+		reset: reset-controller at c1104404 {
+			compatible = "amlogic,meson8b-reset";
+			reg = <0xc1104404 0x20>;
+			#reset-cells = <1>;
+		};
+
 		wdt: watchdog at c1109900 {
 			compatible = "amlogic,meson8b-wdt";
 			reg = <0xc1109900 0x8>;
Signature · 1 line
2.7.0