Re: [PATCH v2 4/7] staging: fsl-mc: Upgraded MC bus driver to match MC fw 7.0.0

[Dan Carpenter <dan.carpenter@oracle.com>]

Sorry, I'm reviewing this patchset slowly.

On Wed, May 06, 2015 at 04:28:25PM -0500, J. German Rivera wrote:
> - Migrated MC bus driver to use DPRC API 0.6.
> - Changed IRQ setup infrastructure to be able to program MSIs
>   for MC objects in an object-independent way.
> 
> Signed-off-by: J. German Rivera <German.Rivera@freescale.com>
> ---
> Changes in v2:
> - Addressed comments from Dan Carpenter:
>   * Added #ifdef GIC_ITS_MC_SUPPORT's that had been removed
>     by mistake.
>   * Removed EXPORTs that are not used by other MC object drivers
>     yet.
>   * Removed unused function dprc_lookup_object()
> 
>  drivers/staging/fsl-mc/bus/dpmcp-cmd.h      |  79 --------------
>  drivers/staging/fsl-mc/bus/dprc-cmd.h       |   6 +-
>  drivers/staging/fsl-mc/bus/dprc-driver.c    |  37 +++++--
>  drivers/staging/fsl-mc/bus/dprc.c           | 155 ++++++++++++++++++++++------
>  drivers/staging/fsl-mc/bus/mc-allocator.c   |  26 +++--
>  drivers/staging/fsl-mc/bus/mc-bus.c         |  77 +++++++++-----
>  drivers/staging/fsl-mc/bus/mc-sys.c         |   6 +-
>  drivers/staging/fsl-mc/include/dpmng.h      |   4 +-
>  drivers/staging/fsl-mc/include/dprc.h       | 114 +++++++++++++++-----
>  drivers/staging/fsl-mc/include/mc-private.h |  29 +++++-
>  drivers/staging/fsl-mc/include/mc.h         |   4 +
>  11 files changed, 345 insertions(+), 192 deletions(-)
> 
> diff --git a/drivers/staging/fsl-mc/bus/dpmcp-cmd.h b/drivers/staging/fsl-mc/bus/dpmcp-cmd.h
> index 57f326b..62bdc18 100644
> --- a/drivers/staging/fsl-mc/bus/dpmcp-cmd.h
> +++ b/drivers/staging/fsl-mc/bus/dpmcp-cmd.h
> @@ -54,83 +54,4 @@
>  #define DPMCP_CMDID_GET_IRQ_STATUS			0x016
>  #define DPMCP_CMDID_CLEAR_IRQ_STATUS			0x017
> 
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_CREATE(cmd, cfg) \
> -	MC_CMD_OP(cmd, 0, 0,  32, int,      cfg->portal_id)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_SET_IRQ(cmd, irq_index, irq_addr, irq_val, user_irq_id) \
> -do { \
> -	MC_CMD_OP(cmd, 0, 0,  8,  uint8_t,  irq_index);\
> -	MC_CMD_OP(cmd, 0, 32, 32, uint32_t, irq_val);\
> -	MC_CMD_OP(cmd, 1, 0,  64, uint64_t, irq_addr); \
> -	MC_CMD_OP(cmd, 2, 0,  32, int,	    user_irq_id); \
> -} while (0)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_GET_IRQ(cmd, irq_index) \
> -	MC_CMD_OP(cmd, 0, 32, 8,  uint8_t,  irq_index)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_RSP_GET_IRQ(cmd, type, irq_addr, irq_val, user_irq_id) \
> -do { \
> -	MC_RSP_OP(cmd, 0, 0,  32, uint32_t, irq_val); \
> -	MC_RSP_OP(cmd, 1, 0,  64, uint64_t, irq_addr); \
> -	MC_RSP_OP(cmd, 2, 0,  32, int,	    user_irq_id); \
> -	MC_RSP_OP(cmd, 2, 32, 32, int,	    type); \
> -} while (0)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_SET_IRQ_ENABLE(cmd, irq_index, en) \
> -do { \
> -	MC_CMD_OP(cmd, 0, 0,  8,  uint8_t,  en); \
> -	MC_CMD_OP(cmd, 0, 32, 8,  uint8_t,  irq_index);\
> -} while (0)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_GET_IRQ_ENABLE(cmd, irq_index) \
> -	MC_CMD_OP(cmd, 0, 32, 8,  uint8_t,  irq_index)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_RSP_GET_IRQ_ENABLE(cmd, en) \
> -	MC_RSP_OP(cmd, 0, 0,  8,  uint8_t,  en)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_SET_IRQ_MASK(cmd, irq_index, mask) \
> -do { \
> -	MC_CMD_OP(cmd, 0, 0,  32, uint32_t, mask);\
> -	MC_CMD_OP(cmd, 0, 32, 8,  uint8_t,  irq_index);\
> -} while (0)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_GET_IRQ_MASK(cmd, irq_index) \
> -	MC_CMD_OP(cmd, 0, 32, 8,  uint8_t,  irq_index)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_RSP_GET_IRQ_MASK(cmd, mask) \
> -	MC_RSP_OP(cmd, 0, 0,  32, uint32_t, mask)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_GET_IRQ_STATUS(cmd, irq_index) \
> -	MC_CMD_OP(cmd, 0, 32, 8,  uint8_t,  irq_index)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_RSP_GET_IRQ_STATUS(cmd, status) \
> -	MC_RSP_OP(cmd, 0, 0,  32, uint32_t, status)
> -
> -/*                cmd, param, offset, width, type, arg_name */
> -#define DPMCP_CMD_CLEAR_IRQ_STATUS(cmd, irq_index, status) \
> -do { \
> -	MC_CMD_OP(cmd, 0, 0,  32, uint32_t, status); \
> -	MC_CMD_OP(cmd, 0, 32, 8,  uint8_t,  irq_index);\
> -} while (0)
> -
> -/*                cmd, param, offset, width, type,	arg_name */
> -#define DPMCP_RSP_GET_ATTRIBUTES(cmd, attr) \
> -do { \
> -	MC_RSP_OP(cmd, 0, 32, 32, int,	    attr->id);\
> -	MC_RSP_OP(cmd, 1, 0,  16, uint16_t, attr->version.major);\
> -	MC_RSP_OP(cmd, 1, 16, 16, uint16_t, attr->version.minor);\
> -} while (0)
> -
>  #endif /* _FSL_DPMCP_CMD_H */


