[PATCH 0/4] Add support for the Allwinner A31 Reset Controllers

[PATCH 0/4] Add support for the Allwinner A31 Reset Controllers

[Maxime Ripard]

Hi everyone,

This patchset adds support for the reset controllers found in the Allwinner A31
SoCs. Since these controllers are pretty simple, basically just a few MMIO
registers, with a single bit controlling the reset state of the other devices
it asserts in reset, the driver is quite simple as well.

However, one of the IPs asserted in reset by these controllers are the timers,
the only thing standing out is that it requires to be registered before the
timers are initialized.

Thanks,
Maxime

Maxime Ripard (4):
  reset: Add Allwinner A31 Reset Controller Driver
  ARM: sunxi: Select ARCH_HAS_RESET_CONTROLLER
  ARM: sunxi: Register the A31 reset IP in init_time
  ARM: sun6i: Add the reset controller to the DTSI

 arch/arm/boot/dts/sun6i-a31.dtsi |  24 +++++++++
 arch/arm/mach-sunxi/Kconfig      |   1 +
 arch/arm/mach-sunxi/sunxi.c      |  22 ++++++++-
 drivers/reset/Makefile           |   1 +
 drivers/reset/reset-sun6i.c      | 102 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 drivers/reset/reset-sun6i.c

-- 
1.8.4

[PATCH 1/4] reset: Add Allwinner A31 Reset Controller Driver

[Maxime Ripard]

The Allwinner A31 has an IP maintaining a few other IPs in the SoC in
reset by default. Among these IPs are the High Speed Timers, hence why
we can't use the regular driver construct, and need to call the
registering function directly during machine initialisation.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/reset/Makefile      |   1 +
 drivers/reset/reset-sun6i.c | 102 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)
 create mode 100644 drivers/reset/reset-sun6i.c

diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 1e2d83f..f216b74 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_RESET_CONTROLLER) += core.o
+obj-$(CONFIG_ARCH_SUNXI) += reset-sun6i.o
diff --git a/drivers/reset/reset-sun6i.c b/drivers/reset/reset-sun6i.c
new file mode 100644
index 0000000..e96db8d
--- /dev/null
+++ b/drivers/reset/reset-sun6i.c
@@ -0,0 +1,102 @@
+/*
+ * Allwinner A31 Reset Controller driver
+ *
+ * Copyright 2013 Maxime Ripard
+ * 
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+struct sun6i_reset_data {
+	void __iomem				*membase;
+	struct reset_controller_dev		rcdev;
+};
+
+static int sun6i_reset_assert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct sun6i_reset_data *data = container_of(rcdev,
+						     struct sun6i_reset_data,
+						     rcdev);
+	int bank = id / BITS_PER_LONG;
+	int offset = id % BITS_PER_LONG;
+	u32 reg = readl(data->membase + (bank * 4));
+
+	writel(reg & ~BIT(offset), data->membase + (bank * 4));
+
+	return 0;
+}
+
+static int sun6i_reset_deassert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct sun6i_reset_data *data = container_of(rcdev,
+						     struct sun6i_reset_data,
+						     rcdev);
+	int bank = id / BITS_PER_LONG;
+	int offset = id % BITS_PER_LONG;
+	u32 reg = readl(data->membase + (bank * 4));
+
+	writel(reg | BIT(offset), data->membase + (bank * 4));
+
+	return 0;
+}
+
+static struct reset_control_ops sun6i_reset_ops = {
+	.assert		= sun6i_reset_assert,
+	.deassert	= sun6i_reset_deassert,
+};
+
+int __init sun6i_reset_init(struct device_node *np) 
+{
+	struct sun6i_reset_data *data;
+	struct resource res;
+	resource_size_t size;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret)
+		goto err_alloc;
+
+	size = resource_size(&res);
+	if (!request_mem_region(res.start, size, np->name)) {
+		ret = -EBUSY;
+		goto err_alloc;
+	}
+
+	data->membase = ioremap(res.start, size);
+	if (!data->membase) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
+
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.nr_resets = size * 32;
+	data->rcdev.ops = &sun6i_reset_ops;
+	data->rcdev.of_node = np;
+	reset_controller_register(&data->rcdev);
+
+	return 0;
+
+err_alloc:
+	kfree(data);
+	return ret;
+}
-- 
1.8.4

[PATCH 2/4] ARM: sunxi: Select ARCH_HAS_RESET_CONTROLLER

[Maxime Ripard]

The A31 has a reset controller, and we have to select this option to
have access to the reset controller framework.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/mach-sunxi/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index 33cef54..98a1859 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -1,5 +1,6 @@
 config ARCH_SUNXI
 	bool "Allwinner A1X SOCs" if ARCH_MULTI_V7
+	select ARCH_HAS_RESET_CONTROLLER
 	select ARCH_REQUIRE_GPIOLIB
 	select CLKSRC_MMIO
 	select CLKSRC_OF
-- 
1.8.4

[PATCH 3/4] ARM: sunxi: Register the A31 reset IP in init_time

[Maxime Ripard]

The A31 has a reset IP that maintains a few other IPs in reset by
default. Among these IPs are the UARTs, and most notably the timers. We
thus need to register the reset driver before initializing the timers so
that the reset timer can use the reset framework.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/mach-sunxi/sunxi.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
index f184f6c..db4dff1 100644
--- a/arch/arm/mach-sunxi/sunxi.c
+++ b/arch/arm/mach-sunxi/sunxi.c
@@ -142,9 +142,29 @@ static const char * const sun6i_board_dt_compat[] = {
 	NULL,
 };
 
+static const struct of_device_id sun6i_reset_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-ahb1-reset", },
+	{ .compatible = "allwinner,sun6i-a31-apb1-reset", },
+	{ .compatible = "allwinner,sun6i-a31-apb2-reset", },
+	{ /* sentinel */ }
+};
+
+extern void __init sun6i_reset_init(struct device_node *np);
+static void __init sun6i_timer_init(void)
+{
+	struct device_node *np;
+
+	sunxi_init_clocks();
+
+	for_each_matching_node(np, sun6i_reset_dt_ids)
+		sun6i_reset_init(np);
+
+	clocksource_of_init();
+}
+
 DT_MACHINE_START(SUN6I_DT, "Allwinner sun6i (A31) Family")
 	.init_machine	= sunxi_dt_init,
-	.init_time	= sunxi_timer_init,
+	.init_time	= sun6i_timer_init,
 	.dt_compat	= sun6i_board_dt_compat,
 	.restart	= sun6i_restart,
 MACHINE_END
-- 
1.8.4

[PATCH 4/4] ARM: sun6i: Add the reset controller to the DTSI

[Maxime Ripard]

The A31 has a reset controller IP that maintains a few other IPs in
reset, among which we can find the UARTs, high speed timers or the I2C.
Now that we have support for them, add the reset controllers to the DTSI.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index f244f5f..c99c946 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -209,6 +209,24 @@
 			};
 		};
 
+		ahb1_rst: reset at 01c202c0 {
+			#reset-cells = <1>;
+			compatible = "allwinner,sun6i-a31-ahb1-reset";
+			reg = <0x01c202c0 0xc>;
+		};
+
+		apb1_rst: reset at 01c202d0 {
+			#reset-cells = <1>;
+			compatible = "allwinner,sun6i-a31-apb1-reset";
+			reg = <0x01c202d0 0x4>;
+		};
+
+		apb2_rst: reset at 01c202d8 {
+			#reset-cells = <1>;
+			compatible = "allwinner,sun6i-a31-apb2-reset";
+			reg = <0x01c202d8 0x4>;
+		};
+
 		timer at 01c20c00 {
 			compatible = "allwinner,sun4i-timer";
 			reg = <0x01c20c00 0xa0>;
@@ -232,6 +250,7 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clocks = <&apb2_gates 16>;
+			resets = <&apb2_rst 16>;
 			status = "disabled";
 		};
 
@@ -242,6 +261,7 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clocks = <&apb2_gates 17>;
+			resets = <&apb2_rst 17>;
 			status = "disabled";
 		};
 
@@ -252,6 +272,7 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clocks = <&apb2_gates 18>;
+			resets = <&apb2_rst 18>;
 			status = "disabled";
 		};
 
@@ -262,6 +283,7 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clocks = <&apb2_gates 19>;
+			resets = <&apb2_rst 19>;
 			status = "disabled";
 		};
 
@@ -272,6 +294,7 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clocks = <&apb2_gates 20>;
+			resets = <&apb2_rst 20>;
 			status = "disabled";
 		};
 
@@ -282,6 +305,7 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clocks = <&apb2_gates 21>;
+			resets = <&apb2rst 21>;
 			status = "disabled";
 		};
 
-- 
1.8.4

[PATCH 4/4] ARM: sun6i: Add the reset controller to the DTSI

[Emilio López]

Hi Maxime,

El 05/10/13 11:39, Maxime Ripard escribi?:
> The A31 has a reset controller IP that maintains a few other IPs in
> reset, among which we can find the UARTs, high speed timers or the I2C.
> Now that we have support for them, add the reset controllers to the DTSI.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>   arch/arm/boot/dts/sun6i-a31.dtsi | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index f244f5f..c99c946 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -209,6 +209,24 @@
>   			};
>   		};
>
> +		ahb1_rst: reset at 01c202c0 {
> +			#reset-cells = <1>;
> +			compatible = "allwinner,sun6i-a31-ahb1-reset";
> +			reg = <0x01c202c0 0xc>;
> +		};
> +
> +		apb1_rst: reset at 01c202d0 {
> +			#reset-cells = <1>;
> +			compatible = "allwinner,sun6i-a31-apb1-reset";
> +			reg = <0x01c202d0 0x4>;
> +		};
> +
> +		apb2_rst: reset at 01c202d8 {
> +			#reset-cells = <1>;
> +			compatible = "allwinner,sun6i-a31-apb2-reset";
> +			reg = <0x01c202d8 0x4>;
> +		};
> +
>   		timer at 01c20c00 {
>   			compatible = "allwinner,sun4i-timer";
>   			reg = <0x01c20c00 0xa0>;
> @@ -232,6 +250,7 @@
>   			reg-shift = <2>;
>   			reg-io-width = <4>;
>   			clocks = <&apb2_gates 16>;
> +			resets = <&apb2_rst 16>;
>   			status = "disabled";
>   		};
>
> @@ -242,6 +261,7 @@
>   			reg-shift = <2>;
>   			reg-io-width = <4>;
>   			clocks = <&apb2_gates 17>;
> +			resets = <&apb2_rst 17>;
>   			status = "disabled";
>   		};
>
> @@ -252,6 +272,7 @@
>   			reg-shift = <2>;
>   			reg-io-width = <4>;
>   			clocks = <&apb2_gates 18>;
> +			resets = <&apb2_rst 18>;
>   			status = "disabled";
>   		};
>
> @@ -262,6 +283,7 @@
>   			reg-shift = <2>;
>   			reg-io-width = <4>;
>   			clocks = <&apb2_gates 19>;
> +			resets = <&apb2_rst 19>;
>   			status = "disabled";
>   		};
>
> @@ -272,6 +294,7 @@
>   			reg-shift = <2>;
>   			reg-io-width = <4>;
>   			clocks = <&apb2_gates 20>;
> +			resets = <&apb2_rst 20>;
>   			status = "disabled";
>   		};
>
> @@ -282,6 +305,7 @@
>   			reg-shift = <2>;
>   			reg-io-width = <4>;
>   			clocks = <&apb2_gates 21>;
> +			resets = <&apb2rst 21>;

You have a typo here (missing _) which breaks the dt build

>   			status = "disabled";
>   		};
>
>

Cheers,

Emilio

[linux-sunxi] [PATCH 0/4] Add support for the Allwinner A31 Reset Controllers

[Arokux X]

Hi Maxime,

On Sat, Oct 5, 2013 at 4:39 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi everyone,
>
> This patchset adds support for the reset controllers found in the Allwinner A31
> SoCs. Since these controllers are pretty simple, basically just a few MMIO
> registers, with a single bit controlling the reset state of the other devices
> it asserts in reset, the driver is quite simple as well.

I think we need something smarter here. There are reset bits all over
the place. After a hint by Emilio and small chat with Oliver I've
realized I have 3 reset bits in USB host clock module [0].

Maybe implementation like this one [1] where a mask can be passed as a
parameter will be more appropriate? (Those reset bits behave the same
as gatable clocks really.)

