mbox series

[v2,00/22] arm64: dts: qcom: remove duplication in PMIC declarations

Message ID 20230401220810.3563708-1-dmitry.baryshkov@linaro.org
Headers show
Series arm64: dts: qcom: remove duplication in PMIC declarations | expand

Message

Dmitry Baryshkov April 1, 2023, 10:07 p.m. UTC
The sc8280xp platform uses its own copy of PMIC declarations. This can
easily end up with the issues that are fixed in the main PMIC include
file, but are not fixed for sc8280xp (and vice versa). For example
commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON
compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the
PON device, while sc8280xp-pmic.dtsi still has the incorrect
"qcom,pm8998-pon".

Another example is pm8280_2_temp_alarm device, which uses interrupts
tied to SID 2, while having SID 3. This can be easily left unnoticed.

Employ a small amount of C preprocessor magic to make
sc8280xp-pmics.dtsi use standard PMIC include files

Also apply the same approach to sa8540p-pmics/pm8150.

Jonathan Cameron Acked merging the dt-bindigns patch together with the
rest of the patches to simplify merge process.

Dmitry Baryshkov (22):
  arm64: dts: qcom: pm8350: fix thermal zone node name
  arm64: dts: qcom: pm8350b: fix thermal zone node name
  arm64: dts: qcom: sc8280xp-pmics: use pmk8350 specifics for pon device
  arm64: dts: qcom: sc8280xp-pmics: correct interrupt routing for
    pm8280_2_temp_alarm
  dt-bindings: iio: qcom,spmi-adc7-pmk8350.h: include sid into defines
  arm64: dts: qcom: pmk8350: rename pon label
  arm64: dts: qcom: pmk8350: port sdam_6 device from sc8280xp-pmics
  arm64: dts: qcom: pmk8350: rename PMK8350_SID to PMIC_SID
  arm64: dts: qcom: pmk8350: allow overriding the label
  arm64: dts: qcom: pmk8350: use interrupts-extended for IRQ
    specification
  arm64: dts: qcom: sc8280xp*: use pmk8350.dtsi
  arm64: dts: qcom: pm8350: allow overriding SID and label
  arm64: dts: qcom: pm8350: use interrupts-extended for IRQ
    specification
  arm64: dts: qcom: sc8280xp*: use pm8350.dtsi
  arm64: dts: qcom: pm8350c: move thermal zone declaration to the top
  arm64: dts: qcom: pm8350c: allow overriding SID and label
  arm64: dts: qcom: pm8350c: use interrupts-extended for IRQ
    specification
  arm64: dts: qcom: sc8280xp*: use pm8350c.dtsi
  arm64: dts: qcom: sc8280xp*: use pmr735a.dtsi
  arm64: dts: qcom: pm8150: convert to use dynamic SID/LABEL
  arch: arm64: dts: qcom: pm8150: support SID greater that 9
  arm64: dts: qcom sa8540p-pmics: switch to pm8150.dtsi

 .../bindings/iio/adc/qcom,spmi-vadc.yaml      |   2 +-
 .../bindings/thermal/qcom-spmi-adc-tm5.yaml   |   4 +-
 arch/arm64/boot/dts/qcom/pm8150.dtsi          |  53 +++--
 arch/arm64/boot/dts/qcom/pm8350.dtsi          |  33 ++-
 arch/arm64/boot/dts/qcom/pm8350b.dtsi         |   6 +-
 arch/arm64/boot/dts/qcom/pm8350c.dtsi         |  73 +++---
 arch/arm64/boot/dts/qcom/pmic-dyn-footer.dtsi |  23 ++
 arch/arm64/boot/dts/qcom/pmic-dyn-header.dtsi |  26 +++
 arch/arm64/boot/dts/qcom/pmk8350.dtsi         |  51 ++--
 arch/arm64/boot/dts/qcom/sa8540p-pmics.dtsi   |  96 ++------
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi      |   2 +-
 arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi    |   2 +-
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts     |   4 +-
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    |   8 +-
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi  | 221 ++----------------
 .../qcom/sm6375-sony-xperia-murray-pdx225.dts |   7 +-
 .../boot/dts/qcom/sm7225-fairphone-fp4.dts    |   8 +-
 arch/arm64/boot/dts/qcom/sm8350-mtp.dts       |   8 +-
 .../dts/qcom/sm8350-sony-xperia-sagami.dtsi   |   8 +-
 .../dts/qcom/sm8450-sony-xperia-nagara.dtsi   |   4 +-
 .../dt-bindings/iio/qcom,spmi-adc7-pmk8350.h  |  52 ++---
 21 files changed, 279 insertions(+), 412 deletions(-)
 create mode 100644 arch/arm64/boot/dts/qcom/pmic-dyn-footer.dtsi
 create mode 100644 arch/arm64/boot/dts/qcom/pmic-dyn-header.dtsi

