On Wednesday, July 27, 2016 8:14:15 AM CEST Stefan Roese wrote:
> Hi,
>
> On 29.06.2016 13:22, Stefan Roese wrote:
> > Hi!
> >
> > On 03.06.2016 18:24, Stefan Roese wrote:
> >> 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 <sr@xxxxxxx>
> >> Cc: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
> >> Cc: Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxxxxxxxxx>
> >> Cc: Marcin Wojtas <mw@xxxxxxxxxxxx>
> >> Cc: Vinod Koul <vinod.koul@xxxxxxxxx>
> >> ---
> >> drivers/dma/mv_xor.c | 107
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 106 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> >> index f4c9f98..2671b11 100644
> >> --- a/drivers/dma/mv_xor.c
> >> +++ b/drivers/dma/mv_xor.c
> >> @@ -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--)
> >> + 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]);
> >> + }
> >> }
> >>
> >> 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?
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?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
