dma: edma: add device_slave_caps() support
Subsystems (2)
PATCH 1/1 dma: edma: add device_slave_caps() support
Implement device_slave_caps(). EDMA has a limited number of slots. Slave drivers such as omap_hsmmc will query the driver to make sure they don't pass in more than these many scatter segments.
---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!
Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html
(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112
drivers/dma/edma.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/dma/edma.c +9 -0
@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); } +static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ; + +return 0 ; +} +static size_t edma_desc_size (struct edma_desc *edesc) {int i;
@@ -594,6 +602,7 @@ static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma, dma->device_issue_pending = edma_issue_pending; dma->device_tx_status = edma_tx_status; dma->device_control = edma_control; + dma->device_slave_caps = edma_slave_caps; dma->dev = dev;INIT_LIST_HEAD (&dma->channels);
Signature · 1 line
1.7.9.5
Quoted 26 lines · Joel Fernandes
Implement
device_slave_caps(). EDMA has a limited number of slots. Slave drivers such asomap_hsmmcwill query the driver to make sure they don't pass in more than these many scatter segments.Signed-off-by Joel Fernandes <[email protected]>---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112drivers/dma/edma.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/dma/edma.c +0
-0
drivers/dma/edma.c static void edma_issue_pending(struct dma_chan *chan) · line 523
Quoted 7 lines
drivers/dma/edma.c +4
-0@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); } +static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ;
Hm, what about the other fields?
- Lars
Quoted 30 lines · Lars-Peter Clausen
Quoted 29 lines · Joel Fernandes
Implement
device_slave_caps(). EDMA has a limited number of slots. Slave drivers such asomap_hsmmcwill query the driver to make sure they don't pass in more than these many scatter segments.Signed-off-by Joel Fernandes <[email protected]>---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112drivers/dma/edma.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/dma/edma.c +0
-0@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); }
Quoted 7 lines
drivers/dma/edma.c static void edma_issue_pending(struct dma_chan *chan) · line 523
Quoted 4 lines
drivers/dma/edma.c +4
-0+static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ;Hm, what about the other fields?
Other fields are unused, the max segment size is supposed to be calculated "given" the address width and burst size. Since these can't be provided to get_caps, I have left it out for now.
See: https://lkml.org/lkml/2013/3/6/464
Even if it did, the "segment size" itself is unused in the MMC driver that this is supposed to fix, unlike the "number of segments" which I'm populating above.
If you know of a better way to populate max segment size, let me know.
Thanks,
-Joel
Quoted 39 lines · Joel Fernandes
Quoted 30 lines · Lars-Peter Clausen
Quoted 29 lines · Joel Fernandes
Implement
device_slave_caps(). EDMA has a limited number of slots. Slave drivers such asomap_hsmmcwill query the driver to make sure they don't pass in more than these many scatter segments.Signed-off-by Joel Fernandes <[email protected]>---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112drivers/dma/edma.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/dma/edma.c +0
-0@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); }Quoted 7 lines
drivers/dma/edma.c static void edma_issue_pending(struct dma_chan *chan) · line 523
Quoted 4 lines
drivers/dma/edma.c +4
-0+static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ;Hm, what about the other fields?
Quoted 4 lines
Other fields are unused, the max segment size is supposed to be calculated "given" the address width and burst size. Since these can't be provided to
get_caps, I have left it out for now.
See: https://lkml.org/lkml/2013/3/6/464
The PL330 driver is similar in this regard, the maximum segment size also depends on address width and burst width. What I did for the get_slave_caps implementation is to set it to the minimum maximum size. E.g. in you case that should be SZ_64K - 1 (burstsize and addrwidth both set to 1).
Quoted 5 lines
Even if it did, the "segment size" itself is unused in the MMC driver that this is supposed to fix, unlike the "number of segments" which I'm populating above.
E.g. for ALSA we'll need to know the max segment size, so I think it doesn't hurt add this in this patch as well.
And you should also initialize all the other fields, even though if there are no users yet. It will be really painful to write generic drivers using the dmaengine API if none of the dmaengine drivers actually initializes the caps struct properly.
- Lars
Quoted 44 lines · Lars-Peter Clausen
Quoted 38 lines · Joel Fernandes
Quoted 30 lines · Lars-Peter Clausen
Quoted 29 lines · Joel Fernandes
Implement
device_slave_caps(). EDMA has a limited number of slots. Slave drivers such asomap_hsmmcwill query the driver to make sure they don't pass in more than these many scatter segments.Signed-off-by Joel Fernandes <[email protected]>---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112
drivers/dma/edma.c| 9 +++++++++1 file changed, 9 insertions(+)drivers/dma/edma.c +0
-0@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); }Quoted 6 lines
drivers/dma/edma.c static void edma_issue_pending(struct dma_chan *chan) · line 523
Quoted 4 lines
drivers/dma/edma.c +4
-0+static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ;Hm, what about the other fields?
Quoted 4 lines
Other fields are unused, the max segment size is supposed to be calculated "given" the address width and burst size. Since these can't be provided to
get_caps, I have left it out for now.
See: https://lkml.org/lkml/2013/3/6/464
Quoted 4 lines
The PL330 driver is similar in this regard, the maximum segment size also depends on address width and burst width. What I did for the
get_slave_capsimplementation is to set it to the minimum maximum size. E.g. in you case that should beSZ_64K- 1 (burstsize and addrwidth both set to 1).
So you're setting max to minimum maximum size? Isn't that like telling the driver that its segments can't be bigger than this... Unless I'm missing something..
Quoted 8 lines
Quoted 5 lines
Even if it did, the "segment size" itself is unused in the MMC driver that this is supposed to fix, unlike the "number of segments" which I'm populating above.
E.g. for ALSA we'll need to know the max segment size, so I think it doesn't hurt add this in this patch as well.
For alsa it would dma only the minimum max size even if the dma controller could do more?
Quoted 5 lines
And you should also initialize all the other fields, even though if there are no users yet. It will be really painful to write generic drivers using the dmaengine API if none of the dmaengine drivers actually initializes the caps struct properly.
Ok sure.
Thanks,
-Joel
Quoted 2 lines
- Lars
Quoted 47 lines · Joel
Quoted 44 lines · Lars-Peter Clausen
Quoted 42 lines · Joel Fernandes
Quoted 30 lines · Lars-Peter Clausen
Quoted 29 lines · Joel Fernandes
Implement
device_slave_caps(). EDMA has a limited number of slots. Slave drivers such asomap_hsmmcwill query the driver to make sure they don't pass in more than these many scatter segments.Signed-off-by Joel Fernandes <[email protected]>---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112
drivers/dma/edma.c| 9 +++++++++1 file changed, 9 insertions(+)drivers/dma/edma.c +0
-0@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); }Quoted 6 lines
drivers/dma/edma.c static void edma_issue_pending(struct dma_chan *chan) · line 523
Quoted 4 lines
drivers/dma/edma.c +4
-0+static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ;Hm, what about the other fields?
Other fields are unused, the max segment size is supposed to be calculated "given" the address width and burst size. Since these can't be provided to
get_caps, I have left it out for now.
See: https://lkml.org/lkml/2013/3/6/464
Quoted 6 lines
Quoted 4 lines
The PL330 driver is similar in this regard, the maximum segment size also depends on address width and burst width. What I did for the
get_slave_capsimplementation is to set it to the minimum maximum size. E.g. in you case that should beSZ_64K- 1 (burstsize and addrwidth both set to 1).So you're setting max to minimum maximum size? Isn't that like telling the driver that its segments can't be bigger than this... Unless I'm missing something..
Yes. This is a limitation of the current slave_caps API. The maximum needs to be the maximum for all possible configurations. A specific configuration may allow a larger maximum. So we maybe have to extend the API to be able to query the limits for a certain configuration. Not sure what the best way would be to do that, either adding a config parameter to get_slave_caps or to break it into two functions like you proposed one for the static capabilities and one for the sg limits.
Quoted 12 lines
Quoted 9 lines
Quoted 6 lines
Even if it did, the "segment size" itself is unused in the MMC driver that this is supposed to fix, unlike the "number of segments" which I'm populating above.
E.g. for ALSA we'll need to know the max segment size, so I think it doesn't hurt add this in this patch as well.
For alsa it would dma only the minimum max size even if the dma controller could do more?
Yes, but I think 64k is still plenty enough for the max period size. The current davinci PCM driver even claims to only support up to 8k.
- Lars
Quoted 55 lines · Lars-Peter Clausen
Quoted 47 lines · Joel
Quoted 44 lines · Lars-Peter Clausen
Quoted 42 lines · Joel Fernandes
Quoted 30 lines · Lars-Peter Clausen
Quoted 29 lines · Joel Fernandes
Implement
device_slave_caps(). EDMA has a limited number of slots. Slave drivers such asomap_hsmmcwill query the driver to make sure they don't pass in more than these many scatter segments.Signed-off-by Joel Fernandes <[email protected]>---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112
drivers/dma/edma.c| 9 +++++++++1 file changed, 9 insertions(+)drivers/dma/edma.c +0
-0@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); }Quoted 6 lines
drivers/dma/edma.c static void edma_issue_pending(struct dma_chan *chan) · line 523
Quoted 4 lines
drivers/dma/edma.c +4
-0+static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ;Hm, what about the other fields?
Other fields are unused, the max segment size is supposed to be calculated "given" the address width and burst size. Since these can't be provided to
get_caps, I have left it out for now.
See: https://lkml.org/lkml/2013/3/6/464Quoted 6 lines
Quoted 4 lines
The PL330 driver is similar in this regard, the maximum segment size also depends on address width and burst width. What I did for the
get_slave_capsimplementation is to set it to the minimum maximum size. E.g. in you case that should beSZ_64K- 1 (burstsize and addrwidth both set to 1).So you're setting max to minimum maximum size? Isn't that like telling the driver that its segments can't be bigger than this... Unless I'm missing something..
Quoted 7 lines
Yes. This is a limitation of the current
slave_capsAPI. The maximum needs to be the maximum for all possible configurations. A specific configuration may allow a larger maximum. So we maybe have to extend the API to be able to query the limits for a certain configuration. Not sure what the best way would be to do that, either adding a config parameter toget_slave_capsor to break it into two functions like you proposed one for the static capabilities and one for the sg limits.
I am OK with either approach as long as a decision can be made quickly by maintainers. Right now lot of back and forth has happened and 3 different versions of the same thing have been posted since January. Since this is such a trivial change, it doesn't make sense to spend so much time on it IMO.... The sad part is though this change is trivial, other drivers such as MMC are broken and cannot be enabled due to this. We cannot afford to leave them broken.
If Vinod is not available, can Dan please respond on how to proceed on this? We really need this trivial change to go into this -rc cycle and not delay it by another kernel release. Thank you.
-Joel
Quoted 5 lines · Joel Fernandes
Quoted 4 lines · Lars-Peter Clausen
Quoted 3 lines · Joel
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!
Sorry I was travelling so not realy on top of email for last few days...
Now I am not sure we can send this to -rc.
If it were just this one, we could have pushed but it also depends on a new API which is sitting in my -next. I am not super comfortable to send that to Linus for the -rcs. Sure, he would scream at me!
Also another point worth considering is the approach Russell suggested, I havent gotten a chance to dig deeper but if I understood it correctly then programming the device_dma_parameters should be the right thing to do. Again I need to look deeper and esp wrt edma
~Vinod
Quoted 9 lines · Vinod Koul
Quoted 5 lines · Joel Fernandes
Quoted 4 lines · Lars-Peter Clausen
Quoted 3 lines · Joel
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!
Sorry I was travelling so not realy on top of email for last few days...
Now I am not sure we can send this to -rc.
OK.
Quoted 3 lines
If it were just this one, we could have pushed but it also depends on a new API which is sitting in my -next. I am not super comfortable to send that to Linus for the -rcs. Sure, he would scream at me!
OK.
Quoted 4 lines
Also another point worth considering is the approach Russell suggested, I havent gotten a chance to dig deeper but if I understood it correctly then programming the
device_dma_parametersshould be the right thing to do. Again I need to look deeper and esp wrt edma
OK. I have some patches sitting in my tree too that I'm working on. With that I don't need to know about maximum number of allowed segments and can send along any number of segment. I will rework them and post them. fwiw, I will also implement caps API incase like Lars did populating the other fields though these will not be unused.
For segment size, at this time I don't know any driver that uses it other than davinci-pcm. For this reason the calculations can be done as Lars suggested (for minimum of maximum). Do you know in advance if you're going to amend to drop segment size if we go with what Russell suggested, or are you going to leave the seg-size in the caps API anyway.
Thanks,
-Joel
Quoted 11 lines · Joel Fernandes
Quoted 4 lines
Also another point worth considering is the approach Russell suggested, I havent gotten a chance to dig deeper but if I understood it correctly then programming the
device_dma_parametersshould be the right thing to do. Again I need to look deeper and esp wrt edmaOK. I have some patches sitting in my tree too that I'm working on. With that I don't need to know about maximum number of allowed segments and can send along any number of segment. I will rework them and post them. fwiw, I will also implement caps API incase like Lars did populating the other fields though these will not be unused.
Quoted 5 lines
For segment size, at this time I don't know any driver that uses it other than davinci-pcm. For this reason the calculations can be done as Lars suggested (for minimum of maximum). Do you know in advance if you're going to amend to drop segment size if we go with what Russell suggested, or are you going to leave the seg-size in the caps API anyway.
I am just back and havent really done my work on this. Let me check and as I said if my understanding is right I would be inclined to remove these fields...
~Vinod
Quoted 12 lines · Vinod Koul
Quoted 11 lines · Joel Fernandes
Quoted 4 lines
Also another point worth considering is the approach Russell suggested, I havent gotten a chance to dig deeper but if I understood it correctly then programming the
device_dma_parametersshould be the right thing to do. Again I need to look deeper and esp wrt edmaOK. I have some patches sitting in my tree too that I'm working on. With that I don't need to know about maximum number of allowed segments and can send along any number of segment. I will rework them and post them. fwiw, I will also implement caps API incase like Lars did populating the other fields though these will not be unused.
Quoted 7 lines
Quoted 5 lines
For segment size, at this time I don't know any driver that uses it other than davinci-pcm. For this reason the calculations can be done as Lars suggested (for minimum of maximum). Do you know in advance if you're going to amend to drop segment size if we go with what Russell suggested, or are you going to leave the seg-size in the caps API anyway.
I am just back and havent really done my work on this. Let me check and as I said if my understanding is right I would be inclined to remove these fields...
Okay so the max sg_len should be done by setting the device_dma_parameters.
See the example in MXS DMA and MMC drivers
Now for second parameter why do you need that to be passed, I thought in edma the clients needs this value somehow to program the channel. It would be good that if we can delink from this and let edma derive. Also there should be no such thing as max_sg, the driver should be able to perform any size of sg_list. Internally it can be breaking the dma into multiple trasnactions.
~Vinod
Hi Vinod,
Quoted 18 lines · Vinod Koul
Quoted 17 lines · Vinod Koul
Quoted 16 lines · Joel Fernandes
Quoted 4 lines
Also another point worth considering is the approach Russell suggested, I havent gotten a chance to dig deeper but if I understood it correctly then programming the
device_dma_parametersshould be the right thing to do. Again I need to look deeper and esp wrt edmaOK. I have some patches sitting in my tree too that I'm working on. With that I don't need to know about maximum number of allowed segments and can send along any number of segment. I will rework them and post them. fwiw, I will also implement caps API incase like Lars did populating the other fields though these will not be unused.
For segment size, at this time I don't know any driver that uses it other than davinci-pcm. For this reason the calculations can be done as Lars suggested (for minimum of maximum). Do you know in advance if you're going to amend to drop segment size if we go with what Russell suggested, or are you going to leave the seg-size in the caps API anyway.
Quoted 5 lines
Quoted 2 lines
I am just back and havent really done my work on this. Let me check and as I said if my understanding is right I would be inclined to remove these fields...
Okay so the max
sg_lenshould be done by setting thedevice_dma_parameters.See the example in MXS DMA and MMC drivers
Ah, I see those examples. Thanks.
Quoted 4 lines
Now for second parameter why do you need that to be passed, I thought in edma the clients needs this value somehow to program the channel. It would be good that if we can delink from this and let edma derive.
Do you mean the burst width and device width parameters?
On second thought, I am thinking do we really need to set this particular parameter if we can rearchitect the driver a bit. EDMA theoretically can transmit very large segment sizes. There are 3 internal counters (ACNT, BCNT, CCNT) that are 16-bit, total size is quite large (product of counters).
EDMA driver however currently assumes that ACNT is device width and BCNT is max burst in units of device width. These parameters are again client configured so these have to be passed to the EDMA driver by configuration or passed through API to calculate maximum segment size. This is required because client may be unaware that CCNT is only 16 bit, so following calculation is being done in the SG segment size patch:
<snip>
+*edma_get_slave_sg_limits(struct dma_chan *chan,
+ enum dma_slave_buswidth addr_width,
+ u32 maxburst)
+{
[..]
+ echan->sg_limits.max_seg_len =
+ (SZ_64K - 1) * addr_width * maxburst;
</snip>
The other down side of assigning ACNT device width and BCNT as burst size it seems to be wasteful of the range allowed by 16-bit width. I am thinking if we can break free from these assumptions, then we can transmit segments of literally any length and not have to set any segment size limits at all. I'll try to work on some patches to see if we can overcome segment size limits.
Quoted 3 lines
Also there should be no such thing as
max_sg, the driver should be able to perform any size ofsg_list. Internally it can be breaking the dma into multiple trasnactions.
Sure, I just submitted a patch series that does exactly that:
http://marc.info/?l=linux-omap&m=137510483230005&w=2
Thanks,
-Joel
Quoted 64 lines · Joel Fernandes
Quoted 55 lines · Lars-Peter Clausen
Quoted 47 lines · Joel
Quoted 44 lines · Lars-Peter Clausen
Quoted 42 lines · Joel Fernandes
Quoted 30 lines · Lars-Peter Clausen
Quoted 29 lines · Joel Fernandes
Implement
device_slave_caps(). EDMA has a limited number of slots. Slave drivers such asomap_hsmmcwill query the driver to make sure they don't pass in more than these many scatter segments.Signed-off-by Joel Fernandes <[email protected]>---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112
drivers/dma/edma.c| 9 +++++++++1 file changed, 9 insertions(+)drivers/dma/edma.c +0
-0@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); }Quoted 6 lines
drivers/dma/edma.c static void edma_issue_pending(struct dma_chan *chan) · line 523
Quoted 4 lines
drivers/dma/edma.c +4
-0+static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ;Hm, what about the other fields?
Other fields are unused, the max segment size is supposed to be calculated "given" the address width and burst size. Since these can't be provided to
get_caps, I have left it out for now.
See: https://lkml.org/lkml/2013/3/6/464Quoted 6 lines
Quoted 4 lines
The PL330 driver is similar in this regard, the maximum segment size also depends on address width and burst width. What I did for the
get_slave_capsimplementation is to set it to the minimum maximum size. E.g. in you case that should beSZ_64K- 1 (burstsize and addrwidth both set to 1).So you're setting max to minimum maximum size? Isn't that like telling the driver that its segments can't be bigger than this... Unless I'm missing something..
Quoted 7 lines
Yes. This is a limitation of the current
slave_capsAPI. The maximum needs to be the maximum for all possible configurations. A specific configuration may allow a larger maximum. So we maybe have to extend the API to be able to query the limits for a certain configuration. Not sure what the best way would be to do that, either adding a config parameter toget_slave_capsor to break it into two functions like you proposed one for the static capabilities and one for the sg limits.
Quoted 7 lines
I am OK with either approach as long as a decision can be made quickly by maintainers. Right now lot of back and forth has happened and 3 different versions of the same thing have been posted since January. Since this is such a trivial change, it doesn't make sense to spend so much time on it IMO.... The sad part is though this change is trivial, other drivers such as MMC are broken and cannot be enabled due to this. We cannot afford to leave them broken.
Well this is a new API, so it is kind of expected that there is some back and forth and that there will be a few revisions.
Quoted 4 lines
If Vinod is not available, can Dan please respond on how to proceed on this? We really need this trivial change to go into this -rc cycle and not delay it by another kernel release. Thank you.
This is not something you'd merge for rc3 or even later. If the MMC driver does not work without this I guess it never worked, so strictly speaking there is no regression and it is just a new feature.
- Lars
Sent from my iPhone
Quoted 73 lines · Lars-Peter Clausen
Quoted 64 lines · Joel Fernandes
Quoted 55 lines · Lars-Peter Clausen
Quoted 47 lines · Joel
Quoted 44 lines · Lars-Peter Clausen
Quoted 42 lines · Joel Fernandes
Quoted 30 lines · Lars-Peter Clausen
Quoted 29 lines · Joel Fernandes
Implement
device_slave_caps(). EDMA has a limited number of slots. Slave drivers such asomap_hsmmcwill query the driver to make sure they don't pass in more than these many scatter segments.Signed-off-by Joel Fernandes <[email protected]>---
Vinod, or Dan- If this patch looks ok, can you please merge in for -rc cycle? This patch is required to fix MMC support on AM33xx. This patch is blocking 3 other patches which fix various MMC things. Thanks!Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112
drivers/dma/edma.c| 9 +++++++++1 file changed, 9 insertions(+)drivers/dma/edma.c +0
-0@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore (&echan->vchan.lock, flags); }Quoted 6 lines
drivers/dma/edma.c static void edma_issue_pending(struct dma_chan *chan) · line 523
Quoted 4 lines
drivers/dma/edma.c +4
-0+static inline int edma_slave_caps (struct dma_chan *chan, +struct dma_slave_caps *caps) +{ + caps->max_sg_nr =MAX_NR_SG ;Hm, what about the other fields?
Other fields are unused, the max segment size is supposed to be calculated "given" the address width and burst size. Since these can't be provided to
get_caps, I have left it out for now.
See: https://lkml.org/lkml/2013/3/6/464Quoted 6 lines
Quoted 4 lines
The PL330 driver is similar in this regard, the maximum segment size also depends on address width and burst width. What I did for the
get_slave_capsimplementation is to set it to the minimum maximum size. E.g. in you case that should beSZ_64K- 1 (burstsize and addrwidth both set to 1).So you're setting max to minimum maximum size? Isn't that like telling the driver that its segments can't be bigger than this... Unless I'm missing something..
Quoted 7 lines
Yes. This is a limitation of the current
slave_capsAPI. The maximum needs to be the maximum for all possible configurations. A specific configuration may allow a larger maximum. So we maybe have to extend the API to be able to query the limits for a certain configuration. Not sure what the best way would be to do that, either adding a config parameter toget_slave_capsor to break it into two functions like you proposed one for the static capabilities and one for the sg limits.Quoted 7 lines
I am OK with either approach as long as a decision can be made quickly by maintainers. Right now lot of back and forth has happened and 3 different versions of the same thing have been posted since January. Since this is such a trivial change, it doesn't make sense to spend so much time on it IMO.... The sad part is though this change is trivial, other drivers such as MMC are broken and cannot be enabled due to this. We cannot afford to leave them broken.
Quoted 1 line
Well this is a new API, so it is kind of expected that there is some back and forth and that there will be a few revisions.
Sure. Only thing bothered me is it is a few lines and is just API semantics, nothing functional really.
The MMC dt patches were posted but not applied. I said regression because the dt was agreed for -rc cycle but only thing missing is this trivial api stuff so possibly counting that as a regression fixes MMC altogether. 6 months for trivial change blocking an otherwise fully working driver is too much. I am speaking collectively for all of us, not me or anyone in particular. Anyway looks like MMC is not going anywhere till then.
Quoted 7 lines
Quoted 5 lines
If Vinod is not available, can Dan please respond on how to proceed on this? We really need this trivial change to go into this -rc cycle and not delay it by another kernel release. Thank you.
This is not something you'd merge for rc3 or even later. If the MMC driver does not work without this I guess it never worked, so strictly speaking there is no regression and it is just a new feature.
Agreed.
-Joel
Quoted 3 lines
- Lars