| Message ID | 20230220061745.1973981-2-ryan_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add ASPEED AST2600 I2Cv2 controller driver | expand |
Hi Ryan, > AST2600 support new register set for I2Cv2 controller, add bindings > document to support driver of i2cv2 new register mode controller. Some comments inline: > + clock-frequency: > + description: > + Desired I2C bus clock frequency in Hz. default 100khz. > + > + multi-master: > + type: boolean > + description: > + states that there is another master active on this bus These are common to all i2c controllers, but I see that i2c-controller.yaml doesn't include them (while i2c.text does). I assume we're OK to include these in the device bindings in the meantime. But in that case, you may also want to include the common "smbus-alert" property, which you consume in your driver. > + timeout: > + type: boolean > + description: Enable i2c bus timeout for master/slave (35ms) > + > + byte-mode: > + type: boolean > + description: Force i2c driver use byte mode transmit > + > + buff-mode: > + type: boolean > + description: Force i2c driver use buffer mode transmit These three aren't really a property of the hardware, more of the intended driver configuration. Do they really belong in the DT? [and how would a DT author know which modes to choose?] > + aspeed,gr: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of i2c global register node. We'll probably want this to be consistent with other instances of aspeed global register references. I've used "aspeed,global-regs" in the proposed i3c binding: https://lore.kernel.org/linux-devicetree/cover.1676532146.git.jk@codeconstruct.com.au/T/#mda2d005f77ca0c481b1f1edadb58fc1b007a5cc3 I'd argue that "global-regs" is a little more clear, but I'm okay with either way - that change has been Acked but not been merged yet. Whichever we choose though, it should be consistent. Cheers, Jeremy
On 20/02/2023 07:17, Ryan Chen wrote: > AST2600 support new register set for I2Cv2 controller, add bindings > document to support driver of i2cv2 new register mode controller. > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > --- > .../devicetree/bindings/i2c/aspeed,i2cv2.yaml | 83 +++++++++++++++++++ > 1 file changed, 83 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml New compatible is okay, but as this is the same controller as old one, this should go to old binding. There are several issues anyway here, but I won't reviewing it except few obvious cases. > > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > new file mode 100644 > index 000000000000..913fb45d5fbe > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > @@ -0,0 +1,83 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/i2c/aspeed,i2cv2.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ASPEED I2Cv2 Controller on the AST26XX SoCs > + > +maintainers: > + - Ryan Chen <ryan_chen@aspeedtech.com> > + > +allOf: > + - $ref: /schemas/i2c/i2c-controller.yaml# > + > +properties: > + compatible: > + enum: > + - aspeed,ast2600-i2cv2 > + > + reg: > + minItems: 1 > + items: > + - description: address offset and range of register > + - description: address offset and range of buffer register Why this is optional? > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + description: > + Reference clock for the I2C bus > + > + resets: > + maxItems: 1 > + > + clock-frequency: > + description: > + Desired I2C bus clock frequency in Hz. default 100khz. > + > + multi-master: > + type: boolean > + description: > + states that there is another master active on this bus Drop description and type. Just :true. > + > + timeout: > + type: boolean > + description: Enable i2c bus timeout for master/slave (35ms) Why this is property for DT? It's for sure not bool, but proper type coming from units. > + > + byte-mode: > + type: boolean > + description: Force i2c driver use byte mode transmit Drop, not a DT property. > + > + buff-mode: > + type: boolean > + description: Force i2c driver use buffer mode transmit Drop, not a DT property. > + > + aspeed,gr: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of i2c global register node. > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - resets > + - aspeed,gr > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/ast2600-clock.h> > + i2c: i2c-bus@80 { You did not test the bindings... This is i2c. Best regards, Krzysztof
Hello Jeremy, Thanks your review. > -----Original Message----- > From: Jeremy Kerr <jk@codeconstruct.com.au> > Sent: Monday, February 20, 2023 4:29 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > Hi Ryan, > > > AST2600 support new register set for I2Cv2 controller, add bindings > > document to support driver of i2cv2 new register mode controller. > > Some comments inline: > > > + clock-frequency: > > + description: > > + Desired I2C bus clock frequency in Hz. default 100khz. > > + > > + multi-master: > > + type: boolean > > + description: > > + states that there is another master active on this bus > > These are common to all i2c controllers, but I see that i2c-controller.yaml > doesn't include them (while i2c.text does). > > I assume we're OK to include these in the device bindings in the meantime. > But in that case, you may also want to include the common "smbus-alert" > property, which you consume in your driver. > Since i2c.text have multi-master, smbus-alert. I don't need those two right? > > + timeout: > > + type: boolean > > + description: Enable i2c bus timeout for master/slave (35ms) > > + > > + byte-mode: > > + type: boolean > > + description: Force i2c driver use byte mode transmit > > + > > + buff-mode: > > + type: boolean > > + description: Force i2c driver use buffer mode transmit > > These three aren't really a property of the hardware, more of the intended > driver configuration. Do they really belong in the DT? > Sorry, I am confused. This is hardware controller mode setting for each i2c transfer. So I add it in property for change different i2c transfer mode. Is my mis-understand the property setting? > [and how would a DT author know which modes to choose?] > > > + aspeed,gr: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of i2c global register node. > > We'll probably want this to be consistent with other instances of aspeed global > register references. I've used "aspeed,global-regs" in the proposed i3c binding: > > > https://lore.kernel.org/linux-devicetree/cover.1676532146.git.jk@codeconstruc > t.com.au/T/#mda2d005f77ca0c481b1f1edadb58fc1b007a5cc3 > > I'd argue that "global-regs" is a little more clear, but I'm okay with either way - > that change has been Acked but not been merged yet. > Whichever we choose though, it should be consistent. > Got it, will rename to aspeed,global-regs Best Regards, Ryan
On 20/02/2023 10:50, Ryan Chen wrote: >> These are common to all i2c controllers, but I see that i2c-controller.yaml >> doesn't include them (while i2c.text does). >> >> I assume we're OK to include these in the device bindings in the meantime. >> But in that case, you may also want to include the common "smbus-alert" >> property, which you consume in your driver. >> > Since i2c.text have multi-master, smbus-alert. I don't need those two right? Yes, these should be dropped. > >>> + timeout: >>> + type: boolean >>> + description: Enable i2c bus timeout for master/slave (35ms) >>> + >>> + byte-mode: >>> + type: boolean >>> + description: Force i2c driver use byte mode transmit >>> + >>> + buff-mode: >>> + type: boolean >>> + description: Force i2c driver use buffer mode transmit >> >> These three aren't really a property of the hardware, more of the intended >> driver configuration. Do they really belong in the DT? >> > Sorry, I am confused. > This is hardware controller mode setting for each i2c transfer. > So I add it in property for change different i2c transfer mode. > Is my mis-understand the property setting? You described the property as "Force I2C driver", so it is a not correct description. DTS is not for controlling driver. You must describe here hardware feature/property, not driver behavior. Also, it must be clear for us why this is being customized per each board. > >> [and how would a DT author know which modes to choose?] Exactly. Best regards, Krzysztof
Hi Ryan, > > > + clock-frequency: > > > + description: > > > + Desired I2C bus clock frequency in Hz. default 100khz. > > > + > > > + multi-master: > > > + type: boolean > > > + description: > > > + states that there is another master active on this bus > > > > These are common to all i2c controllers, but I see that i2c-controller.yaml > > doesn't include them (while i2c.text does). > > > > I assume we're OK to include these in the device bindings in the meantime. > > But in that case, you may also want to include the common "smbus-alert" > > property, which you consume in your driver. > > > Since i2c.text have multi-master, smbus-alert. I don't need those two right? Depends whether the maintainers consider i2c.text as part of the schema, I figure. Might be best to get their input on this. > > > + timeout: > > > + type: boolean > > > + description: Enable i2c bus timeout for master/slave (35ms) > > > + > > > + byte-mode: > > > + type: boolean > > > + description: Force i2c driver use byte mode transmit > > > + > > > + buff-mode: > > > + type: boolean > > > + description: Force i2c driver use buffer mode transmit > > > > These three aren't really a property of the hardware, more of the intended > > driver configuration. Do they really belong in the DT? > > > Sorry, I am confused. > This is hardware controller mode setting for each i2c transfer. > So I add it in property for change different i2c transfer mode. > Is my mis-understand the property setting? It depends what this is configuration is for. Would you set the transfer mode based on the design of the board? Is there something about the physical i2c bus wiring (or some other hardware design choice) that would mean you use one setting over another? On the other hand, if it's just because of OS behaviour, then this doesn't belong in the DT. Maybe to help us understand: why would you ever *not* want DMA mode? Isn't that always preferable? Cheers, Jeremy
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, February 20, 2023 4:35 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > On 20/02/2023 07:17, Ryan Chen wrote: > > AST2600 support new register set for I2Cv2 controller, add bindings > > document to support driver of i2cv2 new register mode controller. > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > --- > > .../devicetree/bindings/i2c/aspeed,i2cv2.yaml | 83 > > +++++++++++++++++++ > > 1 file changed, 83 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > New compatible is okay, but as this is the same controller as old one, this > should go to old binding. > > There are several issues anyway here, but I won't reviewing it except few > obvious cases. > > > > > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > new file mode 100644 > > index 000000000000..913fb45d5fbe > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > @@ -0,0 +1,83 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/i2c/aspeed,i2cv2.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ASPEED I2Cv2 Controller on the AST26XX SoCs > > + > > +maintainers: > > + - Ryan Chen <ryan_chen@aspeedtech.com> > > + > > +allOf: > > + - $ref: /schemas/i2c/i2c-controller.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - aspeed,ast2600-i2cv2 > > + > > + reg: > > + minItems: 1 > > + items: > > + - description: address offset and range of register > > + - description: address offset and range of buffer register > > Why this is optional? Will modify minItems: 1 to 2 > > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + description: > > + Reference clock for the I2C bus > > + > > + resets: > > + maxItems: 1 > > + > > + clock-frequency: > > + description: > > + Desired I2C bus clock frequency in Hz. default 100khz. > > + > > + multi-master: > > + type: boolean > > + description: > > + states that there is another master active on this bus > > Drop description and type. Just :true. > Since i2c.txt have multi-master will drop it. > > + > > + timeout: > > + type: boolean > > + description: Enable i2c bus timeout for master/slave (35ms) > > Why this is property for DT? It's for sure not bool, but proper type coming from > units. This is i2c controller feature for enable slave mode inactive timeout and also master mode sda/scl auto release timeout. So I will modify to aspeed,timeout: type: boolean description: I2C bus timeout enable for master/slave mode > > + > > + byte-mode: > > + type: boolean > > + description: Force i2c driver use byte mode transmit > > Drop, not a DT property. > > > + > > + buff-mode: > > + type: boolean > > + description: Force i2c driver use buffer mode transmit > > Drop, not a DT property. > The controller support 3 different for transfer. Byte mode: it means step by step to issue transfer. Example i2c read, each step will issue interrupt then enable next step. Sr (start read) | D | D | D | P Buffer mode: it means, the data can prepare into buffer register, then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. The DMA mode most like with buffer mode, The differ is data prepare in DRAM, than trigger transfer. So, should I modify to aspeed,byte: type: boolean description: Enable i2c controller transfer with byte mode aspeed,buff: type: boolean description: Enable i2c controller transfer with buff mode > > + > > + aspeed,gr: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of i2c global register node. > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - resets > > + - aspeed,gr > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/clock/ast2600-clock.h> > > + i2c: i2c-bus@80 { > > You did not test the bindings... This is i2c. > I do use command : make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml To test it. is it not correct method? I will modify "i2c: i2c-bus@80" -> "i2c0: i2c@80" > > Best regards, > Krzysztof
Hello Jeremy, > -----Original Message----- > From: Jeremy Kerr <jk@codeconstruct.com.au> > Sent: Monday, February 20, 2023 7:24 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > Hi Ryan, > > > > > + clock-frequency: > > > > + description: > > > > + Desired I2C bus clock frequency in Hz. default 100khz. > > > > + > > > > + multi-master: > > > > + type: boolean > > > > + description: > > > > + states that there is another master active on this bus > > > > > > These are common to all i2c controllers, but I see that > > > i2c-controller.yaml doesn't include them (while i2c.text does). > > > > > > I assume we're OK to include these in the device bindings in the meantime. > > > But in that case, you may also want to include the common "smbus-alert" > > > property, which you consume in your driver. > > > > > Since i2c.text have multi-master, smbus-alert. I don't need those two right? > > Depends whether the maintainers consider i2c.text as part of the schema, I > figure. Might be best to get their input on this. Yes, I will drop this, also integrate into aspeed,i2c.yaml file. > > > > + timeout: > > > > + type: boolean > > > > + description: Enable i2c bus timeout for master/slave (35ms) > > > > + > > > > + byte-mode: > > > > + type: boolean > > > > + description: Force i2c driver use byte mode transmit > > > > + > > > > + buff-mode: > > > > + type: boolean > > > > + description: Force i2c driver use buffer mode transmit > > > > > > These three aren't really a property of the hardware, more of the > > > intended driver configuration. Do they really belong in the DT? > > > > > Sorry, I am confused. > > This is hardware controller mode setting for each i2c transfer. > > So I add it in property for change different i2c transfer mode. > > Is my mis-understand the property setting? > > It depends what this is configuration is for. > > Would you set the transfer mode based on the design of the board? Is there > something about the physical i2c bus wiring (or some other hardware design > choice) that would mean you use one setting over another? > No, it not depend on board design. It is only for register control for controller transfer behave. The controller support 3 different trigger mode for transfer. Byte mode: it means step by step to issue transfer. Example i2c read, each step will issue interrupt then driver need trigger for next step. Sr (start read) | D | D | D | P Buffer mode: it means, the data can prepare into buffer register, then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. The DMA mode most like with buffer mode, The differ is data prepare in DRAM, than trigger transfer. > On the other hand, if it's just because of OS behaviour, then this doesn't belong > in the DT. > > Maybe to help us understand: why would you ever *not* want DMA mode? > Isn't that always preferable? In AST SOC i2c design is 16 i2c bus share one dma engine. It can be switch setting by dts setting. Otherwise driver by default probe is DMA mode. Ryan
On 21/02/2023 03:43, Ryan Chen wrote: > Hello Krzysztof, > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Monday, February 20, 2023 4:35 PM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring >> <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew >> Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; >> openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 >> >> On 20/02/2023 07:17, Ryan Chen wrote: >>> AST2600 support new register set for I2Cv2 controller, add bindings >>> document to support driver of i2cv2 new register mode controller. >>> >>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> >>> --- >>> .../devicetree/bindings/i2c/aspeed,i2cv2.yaml | 83 >>> +++++++++++++++++++ >>> 1 file changed, 83 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml >> >> New compatible is okay, but as this is the same controller as old one, this >> should go to old binding. >> >> There are several issues anyway here, but I won't reviewing it except few >> obvious cases. >> >>> >>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml >>> b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml >>> new file mode 100644 >>> index 000000000000..913fb45d5fbe >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml >>> @@ -0,0 +1,83 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/i2c/aspeed,i2cv2.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: ASPEED I2Cv2 Controller on the AST26XX SoCs >>> + >>> +maintainers: >>> + - Ryan Chen <ryan_chen@aspeedtech.com> >>> + >>> +allOf: >>> + - $ref: /schemas/i2c/i2c-controller.yaml# >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - aspeed,ast2600-i2cv2 >>> + >>> + reg: >>> + minItems: 1 >>> + items: >>> + - description: address offset and range of register >>> + - description: address offset and range of buffer register >> >> Why this is optional? > > Will modify minItems: 1 to 2 >> >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 1 >>> + description: >>> + Reference clock for the I2C bus >>> + >>> + resets: >>> + maxItems: 1 >>> + >>> + clock-frequency: >>> + description: >>> + Desired I2C bus clock frequency in Hz. default 100khz. >>> + >>> + multi-master: >>> + type: boolean >>> + description: >>> + states that there is another master active on this bus >> >> Drop description and type. Just :true. >> > Since i2c.txt have multi-master will drop it. >>> + >>> + timeout: >>> + type: boolean >>> + description: Enable i2c bus timeout for master/slave (35ms) >> >> Why this is property for DT? It's for sure not bool, but proper type coming from >> units. > This is i2c controller feature for enable slave mode inactive timeout and > also master mode sda/scl auto release timeout. > So I will modify to > aspeed,timeout: > type: boolean > description: I2C bus timeout enable for master/slave mode This does not answer my concerns. Why this is board specific? > >>> + >>> + byte-mode: >>> + type: boolean >>> + description: Force i2c driver use byte mode transmit >> >> Drop, not a DT property. >> >>> + >>> + buff-mode: >>> + type: boolean >>> + description: Force i2c driver use buffer mode transmit >> >> Drop, not a DT property. >> > The controller support 3 different for transfer. > Byte mode: it means step by step to issue transfer. > Example i2c read, each step will issue interrupt then enable next step. > Sr (start read) | D | D | D | P > Buffer mode: it means, the data can prepare into buffer register, then > Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. > The DMA mode most like with buffer mode, > The differ is data prepare in DRAM, than trigger transfer. > > So, should I modify to > aspeed,byte: > type: boolean > description: Enable i2c controller transfer with byte mode > > aspeed,buff: > type: boolean > description: Enable i2c controller transfer with buff mode 1. No, these are not bools but enum in such case. 2. And why exactly this is board-specific? Best regards, Krzysztof
On 21/02/2023 04:32, Ryan Chen wrote: > Hello Jeremy, > >> -----Original Message----- >> From: Jeremy Kerr <jk@codeconstruct.com.au> >> Sent: Monday, February 20, 2023 7:24 PM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring >> <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew >> Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; >> openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 >> >> Hi Ryan, >> >>>>> + clock-frequency: >>>>> + description: >>>>> + Desired I2C bus clock frequency in Hz. default 100khz. >>>>> + >>>>> + multi-master: >>>>> + type: boolean >>>>> + description: >>>>> + states that there is another master active on this bus >>>> >>>> These are common to all i2c controllers, but I see that >>>> i2c-controller.yaml doesn't include them (while i2c.text does). >>>> >>>> I assume we're OK to include these in the device bindings in the meantime. >>>> But in that case, you may also want to include the common "smbus-alert" >>>> property, which you consume in your driver. >>>> >>> Since i2c.text have multi-master, smbus-alert. I don't need those two right? >> >> Depends whether the maintainers consider i2c.text as part of the schema, I >> figure. Might be best to get their input on this. > > > Yes, I will drop this, also integrate into aspeed,i2c.yaml file. > >>>>> + timeout: >>>>> + type: boolean >>>>> + description: Enable i2c bus timeout for master/slave (35ms) >>>>> + >>>>> + byte-mode: >>>>> + type: boolean >>>>> + description: Force i2c driver use byte mode transmit >>>>> + >>>>> + buff-mode: >>>>> + type: boolean >>>>> + description: Force i2c driver use buffer mode transmit >>>> >>>> These three aren't really a property of the hardware, more of the >>>> intended driver configuration. Do they really belong in the DT? >>>> >>> Sorry, I am confused. >>> This is hardware controller mode setting for each i2c transfer. >>> So I add it in property for change different i2c transfer mode. >>> Is my mis-understand the property setting? >> >> It depends what this is configuration is for. >> >> Would you set the transfer mode based on the design of the board? Is there >> something about the physical i2c bus wiring (or some other hardware design >> choice) that would mean you use one setting over another? >> > No, it not depend on board design. It is only for register control for controller transfer behave. Then DT does not look like suitable place for it. Drop the property. > The controller support 3 different trigger mode for transfer. > Byte mode: it means step by step to issue transfer. > Example i2c read, each step will issue interrupt then driver need trigger for next step. > Sr (start read) | D | D | D | P > Buffer mode: it means, the data can prepare into buffer register, then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. > The DMA mode most like with buffer mode, The differ is data prepare in DRAM, than trigger transfer. > > >> On the other hand, if it's just because of OS behaviour, then this doesn't belong >> in the DT. >> >> Maybe to help us understand: why would you ever *not* want DMA mode? >> Isn't that always preferable? > In AST SOC i2c design is 16 i2c bus share one dma engine. > It can be switch setting by dts setting. Otherwise driver by default probe is DMA mode. DMA mode is chosen by existence (or lack) of dmas property, isn't it? Why do you need separate property instead of using the standard one? Best regards, Krzysztof
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, February 21, 2023 5:40 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > On 21/02/2023 03:43, Ryan Chen wrote: > > Hello Krzysztof, > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Monday, February 20, 2023 4:35 PM > >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > >> <robh+dt@kernel.org>; Krzysztof Kozlowski > >> <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; > >> Andrew Jeffery <andrew@aj.id.au>; Philipp Zabel > >> <p.zabel@pengutronix.de>; openbmc@lists.ozlabs.org; > >> linux-arm-kernel@lists.infradead.org; > >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED > >> i2Cv2 > >> > >> On 20/02/2023 07:17, Ryan Chen wrote: > >>> AST2600 support new register set for I2Cv2 controller, add bindings > >>> document to support driver of i2cv2 new register mode controller. > >>> > >>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > >>> --- > >>> .../devicetree/bindings/i2c/aspeed,i2cv2.yaml | 83 > >>> +++++++++++++++++++ > >>> 1 file changed, 83 insertions(+) > >>> create mode 100644 > >>> Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > >> > >> New compatible is okay, but as this is the same controller as old > >> one, this should go to old binding. > >> > >> There are several issues anyway here, but I won't reviewing it except > >> few obvious cases. > >> > >>> > >>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > >>> b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > >>> new file mode 100644 > >>> index 000000000000..913fb45d5fbe > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > >>> @@ -0,0 +1,83 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/i2c/aspeed,i2cv2.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: ASPEED I2Cv2 Controller on the AST26XX SoCs > >>> + > >>> +maintainers: > >>> + - Ryan Chen <ryan_chen@aspeedtech.com> > >>> + > >>> +allOf: > >>> + - $ref: /schemas/i2c/i2c-controller.yaml# > >>> + > >>> +properties: > >>> + compatible: > >>> + enum: > >>> + - aspeed,ast2600-i2cv2 > >>> + > >>> + reg: > >>> + minItems: 1 > >>> + items: > >>> + - description: address offset and range of register > >>> + - description: address offset and range of buffer register > >> > >> Why this is optional? > > > > Will modify minItems: 1 to 2 > >> > >>> + > >>> + interrupts: > >>> + maxItems: 1 > >>> + > >>> + clocks: > >>> + maxItems: 1 > >>> + description: > >>> + Reference clock for the I2C bus > >>> + > >>> + resets: > >>> + maxItems: 1 > >>> + > >>> + clock-frequency: > >>> + description: > >>> + Desired I2C bus clock frequency in Hz. default 100khz. > >>> + > >>> + multi-master: > >>> + type: boolean > >>> + description: > >>> + states that there is another master active on this bus > >> > >> Drop description and type. Just :true. > >> > > Since i2c.txt have multi-master will drop it. > >>> + > >>> + timeout: > >>> + type: boolean > >>> + description: Enable i2c bus timeout for master/slave (35ms) > >> > >> Why this is property for DT? It's for sure not bool, but proper type > >> coming from units. > > This is i2c controller feature for enable slave mode inactive timeout > > and also master mode sda/scl auto release timeout. > > So I will modify to > > aspeed,timeout: > > type: boolean > > description: I2C bus timeout enable for master/slave mode > > This does not answer my concerns. Why this is board specific? Sorry, can’t catch your point. It is not board specific. It is controller feature. ASPEED SOC chip is server product, master connect may have fingerprint connect to another board. And also support hotplug. For example I2C controller as slave mode, and suddenly disconnected. Slave state machine will keep waiting for master clock in for rx/tx transfer. So it need timeout setting to enable timeout unlock controller state. And in another side. As master mode, slave is clock stretching. The master will be keep waiting, until slave release cll stretching. So in those reason add this timeout design in controller. > > > > >>> + > >>> + byte-mode: > >>> + type: boolean > >>> + description: Force i2c driver use byte mode transmit > >> > >> Drop, not a DT property. > >> > >>> + > >>> + buff-mode: > >>> + type: boolean > >>> + description: Force i2c driver use buffer mode transmit > >> > >> Drop, not a DT property. > >> > > The controller support 3 different for transfer. > > Byte mode: it means step by step to issue transfer. > > Example i2c read, each step will issue interrupt then enable next step. > > Sr (start read) | D | D | D | P > > Buffer mode: it means, the data can prepare into buffer register, then > > Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. > > The DMA mode most like with buffer mode, The differ is data prepare in > > DRAM, than trigger transfer. > > > > So, should I modify to > > aspeed,byte: > > type: boolean > > description: Enable i2c controller transfer with byte mode > > > > aspeed,buff: > > type: boolean > > description: Enable i2c controller transfer with buff mode > > 1. No, these are not bools but enum in such case. Thanks, will modify following. aspeed,xfer_mode: enum: [0, 1, 2] description: 0: byte mode, 1: buff_mode, 2: dma_mode > 2. And why exactly this is board-specific? No, it not depends on board design. It is only for register control for controller transfer behave. The controller support 3 different trigger mode for transfer. Assign bus#1 ~ 3 : dma tranfer and assign bus#4 ~ 6 : buffer mode transfer, That can reduce the dram usage. Best regards, Ryan
On 21/02/2023 11:42, Ryan Chen wrote: >>>>> + type: boolean >>>>> + description: Enable i2c bus timeout for master/slave (35ms) >>>> >>>> Why this is property for DT? It's for sure not bool, but proper type >>>> coming from units. >>> This is i2c controller feature for enable slave mode inactive timeout >>> and also master mode sda/scl auto release timeout. >>> So I will modify to >>> aspeed,timeout: >>> type: boolean >>> description: I2C bus timeout enable for master/slave mode >> >> This does not answer my concerns. Why this is board specific? > Sorry, can’t catch your point. > It is not board specific. It is controller feature. > ASPEED SOC chip is server product, master connect may have fingerprint > connect to another board. And also support hotplug. > For example I2C controller as slave mode, and suddenly disconnected. > Slave state machine will keep waiting for master clock in for rx/tx transfer. > So it need timeout setting to enable timeout unlock controller state. > And in another side. As master mode, slave is clock stretching. > The master will be keep waiting, until slave release cll stretching. OK, thanks for describing the feature. I still do not see how this is DT related. > > So in those reason add this timeout design in controller. You need to justify why DT is correct place for this property. DT is not for configuring OS, but to describe hardware. I gave you one possibility - why different boards would like to set this property. You said it is not board specific, thus all boards will have it (or none of them). Without any other reason, this is not a DT property. Drop. >> >>> >>>>> + >>>>> + byte-mode: >>>>> + type: boolean >>>>> + description: Force i2c driver use byte mode transmit >>>> >>>> Drop, not a DT property. >>>> >>>>> + >>>>> + buff-mode: >>>>> + type: boolean >>>>> + description: Force i2c driver use buffer mode transmit >>>> >>>> Drop, not a DT property. >>>> >>> The controller support 3 different for transfer. >>> Byte mode: it means step by step to issue transfer. >>> Example i2c read, each step will issue interrupt then enable next step. >>> Sr (start read) | D | D | D | P >>> Buffer mode: it means, the data can prepare into buffer register, then >>> Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. >>> The DMA mode most like with buffer mode, The differ is data prepare in >>> DRAM, than trigger transfer. >>> >>> So, should I modify to >>> aspeed,byte: >>> type: boolean >>> description: Enable i2c controller transfer with byte mode >>> >>> aspeed,buff: >>> type: boolean >>> description: Enable i2c controller transfer with buff mode >> >> 1. No, these are not bools but enum in such case. > > Thanks, will modify following. > aspeed,xfer_mode: > enum: [0, 1, 2] > description: > 0: byte mode, 1: buff_mode, 2: dma_mode Just keep it text - byte, buffered, dma > >> 2. And why exactly this is board-specific? > > No, it not depends on board design. It is only for register control for controller transfer behave. > The controller support 3 different trigger mode for transfer. > Assign bus#1 ~ 3 : dma tranfer and assign bus#4 ~ 6 : buffer mode transfer, > That can reduce the dram usage. Then anyway it does not look like property for Devicetree. DT describes hardware, not OS behavior. Best regards, Krzysztof
On 2/20/2023 7:32 PM, Ryan Chen wrote: >>>>> + timeout: >>>>> + type: boolean >>>>> + description: Enable i2c bus timeout for master/slave (35ms) >>>>> + >>>>> + byte-mode: >>>>> + type: boolean >>>>> + description: Force i2c driver use byte mode transmit >>>>> + >>>>> + buff-mode: >>>>> + type: boolean >>>>> + description: Force i2c driver use buffer mode transmit >>>> >>>> These three aren't really a property of the hardware, more of the >>>> intended driver configuration. Do they really belong in the DT? >>>> >>> Sorry, I am confused. >>> This is hardware controller mode setting for each i2c transfer. >>> So I add it in property for change different i2c transfer mode. >>> Is my mis-understand the property setting? >> >> It depends what this is configuration is for. >> >> Would you set the transfer mode based on the design of the board? Is there >> something about the physical i2c bus wiring (or some other hardware design >> choice) that would mean you use one setting over another? >> > No, it not depend on board design. It is only for register control for controller transfer behave. > The controller support 3 different trigger mode for transfer. > Byte mode: it means step by step to issue transfer. > Example i2c read, each step will issue interrupt then driver need trigger for next step. > Sr (start read) | D | D | D | P > Buffer mode: it means, the data can prepare into buffer register, then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. > The DMA mode most like with buffer mode, The differ is data prepare in DRAM, than trigger transfer. > > Unless these settings like xfer mode are per i2c bus, it could be just a module parameter? Not sure anything other than default mode would be used if DMA mode works for all master/slave transactions. Regards, Dhananjay
Hi Ryan, > > On the other hand, if it's just because of OS behaviour, then this > > doesn't belong > > in the DT. > > > > Maybe to help us understand: why would you ever *not* want DMA > > mode? > > Isn't that always preferable? > In AST SOC i2c design is 16 i2c bus share one dma engine. Does this mean that only one i2c controller in the system can be configured to use DMA? Or is it able to be shared between multiple controllers? > It can be switch setting by dts setting. Otherwise driver by default > probe is DMA mode. You've explained what the modes do, and how they're switched, and what the default is. However this doesn't explain *why* someone would want to choose a particular mode when creating a controller node. Still with the question above: assuming there are no restrictions on DMA usage, why wouldn't a driver implementation just enable it always? Cheers, Jeremy
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, February 21, 2023 7:05 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > On 21/02/2023 11:42, Ryan Chen wrote: > >>>>> + type: boolean > >>>>> + description: Enable i2c bus timeout for master/slave (35ms) > >>>> > >>>> Why this is property for DT? It's for sure not bool, but proper > >>>> type coming from units. > >>> This is i2c controller feature for enable slave mode inactive > >>> timeout and also master mode sda/scl auto release timeout. > >>> So I will modify to > >>> aspeed,timeout: > >>> type: boolean > >>> description: I2C bus timeout enable for master/slave mode > >> > >> This does not answer my concerns. Why this is board specific? > > Sorry, can’t catch your point. > > It is not board specific. It is controller feature. > > ASPEED SOC chip is server product, master connect may have fingerprint > > connect to another board. And also support hotplug. > > For example I2C controller as slave mode, and suddenly disconnected. > > Slave state machine will keep waiting for master clock in for rx/tx transfer. > > So it need timeout setting to enable timeout unlock controller state. > > And in another side. As master mode, slave is clock stretching. > > The master will be keep waiting, until slave release cll stretching. > > OK, thanks for describing the feature. I still do not see how this is DT related. Let me draw more about the board-specific. The following is an example about i2c layout in board. Board A Board B -------------------------------------------------------- -------------------------------------------------------- | i2c bus#1(master/slave) <--------------------> fingerprint.(can be unplug) <--------------------> i2c bus#x (master/slave) | | i2c bus#2(master) -> tmp i2c device | | | | i2c bus#3(master) -> adc i2c device | | | -------------------------------------------------------- -------------------------------------------------------- In this case i2c bus#1 need enable timeout, avoid suddenly unplug the connector. That slave will keep state to drive clock stretching. So it is specific enable in i2c bus#1. Others is not needed enable timeout. Does this draw is more clear in scenario? > > > > So in those reason add this timeout design in controller. > > You need to justify why DT is correct place for this property. DT is not for > configuring OS, but to describe hardware. I gave you one possibility > - why different boards would like to set this property. You said it is not board > specific, thus all boards will have it (or none of them). > Without any other reason, this is not a DT property. Drop. > > >> > >>> > >>>>> + > >>>>> + byte-mode: > >>>>> + type: boolean > >>>>> + description: Force i2c driver use byte mode transmit > >>>> > >>>> Drop, not a DT property. > >>>> > >>>>> + > >>>>> + buff-mode: > >>>>> + type: boolean > >>>>> + description: Force i2c driver use buffer mode transmit > >>>> > >>>> Drop, not a DT property. > >>>> > >>> The controller support 3 different for transfer. > >>> Byte mode: it means step by step to issue transfer. > >>> Example i2c read, each step will issue interrupt then enable next step. > >>> Sr (start read) | D | D | D | P > >>> Buffer mode: it means, the data can prepare into buffer register, > >>> then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. > >>> The DMA mode most like with buffer mode, The differ is data prepare > >>> in DRAM, than trigger transfer. > >>> > >>> So, should I modify to > >>> aspeed,byte: > >>> type: boolean > >>> description: Enable i2c controller transfer with byte mode > >>> > >>> aspeed,buff: > >>> type: boolean > >>> description: Enable i2c controller transfer with buff mode > >> > >> 1. No, these are not bools but enum in such case. > > > > Thanks, will modify following. > > aspeed,xfer_mode: > > enum: [0, 1, 2] > > description: > > 0: byte mode, 1: buff_mode, 2: dma_mode > > Just keep it text - byte, buffered, dma > > > > >> 2. And why exactly this is board-specific? > > > > No, it not depends on board design. It is only for register control for > controller transfer behave. > > The controller support 3 different trigger mode for transfer. > > Assign bus#1 ~ 3 : dma tranfer and assign bus#4 ~ 6 : buffer mode > > transfer, That can reduce the dram usage. > > Then anyway it does not look like property for Devicetree. DT describes > hardware, not OS behavior. The same draw, in this case, i2c bus#1 that is multi-master transfer architecture. Both will inactive with trunk data. That cane enable i2c#1 use DMA transfer to reduce CPU utilized. Others (bus#2/3) can keep byte/buff mode. Board A Board B -------------------------------------------------------- -------------------------------------------------------- | i2c bus#1(master/slave) <--------------------> fingerprint.(can be unplug) <--------------------> i2c bus#x (master/slave) | | i2c bus#2(master) -> tmp i2c device | | | | i2c bus#3(master) -> adc i2c device | | | -------------------------------------------------------- -------------------------------------------------------- Best regards, Ryan
On 22/02/2023 03:59, Ryan Chen wrote: > Hello Krzysztof, > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Tuesday, February 21, 2023 7:05 PM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring >> <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew >> Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; >> openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 >> >> On 21/02/2023 11:42, Ryan Chen wrote: >>>>>>> + type: boolean >>>>>>> + description: Enable i2c bus timeout for master/slave (35ms) >>>>>> >>>>>> Why this is property for DT? It's for sure not bool, but proper >>>>>> type coming from units. >>>>> This is i2c controller feature for enable slave mode inactive >>>>> timeout and also master mode sda/scl auto release timeout. >>>>> So I will modify to >>>>> aspeed,timeout: >>>>> type: boolean >>>>> description: I2C bus timeout enable for master/slave mode >>>> >>>> This does not answer my concerns. Why this is board specific? >>> Sorry, can’t catch your point. >>> It is not board specific. It is controller feature. >>> ASPEED SOC chip is server product, master connect may have fingerprint >>> connect to another board. And also support hotplug. >>> For example I2C controller as slave mode, and suddenly disconnected. >>> Slave state machine will keep waiting for master clock in for rx/tx transfer. >>> So it need timeout setting to enable timeout unlock controller state. >>> And in another side. As master mode, slave is clock stretching. >>> The master will be keep waiting, until slave release cll stretching. >> >> OK, thanks for describing the feature. I still do not see how this is DT related. > > Let me draw more about the board-specific. > The following is an example about i2c layout in board. > Board A Board B > -------------------------------------------------------- -------------------------------------------------------- > | i2c bus#1(master/slave) <--------------------> fingerprint.(can be unplug) <--------------------> i2c bus#x (master/slave) | > | i2c bus#2(master) -> tmp i2c device | | | > | i2c bus#3(master) -> adc i2c device | | | > -------------------------------------------------------- -------------------------------------------------------- > In this case i2c bus#1 need enable timeout, avoid suddenly unplug the connector. That slave will keep state to drive clock stretching. > So it is specific enable in i2c bus#1. Others is not needed enable timeout. > Does this draw is more clear in scenario? I2C bus #1 works in slave mode? So you always need it for slave work? > >>> >>> So in those reason add this timeout design in controller. >> >> You need to justify why DT is correct place for this property. DT is not for >> configuring OS, but to describe hardware. I gave you one possibility >> - why different boards would like to set this property. You said it is not board >> specific, thus all boards will have it (or none of them). >> Without any other reason, this is not a DT property. Drop. >> >>>> >>>>> >>>>>>> + >>>>>>> + byte-mode: >>>>>>> + type: boolean >>>>>>> + description: Force i2c driver use byte mode transmit >>>>>> >>>>>> Drop, not a DT property. >>>>>> >>>>>>> + >>>>>>> + buff-mode: >>>>>>> + type: boolean >>>>>>> + description: Force i2c driver use buffer mode transmit >>>>>> >>>>>> Drop, not a DT property. >>>>>> >>>>> The controller support 3 different for transfer. >>>>> Byte mode: it means step by step to issue transfer. >>>>> Example i2c read, each step will issue interrupt then enable next step. >>>>> Sr (start read) | D | D | D | P >>>>> Buffer mode: it means, the data can prepare into buffer register, >>>>> then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. >>>>> The DMA mode most like with buffer mode, The differ is data prepare >>>>> in DRAM, than trigger transfer. >>>>> >>>>> So, should I modify to >>>>> aspeed,byte: >>>>> type: boolean >>>>> description: Enable i2c controller transfer with byte mode >>>>> >>>>> aspeed,buff: >>>>> type: boolean >>>>> description: Enable i2c controller transfer with buff mode >>>> >>>> 1. No, these are not bools but enum in such case. >>> >>> Thanks, will modify following. >>> aspeed,xfer_mode: >>> enum: [0, 1, 2] >>> description: >>> 0: byte mode, 1: buff_mode, 2: dma_mode >> >> Just keep it text - byte, buffered, dma >> >>> >>>> 2. And why exactly this is board-specific? >>> >>> No, it not depends on board design. It is only for register control for >> controller transfer behave. >>> The controller support 3 different trigger mode for transfer. >>> Assign bus#1 ~ 3 : dma tranfer and assign bus#4 ~ 6 : buffer mode >>> transfer, That can reduce the dram usage. >> >> Then anyway it does not look like property for Devicetree. DT describes >> hardware, not OS behavior. > > The same draw, in this case, i2c bus#1 that is multi-master transfer architecture. > Both will inactive with trunk data. That cane enable i2c#1 use DMA transfer to reduce CPU utilized. > Others (bus#2/3) can keep byte/buff mode. Isn't then current bus configuration for I2C#1 known to the driver? Jeremy asked few other questions around here... Best regards, Krzysztof
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Wednesday, February 22, 2023 4:26 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > On 22/02/2023 03:59, Ryan Chen wrote: > > Hello Krzysztof, > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Tuesday, February 21, 2023 7:05 PM > >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > >> <robh+dt@kernel.org>; Krzysztof Kozlowski > >> <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; > >> Andrew Jeffery <andrew@aj.id.au>; Philipp Zabel > >> <p.zabel@pengutronix.de>; openbmc@lists.ozlabs.org; > >> linux-arm-kernel@lists.infradead.org; > >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED > >> i2Cv2 > >> > >> On 21/02/2023 11:42, Ryan Chen wrote: > >>>>>>> + type: boolean > >>>>>>> + description: Enable i2c bus timeout for master/slave (35ms) > >>>>>> > >>>>>> Why this is property for DT? It's for sure not bool, but proper > >>>>>> type coming from units. > >>>>> This is i2c controller feature for enable slave mode inactive > >>>>> timeout and also master mode sda/scl auto release timeout. > >>>>> So I will modify to > >>>>> aspeed,timeout: > >>>>> type: boolean > >>>>> description: I2C bus timeout enable for master/slave mode > >>>> > >>>> This does not answer my concerns. Why this is board specific? > >>> Sorry, can’t catch your point. > >>> It is not board specific. It is controller feature. > >>> ASPEED SOC chip is server product, master connect may have > >>> fingerprint connect to another board. And also support hotplug. > >>> For example I2C controller as slave mode, and suddenly disconnected. > >>> Slave state machine will keep waiting for master clock in for rx/tx transfer. > >>> So it need timeout setting to enable timeout unlock controller state. > >>> And in another side. As master mode, slave is clock stretching. > >>> The master will be keep waiting, until slave release cll stretching. > >> > >> OK, thanks for describing the feature. I still do not see how this is DT > related. > > > > Let me draw more about the board-specific. > > The following is an example about i2c layout in board. > > Board A > Board B > > -------------------------------------------------------- > -------------------------------------------------------- > > | i2c bus#1(master/slave) <--------------------> fingerprint.(can be unplug) > <--------------------> i2c bus#x (master/slave) | > > | i2c bus#2(master) -> tmp i2c device | > | | > > | i2c bus#3(master) -> adc i2c device | | > | > > -------------------------------------------------------- > -------------------------------------------------------- > > In this case i2c bus#1 need enable timeout, avoid suddenly unplug the > connector. That slave will keep state to drive clock stretching. > > So it is specific enable in i2c bus#1. Others is not needed enable timeout. > > Does this draw is more clear in scenario? > > I2C bus #1 works in slave mode? So you always need it for slave work? Yes, it is both slave/master mode. It is always dual role. Slave must always work. Due to another board master will send. > > > >>> > >>> So in those reason add this timeout design in controller. > >> > >> You need to justify why DT is correct place for this property. DT is > >> not for configuring OS, but to describe hardware. I gave you one > >> possibility > >> - why different boards would like to set this property. You said it > >> is not board specific, thus all boards will have it (or none of them). > >> Without any other reason, this is not a DT property. Drop. > >> > >>>> > >>>>> > >>>>>>> + > >>>>>>> + byte-mode: > >>>>>>> + type: boolean > >>>>>>> + description: Force i2c driver use byte mode transmit > >>>>>> > >>>>>> Drop, not a DT property. > >>>>>> > >>>>>>> + > >>>>>>> + buff-mode: > >>>>>>> + type: boolean > >>>>>>> + description: Force i2c driver use buffer mode transmit > >>>>>> > >>>>>> Drop, not a DT property. > >>>>>> > >>>>> The controller support 3 different for transfer. > >>>>> Byte mode: it means step by step to issue transfer. > >>>>> Example i2c read, each step will issue interrupt then enable next step. > >>>>> Sr (start read) | D | D | D | P > >>>>> Buffer mode: it means, the data can prepare into buffer register, > >>>>> then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. > >>>>> The DMA mode most like with buffer mode, The differ is data > >>>>> prepare in DRAM, than trigger transfer. > >>>>> > >>>>> So, should I modify to > >>>>> aspeed,byte: > >>>>> type: boolean > >>>>> description: Enable i2c controller transfer with byte mode > >>>>> > >>>>> aspeed,buff: > >>>>> type: boolean > >>>>> description: Enable i2c controller transfer with buff mode > >>>> > >>>> 1. No, these are not bools but enum in such case. > >>> > >>> Thanks, will modify following. > >>> aspeed,xfer_mode: > >>> enum: [0, 1, 2] > >>> description: > >>> 0: byte mode, 1: buff_mode, 2: dma_mode > >> > >> Just keep it text - byte, buffered, dma > >> > >>> > >>>> 2. And why exactly this is board-specific? > >>> > >>> No, it not depends on board design. It is only for register control > >>> for > >> controller transfer behave. > >>> The controller support 3 different trigger mode for transfer. > >>> Assign bus#1 ~ 3 : dma tranfer and assign bus#4 ~ 6 : buffer mode > >>> transfer, That can reduce the dram usage. > >> > >> Then anyway it does not look like property for Devicetree. DT > >> describes hardware, not OS behavior. > > > > The same draw, in this case, i2c bus#1 that is multi-master transfer > architecture. > > Both will inactive with trunk data. That cane enable i2c#1 use DMA transfer > to reduce CPU utilized. > > Others (bus#2/3) can keep byte/buff mode. > > Isn't then current bus configuration for I2C#1 known to the driver? > Jeremy asked few other questions around here... No, The driver don't know currently board configuration. Best regards, Ryan
On 22/02/2023 11:31, Ryan Chen wrote: >> Board B >>> -------------------------------------------------------- >> -------------------------------------------------------- >>> | i2c bus#1(master/slave) <--------------------> fingerprint.(can be unplug) >> <--------------------> i2c bus#x (master/slave) | >>> | i2c bus#2(master) -> tmp i2c device | >> | | >>> | i2c bus#3(master) -> adc i2c device | | >> | >>> -------------------------------------------------------- >> -------------------------------------------------------- >>> In this case i2c bus#1 need enable timeout, avoid suddenly unplug the >> connector. That slave will keep state to drive clock stretching. >>> So it is specific enable in i2c bus#1. Others is not needed enable timeout. >>> Does this draw is more clear in scenario? >> >> I2C bus #1 works in slave mode? So you always need it for slave work? > > Yes, it is both slave/master mode. It is always dual role. Slave must always work. > Due to another board master will send. I meant that you need this property when it works in slave mode? It would be then redundant to have in DT as it is implied by the mode. > >>> >>>>> >>>>> So in those reason add this timeout design in controller. >>>> >>>> You need to justify why DT is correct place for this property. DT is >>>> not for configuring OS, but to describe hardware. I gave you one >>>> possibility >>>> - why different boards would like to set this property. You said it >>>> is not board specific, thus all boards will have it (or none of them). >>>> Without any other reason, this is not a DT property. Drop. >>>> >>>>>> >>>>>>> >>>>>>>>> + >>>>>>>>> + byte-mode: >>>>>>>>> + type: boolean >>>>>>>>> + description: Force i2c driver use byte mode transmit >>>>>>>> >>>>>>>> Drop, not a DT property. >>>>>>>> >>>>>>>>> + >>>>>>>>> + buff-mode: >>>>>>>>> + type: boolean >>>>>>>>> + description: Force i2c driver use buffer mode transmit >>>>>>>> >>>>>>>> Drop, not a DT property. >>>>>>>> >>>>>>> The controller support 3 different for transfer. >>>>>>> Byte mode: it means step by step to issue transfer. >>>>>>> Example i2c read, each step will issue interrupt then enable next step. >>>>>>> Sr (start read) | D | D | D | P >>>>>>> Buffer mode: it means, the data can prepare into buffer register, >>>>>>> then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. >>>>>>> The DMA mode most like with buffer mode, The differ is data >>>>>>> prepare in DRAM, than trigger transfer. >>>>>>> >>>>>>> So, should I modify to >>>>>>> aspeed,byte: >>>>>>> type: boolean >>>>>>> description: Enable i2c controller transfer with byte mode >>>>>>> >>>>>>> aspeed,buff: >>>>>>> type: boolean >>>>>>> description: Enable i2c controller transfer with buff mode >>>>>> >>>>>> 1. No, these are not bools but enum in such case. >>>>> >>>>> Thanks, will modify following. >>>>> aspeed,xfer_mode: >>>>> enum: [0, 1, 2] >>>>> description: >>>>> 0: byte mode, 1: buff_mode, 2: dma_mode >>>> >>>> Just keep it text - byte, buffered, dma >>>> >>>>> >>>>>> 2. And why exactly this is board-specific? >>>>> >>>>> No, it not depends on board design. It is only for register control >>>>> for >>>> controller transfer behave. >>>>> The controller support 3 different trigger mode for transfer. >>>>> Assign bus#1 ~ 3 : dma tranfer and assign bus#4 ~ 6 : buffer mode >>>>> transfer, That can reduce the dram usage. >>>> >>>> Then anyway it does not look like property for Devicetree. DT >>>> describes hardware, not OS behavior. >>> >>> The same draw, in this case, i2c bus#1 that is multi-master transfer >> architecture. >>> Both will inactive with trunk data. That cane enable i2c#1 use DMA transfer >> to reduce CPU utilized. >>> Others (bus#2/3) can keep byte/buff mode. >> >> Isn't then current bus configuration for I2C#1 known to the driver? >> Jeremy asked few other questions around here... > > No, The driver don't know currently board configuration. It knows whether it is working in multi-master/slave mode. Best regards, Krzysztof
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Wednesday, February 22, 2023 6:36 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > On 22/02/2023 11:31, Ryan Chen wrote: > >> Board B > >>> -------------------------------------------------------- > >> -------------------------------------------------------- > >>> | i2c bus#1(master/slave) <--------------------> > >>> | fingerprint.(can be unplug) > >> <--------------------> i2c bus#x (master/slave) | > >>> | i2c bus#2(master) -> tmp i2c device | > >> | | > >>> | i2c bus#3(master) -> adc i2c device | > | > >> | > >>> -------------------------------------------------------- > >> -------------------------------------------------------- > >>> In this case i2c bus#1 need enable timeout, avoid suddenly unplug > >>> the > >> connector. That slave will keep state to drive clock stretching. > >>> So it is specific enable in i2c bus#1. Others is not needed enable timeout. > >>> Does this draw is more clear in scenario? > >> > >> I2C bus #1 works in slave mode? So you always need it for slave work? > > > > Yes, it is both slave/master mode. It is always dual role. Slave must always > work. > > Due to another board master will send. > > I meant that you need this property when it works in slave mode? It would be > then redundant to have in DT as it is implied by the mode. But timeout feature is also apply in master. It for avoid suddenly slave miss(un-plug) Master can timeout and release the SDA/SCL, return. > > > > >>> > >>>>> > >>>>> So in those reason add this timeout design in controller. > >>>> > >>>> You need to justify why DT is correct place for this property. DT > >>>> is not for configuring OS, but to describe hardware. I gave you one > >>>> possibility > >>>> - why different boards would like to set this property. You said it > >>>> is not board specific, thus all boards will have it (or none of them). > >>>> Without any other reason, this is not a DT property. Drop. > >>>> > >>>>>> > >>>>>>> > >>>>>>>>> + > >>>>>>>>> + byte-mode: > >>>>>>>>> + type: boolean > >>>>>>>>> + description: Force i2c driver use byte mode transmit > >>>>>>>> > >>>>>>>> Drop, not a DT property. > >>>>>>>> > >>>>>>>>> + > >>>>>>>>> + buff-mode: > >>>>>>>>> + type: boolean > >>>>>>>>> + description: Force i2c driver use buffer mode transmit > >>>>>>>> > >>>>>>>> Drop, not a DT property. > >>>>>>>> > >>>>>>> The controller support 3 different for transfer. > >>>>>>> Byte mode: it means step by step to issue transfer. > >>>>>>> Example i2c read, each step will issue interrupt then enable next > step. > >>>>>>> Sr (start read) | D | D | D | P > >>>>>>> Buffer mode: it means, the data can prepare into buffer > >>>>>>> register, then Trigger transfer. So Sr D D D P, only have only 1 interrupt > handling. > >>>>>>> The DMA mode most like with buffer mode, The differ is data > >>>>>>> prepare in DRAM, than trigger transfer. > >>>>>>> > >>>>>>> So, should I modify to > >>>>>>> aspeed,byte: > >>>>>>> type: boolean > >>>>>>> description: Enable i2c controller transfer with byte mode > >>>>>>> > >>>>>>> aspeed,buff: > >>>>>>> type: boolean > >>>>>>> description: Enable i2c controller transfer with buff mode > >>>>>> > >>>>>> 1. No, these are not bools but enum in such case. > >>>>> > >>>>> Thanks, will modify following. > >>>>> aspeed,xfer_mode: > >>>>> enum: [0, 1, 2] > >>>>> description: > >>>>> 0: byte mode, 1: buff_mode, 2: dma_mode > >>>> > >>>> Just keep it text - byte, buffered, dma > >>>> > >>>>> > >>>>>> 2. And why exactly this is board-specific? > >>>>> > >>>>> No, it not depends on board design. It is only for register > >>>>> control for > >>>> controller transfer behave. > >>>>> The controller support 3 different trigger mode for transfer. > >>>>> Assign bus#1 ~ 3 : dma tranfer and assign bus#4 ~ 6 : buffer mode > >>>>> transfer, That can reduce the dram usage. > >>>> > >>>> Then anyway it does not look like property for Devicetree. DT > >>>> describes hardware, not OS behavior. > >>> > >>> The same draw, in this case, i2c bus#1 that is multi-master transfer > >> architecture. > >>> Both will inactive with trunk data. That cane enable i2c#1 use DMA > >>> transfer > >> to reduce CPU utilized. > >>> Others (bus#2/3) can keep byte/buff mode. > >> > >> Isn't then current bus configuration for I2C#1 known to the driver? > >> Jeremy asked few other questions around here... > > > > No, The driver don't know currently board configuration. > > It knows whether it is working in multi-master/slave mode. But in DT can decide which i2c bus number can use dma or buffer mode transfer. If in another i2c bus support master only, also can use dma to transfer trunk data to another slave. Best regards, Ryan Chen
On 22/02/2023 11:47, Ryan Chen wrote: >>>> connector. That slave will keep state to drive clock stretching. >>>>> So it is specific enable in i2c bus#1. Others is not needed enable timeout. >>>>> Does this draw is more clear in scenario? >>>> >>>> I2C bus #1 works in slave mode? So you always need it for slave work? >>> >>> Yes, it is both slave/master mode. It is always dual role. Slave must always >> work. >>> Due to another board master will send. >> >> I meant that you need this property when it works in slave mode? It would be >> then redundant to have in DT as it is implied by the mode. > > But timeout feature is also apply in master. It for avoid suddenly slave miss(un-plug) > Master can timeout and release the SDA/SCL, return. OK, yet the property should describe the hardware, not the register feature you want to program. You need to properly model it in DT binding to represent hardware setup, not your desired Linux driver behavior. >>>>> The same draw, in this case, i2c bus#1 that is multi-master transfer >>>> architecture. >>>>> Both will inactive with trunk data. That cane enable i2c#1 use DMA >>>>> transfer >>>> to reduce CPU utilized. >>>>> Others (bus#2/3) can keep byte/buff mode. >>>> >>>> Isn't then current bus configuration for I2C#1 known to the driver? >>>> Jeremy asked few other questions around here... >>> >>> No, The driver don't know currently board configuration. >> >> It knows whether it is working in multi-master/slave mode. > > But in DT can decide which i2c bus number can use dma or buffer mode transfer. > If in another i2c bus support master only, also can use dma to transfer trunk data to another slave. and none of these were explained in commit msg or device description. Best regards, Krzysztof
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, February 23, 2023 5:29 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > On 22/02/2023 11:47, Ryan Chen wrote: > >>>> connector. That slave will keep state to drive clock stretching. > >>>>> So it is specific enable in i2c bus#1. Others is not needed enable > timeout. > >>>>> Does this draw is more clear in scenario? > >>>> > >>>> I2C bus #1 works in slave mode? So you always need it for slave work? > >>> > >>> Yes, it is both slave/master mode. It is always dual role. Slave > >>> must always > >> work. > >>> Due to another board master will send. > >> > >> I meant that you need this property when it works in slave mode? It > >> would be then redundant to have in DT as it is implied by the mode. > > > > But timeout feature is also apply in master. It for avoid suddenly > > slave miss(un-plug) Master can timeout and release the SDA/SCL, return. > > OK, yet the property should describe the hardware, not the register feature you > want to program. You need to properly model it in DT binding to represent > hardware setup, not your desired Linux driver behavior. > > >>>>> The same draw, in this case, i2c bus#1 that is multi-master > >>>>> transfer > >>>> architecture. > >>>>> Both will inactive with trunk data. That cane enable i2c#1 use DMA > >>>>> transfer > >>>> to reduce CPU utilized. > >>>>> Others (bus#2/3) can keep byte/buff mode. > >>>> > >>>> Isn't then current bus configuration for I2C#1 known to the driver? > >>>> Jeremy asked few other questions around here... > >>> > >>> No, The driver don't know currently board configuration. > >> > >> It knows whether it is working in multi-master/slave mode. > > > > But in DT can decide which i2c bus number can use dma or buffer mode > transfer. > > If in another i2c bus support master only, also can use dma to transfer trunk > data to another slave. > > and none of these were explained in commit msg or device description. > Thanks your guidance. I will add all those discussion in next patches cover-letter. Best regards, Ryan Chen.
diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml new file mode 100644 index 000000000000..913fb45d5fbe --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml @@ -0,0 +1,83 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/aspeed,i2cv2.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ASPEED I2Cv2 Controller on the AST26XX SoCs + +maintainers: + - Ryan Chen <ryan_chen@aspeedtech.com> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + enum: + - aspeed,ast2600-i2cv2 + + reg: + minItems: 1 + items: + - description: address offset and range of register + - description: address offset and range of buffer register + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + description: + Reference clock for the I2C bus + + resets: + maxItems: 1 + + clock-frequency: + description: + Desired I2C bus clock frequency in Hz. default 100khz. + + multi-master: + type: boolean + description: + states that there is another master active on this bus + + timeout: + type: boolean + description: Enable i2c bus timeout for master/slave (35ms) + + byte-mode: + type: boolean + description: Force i2c driver use byte mode transmit + + buff-mode: + type: boolean + description: Force i2c driver use buffer mode transmit + + aspeed,gr: + $ref: /schemas/types.yaml#/definitions/phandle + description: The phandle of i2c global register node. + +required: + - compatible + - reg + - interrupts + - clocks + - resets + - aspeed,gr + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/ast2600-clock.h> + i2c: i2c-bus@80 { + compatible = "aspeed,ast2600-i2cv2"; + reg = <0x80 0x80>, <0xc00 0x20>; + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>; + aspeed,gr = <&i2c_gr>; + clocks = <&syscon ASPEED_CLK_APB2>; + resets = <&syscon ASPEED_RESET_I2C>; + };
AST2600 support new register set for I2Cv2 controller, add bindings document to support driver of i2cv2 new register mode controller. Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> --- .../devicetree/bindings/i2c/aspeed,i2cv2.yaml | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml