diff mbox series

[v4,1/4] dt-bindings: i2c: aspeed: add transfer mode support

Message ID 20210224191720.7724-2-jae.hyun.yoo@linux.intel.com
State Needs Review / ACK
Headers show
Series i2c: aspeed: Add buffer and DMA modes support | expand

Commit Message

Jae Hyun Yoo Feb. 24, 2021, 7:17 p.m. UTC
Append bindings to support transfer mode.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
---
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(-)

Comments

Rob Herring (Arm) March 6, 2021, 8:30 p.m. UTC | #1
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@linux.intel.com>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> 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? 

>  
>  Example:
>  
> @@ -26,20 +40,29 @@ i2c {
>  	#size-cells = <1>;
>  	ranges = <0 0x1e78a000 0x1000>;
>  
> -	i2c_ic: interrupt-controller@0 {
> -		#interrupt-cells = <1>;
> -		compatible = "aspeed,ast2400-i2c-ic";
> +	i2c_gr: i2c-global-regs@0 {
> +		compatible = "aspeed,ast2500-i2c-gr", "syscon";
>  		reg = <0x0 0x40>;
> -		interrupts = <12>;
> -		interrupt-controller;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x0 0x40>;
> +
> +		i2c_ic: interrupt-controller@0 {
> +			#interrupt-cells = <1>;
> +			compatible = "aspeed,ast2500-i2c-ic";
> +			reg = <0x0 0x4>;
> +			interrupts = <12>;
> +			interrupt-controller;
> +		};
>  	};
>  
>  	i2c0: i2c-bus@40 {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  		#interrupt-cells = <1>;
> -		reg = <0x40 0x40>;
> -		compatible = "aspeed,ast2400-i2c-bus";
> +		reg = <0x40 0x40>, <0x200 0x10>;
> +		compatible = "aspeed,ast2500-i2c-bus";


The example changes are all unrelated to adding the new property. Should 
be a separate patch or just dropped.

>  		clocks = <&syscon ASPEED_CLK_APB>;
>  		resets = <&syscon ASPEED_RESET_I2C>;
>  		bus-frequency = <100000>;
> -- 
> 2.17.1
>
Jae Hyun Yoo March 9, 2021, 5:02 p.m. UTC | #2
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@linux.intel.com>
>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>> ---
>> 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.

>>   
>>   Example:
>>   
>> @@ -26,20 +40,29 @@ i2c {
>>   	#size-cells = <1>;
>>   	ranges = <0 0x1e78a000 0x1000>;
>>   
>> -	i2c_ic: interrupt-controller@0 {
>> -		#interrupt-cells = <1>;
>> -		compatible = "aspeed,ast2400-i2c-ic";
>> +	i2c_gr: i2c-global-regs@0 {
>> +		compatible = "aspeed,ast2500-i2c-gr", "syscon";
>>   		reg = <0x0 0x40>;
>> -		interrupts = <12>;
>> -		interrupt-controller;
>> +
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x0 0x40>;
>> +
>> +		i2c_ic: interrupt-controller@0 {
>> +			#interrupt-cells = <1>;
>> +			compatible = "aspeed,ast2500-i2c-ic";
>> +			reg = <0x0 0x4>;
>> +			interrupts = <12>;
>> +			interrupt-controller;
>> +		};
>>   	};
>>   
>>   	i2c0: i2c-bus@40 {
>>   		#address-cells = <1>;
>>   		#size-cells = <0>;
>>   		#interrupt-cells = <1>;
>> -		reg = <0x40 0x40>;
>> -		compatible = "aspeed,ast2400-i2c-bus";
>> +		reg = <0x40 0x40>, <0x200 0x10>;
>> +		compatible = "aspeed,ast2500-i2c-bus";
> 
> The example changes are all unrelated to adding the new property. Should
> be a separate patch or just dropped.

The example changes are not directly related to the new property but
related to the transfer mode support in this patch set. 'i2c_gr' node is
added to provide a way for accessing I2C global registers to enable
I2C SRAM, and 'reg' is modified to add the SRAM resource range.

Thanks,
Jae

>>   		clocks = <&syscon ASPEED_CLK_APB>;
>>   		resets = <&syscon ASPEED_RESET_I2C>;
>>   		bus-frequency = <100000>;
>> -- 
>> 2.17.1
>>
Rob Herring (Arm) March 10, 2021, 2:15 a.m. UTC | #3
On Tue, Mar 9, 2021 at 10:02 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> 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@linux.intel.com>
> >> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> >> ---
> >> 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.

Rob
Jae Hyun Yoo March 10, 2021, 3:55 p.m. UTC | #4
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@linux.intel.com> 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@linux.intel.com>
>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>>>> ---
>>>> 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.

Thanks,
Jae
Jae Hyun Yoo April 8, 2021, 5:50 p.m. UTC | #5
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@linux.intel.com> 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@linux.intel.com>
>>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>>>>> ---
>>>>> 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.
> 
> Thanks,
> Jae
Brendan Higgins April 13, 2021, 7:50 p.m. UTC | #6
On Thu, Apr 8, 2021 at 10:50 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com>
> >>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> >>>>> ---
> >>>>> 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
Jae Hyun Yoo Oct. 1, 2021, 5:05 p.m. UTC | #7
On 4/13/2021 12:50 PM, Brendan Higgins wrote:
> On Thu, Apr 8, 2021 at 10:50 AM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com>
>>>>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>>>>>>> ---
>>>>>>> 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

Hi Brendan, Thanks for sharing your thought.

Hi Rob,

I'm bringing it back to make progress again. Like Brendan said, I also
prefer sticking to a single mode which is selected through device tree
and it would be right way for enabling all features this hardware
provides. There would be no big benefit if we make dynamic mode change
for 'byte' and 'buf' modes based on transaction size because 'buf' mode
can effectively cover the small transaction size cases well without
making a performance degradation. Means that we can remove the 'byte'
mode but I don't want to hide/block a feature which this hardware
originally provides.

Please let us know your thought.

Thanks,
Jae
diff mbox series

Patch

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
 
 Example:
 
@@ -26,20 +40,29 @@  i2c {
 	#size-cells = <1>;
 	ranges = <0 0x1e78a000 0x1000>;
 
-	i2c_ic: interrupt-controller@0 {
-		#interrupt-cells = <1>;
-		compatible = "aspeed,ast2400-i2c-ic";
+	i2c_gr: i2c-global-regs@0 {
+		compatible = "aspeed,ast2500-i2c-gr", "syscon";
 		reg = <0x0 0x40>;
-		interrupts = <12>;
-		interrupt-controller;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x0 0x40>;
+
+		i2c_ic: interrupt-controller@0 {
+			#interrupt-cells = <1>;
+			compatible = "aspeed,ast2500-i2c-ic";
+			reg = <0x0 0x4>;
+			interrupts = <12>;
+			interrupt-controller;
+		};
 	};
 
 	i2c0: i2c-bus@40 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x40 0x40>;
-		compatible = "aspeed,ast2400-i2c-bus";
+		reg = <0x40 0x40>, <0x200 0x10>;
+		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
 		bus-frequency = <100000>;