[v1] ahci: Controller reset fixes

[SeaBIOS] [PATCH] ahci: Controller reset fixes

Posted by Jiaxun Yang 7 months ago

After adding AHCI controller functionality there are multiple
reports on AHCI booting regression.

As per my experiments on various machines, to reset controller
properly it is necessary to poll HOST_CTL_RESET bit until it's
clear. It is also required to read back HOST_CTL after changing
HOST_CTL_AHCI_EN bits to ensure the controller has accepted write.

Tested on ASMedia ASM1061, Intel H61 native SATA and AMD Phoenix
native SATA.

Link: https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/RDNRKWBN4N5XQX2TQMM5P4WZ2OOPPNAM/
Link: https://github.com/FlyGoat/csmwrap/issues/14
Fixes: 8863cbbd15a7 ("ahci: add controller reset")
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
To: seabios@seabios.org
---
 src/hw/ahci.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/hw/ahci.c b/src/hw/ahci.c
index 2285d33d4ae21335e5222ab04608685b5bff3763..7e7a03ddd010d53fa439c8ca49505a2da00241f4 100644
--- a/src/hw/ahci.c
+++ b/src/hw/ahci.c
@@ -660,8 +660,23 @@ ahci_controller_setup(struct pci_device *pci)
 
     pci_enable_busmaster(pci);
 
-    ahci_ctrl_writel(ctrl, HOST_CTL, HOST_CTL_RESET);
-    ahci_ctrl_writel(ctrl, HOST_CTL, HOST_CTL_AHCI_EN);
+    u32 val = ahci_ctrl_readl(ctrl, HOST_CTL);
+    ahci_ctrl_writel(ctrl, HOST_CTL, val | HOST_CTL_RESET);
+    u32 end = timer_calc(AHCI_RESET_TIMEOUT);
+    for (;;) {
+        val = ahci_ctrl_readl(ctrl, HOST_CTL);
+        if (!(val & HOST_CTL_RESET))
+            break;
+        if (timer_check(end)) {
+            warn_timeout();
+            dprintf(1, "AHCI: controller reset failed\n");
+            free(ctrl);
+            return;
+        }
+        yield();
+    }
+    ahci_ctrl_writel(ctrl, HOST_CTL, val | HOST_CTL_AHCI_EN);
+    (void)ahci_ctrl_readl(ctrl, HOST_CTL); /* Flush */
 
     ctrl->caps = ahci_ctrl_readl(ctrl, HOST_CAP);
     ctrl->ports = ahci_ctrl_readl(ctrl, HOST_PORTS_IMPL);

---
base-commit: 9029a010ec413e6c3c0eb52c29c252a5b9a9f774
change-id: 20250522-ahci-8ac63999b084

Best regards,
-- 
Jiaxun Yang <jiaxun.yang@flygoat.com>

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] ahci: Controller reset fixes

Posted by Gerd Hoffmann via SeaBIOS 6 months, 3 weeks ago

  Hi,

> As per my experiments on various machines, to reset controller
> properly it is necessary to poll HOST_CTL_RESET bit until it's
> clear.

Looks good to me.  Thanks for hunting that down.  That is one of the
areas where qemu emulation and physical hardware have slightly different
behavior (qemu completes the reset instantly whereas physical hardware
may actually need some time for that).

> Tested on ASMedia ASM1061, Intel H61 native SATA and AMD Phoenix
> native SATA.

Additional test results are welcome.

With that (most likely) being settled I think we can look for forward
to finally tag the 1.17.0 release soon.

take care,
  Gerd

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] ahci: Controller reset fixes

Posted by Philippe Mathieu-Daudé 6 months, 3 weeks ago

On 26/5/25 13:35, Gerd Hoffmann via SeaBIOS wrote:
>    Hi,
> 
>> As per my experiments on various machines, to reset controller
>> properly it is necessary to poll HOST_CTL_RESET bit until it's
>> clear.
> 
> Looks good to me.  Thanks for hunting that down.  That is one of the
> areas where qemu emulation and physical hardware have slightly different
> behavior (qemu completes the reset instantly whereas physical hardware
> may actually need some time for that).

Generally if some delay is mentioned in a datasheet, QEMU should
model it IMHO.

