[v2] ARC: Force disable IOC if we don't want to use it

Message ID 20171218152920.4696-1-abrodkin@synopsys.com
State New
Headers show
Series
  • [v2] ARC: Force disable IOC if we don't want to use it
Related show

Commit Message

Alexey Brodkin Dec. 18, 2017, 3:29 p.m.
If software that was executed before Linux kernel [like boot-ROM or
bootloader] enabled IOC but we'd like to not use it [mostly for
debugging of weird DMA issues] we essentially need to disable IOC.
So we do here.

Note we will only disable IOC if "ioc_enable" variable is force set to 0.
As of today that's only possible either before building right in
arch/arc/mm/cache.c or via debugger on target reght before execution of
the kernel starts.

We may make "ioc_enable" a boot-parameter later though.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
---

Chnages in v2:
 * Now disabling really works as in v1 we never entered
   disabling code if ioc_enable=0.
 * Do nothing if IOC was not enabled

 arch/arc/mm/cache.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

Comments

Vineet Gupta Jan. 19, 2018, 7:17 p.m. | #1
On 12/18/2017 07:29 AM, Alexey Brodkin wrote:
> If software that was executed before Linux kernel [like boot-ROM or
> bootloader] enabled IOC but we'd like to not use it [mostly for
> debugging of weird DMA issues] we essentially need to disable IOC.
> So we do here.
>
> Note we will only disable IOC if "ioc_enable" variable is force set to 0.
> As of today that's only possible either before building right in
> arch/arc/mm/cache.c or via debugger on target reght before execution of
> the kernel starts.
>
> We may make "ioc_enable" a boot-parameter later though.
>
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>
> Chnages in v2:
>   * Now disabling really works as in v1 we never entered
>     disabling code if ioc_enable=0.
>   * Do nothing if IOC was not enabled
>
>   arch/arc/mm/cache.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arc/mm/cache.c b/arch/arc/mm/cache.c
> index 69f77c113875..9cadf7b779ed 100644
> --- a/arch/arc/mm/cache.c
> +++ b/arch/arc/mm/cache.c
> @@ -1186,6 +1186,40 @@ noinline void __init arc_ioc_setup(void)
>   	__dc_enable();
>   }
>   
> +/*
> + * Disabling of IOC is quite a tricky action because
> + * nobody knows what happens if there're IOC-ahndled tarnsactions in flight
> + * when we're disabling IOC.
> + *
> + * And the problem is external DMA masters [that were initialized and set in a
> + * bootlaoder that was executed before we got here] might continue to send data
> + * to memory even at this point and we have no way to prevent that.

Isn't this a sufficient red flag to warrant NOT adding this to kernel !

I agree that the use case is bring up of a new RTL version etc with bootloader 
already in there - but in that case can we NOT bypass those and run the kernel 
bare metal ?

> + *
> + * That said it's much safer to not enable IOC at all anywhere before
> + * in boot-ROM, bootloader etc but if we do need to disable it in Linux kernel
> + * it should be done as early as possible and made by master core while all
> + * slaves aren't active.
> + *

Again this is like saying - this is Pandora's Box - don't open it - which is more 
of a invitation for others to actually open it !

> + */
> +noinline void __init arc_ioc_disable(void)
> +{
> +	/* Exit if IOC was never enabled */
> +	if (!read_aux_reg(ARC_REG_IO_COH_ENABLE))
> +		return;
> +
> +	/* Flush + invalidate + disable L1 dcache */
> +	__dc_disable();
> +
> +	/* Flush + invalidate SLC */
> +	if (read_aux_reg(ARC_REG_SLC_BCR))
> +		slc_entire_op(OP_FLUSH_N_INV);
> +
> +	write_aux_reg(ARC_REG_IO_COH_ENABLE, 0);
> +
> +	/* Re-enable L1 dcache */
> +	__dc_enable();
> +}
> +
>   /*
>    * Cache related boot time checks/setups only needed on master CPU:
>    *  - Geometry checks (kernel build and hardware agree: e.g. L1_CACHE_BYTES)
> @@ -1247,8 +1281,12 @@ void __init arc_cache_init_master(void)
>   	if (is_isa_arcv2() && l2_line_sz && !slc_enable)
>   		arc_slc_disable();
>   
> -	if (is_isa_arcv2() && ioc_enable)
> -		arc_ioc_setup();
> +	if (is_isa_arcv2()) {
> +		if (ioc_enable)
> +			arc_ioc_setup();
> +		else if (ioc_exists)
> +			arc_ioc_disable();
> +	}
>   
>   	if (is_isa_arcv2() && ioc_enable) {
>   		__dma_cache_wback_inv = __dma_cache_wback_inv_ioc;
Alexey Brodkin Jan. 19, 2018, 7:23 p.m. | #2
Hi Vineet,

On Fri, 2018-01-19 at 11:17 -0800, Vineet Gupta wrote:
> On 12/18/2017 07:29 AM, Alexey Brodkin wrote:
> > If software that was executed before Linux kernel [like boot-ROM or
> > bootloader] enabled IOC but we'd like to not use it [mostly for
> > debugging of weird DMA issues] we essentially need to disable IOC.
> > So we do here.
> > 
> > Note we will only disable IOC if "ioc_enable" variable is force set to 0.
> > As of today that's only possible either before building right in
> > arch/arc/mm/cache.c or via debugger on target reght before execution of
> > the kernel starts.
> > 
> > We may make "ioc_enable" a boot-parameter later though.
> > 
> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > ---
> > 
> > Chnages in v2:
> >   * Now disabling really works as in v1 we never entered
> >     disabling code if ioc_enable=0.
> >   * Do nothing if IOC was not enabled
> > 
> >   arch/arc/mm/cache.c | 42 ++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 40 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arc/mm/cache.c b/arch/arc/mm/cache.c
> > index 69f77c113875..9cadf7b779ed 100644
> > --- a/arch/arc/mm/cache.c
> > +++ b/arch/arc/mm/cache.c
> > @@ -1186,6 +1186,40 @@ noinline void __init arc_ioc_setup(void)
> >   	__dc_enable();
> >   }
> >   
> > +/*
> > + * Disabling of IOC is quite a tricky action because
> > + * nobody knows what happens if there're IOC-ahndled tarnsactions in flight
> > + * when we're disabling IOC.
> > + *
> > + * And the problem is external DMA masters [that were initialized and set in a
> > + * bootlaoder that was executed before we got here] might continue to send data
> > + * to memory even at this point and we have no way to prevent that.
> 
> Isn't this a sufficient red flag to warrant NOT adding this to kernel !
> 
> I agree that the use case is bring up of a new RTL version etc with bootloader 
> already in there - but in that case can we NOT bypass those and run the kernel 
> bare metal ?
> 
> > + *
> > + * That said it's much safer to not enable IOC at all anywhere before
> > + * in boot-ROM, bootloader etc but if we do need to disable it in Linux kernel
> > + * it should be done as early as possible and made by master core while all
> > + * slaves aren't active.
> > + *
> 
> Again this is like saying - this is Pandora's Box - don't open it - which is more 
> of a invitation for others to actually open it !

I agree with all that above.

In fact I already sent a pull-request to Tom Rini for a series among other things 
disabling IOC in U-Boot, see:
1) Pull-request: https://lists.denx.de/pipermail/u-boot/2018-January/317602.html
2) Actual patch: http://git.denx.de/?p=u-boot/u-boot-arc.git;a=commit;h=b0146f9e29ca2e82262416aca65395c322a618f9

So indeed this patch for Linux could be dropped.

-Alexey

Patch

diff --git a/arch/arc/mm/cache.c b/arch/arc/mm/cache.c
index 69f77c113875..9cadf7b779ed 100644
--- a/arch/arc/mm/cache.c
+++ b/arch/arc/mm/cache.c
@@ -1186,6 +1186,40 @@  noinline void __init arc_ioc_setup(void)
 	__dc_enable();
 }
 
+/*
+ * Disabling of IOC is quite a tricky action because
+ * nobody knows what happens if there're IOC-ahndled tarnsactions in flight
+ * when we're disabling IOC.
+ *
+ * And the problem is external DMA masters [that were initialized and set in a
+ * bootlaoder that was executed before we got here] might continue to send data
+ * to memory even at this point and we have no way to prevent that.
+ *
+ * That said it's much safer to not enable IOC at all anywhere before
+ * in boot-ROM, bootloader etc but if we do need to disable it in Linux kernel
+ * it should be done as early as possible and made by master core while all
+ * slaves aren't active.
+ *
+ */
+noinline void __init arc_ioc_disable(void)
+{
+	/* Exit if IOC was never enabled */
+	if (!read_aux_reg(ARC_REG_IO_COH_ENABLE))
+		return;
+
+	/* Flush + invalidate + disable L1 dcache */
+	__dc_disable();
+
+	/* Flush + invalidate SLC */
+	if (read_aux_reg(ARC_REG_SLC_BCR))
+		slc_entire_op(OP_FLUSH_N_INV);
+
+	write_aux_reg(ARC_REG_IO_COH_ENABLE, 0);
+
+	/* Re-enable L1 dcache */
+	__dc_enable();
+}
+
 /*
  * Cache related boot time checks/setups only needed on master CPU:
  *  - Geometry checks (kernel build and hardware agree: e.g. L1_CACHE_BYTES)
@@ -1247,8 +1281,12 @@  void __init arc_cache_init_master(void)
 	if (is_isa_arcv2() && l2_line_sz && !slc_enable)
 		arc_slc_disable();
 
-	if (is_isa_arcv2() && ioc_enable)
-		arc_ioc_setup();
+	if (is_isa_arcv2()) {
+		if (ioc_enable)
+			arc_ioc_setup();
+		else if (ioc_exists)
+			arc_ioc_disable();
+	}
 
 	if (is_isa_arcv2() && ioc_enable) {
 		__dma_cache_wback_inv = __dma_cache_wback_inv_ioc;