| Message ID | 1431707940-19372-4-git-send-email-jenskuske@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 15, 2015 at 06:38:53PM +0200, Jens Kuske wrote: > Currently, the sunxi clock driver gets the name for the base factor clock > of divs clocks from the name field in factors_data. This prevents reusing > of the factor clock for clocks with same properties, but different name. > > This commit makes the divs setup function try to get a name from > clock-output-names in the devicetree. It also removes the name field where > possible and merges the sun4i PLL5 and PLL6 clocks. > > The sun4i PLL5 clock doesn't have a output for the base factor clock, > so we still have to use the name field there. > > Signed-off-by: Jens Kuske <jenskuske@gmail.com> > --- > drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c > index 17cba4d..afe560c 100644 > --- a/drivers/clk/sunxi/clk-sunxi.c > +++ b/drivers/clk/sunxi/clk-sunxi.c > @@ -708,18 +708,10 @@ static const struct factors_data sun4i_pll5_data __initconst = { > .name = "pll5", > }; > > -static const struct factors_data sun4i_pll6_data __initconst = { > - .enable = 31, > - .table = &sun4i_pll5_config, > - .getter = sun4i_get_pll5_factors, > - .name = "pll6", > -}; > - > static const struct factors_data sun6i_a31_pll6_data __initconst = { > .enable = 31, > .table = &sun6i_a31_pll6_config, > .getter = sun6i_a31_get_pll6_factors, > - .name = "pll6x2", > }; > > static const struct factors_data sun5i_a13_ahb_data __initconst = { > @@ -1099,7 +1091,7 @@ static const struct divs_data pll5_divs_data __initconst = { > }; > > static const struct divs_data pll6_divs_data __initconst = { > - .factors = &sun4i_pll6_data, > + .factors = &sun4i_pll5_data, > .ndivs = 4, > .div = { > { .shift = 0, .table = pll6_sata_tbl, .gate = 14 }, /* M, SATA */ > @@ -1141,6 +1133,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node, > struct clk_gate *gate = NULL; > struct clk_fixed_factor *fix_factor; > struct clk_divider *divider; > + struct factors_data factors = *data->factors; > void __iomem *reg; > int ndivs = SUNXI_DIVS_MAX_QTY, i = 0; > int flags, clkflags; > @@ -1149,8 +1142,17 @@ static void __init sunxi_divs_clk_setup(struct device_node *node, > if (data->ndivs) > ndivs = data->ndivs; > > + /* Try to find a name for base factor clock */ > + for (i = 0; i < ndivs; i++) { > + if (data->div[i].self) { I'm not sure we should expect the factor clock to have a self factor. what about taking the first output and taking the substring up to the first "_" ? Maxime
Hi, On 05/16/15 04:10, Chen-Yu Tsai wrote: > 2015?5?16? ??12:39? "Jens Kuske" <jenskuske@gmail.com>??? [..] >> @@ -1141,6 +1133,7 @@ static void __init sunxi_divs_clk_setup(struct > device_node *node, >> struct clk_gate *gate = NULL; >> struct clk_fixed_factor *fix_factor; >> struct clk_divider *divider; >> + struct factors_data factors = *data->factors; Isn't this a copy? >> void __iomem *reg; >> int ndivs = SUNXI_DIVS_MAX_QTY, i = 0; >> int flags, clkflags; >> @@ -1149,8 +1142,17 @@ static void __init sunxi_divs_clk_setup(struct > device_node *node, >> if (data->ndivs) >> ndivs = data->ndivs; >> >> + /* Try to find a name for base factor clock */ >> + for (i = 0; i < ndivs; i++) { >> + if (data->div[i].self) { >> + of_property_read_string_index(node, > "clock-output-names", >> + i, &factors.name); > > Please excuse the bad formatting. > I'm at the airport without my laptop. > > This will not work. All the static factors_data structs are const. > You should make a copy of it, maybe on the stack, > update the .name field, and pass that to sunxi_factors_clk_setup(). > If I didn't miss anything, or misunderstood what you want to copy, this should be working fine. Jens
On 05/17/15 15:06, Maxime Ripard wrote: > On Fri, May 15, 2015 at 06:38:53PM +0200, Jens Kuske wrote: >> Currently, the sunxi clock driver gets the name for the base factor clock >> of divs clocks from the name field in factors_data. This prevents reusing >> of the factor clock for clocks with same properties, but different name. >> >> This commit makes the divs setup function try to get a name from >> clock-output-names in the devicetree. It also removes the name field where >> possible and merges the sun4i PLL5 and PLL6 clocks. >> >> The sun4i PLL5 clock doesn't have a output for the base factor clock, >> so we still have to use the name field there. >> >> Signed-off-by: Jens Kuske <jenskuske@gmail.com> >> --- >> drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++---------- >> 1 file changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> index 17cba4d..afe560c 100644 >> --- a/drivers/clk/sunxi/clk-sunxi.c >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> @@ -708,18 +708,10 @@ static const struct factors_data sun4i_pll5_data __initconst = { >> .name = "pll5", >> }; >> >> -static const struct factors_data sun4i_pll6_data __initconst = { >> - .enable = 31, >> - .table = &sun4i_pll5_config, >> - .getter = sun4i_get_pll5_factors, >> - .name = "pll6", >> -}; >> - >> static const struct factors_data sun6i_a31_pll6_data __initconst = { >> .enable = 31, >> .table = &sun6i_a31_pll6_config, >> .getter = sun6i_a31_get_pll6_factors, >> - .name = "pll6x2", >> }; >> >> static const struct factors_data sun5i_a13_ahb_data __initconst = { >> @@ -1099,7 +1091,7 @@ static const struct divs_data pll5_divs_data __initconst = { >> }; >> >> static const struct divs_data pll6_divs_data __initconst = { >> - .factors = &sun4i_pll6_data, >> + .factors = &sun4i_pll5_data, >> .ndivs = 4, >> .div = { >> { .shift = 0, .table = pll6_sata_tbl, .gate = 14 }, /* M, SATA */ >> @@ -1141,6 +1133,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node, >> struct clk_gate *gate = NULL; >> struct clk_fixed_factor *fix_factor; >> struct clk_divider *divider; >> + struct factors_data factors = *data->factors; >> void __iomem *reg; >> int ndivs = SUNXI_DIVS_MAX_QTY, i = 0; >> int flags, clkflags; >> @@ -1149,8 +1142,17 @@ static void __init sunxi_divs_clk_setup(struct device_node *node, >> if (data->ndivs) >> ndivs = data->ndivs; >> >> + /* Try to find a name for base factor clock */ >> + for (i = 0; i < ndivs; i++) { >> + if (data->div[i].self) { > > I'm not sure we should expect the factor clock to have a self factor. Maybe not, but in that case it would fall back to the name provided in the factors_data struct, as it is the case for sun4i pll5. > what about taking the first output and taking the substring up to the > first "_" ? That only works for the sun[457]i pll5 & pll6, for sun[68]i the base clock name would have to be "pll6x2". Jens
On Mon, May 18, 2015 at 5:15 PM, Jens Kuske <jenskuske@gmail.com> wrote: > Hi, > > On 05/16/15 04:10, Chen-Yu Tsai wrote: >> 2015?5?16? ??12:39? "Jens Kuske" <jenskuske@gmail.com>??? > [..] >>> @@ -1141,6 +1133,7 @@ static void __init sunxi_divs_clk_setup(struct >> device_node *node, >>> struct clk_gate *gate = NULL; >>> struct clk_fixed_factor *fix_factor; >>> struct clk_divider *divider; >>> + struct factors_data factors = *data->factors; > > Isn't this a copy? You're right. Sorry for the noise. > >>> void __iomem *reg; >>> int ndivs = SUNXI_DIVS_MAX_QTY, i = 0; >>> int flags, clkflags; >>> @@ -1149,8 +1142,17 @@ static void __init sunxi_divs_clk_setup(struct >> device_node *node, >>> if (data->ndivs) >>> ndivs = data->ndivs; >>> >>> + /* Try to find a name for base factor clock */ >>> + for (i = 0; i < ndivs; i++) { >>> + if (data->div[i].self) { >>> + of_property_read_string_index(node, >> "clock-output-names", >>> + i, &factors.name); >> >> Please excuse the bad formatting. >> I'm at the airport without my laptop. >> >> This will not work. All the static factors_data structs are const. >> You should make a copy of it, maybe on the stack, >> update the .name field, and pass that to sunxi_factors_clk_setup(). >> > > If I didn't miss anything, or misunderstood what you want to copy, this > should be working fine. > > Jens >
On Mon, May 18, 2015 at 11:22:32AM +0200, Jens Kuske wrote: > On 05/17/15 15:06, Maxime Ripard wrote: > > On Fri, May 15, 2015 at 06:38:53PM +0200, Jens Kuske wrote: > >> Currently, the sunxi clock driver gets the name for the base factor clock > >> of divs clocks from the name field in factors_data. This prevents reusing > >> of the factor clock for clocks with same properties, but different name. > >> > >> This commit makes the divs setup function try to get a name from > >> clock-output-names in the devicetree. It also removes the name field where > >> possible and merges the sun4i PLL5 and PLL6 clocks. > >> > >> The sun4i PLL5 clock doesn't have a output for the base factor clock, > >> so we still have to use the name field there. > >> > >> Signed-off-by: Jens Kuske <jenskuske@gmail.com> > >> --- > >> drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++---------- > >> 1 file changed, 12 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c > >> index 17cba4d..afe560c 100644 > >> --- a/drivers/clk/sunxi/clk-sunxi.c > >> +++ b/drivers/clk/sunxi/clk-sunxi.c > >> @@ -708,18 +708,10 @@ static const struct factors_data sun4i_pll5_data __initconst = { > >> .name = "pll5", > >> }; > >> > >> -static const struct factors_data sun4i_pll6_data __initconst = { > >> - .enable = 31, > >> - .table = &sun4i_pll5_config, > >> - .getter = sun4i_get_pll5_factors, > >> - .name = "pll6", > >> -}; > >> - > >> static const struct factors_data sun6i_a31_pll6_data __initconst = { > >> .enable = 31, > >> .table = &sun6i_a31_pll6_config, > >> .getter = sun6i_a31_get_pll6_factors, > >> - .name = "pll6x2", > >> }; > >> > >> static const struct factors_data sun5i_a13_ahb_data __initconst = { > >> @@ -1099,7 +1091,7 @@ static const struct divs_data pll5_divs_data __initconst = { > >> }; > >> > >> static const struct divs_data pll6_divs_data __initconst = { > >> - .factors = &sun4i_pll6_data, > >> + .factors = &sun4i_pll5_data, > >> .ndivs = 4, > >> .div = { > >> { .shift = 0, .table = pll6_sata_tbl, .gate = 14 }, /* M, SATA */ > >> @@ -1141,6 +1133,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node, > >> struct clk_gate *gate = NULL; > >> struct clk_fixed_factor *fix_factor; > >> struct clk_divider *divider; > >> + struct factors_data factors = *data->factors; > >> void __iomem *reg; > >> int ndivs = SUNXI_DIVS_MAX_QTY, i = 0; > >> int flags, clkflags; > >> @@ -1149,8 +1142,17 @@ static void __init sunxi_divs_clk_setup(struct device_node *node, > >> if (data->ndivs) > >> ndivs = data->ndivs; > >> > >> + /* Try to find a name for base factor clock */ > >> + for (i = 0; i < ndivs; i++) { > >> + if (data->div[i].self) { > > > > I'm not sure we should expect the factor clock to have a self factor. > > Maybe not, but in that case it would fall back to the name provided in > the factors_data struct, as it is the case for sun4i pll5. Which doesn't really solve the underlying issue, just make it work in your (pll6/pll8) case. > > what about taking the first output and taking the substring up to the > > first "_" ? > > That only works for the sun[457]i pll5 & pll6, for sun[68]i the base > clock name would have to be "pll6x2". Why? It's called pll6 in the datasheet, it should really be called pll6. The fact that there is some fixed factors or dividers on some children only impacts those children, and not the system as a whole. And the fact that the "base" clock is pll6x2 and not pll6 itself is very debatable. How do you know which clock is the base one? Most clocks are actually using pll6 as a reference. Maxime
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c index 17cba4d..afe560c 100644 --- a/drivers/clk/sunxi/clk-sunxi.c +++ b/drivers/clk/sunxi/clk-sunxi.c @@ -708,18 +708,10 @@ static const struct factors_data sun4i_pll5_data __initconst = { .name = "pll5", }; -static const struct factors_data sun4i_pll6_data __initconst = { - .enable = 31, - .table = &sun4i_pll5_config, - .getter = sun4i_get_pll5_factors, - .name = "pll6", -}; - static const struct factors_data sun6i_a31_pll6_data __initconst = { .enable = 31, .table = &sun6i_a31_pll6_config, .getter = sun6i_a31_get_pll6_factors, - .name = "pll6x2", }; static const struct factors_data sun5i_a13_ahb_data __initconst = { @@ -1099,7 +1091,7 @@ static const struct divs_data pll5_divs_data __initconst = { }; static const struct divs_data pll6_divs_data __initconst = { - .factors = &sun4i_pll6_data, + .factors = &sun4i_pll5_data, .ndivs = 4, .div = { { .shift = 0, .table = pll6_sata_tbl, .gate = 14 }, /* M, SATA */ @@ -1141,6 +1133,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node, struct clk_gate *gate = NULL; struct clk_fixed_factor *fix_factor; struct clk_divider *divider; + struct factors_data factors = *data->factors; void __iomem *reg; int ndivs = SUNXI_DIVS_MAX_QTY, i = 0; int flags, clkflags; @@ -1149,8 +1142,17 @@ static void __init sunxi_divs_clk_setup(struct device_node *node, if (data->ndivs) ndivs = data->ndivs; + /* Try to find a name for base factor clock */ + for (i = 0; i < ndivs; i++) { + if (data->div[i].self) { + of_property_read_string_index(node, "clock-output-names", + i, &factors.name); + break; + } + } + /* Set up factor clock that we will be dividing */ - pclk = sunxi_factors_clk_setup(node, data->factors); + pclk = sunxi_factors_clk_setup(node, &factors); parent = __clk_get_name(pclk); reg = of_iomap(node, 0);
Currently, the sunxi clock driver gets the name for the base factor clock of divs clocks from the name field in factors_data. This prevents reusing of the factor clock for clocks with same properties, but different name. This commit makes the divs setup function try to get a name from clock-output-names in the devicetree. It also removes the name field where possible and merges the sun4i PLL5 and PLL6 clocks. The sun4i PLL5 clock doesn't have a output for the base factor clock, so we still have to use the name field there. Signed-off-by: Jens Kuske <jenskuske@gmail.com> --- drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)