* [PATCH 0/3] PCI: emulated PCI bridge common logic
@ 2018-06-29 9:22 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-06-29 9:22 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
The pci-mvebu driver already contains some logic to emulate a root
port PCI bridge. It turns out that we have a similar need for the
pci-aardvark driver. Instead of duplicating the same logic in two
drivers, this patch series starts by adding a small common
infrastructure that helps emulate a root port PCI bridge, converts
pci-mvebu to use it, and finally extends pci-aardvark to use it as
well.
Thanks to this, Marvell Armada 3720 based systems, which use the
Aarkvark PCI controller, will have better PCI support, by having a
root port PCI bridge exposed.
The emulated PCI bridge common logic is a proposal, I very much
welcome comments and suggestions. Also, if you feel that adding a
common logic for only two drivers is too early, I'm fine with
duplicating a bit of code betwen pci-mvebu and pci-aardvark.
Best regards,
Thomas
Thomas Petazzoni (2):
PCI: Introduce PCI software bridge common logic
PCI: mvebu: Convert to PCI software bridge
Zachary Zhang (1):
PCI: aardvark: Implement emulated root PCI bridge
drivers/pci/Kconfig | 3 +
drivers/pci/Makefile | 1 +
drivers/pci/controller/Kconfig | 2 +
drivers/pci/controller/pci-aardvark.c | 119 ++++++++++-
drivers/pci/controller/pci-mvebu.c | 370 ++++++++++------------------------
drivers/pci/pci-sw-bridge.c | 149 ++++++++++++++
include/linux/pci-sw-bridge.h | 125 ++++++++++++
7 files changed, 497 insertions(+), 272 deletions(-)
create mode 100644 drivers/pci/pci-sw-bridge.c
create mode 100644 include/linux/pci-sw-bridge.h
--
2.14.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/3] PCI: Introduce PCI software bridge common logic
2018-06-29 9:22 ` Thomas Petazzoni
@ 2018-06-29 9:22 ` Thomas Petazzoni
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-06-29 9:22 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci
Cc: Russell King, Antoine Tenart, Gregory Clement, Maxime Chevallier,
Nadav Haklai, Victor Gu, Thomas Petazzoni, Miquèl Raynal,
Zachary Zhang, Wilson Ding, linux-arm-kernel
U29tZSBQQ0kgaG9zdCBjb250cm9sbGVycyBkbyBub3QgZXhwb3NlIGF0IHRoZSBoYXJkd2FyZSBs
ZXZlbCBhIHJvb3QKcG9ydCBQQ0kgYnJpZGdlLiBEdWUgdG8gdGhpcywgdGhlIE1hcnZlbGwgQXJt
YWRhIDM3MC8zOHgvWFAgUENJCmNvbnRyb2xsZXIgZHJpdmVyIChwY2ktbXZlYnUpIGVtdWxhdGVz
IGEgcm9vdCBwb3J0IFBDSSBicmlkZ2UsIGFuZAp1c2VzIHRoYXQgdG8gKGFtb25nIG90aGVyIHRo
aW5nc8OgKSBkeW5hbWljYWxseSBjcmVhdGUgdGhlIG1lbW9yeQp3aW5kb3dzIHRoYXQgY29ycmVz
cG9uZCB0byB0aGUgUENJIE1FTSBhbmQgSS9PIHJlZ2lvbnMuCgpTaW5jZSB3ZSBub3cgbmVlZCB0
byBhZGQgYSB2ZXJ5IHNpbWlsYXIgbG9naWMgZm9yIHRoZSBNYXJ2ZWxsIEFybWFkYQozN3h4IFBD
SSBjb250cm9sbGVyIGRyaXZlciAocGNpLWFhcmR2YXJrKSwgaW5zdGVhZCBvZiBkdXBsaWNhdGlu
ZyB0aGUKY29kZSwgd2UgY3JlYXRlIGluIHRoaXMgY29tbWl0IGEgY29tbW9uIGxvZ2ljIGNhbGxl
ZCBwY2ktc3ctYnJpZGdlLgoKVGhlIGlkZWEgb2YgdGhpcyBsb2dpYyBpcyB0byBlbXVsYXRlIGEg
cm9vdCBwb3J0IFBDSSBicmlkZ2UgYnkKcHJvdmlkaW5nIGNvbmZpZ3VyYXRpb24gc3BhY2UgcmVh
ZC93cml0ZSBvcGVyYXRpb25zLCBhbmQgZmFraW5nIGJlaGluZAp0aGUgc2NlbmVzIHRoZSBjb25m
aWd1cmF0aW9uIHNwYWNlIG9mIGEgUENJIGJyaWRnZS4gQSBQQ0kgaG9zdApjb250cm9sbGVyIGRy
aXZlciBzaW1wbHkgaGFzIHRvIGNhbGwgcGNpX3N3X2JyaWRnZV9yZWFkKCkgYW5kCnBjaV9zd19i
cmlkZ2Vfd3JpdGUoKSB0byByZWFkL3dyaXRlIHRoZSBjb25maWd1cmF0aW9uIHNwYWNlIG9mIHRo
ZQpicmlkZ2UuCgpCeSBkZWZhdWx0LCB0aGUgUENJIGJyaWRnZSBjb25maWd1cmF0aW9uIHNwYWNl
IGlzIHNpbXBseSBlbXVsYXRlZCBieSBhCmNodW5rIG9mIG1lbW9yeSwgYnV0IHRoZSBQQ0kgaG9z
dCBjb250cm9sbGVyIGNhbiBvdmVycmlkZSB0aGUgYmVoYXZpb3IKb2YgdGhlIHJlYWQgYW5kIHdy
aXRlIG9wZXJhdGlvbnMgb24gYSBwZXItcmVnaXN0ZXIgYmFzaXMgdG8gZG8KYWRkaXRpb25hbCBh
Y3Rpb25zIGlmIG5lZWRlZC4KClNpZ25lZC1vZmYtYnk6IFRob21hcyBQZXRhenpvbmkgPHRob21h
cy5wZXRhenpvbmlAYm9vdGxpbi5jb20+Ci0tLQogZHJpdmVycy9wY2kvS2NvbmZpZyAgICAgICAg
ICAgfCAgIDMgKwogZHJpdmVycy9wY2kvTWFrZWZpbGUgICAgICAgICAgfCAgIDEgKwogZHJpdmVy
cy9wY2kvcGNpLXN3LWJyaWRnZS5jICAgfCAxNDkgKysrKysrKysrKysrKysrKysrKysrKysrKysr
KysrKysrKysrKysrKysrCiBpbmNsdWRlL2xpbnV4L3BjaS1zdy1icmlkZ2UuaCB8IDEyNSArKysr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKwogNCBmaWxlcyBjaGFuZ2VkLCAyNzggaW5z
ZXJ0aW9ucygrKQogY3JlYXRlIG1vZGUgMTAwNjQ0IGRyaXZlcnMvcGNpL3BjaS1zdy1icmlkZ2Uu
YwogY3JlYXRlIG1vZGUgMTAwNjQ0IGluY2x1ZGUvbGludXgvcGNpLXN3LWJyaWRnZS5oCgpkaWZm
IC0tZ2l0IGEvZHJpdmVycy9wY2kvS2NvbmZpZyBiL2RyaXZlcnMvcGNpL0tjb25maWcKaW5kZXgg
NTZmZjhmNmQzMWZjLi4xZjEzNTc1ZDA1MmIgMTAwNjQ0Ci0tLSBhL2RyaXZlcnMvcGNpL0tjb25m
aWcKKysrIGIvZHJpdmVycy9wY2kvS2NvbmZpZwpAQCAtOTgsNiArOTgsOSBAQCBjb25maWcgUENJ
X0VDQU0KIGNvbmZpZyBQQ0lfTE9DS0xFU1NfQ09ORklHCiAJYm9vbAogCitjb25maWcgUENJX1NX
X0JSSURHRQorCWJvb2wKKwogY29uZmlnIFBDSV9JT1YKIAlib29sICJQQ0kgSU9WIHN1cHBvcnQi
CiAJZGVwZW5kcyBvbiBQQ0kKZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL01ha2VmaWxlIGIvZHJp
dmVycy9wY2kvTWFrZWZpbGUKaW5kZXggNTM1MjAxOTg0YjhiLi4wYmQ2NWRkZDJjNTAgMTAwNjQ0
Ci0tLSBhL2RyaXZlcnMvcGNpL01ha2VmaWxlCisrKyBiL2RyaXZlcnMvcGNpL01ha2VmaWxlCkBA
IC0xOSw2ICsxOSw3IEBAIG9iai0kKENPTkZJR19IT1RQTFVHX1BDSSkJKz0gaG90cGx1Zy8KIG9i
ai0kKENPTkZJR19QQ0lfTVNJKQkJKz0gbXNpLm8KIG9iai0kKENPTkZJR19QQ0lfQVRTKQkJKz0g
YXRzLm8KIG9iai0kKENPTkZJR19QQ0lfSU9WKQkJKz0gaW92Lm8KK29iai0kKENPTkZJR19QQ0lf
U1dfQlJJREdFKQkrPSBwY2ktc3ctYnJpZGdlLm8KIG9iai0kKENPTkZJR19BQ1BJKQkJKz0gcGNp
LWFjcGkubwogb2JqLSQoQ09ORklHX1BDSV9MQUJFTCkJCSs9IHBjaS1sYWJlbC5vCiBvYmotJChD
T05GSUdfWDg2X0lOVEVMX01JRCkJKz0gcGNpLW1pZC5vCmRpZmYgLS1naXQgYS9kcml2ZXJzL3Bj
aS9wY2ktc3ctYnJpZGdlLmMgYi9kcml2ZXJzL3BjaS9wY2ktc3ctYnJpZGdlLmMKbmV3IGZpbGUg
bW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwLi4zMDRlOGMwZTE2NGQKLS0tIC9kZXYvbnVs
bAorKysgYi9kcml2ZXJzL3BjaS9wY2ktc3ctYnJpZGdlLmMKQEAgLTAsMCArMSwxNDkgQEAKKy8v
IFNQRFgtTGljZW5zZS1JZGVudGlmaWVyOiBHUEwtMi4wCisvKgorICogQ29weXJpZ2h0IChDKSAy
MDE4IE1hcnZlbGwKKyAqCisgKiBBdXRob3I6IFRob21hcyBQZXRhenpvbmkgPHRob21hcy5wZXRh
enpvbmlAYm9vdGxpbi5jb20+CisgKgorICogVGhpcyBmaWxlIGhlbHBzIFBDSSBjb250cm9sbGVy
IGRyaXZlcnMgaW1wbGVtZW50IGEgZmFrZSByb290IHBvcnQKKyAqIFBDSSBicmlkZ2Ugd2hlbiB0
aGUgSFcgZG9lc24ndCBwcm92aWRlIHN1Y2ggYSByb290IHBvcnQgUENJCisgKiBicmlkZ2UuCisg
KgorICogSXQgZW11bGF0ZXMgYSBQQ0kgYnJpZGdlIGJ5IHByb3ZpZGluZyBhIGZha2UgUENJIGNv
bmZpZ3VyYXRpb24KKyAqIHNwYWNlIChhbmQgb3B0aW9uYWxseSBhIFBDSWUgY2FwYWJpbGl0eSBj
b25maWd1cmF0aW9uIHNwYWNlKSBpbgorICogbWVtb3J5LiBCeSBkZWZhdWx0IHRoZSByZWFkL3dy
aXRlIG9wZXJhdGlvbnMgc2ltcGx5IHJlYWQgYW5kIHVwZGF0ZQorICogdGhpcyBmYWtlIGNvbmZp
Z3VyYXRpb24gc3BhY2UgaW4gbWVtb3J5LiBIb3dldmVyLCBQQ0kgY29udHJvbGxlcgorICogZHJp
dmVycyBjYW4gcHJvdmlkZSB0aHJvdWdoIHRoZSAnc3RydWN0IHBjaV9zd19icmlkZ2Vfb3BzJwor
ICogc3RydWN0dXJlIGEgc2V0IG9mIG9wZXJhdGlvbnMgdG8gb3ZlcnJpZGUgb3IgY29tcGxlbWVu
dCB0aGlzCisgKiBkZWZhdWx0IGJlaGF2aW9yLgorICovCisKKyNpbmNsdWRlIDxsaW51eC9wY2kt
c3ctYnJpZGdlLmg+CisjaW5jbHVkZSA8bGludXgvcGNpLmg+CisKKyNkZWZpbmUgUENJX0JSSURH
RV9DT05GX0VORAkoUENJX0JSSURHRV9DT05UUk9MICsgMikKKyNkZWZpbmUgUENJX0NBUF9QQ0lF
X1NUQVJUCVBDSV9CUklER0VfQ09ORl9FTkQKKyNkZWZpbmUgUENJX0NBUF9QQ0lFX0VORAkoUENJ
X0NBUF9QQ0lFX1NUQVJUICsgUENJX0VYUF9TTFRTVEEyICsgMikKKworLyoKKyAqIEluaXRpYWxp
emUgYSBwY2lfc3dfYnJpZGdlIHN0cnVjdHVyZSB0byByZXByZXNlbnQgYSBmYWtlIFBDSQorICog
YnJpZGdlLiBUaGUgY2FsbGVyIG5lZWRzIHRvIGhhdmUgaW5pdGlhbGl6ZWQgdGhlIFBDSSBjb25m
aWd1cmF0aW9uCisgKiBzcGFjZSB3aXRoIHdoYXRldmVyIHZhbHVlcyBtYWtlIHNlbnNlICh0eXBp
Y2FsbHkgYXQgbGVhc3QgdmVuZG9yLAorICogZGV2aWNlLCByZXZpc2lvbiksIHRoZSAtPm9wcyBw
b2ludGVyLCBhbmQgcG9zc2libHkgLT5kYXRhIGFuZAorICogLT5oYXNfcGNpZS4KKyAqLwordm9p
ZCBwY2lfc3dfYnJpZGdlX2luaXQoc3RydWN0IHBjaV9zd19icmlkZ2UgKmJyaWRnZSkKK3sKKwli
cmlkZ2UtPmNvbmYuY2xhc3MgPSBQQ0lfQ0xBU1NfQlJJREdFX1BDSTsKKwlicmlkZ2UtPmNvbmYu
aGVhZGVyX3R5cGUgPSBQQ0lfSEVBREVSX1RZUEVfQlJJREdFOworCWJyaWRnZS0+Y29uZi5jYWNo
ZV9saW5lX3NpemUgPSAweDEwOworCWJyaWRnZS0+Y29uZi5zdGF0dXMgPSBQQ0lfU1RBVFVTX0NB
UF9MSVNUOworCisJaWYgKGJyaWRnZS0+aGFzX3BjaWUpIHsKKwkJYnJpZGdlLT5jb25mLmNhcGFi
aWxpdGllc19wb2ludGVyID0gUENJX0NBUF9QQ0lFX1NUQVJUOworCQlicmlkZ2UtPnBjaWVfY29u
Zi5jYXBfaWQgPSBQQ0lfQ0FQX0lEX0VYUDsKKwkJLyogU2V0IFBDSWUgdjIsIHJvb3QgcG9ydCwg
c2xvdCBzdXBwb3J0ICovCisJCWJyaWRnZS0+cGNpZV9jb25mLmNhcCA9IFBDSV9FWFBfVFlQRV9S
T09UX1BPUlQgPDwgNCB8IDIgfAorCQkJUENJX0VYUF9GTEFHU19TTE9UOworCX0KK30KKworLyoK
KyAqIFNob3VsZCBiZSBjYWxsZWQgYnkgdGhlIFBDSSBjb250cm9sbGVyIGRyaXZlciB3aGVuIHJl
YWRpbmcgdGhlIFBDSQorICogY29uZmlndXJhdGlvbiBzcGFjZSBvZiB0aGUgZmFrZSBicmlkZ2Uu
IEl0IHdpbGwgY2FsbCBiYWNrIHRoZQorICogLT5vcHMtPnJlYWRfYmFzZSBvciAtPm9wcy0+cmVh
ZF9wY2llIG9wZXJhdGlvbnMuCisgKi8KK2ludCBwY2lfc3dfYnJpZGdlX3JlYWQoc3RydWN0IHBj
aV9zd19icmlkZ2UgKmJyaWRnZSwgaW50IHdoZXJlLAorCQkgICAgICAgaW50IHNpemUsIHUzMiAq
dmFsdWUpCit7CisJaW50IHJldDsKKwlpbnQgcmVnID0gd2hlcmUgJiB+MzsKKworCWlmIChicmlk
Z2UtPmhhc19wY2llICYmIHJlZyA+PSBQQ0lfQ0FQX1BDSUVfRU5EKSB7CisJCSp2YWx1ZSA9IDA7
CisJCXJldHVybiBQQ0lCSU9TX1NVQ0NFU1NGVUw7CisJfQorCisJaWYgKCFicmlkZ2UtPmhhc19w
Y2llICYmIHJlZyA+PSBQQ0lfQlJJREdFX0NPTkZfRU5EKSB7CisJCSp2YWx1ZSA9IDA7CisJCXJl
dHVybiBQQ0lCSU9TX1NVQ0NFU1NGVUw7CisJfQorCisJaWYgKGJyaWRnZS0+aGFzX3BjaWUgJiYg
cmVnID49IFBDSV9DQVBfUENJRV9TVEFSVCkgeworCQlyZWcgLT0gUENJX0NBUF9QQ0lFX1NUQVJU
OworCisJCWlmIChicmlkZ2UtPm9wcy0+cmVhZF9wY2llKQorCQkJcmV0ID0gYnJpZGdlLT5vcHMt
PnJlYWRfcGNpZShicmlkZ2UsIHJlZywgdmFsdWUpOworCQllbHNlCisJCQlyZXQgPSBQQ0lfU1df
QlJJREdFX05PVF9IQU5ETEVEOworCisJCWlmIChyZXQgPT0gUENJX1NXX0JSSURHRV9OT1RfSEFO
RExFRCkKKwkJCSp2YWx1ZSA9ICooKHUzMiopICZicmlkZ2UtPnBjaWVfY29uZiArIHJlZyAvIDQp
OworCX0gZWxzZSB7CisJCWlmIChicmlkZ2UtPm9wcy0+cmVhZF9iYXNlKQorCQkJcmV0ID0gYnJp
ZGdlLT5vcHMtPnJlYWRfYmFzZShicmlkZ2UsIHJlZywgdmFsdWUpOworCQllbHNlCisJCQlyZXQg
PSBQQ0lfU1dfQlJJREdFX05PVF9IQU5ETEVEOworCisJCWlmIChyZXQgPT0gUENJX1NXX0JSSURH
RV9OT1RfSEFORExFRCkKKwkJCSp2YWx1ZSA9ICooKHUzMiopICZicmlkZ2UtPmNvbmYgKyByZWcg
LyA0KTsKKwl9CisKKwlpZiAoc2l6ZSA9PSAxKQorCQkqdmFsdWUgPSAoKnZhbHVlID4+ICg4ICog
KHdoZXJlICYgMykpKSAmIDB4ZmY7CisJZWxzZSBpZiAoc2l6ZSA9PSAyKQorCQkqdmFsdWUgPSAo
KnZhbHVlID4+ICg4ICogKHdoZXJlICYgMykpKSAmIDB4ZmZmZjsKKwllbHNlIGlmIChzaXplICE9
IDQpCisJCXJldHVybiBQQ0lCSU9TX0JBRF9SRUdJU1RFUl9OVU1CRVI7CisKKwlyZXR1cm4gUENJ
QklPU19TVUNDRVNTRlVMOworfQorCisvKgorICogU2hvdWxkIGJlIGNhbGxlZCBieSB0aGUgUENJ
IGNvbnRyb2xsZXIgZHJpdmVyIHdoZW4gd3JpdGluZyB0aGUgUENJCisgKiBjb25maWd1cmF0aW9u
IHNwYWNlIG9mIHRoZSBmYWtlIGJyaWRnZS4gSXQgd2lsbCBjYWxsIGJhY2sgdGhlCisgKiAtPm9w
cy0+d3JpdGVfYmFzZSBvciAtPm9wcy0+d3JpdGVfcGNpZSBvcGVyYXRpb25zLgorICovCitpbnQg
cGNpX3N3X2JyaWRnZV93cml0ZShzdHJ1Y3QgcGNpX3N3X2JyaWRnZSAqYnJpZGdlLCBpbnQgd2hl
cmUsCisJCQlpbnQgc2l6ZSwgdTMyIHZhbHVlKQoreworCWludCByZWcgPSB3aGVyZSAmIH4zOwor
CWludCBtYXNrLCByZXQsIG9sZCwgbmV3OworCisJaWYgKGJyaWRnZS0+aGFzX3BjaWUgJiYgcmVn
ID49IFBDSV9DQVBfUENJRV9FTkQpCisJCXJldHVybiBQQ0lCSU9TX1NVQ0NFU1NGVUw7CisKKwlp
ZiAoIWJyaWRnZS0+aGFzX3BjaWUgJiYgcmVnID49IFBDSV9CUklER0VfQ09ORl9FTkQpCisJCXJl
dHVybiBQQ0lCSU9TX1NVQ0NFU1NGVUw7CisKKwlpZiAoc2l6ZSA9PSA0KQorCQltYXNrID0gMHhm
ZmZmZmZmZjsKKwllbHNlIGlmIChzaXplID09IDIpCisJCW1hc2sgPSAweGZmZmYgPDwgKCh3aGVy
ZSAmIDB4MykgKiA4KTsKKwllbHNlIGlmIChzaXplID09IDEpCisJCW1hc2sgPSAweGZmIDw8ICgo
d2hlcmUgJiAweDMpICogOCk7CisJZWxzZQorCQlyZXR1cm4gUENJQklPU19CQURfUkVHSVNURVJf
TlVNQkVSOworCisJcmV0ID0gcGNpX3N3X2JyaWRnZV9yZWFkKGJyaWRnZSwgcmVnLCA0LCAmb2xk
KTsKKwlpZiAocmV0ICE9IFBDSUJJT1NfU1VDQ0VTU0ZVTCkKKwkJcmV0dXJuIHJldDsKKworCW5l
dyA9IG9sZCAmIH5tYXNrOworCW5ldyB8PSAodmFsdWUgPDwgKCh3aGVyZSAmIDB4MykgKiA4KSkg
JiBtYXNrOworCisJaWYgKGJyaWRnZS0+aGFzX3BjaWUgJiYgcmVnID49IFBDSV9DQVBfUENJRV9T
VEFSVCkgeworCQlyZWcgLT0gUENJX0NBUF9QQ0lFX1NUQVJUOworCisJCSooKHUzMiopICZicmlk
Z2UtPnBjaWVfY29uZiArIHJlZyAvIDQpID0gbmV3OworCisJCWlmIChicmlkZ2UtPm9wcy0+d3Jp
dGVfcGNpZSkKKwkJCWJyaWRnZS0+b3BzLT53cml0ZV9wY2llKGJyaWRnZSwgcmVnLCBvbGQsIG5l
dywgbWFzayk7CisJfSBlbHNlIHsKKwkJKigodTMyKikgJmJyaWRnZS0+Y29uZiArIHJlZyAvIDQp
ID0gbmV3OworCisJCWlmIChicmlkZ2UtPm9wcy0+d3JpdGVfYmFzZSkKKwkJCWJyaWRnZS0+b3Bz
LT53cml0ZV9iYXNlKGJyaWRnZSwgcmVnLCBvbGQsIG5ldywgbWFzayk7CisJfQorCisJcmV0dXJu
IFBDSUJJT1NfU1VDQ0VTU0ZVTDsKK30KZGlmZiAtLWdpdCBhL2luY2x1ZGUvbGludXgvcGNpLXN3
LWJyaWRnZS5oIGIvaW5jbHVkZS9saW51eC9wY2ktc3ctYnJpZGdlLmgKbmV3IGZpbGUgbW9kZSAx
MDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwLi4xNTI2NDgxMjRkZjUKLS0tIC9kZXYvbnVsbAorKysg
Yi9pbmNsdWRlL2xpbnV4L3BjaS1zdy1icmlkZ2UuaApAQCAtMCwwICsxLDEyNSBAQAorI2lmbmRl
ZiBfX1BDSV9TV19CUklER0VfSF9fCisjZGVmaW5lIF9fUENJX1NXX0JSSURHRV9IX18KKworI2lu
Y2x1ZGUgPGxpbnV4L2tlcm5lbC5oPgorCisvKiBQQ0kgY29uZmlndXJhdGlvbiBzcGFjZSBvZiBh
IFBDSS10by1QQ0kgYnJpZGdlLiAqLworc3RydWN0IHBjaV9zd19icmlkZ2VfY29uZiB7CisJdTE2
IHZlbmRvcjsKKwl1MTYgZGV2aWNlOworCXUxNiBjb21tYW5kOworCXUxNiBzdGF0dXM7CisJdTgg
cmV2aXNpb247CisJdTggaW50ZXJmYWNlOworCXUxNiBjbGFzczsKKwl1OCBjYWNoZV9saW5lX3Np
emU7CisJdTggbGF0ZW5jeV90aW1lcjsKKwl1OCBoZWFkZXJfdHlwZTsKKwl1OCBiaXN0OworCXUz
MiBiYXJbMl07CisJdTggcHJpbWFyeV9idXM7CisJdTggc2Vjb25kYXJ5X2J1czsKKwl1OCBzdWJv
cmRpbmF0ZV9idXM7CisJdTggc2Vjb25kYXJ5X2xhdGVuY3lfdGltZXI7CisJdTggaW9iYXNlOwor
CXU4IGlvbGltaXQ7CisJdTE2IHNlY29uZGFyeV9zdGF0dXM7CisJdTE2IG1lbWJhc2U7CisJdTE2
IG1lbWxpbWl0OworCXUxNiBwcmVmX21lbV9iYXNlOworCXUxNiBwcmVmX21lbV9saW1pdDsKKwl1
MzIgcHJlZmJhc2V1cHBlcjsKKwl1MzIgcHJlZmxpbWl0dXBwZXI7CisJdTE2IGlvYmFzZXVwcGVy
OworCXUxNiBpb2xpbWl0dXBwZXI7CisJdTggY2FwYWJpbGl0aWVzX3BvaW50ZXI7CisJdTggcmVz
ZXJ2ZVszXTsKKwl1MzIgcm9tYWRkcjsKKwl1OCBpbnRsaW5lOworCXU4IGludHBpbjsKKwl1MTYg
YnJpZGdlY3RybDsKK307CisKKy8qIFBDSSBjb25maWd1cmF0aW9uIHNwYWNlIG9mIHRoZSBQQ0ll
IGNhcGFiaWxpdGllcyAqLworc3RydWN0IHBjaV9zd19icmlkZ2VfcGNpZV9jb25mIHsKKwl1OCBj
YXBfaWQ7CisJdTggbmV4dDsKKwl1MTYgY2FwOworCXUzMiBkZXZjYXA7CisJdTE2IGRldmN0bDsK
Kwl1MTYgZGV2c3RhOworCXUzMiBsbmtjYXA7CisJdTE2IGxua2N0bDsKKwl1MTYgbG5rc3RhOwor
CXUzMiBzbG90Y2FwOworCXUxNiBzbG90Y3RsOworCXUxNiBzbG90c3RhOworCXUxNiByb290Y3Rs
OworCXUxNiByc3ZkOworCXUzMiByb290c3RhOworCXUzMiBkZXZjYXAyOworCXUxNiBkZXZjdGwy
OworCXUxNiBkZXZzdGEyOworCXUzMiBsbmtjYXAyOworCXUxNiBsbmtjdGwyOworCXUxNiBsbmtz
dGEyOworCXUzMiBzbG90Y2FwMjsKKwl1MTYgc2xvdGN0bDI7CisJdTE2IHNsb3RzdGEyOworfTsK
Kworc3RydWN0IHBjaV9zd19icmlkZ2U7CisKK3R5cGVkZWYgZW51bSB7IFBDSV9TV19CUklER0Vf
SEFORExFRCwKKwkgICAgICAgUENJX1NXX0JSSURHRV9OT1RfSEFORExFRCB9IHBjaV9zd19icmlk
Z2VfcmVhZF9zdGF0dXNfdDsKKworc3RydWN0IHBjaV9zd19icmlkZ2Vfb3BzIHsKKwkvKgorCSAq
IENhbGxlZCB3aGVuIHJlYWRpbmcgZnJvbSB0aGUgcmVndWxhciBQQ0kgYnJpZGdlCisJICogY29u
ZmlndXJhdGlvbiBzcGFjZS4gUmV0dXJuIFBDSV9TV19CUklER0VfSEFORExFRCB3aGVuIHRoZQor
CSAqIG9wZXJhdGlvbiBoYXMgaGFuZGxlZCB0aGUgcmVhZCBvcGVyYXRpb24gYW5kIGZpbGxlZCBp
biB0aGUKKwkgKiAqdmFsdWUsIG9yIFBDSV9TV19CUklER0VfTk9UX0hBTkRMRUQgd2hlbiB0aGUg
cmVhZCBzaG91bGQKKwkgKiBiZSBlbXVsYXRlZCBieSB0aGUgY29tbW9uIGNvZGUgYnkgcmVhZGlu
ZyBmcm9tIHRoZQorCSAqIGluLW1lbW9yeSBjb3B5IG9mIHRoZSBjb25maWd1cmF0aW9uIHNwYWNl
LgorCSAqLworCXBjaV9zd19icmlkZ2VfcmVhZF9zdGF0dXNfdCAoKnJlYWRfYmFzZSkoc3RydWN0
IHBjaV9zd19icmlkZ2UgKmJyaWRnZSwKKwkJCQkJCSBpbnQgcmVnLCB1MzIgKnZhbHVlKTsKKwor
CS8qCisJICogU2FtZSBhcyAtPnJlYWRfYmFzZSgpLCBleGNlcHQgaXQgaXMgZm9yIHJlYWRpbmcg
ZnJvbSB0aGUKKwkgKiBQQ0llIGNhcGFiaWxpdHkgY29uZmlndXJhdGlvbiBzcGFjZS4KKwkgKi8K
KwlwY2lfc3dfYnJpZGdlX3JlYWRfc3RhdHVzX3QgKCpyZWFkX3BjaWUpKHN0cnVjdCBwY2lfc3df
YnJpZGdlICpicmlkZ2UsCisJCQkJCQkgaW50IHJlZywgdTMyICp2YWx1ZSk7CisJLyoKKwkgKiBD
YWxsZWQgd2hlbiB3cml0aW5nIHRvIHRoZSByZWd1bGFyIFBDSSBicmlkZ2UgY29uZmlndXJhdGlv
bgorCSAqIHNwYWNlLiBvbGQgaXMgdGhlIGN1cnJlbnQgdmFsdWUsIG5ldyBpcyB0aGUgbmV3IHZh
bHVlIGJlaW5nCisJICogd3JpdHRlbiwgYW5kIG1hc2sgaW5kaWNhdGVzIHdoaWNoIHBhcnRzIG9m
IHRoZSB2YWx1ZSBhcmUKKwkgKiBiZWluZyBjaGFuZ2VkLgorCSAqLworCXZvaWQgKCp3cml0ZV9i
YXNlKShzdHJ1Y3QgcGNpX3N3X2JyaWRnZSAqYnJpZGdlLCBpbnQgcmVnLAorCQkJICAgdTMyIG9s
ZCwgdTMyIG5ldywgdTMyIG1hc2spOworCisJLyoKKwkgKiBTYW1lIGFzIC0+cmVhZF9iYXNlKCks
IGV4Y2VwdCBpdCBpcyBmb3IgcmVhZGluZyBmcm9tIHRoZQorCSAqIFBDSWUgY2FwYWJpbGl0eSBj
b25maWd1cmF0aW9uIHNwYWNlLgorCSAqLworCXZvaWQgKCp3cml0ZV9wY2llKShzdHJ1Y3QgcGNp
X3N3X2JyaWRnZSAqYnJpZGdlLCBpbnQgcmVnLAorCQkJICAgdTMyIG9sZCwgdTMyIG5ldywgdTMy
IG1hc2spOworfTsKKworc3RydWN0IHBjaV9zd19icmlkZ2UgeworCXN0cnVjdCBwY2lfc3dfYnJp
ZGdlX2NvbmYgY29uZjsKKwlzdHJ1Y3QgcGNpX3N3X2JyaWRnZV9wY2llX2NvbmYgcGNpZV9jb25m
OworCXN0cnVjdCBwY2lfc3dfYnJpZGdlX29wcyAqb3BzOworCXZvaWQgKmRhdGE7CisJYm9vbCBo
YXNfcGNpZTsKK307CisKK3ZvaWQgcGNpX3N3X2JyaWRnZV9pbml0KHN0cnVjdCBwY2lfc3dfYnJp
ZGdlICpicmlkZ2UpOworaW50IHBjaV9zd19icmlkZ2VfcmVhZChzdHJ1Y3QgcGNpX3N3X2JyaWRn
ZSAqYnJpZGdlLCBpbnQgd2hlcmUsCisJCSAgICAgICBpbnQgc2l6ZSwgdTMyICp2YWx1ZSk7Citp
bnQgcGNpX3N3X2JyaWRnZV93cml0ZShzdHJ1Y3QgcGNpX3N3X2JyaWRnZSAqYnJpZGdlLCBpbnQg
d2hlcmUsCisJCQlpbnQgc2l6ZSwgdTMyIHZhbHVlKTsKKworI2VuZGlmIC8qIF9fUENJX1NXX0JS
SURHRV9IX18gKi8KLS0gCjIuMTQuNAoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0CmxpbnV4LWFybS1r
ZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWls
bWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/3] PCI: Introduce PCI software bridge common logic
@ 2018-06-29 9:22 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-06-29 9:22 UTC (permalink / raw)
To: linux-arm-kernel
Some PCI host controllers do not expose at the hardware level a root
port PCI bridge. Due to this, the Marvell Armada 370/38x/XP PCI
controller driver (pci-mvebu) emulates a root port PCI bridge, and
uses that to (among other things?) dynamically create the memory
windows that correspond to the PCI MEM and I/O regions.
Since we now need to add a very similar logic for the Marvell Armada
37xx PCI controller driver (pci-aardvark), instead of duplicating the
code, we create in this commit a common logic called pci-sw-bridge.
The idea of this logic is to emulate a root port PCI bridge by
providing configuration space read/write operations, and faking behind
the scenes the configuration space of a PCI bridge. A PCI host
controller driver simply has to call pci_sw_bridge_read() and
pci_sw_bridge_write() to read/write the configuration space of the
bridge.
By default, the PCI bridge configuration space is simply emulated by a
chunk of memory, but the PCI host controller can override the behavior
of the read and write operations on a per-register basis to do
additional actions if needed.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
drivers/pci/Kconfig | 3 +
drivers/pci/Makefile | 1 +
drivers/pci/pci-sw-bridge.c | 149 ++++++++++++++++++++++++++++++++++++++++++
include/linux/pci-sw-bridge.h | 125 +++++++++++++++++++++++++++++++++++
4 files changed, 278 insertions(+)
create mode 100644 drivers/pci/pci-sw-bridge.c
create mode 100644 include/linux/pci-sw-bridge.h
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 56ff8f6d31fc..1f13575d052b 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -98,6 +98,9 @@ config PCI_ECAM
config PCI_LOCKLESS_CONFIG
bool
+config PCI_SW_BRIDGE
+ bool
+
config PCI_IOV
bool "PCI IOV support"
depends on PCI
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 535201984b8b..0bd65ddd2c50 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_HOTPLUG_PCI) += hotplug/
obj-$(CONFIG_PCI_MSI) += msi.o
obj-$(CONFIG_PCI_ATS) += ats.o
obj-$(CONFIG_PCI_IOV) += iov.o
+obj-$(CONFIG_PCI_SW_BRIDGE) += pci-sw-bridge.o
obj-$(CONFIG_ACPI) += pci-acpi.o
obj-$(CONFIG_PCI_LABEL) += pci-label.o
obj-$(CONFIG_X86_INTEL_MID) += pci-mid.o
diff --git a/drivers/pci/pci-sw-bridge.c b/drivers/pci/pci-sw-bridge.c
new file mode 100644
index 000000000000..304e8c0e164d
--- /dev/null
+++ b/drivers/pci/pci-sw-bridge.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Marvell
+ *
+ * Author: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
+ *
+ * This file helps PCI controller drivers implement a fake root port
+ * PCI bridge when the HW doesn't provide such a root port PCI
+ * bridge.
+ *
+ * It emulates a PCI bridge by providing a fake PCI configuration
+ * space (and optionally a PCIe capability configuration space) in
+ * memory. By default the read/write operations simply read and update
+ * this fake configuration space in memory. However, PCI controller
+ * drivers can provide through the 'struct pci_sw_bridge_ops'
+ * structure a set of operations to override or complement this
+ * default behavior.
+ */
+
+#include <linux/pci-sw-bridge.h>
+#include <linux/pci.h>
+
+#define PCI_BRIDGE_CONF_END (PCI_BRIDGE_CONTROL + 2)
+#define PCI_CAP_PCIE_START PCI_BRIDGE_CONF_END
+#define PCI_CAP_PCIE_END (PCI_CAP_PCIE_START + PCI_EXP_SLTSTA2 + 2)
+
+/*
+ * Initialize a pci_sw_bridge structure to represent a fake PCI
+ * bridge. The caller needs to have initialized the PCI configuration
+ * space with whatever values make sense (typically at least vendor,
+ * device, revision), the ->ops pointer, and possibly ->data and
+ * ->has_pcie.
+ */
+void pci_sw_bridge_init(struct pci_sw_bridge *bridge)
+{
+ bridge->conf.class = PCI_CLASS_BRIDGE_PCI;
+ bridge->conf.header_type = PCI_HEADER_TYPE_BRIDGE;
+ bridge->conf.cache_line_size = 0x10;
+ bridge->conf.status = PCI_STATUS_CAP_LIST;
+
+ if (bridge->has_pcie) {
+ bridge->conf.capabilities_pointer = PCI_CAP_PCIE_START;
+ bridge->pcie_conf.cap_id = PCI_CAP_ID_EXP;
+ /* Set PCIe v2, root port, slot support */
+ bridge->pcie_conf.cap = PCI_EXP_TYPE_ROOT_PORT << 4 | 2 |
+ PCI_EXP_FLAGS_SLOT;
+ }
+}
+
+/*
+ * Should be called by the PCI controller driver when reading the PCI
+ * configuration space of the fake bridge. It will call back the
+ * ->ops->read_base or ->ops->read_pcie operations.
+ */
+int pci_sw_bridge_read(struct pci_sw_bridge *bridge, int where,
+ int size, u32 *value)
+{
+ int ret;
+ int reg = where & ~3;
+
+ if (bridge->has_pcie && reg >= PCI_CAP_PCIE_END) {
+ *value = 0;
+ return PCIBIOS_SUCCESSFUL;
+ }
+
+ if (!bridge->has_pcie && reg >= PCI_BRIDGE_CONF_END) {
+ *value = 0;
+ return PCIBIOS_SUCCESSFUL;
+ }
+
+ if (bridge->has_pcie && reg >= PCI_CAP_PCIE_START) {
+ reg -= PCI_CAP_PCIE_START;
+
+ if (bridge->ops->read_pcie)
+ ret = bridge->ops->read_pcie(bridge, reg, value);
+ else
+ ret = PCI_SW_BRIDGE_NOT_HANDLED;
+
+ if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
+ *value = *((u32*) &bridge->pcie_conf + reg / 4);
+ } else {
+ if (bridge->ops->read_base)
+ ret = bridge->ops->read_base(bridge, reg, value);
+ else
+ ret = PCI_SW_BRIDGE_NOT_HANDLED;
+
+ if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
+ *value = *((u32*) &bridge->conf + reg / 4);
+ }
+
+ if (size == 1)
+ *value = (*value >> (8 * (where & 3))) & 0xff;
+ else if (size == 2)
+ *value = (*value >> (8 * (where & 3))) & 0xffff;
+ else if (size != 4)
+ return PCIBIOS_BAD_REGISTER_NUMBER;
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+/*
+ * Should be called by the PCI controller driver when writing the PCI
+ * configuration space of the fake bridge. It will call back the
+ * ->ops->write_base or ->ops->write_pcie operations.
+ */
+int pci_sw_bridge_write(struct pci_sw_bridge *bridge, int where,
+ int size, u32 value)
+{
+ int reg = where & ~3;
+ int mask, ret, old, new;
+
+ if (bridge->has_pcie && reg >= PCI_CAP_PCIE_END)
+ return PCIBIOS_SUCCESSFUL;
+
+ if (!bridge->has_pcie && reg >= PCI_BRIDGE_CONF_END)
+ return PCIBIOS_SUCCESSFUL;
+
+ if (size == 4)
+ mask = 0xffffffff;
+ else if (size == 2)
+ mask = 0xffff << ((where & 0x3) * 8);
+ else if (size == 1)
+ mask = 0xff << ((where & 0x3) * 8);
+ else
+ return PCIBIOS_BAD_REGISTER_NUMBER;
+
+ ret = pci_sw_bridge_read(bridge, reg, 4, &old);
+ if (ret != PCIBIOS_SUCCESSFUL)
+ return ret;
+
+ new = old & ~mask;
+ new |= (value << ((where & 0x3) * 8)) & mask;
+
+ if (bridge->has_pcie && reg >= PCI_CAP_PCIE_START) {
+ reg -= PCI_CAP_PCIE_START;
+
+ *((u32*) &bridge->pcie_conf + reg / 4) = new;
+
+ if (bridge->ops->write_pcie)
+ bridge->ops->write_pcie(bridge, reg, old, new, mask);
+ } else {
+ *((u32*) &bridge->conf + reg / 4) = new;
+
+ if (bridge->ops->write_base)
+ bridge->ops->write_base(bridge, reg, old, new, mask);
+ }
+
+ return PCIBIOS_SUCCESSFUL;
+}
diff --git a/include/linux/pci-sw-bridge.h b/include/linux/pci-sw-bridge.h
new file mode 100644
index 000000000000..152648124df5
--- /dev/null
+++ b/include/linux/pci-sw-bridge.h
@@ -0,0 +1,125 @@
+#ifndef __PCI_SW_BRIDGE_H__
+#define __PCI_SW_BRIDGE_H__
+
+#include <linux/kernel.h>
+
+/* PCI configuration space of a PCI-to-PCI bridge. */
+struct pci_sw_bridge_conf {
+ u16 vendor;
+ u16 device;
+ u16 command;
+ u16 status;
+ u8 revision;
+ u8 interface;
+ u16 class;
+ u8 cache_line_size;
+ u8 latency_timer;
+ u8 header_type;
+ u8 bist;
+ u32 bar[2];
+ u8 primary_bus;
+ u8 secondary_bus;
+ u8 subordinate_bus;
+ u8 secondary_latency_timer;
+ u8 iobase;
+ u8 iolimit;
+ u16 secondary_status;
+ u16 membase;
+ u16 memlimit;
+ u16 pref_mem_base;
+ u16 pref_mem_limit;
+ u32 prefbaseupper;
+ u32 preflimitupper;
+ u16 iobaseupper;
+ u16 iolimitupper;
+ u8 capabilities_pointer;
+ u8 reserve[3];
+ u32 romaddr;
+ u8 intline;
+ u8 intpin;
+ u16 bridgectrl;
+};
+
+/* PCI configuration space of the PCIe capabilities */
+struct pci_sw_bridge_pcie_conf {
+ u8 cap_id;
+ u8 next;
+ u16 cap;
+ u32 devcap;
+ u16 devctl;
+ u16 devsta;
+ u32 lnkcap;
+ u16 lnkctl;
+ u16 lnksta;
+ u32 slotcap;
+ u16 slotctl;
+ u16 slotsta;
+ u16 rootctl;
+ u16 rsvd;
+ u32 rootsta;
+ u32 devcap2;
+ u16 devctl2;
+ u16 devsta2;
+ u32 lnkcap2;
+ u16 lnkctl2;
+ u16 lnksta2;
+ u32 slotcap2;
+ u16 slotctl2;
+ u16 slotsta2;
+};
+
+struct pci_sw_bridge;
+
+typedef enum { PCI_SW_BRIDGE_HANDLED,
+ PCI_SW_BRIDGE_NOT_HANDLED } pci_sw_bridge_read_status_t;
+
+struct pci_sw_bridge_ops {
+ /*
+ * Called when reading from the regular PCI bridge
+ * configuration space. Return PCI_SW_BRIDGE_HANDLED when the
+ * operation has handled the read operation and filled in the
+ * *value, or PCI_SW_BRIDGE_NOT_HANDLED when the read should
+ * be emulated by the common code by reading from the
+ * in-memory copy of the configuration space.
+ */
+ pci_sw_bridge_read_status_t (*read_base)(struct pci_sw_bridge *bridge,
+ int reg, u32 *value);
+
+ /*
+ * Same as ->read_base(), except it is for reading from the
+ * PCIe capability configuration space.
+ */
+ pci_sw_bridge_read_status_t (*read_pcie)(struct pci_sw_bridge *bridge,
+ int reg, u32 *value);
+ /*
+ * Called when writing to the regular PCI bridge configuration
+ * space. old is the current value, new is the new value being
+ * written, and mask indicates which parts of the value are
+ * being changed.
+ */
+ void (*write_base)(struct pci_sw_bridge *bridge, int reg,
+ u32 old, u32 new, u32 mask);
+
+ /*
+ * Same as ->read_base(), except it is for reading from the
+ * PCIe capability configuration space.
+ */
+ void (*write_pcie)(struct pci_sw_bridge *bridge, int reg,
+ u32 old, u32 new, u32 mask);
+};
+
+struct pci_sw_bridge {
+ struct pci_sw_bridge_conf conf;
+ struct pci_sw_bridge_pcie_conf pcie_conf;
+ struct pci_sw_bridge_ops *ops;
+ void *data;
+ bool has_pcie;
+};
+
+void pci_sw_bridge_init(struct pci_sw_bridge *bridge);
+int pci_sw_bridge_read(struct pci_sw_bridge *bridge, int where,
+ int size, u32 *value);
+int pci_sw_bridge_write(struct pci_sw_bridge *bridge, int where,
+ int size, u32 value);
+
+#endif /* __PCI_SW_BRIDGE_H__ */
--
2.14.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 1/3] PCI: Introduce PCI software bridge common logic
2018-06-29 9:22 ` Thomas Petazzoni
@ 2018-07-12 19:58 ` Bjorn Helgaas
-1 siblings, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2018-07-12 19:58 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Lorenzo Pieralisi, Russell King, Simon Horman, Antoine Tenart,
linux-pci, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Ray Jui, Miquèl Raynal, Bjorn Helgaas,
Ley Foon Tan, Zachary Zhang, Wilson Ding, linux-arm-kernel
[+cc Ray since he's doing something similar for iProc,
Ley Foon & Simon for a possible Altera & R-Car bug]
On Fri, Jun 29, 2018 at 11:22:29AM +0200, Thomas Petazzoni wrote:
> Some PCI host controllers do not expose at the hardware level a root
> port PCI bridge. Due to this, the Marvell Armada 370/38x/XP PCI
> controller driver (pci-mvebu) emulates a root port PCI bridge, and
> uses that to (among other things=E0) dynamically create the memory
> windows that correspond to the PCI MEM and I/O regions.
> =
> Since we now need to add a very similar logic for the Marvell Armada
> 37xx PCI controller driver (pci-aardvark), instead of duplicating the
> code, we create in this commit a common logic called pci-sw-bridge.
> =
> The idea of this logic is to emulate a root port PCI bridge by
> providing configuration space read/write operations, and faking behind
> the scenes the configuration space of a PCI bridge. A PCI host
> controller driver simply has to call pci_sw_bridge_read() and
> pci_sw_bridge_write() to read/write the configuration space of the
> bridge.
These systems must actually have a Root Port (there's obviously a Port
that originates the link), but it's not visible in a spec-compliant
way. So this is basically a wrapper that translates accesses to
standard config space into the vendor-specific registers.
Since there really *is* a hardware bridge but it just doesn't have the
interface we expect, "sw_bridge" doesn't feel like quite the right
name for it. Maybe something related to adapter/emulation/interface/...?
There are several drivers that do this, and it would be really cool to
have them all do it in a similar way. I found at least these:
hv_pcifront_read_config()
mvebu_pcie_rd_conf()
thunder_ecam_config_read()
thunder_pem_config_read()
xgene_pcie_config_read32()
iproc_pcie_config_read32()
rcar_pcie_read_conf()
I *really* like the fact that your accessors use the normal register
names, e.g., PCI_EXP_DEVCAP instead of PCISWCAP_EXP_DEVCAP. That's
huge for greppability.
> By default, the PCI bridge configuration space is simply emulated by a
> chunk of memory, but the PCI host controller can override the behavior
> of the read and write operations on a per-register basis to do
> additional actions if needed.
> =
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
> drivers/pci/Kconfig | 3 +
> drivers/pci/Makefile | 1 +
> drivers/pci/pci-sw-bridge.c | 149 ++++++++++++++++++++++++++++++++++++=
++++++
> include/linux/pci-sw-bridge.h | 125 +++++++++++++++++++++++++++++++++++
Could this go in drivers/pci instead of include/linux? I'd prefer to
hide it inside the PCI core if that's possible.
> + * Should be called by the PCI controller driver when reading the PCI
> + * configuration space of the fake bridge. It will call back the
> + * ->ops->read_base or ->ops->read_pcie operations.
> + */
> +int pci_sw_bridge_read(struct pci_sw_bridge *bridge, int where,
> + int size, u32 *value)
We don't really have a strong convention about the names of config
accessors: *_rd_conf(), *_read_config(), *config_read() are all used.
Maybe we could at least include "conf" to connect this with config
space as opposed to MMIO space.
> +{
> + int ret;
> + int reg =3D where & ~3;
> +
> + if (bridge->has_pcie && reg >=3D PCI_CAP_PCIE_END) {
> + *value =3D 0;
> + return PCIBIOS_SUCCESSFUL;
> + }
> +
> + if (!bridge->has_pcie && reg >=3D PCI_BRIDGE_CONF_END) {
> + *value =3D 0;
> + return PCIBIOS_SUCCESSFUL;
> + }
> +
> + if (bridge->has_pcie && reg >=3D PCI_CAP_PCIE_START) {
> + reg -=3D PCI_CAP_PCIE_START;
> +
> + if (bridge->ops->read_pcie)
> + ret =3D bridge->ops->read_pcie(bridge, reg, value);
> + else
> + ret =3D PCI_SW_BRIDGE_NOT_HANDLED;
> +
> + if (ret =3D=3D PCI_SW_BRIDGE_NOT_HANDLED)
> + *value =3D *((u32*) &bridge->pcie_conf + reg / 4);
> + } else {
> + if (bridge->ops->read_base)
> + ret =3D bridge->ops->read_base(bridge, reg, value);
> + else
> + ret =3D PCI_SW_BRIDGE_NOT_HANDLED;
> +
> + if (ret =3D=3D PCI_SW_BRIDGE_NOT_HANDLED)
> + *value =3D *((u32*) &bridge->conf + reg / 4);
I'm not sure about this part, where the config space that's not
handled by the bridge's ops ends up just being RAM that's readable and
writable with no effect on the hardware. That seems a little
counter-intuitive. I think dropping writes and reading zeroes would
simplify my mental model.
> + }
> +
> + if (size =3D=3D 1)
> + *value =3D (*value >> (8 * (where & 3))) & 0xff;
> + else if (size =3D=3D 2)
> + *value =3D (*value >> (8 * (where & 3))) & 0xffff;
> + else if (size !=3D 4)
> + return PCIBIOS_BAD_REGISTER_NUMBER;
This isn't directly related to your patches, but this is a common
pattern with some variations between drivers, which makes me wonder
whether there are bugs lurking. These use "where & 3" for size 2:
advk_pcie_rd_conf()
mtk_pcie_hw_rd_cfg()
pci_generic_config_read32()
But these use "where & 2":
_altera_pcie_cfg_read()
rcar_pcie_read_conf()
I *think* this means that unaligned 2-byte config reads on Altera and
R-Car will return incorrect data. In both cases, the actual read seen
by the hardware is a 32-bit read, but I think we extract the wrong 16
bits from that result.
I wonder if there's a way to use a common helper function to do this.
> + return PCIBIOS_SUCCESSFUL;
> +}
> + /*
> + * Called when writing to the regular PCI bridge configuration
> + * space. old is the current value, new is the new value being
> + * written, and mask indicates which parts of the value are
> + * being changed.
> + */
> + void (*write_base)(struct pci_sw_bridge *bridge, int reg,
> + u32 old, u32 new, u32 mask);
> +
> + /*
> + * Same as ->read_base(), except it is for reading from the
> + * PCIe capability configuration space.
I think you mean "->write_base(), except it is for writing to"
> + */
> + void (*write_pcie)(struct pci_sw_bridge *bridge, int reg,
> + u32 old, u32 new, u32 mask);
> +};
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 1/3] PCI: Introduce PCI software bridge common logic
@ 2018-07-12 19:58 ` Bjorn Helgaas
0 siblings, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2018-07-12 19:58 UTC (permalink / raw)
To: linux-arm-kernel
[+cc Ray since he's doing something similar for iProc,
Ley Foon & Simon for a possible Altera & R-Car bug]
On Fri, Jun 29, 2018 at 11:22:29AM +0200, Thomas Petazzoni wrote:
> Some PCI host controllers do not expose at the hardware level a root
> port PCI bridge. Due to this, the Marvell Armada 370/38x/XP PCI
> controller driver (pci-mvebu) emulates a root port PCI bridge, and
> uses that to (among other things?) dynamically create the memory
> windows that correspond to the PCI MEM and I/O regions.
>
> Since we now need to add a very similar logic for the Marvell Armada
> 37xx PCI controller driver (pci-aardvark), instead of duplicating the
> code, we create in this commit a common logic called pci-sw-bridge.
>
> The idea of this logic is to emulate a root port PCI bridge by
> providing configuration space read/write operations, and faking behind
> the scenes the configuration space of a PCI bridge. A PCI host
> controller driver simply has to call pci_sw_bridge_read() and
> pci_sw_bridge_write() to read/write the configuration space of the
> bridge.
These systems must actually have a Root Port (there's obviously a Port
that originates the link), but it's not visible in a spec-compliant
way. So this is basically a wrapper that translates accesses to
standard config space into the vendor-specific registers.
Since there really *is* a hardware bridge but it just doesn't have the
interface we expect, "sw_bridge" doesn't feel like quite the right
name for it. Maybe something related to adapter/emulation/interface/...?
There are several drivers that do this, and it would be really cool to
have them all do it in a similar way. I found at least these:
hv_pcifront_read_config()
mvebu_pcie_rd_conf()
thunder_ecam_config_read()
thunder_pem_config_read()
xgene_pcie_config_read32()
iproc_pcie_config_read32()
rcar_pcie_read_conf()
I *really* like the fact that your accessors use the normal register
names, e.g., PCI_EXP_DEVCAP instead of PCISWCAP_EXP_DEVCAP. That's
huge for greppability.
> By default, the PCI bridge configuration space is simply emulated by a
> chunk of memory, but the PCI host controller can override the behavior
> of the read and write operations on a per-register basis to do
> additional actions if needed.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
> drivers/pci/Kconfig | 3 +
> drivers/pci/Makefile | 1 +
> drivers/pci/pci-sw-bridge.c | 149 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/pci-sw-bridge.h | 125 +++++++++++++++++++++++++++++++++++
Could this go in drivers/pci instead of include/linux? I'd prefer to
hide it inside the PCI core if that's possible.
> + * Should be called by the PCI controller driver when reading the PCI
> + * configuration space of the fake bridge. It will call back the
> + * ->ops->read_base or ->ops->read_pcie operations.
> + */
> +int pci_sw_bridge_read(struct pci_sw_bridge *bridge, int where,
> + int size, u32 *value)
We don't really have a strong convention about the names of config
accessors: *_rd_conf(), *_read_config(), *config_read() are all used.
Maybe we could at least include "conf" to connect this with config
space as opposed to MMIO space.
> +{
> + int ret;
> + int reg = where & ~3;
> +
> + if (bridge->has_pcie && reg >= PCI_CAP_PCIE_END) {
> + *value = 0;
> + return PCIBIOS_SUCCESSFUL;
> + }
> +
> + if (!bridge->has_pcie && reg >= PCI_BRIDGE_CONF_END) {
> + *value = 0;
> + return PCIBIOS_SUCCESSFUL;
> + }
> +
> + if (bridge->has_pcie && reg >= PCI_CAP_PCIE_START) {
> + reg -= PCI_CAP_PCIE_START;
> +
> + if (bridge->ops->read_pcie)
> + ret = bridge->ops->read_pcie(bridge, reg, value);
> + else
> + ret = PCI_SW_BRIDGE_NOT_HANDLED;
> +
> + if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
> + *value = *((u32*) &bridge->pcie_conf + reg / 4);
> + } else {
> + if (bridge->ops->read_base)
> + ret = bridge->ops->read_base(bridge, reg, value);
> + else
> + ret = PCI_SW_BRIDGE_NOT_HANDLED;
> +
> + if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
> + *value = *((u32*) &bridge->conf + reg / 4);
I'm not sure about this part, where the config space that's not
handled by the bridge's ops ends up just being RAM that's readable and
writable with no effect on the hardware. That seems a little
counter-intuitive. I think dropping writes and reading zeroes would
simplify my mental model.
> + }
> +
> + if (size == 1)
> + *value = (*value >> (8 * (where & 3))) & 0xff;
> + else if (size == 2)
> + *value = (*value >> (8 * (where & 3))) & 0xffff;
> + else if (size != 4)
> + return PCIBIOS_BAD_REGISTER_NUMBER;
This isn't directly related to your patches, but this is a common
pattern with some variations between drivers, which makes me wonder
whether there are bugs lurking. These use "where & 3" for size 2:
advk_pcie_rd_conf()
mtk_pcie_hw_rd_cfg()
pci_generic_config_read32()
But these use "where & 2":
_altera_pcie_cfg_read()
rcar_pcie_read_conf()
I *think* this means that unaligned 2-byte config reads on Altera and
R-Car will return incorrect data. In both cases, the actual read seen
by the hardware is a 32-bit read, but I think we extract the wrong 16
bits from that result.
I wonder if there's a way to use a common helper function to do this.
> + return PCIBIOS_SUCCESSFUL;
> +}
> + /*
> + * Called when writing to the regular PCI bridge configuration
> + * space. old is the current value, new is the new value being
> + * written, and mask indicates which parts of the value are
> + * being changed.
> + */
> + void (*write_base)(struct pci_sw_bridge *bridge, int reg,
> + u32 old, u32 new, u32 mask);
> +
> + /*
> + * Same as ->read_base(), except it is for reading from the
> + * PCIe capability configuration space.
I think you mean "->write_base(), except it is for writing to"
> + */
> + void (*write_pcie)(struct pci_sw_bridge *bridge, int reg,
> + u32 old, u32 new, u32 mask);
> +};
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/3] PCI: Introduce PCI software bridge common logic
2018-07-12 19:58 ` Bjorn Helgaas
@ 2018-08-01 8:49 ` Thomas Petazzoni
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-08-01 8:49 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, Russell King,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel, Ray Jui, Ley Foon Tan, Simon Horman
Hello Bjorn,
Thanks for your review!
On Thu, 12 Jul 2018 14:58:02 -0500, Bjorn Helgaas wrote:
> > The idea of this logic is to emulate a root port PCI bridge by
> > providing configuration space read/write operations, and faking behind
> > the scenes the configuration space of a PCI bridge. A PCI host
> > controller driver simply has to call pci_sw_bridge_read() and
> > pci_sw_bridge_write() to read/write the configuration space of the
> > bridge.
>
> These systems must actually have a Root Port (there's obviously a Port
> that originates the link), but it's not visible in a spec-compliant
> way. So this is basically a wrapper that translates accesses to
> standard config space into the vendor-specific registers.
Correct.
> Since there really *is* a hardware bridge but it just doesn't have the
> interface we expect, "sw_bridge" doesn't feel like quite the right
> name for it. Maybe something related to adapter/emulation/interface/...?
sw_adapter ? sw_adap ?
> There are several drivers that do this, and it would be really cool to
> have them all do it in a similar way. I found at least these:
>
> hv_pcifront_read_config()
This one indeed looks potentially a bit similar.
> mvebu_pcie_rd_conf()
This one is converted by my patch series.
> thunder_ecam_config_read()
This one I'm not sure what is happening,
> thunder_pem_config_read()
For this one, it seems like the HW exposes a config space, just that it
is bogus. It is a bit of a different situation. It could be handled by
the same sw_bridge/sw_adapt stuff, but it's a slightly different use
case than "the HW doesn't expose any spec-compliant root port config
space".
> xgene_pcie_config_read32()
I don't see why this one would need sw_bridge/sw_adap.
> iproc_pcie_config_read32()
Not sure about this one either.
> rcar_pcie_read_conf()
Perhaps it needs sw_bridge/sw_adap.
Really for all those ones, it would require some involvement from the
corresponding driver maintainers, who understand better their HW and
see if they would benefit from sw_bridge/sw_adap.
> I *really* like the fact that your accessors use the normal register
> names, e.g., PCI_EXP_DEVCAP instead of PCISWCAP_EXP_DEVCAP. That's
> huge for greppability.
Thanks. There's however one down-side: I had to add separate accessors
for reading/writing the main PCI config space and the PCIe capability
config space. Any additional capability config space would require
adding another pair of accessors. But I really wanted to re-use the
existing PCI_EXP_* definitions. Another approach is perhaps to pass an
"ID" of the config space being accessed, i.e 0x0 for the main config
space, and PCI_CAP_ID_EXP for PCI express.
So instead of 4 accessors in pci_sw_bridge_ops, we would have only two:
pci_sw_bridge_read_status_t (*read)(struct pci_sw_bridge *bridge,
int id, int reg, u32 *value);
void (*write)(struct pci_sw_bridge *bridge, int id, int reg,
u32 old, u32 new, u32 mask);
where "id" would allow the accessor to know if we're accessing the main
config space, or the PCI express capability config space.
How does that sound ?
> > By default, the PCI bridge configuration space is simply emulated by a
> > chunk of memory, but the PCI host controller can override the behavior
> > of the read and write operations on a per-register basis to do
> > additional actions if needed.
> >
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > ---
> > drivers/pci/Kconfig | 3 +
> > drivers/pci/Makefile | 1 +
> > drivers/pci/pci-sw-bridge.c | 149 ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/pci-sw-bridge.h | 125 +++++++++++++++++++++++++++++++++++
>
> Could this go in drivers/pci instead of include/linux? I'd prefer to
> hide it inside the PCI core if that's possible.
Sure, I'll fix that.
> > + * Should be called by the PCI controller driver when reading the PCI
> > + * configuration space of the fake bridge. It will call back the
> > + * ->ops->read_base or ->ops->read_pcie operations.
> > + */
> > +int pci_sw_bridge_read(struct pci_sw_bridge *bridge, int where,
> > + int size, u32 *value)
>
> We don't really have a strong convention about the names of config
> accessors: *_rd_conf(), *_read_config(), *config_read() are all used.
> Maybe we could at least include "conf" to connect this with config
> space as opposed to MMIO space.
No problem, I'll fix that, makes perfect sense.
> > + if (bridge->has_pcie && reg >= PCI_CAP_PCIE_START) {
> > + reg -= PCI_CAP_PCIE_START;
> > +
> > + if (bridge->ops->read_pcie)
> > + ret = bridge->ops->read_pcie(bridge, reg, value);
> > + else
> > + ret = PCI_SW_BRIDGE_NOT_HANDLED;
> > +
> > + if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
> > + *value = *((u32*) &bridge->pcie_conf + reg / 4);
> > + } else {
> > + if (bridge->ops->read_base)
> > + ret = bridge->ops->read_base(bridge, reg, value);
> > + else
> > + ret = PCI_SW_BRIDGE_NOT_HANDLED;
> > +
> > + if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
> > + *value = *((u32*) &bridge->conf + reg / 4);
>
> I'm not sure about this part, where the config space that's not
> handled by the bridge's ops ends up just being RAM that's readable and
> writable with no effect on the hardware. That seems a little
> counter-intuitive. I think dropping writes and reading zeroes would
> simplify my mental model.
The fact that it is handled just like RAM is actually needed for a
number of registers. For example, in the current pci-mvebu driver, when
reading we have:
case PCI_VENDOR_ID:
*value = bridge->device << 16 | bridge->vendor;
break;
case PCI_COMMAND:
*value = bridge->command | bridge->status << 16;
break;
case PCI_CLASS_REVISION:
*value = bridge->class << 16 | bridge->interface << 8 |
bridge->revision;
break;
case PCI_CACHE_LINE_SIZE:
*value = bridge->bist << 24 | bridge->header_type << 16 |
bridge->latency_timer << 8 | bridge->cache_line_size;
break;
case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
*value = bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4];
break;
case PCI_PRIMARY_BUS:
*value = (bridge->secondary_latency_timer << 24 |
bridge->subordinate_bus << 16 |
bridge->secondary_bus << 8 |
bridge->primary_bus);
break;
case PCI_IO_BASE:
if (!mvebu_has_ioport(port))
*value = bridge->secondary_status << 16;
else
*value = (bridge->secondary_status << 16 |
bridge->iolimit << 8 |
bridge->iobase);
break;
We definitely cannot return zeroes for all these values.
And for writes, we have code like this:
case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4] = value;
break;
case PCI_IO_BASE:
/*
* We also keep bit 1 set, it is a read-only bit that
* indicates we support 32 bits addressing for the
* I/O
*/
bridge->iobase = (value & 0xff) | PCI_IO_RANGE_TYPE_32;
bridge->iolimit = ((value >> 8) & 0xff) | PCI_IO_RANGE_TYPE_32;
mvebu_pcie_handle_iobase_change(port);
break;
case PCI_MEMORY_BASE:
bridge->membase = value & 0xffff;
bridge->memlimit = value >> 16;
mvebu_pcie_handle_membase_change(port);
break;
case PCI_IO_BASE_UPPER16:
bridge->iobaseupper = value & 0xffff;
bridge->iolimitupper = value >> 16;
mvebu_pcie_handle_iobase_change(port);
break;
case PCI_PRIMARY_BUS:
bridge->primary_bus = value & 0xff;
bridge->secondary_bus = (value >> 8) & 0xff;
bridge->subordinate_bus = (value >> 16) & 0xff;
bridge->secondary_latency_timer = (value >> 24) & 0xff;
mvebu_pcie_set_local_bus_nr(port, bridge->secondary_bus);
break;
The writes being made to those parts of the config space are being
reflected back when reading the config space later.
> > + if (size == 1)
> > + *value = (*value >> (8 * (where & 3))) & 0xff;
> > + else if (size == 2)
> > + *value = (*value >> (8 * (where & 3))) & 0xffff;
> > + else if (size != 4)
> > + return PCIBIOS_BAD_REGISTER_NUMBER;
>
> This isn't directly related to your patches, but this is a common
> pattern with some variations between drivers, which makes me wonder
> whether there are bugs lurking. These use "where & 3" for size 2:
>
> advk_pcie_rd_conf()
> mtk_pcie_hw_rd_cfg()
> pci_generic_config_read32()
>
> But these use "where & 2":
>
> _altera_pcie_cfg_read()
> rcar_pcie_read_conf()
>
> I *think* this means that unaligned 2-byte config reads on Altera and
> R-Car will return incorrect data. In both cases, the actual read seen
> by the hardware is a 32-bit read, but I think we extract the wrong 16
> bits from that result.
Indeed, if unaligned 2-byte config reads are made, they will return
bogus results. But are such reads allowed ?
> I wonder if there's a way to use a common helper function to do this.
Yes, this small bit of logic is duplicated all over the place. I'll see
if I can come up with some reasonable helpers for that.
> > + return PCIBIOS_SUCCESSFUL;
> > +}
>
> > + /*
> > + * Called when writing to the regular PCI bridge configuration
> > + * space. old is the current value, new is the new value being
> > + * written, and mask indicates which parts of the value are
> > + * being changed.
> > + */
> > + void (*write_base)(struct pci_sw_bridge *bridge, int reg,
> > + u32 old, u32 new, u32 mask);
> > +
> > + /*
> > + * Same as ->read_base(), except it is for reading from the
> > + * PCIe capability configuration space.
>
> I think you mean "->write_base(), except it is for writing to"
Indeed, yes. Will fix that.
So, to sum up, there are really three key questions:
(1) Which name do you want for this ?
(2) Agreeing on whether a RAM-like behavior is acceptable, and if not,
which solution do you propose instead.
(3) How much do you want me to convert other drivers vs. the
maintainers for those drivers being responsible for doing this.
Since "There are only two hard things in Computer Science: cache
invalidation and naming things.", I expect question (1) to be the most
difficult one :-)
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 1/3] PCI: Introduce PCI software bridge common logic
@ 2018-08-01 8:49 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-08-01 8:49 UTC (permalink / raw)
To: linux-arm-kernel
Hello Bjorn,
Thanks for your review!
On Thu, 12 Jul 2018 14:58:02 -0500, Bjorn Helgaas wrote:
> > The idea of this logic is to emulate a root port PCI bridge by
> > providing configuration space read/write operations, and faking behind
> > the scenes the configuration space of a PCI bridge. A PCI host
> > controller driver simply has to call pci_sw_bridge_read() and
> > pci_sw_bridge_write() to read/write the configuration space of the
> > bridge.
>
> These systems must actually have a Root Port (there's obviously a Port
> that originates the link), but it's not visible in a spec-compliant
> way. So this is basically a wrapper that translates accesses to
> standard config space into the vendor-specific registers.
Correct.
> Since there really *is* a hardware bridge but it just doesn't have the
> interface we expect, "sw_bridge" doesn't feel like quite the right
> name for it. Maybe something related to adapter/emulation/interface/...?
sw_adapter ? sw_adap ?
> There are several drivers that do this, and it would be really cool to
> have them all do it in a similar way. I found at least these:
>
> hv_pcifront_read_config()
This one indeed looks potentially a bit similar.
> mvebu_pcie_rd_conf()
This one is converted by my patch series.
> thunder_ecam_config_read()
This one I'm not sure what is happening,
> thunder_pem_config_read()
For this one, it seems like the HW exposes a config space, just that it
is bogus. It is a bit of a different situation. It could be handled by
the same sw_bridge/sw_adapt stuff, but it's a slightly different use
case than "the HW doesn't expose any spec-compliant root port config
space".
> xgene_pcie_config_read32()
I don't see why this one would need sw_bridge/sw_adap.
> iproc_pcie_config_read32()
Not sure about this one either.
> rcar_pcie_read_conf()
Perhaps it needs sw_bridge/sw_adap.
Really for all those ones, it would require some involvement from the
corresponding driver maintainers, who understand better their HW and
see if they would benefit from sw_bridge/sw_adap.
> I *really* like the fact that your accessors use the normal register
> names, e.g., PCI_EXP_DEVCAP instead of PCISWCAP_EXP_DEVCAP. That's
> huge for greppability.
Thanks. There's however one down-side: I had to add separate accessors
for reading/writing the main PCI config space and the PCIe capability
config space. Any additional capability config space would require
adding another pair of accessors. But I really wanted to re-use the
existing PCI_EXP_* definitions. Another approach is perhaps to pass an
"ID" of the config space being accessed, i.e 0x0 for the main config
space, and PCI_CAP_ID_EXP for PCI express.
So instead of 4 accessors in pci_sw_bridge_ops, we would have only two:
pci_sw_bridge_read_status_t (*read)(struct pci_sw_bridge *bridge,
int id, int reg, u32 *value);
void (*write)(struct pci_sw_bridge *bridge, int id, int reg,
u32 old, u32 new, u32 mask);
where "id" would allow the accessor to know if we're accessing the main
config space, or the PCI express capability config space.
How does that sound ?
> > By default, the PCI bridge configuration space is simply emulated by a
> > chunk of memory, but the PCI host controller can override the behavior
> > of the read and write operations on a per-register basis to do
> > additional actions if needed.
> >
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > ---
> > drivers/pci/Kconfig | 3 +
> > drivers/pci/Makefile | 1 +
> > drivers/pci/pci-sw-bridge.c | 149 ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/pci-sw-bridge.h | 125 +++++++++++++++++++++++++++++++++++
>
> Could this go in drivers/pci instead of include/linux? I'd prefer to
> hide it inside the PCI core if that's possible.
Sure, I'll fix that.
> > + * Should be called by the PCI controller driver when reading the PCI
> > + * configuration space of the fake bridge. It will call back the
> > + * ->ops->read_base or ->ops->read_pcie operations.
> > + */
> > +int pci_sw_bridge_read(struct pci_sw_bridge *bridge, int where,
> > + int size, u32 *value)
>
> We don't really have a strong convention about the names of config
> accessors: *_rd_conf(), *_read_config(), *config_read() are all used.
> Maybe we could at least include "conf" to connect this with config
> space as opposed to MMIO space.
No problem, I'll fix that, makes perfect sense.
> > + if (bridge->has_pcie && reg >= PCI_CAP_PCIE_START) {
> > + reg -= PCI_CAP_PCIE_START;
> > +
> > + if (bridge->ops->read_pcie)
> > + ret = bridge->ops->read_pcie(bridge, reg, value);
> > + else
> > + ret = PCI_SW_BRIDGE_NOT_HANDLED;
> > +
> > + if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
> > + *value = *((u32*) &bridge->pcie_conf + reg / 4);
> > + } else {
> > + if (bridge->ops->read_base)
> > + ret = bridge->ops->read_base(bridge, reg, value);
> > + else
> > + ret = PCI_SW_BRIDGE_NOT_HANDLED;
> > +
> > + if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
> > + *value = *((u32*) &bridge->conf + reg / 4);
>
> I'm not sure about this part, where the config space that's not
> handled by the bridge's ops ends up just being RAM that's readable and
> writable with no effect on the hardware. That seems a little
> counter-intuitive. I think dropping writes and reading zeroes would
> simplify my mental model.
The fact that it is handled just like RAM is actually needed for a
number of registers. For example, in the current pci-mvebu driver, when
reading we have:
case PCI_VENDOR_ID:
*value = bridge->device << 16 | bridge->vendor;
break;
case PCI_COMMAND:
*value = bridge->command | bridge->status << 16;
break;
case PCI_CLASS_REVISION:
*value = bridge->class << 16 | bridge->interface << 8 |
bridge->revision;
break;
case PCI_CACHE_LINE_SIZE:
*value = bridge->bist << 24 | bridge->header_type << 16 |
bridge->latency_timer << 8 | bridge->cache_line_size;
break;
case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
*value = bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4];
break;
case PCI_PRIMARY_BUS:
*value = (bridge->secondary_latency_timer << 24 |
bridge->subordinate_bus << 16 |
bridge->secondary_bus << 8 |
bridge->primary_bus);
break;
case PCI_IO_BASE:
if (!mvebu_has_ioport(port))
*value = bridge->secondary_status << 16;
else
*value = (bridge->secondary_status << 16 |
bridge->iolimit << 8 |
bridge->iobase);
break;
We definitely cannot return zeroes for all these values.
And for writes, we have code like this:
case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4] = value;
break;
case PCI_IO_BASE:
/*
* We also keep bit 1 set, it is a read-only bit that
* indicates we support 32 bits addressing for the
* I/O
*/
bridge->iobase = (value & 0xff) | PCI_IO_RANGE_TYPE_32;
bridge->iolimit = ((value >> 8) & 0xff) | PCI_IO_RANGE_TYPE_32;
mvebu_pcie_handle_iobase_change(port);
break;
case PCI_MEMORY_BASE:
bridge->membase = value & 0xffff;
bridge->memlimit = value >> 16;
mvebu_pcie_handle_membase_change(port);
break;
case PCI_IO_BASE_UPPER16:
bridge->iobaseupper = value & 0xffff;
bridge->iolimitupper = value >> 16;
mvebu_pcie_handle_iobase_change(port);
break;
case PCI_PRIMARY_BUS:
bridge->primary_bus = value & 0xff;
bridge->secondary_bus = (value >> 8) & 0xff;
bridge->subordinate_bus = (value >> 16) & 0xff;
bridge->secondary_latency_timer = (value >> 24) & 0xff;
mvebu_pcie_set_local_bus_nr(port, bridge->secondary_bus);
break;
The writes being made to those parts of the config space are being
reflected back when reading the config space later.
> > + if (size == 1)
> > + *value = (*value >> (8 * (where & 3))) & 0xff;
> > + else if (size == 2)
> > + *value = (*value >> (8 * (where & 3))) & 0xffff;
> > + else if (size != 4)
> > + return PCIBIOS_BAD_REGISTER_NUMBER;
>
> This isn't directly related to your patches, but this is a common
> pattern with some variations between drivers, which makes me wonder
> whether there are bugs lurking. These use "where & 3" for size 2:
>
> advk_pcie_rd_conf()
> mtk_pcie_hw_rd_cfg()
> pci_generic_config_read32()
>
> But these use "where & 2":
>
> _altera_pcie_cfg_read()
> rcar_pcie_read_conf()
>
> I *think* this means that unaligned 2-byte config reads on Altera and
> R-Car will return incorrect data. In both cases, the actual read seen
> by the hardware is a 32-bit read, but I think we extract the wrong 16
> bits from that result.
Indeed, if unaligned 2-byte config reads are made, they will return
bogus results. But are such reads allowed ?
> I wonder if there's a way to use a common helper function to do this.
Yes, this small bit of logic is duplicated all over the place. I'll see
if I can come up with some reasonable helpers for that.
> > + return PCIBIOS_SUCCESSFUL;
> > +}
>
> > + /*
> > + * Called when writing to the regular PCI bridge configuration
> > + * space. old is the current value, new is the new value being
> > + * written, and mask indicates which parts of the value are
> > + * being changed.
> > + */
> > + void (*write_base)(struct pci_sw_bridge *bridge, int reg,
> > + u32 old, u32 new, u32 mask);
> > +
> > + /*
> > + * Same as ->read_base(), except it is for reading from the
> > + * PCIe capability configuration space.
>
> I think you mean "->write_base(), except it is for writing to"
Indeed, yes. Will fix that.
So, to sum up, there are really three key questions:
(1) Which name do you want for this ?
(2) Agreeing on whether a RAM-like behavior is acceptable, and if not,
which solution do you propose instead.
(3) How much do you want me to convert other drivers vs. the
maintainers for those drivers being responsible for doing this.
Since "There are only two hard things in Computer Science: cache
invalidation and naming things.", I expect question (1) to be the most
difficult one :-)
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/3] PCI: Introduce PCI software bridge common logic
2018-08-01 8:49 ` Thomas Petazzoni
@ 2018-08-01 9:21 ` Russell King - ARM Linux
-1 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-08-01 9:21 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Bjorn Helgaas, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel, Ray Jui, Ley Foon Tan, Simon Horman
On Wed, Aug 01, 2018 at 10:49:57AM +0200, Thomas Petazzoni wrote:
> Indeed, yes. Will fix that.
>
> So, to sum up, there are really three key questions:
>
> (1) Which name do you want for this ?
>
> (2) Agreeing on whether a RAM-like behavior is acceptable, and if not,
> which solution do you propose instead.
Please take the time to read the PCI(e) specifications and implement
what it recommends, rather than doing something else. That's what I
did when I originally reworked the mvebu PCIe driver for Armada 388,
and it's the only sensible approach to have something that works with
the rest of the Linux PCI(e) layer.
Going off and implementing a non-standard behaviour is a recipe for
things breaking as the PCIe kernel support evolves.
The spec requires reserved registers to read back as zero and ignore
writes, whereas implemented registers have a mixture of behaviours:
- read/write
- read, write to clear
- read-only
Here's the extract(s):
All PCI devices must treat Configuration Space write operations
to reserved registers as no-ops; that is, the access must be
completed normally on the bus and the data discarded. Read
accesses to reserved or unimplemented registers must be completed
normally and a data value of 0 returned.
(eg) PCI status:
Reserved bits should be read-only and return zero when read.
A one bit is reset (if it is not read-only) whenever the register
is written, and the write data in the corresponding bit location
is a 1.
[which is why doing the read-modify-write action that some host
bridges that only support 32-bit accesses is dangerous - it leads
to various status bits being inadvertently reset.]
Getting this right in a software emulation of the register space for
each bit in every register makes for a happy Linux PCIe layer.
Now, configuration read/writes use naturally aligned addresses. The
PCI specification defines the PC IO 0xcf8/0xcfc configuration access
mechanism. The first register defines the address with a 6 bit
"double-word" address to cover the 256 bytes of standard config space.
Accesses to 0xcfc are forwarded to the PCI bus as a single
configuration request - and that means there is nothing to deal with
an attempt to access a mis-aligned word.
Indeed, if 0xcfe was to be accessed as a double word, this would
result in the processor reading the two bytes from IO address 0xcfe,
0xcff, as well as 0xd00 and 0xd01 which are not part of the
configuration data register - resulting in undefined data being read.
So, basically, misaligned configuration accesses are not supported.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/3] PCI: Introduce PCI software bridge common logic
@ 2018-08-01 9:21 ` Russell King - ARM Linux
0 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-08-01 9:21 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 01, 2018 at 10:49:57AM +0200, Thomas Petazzoni wrote:
> Indeed, yes. Will fix that.
>
> So, to sum up, there are really three key questions:
>
> (1) Which name do you want for this ?
>
> (2) Agreeing on whether a RAM-like behavior is acceptable, and if not,
> which solution do you propose instead.
Please take the time to read the PCI(e) specifications and implement
what it recommends, rather than doing something else. That's what I
did when I originally reworked the mvebu PCIe driver for Armada 388,
and it's the only sensible approach to have something that works with
the rest of the Linux PCI(e) layer.
Going off and implementing a non-standard behaviour is a recipe for
things breaking as the PCIe kernel support evolves.
The spec requires reserved registers to read back as zero and ignore
writes, whereas implemented registers have a mixture of behaviours:
- read/write
- read, write to clear
- read-only
Here's the extract(s):
All PCI devices must treat Configuration Space write operations
to reserved registers as no-ops; that is, the access must be
completed normally on the bus and the data discarded. Read
accesses to reserved or unimplemented registers must be completed
normally and a data value of 0 returned.
(eg) PCI status:
Reserved bits should be read-only and return zero when read.
A one bit is reset (if it is not read-only) whenever the register
is written, and the write data in the corresponding bit location
is a 1.
[which is why doing the read-modify-write action that some host
bridges that only support 32-bit accesses is dangerous - it leads
to various status bits being inadvertently reset.]
Getting this right in a software emulation of the register space for
each bit in every register makes for a happy Linux PCIe layer.
Now, configuration read/writes use naturally aligned addresses. The
PCI specification defines the PC IO 0xcf8/0xcfc configuration access
mechanism. The first register defines the address with a 6 bit
"double-word" address to cover the 256 bytes of standard config space.
Accesses to 0xcfc are forwarded to the PCI bus as a single
configuration request - and that means there is nothing to deal with
an attempt to access a mis-aligned word.
Indeed, if 0xcfe was to be accessed as a double word, this would
result in the processor reading the two bytes from IO address 0xcfe,
0xcff, as well as 0xd00 and 0xd01 which are not part of the
configuration data register - resulting in undefined data being read.
So, basically, misaligned configuration accesses are not supported.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] PCI: Introduce PCI software bridge common logic
2018-08-01 9:21 ` Russell King - ARM Linux
@ 2018-08-01 9:37 ` Thomas Petazzoni
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-08-01 9:37 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Bjorn Helgaas, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel, Ray Jui, Ley Foon Tan, Simon Horman
Hello Russell,
Thanks for your feedback.
On Wed, 1 Aug 2018 10:21:19 +0100, Russell King - ARM Linux wrote:
> > (2) Agreeing on whether a RAM-like behavior is acceptable, and if not,
> > which solution do you propose instead.
>
> Please take the time to read the PCI(e) specifications and implement
> what it recommends, rather than doing something else. That's what I
> did when I originally reworked the mvebu PCIe driver for Armada 388,
> and it's the only sensible approach to have something that works with
> the rest of the Linux PCI(e) layer.
Actually, all what I'm doing is generalizing what the mvebu PCIe driver
is doing (which you significantly contributed) to, and try to make it
usable by other drivers that have the same need.
So, I'm precisely trying to make sure we implement this stuff once,
correctly, rather than duplicate it in several drivers. I'm not
pretending that my first iteration is entirely correct, and your review
of it to make it better is much appreciated.
> Going off and implementing a non-standard behaviour is a recipe for
> things breaking as the PCIe kernel support evolves.
Sure.
> The spec requires reserved registers to read back as zero and ignore
> writes, whereas implemented registers have a mixture of behaviours:
>
> - read/write
> - read, write to clear
> - read-only
>
> Here's the extract(s):
>
> All PCI devices must treat Configuration Space write operations
> to reserved registers as no-ops; that is, the access must be
> completed normally on the bus and the data discarded. Read
> accesses to reserved or unimplemented registers must be completed
> normally and a data value of 0 returned.
>
> (eg) PCI status:
> Reserved bits should be read-only and return zero when read.
> A one bit is reset (if it is not read-only) whenever the register
> is written, and the write data in the corresponding bit location
> is a 1.
Right. This pci-sw-bridge layer should implement this behavior.
> [which is why doing the read-modify-write action that some host
> bridges that only support 32-bit accesses is dangerous - it leads
> to various status bits being inadvertently reset.]
>
> Getting this right in a software emulation of the register space for
> each bit in every register makes for a happy Linux PCIe layer.
Absolutely.
> Now, configuration read/writes use naturally aligned addresses. The
> PCI specification defines the PC IO 0xcf8/0xcfc configuration access
> mechanism. The first register defines the address with a 6 bit
> "double-word" address to cover the 256 bytes of standard config space.
> Accesses to 0xcfc are forwarded to the PCI bus as a single
> configuration request - and that means there is nothing to deal with
> an attempt to access a mis-aligned word.
>
> Indeed, if 0xcfe was to be accessed as a double word, this would
> result in the processor reading the two bytes from IO address 0xcfe,
> 0xcff, as well as 0xd00 and 0xd01 which are not part of the
> configuration data register - resulting in undefined data being read.
>
> So, basically, misaligned configuration accesses are not supported.
OK. So the where & 2 that the RCAR driver is doing for 2 bytes accesses
is OK, because bit 0 of where will always be 0 for a 2 bytes access.
Thanks,
Thomas Petazzoni
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/3] PCI: Introduce PCI software bridge common logic
@ 2018-08-01 9:37 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-08-01 9:37 UTC (permalink / raw)
To: linux-arm-kernel
Hello Russell,
Thanks for your feedback.
On Wed, 1 Aug 2018 10:21:19 +0100, Russell King - ARM Linux wrote:
> > (2) Agreeing on whether a RAM-like behavior is acceptable, and if not,
> > which solution do you propose instead.
>
> Please take the time to read the PCI(e) specifications and implement
> what it recommends, rather than doing something else. That's what I
> did when I originally reworked the mvebu PCIe driver for Armada 388,
> and it's the only sensible approach to have something that works with
> the rest of the Linux PCI(e) layer.
Actually, all what I'm doing is generalizing what the mvebu PCIe driver
is doing (which you significantly contributed) to, and try to make it
usable by other drivers that have the same need.
So, I'm precisely trying to make sure we implement this stuff once,
correctly, rather than duplicate it in several drivers. I'm not
pretending that my first iteration is entirely correct, and your review
of it to make it better is much appreciated.
> Going off and implementing a non-standard behaviour is a recipe for
> things breaking as the PCIe kernel support evolves.
Sure.
> The spec requires reserved registers to read back as zero and ignore
> writes, whereas implemented registers have a mixture of behaviours:
>
> - read/write
> - read, write to clear
> - read-only
>
> Here's the extract(s):
>
> All PCI devices must treat Configuration Space write operations
> to reserved registers as no-ops; that is, the access must be
> completed normally on the bus and the data discarded. Read
> accesses to reserved or unimplemented registers must be completed
> normally and a data value of 0 returned.
>
> (eg) PCI status:
> Reserved bits should be read-only and return zero when read.
> A one bit is reset (if it is not read-only) whenever the register
> is written, and the write data in the corresponding bit location
> is a 1.
Right. This pci-sw-bridge layer should implement this behavior.
> [which is why doing the read-modify-write action that some host
> bridges that only support 32-bit accesses is dangerous - it leads
> to various status bits being inadvertently reset.]
>
> Getting this right in a software emulation of the register space for
> each bit in every register makes for a happy Linux PCIe layer.
Absolutely.
> Now, configuration read/writes use naturally aligned addresses. The
> PCI specification defines the PC IO 0xcf8/0xcfc configuration access
> mechanism. The first register defines the address with a 6 bit
> "double-word" address to cover the 256 bytes of standard config space.
> Accesses to 0xcfc are forwarded to the PCI bus as a single
> configuration request - and that means there is nothing to deal with
> an attempt to access a mis-aligned word.
>
> Indeed, if 0xcfe was to be accessed as a double word, this would
> result in the processor reading the two bytes from IO address 0xcfe,
> 0xcff, as well as 0xd00 and 0xd01 which are not part of the
> configuration data register - resulting in undefined data being read.
>
> So, basically, misaligned configuration accesses are not supported.
OK. So the where & 2 that the RCAR driver is doing for 2 bytes accesses
is OK, because bit 0 of where will always be 0 for a 2 bytes access.
Thanks,
Thomas Petazzoni
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] PCI: Introduce PCI software bridge common logic
2018-08-01 9:21 ` Russell King - ARM Linux
@ 2018-08-01 9:54 ` Thomas Petazzoni
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-08-01 9:54 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Bjorn Helgaas, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel, Ray Jui, Ley Foon Tan, Simon Horman
Hello Russell,
On Wed, 1 Aug 2018 10:21:19 +0100, Russell King - ARM Linux wrote:
> All PCI devices must treat Configuration Space write operations
> to reserved registers as no-ops; that is, the access must be
> completed normally on the bus and the data discarded. Read
> accesses to reserved or unimplemented registers must be completed
> normally and a data value of 0 returned.
>
> (eg) PCI status:
> Reserved bits should be read-only and return zero when read.
> A one bit is reset (if it is not read-only) whenever the register
> is written, and the write data in the corresponding bit location
> is a 1.
>
> [which is why doing the read-modify-write action that some host
> bridges that only support 32-bit accesses is dangerous - it leads
> to various status bits being inadvertently reset.]
Speaking of this, the generic pci_generic_config_write32() function
indeed does this incorrectly, and prints a warning.
However, I just looked at the pci-thunder-pem code, and it seems to
correctly account for those W1C bits, by having an explicit list of
registers and their W1C bits (see thunder_pem_bridge_w1c_bits). This
isn't specific to the Thunder PCIe controller at all, and would benefit
from being made generic, no ?
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/3] PCI: Introduce PCI software bridge common logic
@ 2018-08-01 9:54 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-08-01 9:54 UTC (permalink / raw)
To: linux-arm-kernel
Hello Russell,
On Wed, 1 Aug 2018 10:21:19 +0100, Russell King - ARM Linux wrote:
> All PCI devices must treat Configuration Space write operations
> to reserved registers as no-ops; that is, the access must be
> completed normally on the bus and the data discarded. Read
> accesses to reserved or unimplemented registers must be completed
> normally and a data value of 0 returned.
>
> (eg) PCI status:
> Reserved bits should be read-only and return zero when read.
> A one bit is reset (if it is not read-only) whenever the register
> is written, and the write data in the corresponding bit location
> is a 1.
>
> [which is why doing the read-modify-write action that some host
> bridges that only support 32-bit accesses is dangerous - it leads
> to various status bits being inadvertently reset.]
Speaking of this, the generic pci_generic_config_write32() function
indeed does this incorrectly, and prints a warning.
However, I just looked at the pci-thunder-pem code, and it seems to
correctly account for those W1C bits, by having an explicit list of
registers and their W1C bits (see thunder_pem_bridge_w1c_bits). This
isn't specific to the Thunder PCIe controller at all, and would benefit
from being made generic, no ?
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] PCI: Introduce PCI software bridge common logic
2018-08-01 8:49 ` Thomas Petazzoni
@ 2018-08-01 11:13 ` Thomas Petazzoni
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-08-01 11:13 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, Russell King,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel, Ray Jui, Ley Foon Tan, Simon Horman
Bjorn,
On Wed, 1 Aug 2018 10:49:57 +0200, Thomas Petazzoni wrote:
> > I wonder if there's a way to use a common helper function to do this.
>
> Yes, this small bit of logic is duplicated all over the place. I'll see
> if I can come up with some reasonable helpers for that.
Here is an attempt at doing this:
- Introduce some helpers
https://github.com/MISL-EBU-System-SW/mainline-public/commit/55543d2050d9aa3abe297569be830bde8680e1e9
- Use them in drivers/pci/access.c
https://github.com/MISL-EBU-System-SW/mainline-public/commit/ce6158d5c6a039e0cddf5ee6840dadeb46c5fc4b
- Use them in PCI host controller drivers
https://github.com/MISL-EBU-System-SW/mainline-public/commit/df4d0b5272ea3b4c3226c96466e5360c5a89253f
I'm not a big fan of the naming though. Let me know what you think, if
you think it's worth it, I'll submit the patches.
Note: the whole thing is compile-tested only for now.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/3] PCI: Introduce PCI software bridge common logic
@ 2018-08-01 11:13 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-08-01 11:13 UTC (permalink / raw)
To: linux-arm-kernel
Bjorn,
On Wed, 1 Aug 2018 10:49:57 +0200, Thomas Petazzoni wrote:
> > I wonder if there's a way to use a common helper function to do this.
>
> Yes, this small bit of logic is duplicated all over the place. I'll see
> if I can come up with some reasonable helpers for that.
Here is an attempt at doing this:
- Introduce some helpers
https://github.com/MISL-EBU-System-SW/mainline-public/commit/55543d2050d9aa3abe297569be830bde8680e1e9
- Use them in drivers/pci/access.c
https://github.com/MISL-EBU-System-SW/mainline-public/commit/ce6158d5c6a039e0cddf5ee6840dadeb46c5fc4b
- Use them in PCI host controller drivers
https://github.com/MISL-EBU-System-SW/mainline-public/commit/df4d0b5272ea3b4c3226c96466e5360c5a89253f
I'm not a big fan of the naming though. Let me know what you think, if
you think it's worth it, I'll submit the patches.
Note: the whole thing is compile-tested only for now.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/3] PCI: mvebu: Convert to PCI software bridge
2018-06-29 9:22 ` Thomas Petazzoni
@ 2018-06-29 9:22 ` Thomas Petazzoni
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-06-29 9:22 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci
Cc: Russell King, Antoine Tenart, Gregory Clement, Maxime Chevallier,
Nadav Haklai, Victor Gu, Thomas Petazzoni, Miquèl Raynal,
Zachary Zhang, Wilson Ding, linux-arm-kernel
This commit convers the pci-mvebu driver to use the recently
introduced pci-sw-bridge logic, that helps emulating a root port PCI
bridge.
It has been tested on Armada GP XP, with a E1000E NIC.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
drivers/pci/controller/Kconfig | 1 +
drivers/pci/controller/pci-mvebu.c | 370 ++++++++++---------------------------
2 files changed, 102 insertions(+), 269 deletions(-)
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 18fa09b3ac8f..0c307f804c8d 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -9,6 +9,7 @@ config PCI_MVEBU
depends on MVEBU_MBUS
depends on ARM
depends on OF
+ select PCI_SW_BRIDGE
config PCI_AARDVARK
bool "Aardvark PCIe controller"
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 23e270839e6a..358b56ba7c37 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -20,6 +20,7 @@
#include <linux/of_gpio.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-sw-bridge.h>
#include "../pci.h"
@@ -63,61 +64,6 @@
#define PCIE_DEBUG_CTRL 0x1a60
#define PCIE_DEBUG_SOFT_RESET BIT(20)
-enum {
- PCISWCAP = PCI_BRIDGE_CONTROL + 2,
- PCISWCAP_EXP_LIST_ID = PCISWCAP + PCI_CAP_LIST_ID,
- PCISWCAP_EXP_DEVCAP = PCISWCAP + PCI_EXP_DEVCAP,
- PCISWCAP_EXP_DEVCTL = PCISWCAP + PCI_EXP_DEVCTL,
- PCISWCAP_EXP_LNKCAP = PCISWCAP + PCI_EXP_LNKCAP,
- PCISWCAP_EXP_LNKCTL = PCISWCAP + PCI_EXP_LNKCTL,
- PCISWCAP_EXP_SLTCAP = PCISWCAP + PCI_EXP_SLTCAP,
- PCISWCAP_EXP_SLTCTL = PCISWCAP + PCI_EXP_SLTCTL,
- PCISWCAP_EXP_RTCTL = PCISWCAP + PCI_EXP_RTCTL,
- PCISWCAP_EXP_RTSTA = PCISWCAP + PCI_EXP_RTSTA,
- PCISWCAP_EXP_DEVCAP2 = PCISWCAP + PCI_EXP_DEVCAP2,
- PCISWCAP_EXP_DEVCTL2 = PCISWCAP + PCI_EXP_DEVCTL2,
- PCISWCAP_EXP_LNKCAP2 = PCISWCAP + PCI_EXP_LNKCAP2,
- PCISWCAP_EXP_LNKCTL2 = PCISWCAP + PCI_EXP_LNKCTL2,
- PCISWCAP_EXP_SLTCAP2 = PCISWCAP + PCI_EXP_SLTCAP2,
- PCISWCAP_EXP_SLTCTL2 = PCISWCAP + PCI_EXP_SLTCTL2,
-};
-
-/* PCI configuration space of a PCI-to-PCI bridge */
-struct mvebu_sw_pci_bridge {
- u16 vendor;
- u16 device;
- u16 command;
- u16 status;
- u16 class;
- u8 interface;
- u8 revision;
- u8 bist;
- u8 header_type;
- u8 latency_timer;
- u8 cache_line_size;
- u32 bar[2];
- u8 primary_bus;
- u8 secondary_bus;
- u8 subordinate_bus;
- u8 secondary_latency_timer;
- u8 iobase;
- u8 iolimit;
- u16 secondary_status;
- u16 membase;
- u16 memlimit;
- u16 iobaseupper;
- u16 iolimitupper;
- u32 romaddr;
- u8 intline;
- u8 intpin;
- u16 bridgectrl;
-
- /* PCI express capability */
- u32 pcie_sltcap;
- u16 pcie_devctl;
- u16 pcie_rtctl;
-};
-
struct mvebu_pcie_port;
/* Structure representing all PCIe interfaces */
@@ -152,7 +98,7 @@ struct mvebu_pcie_port {
struct clk *clk;
struct gpio_desc *reset_gpio;
char *reset_name;
- struct mvebu_sw_pci_bridge bridge;
+ struct pci_sw_bridge bridge;
struct device_node *dn;
struct mvebu_pcie *pcie;
struct mvebu_pcie_window memwin;
@@ -414,11 +360,12 @@ static void mvebu_pcie_set_window(struct mvebu_pcie_port *port,
static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
{
struct mvebu_pcie_window desired = {};
+ struct pci_sw_bridge_conf *conf = &port->bridge.conf;
/* Are the new iobase/iolimit values invalid? */
- if (port->bridge.iolimit < port->bridge.iobase ||
- port->bridge.iolimitupper < port->bridge.iobaseupper ||
- !(port->bridge.command & PCI_COMMAND_IO)) {
+ if (conf->iolimit < conf->iobase ||
+ conf->iolimitupper < conf->iobaseupper ||
+ !(conf->command & PCI_COMMAND_IO)) {
mvebu_pcie_set_window(port, port->io_target, port->io_attr,
&desired, &port->iowin);
return;
@@ -437,11 +384,11 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
* specifications. iobase is the bus address, port->iowin_base
* is the CPU address.
*/
- desired.remap = ((port->bridge.iobase & 0xF0) << 8) |
- (port->bridge.iobaseupper << 16);
+ desired.remap = ((conf->iobase & 0xF0) << 8) |
+ (conf->iobaseupper << 16);
desired.base = port->pcie->io.start + desired.remap;
- desired.size = ((0xFFF | ((port->bridge.iolimit & 0xF0) << 8) |
- (port->bridge.iolimitupper << 16)) -
+ desired.size = ((0xFFF | ((conf->iolimit & 0xF0) << 8) |
+ (conf->iolimitupper << 16)) -
desired.remap) +
1;
@@ -452,10 +399,11 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
{
struct mvebu_pcie_window desired = {.remap = MVEBU_MBUS_NO_REMAP};
+ struct pci_sw_bridge_conf *conf = &port->bridge.conf;
/* Are the new membase/memlimit values invalid? */
- if (port->bridge.memlimit < port->bridge.membase ||
- !(port->bridge.command & PCI_COMMAND_MEMORY)) {
+ if (conf->memlimit < conf->membase ||
+ !(conf->command & PCI_COMMAND_MEMORY)) {
mvebu_pcie_set_window(port, port->mem_target, port->mem_attr,
&desired, &port->memwin);
return;
@@ -467,130 +415,34 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
* window to setup, according to the PCI-to-PCI bridge
* specifications.
*/
- desired.base = ((port->bridge.membase & 0xFFF0) << 16);
- desired.size = (((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
+ desired.base = ((conf->membase & 0xFFF0) << 16);
+ desired.size = (((conf->memlimit & 0xFFF0) << 16) | 0xFFFFF) -
desired.base + 1;
mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, &desired,
&port->memwin);
}
-/*
- * Initialize the configuration space of the PCI-to-PCI bridge
- * associated with the given PCIe interface.
- */
-static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
+static pci_sw_bridge_read_status_t
+mvebu_pci_sw_bridge_pcie_read(struct pci_sw_bridge *bridge,
+ int reg, u32 *value)
{
- struct mvebu_sw_pci_bridge *bridge = &port->bridge;
-
- memset(bridge, 0, sizeof(struct mvebu_sw_pci_bridge));
-
- bridge->class = PCI_CLASS_BRIDGE_PCI;
- bridge->vendor = PCI_VENDOR_ID_MARVELL;
- bridge->device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
- bridge->revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
- bridge->header_type = PCI_HEADER_TYPE_BRIDGE;
- bridge->cache_line_size = 0x10;
-
- /* We support 32 bits I/O addressing */
- bridge->iobase = PCI_IO_RANGE_TYPE_32;
- bridge->iolimit = PCI_IO_RANGE_TYPE_32;
-
- /* Add capabilities */
- bridge->status = PCI_STATUS_CAP_LIST;
-}
-
-/*
- * Read the configuration space of the PCI-to-PCI bridge associated to
- * the given PCIe interface.
- */
-static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
- unsigned int where, int size, u32 *value)
-{
- struct mvebu_sw_pci_bridge *bridge = &port->bridge;
-
- switch (where & ~3) {
- case PCI_VENDOR_ID:
- *value = bridge->device << 16 | bridge->vendor;
- break;
-
- case PCI_COMMAND:
- *value = bridge->command | bridge->status << 16;
- break;
-
- case PCI_CLASS_REVISION:
- *value = bridge->class << 16 | bridge->interface << 8 |
- bridge->revision;
- break;
-
- case PCI_CACHE_LINE_SIZE:
- *value = bridge->bist << 24 | bridge->header_type << 16 |
- bridge->latency_timer << 8 | bridge->cache_line_size;
- break;
-
- case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
- *value = bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4];
- break;
-
- case PCI_PRIMARY_BUS:
- *value = (bridge->secondary_latency_timer << 24 |
- bridge->subordinate_bus << 16 |
- bridge->secondary_bus << 8 |
- bridge->primary_bus);
- break;
-
- case PCI_IO_BASE:
- if (!mvebu_has_ioport(port))
- *value = bridge->secondary_status << 16;
- else
- *value = (bridge->secondary_status << 16 |
- bridge->iolimit << 8 |
- bridge->iobase);
- break;
-
- case PCI_MEMORY_BASE:
- *value = (bridge->memlimit << 16 | bridge->membase);
- break;
-
- case PCI_PREF_MEMORY_BASE:
- *value = 0;
- break;
-
- case PCI_IO_BASE_UPPER16:
- *value = (bridge->iolimitupper << 16 | bridge->iobaseupper);
- break;
-
- case PCI_CAPABILITY_LIST:
- *value = PCISWCAP;
- break;
+ struct mvebu_pcie_port *port = bridge->data;
- case PCI_ROM_ADDRESS1:
- *value = 0;
- break;
-
- case PCI_INTERRUPT_LINE:
- /* LINE PIN MIN_GNT MAX_LAT */
- *value = 0;
- break;
-
- case PCISWCAP_EXP_LIST_ID:
- /* Set PCIe v2, root port, slot support */
- *value = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2 |
- PCI_EXP_FLAGS_SLOT) << 16 | PCI_CAP_ID_EXP;
- break;
-
- case PCISWCAP_EXP_DEVCAP:
+ switch (reg) {
+ case PCI_EXP_DEVCAP:
*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_DEVCAP);
break;
- case PCISWCAP_EXP_DEVCTL:
+ case PCI_EXP_DEVCTL:
*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_DEVCTL) &
~(PCI_EXP_DEVCTL_URRE | PCI_EXP_DEVCTL_FERE |
PCI_EXP_DEVCTL_NFERE | PCI_EXP_DEVCTL_CERE);
- *value |= bridge->pcie_devctl;
+ /* FIXME */
+ *value |= bridge->pcie_conf.devctl;
break;
- case PCISWCAP_EXP_LNKCAP:
+ case PCI_EXP_LNKCAP:
/*
* PCIe requires the clock power management capability to be
* hard-wired to zero for downstream ports
@@ -599,132 +451,86 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
~PCI_EXP_LNKCAP_CLKPM;
break;
- case PCISWCAP_EXP_LNKCTL:
+ case PCI_EXP_LNKCTL:
*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
break;
- case PCISWCAP_EXP_SLTCAP:
- *value = bridge->pcie_sltcap;
- break;
-
- case PCISWCAP_EXP_SLTCTL:
+ case PCI_EXP_SLTCTL:
*value = PCI_EXP_SLTSTA_PDS << 16;
break;
- case PCISWCAP_EXP_RTCTL:
- *value = bridge->pcie_rtctl;
- break;
-
- case PCISWCAP_EXP_RTSTA:
+ case PCI_EXP_RTSTA:
*value = mvebu_readl(port, PCIE_RC_RTSTA);
break;
- /* PCIe requires the v2 fields to be hard-wired to zero */
- case PCISWCAP_EXP_DEVCAP2:
- case PCISWCAP_EXP_DEVCTL2:
- case PCISWCAP_EXP_LNKCAP2:
- case PCISWCAP_EXP_LNKCTL2:
- case PCISWCAP_EXP_SLTCAP2:
- case PCISWCAP_EXP_SLTCTL2:
default:
- /*
- * PCI defines configuration read accesses to reserved or
- * unimplemented registers to read as zero and complete
- * normally.
- */
- *value = 0;
- return PCIBIOS_SUCCESSFUL;
+ return PCI_SW_BRIDGE_NOT_HANDLED;
}
- if (size == 2)
- *value = (*value >> (8 * (where & 3))) & 0xffff;
- else if (size == 1)
- *value = (*value >> (8 * (where & 3))) & 0xff;
-
- return PCIBIOS_SUCCESSFUL;
+ return PCI_SW_BRIDGE_HANDLED;
}
-/* Write to the PCI-to-PCI bridge configuration space */
-static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
- unsigned int where, int size, u32 value)
+static void mvebu_pci_sw_bridge_base_write(struct pci_sw_bridge *bridge,
+ int reg, u32 old, u32 new, u32 mask)
{
- struct mvebu_sw_pci_bridge *bridge = &port->bridge;
- u32 mask, reg;
- int err;
-
- if (size == 4)
- mask = 0x0;
- else if (size == 2)
- mask = ~(0xffff << ((where & 3) * 8));
- else if (size == 1)
- mask = ~(0xff << ((where & 3) * 8));
- else
- return PCIBIOS_BAD_REGISTER_NUMBER;
-
- err = mvebu_sw_pci_bridge_read(port, where & ~3, 4, ®);
- if (err)
- return err;
+ struct mvebu_pcie_port *port = bridge->data;
+ struct pci_sw_bridge_conf *conf = &bridge->conf;
- value = (reg & mask) | value << ((where & 3) * 8);
-
- switch (where & ~3) {
+ switch (reg) {
case PCI_COMMAND:
{
- u32 old = bridge->command;
-
if (!mvebu_has_ioport(port))
- value &= ~PCI_COMMAND_IO;
+ conf->command &= ~PCI_COMMAND_IO;
- bridge->command = value & 0xffff;
- if ((old ^ bridge->command) & PCI_COMMAND_IO)
+ if ((old ^ new) & PCI_COMMAND_IO)
mvebu_pcie_handle_iobase_change(port);
- if ((old ^ bridge->command) & PCI_COMMAND_MEMORY)
+ if ((old ^ new) & PCI_COMMAND_MEMORY)
mvebu_pcie_handle_membase_change(port);
- break;
- }
- case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
- bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4] = value;
break;
+ }
case PCI_IO_BASE:
/*
- * We also keep bit 1 set, it is a read-only bit that
+ * We keep bit 1 set, it is a read-only bit that
* indicates we support 32 bits addressing for the
* I/O
*/
- bridge->iobase = (value & 0xff) | PCI_IO_RANGE_TYPE_32;
- bridge->iolimit = ((value >> 8) & 0xff) | PCI_IO_RANGE_TYPE_32;
+ conf->iobase |= PCI_IO_RANGE_TYPE_32;
+ conf->iolimit |= PCI_IO_RANGE_TYPE_32;
mvebu_pcie_handle_iobase_change(port);
break;
case PCI_MEMORY_BASE:
- bridge->membase = value & 0xffff;
- bridge->memlimit = value >> 16;
mvebu_pcie_handle_membase_change(port);
break;
case PCI_IO_BASE_UPPER16:
- bridge->iobaseupper = value & 0xffff;
- bridge->iolimitupper = value >> 16;
mvebu_pcie_handle_iobase_change(port);
break;
case PCI_PRIMARY_BUS:
- bridge->primary_bus = value & 0xff;
- bridge->secondary_bus = (value >> 8) & 0xff;
- bridge->subordinate_bus = (value >> 16) & 0xff;
- bridge->secondary_latency_timer = (value >> 24) & 0xff;
- mvebu_pcie_set_local_bus_nr(port, bridge->secondary_bus);
+ mvebu_pcie_set_local_bus_nr(port, conf->secondary_bus);
break;
- case PCISWCAP_EXP_DEVCTL:
+ default:
+ break;
+ }
+}
+
+static void mvebu_pci_sw_bridge_pcie_write(struct pci_sw_bridge *bridge,
+ int reg, u32 old, u32 new, u32 mask)
+{
+ struct mvebu_pcie_port *port = bridge->data;
+
+ switch(reg) {
+ case PCI_EXP_DEVCTL:
/*
* Armada370 data says these bits must always
* be zero when in root complex mode.
*/
- value &= ~(PCI_EXP_DEVCTL_URRE | PCI_EXP_DEVCTL_FERE |
- PCI_EXP_DEVCTL_NFERE | PCI_EXP_DEVCTL_CERE);
+ new &= ~(PCI_EXP_DEVCTL_URRE | PCI_EXP_DEVCTL_FERE |
+ PCI_EXP_DEVCTL_NFERE | PCI_EXP_DEVCTL_CERE);
/*
* If the mask is 0xffff0000, then we only want to write
@@ -732,20 +538,20 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
* RW1C bits in the device status register. Mask out the
* status register bits.
*/
- if (mask == 0xffff0000)
- value &= 0xffff;
+ if (new == 0xffff0000)
+ new &= 0xffff;
- mvebu_writel(port, value, PCIE_CAP_PCIEXP + PCI_EXP_DEVCTL);
+ mvebu_writel(port, new, PCIE_CAP_PCIEXP + PCI_EXP_DEVCTL);
break;
- case PCISWCAP_EXP_LNKCTL:
+ case PCI_EXP_LNKCTL:
/*
* If we don't support CLKREQ, we must ensure that the
* CLKREQ enable bit always reads zero. Since we haven't
* had this capability, and it's dependent on board wiring,
* disable it for the time being.
*/
- value &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
+ new &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
/*
* If the mask is 0xffff0000, then we only want to write
@@ -754,21 +560,47 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
* RW1C status register bits.
*/
if (mask == 0xffff0000)
- value &= ~((PCI_EXP_LNKSTA_LABS |
- PCI_EXP_LNKSTA_LBMS) << 16);
+ new &= ~((PCI_EXP_LNKSTA_LABS |
+ PCI_EXP_LNKSTA_LBMS) << 16);
- mvebu_writel(port, value, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
+ mvebu_writel(port, new, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
break;
- case PCISWCAP_EXP_RTSTA:
- mvebu_writel(port, value, PCIE_RC_RTSTA);
+ case PCI_EXP_RTSTA:
+ mvebu_writel(port, new, PCIE_RC_RTSTA);
break;
+ }
+}
- default:
- break;
+struct pci_sw_bridge_ops mvebu_pci_sw_bridge_ops = {
+ .write_base = mvebu_pci_sw_bridge_base_write,
+ .read_pcie = mvebu_pci_sw_bridge_pcie_read,
+ .write_pcie = mvebu_pci_sw_bridge_pcie_write,
+};
+
+/*
+ * Initialize the configuration space of the PCI-to-PCI bridge
+ * associated with the given PCIe interface.
+ */
+static void mvebu_pci_sw_bridge_init(struct mvebu_pcie_port *port)
+{
+ struct pci_sw_bridge *bridge = &port->bridge;
+
+ bridge->conf.vendor = PCI_VENDOR_ID_MARVELL;
+ bridge->conf.device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
+ bridge->conf.revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
+
+ if (mvebu_has_ioport(port)) {
+ /* We support 32 bits I/O addressing */
+ bridge->conf.iobase = PCI_IO_RANGE_TYPE_32;
+ bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32;
}
- return PCIBIOS_SUCCESSFUL;
+ bridge->has_pcie = true;
+ bridge->data = port;
+ bridge->ops = &mvebu_pci_sw_bridge_ops;
+
+ pci_sw_bridge_init(bridge);
}
static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys)
@@ -788,8 +620,8 @@ static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,
if (bus->number == 0 && port->devfn == devfn)
return port;
if (bus->number != 0 &&
- bus->number >= port->bridge.secondary_bus &&
- bus->number <= port->bridge.subordinate_bus)
+ bus->number >= port->bridge.conf.secondary_bus &&
+ bus->number <= port->bridge.conf.subordinate_bus)
return port;
}
@@ -810,7 +642,7 @@ static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
/* Access the emulated PCI-to-PCI bridge */
if (bus->number == 0)
- return mvebu_sw_pci_bridge_write(port, where, size, val);
+ return pci_sw_bridge_write(&port->bridge, where, size, val);
if (!mvebu_pcie_link_up(port))
return PCIBIOS_DEVICE_NOT_FOUND;
@@ -838,7 +670,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
/* Access the emulated PCI-to-PCI bridge */
if (bus->number == 0)
- return mvebu_sw_pci_bridge_read(port, where, size, val);
+ return pci_sw_bridge_read(&port->bridge, where, size, val);
if (!mvebu_pcie_link_up(port)) {
*val = 0xffffffff;
@@ -1273,7 +1105,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
}
mvebu_pcie_set_local_dev_nr(port, 1);
- mvebu_sw_pci_bridge_init(port);
+ mvebu_pci_sw_bridge_init(port);
}
pcie->nports = i;
--
2.14.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 2/3] PCI: mvebu: Convert to PCI software bridge
@ 2018-06-29 9:22 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-06-29 9:22 UTC (permalink / raw)
To: linux-arm-kernel
This commit convers the pci-mvebu driver to use the recently
introduced pci-sw-bridge logic, that helps emulating a root port PCI
bridge.
It has been tested on Armada GP XP, with a E1000E NIC.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
drivers/pci/controller/Kconfig | 1 +
drivers/pci/controller/pci-mvebu.c | 370 ++++++++++---------------------------
2 files changed, 102 insertions(+), 269 deletions(-)
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 18fa09b3ac8f..0c307f804c8d 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -9,6 +9,7 @@ config PCI_MVEBU
depends on MVEBU_MBUS
depends on ARM
depends on OF
+ select PCI_SW_BRIDGE
config PCI_AARDVARK
bool "Aardvark PCIe controller"
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 23e270839e6a..358b56ba7c37 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -20,6 +20,7 @@
#include <linux/of_gpio.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
+#include <linux/pci-sw-bridge.h>
#include "../pci.h"
@@ -63,61 +64,6 @@
#define PCIE_DEBUG_CTRL 0x1a60
#define PCIE_DEBUG_SOFT_RESET BIT(20)
-enum {
- PCISWCAP = PCI_BRIDGE_CONTROL + 2,
- PCISWCAP_EXP_LIST_ID = PCISWCAP + PCI_CAP_LIST_ID,
- PCISWCAP_EXP_DEVCAP = PCISWCAP + PCI_EXP_DEVCAP,
- PCISWCAP_EXP_DEVCTL = PCISWCAP + PCI_EXP_DEVCTL,
- PCISWCAP_EXP_LNKCAP = PCISWCAP + PCI_EXP_LNKCAP,
- PCISWCAP_EXP_LNKCTL = PCISWCAP + PCI_EXP_LNKCTL,
- PCISWCAP_EXP_SLTCAP = PCISWCAP + PCI_EXP_SLTCAP,
- PCISWCAP_EXP_SLTCTL = PCISWCAP + PCI_EXP_SLTCTL,
- PCISWCAP_EXP_RTCTL = PCISWCAP + PCI_EXP_RTCTL,
- PCISWCAP_EXP_RTSTA = PCISWCAP + PCI_EXP_RTSTA,
- PCISWCAP_EXP_DEVCAP2 = PCISWCAP + PCI_EXP_DEVCAP2,
- PCISWCAP_EXP_DEVCTL2 = PCISWCAP + PCI_EXP_DEVCTL2,
- PCISWCAP_EXP_LNKCAP2 = PCISWCAP + PCI_EXP_LNKCAP2,
- PCISWCAP_EXP_LNKCTL2 = PCISWCAP + PCI_EXP_LNKCTL2,
- PCISWCAP_EXP_SLTCAP2 = PCISWCAP + PCI_EXP_SLTCAP2,
- PCISWCAP_EXP_SLTCTL2 = PCISWCAP + PCI_EXP_SLTCTL2,
-};
-
-/* PCI configuration space of a PCI-to-PCI bridge */
-struct mvebu_sw_pci_bridge {
- u16 vendor;
- u16 device;
- u16 command;
- u16 status;
- u16 class;
- u8 interface;
- u8 revision;
- u8 bist;
- u8 header_type;
- u8 latency_timer;
- u8 cache_line_size;
- u32 bar[2];
- u8 primary_bus;
- u8 secondary_bus;
- u8 subordinate_bus;
- u8 secondary_latency_timer;
- u8 iobase;
- u8 iolimit;
- u16 secondary_status;
- u16 membase;
- u16 memlimit;
- u16 iobaseupper;
- u16 iolimitupper;
- u32 romaddr;
- u8 intline;
- u8 intpin;
- u16 bridgectrl;
-
- /* PCI express capability */
- u32 pcie_sltcap;
- u16 pcie_devctl;
- u16 pcie_rtctl;
-};
-
struct mvebu_pcie_port;
/* Structure representing all PCIe interfaces */
@@ -152,7 +98,7 @@ struct mvebu_pcie_port {
struct clk *clk;
struct gpio_desc *reset_gpio;
char *reset_name;
- struct mvebu_sw_pci_bridge bridge;
+ struct pci_sw_bridge bridge;
struct device_node *dn;
struct mvebu_pcie *pcie;
struct mvebu_pcie_window memwin;
@@ -414,11 +360,12 @@ static void mvebu_pcie_set_window(struct mvebu_pcie_port *port,
static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
{
struct mvebu_pcie_window desired = {};
+ struct pci_sw_bridge_conf *conf = &port->bridge.conf;
/* Are the new iobase/iolimit values invalid? */
- if (port->bridge.iolimit < port->bridge.iobase ||
- port->bridge.iolimitupper < port->bridge.iobaseupper ||
- !(port->bridge.command & PCI_COMMAND_IO)) {
+ if (conf->iolimit < conf->iobase ||
+ conf->iolimitupper < conf->iobaseupper ||
+ !(conf->command & PCI_COMMAND_IO)) {
mvebu_pcie_set_window(port, port->io_target, port->io_attr,
&desired, &port->iowin);
return;
@@ -437,11 +384,11 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
* specifications. iobase is the bus address, port->iowin_base
* is the CPU address.
*/
- desired.remap = ((port->bridge.iobase & 0xF0) << 8) |
- (port->bridge.iobaseupper << 16);
+ desired.remap = ((conf->iobase & 0xF0) << 8) |
+ (conf->iobaseupper << 16);
desired.base = port->pcie->io.start + desired.remap;
- desired.size = ((0xFFF | ((port->bridge.iolimit & 0xF0) << 8) |
- (port->bridge.iolimitupper << 16)) -
+ desired.size = ((0xFFF | ((conf->iolimit & 0xF0) << 8) |
+ (conf->iolimitupper << 16)) -
desired.remap) +
1;
@@ -452,10 +399,11 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
{
struct mvebu_pcie_window desired = {.remap = MVEBU_MBUS_NO_REMAP};
+ struct pci_sw_bridge_conf *conf = &port->bridge.conf;
/* Are the new membase/memlimit values invalid? */
- if (port->bridge.memlimit < port->bridge.membase ||
- !(port->bridge.command & PCI_COMMAND_MEMORY)) {
+ if (conf->memlimit < conf->membase ||
+ !(conf->command & PCI_COMMAND_MEMORY)) {
mvebu_pcie_set_window(port, port->mem_target, port->mem_attr,
&desired, &port->memwin);
return;
@@ -467,130 +415,34 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
* window to setup, according to the PCI-to-PCI bridge
* specifications.
*/
- desired.base = ((port->bridge.membase & 0xFFF0) << 16);
- desired.size = (((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
+ desired.base = ((conf->membase & 0xFFF0) << 16);
+ desired.size = (((conf->memlimit & 0xFFF0) << 16) | 0xFFFFF) -
desired.base + 1;
mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, &desired,
&port->memwin);
}
-/*
- * Initialize the configuration space of the PCI-to-PCI bridge
- * associated with the given PCIe interface.
- */
-static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
+static pci_sw_bridge_read_status_t
+mvebu_pci_sw_bridge_pcie_read(struct pci_sw_bridge *bridge,
+ int reg, u32 *value)
{
- struct mvebu_sw_pci_bridge *bridge = &port->bridge;
-
- memset(bridge, 0, sizeof(struct mvebu_sw_pci_bridge));
-
- bridge->class = PCI_CLASS_BRIDGE_PCI;
- bridge->vendor = PCI_VENDOR_ID_MARVELL;
- bridge->device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
- bridge->revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
- bridge->header_type = PCI_HEADER_TYPE_BRIDGE;
- bridge->cache_line_size = 0x10;
-
- /* We support 32 bits I/O addressing */
- bridge->iobase = PCI_IO_RANGE_TYPE_32;
- bridge->iolimit = PCI_IO_RANGE_TYPE_32;
-
- /* Add capabilities */
- bridge->status = PCI_STATUS_CAP_LIST;
-}
-
-/*
- * Read the configuration space of the PCI-to-PCI bridge associated to
- * the given PCIe interface.
- */
-static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
- unsigned int where, int size, u32 *value)
-{
- struct mvebu_sw_pci_bridge *bridge = &port->bridge;
-
- switch (where & ~3) {
- case PCI_VENDOR_ID:
- *value = bridge->device << 16 | bridge->vendor;
- break;
-
- case PCI_COMMAND:
- *value = bridge->command | bridge->status << 16;
- break;
-
- case PCI_CLASS_REVISION:
- *value = bridge->class << 16 | bridge->interface << 8 |
- bridge->revision;
- break;
-
- case PCI_CACHE_LINE_SIZE:
- *value = bridge->bist << 24 | bridge->header_type << 16 |
- bridge->latency_timer << 8 | bridge->cache_line_size;
- break;
-
- case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
- *value = bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4];
- break;
-
- case PCI_PRIMARY_BUS:
- *value = (bridge->secondary_latency_timer << 24 |
- bridge->subordinate_bus << 16 |
- bridge->secondary_bus << 8 |
- bridge->primary_bus);
- break;
-
- case PCI_IO_BASE:
- if (!mvebu_has_ioport(port))
- *value = bridge->secondary_status << 16;
- else
- *value = (bridge->secondary_status << 16 |
- bridge->iolimit << 8 |
- bridge->iobase);
- break;
-
- case PCI_MEMORY_BASE:
- *value = (bridge->memlimit << 16 | bridge->membase);
- break;
-
- case PCI_PREF_MEMORY_BASE:
- *value = 0;
- break;
-
- case PCI_IO_BASE_UPPER16:
- *value = (bridge->iolimitupper << 16 | bridge->iobaseupper);
- break;
-
- case PCI_CAPABILITY_LIST:
- *value = PCISWCAP;
- break;
+ struct mvebu_pcie_port *port = bridge->data;
- case PCI_ROM_ADDRESS1:
- *value = 0;
- break;
-
- case PCI_INTERRUPT_LINE:
- /* LINE PIN MIN_GNT MAX_LAT */
- *value = 0;
- break;
-
- case PCISWCAP_EXP_LIST_ID:
- /* Set PCIe v2, root port, slot support */
- *value = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2 |
- PCI_EXP_FLAGS_SLOT) << 16 | PCI_CAP_ID_EXP;
- break;
-
- case PCISWCAP_EXP_DEVCAP:
+ switch (reg) {
+ case PCI_EXP_DEVCAP:
*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_DEVCAP);
break;
- case PCISWCAP_EXP_DEVCTL:
+ case PCI_EXP_DEVCTL:
*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_DEVCTL) &
~(PCI_EXP_DEVCTL_URRE | PCI_EXP_DEVCTL_FERE |
PCI_EXP_DEVCTL_NFERE | PCI_EXP_DEVCTL_CERE);
- *value |= bridge->pcie_devctl;
+ /* FIXME */
+ *value |= bridge->pcie_conf.devctl;
break;
- case PCISWCAP_EXP_LNKCAP:
+ case PCI_EXP_LNKCAP:
/*
* PCIe requires the clock power management capability to be
* hard-wired to zero for downstream ports
@@ -599,132 +451,86 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
~PCI_EXP_LNKCAP_CLKPM;
break;
- case PCISWCAP_EXP_LNKCTL:
+ case PCI_EXP_LNKCTL:
*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
break;
- case PCISWCAP_EXP_SLTCAP:
- *value = bridge->pcie_sltcap;
- break;
-
- case PCISWCAP_EXP_SLTCTL:
+ case PCI_EXP_SLTCTL:
*value = PCI_EXP_SLTSTA_PDS << 16;
break;
- case PCISWCAP_EXP_RTCTL:
- *value = bridge->pcie_rtctl;
- break;
-
- case PCISWCAP_EXP_RTSTA:
+ case PCI_EXP_RTSTA:
*value = mvebu_readl(port, PCIE_RC_RTSTA);
break;
- /* PCIe requires the v2 fields to be hard-wired to zero */
- case PCISWCAP_EXP_DEVCAP2:
- case PCISWCAP_EXP_DEVCTL2:
- case PCISWCAP_EXP_LNKCAP2:
- case PCISWCAP_EXP_LNKCTL2:
- case PCISWCAP_EXP_SLTCAP2:
- case PCISWCAP_EXP_SLTCTL2:
default:
- /*
- * PCI defines configuration read accesses to reserved or
- * unimplemented registers to read as zero and complete
- * normally.
- */
- *value = 0;
- return PCIBIOS_SUCCESSFUL;
+ return PCI_SW_BRIDGE_NOT_HANDLED;
}
- if (size == 2)
- *value = (*value >> (8 * (where & 3))) & 0xffff;
- else if (size == 1)
- *value = (*value >> (8 * (where & 3))) & 0xff;
-
- return PCIBIOS_SUCCESSFUL;
+ return PCI_SW_BRIDGE_HANDLED;
}
-/* Write to the PCI-to-PCI bridge configuration space */
-static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
- unsigned int where, int size, u32 value)
+static void mvebu_pci_sw_bridge_base_write(struct pci_sw_bridge *bridge,
+ int reg, u32 old, u32 new, u32 mask)
{
- struct mvebu_sw_pci_bridge *bridge = &port->bridge;
- u32 mask, reg;
- int err;
-
- if (size == 4)
- mask = 0x0;
- else if (size == 2)
- mask = ~(0xffff << ((where & 3) * 8));
- else if (size == 1)
- mask = ~(0xff << ((where & 3) * 8));
- else
- return PCIBIOS_BAD_REGISTER_NUMBER;
-
- err = mvebu_sw_pci_bridge_read(port, where & ~3, 4, ®);
- if (err)
- return err;
+ struct mvebu_pcie_port *port = bridge->data;
+ struct pci_sw_bridge_conf *conf = &bridge->conf;
- value = (reg & mask) | value << ((where & 3) * 8);
-
- switch (where & ~3) {
+ switch (reg) {
case PCI_COMMAND:
{
- u32 old = bridge->command;
-
if (!mvebu_has_ioport(port))
- value &= ~PCI_COMMAND_IO;
+ conf->command &= ~PCI_COMMAND_IO;
- bridge->command = value & 0xffff;
- if ((old ^ bridge->command) & PCI_COMMAND_IO)
+ if ((old ^ new) & PCI_COMMAND_IO)
mvebu_pcie_handle_iobase_change(port);
- if ((old ^ bridge->command) & PCI_COMMAND_MEMORY)
+ if ((old ^ new) & PCI_COMMAND_MEMORY)
mvebu_pcie_handle_membase_change(port);
- break;
- }
- case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
- bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4] = value;
break;
+ }
case PCI_IO_BASE:
/*
- * We also keep bit 1 set, it is a read-only bit that
+ * We keep bit 1 set, it is a read-only bit that
* indicates we support 32 bits addressing for the
* I/O
*/
- bridge->iobase = (value & 0xff) | PCI_IO_RANGE_TYPE_32;
- bridge->iolimit = ((value >> 8) & 0xff) | PCI_IO_RANGE_TYPE_32;
+ conf->iobase |= PCI_IO_RANGE_TYPE_32;
+ conf->iolimit |= PCI_IO_RANGE_TYPE_32;
mvebu_pcie_handle_iobase_change(port);
break;
case PCI_MEMORY_BASE:
- bridge->membase = value & 0xffff;
- bridge->memlimit = value >> 16;
mvebu_pcie_handle_membase_change(port);
break;
case PCI_IO_BASE_UPPER16:
- bridge->iobaseupper = value & 0xffff;
- bridge->iolimitupper = value >> 16;
mvebu_pcie_handle_iobase_change(port);
break;
case PCI_PRIMARY_BUS:
- bridge->primary_bus = value & 0xff;
- bridge->secondary_bus = (value >> 8) & 0xff;
- bridge->subordinate_bus = (value >> 16) & 0xff;
- bridge->secondary_latency_timer = (value >> 24) & 0xff;
- mvebu_pcie_set_local_bus_nr(port, bridge->secondary_bus);
+ mvebu_pcie_set_local_bus_nr(port, conf->secondary_bus);
break;
- case PCISWCAP_EXP_DEVCTL:
+ default:
+ break;
+ }
+}
+
+static void mvebu_pci_sw_bridge_pcie_write(struct pci_sw_bridge *bridge,
+ int reg, u32 old, u32 new, u32 mask)
+{
+ struct mvebu_pcie_port *port = bridge->data;
+
+ switch(reg) {
+ case PCI_EXP_DEVCTL:
/*
* Armada370 data says these bits must always
* be zero when in root complex mode.
*/
- value &= ~(PCI_EXP_DEVCTL_URRE | PCI_EXP_DEVCTL_FERE |
- PCI_EXP_DEVCTL_NFERE | PCI_EXP_DEVCTL_CERE);
+ new &= ~(PCI_EXP_DEVCTL_URRE | PCI_EXP_DEVCTL_FERE |
+ PCI_EXP_DEVCTL_NFERE | PCI_EXP_DEVCTL_CERE);
/*
* If the mask is 0xffff0000, then we only want to write
@@ -732,20 +538,20 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
* RW1C bits in the device status register. Mask out the
* status register bits.
*/
- if (mask == 0xffff0000)
- value &= 0xffff;
+ if (new == 0xffff0000)
+ new &= 0xffff;
- mvebu_writel(port, value, PCIE_CAP_PCIEXP + PCI_EXP_DEVCTL);
+ mvebu_writel(port, new, PCIE_CAP_PCIEXP + PCI_EXP_DEVCTL);
break;
- case PCISWCAP_EXP_LNKCTL:
+ case PCI_EXP_LNKCTL:
/*
* If we don't support CLKREQ, we must ensure that the
* CLKREQ enable bit always reads zero. Since we haven't
* had this capability, and it's dependent on board wiring,
* disable it for the time being.
*/
- value &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
+ new &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
/*
* If the mask is 0xffff0000, then we only want to write
@@ -754,21 +560,47 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
* RW1C status register bits.
*/
if (mask == 0xffff0000)
- value &= ~((PCI_EXP_LNKSTA_LABS |
- PCI_EXP_LNKSTA_LBMS) << 16);
+ new &= ~((PCI_EXP_LNKSTA_LABS |
+ PCI_EXP_LNKSTA_LBMS) << 16);
- mvebu_writel(port, value, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
+ mvebu_writel(port, new, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
break;
- case PCISWCAP_EXP_RTSTA:
- mvebu_writel(port, value, PCIE_RC_RTSTA);
+ case PCI_EXP_RTSTA:
+ mvebu_writel(port, new, PCIE_RC_RTSTA);
break;
+ }
+}
- default:
- break;
+struct pci_sw_bridge_ops mvebu_pci_sw_bridge_ops = {
+ .write_base = mvebu_pci_sw_bridge_base_write,
+ .read_pcie = mvebu_pci_sw_bridge_pcie_read,
+ .write_pcie = mvebu_pci_sw_bridge_pcie_write,
+};
+
+/*
+ * Initialize the configuration space of the PCI-to-PCI bridge
+ * associated with the given PCIe interface.
+ */
+static void mvebu_pci_sw_bridge_init(struct mvebu_pcie_port *port)
+{
+ struct pci_sw_bridge *bridge = &port->bridge;
+
+ bridge->conf.vendor = PCI_VENDOR_ID_MARVELL;
+ bridge->conf.device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
+ bridge->conf.revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
+
+ if (mvebu_has_ioport(port)) {
+ /* We support 32 bits I/O addressing */
+ bridge->conf.iobase = PCI_IO_RANGE_TYPE_32;
+ bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32;
}
- return PCIBIOS_SUCCESSFUL;
+ bridge->has_pcie = true;
+ bridge->data = port;
+ bridge->ops = &mvebu_pci_sw_bridge_ops;
+
+ pci_sw_bridge_init(bridge);
}
static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys)
@@ -788,8 +620,8 @@ static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,
if (bus->number == 0 && port->devfn == devfn)
return port;
if (bus->number != 0 &&
- bus->number >= port->bridge.secondary_bus &&
- bus->number <= port->bridge.subordinate_bus)
+ bus->number >= port->bridge.conf.secondary_bus &&
+ bus->number <= port->bridge.conf.subordinate_bus)
return port;
}
@@ -810,7 +642,7 @@ static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
/* Access the emulated PCI-to-PCI bridge */
if (bus->number == 0)
- return mvebu_sw_pci_bridge_write(port, where, size, val);
+ return pci_sw_bridge_write(&port->bridge, where, size, val);
if (!mvebu_pcie_link_up(port))
return PCIBIOS_DEVICE_NOT_FOUND;
@@ -838,7 +670,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
/* Access the emulated PCI-to-PCI bridge */
if (bus->number == 0)
- return mvebu_sw_pci_bridge_read(port, where, size, val);
+ return pci_sw_bridge_read(&port->bridge, where, size, val);
if (!mvebu_pcie_link_up(port)) {
*val = 0xffffffff;
@@ -1273,7 +1105,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
}
mvebu_pcie_set_local_dev_nr(port, 1);
- mvebu_sw_pci_bridge_init(port);
+ mvebu_pci_sw_bridge_init(port);
}
pcie->nports = i;
--
2.14.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
2018-06-29 9:22 ` Thomas Petazzoni
@ 2018-06-29 9:22 ` Thomas Petazzoni
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-06-29 9:22 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci
Cc: Russell King, Antoine Tenart, Gregory Clement, Maxime Chevallier,
Nadav Haklai, Victor Gu, Thomas Petazzoni, Miquèl Raynal,
Zachary Zhang, Wilson Ding, linux-arm-kernel
From: Zachary Zhang <zhangzg@marvell.com>
The PCI controller in the Marvell Armada 3720 does not implement a
root port PCI bridge at the hardware level. This causes a number of
problems when using PCIe switches or when the Max Payload size needs
to be aligned between the root complex and the endpoint.
Implementing an emulated root PCI bridge, like is already done in the
pci-mvebu driver for older Marvell platforms allows to solve those
issues, and also to support features such as ASR, PME, VC, HP.
Signed-off-by: Zachary Zhang <zhangzg@marvell.com>
[Thomas: convert to the common PCI software bridge logic.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
drivers/pci/controller/Kconfig | 1 +
drivers/pci/controller/pci-aardvark.c | 119 +++++++++++++++++++++++++++++++++-
2 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 0c307f804c8d..27b29b3f34e8 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -16,6 +16,7 @@ config PCI_AARDVARK
depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
depends on OF
depends on PCI_MSI_IRQ_DOMAIN
+ select PCI_SW_BRIDGE
help
Add support for Aardvark 64bit PCIe Host Controller. This
controller is part of the South Bridge of the Marvel Armada
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index d3172d5d3d35..486c41721c89 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -14,6 +14,7 @@
#include <linux/irqdomain.h>
#include <linux/kernel.h>
#include <linux/pci.h>
+#include <linux/pci-sw-bridge.h>
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/of_address.h>
@@ -22,10 +23,13 @@
#include "../pci.h"
/* PCIe core registers */
+#define PCIE_CORE_DEV_ID_REG 0x0
#define PCIE_CORE_CMD_STATUS_REG 0x4
#define PCIE_CORE_CMD_IO_ACCESS_EN BIT(0)
#define PCIE_CORE_CMD_MEM_ACCESS_EN BIT(1)
#define PCIE_CORE_CMD_MEM_IO_REQ_EN BIT(2)
+#define PCIE_CORE_DEV_REV_REG 0x8
+#define PCIE_CORE_PCIEXP_CAP 0xc0
#define PCIE_CORE_DEV_CTRL_STATS_REG 0xc8
#define PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE (0 << 4)
#define PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT 5
@@ -41,7 +45,10 @@
#define PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN BIT(6)
#define PCIE_CORE_ERR_CAPCTL_ECRC_CHCK BIT(7)
#define PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV BIT(8)
-
+#define PCIE_CORE_INT_A_ASSERT_ENABLE 1
+#define PCIE_CORE_INT_B_ASSERT_ENABLE 2
+#define PCIE_CORE_INT_C_ASSERT_ENABLE 3
+#define PCIE_CORE_INT_D_ASSERT_ENABLE 4
/* PIO registers base address and register offsets */
#define PIO_BASE_ADDR 0x4000
#define PIO_CTRL (PIO_BASE_ADDR + 0x0)
@@ -93,7 +100,9 @@
#define PCIE_CORE_CTRL2_STRICT_ORDER_ENABLE BIT(5)
#define PCIE_CORE_CTRL2_OB_WIN_ENABLE BIT(6)
#define PCIE_CORE_CTRL2_MSI_ENABLE BIT(10)
+#define PCIE_MSG_LOG_REG (CONTROL_BASE_ADDR + 0x30)
#define PCIE_ISR0_REG (CONTROL_BASE_ADDR + 0x40)
+#define PCIE_MSG_PM_PME_MASK BIT(7)
#define PCIE_ISR0_MASK_REG (CONTROL_BASE_ADDR + 0x44)
#define PCIE_ISR0_MSI_INT_PENDING BIT(24)
#define PCIE_ISR0_INTX_ASSERT(val) BIT(16 + (val))
@@ -207,6 +216,7 @@ struct advk_pcie {
struct mutex msi_used_lock;
u16 msi_msg;
int root_bus_nr;
+ struct pci_sw_bridge bridge;
};
static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -433,6 +443,101 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
return -ETIMEDOUT;
}
+static pci_sw_bridge_read_status_t
+advk_pci_sw_bridge_pcie_read(struct pci_sw_bridge *bridge,
+ int reg, u32 *value)
+{
+ struct advk_pcie *pcie = bridge->data;
+
+ switch (reg) {
+ case PCI_EXP_SLTCTL:
+ *value = PCI_EXP_SLTSTA_PDS << 16;
+ return PCI_SW_BRIDGE_HANDLED;
+
+ case PCI_EXP_RTCTL:
+ *value = (advk_readl(pcie, PCIE_ISR0_MASK_REG) & PCIE_MSG_PM_PME_MASK) ?
+ PCI_EXP_RTCTL_PMEIE : 0;
+ return PCI_SW_BRIDGE_HANDLED;
+
+ case PCI_EXP_RTSTA:
+ *value = (advk_readl(pcie, PCIE_ISR0_REG) & PCIE_MSG_PM_PME_MASK) << 16 |
+ (advk_readl(pcie, PCIE_MSG_LOG_REG) >> 16);
+ return PCI_SW_BRIDGE_HANDLED;
+
+ case PCI_CAP_LIST_ID:
+ case PCI_EXP_DEVCAP:
+ case PCI_EXP_DEVCTL:
+ case PCI_EXP_LNKCAP:
+ case PCI_EXP_LNKCTL:
+ *value = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg);
+ return PCI_SW_BRIDGE_HANDLED;
+ default:
+ return PCI_SW_BRIDGE_NOT_HANDLED;
+ }
+
+}
+
+static void advk_pci_sw_bridge_pcie_write(struct pci_sw_bridge *bridge,
+ int reg, u32 old, u32 new, u32 mask)
+{
+ struct advk_pcie *pcie = bridge->data;
+
+ switch (reg) {
+ case PCI_EXP_DEVCTL:
+ case PCI_EXP_LNKCTL:
+ advk_writel(pcie, new, PCIE_CORE_PCIEXP_CAP + reg);
+ break;
+
+ case PCI_EXP_RTCTL:
+ new = (new & PCI_EXP_RTCTL_PMEIE) << 3;
+ advk_writel(pcie, new, PCIE_ISR0_MASK_REG);
+ break;
+
+ case PCI_EXP_RTSTA:
+ new = (new & PCI_EXP_RTSTA_PME) >> 9;
+ advk_writel(pcie, new, PCIE_ISR0_REG);
+ break;
+
+ default:
+ break;
+ }
+}
+
+struct pci_sw_bridge_ops advk_pci_sw_bridge_ops = {
+ .read_pcie = advk_pci_sw_bridge_pcie_read,
+ .write_pcie = advk_pci_sw_bridge_pcie_write,
+};
+
+/*
+ * Initialize the configuration space of the PCI-to-PCI bridge
+ * associated with the given PCIe interface.
+ */
+static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
+{
+ struct pci_sw_bridge *bridge = &pcie->bridge;
+
+ bridge->conf.vendor = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff;
+ bridge->conf.device = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16;
+ bridge->conf.revision = advk_readl(pcie, PCIE_CORE_DEV_REV_REG) & 0xff;
+
+ /* Support 32 bits I/O addressing */
+ bridge->conf.iobase = PCI_IO_RANGE_TYPE_32;
+ bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32;
+
+ /* Support 64 bits memory pref */
+ bridge->conf.pref_mem_base = PCI_PREF_RANGE_TYPE_64;
+ bridge->conf.pref_mem_limit = PCI_PREF_RANGE_TYPE_64;
+
+ /* Support interrupt A for MSI feature */
+ bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
+
+ bridge->has_pcie = true;
+ bridge->data = pcie;
+ bridge->ops = &advk_pci_sw_bridge_ops;
+
+ pci_sw_bridge_init(bridge);
+}
+
static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
int where, int size, u32 *val)
{
@@ -445,6 +550,9 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
return PCIBIOS_DEVICE_NOT_FOUND;
}
+ if (bus->number == pcie->root_bus_nr)
+ return pci_sw_bridge_read(&pcie->bridge, where, size, val);
+
/* Start PIO */
advk_writel(pcie, 0, PIO_START);
advk_writel(pcie, 1, PIO_ISR);
@@ -452,7 +560,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
/* Program the control register */
reg = advk_readl(pcie, PIO_CTRL);
reg &= ~PIO_CTRL_TYPE_MASK;
- if (bus->number == pcie->root_bus_nr)
+ if (bus->primary == pcie->root_bus_nr)
reg |= PCIE_CONFIG_RD_TYPE0;
else
reg |= PCIE_CONFIG_RD_TYPE1;
@@ -497,6 +605,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
return PCIBIOS_DEVICE_NOT_FOUND;
+ if (bus->number == pcie->root_bus_nr)
+ return pci_sw_bridge_write(&pcie->bridge, where, size, val);
+
if (where % size)
return PCIBIOS_SET_FAILED;
@@ -507,7 +618,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
/* Program the control register */
reg = advk_readl(pcie, PIO_CTRL);
reg &= ~PIO_CTRL_TYPE_MASK;
- if (bus->number == pcie->root_bus_nr)
+ if (bus->primary == pcie->root_bus_nr)
reg |= PCIE_CONFIG_WR_TYPE0;
else
reg |= PCIE_CONFIG_WR_TYPE1;
@@ -922,6 +1033,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
advk_pcie_setup_hw(pcie);
+ advk_sw_pci_bridge_init(pcie);
+
ret = advk_pcie_init_irq_domain(pcie);
if (ret) {
dev_err(dev, "Failed to initialize irq\n");
--
2.14.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
@ 2018-06-29 9:22 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-06-29 9:22 UTC (permalink / raw)
To: linux-arm-kernel
From: Zachary Zhang <zhangzg@marvell.com>
The PCI controller in the Marvell Armada 3720 does not implement a
root port PCI bridge at the hardware level. This causes a number of
problems when using PCIe switches or when the Max Payload size needs
to be aligned between the root complex and the endpoint.
Implementing an emulated root PCI bridge, like is already done in the
pci-mvebu driver for older Marvell platforms allows to solve those
issues, and also to support features such as ASR, PME, VC, HP.
Signed-off-by: Zachary Zhang <zhangzg@marvell.com>
[Thomas: convert to the common PCI software bridge logic.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
drivers/pci/controller/Kconfig | 1 +
drivers/pci/controller/pci-aardvark.c | 119 +++++++++++++++++++++++++++++++++-
2 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 0c307f804c8d..27b29b3f34e8 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -16,6 +16,7 @@ config PCI_AARDVARK
depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
depends on OF
depends on PCI_MSI_IRQ_DOMAIN
+ select PCI_SW_BRIDGE
help
Add support for Aardvark 64bit PCIe Host Controller. This
controller is part of the South Bridge of the Marvel Armada
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index d3172d5d3d35..486c41721c89 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -14,6 +14,7 @@
#include <linux/irqdomain.h>
#include <linux/kernel.h>
#include <linux/pci.h>
+#include <linux/pci-sw-bridge.h>
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/of_address.h>
@@ -22,10 +23,13 @@
#include "../pci.h"
/* PCIe core registers */
+#define PCIE_CORE_DEV_ID_REG 0x0
#define PCIE_CORE_CMD_STATUS_REG 0x4
#define PCIE_CORE_CMD_IO_ACCESS_EN BIT(0)
#define PCIE_CORE_CMD_MEM_ACCESS_EN BIT(1)
#define PCIE_CORE_CMD_MEM_IO_REQ_EN BIT(2)
+#define PCIE_CORE_DEV_REV_REG 0x8
+#define PCIE_CORE_PCIEXP_CAP 0xc0
#define PCIE_CORE_DEV_CTRL_STATS_REG 0xc8
#define PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE (0 << 4)
#define PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT 5
@@ -41,7 +45,10 @@
#define PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN BIT(6)
#define PCIE_CORE_ERR_CAPCTL_ECRC_CHCK BIT(7)
#define PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV BIT(8)
-
+#define PCIE_CORE_INT_A_ASSERT_ENABLE 1
+#define PCIE_CORE_INT_B_ASSERT_ENABLE 2
+#define PCIE_CORE_INT_C_ASSERT_ENABLE 3
+#define PCIE_CORE_INT_D_ASSERT_ENABLE 4
/* PIO registers base address and register offsets */
#define PIO_BASE_ADDR 0x4000
#define PIO_CTRL (PIO_BASE_ADDR + 0x0)
@@ -93,7 +100,9 @@
#define PCIE_CORE_CTRL2_STRICT_ORDER_ENABLE BIT(5)
#define PCIE_CORE_CTRL2_OB_WIN_ENABLE BIT(6)
#define PCIE_CORE_CTRL2_MSI_ENABLE BIT(10)
+#define PCIE_MSG_LOG_REG (CONTROL_BASE_ADDR + 0x30)
#define PCIE_ISR0_REG (CONTROL_BASE_ADDR + 0x40)
+#define PCIE_MSG_PM_PME_MASK BIT(7)
#define PCIE_ISR0_MASK_REG (CONTROL_BASE_ADDR + 0x44)
#define PCIE_ISR0_MSI_INT_PENDING BIT(24)
#define PCIE_ISR0_INTX_ASSERT(val) BIT(16 + (val))
@@ -207,6 +216,7 @@ struct advk_pcie {
struct mutex msi_used_lock;
u16 msi_msg;
int root_bus_nr;
+ struct pci_sw_bridge bridge;
};
static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -433,6 +443,101 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
return -ETIMEDOUT;
}
+static pci_sw_bridge_read_status_t
+advk_pci_sw_bridge_pcie_read(struct pci_sw_bridge *bridge,
+ int reg, u32 *value)
+{
+ struct advk_pcie *pcie = bridge->data;
+
+ switch (reg) {
+ case PCI_EXP_SLTCTL:
+ *value = PCI_EXP_SLTSTA_PDS << 16;
+ return PCI_SW_BRIDGE_HANDLED;
+
+ case PCI_EXP_RTCTL:
+ *value = (advk_readl(pcie, PCIE_ISR0_MASK_REG) & PCIE_MSG_PM_PME_MASK) ?
+ PCI_EXP_RTCTL_PMEIE : 0;
+ return PCI_SW_BRIDGE_HANDLED;
+
+ case PCI_EXP_RTSTA:
+ *value = (advk_readl(pcie, PCIE_ISR0_REG) & PCIE_MSG_PM_PME_MASK) << 16 |
+ (advk_readl(pcie, PCIE_MSG_LOG_REG) >> 16);
+ return PCI_SW_BRIDGE_HANDLED;
+
+ case PCI_CAP_LIST_ID:
+ case PCI_EXP_DEVCAP:
+ case PCI_EXP_DEVCTL:
+ case PCI_EXP_LNKCAP:
+ case PCI_EXP_LNKCTL:
+ *value = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg);
+ return PCI_SW_BRIDGE_HANDLED;
+ default:
+ return PCI_SW_BRIDGE_NOT_HANDLED;
+ }
+
+}
+
+static void advk_pci_sw_bridge_pcie_write(struct pci_sw_bridge *bridge,
+ int reg, u32 old, u32 new, u32 mask)
+{
+ struct advk_pcie *pcie = bridge->data;
+
+ switch (reg) {
+ case PCI_EXP_DEVCTL:
+ case PCI_EXP_LNKCTL:
+ advk_writel(pcie, new, PCIE_CORE_PCIEXP_CAP + reg);
+ break;
+
+ case PCI_EXP_RTCTL:
+ new = (new & PCI_EXP_RTCTL_PMEIE) << 3;
+ advk_writel(pcie, new, PCIE_ISR0_MASK_REG);
+ break;
+
+ case PCI_EXP_RTSTA:
+ new = (new & PCI_EXP_RTSTA_PME) >> 9;
+ advk_writel(pcie, new, PCIE_ISR0_REG);
+ break;
+
+ default:
+ break;
+ }
+}
+
+struct pci_sw_bridge_ops advk_pci_sw_bridge_ops = {
+ .read_pcie = advk_pci_sw_bridge_pcie_read,
+ .write_pcie = advk_pci_sw_bridge_pcie_write,
+};
+
+/*
+ * Initialize the configuration space of the PCI-to-PCI bridge
+ * associated with the given PCIe interface.
+ */
+static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
+{
+ struct pci_sw_bridge *bridge = &pcie->bridge;
+
+ bridge->conf.vendor = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff;
+ bridge->conf.device = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16;
+ bridge->conf.revision = advk_readl(pcie, PCIE_CORE_DEV_REV_REG) & 0xff;
+
+ /* Support 32 bits I/O addressing */
+ bridge->conf.iobase = PCI_IO_RANGE_TYPE_32;
+ bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32;
+
+ /* Support 64 bits memory pref */
+ bridge->conf.pref_mem_base = PCI_PREF_RANGE_TYPE_64;
+ bridge->conf.pref_mem_limit = PCI_PREF_RANGE_TYPE_64;
+
+ /* Support interrupt A for MSI feature */
+ bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
+
+ bridge->has_pcie = true;
+ bridge->data = pcie;
+ bridge->ops = &advk_pci_sw_bridge_ops;
+
+ pci_sw_bridge_init(bridge);
+}
+
static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
int where, int size, u32 *val)
{
@@ -445,6 +550,9 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
return PCIBIOS_DEVICE_NOT_FOUND;
}
+ if (bus->number == pcie->root_bus_nr)
+ return pci_sw_bridge_read(&pcie->bridge, where, size, val);
+
/* Start PIO */
advk_writel(pcie, 0, PIO_START);
advk_writel(pcie, 1, PIO_ISR);
@@ -452,7 +560,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
/* Program the control register */
reg = advk_readl(pcie, PIO_CTRL);
reg &= ~PIO_CTRL_TYPE_MASK;
- if (bus->number == pcie->root_bus_nr)
+ if (bus->primary == pcie->root_bus_nr)
reg |= PCIE_CONFIG_RD_TYPE0;
else
reg |= PCIE_CONFIG_RD_TYPE1;
@@ -497,6 +605,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
return PCIBIOS_DEVICE_NOT_FOUND;
+ if (bus->number == pcie->root_bus_nr)
+ return pci_sw_bridge_write(&pcie->bridge, where, size, val);
+
if (where % size)
return PCIBIOS_SET_FAILED;
@@ -507,7 +618,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
/* Program the control register */
reg = advk_readl(pcie, PIO_CTRL);
reg &= ~PIO_CTRL_TYPE_MASK;
- if (bus->number == pcie->root_bus_nr)
+ if (bus->primary == pcie->root_bus_nr)
reg |= PCIE_CONFIG_WR_TYPE0;
else
reg |= PCIE_CONFIG_WR_TYPE1;
@@ -922,6 +1033,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
advk_pcie_setup_hw(pcie);
+ advk_sw_pci_bridge_init(pcie);
+
ret = advk_pcie_init_irq_domain(pcie);
if (ret) {
dev_err(dev, "Failed to initialize irq\n");
--
2.14.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
2018-06-29 9:22 ` Thomas Petazzoni
@ 2022-01-07 21:27 ` Bjorn Helgaas
-1 siblings, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2022-01-07 21:27 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, Russell King,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel
On Fri, Jun 29, 2018 at 11:22:31AM +0200, Thomas Petazzoni wrote:
> +static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> +{
> + struct pci_sw_bridge *bridge = &pcie->bridge;
> + /* Support interrupt A for MSI feature */
> + bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
Only 3.5 years later, IIUC, this is the value you get when you read
PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not
PCIE_CORE_INT_A_ASSERT_ENABLE.
Readers expect to get the values defined in the PCI spec, i.e.,
PCI_INTERRUPT_UNKNOWN
PCI_INTERRUPT_INTA
PCI_INTERRUPT_INTB
PCI_INTERRUPT_INTC
PCI_INTERRUPT_INTD
Bjorn
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
@ 2022-01-07 21:27 ` Bjorn Helgaas
0 siblings, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2022-01-07 21:27 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, Russell King,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel
On Fri, Jun 29, 2018 at 11:22:31AM +0200, Thomas Petazzoni wrote:
> +static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> +{
> + struct pci_sw_bridge *bridge = &pcie->bridge;
> + /* Support interrupt A for MSI feature */
> + bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
Only 3.5 years later, IIUC, this is the value you get when you read
PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not
PCIE_CORE_INT_A_ASSERT_ENABLE.
Readers expect to get the values defined in the PCI spec, i.e.,
PCI_INTERRUPT_UNKNOWN
PCI_INTERRUPT_INTA
PCI_INTERRUPT_INTB
PCI_INTERRUPT_INTC
PCI_INTERRUPT_INTD
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
2022-01-07 21:27 ` Bjorn Helgaas
@ 2022-01-07 23:17 ` Bjorn Helgaas
-1 siblings, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2022-01-07 23:17 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, Russell King,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel, Pali Rohár
[+cc Pali; sorry, I meant to cc you on this but forgot]
On Fri, Jan 07, 2022 at 03:27:38PM -0600, Bjorn Helgaas wrote:
> On Fri, Jun 29, 2018 at 11:22:31AM +0200, Thomas Petazzoni wrote:
>
> > +static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> > +{
> > + struct pci_sw_bridge *bridge = &pcie->bridge;
>
> > + /* Support interrupt A for MSI feature */
> > + bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
>
> Only 3.5 years later, IIUC, this is the value you get when you read
> PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not
> PCIE_CORE_INT_A_ASSERT_ENABLE.
>
> Readers expect to get the values defined in the PCI spec, i.e.,
>
> PCI_INTERRUPT_UNKNOWN
> PCI_INTERRUPT_INTA
> PCI_INTERRUPT_INTB
> PCI_INTERRUPT_INTC
> PCI_INTERRUPT_INTD
>
> Bjorn
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
@ 2022-01-07 23:17 ` Bjorn Helgaas
0 siblings, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2022-01-07 23:17 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, Russell King,
Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav Haklai,
Victor Gu, Miquèl Raynal, Zachary Zhang, Wilson Ding,
linux-arm-kernel, Pali Rohár
[+cc Pali; sorry, I meant to cc you on this but forgot]
On Fri, Jan 07, 2022 at 03:27:38PM -0600, Bjorn Helgaas wrote:
> On Fri, Jun 29, 2018 at 11:22:31AM +0200, Thomas Petazzoni wrote:
>
> > +static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> > +{
> > + struct pci_sw_bridge *bridge = &pcie->bridge;
>
> > + /* Support interrupt A for MSI feature */
> > + bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
>
> Only 3.5 years later, IIUC, this is the value you get when you read
> PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not
> PCIE_CORE_INT_A_ASSERT_ENABLE.
>
> Readers expect to get the values defined in the PCI spec, i.e.,
>
> PCI_INTERRUPT_UNKNOWN
> PCI_INTERRUPT_INTA
> PCI_INTERRUPT_INTB
> PCI_INTERRUPT_INTC
> PCI_INTERRUPT_INTD
>
> Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
2022-01-07 23:17 ` Bjorn Helgaas
@ 2022-01-10 9:17 ` Pali Rohár
-1 siblings, 0 replies; 30+ messages in thread
From: Pali Rohár @ 2022-01-10 9:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Thomas Petazzoni, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci,
Russell King, Antoine Tenart, Gregory Clement, Maxime Chevallier,
Nadav Haklai, Victor Gu, Miquèl Raynal, Zachary Zhang,
Wilson Ding, linux-arm-kernel
On Friday 07 January 2022 17:17:34 Bjorn Helgaas wrote:
> [+cc Pali; sorry, I meant to cc you on this but forgot]
>
> On Fri, Jan 07, 2022 at 03:27:38PM -0600, Bjorn Helgaas wrote:
> > On Fri, Jun 29, 2018 at 11:22:31AM +0200, Thomas Petazzoni wrote:
> >
> > > +static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> > > +{
> > > + struct pci_sw_bridge *bridge = &pcie->bridge;
> >
> > > + /* Support interrupt A for MSI feature */
> > > + bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
> >
> > Only 3.5 years later, IIUC, this is the value you get when you read
> > PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not
> > PCIE_CORE_INT_A_ASSERT_ENABLE.
> >
> > Readers expect to get the values defined in the PCI spec, i.e.,
> >
> > PCI_INTERRUPT_UNKNOWN
> > PCI_INTERRUPT_INTA
> > PCI_INTERRUPT_INTB
> > PCI_INTERRUPT_INTC
> > PCI_INTERRUPT_INTD
> >
> > Bjorn
Yes! We have a prepared patch for it and Marek now sent it:
https://lore.kernel.org/linux-pci/20220110015018.26359-2-kabel@kernel.org/
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
@ 2022-01-10 9:17 ` Pali Rohár
0 siblings, 0 replies; 30+ messages in thread
From: Pali Rohár @ 2022-01-10 9:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Thomas Petazzoni, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci,
Russell King, Antoine Tenart, Gregory Clement, Maxime Chevallier,
Nadav Haklai, Victor Gu, Miquèl Raynal, Zachary Zhang,
Wilson Ding, linux-arm-kernel
On Friday 07 January 2022 17:17:34 Bjorn Helgaas wrote:
> [+cc Pali; sorry, I meant to cc you on this but forgot]
>
> On Fri, Jan 07, 2022 at 03:27:38PM -0600, Bjorn Helgaas wrote:
> > On Fri, Jun 29, 2018 at 11:22:31AM +0200, Thomas Petazzoni wrote:
> >
> > > +static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> > > +{
> > > + struct pci_sw_bridge *bridge = &pcie->bridge;
> >
> > > + /* Support interrupt A for MSI feature */
> > > + bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
> >
> > Only 3.5 years later, IIUC, this is the value you get when you read
> > PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not
> > PCIE_CORE_INT_A_ASSERT_ENABLE.
> >
> > Readers expect to get the values defined in the PCI spec, i.e.,
> >
> > PCI_INTERRUPT_UNKNOWN
> > PCI_INTERRUPT_INTA
> > PCI_INTERRUPT_INTB
> > PCI_INTERRUPT_INTC
> > PCI_INTERRUPT_INTD
> >
> > Bjorn
Yes! We have a prepared patch for it and Marek now sent it:
https://lore.kernel.org/linux-pci/20220110015018.26359-2-kabel@kernel.org/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
2022-01-07 21:27 ` Bjorn Helgaas
@ 2022-01-10 2:21 ` Marek Behún
-1 siblings, 0 replies; 30+ messages in thread
From: Marek Behún @ 2022-01-10 2:21 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Thomas Petazzoni, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci,
Russell King, Antoine Tenart, Gregory Clement, Maxime Chevallier,
Nadav Haklai, Victor Gu, Miquèl Raynal, Zachary Zhang,
Wilson Ding, linux-arm-kernel
On Fri, 7 Jan 2022 15:27:36 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Jun 29, 2018 at 11:22:31AM +0200, Thomas Petazzoni wrote:
>
> > +static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> > +{
> > + struct pci_sw_bridge *bridge = &pcie->bridge;
>
> > + /* Support interrupt A for MSI feature */
> > + bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
>
> Only 3.5 years later, IIUC, this is the value you get when you read
> PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not
> PCIE_CORE_INT_A_ASSERT_ENABLE.
>
> Readers expect to get the values defined in the PCI spec, i.e.,
>
> PCI_INTERRUPT_UNKNOWN
> PCI_INTERRUPT_INTA
> PCI_INTERRUPT_INTB
> PCI_INTERRUPT_INTC
> PCI_INTERRUPT_INTD
Hello Bjorn,
now sent v2 of batch 4 of fixes for pci-aardvark, where this is fixed
in the first patch.
https://lore.kernel.org/linux-pci/20220110015018.26359-1-kabel@kernel.org/
Could you find time to review the series? :-)
Marek
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge
@ 2022-01-10 2:21 ` Marek Behún
0 siblings, 0 replies; 30+ messages in thread
From: Marek Behún @ 2022-01-10 2:21 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Thomas Petazzoni, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci,
Russell King, Antoine Tenart, Gregory Clement, Maxime Chevallier,
Nadav Haklai, Victor Gu, Miquèl Raynal, Zachary Zhang,
Wilson Ding, linux-arm-kernel
On Fri, 7 Jan 2022 15:27:36 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Jun 29, 2018 at 11:22:31AM +0200, Thomas Petazzoni wrote:
>
> > +static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> > +{
> > + struct pci_sw_bridge *bridge = &pcie->bridge;
>
> > + /* Support interrupt A for MSI feature */
> > + bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
>
> Only 3.5 years later, IIUC, this is the value you get when you read
> PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not
> PCIE_CORE_INT_A_ASSERT_ENABLE.
>
> Readers expect to get the values defined in the PCI spec, i.e.,
>
> PCI_INTERRUPT_UNKNOWN
> PCI_INTERRUPT_INTA
> PCI_INTERRUPT_INTB
> PCI_INTERRUPT_INTC
> PCI_INTERRUPT_INTD
Hello Bjorn,
now sent v2 of batch 4 of fixes for pci-aardvark, where this is fixed
in the first patch.
https://lore.kernel.org/linux-pci/20220110015018.26359-1-kabel@kernel.org/
Could you find time to review the series? :-)
Marek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/3] PCI: emulated PCI bridge common logic
2018-06-29 9:22 ` Thomas Petazzoni
@ 2018-07-12 9:24 ` Thomas Petazzoni
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-07-12 9:24 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci
Cc: Russell King, Antoine Tenart, Gregory Clement, Maxime Chevallier,
Nadav Haklai, Victor Gu, Miquèl Raynal, Zachary Zhang,
Wilson Ding, linux-arm-kernel
Bjorn, Lorenzo,
On Fri, 29 Jun 2018 11:22:28 +0200, Thomas Petazzoni wrote:
> The pci-mvebu driver already contains some logic to emulate a root
> port PCI bridge. It turns out that we have a similar need for the
> pci-aardvark driver. Instead of duplicating the same logic in two
> drivers, this patch series starts by adding a small common
> infrastructure that helps emulate a root port PCI bridge, converts
> pci-mvebu to use it, and finally extends pci-aardvark to use it as
> well.
>
> Thanks to this, Marvell Armada 3720 based systems, which use the
> Aarkvark PCI controller, will have better PCI support, by having a
> root port PCI bridge exposed.
>
> The emulated PCI bridge common logic is a proposal, I very much
> welcome comments and suggestions. Also, if you feel that adding a
> common logic for only two drivers is too early, I'm fine with
> duplicating a bit of code betwen pci-mvebu and pci-aardvark.
I was wondering if you had any feedback on this patch series. Not
necessarily a detailed review, but at least some general
feeling/feedback on the overall approach would be very nice.
Thanks a lot,
Thomas Petazzoni
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 0/3] PCI: emulated PCI bridge common logic
@ 2018-07-12 9:24 ` Thomas Petazzoni
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Petazzoni @ 2018-07-12 9:24 UTC (permalink / raw)
To: linux-arm-kernel
Bjorn, Lorenzo,
On Fri, 29 Jun 2018 11:22:28 +0200, Thomas Petazzoni wrote:
> The pci-mvebu driver already contains some logic to emulate a root
> port PCI bridge. It turns out that we have a similar need for the
> pci-aardvark driver. Instead of duplicating the same logic in two
> drivers, this patch series starts by adding a small common
> infrastructure that helps emulate a root port PCI bridge, converts
> pci-mvebu to use it, and finally extends pci-aardvark to use it as
> well.
>
> Thanks to this, Marvell Armada 3720 based systems, which use the
> Aarkvark PCI controller, will have better PCI support, by having a
> root port PCI bridge exposed.
>
> The emulated PCI bridge common logic is a proposal, I very much
> welcome comments and suggestions. Also, if you feel that adding a
> common logic for only two drivers is too early, I'm fine with
> duplicating a bit of code betwen pci-mvebu and pci-aardvark.
I was wondering if you had any feedback on this patch series. Not
necessarily a detailed review, but at least some general
feeling/feedback on the overall approach would be very nice.
Thanks a lot,
Thomas Petazzoni
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 30+ messages in thread