[PATCH 0/3] bus: mvebu-mbus: important fixes and improvements

[PATCH 0/3] bus: mvebu-mbus: important fixes and improvements

[Thomas Petazzoni]

Dear mvebu maintainers,

This set of patches contain two very important fixes to the mvebu-mbus
driver fixing significant regressions introduced during the 4.0 cycle,
and one patch re-implementing in a different a feature that caused one
of the two regressions.

 * The first patch, provided by Nicolas Schichan, fixed a regression
    introduced in 4.0 by commit a0b5cd4ac2d6 ("bus: mvebu-mbus: use
    automatic I/O synchronization barriers"). This commit didn't take
    into account non-HW I/O coherent platforms such as Kirkwood or
    Orion, for which the new special bit being set causes problems. It
    makes some of the MBus I/O windows unusable on these SoCs.

    Therefore, this patch is marked for stable and should be merged
    for 4.1-rc.

 * The second patch is a simple revert of commit "bus: mvebu-mbus:
    make sure SDRAM CS for DMA don't overlap the MBus bridge window"
    introduced in 4.0. This commit was added in preparation for the
    merge of the new DMA-capable CESA crypto driver submitted by Boris
    Brezillon. However, it breaks DMA for all the other units if your
    platform has 4 GB of RAM or more. Of course, by reverting it, the
    functionality needed for the crypto driver is no longer present,
    but this is OK since the crypto driver isn't merged yet, and will
    not be merged for 4.1.

    Therefore, this patch is marked for stable and should be merged
    for 4.1-rc.

  * The last re-implements the mechanism originally implemented by
    "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the
    MBus bridge window", but in a completely different way. This patch
    will be needed for the CESA driver.

    This patch is not a fix, and should therefore be merged in 4.2.

Thanks,

Thomas

Nicolas Schichan (1):
  bus: mvebu-mbus: do not set WIN_CTRL_SYNCBARRIER on non io-coherent
    platforms.

Thomas Petazzoni (2):
  Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the
    MBus bridge window"
  bus: mvebu-mbus: add mv_mbus_dram_info_nooverlap()

 drivers/bus/mvebu-mbus.c | 120 ++++++++++++++++++++++++++++++++---------------
 include/linux/mbus.h     |   1 +
 2 files changed, 84 insertions(+), 37 deletions(-)

-- 
2.1.0

[PATCH 1/3] bus: mvebu-mbus: do not set WIN_CTRL_SYNCBARRIER on non io-coherent platforms.

[Thomas Petazzoni]

From: Nicolas Schichan <nschichan@freebox.fr>

Commit a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O
synchronization barriers") enabled the usage of automatic I/O
synchronization barriers by enabling bit WIN_CTRL_SYNCBARRIER in the
control registers of MBus windows, but on non io-coherent platforms
(orion5x, kirkwood and dove) the WIN_CTRL_SYNCBARRIER bit in
the window control register is either reserved (all windows except 6
and 7) or enables read-only protection (windows 6 and 7).

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: <stable@vger.kernel.org> # v4.0+
Fixes: a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O synchronization barriers")
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/bus/mvebu-mbus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index fb9ec62..7fa4510 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -70,6 +70,7 @@
  */
 #define WIN_CTRL_OFF		0x0000
 #define   WIN_CTRL_ENABLE       BIT(0)
+/* Only on HW I/O coherency capable platforms */
 #define   WIN_CTRL_SYNCBARRIER  BIT(1)
 #define   WIN_CTRL_TGT_MASK     0xf0
 #define   WIN_CTRL_TGT_SHIFT    4
@@ -323,8 +324,9 @@ static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus,
 	ctrl = ((size - 1) & WIN_CTRL_SIZE_MASK) |
 		(attr << WIN_CTRL_ATTR_SHIFT)    |
 		(target << WIN_CTRL_TGT_SHIFT)   |
-		WIN_CTRL_SYNCBARRIER             |
 		WIN_CTRL_ENABLE;
+	if (mbus->hw_io_coherency)
+		ctrl |= WIN_CTRL_SYNCBARRIER;
 
 	writel(base & WIN_BASE_LOW, addr + WIN_BASE_OFF);
 	writel(ctrl, addr + WIN_CTRL_OFF);
-- 
2.1.0

[PATCH 1/3] bus: mvebu-mbus: do not set WIN_CTRL_SYNCBARRIER on non io-coherent platforms.

[Thomas Petazzoni]

From: Nicolas Schichan <nschichan@freebox.fr>

Commit a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O
synchronization barriers") enabled the usage of automatic I/O
synchronization barriers by enabling bit WIN_CTRL_SYNCBARRIER in the
control registers of MBus windows, but on non io-coherent platforms
(orion5x, kirkwood and dove) the WIN_CTRL_SYNCBARRIER bit in
the window control register is either reserved (all windows except 6
and 7) or enables read-only protection (windows 6 and 7).

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: <stable@vger.kernel.org> # v4.0+
Fixes: a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O synchronization barriers")
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/bus/mvebu-mbus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index fb9ec62..7fa4510 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -70,6 +70,7 @@
  */
 #define WIN_CTRL_OFF		0x0000
 #define   WIN_CTRL_ENABLE       BIT(0)
+/* Only on HW I/O coherency capable platforms */
 #define   WIN_CTRL_SYNCBARRIER  BIT(1)
 #define   WIN_CTRL_TGT_MASK     0xf0
 #define   WIN_CTRL_TGT_SHIFT    4
@@ -323,8 +324,9 @@ static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus,
 	ctrl = ((size - 1) & WIN_CTRL_SIZE_MASK) |
 		(attr << WIN_CTRL_ATTR_SHIFT)    |
 		(target << WIN_CTRL_TGT_SHIFT)   |
-		WIN_CTRL_SYNCBARRIER             |
 		WIN_CTRL_ENABLE;
+	if (mbus->hw_io_coherency)
+		ctrl |= WIN_CTRL_SYNCBARRIER;
 
 	writel(base & WIN_BASE_LOW, addr + WIN_BASE_OFF);
 	writel(ctrl, addr + WIN_CTRL_OFF);
-- 
2.1.0

Re: [PATCH 1/3] bus: mvebu-mbus: do not set WIN_CTRL_SYNCBARRIER on non io-coherent platforms.

[Gregory CLEMENT]

Hi Thomas,

On 28/05/2015 10:40, Thomas Petazzoni wrote:
> From: Nicolas Schichan <nschichan@freebox.fr>
> 
> Commit a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O
> synchronization barriers") enabled the usage of automatic I/O
> synchronization barriers by enabling bit WIN_CTRL_SYNCBARRIER in the
> control registers of MBus windows, but on non io-coherent platforms
> (orion5x, kirkwood and dove) the WIN_CTRL_SYNCBARRIER bit in
> the window control register is either reserved (all windows except 6
> and 7) or enables read-only protection (windows 6 and 7).
> 
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v4.0+
> Fixes: a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O synchronization barriers")
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

applied on mvebu/fixes

Thanks,

Gregory

> ---
>  drivers/bus/mvebu-mbus.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index fb9ec62..7fa4510 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -70,6 +70,7 @@
>   */
>  #define WIN_CTRL_OFF		0x0000
>  #define   WIN_CTRL_ENABLE       BIT(0)
> +/* Only on HW I/O coherency capable platforms */
>  #define   WIN_CTRL_SYNCBARRIER  BIT(1)
>  #define   WIN_CTRL_TGT_MASK     0xf0
>  #define   WIN_CTRL_TGT_SHIFT    4
> @@ -323,8 +324,9 @@ static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus,
>  	ctrl = ((size - 1) & WIN_CTRL_SIZE_MASK) |
>  		(attr << WIN_CTRL_ATTR_SHIFT)    |
>  		(target << WIN_CTRL_TGT_SHIFT)   |
> -		WIN_CTRL_SYNCBARRIER             |
>  		WIN_CTRL_ENABLE;
> +	if (mbus->hw_io_coherency)
> +		ctrl |= WIN_CTRL_SYNCBARRIER;
>  
>  	writel(base & WIN_BASE_LOW, addr + WIN_BASE_OFF);
>  	writel(ctrl, addr + WIN_CTRL_OFF);
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/3] bus: mvebu-mbus: do not set WIN_CTRL_SYNCBARRIER on non io-coherent platforms.

[Gregory CLEMENT]

Hi Thomas,

On 28/05/2015 10:40, Thomas Petazzoni wrote:
> From: Nicolas Schichan <nschichan@freebox.fr>
> 
> Commit a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O
> synchronization barriers") enabled the usage of automatic I/O
> synchronization barriers by enabling bit WIN_CTRL_SYNCBARRIER in the
> control registers of MBus windows, but on non io-coherent platforms
> (orion5x, kirkwood and dove) the WIN_CTRL_SYNCBARRIER bit in
> the window control register is either reserved (all windows except 6
> and 7) or enables read-only protection (windows 6 and 7).
> 
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v4.0+
> Fixes: a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O synchronization barriers")
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

applied on mvebu/fixes

Thanks,

Gregory

> ---
>  drivers/bus/mvebu-mbus.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index fb9ec62..7fa4510 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -70,6 +70,7 @@
>   */
>  #define WIN_CTRL_OFF		0x0000
>  #define   WIN_CTRL_ENABLE       BIT(0)
> +/* Only on HW I/O coherency capable platforms */
>  #define   WIN_CTRL_SYNCBARRIER  BIT(1)
>  #define   WIN_CTRL_TGT_MASK     0xf0
>  #define   WIN_CTRL_TGT_SHIFT    4
> @@ -323,8 +324,9 @@ static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus,
>  	ctrl = ((size - 1) & WIN_CTRL_SIZE_MASK) |
>  		(attr << WIN_CTRL_ATTR_SHIFT)    |
>  		(target << WIN_CTRL_TGT_SHIFT)   |
> -		WIN_CTRL_SYNCBARRIER             |
>  		WIN_CTRL_ENABLE;
> +	if (mbus->hw_io_coherency)
> +		ctrl |= WIN_CTRL_SYNCBARRIER;
>  
>  	writel(base & WIN_BASE_LOW, addr + WIN_BASE_OFF);
>  	writel(ctrl, addr + WIN_CTRL_OFF);
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/3] bus: mvebu-mbus: do not set WIN_CTRL_SYNCBARRIER on non io-coherent platforms.

[Ian Campbell]

On Thu, 2015-05-28 at 10:40 +0200, Thomas Petazzoni wrote:
> From: Nicolas Schichan <nschichan@freebox.fr>
> 
> Commit a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O
> synchronization barriers") enabled the usage of automatic I/O
> synchronization barriers by enabling bit WIN_CTRL_SYNCBARRIER in the
> control registers of MBus windows, but on non io-coherent platforms
> (orion5x, kirkwood and dove) the WIN_CTRL_SYNCBARRIER bit in
> the window control register is either reserved (all windows except 6
> and 7) or enables read-only protection (windows 6 and 7).
> 
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v4.0+
> Fixes: a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O synchronization barriers")
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Tested-by: Ian Campbell <ijc@debian.org> (On QNAP TS-419P, it fixed SATA
reset issue)

[PATCH 1/3] bus: mvebu-mbus: do not set WIN_CTRL_SYNCBARRIER on non io-coherent platforms.

[Ian Campbell]

On Thu, 2015-05-28 at 10:40 +0200, Thomas Petazzoni wrote:
> From: Nicolas Schichan <nschichan@freebox.fr>
> 
> Commit a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O
> synchronization barriers") enabled the usage of automatic I/O
> synchronization barriers by enabling bit WIN_CTRL_SYNCBARRIER in the
> control registers of MBus windows, but on non io-coherent platforms
> (orion5x, kirkwood and dove) the WIN_CTRL_SYNCBARRIER bit in
> the window control register is either reserved (all windows except 6
> and 7) or enables read-only protection (windows 6 and 7).
> 
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v4.0+
> Fixes: a0b5cd4ac2d6 ("bus: mvebu-mbus: use automatic I/O synchronization barriers")
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Tested-by: Ian Campbell <ijc@debian.org> (On QNAP TS-419P, it fixed SATA
reset issue)

[PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Thomas Petazzoni]

This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS
for DMA don't overlap the MBus bridge window"), because it breaks DMA
on platforms having more than 2 GB of RAM.

This commit changed the information reported to DMA masters device
drivers through the mv_mbus_dram_info() function so that the returned
DRAM ranges do not overlap with I/O windows.

This was necessary as a preparation to support the new CESA Crypto
Engine driver, which will use DMA for cryptographic operations. But
since it does DMA with the SRAM which is mapped as an I/O window,
having DRAM ranges overlapping with I/O windows was problematic.

To solve this, the above mentioned commit changed the mvebu-mbus to
adjust the DRAM ranges so that they don't overlap with the I/O
windows. However, by doing this, we re-adjust the DRAM ranges in a way
that makes them have a size that is no longer a power of two. While
this is perfectly fine for the Crypto Engine, which supports DRAM
ranges with a granularity of 64 KB, it breaks basically all other DMA
masters, which expect power of two sizes for the DRAM ranges.

Due to this, if the installed system memory is 4 GB, in two
chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB
to a little bit less than 2 GB to not overlap with the I/O windows, in
a way that results in a DRAM range that doesn't have a power of two
size. This means that whenever you do a DMA transfer with an address
located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any
serious DMA activity like simply running:

  for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done

in an ext3 partition mounted over a SATA drive will freeze the system.

Since the new CESA crypto driver that uses DMA has not been merged
yet, the easiest fix is to simply revert this commit. A follow-up
commit will introduce a different solution for the CESA crypto driver.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
Cc: <stable@vger.kernel.org> # v4.0+
---
 drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
 1 file changed, 16 insertions(+), 89 deletions(-)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 7fa4510..6f047dc 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -58,7 +58,6 @@
 #include <linux/debugfs.h>
 #include <linux/log2.h>
 #include <linux/syscore_ops.h>
-#include <linux/memblock.h>
 
 /*
  * DDR target is the same on all platforms.
@@ -103,9 +102,7 @@
 
 /* Relative to mbusbridge_base */
 #define MBUS_BRIDGE_CTRL_OFF	0x0
-#define  MBUS_BRIDGE_SIZE_MASK  0xffff0000
 #define MBUS_BRIDGE_BASE_OFF	0x4
-#define  MBUS_BRIDGE_BASE_MASK  0xffff0000
 
 /* Maximum number of windows, for all known platforms */
 #define MBUS_WINS_MAX           20
@@ -579,106 +576,36 @@ static unsigned int armada_xp_mbus_win_remap_offset(int win)
 		return MVEBU_MBUS_NO_REMAP;
 }
 
-/*
- * Use the memblock information to find the MBus bridge hole in the
- * physical address space.
- */
-static void __init
-mvebu_mbus_find_bridge_hole(uint64_t *start, uint64_t *end)
-{
-	struct memblock_region *r;
-	uint64_t s = 0;
-
-	for_each_memblock(memory, r) {
-		/*
-		 * This part of the memory is above 4 GB, so we don't
-		 * care for the MBus bridge hole.
-		 */
-		if (r->base >= 0x100000000)
-			continue;
-
-		/*
-		 * The MBus bridge hole is at the end of the RAM under
-		 * the 4 GB limit.
-		 */
-		if (r->base + r->size > s)
-			s = r->base + r->size;
-	}
-
-	*start = s;
-	*end = 0x100000000;
-}
-
 static void __init
 mvebu_mbus_default_setup_cpu_target(struct mvebu_mbus_state *mbus)
 {
 	int i;
 	int cs;
-	uint64_t mbus_bridge_base, mbus_bridge_end;
 
 	mvebu_mbus_dram_info.mbus_dram_target_id = TARGET_DDR;
 
-	mvebu_mbus_find_bridge_hole(&mbus_bridge_base, &mbus_bridge_end);
-
 	for (i = 0, cs = 0; i < 4; i++) {
-		u64 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
-		u64 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
-		u64 end;
-		struct mbus_dram_window *w;
-
-		/* Ignore entries that are not enabled */
-		if (!(size & DDR_SIZE_ENABLED))
-			continue;
-
-		/*
-		 * Ignore entries whose base address is above 2^32,
-		 * since devices cannot DMA to such high addresses
-		 */
-		if (base & DDR_BASE_CS_HIGH_MASK)
-			continue;
-
-		base = base & DDR_BASE_CS_LOW_MASK;
-		size = (size | ~DDR_SIZE_MASK) + 1;
-		end = base + size;
-
-		/*
-		 * Adjust base/size of the current CS to make sure it
-		 * doesn't overlap with the MBus bridge hole. This is
-		 * particularly important for devices that do DMA from
-		 * DRAM to a SRAM mapped in a MBus window, such as the
-		 * CESA cryptographic engine.
-		 */
+		u32 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
+		u32 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
 
 		/*
-		 * The CS is fully enclosed inside the MBus bridge
-		 * area, so ignore it.
+		 * We only take care of entries for which the chip
+		 * select is enabled, and that don't have high base
+		 * address bits set (devices can only access the first
+		 * 32 bits of the memory).
 		 */
-		if (base >= mbus_bridge_base && end <= mbus_bridge_end)
-			continue;
+		if ((size & DDR_SIZE_ENABLED) &&
+		    !(base & DDR_BASE_CS_HIGH_MASK)) {
+			struct mbus_dram_window *w;
 
-		/*
-		 * Beginning of CS overlaps with end of MBus, raise CS
-		 * base address, and shrink its size.
-		 */
-		if (base >= mbus_bridge_base && end > mbus_bridge_end) {
-			size -= mbus_bridge_end - base;
-			base = mbus_bridge_end;
+			w = &mvebu_mbus_dram_info.cs[cs++];
+			w->cs_index = i;
+			w->mbus_attr = 0xf & ~(1 << i);
+			if (mbus->hw_io_coherency)
+				w->mbus_attr |= ATTR_HW_COHERENCY;
+			w->base = base & DDR_BASE_CS_LOW_MASK;
+			w->size = (size | ~DDR_SIZE_MASK) + 1;
 		}
-
-		/*
-		 * End of CS overlaps with beginning of MBus, shrink
-		 * CS size.
-		 */
-		if (base < mbus_bridge_base && end > mbus_bridge_base)
-			size -= end - mbus_bridge_base;
-
-		w = &mvebu_mbus_dram_info.cs[cs++];
-		w->cs_index = i;
-		w->mbus_attr = 0xf & ~(1 << i);
-		if (mbus->hw_io_coherency)
-			w->mbus_attr |= ATTR_HW_COHERENCY;
-		w->base = base;
-		w->size = size;
 	}
 	mvebu_mbus_dram_info.num_cs = cs;
 }
-- 
2.1.0

[PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Thomas Petazzoni]

This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS
for DMA don't overlap the MBus bridge window"), because it breaks DMA
on platforms having more than 2 GB of RAM.

This commit changed the information reported to DMA masters device
drivers through the mv_mbus_dram_info() function so that the returned
DRAM ranges do not overlap with I/O windows.

This was necessary as a preparation to support the new CESA Crypto
Engine driver, which will use DMA for cryptographic operations. But
since it does DMA with the SRAM which is mapped as an I/O window,
having DRAM ranges overlapping with I/O windows was problematic.

To solve this, the above mentioned commit changed the mvebu-mbus to
adjust the DRAM ranges so that they don't overlap with the I/O
windows. However, by doing this, we re-adjust the DRAM ranges in a way
that makes them have a size that is no longer a power of two. While
this is perfectly fine for the Crypto Engine, which supports DRAM
ranges with a granularity of 64 KB, it breaks basically all other DMA
masters, which expect power of two sizes for the DRAM ranges.

Due to this, if the installed system memory is 4 GB, in two
chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB
to a little bit less than 2 GB to not overlap with the I/O windows, in
a way that results in a DRAM range that doesn't have a power of two
size. This means that whenever you do a DMA transfer with an address
located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any
serious DMA activity like simply running:

  for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done

in an ext3 partition mounted over a SATA drive will freeze the system.

Since the new CESA crypto driver that uses DMA has not been merged
yet, the easiest fix is to simply revert this commit. A follow-up
commit will introduce a different solution for the CESA crypto driver.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
Cc: <stable@vger.kernel.org> # v4.0+
---
 drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
 1 file changed, 16 insertions(+), 89 deletions(-)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 7fa4510..6f047dc 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -58,7 +58,6 @@
 #include <linux/debugfs.h>
 #include <linux/log2.h>
 #include <linux/syscore_ops.h>
-#include <linux/memblock.h>
 
 /*
  * DDR target is the same on all platforms.
@@ -103,9 +102,7 @@
 
 /* Relative to mbusbridge_base */
 #define MBUS_BRIDGE_CTRL_OFF	0x0
-#define  MBUS_BRIDGE_SIZE_MASK  0xffff0000
 #define MBUS_BRIDGE_BASE_OFF	0x4
-#define  MBUS_BRIDGE_BASE_MASK  0xffff0000
 
 /* Maximum number of windows, for all known platforms */
 #define MBUS_WINS_MAX           20
@@ -579,106 +576,36 @@ static unsigned int armada_xp_mbus_win_remap_offset(int win)
 		return MVEBU_MBUS_NO_REMAP;
 }
 
-/*
- * Use the memblock information to find the MBus bridge hole in the
- * physical address space.
- */
-static void __init
-mvebu_mbus_find_bridge_hole(uint64_t *start, uint64_t *end)
-{
-	struct memblock_region *r;
-	uint64_t s = 0;
-
-	for_each_memblock(memory, r) {
-		/*
-		 * This part of the memory is above 4 GB, so we don't
-		 * care for the MBus bridge hole.
-		 */
-		if (r->base >= 0x100000000)
-			continue;
-
-		/*
-		 * The MBus bridge hole is at the end of the RAM under
-		 * the 4 GB limit.
-		 */
-		if (r->base + r->size > s)
-			s = r->base + r->size;
-	}
-
-	*start = s;
-	*end = 0x100000000;
-}
-
 static void __init
 mvebu_mbus_default_setup_cpu_target(struct mvebu_mbus_state *mbus)
 {
 	int i;
 	int cs;
-	uint64_t mbus_bridge_base, mbus_bridge_end;
 
 	mvebu_mbus_dram_info.mbus_dram_target_id = TARGET_DDR;
 
-	mvebu_mbus_find_bridge_hole(&mbus_bridge_base, &mbus_bridge_end);
-
 	for (i = 0, cs = 0; i < 4; i++) {
-		u64 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
-		u64 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
-		u64 end;
-		struct mbus_dram_window *w;
-
-		/* Ignore entries that are not enabled */
-		if (!(size & DDR_SIZE_ENABLED))
-			continue;
-
-		/*
-		 * Ignore entries whose base address is above 2^32,
-		 * since devices cannot DMA to such high addresses
-		 */
-		if (base & DDR_BASE_CS_HIGH_MASK)
-			continue;
-
-		base = base & DDR_BASE_CS_LOW_MASK;
-		size = (size | ~DDR_SIZE_MASK) + 1;
-		end = base + size;
-
-		/*
-		 * Adjust base/size of the current CS to make sure it
-		 * doesn't overlap with the MBus bridge hole. This is
-		 * particularly important for devices that do DMA from
-		 * DRAM to a SRAM mapped in a MBus window, such as the
-		 * CESA cryptographic engine.
-		 */
+		u32 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
+		u32 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
 
 		/*
-		 * The CS is fully enclosed inside the MBus bridge
-		 * area, so ignore it.
+		 * We only take care of entries for which the chip
+		 * select is enabled, and that don't have high base
+		 * address bits set (devices can only access the first
+		 * 32 bits of the memory).
 		 */
-		if (base >= mbus_bridge_base && end <= mbus_bridge_end)
-			continue;
+		if ((size & DDR_SIZE_ENABLED) &&
+		    !(base & DDR_BASE_CS_HIGH_MASK)) {
+			struct mbus_dram_window *w;
 
-		/*
-		 * Beginning of CS overlaps with end of MBus, raise CS
-		 * base address, and shrink its size.
-		 */
-		if (base >= mbus_bridge_base && end > mbus_bridge_end) {
-			size -= mbus_bridge_end - base;
-			base = mbus_bridge_end;
+			w = &mvebu_mbus_dram_info.cs[cs++];
+			w->cs_index = i;
+			w->mbus_attr = 0xf & ~(1 << i);
+			if (mbus->hw_io_coherency)
+				w->mbus_attr |= ATTR_HW_COHERENCY;
+			w->base = base & DDR_BASE_CS_LOW_MASK;
+			w->size = (size | ~DDR_SIZE_MASK) + 1;
 		}
-
-		/*
-		 * End of CS overlaps with beginning of MBus, shrink
-		 * CS size.
-		 */
-		if (base < mbus_bridge_base && end > mbus_bridge_base)
-			size -= end - mbus_bridge_base;
-
-		w = &mvebu_mbus_dram_info.cs[cs++];
-		w->cs_index = i;
-		w->mbus_attr = 0xf & ~(1 << i);
-		if (mbus->hw_io_coherency)
-			w->mbus_attr |= ATTR_HW_COHERENCY;
-		w->base = base;
-		w->size = size;
 	}
 	mvebu_mbus_dram_info.num_cs = cs;
 }
-- 
2.1.0

Re: [PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Arnd Bergmann]

On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote:
> Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> Cc: <stable@vger.kernel.org> # v4.0+
> ---
>  drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
>  1 file changed, 16 insertions(+), 89 deletions(-)

Hmm, the stable kernel rules say that a patch cannot exceed 100
lines with context, so this one is technically too large.

Maybe Greg has a suggestion about what to do here. Is it possible
to make an exception for a revert? In theory you could make a
smaller version of the patch that adds an #if 0 instead of removing
some of the code that was added, in order to get below the limit,
but that seems counterproductive for minimizing the possible risk.

	Arnd

[PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Arnd Bergmann]

On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote:
> Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> Cc: <stable@vger.kernel.org> # v4.0+
> ---
>  drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
>  1 file changed, 16 insertions(+), 89 deletions(-)

Hmm, the stable kernel rules say that a patch cannot exceed 100
lines with context, so this one is technically too large.

Maybe Greg has a suggestion about what to do here. Is it possible
to make an exception for a revert? In theory you could make a
smaller version of the patch that adds an #if 0 instead of removing
some of the code that was added, in order to get below the limit,
but that seems counterproductive for minimizing the possible risk.

	Arnd

Re: [PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Thomas Petazzoni]

Arnd,

On Thu, 28 May 2015 10:49:46 +0200, Arnd Bergmann wrote:
> On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote:
> > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> > Cc: <stable@vger.kernel.org> # v4.0+
> > ---
> >  drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
> >  1 file changed, 16 insertions(+), 89 deletions(-)
> 
> Hmm, the stable kernel rules say that a patch cannot exceed 100
> lines with context, so this one is technically too large.

Ah, okay, I didn't know about this specific rule.

> Maybe Greg has a suggestion about what to do here. Is it possible
> to make an exception for a revert? In theory you could make a
> smaller version of the patch that adds an #if 0 instead of removing
> some of the code that was added, in order to get below the limit,
> but that seems counterproductive for minimizing the possible risk.

In the specific case of such an exact revert, isn't it possible to make
an exception? I guess the very reason we have rules is to have
exceptions for such rules, no? :-)

It would really be more logical to have a revert than a different patch
just disabling the change, since it would actually be more risky than
just reverting to the previous situation.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Thomas Petazzoni]

Arnd,

On Thu, 28 May 2015 10:49:46 +0200, Arnd Bergmann wrote:
> On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote:
> > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> > Cc: <stable@vger.kernel.org> # v4.0+
> > ---
> >  drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
> >  1 file changed, 16 insertions(+), 89 deletions(-)
> 
> Hmm, the stable kernel rules say that a patch cannot exceed 100
> lines with context, so this one is technically too large.

Ah, okay, I didn't know about this specific rule.

> Maybe Greg has a suggestion about what to do here. Is it possible
> to make an exception for a revert? In theory you could make a
> smaller version of the patch that adds an #if 0 instead of removing
> some of the code that was added, in order to get below the limit,
> but that seems counterproductive for minimizing the possible risk.

In the specific case of such an exact revert, isn't it possible to make
an exception? I guess the very reason we have rules is to have
exceptions for such rules, no? :-)

It would really be more logical to have a revert than a different patch
just disabling the change, since it would actually be more risky than
just reverting to the previous situation.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Greg KH]

On Thu, May 28, 2015 at 10:49:46AM +0200, Arnd Bergmann wrote:
> On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote:
> > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> > Cc: <stable@vger.kernel.org> # v4.0+
> > ---
> >  drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
> >  1 file changed, 16 insertions(+), 89 deletions(-)
> 
> Hmm, the stable kernel rules say that a patch cannot exceed 100
> lines with context, so this one is technically too large.

"Technically" yes, but really, it's a guideline, if this is a revert,
this should be fine, I don't have an objection to taking it.

thanks,

greg k-h

[PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Greg KH]

On Thu, May 28, 2015 at 10:49:46AM +0200, Arnd Bergmann wrote:
> On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote:
> > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> > Cc: <stable@vger.kernel.org> # v4.0+
> > ---
> >  drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
> >  1 file changed, 16 insertions(+), 89 deletions(-)
> 
> Hmm, the stable kernel rules say that a patch cannot exceed 100
> lines with context, so this one is technically too large.

"Technically" yes, but really, it's a guideline, if this is a revert,
this should be fine, I don't have an objection to taking it.

thanks,

greg k-h

Re: [PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Gregory CLEMENT]

Hi Thomas,

On 28/05/2015 10:40, Thomas Petazzoni wrote:
> This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS
> for DMA don't overlap the MBus bridge window"), because it breaks DMA
> on platforms having more than 2 GB of RAM.
> 
> This commit changed the information reported to DMA masters device
> drivers through the mv_mbus_dram_info() function so that the returned
> DRAM ranges do not overlap with I/O windows.
> 
> This was necessary as a preparation to support the new CESA Crypto
> Engine driver, which will use DMA for cryptographic operations. But
> since it does DMA with the SRAM which is mapped as an I/O window,
> having DRAM ranges overlapping with I/O windows was problematic.
> 
> To solve this, the above mentioned commit changed the mvebu-mbus to
> adjust the DRAM ranges so that they don't overlap with the I/O
> windows. However, by doing this, we re-adjust the DRAM ranges in a way
> that makes them have a size that is no longer a power of two. While
> this is perfectly fine for the Crypto Engine, which supports DRAM
> ranges with a granularity of 64 KB, it breaks basically all other DMA
> masters, which expect power of two sizes for the DRAM ranges.
> 
> Due to this, if the installed system memory is 4 GB, in two
> chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB
> to a little bit less than 2 GB to not overlap with the I/O windows, in
> a way that results in a DRAM range that doesn't have a power of two
> size. This means that whenever you do a DMA transfer with an address
> located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any
> serious DMA activity like simply running:
> 
>   for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done
> 
> in an ext3 partition mounted over a SATA drive will freeze the system.
> 
> Since the new CESA crypto driver that uses DMA has not been merged
> yet, the easiest fix is to simply revert this commit. A follow-up
> commit will introduce a different solution for the CESA crypto driver.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> Cc: <stable@vger.kernel.org> # v4.0+

applied on mvebu/fixes

I hope that the 100 lines rules will be not a problem for this case

Thanks,

Gregory


> ---
>  drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
>  1 file changed, 16 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index 7fa4510..6f047dc 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -58,7 +58,6 @@
>  #include <linux/debugfs.h>
>  #include <linux/log2.h>
>  #include <linux/syscore_ops.h>
> -#include <linux/memblock.h>
>  
>  /*
>   * DDR target is the same on all platforms.
> @@ -103,9 +102,7 @@
>  
>  /* Relative to mbusbridge_base */
>  #define MBUS_BRIDGE_CTRL_OFF	0x0
> -#define  MBUS_BRIDGE_SIZE_MASK  0xffff0000
>  #define MBUS_BRIDGE_BASE_OFF	0x4
> -#define  MBUS_BRIDGE_BASE_MASK  0xffff0000
>  
>  /* Maximum number of windows, for all known platforms */
>  #define MBUS_WINS_MAX           20
> @@ -579,106 +576,36 @@ static unsigned int armada_xp_mbus_win_remap_offset(int win)
>  		return MVEBU_MBUS_NO_REMAP;
>  }
>  
> -/*
> - * Use the memblock information to find the MBus bridge hole in the
> - * physical address space.
> - */
> -static void __init
> -mvebu_mbus_find_bridge_hole(uint64_t *start, uint64_t *end)
> -{
> -	struct memblock_region *r;
> -	uint64_t s = 0;
> -
> -	for_each_memblock(memory, r) {
> -		/*
> -		 * This part of the memory is above 4 GB, so we don't
> -		 * care for the MBus bridge hole.
> -		 */
> -		if (r->base >= 0x100000000)
> -			continue;
> -
> -		/*
> -		 * The MBus bridge hole is at the end of the RAM under
> -		 * the 4 GB limit.
> -		 */
> -		if (r->base + r->size > s)
> -			s = r->base + r->size;
> -	}
> -
> -	*start = s;
> -	*end = 0x100000000;
> -}
> -
>  static void __init
>  mvebu_mbus_default_setup_cpu_target(struct mvebu_mbus_state *mbus)
>  {
>  	int i;
>  	int cs;
> -	uint64_t mbus_bridge_base, mbus_bridge_end;
>  
>  	mvebu_mbus_dram_info.mbus_dram_target_id = TARGET_DDR;
>  
> -	mvebu_mbus_find_bridge_hole(&mbus_bridge_base, &mbus_bridge_end);
> -
>  	for (i = 0, cs = 0; i < 4; i++) {
> -		u64 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
> -		u64 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
> -		u64 end;
> -		struct mbus_dram_window *w;
> -
> -		/* Ignore entries that are not enabled */
> -		if (!(size & DDR_SIZE_ENABLED))
> -			continue;
> -
> -		/*
> -		 * Ignore entries whose base address is above 2^32,
> -		 * since devices cannot DMA to such high addresses
> -		 */
> -		if (base & DDR_BASE_CS_HIGH_MASK)
> -			continue;
> -
> -		base = base & DDR_BASE_CS_LOW_MASK;
> -		size = (size | ~DDR_SIZE_MASK) + 1;
> -		end = base + size;
> -
> -		/*
> -		 * Adjust base/size of the current CS to make sure it
> -		 * doesn't overlap with the MBus bridge hole. This is
> -		 * particularly important for devices that do DMA from
> -		 * DRAM to a SRAM mapped in a MBus window, such as the
> -		 * CESA cryptographic engine.
> -		 */
> +		u32 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
> +		u32 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
>  
>  		/*
> -		 * The CS is fully enclosed inside the MBus bridge
> -		 * area, so ignore it.
> +		 * We only take care of entries for which the chip
> +		 * select is enabled, and that don't have high base
> +		 * address bits set (devices can only access the first
> +		 * 32 bits of the memory).
>  		 */
> -		if (base >= mbus_bridge_base && end <= mbus_bridge_end)
> -			continue;
> +		if ((size & DDR_SIZE_ENABLED) &&
> +		    !(base & DDR_BASE_CS_HIGH_MASK)) {
> +			struct mbus_dram_window *w;
>  
> -		/*
> -		 * Beginning of CS overlaps with end of MBus, raise CS
> -		 * base address, and shrink its size.
> -		 */
> -		if (base >= mbus_bridge_base && end > mbus_bridge_end) {
> -			size -= mbus_bridge_end - base;
> -			base = mbus_bridge_end;
> +			w = &mvebu_mbus_dram_info.cs[cs++];
> +			w->cs_index = i;
> +			w->mbus_attr = 0xf & ~(1 << i);
> +			if (mbus->hw_io_coherency)
> +				w->mbus_attr |= ATTR_HW_COHERENCY;
> +			w->base = base & DDR_BASE_CS_LOW_MASK;
> +			w->size = (size | ~DDR_SIZE_MASK) + 1;
>  		}
> -
> -		/*
> -		 * End of CS overlaps with beginning of MBus, shrink
> -		 * CS size.
> -		 */
> -		if (base < mbus_bridge_base && end > mbus_bridge_base)
> -			size -= end - mbus_bridge_base;
> -
> -		w = &mvebu_mbus_dram_info.cs[cs++];
> -		w->cs_index = i;
> -		w->mbus_attr = 0xf & ~(1 << i);
> -		if (mbus->hw_io_coherency)
> -			w->mbus_attr |= ATTR_HW_COHERENCY;
> -		w->base = base;
> -		w->size = size;
>  	}
>  	mvebu_mbus_dram_info.num_cs = cs;
>  }
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Gregory CLEMENT]

