diff mbox series

[RFT,v2,02/10] drivers: qcom: rpmh-rsc: Document the register layout better

Message ID 20200311161104.RFT.v2.2.Iaddc29b72772e6ea381238a0ee85b82d3903e5f2@changeid (mailing list archive)
State Superseded
Headers show
Series drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand

Commit Message

Doug Anderson March 11, 2020, 11:13 p.m. UTC 
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.  Let's add
a bunch of comments describing which blocks are part of other blocks.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Now prose in comments instead of struct definitions.
- Pretty ASCII art from Stephen.

 drivers/soc/qcom/rpmh-rsc.c | 78 ++++++++++++++++++++++++++++++++++---
 1 file changed, 73 insertions(+), 5 deletions(-)

Comments

Maulik Shah April 1, 2020, 8:14 a.m. UTC | #1 
Hi,

On 3/12/2020 4:43 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.  Let's add
> a bunch of comments describing which blocks are part of other blocks.
>
> Signed-off-by: Douglas Anderson<dianders@chromium.org>
> ---
>
> Changes in v2:
> - Now prose in comments instead of struct definitions.
> - Pretty ASCII art from Stephen.
>
>   drivers/soc/qcom/rpmh-rsc.c | 78 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index b87b79f0347d..02c8e0ffbbe4 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -37,14 +37,24 @@
>   #define DRV_NCPT_MASK			0x1F
>   #define DRV_NCPT_SHIFT			27
>   
> -/* Register offsets */
> +/*
> + * Register offsets within a TCS.

Change this to

/* Offsets for common TCS Registers, one bit per TCS */

> + *
> + * TCSs are stored one after another; multiply tcs_id by RSC_DRV_TCS_OFFSET
> + * to find a given TCS and add one of the below to find a register.
> + */
Move above comment after these 3 common IRQ registers.
>   #define RSC_DRV_IRQ_ENABLE		0x00
>   #define RSC_DRV_IRQ_STATUS		0x04
> -#define RSC_DRV_IRQ_CLEAR		0x08

please add line break between RSC_DRV_IRQ_CLEAR and 
RSC_DRV_CMD_WAIT_FOR_CMPL

you may want to add one more comment inbetween saying

/* Offsets for per TCS Registers */

> -#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10
> +#define RSC_DRV_IRQ_CLEAR		0x08	/* w/o; write 1 to clear */
> +#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10	/* 1 bit per command */
>   #define RSC_DRV_CONTROL			0x14
> -#define RSC_DRV_STATUS			0x18
> -#define RSC_DRV_CMD_ENABLE		0x1C
> +#define RSC_DRV_STATUS			0x18	/* zero if tcs is busy */
> +#define RSC_DRV_CMD_ENABLE		0x1C	/* 1 bit per command */
> +
> +/*
> + * Commands (up to 16) start at 0x30 in a TCS; multiply command index
> + * by RSC_DRV_CMD_OFFSET and add one of the below to find a register.
> + */
you may also add /* Offsets for per command in a TCS */
>   #define RSC_DRV_CMD_MSGID		0x30
>   #define RSC_DRV_CMD_ADDR		0x34
>   #define RSC_DRV_CMD_DATA		0x38
> @@ -61,6 +71,64 @@
>   #define CMD_STATUS_ISSUED		BIT(8)
>   #define CMD_STATUS_COMPL		BIT(16)
>   
> +/*
> + * Here's a high level overview of how all the registers in RPMH work
> + * together:
> + *
> + * - The main rpmh-rsc address is the base of a register space that can
> + *   be used to find overall configuration of the hardware
> + *   (DRV_PRNT_CHLD_CONFIG).  Also found within the rpmh-rsc register
> + *   space are all the TCS blocks.  The offset of the TCS blocks is
> + *   specified in the device tree by "qcom,tcs-offset" and used to
> + *   compute tcs_base.
> + * - TCS blocks come one after another.  Type, count, and order are
> + *   specified by the device tree as "qcom,tcs-config".
> + * - Each TCS block has some registers, then space for up to 16 commands.
> + *   Note that though address space is reserved for 16 commands, fewer
> + *   might be present.  See ncpt (num cmds per TCS).
> + * - The first TCS block is special in that it has registers to control
> + *   interrupts (RSC_DRV_IRQ_XXX).  Space for these registers is reserved
> + *   in all TCS blocks to make the math easier, but only the first one
> + *   matters.

First TCS block is not special, the RSC_DRV_IRQ_XXX registers are common 
for all

TCSes.  can you please drop this last paragraph and then add one more 
block in

ASCII diagram to have TCS common IRQ registers like below to represent 
it more clear.

+----------------------------------------------+
|TCS                                           |
| IRQ                                          |
|                                              |
| +------------------------------------------+ |
| |TCS0  |  |  |  |  |  |  |  |  |  |  |  |  | |
| |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15| |
| | ctrl |  |  |  |  |  |  |  |  |  |  |  |  | |
| +------------------------------------------+ |
+----------------------------------------------+


> + *
> + * Here's a picture:
> + *
> + *  +---------------------------------------------------+
> + *  |RSC                                                |
> + *  | ctrl                                              |
> + *  |                                                   |
> + *  | Drvs:                                             |
> + *  | +-----------------------------------------------+ |
> + *  | |DRV0                                           | |
> + *  | | ctrl                                          | |
> + *  | |                                               | |
> + *  | | TCSs:                                         | |
> + *  | | +------------------------------------------+  | |
> + *  | | |TCS0  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | | IRQ  | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
> + *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | +------------------------------------------+  | |
> + *  | | +------------------------------------------+  | |
> + *  | | |TCS1  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
> + *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | +------------------------------------------+  | |
> + *  | | +------------------------------------------+  | |
> + *  | | |TCS2  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
> + *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | +------------------------------------------+  | |
> + *  | |                    ......                     | |
> + *  | +-----------------------------------------------+ |
> + *  | +-----------------------------------------------+ |
> + *  | |DRV1                                           | |
> + *  | | (same as DRV0)                                | |
> + *  | +-----------------------------------------------+ |
> + *  |                      ......                       |
> + *  +---------------------------------------------------+
> + */
> +
> +

nit: two blank lines at the end, you can drop one.

Thanks,
Maulik
>   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 +
Doug Anderson April 2, 2020, 8:15 p.m. UTC | #2 
Hi,

On Wed, Apr 1, 2020 at 1:14 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> > + * - The first TCS block is special in that it has registers to control
> > + *   interrupts (RSC_DRV_IRQ_XXX).  Space for these registers is reserved
> > + *   in all TCS blocks to make the math easier, but only the first one
> > + *   matters.
>
> First TCS block is not special, the RSC_DRV_IRQ_XXX registers are common
> for all

Ah.  I think I see it now.  It's much easier to talk about this with
my old struct definition.  Right now I have it documented as:

/* 0x2a0 = 672 bytes big (see RSC_DRV_TCS_OFFSET) */
struct tcs_hw {
  u32 irq_enable;
  u32 irq_status;
  u32 irq_clear;
  char opaque_00c[0x4];
  u32 cmd_wait_for_cmpl;
  u32 control;
  u32 status;
  u32 cmd_enable;
  char opaque_020[0x10];
  struct tcs_cmd_hw tcs_cmd_hw[MAX_CMDS_PER_TCS];
  char opaque_170[0x130];
};

...but you're saying that it's actually:

/* 0x2a0 = 672 bytes big (see RSC_DRV_TCS_OFFSET) */
struct tcs_hw {
  u32 cmd_wait_for_cmpl;
  u32 control;
  u32 status;
  u32 cmd_enable;
  char opaque_010[0x10];
  struct tcs_cmd_hw tcs_cmd_hw[MAX_CMDS_PER_TCS];
  char opaque_160[0x140];
};

So it's still 672 bytes big but the extra "opaque" at the end is where
those bytes go.  Then, before the first TCS, there's actually 0x10
bytes of IRQ stuff.  OK, I will adjust the diagrams.

