* PL-330 DMA driver
@ 2010-02-17 5:50 jassi brar
2010-02-17 7:07 ` Joonyoung Shim
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: jassi brar @ 2010-02-17 5:50 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
Many Samsung SoCs have PL-330 as the DMA controller and the driver
is missing in the mainline.
A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
based on the DMA API(drivers/dma/), but the patches weren't accepted.
Perhaps the driver should go into arch/arm/common/ ? Or inconspicuously
into arch/arm/plat-s3cxxxx/ ?
Could maintainers please suggest what is the best place for the PL330
controller driver?
Regards
^ permalink raw reply [flat|nested] 18+ messages in thread* PL-330 DMA driver 2010-02-17 5:50 PL-330 DMA driver jassi brar @ 2010-02-17 7:07 ` Joonyoung Shim 2010-02-17 9:45 ` jassi brar 2010-02-17 18:26 ` Linus Walleij 2010-02-18 6:24 ` Dan Williams 2 siblings, 1 reply; 18+ messages in thread From: Joonyoung Shim @ 2010-02-17 7:07 UTC (permalink / raw) To: linux-arm-kernel On 2/17/2010 2:50 PM, jassi brar wrote: > Hello, > > Many Samsung SoCs have PL-330 as the DMA controller and the driver > is missing in the mainline. > > A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330 > based on the DMA API(drivers/dma/), but the patches weren't accepted. > Perhaps the driver should go into arch/arm/common/ ? Or inconspicuously > into arch/arm/plat-s3cxxxx/ ? > First we need a decision about we use which dma api. I think it's better we use the DMA API, but i agree about using s3c dma api for compatibility of the existing s3c dma api too. > Could maintainers please suggest what is the best place for the PL330 > controller driver? > > Regards > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-17 7:07 ` Joonyoung Shim @ 2010-02-17 9:45 ` jassi brar 0 siblings, 0 replies; 18+ messages in thread From: jassi brar @ 2010-02-17 9:45 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 17, 2010 at 4:07 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote: > On 2/17/2010 2:50 PM, jassi brar wrote: >> Hello, >> >> ? Many Samsung SoCs have PL-330 as the DMA controller and the driver >> is missing in the mainline. >> >> A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330 >> based on the DMA API(drivers/dma/), but the patches weren't accepted. >> Perhaps the driver should go into arch/arm/common/ ? Or inconspicuously >> into arch/arm/plat-s3cxxxx/ ? > First we need a decision about we use which dma api. I think it's better > we use the DMA API, but i agree about using s3c dma api for > compatibility of the existing s3c dma api too. Also, PL330 is 'configurable' (one implementation can be different from the other) that too adds one reason for it to be in plat-samsung Still, I would like maintainers to suggest. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-17 5:50 PL-330 DMA driver jassi brar 2010-02-17 7:07 ` Joonyoung Shim @ 2010-02-17 18:26 ` Linus Walleij 2010-02-17 20:31 ` Russell King - ARM Linux 2010-02-18 6:24 ` Dan Williams 2 siblings, 1 reply; 18+ messages in thread From: Linus Walleij @ 2010-02-17 18:26 UTC (permalink / raw) To: linux-arm-kernel 2010/2/17 jassi brar <jassisinghbrar@gmail.com>: > Could maintainers please suggest what is the best place for the PL330 > controller driver? drivers/dma/amba-pl033.c would be my bet. I have patches to support MMCI (PL180/PL181) using the generic DMA engine pending, I will attempt to submit patches for spi/amba-pl022.c and serial/amba-pl011.c when I get the time. In drivers/dma/coh901318.c we found that implementing a generic DMA engine over some random custom DMA controller is actually quite straight forward. If you're making it custom I think seriously you're doing something wrong. The key to making the DMA controller wholly abstract is to configure the driver with platform data containing information on how to set up/tear down/refill/etc any DMA channel on the system, so the DMA engine knows all about all channels, and the driver using it just knows that it's requesting some rx or tx channel with some platform filter (all from platform data). This way replacing the DMA enginge, as in resynthesize your SoC with some totally different DMA controller, and the driver using it still works. There may be DMA controllers so esoteric that they cannot be handled by DMA engine, what do I know, I haven't seen one yet. Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-17 18:26 ` Linus Walleij @ 2010-02-17 20:31 ` Russell King - ARM Linux 2010-02-17 21:32 ` Guennadi Liakhovetski 2010-02-17 21:46 ` Linus Walleij 0 siblings, 2 replies; 18+ messages in thread From: Russell King - ARM Linux @ 2010-02-17 20:31 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 17, 2010 at 07:26:00PM +0100, Linus Walleij wrote: > 2010/2/17 jassi brar <jassisinghbrar@gmail.com>: > > > Could maintainers please suggest what is the best place for the PL330 > > controller driver? > > drivers/dma/amba-pl033.c would be my bet. > > I have patches to support MMCI (PL180/PL181) using the generic DMA > engine pending, I will attempt to submit patches for spi/amba-pl022.c and > serial/amba-pl011.c when I get the time. One of the problems with the DMA engine APIs are that the data needed to setup the non-device side is opaque - in other words, it's specific to the DMA engine being used. This means primecell drivers can't define this data; they don't know what kind of DMA engine will be used - and I'm not sure passing it in from the platform side of things makes much sense either. Unless... we define a base structure for DMA engines used with primecell peripherals... ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-17 20:31 ` Russell King - ARM Linux @ 2010-02-17 21:32 ` Guennadi Liakhovetski 2010-02-17 21:53 ` Linus Walleij 2010-02-17 21:46 ` Linus Walleij 1 sibling, 1 reply; 18+ messages in thread From: Guennadi Liakhovetski @ 2010-02-17 21:32 UTC (permalink / raw) To: linux-arm-kernel On Wed, 17 Feb 2010, Russell King - ARM Linux wrote: > On Wed, Feb 17, 2010 at 07:26:00PM +0100, Linus Walleij wrote: > > 2010/2/17 jassi brar <jassisinghbrar@gmail.com>: > > > > > Could maintainers please suggest what is the best place for the PL330 > > > controller driver? > > > > drivers/dma/amba-pl033.c would be my bet. > > > > I have patches to support MMCI (PL180/PL181) using the generic DMA > > engine pending, I will attempt to submit patches for spi/amba-pl022.c and > > serial/amba-pl011.c when I get the time. > > One of the problems with the DMA engine APIs are that the data needed to > setup the non-device side is opaque - in other words, it's specific to > the DMA engine being used. > > This means primecell drivers can't define this data; they don't know what > kind of DMA engine will be used - and I'm not sure passing it in from > the platform side of things makes much sense either. > > Unless... we define a base structure for DMA engines used with primecell > peripherals... Having implemented support for IDMAC on i.MX31 under drivers/dma and slave DMA support for drivers/dma/shdma.c, my current opinion is, that basically only for controllers, that provide generic DMA operations (memcpy, xor,...) it makes sense to put them under dmaengine, and even then your slave DMA support will have to struggle. If a DMA controller only supports slave type DMA (to / from onboard and external peripherals) it normally doesn't make sense to implement it using dmaengine API. Granted, inventing a good proprietary API might be difficult, but having done that reasonably well, you end up with an API, better suitable for your hardware. It might even make sense to implement generic functions using dmaengine API and slave DMA using hardware-specific extensions. Of course, there's also a problem of reusing peripheral drivers with different DMA controllers... my 2p. Thanks Guennadi --- Guennadi Liakhovetski ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-17 21:32 ` Guennadi Liakhovetski @ 2010-02-17 21:53 ` Linus Walleij 2010-02-18 1:14 ` jassi brar 0 siblings, 1 reply; 18+ messages in thread From: Linus Walleij @ 2010-02-17 21:53 UTC (permalink / raw) To: linux-arm-kernel 2010/2/17 Guennadi Liakhovetski <g.liakhovetski@gmx.de>: > Of course, there's also a problem of reusing peripheral drivers with > different DMA controllers... That's what I'm facing with MMCI and a host of other peripherals. We have to either: (1) Use something, i.e the DMA engine slave (2) Fork the driver entirely to some ste-mmci.c and delete all generic code. (Repeat for each vendor using a primecell!) (3) Sprinkle mmci.c with #ifdef ARCH_U300 for our DMA engine. Or, not so bad, atleast sprinkle the smallish part I have now broken out in mmci.c that deals with DMA transfers. I think the current patch does (1) and it actually works, then if it gets burdensome to others, using a generic DMA engine can be a special case of (3) since that works for us so we're on the right path in two cases and (2) doesn't look good to me. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-17 21:53 ` Linus Walleij @ 2010-02-18 1:14 ` jassi brar 0 siblings, 0 replies; 18+ messages in thread From: jassi brar @ 2010-02-18 1:14 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 18, 2010 at 6:53 AM, Linus Walleij <linus.ml.walleij@gmail.com> wrote: > 2010/2/17 Guennadi Liakhovetski <g.liakhovetski@gmx.de>: > >> Of course, there's also a problem of reusing peripheral drivers with >> different DMA controllers... > > That's what I'm facing with MMCI and a host of other peripherals. > > We have to either: > > (1) Use something, i.e the DMA engine slave > > (2) Fork the driver entirely to some ste-mmci.c and delete all generic > ?code. (Repeat for each vendor using a primecell!) > > (3) Sprinkle mmci.c with #ifdef ARCH_U300 for our DMA engine. > ?Or, not so bad, atleast sprinkle the smallish part I have now > ?broken out in mmci.c that deals with DMA transfers. > > I think the current patch does (1) and it actually works, then if > it gets burdensome to others, using a generic DMA engine can > be a special case of (3) since that works for us so we're on the right > path in two cases and (2) doesn't look good to me. > > Yours, > Linus Walleij CCing the S3C maintainer (Ben Dooks) to share his opinion. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-17 20:31 ` Russell King - ARM Linux 2010-02-17 21:32 ` Guennadi Liakhovetski @ 2010-02-17 21:46 ` Linus Walleij 1 sibling, 0 replies; 18+ messages in thread From: Linus Walleij @ 2010-02-17 21:46 UTC (permalink / raw) To: linux-arm-kernel 2010/2/17 Russell King - ARM Linux <linux@arm.linux.org.uk>: > One of the problems with the DMA engine APIs are that the data needed to > setup the non-device side is opaque - in other words, it's specific to > the DMA engine being used. > > This means primecell drivers can't define this data; they don't know what > kind of DMA engine will be used - and I'm not sure passing it in from > the platform side of things makes much sense either. > > Unless... we define a base structure for DMA engines used with primecell > peripherals... Yes I get the point. I haven't seen much such non-device side stuff that's actually needed for the driver to specify in the platforms I've seen. I would say that things like burst size, throttling mechanisms etc are not generic at all and that knowledge has to be tied to the channel and DMA engine, however the address and the width of the endpoint is very generic. For U300 we have two such parameters in the DMA engine config for each DMA channel. It's the memory address where the DMA transaction ends up, like (for PL180) this: const struct coh_dma_channel chan_config[U300_DMA_CHANNELS] = { .priority_high = 0, > .dev_addr = U300_MMCSD_BASE + 0x080, .param.config = COH901318_CX_CFG_CH_DISABLE | (...) COH901318_CX_CTRL_BURST_COUNT_32_BYTES | \ > COH901318_CX_CTRL_SRC_BUS_SIZE_32_BITS | \ COH901318_CX_CTRL_SRC_ADDR_INC_ENABLE | \ > COH901318_CX_CTRL_DST_BUS_SIZE_32_BITS | \ It would indeed be better if the physical .dev_addr for TX or RX could come in from the PrimeCell itself. This is the most obvious thing, I guess things like burst size could be equally relevant on some peripheral but haven't seen such a thing yet. Would it be good if I went in and defined some struct in include/linux/amba/bus.h like this: struct amba_dma_params { bool (*dma_filter)(struct dma_chan *chan, void *filter_param); void *dma_rx_param; void *dma_tx_param; dma_addr_t phy_rx_addr; dma_addr_t phy_tx_addr; int rx_width; int tx_width; }; dma_addr_t could just as well be struct resource (as the platform_device does it) if that is preferred. But why... Then we include that into e.g. struct mmci_platform_data etc. We could pass a subset of this for as the void * parameter to the dma_request_channel(), for example this: struct amba_dma_channel_request { void *dma_param; dma_addr_t phy_addr; int width; /* extend here with stuff needed to request PrimeCell channels */ }; It's also quite simple to make a pair of macros for wrapping the rx or tx part into an amba_dma_channel_request. In this case the DMA engine implementation has to know which channels are primecell channels and dereference these into amba_dma_params. This could be the defined base structure of DMA engines supporting primecells I guess. Otherwise we can require that all DMA engines that support PrimeCells also support an additional function call: dma_request_amba_channel(dma_cap_mask_t *mask, dma_filter_fn fn, struct amba_dma_channel_request *ar); I can make a quick fix on the MMCI driver as example for either path, but I need some rough consensus. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-17 5:50 PL-330 DMA driver jassi brar 2010-02-17 7:07 ` Joonyoung Shim 2010-02-17 18:26 ` Linus Walleij @ 2010-02-18 6:24 ` Dan Williams 2010-02-18 6:36 ` jassi brar ` (2 more replies) 2 siblings, 3 replies; 18+ messages in thread From: Dan Williams @ 2010-02-18 6:24 UTC (permalink / raw) To: linux-arm-kernel On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote: > Hello, > > ?Many Samsung SoCs have PL-330 as the DMA controller and the driver > is missing in the mainline. > > A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330 > based on the DMA API(drivers/dma/), but the patches weren't accepted. Looking back at the archives seems I missed this patch. So it was not rejected just overlooked. Do we want to move forward on that patch as it stands, or go a different direction? If it is resubmitted I will get it in the queue for 2.6.34. -- Dan ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-18 6:24 ` Dan Williams @ 2010-02-18 6:36 ` jassi brar 2010-02-18 10:32 ` Russell King - ARM Linux 2010-02-18 12:01 ` Joonyoung Shim 2 siblings, 0 replies; 18+ messages in thread From: jassi brar @ 2010-02-18 6:36 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 18, 2010 at 3:24 PM, Dan Williams <dan.j.williams@intel.com> wrote: > On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote: >> Hello, >> >> ?Many Samsung SoCs have PL-330 as the DMA controller and the driver >> is missing in the mainline. >> >> A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330 >> based on the DMA API(drivers/dma/), but the patches weren't accepted. > > Looking back at the archives seems I missed this patch. ?So it was not > rejected just overlooked. ?Do we want to move forward on that patch as > it stands, or go a different direction? ?If it is resubmitted I will > get it in the queue for 2.6.34. Great, but you might want to wait for the platform maintainer's (Ben Dooks) opinion as it is going to have impact on extant dma client drivers of the platform which so far shared one dma API across various SOCs. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-18 6:24 ` Dan Williams 2010-02-18 6:36 ` jassi brar @ 2010-02-18 10:32 ` Russell King - ARM Linux 2010-02-24 0:44 ` Ben Dooks 2010-02-18 12:01 ` Joonyoung Shim 2 siblings, 1 reply; 18+ messages in thread From: Russell King - ARM Linux @ 2010-02-18 10:32 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 17, 2010 at 11:24:28PM -0700, Dan Williams wrote: > On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote: > > Hello, > > > > ?Many Samsung SoCs have PL-330 as the DMA controller and the driver > > is missing in the mainline. > > > > A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330 > > based on the DMA API(drivers/dma/), but the patches weren't accepted. > > Looking back at the archives seems I missed this patch. So it was not > rejected just overlooked. Do we want to move forward on that patch as > it stands, or go a different direction? If it is resubmitted I will > get it in the queue for 2.6.34. If it's a generic ARM primecell, it's likely it will be used elsewhere as well (primecells are gaining popularity at the moment.) Having it not be specific to S3C would be a big advantage, unless of course everyone specifically wants to have divergent copies of the same code. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-18 10:32 ` Russell King - ARM Linux @ 2010-02-24 0:44 ` Ben Dooks 2010-02-24 8:31 ` Russell King - ARM Linux 0 siblings, 1 reply; 18+ messages in thread From: Ben Dooks @ 2010-02-24 0:44 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 18, 2010 at 10:32:03AM +0000, Russell King - ARM Linux wrote: > On Wed, Feb 17, 2010 at 11:24:28PM -0700, Dan Williams wrote: > > On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote: > > > Hello, > > > > > > ?Many Samsung SoCs have PL-330 as the DMA controller and the driver > > > is missing in the mainline. > > > > > > A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330 > > > based on the DMA API(drivers/dma/), but the patches weren't accepted. > > > > Looking back at the archives seems I missed this patch. So it was not > > rejected just overlooked. Do we want to move forward on that patch as > > it stands, or go a different direction? If it is resubmitted I will > > get it in the queue for 2.6.34. > > If it's a generic ARM primecell, it's likely it will be used elsewhere as > well (primecells are gaining popularity at the moment.) Having it not > be specific to S3C would be a big advantage, unless of course everyone > specifically wants to have divergent copies of the same code. Which is great, until you find out that the S3C PL080 isn't a vanilla PL080, and has extra registers. I'm in favour of some form of generic DMA API, but we're going to have to sort out whether it is dmaengine, dmaengine with some updates or something entirely new. The current dmaengine API doesn't have any support for circular buffer mangement, and from what I remeber of reading stuff last time, adding to the queue from a buffer done callback is a no-no. -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-24 0:44 ` Ben Dooks @ 2010-02-24 8:31 ` Russell King - ARM Linux 0 siblings, 0 replies; 18+ messages in thread From: Russell King - ARM Linux @ 2010-02-24 8:31 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 24, 2010 at 12:44:22AM +0000, Ben Dooks wrote: > Which is great, until you find out that the S3C PL080 isn't a vanilla > PL080, and has extra registers. We're not going to go hacking drivers to have multiple different DMA interfaces inside them just because SoC maintainers want their own private DMA interfaces. We need to come up with a better solution to this problem. As I've said, the reality is that ARM's primecell IP is starting to appear in multiple different SoCs, so we need to solve this problem now. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-18 6:24 ` Dan Williams 2010-02-18 6:36 ` jassi brar 2010-02-18 10:32 ` Russell King - ARM Linux @ 2010-02-18 12:01 ` Joonyoung Shim 2010-02-18 17:55 ` Linus Walleij 2 siblings, 1 reply; 18+ messages in thread From: Joonyoung Shim @ 2010-02-18 12:01 UTC (permalink / raw) To: linux-arm-kernel On 2/18/2010 3:24 PM, Dan Williams wrote: > On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote: >> Hello, >> >> ?any Samsung SoCs have PL-330 as the DMA controller and the driver >> is missing in the mainline. >> >> A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330 >> based on the DMA API(drivers/dma/), but the patches weren't accepted. > > Looking back at the archives seems I missed this patch. So it was not > rejected just overlooked. Do we want to move forward on that patch as > it stands, or go a different direction? If it is resubmitted I will > get it in the queue for 2.6.34. > You can find the prior patch of pl330 from the below url. http://patchwork.kernel.org/patch/47847/ If needs, i can resubmit patch based from async_tx repo after some test. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-18 12:01 ` Joonyoung Shim @ 2010-02-18 17:55 ` Linus Walleij 2010-02-23 12:14 ` Joonyoung Shim 2010-02-24 0:46 ` Ben Dooks 0 siblings, 2 replies; 18+ messages in thread From: Linus Walleij @ 2010-02-18 17:55 UTC (permalink / raw) To: linux-arm-kernel 2010/2/18 Joonyoung Shim <jy0922.shim@samsung.com>: > You can find the prior patch of pl330 from the below url. > http://patchwork.kernel.org/patch/47847/ This one has a number of review issues, I'll put them in here, hope you can fix them for the next patch if you're at it now: +#include <linux/platform_device.h> No, please, #include <linux/amba/bus.h> instead. That's how we register PrimeCells. More on that at the end. +#include <plat/dma.h> Move that dma.h to include/linux/amba/pl330.h and include as #include <linux/amba/pl330.h> And also include it in the patch or we have no chance to know how struct pl330_platform_data looks (it is used a lot in the driver). +static unsigned int pl330_get_reg(struct pl330_device *pl330_dev, + unsigned int reg) +{ + void __iomem *base = pl330_dev->reg_base; + + return readl(base + reg); +} + +static void pl330_set_reg(struct pl330_device *pl330_dev, unsigned int reg, + unsigned int val) +{ + void __iomem *base = pl330_dev->reg_base; + + writel(val, base + reg); +} Is this kind of abstraction really useful? Isn't it easier in any case to just writel(FOO, pl330_dev->reg_base + BAR); ? In case you really keep them, make them inline. +static void pl330_dump_regs(struct pl330_chan *pl330_ch) +{ + struct device *dev = pl330_ch->pl330_dev->common.dev; + unsigned int val; + unsigned int id = pl330_ch->id; + + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DS); + dev_dbg(dev, "PL330_DS:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DPC); + dev_dbg(dev, "PL330_DPC:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTEN); + dev_dbg(dev, "PL330_INTEN:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_ES); + dev_dbg(dev, "PL330_ES:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTSTATUS); + dev_dbg(dev, "PL330_INTSTATUS:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSM); + dev_dbg(dev, "PL330_FSM:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSC); + dev_dbg(dev, "PL330_FSC:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTM); + dev_dbg(dev, "PL330_FTM:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTC(id)); + dev_dbg(dev, "PL330_FTC(%d):\t\t0x%08x\n", id, val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CS(id)); + dev_dbg(dev, "PL330_CS(%d):\t\t0x%08x\n", id, val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CPC(id)); + dev_dbg(dev, "PL330_CPC(%d):\t\t0x%08x\n", id, val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_SA(id)); + dev_dbg(dev, "PL330_SA(%d):\t\t0x%08x\n", id, val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DA(id)); + dev_dbg(dev, "PL330_DA(%d):\t\t0x%08x\n", id, val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CC(id)); + dev_dbg(dev, "PL330_CC(%d):\t\t0x%08x\n", id, val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC0(id)); + dev_dbg(dev, "PL330_LC0(%d):\t\t0x%08x\n", id, val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC1(id)); + dev_dbg(dev, "PL330_LC1(%d):\t\t0x%08x\n", id, val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DBGSTATUS); + dev_dbg(dev, "PL330_DBGSTATUS:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR0); + dev_dbg(dev, "PL330_CR0:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR1); + dev_dbg(dev, "PL330_CR1:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR2); + dev_dbg(dev, "PL330_CR2:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR3); + dev_dbg(dev, "PL330_CR3:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR4); + dev_dbg(dev, "PL330_CR4:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CRDN); + dev_dbg(dev, "PL330_CRDN:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID0); + dev_dbg(dev, "PL330_PERIPH_ID0:\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID1); + dev_dbg(dev, "PL330_PERIPH_ID1:\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID2); + dev_dbg(dev, "PL330_PERIPH_ID2:\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID3); + dev_dbg(dev, "PL330_PERIPH_ID3:\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID0); + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID1); + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID2); + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID3); + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); +} Turn this into a table. struct pl330_regdump { char *name; u16 reg; } static const struct pl330_regdump dumpregs[] = { { .name = "PL330_DS", .reg = PL330_DS, }, (....) }; int i; for (i = 0; i < ARRAY_SIZE(dumpregs); i++) { struct pl330_regdump *rd = &dumpregs[i]; dev_dbg(dev, "%s:\t\t0x%08x\n", rd->name, readl(pl330_ch->pl330_dev->base + rd->reg)); } Easy! (Beware of bugs in above code, just typing...) +/* instruction set functions */ (...) All these inlines make me think of serious rollerskating races. Are they really necessary? + if (loop_size_rest) + dev_dbg(dev, "TODO\n"); Hm. Perhaps this can be a bit more descriptive... +static struct dma_async_tx_descriptor * +pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, + unsigned int sg_len, enum dma_data_direction direction, + unsigned long flags) +{ + struct pl330_chan *pl330_ch = to_pl330_chan(chan); + struct pl330_register_cc *pl330_reg_cc = &pl330_ch->pl330_reg_cc; + struct pl330_dma_slave *dma_slave = chan->private; + struct pl330_desc *desc; + struct scatterlist *sg; + unsigned int inst_size = 0; + unsigned int i; + + BUG_ON(!dma_slave); + BUG_ON(direction == DMA_BIDIRECTIONAL); Does the PL330 really prohibit bidirectional channels? +static int pl330_probe(struct platform_device *pdev) Add __init macro. +static int pl330_remove(struct platform_device *pdev) Add __exit macro. +static struct platform_driver pl330_driver = { + .driver = { + .owner = THIS_MODULE, + .name = "pl330", + }, + .probe = pl330_probe, + .remove = pl330_remove, +}; This should be converted into an amba_device per <linux/amba/bus.h> as for all other primecells. That inevitably involves changing the probe and remove code a bit, and to register it differently, but we'll be thankful. (See any other PrimeCell driver for examples, e.g. drivers/spi/amba-pl022.c or drivers/mmc/host/mmci.c) Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-18 17:55 ` Linus Walleij @ 2010-02-23 12:14 ` Joonyoung Shim 2010-02-24 0:46 ` Ben Dooks 1 sibling, 0 replies; 18+ messages in thread From: Joonyoung Shim @ 2010-02-23 12:14 UTC (permalink / raw) To: linux-arm-kernel On 2/19/2010 2:55 AM, Linus Walleij wrote: > 2010/2/18 Joonyoung Shim <jy0922.shim@samsung.com>: > >> You can find the prior patch of pl330 from the below url. >> http://patchwork.kernel.org/patch/47847/ > > This one has a number of review issues, I'll put them in here, hope > you can fix them for the next patch if you're at it now: > > +#include <linux/platform_device.h> > > No, please, #include <linux/amba/bus.h> instead. That's how > we register PrimeCells. More on that at the end. > OK, i will convert to amba_device it. > +#include <plat/dma.h> > > Move that dma.h to include/linux/amba/pl330.h and include as > #include <linux/amba/pl330.h> > And also include it in the patch or we have no chance to know > how struct pl330_platform_data looks (it is used a lot in the > driver). > OK, i did this because of some plat specific define. I will move and split it. > +static unsigned int pl330_get_reg(struct pl330_device *pl330_dev, > + unsigned int reg) > +{ > + void __iomem *base = pl330_dev->reg_base; > + > + return readl(base + reg); > +} > + > +static void pl330_set_reg(struct pl330_device *pl330_dev, unsigned int reg, > + unsigned int val) > +{ > + void __iomem *base = pl330_dev->reg_base; > + > + writel(val, base + reg); > +} > > Is this kind of abstraction really useful? Isn't it easier in any case to > just writel(FOO, pl330_dev->reg_base + BAR); ? > In case you really keep them, make them inline. > I think too the abstraction is unnecessary. > +static void pl330_dump_regs(struct pl330_chan *pl330_ch) > +{ > + struct device *dev = pl330_ch->pl330_dev->common.dev; > + unsigned int val; > + unsigned int id = pl330_ch->id; > + > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DS); > + dev_dbg(dev, "PL330_DS:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DPC); > + dev_dbg(dev, "PL330_DPC:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTEN); > + dev_dbg(dev, "PL330_INTEN:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_ES); > + dev_dbg(dev, "PL330_ES:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTSTATUS); > + dev_dbg(dev, "PL330_INTSTATUS:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSM); > + dev_dbg(dev, "PL330_FSM:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSC); > + dev_dbg(dev, "PL330_FSC:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTM); > + dev_dbg(dev, "PL330_FTM:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTC(id)); > + dev_dbg(dev, "PL330_FTC(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CS(id)); > + dev_dbg(dev, "PL330_CS(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CPC(id)); > + dev_dbg(dev, "PL330_CPC(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_SA(id)); > + dev_dbg(dev, "PL330_SA(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DA(id)); > + dev_dbg(dev, "PL330_DA(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CC(id)); > + dev_dbg(dev, "PL330_CC(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC0(id)); > + dev_dbg(dev, "PL330_LC0(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC1(id)); > + dev_dbg(dev, "PL330_LC1(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DBGSTATUS); > + dev_dbg(dev, "PL330_DBGSTATUS:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR0); > + dev_dbg(dev, "PL330_CR0:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR1); > + dev_dbg(dev, "PL330_CR1:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR2); > + dev_dbg(dev, "PL330_CR2:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR3); > + dev_dbg(dev, "PL330_CR3:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR4); > + dev_dbg(dev, "PL330_CR4:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CRDN); > + dev_dbg(dev, "PL330_CRDN:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID0); > + dev_dbg(dev, "PL330_PERIPH_ID0:\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID1); > + dev_dbg(dev, "PL330_PERIPH_ID1:\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID2); > + dev_dbg(dev, "PL330_PERIPH_ID2:\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID3); > + dev_dbg(dev, "PL330_PERIPH_ID3:\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID0); > + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID1); > + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID2); > + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID3); > + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); > +} > > Turn this into a table. > > struct pl330_regdump { > char *name; > u16 reg; > } > > static const struct pl330_regdump dumpregs[] = { > { > .name = "PL330_DS", > .reg = PL330_DS, > }, > (....) > }; > > int i; > for (i = 0; i < ARRAY_SIZE(dumpregs); i++) { > struct pl330_regdump *rd = &dumpregs[i]; > dev_dbg(dev, "%s:\t\t0x%08x\n", > rd->name, > readl(pl330_ch->pl330_dev->base + rd->reg)); > } > > Easy! (Beware of bugs in above code, just typing...) > OK, i will change it. > +/* instruction set functions */ > (...) > > All these inlines make me think of serious rollerskating races. > Are they really necessary? > If inline is a problem, i can remove inline. > + if (loop_size_rest) > + dev_dbg(dev, "TODO\n"); > > Hm. Perhaps this can be a bit more descriptive... > Yes, this is thing to implement additionally, i will add more description. > +static struct dma_async_tx_descriptor * > +pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > + unsigned int sg_len, enum dma_data_direction direction, > + unsigned long flags) > +{ > + struct pl330_chan *pl330_ch = to_pl330_chan(chan); > + struct pl330_register_cc *pl330_reg_cc = &pl330_ch->pl330_reg_cc; > + struct pl330_dma_slave *dma_slave = chan->private; > + struct pl330_desc *desc; > + struct scatterlist *sg; > + unsigned int inst_size = 0; > + unsigned int i; > + > + BUG_ON(!dma_slave); > + BUG_ON(direction == DMA_BIDIRECTIONAL); > > Does the PL330 really prohibit bidirectional channels? > > +static int pl330_probe(struct platform_device *pdev) > > Add __init macro. > OK. > +static int pl330_remove(struct platform_device *pdev) > > Add __exit macro. > OK. > +static struct platform_driver pl330_driver = { > + .driver = { > + .owner = THIS_MODULE, > + .name = "pl330", > + }, > + .probe = pl330_probe, > + .remove = pl330_remove, > +}; > > This should be converted into an amba_device per <linux/amba/bus.h> as for > all other primecells. That inevitably involves changing the probe and > remove code a bit, and to register it differently, but we'll be thankful. > (See any other PrimeCell driver for examples, e.g. drivers/spi/amba-pl022.c > or drivers/mmc/host/mmci.c) > I see, i will refer other primecell drivers. I will fix above things at the next patch, but i cannot start right now it, will try to do it as soon as possible. Thanks for your review. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PL-330 DMA driver 2010-02-18 17:55 ` Linus Walleij 2010-02-23 12:14 ` Joonyoung Shim @ 2010-02-24 0:46 ` Ben Dooks 1 sibling, 0 replies; 18+ messages in thread From: Ben Dooks @ 2010-02-24 0:46 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 18, 2010 at 06:55:54PM +0100, Linus Walleij wrote: > 2010/2/18 Joonyoung Shim <jy0922.shim@samsung.com>: > > > You can find the prior patch of pl330 from the below url. > > http://patchwork.kernel.org/patch/47847/ > > This one has a number of review issues, I'll put them in here, hope > you can fix them for the next patch if you're at it now: > > +#include <linux/platform_device.h> > > No, please, #include <linux/amba/bus.h> instead. That's how > we register PrimeCells. More on that at the end. > > +#include <plat/dma.h> > > Move that dma.h to include/linux/amba/pl330.h and include as > #include <linux/amba/pl330.h> > And also include it in the patch or we have no chance to know > how struct pl330_platform_data looks (it is used a lot in the > driver). > > +static unsigned int pl330_get_reg(struct pl330_device *pl330_dev, > + unsigned int reg) > +{ > + void __iomem *base = pl330_dev->reg_base; > + > + return readl(base + reg); > +} > + > +static void pl330_set_reg(struct pl330_device *pl330_dev, unsigned int reg, > + unsigned int val) > +{ > + void __iomem *base = pl330_dev->reg_base; > + > + writel(val, base + reg); > +} > > Is this kind of abstraction really useful? Isn't it easier in any case to > just writel(FOO, pl330_dev->reg_base + BAR); ? > In case you really keep them, make them inline. > > +static void pl330_dump_regs(struct pl330_chan *pl330_ch) > +{ > + struct device *dev = pl330_ch->pl330_dev->common.dev; > + unsigned int val; > + unsigned int id = pl330_ch->id; > + > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DS); > + dev_dbg(dev, "PL330_DS:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DPC); > + dev_dbg(dev, "PL330_DPC:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTEN); > + dev_dbg(dev, "PL330_INTEN:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_ES); > + dev_dbg(dev, "PL330_ES:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTSTATUS); > + dev_dbg(dev, "PL330_INTSTATUS:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSM); > + dev_dbg(dev, "PL330_FSM:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSC); > + dev_dbg(dev, "PL330_FSC:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTM); > + dev_dbg(dev, "PL330_FTM:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTC(id)); > + dev_dbg(dev, "PL330_FTC(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CS(id)); > + dev_dbg(dev, "PL330_CS(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CPC(id)); > + dev_dbg(dev, "PL330_CPC(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_SA(id)); > + dev_dbg(dev, "PL330_SA(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DA(id)); > + dev_dbg(dev, "PL330_DA(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CC(id)); > + dev_dbg(dev, "PL330_CC(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC0(id)); > + dev_dbg(dev, "PL330_LC0(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC1(id)); > + dev_dbg(dev, "PL330_LC1(%d):\t\t0x%08x\n", id, val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DBGSTATUS); > + dev_dbg(dev, "PL330_DBGSTATUS:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR0); > + dev_dbg(dev, "PL330_CR0:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR1); > + dev_dbg(dev, "PL330_CR1:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR2); > + dev_dbg(dev, "PL330_CR2:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR3); > + dev_dbg(dev, "PL330_CR3:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR4); > + dev_dbg(dev, "PL330_CR4:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CRDN); > + dev_dbg(dev, "PL330_CRDN:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID0); > + dev_dbg(dev, "PL330_PERIPH_ID0:\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID1); > + dev_dbg(dev, "PL330_PERIPH_ID1:\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID2); > + dev_dbg(dev, "PL330_PERIPH_ID2:\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID3); > + dev_dbg(dev, "PL330_PERIPH_ID3:\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID0); > + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID1); > + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID2); > + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); > + val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID3); > + dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val); > +} > > Turn this into a table. > > struct pl330_regdump { > char *name; > u16 reg; > } > > static const struct pl330_regdump dumpregs[] = { > { > .name = "PL330_DS", > .reg = PL330_DS, > }, > (....) > }; > > int i; > for (i = 0; i < ARRAY_SIZE(dumpregs); i++) { > struct pl330_regdump *rd = &dumpregs[i]; > dev_dbg(dev, "%s:\t\t0x%08x\n", > rd->name, > readl(pl330_ch->pl330_dev->base + rd->reg)); > } > > Easy! (Beware of bugs in above code, just typing...) > > +/* instruction set functions */ > (...) > > All these inlines make me think of serious rollerskating races. > Are they really necessary? > > + if (loop_size_rest) > + dev_dbg(dev, "TODO\n"); > > Hm. Perhaps this can be a bit more descriptive... > > +static struct dma_async_tx_descriptor * > +pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > + unsigned int sg_len, enum dma_data_direction direction, > + unsigned long flags) > +{ > + struct pl330_chan *pl330_ch = to_pl330_chan(chan); > + struct pl330_register_cc *pl330_reg_cc = &pl330_ch->pl330_reg_cc; > + struct pl330_dma_slave *dma_slave = chan->private; > + struct pl330_desc *desc; > + struct scatterlist *sg; > + unsigned int inst_size = 0; > + unsigned int i; > + > + BUG_ON(!dma_slave); > + BUG_ON(direction == DMA_BIDIRECTIONAL); > > Does the PL330 really prohibit bidirectional channels? > > +static int pl330_probe(struct platform_device *pdev) > > Add __init macro. > > +static int pl330_remove(struct platform_device *pdev) > > Add __exit macro. > > +static struct platform_driver pl330_driver = { > + .driver = { > + .owner = THIS_MODULE, > + .name = "pl330", > + }, > + .probe = pl330_probe, > + .remove = pl330_remove, > +}; > > This should be converted into an amba_device per <linux/amba/bus.h> as for > all other primecells. That inevitably involves changing the probe and > remove code a bit, and to register it differently, but we'll be thankful. > (See any other PrimeCell driver for examples, e.g. drivers/spi/amba-pl022.c > or drivers/mmc/host/mmci.c) I don't see any real reason to go and use amba devices, especially as this'll be one of the few on many of the s3c/s5p platforms. > Yours, > Linus Walleij > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2010-02-24 8:31 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-17 5:50 PL-330 DMA driver jassi brar 2010-02-17 7:07 ` Joonyoung Shim 2010-02-17 9:45 ` jassi brar 2010-02-17 18:26 ` Linus Walleij 2010-02-17 20:31 ` Russell King - ARM Linux 2010-02-17 21:32 ` Guennadi Liakhovetski 2010-02-17 21:53 ` Linus Walleij 2010-02-18 1:14 ` jassi brar 2010-02-17 21:46 ` Linus Walleij 2010-02-18 6:24 ` Dan Williams 2010-02-18 6:36 ` jassi brar 2010-02-18 10:32 ` Russell King - ARM Linux 2010-02-24 0:44 ` Ben Dooks 2010-02-24 8:31 ` Russell King - ARM Linux 2010-02-18 12:01 ` Joonyoung Shim 2010-02-18 17:55 ` Linus Walleij 2010-02-23 12:14 ` Joonyoung Shim 2010-02-24 0:46 ` Ben Dooks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).