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

[Jingoo Han <jingoohan1@gmail.com>]

On 11/18/19, 2:58 AM, Dilip Kota wrote:

> On 11/16/2019 4:40 AM, Jingoo Han wrote:
> > On 11/14/19, 9:31 PM, 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 configurations. 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>
> >> Reviewed-by: Andrew Murray <andrew.murray@arm.com>
> >> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> >> ---
> > [.....]
> >
> >>   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   | 542 +++++++++++++++++++++++++++
> >>   include/uapi/linux/pci_regs.h                |   1 +
> >>   6 files changed, 623 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..fb6d474477df 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.
> >> +
> > Please add this config alphabetical order!
> > So, this config should be after 'config PCI_IMX6'.
> > There is no reason to put this config at the first place.
> >
> >>   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
> > Ditto.
> PCIE_INTEL_GW wouldnt come after PCI_IMX6, the complete Makefile and 
> Kconfig are not in order,( PCI_* and PCIE_* are not in any order). So i 
> just followed PCIE_DW and placed PCIE_INTEL_GW after PCIE_DW as I is 
> after D (and i see PCI_* immediately after the PCIE_DW*, so i placed 
> PCIE_INTEL_GW after PCIE_DW* and before PCI_*).

Hey, although some of them are not in order, you don't have a right to do so.
If some people do not follow the law, it does not mean that you can break the law.
Anyway, if you don't follow an alphabetical order, my answer is NACK.
Also, other people or I will send a patch to fix the order of other drivers.


> Regards,
> Dilip

>
> Best regards,
> Jingoo Han
>
>>   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..479e250695a0 100644
> [.....]