[PATCH 1/2] i2c: tegra: Maintain CPU endianness

[PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Dmitry Osipenko]

Support CPU BE mode by adding endianness conversion for memcpy interactions.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 28b87e6..e0d3ef1 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -286,6 +286,7 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
 	if (rx_fifo_avail > 0 && buf_remaining > 0) {
 		BUG_ON(buf_remaining > 3);
 		val = i2c_readl(i2c_dev, I2C_RX_FIFO);
+		val = cpu_to_le32(val);
 		memcpy(buf, &val, buf_remaining);
 		buf_remaining = 0;
 		rx_fifo_avail--;
@@ -343,7 +344,9 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
 	 */
 	if (tx_fifo_avail > 0 && buf_remaining > 0) {
 		BUG_ON(buf_remaining > 3);
+		val = 0;
 		memcpy(&val, buf, buf_remaining);
+		val = cpu_to_le32(val);
 
 		/* Again update before writing to FIFO to make sure isr sees. */
 		i2c_dev->msg_buf_remaining = 0;
-- 
2.2.1

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Thierry Reding]

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

On Tue, Jan 20, 2015 at 03:22:25PM +0300, Dmitry Osipenko wrote:
> Support CPU BE mode by adding endianness conversion for memcpy interactions.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 28b87e6..e0d3ef1 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -286,6 +286,7 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
>  	if (rx_fifo_avail > 0 && buf_remaining > 0) {
>  		BUG_ON(buf_remaining > 3);
>  		val = i2c_readl(i2c_dev, I2C_RX_FIFO);
> +		val = cpu_to_le32(val);

Should this not technically be le32_to_cpu() since the data originates
from the I2C controller?

>  		memcpy(buf, &val, buf_remaining);
>  		buf_remaining = 0;
>  		rx_fifo_avail--;
> @@ -343,7 +344,9 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
>  	 */
>  	if (tx_fifo_avail > 0 && buf_remaining > 0) {
>  		BUG_ON(buf_remaining > 3);
> +		val = 0;

Why does this have to be initialized to 0 now?

Thierry

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

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Alexandre Courbot]

On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jan 20, 2015 at 03:22:25PM +0300, Dmitry Osipenko wrote:
>> Support CPU BE mode by adding endianness conversion for memcpy interactions.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-tegra.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 28b87e6..e0d3ef1 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -286,6 +286,7 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
>>       if (rx_fifo_avail > 0 && buf_remaining > 0) {
>>               BUG_ON(buf_remaining > 3);
>>               val = i2c_readl(i2c_dev, I2C_RX_FIFO);
>> +             val = cpu_to_le32(val);
>
> Should this not technically be le32_to_cpu() since the data originates
> from the I2C controller?
>
>>               memcpy(buf, &val, buf_remaining);
>>               buf_remaining = 0;
>>               rx_fifo_avail--;
>> @@ -343,7 +344,9 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
>>        */
>>       if (tx_fifo_avail > 0 && buf_remaining > 0) {
>>               BUG_ON(buf_remaining > 3);
>> +             val = 0;
>
> Why does this have to be initialized to 0 now?

I suspect this is because we are going to memcpy less than 4 bytes
into it, but I cannot figure out how that memcpy if guaranteed to
produce the expected result for both endiannesses.

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Dmitry Osipenko]

22.01.2015 10:55, Alexandre Courbot пишет:
> On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> Should this not technically be le32_to_cpu() since the data originates
>> from the I2C controller?

No, i2c_readl returns value in CPU endianness, so it's correct. But for 
i2c_writel should be used le32_to_cpu(), since it takes value in CPU endianness.
It's my overlook, V2 is coming.

>>
>> Why does this have to be initialized to 0 now?
>
> I suspect this is because we are going to memcpy less than 4 bytes
> into it, but I cannot figure out how that memcpy if guaranteed to
> produce the expected result for both endiannesses.
>
That's correct. Memcpy is working with bytes, so it doesn't care about 
endianness and produces expected result, since I2C message is char array.

-- 
Dmitry

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Dmitry Osipenko]

22.01.2015 18:22, Dmitry Osipenko пишет:
> 22.01.2015 10:55, Alexandre Courbot пишет:
>> On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
>> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>> Should this not technically be le32_to_cpu() since the data originates
>>> from the I2C controller?
>
> No, i2c_readl returns value in CPU endianness, so it's correct. But for
> i2c_writel should be used le32_to_cpu(), since it takes value in CPU endianness.
> It's my overlook, V2 is coming.
>
>>>
>>> Why does this have to be initialized to 0 now?
>>
>> I suspect this is because we are going to memcpy less than 4 bytes
>> into it, but I cannot figure out how that memcpy if guaranteed to
>> produce the expected result for both endiannesses.
>>
> That's correct. Memcpy is working with bytes, so it doesn't care about
> endianness and produces expected result, since I2C message is char array.
>
I'll spend some more time reviewing, to see if nullifying should go as separate 
patch.

-- 
Dmitry

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Dmitry Osipenko]

22.01.2015 19:06, Dmitry Osipenko пишет:
> 22.01.2015 18:22, Dmitry Osipenko пишет:
>> 22.01.2015 10:55, Alexandre Courbot пишет:
>>> On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
>>> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>
>>>> Should this not technically be le32_to_cpu() since the data originates
>>>> from the I2C controller?
>>
>> No, i2c_readl returns value in CPU endianness, so it's correct. But for
>> i2c_writel should be used le32_to_cpu(), since it takes value in CPU endianness.
>> It's my overlook, V2 is coming.
>>
>>>>
>>>> Why does this have to be initialized to 0 now?
>>>
>>> I suspect this is because we are going to memcpy less than 4 bytes
>>> into it, but I cannot figure out how that memcpy if guaranteed to
>>> produce the expected result for both endiannesses.
>>>
>> That's correct. Memcpy is working with bytes, so it doesn't care about
>> endianness and produces expected result, since I2C message is char array.
>>
> I'll spend some more time reviewing, to see if nullifying should go as separate
> patch.
>
Well, I2C_FIFO_STATUS returns 8-bit value. The rest of bits very likely to be 
RAZ, however I don't see anything on it in documentation. In that case it won't 
cause any problems with LE value and nullifying is only needed for BE mode.

-- 
Dmitry

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Thierry Reding]

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

On Thu, Jan 22, 2015 at 08:18:34PM +0300, Dmitry Osipenko wrote:
> 22.01.2015 19:06, Dmitry Osipenko пишет:
> >22.01.2015 18:22, Dmitry Osipenko пишет:
> >>22.01.2015 10:55, Alexandre Courbot пишет:
> >>>On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
> >>><thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>>
> >>>>Should this not technically be le32_to_cpu() since the data originates
> >>>>from the I2C controller?
> >>
> >>No, i2c_readl returns value in CPU endianness, so it's correct. But for
> >>i2c_writel should be used le32_to_cpu(), since it takes value in CPU endianness.
> >>It's my overlook, V2 is coming.
> >>
> >>>>
> >>>>Why does this have to be initialized to 0 now?
> >>>
> >>>I suspect this is because we are going to memcpy less than 4 bytes
> >>>into it, but I cannot figure out how that memcpy if guaranteed to
> >>>produce the expected result for both endiannesses.
> >>>
> >>That's correct. Memcpy is working with bytes, so it doesn't care about
> >>endianness and produces expected result, since I2C message is char array.
> >>
> >I'll spend some more time reviewing, to see if nullifying should go as separate
> >patch.
> >
> Well, I2C_FIFO_STATUS returns 8-bit value. The rest of bits very likely to
> be RAZ, however I don't see anything on it in documentation. In that case it
> won't cause any problems with LE value and nullifying is only needed for BE
> mode.

What does I2C_FIFO_STATUS have to do with anything?

My point was more that we already tell hardware how much data is to be
transferred (via the packet header in tegra_i2c_xfer_msg()), hence the
hardware shouldn't care whether the FIFO is padded with random data or
zeros.

Thierry

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

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Dmitry Osipenko]

