| Message ID | 20220711104719.40939-2-robimarko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/6] clk: qcom: clk-rcg2: add rcg2 mux ops | expand |
On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote: > > While working on IPQ8074 APSS driver it was discovered that IPQ6018 and > IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is > currently broken. > > More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux > clock. > However after debugging why it was always stuck at 800Mhz, it was figured > out that its not regmap_mux compatible at all. > It is a simple mux but it uses RCG2 register layout and control bits, so To utilize control bits, you probably should also use > utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not > having to provide a dummy frequency table. Could you please clarify this. Your new rcg2 ops seems to be literally equivalent to the clk_regmap_mux_closest_ops provided the shift and width are set correctly.. > While we are here, use ARRAY_SIZE for number of parents. > > Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards. > > Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller") > Signed-off-by: Robert Marko <robimarko@gmail.com> > --- > drivers/clk/qcom/apss-ipq6018.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c > index d78ff2f310bf..be952d417ded 100644 > --- a/drivers/clk/qcom/apss-ipq6018.c > +++ b/drivers/clk/qcom/apss-ipq6018.c > @@ -16,7 +16,7 @@ > #include "clk-regmap.h" > #include "clk-branch.h" > #include "clk-alpha-pll.h" > -#include "clk-regmap-mux.h" > +#include "clk-rcg.h" > > enum { > P_XO, > @@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = { > { P_APSS_PLL_EARLY, 5 }, > }; > > -static struct clk_regmap_mux apcs_alias0_clk_src = { > - .reg = 0x0050, > - .width = 3, > - .shift = 7, Judging from rcg2 ops, .shift should be set to 8. > +static struct clk_rcg2 apcs_alias0_clk_src = { > + .cmd_rcgr = 0x0050, > + .hid_width = 5, > .parent_map = parents_apcs_alias0_clk_src_map, > .clkr.hw.init = &(struct clk_init_data){ > .name = "apcs_alias0_clk_src", > .parent_data = parents_apcs_alias0_clk_src, > - .num_parents = 2, > - .ops = &clk_regmap_mux_closest_ops, > + .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src), > + .ops = &clk_rcg2_mux_closest_ops, > .flags = CLK_SET_RATE_PARENT, > }, > }; > -- > 2.36.1 > -- With best wishes Dmitry
On Mon, 11 Jul 2022 at 14:48, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote: > > > > While working on IPQ8074 APSS driver it was discovered that IPQ6018 and > > IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is > > currently broken. > > > > More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux > > clock. > > However after debugging why it was always stuck at 800Mhz, it was figured > > out that its not regmap_mux compatible at all. > > It is a simple mux but it uses RCG2 register layout and control bits, so > > To utilize control bits, you probably should also use Hi, I am not really sure what you mean here? > > > utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not > > having to provide a dummy frequency table. > > Could you please clarify this. Your new rcg2 ops seems to be literally > equivalent to the clk_regmap_mux_closest_ops provided the shift and > width are set correctly.. Well, I have tried playing with the clk_regmap_mux_closest_ops but I just cannot get it to work. The width like you pointed out should be 8, register offset is currently pointing at the RCG control register and not the CFG one, so it obviously does not work. Setting the register to 0x54 and shift to 8 will just fail silently, leaving the shift at 7 and correcting the register won't work as RCG control bits are not utilized at all with regmap_mux and DIRTY_CFG is active when I manually look at the register. So, I am really not sure how clk_regmap_mux_closest_ops are supposed to work here at all. Regards, Robert > > > While we are here, use ARRAY_SIZE for number of parents. > > > > Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards. > > > > Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller") > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > --- > > drivers/clk/qcom/apss-ipq6018.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c > > index d78ff2f310bf..be952d417ded 100644 > > --- a/drivers/clk/qcom/apss-ipq6018.c > > +++ b/drivers/clk/qcom/apss-ipq6018.c > > @@ -16,7 +16,7 @@ > > #include "clk-regmap.h" > > #include "clk-branch.h" > > #include "clk-alpha-pll.h" > > -#include "clk-regmap-mux.h" > > +#include "clk-rcg.h" > > > > enum { > > P_XO, > > @@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = { > > { P_APSS_PLL_EARLY, 5 }, > > }; > > > > -static struct clk_regmap_mux apcs_alias0_clk_src = { > > - .reg = 0x0050, > > - .width = 3, > > - .shift = 7, > > Judging from rcg2 ops, .shift should be set to 8. > > > +static struct clk_rcg2 apcs_alias0_clk_src = { > > + .cmd_rcgr = 0x0050, > > + .hid_width = 5, > > .parent_map = parents_apcs_alias0_clk_src_map, > > .clkr.hw.init = &(struct clk_init_data){ > > .name = "apcs_alias0_clk_src", > > .parent_data = parents_apcs_alias0_clk_src, > > - .num_parents = 2, > > - .ops = &clk_regmap_mux_closest_ops, > > + .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src), > > + .ops = &clk_rcg2_mux_closest_ops, > > .flags = CLK_SET_RATE_PARENT, > > }, > > }; > > -- > > 2.36.1 > > > > > -- > With best wishes > Dmitry
On Mon, 11 Jul 2022 at 16:23, Robert Marko <robimarko@gmail.com> wrote: > > On Mon, 11 Jul 2022 at 14:48, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote: > > > > > > While working on IPQ8074 APSS driver it was discovered that IPQ6018 and > > > IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is > > > currently broken. > > > > > > More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux > > > clock. > > > However after debugging why it was always stuck at 800Mhz, it was figured > > > out that its not regmap_mux compatible at all. > > > It is a simple mux but it uses RCG2 register layout and control bits, so > > > > To utilize control bits, you probably should also use > > Hi, > I am not really sure what you mean here? Ugh, excuse me. Sent the message too early. I mean that to utilize RCG2 control bits, you probably also need to use clk_rcg2_is_enabled, etc. > > > > > > utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not > > > having to provide a dummy frequency table. > > > > Could you please clarify this. Your new rcg2 ops seems to be literally > > equivalent to the clk_regmap_mux_closest_ops provided the shift and > > width are set correctly.. > > Well, I have tried playing with the clk_regmap_mux_closest_ops but I > just cannot get it > to work. > > The width like you pointed out should be 8, register offset is > currently pointing at the RCG control > register and not the CFG one, so it obviously does not work. > > Setting the register to 0x54 and shift to 8 will just fail silently, > leaving the shift at 7 and correcting > the register won't work as RCG control bits are not utilized at all > with regmap_mux and DIRTY_CFG > is active when I manually look at the register. Ok, I missed the update_cfg part. So, yes, this looks correct. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > So, I am really not sure how clk_regmap_mux_closest_ops are supposed > to work here at all. > > Regards, > Robert > > > > > While we are here, use ARRAY_SIZE for number of parents. > > > > > > Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards. > > > > > > Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller") > > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > > --- > > > drivers/clk/qcom/apss-ipq6018.c | 13 ++++++------- > > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c > > > index d78ff2f310bf..be952d417ded 100644 > > > --- a/drivers/clk/qcom/apss-ipq6018.c > > > +++ b/drivers/clk/qcom/apss-ipq6018.c > > > @@ -16,7 +16,7 @@ > > > #include "clk-regmap.h" > > > #include "clk-branch.h" > > > #include "clk-alpha-pll.h" > > > -#include "clk-regmap-mux.h" > > > +#include "clk-rcg.h" > > > > > > enum { > > > P_XO, > > > @@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = { > > > { P_APSS_PLL_EARLY, 5 }, > > > }; > > > > > > -static struct clk_regmap_mux apcs_alias0_clk_src = { > > > - .reg = 0x0050, > > > - .width = 3, > > > - .shift = 7, > > > > Judging from rcg2 ops, .shift should be set to 8. > > > > > +static struct clk_rcg2 apcs_alias0_clk_src = { > > > + .cmd_rcgr = 0x0050, > > > + .hid_width = 5, > > > .parent_map = parents_apcs_alias0_clk_src_map, > > > .clkr.hw.init = &(struct clk_init_data){ > > > .name = "apcs_alias0_clk_src", > > > .parent_data = parents_apcs_alias0_clk_src, > > > - .num_parents = 2, > > > - .ops = &clk_regmap_mux_closest_ops, > > > + .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src), > > > + .ops = &clk_rcg2_mux_closest_ops, > > > .flags = CLK_SET_RATE_PARENT, > > > }, > > > }; > > > -- > > > 2.36.1 > > > > > > > > > -- > > With best wishes > > Dmitry
diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c index d78ff2f310bf..be952d417ded 100644 --- a/drivers/clk/qcom/apss-ipq6018.c +++ b/drivers/clk/qcom/apss-ipq6018.c @@ -16,7 +16,7 @@ #include "clk-regmap.h" #include "clk-branch.h" #include "clk-alpha-pll.h" -#include "clk-regmap-mux.h" +#include "clk-rcg.h" enum { P_XO, @@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = { { P_APSS_PLL_EARLY, 5 }, }; -static struct clk_regmap_mux apcs_alias0_clk_src = { - .reg = 0x0050, - .width = 3, - .shift = 7, +static struct clk_rcg2 apcs_alias0_clk_src = { + .cmd_rcgr = 0x0050, + .hid_width = 5, .parent_map = parents_apcs_alias0_clk_src_map, .clkr.hw.init = &(struct clk_init_data){ .name = "apcs_alias0_clk_src", .parent_data = parents_apcs_alias0_clk_src, - .num_parents = 2, - .ops = &clk_regmap_mux_closest_ops, + .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src), + .ops = &clk_rcg2_mux_closest_ops, .flags = CLK_SET_RATE_PARENT, }, };
While working on IPQ8074 APSS driver it was discovered that IPQ6018 and IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is currently broken. More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux clock. However after debugging why it was always stuck at 800Mhz, it was figured out that its not regmap_mux compatible at all. It is a simple mux but it uses RCG2 register layout and control bits, so utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not having to provide a dummy frequency table. While we are here, use ARRAY_SIZE for number of parents. Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards. Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller") Signed-off-by: Robert Marko <robimarko@gmail.com> --- drivers/clk/qcom/apss-ipq6018.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)