| Message ID | 1509371843-22931-2-git-send-email-mmaddireddy@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | Enable Tegra root port features and apply SW fixups | expand |
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@nvidia.com> > --- > 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 > reset_control_assert(pcie->afi_rst); > reset_control_assert(pcie->pex_rst); > > @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) > port->index, port->lanes); > > tegra_pcie_port_enable(port); > + } > > + /* take the PCIe interface module out of reset */ > + reset_control_deassert(pcie->pcie_xrst); > + > + list_for_each_entry_safe(port, tmp, &pcie->ports, list) { > if (tegra_pcie_port_check_link(port)) > continue; > > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> >> --- >> 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. > >> reset_control_assert(pcie->afi_rst); >> reset_control_assert(pcie->pex_rst); >> >> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) >> port->index, port->lanes); >> >> tegra_pcie_port_enable(port); >> + } >> >> + /* take the PCIe interface module out of reset */ >> + reset_control_deassert(pcie->pcie_xrst); >> + >> + list_for_each_entry_safe(port, tmp, &pcie->ports, list) { >> if (tegra_pcie_port_check_link(port)) >> continue; >> >> -- >> 2.1.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> > >> --- > >> 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 > >> reset_control_assert(pcie->afi_rst); > >> reset_control_assert(pcie->pex_rst); > >> > >> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) > >> port->index, port->lanes); > >> > >> tegra_pcie_port_enable(port); > >> + } > >> > >> + /* take the PCIe interface module out of reset */ > >> + reset_control_deassert(pcie->pcie_xrst); > >> + > >> + list_for_each_entry_safe(port, tmp, &pcie->ports, list) { > >> if (tegra_pcie_port_check_link(port)) > >> continue; > >> > >> -- > >> 2.1.4 > >> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> >>>> --- >>>> 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. >>>> reset_control_assert(pcie->afi_rst); >>>> reset_control_assert(pcie->pex_rst); >>>> >>>> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) >>>> port->index, port->lanes); >>>> >>>> tegra_pcie_port_enable(port); >>>> + } >>>> >>>> + /* take the PCIe interface module out of reset */ >>>> + reset_control_deassert(pcie->pcie_xrst); >>>> + >>>> + list_for_each_entry_safe(port, tmp, &pcie->ports, list) { >>>> if (tegra_pcie_port_check_link(port)) >>>> continue; >>>> >>>> -- >>>> 2.1.4 >>>> > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> > >>>> --- > >>>> 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 > >>>> reset_control_assert(pcie->afi_rst); > >>>> reset_control_assert(pcie->pex_rst); > >>>> > >>>> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) > >>>> port->index, port->lanes); > >>>> > >>>> tegra_pcie_port_enable(port); > >>>> + } > >>>> > >>>> + /* take the PCIe interface module out of reset */ > >>>> + reset_control_deassert(pcie->pcie_xrst); > >>>> + > >>>> + list_for_each_entry_safe(port, tmp, &pcie->ports, list) { > >>>> if (tegra_pcie_port_check_link(port)) > >>>> continue; > >>>> > >>>> -- > >>>> 2.1.4 > >>>> > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> >>>>>> --- >>>>>> 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. >>>>>> reset_control_assert(pcie->afi_rst); >>>>>> reset_control_assert(pcie->pex_rst); >>>>>> >>>>>> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) >>>>>> port->index, port->lanes); >>>>>> >>>>>> tegra_pcie_port_enable(port); >>>>>> + } >>>>>> >>>>>> + /* take the PCIe interface module out of reset */ >>>>>> + reset_control_deassert(pcie->pcie_xrst); >>>>>> + >>>>>> + list_for_each_entry_safe(port, tmp, &pcie->ports, list) { >>>>>> if (tegra_pcie_port_check_link(port)) >>>>>> continue; >>>>>> >>>>>> -- >>>>>> 2.1.4 >>>>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> > >>>>>> --- > >>>>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> > > >>>>>> --- > > >>>>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> >>>>>>>>> --- >>>>>>>>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); reset_control_assert(pcie->afi_rst); reset_control_assert(pcie->pex_rst); @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) port->index, port->lanes); tegra_pcie_port_enable(port); + } + /* take the PCIe interface module out of reset */ + reset_control_deassert(pcie->pcie_xrst); + + list_for_each_entry_safe(port, tmp, &pcie->ports, list) { if (tegra_pcie_port_check_link(port)) continue;
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@nvidia.com> --- 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(-)