HI Linus,
Thanks for reviewing again.
On Wed, Sep 28, 2011 at 1:31 PM, Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
aLim akHtaR
mAin hUn nA :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
