[PATCH v2] clk: qcom: gcc-sc8280xp: add cxo as parent for three ufs ref clks

[PATCH v2] clk: qcom: gcc-sc8280xp: add cxo as parent for three ufs ref clks

[Shazad Hussain]

Added parent_data as CXO for three UFS reference clocks named,
gcc_ufs_ref_clkref_clk, gcc_ufs_card_clkref_clk and
gcc_ufs_1_card_clkref_clk.

Fixes: d65d005f9a6c ("clk: qcom: add sc8280xp GCC driver")
Link: https://lore.kernel.org/lkml/Y2Tber39cHuOSR%2FW@hovoldconsulting.com/
Signed-off-by: Shazad Hussain <quic_shazhuss@quicinc.com>
---
Changes since v1:
- Renamed patch subject. Did not include Brian's T-b comment.
- Added parent data for two ufs phy ref clocks.

v1 of this patch can be found at
https://lore.kernel.org/all/20221030142333.31019-1-quic_shazhuss@quicinc.com/

used below patches for verification on next-20221114
https://lore.kernel.org/lkml/20221104092045.17410-2-johan+linaro@kernel.org/
https://lore.kernel.org/lkml/20221104092045.17410-3-johan+linaro@kernel.org/
https://lore.kernel.org/lkml/20221111113732.461881-1-thierry.reding@gmail.com/

 drivers/clk/qcom/gcc-sc8280xp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
index a18ed88f3b82..b3198784e1c3 100644
--- a/drivers/clk/qcom/gcc-sc8280xp.c
+++ b/drivers/clk/qcom/gcc-sc8280xp.c
@@ -5364,6 +5364,8 @@ static struct clk_branch gcc_ufs_1_card_clkref_clk = {
 		.enable_mask = BIT(0),
 		.hw.init = &(const struct clk_init_data) {
 			.name = "gcc_ufs_1_card_clkref_clk",
+			.parent_data = &gcc_parent_data_tcxo,
+			.num_parents = 1,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -5432,6 +5434,8 @@ static struct clk_branch gcc_ufs_card_clkref_clk = {
 		.enable_mask = BIT(0),
 		.hw.init = &(const struct clk_init_data) {
 			.name = "gcc_ufs_card_clkref_clk",
+			.parent_data = &gcc_parent_data_tcxo,
+			.num_parents = 1,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -5848,6 +5852,8 @@ static struct clk_branch gcc_ufs_ref_clkref_clk = {
 		.enable_mask = BIT(0),
 		.hw.init = &(const struct clk_init_data) {
 			.name = "gcc_ufs_ref_clkref_clk",
+			.parent_data = &gcc_parent_data_tcxo,
+			.num_parents = 1,
 			.ops = &clk_branch2_ops,
 		},
 	},
-- 
2.38.0

Re: [PATCH v2] clk: qcom: gcc-sc8280xp: add cxo as parent for three ufs ref clks

[Johan Hovold]

On Tue, Nov 15, 2022 at 03:52:17PM +0530, Shazad Hussain wrote:
> Added parent_data as CXO for three UFS reference clocks named,
> gcc_ufs_ref_clkref_clk, gcc_ufs_card_clkref_clk and
> gcc_ufs_1_card_clkref_clk.

The commit message should explain why this is needed rather than just
state what is being done. For example, something along the lines of

	The three UFS reference clocks gcc_ufs_ref_clkref_clk,
	gcc_ufs_card_clkref_clk and gcc_ufs_1_card_clkref_clk are all
	sourced from CXO.

	Update the clock driver to describe this.

should do.

> Fixes: d65d005f9a6c ("clk: qcom: add sc8280xp GCC driver")
> Link: https://lore.kernel.org/lkml/Y2Tber39cHuOSR%2FW@hovoldconsulting.com/
> Signed-off-by: Shazad Hussain <quic_shazhuss@quicinc.com>

Tested-by: Johan Hovold <johan+linaro@kernel.org>

And with a tweaked commit message you can also add:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

>---
> Changes since v1:
> - Renamed patch subject. Did not include Brian's T-b comment.
> - Added parent data for two ufs phy ref clocks.
> 
> v1 of this patch can be found at
> https://lore.kernel.org/all/20221030142333.31019-1-quic_shazhuss@quicinc.com/
> 
> used below patches for verification on next-20221114
> https://lore.kernel.org/lkml/20221104092045.17410-2-johan+linaro@kernel.org/
> https://lore.kernel.org/lkml/20221104092045.17410-3-johan+linaro@kernel.org/
> https://lore.kernel.org/lkml/20221111113732.461881-1-thierry.reding@gmail.com/
> 
>  drivers/clk/qcom/gcc-sc8280xp.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
> index a18ed88f3b82..b3198784e1c3 100644
> --- a/drivers/clk/qcom/gcc-sc8280xp.c
> +++ b/drivers/clk/qcom/gcc-sc8280xp.c
> @@ -5364,6 +5364,8 @@ static struct clk_branch gcc_ufs_1_card_clkref_clk = {
>  		.enable_mask = BIT(0),
>  		.hw.init = &(const struct clk_init_data) {
>  			.name = "gcc_ufs_1_card_clkref_clk",
> +			.parent_data = &gcc_parent_data_tcxo,
> +			.num_parents = 1,
>  			.ops = &clk_branch2_ops,
>  		},
>  	},
> @@ -5432,6 +5434,8 @@ static struct clk_branch gcc_ufs_card_clkref_clk = {
>  		.enable_mask = BIT(0),
>  		.hw.init = &(const struct clk_init_data) {
>  			.name = "gcc_ufs_card_clkref_clk",
> +			.parent_data = &gcc_parent_data_tcxo,
> +			.num_parents = 1,
>  			.ops = &clk_branch2_ops,
>  		},
>  	},
> @@ -5848,6 +5852,8 @@ static struct clk_branch gcc_ufs_ref_clkref_clk = {
>  		.enable_mask = BIT(0),
>  		.hw.init = &(const struct clk_init_data) {
>  			.name = "gcc_ufs_ref_clkref_clk",
> +			.parent_data = &gcc_parent_data_tcxo,
> +			.num_parents = 1,
>  			.ops = &clk_branch2_ops,
>  		},
>  	},

Johan

Re: [PATCH v2] clk: qcom: gcc-sc8280xp: add cxo as parent for three ufs ref clks

[Andrew Halaney]

On Tue, Nov 15, 2022 at 01:08:11PM +0100, Johan Hovold wrote:
> On Tue, Nov 15, 2022 at 03:52:17PM +0530, Shazad Hussain wrote:
> > Added parent_data as CXO for three UFS reference clocks named,
> > gcc_ufs_ref_clkref_clk, gcc_ufs_card_clkref_clk and
> > gcc_ufs_1_card_clkref_clk.
> 
> The commit message should explain why this is needed rather than just
> state what is being done. For example, something along the lines of
> 
> 	The three UFS reference clocks gcc_ufs_ref_clkref_clk,
> 	gcc_ufs_card_clkref_clk and gcc_ufs_1_card_clkref_clk are all
> 	sourced from CXO.
> 
> 	Update the clock driver to describe this.
> 
> should do.
> 
> > Fixes: d65d005f9a6c ("clk: qcom: add sc8280xp GCC driver")
> > Link: https://lore.kernel.org/lkml/Y2Tber39cHuOSR%2FW@hovoldconsulting.com/
> > Signed-off-by: Shazad Hussain <quic_shazhuss@quicinc.com>
> 
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> 
> And with a tweaked commit message you can also add:
> 
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Thanks for the patch Shazad! Same as Johan, you can add my T-B:

Tested-by: Andrew Halaney <ahalaney@redhat.com>

and with a tweaked commit message my R-B (based on Bjorn's comments
about hw documentation in v1):

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> 
> >---
> > Changes since v1:
> > - Renamed patch subject. Did not include Brian's T-b comment.
> > - Added parent data for two ufs phy ref clocks.
> > 
> > v1 of this patch can be found at
> > https://lore.kernel.org/all/20221030142333.31019-1-quic_shazhuss@quicinc.com/
> > 
> > used below patches for verification on next-20221114
> > https://lore.kernel.org/lkml/20221104092045.17410-2-johan+linaro@kernel.org/
> > https://lore.kernel.org/lkml/20221104092045.17410-3-johan+linaro@kernel.org/
> > https://lore.kernel.org/lkml/20221111113732.461881-1-thierry.reding@gmail.com/
> > 
> >  drivers/clk/qcom/gcc-sc8280xp.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
> > index a18ed88f3b82..b3198784e1c3 100644
> > --- a/drivers/clk/qcom/gcc-sc8280xp.c
> > +++ b/drivers/clk/qcom/gcc-sc8280xp.c
> > @@ -5364,6 +5364,8 @@ static struct clk_branch gcc_ufs_1_card_clkref_clk = {
> >  		.enable_mask = BIT(0),
> >  		.hw.init = &(const struct clk_init_data) {
> >  			.name = "gcc_ufs_1_card_clkref_clk",
> > +			.parent_data = &gcc_parent_data_tcxo,
> > +			.num_parents = 1,
> >  			.ops = &clk_branch2_ops,
> >  		},
> >  	},
> > @@ -5432,6 +5434,8 @@ static struct clk_branch gcc_ufs_card_clkref_clk = {
> >  		.enable_mask = BIT(0),
> >  		.hw.init = &(const struct clk_init_data) {
> >  			.name = "gcc_ufs_card_clkref_clk",
> > +			.parent_data = &gcc_parent_data_tcxo,
> > +			.num_parents = 1,
> >  			.ops = &clk_branch2_ops,
> >  		},
> >  	},
> > @@ -5848,6 +5852,8 @@ static struct clk_branch gcc_ufs_ref_clkref_clk = {
> >  		.enable_mask = BIT(0),
> >  		.hw.init = &(const struct clk_init_data) {
> >  			.name = "gcc_ufs_ref_clkref_clk",
> > +			.parent_data = &gcc_parent_data_tcxo,
> > +			.num_parents = 1,
> >  			.ops = &clk_branch2_ops,
> >  		},
> >  	},
> 
> Johan
> 

Re: [PATCH v2] clk: qcom: gcc-sc8280xp: add cxo as parent for three ufs ref clks

[Brian Masney]

On Tue, Nov 15, 2022 at 03:52:17PM +0530, Shazad Hussain wrote:
> Added parent_data as CXO for three UFS reference clocks named,
> gcc_ufs_ref_clkref_clk, gcc_ufs_card_clkref_clk and
> gcc_ufs_1_card_clkref_clk.
> 
> Fixes: d65d005f9a6c ("clk: qcom: add sc8280xp GCC driver")
> Link: https://lore.kernel.org/lkml/Y2Tber39cHuOSR%2FW@hovoldconsulting.com/
> Signed-off-by: Shazad Hussain <quic_shazhuss@quicinc.com>

Put this Link beneath the --- since Bjorn will add one that's specific
to your patch when he adds it to his tree.

With that change:

Reviewed-by: Brian Masney <bmasney@redhat.com>

Re: [PATCH v2] clk: qcom: gcc-sc8280xp: add cxo as parent for three ufs ref clks

[Johan Hovold]

On Tue, Nov 15, 2022 at 07:50:22AM -0500, Brian Masney wrote:
> On Tue, Nov 15, 2022 at 03:52:17PM +0530, Shazad Hussain wrote:
> > Added parent_data as CXO for three UFS reference clocks named,
> > gcc_ufs_ref_clkref_clk, gcc_ufs_card_clkref_clk and
> > gcc_ufs_1_card_clkref_clk.
> > 
> > Fixes: d65d005f9a6c ("clk: qcom: add sc8280xp GCC driver")
> > Link: https://lore.kernel.org/lkml/Y2Tber39cHuOSR%2FW@hovoldconsulting.com/
> > Signed-off-by: Shazad Hussain <quic_shazhuss@quicinc.com>
> 
> Put this Link beneath the --- since Bjorn will add one that's specific
> to your patch when he adds it to his tree.

I think it's ok to keep that Link if it was intended as a reference to
the discussions leading up to this patch. There can be more than one
Link tag, and this one is more in line with how Linus thinks they should
be used than the ones added by tooling such as b4 when applying.

Johan

Re: [PATCH v2] clk: qcom: gcc-sc8280xp: add cxo as parent for three ufs ref clks

[Brian Masney]

On Tue, Nov 15, 2022 at 02:01:04PM +0100, Johan Hovold wrote:
> > Put this Link beneath the --- since Bjorn will add one that's specific
> > to your patch when he adds it to his tree.
> 
> I think it's ok to keep that Link if it was intended as a reference to
> the discussions leading up to this patch. There can be more than one
> Link tag, and this one is more in line with how Linus thinks they should
> be used than the ones added by tooling such as b4 when applying.

OK, good to know. Thank you!

Brian