Hi Bjorn Helgaas,
On 10/22/2019 1:17 AM, Bjorn Helgaas wrote:
On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote:
Add support to PCIe RC controller on Intel Gateway SoCs.
PCIe controller is based of Synopsys DesignWare pci core.
Intel PCIe driver requires Upconfig support, fast training
sequence configuration and link speed change. So adding the
respective helper functions in the pcie DesignWare framework.
It also programs hardware autonomous speed during speed
configuration so defining it in pci_regs.h.
+static void intel_pcie_link_setup(struct intel_pcie_port *lpp)
+{
+ u32 val;
+
+ val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP);
+ lpp->max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val);
+ lpp->max_width = FIELD_GET(PCI_EXP_LNKCAP_MLW, val);
+
+ val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL);
+
+ val &= ~(PCI_EXP_LNKCTL_LD | PCI_EXP_LNKCTL_ASPMC);
+ val |= (PCI_EXP_LNKSTA_SLC << 16) | PCI_EXP_LNKCTL_CCC |
+ PCI_EXP_LNKCTL_RCB;
Link Control is only 16 bits wide, so "PCI_EXP_LNKSTA_SLC << 16"
wouldn't make sense. But I guess you're writing a device-specific
register that is not actually the Link Control as documented in PCIe
r5.0, sec 7.5.3.7, even though the bits are similar?
It is not device specific.
You are correct about register size that pcie spec mentions
PCIE_EXP_LNKCTL at 0x10 and PCIE_EXP_LNKSTS at 0x12 each of 2bytes.
Accessing 4bytes at offset 0x10 ends up accessing LNK control and status
register.
In Synopsys PCIe controller LINK_CONTROL_LINK_STATUS_REG is of 4bytes
size at offset 0x10,
In both the cases everything is same except the size of the register, so
i used PCIE_EXP_LNKCTL macro which is already defined in pci_regs.h
Likewise, PCI_EXP_LNKCTL_RCB is RO for Root Ports, but maybe you're
telling the device what it should advertise in its Link Control?
You are correct, PCI_EXP_LNKCTL_RCB is RO. I will remove it.
PCI_EXP_LNKCTL_CCC is RW. But doesn't it depend on the components on
both ends of the link? Do you know what device is at the other end?
I would have assumed that you'd have to start with CCC==0, which
should be most conservative, then set CCC=1 only if you know both ends
have a common clock.
PCIe RC and endpoint device are having the common clock so set the CCC=1.
+ pcie_rc_cfg_wr(lpp, val, PCIE_CAP_OFST + PCI_EXP_LNKCTL);
+}
+
+static void intel_pcie_max_speed_setup(struct intel_pcie_port *lpp)
+{
+ u32 reg, val;
+
+ reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL2);
+ switch (lpp->link_gen) {
+ case PCIE_LINK_SPEED_GEN1:
+ reg &= ~PCI_EXP_LNKCTL2_TLS;
+ reg |= PCI_EXP_LNKCTL2_HASD|
+ PCI_EXP_LNKCTL2_TLS_2_5GT;
+ break;
+ case PCIE_LINK_SPEED_GEN2:
+ reg &= ~PCI_EXP_LNKCTL2_TLS;
+ reg |= PCI_EXP_LNKCTL2_HASD|
+ PCI_EXP_LNKCTL2_TLS_5_0GT;
+ break;
+ case PCIE_LINK_SPEED_GEN3:
+ reg &= ~PCI_EXP_LNKCTL2_TLS;
+ reg |= PCI_EXP_LNKCTL2_HASD|
+ PCI_EXP_LNKCTL2_TLS_8_0GT;
+ break;
+ case PCIE_LINK_SPEED_GEN4:
+ reg &= ~PCI_EXP_LNKCTL2_TLS;
+ reg |= PCI_EXP_LNKCTL2_HASD|
+ PCI_EXP_LNKCTL2_TLS_16_0GT;
+ break;
+ default:
+ /* Use hardware capability */
+ val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP);
+ val = FIELD_GET(PCI_EXP_LNKCAP_SLS, val);
+ reg &= ~PCI_EXP_LNKCTL2_HASD;
+ reg |= val;
+ break;
+ }
+
+ pcie_rc_cfg_wr(lpp, reg, PCIE_CAP_OFST + PCI_EXP_LNKCTL2);
+ dw_pcie_link_set_n_fts(&lpp->pci, lpp->n_fts);
There are other users of of_pci_get_max_link_speed() that look sort of
similar to this (dra7xx_pcie_establish_link(),
ks_pcie_set_link_speed(), tegra_pcie_prepare_host()). Do these *need*
to be different, or is there something that could be factored out?
dra7xx_pcie_establish_link() and ks_pcie_set_link_speed() are doing
speed configuration for GEN1 and GEN1 &2 respectively.
intel_pcie_max_speed_setup() can be moved to dwc framework and dra7xx and ks_pcie drivers can call.
+}
+
+
+
Remove extra blank lines here.
i will remove it.
+static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp)
...
+ /* Intel PCIe doesn't configure IO region, so configure
+ * viewport to not to access IO region during register
+ * read write operations.
+ */
This comment doesn't describe the code. Is there supposed to be some
code here that configures the viewport? Where do we tell the viewport
not to access IO?
I guess maybe this means something like "tell
dw_pcie_access_other_conf() not to program an outbound ATU for I/O"?
I don't know that structure well enough to write this in a way that
makes sense, but this code doesn't look like it's configuring any
viewports.
Yes, you are correct. Telling not to program an outbound ATU for IO.
I will update the description.
Please use usual multi-line comment style, i.e.,
/*
* Intel PCIe ...
*/
Sure, i will correct it.
Thanks for reviewing and providing the valuable inputs!
Regards,
Dilip
+ pci->num_viewport = data->num_viewport;
+ dev_info(dev, "Intel PCIe Root Complex Port %d init done\n", lpp->id);
+
+ return ret;
+}
