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

On Saturday, June 21, 2014 12:31 AM, Murali Karicheri wrote:

On 6/17/2014 8:08 PM, Bjorn Helgaas wrote:

quoted

On Tue, Jun 10, 2014 at 02:51:19PM -0400, Murali Karicheri wrote:

quoted

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 <redacted>
CC: Russell King <redacted>
CC: Grant Likely <redacted>
CC: Rob Herring <robh+dt@kernel.org>
CC: Mohit Kumar <redacted>
CC: Jingoo Han <redacted>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Pratyush Anand <redacted>
CC: Richard Zhu <redacted>
CC: Kishon Vijay Abraham I <redacted>
CC: Marek Vasut <marex@denx.de>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Pawel Moll <redacted>
CC: Mark Rutland <mark.rutland@arm.com>
CC: Ian Campbell <redacted>
CC: Kumar Gala <redacted>
CC: Randy Dunlap <redacted>
CC: Grant Likely <redacted>


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?

I said utterly that "I agree with Bjorn Helgaas's opinion". Also, Bjorn
said that "Adding these "-dw-3_64" files is sort of ugly".
Thus, the point is the file name, not the content, as Santosh Shilimkar
said.

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.

I say again that "I have no idea about the alternate file names".
However, if you want to add v_3_65 specific files, please don't
split the files. Please make these three files as 1 file.

   drivers/pci/host/pci-dw-v3_65-msi.c
   drivers/pci/host/pci-dw-v3_65.c
   drivers/pci/host/pci-dw-v3_65.h
   --> drivers/pci/host/pci-dw-v3-65.c
        (I don't have no idea about the name)

Best regards,
Jingoo Han

Murali