[BUG] k3-j784s4-evm/phy-cadence-torrent: Shared reset using exclusive API

[BUG] k3-j784s4-evm/phy-cadence-torrent: Shared reset using exclusive API

[Andrew Halaney]

Hi,

Running linux-next on a k3-j784s4-evm I got this splat:

	[    4.846253] ------------[ cut here ]------------
	[    4.846266] WARNING: CPU: 4 PID: 308 at drivers/reset/core.c:792 __reset_control_get_internal+0x128/0x160
	[    4.846288] Modules linked in: ti_am335x_adc(+) phy_cadence_torrent(+) can_dev snd_soc_ti_udma snd_soc_j721e_evm snd_soc_pcm3168a_i2c drm_kms_helper rtc_tps6594(+) tps6594_regulator(+) mc at24 kfifo_buf tps6594_pfsm(+) tps6594_esm gpio_regmap snd_soc_pcm3168a snd_soc_ti_edma ti_k3_r5_remoteproc k3_j72xx_bandgap ti_j721e_ufs snd_soc_ti_sdma ti_k3_dsp_remoteproc cdns3_ti drm backlight phy_can_transceiver omap_mailbox tps6594_i2c tps6594_core phy_j721e_wiz ti_am335x_tscadc sa2ul authenc rti_wdt fuse ipv6
	[    4.846336] CPU: 4 UID: 0 PID: 308 Comm: systemd-udevd Not tainted 6.10.0-rc6-next-20240703 #72
	[    4.846342] Hardware name: ti,j784s4-evm Texas Instruments J784S4 EVM/Texas Instruments J784S4 EVM, BIOS 2024.01-rc3 01/01/2024
	[    4.846345] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
	[    4.846349] pc : __reset_control_get_internal+0x128/0x160
	[    4.846354] lr : __of_reset_control_get+0x4e0/0x528
	[    4.846359] sp : ffff8000858b3400
	[    4.846360] x29: ffff8000858b3400 x28: 0000000000000001 x27: ffff000f7c00fa98
	[    4.846365] x26: 0000000000000001 x25: ffff80008324d330 x24: ffff00080aecd2a8
	[    4.846370] x23: 0000000000000001 x22: 0000000000000000 x21: 0000000000000004
	[    4.846374] x20: ffff00080aecd288 x19: fffffffffffffff0 x18: 00000000000001d0
	[    4.846379] x17: 0000000000000230 x16: 00000000000000bc x15: ffff00080a630000
	[    4.846383] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000004
	[    4.846387] x11: 0000000000000000 x10: 0000000000000005 x9 : 0000000000000000
	[    4.846391] x8 : ffff00080741f4c8 x7 : 00736c6c65632d74 x6 : 00000080236d626f
	[    4.846396] x5 : 6f626d2300000000 x4 : 0000000080000000 x3 : 0000000000000001
	[    4.846400] x2 : 0000000000000000 x1 : 0000000000000004 x0 : ffff00080aecd288
	[    4.846405] Call trace:
	[    4.846407]  __reset_control_get_internal+0x128/0x160
	[    4.846413]  __of_reset_control_get+0x4e0/0x528
	[    4.846418]  of_reset_control_array_get+0xa4/0x1f8
	[    4.846423]  cdns_torrent_phy_probe+0xbc8/0x1068 [phy_cadence_torrent]
	[    4.846445]  platform_probe+0xb4/0xe8
	[    4.846456]  really_probe+0x134/0x2e0
	[    4.846460]  __driver_probe_device+0xac/0x140
	[    4.846464]  driver_probe_device+0x48/0x210
	[    4.846468]  __driver_attach+0xe8/0x1b8
	[    4.846472]  bus_for_each_dev+0xf8/0x158
	[    4.846475]  driver_attach+0x30/0x48
	[    4.846480]  bus_add_driver+0x160/0x280
	[    4.846483]  driver_register+0x74/0x118
	[    4.846486]  __platform_driver_register+0x30/0x48
	[    4.846491]  init_module+0x2c/0xff8 [phy_cadence_torrent]
	[    4.846501]  do_one_initcall+0xd0/0x2e0
	[    4.846509]  do_init_module+0x60/0x210
	[    4.846515]  load_module+0x14f4/0x1768
	[    4.846519]  __arm64_sys_finit_module+0x21c/0x2c0
	[    4.846522]  invoke_syscall+0x4c/0x118
	[    4.846529]  el0_svc_common+0x8c/0xf0
	[    4.846534]  do_el0_svc+0x28/0x40
	[    4.846538]  el0_svc+0x34/0x80
	[    4.846548]  el0t_64_sync_handler+0x84/0x100
	[    4.846553]  el0t_64_sync+0x190/0x198
	[    4.846557] ---[ end trace 0000000000000000 ]---
	[    4.846577] cdns-torrent-phy 5060000.serdes: phy@0: failed to get reset

this is because the devicetree has two[0][1] consumers of the same reset:

	&serdes0 {
		status = "okay";

		serdes0_pcie1_link: phy@0 {
			reg = <0>;
			cdns,num-lanes = <4>;
			#phy-cells = <0>;
			cdns,phy-type = <PHY_TYPE_PCIE>;
			resets = <&serdes_wiz0 1>, <&serdes_wiz0 2>,
				 <&serdes_wiz0 3>, <&serdes_wiz0 4>;
		};
	};
	...
	&serdes0 {
		status = "okay";

		serdes0_usb_link: phy@3 {
			reg = <3>;
			cdns,num-lanes = <1>;
			#phy-cells = <0>;
			cdns,phy-type = <PHY_TYPE_USB3>;
			resets = <&serdes_wiz0 4>;
		};
	};

phy-cadence-torrent (the serdes0 consumer here) uses the exclusive consumer API[2],
so this blows up.

Is the problem here that one of the above devicetree nodes is using the wrong
reset, or does the driver need to look into using the shared API? I'm
not sure where to find the reset definitions for the IP here unfortunately,
so I'm hoping someone can help confirm if those are correct or not.

Total aside, I think we should put the above dts snippet into one &serdes0 reference
for readability sake. I'd post the patch but I'm hoping to get the above answered
first in order to clean that up before shuffling things around for readability sake.

[0] pcie serdes0_pcie1_link: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts?h=next-20240703#n1398
[1] usb serdes0_usb_link: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts?h=next-20240703#n1270
[2] phy-cadence-torrent reset usage: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/phy/cadence/phy-cadence-torrent.c?h=next-20240703#n2878

Thanks,
Andrew

Re: [BUG] k3-j784s4-evm/phy-cadence-torrent: Shared reset using exclusive API

[Siddharth Vadapalli]

On Tue, Jul 09, 2024 at 04:57:09PM -0500, Andrew Halaney wrote:
> Hi,
> 

[...]

> 
> this is because the devicetree has two[0][1] consumers of the same reset:
> 
> 	&serdes0 {
> 		status = "okay";
> 
> 		serdes0_pcie1_link: phy@0 {
> 			reg = <0>;
> 			cdns,num-lanes = <4>;
> 			#phy-cells = <0>;
> 			cdns,phy-type = <PHY_TYPE_PCIE>;
> 			resets = <&serdes_wiz0 1>, <&serdes_wiz0 2>,
> 				 <&serdes_wiz0 3>, <&serdes_wiz0 4>;
> 		};
> 	};
> 	...
> 	&serdes0 {
> 		status = "okay";
> 
> 		serdes0_usb_link: phy@3 {
> 			reg = <3>;
> 			cdns,num-lanes = <1>;
> 			#phy-cells = <0>;
> 			cdns,phy-type = <PHY_TYPE_USB3>;
> 			resets = <&serdes_wiz0 4>;
> 		};
> 	};
> 
> phy-cadence-torrent (the serdes0 consumer here) uses the exclusive consumer API[2],
> so this blows up.
> 
> Is the problem here that one of the above devicetree nodes is using the wrong
> reset, or does the driver need to look into using the shared API? I'm
> not sure where to find the reset definitions for the IP here unfortunately,
> so I'm hoping someone can help confirm if those are correct or not.

No, the resets are correct. Both PCIe1 and USB0 use the same instances
of SERDES which is SERDES0. I had posted the series for PCIe at:
https://lore.kernel.org/r/20240529082259.1619695-1-s-vadapalli@ti.com/
with all 4 Lanes of SERDES0 given to PCIe1. Similarly, Ravi had posted
the series for USB at:
https://lore.kernel.org/r/20240507095545.8210-1-r-gunasekaran@ti.com/
with lane 3 of SERDES0 given to USB0.

Since both of the series got merged on the same day (14 Jun 2024):
PCIe series:
https://lore.kernel.org/r/171826022277.240984.16790260886500529482.b4-ty@ti.com/
USB series:
https://lore.kernel.org/r/171826022274.240984.5150753966671933401.b4-ty@ti.com/
the dependency was unknown when the individual series were posted as
neither of them was a part of linux-next/ti-k3-dts-next when the other
one was posted.

> 
> Total aside, I think we should put the above dts snippet into one &serdes0 reference
> for readability sake. I'd post the patch but I'm hoping to get the above answered
> first in order to clean that up before shuffling things around for readability sake.

