Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Dilip Kota <eswara.kota@linux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: jingoohan1@gmail.com, gustavo.pimentel@synopsys.com,
	lorenzo.pieralisi@arm.com, robh@kernel.org,
	martin.blumenstingl@googlemail.com, linux-pci@vger.kernel.org,
	hch@infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, cheol.yong.kim@intel.com,
	chuanhua.lei@linux.intel.com, qi-ming.wu@intel.com
Subject: Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
Date: Fri, 6 Sep 2019 18:39:50 +0800
Message-ID: <6e2a5901-568a-00e7-9877-014041161f4d@linux.intel.com> (raw)
In-Reply-To: <20190904130504.GN2680@smile.fi.intel.com>

Hi Andy,

Thanks for the review comments, please find my response inline.

On 9/4/2019 9:05 PM, Andy Shevchenko wrote:
> On Wed, Sep 04, 2019 at 06:10:31PM +0800, Dilip Kota wrote:
>> Add support to PCIe RC controller on Intel Universal
>> Gateway SoC. PCIe controller is based of Synopsys
>> Designware pci core.
> Thanks for an update. My comments below.
>
>> +config PCIE_INTEL_AXI
>> +        bool "Intel AHB/AXI PCIe host controller support"
>> +        depends on PCI_MSI
>> +        depends on PCI
>> +        depends on OF
>> +        select PCIE_DW_HOST
>> +        help
>> +          Say 'Y' here to enable support for Intel AHB/AXI PCIe Host
>> +	  controller driver.
>> +	  The Intel PCIe controller is based on the Synopsys Designware
>> +	  pcie core and therefore uses the Designware core functions to
>> +	  implement the driver.
> This hunk is full of indentation issues. Please, see how it's done in other
> cases which are already part of upstream kernel.
Sure, I will fix it.
>
>> +#define PCIE_APP_INTX_OFST	12
>> +#define PCIE_APP_IRN_INT	(PCIE_APP_IRN_AER_REPORT | PCIE_APP_IRN_PME | \
> It would be slightly easier to read if the value starts from the next line.
Agree, i will update it in the next patch revision.
>
>> +			PCIE_APP_IRN_RX_VDM_MSG | PCIE_APP_IRN_SYS_ERR_RC | \
>> +			PCIE_APP_IRN_PM_TO_ACK | PCIE_APP_IRN_MSG_LTR | \
>> +			PCIE_APP_IRN_BW_MGT | PCIE_APP_IRN_LINK_AUTO_BW_STAT | \
>> +			(PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTA) | \
>> +			(PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTB) | \
>> +			(PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTC) | \
>> +			(PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTD))
>> +static void intel_pcie_link_setup(struct intel_pcie_port *lpp)
>> +{
>> +	u32 val;
>> +
>> +	val = pcie_rc_cfg_rd(lpp, PCIE_LCAP);
>> +	lpp->max_speed = FIELD_GET(PCIE_LCAP_MAX_LINK_SPEED, val);
>> +	lpp->max_width = FIELD_GET(PCIE_LCAP_MAX_LENGTH_WIDTH, val);
>> +
>> +	val = pcie_rc_cfg_rd(lpp, PCIE_LCTLSTS);
>> +
>> +	val &= ~(PCIE_LCTLSTS_LINK_DISABLE | PCIE_LCTLSTS_ASPM_ENABLE);
>> +	val |= (PCIE_LCTLSTS_SLOT_CLK_CFG | PCIE_LCTLSTS_COM_CLK_CFG |
>> +		PCIE_LCTLSTS_RCB128);
> Unnecessary parentheses.
Sure, i will fix it.
>
>> +	pcie_rc_cfg_wr(lpp, val, PCIE_LCTLSTS);
>> +}
>> +	switch (lpp->max_speed) {
>> +	case PCIE_LINK_SPEED_GEN1:
>> +	case PCIE_LINK_SPEED_GEN2:
>> +		fts = PCIE_AFR_GEN12_FTS_NUM_DFT;
>> +		break;
> You may group this with default case, right?
Sure, i am ok to group it with default case. I will update it in the 
next patch revision.
>
>> +	case PCIE_LINK_SPEED_GEN3:
>> +		fts = PCIE_AFR_GEN3_FTS_NUM_DFT;
>> +		break;
>> +	case PCIE_LINK_SPEED_GEN4:
>> +		fts = PCIE_AFR_GEN4_FTS_NUM_DFT;
>> +		break;
>> +	default:
>> +		fts = PCIE_AFR_GEN12_FTS_NUM_DFT;
>> +		break;
>> +	}
>> +	trace_printk("PCIe misc interrupt status 0x%x\n", reg);
> Hmm... Doesn't it give you a BIG warning?
>
> kernel/trace/trace.c:trace_printk_init_buffers()
I hope this should be fine as it come in debug builds only.
>
>> +	return IRQ_HANDLED;
>> +}
> Perhaps the PCI needs some trace points instead.
>
>> +static int intel_pcie_setup_irq(struct intel_pcie_port *lpp)
>> +{
>> +	struct device *dev = lpp->pci.dev;
>> +	struct platform_device *pdev;
>> +	char *irq_name;
>> +	int irq, ret;
>> +
>> +	pdev = to_platform_device(dev);
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		dev_err(dev, "missing sys integrated irq resource\n");
> Redundant since this cycle.
platform_get_irq() is not printing any error message, so kept a error 
message here.

>
>> +		return irq;
>> +	}
>> +	irq_name = devm_kasprintf(dev, GFP_KERNEL, "pcie_misc%d", lpp->id);
>> +	if (!irq_name) {
>> +		dev_err(dev, "failed to alloc irq name\n");
> Not very useful. initcall_debug shows an error code.
Thanks for pointing it. I will remove this debug print.
>
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, irq, intel_pcie_core_isr,
>> +			       IRQF_SHARED, irq_name, lpp);
>> +	if (ret) {
>> +		dev_err(dev, "request irq %d failed\n", irq);
>> +		return ret;
>> +	}
> + blank line.
Agree, will fix it.
>
>> +	/* Enable integrated interrupts */
>> +	pcie_app_wr_mask(lpp, PCIE_APP_IRN_INT,
>> +			 PCIE_APP_IRN_INT, PCIE_APP_IRNEN);
> At least one parameter can be located on the previous line.
Agree, will fix it.
>
>> +	return ret;
>> +}
>> +static int intel_pcie_get_resources(struct platform_device *pdev)
>> +{
>> +	struct intel_pcie_port *lpp;
>> +	struct resource *res;
>> +	struct dw_pcie *pci;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	lpp = platform_get_drvdata(pdev);
>> +	pci = &lpp->pci;
>> +	dev = pci->dev;
> You may save here and there few LOCs by adding these to corresponding
> definition blocks.
Agree, will fix it.
>
>> +
>> +	ret = device_property_read_u32(dev, "linux,pci-domain", &lpp->id);
>> +	if (ret) {
>> +		dev_err(dev, "failed to get domain id, errno %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
>> +	if (!res)
>> +		return -ENXIO;
> Redundant. It's done in below call.
Agree, will fix it.
>
>> +
>> +	pci->dbi_base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(pci->dbi_base))
>> +		return PTR_ERR(pci->dbi_base);
>> +
>> +	lpp->core_clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(lpp->core_clk)) {
>> +		ret = PTR_ERR(lpp->core_clk);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "failed to get clks: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	lpp->core_rst = devm_reset_control_get(dev, NULL);
>> +	if (IS_ERR(lpp->core_rst)) {
>> +		ret = PTR_ERR(lpp->core_rst);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "failed to get resets: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = device_property_match_string(dev, "device_type", "pci");
>> +	if (ret) {
>> +		dev_err(dev, "failed to find pci device type: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (device_property_read_u32(dev, "reset-assert-ms",
>> +				     &lpp->rst_interval))
> I would rather leave it on one line.
Agree, will fix it.
>
>
>> +		lpp->rst_interval = RST_INTRVL_DFT_MS;
>> +
>> +	if (device_property_read_u32(dev, "max-link-speed", &lpp->link_gen))
>> +		lpp->link_gen = 0; /* Fallback to auto */
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app");
>> +	if (!res)
>> +		return -ENXIO;
> Redundant.
Agree, will fix it.
>
>> +
>> +	lpp->app_base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(lpp->app_base))
>> +		return PTR_ERR(lpp->app_base);
>> +
>> +	ret = intel_pcie_ep_rst_init(lpp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	lpp->phy = devm_phy_get(dev, "pciephy");
>> +	if (IS_ERR(lpp->phy)) {
>> +		ret = PTR_ERR(lpp->phy);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "couldn't get pcie-phy: %d\n", ret);
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>> +static int intel_pcie_host_setup(struct intel_pcie_port *lpp)
>> +{
>> +	int ret;
>> +
>> +	intel_pcie_core_rst_assert(lpp);
>> +	intel_pcie_device_rst_assert(lpp);
>> +
>> +	ret = phy_init(lpp->phy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	intel_pcie_core_rst_deassert(lpp);
>> +
>> +	ret = clk_prepare_enable(lpp->core_clk);
>> +	if (ret) {
>> +		dev_err(lpp->pci.dev, "Core clock enable failed: %d\n", ret);
>> +		goto clk_err;
>> +	}
>> +
>> +	intel_pcie_rc_setup(lpp);
>> +	ret = intel_pcie_app_logic_setup(lpp);
>> +	if (ret)
>> +		goto app_init_err;
>> +
>> +	ret = intel_pcie_setup_irq(lpp);
>> +	if (!ret)
>> +		return ret;
>> +
>> +	intel_pcie_turn_off(lpp);
>> +app_init_err:
>> +	clk_disable_unprepare(lpp->core_clk);
>> +clk_err:
>> +	intel_pcie_core_rst_assert(lpp);
>> +	intel_pcie_deinit_phy(lpp);
> Why not phy_exit()?
intel_pcie_deinit_phy() calls phy_exit().
>
>> +	return ret;
>> +}
>> +static ssize_t pcie_width_store(struct device *dev,
>> +				struct device_attribute *attr,
>> +				const char *buf, size_t len)
>> +{
>> +	struct intel_pcie_port *lpp;
>> +	unsigned long val;
>> +
>> +	lpp = dev_get_drvdata(dev);
>> +
>> +	if (kstrtoul(buf, 10, &val))
>> +		return -EINVAL;
> Why shadowing error code?
Thanks for pointing it, will fix it.
>
>> +
>> +	if (val > lpp->max_width)
>> +		return -EINVAL;
>> +
>> +	lpp->lanes = val;
>> +	intel_pcie_max_link_width_setup(lpp);
>> +
>> +	return len;
>> +}
>> +	pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER,
>> +			    0, PCI_COMMAND);
> 0 is pretty much fits previous line.
Agree, will fix it.
>
> You need to fix your editor settings.
>
>> +int intel_pcie_msi_init(struct pcie_port *pp)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +
>> +	dev_dbg(pci->dev, "PCIe MSI/MSIx is handled by MSI in x86 processor\n");
> Is it useful message?
Yes, it is required because MSI controller can be of PCIe RC or a 
different one.
In this case MSI controller is of x86 processor.
>
>> +	return 0;
>> +}
>> +	dev_info(dev,
>> +		 "Intel AXI PCIe Root Complex Port %d Init Done\n", lpp->id);
> Same, literal can be placed on previous line.
Agree, will fix it.


Regards,

Dilip

>

  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-04 10:10 [PATCH v3 0/2] PCI: Add Intel PCIe Driver and respective dt-binding yaml file Dilip Kota
2019-09-04 10:10 ` [PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller Dilip Kota
2019-09-05  2:23   ` Chuan Hua, Lei
2019-09-06 10:39     ` Dilip Kota
2019-09-05 20:31   ` Martin Blumenstingl
2019-09-06  3:22     ` Chuan Hua, Lei
2019-09-06 17:17       ` Martin Blumenstingl
2019-09-06 17:48         ` Andy Shevchenko
2019-09-07  1:48           ` Ivan Gorinov
2019-09-06  9:19     ` Andy Shevchenko
2019-09-09  6:52       ` Dilip Kota
2019-09-17 18:33       ` Rob Herring
2019-09-18  6:48         ` Dilip Kota
2019-09-17 18:40   ` Rob Herring
2019-09-18  6:56     ` Dilip Kota
     [not found]       ` <b7e549bb-b46c-c393-50ac-9ef3b198fd49@linux.intel.com>
2019-10-03  6:35         ` Fwd: " Dilip Kota
2019-09-04 10:10 ` [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver Dilip Kota
2019-09-04 13:05   ` Andy Shevchenko
2019-09-06 10:39     ` Dilip Kota [this message]
2019-09-05  2:30   ` Chuan Hua, Lei
2019-09-05 10:45   ` Andrew Murray
2019-09-05 11:26     ` Christoph Hellwig
2019-09-05 11:40     ` Andy Shevchenko
2019-09-12  7:01       ` Dilip Kota
2019-09-06 10:58     ` Dilip Kota
2019-09-06 11:20       ` Andrew Murray
2019-09-09  6:51         ` Dilip Kota
2019-09-09  8:31           ` Andrew Murray
2019-09-10  8:08             ` Dilip Kota
     [not found]             ` <22857835-1f98-b251-c94b-16b4b0a6dba2@linux.intel.com>
2019-09-11 10:30               ` Andrew Murray
2019-09-12  6:58                 ` Dilip Kota
2019-09-12  8:25                   ` Andrew Murray
2019-09-12  9:23                     ` Dilip Kota
2019-09-12 10:49                       ` Gustavo Pimentel
2019-09-13  9:20                         ` Dilip Kota
2019-09-13 10:12                           ` andriy.shevchenko
2019-09-16  3:03                             ` Dilip Kota

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6e2a5901-568a-00e7-9877-014041161f4d@linux.intel.com \
    --to=eswara.kota@linux.intel.com \
    --cc=andriy.shevchenko@intel.com \
    --cc=cheol.yong.kim@intel.com \
    --cc=chuanhua.lei@linux.intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gustavo.pimentel@synopsys.com \
    --cc=hch@infradead.org \
    --cc=jingoohan1@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=qi-ming.wu@intel.com \
    --cc=robh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox