| Message ID | 20201222184425.7028-7-p.yadav@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0920a32cf6f20467aa133a47b776ee782daa889f |
| Headers | show |
| Series | spi: cadence-quadspi: Add Octal DTR support | expand |
Hi Pratyush,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on spi/for-next]
[also build test WARNING on v5.11-rc1 next-20201223]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Pratyush-Yadav/spi-cadence-quadspi-Add-Octal-DTR-support/20201223-025328
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: arm-randconfig-r006-20201221 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/04a7bcbc449363e5d6f498376c69116567b49d7d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Pratyush-Yadav/spi-cadence-quadspi-Add-Octal-DTR-support/20201223-025328
git checkout 04a7bcbc449363e5d6f498376c69116567b49d7d
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/spi/spi-cadence-quadspi.c:966:24: warning: comparison of distinct pointer types ('typeof (len) *' (aka 'unsigned int *') and 'typeof (500UL) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
msecs_to_jiffies(max(len, 500UL)))) {
^~~~~~~~~~~~~~~
include/linux/minmax.h:58:19: note: expanded from macro 'max'
#define max(x, y) __careful_cmp(x, y, >)
^~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:42:24: note: expanded from macro '__careful_cmp'
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~~~~~~~
include/linux/minmax.h:32:4: note: expanded from macro '__safe_cmp'
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~~~~~~~
include/linux/minmax.h:18:28: note: expanded from macro '__typecheck'
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
1 warning generated.
vim +966 drivers/spi/spi-cadence-quadspi.c
919
920 static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
921 u_char *buf, loff_t from, size_t len)
922 {
923 struct cqspi_st *cqspi = f_pdata->cqspi;
924 struct device *dev = &cqspi->pdev->dev;
925 enum dma_ctrl_flags flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
926 dma_addr_t dma_src = (dma_addr_t)cqspi->mmap_phys_base + from;
927 int ret = 0;
928 struct dma_async_tx_descriptor *tx;
929 dma_cookie_t cookie;
930 dma_addr_t dma_dst;
931 struct device *ddev;
932
933 if (!cqspi->rx_chan || !virt_addr_valid(buf)) {
934 memcpy_fromio(buf, cqspi->ahb_base + from, len);
935 return 0;
936 }
937
938 ddev = cqspi->rx_chan->device->dev;
939 dma_dst = dma_map_single(ddev, buf, len, DMA_FROM_DEVICE);
940 if (dma_mapping_error(ddev, dma_dst)) {
941 dev_err(dev, "dma mapping failed\n");
942 return -ENOMEM;
943 }
944 tx = dmaengine_prep_dma_memcpy(cqspi->rx_chan, dma_dst, dma_src,
945 len, flags);
946 if (!tx) {
947 dev_err(dev, "device_prep_dma_memcpy error\n");
948 ret = -EIO;
949 goto err_unmap;
950 }
951
952 tx->callback = cqspi_rx_dma_callback;
953 tx->callback_param = cqspi;
954 cookie = tx->tx_submit(tx);
955 reinit_completion(&cqspi->rx_dma_complete);
956
957 ret = dma_submit_error(cookie);
958 if (ret) {
959 dev_err(dev, "dma_submit_error %d\n", cookie);
960 ret = -EIO;
961 goto err_unmap;
962 }
963
964 dma_async_issue_pending(cqspi->rx_chan);
965 if (!wait_for_completion_timeout(&cqspi->rx_dma_complete,
> 966 msecs_to_jiffies(max(len, 500UL)))) {
967 dmaengine_terminate_sync(cqspi->rx_chan);
968 dev_err(dev, "DMA wait_for_completion_timeout\n");
969 ret = -ETIMEDOUT;
970 goto err_unmap;
971 }
972
973 err_unmap:
974 dma_unmap_single(ddev, dma_dst, len, DMA_FROM_DEVICE);
975
976 return ret;
977 }
978
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 29/12/20 11:29AM, kernel test robot wrote: > Hi Pratyush, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on spi/for-next] > [also build test WARNING on v5.11-rc1 next-20201223] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Pratyush-Yadav/spi-cadence-quadspi-Add-Octal-DTR-support/20201223-025328 > base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next > config: arm-randconfig-r006-20201221 (attached as .config) > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install arm cross compiling tool for clang build > # apt-get install binutils-arm-linux-gnueabi > # https://github.com/0day-ci/linux/commit/04a7bcbc449363e5d6f498376c69116567b49d7d > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Pratyush-Yadav/spi-cadence-quadspi-Add-Octal-DTR-support/20201223-025328 > git checkout 04a7bcbc449363e5d6f498376c69116567b49d7d > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > >> drivers/spi/spi-cadence-quadspi.c:966:24: warning: comparison of distinct pointer types ('typeof (len) *' (aka 'unsigned int *') and 'typeof (500UL) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types] > msecs_to_jiffies(max(len, 500UL)))) { > ^~~~~~~~~~~~~~~ > include/linux/minmax.h:58:19: note: expanded from macro 'max' > #define max(x, y) __careful_cmp(x, y, >) > ^~~~~~~~~~~~~~~~~~~~~~ > include/linux/minmax.h:42:24: note: expanded from macro '__careful_cmp' > __builtin_choose_expr(__safe_cmp(x, y), \ > ^~~~~~~~~~~~~~~~ > include/linux/minmax.h:32:4: note: expanded from macro '__safe_cmp' > (__typecheck(x, y) && __no_side_effects(x, y)) > ^~~~~~~~~~~~~~~~~ > include/linux/minmax.h:18:28: note: expanded from macro '__typecheck' > (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) > ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~ > 1 warning generated. On arm64 size_t is defined as unsigned long and on arm is it defined as unsigned int. So using 500U would generate the same warning on 64-bit platforms. Maybe the fix is to do something like: max(len, (size_t)500). Any better ideas? > > > vim +966 drivers/spi/spi-cadence-quadspi.c > > 919 > 920 static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata, > 921 u_char *buf, loff_t from, size_t len) > 922 { > 923 struct cqspi_st *cqspi = f_pdata->cqspi; > 924 struct device *dev = &cqspi->pdev->dev; > 925 enum dma_ctrl_flags flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT; > 926 dma_addr_t dma_src = (dma_addr_t)cqspi->mmap_phys_base + from; > 927 int ret = 0; > 928 struct dma_async_tx_descriptor *tx; > 929 dma_cookie_t cookie; > 930 dma_addr_t dma_dst; > 931 struct device *ddev; > 932 > 933 if (!cqspi->rx_chan || !virt_addr_valid(buf)) { > 934 memcpy_fromio(buf, cqspi->ahb_base + from, len); > 935 return 0; > 936 } > 937 > 938 ddev = cqspi->rx_chan->device->dev; > 939 dma_dst = dma_map_single(ddev, buf, len, DMA_FROM_DEVICE); > 940 if (dma_mapping_error(ddev, dma_dst)) { > 941 dev_err(dev, "dma mapping failed\n"); > 942 return -ENOMEM; > 943 } > 944 tx = dmaengine_prep_dma_memcpy(cqspi->rx_chan, dma_dst, dma_src, > 945 len, flags); > 946 if (!tx) { > 947 dev_err(dev, "device_prep_dma_memcpy error\n"); > 948 ret = -EIO; > 949 goto err_unmap; > 950 } > 951 > 952 tx->callback = cqspi_rx_dma_callback; > 953 tx->callback_param = cqspi; > 954 cookie = tx->tx_submit(tx); > 955 reinit_completion(&cqspi->rx_dma_complete); > 956 > 957 ret = dma_submit_error(cookie); > 958 if (ret) { > 959 dev_err(dev, "dma_submit_error %d\n", cookie); > 960 ret = -EIO; > 961 goto err_unmap; > 962 } > 963 > 964 dma_async_issue_pending(cqspi->rx_chan); > 965 if (!wait_for_completion_timeout(&cqspi->rx_dma_complete, > > 966 msecs_to_jiffies(max(len, 500UL)))) { > 967 dmaengine_terminate_sync(cqspi->rx_chan); > 968 dev_err(dev, "DMA wait_for_completion_timeout\n"); > 969 ret = -ETIMEDOUT; > 970 goto err_unmap; > 971 } > 972 > 973 err_unmap: > 974 dma_unmap_single(ddev, dma_dst, len, DMA_FROM_DEVICE); > 975 > 976 return ret; > 977 } > 978 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Tue, Dec 29, 2020 at 1:18 AM 'Pratyush Yadav' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > On 29/12/20 11:29AM, kernel test robot wrote: > > Hi Pratyush, > > > > Thank you for the patch! Perhaps something to improve: > > > > [auto build test WARNING on spi/for-next] > > [also build test WARNING on v5.11-rc1 next-20201223] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch] > > > > url: https://github.com/0day-ci/linux/commits/Pratyush-Yadav/spi-cadence-quadspi-Add-Octal-DTR-support/20201223-025328 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next > > config: arm-randconfig-r006-20201221 (attached as .config) > > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45) > > reproduce (this is a W=1 build): > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # install arm cross compiling tool for clang build > > # apt-get install binutils-arm-linux-gnueabi > > # https://github.com/0day-ci/linux/commit/04a7bcbc449363e5d6f498376c69116567b49d7d > > git remote add linux-review https://github.com/0day-ci/linux > > git fetch --no-tags linux-review Pratyush-Yadav/spi-cadence-quadspi-Add-Octal-DTR-support/20201223-025328 > > git checkout 04a7bcbc449363e5d6f498376c69116567b49d7d > > # save the attached .config to linux build tree > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot <lkp@intel.com> > > > > All warnings (new ones prefixed by >>): > > > > >> drivers/spi/spi-cadence-quadspi.c:966:24: warning: comparison of distinct pointer types ('typeof (len) *' (aka 'unsigned int *') and 'typeof (500UL) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types] > > msecs_to_jiffies(max(len, 500UL)))) { > > ^~~~~~~~~~~~~~~ > > include/linux/minmax.h:58:19: note: expanded from macro 'max' > > #define max(x, y) __careful_cmp(x, y, >) > > ^~~~~~~~~~~~~~~~~~~~~~ > > include/linux/minmax.h:42:24: note: expanded from macro '__careful_cmp' > > __builtin_choose_expr(__safe_cmp(x, y), \ > > ^~~~~~~~~~~~~~~~ > > include/linux/minmax.h:32:4: note: expanded from macro '__safe_cmp' > > (__typecheck(x, y) && __no_side_effects(x, y)) > > ^~~~~~~~~~~~~~~~~ > > include/linux/minmax.h:18:28: note: expanded from macro '__typecheck' > > (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) > > ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~ > > 1 warning generated. > > On arm64 size_t is defined as unsigned long and on arm is it defined as > unsigned int. So using 500U would generate the same warning on 64-bit > platforms. Maybe the fix is to do something like: max(len, (size_t)500). > Any better ideas? SGTM
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index 1781d4e94ebd..90040664e1b9 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -963,7 +963,7 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata, dma_async_issue_pending(cqspi->rx_chan); if (!wait_for_completion_timeout(&cqspi->rx_dma_complete, - msecs_to_jiffies(len))) { + msecs_to_jiffies(max(len, 500UL)))) { dmaengine_terminate_sync(cqspi->rx_chan); dev_err(dev, "DMA wait_for_completion_timeout\n"); ret = -ETIMEDOUT;
When performing a direct read via DMA the timeout for completion is set equal to the read length. This is fine for larger reads. For a small read like the Read Status Register command, the timeout would be 1 or 2 milliseconds. This is not enough to cover the overhead needed in setting up DMA. Make sure the timeout is at least 500 ms to allow DMA ample time to finish. For reads larger than 500 bytes, the timeout will continue to be equal to the read length. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- drivers/spi/spi-cadence-quadspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)