| 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 | expand |
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.
> 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.
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
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
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.
> 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
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.
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 = {
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(-)