diff mbox series

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

Message ID 20230214095435.2192153-3-konrad.dybcio@linaro.org
State New
Headers
Series [1/3] dt-bindings: power: qcom,rpmpd: Add SA8155P |

Commit Message

Konrad Dybcio Feb. 14, 2023, 9:54 a.m. UTC 
  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.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8155p-adp.dts |  2 +-
 arch/arm64/boot/dts/qcom/sa8155p.dtsi    | 51 ++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/qcom/sa8155p.dtsi

Comments

Bjorn Andersson March 14, 2023, 12:10 a.m. UTC | #1 
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.

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...

Regards,
Bjorn

> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sa8155p-adp.dts |  2 +-
>  arch/arm64/boot/dts/qcom/sa8155p.dtsi    | 51 ++++++++++++++++++++++++
>  2 files changed, 52 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/boot/dts/qcom/sa8155p.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> index 459384ec8f23..9454e8e4e517 100644
> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> @@ -7,7 +7,7 @@
>  
>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>  #include <dt-bindings/gpio/gpio.h>
> -#include "sm8150.dtsi"
> +#include "sa8155p.dtsi"
>  #include "pmm8155au_1.dtsi"
>  #include "pmm8155au_2.dtsi"
>  
> diff --git a/arch/arm64/boot/dts/qcom/sa8155p.dtsi b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
> new file mode 100644
> index 000000000000..f2fd7c28764e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + *
> + * SA8155P is an automotive variant of SM8150, with some minor changes.
> + * Most notably, the RPMhPD setup differs: MMCX and LCX/LMX rails are gone.
> + */
> +
> +#include "sm8150.dtsi"
> +
> +&dispcc {
> +	power-domains = <&rpmhpd SA8155P_CX>;
> +};
> +
> +&mdss_mdp {
> +	power-domains = <&rpmhpd SA8155P_CX>;
> +};
> +
> +&mdss_dsi0 {
> +	power-domains = <&rpmhpd SA8155P_CX>;
> +};
> +
> +&mdss_dsi1 {
> +	power-domains = <&rpmhpd SA8155P_CX>;
> +};
> +
> +&remoteproc_adsp {
> +	power-domains = <&rpmhpd SA8155P_CX>;
> +};
> +
> +&remoteproc_cdsp {
> +	power-domains = <&rpmhpd SA8155P_CX>;
> +};
> +
> +&remoteproc_mpss {
> +	power-domains = <&rpmhpd SA8155P_CX>,
> +			<&rpmhpd SA8155P_MSS>;
> +};
> +
> +&remoteproc_slpi {
> +	power-domains = <&rpmhpd SA8155P_CX>,
> +			<&rpmhpd SA8155P_MX>;
> +};
> +
> +&rpmhpd {
> +	compatible = "qcom,sa8155p-rpmhpd";
> +};
> +
> +&sdhc_2 {
> +	power-domains = <&rpmhpd SA8155P_CX>;
> +};
> -- 
> 2.39.1
>
Konrad Dybcio March 14, 2023, 11:36 a.m. UTC | #2 
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

Konrad
> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sa8155p-adp.dts |  2 +-
>>  arch/arm64/boot/dts/qcom/sa8155p.dtsi    | 51 ++++++++++++++++++++++++
>>  2 files changed, 52 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/arm64/boot/dts/qcom/sa8155p.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> index 459384ec8f23..9454e8e4e517 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> @@ -7,7 +7,7 @@
>>  
>>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>>  #include <dt-bindings/gpio/gpio.h>
>> -#include "sm8150.dtsi"
>> +#include "sa8155p.dtsi"
>>  #include "pmm8155au_1.dtsi"
>>  #include "pmm8155au_2.dtsi"
>>  
>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p.dtsi b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
>> new file mode 100644
>> index 000000000000..f2fd7c28764e
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
>> @@ -0,0 +1,51 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * Copyright (c) 2023, Linaro Limited
>> + *
>> + * SA8155P is an automotive variant of SM8150, with some minor changes.
>> + * Most notably, the RPMhPD setup differs: MMCX and LCX/LMX rails are gone.
>> + */
>> +
>> +#include "sm8150.dtsi"
>> +
>> +&dispcc {
>> +	power-domains = <&rpmhpd SA8155P_CX>;
>> +};
>> +
>> +&mdss_mdp {
>> +	power-domains = <&rpmhpd SA8155P_CX>;
>> +};
>> +
>> +&mdss_dsi0 {
>> +	power-domains = <&rpmhpd SA8155P_CX>;
>> +};
>> +
>> +&mdss_dsi1 {
>> +	power-domains = <&rpmhpd SA8155P_CX>;
>> +};
>> +
>> +&remoteproc_adsp {
>> +	power-domains = <&rpmhpd SA8155P_CX>;
>> +};
>> +
>> +&remoteproc_cdsp {
>> +	power-domains = <&rpmhpd SA8155P_CX>;
>> +};
>> +
>> +&remoteproc_mpss {
>> +	power-domains = <&rpmhpd SA8155P_CX>,
>> +			<&rpmhpd SA8155P_MSS>;
>> +};
>> +
>> +&remoteproc_slpi {
>> +	power-domains = <&rpmhpd SA8155P_CX>,
>> +			<&rpmhpd SA8155P_MX>;
>> +};
>> +
>> +&rpmhpd {
>> +	compatible = "qcom,sa8155p-rpmhpd";
>> +};
>> +
>> +&sdhc_2 {
>> +	power-domains = <&rpmhpd SA8155P_CX>;
>> +};
>> -- 
>> 2.39.1
>>
Konrad Dybcio March 14, 2023, 11:41 a.m. UTC | #3 
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 :/

Konrad
> 
> Konrad
>>
>> Regards,
>> Bjorn
>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sa8155p-adp.dts |  2 +-
>>>  arch/arm64/boot/dts/qcom/sa8155p.dtsi    | 51 ++++++++++++++++++++++++
>>>  2 files changed, 52 insertions(+), 1 deletion(-)
>>>  create mode 100644 arch/arm64/boot/dts/qcom/sa8155p.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>>> index 459384ec8f23..9454e8e4e517 100644
>>> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>>> @@ -7,7 +7,7 @@
>>>  
>>>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>>>  #include <dt-bindings/gpio/gpio.h>
>>> -#include "sm8150.dtsi"
>>> +#include "sa8155p.dtsi"
>>>  #include "pmm8155au_1.dtsi"
>>>  #include "pmm8155au_2.dtsi"
>>>  
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p.dtsi b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
>>> new file mode 100644
>>> index 000000000000..f2fd7c28764e
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
>>> @@ -0,0 +1,51 @@
>>> +// SPDX-License-Identifier: BSD-3-Clause
>>> +/*
>>> + * Copyright (c) 2023, Linaro Limited
>>> + *
>>> + * SA8155P is an automotive variant of SM8150, with some minor changes.
>>> + * Most notably, the RPMhPD setup differs: MMCX and LCX/LMX rails are gone.
>>> + */
>>> +
>>> +#include "sm8150.dtsi"
>>> +
>>> +&dispcc {
>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>> +};
>>> +
>>> +&mdss_mdp {
>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>> +};
>>> +
>>> +&mdss_dsi0 {
>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>> +};
>>> +
>>> +&mdss_dsi1 {
>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>> +};
>>> +
>>> +&remoteproc_adsp {
>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>> +};
>>> +
>>> +&remoteproc_cdsp {
>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>> +};
>>> +
>>> +&remoteproc_mpss {
>>> +	power-domains = <&rpmhpd SA8155P_CX>,
>>> +			<&rpmhpd SA8155P_MSS>;
>>> +};
>>> +
>>> +&remoteproc_slpi {
>>> +	power-domains = <&rpmhpd SA8155P_CX>,
>>> +			<&rpmhpd SA8155P_MX>;
>>> +};
>>> +
>>> +&rpmhpd {
>>> +	compatible = "qcom,sa8155p-rpmhpd";
>>> +};
>>> +
>>> +&sdhc_2 {
>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>> +};
>>> -- 
>>> 2.39.1
>>>
Bjorn Andersson March 15, 2023, 11 p.m. UTC | #4 
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.

