linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sowjanya Komatineni <skomatineni@nvidia.com>
To: Dmitry Osipenko <digetx@gmail.com>,
	"thierry.reding@gmail.com" <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Mantravadi Karthik <mkarthik@nvidia.com>,
	Shardar Mohammed <smohammed@nvidia.com>,
	Timo Alho <talho@nvidia.com>
Cc: "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>
Subject: RE: [PATCH V2 2/4] i2c: tegra: Update I2C transfer using buffer
Date: Fri, 25 Jan 2019 23:26:57 +0000	[thread overview]
Message-ID: <BYAPR12MB33983C7CAD44A6466E04241BC29B0@BYAPR12MB3398.namprd12.prod.outlook.com> (raw)
In-Reply-To: <2a9c9637-c8af-f3ff-ff40-fc2a4b810846@gmail.com>


> >>>> +	buffer = kmalloc(ALIGN(msg->len, BYTES_PER_FIFO_WORD) +
> >>>> +					I2C_PACKET_HEADER_SIZE, GFP_KERNEL);
> >>>> +	if (!buffer)
> >>>> +		return -ENOMEM;
> >>>
> >>> Isn't it possible to avoid "buffer" allocation / copying overhead and keep code as-is for the PIO mode?
> >>>
> >>
> >> Keeping PIO mode code as is and adding DMA mode will have redundant code for header generation and msg complete timeout sections.
> >> Also in existing PIO mode implementation, TX FIFO is filled as soon as the word is ready but for DMA mode need to put all msg bytes along with header together to send thru DMA.
> >> So created common buffer to perform PIO/DMA to reduce redundancy.
> >>
> > 
> > Okay, what about something like in this sketch:
> > 
> > ...
> > 	if (msg->flags & I2C_M_RD)
> > 		xfer_size = msg->len;
> > 	else
> > 		xfer_size = ALIGN(msg->len, BYTES_PER_FIFO_WORD) +
> > 			    I2C_PACKET_HEADER_SIZE;
> > 
> > 	dma = ((xfer_size > I2C_PIO_MODE_MAX_LEN) &&
> > 		i2c_dev->tx_dma_chan && i2c_dev->rx_dma_chan);
> > 
> > 	if (dma) {
> > 		buffer = kmalloc(ALIGN(msg->len, BYTES_PER_FIFO_WORD) +
> > 				I2C_PACKET_HEADER_SIZE, GFP_KERNEL);
> > 		i2c_dev->msg_buf = (u8 *)buffer;
> > 	}
> > 
> > 	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
> > 				PACKET_HEADER0_PROTOCOL_I2C |
> > 				(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
> > 				(1 << PACKET_HEADER0_PACKET_ID_SHIFT);
> > 		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > 
> > 	if (dma)
> > 		(*buffer++) = packet_header;
> > 	else
> > 		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > 
> > ... and so on.
> > 
> > Likely that kmalloc overhead will be bigger than the above variant. Please consider to avoid kmalloc and copying for the PIO case in the next version.
> > 
>
> Also, is the intermediate kmalloc "buffer" really needed at all? Why just not to write directly into the DMA buffer?

Yes, that’s what I am thinking too. Changing it. Will send revised patch..


  reply	other threads:[~2019-01-25 23:27 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-24 20:51 [PATCH V2 1/4] i2c: tegra: Sort all the include headers alphabetically Sowjanya Komatineni
2019-01-24 20:51 ` [PATCH V2 2/4] i2c: tegra: Update I2C transfer using buffer Sowjanya Komatineni
2019-01-25 19:55   ` Dmitry Osipenko
2019-01-25 20:20     ` Sowjanya Komatineni
2019-01-25 21:25       ` Dmitry Osipenko
2019-01-25 23:22         ` Dmitry Osipenko
2019-01-25 23:26           ` Sowjanya Komatineni [this message]
2019-01-24 20:51 ` [PATCH V2 3/4] i2c: tegra: Add DMA Support Sowjanya Komatineni
2019-01-25 19:34   ` Dmitry Osipenko
2019-01-25 20:31     ` Sowjanya Komatineni
2019-01-25 20:40       ` Dmitry Osipenko
2019-01-25 21:11         ` Sowjanya Komatineni
2019-01-25 21:49           ` Dmitry Osipenko
2019-01-25 22:09             ` Dmitry Osipenko
2019-01-25 22:22             ` Sowjanya Komatineni
2019-01-25 23:10           ` Thierry Reding
2019-01-25 23:29             ` Sowjanya Komatineni
2019-01-25 22:03   ` Dmitry Osipenko
2019-01-25 22:35   ` Dmitry Osipenko
2019-01-26  0:28   ` Dmitry Osipenko
2019-01-26  0:49     ` Dmitry Osipenko
2019-01-26  1:51     ` Sowjanya Komatineni
2019-01-24 20:51 ` [PATCH V2 4/4] i2c: tegra: Update transfer timeout Sowjanya Komatineni
2019-01-24 21:31 ` [PATCH V2 1/4] i2c: tegra: Sort all the include headers alphabetically Dmitry Osipenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR12MB33983C7CAD44A6466E04241BC29B0@BYAPR12MB3398.namprd12.prod.outlook.com \
    --to=skomatineni@nvidia.com \
    --cc=digetx@gmail.com \
    --cc=jonathanh@nvidia.com \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=mkarthik@nvidia.com \
    --cc=smohammed@nvidia.com \
    --cc=talho@nvidia.com \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).