Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5d8a7b76-482b-99fe-c55b-6d3e3d2d8394@linaro.org>
Date:   Wed, 29 Mar 2023 21:25:16 +0200
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
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
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>
 <6ae97b26-b1e7-b382-b6f6-053afe26a1a2@linaro.org>
In-Reply-To: <6ae97b26-b1e7-b382-b6f6-053afe26a1a2@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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