Re: [PATCH v2 2/3] mmc: tmio: add eMMC HS400 mode support

From: Wolfram Sang <wsa@the-dreams.de>
To: Simon Horman <horms@verge.net.au>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Magnus Damm <magnus.damm@gmail.com>,
	linux-mmc@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v2 2/3] mmc: tmio: add eMMC HS400 mode support
Date: Tue, 13 Feb 2018 14:00:44 +0100	[thread overview]
Message-ID: <20180213130044.zu22wwhq2y4phnri@katana> (raw)
In-Reply-To: <20180213113353.tioy73y2ggsat7m3@verge.net.au>

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

On Tue, Feb 13, 2018 at 12:33:53PM +0100, Simon Horman wrote:
> On Wed, Feb 07, 2018 at 11:20:12PM +0100, Wolfram Sang wrote:
> > 
> > Hi Simon,
> > 
> > > +	void (*disable_scc)(struct mmc_host *mmc);
> > 
> > Do we really need this callback? I'd think it can be folded into
> > reset_hs400_mode() because it is called only once?
> > 
> > > +	void (*prepare_hs400_tuning)(struct mmc_host *mmc, struct mmc_ios *ios);
> > 
> > Can't we use the host->ops->prepare_hs400_tuning() callback invoked by
> > the core?
> 
> Empirically that does not seem to work.

:( Pity.

> > > +	void (*reset_hs400_mode)(struct mmc_host *mmc);
> > 
> > Maybe we can get rid of this, too? See later...
> > 
> > > +	if (host->disable_scc)
> > > +		host->disable_scc(mmc);
> > 
> > (Here, this can be folded into the next callback)
> 
> Yes, agreed. I've folded the callbacks as you suggest.

Cool, thanks!

> > > +
> > > +	/* reset HS400 mode */
> > > +	if (ios->timing != MMC_TIMING_MMC_HS400 && host->reset_hs400_mode)
> > > +		host->reset_hs400_mode(mmc);
> > 
> > I wonder: If for any ios which is != MMC_TIMING_MMC_HS400, the
> > hs400_mode needs to be reset. Couldn't we as well then disable the mode
> > always after the MMC_TIMING_MMC_HS400 tuning was selected? Just
> > brainstorming here...
> 
> Perhaps but I'm unsure where we would hook in this change, any ideas?

I think this becomes moot if we can't get the prepare-callback of the
core to work. I'd think it makes sense to have both callbacks from here
moved to the core or none to keep at least one kind of symmetry.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]