Skip to contentSkip to conversation

clk: sunxi: Add support for USB clocks and reset bits

PATCH Superseded Hans de Goede 3 participants 17 messages Jan 22, 2014 – Feb 7, 2014

Subsystems (4)
Device tree bindings Common clk Arm/Allwinner sunXi SoC support Arm/Allwinner SoC Clock Support

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Hans de Goede

author
#0 lore raw

v2 cover clk: sunxi: Add support for USB clocks and reset bits

Hi Emilio, Maxime, et al,

Emilio, here is v2 of my patch-set adding support for sunxi-clk USB clocks and reset bits. This addresses all your review comments from v1.

Can you add the first 2 patches to your queue of patches for Mike for 3.15 ?

Maxime, can you add patch 3-5 which add the dt bindings for this to your tree please ?

Thanks & Regards,

Hans

author
#1 lore raw

PATCH v2 1/5 clk: sunxi: Add support for USB clock-register reset bits

The usb-clk register is special in that it not only contains clk gate bits, but also has a few reset bits. This commit adds support for this by allowing gates type sunxi clks to also register a reset controller.

Signed-off-by Hans de Goede <[email protected]>
---
 drivers/clk/sunxi/clk-sunxi.c | 71 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)
drivers/clk/sunxi/clk-sunxi.c +71 -0
index 03bb8b8..f1a147c 100644
@@ -18,6 +18,7 @@
 #include <linux/clkdev.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/reset-controller.h>
 
 #include "clk-factors.h"
 

@@ -741,6 +742,59 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
 
 
 /**
+ * sunxi_gates_reset... - reset bits in leaf gate clk registers handling
+ */
+
+struct gates_reset_data {
+	void __iomem			*reg;
+	spinlock_t			*lock;
+	struct reset_controller_dev	rcdev;
+};
+
+static int sunxi_gates_reset_assert(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct gates_reset_data *data = container_of(rcdev,
+						     struct gates_reset_data,
+						     rcdev);
+	unsigned long flags;
+	u32 reg;
+
+	spin_lock_irqsave(data->lock, flags);
+
+	reg = readl(data->reg);
+	writel(reg & ~BIT(id), data->reg);
+
+	spin_unlock_irqrestore(data->lock, flags);
+
+	return 0;
+}
+
+static int sunxi_gates_reset_deassert(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	struct gates_reset_data *data = container_of(rcdev,
+						     struct gates_reset_data,
+						     rcdev);
+	unsigned long flags;
+	u32 reg;
+
+	spin_lock_irqsave(data->lock, flags);
+
+	reg = readl(data->reg);
+	writel(reg | BIT(id), data->reg);
+
+	spin_unlock_irqrestore(data->lock, flags);
+
+	return 0;
+}
+
+static struct reset_control_ops sunxi_gates_reset_ops = {
+	.assert		= sunxi_gates_reset_assert,
+	.deassert	= sunxi_gates_reset_deassert,
+};
+
+/**
  * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks
  */
 

@@ -748,6 +802,7 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
 
 struct gates_data {
 	DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE);
+	u32 reset_mask;
 };
 
 static const struct gates_data sun4i_axi_gates_data __initconst = {

@@ -818,6 +873,7 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
 					 struct gates_data *data)
 {
 	struct clk_onecell_data *clk_data;
+	struct gates_reset_data *reset_data;
 	const char *clk_parent;
 	const char *clk_name;
 	void *reg;

@@ -861,6 +917,21 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
 	clk_data->clk_num = i;
 
 	of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+
+	/* Register a reset controler for gates with reset bits */
+	if (data->reset_mask == 0)
+		return;
+
+	reset_data = kzalloc(sizeof(*reset_data), GFP_KERNEL);
+	if (!reset_data)
+		return;
+
+	reset_data->reg = reg;
+	reset_data->lock = &clk_lock;
+	reset_data->rcdev.nr_resets = hweight32(data->reset_mask);
+	reset_data->rcdev.ops = &sunxi_gates_reset_ops;
+	reset_data->rcdev.of_node = node;
+	reset_controller_register(&reset_data->rcdev);
 }
Signature · 1 line
1.8.5.3
maintainer
#2 lore raw

PATCH v2 1/5 clk: sunxi: Add support for USB clock-register reset bits

Hi Hans,

El 22/01/14 18:36, Hans de Goede escribi?:

Quoted 6 lines

The usb-clk register is special in that it not only contains clk gate bits, but also has a few reset bits. This commit adds support for this by allowing gates type sunxi clks to also register a reset controller.

Signed-off-by Hans de Goede <[email protected]>

---

(snip)

Quoted 15 lines
  static const struct gates_data sun4i_axi_gates_data __initconst = {
drivers/clk/sunxi/clk-sunxi.c +3 -0
@@ -818,6 +873,7 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
  					 struct gates_data *data)
  {
  	struct clk_onecell_data *clk_data;
+	struct gates_reset_data *reset_data;
  	const char *clk_parent;
  	const char *clk_name;
  	void *reg;

@@ -861,6 +917,21 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
  	clk_data->clk_num = i;
 
  	of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+
+	/* Register a reset controler for gates with reset bits */

drivers/clk/sunxi/clk-sunxi.c static void __init sunxi_gates_clk_setup(struct device_node *node, · line 931

Quoted 10 lines
drivers/clk/sunxi/clk-sunxi.c +10 -0
+	if (data->reset_mask == 0)
+		return;
+
+	reset_data = kzalloc(sizeof(*reset_data), GFP_KERNEL);
+	if (!reset_data)
+		return;
+
+	reset_data->reg = reg;
+	reset_data->lock = &clk_lock;
+	reset_data->rcdev.nr_resets = hweight32(data->reset_mask);

I know I made you change this, but after having a second look into nr_resets, I think your original implementation makes more sense. This will break if you use a mask with holes on it. Sorry :(

drivers/clk/sunxi/clk-sunxi.c static void __init sunxi_gates_clk_setup(struct device_node *node, · line 935

Quoted 4 lines
drivers/clk/sunxi/clk-sunxi.c +3 -0
+	reset_data->rcdev.ops = &sunxi_gates_reset_ops;
+	reset_data->rcdev.of_node = node;
+	reset_controller_register(&reset_data->rcdev);
  }

Emilio

author
#3 lore raw

PATCH v2 1/5 clk: sunxi: Add support for USB clock-register reset bits

Hi,

Quoted 22 lines · Emilio L?pez

Hi Hans,

El 22/01/14 18:36, Hans de Goede escribi?:

Quoted 6 lines

The usb-clk register is special in that it not only contains clk gate bits, but also has a few reset bits. This commit adds support for this by allowing gates type sunxi clks to also register a reset controller.

Signed-off-by Hans de Goede <[email protected]>

---

(snip)

Quoted 12 lines
  static const struct gates_data sun4i_axi_gates_data __initconst = {
drivers/clk/sunxi/clk-sunxi.c +1 -0
@@ -818,6 +873,7 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
                       struct gates_data *data)
  {
      struct clk_onecell_data *clk_data;
+    struct gates_reset_data *reset_data;
      const char *clk_parent;
      const char *clk_name;
      void *reg;

@@ -861,6 +917,21 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
      clk_data->clk_num = i;
Quoted 15 lines

drivers/clk/sunxi/clk-sunxi.c static void __init sunxi_gates_clk_setup(struct device_node *node, · line 931

Quoted 13 lines
drivers/clk/sunxi/clk-sunxi.c +12 -0
      of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+
+    /* Register a reset controler for gates with reset bits */
+    if (data->reset_mask == 0)
+        return;
+
+    reset_data = kzalloc(sizeof(*reset_data), GFP_KERNEL);
+    if (!reset_data)
+        return;
+
+    reset_data->reg = reg;
+    reset_data->lock = &clk_lock;
+    reset_data->rcdev.nr_resets = hweight32(data->reset_mask);

I know I made you change this, but after having a second look into nr_resets, I think your original implementation makes more sense. This will break if you use a mask with holes on it. Sorry :(

No problem, just changed it back :)

Regards,

Hans

author
#4 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

In reply to Hans de Goede

From: Roman Byshko <[email protected]>

Add register definitions for the usb-clk register found on sun4i, sun5i and sun7i SoCs.

Signed-off-by Roman Byshko <[email protected]>
Signed-off-by Hans de Goede <[email protected]>
---
 Documentation/devicetree/bindings/clock/sunxi.txt |  5 +++++
 drivers/clk/sunxi/clk-sunxi.c                     | 12 ++++++++++++
 2 files changed, 17 insertions(+)
Documentation/devicetree/bindings/clock/sunxi.txt +5 -0
index 79c7197..8bccb6a 100644
@@ -37,6 +37,8 @@ Required properties:
 	"allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
 	"allwinner,sun4i-mod0-clk" - for the module 0 family of clocks
 	"allwinner,sun7i-a20-out-clk" - for the external output clocks
+	"allwinner,sun4i-usb-gates-clk" - for usb gates + resets on A10 / A20
+	"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

@@ -49,6 +51,9 @@ Required properties for all clocks:
 Additionally, "allwinner,*-gates-clk" clocks require:
 - clock-output-names : the corresponding gate names that the clock controls
 
+And "allwinner,*-usb-gates-clk" clocks also require:
+- reset-cells : shall be set to 1
+
 Clock consumers should specify the desired clocks they use with a
 "clocks" phandle cell. Consumers that are using a gated clock should
 provide an additional ID in their clock property. This ID is the
drivers/clk/sunxi/clk-sunxi.c +12 -0
index f1a147c..18cbc3c 100644
@@ -813,6 +813,16 @@ static const struct gates_data sun4i_ahb_gates_data __initconst = {
 	.mask = {0x7F77FFF, 0x14FB3F},
 };
 
+static const struct gates_data sun4i_usb_gates_data __initconst = {
+	.mask = {0x1C0},
+	.reset_mask = 0x07,
+};
+
+static const struct gates_data sun5i_a13_usb_gates_data __initconst = {
+	.mask = {0x140},
+	.reset_mask = 0x03,
+};
+
 static const struct gates_data sun5i_a10s_ahb_gates_data __initconst = {
 	.mask = {0x147667e7, 0x185915},
 };

@@ -1159,6 +1169,8 @@ static const struct of_device_id clk_gates_match[] __initconst = {
 	{.compatible = "allwinner,sun6i-a31-apb1-gates-clk", .data = &sun6i_a31_apb1_gates_data,},
 	{.compatible = "allwinner,sun7i-a20-apb1-gates-clk", .data = &sun7i_a20_apb1_gates_data,},
 	{.compatible = "allwinner,sun6i-a31-apb2-gates-clk", .data = &sun6i_a31_apb2_gates_data,},
+	{.compatible = "allwinner,sun4i-usb-gates-clk", .data = &sun4i_usb_gates_data,},
+	{.compatible = "allwinner,sun5i-a13-usb-gates-clk", .data = &sun5i_a13_usb_gates_data,},
 	{}
 };
Signature · 1 line
1.8.5.3
#5 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

Hi Hans,

Mostly looking good, but I have a few comments below.

Quoted 17 lines · Hans de Goede

From: Roman Byshko <[email protected]>

Add register definitions for the usb-clk register found on sun4i, sun5i and sun7i SoCs.

Signed-off-by Roman Byshko <[email protected]>
Signed-off-by Hans de Goede <[email protected]>
---
 Documentation/devicetree/bindings/clock/sunxi.txt |  5 +++++
 drivers/clk/sunxi/clk-sunxi.c                     | 12 ++++++++++++
 2 files changed, 17 insertions(+)
Documentation/devicetree/bindings/clock/sunxi.txt +0 -0
index 79c7197..8bccb6a 100644

Documentation/devicetree/bindings/clock/sunxi.txt Required properties: · line 41

Quoted 5 lines
Documentation/devicetree/bindings/clock/sunxi.txt +2 -0
@@ -37,6 +37,8 @@ Required properties:
 	"allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
 	"allwinner,sun4i-mod0-clk" - for the module 0 family of clocks
 	"allwinner,sun7i-a20-out-clk" - for the external output clocks
+	"allwinner,sun4i-usb-gates-clk" - for usb gates + resets on A10 / A20
+	"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13

Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that.

Documentation/devicetree/bindings/clock/sunxi.txt Required properties for all clocks: · line 56

Quoted 9 lines
Documentation/devicetree/bindings/clock/sunxi.txt +3 -0
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

@@ -49,6 +51,9 @@ Required properties for all clocks:
 Additionally, "allwinner,*-gates-clk" clocks require:
 - clock-output-names : the corresponding gate names that the clock controls
 
+And "allwinner,*-usb-gates-clk" clocks also require:
+- reset-cells : shall be set to 1
+

You should also document what value we should put in the cells, and where to refer to to find the right one.

Quoted 11 lines
Documentation/devicetree/bindings/clock/sunxi.txt +0 -0
 Clock consumers should specify the desired clocks they use with a
 "clocks" phandle cell. Consumers that are using a gated clock should
 provide an additional ID in their clock property. This ID is the
drivers/clk/sunxi/clk-sunxi.c +0 -0
index f1a147c..18cbc3c 100644
@@ -813,6 +813,16 @@ static const struct gates_data sun4i_ahb_gates_data __initconst = {
 	.mask = {0x7F77FFF, 0x14FB3F},
 };

drivers/clk/sunxi/clk-sunxi.c static const struct gates_data sun4i_ahb_gates_data __initconst = { · line 825

Quoted 10 lines
drivers/clk/sunxi/clk-sunxi.c +10 -0
+static const struct gates_data sun4i_usb_gates_data __initconst = {
+	.mask = {0x1C0},
+	.reset_mask = 0x07,
+};
+
+static const struct gates_data sun5i_a13_usb_gates_data __initconst = {
+	.mask = {0x140},
+	.reset_mask = 0x03,
+};
+

I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock?

Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT?

drivers/clk/sunxi/clk-sunxi.c static const struct of_device_id clk_gates_match[] __initconst = { · line 1175

Quoted 11 lines
drivers/clk/sunxi/clk-sunxi.c +2 -0
 static const struct gates_data sun5i_a10s_ahb_gates_data __initconst = {
 	.mask = {0x147667e7, 0x185915},
 };

@@ -1159,6 +1169,8 @@ static const struct of_device_id clk_gates_match[] __initconst = {
 	{.compatible = "allwinner,sun6i-a31-apb1-gates-clk", .data = &sun6i_a31_apb1_gates_data,},
 	{.compatible = "allwinner,sun7i-a20-apb1-gates-clk", .data = &sun7i_a20_apb1_gates_data,},
 	{.compatible = "allwinner,sun6i-a31-apb2-gates-clk", .data = &sun6i_a31_apb2_gates_data,},
+	{.compatible = "allwinner,sun4i-usb-gates-clk", .data = &sun4i_usb_gates_data,},
+	{.compatible = "allwinner,sun5i-a13-usb-gates-clk", .data = &sun5i_a13_usb_gates_data,},
 	{}
 };

Thanks a lot!
Maxime

Signature · 10 lines
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/20140127/3605b147/attachment.sig>
author
#6 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Quoted 7 lines · Maxime Ripard

Hi Hans,

Mostly looking good, but I have a few comments below.

Quoted 2 lines · Hans de Goede

From: Roman Byshko <[email protected]>

Quoted 8 lines
Quoted 6 lines

Add register definitions for the usb-clk register found on sun4i, sun5i and sun7i SoCs.

Signed-off-by Roman Byshko <[email protected]> Signed-off-by: Hans de Goede <[email protected]> --- Documentation/devicetree/bindings/clock/sunxi.txt | 5 +++++ drivers/clk/sunxi/clk-sunxi.c | 12 ++++++++++++ 2 files changed, 17 insertions(+)
Documentation/devicetree/bindings/clock/sunxi.txt index 79c7197..8bccb6a 100644 --- a/Documentation/devicetree/bindings/clock/sunxi.txt +++ b/Documentation/devicetree/bindings/clock/sunxi.txt @@ -37,6 +37,8 @@ Required properties: "allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31 "allwinner,sun4i-mod0-clk" - for the module 0 family of clocks "allwinner,sun7i-a20-out-clk" - for the external output clocks + "allwinner,sun4i-usb-gates-clk" - for usb gates + resets on A10 / A20 + renamed from Documentation/devicetree/bindings/clock/sunxi.txt +0 -0

"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13

Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that.

To be clear you mean s/usb-gates-clk/usb-clk/ right ?

That sounds reasonable :)

Quoted 6 lines
Quoted 4 lines

Required properties for all clocks: - reg : shall be the control register address for the clock. @@ -49,6 +51,9 @@ Required properties for all clocks: Additionally, "allwinner,*-gates-clk" clocks require: - clock-output-names : the corresponding gate names that the clock controls

+And "allwinner,*-usb-gates-clk" clocks also require: +- reset-cells : shall be set to 1 +

You should also document what value we should put in the cells, and where to refer to to find the right one.

Ok.

Quoted 6 lines
Quoted 4 lines

Clock consumers should specify the desired clocks they use with a "clocks" phandle cell. Consumers that are using a gated clock should provide an additional ID in their clock property. This ID is the diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c index f1a147c..18cbc3c 100644 --- a/drivers/clk/sunxi/clk-sunxi.c +++ b/drivers/clk/sunxi/clk-sunxi.c @@ -813,6 +813,16 @@ static const struct gates_data sun4i_ahb_gates_data __initconst = { .mask = {0x7F77FFF, 0x14FB3F}, };

+static const struct gates_data sun4i_usb_gates_data __initconst = { + .mask = {0x1C0}, + .reset_mask = 0x07, +}; + +static const struct gates_data sun5i_a13_usb_gates_data __initconst = { + .mask = {0x140}, + .reset_mask = 0x03, +}; +

I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock?

Correct.

Quoted 1 line

Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT?

Well for the other "gates" type clks we also have holes in the range, and we always refer to the clk with the bit number in the reg as the clock-cell value.

Here the hole just happens to be at the start, but it seems best to me to be consistent and keep using the bit nr inside the reg as clock-cell value, without an offset.

Quoted 5 lines
Quoted 3 lines

static const struct gates_data sun5i_a10s_ahb_gates_data __initconst = { .mask = {0x147667e7, 0x185915}, }; @@ -1159,6 +1169,8 @@ static const struct of_device_id clk_gates_match[] __initconst = { {.compatible = "allwinner,sun6i-a31-apb1-gates-clk", .data = &sun6i_a31_apb1_gates_data,}, {.compatible = "allwinner,sun7i-a20-apb1-gates-clk", .data = &sun7i_a20_apb1_gates_data,}, {.compatible = "allwinner,sun6i-a31-apb2-gates-clk", .data = &sun6i_a31_apb2_gates_data,}, + {.compatible = "allwinner,sun4i-usb-gates-clk", .data = &sun4i_usb_gates_data,}, + {.compatible =
"allwinner,sun5i-a13-usb-gates-clk", .data = &sun5i_a13_usb_gates_data,}, {} };

Thanks a lot! Maxime

Regards,

Hans

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLmcxYACgkQF3VEtJrzE/vjVgCfT8pMd/WAl2lr5HVURDcr6zz6 pDsAnjStUQa3j6WGOHPstjO2kV3WwkKO
=ozuq
-----END PGP SIGNATURE-----

#7 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

Quoted 6 lines · Hans de Goede
Quoted 4 lines
Quoted 1 line

"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13

Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that.

To be clear you mean s/usb-gates-clk/usb-clk/ right ?

Yep, exactly

Quoted 6 lines
Quoted 3 lines

I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock?

Correct.

Quoted 7 lines
Quoted 3 lines

Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT?

Well for the other "gates" type clks we also have holes in the range, and we always refer to the clk with the bit number in the reg as the clock-cell value.

Yes, we have holes, but I see two majors differences here:
- the other gates are just gates, while the usb clocks are a bit more than that.
- the other gates' gating bits thus all start at bit 0, while here, since it's kind of a "mixed" clock, the gating bits start at bit 6 (on the A20 at least)

Signature · 10 lines
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/20140128/4aeec0e2/attachment.sig>
author
#8 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Quoted 11 lines · Maxime Ripard
Quoted 5 lines · Hans de Goede
Quoted 3 lines
Quoted 1 line

"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13

Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that.

To be clear you mean s/usb-gates-clk/usb-clk/ right ?

Yep, exactly

Quoted 2 lines
Quoted 1 line

I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock?

Quoted 7 lines
Quoted 5 lines

Correct.

Quoted 1 line

Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT?

Well for the other "gates" type clks we also have holes in the range, and we always refer to the clk with the bit number in the reg as the clock-cell value.

Yes, we have holes, but I see two majors differences here: - the other gates are just gates, while the usb clocks are a bit more than that.

The usb-clk registers contain more then that, but the bits we are talking about now are gates.

Quoted 1 line

- the other gates' gating bits thus all start at bit 0, while here, since it's kind of a "mixed" clock, the gating bits start at bit 6 (on the A20 at least)

Right, still I believe that the consistent thing to do is keeping the bit-number for the bit in the register controlling the gate as the specifier. When adding new dts entries / reviewing existing ones I'm used to matching the specifier to the bit-nr in the data-sheet, I think making things different just for this one register is counter productive.

Regards,

Hans

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLnf80ACgkQF3VEtJrzE/udugCdEDpN65hazG7H+FD45iOVnTY9 548An3dXeF6f8wp5REck5H3gqQPQkIoX
=6yba
-----END PGP SIGNATURE-----

#9 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

Hi Hans,

Quoted 41 lines · Hans de Goede

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Quoted 20 lines · Maxime Ripard
Quoted 6 lines · Hans de Goede
Quoted 4 lines
Quoted 1 line

"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13

Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that.

To be clear you mean s/usb-gates-clk/usb-clk/ right ?

Yep, exactly

Quoted 10 lines
Quoted 3 lines

I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock?

Correct.

Quoted 3 lines

Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT?

Quoted 7 lines
Quoted 3 lines

Well for the other "gates" type clks we also have holes in the range, and we always refer to the clk with the bit number in the reg as the clock-cell value.

Yes, we have holes, but I see two majors differences here: - the other gates are just gates, while the usb clocks are a bit more than that.

The usb-clk registers contain more then that, but the bits we are talking about now are gates.

Quoted 3 lines

- the other gates' gating bits thus all start at bit 0, while
- here, since it's kind of a "mixed" clock, the gating bits start
- at bit 6 (on the A20 at least)

Quoted 6 lines

Right, still I believe that the consistent thing to do is keeping the bit-number for the bit in the register controlling the gate as the specifier. When adding new dts entries / reviewing existing ones I'm used to matching the specifier to the bit-nr in the data-sheet, I think making things different just for this one register is counter productive.

And if you turn it the other way around, it would be inconsistent that all gates indices start at 0, and we would start at 6 here :)

Plus, this clock is already a special case, since it's the only gate that is more than just a gate so far.

Maxime

Signature · 10 lines
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/20140204/a602e9cd/attachment.sig>
author
#10 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Quoted 27 lines · Maxime Ripard

Hi Hans,

Quoted 24 lines · Hans de Goede

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Hi,

Quoted 18 lines · Maxime Ripard
Quoted 5 lines · Hans de Goede
Quoted 3 lines
Quoted 1 line

"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13

Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that.

To be clear you mean s/usb-gates-clk/usb-clk/ right ?

Yep, exactly

Quoted 7 lines
Quoted 1 line

I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock?

Correct.

Quoted 1 line

Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT?

Well for the other "gates" type clks we also have holes in the range, and we always refer to the clk with the bit number in the reg as the clock-cell value.

Yes, we have holes, but I see two majors differences here: - the other gates are just gates, while the usb clocks are a bit more than that.

Quoted 7 lines
Quoted 5 lines

The usb-clk registers contain more then that, but the bits we are talking about now are gates.

Quoted 1 line

- the other gates' gating bits thus all start at bit 0, while - here, since it's kind of a "mixed" clock, the gating bits start - at bit 6 (on the A20 at least)

Right, still I believe that the consistent thing to do is keeping the bit-number for the bit in the register controlling the gate as the specifier. When adding new dts entries / reviewing existing ones I'm used to matching the specifier to the bit-nr in the data-sheet, I think making things different just for this one register is counter productive.

And if you turn it the other way around, it would be inconsistent that all gates indices start at 0, and we would start at 6 here :)

I think the problem here is that you see the specifier part of the clk phandle as an index, which it is not. All devicetree docs / code talks about specifiers or arguments not indexes. Once you stop seeing this as an index, you will hopefully also stop insisting this needs to start at 0 :)

Also note that it already is not an index for existing sunxi clks which have cells != 0, as there are holes in the bits used in the gates registers and calling the specifier an index suggest we're dealing with an array, and arrays never have holes.

The clk specifier as currently used in sunxi clks is a 1:1 mapping of the gate register bit numbers, as is clearly documented in ie:
Documentation/devicetree/bindings/clock/sunxi/sun4i-a10-gates.txt Where the datasheet is referenced as the source for (most) of the values to put in the specifier.

My biggest objection is that this would loose 1:1 mapping we currently have between the specifier and bit-nr in the register, which really is convenient when writing new dts bindings.

When we add an offset users will need to first lookup which clk they need in the datasheet and then look at both the dts bindings doc to find how this is mapped to the specifier. In my experience such an extra level of indirection in documentation is a PITA, and all that just so that some random number (it is not an index!) can start at 0 ?

To me adding an offset here and making the clk gates different form all our other clock gates just feels wrong.

Regards,

Hans
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLwvZMACgkQF3VEtJrzE/sz0gCfQNwhM/RpimtbhumvKKQ4a4V+ Vo4AoLTNKRZXlPC84hi1JInPGYvZIxuR
=dA68
-----END PGP SIGNATURE-----

#11 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

Quoted 62 lines · Hans de Goede

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Quoted 27 lines · Maxime Ripard

Hi Hans,

Quoted 24 lines · Hans de Goede

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Hi,

Quoted 18 lines · Maxime Ripard
Quoted 5 lines · Hans de Goede
Quoted 3 lines
Quoted 1 line

"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13

Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that.

To be clear you mean s/usb-gates-clk/usb-clk/ right ?

Yep, exactly

Quoted 7 lines
Quoted 1 line

I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock?

Correct.

Quoted 1 line

Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT?

Well for the other "gates" type clks we also have holes in the range, and we always refer to the clk with the bit number in the reg as the clock-cell value.

Yes, we have holes, but I see two majors differences here: - the other gates are just gates, while the usb clocks are a bit more than that.

Quoted 7 lines
Quoted 5 lines

The usb-clk registers contain more then that, but the bits we are talking about now are gates.

Quoted 1 line

- the other gates' gating bits thus all start at bit 0, while - here, since it's kind of a "mixed" clock, the gating bits start - at bit 6 (on the A20 at least)

Right, still I believe that the consistent thing to do is keeping the bit-number for the bit in the register controlling the gate as the specifier. When adding new dts entries / reviewing existing ones I'm used to matching the specifier to the bit-nr in the data-sheet, I think making things different just for this one register is counter productive.

And if you turn it the other way around, it would be inconsistent that all gates indices start at 0, and we would start at 6 here :)

I think the problem here is that you see the specifier part of the clk phandle as an index, which it is not. All devicetree docs / code talks about specifiers or arguments not indexes. Once you stop seeing this as an index, you will hopefully also stop insisting this needs to start at 0 :)

Also note that it already is not an index for existing sunxi clks which have cells != 0, as there are holes in the bits used in the gates registers and calling the specifier an index suggest we're dealing with an array, and arrays never have holes.

The clk specifier as currently used in sunxi clks is a 1:1 mapping of the gate register bit numbers, as is clearly documented in ie:
Documentation/devicetree/bindings/clock/sunxi/sun4i-a10-gates.txt Where the datasheet is referenced as the source for (most) of the values to put in the specifier.

My biggest objection is that this would loose 1:1 mapping we currently have between the specifier and bit-nr in the register, which really is convenient when writing new dts bindings.

Quoted 8 lines

When we add an offset users will need to first lookup which clk they need in the datasheet and then look at both the dts bindings doc to find how this is mapped to the specifier. In my experience such an extra level of indirection in documentation is a PITA, and all that just so that some random number (it is not an index!) can start at 0 ?

To me adding an offset here and making the clk gates different form all our other clock gates just feels wrong.

Emilio pretty much share your feeling. I won't get in the way then :)

I only had the compatible name comment left then.

Maxime

Signature · 10 lines
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/20140207/8b67746f/attachment-0001.sig>
author
#12 lore raw

PATCH v2 2/5 clk: sunxi: Add USB clock register defintions

Hi,

Quoted 69 lines · Maxime Ripard
Quoted 68 lines · Hans de Goede

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Quoted 27 lines · Maxime Ripard

Hi Hans,

Quoted 24 lines · Hans de Goede

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Hi,

Quoted 18 lines · Maxime Ripard
Quoted 5 lines · Hans de Goede
Quoted 3 lines
Quoted 1 line

"allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13

Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that.

To be clear you mean s/usb-gates-clk/usb-clk/ right ?

Yep, exactly

Quoted 7 lines
Quoted 1 line

I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock?

Correct.

Quoted 1 line

Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT?

Well for the other "gates" type clks we also have holes in the range, and we always refer to the clk with the bit number in the reg as the clock-cell value.

Yes, we have holes, but I see two majors differences here: - the other gates are just gates, while the usb clocks are a bit more than that.

Quoted 7 lines
Quoted 5 lines

The usb-clk registers contain more then that, but the bits we are talking about now are gates.

Quoted 1 line

- the other gates' gating bits thus all start at bit 0, while - here, since it's kind of a "mixed" clock, the gating bits start - at bit 6 (on the A20 at least)

Right, still I believe that the consistent thing to do is keeping the bit-number for the bit in the register controlling the gate as the specifier. When adding new dts entries / reviewing existing ones I'm used to matching the specifier to the bit-nr in the data-sheet, I think making things different just for this one register is counter productive.

And if you turn it the other way around, it would be inconsistent that all gates indices start at 0, and we would start at 6 here :)