Comments

Krzysztof Kozlowski April 2, 2023, 9:42 a.m. UTC | #1
On 02/04/2023 00:07, Dmitry Baryshkov wrote:
> Following the commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the
> correct PON compatible") and commit f46ef374e0dc ("arm64: dts: qcom:
> pmk8350: Specify PBS register for PON") use "qcom,pmk8350-pon" compat
> string and add RBS region to the PON device.
> 
> Fixes: ccd3517faf18 ("arm64: dts: qcom: sc8280xp: Add reference device")

There is no compatible qcom,pmk8350-pon documented at ccd3517faf18, so
backporting it there is incorrect. qcom,pmk8350-pon is neither in v5.19
nor in v6.0.


Best regards,
Krzysztof
Krzysztof Kozlowski April 2, 2023, 9:47 a.m. UTC | #2
On 02/04/2023 00:08, Dmitry Baryshkov wrote:
> SA8450p-based platforms have 4 instances of pm8150. Convert pm8150.dtsi
> to use pmic-dyn-header.dtsi in order to support dynamic and label
> assignment.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/pm8150.dtsi          | 53 ++++++++++++-------
>  arch/arm64/boot/dts/qcom/pmic-dyn-footer.dtsi |  1 +
>  2 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi
> index db90c55fa2cf..77bb325e425b 100644
> --- a/arch/arm64/boot/dts/qcom/pm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi
> @@ -9,13 +9,28 @@
>  #include <dt-bindings/spmi/spmi.h>
>  #include <dt-bindings/iio/qcom,spmi-vadc.h>
>  
> +/* (Sadly) this PMIC can be configured to be at different SIDs */
> +#ifndef PMIC_SID
> +	#define PMIC_SID 0
> +#endif

No, the DTS code must be simple, no ifndefs for some defines. This means
that sometimes you expect here define, sometimes not. It's not easy to
maintain and understand the code. Define must be simple and always
defined, not sometimes.

