On 06-Mar-18 5:29 PM, Lorenzo Pieralisi wrote:
> On Tue, Mar 06, 2018 at 03:26:33PM +0530, Manikanta Maddireddy wrote:
>>
>>
>> On 05-Mar-18 3:11 PM, Lorenzo Pieralisi wrote:
>>> 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
>>>
>>
>> Hi Lorenzo,
>>
>> If an endpoint comes up in probe & enumeration is completed and if
>> the same endpoint doesn't come up during resume I am observing
>> PCIe errors because PCIe enumeration hierarchy is unchanged &
>> PCI subsystem is trying to restore PCI device. This observation
>> is same if host driver returns error.
>>
>> To handle this case driver needs to remove the respective
>> PCIe port enumeration hierarchy if link doesn't come up
>> in resume. It will tricky to identify PCI bus structure
>> from host port ID in resume, may be some book keeping required
>> here.
>>
>> However if the PCIe link comes up in probe, it should come up
>> during resume as well else it will be a bug. Do you see the
>> need for changing the PCIe enumeration hierarchy in resume
>> based on link up status?
>
> I think that for the time being it is fine to merge the current
> set - I asked just for clarification; I agree there is no easy answer
> to the issue I raised. It should be safe to merge the series in its
> current form - I will keep this issue on my radar to see if there
> is something we can do to improve it.
>
> Thanks,
> Lorenzo
>
Thank you Lorenzo
>>
>> Thanks,
>> Manikanta
>>
>>>> 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
>>>>>>
