| Message ID | 20241017-sar2130p-clocks-v1-7-f75e740f0a8d@linaro.org (mailing list archive) |
|---|---|
| State | New |
| 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.
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)