Re: [PATCH 04/19] clk: sunxi: Add TCON channel1 clock
On Mon, Nov 09, 2015 at 11:36:15AM +0800, Chen-Yu Tsai wrote: > >> > + sclk1_parents[0] = sclk2_name; > >> > + sclk1_parents[1] = sclk2d2_name; > >> > >> Is there any need to expose these 2 clocks via DT using > >> of_clk_add_provider? > > > > No, as far as I'm aware, there's no user external to this clock > > driver. > > > >> Note that these complex clock trees within a clock node breaks the > >> assigned-clock-parents mechanism, as you can no longer specify the output > >> clock's direct parents. > > > > There's no point of changing the parent either. Hardware blocks are > > always connected to the leaf clock (sclk1). We could also model it as > > an extra 1-bit divider, which would simplify a bit the logic though. > > Probably not. You still have a gate to handle. It's just moving the > divider from 1 clock to the other. I think the current approach of > modeling it like the hardware is better. Not really if you model it using sclk2 being a mux + gate, and sclk1 being a divider + gate. It works great using the composite clocks. > About reparenting, what I meant was if sclk2 is not exposed through > of_clk_add_provider, then we can't do assigned-clocks stuff on it, > like setting a default parent or making each channel use a different > source pll. And we don't really want to. Using the divider allow us to simply set the rate of sclk1, and the mux / divider will do the rest. Since only sclk1 is exposed to the rest of the system, we do not really care about the rate of sclk2 anyway. > What I'm saying is if it is not expected to work with another core > binding, we should probably note it somewhere. Indeed. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 04/19] clk: sunxi: Add TCON channel1 clock
On Sat, Nov 7, 2015 at 8:01 AM, Maxime Ripard wrote: > Hi, > > On Sat, Oct 31, 2015 at 05:53:26PM +0800, Chen-Yu Tsai wrote: >> On Fri, Oct 30, 2015 at 10:20 PM, Maxime Ripard >> wrote: >> > The TCON is a controller generating the timings to output videos signals, >> > acting like both a CRTC and an encoder. >> > >> > It has two channels depending on the output, each channel being driven by >> > its own clock (and own clock controller). >> > >> > Add a driver for the channel 1 clock. >> > >> > Signed-off-by: Maxime Ripard >> > --- >> > drivers/clk/sunxi/Makefile | 1 + >> > drivers/clk/sunxi/clk-sun4i-tcon-ch1.c | 167 >> > + >> > 2 files changed, 168 insertions(+) >> > create mode 100644 drivers/clk/sunxi/clk-sun4i-tcon-ch1.c >> >> According to the documents I have, this variant of the TCON clock >> is specific to sun5i. On sun4i/sun7i, TCON CH1 clock has the same >> layout as TCON CH0 and the other display clocks. > > At least for the A20, it's not true. > > Make sure you do not confuse LCD1 CH0 (p79, which is a channel 0 > clock), with LCD0 CH1 (p81, which is a channel 1 clock). Right. The names are great for confusing the reader. :( >> > + sclk1_parents[0] = sclk2_name; >> > + sclk1_parents[1] = sclk2d2_name; >> >> Is there any need to expose these 2 clocks via DT using of_clk_add_provider? > > No, as far as I'm aware, there's no user external to this clock > driver. > >> Note that these complex clock trees within a clock node breaks the >> assigned-clock-parents mechanism, as you can no longer specify the output >> clock's direct parents. > > There's no point of changing the parent either. Hardware blocks are > always connected to the leaf clock (sclk1). We could also model it as > an extra 1-bit divider, which would simplify a bit the logic though. Probably not. You still have a gate to handle. It's just moving the divider from 1 clock to the other. I think the current approach of modeling it like the hardware is better. About reparenting, what I meant was if sclk2 is not exposed through of_clk_add_provider, then we can't do assigned-clocks stuff on it, like setting a default parent or making each channel use a different source pll. What I'm saying is if it is not expected to work with another core binding, we should probably note it somewhere. Regards ChenYu -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/19] clk: sunxi: Add TCON channel1 clock
Hi, On Fri, Oct 30, 2015 at 02:37:34PM -0700, Stephen Boyd wrote: > On 10/30, Maxime Ripard wrote: > > The TCON is a controller generating the timings to output videos signals, > > acting like both a CRTC and an encoder. > > > > It has two channels depending on the output, each channel being driven by > > its own clock (and own clock controller). > > > > Add a driver for the channel 1 clock. > > > > Signed-off-by: Maxime Ripard > > Similar comments apply to patches 3 and 4. Was the same code > copy/pasted two more times and then changed to have different > values? I don't really recall, but I probably used the same skeleton yeah. > Looks like we should consolidate all that stuff into something more > generic so that we don't have the same problems 3 times. Does it? They're both pretty different actually. One is a gate + mux + reset (patch 3), the other one is actually a combination of two clocks, one that is the parent of the other, the former being a gate + mux, the latter a gate + div. At least at the hardware level, they're very different, and adding more code to deal with both case would complicate quite a lot the probe code, for no real reasons. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 04/19] clk: sunxi: Add TCON channel1 clock
Hi, On Sat, Oct 31, 2015 at 05:53:26PM +0800, Chen-Yu Tsai wrote: > On Fri, Oct 30, 2015 at 10:20 PM, Maxime Ripard > wrote: > > The TCON is a controller generating the timings to output videos signals, > > acting like both a CRTC and an encoder. > > > > It has two channels depending on the output, each channel being driven by > > its own clock (and own clock controller). > > > > Add a driver for the channel 1 clock. > > > > Signed-off-by: Maxime Ripard > > --- > > drivers/clk/sunxi/Makefile | 1 + > > drivers/clk/sunxi/clk-sun4i-tcon-ch1.c | 167 > > + > > 2 files changed, 168 insertions(+) > > create mode 100644 drivers/clk/sunxi/clk-sun4i-tcon-ch1.c > > According to the documents I have, this variant of the TCON clock > is specific to sun5i. On sun4i/sun7i, TCON CH1 clock has the same > layout as TCON CH0 and the other display clocks. At least for the A20, it's not true. Make sure you do not confuse LCD1 CH0 (p79, which is a channel 0 clock), with LCD0 CH1 (p81, which is a channel 1 clock). > > + sclk1_parents[0] = sclk2_name; > > + sclk1_parents[1] = sclk2d2_name; > > Is there any need to expose these 2 clocks via DT using of_clk_add_provider? No, as far as I'm aware, there's no user external to this clock driver. > Note that these complex clock trees within a clock node breaks the > assigned-clock-parents mechanism, as you can no longer specify the output > clock's direct parents. There's no point of changing the parent either. Hardware blocks are always connected to the leaf clock (sclk1). We could also model it as an extra 1-bit divider, which would simplify a bit the logic though. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 04/19] clk: sunxi: Add TCON channel1 clock
On Fri, Oct 30, 2015 at 10:20 PM, Maxime Ripard
wrote:
> The TCON is a controller generating the timings to output videos signals,
> acting like both a CRTC and an encoder.
>
> It has two channels depending on the output, each channel being driven by
> its own clock (and own clock controller).
>
> Add a driver for the channel 1 clock.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/clk/sunxi/Makefile | 1 +
> drivers/clk/sunxi/clk-sun4i-tcon-ch1.c | 167
> +
> 2 files changed, 168 insertions(+)
> create mode 100644 drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
According to the documents I have, this variant of the TCON clock
is specific to sun5i. On sun4i/sun7i, TCON CH1 clock has the same
layout as TCON CH0 and the other display clocks.
>
> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
> index 7821b2b63d58..ed59ee4b3a85 100644
> --- a/drivers/clk/sunxi/Makefile
> +++ b/drivers/clk/sunxi/Makefile
> @@ -13,6 +13,7 @@ obj-y += clk-simple-gates.o
> obj-y += clk-sun4i-display.o
> obj-y += clk-sun4i-pll3.o
> obj-y += clk-sun4i-tcon-ch0.o
> +obj-y += clk-sun4i-tcon-ch1.o
> obj-y += clk-sun8i-mbus.o
> obj-y += clk-sun9i-core.o
> obj-y += clk-sun9i-mmc.o
> diff --git a/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
> b/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
> new file mode 100644
> index ..9f2ab52b0bf9
> --- /dev/null
> +++ b/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright 2015 Maxime Ripard
> + *
> + * Maxime Ripard
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#define SUN4I_TCON_CH1_SCLK_NAME_LEN 32
> +
> +#define SUN4I_A10_TCON_CH1_SCLK1_PARENTS 2
> +#define SUN4I_A10_TCON_CH1_SCLK2_PARENTS 4
> +
> +#define SUN4I_A10_TCON_CH1_SCLK2_GATE_BIT 31
> +#define SUN4I_A10_TCON_CH1_SCLK2_MUX_MASK 3
> +#define SUN4I_A10_TCON_CH1_SCLK2_MUX_SHIFT 24
> +#define SUN4I_A10_TCON_CH1_SCLK2_DIV_WIDTH 4
> +#define SUN4I_A10_TCON_CH1_SCLK2_DIV_SHIFT 0
> +
> +#define SUN4I_A10_TCON_CH1_SCLK1_GATE_BIT 15
> +#define SUN4I_A10_TCON_CH1_SCLK1_MUX_MASK 1
> +#define SUN4I_A10_TCON_CH1_SCLK1_MUX_SHIFT 11
> +
> +static DEFINE_SPINLOCK(sun4i_a10_tcon_ch1_lock);
> +
> +static void __init sun4i_a10_tcon_ch1_setup(struct device_node *node)
> +{
> + const char *sclk1_parents[SUN4I_A10_TCON_CH1_SCLK1_PARENTS];
> + const char *sclk2_parents[SUN4I_A10_TCON_CH1_SCLK2_PARENTS];
> + const char *sclk1_name = node->name;
> + char sclk2_name[SUN4I_TCON_CH1_SCLK_NAME_LEN];
> + char sclk2d2_name[SUN4I_TCON_CH1_SCLK_NAME_LEN];
> + struct clk_gate *sclk1_gate, *sclk2_gate;
> + struct clk_mux *sclk1_mux, *sclk2_mux;
> + struct clk *sclk1, *sclk2, *sclk2d2;
> + struct clk_divider *sclk2_div;
> + void __iomem *reg;
> + int i;
> +
> + of_property_read_string(node, "clock-output-names",
> + &sclk1_name);
> +
> + snprintf(sclk2_name, SUN4I_TCON_CH1_SCLK_NAME_LEN,
> +"%s2", sclk1_name);
> +
> + snprintf(sclk2d2_name, SUN4I_TCON_CH1_SCLK_NAME_LEN,
> +"%s2d2", sclk1_name);
> +
> + reg = of_io_request_and_map(node, 0, of_node_full_name(node));
> + if (IS_ERR(reg)) {
> + pr_err("%s: Could not map the clock registers\n", sclk2_name);
> + return;
> + }
> +
> + for (i = 0; i < SUN4I_A10_TCON_CH1_SCLK2_PARENTS; i++)
> + sclk2_parents[i] = of_clk_get_parent_name(node, i);
> +
> + sclk2_mux = kzalloc(sizeof(*sclk2_mux), GFP_KERNEL);
> + if (!sclk2_mux)
> + return;
> +
> + sclk2_mux->reg = reg;
> + sclk2_mux->shift = SUN4I_A10_TCON_CH1_SCLK2_MUX_SHIFT;
> + sclk2_mux->mask = SUN4I_A10_TCON_CH1_SCLK2_MUX_MASK;
> + sclk2_mux->lock = &sun4i_a10_tcon_ch1_lock;
> +
> + sclk2_gate = kzalloc(sizeof(*sclk2_gate), GFP_KERNEL);
> + if (!sclk2_gate)
> + goto free_sclk2_mux;
> +
> + sclk2_gate->reg = reg;
> + sclk2_gate->bit_idx = SUN4I_A10_TCON_CH1_SCLK2_GATE_BIT;
> + sclk2_gate->lock = &sun4i_a10_tcon_ch1_lock;
> +
> + sclk2_div = kzalloc(sizeof(*sclk2_div), GFP_KERNEL);
> + if (!sclk2_div)
> + goto free_sclk2_gate;
> +
> + sclk2_div->reg = reg;
> + sclk2_div->shift = SUN4I_A10_TCON_CH1_SCLK2_DIV_SHIFT;
> + sclk2_div->width = SUN4I_A
Re: [PATCH 04/19] clk: sunxi: Add TCON channel1 clock
On 10/30, Maxime Ripard wrote: > The TCON is a controller generating the timings to output videos signals, > acting like both a CRTC and an encoder. > > It has two channels depending on the output, each channel being driven by > its own clock (and own clock controller). > > Add a driver for the channel 1 clock. > > Signed-off-by: Maxime Ripard Similar comments apply to patches 3 and 4. Was the same code copy/pasted two more times and then changed to have different values? Looks like we should consolidate all that stuff into something more generic so that we don't have the same problems 3 times. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/19] clk: sunxi: Add TCON channel1 clock
The TCON is a controller generating the timings to output videos signals,
acting like both a CRTC and an encoder.
It has two channels depending on the output, each channel being driven by
its own clock (and own clock controller).
Add a driver for the channel 1 clock.
Signed-off-by: Maxime Ripard
---
drivers/clk/sunxi/Makefile | 1 +
drivers/clk/sunxi/clk-sun4i-tcon-ch1.c | 167 +
2 files changed, 168 insertions(+)
create mode 100644 drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
index 7821b2b63d58..ed59ee4b3a85 100644
--- a/drivers/clk/sunxi/Makefile
+++ b/drivers/clk/sunxi/Makefile
@@ -13,6 +13,7 @@ obj-y += clk-simple-gates.o
obj-y += clk-sun4i-display.o
obj-y += clk-sun4i-pll3.o
obj-y += clk-sun4i-tcon-ch0.o
+obj-y += clk-sun4i-tcon-ch1.o
obj-y += clk-sun8i-mbus.o
obj-y += clk-sun9i-core.o
obj-y += clk-sun9i-mmc.o
diff --git a/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
b/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
new file mode 100644
index ..9f2ab52b0bf9
--- /dev/null
+++ b/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright 2015 Maxime Ripard
+ *
+ * Maxime Ripard
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+
+#define SUN4I_TCON_CH1_SCLK_NAME_LEN 32
+
+#define SUN4I_A10_TCON_CH1_SCLK1_PARENTS 2
+#define SUN4I_A10_TCON_CH1_SCLK2_PARENTS 4
+
+#define SUN4I_A10_TCON_CH1_SCLK2_GATE_BIT 31
+#define SUN4I_A10_TCON_CH1_SCLK2_MUX_MASK 3
+#define SUN4I_A10_TCON_CH1_SCLK2_MUX_SHIFT 24
+#define SUN4I_A10_TCON_CH1_SCLK2_DIV_WIDTH 4
+#define SUN4I_A10_TCON_CH1_SCLK2_DIV_SHIFT 0
+
+#define SUN4I_A10_TCON_CH1_SCLK1_GATE_BIT 15
+#define SUN4I_A10_TCON_CH1_SCLK1_MUX_MASK 1
+#define SUN4I_A10_TCON_CH1_SCLK1_MUX_SHIFT 11
+
+static DEFINE_SPINLOCK(sun4i_a10_tcon_ch1_lock);
+
+static void __init sun4i_a10_tcon_ch1_setup(struct device_node *node)
+{
+ const char *sclk1_parents[SUN4I_A10_TCON_CH1_SCLK1_PARENTS];
+ const char *sclk2_parents[SUN4I_A10_TCON_CH1_SCLK2_PARENTS];
+ const char *sclk1_name = node->name;
+ char sclk2_name[SUN4I_TCON_CH1_SCLK_NAME_LEN];
+ char sclk2d2_name[SUN4I_TCON_CH1_SCLK_NAME_LEN];
+ struct clk_gate *sclk1_gate, *sclk2_gate;
+ struct clk_mux *sclk1_mux, *sclk2_mux;
+ struct clk *sclk1, *sclk2, *sclk2d2;
+ struct clk_divider *sclk2_div;
+ void __iomem *reg;
+ int i;
+
+ of_property_read_string(node, "clock-output-names",
+ &sclk1_name);
+
+ snprintf(sclk2_name, SUN4I_TCON_CH1_SCLK_NAME_LEN,
+"%s2", sclk1_name);
+
+ snprintf(sclk2d2_name, SUN4I_TCON_CH1_SCLK_NAME_LEN,
+"%s2d2", sclk1_name);
+
+ reg = of_io_request_and_map(node, 0, of_node_full_name(node));
+ if (IS_ERR(reg)) {
+ pr_err("%s: Could not map the clock registers\n", sclk2_name);
+ return;
+ }
+
+ for (i = 0; i < SUN4I_A10_TCON_CH1_SCLK2_PARENTS; i++)
+ sclk2_parents[i] = of_clk_get_parent_name(node, i);
+
+ sclk2_mux = kzalloc(sizeof(*sclk2_mux), GFP_KERNEL);
+ if (!sclk2_mux)
+ return;
+
+ sclk2_mux->reg = reg;
+ sclk2_mux->shift = SUN4I_A10_TCON_CH1_SCLK2_MUX_SHIFT;
+ sclk2_mux->mask = SUN4I_A10_TCON_CH1_SCLK2_MUX_MASK;
+ sclk2_mux->lock = &sun4i_a10_tcon_ch1_lock;
+
+ sclk2_gate = kzalloc(sizeof(*sclk2_gate), GFP_KERNEL);
+ if (!sclk2_gate)
+ goto free_sclk2_mux;
+
+ sclk2_gate->reg = reg;
+ sclk2_gate->bit_idx = SUN4I_A10_TCON_CH1_SCLK2_GATE_BIT;
+ sclk2_gate->lock = &sun4i_a10_tcon_ch1_lock;
+
+ sclk2_div = kzalloc(sizeof(*sclk2_div), GFP_KERNEL);
+ if (!sclk2_div)
+ goto free_sclk2_gate;
+
+ sclk2_div->reg = reg;
+ sclk2_div->shift = SUN4I_A10_TCON_CH1_SCLK2_DIV_SHIFT;
+ sclk2_div->width = SUN4I_A10_TCON_CH1_SCLK2_DIV_WIDTH;
+ sclk2_div->lock = &sun4i_a10_tcon_ch1_lock;
+
+ sclk2 = clk_register_composite(NULL, sclk2_name, sclk2_parents,
+ SUN4I_A10_TCON_CH1_SCLK2_PARENTS,
+ &sclk2_mux->hw, &clk_mux_ops,
+ &sclk2_div->hw, &clk_divider_ops,
+ &sclk2_gate->hw, &clk_gate_ops,
+ 0);
+ if
