All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeremy Kerr <jk@codeconstruct.com.au>
To: Ryan Chen <ryan_chen@aspeedtech.com>,
	Andrew Jeffery <andrew@aj.id.au>,
	Brendan Higgins <brendan.higgins@linux.dev>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Joel Stanley <joel@jms.id.au>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Date: Mon, 27 Feb 2023 13:40:12 +0800	[thread overview]
Message-ID: <f4cb3efc9825efa582aa94bd03657b1319ff38fd.camel@codeconstruct.com.au> (raw)
In-Reply-To: <SEZPR06MB52699858C92383E8E07D0832F2AF9@SEZPR06MB5269.apcprd06.prod.outlook.com>

Hi Ryan,

> Yes, I2C controller share the same dma engine. The original thought
> can be enable in all i2c channel. But in AST2600 have ERRATA "I2C DMA
> fails when DRAM bus is busy and it can not take DMA write data
> immediately", So it means only 1 i2c bus can be enable for DMA mode.

OK, this is a pretty important detail! I'd suggest putting it in the
binding document.

Anything in the cover letter will get lost after review. If there is
documentation that would be useful for a DTS author, I'd suggest putting
it in the binding.

> It means only 1 bus channel can be enable DMA for use case.
> That following example for board-specific selection.
> It is description in cover-letter.
> The following is board-specific design example.
> Board A                                         Board B
> -------------------------                       ------------------------
> > i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> > i2c bus#2(master)-> tmp i2c device |        |                   |
> > i2c bus#3(master)-> adc i2c device |        |                   |
> -------------------------                       ------------------------
> 
> - in bus#1 situation, you should use DMA mode.
> Because bus#1 have trunk data needed for transfer, it can enable bus
> dma mode to reduce cpu utilized.

What is "trunk data" in this context? Is this just a statement about the
amount of expected transfers?

> - in bus#2/3 situation, you should use buffer/byte mode
> bus#2/3 is small package transmit, it can enable buffer mode or byte
> mode to reduce memory cache flush overhead.
> Buffer mode is better, because byte mode have interrupt
> overhead(interrupt per byte data transmit),
> 
> -But if you more bus#4 that still have trunk data needed for transfer
> (master/slave),
> it also use buffer mode to transmit. Because bus#1 have been use for
> DMA mode.

So, it sounds like:

 - there's no point in using byte mode, as buffer mode provides
   equivalent functionality with fewer drawbacks (ie, less interrupt
   load)

 - this just leaves the dma and buffer modes

 - only one controller can use dma mode

So: how about just a single boolean property to indicate "use DMA on
this controller"? Something like aspeed,enable-dma? Or if DT binding
experts can suggest something common that might be more suitable?

Cheers,


Jeremy

WARNING: multiple messages have this Message-ID (diff)
From: Jeremy Kerr <jk@codeconstruct.com.au>
To: Ryan Chen <ryan_chen@aspeedtech.com>,
	Andrew Jeffery <andrew@aj.id.au>,
	 Brendan Higgins <brendan.higgins@linux.dev>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Joel Stanley <joel@jms.id.au>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	 "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Date: Mon, 27 Feb 2023 13:40:12 +0800	[thread overview]
Message-ID: <f4cb3efc9825efa582aa94bd03657b1319ff38fd.camel@codeconstruct.com.au> (raw)
In-Reply-To: <SEZPR06MB52699858C92383E8E07D0832F2AF9@SEZPR06MB5269.apcprd06.prod.outlook.com>

Hi Ryan,

> Yes, I2C controller share the same dma engine. The original thought
> can be enable in all i2c channel. But in AST2600 have ERRATA "I2C DMA
> fails when DRAM bus is busy and it can not take DMA write data
> immediately", So it means only 1 i2c bus can be enable for DMA mode.

OK, this is a pretty important detail! I'd suggest putting it in the
binding document.

Anything in the cover letter will get lost after review. If there is
documentation that would be useful for a DTS author, I'd suggest putting
it in the binding.

> It means only 1 bus channel can be enable DMA for use case.
> That following example for board-specific selection.
> It is description in cover-letter.
> The following is board-specific design example.
> Board A                                         Board B
> -------------------------                       ------------------------
> > i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> > i2c bus#2(master)-> tmp i2c device |        |                   |
> > i2c bus#3(master)-> adc i2c device |        |                   |
> -------------------------                       ------------------------
> 
> - in bus#1 situation, you should use DMA mode.
> Because bus#1 have trunk data needed for transfer, it can enable bus
> dma mode to reduce cpu utilized.

What is "trunk data" in this context? Is this just a statement about the
amount of expected transfers?

> - in bus#2/3 situation, you should use buffer/byte mode
> bus#2/3 is small package transmit, it can enable buffer mode or byte
> mode to reduce memory cache flush overhead.
> Buffer mode is better, because byte mode have interrupt
> overhead(interrupt per byte data transmit),
> 
> -But if you more bus#4 that still have trunk data needed for transfer
> (master/slave),
> it also use buffer mode to transmit. Because bus#1 have been use for
> DMA mode.

