1. Patchew
  2. linux
  3. [PATCH v2 0/6] Introduce initial support for the AMD I3C (non-HCI) to DW driver
  4. View patch

[PATCH v2 5/6] i3c: master: Add support for SETAASA CCC

Shyam Sundar S K posted 6 patches 1 month ago
There is a newer version of this series
[PATCH v2 5/6] i3c: master: Add support for SETAASA CCC
Posted by Shyam Sundar S K 1 month ago 
I3C devices like DIMMs over SPD use SETAASA for bus discovery instead of
SETDASA. Add a new routine for I3C host controller drivers to use. If the
I3C slave on the bus is an SPD device, skip the regular DAA process.

According to the SPD spec[1], use SETAASA for bus discovery, and avoid
sending RSTDAA and DISEC, as they are considered illegal. Skip this entire
process if the slave is SPD-compliant, as indicated by the "jdec_spd" flag
from the BIOS.

[1] https://www.jedec.org/system/files/docs/JESD300-5B.01.pdf
(section 2.4 and 2.6.3)

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master.c               | 32 +++++++++++++++++++++++++++++-
 drivers/i3c/master/dw-i3c-master.c |  1 +
 include/linux/i3c/ccc.h            |  1 +
 include/linux/i3c/master.h         |  1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index ba6f17cb8aa6..1596efd6d82a 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1657,6 +1657,21 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 	}
 }
 
+static int i3c_master_setaasa_locked(struct i3c_master_controller *master)
+{
+	struct i3c_ccc_cmd_dest dest;
+	struct i3c_ccc_cmd cmd;
+	int ret;
+
+	i3c_ccc_cmd_dest_init(&dest, I3C_BROADCAST_ADDR, 0);
+	i3c_ccc_cmd_init(&cmd, false, I3C_CCC_SETAASA, &dest, 1);
+
+	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+	i3c_ccc_cmd_dest_cleanup(&dest);
+
+	return ret;
+}
+
 static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
 				  struct i3c_dev_boardinfo *boardinfo)
 {
@@ -1684,6 +1699,10 @@ static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
 		i3cdev->info.pid = i3cdev->boardinfo->pid;
 		i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
 
+		ret = i3c_master_setaasa_locked(master);
+		if (ret)
+			goto err_free_dev;
+
 		i3c_bus_normaluse_lock(&master->bus);
 		i3c_master_register_new_i3c_devs(master);
 		i3c_bus_normaluse_unlock(&master->bus);
@@ -1907,7 +1926,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 		goto err_bus_cleanup;
 	}
 
-	i3c_master_add_spd_dev(master, i3cboardinfo);
+	/*
+	 * If the I3C slave on the bus is SPD device, then do not follow the regular
+	 * DAA process. Also, as per SPD spec SETAASA is required for the bus discovery
+	 * and sending RSTDAA and DISEC is considered as illegal. So skip the entire process
+	 * if the jdec_spd flag has been identified from the BIOS.
+	 */
+	if (master->jdec_spd)
+		return i3c_master_add_spd_dev(master, i3cboardinfo);
 
 	if (master->ops->set_speed) {
 		ret = master->ops->set_speed(master, I3C_OPEN_DRAIN_SLOW_SPEED);
@@ -2311,6 +2337,10 @@ static int i3c_acpi_configure_master(struct i3c_master_controller *master)
 		return -ENODEV;
 	}
 
+	status = acpi_evaluate_object(master->ahandle, "_STR", NULL, NULL);
+	if (ACPI_SUCCESS(status))
+		master->jdec_spd = true;
+
 	num_dev = device_get_child_node_count(dev);
 	if (!num_dev) {
 		dev_err(&master->dev, "Error: no child node present\n");
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index f683e2a398ad..90a43209e55e 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -282,6 +282,7 @@ static bool dw_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
 	case I3C_CCC_GETSTATUS:
 	case I3C_CCC_GETMXDS:
 	case I3C_CCC_GETHDRCAP:
+	case I3C_CCC_SETAASA:
 		return true;
 	default:
 		return false;
diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
index ad59a4ae60d1..a145d766ab6f 100644
--- a/include/linux/i3c/ccc.h
+++ b/include/linux/i3c/ccc.h
@@ -32,6 +32,7 @@
 #define I3C_CCC_DEFSLVS			I3C_CCC_ID(0x8, true)
 #define I3C_CCC_ENTTM			I3C_CCC_ID(0xb, true)
 #define I3C_CCC_ENTHDR(x)		I3C_CCC_ID(0x20 + (x), true)
+#define I3C_CCC_SETAASA			I3C_CCC_ID(0x29, true)
 
 /* Unicast-only commands */
 #define I3C_CCC_SETDASA			I3C_CCC_ID(0x7, false)
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 367faf7c4bf3..cd8390d8b469 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -516,6 +516,7 @@ struct i3c_master_controller {
 	const struct i3c_master_controller_ops *ops;
 	unsigned int secondary : 1;
 	unsigned int init_done : 1;
+	unsigned int jdec_spd : 1;
 	unsigned int hotjoin: 1;
 	struct {
 		struct list_head i3c;
-- 
2.34.1
Re: [PATCH v2 5/6] i3c: master: Add support for SETAASA CCC
Posted by Jarkko Nikula 3 weeks ago 
On 10/23/24 8:51 AM, Shyam Sundar S K wrote:
> I3C devices like DIMMs over SPD use SETAASA for bus discovery instead of
> SETDASA. Add a new routine for I3C host controller drivers to use. If the
> I3C slave on the bus is an SPD device, skip the regular DAA process.
> 
> According to the SPD spec[1], use SETAASA for bus discovery, and avoid
> sending RSTDAA and DISEC, as they are considered illegal. Skip this entire
> process if the slave is SPD-compliant, as indicated by the "jdec_spd" flag
> from the BIOS.
> 
> [1] https://www.jedec.org/system/files/docs/JESD300-5B.01.pdf
> (section 2.4 and 2.6.3)
> 
SETAASA seems to come in MIPI v1.1.1 specification. I think worth to 
mention in commit log.

> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/i3c/master.c               | 32 +++++++++++++++++++++++++++++-
>   drivers/i3c/master/dw-i3c-master.c |  1 +
>   include/linux/i3c/ccc.h            |  1 +
>   include/linux/i3c/master.h         |  1 +
>   4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index ba6f17cb8aa6..1596efd6d82a 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1657,6 +1657,21 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>   	}
>   }
>   
> +static int i3c_master_setaasa_locked(struct i3c_master_controller *master)
> +{
> +	struct i3c_ccc_cmd_dest dest;
> +	struct i3c_ccc_cmd cmd;
> +	int ret;
> +
> +	i3c_ccc_cmd_dest_init(&dest, I3C_BROADCAST_ADDR, 0);
> +	i3c_ccc_cmd_init(&cmd, false, I3C_CCC_SETAASA, &dest, 1);
> +
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +
> +	return ret;
> +}
> +
>   static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
>   				  struct i3c_dev_boardinfo *boardinfo)
>   {
> @@ -1684,6 +1699,10 @@ static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
>   		i3cdev->info.pid = i3cdev->boardinfo->pid;
>   		i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
>   
> +		ret = i3c_master_setaasa_locked(master);
> +		if (ret)
> +			goto err_free_dev;
> +
>   		i3c_bus_normaluse_lock(&master->bus);
>   		i3c_master_register_new_i3c_devs(master);
>   		i3c_bus_normaluse_unlock(&master->bus);
> @@ -1907,7 +1926,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>   		goto err_bus_cleanup;
>   	}
>   
> -	i3c_master_add_spd_dev(master, i3cboardinfo);
> +	/*
> +	 * If the I3C slave on the bus is SPD device, then do not follow the regular
> +	 * DAA process. Also, as per SPD spec SETAASA is required for the bus discovery
> +	 * and sending RSTDAA and DISEC is considered as illegal. So skip the entire process
> +	 * if the jdec_spd flag has been identified from the BIOS.
> +	 */
> +	if (master->jdec_spd)
> +		return i3c_master_add_spd_dev(master, i3cboardinfo);
>   
>   	if (master->ops->set_speed) {
>   		ret = master->ops->set_speed(master, I3C_OPEN_DRAIN_SLOW_SPEED);
> @@ -2311,6 +2337,10 @@ static int i3c_acpi_configure_master(struct i3c_master_controller *master)
>   		return -ENODEV;
>   	}
>   
> +	status = acpi_evaluate_object(master->ahandle, "_STR", NULL, NULL);
> +	if (ACPI_SUCCESS(status))
> +		master->jdec_spd = true;
> +

