| Message ID | 20180621072525.23832-1-joel@jms.id.au |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [linux,dev-4.17,v2] clk: aspeed: Support HPLL strapping on ast2400 | expand |
On Thu, 21 Jun 2018, at 16:55, Joel Stanley wrote: > The HPLL can be configured through a register (SCU24), however some > platforms chose to configure it through the strapping settings and do > not use the register. This was not noticed as the logic for bit 18 in > SCU24 was confused: set means programmed, but the driver read it as set > means strapped. > > This gives us the correct HPLL value on Palmetto systems, from which > most of the peripheral clocks are generated. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > v2: Fix comment and commit message > --- > drivers/clk/clk-aspeed.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c > index c17032bc853a..7d8cc412959d 100644 > --- a/drivers/clk/clk-aspeed.c > +++ b/drivers/clk/clk-aspeed.c > @@ -24,7 +24,7 @@ > #define ASPEED_MPLL_PARAM 0x20 > #define ASPEED_HPLL_PARAM 0x24 > #define AST2500_HPLL_BYPASS_EN BIT(20) > -#define AST2400_HPLL_STRAPPED BIT(18) > +#define AST2400_HPLL_PROGRAMMED BIT(18) > #define AST2400_HPLL_BYPASS_EN BIT(17) > #define ASPEED_MISC_CTRL 0x2c > #define UART_DIV13_EN BIT(12) > @@ -586,8 +586,24 @@ static void __init aspeed_ast2400_cc(struct regmap *map) > * and we assume that it is enabled > */ > regmap_read(map, ASPEED_HPLL_PARAM, &val); > - WARN(val & AST2400_HPLL_STRAPPED, "hpll is strapped not configured"); > - aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_ast2400_calc_pll("hpll", val); > + if (val & AST2400_HPLL_PROGRAMMED) { > + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = > + aspeed_ast2400_calc_pll("hpll", val); > + } else { > + /* hpll is configured by the strap register */ > + regmap_read(map, ASPEED_STRAP, &val); > + val = (val >> 8) & 0x3; > + if (val == 0x00) > + freq = 384000000; > + else if (val == 0x01) > + freq = 360000000; > + else if (val == 0x02) > + freq = 336000000; > + else if (val == 0x03) > + freq = 408000000; > + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = > + clk_hw_register_fixed_rate(NULL, "hpll", "clkin", 0, freq); > + } Logic looks fine, but I feel this might be better as a lookup table. Bikeshed will be hot pink. Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > > /* > * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK) > -- > 2.17.1 >
On 25 June 2018 at 10:41, Andrew Jeffery <andrew@aj.id.au> wrote: > On Thu, 21 Jun 2018, at 16:55, Joel Stanley wrote: >> The HPLL can be configured through a register (SCU24), however some >> platforms chose to configure it through the strapping settings and do >> not use the register. This was not noticed as the logic for bit 18 in >> SCU24 was confused: set means programmed, but the driver read it as set >> means strapped. >> >> This gives us the correct HPLL value on Palmetto systems, from which >> most of the peripheral clocks are generated. >> >> Signed-off-by: Joel Stanley <joel@jms.id.au> >> --- >> v2: Fix comment and commit message >> --- >> drivers/clk/clk-aspeed.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c >> index c17032bc853a..7d8cc412959d 100644 >> --- a/drivers/clk/clk-aspeed.c >> +++ b/drivers/clk/clk-aspeed.c >> @@ -24,7 +24,7 @@ >> #define ASPEED_MPLL_PARAM 0x20 >> #define ASPEED_HPLL_PARAM 0x24 >> #define AST2500_HPLL_BYPASS_EN BIT(20) >> -#define AST2400_HPLL_STRAPPED BIT(18) >> +#define AST2400_HPLL_PROGRAMMED BIT(18) >> #define AST2400_HPLL_BYPASS_EN BIT(17) >> #define ASPEED_MISC_CTRL 0x2c >> #define UART_DIV13_EN BIT(12) >> @@ -586,8 +586,24 @@ static void __init aspeed_ast2400_cc(struct regmap *map) >> * and we assume that it is enabled >> */ >> regmap_read(map, ASPEED_HPLL_PARAM, &val); >> - WARN(val & AST2400_HPLL_STRAPPED, "hpll is strapped not configured"); >> - aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_ast2400_calc_pll("hpll", val); >> + if (val & AST2400_HPLL_PROGRAMMED) { >> + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = >> + aspeed_ast2400_calc_pll("hpll", val); >> + } else { >> + /* hpll is configured by the strap register */ >> + regmap_read(map, ASPEED_STRAP, &val); >> + val = (val >> 8) & 0x3; >> + if (val == 0x00) >> + freq = 384000000; >> + else if (val == 0x01) >> + freq = 360000000; >> + else if (val == 0x02) >> + freq = 336000000; >> + else if (val == 0x03) >> + freq = 408000000; >> + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = >> + clk_hw_register_fixed_rate(NULL, "hpll", "clkin", 0, freq); >> + } > > Logic looks fine, but I feel this might be better as a lookup table. Bikeshed will be hot pink. I anticipated your review, and sent v3 last week with a lookup table. I also merged it into the openbmc tree :)
On Mon, 25 Jun 2018, at 10:43, Joel Stanley wrote: > On 25 June 2018 at 10:41, Andrew Jeffery <andrew@aj.id.au> wrote: > > On Thu, 21 Jun 2018, at 16:55, Joel Stanley wrote: > >> The HPLL can be configured through a register (SCU24), however some > >> platforms chose to configure it through the strapping settings and do > >> not use the register. This was not noticed as the logic for bit 18 in > >> SCU24 was confused: set means programmed, but the driver read it as set > >> means strapped. > >> > >> This gives us the correct HPLL value on Palmetto systems, from which > >> most of the peripheral clocks are generated. > >> > >> Signed-off-by: Joel Stanley <joel@jms.id.au> > >> --- > >> v2: Fix comment and commit message > >> --- > >> drivers/clk/clk-aspeed.c | 22 +++++++++++++++++++--- > >> 1 file changed, 19 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c > >> index c17032bc853a..7d8cc412959d 100644 > >> --- a/drivers/clk/clk-aspeed.c > >> +++ b/drivers/clk/clk-aspeed.c > >> @@ -24,7 +24,7 @@ > >> #define ASPEED_MPLL_PARAM 0x20 > >> #define ASPEED_HPLL_PARAM 0x24 > >> #define AST2500_HPLL_BYPASS_EN BIT(20) > >> -#define AST2400_HPLL_STRAPPED BIT(18) > >> +#define AST2400_HPLL_PROGRAMMED BIT(18) > >> #define AST2400_HPLL_BYPASS_EN BIT(17) > >> #define ASPEED_MISC_CTRL 0x2c > >> #define UART_DIV13_EN BIT(12) > >> @@ -586,8 +586,24 @@ static void __init aspeed_ast2400_cc(struct regmap *map) > >> * and we assume that it is enabled > >> */ > >> regmap_read(map, ASPEED_HPLL_PARAM, &val); > >> - WARN(val & AST2400_HPLL_STRAPPED, "hpll is strapped not configured"); > >> - aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_ast2400_calc_pll("hpll", val); > >> + if (val & AST2400_HPLL_PROGRAMMED) { > >> + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = > >> + aspeed_ast2400_calc_pll("hpll", val); > >> + } else { > >> + /* hpll is configured by the strap register */ > >> + regmap_read(map, ASPEED_STRAP, &val); > >> + val = (val >> 8) & 0x3; > >> + if (val == 0x00) > >> + freq = 384000000; > >> + else if (val == 0x01) > >> + freq = 360000000; > >> + else if (val == 0x02) > >> + freq = 336000000; > >> + else if (val == 0x03) > >> + freq = 408000000; > >> + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = > >> + clk_hw_register_fixed_rate(NULL, "hpll", "clkin", 0, freq); > >> + } > > > > Logic looks fine, but I feel this might be better as a lookup table. Bikeshed will be hot pink. > > I anticipated your review, and sent v3 last week with a lookup table. > I also merged it into the openbmc tree :) Hah, right after I sent it I saw there was v3 and :facepalm:ed. I've got to get more responsive with mail :/
diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c index c17032bc853a..7d8cc412959d 100644 --- a/drivers/clk/clk-aspeed.c +++ b/drivers/clk/clk-aspeed.c @@ -24,7 +24,7 @@ #define ASPEED_MPLL_PARAM 0x20 #define ASPEED_HPLL_PARAM 0x24 #define AST2500_HPLL_BYPASS_EN BIT(20) -#define AST2400_HPLL_STRAPPED BIT(18) +#define AST2400_HPLL_PROGRAMMED BIT(18) #define AST2400_HPLL_BYPASS_EN BIT(17) #define ASPEED_MISC_CTRL 0x2c #define UART_DIV13_EN BIT(12) @@ -586,8 +586,24 @@ static void __init aspeed_ast2400_cc(struct regmap *map) * and we assume that it is enabled */ regmap_read(map, ASPEED_HPLL_PARAM, &val); - WARN(val & AST2400_HPLL_STRAPPED, "hpll is strapped not configured"); - aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_ast2400_calc_pll("hpll", val); + if (val & AST2400_HPLL_PROGRAMMED) { + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = + aspeed_ast2400_calc_pll("hpll", val); + } else { + /* hpll is configured by the strap register */ + regmap_read(map, ASPEED_STRAP, &val); + val = (val >> 8) & 0x3; + if (val == 0x00) + freq = 384000000; + else if (val == 0x01) + freq = 360000000; + else if (val == 0x02) + freq = 336000000; + else if (val == 0x03) + freq = 408000000; + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = + clk_hw_register_fixed_rate(NULL, "hpll", "clkin", 0, freq); + } /* * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
The HPLL can be configured through a register (SCU24), however some platforms chose to configure it through the strapping settings and do not use the register. This was not noticed as the logic for bit 18 in SCU24 was confused: set means programmed, but the driver read it as set means strapped. This gives us the correct HPLL value on Palmetto systems, from which most of the peripheral clocks are generated. Signed-off-by: Joel Stanley <joel@jms.id.au> --- v2: Fix comment and commit message --- drivers/clk/clk-aspeed.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)