Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working
On 2018-03-24 17:52, Wolfram Sang wrote: On Mon, Mar 12, 2018 at 06:44:49PM +0530, Abhishek Sahu wrote: * v2: 1. Address review comments in v1 2. Changed the license to SPDX 3. Changed commit messages for some of the patch having more detail 4. Removed event-based completion and changed transfer completion detection logic in interrupt handler 5. Removed dma_threshold and blk_mode_threshold from global structure 6. Improved determine mode logic for QUP v2 transfers 7. Fixed function comments 8. Fixed auto build test WARNING ‘idx' may be used uninitialized in this function 9. Renamed tx/rx_buf to tx/rx_cnt * v1: The current driver is failing in following test case 1. Handling of failure cases is not working in long run for BAM mode. It generates error message “bam-dma-engine 7884000.dma: Cannot free busy channel” sometimes. 2. Following I2C transfers are failing a. Single transfer with multiple read messages b. Single transfer with multiple read/write message with maximum allowed length per message (65K) in BAM mode c. Single transfer with write greater than 32 bytes in QUP v1 and write greater than 64 bytes in QUP v2 for non-DMA mode. 3. No handling is present for Block/FIFO interrupts. Any non-error interrupts are being treated as the transfer completion and then polling is being done for available/free bytes in FIFO. To fix all these issues, major code changes are required. This patch series fixes all the above issues and makes the driver interrupt based instead of polling based. After these changes, all the mentioned test cases are working properly. The code changes have been tested for QUP v1 (IPQ8064) and QUP v2 (IPQ8074) with sample application written over i2c-dev. Abhishek Sahu (13): i2c: qup: fix copyrights and update to SPDX identifier i2c: qup: fixed releasing dma without flush operation completion i2c: qup: minor code reorganization for use_dma i2c: qup: remove redundant variables for BAM SG count i2c: qup: schedule EOT and FLUSH tags at the end of transfer i2c: qup: fix the transfer length for BAM RX EOT FLUSH tags i2c: qup: proper error handling for i2c error in BAM mode i2c: qup: use the complete transfer length to choose DMA mode i2c: qup: change completion timeout according to transfer length i2c: qup: fix buffer overflow for multiple msg of maximum xfer len i2c: qup: send NACK for last read sub transfers i2c: qup: reorganization of driver code to remove polling for qup v1 i2c: qup: reorganization of driver code to remove polling for qup v2 Applied to for-next, thanks! Also thanks to the reviewers! Thanks Wolfram for your help in getting this big patch series applied to for-next. Thanks to Andy, Sricharan, Austin and other reviewers for reviewing/testing the patches. Regards, Abhishek
Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working
On Mon, Mar 12, 2018 at 06:44:49PM +0530, Abhishek Sahu wrote: > * v2: > > 1. Address review comments in v1 > 2. Changed the license to SPDX > 3. Changed commit messages for some of the patch having more detail > 4. Removed event-based completion and changed transfer completion >detection logic in interrupt handler > 5. Removed dma_threshold and blk_mode_threshold from global structure > 6. Improved determine mode logic for QUP v2 transfers > 7. Fixed function comments > 8. Fixed auto build test WARNING ‘idx' may be used uninitialized >in this function > 9. Renamed tx/rx_buf to tx/rx_cnt > > * v1: > > The current driver is failing in following test case > 1. Handling of failure cases is not working in long run for BAM >mode. It generates error message “bam-dma-engine 7884000.dma: Cannot >free busy channel” sometimes. > 2. Following I2C transfers are failing >a. Single transfer with multiple read messages >b. Single transfer with multiple read/write message with maximum > allowed length per message (65K) in BAM mode >c. Single transfer with write greater than 32 bytes in QUP v1 and > write greater than 64 bytes in QUP v2 for non-DMA mode. > 3. No handling is present for Block/FIFO interrupts. Any non-error >interrupts are being treated as the transfer completion and then >polling is being done for available/free bytes in FIFO. > > To fix all these issues, major code changes are required. This patch > series fixes all the above issues and makes the driver interrupt based > instead of polling based. After these changes, all the mentioned test > cases are working properly. > > The code changes have been tested for QUP v1 (IPQ8064) and QUP > v2 (IPQ8074) with sample application written over i2c-dev. > > Abhishek Sahu (13): > i2c: qup: fix copyrights and update to SPDX identifier > i2c: qup: fixed releasing dma without flush operation completion > i2c: qup: minor code reorganization for use_dma > i2c: qup: remove redundant variables for BAM SG count > i2c: qup: schedule EOT and FLUSH tags at the end of transfer > i2c: qup: fix the transfer length for BAM RX EOT FLUSH tags > i2c: qup: proper error handling for i2c error in BAM mode > i2c: qup: use the complete transfer length to choose DMA mode > i2c: qup: change completion timeout according to transfer length > i2c: qup: fix buffer overflow for multiple msg of maximum xfer len > i2c: qup: send NACK for last read sub transfers > i2c: qup: reorganization of driver code to remove polling for qup v1 > i2c: qup: reorganization of driver code to remove polling for qup v2 Applied to for-next, thanks! Also thanks to the reviewers! signature.asc Description: PGP signature
Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working
I trust the reviews of Andy and Sricharan for this series. From what I looked at, the patches look good to me. Only one question left about copyrights (raised seperately), but we are good to go I think. Thanks for all the review! signature.asc Description: PGP signature
Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working
On Tue, Mar 13, 2018 at 04:12:19PM -0600, Christ, Austin wrote: > Sorry for the miscommunication. I reviewed the patches and tested them on > the Centriq 2400 platform. > > Perhaps the following is the most appropriate. > > Acked-by: Austin Christ If you are okay with that, I'll just read it as "Tested-by" for the whole series. Thanks! signature.asc Description: PGP signature
Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working
On 2018-03-14 03:42, Christ, Austin wrote: Sorry for the miscommunication. I reviewed the patches and tested them on the Centriq 2400 platform. Perhaps the following is the most appropriate. Acked-by: Austin Christ Thanks Austin for reviewing and testing the code changes. It would be more helpful if you provide your 'Tested-by' for last 2 changes. Sricharan has already given 'Reviewed-by' for last 2 major changes and your 'Tested-by' will help in confirming that these changes are working fine in other platforms also. Thanks, Abhishek On 3/13/2018 3:17 PM, Wolfram Sang wrote: On Tue, Mar 13, 2018 at 03:09:00PM -0600, Christ, Austin wrote: I've tested this v2 series on Centriq 2400. Looks good to me. Reviewed-by: Austin Christ I am a little confused. Do you mean Tested-by or Reviewed-by?
Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working
Sorry for the miscommunication. I reviewed the patches and tested them on the Centriq 2400 platform. Perhaps the following is the most appropriate. Acked-by: Austin Christ On 3/13/2018 3:17 PM, Wolfram Sang wrote: On Tue, Mar 13, 2018 at 03:09:00PM -0600, Christ, Austin wrote: I've tested this v2 series on Centriq 2400. Looks good to me. Reviewed-by: Austin Christ I am a little confused. Do you mean Tested-by or Reviewed-by? -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working
On Tue, Mar 13, 2018 at 03:09:00PM -0600, Christ, Austin wrote: > I've tested this v2 series on Centriq 2400. Looks good to me. > > Reviewed-by: Austin Christ I am a little confused. Do you mean Tested-by or Reviewed-by? signature.asc Description: PGP signature
Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working
I've tested this v2 series on Centriq 2400. Looks good to me. Reviewed-by: Austin Christ On 3/12/2018 7:14 AM, Abhishek Sahu wrote: * v2: 1. Address review comments in v1 2. Changed the license to SPDX 3. Changed commit messages for some of the patch having more detail 4. Removed event-based completion and changed transfer completion detection logic in interrupt handler 5. Removed dma_threshold and blk_mode_threshold from global structure 6. Improved determine mode logic for QUP v2 transfers 7. Fixed function comments 8. Fixed auto build test WARNING ‘idx' may be used uninitialized in this function 9. Renamed tx/rx_buf to tx/rx_cnt * v1: The current driver is failing in following test case 1. Handling of failure cases is not working in long run for BAM mode. It generates error message “bam-dma-engine 7884000.dma: Cannot free busy channel” sometimes. 2. Following I2C transfers are failing a. Single transfer with multiple read messages b. Single transfer with multiple read/write message with maximum allowed length per message (65K) in BAM mode c. Single transfer with write greater than 32 bytes in QUP v1 and write greater than 64 bytes in QUP v2 for non-DMA mode. 3. No handling is present for Block/FIFO interrupts. Any non-error interrupts are being treated as the transfer completion and then polling is being done for available/free bytes in FIFO. To fix all these issues, major code changes are required. This patch series fixes all the above issues and makes the driver interrupt based instead of polling based. After these changes, all the mentioned test cases are working properly. The code changes have been tested for QUP v1 (IPQ8064) and QUP v2 (IPQ8074) with sample application written over i2c-dev. Abhishek Sahu (13): i2c: qup: fix copyrights and update to SPDX identifier i2c: qup: fixed releasing dma without flush operation completion i2c: qup: minor code reorganization for use_dma i2c: qup: remove redundant variables for BAM SG count i2c: qup: schedule EOT and FLUSH tags at the end of transfer i2c: qup: fix the transfer length for BAM RX EOT FLUSH tags i2c: qup: proper error handling for i2c error in BAM mode i2c: qup: use the complete transfer length to choose DMA mode i2c: qup: change completion timeout according to transfer length i2c: qup: fix buffer overflow for multiple msg of maximum xfer len i2c: qup: send NACK for last read sub transfers i2c: qup: reorganization of driver code to remove polling for qup v1 i2c: qup: reorganization of driver code to remove polling for qup v2 drivers/i2c/busses/i2c-qup.c | 1507 -- 1 file changed, 880 insertions(+), 627 deletions(-) -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
[PATCH v2 00/13] Major code reorganization to make all i2c transfers working
* v2: 1. Address review comments in v1 2. Changed the license to SPDX 3. Changed commit messages for some of the patch having more detail 4. Removed event-based completion and changed transfer completion detection logic in interrupt handler 5. Removed dma_threshold and blk_mode_threshold from global structure 6. Improved determine mode logic for QUP v2 transfers 7. Fixed function comments 8. Fixed auto build test WARNING ‘idx' may be used uninitialized in this function 9. Renamed tx/rx_buf to tx/rx_cnt * v1: The current driver is failing in following test case 1. Handling of failure cases is not working in long run for BAM mode. It generates error message “bam-dma-engine 7884000.dma: Cannot free busy channel” sometimes. 2. Following I2C transfers are failing a. Single transfer with multiple read messages b. Single transfer with multiple read/write message with maximum allowed length per message (65K) in BAM mode c. Single transfer with write greater than 32 bytes in QUP v1 and write greater than 64 bytes in QUP v2 for non-DMA mode. 3. No handling is present for Block/FIFO interrupts. Any non-error interrupts are being treated as the transfer completion and then polling is being done for available/free bytes in FIFO. To fix all these issues, major code changes are required. This patch series fixes all the above issues and makes the driver interrupt based instead of polling based. After these changes, all the mentioned test cases are working properly. The code changes have been tested for QUP v1 (IPQ8064) and QUP v2 (IPQ8074) with sample application written over i2c-dev. Abhishek Sahu (13): i2c: qup: fix copyrights and update to SPDX identifier i2c: qup: fixed releasing dma without flush operation completion i2c: qup: minor code reorganization for use_dma i2c: qup: remove redundant variables for BAM SG count i2c: qup: schedule EOT and FLUSH tags at the end of transfer i2c: qup: fix the transfer length for BAM RX EOT FLUSH tags i2c: qup: proper error handling for i2c error in BAM mode i2c: qup: use the complete transfer length to choose DMA mode i2c: qup: change completion timeout according to transfer length i2c: qup: fix buffer overflow for multiple msg of maximum xfer len i2c: qup: send NACK for last read sub transfers i2c: qup: reorganization of driver code to remove polling for qup v1 i2c: qup: reorganization of driver code to remove polling for qup v2 drivers/i2c/busses/i2c-qup.c | 1507 -- 1 file changed, 880 insertions(+), 627 deletions(-) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
