| Message ID | 94bb3f22-c5a7-1891-9d89-42a520e9a592@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [RFC,v1] PCI: qcom: Use quirk to override incorrect device class | expand |
Hi Marc, Thanks for the patch! On 3/11/19 4:56 PM, Marc Gonzalez wrote: > Some chips report an incorrect device class. Override the incorrect > value using a quirk, instead of code in the read function. > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > FWIW, this quirk is no longer required on recent chips: > msm8996 (tested by Stanimir), msm8998 (tested by me), sdm845 (untested) are unaffected > apq/ipq8064 is affected => what is the device ID for these chips? > others? > > Stanimir added: "this will become a real problem (now we use the driver as RC) > when someone decide to use it as an endpoint" > --- > drivers/pci/controller/dwc/pcie-qcom.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 3de5510fd3d5..94da2c9c2ad5 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1136,17 +1136,15 @@ static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > - /* the device class is not reported correctly from the register */ > - if (where == PCI_CLASS_REVISION && size == 4) { > - *val = readl(pci->dbi_base + PCI_CLASS_REVISION); > - *val &= 0xff; /* keep revision id */ > - *val |= PCI_CLASS_BRIDGE_PCI << 16; > - return PCIBIOS_SUCCESSFUL; > - } > - once you dropped the above snippet this function becomes absolutely useless so please delete it at all and also from qcom_pcie_dw_ops. > return dw_pcie_read(pci->dbi_base + where, size, val); > } > > +static void qcom_fixup_class(struct pci_dev *dev) > +{ > + dev->class = PCI_CLASS_BRIDGE_PCI << 8; > +} > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, PCI_ANY_ID, qcom_fixup_class); I wonder, in case that dw_pcie_setup_rc() already has a write to PCI_CLASS_DEVICE configuration register to set it as a bridge do we still need to do the above fixup?
On 12/03/2019 13:42, Stanimir Varbanov wrote: > On 3/11/19 4:56 PM, Marc Gonzalez wrote: > >> Some chips report an incorrect device class. Override the incorrect >> value using a quirk, instead of code in the read function. >> >> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> >> --- >> FWIW, this quirk is no longer required on recent chips: >> msm8996 (tested by Stanimir), msm8998 (tested by me), sdm845 (untested) are unaffected >> apq/ipq8064 is affected => what is the device ID for these chips? >> others? >> >> Stanimir added: "this will become a real problem (now we use the driver as RC) >> when someone decide to use it as an endpoint" >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index 3de5510fd3d5..94da2c9c2ad5 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -1136,17 +1136,15 @@ static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, >> { >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> >> - /* the device class is not reported correctly from the register */ >> - if (where == PCI_CLASS_REVISION && size == 4) { >> - *val = readl(pci->dbi_base + PCI_CLASS_REVISION); >> - *val &= 0xff; /* keep revision id */ >> - *val |= PCI_CLASS_BRIDGE_PCI << 16; >> - return PCIBIOS_SUCCESSFUL; >> - } >> - > > once you dropped the above snippet this function becomes absolutely > useless so please delete it at all and also from qcom_pcie_dw_ops. Good catch. >> return dw_pcie_read(pci->dbi_base + where, size, val); >> } >> >> +static void qcom_fixup_class(struct pci_dev *dev) >> +{ >> + dev->class = PCI_CLASS_BRIDGE_PCI << 8; >> +} >> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, PCI_ANY_ID, qcom_fixup_class); > > I wonder, in case that dw_pcie_setup_rc() already has a write to > PCI_CLASS_DEVICE configuration register to set it as a bridge do we > still need to do the above fixup? I don't know, I don't have an affected device. Unless the msm8998 /is/ affected, and dw_pcie_setup_rc() actually fixes it? Regards.
On 12/03/2019 18:18, Marc Gonzalez wrote: > On 12/03/2019 13:42, Stanimir Varbanov wrote: > >> I wonder, in case that dw_pcie_setup_rc() already has a write to >> PCI_CLASS_DEVICE configuration register to set it as a bridge do we >> still need to do the above fixup? > > I don't know, I don't have an affected device. Unless the msm8998 /is/ affected, > and dw_pcie_setup_rc() actually fixes it? I think you hit the nail on the head... If I comment out //dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); from dw_pcie_setup_rc() then pci_class() returns 0xff000000 instead of 0x6040000 So perhaps you're right: the quirk can be omitted altogether. Unless it is not possible to program the device class on older chips? Regards.
On Tue, Mar 12, 2019 at 06:34:55PM +0100, Marc Gonzalez wrote: > On 12/03/2019 18:18, Marc Gonzalez wrote: > > > On 12/03/2019 13:42, Stanimir Varbanov wrote: > > > >> I wonder, in case that dw_pcie_setup_rc() already has a write to > >> PCI_CLASS_DEVICE configuration register to set it as a bridge do we > >> still need to do the above fixup? > > > > I don't know, I don't have an affected device. Unless the msm8998 /is/ affected, > > and dw_pcie_setup_rc() actually fixes it? > > I think you hit the nail on the head... > > If I comment out > //dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); > from dw_pcie_setup_rc() > then pci_class() returns 0xff000000 instead of 0x6040000 > > So perhaps you're right: the quirk can be omitted altogether. > Unless it is not possible to program the device class on older chips? Marc, I would drop this patch from the PCI queue since in a different form it was already merged, please let me know if I am wrong. Thanks, Lorenzo
On 30/04/2019 16:06, Lorenzo Pieralisi wrote: > On Tue, Mar 12, 2019 at 06:34:55PM +0100, Marc Gonzalez wrote: > >> On 12/03/2019 18:18, Marc Gonzalez wrote: >> >>> On 12/03/2019 13:42, Stanimir Varbanov wrote: >>> >>>> I wonder, in case that dw_pcie_setup_rc() already has a write to >>>> PCI_CLASS_DEVICE configuration register to set it as a bridge do we >>>> still need to do the above fixup? >>> >>> I don't know, I don't have an affected device. Unless the msm8998 /is/ affected, >>> and dw_pcie_setup_rc() actually fixes it? >> >> I think you hit the nail on the head... >> >> If I comment out >> //dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); >> from dw_pcie_setup_rc() >> then pci_class() returns 0xff000000 instead of 0x6040000 >> >> So perhaps you're right: the quirk can be omitted altogether. >> Unless it is not possible to program the device class on older chips? > > Marc, > > I would drop this patch from the PCI queue since in a different > form it was already merged, please let me know if I am wrong. I'm confused because you speak of *dropping* this patch (v1) -- but v1 was never picked up AFAIK. You picked up v5 on March 29: https://patchwork.kernel.org/patch/10869519/ I see it in linux-next as 915347f67d41857a514bed77053b212f3696e8a3 Will Bjorn send it to LT during the merge window for 5.2? Regards.
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 3de5510fd3d5..94da2c9c2ad5 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -1136,17 +1136,15 @@ static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); - /* the device class is not reported correctly from the register */ - if (where == PCI_CLASS_REVISION && size == 4) { - *val = readl(pci->dbi_base + PCI_CLASS_REVISION); - *val &= 0xff; /* keep revision id */ - *val |= PCI_CLASS_BRIDGE_PCI << 16; - return PCIBIOS_SUCCESSFUL; - } - return dw_pcie_read(pci->dbi_base + where, size, val); } +static void qcom_fixup_class(struct pci_dev *dev) +{ + dev->class = PCI_CLASS_BRIDGE_PCI << 8; +} +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, PCI_ANY_ID, qcom_fixup_class); + static const struct dw_pcie_host_ops qcom_pcie_dw_ops = { .host_init = qcom_pcie_host_init, .rd_own_conf = qcom_pcie_rd_own_conf,
Some chips report an incorrect device class. Override the incorrect value using a quirk, instead of code in the read function. Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- FWIW, this quirk is no longer required on recent chips: msm8996 (tested by Stanimir), msm8998 (tested by me), sdm845 (untested) are unaffected apq/ipq8064 is affected => what is the device ID for these chips? others? Stanimir added: "this will become a real problem (now we use the driver as RC) when someone decide to use it as an endpoint" --- drivers/pci/controller/dwc/pcie-qcom.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)