* [PATCH] tmio_mmc: Prevents unexpected status clear
@ 2010-07-07 1:59 Yusuke Goda
2010-07-08 21:46 ` Andrew Morton
0 siblings, 1 reply; 15+ messages in thread
From: Yusuke Goda @ 2010-07-07 1:59 UTC (permalink / raw)
To: linux-mmc; +Cc: Andrew Morton, Magnus Damm
This patch clears only necessary bit.
Signed-off-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
---
drivers/mmc/host/tmio_mmc.h | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 64f7d5d..7944604 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -82,10 +82,7 @@
#define ack_mmc_irqs(host, i) \
do { \
- u32 mask;\
- mask = sd_ctrl_read32((host), CTL_STATUS); \
- mask &= ~((i) & TMIO_MASK_IRQ); \
- sd_ctrl_write32((host), CTL_STATUS, mask); \
+ sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
} while (0)
--
1.7.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-07-07 1:59 [PATCH] tmio_mmc: Prevents unexpected status clear Yusuke Goda
@ 2010-07-08 21:46 ` Andrew Morton
2010-07-13 1:16 ` Yusuke Goda
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Morton @ 2010-07-08 21:46 UTC (permalink / raw)
To: Yusuke Goda; +Cc: linux-mmc, Magnus Damm
On Wed, 07 Jul 2010 10:59:52 +0900
Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
> This patch clears only necessary bit.
>
> Signed-off-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
> ---
> drivers/mmc/host/tmio_mmc.h | 5 +----
> 1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 64f7d5d..7944604 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -82,10 +82,7 @@
>
> #define ack_mmc_irqs(host, i) \
> do { \
> - u32 mask;\
> - mask = sd_ctrl_read32((host), CTL_STATUS); \
> - mask &= ~((i) & TMIO_MASK_IRQ); \
> - sd_ctrl_write32((host), CTL_STATUS, mask); \
> + sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
> } while (0)
Can we have a better changelog please?
What was wrong with the old code?
How does the patch fix it?
What are the user-visible runtime effects of the bug?
(It looks like that was a pretty gross bug - how did it pass testing??)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-07-08 21:46 ` Andrew Morton
@ 2010-07-13 1:16 ` Yusuke Goda
2010-07-15 20:25 ` Andrew Morton
0 siblings, 1 reply; 15+ messages in thread
From: Yusuke Goda @ 2010-07-13 1:16 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mmc, Magnus Damm
Hi Andrew
Thank you for your comment.
>> #define ack_mmc_irqs(host, i) \
>> do { \
>> - u32 mask;\
>> - mask = sd_ctrl_read32((host), CTL_STATUS); \
>> - mask &= ~((i) & TMIO_MASK_IRQ); \
>> - sd_ctrl_write32((host), CTL_STATUS, mask); \
>> + sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
>> } while (0)
>
> Can we have a better changelog please?
>
> What was wrong with the old code?
>
> How does the patch fix it?
>
> What are the user-visible runtime effects of the bug?
>
> (It looks like that was a pretty gross bug - how did it pass testing??)
Example
- CMD53(Single block read / Received data size : 64Byte)
1) Send CMD53
2) Receive "CMD53 response"
3) Call tmio_mmc_cmd_irq(host, status);
-- original code ----------------------------------------------------
#define ack_mmc_irqs(host, i) \
do { \
u32 mask;\
mask = sd_ctrl_read32((host), CTL_STATUS); \
< case 1 >
mask &= ~((i) & TMIO_MASK_IRQ); \
< case 2 >
sd_ctrl_write32((host), CTL_STATUS, mask); \
} while (0)
---------------------------------------------------------------------
TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);"
if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".
This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur.
When received data are small, it rarely occurs.
Thanks,
Goda
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-07-13 1:16 ` Yusuke Goda
@ 2010-07-15 20:25 ` Andrew Morton
2010-08-25 22:11 ` Matt Fleming
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Morton @ 2010-07-15 20:25 UTC (permalink / raw)
To: Yusuke Goda; +Cc: linux-mmc, Magnus Damm
On Tue, 13 Jul 2010 10:16:39 +0900
Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
> Hi Andrew
>
> Thank you for your comment.
>
> >> #define ack_mmc_irqs(host, i) \
> >> do { \
> >> - u32 mask;\
> >> - mask = sd_ctrl_read32((host), CTL_STATUS); \
> >> - mask &= ~((i) & TMIO_MASK_IRQ); \
> >> - sd_ctrl_write32((host), CTL_STATUS, mask); \
> >> + sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
> >> } while (0)
> >
> > Can we have a better changelog please?
> >
> > What was wrong with the old code?
> >
> > How does the patch fix it?
> >
> > What are the user-visible runtime effects of the bug?
> >
> > (It looks like that was a pretty gross bug - how did it pass testing??)
> Example
> - CMD53(Single block read / Received data size : 64Byte)
>
> 1) Send CMD53
> 2) Receive "CMD53 response"
> 3) Call tmio_mmc_cmd_irq(host, status);
> -- original code ----------------------------------------------------
> #define ack_mmc_irqs(host, i) \
> do { \
> u32 mask;\
> mask = sd_ctrl_read32((host), CTL_STATUS); \
> < case 1 >
> mask &= ~((i) & TMIO_MASK_IRQ); \
> < case 2 >
> sd_ctrl_write32((host), CTL_STATUS, mask); \
> } while (0)
> ---------------------------------------------------------------------
>
> TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);"
> if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".
>
> This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur.
> When received data are small, it rarely occurs.
>
OK..
But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch"
the changelogs fail to describe the impact of the bug upon our users.
So when I sit here trying to work out whether the patches should be
applied to 2.6.35 and whether they should be backported into -stable, I
don't have enough information.
What are your thoughts on this?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-07-15 20:25 ` Andrew Morton
@ 2010-08-25 22:11 ` Matt Fleming
2010-08-25 23:07 ` Andrew Morton
0 siblings, 1 reply; 15+ messages in thread
From: Matt Fleming @ 2010-08-25 22:11 UTC (permalink / raw)
To: Andrew Morton; +Cc: Yusuke Goda, linux-mmc, Magnus Damm
On Thu, 15 Jul 2010 13:25:52 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 13 Jul 2010 10:16:39 +0900
> Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
>
> > Hi Andrew
> >
> > Thank you for your comment.
> >
> > >> #define ack_mmc_irqs(host, i) \
> > >> do { \
> > >> - u32 mask;\
> > >> - mask = sd_ctrl_read32((host), CTL_STATUS); \
> > >> - mask &= ~((i) & TMIO_MASK_IRQ); \
> > >> - sd_ctrl_write32((host), CTL_STATUS, mask); \
> > >> + sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
> > >> } while (0)
> > >
> > > Can we have a better changelog please?
> > >
> > > What was wrong with the old code?
> > >
> > > How does the patch fix it?
> > >
> > > What are the user-visible runtime effects of the bug?
> > >
> > > (It looks like that was a pretty gross bug - how did it pass testing??)
> > Example
> > - CMD53(Single block read / Received data size : 64Byte)
> >
> > 1) Send CMD53
> > 2) Receive "CMD53 response"
> > 3) Call tmio_mmc_cmd_irq(host, status);
> > -- original code ----------------------------------------------------
> > #define ack_mmc_irqs(host, i) \
> > do { \
> > u32 mask;\
> > mask = sd_ctrl_read32((host), CTL_STATUS); \
> > < case 1 >
> > mask &= ~((i) & TMIO_MASK_IRQ); \
> > < case 2 >
> > sd_ctrl_write32((host), CTL_STATUS, mask); \
> > } while (0)
> > ---------------------------------------------------------------------
> >
> > TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);"
> > if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".
> >
> > This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur.
> > When received data are small, it rarely occurs.
> >
>
> OK..
>
> But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch"
> the changelogs fail to describe the impact of the bug upon our users.
> So when I sit here trying to work out whether the patches should be
> applied to 2.6.35 and whether they should be backported into -stable, I
> don't have enough information.
>
> What are your thoughts on this?
Goda, do you have any more ideas on updating the changelog for this
patch? It looks to me like this patch is a candidate for stable
(whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry
about replying to that one first, I'm reading my mail backwards)
because, without this patch, it's possible to miss interrupts because
the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it
should not do? Is that correct?
If that is the case then would this be a more appropriate changelog,
"tmio_mmc: Don't clear unhandled pending interrupts
Previously, it was possible for ack_mmc_irqs() to clear pending
interrupt bits in the CTL_STATUS register, even though the interrupt
handler had not been called. This was because of a race that existed
when doing a read-modify-write sequence on CTL_STATUS. After the
read step in this sequence, if an interrupt occurred (causing one of the
bits in CTL_STATUS to be set) the write step would inadvertently clear
it.
This patch eliminates this race by only writing to CTL_STATUS and
clearing the interrupts that were passed as an argument to
ack_mmc_irqs()."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-25 22:11 ` Matt Fleming
@ 2010-08-25 23:07 ` Andrew Morton
2010-08-26 0:53 ` Yusuke Goda
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Morton @ 2010-08-25 23:07 UTC (permalink / raw)
To: Matt Fleming; +Cc: Yusuke Goda, linux-mmc, Magnus Damm, Ian Molton, Paul Mundt
On Wed, 25 Aug 2010 23:11:07 +0100
Matt Fleming <matt@console-pimps.org> wrote:
> On Thu, 15 Jul 2010 13:25:52 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > On Tue, 13 Jul 2010 10:16:39 +0900
> > Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
> >
> > > Hi Andrew
> > >
> > > Thank you for your comment.
> > >
> > > >> #define ack_mmc_irqs(host, i) \
> > > >> do { \
> > > >> - u32 mask;\
> > > >> - mask = sd_ctrl_read32((host), CTL_STATUS); \
> > > >> - mask &= ~((i) & TMIO_MASK_IRQ); \
> > > >> - sd_ctrl_write32((host), CTL_STATUS, mask); \
> > > >> + sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
> > > >> } while (0)
> > > >
> > > > Can we have a better changelog please?
> > > >
> > > > What was wrong with the old code?
> > > >
> > > > How does the patch fix it?
> > > >
> > > > What are the user-visible runtime effects of the bug?
> > > >
> > > > (It looks like that was a pretty gross bug - how did it pass testing??)
> > > Example
> > > - CMD53(Single block read / Received data size : 64Byte)
> > >
> > > 1) Send CMD53
> > > 2) Receive "CMD53 response"
> > > 3) Call tmio_mmc_cmd_irq(host, status);
> > > -- original code ----------------------------------------------------
> > > #define ack_mmc_irqs(host, i) \
> > > do { \
> > > u32 mask;\
> > > mask = sd_ctrl_read32((host), CTL_STATUS); \
> > > < case 1 >
> > > mask &= ~((i) & TMIO_MASK_IRQ); \
> > > < case 2 >
> > > sd_ctrl_write32((host), CTL_STATUS, mask); \
> > > } while (0)
> > > ---------------------------------------------------------------------
> > >
> > > TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);"
> > > if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".
> > >
> > > This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur.
> > > When received data are small, it rarely occurs.
> > >
> >
> > OK..
> >
> > But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch"
> > the changelogs fail to describe the impact of the bug upon our users.
> > So when I sit here trying to work out whether the patches should be
> > applied to 2.6.35 and whether they should be backported into -stable, I
> > don't have enough information.
> >
> > What are your thoughts on this?
>
> Goda, do you have any more ideas on updating the changelog for this
> patch? It looks to me like this patch is a candidate for stable
> (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry
> about replying to that one first, I'm reading my mail backwards)
> because, without this patch, it's possible to miss interrupts because
> the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it
> should not do? Is that correct?
>
> If that is the case then would this be a more appropriate changelog,
>
> "tmio_mmc: Don't clear unhandled pending interrupts
>
> Previously, it was possible for ack_mmc_irqs() to clear pending
> interrupt bits in the CTL_STATUS register, even though the interrupt
> handler had not been called. This was because of a race that existed
> when doing a read-modify-write sequence on CTL_STATUS. After the
> read step in this sequence, if an interrupt occurred (causing one of the
> bits in CTL_STATUS to be set) the write step would inadvertently clear
> it.
>
> This patch eliminates this race by only writing to CTL_STATUS and
> clearing the interrupts that were passed as an argument to
> ack_mmc_irqs()."
hm, I seem to have secretly dropped this patch as well.
Oh well. I restored it as
tmio_mmc-dont-clear-unhandled-pending-interrupts.patch and tagged it
for a -stable backport. Unless I hear otherwise I'll send it in to
Linus when we return from Brazil a couple of weeks from now.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-25 23:07 ` Andrew Morton
@ 2010-08-26 0:53 ` Yusuke Goda
2010-08-26 6:53 ` Matt Fleming
0 siblings, 1 reply; 15+ messages in thread
From: Yusuke Goda @ 2010-08-26 0:53 UTC (permalink / raw)
To: Andrew Morton
Cc: Matt Fleming, linux-mmc, Magnus Damm, Ian Molton, Paul Mundt
Hi Matt, Andrew
Andrew Morton wrote:
> On Wed, 25 Aug 2010 23:11:07 +0100
> Matt Fleming <matt@console-pimps.org> wrote:
>
>> On Thu, 15 Jul 2010 13:25:52 -0700
>> Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>>> On Tue, 13 Jul 2010 10:16:39 +0900
>>> Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
>>>
>>>> Hi Andrew
>>>>
>>>> Thank you for your comment.
>>>>
>>>>>> #define ack_mmc_irqs(host, i) \
>>>>>> do { \
>>>>>> - u32 mask;\
>>>>>> - mask = sd_ctrl_read32((host), CTL_STATUS); \
>>>>>> - mask &= ~((i) & TMIO_MASK_IRQ); \
>>>>>> - sd_ctrl_write32((host), CTL_STATUS, mask); \
>>>>>> + sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
>>>>>> } while (0)
>>>>> Can we have a better changelog please?
>>>>>
>>>>> What was wrong with the old code?
>>>>>
>>>>> How does the patch fix it?
>>>>>
>>>>> What are the user-visible runtime effects of the bug?
>>>>>
>>>>> (It looks like that was a pretty gross bug - how did it pass testing??)
>>>> Example
>>>> - CMD53(Single block read / Received data size : 64Byte)
>>>>
>>>> 1) Send CMD53
>>>> 2) Receive "CMD53 response"
>>>> 3) Call tmio_mmc_cmd_irq(host, status);
>>>> -- original code ----------------------------------------------------
>>>> #define ack_mmc_irqs(host, i) \
>>>> do { \
>>>> u32 mask;\
>>>> mask = sd_ctrl_read32((host), CTL_STATUS); \
>>>> < case 1 >
>>>> mask &= ~((i) & TMIO_MASK_IRQ); \
>>>> < case 2 >
>>>> sd_ctrl_write32((host), CTL_STATUS, mask); \
>>>> } while (0)
>>>> ---------------------------------------------------------------------
>>>>
>>>> TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);"
>>>> if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".
>>>>
>>>> This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur.
>>>> When received data are small, it rarely occurs.
>>>>
>>> OK..
>>>
>>> But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch"
>>> the changelogs fail to describe the impact of the bug upon our users.
>>> So when I sit here trying to work out whether the patches should be
>>> applied to 2.6.35 and whether they should be backported into -stable, I
>>> don't have enough information.
>>>
>>> What are your thoughts on this?
>> Goda, do you have any more ideas on updating the changelog for this
>> patch? It looks to me like this patch is a candidate for stable
>> (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry
>> about replying to that one first, I'm reading my mail backwards)
>> because, without this patch, it's possible to miss interrupts because
>> the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it
>> should not do? Is that correct?
>>
>> If that is the case then would this be a more appropriate changelog,
>>
>> "tmio_mmc: Don't clear unhandled pending interrupts
>>
>> Previously, it was possible for ack_mmc_irqs() to clear pending
>> interrupt bits in the CTL_STATUS register, even though the interrupt
>> handler had not been called. This was because of a race that existed
>> when doing a read-modify-write sequence on CTL_STATUS. After the
>> read step in this sequence, if an interrupt occurred (causing one of the
>> bits in CTL_STATUS to be set) the write step would inadvertently clear
>> it.
>>
>> This patch eliminates this race by only writing to CTL_STATUS and
>> clearing the interrupts that were passed as an argument to
>> ack_mmc_irqs()."
>
> hm, I seem to have secretly dropped this patch as well.
>
> Oh well. I restored it as
> tmio_mmc-dont-clear-unhandled-pending-interrupts.patch and tagged it
> for a -stable backport. Unless I hear otherwise I'll send it in to
> Linus when we return from Brazil a couple of weeks from now.
Thank you for your actions.
I agree to new changelog.
In fact, I contributed the patch which changed changelog.
http://www.spinics.net/lists/linux-mmc/msg02623.html
http://www.spinics.net/lists/linux-mmc/msg02624.html
However, these will not be necessary.
Thanks,
Goda
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-26 0:53 ` Yusuke Goda
@ 2010-08-26 6:53 ` Matt Fleming
2010-08-26 6:58 ` Magnus Damm
0 siblings, 1 reply; 15+ messages in thread
From: Matt Fleming @ 2010-08-26 6:53 UTC (permalink / raw)
To: Yusuke Goda; +Cc: Andrew Morton, linux-mmc, Magnus Damm, Ian Molton, Paul Mundt
On Thu, 26 Aug 2010 09:53:33 +0900
Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
> Hi Matt, Andrew
>
> Andrew Morton wrote:
> > On Wed, 25 Aug 2010 23:11:07 +0100
> > Matt Fleming <matt@console-pimps.org> wrote:
> >
> >> On Thu, 15 Jul 2010 13:25:52 -0700
> >> Andrew Morton <akpm@linux-foundation.org> wrote:
> >>
> >>> On Tue, 13 Jul 2010 10:16:39 +0900
> >>> Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
> >>>
> >>>> Hi Andrew
> >>>>
> >>>> Thank you for your comment.
> >>>>
> >>>>>> #define ack_mmc_irqs(host, i) \
> >>>>>> do { \
> >>>>>> - u32 mask;\
> >>>>>> - mask = sd_ctrl_read32((host), CTL_STATUS); \
> >>>>>> - mask &= ~((i) & TMIO_MASK_IRQ); \
> >>>>>> - sd_ctrl_write32((host), CTL_STATUS, mask); \
> >>>>>> + sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
> >>>>>> } while (0)
> >>>>> Can we have a better changelog please?
> >>>>>
> >>>>> What was wrong with the old code?
> >>>>>
> >>>>> How does the patch fix it?
> >>>>>
> >>>>> What are the user-visible runtime effects of the bug?
> >>>>>
> >>>>> (It looks like that was a pretty gross bug - how did it pass testing??)
> >>>> Example
> >>>> - CMD53(Single block read / Received data size : 64Byte)
> >>>>
> >>>> 1) Send CMD53
> >>>> 2) Receive "CMD53 response"
> >>>> 3) Call tmio_mmc_cmd_irq(host, status);
> >>>> -- original code ----------------------------------------------------
> >>>> #define ack_mmc_irqs(host, i) \
> >>>> do { \
> >>>> u32 mask;\
> >>>> mask = sd_ctrl_read32((host), CTL_STATUS); \
> >>>> < case 1 >
> >>>> mask &= ~((i) & TMIO_MASK_IRQ); \
> >>>> < case 2 >
> >>>> sd_ctrl_write32((host), CTL_STATUS, mask); \
> >>>> } while (0)
> >>>> ---------------------------------------------------------------------
> >>>>
> >>>> TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);"
> >>>> if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".
> >>>>
> >>>> This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur.
> >>>> When received data are small, it rarely occurs.
> >>>>
> >>> OK..
> >>>
> >>> But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch"
> >>> the changelogs fail to describe the impact of the bug upon our users.
> >>> So when I sit here trying to work out whether the patches should be
> >>> applied to 2.6.35 and whether they should be backported into -stable, I
> >>> don't have enough information.
> >>>
> >>> What are your thoughts on this?
> >> Goda, do you have any more ideas on updating the changelog for this
> >> patch? It looks to me like this patch is a candidate for stable
> >> (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry
> >> about replying to that one first, I'm reading my mail backwards)
> >> because, without this patch, it's possible to miss interrupts because
> >> the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it
> >> should not do? Is that correct?
> >>
> >> If that is the case then would this be a more appropriate changelog,
> >>
> >> "tmio_mmc: Don't clear unhandled pending interrupts
> >>
> >> Previously, it was possible for ack_mmc_irqs() to clear pending
> >> interrupt bits in the CTL_STATUS register, even though the interrupt
> >> handler had not been called. This was because of a race that existed
> >> when doing a read-modify-write sequence on CTL_STATUS. After the
> >> read step in this sequence, if an interrupt occurred (causing one of the
> >> bits in CTL_STATUS to be set) the write step would inadvertently clear
> >> it.
> >>
> >> This patch eliminates this race by only writing to CTL_STATUS and
> >> clearing the interrupts that were passed as an argument to
> >> ack_mmc_irqs()."
> >
> > hm, I seem to have secretly dropped this patch as well.
> >
> > Oh well. I restored it as
> > tmio_mmc-dont-clear-unhandled-pending-interrupts.patch and tagged it
> > for a -stable backport. Unless I hear otherwise I'll send it in to
> > Linus when we return from Brazil a couple of weeks from now.
>
> Thank you for your actions.
> I agree to new changelog.
>
> In fact, I contributed the patch which changed changelog.
> http://www.spinics.net/lists/linux-mmc/msg02623.html
> http://www.spinics.net/lists/linux-mmc/msg02624.html
> However, these will not be necessary.
Oh, I missed these. Sorry!
Andrew, this bit of Goda's changelog is worth adding to
"tmio_mmc-dont-clear-unhandled-pending-interrupts.patch"
"Observed with the TMIO_STAT_RXRDY bit together with CMD53
on AR6002 and BCM4318 SDIO cards in polled mode."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-26 6:53 ` Matt Fleming
@ 2010-08-26 6:58 ` Magnus Damm
2010-08-26 7:26 ` Matt Fleming
0 siblings, 1 reply; 15+ messages in thread
From: Magnus Damm @ 2010-08-26 6:58 UTC (permalink / raw)
To: Matt Fleming
Cc: Yusuke Goda, Andrew Morton, linux-mmc, Ian Molton, Paul Mundt
On Thu, Aug 26, 2010 at 3:53 PM, Matt Fleming <matt@console-pimps.org> wrote:
> On Thu, 26 Aug 2010 09:53:33 +0900
> Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
>> Andrew Morton wrote:
>> > On Wed, 25 Aug 2010 23:11:07 +0100
>> > Matt Fleming <matt@console-pimps.org> wrote:
>> >
>> >> On Thu, 15 Jul 2010 13:25:52 -0700
>> >> Andrew Morton <akpm@linux-foundation.org> wrote:
>> >>
>> >>> On Tue, 13 Jul 2010 10:16:39 +0900
>> >>> Yusuke Goda <yusuke.goda.sx@renesas.com> wrote:
>> >>>
>> >>>> Hi Andrew
>> >>>>
>> >>>> Thank you for your comment.
>> >>>>
>> >>>>>> #define ack_mmc_irqs(host, i) \
>> >>>>>> do { \
>> >>>>>> - u32 mask;\
>> >>>>>> - mask = sd_ctrl_read32((host), CTL_STATUS); \
>> >>>>>> - mask &= ~((i) & TMIO_MASK_IRQ); \
>> >>>>>> - sd_ctrl_write32((host), CTL_STATUS, mask); \
>> >>>>>> + sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
>> >>>>>> } while (0)
>> >>>>> Can we have a better changelog please?
>> >>>>>
>> >>>>> What was wrong with the old code?
>> >>>>>
>> >>>>> How does the patch fix it?
>> >>>>>
>> >>>>> What are the user-visible runtime effects of the bug?
>> >>>>>
>> >>>>> (It looks like that was a pretty gross bug - how did it pass testing??)
>> >>>> Example
>> >>>> - CMD53(Single block read / Received data size : 64Byte)
>> >>>>
>> >>>> 1) Send CMD53
>> >>>> 2) Receive "CMD53 response"
>> >>>> 3) Call tmio_mmc_cmd_irq(host, status);
>> >>>> -- original code ----------------------------------------------------
>> >>>> #define ack_mmc_irqs(host, i) \
>> >>>> do { \
>> >>>> u32 mask;\
>> >>>> mask = sd_ctrl_read32((host), CTL_STATUS); \
>> >>>> < case 1 >
>> >>>> mask &= ~((i) & TMIO_MASK_IRQ); \
>> >>>> < case 2 >
>> >>>> sd_ctrl_write32((host), CTL_STATUS, mask); \
>> >>>> } while (0)
>> >>>> ---------------------------------------------------------------------
>> >>>>
>> >>>> TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);"
>> >>>> if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".
>> >>>>
>> >>>> This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur.
>> >>>> When received data are small, it rarely occurs.
>> >>>>
>> >>> OK..
>> >>>
>> >>> But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch"
>> >>> the changelogs fail to describe the impact of the bug upon our users.
>> >>> So when I sit here trying to work out whether the patches should be
>> >>> applied to 2.6.35 and whether they should be backported into -stable, I
>> >>> don't have enough information.
>> >>>
>> >>> What are your thoughts on this?
>> >> Goda, do you have any more ideas on updating the changelog for this
>> >> patch? It looks to me like this patch is a candidate for stable
>> >> (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry
>> >> about replying to that one first, I'm reading my mail backwards)
>> >> because, without this patch, it's possible to miss interrupts because
>> >> the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it
>> >> should not do? Is that correct?
>> >>
>> >> If that is the case then would this be a more appropriate changelog,
>> >>
>> >> "tmio_mmc: Don't clear unhandled pending interrupts
>> >>
>> >> Previously, it was possible for ack_mmc_irqs() to clear pending
>> >> interrupt bits in the CTL_STATUS register, even though the interrupt
>> >> handler had not been called. This was because of a race that existed
>> >> when doing a read-modify-write sequence on CTL_STATUS. After the
>> >> read step in this sequence, if an interrupt occurred (causing one of the
>> >> bits in CTL_STATUS to be set) the write step would inadvertently clear
>> >> it.
>> >>
>> >> This patch eliminates this race by only writing to CTL_STATUS and
>> >> clearing the interrupts that were passed as an argument to
>> >> ack_mmc_irqs()."
>> >
>> > hm, I seem to have secretly dropped this patch as well.
>> >
>> > Oh well. I restored it as
>> > tmio_mmc-dont-clear-unhandled-pending-interrupts.patch and tagged it
>> > for a -stable backport. Unless I hear otherwise I'll send it in to
>> > Linus when we return from Brazil a couple of weeks from now.
>>
>> Thank you for your actions.
>> I agree to new changelog.
>>
>> In fact, I contributed the patch which changed changelog.
>> http://www.spinics.net/lists/linux-mmc/msg02623.html
>> http://www.spinics.net/lists/linux-mmc/msg02624.html
>> However, these will not be necessary.
>
> Oh, I missed these. Sorry!
>
> Andrew, this bit of Goda's changelog is worth adding to
> "tmio_mmc-dont-clear-unhandled-pending-interrupts.patch"
>
> "Observed with the TMIO_STAT_RXRDY bit together with CMD53
> on AR6002 and BCM4318 SDIO cards in polled mode."
Hi Matt,
Just FYI, the newer version of these patches also have a whole bunch
of acked-by and tested-by tags, see this email:
http://lkml.org/lkml/2010/8/20/429
Thanks for your help!
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-26 6:58 ` Magnus Damm
@ 2010-08-26 7:26 ` Matt Fleming
2010-08-26 21:12 ` Andrew Morton
0 siblings, 1 reply; 15+ messages in thread
From: Matt Fleming @ 2010-08-26 7:26 UTC (permalink / raw)
To: Magnus Damm; +Cc: Yusuke Goda, Andrew Morton, linux-mmc, Ian Molton, Paul Mundt
On Thu, Aug 26, 2010 at 03:58:38PM +0900, Magnus Damm wrote:
>
> Hi Matt,
>
> Just FYI, the newer version of these patches also have a whole bunch
> of acked-by and tested-by tags, see this email:
>
> http://lkml.org/lkml/2010/8/20/429
>
> Thanks for your help!
Argh, right. Claws-mail searching has completely failed me. I didn't
even see that thread when searching to "tmio_mmc". Back to mutt..
Andrew, can you drop the patch with my changelog and pick up the one in
that thread seeing as it's got all the tags and a new changelog? Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-26 7:26 ` Matt Fleming
@ 2010-08-26 21:12 ` Andrew Morton
2010-08-26 22:10 ` Matt Fleming
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Morton @ 2010-08-26 21:12 UTC (permalink / raw)
To: Matt Fleming; +Cc: Magnus Damm, Yusuke Goda, linux-mmc, Ian Molton, Paul Mundt
On Thu, 26 Aug 2010 08:26:42 +0100
Matt Fleming <matt@console-pimps.org> wrote:
> On Thu, Aug 26, 2010 at 03:58:38PM +0900, Magnus Damm wrote:
> >
> > Hi Matt,
> >
> > Just FYI, the newer version of these patches also have a whole bunch
> > of acked-by and tested-by tags, see this email:
> >
> > http://lkml.org/lkml/2010/8/20/429
> >
> > Thanks for your help!
>
> Argh, right. Claws-mail searching has completely failed me. I didn't
> even see that thread when searching to "tmio_mmc". Back to mutt..
>
> Andrew, can you drop the patch with my changelog and pick up the one in
> that thread seeing as it's got all the tags and a new changelog? Thanks.
I actually already had it, as
tmio_mmc-dont-clear-unhandled-pending-interrupts.patch, scheduled for
2.6.36 and -stable.
What's the score with "tmio_mmc: allow 2 byte requests in 4-bit mode"?
I didn't merge it because Ian said "This change needs to be modified to
test what hardware is present. this wont work on my hardware TTBOMK.".
Then I later _did_ merge it because it got sneakily renamed to
"tmio_mmc: revise a limit of the data size".
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-26 21:12 ` Andrew Morton
@ 2010-08-26 22:10 ` Matt Fleming
2010-08-26 22:16 ` Andrew Morton
2010-08-27 8:02 ` Magnus Damm
0 siblings, 2 replies; 15+ messages in thread
From: Matt Fleming @ 2010-08-26 22:10 UTC (permalink / raw)
To: Andrew Morton; +Cc: Magnus Damm, Yusuke Goda, linux-mmc, Ian Molton, Paul Mundt
On Thu, Aug 26, 2010 at 02:12:37PM -0700, Andrew Morton wrote:
> On Thu, 26 Aug 2010 08:26:42 +0100
> Matt Fleming <matt@console-pimps.org> wrote:
>
> > On Thu, Aug 26, 2010 at 03:58:38PM +0900, Magnus Damm wrote:
> > >
> > > Hi Matt,
> > >
> > > Just FYI, the newer version of these patches also have a whole bunch
> > > of acked-by and tested-by tags, see this email:
> > >
> > > http://lkml.org/lkml/2010/8/20/429
> > >
> > > Thanks for your help!
> >
> > Argh, right. Claws-mail searching has completely failed me. I didn't
> > even see that thread when searching to "tmio_mmc". Back to mutt..
> >
> > Andrew, can you drop the patch with my changelog and pick up the one in
> > that thread seeing as it's got all the tags and a new changelog? Thanks.
>
> I actually already had it, as
> tmio_mmc-dont-clear-unhandled-pending-interrupts.patch, scheduled for
> 2.6.36 and -stable.
>
> What's the score with "tmio_mmc: allow 2 byte requests in 4-bit mode"?
> I didn't merge it because Ian said "This change needs to be modified to
> test what hardware is present. this wont work on my hardware TTBOMK.".
> Then I later _did_ merge it because it got sneakily renamed to
> "tmio_mmc: revise a limit of the data size".
I've stuck my oar in and confused everybody now, it seems. As Ian
pointed out, before "tmio_mmc: revise a limit of the data size" can be
merged something like the following is needed,
diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index ee7d0a5..3eabd91 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -657,11 +657,15 @@ static void tmio_mmc_release_dma(struct tmio_mmc_host *host)
static int tmio_mmc_start_data(struct tmio_mmc_host *host,
struct mmc_data *data)
{
+ struct mfd_cell *cell = host->pdev->dev.platform_data;
+ struct tmio_mmc_data *pdata = cell->driver_data;
+
pr_debug("setup data transfer: blocksize %08x nr_blocks %d\n",
data->blksz, data->blocks);
/* Hardware cannot perform 1 and 2 byte requests in 4 bit mode */
- if (data->blksz < 4 && host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
+ if (data->blksz < 4 && !(pdata->flags & TMIO_MMC_2BYTE_BLKSZ) &&
+ host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
pr_err("%s: %d byte block unsupported in 4 bit mode\n",
mmc_hostname(host->mmc), data->blksz);
return -EINVAL;
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index f07425b..56467cb 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -52,6 +52,11 @@
/* tmio MMC platform flags */
#define TMIO_MMC_WRPROTECT_DISABLE (1 << 0)
+/*
+ * Some controllers can support a 2-byte block size when the bus width
+ * is configured in 4-bit mode.
+ */
+#define TMIO_MMC_2BYTE_BLKSZ (1 << 1)
int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);
Magnus, since people have tested 2-byte blksz transfers on SDHI and it
works, does it make sense to have this change to
drivers/mfd/sh_mobile_sdhi.c? Are you aware of a version of the SDHI
block that doens't support this mode?
diff --git a/drivers/mfd/sh_mobile_sdhi.c b/drivers/mfd/sh_mobile_sdhi.c
index cd16459..1f3a1b1 100644
--- a/drivers/mfd/sh_mobile_sdhi.c
+++ b/drivers/mfd/sh_mobile_sdhi.c
@@ -112,6 +112,12 @@ static int __init sh_mobile_sdhi_probe(struct platform_device *pdev)
mmc_data->ocr_mask = p->tmio_ocr_mask;
}
+ /*
+ * All SDHI blocks support 2-byte and larger block sizes in 4-bit
+ * bus width mode.
+ */
+ mmc_data->flags |= TMIO_MMC_2BYTE_BLKSZ;
+
if (p && p->dma_slave_tx >= 0 && p->dma_slave_rx >= 0) {
priv->param_tx.slave_id = p->dma_slave_tx;
priv->param_rx.slave_id = p->dma_slave_rx;
So I think these patches need to either be merged into "tmio_mmc: revise
a limit of the data size" with a bit in the changelog explaning that
some tmio blocks don't support 2-byte tranfers, or added as separate
patches before it.
Does everyone agree?
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-26 22:10 ` Matt Fleming
@ 2010-08-26 22:16 ` Andrew Morton
2010-08-26 22:30 ` Matt Fleming
2010-08-27 8:02 ` Magnus Damm
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Morton @ 2010-08-26 22:16 UTC (permalink / raw)
To: Matt Fleming; +Cc: Magnus Damm, Yusuke Goda, linux-mmc, Ian Molton, Paul Mundt
On Thu, 26 Aug 2010 23:10:24 +0100
Matt Fleming <matt@console-pimps.org> wrote:
> I've stuck my oar in and confused everybody now, it seems.
I've just unconfused myself my dropping the patch ;) Please send new
one(s) when the dust has settled?
The only tmio_mmc patch in my tree at present is
tmio_mmc-dont-clear-unhandled-pending-interrupts:
From: Yusuke Goda <yusuke.goda.sx@renesas.com>
Previously, it was possible for ack_mmc_irqs() to clear pending interrupt
bits in the CTL_STATUS register, even though the interrupt handler had not
been called. This was because of a race that existed when doing a
read-modify-write sequence on CTL_STATUS. After the read step in this
sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS
to be set) the write step would inadvertently clear it.
This patch eliminates this race by only writing to CTL_STATUS and clearing
the interrupts that were passed as an argument to ack_mmc_irqs()."
[matt@console-pimps.org: rewrote changelog]
Signed-off-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: Ian Molton <ian@mnementh.co.uk>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: <linux-mmc@vger.kernel.org>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/mmc/host/tmio_mmc.h | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff -puN drivers/mmc/host/tmio_mmc.h~tmio_mmc-dont-clear-unhandled-pending-interrupts drivers/mmc/host/tmio_mmc.h
--- a/drivers/mmc/host/tmio_mmc.h~tmio_mmc-dont-clear-unhandled-pending-interrupts
+++ a/drivers/mmc/host/tmio_mmc.h
@@ -82,10 +82,7 @@
#define ack_mmc_irqs(host, i) \
do { \
- u32 mask;\
- mask = sd_ctrl_read32((host), CTL_STATUS); \
- mask &= ~((i) & TMIO_MASK_IRQ); \
- sd_ctrl_write32((host), CTL_STATUS, mask); \
+ sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
} while (0)
_
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-26 22:16 ` Andrew Morton
@ 2010-08-26 22:30 ` Matt Fleming
0 siblings, 0 replies; 15+ messages in thread
From: Matt Fleming @ 2010-08-26 22:30 UTC (permalink / raw)
To: Andrew Morton; +Cc: Magnus Damm, Yusuke Goda, linux-mmc, Ian Molton, Paul Mundt
On Thu, Aug 26, 2010 at 03:16:40PM -0700, Andrew Morton wrote:
> On Thu, 26 Aug 2010 23:10:24 +0100
> Matt Fleming <matt@console-pimps.org> wrote:
>
> > I've stuck my oar in and confused everybody now, it seems.
>
> I've just unconfused myself my dropping the patch ;) Please send new
> one(s) when the dust has settled?
Sure thing.
> The only tmio_mmc patch in my tree at present is
> tmio_mmc-dont-clear-unhandled-pending-interrupts:
>
>
>
>
> From: Yusuke Goda <yusuke.goda.sx@renesas.com>
>
> Previously, it was possible for ack_mmc_irqs() to clear pending interrupt
> bits in the CTL_STATUS register, even though the interrupt handler had not
> been called. This was because of a race that existed when doing a
> read-modify-write sequence on CTL_STATUS. After the read step in this
> sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS
> to be set) the write step would inadvertently clear it.
>
> This patch eliminates this race by only writing to CTL_STATUS and clearing
> the interrupts that were passed as an argument to ack_mmc_irqs()."
>
Would you mind adding this snippet to the end of the changelog?
"Observed with the TMIO_STAT_RXRDY bit together with CMD53
on AR6002 and BCM4318 SDIO cards in polled mode."
And these tags,
Acked-by: Magnus Damm <damm@opensource.se>"
Tested-by: Arnd Hannemann <arnd@arndnet.de>"
Acked-by: Ian Molton <ian@mnementh.co.uk>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tmio_mmc: Prevents unexpected status clear
2010-08-26 22:10 ` Matt Fleming
2010-08-26 22:16 ` Andrew Morton
@ 2010-08-27 8:02 ` Magnus Damm
1 sibling, 0 replies; 15+ messages in thread
From: Magnus Damm @ 2010-08-27 8:02 UTC (permalink / raw)
To: Matt Fleming
Cc: Andrew Morton, Yusuke Goda, linux-mmc, Ian Molton, Paul Mundt
On Fri, Aug 27, 2010 at 7:10 AM, Matt Fleming <matt@console-pimps.org> wrote:
> On Thu, Aug 26, 2010 at 02:12:37PM -0700, Andrew Morton wrote:
>> On Thu, 26 Aug 2010 08:26:42 +0100
>> Matt Fleming <matt@console-pimps.org> wrote:
>>
>> > On Thu, Aug 26, 2010 at 03:58:38PM +0900, Magnus Damm wrote:
>> > >
>> > > Hi Matt,
>> > >
>> > > Just FYI, the newer version of these patches also have a whole bunch
>> > > of acked-by and tested-by tags, see this email:
>> > >
>> > > http://lkml.org/lkml/2010/8/20/429
>> > >
>> > > Thanks for your help!
>> >
>> > Argh, right. Claws-mail searching has completely failed me. I didn't
>> > even see that thread when searching to "tmio_mmc". Back to mutt..
>> >
>> > Andrew, can you drop the patch with my changelog and pick up the one in
>> > that thread seeing as it's got all the tags and a new changelog? Thanks.
>>
>> I actually already had it, as
>> tmio_mmc-dont-clear-unhandled-pending-interrupts.patch, scheduled for
>> 2.6.36 and -stable.
>>
>> What's the score with "tmio_mmc: allow 2 byte requests in 4-bit mode"?
>> I didn't merge it because Ian said "This change needs to be modified to
>> test what hardware is present. this wont work on my hardware TTBOMK.".
>> Then I later _did_ merge it because it got sneakily renamed to
>> "tmio_mmc: revise a limit of the data size".
>
> I've stuck my oar in and confused everybody now, it seems. As Ian
> pointed out, before "tmio_mmc: revise a limit of the data size" can be
> merged something like the following is needed,
>
> diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
> index ee7d0a5..3eabd91 100644
> --- a/drivers/mmc/host/tmio_mmc.c
> +++ b/drivers/mmc/host/tmio_mmc.c
> @@ -657,11 +657,15 @@ static void tmio_mmc_release_dma(struct tmio_mmc_host *host)
> static int tmio_mmc_start_data(struct tmio_mmc_host *host,
> struct mmc_data *data)
> {
> + struct mfd_cell *cell = host->pdev->dev.platform_data;
> + struct tmio_mmc_data *pdata = cell->driver_data;
> +
> pr_debug("setup data transfer: blocksize %08x nr_blocks %d\n",
> data->blksz, data->blocks);
>
> /* Hardware cannot perform 1 and 2 byte requests in 4 bit mode */
> - if (data->blksz < 4 && host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
> + if (data->blksz < 4 && !(pdata->flags & TMIO_MMC_2BYTE_BLKSZ) &&
> + host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
> pr_err("%s: %d byte block unsupported in 4 bit mode\n",
> mmc_hostname(host->mmc), data->blksz);
> return -EINVAL;
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index f07425b..56467cb 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -52,6 +52,11 @@
>
> /* tmio MMC platform flags */
> #define TMIO_MMC_WRPROTECT_DISABLE (1 << 0)
> +/*
> + * Some controllers can support a 2-byte block size when the bus width
> + * is configured in 4-bit mode.
> + */
> +#define TMIO_MMC_2BYTE_BLKSZ (1 << 1)
>
> int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
> int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);
>
>
> Magnus, since people have tested 2-byte blksz transfers on SDHI and it
> works, does it make sense to have this change to
> drivers/mfd/sh_mobile_sdhi.c? Are you aware of a version of the SDHI
> block that doens't support this mode?
Yes, updating drivers/mfd/sh_mobile_sdhi.c seems like a good plan.
We've tested this on a few different SH/ARM processors with SDHI, and
they all seem to be ok with 2-byte mode.
> diff --git a/drivers/mfd/sh_mobile_sdhi.c b/drivers/mfd/sh_mobile_sdhi.c
> index cd16459..1f3a1b1 100644
> --- a/drivers/mfd/sh_mobile_sdhi.c
> +++ b/drivers/mfd/sh_mobile_sdhi.c
> @@ -112,6 +112,12 @@ static int __init sh_mobile_sdhi_probe(struct platform_device *pdev)
> mmc_data->ocr_mask = p->tmio_ocr_mask;
> }
>
> + /*
> + * All SDHI blocks support 2-byte and larger block sizes in 4-bit
> + * bus width mode.
> + */
> + mmc_data->flags |= TMIO_MMC_2BYTE_BLKSZ;
> +
> if (p && p->dma_slave_tx >= 0 && p->dma_slave_rx >= 0) {
> priv->param_tx.slave_id = p->dma_slave_tx;
> priv->param_rx.slave_id = p->dma_slave_rx;
>
> So I think these patches need to either be merged into "tmio_mmc: revise
> a limit of the data size" with a bit in the changelog explaning that
> some tmio blocks don't support 2-byte tranfers, or added as separate
> patches before it.
>
> Does everyone agree?
I certainly agree. =)
By the way, you may want to check that this patch doesn't collide with
the SDHI/MMCIF hotplug patches that were recently posted to the
linux-sh list.
Thanks a lot!
/ magnus
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2010-08-27 8:02 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-07 1:59 [PATCH] tmio_mmc: Prevents unexpected status clear Yusuke Goda
2010-07-08 21:46 ` Andrew Morton
2010-07-13 1:16 ` Yusuke Goda
2010-07-15 20:25 ` Andrew Morton
2010-08-25 22:11 ` Matt Fleming
2010-08-25 23:07 ` Andrew Morton
2010-08-26 0:53 ` Yusuke Goda
2010-08-26 6:53 ` Matt Fleming
2010-08-26 6:58 ` Magnus Damm
2010-08-26 7:26 ` Matt Fleming
2010-08-26 21:12 ` Andrew Morton
2010-08-26 22:10 ` Matt Fleming
2010-08-26 22:16 ` Andrew Morton
2010-08-26 22:30 ` Matt Fleming
2010-08-27 8:02 ` Magnus Damm
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.