Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33582 )
Change subject: libpayload/storage: Add NVMe driver ......................................................................
Patch Set 14:
(7 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/driver... File payloads/libpayload/drivers/storage/nvme.c:
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/driver... PS11, Line 2: * This file is part of the libpayload project. : * : * Copyright (C) 2019 secunet Security Networks AG : * : * Redistribution and use in source and binary forms, with or without : * modification, are permitted provided that the following conditions : * are met: : * 1. Redistributions of source code must retain the above copyright : * notice, this list of conditions and the following disclaimer. : * 2. Redistributions in binary form must reproduce the above copyright : * notice, this list of conditions and the following disclaimer in the : * documentation and/or other materials provided with the distribution. : * 3. The name of the author may not be used to endorse or promote products : * derived from this software without specific prior written permission. : * : * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND : * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE : * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE : * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE : * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL : * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS : * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) : * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT : * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY : * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF : * SUCH DAMAGE.
We can use SPDX.
Done
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/driver... PS11, Line 289: // if io queues NULL
seems redundant
Done
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/driver... PS11, Line 304: // reset controller
huh? seems spurious
Done
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/driver... PS11, Line 372: uint32_t cc = 0;
Why declare it already? we could also just have […]
Done
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/driver... PS11, Line 426: printf("failed\n");
There are already more elaborate error messages on every path leading here.
Done
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/includ... File payloads/libpayload/include/storage/nvme.h:
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/includ... PS11, Line 1: /* : * This file is part of the libpayload project. : * : * Copyright (C) 2019 secunet Security Networks AG : * : * Redistribution and use in source and binary forms, with or without : * modification, are permitted provided that the following conditions : * are met: : * 1. Redistributions of source code must retain the above copyright : * notice, this list of conditions and the following disclaimer. : * 2. Redistributions in binary form must reproduce the above copyright : * notice, this list of conditions and the following disclaimer in the : * documentation and/or other materials provided with the distribution. : * 3. The name of the author may not be used to endorse or promote products : * derived from this software without specific prior written permission. : * : * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND : * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE : * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE : * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE : * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL : * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS : * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) : * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT : * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY : * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF : * SUCH DAMAGE. : */
We can use SPDX.
Done
https://review.coreboot.org/c/coreboot/+/33582/11/payloads/libpayload/includ... PS11, Line 36: struct nvme_dev { : storage_dev_t storage_dev; : : pcidev_t pci_dev; : void *config; : struct { : void *base; : uint32_t *bell; : uint16_t idx; // bool pos 0 or 1 : uint16_t round; // bool round 0 or 1+0xd : } queue[4]; : : uint64_t *prp_list; : };
Doesn't need to be in the API header file, does it?
Done