RE: [RESEND PATCH] mmc: omap_hsmmc: Enable HSPE bit for high speed cards

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 31, 2012 at 11:14:28, Balbi, Felipe wrote:
> Hi,
> 
> On Wed, Oct 31, 2012 at 06:17:02AM +0100, Hebbar, Gururaja wrote:
> > On Mon, Oct 29, 2012 at 21:47:19, Balbi, Felipe wrote:
> > > Hi,
> > > 
> > > On Mon, Oct 29, 2012 at 06:26:48PM +0530, Hebbar, Gururaja wrote:
> > > > HSMMC IP on AM33xx need a special setting to handle High-speed cards.
> > > > Other platforms like TI81xx, OMAP4 may need this as-well. This depends
> > > > on the HSMMC IP timing closure done for the high speed cards.
> > > > 
> > > > From AM335x TRM (SPRUH73F - 18.3.12 Output Signals Generation)
> > > > 
> > > > The MMC/SD/SDIO output signals can be driven on either falling edge or
> > > > rising edge depending on the SD_HCTL[2] HSPE bit. This feature allows
> > > > to reach better timing performance, and thus to increase data transfer
> > > > frequency.
> > > > 
> > > > There are few pre-requisites for enabling the HSPE bit
> > > > - Controller should support High-Speed-Enable Bit and
> > > > - Controller should not be using DDR Mode and
> > > > - Controller should advertise that it supports High Speed in
> > > >   capabilities register and
> > > > - MMC/SD clock coming out of controller > 25MHz
> > > > 
> > > > Signed-off-by: Hebbar, Gururaja <gururaja.hebbar@xxxxxx>
> > > > ---
> > > > Rebased on mmc-next (v3.7.0-rc1)
> > > > Only Build tested since EDMA support for AM335x is not yet added
> > > > 
> > > > :100644 100644 be76a23... ed271fc... M	Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> > > > :100644 100644 8b4e4f2... 346af5b... M	arch/arm/plat-omap/include/plat/mmc.h
> > > > :100644 100644 54bfd0c... 3fd95cb... M	drivers/mmc/host/omap_hsmmc.c
> > > >  .../devicetree/bindings/mmc/ti-omap-hsmmc.txt      |    1 +
> > > >  arch/arm/plat-omap/include/plat/mmc.h              |    1 +
> > > >  drivers/mmc/host/omap_hsmmc.c                      |   30 +++++++++++++++++++-
> > > >  3 files changed, 31 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> > > > index be76a23..ed271fc 100644
> > > > --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> > > > +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> > > > @@ -19,6 +19,7 @@ ti,dual-volt: boolean, supports dual voltage cards
> > > >  "supply-name" examples are "vmmc", "vmmc_aux" etc
> > > >  ti,non-removable: non-removable slot (like eMMC)
> > > >  ti,needs-special-reset: Requires a special softreset sequence
> > > > +ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
> > > 
> > > can't you do runtime revision detection for this ?
> > 
> > Platforms like AM335x & TI814x both use the same IP. 
> > But only AM335x needed this bit to be set in order to sustain high
> > speed transfers. This was concluded after AM335x HSMMC IP timing
> > closure was completed for High speed Cards.
> 
> fair enough.
> 
> > > >  Example:
> > > >  	mmc1: mmc@0x4809c000 {
> > > > diff --git a/arch/arm/plat-omap/include/plat/mmc.h b/arch/arm/plat-omap/include/plat/mmc.h
> > > > index 8b4e4f2..346af5b 100644
> > > > --- a/arch/arm/plat-omap/include/plat/mmc.h
> > > > +++ b/arch/arm/plat-omap/include/plat/mmc.h
> > > > @@ -126,6 +126,7 @@ struct omap_mmc_platform_data {
> > > >  		/* we can put the features above into this variable */
> > > >  #define HSMMC_HAS_PBIAS		(1 << 0)
> > > >  #define HSMMC_HAS_UPDATED_RESET	(1 << 1)
> > > > +#define HSMMC_HAS_HSPE_SUPPORT	(1 << 2)
> > > >  		unsigned features;
> > > >  
> > > >  		int switch_pin;			/* gpio (card detect) */
> > > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> > > > index 54bfd0c..3fd95cb 100644
> > > > --- a/drivers/mmc/host/omap_hsmmc.c
> > > > +++ b/drivers/mmc/host/omap_hsmmc.c
> > > > @@ -62,6 +62,7 @@
> > > >  
> > > >  #define VS18			(1 << 26)
> > > >  #define VS30			(1 << 25)
> > > > +#define HSS			(1 << 21)
> > > >  #define SDVS18			(0x5 << 9)
> > > >  #define SDVS30			(0x6 << 9)
> > > >  #define SDVS33			(0x7 << 9)
> > > > @@ -89,6 +90,7 @@
> > > >  #define MSBS			(1 << 5)
> > > >  #define BCE			(1 << 1)
> > > >  #define FOUR_BIT		(1 << 1)
> > > > +#define HSPE			(1 << 2)
> > > >  #define DDR			(1 << 19)
> > > >  #define DW8			(1 << 5)
> > > >  #define CC			0x1
> > > > @@ -489,6 +491,7 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
> > > >  	struct mmc_ios *ios = &host->mmc->ios;
> > > >  	unsigned long regval;
> > > >  	unsigned long timeout;
> > > > +	unsigned long clkdiv;
> > > >  
> > > >  	dev_vdbg(mmc_dev(host->mmc), "Set clock to %uHz\n", ios->clock);
> > > >  
> > > > @@ -496,7 +499,8 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
> > > >  
> > > >  	regval = OMAP_HSMMC_READ(host->base, SYSCTL);
> > > >  	regval = regval & ~(CLKD_MASK | DTO_MASK);
> > > > -	regval = regval | (calc_divisor(host, ios) << 6) | (DTO << 16);
> > > > +	clkdiv = calc_divisor(host, ios);
> > > > +	regval = regval | (clkdiv << 6) | (DTO << 16);
> > > 
> > > not part of $SUBJECT
> > 
> > The output of the calc_divisor() is reused in the below HSPE checking. So
> > instead of calling it twice I have taken the output of that function into
> > a variable and reused it.
> 
> looks like this could/should go into commit log ;-)

Ok I will update the commit message in V2.

> 
> > > >  	OMAP_HSMMC_WRITE(host->base, SYSCTL, regval);
> > > >  	OMAP_HSMMC_WRITE(host->base, SYSCTL,
> > > >  		OMAP_HSMMC_READ(host->base, SYSCTL) | ICE);
> > > > @@ -507,6 +511,27 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
> > > >  		&& time_before(jiffies, timeout))
> > > >  		cpu_relax();
> > > >  
> > > > +	/*
> > > > +	 * Enable High-Speed Support
> > > > +	 * Pre-Requisites
> > > > +	 *	- Controller should support High-Speed-Enable Bit
> > > > +	 *	- Controller should not be using DDR Mode
> > > > +	 *	- Controller should advertise that it supports High Speed
> > > > +	 *	  in capabilities register
> > > > +	 *	- MMC/SD clock coming out of controller > 25MHz
> > > > +	 */
> > > > +	if ((mmc_slot(host).features & HSMMC_HAS_HSPE_SUPPORT) &&
> > > 
> > > is this really needed...
> > 
> > As explained above, this is needed only on platforms which requires the
> > Bit to be set (currently AM335x). Such platforms will set this
> > parameter in it .dts file
> > 
> > > 
> > > > +	    (ios->timing != MMC_TIMING_UHS_DDR50) &&
> > > > +	    ((OMAP_HSMMC_READ(host->base, CAPA) & HSS) == HSS)) {
> > > 
> > > ... provided you check device's register ?
> > 
> > As explained above, both TI814x & AM335x support HSS but only AM335x
> > require HSPE bit set.
> 
> ok, understood. Just one small simplification:
> 
> if (a & BIT(n)) will have the same effect as if ((a & BIT(n) == BIT(n))),
> so you can simplify that a little bit.

Ok I will correct it in V2. 

Thanks for the review.

> 
> -- 
> balbi
> 


Regards, 
Gururaja
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux