Re: [PATCH 3/3] arm64: dts: qcom: Split out SA8155P and use correct RPMh power domains

On Thu, Mar 16, 2023 at 12:50:49AM +0100, Konrad Dybcio wrote:

On 16.03.2023 00:00, Bjorn Andersson wrote:

quoted

On Tue, Mar 14, 2023 at 12:41:45PM +0100, Konrad Dybcio wrote:

quoted

On 14.03.2023 12:36, Konrad Dybcio wrote:

quoted

On 14.03.2023 01:10, Bjorn Andersson wrote:

quoted

On Tue, Feb 14, 2023 at 10:54:35AM +0100, Konrad Dybcio wrote:

quoted

The RPMhPD setup on SA8155P is different compared to SM8150. Correct
it to ensure the platform will not try accessing forbidden/missing
RPMh entries at boot, as a bad vote will hang the machine.

I don't see that this will scale, as soon as someone adds a new device
in sm8150.dtsi that has the need to scale a power rail this will be
forgotten and we will have a mix of references to the SM8150 and SA8155P
value space.

That said, I think it's reasonable to avoid duplicating the entire
sm8150.dtsi.

Yeah, this problem has no obvious good solutions and even though it's
not very elegant, this seems to be the less bad one..

quoted

How about making the SA8155P_* macros match the SM8150_* macros?
That way things will fail gracefully if a device node references a
resource not defined for either platform...

Okay, let's do that

Re-thinking it, it's good that the indices don't match, as this way the
board will (should) refuse to function properly if there's an oversight,
which may have gone unnoticed if they were matching, so this only guards
us against programmer error which is not great :/

Right, ensuring that the resource indices never collides would be a good
way to capture this issue, as well as copy-paste errors etc. My
pragmatic proposal is that we make SA8155P_x == SM8150_x where a match
exist, and for the ones that doesn't match we pick numbers that doesn't
collide between the platforms.

The alternative is to start SA8155P_x at 11, but it's different and
forces sa8155p.dtsi to redefine every single power-domains property...


This does bring back the feeling that it was a mistake to include the
platform name in these defines in the first place... Not sure if it's
worth mixing generic defines into the picture at this point, given that
we I don't see a way to use them on any existing platform.

TBF we could, think:

sm1234_rpmpds[] = {
	[CX] = &foobar1,
	[CX_AO] = &foobar1_ao,

	[...]

	/* Legacy DT bindings */
	[SM1234_CX] = &foobar1,
	[SM1234_CX_AO] = &foobar1_ao,
};

WDYT?

Given that every platform got these defines different we'd have to start
at the new generic list at 17 (which would throw away 136 bytes per
platform), if we're going to allow the scheme for existing platforms.
Which I don't fancy.

It's not super-pretty to mix and match, but I think I would be okay
switching to this scheme for new platforms.

PS. We'd better prefix the defines with something (perhaps RPM_?)

Regards,
Bjorn