>> Tested on ASMedia ASM1061, Intel H61 native SATA and AMD Phoenix
>> native SATA.
> 
> Additional test results are welcome.
> 
> With that (most likely) being settled I think we can look for forward
> to finally tag the 1.17.0 release soon.
> 
> take care,
>    Gerd
> 
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] ahci: Controller reset fixes

Posted by Jiaxun Yang 6 months, 3 weeks ago


在2025年5月26日周一 下午12:35，Gerd Hoffmann写道：
> Hi,

Hi all,

>
>> As per my experiments on various machines, to reset controller
>> properly it is necessary to poll HOST_CTL_RESET bit until it's
>> clear.
>
> Looks good to me.  Thanks for hunting that down.  That is one of the
> areas where qemu emulation and physical hardware have slightly different
> behavior (qemu completes the reset instantly whereas physical hardware
> may actually need some time for that).

Thanks, I'll respin this patch to address comments from Paul.

>
>> Tested on ASMedia ASM1061, Intel H61 native SATA and AMD Phoenix
>> native SATA.
>
> Additional test results are welcome.
>
> With that (most likely) being settled I think we can look for forward
> to finally tag the 1.17.0 release soon.

I have a couple of build system and feature enhancements that maybe benefit
next release.

I'll aim to send them later today or tomorrow.

Thanks
-- 
- Jiaxun
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] ahci: Controller reset fixes

Posted by Paul Menzel 6 months, 4 weeks ago

[Cc: + Leah]

Dear Jiaxun,


Thank you for your patch. Leah, could you please test this on top of 
SeaBIOS master branch (without any of your fixes)?

Jiaxun, some minor things. For the summary/title a statement would be nice:

ahci: Fix hangs due to controller reset


Am 22.05.25 um 16:54 schrieb Jiaxun Yang:
> After adding AHCI controller functionality there are multiple

… reset functionality …

> reports on AHCI booting regression.
> 
> As per my experiments on various machines, to reset controller
> properly it is necessary to poll HOST_CTL_RESET bit until it's
> clear. It is also required to read back HOST_CTL after changing
> HOST_CTL_AHCI_EN bits to ensure the controller has accepted write.
> 
> Tested on ASMedia ASM1061, Intel H61 native SATA and AMD Phoenix
> native SATA.

Nice. Thank you for testing this.

> Link: https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/RDNRKWBN4N5XQX2TQMM5P4WZ2OOPPNAM/
> Link: https://github.com/FlyGoat/csmwrap/issues/14
> Fixes: 8863cbbd15a7 ("ahci: add controller reset")
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> To: seabios@seabios.org
> ---
>   src/hw/ahci.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/src/hw/ahci.c b/src/hw/ahci.c
> index 2285d33d4ae21335e5222ab04608685b5bff3763..7e7a03ddd010d53fa439c8ca49505a2da00241f4 100644
> --- a/src/hw/ahci.c
> +++ b/src/hw/ahci.c
> @@ -660,8 +660,23 @@ ahci_controller_setup(struct pci_device *pci)
>   
>       pci_enable_busmaster(pci);
>   
> -    ahci_ctrl_writel(ctrl, HOST_CTL, HOST_CTL_RESET);
> -    ahci_ctrl_writel(ctrl, HOST_CTL, HOST_CTL_AHCI_EN);
> +    u32 val = ahci_ctrl_readl(ctrl, HOST_CTL);
> +    ahci_ctrl_writel(ctrl, HOST_CTL, val | HOST_CTL_RESET);
> +    u32 end = timer_calc(AHCI_RESET_TIMEOUT);
> +    for (;;) {
> +        val = ahci_ctrl_readl(ctrl, HOST_CTL);
> +        if (!(val & HOST_CTL_RESET))
> +            break;
> +        if (timer_check(end)) {
> +            warn_timeout();
> +            dprintf(1, "AHCI: controller reset failed\n");
> +            free(ctrl);
> +            return;
> +        }
> +        yield();
> +    }
> +    ahci_ctrl_writel(ctrl, HOST_CTL, val | HOST_CTL_AHCI_EN);
> +    (void)ahci_ctrl_readl(ctrl, HOST_CTL); /* Flush */
>   
>       ctrl->caps = ahci_ctrl_readl(ctrl, HOST_CAP);
>       ctrl->ports = ahci_ctrl_readl(ctrl, HOST_PORTS_IMPL);

Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org