diff mbox series

[3/7] soc: qcom: geni: Add support for gpi dma

Message ID 20210111151651.1616813-4-vkoul@kernel.org
State New
Headers show
Series Add and enable GPI DMA users | expand

Commit Message

Vinod Koul Jan. 11, 2021, 3:16 p.m. UTC
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(-)

Comments

Bjorn Andersson Jan. 11, 2021, 3:40 p.m. UTC | #1
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
>
Doug Anderson Jan. 13, 2021, 12:01 a.m. UTC | #2
Hi,

On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> +static int geni_se_select_gpi_mode(struct geni_se *se)
> +{
> +       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;
> +
> +       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);

Do you really need to do a read/modify/write of SE_GENI_DMA_MODE_EN?
It's a register with a single bit in it.  Just set the bit, no?  There
are cases where read-modify-write is the correct thing to do but IMO
it shouldn't be the default way of working.  If code is initting a
register to a default state it should just be setting the register.
If some later incarnation of the hardware comes along and adds extra
bits to this register then, sure, we might have to modify the code.
However, it has the advantage where we aren't left at the whims of
whatever was programmed by whatever version of whatever firmware was
running on the device.  I've been bitten way more often by firmware
leaving registers in some random / unexpected state than by new bits
introduced by new versions of hardware.

...same for other registers in your patch that you can just init.

(this is true throughout lots of Qualcomm code, but I figure might as
well start trying to do something about it?)


> +       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);
> +
> +       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);

Last time I touched this bit of code I think folks agreed that it
would be better to move managing of the interrupt enables out of the
common code and move them to the various drivers using geni [1].  I
was hoping that someone from Qualcomm would be able to pick this up.
Managing them in the wrapper just ends up causing a whole bunch of
special cases.  Any chance you could take that on as part of your
series?

Presumably if this was mananged in individual drivers you also might
be able to do less read-modify-write type stuff, too...

[1] https://lore.kernel.org/linux-arm-msm/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/


> +       writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR);
> +       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);

This looks mostly like geni_se_irq_clear().  Should they somehow share code?


> 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,

Add a comment like "Also known as GSI" here to help people figure out
what's going on when they're trying to parse the manual or #defines
like "SE_GSI_EVENT_EN"


> @@ -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))

Maybe this define belonged in patch #1?  It doesn't seem related to this patch?
Vinod Koul Jan. 13, 2021, 3:22 a.m. UTC | #3
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:
> >
> > +static int geni_se_select_gpi_mode(struct geni_se *se)
> > +{
> > +       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;
> > +
> > +       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);
> 
> Do you really need to do a read/modify/write of SE_GENI_DMA_MODE_EN?
> It's a register with a single bit in it.  Just set the bit, no?  There
> are cases where read-modify-write is the correct thing to do but IMO
> it shouldn't be the default way of working.  If code is initting a
> register to a default state it should just be setting the register.
> If some later incarnation of the hardware comes along and adds extra
> bits to this register then, sure, we might have to modify the code.
> However, it has the advantage where we aren't left at the whims of
> whatever was programmed by whatever version of whatever firmware was
> running on the device.  I've been bitten way more often by firmware
> leaving registers in some random / unexpected state than by new bits
> introduced by new versions of hardware.
> 
> ...same for other registers in your patch that you can just init.

Yes sounds right to me. I will review this and other register writes and
update the code

> (this is true throughout lots of Qualcomm code, but I figure might as
> well start trying to do something about it?)

yep, we can always start somewhere :)

> > +       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);
> > +
> > +       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);
> 
> Last time I touched this bit of code I think folks agreed that it
> would be better to move managing of the interrupt enables out of the
> common code and move them to the various drivers using geni [1].  I
> was hoping that someone from Qualcomm would be able to pick this up.
> Managing them in the wrapper just ends up causing a whole bunch of
> special cases.  Any chance you could take that on as part of your
> series?
> 
> Presumably if this was mananged in individual drivers you also might
> be able to do less read-modify-write type stuff, too...
> 
> [1] https://lore.kernel.org/linux-arm-msm/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/

If it is okay, I can take this up but after this series. We can handle
the move independently as that is certainly a good thing to do.

> > +       writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR);
> > +       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);
> 
> This looks mostly like geni_se_irq_clear().  Should they somehow share code?

Mostly seems similar, let me move over that and verify

> > 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,
> 
> Add a comment like "Also known as GSI" here to help people figure out
> what's going on when they're trying to parse the manual or #defines
> like "SE_GSI_EVENT_EN"

Sure will add GSI.

> > @@ -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))
> 
> Maybe this define belonged in patch #1?  It doesn't seem related to this patch?

Yes the bit defines should go with the register.
diff mbox series

Patch

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)
+{
+	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;
+
+	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);
+
+	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);
+	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);
+
+	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