Modernize rest of the krait drivers
v2 cover Modernize rest of the krait drivers
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 (should i also convert the kpss-scc driver?)
Patch 15 finally adds all this stuff to the ipq8064 dtsi (and fix the stupid PXO_SRC phandle)
Patch 16 conver the kpss driver to yaml and fix some Docuemntation errors
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.)
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 (16):
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
ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
clocks
dt-bindings: arm: msm: Convert kpss driver Documentation to yaml
.../bindings/arm/msm/qcom,kpss-acc.txt | 49 ----- .../bindings/arm/msm/qcom,kpss-acc.yaml | 97 +++++++++ .../bindings/arm/msm/qcom,kpss-gcc.txt | 44 ----- .../bindings/arm/msm/qcom,kpss-gcc.yaml | 63 ++++++ .../bindings/clock/qcom,krait-cc.txt | 34 ---- .../bindings/clock/qcom,krait-cc.yaml | 63 ++++++ arch/arm/boot/dts/qcom-ipq8064.dtsi | 20 +- 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, 445 insertions(+), 236 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
Signature · 1 line
2.34.1
PATCH v2 1/16 clk: introduce clk_hw_get_index_of_parent new API
Clk can have multiple parents. Some clk may require to get the cached index of other parent that are not current associated with the clk. We have clk_hw_get_parent_index() that returns the index of the current parent but we can't get the index of other parents of the provided clk. Introduce clk_hw_get_index_of_parent() to get the cached index of the parent of the provided clk. This permits a direct access of the internal clk_fetch_parent_index().
--- drivers/clk/clk.c | 14 ++++++++++++++ include/linux/clk-provider.h | 1 + 2 files changed, 15 insertions(+)
drivers/clk/clk.c +14 -0
@@ -1726,6 +1726,20 @@ int clk_hw_get_parent_index(struct clk_hw *hw) }EXPORT_SYMBOL_GPL (clk_hw_get_parent_index); +/** +* clk_hw_get_index_of_parent - return the index of the parent clock +* @hw: clk_hw associated with the clk being consumed +* @parent: clk_hw of the parent to be fetched the index of +* +* Fetches and returns the index of parent clock provided. +* Returns -EINVAL if the given parent index can't be provided. +*/ +int clk_hw_get_index_of_parent (struct clk_hw *hw,const struct clk_hw *parent) +{ +return clk_fetch_parent_index (hw->core, parent->core); +} +EXPORT_SYMBOL_GPL (clk_hw_get_index_of_parent); +/* * Update the orphan status of @core and all its children. */
include/linux/clk-provider.h +1 -0
@@ -1198,6 +1198,7 @@ unsigned int clk_hw_get_num_parents(const struct clk_hw *hw);struct clk_hw *clk_hw_get_parent (const struct clk_hw *hw);struct clk_hw *clk_hw_get_parent_by_index (const struct clk_hw *hw,unsigned int index); +int clk_hw_get_index_of_parent (struct clk_hw *hw,const struct clk_hw *parent);int clk_hw_get_parent_index (struct clk_hw *hw);int clk_hw_set_parent (struct clk_hw *hw,struct clk_hw *new_parent);unsigned int __clk_get_enable_count (struct clk *clk);
Signature · 1 line
2.34.1
PATCH v2 2/16 clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
In reply to Ansuel SmithSkip pxo/cxo clock registration if they are already defined in DTS as fixed clock.
--- drivers/clk/qcom/gcc-ipq806x.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
drivers/clk/qcom/gcc-ipq806x.c +14 -6
@@ -10,6 +10,7 @@#include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/clk.h> #include <linux/clk-provider.h> #include <linux/regmap.h> #include <linux/reset-controller.h>
@@ -3061,15 +3062,22 @@ static int gcc_ipq806x_probe(struct platform_device *pdev) {struct device *dev = &pdev->dev;struct regmap *regmap; +struct clk *clk;int ret; - ret =qcom_cc_register_board_clk (dev,"cxo_board" ,"cxo" ,25000000 ); -if (ret) -return ret; + clk =clk_get (dev,"cxo" ); +if (IS_ERR (clk)) { + ret =qcom_cc_register_board_clk (dev,"cxo_board" ,"cxo" ,25000000 ); +if (ret) +return ret; + } - ret =qcom_cc_register_board_clk (dev,"pxo_board" ,"pxo" ,25000000 ); -if (ret) -return ret; + clk =clk_get (dev,"pxo" ); +if (IS_ERR (clk)) { + ret =qcom_cc_register_board_clk (dev,"pxo_board" ,"pxo" ,25000000 ); +if (ret) +return ret; + } ret =qcom_cc_probe (pdev, &gcc_ipq806x_desc);if (ret)
Signature · 1 line
2.34.1
PATCH v2 3/16 clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
In reply to Ansuel SmithPXO_SRC is currently defined in the gcc include and referenced in the ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel panic if a driver starts to actually use it.
--- drivers/clk/qcom/gcc-ipq806x.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/clk/qcom/gcc-ipq806x.c +7 -0
@@ -26,6 +26,8 @@#include "clk-hfpll.h" #include "reset.h" +static struct clk_regmap pxo = { }; +static struct clk_pll pll0 = { .l_reg =0x30c4 , .m_reg =0x30c8 ,
@@ -2754,6 +2756,7 @@ static struct clk_dyn_rcg ubi32_core2_src_clk = { };static struct clk_regmap *gcc_ipq806x_clks[] = { + [PXO_SRC ] =NULL , [PLL0 ] = &pll0.clkr, [PLL0_VOTE ] = &pll0_vote, [PLL3 ] = &pll3.clkr,
@@ -3083,6 +3086,10 @@ static int gcc_ipq806x_probe(struct platform_device *pdev)if (ret)return ret; + clk =clk_get (dev,"pxo" ); + pxo.hw = *__clk_get_hw (clk); + gcc_ipq806x_clks[PXO_SRC ] = &pxo; + regmap =dev_get_regmap (dev,NULL );if (!regmap)return -ENODEV ;
Signature · 1 line
2.34.1
PATCH v2 4/16 clk: qcom: clk-hfpll: use poll_timeout macro
In reply to Ansuel SmithUse regmap_read_poll_timeout macro instead of do-while structure.
--- drivers/clk/qcom/clk-hfpll.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/clk/qcom/clk-hfpll.c +7 -6
@@ -12,6 +12,8 @@#include "clk-regmap.h" #include "clk-hfpll.h" +#define HFPLL_BUSY_WAIT_TIMEOUT 100 +#define PLL_OUTCTRL BIT(0)#define PLL_BYPASSNL BIT(1)#define PLL_RESET_N BIT(2)
@@ -72,13 +74,12 @@ static void __clk_hfpll_enable(struct clk_hw *hw)regmap_update_bits (regmap, hd->mode_reg,PLL_RESET_N ,PLL_RESET_N );/* Wait for PLL to lock. */ -if (hd->status_reg) { -do { -regmap_read (regmap, hd->status_reg, &val); - }while (!(val &BIT (hd->lock_bit))); - }else { +if (hd->status_reg) +regmap_read_poll_timeout (regmap, hd->status_reg, val, + !(val &BIT (hd->lock_bit)),USEC_PER_MSEC *2 , +HFPLL_BUSY_WAIT_TIMEOUT *USEC_PER_MSEC ); +else udelay (60 ); - }/* Enable PLL output. */ regmap_update_bits (regmap, hd->mode_reg,PLL_OUTCTRL ,PLL_OUTCTRL );
Signature · 1 line
2.34.1
PATCH v2 5/16 clk: qcom: kpss-xcc: convert to parent data API
In reply to Ansuel SmithConvert 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.
--- drivers/clk/qcom/kpss-xcc.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)
drivers/clk/qcom/kpss-xcc.c +8 -17
@@ -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" }, + { .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,
Signature · 1 line
2.34.1
PATCH v2 6/16 clk: qcom: clk-krait: unlock spin after mux completion
In reply to Ansuel SmithUnlock spinlock after the mux switch is completed to prevent any corner case of mux request while the switch still needs to be done.
--- drivers/clk/qcom/clk-krait.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/clk/qcom/clk-krait.c +2 -1
@@ -32,11 +32,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel) regval |= (sel & mux->mask) << (mux->shift +LPL_SHIFT ); }krait_set_l2_indirect_reg (mux->offset, regval); -spin_unlock_irqrestore (&krait_clock_reg_lock, flags);/* Wait for switch to complete. */ mb ();udelay (1 ); + +spin_unlock_irqrestore (&krait_clock_reg_lock, flags); }static int krait_mux_set_parent (struct clk_hw *hw,u8 index)
Signature · 1 line
2.34.1
PATCH v2 7/16 clk: qcom: clk-krait: add hw_parent check for div2_round_rate
In reply to Ansuel SmithCheck if hw_parent is present before calculating the round_rate to prevent kernel panic. On error -EINVAL is reported.
--- drivers/clk/qcom/clk-krait.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/clk/qcom/clk-krait.c +6 -1
@@ -80,7 +80,12 @@ EXPORT_SYMBOL_GPL(krait_mux_clk_ops);static long krait_div2_round_rate (struct clk_hw *hw,unsigned long rate,unsigned long *parent_rate) { - *parent_rate =clk_hw_round_rate (clk_hw_get_parent (hw), rate *2 ); +struct clk_hw *hw_parent =clk_hw_get_parent (hw); + +if (!hw_parent) +return -EINVAL ; + + *parent_rate =clk_hw_round_rate (hw_parent, rate *2 );return DIV_ROUND_UP (*parent_rate,2 ); }
Signature · 1 line
2.34.1
PATCH v2 8/16 clk: qcom: krait-cc: convert to parent_data API
In reply to Ansuel SmithModernize the krait-cc driver to parent-data API and refactor to drop any use of clk_names. From Documentation all the required clocks should be declared in DTS so fw_name can be correctly used to get the parents for all the muxes. Name is also declared to save compatibility with old implementation. Also fix the parent order of the sec_mux that was wrong and incorrectly report the wrong safe parent if it's not hardcoded.
--- drivers/clk/qcom/krait-cc.c | 126 +++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 60 deletions(-)
drivers/clk/qcom/krait-cc.c +66 -60
@@ -69,21 +69,22 @@ static int krait_notifier_register(struct device *dev, struct clk *clk,return ret; } -static int +static struct clk *krait_add_div (struct device *dev,int id,const char *s,unsigned int offset) {struct krait_div2_clk *div; +static struct clk_parent_data p_data[1 ];struct clk_init_data init = { - .num_parents =1 , + .num_parents =ARRAY_SIZE (p_data), .ops = &krait_div2_clk_ops, .flags =CLK_SET_RATE_PARENT , }; -const char *p_names[1 ];struct clk *clk; +char *parent_name; div =devm_kzalloc (dev,sizeof (*div),GFP_KERNEL );if (!div) -return -ENOMEM ; +return ERR_PTR (-ENOMEM ); div->width =2 ; div->shift =6 ;
@@ -93,43 +94,49 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset) init.name =kasprintf (GFP_KERNEL ,"hfpll%s_div" , s);if (!init.name) -return -ENOMEM ; +return ERR_PTR (-ENOMEM ); - init.parent_names = p_names; - p_names[0 ] =kasprintf (GFP_KERNEL ,"hfpll%s" , s); -if (!p_names[0 ]) { -kfree (init.name); -return -ENOMEM ; + init.parent_data = p_data; + parent_name =kasprintf (GFP_KERNEL ,"hfpll%s" , s); +if (!parent_name) { + clk =ERR_PTR (-ENOMEM ); + goto err_parent_name; } + p_data[0 ].fw_name = parent_name; + p_data[0 ].name = parent_name; + clk =devm_clk_register (dev, &div->hw); -kfree (p_names[0 ]); + +kfree (parent_name); +err_parent_name:kfree (init.name); -return PTR_ERR_OR_ZERO (clk); +return clk; } -static int +static struct clk *krait_add_sec_mux (struct device *dev,int id,const char *s,unsigned int offset,bool unique_aux) {int ret;struct krait_mux_clk *mux; -static const char *sec_mux_list[] = { -"acpu_aux" , -"qsb" , +static struct clk_parent_data sec_mux_list[2 ] = { + { .name ="qsb" , .fw_name ="qsb" }, + {}, };struct clk_init_data init = { - .parent_names = sec_mux_list, + .parent_data = sec_mux_list, .num_parents =ARRAY_SIZE (sec_mux_list), .ops = &krait_mux_clk_ops, .flags =CLK_SET_RATE_PARENT , };struct clk *clk; +char *parent_name; mux =devm_kzalloc (dev,sizeof (*mux),GFP_KERNEL );if (!mux) -return -ENOMEM ; +return ERR_PTR (-ENOMEM ); mux->offset = offset; mux->lpl = id >=0 ;
@@ -141,44 +148,51 @@ krait_add_sec_mux(struct device *dev, int id, const char *s, init.name =kasprintf (GFP_KERNEL ,"krait%s_sec_mux" , s);if (!init.name) -return -ENOMEM ; +return ERR_PTR (-ENOMEM );if (unique_aux) { - sec_mux_list[0 ] =kasprintf (GFP_KERNEL ,"acpu%s_aux" , s); -if (!sec_mux_list[0 ]) { + parent_name =kasprintf (GFP_KERNEL ,"acpu%s_aux" , s); +if (!parent_name) { clk =ERR_PTR (-ENOMEM ); goto err_aux; } + sec_mux_list[1 ].fw_name = parent_name; + sec_mux_list[1 ].name = parent_name; + }else { + sec_mux_list[1 ].name ="apu_aux" ; } clk =devm_clk_register (dev, &mux->hw); +if (IS_ERR (clk)) + goto err_clk; ret =krait_notifier_register (dev, clk, mux);if (ret) - goto unique_aux; + clk =ERR_PTR (ret); -unique_aux: +err_clk:if (unique_aux) -kfree (sec_mux_list[0 ]); +kfree (parent_name); err_aux:kfree (init.name); -return PTR_ERR_OR_ZERO (clk); +return clk; }static struct clk * -krait_add_pri_mux (struct device *dev,int id,const char *s, -unsigned int offset) +krait_add_pri_mux (struct device *dev,struct clk *hfpll_div,struct clk *sec_mux, +int id,const char *s,unsigned int offset) {int ret;struct krait_mux_clk *mux; -const char *p_names[3 ]; +static struct clk_parent_data p_data[3 ];struct clk_init_data init = { - .parent_names = p_names, - .num_parents =ARRAY_SIZE (p_names), + .parent_data = p_data, + .num_parents =ARRAY_SIZE (p_data), .ops = &krait_mux_clk_ops, .flags =CLK_SET_RATE_PARENT , };struct clk *clk; +char *hfpll_name; mux =devm_kzalloc (dev,sizeof (*mux),GFP_KERNEL );if (!mux)
@@ -196,36 +210,29 @@ krait_add_pri_mux(struct device *dev, int id, const char *s,if (!init.name)return ERR_PTR (-ENOMEM ); - p_names[0 ] =kasprintf (GFP_KERNEL ,"hfpll%s" , s); -if (!p_names[0 ]) { + hfpll_name =kasprintf (GFP_KERNEL ,"hfpll%s" , s); +if (!hfpll_name) { clk =ERR_PTR (-ENOMEM ); - goto err_p0; + goto err_hfpll; } - p_names[1 ] =kasprintf (GFP_KERNEL ,"hfpll%s_div" , s); -if (!p_names[1 ]) { - clk =ERR_PTR (-ENOMEM ); - goto err_p1; - } + p_data[0 ].fw_name = hfpll_name; + p_data[0 ].name = hfpll_name; - p_names[2 ] =kasprintf (GFP_KERNEL ,"krait%s_sec_mux" , s); -if (!p_names[2 ]) { - clk =ERR_PTR (-ENOMEM ); - goto err_p2; - } + p_data[1 ].hw =__clk_get_hw (hfpll_div); + p_data[2 ].hw =__clk_get_hw (sec_mux); clk =devm_clk_register (dev, &mux->hw); +if (IS_ERR (clk)) + goto err_clk; ret =krait_notifier_register (dev, clk, mux);if (ret) - goto err_p3; -err_p3: -kfree (p_names[2 ]); -err_p2: -kfree (p_names[1 ]); -err_p1: -kfree (p_names[0 ]); -err_p0: + clk =ERR_PTR (ret); + +err_clk: +kfree (hfpll_name); +err_hfpll:kfree (init.name);return clk; }
@@ -233,11 +240,10 @@ krait_add_pri_mux(struct device *dev, int id, const char *s,/* id < 0 for L2, otherwise id == physical CPU number */ static struct clk *krait_add_clks (struct device *dev,int id,bool unique_aux) { -int ret;unsigned int offset;void *p =NULL ;const char *s; -struct clk *clk; +struct clk *hfpll_div, *sec_mux, *clk;if (id >=0 ) { offset =0x4501 + (0x1000 * id);
@@ -249,19 +255,19 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux) s ="_l2" ; } - ret =krait_add_div (dev, id, s, offset); -if (ret) { - clk =ERR_PTR (ret); + hfpll_div =krait_add_div (dev, id, s, offset); +if (IS_ERR (hfpll_div)) { + clk = hfpll_div; goto err; } - ret =krait_add_sec_mux (dev, id, s, offset, unique_aux); -if (ret) { - clk =ERR_PTR (ret); + sec_mux =krait_add_sec_mux (dev, id, s, offset, unique_aux); +if (IS_ERR (sec_mux)) { + clk = sec_mux; goto err; } - clk =krait_add_pri_mux (dev, id, s, offset); + clk =krait_add_pri_mux (dev, hfpll_div, sec_mux, id, s, offset); err:kfree (p);return clk;
Signature · 1 line
2.34.1
PATCH v2 9/16 clk: qcom: krait-cc: drop pr_info and register qsb only if needed
In reply to Ansuel SmithReplace pr_info() with dev_info() to provide better diagnostics. Register qsb fixed clk only if it's not declared in DTS.
--- drivers/clk/qcom/krait-cc.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/clk/qcom/krait-cc.c +7 -5
@@ -308,7 +308,9 @@ static int krait_cc_probe(struct platform_device *pdev)return -ENODEV ;/* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */ - clk =clk_register_fixed_rate (dev,"qsb" ,NULL ,0 ,1 ); +if (IS_ERR (clk_get (dev,"qsb" ))) + clk =clk_register_fixed_rate (dev,"qsb" ,NULL ,0 ,1 ); +if (IS_ERR (clk))return PTR_ERR (clk);
@@ -363,25 +365,25 @@ static int krait_cc_probe(struct platform_device *pdev) cur_rate =clk_get_rate (l2_pri_mux_clk); aux_rate =384000000 ;if (cur_rate ==1 ) { -pr_info ("L2 @ QSB rate. Forcing new rate.\n" ); +dev_info (dev,"L2 @ QSB rate. Forcing new rate.\n" ); cur_rate = aux_rate; }clk_set_rate (l2_pri_mux_clk, aux_rate);clk_set_rate (l2_pri_mux_clk,2 );clk_set_rate (l2_pri_mux_clk, cur_rate); -pr_info ("L2 @ %lu KHz\n" ,clk_get_rate (l2_pri_mux_clk) /1000 ); +dev_info (dev,"L2 @ %lu KHz\n" ,clk_get_rate (l2_pri_mux_clk) /1000 );for_each_possible_cpu (cpu) { clk = clks[cpu]; cur_rate =clk_get_rate (clk);if (cur_rate ==1 ) { -pr_info ("CPU%d @ QSB rate. Forcing new rate.\n" , cpu); +dev_info (dev,"CPU%d @ QSB rate. Forcing new rate.\n" , cpu); cur_rate = aux_rate; }clk_set_rate (clk, aux_rate);clk_set_rate (clk,2 );clk_set_rate (clk, cur_rate); -pr_info ("CPU%d @ %lu KHz\n" , cpu,clk_get_rate (clk) /1000 ); +dev_info (dev,"CPU%d @ %lu KHz\n" , cpu,clk_get_rate (clk) /1000 ); }of_clk_add_provider (dev->of_node, krait_of_get, clks);
Signature · 1 line
2.34.1
PATCH v2 10/16 clk: qcom: krait-cc: drop hardcoded safe_sel
In reply to Ansuel SmithDrop 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.
--- drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 13 deletions(-)
drivers/clk/qcom/krait-cc.c +27 -13
@@ -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_muxif (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;
Signature · 1 line
2.34.1
PATCH v2 11/16 clk: qcom: krait-cc: force sec_mux to QSB
In reply to Ansuel SmithNow that we have converted every driver to parent_data, it was notice that the bootloader can't really leave the system in a strange state where l2 or the cpu0/1 can be sourced in a number of ways for example cpu1 sourcing out of qsb, l2 sourcing out of pxo. To correctly reset the mux and the HFPLL force the sec_mux to QSB.
--- drivers/clk/qcom/krait-cc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/clk/qcom/krait-cc.c +11 -2
@@ -15,6 +15,8 @@#include "clk-krait.h" +#define QSB_RATE 1 +static unsigned int sec_mux_map[] = {2 ,0 ,
@@ -181,6 +183,13 @@ krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,if (ret) clk =ERR_PTR (ret); +/* +* Force the sec_mux to be set to QSB rate. +* This is needed to correctly set the parents and +* to later reset mux and HFPLL to a known freq. +*/ +clk_set_rate (clk,QSB_RATE ); + err_clk:if (unique_aux)kfree (parent_name);
@@ -378,7 +387,7 @@ static int krait_cc_probe(struct platform_device *pdev)*/ cur_rate =clk_get_rate (l2_pri_mux_clk); aux_rate =384000000 ; -if (cur_rate ==1 ) { +if (cur_rate ==QSB_RATE ) {dev_info (dev,"L2 @ QSB rate. Forcing new rate.\n" ); cur_rate = aux_rate; }
@@ -389,7 +398,7 @@ static int krait_cc_probe(struct platform_device *pdev)for_each_possible_cpu (cpu) { clk = clks[cpu]; cur_rate =clk_get_rate (clk); -if (cur_rate ==1 ) { +if (cur_rate ==QSB_RATE ) {dev_info (dev,"CPU%d @ QSB rate. Forcing new rate.\n" , cpu); cur_rate = aux_rate; }
Signature · 1 line
2.34.1
PATCH v2 12/16 clk: qcom: clk-krait: add apq/ipq8064 errata workaround
In reply to Ansuel SmithAdd apq/ipq8064 errata workaround where the sec_src clock gating needs to be disabled during switching. To enable this set disable_sec_src_gating in the mux struct.
--- drivers/clk/qcom/clk-krait.c | 16 ++++++++++++++++ drivers/clk/qcom/clk-krait.h | 1 + drivers/clk/qcom/krait-cc.c | 1 + 3 files changed, 18 insertions(+)
drivers/clk/qcom/clk-krait.c +16 -0
@@ -18,13 +18,23 @@static DEFINE_SPINLOCK (krait_clock_reg_lock);#define LPL_SHIFT 8 +#define SECCLKAGD BIT(4) +static void __krait_mux_set_sel (struct krait_mux_clk *mux,int sel) {unsigned long flags;u32 regval;spin_lock_irqsave (&krait_clock_reg_lock, flags); + regval =krait_get_l2_indirect_reg (mux->offset); + +/* apq/ipq8064 Errata: disable sec_src clock gating during switch. */ +if (mux->disable_sec_src_gating) { + regval |=SECCLKAGD ; +krait_set_l2_indirect_reg (mux->offset, regval); + } + regval &= ~(mux->mask << mux->shift); regval |= (sel & mux->mask) << mux->shift;if (mux->lpl) {
@@ -33,6 +43,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel) }krait_set_l2_indirect_reg (mux->offset, regval); +/* apq/ipq8064 Errata: re-enabled sec_src clock gating. */ +if (mux->disable_sec_src_gating) { + regval &= ~SECCLKAGD ; +krait_set_l2_indirect_reg (mux->offset, regval); + } +/* Wait for switch to complete. */ mb ();udelay (1 );
drivers/clk/qcom/clk-krait.h +1 -0
@@ -15,6 +15,7 @@ struct krait_mux_clk {u8 safe_sel;u8 old_index;bool reparent; +bool disable_sec_src_gating;struct clk_hw hw;struct notifier_block clk_nb;
drivers/clk/qcom/krait-cc.c +1 -0
@@ -157,6 +157,7 @@ krait_add_sec_mux(struct device *dev, struct clk *qsb, int id, mux->shift =2 ; mux->parent_map = sec_mux_map; mux->hw.init = &init; + mux->disable_sec_src_gating = true; init.name =kasprintf (GFP_KERNEL ,"krait%s_sec_mux" , s);if (!init.name)
Signature · 1 line
2.34.1
PATCH v2 13/16 clk: qcom: clk-krait: add enable disable ops
In reply to Ansuel SmithAdd enable/disable ops for krait mux. On disable the mux is set to the safe selection.
--- drivers/clk/qcom/clk-krait.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/clk/qcom/clk-krait.c +18 -0
@@ -85,7 +85,25 @@ static u8 krait_mux_get_parent(struct clk_hw *hw)return clk_mux_val_to_index (hw, mux->parent_map,0 , sel); } +static int krait_mux_enable (struct clk_hw *hw) +{ +struct krait_mux_clk *mux =to_krait_mux_clk (hw); + +__krait_mux_set_sel (mux, mux->en_mask); + +return 0 ; +} + +static void krait_mux_disable (struct clk_hw *hw) +{ +struct krait_mux_clk *mux =to_krait_mux_clk (hw); + +__krait_mux_set_sel (mux, mux->safe_sel); +} +const struct clk_ops krait_mux_clk_ops = { + .enable = krait_mux_enable, + .disable = krait_mux_disable, .set_parent = krait_mux_set_parent, .get_parent = krait_mux_get_parent, .determine_rate = __clk_mux_determine_rate_closest,
Signature · 1 line
2.34.1
PATCH v2 14/16 dt-bindings: clock: Convert qcom,krait-cc to yaml
In reply to Ansuel SmithConvert qcom,krait-cc to yaml and add missing l2 clocks and names definiton.
--- .../bindings/clock/qcom,krait-cc.txt | 34 ---------- .../bindings/clock/qcom,krait-cc.yaml | 63 +++++++++++++++++++ 2 files changed, 63 insertions(+), 34 deletions(-) delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
Documentation/devicetree/bindings/clock/qcom,krait-cc.txt deleted +0 -34
@@ -1,34 +0,0 @@ -Krait Clock Controller - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: must be one of: - "qcom,krait-cc-v1" - "qcom,krait-cc-v2" - -- #clock-cells: - Usage: required - Value type: <u32> - Definition: must be 1 - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the clock parents of hfpll, secondary muxes. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "hfpll0", "hfpll1", "acpu0_aux", "acpu1_aux", "qsb". - -Example: - - kraitcc: clock-controller { - compatible = "qcom,krait-cc-v1"; - clocks = <&hfpll0>, <&hfpll1>, <&acpu0_aux>, <&acpu1_aux>, <qsb>; - clock-names = "hfpll0", "hfpll1", "acpu0_aux", "acpu1_aux", "qsb"; - #clock-cells = <1>; - };
Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml new file +63 -0
63 lines not on this page · raw message
Signature · 1 line
2.34.1
Re: [PATCH v2 14/16] dt-bindings: clock: Convert qcom,krait-cc to yaml
Quoted 2 lines · Ansuel Smith
Convert qcom,krait-cc to yaml and add missing l2 clocks and names definiton.
lang typo.
Quoted 75 lines
Signed-off-by Ansuel Smith <[email protected]>--- .../bindings/clock/qcom,krait-cc.txt | 34 ---------- .../bindings/clock/qcom,krait-cc.yaml | 63 +++++++++++++++++++ 2 files changed, 63 insertions(+), 34 deletions(-) delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yamlDocumentation/devicetree/bindings/clock/qcom,krait-cc.txt deleted +0
-34@@ -1,34 +0,0 @@ -Krait Clock Controller - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: must be one of: - "qcom,krait-cc-v1" - "qcom,krait-cc-v2" - -- #clock-cells: - Usage: required - Value type: <u32> - Definition: must be 1 - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the clock parents of hfpll, secondary muxes. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "hfpll0", "hfpll1", "acpu0_aux", "acpu1_aux", "qsb". - -Example: - - kraitcc: clock-controller { - compatible = "qcom,krait-cc-v1"; - clocks = <&hfpll0>, <&hfpll1>, <&acpu0_aux>, <&acpu1_aux>, <qsb>; - clock-names = "hfpll0", "hfpll1", "acpu0_aux", "acpu1_aux", "qsb"; - #clock-cells = <1>; - };Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml new file +20
-0@@ -0,0 +1,63 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/qcom,krait-cc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Krait Clock Controller + +maintainers: + - Ansuel Smith <[email protected]> + +description: | + Qualcomm Krait Clock Controller used to correctly scale the CPU and the L2 + rates. + +properties: + compatible: + enum: + - qcom,krait-cc-v1 + - qcom,krait-cc-v2
Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml line 30
Quoted 10 lines
Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml new file +10
-0+ + clocks: + items: + - description: phandle to hfpll for CPU0 mux + - description: phandle to hfpll for CPU1 mux + - description: phandle to hfpll for L2 mux + - description: phandle to CPU0 aux clock + - description: phandle to CPU1 aux clock + - description: phandle to L2 aux clock + - description: phandle to QSB fixed clk
None of these were in original bindings, original DTS (because it does not exist), original driver. Therefore you do not need to change bindings during conversion.
I propose to add them in second commit with explanation why do you add them.
Best regards,
Krzysztof
PATCH v2 15/16 ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and clocks
In reply to Ansuel SmithAdd missing krait-cc clock-controller and define missing aux clock for CPUs. Also change phandle for l2cc node to point to pxo_board instead of gcc PXO_SRC.
--- arch/arm/boot/dts/qcom-ipq8064.dtsi | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
arch/arm/boot/dts/qcom-ipq8064.dtsi +19 -1
@@ -468,11 +468,19 @@ IRQ_TYPE_EDGE_RISING)>, acc0: clock-controller@2088000 { compatible = "qcom,kpss-acc-v1"; reg = <0x02088000 0x1000>, <0x02008000 0x1000>; + clock-output-names = "acpu0_aux"; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; + clock-names = "pll8_vote", "pxo"; + #clock-cells = <0>; }; acc1: clock-controller@2098000 { compatible = "qcom,kpss-acc-v1"; reg = <0x02098000 0x1000>, <0x02008000 0x1000>; + clock-output-names = "acpu1_aux"; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; + clock-names = "pll8_vote", "pxo"; + #clock-cells = <0>; }; adm_dma: dma-controller@18300000 {
@@ -782,11 +790,21 @@ tcsr: syscon@1a400000 { l2cc: clock-controller@2011000 { compatible = "qcom,kpss-gcc", "syscon"; reg = <0x2011000 0x1000>; - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; clock-names = "pll8_vote", "pxo"; clock-output-names = "acpu_l2_aux"; }; + kraitcc: clock-controller { + compatible = "qcom,krait-cc-v1"; + clocks = <&gcc PLL9>, <&gcc PLL10>, <&gcc PLL12>, + <&acc0>, <&acc1>, <&l2cc>, <&qsb>; + clock-names = "hfpll0", "hfpll1", "hfpll_l2", + "acpu0_aux", "acpu1_aux", "acpu_l2_aux", + "qsb"; + #clock-cells = <1>; + }; + lcc: clock-controller@28000000 { compatible = "qcom,lcc-ipq8064"; reg = <0x28000000 0x1000>;
Signature · 1 line
2.34.1
PATCH v2 16/16 dt-bindings: arm: msm: Convert kpss driver Documentation to yaml
In reply to Ansuel SmithConvert kpss-acc and kpss-gcc Documentation to yaml. Fix multiple Documentation error and provide additional example for kpss-gcc-v2.
--- .../bindings/arm/msm/qcom,kpss-acc.txt | 49 ---------- .../bindings/arm/msm/qcom,kpss-acc.yaml | 97 +++++++++++++++++++ .../bindings/arm/msm/qcom,kpss-gcc.txt | 44 --------- .../bindings/arm/msm/qcom,kpss-gcc.yaml | 63 ++++++++++++ 4 files changed, 160 insertions(+), 93 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
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt deleted +0 -49
@@ -1,49 +0,0 @@ -Krait Processor Sub-system (KPSS) Application Clock Controller (ACC) - -The KPSS ACC provides clock, power domain, and reset control to a Krait CPU. -There is one ACC register region per CPU within the KPSS remapped region as -well as an alias register region that remaps accesses to the ACC associated -with the CPU accessing the region. - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: should be one of: - "qcom,kpss-acc-v1" - "qcom,kpss-acc-v2" - -- reg: - Usage: required - Value type: <prop-encoded-array> - Definition: the first element specifies the base address and size of - the register region. An optional second element specifies - the base address and size of the alias register region. - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the pll parents. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "pll8_vote", "pxo". - -- clock-output-names: - Usage: optional - Value type: <string> - Definition: Name of the output clock. Typically acpuX_aux where X is a - CPU number starting at 0. - -Example: - - clock-controller@2088000 { - compatible = "qcom,kpss-acc-v2"; - reg = <0x02088000 0x1000>, - <0x02008000 0x1000>; - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>; - clock-names = "pll8_vote", "pxo"; - clock-output-names = "acpu0_aux"; - };
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +97 -0
97 lines not on this page · raw message
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt deleted +0 -44
44 lines not on this page · raw message
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +63 -0
63 lines not on this page · raw message
Signature · 1 line
2.34.1
Re: [PATCH v2 16/16] dt-bindings: arm: msm: Convert kpss driver Documentation to yaml
Quoted 124 lines · Ansuel Smith
Convert kpss-acc and kpss-gcc Documentation to yaml. Fix multiple Documentation error and provide additional example for kpss-gcc-v2.
Signed-off-by Ansuel Smith <[email protected]>--- .../bindings/arm/msm/qcom,kpss-acc.txt | 49 ---------- .../bindings/arm/msm/qcom,kpss-acc.yaml | 97 +++++++++++++++++++ .../bindings/arm/msm/qcom,kpss-gcc.txt | 44 --------- .../bindings/arm/msm/qcom,kpss-gcc.yaml | 63 ++++++++++++ 4 files changed, 160 insertions(+), 93 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.yamlDocumentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt deleted +0
-49@@ -1,49 +0,0 @@ -Krait Processor Sub-system (KPSS) Application Clock Controller (ACC) - -The KPSS ACC provides clock, power domain, and reset control to a Krait CPU. -There is one ACC register region per CPU within the KPSS remapped region as -well as an alias register region that remaps accesses to the ACC associated -with the CPU accessing the region. - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: should be one of: - "qcom,kpss-acc-v1" - "qcom,kpss-acc-v2" - -- reg: - Usage: required - Value type: <prop-encoded-array> - Definition: the first element specifies the base address and size of - the register region. An optional second element specifies - the base address and size of the alias register region. - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the pll parents. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "pll8_vote", "pxo". - -- clock-output-names: - Usage: optional - Value type: <string> - Definition: Name of the output clock. Typically acpuX_aux where X is a - CPU number starting at 0. - -Example: - - clock-controller@2088000 { - compatible = "qcom,kpss-acc-v2"; - reg = <0x02088000 0x1000>, - <0x02008000 0x1000>; - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>; - clock-names = "pll8_vote", "pxo"; - clock-output-names = "acpu0_aux"; - };Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +48
-0@@ -0,0 +1,97 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-acc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Krait Processor Sub-system (KPSS) Application Clock Controller (ACC) + +maintainers: + - Ansuel Smith <[email protected]> + +description: | + The KPSS ACC provides clock, power domain, and reset control to a Krait CPU. + There is one ACC register region per CPU within the KPSS remapped region as + well as an alias register region that remaps accesses to the ACC associated + with the CPU accessing the region. + +properties: + compatible: + enum: + - qcom,kpss-acc-v1 + - qcom,kpss-acc-v2 + + reg: + items: + - description: Base address and size of the register region + - description: Optional base address and size of the alias register region + + clocks: + items: + - description: phandle to pll8_vote + - description: phandle to pxo_board + + clock-names: + items: + - const: pll8_vote + - const: pxo + + clock-output-names: + description: Name of the aux clock. Krait can have at most 4 cpu. + enum: + - acpu0_aux + - acpu1_aux + - acpu2_aux + - acpu3_aux + + '#clock-cells': + const: 0
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml line 58
Quoted 10 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +10
-0+ +required: + - compatible + - reg + +allOf: + - if: + properties: + compatible: + contains:
This is not obvious from the original bindings. Why are these required only on v1? Whatr about v2? Are they allowed on v2? Why at least clock-cells are not required on v2?
The previous bindings make them required which you change here. Your commit does not explain this.
Please explicitly explain in commit msg all changes to the bindings which are necessary for conversion.
Quoted 20 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +20
-0+ const: qcom,kpss-acc-v1 + then: + required: + - clocks + - clock-names + - clock-output-names + - '#clock-cells' + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,gcc-ipq806x.h> + + clock-controller@2088000 { + compatible = "qcom,kpss-acc-v1"; + reg = <0x02088000 0x1000>, <0x02008000 0x1000>; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; + clock-names = "pll8_vote", "pxo"; + clock-output-names = "acpu0_aux";
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml line 88
Quoted 10 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +10
-0+ #clock-cells = <0>; + }; + + clock-controller@2098000 { + compatible = "qcom,kpss-acc-v1"; + reg = <0x02098000 0x1000>, <0x02008000 0x1000>; + clock-output-names = "acpu1_aux"; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; + clock-names = "pll8_vote", "pxo"; + #clock-cells = <0>;
It's the same example as above, no differences. Remove it.
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml line 95
Quoted 7 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +7
-0+ }; + + - | + clock-controller@f9088000 { + compatible = "qcom,kpss-acc-v2"; + reg = <0xf9088000 0x1000>, + <0xf9008000 0x1000>;
You now changed the example. This looks wrong.
Quoted 66 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +2
-0+ }; +...Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt deleted +0
-44@@ -1,44 +0,0 @@ -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC) - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: should be one of the following. The generic compatible - "qcom,kpss-gcc" should also be included. - "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc" - "qcom,kpss-gcc-apq8064", "qcom,kpss-gcc" - "qcom,kpss-gcc-msm8974", "qcom,kpss-gcc" - "qcom,kpss-gcc-msm8960", "qcom,kpss-gcc" - -- reg: - Usage: required - Value type: <prop-encoded-array> - Definition: base address and size of the register region - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the pll parents. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "pll8_vote", "pxo". - -- clock-output-names: - Usage: required - Value type: <string> - Definition: Name of the output clock. Typically acpu_l2_aux indicating - an L2 cache auxiliary clock. - -Example: - - l2cc: clock-controller@2011000 { - compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"; - reg = <0x2011000 0x1000>; - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>; - clock-names = "pll8_vote", "pxo"; - clock-output-names = "acpu_l2_aux"; - };Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +8
-0@@ -0,0 +1,63 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC) +
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 18
Quoted 10 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +10
-0+maintainers: + - Ansuel Smith <[email protected]> + +description: | + Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used + to control L2 mux (in the current implementation). + +properties: + compatible: + const: qcom,kpss-gcc
This is wrong conversion. You removed quite a lot.
Please split this commit into two separate conversions (acc and gcc).
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 22
Quoted 4 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +4
-0+ + reg: + items: + - description: Base address and size of the register region
Just maxItems:1. No need for items and description - it's obvious.
Quoted 4 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +4
-0+ + clocks: + items: + - description: phandle to pll8_vote
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 36
Quoted 10 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +10
-0+ - description: phandle to pxo_board + + clock-names: + items: + - const: pll8_vote + - const: pxo + + clock-output-names: + const: acpu_l2_aux +
Best regards,
Krzysztof
Re: [PATCH v2 16/16] dt-bindings: arm: msm: Convert kpss driver Documentation to yaml
Quoted 131 lines · Krzysztof Kozlowski
Quoted 130 lines · Ansuel Smith
Convert kpss-acc and kpss-gcc Documentation to yaml. Fix multiple Documentation error and provide additional example for kpss-gcc-v2.
Signed-off-by Ansuel Smith <[email protected]>--- .../bindings/arm/msm/qcom,kpss-acc.txt | 49 ---------- .../bindings/arm/msm/qcom,kpss-acc.yaml | 97 +++++++++++++++++++ .../bindings/arm/msm/qcom,kpss-gcc.txt | 44 --------- .../bindings/arm/msm/qcom,kpss-gcc.yaml | 63 ++++++++++++ 4 files changed, 160 insertions(+), 93 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.yamlDocumentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt deleted +0
-49@@ -1,49 +0,0 @@ -Krait Processor Sub-system (KPSS) Application Clock Controller (ACC) - -The KPSS ACC provides clock, power domain, and reset control to a Krait CPU. -There is one ACC register region per CPU within the KPSS remapped region as -well as an alias register region that remaps accesses to the ACC associated -with the CPU accessing the region. - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: should be one of: - "qcom,kpss-acc-v1" - "qcom,kpss-acc-v2" - -- reg: - Usage: required - Value type: <prop-encoded-array> - Definition: the first element specifies the base address and size of - the register region. An optional second element specifies - the base address and size of the alias register region. - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the pll parents. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "pll8_vote", "pxo". - -- clock-output-names: - Usage: optional - Value type: <string> - Definition: Name of the output clock. Typically acpuX_aux where X is a - CPU number starting at 0. - -Example: - - clock-controller@2088000 { - compatible = "qcom,kpss-acc-v2"; - reg = <0x02088000 0x1000>, - <0x02008000 0x1000>; - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>; - clock-names = "pll8_vote", "pxo"; - clock-output-names = "acpu0_aux"; - };Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +54
-0@@ -0,0 +1,97 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-acc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Krait Processor Sub-system (KPSS) Application Clock Controller (ACC) + +maintainers: + - Ansuel Smith <[email protected]> + +description: | + The KPSS ACC provides clock, power domain, and reset control to a Krait CPU. + There is one ACC register region per CPU within the KPSS remapped region as + well as an alias register region that remaps accesses to the ACC associated + with the CPU accessing the region. + +properties: + compatible: + enum: + - qcom,kpss-acc-v1 + - qcom,kpss-acc-v2 + + reg: + items: + - description: Base address and size of the register region + - description: Optional base address and size of the alias register region + + clocks: + items: + - description: phandle to pll8_vote + - description: phandle to pxo_board + + clock-names: + items: + - const: pll8_vote + - const: pxo + + clock-output-names: + description: Name of the aux clock. Krait can have at most 4 cpu. + enum: + - acpu0_aux + - acpu1_aux + - acpu2_aux + - acpu3_aux + + '#clock-cells': + const: 0 + +required: + - compatible + - reg + +allOf:
Quoted 9 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml line 58
Quoted 4 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +4
-0+ - if: + properties: + compatible: + contains:This is not obvious from the original bindings. Why are these required only on v1? Whatr about v2? Are they allowed on v2? Why at least clock-cells are not required on v2?
The original Documentation was a joke. I should have also write it in the cover letter. v2 doesn't export clocks and just expose the kpss regs. Doesn't provide any mux / clocks. It's really an entirely different implementation. Aka the Documentation was wrong from the start.
Quoted 30 lines
The previous bindings make them required which you change here. Your commit does not explain this.
Please explicitly explain in commit msg all changes to the bindings which are necessary for conversion.
Quoted 24 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +24
-0+ const: qcom,kpss-acc-v1 + then: + required: + - clocks + - clock-names + - clock-output-names + - '#clock-cells' + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,gcc-ipq806x.h> + + clock-controller@2088000 { + compatible = "qcom,kpss-acc-v1"; + reg = <0x02088000 0x1000>, <0x02008000 0x1000>; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; + clock-names = "pll8_vote", "pxo"; + clock-output-names = "acpu0_aux"; + #clock-cells = <0>; + }; + + clock-controller@2098000 {
Quoted 9 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml line 88
Quoted 6 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +6
-0+ compatible = "qcom,kpss-acc-v1"; + reg = <0x02098000 0x1000>, <0x02008000 0x1000>; + clock-output-names = "acpu1_aux"; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; + clock-names = "pll8_vote", "pxo"; + #clock-cells = <0>;It's the same example as above, no differences. Remove it.
Mhh this is a direct example on how the kpss-acc are defined for ipq806x with a configuration with 2 cpu. Tell me if I should really remove it or not.
Quoted 10 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml line 95
Quoted 7 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +7
-0+ }; + + - | + clock-controller@f9088000 { + compatible = "qcom,kpss-acc-v2"; + reg = <0xf9088000 0x1000>, + <0xf9008000 0x1000>;You now changed the example. This looks wrong.
Again the example was wrong from the start. There isn't ANY kpss v2 that exports clock and the driver that expose the aux clocks have just the compatible for v1. (that is correct)
Quoted 70 lines
Quoted 70 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +2
-0+ }; +...Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt deleted +0
-44@@ -1,44 +0,0 @@ -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC) - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: should be one of the following. The generic compatible - "qcom,kpss-gcc" should also be included. - "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc" - "qcom,kpss-gcc-apq8064", "qcom,kpss-gcc" - "qcom,kpss-gcc-msm8974", "qcom,kpss-gcc" - "qcom,kpss-gcc-msm8960", "qcom,kpss-gcc" - -- reg: - Usage: required - Value type: <prop-encoded-array> - Definition: base address and size of the register region - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the pll parents. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "pll8_vote", "pxo". - -- clock-output-names: - Usage: required - Value type: <string> - Definition: Name of the output clock. Typically acpu_l2_aux indicating - an L2 cache auxiliary clock. - -Example: - - l2cc: clock-controller@2011000 { - compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"; - reg = <0x2011000 0x1000>; - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>; - clock-names = "pll8_vote", "pxo"; - clock-output-names = "acpu_l2_aux"; - };Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +12
-0@@ -0,0 +1,63 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC) + +maintainers: + - Ansuel Smith <[email protected]> + +description: |
Quoted 9 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 18
Quoted 6 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +6
-0+ Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used + to control L2 mux (in the current implementation). + +properties: + compatible: + const: qcom,kpss-gccThis is wrong conversion. You removed quite a lot.
The driver doesn't have any support for it and we have no user for kpss-gcc. Again another Documentation that was pushed for the sake of it with no real user/no sense at all. Tell me if I should keep random compatible anyway. Should't be better to fix them for good and make devs add a specific compatible when the driver actually add support for them?
Quoted 2 lines
Please split this commit into two separate conversions (acc and gcc).
Will do.
Quoted 7 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 22
Quoted 4 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +4
-0+ + reg: + items: + - description: Base address and size of the register regionJust maxItems:1. No need for items and description - it's obvious.
Ok.
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +10
-0+ + clocks: + items: + - description: phandle to pll8_vote + - description: phandle to pxo_board + + clock-names: + items: + - const: pll8_vote + - const: pxo
Quoted 8 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 36
Quoted 4 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +4
-0+ + clock-output-names: + const: acpu_l2_aux +Best regards,
Krzysztof
I hope I don't look rude with my response. The Documentation for this stuff is really old and was pushed randomly. Guess how fun it was to discover that NONE of them were actually right and we have this from ages broken. Will for sure improve the commit description.
Also considering how brake they are should I really do a direct conversion and then fix them? Instead of dropping them for good and reimplement them the correct way directly?
Signature · 1 line
Ansuel
Re: [PATCH v2 16/16] dt-bindings: arm: msm: Convert kpss driver Documentation to yaml
Quoted 142 lines · Ansuel Smith
Quoted 131 lines · Krzysztof Kozlowski
Quoted 130 lines · Ansuel Smith
Convert kpss-acc and kpss-gcc Documentation to yaml. Fix multiple Documentation error and provide additional example for kpss-gcc-v2.
Signed-off-by Ansuel Smith <[email protected]>--- .../bindings/arm/msm/qcom,kpss-acc.txt | 49 ---------- .../bindings/arm/msm/qcom,kpss-acc.yaml | 97 +++++++++++++++++++ .../bindings/arm/msm/qcom,kpss-gcc.txt | 44 --------- .../bindings/arm/msm/qcom,kpss-gcc.yaml | 63 ++++++++++++ 4 files changed, 160 insertions(+), 93 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.yamlDocumentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt deleted +0
-49@@ -1,49 +0,0 @@ -Krait Processor Sub-system (KPSS) Application Clock Controller (ACC) - -The KPSS ACC provides clock, power domain, and reset control to a Krait CPU. -There is one ACC register region per CPU within the KPSS remapped region as -well as an alias register region that remaps accesses to the ACC associated -with the CPU accessing the region. - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: should be one of: - "qcom,kpss-acc-v1" - "qcom,kpss-acc-v2" - -- reg: - Usage: required - Value type: <prop-encoded-array> - Definition: the first element specifies the base address and size of - the register region. An optional second element specifies - the base address and size of the alias register region. - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the pll parents. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "pll8_vote", "pxo". - -- clock-output-names: - Usage: optional - Value type: <string> - Definition: Name of the output clock. Typically acpuX_aux where X is a - CPU number starting at 0. - -Example: - - clock-controller@2088000 { - compatible = "qcom,kpss-acc-v2"; - reg = <0x02088000 0x1000>, - <0x02008000 0x1000>; - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>; - clock-names = "pll8_vote", "pxo"; - clock-output-names = "acpu0_aux"; - };Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +54
-0@@ -0,0 +1,97 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-acc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Krait Processor Sub-system (KPSS) Application Clock Controller (ACC) + +maintainers: + - Ansuel Smith <[email protected]> + +description: | + The KPSS ACC provides clock, power domain, and reset control to a Krait CPU. + There is one ACC register region per CPU within the KPSS remapped region as + well as an alias register region that remaps accesses to the ACC associated + with the CPU accessing the region. + +properties: + compatible: + enum: + - qcom,kpss-acc-v1 + - qcom,kpss-acc-v2 + + reg: + items: + - description: Base address and size of the register region + - description: Optional base address and size of the alias register region + + clocks: + items: + - description: phandle to pll8_vote + - description: phandle to pxo_board + + clock-names: + items: + - const: pll8_vote + - const: pxo + + clock-output-names: + description: Name of the aux clock. Krait can have at most 4 cpu. + enum: + - acpu0_aux + - acpu1_aux + - acpu2_aux + - acpu3_aux + + '#clock-cells': + const: 0 + +required: + - compatible + - reg + +allOf:Quoted 9 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml line 58
Quoted 4 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +4
-0+ - if: + properties: + compatible: + contains:This is not obvious from the original bindings. Why are these required only on v1? Whatr about v2? Are they allowed on v2? Why at least clock-cells are not required on v2?
Quoted 5 lines
The original Documentation was a joke. I should have also write it in the cover letter. v2 doesn't export clocks and just expose the kpss regs. Doesn't provide any mux / clocks. It's really an entirely different implementation. Aka the Documentation was wrong from the start.
OK, thanks for explanation. Please mention it in the commit msg.
Quoted 41 lines
Quoted 31 lines
The previous bindings make them required which you change here. Your commit does not explain this.
Please explicitly explain in commit msg all changes to the bindings which are necessary for conversion.
Quoted 24 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +24
-0+ const: qcom,kpss-acc-v1 + then: + required: + - clocks + - clock-names + - clock-output-names + - '#clock-cells' + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,gcc-ipq806x.h> + + clock-controller@2088000 { + compatible = "qcom,kpss-acc-v1"; + reg = <0x02088000 0x1000>, <0x02008000 0x1000>; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; + clock-names = "pll8_vote", "pxo"; + clock-output-names = "acpu0_aux"; + #clock-cells = <0>; + }; + + clock-controller@2098000 {Quoted 9 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml line 88
Quoted 6 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +6
-0+ compatible = "qcom,kpss-acc-v1"; + reg = <0x02098000 0x1000>, <0x02008000 0x1000>; + clock-output-names = "acpu1_aux"; + clocks = <&gcc PLL8_VOTE>, <&pxo_board>; + clock-names = "pll8_vote", "pxo"; + #clock-cells = <0>;It's the same example as above, no differences. Remove it.
Quoted 3 lines
Mhh this is a direct example on how the kpss-acc are defined for ipq806x with a configuration with 2 cpu. Tell me if I should really remove it or not.
Yeah, but all properties are exactly the same, there is no difference here, so this as an example does not bring additional information.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +7
-0+ }; + + - | + clock-controller@f9088000 { + compatible = "qcom,kpss-acc-v2"; + reg = <0xf9088000 0x1000>, + <0xf9008000 0x1000>;
Quoted 6 lines
Quoted 2 lines
You now changed the example. This looks wrong.
Again the example was wrong from the start. There isn't ANY kpss v2 that exports clock and the driver that expose the aux clocks have just the compatible for v1. (that is correct)
OK, please also mention it in commit msg.
Quoted 81 lines
Quoted 71 lines
Quoted 71 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml new file +2
-0+ }; +...Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt deleted +0
-44@@ -1,44 +0,0 @@ -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC) - -PROPERTIES - -- compatible: - Usage: required - Value type: <string> - Definition: should be one of the following. The generic compatible - "qcom,kpss-gcc" should also be included. - "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc" - "qcom,kpss-gcc-apq8064", "qcom,kpss-gcc" - "qcom,kpss-gcc-msm8974", "qcom,kpss-gcc" - "qcom,kpss-gcc-msm8960", "qcom,kpss-gcc" - -- reg: - Usage: required - Value type: <prop-encoded-array> - Definition: base address and size of the register region - -- clocks: - Usage: required - Value type: <prop-encoded-array> - Definition: reference to the pll parents. - -- clock-names: - Usage: required - Value type: <stringlist> - Definition: must be "pll8_vote", "pxo". - -- clock-output-names: - Usage: required - Value type: <string> - Definition: Name of the output clock. Typically acpu_l2_aux indicating - an L2 cache auxiliary clock. - -Example: - - l2cc: clock-controller@2011000 { - compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"; - reg = <0x2011000 0x1000>; - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>; - clock-names = "pll8_vote", "pxo"; - clock-output-names = "acpu_l2_aux"; - };Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +12
-0@@ -0,0 +1,63 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC) + +maintainers: + - Ansuel Smith <[email protected]> + +description: |Quoted 9 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 18
Quoted 6 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +6
-0+ Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used + to control L2 mux (in the current implementation). + +properties: + compatible: + const: qcom,kpss-gccThis is wrong conversion. You removed quite a lot.
Quoted 5 lines
The driver doesn't have any support for it and we have no user for kpss-gcc. Again another Documentation that was pushed for the sake of it with no real user/no sense at all. Tell me if I should keep random compatible anyway. Should't be better to fix them for good and make devs add a specific compatible when the driver actually add support for them?
We talk here mostly about bindings, not driver, so the driver's implementation matters less. IOW, driver does not have to implement all compatibles from bindings. Bindings can be used in other projects, outside of mainline kernel, so the compatibles actually might be useful.
These compatibles might be reasonable even if not implemented in the driver, without real user.
More important question is whether these compatibles make sense. That part I don't exactly now. You used term "random compatible", so are they really random? To me they look like specific implementation of GCC, so they look reasonable, but I know nothing about Qualcomm Krait and GCC.
Quoted 35 lines
Quoted 3 lines
Please split this commit into two separate conversions (acc and gcc).
Will do.
Quoted 7 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 22
Quoted 4 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +4
-0+ + reg: + items: + - description: Base address and size of the register regionJust maxItems:1. No need for items and description - it's obvious.
Ok.
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +10
-0+ + clocks: + items: + - description: phandle to pll8_vote + - description: phandle to pxo_board + + clock-names: + items: + - const: pll8_vote + - const: pxoQuoted 8 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 36
Quoted 4 lines
Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +4
-0+ + clock-output-names: + const: acpu_l2_aux +Best regards,
Krzysztof
Quoted 8 lines
I hope I don't look rude with my response. The Documentation for this stuff is really old and was pushed randomly. Guess how fun it was to discover that NONE of them were actually right and we have this from ages broken. Will for sure improve the commit description.
Also considering how brake they are should I really do a direct conversion and then fix them? Instead of dropping them for good and reimplement them the correct way directly?
It depends. If the bindings are really wrong, usually we combine the changes necessary for conversion in one commit. Tweaking compatibles to real life or adding new compatibles - maybe better in a separate commit.
Best regards,
Krzysztof