Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 24 Feb 2022 16:45:55 +0100
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Andy Gross <agross@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Taniya Das <tdas@codeaurora.org>,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 05/16] clk: qcom: gcc-ipq806x: convert parent_names to
 parent_data
Message-ID: <Yhen5cLB32wGmhxu@Ansuel-xps.localdomain>
References: <20220217235703.26641-1-ansuelsmth@gmail.com>
 <20220217235703.26641-6-ansuelsmth@gmail.com>
 <YhcAHQdtvSeROhT+@builder.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YhcAHQdtvSeROhT+@builder.lan>
Precedence: bulk

On Wed, Feb 23, 2022 at 09:48:45PM -0600, Bjorn Andersson wrote:
> On Thu 17 Feb 17:56 CST 2022, Ansuel Smith wrote:
> 
> > Convert parent_names to parent_data to modernize the driver.
> > Where possible use parent_hws directly.
> > 
> 
> Really nice to see this kind of cleanup. Unfortunately I have two
> comments below.
> 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/clk/qcom/gcc-ipq806x.c | 286 ++++++++++++++++++++-------------
> >  1 file changed, 173 insertions(+), 113 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
> > index 34cddf461dba..828383c30322 100644
> > --- a/drivers/clk/qcom/gcc-ipq806x.c
> > +++ b/drivers/clk/qcom/gcc-ipq806x.c
> > @@ -25,6 +25,10 @@
> >  #include "clk-hfpll.h"
> >  #include "reset.h"
> >  
> > +static const struct clk_parent_data gcc_pxo[] = {
> > +	{ .fw_name = "pxo" },
> 
> I expect that this will break booting these boards with existing dtb,
> because there's not yet a clocks <&pxo_board> in the gcc node.
>

Considering the lack of device using ipq806x in the kernel and the fact
that we add the clocks to the global dtsi should we care? The breakage
will be present on boards that use custom kernel anyway so in theory
shouldn't be that hard to refresh the dtsi.

> If you also add .name = "pxo" here that it should still fall back to map
> to the board clock registered in gcc_ipq806x_probe() and once we have
> passed 1-2 kernel releases we can clean out the old mapping.
> 

Just to make sure, you are suggesting to put 2 entry (fw_name AND name)
or replace the fw_name with the generic name variable? 

Anyway thanks for the review!

> > +};
> > +
> >  static struct clk_pll pll0 = {
> >  	.l_reg = 0x30c4,
> >  	.m_reg = 0x30c8,
> > @@ -35,7 +39,7 @@ static struct clk_pll pll0 = {
> >  	.status_bit = 16,
> >  	.clkr.hw.init = &(struct clk_init_data){
> >  		.name = "pll0",
> > -		.parent_names = (const char *[]){ "pxo" },
> > +		.parent_data = gcc_pxo,
> >  		.num_parents = 1,
> >  		.ops = &clk_pll_ops,
> >  	},
> > @@ -46,7 +50,9 @@ static struct clk_regmap pll0_vote = {
> >  	.enable_mask = BIT(0),
> >  	.hw.init = &(struct clk_init_data){
> >  		.name = "pll0_vote",
> > -		.parent_names = (const char *[]){ "pll0" },
> > +		.parent_hws = (const struct clk_hw*[]){
> > +			&pll0.clkr.hw,
> > +		},
> >  		.num_parents = 1,
> >  		.ops = &clk_pll_vote_ops,
> >  	},
> > @@ -62,7 +68,7 @@ static struct clk_pll pll3 = {
> >  	.status_bit = 16,
> >  	.clkr.hw.init = &(struct clk_init_data){
> >  		.name = "pll3",
> > -		.parent_names = (const char *[]){ "pxo" },
> > +		.parent_data = gcc_pxo,
> >  		.num_parents = 1,
> >  		.ops = &clk_pll_ops,
> >  	},
> > @@ -89,7 +95,7 @@ static struct clk_pll pll8 = {
> >  	.status_bit = 16,
> >  	.clkr.hw.init = &(struct clk_init_data){
> >  		.name = "pll8",
> > -		.parent_names = (const char *[]){ "pxo" },
> > +		.parent_data = gcc_pxo,
> >  		.num_parents = 1,
> >  		.ops = &clk_pll_ops,
> >  	},
> > @@ -100,7 +106,9 @@ static struct clk_regmap pll8_vote = {
> >  	.enable_mask = BIT(8),
> >  	.hw.init = &(struct clk_init_data){
> >  		.name = "pll8_vote",
> > -		.parent_names = (const char *[]){ "pll8" },
> > +		.parent_hws = (const struct clk_hw*[]){
> > +			&pll8.clkr.hw,
> > +		},
> >  		.num_parents = 1,
> >  		.ops = &clk_pll_vote_ops,
> >  	},
> > @@ -123,7 +131,7 @@ static struct hfpll_data hfpll0_data = {
> >  static struct clk_hfpll hfpll0 = {
> >  	.d = &hfpll0_data,
> >  	.clkr.hw.init = &(struct clk_init_data){
> > -		.parent_names = (const char *[]){ "pxo" },
> > +		.parent_data = gcc_pxo,
> >  		.num_parents = 1,
> >  		.name = "hfpll0",
> >  		.ops = &clk_ops_hfpll,
> > @@ -149,7 +157,7 @@ static struct hfpll_data hfpll1_data = {
> >  static struct clk_hfpll hfpll1 = {
> >  	.d = &hfpll1_data,
> >  	.clkr.hw.init = &(struct clk_init_data){
> > -		.parent_names = (const char *[]){ "pxo" },
> > +		.parent_data = gcc_pxo,
> >  		.num_parents = 1,
> >  		.name = "hfpll1",
> >  		.ops = &clk_ops_hfpll,
> > @@ -175,7 +183,7 @@ static struct hfpll_data hfpll_l2_data = {
> >  static struct clk_hfpll hfpll_l2 = {
> >  	.d = &hfpll_l2_data,
> >  	.clkr.hw.init = &(struct clk_init_data){
> > -		.parent_names = (const char *[]){ "pxo" },
> > +		.parent_data = gcc_pxo,
> >  		.num_parents = 1,
> >  		.name = "hfpll_l2",
> >  		.ops = &clk_ops_hfpll,
> > @@ -194,7 +202,7 @@ static struct clk_pll pll14 = {
> >  	.status_bit = 16,
> >  	.clkr.hw.init = &(struct clk_init_data){
> >  		.name = "pll14",
> > -		.parent_names = (const char *[]){ "pxo" },
> > +		.parent_data = gcc_pxo,
> >  		.num_parents = 1,
> >  		.ops = &clk_pll_ops,
> >  	},
> > @@ -205,7 +213,9 @@ static struct clk_regmap pll14_vote = {
> >  	.enable_mask = BIT(14),
> >  	.hw.init = &(struct clk_init_data){
> >  		.name = "pll14_vote",
> > -		.parent_names = (const char *[]){ "pll14" },
> > +		.parent_hws = (const struct clk_hw*[]){
> > +			&pll14.clkr.hw,
> > +		},
> >  		.num_parents = 1,
> >  		.ops = &clk_pll_vote_ops,
> >  	},
> > @@ -238,7 +248,7 @@ static struct clk_pll pll18 = {
> >  	.freq_tbl = pll18_freq_tbl,
> >  	.clkr.hw.init = &(struct clk_init_data){
> >  		.name = "pll18",
> > -		.parent_names = (const char *[]){ "pxo" },
> > +		.parent_data = gcc_pxo,
> >  		.num_parents = 1,
> >  		.ops = &clk_pll_ops,
> >  	},
> > @@ -259,9 +269,9 @@ static const struct parent_map gcc_pxo_pll8_map[] = {
> >  	{ P_PLL8, 3 }
> >  };
> >  
> > -static const char * const gcc_pxo_pll8[] = {
> > -	"pxo",
> > -	"pll8_vote",
> > +static const struct clk_parent_data gcc_pxo_pll8[] = {
> > +	{ .fw_name = "pxo" },
> > +	{ .hw = &pll8_vote.hw },
> >  };
> >  
> >  static const struct parent_map gcc_pxo_pll8_cxo_map[] = {
> > @@ -270,10 +280,10 @@ static const struct parent_map gcc_pxo_pll8_cxo_map[] = {
> >  	{ P_CXO, 5 }
> >  };
> >  
> > -static const char * const gcc_pxo_pll8_cxo[] = {
> > -	"pxo",
> > -	"pll8_vote",
> > -	"cxo",
> > +static const struct clk_parent_data gcc_pxo_pll8_cxo[] = {
> > +	{ .fw_name = "pxo" },
> > +	{ .hw = &pll8_vote.hw },
> > +	{ .fw_name = "cxo" },
> 
> As with "pxo", I think you need a .name = "cxo" here as well.
> 
> Regards,
> Bjorn

-- 
	Ansuel