Best,
Arokux

[0] https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/arch/arm/mach-sun4i/include/mach/ccmu_regs.h#L392
[1] https://github.com/arokux/linux/blob/sunxi-next-usb/drivers/clk/sunxi/clk-sunxi.c#L678

[linux-sunxi] [PATCH 0/4] Add support for the Allwinner A31 Reset Controllers

[Maxime Ripard]

Hi Roman,

On Sun, Oct 06, 2013 at 01:32:13AM +0200, Arokux X wrote:
> On Sat, Oct 5, 2013 at 4:39 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi everyone,
> >
> > This patchset adds support for the reset controllers found in the Allwinner A31
> > SoCs. Since these controllers are pretty simple, basically just a few MMIO
> > registers, with a single bit controlling the reset state of the other devices
> > it asserts in reset, the driver is quite simple as well.
> 
> I think we need something smarter here. There are reset bits all over
> the place. After a hint by Emilio and small chat with Oliver I've
> realized I have 3 reset bits in USB host clock module [0].

I wasn't aware there were other IPs behaving like this in older SoCs.
Thanks for pointing this out.

Something smarter in what sense? It's just one bit to put in one
register, I don't really see how it can be "smart".

> Maybe implementation like this one [1] where a mask can be passed as a
> parameter will be more appropriate? (Those reset bits behave the same
> as gatable clocks really.)

No, they don't behave like gatable clocks, and they shouldn't be
implemented with the clock framework. Whenever you disable a clock, the
child device will retain its configuration, while with the reset part,
well, it will just be reset. This makes one huge difference.

We have a shiny new reset framework for this, made exactly for these
cases, why not use it?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131006/a347db60/attachment.sig>

[linux-sunxi] [PATCH 0/4] Add support for the Allwinner A31 Reset Controllers

[Arokux X]

Hi Maxime,

On Sun, Oct 6, 2013 at 10:29 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Roman,
>
> On Sun, Oct 06, 2013 at 01:32:13AM +0200, Arokux X wrote:
>> On Sat, Oct 5, 2013 at 4:39 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > Hi everyone,
>> >
>> > This patchset adds support for the reset controllers found in the Allwinner A31
>> > SoCs. Since these controllers are pretty simple, basically just a few MMIO
>> > registers, with a single bit controlling the reset state of the other devices
>> > it asserts in reset, the driver is quite simple as well.
>>
>> I think we need something smarter here. There are reset bits all over
>> the place. After a hint by Emilio and small chat with Oliver I've
>> realized I have 3 reset bits in USB host clock module [0].
>
> I wasn't aware there were other IPs behaving like this in older SoCs.
> Thanks for pointing this out.
>
> Something smarter in what sense? It's just one bit to put in one
> register, I don't really see how it can be "smart".
By something smarter I meant something that prevents you from using
wrong bits for reset. For example in the USB clock module there are
only 3 reset bits in a register.

>
>> Maybe implementation like this one [1] where a mask can be passed as a
>> parameter will be more appropriate? (Those reset bits behave the same
>> as gatable clocks really.)
>
> No, they don't behave like gatable clocks, and they shouldn't be
> implemented with the clock framework. Whenever you disable a clock, the
> child device will retain its configuration, while with the reset part,
> well, it will just be reset. This makes one huge difference.

I'm sorry I wasn't clear. I did not meant to use clock framework for
reset bits. I was suggesting to implement your driver in a similar
way. You remember gatable clocks also take several bits per register.
We are managing them with one function which accepts a mask - ones at
the positions of the gatable clocks. Now after a closer look at reset
framework I see that nothing is registered, so the approach taken by
us with clocks is not possible indeed.

Let me now ask how I should go about my reset bits.

So far you were dealing with the registers where all the bits were
used for reset bits. In the case of USB clock module (and several
others) only several bits are reset bits. So I will proceed to use
your new driver like this (here, for the sake of clarity I use names
for registers and reset bits)

+               usb_rst: reset at USB_CLK_REG {
+                       #reset-cells = <1>;
+                       compatible = "allwinner,sun4i-a10-usb-reset";
+                       reg = <USB_CLK_REG 0x4>;
+               };

