On Friday 26 August 2022 00:15:58 Marek Behún wrote:

On Thu, 25 Aug 2022 15:00:59 +0200
Pali Rohár [off-list ref] wrote:

quoted

On Sunday 14 August 2022 01:10:50 Pali Rohár wrote:

quoted

On Saturday 13 August 2022 23:02:34 Shinichiro Kawasaki wrote:  

quoted

On Aug 13, 2022 / 14:50, Pali Rohár wrote:  

quoted

On Saturday 13 August 2022 12:47:30 Shinichiro Kawasaki wrote:  

quoted

On Aug 13, 2022 / 14:01, Pali Rohár wrote:  

quoted

On Saturday 13 August 2022 11:53:39 Shinichiro Kawasaki wrote:  

quoted

On Aug 13, 2022 / 11:32, Pali Rohár wrote:  

quoted

On Saturday 13 August 2022 01:35:35 Shinichiro Kawasaki wrote:  

[...]
  

quoted

Ok, thanks for testing. It looks like that reset code has some issues.

Could you please test this change?

@@ -1393,7 +1436,8 @@ static int mvebu_a3700_comphy_probe(struct platform_device *pdev)
 		 * To avoid relying on the bootloader/firmware configuration,
 		 * power off all comphys.
 		 */
-		mvebu_a3700_comphy_reset(phy);
+//		mvebu_a3700_comphy_reset(phy);
+		mvebu_a3700_comphy_power_off(phy);
 		lane->needs_reset = false;
 	}
 

It should replace reset code by power off at beginning / probe time.  

This change also avoided the "ata1: SATA link down (SStatus 0 SControl 300)"
message, and my SSD was detected as /dev/sda. Good symptoms for me. FYI, I
attach the console log with this change. (This time, following messages were
printed. Not sure if they are important or not.)

[    2.937376] mvebu-a3700-comphy d0018300.phy: invalid COMPHY mode
[    2.943581] mvebu-a3700-comphy d0018300.phy: invalid COMPHY mode
[    2.949679] mvebu-a3700-comphy d0018300.phy: invalid COMPHY mode  

Those errors are important, it means that power_off did nothing and was
skipped. So test did nothing.

Could you please replace mvebu_a3700_comphy_power_off implementation
with this one and try it again?

static int mvebu_a3700_comphy_power_off(struct phy *phy)
{
	struct mvebu_a3700_comphy_lane *lane = phy_get_drvdata(phy);

	switch (lane->id) {
	case 0:
		mvebu_a3700_comphy_usb3_power_off(lane);
		mvebu_a3700_comphy_ethernet_power_off(lane);
		return 0;
	case 1:
		mvebu_a3700_comphy_pcie_power_off(lane);
		mvebu_a3700_comphy_ethernet_power_off(lane);
		return 0;
	case 2:
		mvebu_a3700_comphy_usb3_power_off(lane);
		mvebu_a3700_comphy_sata_power_off(lane);
		return 0;
	default:
		dev_err(lane->dev, "invalid COMPHY mode\n");
		return -EINVAL;
	}
}  

Thanks, but this hunk failed to compile with the error below.

rivers/phy/marvell/phy-mvebu-a3700-comphy.c: In function 'mvebu_a3700_comphy_power_off':
drivers/phy/marvell/phy-mvebu-a3700-comphy.c:1229:17: error: implicit declaration of function 'mvebu_a3700_comphy_usb3_power_off'; did you mean 'mvebu_a3700_comphy_usb3_power_on'? [-Werror=implicit-function-declaration]
 1229 |                 mvebu_a3700_comphy_usb3_power_off(lane);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                 mvebu_a3700_comphy_usb3_power_on
cc1: some warnings being treated as errors

Do I need to add mvebu_a3700_comphy_usb3_power_off() function?  

In my original version of this driver I have:

static void mvebu_a3700_comphy_usb3_power_off(struct mvebu_a3700_comphy_lane *lane)
{
	/*
	 * Currently the USB3 MAC will control the USB3 PHY to set it to low
	 * state, thus do not need to power off USB3 PHY again.
	 */
}

So please remove mvebu_a3700_comphy_usb3_power_off() calls as it does
nothing.  

Okay, I tried with the edit and result looks positive:

- "invalid COMPHY mode" messages disappepared
- No "ata1: SATA link down (SStatus 0 SControl 300)" message
- My SSD was detected as /dev/sda

FYI, I attach console log and the patch I used.

-- 
Shin'ichiro Kawasaki  

Perfect! So the issue is with mvebu_a3700_comphy_reset() function.

This function is not in TF-A code and neither in my original kernel
driver implementation (still available here):
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/commit/?h=phy-mvebu-a3700-comphy&id=4588902a3528195bcfdda9f9e1e14262a1955df1

Marek, this function mvebu_a3700_comphy_reset() was implemented by you.
Could you please look at it, why you added this function and try to fix
it? Is this function needed at all?  

PING? Any progress here?

Not yet, sorry. Maybe we could do something like I am attaching, until
I have time to play with it?

See attached file.

Marek

We really should not depend on bootloader configuration and firmware.
This is the whole point of that driver...

-		mvebu_a3700_comphy_reset(phy);
-		lane->needs_reset = false;
 	}
 
 	provider = devm_of_phy_provider_register(&pdev->dev,

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy