PATCH: Fix AHCI init regression on the coreboot payload

[Leah Rowe]

Hello SeaBIOS community,

Please look at the patch I've attached to this email. It fixes a 
regression recently introduced in SeaBIOS, introduced by SeaBIOS git 
commit ID 8863cbbd. I request that this patch by merged, and I will now 
explain why.

A recent SeaBIOS patch (commit 8863cbbd) adds AHCI controller reset 
prior to enablement, as a way to make SeaBIOS's AHCI driver work 
correctly on CSM-based setups.

However, that commit (commit 8863cbbd) broke AHCI initialisation on the 
Lenovo ThinkPad T420 when testing SeaBIOS as a coreboot payload. It 
caused an AHCI timeout. I tried to include these logs on the mailing 
list but it came back telling me my message was too big.

Anyways

To mitigate this, I have provided for SeaBIOS a patch to this email 
(0004-ahci-Only-reset-controller-on-CSM.patch). My patch retains the new 
behaviour, resetting the AHCI controller prior to enablement, so that it 
will continue to function correctly on CSM, but it *only* applies that 
new behaviour on CSM setups.

With my patch, the *old* behaviour is used (enable the controller from 
its current state), if a CSM is not in use. This was confirmed to work 
correctly, fixing the issue when re-tested on the same ThinkPad T420.

I am also using this fix in Libreboot, see:

https://browse.libreboot.org/lbmk.git/commit/?id=c073ee9d4fc4a631c16ff681bb6...

That's where the patch is from. I put it there first. SeaBIOS devs are 
welcome to merge this if they wish.

[Gerd Hoffmann]

On Sat, May 03, 2025 at 03:50:22PM +0100, Leah Rowe via SeaBIOS wrote:
> Hello SeaBIOS community,
>
> Please look at the patch I've attached to this email. It fixes a regression
> recently introduced in SeaBIOS, introduced by SeaBIOS git commit ID
> 8863cbbd. I request that this patch by merged, and I will now explain why.
>
> A recent SeaBIOS patch (commit 8863cbbd) adds AHCI controller reset prior to
> enablement, as a way to make SeaBIOS's AHCI driver work correctly on
> CSM-based setups.
>
> However, that commit (commit 8863cbbd) broke AHCI initialisation on the
> Lenovo ThinkPad T420 when testing SeaBIOS as a coreboot payload. It caused
> an AHCI timeout. I tried to include these logs on the mailing list but it
> came back telling me my message was too big.

Hmm, maybe we should better carry over the old control bits over the
reset.  Does this work for you (and if so, what does the added printk
print) ?

pci_enable_busmaster(pci);

+    u32 val = ahci_ctrl_readl(ctrl, HOST_CTL);
+    dprintf(1, "AHCI host ctl 0x%x\n", val);
     ahci_ctrl_writel(ctrl, HOST_CTL, HOST_CTL_RESET);
-    ahci_ctrl_writel(ctrl, HOST_CTL, HOST_CTL_AHCI_EN);
+    ahci_ctrl_writel(ctrl, HOST_CTL, val | HOST_CTL_AHCI_EN);

ctrl->caps = ahci_ctrl_readl(ctrl, HOST_CAP);
     ctrl->ports = ahci_ctrl_readl(ctrl, HOST_PORTS_IMPL);

If that does not help I'd rather try to do the controller reset on qemu
only where we know it works.

take care,
  Gerd

[Leah Rowe]

Do I do that in addition to, or instead of, my patch?

1) Replace my patch with your change

or

2) Add your change on top of my change

I'm guessing it's #1, but please confirm and I'll have that tested.

On 05/05/2025 09:46, Gerd Hoffmann via SeaBIOS wrote:
> On Sat, May 03, 2025 at 03:50:22PM +0100, Leah Rowe via SeaBIOS wrote:
> > Hello SeaBIOS community,
> >
> > Please look at the patch I've attached to this email. It fixes a regression
> > recently introduced in SeaBIOS, introduced by SeaBIOS git commit ID
> > 8863cbbd. I request that this patch by merged, and I will now explain why.
> >
> > A recent SeaBIOS patch (commit 8863cbbd) adds AHCI controller reset prior to
> > enablement, as a way to make SeaBIOS's AHCI driver work correctly on
> > CSM-based setups.
> >
> > However, that commit (commit 8863cbbd) broke AHCI initialisation on the
> > Lenovo ThinkPad T420 when testing SeaBIOS as a coreboot payload. It caused
> > an AHCI timeout. I tried to include these logs on the mailing list but it
> > came back telling me my message was too big.
>
> Hmm, maybe we should better carry over the old control bits over the
> reset.  Does this work for you (and if so, what does the added printk
> print) ?
>   pci_enable_busmaster(pci);
>
> u32 val = ahci_ctrl_readl(ctrl, HOST_CTL);
> dprintf(1, "AHCI host ctl 0x%x\n", val);
>  ahci_ctrl_writel(ctrl, HOST_CTL, HOST_CTL_RESET);
> ahci_ctrl_writel(ctrl, HOST_CTL, HOST_CTL_AHCI_EN);
>
> ahci_ctrl_writel(ctrl, HOST_CTL, val | HOST_CTL_AHCI_EN);
>
> ctrl->caps = ahci_ctrl_readl(ctrl, HOST_CAP);
>  ctrl->ports = ahci_ctrl_readl(ctrl, HOST_PORTS_IMPL);
>
> If that does not help I'd rather try to do the controller reset on qemu
> only where we know it works.
>
> take care,
>    Gerd
>
> SeaBIOS mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[Kevin O'Connor]

On Mon, May 05, 2025 at 11:12:37AM +0100, Leah Rowe via SeaBIOS wrote:
> Do I do that in addition to, or instead of, my patch?
> Replace my patch with your change
>
> or
> Add your change on top of my change
>
> I'm guessing it's #1, but please confirm and I'll have that tested.

Yes - please return to the upstream code, make the suggested changes,
and let us know the results.

Cheers,
-Kevin