All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christian Marangi <ansuelsmth@gmail.com>
To: Bjorn Andersson <andersson@kernel.org>
Cc: Robert Marko <robimarko@gmail.com>,
	agross@kernel.org, konrad.dybcio@somainline.org,
	mturquette@baylibre.com, sboyd@kernel.org,
	linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] clk: qcom: ipq8074: populate fw_name for all parents
Date: Thu, 10 Nov 2022 14:31:44 +0100	[thread overview]
Message-ID: <636cfd43.050a0220.1e5e0.5c67@mx.google.com> (raw)
In-Reply-To: <20221110032505.hkdlaad2vb7pqpdu@builder.lan>

On Wed, Nov 09, 2022 at 09:25:05PM -0600, Bjorn Andersson wrote:
> On Tue, Nov 08, 2022 at 08:42:17PM +0100, Robert Marko wrote:
> > It appears that having only .name populated in parent_data for clocks
> > which are only globally searchable currently will not work as the clk core
> > won't copy that name if there is no .fw_name present as well.
> > 
> 
> While we want to migrate users to .fw_name and .index, that sounds like
> a bug. Where does this (not) happen?
> 

While we discover this and decided to send a fix, I also check why this
happen and sent a patch about this [0]. The reason is interesting,
in short clk_core_get hardcode the use of fw_name to get parent name and
if that is empty just fails without checking for .name.

I decided to add a warning and a workaround instead of fixing it in
clk_core_get to push devs to use .fw_name. (also I think this should be
documented somewhere but no idea where)

[0] https://patchwork.kernel.org/project/linux-clk/patch/20221108204918.2805-1-ansuelsmth@gmail.com/

> > So, populate .fw_name for all parent clocks in parent_data.
> > 
> > Fixes: ae55ad32e273 ("clk: qcom: ipq8074: convert to parent data")
> > 
> > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com>
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> >  drivers/clk/qcom/gcc-ipq8074.c | 48 +++++++++++++++++-----------------
> >  1 file changed, 24 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/gcc-ipq8074.c b/drivers/clk/qcom/gcc-ipq8074.c
> > index d231866804f6..bf64aa683605 100644
> > --- a/drivers/clk/qcom/gcc-ipq8074.c
> > +++ b/drivers/clk/qcom/gcc-ipq8074.c
> > @@ -1137,7 +1137,7 @@ static const struct freq_tbl ftbl_nss_noc_bfdcd_clk_src[] = {
> >  
> >  static const struct clk_parent_data gcc_xo_bias_pll_nss_noc_clk_gpll0_gpll2[] = {
> >  	{ .fw_name = "xo", .name = "xo" },
> > -	{ .name = "bias_pll_nss_noc_clk" },
> > +	{ .fw_name = "bias_pll_nss_noc_clk", .name = "bias_pll_nss_noc_clk" },
> >  	{ .hw = &gpll0.clkr.hw },
> >  	{ .hw = &gpll2.clkr.hw },
> >  };
> > @@ -1362,7 +1362,7 @@ static const struct freq_tbl ftbl_nss_ppe_clk_src[] = {
> >  
> >  static const struct clk_parent_data gcc_xo_bias_gpll0_gpll4_nss_ubi32[] = {
> >  	{ .fw_name = "xo", .name = "xo" },
> > -	{ .name = "bias_pll_cc_clk" },
> > +	{ .fw_name = "bias_pll_cc_clk", .name = "bias_pll_cc_clk" },
> >  	{ .hw = &gpll0.clkr.hw },
> >  	{ .hw = &gpll4.clkr.hw },
> >  	{ .hw = &nss_crypto_pll.clkr.hw },
> > @@ -1413,10 +1413,10 @@ static const struct freq_tbl ftbl_nss_port1_rx_clk_src[] = {
> >  
> >  static const struct clk_parent_data gcc_xo_uniphy0_rx_tx_ubi32_bias[] = {
> >  	{ .fw_name = "xo", .name = "xo" },
> > -	{ .name = "uniphy0_gcc_rx_clk" },
> > -	{ .name = "uniphy0_gcc_tx_clk" },
> > +	{ .fw_name = "uniphy0_gcc_rx_clk", .name = "uniphy0_gcc_rx_clk" },
> > +	{ .fw_name = "uniphy0_gcc_tx_clk", .name = "uniphy0_gcc_tx_clk" },
> >  	{ .hw = &ubi32_pll.clkr.hw },
> > -	{ .name = "bias_pll_cc_clk" },
> > +	{ .fw_name = "bias_pll_cc_clk", .name = "bias_pll_cc_clk" },
> >  };
> >  
> >  static const struct parent_map gcc_xo_uniphy0_rx_tx_ubi32_bias_map[] = {
> > @@ -1465,10 +1465,10 @@ static const struct freq_tbl ftbl_nss_port1_tx_clk_src[] = {
> >  
> >  static const struct clk_parent_data gcc_xo_uniphy0_tx_rx_ubi32_bias[] = {
> >  	{ .fw_name = "xo", .name = "xo" },
> > -	{ .name = "uniphy0_gcc_tx_clk" },
> > -	{ .name = "uniphy0_gcc_rx_clk" },
> > +	{ .fw_name = "uniphy0_gcc_tx_clk", .name = "uniphy0_gcc_tx_clk" },
> > +	{ .fw_name = "uniphy0_gcc_rx_clk", .name = "uniphy0_gcc_rx_clk" },
> >  	{ .hw = &ubi32_pll.clkr.hw },
> > -	{ .name = "bias_pll_cc_clk" },
> > +	{ .fw_name = "bias_pll_cc_clk", .name = "bias_pll_cc_clk" },
> >  };
> >  
> >  static const struct parent_map gcc_xo_uniphy0_tx_rx_ubi32_bias_map[] = {
> > @@ -1696,12 +1696,12 @@ static const struct freq_tbl ftbl_nss_port5_rx_clk_src[] = {
> >  
> >  static const struct clk_parent_data gcc_xo_uniphy0_rx_tx_uniphy1_rx_tx_ubi32_bias[] = {
> >  	{ .fw_name = "xo", .name = "xo" },
> > -	{ .name = "uniphy0_gcc_rx_clk" },
> > -	{ .name = "uniphy0_gcc_tx_clk" },
> > -	{ .name = "uniphy1_gcc_rx_clk" },
> > -	{ .name = "uniphy1_gcc_tx_clk" },
> > +	{ .fw_name = "uniphy0_gcc_rx_clk", .name = "uniphy0_gcc_rx_clk" },
> > +	{ .fw_name = "uniphy0_gcc_tx_clk", .name = "uniphy0_gcc_tx_clk" },
> > +	{ .fw_name = "uniphy1_gcc_rx_clk", .name = "uniphy1_gcc_rx_clk" },
> > +	{ .fw_name = "uniphy1_gcc_tx_clk", .name = "uniphy1_gcc_tx_clk" },
> >  	{ .hw = &ubi32_pll.clkr.hw },
> > -	{ .name = "bias_pll_cc_clk" },
> > +	{ .fw_name = "bias_pll_cc_clk", .name = "bias_pll_cc_clk" },
> >  };
> >  
> >  static const struct parent_map
> > @@ -1758,12 +1758,12 @@ static const struct freq_tbl ftbl_nss_port5_tx_clk_src[] = {
> >  
> >  static const struct clk_parent_data gcc_xo_uniphy0_tx_rx_uniphy1_tx_rx_ubi32_bias[] = {
> >  	{ .fw_name = "xo", .name = "xo" },
> > -	{ .name = "uniphy0_gcc_tx_clk" },
> > -	{ .name = "uniphy0_gcc_rx_clk" },
> > -	{ .name = "uniphy1_gcc_tx_clk" },
> > -	{ .name = "uniphy1_gcc_rx_clk" },
> > +	{ .fw_name = "uniphy0_gcc_tx_clk", .name = "uniphy0_gcc_tx_clk" },
> > +	{ .fw_name = "uniphy0_gcc_rx_clk", .name = "uniphy0_gcc_rx_clk" },
> > +	{ .fw_name = "uniphy1_gcc_tx_clk", .name = "uniphy1_gcc_tx_clk" },
> > +	{ .fw_name = "uniphy1_gcc_rx_clk", .name = "uniphy1_gcc_rx_clk" },
> >  	{ .hw = &ubi32_pll.clkr.hw },
> > -	{ .name = "bias_pll_cc_clk" },
> > +	{ .fw_name = "bias_pll_cc_clk", .name = "bias_pll_cc_clk" },
> >  };
> >  
> >  static const struct parent_map
> > @@ -1820,10 +1820,10 @@ static const struct freq_tbl ftbl_nss_port6_rx_clk_src[] = {
> >  
> >  static const struct clk_parent_data gcc_xo_uniphy2_rx_tx_ubi32_bias[] = {
> >  	{ .fw_name = "xo", .name = "xo" },
> > -	{ .name = "uniphy2_gcc_rx_clk" },
> > -	{ .name = "uniphy2_gcc_tx_clk" },
> > +	{ .fw_name = "uniphy2_gcc_rx_clk", .name = "uniphy2_gcc_rx_clk" },
> > +	{ .fw_name = "uniphy2_gcc_tx_clk", .name = "uniphy2_gcc_tx_clk" },
> >  	{ .hw = &ubi32_pll.clkr.hw },
> > -	{ .name = "bias_pll_cc_clk" },
> > +	{ .fw_name = "bias_pll_cc_clk", .name = "bias_pll_cc_clk" },
> >  };
> >  
> >  static const struct parent_map gcc_xo_uniphy2_rx_tx_ubi32_bias_map[] = {
> > @@ -1877,10 +1877,10 @@ static const struct freq_tbl ftbl_nss_port6_tx_clk_src[] = {
> >  
> >  static const struct clk_parent_data gcc_xo_uniphy2_tx_rx_ubi32_bias[] = {
> >  	{ .fw_name = "xo", .name = "xo" },
> > -	{ .name = "uniphy2_gcc_tx_clk" },
> > -	{ .name = "uniphy2_gcc_rx_clk" },
> > +	{ .fw_name = "uniphy2_gcc_tx_clk", .name = "uniphy2_gcc_tx_clk" },
> > +	{ .fw_name = "uniphy2_gcc_rx_clk", .name = "uniphy2_gcc_rx_clk" },
> >  	{ .hw = &ubi32_pll.clkr.hw },
> > -	{ .name = "bias_pll_cc_clk" },
> > +	{ .fw_name = "bias_pll_cc_clk", .name = "bias_pll_cc_clk" },
> >  };
> >  
> >  static const struct parent_map gcc_xo_uniphy2_tx_rx_ubi32_bias_map[] = {
> > -- 
> > 2.38.1
> > 

-- 
	Ansuel

      reply	other threads:[~2022-11-10 13:31 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-08 19:42 [PATCH] clk: qcom: ipq8074: populate fw_name for all parents Robert Marko
2022-11-10  3:25 ` Bjorn Andersson
2022-11-10 13:31   ` Christian Marangi [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=636cfd43.050a0220.1e5e0.5c67@mx.google.com \
    --to=ansuelsmth@gmail.com \
    --cc=agross@kernel.org \
    --cc=andersson@kernel.org \
    --cc=konrad.dybcio@somainline.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=robimarko@gmail.com \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.