Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1174201 > unrolled thread

[PATCH v2 0/4] Add Mediatek MT8173 subsystem clocks support

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] Add Mediatek MT8173 subsystem clocks support James Liao <jamesjj.liao@mediatek.com> - 2015-06-30 05:00 +0200
    [PATCH v2 2/4] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers James Liao <jamesjj.liao@mediatek.com> - 2015-06-30 05:10 +0200
      Re: [PATCH v2 2/4] dt-bindings: ARM: Mediatek: Document devicetree  bindings for clock controllers Stephen Boyd <sboyd@codeaurora.org> - 2015-07-03 01:50 +0200
        Re: [PATCH v2 2/4] dt-bindings: ARM: Mediatek: Document devicetree  bindings for clock controllers Daniel Kurtz <djkurtz@chromium.org> - 2015-07-03 12:50 +0200
    [PATCH v2 1/4] clk: mediatek: mt8173: Fix enabling of critical clocks James Liao <jamesjj.liao@mediatek.com> - 2015-06-30 05:10 +0200
      Re: [PATCH v2 1/4] clk: mediatek: mt8173: Fix enabling of critical clocks Daniel Kurtz <djkurtz@chromium.org> - 2015-07-02 04:10 +0200
      Re: [PATCH v2 1/4] clk: mediatek: mt8173: Fix enabling of critical  clocks Stephen Boyd <sboyd@codeaurora.org> - 2015-07-07 01:00 +0200

#1174201 — [PATCH v2 0/4] Add Mediatek MT8173 subsystem clocks support

This patchset is based on 4.1-rc1 plus [1], and contains subsystem
clocks support for Mediatek MT8173.

There are many different implementation suggestions due to each subsystem
HW contains multiple functions, such as clock and reset controllers.

In this patch, I still put subsystem clock implementations in
drivers/clk/mediatek, no mater reset controllers may need to implement
in the same directory in the future. That's because many other vendors
also implement clock and reset controllers in drivers/clk, and it looks
like an acceptible way.

changes since v1:
- Add CA7PLL and CA15PLL as critical clocks.
- Use the same register descriptor for imgsys, vensys and vencltsys.
- Generalize apmixedsys special clocks registration.

[1] https://lkml.org/lkml/2015/5/21/97

James Liao (3):
  dt-bindings: ARM: Mediatek: Document devicetree bindings for clock
    controllers
  clk: mediatek: Add subsystem clocks of MT8173
  clk: mediatek: Add USB clock support in MT8173 APMIXEDSYS

Sascha Hauer (1):
  clk: mediatek: mt8173: Fix enabling of critical clocks

 .../bindings/arm/mediatek/mediatek,imgsys.txt      |  22 +
 .../bindings/arm/mediatek/mediatek,mmsys.txt       |  22 +
 .../bindings/arm/mediatek/mediatek,vdecsys.txt     |  22 +
 .../bindings/arm/mediatek/mediatek,vencltsys.txt   |  22 +
 .../bindings/arm/mediatek/mediatek,vencsys.txt     |  22 +
 drivers/clk/mediatek/clk-mt8173.c                  | 467 ++++++++++++++++++++-
 drivers/clk/mediatek/clk-pll.c                     |   7 +-
 include/dt-bindings/clock/mt8173-clk.h             |  94 ++++-
 8 files changed, 664 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,imgsys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt

--
1.8.1.1.dirty

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

[toc] | [next] | [standalone]

#1174206 — [PATCH v2 2/4] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

This adds the binding documentation for the mmsys, imgsys, vdecsys,
vencsys and vencltsys controllers found on Mediatek SoCs.

Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
---
 .../bindings/arm/mediatek/mediatek,imgsys.txt      | 22 ++++++++++++++++++++++
 .../bindings/arm/mediatek/mediatek,mmsys.txt       | 22 ++++++++++++++++++++++
 .../bindings/arm/mediatek/mediatek,vdecsys.txt     | 22 ++++++++++++++++++++++
 .../bindings/arm/mediatek/mediatek,vencltsys.txt   | 22 ++++++++++++++++++++++
 .../bindings/arm/mediatek/mediatek,vencsys.txt     | 22 ++++++++++++++++++++++
 5 files changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,imgsys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,imgsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,imgsys.txt
new file mode 100644
index 0000000..7612bac
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,imgsys.txt
@@ -0,0 +1,22 @@
+Mediatek imgsys controller
+============================
+
+The Mediatek imgsys controller provides various clocks to the system.
+
+Required Properties:
+
+- compatible: Should be:
+	- "mediatek,mt8173-imgsys", "syscon"
+- #clock-cells: Must be 1
+
+The imgsys controller uses the common clk binding from
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+The available clocks are defined in dt-bindings/clock/mt*-clk.h.
+
+Example:
+
+imgsys: imgsys@15000000 {
+	compatible = "mediatek,mt8173-imgsys", "syscon";
+	reg = <0 0x15000000 0 0x1000>;
+	#clock-cells = <1>;
+};
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
new file mode 100644
index 0000000..b51e417
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
@@ -0,0 +1,22 @@
+Mediatek mmsys controller
+============================
+
+The Mediatek mmsys controller provides various clocks to the system.
+
+Required Properties:
+
+- compatible: Should be:
+	- "mediatek,mt8173-mmsys", "syscon"
+- #clock-cells: Must be 1
+
+The mmsys controller uses the common clk binding from
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+The available clocks are defined in dt-bindings/clock/mt*-clk.h.
+
+Example:
+
+mmsys: mmsys@14000000 {
+	compatible = "mediatek,mt8173-mmsys", "syscon";
+	reg = <0 0x14000000 0 0x1000>;
+	#clock-cells = <1>;
+};
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
new file mode 100644
index 0000000..a5b94a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
@@ -0,0 +1,22 @@
+Mediatek vdecsys controller
+============================
+
+The Mediatek vdecsys controller provides various clocks to the system.
+
+Required Properties:
+
+- compatible: Should be:
+	- "mediatek,mt8173-vdecsys", "syscon"
+- #clock-cells: Must be 1
+
+The vdecsys controller uses the common clk binding from
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+The available clocks are defined in dt-bindings/clock/mt*-clk.h.
+
+Example:
+
+vdecsys: vdecsys@16000000 {
+	compatible = "mediatek,mt8173-vdecsys", "syscon";
+	reg = <0 0x16000000 0 0x1000>;
+	#clock-cells = <1>;
+};
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
new file mode 100644
index 0000000..3d4e8d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
@@ -0,0 +1,22 @@
+Mediatek vencltsys controller
+============================
+
+The Mediatek vencltsys controller provides various clocks to the system.
+
+Required Properties:
+
+- compatible: Should be:
+	- "mediatek,mt8173-vencltsys", "syscon"
+- #clock-cells: Must be 1
+
+The vencltsys controller uses the common clk binding from
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+The available clocks are defined in dt-bindings/clock/mt*-clk.h.
+
+Example:
+
+vencltsys: vencltsys@19000000 {
+	compatible = "mediatek,mt8173-vencltsys", "syscon";
+	reg = <0 0x19000000 0 0x1000>;
+	#clock-cells = <1>;
+};
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt
new file mode 100644
index 0000000..e5b72f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt
@@ -0,0 +1,22 @@
+Mediatek vencsys controller
+============================
+
+The Mediatek vencsys controller provides various clocks to the system.
+
+Required Properties:
+
+- compatible: Should be:
+	- "mediatek,mt8173-vencsys", "syscon"
+- #clock-cells: Must be 1
+
+The vencsys controller uses the common clk binding from
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+The available clocks are defined in dt-bindings/clock/mt*-clk.h.
+
+Example:
+
+vencsys: vencsys@18000000 {
+	compatible = "mediatek,mt8173-vencsys", "syscon";
+	reg = <0 0x18000000 0 0x1000>;
+	#clock-cells = <1>;
+};
-- 
1.8.1.1.dirty

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

