Add vendor specific extended configuration space capability search API
using struct dw_pcie pointer for DW controllers.
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
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)
+{
+ u16 vsec = 0;
+ u32 header;
+
+ while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
+ PCI_EXT_CAP_ID_VNDR)) {
+ header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
+ if (PCI_VNDR_HEADER_ID(header) == vsec_cap)
+ return vsec;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dw_pcie_find_vsec_capability);
+
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
{
return dw_pcie_find_next_ext_capability(pci, 0, cap);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 347ab74ac35a..98a057820bc7 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -476,6 +476,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);
u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);
+u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u8 vsec_cap);
int dw_pcie_read(void __iomem *addr, int size, u32 *val);
int dw_pcie_write(void __iomem *addr, int size, u32 val);
--
2.17.1On 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@samsung.com>
> ---
> 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" and a "u16 vsec_cap".
(pci_find_vsec_capability() takes an "int cap", but I don't think
that's quite right).
> +{
> + u16 vsec = 0;
> + u32 header;
> +
> + while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
> + PCI_EXT_CAP_ID_VNDR)) {
> + header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
> + if (PCI_VNDR_HEADER_ID(header) == vsec_cap)
> + return vsec;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_pcie_find_vsec_capability);
> +
> u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
> {
> return dw_pcie_find_next_ext_capability(pci, 0, cap);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 347ab74ac35a..98a057820bc7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -476,6 +476,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);
>
> u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);
> +u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u8 vsec_cap);
>
> int dw_pcie_read(void __iomem *addr, int size, u32 *val);
> int dw_pcie_write(void __iomem *addr, int size, u32 val);
> --
> 2.17.1
>
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 06 December 2024 21:43
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> fan.ni@samsung.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> quic_krichai@quicinc.com; gost.dev@samsung.com
> 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@samsung.com>
> > ---
> > 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).
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.
>and a "u16 vsec_cap".
>
> (pci_find_vsec_capability() takes an "int cap", but I don't think that's quite right).
>
It should be u16 vsec_cap. You're right. I will fix this in the next patchset.
Shradha
> > +{
> > + u16 vsec = 0;
> > + u32 header;
> > +
> > + while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
> > + PCI_EXT_CAP_ID_VNDR)) {
> > + header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
> > + if (PCI_VNDR_HEADER_ID(header) == vsec_cap)
> > + return vsec;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dw_pcie_find_vsec_capability);
> > +
> > u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap) {
> > return dw_pcie_find_next_ext_capability(pci, 0, cap); diff --git
> > a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 347ab74ac35a..98a057820bc7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -476,6 +476,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);
> >
> > u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> > u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);
> > +u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u8 vsec_cap);
> >
> > int dw_pcie_read(void __iomem *addr, int size, u32 *val); int
> > dw_pcie_write(void __iomem *addr, int size, u32 val);
> > --
> > 2.17.1
> >
On Wed, Dec 11, 2024 at 05:15:50PM +0530, Shradha Todi wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: 06 December 2024 21:43
> > To: Shradha Todi <shradha.t@samsung.com>
> > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> > kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> > fan.ni@samsung.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> > quic_krichai@quicinc.com; gost.dev@samsung.com
> > 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@samsung.com>
> > > ---
> > > 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.
Bjorn
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@kernel.org>
> > > Sent: 06 December 2024 21:43
> > > To: Shradha Todi <shradha.t@samsung.com>
> > > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> > > kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> > > fan.ni@samsung.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> > > quic_krichai@quicinc.com; gost.dev@samsung.com
> > > 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@samsung.com>
> > > > ---
> > > > 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.
- Mani
--
மணிவண்ணன் சதாசிவம்
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@kernel.org>
> > > > Sent: 06 December 2024 21:43
> > > > To: Shradha Todi <shradha.t@samsung.com>
> > > > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> > > > kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> > > > fan.ni@samsung.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> > > > quic_krichai@quicinc.com; gost.dev@samsung.com
> > > > 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@samsung.com>
> > > > > ---
> > > > > 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. 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".
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.
Bjorn
[1] https://lore.kernel.org/r/20241209222938.3219364-1-helgaas@kernel.org/
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@kernel.org>
> > > > > Sent: 06 December 2024 21:43
> > > > > To: Shradha Todi <shradha.t@samsung.com>
> > > > > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> > > > > kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> > > > > fan.ni@samsung.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> > > > > quic_krichai@quicinc.com; gost.dev@samsung.com
> > > > > 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@samsung.com>
> > > > > > ---
> > > > > > 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.
> 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).
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
--
மணிவண்ணன் சதாசிவம்
> -----Original Message-----
> From: Shradha Todi <shradha.t@samsung.com>
> Sent: 15 January 2025 22:33
> To: 'Manivannan Sadhasivam' <manivannan.sadhasivam@linaro.org>; 'Bjorn Helgaas' <helgaas@kernel.org>
> Cc: 'linux-kernel@vger.kernel.org' <linux-kernel@vger.kernel.org>; 'linux-pci@vger.kernel.org' <linux-pci@vger.kernel.org>;
> 'lpieralisi@kernel.org' <lpieralisi@kernel.org>; 'kw@linux.com' <kw@linux.com>; 'robh@kernel.org' <robh@kernel.org>;
> 'bhelgaas@google.com' <bhelgaas@google.com>; 'jingoohan1@gmail.com' <jingoohan1@gmail.com>;
> 'Jonathan.Cameron@huawei.com' <Jonathan.Cameron@huawei.com>; 'fan.ni@samsung.com' <fan.ni@samsung.com>;
> 'a.manzanares@samsung.com' <a.manzanares@samsung.com>; 'pankaj.dubey@samsung.com' <pankaj.dubey@samsung.com>;
> 'quic_nitegupt@quicinc.com' <quic_nitegupt@quicinc.com>; 'quic_krichai@quicinc.com' <quic_krichai@quicinc.com>;
> 'gost.dev@samsung.com' <gost.dev@samsung.com>
> Subject: RE: [PATCH v4 1/2] PCI: dwc: Add support for vendor specific capability search
>
>
>
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 15 January 2025 22:00
> > To: Bjorn Helgaas <helgaas@kernel.org>
> > Cc: Shradha Todi <shradha.t@samsung.com>; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; lpieralisi@kernel.org;
> > kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> > fan.ni@samsung.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> > quic_krichai@quicinc.com; gost.dev@samsung.com
> > 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@kernel.org>
> > > > > > > Sent: 06 December 2024 21:43
> > > > > > > To: Shradha Todi <shradha.t@samsung.com>
> > > > > > > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org;
> > > > > > > manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> > > > > > > kw@linux.com; robh@kernel.org; bhelgaas@google.com;
> > > > > > > jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> > > > > > > fan.ni@samsung.com; a.manzanares@samsung.com;
> > > > > > > pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> > > > > > > quic_krichai@quicinc.com; gost.dev@samsung.com
> > > > > > > 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@samsung.com>
> > > > > > > > ---
> > > > > > > > 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?
>
On second thoughts, I will add the dwc_pcie_rasdes_debugfs_init and deinit calls in the
dwc common PCIe files but in the probe/remove path.
> >
> > > 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
> >
> > --
> > மணிவண்ணன் சதாசிவம்
On Thu, Jan 16, 2025 at 12:42:23PM +0530, Shradha Todi wrote: [...] > > > > 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? > > Adding the API without any in-kernel consumer is not usually recommended. > > On second thoughts, I will add the dwc_pcie_rasdes_debugfs_init and deinit calls in the > dwc common PCIe files but in the probe/remove path. > Can you please be more specific? There are no probe/remove functions in DWC common drivers. We have init/deinit only. For pcie-designware-host, you can call dwc_pcie_rasdes_debugfs_init() from dw_pcie_host_init() and dwc_pcie_rasdes_debugfs_deinit() from dw_pcie_host_deinit(). But for pcie-designware-ep, you should call them from dw_pcie_ep_init_registers() and dw_pcie_ep_cleanup() since reading/writing to these debugfs files will cause DBI access and that requires active refclk. These APIs are used as a placeholder for code that require refclk to work. - Mani -- மணிவண்ணன் சதாசிவம்
> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 15 January 2025 22:00
> To: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Shradha Todi <shradha.t@samsung.com>; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> fan.ni@samsung.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> quic_krichai@quicinc.com; gost.dev@samsung.com
> 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@kernel.org>
> > > > > > Sent: 06 December 2024 21:43
> > > > > > To: Shradha Todi <shradha.t@samsung.com>
> > > > > > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org;
> > > > > > manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> > > > > > kw@linux.com; robh@kernel.org; bhelgaas@google.com;
> > > > > > jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> > > > > > fan.ni@samsung.com; a.manzanares@samsung.com;
> > > > > > pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> > > > > > quic_krichai@quicinc.com; gost.dev@samsung.com
> > > > > > 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@samsung.com>
> > > > > > > ---
> > > > > > > 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
>
> --
> மணிவண்ணன் சதாசிவம்
On Wed, Jan 15, 2025 at 09:59:53PM +0530, Manivannan Sadhasivam wrote: > ... > (Also, I don't see the VSEC_IDs defined in the DWC reference manual > that I got access to). Haha, yeah, that's because DWC can only define VSEC IDs for devices with PCI_VENDOR_ID_SYNOPSYS, and if they want to sell IP for use in devices with other Vendor IDs, it's up to those vendors to define the VSEC ID in their products. That's exactly the issue here :) Bjorn
Hi Shradha,
kernel test robot noticed the following build warnings:
[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus mani-mhi/mhi-next linus/master v6.13-rc1 next-20241205]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Shradha-Todi/PCI-dwc-Add-support-for-vendor-specific-capability-search/20241206-163620
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20241206074456.17401-2-shradha.t%40samsung.com
patch subject: [PATCH v4 1/2] PCI: dwc: Add support for vendor specific capability search
config: arm64-randconfig-003-20241206 (https://download.01.org/0day-ci/archive/20241206/202412062046.0XR2e2V6-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241206/202412062046.0XR2e2V6-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412062046.0XR2e2V6-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/pci/controller/dwc/pcie-designware.c:15:
In file included from include/linux/dma/edma.h:13:
In file included from include/linux/dmaengine.h:12:
In file included from include/linux/scatterlist.h:8:
In file included from include/linux/mm.h:2223:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/controller/dwc/pcie-designware.c:285:14: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
285 | while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
| ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
286 | PCI_EXT_CAP_ID_VNDR)) {
| ~~~~~~~~~~~~~~~~~~~~
drivers/pci/controller/dwc/pcie-designware.c:285:14: note: place parentheses around the assignment to silence this warning
285 | while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
| ^
| (
286 | PCI_EXT_CAP_ID_VNDR)) {
|
| )
drivers/pci/controller/dwc/pcie-designware.c:285:14: note: use '==' to turn this assignment into an equality comparison
285 | while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
| ^
| ==
5 warnings generated.
vim +285 drivers/pci/controller/dwc/pcie-designware.c
279
280 u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u8 vsec_cap)
281 {
282 u16 vsec = 0;
283 u32 header;
284
> 285 while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
286 PCI_EXT_CAP_ID_VNDR)) {
287 header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
288 if (PCI_VNDR_HEADER_ID(header) == vsec_cap)
289 return vsec;
290 }
291
292 return 0;
293 }
294 EXPORT_SYMBOL_GPL(dw_pcie_find_vsec_capability);
295
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Shradha,
kernel test robot noticed the following build warnings:
[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus mani-mhi/mhi-next linus/master v6.13-rc1 next-20241205]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Shradha-Todi/PCI-dwc-Add-support-for-vendor-specific-capability-search/20241206-163620
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20241206074456.17401-2-shradha.t%40samsung.com
patch subject: [PATCH v4 1/2] PCI: dwc: Add support for vendor specific capability search
config: i386-buildonly-randconfig-006 (https://download.01.org/0day-ci/archive/20241206/202412061940.WSlxZ8i1-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241206/202412061940.WSlxZ8i1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412061940.WSlxZ8i1-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/pci/controller/dwc/pcie-designware.c: In function 'dw_pcie_find_vsec_capability':
>> drivers/pci/controller/dwc/pcie-designware.c:285:16: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
285 | while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
| ^~~~
vim +285 drivers/pci/controller/dwc/pcie-designware.c
279
280 u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u8 vsec_cap)
281 {
282 u16 vsec = 0;
283 u32 header;
284
> 285 while (vsec = dw_pcie_find_next_ext_capability(pci, vsec,
286 PCI_EXT_CAP_ID_VNDR)) {
287 header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
288 if (PCI_VNDR_HEADER_ID(header) == vsec_cap)
289 return vsec;
290 }
291
292 return 0;
293 }
294 EXPORT_SYMBOL_GPL(dw_pcie_find_vsec_capability);
295
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2025 Red Hat, Inc.