Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755306AbbGFTw6 (ORCPT ); Mon, 6 Jul 2015 15:52:58 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:46545 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754604AbbGFTw4 (ORCPT ); Mon, 6 Jul 2015 15:52:56 -0400 Message-ID: <559ADC94.4080505@codeaurora.org> Date: Mon, 06 Jul 2015 12:52:52 -0700 From: Stephen Boyd User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Sudeep Holla CC: "linux-kernel@vger.kernel.org" , "linux-pm@vger.kernel.org" , "linux-clk@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Liviu Dudau , Lorenzo Pieralisi , "Jon Medhurst (Tixy)" , Arnd Bergmann , Kevin Hilman , Olof Johansson , Mike Turquette Subject: Re: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System Control Processor) References: <1433760002-24120-1-git-send-email-sudeep.holla@arm.com> <1433760002-24120-4-git-send-email-sudeep.holla@arm.com> <20150702172310.GF4301@codeaurora.org> <5596A1B6.8020307@arm.com> In-Reply-To: <5596A1B6.8020307@arm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4670 Lines: 154 On 07/03/2015 07:52 AM, Sudeep Holla wrote: > Hi Stephen, > > > Thanks for the review. > > On 02/07/15 18:23, Stephen Boyd wrote: >> On 06/08, Sudeep Holla wrote: >>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig >>> index 9897f353bf1a..0fe8daefc105 100644 >>> --- a/drivers/clk/Kconfig >>> +++ b/drivers/clk/Kconfig >>> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808 >>> clocked at 32KHz each. Clkout1 is always on, Clkout2 can off >>> by control register. >>> >>> +config COMMON_CLK_SCPI >>> + tristate "Clock driver controlled via SCPI interface" >>> + depends on ARM_SCPI_PROTOCOL || COMPILE_TEST >>> + ---help--- >>> + This driver provides support for clocks that are controlled >>> + by firmware that implements the SCPI interface. >>> + >>> + This driver uses SCPI Message Protocol to interact with the >>> + firmware providing all the clock controls. >> >> The tabbing is weird here. Both paragraphs should have the same >> alignment. >> > > Indeed, sorry for that, fixed now. > >>> diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c >>> new file mode 100644 >>> index 000000000000..707b3430c55f >>> --- /dev/null >>> +++ b/drivers/clk/clk-scpi.c >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> Please include as well. >> > > Added now, didn't bother as of_platform.h includes it. > >>> + >>> +struct scpi_clk { >>> + u32 id; >>> + const char *name; >> >> Do you need this? Or can you just use __clk_get_name() in places >> where the name is used? >> > > Not used, so removed it. > > > [...] > >>> +static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate, >>> + unsigned long *parent_rate) >>> +{ >> >> Maybe a comment here like: >> >> /* >> * We can't figure out what rate it will be, so just return the rate >> * back to the caller. scpi_clk_recalc_rate() will be called >> * after the rate is set and we'll know what rate the clock is >> * running at then. >> */ >> > > Done > >>> + return rate; >>> +} >>> + >>> +static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate, >>> + unsigned long parent_rate) >>> +{ >>> + struct scpi_clk *clk = to_scpi_clk(hw); >>> + >>> + return clk->scpi_ops->clk_set_val(clk->id, rate); >>> +} >>> + >>> +static void scpi_clk_disable(struct clk_hw *hw) >>> +{ >>> + scpi_clk_set_rate(hw, 0, 0); >> >> Does this mean you have to set a rate to enable the clock? Are >> you relying on drivers to call clk_set_rate() to implicitly >> enable the clock? If so, it would be better to cache the rate of >> the clock in set_rate if the clock isn't enabled in software and >> then send the cached rate during enable. >> > > Agreed, I have asked the firmware/SCPI specification guys about > more details on what to expect from firmware. Once they get back, > will update the code considering your feedback. > > [...] > >>> +static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned >>> long rate) >>> +{ >>> + int idx, max_opp = clk->info->count; >>> + struct scpi_opp *opp = clk->info->opps; >> >> const? >> > > All the 3 above fixed now. > > [...] > >>> + >>> + clk = devm_clk_register(dev, &sclk->hw); >>> + if (!IS_ERR(clk) && max) >>> + clk_set_rate_range(clk, min, max); >> >> Hm.. we're planning to make clk_register() return a struct >> clk_hw, so this will block that. We need some sort of clk_hw API >> that allows us to setup min/max limits on the clock from the >> provider side. Care to add that? >> > > Can you provide pointer to the patches or the tree containing those > changes ? Are they targeted for v4.3 ? > If I have time I may try to start doing the clk_register() conversion, but it will take a while so I doubt it will be in v4.3. I'm asking if you can add a clk_hw based API that does something like clk_set_rate_range() without requiring a struct clk pointer. i.e. clk_hw_set_rate_range(struct clk_hw *hw, min, max) that constraints the min/max rate of the clock. This way, the driver is only using clk provider APIs and not clk consumer APIs. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/