From: Dilip Kota <eswara.kota@linux.intel.com>
To: Andrew Murray <andrew.murray@arm.com>
Cc: gustavo.pimentel@synopsys.com, lorenzo.pieralisi@arm.com,
helgaas@kernel.org, jingoohan1@gmail.com, robh@kernel.org,
martin.blumenstingl@googlemail.com, linux-pci@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
andriy.shevchenko@intel.com, cheol.yong.kim@intel.com,
chuanhua.lei@linux.intel.com, qi-ming.wu@intel.com
Subject: Re: [PATCH v5 2/3] dwc: PCI: intel: PCIe RC controller driver
Date: Mon, 11 Nov 2019 16:09:57 +0800 [thread overview]
Message-ID: <aedb7a6a-7fbe-a7d3-b629-775debbc8a1f@linux.intel.com> (raw)
In-Reply-To: <20191108104254.GF43905@e119886-lin.cambridge.arm.com>
On 11/8/2019 6:42 PM, Andrew Murray 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.
>>
>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
>> ---
>> Changes on v5:
>> Use new, old variables in pcie_update_bits()
>> Correct naming:
>> s/Designware/DesignWare
>> s/pcie/PCIe
>> s/pci/PCI
>> s/Hw/HW
>> Upconfig to Upconfigure
>> Remove extra lines after the intel_pcie_max_speed_setup() def.
>> Use pcie_link_speed[] and remove enum for pcie gen.
>> Remove dw_pcie_link_speed_change() def.
>> Remove "linux,pci-domain" parsing.
>> Remove 'id' variable in intel_pcie_port structure.
>> Remove extra spaces for lpp->link_gen =
>> Correct the offset of PCI_EXP_LNKCTL2_HASD to 0x0020.
>> Remove programming of RCB and CCC bits in PCI_EXP_LCTL reg.
>> Remove programming Slot clock cfg in PCI_EXP_LSTS reg.
>> Update the comments at num_viewport setting.
>> Update the description in Kconfig.
>> Get PCIe cap offset from the registers using dw_pcie_find_capability()
>> Define pcie_cap_ofst var in intel_pcie_port struct to store the same.
>> Remove the PCIE_CAP_OFST macro.
>> Move intel_pcie_max_speed_setup() function to DesignWare framework,
>> defined as dw_pcie_link_set_max_speed()
>> Set EXPORT_SYMBOL_GPL for the newer functions defined in
>> pcie-designware.c
>>
>> Changes on v4:
>> Rename the driver naming and description to
>> "PCIe RC controller on Intel Gateway SoCs".
>> Use PCIe core register macros defined in pci_regs.h
>> and remove respective local definitions.
>> Remove PCIe core interrupt handling.
>> Move PCIe link control speed change, upconfig and FTS.
>> configuration functions to DesignWare framework.
>> Use of_pci_get_max_link_speed().
>> Mark dependency on X86 and COMPILE_TEST in Kconfig.
>> Remove lanes and add n_fts variables in intel_pcie_port structure.
>> Rename rst_interval variable to rst_intrvl in intel_pcie_port structure.
>> Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc()
>> Move sysfs attributes specific code to separate patch.
>> Remove redundant error handling.
>> Reduce LoCs by doing variable initializations while declaration itself.
>> Add extra line after closing parenthesis.
>> Move intel_pcie_ep_rst_init() out of get_resources()
>>
>> changes on v3:
>> Rename PCIe app logic registers with PCIE_APP prefix.
>> PCIE_IOP_CTRL configuration is not required. Remove respective code.
>> Remove wrapper functions for clk enable/disable APIs.
>> Use platform_get_resource_byname() instead of
>> devm_platform_ioremap_resource() to be similar with DWC framework.
>> Rename phy name to "pciephy".
>> Modify debug message in msi_init() callback to be more specific.
>> Remove map_irq() callback.
>> Enable the INTx interrupts at the time of PCIe initialization.
>> Reduce memory fragmentation by using variable "struct dw_pcie pci"
>> instead of allocating memory.
>> Reduce the delay to 100us during enpoint initialization
>> intel_pcie_ep_rst_init().
>> Call dw_pcie_host_deinit() during remove() instead of directly
>> calling PCIe core APIs.
>> Rename "intel,rst-interval" to "reset-assert-ms".
>> Remove unused APP logic Interrupt bit macro definitions.
>> Use dwc framework's dw_pcie_setup_rc() for PCIe host specific
>> configuration instead of redefining the same functionality in
>> the driver.
>> Move the whole DT parsing specific code to intel_pcie_get_resources()
>>
>> drivers/pci/controller/dwc/Kconfig | 10 +
>> drivers/pci/controller/dwc/Makefile | 1 +
>> drivers/pci/controller/dwc/pcie-designware.c | 57 +++
>> drivers/pci/controller/dwc/pcie-designware.h | 12 +
>> drivers/pci/controller/dwc/pcie-intel-gw.c | 538 +++++++++++++++++++++++++++
>> include/uapi/linux/pci_regs.h | 1 +
>> 6 files changed, 619 insertions(+)
>> create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c
>>
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index 0ba988b5b5bc..39c17b16b403 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP
>> order to enable device-specific features PCI_DW_PLAT_EP must be
>> selected.
>>
>> +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.
>> +
>> config PCI_EXYNOS
>> bool "Samsung Exynos PCIe controller"
>> depends on SOC_EXYNOS5440 || COMPILE_TEST
>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>> index b30336181d46..99db83cd2f35 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>> obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>> obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>> obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>> +obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
>> obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>> obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
>> obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>> index 820488dfeaed..20e5c3aec394 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>> @@ -14,6 +14,8 @@
>>
>> #include "pcie-designware.h"
>>
>> +extern const unsigned char pcie_link_speed[];
>> +
>> /*
>> * These interfaces resemble the pci_find_*capability() interfaces, but these
>> * are for configuring host controllers, which are bridges *to* PCI devices but
>> @@ -474,6 +476,61 @@ int dw_pcie_link_up(struct dw_pcie *pci)
>> (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING)));
>> }
>>
>> +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);
>> +}
>> +EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
>> +
>> +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;
>> + case PCIE_SPEED_5_0GT:
>> + reg |= PCI_EXP_LNKCTL2_TLS_5_0GT;
>> + break;
>> + case PCIE_SPEED_8_0GT:
>> + reg |= PCI_EXP_LNKCTL2_TLS_8_0GT;
>> + break;
>> + case PCIE_SPEED_16_0GT:
>> + reg |= PCI_EXP_LNKCTL2_TLS_16_0GT;
>> + break;
>> + default:
>> + /* Use hardware capability */
> I think the style here isn't quite inline with other controller drivers.
> Please indent the break statement.
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.
>
> Also the comment above should probably have the same indentation level
> as the code block.
Sure, i will correct it.
>
> But with those changes you can add:
>
> Reviewed-by: Andrew Murray <andrew.murray@arm.con>
Thanks a lot for reviewing patch and giving the inputs.
Regards,
Dilip
next prev parent reply other threads:[~2019-11-11 8:10 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-06 3:44 [PATCH v5 0/3] PCI: Add Intel PCIe Driver and respective dt-binding yaml file Dilip Kota
2019-11-06 3:44 ` [PATCH v5 1/3] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller Dilip Kota
2019-11-12 19:17 ` Rob Herring
2019-11-13 2:39 ` Dilip Kota
2019-11-06 3:44 ` [PATCH v5 2/3] dwc: PCI: intel: PCIe RC controller driver Dilip Kota
2019-11-06 12:24 ` Andy Shevchenko
2019-11-11 8:08 ` Dilip Kota
2019-11-12 7:18 ` Dilip Kota
2019-11-08 10:42 ` Andrew Murray
2019-11-11 8:09 ` Dilip Kota [this message]
2019-11-06 3:44 ` [PATCH v5 3/3] PCI: artpec6: Configure FTS with dwc helper function Dilip Kota
2019-11-06 9:43 ` Gustavo Pimentel
2019-11-11 6:24 ` Dilip Kota
2019-11-07 21:03 ` Jingoo Han
2019-11-08 10:43 ` Andrew Murray
2019-11-11 8:10 ` Dilip Kota
2019-11-11 8:09 ` 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=aedb7a6a-7fbe-a7d3-b629-775debbc8a1f@linux.intel.com \
--to=eswara.kota@linux.intel.com \
--cc=andrew.murray@arm.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=helgaas@kernel.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).