Some clocks support parent clock dividers but they do not
support clock gating (clk enable/disable). Such types of
clocks might call API's for get/set_reg_val routines with
width as 0 during clk_prepare_enable() call. Handle such
cases by first validating width during clk_prepare_enable()
while still supporting clk_set_rate() correctly.
Signed-off-by: Rahul Tanwar <[email protected]>
---
drivers/clk/x86/clk-cgu.h | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
index 73ce84345f81..46daf9ebd6c9 100644
--- a/drivers/clk/x86/clk-cgu.h
+++ b/drivers/clk/x86/clk-cgu.h
@@ -299,29 +299,51 @@ struct lgm_clk_branch {
static inline void lgm_set_clk_val(struct regmap *membase, u32 reg,
u8 shift, u8 width, u32 set_val)
{
- u32 mask = (GENMASK(width - 1, 0) << shift);
+ u32 mask;
+ /*
+ * Some clocks support parent clock dividers but they do not
+ * support clock gating (clk enable/disable). Such types of
+ * clocks might call this function with width as 0 during
+ * clk_prepare_enable() call. Handle such cases by not doing
+ * anything during clk_prepare_enable() but handle clk_set_rate()
+ * correctly
+ */
+ if (!width)
+ return;
+
+ mask = (GENMASK(width - 1, 0) << shift);
regmap_update_bits(membase, reg, mask, set_val << shift);
}
static inline u32 lgm_get_clk_val(struct regmap *membase, u32 reg,
u8 shift, u8 width)
{
- u32 mask = (GENMASK(width - 1, 0) << shift);
+ u32 mask;
u32 val;
+ /*
+ * Some clocks support parent clock dividers but they do not
+ * support clock gating (clk enable/disable). Such types of
+ * clocks might call this function with width as 0 during
+ * clk_prepare_enable() call. Handle such cases by not doing
+ * anything during clk_prepare_enable() but handle clk_set_rate()
+ * correctly
+ */
+ if (!width)
+ return 0;
+
if (regmap_read(membase, reg, &val)) {
WARN_ONCE(1, "Failed to read clk reg: 0x%x\n", reg);
return 0;
}
+ mask = (GENMASK(width - 1, 0) << shift);
val = (val & mask) >> shift;
return val;
}
-
-
int lgm_clk_register_branches(struct lgm_clk_provider *ctx,
const struct lgm_clk_branch *list,
unsigned int nr_clk);
--
2.17.1
Quoting Rahul Tanwar (2022-09-21 23:24:27)
> Some clocks support parent clock dividers but they do not
> support clock gating (clk enable/disable). Such types of
> clocks might call API's for get/set_reg_val routines with
> width as 0 during clk_prepare_enable() call. Handle such
> cases by first validating width during clk_prepare_enable()
> while still supporting clk_set_rate() correctly.
>
> Signed-off-by: Rahul Tanwar <[email protected]>
> ---
> drivers/clk/x86/clk-cgu.h | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
> index 73ce84345f81..46daf9ebd6c9 100644
> --- a/drivers/clk/x86/clk-cgu.h
> +++ b/drivers/clk/x86/clk-cgu.h
> @@ -299,29 +299,51 @@ struct lgm_clk_branch {
> static inline void lgm_set_clk_val(struct regmap *membase, u32 reg,
> u8 shift, u8 width, u32 set_val)
> {
> - u32 mask = (GENMASK(width - 1, 0) << shift);
> + u32 mask;
>
> + /*
> + * Some clocks support parent clock dividers but they do not
> + * support clock gating (clk enable/disable). Such types of
> + * clocks might call this function with width as 0 during
> + * clk_prepare_enable() call. Handle such cases by not doing
> + * anything during clk_prepare_enable() but handle clk_set_rate()
> + * correctly
> + */
> + if (!width)
> + return;
Why are the clk_ops assigned in a way that makes the code get here? Why
can't we have different clk_ops, or not register the clks at all, when
the hardware can't be written?
> +
> + mask = (GENMASK(width - 1, 0) << shift);
> regmap_update_bits(membase, reg, mask, set_val << shift);
On 29/9/2022 8:20 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
>
>
> Quoting Rahul Tanwar (2022-09-21 23:24:27)
>> Some clocks support parent clock dividers but they do not
>> support clock gating (clk enable/disable). Such types of
>> clocks might call API's for get/set_reg_val routines with
>> width as 0 during clk_prepare_enable() call. Handle such
>> cases by first validating width during clk_prepare_enable()
>> while still supporting clk_set_rate() correctly.
>>
>> Signed-off-by: Rahul Tanwar <[email protected]>
>> ---
>> drivers/clk/x86/clk-cgu.h | 30 ++++++++++++++++++++++++++----
>> 1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
>> index 73ce84345f81..46daf9ebd6c9 100644
>> --- a/drivers/clk/x86/clk-cgu.h
>> +++ b/drivers/clk/x86/clk-cgu.h
>> @@ -299,29 +299,51 @@ struct lgm_clk_branch {
>> static inline void lgm_set_clk_val(struct regmap *membase, u32 reg,
>> u8 shift, u8 width, u32 set_val)
>> {
>> - u32 mask = (GENMASK(width - 1, 0) << shift);
>> + u32 mask;
>>
>> + /*
>> + * Some clocks support parent clock dividers but they do not
>> + * support clock gating (clk enable/disable). Such types of
>> + * clocks might call this function with width as 0 during
>> + * clk_prepare_enable() call. Handle such cases by not doing
>> + * anything during clk_prepare_enable() but handle clk_set_rate()
>> + * correctly
>> + */
>> + if (!width)
>> + return;
>
> Why are the clk_ops assigned in a way that makes the code get here? Why
> can't we have different clk_ops, or not register the clks at all, when
> the hardware can't be written?
The hardware can actually be written for such clks but only for
clk_set_rate() op for setting the clk rate. Just that hardware does not
provide any way to enable/disable such clks.
Alternative way to handle such clks could be that the clk consumer does
not invoke clk_prepare_enable() before invoking clk_set_rate(). But we
want to avoid making changes in the clk consumer code to keep it
standard. And handle it here by just validating the width parameter.
Thanks,
Rahul
>
>> +
>> + mask = (GENMASK(width - 1, 0) << shift);
>> regmap_update_bits(membase, reg, mask, set_val << shift);
>
>
[Resend due to mail delivery failure in earlier reply - one email id got
corrupted somehow in earlier reply]
Hi Stephen,
On 30/9/2022 9:02 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
>
>
> Quoting Rahul Tanwar (2022-09-28 23:10:10)
>> On 29/9/2022 8:20 am, Stephen Boyd wrote:
>>>> + u32 mask;
>>>>
>>>> + /*
>>>> + * Some clocks support parent clock dividers but they do not
>>>> + * support clock gating (clk enable/disable). Such types of
>>>> + * clocks might call this function with width as 0 during
>>>> + * clk_prepare_enable() call. Handle such cases by not doing
>>>> + * anything during clk_prepare_enable() but handle clk_set_rate()
>>>> + * correctly
>>>> + */
>>>> + if (!width)
>>>> + return;
>>>
>>> Why are the clk_ops assigned in a way that makes the code get here? Why
>>> can't we have different clk_ops, or not register the clks at all, when
>>> the hardware can't be written?
>>
>>
>> The hardware can actually be written for such clks but only for
>> clk_set_rate() op for setting the clk rate. Just that hardware does not
>> provide any way to enable/disable such clks.
>>
>> Alternative way to handle such clks could be that the clk consumer does
>> not invoke clk_prepare_enable() before invoking clk_set_rate(). But we
>> want to avoid making changes in the clk consumer code to keep it
>> standard. And handle it here by just validating the width parameter.
>
> Why not have different clk_ops then that doesn't do anything for
> enable/disable and only does it for set_rate?
>
There is only one clk entry which falls in this category. Adding a
different clk_ops for just one clk would need many more lines of code
addition which appears to be a overkill.
I have removed this change in v3 and used the driver internal flag to
handle this particular clk. That requires minimal change and looks
logical addition.
Thanks,
Rahul
>