Messages in this thread | | | Date | Sat, 20 Jan 2024 10:44:06 +0800 | Subject | Re: [PATCH V3 2/2] clk: scmi: support state_ctrl_forbidden | From | Peng Fan <> |
| |
在 1/19/2024 2:27 AM, Cristian Marussi 写道: > On Mon, Jan 15, 2024 at 02:02:03PM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> Some clocks may exported to linux, while those clocks are not allowed >> to configure by Linux. For example: >> >> SYS_CLK1----- >> \ >> --MUX--->MMC1_CLK >> / >> SYS_CLK2----- >> >> MMC1 needs set parent, so SYS_CLK1 and SYS_CLK2 are exported to Linux, >> then the clk propagation will touch SYS_CLK1 or SYS_CLK2. >> So we need bypass the failure for SYS_CLK1 or SYS_CLK2 when enable >> the clock of MMC1. >> > > Hi, > > so this looks good to me and apparently (as noted) the CLK framework is OK > with a driver swallowing the -EACCESS when a clock is immutable, BUT at the > end of the day do we even need to try this SCMI call and hide the failure in > case of immutable clocks ? > > I mean, what if we just dont provide any callback for enable/disable...I can > see plenty of drivers not providing those callbacks ? > Maybe this is probably more of a question for Stephen... > > IOW what about doing something like below...does it make any difference > in your setup ? works fine in my emulated env
It should be fine to use your changes. Do you expect me to use your patch or make it as a follow up patch?
> > (Note that last snippet in clk_gate_restore_context() is probably a fix > that needs to be added anyway by looking at the code in clk.c)
This API seems only used by TI gate driver, this change should be in a standalone change go through clk tree. So I would your changes as a standalone optimization follow up patch, while not included in my patchset.
THanks, Peng.
> > Thanks, > Cristian > > --->8---- > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 5327e0547741..a669a2f2f78b 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -121,11 +121,7 @@ static int scmi_clk_enable(struct clk_hw *hw) > struct scmi_clk *clk = to_scmi_clk(hw); > int ret; > > - ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC); > - if (ret == -EACCES && clk->info->state_ctrl_forbidden) > - return 0; > - > - return ret; > + return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC); > } > > static void scmi_clk_disable(struct clk_hw *hw) > @@ -140,11 +136,7 @@ static int scmi_clk_atomic_enable(struct clk_hw *hw) > struct scmi_clk *clk = to_scmi_clk(hw); > int ret; > > - ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC); > - if (ret == -EACCES && clk->info->state_ctrl_forbidden) > - return 0; > - > - return ret; > + return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC); > } > > static void scmi_clk_atomic_disable(struct clk_hw *hw) > @@ -204,6 +196,15 @@ static const struct clk_ops scmi_atomic_clk_ops = { > .determine_rate = scmi_clk_determine_rate, > }; > > +static const struct clk_ops scmi_no_state_ctrl_clk_ops = { > + .recalc_rate = scmi_clk_recalc_rate, > + .round_rate = scmi_clk_round_rate, > + .set_rate = scmi_clk_set_rate, > + .set_parent = scmi_clk_set_parent, > + .get_parent = scmi_clk_get_parent, > + .determine_rate = scmi_clk_determine_rate, > +}; > + > static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk, > const struct clk_ops *scmi_ops) > { > @@ -300,8 +301,10 @@ static int scmi_clocks_probe(struct scmi_device *sdev) > * specify (or support) an enable_latency associated with a > * clock, we default to use atomic operations mode. > */ > - if (is_atomic && > - sclk->info->enable_latency <= atomic_threshold) > + if (sclk->info->state_ctrl_forbidden) > + scmi_ops = &scmi_no_state_ctrl_clk_ops; > + else if (is_atomic && > + sclk->info->enable_latency <= atomic_threshold) > scmi_ops = &scmi_atomic_clk_ops; > else > scmi_ops = &scmi_clk_ops; > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index f0940af485a5..79b90a8099d7 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1200,9 +1200,11 @@ void clk_gate_restore_context(struct clk_hw *hw) > struct clk_core *core = hw->core; > > if (core->enable_count) > - core->ops->enable(hw); > + if (core->ops->enable) > + core->ops->enable(hw); > else > - core->ops->disable(hw); > + if (core->ops->disable) > + core->ops->disable(hw); > } > EXPORT_SYMBOL_GPL(clk_gate_restore_context); > ---8<--- > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> >> V3: >> Add check in atomic enable >> >> V2: >> New. Take Cristian's suggestion >> >> drivers/clk/clk-scmi.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c >> index 8cbe24789c24..5327e0547741 100644 >> --- a/drivers/clk/clk-scmi.c >> +++ b/drivers/clk/clk-scmi.c >> @@ -119,8 +119,13 @@ static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *r >> static int scmi_clk_enable(struct clk_hw *hw) >> { >> struct scmi_clk *clk = to_scmi_clk(hw); >> + int ret; >> + >> + ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC); >> + if (ret == -EACCES && clk->info->state_ctrl_forbidden) >> + return 0; >> >> - return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC); >> + return ret; >> } >> >> static void scmi_clk_disable(struct clk_hw *hw) >> @@ -133,8 +138,13 @@ static void scmi_clk_disable(struct clk_hw *hw) >> static int scmi_clk_atomic_enable(struct clk_hw *hw) >> { >> struct scmi_clk *clk = to_scmi_clk(hw); >> + int ret; >> + >> + ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC); >> + if (ret == -EACCES && clk->info->state_ctrl_forbidden) >> + return 0; >> >> - return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC); >> + return ret; >> } >> >> static void scmi_clk_atomic_disable(struct clk_hw *hw) >> -- >> 2.37.1 >>
| |