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@xxxxxxxxxx>
> Cc: Joao Pinto <Joao.Pinto@xxxxxxxxxxxx>
> Reviewed-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Signed-off-by: Trent Piepho <tpiepho@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
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
>
