[PATCH linux dev-4.7] drivers/fsi: FSI protocol updates to improve reliability

[Eddie James]

On 04/10/2017 12:55 PM, Christopher Bostic wrote:
> Re-order delays in the clock toggle from: delay clock 0, delay,
> clock 1 to clock 0, delay, clock 1, delay.  Allows removal of
> delay added just prior to sampling SDA line for SDA input.
>
> Slow down the delay period from 1ns to 3us.  The original 1ns
> delay was actually 1-2 us based on real time measurements so
> this isn't a factor of 3000x slowdown.
>
> Removed echo delay on send command as its not needed.
>
> Removed the 100x prime clocks after command and now send 50x
> clocks prior to command as is done by the pdbg tool.
>
> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
> ---
>   drivers/fsi/fsi-master-gpio.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index a8976d5..1517c21 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -13,7 +13,7 @@
>
>   #include "fsi-master.h"
>
> -#define	FSI_GPIO_STD_DLY	1	/* Standard pin delay in nS */
> +#define	FSI_GPIO_STD_DLY	3	/* Standard pin delay in uS */
>   #define	FSI_ECHO_DELAY_CLOCKS	16	/* Number clocks for echo delay */
>   #define	FSI_PRE_BREAK_CLOCKS	50	/* Number clocks to prep for break */
>   #define	FSI_BREAK_CLOCKS	256	/* Number of clocks to issue break */
> @@ -61,7 +61,6 @@
>   #define	FSI_GPIO_CRC_SIZE	4
>   #define	FSI_GPIO_MSG_ID_SIZE		2
>   #define	FSI_GPIO_MSG_RESPID_SIZE	2
> -#define	FSI_GPIO_PRIME_SLAVE_CLOCKS	100
>
>   static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);	/* lock around fsi commands */
>
> @@ -86,10 +85,10 @@ static void clock_toggle(struct fsi_master_gpio *master, int count)
>   	int i;
>
>   	for (i = 0; i < count; i++) {
> -		ndelay(FSI_GPIO_STD_DLY);
>   		gpiod_set_value(master->gpio_clk, 0);
> -		ndelay(FSI_GPIO_STD_DLY);
> +		udelay(FSI_GPIO_STD_DLY);
>   		gpiod_set_value(master->gpio_clk, 1);
> +		udelay(FSI_GPIO_STD_DLY);
>   	}
>   }
>
> @@ -97,7 +96,6 @@ static int sda_in(struct fsi_master_gpio *master)
>   {
>   	int in;
>
> -	ndelay(FSI_GPIO_STD_DLY);
>   	in = gpiod_get_value(master->gpio_data);
>   	return in ? 1 : 0;
>   }
> @@ -287,8 +285,6 @@ static int poll_for_response(struct fsi_master_gpio *master, uint8_t expected,
>   			fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
>   			return -EIO;
>   		}
> -		/* Clock the slave enough to be ready for next operation */
> -		clock_toggle(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);
>   		return 0;
>
>   	} while (busy_count++ < FSI_GPIO_MAX_BUSY);
> @@ -362,8 +358,9 @@ static int send_command(struct fsi_master_gpio *master,
>   	int rc;
>
>   	spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
> +	set_sda_output(master, 1);
> +	clock_toggle(master, 50);
>   	serial_out(master, cmd);
> -	echo_delay(master);
>   	rc = poll_for_response(master, expected, size, val);
>   	spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
>

Looks good to me.

Acked-by: Eddie James <eajames at linux.vnet.ibm.com>