| Message ID | 20210112003749.10565-1-jae.hyun.yoo@linux.intel.com |
|---|---|
| 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/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?
This patch series adds buffer mode and DMA mode transfer support for the Aspeed I2C driver. With this change, default transfer mode will be set to buffer mode for better performance, and DMA mode can be selectively used depends on platform configuration. * Buffer mode AST2400: It has 2 KBytes (256 Bytes x 8 pages) of I2C SRAM buffer pool from 0x1e78a800 to 0x1e78afff that can be used for all busses with buffer pool manipulation. To simplify implementation for supporting both AST2400 and AST2500, it assigns each 128 Bytes per bus without using buffer pool manipulation so total 1792 Bytes of I2C SRAM buffer will be used. AST2500: It has 16 Bytes of individual I2C SRAM buffer per each bus and its range is from 0x1e78a200 to 0x1e78a2df, so it doesn't have 'buffer page selection' bit field in the Function control register, and neither 'base address pointer' bit field in the Pool buffer control register it has. To simplify implementation for supporting both AST2400 and AST2500, it writes zeros on those register bit fields but it's okay because it does nothing in AST2500. AST2600: It has 32 Bytes of individual I2C SRAM buffer per each bus and its range is from 0x1e78ac00 to 0x1e78adff. Works just like AST2500 does. * DMA mode Only AST2500 and later versions support DMA mode under some limitations in case of AST2500: I2C is sharing the DMA H/W with UHCI host controller and MCTP controller. Since those controllers operate with DMA mode only, I2C has to use buffer mode or byte mode instead if one of those controllers is enabled. Also make sure that if SD/eMMC or Port80 snoop uses DMA mode instead of PIO or FIFO respectively, I2C can't use DMA mode. Please review it. Changes since v1: V1: https://lore.kernel.org/linux-arm-kernel/20191007231313.4700-1-jae.hyun.yoo@linux.intel.com/ - Removed a bug fix patch which was merged already from this patch series. - Removed buffer reg settings from default device tree and added the settings into bindings document to show the predefined buffer range per each bus. - Updated commit message and comments. - Refined driver code using abstract functions. Jae Hyun Yoo (4): dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support ARM: dts: aspeed: modify I2C node to support buffer mode i2c: aspeed: add buffer mode transfer support i2c: aspeed: add DMA mode transfer support .../devicetree/bindings/i2c/i2c-aspeed.txt | 126 +++- arch/arm/boot/dts/aspeed-g4.dtsi | 19 +- arch/arm/boot/dts/aspeed-g5.dtsi | 19 +- drivers/i2c/busses/i2c-aspeed.c | 553 ++++++++++++++++-- 4 files changed, 667 insertions(+), 50 deletions(-)