| Message ID | 20240214183006.3403207-7-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | None | expand |
Quoting Cristian Marussi (2024-02-14 10:30:05) > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 5747b6d651f0..b91a0dbd2fe0 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -158,51 +158,6 @@ static int scmi_clk_atomic_is_enabled(struct clk_hw *hw) > return !!enabled; > } > > -/* > - * We can provide enable/disable/is_enabled atomic callbacks only if the > - * underlying SCMI transport for an SCMI instance is configured to handle > - * SCMI commands in an atomic manner. > - * > - * When no SCMI atomic transport support is available we instead provide only > - * the prepare/unprepare API, as allowed by the clock framework when atomic > - * calls are not available. > - * > - * Two distinct sets of clk_ops are provided since we could have multiple SCMI > - * instances with different underlying transport quality, so they cannot be > - * shared. > - */ > -static const struct clk_ops scmi_clk_ops = { > - .recalc_rate = scmi_clk_recalc_rate, > - .round_rate = scmi_clk_round_rate, > - .set_rate = scmi_clk_set_rate, > - .prepare = scmi_clk_enable, > - .unprepare = scmi_clk_disable, > - .set_parent = scmi_clk_set_parent, > - .get_parent = scmi_clk_get_parent, > - .determine_rate = scmi_clk_determine_rate, > -}; > - > -static const struct clk_ops scmi_atomic_clk_ops = { It's not great to move these function pointer structs out of RO memory to RW. I'm also not convinced that it's any better to construct them at runtime. Isn't there a constant set of possible clk configurations? Or why can't we simply add some failures to the clk_ops functions instead? > - .recalc_rate = scmi_clk_recalc_rate, > - .round_rate = scmi_clk_round_rate, > - .set_rate = scmi_clk_set_rate, > - .enable = scmi_clk_atomic_enable, > - .disable = scmi_clk_atomic_disable, > - .is_enabled = scmi_clk_atomic_is_enabled,
On Wed, Feb 21, 2024 at 09:44:14PM -0800, Stephen Boyd wrote: > Quoting Cristian Marussi (2024-02-14 10:30:05) > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > > index 5747b6d651f0..b91a0dbd2fe0 100644 > > --- a/drivers/clk/clk-scmi.c > > +++ b/drivers/clk/clk-scmi.c > > @@ -158,51 +158,6 @@ static int scmi_clk_atomic_is_enabled(struct clk_hw *hw) > > return !!enabled; > > } > > Hi Stephen, thanks for having a look. > > -/* > > - * We can provide enable/disable/is_enabled atomic callbacks only if the > > - * underlying SCMI transport for an SCMI instance is configured to handle > > - * SCMI commands in an atomic manner. > > - * > > - * When no SCMI atomic transport support is available we instead provide only > > - * the prepare/unprepare API, as allowed by the clock framework when atomic > > - * calls are not available. > > - * > > - * Two distinct sets of clk_ops are provided since we could have multiple SCMI > > - * instances with different underlying transport quality, so they cannot be > > - * shared. > > - */ > > -static const struct clk_ops scmi_clk_ops = { > > - .recalc_rate = scmi_clk_recalc_rate, > > - .round_rate = scmi_clk_round_rate, > > - .set_rate = scmi_clk_set_rate, > > - .prepare = scmi_clk_enable, > > - .unprepare = scmi_clk_disable, > > - .set_parent = scmi_clk_set_parent, > > - .get_parent = scmi_clk_get_parent, > > - .determine_rate = scmi_clk_determine_rate, > > -}; > > - > > -static const struct clk_ops scmi_atomic_clk_ops = { > > It's not great to move these function pointer structs out of RO memory > to RW. I'm also not convinced that it's any better to construct them at > runtime. Isn't there a constant set of possible clk configurations? Or > why can't we simply add some failures to the clk_ops functions instead? Well, the real clock devices managed by the SCMI server can be a of varying nature and so the minimum set of possible clk configurations to cover will amount to all the possible combinations of supported ops regarding the specific clock properties (i.e. .set_parent / .set_rate / .enable / .get/set_duty_cycle / atomic_capability ... for now)...we simply cannot know in advance what the backend SCMI server is handling. These seemed to me too much in number (and growing) to be pre-allocated in all possible combinations. (and mostly wasted since you dont really probably use all combinations all the time) Moreover, SCMI latest spec now exposes some clock properties (or not) to be able avoid even sending an actual SCMI message that we know will be denied all the time; one option is that we return an error,, as you said, but what is the point (I thought) to provide at all a clk-callback that we know upfront will fail to be executed every time ? (and some consumer drivers have been reported by partners not to be happy with these errors) What I think could be optimized here instead, and I will try in the next respin, it is that now I am allocating one set of custom ops for each clock at the end, even if exactly the same ops are provided since the clock capabilities are the same; I could instead allocate dynamically and fill only one single set of ops for each distinct set of combinations, so as to avoid useless duplication and use only the miminum strict amount of RW memory needed. Thanks, Cristian
Quoting Cristian Marussi (2024-02-22 00:28:41) > On Wed, Feb 21, 2024 at 09:44:14PM -0800, Stephen Boyd wrote: > > > > It's not great to move these function pointer structs out of RO memory > > to RW. I'm also not convinced that it's any better to construct them at > > runtime. Isn't there a constant set of possible clk configurations? Or > > why can't we simply add some failures to the clk_ops functions instead? > > Well, the real clock devices managed by the SCMI server can be a of SCMI is a server!? :) > varying nature and so the minimum set of possible clk configurations > to cover will amount to all the possible combinations of supported ops > regarding the specific clock properties (i.e. .set_parent / .set_rate / > .enable / .get/set_duty_cycle / atomic_capability ... for now)...we > simply cannot know in advance what the backend SCMI server is handling. > > These seemed to me too much in number (and growing) to be pre-allocated > in all possible combinations. (and mostly wasted since you dont really > probably use all combinations all the time) > > Moreover, SCMI latest spec now exposes some clock properties (or not) to > be able avoid even sending an actual SCMI message that we know will be > denied all the time; one option is that we return an error,, as you said, > but what is the point (I thought) to provide at all a clk-callback that > we know upfront will fail to be executed every time ? (and some consumer > drivers have been reported by partners not to be happy with these errors) > > What I think could be optimized here instead, and I will try in the next > respin, it is that now I am allocating one set of custom ops for each clock > at the end, even if exactly the same ops are provided since the clock > capabilities are the same; I could instead allocate dynamically and fill only > one single set of ops for each distinct set of combinations, so as to avoid > useless duplication and use only the miminum strict amount of RW memory > needed. > Yes please don't allocate a clk_op per clk. And, please add these answers to the commit text so that we know why it's not possible to know all combinations or fail clk_ops calls.
On Wed, Feb 28, 2024 at 06:20:34PM -0800, Stephen Boyd wrote: > Quoting Cristian Marussi (2024-02-22 00:28:41) > > On Wed, Feb 21, 2024 at 09:44:14PM -0800, Stephen Boyd wrote: > > > > > > It's not great to move these function pointer structs out of RO memory > > > to RW. I'm also not convinced that it's any better to construct them at > > > runtime. Isn't there a constant set of possible clk configurations? Or > > > why can't we simply add some failures to the clk_ops functions instead? > > > > Well, the real clock devices managed by the SCMI server can be a of > > SCMI is a server!? :) > ...well the platform fw act as a server in the client-server SCMI model...so...I know these days it's cooler to be "serverless" but..hey... ...at least is not a BO2k server :P > > varying nature and so the minimum set of possible clk configurations > > to cover will amount to all the possible combinations of supported ops > > regarding the specific clock properties (i.e. .set_parent / .set_rate / > > .enable / .get/set_duty_cycle / atomic_capability ... for now)...we > > simply cannot know in advance what the backend SCMI server is handling. > > > > These seemed to me too much in number (and growing) to be pre-allocated > > in all possible combinations. (and mostly wasted since you dont really > > probably use all combinations all the time) > > > > Moreover, SCMI latest spec now exposes some clock properties (or not) to > > be able avoid even sending an actual SCMI message that we know will be > > denied all the time; one option is that we return an error,, as you said, > > but what is the point (I thought) to provide at all a clk-callback that > > we know upfront will fail to be executed every time ? (and some consumer > > drivers have been reported by partners not to be happy with these errors) > > > > What I think could be optimized here instead, and I will try in the next > > respin, it is that now I am allocating one set of custom ops for each clock > > at the end, even if exactly the same ops are provided since the clock > > capabilities are the same; I could instead allocate dynamically and fill only > > one single set of ops for each distinct set of combinations, so as to avoid > > useless duplication and use only the miminum strict amount of RW memory > > needed. > > > > Yes please don't allocate a clk_op per clk. And, please add these > answers to the commit text so that we know why it's not possible to know > all combinations or fail clk_ops calls. Sure I posted this series a couple of days ago about this rework: https://lore.kernel.org/linux-arm-kernel/20240227194812.1209532-1-cristian.marussi@arm.com/ with a bit of context in the cover-letter and in the commit...but I can add more commenting of course if needed. Thanks for the review, Cristian
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 5747b6d651f0..b91a0dbd2fe0 100644 --- a/drivers/clk/clk-scmi.c +++ b/drivers/clk/clk-scmi.c @@ -158,51 +158,6 @@ static int scmi_clk_atomic_is_enabled(struct clk_hw *hw) return !!enabled; } -/* - * We can provide enable/disable/is_enabled atomic callbacks only if the - * underlying SCMI transport for an SCMI instance is configured to handle - * SCMI commands in an atomic manner. - * - * When no SCMI atomic transport support is available we instead provide only - * the prepare/unprepare API, as allowed by the clock framework when atomic - * calls are not available. - * - * Two distinct sets of clk_ops are provided since we could have multiple SCMI - * instances with different underlying transport quality, so they cannot be - * shared. - */ -static const struct clk_ops scmi_clk_ops = { - .recalc_rate = scmi_clk_recalc_rate, - .round_rate = scmi_clk_round_rate, - .set_rate = scmi_clk_set_rate, - .prepare = scmi_clk_enable, - .unprepare = scmi_clk_disable, - .set_parent = scmi_clk_set_parent, - .get_parent = scmi_clk_get_parent, - .determine_rate = scmi_clk_determine_rate, -}; - -static const struct clk_ops scmi_atomic_clk_ops = { - .recalc_rate = scmi_clk_recalc_rate, - .round_rate = scmi_clk_round_rate, - .set_rate = scmi_clk_set_rate, - .enable = scmi_clk_atomic_enable, - .disable = scmi_clk_atomic_disable, - .is_enabled = scmi_clk_atomic_is_enabled, - .set_parent = scmi_clk_set_parent, - .get_parent = scmi_clk_get_parent, - .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) { @@ -239,6 +194,71 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk, return ret; } +/** + * scmi_clk_ops_alloc() - Alloc and configure CLK ops + * @sclk: A reference to an SCMI clock descriptor + * @atomic_capable: A flag to indicate if atomic mode is supported by the + * transport + * @atomic_threshold: Platform atomic threshold value + * + * Allocate and configure a proper set of CLK operations depending on the + * specific SCMI clock characteristics and platform atomic operation capability. + * + * We can provide enable/disable/is_enabled atomic callbacks only if the + * underlying SCMI transport for an SCMI instance is configured to handle + * SCMI commands in an atomic manner. + * + * When no SCMI atomic transport support is available we instead provide only + * the prepare/unprepare API, as allowed by the clock framework when atomic + * calls are not available. + * + * Return: A pointer to the allocated and configured clk_ops on Success, + * NULL otherwise. + */ +static const struct clk_ops * +scmi_clk_ops_alloc(struct scmi_clk *sclk, bool atomic_capable, + unsigned int atomic_threshold) +{ + const struct scmi_clock_info *ci = sclk->info; + struct clk_ops *ops; + + ops = devm_kzalloc(sclk->dev, sizeof(*ops), GFP_KERNEL); + if (!ops) + return NULL; + + /* + * Note that when transport is atomic but SCMI protocol did not + * specify (or support) an enable_latency associated with a + * clock, we default to use atomic operations mode. + */ + if (!ci->state_ctrl_forbidden) { + if (atomic_capable && ci->enable_latency <= atomic_threshold) { + ops->enable = scmi_clk_atomic_enable; + ops->disable = scmi_clk_atomic_disable; + } else { + ops->prepare = scmi_clk_enable; + ops->unprepare = scmi_clk_disable; + } + } + + if (atomic_capable) + ops->is_enabled = scmi_clk_atomic_is_enabled; + + /* Rate ops */ + ops->recalc_rate = scmi_clk_recalc_rate; + ops->round_rate = scmi_clk_round_rate; + ops->determine_rate = scmi_clk_determine_rate; + if (!ci->rate_ctrl_forbidden) + ops->set_rate = scmi_clk_set_rate; + + /* Parent ops */ + ops->get_parent = scmi_clk_get_parent; + if (!ci->parent_ctrl_forbidden) + ops->set_parent = scmi_clk_set_parent; + + return ops; +} + static int scmi_clocks_probe(struct scmi_device *sdev) { int idx, count, err; @@ -294,18 +314,9 @@ static int scmi_clocks_probe(struct scmi_device *sdev) sclk->ph = ph; sclk->dev = dev; - /* - * Note that when transport is atomic but SCMI protocol did not - * specify (or support) an enable_latency associated with a - * clock, we default to use atomic operations mode. - */ - 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; + scmi_ops = scmi_clk_ops_alloc(sclk, is_atomic, atomic_threshold); + if (!scmi_ops) + return -ENOMEM; /* Initialize clock parent data. */ if (sclk->info->num_parents > 0) { @@ -324,13 +335,13 @@ static int scmi_clocks_probe(struct scmi_device *sdev) if (err) { dev_err(dev, "failed to register clock %d\n", idx); devm_kfree(dev, sclk->parent_data); + devm_kfree(dev, scmi_ops); devm_kfree(dev, sclk); hws[idx] = NULL; } else { dev_dbg(dev, "Registered clock:%s%s\n", sclk->info->name, - scmi_ops == &scmi_atomic_clk_ops ? - " (atomic ops)" : ""); + scmi_ops->enable ? " (atomic ops)" : ""); hws[idx] = &sclk->hw; } }
SCMI Clocks descriptors expose and increasing number of properties that in turn lead to a different set of supported CLK operations to be associated dynamically with a clock. Providing statically pre-defined CLK operations structs for all the possible combinations of allowed properties is cumbersome and error-prone. Allocate per-clock operations descriptor dynamically and populate it with the strictly needed set of operations depending on the advertised clock properties. CC: Michael Turquette <mturquette@baylibre.com> CC: Stephen Boyd <sboyd@kernel.org> CC: linux-clk@vger.kernel.org Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/clk/clk-scmi.c | 129 ++++++++++++++++++++++------------------- 1 file changed, 70 insertions(+), 59 deletions(-)