Prev: fs: make sure to invalidate pages if we fall back on buffered reads
Next: [PATCH] perf lookup of ppc64 symbols
From: Linus Walleij on 14 Jun 2010 09:40
Hi Viresh, thanks a lot for reviewing this and I'd be *very* happy if
you could give it a spin on
the SPEAr as well!

2010/6/14 Viresh KUMAR <viresh.kumar(a)st.com>:
>> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
>> (...)
>> + * For peripherals with a FIFO:
>> + * Source � � �burst size == half the depth of the peripheral FIFO
>> + * Destination burst size == width of the peripheral FIFO
>> + *
>
> I didn't get it completely, why burst depends upon width of peripheral FIFO.

I think this is just the wrong word, it should be "depth".

>> + * (Bursts are irrelevant for mem to mem transfers - there are no burst
>> + * signals)
>
> I agree that there are no request lines from memories but still we can program
> them with burst in order to faster the transfer. This burst feature is
> automatically handled by DMA.

Actually in the example platform data I set this to the maxburst size
256 Bytes, however if I read the manual correctly this is simply ignored
when you do mem2mem transfers.

It will simply AHB master the bus until the transaction is finished. That
is why the manual states:

"You must program memory-to-memory transfers with a low channel
priority, otherwise:
� other DMA channels cannot access the bus until the
memory-to-memory transfer
has finished
� other AHB masters cannot perform any transaction."

>> + * @max_num_llis: maximum number of LLIs, i.e. longest linked transfer
>> + * length, submitted so far
>
> What is the significance of this field? What it is used for?

Statistics in Peters original implementation. I'll remove it.

