On Tue 24 Aug 13:46 PDT 2021, Marijn Suijten wrote:
> Hi Bjorn,
>
> Thanks for this cleanup, that's needed and much appreciated!
>
> On 8/24/21 5:06 PM, Bjorn Andersson wrote:
> > Using parent_data and parent_hws, instead of parent_names, does protect
> > against some cases of incompletely defined clock trees. While it turns
> > out that the bug being chased this time was totally unrelated, this
> > patch converts the SDM660 GCC driver to avoid such issues.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>
>
> Tested-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
>
> On the Sony Xperia XA2 Ultra, bar the necessary change in the 14NM DSI PHY
> driver commented below.
>
> > [..]
> > -
> > -static struct clk_fixed_factor xo = {
> > - .mult = 1,
> > - .div = 1,
> > - .hw.init = &(struct clk_init_data){
> > - .name = "xo",
> > - .parent_names = (const char *[]){ "xo_board" },
> > - .num_parents = 1,
> > - .ops = &clk_fixed_factor_ops,
> > - },
> > -};
>
>
> Removing the global "xo" clock makes it so that our 14nm DSI PHY does not
> have a parent clock anymore, as the clock is called "xo_board" nowadays
> ("xo" in the position of fw_name is, as you know, only local to this driver
> because it is named that way in the clock-names property). We (SoMainline)
> suffer the same DSI PHY hardcoding issue on many other boards and are at
> this point investigating whether to provide &xo_board in DT like any other
> sane driver. Do you happen to know if work is already underway to tackle
> this?
>
As far as I can tell most other platforms doesn't define "xo" either.
E.g. according to debugfs dsi0vco_clk doesn't have a parent on sdm845...
Sounds like we should update the dsi phys to specify a fw_name and
update binding and dts to provide this...
Does this cause a noticeable regression or it's just that we have a
dangling clock?
> > static struct clk_alpha_pll gpll0_early = {
> > .offset = 0x0,
> > .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
> > @@ -158,7 +35,9 @@ static struct clk_alpha_pll gpll0_early = {
> > .enable_mask = BIT(0),
> > .hw.init = &(struct clk_init_data){
> > .name = "gpll0_early",
> > - .parent_names = (const char *[]){ "xo" },
> > + .parent_data = &(const struct clk_parent_data){
> > + .fw_name = "xo",
> > + },
>
>
> I wish we could use .parent_names for a list of .fw_name's too
Afaict specifying "name" in struct clk_parent_data is the same as using
parent_names. But I'm not up to speed on the details of how to migrate
the dsi phys.
> > [..]
> > @@ -265,7 +270,7 @@ static struct clk_rcg2 blsp1_qup1_i2c_apps_clk_src = {
> > .freq_tbl = ftbl_blsp1_qup1_i2c_apps_clk_src,
> > .clkr.hw.init = &(struct clk_init_data){
> > .name = "blsp1_qup1_i2c_apps_clk_src",
> > - .parent_names = gcc_parent_names_xo_gpll0_gpll0_early_div,
> > + .parent_data = gcc_parent_data_xo_gpll0_gpll0_early_div,
> > .num_parents = 3,
>
>
> How about using ARRAY_SIZE(gcc_parent_data_xo_gpll0_gpll0_early_div) now?
> Same for every other occurrence of this pattern.
>
I omitted that because it felt unrelated to the change I was doing, but
it could certainly be done.
Regards,
Bjorn
