| Message ID | 1374597804-3961-1-git-send-email-joelf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/23/2013 06:43 PM, Joel Fernandes wrote: > 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. > > Signed-off-by: Joel Fernandes <joelf@ti.com> > --- > 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(+) > > diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c > index 7222cbe..81d5429 100644 > --- a/drivers/dma/edma.c > +++ b/drivers/dma/edma.c > @@ -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
On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote: > On 07/23/2013 06:43 PM, Joel Fernandes wrote: >> 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. >> >> Signed-off-by: Joel Fernandes <joelf@ti.com> >> --- >> 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(+) >> >> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c >> index 7222cbe..81d5429 100644 >> --- a/drivers/dma/edma.c >> +++ b/drivers/dma/edma.c >> @@ -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? > 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
On 07/24/2013 10:11 AM, Joel Fernandes wrote: > On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote: >> On 07/23/2013 06:43 PM, Joel Fernandes wrote: >>> 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. >>> >>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>> --- >>> 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(+) >>> >>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c >>> index 7222cbe..81d5429 100644 >>> --- a/drivers/dma/edma.c >>> +++ b/drivers/dma/edma.c >>> @@ -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? >> > > 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). > > 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
On Jul 24, 2013, at 3:23 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote: > On 07/24/2013 10:11 AM, Joel Fernandes wrote: >> On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote: >>> On 07/23/2013 06:43 PM, Joel Fernandes wrote: >>>> 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. >>>> >>>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>>> --- >>>> 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(+) >>>> >>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c >>>> index 7222cbe..81d5429 100644 >>>> --- a/drivers/dma/edma.c >>>> +++ b/drivers/dma/edma.c >>>> @@ -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? >> >> 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). 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.. > >> >> 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? > > 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 > > - Lars
On 07/24/2013 10:28 AM, Fernandes, Joel wrote: > > On Jul 24, 2013, at 3:23 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote: > >> On 07/24/2013 10:11 AM, Joel Fernandes wrote: >>> On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote: >>>> On 07/23/2013 06:43 PM, Joel Fernandes wrote: >>>>> 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. >>>>> >>>>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>>>> --- >>>>> 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(+) >>>>> >>>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c >>>>> index 7222cbe..81d5429 100644 >>>>> --- a/drivers/dma/edma.c >>>>> +++ b/drivers/dma/edma.c >>>>> @@ -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? >>> >>> 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). > > 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. > >> >>> >>> 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
On Wed, Jul 24, 2013 at 01:55:24PM -0500, Joel Fernandes wrote: > On 07/24/2013 03:40 AM, Lars-Peter Clausen wrote: > > On 07/24/2013 10:28 AM, Fernandes, Joel wrote: > >>>>>> 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
On 07/24/2013 03:40 AM, Lars-Peter Clausen wrote: > On 07/24/2013 10:28 AM, Fernandes, Joel wrote: >> >> On Jul 24, 2013, at 3:23 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote: >> >>> On 07/24/2013 10:11 AM, Joel Fernandes wrote: >>>> On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote: >>>>> On 07/23/2013 06:43 PM, Joel Fernandes wrote: >>>>>> 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. >>>>>> >>>>>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>>>>> --- >>>>>> 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(+) >>>>>> >>>>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c >>>>>> index 7222cbe..81d5429 100644 >>>>>> --- a/drivers/dma/edma.c >>>>>> +++ b/drivers/dma/edma.c >>>>>> @@ -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? >>>> >>>> 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). >> >> 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. 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
On 07/24/2013 08:55 PM, Joel Fernandes wrote: > On 07/24/2013 03:40 AM, Lars-Peter Clausen wrote: >> On 07/24/2013 10:28 AM, Fernandes, Joel wrote: >>> >>> On Jul 24, 2013, at 3:23 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote: >>> >>>> On 07/24/2013 10:11 AM, Joel Fernandes wrote: >>>>> On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote: >>>>>> On 07/23/2013 06:43 PM, Joel Fernandes wrote: >>>>>>> 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. >>>>>>> >>>>>>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>>>>>> --- >>>>>>> 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(+) >>>>>>> >>>>>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c >>>>>>> index 7222cbe..81d5429 100644 >>>>>>> --- a/drivers/dma/edma.c >>>>>>> +++ b/drivers/dma/edma.c >>>>>>> @@ -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? >>>>> >>>>> 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). >>> >>> 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. > > 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. > > 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
On 07/24/2013 01:33 PM, Vinod Koul wrote: > On Wed, Jul 24, 2013 at 01:55:24PM -0500, Joel Fernandes wrote: >> On 07/24/2013 03:40 AM, Lars-Peter Clausen wrote: >>> On 07/24/2013 10:28 AM, Fernandes, Joel wrote: >>>>>>>> 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. > 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. > 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 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
Sent from my iPhone On Jul 24, 2013, at 2:15 PM, "Lars-Peter Clausen" <lars@metafoo.de> wrote: > On 07/24/2013 08:55 PM, Joel Fernandes wrote: >> On 07/24/2013 03:40 AM, Lars-Peter Clausen wrote: >>> On 07/24/2013 10:28 AM, Fernandes, Joel wrote: >>>> >>>> On Jul 24, 2013, at 3:23 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote: >>>> >>>>> On 07/24/2013 10:11 AM, Joel Fernandes wrote: >>>>>> On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote: >>>>>>> On 07/23/2013 06:43 PM, Joel Fernandes wrote: >>>>>>>> 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. >>>>>>>> >>>>>>>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>>>>>>> --- >>>>>>>> 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(+) >>>>>>>> >>>>>>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c >>>>>>>> index 7222cbe..81d5429 100644 >>>>>>>> --- a/drivers/dma/edma.c >>>>>>>> +++ b/drivers/dma/edma.c >>>>>>>> @@ -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? >>>>>> >>>>>> 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). >>>> >>>> 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. >> >> 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. 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. > >> >> 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 > > - Lars >
On Wed, Jul 24, 2013 at 02:36:26PM -0500, Joel Fernandes wrote: > > 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 > > 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. 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
On Thu, Jul 25, 2013 at 12:53:51PM +0530, Vinod Koul wrote: > On Wed, Jul 24, 2013 at 02:36:26PM -0500, Joel Fernandes wrote: > > > 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 > > > > 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. > 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, On 07/29/2013 04:45 AM, Vinod Koul wrote: > On Thu, Jul 25, 2013 at 12:53:51PM +0530, Vinod Koul wrote: >> On Wed, Jul 24, 2013 at 02:36:26PM -0500, Joel Fernandes wrote: >>>> 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 >>> >>> 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. >> 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 Ah, I see those examples. Thanks. > > 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. > 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. Sure, I just submitted a patch series that does exactly that: http://marc.info/?l=linux-omap&m=137510483230005&w=2 Thanks, -Joel
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c index 7222cbe..81d5429 100644 --- a/drivers/dma/edma.c +++ b/drivers/dma/edma.c @@ -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);
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. Signed-off-by: Joel Fernandes <joelf@ti.com> --- 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(+)