Hi Thomas,

On 28/05/2015 10:40, Thomas Petazzoni wrote:
> This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS
> for DMA don't overlap the MBus bridge window"), because it breaks DMA
> on platforms having more than 2 GB of RAM.
> 
> This commit changed the information reported to DMA masters device
> drivers through the mv_mbus_dram_info() function so that the returned
> DRAM ranges do not overlap with I/O windows.
> 
> This was necessary as a preparation to support the new CESA Crypto
> Engine driver, which will use DMA for cryptographic operations. But
> since it does DMA with the SRAM which is mapped as an I/O window,
> having DRAM ranges overlapping with I/O windows was problematic.
> 
> To solve this, the above mentioned commit changed the mvebu-mbus to
> adjust the DRAM ranges so that they don't overlap with the I/O
> windows. However, by doing this, we re-adjust the DRAM ranges in a way
> that makes them have a size that is no longer a power of two. While
> this is perfectly fine for the Crypto Engine, which supports DRAM
> ranges with a granularity of 64 KB, it breaks basically all other DMA
> masters, which expect power of two sizes for the DRAM ranges.
> 
> Due to this, if the installed system memory is 4 GB, in two
> chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB
> to a little bit less than 2 GB to not overlap with the I/O windows, in
> a way that results in a DRAM range that doesn't have a power of two
> size. This means that whenever you do a DMA transfer with an address
> located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any
> serious DMA activity like simply running:
> 
>   for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done
> 
> in an ext3 partition mounted over a SATA drive will freeze the system.
> 
> Since the new CESA crypto driver that uses DMA has not been merged
> yet, the easiest fix is to simply revert this commit. A follow-up
> commit will introduce a different solution for the CESA crypto driver.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> Cc: <stable@vger.kernel.org> # v4.0+

applied on mvebu/fixes

I hope that the 100 lines rules will be not a problem for this case

Thanks,

Gregory


> ---
>  drivers/bus/mvebu-mbus.c | 105 ++++++++---------------------------------------
>  1 file changed, 16 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index 7fa4510..6f047dc 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -58,7 +58,6 @@
>  #include <linux/debugfs.h>
>  #include <linux/log2.h>
>  #include <linux/syscore_ops.h>
> -#include <linux/memblock.h>
>  
>  /*
>   * DDR target is the same on all platforms.
> @@ -103,9 +102,7 @@
>  
>  /* Relative to mbusbridge_base */
>  #define MBUS_BRIDGE_CTRL_OFF	0x0
> -#define  MBUS_BRIDGE_SIZE_MASK  0xffff0000
>  #define MBUS_BRIDGE_BASE_OFF	0x4
> -#define  MBUS_BRIDGE_BASE_MASK  0xffff0000
>  
>  /* Maximum number of windows, for all known platforms */
>  #define MBUS_WINS_MAX           20
> @@ -579,106 +576,36 @@ static unsigned int armada_xp_mbus_win_remap_offset(int win)
>  		return MVEBU_MBUS_NO_REMAP;
>  }
>  
> -/*
> - * Use the memblock information to find the MBus bridge hole in the
> - * physical address space.
> - */
> -static void __init
> -mvebu_mbus_find_bridge_hole(uint64_t *start, uint64_t *end)
> -{
> -	struct memblock_region *r;
> -	uint64_t s = 0;
> -
> -	for_each_memblock(memory, r) {
> -		/*
> -		 * This part of the memory is above 4 GB, so we don't
> -		 * care for the MBus bridge hole.
> -		 */
> -		if (r->base >= 0x100000000)
> -			continue;
> -
> -		/*
> -		 * The MBus bridge hole is at the end of the RAM under
> -		 * the 4 GB limit.
> -		 */
> -		if (r->base + r->size > s)
> -			s = r->base + r->size;
> -	}
> -
> -	*start = s;
> -	*end = 0x100000000;
> -}
> -
>  static void __init
>  mvebu_mbus_default_setup_cpu_target(struct mvebu_mbus_state *mbus)
>  {
>  	int i;
>  	int cs;
> -	uint64_t mbus_bridge_base, mbus_bridge_end;
>  
>  	mvebu_mbus_dram_info.mbus_dram_target_id = TARGET_DDR;
>  
> -	mvebu_mbus_find_bridge_hole(&mbus_bridge_base, &mbus_bridge_end);
> -
>  	for (i = 0, cs = 0; i < 4; i++) {
> -		u64 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
> -		u64 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
> -		u64 end;
> -		struct mbus_dram_window *w;
> -
> -		/* Ignore entries that are not enabled */
> -		if (!(size & DDR_SIZE_ENABLED))
> -			continue;
> -
> -		/*
> -		 * Ignore entries whose base address is above 2^32,
> -		 * since devices cannot DMA to such high addresses
> -		 */
> -		if (base & DDR_BASE_CS_HIGH_MASK)
> -			continue;
> -
> -		base = base & DDR_BASE_CS_LOW_MASK;
> -		size = (size | ~DDR_SIZE_MASK) + 1;
> -		end = base + size;
> -
> -		/*
> -		 * Adjust base/size of the current CS to make sure it
> -		 * doesn't overlap with the MBus bridge hole. This is
> -		 * particularly important for devices that do DMA from
> -		 * DRAM to a SRAM mapped in a MBus window, such as the
> -		 * CESA cryptographic engine.
> -		 */
> +		u32 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
> +		u32 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
>  
>  		/*
> -		 * The CS is fully enclosed inside the MBus bridge
> -		 * area, so ignore it.
> +		 * We only take care of entries for which the chip
> +		 * select is enabled, and that don't have high base
> +		 * address bits set (devices can only access the first
> +		 * 32 bits of the memory).
>  		 */
> -		if (base >= mbus_bridge_base && end <= mbus_bridge_end)
> -			continue;
> +		if ((size & DDR_SIZE_ENABLED) &&
> +		    !(base & DDR_BASE_CS_HIGH_MASK)) {
> +			struct mbus_dram_window *w;
>  
> -		/*
> -		 * Beginning of CS overlaps with end of MBus, raise CS
> -		 * base address, and shrink its size.
> -		 */
> -		if (base >= mbus_bridge_base && end > mbus_bridge_end) {
> -			size -= mbus_bridge_end - base;
> -			base = mbus_bridge_end;
> +			w = &mvebu_mbus_dram_info.cs[cs++];
> +			w->cs_index = i;
> +			w->mbus_attr = 0xf & ~(1 << i);
> +			if (mbus->hw_io_coherency)
> +				w->mbus_attr |= ATTR_HW_COHERENCY;
> +			w->base = base & DDR_BASE_CS_LOW_MASK;
> +			w->size = (size | ~DDR_SIZE_MASK) + 1;
>  		}
> -
> -		/*
> -		 * End of CS overlaps with beginning of MBus, shrink
> -		 * CS size.
> -		 */
> -		if (base < mbus_bridge_base && end > mbus_bridge_base)
> -			size -= end - mbus_bridge_base;
> -
> -		w = &mvebu_mbus_dram_info.cs[cs++];
> -		w->cs_index = i;
> -		w->mbus_attr = 0xf & ~(1 << i);
> -		if (mbus->hw_io_coherency)
> -			w->mbus_attr |= ATTR_HW_COHERENCY;
> -		w->base = base;
> -		w->size = size;
>  	}
>  	mvebu_mbus_dram_info.num_cs = cs;
>  }
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Greg KH]

