| Message ID | 1373451678-20065-1-git-send-email-a13xp0p0v88@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
On Wed, Jul 10, 2013 at 14:21 +0400, Alexander Popov wrote: > > This is SCLPC device driver for the Freescale MPC512x. > It is needed for Direct Memory Access to the devices on LocalPlus Bus. > > Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com> > --- > arch/powerpc/boot/dts/mpc5121.dtsi | 8 +- > arch/powerpc/include/asm/mpc5121.h | 32 ++ > arch/powerpc/platforms/512x/Kconfig | 6 + > arch/powerpc/platforms/512x/Makefile | 1 + > arch/powerpc/platforms/512x/mpc512x_lpbfifo.c | 485 ++++++++++++++++++++++++++ > 5 files changed, 531 insertions(+), 1 deletion(-) > create mode 100644 arch/powerpc/platforms/512x/mpc512x_lpbfifo.c > > diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi > index bd14c00..6e0b0c0 100644 > --- a/arch/powerpc/boot/dts/mpc5121.dtsi > +++ b/arch/powerpc/boot/dts/mpc5121.dtsi > @@ -261,7 +261,13 @@ > /* LocalPlus controller */ > lpc@10000 { > compatible = "fsl,mpc5121-lpc"; > - reg = <0x10000 0x200>; > + reg = <0x10000 0x100>; > + }; > + > + sclpc@10100 { > + compatible = "fsl,mpc512x-lpbfifo"; > + reg = <0x10100 0x50>; > + interrupts = <7 0x8>; > }; > > pata@10200 { > diff --git a/arch/powerpc/include/asm/mpc5121.h b/arch/powerpc/include/asm/mpc5121.h > index 8ae133e..f416a7e 100644 > --- a/arch/powerpc/include/asm/mpc5121.h > +++ b/arch/powerpc/include/asm/mpc5121.h > @@ -69,4 +69,36 @@ struct mpc512x_lpc { > > int mpc512x_cs_config(unsigned int cs, u32 val); > > +/* > + * SCLPC Module (LPB FIFO) > + */ > +enum lpb_dev_portsize { > + LPB_DEV_PORTSIZE_UNDEFINED = 0, > + LPB_DEV_PORTSIZE_1_BYTE = 1, > + LPB_DEV_PORTSIZE_2_BYTES = 2, > + LPB_DEV_PORTSIZE_4_BYTES = 4, > + LPB_DEV_PORTSIZE_8_BYTES = 8, > +}; > + > +enum mpc512x_lpbfifo_req_dir { > + MPC512X_LPBFIFO_REQ_DIR_READ, > + MPC512X_LPBFIFO_REQ_DIR_WRITE, > +}; > + > +struct mpc512x_lpbfifo_request { > + unsigned int cs; > + phys_addr_t bus_phys; /* physical address of some device on lpb */ > + void *ram_virt; /* virtual address of some region in ram */ > + > + /* Details of transfer */ > + u32 size; > + enum lpb_dev_portsize portsize; > + enum mpc512x_lpbfifo_req_dir dir; > + > + /* Call when the transfer is finished */ > + void (*callback)(struct mpc512x_lpbfifo_request *); > +}; > + > +extern int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req); > + > #endif /* __ASM_POWERPC_MPC5121_H__ */ Needs the mpc512x_lpbfifo_request structure be part of the official API? Could it be desirable to hide it behind a "fill-in" routine? Which BTW could auto-determine CS numbers and port width associated with a chip select from the XLB and LPB register set? Who's using the submit routine? I might have missed the "client side" of that API in the series. > diff --git a/arch/powerpc/platforms/512x/Kconfig b/arch/powerpc/platforms/512x/Kconfig > index fc9c1cb..0db8aa9 100644 > --- a/arch/powerpc/platforms/512x/Kconfig > +++ b/arch/powerpc/platforms/512x/Kconfig > @@ -10,6 +10,12 @@ config PPC_MPC512x > select USB_EHCI_BIG_ENDIAN_MMIO > select USB_EHCI_BIG_ENDIAN_DESC > > +config PPC_MPC512x_LPBFIFO > + tristate "MPC512x LocalPlus bus FIFO driver" > + depends on PPC_MPC512x && MPC512X_DMA > + help > + Enable support for the Freescale MPC512x SCLPC. > + > config MPC5121_ADS > bool "Freescale MPC5121E ADS" > depends on PPC_MPC512x > diff --git a/arch/powerpc/platforms/512x/Makefile b/arch/powerpc/platforms/512x/Makefile > index 72fb934..df932fa 100644 > --- a/arch/powerpc/platforms/512x/Makefile > +++ b/arch/powerpc/platforms/512x/Makefile > @@ -2,6 +2,7 @@ > # Makefile for the Freescale PowerPC 512x linux kernel. > # > obj-y += clock.o mpc512x_shared.o > +obj-$(CONFIG_PPC_MPC512x_LPBFIFO) += mpc512x_lpbfifo.o > obj-$(CONFIG_MPC5121_ADS) += mpc5121_ads.o mpc5121_ads_cpld.o > obj-$(CONFIG_MPC512x_GENERIC) += mpc512x_generic.o > obj-$(CONFIG_PDM360NG) += pdm360ng.o s/PPC_// here to align with the other tunables? > diff --git a/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c b/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c > new file mode 100644 > index 0000000..6bd8aab > --- /dev/null > +++ b/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c > @@ -0,0 +1,485 @@ > +/* > + * LocalPlus Bus SCLPC driver for the Freescale MPC512x. > + * > + * Copyright (C) Promcontroller, 2013. > + * > + * Author is Alexander Popov <a13xp0p0v88@gmail.com>. nit pick: is 1337 speak usual and appropriate here? > + * > + * The driver design is based on mpc52xx_lpbfifo driver > + * written by Grant Likely <grant.likely@secretlab.ca>. > + * > + * This file is released under the GPLv2. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <asm/io.h> > +#include <linux/spinlock.h> > +#include <linux/slab.h> > +#include <asm/mpc5121.h> > +#include <linux/dmaengine.h> > +#include <linux/dma-direction.h> > +#include <linux/dma-mapping.h> shouldn't headers get sorted alphabetically? > + > +MODULE_AUTHOR("Alexander Popov <a13xp0p0v88@gmail.com>"); > +MODULE_DESCRIPTION("MPC512x LocalPlus FIFO device driver"); > +MODULE_LICENSE("GPL"); aren't these usually at the end of the source for quick lookup? > + > +#define DRV_NAME "mpc512x_lpbfifo" > + > +#define LPBFIFO_REG_PACKET_SIZE (0x00) > +#define LPBFIFO_REG_START_ADDRESS (0x04) > +#define LPBFIFO_REG_CONTROL (0x08) > +#define LPBFIFO_REG_ENABLE (0x0C) > +#define LPBFIFO_REG_STATUS (0x14) > +#define LPBFIFO_REG_BYTES_DONE (0x18) > +#define LPBFIFO_REG_EMB_SHARE_COUNTER (0x1C) > +#define LPBFIFO_REG_EMB_PAUSE_CONTROL (0x20) > +#define LPBFIFO_REG_FIFO_DATA (0x40) > +#define LPBFIFO_REG_FIFO_STATUS (0x44) > +#define LPBFIFO_REG_FIFO_CONTROL (0x48) > +#define LPBFIFO_REG_FIFO_ALARM (0x4C) Should this not be a struct? Since using member field names allows for compile time checks of data types, which is highly desirable with registers of arbitrarily differing size. You may as well want to define bit masks or shift counts here, to keep magic numbers out of the code. > + > +#define DMA_LPC_CHANNEL_NUMBER 26 > +#define DEFAULT_WORDS_PER_TRANSFER 1 can you eliminate the DMA channel number in the DMA client source? this shall be intimate knowledge of the DMA engine driver, or at least get specified in the device tree BTW did Anatolij suggest OF based DMA channel lookup back in May, see commit e48fc15 and search for 'rx-tx' in the mxcmmc.c file > + > +static struct mpc512x_lpbfifo { > + struct device *dev; > + struct resource res; > + void __iomem *regs; > + int irq; > + spinlock_t lock; > + > + /* Current state data */ > + int im_last; /* For "last one out turns off the lights" principle */ > + struct mpc512x_lpbfifo_request *req; > + dma_addr_t ram_bus_addr; > + struct dma_chan *chan; > +} lpbfifo; > + > +/* > + * Before we can wrap up handling current mpc512x_lpbfifo_request > + * and execute the callback registered in it we should: > + * 1. recognize that everything is really done, > + * 2. free memory and dmaengine channel. > + * > + * Writing from RAM to registers of some device on LPB (transmit) > + * is not really done until the LPB FIFO completion irq triggers. > + * > + * For being sure that writing from registers of some device on LPB > + * to RAM (receive) is really done we should wait > + * for mpc512x_lpbfifo_callback() to be called by DMA driver. > + * In this case LPB FIFO completion irq will not appear at all. > + * > + * Moreover, freeing memory and dmaengine channel is not safe until > + * mpc512x_lpbfifo_callback() is called. > + * > + * So to make it simple: > + * last one out turns off the lights. > + */ Can this "feedback order issue" handling get simplified by having both the LPB controller FIFO and the DMA completion callback invoke a single routine, which tracks the "LPB done" and "DMA done" conditions regardless of their order, and does postprocessing when both were satisfied? It might be desirable to always run the postprocessing only if both involved components finished their activities, regardless of the transfer's direction. This reduces the potential for access to invalid data if these two events are "racy". > + > +/* > + * mpc512x_lpbfifo_irq - IRQ handler for LPB FIFO > + */ > +static irqreturn_t mpc512x_lpbfifo_irq(int irq, void *dev_id) > +{ > + struct mpc512x_lpbfifo_request *req; > + unsigned long flags; > + u32 status; > + > + spin_lock_irqsave(&lpbfifo.lock, flags); > + > + req = lpbfifo.req; > + if (!req) { > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + pr_err("bogus LPBFIFO IRQ\n"); > + return IRQ_HANDLED; > + } > + > + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_READ) { > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + pr_err("bogus LPBFIFO IRQ (we are waiting DMA IRQ)\n"); > + return IRQ_HANDLED; > + } > + > + /* Clear the interrupt flag */ > + status = in_8(lpbfifo.regs + LPBFIFO_REG_STATUS); > + if (status & 0x01) > + out_8(lpbfifo.regs + LPBFIFO_REG_STATUS, 0x01); > + else > + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000); Magic numbers -- it's hard to determine what happens here if you don't know the reference manual by heart. > + > + if (!lpbfifo.im_last) { > + /* I'm not the last: DMA is still in progress. */ > + lpbfifo.im_last = 1; > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + } else { > + /* I'm last. Let's wrap up. */ > + /* Set the FIFO as idle */ > + req = lpbfifo.req; > + lpbfifo.req = NULL; > + > + /* The spinlock must be dropped > + * before executing the callback, > + * otherwise we could end up with a deadlock > + * or nested spinlock condition. */ > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + if (req->callback) > + req->callback(req); > + } > + > + return IRQ_HANDLED; > +} > + > +/* > + * mpc512x_lpbfifo_callback is called by DMA driver > + * when DMA transaction is finished. > + */ > +static void mpc512x_lpbfifo_callback(void *param) > +{ > + unsigned long flags; > + struct mpc512x_lpbfifo_request *req; > + enum dma_data_direction dir; > + > + spin_lock_irqsave(&lpbfifo.lock, flags); > + > + /* Free resources */ > + if (lpbfifo.req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) > + dir = DMA_TO_DEVICE; > + else > + dir = DMA_FROM_DEVICE; > + dma_unmap_single(lpbfifo.chan->device->dev, > + lpbfifo.ram_bus_addr, lpbfifo.req->size, dir); > + dma_release_channel(lpbfifo.chan); > + > + if (lpbfifo.req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE && > + !lpbfifo.im_last) { > + /* I'm not the last: LPB FIFO is still writing data. */ > + lpbfifo.im_last = 1; > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + } else { > + /* I'm the last or alone here. Let's wrap up. */ > + /* Set the FIFO as idle */ > + req = lpbfifo.req; > + lpbfifo.req = NULL; > + > + /* The spinlock must be dropped > + * before executing the callback, > + * otherwise we could end up with a deadlock > + * or nested spinlock condition. */ > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + if (req->callback) > + req->callback(req); > + } > +} > + > +static bool channel_filter(struct dma_chan *chan, void *filter_param) > +{ > + if (chan->chan_id == DMA_LPC_CHANNEL_NUMBER) > + return true; > + else > + return false; > +} Please make use of OF based DMA channel lookup. You don't need the filter routine here and need not know which channel number is appropriate. The client should not care. See Lars-Peter Clausen's of_dma_xlate_by_chan_id() implementation (patchwork 2555701, can't tell whether there's a newer version around of whether it has been applied somewhere,, and missed myself the opportunity to provide feedback and support). > + > +static int mpc512x_lpbfifo_kick(struct mpc512x_lpbfifo_request *req) > +{ > + u32 bits; > + int no_incr = 0; > + u32 bpt; > + dma_cap_mask_t cap_mask; > + dma_filter_fn fn = channel_filter; > + struct dma_device *dma_dev = NULL; > + struct scatterlist sg; > + enum dma_data_direction dir; > + struct dma_slave_config dma_conf = {}; > + struct dma_async_tx_descriptor *dma_tx = NULL; > + dma_cookie_t cookie; > + int e; > + > + /* 1. Check requirements: */ > + /* Packet size must be a multiple of 4 bytes since > + * FIFO Data Word Register (which provides data to DMA controller) > + * allows only "full-word" (4 bytes) access > + * according Reference Manual */ Are you certain about that constraint? Can't the packet size be an "incomplete multiple" of the SCLPC FIFO port's width when the last data item is appropriately aligned? Since you can attach 8bit, 16bit, as well as 32bit wide peripherals to the LPB (the external bus), I feel that insisting in full 32bits getting transferred may be inappropriate. Unless I'm missing something, and it's an SCLPC contraint while the CPU's access to the LPB isn't limited. > + if (!IS_ALIGNED(req->size, 4)) { > + e = -EINVAL; > + goto err_align; > + } > + > + /* Physical address of the device on LPB and packet size > + * must be aligned/multiple of BPT (bytes per transaction) > + * according Reference Manual */ That's interesting, as you can read it the other way around, too: Make sure to pick BPT (which may be considered variable) such that the address and length specs (provided as input from outside) are multiples ... But for this specific case (transfer between memory and the SCLPC FIFO, assuming full 32bit quantities and nothing less), hardcoding a value of four may be appropriate. > + if (req->portsize != LPB_DEV_PORTSIZE_UNDEFINED) { > + bpt = req->portsize; > + no_incr = 1; > + } else > + bpt = DEFAULT_WORDS_PER_TRANSFER << 2; /* makes life easier */ > + > + if (!IS_ALIGNED(req->bus_phys | req->size, bpt)) { > + e = -EFAULT; > + goto err_align; > + } > + > + /* 2. Prepare DMA */ > + dma_cap_zero(cap_mask); > + dma_cap_set(DMA_SLAVE, cap_mask); > + lpbfifo.chan = dma_request_channel(cap_mask, fn, NULL); OF lookup > + if (!lpbfifo.chan) { > + e = -ENODEV; > + goto err_align; > + } > + dma_dev = lpbfifo.chan->device; > + > + sg_init_table(&sg, 1); > + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) > + dir = DMA_TO_DEVICE; > + else > + dir = DMA_FROM_DEVICE; > + sg_dma_address(&sg) = dma_map_single(dma_dev->dev, > + req->ram_virt, req->size, dir); > + if (dma_mapping_error(dma_dev->dev, sg_dma_address(&sg))) { > + pr_err("dma_mapping_error\n"); > + e = -EFAULT; > + goto err_dma_map; > + } > + lpbfifo.ram_bus_addr = sg_dma_address(&sg); /* will free it later */ > + sg_dma_len(&sg) = req->size; > + > + /* We should limit the maximum number of words > + * (units with FIFO Data Register size) > + * that can be read from / written to the FIFO > + * in one DMA burst. > + * This measure and FIFO watermarks will prevent > + * DMA controller from overtaking FIFO > + * and causing FIFO underflow / overflow error. */ > + dma_conf.dst_maxburst = DEFAULT_WORDS_PER_TRANSFER; > + dma_conf.src_maxburst = DEFAULT_WORDS_PER_TRANSFER; > + > + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) { > + dma_conf.direction = DMA_MEM_TO_DEV; > + dma_conf.dst_addr = lpbfifo.res.start + LPBFIFO_REG_FIFO_DATA; > + } else { > + dma_conf.direction = DMA_DEV_TO_MEM; > + dma_conf.src_addr = lpbfifo.res.start + LPBFIFO_REG_FIFO_DATA; > + } > + dma_conf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > + dma_conf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > + > + /* Make DMA channel work with LPB FIFO data register */ > + if (dma_dev->device_control(lpbfifo.chan, > + DMA_SLAVE_CONFIG, (unsigned long)&dma_conf)) { > + goto err_dma_prep; > + } > + > + dma_tx = dmaengine_prep_slave_sg(lpbfifo.chan, &sg, > + 1, dma_conf.direction, 0); > + if (!dma_tx) { > + pr_err("dmaengine_prep_slave_sg failed\n"); > + e = -ENOSPC; > + goto err_dma_prep; > + } > + > + dma_tx->callback = mpc512x_lpbfifo_callback; > + dma_tx->callback_param = NULL; > + > + /* 3. Prepare FIFO */ > + /* Set and clear the reset bits; > + * is good practice in Reference Manual */ > + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000); > + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x0); > + > + /* Configure the watermarks. > + * > + * RAM->DMA->FIFO->LPB_DEV (write): > + * High watermark (7 * 4) free bytes (according Reference Manual) > + * Low watermark 996 bytes (whole FIFO - 28 bytes) > + * > + * LPB_DEV->FIFO->DMA->RAM (read): > + * High watermark (1024 - 4) free bytes > + * (whole FIFO - DEFAULT_WORDS_PER_TRANSFER) > + * Low watermark 0 bytes > + */ > + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) { > + out_be16(lpbfifo.regs + LPBFIFO_REG_FIFO_CONTROL, 0x0700); > + out_be32(lpbfifo.regs + LPBFIFO_REG_FIFO_ALARM, 0x000003e4); > + } else { > + out_be16(lpbfifo.regs + LPBFIFO_REG_FIFO_CONTROL, 0x0); > + out_be32(lpbfifo.regs + LPBFIFO_REG_FIFO_ALARM, 0x000003fc); > + } Can you use symbolic names for the flag bits, and decimal numbers for the watermarks? This way they would match with the comment, or the comment may become obsolete when numbers aren't obfuscated. > + > + /* Start address is a physical address of the region > + * which belongs to the device on localplus bus */ > + out_be32(lpbfifo.regs + LPBFIFO_REG_START_ADDRESS, req->bus_phys); > + > + /* Configure chip select, transfer direction, > + * address increment option and bytes per transfer option */ > + bits = (req->cs & 0x7) << 24; > + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_READ) > + bits |= 3 << 16; /* read mode bit and flush bit */ > + if (no_incr) > + bits |= 1 << 8; > + bits |= bpt & 0x3f; > + out_be32(lpbfifo.regs + LPBFIFO_REG_CONTROL, bits); > + > + /* Unmask irqs */ > + bits = 0x00000201; /* set error irq & master enabled bit */ > + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) > + bits |= 0x00000100; /* set completion irq too */ > + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, bits); > + > + /* 4. Set packet size and kick FIFO off */ > + bits = req->size; > + bits |= (1<<31); /* set restart bit */ > + out_be32(lpbfifo.regs + LPBFIFO_REG_PACKET_SIZE, bits); here you start the SCLPC transfer ... > + > + > + /* 5. Then kick DMA off */ > + cookie = dma_tx->tx_submit(dma_tx); > + if (dma_submit_error(cookie)) { > + pr_err("DMA tx_submit failed\n"); > + e = -ENOSPC; > + goto err_dma_submit; > + } ... and here you setup the DMA job -- isn't this too late? (Please note that I haven't re-checked the "functional description" section in the DMA controller's chapter.) > + > + return 0; > + > + err_dma_submit: > + > + err_dma_prep: > + dma_unmap_single(dma_dev->dev, sg_dma_address(&sg), req->size, dir); > + > + err_dma_map: > + sg_dma_address(&sg) = 0; > + dma_release_channel(lpbfifo.chan); > + > + err_align: > + return e; > +} > + > +int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req) > +{ > + unsigned long flags; > + int result = 0; > + > + if (!lpbfifo.regs) > + return -ENODEV; > + > + spin_lock_irqsave(&lpbfifo.lock, flags); > + > + /* A transfer is in progress > + * if the req pointer is already set */ > + if (lpbfifo.req) { > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + return -EBUSY; > + } > + > + /* (128 kBytes - 4 Bytes) is a maximum packet size > + * that LPB FIFO and DMA controller can handle together > + * while exchanging DEFAULT_WORDS_PER_TRANSFER = 1 > + * per hardware request */ Could you state what the individual limits are? I guess the SCLPC can transfer gigabytes, the FIFO depth either should not matter or is dramatically lower than 128KB (1K?). Is the DMA controller the limiting element, and how so? Does the limit not depend on the biter and citer and width specs? Is the limit mentioned here (in the LPB related code) assuming knowledge of the DMA driver's internals? > + if (req->size > 131068) { > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + return -ENOSPC; > + } > + > + /* Setup the transfer */ > + lpbfifo.im_last = 0; > + lpbfifo.req = req; > + > + result = mpc512x_lpbfifo_kick(req); > + if (result != 0) > + lpbfifo.req = NULL; /* Set the FIFO as idle */ > + > + spin_unlock_irqrestore(&lpbfifo.lock, flags); > + > + return result; > +} > +EXPORT_SYMBOL(mpc512x_lpbfifo_submit); > + > +static int mpc512x_lpbfifo_probe(struct platform_device *pdev) > +{ > + struct resource *r = &lpbfifo.res; > + int e = 0, rc = -ENOMEM; > + > + if (of_address_to_resource(pdev->dev.of_node, 0, r)) { > + e = -ENODEV; > + goto err_res; > + } > + > + if (!request_mem_region(r->start, resource_size(r), DRV_NAME)) { > + e = -EBUSY; > + goto err_res; > + } > + > + lpbfifo.irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > + if (!lpbfifo.irq) { > + e = -ENODEV; > + goto err_res; > + } > + > + lpbfifo.regs = ioremap(r->start, resource_size(r)); > + if (!lpbfifo.regs) { > + e = -ENOMEM; > + goto err_regs; > + } > + > + spin_lock_init(&lpbfifo.lock); > + > + /* Put FIFO into reset state */ > + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000); > + > + lpbfifo.dev = &pdev->dev; > + > + rc = request_irq(lpbfifo.irq, > + mpc512x_lpbfifo_irq, 0, DRV_NAME, &lpbfifo); whitespace (alignment of continuation) (just noticed here, not explicitely checked elsewhere, unless it's a misdetection caused by TABs and email quotation) > + if (rc) { > + e = -ENODEV; > + goto err_irq; > + } DMA channel lookup here? As it should not depend on anything that's provided later at runtime. > + > + return 0; > + > + err_irq: > + iounmap(lpbfifo.regs); > + lpbfifo.regs = NULL; > + > + err_regs: > + release_mem_region(r->start, resource_size(r)); > + > + err_res: > + dev_err(&pdev->dev, "mpc512x_lpbfifo_probe() failed\n"); > + return e; > +} > + > +static int mpc512x_lpbfifo_remove(struct platform_device *pdev) > +{ > + /* Put FIFO in reset */ > + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000); > + > + free_irq(lpbfifo.irq, &lpbfifo); > + iounmap(lpbfifo.regs); > + release_mem_region(lpbfifo.res.start, resource_size(&lpbfifo.res)); > + lpbfifo.regs = NULL; > + lpbfifo.dev = NULL; > + > + return 0; > +} > + > +static const struct of_device_id mpc512x_lpbfifo_match[] = { > + { .compatible = "fsl,mpc512x-lpbfifo", }, > + {}, > +}; > + > +static struct platform_driver mpc512x_lpbfifo_driver = { > + .probe = mpc512x_lpbfifo_probe, > + .remove = mpc512x_lpbfifo_remove, > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, > + .of_match_table = mpc512x_lpbfifo_match, > + }, > +}; > +module_platform_driver(mpc512x_lpbfifo_driver); > -- > 1.7.11.3 virtually yours Gerhard Sittig
On 07/10/2013 08:46 AM, Gerhard Sittig wrote: > On Wed, Jul 10, 2013 at 14:21 +0400, Alexander Popov wrote: >> >> This is SCLPC device driver for the Freescale MPC512x. >> It is needed for Direct Memory Access to the devices on LocalPlus Bus. >> >> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com> >> --- >> arch/powerpc/boot/dts/mpc5121.dtsi | 8 +- >> arch/powerpc/include/asm/mpc5121.h | 32 ++ >> arch/powerpc/platforms/512x/Kconfig | 6 + >> arch/powerpc/platforms/512x/Makefile | 1 + >> arch/powerpc/platforms/512x/mpc512x_lpbfifo.c | 485 ++++++++++++++++++++++++++ >> 5 files changed, 531 insertions(+), 1 deletion(-) >> create mode 100644 arch/powerpc/platforms/512x/mpc512x_lpbfifo.c >> >> diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi >> index bd14c00..6e0b0c0 100644 >> --- a/arch/powerpc/boot/dts/mpc5121.dtsi >> +++ b/arch/powerpc/boot/dts/mpc5121.dtsi >> @@ -261,7 +261,13 @@ >> /* LocalPlus controller */ >> lpc@10000 { >> compatible = "fsl,mpc5121-lpc"; >> - reg = <0x10000 0x200>; >> + reg = <0x10000 0x100>; >> + }; >> + >> + sclpc@10100 { >> + compatible = "fsl,mpc512x-lpbfifo"; The binding needs to be documented. Rob
Gerhard Sittig wrote: >> >+ * Author is Alexander Popov<a13xp0p0v88@gmail.com>. > nit pick: is 1337 speak usual and appropriate here? Um, that's his actual email address. > shouldn't headers get sorted alphabetically? Is that a new policy? I've never heard that requirement. >> +MODULE_AUTHOR("Alexander Popov <a13xp0p0v88@gmail.com>"); >> +MODULE_DESCRIPTION("MPC512x LocalPlus FIFO device driver"); >> +MODULE_LICENSE("GPL"); > > aren't these usually at the end of the source for quick lookup? Yes, and it should be "GPL v2" also.
Hello Gerhard Thanks for the review. I'll do my best to answer your questions and fix my code. 2013/7/10 Gerhard Sittig <gsi@denx.de>: > On Wed, Jul 10, 2013 at 14:21 +0400, Alexander Popov wrote: >> +/* >> + * SCLPC Module (LPB FIFO) >> + */ >> +enum lpb_dev_portsize { >> + LPB_DEV_PORTSIZE_UNDEFINED = 0, >> + LPB_DEV_PORTSIZE_1_BYTE = 1, >> + LPB_DEV_PORTSIZE_2_BYTES = 2, >> + LPB_DEV_PORTSIZE_4_BYTES = 4, >> + LPB_DEV_PORTSIZE_8_BYTES = 8, >> +}; >> + >> +enum mpc512x_lpbfifo_req_dir { >> + MPC512X_LPBFIFO_REQ_DIR_READ, >> + MPC512X_LPBFIFO_REQ_DIR_WRITE, >> +}; >> + >> +struct mpc512x_lpbfifo_request { >> + unsigned int cs; >> + phys_addr_t bus_phys; /* physical address of some device on lpb */ >> + void *ram_virt; /* virtual address of some region in ram */ >> + >> + /* Details of transfer */ >> + u32 size; >> + enum lpb_dev_portsize portsize; >> + enum mpc512x_lpbfifo_req_dir dir; >> + >> + /* Call when the transfer is finished */ >> + void (*callback)(struct mpc512x_lpbfifo_request *); >> +}; >> + >> +extern int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req); >> + >> #endif /* __ASM_POWERPC_MPC5121_H__ */ > Needs the mpc512x_lpbfifo_request structure be part of the > official API? Could it be desirable to hide it behind a > "fill-in" routine? Which BTW could auto-determine CS numbers and > port width associated with a chip select from the XLB and LPB > register set? 1. As I wrote at the beginning of the file, the driver design is based on mpc52xx_lpbfifo driver written by Grant Likely. 2. CS and port width are attributes of some device on LPBus which participates DMA data transfer. It is the driver of this device who knows them. It fills mpc512x_lpbfifo_request as a client side of that API. > Who's using the submit routine? I might have missed the "client > side" of that API in the series. Please look at https://patchwork.kernel.org/patch/2511951/ >> +obj-$(CONFIG_PPC_MPC512x_LPBFIFO) += mpc512x_lpbfifo.o >> obj-$(CONFIG_MPC5121_ADS) += mpc5121_ads.o mpc5121_ads_cpld.o >> obj-$(CONFIG_MPC512x_GENERIC) += mpc512x_generic.o > s/PPC_// here to align with the other tunables? I'll fix that :) >> +MODULE_AUTHOR("Alexander Popov <a13xp0p0v88@gmail.com>"); >> +MODULE_DESCRIPTION("MPC512x LocalPlus FIFO device driver"); >> +MODULE_LICENSE("GPL"); >aren't these usually at the end of the source for quick lookup? I'll fix that too. >> +#define LPBFIFO_REG_PACKET_SIZE (0x00) >> +#define LPBFIFO_REG_START_ADDRESS (0x04) >> +#define LPBFIFO_REG_CONTROL (0x08) >> +#define LPBFIFO_REG_ENABLE (0x0C) >> +#define LPBFIFO_REG_STATUS (0x14) >> +#define LPBFIFO_REG_BYTES_DONE (0x18) >> +#define LPBFIFO_REG_EMB_SHARE_COUNTER (0x1C) >> +#define LPBFIFO_REG_EMB_PAUSE_CONTROL (0x20) >> +#define LPBFIFO_REG_FIFO_DATA (0x40) >> +#define LPBFIFO_REG_FIFO_STATUS (0x44) >> +#define LPBFIFO_REG_FIFO_CONTROL (0x48) >> +#define LPBFIFO_REG_FIFO_ALARM (0x4C) > Should this not be a struct? Since using member field names > allows for compile time checks of data types, which is highly > desirable with registers of arbitrarily differing size. I've read https://lists.ozlabs.org/pipermail/linuxppc-dev/2010-January/079476.html Which way should I use? > You may as well want to define bit masks or shift counts here, to > keep magic numbers out of the code. Yes, I'll try to get rid of them. >> + >> +#define DMA_LPC_CHANNEL_NUMBER 26 >> +#define DEFAULT_WORDS_PER_TRANSFER 1 > can you eliminate the DMA channel number in the DMA client > source? this shall be intimate knowledge of the DMA engine > driver, or at least get specified in the device tree > BTW did Anatolij suggest OF based DMA channel lookup back in May, > see commit e48fc15 and search for 'rx-tx' in the mxcmmc.c file Ok, thanks, I've found it. I'll make OF based DMA channel lookup. >> +/* >> + * Before we can wrap up handling current mpc512x_lpbfifo_request >> + * and execute the callback registered in it we should: >> + * 1. recognize that everything is really done, >> + * 2. free memory and dmaengine channel. >> + * >> + * Writing from RAM to registers of some device on LPB (transmit) >> + * is not really done until the LPB FIFO completion irq triggers. >> + * >> + * For being sure that writing from registers of some device on LPB >> + * to RAM (receive) is really done we should wait >> + * for mpc512x_lpbfifo_callback() to be called by DMA driver. >> + * In this case LPB FIFO completion irq will not appear at all. >> + * >> + * Moreover, freeing memory and dmaengine channel is not safe until >> + * mpc512x_lpbfifo_callback() is called. >> + * >> + * So to make it simple: >> + * last one out turns off the lights. >> + */ > Can this "feedback order issue" handling get simplified by having > both the LPB controller FIFO and the DMA completion callback > invoke a single routine, which tracks the "LPB done" and "DMA > done" conditions regardless of their order, and does > postprocessing when both were satisfied? > > It might be desirable to always run the postprocessing only if > both involved components finished their activities, regardless of > the transfer's direction. This reduces the potential for access > to invalid data if these two events are "racy". Excuse me, I didn't understand your idea. I thought my code is already quite simple. Postprocessing is short: just clean lpbfifo.req and run client's callback. DEV->MEM: only DMA callback appears. It should simply execute postprocessing. MEM->DEV: both DMA callback and LPBFIFO irq appear. Both of them should be done before postprocessing. I.e. the last one should "turn off the lights". Last one is determined using lpbfifo.im_last protected by spinlock. I guess my comment describing that is not clear enough, is it? >> + /* 1. Check requirements: */ >> + /* Packet size must be a multiple of 4 bytes since >> + * FIFO Data Word Register (which provides data to DMA controller) >> + * allows only "full-word" (4 bytes) access >> + * according Reference Manual */ > Are you certain about that constraint? Can't the packet size be > an "incomplete multiple" of the SCLPC FIFO port's width when the > last data item is appropriately aligned? The Reference Manual (page 538) says about FIFO Data Word Register: Only full-word access is allowed. If all byte enables are not asserted when accessing this location, a FIFO error flag is generated. > Since you can attach 8bit, 16bit, as well as 32bit wide > peripherals to the LPB (the external bus), I feel that insisting > in full 32bits getting transferred may be inappropriate. Unless > I'm missing something, and it's an SCLPC contraint while the > CPU's access to the LPB isn't limited. I think there is no contradiction between allowing only full-word access to SCLPC FIFO port and allowing different BPT (bytes per transfer) on LPBus at the same time. I guess it might mean that you should fill (empty) FIFO by 32bit portions from the processor side and empty (fill) it from the LPBus side by portions you want. Moreover the Reference Manual says that if you have some device with for example 8bit port on LPBus, you just should set BPT to 8 and set DAI (disable auto increment) bit to 1 in SCLPC Control Register. >> + /* 4. Set packet size and kick FIFO off */ >> + bits = req->size; >> + bits |= (1<<31); /* set restart bit */ >> + out_be32(lpbfifo.regs + LPBFIFO_REG_PACKET_SIZE, bits); > here you start the SCLPC transfer ... >> + >> + >> + /* 5. Then kick DMA off */ >> + cookie = dma_tx->tx_submit(dma_tx); >> + if (dma_submit_error(cookie)) { >> + pr_err("DMA tx_submit failed\n"); >> + e = -ENOSPC; >> + goto err_dma_submit; >> + } > ... and here you setup the DMA job -- isn't this too late? > (Please note that I haven't re-checked the "functional > description" section in the DMA controller's chapter.) I did it according "SCLPC Programming" chapter (21.3.3.1) in Reference Manual. >> + >> + /* (128 kBytes - 4 Bytes) is a maximum packet size >> + * that LPB FIFO and DMA controller can handle together >> + * while exchanging DEFAULT_WORDS_PER_TRANSFER = 1 >> + * per hardware request */ > Could you state what the individual limits are? > > I guess the SCLPC can transfer gigabytes, the FIFO depth either > should not matter or is dramatically lower than 128KB (1K?). Yes, that's true. > Is the DMA controller the limiting element, and how so? Does the > limit not depend on the biter and citer and width specs? DMA controller is the limiting element: maximum_packet_size = maximum_BITER_value * NBYTES. NBYTES of data is read from / written to SCLPC FIFO in one burst after DMA controller receives service request from SCLPC. But SCLPC wants NBYTES to be 4 bytes, because: thorough testing showed that DMA works with FIFO faster than SCLPC. During DEV->MEM transfer SCLPC FIFO high watermark (the level of data in FIFO at which SCLPC should send service request to DMA controller) must be equal to NBYTES. If not, DMA controller reading from FIFO overtakes SCLPC writing to FIFO and FIFO underflow error happens. Of course, making DEV->MEM transfer FIFO high watermark bigger than 4 bytes would make NBYTES and maximum_packet_size bigger. But in that case we would have to align packet size by something bigger than 4 bytes. I consider it more awkward for clients than 128 kBytes limit of packet size. I guess devices on LPBus (network switches, nvram, etc.) would not suffer from this packet size limit. What do you think? > Is the > limit mentioned here (in the LPB related code) assuming knowledge > of the DMA driver's internals? Yes, it is. It's not good, I should put size check to mpc512x_dma.c. >> + rc = request_irq(lpbfifo.irq, >> + mpc512x_lpbfifo_irq, 0, DRV_NAME, &lpbfifo); > whitespace (alignment of continuation) > (just noticed here, not explicitely checked elsewhere, unless > it's a misdetection caused by TABs and email quotation) There are only several tabs here, checkpatch.pl didn't complain. Which rule could I break? Thanks again, Gerhard. Best regards, Alexander
On Fri, Jul 12, 2013 at 14:42 +0400, Alexander Popov wrote: > > 2013/7/10 Gerhard Sittig <gsi@denx.de>: > > On Wed, Jul 10, 2013 at 14:21 +0400, Alexander Popov wrote: > >> > >> + > >> +struct mpc512x_lpbfifo_request { > >> + unsigned int cs; > >> + phys_addr_t bus_phys; /* physical address of some device on lpb */ > >> + void *ram_virt; /* virtual address of some region in ram */ > >> + > >> + /* Details of transfer */ > >> + u32 size; > >> + enum lpb_dev_portsize portsize; > >> + enum mpc512x_lpbfifo_req_dir dir; > >> + > >> + /* Call when the transfer is finished */ > >> + void (*callback)(struct mpc512x_lpbfifo_request *); > >> +}; > >> + > >> +extern int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req); > >> + > >> #endif /* __ASM_POWERPC_MPC5121_H__ */ > > > Needs the mpc512x_lpbfifo_request structure be part of the > > official API? Could it be desirable to hide it behind a > > "fill-in" routine? Which BTW could auto-determine CS numbers and > > port width associated with a chip select from the XLB and LPB > > register set? > > 1. As I wrote at the beginning of the file, the driver design > is based on mpc52xx_lpbfifo driver written by Grant Likely. > > 2. CS and port width are attributes of some device on LPBus > which participates DMA data transfer. It is the driver of this device > who knows them. I don't think so (I disagree with the "driver knows physical bus attributes" part). Bus width, endianess, timing in access, etc are all properties of a chip select. And the chip select is associated with an address range. See the CS part of the LPB controller, and the LAW part of the XLB. Given a physical address, everything else can get determined from inspecting the SoC's registers. Just think what the SoC does: "Someone", probably the CPU or a bus master like DMA, accesses an address, which happens to be within an address window, which happens to be connected to some bus and maybe map to a chip select, which happens to use the chip select's configuration for the activity on the associated bus -- upon memory access nobody needs to explicitly know whether SRAM or DRAM or IMMR or LPB aka EMB is involved, it's all hidden within the XLB logic. I feel that the SCLPC job description is an exception, and the need to specify a CS index is the unusual case. Drivers actually _need_not_ know the CS number or bus width of what's attached to the EMB. Those parameters usually get setup within boot loaders and the kernel need not care. Linux drivers merely map an address range and "just access memory that happens to have some arbitrary address". Drivers need not care whether the bus is internal or external nor whether the bus has several chip selects nor how these might be configured. The recent addition to re-configure a chip select already was a special case of differing configuration during runtime (when the firmware gets sent, and when the firmware is used and communicated to), and is by no means the normal case. > It fills mpc512x_lpbfifo_request as a client side of that API. > > > Who's using the submit routine? I might have missed the "client > > side" of that API in the series. > > Please look at https://patchwork.kernel.org/patch/2511951/ This answers how the current implementation is, not necessarily how the API should or might be. But I wasn't requesting immediate change, just was pointing at potential for improvement. > >> +#define LPBFIFO_REG_PACKET_SIZE (0x00) > >> +#define LPBFIFO_REG_START_ADDRESS (0x04) > >> +#define LPBFIFO_REG_CONTROL (0x08) > >> +#define LPBFIFO_REG_ENABLE (0x0C) > >> +#define LPBFIFO_REG_STATUS (0x14) > >> +#define LPBFIFO_REG_BYTES_DONE (0x18) > >> +#define LPBFIFO_REG_EMB_SHARE_COUNTER (0x1C) > >> +#define LPBFIFO_REG_EMB_PAUSE_CONTROL (0x20) > >> +#define LPBFIFO_REG_FIFO_DATA (0x40) > >> +#define LPBFIFO_REG_FIFO_STATUS (0x44) > >> +#define LPBFIFO_REG_FIFO_CONTROL (0x48) > >> +#define LPBFIFO_REG_FIFO_ALARM (0x4C) > > > Should this not be a struct? Since using member field names > > allows for compile time checks of data types, which is highly > > desirable with registers of arbitrarily differing size. > > I've read > https://lists.ozlabs.org/pipermail/linuxppc-dev/2010-January/079476.html > Which way should I use? The way Grant puts it, there are valid reasons for both approaches and things may turn out to be a matter of preference. As is written in the post you refer to, neither of them is in itself right or wrong and leads to immediate rejection, it's just that one of them appears more appropriate in a specific situation. Given that we are dealing with SoC internal IP peripherals, their physical attachment is known and stable, the register layout and data width is a given. The phrase out_be32(®s->field, val) is so much more readable than fiddling with a base and offsets. Data types and __iomem attributes are available at compile time, and I'm always glad when tools help me spot stupid mistakes and do so early. Looking at arch/powerpc/include/asm/mpc5121.h and the DMA driver you are changing, and seeing how the contra arguments do not apply here (new driver, no churn, known properties, prior art), the preference in this specific situations here is for structs. Other cultures may prefer something else. These defines just don't feel right here. > >> + > >> +#define DMA_LPC_CHANNEL_NUMBER 26 > >> +#define DEFAULT_WORDS_PER_TRANSFER 1 > > > can you eliminate the DMA channel number in the DMA client > > source? this shall be intimate knowledge of the DMA engine > > driver, or at least get specified in the device tree > > > BTW did Anatolij suggest OF based DMA channel lookup back in May, > > see commit e48fc15 and search for 'rx-tx' in the mxcmmc.c file > > Ok, thanks, I've found it. I'll make OF based DMA channel lookup. See my RFC series, it implements what Anatolij did and addresses the feedback he received. > >> +/* > >> + * Before we can wrap up handling current mpc512x_lpbfifo_request > >> + * and execute the callback registered in it we should: > >> + * 1. recognize that everything is really done, > >> + * 2. free memory and dmaengine channel. > >> + * > >> + * Writing from RAM to registers of some device on LPB (transmit) > >> + * is not really done until the LPB FIFO completion irq triggers. > >> + * > >> + * For being sure that writing from registers of some device on LPB > >> + * to RAM (receive) is really done we should wait > >> + * for mpc512x_lpbfifo_callback() to be called by DMA driver. > >> + * In this case LPB FIFO completion irq will not appear at all. > >> + * > >> + * Moreover, freeing memory and dmaengine channel is not safe until > >> + * mpc512x_lpbfifo_callback() is called. > >> + * > >> + * So to make it simple: > >> + * last one out turns off the lights. > >> + */ > > > Can this "feedback order issue" handling get simplified by having > > both the LPB controller FIFO and the DMA completion callback > > invoke a single routine, which tracks the "LPB done" and "DMA > > done" conditions regardless of their order, and does > > postprocessing when both were satisfied? > > > > It might be desirable to always run the postprocessing only if > > both involved components finished their activities, regardless of > > the transfer's direction. This reduces the potential for access > > to invalid data if these two events are "racy". > > Excuse me, I didn't understand your idea. > I thought my code is already quite simple. > > Postprocessing is short: just clean lpbfifo.req and run client's callback. > DEV->MEM: only DMA callback appears. It should simply execute postprocessing. > MEM->DEV: both DMA callback and LPBFIFO irq appear. Both of them > should be done before postprocessing. I.e. the last one should > "turn off the lights". Last one is determined using lpbfifo.im_last > protected by spinlock. > > I guess my comment describing that is not clear enough, is it? What I had in mind was something simple and straight forward instead of telling special cases apart when you don't have to. You start two parts which are SCLPC and DMA, and you can only tell that everything has completed when both are done. It just doesn't matter which is first, as both are required for completion. Just raise the "LPB done" and "DMA done" flags as these events arrive, and do the postprocessing once. Most important is to only implement the logic once, since duplication is not just tedious but introduces potential for error. BTW have there been reports in the past about hardware with "racy" signalling of completion. The signals may take different paths and might get delayed in arbitrary ways (both in hardware and in software). Assuming a specific order of events just complicates matters and introduces new potential for problems. The simplification and more importantly the unification of logic was what I suggested after the first look (there are two conditions which you try to handle with just one flag and guessing the order of events). Now upon second look I notice that you state there is no LPB completion information for the read direction. Is it that you don't wait for it, or is it that you don't get one? Is this an implementation detail, or because of the specific configuration used here? I'd be worried if this could not get controlled and simplified. Just try to get things more robust. Think of the next person who has to read and maintain the code (including yourself, after a few months have passed and you return from whatever you did in the meantime, having dropped all those details that may be present now but won't last). > >> + /* 1. Check requirements: */ > >> + /* Packet size must be a multiple of 4 bytes since > >> + * FIFO Data Word Register (which provides data to DMA controller) > >> + * allows only "full-word" (4 bytes) access > >> + * according Reference Manual */ > > > Are you certain about that constraint? Can't the packet size be > > an "incomplete multiple" of the SCLPC FIFO port's width when the > > last data item is appropriately aligned? > > The Reference Manual (page 538) says about FIFO Data Word Register: > Only full-word access is allowed. If all byte enables are not asserted > when accessing this location, a FIFO error flag is generated. Yes, I was confusing this with the PSC where the FIFO port is 32bits wide but can be accessed in arbitrary width. virtually yours Gerhard Sittig
Thanks, Gerhard. I'll improve the code and return with the third version. Best regards, Alexander.
diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi index bd14c00..6e0b0c0 100644 --- a/arch/powerpc/boot/dts/mpc5121.dtsi +++ b/arch/powerpc/boot/dts/mpc5121.dtsi @@ -261,7 +261,13 @@ /* LocalPlus controller */ lpc@10000 { compatible = "fsl,mpc5121-lpc"; - reg = <0x10000 0x200>; + reg = <0x10000 0x100>; + }; + + sclpc@10100 { + compatible = "fsl,mpc512x-lpbfifo"; + reg = <0x10100 0x50>; + interrupts = <7 0x8>; }; pata@10200 { diff --git a/arch/powerpc/include/asm/mpc5121.h b/arch/powerpc/include/asm/mpc5121.h index 8ae133e..f416a7e 100644 --- a/arch/powerpc/include/asm/mpc5121.h +++ b/arch/powerpc/include/asm/mpc5121.h @@ -69,4 +69,36 @@ struct mpc512x_lpc { int mpc512x_cs_config(unsigned int cs, u32 val); +/* + * SCLPC Module (LPB FIFO) + */ +enum lpb_dev_portsize { + LPB_DEV_PORTSIZE_UNDEFINED = 0, + LPB_DEV_PORTSIZE_1_BYTE = 1, + LPB_DEV_PORTSIZE_2_BYTES = 2, + LPB_DEV_PORTSIZE_4_BYTES = 4, + LPB_DEV_PORTSIZE_8_BYTES = 8, +}; + +enum mpc512x_lpbfifo_req_dir { + MPC512X_LPBFIFO_REQ_DIR_READ, + MPC512X_LPBFIFO_REQ_DIR_WRITE, +}; + +struct mpc512x_lpbfifo_request { + unsigned int cs; + phys_addr_t bus_phys; /* physical address of some device on lpb */ + void *ram_virt; /* virtual address of some region in ram */ + + /* Details of transfer */ + u32 size; + enum lpb_dev_portsize portsize; + enum mpc512x_lpbfifo_req_dir dir; + + /* Call when the transfer is finished */ + void (*callback)(struct mpc512x_lpbfifo_request *); +}; + +extern int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req); + #endif /* __ASM_POWERPC_MPC5121_H__ */ diff --git a/arch/powerpc/platforms/512x/Kconfig b/arch/powerpc/platforms/512x/Kconfig index fc9c1cb..0db8aa9 100644 --- a/arch/powerpc/platforms/512x/Kconfig +++ b/arch/powerpc/platforms/512x/Kconfig @@ -10,6 +10,12 @@ config PPC_MPC512x select USB_EHCI_BIG_ENDIAN_MMIO select USB_EHCI_BIG_ENDIAN_DESC +config PPC_MPC512x_LPBFIFO + tristate "MPC512x LocalPlus bus FIFO driver" + depends on PPC_MPC512x && MPC512X_DMA + help + Enable support for the Freescale MPC512x SCLPC. + config MPC5121_ADS bool "Freescale MPC5121E ADS" depends on PPC_MPC512x diff --git a/arch/powerpc/platforms/512x/Makefile b/arch/powerpc/platforms/512x/Makefile index 72fb934..df932fa 100644 --- a/arch/powerpc/platforms/512x/Makefile +++ b/arch/powerpc/platforms/512x/Makefile @@ -2,6 +2,7 @@ # Makefile for the Freescale PowerPC 512x linux kernel. # obj-y += clock.o mpc512x_shared.o +obj-$(CONFIG_PPC_MPC512x_LPBFIFO) += mpc512x_lpbfifo.o obj-$(CONFIG_MPC5121_ADS) += mpc5121_ads.o mpc5121_ads_cpld.o obj-$(CONFIG_MPC512x_GENERIC) += mpc512x_generic.o obj-$(CONFIG_PDM360NG) += pdm360ng.o diff --git a/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c b/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c new file mode 100644 index 0000000..6bd8aab --- /dev/null +++ b/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c @@ -0,0 +1,485 @@ +/* + * LocalPlus Bus SCLPC driver for the Freescale MPC512x. + * + * Copyright (C) Promcontroller, 2013. + * + * Author is Alexander Popov <a13xp0p0v88@gmail.com>. + * + * The driver design is based on mpc52xx_lpbfifo driver + * written by Grant Likely <grant.likely@secretlab.ca>. + * + * This file is released under the GPLv2. + */ + +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <asm/io.h> +#include <linux/spinlock.h> +#include <linux/slab.h> +#include <asm/mpc5121.h> +#include <linux/dmaengine.h> +#include <linux/dma-direction.h> +#include <linux/dma-mapping.h> + +MODULE_AUTHOR("Alexander Popov <a13xp0p0v88@gmail.com>"); +MODULE_DESCRIPTION("MPC512x LocalPlus FIFO device driver"); +MODULE_LICENSE("GPL"); + +#define DRV_NAME "mpc512x_lpbfifo" + +#define LPBFIFO_REG_PACKET_SIZE (0x00) +#define LPBFIFO_REG_START_ADDRESS (0x04) +#define LPBFIFO_REG_CONTROL (0x08) +#define LPBFIFO_REG_ENABLE (0x0C) +#define LPBFIFO_REG_STATUS (0x14) +#define LPBFIFO_REG_BYTES_DONE (0x18) +#define LPBFIFO_REG_EMB_SHARE_COUNTER (0x1C) +#define LPBFIFO_REG_EMB_PAUSE_CONTROL (0x20) +#define LPBFIFO_REG_FIFO_DATA (0x40) +#define LPBFIFO_REG_FIFO_STATUS (0x44) +#define LPBFIFO_REG_FIFO_CONTROL (0x48) +#define LPBFIFO_REG_FIFO_ALARM (0x4C) + +#define DMA_LPC_CHANNEL_NUMBER 26 +#define DEFAULT_WORDS_PER_TRANSFER 1 + +static struct mpc512x_lpbfifo { + struct device *dev; + struct resource res; + void __iomem *regs; + int irq; + spinlock_t lock; + + /* Current state data */ + int im_last; /* For "last one out turns off the lights" principle */ + struct mpc512x_lpbfifo_request *req; + dma_addr_t ram_bus_addr; + struct dma_chan *chan; +} lpbfifo; + +/* + * Before we can wrap up handling current mpc512x_lpbfifo_request + * and execute the callback registered in it we should: + * 1. recognize that everything is really done, + * 2. free memory and dmaengine channel. + * + * Writing from RAM to registers of some device on LPB (transmit) + * is not really done until the LPB FIFO completion irq triggers. + * + * For being sure that writing from registers of some device on LPB + * to RAM (receive) is really done we should wait + * for mpc512x_lpbfifo_callback() to be called by DMA driver. + * In this case LPB FIFO completion irq will not appear at all. + * + * Moreover, freeing memory and dmaengine channel is not safe until + * mpc512x_lpbfifo_callback() is called. + * + * So to make it simple: + * last one out turns off the lights. + */ + +/* + * mpc512x_lpbfifo_irq - IRQ handler for LPB FIFO + */ +static irqreturn_t mpc512x_lpbfifo_irq(int irq, void *dev_id) +{ + struct mpc512x_lpbfifo_request *req; + unsigned long flags; + u32 status; + + spin_lock_irqsave(&lpbfifo.lock, flags); + + req = lpbfifo.req; + if (!req) { + spin_unlock_irqrestore(&lpbfifo.lock, flags); + pr_err("bogus LPBFIFO IRQ\n"); + return IRQ_HANDLED; + } + + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_READ) { + spin_unlock_irqrestore(&lpbfifo.lock, flags); + pr_err("bogus LPBFIFO IRQ (we are waiting DMA IRQ)\n"); + return IRQ_HANDLED; + } + + /* Clear the interrupt flag */ + status = in_8(lpbfifo.regs + LPBFIFO_REG_STATUS); + if (status & 0x01) + out_8(lpbfifo.regs + LPBFIFO_REG_STATUS, 0x01); + else + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000); + + if (!lpbfifo.im_last) { + /* I'm not the last: DMA is still in progress. */ + lpbfifo.im_last = 1; + spin_unlock_irqrestore(&lpbfifo.lock, flags); + } else { + /* I'm last. Let's wrap up. */ + /* Set the FIFO as idle */ + req = lpbfifo.req; + lpbfifo.req = NULL; + + /* The spinlock must be dropped + * before executing the callback, + * otherwise we could end up with a deadlock + * or nested spinlock condition. */ + spin_unlock_irqrestore(&lpbfifo.lock, flags); + if (req->callback) + req->callback(req); + } + + return IRQ_HANDLED; +} + +/* + * mpc512x_lpbfifo_callback is called by DMA driver + * when DMA transaction is finished. + */ +static void mpc512x_lpbfifo_callback(void *param) +{ + unsigned long flags; + struct mpc512x_lpbfifo_request *req; + enum dma_data_direction dir; + + spin_lock_irqsave(&lpbfifo.lock, flags); + + /* Free resources */ + if (lpbfifo.req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) + dir = DMA_TO_DEVICE; + else + dir = DMA_FROM_DEVICE; + dma_unmap_single(lpbfifo.chan->device->dev, + lpbfifo.ram_bus_addr, lpbfifo.req->size, dir); + dma_release_channel(lpbfifo.chan); + + if (lpbfifo.req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE && + !lpbfifo.im_last) { + /* I'm not the last: LPB FIFO is still writing data. */ + lpbfifo.im_last = 1; + spin_unlock_irqrestore(&lpbfifo.lock, flags); + } else { + /* I'm the last or alone here. Let's wrap up. */ + /* Set the FIFO as idle */ + req = lpbfifo.req; + lpbfifo.req = NULL; + + /* The spinlock must be dropped + * before executing the callback, + * otherwise we could end up with a deadlock + * or nested spinlock condition. */ + spin_unlock_irqrestore(&lpbfifo.lock, flags); + if (req->callback) + req->callback(req); + } +} + +static bool channel_filter(struct dma_chan *chan, void *filter_param) +{ + if (chan->chan_id == DMA_LPC_CHANNEL_NUMBER) + return true; + else + return false; +} + +static int mpc512x_lpbfifo_kick(struct mpc512x_lpbfifo_request *req) +{ + u32 bits; + int no_incr = 0; + u32 bpt; + dma_cap_mask_t cap_mask; + dma_filter_fn fn = channel_filter; + struct dma_device *dma_dev = NULL; + struct scatterlist sg; + enum dma_data_direction dir; + struct dma_slave_config dma_conf = {}; + struct dma_async_tx_descriptor *dma_tx = NULL; + dma_cookie_t cookie; + int e; + + /* 1. Check requirements: */ + /* Packet size must be a multiple of 4 bytes since + * FIFO Data Word Register (which provides data to DMA controller) + * allows only "full-word" (4 bytes) access + * according Reference Manual */ + if (!IS_ALIGNED(req->size, 4)) { + e = -EINVAL; + goto err_align; + } + + /* Physical address of the device on LPB and packet size + * must be aligned/multiple of BPT (bytes per transaction) + * according Reference Manual */ + if (req->portsize != LPB_DEV_PORTSIZE_UNDEFINED) { + bpt = req->portsize; + no_incr = 1; + } else + bpt = DEFAULT_WORDS_PER_TRANSFER << 2; /* makes life easier */ + + if (!IS_ALIGNED(req->bus_phys | req->size, bpt)) { + e = -EFAULT; + goto err_align; + } + + /* 2. Prepare DMA */ + dma_cap_zero(cap_mask); + dma_cap_set(DMA_SLAVE, cap_mask); + lpbfifo.chan = dma_request_channel(cap_mask, fn, NULL); + if (!lpbfifo.chan) { + e = -ENODEV; + goto err_align; + } + dma_dev = lpbfifo.chan->device; + + sg_init_table(&sg, 1); + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) + dir = DMA_TO_DEVICE; + else + dir = DMA_FROM_DEVICE; + sg_dma_address(&sg) = dma_map_single(dma_dev->dev, + req->ram_virt, req->size, dir); + if (dma_mapping_error(dma_dev->dev, sg_dma_address(&sg))) { + pr_err("dma_mapping_error\n"); + e = -EFAULT; + goto err_dma_map; + } + lpbfifo.ram_bus_addr = sg_dma_address(&sg); /* will free it later */ + sg_dma_len(&sg) = req->size; + + /* We should limit the maximum number of words + * (units with FIFO Data Register size) + * that can be read from / written to the FIFO + * in one DMA burst. + * This measure and FIFO watermarks will prevent + * DMA controller from overtaking FIFO + * and causing FIFO underflow / overflow error. */ + dma_conf.dst_maxburst = DEFAULT_WORDS_PER_TRANSFER; + dma_conf.src_maxburst = DEFAULT_WORDS_PER_TRANSFER; + + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) { + dma_conf.direction = DMA_MEM_TO_DEV; + dma_conf.dst_addr = lpbfifo.res.start + LPBFIFO_REG_FIFO_DATA; + } else { + dma_conf.direction = DMA_DEV_TO_MEM; + dma_conf.src_addr = lpbfifo.res.start + LPBFIFO_REG_FIFO_DATA; + } + dma_conf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + dma_conf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + + /* Make DMA channel work with LPB FIFO data register */ + if (dma_dev->device_control(lpbfifo.chan, + DMA_SLAVE_CONFIG, (unsigned long)&dma_conf)) { + goto err_dma_prep; + } + + dma_tx = dmaengine_prep_slave_sg(lpbfifo.chan, &sg, + 1, dma_conf.direction, 0); + if (!dma_tx) { + pr_err("dmaengine_prep_slave_sg failed\n"); + e = -ENOSPC; + goto err_dma_prep; + } + + dma_tx->callback = mpc512x_lpbfifo_callback; + dma_tx->callback_param = NULL; + + /* 3. Prepare FIFO */ + /* Set and clear the reset bits; + * is good practice in Reference Manual */ + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000); + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x0); + + /* Configure the watermarks. + * + * RAM->DMA->FIFO->LPB_DEV (write): + * High watermark (7 * 4) free bytes (according Reference Manual) + * Low watermark 996 bytes (whole FIFO - 28 bytes) + * + * LPB_DEV->FIFO->DMA->RAM (read): + * High watermark (1024 - 4) free bytes + * (whole FIFO - DEFAULT_WORDS_PER_TRANSFER) + * Low watermark 0 bytes + */ + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) { + out_be16(lpbfifo.regs + LPBFIFO_REG_FIFO_CONTROL, 0x0700); + out_be32(lpbfifo.regs + LPBFIFO_REG_FIFO_ALARM, 0x000003e4); + } else { + out_be16(lpbfifo.regs + LPBFIFO_REG_FIFO_CONTROL, 0x0); + out_be32(lpbfifo.regs + LPBFIFO_REG_FIFO_ALARM, 0x000003fc); + } + + /* Start address is a physical address of the region + * which belongs to the device on localplus bus */ + out_be32(lpbfifo.regs + LPBFIFO_REG_START_ADDRESS, req->bus_phys); + + /* Configure chip select, transfer direction, + * address increment option and bytes per transfer option */ + bits = (req->cs & 0x7) << 24; + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_READ) + bits |= 3 << 16; /* read mode bit and flush bit */ + if (no_incr) + bits |= 1 << 8; + bits |= bpt & 0x3f; + out_be32(lpbfifo.regs + LPBFIFO_REG_CONTROL, bits); + + /* Unmask irqs */ + bits = 0x00000201; /* set error irq & master enabled bit */ + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE) + bits |= 0x00000100; /* set completion irq too */ + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, bits); + + /* 4. Set packet size and kick FIFO off */ + bits = req->size; + bits |= (1<<31); /* set restart bit */ + out_be32(lpbfifo.regs + LPBFIFO_REG_PACKET_SIZE, bits); + + + /* 5. Then kick DMA off */ + cookie = dma_tx->tx_submit(dma_tx); + if (dma_submit_error(cookie)) { + pr_err("DMA tx_submit failed\n"); + e = -ENOSPC; + goto err_dma_submit; + } + + return 0; + + err_dma_submit: + + err_dma_prep: + dma_unmap_single(dma_dev->dev, sg_dma_address(&sg), req->size, dir); + + err_dma_map: + sg_dma_address(&sg) = 0; + dma_release_channel(lpbfifo.chan); + + err_align: + return e; +} + +int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req) +{ + unsigned long flags; + int result = 0; + + if (!lpbfifo.regs) + return -ENODEV; + + spin_lock_irqsave(&lpbfifo.lock, flags); + + /* A transfer is in progress + * if the req pointer is already set */ + if (lpbfifo.req) { + spin_unlock_irqrestore(&lpbfifo.lock, flags); + return -EBUSY; + } + + /* (128 kBytes - 4 Bytes) is a maximum packet size + * that LPB FIFO and DMA controller can handle together + * while exchanging DEFAULT_WORDS_PER_TRANSFER = 1 + * per hardware request */ + if (req->size > 131068) { + spin_unlock_irqrestore(&lpbfifo.lock, flags); + return -ENOSPC; + } + + /* Setup the transfer */ + lpbfifo.im_last = 0; + lpbfifo.req = req; + + result = mpc512x_lpbfifo_kick(req); + if (result != 0) + lpbfifo.req = NULL; /* Set the FIFO as idle */ + + spin_unlock_irqrestore(&lpbfifo.lock, flags); + + return result; +} +EXPORT_SYMBOL(mpc512x_lpbfifo_submit); + +static int mpc512x_lpbfifo_probe(struct platform_device *pdev) +{ + struct resource *r = &lpbfifo.res; + int e = 0, rc = -ENOMEM; + + if (of_address_to_resource(pdev->dev.of_node, 0, r)) { + e = -ENODEV; + goto err_res; + } + + if (!request_mem_region(r->start, resource_size(r), DRV_NAME)) { + e = -EBUSY; + goto err_res; + } + + lpbfifo.irq = irq_of_parse_and_map(pdev->dev.of_node, 0); + if (!lpbfifo.irq) { + e = -ENODEV; + goto err_res; + } + + lpbfifo.regs = ioremap(r->start, resource_size(r)); + if (!lpbfifo.regs) { + e = -ENOMEM; + goto err_regs; + } + + spin_lock_init(&lpbfifo.lock); + + /* Put FIFO into reset state */ + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000); + + lpbfifo.dev = &pdev->dev; + + rc = request_irq(lpbfifo.irq, + mpc512x_lpbfifo_irq, 0, DRV_NAME, &lpbfifo); + if (rc) { + e = -ENODEV; + goto err_irq; + } + + return 0; + + err_irq: + iounmap(lpbfifo.regs); + lpbfifo.regs = NULL; + + err_regs: + release_mem_region(r->start, resource_size(r)); + + err_res: + dev_err(&pdev->dev, "mpc512x_lpbfifo_probe() failed\n"); + return e; +} + +static int mpc512x_lpbfifo_remove(struct platform_device *pdev) +{ + /* Put FIFO in reset */ + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000); + + free_irq(lpbfifo.irq, &lpbfifo); + iounmap(lpbfifo.regs); + release_mem_region(lpbfifo.res.start, resource_size(&lpbfifo.res)); + lpbfifo.regs = NULL; + lpbfifo.dev = NULL; + + return 0; +} + +static const struct of_device_id mpc512x_lpbfifo_match[] = { + { .compatible = "fsl,mpc512x-lpbfifo", }, + {}, +}; + +static struct platform_driver mpc512x_lpbfifo_driver = { + .probe = mpc512x_lpbfifo_probe, + .remove = mpc512x_lpbfifo_remove, + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + .of_match_table = mpc512x_lpbfifo_match, + }, +}; +module_platform_driver(mpc512x_lpbfifo_driver);
This is SCLPC device driver for the Freescale MPC512x. It is needed for Direct Memory Access to the devices on LocalPlus Bus. Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com> --- arch/powerpc/boot/dts/mpc5121.dtsi | 8 +- arch/powerpc/include/asm/mpc5121.h | 32 ++ arch/powerpc/platforms/512x/Kconfig | 6 + arch/powerpc/platforms/512x/Makefile | 1 + arch/powerpc/platforms/512x/mpc512x_lpbfifo.c | 485 ++++++++++++++++++++++++++ 5 files changed, 531 insertions(+), 1 deletion(-) create mode 100644 arch/powerpc/platforms/512x/mpc512x_lpbfifo.c