Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Quoting Tomasz Figa (2014-07-31 15:17:29)
> Humberto,
>
> [dropping few addresses from Cc as this topic is rather irrelevant for
> them and adding Mike and Sylwester]
>
> On 31.07.2014 23:19, Humberto Naves wrote:
> > Hi,
> >
> > On Thu, Jul 31, 2014 at 5:19 PM, Tomasz Figa wrote:
> >>
> >> I'm not sure I get the idea of the field you're suggesting. If I
> >> understand correctly, your intention would be to provide a default
> >> frequency if there is no table provided. I don't think there is a need
> >> for it, because current code can read back current settings from
> >> registers and calculate current rate.
> >>
> > I think I was not clear enough. I am not trying to provide a default
> > frequency for the clock, but I do want to specify the base frequency
> > on which the rate table was based upon. Let me give you an example
> > that will hopefully clarify the matter. Suppose I want to register my
> > PLL, such as in the 5410. The *current* solution would be like this:
> >
> > static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
> >/* PLL_35XX_RATE(rate, m, p, s, k) */
> >PLL_35XX_RATE(86400, 288, 4, 1),
> >{ },
> > };
> > static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
> >[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll",
> > IPLL_LOCK,
> >IPLL_CON0, NULL),
> > };
> >
> > And in the driver initialization function, I would add the rate table
> > if the input clock source matches what I expected, in this case 24Mhz:
> >
> > if (_get_rate("fin_pll") == 24 * MHZ) {
> > exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
> > }
> >
> > An alternative approach would be as follows, we add a new field (say
> > "base_rate") to the structure samsung_pll_clock, and to the macro PLL,
> > and describe the pll table in a simpler way, such as follows:
> >
> > static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
> >[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll",
> > IPLL_LOCK,
> >IPLL_CON0, _24mhz_tbl, 24 * MHZ),
> > };
> >
> > and in the _samsung_clk_register_pll function, all the validation
> > would be performed. Here I am talking about checking if the parent
> > rate is what is specified on the samsung_pll_clock structure, and if
> > so, to check if the rates on the rate table actually match what the
> > coefficients are telling.
>
> What if there are multiple sets of tables for different "base rates"?
> Certain PLLs can run with several reference clock frequencies, e.g. VPLL
> rates are often specified for 24 MHz and 27 MHz, depending on setting of
> mout_vpllsrc.
>
> >
> >> As for the validation itself, one more thing that needs to be considered
> >> is that the rate table must be sorted.
> >
> > The _samsung_clk_register_pll could do that in theory.
> >
> >>
> >> We once decided to rely on the fact that tables in SoC drivers have
> >> rates explicitly specified and are correctly sorted, but now I'm
> >> inclined to reconsider this, based on the fact that those rates often
> >> are already incorrectly calculated in vendor code or even datasheets,
> >> which are main information sources for patch authors.
> >>
> >> Before mainlining PLL drivers (which was quite some time ago), we had a
> >> bit different implementation in our internal tree, which did not use
> >> explicitly specified rates at all (they could have been considered just
> >> comments to improve table readability) and the _register_pll() function
> >> simply calculated rates for all entries creating new table for internal
> >> use of the PLL driver that was in addition explicitly sorted to make
> >> sure that the order is correct. This kind of implementation is what I
> >> would lean toward today.
> >
> > I would strongly object to such as solution. I think that in the
> > table, the frequency *must* be specified. As you said previously, the
> > coefficients should be carefully chosen. We cannot know for sure that
> > the same coefficients that work for a base frequency of 24 Mhz will
> > also work for a different base frequency. So the driver cannot simply
> > compute the frequency from the coefficients, and it must also check
> > that the input rate is correct. This is another reason why I want to
> > add the base_frequency field to that structure.
>
> Sorry, I don't understand your concern. Currently it's SoC driver's duty
> to check which rate table to use depending on input clock rate. If input
> rate matches any table the driver has, it assigns the table pointer.
> Otherwise no table is used and the PLL is working in read-only mode.
>
> If we leave this as is, then PLL driver will have enough information to
> calculate PLL rate, because it can retrieve input clock rate by calling
> clk_get_rate() on its parent clock. No need to specify output_rate in
> rate table, as it is redundant. However...
>
> >
> > I believe that the the _samsung_clk_register_pll must
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Humberto,
[dropping few addresses from Cc as this topic is rather irrelevant for
them and adding Mike and Sylwester]
On 31.07.2014 23:19, Humberto Naves wrote:
> Hi,
>
> On Thu, Jul 31, 2014 at 5:19 PM, Tomasz Figa wrote:
>>
>> I'm not sure I get the idea of the field you're suggesting. If I
>> understand correctly, your intention would be to provide a default
>> frequency if there is no table provided. I don't think there is a need
>> for it, because current code can read back current settings from
>> registers and calculate current rate.
>>
> I think I was not clear enough. I am not trying to provide a default
> frequency for the clock, but I do want to specify the base frequency
> on which the rate table was based upon. Let me give you an example
> that will hopefully clarify the matter. Suppose I want to register my
> PLL, such as in the 5410. The *current* solution would be like this:
>
> static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
>/* PLL_35XX_RATE(rate, m, p, s, k) */
>PLL_35XX_RATE(86400, 288, 4, 1),
>{ },
> };
> static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
>[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll",
> IPLL_LOCK,
>IPLL_CON0, NULL),
> };
>
> And in the driver initialization function, I would add the rate table
> if the input clock source matches what I expected, in this case 24Mhz:
>
> if (_get_rate("fin_pll") == 24 * MHZ) {
> exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
> }
>
> An alternative approach would be as follows, we add a new field (say
> "base_rate") to the structure samsung_pll_clock, and to the macro PLL,
> and describe the pll table in a simpler way, such as follows:
>
> static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
>[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll",
> IPLL_LOCK,
>IPLL_CON0, _24mhz_tbl, 24 * MHZ),
> };
>
> and in the _samsung_clk_register_pll function, all the validation
> would be performed. Here I am talking about checking if the parent
> rate is what is specified on the samsung_pll_clock structure, and if
> so, to check if the rates on the rate table actually match what the
> coefficients are telling.
What if there are multiple sets of tables for different "base rates"?
Certain PLLs can run with several reference clock frequencies, e.g. VPLL
rates are often specified for 24 MHz and 27 MHz, depending on setting of
mout_vpllsrc.
>
>> As for the validation itself, one more thing that needs to be considered
>> is that the rate table must be sorted.
>
> The _samsung_clk_register_pll could do that in theory.
>
>>
>> We once decided to rely on the fact that tables in SoC drivers have
>> rates explicitly specified and are correctly sorted, but now I'm
>> inclined to reconsider this, based on the fact that those rates often
>> are already incorrectly calculated in vendor code or even datasheets,
>> which are main information sources for patch authors.
>>
>> Before mainlining PLL drivers (which was quite some time ago), we had a
>> bit different implementation in our internal tree, which did not use
>> explicitly specified rates at all (they could have been considered just
>> comments to improve table readability) and the _register_pll() function
>> simply calculated rates for all entries creating new table for internal
>> use of the PLL driver that was in addition explicitly sorted to make
>> sure that the order is correct. This kind of implementation is what I
>> would lean toward today.
>
> I would strongly object to such as solution. I think that in the
> table, the frequency *must* be specified. As you said previously, the
> coefficients should be carefully chosen. We cannot know for sure that
> the same coefficients that work for a base frequency of 24 Mhz will
> also work for a different base frequency. So the driver cannot simply
> compute the frequency from the coefficients, and it must also check
> that the input rate is correct. This is another reason why I want to
> add the base_frequency field to that structure.
Sorry, I don't understand your concern. Currently it's SoC driver's duty
to check which rate table to use depending on input clock rate. If input
rate matches any table the driver has, it assigns the table pointer.
Otherwise no table is used and the PLL is working in read-only mode.
If we leave this as is, then PLL driver will have enough information to
calculate PLL rate, because it can retrieve input clock rate by calling
clk_get_rate() on its parent clock. No need to specify output_rate in
rate table, as it is redundant. However...
>
> I believe that the the _samsung_clk_register_pll must double-check if
> the frequencies match what the formula tells, and must drop the
> entries (or the whole frequency table) that are faulty. And then
> finally, it should sort the entries in descending order.
Based on your proposal, another idea came
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Hi,
On Thu, Jul 31, 2014 at 5:19 PM, Tomasz Figa wrote:
>
> I'm not sure I get the idea of the field you're suggesting. If I
> understand correctly, your intention would be to provide a default
> frequency if there is no table provided. I don't think there is a need
> for it, because current code can read back current settings from
> registers and calculate current rate.
>
I think I was not clear enough. I am not trying to provide a default
frequency for the clock, but I do want to specify the base frequency
on which the rate table was based upon. Let me give you an example
that will hopefully clarify the matter. Suppose I want to register my
PLL, such as in the 5410. The *current* solution would be like this:
static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
/* PLL_35XX_RATE(rate, m, p, s, k) */
PLL_35XX_RATE(86400, 288, 4, 1),
{ },
};
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll", IPLL_LOCK,
IPLL_CON0, NULL),
};
And in the driver initialization function, I would add the rate table
if the input clock source matches what I expected, in this case 24Mhz:
if (_get_rate("fin_pll") == 24 * MHZ) {
exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
}
An alternative approach would be as follows, we add a new field (say
"base_rate") to the structure samsung_pll_clock, and to the macro PLL,
and describe the pll table in a simpler way, such as follows:
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll", IPLL_LOCK,
IPLL_CON0, _24mhz_tbl, 24 * MHZ),
};
and in the _samsung_clk_register_pll function, all the validation
would be performed. Here I am talking about checking if the parent
rate is what is specified on the samsung_pll_clock structure, and if
so, to check if the rates on the rate table actually match what the
coefficients are telling.
> As for the validation itself, one more thing that needs to be considered
> is that the rate table must be sorted.
The _samsung_clk_register_pll could do that in theory.
>
> We once decided to rely on the fact that tables in SoC drivers have
> rates explicitly specified and are correctly sorted, but now I'm
> inclined to reconsider this, based on the fact that those rates often
> are already incorrectly calculated in vendor code or even datasheets,
> which are main information sources for patch authors.
>
> Before mainlining PLL drivers (which was quite some time ago), we had a
> bit different implementation in our internal tree, which did not use
> explicitly specified rates at all (they could have been considered just
> comments to improve table readability) and the _register_pll() function
> simply calculated rates for all entries creating new table for internal
> use of the PLL driver that was in addition explicitly sorted to make
> sure that the order is correct. This kind of implementation is what I
> would lean toward today.
I would strongly object to such as solution. I think that in the
table, the frequency *must* be specified. As you said previously, the
coefficients should be carefully chosen. We cannot know for sure that
the same coefficients that work for a base frequency of 24 Mhz will
also work for a different base frequency. So the driver cannot simply
compute the frequency from the coefficients, and it must also check
that the input rate is correct. This is another reason why I want to
add the base_frequency field to that structure.
I believe that the the _samsung_clk_register_pll must double-check if
the frequencies match what the formula tells, and must drop the
entries (or the whole frequency table) that are faulty. And then
finally, it should sort the entries in descending order.
I hope I made myself clear now.
Best regards,
Humberto
>
> Best regards,
> Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
On 31.07.2014 15:37, Humberto Naves wrote: > Hi Tomasz, > > I remember checking these rates on my calculator. You might notice the > odd frequency of 45158401Hz (no pun intended) in the EPLL clock. This > particular clock frequency was giving me a big headache in a previous > project, since it was wrongly listed as 45158400. At first it seems > innocuous, but whenever I would ask for one of the child clocks to > change the rate, the driver would miscalculate the correct frequencies > and errors would propagate up and down the clock tree. > > Anyway, I would double check these tables. And if you want, I can > write a separate patch for the rate table validation. I presume that > you would like to add a new field, such as default_base_freq, to the > structure samsung_pll_clock, and if that field is non-zero, you > perform the validation of the table in _samsung_clk_register_pll? I'm not sure I get the idea of the field you're suggesting. If I understand correctly, your intention would be to provide a default frequency if there is no table provided. I don't think there is a need for it, because current code can read back current settings from registers and calculate current rate. As for the validation itself, one more thing that needs to be considered is that the rate table must be sorted. We once decided to rely on the fact that tables in SoC drivers have rates explicitly specified and are correctly sorted, but now I'm inclined to reconsider this, based on the fact that those rates often are already incorrectly calculated in vendor code or even datasheets, which are main information sources for patch authors. Before mainlining PLL drivers (which was quite some time ago), we had a bit different implementation in our internal tree, which did not use explicitly specified rates at all (they could have been considered just comments to improve table readability) and the _register_pll() function simply calculated rates for all entries creating new table for internal use of the PLL driver that was in addition explicitly sorted to make sure that the order is correct. This kind of implementation is what I would lean toward today. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Hi Tomasz,
I remember checking these rates on my calculator. You might notice the
odd frequency of 45158401Hz (no pun intended) in the EPLL clock. This
particular clock frequency was giving me a big headache in a previous
project, since it was wrongly listed as 45158400. At first it seems
innocuous, but whenever I would ask for one of the child clocks to
change the rate, the driver would miscalculate the correct frequencies
and errors would propagate up and down the clock tree.
Anyway, I would double check these tables. And if you want, I can
write a separate patch for the rate table validation. I presume that
you would like to add a new field, such as default_base_freq, to the
structure samsung_pll_clock, and if that field is non-zero, you
perform the validation of the table in _samsung_clk_register_pll?
Best,
Humberto
On Thu, Jul 31, 2014 at 3:07 PM, Tomasz Figa wrote:
> Hi Humberto,
>
> You can find my comments inline.
>
> On 31.07.2014 13:22, Humberto Silva Naves wrote:
>> Added the remaining PLL clocks, and also added the configuration
>> tables with the PLL coefficients for the supported frequencies.
>> These frequency tables are only installed when a 24MHz clock is
>> supplied as the input clock source. To reflect these changes, new
>> constants were added to the dt-bindings file.
>
> [snip]
>
>> +static struct samsung_pll_rate_table apll_24mhz_tbl[] __initdata = {
>> + /* sorted in descending order */
>> + /* PLL_35XX_RATE(rate, m, p, s) */
>> + PLL_35XX_RATE(21, 175, 2, 0),
>> + PLL_35XX_RATE(20, 250, 3, 0),
>> + PLL_35XX_RATE(19, 475, 6, 0),
>> + PLL_35XX_RATE(18, 225, 3, 0),
>> + PLL_35XX_RATE(17, 425, 6, 0),
>> + PLL_35XX_RATE(16, 200, 3, 0),
>> + PLL_35XX_RATE(15, 250, 4, 0),
>> + PLL_35XX_RATE(14, 175, 3, 0),
>> + PLL_35XX_RATE(13, 325, 6, 0),
>> + PLL_35XX_RATE(12, 100, 2, 0),
>> + PLL_35XX_RATE(11, 275, 3, 1),
>> + PLL_35XX_RATE(10, 250, 3, 1),
>> + PLL_35XX_RATE(9, 150, 2, 1),
>> + PLL_35XX_RATE(8, 200, 3, 1),
>> + PLL_35XX_RATE(7, 175, 3, 1),
>> + PLL_35XX_RATE(6, 100, 2, 1),
>> + PLL_35XX_RATE(5, 250, 3, 2),
>> + PLL_35XX_RATE(4, 200, 3, 2),
>> + PLL_35XX_RATE(3, 100, 2, 2),
>> + PLL_35XX_RATE(2, 200, 3, 3),
>
> nit: The numbers could be aligned to the right using spaces (see exynos4.c).
>
>> + { },
>> +};
>> +
>> +static struct samsung_pll_rate_table cpll_24mhz_tbl[] __initdata = {
>> + /* sorted in descending order */
>> + /* PLL_35XX_RATE(rate, m, p, s) */
>> + PLL_35XX_RATE(66600, 222, 4, 1),
>> + PLL_35XX_RATE(64000, 160, 3, 1),
>> + PLL_35XX_RATE(32000, 160, 3, 2),
>> + { },
>> +};
>> +
>> +static struct samsung_pll_rate_table dpll_24mhz_tbl[] __initdata = {
>> + /* sorted in descending order */
>> + /* PLL_35XX_RATE(rate, m, p, s) */
>> + PLL_35XX_RATE(6, 200, 4, 1),
>> + { },
>> +};
>> +
>> +static struct samsung_pll_rate_table epll_24mhz_tbl[] __initdata = {
>> + /* sorted in descending order */
>> + /* PLL_36XX_RATE(rate, m, p, s, k) */
>> + PLL_36XX_RATE(6, 100, 2, 1, 0),
>> + PLL_36XX_RATE(4, 200, 3, 2, 0),
>> + PLL_36XX_RATE(2, 200, 3, 3, 0),
>> + PLL_36XX_RATE(180633600, 301, 5, 3, -3670),
>> + PLL_36XX_RATE( 67737600, 452, 5, 5, -27263),
>> + PLL_36XX_RATE( 49152000, 197, 3, 5, -25690),
>> + PLL_36XX_RATE( 45158401, 181, 3, 5, -24012),
>
> Have you ensured that the rates specified match the rates calculated
> using PLL equation? You can find how it is calculated in recalc_rate
> callback of this particular PLL type in clk-pll.c.
>
> As a side note, the PLL registration code should be made a bit more
> robust and just calculate the rates itself and printing warnings if they
> don't match the entered ones. I definitely need more hours in a day, so
> much to do. ;)
>
>> + { },
>> +};
>> +
>> +static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
>> + /* sorted in descending order */
>> + /* PLL_35XX_RATE(rate, m, p, s, k) */
>> + PLL_35XX_RATE(86400, 288, 4, 1),
>> + PLL_35XX_RATE(66600, 222, 4, 1),
>> + PLL_35XX_RATE(43200, 288, 4, 2),
>> + { },
>> +};
>> +
>> +static struct samsung_pll_rate_table kpll_24mhz_tbl[] __initdata = {
>> + /* sorted in descending order */
>> + /* PLL_35XX_RATE(rate, m, p, s) */
>> + PLL_35XX_RATE(15, 250, 4, 0),
>> + PLL_35XX_RATE(14, 175, 3, 0),
>> + PLL_35XX_RATE(13, 325, 6, 0),
>> + PLL_35XX_RATE(12, 100, 2, 0),
>> + PLL_35XX_RATE(11, 275, 3, 1),
>> + PLL_35XX_RATE(10, 250, 3, 1),
>> + PLL_35XX_RATE(9, 150, 2, 1),
>> + PLL_35XX_RATE(8, 200, 3, 1),
>> + PLL_35XX_RATE(7, 175, 3, 1),
>> +
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Hi Humberto,
You can find my comments inline.
On 31.07.2014 13:22, Humberto Silva Naves wrote:
> Added the remaining PLL clocks, and also added the configuration
> tables with the PLL coefficients for the supported frequencies.
> These frequency tables are only installed when a 24MHz clock is
> supplied as the input clock source. To reflect these changes, new
> constants were added to the dt-bindings file.
[snip]
> +static struct samsung_pll_rate_table apll_24mhz_tbl[] __initdata = {
> + /* sorted in descending order */
> + /* PLL_35XX_RATE(rate, m, p, s) */
> + PLL_35XX_RATE(21, 175, 2, 0),
> + PLL_35XX_RATE(20, 250, 3, 0),
> + PLL_35XX_RATE(19, 475, 6, 0),
> + PLL_35XX_RATE(18, 225, 3, 0),
> + PLL_35XX_RATE(17, 425, 6, 0),
> + PLL_35XX_RATE(16, 200, 3, 0),
> + PLL_35XX_RATE(15, 250, 4, 0),
> + PLL_35XX_RATE(14, 175, 3, 0),
> + PLL_35XX_RATE(13, 325, 6, 0),
> + PLL_35XX_RATE(12, 100, 2, 0),
> + PLL_35XX_RATE(11, 275, 3, 1),
> + PLL_35XX_RATE(10, 250, 3, 1),
> + PLL_35XX_RATE(9, 150, 2, 1),
> + PLL_35XX_RATE(8, 200, 3, 1),
> + PLL_35XX_RATE(7, 175, 3, 1),
> + PLL_35XX_RATE(6, 100, 2, 1),
> + PLL_35XX_RATE(5, 250, 3, 2),
> + PLL_35XX_RATE(4, 200, 3, 2),
> + PLL_35XX_RATE(3, 100, 2, 2),
> + PLL_35XX_RATE(2, 200, 3, 3),
nit: The numbers could be aligned to the right using spaces (see exynos4.c).
> + { },
> +};
> +
> +static struct samsung_pll_rate_table cpll_24mhz_tbl[] __initdata = {
> + /* sorted in descending order */
> + /* PLL_35XX_RATE(rate, m, p, s) */
> + PLL_35XX_RATE(66600, 222, 4, 1),
> + PLL_35XX_RATE(64000, 160, 3, 1),
> + PLL_35XX_RATE(32000, 160, 3, 2),
> + { },
> +};
> +
> +static struct samsung_pll_rate_table dpll_24mhz_tbl[] __initdata = {
> + /* sorted in descending order */
> + /* PLL_35XX_RATE(rate, m, p, s) */
> + PLL_35XX_RATE(6, 200, 4, 1),
> + { },
> +};
> +
> +static struct samsung_pll_rate_table epll_24mhz_tbl[] __initdata = {
> + /* sorted in descending order */
> + /* PLL_36XX_RATE(rate, m, p, s, k) */
> + PLL_36XX_RATE(6, 100, 2, 1, 0),
> + PLL_36XX_RATE(4, 200, 3, 2, 0),
> + PLL_36XX_RATE(2, 200, 3, 3, 0),
> + PLL_36XX_RATE(180633600, 301, 5, 3, -3670),
> + PLL_36XX_RATE( 67737600, 452, 5, 5, -27263),
> + PLL_36XX_RATE( 49152000, 197, 3, 5, -25690),
> + PLL_36XX_RATE( 45158401, 181, 3, 5, -24012),
Have you ensured that the rates specified match the rates calculated
using PLL equation? You can find how it is calculated in recalc_rate
callback of this particular PLL type in clk-pll.c.
As a side note, the PLL registration code should be made a bit more
robust and just calculate the rates itself and printing warnings if they
don't match the entered ones. I definitely need more hours in a day, so
much to do. ;)
> + { },
> +};
> +
> +static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
> + /* sorted in descending order */
> + /* PLL_35XX_RATE(rate, m, p, s, k) */
> + PLL_35XX_RATE(86400, 288, 4, 1),
> + PLL_35XX_RATE(66600, 222, 4, 1),
> + PLL_35XX_RATE(43200, 288, 4, 2),
> + { },
> +};
> +
> +static struct samsung_pll_rate_table kpll_24mhz_tbl[] __initdata = {
> + /* sorted in descending order */
> + /* PLL_35XX_RATE(rate, m, p, s) */
> + PLL_35XX_RATE(15, 250, 4, 0),
> + PLL_35XX_RATE(14, 175, 3, 0),
> + PLL_35XX_RATE(13, 325, 6, 0),
> + PLL_35XX_RATE(12, 100, 2, 0),
> + PLL_35XX_RATE(11, 275, 3, 1),
> + PLL_35XX_RATE(10, 250, 3, 1),
> + PLL_35XX_RATE(9, 150, 2, 1),
> + PLL_35XX_RATE(8, 200, 3, 1),
> + PLL_35XX_RATE(7, 175, 3, 1),
> + PLL_35XX_RATE(6, 100, 2, 1),
> + PLL_35XX_RATE(5, 250, 3, 2),
> + PLL_35XX_RATE(4, 200, 3, 2),
> + PLL_35XX_RATE(3, 100, 2, 2),
> + PLL_35XX_RATE(2, 200, 3, 3),
nit: Alignment.
Otherwise looks good, thanks.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Hi Humberto,
You can find my comments inline.
On 31.07.2014 13:22, Humberto Silva Naves wrote:
Added the remaining PLL clocks, and also added the configuration
tables with the PLL coefficients for the supported frequencies.
These frequency tables are only installed when a 24MHz clock is
supplied as the input clock source. To reflect these changes, new
constants were added to the dt-bindings file.
[snip]
+static struct samsung_pll_rate_table apll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s) */
+ PLL_35XX_RATE(21, 175, 2, 0),
+ PLL_35XX_RATE(20, 250, 3, 0),
+ PLL_35XX_RATE(19, 475, 6, 0),
+ PLL_35XX_RATE(18, 225, 3, 0),
+ PLL_35XX_RATE(17, 425, 6, 0),
+ PLL_35XX_RATE(16, 200, 3, 0),
+ PLL_35XX_RATE(15, 250, 4, 0),
+ PLL_35XX_RATE(14, 175, 3, 0),
+ PLL_35XX_RATE(13, 325, 6, 0),
+ PLL_35XX_RATE(12, 100, 2, 0),
+ PLL_35XX_RATE(11, 275, 3, 1),
+ PLL_35XX_RATE(10, 250, 3, 1),
+ PLL_35XX_RATE(9, 150, 2, 1),
+ PLL_35XX_RATE(8, 200, 3, 1),
+ PLL_35XX_RATE(7, 175, 3, 1),
+ PLL_35XX_RATE(6, 100, 2, 1),
+ PLL_35XX_RATE(5, 250, 3, 2),
+ PLL_35XX_RATE(4, 200, 3, 2),
+ PLL_35XX_RATE(3, 100, 2, 2),
+ PLL_35XX_RATE(2, 200, 3, 3),
nit: The numbers could be aligned to the right using spaces (see exynos4.c).
+ { },
+};
+
+static struct samsung_pll_rate_table cpll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s) */
+ PLL_35XX_RATE(66600, 222, 4, 1),
+ PLL_35XX_RATE(64000, 160, 3, 1),
+ PLL_35XX_RATE(32000, 160, 3, 2),
+ { },
+};
+
+static struct samsung_pll_rate_table dpll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s) */
+ PLL_35XX_RATE(6, 200, 4, 1),
+ { },
+};
+
+static struct samsung_pll_rate_table epll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_36XX_RATE(rate, m, p, s, k) */
+ PLL_36XX_RATE(6, 100, 2, 1, 0),
+ PLL_36XX_RATE(4, 200, 3, 2, 0),
+ PLL_36XX_RATE(2, 200, 3, 3, 0),
+ PLL_36XX_RATE(180633600, 301, 5, 3, -3670),
+ PLL_36XX_RATE( 67737600, 452, 5, 5, -27263),
+ PLL_36XX_RATE( 49152000, 197, 3, 5, -25690),
+ PLL_36XX_RATE( 45158401, 181, 3, 5, -24012),
Have you ensured that the rates specified match the rates calculated
using PLL equation? You can find how it is calculated in recalc_rate
callback of this particular PLL type in clk-pll.c.
As a side note, the PLL registration code should be made a bit more
robust and just calculate the rates itself and printing warnings if they
don't match the entered ones. I definitely need more hours in a day, so
much to do. ;)
+ { },
+};
+
+static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s, k) */
+ PLL_35XX_RATE(86400, 288, 4, 1),
+ PLL_35XX_RATE(66600, 222, 4, 1),
+ PLL_35XX_RATE(43200, 288, 4, 2),
+ { },
+};
+
+static struct samsung_pll_rate_table kpll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s) */
+ PLL_35XX_RATE(15, 250, 4, 0),
+ PLL_35XX_RATE(14, 175, 3, 0),
+ PLL_35XX_RATE(13, 325, 6, 0),
+ PLL_35XX_RATE(12, 100, 2, 0),
+ PLL_35XX_RATE(11, 275, 3, 1),
+ PLL_35XX_RATE(10, 250, 3, 1),
+ PLL_35XX_RATE(9, 150, 2, 1),
+ PLL_35XX_RATE(8, 200, 3, 1),
+ PLL_35XX_RATE(7, 175, 3, 1),
+ PLL_35XX_RATE(6, 100, 2, 1),
+ PLL_35XX_RATE(5, 250, 3, 2),
+ PLL_35XX_RATE(4, 200, 3, 2),
+ PLL_35XX_RATE(3, 100, 2, 2),
+ PLL_35XX_RATE(2, 200, 3, 3),
nit: Alignment.
Otherwise looks good, thanks.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Hi Tomasz,
I remember checking these rates on my calculator. You might notice the
odd frequency of 45158401Hz (no pun intended) in the EPLL clock. This
particular clock frequency was giving me a big headache in a previous
project, since it was wrongly listed as 45158400. At first it seems
innocuous, but whenever I would ask for one of the child clocks to
change the rate, the driver would miscalculate the correct frequencies
and errors would propagate up and down the clock tree.
Anyway, I would double check these tables. And if you want, I can
write a separate patch for the rate table validation. I presume that
you would like to add a new field, such as default_base_freq, to the
structure samsung_pll_clock, and if that field is non-zero, you
perform the validation of the table in _samsung_clk_register_pll?
Best,
Humberto
On Thu, Jul 31, 2014 at 3:07 PM, Tomasz Figa tomasz.f...@gmail.com wrote:
Hi Humberto,
You can find my comments inline.
On 31.07.2014 13:22, Humberto Silva Naves wrote:
Added the remaining PLL clocks, and also added the configuration
tables with the PLL coefficients for the supported frequencies.
These frequency tables are only installed when a 24MHz clock is
supplied as the input clock source. To reflect these changes, new
constants were added to the dt-bindings file.
[snip]
+static struct samsung_pll_rate_table apll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s) */
+ PLL_35XX_RATE(21, 175, 2, 0),
+ PLL_35XX_RATE(20, 250, 3, 0),
+ PLL_35XX_RATE(19, 475, 6, 0),
+ PLL_35XX_RATE(18, 225, 3, 0),
+ PLL_35XX_RATE(17, 425, 6, 0),
+ PLL_35XX_RATE(16, 200, 3, 0),
+ PLL_35XX_RATE(15, 250, 4, 0),
+ PLL_35XX_RATE(14, 175, 3, 0),
+ PLL_35XX_RATE(13, 325, 6, 0),
+ PLL_35XX_RATE(12, 100, 2, 0),
+ PLL_35XX_RATE(11, 275, 3, 1),
+ PLL_35XX_RATE(10, 250, 3, 1),
+ PLL_35XX_RATE(9, 150, 2, 1),
+ PLL_35XX_RATE(8, 200, 3, 1),
+ PLL_35XX_RATE(7, 175, 3, 1),
+ PLL_35XX_RATE(6, 100, 2, 1),
+ PLL_35XX_RATE(5, 250, 3, 2),
+ PLL_35XX_RATE(4, 200, 3, 2),
+ PLL_35XX_RATE(3, 100, 2, 2),
+ PLL_35XX_RATE(2, 200, 3, 3),
nit: The numbers could be aligned to the right using spaces (see exynos4.c).
+ { },
+};
+
+static struct samsung_pll_rate_table cpll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s) */
+ PLL_35XX_RATE(66600, 222, 4, 1),
+ PLL_35XX_RATE(64000, 160, 3, 1),
+ PLL_35XX_RATE(32000, 160, 3, 2),
+ { },
+};
+
+static struct samsung_pll_rate_table dpll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s) */
+ PLL_35XX_RATE(6, 200, 4, 1),
+ { },
+};
+
+static struct samsung_pll_rate_table epll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_36XX_RATE(rate, m, p, s, k) */
+ PLL_36XX_RATE(6, 100, 2, 1, 0),
+ PLL_36XX_RATE(4, 200, 3, 2, 0),
+ PLL_36XX_RATE(2, 200, 3, 3, 0),
+ PLL_36XX_RATE(180633600, 301, 5, 3, -3670),
+ PLL_36XX_RATE( 67737600, 452, 5, 5, -27263),
+ PLL_36XX_RATE( 49152000, 197, 3, 5, -25690),
+ PLL_36XX_RATE( 45158401, 181, 3, 5, -24012),
Have you ensured that the rates specified match the rates calculated
using PLL equation? You can find how it is calculated in recalc_rate
callback of this particular PLL type in clk-pll.c.
As a side note, the PLL registration code should be made a bit more
robust and just calculate the rates itself and printing warnings if they
don't match the entered ones. I definitely need more hours in a day, so
much to do. ;)
+ { },
+};
+
+static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s, k) */
+ PLL_35XX_RATE(86400, 288, 4, 1),
+ PLL_35XX_RATE(66600, 222, 4, 1),
+ PLL_35XX_RATE(43200, 288, 4, 2),
+ { },
+};
+
+static struct samsung_pll_rate_table kpll_24mhz_tbl[] __initdata = {
+ /* sorted in descending order */
+ /* PLL_35XX_RATE(rate, m, p, s) */
+ PLL_35XX_RATE(15, 250, 4, 0),
+ PLL_35XX_RATE(14, 175, 3, 0),
+ PLL_35XX_RATE(13, 325, 6, 0),
+ PLL_35XX_RATE(12, 100, 2, 0),
+ PLL_35XX_RATE(11, 275, 3, 1),
+ PLL_35XX_RATE(10, 250, 3, 1),
+ PLL_35XX_RATE(9, 150, 2, 1),
+ PLL_35XX_RATE(8, 200, 3, 1),
+ PLL_35XX_RATE(7, 175, 3, 1),
+ PLL_35XX_RATE(6, 100, 2, 1),
+ PLL_35XX_RATE(5, 250, 3, 2),
+ PLL_35XX_RATE(4, 200, 3, 2),
+ PLL_35XX_RATE(3, 100, 2,
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
On 31.07.2014 15:37, Humberto Naves wrote: Hi Tomasz, I remember checking these rates on my calculator. You might notice the odd frequency of 45158401Hz (no pun intended) in the EPLL clock. This particular clock frequency was giving me a big headache in a previous project, since it was wrongly listed as 45158400. At first it seems innocuous, but whenever I would ask for one of the child clocks to change the rate, the driver would miscalculate the correct frequencies and errors would propagate up and down the clock tree. Anyway, I would double check these tables. And if you want, I can write a separate patch for the rate table validation. I presume that you would like to add a new field, such as default_base_freq, to the structure samsung_pll_clock, and if that field is non-zero, you perform the validation of the table in _samsung_clk_register_pll? I'm not sure I get the idea of the field you're suggesting. If I understand correctly, your intention would be to provide a default frequency if there is no table provided. I don't think there is a need for it, because current code can read back current settings from registers and calculate current rate. As for the validation itself, one more thing that needs to be considered is that the rate table must be sorted. We once decided to rely on the fact that tables in SoC drivers have rates explicitly specified and are correctly sorted, but now I'm inclined to reconsider this, based on the fact that those rates often are already incorrectly calculated in vendor code or even datasheets, which are main information sources for patch authors. Before mainlining PLL drivers (which was quite some time ago), we had a bit different implementation in our internal tree, which did not use explicitly specified rates at all (they could have been considered just comments to improve table readability) and the _register_pll() function simply calculated rates for all entries creating new table for internal use of the PLL driver that was in addition explicitly sorted to make sure that the order is correct. This kind of implementation is what I would lean toward today. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Hi,
On Thu, Jul 31, 2014 at 5:19 PM, Tomasz Figa tomasz.f...@gmail.com wrote:
I'm not sure I get the idea of the field you're suggesting. If I
understand correctly, your intention would be to provide a default
frequency if there is no table provided. I don't think there is a need
for it, because current code can read back current settings from
registers and calculate current rate.
I think I was not clear enough. I am not trying to provide a default
frequency for the clock, but I do want to specify the base frequency
on which the rate table was based upon. Let me give you an example
that will hopefully clarify the matter. Suppose I want to register my
PLL, such as in the 5410. The *current* solution would be like this:
static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
/* PLL_35XX_RATE(rate, m, p, s, k) */
PLL_35XX_RATE(86400, 288, 4, 1),
{ },
};
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, fout_ipll, fin_pll, IPLL_LOCK,
IPLL_CON0, NULL),
};
And in the driver initialization function, I would add the rate table
if the input clock source matches what I expected, in this case 24Mhz:
if (_get_rate(fin_pll) == 24 * MHZ) {
exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
}
An alternative approach would be as follows, we add a new field (say
base_rate) to the structure samsung_pll_clock, and to the macro PLL,
and describe the pll table in a simpler way, such as follows:
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, fout_ipll, fin_pll, IPLL_LOCK,
IPLL_CON0, ipll_24mhz_tbl, 24 * MHZ),
};
and in the _samsung_clk_register_pll function, all the validation
would be performed. Here I am talking about checking if the parent
rate is what is specified on the samsung_pll_clock structure, and if
so, to check if the rates on the rate table actually match what the
coefficients are telling.
As for the validation itself, one more thing that needs to be considered
is that the rate table must be sorted.
The _samsung_clk_register_pll could do that in theory.
We once decided to rely on the fact that tables in SoC drivers have
rates explicitly specified and are correctly sorted, but now I'm
inclined to reconsider this, based on the fact that those rates often
are already incorrectly calculated in vendor code or even datasheets,
which are main information sources for patch authors.
Before mainlining PLL drivers (which was quite some time ago), we had a
bit different implementation in our internal tree, which did not use
explicitly specified rates at all (they could have been considered just
comments to improve table readability) and the _register_pll() function
simply calculated rates for all entries creating new table for internal
use of the PLL driver that was in addition explicitly sorted to make
sure that the order is correct. This kind of implementation is what I
would lean toward today.
I would strongly object to such as solution. I think that in the
table, the frequency *must* be specified. As you said previously, the
coefficients should be carefully chosen. We cannot know for sure that
the same coefficients that work for a base frequency of 24 Mhz will
also work for a different base frequency. So the driver cannot simply
compute the frequency from the coefficients, and it must also check
that the input rate is correct. This is another reason why I want to
add the base_frequency field to that structure.
I believe that the the _samsung_clk_register_pll must double-check if
the frequencies match what the formula tells, and must drop the
entries (or the whole frequency table) that are faulty. And then
finally, it should sort the entries in descending order.
I hope I made myself clear now.
Best regards,
Humberto
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Humberto,
[dropping few addresses from Cc as this topic is rather irrelevant for
them and adding Mike and Sylwester]
On 31.07.2014 23:19, Humberto Naves wrote:
Hi,
On Thu, Jul 31, 2014 at 5:19 PM, Tomasz Figa tomasz.f...@gmail.com wrote:
I'm not sure I get the idea of the field you're suggesting. If I
understand correctly, your intention would be to provide a default
frequency if there is no table provided. I don't think there is a need
for it, because current code can read back current settings from
registers and calculate current rate.
I think I was not clear enough. I am not trying to provide a default
frequency for the clock, but I do want to specify the base frequency
on which the rate table was based upon. Let me give you an example
that will hopefully clarify the matter. Suppose I want to register my
PLL, such as in the 5410. The *current* solution would be like this:
static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
/* PLL_35XX_RATE(rate, m, p, s, k) */
PLL_35XX_RATE(86400, 288, 4, 1),
{ },
};
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, fout_ipll, fin_pll,
IPLL_LOCK,
IPLL_CON0, NULL),
};
And in the driver initialization function, I would add the rate table
if the input clock source matches what I expected, in this case 24Mhz:
if (_get_rate(fin_pll) == 24 * MHZ) {
exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
}
An alternative approach would be as follows, we add a new field (say
base_rate) to the structure samsung_pll_clock, and to the macro PLL,
and describe the pll table in a simpler way, such as follows:
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, fout_ipll, fin_pll,
IPLL_LOCK,
IPLL_CON0, ipll_24mhz_tbl, 24 * MHZ),
};
and in the _samsung_clk_register_pll function, all the validation
would be performed. Here I am talking about checking if the parent
rate is what is specified on the samsung_pll_clock structure, and if
so, to check if the rates on the rate table actually match what the
coefficients are telling.
What if there are multiple sets of tables for different base rates?
Certain PLLs can run with several reference clock frequencies, e.g. VPLL
rates are often specified for 24 MHz and 27 MHz, depending on setting of
mout_vpllsrc.
As for the validation itself, one more thing that needs to be considered
is that the rate table must be sorted.
The _samsung_clk_register_pll could do that in theory.
We once decided to rely on the fact that tables in SoC drivers have
rates explicitly specified and are correctly sorted, but now I'm
inclined to reconsider this, based on the fact that those rates often
are already incorrectly calculated in vendor code or even datasheets,
which are main information sources for patch authors.
Before mainlining PLL drivers (which was quite some time ago), we had a
bit different implementation in our internal tree, which did not use
explicitly specified rates at all (they could have been considered just
comments to improve table readability) and the _register_pll() function
simply calculated rates for all entries creating new table for internal
use of the PLL driver that was in addition explicitly sorted to make
sure that the order is correct. This kind of implementation is what I
would lean toward today.
I would strongly object to such as solution. I think that in the
table, the frequency *must* be specified. As you said previously, the
coefficients should be carefully chosen. We cannot know for sure that
the same coefficients that work for a base frequency of 24 Mhz will
also work for a different base frequency. So the driver cannot simply
compute the frequency from the coefficients, and it must also check
that the input rate is correct. This is another reason why I want to
add the base_frequency field to that structure.
Sorry, I don't understand your concern. Currently it's SoC driver's duty
to check which rate table to use depending on input clock rate. If input
rate matches any table the driver has, it assigns the table pointer.
Otherwise no table is used and the PLL is working in read-only mode.
If we leave this as is, then PLL driver will have enough information to
calculate PLL rate, because it can retrieve input clock rate by calling
clk_get_rate() on its parent clock. No need to specify output_rate in
rate table, as it is redundant. However...
I believe that the the _samsung_clk_register_pll must double-check if
the frequencies match what the formula tells, and must drop the
entries (or the whole frequency table) that are faulty. And then
finally, it should sort the entries in descending order.
Based on your proposal, another idea came to my mind.
We can add input_rate to rate table instead, so that each entry will
have its own
Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL
Quoting Tomasz Figa (2014-07-31 15:17:29)
Humberto,
[dropping few addresses from Cc as this topic is rather irrelevant for
them and adding Mike and Sylwester]
On 31.07.2014 23:19, Humberto Naves wrote:
Hi,
On Thu, Jul 31, 2014 at 5:19 PM, Tomasz Figa tomasz.f...@gmail.com wrote:
I'm not sure I get the idea of the field you're suggesting. If I
understand correctly, your intention would be to provide a default
frequency if there is no table provided. I don't think there is a need
for it, because current code can read back current settings from
registers and calculate current rate.
I think I was not clear enough. I am not trying to provide a default
frequency for the clock, but I do want to specify the base frequency
on which the rate table was based upon. Let me give you an example
that will hopefully clarify the matter. Suppose I want to register my
PLL, such as in the 5410. The *current* solution would be like this:
static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
/* PLL_35XX_RATE(rate, m, p, s, k) */
PLL_35XX_RATE(86400, 288, 4, 1),
{ },
};
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, fout_ipll, fin_pll,
IPLL_LOCK,
IPLL_CON0, NULL),
};
And in the driver initialization function, I would add the rate table
if the input clock source matches what I expected, in this case 24Mhz:
if (_get_rate(fin_pll) == 24 * MHZ) {
exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
}
An alternative approach would be as follows, we add a new field (say
base_rate) to the structure samsung_pll_clock, and to the macro PLL,
and describe the pll table in a simpler way, such as follows:
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, fout_ipll, fin_pll,
IPLL_LOCK,
IPLL_CON0, ipll_24mhz_tbl, 24 * MHZ),
};
and in the _samsung_clk_register_pll function, all the validation
would be performed. Here I am talking about checking if the parent
rate is what is specified on the samsung_pll_clock structure, and if
so, to check if the rates on the rate table actually match what the
coefficients are telling.
What if there are multiple sets of tables for different base rates?
Certain PLLs can run with several reference clock frequencies, e.g. VPLL
rates are often specified for 24 MHz and 27 MHz, depending on setting of
mout_vpllsrc.
As for the validation itself, one more thing that needs to be considered
is that the rate table must be sorted.
The _samsung_clk_register_pll could do that in theory.
We once decided to rely on the fact that tables in SoC drivers have
rates explicitly specified and are correctly sorted, but now I'm
inclined to reconsider this, based on the fact that those rates often
are already incorrectly calculated in vendor code or even datasheets,
which are main information sources for patch authors.
Before mainlining PLL drivers (which was quite some time ago), we had a
bit different implementation in our internal tree, which did not use
explicitly specified rates at all (they could have been considered just
comments to improve table readability) and the _register_pll() function
simply calculated rates for all entries creating new table for internal
use of the PLL driver that was in addition explicitly sorted to make
sure that the order is correct. This kind of implementation is what I
would lean toward today.
I would strongly object to such as solution. I think that in the
table, the frequency *must* be specified. As you said previously, the
coefficients should be carefully chosen. We cannot know for sure that
the same coefficients that work for a base frequency of 24 Mhz will
also work for a different base frequency. So the driver cannot simply
compute the frequency from the coefficients, and it must also check
that the input rate is correct. This is another reason why I want to
add the base_frequency field to that structure.
Sorry, I don't understand your concern. Currently it's SoC driver's duty
to check which rate table to use depending on input clock rate. If input
rate matches any table the driver has, it assigns the table pointer.
Otherwise no table is used and the PLL is working in read-only mode.
If we leave this as is, then PLL driver will have enough information to
calculate PLL rate, because it can retrieve input clock rate by calling
clk_get_rate() on its parent clock. No need to specify output_rate in
rate table, as it is redundant. However...
I believe that the the _samsung_clk_register_pll must double-check if
the frequencies match what the formula tells, and must drop the
entries (or the whole frequency table) that are faulty. And then
finally, it should sort the entries in
