Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
> 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... signature.asc Description: Digital signature
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
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 > >>> 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 pgpk1o_K2E6Ws.pgp Description: PGP signature
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
22.01.2015 10:55, Alexandre Courbot пишет: On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
On Thu, Jan 22, 2015 at 4:40 PM, Thierry Reding 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 >> --- >> 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c: tegra: Maintain CPU endianness
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 > --- > 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 pgpYy2UvwAmD5.pgp Description: PGP signature
[PATCH 1/2] i2c: tegra: Maintain CPU endianness
Support CPU BE mode by adding endianness conversion for memcpy interactions. Signed-off-by: Dmitry Osipenko --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/