| 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 >
On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > GPI DMA is one of the DMA modes supported on geni, this adds support to > enable that mode > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/soc/qcom/qcom-geni-se.c | 39 ++++++++++++++++++++++++++++++++- > include/linux/qcom-geni-se.h | 4 ++++ > 2 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > index a3868228ea05..db44dc32e049 100644 > --- a/drivers/soc/qcom/qcom-geni-se.c > +++ b/drivers/soc/qcom/qcom-geni-se.c > @@ -310,6 +310,39 @@ static void geni_se_select_dma_mode(struct geni_se *se) > writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN); > } > > +static int geni_se_select_gpi_mode(struct geni_se *se) This doesn't return any information and the return value isn't looked at, please make it void. > +{ > + unsigned int geni_dma_mode = 0; > + unsigned int gpi_event_en = 0; > + unsigned int common_geni_m_irq_en = 0; > + unsigned int common_geni_s_irq_en = 0; These could certainly be given a shorter name. None of them needs to be initialized, first access in all cases are assignments. > + > + common_geni_m_irq_en = readl_relaxed(se->base + SE_GENI_M_IRQ_EN); > + common_geni_s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN); > + common_geni_m_irq_en &= > + ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | > + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); > + common_geni_s_irq_en &= ~S_CMD_DONE_EN; > + geni_dma_mode = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN); > + gpi_event_en = readl_relaxed(se->base + SE_GSI_EVENT_EN); > + > + geni_dma_mode |= GENI_DMA_MODE_EN; > + gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | > + GENI_M_EVENT_EN | GENI_S_EVENT_EN); Please reorder these so that you do readl(m) mask out bits of m readl(s) mask out bits of s ... > + > + writel_relaxed(0, se->base + SE_IRQ_EN); > + writel_relaxed(common_geni_s_irq_en, se->base + SE_GENI_S_IRQ_EN); > + writel_relaxed(common_geni_m_irq_en, se->base + SE_GENI_M_IRQ_EN); > + writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR); Lowercase hex digits please. > + writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_S_IRQ_CLEAR); > + writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_TX_IRQ_CLR); > + writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_RX_IRQ_CLR); > + writel_relaxed(geni_dma_mode, se->base + SE_GENI_DMA_MODE_EN); > + writel_relaxed(gpi_event_en, se->base + SE_GSI_EVENT_EN); Why is this driver using _relaxed accessors exclusively? Why are you using _relaxed versions? And wouldn't it be suitable to have a wmb() before the "dma mode enable" and "event enable" at least? (I.e. use writel() instead) Regards, Bjorn > + > + return 0; > +} > + > /** > * geni_se_select_mode() - Select the serial engine transfer mode > * @se: Pointer to the concerned serial engine. > @@ -317,7 +350,8 @@ static void geni_se_select_dma_mode(struct geni_se *se) > */ > void geni_se_select_mode(struct geni_se *se, enum geni_se_xfer_mode mode) > { > - WARN_ON(mode != GENI_SE_FIFO && mode != GENI_SE_DMA); > + WARN_ON(mode != GENI_SE_FIFO && mode != GENI_SE_DMA && > + mode != GENI_GPI_DMA); > > switch (mode) { > case GENI_SE_FIFO: > @@ -326,6 +360,9 @@ void geni_se_select_mode(struct geni_se *se, enum geni_se_xfer_mode mode) > case GENI_SE_DMA: > geni_se_select_dma_mode(se); > break; > + case GENI_GPI_DMA: > + geni_se_select_gpi_mode(se); > + break; > case GENI_SE_INVALID: > default: > break; > diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h > index cb4e40908f9f..12003a6cb133 100644 > --- a/include/linux/qcom-geni-se.h > +++ b/include/linux/qcom-geni-se.h > @@ -12,6 +12,7 @@ > enum geni_se_xfer_mode { > GENI_SE_INVALID, > GENI_SE_FIFO, > + GENI_GPI_DMA, > GENI_SE_DMA, > }; > > @@ -123,6 +124,9 @@ struct geni_se { > #define CLK_DIV_MSK GENMASK(15, 4) > #define CLK_DIV_SHFT 4 > > +/* GENI_IF_DISABLE_RO fields */ > +#define FIFO_IF_DISABLE (BIT(0)) > + > /* GENI_FW_REVISION_RO fields */ > #define FW_REV_PROTOCOL_MSK GENMASK(15, 8) > #define FW_REV_PROTOCOL_SHFT 8 > -- > 2.26.2 >
On Mon, Jan 11, 2021 at 08:46:48PM +0530, Vinod Koul wrote: > +static int get_xfer_mode(struct spi_master *spi) > +{ > + struct spi_geni_master *mas = spi_master_get_devdata(spi); > + struct geni_se *se = &mas->se; > + int mode = GENI_SE_FIFO; Why not use the core DMA mapping support?
On Mon 11 Jan 09:16 CST 2021, Vinod Koul 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 | 395 +++++++++++++++++++++++++++++++++++- > 1 file changed, 384 insertions(+), 11 deletions(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index 512e925d5ea4..5bb0e2192734 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -2,6 +2,8 @@ > // 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/interrupt.h> > #include <linux/io.h> > #include <linux/log2.h> > @@ -10,6 +12,7 @@ > #include <linux/pm_opp.h> > #include <linux/pm_runtime.h> > #include <linux/qcom-geni-se.h> > +#include <linux/dma/qcom-gpi-dma.h> > #include <linux/spi/spi.h> > #include <linux/spinlock.h> > > @@ -63,6 +66,35 @@ > #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 desc_cb; > +}; > + > +enum spi_m_cmd_opcode { > + CMD_NONE, > + CMD_XFER, > + CMD_CS, > + CMD_CANCEL, > +}; > + > struct spi_geni_master { > struct geni_se se; > struct device *dev; > @@ -79,10 +111,21 @@ struct spi_geni_master { > struct completion cs_done; > struct completion cancel_done; > struct completion abort_done; > + struct completion xfer_done; > unsigned int oversampling; > spinlock_t lock; > int irq; > bool cs_flag; > + struct spi_geni_gsi *gsi; > + struct dma_chan *tx; > + struct dma_chan *rx; > + struct completion tx_cb; > + struct completion rx_cb; > + bool qn_err; > + int cur_xfer_mode; > + int num_tx_eot; > + int num_rx_eot; > + int num_xfers; > }; > > static int get_spi_clk_cfg(unsigned int speed_hz, > @@ -274,31 +317,269 @@ static int setup_fifo_params(struct spi_device *spi_slv, > return geni_spi_set_clock_and_bw(mas, spi_slv->max_speed_hz); > } > > +static int get_xfer_mode(struct spi_master *spi) > +{ > + struct spi_geni_master *mas = spi_master_get_devdata(spi); > + struct geni_se *se = &mas->se; > + int mode = GENI_SE_FIFO; No need to initialize mode, it's overwritten in all code paths. > + int fifo_disable; > + bool dma_chan_valid; > + > + fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE; > + dma_chan_valid = !(IS_ERR_OR_NULL(mas->tx) || IS_ERR_OR_NULL(mas->rx)); > + > + /* > + * If FIFO Interface is disabled and there are no DMA channels then we > + * can't do this transfer. > + * If FIFO interface is disabled, we can do GSI only, > + * else pick FIFO mode. > + */ > + if (fifo_disable && !dma_chan_valid) { > + dev_err(mas->dev, "Fifo and dma mode disabled!! can't xfer\n"); > + mode = -EINVAL; > + } else if (fifo_disable) { > + mode = GENI_GPI_DMA; > + } else { > + mode = GENI_SE_FIFO; > + } > + > + return mode; Maybe make this tail: if (!dma_chan_valid && fifo_disable) return -EINVAL; return fifo_disable ? GENI_GPI_DMA : GENI_SE_FIFO; At least to me that's more obvious. > +} > + > +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 %s txn failed\n", __func__, tx ? "tx" : "rx"); "GSI DMA %s failed\n" is just as unique, without the need for __func__. > + return; > + } > + > + if (!result->residue) { > + dev_dbg(gsi->mas->dev, "%s\n", __func__); You have @tx, so how about including that to make the debug print slightly more useful? > + if (tx) > + complete(&gsi->mas->tx_cb); > + else > + complete(&gsi->mas->rx_cb); > + } else { > + dev_err(gsi->mas->dev, "DMA xfer has pending: %d\n", 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) > +{ > + int ret = 0; > + unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; > + struct spi_geni_gsi *gsi; > + struct dma_slave_config config; > + struct gpi_spi_config peripheral; > + > + memset(&config, 0, sizeof(config)); Assign {} during the declaration and you don't need to start with a memset. > + memset(&peripheral, 0, sizeof(peripheral)); > + config.peripheral_config = &peripheral; > + config.peripheral_size = sizeof(peripheral); > + > + 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; > + peripheral.set_config = true; > + } > + > + 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; > + } > + > + peripheral.cs = spi_slv->chip_select; > + > + 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.pack_en = true; > + peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN; > + ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, > + &peripheral.clk_src, &peripheral.clk_div); > + if (ret) { > + dev_err(mas->dev, "%s:Err setting clks:%d\n", __func__, ret); Please avoid the __func__. > + return ret; > + } > + > + if (!xfer->cs_change) { > + if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers)) > + peripheral.fragmentation = FRAGMENTATION; > + } > + > + gsi = &mas->gsi[mas->num_xfers]; > + gsi->desc_cb.mas = mas; > + gsi->desc_cb.xfer = xfer; > + if (peripheral.cmd & SPI_RX) { > + dmaengine_slave_config(mas->rx, &config); > + gsi->rx_desc = dmaengine_prep_slave_single(mas->rx, xfer->rx_dma, > + xfer->len, DMA_DEV_TO_MEM, flags); > + if (IS_ERR_OR_NULL(gsi->rx_desc)) { Is this API really returning IS_ERR() or NULL on failure? Looking at the GPI driver it seems to exclusively return NULL on failure (for things that in most other subsystems would be -EINVAL). > + 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->desc_cb; > + mas->num_rx_eot++; > + } > + > + if (peripheral.cmd & SPI_TX_ONLY) > + mas->num_tx_eot++; > + > + dmaengine_slave_config(mas->tx, &config); > + gsi->tx_desc = dmaengine_prep_slave_single(mas->tx, xfer->tx_dma, > + xfer->len, DMA_MEM_TO_DEV, flags); > + > + if (IS_ERR_OR_NULL(gsi->tx_desc)) { Is there anything that will clean up rx_desc when this happens? > + dev_err(mas->dev, "Err setting up tx desc\n"); > + return -EIO; > + } > + gsi->tx_desc->callback_result = spi_gsi_tx_callback_result; > + gsi->tx_desc->callback_param = &gsi->desc_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++; > + return ret; ret can only be 0 here. > +} > + > +static int spi_geni_map_buf(struct spi_geni_master *mas, struct spi_message *msg) > +{ > + struct spi_transfer *xfer; > + struct device *gsi_dev = mas->dev->parent; > + > + list_for_each_entry(xfer, &msg->transfers, transfer_list) { > + if (xfer->rx_buf) { > + xfer->rx_dma = dma_map_single(gsi_dev, xfer->rx_buf, > + xfer->len, DMA_FROM_DEVICE); > + if (dma_mapping_error(mas->dev, xfer->rx_dma)) { > + dev_err(mas->dev, "Err mapping buf\n"); You need to unmap rx_dma/tx_dma for all previous entries in &msg->transfers. > + return -ENOMEM; > + } > + } > + > + if (xfer->tx_buf) { > + xfer->tx_dma = dma_map_single(gsi_dev, (void *)xfer->tx_buf, > + xfer->len, DMA_TO_DEVICE); > + if (dma_mapping_error(gsi_dev, xfer->tx_dma)) { > + dev_err(mas->dev, "Err mapping buf\n"); > + dma_unmap_single(gsi_dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE); This should only be done if xfer->rx_buf != NULL, right? > + return -ENOMEM; > + } > + } > + }; > + > + return 0; > +} > + > +static void spi_geni_unmap_buf(struct spi_geni_master *mas, struct spi_message *msg) > +{ > + struct spi_transfer *xfer; > + struct device *gsi_dev = mas->dev; > + > + list_for_each_entry(xfer, &msg->transfers, transfer_list) { > + if (xfer->rx_buf) > + dma_unmap_single(gsi_dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE); > + if (xfer->tx_buf) > + dma_unmap_single(gsi_dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE); > + }; > +} > + > 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); > + struct geni_se *se = &mas->se; > + > + mas->cur_xfer_mode = get_xfer_mode(spi); > + > + if (mas->cur_xfer_mode == GENI_SE_FIFO) { > + geni_se_select_mode(se, GENI_SE_FIFO); > + reinit_completion(&mas->xfer_done); > + ret = setup_fifo_params(spi_msg->spi, spi); > + if (ret) > + dev_err(mas->dev, "Couldn't select mode %d\n", ret); Afaict all error paths of setup_fifo_params() will have printed an error message when we get here. So switch (mas->cur_xfer_mode) { case GENI_SE_FIFO: ... return setup_fifo_params(); case GENI_GPI_DMA: ... return spi_geni_map_buf(); } return -EINVAL; Seems cleaner to me. > + > + } else if (mas->cur_xfer_mode == GENI_GPI_DMA) { > + mas->num_tx_eot = 0; > + mas->num_rx_eot = 0; > + 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)); > + geni_se_select_mode(se, GENI_GPI_DMA); > + ret = spi_geni_map_buf(mas, spi_msg); > + > + } else { > + dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, mas->cur_xfer_mode); > + ret = -EINVAL; > + } > > - ret = setup_fifo_params(spi_msg->spi, spi); > - if (ret) > - dev_err(mas->dev, "Couldn't select mode %d\n", ret); > return ret; > } > > +static int spi_geni_unprepare_message(struct spi_master *spi_mas, struct spi_message *spi_msg) > +{ > + struct spi_geni_master *mas = spi_master_get_devdata(spi_mas); > + > + mas->cur_speed_hz = 0; > + mas->cur_bits_per_word = 0; > + if (mas->cur_xfer_mode == GENI_GPI_DMA) > + spi_geni_unmap_buf(mas, spi_msg); > + return 0; > +} > + > 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; > + size_t gsi_sz; > + int ret = 0; > > 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; > + ret = -ENXIO; > + goto out_pm; > } > mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se); > > @@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas) > spi_tx_cfg &= ~CS_TOGGLE; > writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG); > > + 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)); I first wrote a rant about breaking backwards compatibility, but then at last realized that this hunk doesn't actually modify "ret", so all this error handling - and error printing - might still result in a successful exit. I also don't think it's accurate to have all errors treated the same way, e.g. EPROBE_DEFER shouldn't be treated the same way as "no dma property specified". > + ret = PTR_ERR(mas->tx); > + goto out_pm; > + } else { > + mas->rx = dma_request_slave_channel(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)); > + dma_release_channel(mas->tx); > + ret = PTR_ERR(mas->rx); Per the use of IS_ERR_OR_NULL() ret might become 0 here. > + goto out_pm; > + } > + > + 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)) { We got both rx and tx, but then this allocation failed, is that reason for returning success without dma operational? (I'm not sure what the right thing to do). But checking for allocation failure isn't done with IS_ERR(). > + dma_release_channel(mas->tx); If you spin this hunk off into its own function then you can use the idiomatic goto cleanup scheme and not have to repeat yourself here. > + dma_release_channel(mas->rx); > + mas->tx = NULL; > + mas->rx = NULL; > + goto out_pm; > + } > + } > + > +out_pm: > pm_runtime_put(mas->dev); > - return 0; > + return ret; > } > > static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas) > @@ -420,6 +727,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, > { > u32 m_cmd = 0; > u32 len; > + u32 m_param = 0; > struct geni_se *se = &mas->se; > int ret; > > @@ -457,6 +765,11 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, > len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1); > len &= TRANS_LEN_MSK; > > + if (!xfer->cs_change) { > + if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers)) > + m_param |= FRAGMENTATION; > + } > + > mas->cur_xfer = xfer; > if (xfer->tx_buf) { > m_cmd |= SPI_TX_ONLY; > @@ -475,7 +788,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, > * interrupt could come in at any time now. > */ > spin_lock_irq(&mas->lock); > - geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION); > + geni_se_setup_m_cmd(se, m_cmd, m_param); > > /* > * TX_WATERMARK_REG should be set after SPI configuration and > @@ -494,13 +807,52 @@ static int spi_geni_transfer_one(struct spi_master *spi, > struct spi_transfer *xfer) > { > struct spi_geni_master *mas = spi_master_get_devdata(spi); > + unsigned long timeout, jiffies; > + int ret = 0i, i; 0i? Is that a sign of you using vim? > > /* Terminate and return success for 0 byte length transfer */ > if (!xfer->len) > - return 0; > + return ret; > + > + if (mas->cur_xfer_mode == GENI_SE_FIFO) { > + setup_fifo_xfer(xfer, mas, slv->mode, spi); > + } else { > + setup_gsi_xfer(xfer, mas, slv, spi); > + if (mas->num_xfers >= NUM_SPI_XFER || > + (list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))) { > + for (i = 0 ; i < mas->num_tx_eot; i++) { > + 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; > + } > + spi_finalize_current_transfer(spi); > + } > + for (i = 0 ; i < mas->num_rx_eot; i++) { > + jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS); > + timeout = wait_for_completion_timeout(&mas->tx_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); > + } > + if (mas->qn_err) { > + ret = -EIO; > + mas->qn_err = false; > + goto err_gsi_geni_transfer_one; > + } > + } > + } > > - setup_fifo_xfer(xfer, mas, slv->mode, spi); > - return 1; > + return ret; All assignments to "ret" are followed by a goto err_gsi_geni_transfer_one, so the only time you actually get here is with ret has been untouched...in which case it's now 0, rather than the 1 it was previously. > + > +err_gsi_geni_transfer_one: > + dmaengine_terminate_all(mas->tx); > + return ret; > } > > static irqreturn_t geni_spi_isr(int irq, void *data) > @@ -595,6 +947,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); > @@ -632,15 +993,18 @@ static int spi_geni_probe(struct platform_device *pdev) > spi->num_chipselect = 4; > spi->max_speed_hz = 50000000; > spi->prepare_message = spi_geni_prepare_message; > + spi->unprepare_message = spi_geni_unprepare_message; > spi->transfer_one = spi_geni_transfer_one; > 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->xfer_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); > @@ -661,6 +1025,15 @@ static int spi_geni_probe(struct platform_device *pdev) > if (ret) > goto spi_geni_probe_runtime_disable; > > + /* > + * query 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 > + */ Is there no SE_DMA mode for SPI? (I know it's not implemented right now) If we get there this would be cur_xfer_mode != GENI_GPI_DMA? Regards, Bjorn > + mas->cur_xfer_mode = get_xfer_mode(spi); > + 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; > -- > 2.26.2 >
On 11-01-21, 09:40, Bjorn Andersson wrote: > On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > > > GPI DMA is one of the DMA modes supported on geni, this adds support to > > enable that mode > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > --- > > drivers/soc/qcom/qcom-geni-se.c | 39 ++++++++++++++++++++++++++++++++- > > include/linux/qcom-geni-se.h | 4 ++++ > > 2 files changed, 42 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > > index a3868228ea05..db44dc32e049 100644 > > --- a/drivers/soc/qcom/qcom-geni-se.c > > +++ b/drivers/soc/qcom/qcom-geni-se.c > > @@ -310,6 +310,39 @@ static void geni_se_select_dma_mode(struct geni_se *se) > > writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN); > > } > > > > +static int geni_se_select_gpi_mode(struct geni_se *se) > > This doesn't return any information and the return value isn't looked > at, please make it void. Sure.. > > +{ > > + unsigned int geni_dma_mode = 0; > > + unsigned int gpi_event_en = 0; > > + unsigned int common_geni_m_irq_en = 0; > > + unsigned int common_geni_s_irq_en = 0; > > These could certainly be given a shorter name. Certainly.. > None of them needs to be initialized, first access in all cases are > assignments. Will update > > + > > + common_geni_m_irq_en = readl_relaxed(se->base + SE_GENI_M_IRQ_EN); > > + common_geni_s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN); > > + common_geni_m_irq_en &= > > + ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | > > + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); > > + common_geni_s_irq_en &= ~S_CMD_DONE_EN; > > + geni_dma_mode = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN); > > + gpi_event_en = readl_relaxed(se->base + SE_GSI_EVENT_EN); > > + > > + geni_dma_mode |= GENI_DMA_MODE_EN; > > + gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | > > + GENI_M_EVENT_EN | GENI_S_EVENT_EN); > > Please reorder these so that you do > readl(m) > mask out bits of m > > readl(s) > mask out bits of s > > ... okay will update > > + > > + writel_relaxed(0, se->base + SE_IRQ_EN); > > + writel_relaxed(common_geni_s_irq_en, se->base + SE_GENI_S_IRQ_EN); > > + writel_relaxed(common_geni_m_irq_en, se->base + SE_GENI_M_IRQ_EN); > > + writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR); > > Lowercase hex digits please. Yeah missed > > + writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_S_IRQ_CLEAR); > > + writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_TX_IRQ_CLR); > > + writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_RX_IRQ_CLR); > > + writel_relaxed(geni_dma_mode, se->base + SE_GENI_DMA_MODE_EN); > > + writel_relaxed(gpi_event_en, se->base + SE_GSI_EVENT_EN); > > Why is this driver using _relaxed accessors exclusively? Why are you > using _relaxed versions? > > And wouldn't it be suitable to have a wmb() before the "dma mode enable" > and "event enable" at least? (I.e. use writel() instead) Yeah we invoke this to select the mode before programming DMA, so yes a wmb() would make sense. Thanks for quick look
On 11-01-21, 09:34, Bjorn Andersson wrote: > On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > > > I2C geni driver needs to access struct geni_wrapper, so move it to > > header. > > > > Please tell me more! > > Glanced through the other patches and the only user I can find it in > patch 5 where you use this to get the struct device * of the wrapper. That is correct. The dma mapping needs to be done with SE device. > At least in the DT case this would be [SE]->dev->parent, perhaps we > can't rely on this due to ACPI? I would have missed that then, but I somehow recall trying that.. Though I have not looked into ACPI.. Given that we would need to worry about ACPI, do you recommend using parent or keeping this -- ~Vinod
On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > This add the device node for gpi dma0 instances found in sdm845. I think the 0 in "dma0" should go? Apart from that, this looks good. > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 46 ++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index bcf888381f14..c9a127bbd606 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -1114,6 +1114,29 @@ opp-128000000 { > }; > }; > > + gpi_dma0: dma-controller@800000 { > + #dma-cells = <3>; Nit. I know #dma-cells are important to you ;) but I would prefer to have the standard properties (e.g. compatible) first. Regards, Bjorn > + compatible = "qcom,sdm845-gpi-dma"; > + reg = <0 0x00800000 0 0x60000>; > + interrupts = <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 246 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 249 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 250 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 254 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 255 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>; > + dma-channels = <13>; > + dma-channel-mask = <0xfa>; > + iommus = <&apps_smmu 0x0016 0x0>; > + status = "disabled"; > + }; > + > qupv3_id_0: geniqup@8c0000 { > compatible = "qcom,geni-se-qup"; > reg = <0 0x008c0000 0 0x6000>; > @@ -1533,6 +1556,29 @@ uart7: serial@89c000 { > }; > }; > > + gpi_dma1: dma-controller@0xa00000 { > + #dma-cells = <3>; > + compatible = "qcom,sdm845-gpi-dma"; > + reg = <0 0x00a00000 0 0x60000>; > + interrupts = <GIC_SPI 279 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 280 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 281 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 293 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 294 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 295 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 296 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 297 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>; > + dma-channels = <13>; > + dma-channel-mask = <0xfa>; > + iommus = <&apps_smmu 0x06d6 0x0>; > + status = "disabled"; > + }; > + > qupv3_id_1: geniqup@ac0000 { > compatible = "qcom,geni-se-qup"; > reg = <0 0x00ac0000 0 0x6000>; > -- > 2.26.2 >
On Mon 11 Jan 11:43 CST 2021, Vinod Koul wrote: > On 11-01-21, 09:34, Bjorn Andersson wrote: > > On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > > > > > I2C geni driver needs to access struct geni_wrapper, so move it to > > > header. > > > > > > > Please tell me more! > > > > Glanced through the other patches and the only user I can find it in > > patch 5 where you use this to get the struct device * of the wrapper. > > That is correct. The dma mapping needs to be done with SE device. > > > At least in the DT case this would be [SE]->dev->parent, perhaps we > > can't rely on this due to ACPI? > > I would have missed that then, but I somehow recall trying that.. Though > I have not looked into ACPI.. > > Given that we would need to worry about ACPI, do you recommend using > parent or keeping this > We get the wrapper by the means of dev_drv_getdata(dev->parent), so afaict we need to figure out how to get hold of the wrapper for ACPI to work either way. We then need to lug around the wrapper's device for your uses and exposing the wrapper struct solves this for us. So I'm okay with your proposal. Regards, Bjorn
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 -- ~Vinod
Hi Mark, On 11-01-21, 16:35, Mark Brown wrote: > On Mon, Jan 11, 2021 at 08:46:48PM +0530, Vinod Koul wrote: > > > +static int get_xfer_mode(struct spi_master *spi) > > +{ > > + struct spi_geni_master *mas = spi_master_get_devdata(spi); > > + struct geni_se *se = &mas->se; > > + int mode = GENI_SE_FIFO; > > Why not use the core DMA mapping support? Sorry I seemed to have missed replying to this one. Looking at the code, that is ideal case. Only issue I can see is that core DMA mapping device being used is incorrect. The core would use ctlr->dev.parent which is the spi0 device here. But in this case, that wont work. We have a parent qup device which is the parent for both spi and dma device and needs to be used for dma-mapping! If we allow drivers to set dma mapping device and use that, then I can reuse the core. Let me know if that is agreeable to you and I can hack this up. Maybe add a new member in spi_controller which is filled by drivers in can_dma() callback? Thanks
On Wed, Jun 16, 2021 at 02:20:45PM +0530, Vinod Koul wrote: > Looking at the code, that is ideal case. Only issue I can see is that > core DMA mapping device being used is incorrect. The core would use > ctlr->dev.parent which is the spi0 device here. Why would the parent of the controller be a SPI device? > But in this case, that wont work. We have a parent qup device which is > the parent for both spi and dma device and needs to be used for > dma-mapping! > If we allow drivers to set dma mapping device and use that, then I can > reuse the core. Let me know if that is agreeable to you and I can hack > this up. Maybe add a new member in spi_controller which is filled by > drivers in can_dma() callback? Possibly, I'd need to see the code.
On 16-06-21, 12:35, Mark Brown wrote: > On Wed, Jun 16, 2021 at 02:20:45PM +0530, Vinod Koul wrote: > > > Looking at the code, that is ideal case. Only issue I can see is that > > core DMA mapping device being used is incorrect. The core would use > > ctlr->dev.parent which is the spi0 device here. > > Why would the parent of the controller be a SPI device? Sorry my bad, I meant the core use ctlr->dev.parent which in this case is the SPI master device, 880000.spi > > But in this case, that wont work. We have a parent qup device which is > > the parent for both spi and dma device and needs to be used for > > dma-mapping! > > > If we allow drivers to set dma mapping device and use that, then I can > > reuse the core. Let me know if that is agreeable to you and I can hack > > this up. Maybe add a new member in spi_controller which is filled by > > drivers in can_dma() callback? > > Possibly, I'd need to see the code. Ok, let me do a prototype and share ... Thanks
On 16-06-21, 17:32, Vinod Koul wrote: > On 16-06-21, 12:35, Mark Brown wrote: > > On Wed, Jun 16, 2021 at 02:20:45PM +0530, Vinod Koul wrote: > > > But in this case, that wont work. We have a parent qup device which is > > > the parent for both spi and dma device and needs to be used for > > > dma-mapping! > > > > > If we allow drivers to set dma mapping device and use that, then I can > > > reuse the core. Let me know if that is agreeable to you and I can hack > > > this up. Maybe add a new member in spi_controller which is filled by > > > drivers in can_dma() callback? > > > > Possibly, I'd need to see the code. > > Ok, let me do a prototype and share ... So setting the dma_map_dev in the can_dma() callback does not work as we would need device before we invoke can_dma(), so modified this to be set earlier by driver (in driver probe, set the dma_map_dev) and use in __spi_map_msg(). With this it works for me & I was able to get rid of driver mapping code -- >8 -- diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index e353b7a9e54e..315f7e7545f7 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -961,11 +961,15 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg) if (ctlr->dma_tx) tx_dev = ctlr->dma_tx->device->dev; + else if (ctlr->dma_map_dev) + tx_dev = ctlr->dma_map_dev; else tx_dev = ctlr->dev.parent; if (ctlr->dma_rx) rx_dev = ctlr->dma_rx->device->dev; + else if (ctlr->dma_map_dev) + rx_dev = ctlr->dma_map_dev; else rx_dev = ctlr->dev.parent; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 74239d65c7fd..4d3f116f5723 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -586,6 +586,7 @@ struct spi_controller { bool (*can_dma)(struct spi_controller *ctlr, struct spi_device *spi, struct spi_transfer *xfer); + struct device *dma_map_dev; /* * These hooks are for drivers that want to use the generic