Re: [PATCH v4 2/2] PCI: tegra: Support per-lane PHYs

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

On Fri, Apr 08, 2016 at 01:05:28PM -0500, Bjorn Helgaas wrote:
> Hi Thierry,
> 
> I think there are a couple typos (one in a message and one that
> actually looks important), and one question below.
> 
> On Fri, Apr 08, 2016 at 06:13:14PM +0200, Thierry Reding wrote:
[...]
> > +static int tegra_pcie_phy_power_off(struct tegra_pcie *pcie)
> > +{
> > +	struct tegra_pcie_port *port;
> > +	int err;
> > +
> > +	if (pcie->legacy_phy) {
> > +		if (pcie->phy)
> > +			err = phy_power_on(pcie->phy);
> 
> s/phy_power_on/phy_power_off/

Good catch. Fixed.

> > @@ -899,13 +1025,9 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> >  		afi_writel(pcie, value, AFI_FUSE);
> >  	}
> >  
> > -	if (!pcie->phy)
> > -		err = tegra_pcie_phy_enable(pcie);
> > -	else
> > -		err = phy_power_on(pcie->phy);
> > -
> > +	err = tegra_pcie_phy_power_on(pcie);
> >  	if (err < 0) {
> > -		dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
> > +		dev_err(pcie->dev, "failed to power off PHY(s): %d\n", err);
> 
> s/off/on/

Fixed.

> > +static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
> > +{
> > +	struct tegra_pcie_port *port;
> > +	int err;
> > +
> > +	if (of_get_property(pcie->dev->of_node, "phys", NULL) != NULL)
> > +		return tegra_pcie_phys_get_legacy(pcie);
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		err = tegra_pcie_port_get_phys(port);
> > +		if (err < 0) {
> > +			return err;
> > +		}
> > +	}
> 
> This seems backwards: if I'm reading this right, you first check for
> the legacy property ("phys") and use it if you find it.  If there is
> no legacy property, you look for the new per-lane PHYs.
> 
> The usual pattern would be "look for the new stuff, and if you don't
> find it, fall back to the old stuff." Is there a configuration that
> could be described either way, e.g., something with only one lane and
> only one PHY?
> 
> I'm not sure whether it matters, but if it *could* use the "new, fall
> back to old" pattern, that would be nice and would keep people from
> wondering whether it's safe to do it backwards.

The reason why I wrote it this way is to special case the legacy code.
The alternative would be to do:

	if (of_get_property(...) == NULL) {
		list_for_each_entry(port, &pcie->ports, list) {
			err = tegra_pcie_port_get_phys(port);
			if (err < 0)
				return err;
		}
	}

	return tegra_pcie_phys_get_legacy(pcie);

Which is better in the way you describe (fall back to legacy if new
binding is not found). But like I said, it makes, from the code flow,
the new binding the exception, which looks odd to me as well.

Perhaps this could be somewhat mitigated by wrapping the new code into
a separate function:

	if (of_get_property(...) == NULL)
		return tegra_pcie_phys_get_per_lane(pcie);

	return tegra_pcie_phys_get_legacy(pcie);

I don't feel very strongly in either direction. Do you have a
preference?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]