Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Apr 2022 20:57:20 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Vinod Koul <vkoul@kernel.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] arm64: dts: qcom: sm8350: Add GENI I2C/SPI DMA
 channels
Message-ID: <YlZKIKR1iHO+Btwk@ripper>
References: <20220412215137.2385831-1-bjorn.andersson@linaro.org>
 <20220412215137.2385831-2-bjorn.andersson@linaro.org>
 <YlZALC0DdSPB9n+/@matsya>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YlZALC0DdSPB9n+/@matsya>
Precedence: bulk

On Tue 12 Apr 20:14 PDT 2022, Vinod Koul wrote:

> On 12-04-22, 14:51, Bjorn Andersson wrote:
> > The GENI I2C and SPI controllers may use the GPI DMA engine, define the
> > rx and tx channels for these controllers to enable this.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8350.dtsi | 108 +++++++++++++++++++++++++++
> >  1 file changed, 108 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > index 7e585d9e4c68..8547c0b2f060 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > @@ -721,6 +721,9 @@ i2c14: i2c@880000 {
> >  				pinctrl-names = "default";
> >  				pinctrl-0 = <&qup_i2c14_default>;
> >  				interrupts = <GIC_SPI 373 IRQ_TYPE_LEVEL_HIGH>;
> > +				dmas = <&gpi_dma2 0 0 QCOM_GPI_I2C>,
> > +				       <&gpi_dma2 1 0 QCOM_GPI_I2C>;
> > +				dma-names = "tx", "rx";
> 
> I have been thinking about this. I dont feel this is right approach here
> as this is board dependent and having the firmware loaded on the board..
> 
> This was tested on HDK and can fail in MTP or other boards.. which might
> be in FIFO mode
> 

But if the controller is in FIFO mode, then !fifo_disable so we wouldn't
try to pick up the dmas. And in the opposite case, i.e. when
fifo_disable, the introduction of the GPI implementation meant that the
i2c driver wouldn't no longer probe without the dmas specified.

Unfortunately we don't have any i2c busses enabled on the MTP currently,
so I'm not able to validate this easily.


For the SPI driver though, the same logic is used to invoke
spi_geni_grab_gpi_chan(). So dmas will only be considered if
fifo_disabled is set.

That said, in the even that the SPI driver finds a fifo_disabled
controller and dma_request_chan() returns an error, we will fall back to
fifo mode instead. I'm not sure if that's desirable...

If that makes sense, we should at least handle EPROBE_DEFER instead of
falling through to fifo mode.

Regards,
Bjorn

> So, I think it might be apt to move these to board dtsi.. what do you
> think?
> 
> -- 
> ~Vinod