| Message ID | 1280805072-26112-1-git-send-email-tie-fei.zang@freescale.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Kumar Gala |
| Headers | show |
On Tue, 3 Aug 2010 11:11:10 +0800 Roy Zang <tie-fei.zang@freescale.com> wrote: > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -240,6 +240,8 @@ struct sdhci_host { > #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN (1<<25) > /* Controller cannot support End Attribute in NOP ADMA descriptor */ > #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC (1<<26) > +/* Controller uses Auto CMD12 command to stop the transfer */ > +#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<27) This becomes 1<<29 in my tree. We're about to run out. What happens then?
> -----Original Message----- > From: Andrew Morton [mailto:akpm@linux-foundation.org] > Sent: Wednesday, August 04, 2010 7:44 AM > To: Zang Roy-R61911 > Cc: linux-mmc@vger.kernel.org; linuxppc-dev@ozlabs.org > Subject: Re: [PATCH 1/3 v2] sdhci: Add auto CMD12 support for > eSDHC driver > > On Tue, 3 Aug 2010 11:11:10 +0800 > Roy Zang <tie-fei.zang@freescale.com> wrote: > > > --- a/drivers/mmc/host/sdhci.h > > +++ b/drivers/mmc/host/sdhci.h > > @@ -240,6 +240,8 @@ struct sdhci_host { > > #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN (1<<25) > > /* Controller cannot support End Attribute in NOP ADMA > descriptor */ > > #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC (1<<26) > > +/* Controller uses Auto CMD12 command to stop the transfer */ > > +#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<27) > > This becomes 1<<29 in my tree. It also works. > > We're about to run out. :-( >What happens then? Rewrite the code to extend some bits, I suppose. Roy
On Mon, Aug 2, 2010 at 9:11 PM, Roy Zang <tie-fei.zang@freescale.com> wrote: > From: Jerry Huang <Chang-Ming.Huang@freescale.com> > > Add auto CMD12 command support for eSDHC driver. > This is needed by P4080 and P1022 for block read/write. > Manual asynchronous CMD12 abort operation causes protocol violations on > these silicons. > > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com> > --- > drivers/mmc/host/sdhci-of-core.c | 4 ++++ > drivers/mmc/host/sdhci.c | 14 ++++++++++++-- > drivers/mmc/host/sdhci.h | 2 ++ > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index c6d1bd8..a92566e 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -817,8 +817,12 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host, > WARN_ON(!host->data); > > mode = SDHCI_TRNS_BLK_CNT_EN; > - if (data->blocks > 1) > - mode |= SDHCI_TRNS_MULTI; > + if (data->blocks > 1) { > + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > + mode |= SDHCI_TRNS_MULTI | SDHCI_TRNS_ACMD12; > + else > + mode |= SDHCI_TRNS_MULTI; nit: + mode |= SDHCI_TRNS_MULTI; + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) + mode |= SDHCI_TRNS_ACMD12; Clearer, no? > + } > if (data->flags & MMC_DATA_READ) > mode |= SDHCI_TRNS_READ; > if (host->flags & SDHCI_REQ_USE_DMA)
> -----Original Message----- > From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On > Behalf Of Grant Likely > Sent: Thursday, August 05, 2010 9:03 AM > To: Zang Roy-R61911 > Cc: linux-mmc@vger.kernel.org; linuxppc-dev@ozlabs.org; > akpm@linux-foundation.org > Subject: Re: [PATCH 1/3 v2] sdhci: Add auto CMD12 support for > eSDHC driver > > On Mon, Aug 2, 2010 at 9:11 PM, Roy Zang > <tie-fei.zang@freescale.com> wrote: > > From: Jerry Huang <Chang-Ming.Huang@freescale.com> > > > > Add auto CMD12 command support for eSDHC driver. > > This is needed by P4080 and P1022 for block read/write. > > Manual asynchronous CMD12 abort operation causes protocol > violations on > > these silicons. > > > > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> > > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com> > > --- > > drivers/mmc/host/sdhci-of-core.c | 4 ++++ > > drivers/mmc/host/sdhci.c | 14 ++++++++++++-- > > drivers/mmc/host/sdhci.h | 2 ++ > > 3 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index c6d1bd8..a92566e 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -817,8 +817,12 @@ static void > sdhci_set_transfer_mode(struct sdhci_host *host, > > WARN_ON(!host->data); > > > > mode = SDHCI_TRNS_BLK_CNT_EN; > > - if (data->blocks > 1) > > - mode |= SDHCI_TRNS_MULTI; > > + if (data->blocks > 1) { > > + if (host->quirks & > SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > > + mode |= SDHCI_TRNS_MULTI | > SDHCI_TRNS_ACMD12; > > + else > > + mode |= SDHCI_TRNS_MULTI; > > nit: > + mode |= SDHCI_TRNS_MULTI; > + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > + mode |= SDHCI_TRNS_ACMD12; > > Clearer, no? why? Roy
On Wed, Aug 4, 2010 at 8:14 PM, Zang Roy-R61911 <r61911@freescale.com> wrote: >> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> > index c6d1bd8..a92566e 100644 >> > --- a/drivers/mmc/host/sdhci.c >> > +++ b/drivers/mmc/host/sdhci.c >> > @@ -817,8 +817,12 @@ static void >> sdhci_set_transfer_mode(struct sdhci_host *host, >> > WARN_ON(!host->data); >> > >> > mode = SDHCI_TRNS_BLK_CNT_EN; >> > - if (data->blocks > 1) >> > - mode |= SDHCI_TRNS_MULTI; >> > + if (data->blocks > 1) { >> > + if (host->quirks & >> SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) >> > + mode |= SDHCI_TRNS_MULTI | >> SDHCI_TRNS_ACMD12; >> > + else >> > + mode |= SDHCI_TRNS_MULTI; >> >> nit: >> + mode |= SDHCI_TRNS_MULTI; >> + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) >> + mode |= SDHCI_TRNS_ACMD12; >> >> Clearer, no? > why? Shorter lines, fewer lines, and the SDHCI_TRNS_MULTI is more obviously unconditional. But as I said, it is a nitpick. g.
On Wed, Aug 04, 2010 at 07:02:56PM -0600, Grant Likely wrote: > On Mon, Aug 2, 2010 at 9:11 PM, Roy Zang <tie-fei.zang@freescale.com> wrote: > > From: Jerry Huang <Chang-Ming.Huang@freescale.com> > > > > Add auto CMD12 command support for eSDHC driver. > > This is needed by P4080 and P1022 for block read/write. > > Manual asynchronous CMD12 abort operation causes protocol violations on > > these silicons. > > > > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> > > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com> > > --- > > drivers/mmc/host/sdhci-of-core.c | 4 ++++ > > drivers/mmc/host/sdhci.c | 14 ++++++++++++-- > > drivers/mmc/host/sdhci.h | 2 ++ > > 3 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index c6d1bd8..a92566e 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -817,8 +817,12 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host, > > WARN_ON(!host->data); > > > > mode = SDHCI_TRNS_BLK_CNT_EN; > > - if (data->blocks > 1) > > - mode |= SDHCI_TRNS_MULTI; > > + if (data->blocks > 1) { > > + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > > + mode |= SDHCI_TRNS_MULTI | SDHCI_TRNS_ACMD12; > > + else > > + mode |= SDHCI_TRNS_MULTI; > > nit: > + mode |= SDHCI_TRNS_MULTI; > + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > + mode |= SDHCI_TRNS_ACMD12; > > Clearer, no? Much clearer. I would prefer the nit incorporated. Another nit: > @@ -154,6 +154,10 @@ static int __devinit sdhci_of_probe(struct of_device *ofdev, > host->ops = &sdhci_of_data->ops; > } > > + if (of_get_property(np, "sdhci,auto-cmd12", NULL)) > + host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12; > + > + ^^ No need for the two empty lines. > if (of_get_property(np, "sdhci,1-bit-only", NULL)) Though, technically the patch looks OK, feel free to add my Acked-by: Anton Vorontsov <cbouatmailru@gmail.com> on the next resend (if any). Thanks Roy!
2010/8/3 Roy Zang <tie-fei.zang@freescale.com>: [...] > @@ -240,6 +240,8 @@ struct sdhci_host { > #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN (1<<25) > /* Controller cannot support End Attribute in NOP ADMA descriptor */ > #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC (1<<26) > +/* Controller uses Auto CMD12 command to stop the transfer */ > +#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<27) > > int irq; /* Device IRQ */ > void __iomem * ioaddr; /* Mapped address */ Just a cosmetic hint: I suggest SDHCI_QUIRK_MULTIBLOCK_READ_AUTO_CMD12 or something for the quirk name, because ACMD12 part might be confused with MMC/SD App CMD 12 (CMD55+CMD12 combo) if/whenever that gets used. Best Regards, Michał Mirosław
> -----Original Message----- > From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On > Behalf Of Grant Likely > Sent: Thursday, August 05, 2010 9:03 AM > To: Zang Roy-R61911 > Cc: linux-mmc@vger.kernel.org; linuxppc-dev@ozlabs.org; > akpm@linux-foundation.org > Subject: Re: [PATCH 1/3 v2] sdhci: Add auto CMD12 support for > eSDHC driver > > On Mon, Aug 2, 2010 at 9:11 PM, Roy Zang > <tie-fei.zang@freescale.com> wrote: > > From: Jerry Huang <Chang-Ming.Huang@freescale.com> > > > > Add auto CMD12 command support for eSDHC driver. > > This is needed by P4080 and P1022 for block read/write. > > Manual asynchronous CMD12 abort operation causes protocol > violations on > > these silicons. > > > > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> > > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com> > > --- > > drivers/mmc/host/sdhci-of-core.c | 4 ++++ > > drivers/mmc/host/sdhci.c | 14 ++++++++++++-- > > drivers/mmc/host/sdhci.h | 2 ++ > > 3 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index c6d1bd8..a92566e 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -817,8 +817,12 @@ static void > sdhci_set_transfer_mode(struct sdhci_host *host, > > WARN_ON(!host->data); > > > > mode = SDHCI_TRNS_BLK_CNT_EN; > > - if (data->blocks > 1) > > - mode |= SDHCI_TRNS_MULTI; > > + if (data->blocks > 1) { > > + if (host->quirks & > SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > > + mode |= SDHCI_TRNS_MULTI | > SDHCI_TRNS_ACMD12; > > + else > > + mode |= SDHCI_TRNS_MULTI; > > nit: > + mode |= SDHCI_TRNS_MULTI; > + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > + mode |= SDHCI_TRNS_ACMD12; > > Clearer, no? It is clear. Thanks. Roy
> -----Original Message----- > From: Anton Vorontsov [mailto:cbouatmailru@gmail.com] > Sent: Tuesday, August 10, 2010 1:44 AM > To: Grant Likely > Cc: Zang Roy-R61911; linuxppc-dev@ozlabs.org; > akpm@linux-foundation.org; linux-mmc@vger.kernel.org > Subject: Re: [PATCH 1/3 v2] sdhci: Add auto CMD12 support for > eSDHC driver > > On Wed, Aug 04, 2010 at 07:02:56PM -0600, Grant Likely wrote: > > On Mon, Aug 2, 2010 at 9:11 PM, Roy Zang > <tie-fei.zang@freescale.com> wrote: > > > From: Jerry Huang <Chang-Ming.Huang@freescale.com> > > > > > > Add auto CMD12 command support for eSDHC driver. > > > This is needed by P4080 and P1022 for block read/write. > > > Manual asynchronous CMD12 abort operation causes protocol > violations on > > > these silicons. > > > > > > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> > > > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com> > > > --- > > > drivers/mmc/host/sdhci-of-core.c | 4 ++++ > > > drivers/mmc/host/sdhci.c | 14 ++++++++++++-- > > > drivers/mmc/host/sdhci.h | 2 ++ > > > 3 files changed, 18 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > > index c6d1bd8..a92566e 100644 > > > --- a/drivers/mmc/host/sdhci.c > > > +++ b/drivers/mmc/host/sdhci.c > > > @@ -817,8 +817,12 @@ static void > sdhci_set_transfer_mode(struct sdhci_host *host, > > > WARN_ON(!host->data); > > > > > > mode = SDHCI_TRNS_BLK_CNT_EN; > > > - if (data->blocks > 1) > > > - mode |= SDHCI_TRNS_MULTI; > > > + if (data->blocks > 1) { > > > + if (host->quirks & > SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > > > + mode |= SDHCI_TRNS_MULTI | > SDHCI_TRNS_ACMD12; > > > + else > > > + mode |= SDHCI_TRNS_MULTI; > > > > nit: > > + mode |= SDHCI_TRNS_MULTI; > > + if (host->quirks & > SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > > + mode |= SDHCI_TRNS_ACMD12; > > > > Clearer, no? > > Much clearer. I would prefer the nit incorporated. Agree. > > Another nit: > > > @@ -154,6 +154,10 @@ static int __devinit > sdhci_of_probe(struct of_device *ofdev, > > host->ops = &sdhci_of_data->ops; > > } > > > > + if (of_get_property(np, "sdhci,auto-cmd12", NULL)) > > + host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12; > > + > > + > > ^^ No need for the two empty lines. Agree. > > > if (of_get_property(np, "sdhci,1-bit-only", NULL)) > > Though, technically the patch looks OK, feel free to add my > > Acked-by: Anton Vorontsov <cbouatmailru@gmail.com> I send a new patch to fix the nip. you are Cced. Thanks. Roy
> -----Original Message----- > From: Michał Mirosław [mailto:mirqus@gmail.com] > Sent: Tuesday, August 10, 2010 2:25 AM > To: Zang Roy-R61911 > Cc: linux-mmc@vger.kernel.org; linuxppc-dev@ozlabs.org; > akpm@linux-foundation.org > Subject: Re: [PATCH 1/3 v2] sdhci: Add auto CMD12 support for > eSDHC driver > > 2010/8/3 Roy Zang <tie-fei.zang@freescale.com>: > [...] > > @@ -240,6 +240,8 @@ struct sdhci_host { > > #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN (1<<25) > > /* Controller cannot support End Attribute in NOP ADMA > descriptor */ > > #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC (1<<26) > > +/* Controller uses Auto CMD12 command to stop the transfer */ > > +#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<27) > > > > int irq; /* Device IRQ */ > > void __iomem * ioaddr; /* Mapped address */ > > Just a cosmetic hint: I suggest SDHCI_QUIRK_MULTIBLOCK_READ_AUTO_CMD12 > or something for the quirk name, because ACMD12 part might be confused > with MMC/SD App CMD 12 (CMD55+CMD12 combo) if/whenever that gets used. Thanks for the suggestion. There are several ACMD12 needed to be updated. Send a new patch to update it. Roy
diff --git a/drivers/mmc/host/sdhci-of-core.c b/drivers/mmc/host/sdhci-of-core.c index a2e9820..dd1bdd1 100644 --- a/drivers/mmc/host/sdhci-of-core.c +++ b/drivers/mmc/host/sdhci-of-core.c @@ -154,6 +154,10 @@ static int __devinit sdhci_of_probe(struct of_device *ofdev, host->ops = &sdhci_of_data->ops; } + if (of_get_property(np, "sdhci,auto-cmd12", NULL)) + host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12; + + if (of_get_property(np, "sdhci,1-bit-only", NULL)) host->quirks |= SDHCI_QUIRK_FORCE_1_BIT_DATA; diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index c6d1bd8..a92566e 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -817,8 +817,12 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host, WARN_ON(!host->data); mode = SDHCI_TRNS_BLK_CNT_EN; - if (data->blocks > 1) - mode |= SDHCI_TRNS_MULTI; + if (data->blocks > 1) { + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) + mode |= SDHCI_TRNS_MULTI | SDHCI_TRNS_ACMD12; + else + mode |= SDHCI_TRNS_MULTI; + } if (data->flags & MMC_DATA_READ) mode |= SDHCI_TRNS_READ; if (host->flags & SDHCI_REQ_USE_DMA) @@ -1108,6 +1112,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq) #ifndef SDHCI_USE_LEDS_CLASS sdhci_activate_led(host); #endif + if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) { + if (mrq->stop) { + mrq->data->stop = NULL; + mrq->stop = NULL; + } + } host->mrq = mrq; diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index c846813..8fb088c 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -240,6 +240,8 @@ struct sdhci_host { #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN (1<<25) /* Controller cannot support End Attribute in NOP ADMA descriptor */ #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC (1<<26) +/* Controller uses Auto CMD12 command to stop the transfer */ +#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<27) int irq; /* Device IRQ */ void __iomem * ioaddr; /* Mapped address */