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 > -- 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
