On 07-Mar-18 5:30 PM, Lorenzo Pieralisi wrote: > On Thu, Dec 14, 2017 at 09:57:24AM +0000, Lorenzo Pieralisi wrote: >> On Thu, Dec 14, 2017 at 12:57:28AM +0530, Manikanta Maddireddy wrote: >>> >>> >>> On 14-Dec-17 12:04 AM, Lorenzo Pieralisi wrote: >>>> On Wed, Dec 13, 2017 at 10:02:02PM +0530, Manikanta Maddireddy wrote: >>>>> >>>>> >>>>> On 13-Dec-17 7:38 PM, Lorenzo Pieralisi wrote: >>>>>> On Wed, Dec 13, 2017 at 05:20:39PM +0530, Manikanta Maddireddy wrote: >>>>>>> >>>>>>> >>>>>>> On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote: >>>>>>>> On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote: >>>>>>>>> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only >>>>>>>>> after all the required root port register programming is completed. >>>>>>>>> >>>>>>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx> >>>>>>>>> --- >>>>>>>>> V3: >>>>>>>>> * no change in this patch >>>>>>>>> V2: >>>>>>>>> * no change in this patch >>>>>>>>> >>>>>>>>> drivers/pci/host/pci-tegra.c | 9 +++++---- >>>>>>>>> 1 file changed, 5 insertions(+), 4 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c >>>>>>>>> index 96e8038c3019..b41c60c7414c 100644 >>>>>>>>> --- a/drivers/pci/host/pci-tegra.c >>>>>>>>> +++ b/drivers/pci/host/pci-tegra.c >>>>>>>>> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie) >>>>>>>>> } >>>>>>>>> } >>>>>>>>> >>>>>>>>> - /* take the PCIe interface module out of reset */ >>>>>>>>> - reset_control_deassert(pcie->pcie_xrst); >>>>>>>>> - >>>>>>>>> /* finally enable PCIe */ >>>>>>>>> value = afi_readl(pcie, AFI_CONFIGURATION); >>>>>>>>> value |= AFI_CONFIGURATION_EN_FPCI; >>>>>>>>> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie) >>>>>>>>> dev_err(dev, "failed to power off PHY(s): %d\n", err); >>>>>>>>> } >>>>>>>>> >>>>>>>>> - reset_control_assert(pcie->pcie_xrst); >>>>>>>> >>>>>>>> This does not look like it is part of the reset de-assertion code move. >>>>>>>> >>>>>>>> tegra_pcie_enable_controller() -> tegra_pcie_enable_ports() >>>>>>>> >>>>>>>> in other words, why are you removing it ? >>>>>>>> >>>>>>>> Lorenzo >>>>>>> >>>>>>> Hi Lorenzo, >>>>>>> >>>>>>> Host driver should start LTSSM after programming all controller registers. >>>>>>> In tegra_pcie_enable_controller() bunch of AFI module programming is done and >>>>>>> I am adding PCIe register programming in this series. >>>>>>> So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(), >>>>>>> which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint. >>>>>> >>>>>> I asked why you removed the reset assertion in tegra_pcie_power_off(), >>>>>> it is not clear to me. You still call tegra_pcie_power_off() in >>>>>> the tegra_pcie_probe() error path and I see no reason why the reset >>>>>> assertion - called through: >>>>>> >>>>>> tegra_pcie_put_resources() >>>>>> -> tegra_pcie_power_of() >>>>>> >>>>>> is removed, if it was needed previously. >>>>>> >>>>>> Lorenzo >>>>>> >>>>> >>>>> New sequence with this patch will be >>>>> tegra_pcie_enable_controller() -> tegra_pcie_request_resources() -> tegra_pcie_enable_ports() >>>>> ->goto put_resources on fail -> reset_control_deassert(pcie->pcie_xrst); >>>>> >>>>> Since pcie_xrst deassert happens after tegra_pcie_request_resources(), there is no need to assert pcie_xrst on put_resource failure. >>>> >>>> I do not understand you, sorry for being blunt. >>>> >>>> What has tegra_pcie_request_resources() to do with the reset >>>> assertion/deassertion ? >>>> >>>> This patch moves: >>>> >>>> reset_control_deassert(pcie->pcie_xrst); >>>> >>>> from: >>>> >>>> tegra_pcie_enable_controller() >>>> >>>> to >>>> >>>> tegra_pcie_enable_ports() >>>> >>>> if: >>>> >>>> reset_control_assert(pcie->pcie_xrst); >>>> >>>> was needed before this patch in tegra_pcie_power_off() >>>> >>>> why it is not needed there after this patch is applied ? >>>> >>>> Lorenzo >>>> >>> Opposite of tegra_pcie_enable_ports() is missing on disable_msi >>> failure in current driver. So I took care of assert pcie_xrst >>> in https://patchwork.ozlabs.org/patch/846043/ along with other >>> resources. >> >> Which is in another series. This patch has to be self-contained with a >> single logical change that is applicable on its own. Even if we end up >> merging the series above every patch has to make sense on its own. This >> does not. >> >> So I would start with re-posting this series as a self contained one, >> addressing all review comments; we will get to other series later when >> this is sorted. > > Hi Manikanta, > > I wanted to check with you where are we with this series. I would mark > it with "Changes requested", waiting for you to rebase it and update it > according to review comments. > > Please let me know if that's OK with you. > > Thanks, > Lorenzo > Hi Lorenzo, Sure, I will address the review comments and publish rebased series soon. Thanks, Manikanta -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
