| Message ID | 20231103105712.1159213-1-quic_adhudase@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | soc: qcom: llcc: Fix dis_cap_alloc and retain_on_pc configuration | expand |
On 11/3/2023 4:27 PM, Atul Dhudase wrote: > While programming dis_cap_alloc and retain_on_pc, set a bit > corresponding to a specific SCID without disturbing the > previously configured bits. > > Fixes: c14e64b46944 ("soc: qcom: llcc: Support chipsets that can write to llcc") > Signed-off-by: Atul Dhudase <quic_adhudase@quicinc.com> > --- > drivers/soc/qcom/llcc-qcom.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c > index 674abd0d6700..509d972c1bd9 100644 > --- a/drivers/soc/qcom/llcc-qcom.c > +++ b/drivers/soc/qcom/llcc-qcom.c > @@ -941,15 +941,15 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config, > u32 disable_cap_alloc, retain_pc; > > disable_cap_alloc = config->dis_cap_alloc << config->slice_id; > - ret = regmap_write(drv_data->bcast_regmap, > - LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc); > + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_SCID_DIS_CAP_ALLOC, > + BIT(config->slice_id), disable_cap_alloc); > if (ret) > return ret; > > if (drv_data->version < LLCC_VERSION_4_1_0_0) { > retain_pc = config->retain_on_pc << config->slice_id; > - ret = regmap_write(drv_data->bcast_regmap, > - LLCC_TRP_PCB_ACT, retain_pc); > + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_PCB_ACT, > + BIT(config->slice_id), retain_pc); Good catch, LGTM Thanks -Mukesh > if (ret) > return ret; > } > -- > 2.25.1 >
Hi, On Fri, Nov 3, 2023 at 3:57 AM Atul Dhudase <quic_adhudase@quicinc.com> wrote: > > While programming dis_cap_alloc and retain_on_pc, set a bit > corresponding to a specific SCID without disturbing the > previously configured bits. > > Fixes: c14e64b46944 ("soc: qcom: llcc: Support chipsets that can write to llcc") > Signed-off-by: Atul Dhudase <quic_adhudase@quicinc.com> > --- > drivers/soc/qcom/llcc-qcom.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c > index 674abd0d6700..509d972c1bd9 100644 > --- a/drivers/soc/qcom/llcc-qcom.c > +++ b/drivers/soc/qcom/llcc-qcom.c > @@ -941,15 +941,15 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config, > u32 disable_cap_alloc, retain_pc; > > disable_cap_alloc = config->dis_cap_alloc << config->slice_id; > - ret = regmap_write(drv_data->bcast_regmap, > - LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc); > + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_SCID_DIS_CAP_ALLOC, > + BIT(config->slice_id), disable_cap_alloc); > if (ret) > return ret; > > if (drv_data->version < LLCC_VERSION_4_1_0_0) { > retain_pc = config->retain_on_pc << config->slice_id; > - ret = regmap_write(drv_data->bcast_regmap, > - LLCC_TRP_PCB_ACT, retain_pc); > + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_PCB_ACT, > + BIT(config->slice_id), retain_pc); Wow, that seems pretty serious. As far as I can tell this looks correct. Reviewed-by: Douglas Anderson <dianders@chromium.org>
On 11/3/2023 4:47 PM, Mukesh Ojha wrote: > > > On 11/3/2023 4:27 PM, Atul Dhudase wrote: >> While programming dis_cap_alloc and retain_on_pc, set a bit >> corresponding to a specific SCID without disturbing the >> previously configured bits. >> >> Fixes: c14e64b46944 ("soc: qcom: llcc: Support chipsets that can write >> to llcc") >> Signed-off-by: Atul Dhudase <quic_adhudase@quicinc.com> >> --- >> drivers/soc/qcom/llcc-qcom.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c >> index 674abd0d6700..509d972c1bd9 100644 >> --- a/drivers/soc/qcom/llcc-qcom.c >> +++ b/drivers/soc/qcom/llcc-qcom.c >> @@ -941,15 +941,15 @@ static int _qcom_llcc_cfg_program(const struct >> llcc_slice_config *config, >> u32 disable_cap_alloc, retain_pc; >> >> disable_cap_alloc = config->dis_cap_alloc << config->slice_id; >> - ret = regmap_write(drv_data->bcast_regmap, >> - LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc); >> + ret = regmap_update_bits(drv_data->bcast_regmap, >> LLCC_TRP_SCID_DIS_CAP_ALLOC, >> + BIT(config->slice_id), disable_cap_alloc); >> if (ret) >> return ret; >> >> if (drv_data->version < LLCC_VERSION_4_1_0_0) { >> retain_pc = config->retain_on_pc << config->slice_id; >> - ret = regmap_write(drv_data->bcast_regmap, >> - LLCC_TRP_PCB_ACT, retain_pc); >> + ret = regmap_update_bits(drv_data->bcast_regmap, >> LLCC_TRP_PCB_ACT, >> + BIT(config->slice_id), retain_pc); > > Good catch, LGTM Forgot to tell., Please tag this to stable as well. Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com> -Mukesh > > Thanks > -Mukesh > >> if (ret) >> return ret; >> } >> -- >> 2.25.1 >>
On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: > While programming dis_cap_alloc and retain_on_pc, set a bit > corresponding to a specific SCID without disturbing the > previously configured bits. > As far as I can see, the only invocation of _qcom_llcc_cfg_program() comes from qcom_llcc_cfg_program(), which is only called once, from qcom_llcc_probe(), and here also seems to only be the single write to these two registers. This implies that "the previously configured bits" would be some unknown configuration provided to us either from the bootloader or by reset of the hardware. As such this changes the value of the two registers from being known, to having 31 unknown bits. I'm not saying that the change is wrong, but you're altering the behavior of every platform except SDM845. As such, I want the commit message to provide an actual problem description, and mention the fact that you're changing the logic to retain the state prior to Linux. Regards, Bjorn > Fixes: c14e64b46944 ("soc: qcom: llcc: Support chipsets that can write to llcc") > Signed-off-by: Atul Dhudase <quic_adhudase@quicinc.com> > --- > drivers/soc/qcom/llcc-qcom.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c > index 674abd0d6700..509d972c1bd9 100644 > --- a/drivers/soc/qcom/llcc-qcom.c > +++ b/drivers/soc/qcom/llcc-qcom.c > @@ -941,15 +941,15 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config, > u32 disable_cap_alloc, retain_pc; > > disable_cap_alloc = config->dis_cap_alloc << config->slice_id; > - ret = regmap_write(drv_data->bcast_regmap, > - LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc); > + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_SCID_DIS_CAP_ALLOC, > + BIT(config->slice_id), disable_cap_alloc); > if (ret) > return ret; > > if (drv_data->version < LLCC_VERSION_4_1_0_0) { > retain_pc = config->retain_on_pc << config->slice_id; > - ret = regmap_write(drv_data->bcast_regmap, > - LLCC_TRP_PCB_ACT, retain_pc); > + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_PCB_ACT, > + BIT(config->slice_id), retain_pc); > if (ret) > return ret; > } > -- > 2.25.1 >
On 11/4/2023 1:03 AM, Bjorn Andersson wrote: > On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: >> While programming dis_cap_alloc and retain_on_pc, set a bit >> corresponding to a specific SCID without disturbing the >> previously configured bits. >> > > As far as I can see, the only invocation of _qcom_llcc_cfg_program() > comes from qcom_llcc_cfg_program(), which is only called once, from > qcom_llcc_probe(), and here also seems to only be the single write to > these two registers. It does not look to be single write but the write is for each slice in the same register which was overriding other slices values. -Mukesh > > This implies that "the previously configured bits" would be some unknown > configuration provided to us either from the bootloader or by reset of > the hardware. As such this changes the value of the two registers from > being known, to having 31 unknown bits. > > > I'm not saying that the change is wrong, but you're altering the > behavior of every platform except SDM845. > As such, I want the commit message to provide an actual problem > description, and mention the fact that you're changing the logic to > retain the state prior to Linux. > > Regards, > Bjorn > >> Fixes: c14e64b46944 ("soc: qcom: llcc: Support chipsets that can write to llcc") >> Signed-off-by: Atul Dhudase <quic_adhudase@quicinc.com> >> --- >> drivers/soc/qcom/llcc-qcom.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c >> index 674abd0d6700..509d972c1bd9 100644 >> --- a/drivers/soc/qcom/llcc-qcom.c >> +++ b/drivers/soc/qcom/llcc-qcom.c >> @@ -941,15 +941,15 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config, >> u32 disable_cap_alloc, retain_pc; >> >> disable_cap_alloc = config->dis_cap_alloc << config->slice_id; >> - ret = regmap_write(drv_data->bcast_regmap, >> - LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc); >> + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_SCID_DIS_CAP_ALLOC, >> + BIT(config->slice_id), disable_cap_alloc); >> if (ret) >> return ret; >> >> if (drv_data->version < LLCC_VERSION_4_1_0_0) { >> retain_pc = config->retain_on_pc << config->slice_id; >> - ret = regmap_write(drv_data->bcast_regmap, >> - LLCC_TRP_PCB_ACT, retain_pc); >> + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_PCB_ACT, >> + BIT(config->slice_id), retain_pc); >> if (ret) >> return ret; >> } >> -- >> 2.25.1 >>
Quoting Mukesh Ojha (2023-11-05 22:54:28) > > > On 11/4/2023 1:03 AM, Bjorn Andersson wrote: > > On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: > >> While programming dis_cap_alloc and retain_on_pc, set a bit > >> corresponding to a specific SCID without disturbing the > >> previously configured bits. > >> > > > > As far as I can see, the only invocation of _qcom_llcc_cfg_program() > > comes from qcom_llcc_cfg_program(), which is only called once, from > > qcom_llcc_probe(), and here also seems to only be the single write to > > these two registers. > > It does not look to be single write but the write is for each slice > in the same register which was overriding other slices values. Can you add that detail to the commit text? What's the seriousness of the issue? Why should it be backported to stable? Is something seriously broken because a slice configuration is overwritten? Does it mean that some allocation made in a slice is being lost over power collapse (pc) when it shouldn't be?
On Mon, Nov 06, 2023 at 12:24:28PM +0530, Mukesh Ojha wrote: > > > On 11/4/2023 1:03 AM, Bjorn Andersson wrote: > > On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: > > > While programming dis_cap_alloc and retain_on_pc, set a bit > > > corresponding to a specific SCID without disturbing the > > > previously configured bits. > > > > > > > As far as I can see, the only invocation of _qcom_llcc_cfg_program() > > comes from qcom_llcc_cfg_program(), which is only called once, from > > qcom_llcc_probe(), and here also seems to only be the single write to > > these two registers. > > It does not look to be single write but the write is for each slice > in the same register which was overriding other slices values. > Ahh, you're right. I missed that it was hitting the same register in each iteration. My concern remains though, unless a platform defines 32 slices only a subset of the bits in the two registers will be touched - leaving the remaining bits in whatever state they happened to have. Regards, Bjorn > -Mukesh > > > > This implies that "the previously configured bits" would be some unknown > > configuration provided to us either from the bootloader or by reset of > > the hardware. As such this changes the value of the two registers from > > being known, to having 31 unknown bits. > > > > > > I'm not saying that the change is wrong, but you're altering the > > behavior of every platform except SDM845. > > As such, I want the commit message to provide an actual problem > > description, and mention the fact that you're changing the logic to > > retain the state prior to Linux. > > > > Regards, > > Bjorn > > > > > Fixes: c14e64b46944 ("soc: qcom: llcc: Support chipsets that can write to llcc") > > > Signed-off-by: Atul Dhudase <quic_adhudase@quicinc.com> > > > --- > > > drivers/soc/qcom/llcc-qcom.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c > > > index 674abd0d6700..509d972c1bd9 100644 > > > --- a/drivers/soc/qcom/llcc-qcom.c > > > +++ b/drivers/soc/qcom/llcc-qcom.c > > > @@ -941,15 +941,15 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config, > > > u32 disable_cap_alloc, retain_pc; > > > > > > disable_cap_alloc = config->dis_cap_alloc << config->slice_id; > > > - ret = regmap_write(drv_data->bcast_regmap, > > > - LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc); > > > + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_SCID_DIS_CAP_ALLOC, > > > + BIT(config->slice_id), disable_cap_alloc); > > > if (ret) > > > return ret; > > > > > > if (drv_data->version < LLCC_VERSION_4_1_0_0) { > > > retain_pc = config->retain_on_pc << config->slice_id; > > > - ret = regmap_write(drv_data->bcast_regmap, > > > - LLCC_TRP_PCB_ACT, retain_pc); > > > + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_PCB_ACT, > > > + BIT(config->slice_id), retain_pc); > > > if (ret) > > > return ret; > > > } > > > -- > > > 2.25.1 > > >
On 11/7/2023 3:25 AM, Stephen Boyd wrote: > Quoting Mukesh Ojha (2023-11-05 22:54:28) >> >> >> On 11/4/2023 1:03 AM, Bjorn Andersson wrote: >>> On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: >>>> While programming dis_cap_alloc and retain_on_pc, set a bit >>>> corresponding to a specific SCID without disturbing the >>>> previously configured bits. >>>> >>> >>> As far as I can see, the only invocation of _qcom_llcc_cfg_program() >>> comes from qcom_llcc_cfg_program(), which is only called once, from >>> qcom_llcc_probe(), and here also seems to only be the single write to >>> these two registers. >> >> It does not look to be single write but the write is for each slice >> in the same register which was overriding other slices values. > > Can you add that detail to the commit text? What's the seriousness of > the issue? Why should it be backported to stable? Is something seriously > broken because a slice configuration is overwritten? Does it mean that > some allocation made in a slice is being lost over power collapse (pc) > when it shouldn't be? @Atul will update the commit text as per suggestion. And yes, without this change, retention feature will not work properly. -Mukesh
Hi, On 11/7/2023 6:46 PM, Mukesh Ojha wrote: > On 11/7/2023 3:25 AM, Stephen Boyd wrote: > > Quoting Mukesh Ojha (2023-11-05 22:54:28) > >> > >> > >> On 11/4/2023 1:03 AM, Bjorn Andersson wrote: > >>> On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: > >>>> While programming dis_cap_alloc and retain_on_pc, set a bit > >>>> corresponding to a specific SCID without disturbing the > >>>> previously configured bits. > >>>> > >>> > >>> As far as I can see, the only invocation of > >>> _qcom_llcc_cfg_program() comes from qcom_llcc_cfg_program(), which > >>> is only called once, from qcom_llcc_probe(), and here also seems > >>> to only be the single write to these two registers. > >> > >> It does not look to be single write but the write is for each slice > >> in the same register which was overriding other slices values. > > > > Can you add that detail to the commit text? What's the seriousness > > of the issue? Why should it be backported to stable? Is something > > seriously broken because a slice configuration is overwritten? Does > > it mean that some allocation made in a slice is being lost over > > power collapse (pc) when it shouldn't be? > > @Atul will update the commit text as per suggestion. > > And yes, without this change, retention feature will not work properly. > > -Mukesh Does this commit text make sense? If so, I will update patch accordingly. When configuring capacity based allocation and power collapse retention, writing to the same register for each slice caused overwriting of the values for other slices, leading to misconfiguration for majority of the slices. To address this, only modify the bits associated with each slice while retaining the values of the remaining bits, as they were prior to the Linux configuration. Thanks, Atul
Quoting Atul Dhudase (QUIC) (2023-11-07 07:27:54) > Hi, > > On 11/7/2023 6:46 PM, Mukesh Ojha wrote: > > On 11/7/2023 3:25 AM, Stephen Boyd wrote: > > > Quoting Mukesh Ojha (2023-11-05 22:54:28) > > >> > > >> > > >> On 11/4/2023 1:03 AM, Bjorn Andersson wrote: > > >>> On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: > > >>>> While programming dis_cap_alloc and retain_on_pc, set a bit > > >>>> corresponding to a specific SCID without disturbing the > > >>>> previously configured bits. > > >>>> > > >>> > > >>> As far as I can see, the only invocation of > > >>> _qcom_llcc_cfg_program() comes from qcom_llcc_cfg_program(), which > > >>> is only called once, from qcom_llcc_probe(), and here also seems > > >>> to only be the single write to these two registers. > > >> > > >> It does not look to be single write but the write is for each slice > > >> in the same register which was overriding other slices values. > > > > > > Can you add that detail to the commit text? What's the seriousness > > > of the issue? Why should it be backported to stable? Is something > > > seriously broken because a slice configuration is overwritten? Does > > > it mean that some allocation made in a slice is being lost over > > > power collapse (pc) when it shouldn't be? > > > > @Atul will update the commit text as per suggestion. > > > > And yes, without this change, retention feature will not work properly. > > > > -Mukesh > > Does this commit text make sense? If so, I will update patch accordingly. > > When configuring capacity based allocation and power collapse retention, writing to the same register for each slice caused overwriting of the values for other slices, leading to misconfiguration for majority of the slices. > To address this, only modify the bits associated with each slice while retaining the values of the remaining bits, as they were prior to the Linux configuration. This commit text doesn't say what, if anything, is broken. Does it save power when it didn't before? Why is it critical to backport this to stable kernels? Was the driver overwriting other bits in this register that were critical to power, performance, or correctness? Or was everything working out because the last slice to be written was the only important one?
Hi, On Thu, Nov 9, 2023 at 12:34 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Atul Dhudase (QUIC) (2023-11-07 07:27:54) > > Hi, > > > > On 11/7/2023 6:46 PM, Mukesh Ojha wrote: > > > On 11/7/2023 3:25 AM, Stephen Boyd wrote: > > > > Quoting Mukesh Ojha (2023-11-05 22:54:28) > > > >> > > > >> > > > >> On 11/4/2023 1:03 AM, Bjorn Andersson wrote: > > > >>> On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: > > > >>>> While programming dis_cap_alloc and retain_on_pc, set a bit > > > >>>> corresponding to a specific SCID without disturbing the > > > >>>> previously configured bits. > > > >>>> > > > >>> > > > >>> As far as I can see, the only invocation of > > > >>> _qcom_llcc_cfg_program() comes from qcom_llcc_cfg_program(), which > > > >>> is only called once, from qcom_llcc_probe(), and here also seems > > > >>> to only be the single write to these two registers. > > > >> > > > >> It does not look to be single write but the write is for each slice > > > >> in the same register which was overriding other slices values. > > > > > > > > Can you add that detail to the commit text? What's the seriousness > > > > of the issue? Why should it be backported to stable? Is something > > > > seriously broken because a slice configuration is overwritten? Does > > > > it mean that some allocation made in a slice is being lost over > > > > power collapse (pc) when it shouldn't be? > > > > > > @Atul will update the commit text as per suggestion. > > > > > > And yes, without this change, retention feature will not work properly. > > > > > > -Mukesh > > > > Does this commit text make sense? If so, I will update patch accordingly. > > > > When configuring capacity based allocation and power collapse retention, writing to the same register for each slice caused overwriting of the values for other slices, leading to misconfiguration for majority of the slices. > > To address this, only modify the bits associated with each slice while retaining the values of the remaining bits, as they were prior to the Linux configuration. > > This commit text doesn't say what, if anything, is broken. Does it save > power when it didn't before? Why is it critical to backport this to > stable kernels? Was the driver overwriting other bits in this register > that were critical to power, performance, or correctness? Or was > everything working out because the last slice to be written was the only > important one? Whatever happened to this patch? It seemed like it might be important and it never landed. I guess the only thing that was blocking it from landing was some commit text that explained _why_ it was important and that never got written. I guess Bjorn was also worried that any bits that weren't updated by the kernel would now be left in their default state (however the bootloader left them). It would be good to indicate in the commit text if that matters and what is in those other bits. -Doug
On 12/6/2023 3:34 AM, Doug Anderson wrote: > Hi, > > On Thu, Nov 9, 2023 at 12:34 PM Stephen Boyd <swboyd@chromium.org> wrote: >> >> Quoting Atul Dhudase (QUIC) (2023-11-07 07:27:54) >>> Hi, >>> >>> On 11/7/2023 6:46 PM, Mukesh Ojha wrote: >>>> On 11/7/2023 3:25 AM, Stephen Boyd wrote: >>>>> Quoting Mukesh Ojha (2023-11-05 22:54:28) >>>>>> >>>>>> >>>>>> On 11/4/2023 1:03 AM, Bjorn Andersson wrote: >>>>>>> On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote: >>>>>>>> While programming dis_cap_alloc and retain_on_pc, set a bit >>>>>>>> corresponding to a specific SCID without disturbing the >>>>>>>> previously configured bits. >>>>>>>> >>>>>>> >>>>>>> As far as I can see, the only invocation of >>>>>>> _qcom_llcc_cfg_program() comes from qcom_llcc_cfg_program(), which >>>>>>> is only called once, from qcom_llcc_probe(), and here also seems >>>>>>> to only be the single write to these two registers. >>>>>> >>>>>> It does not look to be single write but the write is for each slice >>>>>> in the same register which was overriding other slices values. >>>>> >>>>> Can you add that detail to the commit text? What's the seriousness >>>>> of the issue? Why should it be backported to stable? Is something >>>>> seriously broken because a slice configuration is overwritten? Does >>>>> it mean that some allocation made in a slice is being lost over >>>>> power collapse (pc) when it shouldn't be? >>>> >>>> @Atul will update the commit text as per suggestion. >>>> >>>> And yes, without this change, retention feature will not work properly. >>>> >>>> -Mukesh >>> >>> Does this commit text make sense? If so, I will update patch accordingly. >>> >>> When configuring capacity based allocation and power collapse retention, writing to the same register for each slice caused overwriting of the values for other slices, leading to misconfiguration for majority of the slices. >>> To address this, only modify the bits associated with each slice while retaining the values of the remaining bits, as they were prior to the Linux configuration. >> >> This commit text doesn't say what, if anything, is broken. Does it save >> power when it didn't before? Why is it critical to backport this to >> stable kernels? Was the driver overwriting other bits in this register >> that were critical to power, performance, or correctness? Or was >> everything working out because the last slice to be written was the only >> important one? > > Whatever happened to this patch? It seemed like it might be important > and it never landed. I guess the only thing that was blocking it from > landing was some commit text that explained _why_ it was important and > that never got written. > > I guess Bjorn was also worried that any bits that weren't updated by > the kernel would now be left in their default state (however the > bootloader left them). It would be good to indicate in the commit text > if that matters and what is in those other bits. Let me add that. Completely missed to track this change., Thanks for reminding.. -Mukesh > > > -Doug
diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c index 674abd0d6700..509d972c1bd9 100644 --- a/drivers/soc/qcom/llcc-qcom.c +++ b/drivers/soc/qcom/llcc-qcom.c @@ -941,15 +941,15 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config, u32 disable_cap_alloc, retain_pc; disable_cap_alloc = config->dis_cap_alloc << config->slice_id; - ret = regmap_write(drv_data->bcast_regmap, - LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc); + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_SCID_DIS_CAP_ALLOC, + BIT(config->slice_id), disable_cap_alloc); if (ret) return ret; if (drv_data->version < LLCC_VERSION_4_1_0_0) { retain_pc = config->retain_on_pc << config->slice_id; - ret = regmap_write(drv_data->bcast_regmap, - LLCC_TRP_PCB_ACT, retain_pc); + ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_PCB_ACT, + BIT(config->slice_id), retain_pc); if (ret) return ret; }
While programming dis_cap_alloc and retain_on_pc, set a bit corresponding to a specific SCID without disturbing the previously configured bits. Fixes: c14e64b46944 ("soc: qcom: llcc: Support chipsets that can write to llcc") Signed-off-by: Atul Dhudase <quic_adhudase@quicinc.com> --- drivers/soc/qcom/llcc-qcom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) -- 2.25.1