* [PATCH 0/3] PCI: emulated PCI bridge common logic
@ 2018-06-29 9:22 Thomas Petazzoni
2018-06-29 9:22 ` [PATCH 1/3] PCI: Introduce PCI software " Thomas Petazzoni
` (3 more replies)
0 siblings, 4 replies; 15+ 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
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
_______________________________________________
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] 15+ messages in thread* [PATCH 1/3] PCI: Introduce PCI software bridge common logic 2018-06-29 9:22 [PATCH 0/3] PCI: emulated PCI bridge common logic Thomas Petazzoni @ 2018-06-29 9:22 ` Thomas Petazzoni 2018-07-12 19:58 ` Bjorn Helgaas 2018-06-29 9:22 ` [PATCH 2/3] PCI: mvebu: Convert to PCI software bridge Thomas Petazzoni ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ 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] 15+ messages in thread
* Re: [PATCH 1/3] PCI: Introduce PCI software bridge common logic 2018-06-29 9:22 ` [PATCH 1/3] PCI: Introduce PCI software " Thomas Petazzoni @ 2018-07-12 19:58 ` Bjorn Helgaas 2018-08-01 8:49 ` Thomas Petazzoni 0 siblings, 1 reply; 15+ 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] 15+ 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 2018-08-01 9:21 ` Russell King - ARM Linux 2018-08-01 11:13 ` Thomas Petazzoni 0 siblings, 2 replies; 15+ 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] 15+ 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 2018-08-01 9:37 ` Thomas Petazzoni 2018-08-01 9:54 ` Thomas Petazzoni 2018-08-01 11:13 ` Thomas Petazzoni 1 sibling, 2 replies; 15+ 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] 15+ 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 2018-08-01 9:54 ` Thomas Petazzoni 1 sibling, 0 replies; 15+ 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] 15+ 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 @ 2018-08-01 9:54 ` Thomas Petazzoni 1 sibling, 0 replies; 15+ 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] 15+ 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 @ 2018-08-01 11:13 ` Thomas Petazzoni 1 sibling, 0 replies; 15+ 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] 15+ messages in thread
* [PATCH 2/3] PCI: mvebu: Convert to PCI software bridge 2018-06-29 9:22 [PATCH 0/3] PCI: emulated PCI bridge common logic Thomas Petazzoni 2018-06-29 9:22 ` [PATCH 1/3] PCI: Introduce PCI software " Thomas Petazzoni @ 2018-06-29 9:22 ` Thomas Petazzoni 2018-06-29 9:22 ` [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge Thomas Petazzoni 2018-07-12 9:24 ` [PATCH 0/3] PCI: emulated PCI bridge common logic Thomas Petazzoni 3 siblings, 0 replies; 15+ 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] 15+ messages in thread
* [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge 2018-06-29 9:22 [PATCH 0/3] PCI: emulated PCI bridge common logic Thomas Petazzoni 2018-06-29 9:22 ` [PATCH 1/3] PCI: Introduce PCI software " Thomas Petazzoni 2018-06-29 9:22 ` [PATCH 2/3] PCI: mvebu: Convert to PCI software bridge Thomas Petazzoni @ 2018-06-29 9:22 ` Thomas Petazzoni 2022-01-07 21:27 ` Bjorn Helgaas 2018-07-12 9:24 ` [PATCH 0/3] PCI: emulated PCI bridge common logic Thomas Petazzoni 3 siblings, 1 reply; 15+ 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] 15+ messages in thread
* Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge 2018-06-29 9:22 ` [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge Thomas Petazzoni @ 2022-01-07 21:27 ` Bjorn Helgaas 2022-01-07 23:17 ` Bjorn Helgaas 2022-01-10 2:21 ` Marek Behún 0 siblings, 2 replies; 15+ 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] 15+ 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 2022-01-10 9:17 ` Pali Rohár 2022-01-10 2:21 ` Marek Behún 1 sibling, 1 reply; 15+ 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] 15+ 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 0 siblings, 0 replies; 15+ 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] 15+ 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 @ 2022-01-10 2:21 ` Marek Behún 1 sibling, 0 replies; 15+ 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] 15+ messages in thread
* Re: [PATCH 0/3] PCI: emulated PCI bridge common logic 2018-06-29 9:22 [PATCH 0/3] PCI: emulated PCI bridge common logic Thomas Petazzoni ` (2 preceding siblings ...) 2018-06-29 9:22 ` [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge Thomas Petazzoni @ 2018-07-12 9:24 ` Thomas Petazzoni 3 siblings, 0 replies; 15+ 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] 15+ messages in thread
end of thread, other threads:[~2022-01-10 9:17 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-29 9:22 [PATCH 0/3] PCI: emulated PCI bridge common logic Thomas Petazzoni 2018-06-29 9:22 ` [PATCH 1/3] PCI: Introduce PCI software " Thomas Petazzoni 2018-07-12 19:58 ` Bjorn Helgaas 2018-08-01 8:49 ` Thomas Petazzoni 2018-08-01 9:21 ` Russell King - ARM Linux 2018-08-01 9:37 ` Thomas Petazzoni 2018-08-01 9:54 ` Thomas Petazzoni 2018-08-01 11:13 ` Thomas Petazzoni 2018-06-29 9:22 ` [PATCH 2/3] PCI: mvebu: Convert to PCI software bridge Thomas Petazzoni 2018-06-29 9:22 ` [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge Thomas Petazzoni 2022-01-07 21:27 ` Bjorn Helgaas 2022-01-07 23:17 ` Bjorn Helgaas 2022-01-10 9:17 ` Pali Rohár 2022-01-10 2:21 ` Marek Behún 2018-07-12 9:24 ` [PATCH 0/3] PCI: emulated PCI bridge common logic Thomas Petazzoni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).