From mboxrd@z Thu Jan 1 00:00:00 1970 From: benh@kernel.crashing.org (Benjamin Herrenschmidt) Date: Fri, 31 Mar 2017 07:50:14 +1100 Subject: [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master In-Reply-To: <0e1bcf3a-e8d7-9f50-bdf7-2a1e7466665b@linux.vnet.ibm.com> References: <20170329174340.89109-1-cbostic@linux.vnet.ibm.com> <20170329174340.89109-20-cbostic@linux.vnet.ibm.com> <0e1bcf3a-e8d7-9f50-bdf7-2a1e7466665b@linux.vnet.ibm.com> Message-ID: <1490907014.3177.207.camel@kernel.crashing.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, 2017-03-30 at 13:15 -0500, Christopher Bostic wrote: > > > +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg, > > > +?????????????????????? uint8_t num_bits) > > > +{ > > > +?????? uint8_t bit, in_bit; > > > + > > > +?????? set_sda_input(master); > > > + > > > +?????? for (bit = 0; bit < num_bits; bit++) { > > > +?????????????? clock_toggle(master, 1); > > > +?????????????? in_bit = sda_in(master); > > > +?????????????? msg->msg <<= 1; > > > +?????????????? msg->msg |= ~in_bit & 0x1;????? /* Data is negative active */ > > > +?????? } > > > +?????? msg->bits += num_bi ts; > > > +} > > > + > > > +static void serial_out(struct fsi_master_gpio *master, > > > +?????????????????????? const struct fsi_gpio_msg *cmd) > > > +{ > > > +?????? uint8_t bit; > > > +?????? uint64_t msg = ~cmd->msg;?????? /* Data is negative active */ > > > +?????? uint64_t sda_mask = 0x1ULL << (cmd->bits - 1); > > > +?????? uint64_t last_bit = ~0; > > > +?????? int next_bit; > > > + > > > +?????? if (!cmd->bits) { > > > +?????????????? dev_warn(master->dev, "trying to output 0 bits\n"); > > > +?????????????? return; > > > +?????? } > > > +?????? set_sda_output(master, 0); > > > + > > > +?????? /* Send the start bit */ > > > +?????? sda_out(master, 0); > > > +?????? clock_toggle(master, 1); > > > + > > > +?????? /* Send the message */ > > > +?????? for (bit = 0; bit < cmd->bits; bit++) { > > > +?????????????? next_bit = (msg & sda_mask) >> (cmd->bits - 1); > > > +?????????????? if (last_bit ^ next_bit) { > > > +?????????????????????? sda_out(master, next_bit); > > > +?????????????????????? last_bit = next_bit; > > > +?????????????? } > > > +?????????????? clock_toggle(master, 1); > > > +?????????????? msg <<= 1; > > > +?????? } > > > +} As I mentioned privately, I don't think this is right, unless your clock signal is inverted or my protocol spec is wrong... Your clock toggle is written so you call it right after the rising edge. It does delay, 0, delay, 1. But according to the FSI timing diagram I have, you need to establish the data around the falling edge, it gets sampled by the slave on the rising edge. So as it is, your code risks violating the slave hold time. On input, you need to sample on the falling edge, right before it. You are sampling after the rising edge, so you aren't leaving enough time for the slave to establish the data. You could probably just flip clock_toggle() around. Make it: 0, delay, 1, delay. That way you can do for sends: sda_out + toggle, and for receive toggle + sda_in. That will make you establish your output data and sample right before the falling edge, which should be ok provided the diagram I have is right. Cheers, Ben. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Benjamin Herrenschmidt Subject: Re: [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master Date: Fri, 31 Mar 2017 07:50:14 +1100 Message-ID: <1490907014.3177.207.camel@kernel.crashing.org> References: <20170329174340.89109-1-cbostic@linux.vnet.ibm.com> <20170329174340.89109-20-cbostic@linux.vnet.ibm.com> <0e1bcf3a-e8d7-9f50-bdf7-2a1e7466665b@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <0e1bcf3a-e8d7-9f50-bdf7-2a1e7466665b@linux.vnet.ibm.com> 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: Christopher Bostic , Joel Stanley Cc: Mark Rutland , devicetree@vger.kernel.org, Andrew Jeffery , Greg KH , Russell King , rostedt@goodmis.org, Linux Kernel Mailing List , Rob Herring , Jeremy Kerr , "Edward A . James" , Alistair Popple , mingo@redhat.com, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org T24gVGh1LCAyMDE3LTAzLTMwIGF0IDEzOjE1IC0wNTAwLCBDaHJpc3RvcGhlciBCb3N0aWMgd3Jv dGU6Cj4gPiA+ICtzdGF0aWMgdm9pZCBzZXJpYWxfaW4oc3RydWN0IGZzaV9tYXN0ZXJfZ3BpbyAq bWFzdGVyLCBzdHJ1Y3QgZnNpX2dwaW9fbXNnICptc2csCj4gPiA+ICvCoMKgwqDCoMKgwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCB1aW50OF90IG51bV9iaXRzKQo+ID4gPiArewo+ ID4gPiArwqDCoMKgwqDCoMKgIHVpbnQ4X3QgYml0LCBpbl9iaXQ7Cj4gPiA+ICsKPiA+ID4gK8Kg wqDCoMKgwqDCoCBzZXRfc2RhX2lucHV0KG1hc3Rlcik7Cj4gPiA+ICsKPiA+ID4gK8KgwqDCoMKg wqDCoCBmb3IgKGJpdCA9IDA7IGJpdCA8IG51bV9iaXRzOyBiaXQrKykgewo+ID4gPiArwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBjbG9ja190b2dnbGUobWFzdGVyLCAxKTsKPiA+ID4gK8Kg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgaW5fYml0ID0gc2RhX2luKG1hc3Rlcik7Cj4gPiA+ ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIG1zZy0+bXNnIDw8PSAxOwo+ID4gPiArwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBtc2ctPm1zZyB8PSB+aW5fYml0ICYgMHgxO8KgwqDC oMKgwqAgLyogRGF0YSBpcyBuZWdhdGl2ZSBhY3RpdmUgKi8KPiA+ID4gK8KgwqDCoMKgwqDCoCB9 Cj4gPiA+ICvCoMKgwqDCoMKgwqAgbXNnLT5iaXRzICs9IG51bV9iaQl0czsKPiA+ID4gK30KPiA+ ID4gKwo+ID4gPiArc3RhdGljIHZvaWQgc2VyaWFsX291dChzdHJ1Y3QgZnNpX21hc3Rlcl9ncGlv ICptYXN0ZXIsCj4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg wqDCoCBjb25zdCBzdHJ1Y3QgZnNpX2dwaW9fbXNnICpjbWQpCj4gPiA+ICt7Cj4gPiA+ICvCoMKg wqDCoMKgwqAgdWludDhfdCBiaXQ7Cj4gPiA+ICvCoMKgwqDCoMKgwqAgdWludDY0X3QgbXNnID0g fmNtZC0+bXNnO8KgwqDCoMKgwqDCoCAvKiBEYXRhIGlzIG5lZ2F0aXZlIGFjdGl2ZSAqLwo+ID4g PiArwqDCoMKgwqDCoMKgIHVpbnQ2NF90IHNkYV9tYXNrID0gMHgxVUxMIDw8IChjbWQtPmJpdHMg LSAxKTsKPiA+ID4gK8KgwqDCoMKgwqDCoCB1aW50NjRfdCBsYXN0X2JpdCA9IH4wOwo+ID4gPiAr wqDCoMKgwqDCoMKgIGludCBuZXh0X2JpdDsKPiA+ID4gKwo+ID4gPiArwqDCoMKgwqDCoMKgIGlm ICghY21kLT5iaXRzKSB7Cj4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIGRldl93 YXJuKG1hc3Rlci0+ZGV2LCAidHJ5aW5nIHRvIG91dHB1dCAwIGJpdHNcbiIpOwo+ID4gPiArwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCByZXR1cm47Cj4gPiA+ICvCoMKgwqDCoMKgwqAgfQo+ ID4gPiArwqDCoMKgwqDCoMKgIHNldF9zZGFfb3V0cHV0KG1hc3RlciwgMCk7Cj4gPiA+ICsKPiA+ ID4gK8KgwqDCoMKgwqDCoCAvKiBTZW5kIHRoZSBzdGFydCBiaXQgKi8KPiA+ID4gK8KgwqDCoMKg wqDCoCBzZGFfb3V0KG1hc3RlciwgMCk7Cj4gPiA+ICvCoMKgwqDCoMKgwqAgY2xvY2tfdG9nZ2xl KG1hc3RlciwgMSk7Cj4gPiA+ICsKPiA+ID4gK8KgwqDCoMKgwqDCoCAvKiBTZW5kIHRoZSBtZXNz YWdlICovCj4gPiA+ICvCoMKgwqDCoMKgwqAgZm9yIChiaXQgPSAwOyBiaXQgPCBjbWQtPmJpdHM7 IGJpdCsrKSB7Cj4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIG5leHRfYml0ID0g KG1zZyAmIHNkYV9tYXNrKSA+PiAoY21kLT5iaXRzIC0gMSk7Cj4gPiA+ICvCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgIGlmIChsYXN0X2JpdCBeIG5leHRfYml0KSB7Cj4gPiA+ICvCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBzZGFfb3V0KG1hc3RlciwgbmV4 dF9iaXQpOwo+ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg wqAgbGFzdF9iaXQgPSBuZXh0X2JpdDsKPiA+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg wqAgfQo+ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBjbG9ja190b2dnbGUobWFz dGVyLCAxKTsKPiA+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgbXNnIDw8PSAxOwo+ ID4gPiArwqDCoMKgwqDCoMKgIH0KPiA+ID4gK30KCkFzIEkgbWVudGlvbmVkIHByaXZhdGVseSwg SSBkb24ndCB0aGluayB0aGlzIGlzIHJpZ2h0LCB1bmxlc3MgeW91cgpjbG9jayBzaWduYWwgaXMg aW52ZXJ0ZWQgb3IgbXkgcHJvdG9jb2wgc3BlYyBpcyB3cm9uZy4uLgoKWW91ciBjbG9jayB0b2dn bGUgaXMgd3JpdHRlbiBzbyB5b3UgY2FsbCBpdCByaWdodCBhZnRlciB0aGUgcmlzaW5nCmVkZ2Uu IEl0IGRvZXMgZGVsYXksIDAsIGRlbGF5LCAxLgoKQnV0IGFjY29yZGluZyB0byB0aGUgRlNJIHRp bWluZyBkaWFncmFtIEkgaGF2ZSwgeW91IG5lZWQgdG8gZXN0YWJsaXNoCnRoZSBkYXRhIGFyb3Vu ZCB0aGUgZmFsbGluZyBlZGdlLCBpdCBnZXRzIHNhbXBsZWQgYnkgdGhlIHNsYXZlIG9uIHRoZQpy aXNpbmcgZWRnZS4gU28gYXMgaXQgaXMsIHlvdXIgY29kZSByaXNrcyB2aW9sYXRpbmcgdGhlIHNs YXZlIGhvbGQKdGltZS4KCk9uIGlucHV0LCB5b3UgbmVlZCB0byBzYW1wbGUgb24gdGhlIGZhbGxp bmcgZWRnZSwgcmlnaHQgYmVmb3JlIGl0LiBZb3UKYXJlIHNhbXBsaW5nIGFmdGVyIHRoZSByaXNp bmcgZWRnZSwgc28geW91IGFyZW4ndCBsZWF2aW5nIGVub3VnaCB0aW1lCmZvciB0aGUgc2xhdmUg dG8gZXN0YWJsaXNoIHRoZSBkYXRhLgoKWW91IGNvdWxkIHByb2JhYmx5IGp1c3QgZmxpcCBjbG9j a190b2dnbGUoKSBhcm91bmQuIE1ha2UgaXQ6IDAsIGRlbGF5LAoxLCBkZWxheS4KClRoYXQgd2F5 IHlvdSBjYW4gZG8gZm9yIHNlbmRzOiBzZGFfb3V0ICsgdG9nZ2xlLCBhbmQgZm9yIHJlY2VpdmUK dG9nZ2xlICsgc2RhX2luLiBUaGF0IHdpbGwgbWFrZSB5b3UgZXN0YWJsaXNoIHlvdXIgb3V0cHV0 IGRhdGEgYW5kCnNhbXBsZSByaWdodCBiZWZvcmUgdGhlIGZhbGxpbmcgZWRnZSwgd2hpY2ggc2hv dWxkIGJlIG9rIHByb3ZpZGVkIHRoZQpkaWFncmFtIEkgaGF2ZSBpcyByaWdodC4KCkNoZWVycywK QmVuLgoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmxp bnV4LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFk ZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4 LWFybS1rZXJuZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755026AbdC3UwO (ORCPT ); Thu, 30 Mar 2017 16:52:14 -0400 Received: from gate.crashing.org ([63.228.1.57]:44600 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754586AbdC3UwN (ORCPT ); Thu, 30 Mar 2017 16:52:13 -0400 Message-ID: <1490907014.3177.207.camel@kernel.crashing.org> Subject: Re: [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master From: Benjamin Herrenschmidt To: Christopher Bostic , Joel Stanley Cc: Rob Herring , Mark Rutland , Russell King , rostedt@goodmis.org, mingo@redhat.com, Greg KH , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Linux Kernel Mailing List , Andrew Jeffery , Alistair Popple , "Edward A . James" , Jeremy Kerr Date: Fri, 31 Mar 2017 07:50:14 +1100 In-Reply-To: <0e1bcf3a-e8d7-9f50-bdf7-2a1e7466665b@linux.vnet.ibm.com> References: <20170329174340.89109-1-cbostic@linux.vnet.ibm.com> <20170329174340.89109-20-cbostic@linux.vnet.ibm.com> <0e1bcf3a-e8d7-9f50-bdf7-2a1e7466665b@linux.vnet.ibm.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6 (3.22.6-1.fc25) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-03-30 at 13:15 -0500, Christopher Bostic wrote: > > > +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg, > > > +                       uint8_t num_bits) > > > +{ > > > +       uint8_t bit, in_bit; > > > + > > > +       set_sda_input(master); > > > + > > > +       for (bit = 0; bit < num_bits; bit++) { > > > +               clock_toggle(master, 1); > > > +               in_bit = sda_in(master); > > > +               msg->msg <<= 1; > > > +               msg->msg |= ~in_bit & 0x1;      /* Data is negative active */ > > > +       } > > > +       msg->bits += num_bi ts; > > > +} > > > + > > > +static void serial_out(struct fsi_master_gpio *master, > > > +                       const struct fsi_gpio_msg *cmd) > > > +{ > > > +       uint8_t bit; > > > +       uint64_t msg = ~cmd->msg;       /* Data is negative active */ > > > +       uint64_t sda_mask = 0x1ULL << (cmd->bits - 1); > > > +       uint64_t last_bit = ~0; > > > +       int next_bit; > > > + > > > +       if (!cmd->bits) { > > > +               dev_warn(master->dev, "trying to output 0 bits\n"); > > > +               return; > > > +       } > > > +       set_sda_output(master, 0); > > > + > > > +       /* Send the start bit */ > > > +       sda_out(master, 0); > > > +       clock_toggle(master, 1); > > > + > > > +       /* Send the message */ > > > +       for (bit = 0; bit < cmd->bits; bit++) { > > > +               next_bit = (msg & sda_mask) >> (cmd->bits - 1); > > > +               if (last_bit ^ next_bit) { > > > +                       sda_out(master, next_bit); > > > +                       last_bit = next_bit; > > > +               } > > > +               clock_toggle(master, 1); > > > +               msg <<= 1; > > > +       } > > > +} As I mentioned privately, I don't think this is right, unless your clock signal is inverted or my protocol spec is wrong... Your clock toggle is written so you call it right after the rising edge. It does delay, 0, delay, 1. But according to the FSI timing diagram I have, you need to establish the data around the falling edge, it gets sampled by the slave on the rising edge. So as it is, your code risks violating the slave hold time. On input, you need to sample on the falling edge, right before it. You are sampling after the rising edge, so you aren't leaving enough time for the slave to establish the data. You could probably just flip clock_toggle() around. Make it: 0, delay, 1, delay. That way you can do for sends: sda_out + toggle, and for receive toggle + sda_in. That will make you establish your output data and sample right before the falling edge, which should be ok provided the diagram I have is right. Cheers, Ben.