Skip to content

drivers: qcom: rpmh: split target specific configuration for Lemans - #55

Open
Shivam Sanjay (shvm-ap) wants to merge 2 commits into
qualcomm-linux:qcom-nextfrom
shvm-ap:target-specific-with-tests
Open

Shivam Sanjay (shvm-ap) wants to merge 2 commits into
qualcomm-linux:qcom-nextfrom
shvm-ap:target-specific-with-tests

Conversation

@shvm-ap

@shvm-ap Shivam Sanjay (shvm-ap) commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Move the RPMh driver's per-target configuration (rpmh_drv_config.c,
rpmh_hwio.h, rpmh_target_config.h) out of the common driver directory
into per-flavor, currently done for Lemans, similar pattern will be followed
for Nord and sequent target support.

Lemans build/compilation tested.

@shvm-ap
Shivam Sanjay (shvm-ap) marked this pull request as draft August 31, 2026 15:53
@b49020
Sumit Garg (b49020) force-pushed the qcom-next branch 2 times, most recently from 1a117cb to dab3efd Compare September 7, 2026 07:43
@shvm-ap
Shivam Sanjay (shvm-ap) force-pushed the target-specific-with-tests branch 5 times, most recently from 6c4fc2e to c4f4434 Compare September 8, 2026 05:57
@zelvam95

Copy link
Copy Markdown
Contributor

Shivam Sanjay (@shvm-ap)

As we synced offline, you can separate out the RPMH Unit Test framework as separate PR or as separate commit at least; I believe we don't want to merge it and its raised just for reference/testing purposes?

Also, I think the MSG_RAM_SECTION_SIZE movement + Nord support changes are something that we want to merge. So, if you stack those changes on top of the previous RPMH PRs #47 and #57, then we can plan to incrementally merge one after the other as we unit test them and ensure it can work.

@shvm-ap Shivam Sanjay (shvm-ap) changed the title Target specific RPMh and CmdDb drivers with basic unit tests support drivers: qcom: rpmh: split target specific configuration for Lemans Sep 9, 2026
Signed-off-by: Shivam Sanjay <shivsanj@qti.qualcomm.com>
@shvm-ap

Copy link
Copy Markdown
Contributor Author

Selvam (@zelvam95) Added the MSG_RAM_SECTION_SIZE changes for lemans in this commit. Apart from this, added some more files in target specific folders, may get conflict with #57, that we will resolve (may involve some file name changes as well if required), once #57 is merged. This structure will be followed for adding Nord support as well.

@IDineshChoudhary IDineshChoudhary left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments.


#define AOP_MSG_RAM_BASE UL(0x0C300000)
#define AOP_MSG_RAM_SIZE UL(0x00100000)
#define MSG_RAM_SECTION_SIZE UL(0x00010000)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you correct indent here

@@ -13,6 +13,13 @@
#define AOP_CMD_DB_BASE UL(0x90860000)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally C requires U alias at last to indicate it's unsigned int why it's unsigned long here that maps to 64bit right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, UL is not required here. However, it aligns with the existing coding style. May prevent unwanted overflows while adding new entries.


#include <util.h>

#define DRV_STRIDE 0x10000

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you expecting this to be used anywhere I don't think so - please clean up if not used.

@@ -0,0 +1,56 @@
/* SPDX-License-Identifier: BSD-2-Clause */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking to club this existing header itself rather than having a new one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have a rpmh_target.h header like the one used in #57, another possibility is clubbing in target_config.h but that is supposed to be a higher level description, I believe.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, IDineshChoudhary; I discussed with Shivam as well & once he rebases his PR on top of the other PR which we're reviewing in upstream/qcom-next (raised by me), some of these comments would get addressed; We plan to do that once the other PR merges;

#define RSC_DRV_IRQ_CLEAR 0x0d08

#define RSC_DRV_TCS_CONFIG 0x0C
#define TCS_BASE_OFFSET 0x0D10 /* CMD_WAIT_FOR_CMPL base */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment and register name doesn't align

}

dict_addr = base + AOP_MSG_RAM_SIZE - MSG_RAM_SECTION_SIZE;
dict_addr = base + 15 * MSG_RAM_SECTION_SIZE;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you need to use 15 than define this as a micro rather than hardcoding.

@@ -0,0 +1,56 @@
/* SPDX-License-Identifier: BSD-2-Clause */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also can for micros defined if this if any unused ones there and clean them up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

# Per-target DRV configuration, register layout, target config
srcs-y += $(PLATFORM_FLAVOR)/rpmh_drv_config.c
global-incdirs-y += .
global-incdirs-y += $(PLATFORM_FLAVOR)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good.
Selvam (@zelvam95) your pull request require this change too then.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the includes in the same directory can be directly used using #include "" pattern. The includes inside chipset folder it won't be appropriate to include with chipset name in C File (since it won't scale) and hence we've used global-incdirs-y pattern.

The required includes are present in the PR I've raised & its compilation tested + unit tested on Lemans Device. Shivam has also unit tested my PR on Nord without this PR so its taken care.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated as response to the comments given in that PR clarifying the same as well;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually IDineshChoudhary, rpmh_drv_config has been removed (as of now) from #57, that's why that may be different. Also regarding the "global-incdirs-y += .", Selvam (@zelvam95) , should we use this and follow include <> or simply use the include "" pattern, and remove this entry?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the header file is in same driver file path, you could just do "".
If the header file is in some other patch, you can use global includes as required.
Especially if this header would be included by other drivers etc, you could use global includes.

In general - You could refer to existing upstream drivers/patterns and align to that;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, IDineshChoudhary, We plan to rebase this PR on top of the other PR I have raised once that is merged. We could re-use some of the header files I have introduced etc. here and move macros there. That would make it cleaner. Maybe you could hold this review for a bit until the other PR is merged, so it'll be cleaner;

Shivam Sanjay (@shvm-ap), If it'd make sense, you could mark this as draft until the other PR is merged, since you will need the headers etc used in other PR over here for moving the MSG RAM Size etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants