Re: [PATCH v11 3/9] ARM: l2c: Refactor the driver to use commit-like interface

[Nishanth Menon]

On 13:19-20150105, Marek Szyprowski wrote:
> From: Tomasz Figa <t.figa@xxxxxxxxxxx>
> 
> Certain implementations of secure hypervisors (namely the one found on
> Samsung Exynos-based boards) do not provide access to individual L2C
> registers. This makes the .write_sec()-based interface insufficient and
> provoking ugly hacks.
> 
> This patch is first step to make the driver not rely on availability of
> writes to individual registers. This is achieved by refactoring the
> driver to use a commit-like operation scheme: all register values are
> prepared first and stored in an instance of l2x0_regs struct and then a
> single callback is responsible to flush those values to the hardware.
> 
> Signed-off-by: Tomasz Figa <t.figa@xxxxxxxxxxx>
> [mszyprow: rebased onto 'ARM: l2c: use l2c_write_sec() for restoring
>  latency and filter regs' patch]
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> ---
>  arch/arm/mm/cache-l2x0.c | 210 ++++++++++++++++++++++++++---------------------
>  1 file changed, 115 insertions(+), 95 deletions(-)
> 
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 0aeeaa95c42d..f9013320c8ce 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -41,12 +41,14 @@ struct l2c_init_data {
>  	void (*enable)(void __iomem *, u32, unsigned);
>  	void (*fixup)(void __iomem *, u32, struct outer_cache_fns *);
>  	void (*save)(void __iomem *);
> +	void (*configure)(void __iomem *);
>  	struct outer_cache_fns outer_cache;
>  };
>  
>  #define CACHE_LINE_SIZE		32
>  
>  static void __iomem *l2x0_base;
> +static const struct l2c_init_data *l2x0_data;
>  static DEFINE_RAW_SPINLOCK(l2x0_lock);
>  static u32 l2x0_way_mask;	/* Bitmask of active ways */
>  static u32 l2x0_size;
> @@ -106,6 +108,14 @@ static inline void l2c_unlock(void __iomem *base, unsigned num)
>  	}
>  }
>  
> +static void l2c_configure(void __iomem *base)
> +{
> +	if (l2x0_data->configure)
> +		l2x0_data->configure(base);
> +
> +	l2c_write_sec(l2x0_saved_regs.aux_ctrl, base, L2X0_AUX_CTRL);
> +}
> +
>  /*
>   * Enable the L2 cache controller.  This function must only be
>   * called when the cache controller is known to be disabled.
> @@ -114,7 +124,12 @@ static void l2c_enable(void __iomem *base, u32 aux, unsigned num_lock)
>  {
>  	unsigned long flags;
>  
> -	l2c_write_sec(aux, base, L2X0_AUX_CTRL);
> +	/* Do not touch the controller if already enabled. */
> +	if (readl_relaxed(base + L2X0_CTRL) & L2X0_CTRL_EN)
> +		return;
> +
> +	l2x0_saved_regs.aux_ctrl = aux;
> +	l2c_configure(base);
>  
>  	l2c_unlock(base, num_lock);
>  
> @@ -208,6 +223,11 @@ static void l2c_save(void __iomem *base)
>  	l2x0_saved_regs.aux_ctrl = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
>  }
>  
> +static void l2c_resume(void)
> +{
> +	l2c_enable(l2x0_base, l2x0_saved_regs.aux_ctrl, l2x0_data->num_lock);
> +}
> +
>  /*
>   * L2C-210 specific code.
>   *
> @@ -288,14 +308,6 @@ static void l2c210_sync(void)
>  	__l2c210_cache_sync(l2x0_base);
>  }
>  
> -static void l2c210_resume(void)
> -{
> -	void __iomem *base = l2x0_base;
> -
> -	if (!(readl_relaxed(base + L2X0_CTRL) & L2X0_CTRL_EN))
> -		l2c_enable(base, l2x0_saved_regs.aux_ctrl, 1);
> -}
> -
>  static const struct l2c_init_data l2c210_data __initconst = {
>  	.type = "L2C-210",
>  	.way_size_0 = SZ_8K,
> @@ -309,7 +321,7 @@ static const struct l2c_init_data l2c210_data __initconst = {
>  		.flush_all = l2c210_flush_all,
>  		.disable = l2c_disable,
>  		.sync = l2c210_sync,
> -		.resume = l2c210_resume,
> +		.resume = l2c_resume,
>  	},
>  };
>  
> @@ -466,7 +478,7 @@ static const struct l2c_init_data l2c220_data = {
>  		.flush_all = l2c220_flush_all,
>  		.disable = l2c_disable,
>  		.sync = l2c220_sync,
> -		.resume = l2c210_resume,
> +		.resume = l2c_resume,
>  	},
>  };
>  
> @@ -615,39 +627,29 @@ static void __init l2c310_save(void __iomem *base)
>  							L310_POWER_CTRL);
>  }
>  
> -static void l2c310_resume(void)
> +static void l2c310_configure(void __iomem *base)
>  {
> -	void __iomem *base = l2x0_base;
> +	unsigned revision;
>  
> -	if (!(readl_relaxed(base + L2X0_CTRL) & L2X0_CTRL_EN)) {
> -		unsigned revision;
> -
> -		/* restore pl310 setup */
> -		l2c_write_sec(l2x0_saved_regs.tag_latency, base,
> -			      L310_TAG_LATENCY_CTRL);
> -		l2c_write_sec(l2x0_saved_regs.data_latency, base,
> -			      L310_DATA_LATENCY_CTRL);
> -		l2c_write_sec(l2x0_saved_regs.filter_end, base,
> -			      L310_ADDR_FILTER_END);
> -		l2c_write_sec(l2x0_saved_regs.filter_start, base,
> -			      L310_ADDR_FILTER_START);
> -
> -		revision = readl_relaxed(base + L2X0_CACHE_ID) &
> -				L2X0_CACHE_ID_RTL_MASK;
> -
> -		if (revision >= L310_CACHE_ID_RTL_R2P0)
> -			l2c_write_sec(l2x0_saved_regs.prefetch_ctrl, base,
> -				      L310_PREFETCH_CTRL);
> -		if (revision >= L310_CACHE_ID_RTL_R3P0)
> -			l2c_write_sec(l2x0_saved_regs.pwr_ctrl, base,
> -				      L310_POWER_CTRL);
> -
> -		l2c_enable(base, l2x0_saved_regs.aux_ctrl, 8);
> -
> -		/* Re-enable full-line-of-zeros for Cortex-A9 */
> -		if (l2x0_saved_regs.aux_ctrl & L310_AUX_CTRL_FULL_LINE_ZERO)
> -			set_auxcr(get_auxcr() | BIT(3) | BIT(2) | BIT(1));
> -	}
> +	/* restore pl310 setup */
> +	l2c_write_sec(l2x0_saved_regs.tag_latency, base,
> +		      L310_TAG_LATENCY_CTRL);
> +	l2c_write_sec(l2x0_saved_regs.data_latency, base,
> +		      L310_DATA_LATENCY_CTRL);
> +	l2c_write_sec(l2x0_saved_regs.filter_end, base,
> +		      L310_ADDR_FILTER_END);
> +	l2c_write_sec(l2x0_saved_regs.filter_start, base,
> +		      L310_ADDR_FILTER_START);
> +
> +	revision = readl_relaxed(base + L2X0_CACHE_ID) &
> +				 L2X0_CACHE_ID_RTL_MASK;
> +
> +	if (revision >= L310_CACHE_ID_RTL_R2P0)
> +		l2c_write_sec(l2x0_saved_regs.prefetch_ctrl, base,
> +			      L310_PREFETCH_CTRL);
> +	if (revision >= L310_CACHE_ID_RTL_R3P0)
> +		l2c_write_sec(l2x0_saved_regs.pwr_ctrl, base,
> +			      L310_POWER_CTRL);
>  }
>  
>  static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, void *data)
> @@ -699,6 +701,23 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
>  		aux &= ~(L310_AUX_CTRL_FULL_LINE_ZERO | L310_AUX_CTRL_EARLY_BRESP);
>  	}
>  
> +	/* r3p0 or later has power control register */
> +	if (rev >= L310_CACHE_ID_RTL_R3P0)
> +		l2x0_saved_regs.pwr_ctrl = L310_DYNAMIC_CLK_GATING_EN |
> +						L310_STNDBY_MODE_EN;
> +
> +	/*
> +	 * Always enable non-secure access to the lockdown registers -
> +	 * we write to them as part of the L2C enable sequence so they
> +	 * need to be accessible.
> +	 */
> +	aux |= L310_AUX_CTRL_NS_LOCKDOWN;
> +
> +	l2c_enable(base, aux, num_lock);
> +
> +	/* Read back resulting AUX_CTRL value as it could have been altered. */
> +	aux = readl_relaxed(base + L2X0_AUX_CTRL);
> +
>  	if (aux & (L310_AUX_CTRL_DATA_PREFETCH | L310_AUX_CTRL_INSTR_PREFETCH)) {
>  		u32 prefetch = readl_relaxed(base + L310_PREFETCH_CTRL);
>  
> @@ -712,23 +731,12 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
>  	if (rev >= L310_CACHE_ID_RTL_R3P0) {
>  		u32 power_ctrl;
>  
> -		l2c_write_sec(L310_DYNAMIC_CLK_GATING_EN | L310_STNDBY_MODE_EN,
> -			      base, L310_POWER_CTRL);
>  		power_ctrl = readl_relaxed(base + L310_POWER_CTRL);
>  		pr_info("L2C-310 dynamic clock gating %sabled, standby mode %sabled\n",
>  			power_ctrl & L310_DYNAMIC_CLK_GATING_EN ? "en" : "dis",
>  			power_ctrl & L310_STNDBY_MODE_EN ? "en" : "dis");
>  	}
>  
> -	/*
> -	 * Always enable non-secure access to the lockdown registers -
> -	 * we write to them as part of the L2C enable sequence so they
> -	 * need to be accessible.
> -	 */
> -	aux |= L310_AUX_CTRL_NS_LOCKDOWN;
> -
> -	l2c_enable(base, aux, num_lock);
> -
>  	if (aux & L310_AUX_CTRL_FULL_LINE_ZERO) {
>  		set_auxcr(get_auxcr() | BIT(3) | BIT(2) | BIT(1));
>  		cpu_notifier(l2c310_cpu_enable_flz, 0);
> @@ -760,11 +768,11 @@ static void __init l2c310_fixup(void __iomem *base, u32 cache_id,
>  
>  	if (revision >= L310_CACHE_ID_RTL_R3P0 &&
>  	    revision < L310_CACHE_ID_RTL_R3P2) {
> -		u32 val = readl_relaxed(base + L310_PREFETCH_CTRL);
> +		u32 val = l2x0_saved_regs.prefetch_ctrl;
>  		/* I don't think bit23 is required here... but iMX6 does so */
>  		if (val & (BIT(30) | BIT(23))) {
>  			val &= ~(BIT(30) | BIT(23));
> -			l2c_write_sec(val, base, L310_PREFETCH_CTRL);
> +			l2x0_saved_regs.prefetch_ctrl = val;
>  			errata[n++] = "752271";
>  		}
>  	}
> @@ -800,6 +808,15 @@ static void l2c310_disable(void)
>  	l2c_disable();
>  }
>  
> +static void l2c310_resume(void)
> +{
> +	l2c_resume();
> +
> +	/* Re-enable full-line-of-zeros for Cortex-A9 */
> +	if (l2x0_saved_regs.aux_ctrl & L310_AUX_CTRL_FULL_LINE_ZERO)
> +		set_auxcr(get_auxcr() | BIT(3) | BIT(2) | BIT(1));
> +}
> +
>  static const struct l2c_init_data l2c310_init_fns __initconst = {
>  	.type = "L2C-310",
>  	.way_size_0 = SZ_8K,
> @@ -807,6 +824,7 @@ static const struct l2c_init_data l2c310_init_fns __initconst = {
>  	.enable = l2c310_enable,
>  	.fixup = l2c310_fixup,
>  	.save = l2c310_save,
> +	.configure = l2c310_configure,
>  	.outer_cache = {
>  		.inv_range = l2c210_inv_range,
>  		.clean_range = l2c210_clean_range,
> @@ -818,7 +836,7 @@ static const struct l2c_init_data l2c310_init_fns __initconst = {
>  	},
>  };
>  
> -static void __init __l2c_init(const struct l2c_init_data *data,
> +static int __init __l2c_init(const struct l2c_init_data *data,
>  	u32 aux_val, u32 aux_mask, u32 cache_id)
>  {
>  	struct outer_cache_fns fns;
> @@ -826,6 +844,14 @@ static void __init __l2c_init(const struct l2c_init_data *data,
>  	u32 aux, old_aux;
>  
>  	/*
> +	 * Save the pointer globally so that callbacks which do not receive
> +	 * context from callers can access the structure.
> +	 */
> +	l2x0_data = kmemdup(data, sizeof(*data), GFP_KERNEL);
> +	if (!l2x0_data)
> +		return -ENOMEM;
> +
> +	/*
>  	 * Sanity check the aux values.  aux_mask is the bits we preserve
>  	 * from reading the hardware register, and aux_val is the bits we
>  	 * set.
> @@ -910,6 +936,8 @@ static void __init __l2c_init(const struct l2c_init_data *data,
>  		data->type, ways, l2x0_size >> 10);
>  	pr_info("%s: CACHE_ID 0x%08x, AUX_CTRL 0x%08x\n",
>  		data->type, cache_id, aux);
> +
> +	return 0;
>  }
>  
>  void __init l2x0_init(void __iomem *base, u32 aux_val, u32 aux_mask)
> @@ -936,6 +964,10 @@ void __init l2x0_init(void __iomem *base, u32 aux_val, u32 aux_mask)
>  		break;
>  	}
>  
> +	/* Read back current (default) hardware configuration */
> +	if (data->save)
> +		data->save(l2x0_base);
> +
>  	__l2c_init(data, aux_val, aux_mask, cache_id);
>  }
>  
> @@ -1102,7 +1134,7 @@ static const struct l2c_init_data of_l2c210_data __initconst = {
>  		.flush_all   = l2c210_flush_all,
>  		.disable     = l2c_disable,
>  		.sync        = l2c210_sync,
> -		.resume      = l2c210_resume,
> +		.resume      = l2c_resume,
>  	},
>  };
>  
> @@ -1120,7 +1152,7 @@ static const struct l2c_init_data of_l2c220_data __initconst = {
>  		.flush_all   = l2c220_flush_all,
>  		.disable     = l2c_disable,
>  		.sync        = l2c220_sync,
> -		.resume      = l2c210_resume,
> +		.resume      = l2c_resume,
>  	},
>  };
>  
> @@ -1135,28 +1167,26 @@ static void __init l2c310_of_parse(const struct device_node *np,
>  
>  	of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag));
>  	if (tag[0] && tag[1] && tag[2])
> -		writel_relaxed(
> +		l2x0_saved_regs.tag_latency =
>  			L310_LATENCY_CTRL_RD(tag[0] - 1) |
>  			L310_LATENCY_CTRL_WR(tag[1] - 1) |
> -			L310_LATENCY_CTRL_SETUP(tag[2] - 1),
> -			l2x0_base + L310_TAG_LATENCY_CTRL);
> +			L310_LATENCY_CTRL_SETUP(tag[2] - 1);
>  
>  	of_property_read_u32_array(np, "arm,data-latency",
>  				   data, ARRAY_SIZE(data));
>  	if (data[0] && data[1] && data[2])
> -		writel_relaxed(
> +		l2x0_saved_regs.data_latency =
>  			L310_LATENCY_CTRL_RD(data[0] - 1) |
>  			L310_LATENCY_CTRL_WR(data[1] - 1) |
> -			L310_LATENCY_CTRL_SETUP(data[2] - 1),
> -			l2x0_base + L310_DATA_LATENCY_CTRL);
> +			L310_LATENCY_CTRL_SETUP(data[2] - 1);
>  
>  	of_property_read_u32_array(np, "arm,filter-ranges",
>  				   filter, ARRAY_SIZE(filter));
>  	if (filter[1]) {
> -		writel_relaxed(ALIGN(filter[0] + filter[1], SZ_1M),
> -			       l2x0_base + L310_ADDR_FILTER_END);
> -		writel_relaxed((filter[0] & ~(SZ_1M - 1)) | L310_ADDR_FILTER_EN,
> -			       l2x0_base + L310_ADDR_FILTER_START);
> +		l2x0_saved_regs.filter_end =
> +					ALIGN(filter[0] + filter[1], SZ_1M);
> +		l2x0_saved_regs.filter_start = (filter[0] & ~(SZ_1M - 1))
> +					| L310_ADDR_FILTER_EN;
>  	}
>  
>  	ret = l2x0_cache_size_of_parse(np, aux_val, aux_mask, &assoc, SZ_512K);
> @@ -1188,6 +1218,7 @@ static const struct l2c_init_data of_l2c310_data __initconst = {
>  	.enable = l2c310_enable,
>  	.fixup = l2c310_fixup,
>  	.save  = l2c310_save,
> +	.configure = l2c310_configure,
>  	.outer_cache = {
>  		.inv_range   = l2c210_inv_range,
>  		.clean_range = l2c210_clean_range,
> @@ -1216,6 +1247,7 @@ static const struct l2c_init_data of_l2c310_coherent_data __initconst = {
>  	.enable = l2c310_enable,
>  	.fixup = l2c310_fixup,
>  	.save  = l2c310_save,
> +	.configure = l2c310_configure,
>  	.outer_cache = {
>  		.inv_range   = l2c210_inv_range,
>  		.clean_range = l2c210_clean_range,
> @@ -1330,16 +1362,6 @@ static void aurora_save(void __iomem *base)
>  	l2x0_saved_regs.aux_ctrl = readl_relaxed(base + L2X0_AUX_CTRL);
>  }
>  
> -static void aurora_resume(void)
> -{
> -	void __iomem *base = l2x0_base;
> -
> -	if (!(readl(base + L2X0_CTRL) & L2X0_CTRL_EN)) {
> -		writel_relaxed(l2x0_saved_regs.aux_ctrl, base + L2X0_AUX_CTRL);
> -		writel_relaxed(l2x0_saved_regs.ctrl, base + L2X0_CTRL);
> -	}
> -}
> -
>  /*
>   * For Aurora cache in no outer mode, enable via the CP15 coprocessor
>   * broadcasting of cache commands to L2.
> @@ -1401,7 +1423,7 @@ static const struct l2c_init_data of_aurora_with_outer_data __initconst = {
>  		.flush_all   = l2x0_flush_all,
>  		.disable     = l2x0_disable,
>  		.sync        = l2x0_cache_sync,
> -		.resume      = aurora_resume,
> +		.resume      = l2c_resume,
>  	},
>  };
>  
> @@ -1414,7 +1436,7 @@ static const struct l2c_init_data of_aurora_no_outer_data __initconst = {
>  	.fixup = aurora_fixup,
>  	.save  = aurora_save,
>  	.outer_cache = {
> -		.resume      = aurora_resume,
> +		.resume      = l2c_resume,
>  	},
>  };
>  
> @@ -1562,6 +1584,7 @@ static const struct l2c_init_data of_bcm_l2x0_data __initconst = {
>  	.of_parse = l2c310_of_parse,
>  	.enable = l2c310_enable,
>  	.save  = l2c310_save,
> +	.configure = l2c310_configure,
>  	.outer_cache = {
>  		.inv_range   = bcm_inv_range,
>  		.clean_range = bcm_clean_range,
> @@ -1583,18 +1606,12 @@ static void __init tauros3_save(void __iomem *base)
>  		readl_relaxed(base + L310_PREFETCH_CTRL);
>  }
>  
> -static void tauros3_resume(void)
> +static void tauros3_configure(void __iomem *base)
>  {
> -	void __iomem *base = l2x0_base;
> -
> -	if (!(readl_relaxed(base + L2X0_CTRL) & L2X0_CTRL_EN)) {
> -		writel_relaxed(l2x0_saved_regs.aux2_ctrl,
> -			       base + TAUROS3_AUX2_CTRL);
> -		writel_relaxed(l2x0_saved_regs.prefetch_ctrl,
> -			       base + L310_PREFETCH_CTRL);
> -
> -		l2c_enable(base, l2x0_saved_regs.aux_ctrl, 8);
> -	}
> +	writel_relaxed(l2x0_saved_regs.aux2_ctrl,
> +		       base + TAUROS3_AUX2_CTRL);
> +	writel_relaxed(l2x0_saved_regs.prefetch_ctrl,
> +		       base + L310_PREFETCH_CTRL);
>  }
>  
>  static const struct l2c_init_data of_tauros3_data __initconst = {
> @@ -1603,9 +1620,10 @@ static const struct l2c_init_data of_tauros3_data __initconst = {
>  	.num_lock = 8,
>  	.enable = l2c_enable,
>  	.save  = tauros3_save,
> +	.configure = tauros3_configure,
>  	/* Tauros3 broadcasts L1 cache operations to L2 */
>  	.outer_cache = {
> -		.resume      = tauros3_resume,
> +		.resume      = l2c_resume,
>  	},
>  };
>  
> @@ -1661,6 +1679,10 @@ int __init l2x0_of_init(u32 aux_val, u32 aux_mask)
>  	if (!of_property_read_bool(np, "cache-unified"))
>  		pr_err("L2C: device tree omits to specify unified cache\n");
>  
> +	/* Read back current (default) hardware configuration */
> +	if (data->save)
> +		data->save(l2x0_base);
> +
>  	/* L2 configuration can only be changed if the cache is disabled */
>  	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & L2X0_CTRL_EN))
>  		if (data->of_parse)
> @@ -1671,8 +1693,6 @@ int __init l2x0_of_init(u32 aux_val, u32 aux_mask)
>  	else
>  		cache_id = readl_relaxed(l2x0_base + L2X0_CACHE_ID);
>  
> -	__l2c_init(data, aux_val, aux_mask, cache_id);
> -
> -	return 0;
> +	return __l2c_init(data, aux_val, aux_mask, cache_id);
>  }
>  #endif
> -- 
> 1.9.2
> 

Based on two painful debugs.. It would really be nice to have this patch
split up into tinier pieces.. I know I have no issues with this patch
anymore, but for others who might discover similar behavior, debug is a
little easier with a split up series.

-- 
Regards,
Nishanth Menon
--
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/