> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for > AST2600-i2cv2 > > On 05/03/2025 10:35, Ryan Chen wrote: > >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for > >> AST2600-i2cv2 > >> > >> On 27/02/2025 09:19, Ryan Chen wrote: > >>>> > >>>> > >>>>> aspeed,enable-byte: > >>>>> Force i2c controller use byte mode transfer. the byte mode > >>>>> transfer will send i2c data each byte by byte, inlcude address > read/write. > >>>> > >>>> Isn't this standard FIFO mode? > >>> Yes, it is. > >>>> > >>>> Why anyone would need to enable byte mode for given board? > >>> By default, it is buffer-mode, for performance, I don't want user > >>> enable > >> byte-mode, it will increase CPU utilize. > >>> But someone want to be force enable byte-mode, so I add properties. > >>> https://patchwork.ozlabs.org/project/linux-aspeed/patch/202410070352 > >>> 35 .2254138-3-ryan_chen@xxxxxxxxxxxxxx/ > >> > >> > >> I don't see the reason why this would be a board property. > >> > >> I understood need for DMA because it is shared and only some of the > >> controllers can use it. But why choice between buffer and FIFO > >> depending on hardware? > > > > By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes > buffer to transmit data. > > Enabling byte mode will increases CPU utilization, so it is not recommended. > > However, some user might require forcing byte mode, so I added this > property to allow that. > > > You are not answering the question. I asked why the choice depends on > hardware and you answer "user might required". > > Do you understand that this is about hardware, not user choices? The AST2600 I2C controller support 3 modes. 1. Byte mode, 2.buffer mode. 3.dma mode. Which Buffer vs byte mode. is buffer mode have 16 bytes buffer for each i2c transition. Byte mode is 1 by 1 transfer. So, my original submit patch is only for buffer/dma mode selection in property. But someone in review feedback want to use byte mode, so I add byte mode property now. If not acceptable, I can keep buffer/dma two mode selection in property. > > > >> > >> > >>>> > >>>>> > >>>>>> > >>>>>>> + may require a DTS to manually allocate which controller > >>>>>>> + can use > >>>>>> DMA mode. > >>>>>>> + The "aspeed,enable-dma" property allows control of this. > >>>>>>> + > >>>>>>> + In cases where one the hardware design results in a specific > >>>>>>> + controller handling a larger amount of data, a DTS would > likely > >>>>>>> + enable DMA mode for that one controller. > >>>>>>> + > >>>>>>> + aspeed,enable-byte: > >>>>>>> + type: boolean > >>>>>>> + description: | > >>>>>>> + I2C bus enable byte mode transfer. > >>>>>> > >>>>>> No, either this is expressed as lack of dma mode property or if > >>>>>> you have three modes, then rather some enum (aspeed,transfer-mode > >>>>>> ?) > >>>>> > >>>>> Thanks suggestion, I will using an enum property like > >>>>> aspeed,transfer-mode > >>>> instead. > >>>>>> > >>>>>> > >>>>>> > >>>>>>> + > >>>>>>> + aspeed,global-regs: > >>>>>>> + $ref: /schemas/types.yaml#/definitions/phandle > >>>>>>> + description: The phandle of i2c global register node. > >>>>>> > >>>>>> For what? Same question as usual: do not repeat property name, > >>>>>> but say what is this used for and what exactly it points to. > >>>>>> > >>>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is > >>>>>> how you call your device in datasheet? > >>>>>> > >>>>> I do descript in cover, should add into the yaml file ? > >>>> > >>>> > >>>> Again, cover letter does not matter. Your hardware must be explained > here. > >>> Will add into commit. > >>>> > >>>>> > >>>>> aspeed,global-regs: > >>>>> This global register is needed, global register is setting for new > >>>>> clock divide control, and new register set control. > >>>> > >>>> So this means you implement clock controller via syscon? > >>> No, it is just mode switch. It also explain in cover. I will add it in commit. > >>> The legacy register layout is mix controller/target register control together. > >> The following is add more detail description about new register > >> layout. And new feature set add for register. > >>>> > >>>>> > >>>>>> > >>>>>>> + > >>>>>>> required: > >>>>>>> - reg > >>>>>>> - compatible > >>>>>>> - clocks > >>>>>>> - resets > >>>>>>> > >>>>>>> +allOf: > >>>>>>> + - $ref: /schemas/i2c/i2c-controller.yaml# > >>>>>>> + - if: > >>>>>>> + properties: > >>>>>>> + compatible: > >>>>>>> + contains: > >>>>>>> + const: aspeed,ast2600-i2cv2 > >>>>>> > >>>>>> NAK, undocumented compatible. > >>>>> > >>>>> Sorry, I should add what kind of document about this compatible? > >>>> > >>>> You cannot add new compatibles without documenting them. > >>>> Documentation is in the form of DT schema and each compatible must > >>>> be listed (in some > >>>> way) in compatible property description. > >>> > >>> Sorry, do you mean, I add following in yaml or commit message? > >> > >> You need to list this in compatibles first. > > > > I will add it in compatible in next submit. > > > > enum: > > -aspeed,ast2600-i2cv2 > >> > >>> > >>> This series add AST2600 i2cv2 new register set driver. The i2cv2 > >>> driver is new > >> register set that have new clock divider option for more flexiable > generation. > >> And also have separate i2c controller and target register set for > >> control, patch > >> #2 is i2c controller driver only, patch #3 is add i2c target mode driver. > >> > >> All this describes driver, not hardware. > > > > Sorry, the cover letter description the register. I will add int in commit > message. > > > > -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 controller and target mode control. > > -Support 4 individual DMA buffers for controller Tx and Rx, target Tx > > and Rx. > > > > And following is new register set for package transfer sequence. > > -New Master operation mode: > > S -> Aw -> P > > S -> Aw -> TxD -> P > > S -> Ar -> RxD -> P > > S -> Aw -> RxD -> Sr -> Ar -> TxD -> P -Bus SDA lock auto-release > > capability for new controller DMA command mode. > > -Bus auto timeout for new controller/target DMA mode. > > > > The following is two versus register layout. > > Old: > > {I2CD00}: Function Control Register > > {I2CD04}: Clock and AC Timing Control Register > > {I2CD08}: Clock and AC Timing Control Register > > {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 > > I don't understand anything from that and how is this relevant to our > discussion. Sorry, I don't catch your wanted. I assume you want to know new mode register set compare original register set. > > > > >> > >>> > >>> The legacy register layout is mix controller/target register control together. > >> 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 controller and target mode control. > >>> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and > Rx. > >> > >> Does it mean hardware changed on AST2600? > > No Hw change, it is different mode setting will have another mode register > setting. > > Mode setting is in global register, I will add in next commit message > > Neither this. > > So it seems you describe already existing and documented I2C, but for some > reason you want second compatible. The problem is that you do not provide > reason from the point of view of bindings. > > To summarize: what your users want - don't care. Start properly describing > hardware and your SoC. OK, for ast2600 i2c controller have two register mode setting. One, I call it is old register setting, that is right now i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there have a global register that can set i2c controller as new mode register set. That I am going to drive. That I post is all register in new an old register list. For example, Global register [2] = 0 => i2c present as old register set Global register [2] = 1 => i2c present as new register set > > > > > > I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1) > > I2CG00: Device Master/Slave Mode Interrupt Status Register > > (I2CG0C[3]=0) > > I2CG04: Device Slave Mode Interrupt Status Register > > I2CG0C: Global Control Register > > I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1) > > > >> Or these are different devices > >> than aspeed,ast2600-i2c-bus? If this is not a different device, how > >> one SoC can have two different flavors of same device in the same instance? > > > > When global setting for new, will new register mapping, no setting will keep > old register mapping. > > > I cannot parse this. > > > Best regards, > Krzysztof
