| Message ID | 20220321231548.14276-4-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Modernize rest of the krait drivers | expand |
On Wed, 13 Apr 2022 at 20:00, Ansuel Smith <ansuelsmth@gmail.com> wrote: > > On Thu, Mar 24, 2022 at 06:22:29PM -0700, Stephen Boyd wrote: > > Quoting Ansuel Smith (2022-03-24 18:13:49) > > > On Thu, Mar 24, 2022 at 06:10:35PM -0700, Stephen Boyd wrote: > > > > Quoting Ansuel Smith (2022-03-21 16:15:33) > > > > > PXO_SRC is currently defined in the gcc include and referenced in the > > > > > ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel > > > > > panic if a driver starts to actually use it. > > > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > > --- > > > > > > > > What is this patch about? clk providers shouldn't be calling clk_get(). > > > > > > > > > > If pxo is passed as a clock in dts and defined as a fixed clock, what > > > should be used? > > > > clk_parent_data > > Sorry but I'm not following you. No idea if you missed the cover letter > where i describe the problem with PXO_SRC. > > The problem here is that > - In DTS we have node that reference <&gcc PXO_SRC> > But > - gcc driver NEVER defined PXO_SRC > As > - PXO_SRC is actually pxo_board that should be defined as a fixed-clock > in dts or is defined using qcom_cc_register_board_clk. > > So in theory we should just put in PXO_SRC the clk hw of the > fixed-clock. That is why I'm using clk_get(). I can use __clk_lookup() > as an alternative but I really can't find a way to get the clock defined > from DTS or qcom_cc_register_board_clk. > > (I have the same exact problem with the cpu qsb clock where is defined > using fixed-clock API but can also defined directly in DTS and I have to > use clk_get()) > > I'm totally missing something so I would love some hint on how to solve > this. When we were doing such conversion for other platforms, we pointed clock consumers to the board clocks directly. There is no need to go through the gcc to fetch pxo. Instead you can use a <&pxo_board> in the dts directly. Typically the sequence is the following: - Minor cleanup of the clock-controller driver (ARRAY_SIZE(parent_data), removal of unused clock sources, unused enum entries, etc) - update drivers to use both .name and .fw_name in replacement of parent_names. Use parent_hws where possible. - update dtsi to reference clocks using clocks/clock-names properties. Pass board/rpmh/rpm clocks directly to their consumers without bandaids in the gcc driver. - (optionally) after several major releases drop parent_data.name completely. I think we mostly skipped this, since it provides no gain. This way you don't have to play around clk_get to return PXO_SRC from gcc clock-controller.
On Wed, Apr 13, 2022 at 08:32:21PM +0300, Dmitry Baryshkov wrote: > On Wed, 13 Apr 2022 at 20:00, Ansuel Smith <ansuelsmth@gmail.com> wrote: > > > > On Thu, Mar 24, 2022 at 06:22:29PM -0700, Stephen Boyd wrote: > > > Quoting Ansuel Smith (2022-03-24 18:13:49) > > > > On Thu, Mar 24, 2022 at 06:10:35PM -0700, Stephen Boyd wrote: > > > > > Quoting Ansuel Smith (2022-03-21 16:15:33) > > > > > > PXO_SRC is currently defined in the gcc include and referenced in the > > > > > > ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel > > > > > > panic if a driver starts to actually use it. > > > > > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > > > --- > > > > > > > > > > What is this patch about? clk providers shouldn't be calling clk_get(). > > > > > > > > > > > > > If pxo is passed as a clock in dts and defined as a fixed clock, what > > > > should be used? > > > > > > clk_parent_data > > > > Sorry but I'm not following you. No idea if you missed the cover letter > > where i describe the problem with PXO_SRC. > > > > The problem here is that > > - In DTS we have node that reference <&gcc PXO_SRC> > > But > > - gcc driver NEVER defined PXO_SRC > > As > > - PXO_SRC is actually pxo_board that should be defined as a fixed-clock > > in dts or is defined using qcom_cc_register_board_clk. > > > > So in theory we should just put in PXO_SRC the clk hw of the > > fixed-clock. That is why I'm using clk_get(). I can use __clk_lookup() > > as an alternative but I really can't find a way to get the clock defined > > from DTS or qcom_cc_register_board_clk. > > > > (I have the same exact problem with the cpu qsb clock where is defined > > using fixed-clock API but can also defined directly in DTS and I have to > > use clk_get()) > > > > I'm totally missing something so I would love some hint on how to solve > > this. > > When we were doing such conversion for other platforms, we pointed > clock consumers to the board clocks directly. There is no need to go > through the gcc to fetch pxo. > Instead you can use a <&pxo_board> in the dts directly. Typically the > sequence is the following: > - Minor cleanup of the clock-controller driver > (ARRAY_SIZE(parent_data), removal of unused clock sources, unused enum > entries, etc) > - update drivers to use both .name and .fw_name in replacement of > parent_names. Use parent_hws where possible. > - update dtsi to reference clocks using clocks/clock-names properties. > Pass board/rpmh/rpm clocks directly to their consumers without > bandaids in the gcc driver. > - (optionally) after several major releases drop parent_data.name > completely. I think we mostly skipped this, since it provides no gain. > > This way you don't have to play around clk_get to return PXO_SRC from > gcc clock-controller. > > -- > With best wishes > Dmitry Thanks for the list of steps to do this kind of cleanup.
diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c index 27f6d7626abb..7271d3afdc89 100644 --- a/drivers/clk/qcom/gcc-ipq806x.c +++ b/drivers/clk/qcom/gcc-ipq806x.c @@ -26,6 +26,8 @@ #include "clk-hfpll.h" #include "reset.h" +static struct clk_regmap pxo = { }; + static struct clk_pll pll0 = { .l_reg = 0x30c4, .m_reg = 0x30c8, @@ -2754,6 +2756,7 @@ static struct clk_dyn_rcg ubi32_core2_src_clk = { }; static struct clk_regmap *gcc_ipq806x_clks[] = { + [PXO_SRC] = NULL, [PLL0] = &pll0.clkr, [PLL0_VOTE] = &pll0_vote, [PLL3] = &pll3.clkr, @@ -3083,6 +3086,10 @@ static int gcc_ipq806x_probe(struct platform_device *pdev) if (ret) return ret; + clk = clk_get(dev, "pxo"); + pxo.hw = *__clk_get_hw(clk); + gcc_ipq806x_clks[PXO_SRC] = &pxo; + regmap = dev_get_regmap(dev, NULL); if (!regmap) return -ENODEV;
PXO_SRC is currently defined in the gcc include and referenced in the ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel panic if a driver starts to actually use it. Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/clk/qcom/gcc-ipq806x.c | 7 +++++++ 1 file changed, 7 insertions(+)