Re: [PATCH v12 3/4] iommu/arm-smmu: Add the device_link between masters and smmu

[Vivek Gautam]

Hi Rafael,


On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote:
> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote:
> > From: Sricharan R <sricharan@xxxxxxxxxxxxxx>
> >
> > Finally add the device link between the master device and
> > smmu, so that the smmu gets runtime enabled/disabled only when the
> > master needs it. This is done from add_device callback which gets
> > called once when the master is added to the smmu.
> >
> > Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx>
> > Signed-off-by: Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx>
> > Reviewed-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> > Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> > Cc: Lukas Wunner <lukas@xxxxxxxxx>
> > ---
> >
> >   - Change since v11
> >     * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER.
> >
> >   drivers/iommu/arm-smmu.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 09265e206e2d..916cde4954d2 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev)
> >   
> >   	iommu_device_link(&smmu->iommu, dev);
> >   
> > +	if (pm_runtime_enabled(smmu->dev) &&
> Why does the creation of the link depend on whether or not runtime PM
> is enabled for the MMU device?

The main purpose of this device link is to handle the runtime PM 
synchronization
between the supplier (iommu) and consumer (client devices, such as 
GPU/display).
Moreover, the runtime pm is conditionally enabled for smmu devices that 
support
such [1].
> What about system-wide PM and system shutdown?  Are they always guaranteed
> to happen in the right order without the link?

When there's no runtime PM, there's no clocks, and other resources to be 
handled.
So, we don't need device link for system-wide PM and system shutdown to 
work correctly.
That's the case with current arm-smmu driver.
Is it something that i am missing here?

[1] https://lkml.org/lkml/2018/3/8/775

Thanks
Vivek
> > +	    !device_link_add(dev, smmu->dev,
> > +			DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) {
> > +		dev_err(smmu->dev, "Unable to add link to the consumer %s\n",
> > +			dev_name(dev));
> > +		ret = -ENODEV;
> > +		goto out_unlink;
> > +	}
> > +
> >   	return 0;
> >   
> > +out_unlink:
> > +	iommu_device_unlink(&smmu->iommu, dev);
> > +	arm_smmu_master_free_smes(fwspec);
> >   out_cfg_free:
> >   	kfree(cfg);
> >   out_free: