On Thu, Apr 8, 2021 at 10:50 AM Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx> wrote: > > Ping. > > On 3/10/2021 7:55 AM, Jae Hyun Yoo wrote: > > On 3/9/2021 6:15 PM, Rob Herring wrote: > >> On Tue, Mar 9, 2021 at 10:02 AM Jae Hyun Yoo > >> <jae.hyun.yoo@xxxxxxxxxxxxxxx> wrote: > >>> > >>> Hi Rob, > >>> > >>> On 3/6/2021 12:30 PM, Rob Herring wrote: > >>>> On Wed, Feb 24, 2021 at 11:17:17AM -0800, Jae Hyun Yoo wrote: > >>>>> Append bindings to support transfer mode. > >>>>> > >>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx> > >>>>> Reviewed-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx> > >>>>> --- > >>>>> Changes since v3: > >>>>> - None > >>>>> > >>>>> Changes since v2: > >>>>> - Moved SRAM resources back to default dtsi and added mode selection > >>>>> property. > >>>>> > >>>>> Changes since v1: > >>>>> - Removed buffer reg settings from default device tree and added > >>>>> the settings > >>>>> into here to show the predefined buffer range per each bus. > >>>>> > >>>>> .../devicetree/bindings/i2c/i2c-aspeed.txt | 37 > >>>>> +++++++++++++++---- > >>>>> 1 file changed, 30 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt > >>>>> b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt > >>>>> index b47f6ccb196a..242343177324 100644 > >>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt > >>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt > >>>>> @@ -17,6 +17,20 @@ Optional Properties: > >>>>> - bus-frequency : frequency of the bus clock in Hz defaults > >>>>> to 100 kHz when not > >>>>> specified > >>>>> - multi-master : states that there is another master active > >>>>> on this bus. > >>>>> +- aspeed,i2c-xfer-mode : should be "byte", "buf" or "dma" to > >>>>> select transfer > >>>>> + mode defaults to "byte" mode when not > >>>>> specified. > >>>>> + > >>>>> + I2C DMA mode on AST2500 has these restrictions: > >>>>> + - If one of these controllers is enabled > >>>>> + * UHCI host controller > >>>>> + * MCTP controller > >>>>> + I2C has to use buffer mode or byte mode > >>>>> instead > >>>>> + since these controllers run only in DMA > >>>>> mode and > >>>>> + I2C is sharing the same DMA H/W with them. > >>>>> + - If one of these controllers uses DMA > >>>>> mode, I2C > >>>>> + can't use DMA mode > >>>>> + * SD/eMMC > >>>>> + * Port80 snoop > >>>> > >>>> How does one decide between byte or buf mode? > >>> > >>> If a given system makes just one byte r/w transactions most of the time > >>> then byte mode will be a right setting. Otherwise, buf mode is more > >>> efficient because it doesn't generate a bunch of interrupts on every > >>> byte handling. > >> > >> Then why doesn't the driver do byte transactions when it gets small > >> 1-4? byte transactions and buffer transactions when it gets larger > >> sized transactions. > > > > Good question and it could be an option of this implementation. > > Actually, each mode needs different register handling so we need to add > > additional conditional branches to make it dynamic mode change depends > > on the data size which can be a downside. Also, checked that small > > amount of data transfer efficiency in 'buf' transfer mode is almost same > > to 'byte' mode so there would be no big benefit from the dynamic mode > > change. Of course, we can remove the 'byte' transfer mode but we should > > also provide flexibility of configuration on what this hardware can > > support, IMO. I would rather set the choice in device tree or Kconfig, which the former is what I think you did here. As for doing byte mode for small transactions and buffer/DMA for large transactions, I would prefer sticking to a single mode based on what is selected at build/boot time. Seems less error prone to me. Then again, Rob probably has more experience in this area than I do, so maybe this kind of thing is pretty common and I just don't realize it. In any case, as for getting rid of byte mode, I would support that, but not in this patch set. I would rather switch the default and get users on buffer/DMA mode before taking away a fallback option. My 2 cents, but I think the OzLabs and other active OpenBMC people are probably a little more up to date on this. Cheers
