[v3,13/13] bus/fsl-mc: Add a new version for dprc_get_obj_region command
diff mbox series

Message ID 20200706124243.10697-14-diana.craciun@nxp.com
State New
Headers show
Series
  • bus/fsl-mc: Extend mc-bus driver functionalities in preparation for mc-bus VFIO support
Related show

Commit Message

Diana Craciun July 6, 2020, 12:42 p.m. UTC
From: Diana Craciun <diana.craciun@oss.nxp.com>

The region size reported by the firmware for mc and software
portals was less than allocated by the hardware. This may be
problematic when mmapping the region in user space because the
region size is less than page size. However the size as reserved
by the hardware is 64K.

Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com>
---
 drivers/bus/fsl-mc/dprc.c           | 38 ++++++++++++++++++-----------
 drivers/bus/fsl-mc/fsl-mc-private.h |  3 +++
 2 files changed, 27 insertions(+), 14 deletions(-)

Comments

Laurentiu Tudor July 6, 2020, 1:14 p.m. UTC | #1
On 7/6/2020 3:42 PM, Diana Craciun wrote:
> From: Diana Craciun <diana.craciun@oss.nxp.com>
> 
> The region size reported by the firmware for mc and software
> portals was less than allocated by the hardware. This may be
> problematic when mmapping the region in user space because the
> region size is less than page size. However the size as reserved
> by the hardware is 64K.

Should we also mention in the commit msg that this shows up when
compiling the kernel with 64K page size support, or it's obvious enough?

> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com>
> ---
>  drivers/bus/fsl-mc/dprc.c           | 38 ++++++++++++++++++-----------
>  drivers/bus/fsl-mc/fsl-mc-private.h |  3 +++
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c
> index 3f08752c2c19..ba292c56fe19 100644
> --- a/drivers/bus/fsl-mc/dprc.c
> +++ b/drivers/bus/fsl-mc/dprc.c
> @@ -536,20 +536,30 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io,
>  			return err;
>  	}
>  
> -	/**
> -	 * MC API version 6.3 introduced a new field to the region
> -	 * descriptor: base_address. If the older API is in use then the base
> -	 * address is set to zero to indicate it needs to be obtained elsewhere
> -	 * (typically the device tree).
> -	 */
> -	if (dprc_major_ver > 6 || (dprc_major_ver == 6 && dprc_minor_ver >= 3))
> -		cmd.header =
> -			mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V2,
> -					     cmd_flags, token);
> -	else
> -		cmd.header =
> -			mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG,
> -					     cmd_flags, token);
> +	if (dprc_major_ver > 6 || (dprc_major_ver == 6 && dprc_minor_ver >= 6)) {
> +		/**
> +		 * MC API version 6.6 changed the size of the MC portals and software
> +		 * portals to 64K (as implemented by hardware). If older API is in use the
> +		 * size reported is less (64 bytes for mc portals and 4K for software
> +		 * portals).
> +		 */

Here and below, there's no need to use kernel-doc style comments. And a
nit: there's an extra blank line here.

---
Best Regards, Laurentiu

> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V3,
> +						  cmd_flags, token);
> +
> +	} else if (dprc_major_ver == 6 && dprc_minor_ver >= 3) {
> +		/**
> +		 * MC API version 6.3 introduced a new field to the region
> +		 * descriptor: base_address. If the older API is in use then the base
> +		 * address is set to zero to indicate it needs to be obtained elsewhere
> +		 * (typically the device tree).
> +		 */
> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V2,
> +						  cmd_flags, token);
> +	} else {
> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG,
> +						  cmd_flags, token);
> +	}
>  
>  	cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params;
>  	cmd_params->obj_id = cpu_to_le32(obj_id);
> diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
> index e6fcff12c68d..8d65273a78d7 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-private.h
> +++ b/drivers/bus/fsl-mc/fsl-mc-private.h
> @@ -80,10 +80,12 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>  /* DPRC command versioning */
>  #define DPRC_CMD_BASE_VERSION			1
>  #define DPRC_CMD_2ND_VERSION			2
> +#define DPRC_CMD_3RD_VERSION			3
>  #define DPRC_CMD_ID_OFFSET			4
>  
>  #define DPRC_CMD(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_BASE_VERSION)
>  #define DPRC_CMD_V2(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_2ND_VERSION)
> +#define DPRC_CMD_V3(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_3RD_VERSION)
>  
>  /* DPRC command IDs */
>  #define DPRC_CMDID_CLOSE                        DPRC_CMD(0x800)
> @@ -105,6 +107,7 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>  #define DPRC_CMDID_GET_OBJ                      DPRC_CMD(0x15A)
>  #define DPRC_CMDID_GET_OBJ_REG                  DPRC_CMD(0x15E)
>  #define DPRC_CMDID_GET_OBJ_REG_V2               DPRC_CMD_V2(0x15E)
> +#define DPRC_CMDID_GET_OBJ_REG_V3               DPRC_CMD_V3(0x15E)
>  #define DPRC_CMDID_SET_OBJ_IRQ                  DPRC_CMD(0x15F)
>  
>  #define DPRC_CMDID_GET_CONNECTION               DPRC_CMD(0x16C)
>
Diana Craciun OSS July 6, 2020, 3:49 p.m. UTC | #2
On 7/6/2020 4:14 PM, Laurentiu Tudor wrote:
>
> On 7/6/2020 3:42 PM, Diana Craciun wrote:
>> From: Diana Craciun <diana.craciun@oss.nxp.com>
>>
>> The region size reported by the firmware for mc and software
>> portals was less than allocated by the hardware. This may be
>> problematic when mmapping the region in user space because the
>> region size is less than page size. However the size as reserved
>> by the hardware is 64K.
> Should we also mention in the commit msg that this shows up when
> compiling the kernel with 64K page size support, or it's obvious enough?

It's not that obvious. In case of 4K worked by chance because the 
reported region size was 4K. Maybe it worth mentioning.

>
>> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com>
>> ---
>>   drivers/bus/fsl-mc/dprc.c           | 38 ++++++++++++++++++-----------
>>   drivers/bus/fsl-mc/fsl-mc-private.h |  3 +++
>>   2 files changed, 27 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c
>> index 3f08752c2c19..ba292c56fe19 100644
>> --- a/drivers/bus/fsl-mc/dprc.c
>> +++ b/drivers/bus/fsl-mc/dprc.c
>> @@ -536,20 +536,30 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io,
>>   			return err;
>>   	}
>>   
>> -	/**
>> -	 * MC API version 6.3 introduced a new field to the region
>> -	 * descriptor: base_address. If the older API is in use then the base
>> -	 * address is set to zero to indicate it needs to be obtained elsewhere
>> -	 * (typically the device tree).
>> -	 */
>> -	if (dprc_major_ver > 6 || (dprc_major_ver == 6 && dprc_minor_ver >= 3))
>> -		cmd.header =
>> -			mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V2,
>> -					     cmd_flags, token);
>> -	else
>> -		cmd.header =
>> -			mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG,
>> -					     cmd_flags, token);
>> +	if (dprc_major_ver > 6 || (dprc_major_ver == 6 && dprc_minor_ver >= 6)) {
>> +		/**
>> +		 * MC API version 6.6 changed the size of the MC portals and software
>> +		 * portals to 64K (as implemented by hardware). If older API is in use the
>> +		 * size reported is less (64 bytes for mc portals and 4K for software
>> +		 * portals).
>> +		 */
> Here and below, there's no need to use kernel-doc style comments. And a
> nit: there's an extra blank line here.
>
> ---
> Best Regards, Laurentiu
>
>> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V3,
>> +						  cmd_flags, token);
>> +
>> +	} else if (dprc_major_ver == 6 && dprc_minor_ver >= 3) {
>> +		/**
>> +		 * MC API version 6.3 introduced a new field to the region
>> +		 * descriptor: base_address. If the older API is in use then the base
>> +		 * address is set to zero to indicate it needs to be obtained elsewhere
>> +		 * (typically the device tree).
>> +		 */
>> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V2,
>> +						  cmd_flags, token);
>> +	} else {
>> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG,
>> +						  cmd_flags, token);
>> +	}
>>   
>>   	cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params;
>>   	cmd_params->obj_id = cpu_to_le32(obj_id);
>> diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
>> index e6fcff12c68d..8d65273a78d7 100644
>> --- a/drivers/bus/fsl-mc/fsl-mc-private.h
>> +++ b/drivers/bus/fsl-mc/fsl-mc-private.h
>> @@ -80,10 +80,12 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>>   /* DPRC command versioning */
>>   #define DPRC_CMD_BASE_VERSION			1
>>   #define DPRC_CMD_2ND_VERSION			2
>> +#define DPRC_CMD_3RD_VERSION			3
>>   #define DPRC_CMD_ID_OFFSET			4
>>   
>>   #define DPRC_CMD(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_BASE_VERSION)
>>   #define DPRC_CMD_V2(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_2ND_VERSION)
>> +#define DPRC_CMD_V3(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_3RD_VERSION)
>>   
>>   /* DPRC command IDs */
>>   #define DPRC_CMDID_CLOSE                        DPRC_CMD(0x800)
>> @@ -105,6 +107,7 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>>   #define DPRC_CMDID_GET_OBJ                      DPRC_CMD(0x15A)
>>   #define DPRC_CMDID_GET_OBJ_REG                  DPRC_CMD(0x15E)
>>   #define DPRC_CMDID_GET_OBJ_REG_V2               DPRC_CMD_V2(0x15E)
>> +#define DPRC_CMDID_GET_OBJ_REG_V3               DPRC_CMD_V3(0x15E)
>>   #define DPRC_CMDID_SET_OBJ_IRQ                  DPRC_CMD(0x15F)
>>   
>>   #define DPRC_CMDID_GET_CONNECTION               DPRC_CMD(0x16C)
>>

