| Message ID | 20241017-sar2130p-clocks-v1-7-f75e740f0a8d@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: qcom: add support for clock controllers on the SAR2130P platform | expand |
Quoting Dmitry Baryshkov (2024-10-17 09:56:57) > From: Kalpak Kawadkar <quic_kkawadka@quicinc.com> > > Add support for SREG branch ops. This is for the clocks which require What is SREG? Can you spell it out? > additional register operations with the SREG register as a part of > enable / disable operations. > > Signed-off-by: Kalpak Kawadkar <quic_kkawadka@quicinc.com> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> [...] > diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h > index 47bf59a671c3c8516a57c283fce548a6e5f16619..149d04bae25d1a54999e0f938c4fce175a7c3e42 100644 > --- a/drivers/clk/qcom/clk-branch.h > +++ b/drivers/clk/qcom/clk-branch.h > @@ -24,8 +24,11 @@ > struct clk_branch { > u32 hwcg_reg; > u32 halt_reg; > + u32 sreg_enable_reg; > u8 hwcg_bit; > u8 halt_bit; > + u32 sreg_core_ack_bit; > + u32 sreg_periph_ack_bit; Are these bits? Should be u8 then. Or are they a mask? > u8 halt_check; Instead of adding these new members can you wrap the struct in another struct? There are usually a lot of branches in the system and this bloats those structures when the members are never used. struct clk_sreg_branch { u32 sreg_enable_reg; u32 sreg_core_ack_bit; u32 sreg_periph_ack_bit; struct clk_branch branch; }; But I'm not even sure that is needed vs. just putting a clk_regmap inside because the clk_ops don't seem to use any of these other members?
On Thu, Oct 17, 2024 at 11:10:20AM -0700, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2024-10-17 09:56:57) > > From: Kalpak Kawadkar <quic_kkawadka@quicinc.com> > > > > Add support for SREG branch ops. This is for the clocks which require > > What is SREG? Can you spell it out? Unfortunately, no idea. This is the only register name I know. > > > additional register operations with the SREG register as a part of > > enable / disable operations. > > > > Signed-off-by: Kalpak Kawadkar <quic_kkawadka@quicinc.com> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > [...] > > diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h > > index 47bf59a671c3c8516a57c283fce548a6e5f16619..149d04bae25d1a54999e0f938c4fce175a7c3e42 100644 > > --- a/drivers/clk/qcom/clk-branch.h > > +++ b/drivers/clk/qcom/clk-branch.h > > @@ -24,8 +24,11 @@ > > struct clk_branch { > > u32 hwcg_reg; > > u32 halt_reg; > > + u32 sreg_enable_reg; > > u8 hwcg_bit; > > u8 halt_bit; > > + u32 sreg_core_ack_bit; > > + u32 sreg_periph_ack_bit; > > Are these bits? Should be u8 then. Or are they a mask? masks, will rename. > > > u8 halt_check; > > Instead of adding these new members can you wrap the struct in another > struct? There are usually a lot of branches in the system and this > bloats those structures when the members are never used. > > struct clk_sreg_branch { > u32 sreg_enable_reg; > u32 sreg_core_ack_bit; > u32 sreg_periph_ack_bit; > struct clk_branch branch; > }; > > But I'm not even sure that is needed vs. just putting a clk_regmap > inside because the clk_ops don't seem to use any of these other members? Yes, nice idea. Is it ok to keep the _branch suffix or we'd better rename it dropping the _branch (and move to another source file while we are at it)?
Quoting Dmitry Baryshkov (2024-10-17 15:00:03) > On Thu, Oct 17, 2024 at 11:10:20AM -0700, Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2024-10-17 09:56:57) > > > From: Kalpak Kawadkar <quic_kkawadka@quicinc.com> > > > > > > Add support for SREG branch ops. This is for the clocks which require > > > > What is SREG? Can you spell it out? > > Unfortunately, no idea. This is the only register name I know. > Can someone inside qcom tell us? > > > > > > u8 halt_check; > > > > Instead of adding these new members can you wrap the struct in another > > struct? There are usually a lot of branches in the system and this > > bloats those structures when the members are never used. > > > > struct clk_sreg_branch { > > u32 sreg_enable_reg; > > u32 sreg_core_ack_bit; > > u32 sreg_periph_ack_bit; > > struct clk_branch branch; > > }; > > > > But I'm not even sure that is needed vs. just putting a clk_regmap > > inside because the clk_ops don't seem to use any of these other members? > > Yes, nice idea. Is it ok to keep the _branch suffix or we'd better > rename it dropping the _branch (and move to another source file while we > are at it)? > I don't really care. Inside qcom they called things branches in the hardware and that name was carried into the code. If sreg is a branch then that would make sense. From the 'core_ack' and 'periph_ack' it actually looks like some sort of power switch masquerading as a clk.
On Thu, Oct 17, 2024 at 03:28:13PM -0700, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2024-10-17 15:00:03) > > On Thu, Oct 17, 2024 at 11:10:20AM -0700, Stephen Boyd wrote: > > > Quoting Dmitry Baryshkov (2024-10-17 09:56:57) > > > > From: Kalpak Kawadkar <quic_kkawadka@quicinc.com> > > > > > > > > Add support for SREG branch ops. This is for the clocks which require > > > > > > What is SREG? Can you spell it out? > > > > Unfortunately, no idea. This is the only register name I know. > > > > Can someone inside qcom tell us? Taniya, could you possibly help us? This is for gcc_video_axi0_sreg / gcc_video_axi1_sreg / gcc_iris_ss_hf_axi1_sreg / gcc_iris_ss_spd_axi1_sreg clocks on the SAR2130P platform. > > > > > > > > > > u8 halt_check; > > > > > > Instead of adding these new members can you wrap the struct in another > > > struct? There are usually a lot of branches in the system and this > > > bloats those structures when the members are never used. > > > > > > struct clk_sreg_branch { > > > u32 sreg_enable_reg; > > > u32 sreg_core_ack_bit; > > > u32 sreg_periph_ack_bit; > > > struct clk_branch branch; > > > }; > > > > > > But I'm not even sure that is needed vs. just putting a clk_regmap > > > inside because the clk_ops don't seem to use any of these other members? > > > > Yes, nice idea. Is it ok to keep the _branch suffix or we'd better > > rename it dropping the _branch (and move to another source file while we > > are at it)? > > > > I don't really care. Inside qcom they called things branches in the > hardware and that name was carried into the code. If sreg is a branch > then that would make sense. From the 'core_ack' and 'periph_ack' it > actually looks like some sort of power switch masquerading as a clk. Ack.
On 10/18/2024 3:26 PM, Dmitry Baryshkov wrote: > On Thu, Oct 17, 2024 at 03:28:13PM -0700, Stephen Boyd wrote: >> Quoting Dmitry Baryshkov (2024-10-17 15:00:03) >>> On Thu, Oct 17, 2024 at 11:10:20AM -0700, Stephen Boyd wrote: >>>> Quoting Dmitry Baryshkov (2024-10-17 09:56:57) >>>>> From: Kalpak Kawadkar <quic_kkawadka@quicinc.com> >>>>> >>>>> Add support for SREG branch ops. This is for the clocks which require >>>> >>>> What is SREG? Can you spell it out? >>> >>> Unfortunately, no idea. This is the only register name I know. >>> >> >> Can someone inside qcom tell us? > > Taniya, could you possibly help us? This is for gcc_video_axi0_sreg / > gcc_video_axi1_sreg / gcc_iris_ss_hf_axi1_sreg / > gcc_iris_ss_spd_axi1_sreg clocks on the SAR2130P platform. > SREG(Shift Register) are the register interface for clock branches which can control memories connected to them. In principle these SREGs are not required to be controlled via SW interface, but on SAR2130P, we had to control them to flush out the pipeline for users of Video. We are looking into the feasibility of extending the current 'clk_branch2_mem_ops' and can share the patch. You could also drop these clock interfaces for now to move ahead, as I do not see VideoCC support and bring them as part of those Clock controller support. >> >>> >>>> >>>>> u8 halt_check; >>>> >>>> Instead of adding these new members can you wrap the struct in another >>>> struct? There are usually a lot of branches in the system and this >>>> bloats those structures when the members are never used. >>>> >>>> struct clk_sreg_branch { >>>> u32 sreg_enable_reg; >>>> u32 sreg_core_ack_bit; >>>> u32 sreg_periph_ack_bit; >>>> struct clk_branch branch; >>>> }; >>>> >>>> But I'm not even sure that is needed vs. just putting a clk_regmap >>>> inside because the clk_ops don't seem to use any of these other members? >>> >>> Yes, nice idea. Is it ok to keep the _branch suffix or we'd better >>> rename it dropping the _branch (and move to another source file while we >>> are at it)? >>> >> >> I don't really care. Inside qcom they called things branches in the >> hardware and that name was carried into the code. If sreg is a branch >> then that would make sense. From the 'core_ack' and 'periph_ack' it >> actually looks like some sort of power switch masquerading as a clk. > > Ack. > >
On Fri, Oct 18, 2024 at 04:20:45PM +0530, Taniya Das wrote: > > > On 10/18/2024 3:26 PM, Dmitry Baryshkov wrote: > > On Thu, Oct 17, 2024 at 03:28:13PM -0700, Stephen Boyd wrote: > > > Quoting Dmitry Baryshkov (2024-10-17 15:00:03) > > > > On Thu, Oct 17, 2024 at 11:10:20AM -0700, Stephen Boyd wrote: > > > > > Quoting Dmitry Baryshkov (2024-10-17 09:56:57) > > > > > > From: Kalpak Kawadkar <quic_kkawadka@quicinc.com> > > > > > > > > > > > > Add support for SREG branch ops. This is for the clocks which require > > > > > > > > > > What is SREG? Can you spell it out? > > > > > > > > Unfortunately, no idea. This is the only register name I know. > > > > > > > > > > Can someone inside qcom tell us? > > > > Taniya, could you possibly help us? This is for gcc_video_axi0_sreg / > > gcc_video_axi1_sreg / gcc_iris_ss_hf_axi1_sreg / > > gcc_iris_ss_spd_axi1_sreg clocks on the SAR2130P platform. > > > > SREG(Shift Register) are the register interface for clock branches which can > control memories connected to them. > > In principle these SREGs are not required to be controlled via SW interface, > but on SAR2130P, we had to control them to flush out the pipeline for users > of Video. > > We are looking into the feasibility of extending the current > 'clk_branch2_mem_ops' and can share the patch. > > You could also drop these clock interfaces for now to move ahead, as I do > not see VideoCC support and bring them as part of those Clock controller > support. SGTM, thank you for your comment! > > > > > > > > > > > > > > > > > > > u8 halt_check; > > > > > > > > > > Instead of adding these new members can you wrap the struct in another > > > > > struct? There are usually a lot of branches in the system and this > > > > > bloats those structures when the members are never used. > > > > > > > > > > struct clk_sreg_branch { > > > > > u32 sreg_enable_reg; > > > > > u32 sreg_core_ack_bit; > > > > > u32 sreg_periph_ack_bit; > > > > > struct clk_branch branch; > > > > > }; > > > > > > > > > > But I'm not even sure that is needed vs. just putting a clk_regmap > > > > > inside because the clk_ops don't seem to use any of these other members? > > > > > > > > Yes, nice idea. Is it ok to keep the _branch suffix or we'd better > > > > rename it dropping the _branch (and move to another source file while we > > > > are at it)? > > > > > > > > > > I don't really care. Inside qcom they called things branches in the > > > hardware and that name was carried into the code. If sreg is a branch > > > then that would make sense. From the 'core_ack' and 'periph_ack' it > > > actually looks like some sort of power switch masquerading as a clk. > > > > Ack. > > > > > > -- > Thanks & Regards, > Taniya Das.
diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c index c4c7bd565cc9a3926e24bb12ed6355ec6ddd19fb..9142a33b6b3ba72a7dd9ff80a64c17f2a1746e8c 100644 --- a/drivers/clk/qcom/clk-branch.c +++ b/drivers/clk/qcom/clk-branch.c @@ -170,6 +170,31 @@ static void clk_branch2_mem_disable(struct clk_hw *hw) return clk_branch2_disable(hw); } +static int clk_branch2_sreg_enable(struct clk_hw *hw) +{ + struct clk_branch *br = to_clk_branch(hw); + u32 val; + int ret; + + ret = clk_enable_regmap(hw); + if (ret) + return -EINVAL; + + return regmap_read_poll_timeout(br->clkr.regmap, br->sreg_enable_reg, + val, !(val & br->sreg_core_ack_bit), 1, 200); +} + +static void clk_branch2_sreg_disable(struct clk_hw *hw) +{ + struct clk_branch *br = to_clk_branch(hw); + u32 val; + + clk_disable_regmap(hw); + + regmap_read_poll_timeout(br->clkr.regmap, br->sreg_enable_reg, + val, val & br->sreg_periph_ack_bit, 1, 200); +} + const struct clk_ops clk_branch2_mem_ops = { .enable = clk_branch2_mem_enable, .disable = clk_branch2_mem_disable, @@ -203,3 +228,10 @@ const struct clk_ops clk_branch2_prepare_ops = { .is_prepared = clk_is_enabled_regmap, }; EXPORT_SYMBOL_GPL(clk_branch2_prepare_ops); + +const struct clk_ops clk_branch2_sreg_ops = { + .enable = clk_branch2_sreg_enable, + .disable = clk_branch2_sreg_disable, + .is_enabled = clk_is_enabled_regmap, +}; +EXPORT_SYMBOL(clk_branch2_sreg_ops); diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h index 47bf59a671c3c8516a57c283fce548a6e5f16619..149d04bae25d1a54999e0f938c4fce175a7c3e42 100644 --- a/drivers/clk/qcom/clk-branch.h +++ b/drivers/clk/qcom/clk-branch.h @@ -24,8 +24,11 @@ struct clk_branch { u32 hwcg_reg; u32 halt_reg; + u32 sreg_enable_reg; u8 hwcg_bit; u8 halt_bit; + u32 sreg_core_ack_bit; + u32 sreg_periph_ack_bit; u8 halt_check; #define BRANCH_VOTED BIT(7) /* Delay on disable */ #define BRANCH_HALT 0 /* pol: 1 = halt */ @@ -111,6 +114,7 @@ extern const struct clk_ops clk_branch_simple_ops; extern const struct clk_ops clk_branch2_aon_ops; extern const struct clk_ops clk_branch2_mem_ops; extern const struct clk_ops clk_branch2_prepare_ops; +extern const struct clk_ops clk_branch2_sreg_ops; #define to_clk_branch(_hw) \ container_of(to_clk_regmap(_hw), struct clk_branch, clkr)