dmaengine: mv_xor: Add support for IO (PCIe) src/dst areas

To enable the access to a specific area, the MVEBU XOR controllers needs to have this area enabled / mapped via an address window. Right now, only the DRAM memory area is enabled via such memory windows. So using this driver to DMA to / from a e.g. PCIe memory region is currently not supported.

This patch now adds support for such PCIe / IO regions by checking if the src / dst address is located in an IO memory area in contrast to being located in DRAM. This is done by using the newly introduced MBus function mvebu_mbus_get_io_win_info(). If the src / dst address is located in such an IO area, a new address window is created in the XOR DMA controller. Enabling the controller to access this area.

Signed-off-by Stefan Roese <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Marcin Wojtas <[email protected]>

Cc Vinod Koul <[email protected]>

---
  drivers/dma/mv_xor.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 106 insertions(+), 1 deletion(-)

drivers/dma/mv_xor.c +100 -1

index f4c9f98..2671b11 100644
@@ -470,12 +470,107 @@ static int mv_xor_alloc_chan_resources(struct dma_chan *chan)
  	return mv_chan->slots_allocated ? : -ENOMEM;
  }
 
+/*
+ * Check if source or destination is an PCIe/IO address (non-SDRAM) and add
+ * a new MBus window if necessary
+ */
+static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
+{
+	void __iomem *base = mv_chan->mmr_high_base;
+	u32 win_enable;
+	u32 size;
+	u8 target, attr;
+	int ret;
+	int i;
+
+	/* If no IO window is found that addr has to be located in SDRAM */
+	ret = mvebu_mbus_get_io_win_info(addr, &size, &target, &attr);
+	if (ret < 0)
+		return 0;
+
+	/*
+	 * Mask the base addr 'addr' according to 'size' read back from the
+	 * MBus window. Otherwise we might end up with an address located
+	 * somewhere in the middle of this area here.
+	 */
+	size -= 1;
+	addr &= ~size;
+
+	/*
+	 * Reading one of both enabled register is enough, as they are always
+	 * programmed to the identical values
+	 */
+	win_enable = readl(base + WINDOW_BAR_ENABLE(0));
+
+	/*
+	 * Loop over all windows to find a matching window (area wise). If
+	 * one is found it will get disabled and later newly created.
+	 */
+	for (i = 0; i < 8; i++) {
+		u32 wbase;
+		u32 wsize;
+
+		/* Continue if the window is not enabled */
+		if (!(win_enable | (1 << i)))
+			continue;
+
+		wbase = readl(base + WINDOW_BASE(i)) & 0xffff0000;
+		wsize = readl(base + WINDOW_SIZE(i)) & 0xffff0000;
+
+		/* Continue if 'addr' is not in this window */
+		if (addr < wbase || addr > (wbase + wsize))
+			continue;
+
+		/*
+		 * If addr and size match, then this window is already
+		 * configured and we are done
+		 */
+		if (addr == wbase && (size & 0xffff0000) == wsize)
+			return 0;
+
+		/*
+		 * The window is already configured, but the size does not
+		 * match, so lets disable it
+		 */
+		writel(0, base + WINDOW_BASE(i));
+		writel(0, base + WINDOW_SIZE(i));
+		if (i < 4)
+			writel(0, base + WINDOW_REMAP_HIGH(i));
+		win_enable &= ~(1 << i);
+		win_enable &= ~(3 << (16 + (2 * i)));
+		writel(win_enable, base + WINDOW_BAR_ENABLE(0));
+		writel(win_enable, base + WINDOW_BAR_ENABLE(1));
+
+		/*
+		 * We can stop here since we have found and disabled the window
+		 */
+		break;
+	}
+
+	/* Set 'i' to the first free window to write the new values to */
+	i = ffs(~win_enable) - 1;
+	if (i >= 8)
+		return -ENOMEM;
+
+	writel((addr & 0xffff0000) | (attr << 8) | target,
+	       base + WINDOW_BASE(i));
+	writel(size & 0xffff0000, base + WINDOW_SIZE(i));
+
+	win_enable |= (1 << i);
+	win_enable |= 3 << (16 + (2 * i));
+	writel(win_enable, base + WINDOW_BAR_ENABLE(0));
+	writel(win_enable, base + WINDOW_BAR_ENABLE(1));
+
+	return 0;
+}
+
  static struct dma_async_tx_descriptor *
  mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
  		    unsigned int src_cnt, size_t len, unsigned long flags)
  {
  	struct mv_xor_chan *mv_chan = to_mv_xor_chan(chan);
  	struct mv_xor_desc_slot *sw_desc;
+	int ret;
 
  	if (unlikely(len < MV_XOR_MIN_BYTE_COUNT))
  		return NULL;

@@ -486,6 +581,11 @@ mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
  		"%s src_cnt: %d len: %zu dest %pad flags: %ld\n",
  		__func__, src_cnt, len, &dest, flags);
 
+	/* Check if a new window needs to get added for 'dest' */
+	ret = mv_xor_add_io_win(mv_chan, dest);
+	if (ret)
+		return NULL;
+
  	sw_desc = mv_chan_alloc_slot(mv_chan);
  	if (sw_desc) {
  		sw_desc->type = DMA_XOR;

@@ -493,8 +593,13 @@ mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
  		mv_desc_init(sw_desc, dest, len, flags);
  		if (mv_chan->op_in_desc == XOR_MODE_IN_DESC)
  			mv_desc_set_mode(sw_desc);
-		while (src_cnt--)

Hi!

Quoted 167 lines · Stefan Roese

To enable the access to a specific area, the MVEBU XOR controllers needs to have this area enabled / mapped via an address window. Right now, only the DRAM memory area is enabled via such memory windows. So using this driver to DMA to / from a e.g. PCIe memory region is currently not supported.

This patch now adds support for such PCIe / IO regions by checking if the src / dst address is located in an IO memory area in contrast to being located in DRAM. This is done by using the newly introduced MBus function mvebu_mbus_get_io_win_info(). If the src / dst address is located in such an IO area, a new address window is created in the XOR DMA controller. Enabling the controller to access this area.

Signed-off-by Stefan Roese <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Marcin Wojtas <[email protected]>

Cc Vinod Koul <[email protected]>

---
  drivers/dma/mv_xor.c | 107
++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 106 insertions(+), 1 deletion(-)

drivers/dma/mv_xor.c +2 -0

index f4c9f98..2671b11 100644
@@ -470,12 +470,107 @@ static int mv_xor_alloc_chan_resources(struct
 dma_chan *chan)
      return mv_chan->slots_allocated ? : -ENOMEM;
  }
 
+/*
+ * Check if source or destination is an PCIe/IO address (non-SDRAM)

and add
+ * a new MBus window if necessary
+ */

drivers/dma/mv_xor.c +24 -0

+static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
+{
+    void __iomem *base = mv_chan->mmr_high_base;
+    u32 win_enable;
+    u32 size;
+    u8 target, attr;
+    int ret;
+    int i;
+
+    /* If no IO window is found that addr has to be located in SDRAM */
+    ret = mvebu_mbus_get_io_win_info(addr, &size, &target, &attr);
+    if (ret < 0)
+        return 0;
+
+    /*
+     * Mask the base addr 'addr' according to 'size' read back from the
+     * MBus window. Otherwise we might end up with an address located
+     * somewhere in the middle of this area here.
+     */
+    size -= 1;
+    addr &= ~size;
+
+    /*
+     * Reading one of both enabled register is enough, as they are

always

drivers/dma/mv_xor.c +66 -0

+     * programmed to the identical values
+     */
+    win_enable = readl(base + WINDOW_BAR_ENABLE(0));
+
+    /*
+     * Loop over all windows to find a matching window (area wise). If
+     * one is found it will get disabled and later newly created.
+     */
+    for (i = 0; i < 8; i++) {
+        u32 wbase;
+        u32 wsize;
+
+        /* Continue if the window is not enabled */
+        if (!(win_enable | (1 << i)))
+            continue;
+
+        wbase = readl(base + WINDOW_BASE(i)) & 0xffff0000;
+        wsize = readl(base + WINDOW_SIZE(i)) & 0xffff0000;
+
+        /* Continue if 'addr' is not in this window */
+        if (addr < wbase || addr > (wbase + wsize))
+            continue;
+
+        /*
+         * If addr and size match, then this window is already
+         * configured and we are done
+         */
+        if (addr == wbase && (size & 0xffff0000) == wsize)
+            return 0;
+
+        /*
+         * The window is already configured, but the size does not
+         * match, so lets disable it
+         */
+        writel(0, base + WINDOW_BASE(i));
+        writel(0, base + WINDOW_SIZE(i));
+        if (i < 4)
+            writel(0, base + WINDOW_REMAP_HIGH(i));
+        win_enable &= ~(1 << i);
+        win_enable &= ~(3 << (16 + (2 * i)));
+        writel(win_enable, base + WINDOW_BAR_ENABLE(0));
+        writel(win_enable, base + WINDOW_BAR_ENABLE(1));
+
+        /*
+         * We can stop here since we have found and disabled the window
+         */
+        break;
+    }
+
+    /* Set 'i' to the first free window to write the new values to */
+    i = ffs(~win_enable) - 1;
+    if (i >= 8)
+        return -ENOMEM;
+
+    writel((addr & 0xffff0000) | (attr << 8) | target,
+           base + WINDOW_BASE(i));
+    writel(size & 0xffff0000, base + WINDOW_SIZE(i));
+
+    win_enable |= (1 << i);
+    win_enable |= 3 << (16 + (2 * i));
+    writel(win_enable, base + WINDOW_BAR_ENABLE(0));
+    writel(win_enable, base + WINDOW_BAR_ENABLE(1));
+
+    return 0;
+}
+
  static struct dma_async_tx_descriptor *
  mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest,