I think the problem here is that you see the specifier part of the clk phandle as an index, which it is not. All devicetree docs / code talks about specifiers or arguments not indexes. Once you stop seeing this as an index, you will hopefully also stop insisting this needs to start at 0 :)

Also note that it already is not an index for existing sunxi clks which have cells != 0, as there are holes in the bits used in the gates registers and calling the specifier an index suggest we're dealing with an array, and arrays never have holes.

The clk specifier as currently used in sunxi clks is a 1:1 mapping of the gate register bit numbers, as is clearly documented in ie:
Documentation/devicetree/bindings/clock/sunxi/sun4i-a10-gates.txt Where the datasheet is referenced as the source for (most) of the values to put in the specifier.

My biggest objection is that this would loose 1:1 mapping we currently have between the specifier and bit-nr in the register, which really is convenient when writing new dts bindings.

When we add an offset users will need to first lookup which clk they need in the datasheet and then look at both the dts bindings doc to find how this is mapped to the specifier. In my experience such an extra level of indirection in documentation is a PITA, and all that just so that some random number (it is not an index!) can start at 0 ?

Quoted 6 lines
Quoted 2 lines

To me adding an offset here and making the clk gates different form all our other clock gates just feels wrong.

Emilio pretty much share your feeling. I won't get in the way then :)

