On 10/29/2012 01:21 AM, Danny Huang wrote:
> This patch adds speedo-based identification support for T30.
> diff --git a/arch/arm/mach-tegra/fuse.c b/arch/arm/mach-tegra/fuse.c
> -#define FUSE_SPARE_BIT 0x200
> +
> +#define TEGRA20_FUSE_SPARE_BIT 0x200
> +#define TEGRA30_FUSE_SPARE_BIT 0x244
> +
> +static int tegra_fuse_spare_bit;
Can all the spare bit rework, and also prototype changes for
tegra_fuse_readl() and tegra_spare_fuse() be pulled out into a separate
patch at the start of the series?
> +int tegra_cpu_speedo_id;
Does Tegra20 not have a separate cpu_speedo_id? Should this variable be
added in patch 1 and appropriately initialized for Tegra20? If it's
Tegra30-specific, or Tegra30-and-later, a comment to that effect would
be useful. Is there a way to ensure that Tegra20-specific code doesn't
use that variable if it's not applicable?
> @@ -107,9 +112,18 @@ void tegra_init_fuse(void)
> id = readl_relaxed(IO_ADDRESS(TEGRA_APB_MISC_BASE) + 0x804);
> tegra_chip_id = (id >> 8) & 0xff;
>
> - tegra_revision = tegra_get_revision(id);
> -
> - tegra20_init_speedo_data();
> + switch (tegra_chip_id) {
> + case TEGRA20:
> + tegra_fuse_spare_bit = TEGRA20_FUSE_SPARE_BIT;
> + tegra_revision = tegra_get_revision(id);
> + tegra20_init_speedo_data();
> + break;
> + case TEGRA30:
> + tegra_fuse_spare_bit = TEGRA30_FUSE_SPARE_BIT;
> + tegra_revision = tegra_get_revision(id);
> + tegra30_init_speedo_data();
> + break;
> + }
I think there, I'd prefer to see:
switch (tegra_chip_id) {
case TEGRA20:
tegra_fuse_spare_bit = TEGRA20_FUSE_SPARE_BIT;
break;
case TEGRA30:
tegra_fuse_spare_bit = TEGRA30_FUSE_SPARE_BIT;
break;
}
tegra_revision = tegra_get_revision(id);
switch (tegra_chip_id) {
case TEGRA20:
tegra20_init_speedo_data();
break;
case TEGRA30:
tegra30_init_speedo_data();
break;
}
... to avoid duplicating the tegra_get_revision() call.
If this ends up needing a lot of separate switch statements in sequence,
you can always put the SoC-specific data into a struct, and do:
struct tegra_fuse_soc_data *sd = ...;
sd->set_spare_fuse_bit();
tegra_revision = tegra_get_revision(id);
sd->init_speedo_data();
although I don't think the complexity requires that yet.
> diff --git a/arch/arm/mach-tegra/tegra30_speedo.c
> b/arch/arm/mach-tegra/tegra30_speedo.c
(similar comments apply here as for the table/array size checking in
patch 1)
> +static int threshold_index;
> +static int package_id;
Do those need to be globals? Can they simply be passed between the
appropriate functions?
> +static void fuse_speedo_calib(u32 *speedo_g, u32 *speedo_lp)
> + WARN_ON(!speedo_g || !speedo_lp);
That hardly seems worth checking since this function is called from one
specific place later in this file...
> +static void rev_sku_to_speedo_ids(int rev, int sku)
> +{
> + switch (rev) {
> + case TEGRA_REVISION_A01:
> + tegra_cpu_speedo_id = 0;
> + tegra_soc_speedo_id = 0;
> + threshold_index = 0;
> + break;
> + case TEGRA_REVISION_A02:
> + case TEGRA_REVISION_A03:
> + switch (sku) {
> + case 0x87:
...
> + default:
> + pr_err("Tegra3 Rev-A02: Reserved pkg: %d\n",
> + package_id);
> + BUG();
> + break;
> + }
> + break;
Why BUG() there, but not:
> + default:
> + pr_err("Tegra3: Unknown SKU %d\n", sku);
> + tegra_cpu_speedo_id = 0;
> + tegra_soc_speedo_id = 0;
> + threshold_index = 0;
> + break;
> + }
> + break;
> + default:
... but do here:
> + BUG();
> + break;
> + }
> +}
> +void tegra30_init_speedo_data(void)
> + for (i = 0; i < CPU_PROCESS_CORNERS_NUM; i++) {
> + if (cpu_speedo_val <
> + cpu_process_speedos[threshold_index][i]) {
> + break;
> + }
> + }
> + tegra_cpu_process_id = i - 1;
> +
> + if (tegra_cpu_process_id == -1) {
> + pr_err("****************************************************");
> + pr_err("****************************************************");
> + pr_err("* tegra3_speedo: CPU speedo value %3d out of range *",
> + cpu_speedo_val);
> + pr_err("****************************************************");
> + pr_err("****************************************************");
Just drop the lines of ***, and the * around the text in the middle
pr_err() too.
> +
> + tegra_cpu_speedo_id = 1;
Shouldn't that fix the out-of-range tegra_cpu_process_id value?
This and the previous comment apply to the following calculation of
tegra_core_process_id too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
