[PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[James Liao]

This patchset contains subsystem clocks support for Mediatek MT8173.
It also contains some bug fixes before adds new clocks support.

James Liao (4):
  clk: mediatek: Fix apmixedsys clock registration
  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-mt8135.c                  |   2 +-
 drivers/clk/mediatek/clk-mt8173.c                  | 454 ++++++++++++++++++++-
 drivers/clk/mediatek/clk-pll.c                     |   7 +-
 include/dt-bindings/clock/mt8173-clk.h             |  94 ++++-
 9 files changed, 652 insertions(+), 15 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

[PATCH 1/5] clk: mediatek: Fix apmixedsys clock registration

[James Liao]

The size of clk_data should be the same as CLK_APMIXED_NR_CLK
instead of ARRAY_SIZE(plls). CLK_APMIXED_* is numbered from 1, so
CLK_APMIXED_NR_CLK will be greater than ARRAY_SIZE(plls).

Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
---
 drivers/clk/mediatek/clk-mt8135.c | 2 +-
 drivers/clk/mediatek/clk-mt8173.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8135.c b/drivers/clk/mediatek/clk-mt8135.c
index a63435b..08b4b84 100644
--- a/drivers/clk/mediatek/clk-mt8135.c
+++ b/drivers/clk/mediatek/clk-mt8135.c
@@ -634,7 +634,7 @@ static void __init mtk_apmixedsys_init(struct device_node *node)
 {
 	struct clk_onecell_data *clk_data;
 
-	clk_data = mtk_alloc_clk_data(ARRAY_SIZE(plls));
+	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
 	if (!clk_data)
 		return;
 
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 357b080..4b9e04c 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -818,7 +818,7 @@ static void __init mtk_apmixedsys_init(struct device_node *node)
 {
 	struct clk_onecell_data *clk_data;
 
-	clk_data = mtk_alloc_clk_data(ARRAY_SIZE(plls));
+	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
 	if (!clk_data)
 		return;
 
-- 
1.8.1.1.dirty

Re: [PATCH 1/5] clk: mediatek: Fix apmixedsys clock registration

[Sascha Hauer]

On Thu, May 21, 2015 at 03:12:52PM +0800, James Liao wrote:
> The size of clk_data should be the same as CLK_APMIXED_NR_CLK
> instead of ARRAY_SIZE(plls). CLK_APMIXED_* is numbered from 1, so
> CLK_APMIXED_NR_CLK will be greater than ARRAY_SIZE(plls).
> 
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>

Acked-by: Sascha Hauer <s.hauer@pengutronix.de>

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/5] clk: mediatek: Fix apmixedsys clock registration

[Stephen Boyd]

On 05/21, James Liao wrote:
> The size of clk_data should be the same as CLK_APMIXED_NR_CLK
> instead of ARRAY_SIZE(plls). CLK_APMIXED_* is numbered from 1, so
> CLK_APMIXED_NR_CLK will be greater than ARRAY_SIZE(plls).
> 
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/5] clk: mediatek: mt8173: Fix enabling of critical clocks

[James Liao]

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

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

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

diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 4b9e04c..eb175ac 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -700,6 +700,20 @@ 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_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 +726,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 +832,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

Re: [PATCH 2/5] clk: mediatek: mt8173: Fix enabling of critical clocks

[Sascha Hauer]

On Thu, May 21, 2015 at 03:12:53PM +0800, James Liao wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> On the MT8173 the clocks are provided by different units. To enable
> the critical clocks we must be sure that all parent clocks are already
> registered, otherwise the parents of the critical clocks end up being
> unused and get disabled later. To find a place where all parents are
> registered we try each time after we've registered some clocks if
> all known providers are present now and only then we enable the critical
> clocks
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> ---
>  drivers/clk/mediatek/clk-mt8173.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
> index 4b9e04c..eb175ac 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -700,6 +700,20 @@ 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_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]);

Is CLK_TOP_RTC_SEL really a critical clock?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/5] clk: mediatek: mt8173: Fix enabling of critical clocks

[James Liao]

On Tue, 2015-05-26 at 09:46 +0200, Sascha Hauer wrote:
> > +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_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]);
> 
> Is CLK_TOP_RTC_SEL really a critical clock?

CLK_TOP_RTC_SEL is the main 32k clock used by some system hardware such
sleep controller on MT8173. This clock should not be turned off even
when software/CPU is sleeping. So it's a better way to set
CLK_TOP_RTC_SEL as a critical clock (an always on clock).


Best regards,

James

[PATCH 3/5] 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 3/5] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

[Sascha Hauer]

On Thu, May 21, 2015 at 03:12:54PM +0800, James Liao wrote:
> This adds the binding documentation for the mmsys, imgsys, vdecsys,
> vencsys and vencltsys controllers found on Mediatek SoCs.
> 
> index 0000000..a5b94a7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt

Do these really become multiple drivers so that it's worth abstracting
them in the clock framework?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/5] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

[James Liao]

Hi Sascha,

On Tue, 2015-05-26 at 09:56 +0200, Sascha Hauer wrote:
> On Thu, May 21, 2015 at 03:12:54PM +0800, James Liao wrote:
> > This adds the binding documentation for the mmsys, imgsys, vdecsys,
> > vencsys and vencltsys controllers found on Mediatek SoCs.
> > 
> > index 0000000..a5b94a7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
> > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
> > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt
> 
> Do these really become multiple drivers so that it's worth abstracting
> them in the clock framework?

These clocks need to be controlled among several drivers. For example,
vdecsys clocks will be controlled by VDEC driver (not ready yet) and
MT8173 SMI driver [1]. That means these clocks need a mechanism to share
between these 2 drivers. CCF share clocks by using of reference count,
so I think it's suitable to implement these subsystem clocks.

As I know SMI driver need to access clocks among mmsys, imgsys, vdecsys,
vencsys and vencltsys. So in this patch I added clocks of these
subsystems into CCF.

[1]
http://lists.infradead.org/pipermail/linux-mediatek/2015-March/000058.html


Best regards,

James

Re: [PATCH 3/5] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

[Sascha Hauer]

On Tue, May 26, 2015 at 04:55:36PM +0800, James Liao wrote:
> Hi Sascha,
> 
> On Tue, 2015-05-26 at 09:56 +0200, Sascha Hauer wrote:
> > On Thu, May 21, 2015 at 03:12:54PM +0800, James Liao wrote:
> > > This adds the binding documentation for the mmsys, imgsys, vdecsys,
> > > vencsys and vencltsys controllers found on Mediatek SoCs.
> > > 
> > > index 0000000..a5b94a7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
> > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
> > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt
> > 
> > Do these really become multiple drivers so that it's worth abstracting
> > them in the clock framework?
> 
> These clocks need to be controlled among several drivers. For example,
> vdecsys clocks will be controlled by VDEC driver (not ready yet) and
> MT8173 SMI driver [1]. That means these clocks need a mechanism to share
> between these 2 drivers. CCF share clocks by using of reference count,
> so I think it's suitable to implement these subsystem clocks.
> 
> As I know SMI driver need to access clocks among mmsys, imgsys, vdecsys,
> vencsys and vencltsys. So in this patch I added clocks of these
> subsystems into CCF.
> 
> [1]
> http://lists.infradead.org/pipermail/linux-mediatek/2015-March/000058.html

Looking at the 3.18 tree we have this:

	vdecsys: vdecsys@16000000 {
		compatible = "mediatek,mt8173-vdecsys", "syscon";
		reg = <0 0x16000000 0 0x1000>;
		#clock-cells = <1>;
	};

	larb1:larb@16010000 {
		compatible = "mediatek,mt8173-smi-larb";
		reg = <0 0x16010000 0 0x1000>;
		clocks = <&mmsys MM_SMI_COMMON>,
			 <&vdecsys VDEC_CKEN>,
			 <&vdecsys VDEC_LARB_CKEN>;
		clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
	};

I believe that the larb needs the MM_SMI_COMMON clock to modify the larb
registers, but is it really necessary to enable VDEC_CKEN and
VDEC_LARB_CKEN just to set the F_SMI_MMU_EN bit in the larb?

With the above we have the situation that the vdec driver calls into the
iommu driver which then calls into the larb driver which calls back into
the vdec driver via the clk API. This seems very suspicious.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/5] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

[Yong Wu]

On Tue, 2015-05-26 at 13:08 +0200, Sascha Hauer wrote:
> On Tue, May 26, 2015 at 04:55:36PM +0800, James Liao wrote:
> > Hi Sascha,
> > 
> > On Tue, 2015-05-26 at 09:56 +0200, Sascha Hauer wrote:
> > > On Thu, May 21, 2015 at 03:12:54PM +0800, James Liao wrote:
> > > > This adds the binding documentation for the mmsys, imgsys, vdecsys,
> > > > vencsys and vencltsys controllers found on Mediatek SoCs.
> > > > 
> > > > index 0000000..a5b94a7
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt
> > > 
> > > Do these really become multiple drivers so that it's worth abstracting
> > > them in the clock framework?
> > 
> > These clocks need to be controlled among several drivers. For example,
> > vdecsys clocks will be controlled by VDEC driver (not ready yet) and
> > MT8173 SMI driver [1]. That means these clocks need a mechanism to share
> > between these 2 drivers. CCF share clocks by using of reference count,
> > so I think it's suitable to implement these subsystem clocks.
> > 
> > As I know SMI driver need to access clocks among mmsys, imgsys, vdecsys,
> > vencsys and vencltsys. So in this patch I added clocks of these
> > subsystems into CCF.
> > 
> > [1]
> > http://lists.infradead.org/pipermail/linux-mediatek/2015-March/000058.html
> 
> Looking at the 3.18 tree we have this:
> 
> 	vdecsys: vdecsys@16000000 {
> 		compatible = "mediatek,mt8173-vdecsys", "syscon";
> 		reg = <0 0x16000000 0 0x1000>;
> 		#clock-cells = <1>;
> 	};
> 
> 	larb1:larb@16010000 {
> 		compatible = "mediatek,mt8173-smi-larb";
> 		reg = <0 0x16010000 0 0x1000>;
> 		clocks = <&mmsys MM_SMI_COMMON>,
> 			 <&vdecsys VDEC_CKEN>,
> 			 <&vdecsys VDEC_LARB_CKEN>;
> 		clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
> 	};
> 
> I believe that the larb needs the MM_SMI_COMMON clock to modify the larb
> registers, but is it really necessary to enable VDEC_CKEN and
> VDEC_LARB_CKEN just to set the F_SMI_MMU_EN bit in the larb?
Yes. SMI need the two clock while smi work.
the lastest smi binding is [1].
smi need "apb" and "smi" clocks.

[1]http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013025.html

> 
> With the above we have the situation that the vdec driver calls into the
> iommu driver which then calls into the larb driver which calls back into
> the vdec driver via the clk API. This seems very suspicious.
iommu driver will call into the larb driver.
but I don't think the larb driver will call into the vdec driver. is it
right?

At the end of the latest smi driver[2].
smi has provide two interface. the multimedia HW could call it to enable
smi clock, then they can delete some node like <&vdecsys VDEC_CKEN>. in
their dtsi.
//=====
+int mtk_smi_larb_get(struct device *plarbdev);
+void mtk_smi_larb_put(struct device *plarbdev);
//=====
 SMI only help control the smi relational clockes.
The other clocks of multimedia module also should control theirself.

[2]http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013030.html

> 
> Sascha
> 

Re: [PATCH 3/5] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

[Sascha Hauer]

On Wed, May 27, 2015 at 02:12:49PM +0800, Yong Wu wrote:
> On Tue, 2015-05-26 at 13:08 +0200, Sascha Hauer wrote:
> > On Tue, May 26, 2015 at 04:55:36PM +0800, James Liao wrote:
> > > Hi Sascha,
> > > 
> > > On Tue, 2015-05-26 at 09:56 +0200, Sascha Hauer wrote:
> > > > On Thu, May 21, 2015 at 03:12:54PM +0800, James Liao wrote:
> > > > > This adds the binding documentation for the mmsys, imgsys, vdecsys,
> > > > > vencsys and vencltsys controllers found on Mediatek SoCs.
> > > > > 
> > > > > index 0000000..a5b94a7
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
> > > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
> > > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt
> > > > 
> > > > Do these really become multiple drivers so that it's worth abstracting
> > > > them in the clock framework?
> > > 
> > > These clocks need to be controlled among several drivers. For example,
> > > vdecsys clocks will be controlled by VDEC driver (not ready yet) and
> > > MT8173 SMI driver [1]. That means these clocks need a mechanism to share
> > > between these 2 drivers. CCF share clocks by using of reference count,
> > > so I think it's suitable to implement these subsystem clocks.
> > > 
> > > As I know SMI driver need to access clocks among mmsys, imgsys, vdecsys,
> > > vencsys and vencltsys. So in this patch I added clocks of these
> > > subsystems into CCF.
> > > 
> > > [1]
> > > http://lists.infradead.org/pipermail/linux-mediatek/2015-March/000058.html
> > 
> > Looking at the 3.18 tree we have this:
> > 
> > 	vdecsys: vdecsys@16000000 {
> > 		compatible = "mediatek,mt8173-vdecsys", "syscon";
> > 		reg = <0 0x16000000 0 0x1000>;
> > 		#clock-cells = <1>;
> > 	};
> > 
> > 	larb1:larb@16010000 {
> > 		compatible = "mediatek,mt8173-smi-larb";
> > 		reg = <0 0x16010000 0 0x1000>;
> > 		clocks = <&mmsys MM_SMI_COMMON>,
> > 			 <&vdecsys VDEC_CKEN>,
> > 			 <&vdecsys VDEC_LARB_CKEN>;
> > 		clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
> > 	};
> > 
> > I believe that the larb needs the MM_SMI_COMMON clock to modify the larb
> > registers, but is it really necessary to enable VDEC_CKEN and
> > VDEC_LARB_CKEN just to set the F_SMI_MMU_EN bit in the larb?
> Yes. SMI need the two clock while smi work.
> the lastest smi binding is [1].
> smi need "apb" and "smi" clocks.
> 
> [1]http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013025.html
> 
> > 
> > With the above we have the situation that the vdec driver calls into the
> > iommu driver which then calls into the larb driver which calls back into
> > the vdec driver via the clk API. This seems very suspicious.
> iommu driver will call into the larb driver.
> but I don't think the larb driver will call into the vdec driver. is it
> right?

When the larb has clocks from the vdec like in the above example from
the 3.18 kernel then the larb indirectly via the clk API writes to the
vdec register space.
In the latest code you referenced the vdec clocks are no longer assigned
to the larbs. This means we no longer need the vdec clocks abstracted in
CCF.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/5] dt-bindings: ARM: Mediatek: Document devicetree bindings for clock controllers

[Yong Wu]

On Wed, 2015-05-27 at 09:27 +0200, Sascha Hauer wrote:
> On Wed, May 27, 2015 at 02:12:49PM +0800, Yong Wu wrote:
> > On Tue, 2015-05-26 at 13:08 +0200, Sascha Hauer wrote:
> > > On Tue, May 26, 2015 at 04:55:36PM +0800, James Liao wrote:
> > > > Hi Sascha,
> > > > 
> > > > On Tue, 2015-05-26 at 09:56 +0200, Sascha Hauer wrote:
> > > > > On Thu, May 21, 2015 at 03:12:54PM +0800, James Liao wrote:
> > > > > > This adds the binding documentation for the mmsys, imgsys, vdecsys,
> > > > > > vencsys and vencltsys controllers found on Mediatek SoCs.
> > > > > > 
> > > > > > index 0000000..a5b94a7
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys.txt
> > > > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencltsys.txt
> > > > > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,vencsys.txt
> > > > > 
> > > > > Do these really become multiple drivers so that it's worth abstracting
> > > > > them in the clock framework?
> > > > 
> > > > These clocks need to be controlled among several drivers. For example,
> > > > vdecsys clocks will be controlled by VDEC driver (not ready yet) and
> > > > MT8173 SMI driver [1]. That means these clocks need a mechanism to share
> > > > between these 2 drivers. CCF share clocks by using of reference count,
> > > > so I think it's suitable to implement these subsystem clocks.
> > > > 
> > > > As I know SMI driver need to access clocks among mmsys, imgsys, vdecsys,
> > > > vencsys and vencltsys. So in this patch I added clocks of these
> > > > subsystems into CCF.
> > > > 
> > > > [1]
> > > > http://lists.infradead.org/pipermail/linux-mediatek/2015-March/000058.html
> > > 
> > > Looking at the 3.18 tree we have this:
> > > 
> > > 	vdecsys: vdecsys@16000000 {
> > > 		compatible = "mediatek,mt8173-vdecsys", "syscon";
> > > 		reg = <0 0x16000000 0 0x1000>;
> > > 		#clock-cells = <1>;
> > > 	};
> > > 
> > > 	larb1:larb@16010000 {
> > > 		compatible = "mediatek,mt8173-smi-larb";
> > > 		reg = <0 0x16010000 0 0x1000>;
> > > 		clocks = <&mmsys MM_SMI_COMMON>,
> > > 			 <&vdecsys VDEC_CKEN>,
> > > 			 <&vdecsys VDEC_LARB_CKEN>;
> > > 		clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
> > > 	};
> > > 
> > > I believe that the larb needs the MM_SMI_COMMON clock to modify the larb
> > > registers, but is it really necessary to enable VDEC_CKEN and
> > > VDEC_LARB_CKEN just to set the F_SMI_MMU_EN bit in the larb?
> > Yes. SMI need the two clock while smi work.
> > the lastest smi binding is [1].
> > smi need "apb" and "smi" clocks.
> > 
> > [1]http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013025.html
> > 
> > > 
> > > With the above we have the situation that the vdec driver calls into the
> > > iommu driver which then calls into the larb driver which calls back into
> > > the vdec driver via the clk API. This seems very suspicious.
> > iommu driver will call into the larb driver.
> > but I don't think the larb driver will call into the vdec driver. is it
> > right?
> 
> When the larb has clocks from the vdec like in the above example from
> the 3.18 kernel then the larb indirectly via the clk API writes to the
> vdec register space.
> In the latest code you referenced the vdec clocks are no longer assigned
> to the larbs. This means we no longer need the vdec clocks abstracted in
> CCF.
Hi Sascha,
  Sorry. I don't understand "assigned to the larbs" very well.
  The latest SMI driver will call all the local arbiters, It will create
a device for each local arbiter and use their clocks. SMI need enable
the clock to enable/disable iommu and transfer data from EMI for each
multimedia module. 

  Now we focus on the necessity of putting the clocks of subsystem into
CCF.
  
   Vdec only have 2 clocks, Both vdec and SMI need them.
   I take the venc for example.
   venc has 4 clock as below.
     <&vencsys VENC_CKE0>,
     <&vencsys VENC_CKE1>,
     <&vencsys VENC_CKE2>,
     <&vencsys VENC_CKE3>,
   SMI and venc need VENC_CKE0 and VENC_CKE1 both.
   VENC_CKE2 is the clock for jpeg encode.
   VENC_CKE3 is the clock for jpeg decode.
   The four clocks are in the same register.
   And while venc HW working, we also need smi common clock which is in
display subsystem.
  
   Then how to share the clocks between smi and multimedia subsystem.
   A. write the clock register directly in each module.
      ->SMI can not access the register of venc/vdec,this may be wrong.
   B. The multimedia subsystem provide the enable/disable clock
interface, Then smi call it.
      ->There are so many multimedia subsystem, such as display, vdec,
venc, mdp, camera,jpg. if all of them offer the clock interface, then
smi call them. The interface will be so many! even though SMI allow to
register clock callback for each local arbiter.
      Take a example, If jpg need work, SMI should enable the clock of
larb3(venc) and the smi common clock in display subsystem.
      This will strengthen the relationship of all multimedia modules.
      The relationship of clocks may be confuse.
   C. CCF.
      ->if we use ccf, the code may be easier to read.
 
   Could you help give some suggestion about this.
> 
> Sascha
> 
> 

[PATCH 4/5] 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@mediatek.com>
---
 drivers/clk/mediatek/clk-mt8173.c      | 310 +++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/mt8173-clk.h |  87 +++++++++
 2 files changed, 397 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index eb175ac..e2f40ba 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -700,6 +700,195 @@ 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 img_cg_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 = &img_cg_regs,				\
+		.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),
+};
+
+static struct mtk_gate_regs venc_cg_regs = {
+	.set_ofs = 0x0004,
+	.clr_ofs = 0x0008,
+	.sta_ofs = 0x0000,
+};
+
+#define GATE_VENC(_id, _name, _parent, _shift) {	\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &venc_cg_regs,				\
+		.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),
+};
+
+static struct mtk_gate_regs venclt_cg_regs = {
+	.set_ofs = 0x0004,
+	.clr_ofs = 0x0008,
+	.sta_ofs = 0x0000,
+};
+
+#define GATE_VENCLT(_id, _name, _parent, _shift) {	\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &venclt_cg_regs,			\
+		.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;
 
@@ -842,3 +1031,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..94977e6 100644
--- a/include/dt-bindings/clock/mt8173-clk.h
+++ b/include/dt-bindings/clock/mt8173-clk.h
@@ -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

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

[Daniel Kurtz]

Hi James,

On Thu, May 21, 2015 at 3:12 PM, James Liao <jamesjj.liao@mediatek.com> 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.

Is there a reason why this patch (or patch set) does not also include
definitions for the SCP_SYS, AUDIO and MFG_SYS clocks?

-Dan

>
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> ---
>  drivers/clk/mediatek/clk-mt8173.c      | 310 +++++++++++++++++++++++++++++++++
>  include/dt-bindings/clock/mt8173-clk.h |  87 +++++++++
>  2 files changed, 397 insertions(+)
>
> diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
> index eb175ac..e2f40ba 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -700,6 +700,195 @@ 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 img_cg_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 = &img_cg_regs,                           \
> +               .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),
> +};
> +
> +static struct mtk_gate_regs venc_cg_regs = {
> +       .set_ofs = 0x0004,
> +       .clr_ofs = 0x0008,
> +       .sta_ofs = 0x0000,
> +};
> +
> +#define GATE_VENC(_id, _name, _parent, _shift) {       \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &venc_cg_regs,                          \
> +               .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),
> +};
> +
> +static struct mtk_gate_regs venclt_cg_regs = {
> +       .set_ofs = 0x0004,
> +       .clr_ofs = 0x0008,
> +       .sta_ofs = 0x0000,
> +};
> +
> +#define GATE_VENCLT(_id, _name, _parent, _shift) {     \
> +               .id = _id,                                      \
> +               .name = _name,                                  \
> +               .parent_name = _parent,                         \
> +               .regs = &venclt_cg_regs,                        \
> +               .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;
>
> @@ -842,3 +1031,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..94977e6 100644
> --- a/include/dt-bindings/clock/mt8173-clk.h
> +++ b/include/dt-bindings/clock/mt8173-clk.h
> @@ -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
>

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