I only had the compatible name comment left then.

Yes I've already fixed that in my latest sunxi-devel tree:
https://github.com/linux-sunxi/linux-sunxi/commits/sunxi-devel

Which is also rebased to 3.14-rc1 for those interested, I'll resend the usb-clk patches with this + Emilio's reverted remark fixed soon-ish then.

Regards,

Hans

author
#13 lore raw

PATCH v2 3/5 ARM: sun4i: dt: Add bindings for USB clocks

In reply to Hans de Goede

From: Roman Byshko <[email protected]>

Signed-off-by Roman Byshko <[email protected]>
Signed-off-by Hans de Goede <[email protected]>
---
 arch/arm/boot/dts/sun4i-a10.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm/boot/dts/sun4i-a10.dtsi +9 -0
index 7264ef9..913674b 100644
@@ -290,6 +290,15 @@
 			clock-output-names = "ir1";
 		};
 
+		usb_clk: clk at 01c200cc {
+			#clock-cells = <1>;
+		        #reset-cells = <1>;
+			compatible = "allwinner,sun4i-usb-gates-clk";
+			reg = <0x01c200cc 0x4>;
+			clocks = <&pll6 1>;
+			clock-output-names = "usb_ohci0", "usb_ohci1", "usb_phy";
+		};
+
 		spi3_clk: clk at 01c200d4 {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-mod0-clk";
Signature · 1 line
1.8.5.3
author
#14 lore raw

PATCH v2 4/5 ARM: sun5i: dt: Add bindings for USB clocks

In reply to Hans de Goede

From: Roman Byshko <[email protected]>

Signed-off-by Roman Byshko <[email protected]>
Signed-off-by Hans de Goede <[email protected]>
---
 arch/arm/boot/dts/sun5i-a10s.dtsi | 9 +++++++++
 arch/arm/boot/dts/sun5i-a13.dtsi  | 9 +++++++++
 2 files changed, 18 insertions(+)
arch/arm/boot/dts/sun5i-a10s.dtsi +9 -0
index 9591959..44235e7 100644
@@ -255,6 +255,15 @@
 			clock-output-names = "ir0";
 		};
 
+		usb_clk: clk at 01c200cc {
+			#clock-cells = <1>;
+		        #reset-cells = <1>;
+			compatible = "allwinner,sun5i-a13-usb-gates-clk";
+			reg = <0x01c200cc 0x4>;
+			clocks = <&pll6 1>;
+			clock-output-names = "usb_ohci0", "usb_phy";
+		};
+
 		mbus_clk: clk at 01c2015c {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-mod0-clk";
arch/arm/boot/dts/sun5i-a13.dtsi +9 -0
index f6f2c81..4193fdf 100644
@@ -252,6 +252,15 @@
 			clock-output-names = "ir0";
 		};
 
+		usb_clk: clk at 01c200cc {
+			#clock-cells = <1>;
+		        #reset-cells = <1>;
+			compatible = "allwinner,sun5i-a13-usb-gates-clk";
+			reg = <0x01c200cc 0x4>;
+			clocks = <&pll6 1>;
+			clock-output-names = "usb_ohci0", "usb_phy";
+		};
+
 		mbus_clk: clk at 01c2015c {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-mod0-clk";
Signature · 1 line
1.8.5.3
author
#15 lore raw

PATCH v2 5/5 ARM: sun7i: dt: Add bindings for USB clocks

In reply to Hans de Goede

From: Roman Byshko <[email protected]>

Signed-off-by Roman Byshko <[email protected]>
Signed-off-by Hans de Goede <[email protected]>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm/boot/dts/sun7i-a20.dtsi +9 -0
index 57f1fc9..a44b3b3 100644
@@ -280,6 +280,15 @@
 			clock-output-names = "ir1";
 		};
 
+		usb_clk: clk at 01c200cc {
+			#clock-cells = <1>;
+		        #reset-cells = <1>;
+			compatible = "allwinner,sun4i-usb-gates-clk";
+			reg = <0x01c200cc 0x4>;
+			clocks = <&pll6 1>;
+			clock-output-names = "usb_ohci0", "usb_ohci1", "usb_phy";
+		};
+
 		spi3_clk: clk at 01c200d4 {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-mod0-clk";
Signature · 1 line
1.8.5.3
#16 lore raw

v2 cover clk: sunxi: Add support for USB clocks and reset bits

In reply to Hans de Goede
Quoted 9 lines · Hans de Goede

Hi Emilio, Maxime, et al,

Emilio, here is v2 of my patch-set adding support for sunxi-clk USB clocks and reset bits. This addresses all your review comments from v1.

Can you add the first 2 patches to your queue of patches for Mike for 3.15 ?

Maxime, can you add patch 3-5 which add the dt bindings for this to your tree please ?

Apart from the comments I had on patch 2, it looks good for me. Once we agree on something, you have my Acked-by.

Thanks for working on this!
Maxime

Signature · 10 lines
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/20140127/cce4f6e6/attachment.sig>