Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
On Tue, Oct 2, 2018 at 9:44 AM Vivek Gautam wrote:
>
> Hi Will,
>
> On Mon, Oct 1, 2018 at 6:29 PM Will Deacon wrote:
> >
> > Hi Vivek,
> >
> > On Thu, Aug 30, 2018 at 08:15:38PM +0530, Vivek Gautam wrote:
> > > From: Sricharan R
> > >
> > > The smmu device probe/remove and add/remove master device callbacks
> > > gets called when the smmu is not linked to its master, that is without
> > > the context of the master device. So calling runtime apis in those places
> > > separately.
> > > Global locks are also initialized before enabling runtime pm as the
> > > runtime_resume() calls device_reset() which does tlb_sync_global()
> > > that ultimately requires locks to be initialized.
> > >
> > > Signed-off-by: Sricharan R
> > > [vivek: Cleanup pm runtime calls]
> > > Signed-off-by: Vivek Gautam
> > > Reviewed-by: Tomasz Figa
> > > Tested-by: Srinivas Kandagatla
> > > ---
> > > drivers/iommu/arm-smmu.c | 89
> > > +++-
> > > 1 file changed, 81 insertions(+), 8 deletions(-)
> >
> > This doesn't apply on my tree[1], possibly because I've got Robin's
> > non-strict
> > invalidation queued there. However, that got me thinking -- how does this
> > work in conjunction with the timer-based TLB invalidation? Do we need to
> > rpm_{get,put} around flush_iotlb_all()? If so, do we still need the calls
> > in map/unmap when non-strict mode is in use?
For map/unmap(), i think there would be no harm in having additional
power.usage_count even for the non-strict mode.
So, I will just add rpm{get,put} in arm_smmu_flush_iotlb_all(), and
arm_smmu_iotlb_sync().
Regards
Vivek
>
> I haven't tested things with flush queues, but from what it looks like
> both .flush_iotlb_all, and .iotlb_sync callbacks need rpm_get/put().
> I will respin the patches.
>
> Thanks
> Vivek
> >
> > Will
> >
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-joerg/arm-smmu/updates
>
>
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
Hi Will,
On Mon, Oct 1, 2018 at 6:29 PM Will Deacon wrote:
>
> Hi Vivek,
>
> On Thu, Aug 30, 2018 at 08:15:38PM +0530, Vivek Gautam wrote:
> > From: Sricharan R
> >
> > The smmu device probe/remove and add/remove master device callbacks
> > gets called when the smmu is not linked to its master, that is without
> > the context of the master device. So calling runtime apis in those places
> > separately.
> > Global locks are also initialized before enabling runtime pm as the
> > runtime_resume() calls device_reset() which does tlb_sync_global()
> > that ultimately requires locks to be initialized.
> >
> > Signed-off-by: Sricharan R
> > [vivek: Cleanup pm runtime calls]
> > Signed-off-by: Vivek Gautam
> > Reviewed-by: Tomasz Figa
> > Tested-by: Srinivas Kandagatla
> > ---
> > drivers/iommu/arm-smmu.c | 89
> > +++-
> > 1 file changed, 81 insertions(+), 8 deletions(-)
>
> This doesn't apply on my tree[1], possibly because I've got Robin's non-strict
> invalidation queued there. However, that got me thinking -- how does this
> work in conjunction with the timer-based TLB invalidation? Do we need to
> rpm_{get,put} around flush_iotlb_all()? If so, do we still need the calls
> in map/unmap when non-strict mode is in use?
I haven't tested things with flush queues, but from what it looks like
both .flush_iotlb_all, and .iotlb_sync callbacks need rpm_get/put().
I will respin the patches.
Thanks
Vivek
>
> Will
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-joerg/arm-smmu/updates
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
Hi Vivek,
On Thu, Aug 30, 2018 at 08:15:38PM +0530, Vivek Gautam wrote:
> From: Sricharan R
>
> The smmu device probe/remove and add/remove master device callbacks
> gets called when the smmu is not linked to its master, that is without
> the context of the master device. So calling runtime apis in those places
> separately.
> Global locks are also initialized before enabling runtime pm as the
> runtime_resume() calls device_reset() which does tlb_sync_global()
> that ultimately requires locks to be initialized.
>
> Signed-off-by: Sricharan R
> [vivek: Cleanup pm runtime calls]
> Signed-off-by: Vivek Gautam
> Reviewed-by: Tomasz Figa
> Tested-by: Srinivas Kandagatla
> ---
> drivers/iommu/arm-smmu.c | 89
> +++-
> 1 file changed, 81 insertions(+), 8 deletions(-)
This doesn't apply on my tree[1], possibly because I've got Robin's non-strict
invalidation queued there. However, that got me thinking -- how does this
work in conjunction with the timer-based TLB invalidation? Do we need to
rpm_{get,put} around flush_iotlb_all()? If so, do we still need the calls
in map/unmap when non-strict mode is in use?
Will
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-joerg/arm-smmu/updates
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
On 30/08/18 15:45, Vivek Gautam wrote:
From: Sricharan R
The smmu device probe/remove and add/remove master device callbacks
gets called when the smmu is not linked to its master, that is without
the context of the master device. So calling runtime apis in those places
separately.
Global locks are also initialized before enabling runtime pm as the
runtime_resume() calls device_reset() which does tlb_sync_global()
that ultimately requires locks to be initialized.
To the best of my knowledge in this stuff (which is still not quite
enough to be *truly* confident...),
Reviewed-by: Robin Murphy
Signed-off-by: Sricharan R
[vivek: Cleanup pm runtime calls]
Signed-off-by: Vivek Gautam
Reviewed-by: Tomasz Figa
Tested-by: Srinivas Kandagatla
---
drivers/iommu/arm-smmu.c | 89 +++-
1 file changed, 81 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d900e007c3c9..1bf542010be7 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
{ 0, NULL},
};
+static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
+{
+ if (pm_runtime_enabled(smmu->dev))
+ return pm_runtime_get_sync(smmu->dev);
+
+ return 0;
+}
+
+static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
+{
+ if (pm_runtime_enabled(smmu->dev))
+ pm_runtime_put(smmu->dev);
+}
+
static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
{
return container_of(dom, struct arm_smmu_domain, domain);
@@ -913,11 +927,15 @@ static void arm_smmu_destroy_domain_context(struct
iommu_domain *domain)
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;
struct arm_smmu_cfg *cfg = _domain->cfg;
- int irq;
+ int ret, irq;
if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
return;
+ ret = arm_smmu_rpm_get(smmu);
+ if (ret < 0)
+ return;
+
/*
* Disable the context bank and free the page tables before freeing
* it.
@@ -932,6 +950,8 @@ static void arm_smmu_destroy_domain_context(struct
iommu_domain *domain)
free_io_pgtable_ops(smmu_domain->pgtbl_ops);
__arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
+
+ arm_smmu_rpm_put(smmu);
}
static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
@@ -1213,10 +1233,15 @@ static int arm_smmu_attach_dev(struct iommu_domain
*domain, struct device *dev)
return -ENODEV;
smmu = fwspec_smmu(fwspec);
+
+ ret = arm_smmu_rpm_get(smmu);
+ if (ret < 0)
+ return ret;
+
/* Ensure that the domain is finalised */
ret = arm_smmu_init_domain_context(domain, smmu);
if (ret < 0)
- return ret;
+ goto rpm_put;
/*
* Sanity check the domain. We don't support domains across
@@ -1226,33 +1251,50 @@ static int arm_smmu_attach_dev(struct iommu_domain
*domain, struct device *dev)
dev_err(dev,
"cannot attach to SMMU %s whilst already attached to domain
on SMMU %s\n",
dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev));
- return -EINVAL;
+ ret = -EINVAL;
+ goto rpm_put;
}
/* Looks ok, so add the device to the domain */
- return arm_smmu_domain_add_master(smmu_domain, fwspec);
+ ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
+
+rpm_put:
+ arm_smmu_rpm_put(smmu);
+ return ret;
}
static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t size, int prot)
{
struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
+ struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
+ int ret;
if (!ops)
return -ENODEV;
- return ops->map(ops, iova, paddr, size, prot);
+ arm_smmu_rpm_get(smmu);
+ ret = ops->map(ops, iova, paddr, size, prot);
+ arm_smmu_rpm_put(smmu);
+
+ return ret;
}
static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
size_t size)
{
struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
+ struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
+ size_t ret;
if (!ops)
return 0;
- return ops->unmap(ops, iova, size);
+ arm_smmu_rpm_get(smmu);
+ ret = ops->unmap(ops, iova, size);
+ arm_smmu_rpm_put(smmu);
+
+ return ret;
}
static void arm_smmu_iotlb_sync(struct iommu_domain *domain)
@@ -1407,7 +1449,13 @@ static int arm_smmu_add_device(struct device *dev)
while (i--)
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
Hi Vivek, On 2018-09-25 6:56 AM, Vivek Gautam wrote: Hi Robin, Will, On Tue, Sep 18, 2018 at 8:41 AM Vivek Gautam wrote: Hi Robin, On Fri, Sep 7, 2018 at 3:52 PM Vivek Gautam wrote: On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa wrote: On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam wrote: Hi Tomasz, On 9/7/2018 2:46 PM, Tomasz Figa wrote: Hi Vivek, On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam wrote: From: Sricharan R The smmu device probe/remove and add/remove master device callbacks gets called when the smmu is not linked to its master, that is without the context of the master device. So calling runtime apis in those places separately. Global locks are also initialized before enabling runtime pm as the runtime_resume() calls device_reset() which does tlb_sync_global() that ultimately requires locks to be initialized. Signed-off-by: Sricharan R [vivek: Cleanup pm runtime calls] Signed-off-by: Vivek Gautam Reviewed-by: Tomasz Figa Tested-by: Srinivas Kandagatla --- drivers/iommu/arm-smmu.c | 89 +++- 1 file changed, 81 insertions(+), 8 deletions(-) [snip] @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev) if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) dev_err(>dev, "removing device with active domains!\n"); + arm_smmu_rpm_get(smmu); /* Turn the thing off */ writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); + arm_smmu_rpm_put(smmu); + + if (pm_runtime_enabled(smmu->dev)) + pm_runtime_force_suspend(smmu->dev); + else + clk_bulk_disable(smmu->num_clks, smmu->clks); - clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); + clk_bulk_unprepare(smmu->num_clks, smmu->clks); Aren't we missing pm_runtime_disable() here? We'll have the enable count unbalanced if the driver is removed and probed again. pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not wrong. And, as mentioned in a previous thread [1], we were seeing a warning which we avoided by keeping force_suspend(). [1] https://lkml.org/lkml/2018/7/8/124 I see, thanks. I didn't realize that pm_runtime_force_suspend() already disables runtime PM indeed. Sorry for the noise. Hi Tomasz, No problem. Thanks for looking back at it. Hi Robin, If you are fine with this series, then can you please consider giving Reviewed-by, so that we are certain that this series will go in the next merge window. Thanks Gentle ping. You ack will be very helpful in letting Will pull this series for 4.20. Thanks. I would really appreciate if you could provide your ack for this series. Or if there are any concerns, I am willing to address them. Apologies, I thought I'd replied to say I'd be getting to this shortly, but apparently not :( FWIW, "shortly" is now tomorrow - I don't *think* there's anything outstanding, but given the number of subtleties we've turned up so far I do just want one last thorough double-check to make sure. Thanks, Robin. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
Hi Robin, Will, On Tue, Sep 18, 2018 at 8:41 AM Vivek Gautam wrote: > > Hi Robin, > > On Fri, Sep 7, 2018 at 3:52 PM Vivek Gautam > wrote: > > > > On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa wrote: > > > > > > On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam > > > wrote: > > > > > > > > Hi Tomasz, > > > > > > > > > > > > On 9/7/2018 2:46 PM, Tomasz Figa wrote: > > > > > Hi Vivek, > > > > > > > > > > On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam > > > > > wrote: > > > > >> From: Sricharan R > > > > >> > > > > >> The smmu device probe/remove and add/remove master device callbacks > > > > >> gets called when the smmu is not linked to its master, that is > > > > >> without > > > > >> the context of the master device. So calling runtime apis in those > > > > >> places > > > > >> separately. > > > > >> Global locks are also initialized before enabling runtime pm as the > > > > >> runtime_resume() calls device_reset() which does tlb_sync_global() > > > > >> that ultimately requires locks to be initialized. > > > > >> > > > > >> Signed-off-by: Sricharan R > > > > >> [vivek: Cleanup pm runtime calls] > > > > >> Signed-off-by: Vivek Gautam > > > > >> Reviewed-by: Tomasz Figa > > > > >> Tested-by: Srinivas Kandagatla > > > > >> --- > > > > >> drivers/iommu/arm-smmu.c | 89 > > > > >> +++- > > > > >> 1 file changed, 81 insertions(+), 8 deletions(-) > > > > > [snip] > > > > >> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct > > > > >> platform_device *pdev) > > > > >> if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) > > > > >> dev_err(>dev, "removing device with active > > > > >> domains!\n"); > > > > >> > > > > >> + arm_smmu_rpm_get(smmu); > > > > >> /* Turn the thing off */ > > > > >> writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + > > > > >> ARM_SMMU_GR0_sCR0); > > > > >> + arm_smmu_rpm_put(smmu); > > > > >> + > > > > >> + if (pm_runtime_enabled(smmu->dev)) > > > > >> + pm_runtime_force_suspend(smmu->dev); > > > > >> + else > > > > >> + clk_bulk_disable(smmu->num_clks, smmu->clks); > > > > >> > > > > >> - clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); > > > > >> + clk_bulk_unprepare(smmu->num_clks, smmu->clks); > > > > > Aren't we missing pm_runtime_disable() here? We'll have the enable > > > > > count unbalanced if the driver is removed and probed again. > > > > > > > > pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not > > > > wrong. > > > > And, as mentioned in a previous thread [1], we were seeing a warning > > > > which we avoided > > > > by keeping force_suspend(). > > > > > > > > [1] https://lkml.org/lkml/2018/7/8/124 > > > > > > I see, thanks. I didn't realize that pm_runtime_force_suspend() > > > already disables runtime PM indeed. Sorry for the noise. > > > > Hi Tomasz, > > No problem. Thanks for looking back at it. > > > > Hi Robin, > > If you are fine with this series, then can you please consider giving > > Reviewed-by, so that we are certain that this series will go in the next > > merge > > window. > > Thanks > > Gentle ping. > You ack will be very helpful in letting Will pull this series for 4.20. > Thanks. I would really appreciate if you could provide your ack for this series. Or if there are any concerns, I am willing to address them. Thanks. Best regards Vivek > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
Hi Robin, On Fri, Sep 7, 2018 at 3:52 PM Vivek Gautam wrote: > > On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa wrote: > > > > On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam > > wrote: > > > > > > Hi Tomasz, > > > > > > > > > On 9/7/2018 2:46 PM, Tomasz Figa wrote: > > > > Hi Vivek, > > > > > > > > On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam > > > > wrote: > > > >> From: Sricharan R > > > >> > > > >> The smmu device probe/remove and add/remove master device callbacks > > > >> gets called when the smmu is not linked to its master, that is without > > > >> the context of the master device. So calling runtime apis in those > > > >> places > > > >> separately. > > > >> Global locks are also initialized before enabling runtime pm as the > > > >> runtime_resume() calls device_reset() which does tlb_sync_global() > > > >> that ultimately requires locks to be initialized. > > > >> > > > >> Signed-off-by: Sricharan R > > > >> [vivek: Cleanup pm runtime calls] > > > >> Signed-off-by: Vivek Gautam > > > >> Reviewed-by: Tomasz Figa > > > >> Tested-by: Srinivas Kandagatla > > > >> --- > > > >> drivers/iommu/arm-smmu.c | 89 > > > >> +++- > > > >> 1 file changed, 81 insertions(+), 8 deletions(-) > > > > [snip] > > > >> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct > > > >> platform_device *pdev) > > > >> if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) > > > >> dev_err(>dev, "removing device with active > > > >> domains!\n"); > > > >> > > > >> + arm_smmu_rpm_get(smmu); > > > >> /* Turn the thing off */ > > > >> writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + > > > >> ARM_SMMU_GR0_sCR0); > > > >> + arm_smmu_rpm_put(smmu); > > > >> + > > > >> + if (pm_runtime_enabled(smmu->dev)) > > > >> + pm_runtime_force_suspend(smmu->dev); > > > >> + else > > > >> + clk_bulk_disable(smmu->num_clks, smmu->clks); > > > >> > > > >> - clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); > > > >> + clk_bulk_unprepare(smmu->num_clks, smmu->clks); > > > > Aren't we missing pm_runtime_disable() here? We'll have the enable > > > > count unbalanced if the driver is removed and probed again. > > > > > > pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not > > > wrong. > > > And, as mentioned in a previous thread [1], we were seeing a warning > > > which we avoided > > > by keeping force_suspend(). > > > > > > [1] https://lkml.org/lkml/2018/7/8/124 > > > > I see, thanks. I didn't realize that pm_runtime_force_suspend() > > already disables runtime PM indeed. Sorry for the noise. > > Hi Tomasz, > No problem. Thanks for looking back at it. > > Hi Robin, > If you are fine with this series, then can you please consider giving > Reviewed-by, so that we are certain that this series will go in the next merge > window. > Thanks Gentle ping. You ack will be very helpful in letting Will pull this series for 4.20. Thanks. Best regards Vivek -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa wrote: > > On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam > wrote: > > > > Hi Tomasz, > > > > > > On 9/7/2018 2:46 PM, Tomasz Figa wrote: > > > Hi Vivek, > > > > > > On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam > > > wrote: > > >> From: Sricharan R > > >> > > >> The smmu device probe/remove and add/remove master device callbacks > > >> gets called when the smmu is not linked to its master, that is without > > >> the context of the master device. So calling runtime apis in those places > > >> separately. > > >> Global locks are also initialized before enabling runtime pm as the > > >> runtime_resume() calls device_reset() which does tlb_sync_global() > > >> that ultimately requires locks to be initialized. > > >> > > >> Signed-off-by: Sricharan R > > >> [vivek: Cleanup pm runtime calls] > > >> Signed-off-by: Vivek Gautam > > >> Reviewed-by: Tomasz Figa > > >> Tested-by: Srinivas Kandagatla > > >> --- > > >> drivers/iommu/arm-smmu.c | 89 > > >> +++- > > >> 1 file changed, 81 insertions(+), 8 deletions(-) > > > [snip] > > >> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct > > >> platform_device *pdev) > > >> if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) > > >> dev_err(>dev, "removing device with active > > >> domains!\n"); > > >> > > >> + arm_smmu_rpm_get(smmu); > > >> /* Turn the thing off */ > > >> writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + > > >> ARM_SMMU_GR0_sCR0); > > >> + arm_smmu_rpm_put(smmu); > > >> + > > >> + if (pm_runtime_enabled(smmu->dev)) > > >> + pm_runtime_force_suspend(smmu->dev); > > >> + else > > >> + clk_bulk_disable(smmu->num_clks, smmu->clks); > > >> > > >> - clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); > > >> + clk_bulk_unprepare(smmu->num_clks, smmu->clks); > > > Aren't we missing pm_runtime_disable() here? We'll have the enable > > > count unbalanced if the driver is removed and probed again. > > > > pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not > > wrong. > > And, as mentioned in a previous thread [1], we were seeing a warning > > which we avoided > > by keeping force_suspend(). > > > > [1] https://lkml.org/lkml/2018/7/8/124 > > I see, thanks. I didn't realize that pm_runtime_force_suspend() > already disables runtime PM indeed. Sorry for the noise. Hi Tomasz, No problem. Thanks for looking back at it. Hi Robin, If you are fine with this series, then can you please consider giving Reviewed-by, so that we are certain that this series will go in the next merge window. Thanks Best regards Vivek -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam wrote: > > Hi Tomasz, > > > On 9/7/2018 2:46 PM, Tomasz Figa wrote: > > Hi Vivek, > > > > On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam > > wrote: > >> From: Sricharan R > >> > >> The smmu device probe/remove and add/remove master device callbacks > >> gets called when the smmu is not linked to its master, that is without > >> the context of the master device. So calling runtime apis in those places > >> separately. > >> Global locks are also initialized before enabling runtime pm as the > >> runtime_resume() calls device_reset() which does tlb_sync_global() > >> that ultimately requires locks to be initialized. > >> > >> Signed-off-by: Sricharan R > >> [vivek: Cleanup pm runtime calls] > >> Signed-off-by: Vivek Gautam > >> Reviewed-by: Tomasz Figa > >> Tested-by: Srinivas Kandagatla > >> --- > >> drivers/iommu/arm-smmu.c | 89 > >> +++- > >> 1 file changed, 81 insertions(+), 8 deletions(-) > > [snip] > >> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct > >> platform_device *pdev) > >> if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) > >> dev_err(>dev, "removing device with active > >> domains!\n"); > >> > >> + arm_smmu_rpm_get(smmu); > >> /* Turn the thing off */ > >> writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); > >> + arm_smmu_rpm_put(smmu); > >> + > >> + if (pm_runtime_enabled(smmu->dev)) > >> + pm_runtime_force_suspend(smmu->dev); > >> + else > >> + clk_bulk_disable(smmu->num_clks, smmu->clks); > >> > >> - clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); > >> + clk_bulk_unprepare(smmu->num_clks, smmu->clks); > > Aren't we missing pm_runtime_disable() here? We'll have the enable > > count unbalanced if the driver is removed and probed again. > > pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not > wrong. > And, as mentioned in a previous thread [1], we were seeing a warning > which we avoided > by keeping force_suspend(). > > [1] https://lkml.org/lkml/2018/7/8/124 I see, thanks. I didn't realize that pm_runtime_force_suspend() already disables runtime PM indeed. Sorry for the noise. Best regards, Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
Hi Tomasz, On 9/7/2018 2:46 PM, Tomasz Figa wrote: Hi Vivek, On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam wrote: From: Sricharan R The smmu device probe/remove and add/remove master device callbacks gets called when the smmu is not linked to its master, that is without the context of the master device. So calling runtime apis in those places separately. Global locks are also initialized before enabling runtime pm as the runtime_resume() calls device_reset() which does tlb_sync_global() that ultimately requires locks to be initialized. Signed-off-by: Sricharan R [vivek: Cleanup pm runtime calls] Signed-off-by: Vivek Gautam Reviewed-by: Tomasz Figa Tested-by: Srinivas Kandagatla --- drivers/iommu/arm-smmu.c | 89 +++- 1 file changed, 81 insertions(+), 8 deletions(-) [snip] @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev) if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) dev_err(>dev, "removing device with active domains!\n"); + arm_smmu_rpm_get(smmu); /* Turn the thing off */ writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); + arm_smmu_rpm_put(smmu); + + if (pm_runtime_enabled(smmu->dev)) + pm_runtime_force_suspend(smmu->dev); + else + clk_bulk_disable(smmu->num_clks, smmu->clks); - clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); + clk_bulk_unprepare(smmu->num_clks, smmu->clks); Aren't we missing pm_runtime_disable() here? We'll have the enable count unbalanced if the driver is removed and probed again. pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not wrong. And, as mentioned in a previous thread [1], we were seeing a warning which we avoided by keeping force_suspend(). [1] https://lkml.org/lkml/2018/7/8/124 Thanks Vivek Also, if we add pm_runtime_disable(), we can reorder things a bit and simplify into: arm_smmu_rpm_get(smmu); /* Turn the thing off */ writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); if (pm_runtime_enabled()) pm_runtime_disable(); arm_smmu_rpm_put(smmu); clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); Best regards, Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v16 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
Hi Vivek, On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam wrote: > > From: Sricharan R > > The smmu device probe/remove and add/remove master device callbacks > gets called when the smmu is not linked to its master, that is without > the context of the master device. So calling runtime apis in those places > separately. > Global locks are also initialized before enabling runtime pm as the > runtime_resume() calls device_reset() which does tlb_sync_global() > that ultimately requires locks to be initialized. > > Signed-off-by: Sricharan R > [vivek: Cleanup pm runtime calls] > Signed-off-by: Vivek Gautam > Reviewed-by: Tomasz Figa > Tested-by: Srinivas Kandagatla > --- > drivers/iommu/arm-smmu.c | 89 > +++- > 1 file changed, 81 insertions(+), 8 deletions(-) [snip] > @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct > platform_device *pdev) > if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) > dev_err(>dev, "removing device with active domains!\n"); > > + arm_smmu_rpm_get(smmu); > /* Turn the thing off */ > writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); > + arm_smmu_rpm_put(smmu); > + > + if (pm_runtime_enabled(smmu->dev)) > + pm_runtime_force_suspend(smmu->dev); > + else > + clk_bulk_disable(smmu->num_clks, smmu->clks); > > - clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); > + clk_bulk_unprepare(smmu->num_clks, smmu->clks); Aren't we missing pm_runtime_disable() here? We'll have the enable count unbalanced if the driver is removed and probed again. Also, if we add pm_runtime_disable(), we can reorder things a bit and simplify into: arm_smmu_rpm_get(smmu); /* Turn the thing off */ writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); if (pm_runtime_enabled()) pm_runtime_disable(); arm_smmu_rpm_put(smmu); clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); Best regards, Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
