Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210111151651.1616813-1-vkoul@kernel.org> <20210111151651.1616813-4-vkoul@kernel.org>
In-Reply-To: <20210111151651.1616813-4-vkoul@kernel.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 12 Jan 2021 16:01:30 -0800
Message-ID: <CAD=FV=UOR2vxo7_gpjiv-djagq5bVwztCuDoCj5kjY7UtXR2PQ@mail.gmail.com>
Subject: Re: [PATCH 3/7] soc: qcom: geni: Add support for gpi dma
To:     Vinod Koul <vkoul@kernel.org>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mark Brown <broonie@kernel.org>, Wolfram Sang <wsa@kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Andy Gross <agross@kernel.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Amit Pundir <amit.pundir@linaro.org>,
        linux-spi <linux-spi@vger.kernel.org>, linux-i2c@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Roja Rani Yarubandi <rojay@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?