Re: [PATCH v3 0/4] DT clock bindings

From: Shawn Guo
Date: Fri Jun 15 2012 - 04:39:39 EST

On Tue, Jun 12, 2012 at 09:41:47AM -0500, Rob Herring wrote:
> I'm posting this again to solicit further review. There has been some
> discussion[1], but no definite path forward. This series is not changed
> from the last post other than rebasing to v3.5-rc2.
>
Hi Rob,

Per your comment[1], the patch below takes imx6q as example to define
single CCM node with a whole bunch of outputs to support clk lookup
with device tree. (Only uart and usdhc clocks are being put there for
demonstration.) Though it seems working, going through the patch you
will see a couple problems which may need to be solved to make the
binding useful for cases like imx.

* Different clk matching behavior between DT and non-DT lookup

Let's take usdhc example (drivers/mmc/host/sdhci-esdhc-imx.c) to
elaborate the problem. The driver is being shared by several imx
SoCs, imx25, imx35, imx5 and imx6. It needs to get 3 clocks below.

imx_data->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
imx_data->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
imx_data->clk_per = devm_clk_get(&pdev->dev, "per");

But not every single imx SoC clock tree implementation has all these 3
clocks for sdhc available. The imx6q happens to have only "per" clock,
and clk-imx6q driver registers one clkdev for each usdhc instance with
con_id being NULL.

clk_register_clkdev(clk[usdhc1], NULL, "2190000.usdhc");
clk_register_clkdev(clk[usdhc2], NULL, "2194000.usdhc");
clk_register_clkdev(clk[usdhc3], NULL, "2198000.usdhc");
clk_register_clkdev(clk[usdhc4], NULL, "219c000.usdhc");

The non-DT lookup will match all those three clk_get calls to that
single clkdev entry. That said, the NULL con_id is treated as wildcard.
When translating above clk_register_clkdev calls into DT lookup, I
would expect having "clock-names" absent should just work with all
those 3 clk_get calls to get <&clks 2>.

usdhc@02190000 {
...
clocks = <&clks 2>;
};

But with the current of_clk implementation, we will have to have
something like below to keep the usdhc behavior same as non-DT lookup.

usdhc@02190000 {
...
clocks = <&clks 2>, <&clks 2>, <&clks 2>;
clock-names = "ipg", "ahb", "per";
};

Can we force all the clk_get calls with different con_id to get the
only clk listed in "clocks" when "clock-names" is absent, so that we
can somehow match the behavior with non-DT lookup?

* phandle argument is not easy for engineering

As we will have a whole bunch of clock outputs listed in ccm node,
which will be referenced by peripheral phandle in form of <&clks index>.
When the list gets long, it becomes hard for people to find the correct
index number for the clock referenced.

Regards,
Shawn

[1] http://thread.gmane.org/gmane.linux.kernel/1300259/focus=1301107

8<---
.../devicetree/bindings/clk/clk-imx6q.txt | 58 ++++++++++++++++++++
arch/arm/boot/dts/imx6q.dtsi | 27 +++++++++-
arch/arm/mach-imx/clk-imx6q.c | 31 ++++++-----
3 files changed, 101 insertions(+), 15 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clk/clk-imx6q.txt

