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@xxxxxxxxxx>
> > ---
> >   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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/