2013-07-30 14:44:51

by Maxime Ripard

Subject: [PATCH 0/4] Add support for the Allwinner A31 clocks

Hi everyone,

The Allwinner A31 SoC has a quite different clock tree and structure that the
other Allwinner SoCs that we already merged, so it requires more work than what
we had for the other SoCs until now.

It's also an opportunity to do some minor cleanup/refactoring to land the new
code properly.

This patch set is based on the "ARM: sunxi: Add support for the Allwinner A31
SoC" serie I sent previously.

Thanks!
Maxime

Maxime Ripard (4):
clk: sunxi: Rename the structure to prepare the addition of sun6i
clk: sunxi: Allow to specify the divider width from the dividers data
clk: sunxi: Add A31 clocks support
ARM: sun6i: Enable clock support in the DTSI

arch/arm/boot/dts/sun6i-a31.dtsi | 137 +++++++++++++++++++--
drivers/clk/sunxi/clk-sunxi.c | 252 ++++++++++++++++++++++++++++++++-------
2 files changed, 335 insertions(+), 54 deletions(-)

--
1.8.3.4

2013-07-30 14:44:57

by Maxime Ripard

Subject: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI

Now that the clock driver has support for the A31 clocks, we can add
them to the DTSI and start using them in the relevant hardware blocks.

Signed-off-by: Maxime Ripard <[email protected]>
---
arch/arm/boot/dts/sun6i-a31.dtsi | 137 ++++++++++++++++++++++++++++++++++++---
1 file changed, 127 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 0d13dc6..c6a3a91 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -51,13 +51,130 @@

clocks {
#address-cells = <1>;
- #size-cells = <0>;
+ #size-cells = <1>;
+ ranges;

- osc: oscillator {
+ osc24M: hosc {
#clock-cells = <0>;
compatible = "fixed-clock";
clock-frequency = <24000000>;
};
+
+ osc32k: losc {
+ #clock-cells = <0>;
+ compatible = "fixed-clock";
+ clock-frequency = <32768>;
+ };
+
+ pll1: pll1@01c20000 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun6i-pll1-clk";
+ reg = <0x01c20000 0x4>;
+ clocks = <&osc24M>;
+ };
+
+ /*
+ * This is a dummy clock, to be used as placeholder on
+ * other mux clocks when a specific parent clock is not
+ * yet implemented. It should be dropped when the driver
+ * is complete.
+ */
+ pll6: pll6 {
+ #clock-cells = <0>;
+ compatible = "fixed-clock";
+ clock-frequency = <0>;
+ };
+
+ cpu: cpu@01c20050 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun4i-cpu-clk";
+ reg = <0x01c20050 0x4>;
+ clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>;
+ };
+
+ axi: axi@01c20050 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun4i-axi-clk";
+ reg = <0x01c20050 0x4>;
+ clocks = <&cpu>;
+ };
+
+ ahb1_mux: ahb1_mux@01c20054 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun6i-ahb1-mux-clk";
+ reg = <0x01c20054 0x4>;
+ clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
+ };
+
+ ahb1: ahb1@01c20054 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun4i-ahb-clk";
+ reg = <0x01c20054 0x4>;
+ clocks = <&ahb1_mux>;
+ };
+
+ ahb1_gates: ahb1_gates@01c20060 {
+ #clock-cells = <1>;
+ compatible = "allwinner,sun6i-a31-ahb1-gates-clk";
+ reg = <0x01c20060 0x8>;
+ clocks = <&ahb1>;
+ clock-output-names = "ahb1_mipidsi", "ahb1_ss",
+ "ahb1_dma", "ahb1_mmc0", "ahb1_mmc1",
+ "ahb1_mmc2", "ahb1_mmc3", "ahb1_nand1",
+ "ahb1_nand0", "ahb1_sdram",
+ "ahb1_gmac", "ahb1_ts", "ahb1_hstimer",
+ "ahb1_spi0", "ahb1_spi1", "ahb1_spi2",
+ "ahb1_spi3", "ahb1_otg", "ahb1_ehci0",
+ "ahb1_ehci1", "ahb1_ohci0",
+ "ahb1_ohci1", "ahb1_ohci2", "ahb1_ve",
+ "ahb1_lcd0", "ahb1_lcd1", "ahb1_csi",
+ "ahb1_hdmi", "ahb1_de0", "ahb1_de1",
+ "ahb1_fe0", "ahb1_fe1", "ahb1_mp",
+ "ahb1_gpu", "ahb1_deu0", "ahb1_deu1",
+ "ahb1_drc0", "ahb1_drc1";
+ };
+
+ apb1: apb1@01c20054 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun4i-apb0-clk";
+ reg = <0x01c20054 0x4>;
+ clocks = <&ahb1>;
+ };
+
+ apb1_gates: apb1_gates@01c20060 {
+ #clock-cells = <1>;
+ compatible = "allwinner,sun6i-a31-apb1-gates-clk";
+ reg = <0x01c20068 0x4>;
+ clocks = <&apb1>;
+ clock-output-names = "apb1_codec", "apb1_digital_mic",
+ "apb1_pio", "apb1_daudio0",
+ "apb1_daudio1";
+ };
+
+ apb2_mux: apb2_mux@01c20058 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun4i-apb1-mux-clk";
+ reg = <0x01c20058 0x4>;
+ clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>;
+ };
+
+ apb2: apb2@01c20058 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun6i-apb2-div-clk";
+ reg = <0x01c20058 0x4>;
+ clocks = <&apb2_mux>;
+ };
+
+ apb2_gates: apb2_gates@01c2006c {
+ #clock-cells = <1>;
+ compatible = "allwinner,sun6i-a31-apb2-gates-clk";
+ reg = <0x01c2006c 0x8>;
+ clocks = <&apb2>;
+ clock-output-names = "apb2_i2c0", "apb2_i2c1",
+ "apb2_i2c2", "apb2_i2c3", "apb2_uart0",
+ "apb2_uart1", "apb2_uart2", "apb2_uart3",
+ "apb2_uart4", "apb2_uart5";
+ };
};

soc@01c20000 {
@@ -69,7 +186,7 @@
compatible = "allwinner,sun6i-a31-pinctrl";
reg = <0x01c20800 0x400>;
interrupts = <0 11 1>, <0 15 1>, <0 16 1>, <0 17 1>;
- clocks = <&osc>;
+ clocks = <&apb1_gates 5>;
gpio-controller;
interrupt-controller;
#address-cells = <1>;
@@ -92,7 +209,7 @@
<0 20 1>,
<0 21 1>,
<0 22 1>;
- clocks = <&osc>;
+ clocks = <&osc24M>;
};

wdt1: watchdog@01c20ca0 {
@@ -106,7 +223,7 @@
interrupts = <0 0 1>;
reg-shift = <2>;
reg-io-width = <4>;
- clocks = <&osc>;
+ clocks = <&apb2_gates 16>;
status = "disabled";
};

@@ -116,7 +233,7 @@
interrupts = <0 1 1>;
reg-shift = <2>;
reg-io-width = <4>;
- clocks = <&osc>;
+ clocks = <&apb2_gates 17>;
status = "disabled";
};

@@ -126,7 +243,7 @@
interrupts = <0 2 1>;
reg-shift = <2>;
reg-io-width = <4>;
- clocks = <&osc>;
+ clocks = <&apb2_gates 18>;
status = "disabled";
};

@@ -136,7 +253,7 @@
interrupts = <0 3 1>;
reg-shift = <2>;
reg-io-width = <4>;
- clocks = <&osc>;
+ clocks = <&apb2_gates 19>;
status = "disabled";
};

@@ -146,7 +263,7 @@
interrupts = <0 4 1>;
reg-shift = <2>;
reg-io-width = <4>;
- clocks = <&osc>;
+ clocks = <&apb2_gates 20>;
status = "disabled";
};

@@ -156,7 +273,7 @@
interrupts = <0 5 1>;
reg-shift = <2>;
reg-io-width = <4>;
- clocks = <&osc>;
+ clocks = <&apb2_gates 21>;
status = "disabled";
};

--
1.8.3.4

2013-07-30 14:44:55

by Maxime Ripard

Subject: [PATCH 2/4] clk: sunxi: Allow to specify the divider width from the dividers data

