On 11/6/2019 8:24 PM, Andy Shevchenko wrote:
On Wed, Nov 06, 2019 at 11:44:02AM +0800, Dilip Kota wrote:
Add support to PCIe RC controller on Intel Gateway SoCs.
PCIe controller is based of Synopsys DesignWare PCIe core.
Intel PCIe driver requires Upconfigure support, fast training
sequence and link speed configuration. 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.
My comments below, though I may miss the discussion and comment on the settled
things.
+config PCIE_INTEL_GW
+ bool "Intel Gateway PCIe host controller support"
+ depends on OF && (X86 || COMPILE_TEST)
+ select PCIE_DW_HOST
+ help
+ Say 'Y' here to enable PCIe Host controller support on Intel
+ Gateway SoCs.
+ The PCIe controller uses the DesignWare core plus Intel-specific
+ hardware wrappers.
Above has indentation issues.
Typo error, i will fix it.
Thanks for pointing.
+void dw_pcie_upconfig_setup(struct dw_pcie *pci)
+{
+ u32 val;
+
+ val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL);
+ dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL,
+ val | PORT_MLTI_UPCFG_SUPPORT);
Why not to use similar pattern as below?
val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL);
val |= PORT_MLTI_UPCFG_SUPPORT;
dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val);
Ok, i will update it.
+}
+void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
+{
+ u32 reg, val;
+ u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+
+ reg = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
+ reg &= ~PCI_EXP_LNKCTL2_TLS;
+
+ switch (pcie_link_speed[link_gen]) {
+ case PCIE_SPEED_2_5GT:
+ reg |= PCI_EXP_LNKCTL2_TLS_2_5GT;
+ break;
Is this a style or indentation issue?
While writing this switch case, i observed couple of drivers doing like
this(I dont remember the driver names).
I checked now other PCIe controller drivers, break is inline to 'reg |='
line.
I will update it.
+ case PCIE_SPEED_5_0GT:
+ reg |= PCI_EXP_LNKCTL2_TLS_5_0GT;
+ break;
Ditto.
+ case PCIE_SPEED_8_0GT:
+ reg |= PCI_EXP_LNKCTL2_TLS_8_0GT;
+ break;
Ditto.
+ case PCIE_SPEED_16_0GT:
+ reg |= PCI_EXP_LNKCTL2_TLS_16_0GT;
+ break;
Ditto.
+ default:
+ /* Use hardware capability */
Ditto.
I will correct it.
+ val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
+ val = FIELD_GET(PCI_EXP_LNKCAP_SLS, val);
+ reg &= ~PCI_EXP_LNKCTL2_HASD;
+ reg |= FIELD_PREP(PCI_EXP_LNKCTL2_TLS, val);
+ break;
Ditto.
+ }
+
+ dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, reg);
+}
+void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts)
+{
+ u32 val;
+
+ val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
+ val &= ~PORT_LOGIC_N_FTS;
+ val |= n_fts;
What if somebody supplies bits outside of the mask? I guess you need to apply
proper masks to both values.
Agree, i will update it.
+ dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
+}
+#define PORT_LOGIC_N_FTS GENMASK(7, 0)
Shouldn't you use _MASK suffix here?
Agree, i will update it to PORT_LOGIC_N_FTS_MASK .
+#define BUS_IATU_OFFS SZ_256M
Perhaps less cryptic name?
I will update it to BUS_IATU_OFFSET
+#define RST_INTRVL_DFT_MS 100
Less cryptic name would be
RESET_INTERVAL_MS
Agree, I will update it.
+static void pcie_update_bits(void __iomem *base, u32 mask, u32 val, u32 ofs)
+{
+ u32 old, new;
+
+ old = readl(base + ofs);
+ new = old & ~mask;
+ new |= val & mask;
Standard pattern
new = (old & ~mask) | (val & mask);
And actually you may re-use 'val' variable and get rid of 'new' one.
Agree, will reuse the val:
val = (old & ~mask) | (val & mask);
+
+ if (new != old)
+ writel(new, base + ofs);
+}
+static int intel_pcie_get_resources(struct platform_device *pdev)
+{
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+
No need to have this blank line.
Agree, will remove it.
+ pci->dbi_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pci->dbi_base))
+ return PTR_ERR(pci->dbi_base);
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app");
+
Ditto.
Agree, will remove it.
+ lpp->app_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(lpp->app_base))
+ return PTR_ERR(lpp->app_base);
+ return 0;
+}
+ /* Read PMC status and wait for falling into L2 link state */
+ ret = readl_poll_timeout(lpp->app_base + PCIE_APP_PMC, value,
+ (value & PCIE_APP_PMC_IN_L2), 20,
Too many parentheses.
Agree, will remove it.
+ jiffies_to_usecs(5 * HZ));
+ if (!lpp->pcie_cap_ofst) {
+ lpp->pcie_cap_ofst = dw_pcie_find_capability(&lpp->pci,
+ PCI_CAP_ID_EXP);
+ }
Wouldn't be slightly better to have something like
ret = dw_pcie_find_capability(&lpp->pci, PCI_CAP_ID_EXP);
if (ret >= 0 && !lpp->pcie_cap_ofst)
lpp->pcie_cap_ofst = ret;
?
(It can be expanded to print error / warning messages if needed)
This function gets called during initialization and system resume. I
have kept the if check to do dw_pcie_find_capability() only once.(not to
do for system resume)
For error check, i will update this to
if (!lpp->pcie_cap_ofst) { /*pcie_cap_ofst will be 0 for the very
first time as kzalloc() is used for memory allocation for lpp structure*/
ret = dw_pcie_find_capability(&lpp->pci, PCI_CAP_ID_EXP);
if (!ret) { /* Function returns 0 in error case */
ret = -ENXIO;
dev_err();
goto app_init_err;
}
lpp->pcie_cap_ofst = ret;
}
+ return ret;
+}
+ platform_set_drvdata(pdev, lpp);
I think it makes sense to setup at the end of the function (before dev_info()
call).
I have done it immediately after the memory allocation.
Ok, i will move it before dev_info().
+ data = device_get_match_data(dev);
Perhaps
if (!data)
return -ENODEV; // -EINVAL?
Sure i will add it and ENODEV looks appropriate.
+ /*
+ * Intel PCIe doesn't configure IO region, so set viewport
+ * to not to perform IO region access.
+ */
+ pci->num_viewport = data->num_viewport;
Missed blank line?
I will add it.
Thanks a lot for reviewing and giving the inputs. I will update the new
patch version according to your inputs and submit for review.
Regards,
Dilip
+ dev_info(dev, "Intel PCIe Root Complex Port init done\n");
