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

From: Joshua Yeong <joshua.yeong@starfivetech.com>
To: Frank Li <Frank.li@nxp.com>
Cc: "alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>,
	"conor.culhane@silvaco.com" <conor.culhane@silvaco.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"ilpo.jarvinen@linux.intel.com" <ilpo.jarvinen@linux.intel.com>,
	"imx@lists.linux.dev" <imx@lists.linux.dev>,
	"jirislaby@kernel.org" <jirislaby@kernel.org>,
	"joe@perches.com" <joe@perches.com>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"krzysztof.kozlowski@linaro.org" <krzysztof.kozlowski@linaro.org>,
	"linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
	"miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
	"robh@kernel.org" <robh@kernel.org>,
	"zbigniew.lukwinski@linux.intel.com"
	<zbigniew.lukwinski@linux.intel.com>
Subject: RE: [PATCH resend v9 1/8] i3c: add target mode support
Date: Mon, 13 May 2024 16:18:00 +0000	[thread overview]
Message-ID: <SH0PR01MB084189445E4928BC5047495AF9E22@SH0PR01MB0841.CHNPR01.prod.partner.outlook.cn> (raw)
In-Reply-To: <ZkI4GO4czdgs5/Xy@lizhi-Precision-Tower-5810>

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. 

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

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c