RE: [PATCH resend v9 1/8] i3c: add target mode support

[Joshua Yeong <joshua.yeong@starfivetech.com>]

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@nxp.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@intel.com>
> > > > >     | Closes:
> > > > > https://lore.kernel.org/oe-kbuild-all/202402030437.GdGCrKeK-
> > > > > lkp@intel.com/
> > > > >
> > > > >     Chagne from v4 to v5
> > > > >     - add include <linux/slab.h> to fix build error
> > > > >     | Reported-by: kernel test robot <lkp@intel.com>
> > > > >     | Closes:
> > > > > https://lore.kernel.org/oe-kbuild-all/202401270838.wdxHPaAT-
> > > > > lkp@intel.com/
> > > > >
> > > > >     Chagne from v4 to v8
> > > > >     - add include <linux/slab.h> to fix build error
> > > > >     | Reported-by: kernel test robot <lkp@intel.com>
> > > > >     | Closes:
> > > > > https://lore.kernel.org/oe-kbuild-all/202401270838.wdxHPaAT-
> > > > > lkp@intel.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.

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.


> 
> >
> > > 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@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-i3c