Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758819AbcLBAub (ORCPT ); Thu, 1 Dec 2016 19:50:31 -0500 Received: from mail-ua0-f180.google.com ([209.85.217.180]:34854 "EHLO mail-ua0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756202AbcLBAu2 (ORCPT ); Thu, 1 Dec 2016 19:50:28 -0500 MIME-Version: 1.0 In-Reply-To: <8526810.ogr5uYm3lV@wuerfel> References: <20161122213245.17955-1-code@mmayer.net> <8526810.ogr5uYm3lV@wuerfel> From: Markus Mayer Date: Thu, 1 Dec 2016 16:50:26 -0800 Message-ID: Subject: Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs To: Arnd Bergmann Cc: Markus Mayer , Viresh Kumar , "Rafael J . Wysocki" , Power Management List , Broadcom Kernel List , Linux Kernel Mailing List 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: 3370 Lines: 83 On 28 November 2016 at 02:14, Arnd Bergmann wrote: > On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote: >> From: Markus Mayer >> >> This CPUfreq driver provides basic frequency scaling for older Broadcom >> STB SoCs that do not use AVS firmware with DVFS support. There is no >> support for voltage scaling. >> >> Signed-off-by: Markus Mayer >> Acked-by: Viresh Kumar > > This causes multiple build errors in linux-next, please fix asap or > drop the patch again. My feeling is that it's probably too late to > fix it for v4.10, but that's up to Viresh and Rafael of course. > >> +#define BRCMSTB_CPUFREQ_PREFIX "brcmstb" >> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq" >> + >> +/* We search for these compatible strings. */ >> +#define BRCMSTB_DT_CPU_CLK_CTRL "brcm,brcmstb-cpu-clk-div" >> +#define BRCMSTB_DT_MEMC_DDR "brcm,brcmstb-memc-ddr" >> +#define BRCM_AVS_CPU_DATA "brcm,avs-cpu-data-mem" >> + >> +/* We also need a few clocks in device tree. These are node names. */ >> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0" >> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int" >> +#define BRCMSTB_CLK_SW_SCB "sw_scb" > > Not critical but the use of those macros obfuscates the DT interfaces > here and made it harder to analyse what was going on. > > Also, a couple of them are lacking a DT binding. > >> +static int get_frequencies(const struct cpufreq_policy *policy, >> + unsigned int *vco_freq, unsigned int *cpu_freq, >> + unsigned int *scb_freq) >> +{ >> + struct clk *cpu_ndiv_int, *sw_scb; >> + >> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT); >> + if (!cpu_ndiv_int) >> + return -ENODEV; >> + >> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB); >> + if (!sw_scb) >> + return -ENODEV; >> + >> + /* return frequencies in kHz */ >> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000; >> + *cpu_freq = clk_get_rate(policy->clk) / 1000; >> + *scb_freq = clk_get_rate(sw_scb) / 1000; >> + >> + return 0; >> +} > > You really can't do this: > > ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies': > ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of function '__clk_lookup';did you mean 'key_lookup'? [-Werror=implicit-function-declaration] > cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT); > ^~~~~~~~~~~~ > > __clk_lookup is an internal API for the clk providers. > > In particular, relying on undocumented internal names of the > clk provider in a device driver is inappropriate. Do you happen to know of an "approved" way of looking up a clock node? Everything we need is in device tree. We can certainly add bindings for the missing nodes. It just seems somewhat difficult to get at the information in a clean way. >> +static const struct of_device_id brcmstb_cpufreq_match[] = { >> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match); > > This is a simple typo, also causing the build to fail: > > FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct platform_device_id)=24 is not a modulo of the size of section __mod_platform___device_table=392. > > Arnd