Date: Mon, 13 May 2024 11:56:08 -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 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. 

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.

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