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

From: Chuan Hua, Lei
Date: Sun Aug 25 2019 - 23:31:07 EST

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

+ 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