Message-ID: <6ae97b26-b1e7-b382-b6f6-053afe26a1a2@linaro.org>
Date:   Mon, 20 Mar 2023 11:39:56 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH 3/3] arm64: dts: qcom: Split out SA8155P and use correct
 RPMh power domains
Content-Language: en-US
To:     Bjorn Andersson <andersson@kernel.org>
Cc:     linux-arm-msm@vger.kernel.org, agross@kernel.org,
        krzysztof.kozlowski@linaro.org, marijn.suijten@somainline.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230214095435.2192153-1-konrad.dybcio@linaro.org>
 <20230214095435.2192153-3-konrad.dybcio@linaro.org>
 <20230314001052.7qvgbwkl73x22oll@ripper>
 <eaf2ca0d-4d90-b68b-3b36-8bb0148cfb95@linaro.org>
 <ee1ebac4-bf18-019a-f770-5cb88703d06b@linaro.org>
 <20230315230024.wxuqthay74i5zgrq@ripper>
 <3d3117d2-b3eb-1174-7061-b899cdcdf6ce@linaro.org>
 <20230320021957.yzg6zhrhjr36rcz4@ripper>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <20230320021957.yzg6zhrhjr36rcz4@ripper>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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