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.
> +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);
> +}
> +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?
> + 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.
> + 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.
> + 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?
> +#define BUS_IATU_OFFS SZ_256M
Perhaps less cryptic name?
> +#define RST_INTRVL_DFT_MS 100
Less cryptic name would be
RESET_INTERVAL_MS
> +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.
> +
> + 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.
> + 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.
> + 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.
> + 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)
> + return ret;
> +}
> + platform_set_drvdata(pdev, lpp);
I think it makes sense to setup at the end of the function (before dev_info()
call).
> + data = device_get_match_data(dev);
Perhaps
if (!data)
return -ENODEV; // -EINVAL?
> + /*
> + * 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?
> + dev_info(dev, "Intel PCIe Root Complex Port init done\n");
--
With Best Regards,
Andy Shevchenko