And somewhere in the EHCI/OHCI binding:

                 ohci0: .... {
                          ...
+                       resets = <&usb_rst USB0_RESET_BIT>;
                          ...
                }

                 ehci0: .... {
                          ...
+                       resets = <&usb_rst USB0_RESET_BIT>;
                          ...
                }

If I understand right, here is the problem with this approach. As you
see ohci0/ehci0 are sharing the very same bit. So if device_reset is
used in both ohci0/ehci0 kernel modules we will have a situation where
one kernel module will reset the hardware used by the other kernel
module. The problem is that reset bit really belongs to a USB PHY,
which is shared between ohci0/ehci0. So as it looks like I would need
to add another module that handles USB PHY?

                 usb0-phy: {
                          resets = <&usb_rst USB0_RESET_BIT>;
                 }

                 ohci0: .... {
                          ...
+                       phy = <&usb0-phy>
                          ...
                }

                 ehci0: .... {
                          ...
+                       phy = <&usb0-phy>
                          ...
                }

By the way, now you can see the advantages of the semantics that clock
framework is offering. If several devices want to disable a clock it
will only be disabled if no other devices are using it. Reset
framework does not offer this feature.

Best regards

[linux-sunxi] [PATCH 0/4] Add support for the Allwinner A31 Reset Controllers

[Maxime Ripard]

On Sun, Oct 06, 2013 at 11:42:10AM +0200, Arokux X wrote:
> Hi Maxime,
> 
> On Sun, Oct 6, 2013 at 10:29 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi Roman,
> >
> > On Sun, Oct 06, 2013 at 01:32:13AM +0200, Arokux X wrote:
> >> On Sat, Oct 5, 2013 at 4:39 PM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > Hi everyone,
> >> >
> >> > This patchset adds support for the reset controllers found in the Allwinner A31
> >> > SoCs. Since these controllers are pretty simple, basically just a few MMIO
> >> > registers, with a single bit controlling the reset state of the other devices
> >> > it asserts in reset, the driver is quite simple as well.
> >>
> >> I think we need something smarter here. There are reset bits all over
> >> the place. After a hint by Emilio and small chat with Oliver I've
> >> realized I have 3 reset bits in USB host clock module [0].
> >
> > I wasn't aware there were other IPs behaving like this in older SoCs.
> > Thanks for pointing this out.
> >
> > Something smarter in what sense? It's just one bit to put in one
> > register, I don't really see how it can be "smart".
> By something smarter I meant something that prevents you from using
> wrong bits for reset. For example in the USB clock module there are
> only 3 reset bits in a register.

Nothing will ever prevent you from poking in the wrong bits. If your
clock driver has a bug, or if the data coming from device tree are
bogus, it will happen, and there will be no safeguard.

> >> Maybe implementation like this one [1] where a mask can be passed as a
> >> parameter will be more appropriate? (Those reset bits behave the same
> >> as gatable clocks really.)
> >
> > No, they don't behave like gatable clocks, and they shouldn't be
> > implemented with the clock framework. Whenever you disable a clock, the
> > child device will retain its configuration, while with the reset part,
> > well, it will just be reset. This makes one huge difference.
> 
> I'm sorry I wasn't clear. I did not meant to use clock framework for
> reset bits. I was suggesting to implement your driver in a similar
> way. You remember gatable clocks also take several bits per register.
> We are managing them with one function which accepts a mask - ones at
> the positions of the gatable clocks. Now after a closer look at reset
> framework I see that nothing is registered, so the approach taken by
> us with clocks is not possible indeed.

We could do that by overriding the default of_xlate function. I'm not
exactly convinced it makes sense in that case, since bogus data will
possibly come from the DT, and in that case, you are screwed.

> Let me now ask how I should go about my reset bits.
> 
> So far you were dealing with the registers where all the bits were
> used for reset bits. In the case of USB clock module (and several
> others) only several bits are reset bits. So I will proceed to use
> your new driver like this (here, for the sake of clarity I use names
> for registers and reset bits)
> 
> +               usb_rst: reset at USB_CLK_REG {
> +                       #reset-cells = <1>;
> +                       compatible = "allwinner,sun4i-a10-usb-reset";
> +                       reg = <USB_CLK_REG 0x4>;
> +               };
> 
> And somewhere in the EHCI/OHCI binding:
> 
>                  ohci0: .... {
>                           ...
> +                       resets = <&usb_rst USB0_RESET_BIT>;
>                           ...
>                 }
> 
>                  ehci0: .... {
>                           ...
> +                       resets = <&usb_rst USB0_RESET_BIT>;
>                           ...
>                 }
> 
> If I understand right, here is the problem with this approach. As you
> see ohci0/ehci0 are sharing the very same bit. So if device_reset is
> used in both ohci0/ehci0 kernel modules we will have a situation where
> one kernel module will reset the hardware used by the other kernel
> module. The problem is that reset bit really belongs to a USB PHY,
> which is shared between ohci0/ehci0. So as it looks like I would need
> to add another module that handles USB PHY?
> 
>                  usb0-phy: {
>                           resets = <&usb_rst USB0_RESET_BIT>;
>                  }
> 
>                  ohci0: .... {
>                           ...
> +                       phy = <&usb0-phy>
>                           ...
>                 }
> 
>                  ehci0: .... {
>                           ...
> +                       phy = <&usb0-phy>
>                           ...
>                 }

Are those ohci and ehci nodes the same IP?

Anyway, it seems like it's the way to go, yes. There's a USB PHY
framework in review right now: https://lwn.net/Articles/548814/

> By the way, now you can see the advantages of the semantics that clock
> framework is offering. If several devices want to disable a clock it
> will only be disabled if no other devices are using it. Reset
> framework does not offer this feature.

Good thing we have the code and that we can patch it then. However, it's
not much of a problem in our case, since both drivers will only want to
deassert the reset.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131007/8d8bbf23/attachment.sig>

[linux-sunxi] [PATCH 0/4] Add support for the Allwinner A31 Reset Controllers

[Arokux X]

> We could do that by overriding the default of_xlate function. I'm not
> exactly convinced it makes sense in that case, since bogus data will
> possibly come from the DT, and in that case, you are screwed.

Yes, I agree it makes little sense to override of_xlate function.

>
>> Let me now ask how I should go about my reset bits.
>>
>> So far you were dealing with the registers where all the bits were
>> used for reset bits. In the case of USB clock module (and several
>> others) only several bits are reset bits. So I will proceed to use
>> your new driver like this (here, for the sake of clarity I use names
>> for registers and reset bits)
>>
>> +               usb_rst: reset at USB_CLK_REG {
>> +                       #reset-cells = <1>;
>> +                       compatible = "allwinner,sun4i-a10-usb-reset";
>> +                       reg = <USB_CLK_REG 0x4>;
>> +               };
>>
>> And somewhere in the EHCI/OHCI binding:
>>
>>                  ohci0: .... {
>>                           ...
>> +                       resets = <&usb_rst USB0_RESET_BIT>;
>>                           ...
>>                 }
>>
>>                  ehci0: .... {
>>                           ...
>> +                       resets = <&usb_rst USB0_RESET_BIT>;
>>                           ...
>>                 }
>>
>> If I understand right, here is the problem with this approach. As you
>> see ohci0/ehci0 are sharing the very same bit. So if device_reset is
>> used in both ohci0/ehci0 kernel modules we will have a situation where
>> one kernel module will reset the hardware used by the other kernel
>> module. The problem is that reset bit really belongs to a USB PHY,
>> which is shared between ohci0/ehci0. So as it looks like I would need
>> to add another module that handles USB PHY?
>>
>>                  usb0-phy: {
>>                           resets = <&usb_rst USB0_RESET_BIT>;
>>                  }
>>
>>                  ohci0: .... {
>>                           ...
>> +                       phy = <&usb0-phy>
>>                           ...
>>                 }
>>
>>                  ehci0: .... {
>>                           ...
>> +                       phy = <&usb0-phy>
>>                           ...
>>                 }
>
> Are those ohci and ehci nodes the same IP?
As far as I understand - yes.

> Anyway, it seems like it's the way to go, yes. There's a USB PHY
> framework in review right now: https://lwn.net/Articles/548814/

Thanks a lot for this pointer. This is actually merged already and is
currently in Greg's usb-next tree

https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=ff764963479a1b18721ab96e531404c50fefe8b1

