Date: Wed, 15 May 2024 03:07:37 +0000
From: Joshua Yeong <joshua.yeong@...rfivetech.com>
To: Frank Li <Frank.li@....com>
CC: "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
	"conor.culhane@...vaco.com" <conor.culhane@...vaco.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>, "jirislaby@...nel.org"
	<jirislaby@...nel.org>, "joe@...ches.com" <joe@...ches.com>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
	"linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
	"miquel.raynal@...tlin.com" <miquel.raynal@...tlin.com>, "robh@...nel.org"
	<robh@...nel.org>, "zbigniew.lukwinski@...ux.intel.com"
	<zbigniew.lukwinski@...ux.intel.com>
Subject: RE: [PATCH resend v9 1/8] i3c: add target mode support

Frank Li wrote:
> 
> On Tue, May 14, 2024 at 03:07:25AM +0000, Joshua Yeong wrote:
> > Frank Li wrote:
> > >
> > > On Mon, May 13, 2024 at 04:18:00PM +0000, Joshua Yeong wrote:
> > > > Frank Li wrote:
> > > > > On Mon, May 13, 2024 at 03:16:05AM +0000, Joshua Yeong wrote:
> > > > > > Joshua Yeong wrote:
> > > > > > >
> > > > > > > Introduce a new target core layer in order to support target
> > > > > > > functions in linux kernel. This comprises the controller
> > > > > > > library and function
> > > > > library.
> > > > > > > Controller library implements functions specific to an
> > > > > > > target controller and function library implements functions
> > > > > > > specific to an target
> > > > > function.
> > > > > > >
> > > > > > > Introduce a new configfs entry to configure the target
> > > > > > > function configuring and bind the target function with target
> controller.
> > > > > > >
> > > > > > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > > > > > ---
> > > > > > >
> > > > > > > Notes:
> > > > > > >     Change from v8 to v9
> > > > > > >     -none
> > > > > > >
> > > > > > >     Change from v7 to v8
> > > > > > >     Added missed head files
> > > > > > >
> > > > > > >     Change from v5 to v6
> > > > > > >     - fixed build error when have not CONFIG_TARGET_CONFIGFS
> > > > > > >     | Reported-by: kernel test robot <lkp@...el.com>
> > > > > > >     | Closes:
> > > > > > > https://lore.kernel.org/oe-kbuild-all/202402030437.GdGCrKeK-
> > > > > > > lkp@...el.com/
> > > > > > >
> > > > > > >     Chagne from v4 to v5
> > > > > > >     - add include <linux/slab.h> to fix build error
> > > > > > >     | Reported-by: kernel test robot <lkp@...el.com>
> > > > > > >     | Closes:
> > > > > > > https://lore.kernel.org/oe-kbuild-all/202401270838.wdxHPaAT-
> > > > > > > lkp@...el.com/
> > > > > > >
> > > > > > >     Chagne from v4 to v8
> > > > > > >     - add include <linux/slab.h> to fix build error
> > > > > > >     | Reported-by: kernel test robot <lkp@...el.com>
> > > > > > >     | Closes:
> > > > > > > https://lore.kernel.org/oe-kbuild-all/202401270838.wdxHPaAT-
> > > > > > > lkp@...el.com/
> > > > > > >
> > > > > > >  drivers/i3c/Kconfig        |  28 +-
> > > > > > >  drivers/i3c/Makefile       |   2 +
> > > > > > >  drivers/i3c/i3c-cfs.c      | 389 ++++++++++++++++++++++++++
> > > > > > >  drivers/i3c/target.c       | 453
> ++++++++++++++++++++++++++++++
> > > > > > >  include/linux/i3c/target.h | 548
> > > > > > > +++++++++++++++++++++++++++++++++++++
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > diff --git a/include/linux/i3c/target.h
> > > > > > > b/include/linux/i3c/target.h new file mode 100644 index
> > > > > > > 0000000000000..b0bf06685834c
> > > > > > > --- /dev/null
> > > > > > > +++ b/include/linux/i3c/target.h
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > +/**
> > > > > > > + * struct i3c_target_ctrl_ops - set of function pointers
> > > > > > > +for performing controller operations
> > > > > > > + * @set_config: set I3C controller configuration
> > > > > > > + * @enable: enable I3C controller
> > > > > > > + * @disable: disable I3C controller
> > > > > > > + * @raise_ibi: raise IBI interrupt to master
> > > > > > > + * @alloc_request: allocate a i3c_request, optional,
> > > > > > > +default use kmalloc
> > > > > > > + * @free_request: free a i3c_request, default use kfree
> > > > > > > + * @queue: queue an I3C transfer
> > > > > > > + * @dequeue: dequeue an I3C transfer
> > > > > > > + * @cancel_all_reqs: call all pending requests
> > > > > > > + * @fifo_status: current FIFO status
> > > > > > > + * @fifo_flush: flush hardware FIFO
> > > > > > > + * @hotjoin: raise hotjoin request to master
> > > > > > > + * @set_status_format1: set i3c status format1
> > > > > > > + * @get_status_format1: get i3c status format1
> > > > > > > + * @get_addr: get i3c dynmatic address
> > > > > > > + * @get_features: ops to get the features supported by the
> > > > > > > +I3C target controller
> > > > > > > + * @owner: the module owner containing the ops  */ struct
> > > > > > > +i3c_target_ctrl_ops {
> > > > > > > +	int (*set_config)(struct i3c_target_ctrl *ctrl, struct
> > > > > > > +i3c_target_func
> > > > > > > *func);
> > > > > > > +	int (*enable)(struct i3c_target_ctrl *ctrl);
> > > > > > > +	int (*disable)(struct i3c_target_ctrl *ctrl);
> > > > > > > +	int (*raise_ibi)(struct i3c_target_ctrl *ctrl, void *p, u8
> > > > > > > +size);
> > > > > > > +
> > > > > > > +	struct i3c_request *(*alloc_request)(struct
> > > > > > > +i3c_target_ctrl *ctrl, gfp_t
> > > > > > > gfp_flags);
> > > > > > > +	void (*free_request)(struct i3c_request *req);
> > > > > > > +
> > > > > > > +	int (*queue)(struct i3c_request *req, gfp_t gfp_flags);
> > > > > > > +	int (*dequeue)(struct i3c_request *req);
> > > > > > > +
> > > > > > > +	void (*cancel_all_reqs)(struct i3c_target_ctrl *ctrl, bool
> > > > > > > +tx);
> > > > > > > +
> > > > > > > +	int (*fifo_status)(struct i3c_target_ctrl *ctrl, bool tx);
> > > > > > > +	void (*fifo_flush)(struct i3c_target_ctrl *ctrl, bool tx);
> > > > > > > +	int (*hotjoin)(struct i3c_target_ctrl *ctrl);
> > > > > > > +	int (*set_status_format1)(struct i3c_target_ctrl *ctrl,
> > > > > > > +u16
> > > status);
> > > > > > > +	u16 (*get_status_format1)(struct i3c_target_ctrl *ctrl);
> > > > > > > +	u8  (*get_addr)(struct i3c_target_ctrl *ctrl);
> > > > > > > +	const struct i3c_target_ctrl_features
> > > > > > > +*(*get_features)(struct
> > > > > > > i3c_target_ctrl *ctrl);
> > > > > > > +	struct module *owner;
> > > > > > > +};
> > > > > >
> > > > > > This structure looks very different from the master controller
> > > > > > ops
> > > > > 'i3c_master_controller_ops'.
> > > > > > I think you could probably combine 'set_config, enable' into
> > > > > > 'bus_init',
> > > > > 'disable' to 'bus_cleanup'.
> > > > > > Also using the 'struct i3c_priv_xfer' rather redefining
> > > > > > another 'struct
> > > > > i3c_request' would be much cleaner.
> > > > >
> > > > > Thanks, let me think this. i3c_priv_xter may include read and
> > > > > write, or write and read. I3C is quite fast. Most time software
> > > > > are not quite enough to handle it in time.
> > > > >
> > > > > If data len bigger than FIFO size, it have to use DMA to
> > > > > transfer it because there are not flow controler at target side (write
> direction).
> > > > > Read, hardware to use early terminate to tell host read too fast.
> > > > > but for write, 9th bit is parit check bit. Data will be lost if
> > > > > software have not read from FIFO in time. If use external DMA to
> > > > > do that, it needs switch dma channel(generally, DMA rx and tx
> > > > > are two channel). switch channel automatically are quite
> > > > > challenge at current dma
> > > engine framework.
> > > > >
> > > >
> > > > I guess when data is larger than FIFO, the i3c controller (master)
> > > > should 'aware' of this limitation when using such target device.
> > > > So that appropriate handle like make new read command, or do a
> > > > write then another larger read request.
> > >
> > > Between two commands, such as two write command, some data may lost
> > > because first one have not chance to fetch it from FIFO.
> > >
> > > Function driver can add some recovery mathod. But still better to
> > > use modem async method to queue multi transfer and handle by dma
> > > automatically. One transfer each time are just special case for queue.
> > >
> > > If DMA support,  queue can work as big cycle buffer, underrun will
> > > never happen unless system wrong, such as disable irq over 1 second.
> > >
> > > Actually, I worry about still i3c_priv_xfer, write followed read case.
> > > Most likely, host driver do like
> > >
> > > 	write (cmd index/reg index) then read value or command result.
> > >
> > > It is hard to decode cmd and send back data immedicately.
> > >
> > > if there are not special DMA support,  more realistic usage mode is
> > >
> > > cmd1, cmd2, cmd3,....
> > >
> > > send back
> > > 	resp1, resp2, ....
> > >
> > > instead of comebine cmd1 and resp1 at one transfer.
> > >
> > > Host can do that just because all SCL are controller by host controllers.
> >
> > The DMA queue mechanism should still be invisible to the framework.
> > Target could use info from SETMRL/SETMWL to 'prepare' whether
> > internally DMA is needed.
> 
> You are right. DMA should be invisiable to the framework, but framework
> need provide API to prepare receiver buffer advance before transfer happen.
> Only function driver know how much data need be prepared advance. For
> host read/target write case, It also need cancel request function incase host
> have not fetch data in certain time. Data need be updated. function driver
> can't simple update data buffer, because it may already map to DMA.
> function driver need dequeue and queue request again.
> 
> framework should abstract common part of difference i3c target controller
> and target function driver to avoid duplicate code.
> 
> And also need consider linux's irq latency limiation, try to make the function
> driver can work on all target controller drivers.
> 
> I think priv_xfer, W, R, ...  are hard to implement. Maybe limit priv_xfer 's len to
> 1 or 2. But if limited to 1, it becames no big difference with current implement.
> 

I think if the target unable to handle multiple transaction I think it is safe
to throw NACK. In the case small transaction is needed we can still use the 
same 'struc i3c_priv_xfer'. If it is large transaction and target unable to 
support multiple transaction it should be the target limitation rather than 
limiting every target private transfer here. 

> >
> > We can also argue that the max read length should always not larger
> > than the FIFO size. Slave data transfer interface should still match
> > what i3c spec expect to be, rather than the hardware limitation here.
> >
> 
> According to spec:
>  	5.1.9.3.5 Maximum write length in a single command
> 
> I am not resure "singe command here". I think it should be between "start"
> and "repeat start/stop".
> 
> If my understand is correct,  host still send out W R W R ... as whole I3C
> transaction, each one less than FIFO size. but totally size may exceed FIFO size.
> 
> 
> >
> > >
> > > >
> > > > > Of course it will be possible if hardware implement simpilar I3C
> > > > > HCI command queue. But I still not found any hardware that can do
> that yet.
> > > > >
> > > > > If only limited data size to FIFO size, there are still
> > > > > limiation for xfer.  if write, read, write, read, two write may
> > > > > combined in one FIFO, target driver may not split it which
> > > > > depend on hardware detect repeat start and insert something into fifo.
> > > > >
> > > > > It is hard to support all user case by I3C target software in linux.
> > > >
> > > > Can I know the speed limitation that you having issue while
> > > > running on
> > > silicon?
> > >
> > > I am not sure what "speed limitation" here? Do you means irq latency
> issue.
> > >
> > > when I get "stop", there are new data already come into FIFO.  It is
> > > not for problem for tty case now.
> > >
> > > But if it is well defined frame data, it may be problem.
> > >
> > > I saw some data lost when target are busy without DMA support.
> > > driver can query how many fifo space avaible before send data, but
> > > it reduced bus utilities rate.
> > >
> > > >
> > > > >
> > > > > write follow by read, generally used from get hardware register
> > > > > value from target side, which almost impossible by software, it
> > > > > is not quick enough to parse (write value) then send back data.
> > > > >
> > > > > >
> > > > > > In the master i3c side they don't abbreviate
> > > > > > i3c_master_controller to i3c_master_ctrl. I think we should do the
> same here to standardize it.
> > > > > > Thanks
> > > > >
> > > > > The problem is in I3C spec: "master" already depericated
> > > > >
> > > > > from i3c spec 1.1.1
> > > > >
> > > > > "Controller: An I3C Device that is capable of controlling the I3C Bus."
> > > > >
> > > > > master/slave => controller/target.
> > > > >
> > > > > It will become more confused if i3c_target_controller. "controller"
> > > > > are ambiguity.
> > > > >
> > > > > If "master" => "host" would be better situation, but not happen.
> > > > > A point, I use "ctrl"  here to avoid I3C spec defined "controller" term.
> > > > >
> > > > > Anyway, I hope more people that involve target support discussion.
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Joshua
> > > > > >
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > linux-i3c mailing list
> > > > > > > linux-i3c@...ts.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-i3c

Powered by blists - more mailing lists