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

[Ryan Chen]

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Wednesday, February 22, 2023 6:36 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 v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2
> 
> On 22/02/2023 11:31, Ryan Chen wrote:
> >> 	Board B
> >>> --------------------------------------------------------
> >> 	--------------------------------------------------------
> >>> |    i2c bus#1(master/slave)  <-------------------->
> >>> | fingerprint.(can be unplug)
> >> <--------------------> i2c bus#x (master/slave) |
> >>> |    i2c bus#2(master) -> tmp i2c device     |
> >> 	|									|
> >>> |    i2c bus#3(master) -> adc i2c device      |
> 	|
> >> 								|
> >>> --------------------------------------------------------
> >> 	--------------------------------------------------------
> >>> In this case i2c bus#1 need enable timeout, avoid suddenly unplug
> >>> the
> >> connector. That slave will keep state to drive clock stretching.
> >>> So it is specific enable in i2c bus#1. Others is not needed enable timeout.
> >>> Does this draw is more clear in scenario?
> >>
> >> I2C bus #1 works in slave mode? So you always need it for slave work?
> >
> > Yes, it is both slave/master mode. It is always dual role. Slave must always
> work.
> > Due to another board master will send.
> 
> I meant that you need this property when it works in slave mode? It would be
> then redundant to have in DT as it is implied by the mode.

But timeout feature is also apply in master. It for avoid suddenly slave miss(un-plug) 
Master can timeout and release the SDA/SCL, return. 

> 
> >
> >>>
> >>>>>
> >>>>> So in those reason add this timeout design in controller.
> >>>>
> >>>> You need to justify why DT is correct place for this property. DT
> >>>> is not for configuring OS, but to describe hardware. I gave you one
> >>>> possibility
> >>>> - why different boards would like to set this property. You said it
> >>>> is not board specific, thus all boards will have it (or none of them).
> >>>> Without any other reason, this is not a DT property. Drop.
> >>>>
> >>>>>>
> >>>>>>>
> >>>>>>>>> +
> >>>>>>>>> +  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
> >>>>
> >>>> Just keep it text - byte, buffered, dma
> >>>>
> >>>>>
> >>>>>> 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.
> >>>>
> >>>> Then anyway it does not look like property for Devicetree. DT
> >>>> describes hardware, not OS behavior.
> >>>
> >>> The same draw, in this case, i2c bus#1 that is multi-master transfer
> >> architecture.
> >>> Both will inactive with trunk data. That cane enable i2c#1 use DMA
> >>> transfer
> >> to reduce CPU utilized.
> >>> Others (bus#2/3) can keep byte/buff mode.
> >>
> >> Isn't then current bus configuration for I2C#1 known to the driver?
> >> Jeremy asked few other questions around here...
> >
> > No, The driver don't know currently board configuration.
> 
> It knows whether it is working in multi-master/slave mode.

But in DT can decide which i2c bus number can use dma or buffer mode transfer.
If in another i2c bus support master only, also can use dma to transfer trunk data to another slave. 

Best regards,
Ryan Chen