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

[Boris Brezillon <boris.brezillon@collabora.com>]

Hi Vitor,

On Mon, 25 Feb 2019 16:45:21 +0000
vitor <vitor.soares@synopsys.com> wrote:

> On 25/02/19 13:10, Boris Brezillon wrote:
> >>>>>> something more generic than what we currently have.
> >>>>>>
> >>>>>> Something like
> >>>>>>
> >>>>>> enum i3c_xfer_type {
> >>>>>> 	I3C_CCC_XFER,
> >>>>>> 	I3C_SDR_XFER,
> >>>>>> 	I3C_HDR_XFER,

Forgot
		I3C_I2C_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;

and 

			struct i2c_msg i2c;

Since you want to make that fully generic.

> >>>>>> 	};
> >>>>>> }      
> >>>> @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?  
> > Well, all depends on the extra complexity to support this case. If it's
> > simple enough, I won't complain.
> >  
> >> 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).  
> > Exactly, we don't know, so maybe we should just wait before introducing
> > HDR mode...
> >  
> >> and supporting it we are cover more use cases.  
> > Are we even sure all controllers will be able to chain things like
> > that?   
> 
> Why not? for the controllers shouldn't be a problem. The problem can be if it doesn't support.

I don't know, maybe because some HW will be too limited to support
chained requests. I'm speculating, just like you are :-P.

> 
> > For instance, in it's non-DMA mode, the Cadence controller is
> > limited by the FIFO size, which means you anyway won't be able to
> > chain too much frames using a REPEATED START. So, any device relying on
> > REPEATED START for atomic sequences is unlikely to work with all master
> > controllers.  
> 
> IMO this isn't up to us to decide, we just need to provide support and try to cover the most possible use cases.

And yet, you take a decision based on your own speculations. You might
possibly be right, but until we start seeing devices that support HDR
modes that's not something we can be sure of.

> Right now we only have 2 masters to support this kind of change and I just see some effort to support the CCC commands.

Support CCC commands? We already support CCC commands. What do you mean?

> 
> I would like to test this scenario, what do you think that can add extra complexity?

Go ahead and post an implementation, we'll see how it looks like.
BTW, you have quite a few things on your TODO list already, but none of
these have led to patches being posted. Maybe you should focus on one
topic and finish it instead of constantly bringing new things you say
you plan to work on.

Regards,

Boris

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