From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jae Hyun Yoo Date: Tue, 8 Oct 2019 16:28:30 -0700 Subject: [PATCH 3/5] i2c: aspeed: fix master pending state handling In-Reply-To: References: <20191007231313.4700-1-jae.hyun.yoo@linux.intel.com> <20191007231313.4700-4-jae.hyun.yoo@linux.intel.com> <422eea61-7cb9-e471-83fb-3f554ff5e079@fb.com> <6f280195-eef7-1fe7-ac42-ad6879ca9838@linux.intel.com> Message-ID: <63e99afc-17b4-e63d-f00a-d8fd29b8e735@linux.intel.com> List-Id: To: linux-aspeed@lists.ozlabs.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On 10/8/2019 4:15 PM, Tao Ren wrote: > On 10/8/19 3:45 PM, Jae Hyun Yoo wrote: >> Hi Tao, >> >> On 10/8/2019 3:00 PM, Tao Ren wrote: >>> On 10/7/19 4:13 PM, Jae Hyun Yoo wrote: >>>> In case of master pending state, it should not trigger the master >>>> command because this H/W is sharing the same data buffer for slave >>>> and master operations, so this commit fixes the issue with making >>>> the master command triggering happen when the state goes to active >>>> state. >>>> >>>> Signed-off-by: Jae Hyun Yoo >>>> --- >>>> ? drivers/i2c/busses/i2c-aspeed.c | 9 +++++---- >>>> ? 1 file changed, 5 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c >>>> index fa66951b05d0..40f6cf98d32e 100644 >>>> --- a/drivers/i2c/busses/i2c-aspeed.c >>>> +++ b/drivers/i2c/busses/i2c-aspeed.c >>>> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus) >>>> ????? struct i2c_msg *msg = &bus->msgs[bus->msgs_index]; >>>> ????? u8 slave_addr = i2c_8bit_addr_from_msg(msg); >>>> ? -??? bus->master_state = ASPEED_I2C_MASTER_START; >>>> - >>>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE) >>>> ????? /* >>>> ?????? * If it's requested in the middle of a slave session, set the master >>>> ?????? * state to 'pending' then H/W will continue handling this master >>>> ?????? * command when the bus comes back to the idle state. >>>> ?????? */ >>>> -??? if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) >>>> +??? if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) { >>>> ????????? bus->master_state = ASPEED_I2C_MASTER_PENDING; >>>> +??????? return; >>>> +??? } >>>> ? #endif /* CONFIG_I2C_SLAVE */ >>>> ? +??? bus->master_state = ASPEED_I2C_MASTER_START; >>>> ????? bus->buf_index = 0; >>>> ? ????? if (msg->flags & I2C_M_RD) { >>>> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) >>>> ????????? if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) >>>> ????????????? goto out_no_complete; >>>> ? -??????? bus->master_state = ASPEED_I2C_MASTER_START; >>>> +??????? aspeed_i2c_do_start(bus); >>>> ????? } >>> >>> Shall we move the restart-master logic from master_irq to bus_irq? The reason being: >>> master transaction cannot be restarted when aspeed-i2c is running in slave state and >>> receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case. >> >> Even in that case, master can be restarted properly because slave_irq >> will be called first because master is in MASTER_PENDING state, so the >> slave_irq handles the STOP interrupt as well, and then master_irq will >> be called with SLAVE_INACTIVE state so the aspeed_i2c_do_start can be >> called eventually. > > I mean master_irq cannot be called when irq_remaining becomes 0 after slave_irq. Ah, I see. It would be possibly happened. Probably we need to remove 'if (irq_remaining)' checking in bus_irq to make it call irqs always. Will fix the issue in the next round. Thanks, Jae From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jae Hyun Yoo Subject: Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling Date: Tue, 8 Oct 2019 16:28:30 -0700 Message-ID: <63e99afc-17b4-e63d-f00a-d8fd29b8e735@linux.intel.com> References: <20191007231313.4700-1-jae.hyun.yoo@linux.intel.com> <20191007231313.4700-4-jae.hyun.yoo@linux.intel.com> <422eea61-7cb9-e471-83fb-3f554ff5e079@fb.com> <6f280195-eef7-1fe7-ac42-ad6879ca9838@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Tao Ren , Brendan Higgins , Benjamin Herrenschmidt , Joel Stanley , Rob Herring , Mark Rutland , Andrew Jeffery Cc: "devicetree@vger.kernel.org" , "linux-aspeed@lists.ozlabs.org" , "linux-i2c@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "openbmc@lists.ozlabs.org" List-Id: linux-i2c@vger.kernel.org T24gMTAvOC8yMDE5IDQ6MTUgUE0sIFRhbyBSZW4gd3JvdGU6Cj4gT24gMTAvOC8xOSAzOjQ1IFBN LCBKYWUgSHl1biBZb28gd3JvdGU6Cj4+IEhpIFRhbywKPj4KPj4gT24gMTAvOC8yMDE5IDM6MDAg UE0sIFRhbyBSZW4gd3JvdGU6Cj4+PiBPbiAxMC83LzE5IDQ6MTMgUE0sIEphZSBIeXVuIFlvbyB3 cm90ZToKPj4+PiBJbiBjYXNlIG9mIG1hc3RlciBwZW5kaW5nIHN0YXRlLCBpdCBzaG91bGQgbm90 IHRyaWdnZXIgdGhlIG1hc3Rlcgo+Pj4+IGNvbW1hbmQgYmVjYXVzZSB0aGlzIEgvVyBpcyBzaGFy aW5nIHRoZSBzYW1lIGRhdGEgYnVmZmVyIGZvciBzbGF2ZQo+Pj4+IGFuZCBtYXN0ZXIgb3BlcmF0 aW9ucywgc28gdGhpcyBjb21taXQgZml4ZXMgdGhlIGlzc3VlIHdpdGggbWFraW5nCj4+Pj4gdGhl IG1hc3RlciBjb21tYW5kIHRyaWdnZXJpbmcgaGFwcGVuIHdoZW4gdGhlIHN0YXRlIGdvZXMgdG8g YWN0aXZlCj4+Pj4gc3RhdGUuCj4+Pj4KPj4+PiBTaWduZWQtb2ZmLWJ5OiBKYWUgSHl1biBZb28g PGphZS5oeXVuLnlvb0BsaW51eC5pbnRlbC5jb20+Cj4+Pj4gLS0tCj4+Pj4gIMKgIGRyaXZlcnMv aTJjL2J1c3Nlcy9pMmMtYXNwZWVkLmMgfCA5ICsrKysrLS0tLQo+Pj4+ICDCoCAxIGZpbGUgY2hh bmdlZCwgNSBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQo+Pj4+Cj4+Pj4gZGlmZiAtLWdp dCBhL2RyaXZlcnMvaTJjL2J1c3Nlcy9pMmMtYXNwZWVkLmMgYi9kcml2ZXJzL2kyYy9idXNzZXMv aTJjLWFzcGVlZC5jCj4+Pj4gaW5kZXggZmE2Njk1MWIwNWQwLi40MGY2Y2Y5OGQzMmUgMTAwNjQ0 Cj4+Pj4gLS0tIGEvZHJpdmVycy9pMmMvYnVzc2VzL2kyYy1hc3BlZWQuYwo+Pj4+ICsrKyBiL2Ry aXZlcnMvaTJjL2J1c3Nlcy9pMmMtYXNwZWVkLmMKPj4+PiBAQCAtMzM2LDE4ICszMzYsMTkgQEAg c3RhdGljIHZvaWQgYXNwZWVkX2kyY19kb19zdGFydChzdHJ1Y3QgYXNwZWVkX2kyY19idXMgKmJ1 cykKPj4+PiAgwqDCoMKgwqDCoCBzdHJ1Y3QgaTJjX21zZyAqbXNnID0gJmJ1cy0+bXNnc1tidXMt Pm1zZ3NfaW5kZXhdOwo+Pj4+ICDCoMKgwqDCoMKgIHU4IHNsYXZlX2FkZHIgPSBpMmNfOGJpdF9h ZGRyX2Zyb21fbXNnKG1zZyk7Cj4+Pj4gIMKgIC3CoMKgwqAgYnVzLT5tYXN0ZXJfc3RhdGUgPSBB U1BFRURfSTJDX01BU1RFUl9TVEFSVDsKPj4+PiAtCj4+Pj4gIMKgICNpZiBJU19FTkFCTEVEKENP TkZJR19JMkNfU0xBVkUpCj4+Pj4gIMKgwqDCoMKgwqAgLyoKPj4+PiAgwqDCoMKgwqDCoMKgICog SWYgaXQncyByZXF1ZXN0ZWQgaW4gdGhlIG1pZGRsZSBvZiBhIHNsYXZlIHNlc3Npb24sIHNldCB0 aGUgbWFzdGVyCj4+Pj4gIMKgwqDCoMKgwqDCoCAqIHN0YXRlIHRvICdwZW5kaW5nJyB0aGVuIEgv VyB3aWxsIGNvbnRpbnVlIGhhbmRsaW5nIHRoaXMgbWFzdGVyCj4+Pj4gIMKgwqDCoMKgwqDCoCAq IGNvbW1hbmQgd2hlbiB0aGUgYnVzIGNvbWVzIGJhY2sgdG8gdGhlIGlkbGUgc3RhdGUuCj4+Pj4g IMKgwqDCoMKgwqDCoCAqLwo+Pj4+IC3CoMKgwqAgaWYgKGJ1cy0+c2xhdmVfc3RhdGUgIT0gQVNQ RUVEX0kyQ19TTEFWRV9JTkFDVElWRSkKPj4+PiArwqDCoMKgIGlmIChidXMtPnNsYXZlX3N0YXRl ICE9IEFTUEVFRF9JMkNfU0xBVkVfSU5BQ1RJVkUpIHsKPj4+PiAgwqDCoMKgwqDCoMKgwqDCoMKg IGJ1cy0+bWFzdGVyX3N0YXRlID0gQVNQRUVEX0kyQ19NQVNURVJfUEVORElORzsKPj4+PiArwqDC oMKgwqDCoMKgwqAgcmV0dXJuOwo+Pj4+ICvCoMKgwqAgfQo+Pj4+ICDCoCAjZW5kaWYgLyogQ09O RklHX0kyQ19TTEFWRSAqLwo+Pj4+ICDCoCArwqDCoMKgIGJ1cy0+bWFzdGVyX3N0YXRlID0gQVNQ RUVEX0kyQ19NQVNURVJfU1RBUlQ7Cj4+Pj4gIMKgwqDCoMKgwqAgYnVzLT5idWZfaW5kZXggPSAw Owo+Pj4+ICDCoCDCoMKgwqDCoMKgIGlmIChtc2ctPmZsYWdzICYgSTJDX01fUkQpIHsKPj4+PiBA QCAtNDMyLDcgKzQzMyw3IEBAIHN0YXRpYyB1MzIgYXNwZWVkX2kyY19tYXN0ZXJfaXJxKHN0cnVj dCBhc3BlZWRfaTJjX2J1cyAqYnVzLCB1MzIgaXJxX3N0YXR1cykKPj4+PiAgwqDCoMKgwqDCoMKg wqDCoMKgIGlmIChidXMtPnNsYXZlX3N0YXRlICE9IEFTUEVFRF9JMkNfU0xBVkVfSU5BQ1RJVkUp Cj4+Pj4gIMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIGdvdG8gb3V0X25vX2NvbXBsZXRlOwo+ Pj4+ICDCoCAtwqDCoMKgwqDCoMKgwqAgYnVzLT5tYXN0ZXJfc3RhdGUgPSBBU1BFRURfSTJDX01B U1RFUl9TVEFSVDsKPj4+PiArwqDCoMKgwqDCoMKgwqAgYXNwZWVkX2kyY19kb19zdGFydChidXMp Owo+Pj4+ICDCoMKgwqDCoMKgIH0KPj4+Cj4+PiBTaGFsbCB3ZSBtb3ZlIHRoZSByZXN0YXJ0LW1h c3RlciBsb2dpYyBmcm9tIG1hc3Rlcl9pcnEgdG8gYnVzX2lycT8gVGhlIHJlYXNvbiBiZWluZzoK Pj4+IG1hc3RlciB0cmFuc2FjdGlvbiBjYW5ub3QgYmUgcmVzdGFydGVkIHdoZW4gYXNwZWVkLWky YyBpcyBydW5uaW5nIGluIHNsYXZlIHN0YXRlIGFuZAo+Pj4gcmVjZWl2ZXMgU1RPUCBpbnRlcnJ1 cHQsIGJlY2F1c2UgYXNwZWVkX2kyY19tYXN0ZXJfaXJxIHdvbid0IGJlIGNhbGxlZCBpbiB0aGlz IGNhc2UuCj4+Cj4+IEV2ZW4gaW4gdGhhdCBjYXNlLCBtYXN0ZXIgY2FuIGJlIHJlc3RhcnRlZCBw cm9wZXJseSBiZWNhdXNlIHNsYXZlX2lycQo+PiB3aWxsIGJlIGNhbGxlZCBmaXJzdCBiZWNhdXNl IG1hc3RlciBpcyBpbiBNQVNURVJfUEVORElORyBzdGF0ZSwgc28gdGhlCj4+IHNsYXZlX2lycSBo YW5kbGVzIHRoZSBTVE9QIGludGVycnVwdCBhcyB3ZWxsLCBhbmQgdGhlbiBtYXN0ZXJfaXJxIHdp bGwKPj4gYmUgY2FsbGVkIHdpdGggU0xBVkVfSU5BQ1RJVkUgc3RhdGUgc28gdGhlIGFzcGVlZF9p MmNfZG9fc3RhcnQgY2FuIGJlCj4+IGNhbGxlZCBldmVudHVhbGx5Lgo+IAo+IEkgbWVhbiBtYXN0 ZXJfaXJxIGNhbm5vdCBiZSBjYWxsZWQgd2hlbiBpcnFfcmVtYWluaW5nIGJlY29tZXMgMCBhZnRl ciBzbGF2ZV9pcnEuCgpBaCwgSSBzZWUuIEl0IHdvdWxkIGJlIHBvc3NpYmx5IGhhcHBlbmVkLiBQ cm9iYWJseSB3ZSBuZWVkIHRvIHJlbW92ZQonaWYgKGlycV9yZW1haW5pbmcpJyBjaGVja2luZyBp biBidXNfaXJxIHRvIG1ha2UgaXQgY2FsbCBpcnFzIGFsd2F5cy4KV2lsbCBmaXggdGhlIGlzc3Vl IGluIHRoZSBuZXh0IHJvdW5kLgoKVGhhbmtzLAoKSmFlCgpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdAps aW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVh ZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Authentication-Results: lists.ozlabs.org; spf=none (mailfrom) smtp.mailfrom=linux.intel.com (client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=jae.hyun.yoo@linux.intel.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=linux.intel.com Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 46ntnv3Z6hzDqHb; Wed, 9 Oct 2019 10:28:34 +1100 (AEDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Oct 2019 16:28:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,273,1566889200"; d="scan'208";a="187456628" Received: from yoojae-mobl1.amr.corp.intel.com (HELO [10.7.153.148]) ([10.7.153.148]) by orsmga008.jf.intel.com with ESMTP; 08 Oct 2019 16:28:30 -0700 Subject: Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling To: Tao Ren , Brendan Higgins , Benjamin Herrenschmidt , Joel Stanley , Rob Herring , Mark Rutland , Andrew Jeffery Cc: "linux-i2c@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-aspeed@lists.ozlabs.org" , "openbmc@lists.ozlabs.org" References: <20191007231313.4700-1-jae.hyun.yoo@linux.intel.com> <20191007231313.4700-4-jae.hyun.yoo@linux.intel.com> <422eea61-7cb9-e471-83fb-3f554ff5e079@fb.com> <6f280195-eef7-1fe7-ac42-ad6879ca9838@linux.intel.com> From: Jae Hyun Yoo Message-ID: <63e99afc-17b4-e63d-f00a-d8fd29b8e735@linux.intel.com> Date: Tue, 8 Oct 2019 16:28:30 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Oct 2019 23:28:36 -0000 On 10/8/2019 4:15 PM, Tao Ren wrote: > On 10/8/19 3:45 PM, Jae Hyun Yoo wrote: >> Hi Tao, >> >> On 10/8/2019 3:00 PM, Tao Ren wrote: >>> On 10/7/19 4:13 PM, Jae Hyun Yoo wrote: >>>> In case of master pending state, it should not trigger the master >>>> command because this H/W is sharing the same data buffer for slave >>>> and master operations, so this commit fixes the issue with making >>>> the master command triggering happen when the state goes to active >>>> state. >>>> >>>> Signed-off-by: Jae Hyun Yoo >>>> --- >>>>   drivers/i2c/busses/i2c-aspeed.c | 9 +++++---- >>>>   1 file changed, 5 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c >>>> index fa66951b05d0..40f6cf98d32e 100644 >>>> --- a/drivers/i2c/busses/i2c-aspeed.c >>>> +++ b/drivers/i2c/busses/i2c-aspeed.c >>>> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus) >>>>       struct i2c_msg *msg = &bus->msgs[bus->msgs_index]; >>>>       u8 slave_addr = i2c_8bit_addr_from_msg(msg); >>>>   -    bus->master_state = ASPEED_I2C_MASTER_START; >>>> - >>>>   #if IS_ENABLED(CONFIG_I2C_SLAVE) >>>>       /* >>>>        * If it's requested in the middle of a slave session, set the master >>>>        * state to 'pending' then H/W will continue handling this master >>>>        * command when the bus comes back to the idle state. >>>>        */ >>>> -    if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) >>>> +    if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) { >>>>           bus->master_state = ASPEED_I2C_MASTER_PENDING; >>>> +        return; >>>> +    } >>>>   #endif /* CONFIG_I2C_SLAVE */ >>>>   +    bus->master_state = ASPEED_I2C_MASTER_START; >>>>       bus->buf_index = 0; >>>>         if (msg->flags & I2C_M_RD) { >>>> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) >>>>           if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) >>>>               goto out_no_complete; >>>>   -        bus->master_state = ASPEED_I2C_MASTER_START; >>>> +        aspeed_i2c_do_start(bus); >>>>       } >>> >>> Shall we move the restart-master logic from master_irq to bus_irq? The reason being: >>> master transaction cannot be restarted when aspeed-i2c is running in slave state and >>> receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case. >> >> Even in that case, master can be restarted properly because slave_irq >> will be called first because master is in MASTER_PENDING state, so the >> slave_irq handles the STOP interrupt as well, and then master_irq will >> be called with SLAVE_INACTIVE state so the aspeed_i2c_do_start can be >> called eventually. > > I mean master_irq cannot be called when irq_remaining becomes 0 after slave_irq. Ah, I see. It would be possibly happened. Probably we need to remove 'if (irq_remaining)' checking in bus_irq to make it call irqs always. Will fix the issue in the next round. Thanks, Jae