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

[Dilip Kota <eswara.kota@linux.intel.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