Date:   Mon, 11 Nov 2019 16:08:21 +0800
From:   Dilip Kota <eswara.kota@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     gustavo.pimentel@...opsys.com, lorenzo.pieralisi@....com,
        andrew.murray@....com, helgaas@...nel.org, jingoohan1@...il.com,
        robh@...nel.org, martin.blumenstingl@...glemail.com,
        linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, cheol.yong.kim@...el.com,
        chuanhua.lei@...ux.intel.com, qi-ming.wu@...el.com
Subject: Re: [PATCH v5 2/3] dwc: PCI: intel: PCIe RC controller driver


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

Powered by blists - more mailing lists