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: <4b2576c1-c986-a4d8-d6cf-661ca056ecee@canonical.com>
Subject: RE: [PATCH] serial: samsung: use dma_ops of DMA if attached
Date:   Tue, 22 Jun 2021 13:22:22 +0530
Message-ID: <007901d7673b$8a758d10$9f60a730$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQGDfL5lNG5rzFQPe+FwbDp9GpLyVQGDsg9dAbmelg0Cc6DxagIMN/A2q4mTjPA=
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>
        <004f01d766a0$567b9860$0372c920$@samsung.com>
        <4b2576c1-c986-a4d8-d6cf-661ca056ecee@canonical.com>
Precedence: bulk

> > Hi,
> >
> >>
> >> Hi,
> >>
> >> Thanks for the patch.
> >>
> >> 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.
> >>
> >> Hmmm, but why DMA device pointer should be used?
> >>
> >>>
> >>> 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(struc=
t
> >>> 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 */
> >>
> >> You mention here and further comments =22dma_ops=22. I don't see you
> >> changing the DMA ops, but the device. It's quite confusing. I think
> >> you meant a DMA device shall be passed to DMA API?
> >>
> > Yes, DMA device should be used for DMA API because only the DMA device
> > is aware of how the device connects to the memory. There might be an
> > extra level of address 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.
>=20
> Thanks, this should be in commit msg.
>=20
Sure, will add this in commit msg.
> >
> >> Second question: you write that DMA devices should be used if DMA is
> >> attached and in the code you follow such pattern a lot:
> >>
> >>> +	if (dma && dma->tx_chan)
> >>> +		dma_map_ops_dev =3D dma->tx_chan->device->dev;
> >>> +
> >>
> >> 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?
> >>
> > 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.
>=20
> Now I get it. The =22When=22 in your description followed by multiple com=
ments =22if
> DMA device is attached=22 confused me that you expect to use UART device =
for
> DMA operations if DMA is not attached...
>=20
I will change the comments, to avoid this confusion.
> >
> >>>  	if (=21ourport->tx_enabled)
> >>>  		return;
> >>>
> >>> =40=40 -298,7 +303,7 =40=40 static void s3c24xx_serial_stop_tx(struct
> >>> uart_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;
>=20
> Example is this one - you use here =22if=22 suggesting there is =22else=
=22. So what is the
> else condition? There is none...
>=20
> >>>
> >>>  	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=
 static
> >>> 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(st=
ruct
> >> 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=
(struct
> >> 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_comple=
te(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(st=
ruct
> >>> s3c24xx_uart_port *p)
> >>
> >> Offset of hunks looks here significantly different than mainline. The
> >> patch should be based and tested mainline tree. Which one did you choo=
se as
> base?
> >>
> >> Using my email address not from get_maintainers.pl also suggests that
> >> you don't use anything recent as a base.
> >>
> > 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
> >
>=20
>=20
> Best regards,
> Krzysztof

Thanks & Regards,
Tamseel Shams