Re: [PATCH v3 2/4] dma: gpi: Add Lock and Unlock TRE support to access SE exclusively

Vinod Koul
2024-10-14T18:32:00Z

On 27-09-24, 12:01, Mukesh Kumar Savaliya wrote:
> GSI DMA provides specific TREs namely Lock and Unlock TRE, which
> provides mutual exclusive access to SE from any of the subsystem
> (E.g. Apps, TZ, ADSP etc). Lock prevents other subsystems from
> concurrently performing DMA transfers and avoids disturbance to
> data path. Basically for shared SE usecase, lock the SE for
> particular subsystem, complete the transfer, unlock the SE.

it is dmaengine: xxx so please update that

>
> Apply Lock TRE for the first transfer of shared SE and Apply Unlock
> TRE for the last transfer.
>
> Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
>
> TRE = Transfer Ring Element, refers to the queued descriptor.
> SE = Serial Engine
> SS = Subsystems (Apps processor, TZ, ADSP, Modem)
>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
> drivers/dma/qcom/gpi.c | 37 +++++++++++++++++++++++++++++++-
> include/linux/dma/qcom-gpi-dma.h | 6 ++++++
> 2 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index 52a7c8f2498f..120d91234442 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -2,6 +2,7 @@
> /*
> * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
> * Copyright (c) 2020, Linaro Limited
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> */
>
> #include <dt-bindings/dma/qcom-gpi.h>
> @@ -65,6 +66,14 @@
> /* DMA TRE */
> #define TRE_DMA_LEN GENMASK(23, 0)
>
> +/* Lock TRE */
> +#define TRE_I2C_LOCK BIT(0)
> +#define TRE_MINOR_TYPE GENMASK(19, 16)
> +#define TRE_MAJOR_TYPE GENMASK(23, 20)
> +
> +/* Unlock TRE */
> +#define TRE_I2C_UNLOCK BIT(8)
> +
> /* Register offsets from gpi-top */
> #define GPII_n_CH_k_CNTXT_0_OFFS(n, k) (0x20000 + (0x4000 * (n)) + (0x80 * (k)))
> #define GPII_n_CH_k_CNTXT_0_EL_SIZE GENMASK(31, 24)
> @@ -516,7 +525,7 @@ struct gpii {
> bool ieob_set;
> };
>
> -#define MAX_TRE 3
> +#define MAX_TRE 5
>
> struct gpi_desc {
> struct virt_dma_desc vd;
> @@ -1637,6 +1646,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
> struct gpi_tre *tre;
> unsigned int i;
>
> + /* create lock tre for first tranfser */
> + if (i2c->shared_se && i2c->first_msg) {
> + tre = &desc->tre[tre_idx];
> + tre_idx++;
> +
> + tre->dword[0] = 0;
> + tre->dword[1] = 0;
> + tre->dword[2] = 0;
> + tre->dword[3] = u32_encode_bits(1, TRE_I2C_LOCK);
> + tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
> + tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> + }
> +
> /* first create config tre if applicable */
> if (i2c->set_config) {
> tre = &desc->tre[tre_idx];
> @@ -1695,6 +1717,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
> tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT);
> }
>
> + /* Unlock tre for last transfer */
> + if (i2c->shared_se && i2c->last_msg && i2c->op != I2C_READ) {
> + tre = &desc->tre[tre_idx];
> + tre_idx++;
> +
> + tre->dword[0] = 0;
> + tre->dword[1] = 0;
> + tre->dword[2] = 0;
> + tre->dword[3] = u32_encode_bits(1, TRE_I2C_UNLOCK);
> + tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE);
> + tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> + }
> +
> for (i = 0; i < tre_idx; i++)
> dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
> desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
> diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
> index 6680dd1a43c6..8589c711afae 100644
> --- a/include/linux/dma/qcom-gpi-dma.h
> +++ b/include/linux/dma/qcom-gpi-dma.h
> @@ -65,6 +65,9 @@ enum i2c_op {
> * @rx_len: receive length for buffer
> * @op: i2c cmd
> * @muli-msg: is part of multi i2c r-w msgs
> + * @shared_se: bus is shared between subsystems
> + * @bool first_msg: use it for tracking multimessage xfer
> + * @bool last_msg: use it for tracking multimessage xfer
> */
> struct gpi_i2c_config {
> u8 set_config;
> @@ -78,6 +81,9 @@ struct gpi_i2c_config {
> u32 rx_len;
> enum i2c_op op;
> bool multi_msg;
> + bool shared_se;
> + bool first_msg;
> + bool last_msg;

Looking at the usage in following patches, why cant this be handled
internally as part of prep call?

> };
>
> #endif /* QCOM_GPI_DMA_H */
> --
> 2.25.1

--
~Vinod