>> +static void pl08x_set_cregs(struct pl08x_driver_data *pl08x,
>> + � � � � � � � � � � � � struct pl08x_phy_chan *ch)
>> +{
>> + � � u32 val;
>> +
>> + � � /* Wait for channel inactive */
>> + � � val = readl(ch->base + PL080_CH_CONFIG);
>> + � � while (val & PL080_CONFIG_ACTIVE)
>> + � � � � � � val = readl(ch->base + PL080_CH_CONFIG);
>
> can we use pl08x_phy_channel_busy() instead of above code?

Fixed.

>> + � � /*
>> + � � �* Do not access config register until channel shows as inactive
>> + � � �*/
>> + � � val = readl(ch->base + PL080_CH_CONFIG);
>> + � � while ((val & PL080_CONFIG_ACTIVE) || (val & PL080_CONFIG_ENABLE))
>> + � � � � � � val = readl(ch->base + PL080_CH_CONFIG);
>
> above 3 fns are always called in order, i.e. pl08x_enable_phy_chan will
> be called after pl08x_set_cregs, so we may not require these checks
> here. Is my understanding correct??

The previous check if the channel is active before proceeding, this check also
checks the enable bit, this behaviour comes straight from the manual and is
required to avoid hardware races, so I don't dare to touch it really...

>> + � � /* Wait for channel inactive */
>> + � � val = readl(ch->base + PL080_CH_CONFIG);
>> + � � while (val & PL080_CONFIG_ACTIVE)
>> + � � � � � � val = readl(ch->base + PL080_CH_CONFIG);
>
> can we use pl08x_phy_channel_busy() instead of above code?
> Please check everywhere.

It's just these two places. Fixed this one as well.

>> +
>> + � � mb();
>> +
>> + � � return;
>
> return not required!!

Fixed.

>> +static void pl08x_resume_phy_chan(struct pl08x_phy_chan *ch)
>
> can these small fns be made inline???

Probably but the compiler will do that anyway if there some
point. I'm afraid of violating chapter 15 of CodingStyle...

>> +/* Stops the channel */
>> +static void pl08x_stop_phy_chan(struct pl08x_phy_chan *ch)
>> +{
>> + � � u32 val;
>> +
>> + � � pl08x_pause_phy_chan(ch);
>> +
>> + � � /* Disable channel */
>> + � � val = readl(ch->base + PL080_CH_CONFIG);
>> + � � val &= ~PL080_CONFIG_ENABLE;
>> + � � writel(val, ch->base + PL080_CH_CONFIG);
>> + � � mb();
>> +
>> + � � return;
>
> same here. return not required.

Fixed all these.

>> +static inline u32 get_bytes_in_cctl(u32 cctl)
>> +{
>> + � � /* The source width defines the number of bytes */
>> + � � u32 bytes = cctl & PL080_CONTROL_TRANSFER_SIZE_MASK;
>> +
>> + � � switch ((cctl >> 18) & 3) {
>
> better to use Macros instead of magic numbers here!!!

Fixed.

>> +static inline u32 pl08x_cctl_bits(u32 cctl,
>> + � � � � � � � � � � � � � � � u8 srcwidth,
>> + � � � � � � � � � � � � � � � u8 dstwidth,
>> + � � � � � � � � � � � � � � � u32 tsize)
>
> Not sure, if we should write above function prototype in just 2 lines,
> or is it okay to write it the way it is written.

Whatever, I made it two lines instead.

>> +struct dma_device dmac_slave = {
>
> they must be marked static.

Fixed.

>> + � � .device_alloc_chan_resources � �= pl08x_alloc_chan_resources,
>> + � � .device_free_chan_resources � � = pl08x_free_chan_resources,
>> + � � .device_prep_dma_xor � � � � � �= NULL,
>> + � � .device_prep_dma_memset � � � � = NULL,
>> + � � .device_prep_dma_interrupt � � �= pl08x_prep_dma_interrupt,
>> + � � .device_tx_status � � � � � � � = pl08x_dma_tx_status,
>> + � � .device_issue_pending � � � � � = pl08x_issue_pending,
>> + � � .device_prep_slave_sg � � � � � = pl08x_prep_slave_sg,
>> + � � .device_control � � � � � � � � = pl08x_control,
>> +};
>> +
>
> One more thing linus, why do we need to create this separation between
> channels. i.e. few are for memcpy and few for slave_sg. Why don't all
> channels support everything and this is resolved at runtime.

This is done in all in-tree drivers, the reason is (I think) that for example
the dmatest.c test client will look for:

if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
cnt = dmatest_add_threads(dtc, DMA_MEMCPY);
thread_count += cnt > 0 ? cnt : 0;

So if you want to partition some channels for device I/O (typically some
which are hard-coded to some devices) and others for memcpy() you create
a slave instance for the former and a memcpy() instance for the latter.

In this case we multiplex the memcpy and slave transfers on the few
physical channels we have, but I haven't finally decided how to handle this:
perhaps we should always set on physical channel aside for memcpy
so this won't ever fail, and then this special memcpy device entry will help.

Ideas? Use cases?

>> +/*
>> + * Initialise the DMAC slave channels.
>> + * Make a local wrapper to hold required data
>> + */
>> +static int pl08x_dma_init_slave_channels(struct pl08x_driver_data *pl08x,
>> + � � � � � � � � � � � � � � � � � � � � � � struct dma_device *slave)
>
> above 2 functions are almost exactly same, can we have single function
> instead of two.

OK Fixed.

>> + � � /* A DMA memory pool for LLIs, align on 1-byte boundary */
>> + � � pl08x->pool = dma_pool_create(DRIVER_NAME, &pl08x->adev->dev,
>> + � � � � � � � � � � PL08X_LLI_TSFR_SIZE, PL08X_ALIGN, 0);
>> + � � if (!pl08x->pool) {
>> + � � � � � � ret = -ENOMEM;
>> + � � � � � � goto out_no_lli_pool;
>> + � � }
>> + � � pl08x->pool_ctr = 0;
>> + � � pl08x->max_num_llis = 0;
>
> they are already 0.

OK deleted.

>> + � � ret = request_irq(adev->irq[0], pl08x_irq, IRQF_DISABLED,
>> + � � � � � � � � � � � vd->name, pl08x);
>> + � � if (ret) {
>> + � � � � � � dev_err(&adev->dev, "%s failed to request "
>> + � � � � � � � � � � "interrupt %d\n",
>> + � � � � � � � � � � __func__, adev->irq[0]);
>
> can be written in 2 lines only.

Fixed.

>> + � � /* Get the platform data */
>> + � � pl08x->pd = (struct pl08x_platform_data *)(adev->dev.platform_data);
>
> better to use dev_get_platdata, also no need of typecasting as
> platform_data is of type void*.

Fixed.

>> +/* PL080 has 8 channels and the PL080 have just 2 */
>
> "PL080 or PL081 have just 2"?? You mentioned at the beginning of this
> file, that PL081 has 16 channels. Am i missing something??

Wrong words again, this is one of the most confusing things about PL080/PL081,
it has 2 or 8 *channels* but always 16 *signals*.

Almost all other interrupt controllers have a 1-to-1 correspondence between the
number of incoming signals and the number of available slave channels.
Not the PL08x... it has less channels than signals.

I will underscore it again... if it confuses both me and you it will invariably
confuse everybody else too.

Thanks!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jassi Brar on 15 Jun 2010 06:50
On Tue, Jun 15, 2010 at 7:25 PM, Kukjin Kim <kgene.kim(a)samsung.com> wrote:
> Linus Walleij wrote:
>>
>> This creates a DMAengine driver for the ARM PL080/PL081 PrimeCells
>> based on the implementation earlier submitted by Peter Pearse.
>> This is working like a charm for memcpy on the PB11MPCore, but
>> slave DMA to devices is still not working.
>>
>> This DMA controller is used in mostly unmodified form in the ARM
>> RealView and Versatile platforms, in the ST-Ericsson Nomadik, and
>> in the ST SPEAr platform.
>>
>> It has been converted to use the header from the Samsung PL080
>> derivate instead of its own defintions, and can potentially support
>> several controllers in the same system.
>>
>> Cc: Peter Pearse <peter.pearse(a)arm.com>
>> Cc: Ben Dooks <ben-linux(a)fluff.org>
>> Cc: Kukjin Kim <kgene.kim(a)samsung.com>
>
> Looks good, but please give me some time to test on the board(SMDK6410).
> If any problem, let you know. Of course no problem, will ack.
Samsung doesn't use the DMA API, so this driver is unlikely to work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jassi Brar on 15 Jun 2010 07:50
On Tue, Jun 15, 2010 at 8:17 PM, Maurus Cuelenaere
<mcuelenaere(a)gmail.com> wrote:
> Op 15-06-10 12:45, Jassi Brar schreef:
>> On Tue, Jun 15, 2010 at 7:25 PM, Kukjin Kim <kgene.kim(a)samsung.com> wrote:
>>> Linus Walleij wrote:
>>>>
>>>> This creates a DMAengine driver for the ARM PL080/PL081 PrimeCells
>>>> based on the implementation earlier submitted by Peter Pearse.
>>>> This is working like a charm for memcpy on the PB11MPCore, but
>>>> slave DMA to devices is still not working.
>>>>
>>>> This DMA controller is used in mostly unmodified form in the ARM
>>>> RealView and Versatile platforms, in the ST-Ericsson Nomadik, and
>>>> in the ST SPEAr platform.
>>>>
>>>> It has been converted to use the header from the Samsung PL080
>>>> derivate instead of its own defintions, and can potentially support
>>>> several controllers in the same system.
>>>>
>>>> Cc: Peter Pearse <peter.pearse(a)arm.com>
>>>> Cc: Ben Dooks <ben-linux(a)fluff.org>
>>>> Cc: Kukjin Kim <kgene.kim(a)samsung.com>
>>>
>>> Looks good, but please give me some time to test on the board(SMDK6410).
>>> If any problem, let you know. Of course no problem, will ack.
>> Samsung doesn't use the DMA API, so this driver is unlikely to work.
>
> It doesn't indeed, but it could be adapted to be a wrapper around the DMA engine API.
> Or even better, the drivers could be adapted to use that API.
I don't particularly like the idea of making Samsung's drivers use the DMA API.
IMHO the S3C dma api is better atm.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus WALLEIJ on 15 Jun 2010 16:20
[Viresh]

> On 6/14/2010 7:09 PM, Linus Walleij wrote:
> > Hi Viresh, thanks a lot for reviewing this and I'd be *very* happy if
> > you could give it a spin on
> > the SPEAr as well!
>
> I would be happy too linus, will do it in few weeks, right now we are
> running short of time.

Yeah I know that feeling ... anyway I will probably publish a few
more rounds of this before then.

> > In this case we multiplex the memcpy and slave transfers on the few
> > physical channels we have, but I haven't finally decided how to
> handle this:
> > perhaps we should always set on physical channel aside for memcpy
> > so this won't ever fail, and then this special memcpy device entry
> will help.
> >
> > Ideas? Use cases?
>
> Hmmm. I am not sure, but i think we can't hard code a channel for some
> device.
> All channels should be available with both capabilities. If still there
> are
> some conditions (that you might know), where we need to hard code
> channels
> for devices, then this should come from plat data in some way.

Currently I don't hardcode anything, the physical channels
(on the PL081 only two!) will be multiplexed on a first-come
First-served basis. This is a bit problematic since if I start
a DMAengine memcpy test there is a real battle about the channels...
The memcpy test assumes it will always get a channel, see.

I could queue the transfers waiting for a physical channel to
become available but perhaps that's not so good either.

> I have few more doubts that i wanted to ask. Are following supported in
> your
> driver, we need them in SPEAr:
> - Configure burst size of source or destination.

The PrimeCell extension supports this, do you need that in things
that are not PrimeCells? In that case we need to make them generic.

> - Configure DMA Master for src or dest.

Right now I have an algorithm that will (on the PL080, the PL081
has only one master) try to select AHB1 for the memory and AHB2
for the device by checking if one address is fixed. If both or
none addresses are fixed it will simply select AHB1 for source
and AHB2 for destination.

Please elaborate on what algorithm you need for this!

> - Transfer from Peripheral to Peripheral.

Not supported by DMAengine, but would be easy enough to add.

> - Configure Width of src or dest peripheral.

Part of PrimeCell DMA API.

> - Configure Flow controller of transfer.

Currently only done dynamically with DMA as the master for
Mem2mem, mem2per and per2mem. Mastering from the peripherals
is not supported. Do you have advanced features like that?

Anyway it can be passed in from platform data easily.

> - Some callback for fixing Request line multiplexing just before
> initiating transfer.

This is part of this driver. RealView & versatile have exactly
this problem too.

> - Multiple sg elements in slave_sg transfer. I think it is not
> supported.

No, but can be fixed quite easily.

> - Control for autoincrement of addresses, both in case of memory and
> peripherals.

Right now the engine autoincrements the memory pointers if memory
is source/destination and both on mem2mem.

If you actually have peripherals that need increasing pointers it can
Probably be added.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus WALLEIJ on 15 Jun 2010 17:00
[Jassi]
> [Kukjin]
> >> It has been converted to use the header from the Samsung PL080
> >> derivate instead of its own defintions, and can potentially support
> >> several controllers in the same system.
> > Looks good, but please give me some time to test on the
> board(SMDK6410).
> > If any problem, let you know. Of course no problem, will ack.
> Samsung doesn't use the DMA API, so this driver is unlikely to work.

Yeah that was never the intention, the Samsung derivate has one extra
Register and other funny stuff.

I was more hoping on breaking out a subset in the same elegant way
with a generic part and a separate DMAengine as was done with the
PL330... But I need to get it fully working first.

Atleast I used the same include file.

But I have functional UART DMA on the RealView PB11MPCore now!

Yours,
Linus Walleij
 |  Next  |  Last
Pages: 1 2
Prev: fs: make sure to invalidate pages if we fall back on buffered reads
Next: [PATCH] perf lookup of ppc64 symbols