dma_addr_t *src,
unsigned int src_cnt, size_t len, unsigned long flags)

  {
      struct mv_xor_chan *mv_chan = to_mv_xor_chan(chan);
      struct mv_xor_desc_slot *sw_desc;

drivers/dma/mv_xor.c +6 -0

+    int ret;
 
      if (unlikely(len < MV_XOR_MIN_BYTE_COUNT))
          return NULL;

@@ -486,6 +581,11 @@ mv_xor_prep_dma_xor(struct dma_chan *chan,
 dma_addr_t dest, dma_addr_t *src,
          "%s src_cnt: %d len: %zu dest %pad flags: %ld\n",
          __func__, src_cnt, len, &dest, flags);
 
+    /* Check if a new window needs to get added for 'dest' */
+    ret = mv_xor_add_io_win(mv_chan, dest);
+    if (ret)
+        return NULL;
+
      sw_desc = mv_chan_alloc_slot(mv_chan);
      if (sw_desc) {

          sw_desc->type = DMA_XOR;

drivers/dma/mv_xor.c +6 -1

@@ -493,8 +593,13 @@ mv_xor_prep_dma_xor(struct dma_chan *chan,
 dma_addr_t dest, dma_addr_t *src,
          mv_desc_init(sw_desc, dest, len, flags);
          if (mv_chan->op_in_desc == XOR_MODE_IN_DESC)
              mv_desc_set_mode(sw_desc);
-        while (src_cnt--)
+        while (src_cnt--) {
+            /* Check if a new window needs to get added for 'src' */
+            ret = mv_xor_add_io_win(mv_chan, src[src_cnt]);
+            if (ret)
+                return NULL;
              mv_desc_set_src_addr(sw_desc, src_cnt, src[src_cnt]);
+        }
      }
 

Quoted 2 lines

      dev_dbg(mv_chan_to_devp(mv_chan),

I didn't receive any comments on this patch so far. How should we proceed? Is this approach to enable DMA support to and from regions in PCI space acceptable? If yes, do I need to resend this patch as a non-RFC patch?

Hi,

Quoted 170 lines · Stefan Roese

Hi!

Quoted 167 lines · Stefan Roese

To enable the access to a specific area, the MVEBU XOR controllers needs to have this area enabled / mapped via an address window. Right now, only the DRAM memory area is enabled via such memory windows. So using this driver to DMA to / from a e.g. PCIe memory region is currently not supported.

This patch now adds support for such PCIe / IO regions by checking if the src / dst address is located in an IO memory area in contrast to being located in DRAM. This is done by using the newly introduced MBus function mvebu_mbus_get_io_win_info(). If the src / dst address is located in such an IO area, a new address window is created in the XOR DMA controller. Enabling the controller to access this area.

Signed-off-by Stefan Roese <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Marcin Wojtas <[email protected]>

Cc Vinod Koul <[email protected]>

---
  drivers/dma/mv_xor.c | 107
++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 106 insertions(+), 1 deletion(-)

drivers/dma/mv_xor.c +2 -0

index f4c9f98..2671b11 100644
@@ -470,12 +470,107 @@ static int mv_xor_alloc_chan_resources(struct
 dma_chan *chan)
      return mv_chan->slots_allocated ? : -ENOMEM;
  }
 
+/*
+ * Check if source or destination is an PCIe/IO address (non-SDRAM)

and add
+ * a new MBus window if necessary
+ */

drivers/dma/mv_xor.c +24 -0

+static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
+{
+    void __iomem *base = mv_chan->mmr_high_base;
+    u32 win_enable;
+    u32 size;
+    u8 target, attr;
+    int ret;
+    int i;
+
+    /* If no IO window is found that addr has to be located in SDRAM */
+    ret = mvebu_mbus_get_io_win_info(addr, &size, &target, &attr);
+    if (ret < 0)
+        return 0;
+
+    /*
+     * Mask the base addr 'addr' according to 'size' read back from the
+     * MBus window. Otherwise we might end up with an address located
+     * somewhere in the middle of this area here.
+     */
+    size -= 1;
+    addr &= ~size;
+
+    /*
+     * Reading one of both enabled register is enough, as they are

always

drivers/dma/mv_xor.c +66 -0

+     * programmed to the identical values
+     */
+    win_enable = readl(base + WINDOW_BAR_ENABLE(0));
+
+    /*
+     * Loop over all windows to find a matching window (area wise). If
+     * one is found it will get disabled and later newly created.
+     */
+    for (i = 0; i < 8; i++) {
+        u32 wbase;
+        u32 wsize;
+
+        /* Continue if the window is not enabled */
+        if (!(win_enable | (1 << i)))
+            continue;
+
+        wbase = readl(base + WINDOW_BASE(i)) & 0xffff0000;
+        wsize = readl(base + WINDOW_SIZE(i)) & 0xffff0000;
+
+        /* Continue if 'addr' is not in this window */
+        if (addr < wbase || addr > (wbase + wsize))
+            continue;
+
+        /*
+         * If addr and size match, then this window is already
+         * configured and we are done
+         */
+        if (addr == wbase && (size & 0xffff0000) == wsize)
+            return 0;
+
+        /*
+         * The window is already configured, but the size does not
+         * match, so lets disable it
+         */
+        writel(0, base + WINDOW_BASE(i));
+        writel(0, base + WINDOW_SIZE(i));
+        if (i < 4)
+            writel(0, base + WINDOW_REMAP_HIGH(i));
+        win_enable &= ~(1 << i);
+        win_enable &= ~(3 << (16 + (2 * i)));
+        writel(win_enable, base + WINDOW_BAR_ENABLE(0));
+        writel(win_enable, base + WINDOW_BAR_ENABLE(1));
+
+        /*
+         * We can stop here since we have found and disabled the window
+         */
+        break;
+    }
+
+    /* Set 'i' to the first free window to write the new values to */
+    i = ffs(~win_enable) - 1;
+    if (i >= 8)
+        return -ENOMEM;
+
+    writel((addr & 0xffff0000) | (attr << 8) | target,
+           base + WINDOW_BASE(i));
+    writel(size & 0xffff0000, base + WINDOW_SIZE(i));
+
+    win_enable |= (1 << i);
+    win_enable |= 3 << (16 + (2 * i));
+    writel(win_enable, base + WINDOW_BAR_ENABLE(0));
+    writel(win_enable, base + WINDOW_BAR_ENABLE(1));
+
+    return 0;
+}
+
  static struct dma_async_tx_descriptor *
  mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest,

dma_addr_t *src,
unsigned int src_cnt, size_t len, unsigned long flags)

  {
      struct mv_xor_chan *mv_chan = to_mv_xor_chan(chan);
      struct mv_xor_desc_slot *sw_desc;

drivers/dma/mv_xor.c +6 -0

+    int ret;
 
      if (unlikely(len < MV_XOR_MIN_BYTE_COUNT))
          return NULL;

@@ -486,6 +581,11 @@ mv_xor_prep_dma_xor(struct dma_chan *chan,
 dma_addr_t dest, dma_addr_t *src,
          "%s src_cnt: %d len: %zu dest %pad flags: %ld\n",
          __func__, src_cnt, len, &dest, flags);
 
+    /* Check if a new window needs to get added for 'dest' */
+    ret = mv_xor_add_io_win(mv_chan, dest);
+    if (ret)
+        return NULL;
+
      sw_desc = mv_chan_alloc_slot(mv_chan);
      if (sw_desc) {

          sw_desc->type = DMA_XOR;

drivers/dma/mv_xor.c +6 -1

@@ -493,8 +593,13 @@ mv_xor_prep_dma_xor(struct dma_chan *chan,
 dma_addr_t dest, dma_addr_t *src,
          mv_desc_init(sw_desc, dest, len, flags);
          if (mv_chan->op_in_desc == XOR_MODE_IN_DESC)
              mv_desc_set_mode(sw_desc);
-        while (src_cnt--)
+        while (src_cnt--) {
+            /* Check if a new window needs to get added for 'src' */
+            ret = mv_xor_add_io_win(mv_chan, src[src_cnt]);
+            if (ret)
+                return NULL;
              mv_desc_set_src_addr(sw_desc, src_cnt, src[src_cnt]);
+        }
      }
 

Quoted 7 lines

Quoted 2 lines

      dev_dbg(mv_chan_to_devp(mv_chan),

I didn't receive any comments on this patch so far. How should we proceed? Is this approach to enable DMA support to and from regions in PCI space acceptable? If yes, do I need to resend this patch as a non-RFC patch?

Thomas and I discussed this patch a bit off-list in the meantime (thanks Thomas). Here a short summary of the last ideas (Thomas, please correct me, if you feel I didn't summarize it correctly):

Thomas raised some concerns, as this patch adds a big number of register reads in the middle of the submission path for XOR requests. To solve this, I suggested to add a DT property to enable this PCI / IO window check in the XOR driver. But as DT should describe the HW and this is not a "HW feature", acceptance of such a new DT property is very unlikely.

Another idea was to add a similar mechanism for the PCI spaces as done for the DDR space(s) to the MBus driver. So that drivers interested in these PCI windows could query the MBus driver about the currently configured windows:

    mv_mbus_dram_info()

-> mv_mbus_pci_info()

But as the XOR engine only has a limited number of windows, it might happen that on a system with numerous PCI devices, all the MBus PCI windows plus the DRAM windows will exhaust the number of the XOR engine windows. Plus the dynamic nature of PCI devices in hotplug systems. So this won't work either.

Currently we are out of good ideas on how to get this feature added to this XOR engine driver. Perhaps the only way is to accept the additional register accesses introduced by this patch? Thomas explicitly suggested to ask you Arnd, if you had some ideas on this. So here we go:

Arnd, do you have some ideas how to better solve this problem?

Quoted 212 lines · CEST Stefan Roese

Hi,

Quoted 170 lines · Stefan Roese

Hi!

Quoted 167 lines · Stefan Roese

To enable the access to a specific area, the MVEBU XOR controllers needs to have this area enabled / mapped via an address window. Right now, only the DRAM memory area is enabled via such memory windows. So using this driver to DMA to / from a e.g. PCIe memory region is currently not supported.

This patch now adds support for such PCIe / IO regions by checking if the src / dst address is located in an IO memory area in contrast to being located in DRAM. This is done by using the newly introduced MBus function mvebu_mbus_get_io_win_info(). If the src / dst address is located in such an IO area, a new address window is created in the XOR DMA controller. Enabling the controller to access this area.

Signed-off-by Stefan Roese <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Marcin Wojtas <[email protected]>

Cc Vinod Koul <[email protected]>

---
  drivers/dma/mv_xor.c | 107
++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 106 insertions(+), 1 deletion(-)

drivers/dma/mv_xor.c +2 -0

index f4c9f98..2671b11 100644
@@ -470,12 +470,107 @@ static int mv_xor_alloc_chan_resources(struct
 dma_chan *chan)
      return mv_chan->slots_allocated ? : -ENOMEM;
  }
 
+/*
+ * Check if source or destination is an PCIe/IO address (non-SDRAM)

and add
+ * a new MBus window if necessary
+ */

drivers/dma/mv_xor.c +24 -0

+static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
+{
+    void __iomem *base = mv_chan->mmr_high_base;
+    u32 win_enable;
+    u32 size;
+    u8 target, attr;
+    int ret;
+    int i;
+
+    /* If no IO window is found that addr has to be located in SDRAM */
+    ret = mvebu_mbus_get_io_win_info(addr, &size, &target, &attr);
+    if (ret < 0)
+        return 0;
+
+    /*
+     * Mask the base addr 'addr' according to 'size' read back from the
+     * MBus window. Otherwise we might end up with an address located
+     * somewhere in the middle of this area here.
+     */
+    size -= 1;
+    addr &= ~size;
+
+    /*
+     * Reading one of both enabled register is enough, as they are

always

drivers/dma/mv_xor.c +66 -0

+     * programmed to the identical values
+     */
+    win_enable = readl(base + WINDOW_BAR_ENABLE(0));
+
+    /*
+     * Loop over all windows to find a matching window (area wise). If
+     * one is found it will get disabled and later newly created.
+     */
+    for (i = 0; i < 8; i++) {
+        u32 wbase;
+        u32 wsize;
+
+        /* Continue if the window is not enabled */
+        if (!(win_enable | (1 << i)))
+            continue;
+
+        wbase = readl(base + WINDOW_BASE(i)) & 0xffff0000;
+        wsize = readl(base + WINDOW_SIZE(i)) & 0xffff0000;
+
+        /* Continue if 'addr' is not in this window */
+        if (addr < wbase || addr > (wbase + wsize))
+            continue;
+
+        /*
+         * If addr and size match, then this window is already
+         * configured and we are done
+         */
+        if (addr == wbase && (size & 0xffff0000) == wsize)
+            return 0;
+
+        /*
+         * The window is already configured, but the size does not
+         * match, so lets disable it
+         */
+        writel(0, base + WINDOW_BASE(i));
+        writel(0, base + WINDOW_SIZE(i));
+        if (i < 4)
+            writel(0, base + WINDOW_REMAP_HIGH(i));
+        win_enable &= ~(1 << i);
+        win_enable &= ~(3 << (16 + (2 * i)));
+        writel(win_enable, base + WINDOW_BAR_ENABLE(0));
+        writel(win_enable, base + WINDOW_BAR_ENABLE(1));
+
+        /*
+         * We can stop here since we have found and disabled the window
+         */
+        break;
+    }
+
+    /* Set 'i' to the first free window to write the new values to */
+    i = ffs(~win_enable) - 1;
+    if (i >= 8)
+        return -ENOMEM;
+
+    writel((addr & 0xffff0000) | (attr << 8) | target,
+           base + WINDOW_BASE(i));
+    writel(size & 0xffff0000, base + WINDOW_SIZE(i));
+
+    win_enable |= (1 << i);
+    win_enable |= 3 << (16 + (2 * i));
+    writel(win_enable, base + WINDOW_BAR_ENABLE(0));
+    writel(win_enable, base + WINDOW_BAR_ENABLE(1));
+
+    return 0;
+}
+
  static struct dma_async_tx_descriptor *
  mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest,

dma_addr_t *src,
unsigned int src_cnt, size_t len, unsigned long flags)

  {
      struct mv_xor_chan *mv_chan = to_mv_xor_chan(chan);
      struct mv_xor_desc_slot *sw_desc;

drivers/dma/mv_xor.c +6 -0

+    int ret;
 
      if (unlikely(len < MV_XOR_MIN_BYTE_COUNT))
          return NULL;

@@ -486,6 +581,11 @@ mv_xor_prep_dma_xor(struct dma_chan *chan,
 dma_addr_t dest, dma_addr_t *src,
          "%s src_cnt: %d len: %zu dest %pad flags: %ld\n",
          __func__, src_cnt, len, &dest, flags);
 
+    /* Check if a new window needs to get added for 'dest' */
+    ret = mv_xor_add_io_win(mv_chan, dest);
+    if (ret)
+        return NULL;
+
      sw_desc = mv_chan_alloc_slot(mv_chan);
      if (sw_desc) {

          sw_desc->type = DMA_XOR;

drivers/dma/mv_xor.c +6 -1

@@ -493,8 +593,13 @@ mv_xor_prep_dma_xor(struct dma_chan *chan,
 dma_addr_t dest, dma_addr_t *src,
          mv_desc_init(sw_desc, dest, len, flags);
          if (mv_chan->op_in_desc == XOR_MODE_IN_DESC)
              mv_desc_set_mode(sw_desc);
-        while (src_cnt--)
+        while (src_cnt--) {
+            /* Check if a new window needs to get added for 'src' */
+            ret = mv_xor_add_io_win(mv_chan, src[src_cnt]);
+            if (ret)
+                return NULL;
              mv_desc_set_src_addr(sw_desc, src_cnt, src[src_cnt]);
+        }
      }
 

Quoted 7 lines

Quoted 2 lines

      dev_dbg(mv_chan_to_devp(mv_chan),

I didn't receive any comments on this patch so far. How should we proceed? Is this approach to enable DMA support to and from regions in PCI space acceptable? If yes, do I need to resend this patch as a non-RFC patch?

Thomas and I discussed this patch a bit off-list in the meantime (thanks Thomas). Here a short summary of the last ideas (Thomas, please correct me, if you feel I didn't summarize it correctly):

Thomas raised some concerns, as this patch adds a big number of register reads in the middle of the submission path for XOR requests. To solve this, I suggested to add a DT property to enable this PCI / IO window check in the XOR driver. But as DT should describe the HW and this is not a "HW feature", acceptance of such a new DT property is very unlikely.

Another idea was to add a similar mechanism for the PCI spaces as done for the DDR space(s) to the MBus driver. So that drivers interested in these PCI windows could query the MBus driver about the currently configured windows:

    mv_mbus_dram_info()

-> mv_mbus_pci_info()

But as the XOR engine only has a limited number of windows, it might happen that on a system with numerous PCI devices, all the MBus PCI windows plus the DRAM windows will exhaust the number of the XOR engine windows. Plus the dynamic nature of PCI devices in hotplug systems. So this won't work either.

Currently we are out of good ideas on how to get this feature added to this XOR engine driver. Perhaps the only way is to accept the additional register accesses introduced by this patch? Thomas explicitly suggested to ask you Arnd, if you had some ideas on this. So here we go:

Quoted 1 line

Arnd, do you have some ideas how to better solve this problem?

I'm not sure I completely understand the problem yet, but my first thought would be caching: have a copy of the ranges in RAM and loop over that in order to decide whether to evict one entry or not, avoiding the need for mmio register reads completely, and most of the register writes as well.

The mv_mbus_pci_info() should work as well, but it's possible you can actually express that in a more generic way, since the location of the PCI memory space should also be known to the PCI core.

Finally, what is the specific limitation on the number and size of the windows?

Could you just create static mappings that span
all of the potential address space, e.g. a single entry for any address visibile to the CPU? I can't tell if you just need the physical address as seen by the CPU (which would be easy) or if you need the mbus address with their register numbers (which probably doesn't have a single contiguous representation?

Do you just need to cover PCI or also things like the SPI-NOR direct mapping address?

Quoted 8 lines

Quoted 2 lines

Arnd, do you have some ideas how to better solve this problem?

I'm not sure I completely understand the problem yet, but my first thought would be caching: have a copy of the ranges in RAM and loop over that in order to decide whether to evict one entry or not, avoiding the need for mmio register reads completely, and most of the register writes as well.

Good idea. I would still need to call mvebu_mbus_get_io_win_info(), but the mmio accessed in this XOR driver could be minimized this way.

Quoted 4 lines

The mv_mbus_pci_info() should work as well, but it's possible you can actually express that in a more generic way, since the location of the PCI memory space should also be known to the PCI core.

But we need the MBus target and attr for these different windows as well. So querying the MBus driver seems the only way for me.

Quoted 2 lines

Finally, what is the specific limitation on the number and size of the windows?

A max. of 8 windows. The windows can be from 64KiB to 4GiB.

Quoted 6 lines

Could you just create static mappings that span
all of the potential address space, e.g. a single entry for any address visibile to the CPU? I can't tell if you just need the physical address as seen by the CPU (which would be easy) or if you need the mbus address with their register numbers (which probably doesn't have a single contiguous representation?

Unfortunately we need the MBus target and attribute values for each window. So a summarized representation probably won't work.

Quoted 2 lines

Do you just need to cover PCI or also things like the SPI-NOR direct mapping address?

My current use-case is just PCI.

Perhaps the cached mmio version would be acceptable? If I get an "go for it and I'll ack the patch v2" from someone, then I will gladly invest some time in this.