From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Joel Stanley <joel@jms.id.au>
Cc: Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Russell King <linux@armlinux.org.uk>,
rostedt@goodmis.org, mingo@redhat.com,
Greg KH <gregkh@linuxfoundation.org>,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Andrew Jeffery <andrew@aj.id.au>,
Alistair Popple <alistair@popple.id.au>,
"Edward A . James" <eajames@us.ibm.com>,
Jeremy Kerr <jk@ozlabs.org>
Subject: Re: [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master
Date: Tue, 4 Apr 2017 12:32:24 -0500 [thread overview]
Message-ID: <93b21624-11fc-b71b-aa78-6cb4371c87ae@linux.vnet.ibm.com> (raw)
In-Reply-To: <1490907014.3177.207.camel@kernel.crashing.org>
On 3/30/17 3:50 PM, Benjamin Herrenschmidt wrote:
> 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.
Hi Ben,
Agreed that there is room for improvement. I intend to look further
into your suggestions from here and our private conversation on the
matter and make changes as appropriate. I have an open issue to track
this. As it exists in this patch reads/writes from master to slave
fundamentally work. Given the pervasiveness and time to fully evaluate
and test any protocol updates I intend address this in the near future
with a separate follow on patch.
Thanks,
Chris
>
> Cheers,
> Ben.
>
next prev parent reply other threads:[~2017-04-04 17:32 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-29 17:43 [PATCH v4 00/23] FSI device driver implementation Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 01/23] drivers/fsi: Add fsi master definition Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 02/23] drivers/fsi: Add slave definition Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 03/23] drivers/fsi: Add empty master scan Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 04/23] drivers/fsi: Add crc4 helpers Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 05/23] drivers/fsi: Add slave & master read/write APIs Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 06/23] drivers/fsi: Set up links for slave communication Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 07/23] drivers/fsi: Implement slave initialisation Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 08/23] drivers/fsi: Set slave SMODE to init communication Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 09/23] drivers/fsi: scan slaves & register devices Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 10/23] drivers/fsi: Add device read/write/peek API Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 11/23] drivers/fsi: Add master unscan Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 12/23] drivers/fsi: Add documentation for GPIO bindings Christopher Bostic
2017-04-03 15:24 ` Rob Herring
2017-03-29 17:43 ` [PATCH v4 13/23] drivers/fsi: Add client driver register utilities Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 14/23] drivers/fsi: Add sysfs files for FSI master & slave accesses Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 15/23] drivers/fsi: expose direct-access slave API Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 16/23] drivers/fsi: Add tracepoints for low-level operations Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 17/23] drivers/fsi: Add error handling for slave communication errors Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 18/23] drivers/fsi: Document FSI master sysfs files in ABI Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master Christopher Bostic
2017-03-30 5:48 ` Joel Stanley
2017-03-30 18:15 ` Christopher Bostic
2017-03-30 20:50 ` Benjamin Herrenschmidt
2017-04-04 17:32 ` Christopher Bostic [this message]
2017-04-04 22:19 ` Benjamin Herrenschmidt
2017-04-05 1:24 ` Christopher Bostic
2017-04-09 21:22 ` Christopher Bostic
2017-04-09 21:41 ` Benjamin Herrenschmidt
2017-04-09 21:53 ` Benjamin Herrenschmidt
2017-04-09 21:55 ` Benjamin Herrenschmidt
2017-03-30 22:57 ` Joel Stanley
2017-03-29 17:43 ` [PATCH v4 20/23] drivers/fsi/gpio: Add tracepoints for GPIO master Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 21/23] drivers/fsi: Add SCOM FSI client device driver Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 22/23] drivers/fsi: Add hub master support Christopher Bostic
2017-03-29 17:43 ` [PATCH v4 23/23] drivers/fsi: Use asynchronous slave mode Christopher Bostic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=93b21624-11fc-b71b-aa78-6cb4371c87ae@linux.vnet.ibm.com \
--to=cbostic@linux.vnet.ibm.com \
--cc=alistair@popple.id.au \
--cc=andrew@aj.id.au \
--cc=benh@kernel.crashing.org \
--cc=devicetree@vger.kernel.org \
--cc=eajames@us.ibm.com \
--cc=gregkh@linuxfoundation.org \
--cc=jk@ozlabs.org \
--cc=joel@jms.id.au \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=robh+dt@kernel.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).