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

[Konrad Dybcio]

On 20.03.2023 03:19, Bjorn Andersson wrote:
> 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_?)
Perhaps just VDD_{CX/MX/..}? We reference the rpm(h)pd's phandle
each time it's used, anyway.

Konrad
> 
> Regards,
> Bjorn