lkml.org 
[lkml]   [2017]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master
From
Date


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.
>

\
 
 \ /
  Last update: 2017-04-04 19:33    [W:0.075 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site