| Message ID | 87617xfchj.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Morimoto-san, On Wed, May 13, 2015 at 4:18 AM, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > MMCIF IP on R-Car series has parent clock which can be set > several rate, and it was not implemented on old SH-Mobile series > (= SH-Mobile series parent clock was fixed rate) > R-Car series MMCIF can use more high speed access if it setup > parent clock. This patch adds parent clock setup method, > and r8a7790/r8a7791 can use it. Thanks for this update. I'm still wondering if we can do this without hardcoding frequencies in the driver... From the discussion before: "So the upper limit is a limitation of the MMCIF hardware, while the lower limit is a limitation of the CPG." > index e13c5f41..7ade263 100644 > --- a/drivers/mmc/host/sh_mmcif.c > +++ b/drivers/mmc/host/sh_mmcif.c > @@ -224,6 +225,15 @@ enum sh_mmcif_wait_for { > MMCIF_WAIT_FOR_STOP, > }; > > +/* > + * difference for each SoC > + */ > +struct sh_mmcif_ver { > + u32 clkdiv_map; /* see CE_CLK_CTRL::CLKDIV */ > + unsigned int pf_min; /* parent frequency min */ I don't think you need an explicit lower limit: clk_round_rate() will never return a frequency that's lower than the parent clock's lower limit. > + unsigned int pf_max; /* parent frequency max */ > +}; > + > struct sh_mmcif_host { > struct mmc_host *mmc; > struct mmc_request *mrq; > @@ -248,6 +258,7 @@ struct sh_mmcif_host { > bool ccs_enable; /* Command Completion Signal support */ > bool clk_ctrl2_enable; > struct mutex thread_lock; > + const struct sh_mmcif_ver *ver; > > /* DMA support */ > struct dma_chan *chan_rx; > @@ -256,8 +267,16 @@ struct sh_mmcif_host { > bool dma_active; > }; > > +static const struct sh_mmcif_ver sh_mmcif_gen2 = { > + .clkdiv_map = 0x3ff, > + .pf_min = 12187500, > + .pf_max = 97500000, By any chance, does setting "max-frequency = <97500000>" from the standard MMC DT bindings have the same effect? > +}; > + > static const struct of_device_id sh_mmcif_of_match[] = { > { .compatible = "renesas,sh-mmcif" }, > + { .compatible = "renesas,mmcif-r8a7790", .data = &sh_mmcif_gen2 }, > + { .compatible = "renesas,mmcif-r8a7791", .data = &sh_mmcif_gen2 }, > { } > }; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert Thank you for your feedback > > +/* > > + * difference for each SoC > > + */ > > +struct sh_mmcif_ver { > > + u32 clkdiv_map; /* see CE_CLK_CTRL::CLKDIV */ > > + unsigned int pf_min; /* parent frequency min */ > > I don't think you need an explicit lower limit: clk_round_rate() will > never return a frequency that's lower than the parent clock's lower limit. Indeed... Good catch > > +static const struct sh_mmcif_ver sh_mmcif_gen2 = { > > + .clkdiv_map = 0x3ff, > > + .pf_min = 12187500, > > + .pf_max = 97500000, > > By any chance, does setting "max-frequency = <97500000>" from the standard > MMC DT bindings have the same effect? I'm still not understanding about this. If we can specify SoC (via compatible), the clock frequency limitation can be specified in same time. Why do we need to have flexibility for it ? What is yuur "any chance" mean ? -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Morimoto-san, On Wed, May 13, 2015 at 10:37 AM, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: >> > +static const struct sh_mmcif_ver sh_mmcif_gen2 = { >> > + .clkdiv_map = 0x3ff, >> > + .pf_min = 12187500, >> > + .pf_max = 97500000, >> >> By any chance, does setting "max-frequency = <97500000>" from the standard >> MMC DT bindings have the same effect? > > I'm still not understanding about this. > If we can specify SoC (via compatible), the clock frequency limitation > can be specified in same time. > Why do we need to have flexibility for it ? It's better if you don't need to update the C source file for every new SoC, especially if you can use a standard MMC DT property instead. Still, you need it for clkdiv_map, right? > What is yuur "any chance" mean ? Just ignore those words. Does setting "max-frequency = <97500000>" have the same effect as specifying the parent clock upper limit in C? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert > > I'm still not understanding about this. > > If we can specify SoC (via compatible), the clock frequency limitation > > can be specified in same time. > > Why do we need to have flexibility for it ? > > It's better if you don't need to update the C source file for every new SoC, > especially if you can use a standard MMC DT property instead. > Still, you need it for clkdiv_map, right? Hmm... many people tell me different opinion... I understand your opinion, but I'm confusing about dilemma. If we can get all these settings via DT, (maybe) we don't need to update driver for every new SoC, yes. But it works if "this is 100% completed driver/HW". If we get settings from DT, but MMC IP was updated in the future, then, we need to care about ABI compatibility for it. And sometimes (anytime ?) Renesas IP is updated strangely. In such case, keeping ABI compatibility, and adds new feature will be super difficult. I discussed about this topic before with one engineer, and the final answer was "updating C source file is easier than updating DT / ABI compatibility" This is the big reason why I want to get settings from driver as much as possible. We need to update C source code for every new SoC, and Yes this is PITA, but easier than keeping ABI compatibility. And updating C source code for every new SoC can indicates "someone is caring this driver" to user Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c index e13c5f41..7ade263 100644 --- a/drivers/mmc/host/sh_mmcif.c +++ b/drivers/mmc/host/sh_mmcif.c @@ -57,6 +57,7 @@ #include <linux/mmc/slot-gpio.h> #include <linux/mod_devicetable.h> #include <linux/mutex.h> +#include <linux/of_device.h> #include <linux/pagemap.h> #include <linux/platform_device.h> #include <linux/pm_qos.h> @@ -224,6 +225,15 @@ enum sh_mmcif_wait_for { MMCIF_WAIT_FOR_STOP, }; +/* + * difference for each SoC + */ +struct sh_mmcif_ver { + u32 clkdiv_map; /* see CE_CLK_CTRL::CLKDIV */ + unsigned int pf_min; /* parent frequency min */ + unsigned int pf_max; /* parent frequency max */ +}; + struct sh_mmcif_host { struct mmc_host *mmc; struct mmc_request *mrq; @@ -248,6 +258,7 @@ struct sh_mmcif_host { bool ccs_enable; /* Command Completion Signal support */ bool clk_ctrl2_enable; struct mutex thread_lock; + const struct sh_mmcif_ver *ver; /* DMA support */ struct dma_chan *chan_rx; @@ -256,8 +267,16 @@ struct sh_mmcif_host { bool dma_active; }; +static const struct sh_mmcif_ver sh_mmcif_gen2 = { + .clkdiv_map = 0x3ff, + .pf_min = 12187500, + .pf_max = 97500000, +}; + static const struct of_device_id sh_mmcif_of_match[] = { { .compatible = "renesas,sh-mmcif" }, + { .compatible = "renesas,mmcif-r8a7790", .data = &sh_mmcif_gen2 }, + { .compatible = "renesas,mmcif-r8a7791", .data = &sh_mmcif_gen2 }, { } }; MODULE_DEVICE_TABLE(of, sh_mmcif_of_match); @@ -492,19 +511,56 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk) struct sh_mmcif_plat_data *p = dev->platform_data; bool sup_pclk = p ? p->sup_pclk : false; unsigned int current_clk = clk_get_rate(host->clk); + unsigned int clkdiv; sh_mmcif_bitclr(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE); sh_mmcif_bitclr(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR); if (!clk) return; - if (sup_pclk && clk == current_clk) - sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK); - else - sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR & - ((fls(DIV_ROUND_UP(current_clk, - clk) - 1) - 1) << 16)); + if (host->ver) { + const struct sh_mmcif_ver *ver = host->ver; + unsigned int freq, best_freq, myclk, div, diff_min, diff; + int i; + + clkdiv = 0; + diff_min = ~0; + best_freq = 0; + for (i = 31; i >= 0; i--) { + if (!((1 << i) & ver->clkdiv_map)) + continue; + + /* + * clk = parent_freq / div + * -> parent_freq = clk x div + */ + + div = 1 << (i + 1); + freq = clk_round_rate(host->clk, clk * div); + myclk = freq / div; + diff = (myclk > clk) ? myclk - clk : clk - myclk; + + if (diff <= diff_min) { + best_freq = freq; + clkdiv = i; + diff_min = diff; + } + } + + dev_dbg(dev, "clk %u/%u (%u, 0x%x)\n", + (best_freq / (1 << (clkdiv + 1))), clk, + best_freq, clkdiv); + + clk_set_rate(host->clk, best_freq); + clkdiv = clkdiv << 16; + } else if (sup_pclk && clk == current_clk) { + clkdiv = CLK_SUP_PCLK; + } else { + clkdiv = (fls(DIV_ROUND_UP(current_clk, clk) - 1) - 1) << 16; + } + + sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR & clkdiv); sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE); } @@ -1000,10 +1056,28 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq) static void sh_mmcif_clk_setup(struct sh_mmcif_host *host) { - unsigned int clk = clk_get_rate(host->clk); + struct device *dev = sh_mmcif_host_to_dev(host); + const struct of_device_id *of_id; + const struct sh_mmcif_ver *ver = NULL; + + of_id = of_match_device(sh_mmcif_of_match, dev); + if (of_id) + ver = of_id->data; + + if (ver) { + host->mmc->f_min = ver->pf_min / (1 << fls(ver->clkdiv_map)); + host->mmc->f_max = ver->pf_max / (1 << ffs(ver->clkdiv_map)); + + host->ver = ver; + } else { + unsigned int clk = clk_get_rate(host->clk); + + host->mmc->f_max = clk / 2; + host->mmc->f_min = clk / 512; + } - host->mmc->f_max = clk / 2; - host->mmc->f_min = clk / 512; + dev_dbg(dev, "clk max/min = %d/%d\n", + host->mmc->f_max, host->mmc->f_min); } static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)