On Thu, Apr 24, 2014 at 04:22:44PM +0200, Maxime Ripard wrote:
> The Allwinner A31 has a 16 channels DMA controller that it shares with the
> newer A23. Although sharing some similarities with the DMA controller of the
> older Allwinner SoCs, it's significantly different, I don't expect it to be
> possible to share the driver for these two.
>
> The A31 Controller is able to memory-to-memory or memory-to-device transfers on
> the 16 channels in parallel.
Overall driver looks in good shape, few comments though...
> +This driver follows the generic DMA bindings defined in dma.txt.
> +
> +Required properties:
> +
> +- compatible: Must be "allwinner,sun6i-a31-dma"
> +- reg: Should contain the registers base address and length
> +- interrupts: Should contain a reference to the interrupt used by this device
> +- clocks: Should contain a reference to the parent AHB clock
> +- resets: Should contain a reference to the reset controller asserting
> + this device in reset
> +- #dma-cells : Should be 1, a single cell holding a line request number
> +
> +Example:
> + dma: dma-controller@01c02000 {
> + compatible = "allwinner,sun6i-a31-dma";
> + reg = <0x01c02000 0x1000>;
> + interrupts = <0 50 4>;
> + clocks = <&ahb1_gates 6>;
> + resets = <&ahb1_rst 6>;
> + #dma-cells = <1>;
> + };
> +
> +Clients:
> +
> +DMA clients connected to the A31 DMA controller must use the format
> +described in the dma.txt file, using a two-cell specifier for each
> +channel: a phandle plus one integer cells.
> +The two cells in order are:
> +
> +1. A phandle pointing to the DMA controller.
> +2. The port ID as specified in the datasheet
> +
> +Example:
> +spi2: spi@01c6a000 {
> + compatible = "allwinner,sun6i-a31-spi";
> + reg = <0x01c6a000 0x1000>;
> + interrupts = <0 67 4>;
> + clocks = <&ahb1_gates 22>, <&spi2_clk>;
> + clock-names = "ahb", "mod";
> + dmas = <&dma 25>, <&dma 25>;
> + dma-names = "rx", "tx";
> + resets = <&ahb1_rst 22>;
> +};
Ideally binding should be a separate patch
> +static inline u8 convert_burst(u8 maxburst)
> +{
> + if (maxburst == 1 || maxburst > 16)
> + return 0;
are these valid configurations?
> +
> + return fls(maxburst) - 1;
> +}
> +
> +static inline u8 convert_buswidth(enum dma_slave_buswidth addr_width)
> +{
> + switch (addr_width) {
> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
> + return 1;
> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
> + return 2;
return (addr_width >> 1); ..??
since DMA_SLAVE_BUSWIDTH_2_BYTES is numeric 2 and DMA_SLAVE_BUSWIDTH_4_BYTES
numeric 4.
> + default:
> + return 0;
error?
> +static inline void sun6i_dma_cfg_lli(struct sun6i_dma_lli *lli,
> + dma_addr_t src,
> + dma_addr_t dst, u32 len,
> + struct dma_slave_config *config)
> +{
> + u32 src_width, dst_width, src_burst, dst_burst;
> +
> + if (!config)
> + return;
> +
> + src_burst = convert_burst(config->src_maxburst);
> + dst_burst = convert_burst(config->dst_maxburst);
> +
> + src_width = convert_buswidth(config->src_addr_width);
> + dst_width = convert_buswidth(config->dst_addr_width);
is 0 a valid case then?
> +
> +static int sun6i_dma_terminate_all(struct sun6i_vchan *vchan)
> +{
> + struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(vchan->vc.chan.device);
> + struct sun6i_pchan *pchan = vchan->phy;
> + unsigned long flags;
> + LIST_HEAD(head);
> +
> + spin_lock(&sdev->lock);
> + list_del_init(&vchan->node);
> + spin_unlock(&sdev->lock);
> +
> + spin_lock_irqsave(&vchan->vc.lock, flags);
> +
> + vchan_get_all_descriptors(&vchan->vc, &head);
> +
> + if (pchan) {
> + writel(DMA_CHAN_ENABLE_STOP, pchan->base + DMA_CHAN_ENABLE);
> + writel(DMA_CHAN_PAUSE_RESUME, pchan->base + DMA_CHAN_PAUSE);
> +
> + vchan->phy = NULL;
> + pchan->vchan = NULL;
> + pchan->desc = NULL;
> + pchan->done = NULL;
> + }
> +
> + spin_unlock_irqrestore(&vchan->vc.lock, flags);
> +
> + vchan_dma_desc_free_list(&vchan->vc, &head);
shouldn't you kill the tasklet as well here?
> +static inline void sun6i_dma_free(struct sun6i_dma_dev *sdc)
> +{
> + int i;
> +
> + for (i = 0; i < NR_MAX_VCHANS; i++) {
> + struct sun6i_vchan *vchan = &sdc->vchans[i];
> +
> + list_del(&vchan->vc.chan.device_node);
> + tasklet_kill(&vchan->vc.task);
> + }
> +
> + tasklet_kill(&sdc->task);
This is again not good. see http://lwn.net/Articles/588457/
At this point HW can still generate interrupts or you can have irq running!
> +
> +static int sun6i_dma_probe(struct platform_device *pdev)
> +{
> + struct sun6i_dma_dev *sdc;
> + struct resource *res;
> + struct clk *mux, *pll6;
> + int irq;
> + int ret, i;
> +
> + sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
> + if (!sdc)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + sdc->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(sdc->base))
> + return PTR_ERR(sdc->base);
> +
> + irq = platform_get_irq(pdev, 0);
> + ret = devm_request_irq(&pdev->dev, irq, sun6i_dma_interrupt, 0,
> + dev_name(&pdev->dev), sdc);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot request IRQ\n");
> + return ret;
> + }
this is not good. You have registered for irq, so your irq handlers can be
invoked but you initialization is not complete yet.
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
