RE: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver

From: Andrea Paterniani
Date: Tue Oct 03 2006 - 12:09:43 EST

Next message: Randy Dunlap: "Re: [PATCH/RFC] Math-emu kills the kernel on Athlon64 X2"
Previous message: Krzysztof Halasa: "Re: Question on HDLC and raw access to T1/E1 serial streams."
In reply to: David Brownell: "Re: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver"
Next in thread: David Brownell: "Re: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Here some questions and answers to your comments, (please consider I'm nearly new to kernel programming).

> ...
> ug. Why not simply open-code
>
> readl(addr + DATA);

I found usefull to define macros to use inside code something like
rd_CONTROL(regs)
instead of
readl(regs + 0x08)
since to me the macro sounds more friendly.
Should I have to adhere to some standard ?

> 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.

Please suggest me what it's more appropriate.

> > + 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.

What inline function do you mean ?

> > + remaining_data = (u32)(rx_end - rx);
>
> Missing divide?

No divide is missing since u8_reader (u8_writer) reads (writes) data of 1 byte size.
On the other hand dummy_writer that is used to achive reads regardless of transfer bit_per_words must consider single word byte size
to correctly count data.

> Why does it use void*'s for this enumeration?

Because field state of spi_message is defined as void* (see include/linux/spi.h).

> I see tasklets being scheduled, but no tasklet_disable() or tasklet_kill(),
> etc. Is this driver racy against shutdown or rmmod?

Do you mean I should use tasklet_kill() inside spi_imx_remove ?

> > + 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.

File include/asm-arm/types.h defines
typedef u32 dma_addr_t;
Also I think that for ARM architecture resource_size_t in practice is u32 since CONFIG_RESOURCES_64BIT isn't defined.
Is this construction correct ? If not what should I do ?

Reagrds,
- Andrea

-----Messaggio originale-----
Da: Andrew Morton [mailto:akpm@xxxxxxxx]
Inviato: lunedi 2 ottobre 2006 20.00
A: David Brownell
Cc: Andrea Paterniani; Linux Kernel list
Oggetto: Re: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller
driver

On Mon, 2 Oct 2006 08:16:58 -0700
David Brownell <david-b@xxxxxxxxxxx> wrote:

> Subject: SPI controller driver for Freescale iMX
> From: Andrea Paterniani <a.paterniani@xxxxxxxxxxxx>
>
> 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.

> +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.

> + /* 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Randy Dunlap: "Re: [PATCH/RFC] Math-emu kills the kernel on Athlon64 X2"
Previous message: Krzysztof Halasa: "Re: Question on HDLC and raw access to T1/E1 serial streams."
In reply to: David Brownell: "Re: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver"
Next in thread: David Brownell: "Re: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]