Re: [PATCH V9 3/3] PCI: tegra: Add power management support

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Sat, Mar 03, 2018 at 01:47:38PM +0530, Manikanta Maddireddy wrote:

[...]

> >> +static int tegra_pcie_pm_resume(struct device *dev)
> >> +{
> >> +	struct tegra_pcie *pcie = dev_get_drvdata(dev);
> >> +	int err;
> >> +
> >> +	err = tegra_pcie_power_on(pcie);
> >> +	if (err) {
> >> +		dev_err(dev, "tegra pcie power on fail: %d\n", err);
> >> +		return err;
> >> +	}
> >> +	err = tegra_pcie_enable_controller(pcie);
> >> +	if (err) {
> >> +		dev_err(dev, "tegra pcie controller enable fail: %d\n", err);
> >> +		goto poweroff;
> >> +	}
> >> +	tegra_pcie_setup_translations(pcie);
> >> +
> >> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> >> +		tegra_pcie_enable_msi(pcie);
> >> +
> >> +	tegra_pcie_enable_ports(pcie);
> > 
> > Is it correct to report a successfull resume if some of the ports fail
> > to come up (whether within runtime PM or system suspend) ? I think that,
> > if any of the ports fails to come up, returning a failure is more
> > appropriate here given that the host bridge is a single device as far as
> > the kernel is concerned.
> > 
> > Thanks,
> > Lorenzo
> > 
> 
> Hi Lorenzo,
> 
> We(Thierry, Vidya Sagar and myself) had a discussion on this topic
> and we chose not to fail probe/resume because host controller is
> initialized properly, so failing the probe/resume is not the right
> thing to do.

What happens then to endpoints downstream a rootport that failed to
resume but we nonetheless reported that it successfully resume instead ?

Thanks,
Lorenzo

> PCIe link may fail to come up if there is no endpoint in the slot
> or interoperable failure where endpoint specific quirk or work
> around required. In such cases host controller driver shouldn't
> fail.
> 
> Also user can connect endpoint in only one slot and expect it to be
> working. Even though host bridge is a single device, driver should
> allow standalone port to work.
> 
> Thanks,
> Manikanta
> 
> >>  
> >>  	return 0;
> >> +
> >> +poweroff:
> >> +	tegra_pcie_power_off(pcie);
> >> +
> >> +	return err;
> >>  }
> >>  
> >> +static const struct dev_pm_ops tegra_pcie_pm_ops = {
> >> +	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
> >> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
> >> +				      tegra_pcie_pm_resume)
> >> +};
> >> +
> >>  static struct platform_driver tegra_pcie_driver = {
> >>  	.driver = {
> >>  		.name = "tegra-pcie",
> >>  		.of_match_table = tegra_pcie_of_match,
> >>  		.suppress_bind_attrs = true,
> >> +		.pm = &tegra_pcie_pm_ops,
> >>  	},
> >>  	.probe = tegra_pcie_probe,
> >>  	.remove = tegra_pcie_remove,
> >> -- 
> >> 2.1.4
> >>