There are a couple problems with clk parent handling in the common clk
framework. This patch series combines a few different topics together as
all this code is closely related.
First off, we don't do well at determining parents of clks at clk
registration time because the return type of the clk_ops::get_parent()
op is a u8 which isn't expressive enough to cover all our use-cases.
Secondly, we use strings for all parent-child linkages, and this leads
to poorly written code that extracts clk names from struct clk pointers
and makes clk provider drivers use clk consumer APIs.
Thirdly, clkdev.c has a collection of DT parsing logic in it that is
only used when the common clk framework is present but we want to use
that same logic for describing parent-child linkages of clk providers
via in DT. This should all be moved into the common clk framework and
used from there as well as from clkdev.c, so this series changes the way
clkdev interacts with the clk framework by having clkdev get clk_hw
pointers out of DT clk specifiers and then convert those into clk
pointers with clk_hw_create_clk(). Splitting the API this way lets us
get clk_hw pointers for clk providers and skip the struct clk pointer
creation phase that we don't need to do when describing parent-child
linkages.
And finally, we have a few patches in here that lay the groundwork for
supporting device links in the common clk framework. We do that by
pushing the consuming device pointer through to the clk pointer creation
in clk_hw_create_clk(). This wasn't always easy to do when we had
__clk_create_clk() called from multiple places, some being deep in the
clk registration path. This series simplifies that logic so that we can
always attach a consumer device to a clk that we create in one place,
instead of making that linkage in multiple places near where we create
struct clk pointers.
Miquel Raynal (1):
clk: core: clarify the check for runtime PM
Stephen Boyd (8):
clk: Combine __clk_get() and __clk_create_clk()
clk: Introduce get_parent_hw clk op
clk: Introduce of_clk_get_hw_from_clkspec()
clk: Inform the core about consumer devices
clk: Move of_clk_*() APIs into clk.c from clkdev.c
clk: Allow parents to be specified without string names
clk: qcom: gcc-sdm845: Migrate to DT parent mapping
arm64: dts: qcom: Specify XO clk as input to GCC node
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +
drivers/clk/clk.c | 584 ++++++++++++++++++++-------
drivers/clk/clk.h | 23 +-
drivers/clk/clkdev.c | 120 +-----
drivers/clk/qcom/gcc-sdm845.c | 180 ++++-----
include/linux/clk-provider.h | 26 +-
6 files changed, 583 insertions(+), 352 deletions(-)
base-commit: 651022382c7f8da46cb4872a545ee1da6d097d2a
--
Sent by a computer through tubes
TODO: Fully convert driver
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/qcom/gcc-sdm845.c | 180 +++++++++++++++++-----------------
1 file changed, 89 insertions(+), 91 deletions(-)
diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
index f133b7f5652f..9da9a337238d 100644
--- a/drivers/clk/qcom/gcc-sdm845.c
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -25,6 +25,59 @@
#include "gdsc.h"
#include "reset.h"
+static struct clk_alpha_pll gpll0 = {
+ .offset = 0x0,
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+ .clkr = {
+ .enable_reg = 0x52000,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gpll0",
+ .parent_names = (const char *[]){ "bi_tcxo" },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_fixed_fabia_ops,
+ },
+ },
+};
+
+static struct clk_alpha_pll gpll4 = {
+ .offset = 0x76000,
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+ .clkr = {
+ .enable_reg = 0x52000,
+ .enable_mask = BIT(4),
+ .hw.init = &(struct clk_init_data){
+ .name = "gpll4",
+ .parent_names = (const char *[]){ "bi_tcxo" },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_fixed_fabia_ops,
+ },
+ },
+};
+
+static const struct clk_div_table post_div_table_fabia_even[] = {
+ { 0x0, 1 },
+ { 0x1, 2 },
+ { 0x3, 4 },
+ { 0x7, 8 },
+ { }
+};
+
+static struct clk_alpha_pll_postdiv gpll0_out_even = {
+ .offset = 0x0,
+ .post_div_shift = 8,
+ .post_div_table = post_div_table_fabia_even,
+ .num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
+ .width = 4,
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+ .clkr.hw.init = &(struct clk_init_data){
+ .name = "gpll0_out_even",
+ .parent_names = (const char *[]){ "gpll0" },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_postdiv_fabia_ops,
+ },
+};
+
enum {
P_BI_TCXO,
P_AUD_REF_CLK,
@@ -42,11 +95,11 @@ static const struct parent_map gcc_parent_map_0[] = {
{ P_CORE_BI_PLL_TEST_SE, 7 },
};
-static const char * const gcc_parent_names_0[] = {
- "bi_tcxo",
- "gpll0",
- "gpll0_out_even",
- "core_bi_pll_test_se",
+static const struct clk_parent_data gcc_parent_data_0[] = {
+ { .name = "xo", .fallback = "bi_tcxo" },
+ { .hw = &gpll0.clkr.hw },
+ { .hw = &gpll0_out_even.clkr.hw },
+ { .name = "core_bi_pll_test_se", .fallback = "core_bi_pll_test_se" }
};
static const struct parent_map gcc_parent_map_1[] = {
@@ -144,59 +197,6 @@ static const char * const gcc_parent_names_10[] = {
"core_bi_pll_test_se",
};
-static struct clk_alpha_pll gpll0 = {
- .offset = 0x0,
- .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
- .clkr = {
- .enable_reg = 0x52000,
- .enable_mask = BIT(0),
- .hw.init = &(struct clk_init_data){
- .name = "gpll0",
- .parent_names = (const char *[]){ "bi_tcxo" },
- .num_parents = 1,
- .ops = &clk_alpha_pll_fixed_fabia_ops,
- },
- },
-};
-
-static struct clk_alpha_pll gpll4 = {
- .offset = 0x76000,
- .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
- .clkr = {
- .enable_reg = 0x52000,
- .enable_mask = BIT(4),
- .hw.init = &(struct clk_init_data){
- .name = "gpll4",
- .parent_names = (const char *[]){ "bi_tcxo" },
- .num_parents = 1,
- .ops = &clk_alpha_pll_fixed_fabia_ops,
- },
- },
-};
-
-static const struct clk_div_table post_div_table_fabia_even[] = {
- { 0x0, 1 },
- { 0x1, 2 },
- { 0x3, 4 },
- { 0x7, 8 },
- { }
-};
-
-static struct clk_alpha_pll_postdiv gpll0_out_even = {
- .offset = 0x0,
- .post_div_shift = 8,
- .post_div_table = post_div_table_fabia_even,
- .num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
- .width = 4,
- .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
- .clkr.hw.init = &(struct clk_init_data){
- .name = "gpll0_out_even",
- .parent_names = (const char *[]){ "gpll0" },
- .num_parents = 1,
- .ops = &clk_alpha_pll_postdiv_fabia_ops,
- },
-};
-
static const struct freq_tbl ftbl_gcc_cpuss_ahb_clk_src[] = {
F(19200000, P_BI_TCXO, 1, 0, 0),
{ }
@@ -334,7 +334,7 @@ static struct clk_rcg2 gcc_pcie_phy_refgen_clk_src = {
.freq_tbl = ftbl_gcc_pcie_phy_refgen_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_pcie_phy_refgen_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -356,7 +356,7 @@ static struct clk_rcg2 gcc_qspi_core_clk_src = {
.freq_tbl = ftbl_gcc_qspi_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_qspi_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_floor_ops,
},
@@ -377,7 +377,7 @@ static struct clk_rcg2 gcc_pdm2_clk_src = {
.freq_tbl = ftbl_gcc_pdm2_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_pdm2_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -404,7 +404,7 @@ static const struct freq_tbl ftbl_gcc_qupv3_wrap0_s0_clk_src[] = {
static struct clk_init_data gcc_qupv3_wrap0_s0_clk_init = {
.name = "gcc_qupv3_wrap0_s0_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -420,7 +420,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s0_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s1_clk_init = {
.name = "gcc_qupv3_wrap0_s1_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -436,7 +436,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s1_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s2_clk_init = {
.name = "gcc_qupv3_wrap0_s2_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -452,7 +452,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s2_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s3_clk_init = {
.name = "gcc_qupv3_wrap0_s3_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -468,7 +468,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s3_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s4_clk_init = {
.name = "gcc_qupv3_wrap0_s4_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -484,7 +484,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s4_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s5_clk_init = {
.name = "gcc_qupv3_wrap0_s5_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -500,7 +500,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s5_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s6_clk_init = {
.name = "gcc_qupv3_wrap0_s6_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -516,7 +516,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s6_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s7_clk_init = {
.name = "gcc_qupv3_wrap0_s7_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -532,7 +532,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s7_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s0_clk_init = {
.name = "gcc_qupv3_wrap1_s0_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -548,7 +548,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s0_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s1_clk_init = {
.name = "gcc_qupv3_wrap1_s1_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -564,7 +564,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s1_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s2_clk_init = {
.name = "gcc_qupv3_wrap1_s2_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -580,7 +580,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s2_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s3_clk_init = {
.name = "gcc_qupv3_wrap1_s3_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -596,7 +596,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s3_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s4_clk_init = {
.name = "gcc_qupv3_wrap1_s4_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -612,7 +612,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s4_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s5_clk_init = {
.name = "gcc_qupv3_wrap1_s5_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -628,7 +628,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s5_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s6_clk_init = {
.name = "gcc_qupv3_wrap1_s6_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -644,7 +644,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s6_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s7_clk_init = {
.name = "gcc_qupv3_wrap1_s7_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -701,7 +701,7 @@ static struct clk_rcg2 gcc_sdcc4_apps_clk_src = {
.freq_tbl = ftbl_gcc_sdcc4_apps_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_sdcc4_apps_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -743,7 +743,7 @@ static struct clk_rcg2 gcc_ufs_card_axi_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_axi_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_card_axi_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -765,7 +765,7 @@ static struct clk_rcg2 gcc_ufs_card_ice_core_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_ice_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_card_ice_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -800,7 +800,7 @@ static struct clk_rcg2 gcc_ufs_card_unipro_core_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_unipro_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_card_unipro_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -823,7 +823,7 @@ static struct clk_rcg2 gcc_ufs_phy_axi_clk_src = {
.freq_tbl = ftbl_gcc_ufs_phy_axi_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_phy_axi_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -837,7 +837,7 @@ static struct clk_rcg2 gcc_ufs_phy_ice_core_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_ice_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_phy_ice_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -865,7 +865,7 @@ static struct clk_rcg2 gcc_ufs_phy_unipro_core_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_unipro_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_phy_unipro_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -888,7 +888,7 @@ static struct clk_rcg2 gcc_usb30_prim_master_clk_src = {
.freq_tbl = ftbl_gcc_usb30_prim_master_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_usb30_prim_master_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -910,7 +910,7 @@ static struct clk_rcg2 gcc_usb30_prim_mock_utmi_clk_src = {
.freq_tbl = ftbl_gcc_usb30_prim_mock_utmi_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_usb30_prim_mock_utmi_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -924,7 +924,7 @@ static struct clk_rcg2 gcc_usb30_sec_master_clk_src = {
.freq_tbl = ftbl_gcc_usb30_prim_master_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_usb30_sec_master_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -938,7 +938,7 @@ static struct clk_rcg2 gcc_usb30_sec_mock_utmi_clk_src = {
.freq_tbl = ftbl_gcc_usb30_prim_mock_utmi_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_usb30_sec_mock_utmi_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -1030,9 +1030,7 @@ static struct clk_branch gcc_aggre_ufs_card_axi_clk = {
.enable_mask = BIT(0),
.hw.init = &(struct clk_init_data){
.name = "gcc_aggre_ufs_card_axi_clk",
- .parent_names = (const char *[]){
- "gcc_ufs_card_axi_clk_src",
- },
+ .parent_hws = (struct clk_hw *[]){ &gcc_ufs_card_axi_clk_src.clkr.hw },
.num_parents = 1,
.flags = CLK_SET_RATE_PARENT,
.ops = &clk_branch2_ops,
--
Sent by a computer through tubes
The API between clk.c and clkdev.c is purely getting the clk_hw
structure (or the struct clk if it's not CCF) and then turning that
struct clk_hw pointer into a struct clk pointer via clk_hw_create_clk().
There's no need to complicate clkdev.c with these DT parsing details
that are only relevant to the common clk framework. Move the DT parsing
logic into the core framework and just expose the APIs to get a clk_hw
pointer and convert it.
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk.c | 57 ++++++++++++++++++++++++++++++++++++++---
drivers/clk/clk.h | 11 +++++---
drivers/clk/clkdev.c | 60 --------------------------------------------
3 files changed, 62 insertions(+), 66 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 73f8a287bf42..202902e64799 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4080,8 +4080,8 @@ void devm_of_clk_del_provider(struct device *dev)
}
EXPORT_SYMBOL(devm_of_clk_del_provider);
-int of_parse_clkspec(const struct device_node *np, int index, const char *name,
- struct of_phandle_args *out_args)
+static int of_parse_clkspec(const struct device_node *np, int index,
+ const char *name, struct of_phandle_args *out_args)
{
int ret = -ENOENT;
@@ -4131,7 +4131,8 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
return __clk_get_hw(clk);
}
-struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
+static struct clk_hw *
+of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
{
struct of_clk_provider *provider;
struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
@@ -4168,6 +4169,56 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
}
EXPORT_SYMBOL_GPL(of_clk_get_from_provider);
+struct clk_hw *of_clk_get_hw(struct device_node *np, int index,
+ const char *con_id)
+{
+ int ret;
+ struct clk_hw *hw;
+ struct of_phandle_args clkspec;
+
+ ret = of_parse_clkspec(np, index, con_id, &clkspec);
+ if (ret)
+ return ERR_PTR(ret);
+
+ hw = of_clk_get_hw_from_clkspec(&clkspec);
+ of_node_put(clkspec.np);
+
+ return hw;
+}
+
+static struct clk *__of_clk_get(struct device_node *np,
+ int index, const char *dev_id,
+ const char *con_id)
+{
+ struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
+
+ return clk_hw_create_clk(NULL, hw, dev_id, con_id);
+}
+
+struct clk *of_clk_get(struct device_node *np, int index)
+{
+ return __of_clk_get(np, index, np->full_name, NULL);
+}
+EXPORT_SYMBOL(of_clk_get);
+
+/**
+ * of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
+ * @np: pointer to clock consumer node
+ * @name: name of consumer's clock input, or NULL for the first clock reference
+ *
+ * This function parses the clocks and clock-names properties,
+ * and uses them to look up the struct clk from the registered list of clock
+ * providers.
+ */
+struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
+{
+ if (!np)
+ return ERR_PTR(-ENOENT);
+
+ return __of_clk_get(np, -1, np->full_name, name);
+}
+EXPORT_SYMBOL(of_clk_get_by_name);
+
/**
* of_clk_get_parent_count() - Count the number of clocks a device node has
* @np: device node to count
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index fda0ad483416..46e668d64da8 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -14,9 +14,14 @@ struct device;
struct of_phandle_args;
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-int of_parse_clkspec(const struct device_node *np, int index, const char *name,
- struct of_phandle_args *out_args);
-struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec);
+struct clk_hw *of_clk_get_hw(struct device_node *np,
+ int index, const char *con_id);
+#else /* !CONFIG_COMMON_CLK || !CONFIG_OF */
+static inline struct clk_hw *of_clk_get_hw(struct device_node *np,
+ int index, const char *con_id)
+{
+ return ERR_PTR(-ENOENT);
+}
#endif
#ifdef CONFIG_COMMON_CLK
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index f2f4f2afd28c..d3758bf4305c 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -27,66 +27,6 @@
static LIST_HEAD(clocks);
static DEFINE_MUTEX(clocks_mutex);
-#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-static struct clk_hw *of_clk_get_hw(struct device_node *np,
- int index, const char *con_id)
-{
- int ret;
- struct clk_hw *hw;
- struct of_phandle_args clkspec;
-
- ret = of_parse_clkspec(np, index, con_id, &clkspec);
- if (ret)
- return ERR_PTR(ret);
-
- hw = of_clk_get_hw_from_clkspec(&clkspec);
- of_node_put(clkspec.np);
-
- return hw;
-}
-
-static struct clk *__of_clk_get(struct device_node *np,
- int index, const char *dev_id,
- const char *con_id)
-{
- struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
-
- return clk_hw_create_clk(NULL, hw, dev_id, con_id);
-}
-
-struct clk *of_clk_get(struct device_node *np, int index)
-{
- return __of_clk_get(np, index, np->full_name, NULL);
-}
-EXPORT_SYMBOL(of_clk_get);
-
-/**
- * of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
- * @np: pointer to clock consumer node
- * @name: name of consumer's clock input, or NULL for the first clock reference
- *
- * This function parses the clocks and clock-names properties,
- * and uses them to look up the struct clk from the registered list of clock
- * providers.
- */
-struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
-{
- if (!np)
- return ERR_PTR(-ENOENT);
-
- return __of_clk_get(np, -1, np->full_name, name);
-}
-EXPORT_SYMBOL(of_clk_get_by_name);
-
-#else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
-
-static struct clk_hw *of_clk_get_hw(struct device_node *np,
- int index, const char *con_id)
-{
- return ERR_PTR(-ENOENT);
-}
-#endif
-
/*
* Find the correct struct clk for the device and connection ID.
* We do slightly fuzzy matching here:
--
Sent by a computer through tubes
The common clk framework is lacking in ability to describe the clk
topology without specifying strings for every possible parent-child
link. There are a few drawbacks to the current approach:
1) String comparisons are used for everything, including describing
topologies that are 'local' to a single clock controller.
2) clk providers (e.g. i2c clk drivers) need to create globally unique
clk names to avoid collisions in the clk namespace, leading to awkward
name generation code in various clk drivers.
3) DT bindings may not fully describe the clk topology and linkages
between clk controllers because drivers can easily rely on globally unique
strings to describe connections between clks.
This leads to confusing DT bindings, complicated clk name generation
code, and inefficient string comparisons during clk registration just so
that the clk framework can detect the topology of the clk tree.
Furthermore, some drivers call clk_get() and then __clk_get_name() to
extract the globally unique clk name just so they can specify the parent
of the clk they're registering. We have of_clk_parent_fill() but that
mostly only works for single clks registered from a DT node, which isn't
the norm. Let's simplify this all by introducing two new ways of
specifying clk parents.
The first method is an array of pointers to clk_hw structures
corresponding to the parents at that index. This works for clks that are
registered when we have access to all the clk_hw pointers for the
parents.
The second method is a mix of clk_hw pointers and strings of local and
global parent clk names. If the .name member of the map is set we'll
look for that clk by performing a DT based lookup of the device the clk
is registered with and the .name specified in the map. If that fails,
we'll fallback to the .fallback member and perform a global clk name
lookup like we've always done before.
Using either one of these new methods is entirely optional. Existing
drivers will continue to work, and they can migrate to this new approach
as they see fit. Eventually, we'll want to get rid of the 'parent_names'
array in struct clk_init_data and use one of these new methods instead.
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk.c | 215 +++++++++++++++++++++++++----------
include/linux/clk-provider.h | 17 ++-
2 files changed, 173 insertions(+), 59 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 202902e64799..0cd90a2339ad 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -42,6 +42,13 @@ static LIST_HEAD(clk_notifier_list);
/*** private data structures ***/
+struct clk_parent_map {
+ struct clk_hw *hw;
+ struct clk_core *core;
+ const char *name;
+ const char *fallback;
+};
+
struct clk_core {
const char *name;
const struct clk_ops *ops;
@@ -49,8 +56,7 @@ struct clk_core {
struct module *owner;
struct device *dev;
struct clk_core *parent;
- const char **parent_names;
- struct clk_core **parents;
+ struct clk_parent_map *parents;
u8 num_parents;
u8 new_parent_index;
unsigned long rate;
@@ -319,17 +325,77 @@ static struct clk_core *clk_core_lookup(const char *name)
return NULL;
}
+/**
+ * clk_core_get - Find the parent of a clk using a clock specifier in DT
+ * @core: clk to find parent of
+ * @name: name to search for in 'clock-names' of device providing clk
+ *
+ * This is the preferred method for clk providers to find the parent of a
+ * clk when that parent is external to the clk controller. The parent_names
+ * array is indexed and treated as a local name matching a string in the device
+ * node's 'clock-names' property. This allows clk providers to use their own
+ * namespace instead of looking for a globally unique parent string.
+ *
+ * For example the following DT snippet would allow a clock registered by the
+ * clock-controller@c001 that has a clk_init_data::parent_data array
+ * with 'xtal' in the 'name' member to find the clock provided by the
+ * clock-controller@f00abcd without needing to get the globally unique name of
+ * the xtal clk.
+ *
+ * parent: clock-controller@f00abcd {
+ * reg = <0xf00abcd 0xabcd>;
+ * #clock-cells = <0>;
+ * };
+ *
+ * clock-controller@c001 {
+ * reg = <0xc001 0xf00d>;
+ * clocks = <&parent>;
+ * clock-names = "xtal";
+ * #clock-cells = <1>;
+ * };
+ */
+static struct clk_core *clk_core_get(struct clk_core *core, const char *name)
+{
+ struct clk_hw *hw;
+ struct device *dev = core->dev;
+
+ if (!dev)
+ return NULL;
+
+ /* TODO: Support clkdev clk_lookups */
+ hw = of_clk_get_hw(dev->of_node, -1, name);
+ if (IS_ERR_OR_NULL(hw))
+ return NULL;
+
+ return hw->core;
+}
+
+static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
+{
+ struct clk_parent_map *entry = &core->parents[index];
+ struct clk_core *parent = NULL;
+
+ if (entry->hw)
+ parent = entry->hw->core;
+ else if (entry->name)
+ parent = clk_core_get(core, entry->name);
+
+ if (!parent)
+ parent = clk_core_lookup(entry->fallback);
+
+ entry->core = parent;
+}
+
static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
u8 index)
{
- if (!core || index >= core->num_parents)
+ if (!core || index >= core->num_parents || !core->parents)
return NULL;
- if (!core->parents[index])
- core->parents[index] =
- clk_core_lookup(core->parent_names[index]);
+ if (!core->parents[index].core)
+ clk_core_fill_parent_index(core, index);
- return core->parents[index];
+ return core->parents[index].core;
}
struct clk_hw *
@@ -2353,6 +2419,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
bool clk_has_parent(struct clk *clk, struct clk *parent)
{
struct clk_core *core, *parent_core;
+ int i;
/* NULL clocks should be nops, so return success if either is NULL. */
if (!clk || !parent)
@@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
if (core->parent == parent_core)
return true;
- return match_string(core->parent_names, core->num_parents,
- parent_core->name) >= 0;
+ for (i = 0; i < core->num_parents; i++)
+ if (!strcmp(core->parents[i].fallback, parent_core->name))
+ return true;
+
+ return false;
}
EXPORT_SYMBOL_GPL(clk_has_parent);
@@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file *s, void *data)
int i;
for (i = 0; i < core->num_parents - 1; i++)
- seq_printf(s, "%s ", core->parent_names[i]);
+ seq_printf(s, "%s ", core->parents[i].fallback);
- seq_printf(s, "%s\n", core->parent_names[i]);
+ seq_printf(s, "%s\n", core->parents[i].fallback);
return 0;
}
@@ -3085,7 +3155,7 @@ static inline void clk_debug_unregister(struct clk_core *core)
*/
static int __clk_core_init(struct clk_core *core)
{
- int i, ret;
+ int ret;
struct clk_core *orphan;
struct hlist_node *tmp2;
unsigned long rate;
@@ -3139,12 +3209,6 @@ static int __clk_core_init(struct clk_core *core)
goto out;
}
- /* throw a WARN if any entries in parent_names are NULL */
- for (i = 0; i < core->num_parents; i++)
- WARN(!core->parent_names[i],
- "%s: invalid NULL in %s's .parent_names\n",
- __func__, core->name);
-
ret = clk_init_parent(core);
if (ret)
goto out;
@@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
return clk;
}
+static int clk_core_populate_parent_map(struct clk_core *core)
+{
+ const struct clk_init_data *init = core->hw->init;
+ u8 num_parents = init->num_parents;
+ const char * const *parent_names = init->parent_names;
+ struct clk_hw **parent_hws = init->parent_hws;
+ const struct clk_parent_data *parent_data = init->parent_data;
+ int i, ret = 0;
+ struct clk_parent_map *parents, *parent;
+
+ if (!num_parents)
+ return 0;
+
+ /*
+ * Avoid unnecessary string look-ups of clk_core's possible parents by
+ * having a cache of names/clk_hw pointers to clk_core pointers.
+ */
+ parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
+ core->parents = parents;
+ if (!parents)
+ return -ENOMEM;
+
+ /* Copy everything over because it might be __initdata */
+ for (i = 0, parent = parents; i < num_parents; i++, parent++) {
+ if (parent_names) {
+ /* throw a WARN if any entries are NULL */
+ WARN(!parent_names[i],
+ "%s: invalid NULL in %s's .parent_names\n",
+ __func__, core->name);
+ parent->fallback = kstrdup_const(parent_names[i],
+ GFP_KERNEL);
+ if (!parent->fallback) {
+ ret = -ENOMEM;
+ while (--i >= 0)
+ kfree_const(parents[i].fallback);
+ }
+ } else if (parent_data) {
+ parent->hw = parent_data[i].hw;
+ parent->name = parent_data[i].name;
+ parent->fallback = parent_data[i].fallback;
+ } else if (parent_hws) {
+ parent->hw = parent_hws[i];
+ } else {
+ ret = -EINVAL;
+ WARN(1, "Must specify parents if num_parents > 0\n");
+ }
+
+ if (ret) {
+ kfree(parents);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void clk_core_free_parent_map(struct clk_core *core)
+{
+ int i = core->num_parents;
+
+ if (!core->num_parents)
+ return;
+
+ while (--i >= 0)
+ kfree_const(core->parents[i].fallback);
+ kfree(core->parents);
+}
+
/**
* clk_register - allocate a new clock, register it and return an opaque cookie
* @dev: device that is registering this clock
@@ -3373,7 +3505,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
*/
struct clk *clk_register(struct device *dev, struct clk_hw *hw)
{
- int i, ret;
+ int ret;
struct clk_core *core;
core = kzalloc(sizeof(*core), GFP_KERNEL);
@@ -3406,33 +3538,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
core->max_rate = ULONG_MAX;
hw->core = core;
- /* allocate local copy in case parent_names is __initdata */
- core->parent_names = kcalloc(core->num_parents, sizeof(char *),
- GFP_KERNEL);
-
- if (!core->parent_names) {
- ret = -ENOMEM;
- goto fail_parent_names;
- }
-
-
- /* copy each string name in case parent_names is __initdata */
- for (i = 0; i < core->num_parents; i++) {
- core->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
- GFP_KERNEL);
- if (!core->parent_names[i]) {
- ret = -ENOMEM;
- goto fail_parent_names_copy;
- }
- }
-
- /* avoid unnecessary string look-ups of clk_core's possible parents. */
- core->parents = kcalloc(core->num_parents, sizeof(*core->parents),
- GFP_KERNEL);
- if (!core->parents) {
- ret = -ENOMEM;
+ ret = clk_core_populate_parent_map(core);
+ if (ret)
goto fail_parents;
- };
INIT_HLIST_HEAD(&core->clks);
@@ -3443,7 +3551,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
hw->clk = alloc_clk(core, NULL, NULL);
if (IS_ERR(hw->clk)) {
ret = PTR_ERR(hw->clk);
- goto fail_parents;
+ goto fail_create_clk;
}
clk_core_link_consumer(hw->core, hw->clk);
@@ -3459,13 +3567,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
free_clk(hw->clk);
hw->clk = NULL;
+fail_create_clk:
+ clk_core_free_parent_map(core);
fail_parents:
- kfree(core->parents);
-fail_parent_names_copy:
- while (--i >= 0)
- kfree_const(core->parent_names[i]);
- kfree(core->parent_names);
-fail_parent_names:
fail_ops:
kfree_const(core->name);
fail_name:
@@ -3495,15 +3599,10 @@ EXPORT_SYMBOL_GPL(clk_hw_register);
static void __clk_release(struct kref *ref)
{
struct clk_core *core = container_of(ref, struct clk_core, ref);
- int i = core->num_parents;
lockdep_assert_held(&prepare_lock);
- kfree(core->parents);
- while (--i >= 0)
- kfree_const(core->parent_names[i]);
-
- kfree(core->parent_names);
+ clk_core_free_parent_map(core);
kfree_const(core->name);
kfree(core);
}
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 8b84dee942bf..f513f4074a93 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -264,6 +264,18 @@ struct clk_ops {
void (*debug_init)(struct clk_hw *hw, struct dentry *dentry);
};
+/**
+ * struct clk_parent_data - clk parent information
+ * @hw: parent clk_hw pointer (used for clk providers with internal clks)
+ * @name: parent name local to provider registering clk
+ * @fallback: globally unique parent name (used as a fallback)
+ */
+struct clk_parent_data {
+ struct clk_hw *hw;
+ const char *name;
+ const char *fallback;
+};
+
/**
* struct clk_init_data - holds init data that's common to all clocks and is
* shared between the clock provider and the common clock framework.
@@ -277,7 +289,10 @@ struct clk_ops {
struct clk_init_data {
const char *name;
const struct clk_ops *ops;
- const char * const *parent_names;
+ /* Only one of the following three should be assigned */
+ const char * const *parent_names; /* If NULL (and num_parents != 0) look at parent_data and parent_hws */
+ const struct clk_parent_data *parent_data;
+ struct clk_hw **parent_hws;
u8 num_parents;
unsigned long flags;
};
--
Sent by a computer through tubes
This is an example patch to show how DT nodes are updated to specify the
clocks which are external to the clock controller can be specified in DT
and then found by the clk core to do parent-child linkage.
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index b72bdb0a31a5..46e0b467b411 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -338,6 +338,8 @@
#clock-cells = <1>;
#reset-cells = <1>;
#power-domain-cells = <1>;
+ clocks = <&rpmhcc RPMH_CXO_CLK>;
+ clock-names = "xo";
};
qfprom@784000 {
--
Sent by a computer through tubes
We want to get struct clk_hw pointers from a DT clk specifier (i.e. a
clocks property) so that we can find parent clks without searching for
globally unique clk names. This should save time by avoiding the global
string search for clks that are external to the clock controller
providing the clk and let us move away from string comparisons in
general.
Introduce of_clk_get_hw_from_clkspec() which is largely the DT parsing
part of finding clks implemented in clkdev.c and have that return a
clk_hw pointer instead of converting that into a clk pointer. This lets
us push up the clk pointer creation to the caller in clk_get() and
avoids the need to push the dev_id and con_id throughout the DT parsing
code.
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk.c | 46 +++++++++++++++++++++---
drivers/clk/clk.h | 5 +--
drivers/clk/clkdev.c | 83 +++++++++++++-------------------------------
3 files changed, 69 insertions(+), 65 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 01cdb9ae03fa..d4c4cab42375 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4077,6 +4077,42 @@ void devm_of_clk_del_provider(struct device *dev)
}
EXPORT_SYMBOL(devm_of_clk_del_provider);
+int of_parse_clkspec(const struct device_node *np, int index, const char *name,
+ struct of_phandle_args *out_args)
+{
+ int ret = -ENOENT;
+
+ /* Walk up the tree of devices looking for a clock property that matches */
+ while (np) {
+ /*
+ * For named clocks, first look up the name in the
+ * "clock-names" property. If it cannot be found, then index
+ * will be an error code and of_parse_phandle_with_args() will
+ * return -EINVAL.
+ */
+ if (name)
+ index = of_property_match_string(np, "clock-names", name);
+ ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
+ index, out_args);
+ if (!ret)
+ break;
+ if (name && index >= 0)
+ break;
+
+ /*
+ * No matching clock found on this node. If the parent node
+ * has a "clock-ranges" property, then we can try one of its
+ * clocks.
+ */
+ np = np->parent;
+ if (np && !of_get_property(np, "clock-ranges", NULL))
+ break;
+ index = 0;
+ }
+
+ return ret;
+}
+
static struct clk_hw *
__of_clk_get_hw_from_provider(struct of_clk_provider *provider,
struct of_phandle_args *clkspec)
@@ -4092,8 +4128,7 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
return __clk_get_hw(clk);
}
-struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
- const char *dev_id, const char *con_id)
+struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
{
struct of_clk_provider *provider;
struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
@@ -4101,7 +4136,6 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
if (!clkspec)
return ERR_PTR(-EINVAL);
- /* Check if we have such a provider in our array */
mutex_lock(&of_clk_mutex);
list_for_each_entry(provider, &of_clk_providers, link) {
if (provider->node == clkspec->np) {
@@ -4112,7 +4146,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
}
mutex_unlock(&of_clk_mutex);
- return clk_hw_create_clk(hw, dev_id, con_id);
+ return hw;
}
/**
@@ -4125,7 +4159,9 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
*/
struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
{
- return __of_clk_get_from_provider(clkspec, NULL, __func__);
+ struct clk_hw *hw = of_clk_get_hw_from_clkspec(clkspec);
+
+ return clk_hw_create_clk(hw, NULL, __func__);
}
EXPORT_SYMBOL_GPL(of_clk_get_from_provider);
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index c9a14122fadc..cdac6dfa8094 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -12,8 +12,9 @@
struct clk_hw;
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
- const char *dev_id, const char *con_id);
+int of_parse_clkspec(const struct device_node *np, int index, const char *name,
+ struct of_phandle_args *out_args);
+struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec);
#endif
#ifdef CONFIG_COMMON_CLK
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index bdeaffc950ae..5ebb2119c0b9 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -28,69 +28,37 @@ static LIST_HEAD(clocks);
static DEFINE_MUTEX(clocks_mutex);
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-static struct clk *__of_clk_get(struct device_node *np, int index,
- const char *dev_id, const char *con_id)
+static struct clk_hw *of_clk_get_hw(struct device_node *np,
+ int index, const char *con_id)
{
+ int ret;
+ struct clk_hw *hw;
struct of_phandle_args clkspec;
- struct clk *clk;
- int rc;
- rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
- &clkspec);
- if (rc)
- return ERR_PTR(rc);
+ ret = of_parse_clkspec(np, index, con_id, &clkspec);
+ if (ret)
+ return ERR_PTR(ret);
- clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id);
+ hw = of_clk_get_hw_from_clkspec(&clkspec);
of_node_put(clkspec.np);
- return clk;
+ return hw;
}
-struct clk *of_clk_get(struct device_node *np, int index)
+static struct clk *__of_clk_get(struct device_node *np,
+ int index, const char *dev_id,
+ const char *con_id)
{
- return __of_clk_get(np, index, np->full_name, NULL);
+ struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
+
+ return clk_hw_create_clk(hw, dev_id, con_id);
}
-EXPORT_SYMBOL(of_clk_get);
-static struct clk *__of_clk_get_by_name(struct device_node *np,
- const char *dev_id,
- const char *name)
+struct clk *of_clk_get(struct device_node *np, int index)
{
- struct clk *clk = ERR_PTR(-ENOENT);
-
- /* Walk up the tree of devices looking for a clock that matches */
- while (np) {
- int index = 0;
-
- /*
- * For named clocks, first look up the name in the
- * "clock-names" property. If it cannot be found, then
- * index will be an error code, and of_clk_get() will fail.
- */
- if (name)
- index = of_property_match_string(np, "clock-names", name);
- clk = __of_clk_get(np, index, dev_id, name);
- if (!IS_ERR(clk)) {
- break;
- } else if (name && index >= 0) {
- if (PTR_ERR(clk) != -EPROBE_DEFER)
- pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
- np, name ? name : "", index);
- return clk;
- }
-
- /*
- * No matching clock found on this node. If the parent node
- * has a "clock-ranges" property, then we can try one of its
- * clocks.
- */
- np = np->parent;
- if (np && !of_get_property(np, "clock-ranges", NULL))
- break;
- }
-
- return clk;
+ return __of_clk_get(np, index, np->full_name, NULL);
}
+EXPORT_SYMBOL(of_clk_get);
/**
* of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
@@ -106,15 +74,14 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
if (!np)
return ERR_PTR(-ENOENT);
- return __of_clk_get_by_name(np, np->full_name, name);
+ return __of_clk_get(np, -1, np->full_name, name);
}
EXPORT_SYMBOL(of_clk_get_by_name);
#else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
-static struct clk *__of_clk_get_by_name(struct device_node *np,
- const char *dev_id,
- const char *name)
+static struct clk_hw *of_clk_get_hw(struct device_node *np,
+ int index, const char *con_id)
{
return ERR_PTR(-ENOENT);
}
@@ -187,12 +154,12 @@ EXPORT_SYMBOL(clk_get_sys);
struct clk *clk_get(struct device *dev, const char *con_id)
{
const char *dev_id = dev ? dev_name(dev) : NULL;
- struct clk *clk;
+ struct clk_hw *hw;
if (dev && dev->of_node) {
- clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
- if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
- return clk;
+ hw = of_clk_get_hw(dev->of_node, 0, con_id);
+ if (!IS_ERR(hw) || PTR_ERR(hw) == -EPROBE_DEFER)
+ return clk_hw_create_clk(hw, dev_id, con_id);
}
return clk_get_sys(dev_id, con_id);
--
Sent by a computer through tubes
We'd like to have a pointer to the device that's consuming a particular
clk in the clk framework so we can link the consumer to the clk provider
with a PM device link. Add a device argument to clk_hw_create_clk() for
this so it can be used in subsequent patches to add and remove the link.
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk.c | 7 +++++--
drivers/clk/clk.h | 7 +++++--
drivers/clk/clkdev.c | 16 +++++++++++-----
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d4c4cab42375..73f8a287bf42 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -85,6 +85,7 @@ struct clk_core {
struct clk {
struct clk_core *core;
+ struct device *dev;
const char *dev_id;
const char *con_id;
unsigned long min_rate;
@@ -3323,6 +3324,7 @@ static void free_clk(struct clk *clk)
/**
* clk_hw_create_clk: Allocate and link a clk consumer to a clk_core given
* a clk_hw
+ * @dev: clk consumer device
* @hw: clk_hw associated with the clk being consumed
* @dev_id: string describing device name
* @con_id: connection ID string on device
@@ -3331,7 +3333,7 @@ static void free_clk(struct clk *clk)
* consumers. It connects a consumer to the clk_core and clk_hw structures
* used by the framework and clk provider respectively.
*/
-struct clk *clk_hw_create_clk(struct clk_hw *hw,
+struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
const char *dev_id, const char *con_id)
{
struct clk *clk;
@@ -3345,6 +3347,7 @@ struct clk *clk_hw_create_clk(struct clk_hw *hw,
clk = alloc_clk(core, dev_id, con_id);
if (IS_ERR(clk))
return clk;
+ clk->dev = dev;
if (!try_module_get(core->owner)) {
free_clk(clk);
@@ -4161,7 +4164,7 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
{
struct clk_hw *hw = of_clk_get_hw_from_clkspec(clkspec);
- return clk_hw_create_clk(hw, NULL, __func__);
+ return clk_hw_create_clk(NULL, hw, NULL, __func__);
}
EXPORT_SYMBOL_GPL(of_clk_get_from_provider);
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index cdac6dfa8094..fda0ad483416 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -10,6 +10,8 @@
*/
struct clk_hw;
+struct device;
+struct of_phandle_args;
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
int of_parse_clkspec(const struct device_node *np, int index, const char *name,
@@ -18,13 +20,14 @@ struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec);
#endif
#ifdef CONFIG_COMMON_CLK
-struct clk *clk_hw_create_clk(struct clk_hw *hw,
+struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
const char *dev_id, const char *con_id);
void __clk_put(struct clk *clk);
#else
/* All these casts to avoid ifdefs in clkdev... */
static inline struct clk *
-clk_hw_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
+clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id,
+ const char *con_id)
{
return (struct clk *)hw;
}
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 5ebb2119c0b9..f2f4f2afd28c 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -51,7 +51,7 @@ static struct clk *__of_clk_get(struct device_node *np,
{
struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
- return clk_hw_create_clk(hw, dev_id, con_id);
+ return clk_hw_create_clk(NULL, hw, dev_id, con_id);
}
struct clk *of_clk_get(struct device_node *np, int index)
@@ -130,7 +130,8 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
return cl;
}
-struct clk *clk_get_sys(const char *dev_id, const char *con_id)
+static struct clk *__clk_get_sys(struct device *dev, const char *dev_id,
+ const char *con_id)
{
struct clk_lookup *cl;
struct clk *clk = NULL;
@@ -141,7 +142,7 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
if (!cl)
goto out;
- clk = clk_hw_create_clk(cl->clk_hw, dev_id, con_id);
+ clk = clk_hw_create_clk(dev, cl->clk_hw, dev_id, con_id);
if (IS_ERR(clk))
cl = NULL;
out:
@@ -149,6 +150,11 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
return cl ? clk : ERR_PTR(-ENOENT);
}
+
+struct clk *clk_get_sys(const char *dev_id, const char *con_id)
+{
+ return __clk_get_sys(NULL, dev_id, con_id);
+}
EXPORT_SYMBOL(clk_get_sys);
struct clk *clk_get(struct device *dev, const char *con_id)
@@ -159,10 +165,10 @@ struct clk *clk_get(struct device *dev, const char *con_id)
if (dev && dev->of_node) {
hw = of_clk_get_hw(dev->of_node, 0, con_id);
if (!IS_ERR(hw) || PTR_ERR(hw) == -EPROBE_DEFER)
- return clk_hw_create_clk(hw, dev_id, con_id);
+ return clk_hw_create_clk(dev, hw, dev_id, con_id);
}
- return clk_get_sys(dev_id, con_id);
+ return __clk_get_sys(dev, dev_id, con_id);
}
EXPORT_SYMBOL(clk_get);
--
Sent by a computer through tubes
The clk_ops::get_parent function is limited in ability to return errors
because it returns a u8. A "workaround" to return an error is to return
a number >= the number of parents of a clk. This will in turn cause the
framework to "orphan" the clk and make the parent of the clk NULL. This
isn't really correct, because if an error occurs while reading the
parents of a clk we should fail the clk registration, instead of
orphaning the clk and waiting for the clk to appear later.
We really need to have three different return values from the get_parent
clk op. Something valid for a clk that exists, something invalid for a
clk that doesn't exist and never will exist or can't be determined
because the register operation to read the parent failed, and something
for a clk that doesn't exist because the framework doesn't know about
what it is. Introduce a new clk_op that can express all of this by
returning a pointer to the clk_hw of the parent. It's expected that clk
provider drivers will return a valid pointer when the parent is
findable, an error pointer like EPROBE_DEFER if their parent provider
hasn't probed yet but is valid, a NULL pointer if they can't find the
clk but index is valid, and an error pointer with an appropriate error
code otherwise.
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk.c | 117 ++++++++++++++++++++++++++---------
include/linux/clk-provider.h | 9 +++
2 files changed, 96 insertions(+), 30 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 01b36f0851bd..5d82cf25bb29 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2242,14 +2242,84 @@ struct clk *clk_get_parent(struct clk *clk)
}
EXPORT_SYMBOL_GPL(clk_get_parent);
-static struct clk_core *__clk_init_parent(struct clk_core *core)
+static struct clk_core *
+__clk_init_parent(struct clk_core *core, bool update_orphan)
{
u8 index = 0;
+ struct clk_hw *parent_hw = NULL;
- if (core->num_parents > 1 && core->ops->get_parent)
- index = core->ops->get_parent(core->hw);
+ if (core->ops->get_parent_hw) {
+ parent_hw = core->ops->get_parent_hw(core->hw);
+ /*
+ * The provider driver doesn't know what the parent is,
+ * but it's at least something valid, so it's not an
+ * orphan, just a clk with some unknown parent.
+ */
+ if (!parent_hw && update_orphan)
+ core->orphan = false;
+ } else {
+ if (core->num_parents > 1 && core->ops->get_parent)
+ index = core->ops->get_parent(core->hw);
+
+ parent_hw = clk_hw_get_parent_by_index(core->hw, index);
+ }
+
+ if (IS_ERR(parent_hw)) {
+ /* Parent clk provider hasn't probed yet, orphan it */
+ if (PTR_ERR(parent_hw) == -EPROBE_DEFER) {
+ if (update_orphan)
+ core->orphan = true;
+
+ return NULL;
+ }
+
+ return ERR_CAST(parent_hw);
+ }
+
+ if (!parent_hw)
+ return NULL;
+
+ return parent_hw->core;
+}
+
+static int clk_init_parent(struct clk_core *core)
+{
+ core->parent = __clk_init_parent(core, true);
+ if (IS_ERR(core->parent))
+ return PTR_ERR(core->parent);
+
+ /*
+ * Populate core->parent if parent has already been clk_core_init'd. If
+ * parent has not yet been clk_core_init'd then place clk in the orphan
+ * list. If clk doesn't have any parents then place it in the root
+ * clk list.
+ *
+ * Every time a new clk is clk_init'd then we walk the list of orphan
+ * clocks and re-parent any that are children of the clock currently
+ * being clk_init'd.
+ */
+ if (core->parent) {
+ hlist_add_head(&core->child_node,
+ &core->parent->children);
+ core->orphan = core->parent->orphan;
+ } else if (!core->num_parents) {
+ hlist_add_head(&core->child_node, &clk_root_list);
+ core->orphan = false;
+ } else {
+ hlist_add_head(&core->child_node, &clk_orphan_list);
+ }
+
+ return 0;
+}
- return clk_core_get_parent_by_index(core, index);
+static struct clk_core *clk_find_parent(struct clk_core *core)
+{
+ return __clk_init_parent(core, false);
+}
+
+static bool clk_has_parent_op(const struct clk_ops *ops)
+{
+ return ops->get_parent || ops->get_parent_hw;
}
static void clk_core_reparent(struct clk_core *core,
@@ -3045,14 +3115,14 @@ static int __clk_core_init(struct clk_core *core)
goto out;
}
- if (core->ops->set_parent && !core->ops->get_parent) {
+ if (core->ops->set_parent && !clk_has_parent_op(core->ops)) {
pr_err("%s: %s must implement .get_parent & .set_parent\n",
__func__, core->name);
ret = -EINVAL;
goto out;
}
- if (core->num_parents > 1 && !core->ops->get_parent) {
+ if (core->num_parents > 1 && !clk_has_parent_op(core->ops)) {
pr_err("%s: %s must implement .get_parent as it has multi parents\n",
__func__, core->name);
ret = -EINVAL;
@@ -3073,29 +3143,9 @@ static int __clk_core_init(struct clk_core *core)
"%s: invalid NULL in %s's .parent_names\n",
__func__, core->name);
- core->parent = __clk_init_parent(core);
-
- /*
- * Populate core->parent if parent has already been clk_core_init'd. If
- * parent has not yet been clk_core_init'd then place clk in the orphan
- * list. If clk doesn't have any parents then place it in the root
- * clk list.
- *
- * Every time a new clk is clk_init'd then we walk the list of orphan
- * clocks and re-parent any that are children of the clock currently
- * being clk_init'd.
- */
- if (core->parent) {
- hlist_add_head(&core->child_node,
- &core->parent->children);
- core->orphan = core->parent->orphan;
- } else if (!core->num_parents) {
- hlist_add_head(&core->child_node, &clk_root_list);
- core->orphan = false;
- } else {
- hlist_add_head(&core->child_node, &clk_orphan_list);
- core->orphan = true;
- }
+ ret = clk_init_parent(core);
+ if (ret)
+ goto out;
/*
* optional platform-specific magic
@@ -3173,7 +3223,14 @@ static int __clk_core_init(struct clk_core *core)
* parent.
*/
hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
- struct clk_core *parent = __clk_init_parent(orphan);
+ struct clk_core *parent = clk_find_parent(orphan);
+
+ /*
+ * Error parent should have been caught before and returned
+ * as an error during registration.
+ */
+ if (WARN_ON_ONCE(IS_ERR(parent)))
+ continue;
/*
* We need to use __clk_set_parent_before() and _after() to
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 60c51871b04b..8b84dee942bf 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -155,6 +155,14 @@ struct clk_duty {
* multiple parents. It is optional (and unnecessary) for clocks
* with 0 or 1 parents.
*
+ * @get_parent_hw: Queries the hardware to determine the parent of a clock. The
+ * return value is a clk_hw pointer corresponding to
+ * the parent clock. In short, this function translates the parent
+ * value read from hardware into a pointer to the clk_hw for that clk.
+ * Currently only called when the clock is initialized by
+ * __clk_init. This callback is mandatory for clocks with
+ * multiple parents. It is optional for clocks with 0 or 1 parents.
+ *
* @set_rate: Change the rate of this clock. The requested rate is specified
* by the second argument, which should typically be the return
* of .round_rate call. The third argument gives the parent rate
@@ -238,6 +246,7 @@ struct clk_ops {
struct clk_rate_request *req);
int (*set_parent)(struct clk_hw *hw, u8 index);
u8 (*get_parent)(struct clk_hw *hw);
+ struct clk_hw * (*get_parent_hw)(struct clk_hw *hw);
int (*set_rate)(struct clk_hw *hw, unsigned long rate,
unsigned long parent_rate);
int (*set_rate_and_parent)(struct clk_hw *hw,
--
Sent by a computer through tubes
From: Miquel Raynal <[email protected]>
Currently, the core->dev entry is populated only if runtime PM is
enabled. Doing so prevents accessing the device structure in any
case.
Keep the same logic but instead of using the presence of core->dev as
the only condition, also check the status of
pm_runtime_enabled(). Then, we can set the core->dev pointer at any
time as long as a device structure is available.
This change will help supporting device links in the clock subsystem.
Signed-off-by: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
[[email protected]: Change to a boolean flag]
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5d82cf25bb29..01cdb9ae03fa 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -60,6 +60,7 @@ struct clk_core {
struct clk_core *new_child;
unsigned long flags;
bool orphan;
+ bool rpm_enabled;
unsigned int enable_count;
unsigned int prepare_count;
unsigned int protect_count;
@@ -95,9 +96,9 @@ struct clk {
/*** runtime pm ***/
static int clk_pm_runtime_get(struct clk_core *core)
{
- int ret = 0;
+ int ret;
- if (!core->dev)
+ if (!core->rpm_enabled)
return 0;
ret = pm_runtime_get_sync(core->dev);
@@ -106,7 +107,7 @@ static int clk_pm_runtime_get(struct clk_core *core)
static void clk_pm_runtime_put(struct clk_core *core)
{
- if (!core->dev)
+ if (!core->rpm_enabled)
return;
pm_runtime_put_sync(core->dev);
@@ -226,7 +227,7 @@ static bool clk_core_is_enabled(struct clk_core *core)
* taking enable spinlock, but the below check is needed if one tries
* to call it from other places.
*/
- if (core->dev) {
+ if (core->rpm_enabled) {
pm_runtime_get_noresume(core->dev);
if (!pm_runtime_active(core->dev)) {
ret = false;
@@ -236,7 +237,7 @@ static bool clk_core_is_enabled(struct clk_core *core)
ret = core->ops->is_enabled(core->hw);
done:
- if (core->dev)
+ if (core->rpm_enabled)
pm_runtime_put(core->dev);
return ret;
@@ -3391,7 +3392,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
core->ops = hw->init->ops;
if (dev && pm_runtime_enabled(dev))
- core->dev = dev;
+ core->rpm_enabled = true;
+ core->dev = dev;
if (dev && dev->driver)
core->owner = dev->driver->owner;
core->hw = hw;
--
Sent by a computer through tubes
The __clk_get() function is practically a private clk implementation
detail now. No architecture defines it, and given that new code should
be using the common clk framework there isn't a need for it to keep
existing just to serve clkdev purposes. Let's fold it into the
__clk_create_clk() function and make that a little more generic by
renaming it to clk_hw_create_clk(). This will allow the framework to
create a struct clk handle to a particular clk_hw pointer and link it up
as a consumer wherever that's needed.
Doing this also lets us get rid of the __clk_free_clk() API that had to
be kept in sync with __clk_put(). Splitting that API up into the "link
and unlink from consumer list" phase and "free the clk pointer" phase
allows us to reuse that logic in a couple places, simplifying the code.
Cc: Miquel Raynal <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Russell King <[email protected]>
Cc: Michael Turquette <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk.c | 140 +++++++++++++++++++++++++++++--------------
drivers/clk/clk.h | 10 +---
drivers/clk/clkdev.c | 9 +--
3 files changed, 98 insertions(+), 61 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index af011974d4ec..01b36f0851bd 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3202,42 +3202,103 @@ static int __clk_core_init(struct clk_core *core)
return ret;
}
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
+/**
+ * clk_core_link_consumer - Add a clk consumer to the list of consumers in a clk_core
+ * @core: clk to add consumer to
+ * @clk: consumer to link to a clk
+ */
+static void clk_core_link_consumer(struct clk_core *core, struct clk *clk)
+{
+ clk_prepare_lock();
+ hlist_add_head(&clk->clks_node, &core->clks);
+ clk_prepare_unlock();
+}
+
+/**
+ * clk_core_unlink_consumer - Remove a clk consumer from the list of consumers in a clk_core
+ * @clk: consumer to unlink
+ */
+static void clk_core_unlink_consumer(struct clk *clk)
+{
+ lockdep_assert_held(&prepare_lock);
+ hlist_del(&clk->clks_node);
+}
+
+/**
+ * alloc_clk - Allocate a clk consumer, but leave it unlinked to the clk_core
+ * @core: clk to allocate a consumer for
+ * @dev_id: string describing device name
+ * @con_id: connection ID string on device
+ *
+ * Returns: clk consumer left unlinked from the consumer list
+ */
+static struct clk *alloc_clk(struct clk_core *core, const char *dev_id,
const char *con_id)
{
struct clk *clk;
- /* This is to allow this function to be chained to others */
- if (IS_ERR_OR_NULL(hw))
- return ERR_CAST(hw);
-
clk = kzalloc(sizeof(*clk), GFP_KERNEL);
if (!clk)
return ERR_PTR(-ENOMEM);
- clk->core = hw->core;
+ clk->core = core;
clk->dev_id = dev_id;
clk->con_id = kstrdup_const(con_id, GFP_KERNEL);
clk->max_rate = ULONG_MAX;
- clk_prepare_lock();
- hlist_add_head(&clk->clks_node, &hw->core->clks);
- clk_prepare_unlock();
-
return clk;
}
-/* keep in sync with __clk_put */
-void __clk_free_clk(struct clk *clk)
+/**
+ * free_clk - Free a clk consumer
+ * @clk: clk consumer to free
+ *
+ * Note, this assumes the clk has been unlinked from the clk_core consumer
+ * list.
+ */
+static void free_clk(struct clk *clk)
{
- clk_prepare_lock();
- hlist_del(&clk->clks_node);
- clk_prepare_unlock();
-
kfree_const(clk->con_id);
kfree(clk);
}
+/**
+ * clk_hw_create_clk: Allocate and link a clk consumer to a clk_core given
+ * a clk_hw
+ * @hw: clk_hw associated with the clk being consumed
+ * @dev_id: string describing device name
+ * @con_id: connection ID string on device
+ *
+ * This is the main function used to create a clk pointer for use by clk
+ * consumers. It connects a consumer to the clk_core and clk_hw structures
+ * used by the framework and clk provider respectively.
+ */
+struct clk *clk_hw_create_clk(struct clk_hw *hw,
+ const char *dev_id, const char *con_id)
+{
+ struct clk *clk;
+ struct clk_core *core;
+
+ /* This is to allow this function to be chained to others */
+ if (IS_ERR_OR_NULL(hw))
+ return ERR_CAST(hw);
+
+ core = hw->core;
+ clk = alloc_clk(core, dev_id, con_id);
+ if (IS_ERR(clk))
+ return clk;
+
+ if (!try_module_get(core->owner)) {
+ free_clk(clk);
+ return ERR_PTR(-ENOENT);
+ }
+
+ kref_get(&core->ref);
+ clk_core_link_consumer(core, clk);
+
+ return clk;
+}
+
/**
* clk_register - allocate a new clock, register it and return an opaque cookie
* @dev: device that is registering this clock
@@ -3313,17 +3374,27 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
INIT_HLIST_HEAD(&core->clks);
- hw->clk = __clk_create_clk(hw, NULL, NULL);
+ /*
+ * Don't call clk_hw_create_clk() here because that would pin the
+ * provider module to itself and prevent it from ever being removed.
+ */
+ hw->clk = alloc_clk(core, NULL, NULL);
if (IS_ERR(hw->clk)) {
ret = PTR_ERR(hw->clk);
goto fail_parents;
}
+ clk_core_link_consumer(hw->core, hw->clk);
+
ret = __clk_core_init(core);
if (!ret)
return hw->clk;
- __clk_free_clk(hw->clk);
+ clk_prepare_lock();
+ clk_core_unlink_consumer(hw->clk);
+ clk_prepare_unlock();
+
+ free_clk(hw->clk);
hw->clk = NULL;
fail_parents:
@@ -3594,20 +3665,7 @@ EXPORT_SYMBOL_GPL(devm_clk_hw_unregister);
/*
* clkdev helpers
*/
-int __clk_get(struct clk *clk)
-{
- struct clk_core *core = !clk ? NULL : clk->core;
-
- if (core) {
- if (!try_module_get(core->owner))
- return 0;
-
- kref_get(&core->ref);
- }
- return 1;
-}
-/* keep in sync with __clk_free_clk */
void __clk_put(struct clk *clk)
{
struct module *owner;
@@ -3641,8 +3699,7 @@ void __clk_put(struct clk *clk)
module_put(owner);
- kfree_const(clk->con_id);
- kfree(clk);
+ free_clk(clk);
}
/*** clk rate change notifiers ***/
@@ -3980,8 +4037,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
const char *dev_id, const char *con_id)
{
struct of_clk_provider *provider;
- struct clk *clk = ERR_PTR(-EPROBE_DEFER);
- struct clk_hw *hw;
+ struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
if (!clkspec)
return ERR_PTR(-EINVAL);
@@ -3991,21 +4047,13 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
list_for_each_entry(provider, &of_clk_providers, link) {
if (provider->node == clkspec->np) {
hw = __of_clk_get_hw_from_provider(provider, clkspec);
- clk = __clk_create_clk(hw, dev_id, con_id);
- }
-
- if (!IS_ERR(clk)) {
- if (!__clk_get(clk)) {
- __clk_free_clk(clk);
- clk = ERR_PTR(-ENOENT);
- }
-
- break;
+ if (!IS_ERR(hw))
+ break;
}
}
mutex_unlock(&of_clk_mutex);
- return clk;
+ return clk_hw_create_clk(hw, dev_id, con_id);
}
/**
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 70c0ba6336c1..c9a14122fadc 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -17,24 +17,20 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
#endif
#ifdef CONFIG_COMMON_CLK
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
- const char *con_id);
-void __clk_free_clk(struct clk *clk);
-int __clk_get(struct clk *clk);
+struct clk *clk_hw_create_clk(struct clk_hw *hw,
+ const char *dev_id, const char *con_id);
void __clk_put(struct clk *clk);
#else
/* All these casts to avoid ifdefs in clkdev... */
static inline struct clk *
-__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
+clk_hw_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
{
return (struct clk *)hw;
}
-static inline void __clk_free_clk(struct clk *clk) { }
static struct clk_hw *__clk_get_hw(struct clk *clk)
{
return (struct clk_hw *)clk;
}
-static inline int __clk_get(struct clk *clk) { return 1; }
static inline void __clk_put(struct clk *clk) { }
#endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8b3988..bdeaffc950ae 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -174,16 +174,9 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
if (!cl)
goto out;
- clk = __clk_create_clk(cl->clk_hw, dev_id, con_id);
+ clk = clk_hw_create_clk(cl->clk_hw, dev_id, con_id);
if (IS_ERR(clk))
- goto out;
-
- if (!__clk_get(clk)) {
- __clk_free_clk(clk);
cl = NULL;
- goto out;
- }
-
out:
mutex_unlock(&clocks_mutex);
--
Sent by a computer through tubes
On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote:
> The clk_ops::get_parent function is limited in ability to return errors
> because it returns a u8. A "workaround" to return an error is to return
> a number >= the number of parents of a clk. This will in turn cause the
> framework to "orphan" the clk and make the parent of the clk NULL. This
> isn't really correct, because if an error occurs while reading the
> parents of a clk we should fail the clk registration, instead of
> orphaning the clk and waiting for the clk to appear later.
>
> We really need to have three different return values from the get_parent
> clk op. Something valid for a clk that exists, something invalid for a
> clk that doesn't exist and never will exist or can't be determined
> because the register operation to read the parent failed, and something
> for a clk that doesn't exist because the framework doesn't know about
> what it is. Introduce a new clk_op that can express all of this by
> returning a pointer to the clk_hw of the parent. It's expected that clk
> provider drivers will return a valid pointer when the parent is
> findable, an error pointer like EPROBE_DEFER if their parent provider
> hasn't probed yet but is valid, a NULL pointer if they can't find the
> clk but index is valid, and an error pointer with an appropriate error
> code otherwise.
>
> Cc: Miquel Raynal <[email protected]>
> Cc: Jerome Brunet <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Michael Turquette <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/clk/clk.c | 117 ++++++++++++++++++++++++++---------
> include/linux/clk-provider.h | 9 +++
> 2 files changed, 96 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 01b36f0851bd..5d82cf25bb29 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2242,14 +2242,84 @@ struct clk *clk_get_parent(struct clk *clk)
> }
> EXPORT_SYMBOL_GPL(clk_get_parent);
>
> -static struct clk_core *__clk_init_parent(struct clk_core *core)
> +static struct clk_core *
> +__clk_init_parent(struct clk_core *core, bool update_orphan)
> {
> u8 index = 0;
> + struct clk_hw *parent_hw = NULL;
>
> - if (core->num_parents > 1 && core->ops->get_parent)
> - index = core->ops->get_parent(core->hw);
> + if (core->ops->get_parent_hw) {
> + parent_hw = core->ops->get_parent_hw(core->hw);
> + /*
> + * The provider driver doesn't know what the parent is,
> + * but it's at least something valid, so it's not an
> + * orphan, just a clk with some unknown parent.
> + */
I suppose this is the answer the discussion we had last year. I'm not sure it
answer the problem. In the case I presented, we have no idea wether the
setting is valid or not.
We can't assume it is `at least something valid`, the value in the mux is just
something we can't map.
Aslo, could you provide an example of what such callback would be, with clk-
mux maybe ?
I don't get how a clock driver will keep track of the clk_hw pointers it is
connected to. Is there an API for this ? clk-mux must access to clk_core to
explore his own parent ... which already does not scale well, expect if we
plan to expose clk_core at some point ?
> + if (!parent_hw && update_orphan)
> + core->orphan = false;
> + } else {
> + if (core->num_parents > 1 && core->ops->get_parent)
I still get why, when num_parents == 1, it is OK to call get_parent_hw() and
no get_parent(). It does not seems coherent.
> + index = core->ops->get_parent(core->hw);
> +
> + parent_hw = clk_hw_get_parent_by_index(core->hw, index);
> + }
> +
> + if (IS_ERR(parent_hw)) {
> + /* Parent clk provider hasn't probed yet, orphan it */
> + if (PTR_ERR(parent_hw) == -EPROBE_DEFER) {
> + if (update_orphan)
> + core->orphan = true;
> +
> + return NULL;
> + }
> +
> + return ERR_CAST(parent_hw);
> + }
> +
> + if (!parent_hw)
> + return NULL;
> +
> + return parent_hw->core;
> +}
> +
> +static int clk_init_parent(struct clk_core *core)
> +{
> + core->parent = __clk_init_parent(core, true);
> + if (IS_ERR(core->parent))
> + return PTR_ERR(core->parent);
> +
> + /*
> + * Populate core->parent if parent has already been clk_core_init'd.
> If
> + * parent has not yet been clk_core_init'd then place clk in the
> orphan
> + * list. If clk doesn't have any parents then place it in the root
> + * clk list.
> + *
> + * Every time a new clk is clk_init'd then we walk the list of orphan
> + * clocks and re-parent any that are children of the clock currently
> + * being clk_init'd.
> + */
> + if (core->parent) {
> + hlist_add_head(&core->child_node,
> + &core->parent->children);
> + core->orphan = core->parent->orphan;
> + } else if (!core->num_parents) {
> + hlist_add_head(&core->child_node, &clk_root_list);
> + core->orphan = false;
> + } else {
> + hlist_add_head(&core->child_node, &clk_orphan_list);
> + }
> +
> + return 0;
> +}
>
> - return clk_core_get_parent_by_index(core, index);
> +static struct clk_core *clk_find_parent(struct clk_core *core)
> +{
> + return __clk_init_parent(core, false);
> +}
> +
> +static bool clk_has_parent_op(const struct clk_ops *ops)
> +{
> + return ops->get_parent || ops->get_parent_hw;
> }
>
> static void clk_core_reparent(struct clk_core *core,
> @@ -3045,14 +3115,14 @@ static int __clk_core_init(struct clk_core *core)
> goto out;
> }
>
> - if (core->ops->set_parent && !core->ops->get_parent) {
> + if (core->ops->set_parent && !clk_has_parent_op(core->ops)) {
> pr_err("%s: %s must implement .get_parent & .set_parent\n",
> __func__, core->name);
> ret = -EINVAL;
> goto out;
> }
>
> - if (core->num_parents > 1 && !core->ops->get_parent) {
> + if (core->num_parents > 1 && !clk_has_parent_op(core->ops)) {
> pr_err("%s: %s must implement .get_parent as it has multi
> parents\n",
> __func__, core->name);
> ret = -EINVAL;
> @@ -3073,29 +3143,9 @@ static int __clk_core_init(struct clk_core *core)
> "%s: invalid NULL in %s's .parent_names\n",
> __func__, core->name);
>
> - core->parent = __clk_init_parent(core);
> -
> - /*
> - * Populate core->parent if parent has already been clk_core_init'd.
> If
> - * parent has not yet been clk_core_init'd then place clk in the
> orphan
> - * list. If clk doesn't have any parents then place it in the root
> - * clk list.
> - *
> - * Every time a new clk is clk_init'd then we walk the list of orphan
> - * clocks and re-parent any that are children of the clock currently
> - * being clk_init'd.
> - */
> - if (core->parent) {
> - hlist_add_head(&core->child_node,
> - &core->parent->children);
> - core->orphan = core->parent->orphan;
> - } else if (!core->num_parents) {
> - hlist_add_head(&core->child_node, &clk_root_list);
> - core->orphan = false;
> - } else {
> - hlist_add_head(&core->child_node, &clk_orphan_list);
> - core->orphan = true;
> - }
> + ret = clk_init_parent(core);
> + if (ret)
> + goto out;
>
> /*
> * optional platform-specific magic
> @@ -3173,7 +3223,14 @@ static int __clk_core_init(struct clk_core *core)
> * parent.
> */
> hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node)
> {
> - struct clk_core *parent = __clk_init_parent(orphan);
> + struct clk_core *parent = clk_find_parent(orphan);
> +
> + /*
> + * Error parent should have been caught before and returned
> + * as an error during registration.
> + */
> + if (WARN_ON_ONCE(IS_ERR(parent)))
> + continue;
>
> /*
> * We need to use __clk_set_parent_before() and _after() to
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 60c51871b04b..8b84dee942bf 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -155,6 +155,14 @@ struct clk_duty {
> * multiple parents. It is optional (and unnecessary) for clocks
> * with 0 or 1 parents.
> *
> + * @get_parent_hw: Queries the hardware to determine the parent of a
> clock. The
> + * return value is a clk_hw pointer corresponding to
> + * the parent clock. In short, this function translates the
> parent
> + * value read from hardware into a pointer to the clk_hw for that
> clk.
> + * Currently only called when the clock is initialized by
> + * __clk_init. This callback is mandatory for clocks with
> + * multiple parents. It is optional for clocks with 0 or 1
> parents.
> + *
The comments above could imply that get_parent() and get_parent_hw() are both
mandatory if num_parent > 1. (I don't think so but) Is this your intent ?
> * @set_rate: Change the rate of this clock. The requested rate is
> specified
> * by the second argument, which should typically be the return
> * of .round_rate call. The third argument gives the parent rate
> @@ -238,6 +246,7 @@ struct clk_ops {
> struct clk_rate_request *req);
> int (*set_parent)(struct clk_hw *hw, u8 index);
> u8 (*get_parent)(struct clk_hw *hw);
> + struct clk_hw * (*get_parent_hw)(struct clk_hw *hw);
> int (*set_rate)(struct clk_hw *hw, unsigned long rate,
> unsigned long parent_rate);
> int (*set_rate_and_parent)(struct clk_hw *hw,
On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote:
> The common clk framework is lacking in ability to describe the clk
> topology without specifying strings for every possible parent-child
> link. There are a few drawbacks to the current approach:
>
> 1) String comparisons are used for everything, including describing
> topologies that are 'local' to a single clock controller.
>
> 2) clk providers (e.g. i2c clk drivers) need to create globally unique
> clk names to avoid collisions in the clk namespace, leading to awkward
> name generation code in various clk drivers.
>
> 3) DT bindings may not fully describe the clk topology and linkages
> between clk controllers because drivers can easily rely on globally unique
> strings to describe connections between clks.
>
> This leads to confusing DT bindings, complicated clk name generation
> code, and inefficient string comparisons during clk registration just so
> that the clk framework can detect the topology of the clk tree.
> Furthermore, some drivers call clk_get() and then __clk_get_name() to
> extract the globally unique clk name just so they can specify the parent
> of the clk they're registering. We have of_clk_parent_fill() but that
> mostly only works for single clks registered from a DT node, which isn't
> the norm. Let's simplify this all by introducing two new ways of
> specifying clk parents.
>
> The first method is an array of pointers to clk_hw structures
> corresponding to the parents at that index. This works for clks that are
> registered when we have access to all the clk_hw pointers for the
> parents.
>
> The second method is a mix of clk_hw pointers and strings of local and
> global parent clk names. If the .name member of the map is set we'll
> look for that clk by performing a DT based lookup of the device the clk
> is registered with and the .name specified in the map. If that fails,
> we'll fallback to the .fallback member and perform a global clk name
> lookup like we've always done before.
>
> Using either one of these new methods is entirely optional. Existing
> drivers will continue to work, and they can migrate to this new approach
> as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> array in struct clk_init_data and use one of these new methods instead.
This may indeed allow to remove a lot of annoying code.
However, does this remove the globally unique name string constraints ? Are we
now allowed to have 2 instances of a driver registering a clock named "foo" ?
If this is the case, I wonder:
* How will it work with debugfs: clock names are used to create the
directories in there, plus clk_summary will quickly get messy.
* How will it behave if 2 clock registers with "foo" and one clock register
with the fallback parent "foo" ?
>
> Cc: Miquel Raynal <[email protected]>
> Cc: Jerome Brunet <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Michael Turquette <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/clk/clk.c | 215 +++++++++++++++++++++++++----------
> include/linux/clk-provider.h | 17 ++-
> 2 files changed, 173 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 202902e64799..0cd90a2339ad 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -42,6 +42,13 @@ static LIST_HEAD(clk_notifier_list);
>
> /*** private data structures ***/
>
> +struct clk_parent_map {
> + struct clk_hw *hw;
> + struct clk_core *core;
> + const char *name;
> + const char *fallback;
> +};
> +
> struct clk_core {
> const char *name;
> const struct clk_ops *ops;
> @@ -49,8 +56,7 @@ struct clk_core {
> struct module *owner;
> struct device *dev;
> struct clk_core *parent;
> - const char **parent_names;
> - struct clk_core **parents;
> + struct clk_parent_map *parents;
> u8 num_parents;
> u8 new_parent_index;
> unsigned long rate;
> @@ -319,17 +325,77 @@ static struct clk_core *clk_core_lookup(const char
> *name)
> return NULL;
> }
>
> +/**
> + * clk_core_get - Find the parent of a clk using a clock specifier in DT
> + * @core: clk to find parent of
> + * @name: name to search for in 'clock-names' of device providing clk
> + *
> + * This is the preferred method for clk providers to find the parent of a
> + * clk when that parent is external to the clk controller. The parent_names
> + * array is indexed and treated as a local name matching a string in the
> device
> + * node's 'clock-names' property. This allows clk providers to use their
> own
> + * namespace instead of looking for a globally unique parent string.
> + *
> + * For example the following DT snippet would allow a clock registered by
> the
> + * clock-controller@c001 that has a clk_init_data::parent_data array
> + * with 'xtal' in the 'name' member to find the clock provided by the
> + * clock-controller@f00abcd without needing to get the globally unique name
> of
> + * the xtal clk.
> + *
> + * parent: clock-controller@f00abcd {
> + * reg = <0xf00abcd 0xabcd>;
> + * #clock-cells = <0>;
> + * };
> + *
> + * clock-controller@c001 {
> + * reg = <0xc001 0xf00d>;
> + * clocks = <&parent>;
> + * clock-names = "xtal";
> + * #clock-cells = <1>;
> + * };
> + */
> +static struct clk_core *clk_core_get(struct clk_core *core, const char
> *name)
> +{
> + struct clk_hw *hw;
> + struct device *dev = core->dev;
> +
> + if (!dev)
> + return NULL;
> +
> + /* TODO: Support clkdev clk_lookups */
> + hw = of_clk_get_hw(dev->of_node, -1, name);
> + if (IS_ERR_OR_NULL(hw))
> + return NULL;
> +
> + return hw->core;
> +}
> +
> +static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
> +{
> + struct clk_parent_map *entry = &core->parents[index];
> + struct clk_core *parent = NULL;
> +
> + if (entry->hw)
> + parent = entry->hw->core;
> + else if (entry->name)
> + parent = clk_core_get(core, entry->name);
> +
> + if (!parent)
> + parent = clk_core_lookup(entry->fallback);
> +
> + entry->core = parent;
> +}
> +
> static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
> u8 index)
> {
> - if (!core || index >= core->num_parents)
> + if (!core || index >= core->num_parents || !core->parents)
> return NULL;
>
> - if (!core->parents[index])
> - core->parents[index] =
> - clk_core_lookup(core->parent_names[index]);
> + if (!core->parents[index].core)
> + clk_core_fill_parent_index(core, index);
>
> - return core->parents[index];
> + return core->parents[index].core;
> }
>
> struct clk_hw *
> @@ -2353,6 +2419,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw
> *new_parent)
> bool clk_has_parent(struct clk *clk, struct clk *parent)
> {
> struct clk_core *core, *parent_core;
> + int i;
>
> /* NULL clocks should be nops, so return success if either is NULL. */
> if (!clk || !parent)
> @@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk
> *parent)
> if (core->parent == parent_core)
> return true;
>
> - return match_string(core->parent_names, core->num_parents,
> - parent_core->name) >= 0;
> + for (i = 0; i < core->num_parents; i++)
> + if (!strcmp(core->parents[i].fallback, parent_core->name))
> + return true;
> +
> + return false;
> }
> EXPORT_SYMBOL_GPL(clk_has_parent);
>
> @@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file *s,
> void *data)
> int i;
>
> for (i = 0; i < core->num_parents - 1; i++)
> - seq_printf(s, "%s ", core->parent_names[i]);
> + seq_printf(s, "%s ", core->parents[i].fallback);
>
> - seq_printf(s, "%s\n", core->parent_names[i]);
> + seq_printf(s, "%s\n", core->parents[i].fallback);
Wouldn't this show nothing if parent_data is used but fallback is not provided
(like in your example when you provide the clk_hw pointer instead) or did I
miss something ?
>
> return 0;
> }
> @@ -3085,7 +3155,7 @@ static inline void clk_debug_unregister(struct
> clk_core *core)
> */
> static int __clk_core_init(struct clk_core *core)
> {
> - int i, ret;
> + int ret;
> struct clk_core *orphan;
> struct hlist_node *tmp2;
> unsigned long rate;
> @@ -3139,12 +3209,6 @@ static int __clk_core_init(struct clk_core *core)
> goto out;
> }
>
> - /* throw a WARN if any entries in parent_names are NULL */
> - for (i = 0; i < core->num_parents; i++)
> - WARN(!core->parent_names[i],
> - "%s: invalid NULL in %s's .parent_names\n",
> - __func__, core->name);
> -
> ret = clk_init_parent(core);
> if (ret)
> goto out;
> @@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev,
> struct clk_hw *hw,
> return clk;
> }
>
> +static int clk_core_populate_parent_map(struct clk_core *core)
> +{
> + const struct clk_init_data *init = core->hw->init;
> + u8 num_parents = init->num_parents;
> + const char * const *parent_names = init->parent_names;
> + struct clk_hw **parent_hws = init->parent_hws;
> + const struct clk_parent_data *parent_data = init->parent_data;
> + int i, ret = 0;
> + struct clk_parent_map *parents, *parent;
> +
> + if (!num_parents)
> + return 0;
> +
> + /*
> + * Avoid unnecessary string look-ups of clk_core's possible parents by
> + * having a cache of names/clk_hw pointers to clk_core pointers.
> + */
> + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> + core->parents = parents;
> + if (!parents)
> + return -ENOMEM;
> +
> + /* Copy everything over because it might be __initdata */
> + for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> + if (parent_names) {
> + /* throw a WARN if any entries are NULL */
> + WARN(!parent_names[i],
> + "%s: invalid NULL in %s's .parent_names\n",
> + __func__, core->name);
> + parent->fallback = kstrdup_const(parent_names[i],
> + GFP_KERNEL);
> + if (!parent->fallback) {
> + ret = -ENOMEM;
> + while (--i >= 0)
> + kfree_const(parents[i].fallback);
> + }
> + } else if (parent_data) {
> + parent->hw = parent_data[i].hw;
> + parent->name = parent_data[i].name;
> + parent->fallback = parent_data[i].fallback;
I'm a bit confused by the comment at the top of the loop. Strings in
parent_data are not copied over like we used to do with parent_names.
Is it allowed to have parent_data in __initdata ? It could be error prone if
parent_names and parent_data behaved differently on this.
> + } else if (parent_hws) {
> + parent->hw = parent_hws[i];
> + } else {
> + ret = -EINVAL;
> + WARN(1, "Must specify parents if num_parents > 0\n");
> + }
> +
> + if (ret) {
> + kfree(parents);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void clk_core_free_parent_map(struct clk_core *core)
> +{
> + int i = core->num_parents;
> +
> + if (!core->num_parents)
> + return;
> +
> + while (--i >= 0)
> + kfree_const(core->parents[i].fallback);
> + kfree(core->parents);
> +}
> +
> /**
> * clk_register - allocate a new clock, register it and return an opaque
> cookie
> * @dev: device that is registering this clock
> @@ -3373,7 +3505,7 @@ struct clk *clk_hw_create_clk(struct device *dev,
> struct clk_hw *hw,
> */
> struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> {
> - int i, ret;
> + int ret;
> struct clk_core *core;
>
> core = kzalloc(sizeof(*core), GFP_KERNEL);
> @@ -3406,33 +3538,9 @@ struct clk *clk_register(struct device *dev, struct
> clk_hw *hw)
> core->max_rate = ULONG_MAX;
> hw->core = core;
>
> - /* allocate local copy in case parent_names is __initdata */
> - core->parent_names = kcalloc(core->num_parents, sizeof(char *),
> - GFP_KERNEL);
> -
> - if (!core->parent_names) {
> - ret = -ENOMEM;
> - goto fail_parent_names;
> - }
> -
> -
> - /* copy each string name in case parent_names is __initdata */
> - for (i = 0; i < core->num_parents; i++) {
> - core->parent_names[i] = kstrdup_const(hw->init-
> >parent_names[i],
> - GFP_KERNEL);
> - if (!core->parent_names[i]) {
> - ret = -ENOMEM;
> - goto fail_parent_names_copy;
> - }
> - }
> -
> - /* avoid unnecessary string look-ups of clk_core's possible parents.
> */
> - core->parents = kcalloc(core->num_parents, sizeof(*core->parents),
> - GFP_KERNEL);
> - if (!core->parents) {
> - ret = -ENOMEM;
> + ret = clk_core_populate_parent_map(core);
> + if (ret)
> goto fail_parents;
> - };
>
> INIT_HLIST_HEAD(&core->clks);
>
> @@ -3443,7 +3551,7 @@ struct clk *clk_register(struct device *dev, struct
> clk_hw *hw)
> hw->clk = alloc_clk(core, NULL, NULL);
> if (IS_ERR(hw->clk)) {
> ret = PTR_ERR(hw->clk);
> - goto fail_parents;
> + goto fail_create_clk;
> }
>
> clk_core_link_consumer(hw->core, hw->clk);
> @@ -3459,13 +3567,9 @@ struct clk *clk_register(struct device *dev, struct
> clk_hw *hw)
> free_clk(hw->clk);
> hw->clk = NULL;
>
> +fail_create_clk:
> + clk_core_free_parent_map(core);
> fail_parents:
> - kfree(core->parents);
> -fail_parent_names_copy:
> - while (--i >= 0)
> - kfree_const(core->parent_names[i]);
> - kfree(core->parent_names);
> -fail_parent_names:
> fail_ops:
> kfree_const(core->name);
> fail_name:
> @@ -3495,15 +3599,10 @@ EXPORT_SYMBOL_GPL(clk_hw_register);
> static void __clk_release(struct kref *ref)
> {
> struct clk_core *core = container_of(ref, struct clk_core, ref);
> - int i = core->num_parents;
>
> lockdep_assert_held(&prepare_lock);
>
> - kfree(core->parents);
> - while (--i >= 0)
> - kfree_const(core->parent_names[i]);
> -
> - kfree(core->parent_names);
> + clk_core_free_parent_map(core);
> kfree_const(core->name);
> kfree(core);
> }
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 8b84dee942bf..f513f4074a93 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -264,6 +264,18 @@ struct clk_ops {
> void (*debug_init)(struct clk_hw *hw, struct dentry
> *dentry);
> };
>
> +/**
> + * struct clk_parent_data - clk parent information
> + * @hw: parent clk_hw pointer (used for clk providers with internal clks)
> + * @name: parent name local to provider registering clk
> + * @fallback: globally unique parent name (used as a fallback)
> + */
> +struct clk_parent_data {
> + struct clk_hw *hw;
> + const char *name;
> + const char *fallback;
If I understand correctly, .name and .fallback will be ignored if hw is
provided ? Maybe this should be documented somehow ?
> +};
> +
> /**
> * struct clk_init_data - holds init data that's common to all clocks and
> is
> * shared between the clock provider and the common clock framework.
> @@ -277,7 +289,10 @@ struct clk_ops {
> struct clk_init_data {
> const char *name;
> const struct clk_ops *ops;
> - const char * const *parent_names;
> + /* Only one of the following three should be assigned */
> + const char * const *parent_names; /* If NULL (and
> num_parents != 0) look at parent_data and parent_hws */
> + const struct clk_parent_data *parent_data;
> + struct clk_hw **parent_hws;
Isn't parent_hws redundant with parent_data ? you may pass the clk_hw pointer
with both, right ?
> u8 num_parents;
> unsigned long flags;
> };
Hi Stephen,
Stephen Boyd <[email protected]> wrote on Mon, 28 Jan 2019 22:10:12
-0800:
> There are a couple problems with clk parent handling in the common clk
> framework. This patch series combines a few different topics together as
> all this code is closely related.
>
> First off, we don't do well at determining parents of clks at clk
> registration time because the return type of the clk_ops::get_parent()
> op is a u8 which isn't expressive enough to cover all our use-cases.
>
> Secondly, we use strings for all parent-child linkages, and this leads
> to poorly written code that extracts clk names from struct clk pointers
> and makes clk provider drivers use clk consumer APIs.
>
> Thirdly, clkdev.c has a collection of DT parsing logic in it that is
> only used when the common clk framework is present but we want to use
> that same logic for describing parent-child linkages of clk providers
> via in DT. This should all be moved into the common clk framework and
> used from there as well as from clkdev.c, so this series changes the way
> clkdev interacts with the clk framework by having clkdev get clk_hw
> pointers out of DT clk specifiers and then convert those into clk
> pointers with clk_hw_create_clk(). Splitting the API this way lets us
> get clk_hw pointers for clk providers and skip the struct clk pointer
> creation phase that we don't need to do when describing parent-child
> linkages.
>
> And finally, we have a few patches in here that lay the groundwork for
> supporting device links in the common clk framework. We do that by
> pushing the consuming device pointer through to the clk pointer creation
> in clk_hw_create_clk(). This wasn't always easy to do when we had
> __clk_create_clk() called from multiple places, some being deep in the
> clk registration path. This series simplifies that logic so that we can
> always attach a consumer device to a clk that we create in one place,
> instead of making that linkage in multiple places near where we create
> struct clk pointers.
>
> Miquel Raynal (1):
> clk: core: clarify the check for runtime PM
>
> Stephen Boyd (8):
> clk: Combine __clk_get() and __clk_create_clk()
> clk: Introduce get_parent_hw clk op
> clk: Introduce of_clk_get_hw_from_clkspec()
> clk: Inform the core about consumer devices
> clk: Move of_clk_*() APIs into clk.c from clkdev.c
> clk: Allow parents to be specified without string names
> clk: qcom: gcc-sdm845: Migrate to DT parent mapping
> arm64: dts: qcom: Specify XO clk as input to GCC node
>
> Cc: Miquel Raynal <[email protected]>
> Cc: Jerome Brunet <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Michael Turquette <[email protected]>
>
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +
> drivers/clk/clk.c | 584 ++++++++++++++++++++-------
> drivers/clk/clk.h | 23 +-
> drivers/clk/clkdev.c | 120 +-----
> drivers/clk/qcom/gcc-sdm845.c | 180 ++++-----
> include/linux/clk-provider.h | 26 +-
> 6 files changed, 583 insertions(+), 352 deletions(-)
>
>
> base-commit: 651022382c7f8da46cb4872a545ee1da6d097d2a
Thanks for working on this! I rebased my suspend to RAM work on top of
this version, including the patch adding clock device links (applies
cleanly). Everything looks fine:
[ 1.859464] marvell-armada-3700-tbg-clock d0013200.tbg: Linked as a consumer to d0013800.pinctrl:xtal-clk
[ 1.869415] marvell-armada-3700-tbg-clock d0013200.tbg: Dropping the link to d0013800.pinctrl:xtal-clk
[ 1.879045] marvell-armada-3700-tbg-clock d0013200.tbg: Linked as a consumer to d0013800.pinctrl:xtal-clk
[ 1.889466] marvell-armada-3700-periph-clock d0013000.nb-periph-clk: Linked as a consumer to d0013200.tbg
[ 1.899369] marvell-armada-3700-periph-clock d0013000.nb-periph-clk: Linked as a consumer to d0013800.pinctrl:xtal-clk
[ 1.910799] marvell-armada-3700-periph-clock d0018000.sb-periph-clk: Linked as a consumer to d0013200.tbg
[ 1.977034] ahci-mvebu d00e0000.sata: Linked as a consumer to d0013000.nb-periph-clk
[ 2.019689] armada_3700_spi d0010600.spi: Linked as a consumer to d0013000.nb-periph-clk
[ 2.168316] mvneta d0030000.ethernet: Linked as a consumer to d0018000.sb-periph-clk
[ 2.295016] xhci-hcd d0058000.usb: Linked as a consumer to d0018000.sb-periph-clk
[ 2.431512] xenon-sdhci d00d0000.sdhci: Linked as a consumer to d0013000.nb-periph-clk
[ 2.444835] xenon-sdhci d00d0000.sdhci: Dropping the link to d0013000.nb-periph-clk
[ 2.526683] mvebu-uart d0012000.serial: Linked as a consumer to d0013800.pinctrl:xtal-clk
[ 2.586209] advk-pcie d0070000.pcie: Linked as a consumer to d0018000.sb-periph-clk
[ 4.498571] xenon-sdhci d00d0000.sdhci: Linked as a consumer to d0013000.nb-periph-clk
[ 4.510078] xenon-sdhci d00d0000.sdhci: Linked as a consumer to regulator.1
[ 4.559214] cpu cpu0: Linked as a consumer to d0013000.nb-periph-clk
[ 4.565613] cpu cpu0: Dropping the link to d0013000.nb-periph-clk
[ 4.571832] cpu cpu0: Linked as a consumer to d0013000.nb-periph-clk
Tested-by: Miquel Raynal <[email protected]>
Thanks,
Miquèl
Quoting Jerome Brunet (2019-01-29 02:12:00)
> On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote:
> >
> > Using either one of these new methods is entirely optional. Existing
> > drivers will continue to work, and they can migrate to this new approach
> > as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> > array in struct clk_init_data and use one of these new methods instead.
>
> This may indeed allow to remove a lot of annoying code.
>
> However, does this remove the globally unique name string constraints ? Are we
> now allowed to have 2 instances of a driver registering a clock named "foo" ?
Yes it would be allowed to have multiple clocks with the same name but
I'm not trying to solve that exact problem right now. The framework
already complains if that's happening, so drivers need to generate
unique names regardless of this series.
>
> If this is the case, I wonder:
> * How will it work with debugfs: clock names are used to create the
> directories in there, plus clk_summary will quickly get messy.
Agreed. We should probably prepend the device name to the front of the
clk now when creating in debugfs. I'll throw a patch for that into the
pile to get that problem out of the way.
I'm also beginning to think we should add a way to pass in the of_node
for a clk when it's registered. Probably need to do that with the
initdata structure again. That way we can lookup a clk's parents with
the DT node if they don't call clk_register() with a device and also to
generate a better unique name for the clk for debug purposes.
> * How will it behave if 2 clock registers with "foo" and one clock register
> with the fallback parent "foo" ?
Sorry, I'm not following. The fallback is global so we'll be unable to
figure out which parent the clk is referring to. Maybe this is an
argument for keeping everything globally unique in the clk name space?
>
> >
> > Cc: Miquel Raynal <[email protected]>
> > Cc: Jerome Brunet <[email protected]>
> > Cc: Russell King <[email protected]>
> > Cc: Michael Turquette <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
> > ---
> > @@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk
> > *parent)
> > if (core->parent == parent_core)
> > return true;
> >
> > - return match_string(core->parent_names, core->num_parents,
> > - parent_core->name) >= 0;
> > + for (i = 0; i < core->num_parents; i++)
> > + if (!strcmp(core->parents[i].fallback, parent_core->name))
> > + return true;
This is also messy and not great BTW.
> > +
> > + return false;
> > }
> > EXPORT_SYMBOL_GPL(clk_has_parent);
> >
> > @@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file *s,
> > void *data)
> > int i;
> >
> > for (i = 0; i < core->num_parents - 1; i++)
> > - seq_printf(s, "%s ", core->parent_names[i]);
> > + seq_printf(s, "%s ", core->parents[i].fallback);
> >
> > - seq_printf(s, "%s\n", core->parent_names[i]);
> > + seq_printf(s, "%s\n", core->parents[i].fallback);
>
> Wouldn't this show nothing if parent_data is used but fallback is not provided
> (like in your example when you provide the clk_hw pointer instead) or did I
> miss something ?
Yes, it will show nothing. Maybe we need to generate the debug name
somehow? That code sounds quite awful because we're going in reverse to
the device through a DT node pointer. Or add an indicator in the output
if the parent is a global name vs. a local name or a clk debugfs name?
"global_name(g)"
"local_name(l)"
"debug_name"
Or if we can't figure anything out then perhaps just ignoring this
problem for now is fine. It is debugfs after all.
>
> >
> > return 0;
> > }
> > @@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev,
> > struct clk_hw *hw,
> > return clk;
> > }
> >
> > +static int clk_core_populate_parent_map(struct clk_core *core)
> > +{
> > + const struct clk_init_data *init = core->hw->init;
> > + u8 num_parents = init->num_parents;
> > + const char * const *parent_names = init->parent_names;
> > + struct clk_hw **parent_hws = init->parent_hws;
> > + const struct clk_parent_data *parent_data = init->parent_data;
> > + int i, ret = 0;
> > + struct clk_parent_map *parents, *parent;
> > +
> > + if (!num_parents)
> > + return 0;
> > +
> > + /*
> > + * Avoid unnecessary string look-ups of clk_core's possible parents by
> > + * having a cache of names/clk_hw pointers to clk_core pointers.
> > + */
> > + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> > + core->parents = parents;
> > + if (!parents)
> > + return -ENOMEM;
> > +
> > + /* Copy everything over because it might be __initdata */
> > + for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> > + if (parent_names) {
> > + /* throw a WARN if any entries are NULL */
> > + WARN(!parent_names[i],
> > + "%s: invalid NULL in %s's .parent_names\n",
> > + __func__, core->name);
> > + parent->fallback = kstrdup_const(parent_names[i],
> > + GFP_KERNEL);
> > + if (!parent->fallback) {
> > + ret = -ENOMEM;
> > + while (--i >= 0)
> > + kfree_const(parents[i].fallback);
> > + }
> > + } else if (parent_data) {
> > + parent->hw = parent_data[i].hw;
> > + parent->name = parent_data[i].name;
> > + parent->fallback = parent_data[i].fallback;
>
> I'm a bit confused by the comment at the top of the loop. Strings in
> parent_data are not copied over like we used to do with parent_names.
>
> Is it allowed to have parent_data in __initdata ? It could be error prone if
> parent_names and parent_data behaved differently on this.
Good catch, thanks. Will fix.
>
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 8b84dee942bf..f513f4074a93 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -264,6 +264,18 @@ struct clk_ops {
> > void (*debug_init)(struct clk_hw *hw, struct dentry
> > *dentry);
> > };
> >
> > +/**
> > + * struct clk_parent_data - clk parent information
> > + * @hw: parent clk_hw pointer (used for clk providers with internal clks)
> > + * @name: parent name local to provider registering clk
> > + * @fallback: globally unique parent name (used as a fallback)
> > + */
> > +struct clk_parent_data {
> > + struct clk_hw *hw;
> > + const char *name;
> > + const char *fallback;
>
> If I understand correctly, .name and .fallback will be ignored if hw is
> provided ? Maybe this should be documented somehow ?
Sure. I'll add some documentation to the long portion of the kernel-doc
here describing priority order.
>
> > +};
> > +
> > /**
> > * struct clk_init_data - holds init data that's common to all clocks and
> > is
> > * shared between the clock provider and the common clock framework.
> > @@ -277,7 +289,10 @@ struct clk_ops {
> > struct clk_init_data {
> > const char *name;
> > const struct clk_ops *ops;
> > - const char * const *parent_names;
> > + /* Only one of the following three should be assigned */
> > + const char * const *parent_names; /* If NULL (and
> > num_parents != 0) look at parent_data and parent_hws */
> > + const struct clk_parent_data *parent_data;
> > + struct clk_hw **parent_hws;
>
> Isn't parent_hws redundant with parent_data ? you may pass the clk_hw pointer
> with both, right ?
Yeah it's redundant, but I thought that drivers may not want to waste
the extra space for pointers if they had all the pointers in hand for
the parents they care about, or if they had a single parent and just
wanted to point to that directly.
Another thought is to have a union on these three pointers and then a
flag indicating which method is used:
union {
const char * const *parent_names;
const struct clk_parent_data *parent_data;
struct clk_hw **parent_hws;
};
#define CLK_PARENTS_POINTERS BIT(3)
#define CLK_PARENTS_DATA BIT(8)
On Tue, 2019-01-29 at 10:56 -0800, Stephen Boyd wrote:
> Quoting Jerome Brunet (2019-01-29 02:12:00)
> > On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote:
> > > Using either one of these new methods is entirely optional. Existing
> > > drivers will continue to work, and they can migrate to this new approach
> > > as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> > > array in struct clk_init_data and use one of these new methods instead.
> >
> > This may indeed allow to remove a lot of annoying code.
> >
> > However, does this remove the globally unique name string constraints ?
> > Are we
> > now allowed to have 2 instances of a driver registering a clock named
> > "foo" ?
>
> Yes it would be allowed to have multiple clocks with the same name but
> I'm not trying to solve that exact problem right now. The framework
> already complains if that's happening, so drivers need to generate
> unique names regardless of this series.
Ok. Got it
>
> > If this is the case, I wonder:
> > * How will it work with debugfs: clock names are used to create the
> > directories in there, plus clk_summary will quickly get messy.
>
> Agreed. We should probably prepend the device name to the front of the
> clk now when creating in debugfs. I'll throw a patch for that into the
> pile to get that problem out of the way.
If we are supposed to keep globally unique names, maybe we should be keep
things as they are for now.
>
> I'm also beginning to think we should add a way to pass in the of_node
> for a clk when it's registered. Probably need to do that with the
> initdata structure again. That way we can lookup a clk's parents with
> the DT node if they don't call clk_register() with a device and also to
> generate a better unique name for the clk for debug purposes.
I don't get it now, but I'm sure I'll understand better with the code later on
:)
>
> > * How will it behave if 2 clock registers with "foo" and one clock
> > register
> > with the fallback parent "foo" ?
>
> Sorry, I'm not following. The fallback is global so we'll be unable to
> figure out which parent the clk is referring to.
This is what I was thinking as well. But since the global clock namespace is
not the point of this series, the question is off topic. Let's leave this foranother day
> Maybe this is an
> argument for keeping everything globally unique in the clk name space?
For now, sure.
>
> > > Cc: Miquel Raynal <[email protected]>
> > > Cc: Jerome Brunet <[email protected]>
> > > Cc: Russell King <[email protected]>
> > > Cc: Michael Turquette <[email protected]>
> > > Signed-off-by: Stephen Boyd <[email protected]>
> > > ---
> > > @@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk
> > > *parent)
> > > if (core->parent == parent_core)
> > > return true;
> > >
> > > - return match_string(core->parent_names, core->num_parents,
> > > - parent_core->name) >= 0;
> > > + for (i = 0; i < core->num_parents; i++)
> > > + if (!strcmp(core->parents[i].fallback, parent_core->name))
> > > + return true;
>
> This is also messy and not great BTW.
>
> > > +
> > > + return false;
> > > }
> > > EXPORT_SYMBOL_GPL(clk_has_parent);
> > >
> > > @@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file
> > > *s,
> > > void *data)
> > > int i;
> > >
> > > for (i = 0; i < core->num_parents - 1; i++)
> > > - seq_printf(s, "%s ", core->parent_names[i]);
> > > + seq_printf(s, "%s ", core->parents[i].fallback);
> > >
> > > - seq_printf(s, "%s\n", core->parent_names[i]);
> > > + seq_printf(s, "%s\n", core->parents[i].fallback);
> >
> > Wouldn't this show nothing if parent_data is used but fallback is not
> > provided
> > (like in your example when you provide the clk_hw pointer instead) or did
> > I
> > miss something ?
>
> Yes, it will show nothing. Maybe we need to generate the debug name
> somehow?
That would be nice. that or dropping the possible parent entry.
> That code sounds quite awful because we're going in reverse to
> the device through a DT node pointer. Or add an indicator in the output
> if the parent is a global name vs. a local name or a clk debugfs name?
>
> "global_name(g)"
> "local_name(l)"
> "debug_name"
>
> Or if we can't figure anything out then perhaps just ignoring this
> problem for now is fine. It is debugfs after all.
Humm, I don't know if there are any rules regarding debugfs but it bothers me
if we knowingly report wrong values through it. This may lead other developers
to waste their time on this
If we can't report the actual parent_name, we should at very least warn about
it - display [FIXME] or [MISSING] for example.
>
> > >
> > > return 0;
> > > }
> > > @@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev,
> > > struct clk_hw *hw,
> > > return clk;
> > > }
> > >
> > > +static int clk_core_populate_parent_map(struct clk_core *core)
> > > +{
> > > + const struct clk_init_data *init = core->hw->init;
> > > + u8 num_parents = init->num_parents;
> > > + const char * const *parent_names = init->parent_names;
> > > + struct clk_hw **parent_hws = init->parent_hws;
> > > + const struct clk_parent_data *parent_data = init->parent_data;
> > > + int i, ret = 0;
> > > + struct clk_parent_map *parents, *parent;
> > > +
> > > + if (!num_parents)
> > > + return 0;
> > > +
> > > + /*
> > > + * Avoid unnecessary string look-ups of clk_core's possible
> > > parents by
> > > + * having a cache of names/clk_hw pointers to clk_core pointers.
> > > + */
> > > + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> > > + core->parents = parents;
> > > + if (!parents)
> > > + return -ENOMEM;
> > > +
> > > + /* Copy everything over because it might be __initdata */
> > > + for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> > > + if (parent_names) {
> > > + /* throw a WARN if any entries are NULL */
> > > + WARN(!parent_names[i],
> > > + "%s: invalid NULL in %s's
> > > .parent_names\n",
> > > + __func__, core->name);
> > > + parent->fallback = kstrdup_const(parent_names[i],
> > > + GFP_KERNEL);
> > > + if (!parent->fallback) {
> > > + ret = -ENOMEM;
> > > + while (--i >= 0)
> > > + kfree_const(parents[i].fallback);
> > > + }
> > > + } else if (parent_data) {
> > > + parent->hw = parent_data[i].hw;
> > > + parent->name = parent_data[i].name;
> > > + parent->fallback = parent_data[i].fallback;
> >
> > I'm a bit confused by the comment at the top of the loop. Strings in
> > parent_data are not copied over like we used to do with parent_names.
> >
> > Is it allowed to have parent_data in __initdata ? It could be error prone
> > if
> > parent_names and parent_data behaved differently on this.
>
> Good catch, thanks. Will fix.
>
> > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > index 8b84dee942bf..f513f4074a93 100644
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -264,6 +264,18 @@ struct clk_ops {
> > > void (*debug_init)(struct clk_hw *hw, struct dentry
> > > *dentry);
> > > };
> > >
> > > +/**
> > > + * struct clk_parent_data - clk parent information
> > > + * @hw: parent clk_hw pointer (used for clk providers with internal
> > > clks)
> > > + * @name: parent name local to provider registering clk
> > > + * @fallback: globally unique parent name (used as a fallback)
> > > + */
> > > +struct clk_parent_data {
> > > + struct clk_hw *hw;
> > > + const char *name;
> > > + const char *fallback;
> >
> > If I understand correctly, .name and .fallback will be ignored if hw is
> > provided ? Maybe this should be documented somehow ?
>
> Sure. I'll add some documentation to the long portion of the kernel-doc
> here describing priority order.
>
> > > +};
> > > +
> > > /**
> > > * struct clk_init_data - holds init data that's common to all clocks
> > > and
> > > is
> > > * shared between the clock provider and the common clock framework.
> > > @@ -277,7 +289,10 @@ struct clk_ops {
> > > struct clk_init_data {
> > > const char *name;
> > > const struct clk_ops *ops;
> > > - const char * const *parent_names;
> > > + /* Only one of the following three should be assigned */
> > > + const char * const *parent_names; /* If NULL (and
> > > num_parents != 0) look at parent_data and parent_hws */
> > > + const struct clk_parent_data *parent_data;
> > > + struct clk_hw **parent_hws;
> >
> > Isn't parent_hws redundant with parent_data ? you may pass the clk_hw
> > pointer
> > with both, right ?
>
> Yeah it's redundant, but I thought that drivers may not want to waste
> the extra space for pointers if they had all the pointers in hand for
> the parents they care about, or if they had a single parent and just
> wanted to point to that directly.
Fine. Makes sense.
>
> Another thought is to have a union on these three pointers and then a
> flag indicating which method is used:
>
> union {
> const char * const *parent_names;
> const struct clk_parent_data *parent_data;
> struct clk_hw **parent_hws;
> };
>
> #define CLK_PARENTS_POINTERS BIT(3)
> #define CLK_PARENTS_DATA BIT(8)
>
Whatever is simpler :)
If keeping the current solution, we just need to document the priority order.
Quoting Jerome Brunet (2019-01-29 01:34:38)
> On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote:
> > ---
> > drivers/clk/clk.c | 117 ++++++++++++++++++++++++++---------
> > include/linux/clk-provider.h | 9 +++
> > 2 files changed, 96 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 01b36f0851bd..5d82cf25bb29 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2242,14 +2242,84 @@ struct clk *clk_get_parent(struct clk *clk)
> > }
> > EXPORT_SYMBOL_GPL(clk_get_parent);
> >
> > -static struct clk_core *__clk_init_parent(struct clk_core *core)
> > +static struct clk_core *
> > +__clk_init_parent(struct clk_core *core, bool update_orphan)
> > {
> > u8 index = 0;
> > + struct clk_hw *parent_hw = NULL;
> >
> > - if (core->num_parents > 1 && core->ops->get_parent)
> > - index = core->ops->get_parent(core->hw);
> > + if (core->ops->get_parent_hw) {
> > + parent_hw = core->ops->get_parent_hw(core->hw);
> > + /*
> > + * The provider driver doesn't know what the parent is,
> > + * but it's at least something valid, so it's not an
> > + * orphan, just a clk with some unknown parent.
> > + */
>
> I suppose this is the answer the discussion we had last year. I'm not sure it
> answer the problem. In the case I presented, we have no idea wether the
> setting is valid or not.
>
> We can't assume it is `at least something valid`, the value in the mux is just
> something we can't map.
So if you can't map the value in the mux how is that valid? I would
think the mux knows what indexes it has strings for, and if the index
isn't in there it's invalid. Is that not the case here?
>
> Aslo, could you provide an example of what such callback would be, with clk-
> mux maybe ?
Sounds fair. I can convert the clk-mux API to this op. It may be that we
need to make clk_hw_get_parent_by_index() return an error pointer
instead of NULL if it can't find the clk so that we can move the error
codes through this new API.
>
> I don't get how a clock driver will keep track of the clk_hw pointers it is
> connected to. Is there an API for this ? clk-mux must access to clk_core to
> explore his own parent ... which already does not scale well, expect if we
> plan to expose clk_core at some point ?
No we don't want to expose clk_core to provider drivers. It is only for
the use of the clk framework and it's not exposed even as an opaque
pointer. We have that core member of clk_hw but that's just to traverse
from clk_hw to clk_core, and not for anything else.
>
> > + if (!parent_hw && update_orphan)
> > + core->orphan = false;
> > + } else {
> > + if (core->num_parents > 1 && core->ops->get_parent)
>
> I still get why, when num_parents == 1, it is OK to call get_parent_hw() and
> no get_parent(). It does not seems coherent.
I'd rather not change behavior of existing code in this patch, so I took
the route of adding another callback with semantics that we can define
now because there aren't any users. The difference between the two is
made intentionally.
>
> > + index = core->ops->get_parent(core->hw);
> > +
> > + parent_hw = clk_hw_get_parent_by_index(core->hw, index);
> > + }
> > +
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 60c51871b04b..8b84dee942bf 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -155,6 +155,14 @@ struct clk_duty {
> > * multiple parents. It is optional (and unnecessary) for clocks
> > * with 0 or 1 parents.
> > *
> > + * @get_parent_hw: Queries the hardware to determine the parent of a
> > clock. The
> > + * return value is a clk_hw pointer corresponding to
> > + * the parent clock. In short, this function translates the
> > parent
> > + * value read from hardware into a pointer to the clk_hw for that
> > clk.
> > + * Currently only called when the clock is initialized by
> > + * __clk_init. This callback is mandatory for clocks with
> > + * multiple parents. It is optional for clocks with 0 or 1
> > parents.
> > + *
>
> The comments above could imply that get_parent() and get_parent_hw() are both
> mandatory if num_parent > 1. (I don't think so but) Is this your intent ?
It is not the intent. I'll update the docs. Thanks.
On Tue, 2019-01-29 at 13:15 -0800, Stephen Boyd wrote:
> > I suppose this is the answer the discussion we had last year. I'm not sure
> > it
> > answer the problem. In the case I presented, we have no idea wether the
> > setting is valid or not.
> >
> > We can't assume it is `at least something valid`, the value in the mux is
> > just
> > something we can't map.
>
> So if you can't map the value in the mux how is that valid? I would
> think the mux knows what indexes it has strings for, and if the index
> isn't in there it's invalid. Is that not the case here?
It is the value we find in the register after the bootloader. Since we have no
idea what it is, we must assume it is invalid.
The problem was that there only one known parent for this particular clock,
and the CCF quirk (call get_parent() only if num_parent == 1) is making things
difficult for us.
We found a work around to this. We declare a non existing parent, which make
num_parent == 2 and force CCF to call get_parent(), and later on set_parent()
... It works but it sucks.
>
> > Aslo, could you provide an example of what such callback would be, with
> > clk-
> > mux maybe ?
>
> Sounds fair. I can convert the clk-mux API to this op. It may be that we
> need to make clk_hw_get_parent_by_index() return an error pointer
> instead of NULL if it can't find the clk so that we can move the error
> codes through this new API.
Sure, good idea.
If clk_hw_get_parent_by_index() may return an error, it because get_parent()
could provide an invalid index.
Which brings us back to the original point, if it is possible for get_parent()
to return invalid index, which means out of bounds, it shall be called even if
num_parent == 1.
To be coherent, either:
1) the return value of get_parent() *MUST* be valid, it is fine to not call it
if num_parent == 1 because we already know the only possible result.
2) the return value of get_parent *MAY* be invalid, then it should be called
regarless of num_parent.
IMO, (2) is the only valid option because any existing muxes could already be
returning invalid indexes. I know we do on Amlogic (with various muxes, even
with num_parent >= 1) and I'm sure we are not the only ones. Plus a driver
needs to have a mean to tell CCF that things are not as expected.
>
> > I don't get how a clock driver will keep track of the clk_hw pointers it
> > is
> > connected to. Is there an API for this ? clk-mux must access to clk_core
> > to
> > explore his own parent ... which already does not scale well, expect if we
> > plan to expose clk_core at some point ?
>
> No we don't want to expose clk_core to provider drivers. It is only for
> the use of the clk framework and it's not exposed even as an opaque
> pointer. We have that core member of clk_hw but that's just to traverse
> from clk_hw to clk_core, and not for anything else.
>
> > > + if (!parent_hw && update_orphan)
> > > + core->orphan = false;
> > > + } else {
> > > + if (core->num_parents > 1 && core->ops->get_parent)
> >
> > I still get why, when num_parents == 1, it is OK to call get_parent_hw()
> > and
> > no get_parent(). It does not seems coherent.
>
> I'd rather not change behavior of existing code in this patch,
In this patch, I agree
> so I took
> the route of adding another callback with semantics that we can define
> now because there aren't any users. The difference between the two is
> made intentionally.
I still think that the drivers relying on this quirk (only 1 AFAIK) are broken
and should be fixed, rather that keeping the quirk for everyone else.
With this quirk, CCF is making an assumption that might be wrong.
The quirk is very easy put in the get_parent() callback of the said driver, or
even better, don't provide the callback if it should not be called.
I understand the need for a cautious approach. It seems I'm only one with that
issue right now and since I have a work around, there is no rush. But we must
have plan to make it right.
To be clear, I'm not against your new API but I don't think it should be a
reason to keep a broken behavior the framework.
Quoting Jerome Brunet (2019-01-30 01:53:41)
>
> It is the value we find in the register after the bootloader. Since we have no
> idea what it is, we must assume it is invalid.
Ok, so my understanding is correct.
>
> The problem was that there only one known parent for this particular clock,
> and the CCF quirk (call get_parent() only if num_parent == 1) is making things
> difficult for us.
>
> We found a work around to this. We declare a non existing parent, which make
> num_parent == 2 and force CCF to call get_parent(), and later on set_parent()
> ... It works but it sucks.
Yes that is bad.
>
> Sure, good idea.
>
> If clk_hw_get_parent_by_index() may return an error, it because get_parent()
> could provide an invalid index.
>
> Which brings us back to the original point, if it is possible for get_parent()
> to return invalid index, which means out of bounds, it shall be called even if
> num_parent == 1.
>
> To be coherent, either:
> 1) the return value of get_parent() *MUST* be valid, it is fine to not call it
> if num_parent == 1 because we already know the only possible result.
>
> 2) the return value of get_parent *MAY* be invalid, then it should be called
> regarless of num_parent.
>
> IMO, (2) is the only valid option because any existing muxes could already be
> returning invalid indexes. I know we do on Amlogic (with various muxes, even
> with num_parent >= 1) and I'm sure we are not the only ones. Plus a driver
> needs to have a mean to tell CCF that things are not as expected.
Right. Having drivers specify more clks than the kernel knows about or
having the return value of get_parent() be out of range is non-obvious.
I'd rather see any drivers that want to express errors during the parent
initialization phase return an error pointer through the get_parent_hw
clk op instead.
Put another way, if you have any edge case like this you should migrate
to this new clk op and stop using .get_parent and playing tricks with
.num_parents and return values from .get_parent.
>
> >
> > so I took
> > the route of adding another callback with semantics that we can define
> > now because there aren't any users. The difference between the two is
> > made intentionally.
>
> I still think that the drivers relying on this quirk (only 1 AFAIK) are broken
> and should be fixed, rather that keeping the quirk for everyone else.
Agreed. Use the new API?
>
> With this quirk, CCF is making an assumption that might be wrong.
>
> The quirk is very easy put in the get_parent() callback of the said driver, or
> even better, don't provide the callback if it should not be called.
>
> I understand the need for a cautious approach. It seems I'm only one with that
> issue right now and since I have a work around, there is no rush. But we must
> have plan to make it right.
>
> To be clear, I'm not against your new API but I don't think it should be a
> reason to keep a broken behavior the framework.
>
So do you think you can use this new clk_op and ignore the problems with
the .get_parent clk op? Putting effort into fixing the .get_parent
design isn't very useful from my perspective. There's more than just the
problem that we don't call it when .num_parents is 1. There's the
inability to return errors without doing weird things to return an index
out of range and there isn't any way for us to really know if the clk is
an orphan or not. If we can migrate all drivers to use the new clk op
then we can fix these problems too, and deprecate and eventually remove
the broken by design .get_parent clk op API.
BTW, I included this patch in this series because I'm wondering if it
would be useful for there to be a 'parent cookie' pointer that we pass
between the framework and providers instead using a raw index.
struct clk_parent {
struct clk_hw *hw;
};
Then drivers could embed these structs into their parent_data structure
and the framework could pass pointers to these structures in struct
clk_rate_request. That may make it easier for drivers to attach extra
data to the parent for a particular clk when changing parents or rates.
For example, if the index of the parent is not a linear monotonic range
of [0, num_parents) then we could let them throw the index into either a
wrapper structure or a void *data field.
struct my_clk_parent {
struct clk_parent parent;
u32 mask;
u8 select_bit;
};
Or
struct my_clk_parent_data {
u32 mask;
u8 select_bit;
};
struct clk_parent {
struct clk_hw *hw;
void *data;
};
And have data point to struct my_clk_parent. There's code in various clk
drivers that maps between the framework's view of the parent index and
the providers view, i.e. the hardware index. If we already have the
parent_map structure, maybe we should let that live in the provider
driver near the hardware and then just pass a pointer to it so drivers
can do container_of() or ->data accesses to get any driver data they
associate with that parent index.
If we did something like this, it may reduce code further and let us get
rid of the u8 index in the clk framework entirely. But we may want to
have the .get_parent_hw op return a pointer to a struct clk_parent
instead of a struct clk_hw now to make this easier to implement in the
future. Or this could all be overkill and only help a few drivers skip a
mapping step.
On Wed, 2019-01-30 at 13:30 -0800, Stephen Boyd wrote:
> > With this quirk, CCF is making an assumption that might be wrong.
> >
> > The quirk is very easy put in the get_parent() callback of the said
> > driver, or
> > even better, don't provide the callback if it should not be called.
> >
> > I understand the need for a cautious approach. It seems I'm only one with
> > that
> > issue right now and since I have a work around, there is no rush. But we
> > must
> > have plan to make it right.
> >
> > To be clear, I'm not against your new API but I don't think it should be a
> > reason to keep a broken behavior the framework.
> >
>
> So do you think you can use this new clk_op and ignore the problems with
> the .get_parent clk op? Putting effort into fixing the .get_parent
> design isn't very useful from my perspective. There's more than just the
> problem that we don't call it when .num_parents is 1. There's the
> inability to return errors without doing weird things to return an index
> out of range and there isn't any way for us to really know if the clk is
> an orphan or not. If we can migrate all drivers to use the new clk op
> then we can fix these problems too, and deprecate and eventually remove
> the broken by design .get_parent clk op API.
Stephen, I have nothing against your new API, I'm sure it will solve many
issues
I'm also quite sure that, like round_rate() and determine_rate(), migrating to
the new API won't happen overnight. We are likely to still see get_parent()
for a while. I don't understand why we would keep something wrong when it is
that easy to fix.
I have spent quite sometime debugging this weird behavior of CCF, I'd prefer
if it can avoided for others.
Yes, fixing the case I reported does not solves all the problem you have
mentionned. Keeping this bug does not help either, AFAICT.
The fact is that get_parent() already return out of bound values on some
occasion, and we already have to deal with this when converting the index to
parent clk_hw pointer. Doing it in the same way when num_parent == 1 does not
change anything.
I really don't understand why you insist on keeping this special case for
num_parent == 1, when we know it is not coherent.
Considering, that I already proposed the fix, what is the effort here ?
If it is fixing the driver that rely this weird thing, I'd be happy to do it.
Regards
Jerome
Quoting Jerome Brunet (2019-01-31 10:40:07)
> On Wed, 2019-01-30 at 13:30 -0800, Stephen Boyd wrote:
> > > With this quirk, CCF is making an assumption that might be wrong.
> > >
> > > The quirk is very easy put in the get_parent() callback of the said
> > > driver, or
> > > even better, don't provide the callback if it should not be called.
> > >
> > > I understand the need for a cautious approach. It seems I'm only one with
> > > that
> > > issue right now and since I have a work around, there is no rush. But we
> > > must
> > > have plan to make it right.
> > >
> > > To be clear, I'm not against your new API but I don't think it should be a
> > > reason to keep a broken behavior the framework.
> > >
> >
> > So do you think you can use this new clk_op and ignore the problems with
> > the .get_parent clk op? Putting effort into fixing the .get_parent
> > design isn't very useful from my perspective. There's more than just the
> > problem that we don't call it when .num_parents is 1. There's the
> > inability to return errors without doing weird things to return an index
> > out of range and there isn't any way for us to really know if the clk is
> > an orphan or not. If we can migrate all drivers to use the new clk op
> > then we can fix these problems too, and deprecate and eventually remove
> > the broken by design .get_parent clk op API.
>
> Stephen, I have nothing against your new API, I'm sure it will solve many
> issues
>
> I'm also quite sure that, like round_rate() and determine_rate(), migrating to
> the new API won't happen overnight. We are likely to still see get_parent()
> for a while. I don't understand why we would keep something wrong when it is
> that easy to fix.
>
> I have spent quite sometime debugging this weird behavior of CCF, I'd prefer
> if it can avoided for others.
>
> Yes, fixing the case I reported does not solves all the problem you have
> mentionned. Keeping this bug does not help either, AFAICT.
>
> The fact is that get_parent() already return out of bound values on some
> occasion, and we already have to deal with this when converting the index to
> parent clk_hw pointer. Doing it in the same way when num_parent == 1 does not
> change anything.
>
> I really don't understand why you insist on keeping this special case for
> num_parent == 1, when we know it is not coherent.
>
> Considering, that I already proposed the fix, what is the effort here ?
> If it is fixing the driver that rely this weird thing, I'd be happy to do it.
>
>
Ok. I'm happy to merge your patch to always call the .get_parent clk op
when num_parents > 0, but please fix all the drivers and analyze all the
implementations of .get_parent to make sure that they aren't broken by
the change in behavior. Furthermore, please add a debug/warning message
into the code when .get_parent returns a number outside of the range of
[0, num_parents) so that they can be converted to use .get_parent_hw
instead. Ideally there wouldn't be anything returning a parent index
outside the range of possible parents from .get_parent because this
analysis of drivers would find those implementations and migrate them to
.get_parent_hw instead.
In parallel, I'd like to convert all drivers to use .get_parent_hw
instead of .get_parent and then remove the .get_parent clk op right
away. I'll start a sweep of the users of clk_hw_get_parent_by_index() (I
see 50 calls in the tree right now) and see if I can convert them to
handle errors returned from that API, probably by just continuing and
ignoring errors. I'll start doing the same conversion for .round_rate
and .determine_rate so that we can get rid of that duplicate clk op as
well. Hopefully that's a mostly mechanical conversion.
For now I'll move this patch to the end of this series so that it
doesn't hold things up otherwise.
On 1/28/2019 11:10 PM, Stephen Boyd wrote:
> The clk_ops::get_parent function is limited in ability to return errors
> because it returns a u8. A "workaround" to return an error is to return
> a number >= the number of parents of a clk. This will in turn cause the
> framework to "orphan" the clk and make the parent of the clk NULL. This
> isn't really correct, because if an error occurs while reading the
> parents of a clk we should fail the clk registration, instead of
> orphaning the clk and waiting for the clk to appear later.
>
> We really need to have three different return values from the get_parent
> clk op. Something valid for a clk that exists, something invalid for a
> clk that doesn't exist and never will exist or can't be determined
> because the register operation to read the parent failed, and something
> for a clk that doesn't exist because the framework doesn't know about
> what it is. Introduce a new clk_op that can express all of this by
> returning a pointer to the clk_hw of the parent. It's expected that clk
> provider drivers will return a valid pointer when the parent is
> findable, an error pointer like EPROBE_DEFER if their parent provider
> hasn't probed yet but is valid, a NULL pointer if they can't find the
> clk but index is valid, and an error pointer with an appropriate error
> code otherwise.
>
> Cc: Miquel Raynal <[email protected]>
> Cc: Jerome Brunet <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Michael Turquette <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
<snip>
> +static int clk_init_parent(struct clk_core *core)
> +{
> + core->parent = __clk_init_parent(core, true);
> + if (IS_ERR(core->parent))
> + return PTR_ERR(core->parent);
> +
> + /*
> + * Populate core->parent if parent has already been clk_core_init'd. If
> + * parent has not yet been clk_core_init'd then place clk in the orphan
> + * list. If clk doesn't have any parents then place it in the root
> + * clk list.
> + *
> + * Every time a new clk is clk_init'd then we walk the list of orphan
> + * clocks and re-parent any that are children of the clock currently
> + * being clk_init'd.
> + */
> + if (core->parent) {
> + hlist_add_head(&core->child_node,
> + &core->parent->children);
> + core->orphan = core->parent->orphan;
> + } else if (!core->num_parents) {
> + hlist_add_head(&core->child_node, &clk_root_list);
> + core->orphan = false;
> + } else {
> + hlist_add_head(&core->child_node, &clk_orphan_list);
Missing "core->orphan = true;"?
The snippet below had that line. Its not clear why it appears to be
dropped here.
> + }
> +
> + return 0;
> +}
<snip>
> @@ -3073,29 +3143,9 @@ static int __clk_core_init(struct clk_core *core)
> "%s: invalid NULL in %s's .parent_names\n",
> __func__, core->name);
>
> - core->parent = __clk_init_parent(core);
> -
> - /*
> - * Populate core->parent if parent has already been clk_core_init'd. If
> - * parent has not yet been clk_core_init'd then place clk in the orphan
> - * list. If clk doesn't have any parents then place it in the root
> - * clk list.
> - *
> - * Every time a new clk is clk_init'd then we walk the list of orphan
> - * clocks and re-parent any that are children of the clock currently
> - * being clk_init'd.
> - */
> - if (core->parent) {
> - hlist_add_head(&core->child_node,
> - &core->parent->children);
> - core->orphan = core->parent->orphan;
> - } else if (!core->num_parents) {
> - hlist_add_head(&core->child_node, &clk_root_list);
> - core->orphan = false;
> - } else {
> - hlist_add_head(&core->child_node, &clk_orphan_list);
> - core->orphan = true;
> - }
> + ret = clk_init_parent(core);
> + if (ret)
> + goto out;
>
> /*
> * optional platform-specific magic
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On Tue, 2019-02-05 at 16:01 -0800, Stephen Boyd wrote:
> Quoting Jerome Brunet (2019-01-31 10:40:07)
> > On Wed, 2019-01-30 at 13:30 -0800, Stephen Boyd wrote:
> > > > With this quirk, CCF is making an assumption that might be wrong.
> > > >
> > > > The quirk is very easy put in the get_parent() callback of the said
> > > > driver, or
> > > > even better, don't provide the callback if it should not be called.
> > > >
> > > > I understand the need for a cautious approach. It seems I'm only one
> > > > with
> > > > that
> > > > issue right now and since I have a work around, there is no rush. But
> > > > we
> > > > must
> > > > have plan to make it right.
> > > >
> > > > To be clear, I'm not against your new API but I don't think it should
> > > > be a
> > > > reason to keep a broken behavior the framework.
> > > >
> > >
> > > So do you think you can use this new clk_op and ignore the problems with
> > > the .get_parent clk op? Putting effort into fixing the .get_parent
> > > design isn't very useful from my perspective. There's more than just the
> > > problem that we don't call it when .num_parents is 1. There's the
> > > inability to return errors without doing weird things to return an index
> > > out of range and there isn't any way for us to really know if the clk is
> > > an orphan or not. If we can migrate all drivers to use the new clk op
> > > then we can fix these problems too, and deprecate and eventually remove
> > > the broken by design .get_parent clk op API.
> >
> > Stephen, I have nothing against your new API, I'm sure it will solve many
> > issues
> >
> > I'm also quite sure that, like round_rate() and determine_rate(),
> > migrating to
> > the new API won't happen overnight. We are likely to still see
> > get_parent()
> > for a while. I don't understand why we would keep something wrong when it
> > is
> > that easy to fix.
> >
> > I have spent quite sometime debugging this weird behavior of CCF, I'd
> > prefer
> > if it can avoided for others.
> >
> > Yes, fixing the case I reported does not solves all the problem you have
> > mentionned. Keeping this bug does not help either, AFAICT.
> >
> > The fact is that get_parent() already return out of bound values on some
> > occasion, and we already have to deal with this when converting the index
> > to
> > parent clk_hw pointer. Doing it in the same way when num_parent == 1 does
> > not
> > change anything.
> >
> > I really don't understand why you insist on keeping this special case for
> > num_parent == 1, when we know it is not coherent.
> >
> > Considering, that I already proposed the fix, what is the effort here ?
> > If it is fixing the driver that rely this weird thing, I'd be happy to do
> > it.
> >
> >
>
> Ok. I'm happy to merge your patch to always call the .get_parent clk op
> when num_parents > 0, but please fix all the drivers and analyze all the
> implementations of .get_parent to make sure that they aren't broken by
> the change in behavior. Furthermore, please add a debug/warning message
> into the code when .get_parent returns a number outside of the range of
> [0, num_parents) so that they can be converted to use .get_parent_hw
> instead.
Fair enough.
> Ideally there wouldn't be anything returning a parent index
> outside the range of possible parents from .get_parent because this
> analysis of drivers would find those implementations and migrate them to
> .get_parent_hw instead.
>
> In parallel, I'd like to convert all drivers to use .get_parent_hw
> instead of .get_parent and then remove the .get_parent clk op right
> away.
Fine by me. Of course step #1 is not required if you get this is in before.
As long as things are coherent, I'm happy :)
> I'll start a sweep of the users of clk_hw_get_parent_by_index() (I
> see 50 calls in the tree right now) and see if I can convert them to
> handle errors returned from that API, probably by just continuing and
> ignoring errors. I'll start doing the same conversion for .round_rate
> and .determine_rate so that we can get rid of that duplicate clk op as
> well. Hopefully that's a mostly mechanical conversion.
This would be nice !
>
> For now I'll move this patch to the end of this series so that it
> doesn't hold things up otherwise.
It could even be separate series ? with the migration you mentionned above ?
On Tue, 2019-01-29 at 10:56 -0800, Stephen Boyd wrote:
> > > +/**
> > > + * struct clk_parent_data - clk parent information
> > > + * @hw: parent clk_hw pointer (used for clk providers with internal
> > > clks)
> > > + * @name: parent name local to provider registering clk
> > > + * @fallback: globally unique parent name (used as a fallback)
> > > + */
> > > +struct clk_parent_data {
> > > + struct clk_hw *hw;
> > > + const char *name;
> > > + const char *fallback;
One last nitpick about this structure, because I did not figure it out at
first.
'fallback' is what we known as 'name' in CCF so far.
What do you think about renaming 'fallback' to 'name' and 'name' to something
more obvious, like 'of_name' or 'fw_name' or something else ?
> >
> > If I understand correctly, .name and .fallback will be ignored if hw is
> > provided ? Maybe this should be documented somehow ?
>
> Sure. I'll add some documentation to the long portion of the kernel-doc
> here describing priority order.
Anyway, with this patch, I should be able to remove a lot of (ugly) code I
have been writting lately. I'll be happy to test it when you have a v2 ready.
Quoting Jerome Brunet (2019-02-13 01:16:18)
> On Tue, 2019-02-05 at 16:01 -0800, Stephen Boyd wrote:
> > >
> > > I really don't understand why you insist on keeping this special case for
> > > num_parent == 1, when we know it is not coherent.
> > >
> > > Considering, that I already proposed the fix, what is the effort here ?
> > > If it is fixing the driver that rely this weird thing, I'd be happy to do
> > > it.
> > >
> > >
> >
> > Ok. I'm happy to merge your patch to always call the .get_parent clk op
> > when num_parents > 0, but please fix all the drivers and analyze all the
> > implementations of .get_parent to make sure that they aren't broken by
> > the change in behavior. Furthermore, please add a debug/warning message
> > into the code when .get_parent returns a number outside of the range of
> > [0, num_parents) so that they can be converted to use .get_parent_hw
> > instead.
>
> Fair enough.
>
> > Ideally there wouldn't be anything returning a parent index
> > outside the range of possible parents from .get_parent because this
> > analysis of drivers would find those implementations and migrate them to
> > .get_parent_hw instead.
> >
> > In parallel, I'd like to convert all drivers to use .get_parent_hw
> > instead of .get_parent and then remove the .get_parent clk op right
> > away.
>
> Fine by me. Of course step #1 is not required if you get this is in before.
> As long as things are coherent, I'm happy :)
Ok. So does it mean you want everything to be converted over the
.get_parent_hw and then all problems are solved? I think I can use
Coccinelle to convert the callers to pass the index straight into
clk_hw_get_parent_by_index(). I've also stacked a patch on top of this
series to make that API accept a negative index so I can directly chain
that call after the index is figured out.
>
> > I'll start a sweep of the users of clk_hw_get_parent_by_index() (I
> > see 50 calls in the tree right now) and see if I can convert them to
> > handle errors returned from that API, probably by just continuing and
> > ignoring errors. I'll start doing the same conversion for .round_rate
> > and .determine_rate so that we can get rid of that duplicate clk op as
> > well. Hopefully that's a mostly mechanical conversion.
>
> This would be nice !
Great! This isn't as easy to script, but it looks like I'll be bombing
the list with hundreds of patches soon.
>
> >
> > For now I'll move this patch to the end of this series so that it
> > doesn't hold things up otherwise.
>
> It could even be separate series ? with the migration you mentionned above ?
>
Yes that's fine to be a different series. I've reordered the patches now
so this patch comes last and I've also fixed up all callers of
clk_hw_get_parent_by_index() to handle the error case, with lots of
checks for IS_ERR_OR_NULL(). I've also made that API return an error now
and realized that this patch series needs to only do the fallback to the
'fallback' string when there isn't a direct pointer and when the DT
lookup fails with -ENOENT. We should assume that other errors mean that
something is wrong and the lookup actually failed vs. it being a DT
property that hasn't been implemented yet. Hopefully this is clearer
when I repost this series.
Quoting Jeffrey Hugo (2019-02-11 08:09:00)
> On 1/28/2019 11:10 PM, Stephen Boyd wrote:
>
> > +static int clk_init_parent(struct clk_core *core)
> > +{
> > + core->parent = __clk_init_parent(core, true);
> > + if (IS_ERR(core->parent))
> > + return PTR_ERR(core->parent);
> > +
> > + /*
> > + * Populate core->parent if parent has already been clk_core_init'd. If
> > + * parent has not yet been clk_core_init'd then place clk in the orphan
> > + * list. If clk doesn't have any parents then place it in the root
> > + * clk list.
> > + *
> > + * Every time a new clk is clk_init'd then we walk the list of orphan
> > + * clocks and re-parent any that are children of the clock currently
> > + * being clk_init'd.
> > + */
> > + if (core->parent) {
> > + hlist_add_head(&core->child_node,
> > + &core->parent->children);
> > + core->orphan = core->parent->orphan;
> > + } else if (!core->num_parents) {
> > + hlist_add_head(&core->child_node, &clk_root_list);
> > + core->orphan = false;
> > + } else {
> > + hlist_add_head(&core->child_node, &clk_orphan_list);
>
> Missing "core->orphan = true;"?
> The snippet below had that line. Its not clear why it appears to be
> dropped here.
>
Hmm. Weird. I think I may have been getting ahead of myself and moving
the orphan updating code into __clk_init_parent(). I can't remember why
though, so I guess I'll go all the way and move it all into
__clk_init_parent() now. Thanks for pointing it out.
On 2/15/2019 11:47 AM, Stephen Boyd wrote:
> Quoting Jeffrey Hugo (2019-02-11 08:09:00)
>> On 1/28/2019 11:10 PM, Stephen Boyd wrote:
>>
>>> +static int clk_init_parent(struct clk_core *core)
>>> +{
>>> + core->parent = __clk_init_parent(core, true);
>>> + if (IS_ERR(core->parent))
>>> + return PTR_ERR(core->parent);
>>> +
>>> + /*
>>> + * Populate core->parent if parent has already been clk_core_init'd. If
>>> + * parent has not yet been clk_core_init'd then place clk in the orphan
>>> + * list. If clk doesn't have any parents then place it in the root
>>> + * clk list.
>>> + *
>>> + * Every time a new clk is clk_init'd then we walk the list of orphan
>>> + * clocks and re-parent any that are children of the clock currently
>>> + * being clk_init'd.
>>> + */
>>> + if (core->parent) {
>>> + hlist_add_head(&core->child_node,
>>> + &core->parent->children);
>>> + core->orphan = core->parent->orphan;
>>> + } else if (!core->num_parents) {
>>> + hlist_add_head(&core->child_node, &clk_root_list);
>>> + core->orphan = false;
>>> + } else {
>>> + hlist_add_head(&core->child_node, &clk_orphan_list);
>>
>> Missing "core->orphan = true;"?
>> The snippet below had that line. Its not clear why it appears to be
>> dropped here.
>>
>
> Hmm. Weird. I think I may have been getting ahead of myself and moving
> the orphan updating code into __clk_init_parent(). I can't remember why
> though, so I guess I'll go all the way and move it all into
> __clk_init_parent() now. Thanks for pointing it out.
>
No problem.
Just FYI, I've rebased the 8998 mmcc series on top of this, and as far
as I can tell, everything seems to be working great.
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoting Jerome Brunet (2019-02-13 01:32:23)
> On Tue, 2019-01-29 at 10:56 -0800, Stephen Boyd wrote:
> > > > +/**
> > > > + * struct clk_parent_data - clk parent information
> > > > + * @hw: parent clk_hw pointer (used for clk providers with internal
> > > > clks)
> > > > + * @name: parent name local to provider registering clk
> > > > + * @fallback: globally unique parent name (used as a fallback)
> > > > + */
> > > > +struct clk_parent_data {
> > > > + struct clk_hw *hw;
> > > > + const char *name;
> > > > + const char *fallback;
>
> One last nitpick about this structure, because I did not figure it out at
> first.
>
> 'fallback' is what we known as 'name' in CCF so far.
>
> What do you think about renaming 'fallback' to 'name' and 'name' to something
> more obvious, like 'of_name' or 'fw_name' or something else ?
Ok. I'm not super fond of assuming it's the DT specific, so maybe
fw_name is good, or ext_name for external name? Or con_id to match
clkdev?
>
> > >
> > > If I understand correctly, .name and .fallback will be ignored if hw is
> > > provided ? Maybe this should be documented somehow ?
> >
> > Sure. I'll add some documentation to the long portion of the kernel-doc
> > here describing priority order.
>
> Anyway, with this patch, I should be able to remove a lot of (ugly) code I
> have been writting lately. I'll be happy to test it when you have a v2 ready.
>
Great!
Quoting Jeffrey Hugo (2019-02-15 11:29:26)
> On 2/15/2019 11:47 AM, Stephen Boyd wrote:
> > Quoting Jeffrey Hugo (2019-02-11 08:09:00)
> >> On 1/28/2019 11:10 PM, Stephen Boyd wrote:
> >>
> >>> +static int clk_init_parent(struct clk_core *core)
> >>> +{
> >>> + core->parent = __clk_init_parent(core, true);
> >>> + if (IS_ERR(core->parent))
> >>> + return PTR_ERR(core->parent);
> >>> +
> >>> + /*
> >>> + * Populate core->parent if parent has already been clk_core_init'd. If
> >>> + * parent has not yet been clk_core_init'd then place clk in the orphan
> >>> + * list. If clk doesn't have any parents then place it in the root
> >>> + * clk list.
> >>> + *
> >>> + * Every time a new clk is clk_init'd then we walk the list of orphan
> >>> + * clocks and re-parent any that are children of the clock currently
> >>> + * being clk_init'd.
> >>> + */
> >>> + if (core->parent) {
> >>> + hlist_add_head(&core->child_node,
> >>> + &core->parent->children);
> >>> + core->orphan = core->parent->orphan;
> >>> + } else if (!core->num_parents) {
> >>> + hlist_add_head(&core->child_node, &clk_root_list);
> >>> + core->orphan = false;
> >>> + } else {
> >>> + hlist_add_head(&core->child_node, &clk_orphan_list);
> >>
> >> Missing "core->orphan = true;"?
> >> The snippet below had that line. Its not clear why it appears to be
> >> dropped here.
> >>
> >
> > Hmm. Weird. I think I may have been getting ahead of myself and moving
> > the orphan updating code into __clk_init_parent(). I can't remember why
> > though, so I guess I'll go all the way and move it all into
> > __clk_init_parent() now. Thanks for pointing it out.
> >
>
> No problem.
>
> Just FYI, I've rebased the 8998 mmcc series on top of this, and as far
> as I can tell, everything seems to be working great.
>
Cool. Are you using the new way to specify parents or have you
maintained the previous design of using string names for parents?
On 2/15/2019 2:29 PM, Stephen Boyd wrote:
> Quoting Jeffrey Hugo (2019-02-15 11:29:26)
>> On 2/15/2019 11:47 AM, Stephen Boyd wrote:
>>> Quoting Jeffrey Hugo (2019-02-11 08:09:00)
>>>> On 1/28/2019 11:10 PM, Stephen Boyd wrote:
>>>>
>>>>> +static int clk_init_parent(struct clk_core *core)
>>>>> +{
>>>>> + core->parent = __clk_init_parent(core, true);
>>>>> + if (IS_ERR(core->parent))
>>>>> + return PTR_ERR(core->parent);
>>>>> +
>>>>> + /*
>>>>> + * Populate core->parent if parent has already been clk_core_init'd. If
>>>>> + * parent has not yet been clk_core_init'd then place clk in the orphan
>>>>> + * list. If clk doesn't have any parents then place it in the root
>>>>> + * clk list.
>>>>> + *
>>>>> + * Every time a new clk is clk_init'd then we walk the list of orphan
>>>>> + * clocks and re-parent any that are children of the clock currently
>>>>> + * being clk_init'd.
>>>>> + */
>>>>> + if (core->parent) {
>>>>> + hlist_add_head(&core->child_node,
>>>>> + &core->parent->children);
>>>>> + core->orphan = core->parent->orphan;
>>>>> + } else if (!core->num_parents) {
>>>>> + hlist_add_head(&core->child_node, &clk_root_list);
>>>>> + core->orphan = false;
>>>>> + } else {
>>>>> + hlist_add_head(&core->child_node, &clk_orphan_list);
>>>>
>>>> Missing "core->orphan = true;"?
>>>> The snippet below had that line. Its not clear why it appears to be
>>>> dropped here.
>>>>
>>>
>>> Hmm. Weird. I think I may have been getting ahead of myself and moving
>>> the orphan updating code into __clk_init_parent(). I can't remember why
>>> though, so I guess I'll go all the way and move it all into
>>> __clk_init_parent() now. Thanks for pointing it out.
>>>
>>
>> No problem.
>>
>> Just FYI, I've rebased the 8998 mmcc series on top of this, and as far
>> as I can tell, everything seems to be working great.
>>
>
> Cool. Are you using the new way to specify parents or have you
> maintained the previous design of using string names for parents?
>
I fully converted to the new way, although I did it in stages, so both
paths got tested during the conversion. I plan on posting v2 next week.
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.