| Message ID | 3bcdfdf0f66dd2fdcffbdeabb5e3ab0bfb2e3489.1663827071.git.rtanwar@maxlinear.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Modify MxL's CGU clk driver to make it secure boot compatible | expand |
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 <rtanwar@maxlinear.com> > --- > 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 <rtanwar@maxlinear.com> >> --- >> 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); > >
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 >
[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 >
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);
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 <rtanwar@maxlinear.com> --- drivers/clk/x86/clk-cgu.h | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)