From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751716Ab3KSEdu (ORCPT ); Mon, 18 Nov 2013 23:33:50 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:7798 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751350Ab3KSEdr (ORCPT ); Mon, 18 Nov 2013 23:33:47 -0500 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Mon, 18 Nov 2013 20:27:52 -0800 Message-ID: <528AEA27.9090004@nvidia.com> Date: Tue, 19 Nov 2013 13:33:43 +0900 From: Alex Courbot Organization: NVIDIA User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0 MIME-Version: 1.0 To: Thierry Reding CC: Stephen Warren , Peter De Schrijver , Prashant Gaikwad , "linux-tegra@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Mike Turquette Subject: Re: [PATCH] ARM: tegra: properly use FUSE clock References: <1384771247-16547-1-git-send-email-acourbot@nvidia.com> <20131118114330.GD8646@ulmo.nvidia.com> In-Reply-To: <20131118114330.GD8646@ulmo.nvidia.com> X-NVConfidentiality: public Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/18/2013 08:43 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, Nov 18, 2013 at 07:40:47PM +0900, Alexandre Courbot wrote: >> FUSE clock is enabled by most bootloaders, but we cannot expect it to be >> on in all contexts (e.g. kexec). >> >> This patch adds a FUSE clkdev to all Tegra platforms and makes sure >> it is enabled before touching FUSE registers. tegra_init_fuse() is >> invoked during very early boot and thus cannot rely on the clock >> framework ; therefore the FUSE clock is forcibly enabled using a >> register write in that function, and remains that way until the >> clock framework can be used. >> >> Signed-off-by: Alexandre Courbot >> --- >> arch/arm/mach-tegra/fuse.c | 41 +++++++++++++++++++++++++++++++++++++++- >> drivers/clk/tegra/clk-tegra114.c | 1 + >> drivers/clk/tegra/clk-tegra124.c | 1 + >> drivers/clk/tegra/clk-tegra20.c | 1 + > > Isn't this missing the clock driver changes for Tegra30? Ah... Tegra30 > already has this clock defined. I wonder why only Tegra30 has it. grep > says that fuse-tegra isn't used by any drivers, which also indicates > that perhaps we don't need the .dev_id in the first place. We should be > able to get by with just the .con_id = "fuse". Will fix that. > Also are there any reasons to keep this in one single patch? Since none > of the fuse clocks are used yet, I think the clock changes could be a > separate patch that can go in through the clock tree. And there isn't > even a hard runtime dependency, since if the Tegra changes were to go in > without the clock changes, then the fallback code in this patch should > still turn the clock on properly. It just might not be turned off again, > but isn't that something we can live with for a short period of time? I > think perhaps that could even be improved, see further below. > > I've added Mike on Cc, he'll need to either take the patch in through > his tree or Ack this one, so he needs to see it eventually. I will split the change into two patches - at first I thought it would not be worth the trouble, but I overlooked the fact this needed to go through the clock source tree. > >> 4 files changed, 43 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/mach-tegra/fuse.c b/arch/arm/mach-tegra/fuse.c >> index 9a4e910c3796..3b9191b930b5 100644 >> --- a/arch/arm/mach-tegra/fuse.c >> +++ b/arch/arm/mach-tegra/fuse.c >> @@ -22,6 +22,7 @@ >> #include >> #include >> #include >> +#include >> #include >> >> #include "fuse.h" >> @@ -54,6 +55,7 @@ int tegra_cpu_speedo_id; /* only exist in Tegra30 and later */ >> int tegra_soc_speedo_id; >> enum tegra_revision tegra_revision; >> >> +static struct clk *fuse_clk; >> static int tegra_fuse_spare_bit; >> static void (*tegra_init_speedo_data)(void); >> >> @@ -77,6 +79,22 @@ static const char *tegra_revision_name[TEGRA_REVISION_MAX] = { >> [TEGRA_REVISION_A04] = "A04", >> }; >> >> +static void tegra_fuse_enable_clk(void) >> +{ >> + if (IS_ERR(fuse_clk)) >> + fuse_clk = clk_get_sys("fuse-tegra", "fuse"); >> + if (IS_ERR(fuse_clk)) >> + return; > > Perhaps instead of just returning here, this should actually be where > the code to enable the clock should go. > >> + clk_prepare_enable(fuse_clk); >> +} >> + >> +static void tegra_fuse_disable_clk(void) >> +{ >> + if (IS_ERR(fuse_clk)) >> + return; > > And this is where we could disable it again. That way we should get > equal functionality in both cases. What Stephen said, basically - but let me address that in the other mail. Thanks for the review! Alex.