The divider width used to be hardcoded. Some A31 dividers are no longer
with the hardcoded width, so we need to make it specific to each divider
and set it in the dividers data.

Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/clk/sunxi/clk-sunxi.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 3c91888..6e9cbc9 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -277,26 +277,28 @@ static void __init sunxi_mux_clk_setup(struct device_node *node,
* sunxi_divider_clk_setup() - Setup function for simple divider clocks
*/

-#define SUNXI_DIVISOR_WIDTH 2
-
struct div_data {
- u8 shift;
- u8 pow;
+ u8 shift;
+ u8 pow;
+ u8 width;
};

static const __initconst struct div_data sun4i_axi_data = {
- .shift = 0,
- .pow = 0,
+ .shift = 0,
+ .pow = 0,
+ .width = 2,
};

static const __initconst struct div_data sun4i_ahb_data = {
- .shift = 4,
- .pow = 1,
+ .shift = 4,
+ .pow = 1,
+ .width = 2,
};

static const __initconst struct div_data sun4i_apb0_data = {
- .shift = 8,
- .pow = 1,
+ .shift = 8,
+ .pow = 1,
+ .width = 2,
};

static void __init sunxi_divider_clk_setup(struct device_node *node,
@@ -312,7 +314,7 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
clk_parent = of_clk_get_parent_name(node, 0);

clk = clk_register_divider(NULL, clk_name, clk_parent, 0,
- reg, data->shift, SUNXI_DIVISOR_WIDTH,
+ reg, data->shift, data->width,
data->pow ? CLK_DIVIDER_POWER_OF_TWO : 0,
&clk_lock);
if (clk) {
--
1.8.3.4

2013-07-30 14:45:36

by Maxime Ripard

Subject: [PATCH 3/4] clk: sunxi: Add A31 clocks support

The A31 has a mostly different clock set compared to the other older
SoCs currently supported in the Allwinner clock driver.

Add support for the basic useful clocks. The other ones will come in
eventually.

Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/clk/sunxi/clk-sunxi.c | 120 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 120 insertions(+)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 6e9cbc9..8cd69e6 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -124,7 +124,67 @@ static void sun4i_get_pll1_factors(u32 *freq, u32 parent_rate,
*n = div / 4;
}

+/**
+ * sun6i_get_pll1_factors() - calculates n, k and m factors for PLL1
+ * PLL1 rate is calculated as follows
+ * rate = parent_rate * (n + 1) * (k + 1) / (m + 1);
+ * parent_rate should always be 24MHz
+ */
+static void sun6i_get_pll1_factors(u32 *freq, u32 parent_rate,
+ u8 *n, u8 *k, u8 *m, u8 *p)
+{
+ /*
+ * We can operate only on MHz, this will make our life easier
+ * later.
+ */
+ u32 freq_mhz = *freq / 1000000;
+ u32 parent_freq_mhz = parent_rate / 1000000;
+
+ /* If the frequency is a multiple of 32 MHz, k is always 3 */
+ if (!(freq_mhz % 32))
+ *k = 3;
+ /* If the frequency is a multiple of 9 MHz, k is always 2 */
+ else if (!(freq_mhz % 9))
+ *k = 2;
+ /* If the frequency is a multiple of 8 MHz, k is always 1 */
+ else if (!(freq_mhz % 8))
+ *k = 1;
+ /* Otherwise, we don't use the k factor */
+ else
+ *k = 0;

+ /*
+ * If the frequency is a multiple of 2 but not a multiple of
+ * 3, m is 3. This is the first time we use 6 here, yet we
+ * will use it on several other places.
+ * We use this number because it's the lowest frequency we can
+ * generate (with n = 0, k = 0, m = 3), so every other frequency
+ * somehow relates to this frequency.
+ */
+ if ((freq_mhz % 6) == 2 || (freq_mhz % 6) == 4)
+ *m = 2;
+ /*
+ * If the frequency is a multiple of 6MHz, but the factor is
+ * odd, m will be 3
+ */
+ else if ((freq_mhz / 6) & 1)
+ *m = 3;
+ /* Otherwise, we end up with m = 1 */
+ else
+ *m = 1;
+
+ /* Calculate n thanks to the above factors we already got */
+ *n = freq_mhz * (*m + 1) / ((*k + 1) * parent_freq_mhz) - 1;
+
+ /*
+ * If n end up being outbound, and that we can still decrease
+ * m, do it.
+ */
+ if ((*n + 1) > 31 && (*m + 1) > 1) {
+ *n = (*n + 1) / 2 - 1;
+ *m = (*m + 1) / 2 - 1;
+ }
+}

/**
* sun4i_get_apb1_factors() - calculates m, p factors for APB1
@@ -189,6 +249,15 @@ static struct clk_factors_config sun4i_pll1_config = {
.pwidth = 2,
};

+static struct clk_factors_config sun6i_pll1_config = {
+ .nshift = 8,
+ .nwidth = 5,
+ .kshift = 4,
+ .kwidth = 2,
+ .mshift = 0,
+ .mwidth = 2,
+};
+
static struct clk_factors_config sun4i_apb1_config = {
.mshift = 0,
.mwidth = 5,
@@ -201,6 +270,11 @@ static const __initconst struct factors_data sun4i_pll1_data = {
.getter = sun4i_get_pll1_factors,
};

+static const __initconst struct factors_data sun6i_pll1_data = {
+ .table = &sun6i_pll1_config,
+ .getter = sun6i_get_pll1_factors,
+};
+
static const __initconst struct factors_data sun4i_apb1_data = {
.table = &sun4i_apb1_config,
.getter = sun4i_get_apb1_factors,
@@ -243,6 +317,10 @@ static const __initconst struct mux_data sun4i_cpu_mux_data = {
.shift = 16,
};

+static const __initconst struct mux_data sun6i_ahb1_mux_data = {
+ .shift = 12,
+};
+
static const __initconst struct mux_data sun4i_apb1_mux_data = {
.shift = 24,
};
@@ -301,6 +379,12 @@ static const __initconst struct div_data sun4i_apb0_data = {
.width = 2,
};

+static const __initconst struct div_data sun6i_apb2_div_data = {
+ .shift = 0,
+ .pow = 0,
+ .width = 4,
+};
+
static void __init sunxi_divider_clk_setup(struct device_node *node,
struct div_data *data)
{
@@ -347,6 +431,10 @@ static const __initconst struct gates_data sun5i_a13_ahb_gates_data = {
.mask = {0x107067e7, 0x185111},
};

+static const __initconst struct gates_data sun6i_a31_ahb1_gates_data = {
+ .mask = { 0xedfe7f62, 0x794f931 },
+};
+
static const __initconst struct gates_data sun4i_apb0_gates_data = {
.mask = {0x4EF},
};
@@ -363,6 +451,14 @@ static const __initconst struct gates_data sun5i_a13_apb1_gates_data = {
.mask = {0xa0007},
};

+static const __initconst struct gates_data sun6i_a31_apb1_gates_data = {
+ .mask = { 0x3031 },
+};
+
+static const __initconst struct gates_data sun6i_a31_apb2_gates_data = {
+ .mask = { 0x3f000f },
+};
+
static void __init sunxi_gates_clk_setup(struct device_node *node,
struct gates_data *data)
{
@@ -420,6 +516,10 @@ static const __initconst struct of_device_id clk_factors_match[] = {
.data = &sun4i_pll1_data,
},
{
+ .compatible = "allwinner,sun6i-pll1-clk",
+ .data = &sun6i_pll1_data,
+ },
+ {
.compatible = "allwinner,sun4i-apb1-clk",
.data = &sun4i_apb1_data,
},
@@ -440,6 +540,10 @@ static const __initconst struct of_device_id clk_div_match[] = {
.compatible = "allwinner,sun4i-apb0-clk",
.data = &sun4i_apb0_data,
},
+ {
+ .compatible = "allwinner,sun6i-apb2-div-clk",
+ .data = &sun6i_apb2_div_data,
+ },
{}
};

@@ -453,6 +557,10 @@ static const __initconst struct of_device_id clk_mux_match[] = {
.compatible = "allwinner,sun4i-apb1-mux-clk",
.data = &sun4i_apb1_mux_data,
},
+ {
+ .compatible = "allwinner,sun6i-ahb1-mux-clk",
+ .data = &sun6i_ahb1_mux_data,
+ },
{}
};

@@ -471,6 +579,10 @@ static const __initconst struct of_device_id clk_gates_match[] = {
.data = &sun5i_a13_ahb_gates_data,
},
{
+ .compatible = "allwinner,sun6i-a31-ahb1-gates-clk",
+ .data = &sun6i_a31_ahb1_gates_data,
+ },
+ {
.compatible = "allwinner,sun4i-apb0-gates-clk",
.data = &sun4i_apb0_gates_data,
},
@@ -486,6 +598,14 @@ static const __initconst struct of_device_id clk_gates_match[] = {
.compatible = "allwinner,sun5i-a13-apb1-gates-clk",
.data = &sun5i_a13_apb1_gates_data,
},
+ {
+ .compatible = "allwinner,sun6i-a31-apb1-gates-clk",
+ .data = &sun6i_a31_apb1_gates_data,
+ },
+ {
+ .compatible = "allwinner,sun6i-a31-apb2-gates-clk",
+ .data = &sun6i_a31_apb2_gates_data,
+ },
{}
};

--
1.8.3.4

2013-07-30 14:44:53

by Maxime Ripard

Subject: [PATCH 1/4] clk: sunxi: Rename the structure to prepare the addition of sun6i

Rename all the generic-named structure to sun4i to avoid confusion when
we will introduce the sun6i (A31) clocks.

While we're at it, avoid too long lines and wrap the DT compatibles
tables.

Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/clk/sunxi/clk-sunxi.c | 108 +++++++++++++++++++++++++++++-------------
1 file changed, 75 insertions(+), 33 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index fe1528e..3c91888 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -25,12 +25,12 @@
static DEFINE_SPINLOCK(clk_lock);

/**
- * sunxi_osc_clk_setup() - Setup function for gatable oscillator
+ * sun4i_osc_clk_setup() - Setup function for gatable oscillator
*/

#define SUNXI_OSC24M_GATE 0

-static void __init sunxi_osc_clk_setup(struct device_node *node)
+static void __init sun4i_osc_clk_setup(struct device_node *node)
{
struct clk *clk;
struct clk_fixed_rate *fixed;
@@ -73,13 +73,13 @@ static void __init sunxi_osc_clk_setup(struct device_node *node)


/**
- * sunxi_get_pll1_factors() - calculates n, k, m, p factors for PLL1
+ * sun4i_get_pll1_factors() - calculates n, k, m, p factors for PLL1
* PLL1 rate is calculated as follows
* rate = (parent_rate * n * (k + 1) >> p) / (m + 1);
* parent_rate is always 24Mhz
*/

-static void sunxi_get_pll1_factors(u32 *freq, u32 parent_rate,
+static void sun4i_get_pll1_factors(u32 *freq, u32 parent_rate,
u8 *n, u8 *k, u8 *m, u8 *p)
{
u8 div;
@@ -127,12 +127,12 @@ static void sunxi_get_pll1_factors(u32 *freq, u32 parent_rate,


/**
- * sunxi_get_apb1_factors() - calculates m, p factors for APB1
+ * sun4i_get_apb1_factors() - calculates m, p factors for APB1
* APB1 rate is calculated as follows
* rate = (parent_rate >> p) / (m + 1);
*/

-static void sunxi_get_apb1_factors(u32 *freq, u32 parent_rate,
+static void sun4i_get_apb1_factors(u32 *freq, u32 parent_rate,
u8 *n, u8 *k, u8 *m, u8 *p)
{
u8 calcm, calcp;
@@ -178,7 +178,7 @@ struct factors_data {
void (*getter) (u32 *rate, u32 parent_rate, u8 *n, u8 *k, u8 *m, u8 *p);
};

-static struct clk_factors_config pll1_config = {
+static struct clk_factors_config sun4i_pll1_config = {
.nshift = 8,
.nwidth = 5,
.kshift = 4,
@@ -189,21 +189,21 @@ static struct clk_factors_config pll1_config = {
.pwidth = 2,
};

-static struct clk_factors_config apb1_config = {
+static struct clk_factors_config sun4i_apb1_config = {
.mshift = 0,
.mwidth = 5,
.pshift = 16,
.pwidth = 2,
};

-static const __initconst struct factors_data pll1_data = {
- .table = &pll1_config,
- .getter = sunxi_get_pll1_factors,
+static const __initconst struct factors_data sun4i_pll1_data = {
+ .table = &sun4i_pll1_config,
+ .getter = sun4i_get_pll1_factors,
};

-static const __initconst struct factors_data apb1_data = {
- .table = &apb1_config,
- .getter = sunxi_get_apb1_factors,
+static const __initconst struct factors_data sun4i_apb1_data = {
+ .table = &sun4i_apb1_config,
+ .getter = sun4i_get_apb1_factors,
};

static void __init sunxi_factors_clk_setup(struct device_node *node,
@@ -239,11 +239,11 @@ struct mux_data {
u8 shift;
};

-static const __initconst struct mux_data cpu_mux_data = {
+static const __initconst struct mux_data sun4i_cpu_mux_data = {
.shift = 16,
};

-static const __initconst struct mux_data apb1_mux_data = {
+static const __initconst struct mux_data sun4i_apb1_mux_data = {
.shift = 24,
};

@@ -284,17 +284,17 @@ struct div_data {
u8 pow;
};

-static const __initconst struct div_data axi_data = {
+static const __initconst struct div_data sun4i_axi_data = {
.shift = 0,
.pow = 0,
};

-static const __initconst struct div_data ahb_data = {
+static const __initconst struct div_data sun4i_ahb_data = {
.shift = 4,
.pow = 1,
};

-static const __initconst struct div_data apb0_data = {
+static const __initconst struct div_data sun4i_apb0_data = {
.shift = 8,
.pow = 1,
};
@@ -413,35 +413,77 @@ CLK_OF_DECLARE(sunxi_osc, "allwinner,sun4i-osc-clk", sunxi_osc_clk_setup);

/* Matches for factors clocks */
static const __initconst struct of_device_id clk_factors_match[] = {
- {.compatible = "allwinner,sun4i-pll1-clk", .data = &pll1_data,},
- {.compatible = "allwinner,sun4i-apb1-clk", .data = &apb1_data,},
+ {
+ .compatible = "allwinner,sun4i-pll1-clk",
+ .data = &sun4i_pll1_data,
+ },
+ {
+ .compatible = "allwinner,sun4i-apb1-clk",
+ .data = &sun4i_apb1_data,
+ },
{}
};

/* Matches for divider clocks */
static const __initconst struct of_device_id clk_div_match[] = {
- {.compatible = "allwinner,sun4i-axi-clk", .data = &axi_data,},
- {.compatible = "allwinner,sun4i-ahb-clk", .data = &ahb_data,},
- {.compatible = "allwinner,sun4i-apb0-clk", .data = &apb0_data,},
+ {
+ .compatible = "allwinner,sun4i-axi-clk",
+ .data = &sun4i_axi_data,
+ },
+ {
+ .compatible = "allwinner,sun4i-ahb-clk",
+ .data = &sun4i_ahb_data,
+ },
+ {
+ .compatible = "allwinner,sun4i-apb0-clk",
+ .data = &sun4i_apb0_data,
+ },
{}
};

/* Matches for mux clocks */
static const __initconst struct of_device_id clk_mux_match[] = {
- {.compatible = "allwinner,sun4i-cpu-clk", .data = &cpu_mux_data,},
- {.compatible = "allwinner,sun4i-apb1-mux-clk", .data = &apb1_mux_data,},
+ {
+ .compatible = "allwinner,sun4i-cpu-clk",
+ .data = &sun4i_cpu_mux_data,
+ },
+ {
+ .compatible = "allwinner,sun4i-apb1-mux-clk",
+ .data = &sun4i_apb1_mux_data,
+ },
{}
};

/* Matches for gate clocks */
static const __initconst struct of_device_id clk_gates_match[] = {
- {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &sun4i_axi_gates_data,},
- {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &sun4i_ahb_gates_data,},
- {.compatible = "allwinner,sun5i-a13-ahb-gates-clk", .data = &sun5i_a13_ahb_gates_data,},
- {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &sun4i_apb0_gates_data,},
- {.compatible = "allwinner,sun5i-a13-apb0-gates-clk", .data = &sun5i_a13_apb0_gates_data,},
- {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &sun4i_apb1_gates_data,},
- {.compatible = "allwinner,sun5i-a13-apb1-gates-clk", .data = &sun5i_a13_apb1_gates_data,},
+ {
+ .compatible = "allwinner,sun4i-axi-gates-clk",
+ .data = &sun4i_axi_gates_data,
+ },
+ {
+ .compatible = "allwinner,sun4i-ahb-gates-clk",
+ .data = &sun4i_ahb_gates_data,
+ },
+ {
+ .compatible = "allwinner,sun5i-a13-ahb-gates-clk",
+ .data = &sun5i_a13_ahb_gates_data,
+ },
+ {
+ .compatible = "allwinner,sun4i-apb0-gates-clk",
+ .data = &sun4i_apb0_gates_data,
+ },
+ {
+ .compatible = "allwinner,sun5i-a13-apb0-gates-clk",
+ .data = &sun5i_a13_apb0_gates_data,
+ },
+ {
+ .compatible = "allwinner,sun4i-apb1-gates-clk",
+ .data = &sun4i_apb1_gates_data,
+ },
+ {
+ .compatible = "allwinner,sun5i-a13-apb1-gates-clk",
+ .data = &sun5i_a13_apb1_gates_data,
+ },
{}
};

--
1.8.3.4

2013-07-31 00:14:39

by Emilio López

Subject: Re: [PATCH 1/4] clk: sunxi: Rename the structure to prepare the addition of sun6i

Hi Maxime,

El 30/07/13 11:44, Maxime Ripard escribi?:
> Rename all the generic-named structure to sun4i to avoid confusion when
> we will introduce the sun6i (A31) clocks.
>
> While we're at it, avoid too long lines and wrap the DT compatibles
> tables.
>
> Signed-off-by: Maxime Ripard <[email protected]>

Overall the patch looks good :)

> ---
> drivers/clk/sunxi/clk-sunxi.c | 108 +++++++++++++++++++++++++++++-------------
> 1 file changed, 75 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index fe1528e..3c91888 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -25,12 +25,12 @@
> static DEFINE_SPINLOCK(clk_lock);
>
> /**
> - * sunxi_osc_clk_setup() - Setup function for gatable oscillator
> + * sun4i_osc_clk_setup() - Setup function for gatable oscillator
> */
>
> #define SUNXI_OSC24M_GATE 0
>
> -static void __init sunxi_osc_clk_setup(struct device_node *node)
> +static void __init sun4i_osc_clk_setup(struct device_node *node)
> {
> struct clk *clk;
> struct clk_fixed_rate *fixed;
> @@ -73,13 +73,13 @@ static void __init sunxi_osc_clk_setup(struct device_node *node)
>
>
> /**
> - * sunxi_get_pll1_factors() - calculates n, k, m, p factors for PLL1
> + * sun4i_get_pll1_factors() - calculates n, k, m, p factors for PLL1
> * PLL1 rate is calculated as follows
> * rate = (parent_rate * n * (k + 1) >> p) / (m + 1);
> * parent_rate is always 24Mhz
> */
>
> -static void sunxi_get_pll1_factors(u32 *freq, u32 parent_rate,
> +static void sun4i_get_pll1_factors(u32 *freq, u32 parent_rate,
> u8 *n, u8 *k, u8 *m, u8 *p)
> {
> u8 div;
> @@ -127,12 +127,12 @@ static void sunxi_get_pll1_factors(u32 *freq, u32 parent_rate,
>
>
> /**
> - * sunxi_get_apb1_factors() - calculates m, p factors for APB1
> + * sun4i_get_apb1_factors() - calculates m, p factors for APB1
> * APB1 rate is calculated as follows
> * rate = (parent_rate >> p) / (m + 1);
> */
>
> -static void sunxi_get_apb1_factors(u32 *freq, u32 parent_rate,
> +static void sun4i_get_apb1_factors(u32 *freq, u32 parent_rate,
> u8 *n, u8 *k, u8 *m, u8 *p)
> {
> u8 calcm, calcp;
> @@ -178,7 +178,7 @@ struct factors_data {
> void (*getter) (u32 *rate, u32 parent_rate, u8 *n, u8 *k, u8 *m, u8 *p);
> };
>
> -static struct clk_factors_config pll1_config = {
> +static struct clk_factors_config sun4i_pll1_config = {
> .nshift = 8,
> .nwidth = 5,
> .kshift = 4,
> @@ -189,21 +189,21 @@ static struct clk_factors_config pll1_config = {
> .pwidth = 2,
> };
>
> -static struct clk_factors_config apb1_config = {
> +static struct clk_factors_config sun4i_apb1_config = {
> .mshift = 0,
> .mwidth = 5,
> .pshift = 16,
> .pwidth = 2,
> };
>
> -static const __initconst struct factors_data pll1_data = {
> - .table = &pll1_config,
> - .getter = sunxi_get_pll1_factors,
> +static const __initconst struct factors_data sun4i_pll1_data = {
> + .table = &sun4i_pll1_config,
> + .getter = sun4i_get_pll1_factors,
> };
>
> -static const __initconst struct factors_data apb1_data = {
> - .table = &apb1_config,
> - .getter = sunxi_get_apb1_factors,
> +static const __initconst struct factors_data sun4i_apb1_data = {
> + .table = &sun4i_apb1_config,
> + .getter = sun4i_get_apb1_factors,
> };
>
> static void __init sunxi_factors_clk_setup(struct device_node *node,
> @@ -239,11 +239,11 @@ struct mux_data {
> u8 shift;
> };
>
> -static const __initconst struct mux_data cpu_mux_data = {
> +static const __initconst struct mux_data sun4i_cpu_mux_data = {
> .shift = 16,
> };
>
> -static const __initconst struct mux_data apb1_mux_data = {
> +static const __initconst struct mux_data sun4i_apb1_mux_data = {
> .shift = 24,
> };
>
> @@ -284,17 +284,17 @@ struct div_data {
> u8 pow;
> };
>
> -static const __initconst struct div_data axi_data = {
> +static const __initconst struct div_data sun4i_axi_data = {
> .shift = 0,
> .pow = 0,
> };
>
> -static const __initconst struct div_data ahb_data = {
> +static const __initconst struct div_data sun4i_ahb_data = {
> .shift = 4,
> .pow = 1,
> };
>
> -static const __initconst struct div_data apb0_data = {
> +static const __initconst struct div_data sun4i_apb0_data = {
> .shift = 8,
> .pow = 1,
> };
> @@ -413,35 +413,77 @@ CLK_OF_DECLARE(sunxi_osc, "allwinner,sun4i-osc-clk", sunxi_osc_clk_setup);
>
> /* Matches for factors clocks */
> static const __initconst struct of_device_id clk_factors_match[] = {
> - {.compatible = "allwinner,sun4i-pll1-clk", .data = &pll1_data,},
> - {.compatible = "allwinner,sun4i-apb1-clk", .data = &apb1_data,},
> + {
> + .compatible = "allwinner,sun4i-pll1-clk",
> + .data = &sun4i_pll1_data,
> + },
> + {
> + .compatible = "allwinner,sun4i-apb1-clk",
> + .data = &sun4i_apb1_data,
> + },
> {}
> };
>
> /* Matches for divider clocks */
> static const __initconst struct of_device_id clk_div_match[] = {
> - {.compatible = "allwinner,sun4i-axi-clk", .data = &axi_data,},
> - {.compatible = "allwinner,sun4i-ahb-clk", .data = &ahb_data,},
> - {.compatible = "allwinner,sun4i-apb0-clk", .data = &apb0_data,},
> + {
> + .compatible = "allwinner,sun4i-axi-clk",
> + .data = &sun4i_axi_data,
> + },
> + {
> + .compatible = "allwinner,sun4i-ahb-clk",
> + .data = &sun4i_ahb_data,
> + },
> + {
> + .compatible = "allwinner,sun4i-apb0-clk",
> + .data = &sun4i_apb0_data,
> + },
> {}
> };
>
> /* Matches for mux clocks */
> static const __initconst struct of_device_id clk_mux_match[] = {
> - {.compatible = "allwinner,sun4i-cpu-clk", .data = &cpu_mux_data,},
> - {.compatible = "allwinner,sun4i-apb1-mux-clk", .data = &apb1_mux_data,},
> + {
> + .compatible = "allwinner,sun4i-cpu-clk",
> + .data = &sun4i_cpu_mux_data,
> + },
> + {
> + .compatible = "allwinner,sun4i-apb1-mux-clk",
> + .data = &sun4i_apb1_mux_data,
> + },
> {}
> };
>
> /* Matches for gate clocks */
> static const __initconst struct of_device_id clk_gates_match[] = {
> - {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &sun4i_axi_gates_data,},
> - {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &sun4i_ahb_gates_data,},
> - {.compatible = "allwinner,sun5i-a13-ahb-gates-clk", .data = &sun5i_a13_ahb_gates_data,},
> - {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &sun4i_apb0_gates_data,},
> - {.compatible = "allwinner,sun5i-a13-apb0-gates-clk", .data = &sun5i_a13_apb0_gates_data,},
> - {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &sun4i_apb1_gates_data,},
> - {.compatible = "allwinner,sun5i-a13-apb1-gates-clk", .data = &sun5i_a13_apb1_gates_data,},
> + {
> + .compatible = "allwinner,sun4i-axi-gates-clk",
> + .data = &sun4i_axi_gates_data,
> + },
> + {
> + .compatible = "allwinner,sun4i-ahb-gates-clk",
> + .data = &sun4i_ahb_gates_data,
> + },
> + {
> + .compatible = "allwinner,sun5i-a13-ahb-gates-clk",
> + .data = &sun5i_a13_ahb_gates_data,
> + },
> + {
> + .compatible = "allwinner,sun4i-apb0-gates-clk",
> + .data = &sun4i_apb0_gates_data,
> + },
> + {
> + .compatible = "allwinner,sun5i-a13-apb0-gates-clk",
> + .data = &sun5i_a13_apb0_gates_data,
> + },
> + {
> + .compatible = "allwinner,sun4i-apb1-gates-clk",
> + .data = &sun4i_apb1_gates_data,
> + },
> + {
> + .compatible = "allwinner,sun5i-a13-apb1-gates-clk",
> + .data = &sun5i_a13_apb1_gates_data,
> + },
> {}
> };
>

I'm not particularly a fan of this though; in my opinion it hurts
readability a bit and it uses 4x the lines. It looks like a highly
probable source of git conflicts too if not handled appropriately :)

Other than that,

Reviewed-by: Emilio L?pez <[email protected]>

Thanks!

Emilio

2013-07-31 00:27:46

by Emilio López

Subject: Re: [PATCH 2/4] clk: sunxi: Allow to specify the divider width from the dividers data

El 30/07/13 11:44, Maxime Ripard escribi?:
> The divider width used to be hardcoded. Some A31 dividers are no longer
> with the hardcoded width, so we need to make it specific to each divider
> and set it in the dividers data.
>
> Signed-off-by: Maxime Ripard <[email protected]>

Looks good to me,

Reviewed-by: Emilio L?pez <[email protected]>

> ---
> drivers/clk/sunxi/clk-sunxi.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index 3c91888..6e9cbc9 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -277,26 +277,28 @@ static void __init sunxi_mux_clk_setup(struct device_node *node,
> * sunxi_divider_clk_setup() - Setup function for simple divider clocks
> */
>
> -#define SUNXI_DIVISOR_WIDTH 2
> -
> struct div_data {
> - u8 shift;
> - u8 pow;
> + u8 shift;
> + u8 pow;
> + u8 width;
> };
>
> static const __initconst struct div_data sun4i_axi_data = {
> - .shift = 0,
> - .pow = 0,
> + .shift = 0,
> + .pow = 0,
> + .width = 2,
> };

Is there a style rule governing the use of tabs/spaces on structs? Maybe
we should do this alignment cleanup on the full file some time.

>
> static const __initconst struct div_data sun4i_ahb_data = {
> - .shift = 4,
> - .pow = 1,
> + .shift = 4,
> + .pow = 1,
> + .width = 2,
> };
>
> static const __initconst struct div_data sun4i_apb0_data = {
> - .shift = 8,
> - .pow = 1,
> + .shift = 8,
> + .pow = 1,
> + .width = 2,
> };
>
> static void __init sunxi_divider_clk_setup(struct device_node *node,
> @@ -312,7 +314,7 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
> clk_parent = of_clk_get_parent_name(node, 0);
>
> clk = clk_register_divider(NULL, clk_name, clk_parent, 0,
> - reg, data->shift, SUNXI_DIVISOR_WIDTH,
> + reg, data->shift, data->width,
> data->pow ? CLK_DIVIDER_POWER_OF_TWO : 0,
> &clk_lock);
> if (clk) {
>

Thanks!

Emilio

2013-07-31 01:01:40

by Emilio López

Subject: Re: [PATCH 3/4] clk: sunxi: Add A31 clocks support

Hi Maxime,

El 30/07/13 11:44, Maxime Ripard escribi?:
> The A31 has a mostly different clock set compared to the other older
> SoCs currently supported in the Allwinner clock driver.
>
> Add support for the basic useful clocks. The other ones will come in
> eventually.
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> drivers/clk/sunxi/clk-sunxi.c | 120 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 120 insertions(+)
>
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index 6e9cbc9..8cd69e6 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -124,7 +124,67 @@ static void sun4i_get_pll1_factors(u32 *freq, u32 parent_rate,
> *n = div / 4;
> }
>
> +/**
> + * sun6i_get_pll1_factors() - calculates n, k and m factors for PLL1
> + * PLL1 rate is calculated as follows
> + * rate = parent_rate * (n + 1) * (k + 1) / (m + 1);
> + * parent_rate should always be 24MHz
> + */
> +static void sun6i_get_pll1_factors(u32 *freq, u32 parent_rate,
> + u8 *n, u8 *k, u8 *m, u8 *p)
> +{
> + /*
> + * We can operate only on MHz, this will make our life easier
> + * later.
> + */
> + u32 freq_mhz = *freq / 1000000;
> + u32 parent_freq_mhz = parent_rate / 1000000;
> +
> + /* If the frequency is a multiple of 32 MHz, k is always 3 */
> + if (!(freq_mhz % 32))
> + *k = 3;
> + /* If the frequency is a multiple of 9 MHz, k is always 2 */
> + else if (!(freq_mhz % 9))
> + *k = 2;
> + /* If the frequency is a multiple of 8 MHz, k is always 1 */
> + else if (!(freq_mhz % 8))
> + *k = 1;
> + /* Otherwise, we don't use the k factor */
> + else
> + *k = 0;
>
> + /*
> + * If the frequency is a multiple of 2 but not a multiple of
> + * 3, m is 3. This is the first time we use 6 here, yet we
> + * will use it on several other places.
> + * We use this number because it's the lowest frequency we can
> + * generate (with n = 0, k = 0, m = 3), so every other frequency
> + * somehow relates to this frequency.
> + */
> + if ((freq_mhz % 6) == 2 || (freq_mhz % 6) == 4)
> + *m = 2;
> + /*
> + * If the frequency is a multiple of 6MHz, but the factor is
> + * odd, m will be 3
> + */
> + else if ((freq_mhz / 6) & 1)
> + *m = 3;
> + /* Otherwise, we end up with m = 1 */
> + else
> + *m = 1;
> +
> + /* Calculate n thanks to the above factors we already got */
> + *n = freq_mhz * (*m + 1) / ((*k + 1) * parent_freq_mhz) - 1;
> +
> + /*
> + * If n end up being outbound, and that we can still decrease
> + * m, do it.
> + */
> + if ((*n + 1) > 31 && (*m + 1) > 1) {
> + *n = (*n + 1) / 2 - 1;
> + *m = (*m + 1) / 2 - 1;
> + }
> +}

Nice! My rate-to-factors functions pale in comparison :) Remember that
n/k/m/p may be NULL when the caller just wants you to round freq to an
achievable value (see clk_factors_round_rate(...) in clk-factors.c).

> /**
> * sun4i_get_apb1_factors() - calculates m, p factors for APB1
> @@ -189,6 +249,15 @@ static struct clk_factors_config sun4i_pll1_config = {
> .pwidth = 2,
> };
>
> +static struct clk_factors_config sun6i_pll1_config = {
> + .nshift = 8,
> + .nwidth = 5,
> + .kshift = 4,
> + .kwidth = 2,
> + .mshift = 0,
> + .mwidth = 2,
> +};
> +
> static struct clk_factors_config sun4i_apb1_config = {
> .mshift = 0,
> .mwidth = 5,
> @@ -201,6 +270,11 @@ static const __initconst struct factors_data sun4i_pll1_data = {
> .getter = sun4i_get_pll1_factors,
> };
>
> +static const __initconst struct factors_data sun6i_pll1_data = {
> + .table = &sun6i_pll1_config,
> + .getter = sun6i_get_pll1_factors,
> +};
> +
> static const __initconst struct factors_data sun4i_apb1_data = {
> .table = &sun4i_apb1_config,
> .getter = sun4i_get_apb1_factors,
> @@ -243,6 +317,10 @@ static const __initconst struct mux_data sun4i_cpu_mux_data = {
> .shift = 16,
> };
>
> +static const __initconst struct mux_data sun6i_ahb1_mux_data = {
> + .shift = 12,
> +};
> +
> static const __initconst struct mux_data sun4i_apb1_mux_data = {
> .shift = 24,
> };
> @@ -301,6 +379,12 @@ static const __initconst struct div_data sun4i_apb0_data = {
> .width = 2,
> };
>
> +static const __initconst struct div_data sun6i_apb2_div_data = {
> + .shift = 0,
> + .pow = 0,
> + .width = 4,
> +};
> +
> static void __init sunxi_divider_clk_setup(struct device_node *node,
> struct div_data *data)
> {
> @@ -347,6 +431,10 @@ static const __initconst struct gates_data sun5i_a13_ahb_gates_data = {
> .mask = {0x107067e7, 0x185111},
> };
>
> +static const __initconst struct gates_data sun6i_a31_ahb1_gates_data = {
> + .mask = { 0xedfe7f62, 0x794f931 },
> +};
> +
> static const __initconst struct gates_data sun4i_apb0_gates_data = {
> .mask = {0x4EF},
> };
> @@ -363,6 +451,14 @@ static const __initconst struct gates_data sun5i_a13_apb1_gates_data = {
> .mask = {0xa0007},
> };
>
> +static const __initconst struct gates_data sun6i_a31_apb1_gates_data = {
> + .mask = { 0x3031 },
> +};
> +
> +static const __initconst struct gates_data sun6i_a31_apb2_gates_data = {
> + .mask = { 0x3f000f },
> +};
> +

We should decide on a style here and use it across the board, so far we
have definitions with and without spaces between constants and braces,
and constants both with upper and lower case A-F. Personally I don't
feel strongly about the spacing and prefer upper-case constants, but if
the style guide suggests otherwise, I can live with it (as long as we
use it consistently, that is)

> static void __init sunxi_gates_clk_setup(struct device_node *node,
> struct gates_data *data)
> {
> @@ -420,6 +516,10 @@ static const __initconst struct of_device_id clk_factors_match[] = {
> .data = &sun4i_pll1_data,
> },
> {
> + .compatible = "allwinner,sun6i-pll1-clk",
> + .data = &sun6i_pll1_data,
> + },
> + {
> .compatible = "allwinner,sun4i-apb1-clk",
> .data = &sun4i_apb1_data,
> },
> @@ -440,6 +540,10 @@ static const __initconst struct of_device_id clk_div_match[] = {
> .compatible = "allwinner,sun4i-apb0-clk",
> .data = &sun4i_apb0_data,
> },
> + {
> + .compatible = "allwinner,sun6i-apb2-div-clk",
> + .data = &sun6i_apb2_div_data,
> + },
> {}
> };
>
> @@ -453,6 +557,10 @@ static const __initconst struct of_device_id clk_mux_match[] = {
> .compatible = "allwinner,sun4i-apb1-mux-clk",
> .data = &sun4i_apb1_mux_data,
> },
> + {
> + .compatible = "allwinner,sun6i-ahb1-mux-clk",
> + .data = &sun6i_ahb1_mux_data,
> + },
> {}
> };
>
> @@ -471,6 +579,10 @@ static const __initconst struct of_device_id clk_gates_match[] = {
> .data = &sun5i_a13_ahb_gates_data,
> },
> {
> + .compatible = "allwinner,sun6i-a31-ahb1-gates-clk",
> + .data = &sun6i_a31_ahb1_gates_data,
> + },
> + {
> .compatible = "allwinner,sun4i-apb0-gates-clk",
> .data = &sun4i_apb0_gates_data,
> },
> @@ -486,6 +598,14 @@ static const __initconst struct of_device_id clk_gates_match[] = {
> .compatible = "allwinner,sun5i-a13-apb1-gates-clk",
> .data = &sun5i_a13_apb1_gates_data,
> },
> + {
> + .compatible = "allwinner,sun6i-a31-apb1-gates-clk",
> + .data = &sun6i_a31_apb1_gates_data,
> + },
> + {
> + .compatible = "allwinner,sun6i-a31-apb2-gates-clk",
> + .data = &sun6i_a31_apb2_gates_data,
> + },

Please see my comments on the first patch regarding these.

Thanks!

Emilio

2013-07-31 01:37:02

by Emilio López

Subject: Re: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI

Hi Maxime,

Overall this looks good to me, but I have some small comments:

El 30/07/13 11:44, Maxime Ripard escribi?:
> Now that the clock driver has support for the A31 clocks, we can add
> them to the DTSI and start using them in the relevant hardware blocks.
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> arch/arm/boot/dts/sun6i-a31.dtsi | 137 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 127 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 0d13dc6..c6a3a91 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -51,13 +51,130 @@
>
> clocks {
> #address-cells = <1>;
> - #size-cells = <0>;
> + #size-cells = <1>;
> + ranges;
>
> - osc: oscillator {
> + osc24M: hosc {

Please use osc24M and osc32k instead of hosc and losc, respectively.

> #clock-cells = <0>;
> compatible = "fixed-clock";
> clock-frequency = <24000000>;

Is osc24M not gatable on A31?

> };
> +
> + osc32k: losc {
> + #clock-cells = <0>;
> + compatible = "fixed-clock";
> + clock-frequency = <32768>;
> + };
> +
> + pll1: pll1@01c20000 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun6i-pll1-clk";
> + reg = <0x01c20000 0x4>;
> + clocks = <&osc24M>;
> + };
> +
> + /*
> + * This is a dummy clock, to be used as placeholder on
> + * other mux clocks when a specific parent clock is not
> + * yet implemented. It should be dropped when the driver
> + * is complete.
> + */
> + pll6: pll6 {
> + #clock-cells = <0>;
> + compatible = "fixed-clock";
> + clock-frequency = <0>;
> + };
> +
> + cpu: cpu@01c20050 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-cpu-clk";
> + reg = <0x01c20050 0x4>;
> + clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>;

Listing pll1 twice doesn't sound correct, but vendor code seems to
indicate so. A comment to clarify it's not a typo would be good I think.

> + };
> +
> + axi: axi@01c20050 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-axi-clk";
> + reg = <0x01c20050 0x4>;
> + clocks = <&cpu>;
> + };
> +
> + ahb1_mux: ahb1_mux@01c20054 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun6i-ahb1-mux-clk";
> + reg = <0x01c20054 0x4>;
> + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
> + };
> +
> + ahb1: ahb1@01c20054 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-ahb-clk";
> + reg = <0x01c20054 0x4>;
> + clocks = <&ahb1_mux>;
> + };

Depending on when this lands, I believe these two above could be merged
into one with the refactoring introduced on my patchset.

> +
> + ahb1_gates: ahb1_gates@01c20060 {
> + #clock-cells = <1>;
> + compatible = "allwinner,sun6i-a31-ahb1-gates-clk";
> + reg = <0x01c20060 0x8>;
> + clocks = <&ahb1>;
> + clock-output-names = "ahb1_mipidsi", "ahb1_ss",
> + "ahb1_dma", "ahb1_mmc0", "ahb1_mmc1",
> + "ahb1_mmc2", "ahb1_mmc3", "ahb1_nand1",
> + "ahb1_nand0", "ahb1_sdram",
> + "ahb1_gmac", "ahb1_ts", "ahb1_hstimer",
> + "ahb1_spi0", "ahb1_spi1", "ahb1_spi2",
> + "ahb1_spi3", "ahb1_otg", "ahb1_ehci0",
> + "ahb1_ehci1", "ahb1_ohci0",
> + "ahb1_ohci1", "ahb1_ohci2", "ahb1_ve",
> + "ahb1_lcd0", "ahb1_lcd1", "ahb1_csi",
> + "ahb1_hdmi", "ahb1_de0", "ahb1_de1",
> + "ahb1_fe0", "ahb1_fe1", "ahb1_mp",
> + "ahb1_gpu", "ahb1_deu0", "ahb1_deu1",
> + "ahb1_drc0", "ahb1_drc1";
> + };
> +
> + apb1: apb1@01c20054 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-apb0-clk";
> + reg = <0x01c20054 0x4>;
> + clocks = <&ahb1>;
> + };
> +
> + apb1_gates: apb1_gates@01c20060 {
> + #clock-cells = <1>;
> + compatible = "allwinner,sun6i-a31-apb1-gates-clk";
> + reg = <0x01c20068 0x4>;
> + clocks = <&apb1>;
> + clock-output-names = "apb1_codec", "apb1_digital_mic",
> + "apb1_pio", "apb1_daudio0",
> + "apb1_daudio1";
> + };
> +
> + apb2_mux: apb2_mux@01c20058 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-apb1-mux-clk";
> + reg = <0x01c20058 0x4>;
> + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>;
> + };
> +
> + apb2: apb2@01c20058 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun6i-apb2-div-clk";
> + reg = <0x01c20058 0x4>;
> + clocks = <&apb2_mux>;
> + };

Ditto for apb2_mux and apb2.

> +
> + apb2_gates: apb2_gates@01c2006c {
> + #clock-cells = <1>;
> + compatible = "allwinner,sun6i-a31-apb2-gates-clk";
> + reg = <0x01c2006c 0x8>;
> + clocks = <&apb2>;
> + clock-output-names = "apb2_i2c0", "apb2_i2c1",
> + "apb2_i2c2", "apb2_i2c3", "apb2_uart0",
> + "apb2_uart1", "apb2_uart2", "apb2_uart3",
> + "apb2_uart4", "apb2_uart5";
> + };
> };
>
> soc@01c20000 {
> @@ -69,7 +186,7 @@
> compatible = "allwinner,sun6i-a31-pinctrl";
> reg = <0x01c20800 0x400>;
> interrupts = <0 11 1>, <0 15 1>, <0 16 1>, <0 17 1>;
> - clocks = <&osc>;
> + clocks = <&apb1_gates 5>;
> gpio-controller;
> interrupt-controller;
> #address-cells = <1>;
> @@ -92,7 +209,7 @@
> <0 20 1>,
> <0 21 1>,
> <0 22 1>;
> - clocks = <&osc>;
> + clocks = <&osc24M>;
> };
>
> wdt1: watchdog@01c20ca0 {
> @@ -106,7 +223,7 @@
> interrupts = <0 0 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 16>;
> status = "disabled";
> };
>
> @@ -116,7 +233,7 @@
> interrupts = <0 1 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 17>;
> status = "disabled";
> };
>
> @@ -126,7 +243,7 @@
> interrupts = <0 2 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 18>;
> status = "disabled";
> };
>
> @@ -136,7 +253,7 @@
> interrupts = <0 3 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 19>;
> status = "disabled";
> };
>
> @@ -146,7 +263,7 @@
> interrupts = <0 4 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 20>;
> status = "disabled";
> };
>
> @@ -156,7 +273,7 @@
> interrupts = <0 5 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 21>;
> status = "disabled";
> };
>
>

Thanks for working on this!

Emilio

2013-07-31 07:37:31

by Maxime Ripard

Subject: Re: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI

Hi Emilio,

On Tue, Jul 30, 2013 at 10:36:46PM -0300, Emilio L?pez wrote:
> Hi Maxime,
>
> Overall this looks good to me, but I have some small comments:
>
> El 30/07/13 11:44, Maxime Ripard escribi?:
> > Now that the clock driver has support for the A31 clocks, we can add
> > them to the DTSI and start using them in the relevant hardware blocks.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > arch/arm/boot/dts/sun6i-a31.dtsi | 137 ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 127 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> > index 0d13dc6..c6a3a91 100644
> > --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> > +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> > @@ -51,13 +51,130 @@
> >
> > clocks {
> > #address-cells = <1>;
> > - #size-cells = <0>;
> > + #size-cells = <1>;
> > + ranges;
> >
> > - osc: oscillator {
> > + osc24M: hosc {
>
> Please use osc24M and osc32k instead of hosc and losc, respectively.

The problem is a bit more complex than that.

On the A31, the losc clock is actually a mux between an external
oscillator running at 32kHz, and the internal oscillator running at
667MHz, that would be scaled down.

Support for this mux is not quite there yet, since I've not seen any
documentation for it, but this would allow to just rearrange losc
parents and compatible when we will had such support.

Hence why I chose these names.

> > #clock-cells = <0>;
> > compatible = "fixed-clock";
> > clock-frequency = <24000000>;
>
> Is osc24M not gatable on A31?

Nope.

> > };
> > +
> > + osc32k: losc {
> > + #clock-cells = <0>;
> > + compatible = "fixed-clock";
> > + clock-frequency = <32768>;
> > + };
> > +
> > + pll1: pll1@01c20000 {
> > + #clock-cells = <0>;
> > + compatible = "allwinner,sun6i-pll1-clk";
> > + reg = <0x01c20000 0x4>;
> > + clocks = <&osc24M>;
> > + };
> > +
> > + /*
> > + * This is a dummy clock, to be used as placeholder on
> > + * other mux clocks when a specific parent clock is not
> > + * yet implemented. It should be dropped when the driver
> > + * is complete.
> > + */
> > + pll6: pll6 {
> > + #clock-cells = <0>;
> > + compatible = "fixed-clock";
> > + clock-frequency = <0>;
> > + };
> > +
> > + cpu: cpu@01c20050 {
> > + #clock-cells = <0>;
> > + compatible = "allwinner,sun4i-cpu-clk";
> > + reg = <0x01c20050 0x4>;
> > + clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>;
>
> Listing pll1 twice doesn't sound correct, but vendor code seems to
> indicate so. A comment to clarify it's not a typo would be good I think.

Yes, I suspect an error in the datasheet, but until proven otherwise,
it's that way.

I'll add a comment.

> > + };
> > +
> > + axi: axi@01c20050 {
> > + #clock-cells = <0>;
> > + compatible = "allwinner,sun4i-axi-clk";
> > + reg = <0x01c20050 0x4>;
> > + clocks = <&cpu>;
> > + };
> > +
> > + ahb1_mux: ahb1_mux@01c20054 {
> > + #clock-cells = <0>;
> > + compatible = "allwinner,sun6i-ahb1-mux-clk";
> > + reg = <0x01c20054 0x4>;
> > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
> > + };
> > +
> > + ahb1: ahb1@01c20054 {
> > + #clock-cells = <0>;
> > + compatible = "allwinner,sun4i-ahb-clk";
> > + reg = <0x01c20054 0x4>;
> > + clocks = <&ahb1_mux>;
> > + };
>
> Depending on when this lands, I believe these two above could be merged
> into one with the refactoring introduced on my patchset.

Since your patchset is still in RFC and we had no comments from Mike so
far, while this one looks pretty similar to the one we had before, I
guess the safest thing to do would be to rebase your patches on top of
this ones.

But it's right those clocks (AHB1 and APB2) will benefit from your work
as well :)

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

2013-07-31 09:20:49

by Maxime Ripard

Subject: Re: [PATCH 1/4] clk: sunxi: Rename the structure to prepare the addition of sun6i

Hi Emilio,

On Tue, Jul 30, 2013 at 09:14:14PM -0300, Emilio L?pez wrote:
> Hi Maxime,
>
> El 30/07/13 11:44, Maxime Ripard escribi?:
> > Rename all the generic-named structure to sun4i to avoid confusion when
> > we will introduce the sun6i (A31) clocks.
> >
> > While we're at it, avoid too long lines and wrap the DT compatibles
> > tables.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
>
> Overall the patch looks good :)
>
> > ---
> > drivers/clk/sunxi/clk-sunxi.c | 108 +++++++++++++++++++++++++++++-------------
> > 1 file changed, 75 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> > index fe1528e..3c91888 100644
> > --- a/drivers/clk/sunxi/clk-sunxi.c
> > +++ b/drivers/clk/sunxi/clk-sunxi.c
> > @@ -25,12 +25,12 @@
> > static DEFINE_SPINLOCK(clk_lock);
> >
> > /**
> > - * sunxi_osc_clk_setup() - Setup function for gatable oscillator
> > + * sun4i_osc_clk_setup() - Setup function for gatable oscillator
> > */
> >
> > #define SUNXI_OSC24M_GATE 0
> >
> > -static void __init sunxi_osc_clk_setup(struct device_node *node)
> > +static void __init sun4i_osc_clk_setup(struct device_node *node)
> > {
> > struct clk *clk;
> > struct clk_fixed_rate *fixed;
> > @@ -73,13 +73,13 @@ static void __init sunxi_osc_clk_setup(struct device_node *node)
> >
> >
> > /**
> > - * sunxi_get_pll1_factors() - calculates n, k, m, p factors for PLL1
> > + * sun4i_get_pll1_factors() - calculates n, k, m, p factors for PLL1
> > * PLL1 rate is calculated as follows
> > * rate = (parent_rate * n * (k + 1) >> p) / (m + 1);
> > * parent_rate is always 24Mhz
> > */
> >
> > -static void sunxi_get_pll1_factors(u32 *freq, u32 parent_rate,
> > +static void sun4i_get_pll1_factors(u32 *freq, u32 parent_rate,
> > u8 *n, u8 *k, u8 *m, u8 *p)
> > {
> > u8 div;
> > @@ -127,12 +127,12 @@ static void sunxi_get_pll1_factors(u32 *freq, u32 parent_rate,
> >
> >
> > /**
> > - * sunxi_get_apb1_factors() - calculates m, p factors for APB1
> > + * sun4i_get_apb1_factors() - calculates m, p factors for APB1
> > * APB1 rate is calculated as follows
> > * rate = (parent_rate >> p) / (m + 1);
> > */
> >
> > -static void sunxi_get_apb1_factors(u32 *freq, u32 parent_rate,
> > +static void sun4i_get_apb1_factors(u32 *freq, u32 parent_rate,
> > u8 *n, u8 *k, u8 *m, u8 *p)
> > {
> > u8 calcm, calcp;
> > @@ -178,7 +178,7 @@ struct factors_data {
> > void (*getter) (u32 *rate, u32 parent_rate, u8 *n, u8 *k, u8 *m, u8 *p);
> > };
> >
> > -static struct clk_factors_config pll1_config = {
> > +static struct clk_factors_config sun4i_pll1_config = {
> > .nshift = 8,
> > .nwidth = 5,
> > .kshift = 4,
> > @@ -189,21 +189,21 @@ static struct clk_factors_config pll1_config = {
> > .pwidth = 2,
> > };
> >
> > -static struct clk_factors_config apb1_config = {
> > +static struct clk_factors_config sun4i_apb1_config = {
> > .mshift = 0,
> > .mwidth = 5,
> > .pshift = 16,
> > .pwidth = 2,
> > };
> >
> > -static const __initconst struct factors_data pll1_data = {
> > - .table = &pll1_config,
> > - .getter = sunxi_get_pll1_factors,
> > +static const __initconst struct factors_data sun4i_pll1_data = {
> > + .table = &sun4i_pll1_config,
> > + .getter = sun4i_get_pll1_factors,
> > };
> >
> > -static const __initconst struct factors_data apb1_data = {
> > - .table = &apb1_config,
> > - .getter = sunxi_get_apb1_factors,
> > +static const __initconst struct factors_data sun4i_apb1_data = {
> > + .table = &sun4i_apb1_config,
> > + .getter = sun4i_get_apb1_factors,
> > };
> >
> > static void __init sunxi_factors_clk_setup(struct device_node *node,
> > @@ -239,11 +239,11 @@ struct mux_data {
> > u8 shift;
> > };
> >
> > -static const __initconst struct mux_data cpu_mux_data = {
> > +static const __initconst struct mux_data sun4i_cpu_mux_data = {
> > .shift = 16,
> > };
> >
> > -static const __initconst struct mux_data apb1_mux_data = {
> > +static const __initconst struct mux_data sun4i_apb1_mux_data = {
> > .shift = 24,
> > };
> >
> > @@ -284,17 +284,17 @@ struct div_data {
> > u8 pow;
> > };
> >
> > -static const __initconst struct div_data axi_data = {
> > +static const __initconst struct div_data sun4i_axi_data = {
> > .shift = 0,
> > .pow = 0,
> > };
> >
> > -static const __initconst struct div_data ahb_data = {
> > +static const __initconst struct div_data sun4i_ahb_data = {
> > .shift = 4,
> > .pow = 1,
> > };
> >
> > -static const __initconst struct div_data apb0_data = {
> > +static const __initconst struct div_data sun4i_apb0_data = {
> > .shift = 8,
> > .pow = 1,
> > };
> > @@ -413,35 +413,77 @@ CLK_OF_DECLARE(sunxi_osc, "allwinner,sun4i-osc-clk", sunxi_osc_clk_setup);
> >
> > /* Matches for factors clocks */
> > static const __initconst struct of_device_id clk_factors_match[] = {
> > - {.compatible = "allwinner,sun4i-pll1-clk", .data = &pll1_data,},
> > - {.compatible = "allwinner,sun4i-apb1-clk", .data = &apb1_data,},
> > + {
> > + .compatible = "allwinner,sun4i-pll1-clk",
> > + .data = &sun4i_pll1_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun4i-apb1-clk",
> > + .data = &sun4i_apb1_data,
> > + },
> > {}
> > };
> >
> > /* Matches for divider clocks */
> > static const __initconst struct of_device_id clk_div_match[] = {
> > - {.compatible = "allwinner,sun4i-axi-clk", .data = &axi_data,},
> > - {.compatible = "allwinner,sun4i-ahb-clk", .data = &ahb_data,},
> > - {.compatible = "allwinner,sun4i-apb0-clk", .data = &apb0_data,},
> > + {
> > + .compatible = "allwinner,sun4i-axi-clk",
> > + .data = &sun4i_axi_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun4i-ahb-clk",
> > + .data = &sun4i_ahb_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun4i-apb0-clk",
> > + .data = &sun4i_apb0_data,
> > + },
> > {}
> > };
> >
> > /* Matches for mux clocks */
> > static const __initconst struct of_device_id clk_mux_match[] = {
> > - {.compatible = "allwinner,sun4i-cpu-clk", .data = &cpu_mux_data,},
> > - {.compatible = "allwinner,sun4i-apb1-mux-clk", .data = &apb1_mux_data,},
> > + {
> > + .compatible = "allwinner,sun4i-cpu-clk",
> > + .data = &sun4i_cpu_mux_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun4i-apb1-mux-clk",
> > + .data = &sun4i_apb1_mux_data,
> > + },
> > {}
> > };
> >
> > /* Matches for gate clocks */
> > static const __initconst struct of_device_id clk_gates_match[] = {
> > - {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &sun4i_axi_gates_data,},
> > - {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &sun4i_ahb_gates_data,},
> > - {.compatible = "allwinner,sun5i-a13-ahb-gates-clk", .data = &sun5i_a13_ahb_gates_data,},
> > - {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &sun4i_apb0_gates_data,},
> > - {.compatible = "allwinner,sun5i-a13-apb0-gates-clk", .data = &sun5i_a13_apb0_gates_data,},
> > - {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &sun4i_apb1_gates_data,},
> > - {.compatible = "allwinner,sun5i-a13-apb1-gates-clk", .data = &sun5i_a13_apb1_gates_data,},
> > + {
> > + .compatible = "allwinner,sun4i-axi-gates-clk",
> > + .data = &sun4i_axi_gates_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun4i-ahb-gates-clk",
> > + .data = &sun4i_ahb_gates_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun5i-a13-ahb-gates-clk",
> > + .data = &sun5i_a13_ahb_gates_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun4i-apb0-gates-clk",
> > + .data = &sun4i_apb0_gates_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun5i-a13-apb0-gates-clk",
> > + .data = &sun5i_a13_apb0_gates_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun4i-apb1-gates-clk",
> > + .data = &sun4i_apb1_gates_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun5i-a13-apb1-gates-clk",
> > + .data = &sun5i_a13_apb1_gates_data,
> > + },
> > {}
> > };
> >
>
> I'm not particularly a fan of this though; in my opinion it hurts
> readability a bit and it uses 4x the lines. It looks like a highly
> probable source of git conflicts too if not handled appropriately :)

Yeah, I'll drop it just like for pinctrl then.

>
> Other than that,
>
> Reviewed-by: Emilio L?pez <[email protected]>

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

2013-07-31 10:14:39

by Maxime Ripard

Subject: Re: [PATCH 3/4] clk: sunxi: Add A31 clocks support

Hi Emilio,

On Tue, Jul 30, 2013 at 10:01:25PM -0300, Emilio L?pez wrote:
> Hi Maxime,
>
> El 30/07/13 11:44, Maxime Ripard escribi?:
> > The A31 has a mostly different clock set compared to the other older
> > SoCs currently supported in the Allwinner clock driver.
> >
> > Add support for the basic useful clocks. The other ones will come in
> > eventually.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > drivers/clk/sunxi/clk-sunxi.c | 120 ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 120 insertions(+)
> >
> > diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> > index 6e9cbc9..8cd69e6 100644
> > --- a/drivers/clk/sunxi/clk-sunxi.c
> > +++ b/drivers/clk/sunxi/clk-sunxi.c
> > @@ -124,7 +124,67 @@ static void sun4i_get_pll1_factors(u32 *freq, u32 parent_rate,
> > *n = div / 4;
> > }
> >
> > +/**
> > + * sun6i_get_pll1_factors() - calculates n, k and m factors for PLL1
> > + * PLL1 rate is calculated as follows
> > + * rate = parent_rate * (n + 1) * (k + 1) / (m + 1);
> > + * parent_rate should always be 24MHz
> > + */
> > +static void sun6i_get_pll1_factors(u32 *freq, u32 parent_rate,
> > + u8 *n, u8 *k, u8 *m, u8 *p)
> > +{
> > + /*
> > + * We can operate only on MHz, this will make our life easier
> > + * later.
> > + */
> > + u32 freq_mhz = *freq / 1000000;
> > + u32 parent_freq_mhz = parent_rate / 1000000;
> > +
> > + /* If the frequency is a multiple of 32 MHz, k is always 3 */
> > + if (!(freq_mhz % 32))
> > + *k = 3;
> > + /* If the frequency is a multiple of 9 MHz, k is always 2 */
> > + else if (!(freq_mhz % 9))
> > + *k = 2;
> > + /* If the frequency is a multiple of 8 MHz, k is always 1 */
> > + else if (!(freq_mhz % 8))
> > + *k = 1;
> > + /* Otherwise, we don't use the k factor */
> > + else
> > + *k = 0;
> >
> > + /*
> > + * If the frequency is a multiple of 2 but not a multiple of
> > + * 3, m is 3. This is the first time we use 6 here, yet we
> > + * will use it on several other places.
> > + * We use this number because it's the lowest frequency we can
> > + * generate (with n = 0, k = 0, m = 3), so every other frequency
> > + * somehow relates to this frequency.
> > + */
> > + if ((freq_mhz % 6) == 2 || (freq_mhz % 6) == 4)
> > + *m = 2;
> > + /*
> > + * If the frequency is a multiple of 6MHz, but the factor is
> > + * odd, m will be 3
> > + */
> > + else if ((freq_mhz / 6) & 1)
> > + *m = 3;
> > + /* Otherwise, we end up with m = 1 */
> > + else
> > + *m = 1;
> > +
> > + /* Calculate n thanks to the above factors we already got */
> > + *n = freq_mhz * (*m + 1) / ((*k + 1) * parent_freq_mhz) - 1;
> > +
> > + /*
> > + * If n end up being outbound, and that we can still decrease
> > + * m, do it.
> > + */
> > + if ((*n + 1) > 31 && (*m + 1) > 1) {
> > + *n = (*n + 1) / 2 - 1;
> > + *m = (*m + 1) / 2 - 1;
> > + }
> > +}
>
> Nice! My rate-to-factors functions pale in comparison :) Remember that
> n/k/m/p may be NULL when the caller just wants you to round freq to an
> achievable value (see clk_factors_round_rate(...) in clk-factors.c).

Ah, right, I forgot that usecase.

> > /**
> > * sun4i_get_apb1_factors() - calculates m, p factors for APB1
> > @@ -189,6 +249,15 @@ static struct clk_factors_config sun4i_pll1_config = {
> > .pwidth = 2,
> > };
> >
> > +static struct clk_factors_config sun6i_pll1_config = {
> > + .nshift = 8,
> > + .nwidth = 5,
> > + .kshift = 4,
> > + .kwidth = 2,
> > + .mshift = 0,
> > + .mwidth = 2,
> > +};
> > +
> > static struct clk_factors_config sun4i_apb1_config = {
> > .mshift = 0,
> > .mwidth = 5,
> > @@ -201,6 +270,11 @@ static const __initconst struct factors_data sun4i_pll1_data = {
> > .getter = sun4i_get_pll1_factors,
> > };
> >
> > +static const __initconst struct factors_data sun6i_pll1_data = {
> > + .table = &sun6i_pll1_config,
> > + .getter = sun6i_get_pll1_factors,
> > +};
> > +
> > static const __initconst struct factors_data sun4i_apb1_data = {
> > .table = &sun4i_apb1_config,
> > .getter = sun4i_get_apb1_factors,
> > @@ -243,6 +317,10 @@ static const __initconst struct mux_data sun4i_cpu_mux_data = {
> > .shift = 16,
> > };
> >
> > +static const __initconst struct mux_data sun6i_ahb1_mux_data = {
> > + .shift = 12,
> > +};
> > +
> > static const __initconst struct mux_data sun4i_apb1_mux_data = {
> > .shift = 24,
> > };
> > @@ -301,6 +379,12 @@ static const __initconst struct div_data sun4i_apb0_data = {
> > .width = 2,
> > };
> >
> > +static const __initconst struct div_data sun6i_apb2_div_data = {
> > + .shift = 0,
> > + .pow = 0,
> > + .width = 4,
> > +};
> > +
> > static void __init sunxi_divider_clk_setup(struct device_node *node,
> > struct div_data *data)
> > {
> > @@ -347,6 +431,10 @@ static const __initconst struct gates_data sun5i_a13_ahb_gates_data = {
> > .mask = {0x107067e7, 0x185111},
> > };
> >
> > +static const __initconst struct gates_data sun6i_a31_ahb1_gates_data = {
> > + .mask = { 0xedfe7f62, 0x794f931 },
> > +};
> > +
> > static const __initconst struct gates_data sun4i_apb0_gates_data = {
> > .mask = {0x4EF},
> > };
> > @@ -363,6 +451,14 @@ static const __initconst struct gates_data sun5i_a13_apb1_gates_data = {
> > .mask = {0xa0007},
> > };
> >
> > +static const __initconst struct gates_data sun6i_a31_apb1_gates_data = {
> > + .mask = { 0x3031 },
> > +};
> > +
> > +static const __initconst struct gates_data sun6i_a31_apb2_gates_data = {
> > + .mask = { 0x3f000f },
> > +};
> > +
>
> We should decide on a style here and use it across the board, so far we
> have definitions with and without spaces between constants and braces,
> and constants both with upper and lower case A-F. Personally I don't
> feel strongly about the spacing and prefer upper-case constants, but if
> the style guide suggests otherwise, I can live with it (as long as we
> use it consistently, that is)

Hmmm, like you probably noticed, I prefer the { 0xdeadbeef } way, and
that's what is used everywhere else for sunxi I believe. There's no
strong convention on this one, but it's true that we have to remain
consistent.

I'll stick with your convention already in use in the clocks driver for
the time being, and we'll see later to unify the drivers on this
convention later on.

> > static void __init sunxi_gates_clk_setup(struct device_node *node,
> > struct gates_data *data)
> > {
> > @@ -420,6 +516,10 @@ static const __initconst struct of_device_id clk_factors_match[] = {
> > .data = &sun4i_pll1_data,
> > },
> > {
> > + .compatible = "allwinner,sun6i-pll1-clk",
> > + .data = &sun6i_pll1_data,
> > + },
> > + {
> > .compatible = "allwinner,sun4i-apb1-clk",
> > .data = &sun4i_apb1_data,
> > },
> > @@ -440,6 +540,10 @@ static const __initconst struct of_device_id clk_div_match[] = {
> > .compatible = "allwinner,sun4i-apb0-clk",
> > .data = &sun4i_apb0_data,
> > },
> > + {
> > + .compatible = "allwinner,sun6i-apb2-div-clk",
> > + .data = &sun6i_apb2_div_data,
> > + },
> > {}
> > };
> >
> > @@ -453,6 +557,10 @@ static const __initconst struct of_device_id clk_mux_match[] = {
> > .compatible = "allwinner,sun4i-apb1-mux-clk",
> > .data = &sun4i_apb1_mux_data,
> > },
> > + {
> > + .compatible = "allwinner,sun6i-ahb1-mux-clk",
> > + .data = &sun6i_ahb1_mux_data,
> > + },
> > {}
> > };
> >
> > @@ -471,6 +579,10 @@ static const __initconst struct of_device_id clk_gates_match[] = {
> > .data = &sun5i_a13_ahb_gates_data,
> > },
> > {
> > + .compatible = "allwinner,sun6i-a31-ahb1-gates-clk",
> > + .data = &sun6i_a31_ahb1_gates_data,
> > + },
> > + {
> > .compatible = "allwinner,sun4i-apb0-gates-clk",
> > .data = &sun4i_apb0_gates_data,
> > },
> > @@ -486,6 +598,14 @@ static const __initconst struct of_device_id clk_gates_match[] = {
> > .compatible = "allwinner,sun5i-a13-apb1-gates-clk",
> > .data = &sun5i_a13_apb1_gates_data,
> > },
> > + {
> > + .compatible = "allwinner,sun6i-a31-apb1-gates-clk",
> > + .data = &sun6i_a31_apb1_gates_data,
> > + },
> > + {
> > + .compatible = "allwinner,sun6i-a31-apb2-gates-clk",
> > + .data = &sun6i_a31_apb2_gates_data,
> > + },
>
> Please see my comments on the first patch regarding these.

Yes, that will be fixed.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

2013-07-31 11:38:18

by Emilio López

Subject: Re: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI

Hi,

El 31/07/13 05:11, kevin.z.m escribió:
> Hi, Maxime,
>
>> The problem is a bit more complex than that.
>
>> On the A31, the losc clock is actually a mux between an external
>> oscillator running at 32kHz, and the internal oscillator running at
>> 667MHz, that would be scaled down.
>
>> Support for this mux is not quite there yet, since I've not seen any
>> documentation for it, but this would allow to just rearrange losc
>> parents and compatible when we will had such support.
>
> I think there is some misunderstanding. All allwinner's platforms have
> 2 losc clock source. One is the external 32KHz oscillator, and the other
> is internal 32kHz R/C circuit. The internal 32k R/C circuit can't provide
> a very exact clock on 32KHz. The mux is just for select external osc or
> internal
> 32KHz R/C. If there is no external 32kHz oscillator, we can replace it with
> internal 32kHz R/C for some low speed but not exact clock requirement.

Thanks for the clarification :)

(snip)

>> > + ahb1_mux: ahb1_mux@01c20054 {
>> > + #clock-cells = <0>;
>> > + compatible = "allwinner,sun6i-ahb1-mux-clk";
>> > + reg = <0x01c20054 0x4>;
>> > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
>> > + };
>> > +
>> > + ahb1: ahb1@01c20054 {
>> > + #clock-cells = <0>;
>> > + compatible = "allwinner,sun4i-ahb-clk";
>> > + reg = <0x01c20054 0x4>;
>> > + clocks = <&ahb1_mux>;
>> > + };
>>
>> Depending on when this lands, I believe these two above could be merged
>> into one with the refactoring introduced on my patchset.
>
> Since your patchset is still in RFC and we had no comments from Mike so
> far, while this one looks pretty similar to the one we had before, I
> guess the safest thing to do would be to rebase your patches on top of
> this ones.

Ok, I'll do that.

Cheers,

Emilio

2013-07-31 11:50:04

by Maxime Ripard

Subject: Re: Re: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI

Hi Kevin,

On Wed, Jul 31, 2013 at 04:11:11PM +0800, kevin.z.m wrote:
> Hi, Maxime,
>
> > The problem is a bit more complex than that.
>
> > On the A31, the losc clock is actually a mux between an external
> > oscillator running at 32kHz, and the internal oscillator running at
> > 667MHz, that would be scaled down.
>
> > Support for this mux is not quite there yet, since I've not seen any
> > documentation for it, but this would allow to just rearrange losc
> > parents and compatible when we will had such support.
>
> I think there is some misunderstanding. All allwinner's platforms have
> 2 losc clock source. One is the external 32KHz oscillator, and the other
> is internal 32kHz R/C circuit. The internal 32k R/C circuit can't provide
> a very exact clock on 32KHz. The mux is just for select external osc or internal
> 32KHz R/C. If there is no external 32kHz oscillator, we can replace it with
> internal 32kHz R/C for some low speed but not exact clock requirement.

Thanks for the clarification.

How does one select either the internal or the external 32kHz
oscillator? Is this done in software (I can't find any reference to that
in the user manuals) or in hardware?

Thanks,
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

2013-07-31 15:49:59

by Maxime Ripard

Subject: Re: Re: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI

On Wed, Jul 31, 2013 at 08:10:32PM +0800, kevin.z.m wrote:
> > How does one select either the internal or the external 32kHz
> > oscillator? Is this done in software (I can't find any reference to that
> > in the user manuals) or in hardware?
> For A20, it should be in the Timer module, and there is a register named "LOSC_CTRL_REG".
> For A31, it is in the RTC module, same name of "LOSC_CTRL_REG".

Ok, just so that we're on the same page, we have two oscillators running
at 32kHz, the internal being always there, and the external that could
or could not be there, and it's each different hardware block that can
be wired to one, the other or both?

There's no global mux that could use either one of the two 32k to
provide the only 32kHz source in the system, right?

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

2013-08-01 09:53:18

by Maxime Ripard

Subject: Re: Re: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI

Hi Kevin,

On Thu, Aug 01, 2013 at 08:34:35AM +0800, kevin.z.m wrote:
> > Ok, just so that we're on the same page, we have two oscillators running
> > at 32kHz, the internal being always there, and the external that could
> > or could not be there, and it's each different hardware block that can
> > be wired to one, the other or both?
> The selection of internal or external is a global setting for system.
> Hardware blocks （such as cpu、twi for ex.) just see a 32kHz clock
> source, but they don't care where the 32kHz clock
> from.

Ok.

> > There's no global mux that could use either one of the two 32k to
> > provide the only 32kHz source in the system, right?
> There is a global mux to select internal or external, but no mux for
> every hardware blocks. I think that the mux need not be managed in the
> clock driver. It should be set earlier in the boot loader.
> And the selection is keeping either system is power-on or power-off,
> when there is a power supply with battery or DC. Because the mux is in
> RTC power domain.

Yes, in that case, we mostly don't care for now. Maybe we will at some
point if we need to add power management features, but we'd better
consider a fixed-rate clock for now and not worry too much about it.

Thanks for your help!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

2013-08-12 12:54:35

by Mark Rutland

Subject: Re: [PATCH 3/4] clk: sunxi: Add A31 clocks support

On Tue, Jul 30, 2013 at 03:44:21PM +0100, Maxime Ripard wrote:
> The A31 has a mostly different clock set compared to the other older
> SoCs currently supported in the Allwinner clock driver.
>
> Add support for the basic useful clocks. The other ones will come in
> eventually.
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> drivers/clk/sunxi/clk-sunxi.c | 120 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 120 insertions(+)
>

[...]

> @@ -420,6 +516,10 @@ static const __initconst struct of_device_id clk_factors_match[] = {
> .data = &sun4i_pll1_data,
> },
> {
> + .compatible = "allwinner,sun6i-pll1-clk",
> + .data = &sun6i_pll1_data,
> + },
> + {
> .compatible = "allwinner,sun4i-apb1-clk",
> .data = &sun4i_apb1_data,
> },
> @@ -440,6 +540,10 @@ static const __initconst struct of_device_id clk_div_match[] = {
> .compatible = "allwinner,sun4i-apb0-clk",
> .data = &sun4i_apb0_data,
> },
> + {
> + .compatible = "allwinner,sun6i-apb2-div-clk",
> + .data = &sun6i_apb2_div_data,
> + },
> {}
> };
>
> @@ -453,6 +557,10 @@ static const __initconst struct of_device_id clk_mux_match[] = {
> .compatible = "allwinner,sun4i-apb1-mux-clk",
> .data = &sun4i_apb1_mux_data,
> },
> + {
> + .compatible = "allwinner,sun6i-ahb1-mux-clk",
> + .data = &sun6i_ahb1_mux_data,
> + },
> {}
> };
>
> @@ -471,6 +579,10 @@ static const __initconst struct of_device_id clk_gates_match[] = {
> .data = &sun5i_a13_ahb_gates_data,
> },
> {
> + .compatible = "allwinner,sun6i-a31-ahb1-gates-clk",
> + .data = &sun6i_a31_ahb1_gates_data,
> + },
> + {
> .compatible = "allwinner,sun4i-apb0-gates-clk",
> .data = &sun4i_apb0_gates_data,
> },
> @@ -486,6 +598,14 @@ static const __initconst struct of_device_id clk_gates_match[] = {
> .compatible = "allwinner,sun5i-a13-apb1-gates-clk",
> .data = &sun5i_a13_apb1_gates_data,
> },
> + {
> + .compatible = "allwinner,sun6i-a31-apb1-gates-clk",
> + .data = &sun6i_a31_apb1_gates_data,
> + },
> + {
> + .compatible = "allwinner,sun6i-a31-apb2-gates-clk",
> + .data = &sun6i_a31_apb2_gates_data,
> + },
> {}
> };

Could you please document these new strings? I assume they follow the
general conventions of sunxi clocks thus far and the strings can just be
appended to the lists in the existing binding document.

Thanks,
Mark.

2013-08-12 13:02:22

by Emilio López

Subject: Re: [PATCH 3/4] clk: sunxi: Add A31 clocks support

El 12/08/13 09:53, Mark Rutland escribi?:
> On Tue, Jul 30, 2013 at 03:44:21PM +0100, Maxime Ripard wrote:
>> The A31 has a mostly different clock set compared to the other older
>> SoCs currently supported in the Allwinner clock driver.
>>
>> Add support for the basic useful clocks. The other ones will come in
>> eventually.
>>
>> Signed-off-by: Maxime Ripard <[email protected]>
>> ---
>> drivers/clk/sunxi/clk-sunxi.c | 120 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 120 insertions(+)
>>
>
> [...]
>
>> @@ -420,6 +516,10 @@ static const __initconst struct of_device_id clk_factors_match[] = {
>> .data = &sun4i_pll1_data,
>> },
>> {
>> + .compatible = "allwinner,sun6i-pll1-clk",
>> + .data = &sun6i_pll1_data,
>> + },
>> + {
>> .compatible = "allwinner,sun4i-apb1-clk",
>> .data = &sun4i_apb1_data,
>> },
>> @@ -440,6 +540,10 @@ static const __initconst struct of_device_id clk_div_match[] = {
>> .compatible = "allwinner,sun4i-apb0-clk",
>> .data = &sun4i_apb0_data,
>> },
>> + {
>> + .compatible = "allwinner,sun6i-apb2-div-clk",
>> + .data = &sun6i_apb2_div_data,
>> + },
>> {}
>> };
>>
>> @@ -453,6 +557,10 @@ static const __initconst struct of_device_id clk_mux_match[] = {
>> .compatible = "allwinner,sun4i-apb1-mux-clk",
>> .data = &sun4i_apb1_mux_data,
>> },
>> + {
>> + .compatible = "allwinner,sun6i-ahb1-mux-clk",
>> + .data = &sun6i_ahb1_mux_data,
>> + },
>> {}
>> };
>>
>> @@ -471,6 +579,10 @@ static const __initconst struct of_device_id clk_gates_match[] = {
>> .data = &sun5i_a13_ahb_gates_data,
>> },
>> {
>> + .compatible = "allwinner,sun6i-a31-ahb1-gates-clk",
>> + .data = &sun6i_a31_ahb1_gates_data,
>> + },
>> + {
>> .compatible = "allwinner,sun4i-apb0-gates-clk",
>> .data = &sun4i_apb0_gates_data,
>> },
>> @@ -486,6 +598,14 @@ static const __initconst struct of_device_id clk_gates_match[] = {
>> .compatible = "allwinner,sun5i-a13-apb1-gates-clk",
>> .data = &sun5i_a13_apb1_gates_data,
>> },
>> + {
>> + .compatible = "allwinner,sun6i-a31-apb1-gates-clk",
>> + .data = &sun6i_a31_apb1_gates_data,
>> + },
>> + {
>> + .compatible = "allwinner,sun6i-a31-apb2-gates-clk",
>> + .data = &sun6i_a31_apb2_gates_data,
>> + },
>> {}
>> };
>
> Could you please document these new strings? I assume they follow the
> general conventions of sunxi clocks thus far and the strings can just be
> appended to the lists in the existing binding document.

I believe they have been documented already on the latest version of
this patchset (v3 at the time of speaking)

Cheers,

Emilio

2013-08-12 13:55:06

by Mark Rutland

Subject: Re: [PATCH 3/4] clk: sunxi: Add A31 clocks support

On Mon, Aug 12, 2013 at 02:01:43PM +0100, Emilio López wrote:
> El 12/08/13 09:53, Mark Rutland escribió:
> > On Tue, Jul 30, 2013 at 03:44:21PM +0100, Maxime Ripard wrote:
> >> The A31 has a mostly different clock set compared to the other older
> >> SoCs currently supported in the Allwinner clock driver.
> >>
> >> Add support for the basic useful clocks. The other ones will come in
> >> eventually.
> >>
> >> Signed-off-by: Maxime Ripard <[email protected]>
> >> ---
> >> drivers/clk/sunxi/clk-sunxi.c | 120 ++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 120 insertions(+)
> >>
> >
> > [...]
> >
> >> @@ -420,6 +516,10 @@ static const __initconst struct of_device_id clk_factors_match[] = {
> >> .data = &sun4i_pll1_data,
> >> },
> >> {
> >> + .compatible = "allwinner,sun6i-pll1-clk",
> >> + .data = &sun6i_pll1_data,
> >> + },
> >> + {
> >> .compatible = "allwinner,sun4i-apb1-clk",
> >> .data = &sun4i_apb1_data,
> >> },
> >> @@ -440,6 +540,10 @@ static const __initconst struct of_device_id clk_div_match[] = {
> >> .compatible = "allwinner,sun4i-apb0-clk",
> >> .data = &sun4i_apb0_data,
> >> },
> >> + {
> >> + .compatible = "allwinner,sun6i-apb2-div-clk",
> >> + .data = &sun6i_apb2_div_data,
> >> + },
> >> {}
> >> };
> >>
> >> @@ -453,6 +557,10 @@ static const __initconst struct of_device_id clk_mux_match[] = {
> >> .compatible = "allwinner,sun4i-apb1-mux-clk",
> >> .data = &sun4i_apb1_mux_data,
> >> },
> >> + {
> >> + .compatible = "allwinner,sun6i-ahb1-mux-clk",
> >> + .data = &sun6i_ahb1_mux_data,
> >> + },
> >> {}
> >> };
> >>
> >> @@ -471,6 +579,10 @@ static const __initconst struct of_device_id clk_gates_match[] = {
> >> .data = &sun5i_a13_ahb_gates_data,
> >> },
> >> {
> >> + .compatible = "allwinner,sun6i-a31-ahb1-gates-clk",
> >> + .data = &sun6i_a31_ahb1_gates_data,
> >> + },
> >> + {
> >> .compatible = "allwinner,sun4i-apb0-gates-clk",
> >> .data = &sun4i_apb0_gates_data,
> >> },
> >> @@ -486,6 +598,14 @@ static const __initconst struct of_device_id clk_gates_match[] = {
> >> .compatible = "allwinner,sun5i-a13-apb1-gates-clk",
> >> .data = &sun5i_a13_apb1_gates_data,
> >> },
> >> + {
> >> + .compatible = "allwinner,sun6i-a31-apb1-gates-clk",
> >> + .data = &sun6i_a31_apb1_gates_data,
> >> + },
> >> + {
> >> + .compatible = "allwinner,sun6i-a31-apb2-gates-clk",
> >> + .data = &sun6i_a31_apb2_gates_data,
> >> + },
> >> {}
> >> };
> >
> > Could you please document these new strings? I assume they follow the
> > general conventions of sunxi clocks thus far and the strings can just be
> > appended to the lists in the existing binding document.
>
> I believe they have been documented already on the latest version of
> this patchset (v3 at the time of speaking)

Indeed they are, and they look fine. Apologies for the noise.

Thanks,
Mark.