| Message ID | 87r3rdubmv.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Morimoto-san, On Tue, Apr 21, 2015 at 10:31 AM, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > 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. > > renesas,mmcif already contain renesas,mmcif-r8a7790/r8a7791 on > compatible string. So there is no update for binding Document. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Tested-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c > index 5282c5b..6ae836b 100644 > --- a/drivers/mmc/host/sh_mmcif.c > +++ b/drivers/mmc/host/sh_mmcif.c > @@ -224,6 +225,12 @@ enum mmcif_wait_for { > MMCIF_WAIT_FOR_STOP, > }; > > +struct sh_mmcif_parent_clk { > + unsigned int max; /* if exist: R-Car has MMC CKCR on CPG */ > + unsigned int min; /* if exist: R-Car has MMC CKCR on CPG */ > + u32 clkdiv_map; /* see CE_CLK_CTRL::CLKDIV */ > +}; > + > struct sh_mmcif_host { > struct mmc_host *mmc; > struct mmc_request *mrq; > @@ -249,6 +256,7 @@ struct sh_mmcif_host { > bool ccs_enable; /* Command Completion Signal support */ > bool clk_ctrl2_enable; > struct mutex thread_lock; > + const struct sh_mmcif_parent_clk *parent_clk; > > /* DMA support */ > struct dma_chan *chan_rx; > @@ -257,8 +265,16 @@ struct sh_mmcif_host { > bool dma_active; > }; > > +static const struct sh_mmcif_parent_clk mmcif_gen2_parent_clk = { > + .max = 97500000, > + .min = 12187500, > + .clkdiv_map = 0x3ff, Shouldn't this come from private data in the CPG clock driver, which supplies the parent clock? Then the mmcif driver will be independent from the parent clock. > +}; > + > static const struct of_device_id mmcif_of_match[] = { > { .compatible = "renesas,sh-mmcif" }, > + { .compatible = "renesas,mmcif-r8a7790", .data = &mmcif_gen2_parent_clk}, > + { .compatible = "renesas,mmcif-r8a7791", .data = &mmcif_gen2_parent_clk}, > { } > }; > MODULE_DEVICE_TABLE(of, mmcif_of_match); > @@ -490,12 +506,51 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk) > > if (!clk) > return; > - if (sup_pclk && clk == host->clk) > + > + if (host->parent_clk) { > + const struct sh_mmcif_parent_clk *pclk = host->parent_clk; > + unsigned int parent_freq, clkdiv, myclk, diff_min, diff; > + int i, j; > + > + /* FIXME */ > + if (pclk->max != pclk->min * (pclk->max / pclk->min)) { > + dev_err(&host->pd->dev, "not assumed parent clk\n"); > + return; > + } Why? > + parent_freq = 0; > + clkdiv = 0; > + diff_min = ~0; > + for (i = pclk->max / pclk->min; i > 0; i--) { > + for (j = fls(pclk->clkdiv_map); j >= 0; j--) { > + if (!((1 << j) & pclk->clkdiv_map)) > + continue; > + > + myclk = ((pclk->min * i) / (1 << (j + 1))); > + diff = (myclk > clk) ? myclk - clk : clk - myclk; > + > + if (diff <= diff_min) { > + parent_freq = pclk->min * i; > + clkdiv = j; > + diff_min = diff; > + } > + } > + } Shouldn't the above be handled by the CPG clock driver, through a clk API? > + dev_dbg(&host->pd->dev, "clk %d/%d (%d, %x)\n", "%u" (all four) > + (parent_freq / (1 << (clkdiv + 1))), clk, > + parent_freq, clkdiv); > + > + clk_set_rate(host->hclk, parent_freq); Note that the last parameter of clk_set_rate() is "unsigned long", while parent_freq is "unsigned int". > + sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, > + CLK_CLEAR & (clkdiv << 16)); > + } else if (sup_pclk && clk == host->clk) { > sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK); > - else > + } else { > sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR & > ((fls(DIV_ROUND_UP(host->clk, > clk) - 1) - 1) << 16)); > + } > > sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE); > } > @@ -982,10 +1037,23 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host) > { > int ret = clk_prepare_enable(host->hclk); > > - if (!ret) { > + if (ret) > + return ret; > + > + if (!host->parent_clk) { > host->clk = clk_get_rate(host->hclk); > host->mmc->f_max = host->clk / 2; > host->mmc->f_min = host->clk / 512; > + } else { > + const struct sh_mmcif_parent_clk *pclk = host->parent_clk; > + > + host->clk = clk_get_rate(host->hclk); clk_get_rate() returns "unsigned long", while "host->clk" is "unsigned int". > + host->mmc->f_max = pclk->max / (1 << ffs(pclk->clkdiv_map)); > + host->mmc->f_min = pclk->min / (1 << fls(pclk->clkdiv_map)); > + > + dev_dbg(&host->pd->dev, "parent clk %d/%d, %d/%d\n", "%u" (all four) > + pclk->max, pclk->min, > + host->mmc->f_max, host->mmc->f_min); > } > > return ret; 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
Hello, On Tuesday 21 April 2015 12:31:21 Geert Uytterhoeven wrote: > On Tue, Apr 21, 2015 at 10:31 AM, Kuninori Morimoto wrote: > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > > > 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. > > > > renesas,mmcif already contain renesas,mmcif-r8a7790/r8a7791 on > > compatible string. So there is no update for binding Document. > > > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Tested-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> > > > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c > > index 5282c5b..6ae836b 100644 > > --- a/drivers/mmc/host/sh_mmcif.c > > +++ b/drivers/mmc/host/sh_mmcif.c > > @@ -224,6 +225,12 @@ enum mmcif_wait_for { > > MMCIF_WAIT_FOR_STOP, > > }; > > > > +struct sh_mmcif_parent_clk { I'm not sure I would call this parent clock. It refers to the frequency of the functional clock provided to the MMCIF, there's no concept of parent there. True, the clock referenced by the MMCIF DT node is an MSTP gate clock, and frequency control is implemented in the MSTP clock parent, but that hardware architecture is internal to the CPG and shouldn't be considered by the MMCIF. > > + unsigned int max; /* if exist: R-Car has MMC CKCR on CPG */ > > + unsigned int min; /* if exist: R-Car has MMC CKCR on CPG */ > > + u32 clkdiv_map; /* see CE_CLK_CTRL::CLKDIV */ > > +}; > > + > > struct sh_mmcif_host { > > struct mmc_host *mmc; > > struct mmc_request *mrq; > > @@ -249,6 +256,7 @@ struct sh_mmcif_host { > > bool ccs_enable; /* Command Completion Signal > > support */ > > bool clk_ctrl2_enable; > > struct mutex thread_lock; > > + const struct sh_mmcif_parent_clk *parent_clk; > > > > /* DMA support */ > > struct dma_chan *chan_rx; > > @@ -257,8 +265,16 @@ struct sh_mmcif_host { > > bool dma_active; > > }; > > > > +static const struct sh_mmcif_parent_clk mmcif_gen2_parent_clk = { > > + .max = 97500000, > > + .min = 12187500, > > + .clkdiv_map = 0x3ff, > > Shouldn't this come from private data in the CPG clock driver, which > supplies the parent clock? Then the mmcif driver will be independent from > the parent clock. The real question is where the constraint comes from. The Gen2 datasheet documents the MMC clocks maximum frequency as 97.5 MHz in the CPG section. However, I have a feeling the constraint doesn't come from the DIV6 which should be able to output higher frequencies, but from the MMCIF, the clock distribution tree, or both. There's also the option of specifying the admissible clock range in DT, either in the CPG node or the MMCIF node. > > +};
Hi Geert Thank you for your review. > > +static const struct sh_mmcif_parent_clk mmcif_gen2_parent_clk = { > > + .max = 97500000, > > + .min = 12187500, > > + .clkdiv_map = 0x3ff, > > Shouldn't this come from private data in the CPG clock driver, which supplies > the parent clock? Then the mmcif driver will be independent from the parent > clock. As Laurent mentioned in other feedback, parent clock (= CFG::MMCxCKCR) is div6, and it can set 1/1 - 1/64 (= 780000000 - 12187500) But, MMC can use 1/8 - 1/64 (= 97500000 - 12187500), we don't know this limit came from. we thought that having limit on DIV6 is not good idea. > > + /* FIXME */ > > + if (pclk->max != pclk->min * (pclk->max / pclk->min)) { > > + dev_err(&host->pd->dev, "not assumed parent clk\n"); > > + return; > > + } > > Why? This time, max/min = 97500000/12187500 = 8. But, we don't know the future. It will be non-assumed result if it was below or similar max = 91406250 min = 12187500 (max = min * 15/2) > > + parent_freq = 0; > > + clkdiv = 0; > > + diff_min = ~0; > > + for (i = pclk->max / pclk->min; i > 0; i--) { > > + for (j = fls(pclk->clkdiv_map); j >= 0; j--) { > > + if (!((1 << j) & pclk->clkdiv_map)) > > + continue; > > + > > + myclk = ((pclk->min * i) / (1 << (j + 1))); > > + diff = (myclk > clk) ? myclk - clk : clk - myclk; > > + > > + if (diff <= diff_min) { > > + parent_freq = pclk->min * i; > > + clkdiv = j; > > + diff_min = diff; > > + } > > + } > > + } > > Shouldn't the above be handled by the CPG clock driver, through a clk API? I don't think so. it calculates best of parent clock (= from CPG) and divider (= from MMC) Why CPG driver need to care about MMC's divider ?? > > + dev_dbg(&host->pd->dev, "clk %d/%d (%d, %x)\n", > > "%u" (all four) (snip) > > + clk_set_rate(host->hclk, parent_freq); > > Note that the last parameter of clk_set_rate() is "unsigned long", while > parent_freq is "unsigned int". (snip) > > + host->clk = clk_get_rate(host->hclk); > > clk_get_rate() returns "unsigned long", while "host->clk" is "unsigned int". (snip) > > + dev_dbg(&host->pd->dev, "parent clk %d/%d, %d/%d\n", > > "%u" (all four) Thanks ! I will fixup these in v2 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
Hi Laurent > > > +struct sh_mmcif_parent_clk { > > I'm not sure I would call this parent clock. It refers to the frequency of the > functional clock provided to the MMCIF, there's no concept of parent there. > True, the clock referenced by the MMCIF DT node is an MSTP gate clock, and > frequency control is implemented in the MSTP clock parent, but that hardware > architecture is internal to the CPG and shouldn't be considered by the MMCIF. I couldn't understand about last 2 lines. Do you mean I need to add these method in CPG ? But, this calculates best clock of parent clock (= from CPG) and divider (= on MMC) Why CPG need to care about MMC's divider, and how to set it from CPG ?? > > Shouldn't this come from private data in the CPG clock driver, which > > supplies the parent clock? Then the mmcif driver will be independent from > > the parent clock. > > The real question is where the constraint comes from. The Gen2 datasheet > documents the MMC clocks maximum frequency as 97.5 MHz in the CPG section. > However, I have a feeling the constraint doesn't come from the DIV6 which > should be able to output higher frequencies, but from the MMCIF, the clock > distribution tree, or both. > > There's also the option of specifying the admissible clock range in DT, either > in the CPG node or the MMCIF node. It needs not only max/min range, but also divider's limit. I don't know how to do it. but... 1) add admissible max/min range on CPG node add admissible divider range on MMC node -> Does this max/min range really from CPG ? but, we can reuse this on other DIV6 2) add admissible max/min range on MMC node add admissible divider range on MMC node -> reasonable, but we can't reuse it on other DIV6 what is difference between 2) and 3) ? 3) add admissible max/min range on MMC driver add admissible divider range on MMC driver -> This is the reason why we have SoC specific compatible name ? These limit came from SoC. In my opinion, I can accept 1) or 3). 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
Hi Morimoto-san, On Wed, Apr 22, 2015 at 3:04 AM, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: >> > +static const struct sh_mmcif_parent_clk mmcif_gen2_parent_clk = { >> > + .max = 97500000, >> > + .min = 12187500, >> > + .clkdiv_map = 0x3ff, >> >> Shouldn't this come from private data in the CPG clock driver, which supplies >> the parent clock? Then the mmcif driver will be independent from the parent >> clock. > > As Laurent mentioned in other feedback, parent clock (= CFG::MMCxCKCR) > is div6, and it can set 1/1 - 1/64 (= 780000000 - 12187500) > But, MMC can use 1/8 - 1/64 (= 97500000 - 12187500), we don't know this limit > came from. we thought that having limit on DIV6 is not good idea. So the upper limit is a limitation of the MMCIF hardware, while the lower limit is a limitation of the CPG. Then the upper limit should be either in the driver (as you did, using different compatible values), or it can be described in DT. (cfr. "git grep max.*freq -- arch/arm/boot/dts/"). Personally, I'm leaning towards the latter. IMHO the lower limit doesn't belong here. >> > + /* FIXME */ >> > + if (pclk->max != pclk->min * (pclk->max / pclk->min)) { >> > + dev_err(&host->pd->dev, "not assumed parent clk\n"); >> > + return; >> > + } >> >> Why? > > This time, max/min = 97500000/12187500 = 8. > But, we don't know the future. > It will be non-assumed result if it was below or similar > max = 91406250 > min = 12187500 (max = min * 15/2) You can still calculate a divisor if max is not an integer multiple of min, can't you? And currently the check above cannot trigger, as you have hardcoded values in the driver source that don't trigger it. >> > + parent_freq = 0; >> > + clkdiv = 0; >> > + diff_min = ~0; >> > + for (i = pclk->max / pclk->min; i > 0; i--) { >> > + for (j = fls(pclk->clkdiv_map); j >= 0; j--) { >> > + if (!((1 << j) & pclk->clkdiv_map)) >> > + continue; >> > + >> > + myclk = ((pclk->min * i) / (1 << (j + 1))); >> > + diff = (myclk > clk) ? myclk - clk : clk - myclk; >> > + >> > + if (diff <= diff_min) { >> > + parent_freq = pclk->min * i; >> > + clkdiv = j; >> > + diff_min = diff; >> > + } >> > + } >> > + } >> >> Shouldn't the above be handled by the CPG clock driver, through a clk API? > > I don't think so. > it calculates best of parent clock (= from CPG) and divider (= from MMC) > Why CPG driver need to care about MMC's divider ?? The MMCIF driver shouldn't need to be aware of the possible values supported by the CPG driver. Can't you loop over all MMC dividers, and 1. calculate the needed parent clock rate for that MMC divider, 2. use clk_set_rate_range()[*] to find a (close) parent clock rate, 3. calculate the effective clock rate based on MMC divider and parent clock rate. and use the best effective clock rate found? [*] It's a pity the clk API doesn't seem to have a function to query or check clock rates without actually setting them. Or am I missing something? 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
On 22 April 2015 at 09:49, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Morimoto-san, > > On Wed, Apr 22, 2015 at 3:04 AM, Kuninori Morimoto > <kuninori.morimoto.gx@renesas.com> wrote: >>> > +static const struct sh_mmcif_parent_clk mmcif_gen2_parent_clk = { >>> > + .max = 97500000, >>> > + .min = 12187500, >>> > + .clkdiv_map = 0x3ff, >>> >>> Shouldn't this come from private data in the CPG clock driver, which supplies >>> the parent clock? Then the mmcif driver will be independent from the parent >>> clock. >> >> As Laurent mentioned in other feedback, parent clock (= CFG::MMCxCKCR) >> is div6, and it can set 1/1 - 1/64 (= 780000000 - 12187500) >> But, MMC can use 1/8 - 1/64 (= 97500000 - 12187500), we don't know this limit >> came from. we thought that having limit on DIV6 is not good idea. > > So the upper limit is a limitation of the MMCIF hardware, while the lower > limit is a limitation of the CPG. > > Then the upper limit should be either in the driver (as you did, using > different compatible values), or it can be described in DT. > (cfr. "git grep max.*freq -- arch/arm/boot/dts/"). > Personally, I'm leaning towards the latter. > > IMHO the lower limit doesn't belong here. > >>> > + /* FIXME */ >>> > + if (pclk->max != pclk->min * (pclk->max / pclk->min)) { >>> > + dev_err(&host->pd->dev, "not assumed parent clk\n"); >>> > + return; >>> > + } >>> >>> Why? >> >> This time, max/min = 97500000/12187500 = 8. >> But, we don't know the future. >> It will be non-assumed result if it was below or similar >> max = 91406250 >> min = 12187500 (max = min * 15/2) > > You can still calculate a divisor if max is not an integer multiple of min, > can't you? > > And currently the check above cannot trigger, as you have hardcoded values > in the driver source that don't trigger it. > >>> > + parent_freq = 0; >>> > + clkdiv = 0; >>> > + diff_min = ~0; >>> > + for (i = pclk->max / pclk->min; i > 0; i--) { >>> > + for (j = fls(pclk->clkdiv_map); j >= 0; j--) { >>> > + if (!((1 << j) & pclk->clkdiv_map)) >>> > + continue; >>> > + >>> > + myclk = ((pclk->min * i) / (1 << (j + 1))); >>> > + diff = (myclk > clk) ? myclk - clk : clk - myclk; >>> > + >>> > + if (diff <= diff_min) { >>> > + parent_freq = pclk->min * i; >>> > + clkdiv = j; >>> > + diff_min = diff; >>> > + } >>> > + } >>> > + } >>> >>> Shouldn't the above be handled by the CPG clock driver, through a clk API? >> >> I don't think so. >> it calculates best of parent clock (= from CPG) and divider (= from MMC) >> Why CPG driver need to care about MMC's divider ?? > > The MMCIF driver shouldn't need to be aware of the possible values supported > by the CPG driver. > > Can't you loop over all MMC dividers, and > 1. calculate the needed parent clock rate for that MMC divider, > 2. use clk_set_rate_range()[*] to find a (close) parent clock rate, > 3. calculate the effective clock rate based on MMC divider and parent > clock rate. > and use the best effective clock rate found? > > [*] It's a pity the clk API doesn't seem to have a function to query or check > clock rates without actually setting them. Or am I missing something? What about clk_round_rate(), can't that be used? Kind regards Uffe -- 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
On Wed, Apr 22, 2015 at 10:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> The MMCIF driver shouldn't need to be aware of the possible values supported >> by the CPG driver. >> >> Can't you loop over all MMC dividers, and >> 1. calculate the needed parent clock rate for that MMC divider, >> 2. use clk_set_rate_range()[*] to find a (close) parent clock rate, >> 3. calculate the effective clock rate based on MMC divider and parent >> clock rate. >> and use the best effective clock rate found? >> >> [*] It's a pity the clk API doesn't seem to have a function to query or check >> clock rates without actually setting them. Or am I missing something? > > What about clk_round_rate(), can't that be used? Thanks, I knew there had to be something, but couldn't find it at first sight. 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 > >> Can't you loop over all MMC dividers, and > >> 1. calculate the needed parent clock rate for that MMC divider, > >> 2. use clk_set_rate_range()[*] to find a (close) parent clock rate, > >> 3. calculate the effective clock rate based on MMC divider and parent > >> clock rate. > >> and use the best effective clock rate found? (snip) > So the upper limit is a limitation of the MMCIF hardware, while the lower > limit is a limitation of the CPG. > > Then the upper limit should be either in the driver (as you did, using > different compatible values), or it can be described in DT. > (cfr. "git grep max.*freq -- arch/arm/boot/dts/"). > Personally, I'm leaning towards the latter. > > IMHO the lower limit doesn't belong here. I think I understood your opinion. I will try this 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
Hi Morimoto-san, Sorry for the late reply. On Wednesday 22 April 2015 01:05:59 Kuninori Morimoto wrote: > Hi Laurent > > >>> +struct sh_mmcif_parent_clk { > > > > I'm not sure I would call this parent clock. It refers to the frequency of > > the functional clock provided to the MMCIF, there's no concept of parent > > there. True, the clock referenced by the MMCIF DT node is an MSTP gate > > clock, and frequency control is implemented in the MSTP clock parent, but > > that hardware architecture is internal to the CPG and shouldn't be > > considered by the MMCIF. > > I couldn't understand about last 2 lines. > Do you mean I need to add these method in CPG ? > But, this calculates best clock of parent clock (= from CPG) and divider (= > on MMC) Why CPG need to care about MMC's divider, > and how to set it from CPG ?? No, that's not what I meant. The CPG should certainly not care about any MMCIF internal divider. My point was about the name of the structure. However, now that you mention the internal MMCIF clock divider, the word "parent" indeed makes sense. Please ignore my comment. > >> Shouldn't this come from private data in the CPG clock driver, which > >> supplies the parent clock? Then the mmcif driver will be independent > >> from the parent clock. > > > > The real question is where the constraint comes from. The Gen2 datasheet > > documents the MMC clocks maximum frequency as 97.5 MHz in the CPG section. > > However, I have a feeling the constraint doesn't come from the DIV6 which > > should be able to output higher frequencies, but from the MMCIF, the clock > > distribution tree, or both. > > > > There's also the option of specifying the admissible clock range in DT, > > either in the CPG node or the MMCIF node. > > It needs not only max/min range, but also divider's limit. > I don't know how to do it. but... > > 1) add admissible max/min range on CPG node > add admissible divider range on MMC node > -> Does this max/min range really from CPG ? > but, we can reuse this on other DIV6 > > 2) add admissible max/min range on MMC node > add admissible divider range on MMC node > -> reasonable, but we can't reuse it on other DIV6 > what is difference between 2) and 3) ? > > 3) add admissible max/min range on MMC driver > add admissible divider range on MMC driver > -> This is the reason why we have SoC specific compatible name ? > These limit came from SoC. > > In my opinion, I can accept 1) or 3). Once again it really depends where the constraints come from. If the MMC input clock frequency range constraint is dictated by the MMCIF IP core only, options 2 and 3 are equivalent and just differ by whether they specify the range explicitly in DT (option 2) or implicitly through the compatible name (option 3). If the range constraint comes from the CPG, option 1 is the way to go, (although the admissible divider range could also be speficied in the MMC driver as in option 3). If the range constraint is a property of the integration of the MMCIF in the SoC then I think option 2 is the best. The per-SoC compatible strings can be used to express differences between IP core versions found in different SoCs, but they shouldn't be used to express integration properties. To clarify this with an example, it's fine for the DU driver to use the per- SoC compatible string to know how many display channels the DU contains, but it can't be used to hardcode IRQ numbers in the driver, even if we know which IRQ the DU is hooked to for each SoC. The former is a property of the DU version, the latter is a property of the integration and should be expressed in DT.
Hi Laurent > Hi Morimoto-san, > > Sorry for the late reply. Sorry for my late response too. > > > I'm not sure I would call this parent clock. It refers to the frequency of > > > the functional clock provided to the MMCIF, there's no concept of parent > > > there. True, the clock referenced by the MMCIF DT node is an MSTP gate > > > clock, and frequency control is implemented in the MSTP clock parent, but > > > that hardware architecture is internal to the CPG and shouldn't be > > > considered by the MMCIF. > > > > I couldn't understand about last 2 lines. > > Do you mean I need to add these method in CPG ? > > But, this calculates best clock of parent clock (= from CPG) and divider (= > > on MMC) Why CPG need to care about MMC's divider, > > and how to set it from CPG ?? > > No, that's not what I meant. The CPG should certainly not care about any MMCIF > internal divider. > > My point was about the name of the structure. However, now that you mention > the internal MMCIF clock divider, the word "parent" indeed makes sense. Please > ignore my comment. Thank you for your feedback, and thank you for your understanding. 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 5282c5b..6ae836b 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,12 @@ enum mmcif_wait_for { MMCIF_WAIT_FOR_STOP, }; +struct sh_mmcif_parent_clk { + unsigned int max; /* if exist: R-Car has MMC CKCR on CPG */ + unsigned int min; /* if exist: R-Car has MMC CKCR on CPG */ + u32 clkdiv_map; /* see CE_CLK_CTRL::CLKDIV */ +}; + struct sh_mmcif_host { struct mmc_host *mmc; struct mmc_request *mrq; @@ -249,6 +256,7 @@ struct sh_mmcif_host { bool ccs_enable; /* Command Completion Signal support */ bool clk_ctrl2_enable; struct mutex thread_lock; + const struct sh_mmcif_parent_clk *parent_clk; /* DMA support */ struct dma_chan *chan_rx; @@ -257,8 +265,16 @@ struct sh_mmcif_host { bool dma_active; }; +static const struct sh_mmcif_parent_clk mmcif_gen2_parent_clk = { + .max = 97500000, + .min = 12187500, + .clkdiv_map = 0x3ff, +}; + static const struct of_device_id mmcif_of_match[] = { { .compatible = "renesas,sh-mmcif" }, + { .compatible = "renesas,mmcif-r8a7790", .data = &mmcif_gen2_parent_clk}, + { .compatible = "renesas,mmcif-r8a7791", .data = &mmcif_gen2_parent_clk}, { } }; MODULE_DEVICE_TABLE(of, mmcif_of_match); @@ -490,12 +506,51 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk) if (!clk) return; - if (sup_pclk && clk == host->clk) + + if (host->parent_clk) { + const struct sh_mmcif_parent_clk *pclk = host->parent_clk; + unsigned int parent_freq, clkdiv, myclk, diff_min, diff; + int i, j; + + /* FIXME */ + if (pclk->max != pclk->min * (pclk->max / pclk->min)) { + dev_err(&host->pd->dev, "not assumed parent clk\n"); + return; + } + + parent_freq = 0; + clkdiv = 0; + diff_min = ~0; + for (i = pclk->max / pclk->min; i > 0; i--) { + for (j = fls(pclk->clkdiv_map); j >= 0; j--) { + if (!((1 << j) & pclk->clkdiv_map)) + continue; + + myclk = ((pclk->min * i) / (1 << (j + 1))); + diff = (myclk > clk) ? myclk - clk : clk - myclk; + + if (diff <= diff_min) { + parent_freq = pclk->min * i; + clkdiv = j; + diff_min = diff; + } + } + } + + dev_dbg(&host->pd->dev, "clk %d/%d (%d, %x)\n", + (parent_freq / (1 << (clkdiv + 1))), clk, + parent_freq, clkdiv); + + clk_set_rate(host->hclk, parent_freq); + sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, + CLK_CLEAR & (clkdiv << 16)); + } else if (sup_pclk && clk == host->clk) { sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK); - else + } else { sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR & ((fls(DIV_ROUND_UP(host->clk, clk) - 1) - 1) << 16)); + } sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE); } @@ -982,10 +1037,23 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host) { int ret = clk_prepare_enable(host->hclk); - if (!ret) { + if (ret) + return ret; + + if (!host->parent_clk) { host->clk = clk_get_rate(host->hclk); host->mmc->f_max = host->clk / 2; host->mmc->f_min = host->clk / 512; + } else { + const struct sh_mmcif_parent_clk *pclk = host->parent_clk; + + host->clk = clk_get_rate(host->hclk); + host->mmc->f_max = pclk->max / (1 << ffs(pclk->clkdiv_map)); + host->mmc->f_min = pclk->min / (1 << fls(pclk->clkdiv_map)); + + dev_dbg(&host->pd->dev, "parent clk %d/%d, %d/%d\n", + pclk->max, pclk->min, + host->mmc->f_max, host->mmc->f_min); } return ret; @@ -1389,6 +1457,7 @@ static int sh_mmcif_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct sh_mmcif_plat_data *pd = dev->platform_data; struct resource *res; + const struct of_device_id *of_id = of_match_device(mmcif_of_match, dev); void __iomem *reg; const char *name; @@ -1421,6 +1490,10 @@ static int sh_mmcif_probe(struct platform_device *pdev) host->pd = pdev; + host->parent_clk = NULL; + if (of_id) + host->parent_clk = of_id->data; + spin_lock_init(&host->lock); mmc->ops = &sh_mmcif_ops;