| Message ID | 20210111151651.1616813-1-vkoul@kernel.org |
|---|---|
| Headers | show |
| Series | Add and enable GPI DMA users | expand |
On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > GENI_IF_DISABLE_RO is used by geni spi driver as well to check the > status if GENI, so move this to common header qcom-geni-se.h > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/soc/qcom/qcom-geni-se.c | 1 - > include/linux/qcom-geni-se.h | 1 + > 2 files changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > index f42954e2c98e..285ed86c2bab 100644 > --- a/drivers/soc/qcom/qcom-geni-se.c > +++ b/drivers/soc/qcom/qcom-geni-se.c > @@ -108,7 +108,6 @@ static struct geni_wrapper *earlycon_wrapper; > #define GENI_OUTPUT_CTRL 0x24 > #define GENI_CGC_CTRL 0x28 > #define GENI_CLK_CTRL_RO 0x60 > -#define GENI_IF_DISABLE_RO 0x64 > #define GENI_FW_S_REVISION_RO 0x6c > #define SE_GENI_BYTE_GRAN 0x254 > #define SE_GENI_TX_PACKING_CFG0 0x260 > diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h > index ec2ad4b0fe14..e3f4b16040d9 100644 > --- a/include/linux/qcom-geni-se.h > +++ b/include/linux/qcom-geni-se.h > @@ -65,6 +65,7 @@ struct geni_se { > #define SE_GENI_STATUS 0x40 > #define GENI_SER_M_CLK_CFG 0x48 > #define GENI_SER_S_CLK_CFG 0x4c > +#define GENI_IF_DISABLE_RO 0x64 > #define GENI_FW_REVISION_RO 0x68 > #define SE_GENI_CLK_SEL 0x7c > #define SE_GENI_DMA_MODE_EN 0x258 > -- > 2.26.2 >
Hi, looks like sdm845-cheza also uses the spi0 bus, which as far as I understand is going to break with the GPI DMA disabled. Perhaps it should also be enabled over there? Actually, is there a point in disabling DMA for BLSPs/QUPs in the SoC DTSI? I don't think any platform/vendor firmware disables entire hosts.. Konrad
On 11-01-21, 08:47, Doug Anderson wrote: > Hi, > > On Mon, Jan 11, 2021 at 7:18 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > @@ -2622,6 +2626,13 @@ pinmux { > > "gpio2", "gpio3"; > > function = "qup0"; > > }; > > + > > + config { > > Convention in Qualcomm device tree files seems to be that the node is > "pinconf", not "config". Yes missed that, thanks for pointing > > > > + pins = "gpio0", "gpio1", > > + "gpio2", "gpio3"; > > + drive-strength = <6>; > > + bias-disable; > > + }; > > Pin config almost never belongs in the SoC dtsi file. This should be > in the board .dts file. What if pulls are needed on some pins? What > if you need a stronger or weaker drive strength? Right I will move it to dtsi
> If it is working without GPI enabled, it would work.. GPI for QUP is > something that requires firmware and would have to be enabled by > firmware I think with the new code of yours: mas->tx = dma_request_slave_channel(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 out_pm; it *may* fail to probe with the channels assigned in the "dmas=" property but with the engine having "status=disabled", but as I don't have any hardware to test that driver on, please confirm whether my concerns are right.. > Since DMA support may not be available on certain targets (firmware > support), so enabling per board would make sense Oh really, are there SDM845 boards shipping with GPI DMA *disabled to all peripherals*? Konrad
On 11-01-21, 21:45, Konrad Dybcio wrote: > > > If it is working without GPI enabled, it would work.. GPI for QUP is > > something that requires firmware and would have to be enabled by > > firmware > > > I think with the new code of yours: > > > mas->tx = dma_request_slave_channel(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 out_pm; This code would be exercised *only* when DMA mode is enabled in firmware.. Both SPI and I2C driver would probe the mode supported (reason for exposing stuff in first two patches of this series).. Only if the device reports we should enable it.. Also if the device is working with FIFO mode then DMA mode wont work.. > > it *may* fail to probe with the channels assigned in the "dmas=" > property but with the engine having "status=disabled", but as I don't > have any hardware to test that driver on, please confirm whether my > concerns are right.. > > > > Since DMA support may not be available on certain targets (firmware > > support), so enabling per board would make sense > > Oh really, are there SDM845 boards shipping with GPI DMA *disabled to > all peripherals*? Is the peripheral working with FIFO mode, then answer is yes :) Thanks -- ~Vinod
On 11-01-21, 12:14, Bjorn Andersson wrote: > On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > > > This adds capability to use GSI DMA for I2C transfers > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > --- > > drivers/i2c/busses/i2c-qcom-geni.c | 246 ++++++++++++++++++++++++++++- > > 1 file changed, 244 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > > index 046d241183c5..6978480fb4d1 100644 > > --- a/drivers/i2c/busses/i2c-qcom-geni.c > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > > @@ -12,7 +12,9 @@ > > #include <linux/of.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > +#include <linux/dmaengine.h> > > #include <linux/qcom-geni-se.h> > > +#include <linux/dma/qcom-gpi-dma.h> > > #include <linux/spinlock.h> > > > > #define SE_I2C_TX_TRANS_LEN 0x26c > > @@ -48,6 +50,8 @@ > > #define LOW_COUNTER_SHFT 10 > > #define CYCLE_COUNTER_MSK GENMASK(9, 0) > > > > +#define I2C_PACK_EN (BIT(0) | BIT(1)) > > + > > enum geni_i2c_err_code { > > GP_IRQ0, > > NACK, > > @@ -72,6 +76,12 @@ enum geni_i2c_err_code { > > #define XFER_TIMEOUT HZ > > #define RST_TIMEOUT HZ > > > > +enum i2c_se_mode { > > + UNINITIALIZED, > > + FIFO_SE_DMA, > > + GSI_ONLY, > > +}; > > + > > struct geni_i2c_dev { > > struct geni_se se; > > u32 tx_wm; > > @@ -86,6 +96,17 @@ struct geni_i2c_dev { > > u32 clk_freq_out; > > const struct geni_i2c_clk_fld *clk_fld; > > int suspended; > > + struct dma_chan *tx_c; > > + struct dma_chan *rx_c; > > + dma_cookie_t rx_cookie, tx_cookie; > > + dma_addr_t tx_ph; > > + dma_addr_t rx_ph; > > + int cfg_sent; > > bool? ok > > > + struct dma_async_tx_descriptor *tx_desc; > > + struct dma_async_tx_descriptor *rx_desc; > > + enum i2c_se_mode se_mode; > > bool gsi_only; I think fifo_mode would be more apt... since we check for other modes in the code > > > + bool cmd_done; > > Unused? heh, will remove.. > > + bool is_shared; > > Used but meaningless? Will drop > > > }; > > > > struct geni_i2c_err_log { > > @@ -429,6 +450,183 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > > return gi2c->err; > > } > > > > +static void i2c_gsi_cb_result(void *cb, const struct dmaengine_result *result) > > +{ > > + struct geni_i2c_dev *gi2c = cb; > > + > > + if (result->result != DMA_TRANS_NOERROR) { > > + dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result); > > + return; > > + } > > + > > + if (result->residue) > > + dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue); > > + > > + complete(&gi2c->done); > > +} > > + > > +static int geni_i2c_gsi_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], > > + int num) > > +{ > > + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap); > > + struct dma_slave_config config; > > + struct gpi_i2c_config peripheral; > > + int i, ret = 0, timeout = 0; > > + > > + memset(&config, 0, sizeof(config)); > > Assign {} to config during declaration. ok > > > + memset(&peripheral, 0, sizeof(peripheral)); > > + config.peripheral_config = &peripheral; > > + config.peripheral_size = sizeof(peripheral); > > + > > + if (!gi2c->tx_c) { > > + gi2c->tx_c = dma_request_slave_channel(gi2c->se.dev, "tx"); > > So object is reused for all future transfers as well? > Seems reasonable, but it should be released on driver removal? > > Could it be requested at probe time instead? yes it can be done, i would move it.. > > > + if (!gi2c->tx_c) { > > + dev_err(gi2c->se.dev, "tx dma_request_slave_channel fail\n"); > > + ret = -EIO; > > + goto geni_i2c_gsi_xfer_out; > > + } > > + } > > + > > + if (!gi2c->rx_c) { > > + gi2c->rx_c = dma_request_slave_channel(gi2c->se.dev, "rx"); > > + if (!gi2c->rx_c) { > > + dev_err(gi2c->se.dev, "rx dma_request_slave_channel fail\n"); > > + ret = -EIO; > > + goto geni_i2c_gsi_xfer_out; > > + } > > + } > > + > > + if (!gi2c->cfg_sent) { > > + const struct geni_i2c_clk_fld *itr = gi2c->clk_fld; > > + > > + peripheral.pack_enable = I2C_PACK_EN; > > + peripheral.cycle_count = itr->t_cycle_cnt; > > + peripheral.high_count = itr->t_high_cnt; > > + peripheral.low_count = itr->t_low_cnt; > > + peripheral.clk_div = itr->clk_div; > > + gi2c->cfg_sent = true; > > Is this a bool or an int? Now would be a bool :) > > > + peripheral.set_config = true; > > I find this somewhat ugly, you will always > dmaengine_slave_config(&config), but in the case of cfg_sent this will > point to an all-zero peripheral and hence will have set_config = false, > which will cause the skipping of setting up a configuration TRE. > > I would prefer that the value of peripheral.set_config related to > cfg_sent in a more explicit fashion. Sure, i think I can use a single value to do this, will update this > > > + } > > + > > + peripheral.multi_msg = false; > > + for (i = 0; i < num; i++) { > > + struct device *rx_dev = gi2c->se.wrapper->dev; > > + struct device *tx_dev = gi2c->se.wrapper->dev; > > + int stretch = (i < (num - 1)); > > + u8 *dma_buf = NULL; > > No need to initialize this, first use is an assignment. ok > > > + unsigned int flags; > > + > > + gi2c->cur = &msgs[i]; > > + > > + peripheral.addr = msgs[i].addr; > > + peripheral.stretch = stretch; > > + if (msgs[i].flags & I2C_M_RD) > > + peripheral.op = I2C_READ; > > + else > > + peripheral.op = I2C_WRITE; > > + > > + dma_buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); > > + if (!dma_buf) { > > + ret = -ENOMEM; > > + goto geni_i2c_gsi_xfer_out; > > + } > > + > > + if (msgs[i].flags & I2C_M_RD) { > > + gi2c->rx_ph = dma_map_single(rx_dev, dma_buf, > > + msgs[i].len, DMA_FROM_DEVICE); > > + if (dma_mapping_error(rx_dev, gi2c->rx_ph)) { > > + dev_err(gi2c->se.dev, "dma_map_single for rx failed :%d\n", ret); > > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false); > > + goto geni_i2c_gsi_xfer_out; > > + } > > + > > + peripheral.op = I2C_READ; > > + peripheral.stretch = stretch; > > + ret = dmaengine_slave_config(gi2c->rx_c, &config); > > + if (ret) { > > + dev_err(gi2c->se.dev, "rx dma config error:%d\n", ret); > > + goto geni_i2c_gsi_xfer_out; > > Need to unmap rx_ph? yes will update > > > + } > > + peripheral.set_config = false; > > + peripheral.multi_msg = true; > > + peripheral.rx_len = msgs[i].len; > > + > > + flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; > > + gi2c->rx_desc = dmaengine_prep_slave_single(gi2c->rx_c, gi2c->rx_ph, > > + msgs[i].len, > > + DMA_DEV_TO_MEM, flags); > > Is the rx_desc freed by the dmaengine core when > dma_async_issue_pending() finishes it's job? Yes > If so, why do you need to keep this pointer in gi2c? Wouldn't a local > variable suffice? Yes local should suffice, will update > > > + if (!gi2c->rx_desc) { > > + dev_err(gi2c->se.dev, "prep_slave_sg for rx failed\n"); > > + gi2c->err = -EIO; > > + goto geni_i2c_err_prep_sg; > > + } > > + > > + gi2c->rx_desc->callback_result = i2c_gsi_cb_result; > > + gi2c->rx_desc->callback_param = gi2c; > > + > > + /* Issue RX */ > > + gi2c->rx_cookie = dmaengine_submit(gi2c->rx_desc); > > + dma_async_issue_pending(gi2c->rx_c); > > + } > > + > > + dev_dbg(gi2c->se.dev, "msg[%d].len:%d W\n", i, gi2c->cur->len); > > + gi2c->tx_ph = dma_map_single(tx_dev, dma_buf, msgs[i].len, DMA_TO_DEVICE); > > Maybe I've forgotten something important about I2C, but why do we always > TX (even if it's a RX transfer)? I think we need to send the device address for i2c, so even if we want to do RX, that will always involve a TX txn as well > > > + if (dma_mapping_error(tx_dev, gi2c->tx_ph)) { > > + dev_err(gi2c->se.dev, "dma_map_single for tx failed :%d\n", ret); > > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false); > > Need to unmap rx_ph? > > > + goto geni_i2c_gsi_xfer_out; > > + } > > + > > + peripheral.stretch = stretch; > > + peripheral.op = I2C_WRITE; > > + ret = dmaengine_slave_config(gi2c->tx_c, &config); > > + if (ret) { > > + dev_err(gi2c->se.dev, "tx dma config error:%d\n", ret); > > Need to unmap rx_ph and tx_ph? Yeah looks like I missed unrolling, will check and update all these > > > + goto geni_i2c_gsi_xfer_out; > > + } > > + peripheral.set_config = false; > > + peripheral.multi_msg = true; > > + gi2c->tx_desc = dmaengine_prep_slave_single(gi2c->tx_c, gi2c->tx_ph, msgs[i].len, > > + DMA_MEM_TO_DEV, > > + (DMA_PREP_INTERRUPT | DMA_CTRL_ACK)); > > + if (!gi2c->tx_desc) { > > + dev_err(gi2c->se.dev, "prep_slave_sg for tx failed\n"); > > + gi2c->err = -ENOMEM; > > + goto geni_i2c_err_prep_sg; > > + } > > + gi2c->tx_desc->callback_result = i2c_gsi_cb_result; > > + gi2c->tx_desc->callback_param = gi2c; > > + > > + /* Issue TX */ > > + gi2c->tx_cookie = dmaengine_submit(gi2c->tx_desc); > > + dma_async_issue_pending(gi2c->tx_c); > > + > > + timeout = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); > > + if (!timeout) { > > + dev_err(gi2c->se.dev, "I2C timeout gsi flags:%d addr:0x%x\n", > > + gi2c->cur->flags, gi2c->cur->addr); > > + gi2c->err = -ETIMEDOUT; > > + } > > +geni_i2c_err_prep_sg: > > Perhaps you can break the body of this loop out to a separate function > and thereby avoid the goto within the block? > > > + if (gi2c->err) { > > + dmaengine_terminate_all(gi2c->tx_c); > > + gi2c->cfg_sent = 0; > > Is this a bool or an int? > > > + } > > + if (msgs[i].flags & I2C_M_RD) > > + dma_unmap_single(rx_dev, gi2c->rx_ph, msgs[i].len, DMA_FROM_DEVICE); > > You unconditionally map tx_ph, but you only unmap it on ~I2C_M_RD. This > fits better with my expectation, but would mean that the whole tx block > above should be in an else. > > > + else > > + dma_unmap_single(tx_dev, gi2c->tx_ph, msgs[i].len, DMA_TO_DEVICE); > > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], !gi2c->err); > > + if (gi2c->err) > > + goto geni_i2c_gsi_xfer_out; > > This goto is just a "break" in disguise. > > > + } > > + > > +geni_i2c_gsi_xfer_out: > > + if (!ret && gi2c->err) > > + ret = gi2c->err; > > + return ret; > > +} > > + > > static int geni_i2c_xfer(struct i2c_adapter *adap, > > struct i2c_msg msgs[], > > int num) > > @@ -448,6 +646,15 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > > } > > > > qcom_geni_i2c_conf(gi2c); > > + > > + if (gi2c->se_mode == GSI_ONLY) { > > + ret = geni_i2c_gsi_xfer(adap, msgs, num); > > + goto geni_i2c_txn_ret; > > Rather than goto skip_non_gsi_code; I think you should move the non-gsi > part of this function into a separate fifo function and make this Okay let me take a relook at this whole blob and refactor it.. > > if (GSI_ONLY) > ret = geni_i2c_gsi_xfer(); > else > ret = geni_i2c_fifo_xfer(); > > > + } else { > > + /* Don't set shared flag in non-GSI mode */ > > + gi2c->is_shared = false; > > I don't see this flag being looked at elsewhere. > > > + } > > + > > for (i = 0; i < num; i++) { > > u32 m_param = i < (num - 1) ? STOP_STRETCH : 0; > > > > @@ -462,6 +669,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > > if (ret) > > break; > > } > > +geni_i2c_txn_ret: > > if (ret == 0) > > ret = num; > > > > @@ -628,7 +836,8 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) > > int ret; > > struct geni_i2c_dev *gi2c = dev_get_drvdata(dev); > > > > - disable_irq(gi2c->irq); > > + if (gi2c->se_mode == FIFO_SE_DMA) > > + disable_irq(gi2c->irq); > > ret = geni_se_resources_off(&gi2c->se); > > if (ret) { > > enable_irq(gi2c->irq); > > @@ -653,8 +862,41 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev) > > ret = geni_se_resources_on(&gi2c->se); > > if (ret) > > return ret; > > + if (gi2c->se_mode == UNINITIALIZED) { > > + int proto = geni_se_read_proto(&gi2c->se); > > + u32 se_mode; > > Please declare your variables at the top of the function. > > > + > > + if (unlikely(proto != GENI_SE_I2C)) { > > If this was the case at probe time the driver would never have probed, > why has it changed? > > This is not a fastpath, so skip the unlikely() > > > + dev_err(gi2c->se.dev, "Invalid proto %d\n", proto); > > + geni_se_resources_off(&gi2c->se); > > + return -ENXIO; > > + } > > + > > + se_mode = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & > > + FIFO_IF_DISABLE; > > se_mode would better be called "fifo_disabled" or perhaps logically > suited "gsi_only"? O think fifo_mode or just mode might be apt > > Please skip the _relaxed yes > > > + if (se_mode) { > > + gi2c->se_mode = GSI_ONLY; > > + geni_se_select_mode(&gi2c->se, GENI_GPI_DMA); > > + dev_dbg(gi2c->se.dev, "i2c GSI mode\n"); > > + } else { > > + int gi2c_tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se); > > This variable has an unnecessarily long name. will shorten > > > + > > + gi2c->se_mode = FIFO_SE_DMA; > > + gi2c->tx_wm = gi2c_tx_depth - 1; > > + geni_se_init(&gi2c->se, gi2c->tx_wm, gi2c_tx_depth); > > + geni_se_config_packing(&gi2c->se, BITS_PER_BYTE, > > + PACKING_BYTES_PW, true, true, true); > > + qcom_geni_i2c_conf(gi2c); > > + dev_dbg(gi2c->se.dev, > > + "i2c fifo/se-dma mode. fifo depth:%d\n", gi2c_tx_depth); > > + } > > + dev_dbg(gi2c->se.dev, "i2c-%d: %s\n", > > + gi2c->adap.nr, dev_name(gi2c->se.dev)); > > dev_dbg() already provides dev_name. What information does this debug > print actually try to communicate? not much am afraid, will update -- ~Vinod
Hi, On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote: > > Hello, > > This series add the GPI DMA in qcom geni spi and i2c drivers. For this we > first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common > headers and then add support for gpi dma in geni driver. > > Then we add spi and i2c geni driver changes to support this DMA. > > Lastly, add the GPI dma nodes and enable dma for spi found in Rb3 board. > > To merge this, we could merge all thru qcom tree with ack on spi/i2c. It'd be super great if somewhere (ideally in the commit message and maybe somewhere in the code) you could talk more about the different modes. Maybe something like this (if it's correct): GPI Mode (confusingly, also known as "GSI" mode in some places): In this mode something else running on the SoC is sharing access to the geni instance. This mode allows sharing the device between the Linux kernel and other users including handling the fact that other users might be running the geni port at a different clock rate. GPI mode limits what you can do with a port. For instance, direct control of chip select is not allowed. NOTE: if firmware has configured a geni instance for GPI then FIFO and SE_DMA usage is not allowed. Conversely, if firmware has not configured a geni instance for GPI then only FIFO and SE_DMA usage is allowed. SE DMA Mode: Data transfers happen over DMA. SE FIFO Mode: Data is manually transferred into the FIFO by the CPU.
Hello Doug, On 12-01-21, 16:01, Doug Anderson wrote: > Hi, > > On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > Hello, > > > > This series add the GPI DMA in qcom geni spi and i2c drivers. For this we > > first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common > > headers and then add support for gpi dma in geni driver. > > > > Then we add spi and i2c geni driver changes to support this DMA. > > > > Lastly, add the GPI dma nodes and enable dma for spi found in Rb3 board. > > > > To merge this, we could merge all thru qcom tree with ack on spi/i2c. > > It'd be super great if somewhere (ideally in the commit message and > maybe somewhere in the code) you could talk more about the different > modes. Maybe something like this (if it's correct): > > GPI Mode (confusingly, also known as "GSI" mode in some places): In > this mode something else running on the SoC is sharing access to the > geni instance. This mode allows sharing the device between the Linux > kernel and other users including handling the fact that other users > might be running the geni port at a different clock rate. GPI mode > limits what you can do with a port. For instance, direct control of > chip select is not allowed. NOTE: if firmware has configured a geni > instance for GPI then FIFO and SE_DMA usage is not allowed. > Conversely, if firmware has not configured a geni instance for GPI > then only FIFO and SE_DMA usage is allowed. > > SE DMA Mode: Data transfers happen over DMA. > > SE FIFO Mode: Data is manually transferred into the FIFO by the CPU. I think it is a good feedback, there is indeed bunch of confusion wrt QUP DMA and i think we should add above to qcom geni driver and not just in cover letter. FWIW for all practical purposes GSI and GPI can be used interchangeably. There are some nuisances involved like firmware and a microcontroller but for the sake of simplicity we can skip that :) Thanks