| Message ID | 20231011090028.1706653-3-quic_imrashai@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for Qualcomm ECPRI clock controller | expand |
On 10/11/23 11:00, Imran Shaik wrote: > From: Taniya Das <quic_tdas@quicinc.com> > > Clock CBCRs with memories need an update for memory before enable/disable > of the clock, which helps retain the respective block's register contents. > Add support for the mem ops to handle this sequence. > > Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com> > --- > drivers/clk/qcom/clk-branch.c | 37 +++++++++++++++++++++++++++++++++++ > drivers/clk/qcom/clk-branch.h | 21 ++++++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c > index fc4735f74f0f..9ac8d04b425a 100644 > --- a/drivers/clk/qcom/clk-branch.c > +++ b/drivers/clk/qcom/clk-branch.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > * Copyright (c) 2013, The Linux Foundation. All rights reserved. > + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. > */ > > #include <linux/kernel.h> > @@ -134,6 +135,42 @@ static void clk_branch2_disable(struct clk_hw *hw) > clk_branch_toggle(hw, false, clk_branch2_check_halt); > } > > +static int clk_branch2_mem_enable(struct clk_hw *hw) > +{ > + struct clk_mem_branch *mem_br = to_clk_mem_branch(hw); > + const char *name = clk_hw_get_name(&mem_br->branch.clkr.hw); > + u32 val; > + int timeout = 200, ret; Reverse-Christmas-tree, please You can drop the timeout variable and pass the int literal. > + > + regmap_update_bits(mem_br->branch.clkr.regmap, mem_br->mem_enable_reg, > + mem_br->mem_enable_ack_bit, mem_br->mem_enable_ack_bit); This is a mask, not a bit. > + > + ret = regmap_read_poll_timeout(mem_br->branch.clkr.regmap, mem_br->mem_ack_reg, > + val, val & mem_br->mem_enable_ack_bit, 0, timeout); [...] > +/** > + * struct clk_mem_branch - gating clock which are associated with memories > + * > + * @mem_enable_reg: branch clock memory gating register > + * @mem_ack_reg: branch clock memory ack register > + * @mem_enable_ack_bit: ANDed with @mem_ack_reg to check memory enablement @dog: woofs Describe what it is instead. Konrad
Quoting Imran Shaik (2023-10-11 02:00:26) > diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c > index fc4735f74f0f..9ac8d04b425a 100644 > --- a/drivers/clk/qcom/clk-branch.c > +++ b/drivers/clk/qcom/clk-branch.c > @@ -134,6 +135,42 @@ static void clk_branch2_disable(struct clk_hw *hw) > clk_branch_toggle(hw, false, clk_branch2_check_halt); > } > > +static int clk_branch2_mem_enable(struct clk_hw *hw) > +{ > + struct clk_mem_branch *mem_br = to_clk_mem_branch(hw); > + const char *name = clk_hw_get_name(&mem_br->branch.clkr.hw); > + u32 val; > + int timeout = 200, ret; const int timeout? > + > + regmap_update_bits(mem_br->branch.clkr.regmap, mem_br->mem_enable_reg, > + mem_br->mem_enable_ack_bit, mem_br->mem_enable_ack_bit); > + > + ret = regmap_read_poll_timeout(mem_br->branch.clkr.regmap, mem_br->mem_ack_reg, > + val, val & mem_br->mem_enable_ack_bit, 0, timeout); The 'mem_br->branch' is used a few times so please make another local variable for that called 'branch'. > + if (ret) { > + WARN(1, "%s mem enable failed", name); Needs a newline on the message string. > + return ret; > + } > + > + return clk_branch2_enable(hw); > +} > + > +static void clk_branch2_mem_disable(struct clk_hw *hw) > +{ > + struct clk_mem_branch *mem_br = to_clk_mem_branch(hw); > + > + regmap_update_bits(mem_br->branch.clkr.regmap, mem_br->mem_enable_reg, > + mem_br->mem_enable_ack_bit, 0); Please add a newline here. > + return clk_branch2_disable(hw); > +} > +
On 10/11/2023 3:21 PM, Konrad Dybcio wrote: > > > On 10/11/23 11:00, Imran Shaik wrote: >> From: Taniya Das <quic_tdas@quicinc.com> >> >> Clock CBCRs with memories need an update for memory before enable/disable >> of the clock, which helps retain the respective block's register >> contents. >> Add support for the mem ops to handle this sequence. >> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com> >> --- >> drivers/clk/qcom/clk-branch.c | 37 +++++++++++++++++++++++++++++++++++ >> drivers/clk/qcom/clk-branch.h | 21 ++++++++++++++++++++ >> 2 files changed, 58 insertions(+) >> >> diff --git a/drivers/clk/qcom/clk-branch.c >> b/drivers/clk/qcom/clk-branch.c >> index fc4735f74f0f..9ac8d04b425a 100644 >> --- a/drivers/clk/qcom/clk-branch.c >> +++ b/drivers/clk/qcom/clk-branch.c >> @@ -1,6 +1,7 @@ >> // SPDX-License-Identifier: GPL-2.0 >> /* >> * Copyright (c) 2013, The Linux Foundation. All rights reserved. >> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights >> reserved. >> */ >> #include <linux/kernel.h> >> @@ -134,6 +135,42 @@ static void clk_branch2_disable(struct clk_hw *hw) >> clk_branch_toggle(hw, false, clk_branch2_check_halt); >> } >> +static int clk_branch2_mem_enable(struct clk_hw *hw) >> +{ >> + struct clk_mem_branch *mem_br = to_clk_mem_branch(hw); >> + const char *name = clk_hw_get_name(&mem_br->branch.clkr.hw); >> + u32 val; >> + int timeout = 200, ret; > Reverse-Christmas-tree, please > > You can drop the timeout variable and pass the int literal. > Sure, will take care of this in the next series. >> + >> + regmap_update_bits(mem_br->branch.clkr.regmap, >> mem_br->mem_enable_reg, >> + mem_br->mem_enable_ack_bit, mem_br->mem_enable_ack_bit); > This is a mask, not a bit. > Yes, will check and update. >> + >> + ret = regmap_read_poll_timeout(mem_br->branch.clkr.regmap, >> mem_br->mem_ack_reg, >> + val, val & mem_br->mem_enable_ack_bit, 0, timeout); > > [...] > >> +/** >> + * struct clk_mem_branch - gating clock which are associated with >> memories >> + * >> + * @mem_enable_reg: branch clock memory gating register >> + * @mem_ack_reg: branch clock memory ack register >> + * @mem_enable_ack_bit: ANDed with @mem_ack_reg to check memory >> enablement > @dog: woofs > > Describe what it is instead. > > Konrad Sure, will update the description in the next series. Thanks, Imran
On 10/13/2023 1:54 AM, Stephen Boyd wrote: > Quoting Imran Shaik (2023-10-11 02:00:26) >> diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c >> index fc4735f74f0f..9ac8d04b425a 100644 >> --- a/drivers/clk/qcom/clk-branch.c >> +++ b/drivers/clk/qcom/clk-branch.c >> @@ -134,6 +135,42 @@ static void clk_branch2_disable(struct clk_hw *hw) >> clk_branch_toggle(hw, false, clk_branch2_check_halt); >> } >> >> +static int clk_branch2_mem_enable(struct clk_hw *hw) >> +{ >> + struct clk_mem_branch *mem_br = to_clk_mem_branch(hw); >> + const char *name = clk_hw_get_name(&mem_br->branch.clkr.hw); >> + u32 val; >> + int timeout = 200, ret; > > const int timeout? > Will drop the timeout as per Konrad's review comment and pass the int literal. >> + >> + regmap_update_bits(mem_br->branch.clkr.regmap, mem_br->mem_enable_reg, >> + mem_br->mem_enable_ack_bit, mem_br->mem_enable_ack_bit); >> + >> + ret = regmap_read_poll_timeout(mem_br->branch.clkr.regmap, mem_br->mem_ack_reg, >> + val, val & mem_br->mem_enable_ack_bit, 0, timeout); > > The 'mem_br->branch' is used a few times so please make another local > variable for that called 'branch'. > >> + if (ret) { >> + WARN(1, "%s mem enable failed", name); > > Needs a newline on the message string. > Sure, will update this in the next series. >> + return ret; >> + } >> + >> + return clk_branch2_enable(hw); >> +} >> + >> +static void clk_branch2_mem_disable(struct clk_hw *hw) >> +{ >> + struct clk_mem_branch *mem_br = to_clk_mem_branch(hw); >> + >> + regmap_update_bits(mem_br->branch.clkr.regmap, mem_br->mem_enable_reg, >> + mem_br->mem_enable_ack_bit, 0); > > Please add a newline here. > Sure. Thanks, Imran >> + return clk_branch2_disable(hw); >> +} >> +
diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c index fc4735f74f0f..9ac8d04b425a 100644 --- a/drivers/clk/qcom/clk-branch.c +++ b/drivers/clk/qcom/clk-branch.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 /* * Copyright (c) 2013, The Linux Foundation. All rights reserved. + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. */ #include <linux/kernel.h> @@ -134,6 +135,42 @@ static void clk_branch2_disable(struct clk_hw *hw) clk_branch_toggle(hw, false, clk_branch2_check_halt); } +static int clk_branch2_mem_enable(struct clk_hw *hw) +{ + struct clk_mem_branch *mem_br = to_clk_mem_branch(hw); + const char *name = clk_hw_get_name(&mem_br->branch.clkr.hw); + u32 val; + int timeout = 200, ret; + + regmap_update_bits(mem_br->branch.clkr.regmap, mem_br->mem_enable_reg, + mem_br->mem_enable_ack_bit, mem_br->mem_enable_ack_bit); + + ret = regmap_read_poll_timeout(mem_br->branch.clkr.regmap, mem_br->mem_ack_reg, + val, val & mem_br->mem_enable_ack_bit, 0, timeout); + if (ret) { + WARN(1, "%s mem enable failed", name); + return ret; + } + + return clk_branch2_enable(hw); +} + +static void clk_branch2_mem_disable(struct clk_hw *hw) +{ + struct clk_mem_branch *mem_br = to_clk_mem_branch(hw); + + regmap_update_bits(mem_br->branch.clkr.regmap, mem_br->mem_enable_reg, + mem_br->mem_enable_ack_bit, 0); + return clk_branch2_disable(hw); +} + +const struct clk_ops clk_branch2_mem_ops = { + .enable = clk_branch2_mem_enable, + .disable = clk_branch2_mem_disable, + .is_enabled = clk_is_enabled_regmap, +}; +EXPORT_SYMBOL_GPL(clk_branch2_mem_ops); + const struct clk_ops clk_branch2_ops = { .enable = clk_branch2_enable, .disable = clk_branch2_disable, diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h index 0cf800b9d08d..fa1ef1cf1aff 100644 --- a/drivers/clk/qcom/clk-branch.h +++ b/drivers/clk/qcom/clk-branch.h @@ -38,6 +38,23 @@ struct clk_branch { struct clk_regmap clkr; }; +/** + * struct clk_mem_branch - gating clock which are associated with memories + * + * @mem_enable_reg: branch clock memory gating register + * @mem_ack_reg: branch clock memory ack register + * @mem_enable_ack_bit: ANDed with @mem_ack_reg to check memory enablement + * @branch: handle clock gating + * + * Clock which can gate its memories. + */ +struct clk_mem_branch { + u32 mem_enable_reg; + u32 mem_ack_reg; + u32 mem_enable_ack_bit; + struct clk_branch branch; +}; + /* Branch clock common bits for HLOS-owned clocks */ #define CBCR_CLK_OFF BIT(31) #define CBCR_NOC_FSM_STATUS GENMASK(30, 28) @@ -85,8 +102,12 @@ extern const struct clk_ops clk_branch_ops; extern const struct clk_ops clk_branch2_ops; 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; #define to_clk_branch(_hw) \ container_of(to_clk_regmap(_hw), struct clk_branch, clkr) +#define to_clk_mem_branch(_hw) \ + container_of(to_clk_branch(_hw), struct clk_mem_branch, branch) + #endif