| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGOxZ53qZy+VUM65oK68W088QR=RgQ=cNxOCbO7_WESJesJaVQ@mail.gmail.com>
Date: Wed, 28 Sep 2011 14:20:24 +0530
From: Alim Akhtar <alim.akhtar@...il.com>
To: Viresh Kumar <viresh.kumar@...com>
Cc: Alim Akhtar <alim.akhtar@...sung.com>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"vinod.koul@...el.com" <vinod.koul@...el.com>,
"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
"kgene.kim@...sung.com" <kgene.kim@...sung.com>,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux@....linux.org.uk" <linux@....linux.org.uk>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC
Hi Viresh,
Thanks for reviewing the patch.
On Wed, Sep 28, 2011 at 1:15 PM, Viresh Kumar <viresh.kumar@...com> wrote:
> On 9/28/2011 11:20 AM, Alim Akhtar wrote:
>> Signed-off-by: Alim Akhtar <alim.akhtar@...sung.com>
>> ---
>> drivers/dma/amba-pl08x.c | 135 ++++++++++++++++++++++++++++++++++++++--------
>> 1 files changed, 112 insertions(+), 23 deletions(-)
>>
>
> It would be good if you can add pick some part from cover-letter and put it in
> commit log too.
>
Ok, I will write more comments in the commit log.
>> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
>> index cd8df7f..501540f 100644
>> --- a/drivers/dma/amba-pl08x.c
>> +++ b/drivers/dma/amba-pl08x.c
>> @@ -110,6 +129,11 @@ 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;
>
> Will you write transfer_size in cctl also? What is the purpose of cctl1?
>
The main difference between Primecell PL080 and samsung variant is in
LLI control register bit [0:11] is reserved in case of samsung pl080
and one extra register is add to hold the transfer size at offset
0x10. The purpose of cctl1 is store the transfer_size.
>> @@ -215,11 +255,23 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
>> cpu_relax();
>>
>> /* Do not access config register until channel shows as inactive */
>> - val = readl(phychan->base + PL080_CH_CONFIG);
>> - while ((val & PL080_CONFIG_ACTIVE) || (val & PL080_CONFIG_ENABLE))
>> + 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);
>> + }
>
> You have similar stuff in most the the changes in this patch, because offset of
> config register changes for s3c, you placed in if,else block.
>
> If you check these changes again, there is a lot of code duplication in this if,else
> blocks. The only different thing in if,else is the offset of CH_CONFIG register.
>
> It would be much better if you can do following:
>
> u32 ch_cfg_off;
>
> if (pl08x->vd->is_pl080_s3c)
> ch_cfg_off = PL080S_CH_CONFIG;
> else
> ch_cfg_off = PL080_CH_CONFIG;
>
> Now, this offset can be used in existing code, without much code duplication.
>
I will use suggestion and remove the code duplications.
>> @@ -569,6 +641,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>> u32 cctl, early_bytes = 0;
>> size_t max_bytes_per_lli, 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);
>> if (!txd->llis_va) {
>> @@ -700,7 +773,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>> * width left
>> */
>> while (bd.remainder > (mbus->buswidth - 1)) {
>> - size_t lli_len, tsize, width;
>> + size_t width;
>>
>
> why do we need above two changes. odd_bytes and target_len are still unused.
>
sorry, I will remove the used variables.
>> /*
>> * If enough left try to send max possible,
>> @@ -759,6 +832,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>> 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;
>> + /* Keep the TransferSize seperate to fill samsung specific register */
>> + if (pl08x->vd->is_pl080_s3c)
>> + llis_va[num_llis - 1].cctl1 |= lli_len;
>
> I couldn't get this completely. Why do you keep only length of
> last lli in cctl1. what about all other llis. Also why |=
> and not directly cctl1 = lli_len
>
I will write more explanation about it.
> --
> viresh
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists