| Message ID | 1722335443-30080-1-git-send-email-quic_zhenhuah@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver | expand |
Gentle reminder for review :) On 2024/7/30 18:30, Zhenhua Huang wrote: > TBU driver has no runtime pm support now, adding pm_runtime_enable() > seems to be useless. Remove it. > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > --- > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index 36c6b36ad4ff..aff2fe1fda13 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = { > > static int qcom_smmu_tbu_probe(struct platform_device *pdev) > { > - struct device *dev = &pdev->dev; > int ret; > > if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) { > @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev) > return ret; > } > > - if (dev->pm_domain) { > - pm_runtime_set_active(dev); > - pm_runtime_enable(dev); > - } > - > return 0; > } >
On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote: > TBU driver has no runtime pm support now, adding pm_runtime_enable() > seems to be useless. Remove it. > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > --- > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index 36c6b36ad4ff..aff2fe1fda13 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = { > > static int qcom_smmu_tbu_probe(struct platform_device *pdev) > { > - struct device *dev = &pdev->dev; > int ret; > > if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) { > @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev) > return ret; > } > > - if (dev->pm_domain) { > - pm_runtime_set_active(dev); > - pm_runtime_enable(dev); I assumed that this was required to avoid the TBU from being powered down? If so, then I think we shall move it under the previous if condition, i.e. CONFIG_ARM_SMMU_QCOM_DEBUG? If not, we can remove it give that the TBU would be powered ON as needed > - } > - > return 0; > } > > -- > 2.7.4 > > Thanks, Pranjal
On 2024/8/12 21:25, Pranjal Shrivastava wrote: > On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote: >> TBU driver has no runtime pm support now, adding pm_runtime_enable() >> seems to be useless. Remove it. >> >> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> >> --- >> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> index 36c6b36ad4ff..aff2fe1fda13 100644 >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = { >> >> static int qcom_smmu_tbu_probe(struct platform_device *pdev) >> { >> - struct device *dev = &pdev->dev; >> int ret; >> >> if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) { >> @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev) >> return ret; >> } >> >> - if (dev->pm_domain) { >> - pm_runtime_set_active(dev); >> - pm_runtime_enable(dev); > > I assumed that this was required to avoid the TBU from being powered > down? If so, then I think we shall move it under the Hi Pranjal, In my sense, this was giving the TBU ability to power down when necessary(through pm callbacks)? While I haven't seen any RPM impl for TBU device.. hence having the doubt.. Thanks, Zhenhua > previous if condition, i.e. CONFIG_ARM_SMMU_QCOM_DEBUG? > > If not, we can remove it give that the TBU would be powered ON as needed > >> - } >> - >> return 0; >> } >> >> -- >> 2.7.4 >> >> > > Thanks, > Pranjal
On Tue, Aug 13, 2024 at 10:37:33AM +0800, Zhenhua Huang wrote: > > > On 2024/8/12 21:25, Pranjal Shrivastava wrote: > > On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote: > > > TBU driver has no runtime pm support now, adding pm_runtime_enable() > > > seems to be useless. Remove it. > > > > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > > --- > > > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------ > > > 1 file changed, 6 deletions(-) > > > > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > > index 36c6b36ad4ff..aff2fe1fda13 100644 > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > > @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = { > > > static int qcom_smmu_tbu_probe(struct platform_device *pdev) > > > { > > > - struct device *dev = &pdev->dev; > > > int ret; > > > if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) { > > > @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev) > > > return ret; > > > } > > > - if (dev->pm_domain) { > > > - pm_runtime_set_active(dev); > > > - pm_runtime_enable(dev); > > > > I assumed that this was required to avoid the TBU from being powered > > down? If so, then I think we shall move it under the > > Hi Pranjal, > > In my sense, this was giving the TBU ability to power down when > necessary(through pm callbacks)? While I haven't seen any RPM impl for TBU > device.. hence having the doubt.. > > Thanks, > Zhenhua Apologies for being unclear. I just meant to ask if there was a reason to add pm_runtime_set_active & enable in the tbu probe previously? And I *assumed* that it was to set the device state as RPM_ACTIVE to avoid it being RPM_SUSPENDED after enabling pm_runtime. I agree that there are no pm_runtime_suspend/resume calls within the TBU driver. I'm just trying to understand why was pm_runtime enabled here earlier (since it's not implemented) in order to ensure that removing it doesn't cause further troubles? I see Georgi added it as a part of https://lore.kernel.org/all/20240704010759.507798-1-quic_c_gdjako@quicinc.com/ But I'm unsure why was it required to fix that bug? > > > previous if condition, i.e. CONFIG_ARM_SMMU_QCOM_DEBUG? > > > > If not, we can remove it give that the TBU would be powered ON as needed > > > > > - } > > > - > > > return 0; > > > } > > > -- > > > 2.7.4 > > > > > > > > > > Thanks, > > Pranjal Thanks, Pranjal
On 2024/8/13 15:20, Pranjal Shrivastava wrote: > On Tue, Aug 13, 2024 at 10:37:33AM +0800, Zhenhua Huang wrote: >> >> >> On 2024/8/12 21:25, Pranjal Shrivastava wrote: >>> On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote: >>>> TBU driver has no runtime pm support now, adding pm_runtime_enable() >>>> seems to be useless. Remove it. >>>> >>>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> >>>> --- >>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------ >>>> 1 file changed, 6 deletions(-) >>>> >>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> index 36c6b36ad4ff..aff2fe1fda13 100644 >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = { >>>> static int qcom_smmu_tbu_probe(struct platform_device *pdev) >>>> { >>>> - struct device *dev = &pdev->dev; >>>> int ret; >>>> if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) { >>>> @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev) >>>> return ret; >>>> } >>>> - if (dev->pm_domain) { >>>> - pm_runtime_set_active(dev); >>>> - pm_runtime_enable(dev); >>> >>> I assumed that this was required to avoid the TBU from being powered >>> down? If so, then I think we shall move it under the >> >> Hi Pranjal, >> >> In my sense, this was giving the TBU ability to power down when >> necessary(through pm callbacks)? While I haven't seen any RPM impl for TBU >> device.. hence having the doubt.. >> >> Thanks, >> Zhenhua > > Apologies for being unclear. I just meant to ask if there was a reason > to add pm_runtime_set_active & enable in the tbu probe previously? And I It's also my doubt and hope Georgi can help clarify :) Actually I assume this part of codes was copied from arm-smmu driver https://elixir.bootlin.com/linux/v6.11-rc3/source/drivers/iommu/arm/arm-smmu/arm-smmu.c#L2264 .. but for TBU, seems no user ? > *assumed* that it was to set the device state as RPM_ACTIVE to avoid it > being RPM_SUSPENDED after enabling pm_runtime Yeah, it's normal sequence from doc: https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/power/runtime_pm.rst#L574 > > I agree that there are no pm_runtime_suspend/resume calls within the TBU > driver. I'm just trying to understand why was pm_runtime enabled here > earlier (since it's not implemented) in order to ensure that removing it > doesn't cause further troubles? See above my assumption, need Georgi to comment but. > > I see Georgi added it as a part of > https://lore.kernel.org/all/20240704010759.507798-1-quic_c_gdjako@quicinc.com/ > > But I'm unsure why was it required to fix that bug? I'm just thinking it is dead code and want to see if my understanding is correct. Thanks, Zhenhua > >> >>> previous if condition, i.e. CONFIG_ARM_SMMU_QCOM_DEBUG? >>> >>> If not, we can remove it give that the TBU would be powered ON as needed >>> >>>> - } >>>> - >>>> return 0; >>>> } >>>> -- >>>> 2.7.4 >>>> >>>> >>> >>> Thanks, >>> Pranjal > > Thanks, > Pranjal
Hi Zhenhua, On 8/13/2024 10:56 AM, Zhenhua Huang wrote: > > On 2024/8/13 15:20, Pranjal Shrivastava wrote: >> On Tue, Aug 13, 2024 at 10:37:33AM +0800, Zhenhua Huang wrote: >>> >>> On 2024/8/12 21:25, Pranjal Shrivastava wrote: >>>> On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote: >>>>> TBU driver has no runtime pm support now, adding pm_runtime_enable() >>>>> seems to be useless. Remove it. >>>>> >>>>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> [..] >> I agree that there are no pm_runtime_suspend/resume calls within the TBU >> driver. I'm just trying to understand why was pm_runtime enabled here >> earlier (since it's not implemented) in order to ensure that removing it >> doesn't cause further troubles? > > See above my assumption, need Georgi to comment but. Thank you for looking at the code! Your assumptions are mostly correct, but if you try this patch on a real sdm845 device you will notice some issues. So it's actually needed to re-configure the power-domains, three of which (MMNOC GDSCs) are requiring this because of a HW bug. I should have put a comment in the code to avoid confusion, but it took me some time to confirm it. I have sent a patch to handle this more cleanly: https://lore.kernel.org/lkml/20240813120015.3242787-1-quic_c_gdjako@quicinc.com So we should not remove the runtime pm calls until some version of the above patch gets merged. Thanks, Georgi >> I see Georgi added it as a part of >> https://lore.kernel.org/all/20240704010759.507798-1-quic_c_gdjako@quicinc.com/ >> >> But I'm unsure why was it required to fix that bug? > > I'm just thinking it is dead code and want to see if my understanding is correct.
Hi Georgi, On 2024/8/13 20:06, Georgi Djakov wrote: > Hi Zhenhua, > > On 8/13/2024 10:56 AM, Zhenhua Huang wrote: >> >> On 2024/8/13 15:20, Pranjal Shrivastava wrote: >>> On Tue, Aug 13, 2024 at 10:37:33AM +0800, Zhenhua Huang wrote: >>>> >>>> On 2024/8/12 21:25, Pranjal Shrivastava wrote: >>>>> On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote: >>>>>> TBU driver has no runtime pm support now, adding pm_runtime_enable() >>>>>> seems to be useless. Remove it. >>>>>> >>>>>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > [..] >>> I agree that there are no pm_runtime_suspend/resume calls within the TBU >>> driver. I'm just trying to understand why was pm_runtime enabled here >>> earlier (since it's not implemented) in order to ensure that removing it >>> doesn't cause further troubles? >> >> See above my assumption, need Georgi to comment but. > > Thank you for looking at the code! Your assumptions are mostly correct, > but if you try this patch on a real sdm845 device you will notice some > issues. So it's actually needed to re-configure the power-domains, three Thanks Georgi for your comments! Hmm... so you found some bugs on sdm845 ? sorry that I don't have sdm845 on hand... > of which (MMNOC GDSCs) are requiring this because of a HW bug. I should > have put a comment in the code to avoid confusion, but it took me some > time to confirm it. > > I have sent a patch to handle this more cleanly: > https://lore.kernel.org/lkml/20240813120015.3242787-1-quic_c_gdjako@quicinc.com > > So we should not remove the runtime pm calls until some version of the > above patch gets merged. In my sense, above patch should not result in turning off gdsc? It's just open the support for RPM.. I tried to do same change for arm-smmu driver, w/ test I see cx_gdsc which is the power-domain for gfx_smmu, is on: .. /sys/kernel/debug/pm_genpd/cx_gdsc # cat current_state on Are you worrying that not setting active will turn off related PD? or Could you please explain a bit more about how the change impacted power domain status? Thanks in advance :) > > Thanks, > Georgi > >>> I see Georgi added it as a part of >>> https://lore.kernel.org/all/20240704010759.507798-1-quic_c_gdjako@quicinc.com/ >>> >>> But I'm unsure why was it required to fix that bug? >> >> I'm just thinking it is dead code and want to see if my understanding is correct. > >
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c index 36c6b36ad4ff..aff2fe1fda13 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = { static int qcom_smmu_tbu_probe(struct platform_device *pdev) { - struct device *dev = &pdev->dev; int ret; if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) { @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev) return ret; } - if (dev->pm_domain) { - pm_runtime_set_active(dev); - pm_runtime_enable(dev); - } - return 0; }
TBU driver has no runtime pm support now, adding pm_runtime_enable() seems to be useless. Remove it. Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> --- drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------ 1 file changed, 6 deletions(-)