So, it sounds like:

 - there's no point in using byte mode, as buffer mode provides
   equivalent functionality with fewer drawbacks (ie, less interrupt
   load)

 - this just leaves the dma and buffer modes

 - only one controller can use dma mode

So: how about just a single boolean property to indicate "use DMA on
this controller"? Something like aspeed,enable-dma? Or if DT binding
experts can suggest something common that might be more suitable?

Cheers,


Jeremy

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2023-02-27  5:40 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-26  3:13 [PATCH v6 0/2] Add ASPEED AST2600 I2Cv2 controller driver Ryan Chen
2023-02-26  3:13 ` Ryan Chen
2023-02-26  3:13 ` [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Ryan Chen
2023-02-26  3:13   ` Ryan Chen
2023-02-26  7:04   ` Jeremy Kerr
2023-02-26  7:04     ` Jeremy Kerr
2023-02-27  4:12     ` Ryan Chen
2023-02-27  4:12       ` Ryan Chen
2023-02-27  5:40       ` Jeremy Kerr [this message]
2023-02-27  5:40         ` Jeremy Kerr
2023-03-01  3:02         ` Ryan Chen
2023-03-01  3:02           ` Ryan Chen
2023-03-01  3:23           ` Jeremy Kerr
2023-03-01  3:23             ` Jeremy Kerr
2023-03-01  3:40             ` Ryan Chen
2023-03-01  3:40               ` Ryan Chen
2023-02-27  8:25   ` Krzysztof Kozlowski
2023-02-27  8:25     ` Krzysztof Kozlowski
2023-03-01  5:57     ` Ryan Chen
2023-03-01  5:57       ` Ryan Chen
2023-03-03  8:20       ` Krzysztof Kozlowski
2023-03-03  8:20         ` Krzysztof Kozlowski
2023-03-03  8:28         ` Ryan Chen
2023-03-03  8:28           ` Ryan Chen
2023-03-03  8:28           ` Ryan Chen
2023-03-03  8:50           ` Krzysztof Kozlowski
2023-03-03  8:50             ` Krzysztof Kozlowski
2023-03-03  8:55             ` Ryan Chen
2023-03-03  8:55               ` Ryan Chen
2023-03-03  8:55               ` Ryan Chen
2023-03-03  9:26               ` Krzysztof Kozlowski
2023-03-03  9:26                 ` Krzysztof Kozlowski
2023-03-03 10:16                 ` Ryan Chen
2023-03-03 10:16                   ` Ryan Chen
2023-03-03 10:16                   ` Ryan Chen
2023-03-03 10:41                   ` Krzysztof Kozlowski
2023-03-03 10:41                     ` Krzysztof Kozlowski
2023-03-04  1:33                     ` Ryan Chen
2023-03-04  1:33                       ` Ryan Chen
2023-03-04  1:33                       ` Ryan Chen
2023-03-05  9:49                       ` Krzysztof Kozlowski
2023-03-05  9:49                         ` Krzysztof Kozlowski
2023-03-06  0:48                         ` Ryan Chen
2023-03-06  0:48                           ` Ryan Chen
2023-03-06  0:48                           ` Ryan Chen
2023-03-07  8:11                           ` Krzysztof Kozlowski
2023-03-07  8:11                             ` Krzysztof Kozlowski
2023-03-07 10:09                             ` Ryan Chen
2023-03-07 10:09                               ` Ryan Chen
2023-03-07 10:09                               ` Ryan Chen
2023-03-09  8:51                               ` Krzysztof Kozlowski
2023-03-09  8:51                                 ` Krzysztof Kozlowski
2023-03-09  9:15                                 ` Ryan Chen
2023-03-09  9:15                                   ` Ryan Chen
2023-03-09  9:15                                   ` Ryan Chen
2023-03-12 12:33         ` Andi Shyti
2023-03-12 12:33           ` Andi Shyti
2023-03-12 12:33           ` Andi Shyti
2023-03-18  9:09   ` Andi Shyti
2023-03-18  9:09     ` Andi Shyti
2023-03-18  9:09     ` Andi Shyti
2023-03-19  2:05     ` Ryan Chen
2023-03-19  2:05       ` Ryan Chen
2023-03-19  2:05       ` Ryan Chen
2023-02-26  3:13 ` [PATCH v6 2/2] i2c: aspeed: support ast2600 i2cv new register mode driver Ryan Chen
2023-02-26  3:13   ` Ryan Chen

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=f4cb3efc9825efa582aa94bd03657b1319ff38fd.camel@codeconstruct.com.au \
    --to=jk@codeconstruct.com.au \
    --cc=andrew@aj.id.au \
    --cc=benh@kernel.crashing.org \
    --cc=brendan.higgins@linux.dev \
    --cc=devicetree@vger.kernel.org \
    --cc=joel@jms.id.au \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-aspeed@lists.ozlabs.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=openbmc@lists.ozlabs.org \
    --cc=p.zabel@pengutronix.de \
    --cc=robh+dt@kernel.org \
    --cc=ryan_chen@aspeedtech.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.