On 22 April 2015 at 09:49, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> Hi Morimoto-san,
>
> On Wed, Apr 22, 2015 at 3:04 AM, Kuninori Morimoto
> <kuninori.morimoto.gx@xxxxxxxxxxx> 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-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