So I'd need to use yet another framework in my miniature glue
driver... it actually only touches a bunch of registers. :)

>> By the way, now you can see the advantages of the semantics that clock
>> framework is offering. If several devices want to disable a clock it
>> will only be disabled if no other devices are using it. Reset
>> framework does not offer this feature.
>
> Good thing we have the code and that we can patch it then.
What do you mean?

> However, it's
> not much of a problem in our case, since both drivers will only want to
> deassert the reset.

It is a problem, since there will be two different modules one for
ohci and the other for ehci. So if you unload ohci for example it will
assert (write 0 to) the reset register. The ehci module will get into
trouble then.

Best regards

[linux-sunxi] [PATCH 0/4] Add support for the Allwinner A31 Reset Controllers

[Maxime Ripard]

On Mon, Oct 07, 2013 at 09:57:50PM +0200, Arokux X wrote:
> >> Let me now ask how I should go about my reset bits.
> >>
> >> So far you were dealing with the registers where all the bits were
> >> used for reset bits. In the case of USB clock module (and several
> >> others) only several bits are reset bits. So I will proceed to use
> >> your new driver like this (here, for the sake of clarity I use names
> >> for registers and reset bits)
> >>
> >> +               usb_rst: reset at USB_CLK_REG {
> >> +                       #reset-cells = <1>;
> >> +                       compatible = "allwinner,sun4i-a10-usb-reset";
> >> +                       reg = <USB_CLK_REG 0x4>;
> >> +               };
> >>
> >> And somewhere in the EHCI/OHCI binding:
> >>
> >>                  ohci0: .... {
> >>                           ...
> >> +                       resets = <&usb_rst USB0_RESET_BIT>;
> >>                           ...
> >>                 }
> >>
> >>                  ehci0: .... {
> >>                           ...
> >> +                       resets = <&usb_rst USB0_RESET_BIT>;
> >>                           ...
> >>                 }
> >>
> >> If I understand right, here is the problem with this approach. As you
> >> see ohci0/ehci0 are sharing the very same bit. So if device_reset is
> >> used in both ohci0/ehci0 kernel modules we will have a situation where
> >> one kernel module will reset the hardware used by the other kernel
> >> module. The problem is that reset bit really belongs to a USB PHY,
> >> which is shared between ohci0/ehci0. So as it looks like I would need
> >> to add another module that handles USB PHY?
> >>
> >>                  usb0-phy: {
> >>                           resets = <&usb_rst USB0_RESET_BIT>;
> >>                  }
> >>
> >>                  ohci0: .... {
> >>                           ...
> >> +                       phy = <&usb0-phy>
> >>                           ...
> >>                 }
> >>
> >>                  ehci0: .... {
> >>                           ...
> >> +                       phy = <&usb0-phy>
> >>                           ...
> >>                 }
> >
> > Are those ohci and ehci nodes the same IP?
> As far as I understand - yes.

Then, it should be the same DT node. And you get only one driver, that
can handle both the ehci and ohci cases, and problem solved.

> > Anyway, it seems like it's the way to go, yes. There's a USB PHY
> > framework in review right now: https://lwn.net/Articles/548814/
> 
> Thanks a lot for this pointer. This is actually merged already and is
> currently in Greg's usb-next tree
> 
> https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=ff764963479a1b18721ab96e531404c50fefe8b1

Oh, ok :)

> So I'd need to use yet another framework in my miniature glue
> driver... it actually only touches a bunch of registers. :)
> 
> >> By the way, now you can see the advantages of the semantics that clock
> >> framework is offering. If several devices want to disable a clock it
> >> will only be disabled if no other devices are using it. Reset
> >> framework does not offer this feature.
> >
> > Good thing we have the code and that we can patch it then.
> What do you mean?

If that behaviour doesn't fit your needs, change the behaviour, and send
the patches. It's the way things go here.

> > However, it's
> > not much of a problem in our case, since both drivers will only want to
> > deassert the reset.
> 
> It is a problem, since there will be two different modules one for
> ohci and the other for ehci. So if you unload ohci for example it will
> assert (write 0 to) the reset register. The ehci module will get into
> trouble then.

And if no one put the device back into reset, it just works.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131007/7091aede/attachment-0001.sig>