| Message ID | 20210625052213.32260-5-vkoul@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add and enable GPI DMA users | expand |
Hi Vinod,
I love your patch! Perhaps something to improve:
[auto build test WARNING on spi/for-next]
[also build test WARNING on wsa/i2c/for-next asoc/for-next linus/master v5.13-rc7 next-20210624]
[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/Vinod-Koul/Add-and-enable-GPI-DMA-users/20210625-132320
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: x86_64-randconfig-a012-20210625 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9ca0171a9ffdef5fdb1511d197a3fd72490362de)
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 x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/3632ac4a40c3ef7f86f081d462bf17b45e4c0085
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Vinod-Koul/Add-and-enable-GPI-DMA-users/20210625-132320
git checkout 3632ac4a40c3ef7f86f081d462bf17b45e4c0085
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
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-geni-qcom.c:496:44: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout);
^
include/linux/dev_printk.h:112:32: note: expanded from macro 'dev_err'
_dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
^~~~~~~~~~~
drivers/spi/spi-geni-qcom.c:407:12: note: initialize the variable 'i' to silence this warning
int ret, i;
^
= 0
>> drivers/spi/spi-geni-qcom.c:579:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (IS_ERR_OR_NULL(mas->gsi))
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/spi/spi-geni-qcom.c:590:9: note: uninitialized use occurs here
return ret;
^~~
drivers/spi/spi-geni-qcom.c:579:2: note: remove the 'if' if its condition is always false
if (IS_ERR_OR_NULL(mas->gsi))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/spi/spi-geni-qcom.c:562:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
2 warnings generated.
vim +/i +496 drivers/spi/spi-geni-qcom.c
398
399 static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas,
400 struct spi_device *spi_slv, struct spi_master *spi)
401 {
402 unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
403 struct spi_geni_gsi *gsi;
404 struct dma_slave_config config = {};
405 struct gpi_spi_config peripheral = {};
406 unsigned long timeout, jiffies;
407 int ret, i;
408
409 config.peripheral_config = &peripheral;
410 config.peripheral_size = sizeof(peripheral);
411 peripheral.set_config = true;
412
413 if (xfer->bits_per_word != mas->cur_bits_per_word ||
414 xfer->speed_hz != mas->cur_speed_hz) {
415 mas->cur_bits_per_word = xfer->bits_per_word;
416 mas->cur_speed_hz = xfer->speed_hz;
417 }
418
419 if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
420 peripheral.rx_len = ((xfer->len << 3) / mas->cur_bits_per_word);
421 } else {
422 int bytes_per_word = (mas->cur_bits_per_word / BITS_PER_BYTE) + 1;
423
424 peripheral.rx_len = (xfer->len / bytes_per_word);
425 }
426
427 if (xfer->tx_buf && xfer->rx_buf) {
428 peripheral.cmd = SPI_DUPLEX;
429 } else if (xfer->tx_buf) {
430 peripheral.cmd = SPI_TX;
431 peripheral.rx_len = 0;
432 } else if (xfer->rx_buf) {
433 peripheral.cmd = SPI_RX;
434 }
435
436 if (spi_slv->mode & SPI_LOOP)
437 peripheral.loopback_en = true;
438 if (spi_slv->mode & SPI_CPOL)
439 peripheral.clock_pol_high = true;
440 if (spi_slv->mode & SPI_CPHA)
441 peripheral.data_pol_high = true;
442
443 peripheral.cs = spi_slv->chip_select;
444 peripheral.pack_en = true;
445 peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN;
446 peripheral.fragmentation = FRAGMENTATION;
447
448 ret = get_spi_clk_cfg(mas->cur_speed_hz, mas,
449 &peripheral.clk_src, &peripheral.clk_div);
450 if (ret) {
451 dev_err(mas->dev, "Err in get_spi_clk_cfg() :%d\n", ret);
452 return ret;
453 }
454
455 gsi = &mas->gsi[mas->num_xfers];
456 gsi->rx_cb.mas = mas;
457 gsi->rx_cb.xfer = xfer;
458
459 if (peripheral.cmd & SPI_RX) {
460 dmaengine_slave_config(mas->rx, &config);
461 gsi->rx_desc = dmaengine_prep_slave_sg(mas->rx, xfer->rx_sg.sgl, xfer->rx_sg.nents,
462 DMA_DEV_TO_MEM, flags);
463 if (!gsi->rx_desc) {
464 dev_err(mas->dev, "Err setting up rx desc\n");
465 return -EIO;
466 }
467 gsi->rx_desc->callback_result = spi_gsi_rx_callback_result;
468 gsi->rx_desc->callback_param = &gsi->rx_cb;
469 }
470
471 dmaengine_slave_config(mas->tx, &config);
472 gsi->tx_desc = dmaengine_prep_slave_sg(mas->tx, xfer->tx_sg.sgl, xfer->tx_sg.nents,
473 DMA_MEM_TO_DEV, flags);
474 if (!gsi->tx_desc) {
475 dev_err(mas->dev, "Err setting up tx desc\n");
476 return -EIO;
477 }
478
479 gsi->tx_cb.mas = mas;
480 gsi->tx_cb.xfer = xfer;
481 gsi->tx_desc->callback_result = spi_gsi_tx_callback_result;
482 gsi->tx_desc->callback_param = &gsi->tx_cb;
483
484 if (peripheral.cmd & SPI_RX)
485 gsi->rx_cookie = dmaengine_submit(gsi->rx_desc);
486 gsi->tx_cookie = dmaengine_submit(gsi->tx_desc);
487
488 if (peripheral.cmd & SPI_RX)
489 dma_async_issue_pending(mas->rx);
490 dma_async_issue_pending(mas->tx);
491 mas->num_xfers++;
492
493 jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
494 timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies);
495 if (timeout <= 0) {
> 496 dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout);
497 ret = -ETIMEDOUT;
498 goto err_gsi_geni_transfer_one;
499 }
500
501 if (peripheral.cmd & SPI_RX) {
502 jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
503 timeout = wait_for_completion_timeout(&mas->rx_cb, jiffies);
504 if (timeout <= 0) {
505 dev_err(mas->dev, "Rx[%d] timeout%lu\n", i, timeout);
506 ret = -ETIMEDOUT;
507 goto err_gsi_geni_transfer_one;
508 }
509 }
510
511 spi_finalize_current_transfer(spi);
512 return 0;
513
514 err_gsi_geni_transfer_one:
515 dmaengine_terminate_all(mas->tx);
516 return ret;
517 }
518
519 static bool geni_can_dma(struct spi_controller *ctlr,
520 struct spi_device *slv, struct spi_transfer *xfer)
521 {
522 struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
523
524 /* check if dma is supported */
525 if (mas->cur_xfer_mode == GENI_GPI_DMA)
526 return true;
527
528 return false;
529 }
530
531 static int spi_geni_prepare_message(struct spi_master *spi,
532 struct spi_message *spi_msg)
533 {
534 struct spi_geni_master *mas = spi_master_get_devdata(spi);
535 int ret;
536
537 switch (mas->cur_xfer_mode) {
538 case GENI_SE_FIFO:
539 if (spi_geni_is_abort_still_pending(mas))
540 return -EBUSY;
541 ret = setup_fifo_params(spi_msg->spi, spi);
542 if (ret)
543 dev_err(mas->dev, "Couldn't select mode %d\n", ret);
544 return ret;
545
546 case GENI_GPI_DMA:
547 mas->num_xfers = 0;
548 reinit_completion(&mas->tx_cb);
549 reinit_completion(&mas->rx_cb);
550 memset(mas->gsi, 0, (sizeof(struct spi_geni_gsi) * NUM_SPI_XFER));
551
552 return 0;
553 }
554
555 dev_err(mas->dev, "Mode not supported %d", mas->cur_xfer_mode);
556 return -EINVAL;
557 }
558
559 static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
560 {
561 size_t gsi_sz;
562 int ret;
563
564 mas->tx = dma_request_chan(mas->dev, "tx");
565 if (IS_ERR_OR_NULL(mas->tx)) {
566 dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx));
567 ret = PTR_ERR(mas->tx);
568 goto err_tx;
569 }
570 mas->rx = dma_request_chan(mas->dev, "rx");
571 if (IS_ERR_OR_NULL(mas->rx)) {
572 dev_err(mas->dev, "Failed to get rx DMA ch %ld", PTR_ERR(mas->rx));
573 ret = PTR_ERR(mas->rx);
574 goto err_rx;
575 }
576
577 gsi_sz = sizeof(struct spi_geni_gsi) * NUM_SPI_XFER;
578 mas->gsi = devm_kzalloc(mas->dev, gsi_sz, GFP_KERNEL);
> 579 if (IS_ERR_OR_NULL(mas->gsi))
580 goto err_mem;
581 return 0;
582
583 err_mem:
584 dma_release_channel(mas->rx);
585 err_rx:
586 dma_release_channel(mas->tx);
587 err_tx:
588 mas->tx = NULL;
589 mas->rx = NULL;
590 return ret;
591 }
592
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi, On Thu, Jun 24, 2021 at 10:22 PM Vinod Koul <vkoul@kernel.org> wrote: > > We can use GPI DMA for devices where it is enabled by firmware. Add > support for this mode > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/spi/spi-geni-qcom.c | 329 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 315 insertions(+), 14 deletions(-) Not a truly full review since I haven't looked into the whole GSI DMA driver, but I tried to go over most of your changes. > +#define GSI_LOOPBACK_EN (BIT(0)) > +#define GSI_CS_TOGGLE (BIT(3)) > +#define GSI_CPHA (BIT(4)) > +#define GSI_CPOL (BIT(5)) > + > +#define MAX_TX_SG (3) > +#define NUM_SPI_XFER (8) > +#define SPI_XFER_TIMEOUT_MS (250) nit: don't need the extra parenthesis around everything above and it doesn't match the existing definitions too. > +struct gsi_desc_cb { > + struct spi_geni_master *mas; > + struct spi_transfer *xfer; > +}; > + > +struct spi_geni_gsi { > + dma_cookie_t tx_cookie; > + dma_cookie_t rx_cookie; > + struct dma_async_tx_descriptor *tx_desc; > + struct dma_async_tx_descriptor *rx_desc; > + struct gsi_desc_cb tx_cb; > + struct gsi_desc_cb rx_cb; > +}; I'd be curious if others feel the same way, but to me it kinda feels like this should be: struct spi_geni_gsi_pipe { dma_cookie_t cookie; struct dma_async_tx_descriptor *desc; struct gsi_desc_cb cb; }; struct spi_geni_gsi { spi_geni_gsi_pipe tx; spi_geni_gsi_pipe rx; }; That would actually make spi_gsi_callback_result() more elegant because you could pass in the correct structure and then a string "RX" or "TX" and get rid of the bool. > @@ -84,6 +110,13 @@ struct spi_geni_master { > int irq; > bool cs_flag; > bool abort_failed; > + struct spi_geni_gsi *gsi; Maybe do something to make it obvious that this points to an array of NUM_SPI_XFER transfers. If my math is right then "gsi" is expected to be about 256 bytes big. I guess you made this a pointer and only allocated it in GSI mode to save those 256 bytes in non-GSI mode? > +static void > +spi_gsi_callback_result(void *cb, const struct dmaengine_result *result, bool tx) > +{ > + struct gsi_desc_cb *gsi = cb; > + > + if (result->result != DMA_TRANS_NOERROR) {id'd do > + dev_err(gsi->mas->dev, "%s DMA txn failed\n", tx ? "TX" : "RX"); > + return; > + } > + > + if (!result->residue) { > + if (tx) > + complete(&gsi->mas->tx_cb); > + else > + complete(&gsi->mas->rx_cb); > + } else { > + dev_err(gsi->mas->dev, "%s DMA txn has pending %d data\n", > + tx ? "TX" : "RX", result->residue); I guess you just wait for the timeout in this case since you don't complete? > +static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas, > + struct spi_device *spi_slv, struct spi_master *spi) > +{ > + unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; > + struct spi_geni_gsi *gsi; > + struct dma_slave_config config = {}; > + struct gpi_spi_config peripheral = {}; > + unsigned long timeout, jiffies; Is it really a good idea to have a local variable called "jiffies"? Isn't the global called "jiffies"? > + int ret, i; > + > + config.peripheral_config = &peripheral; > + config.peripheral_size = sizeof(peripheral); > + peripheral.set_config = true; > + > + if (xfer->bits_per_word != mas->cur_bits_per_word || > + xfer->speed_hz != mas->cur_speed_hz) { > + mas->cur_bits_per_word = xfer->bits_per_word; > + mas->cur_speed_hz = xfer->speed_hz; > + } It seems a bit pointless to copy these things into "mas" for GSI mode and even more pointless to have the "if" test first. In FIFO mode I believe we stored them in "mas" because we needed them from the interrupt handler. It also lets us optimize and avoid calls if the config didn't change. You don't need them for either of these reasons, do you? Actually, you might possibly want to keep "cur_speed_hz" and use it to avoid a call to get_spi_clk_cfg() which, at least at one point in time, was a bit slow. > + if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) { > + peripheral.rx_len = ((xfer->len << 3) / mas->cur_bits_per_word); > + } else { > + int bytes_per_word = (mas->cur_bits_per_word / BITS_PER_BYTE) + 1; > + > + peripheral.rx_len = (xfer->len / bytes_per_word); > + } I guess you could only do the above if "rx_buf" and then you don't have to zero it back out below. > + if (xfer->tx_buf && xfer->rx_buf) { > + peripheral.cmd = SPI_DUPLEX; > + } else if (xfer->tx_buf) { > + peripheral.cmd = SPI_TX; > + peripheral.rx_len = 0; > + } else if (xfer->rx_buf) { > + peripheral.cmd = SPI_RX; > + } > + > + if (spi_slv->mode & SPI_LOOP) > + peripheral.loopback_en = true; > + if (spi_slv->mode & SPI_CPOL) > + peripheral.clock_pol_high = true; > + if (spi_slv->mode & SPI_CPHA) > + peripheral.data_pol_high = true; nit: I'd do it without the "if"s. peripheral.loopback_en = spi_slv->mode & SPI_LOOP; In theory much of this could also be done at function init time but I dunno if it makes a huge difference, like: struct gpi_spi_config peripheral = { .loopback_en = spi_slv->mode & SPI_LOOP, .clock_pol_high = spi_slv->mode & SPI_CPOL, ... }; > + peripheral.cs = spi_slv->chip_select; > + peripheral.pack_en = true; > + peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN; > + peripheral.fragmentation = FRAGMENTATION; > + > + ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, > + &peripheral.clk_src, &peripheral.clk_div); > + if (ret) { > + dev_err(mas->dev, "Err in get_spi_clk_cfg() :%d\n", ret); > + return ret; > + } > + > + gsi = &mas->gsi[mas->num_xfers]; It sure feels like you should error-check this against NUM_SPI_XFER. Otherwise if someone does a transfer with more than 8 parts you're totally hosed, right? Actually, why do you even need an array here? It seems like by the time this function exits the transfer will be all done, right? So you can just keep using the same structure over and over again? It can even be on the stack? > + gsi->rx_cb.mas = mas; > + gsi->rx_cb.xfer = xfer; > + > + if (peripheral.cmd & SPI_RX) { > + dmaengine_slave_config(mas->rx, &config); > + gsi->rx_desc = dmaengine_prep_slave_sg(mas->rx, xfer->rx_sg.sgl, xfer->rx_sg.nents, > + DMA_DEV_TO_MEM, flags); > + if (!gsi->rx_desc) { > + dev_err(mas->dev, "Err setting up rx desc\n"); > + return -EIO; > + } > + gsi->rx_desc->callback_result = spi_gsi_rx_callback_result; > + gsi->rx_desc->callback_param = &gsi->rx_cb; > + } > + > + dmaengine_slave_config(mas->tx, &config); > + gsi->tx_desc = dmaengine_prep_slave_sg(mas->tx, xfer->tx_sg.sgl, xfer->tx_sg.nents, > + DMA_MEM_TO_DEV, flags); > + if (!gsi->tx_desc) { > + dev_err(mas->dev, "Err setting up tx desc\n"); > + return -EIO; > + } I haven't dug deeply, but it surprises me that we do all the TX config even if "xfer->tx_buf" is NULL. > + gsi->tx_cb.mas = mas; > + gsi->tx_cb.xfer = xfer; > + gsi->tx_desc->callback_result = spi_gsi_tx_callback_result; > + gsi->tx_desc->callback_param = &gsi->tx_cb; > + > + if (peripheral.cmd & SPI_RX) > + gsi->rx_cookie = dmaengine_submit(gsi->rx_desc); > + gsi->tx_cookie = dmaengine_submit(gsi->tx_desc); > + > + if (peripheral.cmd & SPI_RX) > + dma_async_issue_pending(mas->rx); > + dma_async_issue_pending(mas->tx); > + mas->num_xfers++; > + > + jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS); > + timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies); You're waiting a hardcoded 250 ms for every transfer. That could be too short for a long transfer. Can you use some logic like the SPI framework? It's probably also worth adding a comment about why you can't just return "1" from transfer_one() for GSI mode and let the SPI framework handle the timeout calculations. My guess is that you might need extra time because another processor might hold the SPI bus and keep you from starting your transfer right away and thus your timeout needs to be somehow longer than the normal SPI framework. Is that true? Actually, it looks like spi_transfer_wait() already has 200 ms of slop. Is that enough for you? Can you just let the SPI framework handle the timeout? > + if (timeout <= 0) { Just "== 0". "timeout" is unsigned, right? > + dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout); timeout is always 0 in this printout. > + ret = -ETIMEDOUT; > + goto err_gsi_geni_transfer_one; > + } > + > + if (peripheral.cmd & SPI_RX) { > + jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS); > + timeout = wait_for_completion_timeout(&mas->rx_cb, jiffies); Why do you need to re-init the timeout? You should just wait for however much is left from the previous wait? The TX and RX should be happening in parallel, right? So you don't want to wait for a full second transfer. > + if (timeout <= 0) { > + dev_err(mas->dev, "Rx[%d] timeout%lu\n", i, timeout); > + ret = -ETIMEDOUT; > + goto err_gsi_geni_transfer_one; > + } > + } > + > + spi_finalize_current_transfer(spi); > + return 0; > + > +err_gsi_geni_transfer_one: > + dmaengine_terminate_all(mas->tx); I know not what I'm talking about, but the description of dmaengine_terminate_all says "This function is DEPRECATED". It also seems like the old "terminate_all" is async? That's bad because you gave it stack pointers... You almost certainly want a WARN_ON or something like that if you fail to terminate the transfer because, presumably, the DMA engine will keep looking at your config that's stored on the stack (that you're freeing by ending the function). That's super bad and I'd want a pretty serious warning in my logs if it might be happening. > +static bool geni_can_dma(struct spi_controller *ctlr, > + struct spi_device *slv, struct spi_transfer *xfer) > +{ > + struct spi_geni_master *mas = spi_master_get_devdata(slv->master); > + > + /* check if dma is supported */ > + if (mas->cur_xfer_mode == GENI_GPI_DMA) > + return true; > + > + return false; > +} nit: might as well handle GENI_SE_DMA as well since it's just as easy to test against GENI_SE_FIFO? nit: no need for an if. So just: return mas->cur_xfer_mode != GENI_SE_FIFO; > static int spi_geni_prepare_message(struct spi_master *spi, > struct spi_message *spi_msg) > { > - int ret; > struct spi_geni_master *mas = spi_master_get_devdata(spi); > + int ret; > > - if (spi_geni_is_abort_still_pending(mas)) > - return -EBUSY; > + switch (mas->cur_xfer_mode) { > + case GENI_SE_FIFO: > + if (spi_geni_is_abort_still_pending(mas)) > + return -EBUSY; > + ret = setup_fifo_params(spi_msg->spi, spi); > + if (ret) > + dev_err(mas->dev, "Couldn't select mode %d\n", ret); > + return ret; > > - ret = setup_fifo_params(spi_msg->spi, spi); > - if (ret) > - dev_err(mas->dev, "Couldn't select mode %d\n", ret); > + case GENI_GPI_DMA: > + mas->num_xfers = 0; > + reinit_completion(&mas->tx_cb); > + reinit_completion(&mas->rx_cb); > + memset(mas->gsi, 0, (sizeof(struct spi_geni_gsi) * NUM_SPI_XFER)); nit: make a #define for the size and use it here and in allocation? > +static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas) > +{ > + size_t gsi_sz; > + int ret; > + > + mas->tx = dma_request_chan(mas->dev, "tx"); > + if (IS_ERR_OR_NULL(mas->tx)) { I don't think dma_request_chan() can return NULL. > + dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx)); > + ret = PTR_ERR(mas->tx); niftier: ret = dev_err_probe(mas->dev, PTR_ERR(mas->tx), "Failed to get tx DMA ch\n"); If you don't think that's nifty, at least you should add a "\n" to your error message. > + goto err_tx; > + } Personally I'm a fan of devm_add_action_or_reset() to simplify error handling like this. Everything becomes easier to handle then. ret = devm_add_action_or_reset(mas->dev, spi_geni_dma_release_chan, mas->tx); if (ret) return ret; You've got to make a tiny spi_geni_dma_release_chan() function that just wraps dma_release_chan() (or cast and violate the C standard) but then you get rid of all error handling in this function, get rid of the code you had to add to remove and the code you forgot to add the the probe() function (see below). :-) > + mas->rx = dma_request_chan(mas->dev, "rx"); > + if (IS_ERR_OR_NULL(mas->rx)) { > + dev_err(mas->dev, "Failed to get rx DMA ch %ld", PTR_ERR(mas->rx)); > + ret = PTR_ERR(mas->rx); > + goto err_rx; > + } > + > + gsi_sz = sizeof(struct spi_geni_gsi) * NUM_SPI_XFER; > + mas->gsi = devm_kzalloc(mas->dev, gsi_sz, GFP_KERNEL); > + if (IS_ERR_OR_NULL(mas->gsi)) > + goto err_mem; This is not correct. kzalloc() never returns errors, just NULL. ...and you don't set "ret". Also: since you re-use this over and over and zero it before each transfer, you could probably avoid the zero-allocation. > @@ -349,15 +594,15 @@ static int spi_geni_init(struct spi_geni_master *mas) > { > struct geni_se *se = &mas->se; > unsigned int proto, major, minor, ver; > - u32 spi_tx_cfg; > + u32 spi_tx_cfg, fifo_disable; > + int ret = -ENXIO; > > pm_runtime_get_sync(mas->dev); > > proto = geni_se_read_proto(se); > if (proto != GENI_SE_SPI) { > dev_err(mas->dev, "Invalid proto %d\n", proto); > - pm_runtime_put(mas->dev); > - return -ENXIO; > + goto out_pm; nit: In theory this change could be relegated to a tiny cleanup patch just to make your GPI change smaller to review, but I won't insist. > @@ -380,15 +625,38 @@ static int spi_geni_init(struct spi_geni_master *mas) > else > mas->oversampling = 1; > > - geni_se_select_mode(se, GENI_SE_FIFO); > + fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE; > + switch (fifo_disable) { > + case 1: > + ret = spi_geni_grab_gpi_chan(mas); > + if (!ret) { /* success case */ > + mas->cur_xfer_mode = GENI_GPI_DMA; > + geni_se_select_mode(se, GENI_GPI_DMA); > + dev_dbg(mas->dev, "Using GPI DMA mode for SPI\n"); > + break; > + } > + /* > + * in case of failure to get dma channel, we can till do the > + * FIFO mode, so fallthrough s/till/still/ > + */ > + dev_warn(mas->dev, "FIFO mode disabled, but couldn't get DMA, fall back to FIFO mode\n"); > + fallthrough; I was under the impression that if `FIFO_IF_DISABLE` was set that the FIFO was, how shall we say it, "disabled". ;-) If you can in fact fall back to FIFO mode then that bit seems pretty poorly named. > static irqreturn_t geni_spi_isr(int irq, void *data) > @@ -671,6 +942,15 @@ static int spi_geni_probe(struct platform_device *pdev) > if (irq < 0) > return irq; > > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); nit: this function already has the line: struct device *dev = &pdev->dev; ...so you can use "dev". :-) > + if (ret) { > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > + if (ret) { > + dev_err(&pdev->dev, "could not set DMA mask\n"); > + return ret; niftier: return dev_err_probe(dev, ret, "could not set DMA mask\n"); > @@ -710,14 +990,17 @@ static int spi_geni_probe(struct platform_device *pdev) > spi->max_speed_hz = 50000000; > spi->prepare_message = spi_geni_prepare_message; > spi->transfer_one = spi_geni_transfer_one; > + spi->can_dma = geni_can_dma; > + spi->dma_map_dev = mas->dev->parent; mas->dev->parent == dev->parent ? > @@ -738,6 +1021,14 @@ static int spi_geni_probe(struct platform_device *pdev) > if (ret) > goto spi_geni_probe_runtime_disable; > > + /* > + * check the mode supported and set_cs for fifo mode only > + * for dma (gsi) mode, the gsi will set cs based on params passed in > + * TRE > + */ > + if (mas->cur_xfer_mode == GENI_SE_FIFO) > + spi->set_cs = spi_geni_set_cs; I'm curious: is there no way to get set_cs() working in GPI mode? In an off-thread conversation Qualcomm seemed to indicate that it was possible, but maybe they didn't quite understand what I was asking. Without an implementation of set_cs() there will be drivers in Linux that simply aren't compatible because they make the assumption that they can lock the bus, set the CS, and do several transfers that are part of some logical "transaction". I believe that both of the SPI peripherals on boards that I work on, cros-ec and the SPI TPM make this assumption. I don't even believe that the drivers can be "fixed" because the requirement is more at the protocol level. The protocol requires you to do things like: 0. Lock the bus. 1. Set the CS. 2. Transfer a few bytes, reading the response as you go. 3. Once you see the other side respond that it's ready, transfer some more bytes. 4. Release the CS. 5. Unlock the bus. You can't do this without a set_cs() implementation because of the requirement to read the responses of the other side before moving on to the next phase of the transfer. As I understand it this is roughly the equivalent of i2c clock stretching but much more ad-hoc and defined peripherals-by-peripheral. In any case, I guess you must have examples of peripherals that need GPI mode and don't need set_cs() so we shouldn't block your way forward, but I'm just curious if you had more info on this. > static int spi_geni_remove(struct platform_device *pdev) > { > struct spi_master *spi = platform_get_drvdata(pdev); > @@ -762,6 +1061,8 @@ static int spi_geni_remove(struct platform_device *pdev) > /* Unregister _before_ disabling pm_runtime() so we stop transfers */ > spi_unregister_master(spi); > > + spi_geni_release_dma_chan(mas); > + Why isn't there a call to spi_geni_release_dma_chan() in the error paths for probe? -Doug
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index 3d0d8ddd5772..c64355c246be 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -2,6 +2,9 @@ // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. #include <linux/clk.h> +#include <linux/dmaengine.h> +#include <linux/dma-mapping.h> +#include <linux/dma/qcom-gpi-dma.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/log2.h> @@ -63,6 +66,29 @@ #define TIMESTAMP_AFTER BIT(3) #define POST_CMD_DELAY BIT(4) +#define GSI_LOOPBACK_EN (BIT(0)) +#define GSI_CS_TOGGLE (BIT(3)) +#define GSI_CPHA (BIT(4)) +#define GSI_CPOL (BIT(5)) + +#define MAX_TX_SG (3) +#define NUM_SPI_XFER (8) +#define SPI_XFER_TIMEOUT_MS (250) + +struct gsi_desc_cb { + struct spi_geni_master *mas; + struct spi_transfer *xfer; +}; + +struct spi_geni_gsi { + dma_cookie_t tx_cookie; + dma_cookie_t rx_cookie; + struct dma_async_tx_descriptor *tx_desc; + struct dma_async_tx_descriptor *rx_desc; + struct gsi_desc_cb tx_cb; + struct gsi_desc_cb rx_cb; +}; + struct spi_geni_master { struct geni_se se; struct device *dev; @@ -84,6 +110,13 @@ struct spi_geni_master { int irq; bool cs_flag; bool abort_failed; + struct spi_geni_gsi *gsi; + struct dma_chan *tx; + struct dma_chan *rx; + struct completion tx_cb; + struct completion rx_cb; + int cur_xfer_mode; + int num_xfers; }; static int get_spi_clk_cfg(unsigned int speed_hz, @@ -330,18 +363,230 @@ static int setup_fifo_params(struct spi_device *spi_slv, return geni_spi_set_clock_and_bw(mas, spi_slv->max_speed_hz); } +static void +spi_gsi_callback_result(void *cb, const struct dmaengine_result *result, bool tx) +{ + struct gsi_desc_cb *gsi = cb; + + if (result->result != DMA_TRANS_NOERROR) { + dev_err(gsi->mas->dev, "%s DMA txn failed\n", tx ? "TX" : "RX"); + return; + } + + if (!result->residue) { + if (tx) + complete(&gsi->mas->tx_cb); + else + complete(&gsi->mas->rx_cb); + } else { + dev_err(gsi->mas->dev, "%s DMA txn has pending %d data\n", + tx ? "TX" : "RX", result->residue); + } +} + +static void +spi_gsi_rx_callback_result(void *cb, const struct dmaengine_result *result) +{ + spi_gsi_callback_result(cb, result, false); +} + +static void +spi_gsi_tx_callback_result(void *cb, const struct dmaengine_result *result) +{ + spi_gsi_callback_result(cb, result, true); +} + +static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas, + struct spi_device *spi_slv, struct spi_master *spi) +{ + unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; + struct spi_geni_gsi *gsi; + struct dma_slave_config config = {}; + struct gpi_spi_config peripheral = {}; + unsigned long timeout, jiffies; + int ret, i; + + config.peripheral_config = &peripheral; + config.peripheral_size = sizeof(peripheral); + peripheral.set_config = true; + + if (xfer->bits_per_word != mas->cur_bits_per_word || + xfer->speed_hz != mas->cur_speed_hz) { + mas->cur_bits_per_word = xfer->bits_per_word; + mas->cur_speed_hz = xfer->speed_hz; + } + + if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) { + peripheral.rx_len = ((xfer->len << 3) / mas->cur_bits_per_word); + } else { + int bytes_per_word = (mas->cur_bits_per_word / BITS_PER_BYTE) + 1; + + peripheral.rx_len = (xfer->len / bytes_per_word); + } + + if (xfer->tx_buf && xfer->rx_buf) { + peripheral.cmd = SPI_DUPLEX; + } else if (xfer->tx_buf) { + peripheral.cmd = SPI_TX; + peripheral.rx_len = 0; + } else if (xfer->rx_buf) { + peripheral.cmd = SPI_RX; + } + + if (spi_slv->mode & SPI_LOOP) + peripheral.loopback_en = true; + if (spi_slv->mode & SPI_CPOL) + peripheral.clock_pol_high = true; + if (spi_slv->mode & SPI_CPHA) + peripheral.data_pol_high = true; + + peripheral.cs = spi_slv->chip_select; + peripheral.pack_en = true; + peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN; + peripheral.fragmentation = FRAGMENTATION; + + ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, + &peripheral.clk_src, &peripheral.clk_div); + if (ret) { + dev_err(mas->dev, "Err in get_spi_clk_cfg() :%d\n", ret); + return ret; + } + + gsi = &mas->gsi[mas->num_xfers]; + gsi->rx_cb.mas = mas; + gsi->rx_cb.xfer = xfer; + + if (peripheral.cmd & SPI_RX) { + dmaengine_slave_config(mas->rx, &config); + gsi->rx_desc = dmaengine_prep_slave_sg(mas->rx, xfer->rx_sg.sgl, xfer->rx_sg.nents, + DMA_DEV_TO_MEM, flags); + if (!gsi->rx_desc) { + dev_err(mas->dev, "Err setting up rx desc\n"); + return -EIO; + } + gsi->rx_desc->callback_result = spi_gsi_rx_callback_result; + gsi->rx_desc->callback_param = &gsi->rx_cb; + } + + dmaengine_slave_config(mas->tx, &config); + gsi->tx_desc = dmaengine_prep_slave_sg(mas->tx, xfer->tx_sg.sgl, xfer->tx_sg.nents, + DMA_MEM_TO_DEV, flags); + if (!gsi->tx_desc) { + dev_err(mas->dev, "Err setting up tx desc\n"); + return -EIO; + } + + gsi->tx_cb.mas = mas; + gsi->tx_cb.xfer = xfer; + gsi->tx_desc->callback_result = spi_gsi_tx_callback_result; + gsi->tx_desc->callback_param = &gsi->tx_cb; + + if (peripheral.cmd & SPI_RX) + gsi->rx_cookie = dmaengine_submit(gsi->rx_desc); + gsi->tx_cookie = dmaengine_submit(gsi->tx_desc); + + if (peripheral.cmd & SPI_RX) + dma_async_issue_pending(mas->rx); + dma_async_issue_pending(mas->tx); + mas->num_xfers++; + + jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS); + timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies); + if (timeout <= 0) { + dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout); + ret = -ETIMEDOUT; + goto err_gsi_geni_transfer_one; + } + + if (peripheral.cmd & SPI_RX) { + jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS); + timeout = wait_for_completion_timeout(&mas->rx_cb, jiffies); + if (timeout <= 0) { + dev_err(mas->dev, "Rx[%d] timeout%lu\n", i, timeout); + ret = -ETIMEDOUT; + goto err_gsi_geni_transfer_one; + } + } + + spi_finalize_current_transfer(spi); + return 0; + +err_gsi_geni_transfer_one: + dmaengine_terminate_all(mas->tx); + return ret; +} + +static bool geni_can_dma(struct spi_controller *ctlr, + struct spi_device *slv, struct spi_transfer *xfer) +{ + struct spi_geni_master *mas = spi_master_get_devdata(slv->master); + + /* check if dma is supported */ + if (mas->cur_xfer_mode == GENI_GPI_DMA) + return true; + + return false; +} + static int spi_geni_prepare_message(struct spi_master *spi, struct spi_message *spi_msg) { - int ret; struct spi_geni_master *mas = spi_master_get_devdata(spi); + int ret; - if (spi_geni_is_abort_still_pending(mas)) - return -EBUSY; + switch (mas->cur_xfer_mode) { + case GENI_SE_FIFO: + if (spi_geni_is_abort_still_pending(mas)) + return -EBUSY; + ret = setup_fifo_params(spi_msg->spi, spi); + if (ret) + dev_err(mas->dev, "Couldn't select mode %d\n", ret); + return ret; - ret = setup_fifo_params(spi_msg->spi, spi); - if (ret) - dev_err(mas->dev, "Couldn't select mode %d\n", ret); + case GENI_GPI_DMA: + mas->num_xfers = 0; + reinit_completion(&mas->tx_cb); + reinit_completion(&mas->rx_cb); + memset(mas->gsi, 0, (sizeof(struct spi_geni_gsi) * NUM_SPI_XFER)); + + return 0; + } + + dev_err(mas->dev, "Mode not supported %d", mas->cur_xfer_mode); + return -EINVAL; +} + +static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas) +{ + size_t gsi_sz; + int ret; + + mas->tx = dma_request_chan(mas->dev, "tx"); + if (IS_ERR_OR_NULL(mas->tx)) { + dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx)); + ret = PTR_ERR(mas->tx); + goto err_tx; + } + mas->rx = dma_request_chan(mas->dev, "rx"); + if (IS_ERR_OR_NULL(mas->rx)) { + dev_err(mas->dev, "Failed to get rx DMA ch %ld", PTR_ERR(mas->rx)); + ret = PTR_ERR(mas->rx); + goto err_rx; + } + + gsi_sz = sizeof(struct spi_geni_gsi) * NUM_SPI_XFER; + mas->gsi = devm_kzalloc(mas->dev, gsi_sz, GFP_KERNEL); + if (IS_ERR_OR_NULL(mas->gsi)) + goto err_mem; + return 0; + +err_mem: + dma_release_channel(mas->rx); +err_rx: + dma_release_channel(mas->tx); +err_tx: + mas->tx = NULL; + mas->rx = NULL; return ret; } @@ -349,15 +594,15 @@ static int spi_geni_init(struct spi_geni_master *mas) { struct geni_se *se = &mas->se; unsigned int proto, major, minor, ver; - u32 spi_tx_cfg; + u32 spi_tx_cfg, fifo_disable; + int ret = -ENXIO; pm_runtime_get_sync(mas->dev); proto = geni_se_read_proto(se); if (proto != GENI_SE_SPI) { dev_err(mas->dev, "Invalid proto %d\n", proto); - pm_runtime_put(mas->dev); - return -ENXIO; + goto out_pm; } mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se); @@ -380,15 +625,38 @@ static int spi_geni_init(struct spi_geni_master *mas) else mas->oversampling = 1; - geni_se_select_mode(se, GENI_SE_FIFO); + fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE; + switch (fifo_disable) { + case 1: + ret = spi_geni_grab_gpi_chan(mas); + if (!ret) { /* success case */ + mas->cur_xfer_mode = GENI_GPI_DMA; + geni_se_select_mode(se, GENI_GPI_DMA); + dev_dbg(mas->dev, "Using GPI DMA mode for SPI\n"); + break; + } + /* + * in case of failure to get dma channel, we can till do the + * FIFO mode, so fallthrough + */ + dev_warn(mas->dev, "FIFO mode disabled, but couldn't get DMA, fall back to FIFO mode\n"); + fallthrough; + + case 0: + mas->cur_xfer_mode = GENI_SE_FIFO; + geni_se_select_mode(se, GENI_SE_FIFO); + ret = 0; + break; + } /* We always control CS manually */ spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG); spi_tx_cfg &= ~CS_TOGGLE; writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG); +out_pm: pm_runtime_put(mas->dev); - return 0; + return ret; } static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas) @@ -575,8 +843,11 @@ static int spi_geni_transfer_one(struct spi_master *spi, if (!xfer->len) return 0; - setup_fifo_xfer(xfer, mas, slv->mode, spi); - return 1; + if (mas->cur_xfer_mode == GENI_SE_FIFO) { + setup_fifo_xfer(xfer, mas, slv->mode, spi); + return 1; + } + return setup_gsi_xfer(xfer, mas, slv, spi); } static irqreturn_t geni_spi_isr(int irq, void *data) @@ -671,6 +942,15 @@ static int spi_geni_probe(struct platform_device *pdev) if (irq < 0) return irq; + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); + if (ret) { + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); + if (ret) { + dev_err(&pdev->dev, "could not set DMA mask\n"); + return ret; + } + } + base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(base)) return PTR_ERR(base); @@ -710,14 +990,17 @@ static int spi_geni_probe(struct platform_device *pdev) spi->max_speed_hz = 50000000; spi->prepare_message = spi_geni_prepare_message; spi->transfer_one = spi_geni_transfer_one; + spi->can_dma = geni_can_dma; + spi->dma_map_dev = mas->dev->parent; spi->auto_runtime_pm = true; spi->handle_err = handle_fifo_timeout; - spi->set_cs = spi_geni_set_cs; spi->use_gpio_descriptors = true; init_completion(&mas->cs_done); init_completion(&mas->cancel_done); init_completion(&mas->abort_done); + init_completion(&mas->tx_cb); + init_completion(&mas->rx_cb); spin_lock_init(&mas->lock); pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_set_autosuspend_delay(&pdev->dev, 250); @@ -738,6 +1021,14 @@ static int spi_geni_probe(struct platform_device *pdev) if (ret) goto spi_geni_probe_runtime_disable; + /* + * check the mode supported and set_cs for fifo mode only + * for dma (gsi) mode, the gsi will set cs based on params passed in + * TRE + */ + if (mas->cur_xfer_mode == GENI_SE_FIFO) + spi->set_cs = spi_geni_set_cs; + ret = request_irq(mas->irq, geni_spi_isr, 0, dev_name(dev), spi); if (ret) goto spi_geni_probe_runtime_disable; @@ -754,6 +1045,14 @@ static int spi_geni_probe(struct platform_device *pdev) return ret; } +static void spi_geni_release_dma_chan(struct spi_geni_master *mas) +{ + if (mas->rx) + dma_release_channel(mas->rx); + if (mas->tx) + dma_release_channel(mas->tx); +} + static int spi_geni_remove(struct platform_device *pdev) { struct spi_master *spi = platform_get_drvdata(pdev); @@ -762,6 +1061,8 @@ static int spi_geni_remove(struct platform_device *pdev) /* Unregister _before_ disabling pm_runtime() so we stop transfers */ spi_unregister_master(spi); + spi_geni_release_dma_chan(mas); + free_irq(mas->irq, spi); pm_runtime_disable(&pdev->dev); return 0;
We can use GPI DMA for devices where it is enabled by firmware. Add support for this mode Signed-off-by: Vinod Koul <vkoul@kernel.org> --- drivers/spi/spi-geni-qcom.c | 329 ++++++++++++++++++++++++++++++++++-- 1 file changed, 315 insertions(+), 14 deletions(-) -- 2.31.1