| Message ID | 1431445063-20226-3-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Tuesday 12 May 2015 17:37:37 Maxime Ripard wrote: > From: Lior Amsalem <alior@marvell.com> > > The Marvell Armada 38x SoC introduce new features to the XOR engine, > especially the fact that the engine mode (MEMCPY/XOR/PQ/etc) can be part of > the descriptor and not set through the controller registers. > > This new feature allows mixing of different commands (even PQ) on the same > channel/chain without the need to stop the engine to reconfigure the engine > mode. > > Refactor the driver to be able to use that new feature on the Armada 38x, > while keeping the old behaviour on the older SoCs. > > Signed-off-by: Lior Amsalem <alior@marvell.com> > Reviewed-by: Ofer Heifetz <oferh@marvell.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Two minimal style comments: > +static void mv_chan_set_mode_to_desc(struct mv_xor_chan *chan) > +{ > + u32 op_mode; > + u32 config = readl_relaxed(XOR_CONFIG(chan)); > + > + op_mode = XOR_OPERATION_MODE_IN_DESC; > + > + config &= ~0x7; > + config |= op_mode; > + > +#if defined(__BIG_ENDIAN) > + config |= XOR_DESCRIPTOR_SWAP; > +#else > + config &= ~XOR_DESCRIPTOR_SWAP; > +#endif > + > + writel_relaxed(config, XOR_CONFIG(chan)); > +} Using if (IS_ENABLED(__BIG_ENDIAN)) here would make it more readable by avoiding the #if. Alternatively, you could leave the XOR_DESCRIPTOR_SWAP flag disabled and just swap the descriptors manually like a lot of other drivers do. You have to swap the mmio accesses anywya. > } > > +#ifdef CONFIG_OF > +static const struct of_device_id mv_xor_dt_ids[] = { > + { .compatible = "marvell,orion-xor", .data = (void *)XOR_MODE_IN_REG }, > + { .compatible = "marvell,a38x-xor", .data = (void *)XOR_MODE_IN_DESC }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, mv_xor_dt_ids); > +#endif > + Just leave out the #ifdef here. Almost all the mvebu machines use DT now, so it's not worth the size benefit of leaving it out on the few machines that don't. You'll have to remove the of_match_ptr() invocation as well if you do that, to avoid a warning about an unused symbol. Arnd -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Maxime, On Tue, 12 May 2015 17:37:37 +0200, Maxime Ripard wrote: > Required properties: > -- compatible: Should be "marvell,orion-xor" > +- compatible: Should be "marvell,orion-xor" or "marvell,a38x-xor" I believe the new compatible string should be armada-380-xor or armada380-xor. Wildcards in compatible strings are generally not recommended, and we don't use the a38x- prefix anywhere in the tree, as far as I can see: drivers/ata/ahci_mvebu.c: { .compatible = "marvell,armada-380-ahci", }, drivers/bus/mvebu-mbus.c: { .compatible = "marvell,armada380-mbus", drivers/mmc/host/sdhci-pxav3.c: .compatible = "marvell,armada-380-sdhci", drivers/rtc/rtc-armada38x.c: { .compatible = "marvell,armada-380-rtc", }, drivers/thermal/armada_thermal.c: .compatible = "marvell,armada380-thermal", drivers/usb/host/xhci-plat.c: { .compatible = "marvell,armada-380-xhci"}, drivers/watchdog/orion_wdt.c: .compatible = "marvell,armada-380-wdt", Yes, we're not very consistent between armada380 and armada-380, but we're not using a38x anywhere. Best regards, Thomas
Dear Arnd Bergmann, On Tue, 12 May 2015 17:49:08 +0200, Arnd Bergmann wrote: > Using > > if (IS_ENABLED(__BIG_ENDIAN)) > > here would make it more readable by avoiding the #if. Alternatively, > you could leave the XOR_DESCRIPTOR_SWAP flag disabled and just swap > the descriptors manually like a lot of other drivers do. You have > to swap the mmio accesses anywya. Agreed on IS_ENABLED(). However, I don't understand the comment about the need to swap mmio accesses? We're using readl_relaxed() / writel_relaxed(), and they do the swapping already. Am I missing something? Best regards, Thomas
On Tue, May 12, 2015 at 05:49:36PM +0200, Thomas Petazzoni wrote: > Maxime, > > On Tue, 12 May 2015 17:37:37 +0200, Maxime Ripard wrote: > > > Required properties: > > -- compatible: Should be "marvell,orion-xor" > > +- compatible: Should be "marvell,orion-xor" or "marvell,a38x-xor" > > I believe the new compatible string should be armada-380-xor or > armada380-xor. Wildcards in compatible strings are generally not > recommended, and we don't use the a38x- prefix anywhere in the tree, as > far as I can see: > > drivers/ata/ahci_mvebu.c: { .compatible = "marvell,armada-380-ahci", }, > drivers/bus/mvebu-mbus.c: { .compatible = "marvell,armada380-mbus", > drivers/mmc/host/sdhci-pxav3.c: .compatible = "marvell,armada-380-sdhci", > drivers/rtc/rtc-armada38x.c: { .compatible = "marvell,armada-380-rtc", }, > drivers/thermal/armada_thermal.c: .compatible = "marvell,armada380-thermal", > drivers/usb/host/xhci-plat.c: { .compatible = "marvell,armada-380-xhci"}, > drivers/watchdog/orion_wdt.c: .compatible = "marvell,armada-380-wdt", > > Yes, we're not very consistent between armada380 and armada-380, but > we're not using a38x anywhere. A quick grep for marvell,orion, marvell,kirkwood and marvell,dove suggests it should have the - if we want to be consistent with older Marvell devices. As you said, starting with marvell,armanda it gets pretty inconsistent :-( Andrew -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 12 May 2015 17:54:31 Thomas Petazzoni wrote: > Dear Arnd Bergmann, > > On Tue, 12 May 2015 17:49:08 +0200, Arnd Bergmann wrote: > > > Using > > > > if (IS_ENABLED(__BIG_ENDIAN)) > > > > here would make it more readable by avoiding the #if. Alternatively, > > you could leave the XOR_DESCRIPTOR_SWAP flag disabled and just swap > > the descriptors manually like a lot of other drivers do. You have > > to swap the mmio accesses anywya. > > Agreed on IS_ENABLED(). However, I don't understand the comment about > the need to swap mmio accesses? We're using readl_relaxed() / > writel_relaxed(), and they do the swapping already. Am I missing > something? > Sorry for being unclear. This was exactly my point: readl_relaxed() has to do swaps internally, and you can also do swaps when accessing the descriptor in memory. Arnd -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrew, On Tue, 12 May 2015 17:58:24 +0200, Andrew Lunn wrote: > A quick grep for marvell,orion, marvell,kirkwood and marvell,dove > suggests it should have the - if we want to be consistent with older > Marvell devices. As you said, starting with marvell,armanda it gets > pretty inconsistent :-( Yes, with the "-" would be better IMO. Thomas
Hi Arnd, On Tue, May 12, 2015 at 05:49:08PM +0200, Arnd Bergmann wrote: > On Tuesday 12 May 2015 17:37:37 Maxime Ripard wrote: > > From: Lior Amsalem <alior@marvell.com> > > > > The Marvell Armada 38x SoC introduce new features to the XOR engine, > > especially the fact that the engine mode (MEMCPY/XOR/PQ/etc) can be part of > > the descriptor and not set through the controller registers. > > > > This new feature allows mixing of different commands (even PQ) on the same > > channel/chain without the need to stop the engine to reconfigure the engine > > mode. > > > > Refactor the driver to be able to use that new feature on the Armada 38x, > > while keeping the old behaviour on the older SoCs. > > > > Signed-off-by: Lior Amsalem <alior@marvell.com> > > Reviewed-by: Ofer Heifetz <oferh@marvell.com> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > Two minimal style comments: > > > +static void mv_chan_set_mode_to_desc(struct mv_xor_chan *chan) > > +{ > > + u32 op_mode; > > + u32 config = readl_relaxed(XOR_CONFIG(chan)); > > + > > + op_mode = XOR_OPERATION_MODE_IN_DESC; > > + > > + config &= ~0x7; > > + config |= op_mode; > > + > > +#if defined(__BIG_ENDIAN) > > + config |= XOR_DESCRIPTOR_SWAP; > > +#else > > + config &= ~XOR_DESCRIPTOR_SWAP; > > +#endif > > + > > + writel_relaxed(config, XOR_CONFIG(chan)); > > +} > > Using > > if (IS_ENABLED(__BIG_ENDIAN)) > > here would make it more readable by avoiding the #if. Indeed. I'll change that. > Alternatively, you could leave the XOR_DESCRIPTOR_SWAP flag disabled > and just swap the descriptors manually like a lot of other drivers > do. You have to swap the mmio accesses anywya. That won't be easily doable however. Not only the endianness of the individual fields in the descriptor changes, but changing the endianness also swaps the fields themselves by pair of u32. You can see that here: http://lxr.free-electrons.com/source/drivers/dma/mv_xor.h#L159 So I'm guessing that leaving it like it is was the more readable solution. > > } > > > > +#ifdef CONFIG_OF > > +static const struct of_device_id mv_xor_dt_ids[] = { > > + { .compatible = "marvell,orion-xor", .data = (void *)XOR_MODE_IN_REG }, > > + { .compatible = "marvell,a38x-xor", .data = (void *)XOR_MODE_IN_DESC }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, mv_xor_dt_ids); > > +#endif > > + > > Just leave out the #ifdef here. Almost all the mvebu machines use DT now, > so it's not worth the size benefit of leaving it out on the few machines > that don't. > > You'll have to remove the of_match_ptr() invocation as well if you do that, > to avoid a warning about an unused symbol. Will do. Thanks! Maxime
On Tue, May 12, 2015 at 06:05:39PM +0200, Thomas Petazzoni wrote: > Andrew, > > On Tue, 12 May 2015 17:58:24 +0200, Andrew Lunn wrote: > > > A quick grep for marvell,orion, marvell,kirkwood and marvell,dove > > suggests it should have the - if we want to be consistent with older > > Marvell devices. As you said, starting with marvell,armanda it gets > > pretty inconsistent :-( > > Yes, with the "-" would be better IMO. changed to armada-380-xor, thanks! Maxime
On Wednesday 13 May 2015 10:15:35 Maxime Ripard wrote: > > Alternatively, you could leave the XOR_DESCRIPTOR_SWAP flag disabled > > and just swap the descriptors manually like a lot of other drivers > > do. You have to swap the mmio accesses anywya. > > That won't be easily doable however. > > Not only the endianness of the individual fields in the descriptor > changes, but changing the endianness also swaps the fields themselves > by pair of u32. > > You can see that here: > http://lxr.free-electrons.com/source/drivers/dma/mv_xor.h#L159 > > So I'm guessing that leaving it like it is was the more readable > solution. I see, yes that would get a bit ugly. Arnd -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/dma/mv-xor.txt b/Documentation/devicetree/bindings/dma/mv-xor.txt index 7c6cb7fcecd2..5b10f9ee0937 100644 --- a/Documentation/devicetree/bindings/dma/mv-xor.txt +++ b/Documentation/devicetree/bindings/dma/mv-xor.txt @@ -1,7 +1,7 @@ * Marvell XOR engines Required properties: -- compatible: Should be "marvell,orion-xor" +- compatible: Should be "marvell,orion-xor" or "marvell,a38x-xor" - reg: Should contain registers location and length (two sets) the first set is the low registers, the second set the high registers for the XOR engine. diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c index 1affafa888c5..bcc9aec0ce02 100644 --- a/drivers/dma/mv_xor.c +++ b/drivers/dma/mv_xor.c @@ -19,6 +19,7 @@ #include <linux/dma-mapping.h> #include <linux/spinlock.h> #include <linux/interrupt.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/memory.h> #include <linux/clk.h> @@ -30,6 +31,11 @@ #include "dmaengine.h" #include "mv_xor.h" +enum mv_xor_mode { + XOR_MODE_IN_REG, + XOR_MODE_IN_DESC, +}; + static void mv_xor_issue_pending(struct dma_chan *chan); #define to_mv_xor_chan(chan) \ @@ -56,6 +62,24 @@ static void mv_desc_init(struct mv_xor_desc_slot *desc, hw_desc->byte_count = byte_count; } +static void mv_desc_set_mode(struct mv_xor_desc_slot *desc) +{ + struct mv_xor_desc *hw_desc = desc->hw_desc; + + switch (desc->type) { + case DMA_XOR: + case DMA_INTERRUPT: + hw_desc->desc_command |= XOR_DESC_OPERATION_XOR; + break; + case DMA_MEMCPY: + hw_desc->desc_command |= XOR_DESC_OPERATION_MEMCPY; + break; + default: + BUG(); + return; + } +} + static void mv_desc_set_next_desc(struct mv_xor_desc_slot *desc, u32 next_desc_addr) { @@ -154,6 +178,25 @@ static void mv_chan_set_mode(struct mv_xor_chan *chan, chan->current_type = type; } +static void mv_chan_set_mode_to_desc(struct mv_xor_chan *chan) +{ + u32 op_mode; + u32 config = readl_relaxed(XOR_CONFIG(chan)); + + op_mode = XOR_OPERATION_MODE_IN_DESC; + + config &= ~0x7; + config |= op_mode; + +#if defined(__BIG_ENDIAN) + config |= XOR_DESCRIPTOR_SWAP; +#else + config &= ~XOR_DESCRIPTOR_SWAP; +#endif + + writel_relaxed(config, XOR_CONFIG(chan)); +} + static void mv_chan_activate(struct mv_xor_chan *chan) { dev_dbg(mv_chan_to_devp(chan), " activate chan.\n"); @@ -510,6 +553,8 @@ mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src, sw_desc->type = DMA_XOR; sw_desc->async_tx.flags = flags; 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--) mv_desc_set_src_addr(sw_desc, src_cnt, src[src_cnt]); } @@ -952,7 +997,7 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan) static struct mv_xor_chan * mv_xor_channel_add(struct mv_xor_device *xordev, struct platform_device *pdev, - int idx, dma_cap_mask_t cap_mask, int irq) + int idx, dma_cap_mask_t cap_mask, int irq, int op_in_desc) { int ret = 0; struct mv_xor_chan *mv_chan; @@ -964,6 +1009,7 @@ mv_xor_channel_add(struct mv_xor_device *xordev, mv_chan->idx = idx; mv_chan->irq = irq; + mv_chan->op_in_desc = op_in_desc; dma_dev = &mv_chan->dmadev; @@ -1024,7 +1070,10 @@ mv_xor_channel_add(struct mv_xor_device *xordev, mv_chan_unmask_interrupts(mv_chan); - mv_chan_set_mode(mv_chan, DMA_XOR); + if (mv_chan->op_in_desc == XOR_MODE_IN_DESC) + mv_chan_set_mode_to_desc(mv_chan); + else + mv_chan_set_mode(mv_chan, DMA_XOR); spin_lock_init(&mv_chan->lock); INIT_LIST_HEAD(&mv_chan->chain); @@ -1049,7 +1098,8 @@ mv_xor_channel_add(struct mv_xor_device *xordev, goto err_free_irq; } - dev_info(&pdev->dev, "Marvell XOR: ( %s%s%s)\n", + dev_info(&pdev->dev, "Marvell XOR (%s): ( %s%s%s)\n", + mv_chan->op_in_desc ? "Descriptor Mode" : "Registers Mode", dma_has_cap(DMA_XOR, dma_dev->cap_mask) ? "xor " : "", dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask) ? "cpy " : "", dma_has_cap(DMA_INTERRUPT, dma_dev->cap_mask) ? "intr " : ""); @@ -1098,6 +1148,15 @@ mv_xor_conf_mbus_windows(struct mv_xor_device *xordev, writel(0, base + WINDOW_OVERRIDE_CTRL(1)); } +#ifdef CONFIG_OF +static const struct of_device_id mv_xor_dt_ids[] = { + { .compatible = "marvell,orion-xor", .data = (void *)XOR_MODE_IN_REG }, + { .compatible = "marvell,a38x-xor", .data = (void *)XOR_MODE_IN_DESC }, + {}, +}; +MODULE_DEVICE_TABLE(of, mv_xor_dt_ids); +#endif + static int mv_xor_probe(struct platform_device *pdev) { const struct mbus_dram_target_info *dram; @@ -1105,6 +1164,7 @@ static int mv_xor_probe(struct platform_device *pdev) struct mv_xor_platform_data *pdata = dev_get_platdata(&pdev->dev); struct resource *res; int i, ret; + int op_in_desc; dev_notice(&pdev->dev, "Marvell shared XOR driver\n"); @@ -1149,11 +1209,15 @@ static int mv_xor_probe(struct platform_device *pdev) if (pdev->dev.of_node) { struct device_node *np; int i = 0; + const struct of_device_id *of_id = + of_match_device(of_match_ptr(mv_xor_dt_ids), + &pdev->dev); for_each_child_of_node(pdev->dev.of_node, np) { struct mv_xor_chan *chan; dma_cap_mask_t cap_mask; int irq; + op_in_desc = (int)of_id->data; dma_cap_zero(cap_mask); if (of_property_read_bool(np, "dmacap,memcpy")) @@ -1170,7 +1234,7 @@ static int mv_xor_probe(struct platform_device *pdev) } chan = mv_xor_channel_add(xordev, pdev, i, - cap_mask, irq); + cap_mask, irq, op_in_desc); if (IS_ERR(chan)) { ret = PTR_ERR(chan); irq_dispose_mapping(irq); @@ -1199,7 +1263,8 @@ static int mv_xor_probe(struct platform_device *pdev) } chan = mv_xor_channel_add(xordev, pdev, i, - cd->cap_mask, irq); + cd->cap_mask, irq, + XOR_MODE_IN_REG); if (IS_ERR(chan)) { ret = PTR_ERR(chan); goto err_channel_add; @@ -1245,14 +1310,6 @@ static int mv_xor_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_OF -static const struct of_device_id mv_xor_dt_ids[] = { - { .compatible = "marvell,orion-xor", }, - {}, -}; -MODULE_DEVICE_TABLE(of, mv_xor_dt_ids); -#endif - static struct platform_driver mv_xor_driver = { .probe = mv_xor_probe, .remove = mv_xor_remove, diff --git a/drivers/dma/mv_xor.h b/drivers/dma/mv_xor.h index 91958dba39a2..89a8c85539a1 100644 --- a/drivers/dma/mv_xor.h +++ b/drivers/dma/mv_xor.h @@ -30,8 +30,13 @@ /* Values for the XOR_CONFIG register */ #define XOR_OPERATION_MODE_XOR 0 #define XOR_OPERATION_MODE_MEMCPY 2 +#define XOR_OPERATION_MODE_IN_DESC 7 #define XOR_DESCRIPTOR_SWAP BIT(14) +#define XOR_DESC_OPERATION_XOR (0 << 24) +#define XOR_DESC_OPERATION_CRC32C (1 << 24) +#define XOR_DESC_OPERATION_MEMCPY (2 << 24) + #define XOR_DESC_DMA_OWNED BIT(31) #define XOR_DESC_EOD_INT_EN BIT(31) @@ -95,6 +100,7 @@ struct mv_xor_device { * @all_slots: complete domain of slots usable by the channel * @slots_allocated: records the actual size of the descriptor slot pool * @irq_tasklet: bottom half where mv_xor_slot_cleanup runs + * @op_in_desc: new mode of driver, each op is writen to descriptor. */ struct mv_xor_chan { int pending; @@ -115,6 +121,7 @@ struct mv_xor_chan { struct list_head all_slots; int slots_allocated; struct tasklet_struct irq_tasklet; + int op_in_desc; char dummy_src[MV_XOR_MIN_BYTE_COUNT]; char dummy_dst[MV_XOR_MIN_BYTE_COUNT]; dma_addr_t dummy_src_addr, dummy_dst_addr;