From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 53041C4338F for ; Sat, 31 Jul 2021 09:28:37 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 62A4D600D1 for ; Sat, 31 Jul 2021 09:28:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 62A4D600D1 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 6888082E02; Sat, 31 Jul 2021 11:28:33 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="fhTnZa+U"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id C420782F32; Sat, 31 Jul 2021 11:28:30 +0200 (CEST) Received: from mail-yb1-xb33.google.com (mail-yb1-xb33.google.com [IPv6:2607:f8b0:4864:20::b33]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 0544E82C8B for ; Sat, 31 Jul 2021 11:28:27 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=mibodhi@gmail.com Received: by mail-yb1-xb33.google.com with SMTP id m193so5477501ybf.9 for ; Sat, 31 Jul 2021 02:28:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vBOqylCGsRtsA/CHiSUcymkDXwo6gqRFN/tWR7AqT78=; b=fhTnZa+UvBDp8+u8ItJ0ovpY+AGrmzXJACRV4ti4YWEEjTdTQBJmN/C7PnNRjlD7Lp QQBllQdsDxf51TGVBZZUEBMemFAfFrXOctHjnTxgyWzD+7uOF/+KrN5OWezSBRtzDFtw hvboHsPt1GAt0fc/OFwXX1rtNz/Bo6DbrMeuUmkkO9SoAcBT/Pzuc9hFvSj2mKMfAeOj sPbgt8hWRIZKQPjynPvYdBiNu1IR4PUdyIeKf261Fx2dZAVrS3MMkvB3eTd6FfIsBa2y ZVzdEDUSns3G/6/LBMaKF+j6cP0Ze2V5GuugA3rysEpGF1bP0Q+6BDD1Lpcugjx149Vq Q5Rg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vBOqylCGsRtsA/CHiSUcymkDXwo6gqRFN/tWR7AqT78=; b=nsQBy5cb8fS0g/OEJvBjKiS3P68H+QVet1dVNpBY5zfW7V4gFggZGikS38ZhZc8+lu eEvgRYURuomFrPafTEYB7SYJZitlRGU9E97j9P1sI04lgbBIkGkmuQtyllwIFuEDoKXH jJzNA2xr4BgniMF9VjBAxGO0xUKfiJvzcSpSFy2GoDlcGNhUuQBWm/div6p35G6ljQ/F rvKuud2dpoo2IJsSnSszmNE+LSVd1wjrvULS3aq+gO43Kqiyaa/uO9Zxezau9SBmESVD eegZTyennVEN535bZxujM4lPqNzCRBrG6JBFLYSLcnpTUhkbGVD+R/laPfponMC2p7KM sWLg== X-Gm-Message-State: AOAM530jeuxstU9stgN0HJf7qJ2myWGjTuV2g49dEUY6CJrpZqko4PiW 3sXb0mIU1MA75tBhNXOb4CqDBN3nO5l9wd+5eoE= X-Google-Smtp-Source: ABdhPJxFiEGn8cGn1lpDovb42YGe29R/6lq97EhqtHPddPCi5RxmYAbJofaH9TiM7HjB3u5WOoH86Bwu1+lTYPyuGho= X-Received: by 2002:a05:6902:1549:: with SMTP id r9mr8848683ybu.308.1627723705802; Sat, 31 Jul 2021 02:28:25 -0700 (PDT) MIME-Version: 1.0 References: <20210725215751.26295-1-mibodhi@gmail.com> <26c76127-7415-d3dd-f20d-ac05cd04a6b0@denx.de> In-Reply-To: <26c76127-7415-d3dd-f20d-ac05cd04a6b0@denx.de> From: Tony Dinh Date: Sat, 31 Jul 2021 02:28:14 -0700 Message-ID: Subject: Re: [PATCH] arm: mvebu: sata_mv failed to identify HDDs during cold start To: Stefan Roese Cc: U-Boot Mailing List , Simon Glass , Tom Rini Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean Hi Stefan, On Sat, Jul 31, 2021 at 12:31 AM Stefan Roese wrote: > > Hi Tony, > > On 25.07.21 23:57, Tony Dinh wrote: > > During cold start, with some HDDs, mv_sata_identify() does not populate > > the ID words on the 1st ATA ID command. In fact, the first ATA ID > > command will only power up the drive, and then the ATA ID command > > processing is lost in the process. > > > > Tests with: > > > > - Seagate ST9250320AS 250GB HDD and Seagate ST4000DM004-2CV104 4TB HDD. > > - Zyxel NSA310S (Kirkwood 88F6702), Marvell Dreamplug (Kirkwood 88F6281), > > Seagate GoFlex Home (Kirkwood 88F6281), Pogoplug V4 (Kirkwood 88F6192). > > > > Observation: > > > > - The Seagate ST9250320AS 250GB took about 3 seconds to spin up. > > - The Seagate ST4000DM004-2CV104 4TB took about 8 seconds to spin up. > > - mv_sata_identify() did not populate the ID words after the call to > > mv_ata_exec_ata_cmd_nondma(). > > - Attempt to insert a long delay of 30 seconds, ie. mdelay(30_000), after > > the call to ata_wait_register() inside mv_ata_exec_ata_cmd_nondma() did > > not help with the 4TB drive. The ID words were still empty after that 30s > > delay. > > IIRC, I've seen similar issues with a SATA SSD on theadorable before as > well. Thanks for digging into this. > > One some nit below... > > > Patch Description: > > > > - Added a second ATA ID command in mv_sata_identify(), which will be > > executed if the 1st ATA ID command did not return with valid ID words. > > - Use the HDD drive capacity in the ID words as a successful indicator of > > ATA ID command. > > - In the scenario where a box is rebooted, the 1st ATA ID command is always > > successful, so there is no extra time wasted. > > - In the scenario where a box is cold started, the 1st ATA command is the > > power up command. The 2nd ATA ID command alleviates the uncertainty of > > how long we have to wait for the ID words to be populated by the SATA > > controller. > > > > Signed-off-by: Tony Dinh > > --- > > > > drivers/ata/sata_mv.c | 29 +++++++++++++++++++++++++++-- > > 1 file changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c > > index 1012cb5374..7d1515d5f8 100644 > > --- a/drivers/ata/sata_mv.c > > +++ b/drivers/ata/sata_mv.c > > @@ -809,6 +809,7 @@ static int mv_ata_exec_ata_cmd_nondma(struct udevice *dev, int port, > > static int mv_sata_identify(struct udevice *dev, int port, u16 *id) > > { > > struct sata_fis_h2d h2d; > > + int len; > > > > memset(&h2d, 0, sizeof(struct sata_fis_h2d)); > > > > @@ -818,8 +819,32 @@ static int mv_sata_identify(struct udevice *dev, int port, u16 *id) > > /* Give device time to get operational */ > > mdelay(10); > > > > - return mv_ata_exec_ata_cmd_nondma(dev, port, &h2d, (u8 *)id, > > - ATA_ID_WORDS * 2, READ_CMD); > > + /* During cold start, with some HDDs, the first ATA ID command does > > + * not populate the ID words. In fact, the first ATA ID > > + * command will only power up the drive, and then the ATA ID command > > + * processing is lost in the process. > > + */ > > + len = mv_ata_exec_ata_cmd_nondma(dev, port, &h2d, (u8 *)id, > > + ATA_ID_WORDS * 2, READ_CMD); > > + > > + /* If drive capacity has been filled in, then it was successfully > > + * identified (the drive has been powered up before, i.e. > > + * this function is invoked during a reboot) > > + */ > > + if (ata_id_n_sectors(id) != 0) > > + return len; > > + > > + /* Issue the 2nd ATA ID command to make sure the ID words are > > + * populated properly. > > + */ > > + mdelay(10); > > + len = mv_ata_exec_ata_cmd_nondma(dev, port, &h2d, (u8 *)id, > > + ATA_ID_WORDS * 2, READ_CMD); > > + if (ata_id_n_sectors(id) != 0) > > + return len; > > + > > + printf("Err: Failed to identify SATA device %d\n", port); > > + return -1; > > Perhaps better to return -ENODEV or something similar instead of the -1 > here? Much agreed. However, I could not find any other error status that's a bit more descriptive. So I guess we should use -ENODEV. I will send a v2 version for this patch. Thanks, Tony > > Thanks, > Stefan