Re: [PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]

Hi Sifan,

On Mon, Jul 27, 2015 at 12:47:14PM +0100, Sifan Naeem wrote:
> The code to read from the master read fifo, and write to the master
> write fifo, checks a bit in an SCB register before every byte to
> ensure that the fifo is not full (write fifo) or empty (read fifo).
> Due to clock domain crossing inside the SCB block the updated value
> of this bit is only visible after 2 cycles.
> 
> The scb_wr_rd_fence() function does 2 dummy writes (to the read-only
> revision register), and it's called before reading from or writing to the
> fifos to ensure that subsequent reads of the fifo status bits do not read
> stale values.
> 
> As the 2 dummy writes are required in all versions of the ip, the version
> check is dropped.

Is it anticipated that a future version of the hardware will probably
resolve the clock domain crossing issue? If so fine, but if not its
probably worth removing need_wr_rd_fence.

> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")

I believe 12 digits of SHA1 is recommended now, to avoid collisions. I
suggest doing this:
$ git config --global core.abbrev 12

> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>

That's a fairly non-conventional way to specify stable versions. The
recommended way to Cc stable according to
Documentation/stable_kernel_rules.txt is more like this:
Cc: <stable@vger.kernel.org> # 3.19.x-

Patch looks fine though

Acked-by: James Hogan <james.hogan@imgtec.com>

Thanks
James

> ---
>  drivers/i2c/busses/i2c-img-scb.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 00ffd66..5c3c615 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -278,8 +278,6 @@
>  #define ISR_COMPLETE(err)	(ISR_COMPLETE_M | (ISR_STATUS_M & (err)))
>  #define ISR_FATAL(err)		(ISR_COMPLETE(err) | ISR_FATAL_M)
>  
> -#define REL_SOC_IP_SCB_2_2_1	0x00020201
> -
>  enum img_i2c_mode {
>  	MODE_INACTIVE,
>  	MODE_RAW,
> @@ -1120,10 +1118,8 @@ static int img_i2c_init(struct img_i2c *i2c)
>  		return -EINVAL;
>  	}
>  
> -	if (rev == REL_SOC_IP_SCB_2_2_1) {
> -		i2c->need_wr_rd_fence = true;
> -		dev_info(i2c->adap.dev.parent, "fence quirk enabled");
> -	}
> +	/* Fencing enabled by default. */
> +	i2c->need_wr_rd_fence = true;
>  
>  	bitrate_khz = i2c->bitrate / 1000;
>  	clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> -- 
> 1.7.9.5
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]