These changes are not related to the rest.  They should be sent by
themselves as:

[patch] remove unused defines

> diff --git a/drivers/staging/fsl-mc/bus/dprc-cmd.h b/drivers/staging/fsl-mc/bus/dprc-cmd.h
> index 0920248..df5ad5f 100644
> --- a/drivers/staging/fsl-mc/bus/dprc-cmd.h
> +++ b/drivers/staging/fsl-mc/bus/dprc-cmd.h
> @@ -41,7 +41,7 @@
>  #define _FSL_DPRC_CMD_H
> 
>  /* DPRC Version */
> -#define DPRC_VER_MAJOR				3
> +#define DPRC_VER_MAJOR				4
>  #define DPRC_VER_MINOR				0
> 
>  /* Command IDs */
> @@ -72,12 +72,14 @@
>  #define DPRC_CMDID_GET_RES_COUNT		0x15B
>  #define DPRC_CMDID_GET_RES_IDS			0x15C
>  #define DPRC_CMDID_GET_OBJ_REG			0x15E
> +#define DPRC_CMDID_OBJ_SET_IRQ			0x15F
> +#define DPRC_CMDID_OBJ_GET_IRQ			0x160
> +#define DPRC_CMDID_SET_OBJ_LABEL		0x161
> 
>  #define DPRC_CMDID_CONNECT			0x167
>  #define DPRC_CMDID_DISCONNECT			0x168
>  #define DPRC_CMDID_GET_POOL			0x169
>  #define DPRC_CMDID_GET_POOL_COUNT		0x16A
> -#define DPRC_CMDID_GET_PORTAL_PADDR		0x16B
> 
>  #define DPRC_CMDID_GET_CONNECTION		0x16C
> 
> diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c b/drivers/staging/fsl-mc/bus/dprc-driver.c
> index 85e293b..338fd7d 100644
> --- a/drivers/staging/fsl-mc/bus/dprc-driver.c
> +++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
> @@ -500,6 +500,21 @@ static int register_dprc_irq_handlers(struct fsl_mc_device *mc_dev)
> 
>  	for (i = 0; i < ARRAY_SIZE(irq_handlers); i++) {
>  		irq = mc_dev->irqs[i];
> +
> +		if (WARN_ON(irq->dev_irq_index != i)) {
> +			error = -EINVAL;
> +			goto error_unregister_irq_handlers;


We added ->dev_irq_index and ->mc_dev to the irq pointer but this is the
only place where we use it.  This WARN_ON() can never trigger unless we
have memory corruption.

Anyway, since it's not connected to anything else in the patch we can
do it as a separate patch.

> +		}
> +
> +		/*
> +		 * NOTE: Normally, devm_request_threaded_irq() programs the MSI
> +		 * physically in the device (by invoking a device-specific
> +		 * callback). However, for MC IRQs, we have to program the MSI
> +		 * outside of this callback in an object-specific way, because
> +		 * the object-independent way of programming MSI is not reliable
> +		 * yet. For now, the MC callback just sets the msi_paddr and
> +		 * msi_value fields of the irq structure.
> +		 */
>  		error = devm_request_threaded_irq(&mc_dev->dev,
>  						  irq->irq_number,
>  						  irq_handlers[i].irq_handler,
> @@ -518,18 +533,17 @@ static int register_dprc_irq_handlers(struct fsl_mc_device *mc_dev)
> 
>  		/*
>  		 * Program the MSI (paddr, value) pair in the device:
> -		 *
> -		 * TODO: This needs to be moved to mc_bus_msi_domain_write_msg()
> -		 * when the MC object-independent dprc_set_irq() flib API
> -		 * becomes available
>  		 */
> -		error = dprc_set_irq(mc_dev->mc_io, mc_dev->mc_handle,
> -				     i, irq->msi_paddr,
> +		error = dprc_set_irq(mc_dev->mc_io,
> +				     mc_dev->mc_handle,
> +				     i,
> +				     irq->msi_paddr,
>  				     irq->msi_value,
>  				     irq->irq_number);
>  		if (error < 0) {
>  			dev_err(&mc_dev->dev,
> -				"mc_set_irq() failed: %d\n", error);
> +				"dprc_set_irq() failed for IRQ %u: %d\n",
> +				i, error);
>  			return error;
>  		}
>  	}

This chunk is basically white space cleanups.  It would be better to
fold it into patch 1/7 where the function was first introduced.

> @@ -663,15 +677,16 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
>  	 */
>  	error = dprc_setup_irqs(mc_dev);
>  	if (error < 0)
> -		goto error_cleanup_open;
> +		goto error_cleanup_dprc_scan;
> 
>  	dev_info(&mc_dev->dev, "DPRC device bound to driver");
>  	return 0;
> 
> -error_cleanup_open:
> -	if (mc_bus->irq_resources)
> -		fsl_mc_cleanup_irq_pool(mc_bus);
> +error_cleanup_dprc_scan:
> +	device_for_each_child(&mc_dev->dev, NULL, __fsl_mc_device_remove);

I assume this is a bug fix?  It would be better to fold this into patch
1/7 where the bug was introduced?

> +	fsl_mc_cleanup_irq_pool(mc_bus);
> 
> +error_cleanup_open:
>  	(void)dprc_close(mc_dev->mc_io, mc_dev->mc_handle);
> 
>  error_cleanup_mc_io:
> diff --git a/drivers/staging/fsl-mc/bus/dprc.c b/drivers/staging/fsl-mc/bus/dprc.c
> index 19b26e6..62087cc 100644
> --- a/drivers/staging/fsl-mc/bus/dprc.c
> +++ b/drivers/staging/fsl-mc/bus/dprc.c
> @@ -73,7 +73,7 @@ int dprc_create_container(struct fsl_mc_io *mc_io,
>  			  uint16_t token,
>  			  struct dprc_cfg *cfg,
>  			  int *child_container_id,
> -			  uint64_t *child_portal_paddr)
> +			  uint64_t *child_portal_offset)
>  {
>  	struct mc_command cmd = { 0 };
>  	int err;


The renaming of "paddr" to "offset" is just a white space change.  It's
really easy to review on its own but it makes reviewing difficult to
mix everything together.

[patch 2] rename paddr to offset

> @@ -82,6 +82,22 @@ int dprc_create_container(struct fsl_mc_io *mc_io,
>  	cmd.params[0] |= mc_enc(32, 16, cfg->icid);
>  	cmd.params[0] |= mc_enc(0, 32, cfg->options);
>  	cmd.params[1] |= mc_enc(32, 32, cfg->portal_id);
> +	cmd.params[2] |= mc_enc(0, 8, cfg->label[0]);
> +	cmd.params[2] |= mc_enc(8, 8, cfg->label[1]);
> +	cmd.params[2] |= mc_enc(16, 8, cfg->label[2]);
> +	cmd.params[2] |= mc_enc(24, 8, cfg->label[3]);
> +	cmd.params[2] |= mc_enc(32, 8, cfg->label[4]);
> +	cmd.params[2] |= mc_enc(40, 8, cfg->label[5]);
> +	cmd.params[2] |= mc_enc(48, 8, cfg->label[6]);
> +	cmd.params[2] |= mc_enc(56, 8, cfg->label[7]);
> +	cmd.params[3] |= mc_enc(0, 8, cfg->label[8]);
> +	cmd.params[3] |= mc_enc(8, 8, cfg->label[9]);
> +	cmd.params[3] |= mc_enc(16, 8, cfg->label[10]);
> +	cmd.params[3] |= mc_enc(24, 8, cfg->label[11]);
> +	cmd.params[3] |= mc_enc(32, 8, cfg->label[12]);
> +	cmd.params[3] |= mc_enc(40, 8, cfg->label[13]);
> +	cmd.params[3] |= mc_enc(48, 8, cfg->label[14]);
> +	cmd.params[3] |= mc_enc(56, 8, cfg->label[15]);
> 
>  	cmd.header = mc_encode_cmd_header(DPRC_CMDID_CREATE_CONT,
>  					  MC_CMD_PRI_LOW, token);
> @@ -93,7 +109,7 @@ int dprc_create_container(struct fsl_mc_io *mc_io,
> 
>  	/* retrieve response parameters */
>  	*child_container_id = mc_dec(cmd.params[1], 0, 32);
> -	*child_portal_paddr = mc_dec(cmd.params[2], 0, 64);
> +	*child_portal_offset = mc_dec(cmd.params[2], 0, 64);
> 
>  	return 0;
>  }
> @@ -159,6 +175,39 @@ int dprc_get_irq(struct fsl_mc_io *mc_io,
>  	return 0;
>  }
> 
> +int dprc_obj_get_irq(struct fsl_mc_io *mc_io,
> +		     uint16_t token,
> +		     int obj_index,
> +		     uint8_t irq_index,
> +		     int *type,
> +		     uint64_t *irq_addr,
> +		     uint32_t *irq_val,
> +		     int *user_irq_id)
> +{
> +	struct mc_command cmd = { 0 };
> +	int err;
> +
> +	/* prepare command */
> +	cmd.header = mc_encode_cmd_header(DPRC_CMDID_OBJ_GET_IRQ,
> +					  MC_CMD_PRI_LOW,
> +					  token);
> +
> +	cmd.params[0] |= mc_enc(0, 32, obj_index);
> +	cmd.params[0] |= mc_enc(32, 8, irq_index);
> +
> +	/* send command to mc*/
> +	err = mc_send_command(mc_io, &cmd);
> +	if (err)
> +		return err;
> +
> +	/* retrieve response parameters */
> +	*irq_val = mc_dec(cmd.params[0], 0, 32);
> +	*irq_addr = mc_dec(cmd.params[1], 0, 64);
> +	*user_irq_id = mc_dec(cmd.params[2], 0, 32);
> +	*type = mc_dec(cmd.params[2], 32, 32);
> +	return 0;
> +}
> +

This function is never called.  I checked the later functions and it's
not called in those either.  Drop it until we have a user.

>  int dprc_set_irq(struct fsl_mc_io *mc_io,
>  		 uint16_t token,
>  		 uint8_t irq_index,
> @@ -181,6 +230,31 @@ int dprc_set_irq(struct fsl_mc_io *mc_io,
>  	return mc_send_command(mc_io, &cmd);
>  }
> 
> +int dprc_obj_set_irq(struct fsl_mc_io *mc_io,
> +		     uint16_t token,
> +		     int obj_index,
> +		     uint8_t irq_index,
> +		     uint64_t irq_addr,
> +		     uint32_t irq_val,
> +		     int user_irq_id)
> +{
> +	struct mc_command cmd = { 0 };
> +
> +	/* prepare command */
> +	cmd.header = mc_encode_cmd_header(DPRC_CMDID_OBJ_SET_IRQ,
> +					  MC_CMD_PRI_LOW,
> +					  token);
> +
> +	cmd.params[0] |= mc_enc(32, 8, irq_index);
> +	cmd.params[0] |= mc_enc(0, 32, irq_val);
> +	cmd.params[1] |= mc_enc(0, 64, irq_addr);
> +	cmd.params[2] |= mc_enc(0, 32, user_irq_id);
> +	cmd.params[2] |= mc_enc(32, 32, obj_index);
> +
> +	/* send command to mc*/
> +	return mc_send_command(mc_io, &cmd);
> +}
> +
>  int dprc_get_irq_enable(struct fsl_mc_io *mc_io,
>  			uint16_t token,
>  			uint8_t irq_index,
> @@ -604,7 +678,22 @@ int dprc_get_obj(struct fsl_mc_io *mc_io,
>  	obj_desc->type[13] = mc_dec(cmd.params[4], 40, 8);
>  	obj_desc->type[14] = mc_dec(cmd.params[4], 48, 8);
>  	obj_desc->type[15] = '\0';
> -
> +	obj_desc->label[0] = mc_dec(cmd.params[5], 0, 8);
> +	obj_desc->label[1] = mc_dec(cmd.params[5], 8, 8);
> +	obj_desc->label[2] = mc_dec(cmd.params[5], 16, 8);
> +	obj_desc->label[3] = mc_dec(cmd.params[5], 24, 8);
> +	obj_desc->label[4] = mc_dec(cmd.params[5], 32, 8);
> +	obj_desc->label[5] = mc_dec(cmd.params[5], 40, 8);
> +	obj_desc->label[6] = mc_dec(cmd.params[5], 48, 8);
> +	obj_desc->label[7] = mc_dec(cmd.params[5], 56, 8);
> +	obj_desc->label[8] = mc_dec(cmd.params[6], 0, 8);
> +	obj_desc->label[9] = mc_dec(cmd.params[6], 8, 8);
> +	obj_desc->label[10] = mc_dec(cmd.params[6], 16, 8);
> +	obj_desc->label[11] = mc_dec(cmd.params[6], 24, 8);
> +	obj_desc->label[12] = mc_dec(cmd.params[6], 32, 8);
> +	obj_desc->label[13] = mc_dec(cmd.params[6], 40, 8);
> +	obj_desc->label[14] = mc_dec(cmd.params[6], 48, 8);
> +	obj_desc->label[15] = '\0';
>  	return 0;
>  }
>  EXPORT_SYMBOL(dprc_get_obj);
> @@ -696,31 +785,6 @@ int dprc_get_res_ids(struct fsl_mc_io *mc_io,
>  }
>  EXPORT_SYMBOL(dprc_get_res_ids);
> 
> -int dprc_get_portal_paddr(struct fsl_mc_io *mc_io,
> -			  uint16_t token,
> -			  int portal_id,
> -			  uint64_t *portal_addr)
> -{
> -	struct mc_command cmd = { 0 };
> -	int err;
> -
> -	/* prepare command */
> -	cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_PORTAL_PADDR,
> -					  MC_CMD_PRI_LOW, token);
> -	cmd.params[0] |= mc_enc(0, 32, portal_id);
> -
> -	/* send command to mc*/
> -	err = mc_send_command(mc_io, &cmd);
> -	if (err)
> -		return err;
> -
> -	/* retrieve response parameters */
> -	*portal_addr = mc_dec(cmd.params[1], 0, 64);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(dprc_get_portal_paddr);
> -

This should be a separate patch.

[patch 3] remove an unused function

>  int dprc_get_obj_region(struct fsl_mc_io *mc_io,
>  			uint16_t token,
>  			char *obj_type,
> @@ -759,13 +823,46 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io,
>  		return err;
> 
>  	/* retrieve response parameters */
> -	region_desc->base_paddr = mc_dec(cmd.params[1], 0, 64);
> +	region_desc->base_offset = mc_dec(cmd.params[1], 0, 64);
>  	region_desc->size = mc_dec(cmd.params[2], 0, 32);
> 
>  	return 0;
>  }

This hunk was part of patch 2.

>  EXPORT_SYMBOL(dprc_get_obj_region);
> 
> +int dprc_set_obj_label(struct fsl_mc_io *mc_io,
> +		       uint16_t  token,
> +		       int  obj_index,
> +		       char *label)
> +{
> +	struct mc_command cmd = { 0 };
> +
> +	/* prepare command */
> +	cmd.header = mc_encode_cmd_header(DPRC_CMDID_SET_OBJ_LABEL,
> +					  MC_CMD_PRI_LOW, token);
> +
> +	cmd.params[0] |= mc_enc(0, 32, obj_index);
> +	cmd.params[1] |= mc_enc(0, 8, label[0]);
> +	cmd.params[1] |= mc_enc(8, 8, label[1]);
> +	cmd.params[1] |= mc_enc(16, 8, label[2]);
> +	cmd.params[1] |= mc_enc(24, 8, label[3]);
> +	cmd.params[1] |= mc_enc(32, 8, label[4]);
> +	cmd.params[1] |= mc_enc(40, 8, label[5]);
> +	cmd.params[1] |= mc_enc(48, 8, label[6]);
> +	cmd.params[1] |= mc_enc(56, 8, label[7]);
> +	cmd.params[2] |= mc_enc(0, 8, label[8]);
> +	cmd.params[2] |= mc_enc(8, 8, label[9]);
> +	cmd.params[2] |= mc_enc(16, 8, label[10]);
> +	cmd.params[2] |= mc_enc(24, 8, label[11]);
> +	cmd.params[2] |= mc_enc(32, 8, label[12]);
> +	cmd.params[2] |= mc_enc(40, 8, label[13]);
> +	cmd.params[2] |= mc_enc(48, 8, label[14]);
> +	cmd.params[2] |= mc_enc(56, 8, label[15]);
> +
> +	/* send command to mc*/
> +	return mc_send_command(mc_io, &cmd);
> +}
> +
>  int dprc_connect(struct fsl_mc_io *mc_io,
>  		 uint16_t token,
>  		 const struct dprc_endpoint *endpoint1,
> diff --git a/drivers/staging/fsl-mc/bus/mc-allocator.c b/drivers/staging/fsl-mc/bus/mc-allocator.c
> index aa8280a..e445f79 100644
> --- a/drivers/staging/fsl-mc/bus/mc-allocator.c
> +++ b/drivers/staging/fsl-mc/bus/mc-allocator.c
> @@ -523,14 +523,20 @@ int __must_check fsl_mc_allocate_irqs(struct fsl_mc_device *mc_dev)
> 
>  		irqs[i] = to_fsl_mc_irq(resource);
>  		res_allocated_count++;
> +
> +		WARN_ON(irqs[i]->mc_dev);
> +		irqs[i]->mc_dev = mc_dev;
> +		irqs[i]->dev_irq_index = i;
>  	}
> 
>  	mc_dev->irqs = irqs;
>  	return 0;
> 
>  error_resource_alloc:
> -	for (i = 0; i < res_allocated_count; i++)
> +	for (i = 0; i < res_allocated_count; i++) {
> +		irqs[i]->mc_dev = NULL;
>  		fsl_mc_resource_free(&irqs[i]->resource);
> +	}
> 
>  	return error;
>  }
> @@ -545,8 +551,9 @@ void fsl_mc_free_irqs(struct fsl_mc_device *mc_dev)
>  	int i;
>  	int irq_count;
>  	struct fsl_mc_bus *mc_bus;
> +	struct fsl_mc_device_irq **irqs = mc_dev->irqs;
> 
> -	if (WARN_ON(!mc_dev->irqs))
> +	if (WARN_ON(!irqs))
>  		return;
> 
>  	irq_count = mc_dev->obj_desc.irq_count;
> @@ -559,8 +566,11 @@ void fsl_mc_free_irqs(struct fsl_mc_device *mc_dev)
>  	if (WARN_ON(!mc_bus->irq_resources))
>  		return;
> 
> -	for (i = 0; i < irq_count; i++)
> -		fsl_mc_resource_free(&mc_dev->irqs[i]->resource);
> +	for (i = 0; i < irq_count; i++) {
> +		WARN_ON(!irqs[i]->mc_dev);
> +		irqs[i]->mc_dev = NULL;
> +		fsl_mc_resource_free(&irqs[i]->resource);
> +	}
> 
>  	mc_dev->irqs = NULL;
>  }
> @@ -593,8 +603,8 @@ static int fsl_mc_allocator_probe(struct fsl_mc_device *mc_dev)
>  	if (error < 0)
>  		goto error;
> 
> -	dev_info(&mc_dev->dev,
> -		 "Allocatable MC object device bound to fsl_mc_allocator driver");
> +	dev_dbg(&mc_dev->dev,
> +		"Allocatable MC object device bound to fsl_mc_allocator driver");
>  	return 0;
>  error:
> 
> @@ -616,8 +626,8 @@ static int fsl_mc_allocator_remove(struct fsl_mc_device *mc_dev)
>  	if (error < 0)
>  		goto out;
> 
> -	dev_info(&mc_dev->dev,
> -		 "Allocatable MC object device unbound from fsl_mc_allocator driver");
> +	dev_dbg(&mc_dev->dev,
> +		"Allocatable MC object device unbound from fsl_mc_allocator driver");
>  	error = 0;
>  out:
>  	return error;

These two can separated out:

[patch 4] make dmesg not so spammy

> diff --git a/drivers/staging/fsl-mc/bus/mc-bus.c b/drivers/staging/fsl-mc/bus/mc-bus.c
> index 83eb906..b82fd7b 100644
> --- a/drivers/staging/fsl-mc/bus/mc-bus.c
> +++ b/drivers/staging/fsl-mc/bus/mc-bus.c
> @@ -315,7 +315,8 @@ common_cleanup:
>  	return error;
>  }
> 
> -static int translate_mc_addr(uint64_t mc_addr, phys_addr_t *phys_addr)
> +static int translate_mc_addr(enum fsl_mc_region_types mc_region_type,
> +			     uint64_t mc_offset, phys_addr_t *phys_addr)
>  {
>  	int i;
>  	struct fsl_mc *mc = dev_get_drvdata(fsl_mc_bus_type.dev_root->parent);
> @@ -324,7 +325,7 @@ static int translate_mc_addr(uint64_t mc_addr, phys_addr_t *phys_addr)
>  		/*
>  		 * Do identity mapping:
>  		 */
> -		*phys_addr = mc_addr;
> +		*phys_addr = mc_offset;
>  		return 0;
>  	}
> 
> @@ -332,10 +333,11 @@ static int translate_mc_addr(uint64_t mc_addr, phys_addr_t *phys_addr)
>  		struct fsl_mc_addr_translation_range *range =
>  			&mc->translation_ranges[i];
> 
> -		if (mc_addr >= range->start_mc_addr &&
> -		    mc_addr < range->end_mc_addr) {
> +		if (mc_region_type == range->mc_region_type &&
> +		    mc_offset >= range->start_mc_offset &&
> +		    mc_offset < range->end_mc_offset) {
>  			*phys_addr = range->start_phys_addr +
> -				     (mc_addr - range->start_mc_addr);
> +				     (mc_offset - range->start_mc_offset);
>  			return 0;
>  		}
>  	}
> @@ -351,6 +353,22 @@ static int fsl_mc_device_get_mmio_regions(struct fsl_mc_device *mc_dev,
>  	struct resource *regions;
>  	struct dprc_obj_desc *obj_desc = &mc_dev->obj_desc;
>  	struct device *parent_dev = mc_dev->dev.parent;
> +	enum fsl_mc_region_types mc_region_type;
> +
> +	if (strcmp(obj_desc->type, "dprc") == 0 ||
> +	    strcmp(obj_desc->type, "dpmcp") == 0) {
> +		mc_region_type = FSL_MC_PORTAL;
> +	} else if (strcmp(obj_desc->type, "dpio") == 0) {
> +		mc_region_type = FSL_QBMAN_PORTAL;
> +	} else {
> +		/*
> +		 * This function should not have been called for this MC object
> +		 * type, as this object type is not supposed to have MMIO
> +		 * regions
> +		 */
> +		WARN_ON(true);
> +		return -EINVAL;
> +	}
> 
>  	regions = kmalloc_array(obj_desc->region_count,
>  				sizeof(regions[0]), GFP_KERNEL);
> @@ -370,14 +388,14 @@ static int fsl_mc_device_get_mmio_regions(struct fsl_mc_device *mc_dev,
>  			goto error_cleanup_regions;
>  		}
> 
> -		WARN_ON(region_desc.base_paddr == 0x0);
>  		WARN_ON(region_desc.size == 0);
> -		error = translate_mc_addr(region_desc.base_paddr,
> +		error = translate_mc_addr(mc_region_type,
> +					  region_desc.base_offset,
>  					  &regions[i].start);
>  		if (error < 0) {
>  			dev_err(parent_dev,
> -				"Invalid MC address: %#llx (for %s.%d\'s region %d)\n",
> -				region_desc.base_paddr,
> +				"Invalid MC offset: %#llx (for %s.%d\'s region %d)\n",
> +				region_desc.base_offset,
>  				obj_desc->type, obj_desc->id, i);
>  			goto error_cleanup_regions;
>  		}
> @@ -641,6 +659,10 @@ static void mc_bus_unmask_msi_irq(struct irq_data *d)
>  	irq_chip_unmask_parent(d);
>  }
> 
> +/*
> + * This function is invoked from devm_request_irq(),
> + * devm_request_threaded_irq(), dev_free_irq()
> + */
>  static void mc_bus_msi_domain_write_msg(struct irq_data *irq_data,
>  					struct msi_msg *msg)
>  {
> @@ -657,6 +679,13 @@ static void mc_bus_msi_domain_write_msg(struct irq_data *irq_data,
>  		irq_res->msi_paddr =
>  			((u64)msg->address_hi << 32) | msg->address_lo;
>  		irq_res->msi_value = msg->data;
> +
> +		/*
> +		 * NOTE: We cannot do the actual programming of the MSI
> +		 * in the MC, in this function, as the object-independent
> +		 * way of programming MSIs for MC objects is not reliable
> +		 * if objects are being added/removed dynamically.
> +		 */
>  	}
>  }
> 
> @@ -706,10 +735,6 @@ static int create_mc_irq_domain(struct platform_device *mc_pdev,
>  		goto cleanup_its_of_node;
>  	}
> 
> -	/*
> -	 * FIXME: Enable this code when the GIC-ITS MC support patch is merged
> -	 */
> -#ifdef GIC_ITS_MC_SUPPORT
>  	irq_domain = msi_create_irq_domain(mc_of_node, &mc_bus_msi_domain_info,
>  					   its_domain->parent);
>  	if (!irq_domain) {
> @@ -719,9 +744,6 @@ static int create_mc_irq_domain(struct platform_device *mc_pdev,
>  	}
> 
>  	dev_dbg(&mc_pdev->dev, "Allocated MSI domain\n");
> -#else
> -	irq_domain = NULL;
> -#endif
>  	*new_irq_domain = irq_domain;
>  	return 0;
> 


We still seem to be removing GIC_ITS_MC_SUPPORT.  In v1 this was a
mistake.  Is this intentional now?

regards,
dan carpenter