Message-ID:
 <OS8PR06MB7541C69AB8E6425313DA8606F2DF2@OS8PR06MB7541.apcprd06.prod.outlook.com>
Date: Mon, 17 Mar 2025 09:21:57 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: "benh@...nel.crashing.org" <benh@...nel.crashing.org>, "joel@....id.au"
	<joel@....id.au>, "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.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>, Mo Elbadry <elbadrym@...gle.com>
Subject: RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
 AST2600-i2cv2

> 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@...eedtech.com/
> >>
> >>
> >> 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

Powered by blists - more mailing lists