Thread (32 messages) 32 messages, 3 authors, 2022-08-29

Re: ARMADA espressobin SATA drive detection failure

From: Pali Rohár <pali@kernel.org>
Date: 2022-08-25 13:04:55
Subsystem: generic phy framework, marvell armada 3700 phy drivers, the rest · Maintainers: Vinod Koul, Miquel Raynal, Linus Torvalds

On Sunday 14 August 2022 01:10:50 Pali Rohár wrote:
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
Shinichiro, meanwhile, could you try following change on top of clean
linux tree without any additional patches?
diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
index a4d7d9bd100d..67712c77d806 100644
--- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
+++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
@@ -274,7 +274,6 @@ struct mvebu_a3700_comphy_lane {
 	int submode;
 	bool invert_tx;
 	bool invert_rx;
-	bool needs_reset;
 };
 
 struct gbe_phy_init_data_fix {
@@ -1097,40 +1096,12 @@ mvebu_a3700_comphy_pcie_power_off(struct mvebu_a3700_comphy_lane *lane)
 			    0x0, PU_PLL_BIT | PU_RX_BIT | PU_TX_BIT);
 }
 
-static int mvebu_a3700_comphy_reset(struct phy *phy)
+static void mvebu_a3700_comphy_usb3_power_off(struct mvebu_a3700_comphy_lane *lane)
 {
-	struct mvebu_a3700_comphy_lane *lane = phy_get_drvdata(phy);
-	u16 mask, data;
-
-	dev_dbg(lane->dev, "resetting lane %d\n", lane->id);
-
-	/* COMPHY reset for internal logic */
-	comphy_lane_reg_set(lane, COMPHY_SFT_RESET,
-			    SFT_RST_NO_REG, SFT_RST_NO_REG);
-
-	/* COMPHY register reset (cleared automatically) */
-	comphy_lane_reg_set(lane, COMPHY_SFT_RESET, SFT_RST, SFT_RST);
-
-	/* PIPE soft and register reset */
-	data = PIPE_SOFT_RESET | PIPE_REG_RESET;
-	mask = data;
-	comphy_lane_reg_set(lane, COMPHY_PIPE_RST_CLK_CTRL, data, mask);
-
-	/* Release PIPE register reset */
-	comphy_lane_reg_set(lane, COMPHY_PIPE_RST_CLK_CTRL,
-			    0x0, PIPE_REG_RESET);
-
-	/* Reset SB configuration register (only for lanes 0 and 1) */
-	if (lane->id == 0 || lane->id == 1) {
-		u32 mask, data;
-
-		data = PIN_RESET_CORE_BIT | PIN_RESET_COMPHY_BIT |
-		       PIN_PU_PLL_BIT | PIN_PU_RX_BIT | PIN_PU_TX_BIT;
-		mask = data | PIN_PU_IVREF_BIT | PIN_TX_IDLE_BIT;
-		comphy_periph_reg_set(lane, COMPHY_PHY_CFG1, data, mask);
-	}
-
-	return 0;
+	/*
+	 * The USB3 MAC sets the USB3 PHY to low state, so we do not
+	 * need to power off USB3 PHY again.
+	 */
 }
 
 static bool mvebu_a3700_comphy_check_mode(int lane,
@@ -1171,10 +1142,6 @@ static int mvebu_a3700_comphy_set_mode(struct phy *phy, enum phy_mode mode,
 	    (lane->mode != mode || lane->submode != submode))
 		return -EBUSY;
 
-	/* If changing mode, ensure reset is called */
-	if (lane->mode != PHY_MODE_INVALID && lane->mode != mode)
-		lane->needs_reset = true;
-
 	/* Just remember the mode, ->power_on() will do the real setup */
 	lane->mode = mode;
 	lane->submode = submode;
@@ -1185,7 +1152,6 @@ static int mvebu_a3700_comphy_set_mode(struct phy *phy, enum phy_mode mode,
 static int mvebu_a3700_comphy_power_on(struct phy *phy)
 {
 	struct mvebu_a3700_comphy_lane *lane = phy_get_drvdata(phy);
-	int ret;
 
 	if (!mvebu_a3700_comphy_check_mode(lane->id, lane->mode,
 					   lane->submode)) {
@@ -1193,14 +1159,6 @@ static int mvebu_a3700_comphy_power_on(struct phy *phy)
 		return -EINVAL;
 	}
 
-	if (lane->needs_reset) {
-		ret = mvebu_a3700_comphy_reset(phy);
-		if (ret)
-			return ret;
-
-		lane->needs_reset = false;
-	}
-
 	switch (lane->mode) {
 	case PHY_MODE_USB_HOST_SS:
 		dev_dbg(lane->dev, "set lane %d to USB3 host mode\n", lane->id);
@@ -1224,38 +1182,28 @@ static int mvebu_a3700_comphy_power_off(struct phy *phy)
 {
 	struct mvebu_a3700_comphy_lane *lane = phy_get_drvdata(phy);
 
-	switch (lane->mode) {
-	case PHY_MODE_USB_HOST_SS:
-		/*
-		 * The USB3 MAC sets the USB3 PHY to low state, so we do not
-		 * need to power off USB3 PHY again.
-		 */
-		break;
-
-	case PHY_MODE_SATA:
-		mvebu_a3700_comphy_sata_power_off(lane);
-		break;
-
-	case PHY_MODE_ETHERNET:
+	switch (lane->id) {
+	case 0:
+		mvebu_a3700_comphy_usb3_power_off(lane);
 		mvebu_a3700_comphy_ethernet_power_off(lane);
-		break;
-
-	case PHY_MODE_PCIE:
+		return 0;
+	case 1:
 		mvebu_a3700_comphy_pcie_power_off(lane);
-		break;
-
+		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;
 	}
-
-	return 0;
 }
 
 static const struct phy_ops mvebu_a3700_comphy_ops = {
 	.power_on	= mvebu_a3700_comphy_power_on,
 	.power_off	= mvebu_a3700_comphy_power_off,
-	.reset		= mvebu_a3700_comphy_reset,
 	.set_mode	= mvebu_a3700_comphy_set_mode,
 	.owner		= THIS_MODULE,
 };
@@ -1393,8 +1341,7 @@ 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);
-		lane->needs_reset = false;
+		mvebu_a3700_comphy_power_off(phy);
 	}
 
 	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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help