Re: [PATCH] PCI: designware: set PORT_LOGIC_SPEED_CHANGE before linkup

[Zhou Wang <wangzhou1@hisilicon.com>]

On 2015/6/3 3:37, Bjorn Helgaas wrote:
> [+cc Richard, Lucas for imx6 questions below]
> 
> On Tue, Jun 02, 2015 at 10:14:33AM +0800, Zhou Wang wrote:
>> When connected some PCIe3.0 cards(e.g. LSI 2208 PCIe-RAID card, Mellanox IB card),
>> It will appear link unstable which will lead reading/writing fail.
>>
>> Here just move the setting of PORT_LOGIC_SPEED_CHANGE bit before starting
>> building link. After doing this, it will work fine with above PCIe3.0 card.
>>
>> This patch is based on v4.1-rc4
>>
>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
>> ---
>>  drivers/pci/host/pcie-designware.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>> index 2e9f84f..64ebc51 100644
>> --- a/drivers/pci/host/pcie-designware.c
>> +++ b/drivers/pci/host/pcie-designware.c
>> @@ -498,10 +498,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>  	/* program correct class for RC */
>>  	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>>  
>> -	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
>> -	val |= PORT_LOGIC_SPEED_CHANGE;
>> -	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
>> -
>>  #ifdef CONFIG_PCI_MSI
>>  	dw_pcie_msi_chip.dev = pp->dev;
>>  	dw_pci.msi_ctrl = &dw_pcie_msi_chip;
>> @@ -797,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>  	}
>>  	dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL);
>>  
>> +	dw_pcie_readl_rc(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, &val);
>> +	val |= PORT_LOGIC_SPEED_CHANGE;
>> +	dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL);
> 
> It makes sense to me to do this at the same place we do the other write to
> PCIE_LINK_WIDTH_SPEED_CONTROL.
> 
> 1) Can this be combined with the previous write, so we only need a single
>    write to PCIE_LINK_WIDTH_SPEED_CONTROL, e.g., something like this?
> 
>     switch (pp->lanes) {
>     case 1:
>         val |= PORT_LOGIC_LINK_WIDTH_1_LANES;
>         ...
>     }
>     val |= PORT_LOGIC_SPEED_CHANGE;
>     dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL);
> 

I think it is OK to put them together.

> 2) imx6_pcie_start_link() also writes PCIE_LINK_WIDTH_SPEED_CONTROL.  It
>    has a loop to wait for the speed change to finish.  Does
>    dw_pcie_setup_rc() need a similar loop?

imx6 has a loop there to make sure link is stable, because enumeration will start
soon. Also in other drivers which are based on pcie-designware, we should check
if speed change is done before enumeration. In pcie-designware, now it does not
make this check.

Maybe we may not move PORT_LOGIC_SPEED_CHANGE set, but add a loop check after it
to make sure the link is stable. Then related same setting in imx6 can be deleted.

I wonder if other PCIe hosts which is based on DW have met the same problem?

> 
> 3) Since you are moving the PORT_LOGIC_SPEED_CHANGE write from
>    dw_pcie_host_init() to dw_pcie_setup_rc(), it will now happen before
>    imx6_pcie_start_link().  Does that mean we can remove the write from
>    imx6_pcie_start_link() completely?
> 

I think so.

Best Regards,
Zhou

>>  	/* setup RC BARs */
>>  	dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0);
>>  	dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1);
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
>