| Message ID | 20240115060203.813168-2-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [V3,1/2] firmware: arm_scmi: Implement Clock get permissions | expand |
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 (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) 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 >
在 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 >>
On Sat, Jan 20, 2024 at 10:44:06AM +0800, Peng Fan wrote: > > > 在 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? > It was just a suggestion, if you think is fine just include it in your series, I dont mind. > > > > (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. Yes, indeed I have made a small patch of it to post it separately..and I forgot :D... sending it now. Thanks, Cristian
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)