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 >
next prev parent 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 publicly 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 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.git