Re: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better

[Maulik Shah]

Hi Doug,

On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> Perhaps it's just me, it took a really long time to understand what
> the register layout of rpmh-rsc was just from the #defines.  
i don't understand why register layout is so important for you to understand?

besides, i think all required registers are properly named with #define

for e.g.
/* Register offsets */
#define RSC_DRV_IRQ_ENABLEÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00
#define RSC_DRV_IRQ_STATUSÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x04
#define RSC_DRV_IRQ_CLEARÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x08

now when you want to enable/disable irq in driver code, its pretty simple to figure out
that we need to read/write at RSC_DRV_IRQ_ENABLE offset.

this seems unnecessary change to me, can you please drop this when you spin v2?

Thanks,
Maulik
> It's much
> easier to understand this if we define some structures.  At the moment
> these structures aren't used at all (so think of them as
> documentation), but to me they really help in understanding.
>
> These structures were all figured out from the #defines and
> reading/writing functions.  Anything that wasn't used in the driver is
> marked as "opaque".
>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
>  drivers/soc/qcom/rpmh-rsc.c | 67 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 5c88b8cd5bf8..0a409988d103 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -61,6 +61,73 @@
>  #define CMD_STATUS_ISSUED		BIT(8)
>  #define CMD_STATUS_COMPL		BIT(16)
>  
> +/*
> + * The following structures aren't used in the code anywhere (right now), but
> + * help to document how the register space is laid out.  In other words it's
> + * another way to visualize the "Register offsets".
> + *
> + * Couch this in a bogus #ifdef instead of comments to allow the embedded
> + * comments to work.
> + */
> +#ifdef STRUCTS_TO_DOCUMENT_HW_REGISTER_MAP
> +
> +/* 0x14 = 20 bytes big (see RSC_DRV_CMD_OFFSET) */
> +struct tcs_cmd_hw {
> +	u32 msgid;
> +	u32 addr;
> +	u32 data;
> +	u32 status;
> +	u32 resp_data;
> +};
> +
> +/* 0x2a0 = 672 bytes big (see RSC_DRV_TCS_OFFSET) */
> +struct tcs_hw {
> +	/*
> +	 * These are only valid on TCS 0 but are present everywhere.
> +	 * Contains 1 bit per TCS.
> +	 */
> +	u32 irq_enable;
> +	u32 irq_status;
> +	u32 irq_clear;			/* Write only; write 1 to clear */
> +
> +	char opaque_00c[0x4];
> +
> +	u32 cmd_wait_for_cmpl;		/* Bit field, 1 bit per command */
> +	u32 control;
> +	u32 status;			/* status is 0 if tcs is busy */
> +	u32 cmd_enable;			/* Bit field, 1 bit per command */
> +
> +	char opaque_01c[0x10];
> +
> +	struct tcs_cmd_hw tcs_cmd_hw[MAX_CMDS_PER_TCS];
> +
> +	char opaque_170[0x130];
> +};
> +
> +/* Example for sc7180 based on current dts */
> +struct rpmh_rsc_hw_sc7180 {
> +	char opaque_000[0xc];
> +
> +	u32 prnt_chld_config;
> +
> +	char opaque_010[0xcf0];
> +
> +	/*
> +	 * Offset 0xd00 aka qcom,tcs-offset from device tree.  Presumably
> +	 * could be different for different SoCs?  Currently driver stores
> +	 * a pointer to the first tcs in tcs_base.
> +	 *
> +	 * Count of various TCS entries also comes from dts.
> +	 */
> +	struct tcs_hw active[2];
> +	struct tcs_hw sleep[3];
> +	struct tcs_hw wake[3];
> +	struct tcs_hw control[1];
> +};
> +
> +#endif /* STRUCTS_TO_DOCUMENT_HW_REGISTER_MAP */
> +
> +
>  static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
>  {
>  	return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation