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:   "M Tamseel Shams" <m.shams@samsung.com>
To:     "'Krzysztof Kozlowski'" <krzysztof.kozlowski@canonical.com>,
        <kgene@kernel.org>, <gregkh@linuxfoundation.org>, <jslaby@suse.com>
Cc:     <linux-arm-kernel@lists.infradead.org>,
        <linux-samsung-soc@vger.kernel.org>,
        <linux-serial@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <alim.akhtar@samsung.com>, <ajaykumar.rs@samsung.com>
In-Reply-To: <8935a448-04b7-91ce-203a-9f0d7e377052@canonical.com>
Subject: RE: [PATCH] serial: samsung: use dma_ops of DMA if attached
Date:   Mon, 21 Jun 2021 18:51:23 +0530
Message-ID: <004f01d766a0$567b9860$0372c920$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQGDfL5lNG5rzFQPe+FwbDp9GpLyVQGDsg9dAbmelg2rrFi5MA==
Content-Language: en-us
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
References: <CGME20210621044517epcas5p187affa518a18a3d019deb0c189cd8396@epcas5p1.samsung.com>
        <20210621044916.41564-1-m.shams@samsung.com>
        <8935a448-04b7-91ce-203a-9f0d7e377052@canonical.com>
Precedence: bulk

Hi,

>=20
> Hi,
>=20
> Thanks for the patch.
>=20
> On 21/06/2021 06:49, Tamseel Shams wrote:
> > When DMA is used for TX and RX by serial driver, it should pass the
> > DMA device pointer to DMA API instead of UART device pointer.
>=20
> Hmmm, but why DMA device pointer should be used?
>=20
> >
> > This patch is necessary to fix the SMMU page faults which is observed
> > when a DMA(with SMMU enabled) is attached to UART for transfer.
> >
> > Signed-off-by: Tamseel Shams <m.shams=40samsung.com>
> > Signed-off-by: Ajay Kumar <ajaykumar.rs=40samsung.com>
> > ---
> >  drivers/tty/serial/samsung_tty.c =7C 60
> > +++++++++++++++++++++++++-------
> >  1 file changed, 48 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index b923683e6a25..5bdc7dd2a5e2 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > =40=40 -284,8 +284,13 =40=40 static void s3c24xx_serial_stop_tx(struct =
uart_port
> *port)
> >  	struct s3c24xx_uart_dma *dma =3D ourport->dma;
> >  	struct circ_buf *xmit =3D &port->state->xmit;
> >  	struct dma_tx_state state;
> > +	struct device *dma_map_ops_dev =3D ourport->port.dev;
> >  	int count;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
>=20
> You mention here and further comments =22dma_ops=22. I don't see you chan=
ging
> the DMA ops, but the device. It's quite confusing. I think you meant a DM=
A
> device shall be passed to DMA API?
>=20
Yes, DMA device should be used for DMA API because only the DMA device is a=
ware of
how the device connects to the memory. There might be an extra level of add=
ress translation
due to a SMMU attached to the DMA device. When serial device pointer device=
 is used
for DMA API, the DMA API will have no clue of the SMMU attached to the DMA =
device.

> Second question: you write that DMA devices should be used if DMA is atta=
ched
> and in the code you follow such pattern a lot:
>=20
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev =3D dma->tx_chan->device->dev;
> > +
>=20
> Are you trying to say that if DMA is not attached, UART device should be =
used? If
> DMA is not attached, how are the DMA operations used then?
>=20
If DMA is not attached, this part of code related to dma_engine or DMA API =
do not
get called. There will not be any DMA operations at all.

> >  	if (=21ourport->tx_enabled)
> >  		return;
> >
> > =40=40 -298,7 +303,7 =40=40 static void s3c24xx_serial_stop_tx(struct u=
art_port
> *port)
> >  		dmaengine_pause(dma->tx_chan);
> >  		dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> >  		dmaengine_terminate_all(dma->tx_chan);
> > -		dma_sync_single_for_cpu(ourport->port.dev,
> > +		dma_sync_single_for_cpu(dma_map_ops_dev,
> >  			dma->tx_transfer_addr, dma->tx_size,
> DMA_TO_DEVICE);
> >  		async_tx_ack(dma->tx_desc);
> >  		count =3D dma->tx_bytes_requested - state.residue; =40=40 -324,15
> +329,19
> > =40=40 static void s3c24xx_serial_tx_dma_complete(void *args)
> >  	struct circ_buf *xmit =3D &port->state->xmit;
> >  	struct s3c24xx_uart_dma *dma =3D ourport->dma;
> >  	struct dma_tx_state state;
> > +	struct device *dma_map_ops_dev =3D ourport->port.dev;
> >  	unsigned long flags;
> >  	int count;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev =3D dma->tx_chan->device->dev;
> >
> >  	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> >  	count =3D dma->tx_bytes_requested - state.residue;
> >  	async_tx_ack(dma->tx_desc);
> >
> > -	dma_sync_single_for_cpu(ourport->port.dev, dma->tx_transfer_addr,
> > +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->tx_transfer_addr,
> >  				dma->tx_size, DMA_TO_DEVICE);
> >
> >  	spin_lock_irqsave(&port->lock, flags); =40=40 -408,7 +417,11 =40=40 s=
tatic
> > int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
> >  	struct uart_port *port =3D &ourport->port;
> >  	struct circ_buf *xmit =3D &port->state->xmit;
> >  	struct s3c24xx_uart_dma *dma =3D ourport->dma;
> > +	struct device *dma_map_ops_dev =3D ourport->port.dev;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev =3D dma->tx_chan->device->dev;
> >
> >  	if (ourport->tx_mode =21=3D S3C24XX_TX_DMA)
> >  		enable_tx_dma(ourport);
> > =40=40 -416,7 +429,7 =40=40 static int s3c24xx_serial_start_tx_dma(stru=
ct
> s3c24xx_uart_port *ourport,
> >  	dma->tx_size =3D count & =7E(dma_get_cache_alignment() - 1);
> >  	dma->tx_transfer_addr =3D dma->tx_addr + xmit->tail;
> >
> > -	dma_sync_single_for_device(ourport->port.dev, dma-
> >tx_transfer_addr,
> > +	dma_sync_single_for_device(dma_map_ops_dev, dma-
> >tx_transfer_addr,
> >  				dma->tx_size, DMA_TO_DEVICE);
> >
> >  	dma->tx_desc =3D dmaengine_prep_slave_single(dma->tx_chan,
> > =40=40 -483,12 +496,17 =40=40 static void s3c24xx_uart_copy_rx_to_tty(s=
truct
> s3c24xx_uart_port *ourport,
> >  		struct tty_port *tty, int count)
> >  =7B
> >  	struct s3c24xx_uart_dma *dma =3D ourport->dma;
> > +	struct device *dma_map_ops_dev =3D ourport->port.dev;
> >  	int copied;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->rx_chan)
> > +		dma_map_ops_dev =3D dma->rx_chan->device->dev;
> > +
> >  	if (=21count)
> >  		return;
> >
> > -	dma_sync_single_for_cpu(ourport->port.dev, dma->rx_addr,
> > +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->rx_addr,
> >  				dma->rx_size, DMA_FROM_DEVICE);
> >
> >  	ourport->port.icount.rx +=3D count;
> > =40=40 -600,8 +618,13 =40=40 static void s3c24xx_serial_rx_dma_complete=
(void
> > *args)  static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port
> > *ourport)  =7B
> >  	struct s3c24xx_uart_dma *dma =3D ourport->dma;
> > +	struct device *dma_map_ops_dev =3D ourport->port.dev;
> >
> > -	dma_sync_single_for_device(ourport->port.dev, dma->rx_addr,
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->rx_chan)
> > +		dma_map_ops_dev =3D dma->rx_chan->device->dev;
> > +
> > +	dma_sync_single_for_device(dma_map_ops_dev, dma->rx_addr,
> >  				dma->rx_size, DMA_FROM_DEVICE);
> >
> >  	dma->rx_desc =3D dmaengine_prep_slave_single(dma->rx_chan,
> > =40=40 -983,6 +1006,7 =40=40 static int s3c24xx_serial_request_dma(stru=
ct
> > s3c24xx_uart_port *p)
>=20
> Offset of hunks looks here significantly different than mainline. The pat=
ch should
> be based and tested mainline tree. Which one did you choose as base?
>=20
> Using my email address not from get_maintainers.pl also suggests that you
> don't use anything recent as a base.
>=20
I used =22master=22 branch of main linux-next tree as the base.
I will rebase on =22tty-next=22 branch of TTY tree and post again.

Thanks & Regards,
Tamseel Shams