Re: [PATCH/RFC 2/3] mmc: sh_mobile_sdhi: Add tuning support

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

 



On Tue, May 10, 2016 at 06:25:58AM +0000, Kuninori Morimoto wrote:
> 
> Hi Simon
> 
> Some question/comment from me
> 
> about new flags "TMIO_MMC_HAS_UHS_SCC",
> you added it on [1/3] patch with this comment
> "Remove unused TMIO_MMC_HAS_UHS_SCC define"
> but, [1/3] patch adds it, not removed ?

Thanks, the comment got out of sync with the code at some point.

> > +static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
> > +{
> > +	struct tmio_mmc_data *pdata = host->pdata;
> > +
> > +	if (pdata->flags & TMIO_MMC_HAS_UHS_SCC) {
> > +		/* Reset SCC */
> (snip)
> > +	if (mmc_data->capabilities & MMC_CAP_UHS_SDR104) {
> > +		mmc_data->capabilities |= MMC_CAP_HW_RESET;
> > +		mmc_data->flags |= TMIO_MMC_HAS_UHS_SCC;
> > +	}
> 
> And, how about this on sh_mobile_sdhi_hw_reset() ?
> We can avoid to add new flags TMIO_MMC_HAS_UHS_SCC ?
> 
> static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
> {
> 	...
> 
> 	if (pdata->capabilities & MMC_CAP_UHS_SDR104) {
> 		...
> 	}

That seems reasonable, I'll see if I can get it to work.

> > +static inline void sd_scc_write32(struct tmio_mmc_host *host, int addr,
> > +				  u32 val)
> > +{
> > +	struct platform_device *pdev = host->pdev;
> > +	const struct of_device_id *of_id =
> > +		of_match_device(sh_mobile_sdhi_of_match, &pdev->dev);
> > +	const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
> > +
> > +	writel(val, host->ctl + of_data->scc_offset +
> > +	       (addr << host->bus_shift));
> > +}
> (snip)
> >  static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
> >  	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE |
> >  			  TMIO_MMC_CLK_ACTUAL | TMIO_MMC_MIN_RCAR2,
> >  	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
> >  	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_4_BYTES,
> >  	.dma_rx_offset	= 0x2000,
> > +	.scc_offset	= 0x0300,
> > +	.taps		= rcar_gen2_scc_taps,
> > +	.taps_num	= ARRAY_SIZE(rcar_gen2_scc_taps),
> >  };
> (snip)
> > +	host->set_clk_div	= sh_mobile_sdhi_set_clk_div;
> >  	host->dma		= dma_priv;
> >  	host->write16_hook	= sh_mobile_sdhi_write16_hook;
> >  	host->clk_enable	= sh_mobile_sdhi_clk_enable;
> > @@ -364,6 +596,12 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
> >  	host->clk_disable	= sh_mobile_sdhi_clk_disable;
> >  	host->multi_io_quirk	= sh_mobile_sdhi_multi_io_quirk;
> >  	host->start_signal_voltage_switch = sh_mobile_sdhi_start_signal_voltage_switch;
> > +	host->inquiry_tuning	= sh_mobile_sdhi_inquiry_tuning;
> > +	host->init_tuning	= sh_mobile_sdhi_init_tuning;
> > +	host->prepare_tuning	= sh_mobile_sdhi_prepare_tuning;
> > +	host->select_tuning	= sh_mobile_sdhi_select_tuning;
> > +	host->retuning		= sh_mobile_sdhi_retuning;
> > +	host->hw_reset		= sh_mobile_sdhi_hw_reset;
> 
> You registered new callback functions on this driver,
> and it is using sd_scc_read/write function, and it is based on "scc_offset".
> This driver is used from not only Gen2, but also Gen1/Gen3/SH-Mobile.
> But you added .scc_offset only on of_rcar_gen2_compatible.
> What's happen on Gen1/Gen3/SH-Mobile ?

Probably not much good :(

I'll work on correcting that.

> > +static bool sh_mobile_sdhi_inquiry_tuning(struct tmio_mmc_host *host)
> > +{
> > +	/* SDHI should be tuning only SDR104 */
> > +	if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104)
> > +		return true;
> > +	else
> > +		return false;
> > +}
> 
> How about this ?
> 
> 	return (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104);

Sure, thanks for the suggestion.



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux