Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

[Krzysztof Kozlowski]

On 20/02/2023 10:14, Ryan Chen wrote:
> Hello Krzysztof,
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>> Sent: Monday, February 20, 2023 3:59 PM
>> To: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>; Rob Herring
>> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Joel Stanley <joel@xxxxxxxxx>; Andrew
>> Jeffery <andrew@xxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>;
>> openbmc@xxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>> linux-aspeed@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxx>
>>>> Sent: Friday, February 17, 2023 4:37 PM
>>>> To: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>; Rob Herring
>>>> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
>>>> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Joel Stanley <joel@xxxxxxxxx>;
>>>> Andrew Jeffery <andrew@xxxxxxxx>; Philipp Zabel
>>>> <p.zabel@xxxxxxxxxxxxxx>; openbmc@xxxxxxxxxxxxxxxx;
>>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>>>> linux-aspeed@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>>>> 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@xxxxxxxxxx>
>>>>>> Sent: Thursday, February 16, 2023 5:22 PM
>>>>>> To: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>; Rob Herring
>>>>>> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
>>>>>> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Joel Stanley <joel@xxxxxxxxx>;
>>>>>> Andrew Jeffery <andrew@xxxxxxxx>; Philipp Zabel
>>>>>> <p.zabel@xxxxxxxxxxxxxx>; openbmc@xxxxxxxxxxxxxxxx;
>>>>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>>>>>> linux-aspeed@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>>>>>> 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@xxxxxxxxxx>
>>>>>>>> Sent: Thursday, February 16, 2023 4:18 AM
>>>>>>>> To: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>; Rob Herring
>>>>>>>> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
>>>>>>>> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Joel Stanley
>>>>>>>> <joel@xxxxxxxxx>; Andrew Jeffery <andrew@xxxxxxxx>; Philipp Zabel
>>>>>>>> <p.zabel@xxxxxxxxxxxxxx>; openbmc@xxxxxxxxxxxxxxxx;
>>>>>>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>>>>>>>> linux-aspeed@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>>>>>>>> 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