On 5/22/2019 7:32 PM, Bjorn Helgaas wrote:

On Wed, May 22, 2019 at 02:26:08PM +0530, Vidya Sagar wrote:

quoted

On 5/22/2019 2:47 AM, Bjorn Helgaas wrote:

quoted

On Fri, May 17, 2019 at 06:08:35PM +0530, Vidya Sagar wrote:

quoted

Move PCIe config space capability search API to common DesignWare file
as this can be used by both host and ep mode codes.

quoted

quoted

quoted

   .../pci/controller/dwc/pcie-designware-ep.c   | 37 +----------------
   drivers/pci/controller/dwc/pcie-designware.c  | 40 +++++++++++++++++++
   drivers/pci/controller/dwc/pcie-designware.h  |  2 +
   3 files changed, 44 insertions(+), 35 deletions(-)

quoted

quoted

quoted

--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c

@@ -14,6 +14,46 @@
   #include "pcie-designware.h"
+/*
+ * These APIs are different from standard pci_find_*capability() APIs in the
+ * sense that former can only be used post device enumeration as they require
+ * 'struct pci_dev *' pointer whereas these APIs require 'struct dw_pcie *'
+ * pointer and can be used before link up also.

I think this comment is slightly misleading because it suggests the
reason we need these DW interfaces is because we're doing something
before a pci_dev pointer is available.

But these DW interfaces are used on devices that will *never* have a
pci_dev pointer because they are not PCI devices.  They're used on
host controller devices, which have a PCIe link on the downstream
side, but the host controller driver operates them using their
upstream, non-PCI interfaces.  Logically, I think they would be
considered parts of Root Complexes, not Root Ports.

There's actually no reason why that upstream interface should look
anything like PCI; it doesn't need to organize registers into
capability lists at all.  It might be convenient for the hardware to
do that and share things with a Root Port device, which *is* a PCI
device, but it's not required.

It also really has nothing to do with whether the link is up.  This
code operates on hardware that is upstream from the link, so we can
reach it regardless of the link.

I added this comment after receiving a review comment to justify why
standard pci_find_*capability() APIs can't be used here. Hence added
this.  I understand your comment that DW interface need not have to
be a PCI device, but what is present in the hardware is effectively
a root port implementation and post enumeration, we get a 'struct
pci_dev' created for it, hence I thought it is fine to bring 'struct
pci_dev' into picture.

This code operates on the host controller.  It configures the bridge
that leads *to* PCI devices.  Since that bridge is not a PCI device,
the PCI specs don't say anything about how to program it.

The fact that the host controller programming interface happens to
resemble the PCI programming interface is purely coincidental.

quoted

Also, I agree that mention of 'link up' is unwarranted and could be
reworded in a better way.

Do you suggest to remove this comment altogether or reword it s/and
can be used before link up also/and can be used before 'struct
pci_dev' is available/ ?

Maybe something like this?

     These interfaces resemble the pci_find_*capability() interfaces,
     but these are for configuring host controllers, which are bridges
     *to* PCI devices but are not PCI devices themselves.

Ok. Done.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel