[PATCH 1/3] dt-bindings: power: qcom,rpmpd: Add SA8155P

[PATCH 1/3] dt-bindings: power: qcom,rpmpd: Add SA8155P

[Konrad Dybcio]

Add a compatible for SA8155P platforms and relevant defines to the
include file.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 Documentation/devicetree/bindings/power/qcom,rpmpd.yaml | 1 +
 include/dt-bindings/power/qcom-rpmpd.h                  | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml b/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
index afad3135ed67..f9c211a9a938 100644
--- a/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
+++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
@@ -29,6 +29,7 @@ properties:
       - qcom,qcm2290-rpmpd
       - qcom,qcs404-rpmpd
       - qcom,qdu1000-rpmhpd
+      - qcom,sa8155p-rpmhpd
       - qcom,sa8540p-rpmhpd
       - qcom,sa8775p-rpmhpd
       - qcom,sdm660-rpmpd
diff --git a/include/dt-bindings/power/qcom-rpmpd.h b/include/dt-bindings/power/qcom-rpmpd.h
index 1bf8e87ecd7e..5a6dd5ded087 100644
--- a/include/dt-bindings/power/qcom-rpmpd.h
+++ b/include/dt-bindings/power/qcom-rpmpd.h
@@ -4,6 +4,15 @@
 #ifndef _DT_BINDINGS_POWER_QCOM_RPMPD_H
 #define _DT_BINDINGS_POWER_QCOM_RPMPD_H
 
+/* SA8155P Power Domain Indexes */
+#define SA8155P_CX	0
+#define SA8155P_CX_AO	1
+#define SA8155P_EBI	2
+#define SA8155P_GFX	3
+#define SA8155P_MSS	4
+#define SA8155P_MX	5
+#define SA8155P_MX_AO	6
+
 /* SA8775P Power Domain Indexes */
 #define SA8775P_CX	0
 #define SA8775P_CX_AO	1
-- 
2.39.1

[PATCH 2/3] soc: qcom: rpmhpd: Add SA8155P power domains

[Konrad Dybcio]

Add the power domains exposed by RPMh in the Qualcomm SA8155P platform.
Turns out they differ from SM8150.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/soc/qcom/rpmhpd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
index f20e2a49a669..63c35a32065b 100644
--- a/drivers/soc/qcom/rpmhpd.c
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -342,6 +342,21 @@ static const struct rpmhpd_desc sm8150_desc = {
 	.num_pds = ARRAY_SIZE(sm8150_rpmhpds),
 };
 
+static struct rpmhpd *sa8155p_rpmhpds[] = {
+	[SA8155P_CX] = &cx_w_mx_parent,
+	[SA8155P_CX_AO] = &cx_ao_w_mx_parent,
+	[SA8155P_EBI] = &ebi,
+	[SA8155P_GFX] = &gfx,
+	[SA8155P_MSS] = &mss,
+	[SA8155P_MX] = &mx,
+	[SA8155P_MX_AO] = &mx_ao,
+};
+
+static const struct rpmhpd_desc sa8155p_desc = {
+	.rpmhpds = sa8155p_rpmhpds,
+	.num_pds = ARRAY_SIZE(sa8155p_rpmhpds),
+};
+
 /* SM8250 RPMH powerdomains */
 static struct rpmhpd *sm8250_rpmhpds[] = {
 	[SM8250_CX] = &cx_w_mx_parent,
@@ -519,6 +534,7 @@ static const struct rpmhpd_desc sc8280xp_desc = {
 
 static const struct of_device_id rpmhpd_match_table[] = {
 	{ .compatible = "qcom,qdu1000-rpmhpd", .data = &qdu1000_desc },
+	{ .compatible = "qcom,sa8155p-rpmhpd", .data = &sa8155p_desc },
 	{ .compatible = "qcom,sa8540p-rpmhpd", .data = &sa8540p_desc },
 	{ .compatible = "qcom,sa8775p-rpmhpd", .data = &sa8775p_desc },
 	{ .compatible = "qcom,sc7180-rpmhpd", .data = &sc7180_desc },
-- 
2.39.1

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

[Konrad Dybcio]

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

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

Re: [PATCH 1/3] dt-bindings: power: qcom,rpmpd: Add SA8155P

[Krzysztof Kozlowski]

On 14/02/2023 10:54, Konrad Dybcio wrote:
> Add a compatible for SA8155P platforms and relevant defines to the
> include file.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: power: qcom,rpmpd: Add SA8155P

[Bhupesh Sharma]

On Tue, 14 Feb 2023 at 15:25, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> Add a compatible for SA8155P platforms and relevant defines to the
> include file.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  Documentation/devicetree/bindings/power/qcom,rpmpd.yaml | 1 +
>  include/dt-bindings/power/qcom-rpmpd.h                  | 9 +++++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml b/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
> index afad3135ed67..f9c211a9a938 100644
> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
> @@ -29,6 +29,7 @@ properties:
>        - qcom,qcm2290-rpmpd
>        - qcom,qcs404-rpmpd
>        - qcom,qdu1000-rpmhpd
> +      - qcom,sa8155p-rpmhpd
>        - qcom,sa8540p-rpmhpd
>        - qcom,sa8775p-rpmhpd
>        - qcom,sdm660-rpmpd
> diff --git a/include/dt-bindings/power/qcom-rpmpd.h b/include/dt-bindings/power/qcom-rpmpd.h
> index 1bf8e87ecd7e..5a6dd5ded087 100644
> --- a/include/dt-bindings/power/qcom-rpmpd.h
> +++ b/include/dt-bindings/power/qcom-rpmpd.h
> @@ -4,6 +4,15 @@
>  #ifndef _DT_BINDINGS_POWER_QCOM_RPMPD_H
>  #define _DT_BINDINGS_POWER_QCOM_RPMPD_H
>
> +/* SA8155P Power Domain Indexes */
> +#define SA8155P_CX     0
> +#define SA8155P_CX_AO  1
> +#define SA8155P_EBI    2
> +#define SA8155P_GFX    3
> +#define SA8155P_MSS    4
> +#define SA8155P_MX     5
> +#define SA8155P_MX_AO  6
> +
>  /* SA8775P Power Domain Indexes */
>  #define SA8775P_CX     0
>  #define SA8775P_CX_AO  1
> --
> 2.39.1

Tested on my SA8155p-ADP, so for the series:

Reviewed-and-Tested-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>

Thanks.

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

[Bjorn Andersson]

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
> 

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

[Konrad Dybcio]

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

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

[Konrad Dybcio]

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

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

[Bjorn Andersson]

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

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

[Konrad Dybcio]

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

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

[Bjorn Andersson]

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

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

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

Re: (subset) [PATCH 1/3] dt-bindings: power: qcom,rpmpd: Add SA8155P

[Bjorn Andersson]

On Tue, 14 Feb 2023 10:54:33 +0100, Konrad Dybcio wrote:
> Add a compatible for SA8155P platforms and relevant defines to the
> include file.
> 
> 

Applied, thanks!

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

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>