> +
> +#ifndef PMIC_SID1
> +	#define PMIC_SID1 1
> +#endif
> +
> +#ifndef PMIC_LABEL
> +	#define PMIC_LABEL pm8150
> +#endif
> +
> +#include "pmic-dyn-header.dtsi"
> +
>  / {
>  	thermal-zones {
> -		pm8150-thermal {
> +		NODE(thermal) {
>  			polling-delay-passive = <100>;
>  			polling-delay = <0>;
>  
> -			thermal-sensors = <&pm8150_temp>;
> +			thermal-sensors = <&LABEL(temp)>;
>  
>  			trips {
>  				trip0 {
> @@ -41,9 +56,9 @@ trip2 {
>  };
>  
>  &spmi_bus {
> -	pm8150_0: pmic@0 {
> +	pmic@0 {
>  		compatible = "qcom,pm8150", "qcom,spmi-pmic";
> -		reg = <0x0 SPMI_USID>;
> +		reg = <PMIC_SID SPMI_USID>;
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
> @@ -55,7 +70,7 @@ pon: pon@800 {
>  
>  			pon_pwrkey: pwrkey {
>  				compatible = "qcom,pm8941-pwrkey";
> -				interrupts = <0x0 0x8 0x0 IRQ_TYPE_EDGE_BOTH>;
> +				interrupts = <PMIC_SID 0x8 0x0 IRQ_TYPE_EDGE_BOTH>;
>  				debounce = <15625>;
>  				bias-pull-up;
>  				linux,code = <KEY_POWER>;
> @@ -65,7 +80,7 @@ pon_pwrkey: pwrkey {
>  
>  			pon_resin: resin {
>  				compatible = "qcom,pm8941-resin";
> -				interrupts = <0x0 0x8 0x1 IRQ_TYPE_EDGE_BOTH>;
> +				interrupts = <PMIC_SID 0x8 0x1 IRQ_TYPE_EDGE_BOTH>;
>  				debounce = <15625>;
>  				bias-pull-up;
>  
> @@ -73,22 +88,22 @@ pon_resin: resin {
>  			};
>  		};
>  
> -		pm8150_temp: temp-alarm@2400 {
> +		LABEL(temp): temp-alarm@2400 {

NAK for all defines creating labels.


Best regards,
Krzysztof
Krzysztof Kozlowski April 2, 2023, 9:55 a.m. UTC | #3
On 02/04/2023 00:07, Dmitry Baryshkov wrote:
> The sc8280xp platform uses its own copy of PMIC declarations. This can
> easily end up with the issues that are fixed in the main PMIC include
> file, but are not fixed for sc8280xp (and vice versa). For example
> commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON
> compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the
> PON device, while sc8280xp-pmic.dtsi still has the incorrect
> "qcom,pm8998-pon".
> 
> Another example is pm8280_2_temp_alarm device, which uses interrupts
> tied to SID 2, while having SID 3. This can be easily left unnoticed.
> 
> Employ a small amount of C preprocessor magic to make
> sc8280xp-pmics.dtsi use standard PMIC include files

Preprocessor magic is disliked in DTS. We allow only simple defines, no
undefs. Sometimes some nodes or strings could be concatenated, but in
obvious way. You should not parametrize it and have different, generated
labels in DTS based on something coming external to that DTS.

Best regards,
Krzysztof
Dmitry Baryshkov April 2, 2023, 10:25 a.m. UTC | #4
On Sun, 2 Apr 2023 at 12:42, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
> > Following the commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the
> > correct PON compatible") and commit f46ef374e0dc ("arm64: dts: qcom:
> > pmk8350: Specify PBS register for PON") use "qcom,pmk8350-pon" compat
> > string and add RBS region to the PON device.
> >
> > Fixes: ccd3517faf18 ("arm64: dts: qcom: sc8280xp: Add reference device")
>
> There is no compatible qcom,pmk8350-pon documented at ccd3517faf18, so
> backporting it there is incorrect. qcom,pmk8350-pon is neither in v5.19
> nor in v6.0.

Well, according to Documentation/process/submitting-patches.rst, Fixes
tag is about noting that there was an issue fixed in the commit. The
mentioned commit has an issue, as the device should have a second
region. I did not intend to have this patch backported (no Cc stable).
If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce.
Krzysztof Kozlowski April 2, 2023, 10:32 a.m. UTC | #5
On 02/04/2023 12:25, Dmitry Baryshkov wrote:
> On Sun, 2 Apr 2023 at 12:42, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>>> Following the commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the
>>> correct PON compatible") and commit f46ef374e0dc ("arm64: dts: qcom:
>>> pmk8350: Specify PBS register for PON") use "qcom,pmk8350-pon" compat
>>> string and add RBS region to the PON device.
>>>
>>> Fixes: ccd3517faf18 ("arm64: dts: qcom: sc8280xp: Add reference device")
>>
>> There is no compatible qcom,pmk8350-pon documented at ccd3517faf18, so
>> backporting it there is incorrect. qcom,pmk8350-pon is neither in v5.19
>> nor in v6.0.
> 
> Well, according to Documentation/process/submitting-patches.rst, Fixes
> tag is about noting that there was an issue fixed in the commit. The
> mentioned commit has an issue, as the device should have a second

Depends. If device was working in some limited way with old compatible
and one region, there is nothing to fix maybe. It was just incomplete.

If second region is needed for the work, then only that commit should be
marked as fix. Changing compatible is not a fix of that submission
because at the time, the compatible was correct. That time in Git
history, the "qcom,pmk8350-pon" was not correct.

> region. I did not intend to have this patch backported (no Cc stable).
> If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce.

AUTOSEL will backport it anyway, if you do not mention otherwise.

Best regards,
Krzysztof
Dmitry Baryshkov April 2, 2023, 11:03 a.m. UTC | #6
On 02/04/2023 13:32, Krzysztof Kozlowski wrote:
> On 02/04/2023 12:25, Dmitry Baryshkov wrote:
>> On Sun, 2 Apr 2023 at 12:42, Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>>>> Following the commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the
>>>> correct PON compatible") and commit f46ef374e0dc ("arm64: dts: qcom:
>>>> pmk8350: Specify PBS register for PON") use "qcom,pmk8350-pon" compat
>>>> string and add RBS region to the PON device.
>>>>
>>>> Fixes: ccd3517faf18 ("arm64: dts: qcom: sc8280xp: Add reference device")
>>>
>>> There is no compatible qcom,pmk8350-pon documented at ccd3517faf18, so
>>> backporting it there is incorrect. qcom,pmk8350-pon is neither in v5.19
>>> nor in v6.0.
>>
>> Well, according to Documentation/process/submitting-patches.rst, Fixes
>> tag is about noting that there was an issue fixed in the commit. The
>> mentioned commit has an issue, as the device should have a second
> 
> Depends. If device was working in some limited way with old compatible
> and one region, there is nothing to fix maybe. It was just incomplete.
> 
> If second region is needed for the work, then only that commit should be
> marked as fix. Changing compatible is not a fix of that submission
> because at the time, the compatible was correct. That time in Git
> history, the "qcom,pmk8350-pon" was not correct.

Ack, so we have to backport the schema too.

> 
>> region. I did not intend to have this patch backported (no Cc stable).
>> If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce.
> 
> AUTOSEL will backport it anyway, if you do not mention otherwise.

Is there a way to influence AUTOSEL to make it also pick up another commit?

> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski April 2, 2023, 11:12 a.m. UTC | #7
On 02/04/2023 13:03, Dmitry Baryshkov wrote:
> 
>>
>>> region. I did not intend to have this patch backported (no Cc stable).
>>> If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce.
>>
>> AUTOSEL will backport it anyway, if you do not mention otherwise.
> 
> Is there a way to influence AUTOSEL to make it also pick up another commit?

Only via usual stable kernel patch rules/syntax.

Best regards,
Krzysztof
Konrad Dybcio April 3, 2023, 10:06 a.m. UTC | #8
On 2.04.2023 13:12, Krzysztof Kozlowski wrote:
> On 02/04/2023 13:03, Dmitry Baryshkov wrote:
>>
>>>
>>>> region. I did not intend to have this patch backported (no Cc stable).
>>>> If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce.
>>>
>>> AUTOSEL will backport it anyway, if you do not mention otherwise.
>>
>> Is there a way to influence AUTOSEL to make it also pick up another commit?
> 
> Only via usual stable kernel patch rules/syntax.

Additionally, some patches submitted via Option 1 may have additional patch prerequisites which can be cherry-picked. This can be specified in the following format in the sign-off area:

Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
Cc: <stable@vger.kernel.org> # 3.3.x: fd21073: sched: Fix affinity logic
Cc: <stable@vger.kernel.org> # 3.3.x
Signed-off-by: Ingo Molnar <mingo@elte.hu>

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html


Konrad

> 
> Best regards,
> Krzysztof
>
Konrad Dybcio April 3, 2023, 10:44 a.m. UTC | #9
On 2.04.2023 11:55, Krzysztof Kozlowski wrote:
> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>> The sc8280xp platform uses its own copy of PMIC declarations. This can
>> easily end up with the issues that are fixed in the main PMIC include
>> file, but are not fixed for sc8280xp (and vice versa). For example
>> commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON
>> compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the
>> PON device, while sc8280xp-pmic.dtsi still has the incorrect
>> "qcom,pm8998-pon".
>>
>> Another example is pm8280_2_temp_alarm device, which uses interrupts
>> tied to SID 2, while having SID 3. This can be easily left unnoticed.
>>
>> Employ a small amount of C preprocessor magic to make
>> sc8280xp-pmics.dtsi use standard PMIC include files
> 
> Preprocessor magic is disliked in DTS. We allow only simple defines, no
> undefs. Sometimes some nodes or strings could be concatenated, but in
> obvious way. You should not parametrize it and have different, generated
> labels in DTS based on something coming external to that DTS.
This again begs the question, is it time we start moving parts of the
dts code to be autogenerated?

Should we keep a separate file for each SID?

Or should we consider the SPMI 'interrupts' implementation flawed and
work towards one that does not require a SID to be specified within?

Currently it's:

interrupts = <USID PERIPH_ADDR>>8 IRQ_WITHIN_PERIPH IRQ_TYPE>;

So the first two cells are effectively useless and can be retrieved
from the parent node and the reg property.

Getting rid of that would solve a decent chunk of problems that this
patchset concerns.

Konrad

> 
> Best regards,
> Krzysztof
>
Dmitry Baryshkov April 3, 2023, 11:37 a.m. UTC | #10
On Mon, 3 Apr 2023 at 13:44, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 2.04.2023 11:55, Krzysztof Kozlowski wrote:
> > On 02/04/2023 00:07, Dmitry Baryshkov wrote:
> >> The sc8280xp platform uses its own copy of PMIC declarations. This can
> >> easily end up with the issues that are fixed in the main PMIC include
> >> file, but are not fixed for sc8280xp (and vice versa). For example
> >> commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON
> >> compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the
> >> PON device, while sc8280xp-pmic.dtsi still has the incorrect
> >> "qcom,pm8998-pon".
> >>
> >> Another example is pm8280_2_temp_alarm device, which uses interrupts
> >> tied to SID 2, while having SID 3. This can be easily left unnoticed.
> >>
> >> Employ a small amount of C preprocessor magic to make
> >> sc8280xp-pmics.dtsi use standard PMIC include files
> >
> > Preprocessor magic is disliked in DTS. We allow only simple defines, no
> > undefs. Sometimes some nodes or strings could be concatenated, but in
> > obvious way. You should not parametrize it and have different, generated
> > labels in DTS based on something coming external to that DTS.
> This again begs the question, is it time we start moving parts of the
> dts code to be autogenerated?
>
> Should we keep a separate file for each SID?
>
> Or should we consider the SPMI 'interrupts' implementation flawed and
> work towards one that does not require a SID to be specified within?
>
> Currently it's:
>
> interrupts = <USID PERIPH_ADDR>>8 IRQ_WITHIN_PERIPH IRQ_TYPE>;
>
> So the first two cells are effectively useless and can be retrieved
> from the parent node and the reg property.
>
> Getting rid of that would solve a decent chunk of problems that this
> patchset concerns.

While I agree with the USID part, the PERIPH_ADDR part is not always
like that, see pm8916.dtsi / audio-codec@f000.

I'll give it a thought, but it is probably not in the forthcoming future.
Krzysztof Kozlowski April 3, 2023, 1 p.m. UTC | #11
On 03/04/2023 12:44, Konrad Dybcio wrote:
> 
> 
> On 2.04.2023 11:55, Krzysztof Kozlowski wrote:
>> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>>> The sc8280xp platform uses its own copy of PMIC declarations. This can
>>> easily end up with the issues that are fixed in the main PMIC include
>>> file, but are not fixed for sc8280xp (and vice versa). For example
>>> commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON
>>> compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the
>>> PON device, while sc8280xp-pmic.dtsi still has the incorrect
>>> "qcom,pm8998-pon".
>>>
>>> Another example is pm8280_2_temp_alarm device, which uses interrupts
>>> tied to SID 2, while having SID 3. This can be easily left unnoticed.
>>>
>>> Employ a small amount of C preprocessor magic to make
>>> sc8280xp-pmics.dtsi use standard PMIC include files
>>
>> Preprocessor magic is disliked in DTS. We allow only simple defines, no
>> undefs. Sometimes some nodes or strings could be concatenated, but in
>> obvious way. You should not parametrize it and have different, generated
>> labels in DTS based on something coming external to that DTS.
> This again begs the question, is it time we start moving parts of the
> dts code to be autogenerated?

Do we auto-generate C-code? Just a bit, but in general no. There are
exceptions but coding style is here clear:
https://elixir.bootlin.com/linux/v6.1-rc2/source/Documentation/process/coding-style.rst#L828

Pre-processor generated code should be narrowed to some cases or simpler
structures.

For DTS we actually are even stricter.


Best regards,
Krzysztof