Re: [PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>]

Hi Joel

On 1/27/2021 4:06 PM, Joel Stanley wrote:
> On Thu, 14 Jan 2021 at 20:05, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> Hi Rob,
>>
>> On 1/14/2021 11:34 AM, Rob Herring wrote:
>>>> -- reg                       : address offset and range of bus
>>>> +- reg                       : Address offset and range of bus registers.
>>>> +
>>>> +                      An additional SRAM buffer address offset and range is
>>>> +                      optional in case of enabling I2C dedicated SRAM for
>>>> +                      buffer mode transfer support. If the optional range
>>>> +                      is defined, buffer mode will be enabled.
>>>> +                      - AST2400
>>>> +                        &i2c0 { reg = <0x40 0x40>, <0x800 0x80>; };
>>>> +                        &i2c1 { reg = <0x80 0x40>, <0x880 0x80>; };
>>>> +                        &i2c2 { reg = <0xc0 0x40>, <0x900 0x80>; };
>>>> +                        &i2c3 { reg = <0x100 0x40>, <0x980 0x80>; };
>>>> +                        &i2c4 { reg = <0x140 0x40>, <0xa00 0x80>; };
>>>> +                        &i2c5 { reg = <0x180 0x40>, <0xa80 0x80>; };
>>>> +                        &i2c6 { reg = <0x1c0 0x40>, <0xb00 0x80>; };
>>>> +                        &i2c7 { reg = <0x300 0x40>, <0xb80 0x80>; };
>>>> +                        &i2c8 { reg = <0x340 0x40>, <0xc00 0x80>; };
>>>> +                        &i2c9 { reg = <0x380 0x40>, <0xc80 0x80>; };
>>>> +                        &i2c10 { reg = <0x3c0 0x40>, <0xd00 0x80>; };
>>>> +                        &i2c11 { reg = <0x400 0x40>, <0xd80 0x80>; };
>>>> +                        &i2c12 { reg = <0x440 0x40>, <0xe00 0x80>; };
>>>> +                        &i2c13 { reg = <0x480 0x40>, <0xe80 0x80>; };
>>>
>>> All this information doesn't need to be in the binding.
>>>
>>> It's also an oddly structured dts file if this is what you are doing...
>>
>> I removed the default buffer mode settings that I added into
>> 'aspeed-g4.dtsi' and 'aspeed-g5.dtsi' in v1 to avoid touching of the
>> default transfer mode setting, but each bus should use its dedicated
>> SRAM buffer range for enabling buffer mode so I added this information
>> at here as overriding examples instead. I thought that binding document
>> is a right place for providing this information but looks like it's not.
>> Any recommended place for it? Is it good enough if I add it just into
>> the commit message?
> 
> I agree with Rob, we don't need this described in the device tree
> (binding or dts). We know what the layout is for a given aspeed
> family, so the driver can have this information hard coded.
> 
> (Correct me if I've misinterpted here Rob)
> 

Makes sense. Will add these settings into the driver module as hard
coded per each bus.

>>
>>>> @@ -17,6 +72,25 @@ 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,dma-buf-size       : size of DMA buffer.
>>>> +                        AST2400: N/A
>>>> +                        AST2500: 2 ~ 4095
>>>> +                        AST2600: 2 ~ 4096
>>>
>>> If based on the SoC, then all this can be implied from the compatible
>>> string.
>>>
>>
>> Please help me to clarify your comment. Should I remove it from here
>> with keeping the driver handling code for each SoC compatible string?
>> Or should I change it like below?
>> aspeed,ast2400-i2c-bus: N/A
>> aspeed,ast2500-i2c-bus: 2 ~ 4095
>> aspeed,ast2600-i2c-bus: 2 ~ 4096
> 
> As above, we know what the buffer size is for the specific soc family,
> so we can hard code the value to expect.
> 
> The downside of this hard coding is it takes away the option of using
> more buffer space for a given master in a system that only enables
> some of the masters. Is this a use case you were considering? If so,
> then we might revisit some of the advice in this thread.
> 

I added flexibility on this setting but it doesn't need to be. I'll add
hard coded setting for the maximum DMA length into the driver as you
suggested. If I add a xfer mode setting in device tree instead, enabling
of DMA can be configured as each bus basis so there would be no concern
I believe. Will submit v3 soon.

Thanks,
Jae