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

[Konrad Dybcio]

On 20.03.2023 11:39, Konrad Dybcio wrote:
> 
> 
> 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.
So, back to this patch.. do you want me to make any changes or should
we take it as-is to fix 8155?

Konrad
> 
> Konrad
>>
>> Regards,
>> Bjorn