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

[Dilip Kota <eswara.kota@linux.intel.com>]

[Got delivery failure mail , so re-sending the mail]

Hi Martin,

Thanks for review comments, please find my response inline.

On 8/26/2019 11:30 AM, Chuan Hua, Lei wrote:
> Hi Martin,
>
> Thanks for your valuable comments. I reply some of them as below.
>
> Regards,
>
> Chuanhua
>
> On 8/25/2019 5:03 AM, Martin Blumenstingl wrote:
>> Hi Dilip,
>>
>> first of all: thank you for submitting this upstream!
>> I hope that we can use this driver to replace the out-of-tree PCIe
>> driver that's used in OpenWrt for the Lantiq VRX200 SoCs.
>>
>> a small disclaimer: I don't have access to any Lantiq, Intel or
>> DesignWare datasheets. so everything I write below is based on my own
>> understanding of the Tegra public datasheet (which describes the
>> DesignWare PCIe registers) as well as the public Lantiq UGW code drops
>> with out-of-tree drivers for an older version of this PCIe IP.
>> thus some of my statements below may be wrong - in this case I would
>> appreciate an explanation of how the hardware really works.
>>
>> please keep me CC'ed on further revisions of this series. I am highly
>> interested in making this driver work on the Lantiq VRX200 SoCs once
>> the initial version has landed in linux-next.
>>
>>> +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.
>
> After that, we have other PCe 1.1/10. products such as ARX300/390.  
> However, these products are EOL,
>
> that is why we don't put effort to support VRX200/ARX300/390.
>
> PCIE_LANTIQ was also a name used internally in the product as in 
> linux-3.10.x.
>
>
>>
>> [...]
>>> +#define PCIE_CCRID                0x8
>>> +
>>> +#define PCIE_LCAP                0x7C
>>> +#define PCIE_LCAP_MAX_LINK_SPEED        GENMASK(3, 0)
>>> +#define PCIE_LCAP_MAX_LENGTH_WIDTH        GENMASK(9, 4)
>>> +
>>> +/* Link Control and Status Register */
>>> +#define PCIE_LCTLSTS                0x80
>>> +#define PCIE_LCTLSTS_ASPM_ENABLE        GENMASK(1, 0)
>>> +#define PCIE_LCTLSTS_RCB128            BIT(3)
>>> +#define PCIE_LCTLSTS_LINK_DISABLE        BIT(4)
>>> +#define PCIE_LCTLSTS_COM_CLK_CFG        BIT(6)
>>> +#define PCIE_LCTLSTS_HW_AW_DIS            BIT(9)
>>> +#define PCIE_LCTLSTS_LINK_SPEED            GENMASK(19, 16)
>>> +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH    GENMASK(25, 20)
>>> +#define PCIE_LCTLSTS_SLOT_CLK_CFG        BIT(28)
>>> +
>>> +#define PCIE_LCTLSTS2                0xA0
>>> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED        GENMASK(3, 0)
>>> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT    0x1
>>> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT    0x2
>>> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT    0x3
>>> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT    0x4
>>> +#define PCIE_LCTLSTS2_HW_AUTO_DIS        BIT(5)
>>> +
>>> +/* Ack Frequency Register */
>>> +#define PCIE_AFR                0x70C
>>> +#define PCIE_AFR_FTS_NUM            GENMASK(15, 8)
>>> +#define PCIE_AFR_COM_FTS_NUM            GENMASK(23, 16)
>>> +#define PCIE_AFR_GEN12_FTS_NUM_DFT        (SZ_128 - 1)
>>> +#define PCIE_AFR_GEN3_FTS_NUM_DFT        180
>>> +#define PCIE_AFR_GEN4_FTS_NUM_DFT        196
>>> +
>>> +#define PCIE_PLCR_DLL_LINK_EN            BIT(5)
>>> +#define PCIE_PORT_LOGIC_FTS            GENMASK(7, 0)
>>> +#define PCIE_PORT_LOGIC_DFT_FTS_NUM        (SZ_128 - 1)
>>> +
>>> +#define PCIE_MISC_CTRL                0x8BC
>>> +#define PCIE_MISC_CTRL_DBI_RO_WR_EN        BIT(0)
>>> +
>>> +#define PCIE_MULTI_LANE_CTRL            0x8C0
>>> +#define PCIE_UPCONFIG_SUPPORT            BIT(7)
>>> +#define PCIE_DIRECT_LINK_WIDTH_CHANGE        BIT(6)
>>> +#define PCIE_TARGET_LINK_WIDTH            GENMASK(5, 0)
>>> +
>>> +#define PCIE_IOP_CTRL                0x8C4
>>> +#define PCIE_IOP_RX_STANDBY_CTRL        GENMASK(6, 0)
> no need for IOP
>> are you sure that you need any of the registers above?
>> as far as I can tell most (all?) of them are part of the DesignWare
>> register set. why doesn't pcie-designware-host initialize these?
>> I don't have any datasheet or register documentation for the DesignWare
>> PCIe core. In my own experiment from a month ago on the Lantiq VRX200
>> SoC I didn't need any of this: [0]
>
> As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our 
> latest SoC Lightning
>
> Mountain, we are using synopsys controller 5.20/5.50a. We support 
> Gen2(XRX350/550),
>
> Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and 
> single lane.
>
> Some of the above registers are needed to control FTS, link width and 
> link speed.
>
>>
>> this also makes me wonder if various functions below like
>> intel_pcie_link_setup() and intel_pcie_max_speed_setup() (and probably
>> more) are really needed or if it's just cargo cult / copy paste from
>> an out-of-tree driver).
>
> intel_pcie_link_setup is to record maximum speed and and link width. 
> We need these
> to change speed and link width on the fly which is not supported by 
> dwc driver common
> source.
> There are two major purposes.
> 1. For cable applications, they have battery support mode. In this 
> case, it has to
> switch from x2 and gen4 to x1 and gen1 on the fly
> 2. Some customers have high EMI issues. They can try to switch to 
> lower speed and
> lower link width to check on the fly. Otherwise, they have to change 
> the device tree
> and reboot the system.
>
>>
>>> +/* APP RC Core Control Register */
>>> +#define PCIE_RC_CCR                0x10
>>> +#define PCIE_RC_CCR_LTSSM_ENABLE        BIT(0)
>>> +#define PCIE_DEVICE_TYPE            GENMASK(7, 4)
>>> +#define PCIE_RC_CCR_RC_MODE            BIT(2)
>>> +
>>> +/* PCIe Message Control */
>>> +#define PCIE_MSG_CR                0x30
>>> +#define PCIE_XMT_PM_TURNOFF            BIT(0)
>>> +
>>> +/* PCIe Power Management Control */
>>> +#define PCIE_PMC                0x44
>>> +#define PCIE_PM_IN_L2                BIT(20)
>>> +
>>> +/* Interrupt Enable Register */
>>> +#define PCIE_IRNEN                0xF4
>>> +#define PCIE_IRNCR                0xF8
>>> +#define PCIE_IRN_AER_REPORT            BIT(0)
>>> +#define PCIE_IRN_PME                BIT(2)
>>> +#define PCIE_IRN_HOTPLUG            BIT(3)
>>> +#define PCIE_IRN_RX_VDM_MSG            BIT(4)
>>> +#define PCIE_IRN_PM_TO_ACK            BIT(9)
>>> +#define PCIE_IRN_PM_TURNOFF_ACK            BIT(10)
>>> +#define PCIE_IRN_LINK_AUTO_BW_STATUS        BIT(11)
>>> +#define PCIE_IRN_BW_MGT                BIT(12)
>>> +#define PCIE_IRN_WAKEUP                BIT(17)
>>> +#define PCIE_IRN_MSG_LTR            BIT(18)
>>> +#define PCIE_IRN_SYS_INT            BIT(28)
>>> +#define PCIE_IRN_SYS_ERR_RC            BIT(29)
>>> +
>>> +#define PCIE_IRN_IR_INT    (PCIE_IRN_AER_REPORT | PCIE_IRN_PME | \
>>> +            PCIE_IRN_RX_VDM_MSG | PCIE_IRN_SYS_ERR_RC | \
>>> +            PCIE_IRN_PM_TO_ACK | PCIE_IRN_LINK_AUTO_BW_STATUS | \
>>> +            PCIE_IRN_BW_MGT | PCIE_IRN_MSG_LTR)
>> I would rename all of the APP register #defines to match the pattern
>> PCIE_APP_*
>> That makes it easy to differentiate the PCIe (DBI) registers from the
>> APP registers.
>>
>> [...]
> Agree.
>>> +static inline u32 pcie_app_rd(struct intel_pcie_port *lpp, u32 ofs)
>>> +{
>>> +    return readl(lpp->app_base + ofs);
>>> +}
>>> +
>>> +static inline void pcie_app_wr(struct intel_pcie_port *lpp, u32 
>>> val, u32 ofs)
>>> +{
>>> +    writel(val, lpp->app_base + ofs);
>>> +}
>>> +
>>> +static void pcie_app_wr_mask(struct intel_pcie_port *lpp,
>>> +                 u32 mask, u32 val, u32 ofs)
>>> +{
>>> +    pcie_update_bits(lpp->app_base, mask, val, ofs);
>>> +}
>> do you have plans to support the MIPS SoCs (VRX200, ARX300, XRX350,
>> XRX550)?
>> These will need register writes in big endian. in my own experiment [0]
>> I simply used the regmap interface which will default to little endian
>> register access but switch to big endian when the devicetree node is
>> marked with big-endian.
>>
>> [...]
>
> We can support up to XRX350/XRX500/PRX300 for MIPS SoC since we still
>
> sell these products. However, we have no effort to support EOL product
>
> such as VRX200 which also makes driver quite complex since the glue
>
> logic(reset, clock and endianness). For MIPS based platform, we have
>
> endianness control in device tree such as inbound_swap and outbound_swap
>
> For VRX200, we have another big concern, that is PCI and PCIe has 
> coupling
>
> for endiannes which makes things very bad.
>
> However, if you are interested in supporting VRX200, it is highly 
> appreciated.
>
>>> +static int intel_pcie_bios_map_irq(const struct pci_dev *dev, u8 
>>> slot, u8 pin)
>>> +{
>>> +
>>> +    struct pcie_port *pp = dev->bus->sysdata;
>>> +    struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +    struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev);
>>> +    struct device *pdev = lpp->pci->dev;
>>> +    u32 irq_bit;
>>> +    int irq;
>>> +
>>> +    if (pin == PCI_INTERRUPT_UNKNOWN || pin > PCI_NUM_INTX) {
>>> +        dev_warn(pdev, "WARNING: dev %s: invalid interrupt pin %d\n",
>>> +             pci_name(dev), pin);
>>> +        return -1;
>>> +    }
>>> +    irq = of_irq_parse_and_map_pci(dev, slot, pin);
>>> +    if (!irq) {
>>> +        dev_err(pdev, "trying to map irq for unknown slot:%d 
>>> pin:%d\n",
>>> +            slot, pin);
>>> +        return -1;
>>> +    }
>>> +    /* Pin to irq offset bit position */
>>> +    irq_bit = BIT(pin + PCIE_INTX_OFFSET);
>>> +
>>> +    /* Clear possible pending interrupts first */
>>> +    pcie_app_wr(lpp, irq_bit, PCIE_IRNCR);
>>> +
>>> +    pcie_app_wr_mask(lpp, irq_bit, irq_bit, PCIE_IRNEN);
>>> +    return irq;
>>> +}
>> my interpretation is that there's an interrupt controller embedded into
>> the APP registers. The integrated interrupt controller takes 5
>> interrupts and provides the legacy PCI_INTA/B/C/D interrupts as well as
>> a WAKEUP IRQ. Each of it's own interrupts is tied to one of the parent
>> interrupts.
>
> For MIPS base SoC, there is no interrupt controller for such APP 
> registers.
>
> They are directly connected with centralized PIC(ICU for 
> VRX200/ARX300, GIC
>
> for XRX500/PRX300, IOAPIC for lightning mountain).That is why we don't
>
> implement the interrupt controller here.
>
>>
>> my solution (in the experiment on the VRX200 SoC [1]) was to 
>> implement an
>> interrupt controller and have it as a child devicetree node. then I used
>> interrupt-map to route the interrupts to the PCIe interrupt controller.
>> with that I didn't have to overwrite .map_irq.
>>
>> (note that this comment is only related to the PCI_INTx and WAKE
>> interrupts but not the other interrupt configuration bits, because as
>> far as I understand the other ones are only related to the controller)
>>
>>> +static void intel_pcie_bridge_class_code_setup(struct 
>>> intel_pcie_port *lpp)
>>> +{
>>> +    pcie_rc_cfg_wr_mask(lpp, PCIE_MISC_CTRL_DBI_RO_WR_EN,
>>> +                PCIE_MISC_CTRL_DBI_RO_WR_EN, PCIE_MISC_CTRL);
>>> +    pcie_rc_cfg_wr_mask(lpp, 0xffffff00, PCI_CLASS_BRIDGE_PCI << 16,
>>> +                PCIE_CCRID);
>>> +    pcie_rc_cfg_wr_mask(lpp, PCIE_MISC_CTRL_DBI_RO_WR_EN, 0,
>>> +                PCIE_MISC_CTRL);
>>> +}
>> in my own experiments I didn't need this - have you confirmed that it's
>> required? and if it is required: why is that?
>> if others are curious as well then maybe add the explanation as comment
>> to the driver
>>
>> [...]
>
> This is needed. In the old driver, we fixed this by fixup. The 
> original comment as follows,
>
> /*
>  * The root complex has a hardwired class of PCI_CLASS_NETWORK_OTHER or
>  * PCI_CLASS_BRIDGE_HOST, when it is operating as a root complex this
>  * needs to be switched to * PCI_CLASS_BRIDGE_PCI
>  */
>
>>> +static const char *pcie_link_gen_to_str(int gen)
>>> +{
>>> +    switch (gen) {
>>> +    case PCIE_LINK_SPEED_GEN1:
>>> +        return "2.5";
>>> +    case PCIE_LINK_SPEED_GEN2:
>>> +        return "5.0";
>>> +    case PCIE_LINK_SPEED_GEN3:
>>> +        return "8.0";
>>> +    case PCIE_LINK_SPEED_GEN4:
>>> +        return "16.0";
>>> +    default:
>>> +        return "???";
>>> +    }
>>> +}
>> why duplicate PCIE_SPEED2STR from drivers/pci/pci.h?
>
> Great! even link_device_setup can be reduced since pcie_get_speed_cap is
>
> implementing similar stuff.
>
>>
>>> +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.
>
>>
>> [...]
>>> +static int intel_pcie_setup_irq(struct intel_pcie_port *lpp)
>>> +{
>>> +    struct device *dev = lpp->pci->dev;
>>> +    struct platform_device *pdev;
>>> +    char *irq_name;
>>> +    int irq, ret;
>>> +
>>> +    pdev = to_platform_device(dev);
>>> +    irq = platform_get_irq(pdev, 0);
>>> +    if (irq < 0) {
>>> +        dev_err(dev, "missing sys integrated irq resource\n");
>>> +        return irq;
>>> +    }
>>> +
>>> +    irq_name = devm_kasprintf(dev, GFP_KERNEL, "pcie_misc%d", 
>>> lpp->id);
>>> +    if (!irq_name) {
>>> +        dev_err(dev, "failed to alloc irq name\n");
>>> +        return -ENOMEM;
>> you are only requesting one IRQ line for the whole driver. personally
>> I would drop the custom irq_name and pass NULL to devm_request_irq
>> because that will then fall-back to auto-generating an IRQ name based
>> on the devicetree node. I assume it's the same for ACPI but I haven't
>> tried that yet.
>
> Not sure I understand what you mean.  As you know from the code, we 
> have lpp->id which means
>
> we have multiple instances of Root Complex(1,2,3,4,8), so we need this 
> for identification.
>
> this irq in old product called ir(integrated interrupt or misc 
> interrupt).
>
>>
>>> +static void intel_pcie_disable_clks(struct intel_pcie_port *lpp)
>>> +{
>>> +    clk_disable_unprepare(lpp->core_clk);
>>> +}
>>> +
>>> +static int intel_pcie_enable_clks(struct intel_pcie_port *lpp)
>>> +{
>>> +    int ret = clk_prepare_enable(lpp->core_clk);
>>> +
>>> +    if (ret)
>>> +        dev_err(lpp->pci->dev, "Core clock enable failed: %d\n", ret);
>>> +
>>> +    return ret;
>>> +}
>> you have some functions (using these two as an example) which are only
>> used once. they add some boilerplate and (in my opinion) make the code
>> harder to read.
>
> Yes. If we plan to support old MIPS SoC, we have a lot of clocks. The
>
> code is from old code. We can remove this wrapper for new SoC. Later we
>
> can add them back.
>
>>
>>> +static int intel_pcie_get_resources(struct platform_device *pdev)
>>> +{
>>> +    struct intel_pcie_port *lpp;
>>> +    struct device *dev;
>>> +    int ret;
>>> +
>>> +    lpp = platform_get_drvdata(pdev);
>>> +    dev = lpp->pci->dev;
>>> +
>>> +    lpp->core_clk = devm_clk_get(dev, NULL);
>>> +    if (IS_ERR(lpp->core_clk)) {
>>> +        ret = PTR_ERR(lpp->core_clk);
>>> +        if (ret != -EPROBE_DEFER)
>>> +            dev_err(dev, "failed to get clks: %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    lpp->core_rst = devm_reset_control_get(dev, NULL);
>>> +    if (IS_ERR(lpp->core_rst)) {
>>> +        ret = PTR_ERR(lpp->core_rst);
>>> +        if (ret != -EPROBE_DEFER)
>>> +            dev_err(dev, "failed to get resets: %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = device_property_match_string(dev, "device_type", "pci");
>>> +    if (ret) {
>>> +        dev_err(dev, "failed to find pci device type: %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (device_property_read_u32(dev, "intel,rst-interval",
>>> +                     &lpp->rst_interval))
>>> +        lpp->rst_interval = RST_INTRVL_DFT_MS;
>>> +
>>> +    if (device_property_read_u32(dev, "max-link-speed", 
>>> &lpp->link_gen))
>>> +        lpp->link_gen = 0; /* Fallback to auto */
>>> +
>>> +    lpp->app_base = devm_platform_ioremap_resource(pdev, 2);
>> I suggest using platform_get_resource_byname(pdev, "app") because
>> pcie-designware uses named resources for the "config" space already
>>
>> that said, devm_platform_ioremap_resource_byname would be a great
>> addition in my opinion ;)
> Agree.
>>
>>> +    if (IS_ERR(lpp->app_base))
>>> +        return PTR_ERR(lpp->app_base);
>>> +
>>> +    ret = intel_pcie_ep_rst_init(lpp);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    lpp->phy = devm_phy_get(dev, "phy");
>> I suggest to use "pcie" as phy-name, otherwise the binding looks odd:
>>    phys = <&pcie0_phy>;
>>    phy-names = "phy";
>> versus:
>>    phys = <&pcie0_phy>;
>>    phy-names = "pcie";
> Agree.
>>> +static ssize_t
>>> +pcie_link_status_show(struct device *dev, struct device_attribute 
>>> *attr,
>>> +              char *buf)
>>> +{
>>> +    u32 reg, width, gen;
>>> +    struct intel_pcie_port *lpp;
>>> +
>>> +    lpp = dev_get_drvdata(dev);
>>> +
>>> +    reg = pcie_rc_cfg_rd(lpp, PCIE_LCTLSTS);
>>> +    width = FIELD_GET(PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH, reg);
>>> +    gen = FIELD_GET(PCIE_LCTLSTS_LINK_SPEED, reg);
>>> +    if (gen > lpp->max_speed)
>>> +        return -EINVAL;
>>> +
>>> +    return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id,
>>> +               width, pcie_link_gen_to_str(gen));
>>> +}
>>> +static DEVICE_ATTR_RO(pcie_link_status);
>> "lspci -vv | grep LnkSta" already shows the link speed and width.
>> why do you need this?
>
> In most embedded package, lspci from busybox only showed deviceid and 
> vendor id.
>
> They don't install lspci utilities.
>
>>> +static ssize_t pcie_speed_store(struct device *dev,
>>> +                struct device_attribute *attr,
>>> +                const char *buf, size_t len)
>>> +{
>>> +    struct intel_pcie_port *lpp;
>>> +    unsigned long val;
>>> +    int ret;
>>> +
>>> +    lpp = dev_get_drvdata(dev);
>>> +
>>> +    ret = kstrtoul(buf, 10, &val);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (val > lpp->max_speed)
>>> +        return -EINVAL;
>>> +
>>> +    lpp->link_gen = val;
>>> +    intel_pcie_max_speed_setup(lpp);
>>> +    intel_pcie_speed_change_disable(lpp);
>>> +    intel_pcie_speed_change_enable(lpp);
>>> +
>>> +    return len;
>>> +}
>>> +static DEVICE_ATTR_WO(pcie_speed);
>> this is already configurable via devicetree, why do you need this?
> On the fly change as I mentioned in the beginning.
>>
>>> +/*
>>> + * Link width change on the fly is not always successful.
>>> + * It also depends on the partner.
>>> + */
>>> +static ssize_t pcie_width_store(struct device *dev,
>>> +                struct device_attribute *attr,
>>> +                const char *buf, size_t len)
>>> +{
>>> +    struct intel_pcie_port *lpp;
>>> +    unsigned long val;
>>> +
>>> +    lpp = dev_get_drvdata(dev);
>>> +
>>> +    if (kstrtoul(buf, 10, &val))
>>> +        return -EINVAL;
>>> +
>>> +    if (val > lpp->max_width)
>>> +        return -EINVAL;
>>> +
>>> +    lpp->lanes = val;
>>> +    intel_pcie_max_link_width_setup(lpp);
>>> +
>>> +    return len;
>>> +}
>>> +static DEVICE_ATTR_WO(pcie_width);
>> is it needed because of a bug somewhere? who do you expect to use this
>> sysfs attribute and when (which condition) do you expect people to use
>> this?
>>
>> [...]
> On the fly change as I mentioned in the beginning.
>>> +static void __intel_pcie_remove(struct intel_pcie_port *lpp)
>>> +{
>>> +    pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER,
>>> +                0, PCI_COMMAND);
>> I expect logic like this to be part of the PCI subsystem in Linux.
>> why is this needed?
>>
>> [...]
>
> bind/unbind case we use this. For extreme cases, we use unbind and 
> bind to reset
>
> PCI instead of rebooting.
>
>>> +int intel_pcie_msi_init(struct pcie_port *pp)
>>> +{
>>> +    struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +
>>> +    dev_dbg(pci->dev, "MSI is handled in x86 arch\n");
>> I would rephrase this to "MSI is handled by a separate MSI interrupt
>> controller"
>> on the VRX200 SoC there's also a MSI interrupt controller and it seems
>> that "x86" has this as well (even though it might be two different MSI
>> IRQ IP blocks).
>>
>> [...]
>
> Agree. For X86/64, MSI is handled by X86 arch. We don't need to
>
> implement another MSI controller separately.
>
> For MIPS based SoC, we don't use synopsys MSI controller. The MSI still
>
> connects with central interrupt controller with MSI decoding (we can 
> name it
>
> as MSI controller)
>
>>> +static int intel_pcie_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    const struct intel_pcie_soc *data;
>>> +    struct intel_pcie_port *lpp;
>>> +    struct pcie_port *pp;
>>> +    struct dw_pcie *pci;
>>> +    int ret;
>>> +
>>> +    lpp = devm_kzalloc(dev, sizeof(*lpp), GFP_KERNEL);
>>> +    if (!lpp)
>>> +        return -ENOMEM;
>>> +
>>> +    pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
>>> +    if (!pci)
>>> +        return -ENOMEM;
>> other drivers have a "struct dw_pcie    pci" struct member (I assume
>> that it's to prevent memory fragmentation). why not do the same here
>> and embed it into struct intel_pcie_port?
> Dilip should explain this
Almost all the drivers are following the same way. I don't see any issue 
in this way.
Please help me with more description if you see an issue here.

struct qcom_pcie {
     struct dw_pcie *pci;
Ref: 
https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-qcom.c

struct armada8k_pcie {
     struct dw_pcie *pci;
Ref: 
https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-armada8k.c

struct artpec6_pcie {
     struct dw_pcie        *pci;
Ref: 
https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-artpec6.c

struct kirin_pcie {
     struct dw_pcie    *pci;
Ref: 
https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-kirin.c

struct spear13xx_pcie {
     struct dw_pcie        *pci;
Ref: 
https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-spear13xx.c

Regards,

Dilip
>>
>>> +    platform_set_drvdata(pdev, lpp);
>>> +    lpp->pci = pci;
>>> +    pci->dev = dev;
>>> +    pp = &pci->pp;
>>> +
>>> +    ret = device_property_read_u32(dev, "linux,pci-domain", &lpp->id);
>>> +    if (ret) {
>>> +        dev_err(dev, "failed to get domain id, errno %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    pci->dbi_base = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(pci->dbi_base))
>>> +        return PTR_ERR(pci->dbi_base);
>>> +
>> as stated above I would use the _byname variant
> Agree.
>>
>>
>> Martin
>>
>>
>> [0] 
>> https://github.com/xdarklight/linux/blob/lantiq-pcie-driver-next-20190727/drivers/pci/controller/dwc/pcie-lantiq.c
>> [1] 
>> https://github.com/xdarklight/linux/blob/lantiq-pcie-driver-next-20190727/Documentation/devicetree/bindings/pci/lantiq%2Cdw-pcie.yaml