| Message ID | 20240918-x1e-fix-pdm-pdr-v1-1-cefc79bb33d1@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | soc: qcom: pd_mapper: fix ADSP PD maps | expand |
On Wed, Sep 18, 2024 at 04:02:39PM +0300, Dmitry Baryshkov wrote: > On X1E8 devices root ADSP domain should have tms/pdr_enabled registered. > Change the PDM domain data that is used for X1E80100 ADSP. Please expand the commit message so that it explains why this is needed and not just describes what the patch does. What is the expected impact of this and is there any chance that this is related to some of the in-kernel pd-mapper regression I've reported (e.g. audio not being registered and failing with a PDR error)? https://lore.kernel.org/all/ZthVTC8dt1kSdjMb@hovoldconsulting.com/ > Fixes: bd6db1f1486e ("soc: qcom: pd_mapper: Add X1E80100") > Cc: stable@vger.kernel.org Since the offending commit has not reached mainline yet, there's no need for a stable tag. Johan
On Fri, Sep 20, 2024 at 10:21:03AM GMT, Johan Hovold wrote: > On Wed, Sep 18, 2024 at 04:02:39PM +0300, Dmitry Baryshkov wrote: > > On X1E8 devices root ADSP domain should have tms/pdr_enabled registered. > > Change the PDM domain data that is used for X1E80100 ADSP. > > Please expand the commit message so that it explains why this is > needed and not just describes what the patch does. Unfortunately in this case I have no idea. It marks the domain as restartable (?), this is what json files for CRD and T14s do. Maybe Chris can comment more. > What is the expected impact of this and is there any chance that this is > related to some of the in-kernel pd-mapper regression I've reported > (e.g. audio not being registered and failing with a PDR error)? > > https://lore.kernel.org/all/ZthVTC8dt1kSdjMb@hovoldconsulting.com/ Still debugging this, sidetracked by OSS / LPC. > > > Fixes: bd6db1f1486e ("soc: qcom: pd_mapper: Add X1E80100") > > Cc: stable@vger.kernel.org > > Since the offending commit has not reached mainline yet, there's no need > for a stable tag. Ack, nice.
On Fri, Sep 20, 2024 at 11:49:46AM +0300, Dmitry Baryshkov wrote: > On Fri, Sep 20, 2024 at 10:21:03AM GMT, Johan Hovold wrote: > > On Wed, Sep 18, 2024 at 04:02:39PM +0300, Dmitry Baryshkov wrote: > > > On X1E8 devices root ADSP domain should have tms/pdr_enabled registered. > > > Change the PDM domain data that is used for X1E80100 ADSP. > > > > Please expand the commit message so that it explains why this is > > needed and not just describes what the patch does. > > Unfortunately in this case I have no idea. It marks the domain as > restartable (?), this is what json files for CRD and T14s do. Maybe > Chris can comment more. Chris, could you help sort out if and why this change is needed? https://lore.kernel.org/all/20240918-x1e-fix-pdm-pdr-v1-1-cefc79bb33d1@linaro.org/ > > What is the expected impact of this and is there any chance that this is > > related to some of the in-kernel pd-mapper regression I've reported > > (e.g. audio not being registered and failing with a PDR error)? > > > > https://lore.kernel.org/all/ZthVTC8dt1kSdjMb@hovoldconsulting.com/ > > Still debugging this, sidetracked by OSS / LPC. Johan
On 9/20/2024 2:02 AM, Johan Hovold wrote: > On Fri, Sep 20, 2024 at 11:49:46AM +0300, Dmitry Baryshkov wrote: >> On Fri, Sep 20, 2024 at 10:21:03AM GMT, Johan Hovold wrote: >>> On Wed, Sep 18, 2024 at 04:02:39PM +0300, Dmitry Baryshkov wrote: >>>> On X1E8 devices root ADSP domain should have tms/pdr_enabled registered. >>>> Change the PDM domain data that is used for X1E80100 ADSP. >>> >>> Please expand the commit message so that it explains why this is >>> needed and not just describes what the patch does. >> >> Unfortunately in this case I have no idea. It marks the domain as >> restartable (?), this is what json files for CRD and T14s do. Maybe >> Chris can comment more. > > Chris, could you help sort out if and why this change is needed? > > https://lore.kernel.org/all/20240918-x1e-fix-pdm-pdr-v1-1-cefc79bb33d1@linaro.org/ > I don't think this change would help with the issue reported by Johan. From a quick glance, I couldn't find where exactly the restartable attribute is used, but this type of change would only matter when the ChargerPD is started or restarted. The PMIC_GLINK channel probing in rpmsg is dependent on ChargerPD starting, so we know ChargerPD can start with or without this change. I can give this change a try next week to help give a better analysis. >>> What is the expected impact of this and is there any chance that this is >>> related to some of the in-kernel pd-mapper regression I've reported >>> (e.g. audio not being registered and failing with a PDR error)? >>> >>> https://lore.kernel.org/all/ZthVTC8dt1kSdjMb@hovoldconsulting.com/ >> >> Still debugging this, sidetracked by OSS / LPC. > > Johan
On Fri, Sep 20, 2024 at 07:00:11AM GMT, Chris Lew wrote: > > > On 9/20/2024 2:02 AM, Johan Hovold wrote: > > On Fri, Sep 20, 2024 at 11:49:46AM +0300, Dmitry Baryshkov wrote: > > > On Fri, Sep 20, 2024 at 10:21:03AM GMT, Johan Hovold wrote: > > > > On Wed, Sep 18, 2024 at 04:02:39PM +0300, Dmitry Baryshkov wrote: > > > > > On X1E8 devices root ADSP domain should have tms/pdr_enabled registered. > > > > > Change the PDM domain data that is used for X1E80100 ADSP. > > > > > > > > Please expand the commit message so that it explains why this is > > > > needed and not just describes what the patch does. > > > > > > Unfortunately in this case I have no idea. It marks the domain as > > > restartable (?), this is what json files for CRD and T14s do. Maybe > > > Chris can comment more. > > > > Chris, could you help sort out if and why this change is needed? > > > > https://lore.kernel.org/all/20240918-x1e-fix-pdm-pdr-v1-1-cefc79bb33d1@linaro.org/ > > > > I don't think this change would help with the issue reported by Johan. From > a quick glance, I couldn't find where exactly the restartable attribute is > used, but this type of change would only matter when the ChargerPD is > started or restarted. This raises a question: should we care at all about the pdr_enabled? Is it fine to drop it fromm all PD maps? > > The PMIC_GLINK channel probing in rpmsg is dependent on ChargerPD starting, > so we know ChargerPD can start with or without this change. > > I can give this change a try next week to help give a better analysis. > > > > > What is the expected impact of this and is there any chance that this is > > > > related to some of the in-kernel pd-mapper regression I've reported > > > > (e.g. audio not being registered and failing with a PDR error)? > > > > > > > > https://lore.kernel.org/all/ZthVTC8dt1kSdjMb@hovoldconsulting.com/ > > > > > > Still debugging this, sidetracked by OSS / LPC. > > > > Johan
On Fri, Sep 20, 2024 at 05:07:13PM +0300, Dmitry Baryshkov wrote: > On Fri, Sep 20, 2024 at 07:00:11AM GMT, Chris Lew wrote: > > > > > > On 9/20/2024 2:02 AM, Johan Hovold wrote: > > > On Fri, Sep 20, 2024 at 11:49:46AM +0300, Dmitry Baryshkov wrote: > > > > On Fri, Sep 20, 2024 at 10:21:03AM GMT, Johan Hovold wrote: > > > > > On Wed, Sep 18, 2024 at 04:02:39PM +0300, Dmitry Baryshkov wrote: > > > > > > On X1E8 devices root ADSP domain should have tms/pdr_enabled registered. > > > > > > Change the PDM domain data that is used for X1E80100 ADSP. > > > > > > > > > > Please expand the commit message so that it explains why this is > > > > > needed and not just describes what the patch does. > > > > > > > > Unfortunately in this case I have no idea. It marks the domain as > > > > restartable (?), this is what json files for CRD and T14s do. Maybe > > > > Chris can comment more. > > > > > > Chris, could you help sort out if and why this change is needed? > > > > > > https://lore.kernel.org/all/20240918-x1e-fix-pdm-pdr-v1-1-cefc79bb33d1@linaro.org/ > > > > > > > I don't think this change would help with the issue reported by Johan. From > > a quick glance, I couldn't find where exactly the restartable attribute is > > used, but this type of change would only matter when the ChargerPD is > > started or restarted. > > This raises a question: should we care at all about the pdr_enabled? Is > it fine to drop it fromm all PD maps? > There's definitely benefits to pdr_enabled. I'd expect you could have examples such as audio firmware restarting without USB Type-C being reset. So, the appropriate path forward would be to figure out how we can properly test the various levels of restarts in a continuous fashion and make sure it's enabled where it can be... Regards, Bjorn > > > > The PMIC_GLINK channel probing in rpmsg is dependent on ChargerPD starting, > > so we know ChargerPD can start with or without this change. > > > > I can give this change a try next week to help give a better analysis. > > > > > > > What is the expected impact of this and is there any chance that this is > > > > > related to some of the in-kernel pd-mapper regression I've reported > > > > > (e.g. audio not being registered and failing with a PDR error)? > > > > > > > > > > https://lore.kernel.org/all/ZthVTC8dt1kSdjMb@hovoldconsulting.com/ > > > > > > > > Still debugging this, sidetracked by OSS / LPC. > > > > > > Johan > > -- > With best wishes > Dmitry > >
diff --git a/drivers/soc/qcom/qcom_pd_mapper.c b/drivers/soc/qcom/qcom_pd_mapper.c index c940f4da28ed..9d33a8c71778 100644 --- a/drivers/soc/qcom/qcom_pd_mapper.c +++ b/drivers/soc/qcom/qcom_pd_mapper.c @@ -519,7 +519,7 @@ static const struct qcom_pdm_domain_data *sm8550_domains[] = { static const struct qcom_pdm_domain_data *x1e80100_domains[] = { &adsp_audio_pd, - &adsp_root_pd, + &adsp_root_pd_pdr, &adsp_charger_pd, &adsp_sensor_pd, &cdsp_root_pd,
On X1E8 devices root ADSP domain should have tms/pdr_enabled registered. Change the PDM domain data that is used for X1E80100 ADSP. Fixes: bd6db1f1486e ("soc: qcom: pd_mapper: Add X1E80100") Cc: stable@vger.kernel.org Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/soc/qcom/qcom_pd_mapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 32ffa5373540a8d1c06619f52d019c6cdc948bb4 change-id: 20240918-x1e-fix-pdm-pdr-b7c4d978aaf3 Best regards,