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

[Ryan Chen]

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...

> 
> I don't understand why we struggle so much to get basic info justifying this
> patchset.
> 
> 
> Best regards,
> Krzysztof