[PATCH] ARM: tegra: properly use FUSE clock

[PATCH] ARM: tegra: properly use FUSE clock

[Alexandre Courbot]

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 <acourbot@nvidia.com>
---
 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 +
 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 <linux/io.h>
 #include <linux/export.h>
 #include <linux/random.h>
+#include <linux/clk.h>
 #include <linux/tegra-soc.h>
 
 #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;
+	clk_prepare_enable(fuse_clk);
+}
+
+static void tegra_fuse_disable_clk(void)
+{
+	if (IS_ERR(fuse_clk))
+		return;
+	clk_disable_unprepare(fuse_clk);
+}
+
 u32 tegra_fuse_readl(unsigned long offset)
 {
 	return tegra_apb_readl(TEGRA_FUSE_BASE + offset);
@@ -84,7 +102,15 @@ u32 tegra_fuse_readl(unsigned long offset)
 
 bool tegra_spare_fuse(int bit)
 {
-	return tegra_fuse_readl(tegra_fuse_spare_bit + bit * 4);
+	bool ret;
+
+	tegra_fuse_enable_clk();
+
+	ret = tegra_fuse_readl(tegra_fuse_spare_bit + bit * 4);
+
+	tegra_fuse_disable_clk();
+
+	return ret;
 }
 
 static enum tegra_revision tegra_get_revision(u32 id)
@@ -113,10 +139,14 @@ static void tegra_get_process_id(void)
 {
 	u32 reg;
 
+	tegra_fuse_enable_clk();
+
 	reg = tegra_fuse_readl(tegra_fuse_spare_bit);
 	tegra_cpu_process_id = (reg >> 6) & 3;
 	reg = tegra_fuse_readl(tegra_fuse_spare_bit);
 	tegra_core_process_id = (reg >> 12) & 3;
+
+	tegra_fuse_disable_clk();
 }
 
 u32 tegra_read_chipid(void)
@@ -159,6 +189,15 @@ void __init tegra_init_fuse(void)
 	reg |= 1 << 28;
 	writel(reg, IO_ADDRESS(TEGRA_CLK_RESET_BASE + 0x48));
 
+	/*
+	 * Enable FUSE clock. This needs to be hardcoded because the clock
+	 * subsystem is not active during early boot.
+	 */
+	reg = readl(IO_ADDRESS(TEGRA_CLK_RESET_BASE + 0x14));
+	reg |= 1 << 7;
+	writel(reg, IO_ADDRESS(TEGRA_CLK_RESET_BASE + 0x14));
+	fuse_clk = ERR_PTR(-EINVAL);
+
 	reg = tegra_fuse_readl(FUSE_SKU_INFO);
 	randomness[0] = reg;
 	tegra_sku_id = reg & 0xFF;
diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
index dbab27f773b5..a83fdfd19bfc 100644
--- a/drivers/clk/tegra/clk-tegra114.c
+++ b/drivers/clk/tegra/clk-tegra114.c
@@ -925,6 +925,7 @@ static struct tegra_devclk devclks[] __initdata = {
 	{ .con_id = "sclk", .dt_id = TEGRA114_CLK_SCLK },
 	{ .con_id = "hclk", .dt_id = TEGRA114_CLK_HCLK },
 	{ .con_id = "pclk", .dt_id = TEGRA114_CLK_PCLK },
+	{ .con_id = "fuse", .dev_id = "fuse-tegra", .dt_id = TEGRA114_CLK_FUSE },
 	{ .dev_id = "rtc-tegra", .dt_id = TEGRA114_CLK_RTC },
 	{ .dev_id = "timer", .dt_id = TEGRA114_CLK_TIMER },
 };
diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
index 266e80b51d38..0a324a2d7309 100644
--- a/drivers/clk/tegra/clk-tegra124.c
+++ b/drivers/clk/tegra/clk-tegra124.c
@@ -1007,6 +1007,7 @@ static struct tegra_devclk devclks[] __initdata = {
 	{ .con_id = "sclk", .dt_id = TEGRA124_CLK_SCLK },
 	{ .con_id = "hclk", .dt_id = TEGRA124_CLK_HCLK },
 	{ .con_id = "pclk", .dt_id = TEGRA124_CLK_PCLK },
+	{ .con_id = "fuse", .dev_id = "fuse-tegra", .dt_id = TEGRA124_CLK_FUSE },
 	{ .dev_id = "rtc-tegra", .dt_id = TEGRA124_CLK_RTC },
 	{ .dev_id = "timer", .dt_id = TEGRA124_CLK_TIMER },
 };
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 58faac5f509e..48dca0f4762a 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -452,6 +452,7 @@ static struct tegra_devclk devclks[] __initdata = {
 	{ .con_id = "sclk", .dt_id = TEGRA20_CLK_SCLK },
 	{ .con_id = "hclk", .dt_id = TEGRA20_CLK_HCLK },
 	{ .con_id = "pclk", .dt_id = TEGRA20_CLK_PCLK },
+	{ .con_id = "fuse", .dev_id = "fuse-tegra", .dt_id = TEGRA20_CLK_FUSE },
 	{ .con_id = "twd", .dt_id = TEGRA20_CLK_TWD },
 	{ .con_id = "audio", .dt_id = TEGRA20_CLK_AUDIO },
 	{ .con_id = "audio_2x", .dt_id = TEGRA20_CLK_AUDIO_2X },
-- 
1.8.4.2

Re: [PATCH] ARM: tegra: properly use FUSE clock

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6844 bytes --]

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 <acourbot@nvidia.com>
> ---
>  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".

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.

>  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 <linux/io.h>
>  #include <linux/export.h>
>  #include <linux/random.h>
> +#include <linux/clk.h>
>  #include <linux/tegra-soc.h>
>  
>  #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.

Thierry

> +	clk_disable_unprepare(fuse_clk);
> +}
> +
>  u32 tegra_fuse_readl(unsigned long offset)
>  {
>  	return tegra_apb_readl(TEGRA_FUSE_BASE + offset);
> @@ -84,7 +102,15 @@ u32 tegra_fuse_readl(unsigned long offset)
>  
>  bool tegra_spare_fuse(int bit)
>  {
> -	return tegra_fuse_readl(tegra_fuse_spare_bit + bit * 4);
> +	bool ret;
> +
> +	tegra_fuse_enable_clk();
> +
> +	ret = tegra_fuse_readl(tegra_fuse_spare_bit + bit * 4);
> +
> +	tegra_fuse_disable_clk();
> +
> +	return ret;
>  }
>  
>  static enum tegra_revision tegra_get_revision(u32 id)
> @@ -113,10 +139,14 @@ static void tegra_get_process_id(void)
>  {
>  	u32 reg;
>  
> +	tegra_fuse_enable_clk();
> +
>  	reg = tegra_fuse_readl(tegra_fuse_spare_bit);
>  	tegra_cpu_process_id = (reg >> 6) & 3;
>  	reg = tegra_fuse_readl(tegra_fuse_spare_bit);
>  	tegra_core_process_id = (reg >> 12) & 3;
> +
> +	tegra_fuse_disable_clk();
>  }
>  
>  u32 tegra_read_chipid(void)
> @@ -159,6 +189,15 @@ void __init tegra_init_fuse(void)
>  	reg |= 1 << 28;
>  	writel(reg, IO_ADDRESS(TEGRA_CLK_RESET_BASE + 0x48));
>  
> +	/*
> +	 * Enable FUSE clock. This needs to be hardcoded because the clock
> +	 * subsystem is not active during early boot.
> +	 */
> +	reg = readl(IO_ADDRESS(TEGRA_CLK_RESET_BASE + 0x14));
> +	reg |= 1 << 7;
> +	writel(reg, IO_ADDRESS(TEGRA_CLK_RESET_BASE + 0x14));
> +	fuse_clk = ERR_PTR(-EINVAL);
> +
>  	reg = tegra_fuse_readl(FUSE_SKU_INFO);
>  	randomness[0] = reg;
>  	tegra_sku_id = reg & 0xFF;
> diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
> index dbab27f773b5..a83fdfd19bfc 100644
> --- a/drivers/clk/tegra/clk-tegra114.c
> +++ b/drivers/clk/tegra/clk-tegra114.c
> @@ -925,6 +925,7 @@ static struct tegra_devclk devclks[] __initdata = {
>  	{ .con_id = "sclk", .dt_id = TEGRA114_CLK_SCLK },
>  	{ .con_id = "hclk", .dt_id = TEGRA114_CLK_HCLK },
>  	{ .con_id = "pclk", .dt_id = TEGRA114_CLK_PCLK },
> +	{ .con_id = "fuse", .dev_id = "fuse-tegra", .dt_id = TEGRA114_CLK_FUSE },
>  	{ .dev_id = "rtc-tegra", .dt_id = TEGRA114_CLK_RTC },
>  	{ .dev_id = "timer", .dt_id = TEGRA114_CLK_TIMER },
>  };
> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
> index 266e80b51d38..0a324a2d7309 100644
> --- a/drivers/clk/tegra/clk-tegra124.c
> +++ b/drivers/clk/tegra/clk-tegra124.c
> @@ -1007,6 +1007,7 @@ static struct tegra_devclk devclks[] __initdata = {
>  	{ .con_id = "sclk", .dt_id = TEGRA124_CLK_SCLK },
>  	{ .con_id = "hclk", .dt_id = TEGRA124_CLK_HCLK },
>  	{ .con_id = "pclk", .dt_id = TEGRA124_CLK_PCLK },
> +	{ .con_id = "fuse", .dev_id = "fuse-tegra", .dt_id = TEGRA124_CLK_FUSE },
>  	{ .dev_id = "rtc-tegra", .dt_id = TEGRA124_CLK_RTC },
>  	{ .dev_id = "timer", .dt_id = TEGRA124_CLK_TIMER },
>  };
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 58faac5f509e..48dca0f4762a 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -452,6 +452,7 @@ static struct tegra_devclk devclks[] __initdata = {
>  	{ .con_id = "sclk", .dt_id = TEGRA20_CLK_SCLK },
>  	{ .con_id = "hclk", .dt_id = TEGRA20_CLK_HCLK },
>  	{ .con_id = "pclk", .dt_id = TEGRA20_CLK_PCLK },
> +	{ .con_id = "fuse", .dev_id = "fuse-tegra", .dt_id = TEGRA20_CLK_FUSE },
>  	{ .con_id = "twd", .dt_id = TEGRA20_CLK_TWD },
>  	{ .con_id = "audio", .dt_id = TEGRA20_CLK_AUDIO },
>  	{ .con_id = "audio_2x", .dt_id = TEGRA20_CLK_AUDIO_2X },
> -- 
> 1.8.4.2
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ARM: tegra: properly use FUSE clock

[Stephen Warren]

On 11/18/2013 04:43 AM, Thierry Reding wrote:
> 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 <acourbot@nvidia.com> --- 
>> 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".
> 
> 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.
> 
>> 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
>> <linux/io.h> #include <linux/export.h> #include <linux/random.h> 
>> +#include <linux/clk.h> #include <linux/tegra-soc.h>
>> 
>> #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.

That would need a shared lock with the clock code; at some point, the
clock will be registered, and the clock subsystem in control of the
enable bit. I think having a very early tegra_init_fuse() come along
and force the clock on, and then having the rest of the fuse code use
the clock object as soon as it's available, is the safest approach.

Of course, I suppose there's still a window where the following might
happen:

cpu 0:
- tegra_fuse_enable_clk entered
- fails to clk_get
				cpu 1
				- tegra clk driver is registered
				- clk subsystem initcall disables all
				  unused clocks
- access a fuse register

-> badness

I'm not sure how to protect against that, unless we simply assume that
all the fuse driver functions are guaranteed to happen early and
before the clk subsystem's initcall, so that can't happen?

Re: [PATCH] ARM: tegra: properly use FUSE clock

[Alex Courbot]

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 <acourbot@nvidia.com>
>> ---
>>   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 <linux/io.h>
>>   #include <linux/export.h>
>>   #include <linux/random.h>
>> +#include <linux/clk.h>
>>   #include <linux/tegra-soc.h>
>>
>>   #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.

Re: [PATCH] ARM: tegra: properly use FUSE clock

[Alex Courbot]

On 11/19/2013 08:48 AM, Stephen Warren wrote:
> On 11/18/2013 04:43 AM, Thierry Reding wrote:
>> 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 <acourbot@nvidia.com> ---
>>> 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".
>>
>> 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.
>>
>>> 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
>>> <linux/io.h> #include <linux/export.h> #include <linux/random.h>
>>> +#include <linux/clk.h> #include <linux/tegra-soc.h>
>>>
>>> #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.
>
> That would need a shared lock with the clock code; at some point, the
> clock will be registered, and the clock subsystem in control of the
> enable bit. I think having a very early tegra_init_fuse() come along
> and force the clock on, and then having the rest of the fuse code use
> the clock object as soon as it's available, is the safest approach.
>
> Of course, I suppose there's still a window where the following might
> happen:
>
> cpu 0:
> - tegra_fuse_enable_clk entered
> - fails to clk_get
> 				cpu 1
> 				- tegra clk driver is registered
> 				- clk subsystem initcall disables all
> 				  unused clocks
> - access a fuse register
>
> -> badness

It seems to me that both solutions require a shared lock with the clock 
code in order to be theoretically safe. The situation you described 
requires a lock to be addressed ; and unless I missed something, 
anything that could break with Thierry's proposal could also only do so 
if we "hold" the clock when the tegra clk driver is registered.

However we can consider ourselves safe for both cases if we know for 
sure that there is no fuse function in use when this happens. Since 
of_clk_init() is called very early during boot with SMP and preemption 
disabled, isn't that always the case?

>
> I'm not sure how to protect against that, unless we simply assume that
> all the fuse driver functions are guaranteed to happen early and
> before the clk subsystem's initcall, so that can't happen?

That's probably the current behavior actually - the clk subsystem's 
initcall disables the fuse clock, fuse functions are never used after 
that, and we only notice when we try to kexec into another kernel that 
the fuse clock is not enabled as expected.

With that in mind, it's probably a good idea to preemptively implement 
proper clock enabling in fuse.c, in case we come to use it more in the 
future.

Thanks,
Alex.

Re: [PATCH] ARM: tegra: properly use FUSE clock

[Stephen Warren]

On 11/18/2013 09:53 PM, Alex Courbot wrote:
> On 11/19/2013 08:48 AM, Stephen Warren wrote:
>> On 11/18/2013 04:43 AM, Thierry Reding wrote:
>>> 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 <acourbot@nvidia.com> ---
>>>> 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".
>>>
>>> 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.
>>>
>>>> 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
>>>> <linux/io.h> #include <linux/export.h> #include <linux/random.h>
>>>> +#include <linux/clk.h> #include <linux/tegra-soc.h>
>>>>
>>>> #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.
>>
>> That would need a shared lock with the clock code; at some point, the
>> clock will be registered, and the clock subsystem in control of the
>> enable bit. I think having a very early tegra_init_fuse() come along
>> and force the clock on, and then having the rest of the fuse code use
>> the clock object as soon as it's available, is the safest approach.
>>
>> Of course, I suppose there's still a window where the following might
>> happen:
>>
>> cpu 0:
>> - tegra_fuse_enable_clk entered
>> - fails to clk_get
>>                 cpu 1
>>                 - tegra clk driver is registered
>>                 - clk subsystem initcall disables all
>>                   unused clocks
>> - access a fuse register
>>
>> -> badness
> 
> It seems to me that both solutions require a shared lock with the clock
> code in order to be theoretically safe. The situation you described
> requires a lock to be addressed ; and unless I missed something,
> anything that could break with Thierry's proposal could also only do so
> if we "hold" the clock when the tegra clk driver is registered.
> 
> However we can consider ourselves safe for both cases if we know for
> sure that there is no fuse function in use when this happens. Since
> of_clk_init() is called very early during boot with SMP and preemption
> disabled, isn't that always the case?

Yes, that's true. So, I think it's safe, in practice, without the lock.