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

[Santosh Shilimkar]

On Friday 20 June 2014 11:31 AM, Murali Karicheri wrote:
> 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@xxxxxx>
>>> CC: Russell King <linux@xxxxxxxxxxxxxxxx>
>>> CC: Grant Likely <grant.likely@xxxxxxxxxx>
>>> CC: Rob Herring <robh+dt@xxxxxxxxxx>
>>> CC: Mohit Kumar <mohit.kumar@xxxxxx>
>>> CC: Jingoo Han <jg1.han@xxxxxxxxxxx>
>>> CC: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>>> CC: Pratyush Anand <pratyush.anand@xxxxxx>
>>> CC: Richard Zhu <r65037@xxxxxxxxxxxxx>
>>> CC: Kishon Vijay Abraham I <kishon@xxxxxx>
>>> CC: Marek Vasut <marex@xxxxxxx>
>>> CC: Arnd Bergmann <arnd@xxxxxxxx>
>>> CC: Pawel Moll <pawel.moll@xxxxxxx>
>>> CC: Mark Rutland <mark.rutland@xxxxxxx>
>>> CC: Ian Campbell <ijc+devicetree@xxxxxxxxxxxxxx>
>>> CC: Kumar Gala <galak@xxxxxxxxxxxxxx>
>>> CC: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
>>> CC: Grant Likely <grant.likely@xxxxxxxxxx>
>>>
>>>
>>> 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.
> 
Arnd suggestion was to have the version 3.65 code in generic place since
its IP specific and just in case some other vendor using the same version
can leverage the code.

Concern here seems toe really those name of the files. I can't think of
any other appropriate name. 

Regards,
Santosh


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/