LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Doug Anderson <dianders@chromium.org>,
	Maulik Shah <mkshah@codeaurora.org>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Evan Green <evgreen@chromium.org>,
	Lina Iyer <ilina@codeaurora.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better
Date: Wed, 11 Mar 2020 13:08:38 -0700	[thread overview]
Message-ID: <158395731898.149997.1612077509811625118@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAD=FV=W5RGwu=ywtM7aCv3H-EpJ1i23S0awgVc8QtOsXtige4w@mail.gmail.com>

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?

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

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

  parent reply	other threads:[~2020-03-11 20:08 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-06 23:59 [RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds Douglas Anderson
2020-03-11  8:47   ` Maulik Shah
2020-03-11 15:03     ` Doug Anderson
2020-03-11 16:17       ` Matthias Kaehlcke
2020-03-11 19:30         ` Stephen Boyd
2020-03-06 23:59 ` [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better Douglas Anderson
2020-03-11  9:35   ` Maulik Shah
2020-03-11 15:27     ` Doug Anderson
2020-03-11 18:49       ` Evan Green
2020-03-11 20:08       ` Stephen Boyd [this message]
2020-03-11 22:35         ` Doug Anderson
2020-03-06 23:59 ` [RFT PATCH 3/9] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller Douglas Anderson
2020-03-11  9:50   ` Maulik Shah
2020-03-06 23:59 ` [RFT PATCH 4/9] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction Douglas Anderson
2020-03-11 12:02   ` Maulik Shah
2020-03-06 23:59 ` [RFT PATCH 5/9] drivers: qcom: rpmh-rsc: A lot of comments Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 6/9] drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY Douglas Anderson
2020-03-11  0:33   ` Doug Anderson
2020-03-06 23:59 ` [RFT PATCH 7/9] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 8/9] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate() Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 9/9] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire Douglas Anderson
2020-03-11  0:35   ` Doug Anderson
2020-03-11  9:48 ` [RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Maulik Shah

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=158395731898.149997.1612077509811625118@swboyd.mtv.corp.google.com \
    --to=swboyd@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dianders@chromium.org \
    --cc=evgreen@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=mkshah@codeaurora.org \
    --cc=rnayak@codeaurora.org \
    --subject='Re: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).