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
