Messages in this thread | | | Date | Wed, 6 Nov 2019 14:24:52 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v5 2/3] dwc: PCI: intel: PCIe RC controller driver |
| |
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
| |