RE: [PATCH 2/2] dma: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Vinod,

Thanks for the review.

> -----Original Message-----
> From: dmaengine-owner@xxxxxxxxxxxxxxx [mailto:dmaengine-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Vinod Koul
> Sent: Friday, January 12, 2018 6:14 AM
> To: Hyun Kwon <hyunk@xxxxxxxxxx>
> Cc: dmaengine@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Michal Simek
> <michal.simek@xxxxxxxxxx>; Tejas Upadhyay <TEJASU@xxxxxxxxxx>
> Subject: Re: [PATCH 2/2] dma: xilinx: dpdma: Add the Xilinx DisplayPort DMA
> engine driver
> 
> On Fri, Jan 05, 2018 at 06:14:08PM -0800, Hyun Kwon wrote:
> > The ZynqMP includes the DisplayPort subsystem with own DMA engine
> > called DPDMA. The DPDMA IP comes with 6 individual channels
> > (4 for display, 2 for audio). This driver implements DMAENGINE API
> > which can be used for audio (ALSA) and display (DRM) drivers.
> 
> The subsystem name is dmaengine.
> 
> > Signed-off-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> > Signed-off-by: Tejas Upadhyay <tejasu@xxxxxxxxxx>
> > Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> > ---
> >  MAINTAINERS                       |    7 +
> >  drivers/dma/Kconfig               |   19 +
> >  drivers/dma/xilinx/Makefile       |    1 +
> >  drivers/dma/xilinx/xilinx_dpdma.c | 2309
> +++++++++++++++++++++++++++++++++++++
> 
> That is a very long file for review! Pls split it up to reasonable logical
> chunks

Sure. Will split.

> 
> > +config XILINX_DPDMA_DEBUG_FS
> > +        bool "Xilinx DPDMA debugfs"
> > +        depends on DEBUG_FS && XILINX_DPDMA
> > +        help
> > +          Enable the debugfs code for DPDMA driver. The debugfs code
> > +          enables debugging or testing related features. It exposes some
> > +          low level controls to the user space to help testing automation,
> > +          as well as can enable additional diagnostic or statistical
> > +          information.
> 
> why should this be a compile option
> 

This debugfs code is for testing / regression, and we don't want to enable it
for regular users.

> > + * Xilinx ZynqMP DPDMA Engine driver
> > + *
> > + *  Copyright (C) 2015 - 2018 Xilinx, Inc.
> > + *
> > + *  Author: Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> > + *
> > + * SPDX-License-Identifier: GPL-2.0
> 
> this should be first line in file with c99 style,
> 
> // SPDX-License-Identifier: GPL-2.0
> 

Agreed. Will change.

> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dmapool.h>
> > +#include <linux/gfp.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_dma.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sched.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/wait.h>
> 
> do you need all these headers?
> 

I directly included all headers that are used in this driver. Some of them can
be removed from indirect inclusions, and I'm fine with that. Please let me know
if that's your preference.

> > +
> > +#include "../dmaengine.h"
> > +
> > +/* DPDMA registers */
> > +#define XILINX_DPDMA_ERR_CTRL				0x0
> > +#define XILINX_DPDMA_ISR				0x4
> > +#define XILINX_DPDMA_IMR				0x8
> > +#define XILINX_DPDMA_IEN				0xc
> > +#define XILINX_DPDMA_IDS				0x10
> > +#define XILINX_DPDMA_INTR_DESC_DONE_MASK		(0x3f << 0)
> 
> GENMASK() for this and others
> 

Agreed. Will fix.

> > +#define XILINX_DPDMA_INTR_DESC_DONE_SHIFT		0
> 
> you can define a common shift using ffs
> 

I guess you mean to replace, (value & MASK) << SHIFT, with
(value & MASK) << ffs(MASK). I'll change to that way. Let me know otherwise.

> > + * struct xilinx_dpdma_hw_desc - DPDMA hardware descriptor
> > + * @control: control configuration field
> > + * @desc_id: descriptor ID
> > + * @xfer_size: transfer size
> > + * @hsize_stride: horizontal size and stride
> > + * @timestamp_lsb: LSB of time stamp
> > + * @timestamp_msb: MSB of time stamp
> > + * @addr_ext: upper 16 bit of 48 bit address (next_desc and src_addr)
> > + * @next_desc: next descriptor 32 bit address
> > + * @src_addr: payload source address (lower 32 bit of 1st 4KB page)
> > + * @addr_ext_23: upper 16 bit of 48 bit address (src_addr2 and src_addr3)
> > + * @addr_ext_45: upper 16 bit of 48 bit address (src_addr4 and src_addr5)
> > + * @src_addr2: payload source address (lower 32 bit of 2nd 4KB page)
> > + * @src_addr3: payload source address (lower 32 bit of 3rd 4KB page)
> > + * @src_addr4: payload source address (lower 32 bit of 4th 4KB page)
> > + * @src_addr5: payload source address (lower 32 bit of 5th 4KB page)
> 
> what does it mean (lower 32 bit of Nth 4KB page)
> 

Each descriptor can point up to 5 - 48bit address payloads. src_addr* fields
contain lower 32bit of 48bit address. Remaining upper 16bit is programmed in
addr_ext* fields.

> > +struct xilinx_dpdma_sw_desc {
> > +	struct xilinx_dpdma_hw_desc hw;
> > +	struct list_head node;
> > +	dma_addr_t phys;
> 
> dma_addr_t is not a physical addr
> 

Agreed. Will rename.

> > +struct xilinx_dpdma_device {
> > +	struct dma_device common;
> > +	void __iomem *reg;
> > +	struct device *dev;
> > +
> > +	struct clk *axi_clk;
> > +	struct xilinx_dpdma_chan *chan[XILINX_DPDMA_NUM_CHAN];
> > +
> > +	bool ext_addr;
> > +	void (*desc_addr)(struct xilinx_dpdma_sw_desc *sw_desc,
> > +			  struct xilinx_dpdma_sw_desc *prev,
> > +			  dma_addr_t dma_addr[], unsigned int num_src_addr);
> > +};
> > +
> > +#ifdef CONFIG_XILINX_DPDMA_DEBUG_FS
> 
> this can be CONFIG_DEBUGFS, also am skipping this part to focus on driver.
> Pls split this to a separate patch
> 

The intention was to enable debugfs code of this module individually. I'll
split into a separate patch, so it can be reviewed better.

> > +static int xilinx_dpdma_config(struct dma_chan *dchan,
> > +			       struct dma_slave_config *config)
> > +{
> > +	if (config->direction != DMA_MEM_TO_DEV)
> > +		return -EINVAL;
> 
> ?? Why are the config values not used, how else do you configure the
> channel?
> 

There's not much configuration to be done. Channels are pretty much identical
with fixed configurations.

> > +
> > +	return 0;
> > +}
> > +
> > +static int xilinx_dpdma_pause(struct dma_chan *dchan)
> > +{
> > +	xilinx_dpdma_chan_pause(to_xilinx_chan(dchan));
> > +
> > +	return 0;
> > +}
> > +
> > +static int xilinx_dpdma_resume(struct dma_chan *dchan)
> > +{
> > +	xilinx_dpdma_chan_unpause(to_xilinx_chan(dchan));
> > +
> > +	return 0;
> > +}
> > +
> > +static int xilinx_dpdma_terminate_all(struct dma_chan *dchan)
> > +{
> > +	return xilinx_dpdma_chan_terminate_all(to_xilinx_chan(dchan));
> > +}
> > +
> > +static void xilinx_dpdma_synchronize(struct dma_chan *dchan)
> > +{
> > +	xilinx_dpdma_chan_synchronize(to_xilinx_chan(dchan));
> > +}
> 
> why have these wrappers which do not do anything?
> 

It's just my personal preference to split into different code partitions, and
each section is partitioned / grouped with some comment line. :-)
Ex, a partition for struct dma_chan, and another one for
struct xilinx_dpdma_chan. It gives me sort of abstracted view. But it may be
just me, and it comes with extra indirections. I'll remove unnecessary wrappers.

> > +
> > +	chan->reg = xdev->reg + XILINX_DPDMA_CH_BASE +
> XILINX_DPDMA_CH_OFFSET *
> > +		    chan->id;
> > +	chan->status = IDLE;
> > +
> > +	spin_lock_init(&chan->lock);
> > +	INIT_LIST_HEAD(&chan->done_list);
> > +	init_waitqueue_head(&chan->wait_to_stop);
> > +
> > +	tasklet_init(&chan->done_task, xilinx_dpdma_chan_done_task,
> > +		     (unsigned long)chan);
> > +	tasklet_init(&chan->err_task, xilinx_dpdma_chan_err_task,
> > +		     (unsigned long)chan);
> 
> Have you checked the vchan code, i dont see that used. It will help you in
> descriptor management and reduce code from driver
> 

Thanks for the pointer. I'll take a look.

> > +static int xilinx_dpdma_remove(struct platform_device *pdev)
> > +{
> > +	struct xilinx_dpdma_device *xdev;
> > +	unsigned int i;
> > +
> > +	xdev = platform_get_drvdata(pdev);
> > +
> > +	xilinx_dpdma_disable_intr(xdev);
> > +	of_dma_controller_free(pdev->dev.of_node);
> > +	dma_async_device_unregister(&xdev->common);
> > +	clk_disable_unprepare(xdev->axi_clk);
> > +
> > +	for (i = 0; i < XILINX_DPDMA_NUM_CHAN; i++)
> > +		if (xdev->chan[i])
> > +			xilinx_dpdma_chan_remove(xdev->chan[i]);
> 
> At this point your irq is still enabled and can fire, and can schedule
> tasklet.. Are you sure you are okay with that?
> 

Ok. You mean that an interrupt can occur right before
xilinx_dpdma_disable_intr(), and the interrupt may be handled  in the middle or
after removing this driver. I'll switch to request_irq() from
devm_request_irq(), and remove the irq when removing the driver.

> > +MODULE_AUTHOR("Xilinx, Inc.");
> > +MODULE_DESCRIPTION("Xilinx DPDMA driver");
> > +MODULE_LICENSE("GPL v2");
> 
> No MODULE_ALIAS()?

Is it required? Could you please elaborate, to help my understanding? 

> 
> I haven't reviewed in details though as it is too large patch. Pls use vchan
> and split things up for better review.

Sure will do.

Thanks,
-hyun

> 
> --
> ~Vinod
> --
> 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
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux