[PATCH v2 0/6] i3c: master: some improvment for i3c master

[PATCH v2 0/6] i3c: master: some improvment for i3c master

[Frank Li]

These are dependent on bugs fixed serial: https://lore.kernel.org/imx/20231016153232.2851095-1-Frank.Li@nxp.com/T/#t
There are three major improvement

1. Add actual size in i3c_transfer because i3c allow target early termiate
transfer.
2. Add API for i3c_dev_gettstatus_format1 for i3c comand GET_STATUS.
3. svc master enable hotjoin default

Change from v1 to v2.
- Hotjoin control by sys entry, default enable hotjoin, which standard i3c
feature, user can disable by echo 0 > /sys/bus/i3c/i3c-0/hotjoin
- use actual_len, add document
- using &xfers[i]
- still keep check cmd->xfer because i2c and ccc transfer, xfer is NULL.
- fixed issue found by checkpatch --strict

Frank Li (6):
  i3c: master: add enable(disable) hot join in sys entry
  i3c: master: svc: add hot join support
  i3c: add actual_len in i3c_priv_xfer
  i3c: svc: rename read_len as actual_len
  i3c: master: svc return actual transfer data len
  i3c: add API i3c_dev_gettstatus_format1() to get target device status

 drivers/i3c/device.c                |  24 ++++++
 drivers/i3c/internals.h             |   1 +
 drivers/i3c/master.c                | 110 ++++++++++++++++++++++++++++
 drivers/i3c/master/svc-i3c-master.c |  91 ++++++++++++++++++-----
 include/linux/i3c/device.h          |   3 +
 include/linux/i3c/master.h          |   5 ++
 6 files changed, 217 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Frank Li]

Add hotjoin entry in sys file system allow user enable/disable hotjoin
feature.

Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
Add api i3c_master_enable(disable)_hotjoin();

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c       | 84 ++++++++++++++++++++++++++++++++++++++
 include/linux/i3c/master.h |  5 +++
 2 files changed, 89 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 08aeb69a78003..ed5e27cd20811 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -526,6 +526,89 @@ static ssize_t i2c_scl_frequency_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(i2c_scl_frequency);
 
+static int i3c_set_hotjoin(struct i3c_master_controller *master, bool enable)
+{
+	int ret;
+
+	if (!master ||
+	    !master->ops ||
+	    !master->ops->enable_hotjoin ||
+	    !master->ops->disable_hotjoin
+	   )
+		return -EINVAL;
+
+	i3c_bus_normaluse_lock(&master->bus);
+
+	if (enable)
+		ret = master->ops->enable_hotjoin(master);
+	else
+		ret = master->ops->disable_hotjoin(master);
+
+	master->hotjoin = enable;
+
+	i3c_bus_normaluse_unlock(&master->bus);
+
+	return ret;
+}
+
+static ssize_t hotjoin_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+	int ret;
+	long res;
+
+	if (!i3cbus->cur_master)
+		return -EINVAL;
+
+	if (kstrtol(buf, 10, &res))
+		return -EINVAL;
+
+	ret = i3c_set_hotjoin(i3cbus->cur_master->common.master, !!res);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+/*
+ * i3c_master_enable_hotjoin - Enable hotjoin
+ * @master: I3C master object
+ *
+ * Return: a 0 in case of success, an negative error code otherwise.
+ */
+int i3c_master_enable_hotjoin(struct i3c_master_controller *master)
+{
+	return i3c_set_hotjoin(master, true);
+}
+EXPORT_SYMBOL_GPL(i3c_master_enable_hotjoin);
+
+/*
+ * i3c_master_disable_hotjoin - Disable hotjoin
+ * @master: I3C master object
+ *
+ * Return: a 0 in case of success, an negative error code otherwise.
+ */
+int i3c_master_disable_hotjoin(struct i3c_master_controller *master)
+{
+	return i3c_set_hotjoin(master, false);
+}
+EXPORT_SYMBOL_GPL(i3c_master_disable_hotjoin);
+
+static ssize_t hotjoin_show(struct device *dev, struct device_attribute *da, char *buf)
+{
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+	ssize_t ret;
+
+	i3c_bus_normaluse_lock(i3cbus);
+	ret = sysfs_emit(buf, "%d\n", i3cbus->cur_master->common.master->hotjoin);
+	i3c_bus_normaluse_unlock(i3cbus);
+
+	return ret;
+}
+
+static DEVICE_ATTR_RW(hotjoin);
+
 static struct attribute *i3c_masterdev_attrs[] = {
 	&dev_attr_mode.attr,
 	&dev_attr_current_master.attr,
@@ -536,6 +619,7 @@ static struct attribute *i3c_masterdev_attrs[] = {
 	&dev_attr_pid.attr,
 	&dev_attr_dynamic_address.attr,
 	&dev_attr_hdrcap.attr,
+	&dev_attr_hotjoin.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(i3c_masterdev);
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 0b52da4f23467..65b8965968af2 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -452,6 +452,8 @@ struct i3c_master_controller_ops {
 	int (*disable_ibi)(struct i3c_dev_desc *dev);
 	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
 				 struct i3c_ibi_slot *slot);
+	int (*enable_hotjoin)(struct i3c_master_controller *master);
+	int (*disable_hotjoin)(struct i3c_master_controller *master);
 };
 
 /**
@@ -487,6 +489,7 @@ struct i3c_master_controller {
 	const struct i3c_master_controller_ops *ops;
 	unsigned int secondary : 1;
 	unsigned int init_done : 1;
+	unsigned int hotjoin: 1;
 	struct {
 		struct list_head i3c;
 		struct list_head i2c;
@@ -543,6 +546,8 @@ int i3c_master_register(struct i3c_master_controller *master,
 			const struct i3c_master_controller_ops *ops,
 			bool secondary);
 void i3c_master_unregister(struct i3c_master_controller *master);
+int i3c_master_enable_hotjoin(struct i3c_master_controller *master);
+int i3c_master_disable_hotjoin(struct i3c_master_controller *master);
 
 /**
  * i3c_dev_get_master_data() - get master private data attached to an I3C
-- 
2.34.1

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Zbigniew, Lukwinski]

On 10/18/2023 10:59 PM, Frank Li wrote:
> Add hotjoin entry in sys file system allow user enable/disable hotjoin
> feature.
>
> Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> Add api i3c_master_enable(disable)_hotjoin();

What is the use case for having HJ enable knob in sysfs available for 
user space other than for debug stuff? In other words, does user space 
really need to enable/disable HJ in runtime for other reason but debug? 
If it is only for debug maybe it  could be move to debugFS?

I think that maybe HJ enable knob shall be available in DT so you could 
control the default state for this feature also at compilation phase?

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>   drivers/i3c/master.c       | 84 ++++++++++++++++++++++++++++++++++++++
>   include/linux/i3c/master.h |  5 +++
>   2 files changed, 89 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 08aeb69a78003..ed5e27cd20811 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -526,6 +526,89 @@ static ssize_t i2c_scl_frequency_show(struct device *dev,
>   }
>   static DEVICE_ATTR_RO(i2c_scl_frequency);
>   
> +static int i3c_set_hotjoin(struct i3c_master_controller *master, bool enable)
> +{
> +	int ret;
> +
> +	if (!master ||
> +	    !master->ops ||
> +	    !master->ops->enable_hotjoin ||
> +	    !master->ops->disable_hotjoin
> +	   )
> +		return -EINVAL;
> +
> +	i3c_bus_normaluse_lock(&master->bus);
> +
> +	if (enable)
> +		ret = master->ops->enable_hotjoin(master);
> +	else
> +		ret = master->ops->disable_hotjoin(master);
> +
> +	master->hotjoin = enable;
> +
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	return ret;
> +}
> +
> +static ssize_t hotjoin_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	int ret;
> +	long res;
> +
> +	if (!i3cbus->cur_master)
> +		return -EINVAL;
> +
> +	if (kstrtol(buf, 10, &res))
> +		return -EINVAL;
> +
> +	ret = i3c_set_hotjoin(i3cbus->cur_master->common.master, !!res);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +/*
> + * i3c_master_enable_hotjoin - Enable hotjoin
> + * @master: I3C master object
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +int i3c_master_enable_hotjoin(struct i3c_master_controller *master)
> +{
> +	return i3c_set_hotjoin(master, true);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_enable_hotjoin);
> +
> +/*
> + * i3c_master_disable_hotjoin - Disable hotjoin
> + * @master: I3C master object
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +int i3c_master_disable_hotjoin(struct i3c_master_controller *master)
> +{
> +	return i3c_set_hotjoin(master, false);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_disable_hotjoin);
> +
> +static ssize_t hotjoin_show(struct device *dev, struct device_attribute *da, char *buf)
> +{
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	ssize_t ret;
> +
> +	i3c_bus_normaluse_lock(i3cbus);
> +	ret = sysfs_emit(buf, "%d\n", i3cbus->cur_master->common.master->hotjoin);
> +	i3c_bus_normaluse_unlock(i3cbus);
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR_RW(hotjoin);
> +
>   static struct attribute *i3c_masterdev_attrs[] = {
>   	&dev_attr_mode.attr,
>   	&dev_attr_current_master.attr,
> @@ -536,6 +619,7 @@ static struct attribute *i3c_masterdev_attrs[] = {
>   	&dev_attr_pid.attr,
>   	&dev_attr_dynamic_address.attr,
>   	&dev_attr_hdrcap.attr,
> +	&dev_attr_hotjoin.attr,
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(i3c_masterdev);
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 0b52da4f23467..65b8965968af2 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -452,6 +452,8 @@ struct i3c_master_controller_ops {
>   	int (*disable_ibi)(struct i3c_dev_desc *dev);
>   	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
>   				 struct i3c_ibi_slot *slot);
> +	int (*enable_hotjoin)(struct i3c_master_controller *master);
> +	int (*disable_hotjoin)(struct i3c_master_controller *master);
>   };
>   
>   /**
> @@ -487,6 +489,7 @@ struct i3c_master_controller {
>   	const struct i3c_master_controller_ops *ops;
>   	unsigned int secondary : 1;
>   	unsigned int init_done : 1;
> +	unsigned int hotjoin: 1;
>   	struct {
>   		struct list_head i3c;
>   		struct list_head i2c;
> @@ -543,6 +546,8 @@ int i3c_master_register(struct i3c_master_controller *master,
>   			const struct i3c_master_controller_ops *ops,
>   			bool secondary);
>   void i3c_master_unregister(struct i3c_master_controller *master);
> +int i3c_master_enable_hotjoin(struct i3c_master_controller *master);
> +int i3c_master_disable_hotjoin(struct i3c_master_controller *master);
>   
>   /**
>    * i3c_dev_get_master_data() - get master private data attached to an I3C

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Miquel Raynal]

Hi Lukwinski,

zbigniew.lukwinski@linux.intel.com wrote on Fri, 20 Oct 2023 10:55:27
+0200:

> On 10/18/2023 10:59 PM, Frank Li wrote:
> > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > feature.
> >
> > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > Add api i3c_master_enable(disable)_hotjoin();  
> 
> What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?

I don't think hotjoin should be considered as a debug feature. The
problem here is the power consumption which is higher if you enable
this feature (you need to keep everything clocked and ready to handle
an IBI) whereas if your design is "fixed" (more like an I2C bus) you
may save power by disabling this feature.

A module parameter does not fit here because it's a per-bus
configuration.

Thanks,
Miquèl

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Frank Li]

On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> Hi Lukwinski,
> 
> zbigniew.lukwinski@linux.intel.com wrote on Fri, 20 Oct 2023 10:55:27
> +0200:
> 
> > On 10/18/2023 10:59 PM, Frank Li wrote:
> > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > feature.
> > >
> > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > Add api i3c_master_enable(disable)_hotjoin();  
> > 
> > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
> 
> I don't think hotjoin should be considered as a debug feature. The
> problem here is the power consumption which is higher if you enable
> this feature (you need to keep everything clocked and ready to handle
> an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> may save power by disabling this feature.
> 
> A module parameter does not fit here because it's a per-bus
> configuration.

I agree. sys entry is more flexiable. and let controller choose better
power saving policy for difference user case.

Frank

> 
> Thanks,
> Miquèl

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Miquel Raynal]

Hi Frank,

Frank.li@nxp.com wrote on Fri, 20 Oct 2023 10:20:57 -0400:

> On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > Hi Lukwinski,
> > 
> > zbigniew.lukwinski@linux.intel.com wrote on Fri, 20 Oct 2023 10:55:27
> > +0200:
> >   
> > > On 10/18/2023 10:59 PM, Frank Li wrote:  
> > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > feature.
> > > >
> > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > Add api i3c_master_enable(disable)_hotjoin();    
> > > 
> > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?  
> > 
> > I don't think hotjoin should be considered as a debug feature. The
> > problem here is the power consumption which is higher if you enable
> > this feature (you need to keep everything clocked and ready to handle
> > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > may save power by disabling this feature.
> > 
> > A module parameter does not fit here because it's a per-bus
> > configuration.  
> 
> I agree. sys entry is more flexiable. and let controller choose better
> power saving policy for difference user case.

Maybe it's not the first time this case is faced, would you mind
including power management maintainers in this discussion? Perhaps they
might have pointers or even have the solution already.

Thanks,
Miquèl

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Frank Li]

On Fri, Oct 20, 2023 at 04:33:48PM +0200, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.li@nxp.com wrote on Fri, 20 Oct 2023 10:20:57 -0400:
> 
> > On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > > Hi Lukwinski,
> > > 
> > > zbigniew.lukwinski@linux.intel.com wrote on Fri, 20 Oct 2023 10:55:27
> > > +0200:
> > >   
> > > > On 10/18/2023 10:59 PM, Frank Li wrote:  
> > > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > > feature.
> > > > >
> > > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > > Add api i3c_master_enable(disable)_hotjoin();    
> > > > 
> > > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?  
> > > 
> > > I don't think hotjoin should be considered as a debug feature. The
> > > problem here is the power consumption which is higher if you enable
> > > this feature (you need to keep everything clocked and ready to handle
> > > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > > may save power by disabling this feature.
> > > 
> > > A module parameter does not fit here because it's a per-bus
> > > configuration.  
> > 
> > I agree. sys entry is more flexiable. and let controller choose better
> > power saving policy for difference user case.
> 
> Maybe it's not the first time this case is faced, would you mind
> including power management maintainers in this discussion? Perhaps they
> might have pointers or even have the solution already.

@Ulf and pm experts.

I3C have a features, which call hotjoin. Some controller need enable clock
and some power domain if support hotjoin.

there are two kinds user case.

case1:  All devices attached into I3C bus, not hotjoin happen. So
controller can use runtime_pm frame to make clock and power domain on only
when transferring data.

case2: some devices can dynamitc join when system running. Some clocks or
power domain need be kept to make hotjoin event detect logic work.

In one system, two cases may exist at same time. I3C bus1 for case1, I3C
bus2 for case 2. 

I add sys entry in I3C bus driver to let user can turn on/off this feature
for specific controller. 

My question: any exited solution can handle these in current power
mnanagement system. 

Frank

> 
> Thanks,
> Miquèl

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Alexandre Belloni]

On 20/10/2023 17:12:53+0200, Zbigniew, Lukwinski wrote:
> On 10/20/2023 4:33 PM, Miquel Raynal wrote:
> > Hi Frank,
> > 
> > Frank.li@nxp.com  wrote on Fri, 20 Oct 2023 10:20:57 -0400:
> > 
> > > On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > > > Hi Lukwinski,
> > > > 
> > > > zbigniew.lukwinski@linux.intel.com  wrote on Fri, 20 Oct 2023 10:55:27
> > > > +0200:
> > > > > On 10/18/2023 10:59 PM, Frank Li wrote:
> > > > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > > > feature.
> > > > > > 
> > > > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > > > Add api i3c_master_enable(disable)_hotjoin();
> > > > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
> > > > I don't think hotjoin should be considered as a debug feature. The
> > > > problem here is the power consumption which is higher if you enable
> > > > this feature (you need to keep everything clocked and ready to handle
> > > > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > > > may save power by disabling this feature.
> > > > 
> > > > A module parameter does not fit here because it's a per-bus
> > > > configuration.
> > > I agree. sys entry is more flexiable. and let controller choose better
> > > power saving policy for difference user case.
> > Maybe it's not the first time this case is faced, would you mind
> > including power management maintainers in this discussion? Perhaps they
> > might have pointers or even have the solution already.
> 
> I did not mind HJ as debug feature. But enabling / disabling the HJ sounds
> to me like debug option.
> 
> So the flow you are considering here is like this:?
> 
>     1. system boot with HJ enabled, so HJ works during initial bus
>        discovery
>     2. some entity in user space decides to disable HJ because power
>        consumption?
>     3. some entity in use space decide some time later to re-enable HJ
>        because some reason?
> 
> I am just wondering whether there is real use case when you starts with HJ
> enabled and than disable it
> 
> in runtime or start with HJ disabled and enable it in runtime. If you are
> taking care about power saving
> 
>  let's keep HJ disabled all the time. Default state for HJ could be
> controlled by DT entry.
> 

This would be HW configuration and not HW description.


> 
> > Thanks,
> > Miquèl

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Frank Li]

On Fri, Oct 20, 2023 at 10:25:19PM +0200, Alexandre Belloni wrote:
> On 20/10/2023 17:12:53+0200, Zbigniew, Lukwinski wrote:
> > On 10/20/2023 4:33 PM, Miquel Raynal wrote:
> > > Hi Frank,
> > > 
> > > Frank.li@nxp.com  wrote on Fri, 20 Oct 2023 10:20:57 -0400:
> > > 
> > > > On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > > > > Hi Lukwinski,
> > > > > 
> > > > > zbigniew.lukwinski@linux.intel.com  wrote on Fri, 20 Oct 2023 10:55:27
> > > > > +0200:
> > > > > > On 10/18/2023 10:59 PM, Frank Li wrote:
> > > > > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > > > > feature.
> > > > > > > 
> > > > > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > > > > Add api i3c_master_enable(disable)_hotjoin();
> > > > > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
> > > > > I don't think hotjoin should be considered as a debug feature. The
> > > > > problem here is the power consumption which is higher if you enable
> > > > > this feature (you need to keep everything clocked and ready to handle
> > > > > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > > > > may save power by disabling this feature.
> > > > > 
> > > > > A module parameter does not fit here because it's a per-bus
> > > > > configuration.
> > > > I agree. sys entry is more flexiable. and let controller choose better
> > > > power saving policy for difference user case.
> > > Maybe it's not the first time this case is faced, would you mind
> > > including power management maintainers in this discussion? Perhaps they
> > > might have pointers or even have the solution already.
> > 
> > I did not mind HJ as debug feature. But enabling / disabling the HJ sounds
> > to me like debug option.
> > 
> > So the flow you are considering here is like this:?
> > 
> >     1. system boot with HJ enabled, so HJ works during initial bus
> >        discovery
> >     2. some entity in user space decides to disable HJ because power
> >        consumption?
> >     3. some entity in use space decide some time later to re-enable HJ
> >        because some reason?
> > 
> > I am just wondering whether there is real use case when you starts with HJ
> > enabled and than disable it

I think it is validate user case. Assume a I3C GPS device, user only use
it when open map. Before map application open, enable i3c hotjoin and
power on GPS module. After map application close, disable i3c hotjoin and
power off GPS module.

Frank

> > 
> > in runtime or start with HJ disabled and enable it in runtime. If you are
> > taking care about power saving
> > 
> >  let's keep HJ disabled all the time. Default state for HJ could be
> > controlled by DT entry.
> > 
> 
> This would be HW configuration and not HW description.

Yes, DT maintainer may not accept this entry because it is not HW
description.

> 
> 
> > 
> > > Thanks,
> > > Miquèl
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Zbigniew, Lukwinski]

On 10/20/2023 10:36 PM, Frank Li wrote:
> On Fri, Oct 20, 2023 at 10:25:19PM +0200, Alexandre Belloni wrote:
>> On 20/10/2023 17:12:53+0200, Zbigniew, Lukwinski wrote:
>>> On 10/20/2023 4:33 PM, Miquel Raynal wrote:
>>>> Hi Frank,
>>>>
>>>> Frank.li@nxp.com  wrote on Fri, 20 Oct 2023 10:20:57 -0400:
>>>>
>>>>> On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
>>>>>> Hi Lukwinski,
>>>>>>
>>>>>> zbigniew.lukwinski@linux.intel.com  wrote on Fri, 20 Oct 2023 10:55:27
>>>>>> +0200:
>>>>>>> On 10/18/2023 10:59 PM, Frank Li wrote:
>>>>>>>> Add hotjoin entry in sys file system allow user enable/disable hotjoin
>>>>>>>> feature.
>>>>>>>>
>>>>>>>> Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
>>>>>>>> Add api i3c_master_enable(disable)_hotjoin();
>>>>>>> What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
>>>>>> I don't think hotjoin should be considered as a debug feature. The
>>>>>> problem here is the power consumption which is higher if you enable
>>>>>> this feature (you need to keep everything clocked and ready to handle
>>>>>> an IBI) whereas if your design is "fixed" (more like an I2C bus) you
>>>>>> may save power by disabling this feature.
>>>>>>
>>>>>> A module parameter does not fit here because it's a per-bus
>>>>>> configuration.
>>>>> I agree. sys entry is more flexiable. and let controller choose better
>>>>> power saving policy for difference user case.
>>>> Maybe it's not the first time this case is faced, would you mind
>>>> including power management maintainers in this discussion? Perhaps they
>>>> might have pointers or even have the solution already.
>>>
>>> I did not mind HJ as debug feature. But enabling / disabling the HJ sounds
>>> to me like debug option.
>>>
>>> So the flow you are considering here is like this:?
>>>
>>>      1. system boot with HJ enabled, so HJ works during initial bus
>>>         discovery
>>>      2. some entity in user space decides to disable HJ because power
>>>         consumption?
>>>      3. some entity in use space decide some time later to re-enable HJ
>>>         because some reason?
>>>
>>> I am just wondering whether there is real use case when you starts with HJ
>>> enabled and than disable it
> 
> I think it is validate user case. Assume a I3C GPS device, user only use
> it when open map. Before map application open, enable i3c hotjoin and
> power on GPS module. After map application close, disable i3c hotjoin and
> power off GPS module.
> 
Got it. I think we are on the same page. Thanks for explanations!
> Frank
> 
>>>
>>> in runtime or start with HJ disabled and enable it in runtime. If you are
>>> taking care about power saving
>>>
>>>   let's keep HJ disabled all the time. Default state for HJ could be
>>> controlled by DT entry.
>>>
>>
>> This would be HW configuration and not HW description.
> 
> Yes, DT maintainer may not accept this entry because it is not HW
> description.
> 
Sure. Makes sense.
>>
>>
>>>
>>>> Thanks,
>>>> Miquèl
>>
>> -- 
>> Alexandre Belloni, co-owner and COO, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com

Re: [PATCH v2 1/6] i3c: master: add enable(disable) hot join in sys entry

[Frank Li]

On Fri, Oct 20, 2023 at 10:42:20PM +0200, Zbigniew, Lukwinski wrote:
> On 10/20/2023 10:36 PM, Frank Li wrote:
> > On Fri, Oct 20, 2023 at 10:25:19PM +0200, Alexandre Belloni wrote:
> > > On 20/10/2023 17:12:53+0200, Zbigniew, Lukwinski wrote:
> > > > On 10/20/2023 4:33 PM, Miquel Raynal wrote:
> > > > > Hi Frank,
> > > > > 
> > > > > Frank.li@nxp.com  wrote on Fri, 20 Oct 2023 10:20:57 -0400:
> > > > > 
> > > > > > On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > > > > > > Hi Lukwinski,
> > > > > > > 
> > > > > > > zbigniew.lukwinski@linux.intel.com  wrote on Fri, 20 Oct 2023 10:55:27
> > > > > > > +0200:
> > > > > > > > On 10/18/2023 10:59 PM, Frank Li wrote:
> > > > > > > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > > > > > > feature.
> > > > > > > > > 
> > > > > > > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > > > > > > Add api i3c_master_enable(disable)_hotjoin();
> > > > > > > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
> > > > > > > I don't think hotjoin should be considered as a debug feature. The
> > > > > > > problem here is the power consumption which is higher if you enable
> > > > > > > this feature (you need to keep everything clocked and ready to handle
> > > > > > > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > > > > > > may save power by disabling this feature.
> > > > > > > 
> > > > > > > A module parameter does not fit here because it's a per-bus
> > > > > > > configuration.
> > > > > > I agree. sys entry is more flexiable. and let controller choose better
> > > > > > power saving policy for difference user case.
> > > > > Maybe it's not the first time this case is faced, would you mind
> > > > > including power management maintainers in this discussion? Perhaps they
> > > > > might have pointers or even have the solution already.
> > > > 
> > > > I did not mind HJ as debug feature. But enabling / disabling the HJ sounds
> > > > to me like debug option.
> > > > 
> > > > So the flow you are considering here is like this:?
> > > > 
> > > >      1. system boot with HJ enabled, so HJ works during initial bus
> > > >         discovery
> > > >      2. some entity in user space decides to disable HJ because power
> > > >         consumption?
> > > >      3. some entity in use space decide some time later to re-enable HJ
> > > >         because some reason?
> > > > 
> > > > I am just wondering whether there is real use case when you starts with HJ
> > > > enabled and than disable it
> > 
> > I think it is validate user case. Assume a I3C GPS device, user only use
> > it when open map. Before map application open, enable i3c hotjoin and
> > power on GPS module. After map application close, disable i3c hotjoin and
> > power off GPS module.
> > 
> Got it. I think we are on the same page. Thanks for explanations!
> > Frank

Does everyone agree on this method? If still need further discusion, I
can repost patch 3-6, which is independent on these.

Frank Li 

> > 
> > > > 
> > > > in runtime or start with HJ disabled and enable it in runtime. If you are
> > > > taking care about power saving
> > > > 
> > > >   let's keep HJ disabled all the time. Default state for HJ could be
> > > > controlled by DT entry.
> > > > 
> > > 
> > > This would be HW configuration and not HW description.
> > 
> > Yes, DT maintainer may not accept this entry because it is not HW
> > description.
> > 
> Sure. Makes sense.
> > > 
> > > 
> > > > 
> > > > > Thanks,
> > > > > Miquèl
> > > 
> > > -- 
> > > Alexandre Belloni, co-owner and COO, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com
> 

[PATCH v2 2/6] i3c: master: svc: add hot join support

[Frank Li]

Add hot join support for svc master controller. Enable hot join defaultly.
User can use sys entry to disable hot join.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 57 +++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 4 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index fedb31e0076c4..d8467607602af 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -128,6 +128,9 @@
 /* This parameter depends on the implementation and may be tuned */
 #define SVC_I3C_FIFO_SIZE 16
 
+#define SVC_I3C_EVENT_IBI	BIT(0)
+#define SVC_I3C_EVENT_HOTJOIN	BIT(1)
+
 struct svc_i3c_cmd {
 	u8 addr;
 	bool rnw;
@@ -205,6 +208,7 @@ struct svc_i3c_master {
 		spinlock_t lock;
 	} ibi;
 	struct mutex lock;
+	int enabled_events;
 };
 
 /**
@@ -425,13 +429,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 	switch (ibitype) {
 	case SVC_I3C_MSTATUS_IBITYPE_IBI:
 		dev = svc_i3c_master_dev_from_addr(master, ibiaddr);
-		if (!dev)
+		if (!dev || !(master->enabled_events & SVC_I3C_EVENT_IBI))
 			svc_i3c_master_nack_ibi(master);
 		else
 			svc_i3c_master_handle_ibi(master, dev);
 		break;
 	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
-		svc_i3c_master_ack_ibi(master, false);
+		if (master->enabled_events & SVC_I3C_EVENT_HOTJOIN)
+			svc_i3c_master_ack_ibi(master, false);
+		else
+			svc_i3c_master_nack_ibi(master);
 		break;
 	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
 		svc_i3c_master_nack_ibi(master);
@@ -468,7 +475,9 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 		svc_i3c_master_emit_stop(master);
 		break;
 	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
-		queue_work(master->base.wq, &master->hj_work);
+		svc_i3c_master_emit_stop(master);
+		if (master->enabled_events & SVC_I3C_EVENT_HOTJOIN)
+			queue_work(master->base.wq, &master->hj_work);
 		break;
 	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
 	default:
@@ -1468,6 +1477,7 @@ static int svc_i3c_master_enable_ibi(struct i3c_dev_desc *dev)
 		return ret;
 	}
 
+	master->enabled_events |= SVC_I3C_EVENT_IBI;
 	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
 
 	return i3c_master_enec_locked(m, dev->info.dyn_addr, I3C_CCC_EVENT_SIR);
@@ -1479,7 +1489,9 @@ static int svc_i3c_master_disable_ibi(struct i3c_dev_desc *dev)
 	struct svc_i3c_master *master = to_svc_i3c_master(m);
 	int ret;
 
-	svc_i3c_master_disable_interrupts(master);
+	master->enabled_events &= ~SVC_I3C_EVENT_IBI;
+	if (!master->enabled_events)
+		svc_i3c_master_disable_interrupts(master);
 
 	ret = i3c_master_disec_locked(m, dev->info.dyn_addr, I3C_CCC_EVENT_SIR);
 
@@ -1489,6 +1501,39 @@ static int svc_i3c_master_disable_ibi(struct i3c_dev_desc *dev)
 	return ret;
 }
 
+static int svc_i3c_master_enable_hotjoin(struct i3c_master_controller *m)
+{
+	struct svc_i3c_master *master = to_svc_i3c_master(m);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(master->dev);
+	if (ret < 0) {
+		dev_err(master->dev, "<%s> Cannot get runtime PM.\n", __func__);
+		return ret;
+	}
+
+	master->enabled_events |= SVC_I3C_EVENT_HOTJOIN;
+
+	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
+
+	return 0;
+}
+
+static int svc_i3c_master_disable_hotjoin(struct i3c_master_controller *m)
+{
+	struct svc_i3c_master *master = to_svc_i3c_master(m);
+
+	master->enabled_events &= ~SVC_I3C_EVENT_HOTJOIN;
+
+	if (!master->enabled_events)
+		svc_i3c_master_disable_interrupts(master);
+
+	pm_runtime_mark_last_busy(master->dev);
+	pm_runtime_put_autosuspend(master->dev);
+
+	return 0;
+}
+
 static void svc_i3c_master_recycle_ibi_slot(struct i3c_dev_desc *dev,
 					    struct i3c_ibi_slot *slot)
 {
@@ -1515,6 +1560,8 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = {
 	.recycle_ibi_slot = svc_i3c_master_recycle_ibi_slot,
 	.enable_ibi = svc_i3c_master_enable_ibi,
 	.disable_ibi = svc_i3c_master_disable_ibi,
+	.enable_hotjoin = svc_i3c_master_enable_hotjoin,
+	.disable_hotjoin = svc_i3c_master_disable_hotjoin,
 };
 
 static int svc_i3c_master_prepare_clks(struct svc_i3c_master *master)
@@ -1627,6 +1674,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
 
+	i3c_master_enable_hotjoin(&master->base);
+
 	return 0;
 
 rpm_disable:
-- 
2.34.1

Re: [PATCH v2 2/6] i3c: master: svc: add hot join support

[kernel test robot]

Hi Frank,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.6-rc6 next-20231018]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Frank-Li/i3c-master-add-enable-disable-hot-join-in-sys-entry/20231019-051444
base:   linus/master
patch link:    https://lore.kernel.org/r/20231018205929.3435110-3-Frank.Li%40nxp.com
patch subject: [PATCH v2 2/6] i3c: master: svc: add hot join support
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231019/202310190611.b14M4mH3-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231019/202310190611.b14M4mH3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <yujie.liu@intel.com>
| Closes: https://lore.kernel.org/r/202310190611.b14M4mH3-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/i3c/master/svc-i3c-master.c:212: warning: Function parameter or member 'lock' not described in 'svc_i3c_master'
>> drivers/i3c/master/svc-i3c-master.c:212: warning: Function parameter or member 'enabled_events' not described in 'svc_i3c_master'
   2 warnings as Errors


vim +212 drivers/i3c/master/svc-i3c-master.c

1c5ee2a77b1bac Clark Wang    2023-05-17  157  
dd3c52846d5954 Miquel Raynal 2021-01-21  158  /**
dd3c52846d5954 Miquel Raynal 2021-01-21  159   * struct svc_i3c_master - Silvaco I3C Master structure
dd3c52846d5954 Miquel Raynal 2021-01-21  160   * @base: I3C master controller
dd3c52846d5954 Miquel Raynal 2021-01-21  161   * @dev: Corresponding device
dd3c52846d5954 Miquel Raynal 2021-01-21  162   * @regs: Memory mapping
5496eac6ad7428 Miquel Raynal 2023-08-17  163   * @saved_regs: Volatile values for PM operations
dd3c52846d5954 Miquel Raynal 2021-01-21  164   * @free_slots: Bit array of available slots
dd3c52846d5954 Miquel Raynal 2021-01-21  165   * @addrs: Array containing the dynamic addresses of each attached device
dd3c52846d5954 Miquel Raynal 2021-01-21  166   * @descs: Array of descriptors, one per attached device
dd3c52846d5954 Miquel Raynal 2021-01-21  167   * @hj_work: Hot-join work
dd3c52846d5954 Miquel Raynal 2021-01-21  168   * @ibi_work: IBI work
dd3c52846d5954 Miquel Raynal 2021-01-21  169   * @irq: Main interrupt
dd3c52846d5954 Miquel Raynal 2021-01-21  170   * @pclk: System clock
dd3c52846d5954 Miquel Raynal 2021-01-21  171   * @fclk: Fast clock (bus)
dd3c52846d5954 Miquel Raynal 2021-01-21  172   * @sclk: Slow clock (other events)
dd3c52846d5954 Miquel Raynal 2021-01-21  173   * @xferqueue: Transfer queue structure
dd3c52846d5954 Miquel Raynal 2021-01-21  174   * @xferqueue.list: List member
dd3c52846d5954 Miquel Raynal 2021-01-21  175   * @xferqueue.cur: Current ongoing transfer
dd3c52846d5954 Miquel Raynal 2021-01-21  176   * @xferqueue.lock: Queue lock
dd3c52846d5954 Miquel Raynal 2021-01-21  177   * @ibi: IBI structure
dd3c52846d5954 Miquel Raynal 2021-01-21  178   * @ibi.num_slots: Number of slots available in @ibi.slots
dd3c52846d5954 Miquel Raynal 2021-01-21  179   * @ibi.slots: Available IBI slots
dd3c52846d5954 Miquel Raynal 2021-01-21  180   * @ibi.tbq_slot: To be queued IBI slot
dd3c52846d5954 Miquel Raynal 2021-01-21  181   * @ibi.lock: IBI lock

* @lock: <description>
* @enabled_events: <description>

dd3c52846d5954 Miquel Raynal 2021-01-21  182   */
dd3c52846d5954 Miquel Raynal 2021-01-21  183  struct svc_i3c_master {
dd3c52846d5954 Miquel Raynal 2021-01-21  184  	struct i3c_master_controller base;
dd3c52846d5954 Miquel Raynal 2021-01-21  185  	struct device *dev;
dd3c52846d5954 Miquel Raynal 2021-01-21  186  	void __iomem *regs;
1c5ee2a77b1bac Clark Wang    2023-05-17  187  	struct svc_i3c_regs_save saved_regs;
dd3c52846d5954 Miquel Raynal 2021-01-21  188  	u32 free_slots;
dd3c52846d5954 Miquel Raynal 2021-01-21  189  	u8 addrs[SVC_I3C_MAX_DEVS];
dd3c52846d5954 Miquel Raynal 2021-01-21  190  	struct i3c_dev_desc *descs[SVC_I3C_MAX_DEVS];
dd3c52846d5954 Miquel Raynal 2021-01-21  191  	struct work_struct hj_work;
dd3c52846d5954 Miquel Raynal 2021-01-21  192  	struct work_struct ibi_work;
dd3c52846d5954 Miquel Raynal 2021-01-21  193  	int irq;
dd3c52846d5954 Miquel Raynal 2021-01-21  194  	struct clk *pclk;
dd3c52846d5954 Miquel Raynal 2021-01-21  195  	struct clk *fclk;
dd3c52846d5954 Miquel Raynal 2021-01-21  196  	struct clk *sclk;
dd3c52846d5954 Miquel Raynal 2021-01-21  197  	struct {
dd3c52846d5954 Miquel Raynal 2021-01-21  198  		struct list_head list;
dd3c52846d5954 Miquel Raynal 2021-01-21  199  		struct svc_i3c_xfer *cur;
dd3c52846d5954 Miquel Raynal 2021-01-21  200  		/* Prevent races between transfers */
dd3c52846d5954 Miquel Raynal 2021-01-21  201  		spinlock_t lock;
dd3c52846d5954 Miquel Raynal 2021-01-21  202  	} xferqueue;
dd3c52846d5954 Miquel Raynal 2021-01-21  203  	struct {
dd3c52846d5954 Miquel Raynal 2021-01-21  204  		unsigned int num_slots;
dd3c52846d5954 Miquel Raynal 2021-01-21  205  		struct i3c_dev_desc **slots;
dd3c52846d5954 Miquel Raynal 2021-01-21  206  		struct i3c_ibi_slot *tbq_slot;
dd3c52846d5954 Miquel Raynal 2021-01-21  207  		/* Prevent races within IBI handlers */
dd3c52846d5954 Miquel Raynal 2021-01-21  208  		spinlock_t lock;
dd3c52846d5954 Miquel Raynal 2021-01-21  209  	} ibi;
85bebb554b09b1 Frank Li      2023-10-16  210  	struct mutex lock;
e00571e38f466c Frank Li      2023-10-18  211  	int enabled_events;
dd3c52846d5954 Miquel Raynal 2021-01-21 @212  };
dd3c52846d5954 Miquel Raynal 2021-01-21  213  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 3/6] i3c: add actual_len in i3c_priv_xfer

[Frank Li]

In MIPI I3C Specification:

"Ninth Bit of SDR Target Returned (Read) Data as End-of-Data: In I2C, the
ninth Data bit from Target to Controller is an ACK by the Controller. By
contrast, in I3C this bit allows the Target to end a Read, and allows the
Controller to Abort a Read. In SDR terms, the ninth bit of Read data is
referred to as the T-Bit (for ‘Transition’)"

I3C allow devices early terminate data transfer. So need "actual_len" field
to indicate how much get by i3c_priv_xfer.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 include/linux/i3c/device.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index 90fa83464f003..ef6217da8253b 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -54,6 +54,7 @@ enum i3c_hdr_mode {
  * struct i3c_priv_xfer - I3C SDR private transfer
  * @rnw: encodes the transfer direction. true for a read, false for a write
  * @len: transfer length in bytes of the transfer
+ * @actual_len: actual length in bytes are transferred by the controller
  * @data: input/output buffer
  * @data.in: input buffer. Must point to a DMA-able buffer
  * @data.out: output buffer. Must point to a DMA-able buffer
@@ -62,6 +63,7 @@ enum i3c_hdr_mode {
 struct i3c_priv_xfer {
 	u8 rnw;
 	u16 len;
+	u16 actual_len;
 	union {
 		void *in;
 		const void *out;
-- 
2.34.1

[PATCH v2 4/6] i3c: svc: rename read_len as actual_len

[Frank Li]

I3C transfer (SDR), target can early terminate read transfer.
I3C transfer (HDR), target can end write transfer.
I2C transfer, target can NACK write transfer.

'actual_len' is better name than 'read_len'.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index d8467607602af..6cd4cab26141c 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -137,7 +137,7 @@ struct svc_i3c_cmd {
 	u8 *in;
 	const void *out;
 	unsigned int len;
-	unsigned int read_len;
+	unsigned int actual_len;
 	bool continued;
 };
 
@@ -1029,7 +1029,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
 static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 			       bool rnw, unsigned int xfer_type, u8 addr,
 			       u8 *in, const u8 *out, unsigned int xfer_len,
-			       unsigned int *read_len, bool continued)
+			       unsigned int *actual_len, bool continued)
 {
 	u32 reg;
 	int ret;
@@ -1042,7 +1042,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 	       SVC_I3C_MCTRL_IBIRESP_NACK |
 	       SVC_I3C_MCTRL_DIR(rnw) |
 	       SVC_I3C_MCTRL_ADDR(addr) |
-	       SVC_I3C_MCTRL_RDTERM(*read_len),
+	       SVC_I3C_MCTRL_RDTERM(*actual_len),
 	       master->regs + SVC_I3C_MCTRL);
 
 	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
@@ -1080,7 +1080,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		goto emit_stop;
 
 	if (rnw)
-		*read_len = ret;
+		*actual_len = ret;
 
 	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
 				 SVC_I3C_MSTATUS_COMPLETE(reg), 0, 1000);
@@ -1162,7 +1162,7 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
 
 		ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
 					  cmd->addr, cmd->in, cmd->out,
-					  cmd->len, &cmd->read_len,
+					  cmd->len, &cmd->actual_len,
 					  cmd->continued);
 		if (ret)
 			break;
@@ -1248,7 +1248,7 @@ static int svc_i3c_master_send_bdcast_ccc_cmd(struct svc_i3c_master *master,
 	cmd->in = NULL;
 	cmd->out = buf;
 	cmd->len = xfer_len;
-	cmd->read_len = 0;
+	cmd->actual_len = 0;
 	cmd->continued = false;
 
 	mutex_lock(&master->lock);
@@ -1268,7 +1268,7 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 					      struct i3c_ccc_cmd *ccc)
 {
 	unsigned int xfer_len = ccc->dests[0].payload.len;
-	unsigned int read_len = ccc->rnw ? xfer_len : 0;
+	unsigned int actual_len = ccc->rnw ? xfer_len : 0;
 	struct svc_i3c_xfer *xfer;
 	struct svc_i3c_cmd *cmd;
 	int ret;
@@ -1286,7 +1286,7 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 	cmd->in = NULL;
 	cmd->out = &ccc->id;
 	cmd->len = 1;
-	cmd->read_len = 0;
+	cmd->actual_len = 0;
 	cmd->continued = true;
 
 	/* Directed message */
@@ -1296,7 +1296,7 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 	cmd->in = ccc->rnw ? ccc->dests[0].payload.data : NULL;
 	cmd->out = ccc->rnw ? NULL : ccc->dests[0].payload.data,
 	cmd->len = xfer_len;
-	cmd->read_len = read_len;
+	cmd->actual_len = actual_len;
 	cmd->continued = false;
 
 	mutex_lock(&master->lock);
@@ -1305,8 +1305,8 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 		svc_i3c_master_dequeue_xfer(master, xfer);
 	mutex_unlock(&master->lock);
 
-	if (cmd->read_len != xfer_len)
-		ccc->dests[0].payload.len = cmd->read_len;
+	if (cmd->actual_len != xfer_len)
+		ccc->dests[0].payload.len = cmd->actual_len;
 
 	ret = xfer->ret;
 	svc_i3c_master_free_xfer(xfer);
@@ -1356,7 +1356,7 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
 		cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL;
 		cmd->out = xfers[i].rnw ? NULL : xfers[i].data.out;
 		cmd->len = xfers[i].len;
-		cmd->read_len = xfers[i].rnw ? xfers[i].len : 0;
+		cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0;
 		cmd->continued = (i + 1) < nxfers;
 	}
 
