Message-Id: <200610021316.18568.david-b@pacbell.net>
Date:	Mon, 2 Oct 2006 13:16:17 -0700
From:	David Brownell <david-b@...bell.net>
To:	"Andrea Paterniani" <a.paterniani@...pp-eng.it>
Cc:	Linux Kernel list <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...l.org>
Subject: Re: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver

On Monday 02 October 2006 10:59 am, Andrew Morton wrote:
> On Mon, 2 Oct 2006 08:16:58 -0700
> David Brownell <david-b@...bell.net> wrote:
> 
> > Subject: SPI controller driver for Freescale iMX
> > From: Andrea Paterniani <a.paterniani@...pp-eng.it>

Andrea, can you provide an updated version of the patch that I sent?

- Dave


> > This patch adds a SPI controller driver for the Freescale i.MX(S/L/1).
> > The code is inspired by pxa2xx_spi driver.  Main features summary:
> >  -  Per chip setup via board specific code and/or protocol driver.
> >  -  Per transfer setup.
> >  -  PIO transfers.
> >  -  DMA transfers.
> >  -  Managing of NULL tx / rx buffer for rd only / wr only transfers.
> > 
> > ...
> >
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/ioport.h>
> > +#include <linux/errno.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/errno.h>
> 
> we already did that include.
> 
> > +
> > +#define DEFINE_SPI_REG_RD(reg, off)			\
> > +	static inline u32 rd_##reg(void __iomem *p)	\
> > +	{						\
> > +		return readl(p + (off));		\
> > +	}
> > +
> > +#define DEFINE_SPI_REG_WR(reg, off)				\
> > +	static inline void wr_##reg(u32 v, void __iomem *p)	\
> > +	{							\
> > +		writel(v, p + (off));				\
> > +	}
> > +
> > +DEFINE_SPI_REG_RD(DATA, 0x00)
> > +DEFINE_SPI_REG_WR(DATA, 0x04)
> > +DEFINE_SPI_REG_RD(CONTROL, 0x08)
> > +DEFINE_SPI_REG_WR(CONTROL, 0x08)
> > +DEFINE_SPI_REG_RD(INT_STATUS, 0x0C)
> > +DEFINE_SPI_REG_WR(INT_STATUS, 0x0C)
> > +DEFINE_SPI_REG_RD(TEST, 0x10)
> > +DEFINE_SPI_REG_WR(TEST, 0x10)
> > +DEFINE_SPI_REG_RD(PERIOD, 0x14)
> > +DEFINE_SPI_REG_WR(PERIOD, 0x14)
> > +DEFINE_SPI_REG_RD(DMA, 0x18)
> > +DEFINE_SPI_REG_WR(DMA, 0x18)
> > +DEFINE_SPI_REG_WR(RESET, 0x1C)
> 
> ug.  Why not simply open-code
> 
> 	readl(addr + DATA);
> 
> ?
> 
> > +
> > +#define PRINT_DMA_GLOBAL_REGS(dev)	\
> > +	dev_dbg(dev,			\
> > +		"DMA_GLOBAL\n"		\
> > +		"    DCR    = 0x%08X\n"	\
> > +		"    DISR   = 0x%08X\n"	\
> > +		"    DIMR   = 0x%08X\n"	\
> > +		"    DBTOSR = 0x%08X\n"	\
> > +		"    DRTOSR = 0x%08X\n"	\
> > +		"    DSESR  = 0x%08X\n"	\
> > +		"    DBOSR  = 0x%08X\n"	\
> > +		"    DBTOCR = 0x%08X\n",\
> > +		DCR,			\
> > +		DISR,			\
> > +		DIMR,			\
> > +		DBTOSR,			\
> > +		DRTOSR,			\
> > +		DSESR,			\
> > +		DBOSR,			\
> > +		DBTOCR)
> 
> Unless the callsites have been cunningly hidden inside even more macros,
> this thankfully has no users and can be removed.
> 
> > +#define PRINT_DMA_CH_REGS(dev, channel)	\
> > +	dev_dbg(dev,			\
> > +		"DMA(%d)\n"		\
> > +		"    SAR    = 0x%08X\n"	\
> > +		"    DAR    = 0x%08X\n"	\
> > +		"    CNTR   = 0x%08X\n"	\
> > +		"    CCR    = 0x%08X\n"	\
> > +		"    RSSR   = 0x%08X\n"	\
> > +		"    BLR    = 0x%08X\n"	\
> > +		"    RTOR   = 0x%08X\n"	\
> > +		"    BUCR   = 0x%08X\n",\
> > +		channel,		\
> > +		SAR(channel),		\
> > +		DAR(channel),		\
> > +		CNTR(channel),		\
> > +		CCR(channel),		\
> > +		RSSR(channel),		\
> > +		BLR(channel),		\
> > +		RTOR(channel),		\
> > +		BUCR(channel))
> 
> ditto
> 
> > +#define PRINT_SPI_REGS(dev, regs)		\
> > +	dev_dbg(dev,				\
> > +		"SPI_REGS\n"			\
> > +		"    CONTROL    = 0x%08X\n"	\
> > +		"    INT_STATUS = 0x%08X\n"	\
> > +		"    TEST       = 0x%08X\n"	\
> > +		"    PERIOD     = 0x%08X\n"	\
> > +		"    DMA        = 0x%08X\n",	\
> > +		rd_CONTROL(regs),		\
> > +		rd_INT_STATUS(regs),		\
> > +		rd_TEST(regs),			\
> > +		rd_PERIOD(regs),			\
> > +		rd_DMA(regs))
> 
> tritto.