[toc] | [prev] | [next] | [standalone]

#1176268 — Re: [PATCH v2 2/4] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

On 07/01/2015 09:26 PM, Daniel Kurtz wrote:
> On Thu, Jul 2, 2015 at 10:52 AM, James Liao <jamesjj.liao@mediatek.com> wrote:
>> Hi Daniel,
>>
>>>> +Required Properties:
>>>> +
>>>> +- compatible: Should be:
>>>> +       - "mediatek,mt8173-imgsys", "syscon"
>>>> +- #clock-cells: Must be 1
>>>> +
>>>> +The imgsys controller uses the common clk binding from
>>>> +Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> +The available clocks are defined in dt-bindings/clock/mt*-clk.h.
>>>> +
>>>> +Example:
>>>> +
>>>> +imgsys: imgsys@15000000 {
>>> Since these nodes will be supplying clocks to the rest of the system,
>>> I think the "name" part of each of these should all be
>>> "clock-controller", like topckgen and apmixedsys:
>>>
>>>   imgsys: clock-controller@15000000 {
>> These subsystems (and topckgen also) also contains other functions such
>> as reset controller, which may be implemented in clk/mediatek/ in the
>> future. It is suitable to use "clock-controller" as their name?
> Hmm,
>
> I don't know the "right way" to do this either.
> Pardon me if you've already had these discussions.
> I only recently started looking at these clock nodes in detail :-).
>
> I think what we really have in register space is a "syscon", as
> described in [0]:
> [0] Documentation/devicetree/bindings/mfd/syscon.txt
>
> So, we can define this block of registers as a syscon:
>
> mmsys_syscon: syscon@14000000 {
>        compatible = "mediatek,mt8173-mmsys", "syscon";
>        reg = <0 0x14000000 0 0x1000>;
> };
>
>
> Then for the clock controller functionality, we create a node with a
> "clock-controller" name and a "-clock" compatible, like this:
>
> mmsys_clock: clock-controller {
>        compatible = "mediatek,mt8173-mmsys-clock";
>        #clock-cells = <1>;
>        mediatek,syscon = <&mmsys_syscon>;
> };
>
> You could then do:
> CLK_OF_DECLARE(mtk_mmsys, "mediatek,mt8173-mmsys-clock", mtk_mmsys_init);
>
>
> If you want to reuse the same register range for some other
> functionality, we could then use a different node, with a different
> compatible:
>
> mmsys: reset-controller {
>        compatible = "mediatek,mt8173-mmsys-reset";
>        mediatek,syscon = <&mmsys_syscon>;
> };
>
> What do you think of this approach?

DT nodes typically have a reg property. Not having a reg property is a
good indicator of a problem with the binding. A syscon is used when you
have a DT node with a reg property and some driver attached to it, but
you need to poke some bits in another register region that isn't part of
the reg property. Instead of having multiple nodes with two reg
properties where the second one is the same, we use a phandle and a syscon.

If clock-controller isn't acceptable maybe clock-reset-contoller would
work? Or "power-controller"? We certainly shouldn't be making up
multiple nodes for one hardware block. Of course, the subject of the
patch is "bindings for clock controllers", so it may be that the
registers are predominantly clock related and so the name is appropriate
already.

-- 
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176473 — Re: [PATCH v2 2/4] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

On Fri, Jul 3, 2015 at 7:40 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 07/01/2015 09:26 PM, Daniel Kurtz wrote:
>> On Thu, Jul 2, 2015 at 10:52 AM, James Liao <jamesjj.liao@mediatek.com> wrote:
>>> Hi Daniel,
>>>
>>>>> +Required Properties:
>>>>> +
>>>>> +- compatible: Should be:
>>>>> +       - "mediatek,mt8173-imgsys", "syscon"
>>>>> +- #clock-cells: Must be 1
>>>>> +
>>>>> +The imgsys controller uses the common clk binding from
>>>>> +Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> +The available clocks are defined in dt-bindings/clock/mt*-clk.h.
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +imgsys: imgsys@15000000 {
>>>> Since these nodes will be supplying clocks to the rest of the system,
>>>> I think the "name" part of each of these should all be
>>>> "clock-controller", like topckgen and apmixedsys:
>>>>
>>>>   imgsys: clock-controller@15000000 {
>>> These subsystems (and topckgen also) also contains other functions such
>>> as reset controller, which may be implemented in clk/mediatek/ in the
>>> future. It is suitable to use "clock-controller" as their name?
>> Hmm,
>>
>> I don't know the "right way" to do this either.
>> Pardon me if you've already had these discussions.
>> I only recently started looking at these clock nodes in detail :-).
>>
>> I think what we really have in register space is a "syscon", as
>> described in [0]:
>> [0] Documentation/devicetree/bindings/mfd/syscon.txt
>>
>> So, we can define this block of registers as a syscon:
>>
>> mmsys_syscon: syscon@14000000 {
>>        compatible = "mediatek,mt8173-mmsys", "syscon";
>>        reg = <0 0x14000000 0 0x1000>;
>> };
>>
>>
>> Then for the clock controller functionality, we create a node with a
>> "clock-controller" name and a "-clock" compatible, like this:
>>
>> mmsys_clock: clock-controller {
>>        compatible = "mediatek,mt8173-mmsys-clock";
>>        #clock-cells = <1>;
>>        mediatek,syscon = <&mmsys_syscon>;
>> };
>>
>> You could then do:
>> CLK_OF_DECLARE(mtk_mmsys, "mediatek,mt8173-mmsys-clock", mtk_mmsys_init);
>>
>>
>> If you want to reuse the same register range for some other
>> functionality, we could then use a different node, with a different
>> compatible:
>>
>> mmsys: reset-controller {
>>        compatible = "mediatek,mt8173-mmsys-reset";
>>        mediatek,syscon = <&mmsys_syscon>;
>> };
>>
>> What do you think of this approach?
>
> DT nodes typically have a reg property. Not having a reg property is a
> good indicator of a problem with the binding. A syscon is used when you
> have a DT node with a reg property and some driver attached to it, but
> you need to poke some bits in another register region that isn't part of
> the reg property. Instead of having multiple nodes with two reg
> properties where the second one is the same, we use a phandle and a syscon.
>
> If clock-controller isn't acceptable maybe clock-reset-contoller would
> work? Or "power-controller"? We certainly shouldn't be making up
> multiple nodes for one hardware block. Of course, the subject of the
> patch is "bindings for clock controllers", so it may be that the
> registers are predominantly clock related and so the name is appropriate
> already.

Using "clock-controller" seems to fit best with the bindings
introduced by this patch.

However, if these bindings are for hardware blocks that contain a grab
bag of various functionality that will be added in later patches, then
I think "syscon" might be best.

-Dan

>
> --
> 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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1174207 — [PATCH v2 1/4] clk: mediatek: mt8173: Fix enabling of critical clocks

From: Sascha Hauer <s.hauer@pengutronix.de>

On the MT8173 the clocks are provided by different units. To enable
the critical clocks we must be sure that all parent clocks are already
registered, otherwise the parents of the critical clocks end up being
unused and get disabled later. To find a place where all parents are
registered we try each time after we've registered some clocks if
all known providers are present now and only then we enable the critical
clocks

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
---
 drivers/clk/mediatek/clk-mt8173.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 4b9e04c..c483336 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -700,6 +700,22 @@ static const struct mtk_composite peri_clks[] __initconst = {
 	MUX(CLK_PERI_UART3_SEL, "uart3_ck_sel", uart_ck_sel_parents, 0x40c, 3, 1),
 };
 
+static struct clk_onecell_data *mt8173_top_clk_data;
+static struct clk_onecell_data *mt8173_pll_clk_data;
+
+static void mtk_clk_enable_critical(void)
+{
+	if (!mt8173_top_clk_data || !mt8173_pll_clk_data)
+		return;
+
+	clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA15PLL]);
+	clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA7PLL]);
+	clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_MEM_SEL]);
+	clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_DDRPHYCFG_SEL]);
+	clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_CCI400_SEL]);
+	clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_RTC_SEL]);
+}
+
 static void __init mtk_topckgen_init(struct device_node *node)
 {
 	struct clk_onecell_data *clk_data;
@@ -712,19 +728,19 @@ static void __init mtk_topckgen_init(struct device_node *node)
 		return;
 	}
 
