Message-ID: <ZkONGQPBeuR6oZ0Q@lizhi-Precision-Tower-5810>
Date: Tue, 14 May 2024 12:11:05 -0400
From: Frank Li <Frank.li@....com>
To: Joshua Yeong <joshua.yeong@...rfivetech.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

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.

> 
> 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