| Message ID | 1509371843-22931-3-git-send-email-mmaddireddy@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote: > In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped It is time you defined what FW does and what the kernel does and stick to that SW initialization sequence for all Tegra PCI host bridges from now onwards. Those tegra_pcie_soc structures in the driver will end up with a gazillion hardcoded parameters if you do not take a stance from that perspective and I do not like that - at all. You may want to use DT bindings to describe the FW<->OS handover - or just enforce firmware guidelines for Tegra systems - something has to be done. This patch makes sense stand-alone but I wanted to get the message across. Thanks, Lorenzo > in driver. REFCLK pad settings are independent of PHY and should be > programmed by driver. So move REFCLK pad settings out of phy_power_on(). > These pad settings improve REFCLK peak to peak amplitude. > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > --- > V3: > * Corrected commit log > V2: > * no change in this patch > > drivers/pci/host/pci-tegra.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c > index b41c60c7414c..068510b40c1a 100644 > --- a/drivers/pci/host/pci-tegra.c > +++ b/drivers/pci/host/pci-tegra.c > @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port) > static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) > { > struct device *dev = pcie->dev; > - const struct tegra_pcie_soc *soc = pcie->soc; > struct tegra_pcie_port *port; > int err; > > @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) > } > } > > - /* Configure the reference clock driver */ > - pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); > - > - if (soc->num_ports > 2) > - pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); > - > return 0; > } > > @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > return 0; > } > > +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie) > +{ > + const struct tegra_pcie_soc *soc = pcie->soc; > + > + /* Configure the reference clock driver */ > + pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); > + > + if (soc->num_ports > 2) > + pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); > +} > + > /* > * FIXME: If there are no PCIe cards attached, then calling this function > * can result in the increase of the bootup time as there are big timeout > @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) > struct device *dev = pcie->dev; > struct tegra_pcie_port *port, *tmp; > > + tegra_pcie_apply_pad_settings(pcie); > + > list_for_each_entry_safe(port, tmp, &pcie->ports, list) { > dev_info(dev, "probing port %u, using %u lanes\n", > port->index, port->lanes); > -- > 2.1.4 >
On 13.12.2017 16:23, Lorenzo Pieralisi wrote: > On Wed, Dec 13, 2017 at 05:32:32PM +0530, Manikanta Maddireddy wrote: >> >> >> On 12-Dec-17 5:15 PM, Lorenzo Pieralisi wrote: >>> On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote: >>>> In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped >>> >>> It is time you defined what FW does and what the kernel does and stick >>> to that SW initialization sequence for all Tegra PCI host bridges from >>> now onwards. Those tegra_pcie_soc structures in the driver will end up >>> with a gazillion hardcoded parameters if you do not take a stance from >>> that perspective and I do not like that - at all. >>> >>> You may want to use DT bindings to describe the FW<->OS handover - or >>> just enforce firmware guidelines for Tegra systems - something has to be >>> done. >>> >>> This patch makes sense stand-alone but I wanted to get the message >>> across. >>> >>> Thanks, >>> Lorenzo >>> >> >> In Tegra186 BPMP-FW will take care of programming the UPHY, this is >> updated in commit log of "9cea513d8cbc ("PCI: tegra: Add Tegra186 PCIe support")". >> I didn't put it in DT bindings because there is no DT property for this purpose. >> Do you want me to add a comment in PCIe tegra driver, perhaps near program_uphy variable? > > I would like to see some consistency from now onwards on what's done > in BPMP (or host) FW and what's done in the driver and that's something > you should drive because, as I have already said, the current approch > based on boolean flags per-SoC hardcoded in the kernel does not scale - > I may tolerate it for now but that has to change. > The model has changed once, when BPMP was introduced. So pre-BPMP SoCs (Tegra20/30/114/124/210) program the UPHY in kernel and post-BPMP SoCs (Tegra186+) have the BPMP program it. So there should not be further changes to this. Mikko > Is it that hard to define a software programming model (to describe > what's done in what piece of software/firmware) for tegra PCI host > bridges > >> In current host driver, REFCLK pads settings are done in >> tegra_pcie_port_phy_power_off() which is not apt. REFCLK pad settings >> tunes the PCIe sideband signal REFCLK, it has nothing to do with UPHY. >> So I am moving it out of tegra_pcie_port_phy_power_off(). > > I gathered that, I was not complaining on this patch on his own. > > Thanks, > Lorenzo > >> Thanks, >> Manikanta >> >> >>>> in driver. REFCLK pad settings are independent of PHY and should be >>>> programmed by driver. So move REFCLK pad settings out of phy_power_on(). >>>> These pad settings improve REFCLK peak to peak amplitude. >>>> >>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> >>>> --- >>>> V3: >>>> * Corrected commit log >>>> V2: >>>> * no change in this patch >>>> >>>> drivers/pci/host/pci-tegra.c | 20 +++++++++++++------- >>>> 1 file changed, 13 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c >>>> index b41c60c7414c..068510b40c1a 100644 >>>> --- a/drivers/pci/host/pci-tegra.c >>>> +++ b/drivers/pci/host/pci-tegra.c >>>> @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port) >>>> static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) >>>> { >>>> struct device *dev = pcie->dev; >>>> - const struct tegra_pcie_soc *soc = pcie->soc; >>>> struct tegra_pcie_port *port; >>>> int err; >>>> >>>> @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) >>>> } >>>> } >>>> >>>> - /* Configure the reference clock driver */ >>>> - pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); >>>> - >>>> - if (soc->num_ports > 2) >>>> - pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); >>>> - >>>> return 0; >>>> } >>>> >>>> @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >>>> return 0; >>>> } >>>> >>>> +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie) >>>> +{ >>>> + const struct tegra_pcie_soc *soc = pcie->soc; >>>> + >>>> + /* Configure the reference clock driver */ >>>> + pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); >>>> + >>>> + if (soc->num_ports > 2) >>>> + pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); >>>> +} >>>> + >>>> /* >>>> * FIXME: If there are no PCIe cards attached, then calling this function >>>> * can result in the increase of the bootup time as there are big timeout >>>> @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) >>>> struct device *dev = pcie->dev; >>>> struct tegra_pcie_port *port, *tmp; >>>> >>>> + tegra_pcie_apply_pad_settings(pcie); >>>> + >>>> list_for_each_entry_safe(port, tmp, &pcie->ports, list) { >>>> dev_info(dev, "probing port %u, using %u lanes\n", >>>> port->index, port->lanes); >>>> -- >>>> 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 12-Dec-17 5:15 PM, Lorenzo Pieralisi wrote: > On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote: >> In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped > > It is time you defined what FW does and what the kernel does and stick > to that SW initialization sequence for all Tegra PCI host bridges from > now onwards. Those tegra_pcie_soc structures in the driver will end up > with a gazillion hardcoded parameters if you do not take a stance from > that perspective and I do not like that - at all. > > You may want to use DT bindings to describe the FW<->OS handover - or > just enforce firmware guidelines for Tegra systems - something has to be > done. > > This patch makes sense stand-alone but I wanted to get the message > across. > > Thanks, > Lorenzo > In Tegra186 BPMP-FW will take care of programming the UPHY, this is updated in commit log of "9cea513d8cbc ("PCI: tegra: Add Tegra186 PCIe support")". I didn't put it in DT bindings because there is no DT property for this purpose. Do you want me to add a comment in PCIe tegra driver, perhaps near program_uphy variable? In current host driver, REFCLK pads settings are done in tegra_pcie_port_phy_power_off() which is not apt. REFCLK pad settings tunes the PCIe sideband signal REFCLK, it has nothing to do with UPHY. So I am moving it out of tegra_pcie_port_phy_power_off(). Thanks, Manikanta >> in driver. REFCLK pad settings are independent of PHY and should be >> programmed by driver. So move REFCLK pad settings out of phy_power_on(). >> These pad settings improve REFCLK peak to peak amplitude. >> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> >> --- >> V3: >> * Corrected commit log >> V2: >> * no change in this patch >> >> drivers/pci/host/pci-tegra.c | 20 +++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c >> index b41c60c7414c..068510b40c1a 100644 >> --- a/drivers/pci/host/pci-tegra.c >> +++ b/drivers/pci/host/pci-tegra.c >> @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port) >> static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) >> { >> struct device *dev = pcie->dev; >> - const struct tegra_pcie_soc *soc = pcie->soc; >> struct tegra_pcie_port *port; >> int err; >> >> @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) >> } >> } >> >> - /* Configure the reference clock driver */ >> - pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); >> - >> - if (soc->num_ports > 2) >> - pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); >> - >> return 0; >> } >> >> @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >> return 0; >> } >> >> +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie) >> +{ >> + const struct tegra_pcie_soc *soc = pcie->soc; >> + >> + /* Configure the reference clock driver */ >> + pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); >> + >> + if (soc->num_ports > 2) >> + pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); >> +} >> + >> /* >> * FIXME: If there are no PCIe cards attached, then calling this function >> * can result in the increase of the bootup time as there are big timeout >> @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) >> struct device *dev = pcie->dev; >> struct tegra_pcie_port *port, *tmp; >> >> + tegra_pcie_apply_pad_settings(pcie); >> + >> list_for_each_entry_safe(port, tmp, &pcie->ports, list) { >> dev_info(dev, "probing port %u, using %u lanes\n", >> port->index, port->lanes); >> -- >> 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:32:32PM +0530, Manikanta Maddireddy wrote: > > > On 12-Dec-17 5:15 PM, Lorenzo Pieralisi wrote: > > On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote: > >> In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped > > > > It is time you defined what FW does and what the kernel does and stick > > to that SW initialization sequence for all Tegra PCI host bridges from > > now onwards. Those tegra_pcie_soc structures in the driver will end up > > with a gazillion hardcoded parameters if you do not take a stance from > > that perspective and I do not like that - at all. > > > > You may want to use DT bindings to describe the FW<->OS handover - or > > just enforce firmware guidelines for Tegra systems - something has to be > > done. > > > > This patch makes sense stand-alone but I wanted to get the message > > across. > > > > Thanks, > > Lorenzo > > > > In Tegra186 BPMP-FW will take care of programming the UPHY, this is > updated in commit log of "9cea513d8cbc ("PCI: tegra: Add Tegra186 PCIe support")". > I didn't put it in DT bindings because there is no DT property for this purpose. > Do you want me to add a comment in PCIe tegra driver, perhaps near program_uphy variable? I would like to see some consistency from now onwards on what's done in BPMP (or host) FW and what's done in the driver and that's something you should drive because, as I have already said, the current approch based on boolean flags per-SoC hardcoded in the kernel does not scale - I may tolerate it for now but that has to change. Is it that hard to define a software programming model (to describe what's done in what piece of software/firmware) for tegra PCI host bridges ? > In current host driver, REFCLK pads settings are done in > tegra_pcie_port_phy_power_off() which is not apt. REFCLK pad settings > tunes the PCIe sideband signal REFCLK, it has nothing to do with UPHY. > So I am moving it out of tegra_pcie_port_phy_power_off(). I gathered that, I was not complaining on this patch on his own. Thanks, Lorenzo > Thanks, > Manikanta > > > >> in driver. REFCLK pad settings are independent of PHY and should be > >> programmed by driver. So move REFCLK pad settings out of phy_power_on(). > >> These pad settings improve REFCLK peak to peak amplitude. > >> > >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > >> --- > >> V3: > >> * Corrected commit log > >> V2: > >> * no change in this patch > >> > >> drivers/pci/host/pci-tegra.c | 20 +++++++++++++------- > >> 1 file changed, 13 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c > >> index b41c60c7414c..068510b40c1a 100644 > >> --- a/drivers/pci/host/pci-tegra.c > >> +++ b/drivers/pci/host/pci-tegra.c > >> @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port) > >> static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) > >> { > >> struct device *dev = pcie->dev; > >> - const struct tegra_pcie_soc *soc = pcie->soc; > >> struct tegra_pcie_port *port; > >> int err; > >> > >> @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) > >> } > >> } > >> > >> - /* Configure the reference clock driver */ > >> - pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); > >> - > >> - if (soc->num_ports > 2) > >> - pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); > >> - > >> return 0; > >> } > >> > >> @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > >> return 0; > >> } > >> > >> +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie) > >> +{ > >> + const struct tegra_pcie_soc *soc = pcie->soc; > >> + > >> + /* Configure the reference clock driver */ > >> + pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); > >> + > >> + if (soc->num_ports > 2) > >> + pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); > >> +} > >> + > >> /* > >> * FIXME: If there are no PCIe cards attached, then calling this function > >> * can result in the increase of the bootup time as there are big timeout > >> @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) > >> struct device *dev = pcie->dev; > >> struct tegra_pcie_port *port, *tmp; > >> > >> + tegra_pcie_apply_pad_settings(pcie); > >> + > >> list_for_each_entry_safe(port, tmp, &pcie->ports, list) { > >> dev_info(dev, "probing port %u, using %u lanes\n", > >> port->index, port->lanes); > >> -- > >> 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 Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote: > In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped > in driver. REFCLK pad settings are independent of PHY and should be > programmed by driver. So move REFCLK pad settings out of phy_power_on(). > These pad settings improve REFCLK peak to peak amplitude. > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > --- > V3: > * Corrected commit log > V2: > * no change in this patch > > drivers/pci/host/pci-tegra.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) As far as I can tell, this is a bugfix that can and should be applied independently of the rest of the series. Acked-by: Thierry Reding <treding@nvidia.com>
On Thu, Dec 14, 2017 at 04:14:52PM +0100, Thierry Reding wrote: > On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote: > > In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped > > in driver. REFCLK pad settings are independent of PHY and should be > > programmed by driver. So move REFCLK pad settings out of phy_power_on(). > > These pad settings improve REFCLK peak to peak amplitude. > > > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > > --- > > V3: > > * Corrected commit log > > V2: > > * no change in this patch > > > > drivers/pci/host/pci-tegra.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > As far as I can tell, this is a bugfix that can and should be applied > independently of the rest of the series. > > Acked-by: Thierry Reding <treding@nvidia.com> Ok, I will apply - I would add a Fixes: tag though. Is: Fixes: cf5d31801278 ("PCI: tegra: Program PADS_REFCLK_CFG* always, not just on legacy SoCs") the correct commit ? Thanks, Lorenzo
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c index b41c60c7414c..068510b40c1a 100644 --- a/drivers/pci/host/pci-tegra.c +++ b/drivers/pci/host/pci-tegra.c @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port) static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) { struct device *dev = pcie->dev; - const struct tegra_pcie_soc *soc = pcie->soc; struct tegra_pcie_port *port; int err; @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie) } } - /* Configure the reference clock driver */ - pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); - - if (soc->num_ports > 2) - pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); - return 0; } @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) return 0; } +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie) +{ + const struct tegra_pcie_soc *soc = pcie->soc; + + /* Configure the reference clock driver */ + pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0); + + if (soc->num_ports > 2) + pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1); +} + /* * FIXME: If there are no PCIe cards attached, then calling this function * can result in the increase of the bootup time as there are big timeout @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) struct device *dev = pcie->dev; struct tegra_pcie_port *port, *tmp; + tegra_pcie_apply_pad_settings(pcie); + list_for_each_entry_safe(port, tmp, &pcie->ports, list) { dev_info(dev, "probing port %u, using %u lanes\n", port->index, port->lanes);
In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped in driver. REFCLK pad settings are independent of PHY and should be programmed by driver. So move REFCLK pad settings out of phy_power_on(). These pad settings improve REFCLK peak to peak amplitude. Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> --- V3: * Corrected commit log V2: * no change in this patch drivers/pci/host/pci-tegra.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)