2019-03-11 14:58:02

by Marc Gonzalez

[permalink] [raw]
Subject: [RFC PATCH v1] PCI: qcom: Use quirk to override incorrect device class

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 <[email protected]>
---
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;
- }
-
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,
--
2.17.1


2019-03-12 12:43:28

by Stanimir Varbanov

[permalink] [raw]
Subject: Re: [RFC PATCH v1] PCI: qcom: Use quirk to override incorrect device class

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 <[email protected]>
> ---
> 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?

--
regards,
Stan

2019-03-12 17:21:04

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [RFC PATCH v1] PCI: qcom: Use quirk to override incorrect device class

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 <[email protected]>
>> ---
>> 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.

2019-03-12 17:36:32

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [RFC PATCH v1] PCI: qcom: Use quirk to override incorrect device class

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.

2019-03-13 11:46:39

by Marc Gonzalez

[permalink] [raw]
Subject: [PATCH v2] PCI: qcom: Use default config space read function

We don't need to fudge the device class in qcom_pcie_rd_own_conf()
because dw_pcie_setup_rc() already does the right thing:

/* Program correct class for RC */
dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);

Signed-off-by: Marc Gonzalez <[email protected]>
---
Changes from v1 to v2: Completely drop qcom_pcie_rd_own_conf
Srinivas, could you test this patch on apq/ipq8064?
---
drivers/pci/controller/dwc/pcie-qcom.c | 17 -----------------
1 file changed, 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 3de5510fd3d5..d0c5c7616b3e 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1131,25 +1131,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
return ret;
}

-static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
- u32 *val)
-{
- 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
.host_init = qcom_pcie_host_init,
- .rd_own_conf = qcom_pcie_rd_own_conf,
};

/* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
--
2.17.1

2019-03-13 13:14:26

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH v2] PCI: qcom: Use default config space read function

On 13/03/2019 12:45, Marc Gonzalez wrote:

> We don't need to fudge the device class in qcom_pcie_rd_own_conf()
> because dw_pcie_setup_rc() already does the right thing:
>
> /* Program correct class for RC */
> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);

qcom-pcie 1c00000.pci: 1c00000.pci supply vdda not found, using dummy regulator
qcom-pcie 1c00000.pci: 1c00000.pci supply vddpe-3v3 not found, using dummy regulator
qcom-pcie 1c00000.pci: host bridge /soc/pci@1c00000 ranges:
qcom-pcie 1c00000.pci: Parsing ranges property...
qcom-pcie 1c00000.pci: IO 0x1b200000..0x1b2fffff -> 0x1b200000
qcom-pcie 1c00000.pci: MEM 0x1b300000..0x1bffffff -> 0x1b300000
qcom-pcie 1c00000.pci: Link up
qcom-pcie 1c00000.pci: PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [bus 00-ff]
pci_bus 0000:00: root bus resource [io 0x0000-0xfffff] (bus address [0x1b200000-0x1b2fffff])
pci_bus 0000:00: root bus resource [mem 0x1b300000-0x1bffffff]
pci_bus 0000:00: scanning bus
pci 0000:00:00.0: [17cb:0105] type 01 class 0x060400
pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x00000fff 64bit]
pci 0000:00:00.0: PME# supported from D0 D3hot
pci 0000:00:00.0: PME# disabled
pci_bus 0000:00: fixups for bus
pci 0000:00:00.0: scanning [bus 01-ff] behind bridge, pass 0
pci_bus 0000:01: scanning bus
pci 0000:01:00.0: [1969:1083] type 00 class 0x020000
pci 0000:01:00.0: reg 0x10: [mem 0x1b300000-0x1b33ffff 64bit]
pci 0000:01:00.0: reg 0x18: [io 0x1000-0x107f]
pci 0000:01:00.0: PME# supported from D0 D1 D2 D3hot D3cold
pci 0000:01:00.0: PME# disabled
pci_bus 0000:01: fixups for bus
pci_bus 0000:01: bus scan returning with max=01
pci 0000:00:00.0: scanning [bus 01-ff] behind bridge, pass 1
pci_bus 0000:00: bus scan returning with max=ff
pci 0000:00:00.0: BAR 8: assigned [mem 0x1b300000-0x1b3fffff]
pci 0000:00:00.0: BAR 0: assigned [mem 0x1b400000-0x1b400fff 64bit]
pci 0000:00:00.0: BAR 7: assigned [io 0x1000-0x1fff]
pci 0000:01:00.0: BAR 0: assigned [mem 0x1b300000-0x1b33ffff 64bit]
pci 0000:01:00.0: BAR 2: assigned [io 0x1000-0x107f]
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0: bridge window [io 0x1000-0x1fff]
pci 0000:00:00.0: bridge window [mem 0x1b300000-0x1b3fffff]
pci 0000:01:00.0: [Firmware Bug]: disabling VPD access (can't determine size of non-standard VPD format)

# lspci -vvv >tmp
pcilib: sysfs_read_vpd: read failed: Input/output error

# cat tmp
00:00.0 PCI bridge: Qualcomm Device 0105 (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin A routed to IRQ 25
Region 0: Memory at 1b400000 (64-bit, non-prefetchable) [size=4K]
Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
I/O behind bridge: 00001000-00001fff [size=4K]
Memory behind bridge: 1b300000-1b3fffff [size=1M]
Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff [empty]
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold-)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [50] MSI: Enable+ Count=1/32 Maskable+ 64bit+
Address: 00000000fbfff000 Data: 0000
Masking: fffffffe Pending: 00000000
Capabilities: [70] Express (v2) Root Port (Slot-), MSI 00
DevCap: MaxPayload 128 bytes, PhantFunc 0
ExtTag- RBE+
DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
MaxPayload 128 bytes, MaxReadReq 512 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
LnkCap: Port #0, Speed 5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <1us, L1 <16us
ClockPM- Surprise- LLActRep+ BwNot+ ASPMOptComp+
LnkCtl: ASPM Disabled; RCB 128 bytes Disabled- CommClk+
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt-
RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible-
RootCap: CRSVisible-
RootSta: PME ReqID 0000, PMEStatus- PMEPending-
DevCap2: Completion Timeout: Range ABCD, TimeoutDis+, LTR+, OBFF Not Supported ARIFwd-
AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS-
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR+, OBFF Disabled ARIFwd-
AtomicOpsCtl: ReqEn- EgressBlck-
LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis-
Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete-, EqualizationPhase1-
EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
Capabilities: [100 v2] Advanced Error Reporting
UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
HeaderLog: 00000000 00000000 00000000 00000000
RootCmd: CERptEn+ NFERptEn+ FERptEn+
RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd-
FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0
ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000
Capabilities: [148 v1] Transaction Processing Hints
No steering table available
Capabilities: [1dc v1] L1 PM Substates
L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
PortCommonModeRestoreTime=70us PortTPowerOnTime=0us
L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
T_CommonMode=70us LTR1.2_Threshold=0ns
L1SubCtl2: T_PwrOn=10us
Kernel driver in use: pcieport

01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)
Subsystem: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin A routed to IRQ 0
Region 0: Memory at 1b300000 (64-bit, non-prefetchable) [size=256K]
Region 2: I/O ports at 1000 [size=128]
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [48] MSI: Enable- Count=1/1 Maskable- 64bit+
Address: 0000000000000000 Data: 0000
Capabilities: [58] Express (v1) Endpoint, MSI 00
DevCap: MaxPayload 4096 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited
ExtTag- AttnBtn+ AttnInd+ PwrInd+ RBE+ FLReset- SlotPowerLimit 0.000W
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
MaxPayload 128 bytes, MaxReadReq 512 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s unlimited, L1 unlimited
ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp-
LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
Capabilities: [6c] Vital Product Data
Not readable
Capabilities: [100 v1] Advanced Error Reporting
UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UESvrt: DLP- SDES+ TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
HeaderLog: 00000000 00000000 00000000 00000000
Capabilities: [180 v1] Device Serial Number ff-c0-2f-ab-1e-89-4e-ff
Kernel driver in use: atl1c

