Re: [PATCH V2 2/4] PCI: tegra: Add Tegra186 PCIe support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 18, 2017 at 07:44:12PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 18-Oct-17 6:57 PM, Bjorn Helgaas wrote:
> > On Tue, Oct 17, 2017 at 10:27:49PM +0200, Thierry Reding wrote:
> >> On Tue, Oct 17, 2017 at 12:34:28PM -0500, Bjorn Helgaas wrote:
> >>> On Wed, Sep 27, 2017 at 05:28:35PM +0530, Manikanta Maddireddy wrote:
> >>>> UPHY programming is performed by BPMP, PHY enable calls are
> >>>> not required for Tegra186 PCIe. Power partition ungate is
> >>>> done by BPMP powergate driver.
> >>>>
> >>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx>
> >>>> Reviewed-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> >>>> Tested-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> >>>> ---
> >>>> V2: Add soc->program_uphy check for phy_exit call
> >>>>  drivers/pci/host/pci-tegra.c | 132 +++++++++++++++++++++++++++++++++++--------
> >>>>  1 file changed, 108 insertions(+), 24 deletions(-)
> >>>
> >>>> @@ -1012,10 +1016,12 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> >>>>  		afi_writel(pcie, value, AFI_FUSE);
> >>>>  	}
> >>>>  
> >>>> -	err = tegra_pcie_phy_power_on(pcie);
> >>>> -	if (err < 0) {
> >>>> -		dev_err(dev, "failed to power on PHY(s): %d\n", err);
> >>>> -		return err;
> >>>> +	if (soc->program_uphy) {
> >>>> +		err = tegra_pcie_phy_power_on(pcie);
> >>>> +		if (err < 0) {
> >>>> +			dev_err(dev, "failed to power on PHY(s): %d\n", err);
> >>>> +			return err;
> >>>> +		}
> >>>
> >>> This looks good: it's obvious that the previously-supported devices
> >>> continue to work the same way because you set ".program_uphy = true"
> >>> for them.  (It would be even more obvious if you changed the sense so
> >>> that only the new device had to add this initializaer, but in general
> >>> I prefer the positive logic as you have here, and I did verify that
> >>> you added the initializer for all tegra_pcie_soc variants.)
> >>>
> >>>> @@ -1048,19 +1054,23 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> >>>>  static void tegra_pcie_power_off(struct tegra_pcie *pcie)
> >>>>  {
> >>>>  	struct device *dev = pcie->dev;
> >>>> +	const struct tegra_pcie_soc *soc = pcie->soc;
> >>>>  	int err;
> >>>>  
> >>>>  	/* TODO: disable and unprepare clocks? */
> >>>>  
> >>>> -	err = tegra_pcie_phy_power_off(pcie);
> >>>> -	if (err < 0)
> >>>> -		dev_err(dev, "failed to power off PHY(s): %d\n", err);
> >>>> +	if (soc->program_uphy) {
> >>>> +		err = tegra_pcie_phy_power_off(pcie);
> >>>> +		if (err < 0)
> >>>> +			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);
> >>>>  
> >>>> -	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
> >>>> +	if (!dev->pm_domain)
> >>>> +		tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
> >>>
> >>> But this one isn't obvious because nothing in your patch obviously
> >>> affects dev->pm_domain, so I can't tell whether this is safe.  It's
> >>> worth a changelog comment to help justify this.
> >>
> >> The last sentence in the commit message is what this is about. Maybe it
> >> should be more verbose:
> >>
> >> 	Since the BPMP controls the powergates on Tegra186, there is no
> >> 	need to manually power on and off the PCIe power partition. The
> >> 	BPMP generic power domain driver takes care of it instead.
> > 
> > If you know Tegra intimately, maybe that clarifies it.  I don't, so my
> > problem is that there's nothing in the patch that helps me verify
> > this.  I infer that maybe there's something different in the DT that
> > means dev->pm_domain will be set for Tegra186, but not for other
> > variants?
> > 
> > There should be something that helps the reader connect the dots.
> 
> Till tegra210 pmc driver provides direct powergate power ON/OFF functions.
> 
> In tegra186.dtsi power-domains property is added in pcie@10003000 DT node. reference: https://patchwork.ozlabs.org/patch/819113/
> powergate-bpmp.c is the pm domain provider for pcie powergate. 
> Platform driver will look for pm domain provider of Tegra pcie host driver and sets dev->pm_domains.
> powergate-bpmp.c driver registers powergate power ON/OFF functions to generic PD power_on/powerr_off callback functions.
> Generic power domain will take care of calling power_on before Tegra PCIe probe.
> 
> So in this patch I used dev->pm_domain to skip pmc driver calls.

Thanks, I updated the changelog as follows:


commit 9cea513d8cbc75ee26327d3d8971fe7b58288d8f
Author: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx>
Date:   Wed Sep 27 17:28:35 2017 +0530

    PCI: tegra: Add Tegra186 PCIe support
    
    Add Tegra186 PCIe support.  UPHY programming is performed by BPMP; PHY
    enable calls are not required for Tegra186 PCIe.
    
    Power partition ungate is done by BPMP powergate driver.  The Tegra186
    DT description must include a "power-domains" property, which results in
    dev->pm_domain being set.



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux