diff mbox series

[v1,2/2] arm64: dts: qcom: ipq9574: Fix USB 'vdda-pll-supply'

Message ID f98bbf0a515236709d999010f08c8f2470a31209.1701160842.git.varada@hu-varada-blr.qualcomm.com (mailing list archive)
State Changes Requested
Headers show
Series ipq9574: Fix USB 'vdda-pll-supply' | expand

Commit Message

Varadarajan Narayanan Nov. 28, 2023, 8:46 a.m. UTC 
From: Varadarajan Narayanan <quic_varada@quicinc.com>

The earlier patch ec4f047679d5, incorrectly used 'l2'
as the vdda-pll-supply. However, 'l5' is the correct
ldo that supplies power to the USB PHY.

Fixes: ec4f047679d5 ("arm64: dts: qcom: ipq9574: Enable USB")
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski Nov. 28, 2023, 8:51 a.m. UTC | #1 
On 28/11/2023 09:46, Varadarajan Narayanan wrote:
> From: Varadarajan Narayanan <quic_varada@quicinc.com>
> 
> The earlier patch ec4f047679d5, incorrectly used 'l2'
> as the vdda-pll-supply. However, 'l5' is the correct
> ldo that supplies power to the USB PHY.
> 
> Fixes: ec4f047679d5 ("arm64: dts: qcom: ipq9574: Enable USB")

Doesn't this depend on the driver change? It affects both existing
kernel and backports which you claim here should happen.

Best regards,
Krzysztof
Varadarajan Narayanan Nov. 28, 2023, 10:14 a.m. UTC | #2 
On Tue, Nov 28, 2023 at 09:51:50AM +0100, Krzysztof Kozlowski wrote:
> On 28/11/2023 09:46, Varadarajan Narayanan wrote:
> > From: Varadarajan Narayanan <quic_varada@quicinc.com>
> >
> > The earlier patch ec4f047679d5, incorrectly used 'l2'
> > as the vdda-pll-supply. However, 'l5' is the correct
> > ldo that supplies power to the USB PHY.
> >
> > Fixes: ec4f047679d5 ("arm64: dts: qcom: ipq9574: Enable USB")
>
> Doesn't this depend on the driver change?

Yes, will mention in the cover letter.

> It affects both existing
> kernel and backports which you claim here should happen.

Ok. Will include stable@vger.kernel.org in the next revision.

Thanks
Varada
Krzysztof Kozlowski Nov. 28, 2023, 2:01 p.m. UTC | #3 
On 28/11/2023 11:14, Varadarajan Narayanan wrote:
> On Tue, Nov 28, 2023 at 09:51:50AM +0100, Krzysztof Kozlowski wrote:
>> On 28/11/2023 09:46, Varadarajan Narayanan wrote:
>>> From: Varadarajan Narayanan <quic_varada@quicinc.com>
>>>
>>> The earlier patch ec4f047679d5, incorrectly used 'l2'
>>> as the vdda-pll-supply. However, 'l5' is the correct
>>> ldo that supplies power to the USB PHY.
>>>
>>> Fixes: ec4f047679d5 ("arm64: dts: qcom: ipq9574: Enable USB")
>>
>> Doesn't this depend on the driver change?
> 
> Yes, will mention in the cover letter.

This commit should have it in its changelog ---

> 
>> It affects both existing
>> kernel and backports which you claim here should happen.
> 
> Ok. Will include stable@vger.kernel.org in the next revision.

I wasn't speaking about Cc. You indicated this should be backported.
Then please backport it, without previous commit, and check the result.
Is stable tree working correctly or not?

Best regards,
Krzysztof
Varadarajan Narayanan Dec. 6, 2023, 11:38 a.m. UTC | #4 
On Tue, Nov 28, 2023 at 03:01:12PM +0100, Krzysztof Kozlowski wrote:
> On 28/11/2023 11:14, Varadarajan Narayanan wrote:
> > On Tue, Nov 28, 2023 at 09:51:50AM +0100, Krzysztof Kozlowski wrote:
> >> On 28/11/2023 09:46, Varadarajan Narayanan wrote:
> >>> From: Varadarajan Narayanan <quic_varada@quicinc.com>
> >>>
> >>> The earlier patch ec4f047679d5, incorrectly used 'l2'
> >>> as the vdda-pll-supply. However, 'l5' is the correct
> >>> ldo that supplies power to the USB PHY.
> >>>
> >>> Fixes: ec4f047679d5 ("arm64: dts: qcom: ipq9574: Enable USB")
> >>
> >> Doesn't this depend on the driver change?
> >
> > Yes, will mention in the cover letter.
>
> This commit should have it in its changelog ---
>
> >
> >> It affects both existing
> >> kernel and backports which you claim here should happen.
> >
> > Ok. Will include stable@vger.kernel.org in the next revision.
>
> I wasn't speaking about Cc. You indicated this should be backported.
> Then please backport it, without previous commit, and check the result.
> Is stable tree working correctly or not?

Without the previous commit, it would fail in both the latest
and stable tree. (Please see below for the error messages and
stack dump)

The previous commit is necessary for this commit to work.

Thanks
Varada

Linux version 6.7.0-rc3-next-20231128-00002-gf98bbf0a5152
---------------------------------------------------------
	[    1.073091] l5: Bringing 0uV into 1800000-1800000uV
	[    1.095184] l5: failed to enable: -ENXIO
	[    1.100751] clk: Disabling unused clocks
	[    1.105428] ------------[ cut here ]------------
	[    1.120170] WARNING: CPU: 2 PID: 58 at drivers/regulator/core.c:2397 _regulator_put.part.36+0x154/0x15c
	[    1.124774] Modules▒ $HL▒137014] Hardware name: Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7 (DT)
	[    1.137033] Workqueue: events_unbound async_run_entry_fn
	[    1.143111] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
	[    1.148579] pc : _regulator_put.part.36+0x154/0x15c
	[    1.155261] lr : regulator_put+0x34/0x4c
	[    1.160122] sp : ffff80008136ba10
	[    1.164288] x29: ffff80008136ba10 x28: ffff80008136bba8 x27: 0000000000000000
	[    1.167504] x26: ffff000002b76810 x25: fffffffffffffffa x24: ffff800080902000
	[    1.174622] x23: ffff00000039f800 x22: ffff0000009dfa80 x21: ffff0000009df898
	[    1.181740] x20: ffff00000256f840 x19: ffff00000256f840 x18: ffffffffffffffff
	[    1.188858] x17: 7571657266206c61 x16: 6974696e69206465 x15: ffff800080901480
	[    1.195976] x14: 0000000000000000 x13: 307475706e692f74 x12: 75706e692f737965
	[    1.203094] x11: 6b2d6f6970672f6d x10: 000000000000000d x9 : 0000000000000000
	[    1.210212] x8 : ffff800081035000 x7 : 000000000000000a x6 : 0000000000000000
	[    1.217330] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
	[    1.224449] x2 : ffff000000286a00 x1 : 0000000000000000 x0 : 0000000000000001
	[    1.231567] Call trace:
	[    1.238681]  _regulator_put.part.36+0x154/0x15c
	[    1.241633]  regulator_put+0x34/0x4c
	[    1.245886]  regulator_register+0x420/0x9e8
	[    1.249707]  devm_regulator_register+0x58/0xb4
	[    1.253613]  rpm_reg_probe+0x12c/0x238
	[    1.258126]  platform_probe+0x4c/0xa8
	[    1.261859]  really_probe+0x144/0x298
	[    1.265591]  __driver_probe_device+0xc4/0xe8
	[    1.269238]  driver_probe_device+0x38/0x114
	[    1.273578]  __device_attach_driver+0xac/0xe8
	[    1.277484]  bus_for_each_drv+0x6c/0xd8
	[    1.281997]  __device_attach_async_helper+0xac/0xb4
	[    1.285643]  async_run_entry_fn+0x2c/0xdc
	[    1.290505]  process_scheduled_works+0x16c/0x288
	[    1.294672]  worker_thread+0x160/0x33c
	[    1.299358]  kthread+0x100/0x10c
	[    1.302917]  ret_from_fork+0x10/0x20
	[    1.306303] ---[ end trace 0000000000000000 ]---
	[    1.309902] qcom_rpm_smd_regulator remoteproc:glink-edge:rpm-requests:regulators: l5: devm_regulator_register() failed, ret=-6

Linux version 6.6.4-dirty
-------------------------
	[    1.028110] qcom_rpm_smd_regulator remoteproc:glink-edge:rpm-requests:regulators: Unknown regulator l5
	[    1.036839] clk: Disabling unused clocks
	[    1.039163] mmc0: Failed to initialize a non-removable card
	[    1.046249] qcom_rpm_smd_regulator: probe of remoteproc:glink-edge:rpm-requests:regulators failed with error -22
	[    1.046329] ------------[ cut here ]------------
	[    1.075147] WARNING: CPU: 3 PID: 56 at drivers/regulator/core.c:5760 regulator_unregister+0xd0/0xd8
	[    1.079750] Modulesm▒▒r▒ʊ▒▒▒▒        *▒.YW,▒Y▒name: Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7 (DT)
	[    1.091661] Workqueue: events_unbound async_run_entry_fn
	[    1.097738] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
	[    1.103207] pc : regulator_unregister+0xd0/0xd8
	[    1.109889] lr : regulator_unregister+0x4c/0xd8
	[    1.114402] sp : ffff8000813bbb30
	[    1.118915] x29: ffff8000813bbb30 x28: ffff00000000e205 x27: ffff00000009c6e0
	[    1.122912] x26: ffff00000083be00 x25: 61c8864680b583eb x24: ffff0000002321a4
	[    1.129944] x23: ffff0000002321a8 x22: ffff000000232010 x21: ffff8000813bbbd8
	[    1.137062] x20: ffff800080f45000 x19: ffff000000911c00 x18: ffff000000030310
	[    1.144180] x17: 65757165722d6d70 x16: 723a656764652d6b x15: ffff0000004a46a0
	[    1.151298] x14: 0000000000000000 x13: ffff000000030310 x12: ffff0000004a4490
	[    1.158415] x11: 0001ffffffffffff x10: ffff000000030318 x9 : ffff000000030310
	[    1.165534] x8 : ffff0000004a44b8 x7 : 0000000000000000 x6 : ffff0000008f5cc0
	[    1.172652] x5 : ffff00000039c0f8 x4 : 0000000000000000 x3 : 0000000000000000
	[    1.179770] x2 : ffff0000008f5cc0 x1 : 0000000000000000 x0 : 0000000000000001
	[    1.186888] Call trace:
	[    1.194002]  regulator_unregister+0xd0/0xd8
	[    1.196260]  devm_rdev_release+0x10/0x18
	[    1.200426]  release_nodes+0x38/0x60
	[    1.204593]  devres_release_all+0x90/0xd8
	[    1.208153]  device_unbind_cleanup+0x14/0x50
	[    1.212059]  really_probe+0xdc/0x298
	[    1.216399]  __driver_probe_device+0xc4/0xe8
	[    1.219958]  driver_probe_device+0x34/0x10c
	[    1.224211]  __device_attach_driver+0xac/0xe8
	[    1.228118]  bus_for_each_drv+0x6c/0xd8
	[    1.232632]  __device_attach_async_helper+0xac/0xb4
	[    1.236277]  async_run_entry_fn+0x2c/0xdc
	[    1.241139]  process_scheduled_works+0x16c/0x288
	[    1.245305]  worker_thread+0x15c/0x338
	[    1.249992]  kthread+0x100/0x10c
	[    1.253551]  ret_from_fork+0x10/0x20
	[    1.256937] ---[ end trace 0000000000000000 ]---
Krzysztof Kozlowski Dec. 6, 2023, 12:31 p.m. UTC | #5 
On 06/12/2023 12:38, Varadarajan Narayanan wrote:
> On Tue, Nov 28, 2023 at 03:01:12PM +0100, Krzysztof Kozlowski wrote:
>> On 28/11/2023 11:14, Varadarajan Narayanan wrote:
>>> On Tue, Nov 28, 2023 at 09:51:50AM +0100, Krzysztof Kozlowski wrote:
>>>> On 28/11/2023 09:46, Varadarajan Narayanan wrote:
>>>>> From: Varadarajan Narayanan <quic_varada@quicinc.com>
>>>>>
>>>>> The earlier patch ec4f047679d5, incorrectly used 'l2'
>>>>> as the vdda-pll-supply. However, 'l5' is the correct
>>>>> ldo that supplies power to the USB PHY.
>>>>>
>>>>> Fixes: ec4f047679d5 ("arm64: dts: qcom: ipq9574: Enable USB")
>>>>
>>>> Doesn't this depend on the driver change?
>>>
>>> Yes, will mention in the cover letter.
>>
>> This commit should have it in its changelog ---
>>
>>>
>>>> It affects both existing
>>>> kernel and backports which you claim here should happen.
>>>
>>> Ok. Will include stable@vger.kernel.org in the next revision.
>>
>> I wasn't speaking about Cc. You indicated this should be backported.
>> Then please backport it, without previous commit, and check the result.
>> Is stable tree working correctly or not?
> 
> Without the previous commit, it would fail in both the latest
> and stable tree. (Please see below for the error messages and
> stack dump)
> 
> The previous commit is necessary for this commit to work.

Yep, exactly. It's visible from the patches. I don't know how to solve
this exactly. The Fixes tag here is logically correct, but then any
backporting must include previous commit. Dependency can be provided in
cc-stable tag, but you did not cc-stable, I suppose on purpose.

If this is chosen by AUTOSEL, are you going to check if backport
includes previous patch and object/review such backport?

Best regards,
Krzysztof
Krzysztof Kozlowski Dec. 6, 2023, 12:33 p.m. UTC | #6 
On 06/12/2023 13:31, Krzysztof Kozlowski wrote:
> On 06/12/2023 12:38, Varadarajan Narayanan wrote:
>> On Tue, Nov 28, 2023 at 03:01:12PM +0100, Krzysztof Kozlowski wrote:
>>> On 28/11/2023 11:14, Varadarajan Narayanan wrote:
>>>> On Tue, Nov 28, 2023 at 09:51:50AM +0100, Krzysztof Kozlowski wrote:
>>>>> On 28/11/2023 09:46, Varadarajan Narayanan wrote:
>>>>>> From: Varadarajan Narayanan <quic_varada@quicinc.com>
>>>>>>
>>>>>> The earlier patch ec4f047679d5, incorrectly used 'l2'
>>>>>> as the vdda-pll-supply. However, 'l5' is the correct
>>>>>> ldo that supplies power to the USB PHY.
>>>>>>
>>>>>> Fixes: ec4f047679d5 ("arm64: dts: qcom: ipq9574: Enable USB")
>>>>>
>>>>> Doesn't this depend on the driver change?
>>>>
>>>> Yes, will mention in the cover letter.
>>>
>>> This commit should have it in its changelog ---
>>>
>>>>
>>>>> It affects both existing
>>>>> kernel and backports which you claim here should happen.
>>>>
>>>> Ok. Will include stable@vger.kernel.org in the next revision.
>>>
>>> I wasn't speaking about Cc. You indicated this should be backported.
>>> Then please backport it, without previous commit, and check the result.
>>> Is stable tree working correctly or not?
>>
>> Without the previous commit, it would fail in both the latest
>> and stable tree. (Please see below for the error messages and
>> stack dump)
>>
>> The previous commit is necessary for this commit to work.
> 
> Yep, exactly. It's visible from the patches. I don't know how to solve
> this exactly. The Fixes tag here is logically correct, but then any
> backporting must include previous commit. Dependency can be provided in
> cc-stable tag, but you did not cc-stable, I suppose on purpose.
> 
> If this is chosen by AUTOSEL, are you going to check if backport
> includes previous patch and object/review such backport?

One more point. Except issues with backporting, you did not annotate any
dependency so patches can be applied independently. This will lead to
non-bisectable tree or even broken tree. What's more DTS goes always via
separate tree and branches, so this patch must be delayed.

You always must explicitly mention such dependencies and changes to
default applying process, so maintainers know what to do. Nothing like
this was explained anywhere here.

Best regards,
Krzysztof
Varadarajan Narayanan Dec. 7, 2023, 4:12 a.m. UTC | #7 
On Wed, Dec 06, 2023 at 01:33:17PM +0100, Krzysztof Kozlowski wrote:
> On 06/12/2023 13:31, Krzysztof Kozlowski wrote:
> > On 06/12/2023 12:38, Varadarajan Narayanan wrote:
> >> On Tue, Nov 28, 2023 at 03:01:12PM +0100, Krzysztof Kozlowski wrote:
> >>> On 28/11/2023 11:14, Varadarajan Narayanan wrote:
> >>>> On Tue, Nov 28, 2023 at 09:51:50AM +0100, Krzysztof Kozlowski wrote:
> >>>>> On 28/11/2023 09:46, Varadarajan Narayanan wrote:
> >>>>>> From: Varadarajan Narayanan <quic_varada@quicinc.com>
> >>>>>>
> >>>>>> The earlier patch ec4f047679d5, incorrectly used 'l2'
> >>>>>> as the vdda-pll-supply. However, 'l5' is the correct
> >>>>>> ldo that supplies power to the USB PHY.
> >>>>>>
> >>>>>> Fixes: ec4f047679d5 ("arm64: dts: qcom: ipq9574: Enable USB")
> >>>>>
> >>>>> Doesn't this depend on the driver change?
> >>>>
> >>>> Yes, will mention in the cover letter.
> >>>
> >>> This commit should have it in its changelog ---
> >>>
> >>>>
> >>>>> It affects both existing
> >>>>> kernel and backports which you claim here should happen.
> >>>>
> >>>> Ok. Will include stable@vger.kernel.org in the next revision.
> >>>
> >>> I wasn't speaking about Cc. You indicated this should be backported.
> >>> Then please backport it, without previous commit, and check the result.
> >>> Is stable tree working correctly or not?
> >>
> >> Without the previous commit, it would fail in both the latest
> >> and stable tree. (Please see below for the error messages and
> >> stack dump)
> >>
> >> The previous commit is necessary for this commit to work.
> >
> > Yep, exactly. It's visible from the patches. I don't know how to solve
> > this exactly. The Fixes tag here is logically correct, but then any
> > backporting must include previous commit. Dependency can be provided in
> > cc-stable tag, but you did not cc-stable, I suppose on purpose.

It was not on purpose. Got lucky :).
Shall I separate the patches and wait till the first one gets
merged (in stable and top of tree) and then post the second one?

> > If this is chosen by AUTOSEL, are you going to check if backport
> > includes previous patch and object/review such backport?
>
> One more point. Except issues with backporting, you did not annotate any
> dependency so patches can be applied independently. This will lead to
> non-bisectable tree or even broken tree. What's more DTS goes always via
> separate tree and branches, so this patch must be delayed.
>
> You always must explicitly mention such dependencies and changes to
> default applying process, so maintainers know what to do. Nothing like
> this was explained anywhere here.

Sorry, my mistake. Will be careful in future.

Thanks for the feedback.

Regards
Varada
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
index 49c9b6478357..6d4c758c7f5f 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
@@ -96,6 +96,13 @@  mp5496_l2: l2 {
 			regulator-always-on;
 			regulator-boot-on;
 		};
+
+		mp5496_l5: l5 {
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-always-on;
+			regulator-boot-on;
+		};
 	};
 };
 
@@ -124,7 +131,7 @@  &usb_0_dwc3 {
 };
 
 &usb_0_qmpphy {
-	vdda-pll-supply = <&mp5496_l2>;
+	vdda-pll-supply = <&mp5496_l5>;
 	vdda-phy-supply = <&regulator_fixed_0p925>;
 
 	status = "okay";
@@ -132,7 +139,7 @@  &usb_0_qmpphy {
 
 &usb_0_qusbphy {
 	vdd-supply = <&regulator_fixed_0p925>;
-	vdda-pll-supply = <&mp5496_l2>;
+	vdda-pll-supply = <&mp5496_l5>;
 	vdda-phy-dpdm-supply = <&regulator_fixed_3p3>;
 
 	status = "okay";