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;
From:   Robin Gong <yibin.gong@nxp.com>
To:     Mark Brown <broonie@kernel.org>, Vinod Koul <vkoul@kernel.org>
CC:     "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "festevam@gmail.com" <festevam@gmail.com>,
        "robin.murphy@arm.com" <robin.murphy@arm.com>,
        "matthias.schiffer@ew.tq-group.com" 
        <matthias.schiffer@ew.tq-group.com>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>,
        dl-linux-imx <linux-imx@nxp.com>,
        "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v1 RFC 1/2] spi: introduce fallback to pio
Thread-Topic: [PATCH v1 RFC 1/2] spi: introduce fallback to pio
Thread-Index: AQHWP6z7CX2etPvPnk+oddH8JEW5FKjTbHIAgADDYUCAAJUwgIAACjoQ
Date:   Fri, 12 Jun 2020 13:48:41 +0000
Message-ID: <VE1PR04MB66384013797FE6B01943F2A889810@VE1PR04MB6638.eurprd04.prod.outlook.com>
References: <1591880310-1813-1-git-send-email-yibin.gong@nxp.com>
 <1591880310-1813-2-git-send-email-yibin.gong@nxp.com>
 <20200611134042.GG4671@sirena.org.uk>
 <VE1PR04MB66383245FAD2AE33CFEA76F789810@VE1PR04MB6638.eurprd04.prod.outlook.com>
 <20200612101357.GA5396@sirena.org.uk>
In-Reply-To: <20200612101357.GA5396@sirena.org.uk>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 4acdc9ae-97ab-4a33-187d-08d80ed7518c
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jun 2020 13:48:41.9259
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Eri9epfNICapusUFyS4dw+S2KaQHhFVxPrEQ4SpeKWCC3kRWByOvOyWm3oqSvCYQ3oL9lOHNqCUwNvx220Uvtg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6512
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2020/06/12 18:14 Mark Brown <broonie@kernel.org> wrote:=20
> On Fri, Jun 12, 2020 at 02:18:32AM +0000, Robin Gong wrote:
> > On 2020/06/11 21: 41 Mark Brown <broonie@kernel.org> wrote:
>=20
> Please look at the formatting of your e-mails - they're really hard to re=
ad.  The
> line length is over 80 columns and there's no breaks between paragraphs.
Sorry for that, seems my outlook format issue, hope it's ok now this time :=
)

>=20
> > > If we were going to do this I don't see why we'd have a flag for
> > > this rather than just doing it unconditionally but...
>=20
> > What do you mean flag here, 'master->flags' or SPI_MASTER_FALLBACK?
> 'master->flags'
> > could let client fallback to PIO finally and spi core clear this flag
> > once this transfer done, so that DMA could be tried again in the next t=
ransfer.
> Client could enable this feature by choosing SPI_MASTER_FALLBACK freely
> without any impact on others.
>=20
> SPI_MASTER_FALLBACK.  If this works why would any driver not enable the
> flag?
Just make sure little impact if it's not good enough and potential issue ma=
y
come out after it's merged into mainline. TBH, I'm not sure if it has taken
care all in spi core. Besides, I don't know if other spi client need this f=
eature.

>=20
> > > ...I don't think this can work sensibly - this is going to try PIO
> > > if there's *any* error.  We might have had some sort of issue during
> > > the transfer for example so have some noise on the bus.  Like I said
> > > on a prior version of this I really
>=20
> > Any error happen in DMA could fallback to PIO , seems a nice to have,
> because it could
> > give chance to run in PIO which is more reliable. But if there is also =
error in
> PIO, thus may loop here, it's better adding limit try times here?
>=20
> An error doesn't mean nothing happened on the bus, an error could for
> example also be something like a FIFO overrun which corrupts data.
Do you mean fallback to PIO may cause FIFO overrun since some latency
involved so that this patch seems not useful as expected?

>=20
> > > think that we need to be figuring out if the DMA controller can
> > > support the transaction before we even map the buffer for it, having
> > > the controller just randomly fail underneath the consumer just does n=
ot
> sound robust.
>=20
> > But dmaengine_prep_slave_sg still may return failure even if anything
> > about DMA is ok before spi transfer start, such as dma description
> > malloc failure. This patch seems could make spi a bit robust...
>=20
> It *could* but only in extreme situations, and again this isn't just hand=
ling
> errors from failure to prepare the hardware but also anything that happen=
s
> after it.
Okay, understood your point. You prefer to some interface provided by dma
engine before dmaengine_prep_slave_sg so that can_dma() can know if
this dma channel is ready indeed. But unfortunately, seems there is no one.=
...

dmaengine_slave_config is the best one I think probably do that, but the
'direction' is deprecated and Vinod may remove it in the future. That's
important for sdma since sdma need to know the load_address of script which
is running on this channel by checking 'direction', then, could know if
the script is ready or not(ram script not ready if sdma firmware not loaded=
, but
rom script should be ready always). But now, as it's 'deprecated', until
dmaengine_prep_* phase sdma could know what's the load_address of script
running on this dma channel and if it's ready or not(firmware loaded or not=
).
commit 107d06441b70 ("dmaengine: imx-sdma: remove dma_slave_config directio=
n usage and leave sdma_event_enable() ")

Hi @Vinod Koul, sorry for pulling you, Could we keep 'direction' in dma_sla=
ve_config?
This's useful for checking if the script used on channel is ready or not in
dmaengine_slave_config phase so that can easily for spi or other drivers de=
cide to
start dma rather than the last dmaengine_prep_* phase where dma buffers hav=
e
been already map in spi core. =20
=20