| Message ID | 20231207093345.581048-2-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [V2,1/2] firmware: arm_scmi: clock: implement get permissions | expand |
On Thu, Dec 07, 2023 at 05:33:45PM +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: > Hi, > 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. > So I was puzzled a bit at first (as said) by the fact that here we silently swallow the failure if the SCMI Clock cannot be disabled, BUT then I spotted in include/linux/clk.h /** * clk_enable - inform the system when the clock source should be running. * @clk: clock source * * If the clock can not be enabled/disabled, this should return success. ...so I suppose it is fine for the CLK framework at the end. My next remaining question is why are you not doing the same when (ret == -EACCES && clk->info->state_ctrl_forbidden) for atomic_ops ? I.e. in: clk-scmi.c::static int scmi_clk_atomic_enable(struct clk_hw *hw) Any particular reason (beside not needing it in your particular case...) Thanks, Cristian
> Subject: Re: [PATCH V2 2/2] clk: scmi: support state_ctrl_forbidden > > On Thu, Dec 07, 2023 at 05:33:45PM +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: > > > > Hi, > > > 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. > > > > > So I was puzzled a bit at first (as said) by the fact that here we silently swallow > the failure if the SCMI Clock cannot be disabled, BUT then I spotted in > include/linux/clk.h > > /** > * clk_enable - inform the system when the clock source should be > running. > * @clk: clock source > * > * If the clock can not be enabled/disabled, this should return success. > > ...so I suppose it is fine for the CLK framework at the end. > > My next remaining question is why are you not doing the same when (ret == - > EACCES && clk->info->state_ctrl_forbidden) for atomic_ops ? > > I.e. in: > > clk-scmi.c::static int scmi_clk_atomic_enable(struct clk_hw *hw) > > Any particular reason (beside not needing it in your particular case...) No particular reason, we not use atomic_ops in our case. So I am not able to test it. I could add the same in atomic_ops in V3. Thanks, Peng. > > Thanks, > Cristian
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 8cbe24789c24..1d809813eabd 100644 --- a/drivers/clk/clk-scmi.c +++ b/drivers/clk/clk-scmi.c @@ -119,8 +119,14 @@ 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); - return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC); + if (ret == -EACCES && clk->info->state_ctrl_forbidden) + return 0; + + return ret; } static void scmi_clk_disable(struct clk_hw *hw)