* [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on sh7372
@ 2010-06-30 9:55 Guennadi Liakhovetski
2010-07-02 8:21 ` [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on Magnus Damm
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2010-06-30 9:55 UTC (permalink / raw)
To: linux-fbdev
Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
other clocks.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
arch/arm/mach-shmobile/clock-sh7372.c | 171 +++++++++++++++++++++++++++++++--
1 files changed, 164 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
index 26521a7..21cb629 100644
--- a/arch/arm/mach-shmobile/clock-sh7372.c
+++ b/arch/arm/mach-shmobile/clock-sh7372.c
@@ -50,6 +50,12 @@
#define SMSTPCR3 0xe615013c
#define SMSTPCR4 0xe6150140
+/* Fixed 27 MHz video clock from DV_CLKI pin */
+static struct clk dv_clki_clk = {
+ .name = "dv_clki",
+ .rate = 27000000,
+};
+
/* Fixed 32 KHz root clock from EXTALR pin */
static struct clk r_clk = {
.rate = 32768,
@@ -81,14 +87,23 @@ static struct clk_ops div2_clk_ops = {
.recalc = div2_recalc,
};
+/* Divide dv_clki by two */
+static struct clk dv_clki_div2_clk = {
+ .name = "dv_clki_div2",
+ .ops = &div2_clk_ops,
+ .parent = &dv_clki_clk,
+};
+
/* Divide extal1 by two */
static struct clk extal1_div2_clk = {
+ .name = "extal1_div2",
.ops = &div2_clk_ops,
.parent = &sh7372_extal1_clk,
};
/* Divide extal2 by two */
static struct clk extal2_div2_clk = {
+ .name = "extal2_div2",
.ops = &div2_clk_ops,
.parent = &sh7372_extal2_clk,
};
@@ -130,35 +145,157 @@ static struct clk pllc1_clk = {
/* Divide PLLC1 by two */
static struct clk pllc1_div2_clk = {
+ .name = "pllc1_div2",
.ops = &div2_clk_ops,
.parent = &pllc1_clk,
};
/* PLLC2 */
+
+/* Indices are important - they are the actual src selecting values */
+static struct clk *pllc2_parent[] = {
+ [0] = &extal1_div2_clk,
+ [1] = &extal2_div2_clk,
+ [2] = &dv_clki_div2_clk,
+ [3] = NULL, /* extal2_div4 not implemented yet*/
+};
+
+/* Only multipliers 20 * 2 to 46 * 2 are valid, last entry for CPUFREQ_TABLE_END */
+static struct cpufreq_frequency_table pllc2_freq_table[28];
+
+static void pllc2_table_rebuild(struct clk *clk)
+{
+ int i;
+
+ /* Initialise PLLC2 frequency table */
+ for (i = 0; i < ARRAY_SIZE(pllc2_freq_table) - 1; i++) {
+ pllc2_freq_table[i].frequency = clk->parent->rate * (i + 20) * 2;
+ pllc2_freq_table[i].index = i;
+ }
+
+ pllc2_freq_table[i].frequency = CPUFREQ_TABLE_END;
+ pllc2_freq_table[i].index = i;
+}
+
static unsigned long pllc2_recalc(struct clk *clk)
{
- unsigned long mult = 1;
+ unsigned long mult;
- if (__raw_readl(PLLC2CR) & (1 << 31))
- mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;
+ pllc2_table_rebuild(clk);
+
+ /*
+ * TODO: If the PLL is off, mult should be = 1, but the clock must be
+ * stopped during re-parenting, a better solution to this conflict
+ * should be found.
+ */
+ mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;
return clk->parent->rate * mult;
}
+static long pllc2_round_rate(struct clk *clk, unsigned long rate)
+{
+ return clk_rate_table_round(clk, clk->freq_table, rate);
+}
+
+static int pllc2_enable(struct clk *clk)
+{
+ int i;
+
+ __raw_writel(__raw_readl(PLLC2CR) | 0x80000000, PLLC2CR);
+
+ for (i = 0; i < 100; i++)
+ if (__raw_readl(PLLC2CR) & 0x80000000)
+ return 0;
+
+ pr_err("%s(): timeout!\n", __func__);
+
+ return -ETIMEDOUT;
+}
+
+static void pllc2_disable(struct clk *clk)
+{
+ __raw_writel(__raw_readl(PLLC2CR) & ~0x80000000, PLLC2CR);
+}
+
+static int pllc2_set_rate(struct clk *clk,
+ unsigned long rate, int algo_id)
+{
+ unsigned long value;
+ int idx;
+
+ idx = clk_rate_table_find(clk, clk->freq_table, rate);
+ if (idx < 0)
+ return idx;
+
+ value = __raw_readl(PLLC2CR) & ~(0x3f << 24);
+
+ if (value & 0x80000000)
+ pllc2_disable(clk);
+
+ __raw_writel((value & ~0x80000000) | ((idx + 19) << 24), PLLC2CR);
+
+ if (value & 0x80000000)
+ return pllc2_enable(clk);
+
+ return 0;
+}
+
+static int pllc2_set_parent(struct clk *clk, struct clk *parent)
+{
+ u32 value;
+ int ret, i;
+
+ if (!clk->parent_table || !clk->parent_num)
+ return -EINVAL;
+
+ /* Search the parent */
+ for (i = 0; i < clk->parent_num; i++)
+ if (clk->parent_table[i] = parent)
+ break;
+
+ if (i = clk->parent_num)
+ return -ENODEV;
+
+ ret = clk_reparent(clk, parent);
+ if (ret < 0)
+ return ret;
+
+ value = __raw_readl(PLLC2CR) & ~(3 << 6);
+
+ __raw_writel(value | (i << 6), PLLC2CR);
+
+ /* Rebiuld the frequency table */
+ pllc2_table_rebuild(clk);
+
+ return 0;
+}
+
static struct clk_ops pllc2_clk_ops = {
.recalc = pllc2_recalc,
+ .round_rate = pllc2_round_rate,
+ .set_rate = pllc2_set_rate,
+ .enable = pllc2_enable,
+ .disable = pllc2_disable,
+ .set_parent = pllc2_set_parent,
};
static struct clk pllc2_clk = {
+ .name = "pllc2",
.ops = &pllc2_clk_ops,
.flags = CLK_ENABLE_ON_INIT,
.parent = &extal1_div2_clk,
+ .freq_table = pllc2_freq_table,
+ .parent_table = pllc2_parent,
+ .parent_num = ARRAY_SIZE(pllc2_parent),
};
static struct clk *main_clks[] = {
+ &dv_clki_clk,
&r_clk,
&sh7372_extal1_clk,
&sh7372_extal2_clk,
+ &dv_clki_div2_clk,
&extal1_div2_clk,
&extal2_div2_clk,
&extal2_div4_clk,
@@ -219,7 +356,7 @@ static struct clk div4_clks[DIV4_NR] = {
enum { DIV6_VCK1, DIV6_VCK2, DIV6_VCK3, DIV6_FMSI, DIV6_FMSO,
DIV6_FSIA, DIV6_FSIB, DIV6_SUB, DIV6_SPU,
- DIV6_VOU, DIV6_HDMI, DIV6_DSIT, DIV6_DSI0P, DIV6_DSI1P,
+ DIV6_VOU, DIV6_DSIT, DIV6_DSI0P, DIV6_DSI1P,
DIV6_NR };
static struct clk div6_clks[DIV6_NR] = {
@@ -233,12 +370,26 @@ static struct clk div6_clks[DIV6_NR] = {
[DIV6_SUB] = SH_CLK_DIV6(&sh7372_extal2_clk, SUBCKCR, 0),
[DIV6_SPU] = SH_CLK_DIV6(&pllc1_div2_clk, SPUCKCR, 0),
[DIV6_VOU] = SH_CLK_DIV6(&pllc1_div2_clk, VOUCKCR, 0),
- [DIV6_HDMI] = SH_CLK_DIV6(&pllc1_div2_clk, HDMICKCR, 0),
[DIV6_DSIT] = SH_CLK_DIV6(&pllc1_div2_clk, DSITCKCR, 0),
[DIV6_DSI0P] = SH_CLK_DIV6(&pllc1_div2_clk, DSI0PCKCR, 0),
[DIV6_DSI1P] = SH_CLK_DIV6(&pllc1_div2_clk, DSI1PCKCR, 0),
};
+enum { DIV6_HDMI, DIV6_REPARENT_NR };
+
+/* Indices are important - they are the actual src selecting values */
+static struct clk *hdmi_parent[] = {
+ [0] = &pllc1_div2_clk,
+ [1] = &pllc2_clk,
+ [2] = &dv_clki_clk,
+ [3] = NULL, /* pllc2_div4 not implemented yet */
+};
+
+static struct clk div6_reparent_clks[DIV6_REPARENT_NR] = {
+ [DIV6_HDMI] = SH_CLK_DIV6_EXT(&pllc1_div2_clk, HDMICKCR, 0,
+ hdmi_parent, ARRAY_SIZE(hdmi_parent), 6, 2),
+};
+
enum { MSTP001,
MSTP131, MSTP130,
MSTP129, MSTP128,
@@ -247,7 +398,7 @@ enum { MSTP001,
MSTP223,
MSTP207, MSTP206, MSTP204, MSTP203, MSTP202, MSTP201, MSTP200,
MSTP329, MSTP328, MSTP323, MSTP322, MSTP314, MSTP313, MSTP312,
- MSTP415, MSTP410, MSTP411, MSTP406, MSTP403,
+ MSTP415, MSTP413, MSTP411, MSTP410, MSTP406, MSTP403,
MSTP_NR };
#define MSTP(_parent, _reg, _bit, _flags) \
@@ -281,6 +432,7 @@ static struct clk mstp_clks[MSTP_NR] = {
[MSTP313] = MSTP(&div4_clks[DIV4_HP], SMSTPCR3, 13, 0), /* SDHI1 */
[MSTP312] = MSTP(&div4_clks[DIV4_HP], SMSTPCR3, 12, 0), /* MMC */
[MSTP415] = MSTP(&div4_clks[DIV4_HP], SMSTPCR4, 15, 0), /* SDHI2 */
+ [MSTP413] = MSTP(&pllc1_div2_clk, SMSTPCR4, 13, 0), /* HDMI */
[MSTP411] = MSTP(&div6_clks[DIV6_SUB], SMSTPCR4, 11, 0), /* IIC3 */
[MSTP410] = MSTP(&div6_clks[DIV6_SUB], SMSTPCR4, 10, 0), /* IIC4 */
[MSTP406] = MSTP(&div6_clks[DIV6_SUB], SMSTPCR4, 6, 0), /* USB1 */
@@ -292,6 +444,7 @@ static struct clk mstp_clks[MSTP_NR] = {
static struct clk_lookup lookups[] = {
/* main clocks */
+ CLKDEV_CON_ID("dv_clki_div2_clk", &dv_clki_div2_clk),
CLKDEV_CON_ID("r_clk", &r_clk),
CLKDEV_CON_ID("extal1", &sh7372_extal1_clk),
CLKDEV_CON_ID("extal2", &sh7372_extal2_clk),
@@ -331,7 +484,7 @@ static struct clk_lookup lookups[] = {
CLKDEV_CON_ID("sub_clk", &div6_clks[DIV6_SUB]),
CLKDEV_CON_ID("spu_clk", &div6_clks[DIV6_SPU]),
CLKDEV_CON_ID("vou_clk", &div6_clks[DIV6_VOU]),
- CLKDEV_CON_ID("hdmi_clk", &div6_clks[DIV6_HDMI]),
+ CLKDEV_CON_ID("hdmi_clk", &div6_reparent_clks[DIV6_HDMI]),
CLKDEV_CON_ID("dsit_clk", &div6_clks[DIV6_DSIT]),
CLKDEV_CON_ID("dsi0p_clk", &div6_clks[DIV6_DSI0P]),
CLKDEV_CON_ID("dsi1p_clk", &div6_clks[DIV6_DSI1P]),
@@ -366,6 +519,7 @@ static struct clk_lookup lookups[] = {
CLKDEV_DEV_ID("sh_mobile_sdhi.1", &mstp_clks[MSTP313]), /* SDHI1 */
CLKDEV_DEV_ID("sh_mmcif.0", &mstp_clks[MSTP312]), /* MMC */
CLKDEV_DEV_ID("sh_mobile_sdhi.2", &mstp_clks[MSTP415]), /* SDHI2 */
+ CLKDEV_CON_ID("sh-hdmi.0", &mstp_clks[MSTP413]), /* HDMI */
CLKDEV_DEV_ID("i2c-sh_mobile.3", &mstp_clks[MSTP411]), /* IIC3 */
CLKDEV_DEV_ID("i2c-sh_mobile.4", &mstp_clks[MSTP410]), /* IIC4 */
CLKDEV_DEV_ID("r8a66597_hcd.1", &mstp_clks[MSTP406]), /* USB1 */
@@ -387,6 +541,9 @@ void __init sh7372_clock_init(void)
ret = sh_clk_div6_register(div6_clks, DIV6_NR);
if (!ret)
+ ret = sh_clk_div6_reparent_register(div6_reparent_clks, DIV6_NR);
+
+ if (!ret)
ret = sh_clk_mstp32_register(mstp_clks, MSTP_NR);
clkdev_add_table(lookups, ARRAY_SIZE(lookups));
--
1.6.2.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on
2010-06-30 9:55 [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on sh7372 Guennadi Liakhovetski
@ 2010-07-02 8:21 ` Magnus Damm
2010-07-02 12:33 ` Guennadi Liakhovetski
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2010-07-02 8:21 UTC (permalink / raw)
To: linux-fbdev
On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
> other clocks.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Hi Guennadi,
Thanks for your work on this.
> diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
> index 26521a7..21cb629 100644
> --- a/arch/arm/mach-shmobile/clock-sh7372.c
> +++ b/arch/arm/mach-shmobile/clock-sh7372.c
> @@ -50,6 +50,12 @@
> #define SMSTPCR3 0xe615013c
> #define SMSTPCR4 0xe6150140
>
> +/* Fixed 27 MHz video clock from DV_CLKI pin */
> +static struct clk dv_clki_clk = {
> + .name = "dv_clki",
> + .rate = 27000000,
> +};
Hm, 27MHz is a board property, not a cpu property.
Also, you don't want to use "name" in struct clk. clkdev is used for
lookup these days.
> /* PLLC2 */
> +
> +/* Indices are important - they are the actual src selecting values */
> +static struct clk *pllc2_parent[] = {
> + [0] = &extal1_div2_clk,
> + [1] = &extal2_div2_clk,
> + [2] = &dv_clki_div2_clk,
> + [3] = NULL, /* extal2_div4 not implemented yet*/
> +};
Why not implemented yet?
> + /*
> + * TODO: If the PLL is off, mult should be = 1, but the clock must be
> + * stopped during re-parenting, a better solution to this conflict
> + * should be found.
> + */
> + mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;
Yes, this needs to be fixed.
> +enum { DIV6_HDMI, DIV6_REPARENT_NR };
> +
> +/* Indices are important - they are the actual src selecting values */
> +static struct clk *hdmi_parent[] = {
> + [0] = &pllc1_div2_clk,
> + [1] = &pllc2_clk,
> + [2] = &dv_clki_clk,
> + [3] = NULL, /* pllc2_div4 not implemented yet */
> +};
> +
> +static struct clk div6_reparent_clks[DIV6_REPARENT_NR] = {
> + [DIV6_HDMI] = SH_CLK_DIV6_EXT(&pllc1_div2_clk, HDMICKCR, 0,
> + hdmi_parent, ARRAY_SIZE(hdmi_parent), 6, 2),
> +};
This part looks nice and clean IMO. Thanks!
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on
2010-06-30 9:55 [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on sh7372 Guennadi Liakhovetski
2010-07-02 8:21 ` [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on Magnus Damm
@ 2010-07-02 12:33 ` Guennadi Liakhovetski
2010-07-05 4:20 ` Magnus Damm
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2010-07-02 12:33 UTC (permalink / raw)
To: linux-fbdev
On Fri, 2 Jul 2010, Magnus Damm wrote:
> On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
> > other clocks.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>
> Hi Guennadi,
>
> Thanks for your work on this.
>
> > diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
> > index 26521a7..21cb629 100644
> > --- a/arch/arm/mach-shmobile/clock-sh7372.c
> > +++ b/arch/arm/mach-shmobile/clock-sh7372.c
> > @@ -50,6 +50,12 @@
> > #define SMSTPCR3 0xe615013c
> > #define SMSTPCR4 0xe6150140
> >
> > +/* Fixed 27 MHz video clock from DV_CLKI pin */
> > +static struct clk dv_clki_clk = {
> > + .name = "dv_clki",
> > + .rate = 27000000,
> > +};
>
> Hm, 27MHz is a board property, not a cpu property.
Well, yes, but I think somewhere something suggested, that 27MHz is what
you're actually supposed to provide there. Otherwise your board can always
just call clk_set_rate(), right? Or what's the preferred way to let
platforms set up clocks?
> Also, you don't want to use "name" in struct clk. clkdev is used for
> lookup these days.
Right, it is not needed, thanks.
> > /* PLLC2 */
> > +
> > +/* Indices are important - they are the actual src selecting values */
> > +static struct clk *pllc2_parent[] = {
> > + [0] = &extal1_div2_clk,
> > + [1] = &extal2_div2_clk,
> > + [2] = &dv_clki_div2_clk,
> > + [3] = NULL, /* extal2_div4 not implemented yet*/
> > +};
>
> Why not implemented yet?
Because it's not used yet. Should I implement it anyway?
> > + /*
> > + * TODO: If the PLL is off, mult should be = 1, but the clock must be
> > + * stopped during re-parenting, a better solution to this conflict
> > + * should be found.
> > + */
> > + mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;
>
> Yes, this needs to be fixed.
You mean now or in the future? If now - I don't see a reasonable fix so
far... Do you?
> > +enum { DIV6_HDMI, DIV6_REPARENT_NR };
> > +
> > +/* Indices are important - they are the actual src selecting values */
> > +static struct clk *hdmi_parent[] = {
> > + [0] = &pllc1_div2_clk,
> > + [1] = &pllc2_clk,
> > + [2] = &dv_clki_clk,
> > + [3] = NULL, /* pllc2_div4 not implemented yet */
> > +};
> > +
> > +static struct clk div6_reparent_clks[DIV6_REPARENT_NR] = {
> > + [DIV6_HDMI] = SH_CLK_DIV6_EXT(&pllc1_div2_clk, HDMICKCR, 0,
> > + hdmi_parent, ARRAY_SIZE(hdmi_parent), 6, 2),
> > +};
>
> This part looks nice and clean IMO. Thanks!
Thanks for the review!
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on
2010-06-30 9:55 [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on sh7372 Guennadi Liakhovetski
2010-07-02 8:21 ` [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on Magnus Damm
2010-07-02 12:33 ` Guennadi Liakhovetski
@ 2010-07-05 4:20 ` Magnus Damm
2010-07-05 15:04 ` Guennadi Liakhovetski
2010-07-06 9:46 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2010-07-05 4:20 UTC (permalink / raw)
To: linux-fbdev
On Fri, Jul 2, 2010 at 9:33 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Fri, 2 Jul 2010, Magnus Damm wrote:
>> On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
>> > other clocks.
>> >
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > --- a/arch/arm/mach-shmobile/clock-sh7372.c
>> > +++ b/arch/arm/mach-shmobile/clock-sh7372.c
>> > @@ -50,6 +50,12 @@
>> > #define SMSTPCR3 0xe615013c
>> > #define SMSTPCR4 0xe6150140
>> >
>> > +/* Fixed 27 MHz video clock from DV_CLKI pin */
>> > +static struct clk dv_clki_clk = {
>> > + .name = "dv_clki",
>> > + .rate = 27000000,
>> > +};
>>
>> Hm, 27MHz is a board property, not a cpu property.
>
> Well, yes, but I think somewhere something suggested, that 27MHz is what
> you're actually supposed to provide there. Otherwise your board can always
> just call clk_set_rate(), right? Or what's the preferred way to let
> platforms set up clocks?
You're not supposed to set that clock to any special frequency at all.
It's totally up to the board designer. On the AP4EVB board it happens
to be set to 27 MHz. I suggest that you make the clock non-static and
use clk_set_rate() in the board code.
>> Also, you don't want to use "name" in struct clk. clkdev is used for
>> lookup these days.
>
> Right, it is not needed, thanks.
>
>> > /* PLLC2 */
>> > +
>> > +/* Indices are important - they are the actual src selecting values */
>> > +static struct clk *pllc2_parent[] = {
>> > + [0] = &extal1_div2_clk,
>> > + [1] = &extal2_div2_clk,
>> > + [2] = &dv_clki_div2_clk,
>> > + [3] = NULL, /* extal2_div4 not implemented yet*/
>> > +};
>>
>> Why not implemented yet?
>
> Because it's not used yet. Should I implement it anyway?
Is it needed by your other patches at this point?
So far the clock routing has been static, so no clocks have got their
parents updated over time. This patch adds a much needed parent
selection support, but only implementing some of the parents does not
make so much sense to me.
>> > + /*
>> > + * TODO: If the PLL is off, mult should be = 1, but the clock must be
>> > + * stopped during re-parenting, a better solution to this conflict
>> > + * should be found.
>> > + */
>> > + mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;
>>
>> Yes, this needs to be fixed.
>
> You mean now or in the future? If now - I don't see a reasonable fix so
> far... Do you?
I'm not sure which way is the best, but I think this patch breaks the
case when the PLL is turned off.
/ magnus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on
2010-06-30 9:55 [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on sh7372 Guennadi Liakhovetski
` (2 preceding siblings ...)
2010-07-05 4:20 ` Magnus Damm
@ 2010-07-05 15:04 ` Guennadi Liakhovetski
2010-07-06 9:46 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2010-07-05 15:04 UTC (permalink / raw)
To: linux-fbdev
On Mon, 5 Jul 2010, Magnus Damm wrote:
> On Fri, Jul 2, 2010 at 9:33 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Fri, 2 Jul 2010, Magnus Damm wrote:
> >> On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
> >> > other clocks.
> >> >
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> > --- a/arch/arm/mach-shmobile/clock-sh7372.c
> >> > +++ b/arch/arm/mach-shmobile/clock-sh7372.c
> >> > @@ -50,6 +50,12 @@
> >> > #define SMSTPCR3 0xe615013c
> >> > #define SMSTPCR4 0xe6150140
> >> >
> >> > +/* Fixed 27 MHz video clock from DV_CLKI pin */
> >> > +static struct clk dv_clki_clk = {
> >> > + .name = "dv_clki",
> >> > + .rate = 27000000,
> >> > +};
> >>
> >> Hm, 27MHz is a board property, not a cpu property.
> >
> > Well, yes, but I think somewhere something suggested, that 27MHz is what
> > you're actually supposed to provide there. Otherwise your board can always
> > just call clk_set_rate(), right? Or what's the preferred way to let
> > platforms set up clocks?
>
> You're not supposed to set that clock to any special frequency at all.
> It's totally up to the board designer. On the AP4EVB board it happens
> to be set to 27 MHz. I suggest that you make the clock non-static and
> use clk_set_rate() in the board code.
Hm, is exporting the clock object really better than doing a
'clk_get(NULL, "name")'?
> >> Also, you don't want to use "name" in struct clk. clkdev is used for
> >> lookup these days.
> >
> > Right, it is not needed, thanks.
> >
> >> > /* PLLC2 */
> >> > +
> >> > +/* Indices are important - they are the actual src selecting values */
> >> > +static struct clk *pllc2_parent[] = {
> >> > + [0] = &extal1_div2_clk,
> >> > + [1] = &extal2_div2_clk,
> >> > + [2] = &dv_clki_div2_clk,
> >> > + [3] = NULL, /* extal2_div4 not implemented yet*/
> >> > +};
> >>
> >> Why not implemented yet?
> >
> > Because it's not used yet. Should I implement it anyway?
>
> Is it needed by your other patches at this point?
>
> So far the clock routing has been static, so no clocks have got their
> parents updated over time. This patch adds a much needed parent
> selection support, but only implementing some of the parents does not
> make so much sense to me.
I'll remove the last entry.
> >> > + /*
> >> > + * TODO: If the PLL is off, mult should be = 1, but the clock must be
> >> > + * stopped during re-parenting, a better solution to this conflict
> >> > + * should be found.
> >> > + */
> >> > + mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;
> >>
> >> Yes, this needs to be fixed.
> >
> > You mean now or in the future? If now - I don't see a reasonable fix so
> > far... Do you?
>
> I'm not sure which way is the best, but I think this patch breaks the
> case when the PLL is turned off.
Ok, I can re-add the test and just add a line
clk->rate = pllc2_recalc(clk);
in pllc2_enable()? This even makes sense in a way - if we know, that
after enabling the ->rate can get out of sync with the actual frequency...
I'll prepare an updated patch to make it easier to see.
Thanks
Guennadi
---
Guennadi Liakhovetski
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on
2010-06-30 9:55 [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on sh7372 Guennadi Liakhovetski
` (3 preceding siblings ...)
2010-07-05 15:04 ` Guennadi Liakhovetski
@ 2010-07-06 9:46 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2010-07-06 9:46 UTC (permalink / raw)
To: linux-fbdev
2010/7/6 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Mon, 5 Jul 2010, Magnus Damm wrote:
>> On Fri, Jul 2, 2010 at 9:33 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > On Fri, 2 Jul 2010, Magnus Damm wrote:
>> >> On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
>> >> <g.liakhovetski@gmx.de> wrote:
>> >> > Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
>> >> > other clocks.
>> >> >
>> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> >> > + /*
>> >> > + * TODO: If the PLL is off, mult should be = 1, but the clock must be
>> >> > + * stopped during re-parenting, a better solution to this conflict
>> >> > + * should be found.
>> >> > + */
>> >> > + mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;
>> >>
>> >> Yes, this needs to be fixed.
>> >
>> > You mean now or in the future? If now - I don't see a reasonable fix so
>> > far... Do you?
>>
>> I'm not sure which way is the best, but I think this patch breaks the
>> case when the PLL is turned off.
>
> Ok, I can re-add the test and just add a line
>
> š š š š š š š šclk->rate = pllc2_recalc(clk);
>
> in pllc2_enable()? This even makes sense in a way - if we know, that
> after enabling the ->rate can get out of sync with the actual frequency...
> I'll prepare an updated patch to make it easier to see.
After looking into the pllc2 part of the data sheet it looks more like
it should be exported like a clock that cannot be disabled. So
basically, extend the frequency table to include one more frequency
and handle the pll-off case as a 1:1 mapping.
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-07-06 9:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-30 9:55 [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on sh7372 Guennadi Liakhovetski
2010-07-02 8:21 ` [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on Magnus Damm
2010-07-02 12:33 ` Guennadi Liakhovetski
2010-07-05 4:20 ` Magnus Damm
2010-07-05 15:04 ` Guennadi Liakhovetski
2010-07-06 9:46 ` Magnus Damm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).