Am I right "_STR" object should carry a string "jdec_spd"? But this code 
is not actually checking it, only the existence of _STR?

>   	num_dev = device_get_child_node_count(dev);
>   	if (!num_dev) {
>   		dev_err(&master->dev, "Error: no child node present\n");
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index f683e2a398ad..90a43209e55e 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -282,6 +282,7 @@ static bool dw_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
>   	case I3C_CCC_GETSTATUS:
>   	case I3C_CCC_GETMXDS:
>   	case I3C_CCC_GETHDRCAP:
> +	case I3C_CCC_SETAASA:
>   		return true;
>   	default:
>   		return false;
> diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
> index ad59a4ae60d1..a145d766ab6f 100644
> --- a/include/linux/i3c/ccc.h
> +++ b/include/linux/i3c/ccc.h
> @@ -32,6 +32,7 @@
>   #define I3C_CCC_DEFSLVS			I3C_CCC_ID(0x8, true)
>   #define I3C_CCC_ENTTM			I3C_CCC_ID(0xb, true)
>   #define I3C_CCC_ENTHDR(x)		I3C_CCC_ID(0x20 + (x), true)
> +#define I3C_CCC_SETAASA			I3C_CCC_ID(0x29, true)
>   
>   /* Unicast-only commands */
>   #define I3C_CCC_SETDASA			I3C_CCC_ID(0x7, false)
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 367faf7c4bf3..cd8390d8b469 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -516,6 +516,7 @@ struct i3c_master_controller {
>   	const struct i3c_master_controller_ops *ops;
>   	unsigned int secondary : 1;
>   	unsigned int init_done : 1;
> +	unsigned int jdec_spd : 1;
>   	unsigned int hotjoin: 1;
>   	struct {
>   		struct list_head i3c;
Re: [PATCH v2 5/6] i3c: master: Add support for SETAASA CCC
Posted by Shyam Sundar S K 2 weeks, 5 days ago 

On 11/4/2024 20:30, Jarkko Nikula wrote:
> On 10/23/24 8:51 AM, Shyam Sundar S K wrote:
>> I3C devices like DIMMs over SPD use SETAASA for bus discovery
>> instead of
>> SETDASA. Add a new routine for I3C host controller drivers to use.
>> If the
>> I3C slave on the bus is an SPD device, skip the regular DAA process.
>>
>> According to the SPD spec[1], use SETAASA for bus discovery, and avoid
>> sending RSTDAA and DISEC, as they are considered illegal. Skip this
>> entire
>> process if the slave is SPD-compliant, as indicated by the
>> "jdec_spd" flag
>> from the BIOS.
>>
>> [1] https://www.jedec.org/system/files/docs/JESD300-5B.01.pdf
>> (section 2.4 and 2.6.3)
>>
> SETAASA seems to come in MIPI v1.1.1 specification. I think worth to
> mention in commit log.
> 

OK. I will do it in the next version.

>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>   drivers/i3c/master.c               | 32
>> +++++++++++++++++++++++++++++-
>>   drivers/i3c/master/dw-i3c-master.c |  1 +
>>   include/linux/i3c/ccc.h            |  1 +
>>   include/linux/i3c/master.h         |  1 +
>>   4 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index ba6f17cb8aa6..1596efd6d82a 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -1657,6 +1657,21 @@ i3c_master_register_new_i3c_devs(struct
>> i3c_master_controller *master)
>>       }
>>   }
>>   +static int i3c_master_setaasa_locked(struct i3c_master_controller
>> *master)
>> +{
>> +    struct i3c_ccc_cmd_dest dest;
>> +    struct i3c_ccc_cmd cmd;
>> +    int ret;
>> +
>> +    i3c_ccc_cmd_dest_init(&dest, I3C_BROADCAST_ADDR, 0);
>> +    i3c_ccc_cmd_init(&cmd, false, I3C_CCC_SETAASA, &dest, 1);
>> +
>> +    ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
>> +    i3c_ccc_cmd_dest_cleanup(&dest);
>> +
>> +    return ret;
>> +}
>> +
>>   static int i3c_master_add_spd_dev(struct i3c_master_controller
>> *master,
>>                     struct i3c_dev_boardinfo *boardinfo)
>>   {
>> @@ -1684,6 +1699,10 @@ static int i3c_master_add_spd_dev(struct
>> i3c_master_controller *master,
>>           i3cdev->info.pid = i3cdev->boardinfo->pid;
>>           i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
>>   +        ret = i3c_master_setaasa_locked(master);
>> +        if (ret)
>> +            goto err_free_dev;
>> +
>>           i3c_bus_normaluse_lock(&master->bus);
>>           i3c_master_register_new_i3c_devs(master);
>>           i3c_bus_normaluse_unlock(&master->bus);
>> @@ -1907,7 +1926,14 @@ static int i3c_master_bus_init(struct
>> i3c_master_controller *master)
>>           goto err_bus_cleanup;
>>       }
>>   -    i3c_master_add_spd_dev(master, i3cboardinfo);
>> +    /*
>> +     * If the I3C slave on the bus is SPD device, then do not
>> follow the regular
>> +     * DAA process. Also, as per SPD spec SETAASA is required for
>> the bus discovery
>> +     * and sending RSTDAA and DISEC is considered as illegal. So
>> skip the entire process
>> +     * if the jdec_spd flag has been identified from the BIOS.
>> +     */
>> +    if (master->jdec_spd)
>> +        return i3c_master_add_spd_dev(master, i3cboardinfo);
>>         if (master->ops->set_speed) {
>>           ret = master->ops->set_speed(master,
>> I3C_OPEN_DRAIN_SLOW_SPEED);
>> @@ -2311,6 +2337,10 @@ static int i3c_acpi_configure_master(struct
>> i3c_master_controller *master)
>>           return -ENODEV;
>>       }
>>   +    status = acpi_evaluate_object(master->ahandle, "_STR", NULL,
>> NULL);
>> +    if (ACPI_SUCCESS(status))
>> +        master->jdec_spd = true;
>> +
> 
> Am I right "_STR" object should carry a string "jdec_spd"? But this
> code is not actually checking it, only the existence of _STR?
> 

I don't think so.. its just the string SPD/DIMM.

Scope (\_SB.I3CA)
{
	Name (_STR, Unicode ("SPD/DIMM"))  // _STR: Description String
	Name (_DSD, Package (0x04)  // _DSD: Device-Specific Data
	{
		...
	})
	Name (MST0, Package (0x02)
	{
		...
	})
	Device (SPD0)
	{
		...
	}

	Device (PMI0)
	{
		...
	}

	Device (RCD0)
	{
		...
	}
}

Thanks,
Shyam

>>       num_dev = device_get_child_node_count(dev);
>>       if (!num_dev) {
>>           dev_err(&master->dev, "Error: no child node present\n");
>> diff --git a/drivers/i3c/master/dw-i3c-master.c
>> b/drivers/i3c/master/dw-i3c-master.c
>> index f683e2a398ad..90a43209e55e 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> @@ -282,6 +282,7 @@ static bool
>> dw_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
>>       case I3C_CCC_GETSTATUS:
>>       case I3C_CCC_GETMXDS:
>>       case I3C_CCC_GETHDRCAP:
>> +    case I3C_CCC_SETAASA:
>>           return true;
>>       default:
>>           return false;
>> diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
>> index ad59a4ae60d1..a145d766ab6f 100644
>> --- a/include/linux/i3c/ccc.h
>> +++ b/include/linux/i3c/ccc.h
>> @@ -32,6 +32,7 @@
>>   #define I3C_CCC_DEFSLVS            I3C_CCC_ID(0x8, true)
>>   #define I3C_CCC_ENTTM            I3C_CCC_ID(0xb, true)
>>   #define I3C_CCC_ENTHDR(x)        I3C_CCC_ID(0x20 + (x), true)
>> +#define I3C_CCC_SETAASA            I3C_CCC_ID(0x29, true)
>>     /* Unicast-only commands */
>>   #define I3C_CCC_SETDASA            I3C_CCC_ID(0x7, false)
>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>> index 367faf7c4bf3..cd8390d8b469 100644
>> --- a/include/linux/i3c/master.h
>> +++ b/include/linux/i3c/master.h
>> @@ -516,6 +516,7 @@ struct i3c_master_controller {
>>       const struct i3c_master_controller_ops *ops;
>>       unsigned int secondary : 1;
>>       unsigned int init_done : 1;
>> +    unsigned int jdec_spd : 1;
>>       unsigned int hotjoin: 1;
>>       struct {
>>           struct list_head i3c;
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.