From: Peng Fan <[email protected]>
ARM SCMI Spec 3.2 introduces Clock Get Permission command. This patch
is to add the support. Add three bool entries to scmi_clock_info to
indicate the operation is forbidden or not. If the CLOCK_GET_PERMISSIONS
command is not supported, the three bool variables will default
set to false, otherwise they will be set according to the return result
of CLOCK_GET_PERMISSIONS.
Signed-off-by: Peng Fan <[email protected]>
---
V3:
Rebased on https://lore.kernel.org/linux-arm-kernel/[email protected]/
Drop attribute which is no needed
Use scmi_clock_domain_lookup
Update patch subject
V2:
Take Cristian's suggestion, https://lore.kernel.org/all/ZWiqqfQ73tezFmSk@pluto/
drivers/firmware/arm_scmi/clock.c | 64 +++++++++++++++++++++++++++++++
include/linux/scmi_protocol.h | 3 ++
2 files changed, 67 insertions(+)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 2e4d6479a639..959e48aba1b5 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -28,8 +28,13 @@ enum scmi_clock_protocol_cmd {
CLOCK_POSSIBLE_PARENTS_GET = 0xC,
CLOCK_PARENT_SET = 0xD,
CLOCK_PARENT_GET = 0xE,
+ CLOCK_GET_PERMISSIONS = 0xF,
};
+#define CLOCK_STATE_CONTROL_ALLOWED BIT(31)
+#define CLOCK_PARENT_CONTROL_ALLOWED BIT(30)
+#define CLOCK_RATE_CONTROL_ALLOWED BIT(29)
+
enum clk_state {
CLK_STATE_DISABLE,
CLK_STATE_ENABLE,
@@ -49,6 +54,7 @@ struct scmi_msg_resp_clock_attributes {
#define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) & BIT(30))
#define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
#define SUPPORTS_PARENT_CLOCK(x) ((x) & BIT(28))
+#define SUPPORTS_GET_PERMISSIONS(x) ((x) & BIT(1))
u8 name[SCMI_SHORT_NAME_MAX_SIZE];
__le32 clock_enable_latency;
};
@@ -293,6 +299,35 @@ static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph, u3
return ret;
}
+static int
+scmi_clock_get_permissions(const struct scmi_protocol_handle *ph, u32 clk_id,
+ struct scmi_clock_info *clk)
+{
+ struct scmi_xfer *t;
+ u32 perm;
+ int ret;
+
+ ret = ph->xops->xfer_get_init(ph, CLOCK_GET_PERMISSIONS,
+ sizeof(clk_id), sizeof(perm), &t);
+ if (ret)
+ return ret;
+
+ put_unaligned_le32(clk_id, t->tx.buf);
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (!ret) {
+ perm = get_unaligned_le32(t->rx.buf);
+
+ clk->state_ctrl_forbidden = !(perm & CLOCK_STATE_CONTROL_ALLOWED);
+ clk->rate_ctrl_forbidden = !(perm & CLOCK_RATE_CONTROL_ALLOWED);
+ clk->parent_ctrl_forbidden = !(perm & CLOCK_PARENT_CONTROL_ALLOWED);
+ }
+
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
u32 clk_id, struct scmi_clock_info *clk,
u32 version)
@@ -339,6 +374,8 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
clk->rate_change_requested_notifications = true;
if (SUPPORTS_PARENT_CLOCK(attributes))
scmi_clock_possible_parents(ph, clk_id, clk);
+ if (SUPPORTS_GET_PERMISSIONS(attributes))
+ scmi_clock_get_permissions(ph, clk_id, clk);
}
return ret;
@@ -511,6 +548,14 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
struct scmi_xfer *t;
struct scmi_clock_set_rate *cfg;
struct clock_info *ci = ph->get_priv(ph);
+ struct scmi_clock_info *clk;
+
+ clk = scmi_clock_domain_lookup(ci, clk_id);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ if (clk->rate_ctrl_forbidden)
+ return -EACCES;
ret = ph->xops->xfer_get_init(ph, CLOCK_RATE_SET, sizeof(*cfg), 0, &t);
if (ret)
@@ -596,6 +641,9 @@ scmi_clock_set_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
if (parent_id >= clk->num_parents)
return -EINVAL;
+ if (clk->parent_ctrl_forbidden)
+ return -EACCES;
+
ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
sizeof(*cfg), 0, &t);
if (ret)
@@ -679,6 +727,14 @@ static int scmi_clock_enable(const struct scmi_protocol_handle *ph, u32 clk_id,
bool atomic)
{
struct clock_info *ci = ph->get_priv(ph);
+ struct scmi_clock_info *clk;
+
+ clk = scmi_clock_domain_lookup(ci, clk_id);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ if (clk->state_ctrl_forbidden)
+ return -EACCES;
return ci->clock_config_set(ph, clk_id, CLK_STATE_ENABLE,
NULL_OEM_TYPE, 0, atomic);
@@ -688,6 +744,14 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
bool atomic)
{
struct clock_info *ci = ph->get_priv(ph);
+ struct scmi_clock_info *clk;
+
+ clk = scmi_clock_domain_lookup(ci, clk_id);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ if (clk->state_ctrl_forbidden)
+ return -EACCES;
return ci->clock_config_set(ph, clk_id, CLK_STATE_DISABLE,
NULL_OEM_TYPE, 0, atomic);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index f2f05fb42d28..0cc40af5519a 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -47,6 +47,9 @@ struct scmi_clock_info {
bool rate_discrete;
bool rate_changed_notifications;
bool rate_change_requested_notifications;
+ bool state_ctrl_forbidden;
+ bool rate_ctrl_forbidden;
+ bool parent_ctrl_forbidden;
union {
struct {
int num_rates;
--
2.37.1
From: Peng Fan <[email protected]>
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.
Signed-off-by: Peng Fan <[email protected]>
---
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 Mon, Jan 15, 2024 at 02:02:02PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> ARM SCMI Spec 3.2 introduces Clock Get Permission command. This patch
> is to add the support. Add three bool entries to scmi_clock_info to
> indicate the operation is forbidden or not. If the CLOCK_GET_PERMISSIONS
> command is not supported, the three bool variables will default
> set to false, otherwise they will be set according to the return result
> of CLOCK_GET_PERMISSIONS.
>
LGTM.
Reviewed-by: Cristian Marussi <[email protected]>
Thanks,
Cristian
> Signed-off-by: Peng Fan <[email protected]>
> ---
>
> V3:
> Rebased on https://lore.kernel.org/linux-arm-kernel/[email protected]/
> Drop attribute which is no needed
> Use scmi_clock_domain_lookup
> Update patch subject
>
> V2:
> Take Cristian's suggestion, https://lore.kernel.org/all/ZWiqqfQ73tezFmSk@pluto/
>
> drivers/firmware/arm_scmi/clock.c | 64 +++++++++++++++++++++++++++++++
> include/linux/scmi_protocol.h | 3 ++
> 2 files changed, 67 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 2e4d6479a639..959e48aba1b5 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -28,8 +28,13 @@ enum scmi_clock_protocol_cmd {
> CLOCK_POSSIBLE_PARENTS_GET = 0xC,
> CLOCK_PARENT_SET = 0xD,
> CLOCK_PARENT_GET = 0xE,
> + CLOCK_GET_PERMISSIONS = 0xF,
> };
>
> +#define CLOCK_STATE_CONTROL_ALLOWED BIT(31)
> +#define CLOCK_PARENT_CONTROL_ALLOWED BIT(30)
> +#define CLOCK_RATE_CONTROL_ALLOWED BIT(29)
> +
> enum clk_state {
> CLK_STATE_DISABLE,
> CLK_STATE_ENABLE,
> @@ -49,6 +54,7 @@ struct scmi_msg_resp_clock_attributes {
> #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) & BIT(30))
> #define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
> #define SUPPORTS_PARENT_CLOCK(x) ((x) & BIT(28))
> +#define SUPPORTS_GET_PERMISSIONS(x) ((x) & BIT(1))
> u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> __le32 clock_enable_latency;
> };
> @@ -293,6 +299,35 @@ static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph, u3
> return ret;
> }
>
> +static int
> +scmi_clock_get_permissions(const struct scmi_protocol_handle *ph, u32 clk_id,
> + struct scmi_clock_info *clk)
> +{
> + struct scmi_xfer *t;
> + u32 perm;
> + int ret;
> +
> + ret = ph->xops->xfer_get_init(ph, CLOCK_GET_PERMISSIONS,
> + sizeof(clk_id), sizeof(perm), &t);
> + if (ret)
> + return ret;
> +
> + put_unaligned_le32(clk_id, t->tx.buf);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + perm = get_unaligned_le32(t->rx.buf);
> +
> + clk->state_ctrl_forbidden = !(perm & CLOCK_STATE_CONTROL_ALLOWED);
> + clk->rate_ctrl_forbidden = !(perm & CLOCK_RATE_CONTROL_ALLOWED);
> + clk->parent_ctrl_forbidden = !(perm & CLOCK_PARENT_CONTROL_ALLOWED);
> + }
> +
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> u32 clk_id, struct scmi_clock_info *clk,
> u32 version)
> @@ -339,6 +374,8 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> clk->rate_change_requested_notifications = true;
> if (SUPPORTS_PARENT_CLOCK(attributes))
> scmi_clock_possible_parents(ph, clk_id, clk);
> + if (SUPPORTS_GET_PERMISSIONS(attributes))
> + scmi_clock_get_permissions(ph, clk_id, clk);
> }
>
> return ret;
> @@ -511,6 +548,14 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
> struct scmi_xfer *t;
> struct scmi_clock_set_rate *cfg;
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk;
> +
> + clk = scmi_clock_domain_lookup(ci, clk_id);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + if (clk->rate_ctrl_forbidden)
> + return -EACCES;
>
> ret = ph->xops->xfer_get_init(ph, CLOCK_RATE_SET, sizeof(*cfg), 0, &t);
> if (ret)
> @@ -596,6 +641,9 @@ scmi_clock_set_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
> if (parent_id >= clk->num_parents)
> return -EINVAL;
>
> + if (clk->parent_ctrl_forbidden)
> + return -EACCES;
> +
> ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
> sizeof(*cfg), 0, &t);
> if (ret)
> @@ -679,6 +727,14 @@ static int scmi_clock_enable(const struct scmi_protocol_handle *ph, u32 clk_id,
> bool atomic)
> {
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk;
> +
> + clk = scmi_clock_domain_lookup(ci, clk_id);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + if (clk->state_ctrl_forbidden)
> + return -EACCES;
>
> return ci->clock_config_set(ph, clk_id, CLK_STATE_ENABLE,
> NULL_OEM_TYPE, 0, atomic);
> @@ -688,6 +744,14 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
> bool atomic)
> {
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk;
> +
> + clk = scmi_clock_domain_lookup(ci, clk_id);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + if (clk->state_ctrl_forbidden)
> + return -EACCES;
>
> return ci->clock_config_set(ph, clk_id, CLK_STATE_DISABLE,
> NULL_OEM_TYPE, 0, atomic);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index f2f05fb42d28..0cc40af5519a 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -47,6 +47,9 @@ struct scmi_clock_info {
> bool rate_discrete;
> bool rate_changed_notifications;
> bool rate_change_requested_notifications;
> + bool state_ctrl_forbidden;
> + bool rate_ctrl_forbidden;
> + bool parent_ctrl_forbidden;
> union {
> struct {
> int num_rates;
> --
> 2.37.1
>
On Mon, Jan 15, 2024 at 02:02:03PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>
>> 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 <[email protected]>
> > >
> > > 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