Date:   Mon, 16 Jan 2017 22:44:37 +0100
From:   Lukasz Majewski <lukma@...x.de>
To:     Joao Pinto <Joao.Pinto@...opsys.com>
Cc:     Kishon Vijay Abraham I <kishon@...com>,
        "jingoohan1@...il.com" <jingoohan1@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        <linux-omap@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

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@...x.de>
> >>>>>
> >>>>> 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?

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

> b) Clear "Directed Speed Change"

CFG_DIRECTED_SPEED_CHANGE @ 0x5180 080C 

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

> 
> 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@...x.de

Powered by blists - more mailing lists