Regards,
Bjorn

> Konrad
> > 
> > Konrad
> >>
> >> Regards,
> >> Bjorn
> >>
> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>> ---
> >>>  arch/arm64/boot/dts/qcom/sa8155p-adp.dts |  2 +-
> >>>  arch/arm64/boot/dts/qcom/sa8155p.dtsi    | 51 ++++++++++++++++++++++++
> >>>  2 files changed, 52 insertions(+), 1 deletion(-)
> >>>  create mode 100644 arch/arm64/boot/dts/qcom/sa8155p.dtsi
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> >>> index 459384ec8f23..9454e8e4e517 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> >>> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> >>> @@ -7,7 +7,7 @@
> >>>  
> >>>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> >>>  #include <dt-bindings/gpio/gpio.h>
> >>> -#include "sm8150.dtsi"
> >>> +#include "sa8155p.dtsi"
> >>>  #include "pmm8155au_1.dtsi"
> >>>  #include "pmm8155au_2.dtsi"
> >>>  
> >>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p.dtsi b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
> >>> new file mode 100644
> >>> index 000000000000..f2fd7c28764e
> >>> --- /dev/null
> >>> +++ b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
> >>> @@ -0,0 +1,51 @@
> >>> +// SPDX-License-Identifier: BSD-3-Clause
> >>> +/*
> >>> + * Copyright (c) 2023, Linaro Limited
> >>> + *
> >>> + * SA8155P is an automotive variant of SM8150, with some minor changes.
> >>> + * Most notably, the RPMhPD setup differs: MMCX and LCX/LMX rails are gone.
> >>> + */
> >>> +
> >>> +#include "sm8150.dtsi"
> >>> +
> >>> +&dispcc {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>;
> >>> +};
> >>> +
> >>> +&mdss_mdp {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>;
> >>> +};
> >>> +
> >>> +&mdss_dsi0 {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>;
> >>> +};
> >>> +
> >>> +&mdss_dsi1 {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>;
> >>> +};
> >>> +
> >>> +&remoteproc_adsp {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>;
> >>> +};
> >>> +
> >>> +&remoteproc_cdsp {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>;
> >>> +};
> >>> +
> >>> +&remoteproc_mpss {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>,
> >>> +			<&rpmhpd SA8155P_MSS>;
> >>> +};
> >>> +
> >>> +&remoteproc_slpi {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>,
> >>> +			<&rpmhpd SA8155P_MX>;
> >>> +};
> >>> +
> >>> +&rpmhpd {
> >>> +	compatible = "qcom,sa8155p-rpmhpd";
> >>> +};
> >>> +
> >>> +&sdhc_2 {
> >>> +	power-domains = <&rpmhpd SA8155P_CX>;
> >>> +};
> >>> -- 
> >>> 2.39.1
> >>>
Konrad Dybcio March 15, 2023, 11:50 p.m. UTC | #5 
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?

Konrad
> 
> Regards,
> Bjorn
> 
>> Konrad
>>>
>>> Konrad
>>>>
>>>> Regards,
>>>> Bjorn
>>>>
>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sa8155p-adp.dts |  2 +-
>>>>>  arch/arm64/boot/dts/qcom/sa8155p.dtsi    | 51 ++++++++++++++++++++++++
>>>>>  2 files changed, 52 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 arch/arm64/boot/dts/qcom/sa8155p.dtsi
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>>>>> index 459384ec8f23..9454e8e4e517 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>>>>> @@ -7,7 +7,7 @@
>>>>>  
>>>>>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>>>>>  #include <dt-bindings/gpio/gpio.h>
>>>>> -#include "sm8150.dtsi"
>>>>> +#include "sa8155p.dtsi"
>>>>>  #include "pmm8155au_1.dtsi"
>>>>>  #include "pmm8155au_2.dtsi"
>>>>>  
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p.dtsi b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
>>>>> new file mode 100644
>>>>> index 000000000000..f2fd7c28764e
>>>>> --- /dev/null
>>>>> +++ b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
>>>>> @@ -0,0 +1,51 @@
>>>>> +// SPDX-License-Identifier: BSD-3-Clause
>>>>> +/*
>>>>> + * Copyright (c) 2023, Linaro Limited
>>>>> + *
>>>>> + * SA8155P is an automotive variant of SM8150, with some minor changes.
>>>>> + * Most notably, the RPMhPD setup differs: MMCX and LCX/LMX rails are gone.
>>>>> + */
>>>>> +
>>>>> +#include "sm8150.dtsi"
>>>>> +
>>>>> +&dispcc {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>>>> +};
>>>>> +
>>>>> +&mdss_mdp {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>>>> +};
>>>>> +
>>>>> +&mdss_dsi0 {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>>>> +};
>>>>> +
>>>>> +&mdss_dsi1 {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>>>> +};
>>>>> +
>>>>> +&remoteproc_adsp {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>>>> +};
>>>>> +
>>>>> +&remoteproc_cdsp {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>>>> +};
>>>>> +
>>>>> +&remoteproc_mpss {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>,
>>>>> +			<&rpmhpd SA8155P_MSS>;
>>>>> +};
>>>>> +
>>>>> +&remoteproc_slpi {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>,
>>>>> +			<&rpmhpd SA8155P_MX>;
>>>>> +};
>>>>> +
>>>>> +&rpmhpd {
>>>>> +	compatible = "qcom,sa8155p-rpmhpd";
>>>>> +};
>>>>> +
>>>>> +&sdhc_2 {
>>>>> +	power-domains = <&rpmhpd SA8155P_CX>;
>>>>> +};
>>>>> -- 
>>>>> 2.39.1
>>>>>
Bjorn Andersson March 20, 2023, 2:19 a.m. UTC | #6 
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
Konrad Dybcio March 20, 2023, 10:39 a.m. UTC | #7 
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
Konrad Dybcio March 29, 2023, 7:25 p.m. UTC | #8 
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
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
index 459384ec8f23..9454e8e4e517 100644
--- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
@@ -7,7 +7,7 @@ 
 
 #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
 #include <dt-bindings/gpio/gpio.h>
-#include "sm8150.dtsi"
+#include "sa8155p.dtsi"
 #include "pmm8155au_1.dtsi"
 #include "pmm8155au_2.dtsi"
 
diff --git a/arch/arm64/boot/dts/qcom/sa8155p.dtsi b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
new file mode 100644
index 000000000000..f2fd7c28764e
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sa8155p.dtsi
@@ -0,0 +1,51 @@ 
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2023, Linaro Limited
+ *
+ * SA8155P is an automotive variant of SM8150, with some minor changes.
+ * Most notably, the RPMhPD setup differs: MMCX and LCX/LMX rails are gone.
+ */
+
+#include "sm8150.dtsi"
+
+&dispcc {
+	power-domains = <&rpmhpd SA8155P_CX>;
+};
+
+&mdss_mdp {
+	power-domains = <&rpmhpd SA8155P_CX>;
+};
+
+&mdss_dsi0 {
+	power-domains = <&rpmhpd SA8155P_CX>;
+};
+
+&mdss_dsi1 {
+	power-domains = <&rpmhpd SA8155P_CX>;
+};
+
+&remoteproc_adsp {
+	power-domains = <&rpmhpd SA8155P_CX>;
+};
+
+&remoteproc_cdsp {
+	power-domains = <&rpmhpd SA8155P_CX>;
+};
+
+&remoteproc_mpss {
+	power-domains = <&rpmhpd SA8155P_CX>,
+			<&rpmhpd SA8155P_MSS>;
+};
+
+&remoteproc_slpi {
+	power-domains = <&rpmhpd SA8155P_CX>,
+			<&rpmhpd SA8155P_MX>;
+};
+
+&rpmhpd {
+	compatible = "qcom,sa8155p-rpmhpd";
+};
+
+&sdhc_2 {
+	power-domains = <&rpmhpd SA8155P_CX>;
+};