> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Sent: 15 January 2025 22:00
> To: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Cc: Shradha Todi <shradha.t@xxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; lpieralisi@xxxxxxxxxx;
> kw@xxxxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx; jingoohan1@xxxxxxxxx; Jonathan.Cameron@xxxxxxxxxx;
> fan.ni@xxxxxxxxxxx; a.manzanares@xxxxxxxxxxx; pankaj.dubey@xxxxxxxxxxx; quic_nitegupt@xxxxxxxxxxx;
> quic_krichai@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx
> Subject: Re: [PATCH v4 1/2] PCI: dwc: Add support for vendor specific capability search
>
> On Wed, Jan 15, 2025 at 10:12:01AM -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 15, 2025 at 08:57:42PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Dec 11, 2024 at 08:43:30AM -0600, Bjorn Helgaas wrote:
> > > > On Wed, Dec 11, 2024 at 05:15:50PM +0530, Shradha Todi wrote:
> > > > > > -----Original Message-----
> > > > > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > > > > Sent: 06 December 2024 21:43
> > > > > > To: Shradha Todi <shradha.t@xxxxxxxxxxx>
> > > > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> > > > > > manivannan.sadhasivam@xxxxxxxxxx; lpieralisi@xxxxxxxxxx;
> > > > > > kw@xxxxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> > > > > > jingoohan1@xxxxxxxxx; Jonathan.Cameron@xxxxxxxxxx;
> > > > > > fan.ni@xxxxxxxxxxx; a.manzanares@xxxxxxxxxxx;
> > > > > > pankaj.dubey@xxxxxxxxxxx; quic_nitegupt@xxxxxxxxxxx;
> > > > > > quic_krichai@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx
> > > > > > Subject: Re: [PATCH v4 1/2] PCI: dwc: Add support for vendor
> > > > > > specific capability search
> > > > > >
> > > > > > On Fri, Dec 06, 2024 at 01:14:55PM +0530, Shradha Todi wrote:
> > > > > > > Add vendor specific extended configuration space capability
> > > > > > > search API using struct dw_pcie pointer for DW controllers.
> > > > > > >
> > > > > > > Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
> > > > > > > ---
> > > > > > > drivers/pci/controller/dwc/pcie-designware.c | 16
> > > > > > > ++++++++++++++++
> > > > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 +
> > > > > > > 2 files changed, 17 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > > b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > > index 6d6cbc8b5b2c..41230c5e4a53 100644
> > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > > @@ -277,6 +277,22 @@ static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
> > > > > > > return 0;
> > > > > > > }
> > > > > > >
> > > > > > > +u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u8
> > > > > > > +vsec_cap)
> > > > > >
> > > > > > To make sure that we find a VSEC ID that corresponds to the
> > > > > > expected vendor, I think this interface needs to be the same
> > > > > > as pci_find_vsec_capability(). In particular, it needs to
> > > > > > take a
> > > > > > "u16 vendor"
> > > > >
> > > > > As per my understanding, Synopsys is the vendor here when we
> > > > > talk about vsec capabilities. VSEC cap IDs are fixed for each
> > > > > vendor
> > > > > (eg: For Synopsys Designware controllers, 0x2 is always RAS CAP,
> > > > > 0x4 is always PTM responder and so on).
> > > >
> > > > For VSEC, the vendor that matters is the one identified at 0x0 in
> > > > config space. That's why pci_find_vsec_capability() checks the
> > > > supplied "vendor" against "dev->vendor".
> > > >
> > > > > So no matter if the DWC IP is being integrated by Samsung, NVDIA
> > > > > or Qcom, the vendor specific CAP IDs will remain constant. Now
> > > > > since this function is being written as part of designware file,
> > > > > the control will reach here only when the PCIe IP is DWC. So, we
> > > > > don't really require a vendor ID to be checked here. EG: If 0x2
> > > > > VSEC ID is present in any DWC controller, it means RAS is
> > > > > supported. Please correct me if I'm wrong.
> > > >
> > > > In this case, the Vendor ID is typically Samsung, NVIDIA, Qcom,
> > > > etc., even though it may contain Synopsys DWC IP. Each vendor
> > > > assigns VSEC IDs independently, so VSEC ID 0x2 may mean something
> > > > different to Samsung than it does to NVIDIA or Qcom.
> > > >
> > > > PCIe r6.0, sec 7.9.5 has the details, but the important part is this:
> > > >
> > > > With a PCI Express Function, the structure and definition of the
> > > > vendor-specific Registers area is determined by the vendor indicated
> > > > by the Vendor ID field located at byte offset 00h in PCI-compatible
> > > > Configuration Space.
> > > >
> > > > There IS a separate DVSEC ("Designated Vendor-Specific")
> > > > Capability; see sec 7.9.6. That one does include a DVSEC Vendor
> > > > ID in the Capability itself, and this would make more sense for this situation.
> > > >
> > > > If Synopsys assigned DVSEC ID 0x2 from the Synopsys namespace for
> > > > RAS, then devices from Samsung, NVIDIA, Qcom, etc., could
> > > > advertise a DVSEC Capability that contained a DVSEC Vendor ID of
> > > > PCI_VENDOR_ID_SYNOPSYS with DVSEC ID 0x2, and all those devices could easily locate it.
> > > >
> > > > Unfortunately Samsung et al used VSEC instead of DVSEC, so we're
> > > > stuck with having to specify the device vendor and the VSEC ID
> > > > assigned by that vendor, and those VSEC IDs might be different per vendor.
> > >
> > > Atleast on Qcom platforms, VSEC_ID is 0x2 for RAS. But this is not
> > > guaranteed to be the same as per the PCIe spec as you mentioned.
> > > Though, I think it is safe to go with it since we have seen the same
> > > IDs on 2 platforms (my gut feeling is that it is going to be the
> > > same on other DWC vendor platforms as well). If we encounter
> > > different IDs, then we can add vendor id check.
> >
> > This series uses:
> >
> > dw_pcie_find_vsec_capability(pci, DW_PCIE_VSEC_EXT_CAP_RAS_DES)
> >
> > in dwc_pcie_rasdes_debugfs_init(), but I don't see any calls of that
> > function yet.
>
> I guess that the caller got missed unintentionally in patch 2/2.
Actually the missing caller is intentional. Jonathan rightly pointed out in the
previous version that the function : dw_pcie_setup() was being called in the
resume path as well and so I thought it would be best to leave it up to the
platform drivers to decide when and how to call the rasdes init. Do you suggest any
other approach?
>
> > If it is called only from code that already knows the device vendor
> > has assigned VSEC ID 0x02 for the DWC RAS functionality, I guess it is
> > "safe".
> >
>
> It should be called from the DWC code driver (pcie-desginware-host.c).
>
> > But I think it would be a bad idea because it perpetuates the
> > misunderstanding that DesignWare can independently claim ownership of
> > VSEC ID 0x02 for *all* vendors, and other vendors have already used
> > VSEC ID 0x02 for different things (examples at [1]). If any of them
> > incorporates this DWC IP, they will have to use a different VSEC ID to
> > avoid a collision with their existing VSEC ID 0x02.
> >
>
> Fair enough. I was trying to avoid updating the vendor id table for enabling the RAS DES debug feature, but I think it would be worth
> doing so (perf driver is also doing the same).
Makes sense to add the vendor ID check. Will include it in the next version.
>
> So yeah, I'm OK with the idea of having the vendor_id check in this API.
>
> (Also, I don't see the VSEC_IDs defined in the DWC reference manual that I got access to).
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
