Message-ID: <ZkTbBRRtmGlTU142@lizhi-Precision-Tower-5810>
Date: Wed, 15 May 2024 11:55:49 -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 Wed, May 15, 2024 at 03:07:37AM +0000, Joshua Yeong wrote:
> 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. 
> 

Do you have any actual user case for i3c target? So I will have better
overy all view?

Frank

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