lkml.org 
[lkml]   [2020]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better
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.
* - 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.
*/

I'll also move the documentation of "irq_clear", "cmd_wait_for_cmpl",
"status", and "cmd_enable" next to the respective #defines.

\
 
 \ /
  Last update: 2020-03-11 16:29    [W:0.284 / U:7.708 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site