LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/2] i2c: tegra: Maintain CPU endianness
@ 2015-01-20 12:22 Dmitry Osipenko
  2015-01-22  7:40 ` Thierry Reding
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2015-01-20 12:22 UTC (permalink / raw)
  To: digetx, Wolfram Sang, Stephen Warren, Thierry Reding,
	Alexandre Courbot, Laxman Dewangan
  Cc: Ben Dooks, Bob Mottram, linux-tegra, linux-i2c, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-20 12:22 [PATCH 1/2] i2c: tegra: Maintain CPU endianness Dmitry Osipenko
@ 2015-01-22  7:40 ` Thierry Reding
  2015-01-22  7:55   ` Alexandre Courbot
  0 siblings, 1 reply; 11+ messages in thread
From: Thierry Reding @ 2015-01-22  7:40 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Wolfram Sang, Stephen Warren, Alexandre Courbot, Laxman Dewangan,
	Ben Dooks, Bob Mottram, linux-tegra, linux-i2c, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1181 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@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);

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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-22  7:40 ` Thierry Reding
@ 2015-01-22  7:55   ` Alexandre Courbot
  2015-01-22 15:22     ` Dmitry Osipenko
  0 siblings, 1 reply; 11+ messages in thread
From: Alexandre Courbot @ 2015-01-22  7:55 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Dmitry Osipenko, Wolfram Sang, Stephen Warren, Laxman Dewangan,
	Ben Dooks, Bob Mottram, linux-tegra, linux-i2c,
	Linux Kernel Mailing List

On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
<thierry.reding@gmail.com> 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@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);
>
> 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-22  7:55   ` Alexandre Courbot
@ 2015-01-22 15:22     ` Dmitry Osipenko
  2015-01-22 16:06       ` Dmitry Osipenko
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2015-01-22 15:22 UTC (permalink / raw)
  To: Alexandre Courbot, Thierry Reding
  Cc: Wolfram Sang, Stephen Warren, Laxman Dewangan, Ben Dooks,
	Bob Mottram, linux-tegra, linux-i2c, Linux Kernel Mailing List

22.01.2015 10:55, Alexandre Courbot пишет:
> On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding
> <thierry.reding@gmail.com> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-22 15:22     ` Dmitry Osipenko
@ 2015-01-22 16:06       ` Dmitry Osipenko
  2015-01-22 17:18         ` Dmitry Osipenko
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2015-01-22 16:06 UTC (permalink / raw)
  To: Alexandre Courbot, Thierry Reding
  Cc: Wolfram Sang, Stephen Warren, Laxman Dewangan, Ben Dooks,
	Bob Mottram, linux-tegra, linux-i2c, Linux Kernel Mailing List

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@gmail.com> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-22 16:06       ` Dmitry Osipenko
@ 2015-01-22 17:18         ` Dmitry Osipenko
  2015-01-23  9:45           ` Thierry Reding
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2015-01-22 17:18 UTC (permalink / raw)
  To: Alexandre Courbot, Thierry Reding
  Cc: Wolfram Sang, Stephen Warren, Laxman Dewangan, Ben Dooks,
	Bob Mottram, linux-tegra, linux-i2c, Linux Kernel Mailing List

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@gmail.com> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-22 17:18         ` Dmitry Osipenko
@ 2015-01-23  9:45           ` Thierry Reding
  2015-01-23 13:27             ` Dmitry Osipenko
  0 siblings, 1 reply; 11+ messages in thread
From: Thierry Reding @ 2015-01-23  9:45 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Alexandre Courbot, Wolfram Sang, Stephen Warren, Laxman Dewangan,
	Ben Dooks, Bob Mottram, linux-tegra, linux-i2c,
	Linux Kernel Mailing List

[-- Attachment #1: Type: text/plain, Size: 1719 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@gmail.com> 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-23  9:45           ` Thierry Reding
@ 2015-01-23 13:27             ` Dmitry Osipenko
  2015-01-23 14:52               ` Dmitry Osipenko
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2015-01-23 13:27 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Alexandre Courbot, Wolfram Sang, Stephen Warren, Laxman Dewangan,
	Ben Dooks, Bob Mottram, linux-tegra, linux-i2c,
	Linux Kernel Mailing List

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@gmail.com> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-23 13:27             ` Dmitry Osipenko
@ 2015-01-23 14:52               ` Dmitry Osipenko
  2015-01-26 16:03                 ` Wolfram Sang
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2015-01-23 14:52 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Alexandre Courbot, Wolfram Sang, Stephen Warren, Laxman Dewangan,
	Ben Dooks, Bob Mottram, linux-tegra, linux-i2c,
	Linux Kernel Mailing List

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@gmail.com> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-23 14:52               ` Dmitry Osipenko
@ 2015-01-26 16:03                 ` Wolfram Sang
  2015-01-26 16:11                   ` Dmitry Osipenko
  0 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2015-01-26 16:03 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Thierry Reding, Alexandre Courbot, Stephen Warren,
	Laxman Dewangan, Ben Dooks, Bob Mottram, linux-tegra, linux-i2c,
	Linux Kernel Mailing List

[-- 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
  2015-01-26 16:03                 ` Wolfram Sang
@ 2015-01-26 16:11                   ` Dmitry Osipenko
  0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2015-01-26 16:11 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Thierry Reding, Alexandre Courbot, Stephen Warren,
	Laxman Dewangan, Ben Dooks, Bob Mottram, linux-tegra, linux-i2c,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-01-26 16:12 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-20 12:22 [PATCH 1/2] i2c: tegra: Maintain CPU endianness Dmitry Osipenko
2015-01-22  7:40 ` Thierry Reding
2015-01-22  7:55   ` Alexandre Courbot
2015-01-22 15:22     ` Dmitry Osipenko
2015-01-22 16:06       ` Dmitry Osipenko
2015-01-22 17:18         ` Dmitry Osipenko
2015-01-23  9:45           ` Thierry Reding
2015-01-23 13:27             ` Dmitry Osipenko
2015-01-23 14:52               ` Dmitry Osipenko
2015-01-26 16:03                 ` Wolfram Sang
2015-01-26 16:11                   ` Dmitry Osipenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).