| Message ID | 20230201103359.1742140-3-ryan_chen@aspeedtech.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add ASPEED AST2600 I2C new controller driver | expand |
On 01/02/2023 11:33, Ryan Chen wrote: > AST2600 support new register set for I2C controller, add bindings document > to support driver of i2c new register mode controller. Subject: drop "driver". You are not adding here driver. Use subject prefixes matching the subsystem (which you can get for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching). > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > --- > .../bindings/i2c/aspeed,i2c-ast2600.yaml | 78 +++++++++++++++++++ > 1 file changed, 78 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > new file mode 100644 > index 000000000000..b7d7bc303e77 > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml Filename based on compatible. > @@ -0,0 +1,78 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree Bindings Drop "Device Tree Bindings" > + > +maintainers: > + - Ryan Chen <ryan_chen@aspeedtech.com> > + > +allOf: > + - $ref: /schemas/i2c/i2c-controller.yaml# > + > +properties: > + compatible: > + enum: > + - aspeed,ast2600-i2c > + > + reg: > + minItems: 1 Why second item is optional? > + items: > + - description: address offset and range of bus > + - description: address offset and range of bus buffer > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + description: > + root clock of bus, should reference the APB > + clock in the second cell Either this is root clock or APB clock. Decide and describe the clock (hardware), not the DT syntax (drop "cell"). > + > + resets: > + maxItems: 1 > + > + bus-frequency: > + minimum: 500 > + maximum: 2000000 > + default: 100000 > + description: frequency of the bus clock in Hz defaults to 100 kHz when not > + specified Don't repeat constraints in free form text. > + > + multi-master: > + type: boolean > + description: > + states that there is another master active on this bus > + > +required: > + - reg > + - compatible > + - clocks > + - resets > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/ast2600-clock.h> > + > + i2c_gr: i2c-global-regs@0 { > + compatible = "aspeed,ast2600-i2c-global", "syscon"; > + reg = <0x0 0x20>; > + resets = <&syscon ASPEED_RESET_I2C>; Drop. not related. > + }; > + > + i2c0: i2c-bus@80 { Drop label. Node name: i2c I guess this wasn't tested, right? > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <1>; > + compatible = "aspeed,ast2600-i2c"; > + reg = <0x80 0x80>, <0xC00 0x20>; Compatible is first, reg is second. Use lowercase hex. > + clocks = <&syscon ASPEED_CLK_APB2>; > + resets = <&syscon ASPEED_RESET_I2C>; > + bus-frequency = <100000>; > + }; Best regards, Krzysztof
On 01/02/2023 11:33, Ryan Chen wrote: > AST2600 support new register set for I2C controller, add bindings document > to support driver of i2c new register mode controller. > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > --- > .../bindings/i2c/aspeed,i2c-ast2600.yaml | 78 +++++++++++++++++++ > 1 file changed, 78 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > new file mode 100644 > index 000000000000..b7d7bc303e77 > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > @@ -0,0 +1,78 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree Bindings > + > +maintainers: > + - Ryan Chen <ryan_chen@aspeedtech.com> > + > +allOf: > + - $ref: /schemas/i2c/i2c-controller.yaml# > + > +properties: > + compatible: > + enum: > + - aspeed,ast2600-i2c NAK. It's already there. Please do not waste our time in submitting duplicated drivers. Best regards, Krzysztof
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, February 2, 2023 4:58 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 > i2C driver > > On 01/02/2023 11:33, Ryan Chen wrote: > > AST2600 support new register set for I2C controller, add bindings > > document to support driver of i2c new register mode controller. > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > --- > > .../bindings/i2c/aspeed,i2c-ast2600.yaml | 78 > +++++++++++++++++++ > > 1 file changed, 78 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > > b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > > new file mode 100644 > > index 000000000000..b7d7bc303e77 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml > > @@ -0,0 +1,78 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree > > +Bindings > > + > > +maintainers: > > + - Ryan Chen <ryan_chen@aspeedtech.com> > > + > > +allOf: > > + - $ref: /schemas/i2c/i2c-controller.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - aspeed,ast2600-i2c > > NAK. It's already there. Please do not waste our time in submitting duplicated > drivers. It is not duplicated, as my description in cover " This series add AST2600 i2c new register set driver" So, this will be different driver compatible. The original compatible is - aspeed,ast2400-i2c-bus - aspeed,ast2500-i2c-bus - aspeed,ast2600-i2c-bus So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus". Best regards, Ryan Chen
On 15/02/2023 06:43, Ryan Chen wrote: >>> + - $ref: /schemas/i2c/i2c-controller.yaml# >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - aspeed,ast2600-i2c >> >> NAK. It's already there. Please do not waste our time in submitting duplicated >> drivers. > > It is not duplicated, as my description in cover " This series add AST2600 i2c new register set driver" > So, this will be different driver compatible. > The original compatible is > - aspeed,ast2400-i2c-bus > - aspeed,ast2500-i2c-bus > - aspeed,ast2600-i2c-bus > So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus". Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a differentiating name. Best regards, Krzysztof
Hi Ryan, On 2/14/2023 9:43 PM, Ryan Chen wrote: > It is not duplicated, as my description in cover " This series add AST2600 i2c new register set driver" > So, this will be different driver compatible. > The original compatible is > - aspeed,ast2400-i2c-bus > - aspeed,ast2500-i2c-bus > - aspeed,ast2600-i2c-bus > So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus". Is it possible to keep existing driver drivers/i2c/busses/i2c-aspeed.c for ast2400/ast2500, while move ast2600 support to new driver altogether, say i2c-ast2600.c along with new register mode? By default new driver can support legacy mode with same compatible "aspeed,ast2600- i2c-bus", additionally driven by dt props, switch to new mode. Regards, Dhananjay
On 16/02/2023 05:17, Dhananjay Phadke wrote: > Hi Ryan, > > On 2/14/2023 9:43 PM, Ryan Chen wrote: >> It is not duplicated, as my description in cover " This series add AST2600 i2c new register set driver" >> So, this will be different driver compatible. >> The original compatible is >> - aspeed,ast2400-i2c-bus >> - aspeed,ast2500-i2c-bus >> - aspeed,ast2600-i2c-bus >> So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus". > > Is it possible to keep existing driver drivers/i2c/busses/i2c-aspeed.c > for ast2400/ast2500, while move ast2600 support to new driver > altogether, say i2c-ast2600.c along with new register mode? By default > new driver can support legacy mode with same compatible "aspeed,ast2600- > i2c-bus", additionally driven by dt props, switch to new mode. So this is for the same hardware? Then the bindings are duplicated. Driver is actually duplicated as well - rework the old one, instead of adding this. Best regards, Krzysztof
Hello Dhananjay, > -----Original Message----- > From: Dhananjay Phadke <dphadke@linux.microsoft.com> > Sent: Thursday, February 16, 2023 12:17 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org>; 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 > i2C driver > > Hi Ryan, > > On 2/14/2023 9:43 PM, Ryan Chen wrote: > > It is not duplicated, as my description in cover " This series add AST2600 i2c > new register set driver" > > So, this will be different driver compatible. > > The original compatible is > > - aspeed,ast2400-i2c-bus > > - aspeed,ast2500-i2c-bus > > - aspeed,ast2600-i2c-bus > > So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus". > > Is it possible to keep existing driver drivers/i2c/busses/i2c-aspeed.c for > ast2400/ast2500, while move ast2600 support to new driver altogether, say > i2c-ast2600.c along with new register mode? By default new driver can > support legacy mode with same compatible "aspeed,ast2600- i2c-bus", > additionally driven by dt props, switch to new mode. No, as the patch cover latter description, it is two different register offsets. Pure new register control, and also different control sequence. Regards, Ryan
Hello Krzysztof > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, February 16, 2023 4:18 AM > 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 > i2C driver > > On 15/02/2023 06:43, Ryan Chen wrote: > >>> + - $ref: /schemas/i2c/i2c-controller.yaml# > >>> + > >>> +properties: > >>> + compatible: > >>> + enum: > >>> + - aspeed,ast2600-i2c > >> > >> NAK. It's already there. Please do not waste our time in submitting > >> duplicated drivers. > > > > It is not duplicated, as my description in cover " This series add AST2600 i2c > new register set driver" > > So, this will be different driver compatible. > > The original compatible is > > - aspeed,ast2400-i2c-bus > > - aspeed,ast2500-i2c-bus > > - aspeed,ast2600-i2c-bus > > So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus". > > Bindings are documenting hardware, so I claim - we already have this > hardware described and this is duplicated. Otherwise - what are these two I2C > controllers and what are the differences? Why they do not have really different > name? Bus looks more like a mistake than a differentiating name. For misunderstanding, or mistaken. I purpose to be aspeed,ast2600-i2cv2, will it more clear way ? Best regards, Ryan Chen
On 16/02/2023 10:20, Ryan Chen wrote: > Hello Krzysztof > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Thursday, February 16, 2023 4:18 AM >> 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 >> i2C driver >> >> On 15/02/2023 06:43, Ryan Chen wrote: >>>>> + - $ref: /schemas/i2c/i2c-controller.yaml# >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + enum: >>>>> + - aspeed,ast2600-i2c >>>> >>>> NAK. It's already there. Please do not waste our time in submitting >>>> duplicated drivers. >>> >>> It is not duplicated, as my description in cover " This series add AST2600 i2c >> new register set driver" >>> So, this will be different driver compatible. >>> The original compatible is >>> - aspeed,ast2400-i2c-bus >>> - aspeed,ast2500-i2c-bus >>> - aspeed,ast2600-i2c-bus >>> So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus". >> >> Bindings are documenting hardware, so I claim - we already have this >> hardware described and this is duplicated. Otherwise - what are these two I2C >> controllers and what are the differences? Why they do not have really different >> name? Bus looks more like a mistake than a differentiating name. > For misunderstanding, or mistaken. > I purpose to be aspeed,ast2600-i2cv2, will it more clear way ? I don't know. I still did not get answers. I asked here several questions. Best regards, Krzysztof
Hello Krzysztof > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, February 16, 2023 5:22 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 > i2C driver > > On 16/02/2023 10:20, Ryan Chen wrote: > > Hello Krzysztof > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Thursday, February 16, 2023 4:18 AM > >> 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for > >> AST2600 i2C driver > >> > >> On 15/02/2023 06:43, Ryan Chen wrote: > >>>>> + - $ref: /schemas/i2c/i2c-controller.yaml# > >>>>> + > >>>>> +properties: > >>>>> + compatible: > >>>>> + enum: > >>>>> + - aspeed,ast2600-i2c > >>>> > >>>> NAK. It's already there. Please do not waste our time in submitting > >>>> duplicated drivers. > >>> > >>> It is not duplicated, as my description in cover " This series add > >>> AST2600 i2c > >> new register set driver" > >>> So, this will be different driver compatible. > >>> The original compatible is > >>> - aspeed,ast2400-i2c-bus > >>> - aspeed,ast2500-i2c-bus > >>> - aspeed,ast2600-i2c-bus > >>> So the new register set compatible is "- aspeed,ast2600-i2c", remove > "bus". > >> > >> Bindings are documenting hardware, so I claim - we already have this > >> hardware described and this is duplicated. Otherwise - what are these > >> two I2C controllers and what are the differences? Why they do not > >> have really different name? Bus looks more like a mistake than a > differentiating name. > > For misunderstanding, or mistaken. > > I purpose to be aspeed,ast2600-i2cv2, will it more clear way ? > > I don't know. I still did not get answers. I asked here several questions. Those are different i2c controller, as I description in cover letter. The i2c new register mode, there have two separate slave/master register. And different register with old register. So now, avoid misunderstanding, or mistaken. I purpose to be aspeed,ast2600-i2cv2. Best regards, Ryan Chen
On 16/02/2023 10:26, Ryan Chen wrote: > Hello Krzysztof > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Thursday, February 16, 2023 5:22 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 >> i2C driver >> >> On 16/02/2023 10:20, Ryan Chen wrote: >>> Hello Krzysztof >>> >>>> -----Original Message----- >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> Sent: Thursday, February 16, 2023 4:18 AM >>>> 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for >>>> AST2600 i2C driver >>>> >>>> On 15/02/2023 06:43, Ryan Chen wrote: >>>>>>> + - $ref: /schemas/i2c/i2c-controller.yaml# >>>>>>> + >>>>>>> +properties: >>>>>>> + compatible: >>>>>>> + enum: >>>>>>> + - aspeed,ast2600-i2c >>>>>> >>>>>> NAK. It's already there. Please do not waste our time in submitting >>>>>> duplicated drivers. >>>>> >>>>> It is not duplicated, as my description in cover " This series add >>>>> AST2600 i2c >>>> new register set driver" >>>>> So, this will be different driver compatible. >>>>> The original compatible is >>>>> - aspeed,ast2400-i2c-bus >>>>> - aspeed,ast2500-i2c-bus >>>>> - aspeed,ast2600-i2c-bus >>>>> So the new register set compatible is "- aspeed,ast2600-i2c", remove >> "bus". >>>> >>>> Bindings are documenting hardware, so I claim - we already have this >>>> hardware described and this is duplicated. Otherwise - what are these >>>> two I2C controllers and what are the differences? Why they do not >>>> have really different name? Bus looks more like a mistake than a >> differentiating name. >>> For misunderstanding, or mistaken. >>> I purpose to be aspeed,ast2600-i2cv2, will it more clear way ? >> >> I don't know. I still did not get answers. I asked here several questions. > Those are different i2c controller, as I description in cover letter. The cover letter does not explain here anything. It barely mentions "new register set" and "separate register set". This is really short, so without proper explanations you will get all these questions. Are they compatible? Do they overlap? Are they completely different? If so, why datasheet uses same name for them? So many questions but cover letter is basically two sentences and here: > The i2c new register mode, there have two separate slave/master register. > And different register with old register. you repeat the same. > So now, avoid misunderstanding, or mistaken. > I purpose to be aspeed,ast2600-i2cv2. Best regards, Krzysztof
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, February 17, 2023 4:37 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 > i2C driver > > On 16/02/2023 10:26, Ryan Chen wrote: > > Hello Krzysztof > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Thursday, February 16, 2023 5:22 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for > >> AST2600 i2C driver > >> > >> On 16/02/2023 10:20, Ryan Chen wrote: > >>> Hello Krzysztof > >>> > >>>> -----Original Message----- > >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>>> Sent: Thursday, February 16, 2023 4:18 AM > >>>> 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 v4 2/3] dt-bindings: i2c-ast2600: Add support > >>>> for > >>>> AST2600 i2C driver > >>>> > >>>> On 15/02/2023 06:43, Ryan Chen wrote: > >>>>>>> + - $ref: /schemas/i2c/i2c-controller.yaml# > >>>>>>> + > >>>>>>> +properties: > >>>>>>> + compatible: > >>>>>>> + enum: > >>>>>>> + - aspeed,ast2600-i2c > >>>>>> > >>>>>> NAK. It's already there. Please do not waste our time in > >>>>>> submitting duplicated drivers. > >>>>> > >>>>> It is not duplicated, as my description in cover " This series add > >>>>> AST2600 i2c > >>>> new register set driver" > >>>>> So, this will be different driver compatible. > >>>>> The original compatible is > >>>>> - aspeed,ast2400-i2c-bus > >>>>> - aspeed,ast2500-i2c-bus > >>>>> - aspeed,ast2600-i2c-bus > >>>>> So the new register set compatible is "- aspeed,ast2600-i2c", > >>>>> remove > >> "bus". > >>>> > >>>> Bindings are documenting hardware, so I claim - we already have > >>>> this hardware described and this is duplicated. Otherwise - what > >>>> are these two I2C controllers and what are the differences? Why > >>>> they do not have really different name? Bus looks more like a > >>>> mistake than a > >> differentiating name. > >>> For misunderstanding, or mistaken. > >>> I purpose to be aspeed,ast2600-i2cv2, will it more clear way ? > >> > >> I don't know. I still did not get answers. I asked here several questions. > > Those are different i2c controller, as I description in cover letter. > > The cover letter does not explain here anything. It barely mentions "new > register set" and "separate register set". This is really short, so without proper > explanations you will get all these questions. Are they compatible? Do they > overlap? Are they completely different? If so, why datasheet uses same name > for them? So many questions but cover letter is basically two sentences and > here: Sorry my misunderstanding. The legacy register layout is mix master/slave register control together. So will let confuse about register. The following is add more detail description about new register layout. And new feature set add for register. -Add new clock divider option for more flexible and accurate clock rate generation -Add tCKHighMin timing to guarantee SCL high pulse width. -Add support dual pool buffer mode, split 32 bytes pool buffer of each device into 2 x 16 bytes for Tx and Rx individually. -Increase DMA buffer size to 4096 bytes and support byte alignment. -Re-define the base address of BUS1 ~ BUS16 and Pool buffer. -Re-define registers for separating master and slave mode control. -Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx. And following is new register set for package transfer sequence. New Master operation mode: S -> Aw -> P {S: Start, Sr: Repeat Start, Aw/r: Address for write/read, P: Stop}. New Master operation mode: S -> Aw -> TxD -> P New Master operation mode: S -> Ar -> RxD -> P New Slave operation mode: S -> Aw -> RxD -> Sr -> Ar -> TxD -> P. -Bus SDA lock auto-release capability for new master DMA command mode. -Bus auto timeout for new master/slave DMA mode. The following is two versus register layout. Old: {I2CD00}: Function Control Register {I2CD04}: Clock and AC Timing Control Register \#1 {I2CD08}: Clock and AC Timing Control Register \#2 {I2CD0C}: Interrupt Control Register {I2CD10}: Interrupt Status Register {I2CD14}: Command/Status Register {I2CD18}: Slave Device Address Register {I2CD1C}: Pool Buffer Control Register {I2CD20}: Transmit/Receive Byte Buffer Register {I2CD24}: DMA Mode Buffer Address Register {I2CD28}: DMA Transfer Length Register {I2CD2C}: Original DMA Mode Buffer Address Setting {I2CD30}: Original DMA Transfer Length Setting and Final Status New Register mode {I2CC00}: Master/Slave Function Control Register {I2CC04}: Master/Slave Clock and AC Timing Control Register {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register {I2CC0C}: Master/Slave Pool Buffer Control Register {I2CM10}: Master Interrupt Control Register {I2CM14}: Master Interrupt Status Register {I2CM18}: Master Command/Status Register {I2CM1C}: Master DMA Buffer Length Register {I2CS20}: Slave~ Interrupt Control Register {I2CS24}: Slave~ Interrupt Status Register {I2CS28}: Slave~ Command/Status Register {I2CS2C}: Slave~ DMA Buffer Length Register {I2CM30}: Master DMA Mode Tx Buffer Base Address {I2CM34}: Master DMA Mode Rx Buffer Base Address {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address {I2CS40}: Slave Device Address Register {I2CM48}: Master DMA Length Status Register {I2CS4C}: Slave DMA Length Status Register {I2CC50}: Current DMA Operating Address Status {I2CC54}: Current DMA Operating Length Status Sorry Krzysztof, I will add those in cover letter, it will more clear picture for review. Thanks a lot. > > The i2c new register mode, there have two separate slave/master register. > > And different register with old register. > > you repeat the same. > > > So now, avoid misunderstanding, or mistaken. > > I purpose to be aspeed,ast2600-i2cv2. > > > Best regards, > Krzysztof
On 18/02/2023 02:19, Ryan Chen wrote: > Hello Krzysztof, > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Friday, February 17, 2023 4:37 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 >> i2C driver >> >> On 16/02/2023 10:26, Ryan Chen wrote: >>> Hello Krzysztof >>> >>>> -----Original Message----- >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> Sent: Thursday, February 16, 2023 5:22 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for >>>> AST2600 i2C driver >>>> >>>> On 16/02/2023 10:20, Ryan Chen wrote: >>>>> Hello Krzysztof >>>>> >>>>>> -----Original Message----- >>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>> Sent: Thursday, February 16, 2023 4:18 AM >>>>>> 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 v4 2/3] dt-bindings: i2c-ast2600: Add support >>>>>> for >>>>>> AST2600 i2C driver >>>>>> >>>>>> On 15/02/2023 06:43, Ryan Chen wrote: >>>>>>>>> + - $ref: /schemas/i2c/i2c-controller.yaml# >>>>>>>>> + >>>>>>>>> +properties: >>>>>>>>> + compatible: >>>>>>>>> + enum: >>>>>>>>> + - aspeed,ast2600-i2c >>>>>>>> >>>>>>>> NAK. It's already there. Please do not waste our time in >>>>>>>> submitting duplicated drivers. >>>>>>> >>>>>>> It is not duplicated, as my description in cover " This series add >>>>>>> AST2600 i2c >>>>>> new register set driver" >>>>>>> So, this will be different driver compatible. >>>>>>> The original compatible is >>>>>>> - aspeed,ast2400-i2c-bus >>>>>>> - aspeed,ast2500-i2c-bus >>>>>>> - aspeed,ast2600-i2c-bus >>>>>>> So the new register set compatible is "- aspeed,ast2600-i2c", >>>>>>> remove >>>> "bus". >>>>>> >>>>>> Bindings are documenting hardware, so I claim - we already have >>>>>> this hardware described and this is duplicated. Otherwise - what >>>>>> are these two I2C controllers and what are the differences? Why >>>>>> they do not have really different name? Bus looks more like a >>>>>> mistake than a >>>> differentiating name. >>>>> For misunderstanding, or mistaken. >>>>> I purpose to be aspeed,ast2600-i2cv2, will it more clear way ? >>>> >>>> I don't know. I still did not get answers. I asked here several questions. >>> Those are different i2c controller, as I description in cover letter. >> >> The cover letter does not explain here anything. It barely mentions "new >> register set" and "separate register set". This is really short, so without proper >> explanations you will get all these questions. Are they compatible? Do they >> overlap? Are they completely different? If so, why datasheet uses same name >> for them? So many questions but cover letter is basically two sentences and >> here: > > Sorry my misunderstanding. > The legacy register layout is mix master/slave register control together. > So will let confuse about register. > The following is add more detail description about new register layout. > And new feature set add for register. > > -Add new clock divider option for more flexible and accurate clock rate generation > -Add tCKHighMin timing to guarantee SCL high pulse width. > -Add support dual pool buffer mode, split 32 bytes pool buffer of each device into 2 x 16 bytes for Tx and Rx individually. > -Increase DMA buffer size to 4096 bytes and support byte alignment. > -Re-define the base address of BUS1 ~ BUS16 and Pool buffer. > -Re-define registers for separating master and slave mode control. > -Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx. > > And following is new register set for package transfer sequence. > New Master operation mode: S -> Aw -> P {S: Start, Sr: Repeat Start, Aw/r: Address for write/read, P: Stop}. > New Master operation mode: S -> Aw -> TxD -> P > New Master operation mode: S -> Ar -> RxD -> P > New Slave operation mode: S -> Aw -> RxD -> Sr -> Ar -> TxD -> P. > -Bus SDA lock auto-release capability for new master DMA command mode. > -Bus auto timeout for new master/slave DMA mode. > > The following is two versus register layout. > Old: > {I2CD00}: Function Control Register > {I2CD04}: Clock and AC Timing Control Register \#1 > {I2CD08}: Clock and AC Timing Control Register \#2 > {I2CD0C}: Interrupt Control Register > {I2CD10}: Interrupt Status Register > {I2CD14}: Command/Status Register > {I2CD18}: Slave Device Address Register > {I2CD1C}: Pool Buffer Control Register > {I2CD20}: Transmit/Receive Byte Buffer Register > {I2CD24}: DMA Mode Buffer Address Register > {I2CD28}: DMA Transfer Length Register > {I2CD2C}: Original DMA Mode Buffer Address Setting > {I2CD30}: Original DMA Transfer Length Setting and Final Status > > New Register mode > {I2CC00}: Master/Slave Function Control Register > {I2CC04}: Master/Slave Clock and AC Timing Control Register > {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register > {I2CC0C}: Master/Slave Pool Buffer Control Register > {I2CM10}: Master Interrupt Control Register > {I2CM14}: Master Interrupt Status Register > {I2CM18}: Master Command/Status Register > {I2CM1C}: Master DMA Buffer Length Register > {I2CS20}: Slave~ Interrupt Control Register > {I2CS24}: Slave~ Interrupt Status Register > {I2CS28}: Slave~ Command/Status Register > {I2CS2C}: Slave~ DMA Buffer Length Register > {I2CM30}: Master DMA Mode Tx Buffer Base Address > {I2CM34}: Master DMA Mode Rx Buffer Base Address > {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address > {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address > {I2CS40}: Slave Device Address Register > {I2CM48}: Master DMA Length Status Register > {I2CS4C}: Slave DMA Length Status Register > {I2CC50}: Current DMA Operating Address Status > {I2CC54}: Current DMA Operating Length Status Thanks for explanation, yet still I don't get whether these are separate devices or not. So again, you got several questions and you should answer them, not only parts of them. Are they compatible? Do they overlap? Are they completely different? If so, why datasheet uses same name for them? I don't understand why we struggle so much to get basic info justifying this patchset. Best regards, Krzysztof
Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, February 20, 2023 3:59 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 > i2C driver > > On 18/02/2023 02:19, Ryan Chen wrote: > > Hello Krzysztof, > > > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Friday, February 17, 2023 4:37 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for > >> AST2600 i2C driver > >> > >> On 16/02/2023 10:26, Ryan Chen wrote: > >>> Hello Krzysztof > >>> > >>>> -----Original Message----- > >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>>> Sent: Thursday, February 16, 2023 5:22 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 v4 2/3] dt-bindings: i2c-ast2600: Add support > >>>> for > >>>> AST2600 i2C driver > >>>> > >>>> On 16/02/2023 10:20, Ryan Chen wrote: > >>>>> Hello Krzysztof > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>>>>> Sent: Thursday, February 16, 2023 4:18 AM > >>>>>> 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 v4 2/3] dt-bindings: i2c-ast2600: Add support > >>>>>> for > >>>>>> AST2600 i2C driver > >>>>>> > >>>>>> On 15/02/2023 06:43, Ryan Chen wrote: > >>>>>>>>> + - $ref: /schemas/i2c/i2c-controller.yaml# > >>>>>>>>> + > >>>>>>>>> +properties: > >>>>>>>>> + compatible: > >>>>>>>>> + enum: > >>>>>>>>> + - aspeed,ast2600-i2c > >>>>>>>> > >>>>>>>> NAK. It's already there. Please do not waste our time in > >>>>>>>> submitting duplicated drivers. > >>>>>>> > >>>>>>> It is not duplicated, as my description in cover " This series > >>>>>>> add > >>>>>>> AST2600 i2c > >>>>>> new register set driver" > >>>>>>> So, this will be different driver compatible. > >>>>>>> The original compatible is > >>>>>>> - aspeed,ast2400-i2c-bus > >>>>>>> - aspeed,ast2500-i2c-bus > >>>>>>> - aspeed,ast2600-i2c-bus > >>>>>>> So the new register set compatible is "- aspeed,ast2600-i2c", > >>>>>>> remove > >>>> "bus". > >>>>>> > >>>>>> Bindings are documenting hardware, so I claim - we already have > >>>>>> this hardware described and this is duplicated. Otherwise - what > >>>>>> are these two I2C controllers and what are the differences? Why > >>>>>> they do not have really different name? Bus looks more like a > >>>>>> mistake than a > >>>> differentiating name. > >>>>> For misunderstanding, or mistaken. > >>>>> I purpose to be aspeed,ast2600-i2cv2, will it more clear way ? > >>>> > >>>> I don't know. I still did not get answers. I asked here several questions. > >>> Those are different i2c controller, as I description in cover letter. > >> > >> The cover letter does not explain here anything. It barely mentions > >> "new register set" and "separate register set". This is really short, > >> so without proper explanations you will get all these questions. Are > >> they compatible? Do they overlap? Are they completely different? If > >> so, why datasheet uses same name for them? So many questions but > >> cover letter is basically two sentences and > >> here: > > > > Sorry my misunderstanding. > > The legacy register layout is mix master/slave register control together. > > So will let confuse about register. > > The following is add more detail description about new register layout. > > And new feature set add for register. > > > > -Add new clock divider option for more flexible and accurate clock > > rate generation -Add tCKHighMin timing to guarantee SCL high pulse width. > > -Add support dual pool buffer mode, split 32 bytes pool buffer of each device > into 2 x 16 bytes for Tx and Rx individually. > > -Increase DMA buffer size to 4096 bytes and support byte alignment. > > -Re-define the base address of BUS1 ~ BUS16 and Pool buffer. > > -Re-define registers for separating master and slave mode control. > > -Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx. > > > > And following is new register set for package transfer sequence. > > New Master operation mode: S -> Aw -> P {S: Start, Sr: Repeat Start, Aw/r: > Address for write/read, P: Stop}. > > New Master operation mode: S -> Aw -> TxD -> P New Master operation > > mode: S -> Ar -> RxD -> P New Slave operation mode: S -> Aw -> RxD -> > > Sr -> Ar -> TxD -> P. > > -Bus SDA lock auto-release capability for new master DMA command mode. > > -Bus auto timeout for new master/slave DMA mode. > > > > The following is two versus register layout. > > Old: > > {I2CD00}: Function Control Register > > {I2CD04}: Clock and AC Timing Control Register \#1 > > {I2CD08}: Clock and AC Timing Control Register \#2 > > {I2CD0C}: Interrupt Control Register > > {I2CD10}: Interrupt Status Register > > {I2CD14}: Command/Status Register > > {I2CD18}: Slave Device Address Register > > {I2CD1C}: Pool Buffer Control Register > > {I2CD20}: Transmit/Receive Byte Buffer Register > > {I2CD24}: DMA Mode Buffer Address Register > > {I2CD28}: DMA Transfer Length Register > > {I2CD2C}: Original DMA Mode Buffer Address Setting > > {I2CD30}: Original DMA Transfer Length Setting and Final Status > > > > New Register mode > > {I2CC00}: Master/Slave Function Control Register > > {I2CC04}: Master/Slave Clock and AC Timing Control Register > > {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register > > {I2CC0C}: Master/Slave Pool Buffer Control Register > > {I2CM10}: Master Interrupt Control Register > > {I2CM14}: Master Interrupt Status Register > > {I2CM18}: Master Command/Status Register > > {I2CM1C}: Master DMA Buffer Length Register > > {I2CS20}: Slave~ Interrupt Control Register > > {I2CS24}: Slave~ Interrupt Status Register > > {I2CS28}: Slave~ Command/Status Register > > {I2CS2C}: Slave~ DMA Buffer Length Register > > {I2CM30}: Master DMA Mode Tx Buffer Base Address > > {I2CM34}: Master DMA Mode Rx Buffer Base Address > > {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address > > {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address > > {I2CS40}: Slave Device Address Register > > {I2CM48}: Master DMA Length Status Register > > {I2CS4C}: Slave DMA Length Status Register > > {I2CC50}: Current DMA Operating Address Status > > {I2CC54}: Current DMA Operating Length Status > > Thanks for explanation, yet still I don't get whether these are separate devices > or not. So again, you got several questions and you should answer them, not > only parts of them. > > Are they compatible? Do they overlap? Are they completely different? If so, > why datasheet uses same name for them? They are not compatible. The register offset is overlap. Old register is from 0x00 ~ 0x30 New register is from 0x00 ~ 0x54 The new design has another register, call global register that do 1 bit to set switch register decode to be new or old register layout. For example, old register AC timing have two setting I2CD04/08 but new is I2CC04. And now register setting for AC timing. And also master/slave register separate control. Not mix at I2CD14. About naming in datasheet, due to you can see the new design concept is the same. So most use the same name to be offset register name. From mix to separate master/slave control. The following an example. IER 1 to 2 {I2CD0C}: Interrupt Control Register to 2 set {I2CM10}: Master Interrupt Control Register, {I2CS20}: Slave~ Interrupt Control Register ISR, 1 to 2 {I2CD0C}: Interrupt Control Register -> {I2CM14}: Master Interrupt Status Register, {I2CS24}: Slave~ Interrupt Status Register And so on... > > I don't understand why we struggle so much to get basic info justifying this > patchset. > > > Best regards, > Krzysztof
On 20/02/2023 10:14, Ryan Chen wrote: > Hello Krzysztof, > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Monday, February 20, 2023 3:59 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 >> i2C driver >> >> On 18/02/2023 02:19, Ryan Chen wrote: >>> Hello Krzysztof, >>> >>> >>>> -----Original Message----- >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> Sent: Friday, February 17, 2023 4:37 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 v4 2/3] dt-bindings: i2c-ast2600: Add support for >>>> AST2600 i2C driver >>>> >>>> On 16/02/2023 10:26, Ryan Chen wrote: >>>>> Hello Krzysztof >>>>> >>>>>> -----Original Message----- >>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>> Sent: Thursday, February 16, 2023 5:22 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 v4 2/3] dt-bindings: i2c-ast2600: Add support >>>>>> for >>>>>> AST2600 i2C driver >>>>>> >>>>>> On 16/02/2023 10:20, Ryan Chen wrote: >>>>>>> Hello Krzysztof >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>>>> Sent: Thursday, February 16, 2023 4:18 AM >>>>>>>> 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 v4 2/3] dt-bindings: i2c-ast2600: Add support >>>>>>>> for >>>>>>>> AST2600 i2C driver >>>>>>>> >>>>>>>> On 15/02/2023 06:43, Ryan Chen wrote: >>>>>>>>>>> + - $ref: /schemas/i2c/i2c-controller.yaml# >>>>>>>>>>> + >>>>>>>>>>> +properties: >>>>>>>>>>> + compatible: >>>>>>>>>>> + enum: >>>>>>>>>>> + - aspeed,ast2600-i2c >>>>>>>>>> >>>>>>>>>> NAK. It's already there. Please do not waste our time in >>>>>>>>>> submitting duplicated drivers. >>>>>>>>> >>>>>>>>> It is not duplicated, as my description in cover " This series >>>>>>>>> add >>>>>>>>> AST2600 i2c >>>>>>>> new register set driver" >>>>>>>>> So, this will be different driver compatible. >>>>>>>>> The original compatible is >>>>>>>>> - aspeed,ast2400-i2c-bus >>>>>>>>> - aspeed,ast2500-i2c-bus >>>>>>>>> - aspeed,ast2600-i2c-bus >>>>>>>>> So the new register set compatible is "- aspeed,ast2600-i2c", >>>>>>>>> remove >>>>>> "bus". >>>>>>>> >>>>>>>> Bindings are documenting hardware, so I claim - we already have >>>>>>>> this hardware described and this is duplicated. Otherwise - what >>>>>>>> are these two I2C controllers and what are the differences? Why >>>>>>>> they do not have really different name? Bus looks more like a >>>>>>>> mistake than a >>>>>> differentiating name. >>>>>>> For misunderstanding, or mistaken. >>>>>>> I purpose to be aspeed,ast2600-i2cv2, will it more clear way ? >>>>>> >>>>>> I don't know. I still did not get answers. I asked here several questions. >>>>> Those are different i2c controller, as I description in cover letter. >>>> >>>> The cover letter does not explain here anything. It barely mentions >>>> "new register set" and "separate register set". This is really short, >>>> so without proper explanations you will get all these questions. Are >>>> they compatible? Do they overlap? Are they completely different? If >>>> so, why datasheet uses same name for them? So many questions but >>>> cover letter is basically two sentences and >>>> here: >>> >>> Sorry my misunderstanding. >>> The legacy register layout is mix master/slave register control together. >>> So will let confuse about register. >>> The following is add more detail description about new register layout. >>> And new feature set add for register. >>> >>> -Add new clock divider option for more flexible and accurate clock >>> rate generation -Add tCKHighMin timing to guarantee SCL high pulse width. >>> -Add support dual pool buffer mode, split 32 bytes pool buffer of each device >> into 2 x 16 bytes for Tx and Rx individually. >>> -Increase DMA buffer size to 4096 bytes and support byte alignment. >>> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer. >>> -Re-define registers for separating master and slave mode control. >>> -Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx. >>> >>> And following is new register set for package transfer sequence. >>> New Master operation mode: S -> Aw -> P {S: Start, Sr: Repeat Start, Aw/r: >> Address for write/read, P: Stop}. >>> New Master operation mode: S -> Aw -> TxD -> P New Master operation >>> mode: S -> Ar -> RxD -> P New Slave operation mode: S -> Aw -> RxD -> >>> Sr -> Ar -> TxD -> P. >>> -Bus SDA lock auto-release capability for new master DMA command mode. >>> -Bus auto timeout for new master/slave DMA mode. >>> >>> The following is two versus register layout. >>> Old: >>> {I2CD00}: Function Control Register >>> {I2CD04}: Clock and AC Timing Control Register \#1 >>> {I2CD08}: Clock and AC Timing Control Register \#2 >>> {I2CD0C}: Interrupt Control Register >>> {I2CD10}: Interrupt Status Register >>> {I2CD14}: Command/Status Register >>> {I2CD18}: Slave Device Address Register >>> {I2CD1C}: Pool Buffer Control Register >>> {I2CD20}: Transmit/Receive Byte Buffer Register >>> {I2CD24}: DMA Mode Buffer Address Register >>> {I2CD28}: DMA Transfer Length Register >>> {I2CD2C}: Original DMA Mode Buffer Address Setting >>> {I2CD30}: Original DMA Transfer Length Setting and Final Status >>> >>> New Register mode >>> {I2CC00}: Master/Slave Function Control Register >>> {I2CC04}: Master/Slave Clock and AC Timing Control Register >>> {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register >>> {I2CC0C}: Master/Slave Pool Buffer Control Register >>> {I2CM10}: Master Interrupt Control Register >>> {I2CM14}: Master Interrupt Status Register >>> {I2CM18}: Master Command/Status Register >>> {I2CM1C}: Master DMA Buffer Length Register >>> {I2CS20}: Slave~ Interrupt Control Register >>> {I2CS24}: Slave~ Interrupt Status Register >>> {I2CS28}: Slave~ Command/Status Register >>> {I2CS2C}: Slave~ DMA Buffer Length Register >>> {I2CM30}: Master DMA Mode Tx Buffer Base Address >>> {I2CM34}: Master DMA Mode Rx Buffer Base Address >>> {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address >>> {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address >>> {I2CS40}: Slave Device Address Register >>> {I2CM48}: Master DMA Length Status Register >>> {I2CS4C}: Slave DMA Length Status Register >>> {I2CC50}: Current DMA Operating Address Status >>> {I2CC54}: Current DMA Operating Length Status >> >> Thanks for explanation, yet still I don't get whether these are separate devices >> or not. So again, you got several questions and you should answer them, not >> only parts of them. >> >> Are they compatible? Do they overlap? Are they completely different? If so, >> why datasheet uses same name for them? > > They are not compatible. The register offset is overlap. > Old register is from 0x00 ~ 0x30 > New register is from 0x00 ~ 0x54 > The new design has another register, call global register that do 1 bit to set switch > register decode to be new or old register layout. > For example, old register AC timing have two setting I2CD04/08 but new is I2CC04. > And now register setting for AC timing. > And also master/slave register separate control. Not mix at I2CD14. > About naming in datasheet, due to you can see the new design concept is the same. > So most use the same name to be offset register name. > From mix to separate master/slave control. > The following an example. > IER 1 to 2 > {I2CD0C}: Interrupt Control Register to 2 set {I2CM10}: Master Interrupt Control Register, {I2CS20}: Slave~ Interrupt Control Register > ISR, 1 to 2 > {I2CD0C}: Interrupt Control Register -> {I2CM14}: Master Interrupt Status Register, {I2CS24}: Slave~ Interrupt Status Register > And so on... OK, that helps to understand. If I understand correctly, still the same hardware, but you just switch from old to new register interface (or mode). Separate compatible: yes. Separate binding: I don't think. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file mode 100644 index 000000000000..b7d7bc303e77 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml @@ -0,0 +1,78 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree Bindings + +maintainers: + - Ryan Chen <ryan_chen@aspeedtech.com> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + enum: + - aspeed,ast2600-i2c + + reg: + minItems: 1 + items: + - description: address offset and range of bus + - description: address offset and range of bus buffer + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + description: + root clock of bus, should reference the APB + clock in the second cell + + resets: + maxItems: 1 + + bus-frequency: + minimum: 500 + maximum: 2000000 + default: 100000 + description: frequency of the bus clock in Hz defaults to 100 kHz when not + specified + + multi-master: + type: boolean + description: + states that there is another master active on this bus + +required: + - reg + - compatible + - clocks + - resets + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/ast2600-clock.h> + + i2c_gr: i2c-global-regs@0 { + compatible = "aspeed,ast2600-i2c-global", "syscon"; + reg = <0x0 0x20>; + resets = <&syscon ASPEED_RESET_I2C>; + }; + + i2c0: i2c-bus@80 { + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <1>; + compatible = "aspeed,ast2600-i2c"; + reg = <0x80 0x80>, <0xC00 0x20>; + clocks = <&syscon ASPEED_CLK_APB2>; + resets = <&syscon ASPEED_RESET_I2C>; + bus-frequency = <100000>; + };
AST2600 support new register set for I2C controller, add bindings document to support driver of i2c new register mode controller. Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> --- .../bindings/i2c/aspeed,i2c-ast2600.yaml | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml