| Message ID | 20230808051407.647395-3-quic_imrashai@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for Qualcomm ECPRI clock controller | expand |
On 8.08.2023 07:14, 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. 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> > --- Could you expand the commit message a bit? What does this clock memory do? [..] > +static int clk_branch2_mem_enable(struct clk_hw *hw) > +{ > + struct clk_branch *br = to_clk_branch(hw); > + u32 val; > + int count = 200; > + > + regmap_update_bits(br->clkr.regmap, br->mem_enable_reg, > + br->mem_enable_ack_bit, br->mem_enable_ack_bit); > + > + regmap_read(br->clkr.regmap, br->mem_ack_reg, &val); > + > + while (count-- > 0) { > + if (val & br->mem_enable_ack_bit) > + return clk_branch2_enable(hw); > + udelay(1); > + regmap_read(br->clkr.regmap, br->mem_ack_reg, &val); > + } readl_poll_timeout? Konrad
On 8.08.2023 07:14, 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. 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 | 38 +++++++++++++++++++++++++++++++++++ > drivers/clk/qcom/clk-branch.h | 4 ++++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c > index fc4735f74f0f..95ffcd380039 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,43 @@ 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_branch *br = to_clk_branch(hw); > + u32 val; > + int count = 200; > + > + regmap_update_bits(br->clkr.regmap, br->mem_enable_reg, > + br->mem_enable_ack_bit, br->mem_enable_ack_bit); > + > + regmap_read(br->clkr.regmap, br->mem_ack_reg, &val); > + > + while (count-- > 0) { > + if (val & br->mem_enable_ack_bit) One more comment, since the variable is named "ack bit", perhaps the value within could be a bit number and you could use BIT() here. Otherwise with you having chosen u8 for the type, there's not a whole lot of flexibility. Konrad
Quoting Imran Shaik (2023-08-07 22:14:05) > diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h > index 0cf800b9d08d..0ffda6bef00e 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 mem_enable_reg; > + u32 mem_ack_reg; > u8 hwcg_bit; > u8 halt_bit; > + u8 mem_enable_ack_bit; > u8 halt_check; > #define BRANCH_VOTED BIT(7) /* Delay on disable */ > #define BRANCH_HALT 0 /* pol: 1 = halt */ I suspect making a wrapper around struct clk_branch would be a better approach so that we don't bloat all the other clk_branch structures that exist in the qcom clk drivers. $ git grep 'struct clk_branch' -- drivers/clk/qcom | wc -l 6357 How many of these are going to be using these new registers? It may also make sense to do that for hardware clock gating as well, but I'm not really sure. Anyway, the idea is struct clk_mem_branch { u32 enable_reg; u32 ack_reg; u8 ack_bit; struct clk_branch branch; }; and then a container_of define. Plus, you can put some comment above the structure to describe when these clks are used.
On 8/23/2023 12:22 AM, Stephen Boyd wrote: > Quoting Imran Shaik (2023-08-07 22:14:05) >> diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h >> index 0cf800b9d08d..0ffda6bef00e 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 mem_enable_reg; >> + u32 mem_ack_reg; >> u8 hwcg_bit; >> u8 halt_bit; >> + u8 mem_enable_ack_bit; >> u8 halt_check; >> #define BRANCH_VOTED BIT(7) /* Delay on disable */ >> #define BRANCH_HALT 0 /* pol: 1 = halt */ > > I suspect making a wrapper around struct clk_branch would be a better > approach so that we don't bloat all the other clk_branch structures that > exist in the qcom clk drivers. > > $ git grep 'struct clk_branch' -- drivers/clk/qcom | wc -l > 6357 > > How many of these are going to be using these new registers? It may also > make sense to do that for hardware clock gating as well, but I'm not > really sure. Anyway, the idea is > > struct clk_mem_branch { > u32 enable_reg; > u32 ack_reg; > u8 ack_bit; > struct clk_branch branch; > }; > > and then a container_of define. Plus, you can put some comment above the > structure to describe when these clks are used. Sure, will use the approach mentioned above and push the next series. Thanks, Imran
On 8/10/2023 1:29 AM, Konrad Dybcio wrote: > On 8.08.2023 07:14, 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. 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 | 38 +++++++++++++++++++++++++++++++++++ >> drivers/clk/qcom/clk-branch.h | 4 ++++ >> 2 files changed, 42 insertions(+) >> >> diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c >> index fc4735f74f0f..95ffcd380039 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,43 @@ 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_branch *br = to_clk_branch(hw); >> + u32 val; >> + int count = 200; >> + >> + regmap_update_bits(br->clkr.regmap, br->mem_enable_reg, >> + br->mem_enable_ack_bit, br->mem_enable_ack_bit); >> + >> + regmap_read(br->clkr.regmap, br->mem_ack_reg, &val); >> + >> + while (count-- > 0) { >> + if (val & br->mem_enable_ack_bit) > One more comment, since the variable is named "ack bit", perhaps the > value within could be a bit number and you could use BIT() here. > > Otherwise with you having chosen u8 for the type, there's not a whole > lot of flexibility. > Sure, will check and update accordingly in next series. Thanks, Imran > Konrad
On 8/10/2023 1:27 AM, Konrad Dybcio wrote: > On 8.08.2023 07:14, 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. 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> >> --- > Could you expand the commit message a bit? What does this clock > memory do? > Sure, will expand the commit message with more details and push the next series. > [..] > >> +static int clk_branch2_mem_enable(struct clk_hw *hw) >> +{ >> + struct clk_branch *br = to_clk_branch(hw); >> + u32 val; >> + int count = 200; >> + >> + regmap_update_bits(br->clkr.regmap, br->mem_enable_reg, >> + br->mem_enable_ack_bit, br->mem_enable_ack_bit); >> + >> + regmap_read(br->clkr.regmap, br->mem_ack_reg, &val); >> + >> + while (count-- > 0) { >> + if (val & br->mem_enable_ack_bit) >> + return clk_branch2_enable(hw); >> + udelay(1); >> + regmap_read(br->clkr.regmap, br->mem_ack_reg, &val); >> + } > readl_poll_timeout? > Sure, will check and use this. Thanks, Imran > Konrad
diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c index fc4735f74f0f..95ffcd380039 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,43 @@ 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_branch *br = to_clk_branch(hw); + u32 val; + int count = 200; + + regmap_update_bits(br->clkr.regmap, br->mem_enable_reg, + br->mem_enable_ack_bit, br->mem_enable_ack_bit); + + regmap_read(br->clkr.regmap, br->mem_ack_reg, &val); + + while (count-- > 0) { + if (val & br->mem_enable_ack_bit) + return clk_branch2_enable(hw); + udelay(1); + regmap_read(br->clkr.regmap, br->mem_ack_reg, &val); + } + + return -EBUSY; +} + +static void clk_branch2_mem_disable(struct clk_hw *hw) +{ + struct clk_branch *br = to_clk_branch(hw); + + regmap_update_bits(br->clkr.regmap, br->mem_enable_reg, + 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(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..0ffda6bef00e 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 mem_enable_reg; + u32 mem_ack_reg; u8 hwcg_bit; u8 halt_bit; + u8 mem_enable_ack_bit; u8 halt_check; #define BRANCH_VOTED BIT(7) /* Delay on disable */ #define BRANCH_HALT 0 /* pol: 1 = halt */ @@ -85,6 +88,7 @@ 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)