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. Thanks, Lorenzo
