Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Joao Pinto]

Hello,

Ãs 11:38 AM de 1/17/2017, Lukasz Majewski escreveu:
> Hi Joao,
> 
> Thank you for your reply.
> 
>> Ãs 10:43 AM de 1/17/2017, Joao Pinto escreveu:
>>>
>>> Hi Lukasz,
>>>
>>> Ãs 9:44 PM de 1/16/2017, Lukasz Majewski escreveu:
>>>> Hi Joao,
>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Ãs 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu:
>>>>>> + Joao, Jingoo
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote:
>>>>>>> Hi Kishon,
>>>>>>>
>>>>>>>> Hi Åukasz,
>>>>>>>>
>>>>>>>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote:
>>>>>>>>> Hi Kishon,
>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote:
>>>>>>>>>>> Some devices (due to e.g. bad PCIe signal integrity)
>>>>>>>>>>> require to run with forced GEN1 speed on PCIe bus.
>>>>>>>>>>>
>>>>>>>>>>> This patch changes the speed explicitly on dra7 based
>>>>>>>>>>> devices when proper device tree attribute is defined for
>>>>>>>>>>> the PCIe controller.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Lukasz Majewski <lukma@xxxxxxx>
>>>>>>>>>>
>>>>>>>>>> Bjorn has already queued a patch to do the same thing
>>>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= 
>>>>>>>>>
>>>>>>>>> It seems like Bjorn only modifies CAP registers.
>>>>>>>>
>>>>>>>> The patch also modifies the LNKCTL2 register.
>>>>>>>>>
>>>>>>>>> He also needs to change register with 0x080C offset to
>>>>>>>>> actually ( PCIECTRL_PL_WIDTH_SPEED_CTL )
>>>>>>>>
>>>>>>>> This bit is used to initiate speed change (after the link is
>>>>>>>> initialized in GEN1). Resetting the bit (like what you have
>>>>>>>> done here) prevents speed change.
>>>>>>>
>>>>>>> This is strange, but e2e advised me to do things as I did in the
>>>>>>> patch to _force_ GEN1 operation on PCIe2 port [1] (AM5728)
>>>>>>>
>>>>>>> Link:
>>>>>>> [1]
>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= 
>>>>>>>
>>>>>>> Both patches modify 0x5180 007C register to set GEN1 capability
>>>>>>> (PCI_EXP_LNKCAP_SLS_2_5GB)
>>>>>>>
>>>>>>> The problem is with second register (in your patch):
>>>>>>>
>>>>>>> From SPRUHZ6G TRM:
>>>>>>>
>>>>>>> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0)
>>>>>>> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more
>>>>>>>   description in TRM
>>>>>>>
>>>>>>> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same
>>>>>>> as default /reset value.
>>>>>>
>>>>>> The default value is 0x2 (or else none of the cards would have
>>>>>> enumerated in GEN2)
>>>>>>>
>>>>>>>
>>>>>>> Could you clarify which way to _force_ PCIe GEN1 operation is
>>>>>>> correct? Mine shows differences in lspci output (as posted in
>>>>>>> [1]).
>>>>>>
>>>>>> You'll see the difference even with the patch in Bjorn's tree ;-)
>>>>>>
>>>>>> I think these are 2 different approaches to keep the link at
>>>>>> GEN1. Joao or Jingoo, do you have any suggestion here?
>>>>>
>>>>> I studied the Databook,
>>>>
>>>> Could you reveal which databook do you have in mind? Is that the
>>>> TRM for AM5728?
>>>
>>> I checked the Designware PCIe Databook, since it is based on this
>>> IP.
>>>
>>>>
>>>>> and both approaches seem to be right,
>>>>> dependently of the Core configuration and setup.
>>>>>
>>>>> The standard manual speed change sequence is:
>>>>> a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed)
>>>>
>>>> Do you mean TRGT_LINK_SPEED @ 0x5180 00A0 ?
>>>
>>> Correct.
>>>
>>>>
>>>>> b) Clear "Directed Speed Change"
>>>>
>>>> CFG_DIRECTED_SPEED_CHANGE @ 0x5180 080C 
>>>
>>> Correct.
>>>
>>>>
>>>>> c) Set "Directed Speed Change"
>>>>>
>>>>> If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is
>>>>> the default value), it will execute LTSSM to initiate speed
>>>>> change to Gen2 or Gen3, after link is started in Gen1, and then
>>>>> the bit is automatically cleared.
>>>>
>>>> Ok, so with default settings (after reset) we do have Gen1 speed
>>>> link and when we enable LTSSM (with LTSSM_EN bit setting) we
>>>> negotiate to Gen2/Gen3.
>>>
>>> Yes, that's the expected behavior. I submited this direct question
>>> to R&D and will have your doubt answered soon.
>>
>> According to R&D if you set "Target Link Speed" to Gen1 before
>> setting LTSSM_EN bit, the controller should stay in GEN1.
> 
> I assume that this is the "recommended" and most robust possible
> approach?
> 
> And the patch already submitted to ML is 100% correct (so I don't need
> to clear PCIECTRL_PL_WIDTH_SPEED_CTL) ?
> 

