1. Patchew
  2. linux
  3. View series

[PATCH] soc: qcom: pd_mapper: fix ADSP PD maps

Dmitry Baryshkov posted 1 patch 2 months, 1 week ago 
drivers/soc/qcom/qcom_pd_mapper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] soc: qcom: pd_mapper: fix ADSP PD maps
Posted by Dmitry Baryshkov 2 months, 1 week ago 
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(-)

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,

---
base-commit: 32ffa5373540a8d1c06619f52d019c6cdc948bb4
change-id: 20240918-x1e-fix-pdm-pdr-b7c4d978aaf3

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Re: [PATCH] soc: qcom: pd_mapper: fix ADSP PD maps
Posted by Johan Hovold 2 months, 1 week ago 
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
Re: [PATCH] soc: qcom: pd_mapper: fix ADSP PD maps
Posted by Dmitry Baryshkov 2 months, 1 week ago 
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.

-- 
With best wishes
Dmitry
Re: [PATCH] soc: qcom: pd_mapper: fix ADSP PD maps
Posted by Johan Hovold 2 months, 1 week ago 
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
Re: [PATCH] soc: qcom: pd_mapper: fix ADSP PD maps
Posted by Chris Lew 2 months, 1 week ago 

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
Re: [PATCH] soc: qcom: pd_mapper: fix ADSP PD maps
Posted by Dmitry Baryshkov 2 months, 1 week ago 
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

-- 
With best wishes
Dmitry
Re: [PATCH] soc: qcom: pd_mapper: fix ADSP PD maps
Posted by Bjorn Andersson 2 months ago 
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
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.