-	clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK);
+	mt8173_top_clk_data = clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK);
 
 	mtk_clk_register_factors(root_clk_alias, ARRAY_SIZE(root_clk_alias), clk_data);
 	mtk_clk_register_factors(top_divs, ARRAY_SIZE(top_divs), clk_data);
 	mtk_clk_register_composites(top_muxes, ARRAY_SIZE(top_muxes), base,
 			&mt8173_clk_lock, clk_data);
 
-	clk_prepare_enable(clk_data->clks[CLK_TOP_CCI400_SEL]);
-
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
 	if (r)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
+
+	mtk_clk_enable_critical();
 }
 CLK_OF_DECLARE(mtk_topckgen, "mediatek,mt8173-topckgen", mtk_topckgen_init);
 
@@ -818,13 +834,13 @@ static void __init mtk_apmixedsys_init(struct device_node *node)
 {
 	struct clk_onecell_data *clk_data;
 
-	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
+	mt8173_pll_clk_data = clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
 	if (!clk_data)
 		return;
 
 	mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
 
-	clk_prepare_enable(clk_data->clks[CLK_APMIXED_ARMCA15PLL]);
+	mtk_clk_enable_critical();
 }
 CLK_OF_DECLARE(mtk_apmixedsys, "mediatek,mt8173-apmixedsys",
 		mtk_apmixedsys_init);
