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

From: Dan Carpenter
Date: Fri May 08 2015 - 03:23:00 EST

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/