Re: [PATCH v6 4/5] PCI: add PCI controller for keystone PCIe h/w

[Murali Karicheri <m-karicheri2@ti.com>]

On 07/22/2014 11:41 AM, Rob Herring wrote:
> On Mon, Jul 21, 2014 at 11:39 AM, Murali Karicheri<m-karicheri2@ti.com>  wrote:
>> On 07/20/2014 09:44 PM, Jingoo Han wrote:
>>> On Saturday, July 19, 2014 5:29 AM, Murali Karicheri wrote:
>>>> On 07/18/2014 03:31 PM, Rob Herring wrote:
>>>>> On Fri, Jul 18, 2014 at 10:14 AM, Murali Karicheri<m-karicheri2@ti.com>
>>>>> wrote:
>>>> --- Cut ---
>>>>>> +
>>>>>> +Optional properties:-
>>>>>> +       phys: phandle to Generic Keystone SerDes phy for PCI
>>>>>> +       phy-names: name of the Generic Keystine SerDes phy for PCI
>>>>>> +         - If boot loader already does PCI link establishment, then
>>>>>> phys and
>>>>>> +           phy-names shouldn't be present.
>>>>>> +       ti,enable-linktrain - Enable Link training.
>>>>>> +         - If boot loader already does PCI link establishment, then
>>>>>> this
>>>>>> +           shouldn't be present.
>>>>>
>>>>> Can't you read from the h/w if the link is trained?
>>>
>>> I agree with Rob Herring's suggestion.
>>>
>>>> Yes.
>>>>
>>>> There are customers who use this driver with PCI Link setup done in the
>>>> boot loader. This property tells the driver to bypass Link setup
>>>> procedure in that case. Is this undesirable and if so. how other
>>>> platforms handle it? Check first if link is trained or start the link
>>>> setup procedure? Let me know. If this is fine, please provide your Ack.
>>>
>>> Please, check the following code of Exynos PCIe diver.
>>>
>>> ./drivers/pci/host/pci-exynos.c
>>>
>>> static int exynos_pcie_establish_link(struct pcie_port *pp)
>>> {
>>>          struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
>>>          void __iomem *elbi_base = exynos_pcie->elbi_base;
>>>          void __iomem *pmu_base = exynos_pcie->pmu_base;
>>>
>>>          if (dw_pcie_link_up(pp)) {
>>>                  dev_err(pp->dev, "Link already up\n");
>>>                  return 0;
>>>          }
>>>          .....
>>>
>>> In the case of Exynos PCIe, the Exynos PCIe driver checks the
>>> h/w bit such as PCIE_ELBI_LTSSM_ENABLE bit of PCIE_ELBI_RDLH_LINKUP
>>> offset register.
>>>
>>> If the link is already set up by the boot loader or other reasons,
>>> the driver will skip some initialization codes.
>>>
>>> The first step is that you find such h/w bit for checking link up.
>>> If so, please add the code for skipping, when the link is already
>>> set up.
>>>
>> Rob, Jingoo,
>>
>> We have similar bit to check for Link status and I have removed the DT
>> property and skip Link retrain if PCIe Link is already Up. I will be
>> resending the series with Patch 4/5 updated.
> For both keystone and exynos, is this status bit in the phy? If so,
> sounds like this should be a standard phy function op.
>
> Rob
Rob,

There is phy bit as well as PCIe specific Link LTSSM state for Keystone.
Phy one is handled by the Phy driver and PCie driver care about LTSSM
state bit. I have removed the enable_link_train DT attribute from
patch 4/5. Can you ack it now?

Murali