[PATCH V2] MIPS: bugfix of coherentio variable default setup

[PATCH V2] MIPS: bugfix of coherentio variable default setup

[Leonid Yegoshin]

Patch commit b6d92b4a6bdb880b39789c677b952c53a437028d

    MIPS: Add option to disable software I/O coherency.

    Some MIPS controllers have hardware I/O coherency. This patch
    detects those and turns off software coherency. A new kernel
    command line option also allows the user to manually turn
    software coherency on or off.

in fact enforces L2 cache flushes even on systems with IOCU.
The default value of coherentio is 0 and is not changed even with IOCU.
It is a serious performance problem because it destroys all IOCU performance
advantages.

It is fixed by setting coherentio to tri-state with default value as (-1) and
setup a final value during platform coherency setup.

Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
---
V2: Missed signature added
---
 arch/mips/include/asm/mach-generic/dma-coherence.h |    7 ++++++-
 arch/mips/mm/c-r4k.c                               |    2 +-
 arch/mips/mm/dma-default.c                         |    2 +-
 arch/mips/mti-malta/malta-setup.c                  |    8 ++++++--
 arch/mips/pci/pci-alchemy.c                        |    2 +-
 5 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/mach-generic/dma-coherence.h b/arch/mips/include/asm/mach-generic/dma-coherence.h
index 7629c35..b4563df 100644
--- a/arch/mips/include/asm/mach-generic/dma-coherence.h
+++ b/arch/mips/include/asm/mach-generic/dma-coherence.h
@@ -49,7 +49,12 @@ static inline int plat_dma_supported(struct device *dev, u64 mask)
 
 static inline int plat_device_is_coherent(struct device *dev)
 {
-	return coherentio;
+#ifdef CONFIG_DMA_COHERENT
+	return 1;
+#else
+	return (coherentio > 0);
+#endif
+
 }
 
 #ifdef CONFIG_SWIOTLB
diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
index fbcd867..ad6ff7b 100644
--- a/arch/mips/mm/c-r4k.c
+++ b/arch/mips/mm/c-r4k.c
@@ -1660,7 +1660,7 @@ void r4k_cache_init(void)
 	local_flush_icache_range	= local_r4k_flush_icache_range;
 
 #if defined(CONFIG_DMA_NONCOHERENT) || defined(CONFIG_DMA_MAYBE_COHERENT)
-	if (coherentio) {
+	if (coherentio > 0) {
 		_dma_cache_wback_inv	= (void *)cache_noop;
 		_dma_cache_wback	= (void *)cache_noop;
 		_dma_cache_inv		= (void *)cache_noop;
diff --git a/arch/mips/mm/dma-default.c b/arch/mips/mm/dma-default.c
index 44b6dff..42c819a 100644
--- a/arch/mips/mm/dma-default.c
+++ b/arch/mips/mm/dma-default.c
@@ -24,7 +24,7 @@
 #include <dma-coherence.h>
 
 #ifdef CONFIG_DMA_MAYBE_COHERENT
-int coherentio = 0;	/* User defined DMA coherency from command line. */
+int coherentio = -1;    /* User defined DMA coherency is not defined yet. */
 EXPORT_SYMBOL_GPL(coherentio);
 int hw_coherentio = 0;	/* Actual hardware supported DMA coherency setting. */
 
diff --git a/arch/mips/mti-malta/malta-setup.c b/arch/mips/mti-malta/malta-setup.c
index db7c9e5..48039fd 100644
--- a/arch/mips/mti-malta/malta-setup.c
+++ b/arch/mips/mti-malta/malta-setup.c
@@ -147,13 +147,17 @@ static void __init plat_setup_iocoherency(void)
 	if (plat_enable_iocoherency()) {
 		if (coherentio == 0)
 			pr_info("Hardware DMA cache coherency disabled\n");
-		else
+		else {
+			coherentio = 1;
 			pr_info("Hardware DMA cache coherency enabled\n");
+		}
 	} else {
 		if (coherentio == 1)
 			pr_info("Hardware DMA cache coherency unsupported, but enabled from command line!\n");
-		else
+		else {
+			coherentio = 0;
 			pr_info("Software DMA cache coherency enabled\n");
+		}
 	}
 #else
 	if (!plat_enable_iocoherency())
diff --git a/arch/mips/pci/pci-alchemy.c b/arch/mips/pci/pci-alchemy.c
index c19600a..0d0b6c1 100644
--- a/arch/mips/pci/pci-alchemy.c
+++ b/arch/mips/pci/pci-alchemy.c
@@ -429,7 +429,7 @@ static int alchemy_pci_probe(struct platform_device *pdev)
 
 	/* Au1500 revisions older than AD have borked coherent PCI */
 	if ((alchemy_get_cputype() == ALCHEMY_CPU_AU1500) &&
-	    (read_c0_prid() < 0x01030202) && !coherentio) {
+	    (read_c0_prid() < 0x01030202) && (coherentio <= 0)) {
 		val = __raw_readl(ctx->regs + PCI_REG_CONFIG);
 		val |= PCI_CONFIG_NC;
 		__raw_writel(val, ctx->regs + PCI_REG_CONFIG);

Re: [PATCH V2] MIPS: bugfix of coherentio variable default setup

[Markos Chandras]

Hi Leonid,

On 09/08/2014 08:10 PM, Leonid Yegoshin wrote:
> Patch commit b6d92b4a6bdb880b39789c677b952c53a437028d
> 
>     MIPS: Add option to disable software I/O coherency.
> 
>     Some MIPS controllers have hardware I/O coherency. This patch
>     detects those and turns off software coherency. A new kernel
>     command line option also allows the user to manually turn
>     software coherency on or off.
> 
> in fact enforces L2 cache flushes even on systems with IOCU.
> The default value of coherentio is 0 and is not changed even with IOCU.
> It is a serious performance problem because it destroys all IOCU performance
> advantages.
> 
> It is fixed by setting coherentio to tri-state with default value as (-1) and
> setup a final value during platform coherency setup.
> 
> Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> [...]
> diff --git a/arch/mips/mti-malta/malta-setup.c b/arch/mips/mti-malta/malta-setup.c
> index db7c9e5..48039fd 100644
> --- a/arch/mips/mti-malta/malta-setup.c
> +++ b/arch/mips/mti-malta/malta-setup.c
> @@ -147,13 +147,17 @@ static void __init plat_setup_iocoherency(void)
>  	if (plat_enable_iocoherency()) {
>  		if (coherentio == 0)
>  			pr_info("Hardware DMA cache coherency disabled\n");
> -		else
> +		else {
> +			coherentio = 1;
>  			pr_info("Hardware DMA cache coherency enabled\n");
> +		}
>  	} else {
>  		if (coherentio == 1)
>  			pr_info("Hardware DMA cache coherency unsupported, but enabled from command line!\n");
> -		else
> +		else {
> +			coherentio = 0;
>  			pr_info("Software DMA cache coherency enabled\n");
> +		}

This is not acceptable coding style for the kernel. See
Documentation/CodingStyle, Chapter 3. Since you are adding braces in the
"else" case, you should also add them in the "if" case as well.

-- 
markos

Re: [PATCH V2] MIPS: bugfix of coherentio variable default setup

[James Hogan]

On 08/09/14 20:10, Leonid Yegoshin wrote:
> diff --git a/arch/mips/include/asm/mach-generic/dma-coherence.h b/arch/mips/include/asm/mach-generic/dma-coherence.h
> index 7629c35..b4563df 100644
> --- a/arch/mips/include/asm/mach-generic/dma-coherence.h
> +++ b/arch/mips/include/asm/mach-generic/dma-coherence.h
> @@ -49,7 +49,12 @@ static inline int plat_dma_supported(struct device *dev, u64 mask)
>  
>  static inline int plat_device_is_coherent(struct device *dev)
>  {
> -	return coherentio;
> +#ifdef CONFIG_DMA_COHERENT
> +	return 1;

if DMA_COHERENT=y (which seems to imply DMA_MAYBE_COHERENT=n),
coherentio is already defined as 1 in
arch/mips/include/asm/dma-coherent.h, so I don't think you need this case.

> +#else
> +	return (coherentio > 0);
> +#endif
> +

No need for a new blank line here

>  }

...

> diff --git a/arch/mips/mm/dma-default.c b/arch/mips/mm/dma-default.c
> index 44b6dff..42c819a 100644
> --- a/arch/mips/mm/dma-default.c
> +++ b/arch/mips/mm/dma-default.c
> @@ -24,7 +24,7 @@
>  #include <dma-coherence.h>
>  
>  #ifdef CONFIG_DMA_MAYBE_COHERENT
> -int coherentio = 0;	/* User defined DMA coherency from command line. */
> +int coherentio = -1;    /* User defined DMA coherency is not defined yet. */

Please avoid unnecessarily turned tabs into spaces. It makes files
inconsistent.

Other than that and Markos' style comment it looks reasonable to me.

Cheers
James

Re: [PATCH V2] MIPS: bugfix of coherentio variable default setup

[Sergei Shtylyov]

Hello.

On 9/8/2014 10:10 PM, Leonid Yegoshin wrote:

> Patch commit b6d92b4a6bdb880b39789c677b952c53a437028d

    Just commit.

>      MIPS: Add option to disable software I/O coherency.

    It's enough to just cite the summary in parens after SHA1 ID.

>      Some MIPS controllers have hardware I/O coherency. This patch
>      detects those and turns off software coherency. A new kernel
>      command line option also allows the user to manually turn
>      software coherency on or off.

> in fact enforces L2 cache flushes even on systems with IOCU.
> The default value of coherentio is 0 and is not changed even with IOCU.
> It is a serious performance problem because it destroys all IOCU performance
> advantages.

> It is fixed by setting coherentio to tri-state with default value as (-1) and
> setup a final value during platform coherency setup.

> Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> ---
> V2: Missed signature added
> ---
>   arch/mips/include/asm/mach-generic/dma-coherence.h |    7 ++++++-
>   arch/mips/mm/c-r4k.c                               |    2 +-
>   arch/mips/mm/dma-default.c                         |    2 +-
>   arch/mips/mti-malta/malta-setup.c                  |    8 ++++++--
>   arch/mips/pci/pci-alchemy.c                        |    2 +-
>   5 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/arch/mips/include/asm/mach-generic/dma-coherence.h b/arch/mips/include/asm/mach-generic/dma-coherence.h
> index 7629c35..b4563df 100644
> --- a/arch/mips/include/asm/mach-generic/dma-coherence.h
> +++ b/arch/mips/include/asm/mach-generic/dma-coherence.h
> @@ -49,7 +49,12 @@ static inline int plat_dma_supported(struct device *dev, u64 mask)
>
>   static inline int plat_device_is_coherent(struct device *dev)
>   {
> -	return coherentio;
> +#ifdef CONFIG_DMA_COHERENT
> +	return 1;
> +#else
> +	return (coherentio > 0);

    *return* never needs parens around the value.

> +#endif
> +
>   }
>
>   #ifdef CONFIG_SWIOTLB
> diff --git a/arch/mips/mti-malta/malta-setup.c b/arch/mips/mti-malta/malta-setup.c
> index db7c9e5..48039fd 100644
> --- a/arch/mips/mti-malta/malta-setup.c
> +++ b/arch/mips/mti-malta/malta-setup.c
> @@ -147,13 +147,17 @@ static void __init plat_setup_iocoherency(void)
>   	if (plat_enable_iocoherency()) {
>   		if (coherentio == 0)
>   			pr_info("Hardware DMA cache coherency disabled\n");
> -		else
> +		else {
> +			coherentio = 1;

    You now need the parens around another arm of the *if* statement too. Such 
is the kernel style. :-)

>   			pr_info("Hardware DMA cache coherency enabled\n");
> +		}
>   	} else {
>   		if (coherentio == 1)
>   			pr_info("Hardware DMA cache coherency unsupported, but enabled from command line!\n");
> -		else
> +		else {
> +			coherentio = 0;

    Likewise.

>   			pr_info("Software DMA cache coherency enabled\n");
> +		}
>   	}
>   #else
>   	if (!plat_enable_iocoherency())

WBR, Sergei