Re: [PATCH v3 13/13] bus/fsl-mc: Add a new version for dprc_get_obj_region command

From: Laurentiu Tudor
Date: Mon Jul 06 2020 - 09:14:34 EST




On 7/6/2020 3:42 PM, Diana Craciun wrote:
> From: Diana Craciun <diana.craciun@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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)
>