| Message ID | 20220711104719.40939-1-robimarko@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] clk: qcom: clk-rcg2: add rcg2 mux ops | expand |
On 11/07/2022 12:47, Robert Marko wrote: > Add MODULE_ALIAS so that driver will be autoloaded if built as a module. > > Signed-off-by: Robert Marko <robimarko@gmail.com> > --- > drivers/clk/qcom/apss-ipq6018.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c > index f2f502e2d5a4..963c69f2c0c2 100644 > --- a/drivers/clk/qcom/apss-ipq6018.c > +++ b/drivers/clk/qcom/apss-ipq6018.c > @@ -101,5 +101,6 @@ static struct platform_driver apss_ipq6018_driver = { > > module_platform_driver(apss_ipq6018_driver); > > +MODULE_ALIAS("platform:qcom,apss-ipq6018-clk"); That's not correct alias (no commas) and usually alias is not needed at all. If you need one, please explain why it is needed. Module autoloading works fine without aliases... Best regards, Krzysztof
On Mon, 11 Jul 2022 at 14:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 11/07/2022 13:46, Robert Marko wrote: > > On Mon, 11 Jul 2022 at 13:05, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 11/07/2022 12:47, Robert Marko wrote: > >>> Add MODULE_ALIAS so that driver will be autoloaded if built as a module. > >>> > >>> Signed-off-by: Robert Marko <robimarko@gmail.com> > >>> --- > >>> drivers/clk/qcom/apss-ipq6018.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c > >>> index f2f502e2d5a4..963c69f2c0c2 100644 > >>> --- a/drivers/clk/qcom/apss-ipq6018.c > >>> +++ b/drivers/clk/qcom/apss-ipq6018.c > >>> @@ -101,5 +101,6 @@ static struct platform_driver apss_ipq6018_driver = { > >>> > >>> module_platform_driver(apss_ipq6018_driver); > >>> > >>> +MODULE_ALIAS("platform:qcom,apss-ipq6018-clk"); > >> > >> That's not correct alias (no commas) and usually alias is not needed at > >> all. If you need one, please explain why it is needed. Module > >> autoloading works fine without aliases... > > > > Hi Krzysztof, > > alias is required here as the driver does not use a DT compatible but > > is registered > > by the APCS driver, if built as a module, it won't get autoloaded > > without an alias. > > Instead you need device ID table. Aliases are not a workaround for > missing core driver elements. Thanks for pointing this out, it looks like a proper solution for this. I will drop this patch and fix up autoloading after this series gets merged as APCS also requires a fixup, especially since the name in the platform table is limited to 20 characters and the current name does not fit. > > > > > I can only fix up the driver name here and in APCS first to have an > > alias without commas. > > I see that the comma is used in driver name, so this is an independent > issue. Maybe change it to '-' in separate commit? Like with the previous point, I will drop this patch and fix it after this series gets merged. Regards, Robert > > > Best regards, > Krzysztof
On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote: > > Add support for IPQ8074 since it uses the same PLL setup, however it does > not require the Alpha PLL to be reconfigured. > > Signed-off-by: Robert Marko <robimarko@gmail.com> > --- > drivers/clk/qcom/apss-ipq-pll.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c > index bef7899ad0d6..acfb3ec4f142 100644 > --- a/drivers/clk/qcom/apss-ipq-pll.c > +++ b/drivers/clk/qcom/apss-ipq-pll.c > @@ -55,6 +55,7 @@ static const struct regmap_config ipq_pll_regmap_config = { > static int apss_ipq_pll_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > struct regmap *regmap; > void __iomem *base; > int ret; > @@ -67,7 +68,8 @@ static int apss_ipq_pll_probe(struct platform_device *pdev) > if (IS_ERR(regmap)) > return PTR_ERR(regmap); > > - clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config); > + if (of_device_is_compatible(node, "qcom,ipq6018-a53pll")) > + clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config); I'd suggest having the 8074 config here too. It seems logical to me to make sure that the pll is configured correctly. > > ret = devm_clk_register_regmap(dev, &ipq_pll.clkr); > if (ret) > @@ -79,6 +81,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev) > > static const struct of_device_id apss_ipq_pll_match_table[] = { > { .compatible = "qcom,ipq6018-a53pll" }, > + { .compatible = "qcom,ipq8074-a53pll" }, > { } > }; > MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table); > -- > 2.36.1 >
On Mon, 11 Jul 2022 at 15:55, Robert Marko <robimarko@gmail.com> wrote: > > On Mon, 11 Jul 2022 at 14:51, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote: > > > > > > Add support for IPQ8074 since it uses the same PLL setup, however it does > > > not require the Alpha PLL to be reconfigured. > > > > > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > > --- > > > drivers/clk/qcom/apss-ipq-pll.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c > > > index bef7899ad0d6..acfb3ec4f142 100644 > > > --- a/drivers/clk/qcom/apss-ipq-pll.c > > > +++ b/drivers/clk/qcom/apss-ipq-pll.c > > > @@ -55,6 +55,7 @@ static const struct regmap_config ipq_pll_regmap_config = { > > > static int apss_ipq_pll_probe(struct platform_device *pdev) > > > { > > > struct device *dev = &pdev->dev; > > > + struct device_node *node = dev->of_node; > > > struct regmap *regmap; > > > void __iomem *base; > > > int ret; > > > @@ -67,7 +68,8 @@ static int apss_ipq_pll_probe(struct platform_device *pdev) > > > if (IS_ERR(regmap)) > > > return PTR_ERR(regmap); > > > > > > - clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config); > > > + if (of_device_is_compatible(node, "qcom,ipq6018-a53pll")) > > > + clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config); > > > > I'd suggest having the 8074 config here too. It seems logical to me to > > make sure that the pll is configured correctly. > > Hi, > > I have reworked the driver to use match data so it can be easily provided, > However, I dont have it as the downstream QCA kernel does not > reconfigure the PLL, unlike IPQ6018. > I can probably read the registers from a running board and provide that? Yes, please try that. > > Regards, > Robert > > > > > > > > ret = devm_clk_register_regmap(dev, &ipq_pll.clkr); > > > if (ret) > > > @@ -79,6 +81,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev) > > > > > > static const struct of_device_id apss_ipq_pll_match_table[] = { > > > { .compatible = "qcom,ipq6018-a53pll" }, > > > + { .compatible = "qcom,ipq8074-a53pll" }, > > > { } > > > }; > > > MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table); > > > -- > > > 2.36.1 > > > > > > > > > -- > > With best wishes > > Dmitry
On Mon, 11 Jul 2022 at 16:23, Robert Marko <robimarko@gmail.com> wrote: > > On Mon, 11 Jul 2022 at 14:48, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote: > > > > > > While working on IPQ8074 APSS driver it was discovered that IPQ6018 and > > > IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is > > > currently broken. > > > > > > More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux > > > clock. > > > However after debugging why it was always stuck at 800Mhz, it was figured > > > out that its not regmap_mux compatible at all. > > > It is a simple mux but it uses RCG2 register layout and control bits, so > > > > To utilize control bits, you probably should also use > > Hi, > I am not really sure what you mean here? Ugh, excuse me. Sent the message too early. I mean that to utilize RCG2 control bits, you probably also need to use clk_rcg2_is_enabled, etc. > > > > > > utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not > > > having to provide a dummy frequency table. > > > > Could you please clarify this. Your new rcg2 ops seems to be literally > > equivalent to the clk_regmap_mux_closest_ops provided the shift and > > width are set correctly.. > > Well, I have tried playing with the clk_regmap_mux_closest_ops but I > just cannot get it > to work. > > The width like you pointed out should be 8, register offset is > currently pointing at the RCG control > register and not the CFG one, so it obviously does not work. > > Setting the register to 0x54 and shift to 8 will just fail silently, > leaving the shift at 7 and correcting > the register won't work as RCG control bits are not utilized at all > with regmap_mux and DIRTY_CFG > is active when I manually look at the register. Ok, I missed the update_cfg part. So, yes, this looks correct. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > So, I am really not sure how clk_regmap_mux_closest_ops are supposed > to work here at all. > > Regards, > Robert > > > > > While we are here, use ARRAY_SIZE for number of parents. > > > > > > Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards. > > > > > > Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller") > > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > > --- > > > drivers/clk/qcom/apss-ipq6018.c | 13 ++++++------- > > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c > > > index d78ff2f310bf..be952d417ded 100644 > > > --- a/drivers/clk/qcom/apss-ipq6018.c > > > +++ b/drivers/clk/qcom/apss-ipq6018.c > > > @@ -16,7 +16,7 @@ > > > #include "clk-regmap.h" > > > #include "clk-branch.h" > > > #include "clk-alpha-pll.h" > > > -#include "clk-regmap-mux.h" > > > +#include "clk-rcg.h" > > > > > > enum { > > > P_XO, > > > @@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = { > > > { P_APSS_PLL_EARLY, 5 }, > > > }; > > > > > > -static struct clk_regmap_mux apcs_alias0_clk_src = { > > > - .reg = 0x0050, > > > - .width = 3, > > > - .shift = 7, > > > > Judging from rcg2 ops, .shift should be set to 8. > > > > > +static struct clk_rcg2 apcs_alias0_clk_src = { > > > + .cmd_rcgr = 0x0050, > > > + .hid_width = 5, > > > .parent_map = parents_apcs_alias0_clk_src_map, > > > .clkr.hw.init = &(struct clk_init_data){ > > > .name = "apcs_alias0_clk_src", > > > .parent_data = parents_apcs_alias0_clk_src, > > > - .num_parents = 2, > > > - .ops = &clk_regmap_mux_closest_ops, > > > + .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src), > > > + .ops = &clk_rcg2_mux_closest_ops, > > > .flags = CLK_SET_RATE_PARENT, > > > }, > > > }; > > > -- > > > 2.36.1 > > > > > > > > > -- > > With best wishes > > Dmitry
diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h index 012e745794fd..01581f4d2c39 100644 --- a/drivers/clk/qcom/clk-rcg.h +++ b/drivers/clk/qcom/clk-rcg.h @@ -167,6 +167,7 @@ struct clk_rcg2_gfx3d { extern const struct clk_ops clk_rcg2_ops; extern const struct clk_ops clk_rcg2_floor_ops; +extern const struct clk_ops clk_rcg2_mux_closest_ops; extern const struct clk_ops clk_edp_pixel_ops; extern const struct clk_ops clk_byte_ops; extern const struct clk_ops clk_byte2_ops; diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c index 28019edd2a50..609c10f8d0d9 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -509,6 +509,13 @@ const struct clk_ops clk_rcg2_floor_ops = { }; EXPORT_SYMBOL_GPL(clk_rcg2_floor_ops); +const struct clk_ops clk_rcg2_mux_closest_ops = { + .determine_rate = __clk_mux_determine_rate_closest, + .get_parent = clk_rcg2_get_parent, + .set_parent = clk_rcg2_set_parent, +}; +EXPORT_SYMBOL_GPL(clk_rcg2_mux_closest_ops); + struct frac_entry { int num; int den;