-----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: > From: yinbo.zhu <yinbo.zhu@xxxxxxx> > > Convert to use soc_device_match method to fix up eSDHC clock for > ls1046a/ls1012a/p1010. Also add eSDHC clock fixup for ls1021a > according to its datasheet. The maxmum speed for ls1021a eSDHC high > speed mode is 46.5MHz. > > Signed-off-by: Yinbo Zhu <yinbo.zhu@xxxxxxx> > --- > drivers/mmc/host/sdhci-of-esdhc.c | 105 > ++++++++++++++++++++++++++++++------- > 1 files changed, 85 insertions(+), 20 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c > b/drivers/mmc/host/sdhci-of-esdhc.c > index 4ffa6b1..4d82c6d 100644 > --- a/drivers/mmc/host/sdhci-of-esdhc.c > +++ b/drivers/mmc/host/sdhci-of-esdhc.c > @@ -29,11 +29,58 @@ > #define VENDOR_V_22 0x12 > #define VENDOR_V_23 0x13 > > +struct esdhc_clk_fixup { > + /* SD speed modes */ > + long ds; > + long hs; > + long sdr12; > + long sdr25; > + long sdr50; > + long sdr104; > + long ddr50; > + /* MMC speed modes */ > + long mmc_ds; > + long mmc_hs; > + long hs200; > + long ddr52; > +}; > + > +static struct esdhc_clk_fixup ls1021a_esdhc_clk = { > + .mmc_hs = 46500000, > + .hs = 46500000, > +}; > + > +static struct esdhc_clk_fixup ls1046a_esdhc_clk = { > + .sdr104 = 167000000, > + .hs200 = 167000000, > +}; > + > +static struct esdhc_clk_fixup ls1012a_esdhc_clk = { > + .sdr104 = 125000000, > + .hs200 = 125000000, > +}; > + > +static struct esdhc_clk_fixup p1010_esdhc_clk = { > + .ds = 20000000, > + .hs = 40000000, > + .mmc_ds = 20000000, > + .mmc_hs = 42000000, > +}; > + > +static struct soc_device_attribute soc_esdhc_clk_fixup[] = { > + { .family = "QorIQ LS1021A", .data = &ls1021a_esdhc_clk}, > + { .family = "QorIQ LS1046A", .data = &ls1046a_esdhc_clk}, > + { .family = "QorIQ LS1012A", .data = &ls1012a_esdhc_clk}, > + { .family = "QorIQ P1010", .data = &p1010_esdhc_clk}, > + {}, > +}; >These static structs could be const also. Thanks, I'm going to correct that > + > struct sdhci_esdhc { > u8 vendor_ver; > u8 spec_ver; > bool quirk_incorrect_hostver; > unsigned int peripheral_clock; > + const struct esdhc_clk_fixup *clk_fixup; > }; > > /** > @@ -485,6 +532,14 @@ static void esdhc_clock_enable(struct sdhci_host *host, bool enable) > } > } > > +static long esdhc_clock_fixup(long clock, long clock_standard, long > +fixup) { > + if (!fixup) > + return clock; > + > + return (clock == clock_standard) ? fixup : clock; } > + > static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int > clock) { > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -492,6 > +547,7 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock) > int pre_div = 1; > int div = 1; > ktime_t timeout; > + long fixup; > u32 temp; > > host->mmc->actual_clock = 0; > @@ -505,26 +561,31 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock) > if (esdhc->vendor_ver < VENDOR_V_23) > pre_div = 2; > > - /* > - * Limit SD clock to 167MHz for ls1046a according to its datasheet > - */ > - if (clock > 167000000 && > - of_find_compatible_node(NULL, NULL, "fsl,ls1046a-esdhc")) > - clock = 167000000; > - > - /* > - * Limit SD clock to 125MHz for ls1012a according to its datasheet > - */ > - if (clock > 125000000 && > - of_find_compatible_node(NULL, NULL, "fsl,ls1012a-esdhc")) > - clock = 125000000; > - > - /* Workaround to reduce the clock frequency for p1010 esdhc */ > - if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) { > - if (clock > 20000000) > - clock -= 5000000; > - if (clock > 40000000) > - clock -= 5000000; > + 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 > } > > temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL); @@ -783,6 +844,7 @@ > static SIMPLE_DEV_PM_OPS(esdhc_of_dev_pm_ops, > > static void esdhc_init(struct platform_device *pdev, struct > sdhci_host *host) { > + const struct soc_device_attribute *matches; > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_esdhc *esdhc; > struct device_node *np; > @@ -802,6 +864,9 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host) > else > esdhc->quirk_incorrect_hostver = false; > > + matches = soc_device_match(soc_esdhc_clk_fixup); > + if (matches) > + esdhc->clk_fixup = matches->data; > np = pdev->dev.of_node; > clk = of_clk_get(np, 0); > if (!IS_ERR(clk)) { > ��.n��������+%������w��{.n�����{��i��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
