[PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[alim.akhtar@gmail.com (Alim Akhtar)]

HI Linus,
Thanks for reviewing again.

On Wed, Sep 28, 2011 at 1:31 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Sorry if I missed a few nitpicks last time, anyway it's looking much better now:
>
> On Wed, Sep 28, 2011 at 7:50 AM, Alim Akhtar <alim.akhtar@samsung.com> wrote:
>> + ? ? ? /*
>> + ? ? ? ?* Samsung pl080 DMAC has one exrta control register
>
> s/exrta/exstra
>
I will correct this.
>> + ? ? ? if (pl08x->vd->is_pl080_s3c) {
>> + ? ? ? ? ? ? ? writel(txd->ccfg, phychan->base + PL080S_CH_CONFIG);
>> + ? ? ? ? ? ? ? writel(lli->cctl1, phychan->base + PL080S_CH_CONTROL2);
>> + ? ? ? } else
>> + ? ? ? ? ? ? ? writel(txd->ccfg, phychan->base + PL080_CH_CONFIG);
>
> What do you think about adding a field to the struct pl08x
> like
>
> u32 config_reg
>
> that we set to the proper config register (PL080S_CH_CONFIG or
> PL080_CH_CONFIG in probe(), so the above
> becomes the simpler variant:
>
> writel(txd->ccfg, phychan->base + pl08x->config_reg);
> if (pl08x->vd->is_pl080_s3c)
> ? ?writel(lli->cctl1, phychan->base + PL080S_CH_CONTROL2);
>
I will try implement this way or as viresh has pointed out to remove
the code duplications.

>> + ? ? ? if (pl08x->vd->is_pl080_s3c) {
>> + ? ? ? ? ? ? ? val = readl(phychan->base + PL080S_CH_CONFIG);
>> + ? ? ? ? ? ? ? while ((val & PL080_CONFIG_ACTIVE) ||
>> + ? ? ? ? ? ? ? ? ? ? ? (val & PL080_CONFIG_ENABLE))
>> + ? ? ? ? ? ? ? ? ? ? ? val = readl(phychan->base + PL080S_CH_CONFIG);
>> +
>> + ? ? ? ? ? ? ? writel(val | PL080_CONFIG_ENABLE,
>> + ? ? ? ? ? ? ? ? ? ? ? phychan->base + PL080S_CH_CONFIG);
>> + ? ? ? } else {
>> ? ? ? ? ? ? ? ?val = readl(phychan->base + PL080_CH_CONFIG);
>> + ? ? ? ? ? ? ? ? ? ? ? while ((val & PL080_CONFIG_ACTIVE) ||
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (val & PL080_CONFIG_ENABLE))
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? val = readl(phychan->base + PL080_CH_CONFIG);
>>
>> - ? ? ? writel(val | PL080_CONFIG_ENABLE, phychan->base + PL080_CH_CONFIG);
>> + ? ? ? ? ? ? ? writel(val | PL080_CONFIG_ENABLE,
>> + ? ? ? ? ? ? ? ? ? ? ? phychan->base + PL080_CH_CONFIG);
>> + ? ? ? }
>
> This would also become much simpler with that approach I think...
>
OK, i will do that.
>> ? ? ? ?/* Set the HALT bit and wait for the FIFO to drain */
>> - ? ? ? val = readl(ch->base + PL080_CH_CONFIG);
>> - ? ? ? val |= PL080_CONFIG_HALT;
>> - ? ? ? writel(val, ch->base + PL080_CH_CONFIG);
>> -
>> + ? ? ? if (pl08x->vd->is_pl080_s3c) {
>> + ? ? ? ? ? ? ? val = readl(ch->base + PL080S_CH_CONFIG);
>> + ? ? ? ? ? ? ? val |= PL080_CONFIG_HALT;
>> + ? ? ? ? ? ? ? writel(val, ch->base + PL080S_CH_CONFIG);
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? val = readl(ch->base + PL080_CH_CONFIG);
>> + ? ? ? ? ? ? ? val |= PL080_CONFIG_HALT;
>> + ? ? ? ? ? ? ? writel(val, ch->base + PL080_CH_CONFIG);
>> + ? ? ? }
>
> This would become simply:
>
> val = readl(ch->base + pl08x->config_reg);
> val |= PL080_CONFIG_HALT;
> writel(val, ch->base + pl08x->config_reg);
>
>
ok, i will do that.
>> ? ? ? ?/* Clear the HALT bit */
>> - ? ? ? val = readl(ch->base + PL080_CH_CONFIG);
>> - ? ? ? val &= ~PL080_CONFIG_HALT;
>> - ? ? ? writel(val, ch->base + PL080_CH_CONFIG);
>> + ? ? ? if (pl08x->vd->is_pl080_s3c) {
>> + ? ? ? ? ? ? ? val = readl(ch->base + PL080S_CH_CONFIG);
>> + ? ? ? ? ? ? ? val &= ~PL080_CONFIG_HALT;
>> + ? ? ? ? ? ? ? writel(val, ch->base + PL080S_CH_CONFIG);
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? val = readl(ch->base + PL080_CH_CONFIG);
>> + ? ? ? ? ? ? ? val &= ~PL080_CONFIG_HALT;
>> + ? ? ? ? ? ? ? writel(val, ch->base + PL080_CH_CONFIG);
>> + ? ? ? }
>
> This would get rid of the if/else clauses
>
ok, understand
>> + ? ? ? if (pl08x->vd->is_pl080_s3c) {
>> + ? ? ? ? ? ? ? u32 val = readl(ch->base + PL080S_CH_CONFIG);
>> + ? ? ? ? ? ? ? val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? PL080_CONFIG_TC_IRQ_MASK);
>> + ? ? ? ? ? ? ? writel(val, ch->base + PL080S_CH_CONFIG);
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? u32 val = readl(ch->base + PL080_CH_CONFIG);
>> + ? ? ? ? ? ? ? val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? PL080_CONFIG_TC_IRQ_MASK);
>> + ? ? ? ? ? ? ? writel(val, ch->base + PL080_CH_CONFIG);
>> + ? ? ? }
>
> As would this...
>
>> + ? ? ? /* Samsung DMAC is PL080 variant*/
>> + ? ? ? {
>> + ? ? ? ? ? ? ? .id ? ? = 0x00041082,
>> + ? ? ? ? ? ? ? .mask ? = 0x000fffff,
>> + ? ? ? ? ? ? ? .data ? = &vendor_pl080_s3c,
>
> Does the hardware realy have this primecell number or is it something that is
> hardcoded from the board/device tree?
>
It is a hardcoded value as s3c64xx does not have Identification registers.

> If it is hardcoded then no objections.
>
> In the latter case, replace 0x41 (= 'A', ARM) with something like
> 0x55 'U' for Samsung or so. Or 0x51 'S'. Or whatever you like.
>
> Then add that to include/linux/amba/bus.h as
> AMBA_VENDOR_SAMSUNG = 0x55,
> so we have this under some kind of control.
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
aLim akHtaR
mAin hUn nA :-)