@@ -1396,7 +1396,7 @@ static int svc_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
 		cmd->in = cmd->rnw ? xfers[i].buf : NULL;
 		cmd->out = cmd->rnw ? NULL : xfers[i].buf;
 		cmd->len = xfers[i].len;
-		cmd->read_len = cmd->rnw ? xfers[i].len : 0;
+		cmd->actual_len = cmd->rnw ? xfers[i].len : 0;
 		cmd->continued = (i + 1 < nxfers);
 	}
 
-- 
2.34.1

[PATCH v2 5/6] i3c: master: svc return actual transfer data len

[Frank Li]

I3C allow devices early terminate data transfer. So set "actual_len" to
indicate how much data get by i3c_priv_xfer.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 6cd4cab26141c..17e953a47b1bb 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -138,6 +138,7 @@ struct svc_i3c_cmd {
 	const void *out;
 	unsigned int len;
 	unsigned int actual_len;
+	struct i3c_priv_xfer *xfer;
 	bool continued;
 };
 
@@ -1052,6 +1053,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 
 	if (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_NACK) {
 		ret = -ENXIO;
+		*actual_len = 0;
 		goto emit_stop;
 	}
 
@@ -1069,6 +1071,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 	 */
 	if (SVC_I3C_MSTATUS_IBIWON(reg)) {
 		ret = -ENXIO;
+		*actual_len = 0;
 		goto emit_stop;
 	}
 
