From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754016AbbE1NZX (ORCPT ); Thu, 28 May 2015 09:25:23 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:33733 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752514AbbE1NZL (ORCPT ); Thu, 28 May 2015 09:25:11 -0400 Date: Thu, 28 May 2015 15:24:52 +0200 From: Sascha Hauer To: James Liao Cc: Matthias Brugger , Mike Turquette , Stephen Boyd , srv_heupstream@mediatek.com, Eddie Huang , Henry Chen , Yingjoe Chen , Daniel Kurtz , Ricky Liang , Rob Herring , Sascha Hauer , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org Subject: Re: [PATCH 0/5] Add Mediatek MT8173 subsystem clocks support Message-ID: <20150528132452.GI26575@pengutronix.de> References: <1432192376-6712-1-git-send-email-jamesjj.liao@mediatek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1432192376-6712-1-git-send-email-jamesjj.liao@mediatek.com> X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 14:59:56 up 73 days, 51 min, 110 users, load average: 0.14, 0.26, 0.22 User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 |