Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver

[Kishon Vijay Abraham I]

Hi Martin,

On 28/08/19 2:08 AM, Martin Blumenstingl wrote:
> Hello,
> 
> On Tue, Aug 27, 2019 at 5:09 AM Chuan Hua, Lei
> <chuanhua.lei@xxxxxxxxxxxxxxx> wrote:
>>
>> Hi Martin,
>>
>> Thanks for your feedback. Please check the comments below.
>>
>> On 8/27/2019 5:15 AM, Martin Blumenstingl wrote:
>>> Hello,
>>>
>>> On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei
>>> <chuanhua.lei@xxxxxxxxxxxxxxx> wrote:
>>>> Hi Martin,
>>>>
>>>> Thanks for your valuable comments. I reply some of them as below.
>>> you're welcome
>>>
>>> [...]
>>>>>> +config PCIE_INTEL_AXI
>>>>>> +        bool "Intel AHB/AXI PCIe host controller support"
>>>>> I believe that this is mostly the same IP block as it's used in Lantiq
>>>>> (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010
>>>>> (before Intel acquired Lantiq).
>>>>> This is why I would have personally called the driver PCIE_LANTIQ
>>>> VRX200 SoC(internally called VR9) was the first PCIe SoC product which
>>>> was using synopsys
>>>>
>>>> controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal
>>>> phy from infineon.
>>> thank you for these details
>>> I wasn't aware that the PCIe PHY on these SoCs was developed by
>>> Infineon nor is the DWC version documented anywhere
>>
>> VRX200/ARX300 PHY is internal value. There are a lot of hardcode which was
>> from hardware people. From XRX500, we switch to synopsis PHY. However, later
>> comboPHY is coming to the picture. Even though we have one same controller
>> with different versions, we most likely will have three different phy
>> drivers.
> that is a good argument for using a separate PHY driver and
> integrating that using the PHY subsystem (which is already the case in
> this patch revision)
> 
.
.
<snip>
>>>>>> +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp)
>>>>>> +{
>>>>>> +    struct device *dev = lpp->pci->dev;
>>>>>> +    int ret = 0;
>>>>>> +
>>>>>> +    lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>>>>>> +    if (IS_ERR(lpp->reset_gpio)) {
>>>>>> +            ret = PTR_ERR(lpp->reset_gpio);
>>>>>> +            if (ret != -EPROBE_DEFER)
>>>>>> +                    dev_err(dev, "failed to request PCIe GPIO: %d\n", ret);
>>>>>> +            return ret;
>>>>>> +    }
>>>>>> +    /* Make initial reset last for 100ms */
>>>>>> +    msleep(100);
>>>>> why is there lpp->rst_interval when you hardcode 100ms here?
>>>> There are different purpose. rst_interval is purely for asserted reset
>>>> pulse.
>>>>
>>>> Here 100ms is to make sure the initial state keeps at least 100ms, then we
>>>> can reset.
>>> my interpretation is that it totally depends on the board design or
>>> the bootloader setup.
>>
>> Partially, you are right. However, we should not add some dependency
>> here from
>> bootloader and board. rst_interval is just to make sure the pulse (low
>> active or high active)
>> lasts the specified the time.
> +Cc Kishon
> 
> he recently added support for a GPIO reset line to the
> pcie-cadence-host.c [0] and I believe he's also maintaining
> pci-keystone.c which are both using a 100uS delay (instead of 100ms).
> I don't know the PCIe spec so maybe Kishon can comment on the values
> that should be used according to the spec.
> if there's then a reason why values other than the ones from the spec
> are needed then there should be a comment explaining why different
> values are needed (what problem does it solve).

The PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION defines the Power
Sequencing and Reset Signal Timings in Table 2-4. Please also refer Figure
2-10: Power Up of the CEM.

ââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ
â Symbol      â Parameter                            â Min â Max â Units â
ââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ
â T PVPERL    â Power stable to PERST# inactive      â 100 â     â ms    â
ââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ
â T PERST-CLK â REFCLK stable before PERST# inactive â 100 â     â Îs    â
ââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ
â T PERST     â PERST# active time                   â 100 â     â Îs    â
ââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ
â T FAIL      â Power level invalid to PERST# active â     â 500 â ns    â
ââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ
â T WKRF      â WAKE# rise â fall time               â     â 100 â ns    â
ââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ

In my code I used T PERST-CLK (i.e REFCLK stable before PERST# inactive).
REFCLK to the card is enabled as part of PHY enable and then wait for 100Îs
before making PERST# inactive.

Power to the device is given during board power up and the assumption here is
it will take more the 100ms for the probe to be invoked after board power up
(i.e after ROM, bootloaders and linux kernel). But if you have a regulator that
is enabled in PCI probe, then T PVPERL (100ms) should also used in probe.

Thanks
Kishon