spi/spi-pl022 fixes

There were few formatting related issues in code. This patch fixes them. Fixes include:
- Remove extra blank lines
- align code to 80 cols
- combine several lines to one line
- Replace multiple spaces with tabs
- Remove spaces before labels

Signed-off-by Viresh Kumar <[email protected]>

Tested-by Linus Walleij <[email protected]>

drivers/spi/spi-pl022.c +5 -5

@@ -2243,23 +2233,23 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	amba_vcore_disable(adev);
 	return 0;
 
- err_spi_register:
- err_start_queue:
- err_init_queue:
+err_spi_register:
+err_start_queue:
+err_init_queue:
 	destroy_queue(pl022);
 	pl022_dma_remove(pl022);
 	free_irq(adev->irq[0], pl022);
 	pm_runtime_disable(&adev->dev);
- err_no_irq:
+err_no_irq:
 	clk_put(pl022->clk);
- err_no_clk:
+err_no_clk:
 	iounmap(pl022->virtbase);

In routine configure_dma(), if transfer->len = PAGE_SIZE, then pages is one more than required. While leads to one more sg getting allocated.

This is wrong. Correct this to allocate correct number of sg.

Signed-off-by Viresh Kumar <[email protected]>

Tested-by Linus Walleij <[email protected]>

---
 drivers/spi/spi-pl022.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/spi/spi-pl022.c +0 -0

index 80116be..1c8b9ec 100644

It would be far better for this to be:

	pages = DIV_ROUND_UP(pl022->cur_transfer->len, PAGE_SIZE);

The compiler will probably optimize it to the same code anyway.

In routine configure_dma(), if transfer->len = PAGE_SIZE, then pages is one more than required. While leads to one more sg getting allocated.

This is wrong. Correct this to allocate correct number of sg.

Signed-off-by Viresh Kumar<[email protected]>

Tested-by Linus Walleij<[email protected]>

---
  drivers/spi/spi-pl022.c |    3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

drivers/spi/spi-pl022.c +0 -0

index 80116be..1c8b9ec 100644

Quoted 3 lines

drivers/spi/spi-pl022.c static int configure_dma(struct pl022 *pl022) · line 1020

Quoted 3 lines

drivers/spi/spi-pl022.c +2 -1

-	pages = (pl022->cur_transfer->len>>  PAGE_SHIFT) + 1;
+	pages = ((pl022->cur_transfer->len + (1 << PAGE_SHIFT) - 1)
+			>>  PAGE_SHIFT);

No need for parens around the rvalue.

Currently we request DMA channels at probe time and free them at remove. They are always occupied, irrespective of their usage.

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Quoted 2 lines

Quoted 2 lines

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required.

Quoted 4 lines · Russell King - ARM Linux

Quoted 2 lines

Quoted 2 lines

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

dw_dmac.c

Quoted 5 lines

Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required.

If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation.

@Dan/Vinod: What do you say?

And on your patch, are you able to dynamically assign the channels for platform? What is the intended usage? (as Russell articulated it is bad to dynamically assign channel for something like uart)

Quoted 3 lines · Vinod

And on your patch, are you able to dynamically assign the channels for platform? What is the intended usage? (as Russell articulated it is bad to dynamically assign channel for something like uart)

Are you talking about channels or DMA request lines? For channels yes, we can always allocate channels as they are independent of peripherals. About request lines, they are muxed in our case between several peripherals, but support for that has to be added in dw_dmac.

Quoted 9 lines · viresh kumar

Quoted 3 lines · Vinod

And on your patch, are you able to dynamically assign the channels for platform? What is the intended usage? (as Russell articulated it is bad to dynamically assign channel for something like uart)

Are you talking about channels or DMA request lines? For channels yes, we can always allocate channels as they are independent of peripherals. About request lines, they are muxed in our case between several peripherals, but support for that has to be added in dw_dmac.

Right, and when you do, you'll probably have to go to a virtual channel implementation, which solves the problem of keeping a channel allocated and makes this patch redundant.

I assert that any DMA engine implementation where request signals can be assigned dynamically to DMA channels should be using a virtual channel implementation.

I would agree on both counts :)

In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer.
But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization.

Quoted 14 lines · viresh kumar

Quoted 4 lines · Russell King - ARM Linux

Quoted 2 lines

Quoted 2 lines

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

dw_dmac.c

