| Message ID | 20220321231548.14276-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | Modernize rest of the krait drivers | expand |
On 13/04/2022 20:35, Ansuel Smith wrote: > On Wed, Apr 13, 2022 at 08:25:31PM +0300, Dmitry Baryshkov wrote: >> On 22/03/2022 02:15, Ansuel Smith wrote: >>> Drop hardcoded safe_sel definition and use helper to correctly calculate >>> it. We assume qsb clk is always present as it should be declared in DTS >>> per Documentation and in the absence of that, it's declared as a fixed >>> clk. >> >> Why? Can safe_sel (sec_mux index) change? >> > > No it can't but I think it would be better to have stuff that is based > on real defined data instead of wrong struct that works just because a > hardcoded value is used. > > Example for the reason of using this hardcoded value, nobody notice that > the mux list for the secondary mux was wrong. Now it didn't cause any > problem as we use the secondary mux just to source out of qsb and we > used the hardcoded value so the error was bypassed but as soon as the > value was actually parsed from the defined table, bam secondary mux was > sourcing out of pll8. > > I honestly think that dropping the hardcoded value would make more clear > what the safe sel is and what is referring to. Can you define indices in the parents and use defined indice instead? I see your point, but in my opinion adding an API to determine a compile-time constant value is a bit of overkill. > >>> >>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> >>> --- >>> drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------ >>> 1 file changed, 27 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c >>> index e9508e3104ea..5f98ee1c3681 100644 >>> --- a/drivers/clk/qcom/krait-cc.c >>> +++ b/drivers/clk/qcom/krait-cc.c >>> @@ -26,6 +26,17 @@ static unsigned int pri_mux_map[] = { >>> 0, >>> }; >>> +static u8 krait_get_mux_sel(struct krait_mux_clk *mux, struct clk *safe_clk) >>> +{ >>> + struct clk_hw *safe_hw = __clk_get_hw(safe_clk); >>> + >>> + /* >>> + * We can ignore errors from clk_hw_get_index_of_parent() >>> + * as we create these parents in this driver. >>> + */ >>> + return clk_hw_get_index_of_parent(&mux->hw, safe_hw); >>> +} >>> + >>> /* >>> * Notifier function for switching the muxes to safe parent >>> * while the hfpll is getting reprogrammed. >>> @@ -116,8 +127,8 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset) >>> } >>> static struct clk * >>> -krait_add_sec_mux(struct device *dev, int id, const char *s, >>> - unsigned int offset, bool unique_aux) >>> +krait_add_sec_mux(struct device *dev, struct clk *qsb, int id, >>> + const char *s, unsigned int offset, bool unique_aux) >>> { >>> int ret; >>> struct krait_mux_clk *mux; >>> @@ -144,7 +155,6 @@ krait_add_sec_mux(struct device *dev, int id, const char *s, >>> mux->shift = 2; >>> mux->parent_map = sec_mux_map; >>> mux->hw.init = &init; >>> - mux->safe_sel = 0; >>> init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s); >>> if (!init.name) >>> @@ -166,6 +176,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s, >>> if (IS_ERR(clk)) >>> goto err_clk; >>> + mux->safe_sel = krait_get_mux_sel(mux, qsb); >>> ret = krait_notifier_register(dev, clk, mux); >>> if (ret) >>> clk = ERR_PTR(ret); >>> @@ -204,7 +215,6 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux >>> mux->lpl = id >= 0; >>> mux->parent_map = pri_mux_map; >>> mux->hw.init = &init; >>> - mux->safe_sel = 2; >>> init.name = kasprintf(GFP_KERNEL, "krait%s_pri_mux", s); >>> if (!init.name) >>> @@ -226,6 +236,7 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux >>> if (IS_ERR(clk)) >>> goto err_clk; >>> + mux->safe_sel = krait_get_mux_sel(mux, sec_mux); >>> ret = krait_notifier_register(dev, clk, mux); >>> if (ret) >>> clk = ERR_PTR(ret); >>> @@ -238,7 +249,9 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux >>> } >>> /* id < 0 for L2, otherwise id == physical CPU number */ >>> -static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux) >>> +static struct clk * >>> +krait_add_clks(struct device *dev, struct clk *qsb, int id, >>> + bool unique_aux) >>> { >>> unsigned int offset; >>> void *p = NULL; >>> @@ -261,7 +274,7 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux) >>> goto err; >>> } >>> - sec_mux = krait_add_sec_mux(dev, id, s, offset, unique_aux); >>> + sec_mux = krait_add_sec_mux(dev, qsb, id, s, offset, unique_aux); >>> if (IS_ERR(sec_mux)) { >>> clk = sec_mux; >>> goto err; >>> @@ -301,18 +314,19 @@ static int krait_cc_probe(struct platform_device *pdev) >>> int cpu; >>> struct clk *clk; >>> struct clk **clks; >>> - struct clk *l2_pri_mux_clk; >>> + struct clk *l2_pri_mux_clk, *qsb; >>> id = of_match_device(krait_cc_match_table, dev); >>> if (!id) >>> return -ENODEV; >>> /* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */ >>> - if (IS_ERR(clk_get(dev, "qsb"))) >>> - clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1); >>> + qsb = clk_get(dev, "qsb"); >>> + if (IS_ERR(qsb)) >>> + qsb = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1); >>> - if (IS_ERR(clk)) >>> - return PTR_ERR(clk); >>> + if (IS_ERR(qsb)) >>> + return PTR_ERR(qsb); >>> if (!id->data) { >>> clk = clk_register_fixed_factor(dev, "acpu_aux", >>> @@ -327,13 +341,13 @@ static int krait_cc_probe(struct platform_device *pdev) >>> return -ENOMEM; >>> for_each_possible_cpu(cpu) { >>> - clk = krait_add_clks(dev, cpu, id->data); >>> + clk = krait_add_clks(dev, qsb, cpu, id->data); >>> if (IS_ERR(clk)) >>> return PTR_ERR(clk); >>> clks[cpu] = clk; >>> } >>> - l2_pri_mux_clk = krait_add_clks(dev, -1, id->data); >>> + l2_pri_mux_clk = krait_add_clks(dev, qsb, -1, id->data); >>> if (IS_ERR(l2_pri_mux_clk)) >>> return PTR_ERR(l2_pri_mux_clk); >>> clks[4] = l2_pri_mux_clk; >> >> >> -- >> With best wishes >> Dmitry >
On 22/03/2022 02:15, Ansuel Smith wrote: > Convert the driver to parent data API. From the Documentation pll8_vote > and pxo should be declared in the DTS so fw_name can be used instead of > parent_names. Name is still used to save regression on old definition. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/clk/qcom/kpss-xcc.c | 25 ++++++++----------------- > 1 file changed, 8 insertions(+), 17 deletions(-) > > diff --git a/drivers/clk/qcom/kpss-xcc.c b/drivers/clk/qcom/kpss-xcc.c > index 4fec1f9142b8..347f70e9f5fe 100644 > --- a/drivers/clk/qcom/kpss-xcc.c > +++ b/drivers/clk/qcom/kpss-xcc.c > @@ -12,9 +12,9 @@ > #include <linux/clk.h> > #include <linux/clk-provider.h> > > -static const char *aux_parents[] = { > - "pll8_vote", > - "pxo", > +static const struct clk_parent_data aux_parents[] = { > + { .name = "pll8_vote", .fw_name = "pll8_vote" }, I'd just use "pll" here for the .fw_name. > + { .name = "pxo", .fw_name = "pxo" }, > }; > > static unsigned int aux_parent_map[] = { > @@ -32,8 +32,8 @@ MODULE_DEVICE_TABLE(of, kpss_xcc_match_table); > static int kpss_xcc_driver_probe(struct platform_device *pdev) > { > const struct of_device_id *id; > - struct clk *clk; > void __iomem *base; > + struct clk_hw *hw; > const char *name; > > id = of_match_device(kpss_xcc_match_table, &pdev->dev); > @@ -55,24 +55,15 @@ static int kpss_xcc_driver_probe(struct platform_device *pdev) > base += 0x28; > } > > - clk = clk_register_mux_table(&pdev->dev, name, aux_parents, > - ARRAY_SIZE(aux_parents), 0, base, 0, 0x3, > - 0, aux_parent_map, NULL); > + hw = __devm_clk_hw_register_mux(&pdev->dev, NULL, name, ARRAY_SIZE(aux_parents), > + NULL, NULL, aux_parents, 0, base, 0, 0x3, > + 0, aux_parent_map, NULL); > > - platform_set_drvdata(pdev, clk); > - > - return PTR_ERR_OR_ZERO(clk); > -} > - > -static int kpss_xcc_driver_remove(struct platform_device *pdev) > -{ > - clk_unregister_mux(platform_get_drvdata(pdev)); > - return 0; > + return PTR_ERR_OR_ZERO(hw); > } > > static struct platform_driver kpss_xcc_driver = { > .probe = kpss_xcc_driver_probe, > - .remove = kpss_xcc_driver_remove, > .driver = { > .name = "kpss-xcc", > .of_match_table = kpss_xcc_match_table,
This is a follow-up to the ipq806x gcc modernize series. Manu cleanup changes and also some discoveries of wrong definition notice only with all these conversions. The first patch is an improvement of the clk_hw_get_parent_index. The original idea of clk_hw_get_parent_index was to give a way to access the parent index but for some reason the final version limited it to the current index. We change it to give the current parent if is not provided and to give the requested parent if provided. Any user of this function is updated to follow the new implementation. The patch 2 and 3 are some additional fixes for gcc. The first one is a fix that register the pxo and cxo fixed clock only if they are not defined in DTS. The patch 3 require some explaination. In short is a big HACK to prevent kernel panic with this series. The kpss-xcc driver is a mess. The Documentation declare that the clocks should be provided but for some reason it was never followed. In fact in the ipq8064 DTSI only the clocks for l2cc are declared but for cpu0 and cpu1 the clocks are not defined. The kpss-xcc driver use parent_names so the clks are ignored and never used so till now it wasn't a problem (ignoring the fact that they doesn't follow documentation at all) On top of that, the l2cc node declare the pxo clock in a really strange way. It's declared using the PXO_SRC gcc clock that is never defined in the gcc ipq8064 clock table. (the correct way was to declare a fixed clock in dts and reference that) To prevent any kind of problem we use the patch 3 and provide the clk for PXO_SRC in the gcc clock table. We manually provide the clk after gcc probe. Patch 4 is just a minor cleanup where we use the poll macro Patch 5 is the actually kpss-xcc conversion to parent data Patch 6-7 should be a fixup of a real conver case Patch 8 converts the krait-cc to parent_data Patch 9 give some love to the code with some minor fixup Patch 10 drop the hardcoded safe sel and use the new clk_hw_get_parent_index to get the safe parent index. (also I discovered that the parent order was wrong) Patch 11 is an additional fixup to force the reset of the muxes even more. Patch 12-13 are some additiona taken from the qsdk that were missing in the upstream driver Patch 14 converts krait-cc to yaml Patch 15 add to krait-cc Documentation the L2 clocks Patch 16 converts the kpss-acc driver to yaml and fix some Documentation error Patch 17 convets the kpss-gcc driver to yaml Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the stupid PXO_SRC phandle) I tested this series on a ipq8064 SoC by running a cache benchmark test to make sure the changes are correct and we don't silently cause regressions. Also I compared the output of the clk_summary every time and we finally have a sane output where the mux are correctly placed in the correct parent. (till now we had the cpu aux clock all over the place, probably never cause problems but who knows.) v6: - Move dts patch as last patch - Address commencts from Rob - Fix warning from make dtbs_check v5: - Address comments from Krzysztof v4: - Fix more dt-bindings bog errors v3: - Split Documentation files for kpss and krait-cc v2: - introduce new API instead of fixing the existing one - do not reorganize variables in krait-cc - fix some comments error and improve it - return better error for patch 7 - fix missing new line on patch 16 Ansuel Smith (18): clk: introduce clk_hw_get_index_of_parent new API clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present clk: qcom: gcc-ipq806x: add PXO_SRC in clk table clk: qcom: clk-hfpll: use poll_timeout macro clk: qcom: kpss-xcc: convert to parent data API clk: qcom: clk-krait: unlock spin after mux completion clk: qcom: clk-krait: add hw_parent check for div2_round_rate clk: qcom: krait-cc: convert to parent_data API clk: qcom: krait-cc: drop pr_info and register qsb only if needed clk: qcom: krait-cc: drop hardcoded safe_sel clk: qcom: krait-cc: force sec_mux to QSB clk: qcom: clk-krait: add apq/ipq8064 errata workaround clk: qcom: clk-krait: add enable disable ops dt-bindings: clock: Convert qcom,krait-cc to yaml dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and clocks .../bindings/arm/msm/qcom,kpss-acc.txt | 49 ----- .../bindings/arm/msm/qcom,kpss-acc.yaml | 94 +++++++++ .../bindings/arm/msm/qcom,kpss-gcc.txt | 44 ----- .../bindings/arm/msm/qcom,kpss-gcc.yaml | 69 +++++++ .../bindings/clock/qcom,krait-cc.txt | 34 ---- .../bindings/clock/qcom,krait-cc.yaml | 65 ++++++ arch/arm/boot/dts/qcom-ipq8064.dtsi | 24 ++- drivers/clk/clk.c | 14 ++ drivers/clk/qcom/clk-hfpll.c | 13 +- drivers/clk/qcom/clk-krait.c | 44 ++++- drivers/clk/qcom/clk-krait.h | 1 + drivers/clk/qcom/gcc-ipq806x.c | 27 ++- drivers/clk/qcom/kpss-xcc.c | 25 +-- drivers/clk/qcom/krait-cc.c | 186 ++++++++++-------- include/linux/clk-provider.h | 1 + 15 files changed, 453 insertions(+), 237 deletions(-) delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml