| Message ID | 20220623142837.3140680-1-bmasney@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | clk: qcom: sc8280xp: add parent to gcc_ufs_phy_axi_clk for sa8540p | expand |
On Thu, Jun 23, 2022 at 10:28:37AM -0400, Brian Masney wrote: > The sa8540p automotive board has the same SOC as the sc8280xp. In order > to get the first UFS controller working on the sa8540p, > GCC_UFS_REF_CLKREF_CLK needs to be setup as a parent to > GCC_UFS_PHY_AXI_CLK. > > This clock name came from the DTS for the downstream MSM 5.4 kernel > sources for the sa8540p. It also references GCC_UFS_CARD_CLKREF_CLK, > however that wasn't needed to get the first UFS controller working. > I'm not knowledgable enough on the clock subsystem or have access to documentation to comment on correctness outside of the 5.4 MSM kernel you mentioned, but I think this probably deserves a Fixes: tag? I can at least also comment that without this change the sa8540p doesn't boot on the QDrive3 boards we have. Thanks, Andrew > Signed-off-by: Brian Masney <bmasney@redhat.com> > --- > I originally added this under GCC_UFS_PHY_PHY_AUX_CLK since that's > what's in the downstream DTS. I was getting errors about > GCC_UFS_PHY_AXI_CLK being stuck at off so I moved it there. > > Also I don't have access to any documentation for this board so I'm > hoping that someone with docs access can verify that this is the > appropriate place to put this. > > drivers/clk/qcom/gcc-sc8280xp.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c > index 4b894442fdf5..4639b50da418 100644 > --- a/drivers/clk/qcom/gcc-sc8280xp.c > +++ b/drivers/clk/qcom/gcc-sc8280xp.c > @@ -5685,6 +5685,19 @@ static struct clk_branch gcc_ufs_phy_ahb_clk = { > }, > }; > > +static struct clk_branch gcc_ufs_ref_clkref_clk = { > + .halt_reg = 0x8c058, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0x8c058, > + .enable_mask = BIT(0), > + .hw.init = &(const struct clk_init_data) { > + .name = "gcc_ufs_ref_clkref_clk", > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > static struct clk_branch gcc_ufs_phy_axi_clk = { > .halt_reg = 0x77010, > .halt_check = BRANCH_HALT_VOTED, > @@ -5696,6 +5709,7 @@ static struct clk_branch gcc_ufs_phy_axi_clk = { > .hw.init = &(const struct clk_init_data) { > .name = "gcc_ufs_phy_axi_clk", > .parent_hws = (const struct clk_hw*[]){ > + &gcc_ufs_ref_clkref_clk.clkr.hw, > &gcc_ufs_phy_axi_clk_src.clkr.hw, > }, > .num_parents = 1, > @@ -5899,19 +5913,6 @@ static struct clk_branch gcc_ufs_phy_unipro_core_hw_ctl_clk = { > }, > }; > > -static struct clk_branch gcc_ufs_ref_clkref_clk = { > - .halt_reg = 0x8c058, > - .halt_check = BRANCH_HALT, > - .clkr = { > - .enable_reg = 0x8c058, > - .enable_mask = BIT(0), > - .hw.init = &(const struct clk_init_data) { > - .name = "gcc_ufs_ref_clkref_clk", > - .ops = &clk_branch2_ops, > - }, > - }, > -}; > - > static struct clk_branch gcc_usb2_hs0_clkref_clk = { > .halt_reg = 0x8c044, > .halt_check = BRANCH_HALT, > -- > 2.36.1 >
On Thu, Jun 23, 2022 at 10:32:26AM -0500, Andrew Halaney wrote: > I'm not knowledgable enough on the clock subsystem or have access to > documentation to comment on correctness outside of the 5.4 MSM kernel > you mentioned, but I think this probably deserves a Fixes: tag? Good call! Fixes: d65d005f9a6c ("clk: qcom: add sc8280xp GCC driver")
Quoting Brian Masney (2022-06-23 07:28:37) > diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c > index 4b894442fdf5..4639b50da418 100644 > --- a/drivers/clk/qcom/gcc-sc8280xp.c > +++ b/drivers/clk/qcom/gcc-sc8280xp.c > @@ -5696,6 +5709,7 @@ static struct clk_branch gcc_ufs_phy_axi_clk = { > .hw.init = &(const struct clk_init_data) { > .name = "gcc_ufs_phy_axi_clk", > .parent_hws = (const struct clk_hw*[]){ > + &gcc_ufs_ref_clkref_clk.clkr.hw, > &gcc_ufs_phy_axi_clk_src.clkr.hw, > }, > .num_parents = 1, num_parents needs an update. But this is a branch, not a mux, so it can't have more than one parent.
On Thu, Jun 23, 2022 at 05:20:53PM -0700, Stephen Boyd wrote: > Quoting Brian Masney (2022-06-23 07:28:37) > > diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c > > index 4b894442fdf5..4639b50da418 100644 > > --- a/drivers/clk/qcom/gcc-sc8280xp.c > > +++ b/drivers/clk/qcom/gcc-sc8280xp.c > > @@ -5696,6 +5709,7 @@ static struct clk_branch gcc_ufs_phy_axi_clk = { > > .hw.init = &(const struct clk_init_data) { > > .name = "gcc_ufs_phy_axi_clk", > > .parent_hws = (const struct clk_hw*[]){ > > + &gcc_ufs_ref_clkref_clk.clkr.hw, > > &gcc_ufs_phy_axi_clk_src.clkr.hw, > > }, > > .num_parents = 1, > > num_parents needs an update. Oops! > But this is a branch, not a mux, so it can't have more than one > parent. Would a mux be represented with 'struct clk_rcg2'? Brian
Quoting Brian Masney (2022-06-24 09:40:26) > On Thu, Jun 23, 2022 at 05:20:53PM -0700, Stephen Boyd wrote: > > Quoting Brian Masney (2022-06-23 07:28:37) > > > diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c > > > index 4b894442fdf5..4639b50da418 100644 > > > --- a/drivers/clk/qcom/gcc-sc8280xp.c > > > +++ b/drivers/clk/qcom/gcc-sc8280xp.c > > > @@ -5696,6 +5709,7 @@ static struct clk_branch gcc_ufs_phy_axi_clk = { > > > .hw.init = &(const struct clk_init_data) { > > > .name = "gcc_ufs_phy_axi_clk", > > > .parent_hws = (const struct clk_hw*[]){ > > > + &gcc_ufs_ref_clkref_clk.clkr.hw, > > > &gcc_ufs_phy_axi_clk_src.clkr.hw, > > > }, > > > .num_parents = 1, > > > > num_parents needs an update. > > Oops! > > > But this is a branch, not a mux, so it can't have more than one > > parent. > > Would a mux be represented with 'struct clk_rcg2'? > Could be. An RCG is more than a mux, because it also has a divider and an m/n counter.
On Thu 23 Jun 09:28 CDT 2022, Brian Masney wrote: > The sa8540p automotive board has the same SOC as the sc8280xp. In order > to get the first UFS controller working on the sa8540p, > GCC_UFS_REF_CLKREF_CLK needs to be setup as a parent to > GCC_UFS_PHY_AXI_CLK. > > This clock name came from the DTS for the downstream MSM 5.4 kernel > sources for the sa8540p. It also references GCC_UFS_CARD_CLKREF_CLK, > however that wasn't needed to get the first UFS controller working. > > Signed-off-by: Brian Masney <bmasney@redhat.com> > --- > I originally added this under GCC_UFS_PHY_PHY_AUX_CLK since that's > what's in the downstream DTS. I was getting errors about > GCC_UFS_PHY_AXI_CLK being stuck at off so I moved it there. > > Also I don't have access to any documentation for this board so I'm > hoping that someone with docs access can verify that this is the > appropriate place to put this. > > drivers/clk/qcom/gcc-sc8280xp.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c > index 4b894442fdf5..4639b50da418 100644 > --- a/drivers/clk/qcom/gcc-sc8280xp.c > +++ b/drivers/clk/qcom/gcc-sc8280xp.c > @@ -5685,6 +5685,19 @@ static struct clk_branch gcc_ufs_phy_ahb_clk = { > }, > }; > > +static struct clk_branch gcc_ufs_ref_clkref_clk = { > + .halt_reg = 0x8c058, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0x8c058, > + .enable_mask = BIT(0), > + .hw.init = &(const struct clk_init_data) { > + .name = "gcc_ufs_ref_clkref_clk", > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > static struct clk_branch gcc_ufs_phy_axi_clk = { > .halt_reg = 0x77010, > .halt_check = BRANCH_HALT_VOTED, > @@ -5696,6 +5709,7 @@ static struct clk_branch gcc_ufs_phy_axi_clk = { > .hw.init = &(const struct clk_init_data) { > .name = "gcc_ufs_phy_axi_clk", > .parent_hws = (const struct clk_hw*[]){ > + &gcc_ufs_ref_clkref_clk.clkr.hw, gcc_ufs_ref_clkref_clk isn't the parent of gcc_ufs_phy_axi_clk. But I presume that if this works, then you're just enabling the ufs clkref clock, indirectly. Seems reasonable that you should be specifying gcc_ufs_ref_clkref_clk as "ref_clk" instead. Could you please give that a go? Regards, Bjorn > &gcc_ufs_phy_axi_clk_src.clkr.hw, > }, > .num_parents = 1, > @@ -5899,19 +5913,6 @@ static struct clk_branch gcc_ufs_phy_unipro_core_hw_ctl_clk = { > }, > }; > > -static struct clk_branch gcc_ufs_ref_clkref_clk = { > - .halt_reg = 0x8c058, > - .halt_check = BRANCH_HALT, > - .clkr = { > - .enable_reg = 0x8c058, > - .enable_mask = BIT(0), > - .hw.init = &(const struct clk_init_data) { > - .name = "gcc_ufs_ref_clkref_clk", > - .ops = &clk_branch2_ops, > - }, > - }, > -}; > - > static struct clk_branch gcc_usb2_hs0_clkref_clk = { > .halt_reg = 0x8c044, > .halt_check = BRANCH_HALT, > -- > 2.36.1 >
On Sat, Jul 16, 2022 at 09:54:43AM -0500, Bjorn Andersson wrote: > gcc_ufs_ref_clkref_clk isn't the parent of gcc_ufs_phy_axi_clk. > > But I presume that if this works, then you're just enabling the ufs > clkref clock, indirectly. Seems reasonable that you should be specifying > gcc_ufs_ref_clkref_clk as "ref_clk" instead. > > Could you please give that a go? Hey Bjorn, I assume you mean ref_aux on the phy with this patch: --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi @@ -1464,7 +1464,7 @@ ufs_mem_phy: phy@1d87000 { clock-names = "ref", "ref_aux"; clocks = <&rpmhcc RPMH_CXO_CLK>, - <&gcc GCC_UFS_PHY_PHY_AUX_CLK>; + <&gcc GCC_UFS_REF_CLKREF_CLK>; resets = <&ufs_mem_hc 0>; reset-names = "ufsphy"; This works on the sa8540p. I tried changing ref_clk on the ufs host controller from <&rpmhcc RPMH_CXO_CLK> to the new value and the board resets. Brian
diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c index 4b894442fdf5..4639b50da418 100644 --- a/drivers/clk/qcom/gcc-sc8280xp.c +++ b/drivers/clk/qcom/gcc-sc8280xp.c @@ -5685,6 +5685,19 @@ static struct clk_branch gcc_ufs_phy_ahb_clk = { }, }; +static struct clk_branch gcc_ufs_ref_clkref_clk = { + .halt_reg = 0x8c058, + .halt_check = BRANCH_HALT, + .clkr = { + .enable_reg = 0x8c058, + .enable_mask = BIT(0), + .hw.init = &(const struct clk_init_data) { + .name = "gcc_ufs_ref_clkref_clk", + .ops = &clk_branch2_ops, + }, + }, +}; + static struct clk_branch gcc_ufs_phy_axi_clk = { .halt_reg = 0x77010, .halt_check = BRANCH_HALT_VOTED, @@ -5696,6 +5709,7 @@ static struct clk_branch gcc_ufs_phy_axi_clk = { .hw.init = &(const struct clk_init_data) { .name = "gcc_ufs_phy_axi_clk", .parent_hws = (const struct clk_hw*[]){ + &gcc_ufs_ref_clkref_clk.clkr.hw, &gcc_ufs_phy_axi_clk_src.clkr.hw, }, .num_parents = 1, @@ -5899,19 +5913,6 @@ static struct clk_branch gcc_ufs_phy_unipro_core_hw_ctl_clk = { }, }; -static struct clk_branch gcc_ufs_ref_clkref_clk = { - .halt_reg = 0x8c058, - .halt_check = BRANCH_HALT, - .clkr = { - .enable_reg = 0x8c058, - .enable_mask = BIT(0), - .hw.init = &(const struct clk_init_data) { - .name = "gcc_ufs_ref_clkref_clk", - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gcc_usb2_hs0_clkref_clk = { .halt_reg = 0x8c044, .halt_check = BRANCH_HALT,
The sa8540p automotive board has the same SOC as the sc8280xp. In order to get the first UFS controller working on the sa8540p, GCC_UFS_REF_CLKREF_CLK needs to be setup as a parent to GCC_UFS_PHY_AXI_CLK. This clock name came from the DTS for the downstream MSM 5.4 kernel sources for the sa8540p. It also references GCC_UFS_CARD_CLKREF_CLK, however that wasn't needed to get the first UFS controller working. Signed-off-by: Brian Masney <bmasney@redhat.com> --- I originally added this under GCC_UFS_PHY_PHY_AUX_CLK since that's what's in the downstream DTS. I was getting errors about GCC_UFS_PHY_AXI_CLK being stuck at off so I moved it there. Also I don't have access to any documentation for this board so I'm hoping that someone with docs access can verify that this is the appropriate place to put this. drivers/clk/qcom/gcc-sc8280xp.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)