Hello Jeremy,
> -----Original Message-----
> From: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx>
> Sent: Monday, February 27, 2023 1:40 PM
> To: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>; Andrew Jeffery
> <andrew@xxxxxxxx>; Brendan Higgins <brendan.higgins@xxxxxxxxx>; Benjamin
> Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>; Joel Stanley <joel@xxxxxxxxx>;
> Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>;
> linux-i2c@xxxxxxxxxxxxxxx; openbmc@xxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-aspeed@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>
> 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.
Sorry, Do you mean add in description like following??
aspeed,xfer-mode:
description: |
I2C bus transfer mode selection.
ERRATA "I2C DMA fails when DRAM bus is busy and it can not take DMA write data
Immediately", only 1 i2c bus can be enable for DMA mode.
- "byte": I2C bus byte transfer mode.
- "buffered": I2C bus buffer register transfer mode.
- "dma": I2C bus dma transfer mode (default)
> 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?
Sorry, I can't catch your point, for example for most server application usage.
The i2c not only connect with small device (like temperature sensor/ adc).
It also connect with other mcu base device support i2c slave.
Most case is transfer MCTP package. (basic 64kbytes). So I say "trunk data".
>
> > - 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?
If so, just leave enable-dma and only support for buffer mode and dma mode, am I right?
Best Regards.
Ryan
