diff mbox series

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

Message ID 20200306155707.RFT.2.Iaddc29b72772e6ea381238a0ee85b82d3903e5f2@changeid
State New
Headers show
Series None | expand

Commit Message

Doug Anderson March 6, 2020, 11:59 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.  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@chromium.org>
---

 drivers/soc/qcom/rpmh-rsc.c | 67 +++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

Comments

Maulik Shah March 11, 2020, 9:35 a.m. UTC | #1 
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@chromium.org>
> ---
>
>  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 +
Doug Anderson March 11, 2020, 10:35 p.m. UTC | #2 
Hi,

On Wed, Mar 11, 2020 at 1:08 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-03-11 08:27:44)
> > Hi,
> >
> > On Wed, Mar 11, 2020 at 2:35 AM Maulik Shah <mkshah@codeaurora.org> wrote:
> > >
> > > 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.
> >
> > It's not the specific layout that's important to me.  It's the
> > relationships between everything.  In other words:
> >
> > a) one rpmh-rsc contains some registers and then a whole bunch of TCS blocks
> >
> > b) one TCS block contains some registers and space for up to 16 commands.
> >
> > c) each command has a handful of registers
> >
> > Nothing describes this in the existing code--you've gotta read through
> > all the code and figure out how it's reading/writing registers to
> > figure it out.
> >
> >
> > > this seems unnecessary change to me, can you please drop this when you spin v2?
> >
> > In v2 I'll replace this with the prose below.  Personally I find this
> > inferior to the struct definitions which are easier to read
> > at-a-glance, but it seems like it would be less controversial...
> >
> > /*
> >  * 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.
>
> I never liked the qcom,tcs-offset property. Why can't that be hardcoded
> in the driver based on the compatible string? Why does it need to be
> specified in DT at all?
>
> >  * - TCS blocks come one after another.  Type, count, and order are
> >  *   specified by the device tree as "qcom,tcs-config".
>
> This one too. Is the idea that some boards are going to change how the TCS
> FIFOs are used?

Yeah, those thoughts occurred to me as well.  ...but right now there
actually _is_ no SoC-specific compatible string.  From sc7180:

  apps_rsc: rsc@18200000 {
    compatible = "qcom,rpmh-rsc";

I wouldn't be opposed to deprecating those properties and adding an
SoC-specific compatible string, but I probably won't take that up in
my series.  It's already getting a little unwieldy.


> >  * - 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.
>
> I suspect an ascii block diagram would be useful to understand how it is
> all done up. But given that the first TCS block has some sort of irq
> control registers maybe it isn't correct to draw it split out of TCS0. I
> think the first TCS is always the "active" TCS so it has these registers
> for "irq control". The other two are the sleep and wake ones. But
> sometimes there isn't an active TCS or software doesn't have a use for
> it? Can probably indicate this in the diagram too.

One thing that indicates that the first TCS is special and it's not
just that the first TCS happens to be the used for active-only is that
there are actually two active-only TCSs but we check the IRQ registers
in TCS 0 to tell us about both of them.


> Oh and maybe there should be CMD registers inside each TCS in this
> diagram. It'd be great to have something like this so visual learners
> like me can understand it all.
>
>   +--------------------------------------------------+
>   |RSC                                               |
>   | +----------------------------------------------+ |
>   | |DRV0                                          | |
>   | | +-----------------------------------------+  | |
>   | | |IRQ CONTROL                              |  | |
>   | | |                                         |  | |
>   | | +-----------------------------------------+  | |
>   | | +-----------------------------------------+  | |
>   | | |TCS0 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
>   | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | +-----------------------------------------+  | |
>   | | +-----------------------------------------+  | |
>   | | |TCS1 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
>   | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | +-----------------------------------------+  | |
>   | | +-----------------------------------------+  | |
>   | | |TCS2 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
>   | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | +-----------------------------------------+  | |
>   | |                   ......                     | |
>   | +----------------------------------------------+ |
>   | +----------------------------------------------+ |
>   | |DRV1                                          | |
>   | | (same as DRV0)                               | |
>   | +----------------------------------------------+ |
>   +--------------------------------------------------+

Thanks!  I was trying to figure out some sort of ASCII art at one
point but I gave up.  I was thinking that the struct was as good was
we were going to get in terms of "visual".  What you've got looks
useful though and I'll paste something like this in.  I'm going to
leave out the "IRQ control" parts though since it (to me) makes it
feel like it's a whole separate memory region.  In fact the IRQ
control registers seem (from the math) to be present in all the TCSs.
I'm going to go with this:

 *  +---------------------------------------------------+
 *  |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)                                | |
 *  | +-----------------------------------------------+ |
 *  |                      ......                       |
 *  +---------------------------------------------------+

So I'll end up skipping the fake struct definitions and go with the
prose + diagram.  If nothing else it's less controversial than
bogus/unused struct definitions.

-Doug
diff mbox series

Patch

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 +