| Message ID | 1489411604-18700-4-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Neil, Quoting Neil Armstrong (2017-03-13 06:26:42) > @@ -821,6 +893,7 @@ struct pll_params_table gxbb_gp0_params_table[] = { > &gxbb_hdmi_pll, > &gxbb_sys_pll, > &gxbb_gp0_pll, > + &gxl_gp0_pll, Is there a reason for adding the pointer to this array here? It seems to me that the gxbb_gp0_pll and gxl_gp0_pll are mutually exclusive, so perhaps two different tables should be used? > }; > > static struct meson_clk_mpll *const gxbb_clk_mplls[] = { > @@ -923,6 +996,10 @@ static int gxbb_clkc_probe(struct platform_device *pdev) > struct clk *parent_clk; > struct device *dev = &pdev->dev; > > + /* Override GP0 clock for GXL/GXM */ > + if (of_device_is_compatible(dev->of_node, "amlogic,gxl-clkc")) > + gxbb_hw_onecell_data.hws[CLKID_GP0_PLL] = &gxl_gp0_pll.hw; Similarly, this above is a little ugly compared to dedicated tables for each variant. Regards, Mike > + > /* Generic clocks and PLLs */ > clk_base = of_iomap(dev->of_node, 0); > if (!clk_base) { > @@ -996,6 +1073,7 @@ static int gxbb_clkc_probe(struct platform_device *pdev) > > static const struct of_device_id gxbb_clkc_match_table[] = { > { .compatible = "amlogic,gxbb-clkc" }, > + { .compatible = "amlogic,gxl-clkc" }, > { } > }; > > diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h > index 8ee2022..7f99bf6 100644 > --- a/drivers/clk/meson/gxbb.h > +++ b/drivers/clk/meson/gxbb.h > @@ -71,6 +71,8 @@ > #define HHI_GP0_PLL_CNTL2 0x44 /* 0x11 offset in data sheet */ > #define HHI_GP0_PLL_CNTL3 0x48 /* 0x12 offset in data sheet */ > #define HHI_GP0_PLL_CNTL4 0x4c /* 0x13 offset in data sheet */ > +#define HHI_GP0_PLL_CNTL5 0x50 /* 0x14 offset in data sheet */ > +#define HHI_GP0_PLL_CNTL1 0x58 /* 0x16 offset in data sheet */ > > #define HHI_XTAL_DIVN_CNTL 0xbc /* 0x2f offset in data sheet */ > #define HHI_TIMER90K 0xec /* 0x3b offset in data sheet */ > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/22/2017 12:49 AM, Michael Turquette wrote: > Hi Neil, > > Quoting Neil Armstrong (2017-03-13 06:26:42) >> @@ -821,6 +893,7 @@ struct pll_params_table gxbb_gp0_params_table[] = { >> &gxbb_hdmi_pll, >> &gxbb_sys_pll, >> &gxbb_gp0_pll, >> + &gxl_gp0_pll, Yes, because this is the table used to change the register base, this won't harm in any way to add SoC variant clocks, since they they are initialized using the gxbb_hw_onecell_data table. > > Is there a reason for adding the pointer to this array here? It seems to > me that the gxbb_gp0_pll and gxl_gp0_pll are mutually exclusive, so > perhaps two different tables should be used? > >> }; >> >> static struct meson_clk_mpll *const gxbb_clk_mplls[] = { >> @@ -923,6 +996,10 @@ static int gxbb_clkc_probe(struct platform_device *pdev) >> struct clk *parent_clk; >> struct device *dev = &pdev->dev; >> >> + /* Override GP0 clock for GXL/GXM */ >> + if (of_device_is_compatible(dev->of_node, "amlogic,gxl-clkc")) >> + gxbb_hw_onecell_data.hws[CLKID_GP0_PLL] = &gxl_gp0_pll.hw; > > Similarly, this above is a little ugly compared to dedicated tables for > each variant. Here is the true uglyness, but would you like to have the exact same gxbb_hw_onecell_data duplicated for only two different clocks ? The gxbb_hw_onecell_data table is 105 lines, and when adding new clocks, we will need to make sure they are still synchronized. If you have a better idea... I can still push a v2 with such table with also a separate gxbb_clk_plls table stored in a struct given from of_device_get_match_data() Neil > > Regards, > Mike > >> + >> /* Generic clocks and PLLs */ >> clk_base = of_iomap(dev->of_node, 0); >> if (!clk_base) { >> @@ -996,6 +1073,7 @@ static int gxbb_clkc_probe(struct platform_device *pdev) >> >> static const struct of_device_id gxbb_clkc_match_table[] = { >> { .compatible = "amlogic,gxbb-clkc" }, >> + { .compatible = "amlogic,gxl-clkc" }, >> { } >> }; >> >> diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h >> index 8ee2022..7f99bf6 100644 >> --- a/drivers/clk/meson/gxbb.h >> +++ b/drivers/clk/meson/gxbb.h >> @@ -71,6 +71,8 @@ >> #define HHI_GP0_PLL_CNTL2 0x44 /* 0x11 offset in data sheet */ >> #define HHI_GP0_PLL_CNTL3 0x48 /* 0x12 offset in data sheet */ >> #define HHI_GP0_PLL_CNTL4 0x4c /* 0x13 offset in data sheet */ >> +#define HHI_GP0_PLL_CNTL5 0x50 /* 0x14 offset in data sheet */ >> +#define HHI_GP0_PLL_CNTL1 0x58 /* 0x16 offset in data sheet */ >> >> #define HHI_XTAL_DIVN_CNTL 0xbc /* 0x2f offset in data sheet */ >> #define HHI_TIMER90K 0xec /* 0x3b offset in data sheet */ >> -- >> 1.9.1 >> -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Neil Armstrong (2017-03-22 02:22:57) > On 03/22/2017 12:49 AM, Michael Turquette wrote: > > Hi Neil, > > > > Quoting Neil Armstrong (2017-03-13 06:26:42) > >> @@ -821,6 +893,7 @@ struct pll_params_table gxbb_gp0_params_table[] = { > >> &gxbb_hdmi_pll, > >> &gxbb_sys_pll, > >> &gxbb_gp0_pll, > >> + &gxl_gp0_pll, > > Yes, because this is the table used to change the register base, this won't harm in any way > to add SoC variant clocks, since they they are initialized using the gxbb_hw_onecell_data table. > > > > > Is there a reason for adding the pointer to this array here? It seems to > > me that the gxbb_gp0_pll and gxl_gp0_pll are mutually exclusive, so > > perhaps two different tables should be used? > > > >> }; > >> > >> static struct meson_clk_mpll *const gxbb_clk_mplls[] = { > >> @@ -923,6 +996,10 @@ static int gxbb_clkc_probe(struct platform_device *pdev) > >> struct clk *parent_clk; > >> struct device *dev = &pdev->dev; > >> > >> + /* Override GP0 clock for GXL/GXM */ > >> + if (of_device_is_compatible(dev->of_node, "amlogic,gxl-clkc")) > >> + gxbb_hw_onecell_data.hws[CLKID_GP0_PLL] = &gxl_gp0_pll.hw; > > > > Similarly, this above is a little ugly compared to dedicated tables for > > each variant. > > Here is the true uglyness, but would you like to have the exact same gxbb_hw_onecell_data > duplicated for only two different clocks ? > The gxbb_hw_onecell_data table is 105 lines, and when adding new clocks, we will need to > make sure they are still synchronized. > > If you have a better idea... I can still push a v2 with such table with also a > separate gxbb_clk_plls table stored in a struct given from of_device_get_match_data() I was not thinking of duplicating all of the clock data table, but breaking out the parts with variation into separate tables. E.g. a common table, a gxbb table and a gp0 table. But on second look your original solution is fine, especially since those two new tables I mentioned would only have a single element in them, which is silly. Ack. Regards, Mike > > Neil > > > > > Regards, > > Mike > > > >> + > >> /* Generic clocks and PLLs */ > >> clk_base = of_iomap(dev->of_node, 0); > >> if (!clk_base) { > >> @@ -996,6 +1073,7 @@ static int gxbb_clkc_probe(struct platform_device *pdev) > >> > >> static const struct of_device_id gxbb_clkc_match_table[] = { > >> { .compatible = "amlogic,gxbb-clkc" }, > >> + { .compatible = "amlogic,gxl-clkc" }, > >> { } > >> }; > >> > >> diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h > >> index 8ee2022..7f99bf6 100644 > >> --- a/drivers/clk/meson/gxbb.h > >> +++ b/drivers/clk/meson/gxbb.h > >> @@ -71,6 +71,8 @@ > >> #define HHI_GP0_PLL_CNTL2 0x44 /* 0x11 offset in data sheet */ > >> #define HHI_GP0_PLL_CNTL3 0x48 /* 0x12 offset in data sheet */ > >> #define HHI_GP0_PLL_CNTL4 0x4c /* 0x13 offset in data sheet */ > >> +#define HHI_GP0_PLL_CNTL5 0x50 /* 0x14 offset in data sheet */ > >> +#define HHI_GP0_PLL_CNTL1 0x58 /* 0x16 offset in data sheet */ > >> > >> #define HHI_XTAL_DIVN_CNTL 0xbc /* 0x2f offset in data sheet */ > >> #define HHI_TIMER90K 0xec /* 0x3b offset in data sheet */ > >> -- > >> 1.9.1 > >> > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" 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/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c index 5e1a7dc4..1838f22 100644 --- a/drivers/clk/meson/gxbb.c +++ b/drivers/clk/meson/gxbb.c @@ -120,7 +120,7 @@ { /* sentinel */ }, }; -static const struct pll_rate_table gp0_pll_rate_table[] = { +static const struct pll_rate_table gxbb_gp0_pll_rate_table[] = { PLL_RATE(96000000, 32, 1, 3), PLL_RATE(99000000, 33, 1, 3), PLL_RATE(102000000, 34, 1, 3), @@ -248,6 +248,35 @@ { /* sentinel */ }, }; +static const struct pll_rate_table gxl_gp0_pll_rate_table[] = { + PLL_RATE(504000000, 42, 1, 1), + PLL_RATE(516000000, 43, 1, 1), + PLL_RATE(528000000, 44, 1, 1), + PLL_RATE(540000000, 45, 1, 1), + PLL_RATE(552000000, 46, 1, 1), + PLL_RATE(564000000, 47, 1, 1), + PLL_RATE(576000000, 48, 1, 1), + PLL_RATE(588000000, 49, 1, 1), + PLL_RATE(600000000, 50, 1, 1), + PLL_RATE(612000000, 51, 1, 1), + PLL_RATE(624000000, 52, 1, 1), + PLL_RATE(636000000, 53, 1, 1), + PLL_RATE(648000000, 54, 1, 1), + PLL_RATE(660000000, 55, 1, 1), + PLL_RATE(672000000, 56, 1, 1), + PLL_RATE(684000000, 57, 1, 1), + PLL_RATE(696000000, 58, 1, 1), + PLL_RATE(708000000, 59, 1, 1), + PLL_RATE(720000000, 60, 1, 1), + PLL_RATE(732000000, 61, 1, 1), + PLL_RATE(744000000, 62, 1, 1), + PLL_RATE(756000000, 63, 1, 1), + PLL_RATE(768000000, 64, 1, 1), + PLL_RATE(780000000, 65, 1, 1), + PLL_RATE(792000000, 66, 1, 1), + { /* sentinel */ }, +}; + static const struct clk_div_table cpu_div_table[] = { { .val = 1, .div = 1 }, { .val = 2, .div = 2 }, @@ -381,8 +410,51 @@ struct pll_params_table gxbb_gp0_params_table[] = { .no_init_reset = true, .unreset_for_lock = true, }, - .rate_table = gp0_pll_rate_table, - .rate_count = ARRAY_SIZE(gp0_pll_rate_table), + .rate_table = gxbb_gp0_pll_rate_table, + .rate_count = ARRAY_SIZE(gxbb_gp0_pll_rate_table), + .lock = &clk_lock, + .hw.init = &(struct clk_init_data){ + .name = "gp0_pll", + .ops = &meson_clk_pll_ops, + .parent_names = (const char *[]){ "xtal" }, + .num_parents = 1, + .flags = CLK_GET_RATE_NOCACHE, + }, +}; + +struct pll_params_table gxl_gp0_params_table[] = { + PLL_PARAM(HHI_GP0_PLL_CNTL, 0x40010250), + PLL_PARAM(HHI_GP0_PLL_CNTL1, 0xc084a000), + PLL_PARAM(HHI_GP0_PLL_CNTL2, 0xb75020be), + PLL_PARAM(HHI_GP0_PLL_CNTL3, 0x0a59a288), + PLL_PARAM(HHI_GP0_PLL_CNTL4, 0xc000004d), + PLL_PARAM(HHI_GP0_PLL_CNTL5, 0x00078000), +}; + +static struct meson_clk_pll gxl_gp0_pll = { + .m = { + .reg_off = HHI_GP0_PLL_CNTL, + .shift = 0, + .width = 9, + }, + .n = { + .reg_off = HHI_GP0_PLL_CNTL, + .shift = 9, + .width = 5, + }, + .od = { + .reg_off = HHI_GP0_PLL_CNTL, + .shift = 16, + .width = 2, + }, + .params = { + .params_table = gxl_gp0_params_table, + .params_count = ARRAY_SIZE(gxl_gp0_params_table), + .no_init_reset = true, + .reset_lock_loop = true, + }, + .rate_table = gxl_gp0_pll_rate_table, + .rate_count = ARRAY_SIZE(gxl_gp0_pll_rate_table), .lock = &clk_lock, .hw.init = &(struct clk_init_data){ .name = "gp0_pll", @@ -821,6 +893,7 @@ struct pll_params_table gxbb_gp0_params_table[] = { &gxbb_hdmi_pll, &gxbb_sys_pll, &gxbb_gp0_pll, + &gxl_gp0_pll, }; static struct meson_clk_mpll *const gxbb_clk_mplls[] = { @@ -923,6 +996,10 @@ static int gxbb_clkc_probe(struct platform_device *pdev) struct clk *parent_clk; struct device *dev = &pdev->dev; + /* Override GP0 clock for GXL/GXM */ + if (of_device_is_compatible(dev->of_node, "amlogic,gxl-clkc")) + gxbb_hw_onecell_data.hws[CLKID_GP0_PLL] = &gxl_gp0_pll.hw; + /* Generic clocks and PLLs */ clk_base = of_iomap(dev->of_node, 0); if (!clk_base) { @@ -996,6 +1073,7 @@ static int gxbb_clkc_probe(struct platform_device *pdev) static const struct of_device_id gxbb_clkc_match_table[] = { { .compatible = "amlogic,gxbb-clkc" }, + { .compatible = "amlogic,gxl-clkc" }, { } }; diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h index 8ee2022..7f99bf6 100644 --- a/drivers/clk/meson/gxbb.h +++ b/drivers/clk/meson/gxbb.h @@ -71,6 +71,8 @@ #define HHI_GP0_PLL_CNTL2 0x44 /* 0x11 offset in data sheet */ #define HHI_GP0_PLL_CNTL3 0x48 /* 0x12 offset in data sheet */ #define HHI_GP0_PLL_CNTL4 0x4c /* 0x13 offset in data sheet */ +#define HHI_GP0_PLL_CNTL5 0x50 /* 0x14 offset in data sheet */ +#define HHI_GP0_PLL_CNTL1 0x58 /* 0x16 offset in data sheet */ #define HHI_XTAL_DIVN_CNTL 0xbc /* 0x2f offset in data sheet */ #define HHI_TIMER90K 0xec /* 0x3b offset in data sheet */
The clock tree in the Amlogic GXBB and GXL/GXM SoCs is shared, but the GXL/GXM SoCs embeds a different GP0 PLL, and needs different parameters with a vendor provided reduced rate table. This patch adds the GXL GP0 variant, and adds a GXL DT compatible in order to use the GXL GP0 PLL instead of the GXBB specific one. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/clk/meson/gxbb.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++-- drivers/clk/meson/gxbb.h | 2 ++ 2 files changed, 83 insertions(+), 3 deletions(-)