On Thu, May 28, 2015 at 11:17:10AM +0200, Gregory CLEMENT wrote:
> Hi Thomas,
> 
> On 28/05/2015 10:40, Thomas Petazzoni wrote:
> > This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS
> > for DMA don't overlap the MBus bridge window"), because it breaks DMA
> > on platforms having more than 2 GB of RAM.
> > 
> > This commit changed the information reported to DMA masters device
> > drivers through the mv_mbus_dram_info() function so that the returned
> > DRAM ranges do not overlap with I/O windows.
> > 
> > This was necessary as a preparation to support the new CESA Crypto
> > Engine driver, which will use DMA for cryptographic operations. But
> > since it does DMA with the SRAM which is mapped as an I/O window,
> > having DRAM ranges overlapping with I/O windows was problematic.
> > 
> > To solve this, the above mentioned commit changed the mvebu-mbus to
> > adjust the DRAM ranges so that they don't overlap with the I/O
> > windows. However, by doing this, we re-adjust the DRAM ranges in a way
> > that makes them have a size that is no longer a power of two. While
> > this is perfectly fine for the Crypto Engine, which supports DRAM
> > ranges with a granularity of 64 KB, it breaks basically all other DMA
> > masters, which expect power of two sizes for the DRAM ranges.
> > 
> > Due to this, if the installed system memory is 4 GB, in two
> > chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB
> > to a little bit less than 2 GB to not overlap with the I/O windows, in
> > a way that results in a DRAM range that doesn't have a power of two
> > size. This means that whenever you do a DMA transfer with an address
> > located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any
> > serious DMA activity like simply running:
> > 
> >   for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done
> > 
> > in an ext3 partition mounted over a SATA drive will freeze the system.
> > 
> > Since the new CESA crypto driver that uses DMA has not been merged
> > yet, the easiest fix is to simply revert this commit. A follow-up
> > commit will introduce a different solution for the CESA crypto driver.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> > Cc: <stable@vger.kernel.org> # v4.0+
> 
> applied on mvebu/fixes
> 
> I hope that the 100 lines rules will be not a problem for this case

It will not be, don't worry.

thanks,

greg k-h

[PATCH 2/3] Revert "bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"

[Greg KH]

On Thu, May 28, 2015 at 11:17:10AM +0200, Gregory CLEMENT wrote:
> Hi Thomas,
> 
> On 28/05/2015 10:40, Thomas Petazzoni wrote:
> > This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS
> > for DMA don't overlap the MBus bridge window"), because it breaks DMA
> > on platforms having more than 2 GB of RAM.
> > 
> > This commit changed the information reported to DMA masters device
> > drivers through the mv_mbus_dram_info() function so that the returned
> > DRAM ranges do not overlap with I/O windows.
> > 
> > This was necessary as a preparation to support the new CESA Crypto
> > Engine driver, which will use DMA for cryptographic operations. But
> > since it does DMA with the SRAM which is mapped as an I/O window,
> > having DRAM ranges overlapping with I/O windows was problematic.
> > 
> > To solve this, the above mentioned commit changed the mvebu-mbus to
> > adjust the DRAM ranges so that they don't overlap with the I/O
> > windows. However, by doing this, we re-adjust the DRAM ranges in a way
> > that makes them have a size that is no longer a power of two. While
> > this is perfectly fine for the Crypto Engine, which supports DRAM
> > ranges with a granularity of 64 KB, it breaks basically all other DMA
> > masters, which expect power of two sizes for the DRAM ranges.
> > 
> > Due to this, if the installed system memory is 4 GB, in two
> > chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB
> > to a little bit less than 2 GB to not overlap with the I/O windows, in
> > a way that results in a DRAM range that doesn't have a power of two
> > size. This means that whenever you do a DMA transfer with an address
> > located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any
> > serious DMA activity like simply running:
> > 
> >   for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done
> > 
> > in an ext3 partition mounted over a SATA drive will freeze the system.
> > 
> > Since the new CESA crypto driver that uses DMA has not been merged
> > yet, the easiest fix is to simply revert this commit. A follow-up
> > commit will introduce a different solution for the CESA crypto driver.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window")
> > Cc: <stable@vger.kernel.org> # v4.0+
> 
> applied on mvebu/fixes
> 
> I hope that the 100 lines rules will be not a problem for this case

It will not be, don't worry.

thanks,

greg k-h

[PATCH 3/3] bus: mvebu-mbus: add mv_mbus_dram_info_nooverlap()

[Thomas Petazzoni]

This commit introduces a variant of the mv_mbus_dram_info() function
called mv_mbus_dram_info_nooverlap(). Both functions are used by
Marvell drivers supporting devices doing DMA, and provide them a
description the DRAM ranges that they need to configure their DRAM
windows.

The ranges provided by the mv_mbus_dram_info() function may overlap
with the I/O windows if there is a lot (>= 4 GB) of RAM
installed. This is not a problem for most of the DMA masters, except
for the upcoming new CESA crypto driver because it does DMA to the
SRAM, which is mapped through an I/O window. For this unit, we need to
have DRAM ranges that do not overlap with the I/O windows.

