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.