23.01.2015 12:45, Thierry Reding пишет:
> On Thu, Jan 22, 2015 at 08:18:34PM +0300, Dmitry Osipenko wrote:
>> 22.01.2015 19:06, Dmitry Osipenko пишет:
>>> 22.01.2015 18:22, Dmitry Osipenko пишет:
>>>> 22.01.2015 10:55, Alexandre Courbot пишет:
>>>>> On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
>>>>> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>
>>>>>> Should this not technically be le32_to_cpu() since the data originates
>>>>> >from the I2C controller?
>>>>
>>>> No, i2c_readl returns value in CPU endianness, so it's correct. But for
>>>> i2c_writel should be used le32_to_cpu(), since it takes value in CPU endianness.
>>>> It's my overlook, V2 is coming.
>>>>
>>>>>>
>>>>>> Why does this have to be initialized to 0 now?
>>>>>
>>>>> I suspect this is because we are going to memcpy less than 4 bytes
>>>>> into it, but I cannot figure out how that memcpy if guaranteed to
>>>>> produce the expected result for both endiannesses.
>>>>>
>>>> That's correct. Memcpy is working with bytes, so it doesn't care about
>>>> endianness and produces expected result, since I2C message is char array.
>>>>
>>> I'll spend some more time reviewing, to see if nullifying should go as separate
>>> patch.
>>>
>> Well, I2C_FIFO_STATUS returns 8-bit value. The rest of bits very likely to
>> be RAZ, however I don't see anything on it in documentation. In that case it
>> won't cause any problems with LE value and nullifying is only needed for BE
>> mode.
>
> What does I2C_FIFO_STATUS have to do with anything?
>
> My point was more that we already tell hardware how much data is to be
> transferred (via the packet header in tegra_i2c_xfer_msg()), hence the
> hardware shouldn't care whether the FIFO is padded with random data or
> zeros.
>
> Thierry
>
Got your point. I was thinking it's expected behavior, but now I'll elaborate 
this more.

-- 
Dmitry

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Dmitry Osipenko]

23.01.2015 16:27, Dmitry Osipenko пишет:
> 23.01.2015 12:45, Thierry Reding пишет:
>> On Thu, Jan 22, 2015 at 08:18:34PM +0300, Dmitry Osipenko wrote:
>>> 22.01.2015 19:06, Dmitry Osipenko пишет:
>>>> 22.01.2015 18:22, Dmitry Osipenko пишет:
>>>>> 22.01.2015 10:55, Alexandre Courbot пишет:
>>>>>> On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
>>>>>> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>
>>>>>>> Should this not technically be le32_to_cpu() since the data originates
>>>>>> >from the I2C controller?
>>>>>
>>>>> No, i2c_readl returns value in CPU endianness, so it's correct. But for
>>>>> i2c_writel should be used le32_to_cpu(), since it takes value in CPU
>>>>> endianness.
>>>>> It's my overlook, V2 is coming.
>>>>>
>>>>>>>
>>>>>>> Why does this have to be initialized to 0 now?
>>>>>>
>>>>>> I suspect this is because we are going to memcpy less than 4 bytes
>>>>>> into it, but I cannot figure out how that memcpy if guaranteed to
>>>>>> produce the expected result for both endiannesses.
>>>>>>
>>>>> That's correct. Memcpy is working with bytes, so it doesn't care about
>>>>> endianness and produces expected result, since I2C message is char array.
>>>>>
>>>> I'll spend some more time reviewing, to see if nullifying should go as separate
>>>> patch.
>>>>
>>> Well, I2C_FIFO_STATUS returns 8-bit value. The rest of bits very likely to
>>> be RAZ, however I don't see anything on it in documentation. In that case it
>>> won't cause any problems with LE value and nullifying is only needed for BE
>>> mode.
>>
>> What does I2C_FIFO_STATUS have to do with anything?
>>
>> My point was more that we already tell hardware how much data is to be
>> transferred (via the packet header in tegra_i2c_xfer_msg()), hence the
>> hardware shouldn't care whether the FIFO is padded with random data or
>> zeros.
>>
>> Thierry
>>
> Got your point. I was thinking it's expected behavior, but now I'll elaborate
> this more.
>
Gaahh! I'm sure it wasn't working before! I'll make more testing and send v3 
without "val = 0", if all will be fine.

-- 
Dmitry

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Wolfram Sang]

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


> Gaahh! I'm sure it wasn't working before! I'll make more testing and send v3
> without "val = 0", if all will be fine.

Please either send V3 or explicitly say V2 is OK. No need to hurry, just
saying that I am waiting for updates here...


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

Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness

[Dmitry Osipenko]

26.01.2015 19:03, Wolfram Sang пишет:
>
>> Gaahh! I'm sure it wasn't working before! I'll make more testing and send v3
>> without "val = 0", if all will be fine.
>
> Please either send V3 or explicitly say V2 is OK. No need to hurry, just
> saying that I am waiting for updates here...
>
Sure!

-- 
Dmitry