[PATCH v2 0/8] Add Keystone PCIe controller driver

[Murali Karicheri]

On 6/17/2014 8:08 PM, Bjorn Helgaas wrote:
> On Tue, Jun 10, 2014 at 02:51:19PM -0400, Murali Karicheri wrote:
>> This patch adds a PCIe controller driver for Keystone SoCs. This
>> is based on v1 of the series posted to the mailing list.
>>
>> CC: Santosh Shilimkar <santosh.shilimkar at ti.com>
>> CC: Russell King <linux at arm.linux.org.uk>
>> CC: Grant Likely <grant.likely at linaro.org>
>> CC: Rob Herring <robh+dt at kernel.org>
>> CC: Mohit Kumar <mohit.kumar at st.com>
>> CC: Jingoo Han <jg1.han at samsung.com>
>> CC: Bjorn Helgaas <bhelgaas at google.com>
>> CC: Pratyush Anand <pratyush.anand at st.com>
>> CC: Richard Zhu <r65037 at freescale.com>
>> CC: Kishon Vijay Abraham I <kishon at ti.com>
>> CC: Marek Vasut <marex at denx.de>
>> CC: Arnd Bergmann <arnd at arndb.de>
>> CC: Pawel Moll <pawel.moll at arm.com>
>> CC: Mark Rutland <mark.rutland at arm.com>
>> CC: Ian Campbell <ijc+devicetree at hellion.org.uk>
>> CC: Kumar Gala <galak at codeaurora.org>
>> CC: Randy Dunlap <rdunlap at infradead.org>
>> CC: Grant Likely <grant.likely at linaro.org>
>>
>>
>> Changelog:
>>
>> V2
>>   - Split the designware pcie enhancement patch to multiple
>>     patches based on functionality added
>>   - Remove the quirk code and add a patch to fix mps/mrss
>>     tuning for ARM. Use kernel command line parameter
>>     pci=pcie_bus_perf to work with Keystone PCI Controller.
>>     Following patch addressed this.
>>       [PATCH v1] ARM: pci: add call to pcie_bus_configure_settings()
>>   - Add documentation for device tree bindings
>>   - Add separate interrupt controller nodes for MSI and Legacy
>>     IRQs and use irq map for legacy IRQ
>>   - Use compatibility to identify v3.65 version of the DW hardware
>>     and use it to customize the designware common code.
>>   - Use reg property for configuration space instead of range
>>   - Other minor updates based on code inspection.
>>
>> V1
>>   - Add an interrupt controller node for Legacy irq chip and use
>>     interrupt map/map-mask property to map legacy IRQs A/B/C/D
>>   - Add a Phy driver to replace the original serdes driver
>>   - Move common application register handling code to a separate
>>     file to allow re-use across other platforms that use older
>>     DW PCIe h/w
>>   - PCI quirk for maximum read request size. Check and override only
>>     if the maximum is higher than what controller can handle.
>>   - Converted to a module platform driver.
>>
>>
>> Murali Karicheri (8):
>>    PCI: designware: add rd[wr]_other_conf API
>>    PCI: designware: refactor host init code to re-use on v3.65 DW pci hw
>>    PCI: designware: update pcie core driver to work with dw hw version
>>      3.65
>>    PCI: designware: add msi controller functions for v3.65 hw
>>    PCI: designware: add PCI controller functions for v3.65 DW hw
>>    phy: Add serdes phy driver for keystone
>>    PCI: keystone: add pcie driver based on designware core driver
>>    ARM: keystone: add pcie related options
>>
>>   .../devicetree/bindings/pci/designware-pcie.txt    |   42 ++
>>   .../devicetree/bindings/pci/pci-keystone.txt       |   56 +++
>>   .../bindings/phy/phy-keystone-serdes.txt           |   25 ++
>>   arch/arm/mach-keystone/Kconfig                     |    1 +
>>   drivers/pci/host/Kconfig                           |   12 +
>>   drivers/pci/host/Makefile                          |    2 +
>>   drivers/pci/host/pci-dw-v3_65-msi.c                |  149 +++++++
>>   drivers/pci/host/pci-dw-v3_65.c                    |  390 ++++++++++++++++++
>>   drivers/pci/host/pci-dw-v3_65.h                    |   34 ++
>>   drivers/pci/host/pci-keystone.c                    |  418 ++++++++++++++++++++
>>   drivers/pci/host/pcie-designware.c                 |  175 +++++---
>>   drivers/pci/host/pcie-designware.h                 |   42 +-
>>   drivers/phy/Kconfig                                |    6 +
>>   drivers/phy/Makefile                               |    1 +
>>   drivers/phy/phy-keystone-serdes.c                  |  230 +++++++++++
>>   15 files changed, 1531 insertions(+), 52 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/pci/pci-keystone.txt
>>   create mode 100644 Documentation/devicetree/bindings/phy/phy-keystone-serdes.txt
>>   create mode 100644 drivers/pci/host/pci-dw-v3_65-msi.c
>>   create mode 100644 drivers/pci/host/pci-dw-v3_65.c
>>   create mode 100644 drivers/pci/host/pci-dw-v3_65.h
>>   create mode 100644 drivers/pci/host/pci-keystone.c
>>   create mode 100644 drivers/phy/phy-keystone-serdes.c
> I'm not willing to merge phy-keystone-serdes.c because I don't
> maintain drivers/phy and because of the binary blob of register values
> it contains, but maybe somebody else will.  I assume it could be
> merged by itself before the rest of this.
>
> I'm looking for acks from Mohit and/or Jingoo for the pci/host
> changes, and from Arnd for the devicetree/bindings changes.
>
> Adding these "-dw-3_64" files is sort of ugly.  If that code is only
> used by keystone, maybe it could just be moved to pci-keystone.c?  But
> I'll defer to Mohit and Jingoo on that and the way you modify
> pcie-designware.c.
>
> Bjorn
Bjorn, Jingoo,

Unfortunately I lost Jingoo's email from my Inbox. So I cut-n-paste the 
comment from
internet and respond.

Jingoo Han wrote:-
=============================================================================
I think so, too.

DT maintainers and arch maintainers should review the following
dt bindings.
   .../devicetree/bindings/pci/designware-pcie.txt    |   42 ++
   .../devicetree/bindings/pci/pci-keystone.txt       |   56 +++
Generic PHY maintainer (Kishon Vijay Abraham I) should review the
following phy driver.
   drivers/phy/phy-keystone-serdes.c

 >
 > I'm looking for acks from Mohit and/or Jingoo for the pci/host
 > changes, and from Arnd for the devicetree/bindings changes.
 >
 > Adding these "-dw-3_64" files is sort of ugly.  If that code is only
 > used by keystone, maybe it could just be moved to pci-keystone.c?  But
 > I'll defer to Mohit and Jingoo on that and the way you modify
 > pcie-designware.c.

I agree with Bjorn Helgaas's opinion. These three "-dw-3_64" files
look terrible! I don't have a good way to handle this; however,
moving this code to pci-keystone.c looks better.
======================================================================

The original RFC I had submitted had all of the application space register
handling code as part of the Keystone PCI driver. As per Arnd's comment 
(See
my change log against v1), the code was moved to a separate file so that
the next driver that has same version of the DW hw could re-use this code.
I agree with Arnd and moved the code to v_3_65 specific files.  What is
your proposal?  Do you have objection to the file name? or it's content?

If objection is on the file name,  please suggest alternate names. If you
are okay with the file name, and doesn't like the code, it will be helpful
to review the code and provide specific comments against the patch itself
so that I can address the same.

Murali