[James Liao]

Hi Daniel,

On Fri, 2015-05-22 at 12:22 +0800, Daniel Kurtz wrote:
> On Thu, May 21, 2015 at 3:12 PM, James Liao <jamesjj.liao@mediatek.com> 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.
> 
> Is there a reason why this patch (or patch set) does not also include
> definitions for the SCP_SYS, AUDIO and MFG_SYS clocks?

Clocks of SCP_SYS is a workaround in our previous internal
implementation. It should be replaced with new mtk-scpsys driver which
implemented in pm_domain framework because they are used to power on/off
subsystem domains.

I had discussed with Sascha, and he said it not worth the overhead of
CCF for clocks that are not shared by different units. As I know AUDIO
and MFG clocks are only used by their own drivers. So I don't include
them in this patch.


Best regards,

James

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

[Matthias Brugger]

On Thursday, May 21, 2015 03:12:55 PM James Liao 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@mediatek.com>
> ---
>  drivers/clk/mediatek/clk-mt8173.c      | 310
> +++++++++++++++++++++++++++++++++ include/dt-bindings/clock/mt8173-clk.h | 
> 87 +++++++++
>  2 files changed, 397 insertions(+)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8173.c
> b/drivers/clk/mediatek/clk-mt8173.c index eb175ac..e2f40ba 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -700,6 +700,195 @@ 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 img_cg_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 = &img_cg_regs,				\
> +		.shift = _shift,				\
> +		.ops = &mtk_clk_gate_ops_setclr,			\
> +	}
> +
[...]
> +
> +static struct mtk_gate_regs venc_cg_regs = {
> +	.set_ofs = 0x0004,
> +	.clr_ofs = 0x0008,
> +	.sta_ofs = 0x0000,
> +};
> +
> +#define GATE_VENC(_id, _name, _parent, _shift) {	\
> +		.id = _id,					\
> +		.name = _name,					\
> +		.parent_name = _parent,				\
> +		.regs = &venc_cg_regs,				\
> +		.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),
> +};
> +
> +static struct mtk_gate_regs venclt_cg_regs = {
> +	.set_ofs = 0x0004,
> +	.clr_ofs = 0x0008,
> +	.sta_ofs = 0x0000,
> +};


The register for imagesys, vencsys and vencltsys have all the same offset.
We could use just one struct for all of them. 

Cheers,
Matthias

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

[James Liao]

On Fri, 2015-06-12 at 19:09 +0200, Matthias Brugger wrote:
> On Thursday, May 21, 2015 03:12:55 PM James Liao wrote:
> > +
> > +static struct mtk_gate_regs venclt_cg_regs = {
> > +	.set_ofs = 0x0004,
> > +	.clr_ofs = 0x0008,
> > +	.sta_ofs = 0x0000,
> > +};
> 
> 
> The register for imagesys, vencsys and vencltsys have all the same offset.
> We could use just one struct for all of them. 

OK. I'll merge them in next patch.


Best regards,

James

[PATCH 5/5] 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@mediatek.com>
---
 drivers/clk/mediatek/clk-mt8173.c      | 120 +++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-pll.c         |   7 +-
 include/dt-bindings/clock/mt8173-clk.h |   7 +-
 3 files changed, 125 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index e2f40ba..105844f 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>
@@ -978,6 +979,118 @@ 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,
+			void __iomem *reg)
+{
+	struct mtk_ref2usb_tx *tx;
+	struct clk_init_data init = {};
+	struct clk *clk;
+	const char *parent_name = "clk26m";
+
+	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;
+}
+
+static void __init mtk_clk_register_apmixedsys_special(struct device_node *node,
+			struct clk_onecell_data *clk_data)
+{
+	void __iomem *base;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return;
+	}
+
+	clk = mtk_clk_register_ref2usb_tx("ref2usb_tx", base + 0x8);
+
+	if (IS_ERR(clk)) {
+		pr_err("Failed to register clk ref2usb_tx: %ld\n",
+				PTR_ERR(clk));
+		return;
+	}
+
+	clk_data->clks[CLK_APMIXED_REF2USB_TX] = clk;
+}
+
 #define MT8173_PLL_FMAX		(3000UL * MHZ)
 
 #define CON0_MT8173_RST_BAR	BIT(24)
@@ -1020,12 +1133,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 94977e6..7b7c555 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
@@ -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 5/5] clk: mediatek: Add USB clock support in MT8173 APMIXEDSYS

[Sascha Hauer]

On Thu, May 21, 2015 at 03:12:56PM +0800, James Liao wrote:
> Add REF2USB_TX clock support into MT8173 APMIXEDSYS. This clock
> is needed by USB 3.0.
> 
> +
> +static struct clk *mtk_clk_register_ref2usb_tx(const char *name,
> +			void __iomem *reg)
> +{
> +	struct mtk_ref2usb_tx *tx;
> +	struct clk_init_data init = {};
> +	struct clk *clk;
> +	const char *parent_name = "clk26m";
> +
> +	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;
> +}
> +
> +static void __init mtk_clk_register_apmixedsys_special(struct device_node *node,
> +			struct clk_onecell_data *clk_data)
> +{
> +	void __iomem *base;
> +	struct clk *clk;
> +
> +	base = of_iomap(node, 0);
> +	if (!base) {
> +		pr_err("%s(): ioremap failed\n", __func__);
> +		return;
> +	}
> +
> +	clk = mtk_clk_register_ref2usb_tx("ref2usb_tx", base + 0x8);

The function seems to be for one special clock only. Why do you pass the
name to it? They will never be called with another name, right?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[James Liao]

Hi Sascha,

On Tue, 2015-05-26 at 10:05 +0200, Sascha Hauer wrote:
> On Thu, May 21, 2015 at 03:12:56PM +0800, James Liao wrote:
> > +static void __init mtk_clk_register_apmixedsys_special(struct device_node *node,
> > +			struct clk_onecell_data *clk_data)
> > +{
> > +	void __iomem *base;
> > +	struct clk *clk;
> > +
> > +	base = of_iomap(node, 0);
> > +	if (!base) {
> > +		pr_err("%s(): ioremap failed\n", __func__);
> > +		return;
> > +	}
> > +
> > +	clk = mtk_clk_register_ref2usb_tx("ref2usb_tx", base + 0x8);
> 
> The function seems to be for one special clock only. Why do you pass the
> name to it? They will never be called with another name, right?

This function decides clock name and associates clock ID for special
clocks. In fact there may be another "special clocks" need to add into
apmixedsys. I think it's a better way to group clock names and clock IDs
in the same function for maintenance.


Best regards,

James 

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

[Sascha Hauer]

On Tue, May 26, 2015 at 05:11:15PM +0800, James Liao wrote:
> Hi Sascha,
> 
> On Tue, 2015-05-26 at 10:05 +0200, Sascha Hauer wrote:
> > On Thu, May 21, 2015 at 03:12:56PM +0800, James Liao wrote:
> > > +static void __init mtk_clk_register_apmixedsys_special(struct device_node *node,
> > > +			struct clk_onecell_data *clk_data)
> > > +{
> > > +	void __iomem *base;
> > > +	struct clk *clk;
> > > +
> > > +	base = of_iomap(node, 0);
> > > +	if (!base) {
> > > +		pr_err("%s(): ioremap failed\n", __func__);
> > > +		return;
> > > +	}
> > > +
> > > +	clk = mtk_clk_register_ref2usb_tx("ref2usb_tx", base + 0x8);
> > 
> > The function seems to be for one special clock only. Why do you pass the
> > name to it? They will never be called with another name, right?
> 
> This function decides clock name and associates clock ID for special
> clocks. In fact there may be another "special clocks" need to add into
> apmixedsys. I think it's a better way to group clock names and clock IDs
> in the same function for maintenance.

How can a function with ref2usb_tx in its name ever register a clock
with another name? Then it seems the function name is wrong.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[James Liao]

Hi Sascha,

On Tue, 2015-05-26 at 11:41 +0200, Sascha Hauer wrote:
> On Tue, May 26, 2015 at 05:11:15PM +0800, James Liao wrote:
> > On Tue, 2015-05-26 at 10:05 +0200, Sascha Hauer wrote:
> > > On Thu, May 21, 2015 at 03:12:56PM +0800, James Liao wrote:
> > > > +static void __init mtk_clk_register_apmixedsys_special(struct device_node *node,
> > > > +			struct clk_onecell_data *clk_data)
> > > > +{
> > > > +	void __iomem *base;
> > > > +	struct clk *clk;
> > > > +
> > > > +	base = of_iomap(node, 0);
> > > > +	if (!base) {
> > > > +		pr_err("%s(): ioremap failed\n", __func__);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	clk = mtk_clk_register_ref2usb_tx("ref2usb_tx", base + 0x8);
> > > 
> > > The function seems to be for one special clock only. Why do you pass the
> > > name to it? They will never be called with another name, right?
> > 
> > This function decides clock name and associates clock ID for special
> > clocks. In fact there may be another "special clocks" need to add into
> > apmixedsys. I think it's a better way to group clock names and clock IDs
> > in the same function for maintenance.
> 
> How can a function with ref2usb_tx in its name ever register a clock
> with another name? Then it seems the function name is wrong.

I mean mtk_clk_register_apmixedsys_special() decides the clock names and
clock ID bindings.

mtk_clk_register_ref2usb_tx() is only used by ref2usb_tx clock. Other
special clocks will not share the implementation with ref2usb_tx. But we
can set names and clock IDs for different special clocks in the same
function (mtk_clk_register_apmixedsys_special()) to avoid inconsistence
naming.


Best regards,

James 

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[Sascha Hauer]

On Thu, May 21, 2015 at 03:12:51PM +0800, James Liao wrote:
> This patchset contains subsystem clocks support for Mediatek MT8173.
> It also contains some bug fixes before adds new clocks support.
> 
> James Liao (4):
>   clk: mediatek: Fix apmixedsys clock registration
>   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-mt8135.c                  |   2 +-
>  drivers/clk/mediatek/clk-mt8173.c                  | 454 ++++++++++++++++++++-
>  drivers/clk/mediatek/clk-pll.c                     |   7 +-
>  include/dt-bindings/clock/mt8173-clk.h             |  94 ++++-
>  9 files changed, 652 insertions(+), 15 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

I have looked a bit closer at the vdec, venc, mmsys and other units. I
have come to the conclusion that the units and subunits should be
rearranged in the device tree. Currently we have a flat list of units,
but really they are hierarchical with top devices and subdevices. Take
vencsys as an example. Instead of:

	vencsys: vencsys@18000000 {
		compatible = "mediatek,mt8173-vencsys", "syscon";
		reg = <0 0x18000000 0 0x1000>;
		#clock-cells = <1>;
	};

	larb3:larb@18001000 {
		compatible = "mediatek,mt8173-smi-larb";
		reg = <0 0x18001000 0 0x1000>;
		clocks = <&mmsys MM_SMI_COMMON>,
			 <&vencsys VENC_CKE0>,
			 <&vencsys VENC_CKE1>;
		clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
	};

This should be:

	vencsys: vencsys@18000000 {
		compatible = "mediatek,mt8173-vencsys";
		reg = <0 0x18000000 0 0x01000000>;
		#clock-cells = <1>;
		ranges;

		larb3:larb@18001000 {
			compatible = "mediatek,mt8173-smi-larb";
			reg = <0 0x18001000 0 0x1000>;
			clocks = <&mmsys MM_SMI_COMMON>,
				 <&vencsys VENC_CKE0>,
				 <&vencsys VENC_CKE1>;
			clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
		};

		/* The actual video encoder */
		venc:video-encoder@?? {
			compatible = "mediatek,mtxxxx-videoenc";
		};
	};

And really the driver matching "mediatek,mt8173-vencsys" should register
the necessary clocks and reset lines and call of_platform_populate on
the subnodes. The driver should also be a real driver, not something
matched by CLK_OF_DECLARE. The "mediatek,mt8173-vencsys" driver now has
the possibility to manage the toplevel vencsys unit, do runtime pm, turn
the whole thing off and on. Using CCF for abstracting these clocks may
be the right thing, but I believe that we should keep the code for the
toplevel vencsys register space together in a single file and not put
the clk bits in drivers/clk/mediatek/mt8173.c, the reset bits in
drivers/reset/ and the remaining misc stuff in drivers/soc/mediatek/.

So I think we should have a drivers/soc/mediatek/mtk-vencsys.c which
is a regular driver, calls clk_register() for its clocks, calls
reset_controller_register() for the reset bits, provides plain functions
for the remaining bits which are not handled by any Linux framework.
Finally of_platform_populate will register the child devices.

I showed this using the vencsys example, but it's the same for vdecsys,
vencltsys, imgsys and mmsys.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[James Liao]

Hi Sascha,

On Thu, 2015-05-28 at 15:24 +0200, Sascha Hauer wrote:
> On Thu, May 21, 2015 at 03:12:51PM +0800, James Liao wrote:
> > This patchset contains subsystem clocks support for Mediatek MT8173.
> > It also contains some bug fixes before adds new clocks support.
> > 
> > James Liao (4):
> >   clk: mediatek: Fix apmixedsys clock registration
> >   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-mt8135.c                  |   2 +-
> >  drivers/clk/mediatek/clk-mt8173.c                  | 454 ++++++++++++++++++++-
> >  drivers/clk/mediatek/clk-pll.c                     |   7 +-
> >  include/dt-bindings/clock/mt8173-clk.h             |  94 ++++-
> >  9 files changed, 652 insertions(+), 15 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
> 
> I have looked a bit closer at the vdec, venc, mmsys and other units. I
> have come to the conclusion that the units and subunits should be
> rearranged in the device tree. Currently we have a flat list of units,
> but really they are hierarchical with top devices and subdevices. Take
> vencsys as an example. Instead of:
> 
> 	vencsys: vencsys@18000000 {
> 		compatible = "mediatek,mt8173-vencsys", "syscon";
> 		reg = <0 0x18000000 0 0x1000>;
> 		#clock-cells = <1>;
> 	};
> 
> 	larb3:larb@18001000 {
> 		compatible = "mediatek,mt8173-smi-larb";
> 		reg = <0 0x18001000 0 0x1000>;
> 		clocks = <&mmsys MM_SMI_COMMON>,
> 			 <&vencsys VENC_CKE0>,
> 			 <&vencsys VENC_CKE1>;
> 		clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
> 	};
> 
> This should be:
> 
> 	vencsys: vencsys@18000000 {
> 		compatible = "mediatek,mt8173-vencsys";
> 		reg = <0 0x18000000 0 0x01000000>;
> 		#clock-cells = <1>;
> 		ranges;
> 
> 		larb3:larb@18001000 {
> 			compatible = "mediatek,mt8173-smi-larb";
> 			reg = <0 0x18001000 0 0x1000>;
> 			clocks = <&mmsys MM_SMI_COMMON>,
> 				 <&vencsys VENC_CKE0>,
> 				 <&vencsys VENC_CKE1>;
> 			clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
> 		};
> 
> 		/* The actual video encoder */
> 		venc:video-encoder@?? {
> 			compatible = "mediatek,mtxxxx-videoenc";
> 		};
> 	};
> 
> And really the driver matching "mediatek,mt8173-vencsys" should register
> the necessary clocks and reset lines and call of_platform_populate on
> the subnodes. The driver should also be a real driver, not something
> matched by CLK_OF_DECLARE. The "mediatek,mt8173-vencsys" driver now has
> the possibility to manage the toplevel vencsys unit, do runtime pm, turn
> the whole thing off and on. Using CCF for abstracting these clocks may
> be the right thing, but I believe that we should keep the code for the
> toplevel vencsys register space together in a single file and not put
> the clk bits in drivers/clk/mediatek/mt8173.c, the reset bits in
> drivers/reset/ and the remaining misc stuff in drivers/soc/mediatek/.
> 
> So I think we should have a drivers/soc/mediatek/mtk-vencsys.c which
> is a regular driver, calls clk_register() for its clocks, calls
> reset_controller_register() for the reset bits, provides plain functions
> for the remaining bits which are not handled by any Linux framework.
> Finally of_platform_populate will register the child devices.
> 
> I showed this using the vencsys example, but it's the same for vdecsys,
> vencltsys, imgsys and mmsys.

So you agree to manage these subsystem clocks in CCF, but they should be
provided by their own (globalcon) drivers, right?

I have an implementation question. These subsystem clocks can't be
implemented in CCF default clock-gate. So in our previous patches, we
added a drivers/clk/mediate/clk-gate.c to implement new clock gate
operations. Is it a good way to export mediatek/clk-gate.h (put it in
include/linux/) for other drivers to implement their own clocks?


Best regards,

James

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[Sascha Hauer]

On Fri, May 29, 2015 at 10:47:29AM +0800, James Liao wrote:
> Hi Sascha,
> 
> > And really the driver matching "mediatek,mt8173-vencsys" should register
> > the necessary clocks and reset lines and call of_platform_populate on
> > the subnodes. The driver should also be a real driver, not something
> > matched by CLK_OF_DECLARE. The "mediatek,mt8173-vencsys" driver now has
> > the possibility to manage the toplevel vencsys unit, do runtime pm, turn
> > the whole thing off and on. Using CCF for abstracting these clocks may
> > be the right thing, but I believe that we should keep the code for the
> > toplevel vencsys register space together in a single file and not put
> > the clk bits in drivers/clk/mediatek/mt8173.c, the reset bits in
> > drivers/reset/ and the remaining misc stuff in drivers/soc/mediatek/.
> > 
> > So I think we should have a drivers/soc/mediatek/mtk-vencsys.c which
> > is a regular driver, calls clk_register() for its clocks, calls
> > reset_controller_register() for the reset bits, provides plain functions
> > for the remaining bits which are not handled by any Linux framework.
> > Finally of_platform_populate will register the child devices.
> > 
> > I showed this using the vencsys example, but it's the same for vdecsys,
> > vencltsys, imgsys and mmsys.
> 
> So you agree to manage these subsystem clocks in CCF, but they should be
> provided by their own (globalcon) drivers, right?

Yes. I previously got the impression that the subsystem clocks are not
directly associated to the larbs, but needed to be handled by the larb
code due to some side effect. Now that I saw that the larbs are directly
in the subsystem register space it all makes sense.

Note that the way Mediatek SoCs are designed around sub modules is bit
unusual and does not fit very well in the Linux directory structure.
Normally SoCs have a single clocks controller which controls all clocks
in the SoC. Then you often have a reset controller providing reset lines
in the SoC. In this case it's clear that the clk driver goes to
drivers/clk/, the reset controller driver to drivers/reset/. Mediatek
SoCs instead have several blocks, each with its own clock and reset
controller. Splitting each block up into parts in drivers/clk/ and
drivers/reset/ leads to quite a code fragmentation.
This is my opinion, it would be great to hear something from others.
Matthias? I'd like to avoid running into a direction that is not
acceptable in the end.

> 
> I have an implementation question. These subsystem clocks can't be
> implemented in CCF default clock-gate. So in our previous patches, we
> added a drivers/clk/mediate/clk-gate.c to implement new clock gate
> operations. Is it a good way to export mediatek/clk-gate.h (put it in
> include/linux/) for other drivers to implement their own clocks?

Give it a try. If that's not acceptable we can still shuffle this around
without much effort.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[Stephen Boyd]

On 05/29, Sascha Hauer wrote:
> On Fri, May 29, 2015 at 10:47:29AM +0800, James Liao wrote:
> > Hi Sascha,
> > 
> > > And really the driver matching "mediatek,mt8173-vencsys" should register
> > > the necessary clocks and reset lines and call of_platform_populate on
> > > the subnodes. The driver should also be a real driver, not something
> > > matched by CLK_OF_DECLARE. The "mediatek,mt8173-vencsys" driver now has
> > > the possibility to manage the toplevel vencsys unit, do runtime pm, turn
> > > the whole thing off and on. Using CCF for abstracting these clocks may
> > > be the right thing, but I believe that we should keep the code for the
> > > toplevel vencsys register space together in a single file and not put
> > > the clk bits in drivers/clk/mediatek/mt8173.c, the reset bits in
> > > drivers/reset/ and the remaining misc stuff in drivers/soc/mediatek/.
> > > 
> > > So I think we should have a drivers/soc/mediatek/mtk-vencsys.c which
> > > is a regular driver, calls clk_register() for its clocks, calls
> > > reset_controller_register() for the reset bits, provides plain functions
> > > for the remaining bits which are not handled by any Linux framework.
> > > Finally of_platform_populate will register the child devices.
> > > 
> > > I showed this using the vencsys example, but it's the same for vdecsys,
> > > vencltsys, imgsys and mmsys.
> > 
> > So you agree to manage these subsystem clocks in CCF, but they should be
> > provided by their own (globalcon) drivers, right?
> 
> Yes. I previously got the impression that the subsystem clocks are not
> directly associated to the larbs, but needed to be handled by the larb
> code due to some side effect. Now that I saw that the larbs are directly
> in the subsystem register space it all makes sense.
> 
> Note that the way Mediatek SoCs are designed around sub modules is bit
> unusual and does not fit very well in the Linux directory structure.
> Normally SoCs have a single clocks controller which controls all clocks
> in the SoC. Then you often have a reset controller providing reset lines
> in the SoC. In this case it's clear that the clk driver goes to
> drivers/clk/, the reset controller driver to drivers/reset/. Mediatek
> SoCs instead have several blocks, each with its own clock and reset
> controller. Splitting each block up into parts in drivers/clk/ and
> drivers/reset/ leads to quite a code fragmentation.
> This is my opinion, it would be great to hear something from others.
> Matthias? I'd like to avoid running into a direction that is not
> acceptable in the end.

We already have drivers registering clocks and resets under
drivers/clk, so it's not unheard of. An alternative solution is
to make child devices for the clock part and the reset part at
runtime in the toplevel driver for the vencsys device (don't do
any sort of DT description for this) and use regmap to mediate
the register accesses and locking. That way we can put the clk
driver in drivers/clk/, the reset driver in drivers/reset, etc.
so that logically related code is grouped.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[James Liao]

Hi Stephen,

On Thu, 2015-06-04 at 14:02 -0700, Stephen Boyd wrote:
> On 05/29, Sascha Hauer wrote:
> > Yes. I previously got the impression that the subsystem clocks are not
> > directly associated to the larbs, but needed to be handled by the larb
> > code due to some side effect. Now that I saw that the larbs are directly
> > in the subsystem register space it all makes sense.
> > 
> > Note that the way Mediatek SoCs are designed around sub modules is bit
> > unusual and does not fit very well in the Linux directory structure.
> > Normally SoCs have a single clocks controller which controls all clocks
> > in the SoC. Then you often have a reset controller providing reset lines
> > in the SoC. In this case it's clear that the clk driver goes to
> > drivers/clk/, the reset controller driver to drivers/reset/. Mediatek
> > SoCs instead have several blocks, each with its own clock and reset
> > controller. Splitting each block up into parts in drivers/clk/ and
> > drivers/reset/ leads to quite a code fragmentation.
> > This is my opinion, it would be great to hear something from others.
> > Matthias? I'd like to avoid running into a direction that is not
> > acceptable in the end.
> 
> We already have drivers registering clocks and resets under
> drivers/clk, so it's not unheard of. An alternative solution is
> to make child devices for the clock part and the reset part at
> runtime in the toplevel driver for the vencsys device (don't do
> any sort of DT description for this) and use regmap to mediate
> the register accesses and locking. That way we can put the clk
> driver in drivers/clk/, the reset driver in drivers/reset, etc.
> so that logically related code is grouped.

I have a question about the alternative way you mentioned. Currently
clock providers and consumers describe what clocks they will provide /
consume in device tree. If we don't describe vencsys clocks in device
tree, how to get vencsys clocks for drivers that need to control them?


Best regards,

James

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[Stephen Boyd]

On 06/05, James Liao wrote:
> Hi Stephen,
> 
> On Thu, 2015-06-04 at 14:02 -0700, Stephen Boyd wrote:
> > On 05/29, Sascha Hauer wrote:
> > > Yes. I previously got the impression that the subsystem clocks are not
> > > directly associated to the larbs, but needed to be handled by the larb
> > > code due to some side effect. Now that I saw that the larbs are directly
> > > in the subsystem register space it all makes sense.
> > > 
> > > Note that the way Mediatek SoCs are designed around sub modules is bit
> > > unusual and does not fit very well in the Linux directory structure.
> > > Normally SoCs have a single clocks controller which controls all clocks
> > > in the SoC. Then you often have a reset controller providing reset lines
> > > in the SoC. In this case it's clear that the clk driver goes to
> > > drivers/clk/, the reset controller driver to drivers/reset/. Mediatek
> > > SoCs instead have several blocks, each with its own clock and reset
> > > controller. Splitting each block up into parts in drivers/clk/ and
> > > drivers/reset/ leads to quite a code fragmentation.
> > > This is my opinion, it would be great to hear something from others.
> > > Matthias? I'd like to avoid running into a direction that is not
> > > acceptable in the end.
> > 
> > We already have drivers registering clocks and resets under
> > drivers/clk, so it's not unheard of. An alternative solution is
> > to make child devices for the clock part and the reset part at
> > runtime in the toplevel driver for the vencsys device (don't do
> > any sort of DT description for this) and use regmap to mediate
> > the register accesses and locking. That way we can put the clk
> > driver in drivers/clk/, the reset driver in drivers/reset, etc.
> > so that logically related code is grouped.
> 
> I have a question about the alternative way you mentioned. Currently
> clock providers and consumers describe what clocks they will provide /
> consume in device tree. If we don't describe vencsys clocks in device
> tree, how to get vencsys clocks for drivers that need to control them?
> 

Perhaps an example would be best. In DT we would have:

	vencsys: vencsys@10000 {
		compatible = "mtk,vencsys";
		reg = <0x10000 0x1000>;
		#clock-cells = <1>;
		#reset-cells = <1>;
	};

	myconsumer@12000 {
		compatible = "mtk,vencsys";
		reg = <0x12000 0x100>;
		clocks = <&vencsys 10>;
		clock-names = "core";
	};

	(Or are the consumers only children of the subsystem?
	It's not clear to me)

And then in the mtk,vencsys driver we would create a platform
device named something like "mtk-vencsys-clk" and assign the
of_node of the device to be the of_node that is assigned to the
mtk,vencsys device.

	static int vencsys_probe(struct platform_device *pdev)
	{
		int ret;
		struct device_node *np = pdev->dev.of_node;
		struct platform_device *clk_pdev;

		clk_pdev = platform_device_alloc("mtk-vencsys-clk", -1);
		clk_pdev->dev.of_node = of_node;
		ret = platform_device_add(clk_pdev);
		if (ret)
			return ret;
	}

Then we could put a mtk-vencsys-clk driver in drivers/clk/ that
does the clk driver part...

	static int clk_vencsys_probe(struct platform_device *pdev)
	{
		int ret;
		struct device_node *np = pdev->dev.of_node;
		struct regmap *regmap;

		ret = of_clk_add_provider(np, of_clk_src_onecell_get, ..);
		if (ret)
			return ret;

		regmap = dev_get_regmap(pdev->dev.parent, NULL);

	}

And similar things could be done for the reset driver.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[James Liao]

Hi Stephen,

On Fri, 2015-06-05 at 17:59 -0700, Stephen Boyd wrote:
> Perhaps an example would be best. In DT we would have:
> 
> 	vencsys: vencsys@10000 {
> 		compatible = "mtk,vencsys";
> 		reg = <0x10000 0x1000>;
> 		#clock-cells = <1>;
> 		#reset-cells = <1>;
> 	};
> 
> 	myconsumer@12000 {
> 		compatible = "mtk,vencsys";
> 		reg = <0x12000 0x100>;
> 		clocks = <&vencsys 10>;
> 		clock-names = "core";
> 	};
> 
> 	(Or are the consumers only children of the subsystem?
> 	It's not clear to me)

In our case the vencsys clocks may use used by its own driver and other
drivers (such as SMI driver).

> And then in the mtk,vencsys driver we would create a platform
> device named something like "mtk-vencsys-clk" and assign the
> of_node of the device to be the of_node that is assigned to the
> mtk,vencsys device.
> 
> 	static int vencsys_probe(struct platform_device *pdev)
> 	{
> 		int ret;
> 		struct device_node *np = pdev->dev.of_node;
> 		struct platform_device *clk_pdev;
> 
> 		clk_pdev = platform_device_alloc("mtk-vencsys-clk", -1);
> 		clk_pdev->dev.of_node = of_node;
> 		ret = platform_device_add(clk_pdev);
> 		if (ret)
> 			return ret;
> 	}
> 
> Then we could put a mtk-vencsys-clk driver in drivers/clk/ that
> does the clk driver part...
> 
> 	static int clk_vencsys_probe(struct platform_device *pdev)
> 	{
> 		int ret;
> 		struct device_node *np = pdev->dev.of_node;
> 		struct regmap *regmap;
> 
> 		ret = of_clk_add_provider(np, of_clk_src_onecell_get, ..);
> 		if (ret)
> 			return ret;
> 
> 		regmap = dev_get_regmap(pdev->dev.parent, NULL);
> 
> 	}
> 
> And similar things could be done for the reset driver.
> 

It looks like there is only one DT node (vencsys@10000) but there are
several drivers refer to this DT node, and then we use a dummy driver
(or a "top driver") to aggregate drivers with different features (clk,
reset etc.).

It means we need to provide vencsys clk driver in drivers/clk and
vencsys reset driver in drivers/reset, and also to provide a vencsys top
driver in, for example, soc/mediatek, right?

Now we have 3 choice:

1. implement vencsys clock driver and reset driver in drivers/clk.
2. implement vencsys clock driver and reset driver in drivers/soc.
3. implement vencsys clock driver in drivers/clk, vencsys reset driver
in drivers/reset, and vencsys top driver in drivers/soc.

Sascha, do you have comments for these 3 solutions?


Best regards,

James

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[Sascha Hauer]

On Fri, Jun 05, 2015 at 05:59:12PM -0700, Stephen Boyd wrote:
> On 06/05, James Liao wrote:
> > Hi Stephen,
> > 
> > On Thu, 2015-06-04 at 14:02 -0700, Stephen Boyd wrote:
> > > On 05/29, Sascha Hauer wrote:
> > > > Yes. I previously got the impression that the subsystem clocks are not
> > > > directly associated to the larbs, but needed to be handled by the larb
> > > > code due to some side effect. Now that I saw that the larbs are directly
> > > > in the subsystem register space it all makes sense.
> > > > 
> > > > Note that the way Mediatek SoCs are designed around sub modules is bit
> > > > unusual and does not fit very well in the Linux directory structure.
> > > > Normally SoCs have a single clocks controller which controls all clocks
> > > > in the SoC. Then you often have a reset controller providing reset lines
> > > > in the SoC. In this case it's clear that the clk driver goes to
> > > > drivers/clk/, the reset controller driver to drivers/reset/. Mediatek
> > > > SoCs instead have several blocks, each with its own clock and reset
> > > > controller. Splitting each block up into parts in drivers/clk/ and
> > > > drivers/reset/ leads to quite a code fragmentation.
> > > > This is my opinion, it would be great to hear something from others.
> > > > Matthias? I'd like to avoid running into a direction that is not
> > > > acceptable in the end.
> > > 
> > > We already have drivers registering clocks and resets under
> > > drivers/clk, so it's not unheard of. An alternative solution is
> > > to make child devices for the clock part and the reset part at
> > > runtime in the toplevel driver for the vencsys device (don't do
> > > any sort of DT description for this) and use regmap to mediate
> > > the register accesses and locking. That way we can put the clk
> > > driver in drivers/clk/, the reset driver in drivers/reset, etc.
> > > so that logically related code is grouped.
> > 
> > I have a question about the alternative way you mentioned. Currently
> > clock providers and consumers describe what clocks they will provide /
> > consume in device tree. If we don't describe vencsys clocks in device
> > tree, how to get vencsys clocks for drivers that need to control them?
> > 
> 
> Perhaps an example would be best. In DT we would have:
> 
> 	vencsys: vencsys@10000 {
> 		compatible = "mtk,vencsys";
> 		reg = <0x10000 0x1000>;
> 		#clock-cells = <1>;
> 		#reset-cells = <1>;
> 	};
> 
> 	myconsumer@12000 {
> 		compatible = "mtk,vencsys";
> 		reg = <0x12000 0x100>;
> 		clocks = <&vencsys 10>;
> 		clock-names = "core";
> 	};
> 
> 	(Or are the consumers only children of the subsystem?
> 	It's not clear to me)
> 
> And then in the mtk,vencsys driver we would create a platform
> device named something like "mtk-vencsys-clk" and assign the
> of_node of the device to be the of_node that is assigned to the
> mtk,vencsys device.
> 
> 	static int vencsys_probe(struct platform_device *pdev)
> 	{
> 		int ret;
> 		struct device_node *np = pdev->dev.of_node;
> 		struct platform_device *clk_pdev;
> 
> 		clk_pdev = platform_device_alloc("mtk-vencsys-clk", -1);
> 		clk_pdev->dev.of_node = of_node;
> 		ret = platform_device_add(clk_pdev);
> 		if (ret)
> 			return ret;
> 	}
> 
> Then we could put a mtk-vencsys-clk driver in drivers/clk/ that
> does the clk driver part...
> 
> 	static int clk_vencsys_probe(struct platform_device *pdev)
> 	{
> 		int ret;
> 		struct device_node *np = pdev->dev.of_node;
> 		struct regmap *regmap;
> 
> 		ret = of_clk_add_provider(np, of_clk_src_onecell_get, ..);
> 		if (ret)
> 			return ret;
> 
> 		regmap = dev_get_regmap(pdev->dev.parent, NULL);
> 
> 	}
> 
> And similar things could be done for the reset driver.

The problem I see with this approach is that we scatter the code for a
otherwise simple driver over a bunch of directories. We would have

drivers/clk/mediatek/vencsys.c
drivers/reset/mediatek/vencsys.c
drivers/soc/mediatek/vencsys.c

The same must be added for vdecsys, imgsys and vencltsys. That will make
12 drivers and three maintainers for 12 registers.  I think this will be
a pain to maintain, hence my suggestion to put the vencsys code into a
single file and not split this up into more subsystem specific files.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[Stephen Boyd]

On 06/08, Sascha Hauer wrote:
> On Fri, Jun 05, 2015 at 05:59:12PM -0700, Stephen Boyd wrote:
> > 
> > And similar things could be done for the reset driver.
> 
> The problem I see with this approach is that we scatter the code for a
> otherwise simple driver over a bunch of directories. We would have
> 
> drivers/clk/mediatek/vencsys.c
> drivers/reset/mediatek/vencsys.c
> drivers/soc/mediatek/vencsys.c
> 
> The same must be added for vdecsys, imgsys and vencltsys. That will make
> 12 drivers and three maintainers for 12 registers.  I think this will be
> a pain to maintain, hence my suggestion to put the vencsys code into a
> single file and not split this up into more subsystem specific files.
> 

I probably don't have enough information here, but why is it a
pain to maintain? It seems more like a pain to setup the first
time and then little to no pain to maintain because we clearly
split functionality based on subsystem. No merge conflicts, clear
division of functionality, etc. But again, I don't think it
matters much either way given that reset and clk drivers are
combined sometimes and don't always reside in drivers/clk or
drivers/reset either.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support

[Matthias Brugger]

On Thursday, June 11, 2015 04:52:12 PM Stephen Boyd wrote:
> On 06/08, Sascha Hauer wrote:
> > On Fri, Jun 05, 2015 at 05:59:12PM -0700, Stephen Boyd wrote:
> > > And similar things could be done for the reset driver.
> > 
> > The problem I see with this approach is that we scatter the code for a
> > otherwise simple driver over a bunch of directories. We would have
> > 
> > drivers/clk/mediatek/vencsys.c
> > drivers/reset/mediatek/vencsys.c
> > drivers/soc/mediatek/vencsys.c
> > 
> > The same must be added for vdecsys, imgsys and vencltsys. That will make
> > 12 drivers and three maintainers for 12 registers.  I think this will be
> > a pain to maintain, hence my suggestion to put the vencsys code into a
> > single file and not split this up into more subsystem specific files.
> 
> I probably don't have enough information here, but why is it a
> pain to maintain? It seems more like a pain to setup the first
> time and then little to no pain to maintain because we clearly
> split functionality based on subsystem. No merge conflicts, clear
> division of functionality, etc. But again, I don't think it
> matters much either way given that reset and clk drivers are
> combined sometimes and don't always reside in drivers/clk or
> drivers/reset either.

Actually I would prefer to have the clock and resets in drivers/clk, just as 
we already did for Mediatek up to now.