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: 4282 bytes --]

On 28/07/15 10:26, Sifan Naeem wrote:
> Hi James,
> 
>> -----Original Message-----
>> From: James Hogan
>> Sent: 27 July 2015 21:21
>> To: Sifan Naeem
>> Cc: Wolfram Sang; linux-i2c@vger.kernel.org; Stable kernel (v3.19+)
>> Subject: Re: [PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip
>>
>> 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.
>>
> Yes, it's expected to be fixed in the future, albeit not in the near future.

Okay, no problem then.

> 
>>>
>>> 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
>>
> Should I send a new patch with 12 digit SHA1?

I need to review the other patches anyway (sorry i've been slow to look
through them properly).

> 
>>> 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-
>>
> 
> I go this error when doing that way:
> 
> (body) Adding cc: Sifan Naeem <sifan.naeem@imgtec.com> from line 'Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>'
> (body) Adding cc: <stable@vger.kernel.org> # 4.1 from line 'Cc: <stable@vger.kernel.org> # 4.1'
> Use of uninitialized value $cc in string eq at /usr/lib/git-core/git-send-email line 983.
> Use of uninitialized value $cc in quotemeta at /usr/lib/git-core/git-send-email line 983.
> W: unable to extract a valid address from: <stable@vger.kernel.org> # 4.1
> W: unable to extract a valid address from: <stable@vger.kernel.org> # 4.1

Sounds like you're using an old version of git. Something similar is
described here:
http://comments.gmane.org/gmane.comp.version-control.git/210042

If the warning can be ignored, you can always add stable back to cc list
with --cc=stable@vger.kernel.org.

Cheers
James

> 
>> Patch looks fine though
>>
>> Acked-by: James Hogan <james.hogan@imgtec.com>
>>
> Thanks,
> Sifan
> 
>> 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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]