From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [RFC,v2,04/15] usb:cdns3: Driver initialization code. From: Roger Quadros Message-Id: <5C065AEE.4010205@ti.com> Date: Tue, 4 Dec 2018 12:46:06 +0200 To: Peter Chen Cc: pawell@cadence.com, devicetree@vger.kernel.org, Greg Kroah-Hartman , linux-usb@vger.kernel.org, lkml , adouglas@cadence.com, jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com, pjez@cadence.com, kurahul@cadence.com List-ID: T24gMDQvMTIvMTggMTA6NTAsIFBldGVyIENoZW4gd3JvdGU6Cj4+PiArICogQ2FkZW5jZSBVU0JT UyBEUkQgRHJpdmVyLgo+Pj4gKyAqCj4+PiArICogQ29weXJpZ2h0IChDKSAyMDE4IENhZGVuY2Uu Cj4+PiArICoKPj4+ICsgKiBBdXRob3I6IFBldGVyIENoZW4gPHBldGVyLmNoZW5AbnhwLmNvbT4K Pj4+ICsgKiAgICAgICAgIFBhd2VsIExhc3pjemFrIDxwYXdlbGxAY2FkZW5jZS5jb20+Cj4+PiAr ICovCj4+PiArCj4+PiArI2luY2x1ZGUgPGxpbnV4L21vZHVsZS5oPgo+Pj4gKyNpbmNsdWRlIDxs aW51eC9rZXJuZWwuaD4KPj4+ICsjaW5jbHVkZSA8bGludXgvcGxhdGZvcm1fZGV2aWNlLmg+Cj4+ PiArI2luY2x1ZGUgPGxpbnV4L2ludGVycnVwdC5oPgo+Pj4gKyNpbmNsdWRlIDxsaW51eC9pby5o Pgo+Pj4gKyNpbmNsdWRlIDxsaW51eC9wbV9ydW50aW1lLmg+Cj4+PiArCj4+PiArI2luY2x1ZGUg ImdhZGdldC5oIgo+Pj4gKyNpbmNsdWRlICJjb3JlLmgiCj4+PiArCj4+PiArc3RhdGljIGlubGlu ZSBzdHJ1Y3QgY2RuczNfcm9sZV9kcml2ZXIgKmNkbnMzX2dldF9jdXJyZW50X3JvbGVfZHJpdmVy KHN0cnVjdCBjZG5zMyAqY2RucykKPj4+ICt7Cj4+PiArICAgICBXQVJOX09OKGNkbnMtPnJvbGUg Pj0gQ0ROUzNfUk9MRV9FTkQgfHwgIWNkbnMtPnJvbGVzW2NkbnMtPnJvbGVdKTsKPj4+ICsgICAg IHJldHVybiBjZG5zLT5yb2xlc1tjZG5zLT5yb2xlXTsKPj4+ICt9Cj4+PiArCj4+PiArc3RhdGlj IGlubGluZSBpbnQgY2RuczNfcm9sZV9zdGFydChzdHJ1Y3QgY2RuczMgKmNkbnMsIGVudW0gY2Ru czNfcm9sZXMgcm9sZSkKPj4+ICt7Cj4+PiArICAgICBpbnQgcmV0Owo+Pj4gKwo+Pj4gKyAgICAg aWYgKHJvbGUgPj0gQ0ROUzNfUk9MRV9FTkQpCj4+Cj4+IFdBUk5fT04oKT8KPj4KPj4+ICsgICAg ICAgICAgICAgcmV0dXJuIDA7Cj4+PiArCj4+PiArICAgICBpZiAoIWNkbnMtPnJvbGVzW3JvbGVd KQo+Pj4gKyAgICAgICAgICAgICByZXR1cm4gLUVOWElPOwo+Pj4gKwo+Pj4gKyAgICAgbXV0ZXhf bG9jaygmY2Rucy0+bXV0ZXgpOwo+Pj4gKyAgICAgY2Rucy0+cm9sZSA9IHJvbGU7Cj4+PiArICAg ICByZXQgPSBjZG5zLT5yb2xlc1tyb2xlXS0+c3RhcnQoY2Rucyk7Cj4+PiArICAgICBtdXRleF91 bmxvY2soJmNkbnMtPm11dGV4KTsKPj4+ICsgICAgIHJldHVybiByZXQ7Cj4+PiArfQo+Pj4gKwo+ Pj4gK3N0YXRpYyBpbmxpbmUgdm9pZCBjZG5zM19yb2xlX3N0b3Aoc3RydWN0IGNkbnMzICpjZG5z KQo+Pj4gK3sKPj4+ICsgICAgIGVudW0gY2RuczNfcm9sZXMgcm9sZSA9IGNkbnMtPnJvbGU7Cj4+ PiArCj4+PiArICAgICBpZiAocm9sZSA9PSBDRE5TM19ST0xFX0VORCkKPj4KPj4gV0FSTl9PTihy b2xlID49IENORFMzX1JPTEVfRU5EKSA/Cj4+Cj4+PiArICAgICAgICAgICAgIHJldHVybjsKPj4+ ICsKPj4+ICsgICAgIG11dGV4X2xvY2soJmNkbnMtPm11dGV4KTsKPj4+ICsgICAgIGNkbnMtPnJv bGVzW3JvbGVdLT5zdG9wKGNkbnMpOwo+Pj4gKyAgICAgY2Rucy0+cm9sZSA9IENETlMzX1JPTEVf RU5EOwo+Pgo+PiBXaHkgY2hhbmdlIHRoZSByb2xlIGhlcmU/IFlvdSBhcmUganVzdCBzdG9wcGlu ZyB0aGUgcm9sZSBub3QgY2hhbmdpbmcgaXQuCj4+IEkgdGhpbmsgY2Rucy0+cm9sZSBzaG91bGQg cmVtYWluIHVuY2hhbmdlZCwgc28gd2UgY2FuIGNhbGwgY2RuczNfcm9sZV9zdGFydCgpCj4+IGlm IHJlcXVpcmVkIHdpdGhvdXQgZXJyb3IuCj4+Cj4gCj4gVGhlIGN1cnJlbnQgdmVyc2lvbiBvZiB0 aGlzIElQIGhhcyBzb21lIGlzc3VlcyB0byBkZXRlY3QgdmJ1cyBzdGF0dXMgY29ycmVjdGx5LAo+ IHdlIGhhdmUgdG8gZm9yY2UgdmJ1cyBzdGF0dXMgYWNjb3JkaW5nbHksIHNvIHdlIG5lZWQgYSBz dGF0dXMgdG8gaW5kaWNhdGUKPiB2YnVzIGRpc2Nvbm5lY3Rpb24sIGFuZCBhZGQgc29tZSBjb2Rl IHRvIGxldCBjb250cm9sbGVyIGtub3cgdmJ1cwo+IHJlbW92YWwsIGluIHRoYXQgY2FzZSwgdGhl IGNvbnRyb2xsZXIncyBzdGF0ZSBtYWNoaW5lIGNhbiBiZSBjb3JyZWN0Lgo+IFNvLCB3ZSBpbmNy ZWFzZSBvbmUgcm9sZSAnQ0ROUzNfUk9MRV9FTkQnIHRvIGZvciB0aGlzIHB1cnBvc2UuCj4gCj4g Q0ROUzNfUk9MRV9HQURHRVQ6IGdhZGdldCBtb2RlIGFuZCBWQlVTIG9uCj4gQ0ROUzNfUk9MRV9I T1NUOiBob3N0IG1vZGUgYW5kIFZCVVMgb24KPiBDRE5TM19ST0xFX0VORDogVkJVUyBvZmYsIGVn IGVpdGhlciBob3N0IG9yIGRldmljZSBjYWJsZSBvbiB0aGUgcG9ydC4KPiAKPiBTbywgd2UgbWF5 IHN0YXJ0IHJvbGUgZnJvbSBDRE5TM19ST0xFX0VORCBhdCBwcm9iZSB3aGVuIG5vdGhpbmcgaXMg Y29ubmVjdGVkLAo+IGFuZCBuZWVkIHRvIHNldCByb2xlIGFzIENETlMzX1JPTEVfRU5EIGF0IC0+ c3RvcCBmb3IgZnVydGhlciBoYW5kbGluZyBhdAo+IHJvbGUgc3dpdGNoIHJvdXRpbmUuCgpPSy4g YnV0IHN0aWxsIHRoaXMgKGNoYW5naW5nIHRvIFJPTEVfRU5EKSBtdXN0IGJlIG1vdmVkIHRvIHRo ZSByb2xlIHN3aXRjaCByb3V0aW5lCmFuZCB0aGUgZXhwbGFuYXRpb24geW91IGp1c3QgbWVudGlv bmVkIG11c3QgYmUgYWRkZWQgdGhlcmUuCgo+IAo+Pj4gKyAgICAgbXV0ZXhfdW5sb2NrKCZjZG5z LT5tdXRleCk7Cj4+PiArfQo+Pj4gKwo+Pj4gK3N0YXRpYyBlbnVtIGNkbnMzX3JvbGVzIGNkbnMz X2dldF9yb2xlKHN0cnVjdCBjZG5zMyAqY2RucykKPj4+ICt7Cj4+PiArICAgICBpZiAoY2Rucy0+ cm9sZXNbQ0ROUzNfUk9MRV9IT1NUXSAmJiBjZG5zLT5yb2xlc1tDRE5TM19ST0xFX0dBREdFVF0p IHsKPj4+ICsgICAgICAgICAgICAgLy9UT0RPOiBpbXBsZW1lbnRzIHNlbGVjdGluZyBkZXZpY2Uv aG9zdCBtb2RlCj4+PiArICAgICAgICAgICAgIHJldHVybiBDRE5TM19ST0xFX0hPU1Q7Cj4+PiAr ICAgICB9Cj4+PiArICAgICByZXR1cm4gY2Rucy0+cm9sZXNbQ0ROUzNfUk9MRV9IT1NUXQo+Pj4g KyAgICAgICAgICAgICA/IENETlMzX1JPTEVfSE9TVAo+Pj4gKyAgICAgICAgICAgICA6IENETlMz X1JPTEVfR0FER0VUOwo+Pgo+PiBXaHkgbm90IGp1c3QKPj4gICAgICAgICByZXR1cm4gY2Rucy0+ cm9sZTsKPj4KPj4gSSdtIHdvbmRlcmluZyBpZiB3ZSByZWFsbHkgbmVlZCB0aGlzIGZ1bmN0aW9u Lgo+IAo+IGNkbnMtPnJvbGUgZ2V0cyBmcm9tIGNkbnMzX2dldF9yb2xlLCBhbmQgdGhpcyBBUEkg dGVsbHMgcm9sZSBhdCB0aGUgcnVudGltZS4KPiBJZiBib3RoIHJvbGVzIGFyZSBzdXBwb3J0ZWQs IHRoZSByb2xlIGlzIGRlY2lkZWQgYnkgZXh0ZXJuYWwKPiBjb25kaXRpb25zLCBlZywgdmJ1cy9p ZAo+IG9yIGV4dGVybmFsIGNvbm5lY3Rvci4gSWYgb25seSBzaW5nbGUgcm9sZSBpcyBzdXBwb3J0 ZWQsIG9ubHkgb25lIHJvbGUgc3RydWN0dXJlCj4gaXMgYWxsb2NhdGVkLCBjZG5zLT5yb2xlc1tD RE5TM19ST0xFX0hPU1RdIG9yIGNkbnMtPnJvbGVzW0NETlMzX1JPTEVfR0FER0VUXQo+IAoKSG93 IGFib3V0IGFkZGluZyB0aGlzIGRlc2NyaXB0aW9uIGluIGZ1bmN0aW9uIGRvY3VtZW50YXRpb24u Cgo+Pj4gK30KPj4KPj4+ICsKPj4+ICsvKioKPj4+ICsgKiBjZG5zM19jb3JlX2luaXRfcm9sZSAt IGluaXRpYWxpemUgcm9sZSBvZiBvcGVyYXRpb24KPj4+ICsgKiBAY2RuczogUG9pbnRlciB0byBj ZG5zMyBzdHJ1Y3R1cmUKPj4+ICsgKgo+Pj4gKyAqIFJldHVybnMgMCBvbiBzdWNjZXNzIG90aGVy d2lzZSBuZWdhdGl2ZSBlcnJubwo+Pj4gKyAqLwo+Pj4gK3N0YXRpYyBpbnQgY2RuczNfY29yZV9p bml0X3JvbGUoc3RydWN0IGNkbnMzICpjZG5zKQo+Pj4gK3sKPj4+ICsgICAgIHN0cnVjdCBkZXZp Y2UgKmRldiA9IGNkbnMtPmRldjsKPj4+ICsgICAgIGVudW0gdXNiX2RyX21vZGUgZHJfbW9kZTsK Pj4+ICsKPj4+ICsgICAgIGRyX21vZGUgPSB1c2JfZ2V0X2RyX21vZGUoZGV2KTsKPj4+ICsgICAg IGNkbnMtPnJvbGUgPSBDRE5TM19ST0xFX0VORDsKPj4+ICsKPj4+ICsgICAgIC8qCj4+PiArICAg ICAgKiBJZiBkcml2ZXIgY2FuJ3QgcmVhZCBtb2RlIGJ5IG1lYW5zIG9mIHVzYl9nZXRfZHJfbWRv ZSBmdW5jdGlvbiB0aGVuCj4+PiArICAgICAgKiBjaG9vc2VzIG1vZGUgYWNjb3JkaW5nIHdpdGgg S2VybmVsIGNvbmZpZ3VyYXRpb24uIFRoaXMgc2V0dGluZwo+Pj4gKyAgICAgICogY2FuIGJlIHJl c3RyaWN0ZWQgbGF0ZXIgZGVwZW5kaW5nIG9uIHN0cmFwIHBpbiBjb25maWd1cmF0aW9uLgo+Pj4g KyAgICAgICovCj4+PiArICAgICBpZiAoZHJfbW9kZSA9PSBVU0JfRFJfTU9ERV9VTktOT1dOKSB7 Cj4+PiArICAgICAgICAgICAgIGlmIChJU19FTkFCTEVEKENPTkZJR19VU0JfQ0ROUzNfSE9TVCkg JiYKPj4+ICsgICAgICAgICAgICAgICAgIElTX0VOQUJMRUQoQ09ORklHX1VTQl9DRE5TM19HQURH RVQpKQo+Pj4gKyAgICAgICAgICAgICAgICAgICAgIGRyX21vZGUgPSBVU0JfRFJfTU9ERV9PVEc7 Cj4+PiArICAgICAgICAgICAgIGVsc2UgaWYgKElTX0VOQUJMRUQoQ09ORklHX1VTQl9DRE5TM19I T1NUKSkKPj4+ICsgICAgICAgICAgICAgICAgICAgICBkcl9tb2RlID0gVVNCX0RSX01PREVfSE9T VDsKPj4+ICsgICAgICAgICAgICAgZWxzZSBpZiAoSVNfRU5BQkxFRChDT05GSUdfVVNCX0NETlMz X0dBREdFVCkpCj4+PiArICAgICAgICAgICAgICAgICAgICAgZHJfbW9kZSA9IFVTQl9EUl9NT0RF X1BFUklQSEVSQUw7Cj4+PiArICAgICB9Cj4+PiArCj4+PiArICAgICBpZiAoZHJfbW9kZSA9PSBV U0JfRFJfTU9ERV9PVEcgfHwgZHJfbW9kZSA9PSBVU0JfRFJfTU9ERV9IT1NUKSB7Cj4+PiArICAg ICAgICAgICAgIC8vVE9ETzogaW1wbGVtZW50cyBob3N0IGluaXRpYWxpemF0aW9uCj4+Cj4+ICAg ICAgICAgICAgICAgICAvKiBUT0RPOiBBZGQgaG9zdCByb2xlICovID8KPj4KPj4+ICsgICAgIH0K Pj4+ICsKPj4+ICsgICAgIGlmIChkcl9tb2RlID09IFVTQl9EUl9NT0RFX09URyB8fCBkcl9tb2Rl ID09IFVTQl9EUl9NT0RFX1BFUklQSEVSQUwpIHsKPj4+ICsgICAgICAgICAgICAgLy9UT0RPOiBp bXBsZW1lbnRzIGRldmljZSBpbml0aWFsaXphdGlvbgo+Pgo+PiAgICAgICAgICAgICAgICAgLyog VE9ETzogQWRkIGRldmljZSByb2xlICovID8KPj4KPiAKPiBZZXMsIGl0IG5lZWRzIHRvIGFsbG9j YXRlIGNkbnMtPnJvbGVzW0NETlMzX1JPTEVfSE9TVF0gYW5kCj4gY2Rucy0+cm9sZXNbQ0ROUzNf Uk9MRV9HQURHRVRdLgo+IAo+Pj4gKyAgICAgfQo+Pj4gKwo+Pj4gKyAgICAgaWYgKCFjZG5zLT5y b2xlc1tDRE5TM19ST0xFX0hPU1RdICYmICFjZG5zLT5yb2xlc1tDRE5TM19ST0xFX0dBREdFVF0p IHsKPj4+ICsgICAgICAgICAgICAgZGV2X2VycihkZXYsICJubyBzdXBwb3J0ZWQgcm9sZXNcbiIp Owo+Pj4gKyAgICAgICAgICAgICByZXR1cm4gLUVOT0RFVjsKPj4+ICsgICAgIH0KPj4+ICsKPj4+ ICsgICAgIGNkbnMtPmRyX21vZGUgPSBkcl9tb2RlOwo+IAo+IFBhd2VsLCB3aHkgZHJfbW9kZSBu ZWVkcyB0byBiZSBpbnRyb2R1Y2VkPwo+IAo+Pj4gKyAgICAgcmV0dXJuIDA7Cj4+PiArfQo+Pj4g Kwo+Pj4gKy8qKgo+Pj4gKyAqIGNkbnMzX2lycSAtIGludGVycnVwdCBoYW5kbGVyIGZvciBjZG5z MyBjb3JlIGRldmljZQo+Pj4gKyAqCj4+PiArICogQGlycTogaXJxIG51bWJlciBmb3IgY2RuczMg Y29yZSBkZXZpY2UKPj4+ICsgKiBAZGF0YTogc3RydWN0dXJlIG9mIGNkbnMzCj4+PiArICoKPj4+ ICsgKiBSZXR1cm5zIElSUV9IQU5ETEVEIG9yIElSUV9OT05FCj4+PiArICovCj4+PiArc3RhdGlj IGlycXJldHVybl90IGNkbnMzX2lycShpbnQgaXJxLCB2b2lkICpkYXRhKQo+Pj4gK3sKPj4+ICsg ICAgIHN0cnVjdCBjZG5zMyAqY2RucyA9IGRhdGE7Cj4+PiArICAgICBpcnFyZXR1cm5fdCByZXQg PSBJUlFfTk9ORTsKPj4+ICsKPj4+ICsgICAgIC8qIEhhbmRsZSBkZXZpY2UvaG9zdCBpbnRlcnJ1 cHQgKi8KPj4+ICsgICAgIGlmIChjZG5zLT5yb2xlICE9IENETlMzX1JPTEVfRU5EKQo+Pgo+PiBJ cyBpdCBiZWNhdXNlIG9mIHRoaXMgdGhhdCB5b3UgbmVlZCB0byBzZXQgcm9sZSB0byBFTkQgYXQg cm9sZV9zdG9wPwo+PiBJIHRoaW5rIGl0IGlzIGJldHRlciB0byBhZGQgYSBzdGF0ZSB2YXJpYWJs ZSB0byBzdHJ1Y3QgY2RuczNfcm9sZV9kcml2ZXIsIHNvIHdlIGNhbgo+PiBjaGVjayBpZiBpdCBp cyBhY3RpdmUgb3Igc3RvcHBlZC4KPj4KPj4gZS5nLgo+PiAgICAgICAgIGlmIChjZG5zM19nZXRf Y3VycmVudF9yb2xlX2RyaXZlcihjZG5zKS0+c3RhdGUgPT0gQ0ROUzNfUk9MRV9TVEFURV9BQ1RJ VkUpCj4+Cj4+PiArICAgICAgICAgICAgIHJldCA9IGNkbnMzX2dldF9jdXJyZW50X3JvbGVfZHJp dmVyKGNkbnMpLT5pcnEoY2Rucyk7Cj4+PiArCj4+PiArICAgICByZXR1cm4gcmV0Owo+Pj4gK30K Pj4+ICsKPiAKPiAgQ0ROUzNfUk9MRV9FTkQgaXMgaW50cm9kdWNlZCBmcm9tIGFib3ZlIGNvbW1l bnRzLCB3ZSBkb24ndAo+IG5lZWQgYW5vdGhlciBmbGFnIGZvciBpdC4KPiBJZiBjZG5zLT5yb2xl ID09IENETlMzX1JPTEVfRU5ELCBpdCBoYW5kbGVzIFZCVVMgYW5kIElEIGludGVycnVwdC4KPiAK Pj4+ICtzdGF0aWMgdm9pZCBjZG5zM19yZW1vdmVfcm9sZXMoc3RydWN0IGNkbnMzICpjZG5zKQo+ Pgo+PiBTaG91bGQgdGhpcyBiZSBjYWxsZWQgY2RuczNfZXhpdF9yb2xlcygpIHRvIGJlIG9wcG9z aXRlIG9mIGNkbnMzX2luaXRfcm9sZXMoKT8KPj4KPiAKPiBJdCBpcyBwbGFuZWQgdG8gY2FsbGVk IHdoZW4gYXQgLT5yZW1vdmUuCj4+PiArewo+Pj4gKyAgICAgLy9UT0RPOiBpbXBsZW1lbnRzIHRo aXMgZnVuY3Rpb24KPj4+ICt9Cj4+Cj4+PiArCj4+PiArc3RhdGljIGludCBjZG5zM19kb19yb2xl X3N3aXRjaChzdHJ1Y3QgY2RuczMgKmNkbnMsIGVudW0gY2RuczNfcm9sZXMgcm9sZSkKPj4+ICt7 Cj4+PiArICAgICBlbnVtIGNkbnMzX3JvbGVzIGN1cnJlbnRfcm9sZTsKPj4+ICsgICAgIGludCBy ZXQgPSAwOwo+Pj4gKwo+Pj4gKyAgICAgY3VycmVudF9yb2xlID0gY2Rucy0+cm9sZTsKPj4+ICsK Pj4+ICsgICAgIGlmIChyb2xlID09IENETlMzX1JPTEVfRU5EKQo+Pj4gKyAgICAgICAgICAgICBy ZXR1cm4gMDsKPj4KPj4gcm9sZSA9PSBFTkQgbG9va3MgbGlrZSBlcnJvciBzdGF0ZS4gYW5kIGl0 IHNob3VsZCBuZXZlciBoYXBwZW4uCj4+IFdBUk4gaGVyZT8KPj4KPiAKPiBTZWUgbXkgY29tbWVu dHMgYWJvdmUuCj4gCj4+PiArCj4+PiArICAgICBkZXZfZGJnKGNkbnMtPmRldiwgIlN3aXRjaGlu ZyByb2xlIik7Cj4+PiArCj4+Cj4+IERvbid0IHlvdSBoYXZlIHRvIHN0b3AgdGhlIHByZXZpb3Vz IHJvbGUgYmVmb3JlIHN0YXJ0aW5nIHRoZSBuZXcgcm9sZT8KPj4KPiAKPiBZZXMsIGl0IGlzIG5l ZWRlZC4gUGF3ZWwgbWF5IHNpbXBseSBzb21lIGZsb3dzIHRvIHN1aXQgaGlzIHBsYXRmb3JtLgo+ IAo+Pj4gKyAgICAgcmV0ID0gY2RuczNfcm9sZV9zdGFydChjZG5zLCByb2xlKTsKPj4+ICsgICAg IGlmIChyZXQpIHsKPj4+ICsgICAgICAgICAgICAgLyogQmFjayB0byBjdXJyZW50IHJvbGUgKi8K Pj4+ICsgICAgICAgICAgICAgZGV2X2VycihjZG5zLT5kZXYsICJzZXQgJWQgaGFzIGZhaWxlZCwg YmFjayB0byAlZFxuIiwKPj4+ICsgICAgICAgICAgICAgICAgICAgICByb2xlLCBjdXJyZW50X3Jv bGUpOwo+Pj4gKyAgICAgICAgICAgICByZXQgPSBjZG5zM19yb2xlX3N0YXJ0KGNkbnMsIGN1cnJl bnRfcm9sZSk7Cj4+PiArICAgICB9Cj4+PiArCj4+PiArICAgICByZXR1cm4gcmV0Owo+Pj4gK30K Pj4+ICsKPj4+ICsvKioKPj4+ICsgKiBjZG5zM19yb2xlX3N3aXRjaCAtIHdvcmsgcXVldWUgaGFu ZGxlciBmb3Igcm9sZSBzd2l0Y2gKPj4+ICsgKgo+Pj4gKyAqIEB3b3JrOiB3b3JrIHF1ZXVlIGl0 ZW0gc3RydWN0dXJlCj4+PiArICoKPj4+ICsgKiBIYW5kbGVzIGJlbG93IGV2ZW50czoKPj4+ICsg KiAtIFJvbGUgc3dpdGNoIGZvciBkdWFsLXJvbGUgZGV2aWNlcwo+Pj4gKyAqIC0gQ0ROUzNfUk9M RV9HQURHRVQgPC0tPiBDRE5TM19ST0xFX0VORCBmb3IgcGVyaXBoZXJhbC1vbmx5IGRldmljZXMK Pj4+ICsgKi8KPj4+ICtzdGF0aWMgdm9pZCBjZG5zM19yb2xlX3N3aXRjaChzdHJ1Y3Qgd29ya19z dHJ1Y3QgKndvcmspCj4+PiArewo+Pj4gKyAgICAgZW51bSBjZG5zM19yb2xlcyByb2xlID0gQ0RO UzNfUk9MRV9FTkQ7Cj4+PiArICAgICBzdHJ1Y3QgY2RuczMgKmNkbnM7Cj4+PiArICAgICBib29s IGRldmljZSwgaG9zdDsKPj4+ICsKPj4+ICsgICAgIGNkbnMgPSBjb250YWluZXJfb2Yod29yaywg c3RydWN0IGNkbnMzLCByb2xlX3N3aXRjaF93cSk7Cj4+PiArCj4+PiArICAgICAvL1RPRE86IGlt cGxlbWVudHMgdGhpcyBmdW5jdGlvbnMuCj4+PiArICAgICAvL2hvc3QgPSBjZG5zM19pc19ob3N0 KGNkbnMpOwo+Pj4gKyAgICAgLy9kZXZpY2UgPSBjZG5zM19pc19kZXZpY2UoY2Rucyk7Cj4+PiAr ICAgICBob3N0ID0gMTsKPj4+ICsgICAgIGRldmljZSA9IDA7Cj4+PiArCj4+PiArICAgICBpZiAo aG9zdCkKPj4+ICsgICAgICAgICAgICAgcm9sZSA9IENETlMzX1JPTEVfSE9TVDsKPj4+ICsgICAg IGVsc2UgaWYgKGRldmljZSkKPj4+ICsgICAgICAgICAgICAgcm9sZSA9IENETlMzX1JPTEVfR0FE R0VUOwo+Pj4gKwo+Pj4gKyAgICAgaWYgKGNkbnMtPmRlc2lyZWRfZHJfbW9kZSA9PSBjZG5zLT5j dXJyZW50X2RyX21vZGUgJiYKPj4+ICsgICAgICAgICBjZG5zLT5yb2xlID09IHJvbGUpCj4+PiAr ICAgICAgICAgICAgIHJldHVybjsKPj4+ICsKPj4KPj4gSSB0aGluayBhbGwgdGhlIGJlbG93IGNv ZGUgY2FuIGJlIG1vdmVkIHRvIGNkbnMzX2RvX3JvbGVfc3dpdGNoKCkuCj4+Cj4+PiArICAgICBw bV9ydW50aW1lX2dldF9zeW5jKGNkbnMtPmRldik7Cj4+PiArICAgICBjZG5zM19yb2xlX3N0b3Ao Y2Rucyk7Cj4+PiArCj4+PiArICAgICBpZiAoaG9zdCkgewo+Pj4gKyAgICAgICAgICAgICBpZiAo Y2Rucy0+cm9sZXNbQ0ROUzNfUk9MRV9IT1NUXSkKPj4+ICsgICAgICAgICAgICAgICAgICAgICBj ZG5zM19kb19yb2xlX3N3aXRjaChjZG5zLCBDRE5TM19ST0xFX0hPU1QpOwo+Pj4gKyAgICAgICAg ICAgICBwbV9ydW50aW1lX3B1dF9zeW5jKGNkbnMtPmRldik7Cj4+PiArICAgICAgICAgICAgIHJl dHVybjsKPj4+ICsgICAgIH0KPj4+ICsKPj4+ICsgICAgIGlmIChkZXZpY2UpCj4+PiArICAgICAg ICAgICAgIGNkbnMzX2RvX3JvbGVfc3dpdGNoKGNkbnMsIENETlMzX1JPTEVfR0FER0VUKTsKPj4+ ICsgICAgIGVsc2UKPj4+ICsgICAgICAgICAgICAgY2RuczNfZG9fcm9sZV9zd2l0Y2goY2Rucywg Q0ROUzNfUk9MRV9FTkQpOwo+Pj4gKwo+Pj4gKyAgICAgcG1fcnVudGltZV9wdXRfc3luYyhjZG5z LT5kZXYpOwo+Pj4gK30KPj4+ICsKPj4+ICsvKioKPj4+ICsgKiBjZG5zM19wcm9iZSAtIHByb2Jl IGZvciBjZG5zMyBjb3JlIGRldmljZQo+Pj4gKyAqIEBwZGV2OiBQb2ludGVyIHRvIGNkbnMzIGNv cmUgcGxhdGZvcm0gZGV2aWNlCj4+PiArICoKPj4+ICsgKiBSZXR1cm5zIDAgb24gc3VjY2VzcyBv dGhlcndpc2UgbmVnYXRpdmUgZXJybm8KPj4+ICsgKi8KPj4+ICtzdGF0aWMgaW50IGNkbnMzX3By b2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYpCj4+PiArewo+Pj4gKyAgICAgc3RydWN0 IGRldmljZSAqZGV2ID0gJnBkZXYtPmRldjsKPj4+ICsgICAgIHN0cnVjdCByZXNvdXJjZSAqcmVz Owo+Pj4gKyAgICAgc3RydWN0IGNkbnMzICpjZG5zOwo+Pj4gKyAgICAgdm9pZCBfX2lvbWVtICpy ZWdzOwo+Pj4gKyAgICAgaW50IHJldDsKPj4+ICsKPj4+ICsgICAgIGNkbnMgPSBkZXZtX2t6YWxs b2MoZGV2LCBzaXplb2YoKmNkbnMpLCBHRlBfS0VSTkVMKTsKPj4+ICsgICAgIGlmICghY2RucykK Pj4+ICsgICAgICAgICAgICAgcmV0dXJuIC1FTk9NRU07Cj4+PiArCj4+PiArICAgICBjZG5zLT5k ZXYgPSBkZXY7Cj4+PiArCj4+PiArICAgICBwbGF0Zm9ybV9zZXRfZHJ2ZGF0YShwZGV2LCBjZG5z KTsKPj4+ICsKPj4+ICsgICAgIHJlcyA9IHBsYXRmb3JtX2dldF9yZXNvdXJjZShwZGV2LCBJT1JF U09VUkNFX0lSUSwgMCk7Cj4+PiArICAgICBpZiAoIXJlcykgewo+Pj4gKyAgICAgICAgICAgICBk ZXZfZXJyKGRldiwgIm1pc3NpbmcgSVJRXG4iKTsKPj4+ICsgICAgICAgICAgICAgcmV0dXJuIC1F Tk9ERVY7Cj4+PiArICAgICB9Cj4+PiArICAgICBjZG5zLT5pcnEgPSByZXMtPnN0YXJ0Owo+Pj4g Kwo+Pj4gKyAgICAgLyoKPj4+ICsgICAgICAqIFJlcXVlc3QgbWVtb3J5IHJlZ2lvbgo+Pj4gKyAg ICAgICogcmVnaW9uLTA6IHhIQ0kKPj4+ICsgICAgICAqIHJlZ2lvbi0xOiBQZXJpcGhlcmFsCj4+ PiArICAgICAgKiByZWdpb24tMjogT1RHIHJlZ2lzdGVycwo+Pj4gKyAgICAgICovCj4+Cj4+IFRo ZSBtZW1vcnkgcmVnaW9uIG9yZGVyIGlzIGRpZmZlcmVudCBmcm9tIHRoZSBkdC1iaW5kaW5nLgo+ PiBUaGVyZSBpdCBpcyBPVEcsIGhvc3QoeGhjaSksIGRldmljZSAocGVyaXBoZXJhbCkuCj4+Cj4+ PiArICAgICByZXMgPSBwbGF0Zm9ybV9nZXRfcmVzb3VyY2UocGRldiwgSU9SRVNPVVJDRV9NRU0s IDApOwo+Pj4gKyAgICAgcmVncyA9IGRldm1faW9yZW1hcF9yZXNvdXJjZShkZXYsIHJlcyk7Cj4+ PiArCj4+PiArICAgICBpZiAoSVNfRVJSKHJlZ3MpKQo+Pj4gKyAgICAgICAgICAgICByZXR1cm4g UFRSX0VSUihyZWdzKTsKPj4+ICsgICAgIGNkbnMtPnhoY2lfcmVncyA9IHJlZ3M7Cj4+PiArICAg ICBjZG5zLT54aGNpX3JlcyA9IHJlczsKPj4+ICsKPj4+ICsgICAgIHJlcyA9IHBsYXRmb3JtX2dl dF9yZXNvdXJjZShwZGV2LCBJT1JFU09VUkNFX01FTSwgMSk7Cj4+PiArICAgICByZWdzID0gZGV2 bV9pb3JlbWFwX3Jlc291cmNlKGRldiwgcmVzKTsKPj4+ICsgICAgIGlmIChJU19FUlIocmVncykp Cj4+PiArICAgICAgICAgICAgIHJldHVybiBQVFJfRVJSKHJlZ3MpOwo+Pj4gKyAgICAgY2Rucy0+ ZGV2X3JlZ3MgID0gcmVnczsKPj4+ICsKPj4+ICsgICAgIHJlcyA9IHBsYXRmb3JtX2dldF9yZXNv dXJjZShwZGV2LCBJT1JFU09VUkNFX01FTSwgMik7Cj4+PiArICAgICByZWdzID0gZGV2bV9pb3Jl bWFwX3Jlc291cmNlKGRldiwgcmVzKTsKPj4+ICsgICAgIGlmIChJU19FUlIocmVncykpCj4+PiAr ICAgICAgICAgICAgIHJldHVybiBQVFJfRVJSKHJlZ3MpOwo+Pj4gKyAgICAgY2Rucy0+b3RnX3Jl Z3MgPSByZWdzOwo+Pj4gKwo+Pj4gKyAgICAgbXV0ZXhfaW5pdCgmY2Rucy0+bXV0ZXgpOwo+Pj4g Kwo+Pj4gKyAgICAgY2Rucy0+cGh5ID0gZGV2bV9waHlfZ2V0KGRldiwgImNkbnMzLHVzYnBoeSIp Owo+Pgo+PiAiY2RuczMsdXNicGh5IiBpcyBub3QgZG9jdW1lbnRlZCBpbiBkdC1iaW5kaW5nLgo+ Pgo+Pj4gKyAgICAgaWYgKElTX0VSUihjZG5zLT5waHkpKSB7Cj4+PiArICAgICAgICAgICAgIGRl dl9pbmZvKGRldiwgIm5vIGdlbmVyaWMgcGh5IGZvdW5kXG4iKTsKPj4+ICsgICAgICAgICAgICAg Y2Rucy0+cGh5ID0gTlVMTDsKPj4+ICsgICAgICAgICAgICAgLyoKPj4+ICsgICAgICAgICAgICAg ICogZmFsbCB0aHJvdWdoIGhlcmUhCj4+PiArICAgICAgICAgICAgICAqIGlmIG5vIGdlbmVyaWMg cGh5IGZvdW5kLCBwaHkgaW5pdAo+Pj4gKyAgICAgICAgICAgICAgKiBzaG91bGQgYmUgZG9uZSB1 bmRlciBib290IQo+Pj4gKyAgICAgICAgICAgICAgKi8KPj4KPj4gTm8geW91IHNob3VsZG4ndCBm YWxsIHRocm91Z2ggYWx3YXlzIGlmIGl0IGlzIGFuIGVycm9yIGNvbmRpdGlvbi4KPj4gU29tZXRo aW5nIGxpa2UgdGhpcyBzaG91bGQgd29yayBiZXR0ZXIuCj4+Cj4+ICAgICAgICAgaWYgKElTX0VS UihjbmRzLT5waHkpKSB7Cj4+ICAgICAgICAgICAgICAgICByZXQgPSBQVFJfRVJSKGNkbnMtPnBo eSk7Cj4+ICAgICAgICAgICAgICAgICBpZiAocmV0ID09IC1FTk9TWVMgfHwgcmV0ID09IC1FTk9E RVYpIHsKPj4gICAgICAgICAgICAgICAgICAgICAgICAgY2Rucy0+cGh5ID0gTlVMTDsKPj4gICAg ICAgICAgICAgICAgIH0gZWxzZSBpZiAocmV0ID09IC1FUFJPQkVfREVGRVIpIHsKPj4gICAgICAg ICAgICAgICAgICAgICAgICAgcmV0dXJuIHJldDsKPj4gICAgICAgICAgICAgICAgIH0gZWxzZSB7 Cj4+ICAgICAgICAgICAgICAgICAgICAgICAgIGRldl9lcnIoZGV2LCAibm8gcGh5IGZvdW5kXG4i KTsKPj4gICAgICAgICAgICAgICAgICAgICAgICAgZ290byBlcnIwOwo+PiAgICAgICAgICAgICAg ICAgfQo+PiAgICAgICAgIH0KPj4KPj4gU28gaWYgUEhZIHdhcyBwcm92aWRlZCBpbiBEVCwgYW5k IFBIWSBzdXBwb3J0L2RyaXZlcnMgaXMgcHJlc2VudAo+PiBhbmQgZXJyb3IgY29uZGl0aW9uIG1l YW5zIHNvbWV0aGluZyBpcyB3cm9uZyBhbmQgd2UgaGF2ZSB0byBlcnJvciBvdXQuCj4+Cj4+PiAr ICAgICB9IGVsc2Ugewo+Pj4gKyAgICAgICAgICAgICBwaHlfaW5pdChjZG5zLT5waHkpOwo+Pj4g KyAgICAgfQo+Pgo+PiBZb3UgY2FuIGRvIHBoeV9pbml0KCkgb3V0c2lkZSB0aGUgZWxzZS4KPj4K Pj4+ICsKPj4+ICsgICAgIHJldCA9IGNkbnMzX2NvcmVfaW5pdF9yb2xlKGNkbnMpOwo+Pj4gKyAg ICAgaWYgKHJldCkKPj4+ICsgICAgICAgICAgICAgZ290byBlcnIxOwo+Pj4gKwo+Pj4gKyAgICAg SU5JVF9XT1JLKCZjZG5zLT5yb2xlX3N3aXRjaF93cSwgY2RuczNfcm9sZV9zd2l0Y2gpOwo+Pj4g KyAgICAgaWYgKHJldCkKPj4+ICsgICAgICAgICAgICAgZ290byBlcnIyOwo+Pj4gKwo+Pj4gKyAg ICAgaWYgKHJldCkKPj4+ICsgICAgICAgICAgICAgZ290byBlcnIyOwo+Pj4gKwo+Pj4gKyAgICAg Y2Rucy0+cm9sZSA9IGNkbnMzX2dldF9yb2xlKGNkbnMpOwo+Pgo+PiBJIHRoaW5rIHRoaXMgc2hv dWxkIG1vdmUgdG8gY2RuczNfY29yZV9pbml0X3JvbGUoKS4KPj4KPiAKPiBJIGFncmVlLgo+IAo+ Pj4gKwo+Pj4gKyAgICAgcmV0ID0gZGV2bV9yZXF1ZXN0X2lycShkZXYsIGNkbnMtPmlycSwgY2Ru czNfaXJxLCBJUlFGX1NIQVJFRCwKPj4+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgZGV2 X25hbWUoZGV2KSwgY2Rucyk7Cj4+PiArCj4+PiArICAgICBpZiAocmV0KQo+Pj4gKyAgICAgICAg ICAgICBnb3RvIGVycjI7Cj4+Cj4+IEhvdyBhYm91dCBtb3ZpbmcgcmVxdWVzdF9pcnEgdG8gYmVm b3JlIGNkc24zX2NvcmVfaW5pdF9yb2xlKCk/Cj4+Cj4+IFRoZW4geW91IGNhbiBtb3ZlIGNkbnMz X3JvbGVfc3RhcnQoKSBhcyB3ZWxsIHRvIGNvcmVfaW5pdF9yb2xlKCkuCj4+Cj4gCj4gVXN1YWxs eSwgd2UgcmVxdWVzdCBpcnEgYWZ0ZXIgaGFyZHdhcmUgaW5pdGlhbGl6YXRpb24gaGFzIGZpbmlz aGVkLCBpZiBub3QsCj4gdGhlcmUgbWF5IHVuZXhwZWN0ZWQgaW50ZXJydXB0LgoKRG9lc24ndCBr ZXJuZWwgd2FybiBpZiBpbnRlcnJ1cHQgaGFwcGVucyBhbmQgdGhlcmUgaXMgbm8gaGFuZGxlcj8K VG8gYXZvaWQgdGhhdCBJIHdhcyBzdWdnZXN0aW5nIHRvIHJlcXVlc3RfaXJxIGZpcnN0LgoKY2hl ZXJzLAotcm9nZXIK From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Quadros Subject: Re: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code. Date: Tue, 4 Dec 2018 12:46:06 +0200 Message-ID: <5C065AEE.4010205@ti.com> References: <1542535751-16079-1-git-send-email-pawell@cadence.com> <1542535751-16079-5-git-send-email-pawell@cadence.com> <5BF7E5E8.3090406@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Peter Chen Cc: pawell@cadence.com, devicetree@vger.kernel.org, Greg Kroah-Hartman , linux-usb@vger.kernel.org, lkml , adouglas@cadence.com, jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com, pjez@cadence.com, kurahul@cadence.com List-Id: devicetree@vger.kernel.org On 04/12/18 10:50, Peter Chen wrote: >>> + * Cadence USBSS DRD Driver. >>> + * >>> + * Copyright (C) 2018 Cadence. >>> + * >>> + * Author: Peter Chen >>> + * Pawel Laszczak >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "gadget.h" >>> +#include "core.h" >>> + >>> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns) >>> +{ >>> + WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]); >>> + return cdns->roles[cdns->role]; >>> +} >>> + >>> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role) >>> +{ >>> + int ret; >>> + >>> + if (role >= CDNS3_ROLE_END) >> >> WARN_ON()? >> >>> + return 0; >>> + >>> + if (!cdns->roles[role]) >>> + return -ENXIO; >>> + >>> + mutex_lock(&cdns->mutex); >>> + cdns->role = role; >>> + ret = cdns->roles[role]->start(cdns); >>> + mutex_unlock(&cdns->mutex); >>> + return ret; >>> +} >>> + >>> +static inline void cdns3_role_stop(struct cdns3 *cdns) >>> +{ >>> + enum cdns3_roles role = cdns->role; >>> + >>> + if (role == CDNS3_ROLE_END) >> >> WARN_ON(role >= CNDS3_ROLE_END) ? >> >>> + return; >>> + >>> + mutex_lock(&cdns->mutex); >>> + cdns->roles[role]->stop(cdns); >>> + cdns->role = CDNS3_ROLE_END; >> >> Why change the role here? You are just stopping the role not changing it. >> I think cdns->role should remain unchanged, so we can call cdns3_role_start() >> if required without error. >> > > The current version of this IP has some issues to detect vbus status correctly, > we have to force vbus status accordingly, so we need a status to indicate > vbus disconnection, and add some code to let controller know vbus > removal, in that case, the controller's state machine can be correct. > So, we increase one role 'CDNS3_ROLE_END' to for this purpose. > > CDNS3_ROLE_GADGET: gadget mode and VBUS on > CDNS3_ROLE_HOST: host mode and VBUS on > CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port. > > So, we may start role from CDNS3_ROLE_END at probe when nothing is connected, > and need to set role as CDNS3_ROLE_END at ->stop for further handling at > role switch routine. OK. but still this (changing to ROLE_END) must be moved to the role switch routine and the explanation you just mentioned must be added there. > >>> + mutex_unlock(&cdns->mutex); >>> +} >>> + >>> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns) >>> +{ >>> + if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) { >>> + //TODO: implements selecting device/host mode >>> + return CDNS3_ROLE_HOST; >>> + } >>> + return cdns->roles[CDNS3_ROLE_HOST] >>> + ? CDNS3_ROLE_HOST >>> + : CDNS3_ROLE_GADGET; >> >> Why not just >> return cdns->role; >> >> I'm wondering if we really need this function. > > cdns->role gets from cdns3_get_role, and this API tells role at the runtime. > If both roles are supported, the role is decided by external > conditions, eg, vbus/id > or external connector. If only single role is supported, only one role structure > is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET] > How about adding this description in function documentation. >>> +} >> >>> + >>> +/** >>> + * cdns3_core_init_role - initialize role of operation >>> + * @cdns: Pointer to cdns3 structure >>> + * >>> + * Returns 0 on success otherwise negative errno >>> + */ >>> +static int cdns3_core_init_role(struct cdns3 *cdns) >>> +{ >>> + struct device *dev = cdns->dev; >>> + enum usb_dr_mode dr_mode; >>> + >>> + dr_mode = usb_get_dr_mode(dev); >>> + cdns->role = CDNS3_ROLE_END; >>> + >>> + /* >>> + * If driver can't read mode by means of usb_get_dr_mdoe function then >>> + * chooses mode according with Kernel configuration. This setting >>> + * can be restricted later depending on strap pin configuration. >>> + */ >>> + if (dr_mode == USB_DR_MODE_UNKNOWN) { >>> + if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) && >>> + IS_ENABLED(CONFIG_USB_CDNS3_GADGET)) >>> + dr_mode = USB_DR_MODE_OTG; >>> + else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST)) >>> + dr_mode = USB_DR_MODE_HOST; >>> + else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET)) >>> + dr_mode = USB_DR_MODE_PERIPHERAL; >>> + } >>> + >>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { >>> + //TODO: implements host initialization >> >> /* TODO: Add host role */ ? >> >>> + } >>> + >>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) { >>> + //TODO: implements device initialization >> >> /* TODO: Add device role */ ? >> > > Yes, it needs to allocate cdns->roles[CDNS3_ROLE_HOST] and > cdns->roles[CDNS3_ROLE_GADGET]. > >>> + } >>> + >>> + if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) { >>> + dev_err(dev, "no supported roles\n"); >>> + return -ENODEV; >>> + } >>> + >>> + cdns->dr_mode = dr_mode; > > Pawel, why dr_mode needs to be introduced? > >>> + return 0; >>> +} >>> + >>> +/** >>> + * cdns3_irq - interrupt handler for cdns3 core device >>> + * >>> + * @irq: irq number for cdns3 core device >>> + * @data: structure of cdns3 >>> + * >>> + * Returns IRQ_HANDLED or IRQ_NONE >>> + */ >>> +static irqreturn_t cdns3_irq(int irq, void *data) >>> +{ >>> + struct cdns3 *cdns = data; >>> + irqreturn_t ret = IRQ_NONE; >>> + >>> + /* Handle device/host interrupt */ >>> + if (cdns->role != CDNS3_ROLE_END) >> >> Is it because of this that you need to set role to END at role_stop? >> I think it is better to add a state variable to struct cdns3_role_driver, so we can >> check if it is active or stopped. >> >> e.g. >> if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE) >> >>> + ret = cdns3_get_current_role_driver(cdns)->irq(cdns); >>> + >>> + return ret; >>> +} >>> + > > CDNS3_ROLE_END is introduced from above comments, we don't > need another flag for it. > If cdns->role == CDNS3_ROLE_END, it handles VBUS and ID interrupt. > >>> +static void cdns3_remove_roles(struct cdns3 *cdns) >> >> Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()? >> > > It is planed to called when at ->remove. >>> +{ >>> + //TODO: implements this function >>> +} >> >>> + >>> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role) >>> +{ >>> + enum cdns3_roles current_role; >>> + int ret = 0; >>> + >>> + current_role = cdns->role; >>> + >>> + if (role == CDNS3_ROLE_END) >>> + return 0; >> >> role == END looks like error state. and it should never happen. >> WARN here? >> > > See my comments above. > >>> + >>> + dev_dbg(cdns->dev, "Switching role"); >>> + >> >> Don't you have to stop the previous role before starting the new role? >> > > Yes, it is needed. Pawel may simply some flows to suit his platform. > >>> + ret = cdns3_role_start(cdns, role); >>> + if (ret) { >>> + /* Back to current role */ >>> + dev_err(cdns->dev, "set %d has failed, back to %d\n", >>> + role, current_role); >>> + ret = cdns3_role_start(cdns, current_role); >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +/** >>> + * cdns3_role_switch - work queue handler for role switch >>> + * >>> + * @work: work queue item structure >>> + * >>> + * Handles below events: >>> + * - Role switch for dual-role devices >>> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices >>> + */ >>> +static void cdns3_role_switch(struct work_struct *work) >>> +{ >>> + enum cdns3_roles role = CDNS3_ROLE_END; >>> + struct cdns3 *cdns; >>> + bool device, host; >>> + >>> + cdns = container_of(work, struct cdns3, role_switch_wq); >>> + >>> + //TODO: implements this functions. >>> + //host = cdns3_is_host(cdns); >>> + //device = cdns3_is_device(cdns); >>> + host = 1; >>> + device = 0; >>> + >>> + if (host) >>> + role = CDNS3_ROLE_HOST; >>> + else if (device) >>> + role = CDNS3_ROLE_GADGET; >>> + >>> + if (cdns->desired_dr_mode == cdns->current_dr_mode && >>> + cdns->role == role) >>> + return; >>> + >> >> I think all the below code can be moved to cdns3_do_role_switch(). >> >>> + pm_runtime_get_sync(cdns->dev); >>> + cdns3_role_stop(cdns); >>> + >>> + if (host) { >>> + if (cdns->roles[CDNS3_ROLE_HOST]) >>> + cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST); >>> + pm_runtime_put_sync(cdns->dev); >>> + return; >>> + } >>> + >>> + if (device) >>> + cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET); >>> + else >>> + cdns3_do_role_switch(cdns, CDNS3_ROLE_END); >>> + >>> + pm_runtime_put_sync(cdns->dev); >>> +} >>> + >>> +/** >>> + * cdns3_probe - probe for cdns3 core device >>> + * @pdev: Pointer to cdns3 core platform device >>> + * >>> + * Returns 0 on success otherwise negative errno >>> + */ >>> +static int cdns3_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct resource *res; >>> + struct cdns3 *cdns; >>> + void __iomem *regs; >>> + int ret; >>> + >>> + cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL); >>> + if (!cdns) >>> + return -ENOMEM; >>> + >>> + cdns->dev = dev; >>> + >>> + platform_set_drvdata(pdev, cdns); >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>> + if (!res) { >>> + dev_err(dev, "missing IRQ\n"); >>> + return -ENODEV; >>> + } >>> + cdns->irq = res->start; >>> + >>> + /* >>> + * Request memory region >>> + * region-0: xHCI >>> + * region-1: Peripheral >>> + * region-2: OTG registers >>> + */ >> >> The memory region order is different from the dt-binding. >> There it is OTG, host(xhci), device (peripheral). >> >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + regs = devm_ioremap_resource(dev, res); >>> + >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + cdns->xhci_regs = regs; >>> + cdns->xhci_res = res; >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >>> + regs = devm_ioremap_resource(dev, res); >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + cdns->dev_regs = regs; >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); >>> + regs = devm_ioremap_resource(dev, res); >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + cdns->otg_regs = regs; >>> + >>> + mutex_init(&cdns->mutex); >>> + >>> + cdns->phy = devm_phy_get(dev, "cdns3,usbphy"); >> >> "cdns3,usbphy" is not documented in dt-binding. >> >>> + if (IS_ERR(cdns->phy)) { >>> + dev_info(dev, "no generic phy found\n"); >>> + cdns->phy = NULL; >>> + /* >>> + * fall through here! >>> + * if no generic phy found, phy init >>> + * should be done under boot! >>> + */ >> >> No you shouldn't fall through always if it is an error condition. >> Something like this should work better. >> >> if (IS_ERR(cnds->phy)) { >> ret = PTR_ERR(cdns->phy); >> if (ret == -ENOSYS || ret == -ENODEV) { >> cdns->phy = NULL; >> } else if (ret == -EPROBE_DEFER) { >> return ret; >> } else { >> dev_err(dev, "no phy found\n"); >> goto err0; >> } >> } >> >> So if PHY was provided in DT, and PHY support/drivers is present >> and error condition means something is wrong and we have to error out. >> >>> + } else { >>> + phy_init(cdns->phy); >>> + } >> >> You can do phy_init() outside the else. >> >>> + >>> + ret = cdns3_core_init_role(cdns); >>> + if (ret) >>> + goto err1; >>> + >>> + INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch); >>> + if (ret) >>> + goto err2; >>> + >>> + if (ret) >>> + goto err2; >>> + >>> + cdns->role = cdns3_get_role(cdns); >> >> I think this should move to cdns3_core_init_role(). >> > > I agree. > >>> + >>> + ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED, >>> + dev_name(dev), cdns); >>> + >>> + if (ret) >>> + goto err2; >> >> How about moving request_irq to before cdsn3_core_init_role()? >> >> Then you can move cdns3_role_start() as well to core_init_role(). >> > > Usually, we request irq after hardware initialization has finished, if not, > there may unexpected interrupt. Doesn't kernel warn if interrupt happens and there is no handler? To avoid that I was suggesting to request_irq first. cheers, -roger -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F3D51C04EB8 for ; Tue, 4 Dec 2018 10:46:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A38372087F for ; Tue, 4 Dec 2018 10:46:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b="ocdt+uBN" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A38372087F Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=ti.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726231AbeLDKqT (ORCPT ); Tue, 4 Dec 2018 05:46:19 -0500 Received: from lelv0143.ext.ti.com ([198.47.23.248]:49656 "EHLO lelv0143.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725764AbeLDKqS (ORCPT ); Tue, 4 Dec 2018 05:46:18 -0500 Received: from fllv0035.itg.ti.com ([10.64.41.0]) by lelv0143.ext.ti.com (8.15.2/8.15.2) with ESMTP id wB4AkCw8028646; Tue, 4 Dec 2018 04:46:12 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1543920372; bh=R1yKLBmdJys6hh7yvq8gRSTq+QAAxvCGv1gm9K0j4wc=; h=Subject:To:References:CC:From:Date:In-Reply-To; b=ocdt+uBNeLdoeIOAllzY4rB2fgpKX77gQB/UCc60g6kUjkNnvGxc9Z7YUPEq0D2wC Six+TammpY15nXyRA9RwVYDtFWoBbf1S/kMOQGeC8A7Nyg4j0C+ibjeBVbp9I+3qr/ uZl/Stjdy2oDKXw7xHmnnKAv9byK4QACTADHpeY4= Received: from DFLE107.ent.ti.com (dfle107.ent.ti.com [10.64.6.28]) by fllv0035.itg.ti.com (8.15.2/8.15.2) with ESMTPS id wB4AkCGI120835 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 4 Dec 2018 04:46:12 -0600 Received: from DFLE110.ent.ti.com (10.64.6.31) by DFLE107.ent.ti.com (10.64.6.28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1591.10; Tue, 4 Dec 2018 04:46:10 -0600 Received: from dlep32.itg.ti.com (157.170.170.100) by DFLE110.ent.ti.com (10.64.6.31) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_RSA_WITH_AES_256_CBC_SHA) id 15.1.1591.10 via Frontend Transport; Tue, 4 Dec 2018 04:46:10 -0600 Received: from [192.168.2.6] (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep32.itg.ti.com (8.14.3/8.13.8) with ESMTP id wB4Ak73w009716; Tue, 4 Dec 2018 04:46:07 -0600 Subject: Re: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code. To: Peter Chen References: <1542535751-16079-1-git-send-email-pawell@cadence.com> <1542535751-16079-5-git-send-email-pawell@cadence.com> <5BF7E5E8.3090406@ti.com> CC: , , Greg Kroah-Hartman , , lkml , , , , , , , , From: Roger Quadros Message-ID: <5C065AEE.4010205@ti.com> Date: Tue, 4 Dec 2018 12:46:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/12/18 10:50, Peter Chen wrote: >>> + * Cadence USBSS DRD Driver. >>> + * >>> + * Copyright (C) 2018 Cadence. >>> + * >>> + * Author: Peter Chen >>> + * Pawel Laszczak >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "gadget.h" >>> +#include "core.h" >>> + >>> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns) >>> +{ >>> + WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]); >>> + return cdns->roles[cdns->role]; >>> +} >>> + >>> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role) >>> +{ >>> + int ret; >>> + >>> + if (role >= CDNS3_ROLE_END) >> >> WARN_ON()? >> >>> + return 0; >>> + >>> + if (!cdns->roles[role]) >>> + return -ENXIO; >>> + >>> + mutex_lock(&cdns->mutex); >>> + cdns->role = role; >>> + ret = cdns->roles[role]->start(cdns); >>> + mutex_unlock(&cdns->mutex); >>> + return ret; >>> +} >>> + >>> +static inline void cdns3_role_stop(struct cdns3 *cdns) >>> +{ >>> + enum cdns3_roles role = cdns->role; >>> + >>> + if (role == CDNS3_ROLE_END) >> >> WARN_ON(role >= CNDS3_ROLE_END) ? >> >>> + return; >>> + >>> + mutex_lock(&cdns->mutex); >>> + cdns->roles[role]->stop(cdns); >>> + cdns->role = CDNS3_ROLE_END; >> >> Why change the role here? You are just stopping the role not changing it. >> I think cdns->role should remain unchanged, so we can call cdns3_role_start() >> if required without error. >> > > The current version of this IP has some issues to detect vbus status correctly, > we have to force vbus status accordingly, so we need a status to indicate > vbus disconnection, and add some code to let controller know vbus > removal, in that case, the controller's state machine can be correct. > So, we increase one role 'CDNS3_ROLE_END' to for this purpose. > > CDNS3_ROLE_GADGET: gadget mode and VBUS on > CDNS3_ROLE_HOST: host mode and VBUS on > CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port. > > So, we may start role from CDNS3_ROLE_END at probe when nothing is connected, > and need to set role as CDNS3_ROLE_END at ->stop for further handling at > role switch routine. OK. but still this (changing to ROLE_END) must be moved to the role switch routine and the explanation you just mentioned must be added there. > >>> + mutex_unlock(&cdns->mutex); >>> +} >>> + >>> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns) >>> +{ >>> + if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) { >>> + //TODO: implements selecting device/host mode >>> + return CDNS3_ROLE_HOST; >>> + } >>> + return cdns->roles[CDNS3_ROLE_HOST] >>> + ? CDNS3_ROLE_HOST >>> + : CDNS3_ROLE_GADGET; >> >> Why not just >> return cdns->role; >> >> I'm wondering if we really need this function. > > cdns->role gets from cdns3_get_role, and this API tells role at the runtime. > If both roles are supported, the role is decided by external > conditions, eg, vbus/id > or external connector. If only single role is supported, only one role structure > is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET] > How about adding this description in function documentation. >>> +} >> >>> + >>> +/** >>> + * cdns3_core_init_role - initialize role of operation >>> + * @cdns: Pointer to cdns3 structure >>> + * >>> + * Returns 0 on success otherwise negative errno >>> + */ >>> +static int cdns3_core_init_role(struct cdns3 *cdns) >>> +{ >>> + struct device *dev = cdns->dev; >>> + enum usb_dr_mode dr_mode; >>> + >>> + dr_mode = usb_get_dr_mode(dev); >>> + cdns->role = CDNS3_ROLE_END; >>> + >>> + /* >>> + * If driver can't read mode by means of usb_get_dr_mdoe function then >>> + * chooses mode according with Kernel configuration. This setting >>> + * can be restricted later depending on strap pin configuration. >>> + */ >>> + if (dr_mode == USB_DR_MODE_UNKNOWN) { >>> + if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) && >>> + IS_ENABLED(CONFIG_USB_CDNS3_GADGET)) >>> + dr_mode = USB_DR_MODE_OTG; >>> + else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST)) >>> + dr_mode = USB_DR_MODE_HOST; >>> + else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET)) >>> + dr_mode = USB_DR_MODE_PERIPHERAL; >>> + } >>> + >>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { >>> + //TODO: implements host initialization >> >> /* TODO: Add host role */ ? >> >>> + } >>> + >>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) { >>> + //TODO: implements device initialization >> >> /* TODO: Add device role */ ? >> > > Yes, it needs to allocate cdns->roles[CDNS3_ROLE_HOST] and > cdns->roles[CDNS3_ROLE_GADGET]. > >>> + } >>> + >>> + if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) { >>> + dev_err(dev, "no supported roles\n"); >>> + return -ENODEV; >>> + } >>> + >>> + cdns->dr_mode = dr_mode; > > Pawel, why dr_mode needs to be introduced? > >>> + return 0; >>> +} >>> + >>> +/** >>> + * cdns3_irq - interrupt handler for cdns3 core device >>> + * >>> + * @irq: irq number for cdns3 core device >>> + * @data: structure of cdns3 >>> + * >>> + * Returns IRQ_HANDLED or IRQ_NONE >>> + */ >>> +static irqreturn_t cdns3_irq(int irq, void *data) >>> +{ >>> + struct cdns3 *cdns = data; >>> + irqreturn_t ret = IRQ_NONE; >>> + >>> + /* Handle device/host interrupt */ >>> + if (cdns->role != CDNS3_ROLE_END) >> >> Is it because of this that you need to set role to END at role_stop? >> I think it is better to add a state variable to struct cdns3_role_driver, so we can >> check if it is active or stopped. >> >> e.g. >> if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE) >> >>> + ret = cdns3_get_current_role_driver(cdns)->irq(cdns); >>> + >>> + return ret; >>> +} >>> + > > CDNS3_ROLE_END is introduced from above comments, we don't > need another flag for it. > If cdns->role == CDNS3_ROLE_END, it handles VBUS and ID interrupt. > >>> +static void cdns3_remove_roles(struct cdns3 *cdns) >> >> Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()? >> > > It is planed to called when at ->remove. >>> +{ >>> + //TODO: implements this function >>> +} >> >>> + >>> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role) >>> +{ >>> + enum cdns3_roles current_role; >>> + int ret = 0; >>> + >>> + current_role = cdns->role; >>> + >>> + if (role == CDNS3_ROLE_END) >>> + return 0; >> >> role == END looks like error state. and it should never happen. >> WARN here? >> > > See my comments above. > >>> + >>> + dev_dbg(cdns->dev, "Switching role"); >>> + >> >> Don't you have to stop the previous role before starting the new role? >> > > Yes, it is needed. Pawel may simply some flows to suit his platform. > >>> + ret = cdns3_role_start(cdns, role); >>> + if (ret) { >>> + /* Back to current role */ >>> + dev_err(cdns->dev, "set %d has failed, back to %d\n", >>> + role, current_role); >>> + ret = cdns3_role_start(cdns, current_role); >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +/** >>> + * cdns3_role_switch - work queue handler for role switch >>> + * >>> + * @work: work queue item structure >>> + * >>> + * Handles below events: >>> + * - Role switch for dual-role devices >>> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices >>> + */ >>> +static void cdns3_role_switch(struct work_struct *work) >>> +{ >>> + enum cdns3_roles role = CDNS3_ROLE_END; >>> + struct cdns3 *cdns; >>> + bool device, host; >>> + >>> + cdns = container_of(work, struct cdns3, role_switch_wq); >>> + >>> + //TODO: implements this functions. >>> + //host = cdns3_is_host(cdns); >>> + //device = cdns3_is_device(cdns); >>> + host = 1; >>> + device = 0; >>> + >>> + if (host) >>> + role = CDNS3_ROLE_HOST; >>> + else if (device) >>> + role = CDNS3_ROLE_GADGET; >>> + >>> + if (cdns->desired_dr_mode == cdns->current_dr_mode && >>> + cdns->role == role) >>> + return; >>> + >> >> I think all the below code can be moved to cdns3_do_role_switch(). >> >>> + pm_runtime_get_sync(cdns->dev); >>> + cdns3_role_stop(cdns); >>> + >>> + if (host) { >>> + if (cdns->roles[CDNS3_ROLE_HOST]) >>> + cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST); >>> + pm_runtime_put_sync(cdns->dev); >>> + return; >>> + } >>> + >>> + if (device) >>> + cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET); >>> + else >>> + cdns3_do_role_switch(cdns, CDNS3_ROLE_END); >>> + >>> + pm_runtime_put_sync(cdns->dev); >>> +} >>> + >>> +/** >>> + * cdns3_probe - probe for cdns3 core device >>> + * @pdev: Pointer to cdns3 core platform device >>> + * >>> + * Returns 0 on success otherwise negative errno >>> + */ >>> +static int cdns3_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct resource *res; >>> + struct cdns3 *cdns; >>> + void __iomem *regs; >>> + int ret; >>> + >>> + cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL); >>> + if (!cdns) >>> + return -ENOMEM; >>> + >>> + cdns->dev = dev; >>> + >>> + platform_set_drvdata(pdev, cdns); >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>> + if (!res) { >>> + dev_err(dev, "missing IRQ\n"); >>> + return -ENODEV; >>> + } >>> + cdns->irq = res->start; >>> + >>> + /* >>> + * Request memory region >>> + * region-0: xHCI >>> + * region-1: Peripheral >>> + * region-2: OTG registers >>> + */ >> >> The memory region order is different from the dt-binding. >> There it is OTG, host(xhci), device (peripheral). >> >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + regs = devm_ioremap_resource(dev, res); >>> + >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + cdns->xhci_regs = regs; >>> + cdns->xhci_res = res; >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >>> + regs = devm_ioremap_resource(dev, res); >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + cdns->dev_regs = regs; >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); >>> + regs = devm_ioremap_resource(dev, res); >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + cdns->otg_regs = regs; >>> + >>> + mutex_init(&cdns->mutex); >>> + >>> + cdns->phy = devm_phy_get(dev, "cdns3,usbphy"); >> >> "cdns3,usbphy" is not documented in dt-binding. >> >>> + if (IS_ERR(cdns->phy)) { >>> + dev_info(dev, "no generic phy found\n"); >>> + cdns->phy = NULL; >>> + /* >>> + * fall through here! >>> + * if no generic phy found, phy init >>> + * should be done under boot! >>> + */ >> >> No you shouldn't fall through always if it is an error condition. >> Something like this should work better. >> >> if (IS_ERR(cnds->phy)) { >> ret = PTR_ERR(cdns->phy); >> if (ret == -ENOSYS || ret == -ENODEV) { >> cdns->phy = NULL; >> } else if (ret == -EPROBE_DEFER) { >> return ret; >> } else { >> dev_err(dev, "no phy found\n"); >> goto err0; >> } >> } >> >> So if PHY was provided in DT, and PHY support/drivers is present >> and error condition means something is wrong and we have to error out. >> >>> + } else { >>> + phy_init(cdns->phy); >>> + } >> >> You can do phy_init() outside the else. >> >>> + >>> + ret = cdns3_core_init_role(cdns); >>> + if (ret) >>> + goto err1; >>> + >>> + INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch); >>> + if (ret) >>> + goto err2; >>> + >>> + if (ret) >>> + goto err2; >>> + >>> + cdns->role = cdns3_get_role(cdns); >> >> I think this should move to cdns3_core_init_role(). >> > > I agree. > >>> + >>> + ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED, >>> + dev_name(dev), cdns); >>> + >>> + if (ret) >>> + goto err2; >> >> How about moving request_irq to before cdsn3_core_init_role()? >> >> Then you can move cdns3_role_start() as well to core_init_role(). >> > > Usually, we request irq after hardware initialization has finished, if not, > there may unexpected interrupt. Doesn't kernel warn if interrupt happens and there is no handler? To avoid that I was suggesting to request_irq first. cheers, -roger -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki