Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751706AbcDPQAG (ORCPT ); Sat, 16 Apr 2016 12:00:06 -0400 Received: from mail-ig0-f195.google.com ([209.85.213.195]:34368 "EHLO mail-ig0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751491AbcDPQAB (ORCPT ); Sat, 16 Apr 2016 12:00:01 -0400 MIME-Version: 1.0 In-Reply-To: <20160414172838.GS4005@lukather> References: <1458144276-31108-1-git-send-email-vishnupatekar0510@gmail.com> <1458144276-31108-3-git-send-email-vishnupatekar0510@gmail.com> <20160317104042.GK30977@lukather> <20160414172838.GS4005@lukather> From: Vishnu Patekar Date: Sat, 16 Apr 2016 23:59:40 +0800 Message-ID: Subject: Re: [PATCH v4 02/13] clk: sunxi: add ahb1 clock for A83T To: Maxime Ripard Cc: "robh+dt@kernel.org" , Jonathan Corbet , "pawel.moll@arm.com" , "mark.rutland@arm.com" , "ijc+devicetree@hellion.org.uk" , Kumar Gala , "linux@arm.linux.org.uk" , Emilio Lopez , Jens Kuske , Hans de Goede , Chen-Yu Tsai , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-sunxi@googlegroups.com" , linux-gpio@vger.kernel.org, Linus Walleij , Michael Turquette , Stephen Boyd , Reinder de Haan , linux-clk Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5648 Lines: 143 Hello Maxime, On Fri, Apr 15, 2016 at 1:28 AM, Maxime Ripard wrote: > On Mon, Apr 04, 2016 at 11:07:29AM +0800, Vishnu Patekar wrote: >> Hello Maxime, >> >> On Thu, Mar 17, 2016 at 6:40 PM, Maxime Ripard >> wrote: >> > On Thu, Mar 17, 2016 at 12:04:25AM +0800, Vishnu Patekar wrote: >> >> AHB1 on A83T is similar to ahb1 on A31, except parents are different. >> >> clock index 0b1x is PLL6. >> >> >> >> Signed-off-by: Vishnu Patekar >> >> Acked-by: Chen-Yu Tsai >> >> Acked-by: Rob Herring >> >> --- >> >> Documentation/devicetree/bindings/clock/sunxi.txt | 1 + >> >> drivers/clk/sunxi/clk-sunxi.c | 76 +++++++++++++++++++++++ >> >> 2 files changed, 77 insertions(+) >> >> >> >> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt >> >> index 834436f..cba9fe55 100644 >> >> --- a/Documentation/devicetree/bindings/clock/sunxi.txt >> >> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt >> >> @@ -30,6 +30,7 @@ Required properties: >> >> "allwinner,sun6i-a31-ar100-clk" - for the AR100 on A31 >> >> "allwinner,sun9i-a80-cpus-clk" - for the CPUS on A80 >> >> "allwinner,sun6i-a31-ahb1-clk" - for the AHB1 clock on A31 >> >> + "allwinner,sun8i-a83t-ahb1-clk" - for the AHB1 clock on A83T >> >> "allwinner,sun8i-h3-ahb2-clk" - for the AHB2 clock on H3 >> >> "allwinner,sun6i-a31-ahb1-gates-clk" - for the AHB1 gates on A31 >> >> "allwinner,sun8i-a23-ahb1-gates-clk" - for the AHB1 gates on A23 >> >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> >> index 91de0a0..a7aab65 100644 >> >> --- a/drivers/clk/sunxi/clk-sunxi.c >> >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> >> @@ -344,6 +344,67 @@ static void sun6i_ahb1_recalc(struct factors_request *req) >> >> req->rate >>= req->p; >> >> } >> >> >> >> +#define SUN8I_A83T_AHB1_PARENT_PLL6 2 >> >> +/** >> >> + * sun8i_a83t_get_ahb_factors() - calculates m, p factors for AHB >> >> + * AHB rate is calculated as follows >> >> + * rate = parent_rate >> p >> >> + * >> >> + * if parent is pll6, then >> >> + * parent_rate = pll6 rate / (m + 1) >> >> + */ >> >> + >> >> +static void sun8i_a83t_get_ahb1_factors(struct factors_request *req) >> >> +{ >> >> + u8 div, calcp, calcm = 1; >> >> + >> >> + /* >> >> + * clock can only divide, so we will never be able to achieve >> >> + * frequencies higher than the parent frequency >> >> + */ >> >> + if (req->parent_rate && req->rate > req->parent_rate) >> >> + req->rate = req->parent_rate; >> >> + >> >> + div = DIV_ROUND_UP(req->parent_rate, req->rate); >> >> + >> >> + /* calculate pre-divider if parent is pll6 */ >> >> + if (req->parent_index >= SUN8I_A83T_AHB1_PARENT_PLL6) { >> >> + if (div < 4) >> >> + calcp = 0; >> >> + else if (div / 2 < 4) >> >> + calcp = 1; >> >> + else if (div / 4 < 4) >> >> + calcp = 2; >> >> + else >> >> + calcp = 3; >> >> + >> >> + calcm = DIV_ROUND_UP(div, 1 << calcp); >> >> + } else { >> >> + calcp = __roundup_pow_of_two(div); >> >> + calcp = calcp > 3 ? 3 : calcp; >> >> + } >> >> + >> >> + req->rate = (req->parent_rate / calcm) >> calcp; >> >> + req->p = calcp; >> >> + req->m = calcm - 1; >> >> +} >> >> + >> >> +/** >> >> +* sun8i_a83t_ahb1_recalc() - calculates AHB clock rate from m, p factors and >> >> +* parent index >> >> +*/ >> >> +static void sun8i_a83t_ahb1_recalc(struct factors_request *req) >> >> +{ >> >> + req->rate = req->parent_rate; >> >> + >> >> +/* apply pre-divider first if parent is pll6 */ >> > >> > The comment indentation is wrong >> > >> >> + if (req->parent_index >= SUN6I_AHB1_PARENT_PLL6) >> > >> > And this is not the right define you're using. >> > >> > I still believe that duplicating the same logic just because of >> > different dividers is not the way to go. >> > >> > You could solve that easily by adding a table for the muxes, and >> > associate it with parents and dividers, that you could store in >> > clk_factors. >> >> I've similar solution (please ignore a83 specific functions those will >> be common for a31 and a83t). >> https://github.com/vishnupatekar/linux/commit/f7de5b48d886a672b1f6db112fbfd5d2c9849afa >> >> is it aligned to what you're saying? > > Yep. I'd even go a step further, and allow to have directly the core > deal with the pre-divider. > > I guess in your prediv table you could have the index, and either the > offset and width of the divider (if it's a variable one), or its fixed > value. That's good idea, index with shift and width will do as prediv can be variable. > > The generic function would then be able to deal with the rate > adjustments, and you wouldn't need to be able to have anything related > to the parent index in the clock specific functions anymore. > > Does that make sense? Adjusting rate in generic clk_factors_recalc_rate, with prediv shift and width is straight forward, and we can remove recalc function itself, currently it's used only for a31 ahb1. I'm doubtful about getter, In sun6i_get_ahb1_factors we still need to pass req->prediv_width instead of req->parent_index. I'll send separate patch for this prediv handling. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com