Masanori Ogino has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 6:
(8 comments)
Great work to reduce duplicates!
I have some minor comments, not affecting main functionalities.
https://review.coreboot.org/c/coreboot/+/37137/6/src/include/ahci.h File src/include/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/6/src/include/ahci.h@1 PS6, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org : * : * This program is free software; you can redistribute it and/or : * modify it under the terms of the GNU General Public License as : * published by the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ You can replace this copyright and license information with:
/* SPDX-License-Identifier: GPL-2.0-only */
as per recent treewide changes.
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org : * : * This program is free software; you can redistribute it and/or : * modify it under the terms of the GNU General Public License as : * published by the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ /* SPDX-License-Identifier: GPL-2.0-only */
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org : * : * SPDX-License-Identifier: GPL-2.0-or-later : */ /* SPDX-License-Identifier: GPL-2.0-or-later */
(By the way, is it intentional to publish headers in GPLv2 only while this file in GPLv2 or later?)
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 21: * @param iss Limit the controller to sata gen x s/sata gen x/SATA Gen x/
It might be better if this states that `iss == 0` means no limit.
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 62: printk(BIOS_INFO, "AHCI: supports SATA Gen%d\n", (reg32 >> 20) & 0xf); Nits: a space between "Gen" and "%d"?
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 81: printk(BIOS_INFO, "AHCI: limited to Gen%d\n", max_iss); Nits: a space between "Gen" and "%d"?
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 115: printk(BIOS_ERR, "AHCI: Invalid port map given!\n"); It might be better if this message contains the port map (in hexadecimal).
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 123: reg32 = read32(abar + AHCI_VENDOR); : reg32 &= ~clear_vnd; : write32(abar + AHCI_VENDOR, reg32); Not sure but you could save 1 read + 1 write by surrounding this block with `if (clear_vnd) { ... }`. Please ignore this comment if it is needed even if clear_vnd is zero.