Quoted 5 lines

Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. ?This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required.

Quoted 4 lines

If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation.

@Dan/Vinod: What do you say?

I would agree on both counts :)

In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer.
But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization.
So i would argue that there is no "one size fits all" here, if you can manage channels dynamically and utilize more efficiently then go ahead, but if you cant (h/w and usage constraint) then you should not be forced to do so.

Quoted 21 lines · Vinod

Quoted 14 lines · viresh kumar

Quoted 4 lines · Russell King - ARM Linux

Quoted 2 lines

Quoted 2 lines

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

dw_dmac.c

Quoted 5 lines

Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. ?This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required.

Quoted 4 lines

If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation.

@Dan/Vinod: What do you say?

I would agree on both counts :)

Quoted 10 lines

In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer.
But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization.
So i would argue that there is no "one size fits all" here, if you can manage channels dynamically and utilize more efficiently then go ahead, but if you cant (h/w and usage constraint) then you should not be forced to do so.

The idea is to have channel allocation as purely a s/w thing - no actual commitment
of h/w resources. So we can afford to have channel allocated for the whole lifetime
of a client.

Some dmac drivers are written 'improperly', keeping in mind the platforms that have fixed
ReqSig->Peri map and no more clients than actual req-sigs are active simultaneously.
But such dmac drivers will fail if a new platform decides to hijack req-signals.

So, imho, it is absolutely a good thing for every dmac driver to be designed for re-routable
ReqSig->Peri map... which would force their design to allocate virtual/software channels to clients
without commit much(any?) h/w resources.

Quoted 45 lines · Jassi Brar

Quoted 21 lines · Vinod

Quoted 14 lines · viresh kumar

Quoted 4 lines · Russell King - ARM Linux

Quoted 2 lines

Quoted 2 lines

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

dw_dmac.c

Quoted 5 lines

Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. ?This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required.

Quoted 4 lines

If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation.

@Dan/Vinod: What do you say?

I would agree on both counts :)

Quoted 10 lines

In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer.
But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization.
So i would argue that there is no "one size fits all" here, if you can manage channels dynamically and utilize more efficiently then go ahead, but if you cant (h/w and usage constraint) then you should not be forced to do so.

The idea is to have channel allocation as purely a s/w thing - no actual commitment
of h/w resources. So we can afford to have channel allocated for the whole lifetime
of a client.

Some dmac drivers are written 'improperly', keeping in mind the platforms that have fixed
ReqSig->Peri map and no more clients than actual req-sigs are active simultaneously.
But such dmac drivers will fail if a new platform decides to hijack req-signals.

Quoted 5 lines

So, imho, it is absolutely a good thing for every dmac driver to be designed for re-routable
ReqSig->Peri map... which would force their design to allocate virtual/software channels to clients
without commit much(any?) h/w resources.

We discussed channel allocation at Linaro. ?However, I am now _really_ disappointed.

I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. ?So that's essentially invalidated everything we discussed there as part of my thinking was "if Linus is happy with it, then...".

On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux

Quoted 7 lines

I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. ?So that's essentially invalidated everything we discussed there as part of my thinking was "if Linus is happy with it, then...".

IIRC, Linus W mainly opined to involve device pointer during channel selection. Other than that I thought there was only ambiguity about implementation details.

Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record.

Quoted 11 lines · Jassi Brar

On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux

Quoted 7 lines

I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. ?So that's essentially invalidated everything we discussed there as part of my thinking was "if Linus is happy with it, then...".

IIRC, Linus W mainly opined to involve device pointer during channel selection. Other than that I thought there was only ambiguity about implementation details.

Bah now we have two "Linus oracles" in this world instead of just one... haha :-)

Quoted 2 lines

Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record.

It would *work* but the current proposal is *not elegant* IMO.

I think your scheme passing in a device ID and instance tuple e.g. REQ(MMC,2) would *work*, but I don't think it is a proper solution and I would never ACK it, as I said.

The lookup of a device DMA channel should follow the design pattern set by regulators and clocks.

Which means
you use struct device * or alternatively a string (if the instance is not available) to look it up, such that:

struct dma_slave_map {
? char name[16];
? struct device *dev;
? char dev_name[16];
? struct devive *dma_dev;
? char dma_dev_name[16];
};

struct dma_slave_map[] = {
?{
? ? .name = "MMC-RX",
? ? .dev_name = "mmc.0",
? ? .dma_dev_name = "pl08x.0",
?},
?{
? ? .name = "MMC-TX",
? ? .dev_name = "mmc.0",
? ? .dma_dev_name = "pl08x.0",
?},
?{
? ? .name = "SSP-RX",
? ? .dev_name = "pl022.0",
? ? .dma_dev_name = "pl08x.1",
?},

The dmaengine core should take this mapping, and
the device driver would only have to:

struct dma_chan *rxc;
struct dma_chan *txc;
struct device *dev;

rxc = dma_request_slave_channel(dev, "MMC-RX");
txc = dma_request_slave_channel(dev, "MMC-TX");

I also recognized that you needed a priority scheme and a few other bits for the above,

If the majority is happy with this scheme I can even try implementing it.

Quoted 18 lines · Linus Walleij

Quoted 11 lines · Jassi Brar

On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux

Quoted 7 lines

I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. So that's essentially invalidated everything we discussed there as part of my thinking was "if Linus is happy with it, then...".

IIRC, Linus W mainly opined to involve device pointer during channel selection. Other than that I thought there was only ambiguity about implementation details.

Bah now we have two "Linus oracles" in this world instead of just one... haha :-)

Quoted 4 lines

Quoted 2 lines

Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record.

It would *work* but the current proposal is *not elegant* IMO.

would *work* -> You could find no case that the scheme wouldn't support. *not elegant* -> Your opinion. Let us see.

Quoted 3 lines

I think your scheme passing in a device ID and instance tuple e.g. REQ(MMC,2) would *work*, but I don't think it is a proper solution and I would never ACK it, as I said.

As much as I would feel 'deprived' of your ACK, I am not yet changing the implementation for that part.

Quoted 2 lines

The lookup of a device DMA channel should follow the design pattern set by regulators and clocks.

Nopes. It depends upon the subsystem.
We should strive towards making this scheme as 'standalone' as possible.
Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection!

Also that way, a client is actually asking for a 'channel' rather than only specifying its requirements to the API and the API has enough information to return the appropriate channel(which is what I propose).

Quoted 27 lines

Which means
you use struct device * or alternatively a string (if the instance is not available) to look it up, such that:

struct dma_slave_map {
char name[16];
struct device *dev;
char dev_name[16];
struct devive *dma_dev;
char dma_dev_name[16];
};

struct dma_slave_map[] = {

 {
    .name = "MMC-RX",
    .dev_name = "mmc.0",
    .dma_dev_name = "pl08x.0",
 },
 {
    .name = "MMC-TX",
    .dev_name = "mmc.0",
    .dma_dev_name = "pl08x.0",
 },
 {
    .name = "SSP-RX",
    .dev_name = "pl022.0",
    .dma_dev_name = "pl08x.1",

Quoted 1 line

},

1) This is implementation detail.
2) Not a very good one at that.
a) Just think how many bytes you take to specify a channel ? Mine takes less than 4 bytes for equivalent information.
b) Think about the mess of string copy, compare etc, when we can simply extract and manipulate bit-fields from, say, u32?

Quoted 10 lines

The dmaengine core should take this mapping, and
the device driver would only have to:

struct dma_chan *rxc;
struct dma_chan *txc;
struct device *dev;

rxc = dma_request_slave_channel(dev, "MMC-RX");
txc = dma_request_slave_channel(dev, "MMC-TX");

Absolutely "not-very-good" !
We can do without the 'dev' argument.
How does a client request duplex channel ?
MMC-RXTX or MMC-TXRX or TXRX-MMC ?
Do you propose to implement a string parser in the core ?! Not to mention how limited requirements this scheme could express to the core.

Quoted 2 lines

I also recognized that you needed a priority scheme and a few other bits for the above,

You didn't recognize. I told you. IIRC you suggested I should actually sell that point!
The priority is an additional feature that easily helps a situation when a peri could be reached from two different dmacs and the board can prioritize an already busy dmac over the one that would only serve this peripheral - to save power by keeping the idle dmac off. Your string manipulation would only get messier if it had to support that feature.

Quoted 2 lines

If the majority is happy with this scheme I can even try implementing it.

Interesting! Only a few days ago you were seeing eternal sunshine in filter-functions... and now you plan to implement my proposal by replacing bit-fields with strings.
Frankly, I don't care much who, if anybody, implements it anymore.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 8 lines

