Re: [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master

[Benjamin Herrenschmidt]

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.