[RFC] using amba_pl08x driver for s3c64xx SoCs

Alim Akhtar alim.akhtar at gmail.com
Fri Aug 5 07:10:18 EDT 2011


On Fri, Aug 5, 2011 at 2:18 PM, Linus Walleij <linus.walleij at linaro.org> wrote:
> 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.
>
Ok, I will rebase and test against latest changes by Russell King and
Viresh Kumar.
>> 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.
>
Ok, I will do that.
> I have some quick review comments below, also make sure you have run
> checkpatch (you probably have, just checking) on it.
>
Ok, I will recheck and run checkpatch before sending.
>> + * 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!
>
Thanks.
> Probably we should name the Samsung-specific registers in a special way,
> but we can leave that for later.
>
Do you have anything in mind currently?
may be something like PL080_S3C_CH_CONFIG?
>>  #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
>
Ok, i will us this.
>> @@ -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
>  */
>
Ok, I will follow this.
>> @@ -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)
>
actually one line is missed here from the patch (dont know how, may be
the editor problem)
which looks like
@@ -489,7 +549,6 @@ static inline u32 pl08x_cctl_bits(u32 cctl, u8
srcwidth, u8 dstwidth,
 		break;
 	}

-	retbits |= tsize << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
 	return retbits;
since S3C64XX has a separate register for transfer size, I am making
pl08x_cctl_bits() to return only the control bits.
While calling pl08x_fill_lli_for_desc() i am doing cctl |= tsize <<
PL080_CONTROL_TRANSFER_SIZE_SHIFT for the other SoCs
except S3C64XX.

>> @@ -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 |= ...
>
As explained above.
I will put the comments in the patch.
> 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);
>
>
OK. I will rearrange this.
>> @@ -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.
>
OK. I will rearrange this
>> @@ -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.
>
OK. I will rearrange this
>> @@ -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.
>
OK. I will rearrange this
>> @@ -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.
>
OK. I will rearrange this
>> @@ -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.
>
I will write the explanation here.
>> @@ -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.
>
Ok. I will modify this.
>> @@ -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.
>
I have a concern here, let say if the dev_dbg is enabled, first call
to pl08x_phy_channel_busy() with ch->serving=NULL will give me NULL
pointer dereferencing crash.
please see pl08x_phy_channel_busy() definition.
Does this dbg really needed (or we can remove this), if yes, then how
to handle this case?
Please suggest.
>> @@ -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.
>
Ok. I will remove this.
>>
>>  static struct vendor_data vendor_pl081 = {
>>        .channels = 2,
>>        .dualmaster = false,
>> +       .is_pl080s = false,
>>  };
>
> Dito.
>
This as well
> The rest looks good to me!
>
> Looking forward to version 2, thanks!
>
I will send V2 soon
> Yours,
> Linus Walleij
>

Regards,
Alim



More information about the linux-arm-kernel mailing list