| Message ID | 1478880259-24943-1-git-send-email-michael@walle.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Michael Walle [mailto:michael@walle.cc] > Sent: Saturday, November 12, 2016 12:04 AM > To: linux-kernel@vger.kernel.org > Cc: linux-mmc@vger.kernel.org; Ulf Hansson; Adrian Hunter; yangbo lu; > Michael Walle > Subject: [PATCH v2] mmc: sdhci-of-esdhc: fixup PRESENT_STATE read > > Since commit 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy > cards in __mmc_switch()") the ESDHC driver is broken: > mmc0: Card stuck in programming state! __mmc_switch > mmc0: error -110 whilst initialising MMC card > > Since this commit __mmc_switch() uses ->card_busy(), which is > sdhci_card_busy() for the esdhc driver. sdhci_card_busy() uses the > PRESENT_STATE register, specifically the DAT0 signal level bit. But the > ESDHC uses a non-conformant PRESENT_STATE register, thus a read fixup is > required to make the driver work again. > > Signed-off-by: Michael Walle <michael@walle.cc> > Fixes: 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy cards in > __mmc_switch()") > --- > v2: > - use lower bits of the original value (that was actually a typo) > - add fixes tag > - fix typo > > drivers/mmc/host/sdhci-of-esdhc.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci- > of-esdhc.c > index fb71c86..f9c84bb 100644 > --- a/drivers/mmc/host/sdhci-of-esdhc.c > +++ b/drivers/mmc/host/sdhci-of-esdhc.c > @@ -66,6 +66,18 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host, > return ret; > } > } > + /* > + * The DAT[3:0] line signal levels and the CMD line signal level is > + * not compatible with standard SDHC register. Move the > corresponding > + * bits around. > + */ > + if (spec_reg == SDHCI_PRESENT_STATE) { > + ret = value & ~0xf8000000; [Lu Yangbo-B47093] I think the bits which should be cleaned before following '|=' are 0x01f00000 not 0xf8000000, right? :) > + ret |= (value >> 4) & SDHCI_DATA_LVL_MASK; > + ret |= (value << 1) & 0x01000000; > + return ret; > + } > + > ret = value; > return ret; > } > -- > 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 2016-11-14 04:00, schrieb Y.B. Lu: >> -----Original Message----- >> From: Michael Walle [mailto:michael@walle.cc] >> Sent: Saturday, November 12, 2016 12:04 AM >> To: linux-kernel@vger.kernel.org >> Cc: linux-mmc@vger.kernel.org; Ulf Hansson; Adrian Hunter; yangbo lu; >> Michael Walle >> Subject: [PATCH v2] mmc: sdhci-of-esdhc: fixup PRESENT_STATE read >> >> Since commit 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy >> cards in __mmc_switch()") the ESDHC driver is broken: >> mmc0: Card stuck in programming state! __mmc_switch >> mmc0: error -110 whilst initialising MMC card >> >> Since this commit __mmc_switch() uses ->card_busy(), which is >> sdhci_card_busy() for the esdhc driver. sdhci_card_busy() uses the >> PRESENT_STATE register, specifically the DAT0 signal level bit. But >> the >> ESDHC uses a non-conformant PRESENT_STATE register, thus a read fixup >> is >> required to make the driver work again. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> Fixes: 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy cards >> in >> __mmc_switch()") >> --- >> v2: >> - use lower bits of the original value (that was actually a typo) >> - add fixes tag >> - fix typo >> >> drivers/mmc/host/sdhci-of-esdhc.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c >> b/drivers/mmc/host/sdhci- >> of-esdhc.c >> index fb71c86..f9c84bb 100644 >> --- a/drivers/mmc/host/sdhci-of-esdhc.c >> +++ b/drivers/mmc/host/sdhci-of-esdhc.c >> @@ -66,6 +66,18 @@ static u32 esdhc_readl_fixup(struct sdhci_host >> *host, >> return ret; >> } >> } >> + /* >> + * The DAT[3:0] line signal levels and the CMD line signal level is >> + * not compatible with standard SDHC register. Move the >> corresponding >> + * bits around. >> + */ >> + if (spec_reg == SDHCI_PRESENT_STATE) { >> + ret = value & ~0xf8000000; > > [Lu Yangbo-B47093] I think the bits which should be cleaned before > following '|=' are 0x01f00000 not 0xf8000000, right? > :) Its neither 0x01f00000 nor 0xf8000000 :( I'll put the bits definition into the comment the next time, so everyone can review them. bit[31:24] are the line DAT[7:0] line signal level. bit[23] is command signal level. All other bits are the same as in the standard SDHC PRESENT_STATE register. I want to keep all but the upper 9 bits from the original value, therefore, this should be the correct mask: ret = value & ~0xff800000; -michael > >> + ret |= (value >> 4) & SDHCI_DATA_LVL_MASK; >> + ret |= (value << 1) & 0x01000000; >> + return ret; >> + } >> + >> ret = value; >> return ret; >> } >> -- >> 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/11/16 10:50, Michael Walle wrote: > Am 2016-11-14 04:00, schrieb Y.B. Lu: >>> -----Original Message----- >>> From: Michael Walle [mailto:michael@walle.cc] >>> Sent: Saturday, November 12, 2016 12:04 AM >>> To: linux-kernel@vger.kernel.org >>> Cc: linux-mmc@vger.kernel.org; Ulf Hansson; Adrian Hunter; yangbo lu; >>> Michael Walle >>> Subject: [PATCH v2] mmc: sdhci-of-esdhc: fixup PRESENT_STATE read >>> >>> Since commit 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy >>> cards in __mmc_switch()") the ESDHC driver is broken: >>> mmc0: Card stuck in programming state! __mmc_switch >>> mmc0: error -110 whilst initialising MMC card >>> >>> Since this commit __mmc_switch() uses ->card_busy(), which is >>> sdhci_card_busy() for the esdhc driver. sdhci_card_busy() uses the >>> PRESENT_STATE register, specifically the DAT0 signal level bit. But the >>> ESDHC uses a non-conformant PRESENT_STATE register, thus a read fixup is >>> required to make the driver work again. >>> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> Fixes: 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy cards in >>> __mmc_switch()") >>> --- >>> v2: >>> - use lower bits of the original value (that was actually a typo) >>> - add fixes tag >>> - fix typo >>> >>> drivers/mmc/host/sdhci-of-esdhc.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci- >>> of-esdhc.c >>> index fb71c86..f9c84bb 100644 >>> --- a/drivers/mmc/host/sdhci-of-esdhc.c >>> +++ b/drivers/mmc/host/sdhci-of-esdhc.c >>> @@ -66,6 +66,18 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host, >>> return ret; >>> } >>> } >>> + /* >>> + * The DAT[3:0] line signal levels and the CMD line signal level is >>> + * not compatible with standard SDHC register. Move the >>> corresponding >>> + * bits around. >>> + */ >>> + if (spec_reg == SDHCI_PRESENT_STATE) { >>> + ret = value & ~0xf8000000; >> >> [Lu Yangbo-B47093] I think the bits which should be cleaned before >> following '|=' are 0x01f00000 not 0xf8000000, right? >> :) > > Its neither 0x01f00000 nor 0xf8000000 :( I'll put the bits definition into > the comment the next time, so everyone can review them. bit[31:24] are the > line DAT[7:0] line signal level. bit[23] is command signal level. All other > bits are the same as in the standard SDHC PRESENT_STATE register. > > I want to keep all but the upper 9 bits from the original value, therefore, > this should be the correct mask: > ret = value & ~0xff800000; Why keep bits 22:20 ? Isn't it more logical to keep 19:0 (i.e. ret = value & 0xfffff) > > -michael > >> >>> + ret |= (value >> 4) & SDHCI_DATA_LVL_MASK; >>> + ret |= (value << 1) & 0x01000000; >>> + return ret; >>> + } >>> + >>> ret = value; >>> return ret; >>> } >>> -- >>> 2.1.4 > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 2016-11-14 10:37, schrieb Adrian Hunter: > On 14/11/16 10:50, Michael Walle wrote: >> Am 2016-11-14 04:00, schrieb Y.B. Lu: >>>> -----Original Message----- >>>> From: Michael Walle [mailto:michael@walle.cc] >>>> Sent: Saturday, November 12, 2016 12:04 AM >>>> To: linux-kernel@vger.kernel.org >>>> Cc: linux-mmc@vger.kernel.org; Ulf Hansson; Adrian Hunter; yangbo >>>> lu; >>>> Michael Walle >>>> Subject: [PATCH v2] mmc: sdhci-of-esdhc: fixup PRESENT_STATE read >>>> >>>> Since commit 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect >>>> busy >>>> cards in __mmc_switch()") the ESDHC driver is broken: >>>> mmc0: Card stuck in programming state! __mmc_switch >>>> mmc0: error -110 whilst initialising MMC card >>>> >>>> Since this commit __mmc_switch() uses ->card_busy(), which is >>>> sdhci_card_busy() for the esdhc driver. sdhci_card_busy() uses the >>>> PRESENT_STATE register, specifically the DAT0 signal level bit. But >>>> the >>>> ESDHC uses a non-conformant PRESENT_STATE register, thus a read >>>> fixup is >>>> required to make the driver work again. >>>> >>>> Signed-off-by: Michael Walle <michael@walle.cc> >>>> Fixes: 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy >>>> cards in >>>> __mmc_switch()") >>>> --- >>>> v2: >>>> - use lower bits of the original value (that was actually a typo) >>>> - add fixes tag >>>> - fix typo >>>> >>>> drivers/mmc/host/sdhci-of-esdhc.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c >>>> b/drivers/mmc/host/sdhci- >>>> of-esdhc.c >>>> index fb71c86..f9c84bb 100644 >>>> --- a/drivers/mmc/host/sdhci-of-esdhc.c >>>> +++ b/drivers/mmc/host/sdhci-of-esdhc.c >>>> @@ -66,6 +66,18 @@ static u32 esdhc_readl_fixup(struct sdhci_host >>>> *host, >>>> return ret; >>>> } >>>> } >>>> + /* >>>> + * The DAT[3:0] line signal levels and the CMD line signal >>>> level is >>>> + * not compatible with standard SDHC register. Move the >>>> corresponding >>>> + * bits around. >>>> + */ >>>> + if (spec_reg == SDHCI_PRESENT_STATE) { >>>> + ret = value & ~0xf8000000; >>> >>> [Lu Yangbo-B47093] I think the bits which should be cleaned before >>> following '|=' are 0x01f00000 not 0xf8000000, right? >>> :) >> >> Its neither 0x01f00000 nor 0xf8000000 :( I'll put the bits definition >> into >> the comment the next time, so everyone can review them. bit[31:24] are >> the >> line DAT[7:0] line signal level. bit[23] is command signal level. All >> other >> bits are the same as in the standard SDHC PRESENT_STATE register. >> >> I want to keep all but the upper 9 bits from the original value, >> therefore, >> this should be the correct mask: >> ret = value & ~0xff800000; > > Why keep bits 22:20 ? Isn't it more logical to keep 19:0 (i.e. ret = > value > & 0xfffff) These are 0 according to the datasheet but of course, it makes more sense to mask these, too. -michael -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c index fb71c86..f9c84bb 100644 --- a/drivers/mmc/host/sdhci-of-esdhc.c +++ b/drivers/mmc/host/sdhci-of-esdhc.c @@ -66,6 +66,18 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host, return ret; } } + /* + * The DAT[3:0] line signal levels and the CMD line signal level is + * not compatible with standard SDHC register. Move the corresponding + * bits around. + */ + if (spec_reg == SDHCI_PRESENT_STATE) { + ret = value & ~0xf8000000; + ret |= (value >> 4) & SDHCI_DATA_LVL_MASK; + ret |= (value << 1) & 0x01000000; + return ret; + } + ret = value; return ret; }
Since commit 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy cards in __mmc_switch()") the ESDHC driver is broken: mmc0: Card stuck in programming state! __mmc_switch mmc0: error -110 whilst initialising MMC card Since this commit __mmc_switch() uses ->card_busy(), which is sdhci_card_busy() for the esdhc driver. sdhci_card_busy() uses the PRESENT_STATE register, specifically the DAT0 signal level bit. But the ESDHC uses a non-conformant PRESENT_STATE register, thus a read fixup is required to make the driver work again. Signed-off-by: Michael Walle <michael@walle.cc> Fixes: 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy cards in __mmc_switch()") --- v2: - use lower bits of the original value (that was actually a typo) - add fixes tag - fix typo drivers/mmc/host/sdhci-of-esdhc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)