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) ? Our problem has been described here: https://e2e.ti.com/support/arm/sitara_arm/f/791/p/567936/2081573#2081573 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