A first implementation done in commit 1737cac69369 ("bus: mvebu-mbus:
make sure SDRAM CS for DMA don't overlap the MBus bridge window"),
changed the information returned by mv_mbus_dram_info() to match this
requirement. However, it broke the requirement of the other DMA
masters than the DRAM ranges should have power of two sizes.

To solve this situation, this commit introduces a new
mv_mbus_dram_info_nooverlap() function, which returns the same
information as mv_mbus_dram_info(), but guaranteed to not overlap with
the I/O windows.

In the end, it gives us two variants of the mv_mbus_dram_info*()
functions:

 - The normal one, mv_mbus_dram_info(), which has been around for many
   years. This function returns the raw DRAM ranges, which are
   guaranteed to use power of two sizes, but will overlap with I/O
   windows. This function will therefore be used by all DMA masters
   (SATA, XOR, Ethernet, etc.) except the CESA crypto driver.

 - The new 'nooverlap' variant, mv_mbus_dram_info_nooverlap(). This
   function returns DRAM ranges after they have been "tweaked" to make
   sure they don't overlap with I/O windows. By doing this tweaking,
   we remove the power of two size guarantee. This variant will be
   used by the new CESA crypto driver.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/bus/mvebu-mbus.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mbus.h     |   1 +
 2 files changed, 118 insertions(+)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 6f047dc..c43c3d2 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -57,6 +57,7 @@
 #include <linux/of_address.h>
 #include <linux/debugfs.h>
 #include <linux/log2.h>
+#include <linux/memblock.h>
 #include <linux/syscore_ops.h>
 
 /*
@@ -152,13 +153,39 @@ struct mvebu_mbus_state {
 
 static struct mvebu_mbus_state mbus_state;
 
+/*
+ * We provide two variants of the mv_mbus_dram_info() function:
+ *
+ * - The normal one, where the described DRAM ranges may overlap with
+ *   the I/O windows, but for which the DRAM ranges are guaranteed to
+ *   have a power of two size. Such ranges are suitable for the DMA
+ *   masters that only DMA between the RAM and the device, which is
+ *   actually all devices except the crypto engines.
+ *
+ * - The 'nooverlap' one, where the described DRAM ranges are
+ *   guaranteed to not overlap with the I/O windows, but for which the
+ *   DRAM ranges will not have power of two sizes. They will only be
+ *   aligned on a 64 KB boundary, and have a size multiple of 64
+ *   KB. Such ranges are suitable for the DMA masters that DMA between
+ *   the crypto SRAM (which is mapped through an I/O window) and a
+ *   device. This is the case for the crypto engines.
+ */
+
 static struct mbus_dram_target_info mvebu_mbus_dram_info;
+static struct mbus_dram_target_info mvebu_mbus_dram_info_nooverlap;
+
 const struct mbus_dram_target_info *mv_mbus_dram_info(void)
 {
 	return &mvebu_mbus_dram_info;
 }
 EXPORT_SYMBOL_GPL(mv_mbus_dram_info);
 
+const struct mbus_dram_target_info *mv_mbus_dram_info_nooverlap(void)
+{
+	return &mvebu_mbus_dram_info_nooverlap;
+}
+EXPORT_SYMBOL_GPL(mv_mbus_dram_info_nooverlap);
+
 /* Checks whether the given window has remap capability */
 static bool mvebu_mbus_window_is_remappable(struct mvebu_mbus_state *mbus,
 					    const int win)
@@ -576,6 +603,95 @@ static unsigned int armada_xp_mbus_win_remap_offset(int win)
 		return MVEBU_MBUS_NO_REMAP;
 }
 
+/*
+ * Use the memblock information to find the MBus bridge hole in the
+ * physical address space.
+ */
+static void __init
+mvebu_mbus_find_bridge_hole(uint64_t *start, uint64_t *end)
+{
+	struct memblock_region *r;
+	uint64_t s = 0;
+
+	for_each_memblock(memory, r) {
+		/*
+		 * This part of the memory is above 4 GB, so we don't
+		 * care for the MBus bridge hole.
+		 */
+		if (r->base >= 0x100000000ULL)
+			continue;
+
+		/*
+		 * The MBus bridge hole is at the end of the RAM under
+		 * the 4 GB limit.
+		 */
+		if (r->base + r->size > s)
+			s = r->base + r->size;
+	}
+
+	*start = s;
+	*end = 0x100000000ULL;
+}
+
+/*
+ * This function fills in the mvebu_mbus_dram_info_nooverlap data
+ * structure, by looking at the mvebu_mbus_dram_info data, and
+ * removing the parts of it that overlap with I/O windows.
+ */
+static void __init
+mvebu_mbus_setup_cpu_target_nooverlap(struct mvebu_mbus_state *mbus)
+{
+	uint64_t mbus_bridge_base, mbus_bridge_end;
+	int cs_nooverlap = 0;
+	int i;
+
+	mvebu_mbus_find_bridge_hole(&mbus_bridge_base, &mbus_bridge_end);
+
+	for (i = 0; i < mvebu_mbus_dram_info.num_cs; i++) {
+		struct mbus_dram_window *w;
+		u64 base, size, end;
+
+		w = &mvebu_mbus_dram_info.cs[i];
+		base = w->base;
+		size = w->size;
+		end = base + size;
+
+		/*
+		 * The CS is fully enclosed inside the MBus bridge
+		 * area, so ignore it.
+		 */
+		if (base >= mbus_bridge_base && end <= mbus_bridge_end)
+			continue;
+
+		/*
+		 * Beginning of CS overlaps with end of MBus, raise CS
+		 * base address, and shrink its size.
+		 */
+		if (base >= mbus_bridge_base && end > mbus_bridge_end) {
+			size -= mbus_bridge_end - base;
+			base = mbus_bridge_end;
+		}
+
+		/*
+		 * End of CS overlaps with beginning of MBus, shrink
+		 * CS size.
+		 */
+		if (base < mbus_bridge_base && end > mbus_bridge_base)
+			size -= end - mbus_bridge_base;
+
+		w = &mvebu_mbus_dram_info_nooverlap.cs[cs_nooverlap++];
+		w->cs_index = i;
+		w->mbus_attr = 0xf & ~(1 << i);
+		if (mbus->hw_io_coherency)
+			w->mbus_attr |= ATTR_HW_COHERENCY;
+		w->base = base;
+		w->size = size;
+	}
+
+	mvebu_mbus_dram_info_nooverlap.mbus_dram_target_id = TARGET_DDR;
+	mvebu_mbus_dram_info_nooverlap.num_cs = cs_nooverlap;
+}
+
 static void __init
 mvebu_mbus_default_setup_cpu_target(struct mvebu_mbus_state *mbus)
 {
@@ -964,6 +1080,7 @@ static int __init mvebu_mbus_common_init(struct mvebu_mbus_state *mbus,
 		mvebu_mbus_disable_window(mbus, win);
 
 	mbus->soc->setup_cpu_target(mbus);
+	mvebu_mbus_setup_cpu_target_nooverlap(mbus);
 
 	if (is_coherent)
 		writel(UNIT_SYNC_BARRIER_ALL,
diff --git a/include/linux/mbus.h b/include/linux/mbus.h
index 611b69f..c7f318d 100644
--- a/include/linux/mbus.h
+++ b/include/linux/mbus.h
@@ -54,6 +54,7 @@ struct mbus_dram_target_info
  */
 #ifdef CONFIG_PLAT_ORION
 extern const struct mbus_dram_target_info *mv_mbus_dram_info(void);
+extern const struct mbus_dram_target_info *mv_mbus_dram_info_nooverlap(void);
 #else
 static inline const struct mbus_dram_target_info *mv_mbus_dram_info(void)
 {
-- 
2.1.0

[PATCH 3/3] bus: mvebu-mbus: add mv_mbus_dram_info_nooverlap()

[Thomas Petazzoni]

Hello,

On Thu, 28 May 2015 10:40:14 +0200, Thomas Petazzoni wrote:

> diff --git a/include/linux/mbus.h b/include/linux/mbus.h
> index 611b69f..c7f318d 100644
> --- a/include/linux/mbus.h
> +++ b/include/linux/mbus.h
> @@ -54,6 +54,7 @@ struct mbus_dram_target_info
>   */
>  #ifdef CONFIG_PLAT_ORION
>  extern const struct mbus_dram_target_info *mv_mbus_dram_info(void);
> +extern const struct mbus_dram_target_info *mv_mbus_dram_info_nooverlap(void);
>  #else
>  static inline const struct mbus_dram_target_info *mv_mbus_dram_info(void)
>  {

I obviously forgot to add mv_mbus_dram_info_nooverlap() in the #else
case here. I'll do it for v2, but I'll wait a bit for other comments
before sending the v2.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com