diff --git a/Documentation/devicetree/bindings/clk/clk-imx6q.txt b/Documentation/devicetree/bindings/clk/clk-imx6q.txt
new file mode 100644
index 0000000..c5698a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/clk/clk-imx6q.txt
@@ -0,0 +1,58 @@
+* Clock bindings for Freescale i.MX6Q
+
+Required properties:
+- compatible: Should be "fsl,imx6q-ccm"
+- reg: Address and length of the register set
+- interrupts: Should contain CCM interrupt
+- #clock-cells: Should be <1>
+- clock-output-names: A list of clock output that CCM provides. The valid
+ clock names include:
+
+ dummy, ckil, ckih, osc, pll2_pfd0_352m, pll2_pfd1_594m, pll2_pfd2_396m,
+ pll3_pfd0_720m, pll3_pfd1_540m, pll3_pfd2_508m, pll3_pfd3_454m,
+ pll2_198m, pll3_120m, pll3_80m, pll3_60m, twd, step, pll1_sw,
+ periph_pre, periph2_pre, periph_clk2_sel, periph2_clk2_sel, axi_sel,
+ esai_sel, asrc_sel, spdif_sel, gpu2d_axi, gpu3d_axi, gpu2d_core_sel,
+ gpu3d_core_sel, gpu3d_shader_sel, ipu1_sel, ipu2_sel, ldb_di0_sel,
+ ldb_di1_sel, ipu1_di0_pre_sel, ipu1_di1_pre_sel, ipu2_di0_pre_sel,
+ ipu2_di1_pre_sel, ipu1_di0_sel, ipu1_di1_sel, ipu2_di0_sel,
+ ipu2_di1_sel, hsi_tx_sel, pcie_axi_sel, ssi1_sel, ssi2_sel, ssi3_sel,
+ usdhc1_sel, usdhc2_sel, usdhc3_sel, usdhc4_sel, enfc_sel, emi_sel,
+ emi_slow_sel, vdo_axi_sel, vpu_axi_sel, cko1_sel, periph, periph2,
+ periph_clk2, periph2_clk2, ipg, ipg_per, esai_pred, esai_podf,
+ asrc_pred, asrc_podf, spdif_pred, spdif_podf, can_root, ecspi_root,
+ gpu2d_core_podf, gpu3d_core_podf, gpu3d_shader, ipu1_podf, ipu2_podf,
+ ldb_di0_podf, ldb_di1_podf, ipu1_di0_pre, ipu1_di1_pre, ipu2_di0_pre,
+ ipu2_di1_pre, hsi_tx_podf, ssi1_pred, ssi1_podf, ssi2_pred, ssi2_podf,
+ ssi3_pred, ssi3_podf, uart_serial_podf, usdhc1_podf, usdhc2_podf,
+ usdhc3_podf, usdhc4_podf, enfc_pred, enfc_podf, emi_podf,
+ emi_slow_podf, vpu_axi_podf, cko1_podf, axi, mmdc_ch0_axi_podf,
+ mmdc_ch1_axi_podf, arm, ahb, apbh_dma, asrc, can1_ipg, can1_serial,
+ can2_ipg, can2_serial, ecspi1, ecspi2, ecspi3, ecspi4, ecspi5, enet,
+ esai, gpt_ipg, gpt_ipg_per, gpu2d_core, gpu3d_core, hdmi_iahb,
+ hdmi_isfr, i2c1, i2c2, i2c3, iim, enfc, ipu1, ipu1_di0, ipu1_di1, ipu2,
+ ipu2_di0, ldb_di0, ldb_di1, ipu2_di1, hsi_tx, mlb, mmdc_ch0_axi,
+ mmdc_ch1_axi, ocram, openvg_axi, pcie_axi, pwm1, pwm2, pwm3, pwm4,
+ gpmi_bch_apb, gpmi_bch, gpmi_io, gpmi_apb, sata, sdma, spba, ssi1,
+ ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
+ usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
+ pll4_audio, pll5_video, pll6_mlb, pll7_usb_host, pll8_enet, ssi1_ipg,
+ ssi2_ipg, ssi3_ipg, rom,
+
+ But generally, only the clocks that peripheral nodes have a phandle pointing
+ to need to be listed there.
+
+Examples:
+
+clks: ccm@020c4000 {
+ compatible = "fsl,imx6q-ccm";
+ reg = <0x020c4000 0x4000>;
+ interrupts = <0 87 0x04 0 88 0x04>;
+ #clock-cells = <1>;
+ clock-output-names = "uart_ipg",
+ "uart_serial",
+ "usdhc1",
+ "usdhc2",
+ "usdhc3",
+ "usdhc4";
+};
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 8c90cba..51b915835 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -169,6 +169,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x02020000 0x4000>;
interrupts = <0 26 0x04>;
+ clocks = <&clks 0>, <&clks 1>;
+ clock-names = "ipg", "per";
status = "disabled";
};

@@ -348,10 +350,17 @@
status = "disabled";
};

- ccm@020c4000 {
+ clks: ccm@020c4000 {
compatible = "fsl,imx6q-ccm";
reg = <0x020c4000 0x4000>;
interrupts = <0 87 0x04 0 88 0x04>;
+ #clock-cells = <1>;
+ clock-output-names = "uart_ipg",
+ "uart_serial",
+ "usdhc1",
+ "usdhc2",
+ "usdhc3",
+ "usdhc4";
};

anatop@020c8000 {
@@ -589,6 +598,8 @@
compatible = "fsl,imx6q-usdhc";
reg = <0x02190000 0x4000>;
interrupts = <0 22 0x04>;
+ clocks = <&clks 2>, <&clks 2>, <&clks 2>;
+ clock-names = "ipg", "ahb", "per";
status = "disabled";
};

@@ -596,6 +607,8 @@
compatible = "fsl,imx6q-usdhc";
reg = <0x02194000 0x4000>;
interrupts = <0 23 0x04>;
+ clocks = <&clks 3>, <&clks 3>, <&clks 3>;
+ clock-names = "ipg", "ahb", "per";
status = "disabled";
};

@@ -603,6 +616,8 @@
compatible = "fsl,imx6q-usdhc";
reg = <0x02198000 0x4000>;
interrupts = <0 24 0x04>;
+ clocks = <&clks 4>, <&clks 4>, <&clks 4>;
+ clock-names = "ipg", "ahb", "per";
status = "disabled";
};

@@ -610,6 +625,8 @@
compatible = "fsl,imx6q-usdhc";
reg = <0x0219c000 0x4000>;
interrupts = <0 25 0x04>;
+ clocks = <&clks 5>, <&clks 5>, <&clks 5>;
+ clock-names = "ipg", "ahb", "per";
status = "disabled";
};

@@ -700,6 +717,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x021e8000 0x4000>;
interrupts = <0 27 0x04>;
+ clocks = <&clks 0>, <&clks 1>;
+ clock-names = "ipg", "per";
status = "disabled";
};

@@ -707,6 +726,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x021ec000 0x4000>;
interrupts = <0 28 0x04>;
+ clocks = <&clks 0>, <&clks 1>;
+ clock-names = "ipg", "per";
status = "disabled";
};

@@ -714,6 +735,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x021f0000 0x4000>;
interrupts = <0 29 0x04>;
+ clocks = <&clks 0>, <&clks 1>;
+ clock-names = "ipg", "per";
status = "disabled";
};

@@ -721,6 +744,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x021f4000 0x4000>;
interrupts = <0 30 0x04>;
+ clocks = <&clks 0>, <&clks 1>;
+ clock-names = "ipg", "per";
status = "disabled";
};
};
diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index abb42e7..87b134e 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -57,6 +57,21 @@ static void __iomem *ccm_base;

void __init imx6q_clock_map_io(void) { }

+static struct clk *imx_clk_src_get(struct of_phandle_args *clkspec,
+ void *data)
+{
+ const char *clk_name;
+ int idx = clkspec->args[0];
+ int ret;
+
+ ret = of_property_read_string_index(clkspec->np, "clock-output-names",
+ idx, &clk_name);
+ if (ret < 0)
+ return ERR_PTR(ret);
+
+ return __clk_lookup(clk_name);
+}
+
int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
{
u32 val = readl_relaxed(ccm_base + CLPCR);
@@ -391,21 +406,7 @@ int __init mx6q_clocks_init(void)
clk_register_clkdev(clk[gpt_ipg], "ipg", "imx-gpt.0");
clk_register_clkdev(clk[gpt_ipg_per], "per", "imx-gpt.0");
clk_register_clkdev(clk[twd], NULL, "smp_twd");
- clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
- clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "21e8000.serial");
- clk_register_clkdev(clk[uart_serial], "per", "21ec000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "21ec000.serial");
- clk_register_clkdev(clk[uart_serial], "per", "21f0000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "21f0000.serial");
- clk_register_clkdev(clk[uart_serial], "per", "21f4000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "21f4000.serial");
clk_register_clkdev(clk[enet], NULL, "2188000.ethernet");
- clk_register_clkdev(clk[usdhc1], NULL, "2190000.usdhc");
- clk_register_clkdev(clk[usdhc2], NULL, "2194000.usdhc");
- clk_register_clkdev(clk[usdhc3], NULL, "2198000.usdhc");
- clk_register_clkdev(clk[usdhc4], NULL, "219c000.usdhc");
clk_register_clkdev(clk[i2c1], NULL, "21a0000.i2c");
clk_register_clkdev(clk[i2c2], NULL, "21a4000.i2c");
clk_register_clkdev(clk[i2c3], NULL, "21a8000.i2c");
@@ -422,6 +423,8 @@ int __init mx6q_clocks_init(void)
clk_register_clkdev(clk[ahb], "ahb", NULL);
clk_register_clkdev(clk[cko1], "cko1", NULL);

+ of_clk_add_provider(np, imx_clk_src_get, NULL);
+
for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
clk_prepare_enable(clk[clks_init_on[i]]);

--
1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/