On 01/06/18 09:53, Y.b. Lu wrote: >> -----Original Message----- >> From: Ulf Hansson [mailto:ulf.hansson@xxxxxxxxxx] >> Sent: Friday, June 1, 2018 2:38 PM >> To: Y.b. Lu <yangbo.lu@xxxxxxx> >> Cc: Yinbo Zhu <yinbo.zhu@xxxxxxx>; Adrian Hunter >> <adrian.hunter@xxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; Xiaobo Xie >> <xiaobo.xie@xxxxxxx> >> Subject: Re: [PATCH v2] mmc: sdhci-of-esdhc: modify the sd clock in >> soc_device_match way >> >> On 1 June 2018 at 08:31, Y.b. Lu <yangbo.lu@xxxxxxx> wrote: >>>> -----Original Message----- >>>> From: Yinbo Zhu >>>> Sent: Monday, May 28, 2018 5:58 PM >>>> To: Adrian Hunter <adrian.hunter@xxxxxxxxx>; Y.b. Lu >>>> <yangbo.lu@xxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx >>>> Cc: Xiaobo Xie <xiaobo.xie@xxxxxxx> >>>> Subject: RE: [PATCH v2] mmc: sdhci-of-esdhc: modify the sd clock in >>>> soc_device_match way >>>> >>>> >>>> >>>> -----Original Message----- >>>> From: Adrian Hunter [mailto:adrian.hunter@xxxxxxxxx] >>>> Sent: 2018年5月24日 16:58 >>>> To: Yinbo Zhu <yinbo.zhu@xxxxxxx>; Y.b. Lu <yangbo.lu@xxxxxxx>; >>>> linux-mmc@xxxxxxxxxxxxxxx >>>> Cc: Xiaobo Xie <xiaobo.xie@xxxxxxx> >>>> Subject: Re: [PATCH v2] mmc: sdhci-of-esdhc: modify the sd clock in >>>> soc_device_match way >>>> >>>> On 22/05/18 15:06, Yinbo Zhu wrote: >>> [...] >>>>> + switch (host->mmc->ios.timing) { >>>>> + case MMC_TIMING_LEGACY: >>>>> + fixup = esdhc->clk_fixup->ds; >>>>> + clock = esdhc_clock_fixup(clock, FULL_SPEED_MAX_DTR, >> fixup); >>>>> + fixup = esdhc->clk_fixup->mmc_ds; >>>>> + clock = esdhc_clock_fixup(clock, MMC_HIGH_26_MAX_DTR, >> fixup); >>>>> + break; >>>>> + case MMC_TIMING_SD_HS: >>>>> + fixup = esdhc->clk_fixup->hs; >>>>> + clock = esdhc_clock_fixup(clock, HIGH_SPEED_MAX_DTR, >> fixup); >>>>> + break; >>>>> + case MMC_TIMING_MMC_HS: >>>>> + fixup = esdhc->clk_fixup->mmc_hs; >>>>> + clock = esdhc_clock_fixup(clock, MMC_HIGH_52_MAX_DTR, >> fixup); >>>>> + break; >>>>> + case MMC_TIMING_UHS_SDR104: >>>>> + fixup = esdhc->clk_fixup->sdr104; >>>>> + clock = esdhc_clock_fixup(clock, UHS_SDR104_MAX_DTR, >> fixup); >>>>> + break; >>>>> + case MMC_TIMING_MMC_HS200: >>>>> + fixup = esdhc->clk_fixup->hs200; >>>>> + clock = esdhc_clock_fixup(clock, MMC_HS200_MAX_DTR, >> fixup); >>>>> + break; >>>>> + default: >>>>> + break; >>>> >>>>> This logic looks fragile. I would expect to see something that >>>>> looks more >>>> like: >>>> >>>>> max_clk = esdhc->clk_fixup[host->mmc->ios.timing]; >>>>> if (max_clk && clock > max_clk) >>>>> clock = max_CLK; >>>> Hi Adrian Hunter, >>>> >>>> Thanks your advice, your way will make code logic more clearly, but >>>> MMC default max clock is 26MHz, SD default max clock is 25MHz, they >>>> all use the same ios.timing in public code, If use array, There will >>>> be a conflict, Do you have any good ways to resolve the conflict. >>>> Thanks. >>>> >>>> Regards, >>>> Yinbo >>>> >>> >>> [Y.b. Lu] The MMC core driver used the timing MMC_TIMING_LEGACY for >> both MMC and SD/SDIO at card initialization. >>> However, the timing was different between MMC and SD/SDIO at >> initialization. MMC was at legacy speed mode (up to 26MHz) while SD/SDIO >> was at default speed mode (up to 25MHz). >>> Actually I think we should define MMC_TIMING_MMC_LEGACY and >> MMC_TIMING_SD_DEFAULT for MMC and SD/SDIO initializing timing. >>> And use the right timing at the beginning in mmc_attach_sd/sdio/mmc(). >>> Any suggestion? Adrian and Uffe :) >> >> I am fine with that. However, you need to make sure all host drivers can cope >> with changing this. >> >> Perhaps, it's also sufficient to introduce MMC_TIMING_DEFAULT (used for SD) >> and keep MMC_TIMING_LEGACY (for MMC). > > [Y.b. Lu] Great. It's safe to just introduce MMC_TIMING_DEFAULT. Thanks. Several drivers check MMC_TIMING_LEGACY and we don't know if it is MMC or SD when initializing. I suggest just making a special case in your logic: if (host->card && mmc_card_sd(host->card) && host->mmc->ios.timing == MMC_TIMING_LEGACY) max_clk = esdhc->clk_fixup.sd_dflt_max_clk; else max_clk = esdhc->clk_fixup.max_clk[host->mmc->ios.timing]; > > >> >>> >>> #define MMC_TIMING_LEGACY 0 >>> #define MMC_TIMING_MMC_LEGACY 1 >>> #define MMC_TIMING_SD_DEFAULT 2 >>> #define MMC_TIMING_MMC_HS 3 >>> #define MMC_TIMING_SD_HS 4 >>> #define MMC_TIMING_UHS_SDR12 5 >>> #define MMC_TIMING_UHS_SDR25 6 >>> #define MMC_TIMING_UHS_SDR50 7 >>> #define MMC_TIMING_UHS_SDR104 8 >>> #define MMC_TIMING_UHS_DDR50 9 >>> #define MMC_TIMING_MMC_DDR52 10 >>> #define MMC_TIMING_MMC_HS200 11 >>> #define MMC_TIMING_MMC_HS400 12 >>> >> >> Kind regards >> Uffe -- 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
