Date: Mon, 13 May 2024 14:48:31 -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 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.

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