linux-kernel - RE: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SEZPR06MB5269502D7CBCD5698B65FF9FF2A59@SEZPR06MB5269.apcprd06.prod.outlook.com>
Date:   Tue, 21 Feb 2023 10:42:21 +0000
From:   Ryan Chen <ryan_chen@...eedtech.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Joel Stanley <joel@....id.au>,
        Andrew Jeffery <andrew@...id.au>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2

Hello Krzysztof,


> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Tuesday, February 21, 2023 5:40 PM
> To: Ryan Chen <ryan_chen@...eedtech.com>; Rob Herring
> <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Joel Stanley <joel@....id.au>; Andrew
> Jeffery <andrew@...id.au>; Philipp Zabel <p.zabel@...gutronix.de>;
> openbmc@...ts.ozlabs.org; linux-arm-kernel@...ts.infradead.org;
> linux-aspeed@...ts.ozlabs.org; linux-kernel@...r.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@...aro.org>
> >> Sent: Monday, February 20, 2023 4:35 PM
> >> To: Ryan Chen <ryan_chen@...eedtech.com>; Rob Herring
> >> <robh+dt@...nel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@...aro.org>; Joel Stanley <joel@....id.au>;
> >> Andrew Jeffery <andrew@...id.au>; Philipp Zabel
> >> <p.zabel@...gutronix.de>; openbmc@...ts.ozlabs.org;
> >> linux-arm-kernel@...ts.infradead.org;
> >> linux-aspeed@...ts.ozlabs.org; linux-kernel@...r.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@...eedtech.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@...eedtech.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