Re: [PATCH 1/2] i3c: Add support for HDR modes.

[vitor <vitor.soares@synopsys.com>]

On 25/02/19 08:56, Boris Brezillon wrote:
> Hi Vitor
>
> On Fri, 22 Feb 2019 17:41:34 +0000
> vitor <vitor.soares@synopsys.com> wrote:
>
>> On 22/02/19 15:02, Przemyslaw Gaj wrote:
>>>>>>  struct i3c_ibi_payload {
>>>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>>>>>> index ada956a..fd50473 100644
>>>>>> --- a/include/linux/i3c/master.h
>>>>>> +++ b/include/linux/i3c/master.h
>>>>>> @@ -386,6 +386,10 @@ struct i3c_bus {
>>>>>>   *		  This method is mandatory.
>>>>>>   * @priv_xfers: do one or several private I3C SDR transfers
>>>>>>   *		This method is mandatory.
>>>>>> + * @send_hdr_cmds: send one or several HDR commands. If there is more than one
>>>>>> + *		   command, they should ideally be sent in the same HDR
>>>>>> + *		   transaction.
>>>>>> + *		   This method is optional.
>>>>>>   * @attach_i2c_dev: called every time an I2C device is attached to the bus.
>>>>>>   *		    This is a good place to attach master controller specific
>>>>>>   *		    data to I2C devices.
>>>>>> @@ -457,6 +461,9 @@ struct i3c_master_controller_ops {
>>>>>>  	int (*priv_xfers)(struct i3c_dev_desc *dev,
>>>>>>  			  struct i3c_priv_xfer *xfers,
>>>>>>  			  int nxfers);
>>>>>> +	int (*send_hdr_cmds)(struct i3c_dev_desc *dev,
>>>>>> +			     const struct i3c_hdr_cmd *cmds,
>>>>>> +			     int ncmds);
>>>>>>  	int (*attach_i2c_dev)(struct i2c_dev_desc *dev);
>>>>>>  	void (*detach_i2c_dev)(struct i2c_dev_desc *dev);
>>>>>>  	int (*i2c_xfers)(struct i2c_dev_desc *dev,    
>>>>> With this approach the controller between a start and stop can only transmit in SDR or HDR.
>>>>>
>>>>> This is limited for devices that need the following frame:
>>>>>     <Start><SDR xfer><Repeated Start><HDR command><Stop>  
>>> Actually, there is repeated start between SDR and ENTHDR. There is no repeated
>>> start between ENTHDR and HDR commands, HDR command has to start immedetly after
>>> ENTHDR. Of course, hdr restart may occur between HDR commands.  
>> Sorry, I meant all HDR frame.
> Then the proposed interface should work just fine, since we pass an
> array of HDR commands to execute.

I think it is not clear yet. When I referred <HDR command> I meant all HDR frame (ENTHDRx, HDR command, HDR CRC).

>>>> If this is a use case we want to support, then we should probably have
>>>> something more generic than what we currently have.
>>>>
>>>> Something like
>>>>
>>>> enum i3c_xfer_type {
>>>> 	I3C_CCC_XFER,
>>>> 	I3C_SDR_XFER,
>>>> 	I3C_HDR_XFER,
>>>> }
>>>>
>>>> struct i3c_xfer {
>>>> 	enum i3c_xfer_type type;
>>>> 	union {
>>>> 		struct i3c_ccc_cmd ccc;
>>>> 		struct i3c_priv_xfer sdr;
>>>> 		struct i3c_hdr_cmd hdr;
>>>> 	};
>>>> }  
>> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too.
>>
>> Do you agree?
> Well, if we want to have a generic interface and not force all drivers
> to implement one hook per transfer type, we should also have CCC
> commands in this enum. We can still make sure the user does not pass
> CCC commands before forwarding the request to the controller.

I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later.

> Anyway, as for all the other changes proposed so far, I'd like to see a
> real use case for this SDR/HDR[/CCC] mix. Do you have a device spec
> describing such a sequence?

From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case.

But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START).
So, the question is why do not support it instead of limiting it?

We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR).

and supporting it we are cover more use cases.
> Regards,
>
> Boris

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c