Re: ARMADA espressobin SATA drive detection failure

["Pali Rohár" <pali@kernel.org>]

On Friday 26 August 2022 00:15:58 Marek Behún wrote:
> On Thu, 25 Aug 2022 15:00:59 +0200
> Pali Rohár <pali@kernel.org> wrote:
> 
> > On Sunday 14 August 2022 01:10:50 Pali Rohár wrote:
> > > On Saturday 13 August 2022 23:02:34 Shinichiro Kawasaki wrote:  
> > > > On Aug 13, 2022 / 14:50, Pali Rohár wrote:  
> > > > > On Saturday 13 August 2022 12:47:30 Shinichiro Kawasaki wrote:  
> > > > > > On Aug 13, 2022 / 14:01, Pali Rohár wrote:  
> > > > > > > On Saturday 13 August 2022 11:53:39 Shinichiro Kawasaki wrote:  
> > > > > > > > On Aug 13, 2022 / 11:32, Pali Rohár wrote:  
> > > > > > > > > On Saturday 13 August 2022 01:35:35 Shinichiro Kawasaki wrote:  
> > > > > > > > 
> > > > > > > > [...]
> > > > > > > >   
> > > > > > > > > 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

> diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
> index a4d7d9bd100d..064be967a58b 100644
> --- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
> +++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
> @@ -1171,8 +1171,12 @@ 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)
> +	/* If changing mode, ensure reset is called.
> +	 * Reset can currently break support for some SATA disks, so don't do it
> +	 * for PHY_MODE_SATA.
> +	 */
> +	if (lane->mode != PHY_MODE_INVALID && lane->mode != mode &&
> +	    mode != PHY_MODE_SATA)
>  		lane->needs_reset = true;
>  
>  	/* Just remember the mode, ->power_on() will do the real setup */
> @@ -1388,13 +1392,6 @@ static int mvebu_a3700_comphy_probe(struct platform_device *pdev)
>  		lane->invert_tx = false;
>  		lane->invert_rx = false;
>  		phy_set_drvdata(phy, lane);
> -
> -		/*
> -		 * To avoid relying on the bootloader/firmware configuration,
> -		 * power off all comphys.
> -		 */

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