Messages in this thread | | | Subject | Re: [PATCH v5 2/3] dwc: PCI: intel: PCIe RC controller driver | From | Dilip Kota <> | Date | Mon, 11 Nov 2019 16:08:21 +0800 |
| |
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");
| |