Patch
diff mbox series

diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c
index 3f08752c2c19..ba292c56fe19 100644
--- a/drivers/bus/fsl-mc/dprc.c
+++ b/drivers/bus/fsl-mc/dprc.c
@@ -536,20 +536,30 @@  int dprc_get_obj_region(struct fsl_mc_io *mc_io,
 			return err;
 	}
 
-	/**
-	 * MC API version 6.3 introduced a new field to the region
-	 * descriptor: base_address. If the older API is in use then the base
-	 * address is set to zero to indicate it needs to be obtained elsewhere
-	 * (typically the device tree).
-	 */
-	if (dprc_major_ver > 6 || (dprc_major_ver == 6 && dprc_minor_ver >= 3))
-		cmd.header =
-			mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V2,
-					     cmd_flags, token);
-	else
-		cmd.header =
-			mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG,
-					     cmd_flags, token);
+	if (dprc_major_ver > 6 || (dprc_major_ver == 6 && dprc_minor_ver >= 6)) {
+		/**
+		 * MC API version 6.6 changed the size of the MC portals and software
+		 * portals to 64K (as implemented by hardware). If older API is in use the
+		 * size reported is less (64 bytes for mc portals and 4K for software
+		 * portals).
+		 */
+
+		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V3,
+						  cmd_flags, token);
+
+	} else if (dprc_major_ver == 6 && dprc_minor_ver >= 3) {
+		/**
+		 * MC API version 6.3 introduced a new field to the region
+		 * descriptor: base_address. If the older API is in use then the base
+		 * address is set to zero to indicate it needs to be obtained elsewhere
+		 * (typically the device tree).
+		 */
+		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V2,
+						  cmd_flags, token);
+	} else {
+		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG,
+						  cmd_flags, token);
+	}
 
 	cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params;
 	cmd_params->obj_id = cpu_to_le32(obj_id);
diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
index e6fcff12c68d..8d65273a78d7 100644
--- a/drivers/bus/fsl-mc/fsl-mc-private.h
+++ b/drivers/bus/fsl-mc/fsl-mc-private.h
@@ -80,10 +80,12 @@  int dpmcp_reset(struct fsl_mc_io *mc_io,
 /* DPRC command versioning */
 #define DPRC_CMD_BASE_VERSION			1
 #define DPRC_CMD_2ND_VERSION			2
+#define DPRC_CMD_3RD_VERSION			3
 #define DPRC_CMD_ID_OFFSET			4
 
 #define DPRC_CMD(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_BASE_VERSION)
 #define DPRC_CMD_V2(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_2ND_VERSION)
+#define DPRC_CMD_V3(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_3RD_VERSION)
 
 /* DPRC command IDs */
 #define DPRC_CMDID_CLOSE                        DPRC_CMD(0x800)
@@ -105,6 +107,7 @@  int dpmcp_reset(struct fsl_mc_io *mc_io,
 #define DPRC_CMDID_GET_OBJ                      DPRC_CMD(0x15A)
 #define DPRC_CMDID_GET_OBJ_REG                  DPRC_CMD(0x15E)
 #define DPRC_CMDID_GET_OBJ_REG_V2               DPRC_CMD_V2(0x15E)
+#define DPRC_CMDID_GET_OBJ_REG_V3               DPRC_CMD_V3(0x15E)
 #define DPRC_CMDID_SET_OBJ_IRQ                  DPRC_CMD(0x15F)
 
 #define DPRC_CMDID_GET_CONNECTION               DPRC_CMD(0x16C)