-Doug
diff mbox series

Patch

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index b87b79f0347d..02c8e0ffbbe4 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -37,14 +37,24 @@ 
 #define DRV_NCPT_MASK			0x1F
 #define DRV_NCPT_SHIFT			27
 
-/* Register offsets */
+/*
+ * Register offsets within a TCS.
+ *
+ * TCSs are stored one after another; multiply tcs_id by RSC_DRV_TCS_OFFSET
+ * to find a given TCS and add one of the below to find a register.
+ */
 #define RSC_DRV_IRQ_ENABLE		0x00
 #define RSC_DRV_IRQ_STATUS		0x04
-#define RSC_DRV_IRQ_CLEAR		0x08
-#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10
+#define RSC_DRV_IRQ_CLEAR		0x08	/* w/o; write 1 to clear */
+#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10	/* 1 bit per command */
 #define RSC_DRV_CONTROL			0x14
-#define RSC_DRV_STATUS			0x18
-#define RSC_DRV_CMD_ENABLE		0x1C
+#define RSC_DRV_STATUS			0x18	/* zero if tcs is busy */
+#define RSC_DRV_CMD_ENABLE		0x1C	/* 1 bit per command */
+
+/*
+ * Commands (up to 16) start at 0x30 in a TCS; multiply command index
+ * by RSC_DRV_CMD_OFFSET and add one of the below to find a register.
+ */
 #define RSC_DRV_CMD_MSGID		0x30
 #define RSC_DRV_CMD_ADDR		0x34
 #define RSC_DRV_CMD_DATA		0x38
@@ -61,6 +71,64 @@ 
 #define CMD_STATUS_ISSUED		BIT(8)
 #define CMD_STATUS_COMPL		BIT(16)
 
+/*
+ * Here's a high level overview of how all the registers in RPMH work
+ * together:
+ *
+ * - The main rpmh-rsc address is the base of a register space that can
+ *   be used to find overall configuration of the hardware
+ *   (DRV_PRNT_CHLD_CONFIG).  Also found within the rpmh-rsc register
+ *   space are all the TCS blocks.  The offset of the TCS blocks is
+ *   specified in the device tree by "qcom,tcs-offset" and used to
+ *   compute tcs_base.
+ * - TCS blocks come one after another.  Type, count, and order are
+ *   specified by the device tree as "qcom,tcs-config".
+ * - Each TCS block has some registers, then space for up to 16 commands.
+ *   Note that though address space is reserved for 16 commands, fewer
+ *   might be present.  See ncpt (num cmds per TCS).
+ * - The first TCS block is special in that it has registers to control
+ *   interrupts (RSC_DRV_IRQ_XXX).  Space for these registers is reserved
+ *   in all TCS blocks to make the math easier, but only the first one
+ *   matters.
+ *
+ * Here's a picture:
+ *
+ *  +---------------------------------------------------+
+ *  |RSC                                                |
+ *  | ctrl                                              |
+ *  |                                                   |
+ *  | Drvs:                                             |
+ *  | +-----------------------------------------------+ |
+ *  | |DRV0                                           | |
+ *  | | ctrl                                          | |
+ *  | |                                               | |
+ *  | | TCSs:                                         | |
+ *  | | +------------------------------------------+  | |
+ *  | | |TCS0  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | | IRQ  | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
+ *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | +------------------------------------------+  | |
+ *  | | +------------------------------------------+  | |
+ *  | | |TCS1  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
+ *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | +------------------------------------------+  | |
+ *  | | +------------------------------------------+  | |
+ *  | | |TCS2  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
+ *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | +------------------------------------------+  | |
+ *  | |                    ......                     | |
+ *  | +-----------------------------------------------+ |
+ *  | +-----------------------------------------------+ |
+ *  | |DRV1                                           | |
+ *  | | (same as DRV0)                                | |
+ *  | +-----------------------------------------------+ |
+ *  |                      ......                       |
+ *  +---------------------------------------------------+
+ */
+
+
 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 +