From patchwork Thu Jun 13 07:02:05 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yadwinder Singh Brar X-Patchwork-Id: 2714621 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id D34CD9F3B5 for ; Thu, 13 Jun 2013 08:37:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9138520170 for ; Thu, 13 Jun 2013 08:37:29 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 55AF52016B for ; Thu, 13 Jun 2013 08:37:28 +0000 (UTC) Received: from merlin.infradead.org ([205.233.59.134]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Un1g0-00045n-52; Thu, 13 Jun 2013 07:10:40 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Un1eB-0003j9-DH; Thu, 13 Jun 2013 07:08:39 +0000 Received: from mail-ie0-x22a.google.com ([2607:f8b0:4001:c03::22a]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Un1YE-0003FW-6i for linux-arm-kernel@lists.infradead.org; Thu, 13 Jun 2013 07:02:32 +0000 Received: by mail-ie0-f170.google.com with SMTP id e11so9997931iej.29 for ; Thu, 13 Jun 2013 00:02:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=EdTIjfEHHvT/920uEi3g09C/5w1z+2LsbaB0JfpwwbI=; b=avlpOxKwL4Q53deO4uwf61aErSnuwJOttPq10e4sZYF0xR5oZX/EgtQMsbVcloZA1W sb25wupd4ErymgK1s/rv/zysm4wCOmeOUhKErqLyzw+eYJjJQ0ldchVcBYjW9ab9TuLj 0D9z/ZGcw110EqL4fkw8jiCHxREc6k9x4Fkv9IR2uXfKBXpfEae6GGxo0n2DCSkWbFyO 1v0YcUiM2T/cRQUUvWpyi6fbXZ8Ybez7PqQi8CpJwKhaDI9GjOquEQx4cWVk6HQ0p/OR IN8yioKAbRD3Ub/qZMZOiQ1XIVf5Xr7QDntZtbm75OQZT+rnd3yG7LiFmutZpjCAXaaY MWSw== MIME-Version: 1.0 X-Received: by 10.50.17.166 with SMTP id p6mr5113328igd.12.1371106925410; Thu, 13 Jun 2013 00:02:05 -0700 (PDT) Received: by 10.50.25.193 with HTTP; Thu, 13 Jun 2013 00:02:05 -0700 (PDT) In-Reply-To: References: <1370272196-4346-1-git-send-email-yadi.brar@samsung.com> <1370272196-4346-2-git-send-email-yadi.brar@samsung.com> <1501124.EVQA8KO0ic@flatron> Date: Thu, 13 Jun 2013 12:32:05 +0530 Message-ID: Subject: Re: [PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx From: Yadwinder Singh Brar To: Andrew Bresticker X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130613_030230_464338_8C28C94E X-CRM114-Status: GOOD ( 22.48 ) X-Spam-Score: -1.8 (-) Cc: Yadwinder Singh Brar , Kukjin Kim , Mike Turquette , Patch Tracking , Tomasz Figa , Doug Anderson , Tomasz Figa , linux-samsung-soc , Thomas Abraham , Vikas Sajjan , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jun 13, 2013 at 3:32 AM, Andrew Bresticker wrote: > Doug, > >>> Hmm, if done properly, it could simplify PLL registration in SoC clock >>> initialization code a lot. >>> >>> I'm not sure if this is really the best solution (feel free to suggest >>> anything better), but we could put PLLs in an array, like other clocks, >>> e.g. >>> >>> ... exynos4210_pll_clks[] = { >>> CLK_PLL45XX(...), >>> CLK_PLL45XX(...), >>> CLK_PLL46XX(...), >>> CLK_PLL46XX(...), >>> }; >>> >>> and then just call a helper like >>> >>> samsung_clk_register_pll(exynos4210_pll_clks, >>> ARRAY_SIZE(exynos4210_pll_clks)); >> >> Something like that looks like what I was thinking. I'd have to see >> it actually coded up to see if there's something I'm missing that >> would prevent us from doing that, but I don't see anything. > > The only issue I see with this is that we may only want to register a > rate table with a PLL only if fin_pll is running at a certain rate. > On 5250 and 5420, for example, we have EPLL and VPLL rate tables that > should only be registered if fin_pll is 24Mhz. We may have to > register those separately, but this approach seems fine otherwise. > As Andrew Bresticker said, we will face problem with different table, and it will give some pain while handling such cases but I think overall code may look better. Similar thoughts were their in my mind also, but i didn't want to disturb this series :). Anyways, I think we can do it now only rather going for incremental patches after this series. I was thinking to make samsung_clk_register_pllxxxx itself little generic instead of using helper, as we are almost duplicating code for most PLLs. A rough picture in my mind was, After implementing generic samung_clk_register_pll(), code may look like below. Its just an idea, please feel free to correct it. Later we can factor out other common clk.ops for PLLs also. this diff is over this series. Assuming a generic samung_clk_register_pll() is their(which i think is not impossible) 8<-------------------------------------------------------------------------- --- a/drivers/clk/samsung/clk-exynos5250.c +++ b/drivers/clk/samsung/clk-exynos5250.c @@ -493,6 +493,20 @@ static __initdata struct samsung_pll_rate_table epll_24mhz_tbl[] = { PLL_36XX_RATE(32768000, 131, 3, 5, 4719), }; +struct __initdata samsung_pll_init_data samsung_plls[] = { + PLL(pll_3500, "fout_apll", "fin_pll", APLL_LOCK, APLL_CON0, NULL), + PLL(pll_3500, "fout_mpll", "fin_pll", MPLL_LOCK, MPLL_CON0, NULL), + PLL(pll_3500, "fout_bpll", "fin_pll",BPLL_LOCK, BPLL_CON0, NULL), + PLL(pll_3500, "fout_gpll", "fin_pll",GPLL_LOCK, GPLL_CON0, NULL), + PLL(pll_3500, "fout_cpll", "fin_pll",BPLL_LOCK, CPLL_CON0, NULL), +}; + +struct __initdata samsung_pll_init_data epll_init_data = + PLL(pll_3600, "fout_epll", "fin_pll", EPLL_LOCK, EPLL_CON0, NULL); + +struct __initdata samsung_pll_init_data vpll_init_data = + PLL(pll_3600, "fout_epll", "fin_pll", VPLL_LOCK, VPLL_CON0, NULL); + /* register exynox5250 clocks */ void __init exynos5250_clk_init(struct device_node *np) { @@ -519,44 +533,22 @@ void __init exynos5250_clk_init(struct device_node *np) samsung_clk_register_mux(exynos5250_pll_pmux_clks, ARRAY_SIZE(exynos5250_pll_pmux_clks)); - fin_pll_rate = _get_rate("fin_pll"); + samsung_clk_register_pll(samsung_plls, ARRAY_SIZE(samsung_plls)); + vpllsrc = __clk_lookup("mout_vpllsrc"); if (vpllsrc) mout_vpllsrc_rate = clk_get_rate(vpllsrc); - apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll", - reg_base, NULL, 0); - mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll", - reg_base + 0x4000, NULL, 0); - bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll", - reg_base + 0x20010, NULL, 0); - gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll", - reg_base + 0x10050, NULL, 0); - cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll", - reg_base + 0x10020, NULL, 0); - + fin_pll_rate = _get_rate("fin_pll"); if (fin_pll_rate == (24 * MHZ)) { - epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll", - reg_base + 0x10030, epll_24mhz_tbl, - ARRAY_SIZE(epll_24mhz_tbl)); - } else { - pr_warn("%s: valid epll rate_table missing for\n" - "parent fin_pll:%lu hz\n", __func__, fin_pll_rate); - epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll", - reg_base + 0x10030, NULL, 0); + epll_init_data.rate_table = epll_24mhz_tb; } + samsung_clk_register_pll(&fout_epll_data, 1); if (mout_vpllsrc_rate == (24 * MHZ)) { - vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc" - , reg_base + 0x10040, vpll_24mhz_tbl, - ARRAY_SIZE(vpll_24mhz_tbl)); - } else { - pr_warn("%s: valid vpll rate_table missing for\n" - "parent mout_vpllsrc_rate:%lu hz\n", __func__, - mout_vpllsrc_rate); - samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc", - reg_base + 0x10040, NULL, 0); + vpll_init_data.rate_table = epll_24mhz_tb; } + samsung_clk_register_pll(&fout_epll_data, 1); samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks, ARRAY_SIZE(exynos5250_fixed_rate_clks)); diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h index 4780e6c..3b02dc5 100644 --- a/drivers/clk/samsung/clk-pll.h +++ b/drivers/clk/samsung/clk-pll.h @@ -39,6 +39,39 @@ struct samsung_pll_rate_table { unsigned int kdiv; }; +#define PLL(_type, _name, _pname, _lock_reg, _con_reg, _rate_table) \ + { \ + .type = _type, \ + .name = _name, \ + .parent_name = _pname, \ + .flags = CLK_GET_RATE_NOCACHE, \ + .rate_table = _rate_table, \ + .con_reg = _con_reg, \ + .lock_reg = _lock_reg, \ + } + +enum samsung_pll_type { + pll_3500, + pll_45xx, + pll_2550, + pll_3600, + pll_46xx, + pll_2660, +}; + + +struct samsung_pll_init_data { + enum samsung_pll_type type; + struct samsung_pll_rate_table *rate_table; + const char *name; + const char *parent_name; + unsigned long flags; + const void __iomem *con_reg; + const void __iomem *lock_reg; +}; + +extern int __init samsung_clk_register_pll(struct samsung_pll_init_data *data, + unsigned int nr_pll); + Regards, Yadwinder