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@xxxxxxx> > > > > > > > > --- > > > > > > > > > > > > > > > > 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@xxxxxxxxx> > > > > > > > > | Closes: > > > > > > > > https://lore.kernel.org/oe-kbuild-all/202402030437.GdGCrKeK- > > > > > > > > lkp@xxxxxxxxx/ > > > > > > > > > > > > > > > > Chagne from v4 to v5 > > > > > > > > - add include <linux/slab.h> to fix build error > > > > > > > > | Reported-by: kernel test robot <lkp@xxxxxxxxx> > > > > > > > > | Closes: > > > > > > > > https://lore.kernel.org/oe-kbuild-all/202401270838.wdxHPaAT- > > > > > > > > lkp@xxxxxxxxx/ > > > > > > > > > > > > > > > > Chagne from v4 to v8 > > > > > > > > - add include <linux/slab.h> to fix build error > > > > > > > > | Reported-by: kernel test robot <lkp@xxxxxxxxx> > > > > > > > > | Closes: > > > > > > > > https://lore.kernel.org/oe-kbuild-all/202401270838.wdxHPaAT- > > > > > > > > lkp@xxxxxxxxx/ > > > > > > > > > > > > > > > > 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@xxxxxxxxxxxxxxxxxxx > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-i3c
