[RFC] using amba_pl08x driver for s3c64xx SoCs
Linus Walleij
linus.walleij at linaro.org
Fri Aug 5 04:48:18 EDT 2011
On Fri, Aug 5, 2011 at 7:47 AM, Alim Akhtar <alim.akhtar at gmail.com> wrote:
> Samsung's S3C64XX SoCs uses a modified version of standard PrimeCell PL080 DMAC,
> which has extra control register and config register. Hence the
> current mailline amba_pl08x driver can not be used as it is.
> I have made the requisite changes to the amba_pl08x driver to
> accommodate the S3C64XX DMAC.
Great!
Make sure you are following and rebasing and testing the latest changes from
Russell King and Viresh Kumar to the same driver, especially Vireshs
code removing the 0x400 (1K) limit to chunk size.
> Below is the modification, if you think it is ok I can submit the same
> as a patch.
Yes, generate a common patch from GIT with git format-patch then submit this to
linux-kernel, linux-arm-kernel and explicitly to the DMA engine maintainer
Vinod Koul.
I have some quick review comments below, also make sure you have run
checkpatch (you probably have, just checking) on it.
> + * Samsung S3C64xx SoCs uses a variant of PL080 DMAC. It contains an extra
> + * control register to hold the TransferSize. Below is the LLI structure
> + * and offsets of S3C64xx DMAC.
> + * -----------------------------------------------------------------
> + * | Offset | Contents |
> + * -----------------------------------------------------------------
> + * | Next LLI Address | Source Address for Next xfer |
> + * -----------------------------------------------------------------
> + * | Next LLI Address+0x04 | Destination Address for Next xfer |
> + * -----------------------------------------------------------------
> + * | Next LLI Address+0x08 | Next LLI address for next xfer |
> + * -----------------------------------------------------------------
> + * | Next LLI Address+0x0c | DMACCxControl0 data for next xfer |
> + * -----------------------------------------------------------------
> + * | Next LLI Address+0x10 | DMACCxControl1 xfer size for next xfer|
> + * -----------------------------------------------------------------
> + * Also S3C64XX has a config register at offset 0x14
> + * Have a look at arch/arm/include/asm/hardware/pl080.h for complete register
> + * details.
> */
Good!
Probably we should name the Samsung-specific registers in a special way,
but we can leave that for later.
> #include <linux/device.h>
> #include <linux/init.h>
> #include <linux/module.h>
> @@ -99,6 +117,8 @@
> struct vendor_data {
> u8 channels;
> bool dualmaster;
> + /* To identify samsung DMAC */
> + bool is_pl080s;
I would name it more explicitly like
is_pl080_s3c
> @@ -112,6 +132,9 @@ struct pl08x_lli {
> u32 dst;
> u32 lli;
> u32 cctl;
> + /* Samsung pl080 DMAC has one exrta control register
> + * which is used to hold the transfer_size */
> + u32 cctl1;
> };
Comment style
/*
* Foo
* Bar
*/
> @@ -452,7 +513,6 @@ static inline u32 pl08x_cctl_bits(u32 cctl, u8
> srcwidth, u8 dstwidth,
> size_t tsize)
> {
> u32 retbits = cctl;
> -
Refrain from removing whitespace here.
(No big deal)
> @@ -591,6 +650,7 @@ static int pl08x_fill_llis_for_desc(struct
> pl08x_driver_data *pl08x,
> size_t max_bytes_per_lli;
> size_t total_bytes = 0;
> struct pl08x_lli *llis_va;
> + size_t lli_len = 0, target_len, tsize, odd_bytes;
>
> txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT,
> &txd->llis_bus);
> @@ -657,7 +717,12 @@ static int pl08x_fill_llis_for_desc(struct
> pl08x_driver_data *pl08x,
> "less than a bus width (remain 0x%08x)\n",
> __func__, bd.remainder);
> cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
> - pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + if (pl08x->vd->is_pl080s)
> + pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + else {
> + cctl |= 1 << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
> + pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + }
Hm! What is happening here? That is not a S3C-specific register
in control transfer?
Please insert a comment explaining why you do this cctl |= ...
Also since the second statement is the same in both cases just:
if (pl08x->vd->is_pl080s)
/* Here is a detailed description of why we need to do this on S3C */
cctl |= 1 << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> @@ -668,7 +733,12 @@ static int pl08x_fill_llis_for_desc(struct
> pl08x_driver_data *pl08x,
> "(remain 0x%08x)\n",
> __func__, bd.remainder);
> cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
> - pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + if (pl08x->vd->is_pl080s)
> + pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + else {
> + cctl |= 1 << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
> + pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + }
> total_bytes++;
> }
Same here.
> @@ -779,8 +847,14 @@ static int pl08x_fill_llis_for_desc(struct
> pl08x_driver_data *pl08x,
> dev_vdbg(&pl08x->adev->dev,
> "%s fill lli with single lli chunk of size 0x%08zx (remainder 0x%08zx)\n",
> __func__, lli_len, bd.remainder);
> - pl08x_fill_lli_for_desc(&bd, num_llis++,
> - lli_len, cctl);
> + if (pl08x->vd->is_pl080s)
> + pl08x_fill_lli_for_desc(&bd, num_llis++,
> + lli_len, cctl);
> + else {
> + cctl |= tsize << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
> + pl08x_fill_lli_for_desc(&bd, num_llis++,
> + lli_len, cctl);
> + }
> total_bytes += lli_len;
> }
Same here.
> @@ -797,8 +871,14 @@ static int pl08x_fill_llis_for_desc(struct
> pl08x_driver_data *pl08x,
> dev_vdbg(&pl08x->adev->dev,
> "%s align with boundary, single byte (remain 0x%08zx)\n",
> __func__, bd.remainder);
> - pl08x_fill_lli_for_desc(&bd,
> - num_llis++, 1, cctl);
> + if (pl08x->vd->is_pl080s)
> + pl08x_fill_lli_for_desc(&bd,
> + num_llis++, 1, cctl);
> + else {
> + cctl |= 1 << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
> + pl08x_fill_lli_for_desc(&bd,
> + num_llis++, 1, cctl);
> + }
> total_bytes++;
> }
> }
And here again.
> @@ -812,7 +892,12 @@ static int pl08x_fill_llis_for_desc(struct
> pl08x_driver_data *pl08x,
> dev_vdbg(&pl08x->adev->dev,
> "%s align with boundary, single odd byte (remain %zu)\n",
> __func__, bd.remainder);
> - pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + if (pl08x->vd->is_pl080s)
> + pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + else {
> + cctl |= 1 << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
> + pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> + }
> total_bytes++;
> }
> }
And here.
> @@ -829,13 +914,15 @@ static int pl08x_fill_llis_for_desc(struct
> pl08x_driver_data *pl08x,
> __func__, (u32) MAX_NUM_TSFR_LLIS);
> return 0;
> }
> -
> llis_va = txd->llis_va;
> /* The final LLI terminates the LLI. */
> llis_va[num_llis - 1].lli = 0;
> /* The final LLI element shall also fire an interrupt. */
> llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
>
> + if(pl08x->vd->is_pl080s)
> + llis_va[num_llis - 1].cctl1 |= lli_len;
> +
Explanation here, again I think is needed.
> @@ -1920,6 +2008,13 @@ static int pl08x_probe(struct amba_device
> *adev, const struct amba_id *id)
> goto out_no_ioremap;
> }
>
> + clk = clk_get(&pl08x->adev->dev, "dma");
> + if (IS_ERR(clk)) {
> + dev_err(&pl08x->adev->dev, "Cannot get operation clock.\n");
> + ret = -EINVAL;
> + }
> + clk_enable(clk);
I suspect this is wrong. Since this is an AMBA device, you probably
can rely on the
code already present in drivers/amba/bus.c.
Just rename the clock from "dma" to "pclk" in the S3C platform.
> @@ -1953,7 +2048,7 @@ static int pl08x_probe(struct amba_device *adev,
> const struct amba_id *id)
> spin_lock_init(&ch->lock);
> ch->serving = NULL;
> ch->signal = -1;
> - dev_info(&adev->dev,
> + dev_dbg(&adev->dev,
> "physical channel %d is %s\n", i,
> pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");
> }
Check Russells and Vireshs recent patches, I think they have already
fixed up a lot of debug prints.
> @@ -2030,13 +2125,21 @@ out_no_pl08x:
> static struct vendor_data vendor_pl080 = {
> .channels = 8,
> .dualmaster = true,
> + .is_pl080s = false,
> };
Don't assign false, it is initialized to zero i.e. fals by default.
>
> static struct vendor_data vendor_pl081 = {
> .channels = 2,
> .dualmaster = false,
> + .is_pl080s = false,
> };
Dito.
The rest looks good to me!
Looking forward to version 2, thanks!
Yours,
Linus Walleij
More information about the linux-arm-kernel
mailing list