@@ -1164,6 +1167,10 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
 					  cmd->addr, cmd->in, cmd->out,
 					  cmd->len, &cmd->actual_len,
 					  cmd->continued);
+		/* cmd->xfer is NULL if I2C or CCC transfer */
+		if (cmd->xfer)
+			cmd->xfer->actual_len = cmd->actual_len;
+
 		if (ret)
 			break;
 	}
@@ -1351,6 +1358,7 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
 	for (i = 0; i < nxfers; i++) {
 		struct svc_i3c_cmd *cmd = &xfer->cmds[i];
 
+		cmd->xfer = &xfers[i];
 		cmd->addr = master->addrs[data->index];
 		cmd->rnw = xfers[i].rnw;
 		cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL;
-- 
2.34.1

[PATCH v2 6/6] i3c: add API i3c_dev_gettstatus_format1() to get target device status

[Frank Li]

I3C standard 5.1.9.3.15 Get Device Status (GETSTATUS):
Get request for one I3C Target Device to return its current status.

Add API to fetch it with format1.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/device.c       | 24 ++++++++++++++++++++++++
 drivers/i3c/internals.h    |  1 +
 drivers/i3c/master.c       | 26 ++++++++++++++++++++++++++
 include/linux/i3c/device.h |  1 +
 4 files changed, 52 insertions(+)

diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
index 1a6a8703dbc3a..aa26cf50ab9c6 100644
--- a/drivers/i3c/device.c
+++ b/drivers/i3c/device.c
@@ -196,6 +196,30 @@ void i3c_device_free_ibi(struct i3c_device *dev)
 }
 EXPORT_SYMBOL_GPL(i3c_device_free_ibi);
 
+/**
+ * i3c_device_getstatus_format1() - Get device status with format 1.
+ * @dev: device for which you want to get status.
+ * @status: I3C status format 1
+ *
+ * Return: 0 in case of success, a negative error core otherwise.
+ */
+int i3c_device_getstatus_format1(struct i3c_device *dev, u16 *status)
+{
+	int ret = -EINVAL;
+
+	if (!status)
+		return -EINVAL;
+
+	i3c_bus_normaluse_lock(dev->bus);
+	if (dev->desc)
+		ret = i3c_dev_getstatus_format1_locked(dev->desc, status);
+
+	i3c_bus_normaluse_unlock(dev->bus);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_device_getstatus_format1);
+
 /**
  * i3cdev_to_dev() - Returns the device embedded in @i3cdev
  * @i3cdev: I3C device
diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 908a807badaf9..976ad26ca79c2 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -24,4 +24,5 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
 int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
 			       const struct i3c_ibi_setup *req);
 void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
+int i3c_dev_getstatus_format1_locked(struct i3c_dev_desc *dev, u16 *status);
 #endif /* I3C_INTERNAL_H */
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index ed5e27cd20811..6a16ebdd180b5 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2924,6 +2924,32 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
 	dev->ibi = NULL;
 }
 
+int i3c_dev_getstatus_format1_locked(struct i3c_dev_desc *dev, u16 *status)
+{
+	struct i3c_master_controller *master = i3c_dev_get_master(dev);
+	struct i3c_ccc_getstatus *format1;
+	struct i3c_ccc_cmd_dest dest;
+	struct i3c_ccc_cmd cmd;
+	int ret;
+
+	format1 = i3c_ccc_cmd_dest_init(&dest, dev->info.dyn_addr, sizeof(*format1));
+	if (!format1)
+		return -ENOMEM;
+
+	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETSTATUS, &dest, 1);
+
+	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+	if (ret)
+		goto out;
+
+	*status = be16_to_cpu(format1->status);
+
+out:
+	i3c_ccc_cmd_dest_cleanup(&dest);
+
+	return ret;
+}
+
 static int __init i3c_init(void)
 {
 	int res;
diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index ef6217da8253b..5f511bd400f11 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -345,4 +345,5 @@ void i3c_device_free_ibi(struct i3c_device *dev);
 int i3c_device_enable_ibi(struct i3c_device *dev);
 int i3c_device_disable_ibi(struct i3c_device *dev);
 
+int i3c_device_getstatus_format1(struct i3c_device *dev, u16 *status);
 #endif /* I3C_DEV_H */
-- 
2.34.1