[V15,3/6] i2c: tegra: fix maximum transfer size
diff mbox series

Message ID 1549576040-15907-3-git-send-email-skomatineni@nvidia.com
State Changes Requested
Headers show
Series
  • [V15,1/6] i2c: tegra: sort all the include headers alphabetically
Related show

Commit Message

Sowjanya Komatineni Feb. 7, 2019, 9:47 p.m. UTC
Tegra194 supports maximum 64K bytes transfer per packet.
Tegra186 and prior supports maximum 4K bytes transfer per packet.
This includes 12 bytes of packet header.

This patch fixes max write length to account for packet header size
for transfers.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V15]	: This is new patch in this series.

 drivers/i2c/busses/i2c-tegra.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Dmitry Osipenko Feb. 7, 2019, 10:09 p.m. UTC | #1
08.02.2019 0:47, Sowjanya Komatineni пишет:
> Tegra194 supports maximum 64K bytes transfer per packet.
> Tegra186 and prior supports maximum 4K bytes transfer per packet.
> This includes 12 bytes of packet header.
> 
> This patch fixes max write length to account for packet header size
> for transfers.

The commit message isn't accurate. We are not actually fixing anything here, this patch is just a preparation for adding DMA support that require the packet-header size to be excluded from the overall TX size-quirk because the packet-header is the part of the DMA transfer.

Given how small this change is, it's probably not really worth to factor out it into a separate patch, let's just squash it to to the DMA-patch.

> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V15]	: This is new patch in this series.
> 
>  drivers/i2c/busses/i2c-tegra.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 3758c7a2c781..ab474cc86e0a 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -125,6 +125,9 @@
>  #define I2C_MST_FIFO_STATUS_TX_MASK		0xff0000
>  #define I2C_MST_FIFO_STATUS_TX_SHIFT		16
>  
> +/* Packet header size in bytes */
> +#define I2C_PACKET_HEADER_SIZE			12
> +
>  /*
>   * msg_end_type: The bus control which need to be send at end of transfer.
>   * @MSG_END_STOP: Send stop pulse at end of transfer.
> @@ -900,11 +903,13 @@ static const struct i2c_algorithm tegra_i2c_algo = {
>  static const struct i2c_adapter_quirks tegra_i2c_quirks = {
>  	.flags = I2C_AQ_NO_ZERO_LEN,
>  	.max_read_len = 4096,
> -	.max_write_len = 4096,
> +	.max_write_len = 4096 - I2C_PACKET_HEADER_SIZE,
>  };
>  
>  static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
>  	.flags = I2C_AQ_NO_ZERO_LEN,
> +	.max_read_len = 65535,
> +	.max_write_len = 65535 - I2C_PACKET_HEADER_SIZE,
>  };

This is wrong, 65535 = 64 * 1024 - 1.

Let's just use size-constants provided by kernel:

static const struct i2c_adapter_quirks tegra_i2c_quirks = {
	.flags = I2C_AQ_NO_ZERO_LEN,
	.max_read_len = SZ_4K,
	.max_write_len = SZ_4K - I2C_PACKET_HEADER_SIZE,
};

static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
	.flags = I2C_AQ_NO_ZERO_LEN,
	.max_read_len = SZ_64K,
	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE,
};


I'll take a look at other patches later today, no need to send out new version right now.
Dmitry Osipenko Feb. 7, 2019, 10:25 p.m. UTC | #2
> 08.02.2019 1:16, Sowjanya Komatineni пишет:> 
>>> This is wrong, 65535 = 64 * 1024 - 1.
>>>
>>> Let's just use size-constants provided by kernel:
>>>
>>> static const struct i2c_adapter_quirks tegra_i2c_quirks = {
>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>> 	.max_read_len = SZ_4K,
>>> 	.max_write_len = SZ_4K - I2C_PACKET_HEADER_SIZE, };
>>>
>>> static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>> 	.max_read_len = SZ_64K,
>>> 	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE, };
>>>
>>>
>>> I'll take a look at other patches later today, no need to send out new version right now.
>>>
>>>
>> 
>> SIZE_64K is 0x00010000 (65536)
>> 
>> msg len is u16 and max that can be set to msg.len is 65535

Ah, okay. Then it looks about right, maybe it's time to change u16 to u32 in the I2C core.
Dmitry Osipenko Feb. 8, 2019, 12:46 p.m. UTC | #3
08.02.2019 1:25, Dmitry Osipenko пишет:
>> 08.02.2019 1:16, Sowjanya Komatineni пишет:> 
>>>> This is wrong, 65535 = 64 * 1024 - 1.
>>>>
>>>> Let's just use size-constants provided by kernel:
>>>>
>>>> static const struct i2c_adapter_quirks tegra_i2c_quirks = {
>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>>> 	.max_read_len = SZ_4K,
>>>> 	.max_write_len = SZ_4K - I2C_PACKET_HEADER_SIZE, };
>>>>
>>>> static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>>> 	.max_read_len = SZ_64K,
>>>> 	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE, };
>>>>
>>>>
>>>> I'll take a look at other patches later today, no need to send out new version right now.
>>>>
>>>>
>>>
>>> SIZE_64K is 0x00010000 (65536)
>>>
>>> msg len is u16 and max that can be set to msg.len is 65535
> 
> Ah, okay. Then it looks about right, maybe it's time to change u16 to u32 in the I2C core.
> 

Here is my suggestion:

1) Use SZ_4K / SZ_64K that I suggested above.

2) Squash this patch into the DMA-patch.

3) Add another patch (before the DMA-patch) that changes max_read_len/max_write_len types to u32 in the include/linux/i2c.h
Peter Rosin Feb. 8, 2019, 2:12 p.m. UTC | #4
On 2019-02-08 13:46, Dmitry Osipenko wrote:
> 08.02.2019 1:25, Dmitry Osipenko пишет:
>>> 08.02.2019 1:16, Sowjanya Komatineni пишет:> 
>>>>> This is wrong, 65535 = 64 * 1024 - 1.
>>>>>
>>>>> Let's just use size-constants provided by kernel:
>>>>>
>>>>> static const struct i2c_adapter_quirks tegra_i2c_quirks = {
>>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>>>> 	.max_read_len = SZ_4K,
>>>>> 	.max_write_len = SZ_4K - I2C_PACKET_HEADER_SIZE, };
>>>>>
>>>>> static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
>>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>>>> 	.max_read_len = SZ_64K,
>>>>> 	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE, };
>>>>>
>>>>>
>>>>> I'll take a look at other patches later today, no need to send out new version right now.
>>>>>
>>>>>
>>>>
>>>> SIZE_64K is 0x00010000 (65536)
>>>>
>>>> msg len is u16 and max that can be set to msg.len is 65535
>>
>> Ah, okay. Then it looks about right, maybe it's time to change u16 to u32 in the I2C core.
>>
> 
> Here is my suggestion:
> 
> 1) Use SZ_4K / SZ_64K that I suggested above.
> 
> 2) Squash this patch into the DMA-patch.
> 
> 3) Add another patch (before the DMA-patch) that changes max_read_len/max_write_len types to u32 in the include/linux/i2c.h

That is a bit naive and will not fly since the I2C core has a max message
size of 65535. Which is exposed to user space...

Given the above, saying .max_read_len = 65535 a NOP (and is in fact omitted
in places where it would be needed if the type is changed). I suggest that
it is omitted here as well. But SZ_64K - I2C_PACKET_HEADER_SIZE is still
needed of course. At least if I2C_PACKET_HEADER_SIZE is guaranteed to be
non-zero?

Cheers,
Peter
Dmitry Osipenko Feb. 8, 2019, 2:32 p.m. UTC | #5
08.02.2019 17:12, Peter Rosin пишет:
> On 2019-02-08 13:46, Dmitry Osipenko wrote:
>> 08.02.2019 1:25, Dmitry Osipenko пишет:
>>>> 08.02.2019 1:16, Sowjanya Komatineni пишет:> 
>>>>>> This is wrong, 65535 = 64 * 1024 - 1.
>>>>>>
>>>>>> Let's just use size-constants provided by kernel:
>>>>>>
>>>>>> static const struct i2c_adapter_quirks tegra_i2c_quirks = {
>>>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>>>>> 	.max_read_len = SZ_4K,
>>>>>> 	.max_write_len = SZ_4K - I2C_PACKET_HEADER_SIZE, };
>>>>>>
>>>>>> static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
>>>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>>>>> 	.max_read_len = SZ_64K,
>>>>>> 	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE, };
>>>>>>
>>>>>>
>>>>>> I'll take a look at other patches later today, no need to send out new version right now.
>>>>>>
>>>>>>
>>>>>
>>>>> SIZE_64K is 0x00010000 (65536)
>>>>>
>>>>> msg len is u16 and max that can be set to msg.len is 65535
>>>
>>> Ah, okay. Then it looks about right, maybe it's time to change u16 to u32 in the I2C core.
>>>
>>
>> Here is my suggestion:
>>
>> 1) Use SZ_4K / SZ_64K that I suggested above.
>>
>> 2) Squash this patch into the DMA-patch.
>>
>> 3) Add another patch (before the DMA-patch) that changes max_read_len/max_write_len types to u32 in the include/linux/i2c.h
> 
> That is a bit naive and will not fly since the I2C core has a max message
> size of 65535. Which is exposed to user space...
> 
> Given the above, saying .max_read_len = 65535 a NOP (and is in fact omitted
> in places where it would be needed if the type is changed). I suggest that
> it is omitted here as well. But SZ_64K - I2C_PACKET_HEADER_SIZE is still
> needed of course. At least if I2C_PACKET_HEADER_SIZE is guaranteed to be
> non-zero?

Ah, I missed that message len is also u16 and then we indeed could just drop the "max_read_len" for T194. Thank you Peter.

Sowjanya, given the above, it probably better to define dma_buf max-size in the tegra_i2c_hw_feature.
Sowjanya Komatineni Feb. 8, 2019, 3:25 p.m. UTC | #6
> 08.02.2019 17:12, Peter Rosin пишет:
> > On 2019-02-08 13:46, Dmitry Osipenko wrote:
> >> 08.02.2019 1:25, Dmitry Osipenko пишет:
> >>>> 08.02.2019 1:16, Sowjanya Komatineni пишет:>
> >>>>>> This is wrong, 65535 = 64 * 1024 - 1.
> >>>>>>
> >>>>>> Let's just use size-constants provided by kernel:
> >>>>>>
> >>>>>> static const struct i2c_adapter_quirks tegra_i2c_quirks = {
> >>>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
> >>>>>> 	.max_read_len = SZ_4K,
> >>>>>> 	.max_write_len = SZ_4K - I2C_PACKET_HEADER_SIZE, };
> >>>>>>
> >>>>>> static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
> >>>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
> >>>>>> 	.max_read_len = SZ_64K,
> >>>>>> 	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE, };
> >>>>>>
> >>>>>>
> >>>>>> I'll take a look at other patches later today, no need to send out new version right now.
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> SIZE_64K is 0x00010000 (65536)
> >>>>>
> >>>>> msg len is u16 and max that can be set to msg.len is 65535
> >>>
> >>> Ah, okay. Then it looks about right, maybe it's time to change u16 to u32 in the I2C core.
> >>>
> >>
> >> Here is my suggestion:
> >>
> >> 1) Use SZ_4K / SZ_64K that I suggested above.
> >>
> >> 2) Squash this patch into the DMA-patch.
> >>
> >> 3) Add another patch (before the DMA-patch) that changes 
> >> max_read_len/max_write_len types to u32 in the include/linux/i2c.h
> > 
> > That is a bit naive and will not fly since the I2C core has a max 
> > message size of 65535. Which is exposed to user space...
> > 
> > Given the above, saying .max_read_len = 65535 a NOP (and is in fact 
> > omitted in places where it would be needed if the type is changed). I 
> > suggest that it is omitted here as well. But SZ_64K - 
> > I2C_PACKET_HEADER_SIZE is still needed of course. At least if 
> > I2C_PACKET_HEADER_SIZE is guaranteed to be non-zero?
>
> Ah, I missed that message len is also u16 and then we indeed could just drop the "max_read_len" for T194. Thank you Peter.
>
> Sowjanya, given the above, it probably better to define dma_buf max-size in the tegra_i2c_hw_feature.
>

I2C_PACKET_HEADER_SIZE is always fixed 12. So in that case, we can just add max_write_len 
If no max_read_len, max will be 65535 anyway right

static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
	.flags = I2C_AQ_NO_ZERO_LEN,
	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE, };


sowjanya
Dmitry Osipenko Feb. 8, 2019, 3:44 p.m. UTC | #7
08.02.2019 18:25, Sowjanya Komatineni пишет:
>> 08.02.2019 17:12, Peter Rosin пишет:
>>> On 2019-02-08 13:46, Dmitry Osipenko wrote:
>>>> 08.02.2019 1:25, Dmitry Osipenko пишет:
>>>>>> 08.02.2019 1:16, Sowjanya Komatineni пишет:>
>>>>>>>> This is wrong, 65535 = 64 * 1024 - 1.
>>>>>>>>
>>>>>>>> Let's just use size-constants provided by kernel:
>>>>>>>>
>>>>>>>> static const struct i2c_adapter_quirks tegra_i2c_quirks = {
>>>>>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>>>>>>> 	.max_read_len = SZ_4K,
>>>>>>>> 	.max_write_len = SZ_4K - I2C_PACKET_HEADER_SIZE, };
>>>>>>>>
>>>>>>>> static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
>>>>>>>> 	.flags = I2C_AQ_NO_ZERO_LEN,
>>>>>>>> 	.max_read_len = SZ_64K,
>>>>>>>> 	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE, };
>>>>>>>>
>>>>>>>>
>>>>>>>> I'll take a look at other patches later today, no need to send out new version right now.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> SIZE_64K is 0x00010000 (65536)
>>>>>>>
>>>>>>> msg len is u16 and max that can be set to msg.len is 65535
>>>>>
>>>>> Ah, okay. Then it looks about right, maybe it's time to change u16 to u32 in the I2C core.
>>>>>
>>>>
>>>> Here is my suggestion:
>>>>
>>>> 1) Use SZ_4K / SZ_64K that I suggested above.
>>>>
>>>> 2) Squash this patch into the DMA-patch.
>>>>
>>>> 3) Add another patch (before the DMA-patch) that changes 
>>>> max_read_len/max_write_len types to u32 in the include/linux/i2c.h
>>>
>>> That is a bit naive and will not fly since the I2C core has a max 
>>> message size of 65535. Which is exposed to user space...
>>>
>>> Given the above, saying .max_read_len = 65535 a NOP (and is in fact 
>>> omitted in places where it would be needed if the type is changed). I 
>>> suggest that it is omitted here as well. But SZ_64K - 
>>> I2C_PACKET_HEADER_SIZE is still needed of course. At least if 
>>> I2C_PACKET_HEADER_SIZE is guaranteed to be non-zero?
>>
>> Ah, I missed that message len is also u16 and then we indeed could just drop the "max_read_len" for T194. Thank you Peter.
>>
>> Sowjanya, given the above, it probably better to define dma_buf max-size in the tegra_i2c_hw_feature.
>>
> 
> I2C_PACKET_HEADER_SIZE is always fixed 12. So in that case, we can just add max_write_len 
> If no max_read_len, max will be 65535 anyway right
> 
> static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
> 	.flags = I2C_AQ_NO_ZERO_LEN,
> 	.max_write_len = SZ_64K - I2C_PACKET_HEADER_SIZE, };

Yes, adding header size to the max_write_len for the dma_buf size also will be fine.

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 3758c7a2c781..ab474cc86e0a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -125,6 +125,9 @@ 
 #define I2C_MST_FIFO_STATUS_TX_MASK		0xff0000
 #define I2C_MST_FIFO_STATUS_TX_SHIFT		16
 
+/* Packet header size in bytes */
+#define I2C_PACKET_HEADER_SIZE			12
+
 /*
  * msg_end_type: The bus control which need to be send at end of transfer.
  * @MSG_END_STOP: Send stop pulse at end of transfer.
@@ -900,11 +903,13 @@  static const struct i2c_algorithm tegra_i2c_algo = {
 static const struct i2c_adapter_quirks tegra_i2c_quirks = {
 	.flags = I2C_AQ_NO_ZERO_LEN,
 	.max_read_len = 4096,
-	.max_write_len = 4096,
+	.max_write_len = 4096 - I2C_PACKET_HEADER_SIZE,
 };
 
 static const struct i2c_adapter_quirks tegra194_i2c_quirks = {
 	.flags = I2C_AQ_NO_ZERO_LEN,
+	.max_read_len = 65535,
+	.max_write_len = 65535 - I2C_PACKET_HEADER_SIZE,
 };
 
 static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {