[v2] PCI: imx6: Check for link training status in link up check

This fixes a regression introduced in merge 562df5c8521e.

Prior to this the link up check done by imx6_pcie_wait_for_link()
consisted of a polling loop on imx6_pcie_link_up() (via the former
calling dw_pcie_link_up() which called the latter as callback), and
imx6_pcie_link_up() polled the link status register checking for link
up *and link not still training*.

This was a polling loop inside another polling loop.  And the outermost
loop was duplicated with minor variations in a number of other dwc based
host drivers.

This was addressed in two commits.  Commit 4d107d3b5a68 ("PCI: imx6: Move
link up check into imx6_pcie_wait_for_link()"), changed
imx6_pcie_wait_for_link() to poll the link status register directly,
checking for link up and not training, and made imx6_pcie_link_up() only
check the link up bit (once, not a polling loop).

While commit commit 886bc5ceb5cc ("PCI: designware: Add generic
dw_pcie_wait_for_link()"), replaced the loop in imx6_pcie_wait_for_link()
with a call to a new dwc core function, which polled imx6_pcie_link_up(),
which still checked both link up and not training in a loop.

When these two commits were merged, the version of
imx6_pcie_wait_for_link() from '886 was kept, which eliminated the link
training check placed there by '4d1.  But the version of
imx6_pcie_link_up() from '4d1 was kept, which eliminated the link training
check that had been there and was moved to imx6_pcie_wait_for_link().

There result is no link training check.

Then commit dac29e6c5460 ("PCI: designware: Add default link up check if
sub-driver doesn't override") added a default check into
dw_pcie_link_up(), which could have been used by imx6, but wasn't.  Then
commit 01c076732e82 ("PCI: designware: Check LTSSM training bit before
deciding link is up") added a link training check to the default from
'dac, but this code was still not used by imx6.

This commit eliminates imx6_pcie_link_up() so that the default
dw_pcie_link_up() is used.  The default has the correct code and is what
the imx6 driver used to do.

Fixes: 562df5c8521e1371f3cbd0b7b868034da376d714
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Joao Pinto <Joao.Pinto@synopsys.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

On Wed, Oct 31, 2018 at 07:49:59PM +0000, Trent Piepho wrote:
> This fixes a regression introduced in merge 562df5c8521e.

A merge being a commits collection, the regression was certainly
introduced by a specific commit in it, not the merge itself.

Please remove this line and add a proper Fixes: tag below.

Please use the canonical commit format:

git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h
(\"%s\")%n" SHA-1 ID

Also for the Fixes: tag and all references.

> Prior to this the link up check done by imx6_pcie_wait_for_link()
> consisted of a polling loop on imx6_pcie_link_up() (via the former
> calling dw_pcie_link_up() which called the latter as callback), and
> imx6_pcie_link_up() polled the link status register checking for link
> up *and link not still training*.
> 
> This was a polling loop inside another polling loop.  And the outermost
> loop was duplicated with minor variations in a number of other dwc based
> host drivers.
> 
> This was addressed in two commits.  Commit 4d107d3b5a68 ("PCI: imx6: Move
> link up check into imx6_pcie_wait_for_link()"), changed
> imx6_pcie_wait_for_link() to poll the link status register directly,
> checking for link up and not training, and made imx6_pcie_link_up() only
> check the link up bit (once, not a polling loop).
> 
> While commit commit 886bc5ceb5cc ("PCI: designware: Add generic

One "commit" is enough.

> dw_pcie_wait_for_link()"), replaced the loop in imx6_pcie_wait_for_link()
> with a call to a new dwc core function, which polled imx6_pcie_link_up(),
> which still checked both link up and not training in a loop.
> 
> When these two commits were merged, the version of
> imx6_pcie_wait_for_link() from '886 was kept, which eliminated the link
> training check placed there by '4d1.  But the version of
> imx6_pcie_link_up() from '4d1 was kept, which eliminated the link training
> check that had been there and was moved to imx6_pcie_wait_for_link().
> 
> There result is no link training check.
> 
> Then commit dac29e6c5460 ("PCI: designware: Add default link up check if
> sub-driver doesn't override") added a default check into
> dw_pcie_link_up(), which could have been used by imx6, but wasn't.  Then
> commit 01c076732e82 ("PCI: designware: Check LTSSM training bit before
> deciding link is up") added a link training check to the default from
> 'dac, but this code was still not used by imx6.
> 
> This commit eliminates imx6_pcie_link_up() so that the default
> dw_pcie_link_up() is used.  The default has the correct code and is what
> the imx6 driver used to do.

Write one sentence in imperative form, eg "Eliminate imx6_pcie_link_up()..",
see Bjorn's guidelines below.

> Fixes: 562df5c8521e1371f3cbd0b7b868034da376d714

Report the commit you are fixing, not the merge commit and report it
in canonical format.

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Joao Pinto <Joao.Pinto@synopsys.com>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

This commit log is a tad long, I would appreciate if you tried to
summarize it, I can do it for you if you wish so.

Please read:

https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/

You should CC at least what

get_maintainer.pl -f drivers/pci/controller/dwc/pci-imx6.c

reports.

Thanks,
Lorenzo

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 4a9a673b4777..975050a69494 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -80,8 +80,6 @@ struct imx6_pcie {
>  #define PCIE_PL_PFLR_FORCE_LINK			(1 << 15)
>  #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
>  #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
> -#define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING	(1 << 29)
> -#define PCIE_PHY_DEBUG_R1_XMLH_LINK_UP		(1 << 4)
>  
>  #define PCIE_PHY_CTRL (PL_OFFSET + 0x114)
>  #define PCIE_PHY_CTRL_DATA_LOC 0
> @@ -641,12 +639,6 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
>  	return 0;
>  }
>  
> -static int imx6_pcie_link_up(struct dw_pcie *pci)
> -{
> -	return dw_pcie_readl_dbi(pci, PCIE_PHY_DEBUG_R1) &
> -			PCIE_PHY_DEBUG_R1_XMLH_LINK_UP;
> -}
> -
>  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
>  	.host_init = imx6_pcie_host_init,
>  };
> @@ -679,7 +671,7 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
>  }
>  
>  static const struct dw_pcie_ops dw_pcie_ops = {
> -	.link_up = imx6_pcie_link_up,
> +	/* No special ops needed, but pcie-designware still expects this struct */
>  };
>  
>  static int imx6_pcie_probe(struct platform_device *pdev)
> -- 
> 2.14.4
>

On Thu, 2018-11-01 at 13:55 +0000, Lorenzo Pieralisi wrote:
> On Wed, Oct 31, 2018 at 07:49:59PM +0000, Trent Piepho wrote:
> > This fixes a regression introduced in merge 562df5c8521e.
> 
> A merge being a commits collection, the regression was certainly
> introduced by a specific commit in it, not the merge itself.

In this case it really is merge commit.

While the problem and fix are relatively obvious, finding how it came
to be broken was a challenge.  Most of the commit message is my proof
that this is a bug and not something done intentionally, by tracking
back the complicated route that caused the code to be in its current
state.

Basically there are two commits, on both branches of the merge, neither
of which caused a problem nor were they incorrect in any way at the
time they were committed.  When that merge combined this collection of
multiple commits, it did not do so correctly and that is the point a
bug appeared.

Or put another way, this problem was already fixed some time ago, but
in the merge commit the fix was dropped.

It seems like the proper way to attribute my fix is to the merge, as
that is what caused the regression.  There is no prior commit where one
can observe the regression.

> Also for the Fixes: tag and all references.

Would it be ok to refer to the commit this way the first time, then use
a shortened method for subsequent usages?  Otherwise talking about two
commits becomes very long.  This it what I have mostly done, other than
the "Fixes" line.  I'm surprised checkpatch didn't catch that.

On Fri, Nov 02, 2018 at 12:23:12AM +0000, Trent Piepho wrote:
> On Thu, 2018-11-01 at 13:55 +0000, Lorenzo Pieralisi wrote:
> > On Wed, Oct 31, 2018 at 07:49:59PM +0000, Trent Piepho wrote:
> > > This fixes a regression introduced in merge 562df5c8521e.
> > 
> > A merge being a commits collection, the regression was certainly
> > introduced by a specific commit in it, not the merge itself.
> 
> In this case it really is merge commit.
> 
> While the problem and fix are relatively obvious, finding how it came
> to be broken was a challenge.  Most of the commit message is my proof
> that this is a bug and not something done intentionally, by tracking
> back the complicated route that caused the code to be in its current
> state.
> 
> Basically there are two commits, on both branches of the merge, neither
> of which caused a problem nor were they incorrect in any way at the
> time they were committed.  When that merge combined this collection of
> multiple commits, it did not do so correctly and that is the point a
> bug appeared.
> 
> Or put another way, this problem was already fixed some time ago, but
> in the merge commit the fix was dropped.
> 
> It seems like the proper way to attribute my fix is to the merge, as
> that is what caused the regression.  There is no prior commit where one
> can observe the regression.

You are right, I went through git history and I agree with your
summary, that's what happened.

> > Also for the Fixes: tag and all references.
> 
> Would it be ok to refer to the commit this way the first time, then use
> a shortened method for subsequent usages?  Otherwise talking about two
> commits becomes very long.  This it what I have mostly done, other than
> the "Fixes" line.  I'm surprised checkpatch didn't catch that.

I would like to ask Bjorn's opinion on this, I do not know if adding
a Fixes: tag with a merge commit in it is common practice but that
summarizes what happened so I assume it should be fine.

As for the shortened commit format I understand your point, I do not
know if that's "official" from a kernel process standpoint.

Lorenzo

On Fri, Nov 02, 2018 at 12:21:13PM +0000, Lorenzo Pieralisi wrote:
> On Fri, Nov 02, 2018 at 12:23:12AM +0000, Trent Piepho wrote:
> > On Thu, 2018-11-01 at 13:55 +0000, Lorenzo Pieralisi wrote:
> > > On Wed, Oct 31, 2018 at 07:49:59PM +0000, Trent Piepho wrote:
> > > > This fixes a regression introduced in merge 562df5c8521e.
> > > 
> > > A merge being a commits collection, the regression was certainly
> > > introduced by a specific commit in it, not the merge itself.
> > 
> > In this case it really is merge commit.
> > 
> > While the problem and fix are relatively obvious, finding how it came
> > to be broken was a challenge.  Most of the commit message is my proof
> > that this is a bug and not something done intentionally, by tracking
> > back the complicated route that caused the code to be in its current
> > state.
> > 
> > Basically there are two commits, on both branches of the merge, neither
> > of which caused a problem nor were they incorrect in any way at the
> > time they were committed.  When that merge combined this collection of
> > multiple commits, it did not do so correctly and that is the point a
> > bug appeared.
> > 
> > Or put another way, this problem was already fixed some time ago, but
> > in the merge commit the fix was dropped.
> > 
> > It seems like the proper way to attribute my fix is to the merge, as
> > that is what caused the regression.  There is no prior commit where one
> > can observe the regression.
> 
> You are right, I went through git history and I agree with your
> summary, that's what happened.
> 
> > > Also for the Fixes: tag and all references.
> > 
> > Would it be ok to refer to the commit this way the first time, then use
> > a shortened method for subsequent usages?  Otherwise talking about two
> > commits becomes very long.  This it what I have mostly done, other than
> > the "Fixes" line.  I'm surprised checkpatch didn't catch that.
> 
> I would like to ask Bjorn's opinion on this, I do not know if adding
> a Fixes: tag with a merge commit in it is common practice but that
> summarizes what happened so I assume it should be fine.

That sounds good to me.

> As for the shortened commit format I understand your point, I do not
> know if that's "official" from a kernel process standpoint.

It took me a while to figure out what '886 meant because it's unusual and
the "'" usually goes where the *missing* characters are.  Personally I
would use the full 

  886bc5ceb5cc ("PCI: designware: Add generic dw_pcie_wait_for_link()")

at the first reference and just 886bc5ceb5cc for subsequent references.

Thanks for chasing this down!  It's really a hassle to figure out things
that work separately but not together.

Bjorn