| Message ID | 20210112003749.10565-2-jae.hyun.yoo@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: aspeed: Add buffer and DMA modes support | expand |
On Mon, Jan 11, 2021 at 04:37:46PM -0800, Jae Hyun Yoo wrote: > Append bindings to support buffer mode and DMA mode transfer. > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > --- > 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 | 126 +++++++++++++++++- > 1 file changed, 119 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt > index b47f6ccb196a..978e8402fdfc 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt > @@ -3,7 +3,62 @@ Device tree configuration for the I2C busses on the AST24XX, AST25XX, and AST26X > Required Properties: > - #address-cells : should be 1 > - #size-cells : should be 0 > -- 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... > + > + - AST2500 > + &i2c0 { reg = <0x40 0x40>, <0x200 0x10>; }; > + &i2c1 { reg = <0x80 0x40>, <0x210 0x10>; }; > + &i2c2 { reg = <0xc0 0x40>, <0x220 0x10>; }; > + &i2c3 { reg = <0x100 0x40>, <0x230 0x10>; }; > + &i2c4 { reg = <0x140 0x40>, <0x240 0x10>; }; > + &i2c5 { reg = <0x180 0x40>, <0x250 0x10>; }; > + &i2c6 { reg = <0x1c0 0x40>, <0x260 0x10>; }; > + &i2c7 { reg = <0x300 0x40>, <0x270 0x10>; }; > + &i2c8 { reg = <0x340 0x40>, <0x280 0x10>; }; > + &i2c9 { reg = <0x380 0x40>, <0x290 0x10>; }; > + &i2c10 { reg = <0x3c0 0x40>, <0x2a0 0x10>; }; > + &i2c11 { reg = <0x400 0x40>, <0x2b0 0x10>; }; > + &i2c12 { reg = <0x440 0x40>, <0x2c0 0x10>; }; > + &i2c13 { reg = <0x480 0x40>, <0x2d0 0x10>; }; > + > + - AST2600 > + &i2c0 { reg = <0x80 0x80>, <0xc00 0x20>; }; > + &i2c1 { reg = <0x100 0x80>, <0xc20 0x20>; }; > + &i2c2 { reg = <0x180 0x80>, <0xc40 0x20>; }; > + &i2c3 { reg = <0x200 0x80>, <0xc60 0x20>; }; > + &i2c4 { reg = <0x280 0x80>, <0xc80 0x20>; }; > + &i2c5 { reg = <0x300 0x80>, <0xca0 0x20>; }; > + &i2c6 { reg = <0x380 0x80>, <0xcc0 0x20>; }; > + &i2c7 { reg = <0x400 0x80>, <0xce0 0x20>; }; > + &i2c8 { reg = <0x480 0x80>, <0xd00 0x20>; }; > + &i2c9 { reg = <0x500 0x80>, <0xd20 0x20>; }; > + &i2c10 { reg = <0x580 0x80>, <0xd40 0x20>; }; > + &i2c11 { reg = <0x600 0x80>, <0xd60 0x20>; }; > + &i2c12 { reg = <0x680 0x80>, <0xd80 0x20>; }; > + &i2c13 { reg = <0x700 0x80>, <0xda0 0x20>; }; > + &i2c14 { reg = <0x780 0x80>, <0xdc0 0x20>; }; > + &i2c15 { reg = <0x800 0x80>, <0xde0 0x20>; }; > + > - compatible : should be "aspeed,ast2400-i2c-bus" > or "aspeed,ast2500-i2c-bus" > or "aspeed,ast2600-i2c-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. > + > + If both DMA and buffer modes are enabled in device > + tree, DMA mode will be selected. > + > + 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,12 +100,21 @@ 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 { > @@ -39,11 +122,40 @@ i2c { > #size-cells = <0>; > #interrupt-cells = <1>; > reg = <0x40 0x40>; > - compatible = "aspeed,ast2400-i2c-bus"; > + compatible = "aspeed,ast2500-i2c-bus"; > clocks = <&syscon ASPEED_CLK_APB>; > resets = <&syscon ASPEED_RESET_I2C>; > bus-frequency = <100000>; > interrupts = <0>; > interrupt-parent = <&i2c_ic>; > }; > + > + /* buffer mode transfer enabled */ > + i2c1: i2c-bus@80 { > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <1>; > + reg = <0x80 0x40>, <0x210 0x10>; > + compatible = "aspeed,ast2500-i2c-bus"; > + clocks = <&syscon ASPEED_CLK_APB>; > + resets = <&syscon ASPEED_RESET_I2C>; > + bus-frequency = <100000>; > + interrupts = <1>; > + interrupt-parent = <&i2c_ic>; > + }; > + > + /* DMA mode transfer enabled */ > + i2c2: i2c-bus@c0 { > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <1>; > + reg = <0xc0 0x40>; > + aspeed,dma-buf-size = <4095>; > + compatible = "aspeed,ast2500-i2c-bus"; > + clocks = <&syscon ASPEED_CLK_APB>; > + resets = <&syscon ASPEED_RESET_I2C>; > + bus-frequency = <100000>; > + interrupts = <2>; > + interrupt-parent = <&i2c_ic>; > + }; > }; > -- > 2.17.1 >
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? >> @@ -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 Thanks, Jae
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) > > >> @@ -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. Cheers, Joel
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
Hi Joel On 1/28/2021 11:36 AM, Jae Hyun Yoo wrote: > 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. > Realized that the SRAM buffer range setting should be added into device tree because each bus module should get the dedicated IO resource range. So I'm going to add it to dtsi default reg setting for each I2C bus and will remove this description in binding. Also, I'll add a mode setting property instead to keep the current setting as byte mode. Please let me know if you have any different thought. Thanks, Jae
On Wed, 3 Feb 2021 at 23:03, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote: > > Hi Joel > > On 1/28/2021 11:36 AM, Jae Hyun Yoo wrote: > > Hi Joel > > > > On 1/27/2021 4:06 PM, Joel Stanley wrote: > >>>> 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. > > > > Realized that the SRAM buffer range setting should be added into device > tree because each bus module should get the dedicated IO resource range. > So I'm going to add it to dtsi default reg setting for each I2C bus > and will remove this description in binding. Also, I'll add a mode > setting property instead to keep the current setting as byte mode. I don't understand. What do you propose adding?
On 2/9/2021 4:10 AM, Joel Stanley wrote: > On Wed, 3 Feb 2021 at 23:03, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote: >> >> Hi Joel >> >> On 1/28/2021 11:36 AM, Jae Hyun Yoo wrote: >>> Hi Joel >>> >>> On 1/27/2021 4:06 PM, Joel Stanley wrote: >>>>>> 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. >>> >> >> Realized that the SRAM buffer range setting should be added into device >> tree because each bus module should get the dedicated IO resource range. >> So I'm going to add it to dtsi default reg setting for each I2C bus >> and will remove this description in binding. Also, I'll add a mode >> setting property instead to keep the current setting as byte mode. > > I don't understand. What do you propose adding? > I'm going to add reg resource for the SRAM buffer per each bus like reg = <0x40 0x40>, <0x800 0x80>; into the aspeed-g*.dtsi but adding like this will not be a key for enabling buffer mode like this v2 does. Also, I'm going to remove the 'aspeed,dma-buf-size' in this patch series and I'll add fixed DMA length as a configuration per each SoC. Instead, I'm going to add a xfer mode property e.g. 'aspeed,i2c-xfer-mode' to select 'byte', 'buffer' or 'dma' modes. Thanks, Jae
diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt index b47f6ccb196a..978e8402fdfc 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt @@ -3,7 +3,62 @@ Device tree configuration for the I2C busses on the AST24XX, AST25XX, and AST26X Required Properties: - #address-cells : should be 1 - #size-cells : should be 0 -- 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>; }; + + - AST2500 + &i2c0 { reg = <0x40 0x40>, <0x200 0x10>; }; + &i2c1 { reg = <0x80 0x40>, <0x210 0x10>; }; + &i2c2 { reg = <0xc0 0x40>, <0x220 0x10>; }; + &i2c3 { reg = <0x100 0x40>, <0x230 0x10>; }; + &i2c4 { reg = <0x140 0x40>, <0x240 0x10>; }; + &i2c5 { reg = <0x180 0x40>, <0x250 0x10>; }; + &i2c6 { reg = <0x1c0 0x40>, <0x260 0x10>; }; + &i2c7 { reg = <0x300 0x40>, <0x270 0x10>; }; + &i2c8 { reg = <0x340 0x40>, <0x280 0x10>; }; + &i2c9 { reg = <0x380 0x40>, <0x290 0x10>; }; + &i2c10 { reg = <0x3c0 0x40>, <0x2a0 0x10>; }; + &i2c11 { reg = <0x400 0x40>, <0x2b0 0x10>; }; + &i2c12 { reg = <0x440 0x40>, <0x2c0 0x10>; }; + &i2c13 { reg = <0x480 0x40>, <0x2d0 0x10>; }; + + - AST2600 + &i2c0 { reg = <0x80 0x80>, <0xc00 0x20>; }; + &i2c1 { reg = <0x100 0x80>, <0xc20 0x20>; }; + &i2c2 { reg = <0x180 0x80>, <0xc40 0x20>; }; + &i2c3 { reg = <0x200 0x80>, <0xc60 0x20>; }; + &i2c4 { reg = <0x280 0x80>, <0xc80 0x20>; }; + &i2c5 { reg = <0x300 0x80>, <0xca0 0x20>; }; + &i2c6 { reg = <0x380 0x80>, <0xcc0 0x20>; }; + &i2c7 { reg = <0x400 0x80>, <0xce0 0x20>; }; + &i2c8 { reg = <0x480 0x80>, <0xd00 0x20>; }; + &i2c9 { reg = <0x500 0x80>, <0xd20 0x20>; }; + &i2c10 { reg = <0x580 0x80>, <0xd40 0x20>; }; + &i2c11 { reg = <0x600 0x80>, <0xd60 0x20>; }; + &i2c12 { reg = <0x680 0x80>, <0xd80 0x20>; }; + &i2c13 { reg = <0x700 0x80>, <0xda0 0x20>; }; + &i2c14 { reg = <0x780 0x80>, <0xdc0 0x20>; }; + &i2c15 { reg = <0x800 0x80>, <0xde0 0x20>; }; + - compatible : should be "aspeed,ast2400-i2c-bus" or "aspeed,ast2500-i2c-bus" or "aspeed,ast2600-i2c-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 both DMA and buffer modes are enabled in device + tree, DMA mode will be selected. + + 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,12 +100,21 @@ 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 { @@ -39,11 +122,40 @@ i2c { #size-cells = <0>; #interrupt-cells = <1>; reg = <0x40 0x40>; - compatible = "aspeed,ast2400-i2c-bus"; + compatible = "aspeed,ast2500-i2c-bus"; clocks = <&syscon ASPEED_CLK_APB>; resets = <&syscon ASPEED_RESET_I2C>; bus-frequency = <100000>; interrupts = <0>; interrupt-parent = <&i2c_ic>; }; + + /* buffer mode transfer enabled */ + i2c1: i2c-bus@80 { + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <1>; + reg = <0x80 0x40>, <0x210 0x10>; + compatible = "aspeed,ast2500-i2c-bus"; + clocks = <&syscon ASPEED_CLK_APB>; + resets = <&syscon ASPEED_RESET_I2C>; + bus-frequency = <100000>; + interrupts = <1>; + interrupt-parent = <&i2c_ic>; + }; + + /* DMA mode transfer enabled */ + i2c2: i2c-bus@c0 { + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <1>; + reg = <0xc0 0x40>; + aspeed,dma-buf-size = <4095>; + compatible = "aspeed,ast2500-i2c-bus"; + clocks = <&syscon ASPEED_CLK_APB>; + resets = <&syscon ASPEED_RESET_I2C>; + bus-frequency = <100000>; + interrupts = <2>; + interrupt-parent = <&i2c_ic>; + }; };
Append bindings to support buffer mode and DMA mode transfer. Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> --- 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 | 126 +++++++++++++++++- 1 file changed, 119 insertions(+), 7 deletions(-)