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

[Krzysztof Kozlowski]

On 21/02/2023 04:32, Ryan Chen wrote:
> Hello Jeremy,
> 
>> -----Original Message-----
>> From: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx>
>> Sent: Monday, February 20, 2023 7:24 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
>>
>> Hi Ryan,
>>
>>>>> +  clock-frequency:
>>>>> +    description:
>>>>> +      Desired I2C bus clock frequency in Hz. default 100khz.
>>>>> +
>>>>> +  multi-master:
>>>>> +    type: boolean
>>>>> +    description:
>>>>> +      states that there is another master active on this bus
>>>>
>>>> These are common to all i2c controllers, but I see that
>>>> i2c-controller.yaml doesn't include them (while i2c.text does).
>>>>
>>>> I assume we're OK to include these in the device bindings in the meantime.
>>>> But in that case, you may also want to include the common "smbus-alert"
>>>> property, which you consume in your driver.
>>>>
>>> Since i2c.text have multi-master, smbus-alert. I don't need those two right?
>>
>> Depends whether the maintainers consider i2c.text as part of the schema, I
>> figure. Might be best to get their input on this.
> 
> 
> Yes, I will drop this, also integrate into aspeed,i2c.yaml file.
> 
>>>>> +  timeout:
>>>>> +    type: boolean
>>>>> +    description: Enable i2c bus timeout for master/slave (35ms)
>>>>> +
>>>>> +  byte-mode:
>>>>> +    type: boolean
>>>>> +    description: Force i2c driver use byte mode transmit
>>>>> +
>>>>> +  buff-mode:
>>>>> +    type: boolean
>>>>> +    description: Force i2c driver use buffer mode transmit
>>>>
>>>> These three aren't really a property of the hardware, more of the
>>>> intended driver configuration. Do they really belong in the DT?
>>>>
>>> Sorry, I am confused.
>>> This is hardware controller mode setting for each i2c transfer.
>>> So I add it in property for change different i2c transfer mode.
>>> Is my mis-understand the property setting?
>>
>> It depends what this is configuration is for.
>>
>> Would you set the transfer mode based on the design of the board? Is there
>> something about the physical i2c bus wiring (or some other hardware design
>> choice) that would mean you use one setting over another?
>>
> No, it not depend on board design. It is only for register control for controller transfer behave.

Then DT does not look like suitable place for it. Drop the property.


> The controller support 3 different trigger mode for transfer.
> Byte mode: it means step by step to issue transfer.
> Example i2c read, each step will issue interrupt then driver need trigger for 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. 
> 
> 
>> On the other hand, if it's just because of OS behaviour, then this doesn't belong
>> in the DT.
>>
>> Maybe to help us understand: why would you ever *not* want DMA mode?
>> Isn't that always preferable?
> In AST SOC i2c design is 16 i2c bus share one dma engine. 
> It can be switch setting by dts setting. Otherwise driver by default probe is DMA mode.

DMA mode is chosen by existence (or lack) of dmas property, isn't it?
Why do you need separate property instead of using the standard one?

Best regards,
Krzysztof