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.