2019-03-13 18:30:31

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2] PCI: qcom: Use default config space read function

Hi Marc,

On 13/03/2019 11:45, Marc Gonzalez wrote:
> We don't need to fudge the device class in qcom_pcie_rd_own_conf()
> because dw_pcie_setup_rc() already does the right thing:
>
> /* Program correct class for RC */
> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> ---
> Changes from v1 to v2: Completely drop qcom_pcie_rd_own_conf
> Srinivas, could you test this patch on apq/ipq8064?
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 17 -----------------
> 1 file changed, 17 deletions(-)
>
I tried this patch on IFC6410 board which has APQ8064, and the pci does
not work anymore after applying this patch.

non-working output of 'lspci -vvv'
https://paste.ubuntu.com/p/ndmdsW4zkJ/ and bootlog
ahttps://paste.ubuntu.com/p/CqVzGymnq8/

working without patch : https://paste.ubuntu.com/p/TJm4hgjGW4/

I have not debugged it any further other than just testing the patch.
Let me know if you need any more dumps for more debug.

Thanks,
srini

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 3de5510fd3d5..d0c5c7616b3e 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1131,25 +1131,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
> return ret;
> }
>
> -static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> - u32 *val)
> -{
> - 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> .host_init = qcom_pcie_host_init,
> - .rd_own_conf = qcom_pcie_rd_own_conf,
> };
>
> /* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
>

2019-03-13 20:42:38

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH v2] PCI: qcom: Use default config space read function

On 13/03/2019 19:29, Srinivas Kandagatla wrote:

> On 13/03/2019 11:45, Marc Gonzalez wrote:
>
>> We don't need to fudge the device class in qcom_pcie_rd_own_conf()
>> because dw_pcie_setup_rc() already does the right thing:
>>
>> /* Program correct class for RC */
>> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>>
>> Signed-off-by: Marc Gonzalez <[email protected]>
>> ---
>> Changes from v1 to v2: Completely drop qcom_pcie_rd_own_conf
>> Srinivas, could you test this patch on apq/ipq8064?
>> ---
>> drivers/pci/controller/dwc/pcie-qcom.c | 17 -----------------
>> 1 file changed, 17 deletions(-)
>>
> I tried this patch on IFC6410 board which has APQ8064, and the pci does
> not work anymore after applying this patch.
>
> non-working output of 'lspci -vvv'
> https://paste.ubuntu.com/p/ndmdsW4zkJ/ and bootlog
> https://paste.ubuntu.com/p/CqVzGymnq8/
>
> working without patch : https://paste.ubuntu.com/p/TJm4hgjGW4/
>
> I have not debugged it any further other than just testing the patch.
> Let me know if you need any more dumps for more debug.

Could you pastebin the output of lspci -vvv -n in the working case?

I think that programming the device class might not work on APQ8064,
and therefore that platform requires the quirk.

So maybe I can add the quirk back, just for APQ8064?

Regards.

2019-03-13 21:53:23

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2] PCI: qcom: Use default config space read function



On 13/03/2019 20:39, Marc Gonzalez wrote:
>> working without patch :https://paste.ubuntu.com/p/TJm4hgjGW4/
>>
>> I have not debugged it any further other than just testing the patch.
>> Let me know if you need any more dumps for more debug.
> Could you pastebin the output of lspci -vvv -n in the working case?
This was already in my original reply:
working without patch : https://paste.ubuntu.com/p/TJm4hgjGW4/

--srini

2019-03-13 22:27:08

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH v2] PCI: qcom: Use default config space read function

On 13/03/2019 22:52, Srinivas Kandagatla wrote:

> On 13/03/2019 20:39, Marc Gonzalez wrote:
>
>> Could you pastebin the output of lspci -vvv -n in the working case?
>
> This was already in my original reply:
> working without patch : https://paste.ubuntu.com/p/TJm4hgjGW4/

I needed to see the numeric values, hence the -n flag ;-)

lspci -vvv -n

Regards.

2019-03-14 12:01:55

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2] PCI: qcom: Use default config space read function



On 13/03/2019 22:26, Marc Gonzalez wrote:
> I needed to see the numeric values, hence the -n flag;-)
>
> lspci -vvv -n
>

Here is the output: https://paste.ubuntu.com/p/kFWZ4kXCxT/

Thanks,
srini

> Regards.

2019-03-14 13:20:15

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH v2] PCI: qcom: Use default config space read function

On 14/03/2019 12:10, Srinivas Kandagatla wrote:

> Here is the output: https://paste.ubuntu.com/p/kFWZ4kXCxT/

Could you test the following patch:
And report boot log as well as output of lspci -vv ?

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 45ff5e4f8af6..aca656eff8ca 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -727,7 +727,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
/* Enable write permission for the DBI read-only register */
dw_pcie_dbi_ro_wr_en(pci);
/* Program correct class for RC */
+ dw_pcie_rd_own_conf(pp, PCI_CLASS_REVISION, 4, &val);
+ printk("\tBEFORE: CLASS/REV=%08x\n", val);
dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
+ dw_pcie_rd_own_conf(pp, PCI_CLASS_REVISION, 4, &val);
+ printk("\t AFTER: CLASS/REV=%08x\n", val);
/* Better disable write permission right after the update */
dw_pcie_dbi_ro_wr_dis(pci);

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d185ea5fe996..c5e16f9aaf44 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
return ret;
}

-static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
- u32 *val)
-{
- 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
.host_init = qcom_pcie_host_init,
- .rd_own_conf = qcom_pcie_rd_own_conf,
};

/* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
@@ -1299,6 +1282,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
return ret;
}

+static void qcom_fixup_class(struct pci_dev *dev)
+{
+ printk("\tAPPLYING %s\n", __func__);
+ dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0101, qcom_fixup_class); // 8064
+
static const struct of_device_id qcom_pcie_match[] = {
{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },

2019-03-18 14:29:49

by Marc Gonzalez

[permalink] [raw]
Subject: [PATCH v3] PCI: qcom: Use default config space read function

We don't need to fudge the device class in qcom_pcie_rd_own_conf()
because dw_pcie_setup_rc() already does the right thing:

/* Program correct class for RC */
dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);

However, the above has no effect on 8064, thus a fixup is required.

Signed-off-by: Marc Gonzalez <[email protected]>
---
Changes from v2 to v3: Add fixup for 8064 quirk
Changes from v1 to v2: Completely drop qcom_pcie_rd_own_conf

Stanimir, could you test this submission and send a Tested-by if it works?
---
drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d185ea5fe996..6fefff106b87 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
return ret;
}

-static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
- u32 *val)
-{
- 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
.host_init = qcom_pcie_host_init,
- .rd_own_conf = qcom_pcie_rd_own_conf,
};

/* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
@@ -1299,6 +1282,12 @@ static int qcom_pcie_probe(struct platform_device *pdev)
return ret;
}

+static void qcom_fixup_class(struct pci_dev *dev)
+{
+ dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0101, qcom_fixup_class); // 8064
+
static const struct of_device_id qcom_pcie_match[] = {
{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
--
2.17.1

2019-03-18 14:40:42

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v3] PCI: qcom: Use default config space read function



On 18/03/2019 14:28, Marc Gonzalez wrote:
> We don't need to fudge the device class in qcom_pcie_rd_own_conf()
> because dw_pcie_setup_rc() already does the right thing:
>
> /* Program correct class for RC */
> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>
> However, the above has no effect on 8064, thus a fixup is required.
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> ---
> Changes from v2 to v3: Add fixup for 8064 quirk

Tested this on IFC6410 which is based of APQ8064.

Tested-by: Srinivas Kandagatla <[email protected]>

Thanks,
srini

2019-03-18 16:37:44

by Stanimir Varbanov

[permalink] [raw]
Subject: Re: [PATCH v3] PCI: qcom: Use default config space read function

Hi Marc,

Thanks for the patch!

On 3/18/19 4:28 PM, Marc Gonzalez wrote:
> We don't need to fudge the device class in qcom_pcie_rd_own_conf()
> because dw_pcie_setup_rc() already does the right thing:
>
> /* Program correct class for RC */
> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>
> However, the above has no effect on 8064, thus a fixup is required.
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> ---
> Changes from v2 to v3: Add fixup for 8064 quirk
> Changes from v1 to v2: Completely drop qcom_pcie_rd_own_conf
>
> Stanimir, could you test this submission and send a Tested-by if it works?
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++-----------------
> 1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index d185ea5fe996..6fefff106b87 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
> return ret;
> }
>
> -static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> - u32 *val)
> -{
> - 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> .host_init = qcom_pcie_host_init,
> - .rd_own_conf = qcom_pcie_rd_own_conf,
> };
>
> /* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
> @@ -1299,6 +1282,12 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> +static void qcom_fixup_class(struct pci_dev *dev)
> +{
> + dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0101, qcom_fixup_class); // 8064

Please make 0x101 a define, and drop //

I will try to find device IDs for ipq8064 and ipq4019.

--
regards,
Stan

2019-03-18 17:16:12

by Marc Gonzalez

[permalink] [raw]
Subject: [PATCH v4] PCI: qcom: Use default config space read function

We don't need to fudge the device class in qcom_pcie_rd_own_conf()
because dw_pcie_setup_rc() already does the right thing:

/* Program correct class for RC */
dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);

However, the above has no effect on 8064, thus a fixup is required.

Signed-off-by: Marc Gonzalez <[email protected]>
Tested-by: Srinivas Kandagatla <[email protected]>
---
Changes from v3 to v4: Define and use DEV_ID_8064 (not in include/linux/pci_ids.h because not shared)
---
drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d185ea5fe996..712a83354f9d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
return ret;
}

-static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
- u32 *val)
-{
- 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
.host_init = qcom_pcie_host_init,
- .rd_own_conf = qcom_pcie_rd_own_conf,
};

/* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
@@ -1309,6 +1292,14 @@ static const struct of_device_id qcom_pcie_match[] = {
{ }
};

+#define DEV_ID_8064 0x0101
+
+static void qcom_fixup_class(struct pci_dev *dev)
+{
+ dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, DEV_ID_8064, qcom_fixup_class);
+
static struct platform_driver qcom_pcie_driver = {
.probe = qcom_pcie_probe,
.driver = {
--
2.17.1

2019-03-25 12:12:15

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH v4] PCI: qcom: Use default config space read function

Stanimir,

Is v4 good enough for Bjorn to pick up?

Regards.

On 18/03/2019 18:14, Marc Gonzalez wrote:

> We don't need to fudge the device class in qcom_pcie_rd_own_conf()
> because dw_pcie_setup_rc() already does the right thing:
>
> /* Program correct class for RC */
> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>
> However, the above has no effect on 8064, thus a fixup is required.
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> Tested-by: Srinivas Kandagatla <[email protected]>
> ---
> Changes from v3 to v4: Define and use DEV_ID_8064 (not in include/linux/pci_ids.h because not shared)
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++-----------------
> 1 file changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index d185ea5fe996..712a83354f9d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
> return ret;
> }
>
> -static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> - u32 *val)
> -{
> - 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> .host_init = qcom_pcie_host_init,
> - .rd_own_conf = qcom_pcie_rd_own_conf,
> };
>
> /* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
> @@ -1309,6 +1292,14 @@ static const struct of_device_id qcom_pcie_match[] = {
> { }
> };
>
> +#define DEV_ID_8064 0x0101
> +
> +static void qcom_fixup_class(struct pci_dev *dev)
> +{
> + dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, DEV_ID_8064, qcom_fixup_class);
> +
> static struct platform_driver qcom_pcie_driver = {
> .probe = qcom_pcie_probe,
> .driver = {

2019-03-25 13:34:42

by Stanimir Varbanov

[permalink] [raw]
Subject: Re: [PATCH v4] PCI: qcom: Use default config space read function

Hi Marc,

On 3/25/19 2:11 PM, Marc Gonzalez wrote:
> Stanimir,
>
> Is v4 good enough for Bjorn to pick up?

Yes it is good but to avoid breaking another SoCs could you add fixups
for the following SoCs:

SoC device ID
ipq4019 0x1001
ipq8064 0x101
ipq8074 0x108

ipq8064 has the same device ID as apq8064, but I'm not sure do we need
defines per SoC or just rename DEV_ID_8064 ? I'm fine with both ways.

>
> Regards.
>
> On 18/03/2019 18:14, Marc Gonzalez wrote:
>
>> We don't need to fudge the device class in qcom_pcie_rd_own_conf()
>> because dw_pcie_setup_rc() already does the right thing:
>>
>> /* Program correct class for RC */
>> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>>
>> However, the above has no effect on 8064, thus a fixup is required.
>>
>> Signed-off-by: Marc Gonzalez <[email protected]>
>> Tested-by: Srinivas Kandagatla <[email protected]>
>> ---
>> Changes from v3 to v4: Define and use DEV_ID_8064 (not in include/linux/pci_ids.h because not shared)
>> ---
>> drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++-----------------
>> 1 file changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index d185ea5fe996..712a83354f9d 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>> return ret;
>> }
>>
>> -static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>> - u32 *val)
>> -{
>> - 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
>> .host_init = qcom_pcie_host_init,
>> - .rd_own_conf = qcom_pcie_rd_own_conf,
>> };
>>
>> /* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
>> @@ -1309,6 +1292,14 @@ static const struct of_device_id qcom_pcie_match[] = {
>> { }
>> };
>>
>> +#define DEV_ID_8064 0x0101
>> +
>> +static void qcom_fixup_class(struct pci_dev *dev)
>> +{
>> + dev->class = PCI_CLASS_BRIDGE_PCI << 8;
>> +}
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, DEV_ID_8064, qcom_fixup_class);
>> +
>> static struct platform_driver qcom_pcie_driver = {
>> .probe = qcom_pcie_probe,
>> .driver = {

--
regards,
Stan

2019-03-25 15:43:59

by Marc Gonzalez

[permalink] [raw]
Subject: [PATCH v5] PCI: qcom: Use default config space read function

Move the device class fudge to a proper fixup function, and remove
qcom_pcie_rd_own_conf() which has become useless.

NB: dw_pcie_setup_rc() already did the right thing, but it's broken
on older qcom chips, such as 8064.

Signed-off-by: Marc Gonzalez <[email protected]>
---
Changes from v4 to v5: Apply fixup to all qcom chips, the same way it was before
(thus the code remains functionally equivalent)
Drop Srinivas' Tested-by tag because of the change
---
drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index a7f703556790..0ed235d560e3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
return ret;
}

-static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
- u32 *val)
-{
- 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
.host_init = qcom_pcie_host_init,
- .rd_own_conf = qcom_pcie_rd_own_conf,
};

/* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
@@ -1309,6 +1292,12 @@ static const struct of_device_id qcom_pcie_match[] = {
{ }
};

+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 struct platform_driver qcom_pcie_driver = {
.probe = qcom_pcie_probe,
.driver = {
--
2.17.1

2019-03-29 13:49:03

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH v5] PCI: qcom: Use default config space read function

Bjorn,

I think this patch is good enough to land now.

Regards.

On 25/03/2019 16:42, Marc Gonzalez wrote:

> Move the device class fudge to a proper fixup function, and remove
> qcom_pcie_rd_own_conf() which has become useless.
>
> NB: dw_pcie_setup_rc() already did the right thing, but it's broken
> on older qcom chips, such as 8064.
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> ---
> Changes from v4 to v5: Apply fixup to all qcom chips, the same way it was before
> (thus the code remains functionally equivalent)
> Drop Srinivas' Tested-by tag because of the change
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++-----------------
> 1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index a7f703556790..0ed235d560e3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
> return ret;
> }
>
> -static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> - u32 *val)
> -{
> - 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> .host_init = qcom_pcie_host_init,
> - .rd_own_conf = qcom_pcie_rd_own_conf,
> };
>
> /* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
> @@ -1309,6 +1292,12 @@ static const struct of_device_id qcom_pcie_match[] = {
> { }
> };
>
> +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 struct platform_driver qcom_pcie_driver = {
> .probe = qcom_pcie_probe,
> .driver = {

2019-03-29 15:56:24

by Stanimir Varbanov

[permalink] [raw]
Subject: Re: [PATCH v5] PCI: qcom: Use default config space read function

Hi Marc,

Thanks for the patch!

On 3/25/19 5:42 PM, Marc Gonzalez wrote:
> Move the device class fudge to a proper fixup function, and remove
> qcom_pcie_rd_own_conf() which has become useless.
>
> NB: dw_pcie_setup_rc() already did the right thing, but it's broken
> on older qcom chips, such as 8064.
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> ---
> Changes from v4 to v5: Apply fixup to all qcom chips, the same way it was before
> (thus the code remains functionally equivalent)
> Drop Srinivas' Tested-by tag because of the change
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++-----------------
> 1 file changed, 6 insertions(+), 17 deletions(-)

Acked-by: Stanimir Varbanov <[email protected]>


--
regards,
Stan

2019-03-29 16:39:10

by Lorenzo Pieralisi

[permalink] [raw]
Subject: Re: [PATCH v5] PCI: qcom: Use default config space read function

On Mon, Mar 25, 2019 at 04:42:55PM +0100, Marc Gonzalez wrote:
> Move the device class fudge to a proper fixup function, and remove
> qcom_pcie_rd_own_conf() which has become useless.
>
> NB: dw_pcie_setup_rc() already did the right thing, but it's broken
> on older qcom chips, such as 8064.
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> ---
> Changes from v4 to v5: Apply fixup to all qcom chips, the same way it was before
> (thus the code remains functionally equivalent)
> Drop Srinivas' Tested-by tag because of the change
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++-----------------
> 1 file changed, 6 insertions(+), 17 deletions(-)

Applied to pci/dwc for v5.2, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index a7f703556790..0ed235d560e3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1129,25 +1129,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
> return ret;
> }
>
> -static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> - u32 *val)
> -{
> - 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 const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> .host_init = qcom_pcie_host_init,
> - .rd_own_conf = qcom_pcie_rd_own_conf,
> };
>
> /* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
> @@ -1309,6 +1292,12 @@ static const struct of_device_id qcom_pcie_match[] = {
> { }
> };
>
> +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 struct platform_driver qcom_pcie_driver = {
> .probe = qcom_pcie_probe,
> .driver = {
> --
> 2.17.1

2019-04-30 14:09:37

by Lorenzo Pieralisi

[permalink] [raw]
Subject: Re: [RFC PATCH v1] PCI: qcom: Use quirk to override incorrect device class

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

2019-05-02 09:43:49

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [RFC PATCH v1] PCI: qcom: Use quirk to override incorrect device class

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.