Yes, according to R&D the approach available in Bjorn' tree should be ok, since
it sets GEN1 before enabling LTSSM. Of course this is from a standard designware
PCie RC ocre perspective.

Joao

> Our problem has been described here:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_p_567936_2081573-232081573&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=0i9CSBYPpoLydd2AtYg7xapHfGpCvlyir1etY0ZbIwY&s=6f24idB3Pa7aqJEpfsuMpGxkyFUwWwyOFwQtnDztWLA&e= 
> 
> Best regards,
> Åukasz Majewski
> 
>>
>>>
>>>>
>>>>>
>>>>> Lukasz is reseting this bit, in order to avoid the LTSSM to be
>>>>> executed, which is correct. 
>>>>
>>>> So with CFG_DIRECTED_SPEED_CHANGE = 0, when I start LTSSM (with
>>>> LTSSM_EN) the state machine returns immediately and leaves link in
>>>> the Gen1?
>>>>
>>>> The pci-dra7 driver always sets LTSSM_EN bit to start link
>>>> negotiation.
>>>>
>>>>> There is another way to prevent this
>>>>> automatic speed change, which is to set GEN1 speed before link up
>>>>> which might be difficult in some setups, so Kishon's also right.
>>>>>
>>>>> In my opinion Lukasz approach would be the one that might be more
>>>>> universal and more "secure".
>>>>
>>>> The robustness is the key here since there are some devices, which
>>>> on particular HW must only work with Gen1 speed. When we start
>>>> LTSSM state machine and hence start negotiation to Gen2, not
>>>> always the result of LTSSM is correct and device is properly
>>>> recognized.
>>>>
>>>>>
>>>>> Joao
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> IMO the better way is to set the LNKCTL2 to GEN1 instead of
>>>>>>>> hacking the IP register.
>>>>>>>
>>>>>>> From the original patch description:
>>>>>>>
>>>>>>> "Add support to force Root Complex to work in GEN1 mode if so
>>>>>>> desired, but don't force GEN1 mode on any board just yet."
>>>>>>>
>>>>>>> Are there any (floating around) patches allowing forcing GEN1
>>>>>>> operation on any board (I would like to reuse/port them to my
>>>>>>> current solution)?
>>>>>>
>>>>>> For setting to GEN1 mode, "max-link-speed" should be set to 1 in
>>>>>> dt with the patch in Bjorn's tree.
>>>>>>
>>>>>> Thanks
>>>>>> Kishon
>>>>>>
>>>>>
>>>>
>>>> Best regards,
>>>>
>>>> Lukasz Majewski
>>>>
>>>> --
>>>>
>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
>>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>>>> Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
>>>> wd@xxxxxxx
>>>>
>>>
>>
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@xxxxxxx
>