Yes, I agree that both sub-nodes should go into the same referenced
serdes0 node in k3-j784s4-evm.dts. The reason it didn't happen that way
to begin with is due to the fact that both series got merged on the same
day as I pointed out above.

The fix in this case will be to assign lanes 0 and 1 of SERDES0 to PCIe1
and lane 3 to USB0 with lane 2 left unused since PCIe doesn't have the
concept of a x3 link. In such a configuration, the device-tree nodes
will look like:

&serdes0 {
	status = "okay";

	serdes0_pcie1_link: phy@0 {
		reg = <0>;
		cdns,num-lanes = <2>;
		#phy-cells = <0>;
		cdns,phy-type = <PHY_TYPE_PCIE>;
		resets = <&serdes_wiz0 1>, <&serdes_wiz0 2>;
	};

	serdes0_usb_link: phy@3 {
		reg = <3>;
		cdns,num-lanes = <1>;
		#phy-cells = <0>;
		cdns,phy-type = <PHY_TYPE_USB3>;
		resets = <&serdes_wiz0 4>;
	};
};

Thank you for pointing out this issue. Please let me know if you plan to
post the patch with the above fix or you want me to post the patch for it.

Regards,
Siddharth.

Re: [BUG] k3-j784s4-evm/phy-cadence-torrent: Shared reset using exclusive API

[Andrew Halaney]

On Wed, Jul 10, 2024 at 10:29:46AM GMT, Siddharth Vadapalli wrote:

<snip>

> 
> No, the resets are correct. Both PCIe1 and USB0 use the same instances
> of SERDES which is SERDES0. I had posted the series for PCIe at:
> https://lore.kernel.org/r/20240529082259.1619695-1-s-vadapalli@ti.com/
> with all 4 Lanes of SERDES0 given to PCIe1. Similarly, Ravi had posted
> the series for USB at:
> https://lore.kernel.org/r/20240507095545.8210-1-r-gunasekaran@ti.com/
> with lane 3 of SERDES0 given to USB0.
> 
> Since both of the series got merged on the same day (14 Jun 2024):
> PCIe series:
> https://lore.kernel.org/r/171826022277.240984.16790260886500529482.b4-ty@ti.com/
> USB series:
> https://lore.kernel.org/r/171826022274.240984.5150753966671933401.b4-ty@ti.com/
> the dependency was unknown when the individual series were posted as
> neither of them was a part of linux-next/ti-k3-dts-next when the other
> one was posted.
> 
> > 
> > Total aside, I think we should put the above dts snippet into one &serdes0 reference
> > for readability sake. I'd post the patch but I'm hoping to get the above answered
> > first in order to clean that up before shuffling things around for readability sake.
> 
> Yes, I agree that both sub-nodes should go into the same referenced
> serdes0 node in k3-j784s4-evm.dts. The reason it didn't happen that way
> to begin with is due to the fact that both series got merged on the same
> day as I pointed out above.
> 
> The fix in this case will be to assign lanes 0 and 1 of SERDES0 to PCIe1
> and lane 3 to USB0 with lane 2 left unused since PCIe doesn't have the
> concept of a x3 link. In such a configuration, the device-tree nodes
> will look like:

Thanks alot for the quick explanation and suggestion!

> 
> &serdes0 {
> 	status = "okay";
> 
> 	serdes0_pcie1_link: phy@0 {
> 		reg = <0>;
> 		cdns,num-lanes = <2>;
> 		#phy-cells = <0>;
> 		cdns,phy-type = <PHY_TYPE_PCIE>;
> 		resets = <&serdes_wiz0 1>, <&serdes_wiz0 2>;
> 	};
> 
> 	serdes0_usb_link: phy@3 {
> 		reg = <3>;
> 		cdns,num-lanes = <1>;
> 		#phy-cells = <0>;
> 		cdns,phy-type = <PHY_TYPE_USB3>;
> 		resets = <&serdes_wiz0 4>;
> 	};
> };
> 
> Thank you for pointing out this issue. Please let me know if you plan to
> post the patch with the above fix or you want me to post the patch for it.
> 

I've posted a series (with you CC'ed, just realized I CC'ed Matt instead
of Ravi -- whoops): https://lore.kernel.org/all/20240710-k3-j784s4-evm-serdes0-cleanup-v1-0-03850fe33922@redhat.com/

I'll be on PTO the next few days, so if any changes are required there
do feel free to do them on my behalf if I don't respin by end of today
US-time, otherwise I'll make the changes when I return next week.

Thanks again,
Andrew!