| Message ID | 1421756555-20266-1-git-send-email-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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.
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
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.
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.
> 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...
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!
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;
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(+)