* [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.