Re: [PATCH v5 2/3] dwc: PCI: intel: PCIe RC controller driver

[Dilip Kota]

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");