My lessions in Italian are progressing apace.  ;)


> > +static int flush(struct driver_data *drv_data)
> > +{
> > +	unsigned long limit = loops_per_jiffy << 1;
> > +	void __iomem *regs = drv_data->regs;
> > +	volatile u32 d;
> > +
> > +	dev_dbg(&drv_data->pdev->dev, "flush\n");
> > +
> > +	do {
> > +		while (rd_INT_STATUS(regs) & SPI_STATUS_RR)
> > +			d = rd_DATA(regs);
> > +	} while ((rd_CONTROL(regs) & SPI_CONTROL_XCH) && limit--);
> > +
> > +	return limit;
> > +}
> 
> The use of loops_per_jiffy seems inappropriate.  That's an IO-space read in
> there, which is slow.  This timeout will be very long indeed.
> 
> > +static int dummy_writer(struct driver_data *drv_data)
> > +{
> > +	void __iomem *regs = drv_data->regs;
> > +	u8 *tx, *tx_end;
> > +	u32 remaining_data;
> > +	u32 fifo_avail_space;
> > +	u32 n;
> > +
> > +	/* Compute how many fifo writes to do */
> > +	tx = (u8*)drv_data->tx;
> > +	tx_end = (u8*)drv_data->tx_end;
> 
> Two unneeded casts.
> 
> > +	remaining_data = (u32)(tx_end - tx) / drv_data->n_bytes;
> 
> hm, we just wrote an inline function to do that, then didn't use it.
> 
> > +static int u8_writer(struct driver_data *drv_data)
> > +{
> > +	void __iomem *regs = drv_data->regs;
> > +	u8 *tx, *tx_end;
> > +	u32 remaining_data;
> > +	u32 fifo_avail_space;
> > +	u32 n;
> > +
> > +	/* Compute how many fifo writes to do */
> > +	tx = (u8*)drv_data->tx;
> > +	tx_end = (u8*)drv_data->tx_end;
> 
> Unneeded casts
> 
> > +	remaining_data = (u32)(tx_end - tx);
> 
> How come we didn't divide by drv_data->n_bytes this time?
> 
> > +static int u8_reader(struct driver_data *drv_data)
> > +{
> > +	void __iomem *regs = drv_data->regs;
> > +	u8 *rx, *rx_end;
> > +	u32 remaining_data;
> > +	u32 fifo_rxcnt;
> > +	u32 n;
> > +
> > +	/* Compute how many fifo reads to do */
> > +	rx = (u8*)drv_data->rx;
> > +	rx_end = (u8*)drv_data->rx_end;
> 
> casts
> 
> > +	remaining_data = (u32)(rx_end - rx);
> 
> Missing divide?
> 
> > +static int u16_writer(struct driver_data *drv_data)
> > +{
> > +	void __iomem *regs = drv_data->regs;
> > +	u16 *tx, *tx_end;
> > +	u32 remaining_data;
> > +	u32 fifo_avail_space;
> > +	u32 n;
> > +
> > +	/* Compute how many fifo writes to do */
> > +	tx = (u16*)drv_data->tx;
> > +	tx_end = (u16*)drv_data->tx_end;
> > +	remaining_data = (u32)(tx_end - tx);
> 
> ditto
> 
> > +static int u16_reader(struct driver_data *drv_data)
> > +{
> > +	struct spi_regs __iomem *regs;
> > +	u16 *rx, *rx_end;
> > +	u32 remaining_data;
> > +	u32 fifo_rxcnt;
> > +	u32 n;
> > +
> > +	regs = drv_data->regs;
> > +
> > +	/* Compute how many fifo reads to do */
> > +	rx = (u16*)drv_data->rx;
> > +	rx_end = (u16*)drv_data->rx_end;
> 
> This code's awfully repetitive.  Can it be consolidated?
> 
> > +	remaining_data = (u32)(rx_end - rx);
> > +	fifo_rxcnt = (rd_TEST(regs) & SPI_TEST_RXCNT) >> SPI_TEST_RXCNT_LSB;
> > +	n = min(remaining_data, fifo_rxcnt);
> > +	dev_dbg(&drv_data->pdev->dev,
> > +		"u16_reader\n"
> > +		"    remaining data = %d\n"
> > +		"    fifo_rxcnt     = %d\n"
> > +		"    fifo reads     = %d\n",
> > +		remaining_data,
> > +		fifo_rxcnt,
> > +		n);
> > +
> > +	if (n > 0) {
> > +		/* Read SPI RXFIFO */
> > +		while (n--)
> > +			*rx++ = rd_DATA(regs);
> > +
> > +		/* Update rx pointer */
> > +		drv_data->rx = rx;
> > +	}
> > +
> > +	return (rx >= rx_end);
> > +}
> > +
> > +static void *next_transfer(struct driver_data *drv_data)
> > +{
> > +	struct spi_message *msg = drv_data->cur_msg;
> > +	struct spi_transfer *trans = drv_data->cur_transfer;
> > +
> > +	/* Move to next transfer */
> > +	if (trans->transfer_list.next != &msg->transfers) {
> > +		drv_data->cur_transfer =
> > +			list_entry(trans->transfer_list.next,
> > +					struct spi_transfer,
> > +					transfer_list);
> > +		return RUNNING_STATE;
> > +	}
> > +
> > +	return DONE_STATE;
> > +}
> 
> Why does it use void*'s for this enumeration?
> 
> > +				dev_err(&drv_data->pdev->dev,
> > +					"interrupt_transfer - "
> > +					"trailing byte read failed\n");
> > +			else
> > +				dev_dbg(&drv_data->pdev->dev,
> > +					"interrupt_transfer - end of rx\n");
> > +
> > +			/* End of transfer, update total byte transfered */
> > +			msg->actual_length += drv_data->len;
> > +
> > +			/* Release chip select if requested, transfer delays are
> > +			   handled in pump_transfers */
> > +			if (drv_data->cs_change)
> > +				drv_data->cs_control(SPI_CS_DEASSERT);
> > +
> > +			/* Move to next transfer */
> > +			msg->state = next_transfer(drv_data);
> > +
> > +			/* Schedule transfer tasklet */
> > +			tasklet_schedule(&drv_data->pump_transfers);
> 
> I see tasklets being scheduled, but no tasklet_disable() or tasklet_kill(),
> etc.  Is this driver racy against shutdown or rmmod?
> 
> > +			return IRQ_HANDLED;
> > +		}
> > +
> > +		status = rd_INT_STATUS(regs);
> > +
> > +		/* We did something */
> > +		handled = IRQ_HANDLED;
> > +	}
> > +
> > +	return handled;
> > +}
> > +
> > +static irqreturn_t spi_int(int irq, void *dev_id, struct pt_regs *regs)
> > +{
> > +	struct driver_data *drv_data = (struct driver_data *)dev_id;
> > +
> > +	if (!drv_data->cur_msg) {
> > +		dev_err(&drv_data->pdev->dev,
> > +			"spi_int - bad message state\n");
> > +		/* Never fail */
> > +		return IRQ_HANDLED;
> 
> IRQ_NONE?
> 
> > +	}
> > +
> > +	return drv_data->transfer_handler(drv_data);
> > +}
> > +
> >
> > ..
> >
> > +	if ((drv_data->n_bytes == 2) &&
> > +		(drv_data->len > SPI_FIFO_DEPTH*SPI_FIFO_BYTE_WIDTH) &&
> > +		map_dma_buffers(drv_data)) {
> > +		dev_dbg(&drv_data->pdev->dev,
> > +			"pump dma transfer\n"
> > +			"    tx      = 0x%08X\n"
> > +			"    tx_dma  = 0x%08X\n"
> > +			"    rx      = 0x%08X\n"
> > +			"    rx_dma  = 0x%08X\n"
> > +			"    len     = %d\n",
> > +			(u32)drv_data->tx,
> > +			(u32)drv_data->tx_dma,
> > +			(u32)drv_data->rx,
> > +			(u32)drv_data->rx_dma,
> > +			(u32)drv_data->len);
> 
> The way to print a pointer is with %p, not with a cast to u32.
> 
> Also, it's incorrect (but it happens to work) to print u32's with %X.  %X
> is defined on ints and unsigneds - you don't know what type the
> architecture actually chose to use.  It could have used unsigned long.
> 
> So if one insists on casting pointers here, cast them to `unsigned'.
> 
> But %p would be better.
> 
> > +		/* Ensure we have the correct interrupt handler */
> > +		drv_data->transfer_handler = dma_transfer;
> > +
> > +		/* Enable SPI and arm transfer */
> > +		wr_CONTROL(rd_CONTROL(regs) |
> > +				SPI_CONTROL_SPIEN | SPI_CONTROL_XCH,
> > +				regs);
> > +
> > +		/* Setup tx DMA */
> > +		if (drv_data->tx)
> > +			/* Linear source address */
> > +			CCR(drv_data->tx_channel) =
> > +				CCR_DMOD_FIFO |
> > +				CCR_SMOD_LINEAR |
> > +				CCR_SSIZ_32 | CCR_DSIZ_16 |
> > +				CCR_REN;
> > +		else
> > +			/* Read only transfer -> fixed source address for
> > +			   dummy write to achive read */
> > +			CCR(drv_data->tx_channel) =
> > +				CCR_DMOD_FIFO |
> > +				CCR_SMOD_FIFO |
> > +				CCR_SSIZ_32 | CCR_DSIZ_16 |
> > +				CCR_REN;
> > +
> > +		imx_dma_setup_single(
> > +			drv_data->tx_channel,
> > +			drv_data->tx_dma,
> > +			drv_data->len,
> > +			drv_data->rd_data_phys + 4,
> > +			DMA_MODE_WRITE
> > +		);
> 
> The ); all on its own is overdoing things a bit.
> 
> > +		if (drv_data->rx) {
> > +			/* Setup rx DMA for linear destination address */
> > +			CCR(drv_data->rx_channel) =
> > +				CCR_DMOD_LINEAR |
> > +				CCR_SMOD_FIFO |
> > +				CCR_DSIZ_32 | CCR_SSIZ_16 |
> > +				CCR_REN;
> > +			imx_dma_setup_single(
> > +				drv_data->rx_channel,
> > +				drv_data->rx_dma,
> > +				drv_data->len,
> > +				drv_data->rd_data_phys,
> > +				DMA_MODE_READ);
> 
> And inconsistent.
> 
> > +			imx_dma_enable(drv_data->rx_channel);
> > +
> > +			/* Enable SPI interrupt */
> > +			wr_INT_STATUS(SPI_INTEN_RO, regs);
> > +
> > +			/* Set SPI to request DMA service on both
> > +			   Rx and Tx half fifo watermark */
> > +			wr_DMA(SPI_DMA_RHDEN | SPI_DMA_THDEN, regs);
> > +		} else
> > +			/* Write only access -> set SPI to request DMA
> > +			   service on Tx half fifo watermark */
> > +			wr_DMA(SPI_DMA_THDEN, regs);
> > +
> > +		imx_dma_enable(drv_data->tx_channel);
> > +	} else {
> > +		dev_dbg(&drv_data->pdev->dev,
> > +			"pump pio transfer\n"
> > +			"    tx      = 0x%08X\n"
> > +			"    rx      = 0x%08X\n"
> > +			"    len     = %d\n",
> > +			(u32)drv_data->tx,
> > +			(u32)drv_data->rx,
> > +			(u32)drv_data->len);
> 
> More bad casting.  Please review entire patch.
> 
> > +		/* Ensure we have the correct interrupt handler	*/
> > +		if (drv_data->rx)
> > +			drv_data->transfer_handler = interrupt_transfer;
> > +		else
> > +			drv_data->transfer_handler = interrupt_wronly_transfer;
> > +
> > +		/* Enable SPI */
> > +		wr_CONTROL(rd_CONTROL(regs) | SPI_CONTROL_SPIEN, regs);
> > +
> > +		/* Enable SPI interrupt */
> > +		if (drv_data->rx)
> > +			wr_INT_STATUS(SPI_INTEN_TH | SPI_INTEN_RO, regs);
> > +		else
> > +			wr_INT_STATUS(SPI_INTEN_TH, regs);
> > +	}
> > +}
> > +
> >
> > ...
> >
> > +static int spi_imx_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct spi_imx_master *platform_info;
> > +	struct spi_master *master;
> > +	struct driver_data *drv_data = NULL;
> > +	struct resource *res;
> > +	int irq, status = 0;
> > +
> > +	platform_info = dev->platform_data;
> > +	if (platform_info == NULL) {
> > +		dev_err(&pdev->dev, "probe - no platform data supplied\n");
> > +		status = -ENODEV;
> > +		goto err_no_pdata;
> > +	}
> > +
> > +	/* Allocate master with space for drv_data */
> > +	master = spi_alloc_master(dev, sizeof(struct driver_data));
> > +	if (!master) {
> > +		dev_err(&pdev->dev, "probe - cannot alloc spi_master\n");
> > +		status = -ENOMEM;
> > +		goto err_no_mem;
> > +	}
> > +	drv_data = spi_master_get_devdata(master);
> > +	drv_data->master = master;
> > +	drv_data->master_info = platform_info;
> > +	drv_data->pdev = pdev;
> > +
> > +	master->bus_num = pdev->id;
> > +	master->num_chipselect = platform_info->num_chipselect;
> > +	master->cleanup = cleanup;
> > +	master->setup = setup;
> > +	master->transfer = transfer;
> > +
> > +	drv_data->dummy_dma_buf = SPI_DUMMY_u32;
> > +
> > +	/* Find and map resources */
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "probe - MEM resources not defined\n");
> > +		status = -ENODEV;
> > +		goto err_no_iores;
> > +	}
> > +	drv_data->ioarea = request_mem_region(res->start,
> > +						res->end - res->start + 1,
> > +						pdev->name);
> > +	if (drv_data->ioarea == NULL) {
> > +		dev_err(&pdev->dev, "probe - cannot reserve region\n");
> > +		status = -ENXIO;
> > +		goto err_no_iores;
> > +	}
> > +	drv_data->regs = ioremap(res->start, res->end - res->start + 1);
> > +	if (drv_data->regs == NULL) {
> > +		dev_err(&pdev->dev, "probe - cannot map IO\n");
> > +		status = -ENXIO;
> > +		goto err_no_iomap;
> > +	}
> > +	drv_data->rd_data_phys = (dma_addr_t)res->start;
> 
> I don't think it's correct to cast a kernel virtual address straight to a
> dma_addr_t.

Resource addresses should be physical, not virtual; so it's
common for platform-specific drivers to know that phys==dma.


> > +	/* Attach to IRQ */
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "probe - IRQ resource not defined\n");
> > +		status = -ENODEV;
> > +		goto err_no_irqres;
> > +	}
> > +	status = request_irq(irq, spi_int, SA_INTERRUPT, dev->bus_id, drv_data);
> 
> SA_INTERRUPT is deprecated.  Use IRQF_DISABLED.
> 
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists