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

[Bjorn Andersson]

On Thu, Mar 16, 2023 at 12:50:49AM +0100, Konrad Dybcio wrote:
> On 16.03.2023 00:00, Bjorn Andersson wrote:
> > On Tue, Mar 14, 2023 at 12:41:45PM +0100, Konrad Dybcio wrote:
> >>
> >>
> >> On 14.03.2023 12:36, Konrad Dybcio wrote:
> >>>
> >>>
> >>> On 14.03.2023 01:10, Bjorn Andersson wrote:
> >>>> On Tue, Feb 14, 2023 at 10:54:35AM +0100, Konrad Dybcio wrote:
> >>>>> 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..
> >>>
> >>>>
> >>>> 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