* [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma
@ 2021-06-18 14:18 Vinod Koul
2021-06-18 14:18 ` [PATCH v2 1/3] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header Vinod Koul
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Vinod Koul @ 2021-06-18 14:18 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, Vinod Koul, Andy Gross, Matthias Kaehlcke,
Douglas Anderson, Sumit Semwal, linux-kernel
This is version 2 of the GPI dma series [1]
This series add the GPI DMA in qcom geni drivers. For this we
first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common
headers and then add support in the gpi dma in geni driver.
The SPI and I2C driver changes shall follow shortly
[1]: http://lore.kernel.org/r/20210111151651.1616813-1-vkoul@kernel.org
Changes in v2:
- add r-b from Bjorn on patch 1
- add more details in log for patch 2
- Fix the comments from Doug and Bjorn for patch3, notable use read, modify
update for irq registers, use geni_se_irq_clear() for irq, dont update
single bit registers, add a bit more description for gpi dma etc
Vinod Koul (3):
soc: qcom: geni: move GENI_IF_DISABLE_RO to common header
soc: qcom: geni: move struct geni_wrapper to header
soc: qcom: geni: Add support for gpi dma
drivers/soc/qcom/qcom-geni-se.c | 47 ++++++++++++++++++++++-----------
include/linux/qcom-geni-se.h | 23 ++++++++++++++--
2 files changed, 52 insertions(+), 18 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header
2021-06-18 14:18 [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma Vinod Koul
@ 2021-06-18 14:18 ` Vinod Koul
2021-06-18 17:03 ` Bjorn Andersson
2021-06-18 14:18 ` [PATCH v2 2/3] soc: qcom: geni: move struct geni_wrapper to header Vinod Koul
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Vinod Koul @ 2021-06-18 14:18 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, Vinod Koul, Andy Gross, Matthias Kaehlcke,
Douglas Anderson, Sumit Semwal, linux-kernel
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
Also, add FIFO_IF_DISABLE define.
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/soc/qcom/qcom-geni-se.c | 1 -
include/linux/qcom-geni-se.h | 4 ++++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 5bdfb1565c14..fe666ea0c487 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -104,7 +104,6 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
#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 7c811eebcaab..5114e2144b17 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -63,6 +63,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
@@ -105,6 +106,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.31.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] soc: qcom: geni: move struct geni_wrapper to header
2021-06-18 14:18 [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma Vinod Koul
2021-06-18 14:18 ` [PATCH v2 1/3] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header Vinod Koul
@ 2021-06-18 14:18 ` Vinod Koul
2021-06-18 17:10 ` Bjorn Andersson
2021-06-18 14:18 ` [PATCH v2 3/3] soc: qcom: geni: Add support for gpi dma Vinod Koul
2021-06-18 16:59 ` [PATCH v2 0/3] soc: qcom: geni: add " Bjorn Andersson
3 siblings, 1 reply; 11+ messages in thread
From: Vinod Koul @ 2021-06-18 14:18 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, Vinod Koul, Andy Gross, Matthias Kaehlcke,
Douglas Anderson, Sumit Semwal, linux-kernel
SPI & I2C geni driver needs to access struct geni_wrapper, so move it to
header. The drivers needs this header to find the geni device and use it
in dma mapping.
Using this method works for both DT and ACPI systems
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/soc/qcom/qcom-geni-se.c | 14 --------------
include/linux/qcom-geni-se.h | 14 ++++++++++++++
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index fe666ea0c487..08d645b90ed3 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -78,20 +78,6 @@
*/
#define MAX_CLK_PERF_LEVEL 32
-#define NUM_AHB_CLKS 2
-
-/**
- * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
- * @dev: Device pointer of the QUP wrapper core
- * @base: Base address of this instance of QUP wrapper core
- * @ahb_clks: Handle to the primary & secondary AHB clocks
- * @to_core: Core ICC path
- */
-struct geni_wrapper {
- struct device *dev;
- void __iomem *base;
- struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
-};
static const char * const icc_path_names[] = {"qup-core", "qup-config",
"qup-memory"};
diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
index 5114e2144b17..5fda675c5cfe 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -38,6 +38,20 @@ struct geni_icc_path {
unsigned int avg_bw;
};
+#define NUM_AHB_CLKS 2
+
+/**
+ * @struct geni_wrapper - Data structure to represent the QUP Wrapper Core
+ * @dev: Device pointer of the QUP wrapper core
+ * @base: Base address of this instance of QUP wrapper core
+ * @ahb_clks: Handle to the primary & secondary AHB clocks
+ */
+struct geni_wrapper {
+ struct device *dev;
+ void __iomem *base;
+ struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
+};
+
/**
* struct geni_se - GENI Serial Engine
* @base: Base Address of the Serial Engine's register block
--
2.31.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] soc: qcom: geni: Add support for gpi dma
2021-06-18 14:18 [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma Vinod Koul
2021-06-18 14:18 ` [PATCH v2 1/3] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header Vinod Koul
2021-06-18 14:18 ` [PATCH v2 2/3] soc: qcom: geni: move struct geni_wrapper to header Vinod Koul
@ 2021-06-18 14:18 ` Vinod Koul
2021-06-18 17:23 ` Bjorn Andersson
2021-06-18 16:59 ` [PATCH v2 0/3] soc: qcom: geni: add " Bjorn Andersson
3 siblings, 1 reply; 11+ messages in thread
From: Vinod Koul @ 2021-06-18 14:18 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, Vinod Koul, Andy Gross, Matthias Kaehlcke,
Douglas Anderson, Sumit Semwal, linux-kernel
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 | 32 +++++++++++++++++++++++++++++++-
include/linux/qcom-geni-se.h | 5 +++--
2 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 08d645b90ed3..40a0a1f88070 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -307,6 +307,32 @@ static void geni_se_select_dma_mode(struct geni_se *se)
writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
}
+static void geni_se_select_gpi_mode(struct geni_se *se)
+{
+ unsigned int gpi_event_en;
+ unsigned int m_irq_en;
+ unsigned int s_irq_en;
+
+ geni_se_irq_clear(se);
+ writel(0, se->base + SE_IRQ_EN);
+
+ s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
+ s_irq_en &= ~S_CMD_DONE_EN;
+ writel(s_irq_en, se->base + SE_GENI_S_IRQ_EN);
+
+ m_irq_en = readl(se->base + SE_GENI_M_IRQ_EN);
+ m_irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
+ M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
+ writel(m_irq_en, se->base + SE_GENI_M_IRQ_EN);
+
+ writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
+
+ gpi_event_en = readl(se->base + SE_GSI_EVENT_EN);
+ gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
+ GENI_M_EVENT_EN | GENI_S_EVENT_EN);
+ writel(gpi_event_en, se->base + SE_GSI_EVENT_EN);
+}
+
/**
* geni_se_select_mode() - Select the serial engine transfer mode
* @se: Pointer to the concerned serial engine.
@@ -314,7 +340,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:
@@ -323,6 +350,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 5fda675c5cfe..336b682392b1 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -11,8 +11,9 @@
/* Transfer mode supported by GENI Serial Engines */
enum geni_se_xfer_mode {
GENI_SE_INVALID,
- GENI_SE_FIFO,
- GENI_SE_DMA,
+ GENI_SE_FIFO, /* FIFO mode */
+ GENI_GPI_DMA, /* GSI aka GPI DMA mode */
+ GENI_SE_DMA, /* SE DMA mode */
};
/* Protocols supported by GENI Serial Engines */
--
2.31.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma
2021-06-18 14:18 [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma Vinod Koul
` (2 preceding siblings ...)
2021-06-18 14:18 ` [PATCH v2 3/3] soc: qcom: geni: Add support for gpi dma Vinod Koul
@ 2021-06-18 16:59 ` Bjorn Andersson
2021-06-20 11:06 ` Vinod Koul
3 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2021-06-18 16:59 UTC (permalink / raw)
To: Vinod Koul
Cc: linux-arm-msm, Andy Gross, Matthias Kaehlcke, Douglas Anderson,
Sumit Semwal, linux-kernel
On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:
> This is version 2 of the GPI dma series [1]
>
> This series add the GPI DMA in qcom geni drivers. For this we
> first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common
> headers and then add support in the gpi dma in geni driver.
>
> The SPI and I2C driver changes shall follow shortly
>
Given that the continuation of this series will have build time
dependencies on these patches I think it would be good to see them all
as one chunk (and practically I can create a immutable branch of the
soc/qcom pieces and share with Wolfram and Mark).
Regards,
Bjorn
> [1]: http://lore.kernel.org/r/20210111151651.1616813-1-vkoul@kernel.org
>
> Changes in v2:
> - add r-b from Bjorn on patch 1
> - add more details in log for patch 2
> - Fix the comments from Doug and Bjorn for patch3, notable use read, modify
> update for irq registers, use geni_se_irq_clear() for irq, dont update
> single bit registers, add a bit more description for gpi dma etc
>
> Vinod Koul (3):
> soc: qcom: geni: move GENI_IF_DISABLE_RO to common header
> soc: qcom: geni: move struct geni_wrapper to header
> soc: qcom: geni: Add support for gpi dma
>
> drivers/soc/qcom/qcom-geni-se.c | 47 ++++++++++++++++++++++-----------
> include/linux/qcom-geni-se.h | 23 ++++++++++++++--
> 2 files changed, 52 insertions(+), 18 deletions(-)
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header
2021-06-18 14:18 ` [PATCH v2 1/3] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header Vinod Koul
@ 2021-06-18 17:03 ` Bjorn Andersson
2021-06-22 12:13 ` Vinod Koul
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2021-06-18 17:03 UTC (permalink / raw)
To: Vinod Koul
Cc: linux-arm-msm, Andy Gross, Matthias Kaehlcke, Douglas Anderson,
Sumit Semwal, linux-kernel
On Fri 18 Jun 09:18 CDT 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
>
> Also, add FIFO_IF_DISABLE define.
>
Afaict these registers relates to the hardware block that is primarily
owned by the individual engine-drivers, would it not make sense to move
them all to the shared header file?
(The patch itself still looks ok, but needs the consuming patch to be
merged)
Regards,
Bjorn
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/soc/qcom/qcom-geni-se.c | 1 -
> include/linux/qcom-geni-se.h | 4 ++++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 5bdfb1565c14..fe666ea0c487 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -104,7 +104,6 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
> #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 7c811eebcaab..5114e2144b17 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -63,6 +63,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
> @@ -105,6 +106,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.31.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] soc: qcom: geni: move struct geni_wrapper to header
2021-06-18 14:18 ` [PATCH v2 2/3] soc: qcom: geni: move struct geni_wrapper to header Vinod Koul
@ 2021-06-18 17:10 ` Bjorn Andersson
0 siblings, 0 replies; 11+ messages in thread
From: Bjorn Andersson @ 2021-06-18 17:10 UTC (permalink / raw)
To: Vinod Koul
Cc: linux-arm-msm, Andy Gross, Matthias Kaehlcke, Douglas Anderson,
Sumit Semwal, linux-kernel
On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:
> SPI & I2C geni driver needs to access struct geni_wrapper, so move it to
> header. The drivers needs this header to find the geni device and use it
> in dma mapping.
>
How does this differ from engine->dev->parent?
> Using this method works for both DT and ACPI systems
>
I was under the impression that the wrapper and engines are describe
completely independently in ACPI, so we don't have a link between them.
If that's not the case, then I guess that answers the above question
about ->parent.
Regards,
Bjorn
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/soc/qcom/qcom-geni-se.c | 14 --------------
> include/linux/qcom-geni-se.h | 14 ++++++++++++++
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index fe666ea0c487..08d645b90ed3 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -78,20 +78,6 @@
> */
>
> #define MAX_CLK_PERF_LEVEL 32
> -#define NUM_AHB_CLKS 2
> -
> -/**
> - * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
> - * @dev: Device pointer of the QUP wrapper core
> - * @base: Base address of this instance of QUP wrapper core
> - * @ahb_clks: Handle to the primary & secondary AHB clocks
> - * @to_core: Core ICC path
> - */
> -struct geni_wrapper {
> - struct device *dev;
> - void __iomem *base;
> - struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> -};
>
> static const char * const icc_path_names[] = {"qup-core", "qup-config",
> "qup-memory"};
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index 5114e2144b17..5fda675c5cfe 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -38,6 +38,20 @@ struct geni_icc_path {
> unsigned int avg_bw;
> };
>
> +#define NUM_AHB_CLKS 2
> +
> +/**
> + * @struct geni_wrapper - Data structure to represent the QUP Wrapper Core
> + * @dev: Device pointer of the QUP wrapper core
> + * @base: Base address of this instance of QUP wrapper core
> + * @ahb_clks: Handle to the primary & secondary AHB clocks
> + */
> +struct geni_wrapper {
> + struct device *dev;
> + void __iomem *base;
> + struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> +};
> +
> /**
> * struct geni_se - GENI Serial Engine
> * @base: Base Address of the Serial Engine's register block
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] soc: qcom: geni: Add support for gpi dma
2021-06-18 14:18 ` [PATCH v2 3/3] soc: qcom: geni: Add support for gpi dma Vinod Koul
@ 2021-06-18 17:23 ` Bjorn Andersson
2021-06-22 12:15 ` Vinod Koul
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2021-06-18 17:23 UTC (permalink / raw)
To: Vinod Koul
Cc: linux-arm-msm, Andy Gross, Matthias Kaehlcke, Douglas Anderson,
Sumit Semwal, linux-kernel
On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:
> GPI DMA is one of the DMA modes supported on geni, this adds support to
> enable that mode
I think you're missing an opportunity to describe what GPI DMA is.
Perhaps something like:
In GPI DMA mode the serial engine uses the DMA controller found in the
associated wrapper to perform its transaction, add support for enabling
this mode in the engine.
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/soc/qcom/qcom-geni-se.c | 32 +++++++++++++++++++++++++++++++-
> include/linux/qcom-geni-se.h | 5 +++--
> 2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 08d645b90ed3..40a0a1f88070 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -307,6 +307,32 @@ static void geni_se_select_dma_mode(struct geni_se *se)
> writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
> }
>
> +static void geni_se_select_gpi_mode(struct geni_se *se)
> +{
> + unsigned int gpi_event_en;
> + unsigned int m_irq_en;
> + unsigned int s_irq_en;
readl and writel operates on u32, so better to use that.
> +
> + geni_se_irq_clear(se);
> + writel(0, se->base + SE_IRQ_EN);
> +
> + s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
I don't mind this being _relaxed if done on purpose, but as it's the
only one (and there's no comment) I get the feeling that it's a mistake.
> + s_irq_en &= ~S_CMD_DONE_EN;
> + writel(s_irq_en, se->base + SE_GENI_S_IRQ_EN);
The use of 3 different variables (gpi_event_en, m_irq_en, s_irq_en)
forced me to really look if the three sets of operations somehow reused
previous results.
To clarify that this isn't the case I would suggest that you use a
single variable ("val"?) instead.
> +
> + m_irq_en = readl(se->base + SE_GENI_M_IRQ_EN);
> + m_irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> + writel(m_irq_en, se->base + SE_GENI_M_IRQ_EN);
> +
> + writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
> +
> + gpi_event_en = readl(se->base + SE_GSI_EVENT_EN);
> + gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> + GENI_M_EVENT_EN | GENI_S_EVENT_EN);
> + writel(gpi_event_en, se->base + SE_GSI_EVENT_EN);
> +}
> +
> /**
> * geni_se_select_mode() - Select the serial engine transfer mode
> * @se: Pointer to the concerned serial engine.
> @@ -314,7 +340,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);
This line can be left unbroken.
>
> switch (mode) {
> case GENI_SE_FIFO:
> @@ -323,6 +350,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 5fda675c5cfe..336b682392b1 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -11,8 +11,9 @@
> /* Transfer mode supported by GENI Serial Engines */
> enum geni_se_xfer_mode {
> GENI_SE_INVALID,
> - GENI_SE_FIFO,
> - GENI_SE_DMA,
Is the order significant? Is there a reason why SE_DMA ended up last?
> + GENI_SE_FIFO, /* FIFO mode */
> + GENI_GPI_DMA, /* GSI aka GPI DMA mode */
If the TLA soup is too soupy, then perhaps we should just document it as
"external DMA" or "Use wrapper's DMA controller"?
And perhaps use kernel-doc instead?
Regards,
Bjorn
> + GENI_SE_DMA, /* SE DMA mode */
> };
>
> /* Protocols supported by GENI Serial Engines */
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma
2021-06-18 16:59 ` [PATCH v2 0/3] soc: qcom: geni: add " Bjorn Andersson
@ 2021-06-20 11:06 ` Vinod Koul
0 siblings, 0 replies; 11+ messages in thread
From: Vinod Koul @ 2021-06-20 11:06 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, Andy Gross, Matthias Kaehlcke, Douglas Anderson,
Sumit Semwal, linux-kernel
On 18-06-21, 11:59, Bjorn Andersson wrote:
> On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:
>
> > This is version 2 of the GPI dma series [1]
> >
> > This series add the GPI DMA in qcom geni drivers. For this we
> > first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common
> > headers and then add support in the gpi dma in geni driver.
> >
> > The SPI and I2C driver changes shall follow shortly
> >
>
> Given that the continuation of this series will have build time
> dependencies on these patches I think it would be good to see them all
> as one chunk (and practically I can create a immutable branch of the
> soc/qcom pieces and share with Wolfram and Mark).
Okay, let me post full series in few days
>
> Regards,
> Bjorn
>
> > [1]: http://lore.kernel.org/r/20210111151651.1616813-1-vkoul@kernel.org
> >
> > Changes in v2:
> > - add r-b from Bjorn on patch 1
> > - add more details in log for patch 2
> > - Fix the comments from Doug and Bjorn for patch3, notable use read, modify
> > update for irq registers, use geni_se_irq_clear() for irq, dont update
> > single bit registers, add a bit more description for gpi dma etc
> >
> > Vinod Koul (3):
> > soc: qcom: geni: move GENI_IF_DISABLE_RO to common header
> > soc: qcom: geni: move struct geni_wrapper to header
> > soc: qcom: geni: Add support for gpi dma
> >
> > drivers/soc/qcom/qcom-geni-se.c | 47 ++++++++++++++++++++++-----------
> > include/linux/qcom-geni-se.h | 23 ++++++++++++++--
> > 2 files changed, 52 insertions(+), 18 deletions(-)
> >
> > --
> > 2.31.1
> >
--
~Vinod
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header
2021-06-18 17:03 ` Bjorn Andersson
@ 2021-06-22 12:13 ` Vinod Koul
0 siblings, 0 replies; 11+ messages in thread
From: Vinod Koul @ 2021-06-22 12:13 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, Andy Gross, Matthias Kaehlcke, Douglas Anderson,
Sumit Semwal, linux-kernel
On 18-06-21, 12:03, Bjorn Andersson wrote:
> On Fri 18 Jun 09:18 CDT 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
> >
> > Also, add FIFO_IF_DISABLE define.
> >
>
> Afaict these registers relates to the hardware block that is primarily
> owned by the individual engine-drivers, would it not make sense to move
> them all to the shared header file?
the GENI_IF_DISABLE_RO is used by SPI and I2C drivers, so we would create two
copies. So better to be defined in geni header
--
~Vinod
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] soc: qcom: geni: Add support for gpi dma
2021-06-18 17:23 ` Bjorn Andersson
@ 2021-06-22 12:15 ` Vinod Koul
0 siblings, 0 replies; 11+ messages in thread
From: Vinod Koul @ 2021-06-22 12:15 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, Andy Gross, Matthias Kaehlcke, Douglas Anderson,
Sumit Semwal, linux-kernel
On 18-06-21, 12:23, Bjorn Andersson wrote:
> On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:
>
> > GPI DMA is one of the DMA modes supported on geni, this adds support to
> > enable that mode
>
> I think you're missing an opportunity to describe what GPI DMA is.
> Perhaps something like:
>
> In GPI DMA mode the serial engine uses the DMA controller found in the
> associated wrapper to perform its transaction, add support for enabling
> this mode in the engine.
Hmm looks like I did, I will update more kernel-doc style comments to
explain more details
>
> >
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> > drivers/soc/qcom/qcom-geni-se.c | 32 +++++++++++++++++++++++++++++++-
> > include/linux/qcom-geni-se.h | 5 +++--
> > 2 files changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> > index 08d645b90ed3..40a0a1f88070 100644
> > --- a/drivers/soc/qcom/qcom-geni-se.c
> > +++ b/drivers/soc/qcom/qcom-geni-se.c
> > @@ -307,6 +307,32 @@ static void geni_se_select_dma_mode(struct geni_se *se)
> > writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
> > }
> >
> > +static void geni_se_select_gpi_mode(struct geni_se *se)
> > +{
> > + unsigned int gpi_event_en;
> > + unsigned int m_irq_en;
> > + unsigned int s_irq_en;
>
> readl and writel operates on u32, so better to use that.
Yes
>
> > +
> > + geni_se_irq_clear(se);
> > + writel(0, se->base + SE_IRQ_EN);
> > +
> > + s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
>
> I don't mind this being _relaxed if done on purpose, but as it's the
> only one (and there's no comment) I get the feeling that it's a mistake.
Yes it is a leftover, fixed now
>
> > + s_irq_en &= ~S_CMD_DONE_EN;
> > + writel(s_irq_en, se->base + SE_GENI_S_IRQ_EN);
>
> The use of 3 different variables (gpi_event_en, m_irq_en, s_irq_en)
> forced me to really look if the three sets of operations somehow reused
> previous results.
>
> To clarify that this isn't the case I would suggest that you use a
> single variable ("val"?) instead.
Yes updated
>
> > +
> > + m_irq_en = readl(se->base + SE_GENI_M_IRQ_EN);
> > + m_irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> > + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> > + writel(m_irq_en, se->base + SE_GENI_M_IRQ_EN);
> > +
> > + writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
> > +
> > + gpi_event_en = readl(se->base + SE_GSI_EVENT_EN);
> > + gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> > + GENI_M_EVENT_EN | GENI_S_EVENT_EN);
> > + writel(gpi_event_en, se->base + SE_GSI_EVENT_EN);
> > +}
> > +
> > /**
> > * geni_se_select_mode() - Select the serial engine transfer mode
> > * @se: Pointer to the concerned serial engine.
> > @@ -314,7 +340,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);
>
> This line can be left unbroken.
yaay 100 chars :-) will do
>
> >
> > switch (mode) {
> > case GENI_SE_FIFO:
> > @@ -323,6 +350,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 5fda675c5cfe..336b682392b1 100644
> > --- a/include/linux/qcom-geni-se.h
> > +++ b/include/linux/qcom-geni-se.h
> > @@ -11,8 +11,9 @@
> > /* Transfer mode supported by GENI Serial Engines */
> > enum geni_se_xfer_mode {
> > GENI_SE_INVALID,
> > - GENI_SE_FIFO,
> > - GENI_SE_DMA,
>
> Is the order significant? Is there a reason why SE_DMA ended up last?
Nope not that I can tjing off.. I can add this as last!
--
~Vinod
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-22 12:15 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18 14:18 [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma Vinod Koul
2021-06-18 14:18 ` [PATCH v2 1/3] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header Vinod Koul
2021-06-18 17:03 ` Bjorn Andersson
2021-06-22 12:13 ` Vinod Koul
2021-06-18 14:18 ` [PATCH v2 2/3] soc: qcom: geni: move struct geni_wrapper to header Vinod Koul
2021-06-18 17:10 ` Bjorn Andersson
2021-06-18 14:18 ` [PATCH v2 3/3] soc: qcom: geni: Add support for gpi dma Vinod Koul
2021-06-18 17:23 ` Bjorn Andersson
2021-06-22 12:15 ` Vinod Koul
2021-06-18 16:59 ` [PATCH v2 0/3] soc: qcom: geni: add " Bjorn Andersson
2021-06-20 11:06 ` Vinod Koul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).