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

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

[James Liao]

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[James Liao]

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

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

[Daniel Kurtz]

On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>
> 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  .../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 {

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 {

-Dan

>
> +       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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[James Liao]

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?


Best regards,

James


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Kurtz]

On Thu, Jul 2, 2015 at 10:52 AM, James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> 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?

Thanks!
-Dan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Boyd]

On 07/01/2015 09:26 PM, Daniel Kurtz wrote:
> On Thu, Jul 2, 2015 at 10:52 AM, James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Kurtz]

On Fri, Jul 3, 2015 at 7:40 AM, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[James Liao]

Hi Daniel,

On Fri, 2015-07-03 at 18:46 +0800, Daniel Kurtz wrote:
> On Fri, Jul 3, 2015 at 7:40 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > 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.

I referred some dt-binding documents that contain clock and reset
controller in the same unit:

qcom,gcc.txt:

	clock-controller@900000 {
		compatible = "qcom,gcc-msm8960";

nvidia,tegra124-car.txt:

	tegra_car: clock {
		compatible = "nvidia,tegra124-car";

rockchip,rk3188-cru.txt:

	cru: cru@20000000 {
		compatible = "rockchip,rk3188-cru";

Rockchip use "Clock and Reset Unit" as their controller's name. The
other two use "clock" or "clock-controller" as their controller's name.

It looks like "clock-controller" is also an acceptable name for
clock/reset controllers. So I would like to keep this name in next
patch.


Best regards,

James

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

[Rob Herring]

On Mon, Jun 29, 2015 at 9:58 PM, James Liao <jamesjj.liao@mediatek.com> wrote:
> 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.

Perhaps some detail on what clocks exactly.

> +
> +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.

The header should be part of this patch as it is part of the binding doc.

Same comments apply to the other files.

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

[James Liao]

From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Kurtz]

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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  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

> +
> +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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Kurtz]

On Wed, Jul 1, 2015 at 10:21 PM, Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>
>> 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> ---
>>  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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[James Liao]

Hi Daniel,

On Wed, 2015-07-01 at 22:21 +0800, Daniel Kurtz wrote:
> 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.

OK. I'll add them.

> 
> >
> > +static struct clk_onecell_data *mt8173_top_clk_data;
> > +static struct clk_onecell_data *mt8173_pll_clk_data;
> 
> These globals can be:
> __initdata
> 
> > +
> > +static void mtk_clk_enable_critical(void)
> 
> And this function is:
> 
> static void __init

I'll add them in next patch.


Best regards,

James

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Boyd]

On 06/30, James Liao wrote:
> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---

Applied to clk-fixes

-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Kurtz]

Hi Stephen,

On Fri, Jul 3, 2015 at 7:03 AM, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> On 06/30, James Liao wrote:
>> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>
>> 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> ---
>
> Applied to clk-fixes

I think James plans to send an updated version of this patch.

>
> --
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Boyd]

On 07/02/2015 11:29 PM, Daniel Kurtz wrote:
> Hi Stephen,
>
> On Fri, Jul 3, 2015 at 7:03 AM, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> On 06/30, James Liao wrote:
>>> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>
>>> 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>>> ---
>> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/4] clk: mediatek: Add subsystem clocks of MT8173

[James Liao]

Most multimedia subsystem clocks will be accessed by multiple
drivers, so it's a better way to manage these clocks in CCF.
This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
subsystems.

Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/clk/mediatek/clk-mt8173.c      | 298 +++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/mt8173-clk.h |  91 +++++++++-
 2 files changed, 387 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index c483336..75be757 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -700,6 +700,183 @@ 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 mtk_gate_regs cg_regs_4_8_0 = {
+	.set_ofs = 0x0004,
+	.clr_ofs = 0x0008,
+	.sta_ofs = 0x0000,
+};
+
+#define GATE_IMG(_id, _name, _parent, _shift) {			\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &cg_regs_4_8_0,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr,		\
+	}
+
+static struct mtk_gate img_clks[] __initdata = {
+	GATE_IMG(CLK_IMG_LARB2_SMI, "img_larb2_smi", "mm_sel", 0),
+	GATE_IMG(CLK_IMG_CAM_SMI, "img_cam_smi", "mm_sel", 5),
+	GATE_IMG(CLK_IMG_CAM_CAM, "img_cam_cam", "mm_sel", 6),
+	GATE_IMG(CLK_IMG_SEN_TG, "img_sen_tg", "camtg_sel", 7),
+	GATE_IMG(CLK_IMG_SEN_CAM, "img_sen_cam", "mm_sel", 8),
+	GATE_IMG(CLK_IMG_CAM_SV, "img_cam_sv", "mm_sel", 9),
+	GATE_IMG(CLK_IMG_FD, "img_fd", "mm_sel", 11),
+};
+
+static struct mtk_gate_regs mm0_cg_regs = {
+	.set_ofs = 0x0104,
+	.clr_ofs = 0x0108,
+	.sta_ofs = 0x0100,
+};
+
+static struct mtk_gate_regs mm1_cg_regs = {
+	.set_ofs = 0x0114,
+	.clr_ofs = 0x0118,
+	.sta_ofs = 0x0110,
+};
+
+#define GATE_MM0(_id, _name, _parent, _shift) {			\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &mm0_cg_regs,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr,		\
+	}
+
+#define GATE_MM1(_id, _name, _parent, _shift) {			\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &mm1_cg_regs,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr,		\
+	}
+
+static struct mtk_gate mm_clks[] __initdata = {
+	/* MM0 */
+	GATE_MM0(CLK_MM_SMI_COMMON, "mm_smi_common", "mm_sel", 0),
+	GATE_MM0(CLK_MM_SMI_LARB0, "mm_smi_larb0", "mm_sel", 1),
+	GATE_MM0(CLK_MM_CAM_MDP, "mm_cam_mdp", "mm_sel", 2),
+	GATE_MM0(CLK_MM_MDP_RDMA0, "mm_mdp_rdma0", "mm_sel", 3),
+	GATE_MM0(CLK_MM_MDP_RDMA1, "mm_mdp_rdma1", "mm_sel", 4),
+	GATE_MM0(CLK_MM_MDP_RSZ0, "mm_mdp_rsz0", "mm_sel", 5),
+	GATE_MM0(CLK_MM_MDP_RSZ1, "mm_mdp_rsz1", "mm_sel", 6),
+	GATE_MM0(CLK_MM_MDP_RSZ2, "mm_mdp_rsz2", "mm_sel", 7),
+	GATE_MM0(CLK_MM_MDP_TDSHP0, "mm_mdp_tdshp0", "mm_sel", 8),
+	GATE_MM0(CLK_MM_MDP_TDSHP1, "mm_mdp_tdshp1", "mm_sel", 9),
+	GATE_MM0(CLK_MM_MDP_WDMA, "mm_mdp_wdma", "mm_sel", 11),
+	GATE_MM0(CLK_MM_MDP_WROT0, "mm_mdp_wrot0", "mm_sel", 12),
+	GATE_MM0(CLK_MM_MDP_WROT1, "mm_mdp_wrot1", "mm_sel", 13),
+	GATE_MM0(CLK_MM_FAKE_ENG, "mm_fake_eng", "mm_sel", 14),
+	GATE_MM0(CLK_MM_MUTEX_32K, "mm_mutex_32k", "rtc_sel", 15),
+	GATE_MM0(CLK_MM_DISP_OVL0, "mm_disp_ovl0", "mm_sel", 16),
+	GATE_MM0(CLK_MM_DISP_OVL1, "mm_disp_ovl1", "mm_sel", 17),
+	GATE_MM0(CLK_MM_DISP_RDMA0, "mm_disp_rdma0", "mm_sel", 18),
+	GATE_MM0(CLK_MM_DISP_RDMA1, "mm_disp_rdma1", "mm_sel", 19),
+	GATE_MM0(CLK_MM_DISP_RDMA2, "mm_disp_rdma2", "mm_sel", 20),
+	GATE_MM0(CLK_MM_DISP_WDMA0, "mm_disp_wdma0", "mm_sel", 21),
+	GATE_MM0(CLK_MM_DISP_WDMA1, "mm_disp_wdma1", "mm_sel", 22),
+	GATE_MM0(CLK_MM_DISP_COLOR0, "mm_disp_color0", "mm_sel", 23),
+	GATE_MM0(CLK_MM_DISP_COLOR1, "mm_disp_color1", "mm_sel", 24),
+	GATE_MM0(CLK_MM_DISP_AAL, "mm_disp_aal", "mm_sel", 25),
+	GATE_MM0(CLK_MM_DISP_GAMMA, "mm_disp_gamma", "mm_sel", 26),
+	GATE_MM0(CLK_MM_DISP_UFOE, "mm_disp_ufoe", "mm_sel", 27),
+	GATE_MM0(CLK_MM_DISP_SPLIT0, "mm_disp_split0", "mm_sel", 28),
+	GATE_MM0(CLK_MM_DISP_SPLIT1, "mm_disp_split1", "mm_sel", 29),
+	GATE_MM0(CLK_MM_DISP_MERGE, "mm_disp_merge", "mm_sel", 30),
+	GATE_MM0(CLK_MM_DISP_OD, "mm_disp_od", "mm_sel", 31),
+	/* MM1 */
+	GATE_MM1(CLK_MM_DISP_PWM0MM, "mm_disp_pwm0mm", "mm_sel", 0),
+	GATE_MM1(CLK_MM_DISP_PWM026M, "mm_disp_pwm026m", "pwm_sel", 1),
+	GATE_MM1(CLK_MM_DISP_PWM1MM, "mm_disp_pwm1mm", "mm_sel", 2),
+	GATE_MM1(CLK_MM_DISP_PWM126M, "mm_disp_pwm126m", "pwm_sel", 3),
+	GATE_MM1(CLK_MM_DSI0_ENGINE, "mm_dsi0_engine", "mm_sel", 4),
+	GATE_MM1(CLK_MM_DSI0_DIGITAL, "mm_dsi0_digital", "clk_null", 5),
+	GATE_MM1(CLK_MM_DSI1_ENGINE, "mm_dsi1_engine", "mm_sel", 6),
+	GATE_MM1(CLK_MM_DSI1_DIGITAL, "mm_dsi1_digital", "clk_null", 7),
+	GATE_MM1(CLK_MM_DPI_PIXEL, "mm_dpi_pixel", "dpi0_sel", 8),
+	GATE_MM1(CLK_MM_DPI_ENGINE, "mm_dpi_engine", "mm_sel", 9),
+	GATE_MM1(CLK_MM_DPI1_PIXEL, "mm_dpi1_pixel", "clk_null", 10),
+	GATE_MM1(CLK_MM_DPI1_ENGINE, "mm_dpi1_engine", "mm_sel", 11),
+	GATE_MM1(CLK_MM_HDMI_PIXEL, "mm_hdmi_pixel", "dpi0_sel", 12),
+	GATE_MM1(CLK_MM_HDMI_PLLCK, "mm_hdmi_pllck", "hdmi_sel", 13),
+	GATE_MM1(CLK_MM_HDMI_AUDIO, "mm_hdmi_audio", "apll1", 14),
+	GATE_MM1(CLK_MM_HDMI_SPDIF, "mm_hdmi_spdif", "apll2", 15),
+	GATE_MM1(CLK_MM_LVDS_PIXEL, "mm_lvds_pixel", "clk_null", 16),
+	GATE_MM1(CLK_MM_LVDS_CTS, "mm_lvds_cts", "clk_null", 17),
+	GATE_MM1(CLK_MM_SMI_LARB4, "mm_smi_larb4", "mm_sel", 18),
+	GATE_MM1(CLK_MM_HDMI_HDCP, "mm_hdmi_hdcp", "hdcp_sel", 19),
+	GATE_MM1(CLK_MM_HDMI_HDCP24M, "mm_hdmi_hdcp24m", "hdcp_24m_sel", 20),
+};
+
+static struct mtk_gate_regs vdec0_cg_regs = {
+	.set_ofs = 0x0000,
+	.clr_ofs = 0x0004,
+	.sta_ofs = 0x0000,
+};
+
+static struct mtk_gate_regs vdec1_cg_regs = {
+	.set_ofs = 0x0008,
+	.clr_ofs = 0x000c,
+	.sta_ofs = 0x0008,
+};
+
+#define GATE_VDEC0(_id, _name, _parent, _shift) {		\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &vdec0_cg_regs,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr_inv,		\
+	}
+
+#define GATE_VDEC1(_id, _name, _parent, _shift) {		\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &vdec1_cg_regs,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr_inv,		\
+	}
+
+static struct mtk_gate vdec_clks[] __initdata = {
+	GATE_VDEC0(CLK_VDEC_CKEN, "vdec_cken", "vdec_sel", 0),
+	GATE_VDEC1(CLK_VDEC_LARB_CKEN, "vdec_larb_cken", "mm_sel", 0),
+};
+
+#define GATE_VENC(_id, _name, _parent, _shift) {		\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &cg_regs_4_8_0,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr_inv,		\
+	}
+
+static struct mtk_gate venc_clks[] __initdata = {
+	GATE_VENC(CLK_VENC_CKE0, "venc_cke0", "mm_sel", 0),
+	GATE_VENC(CLK_VENC_CKE1, "venc_cke1", "venc_sel", 4),
+	GATE_VENC(CLK_VENC_CKE2, "venc_cke2", "venc_sel", 8),
+	GATE_VENC(CLK_VENC_CKE3, "venc_cke3", "venc_sel", 12),
+};
+
+#define GATE_VENCLT(_id, _name, _parent, _shift) {		\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &cg_regs_4_8_0,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr_inv,		\
+	}
+
+static struct mtk_gate venclt_clks[] __initdata = {
+	GATE_VENCLT(CLK_VENCLT_CKE0, "venclt_cke0", "mm_sel", 0),
+	GATE_VENCLT(CLK_VENCLT_CKE1, "venclt_cke1", "venclt_sel", 4),
+};
+
 static struct clk_onecell_data *mt8173_top_clk_data;
 static struct clk_onecell_data *mt8173_pll_clk_data;
 
@@ -844,3 +1021,124 @@ static void __init mtk_apmixedsys_init(struct device_node *node)
 }
 CLK_OF_DECLARE(mtk_apmixedsys, "mediatek,mt8173-apmixedsys",
 		mtk_apmixedsys_init);
+
+static void __init mtk_imgsys_init(struct device_node *node)
+{
+	struct clk_onecell_data *clk_data;
+	void __iomem *base;
+	int r;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return;
+	}
+
+	clk_data = mtk_alloc_clk_data(CLK_IMG_NR_CLK);
+
+	mtk_clk_register_gates(node, img_clks, ARRAY_SIZE(img_clks),
+						clk_data);
+
+	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);
+}
+CLK_OF_DECLARE(mtk_imgsys, "mediatek,mt8173-imgsys", mtk_imgsys_init);
+
+static void __init mtk_mmsys_init(struct device_node *node)
+{
+	struct clk_onecell_data *clk_data;
+	void __iomem *base;
+	int r;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return;
+	}
+
+	clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
+
+	mtk_clk_register_gates(node, mm_clks, ARRAY_SIZE(mm_clks),
+						clk_data);
+
+	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);
+}
+CLK_OF_DECLARE(mtk_mmsys, "mediatek,mt8173-mmsys", mtk_mmsys_init);
+
+static void __init mtk_vdecsys_init(struct device_node *node)
+{
+	struct clk_onecell_data *clk_data;
+	void __iomem *base;
+	int r;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return;
+	}
+
+	clk_data = mtk_alloc_clk_data(CLK_VDEC_NR_CLK);
+
+	mtk_clk_register_gates(node, vdec_clks, ARRAY_SIZE(vdec_clks),
+						clk_data);
+
+	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);
+}
+CLK_OF_DECLARE(mtk_vdecsys, "mediatek,mt8173-vdecsys", mtk_vdecsys_init);
+
+static void __init mtk_vencsys_init(struct device_node *node)
+{
+	struct clk_onecell_data *clk_data;
+	void __iomem *base;
+	int r;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return;
+	}
+
+	clk_data = mtk_alloc_clk_data(CLK_VENC_NR_CLK);
+
+	mtk_clk_register_gates(node, venc_clks, ARRAY_SIZE(venc_clks),
+						clk_data);
+
+	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);
+}
+CLK_OF_DECLARE(mtk_vencsys, "mediatek,mt8173-vencsys", mtk_vencsys_init);
+
+static void __init mtk_vencltsys_init(struct device_node *node)
+{
+	struct clk_onecell_data *clk_data;
+	void __iomem *base;
+	int r;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return;
+	}
+
+	clk_data = mtk_alloc_clk_data(CLK_VENCLT_NR_CLK);
+
+	mtk_clk_register_gates(node, venclt_clks, ARRAY_SIZE(venclt_clks),
+						clk_data);
+
+	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);
+}
+CLK_OF_DECLARE(mtk_vencltsys, "mediatek,mt8173-vencltsys", mtk_vencltsys_init);
diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h
index 4ad76ed..6ce88bf 100644
--- a/include/dt-bindings/clock/mt8173-clk.h
+++ b/include/dt-bindings/clock/mt8173-clk.h
@@ -158,8 +158,8 @@
 
 /* APMIXED_SYS */
 
-#define CLK_APMIXED_ARMCA15PLL	1
-#define CLK_APMIXED_ARMCA7PLL	2
+#define CLK_APMIXED_ARMCA15PLL		1
+#define CLK_APMIXED_ARMCA7PLL		2
 #define CLK_APMIXED_MAINPLL		3
 #define CLK_APMIXED_UNIVPLL		4
 #define CLK_APMIXED_MMPLL		5
@@ -232,4 +232,91 @@
 #define CLK_PERI_UART3_SEL		39
 #define CLK_PERI_NR_CLK			40
 
+/* IMG_SYS */
+
+#define CLK_IMG_LARB2_SMI		1
+#define CLK_IMG_CAM_SMI			2
+#define CLK_IMG_CAM_CAM			3
+#define CLK_IMG_SEN_TG			4
+#define CLK_IMG_SEN_CAM			5
+#define CLK_IMG_CAM_SV			6
+#define CLK_IMG_FD			7
+#define CLK_IMG_NR_CLK			8
+
+/* MM_SYS */
+
+#define CLK_MM_SMI_COMMON		1
+#define CLK_MM_SMI_LARB0		2
+#define CLK_MM_CAM_MDP			3
+#define CLK_MM_MDP_RDMA0		4
+#define CLK_MM_MDP_RDMA1		5
+#define CLK_MM_MDP_RSZ0			6
+#define CLK_MM_MDP_RSZ1			7
+#define CLK_MM_MDP_RSZ2			8
+#define CLK_MM_MDP_TDSHP0		9
+#define CLK_MM_MDP_TDSHP1		10
+#define CLK_MM_MDP_WDMA			11
+#define CLK_MM_MDP_WROT0		12
+#define CLK_MM_MDP_WROT1		13
+#define CLK_MM_FAKE_ENG			14
+#define CLK_MM_MUTEX_32K		15
+#define CLK_MM_DISP_OVL0		16
+#define CLK_MM_DISP_OVL1		17
+#define CLK_MM_DISP_RDMA0		18
+#define CLK_MM_DISP_RDMA1		19
+#define CLK_MM_DISP_RDMA2		20
+#define CLK_MM_DISP_WDMA0		21
+#define CLK_MM_DISP_WDMA1		22
+#define CLK_MM_DISP_COLOR0		23
+#define CLK_MM_DISP_COLOR1		24
+#define CLK_MM_DISP_AAL			25
+#define CLK_MM_DISP_GAMMA		26
+#define CLK_MM_DISP_UFOE		27
+#define CLK_MM_DISP_SPLIT0		28
+#define CLK_MM_DISP_SPLIT1		29
+#define CLK_MM_DISP_MERGE		30
+#define CLK_MM_DISP_OD			31
+#define CLK_MM_DISP_PWM0MM		32
+#define CLK_MM_DISP_PWM026M		33
+#define CLK_MM_DISP_PWM1MM		34
+#define CLK_MM_DISP_PWM126M		35
+#define CLK_MM_DSI0_ENGINE		36
+#define CLK_MM_DSI0_DIGITAL		37
+#define CLK_MM_DSI1_ENGINE		38
+#define CLK_MM_DSI1_DIGITAL		39
+#define CLK_MM_DPI_PIXEL		40
+#define CLK_MM_DPI_ENGINE		41
+#define CLK_MM_DPI1_PIXEL		42
+#define CLK_MM_DPI1_ENGINE		43
+#define CLK_MM_HDMI_PIXEL		44
+#define CLK_MM_HDMI_PLLCK		45
+#define CLK_MM_HDMI_AUDIO		46
+#define CLK_MM_HDMI_SPDIF		47
+#define CLK_MM_LVDS_PIXEL		48
+#define CLK_MM_LVDS_CTS			49
+#define CLK_MM_SMI_LARB4		50
+#define CLK_MM_HDMI_HDCP		51
+#define CLK_MM_HDMI_HDCP24M		52
+#define CLK_MM_NR_CLK			53
+
+/* VDEC_SYS */
+
+#define CLK_VDEC_CKEN			1
+#define CLK_VDEC_LARB_CKEN		2
+#define CLK_VDEC_NR_CLK			3
+
+/* VENC_SYS */
+
+#define CLK_VENC_CKE0			1
+#define CLK_VENC_CKE1			2
+#define CLK_VENC_CKE2			3
+#define CLK_VENC_CKE3			4
+#define CLK_VENC_NR_CLK			5
+
+/* VENCLT_SYS */
+
+#define CLK_VENCLT_CKE0			1
+#define CLK_VENCLT_CKE1			2
+#define CLK_VENCLT_NR_CLK		3
+
 #endif /* _DT_BINDINGS_CLK_MT8173_H */
-- 
1.8.1.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] clk: mediatek: Add subsystem clocks of MT8173

[Daniel Kurtz]

On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> Most multimedia subsystem clocks will be accessed by multiple
> drivers, so it's a better way to manage these clocks in CCF.
> This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
> subsystems.
>
> Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/clk/mediatek/clk-mt8173.c      | 298 +++++++++++++++++++++++++++++++++
>  include/dt-bindings/clock/mt8173-clk.h |  91 +++++++++-
>  2 files changed, 387 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
> index c483336..75be757 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -700,6 +700,183 @@ 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 mtk_gate_regs cg_regs_4_8_0 = {

These should all be:

static const struct mtk_gate_regs ...

> +       .set_ofs = 0x0004,
> +       .clr_ofs = 0x0008,
> +       .sta_ofs = 0x0000,
> +};
> +
> +#define GATE_IMG(_id, _name, _parent, _shift) {                        \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &cg_regs_4_8_0,                         \
> +               .shift = _shift,                                \
> +               .ops = &mtk_clk_gate_ops_setclr,                \
> +       }
> +
> +static struct mtk_gate img_clks[] __initdata = {

These should all be:

static const ... __initconst = {

> +       GATE_IMG(CLK_IMG_LARB2_SMI, "img_larb2_smi", "mm_sel", 0),
> +       GATE_IMG(CLK_IMG_CAM_SMI, "img_cam_smi", "mm_sel", 5),
> +       GATE_IMG(CLK_IMG_CAM_CAM, "img_cam_cam", "mm_sel", 6),
> +       GATE_IMG(CLK_IMG_SEN_TG, "img_sen_tg", "camtg_sel", 7),
> +       GATE_IMG(CLK_IMG_SEN_CAM, "img_sen_cam", "mm_sel", 8),
> +       GATE_IMG(CLK_IMG_CAM_SV, "img_cam_sv", "mm_sel", 9),
> +       GATE_IMG(CLK_IMG_FD, "img_fd", "mm_sel", 11),
> +};
> +
> +static struct mtk_gate_regs mm0_cg_regs = {
> +       .set_ofs = 0x0104,
> +       .clr_ofs = 0x0108,
> +       .sta_ofs = 0x0100,
> +};
> +
> +static struct mtk_gate_regs mm1_cg_regs = {
> +       .set_ofs = 0x0114,
> +       .clr_ofs = 0x0118,
> +       .sta_ofs = 0x0110,
> +};
> +
> +#define GATE_MM0(_id, _name, _parent, _shift) {                        \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &mm0_cg_regs,                           \
> +               .shift = _shift,                                \
> +               .ops = &mtk_clk_gate_ops_setclr,                \
> +       }
> +
> +#define GATE_MM1(_id, _name, _parent, _shift) {                        \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &mm1_cg_regs,                           \
> +               .shift = _shift,                                \
> +               .ops = &mtk_clk_gate_ops_setclr,                \
> +       }
> +
> +static struct mtk_gate mm_clks[] __initdata = {
> +       /* MM0 */
> +       GATE_MM0(CLK_MM_SMI_COMMON, "mm_smi_common", "mm_sel", 0),
> +       GATE_MM0(CLK_MM_SMI_LARB0, "mm_smi_larb0", "mm_sel", 1),
> +       GATE_MM0(CLK_MM_CAM_MDP, "mm_cam_mdp", "mm_sel", 2),
> +       GATE_MM0(CLK_MM_MDP_RDMA0, "mm_mdp_rdma0", "mm_sel", 3),
> +       GATE_MM0(CLK_MM_MDP_RDMA1, "mm_mdp_rdma1", "mm_sel", 4),
> +       GATE_MM0(CLK_MM_MDP_RSZ0, "mm_mdp_rsz0", "mm_sel", 5),
> +       GATE_MM0(CLK_MM_MDP_RSZ1, "mm_mdp_rsz1", "mm_sel", 6),
> +       GATE_MM0(CLK_MM_MDP_RSZ2, "mm_mdp_rsz2", "mm_sel", 7),
> +       GATE_MM0(CLK_MM_MDP_TDSHP0, "mm_mdp_tdshp0", "mm_sel", 8),
> +       GATE_MM0(CLK_MM_MDP_TDSHP1, "mm_mdp_tdshp1", "mm_sel", 9),
> +       GATE_MM0(CLK_MM_MDP_WDMA, "mm_mdp_wdma", "mm_sel", 11),
> +       GATE_MM0(CLK_MM_MDP_WROT0, "mm_mdp_wrot0", "mm_sel", 12),
> +       GATE_MM0(CLK_MM_MDP_WROT1, "mm_mdp_wrot1", "mm_sel", 13),
> +       GATE_MM0(CLK_MM_FAKE_ENG, "mm_fake_eng", "mm_sel", 14),
> +       GATE_MM0(CLK_MM_MUTEX_32K, "mm_mutex_32k", "rtc_sel", 15),
> +       GATE_MM0(CLK_MM_DISP_OVL0, "mm_disp_ovl0", "mm_sel", 16),
> +       GATE_MM0(CLK_MM_DISP_OVL1, "mm_disp_ovl1", "mm_sel", 17),
> +       GATE_MM0(CLK_MM_DISP_RDMA0, "mm_disp_rdma0", "mm_sel", 18),
> +       GATE_MM0(CLK_MM_DISP_RDMA1, "mm_disp_rdma1", "mm_sel", 19),
> +       GATE_MM0(CLK_MM_DISP_RDMA2, "mm_disp_rdma2", "mm_sel", 20),
> +       GATE_MM0(CLK_MM_DISP_WDMA0, "mm_disp_wdma0", "mm_sel", 21),
> +       GATE_MM0(CLK_MM_DISP_WDMA1, "mm_disp_wdma1", "mm_sel", 22),
> +       GATE_MM0(CLK_MM_DISP_COLOR0, "mm_disp_color0", "mm_sel", 23),
> +       GATE_MM0(CLK_MM_DISP_COLOR1, "mm_disp_color1", "mm_sel", 24),
> +       GATE_MM0(CLK_MM_DISP_AAL, "mm_disp_aal", "mm_sel", 25),
> +       GATE_MM0(CLK_MM_DISP_GAMMA, "mm_disp_gamma", "mm_sel", 26),
> +       GATE_MM0(CLK_MM_DISP_UFOE, "mm_disp_ufoe", "mm_sel", 27),
> +       GATE_MM0(CLK_MM_DISP_SPLIT0, "mm_disp_split0", "mm_sel", 28),
> +       GATE_MM0(CLK_MM_DISP_SPLIT1, "mm_disp_split1", "mm_sel", 29),
> +       GATE_MM0(CLK_MM_DISP_MERGE, "mm_disp_merge", "mm_sel", 30),
> +       GATE_MM0(CLK_MM_DISP_OD, "mm_disp_od", "mm_sel", 31),
> +       /* MM1 */
> +       GATE_MM1(CLK_MM_DISP_PWM0MM, "mm_disp_pwm0mm", "mm_sel", 0),
> +       GATE_MM1(CLK_MM_DISP_PWM026M, "mm_disp_pwm026m", "pwm_sel", 1),
> +       GATE_MM1(CLK_MM_DISP_PWM1MM, "mm_disp_pwm1mm", "mm_sel", 2),
> +       GATE_MM1(CLK_MM_DISP_PWM126M, "mm_disp_pwm126m", "pwm_sel", 3),
> +       GATE_MM1(CLK_MM_DSI0_ENGINE, "mm_dsi0_engine", "mm_sel", 4),
> +       GATE_MM1(CLK_MM_DSI0_DIGITAL, "mm_dsi0_digital", "clk_null", 5),
> +       GATE_MM1(CLK_MM_DSI1_ENGINE, "mm_dsi1_engine", "mm_sel", 6),
> +       GATE_MM1(CLK_MM_DSI1_DIGITAL, "mm_dsi1_digital", "clk_null", 7),
> +       GATE_MM1(CLK_MM_DPI_PIXEL, "mm_dpi_pixel", "dpi0_sel", 8),
> +       GATE_MM1(CLK_MM_DPI_ENGINE, "mm_dpi_engine", "mm_sel", 9),
> +       GATE_MM1(CLK_MM_DPI1_PIXEL, "mm_dpi1_pixel", "clk_null", 10),
> +       GATE_MM1(CLK_MM_DPI1_ENGINE, "mm_dpi1_engine", "mm_sel", 11),
> +       GATE_MM1(CLK_MM_HDMI_PIXEL, "mm_hdmi_pixel", "dpi0_sel", 12),
> +       GATE_MM1(CLK_MM_HDMI_PLLCK, "mm_hdmi_pllck", "hdmi_sel", 13),
> +       GATE_MM1(CLK_MM_HDMI_AUDIO, "mm_hdmi_audio", "apll1", 14),
> +       GATE_MM1(CLK_MM_HDMI_SPDIF, "mm_hdmi_spdif", "apll2", 15),
> +       GATE_MM1(CLK_MM_LVDS_PIXEL, "mm_lvds_pixel", "clk_null", 16),
> +       GATE_MM1(CLK_MM_LVDS_CTS, "mm_lvds_cts", "clk_null", 17),
> +       GATE_MM1(CLK_MM_SMI_LARB4, "mm_smi_larb4", "mm_sel", 18),
> +       GATE_MM1(CLK_MM_HDMI_HDCP, "mm_hdmi_hdcp", "hdcp_sel", 19),
> +       GATE_MM1(CLK_MM_HDMI_HDCP24M, "mm_hdmi_hdcp24m", "hdcp_24m_sel", 20),
> +};
> +
> +static struct mtk_gate_regs vdec0_cg_regs = {
> +       .set_ofs = 0x0000,
> +       .clr_ofs = 0x0004,
> +       .sta_ofs = 0x0000,
> +};
> +
> +static struct mtk_gate_regs vdec1_cg_regs = {
> +       .set_ofs = 0x0008,
> +       .clr_ofs = 0x000c,
> +       .sta_ofs = 0x0008,
> +};
> +
> +#define GATE_VDEC0(_id, _name, _parent, _shift) {              \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &vdec0_cg_regs,                         \
> +               .shift = _shift,                                \
> +               .ops = &mtk_clk_gate_ops_setclr_inv,            \
> +       }
> +
> +#define GATE_VDEC1(_id, _name, _parent, _shift) {              \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &vdec1_cg_regs,                         \
> +               .shift = _shift,                                \
> +               .ops = &mtk_clk_gate_ops_setclr_inv,            \
> +       }
> +
> +static struct mtk_gate vdec_clks[] __initdata = {
> +       GATE_VDEC0(CLK_VDEC_CKEN, "vdec_cken", "vdec_sel", 0),
> +       GATE_VDEC1(CLK_VDEC_LARB_CKEN, "vdec_larb_cken", "mm_sel", 0),
> +};
> +
> +#define GATE_VENC(_id, _name, _parent, _shift) {               \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &cg_regs_4_8_0,                         \
> +               .shift = _shift,                                \
> +               .ops = &mtk_clk_gate_ops_setclr_inv,            \
> +       }
> +
> +static struct mtk_gate venc_clks[] __initdata = {
> +       GATE_VENC(CLK_VENC_CKE0, "venc_cke0", "mm_sel", 0),
> +       GATE_VENC(CLK_VENC_CKE1, "venc_cke1", "venc_sel", 4),
> +       GATE_VENC(CLK_VENC_CKE2, "venc_cke2", "venc_sel", 8),
> +       GATE_VENC(CLK_VENC_CKE3, "venc_cke3", "venc_sel", 12),
> +};
> +
> +#define GATE_VENCLT(_id, _name, _parent, _shift) {             \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &cg_regs_4_8_0,                         \
> +               .shift = _shift,                                \
> +               .ops = &mtk_clk_gate_ops_setclr_inv,            \
> +       }
> +
> +static struct mtk_gate venclt_clks[] __initdata = {
> +       GATE_VENCLT(CLK_VENCLT_CKE0, "venclt_cke0", "mm_sel", 0),
> +       GATE_VENCLT(CLK_VENCLT_CKE1, "venclt_cke1", "venclt_sel", 4),
> +};
> +
>  static struct clk_onecell_data *mt8173_top_clk_data;
>  static struct clk_onecell_data *mt8173_pll_clk_data;
>
> @@ -844,3 +1021,124 @@ static void __init mtk_apmixedsys_init(struct device_node *node)
>  }
>  CLK_OF_DECLARE(mtk_apmixedsys, "mediatek,mt8173-apmixedsys",
>                 mtk_apmixedsys_init);
> +
> +static void __init mtk_imgsys_init(struct device_node *node)
> +{
> +       struct clk_onecell_data *clk_data;
> +       void __iomem *base;

I don't think you need base for any of these.
mtk_clk_register_gates() will use syscon_node_to_regmap() to lookup
the regmap by itself.

> +       int r;
> +
> +       base = of_iomap(node, 0);
> +       if (!base) {
> +               pr_err("%s(): ioremap failed\n", __func__);
> +               return;
> +       }
> +
> +       clk_data = mtk_alloc_clk_data(CLK_IMG_NR_CLK);

Unrelated to this patch, but:
I think each clock node should statically declare its
clk_onecell_data, and pass it to mtk_alloc_clk_data().
mtk_alloc_clk_data() should then just allocate and initialize the clks array.

> +
> +       mtk_clk_register_gates(node, img_clks, ARRAY_SIZE(img_clks),
> +                                               clk_data);
> +
> +       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);
> +}
> +CLK_OF_DECLARE(mtk_imgsys, "mediatek,mt8173-imgsys", mtk_imgsys_init);
> +
> +static void __init mtk_mmsys_init(struct device_node *node)
> +{
> +       struct clk_onecell_data *clk_data;
> +       void __iomem *base;
> +       int r;
> +
> +       base = of_iomap(node, 0);
> +       if (!base) {
> +               pr_err("%s(): ioremap failed\n", __func__);
> +               return;
> +       }
> +
> +       clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
> +
> +       mtk_clk_register_gates(node, mm_clks, ARRAY_SIZE(mm_clks),
> +                                               clk_data);
> +
> +       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);
> +}
> +CLK_OF_DECLARE(mtk_mmsys, "mediatek,mt8173-mmsys", mtk_mmsys_init);
> +
> +static void __init mtk_vdecsys_init(struct device_node *node)
> +{
> +       struct clk_onecell_data *clk_data;
> +       void __iomem *base;
> +       int r;
> +
> +       base = of_iomap(node, 0);
> +       if (!base) {
> +               pr_err("%s(): ioremap failed\n", __func__);
> +               return;
> +       }
> +
> +       clk_data = mtk_alloc_clk_data(CLK_VDEC_NR_CLK);
> +
> +       mtk_clk_register_gates(node, vdec_clks, ARRAY_SIZE(vdec_clks),
> +                                               clk_data);
> +
> +       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);
> +}
> +CLK_OF_DECLARE(mtk_vdecsys, "mediatek,mt8173-vdecsys", mtk_vdecsys_init);
> +
> +static void __init mtk_vencsys_init(struct device_node *node)
> +{
> +       struct clk_onecell_data *clk_data;
> +       void __iomem *base;
> +       int r;
> +
> +       base = of_iomap(node, 0);
> +       if (!base) {
> +               pr_err("%s(): ioremap failed\n", __func__);
> +               return;
> +       }
> +
> +       clk_data = mtk_alloc_clk_data(CLK_VENC_NR_CLK);
> +
> +       mtk_clk_register_gates(node, venc_clks, ARRAY_SIZE(venc_clks),
> +                                               clk_data);
> +
> +       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);
> +}
> +CLK_OF_DECLARE(mtk_vencsys, "mediatek,mt8173-vencsys", mtk_vencsys_init);
> +
> +static void __init mtk_vencltsys_init(struct device_node *node)
> +{
> +       struct clk_onecell_data *clk_data;
> +       void __iomem *base;
> +       int r;
> +
> +       base = of_iomap(node, 0);
> +       if (!base) {
> +               pr_err("%s(): ioremap failed\n", __func__);
> +               return;
> +       }
> +
> +       clk_data = mtk_alloc_clk_data(CLK_VENCLT_NR_CLK);
> +
> +       mtk_clk_register_gates(node, venclt_clks, ARRAY_SIZE(venclt_clks),
> +                                               clk_data);
> +
> +       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);
> +}
> +CLK_OF_DECLARE(mtk_vencltsys, "mediatek,mt8173-vencltsys", mtk_vencltsys_init);
> diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h
> index 4ad76ed..6ce88bf 100644
> --- a/include/dt-bindings/clock/mt8173-clk.h
> +++ b/include/dt-bindings/clock/mt8173-clk.h
> @@ -158,8 +158,8 @@
>
>  /* APMIXED_SYS */
>
> -#define CLK_APMIXED_ARMCA15PLL 1
> -#define CLK_APMIXED_ARMCA7PLL  2
> +#define CLK_APMIXED_ARMCA15PLL         1
> +#define CLK_APMIXED_ARMCA7PLL          2
>  #define CLK_APMIXED_MAINPLL            3
>  #define CLK_APMIXED_UNIVPLL            4
>  #define CLK_APMIXED_MMPLL              5
> @@ -232,4 +232,91 @@
>  #define CLK_PERI_UART3_SEL             39
>  #define CLK_PERI_NR_CLK                        40

Why do we count up from 1 instead of 0?
This means that for each clock-controller:
  clk_onecell_data->clk[0] == ERR_PTR(-ENOENT)


Thanks!
-Dan

>
> +/* IMG_SYS */
> +
> +#define CLK_IMG_LARB2_SMI              1
> +#define CLK_IMG_CAM_SMI                        2
> +#define CLK_IMG_CAM_CAM                        3
> +#define CLK_IMG_SEN_TG                 4
> +#define CLK_IMG_SEN_CAM                        5
> +#define CLK_IMG_CAM_SV                 6
> +#define CLK_IMG_FD                     7
> +#define CLK_IMG_NR_CLK                 8
> +
> +/* MM_SYS */
> +
> +#define CLK_MM_SMI_COMMON              1
> +#define CLK_MM_SMI_LARB0               2
> +#define CLK_MM_CAM_MDP                 3
> +#define CLK_MM_MDP_RDMA0               4
> +#define CLK_MM_MDP_RDMA1               5
> +#define CLK_MM_MDP_RSZ0                        6
> +#define CLK_MM_MDP_RSZ1                        7
> +#define CLK_MM_MDP_RSZ2                        8
> +#define CLK_MM_MDP_TDSHP0              9
> +#define CLK_MM_MDP_TDSHP1              10
> +#define CLK_MM_MDP_WDMA                        11
> +#define CLK_MM_MDP_WROT0               12
> +#define CLK_MM_MDP_WROT1               13
> +#define CLK_MM_FAKE_ENG                        14
> +#define CLK_MM_MUTEX_32K               15
> +#define CLK_MM_DISP_OVL0               16
> +#define CLK_MM_DISP_OVL1               17
> +#define CLK_MM_DISP_RDMA0              18
> +#define CLK_MM_DISP_RDMA1              19
> +#define CLK_MM_DISP_RDMA2              20
> +#define CLK_MM_DISP_WDMA0              21
> +#define CLK_MM_DISP_WDMA1              22
> +#define CLK_MM_DISP_COLOR0             23
> +#define CLK_MM_DISP_COLOR1             24
> +#define CLK_MM_DISP_AAL                        25
> +#define CLK_MM_DISP_GAMMA              26
> +#define CLK_MM_DISP_UFOE               27
> +#define CLK_MM_DISP_SPLIT0             28
> +#define CLK_MM_DISP_SPLIT1             29
> +#define CLK_MM_DISP_MERGE              30
> +#define CLK_MM_DISP_OD                 31
> +#define CLK_MM_DISP_PWM0MM             32
> +#define CLK_MM_DISP_PWM026M            33
> +#define CLK_MM_DISP_PWM1MM             34
> +#define CLK_MM_DISP_PWM126M            35
> +#define CLK_MM_DSI0_ENGINE             36
> +#define CLK_MM_DSI0_DIGITAL            37
> +#define CLK_MM_DSI1_ENGINE             38
> +#define CLK_MM_DSI1_DIGITAL            39
> +#define CLK_MM_DPI_PIXEL               40
> +#define CLK_MM_DPI_ENGINE              41
> +#define CLK_MM_DPI1_PIXEL              42
> +#define CLK_MM_DPI1_ENGINE             43
> +#define CLK_MM_HDMI_PIXEL              44
> +#define CLK_MM_HDMI_PLLCK              45
> +#define CLK_MM_HDMI_AUDIO              46
> +#define CLK_MM_HDMI_SPDIF              47
> +#define CLK_MM_LVDS_PIXEL              48
> +#define CLK_MM_LVDS_CTS                        49
> +#define CLK_MM_SMI_LARB4               50
> +#define CLK_MM_HDMI_HDCP               51
> +#define CLK_MM_HDMI_HDCP24M            52
> +#define CLK_MM_NR_CLK                  53
> +
> +/* VDEC_SYS */
> +
> +#define CLK_VDEC_CKEN                  1
> +#define CLK_VDEC_LARB_CKEN             2
> +#define CLK_VDEC_NR_CLK                        3
> +
> +/* VENC_SYS */
> +
> +#define CLK_VENC_CKE0                  1
> +#define CLK_VENC_CKE1                  2
> +#define CLK_VENC_CKE2                  3
> +#define CLK_VENC_CKE3                  4
> +#define CLK_VENC_NR_CLK                        5
> +
> +/* VENCLT_SYS */
> +
> +#define CLK_VENCLT_CKE0                        1
> +#define CLK_VENCLT_CKE1                        2
> +#define CLK_VENCLT_NR_CLK              3
> +
>  #endif /* _DT_BINDINGS_CLK_MT8173_H */
> --
> 1.8.1.1.dirty
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] clk: mediatek: Add subsystem clocks of MT8173