-- 
1.8.1.1.dirty

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

[toc] | [prev] | [next] | [standalone]

#1175656 — Re: [PATCH v2 1/4] clk: mediatek: mt8173: Fix enabling of critical clocks

On Wed, Jul 1, 2015 at 10:21 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Hi James,
>
> To be precise, it is the CLK_TOP clocks that have CLK_APMIXED PLLs as
> their parents, so we cannot enable the CLK_TOP critical clocks until
> the CLK_APMIXED clocks have all been registered.
>
> Please add something like the above to the commit message.
>
> More comments inline...
>
> On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao@mediatek.com> wrote:
>> From: Sascha Hauer <s.hauer@pengutronix.de>
>>
>> On the MT8173 the clocks are provided by different units. To enable
>> the critical clocks we must be sure that all parent clocks are already
>> registered, otherwise the parents of the critical clocks end up being
>> unused and get disabled later. To find a place where all parents are
>> registered we try each time after we've registered some clocks if
>> all known providers are present now and only then we enable the critical
>> clocks
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
>> ---
>>  drivers/clk/mediatek/clk-mt8173.c | 26 +++++++++++++++++++++-----
>>  1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
>> index 4b9e04c..c483336 100644
>> --- a/drivers/clk/mediatek/clk-mt8173.c
>> +++ b/drivers/clk/mediatek/clk-mt8173.c
>> @@ -700,6 +700,22 @@ static const struct mtk_composite peri_clks[] __initconst = {
>>         MUX(CLK_PERI_UART3_SEL, "uart3_ck_sel", uart_ck_sel_parents, 0x40c, 3, 1),
>>  };
>>
>> +static struct clk_onecell_data *mt8173_top_clk_data;
>> +static struct clk_onecell_data *mt8173_pll_clk_data;
>
> These globals can be:
> __initdata

Oops, never mind about this comment...
Using "__initdata" for uninitialized variables doesn't make sense -
"__initdata" is for initialization data, not variables themselves.

Sorry for the noise,
-Dan

>
>> +
>> +static void mtk_clk_enable_critical(void)
>
> And this function is:
>
> static void __init
>
> Other than the above, this one is:
>
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
>
> -Dan
>
>> +{
>> +       if (!mt8173_top_clk_data || !mt8173_pll_clk_data)
>> +               return;
>> +
>> +       clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA15PLL]);
>> +       clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA7PLL]);
>> +       clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_MEM_SEL]);
>> +       clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_DDRPHYCFG_SEL]);
>> +       clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_CCI400_SEL]);
>> +       clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_RTC_SEL]);
>> +}
>> +
>>  static void __init mtk_topckgen_init(struct device_node *node)
>>  {
>>         struct clk_onecell_data *clk_data;
>> @@ -712,19 +728,19 @@ static void __init mtk_topckgen_init(struct device_node *node)
>>                 return;
>>         }
>>
>> -       clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK);
>> +       mt8173_top_clk_data = clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK);
>>
>>         mtk_clk_register_factors(root_clk_alias, ARRAY_SIZE(root_clk_alias), clk_data);
>>         mtk_clk_register_factors(top_divs, ARRAY_SIZE(top_divs), clk_data);
>>         mtk_clk_register_composites(top_muxes, ARRAY_SIZE(top_muxes), base,
>>                         &mt8173_clk_lock, clk_data);
>>
>> -       clk_prepare_enable(clk_data->clks[CLK_TOP_CCI400_SEL]);
>> -
>>         r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
>>         if (r)
>>                 pr_err("%s(): could not register clock provider: %d\n",
>>                         __func__, r);
>> +
>> +       mtk_clk_enable_critical();
>>  }
>>  CLK_OF_DECLARE(mtk_topckgen, "mediatek,mt8173-topckgen", mtk_topckgen_init);
>>
>> @@ -818,13 +834,13 @@ static void __init mtk_apmixedsys_init(struct device_node *node)
>>  {
>>         struct clk_onecell_data *clk_data;
>>
>> -       clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
>> +       mt8173_pll_clk_data = clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
>>         if (!clk_data)
>>                 return;
>>
>>         mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
>>
>> -       clk_prepare_enable(clk_data->clks[CLK_APMIXED_ARMCA15PLL]);
>> +       mtk_clk_enable_critical();
>>  }
>>  CLK_OF_DECLARE(mtk_apmixedsys, "mediatek,mt8173-apmixedsys",
>>                 mtk_apmixedsys_init);
>> --
>> 1.8.1.1.dirty
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1177952 — Re: [PATCH v2 1/4] clk: mediatek: mt8173: Fix enabling of critical clocks

On 07/02/2015 11:29 PM, Daniel Kurtz wrote:
> Hi Stephen,
>
> On Fri, Jul 3, 2015 at 7:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 06/30, James Liao wrote:
>>> From: Sascha Hauer <s.hauer@pengutronix.de>
>>>
>>> On the MT8173 the clocks are provided by different units. To enable
>>> the critical clocks we must be sure that all parent clocks are already
>>> registered, otherwise the parents of the critical clocks end up being
>>> unused and get disabled later. To find a place where all parents are
>>> registered we try each time after we've registered some clocks if
>>> all known providers are present now and only then we enable the critical
>>> clocks
>>>
>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
>>> ---
>> Applied to clk-fixes
> I think James plans to send an updated version of this patch.
>
>

Oh right. Well those comments are minor so I'll just fold them in given
that I'm rewinding clk-fixes today. I see that comment in init.h about
uninitialized variables and __initdata too. I found this 10 year old
thread about that [1]. At least with my gcc-4.9 arm compiler I don't see
uninitialized __initdata moved to bss. A workaround seems to be to
initialize it to NULL in this case, but maybe that doesn't matter
because compilers don't have that problem anymore.

[1] http://lkml.iu.edu/hypermail/linux/kernel/0512.0/1366.html

-- 
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web