Quoted 2 lines

The lookup of a device DMA channel should follow the design pattern set by regulators and clocks.

Nopes. It depends upon the subsystem.
We should strive towards making this scheme as 'standalone' as possible.
Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection!

On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3 can only use channels from controller 1, and the peripherals 4, 5 from controller 2 and so on. They _absolutely_ need channel from specific controller, so above suits it :).
Btw all three controllers _have_exactly_same_caps_ so dma engine will not see any difference in channels. They don't know which peripherals are connected to them, that's platform information which this scheme seems to be suited to...
Can you tell me how your scheme will work in this case?

Quoted 8 lines · Vinod Koul

Quoted 8 lines

Quoted 2 lines

The lookup of a device DMA channel should follow the design pattern set by regulators and clocks.

Nopes. It depends upon the subsystem.
We should strive towards making this scheme as 'standalone' as possible.
Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection!

Quoted 9 lines

On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3 can only use channels from controller 1, and the peripherals 4, 5 from controller 2 and so on. They _absolutely_ need channel from specific controller, so above suits it :).
Btw all three controllers _have_exactly_same_caps_ so dma engine will not see any difference in channels. They don't know which peripherals are connected to them, that's platform information which this scheme seems to be suited to...
Can you tell me how your scheme will work in this case?

Actually the setup is quite simple.
Say, the Peri-4 is the UART which could only be reached from DMAC-2.

DMAENGINE should simply manage a pool of total channels in the platform, disregarding the dmacs they come from.

Each channel in the common pool will have a 'capability-tag' of u32 type - which specifies various capabilities of the channel expressed in bit-fields. It is the job of
platform (via dmac driver) to set 'tag' of each channel appropriately. Among others, a channel's reach to a particular device is a 'capability' (expressed
in 7-bits DEV_TYPE field-mask of the 'tag')

In your case, while setting up channels before adding them to the common pool, the platform (via dmac driver) will ensure exactly the channel which can reach UART
has set DEV_UART value in the DEV_TYPE field of its capability-tag.

After all the dmac instances have been probed, _exactly_ one channel in the pool will have the 'UART' capability mentioned in the DEV_TYPE field.

The serial driver(client), will simply specify 'Ability to reach UART' as one of its
requirements to the core - by setting the DEV_TYPE field with DEV_UART value of the 'request-tag' (a u32 number, say).

While searching for appropriate channel for the serial client, the core will iterate over the pool and, of course, only one channel will have DEV_TYPE field set to DEV_UART - the one which platform decided!

Please have a look at the end of https://lkml.org/lkml/2011/7/29/211
(esp the helpers) to get an idea of data structures involved.

*******************************
UART(client) driver snippet :-
*******************************
struct dma_chan *chan_rx;
u32 req_cap;

/* Reset the requirement list */
DCH_RESET_CAP(req_cap);

/* Specify ability to reach UART as a requirement */ DCH_REQCAP_DEV(req_cap, DCHAN_TODEV_UART);

/* Specify we require to Read from UART */
DCH_REQCAP_P2M(req_cap);

/* ...... specify other requirements */

/* Ask for the channel */
chan_rx = dma_request_channel(req_cap);

***************
dmaengine.c
***************
struct dma_chan *dma_request_channel(u32 req_cap) // typedef u32 to look nice ;)
{
struct dma_chan *ch;

           /* !!! Not final implementation !!! */
 list_for_each_entry(ch, &channel_pool, chan_node) {
      if (GET_DEVTYPE(req_cap) != GET_DEVTYPE(ch->tag))
          continue;

      if ((IS_DCH_M2M(req_cap) && !IS_DCH_M2M(ch->tag))
         continue;

      if ((IS_DCH_M2P(req_cap) && !IS_DCH_M2P(ch->tag))
         continue;

      if ((IS_DCH_P2M(req_cap) && !IS_DCH_P2M(ch->tag))
         continue;

      if ((IS_DCH_P2P(req_cap) && !IS_DCH_P2P(ch->tag))
         continue;

      /* weed out channels that don't have further capabilities required */

      /* At the end of checks this channel meets every requirement */
      return ch;

}
/* Will never reach here in a bug-free platform */ return NULL;
}

**************************************
DMAC <- PLATFORM <- BOARD
***************************************
Well, that is between dmac and platform and out of scope of DMAENGINE API. They can decide upon any data structure to pass the info needed.

But at some point someone must set :-
_Only_ for the channel of DMAC-2 that could talk to the UART.

Quoted 115 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

The lookup of a device DMA channel should follow the design pattern set by regulators and clocks.

Nopes. It depends upon the subsystem.
We should strive towards making this scheme as 'standalone' as possible.
Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection!

Quoted 9 lines

On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3 can only use channels from controller 1, and the peripherals 4, 5 from controller 2 and so on. They _absolutely_ need channel from specific controller, so above suits it :).
Btw all three controllers _have_exactly_same_caps_ so dma engine will not see any difference in channels. They don't know which peripherals are connected to them, that's platform information which this scheme seems to be suited to...
Can you tell me how your scheme will work in this case?

Actually the setup is quite simple.
Say, the Peri-4 is the UART which could only be reached from DMAC-2.

DMAENGINE should simply manage a pool of total channels in the platform, disregarding the dmacs they come from.

Each channel in the common pool will have a 'capability-tag' of u32 type - which specifies various capabilities of the channel expressed in bit-fields. It is the job of
platform (via dmac driver) to set 'tag' of each channel appropriately. Among others, a channel's reach to a particular device is a 'capability' (expressed
in 7-bits DEV_TYPE field-mask of the 'tag')

In your case, while setting up channels before adding them to the common pool, the platform (via dmac driver) will ensure exactly the channel which can reach UART
has set DEV_UART value in the DEV_TYPE field of its capability-tag.

After all the dmac instances have been probed, _exactly_ one channel in the pool will have the 'UART' capability mentioned in the DEV_TYPE field.

The serial driver(client), will simply specify 'Ability to reach UART' as one of its
requirements to the core - by setting the DEV_TYPE field with DEV_UART value of the 'request-tag' (a u32 number, say).

While searching for appropriate channel for the serial client, the core will iterate over the pool and, of course, only one channel will have DEV_TYPE field set to DEV_UART - the one which platform decided!

Please have a look at the end of ?https://lkml.org/lkml/2011/7/29/211 (esp the helpers) to get an idea of data structures involved.

*******************************
UART(client) driver snippet :-
*******************************
struct dma_chan *chan_rx;
u32 req_cap;

/* Reset the requirement list */
DCH_RESET_CAP(req_cap);

/* Specify ability to reach UART as a requirement */ DCH_REQCAP_DEV(req_cap, DCHAN_TODEV_UART);

/* Specify we require to Read from UART */
DCH_REQCAP_P2M(req_cap);

/* ...... specify other requirements */

/* Ask for the channel */
chan_rx = dma_request_channel(req_cap);

***************
dmaengine.c
***************
struct dma_chan *dma_request_channel(u32 req_cap) ?// typedef u32 to look nice ;)
{
?struct dma_chan *ch;

? ? ? ? ? ?/* !!! Not final implementation !!! */ ?list_for_each_entry(ch, &channel_pool, chan_node) { ? ? ? if (GET_DEVTYPE(req_cap) != GET_DEVTYPE(ch->tag)) ? ? ? ? ? continue;

? ? ? if ((IS_DCH_M2M(req_cap) && !IS_DCH_M2M(ch->tag)) ? ? ? ? ?continue;

? ? ? if ((IS_DCH_M2P(req_cap) && !IS_DCH_M2P(ch->tag)) ? ? ? ? ?continue;

? ? ? if ((IS_DCH_P2M(req_cap) && !IS_DCH_P2M(ch->tag)) ? ? ? ? ?continue;

? ? ? if ((IS_DCH_P2P(req_cap) && !IS_DCH_P2P(ch->tag)) ? ? ? ? ?continue;

? ? ? /* weed out channels that don't have further capabilities required */

? ? ? /* At the end of checks this channel meets every requirement */ ? ? ? return ch;
? }
? /* Will never reach here in a bug-free platform */
? return NULL;
}

**************************************
DMAC <- PLATFORM <- BOARD
***************************************
Well, that is between dmac and platform and out of scope of DMAENGINE API. They can decide upon any data structure to pass the info needed.

Quoted 2 lines

But at some point someone must set :-
? _Only_ for the channel of DMAC-2 that could talk to the UART.

That is why I am skeptical about this implementation approach. Here CHAN_TODEV_UART is how peripheral is connected to DMAC which is a platform capability which magically needs to be published by generic DMAC driver and requested by UART driver...

Sorry I still don't get this schema and how it can be scaled and be generic enough to let it carry with various implementations.

Can you publish your complete idea rather than bits and pieces...

Quoted 4 lines · Vinod

Sorry I still don't get this schema and how it can be scaled and be generic enough to let it carry with various implementations.

Can you publish your complete idea rather than bits and pieces...

I already explained the complete idea. I don't have any implementation yet. Clients and dmaengine.c is easier to manage.
But changes to >20 dmac drivers is the biggest effort - though they anyway need such modifications if we are to have the DMAENGINE utopia someday. In free time, I will modify a dmac driver or two, but it might take prohibitively
long if I am expected to update possibly all the 20 dmac drivers and the backend platforms by

a) Making dmac drivers platform agnostic and for re-routable ReqSig-Peri map. That implies dmac drivers managing 'virtual-channel' front end
and physical

       channel and ReqSig->Peri link management in the backend with

help from platform.
b) Modifying platforms/boards to pass channel map and link re-routing callback pointers to generic dmac drivers.

Quoted 4 lines

Quoted 2 lines

Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record.

It would *work* but the current proposal is *not elegant* IMO.

would *work* ?-> You could find no case that the scheme wouldn't support.

*not elegant* ?-> Your opinion. Let us see.

Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection!

Quoted 2 lines

rxc = dma_request_slave_channel(dev, "MMC-RX");
txc = dma_request_slave_channel(dev, "MMC-TX");

Absolutely "not-very-good" !
We can do without the 'dev' argument.

How does a client request duplex channel ?

Do you propose to implement a string parser in the core ?!

Quoted 2 lines

I also recognized that you needed a priority scheme and a few other bits for the above,

You didn't recognize. I told you. IIRC you suggested I should actually sell that point!

Quoted 2 lines

If the majority is happy with this scheme I can even try implementing it.

Interesting! Only a few days ago you were seeing eternal sunshine in filter-functions... and now you plan to implement my proposal by replacing bit-fields with strings.

2011/8/10 Jassi Brar <[email protected]>:

Quoted 7 lines · Linus Walleij

Quoted 4 lines

Quoted 2 lines

Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record.

It would *work* but the current proposal is *not elegant* IMO.

would *work* ?-> You could find no case that the scheme wouldn't support.

Well there is the usecase where we have a lot of devices, but it is true we don't have that kind of hardware around.

Quoted 3 lines

Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection!

I think I saw you proposal define REQ(MMC,2) for example, isn't that specifying the device?

Quoted 5 lines

Quoted 2 lines

rxc = dma_request_slave_channel(dev, "MMC-RX");
txc = dma_request_slave_channel(dev, "MMC-TX");

Absolutely "not-very-good" !
We can do without the 'dev' argument.

You still need to pass in something referring you back to the device.

Quoted 1 line

Do you propose to implement a string parser in the core ?!

Yes, the clock and regulator framework already does that. But it is only used when you cannot pass in a struct device * directly, like from device tree.

Do you have any reason for using device pointer and strings, other than just "because clock and regulator use them" ??

Quoted 5 lines

Quoted 1 line

Do you propose to implement a string parser in the core ?!

Yes, the clock and regulator framework already does that. But it is only used when you cannot pass in a struct device * directly, like from device tree.

Dude, I have utter disrespect for using strings in a case such as expressing requirements.

I have already explained how we can easily and in a _better_ way do without them (again see my last reply to Vindo's setup). Tell me 1 reason why using strings, in this case, would be better ?

2011/8/11 Jassi Brar <[email protected]>:

Quoted 2 lines

Do you have any reason for using device pointer and strings, other than just "because clock and regulator use them" ??

Basically no.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Do you propose to implement a string parser in the core ?!

Yes, the clock and regulator framework already does that. But it is only used when you cannot pass in a struct device * directly, like from device tree.

Dude, I have utter disrespect for using strings in a case such as expressing requirements.

It's mainly a strcmp(), and it's only comparing to the well-established namespace that all devices have anyway, due to the way the device model is done.

Basically when binding clocks or regulators it's:

struct device *dev;
strcmp(map_string, dev_name(dev));

By this time struct device exist of course, since it's the device driver calling to get its clock/regulator.

dev_name() comes from <linux/device.h> and basically takes the kobject name or an optional initilizer name for the device. So the names are pretty static, you don't need to parse them, just compare.

Quoted 3 lines

I have already explained how we can easily and in a _better_ way do without them (again see my last reply to Vindo's setup). Tell me 1 reason why using strings, in this case, would be better ?

I have no other reasons than the above.

People like Russell (clkdevice) and Liam Girdwood (regulator) who I know are smarter than me and have worked with these subsystems for years choose that model, so I trust their judgement.

2011/8/11 Jassi Brar <[email protected]>:

Quoted 2 lines

Do you have any reason for using device pointer and strings, other than just "because clock and regulator use them" ??

Basically no.

But I think these frameworks are very workable and proven to work in practice. So I like them.

When setting up the platform the coder would to be aware that there are totally orthogonal concepts, which IMO makes things complicated.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Do you propose to implement a string parser in the core ?!

Yes, the clock and regulator framework already does that. But it is only used when you cannot pass in a struct device * directly, like from device tree.

Dude, I have utter disrespect for using strings in a case such as expressing requirements.

It's mainly a strcmp(), and it's only comparing to the well-established namespace that all devices have anyway, due to the way the device model is done.

Basically when binding clocks or regulators it's:

struct device *dev;
strcmp(map_string, dev_name(dev));

By this time struct device exist of course, since it's the device driver calling to get its clock/regulator.

dev_name() comes from <linux/device.h> and basically takes the kobject name or an optional initilizer name for the device. So the names are pretty static, you don't need to parse them, just compare.

Quoted 3 lines

I have already explained how we can easily and in a _better_ way do without them (again see my last reply to Vindo's setup). Tell me 1 reason why using strings, in this case, would be better ?

I have no other reasons than the above.

People like Russell (clkdevice) and Liam Girdwood (regulator) who I know are smarter than me and have worked with these subsystems for years choose that model, so I trust their judgement.

Quoted 4 lines · Russell King - ARM Linux

Quoted 2 lines

Quoted 2 lines

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

dw_dmac.c

Quoted 5 lines

Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. ?This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required.

If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation.

Quoted 14 lines · viresh kumar

Quoted 4 lines · Russell King - ARM Linux

Quoted 2 lines

Quoted 2 lines

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

dw_dmac.c

Quoted 5 lines

Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. ?This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required.

Quoted 2 lines

If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation.

The requirement stems from the fact that most DMACs(esp third party) could be made to reroute req-signals by the platform, it has not much to do with the API. IMO, all dmac drivers should be implemented that way to be on the safer side.

Quoted 15 lines · Jassi Brar

Quoted 14 lines · viresh kumar

Quoted 4 lines · Russell King - ARM Linux

Quoted 2 lines

Quoted 2 lines

They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

dw_dmac.c

Quoted 5 lines

Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. ?This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required.

Quoted 6 lines

Quoted 2 lines

If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation.

The requirement stems from the fact that most DMACs(esp third party) could be made to reroute req-signals by the platform, it has not much to do with the API. IMO, all dmac drivers should be implemented that way to be on the safer side.

No it isn't. ?It's to do with how the physical channels are used.

Hi Grant,

I have added Tested-by: Linus Walleij <[email protected]> on all patches.

This patchset mainly covers following fixes:
- formatting related issues
- Passing GFP_ATOMIC for sg allocation from tasklet
- Fixing calculate_effective_freq() routine
- Allocate/free DMA channels as and when required.

Quoted 7 lines · Viresh KUMAR

This patchset mainly covers following fixes:
- formatting related issues
- Passing GFP_ATOMIC for sg allocation from tasklet
- Fixing calculate_effective_freq() routine
- Allocate/free DMA channels as and when required.

Grant,

Did you already pushed this patchset?
There are few concerns on PATCH 6/6 and i am not sure if there are any decisions on it.
@Linus: What do you say?

I agree. Acked-by: Linus Walleij <[email protected]> for all except 6/6, it's probably just as possible to keep the channel handle taken in the driver if we modify the affected DMA driver instead. This would be better for performance I think.

Quoted 4 lines · Linus Walleij

I agree. Acked-by: Linus Walleij <[email protected]> for all except 6/6, it's probably just as possible to keep the channel handle taken in the driver if we modify the affected DMA driver instead. This would be better for performance I think.

Hi Grant,

Any updates on this?

Signature · 2 lines

-- 
viresh