[James Liao]

On Wed, 2015-07-01 at 22:54 +0800, Daniel Kurtz wrote:
> On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao@mediatek.com> wrote:
> >
> > +static struct mtk_gate_regs cg_regs_4_8_0 = {
> 
> These should all be:
> 
> static const struct mtk_gate_regs ...

OK, I'll fix it.

> > +       .set_ofs = 0x0004,
> > +       .clr_ofs = 0x0008,
> > +       .sta_ofs = 0x0000,
> > +};
> > +
> > +#define GATE_IMG(_id, _name, _parent, _shift) {                        \
> > +               .id = _id,                                      \
> > +               .name = _name,                                  \
> > +               .parent_name = _parent,                         \
> > +               .regs = &cg_regs_4_8_0,                         \
> > +               .shift = _shift,                                \
> > +               .ops = &mtk_clk_gate_ops_setclr,                \
> > +       }
> > +
> > +static struct mtk_gate img_clks[] __initdata = {
> 
> These should all be:
> 
> static const ... __initconst = {

OK, I'll fix it.

> > +
> > +static void __init mtk_imgsys_init(struct device_node *node)
> > +{
> > +       struct clk_onecell_data *clk_data;
> > +       void __iomem *base;
> 
> I don't think you need base for any of these.
> mtk_clk_register_gates() will use syscon_node_to_regmap() to lookup
> the regmap by itself.

OK, I'll remove it.

> > +       int r;
> > +
> > +       base = of_iomap(node, 0);
> > +       if (!base) {
> > +               pr_err("%s(): ioremap failed\n", __func__);
> > +               return;
> > +       }
> > +
> > +       clk_data = mtk_alloc_clk_data(CLK_IMG_NR_CLK);
> 
> Unrelated to this patch, but:
> I think each clock node should statically declare its
> clk_onecell_data, and pass it to mtk_alloc_clk_data().
> mtk_alloc_clk_data() should then just allocate and initialize the clks array.

Is there any different from allocating clk_onecell_data and clks arary
dynamically?


> >  /* APMIXED_SYS */
> >
> > -#define CLK_APMIXED_ARMCA15PLL 1
> > -#define CLK_APMIXED_ARMCA7PLL  2
> > +#define CLK_APMIXED_ARMCA15PLL         1
> > +#define CLK_APMIXED_ARMCA7PLL          2
> >  #define CLK_APMIXED_MAINPLL            3
> >  #define CLK_APMIXED_UNIVPLL            4
> >  #define CLK_APMIXED_MMPLL              5
> > @@ -232,4 +232,91 @@
> >  #define CLK_PERI_UART3_SEL             39
> >  #define CLK_PERI_NR_CLK                        40
> 
> Why do we count up from 1 instead of 0?
> This means that for each clock-controller:
>   clk_onecell_data->clk[0] == ERR_PTR(-ENOENT)

It's a legacy implementation. I think it doesn't matter for function or
size in this implementation. We may change clock index to start from 0
in next SoC clock implementation.


Best regards,

James

Re: [PATCH v2 3/4] clk: mediatek: Add subsystem clocks of MT8173

[Daniel Kurtz]

On Fri, Jul 3, 2015 at 1:15 PM, James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> On Wed, 2015-07-01 at 22:54 +0800, Daniel Kurtz wrote:
>> On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>> >
>> > +static struct mtk_gate_regs cg_regs_4_8_0 = {
>>
>> These should all be:
>>
>> static const struct mtk_gate_regs ...
>
> OK, I'll fix it.
>
>> > +       .set_ofs = 0x0004,
>> > +       .clr_ofs = 0x0008,
>> > +       .sta_ofs = 0x0000,
>> > +};
>> > +
>> > +#define GATE_IMG(_id, _name, _parent, _shift) {                        \
>> > +               .id = _id,                                      \
>> > +               .name = _name,                                  \
>> > +               .parent_name = _parent,                         \
>> > +               .regs = &cg_regs_4_8_0,                         \
>> > +               .shift = _shift,                                \
>> > +               .ops = &mtk_clk_gate_ops_setclr,                \
>> > +       }
>> > +
>> > +static struct mtk_gate img_clks[] __initdata = {
>>
>> These should all be:
>>
>> static const ... __initconst = {
>
> OK, I'll fix it.
>
>> > +
>> > +static void __init mtk_imgsys_init(struct device_node *node)
>> > +{
>> > +       struct clk_onecell_data *clk_data;
>> > +       void __iomem *base;
>>
>> I don't think you need base for any of these.
>> mtk_clk_register_gates() will use syscon_node_to_regmap() to lookup
>> the regmap by itself.
>
> OK, I'll remove it.
>
>> > +       int r;
>> > +
>> > +       base = of_iomap(node, 0);
>> > +       if (!base) {
>> > +               pr_err("%s(): ioremap failed\n", __func__);
>> > +               return;
>> > +       }
>> > +
>> > +       clk_data = mtk_alloc_clk_data(CLK_IMG_NR_CLK);
>>
>> Unrelated to this patch, but:
>> I think each clock node should statically declare its
>> clk_onecell_data, and pass it to mtk_alloc_clk_data().
>> mtk_alloc_clk_data() should then just allocate and initialize the clks array.
>
> Is there any different from allocating clk_onecell_data and clks arary
> dynamically?

I generally believe it is always a bit better to statically allocate
memory where possible.  There would then be slightly less to do at
runtime (in this case on the critical boot path), and less churn on
the heap.  Each individual allocation probably doesn't matter very
much, but it adds up.

>> >  /* APMIXED_SYS */
>> >
>> > -#define CLK_APMIXED_ARMCA15PLL 1
>> > -#define CLK_APMIXED_ARMCA7PLL  2
>> > +#define CLK_APMIXED_ARMCA15PLL         1
>> > +#define CLK_APMIXED_ARMCA7PLL          2
>> >  #define CLK_APMIXED_MAINPLL            3
>> >  #define CLK_APMIXED_UNIVPLL            4
>> >  #define CLK_APMIXED_MMPLL              5
>> > @@ -232,4 +232,91 @@
>> >  #define CLK_PERI_UART3_SEL             39
>> >  #define CLK_PERI_NR_CLK                        40
>>
>> Why do we count up from 1 instead of 0?
>> This means that for each clock-controller:
>>   clk_onecell_data->clk[0] == ERR_PTR(-ENOENT)
>
> It's a legacy implementation. I think it doesn't matter for function or
> size in this implementation. We may change clock index to start from 0
> in next SoC clock implementation.

Ok.  I don't think it matters a lot.  Just curious.

Thanks!
-Dan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/4] clk: mediatek: Add USB clock support in MT8173 APMIXEDSYS

[James Liao]

Add REF2USB_TX clock support into MT8173 APMIXEDSYS. This clock
is needed by USB 3.0.

Signed-off-by: James Liao <jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/clk/mediatek/clk-mt8173.c      | 143 +++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-pll.c         |   7 +-
 include/dt-bindings/clock/mt8173-clk.h |   3 +-
 3 files changed, 146 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 75be757..333b6af 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -12,6 +12,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/delay.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/slab.h>
@@ -968,6 +969,141 @@ static void __init mtk_pericfg_init(struct device_node *node)
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg", mtk_pericfg_init);
 
+#define REF2USB_TX_EN		BIT(0)
+#define REF2USB_TX_LPF_EN	BIT(1)
+#define REF2USB_TX_OUT_EN	BIT(2)
+#define REF2USB_EN_MASK		(REF2USB_TX_EN | REF2USB_TX_LPF_EN | \
+				 REF2USB_TX_OUT_EN)
+
+struct mtk_ref2usb_tx {
+	struct clk_hw	hw;
+	void __iomem	*base_addr;
+};
+
+static inline struct mtk_ref2usb_tx *to_mtk_ref2usb_tx(struct clk_hw *hw)
+{
+	return container_of(hw, struct mtk_ref2usb_tx, hw);
+}
+
+static int mtk_ref2usb_tx_is_prepared(struct clk_hw *hw)
+{
+	struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
+
+	return (readl(tx->base_addr) & REF2USB_EN_MASK) == REF2USB_EN_MASK;
+}
+
+static int mtk_ref2usb_tx_prepare(struct clk_hw *hw)
+{
+	struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
+	u32 val;
+
+	val = readl(tx->base_addr);
+
+	val |= REF2USB_TX_EN;
+	writel(val, tx->base_addr);
+	udelay(100);
+
+	val |= REF2USB_TX_LPF_EN;
+	writel(val, tx->base_addr);
+
+	val |= REF2USB_TX_OUT_EN;
+	writel(val, tx->base_addr);
+
+	return 0;
+}
+
+static void mtk_ref2usb_tx_unprepare(struct clk_hw *hw)
+{
+	struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
+	u32 val;
+
+	val = readl(tx->base_addr);
+	val &= ~REF2USB_EN_MASK;
+	writel(val, tx->base_addr);
+}
+
+static const struct clk_ops mtk_ref2usb_tx_ops = {
+	.is_prepared	= mtk_ref2usb_tx_is_prepared,
+	.prepare	= mtk_ref2usb_tx_prepare,
+	.unprepare	= mtk_ref2usb_tx_unprepare,
+};
+
+static struct clk *mtk_clk_register_ref2usb_tx(const char *name,
+			const char *parent_name, void __iomem *reg)
+{
+	struct mtk_ref2usb_tx *tx;
+	struct clk_init_data init = {};
+	struct clk *clk;
+
+	tx = kzalloc(sizeof(*tx), GFP_KERNEL);
+	if (!tx)
+		return ERR_PTR(-ENOMEM);
+
+	tx->base_addr = reg;
+	tx->hw.init = &init;
+
+	init.name = name;
+	init.ops = &mtk_ref2usb_tx_ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	clk = clk_register(NULL, &tx->hw);
+
+	if (IS_ERR(clk)) {
+		pr_err("Failed to register clk %s: %ld\n", name, PTR_ERR(clk));
+		kfree(tx);
+	}
+
+	return clk;
+}
+
+struct mtk_apmixed_ex {
+	int id;
+	const char *name;
+	const char *parent;
+	u32 reg_ofs;
+};
+
+#define APMIXED_EX(_id, _name, _parent, _reg_ofs) {			\
+		.id = _id,						\
+		.name = _name,						\
+		.parent = _parent,					\
+		.reg_ofs = _reg_ofs,					\
+	}
+
+static const struct mtk_apmixed_ex apmixed_ex[] = {
+	APMIXED_EX(CLK_APMIXED_REF2USB_TX, "ref2usb_tx", "clk26m", 0x8),
+};
+
+static void __init mtk_clk_register_apmixedsys_special(struct device_node *node,
+			struct clk_onecell_data *clk_data)
+{
+	void __iomem *base;
+	struct clk *clk;
+	int i;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(apmixed_ex); i++) {
+		const struct mtk_apmixed_ex *ape = &apmixed_ex[i];
+
+		clk = mtk_clk_register_ref2usb_tx(ape->name, ape->parent,
+							base + ape->reg_ofs);
+
+		if (IS_ERR(clk)) {
+			pr_err("Failed to register clk %s: %ld\n", ape->name,
+					PTR_ERR(clk));
+			continue;
+		}
+
+		clk_data->clks[CLK_APMIXED_REF2USB_TX] = clk;
+	}
+}
+
 #define MT8173_PLL_FMAX		(3000UL * MHZ)
 
 #define CON0_MT8173_RST_BAR	BIT(24)
@@ -1010,12 +1146,19 @@ static const struct mtk_pll_data plls[] = {
 static void __init mtk_apmixedsys_init(struct device_node *node)
 {
 	struct clk_onecell_data *clk_data;
+	int r;
 
 	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);
+	mtk_clk_register_apmixedsys_special(node, clk_data);
+
+	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();
 }
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 44409e9..813f0c7 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -302,7 +302,7 @@ void __init mtk_clk_register_plls(struct device_node *node,
 		const struct mtk_pll_data *plls, int num_plls, struct clk_onecell_data *clk_data)
 {
 	void __iomem *base;
-	int r, i;
+	int i;
 	struct clk *clk;
 
 	base = of_iomap(node, 0);
@@ -324,9 +324,4 @@ void __init mtk_clk_register_plls(struct device_node *node,
 
 		clk_data->clks[pll->id] = clk;
 	}
-
-	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);
 }
diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h
index 6ce88bf..7b7c555 100644
--- a/include/dt-bindings/clock/mt8173-clk.h
+++ b/include/dt-bindings/clock/mt8173-clk.h
@@ -172,7 +172,8 @@
 #define CLK_APMIXED_APLL2		12
 #define CLK_APMIXED_LVDSPLL		13
 #define CLK_APMIXED_MSDCPLL2		14
-#define CLK_APMIXED_NR_CLK		15
+#define CLK_APMIXED_REF2USB_TX		15
+#define CLK_APMIXED_NR_CLK		16
 
 /* INFRA_SYS */
 
-- 
1.8.1.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] clk: mediatek: Add USB clock support in MT8173 APMIXEDSYS

[Daniel Kurtz]

[-- Attachment #1: Type: text/plain, Size: 8184 bytes --]

Hi James,

This looks like 3 separate gate clocks in a chain, with a timing
constraint: USB_LPF must be enabled 100 us after USB_TX.

  26MHz--> [GATE] --USB_TX--> [LPF] --USB_LPF--> [GATE] --USB_OUT-->
            ^                  ^                  ^
            +--------------+   |                  |
AP_PLL_CON2.REF2USB_TX_EN -+   |                  |
AP_PLL_CON2.REF2USB_TX_LPF_EN -+                  |
AP_PLL_CON2.REF2USB_TX_OUT_EN --------------------+


I think we can model the gate parts using a proper clock tree model and the
existing clock gate semantics.
I'm not sure the best way to model the delay; but in theory that could be
handled by the clock user (USB driver).

-Dan

On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao@mediatek.com>
wrote:
> Add REF2USB_TX clock support into MT8173 APMIXEDSYS. This clock
> is needed by USB 3.0.
>
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> ---
>  drivers/clk/mediatek/clk-mt8173.c      | 143
+++++++++++++++++++++++++++++++++
>  drivers/clk/mediatek/clk-pll.c         |   7 +-
>  include/dt-bindings/clock/mt8173-clk.h |   3 +-
>  3 files changed, 146 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8173.c
b/drivers/clk/mediatek/clk-mt8173.c
> index 75be757..333b6af 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -12,6 +12,7 @@
>   * GNU General Public License for more details.
>   */
>
> +#include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/slab.h>
> @@ -968,6 +969,141 @@ static void __init mtk_pericfg_init(struct
device_node *node)
>  }
>  CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg", mtk_pericfg_init);
>
> +#define REF2USB_TX_EN          BIT(0)
> +#define REF2USB_TX_LPF_EN      BIT(1)
> +#define REF2USB_TX_OUT_EN      BIT(2)
> +#define REF2USB_EN_MASK                (REF2USB_TX_EN |
REF2USB_TX_LPF_EN | \
> +                                REF2USB_TX_OUT_EN)
> +
> +struct mtk_ref2usb_tx {
> +       struct clk_hw   hw;
> +       void __iomem    *base_addr;
> +};
> +
> +static inline struct mtk_ref2usb_tx *to_mtk_ref2usb_tx(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct mtk_ref2usb_tx, hw);
> +}
> +
> +static int mtk_ref2usb_tx_is_prepared(struct clk_hw *hw)
> +{
> +       struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
> +
> +       return (readl(tx->base_addr) & REF2USB_EN_MASK) ==
REF2USB_EN_MASK;
> +}
> +
> +static int mtk_ref2usb_tx_prepare(struct clk_hw *hw)
> +{
> +       struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
> +       u32 val;
> +
> +       val = readl(tx->base_addr);
> +
> +       val |= REF2USB_TX_EN;
> +       writel(val, tx->base_addr);
> +       udelay(100);
> +
> +       val |= REF2USB_TX_LPF_EN;
> +       writel(val, tx->base_addr);
> +
> +       val |= REF2USB_TX_OUT_EN;
> +       writel(val, tx->base_addr);
> +
> +       return 0;
> +}
> +
> +static void mtk_ref2usb_tx_unprepare(struct clk_hw *hw)
> +{
> +       struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
> +       u32 val;
> +
> +       val = readl(tx->base_addr);
> +       val &= ~REF2USB_EN_MASK;
> +       writel(val, tx->base_addr);
> +}
> +
> +static const struct clk_ops mtk_ref2usb_tx_ops = {
> +       .is_prepared    = mtk_ref2usb_tx_is_prepared,
> +       .prepare        = mtk_ref2usb_tx_prepare,
> +       .unprepare      = mtk_ref2usb_tx_unprepare,
> +};
> +
> +static struct clk *mtk_clk_register_ref2usb_tx(const char *name,
> +                       const char *parent_name, void __iomem *reg)
> +{
> +       struct mtk_ref2usb_tx *tx;
> +       struct clk_init_data init = {};
> +       struct clk *clk;
> +
> +       tx = kzalloc(sizeof(*tx), GFP_KERNEL);
> +       if (!tx)
> +               return ERR_PTR(-ENOMEM);
> +
> +       tx->base_addr = reg;
> +       tx->hw.init = &init;
> +
> +       init.name = name;
> +       init.ops = &mtk_ref2usb_tx_ops;
> +       init.parent_names = &parent_name;
> +       init.num_parents = 1;
> +
> +       clk = clk_register(NULL, &tx->hw);
> +
> +       if (IS_ERR(clk)) {
> +               pr_err("Failed to register clk %s: %ld\n", name,
PTR_ERR(clk));
> +               kfree(tx);
> +       }
> +
> +       return clk;
> +}
> +
> +struct mtk_apmixed_ex {
> +       int id;
> +       const char *name;
> +       const char *parent;
> +       u32 reg_ofs;
> +};
> +
> +#define APMIXED_EX(_id, _name, _parent, _reg_ofs) {                    \
> +               .id = _id,                                              \
> +               .name = _name,                                          \
> +               .parent = _parent,                                      \
> +               .reg_ofs = _reg_ofs,                                    \
> +       }
> +
> +static const struct mtk_apmixed_ex apmixed_ex[] = {
> +       APMIXED_EX(CLK_APMIXED_REF2USB_TX, "ref2usb_tx", "clk26m", 0x8),
> +};
> +
> +static void __init mtk_clk_register_apmixedsys_special(struct
device_node *node,
> +                       struct clk_onecell_data *clk_data)
> +{
> +       void __iomem *base;
> +       struct clk *clk;
> +       int i;
> +
> +       base = of_iomap(node, 0);
> +       if (!base) {
> +               pr_err("%s(): ioremap failed\n", __func__);
> +               return;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(apmixed_ex); i++) {
> +               const struct mtk_apmixed_ex *ape = &apmixed_ex[i];
> +
> +               clk = mtk_clk_register_ref2usb_tx(ape->name, ape->parent,
> +                                                       base +
ape->reg_ofs);
> +
> +               if (IS_ERR(clk)) {
> +                       pr_err("Failed to register clk %s: %ld\n",
ape->name,
> +                                       PTR_ERR(clk));
> +                       continue;
> +               }
> +
> +               clk_data->clks[CLK_APMIXED_REF2USB_TX] = clk;
> +       }
> +}
> +
>  #define MT8173_PLL_FMAX                (3000UL * MHZ)
>
>  #define CON0_MT8173_RST_BAR    BIT(24)
> @@ -1010,12 +1146,19 @@ static const struct mtk_pll_data plls[] = {
>  static void __init mtk_apmixedsys_init(struct device_node *node)
>  {
>         struct clk_onecell_data *clk_data;
> +       int r;
>
>         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);
> +       mtk_clk_register_apmixedsys_special(node, clk_data);
> +
> +       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();
>  }
> diff --git a/drivers/clk/mediatek/clk-pll.c
b/drivers/clk/mediatek/clk-pll.c
> index 44409e9..813f0c7 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -302,7 +302,7 @@ void __init mtk_clk_register_plls(struct device_node
*node,
>                 const struct mtk_pll_data *plls, int num_plls, struct
clk_onecell_data *clk_data)
>  {
>         void __iomem *base;
> -       int r, i;
> +       int i;
>         struct clk *clk;
>
>         base = of_iomap(node, 0);
> @@ -324,9 +324,4 @@ void __init mtk_clk_register_plls(struct device_node
*node,
>
>                 clk_data->clks[pll->id] = clk;
>         }
> -
> -       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);
>  }
> diff --git a/include/dt-bindings/clock/mt8173-clk.h
b/include/dt-bindings/clock/mt8173-clk.h
> index 6ce88bf..7b7c555 100644
> --- a/include/dt-bindings/clock/mt8173-clk.h
> +++ b/include/dt-bindings/clock/mt8173-clk.h
> @@ -172,7 +172,8 @@
>  #define CLK_APMIXED_APLL2              12
>  #define CLK_APMIXED_LVDSPLL            13
>  #define CLK_APMIXED_MSDCPLL2           14
> -#define CLK_APMIXED_NR_CLK             15
> +#define CLK_APMIXED_REF2USB_TX         15
> +#define CLK_APMIXED_NR_CLK             16
>
>  /* INFRA_SYS */
>
> --
> 1.8.1.1.dirty
>

[-- Attachment #2: Type: text/html, Size: 11750 bytes --]

Re: [PATCH v2 4/4] clk: mediatek: Add USB clock support in MT8173 APMIXEDSYS

[Daniel Kurtz]

Hi James,

This looks like 3 separate gate clocks in a chain, with a timing
constraint: USB_LPF must be enabled 100 us after USB_TX.

  26MHz--> [GATE] --USB_TX--> [LPF] --USB_LPF--> [GATE] --USB_OUT-->
            ^                  ^                  ^
            +--------------+   |                  |
AP_PLL_CON2.REF2USB_TX_EN -+   |                  |
AP_PLL_CON2.REF2USB_TX_LPF_EN -+                  |
AP_PLL_CON2.REF2USB_TX_OUT_EN --------------------+


I think we can model the gate parts using a proper clock tree model
and the existing clock gate semantics.
I'm not sure the best way to model the delay; but in theory that could
be handled by the clock user (USB driver).

-Dan

On Wed, Jul 1, 2015 at 11:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Hi James,
>
> This looks like 3 separate gate clocks in a chain, with a timing constraint:
> USB_LPF must be enabled 100 us after USB_TX.
>
>   26MHz--> [GATE] --USB_TX--> [LPF] --USB_LPF--> [GATE] --USB_OUT-->
>             ^                  ^                  ^
>             +--------------+   |                  |
> AP_PLL_CON2.REF2USB_TX_EN -+   |                  |
> AP_PLL_CON2.REF2USB_TX_LPF_EN -+                  |
> AP_PLL_CON2.REF2USB_TX_OUT_EN --------------------+
>
>
> I think we can model the gate parts using a proper clock tree model and the
> existing clock gate semantics.
> I'm not sure the best way to model the delay; but in theory that could be
> handled by the clock user (USB driver).
>
> -Dan
>
> On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao@mediatek.com>
> wrote:
>> Add REF2USB_TX clock support into MT8173 APMIXEDSYS. This clock
>> is needed by USB 3.0.
>>
>> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
>> ---
>>  drivers/clk/mediatek/clk-mt8173.c      | 143
>> +++++++++++++++++++++++++++++++++
>>  drivers/clk/mediatek/clk-pll.c         |   7 +-
>>  include/dt-bindings/clock/mt8173-clk.h |   3 +-
>>  3 files changed, 146 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8173.c
>> b/drivers/clk/mediatek/clk-mt8173.c
>> index 75be757..333b6af 100644
>> --- a/drivers/clk/mediatek/clk-mt8173.c
>> +++ b/drivers/clk/mediatek/clk-mt8173.c
>> @@ -12,6 +12,7 @@
>>   * GNU General Public License for more details.
>>   */
>>
>> +#include <linux/delay.h>
>>  #include <linux/of.h>
>>  #include <linux/of_address.h>
>>  #include <linux/slab.h>
>> @@ -968,6 +969,141 @@ static void __init mtk_pericfg_init(struct
>> device_node *node)
>>  }
>>  CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg", mtk_pericfg_init);
>>
>> +#define REF2USB_TX_EN          BIT(0)
>> +#define REF2USB_TX_LPF_EN      BIT(1)
>> +#define REF2USB_TX_OUT_EN      BIT(2)
>> +#define REF2USB_EN_MASK                (REF2USB_TX_EN | REF2USB_TX_LPF_EN
>> | \
>> +                                REF2USB_TX_OUT_EN)
>> +
>> +struct mtk_ref2usb_tx {
>> +       struct clk_hw   hw;
>> +       void __iomem    *base_addr;
>> +};
>> +
>> +static inline struct mtk_ref2usb_tx *to_mtk_ref2usb_tx(struct clk_hw *hw)
>> +{
>> +       return container_of(hw, struct mtk_ref2usb_tx, hw);
>> +}
>> +
>> +static int mtk_ref2usb_tx_is_prepared(struct clk_hw *hw)
>> +{
>> +       struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
>> +
>> +       return (readl(tx->base_addr) & REF2USB_EN_MASK) ==
>> REF2USB_EN_MASK;
>> +}
>> +
>> +static int mtk_ref2usb_tx_prepare(struct clk_hw *hw)
>> +{
>> +       struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
>> +       u32 val;
>> +
>> +       val = readl(tx->base_addr);
>> +
>> +       val |= REF2USB_TX_EN;
>> +       writel(val, tx->base_addr);
>> +       udelay(100);
>> +
>> +       val |= REF2USB_TX_LPF_EN;
>> +       writel(val, tx->base_addr);
>> +
>> +       val |= REF2USB_TX_OUT_EN;
>> +       writel(val, tx->base_addr);
>> +
>> +       return 0;
>> +}
>> +
>> +static void mtk_ref2usb_tx_unprepare(struct clk_hw *hw)
>> +{
>> +       struct mtk_ref2usb_tx *tx = to_mtk_ref2usb_tx(hw);
>> +       u32 val;
>> +
>> +       val = readl(tx->base_addr);
>> +       val &= ~REF2USB_EN_MASK;
>> +       writel(val, tx->base_addr);
>> +}
>> +
>> +static const struct clk_ops mtk_ref2usb_tx_ops = {
>> +       .is_prepared    = mtk_ref2usb_tx_is_prepared,
>> +       .prepare        = mtk_ref2usb_tx_prepare,
>> +       .unprepare      = mtk_ref2usb_tx_unprepare,
>> +};
>> +
>> +static struct clk *mtk_clk_register_ref2usb_tx(const char *name,
>> +                       const char *parent_name, void __iomem *reg)
>> +{
>> +       struct mtk_ref2usb_tx *tx;
>> +       struct clk_init_data init = {};
>> +       struct clk *clk;
>> +
>> +       tx = kzalloc(sizeof(*tx), GFP_KERNEL);
>> +       if (!tx)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       tx->base_addr = reg;
>> +       tx->hw.init = &init;
>> +
>> +       init.name = name;
>> +       init.ops = &mtk_ref2usb_tx_ops;
>> +       init.parent_names = &parent_name;
>> +       init.num_parents = 1;
>> +
>> +       clk = clk_register(NULL, &tx->hw);
>> +
>> +       if (IS_ERR(clk)) {
>> +               pr_err("Failed to register clk %s: %ld\n", name,
>> PTR_ERR(clk));
>> +               kfree(tx);
>> +       }
>> +
>> +       return clk;
>> +}
>> +
>> +struct mtk_apmixed_ex {
>> +       int id;
>> +       const char *name;
>> +       const char *parent;
>> +       u32 reg_ofs;
>> +};
>> +
>> +#define APMIXED_EX(_id, _name, _parent, _reg_ofs) {                    \
>> +               .id = _id,                                              \
>> +               .name = _name,                                          \
>> +               .parent = _parent,                                      \
>> +               .reg_ofs = _reg_ofs,                                    \
>> +       }
>> +
>> +static const struct mtk_apmixed_ex apmixed_ex[] = {
>> +       APMIXED_EX(CLK_APMIXED_REF2USB_TX, "ref2usb_tx", "clk26m", 0x8),
>> +};
>> +
>> +static void __init mtk_clk_register_apmixedsys_special(struct device_node
>> *node,
>> +                       struct clk_onecell_data *clk_data)
>> +{
>> +       void __iomem *base;
>> +       struct clk *clk;
>> +       int i;
>> +
>> +       base = of_iomap(node, 0);
>> +       if (!base) {
>> +               pr_err("%s(): ioremap failed\n", __func__);
>> +               return;
>> +       }
>> +
>> +       for (i = 0; i < ARRAY_SIZE(apmixed_ex); i++) {
>> +               const struct mtk_apmixed_ex *ape = &apmixed_ex[i];
>> +
>> +               clk = mtk_clk_register_ref2usb_tx(ape->name, ape->parent,
>> +                                                       base +
>> ape->reg_ofs);
>> +
>> +               if (IS_ERR(clk)) {
>> +                       pr_err("Failed to register clk %s: %ld\n",
>> ape->name,
>> +                                       PTR_ERR(clk));
>> +                       continue;
>> +               }
>> +
>> +               clk_data->clks[CLK_APMIXED_REF2USB_TX] = clk;
>> +       }
>> +}
>> +
>>  #define MT8173_PLL_FMAX                (3000UL * MHZ)
>>
>>  #define CON0_MT8173_RST_BAR    BIT(24)
>> @@ -1010,12 +1146,19 @@ static const struct mtk_pll_data plls[] = {
>>  static void __init mtk_apmixedsys_init(struct device_node *node)
>>  {
>>         struct clk_onecell_data *clk_data;
>> +       int r;
>>
>>         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);
>> +       mtk_clk_register_apmixedsys_special(node, clk_data);
>> +
>> +       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();
>>  }
>> diff --git a/drivers/clk/mediatek/clk-pll.c
>> b/drivers/clk/mediatek/clk-pll.c
>> index 44409e9..813f0c7 100644
>> --- a/drivers/clk/mediatek/clk-pll.c
>> +++ b/drivers/clk/mediatek/clk-pll.c
>> @@ -302,7 +302,7 @@ void __init mtk_clk_register_plls(struct device_node
>> *node,
>>                 const struct mtk_pll_data *plls, int num_plls, struct
>> clk_onecell_data *clk_data)
>>  {
>>         void __iomem *base;
>> -       int r, i;
>> +       int i;
>>         struct clk *clk;
>>
>>         base = of_iomap(node, 0);
>> @@ -324,9 +324,4 @@ void __init mtk_clk_register_plls(struct device_node
>> *node,
>>
>>                 clk_data->clks[pll->id] = clk;
>>         }
>> -
>> -       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);
>>  }
>> diff --git a/include/dt-bindings/clock/mt8173-clk.h
>> b/include/dt-bindings/clock/mt8173-clk.h
>> index 6ce88bf..7b7c555 100644
>> --- a/include/dt-bindings/clock/mt8173-clk.h
>> +++ b/include/dt-bindings/clock/mt8173-clk.h
>> @@ -172,7 +172,8 @@
>>  #define CLK_APMIXED_APLL2              12
>>  #define CLK_APMIXED_LVDSPLL            13
>>  #define CLK_APMIXED_MSDCPLL2           14
>> -#define CLK_APMIXED_NR_CLK             15
>> +#define CLK_APMIXED_REF2USB_TX         15
>> +#define CLK_APMIXED_NR_CLK             16
>>
>>  /* INFRA_SYS */
>>
>> --
>> 1.8.1.1.dirty
>>
>

Re: [PATCH v2 4/4] clk: mediatek: Add USB clock support in MT8173 APMIXEDSYS

[James Liao]

Hi Daniel,

On Wed, 2015-07-01 at 23:22 +0800, Daniel Kurtz wrote:
> This looks like 3 separate gate clocks in a chain, with a timing
> constraint: USB_LPF must be enabled 100 us after USB_TX.
> 
>   26MHz--> [GATE] --USB_TX--> [LPF] --USB_LPF--> [GATE] --USB_OUT-->
>             ^                  ^                  ^
>             +--------------+   |                  |
> AP_PLL_CON2.REF2USB_TX_EN -+   |                  |
> AP_PLL_CON2.REF2USB_TX_LPF_EN -+                  |
> AP_PLL_CON2.REF2USB_TX_OUT_EN --------------------+
> 
> 
> I think we can model the gate parts using a proper clock tree model
> and the existing clock gate semantics.
> I'm not sure the best way to model the delay; but in theory that could
> be handled by the clock user (USB driver).

Do you mean to create 3 hierarchical clocks (may be clock gates) to
model these clocks as the following ?

EN -- LPF -- OUT_EN

(EN is the parent of LPF, and LPF is the parent of EN)

If we model these 3 clocks like above, we can't prevent clock users to
enable OUT_EN directly, and there will be no delay between EN and LPF.

Or you have other suggestions to model these 3 clcoks?


Best regards,

James

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] clk: mediatek: Add USB clock support in MT8173 APMIXEDSYS

[Daniel Kurtz]

On Fri, Jul 3, 2015 at 1:38 PM, James Liao <jamesjj.liao@mediatek.com> wrote:
> Hi Daniel,
>
> On Wed, 2015-07-01 at 23:22 +0800, Daniel Kurtz wrote:
>> This looks like 3 separate gate clocks in a chain, with a timing
>> constraint: USB_LPF must be enabled 100 us after USB_TX.
>>
>>   26MHz--> [GATE] --USB_TX--> [LPF] --USB_LPF--> [GATE] --USB_OUT-->
>>             ^                  ^                  ^
>>             +--------------+   |                  |
>> AP_PLL_CON2.REF2USB_TX_EN -+   |                  |
>> AP_PLL_CON2.REF2USB_TX_LPF_EN -+                  |
>> AP_PLL_CON2.REF2USB_TX_OUT_EN --------------------+
>>
>>
>> I think we can model the gate parts using a proper clock tree model
>> and the existing clock gate semantics.
>> I'm not sure the best way to model the delay; but in theory that could
>> be handled by the clock user (USB driver).
>
> Do you mean to create 3 hierarchical clocks (may be clock gates) to
> model these clocks as the following ?
>
> EN -- LPF -- OUT_EN
>
> (EN is the parent of LPF, and LPF is the parent of EN)
>
> If we model these 3 clocks like above, we can't prevent clock users to
> enable OUT_EN directly, and there will be no delay between EN and LPF.
>
> Or you have other suggestions to model these 3 clcoks?

I don't really know.
I was hoping someone would know of a way to add a "delay" when
enabling a clock gate. :-)
Why do we need the delay anyway?
Is the delay really between LPF and OUT_EN?
Or, is the delay between enabling OUT_EN and when the USB block can
use the clock?

Some ideas:
(1) Handled the LPF delay in the USB driver itself:
  (a) if the delay is really between OUT_EN and when it is used:
  /* Enabling USB_OUT will enable its parents, USB_LPF and USB_TX */
  clk_enable(clk_usb_out);
  udelay();

  (b) Or if the delay is really needed between LPF and OUT, this can
be implemented in the USB driver:

  /* Enable LPF and its parent USB_TX */
  clk_enable(clk_lpf);
  udelay();
  clk_enable(clk_usb_out);

 (2) Add a "delay" property to "struct mtk_clk_gate", with a non-zero
value for just this one special LPF clock.  The downside is then every
gate clock would have to carry this extra field.

 (3) Add a new clk type, struct mtk_clk_delayed_gate, that includes a
struct mtk_clk_gate, plus a delay.  I would add this to its own file,
or maybe clk-gate.c, though, rather than mediatek/clk-mt8173.c.  And
then use this for instantiating the LPF clock.

-Dan



> Best regards,
>
> James
>