| Message ID | 20240330182817.3272224-2-quic_ajipan@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: qcom: Add support for DISPCC, CAMCC and GPUCC on SM4450 | expand |
On 30/03/2024 19:28, Ajit Pandey wrote: > In LUCID EVO PLL CAL_L_VAL and L_VAL bitfields are part of single > PLL_L_VAL register. Update for L_VAL bitfield values in PLL_L_VAL > register using regmap_write() API in __alpha_pll_trion_set_rate > callback will override LUCID EVO PLL initial configuration related > to PLL_CAL_L_VAL bit fields in PLL_L_VAL register. > > Observed random PLL lock failures during PLL enable due to such > override in PLL calibration value. Use regmap_update_bits() with > L_VAL bitfield mask instead of regmap_write() API to update only > PLL_L_VAL bitfields in __alpha_pll_trion_set_rate callback. > > Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces") > No blank lines between tags. Add Cc-stable tag. Please do not combine fixes with new features. Best regards, Krzysztof
On 3/31/2024 12:49 AM, Krzysztof Kozlowski wrote: > On 30/03/2024 19:28, Ajit Pandey wrote: >> In LUCID EVO PLL CAL_L_VAL and L_VAL bitfields are part of single >> PLL_L_VAL register. Update for L_VAL bitfield values in PLL_L_VAL >> register using regmap_write() API in __alpha_pll_trion_set_rate >> callback will override LUCID EVO PLL initial configuration related >> to PLL_CAL_L_VAL bit fields in PLL_L_VAL register. >> >> Observed random PLL lock failures during PLL enable due to such >> override in PLL calibration value. Use regmap_update_bits() with >> L_VAL bitfield mask instead of regmap_write() API to update only >> PLL_L_VAL bitfields in __alpha_pll_trion_set_rate callback. >> >> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces") >> > > No blank lines between tags. > > Add Cc-stable tag. > Sure, will update in next series > Please do not combine fixes with new features. > > Best regards, > Krzysztof > Actually this fix is required for correct scaling for few frequencies in this patch series, hence combined them together and pushed this fix as first patch in series so that they get mainlined together and feature functionality will not get impacted.
On 02/04/2024 20:35, Ajit Pandey wrote: > > > On 3/31/2024 12:49 AM, Krzysztof Kozlowski wrote: >> On 30/03/2024 19:28, Ajit Pandey wrote: >>> In LUCID EVO PLL CAL_L_VAL and L_VAL bitfields are part of single >>> PLL_L_VAL register. Update for L_VAL bitfield values in PLL_L_VAL >>> register using regmap_write() API in __alpha_pll_trion_set_rate >>> callback will override LUCID EVO PLL initial configuration related >>> to PLL_CAL_L_VAL bit fields in PLL_L_VAL register. >>> >>> Observed random PLL lock failures during PLL enable due to such >>> override in PLL calibration value. Use regmap_update_bits() with >>> L_VAL bitfield mask instead of regmap_write() API to update only >>> PLL_L_VAL bitfields in __alpha_pll_trion_set_rate callback. >>> >>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces") >>> >> >> No blank lines between tags. >> >> Add Cc-stable tag. >> > Sure, will update in next series > >> Please do not combine fixes with new features. >> > Best regards, >> Krzysztof >> > > Actually this fix is required for correct scaling for few frequencies in > this patch series, hence combined them together and pushed this fix as > first patch in series so that they get mainlined together and feature > functionality will not get impacted. OK, that's fine but usual way is that such need is expressed in the cover letter, so maintainer will know what to do. What if this patch should go to fixes and rest normally to for-next? How do you expect maintainer to apply the patch? Entire thread and then manually move the commits? Why making it so complicated for the maintainers? Best regards, Krzysztof
On Wed, 3 Apr 2024 at 09:49, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 02/04/2024 20:35, Ajit Pandey wrote: > > > > > > On 3/31/2024 12:49 AM, Krzysztof Kozlowski wrote: > >> On 30/03/2024 19:28, Ajit Pandey wrote: > >>> In LUCID EVO PLL CAL_L_VAL and L_VAL bitfields are part of single > >>> PLL_L_VAL register. Update for L_VAL bitfield values in PLL_L_VAL > >>> register using regmap_write() API in __alpha_pll_trion_set_rate > >>> callback will override LUCID EVO PLL initial configuration related > >>> to PLL_CAL_L_VAL bit fields in PLL_L_VAL register. > >>> > >>> Observed random PLL lock failures during PLL enable due to such > >>> override in PLL calibration value. Use regmap_update_bits() with > >>> L_VAL bitfield mask instead of regmap_write() API to update only > >>> PLL_L_VAL bitfields in __alpha_pll_trion_set_rate callback. > >>> > >>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces") > >>> > >> > >> No blank lines between tags. > >> > >> Add Cc-stable tag. > >> > > Sure, will update in next series > > > >> Please do not combine fixes with new features. > >> > Best regards, > >> Krzysztof > >> > > > > Actually this fix is required for correct scaling for few frequencies in > > this patch series, hence combined them together and pushed this fix as > > first patch in series so that they get mainlined together and feature > > functionality will not get impacted. > > OK, that's fine but usual way is that such need is expressed in the > cover letter, so maintainer will know what to do. What if this patch > should go to fixes and rest normally to for-next? How do you expect > maintainer to apply the patch? Entire thread and then manually move the > commits? Why making it so complicated for the maintainers? Huh? I think it's pretty normal to have fixes in front of the patch series. Having it in the middle would be troublesome indeed. You are the first person to complain. -- With best wishes Dmitry
On 03/04/2024 10:37, Dmitry Baryshkov wrote: > On Wed, 3 Apr 2024 at 09:49, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 02/04/2024 20:35, Ajit Pandey wrote: >>> >>> >>> On 3/31/2024 12:49 AM, Krzysztof Kozlowski wrote: >>>> On 30/03/2024 19:28, Ajit Pandey wrote: >>>>> In LUCID EVO PLL CAL_L_VAL and L_VAL bitfields are part of single >>>>> PLL_L_VAL register. Update for L_VAL bitfield values in PLL_L_VAL >>>>> register using regmap_write() API in __alpha_pll_trion_set_rate >>>>> callback will override LUCID EVO PLL initial configuration related >>>>> to PLL_CAL_L_VAL bit fields in PLL_L_VAL register. >>>>> >>>>> Observed random PLL lock failures during PLL enable due to such >>>>> override in PLL calibration value. Use regmap_update_bits() with >>>>> L_VAL bitfield mask instead of regmap_write() API to update only >>>>> PLL_L_VAL bitfields in __alpha_pll_trion_set_rate callback. >>>>> >>>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces") >>>>> >>>> >>>> No blank lines between tags. >>>> >>>> Add Cc-stable tag. >>>> >>> Sure, will update in next series >>> >>>> Please do not combine fixes with new features. >>>> > Best regards, >>>> Krzysztof >>>> >>> >>> Actually this fix is required for correct scaling for few frequencies in >>> this patch series, hence combined them together and pushed this fix as >>> first patch in series so that they get mainlined together and feature >>> functionality will not get impacted. >> >> OK, that's fine but usual way is that such need is expressed in the >> cover letter, so maintainer will know what to do. What if this patch >> should go to fixes and rest normally to for-next? How do you expect >> maintainer to apply the patch? Entire thread and then manually move the >> commits? Why making it so complicated for the maintainers? > > Huh? I think it's pretty normal to have fixes in front of the patch > series. Having it in the middle would be troublesome indeed. You are > the first person to complain. No, I am not the first. It differs between subsystems and I do not recall all folks, but the one person coming to my mind is Mark Brown who expressed it numerous times. Best regards, Krzysztof
On 4/3/2024 2:20 PM, Krzysztof Kozlowski wrote: > On 03/04/2024 10:37, Dmitry Baryshkov wrote: >> On Wed, 3 Apr 2024 at 09:49, Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> wrote: >>> >>> On 02/04/2024 20:35, Ajit Pandey wrote: >>>> >>>> >>>> On 3/31/2024 12:49 AM, Krzysztof Kozlowski wrote: >>>>> On 30/03/2024 19:28, Ajit Pandey wrote: >>>>>> In LUCID EVO PLL CAL_L_VAL and L_VAL bitfields are part of single >>>>>> PLL_L_VAL register. Update for L_VAL bitfield values in PLL_L_VAL >>>>>> register using regmap_write() API in __alpha_pll_trion_set_rate >>>>>> callback will override LUCID EVO PLL initial configuration related >>>>>> to PLL_CAL_L_VAL bit fields in PLL_L_VAL register. >>>>>> >>>>>> Observed random PLL lock failures during PLL enable due to such >>>>>> override in PLL calibration value. Use regmap_update_bits() with >>>>>> L_VAL bitfield mask instead of regmap_write() API to update only >>>>>> PLL_L_VAL bitfields in __alpha_pll_trion_set_rate callback. >>>>>> >>>>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces") >>>>>> >>>>> >>>>> No blank lines between tags. >>>>> >>>>> Add Cc-stable tag. >>>>> >>>> Sure, will update in next series >>>> >>>>> Please do not combine fixes with new features. >>>>> > Best regards, >>>>> Krzysztof >>>>> >>>> >>>> Actually this fix is required for correct scaling for few frequencies in >>>> this patch series, hence combined them together and pushed this fix as >>>> first patch in series so that they get mainlined together and feature >>>> functionality will not get impacted. >>> >>> OK, that's fine but usual way is that such need is expressed in the >>> cover letter, so maintainer will know what to do. What if this patch >>> should go to fixes and rest normally to for-next? How do you expect >>> maintainer to apply the patch? Entire thread and then manually move the >>> commits? Why making it so complicated for the maintainers? >> OK, for the ease and more clarity I'll update the cover letter with fix details and required dependency on this feature in next series. >> Huh? I think it's pretty normal to have fixes in front of the patch >> series. Having it in the middle would be troublesome indeed. You are >> the first person to complain. > > No, I am not the first. It differs between subsystems and I do not > recall all folks, but the one person coming to my mind is Mark Brown who > expressed it numerous times. > > Best regards, > Krzysztof >
diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c index 8a412ef47e16..81cabd28eabe 100644 --- a/drivers/clk/qcom/clk-alpha-pll.c +++ b/drivers/clk/qcom/clk-alpha-pll.c @@ -1656,7 +1656,7 @@ static int __alpha_pll_trion_set_rate(struct clk_hw *hw, unsigned long rate, if (ret < 0) return ret; - regmap_write(pll->clkr.regmap, PLL_L_VAL(pll), l); + regmap_update_bits(pll->clkr.regmap, PLL_L_VAL(pll), LUCID_EVO_PLL_L_VAL_MASK, l); regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a); /* Latch the PLL input */
In LUCID EVO PLL CAL_L_VAL and L_VAL bitfields are part of single PLL_L_VAL register. Update for L_VAL bitfield values in PLL_L_VAL register using regmap_write() API in __alpha_pll_trion_set_rate callback will override LUCID EVO PLL initial configuration related to PLL_CAL_L_VAL bit fields in PLL_L_VAL register. Observed random PLL lock failures during PLL enable due to such override in PLL calibration value. Use regmap_update_bits() with L_VAL bitfield mask instead of regmap_write() API to update only PLL_L_VAL bitfields in __alpha_pll_trion_set_rate callback. Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces") Signed-off-by: Ajit Pandey <quic_ajipan@quicinc.com> --- drivers/clk/qcom/clk-alpha-pll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)