Attention is currently required from: Ravi Kumar Bokka, Furquan Shaikh, Julius Werner, Prasad Malisetty, mturney mturney. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/53902 )
Change subject: sc7280: Add PCIe host controller driver ......................................................................
Patch Set 81: Code-Review-1
(68 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/53902/comment/ca5d15a5_40df4a6e PS81, Line 7: sc7280 soc/qualcomm/sc7280
https://review.coreboot.org/c/coreboot/+/53902/comment/20f416dc_faa8e4bc PS81, Line 9: Add support for PCIe host driver to configure : in RC (root complex) mode. Host driver will power on : the PHY, enable the PCIe link and start the bus enumeration. Please reflow for 72 characters per line.
https://review.coreboot.org/c/coreboot/+/53902/comment/887c6ae0_270edede PS81, Line 12: Please mention the source for your implementation (datasheet name, revision, section), describe the implementation, and discuss why it’s implemented that way and not more of coreboot’s “PCIe framework” is used. Currently, it looks like a lot of duplication, and maintenance is going to be hard.
Also, is there already another Qualcomm PCIe implementation in U-Boot or the Linux kernel for example?
https://review.coreboot.org/c/coreboot/+/53902/comment/a640ee65_eec69613 PS81, Line 14: TEST=Validated on qualcomm sc7280 development board With what PCIe card?
File src/mainboard/google/herobrine/mainboard.c:
https://review.coreboot.org/c/coreboot/+/53902/comment/c5564a89_7007267a PS81, Line 37: /* Assert NVMe External LDO's GPIO */ Why is here something NVMe specific?
https://review.coreboot.org/c/coreboot/+/53902/comment/f86ab05c_3fb8258b PS81, Line 43: pcie1 What does the index 1 mean?
https://review.coreboot.org/c/coreboot/+/53902/comment/a041697e_932b59f8 PS81, Line 43: /*Initialize Missing space.
https://review.coreboot.org/c/coreboot/+/53902/comment/27a4ae07_c1973efc PS81, Line 124: RC root complex
https://review.coreboot.org/c/coreboot/+/53902/comment/00308ee1_a06ffd5b PS81, Line 125: setup_pcie Why not: setup_pcie_in_rootcomplex_mode() or something similar? Then you also can get rid of the comment.
File src/soc/qualcomm/sc7280/include/soc/addressmap.h:
https://review.coreboot.org/c/coreboot/+/53902/comment/804a83a3_54b226fd PS81, Line 91: definitions */ One space too much.
https://review.coreboot.org/c/coreboot/+/53902/comment/cc993651_4441b855 PS81, Line 100: /*PHY Missing space.
File src/soc/qualcomm/sc7280/include/soc/pcie.h:
https://review.coreboot.org/c/coreboot/+/53902/comment/8e6f31d7_55a304b1 PS81, Line 2: #include <types.h> Please add a blank line above.
https://review.coreboot.org/c/coreboot/+/53902/comment/83b26c2e_ba10cd59 PS81, Line 17: Tab for alignment?
https://review.coreboot.org/c/coreboot/+/53902/comment/5abddc40_bfc02cb3 PS81, Line 70: *< Remove?
https://review.coreboot.org/c/coreboot/+/53902/comment/83709bea_e8fc51b1 PS81, Line 115: From 4.80 core version What “core version”?
https://review.coreboot.org/c/coreboot/+/53902/comment/027c8499_e9a72621 PS81, Line 152: */ Remove one space.
https://review.coreboot.org/c/coreboot/+/53902/comment/39456142_5713a341 PS81, Line 173: Start on the bus Sounds strange.
https://review.coreboot.org/c/coreboot/+/53902/comment/88444b2a_a18697c7 PS81, Line 196: )<<8 Please add spaces around the operator.
https://review.coreboot.org/c/coreboot/+/53902/comment/0052dead_7dacd196 PS81, Line 215: } Should the {} align?
https://review.coreboot.org/c/coreboot/+/53902/comment/21a66ff4_2a6eb524 PS81, Line 217: /* PCIe setup */ The comment is redundant. Either remove or elaborate.
File src/soc/qualcomm/sc7280/pcie_host.c:
https://review.coreboot.org/c/coreboot/+/53902/comment/7a89f649_a93483be PS81, Line 32: *@lanes Missing space.
https://review.coreboot.org/c/coreboot/+/53902/comment/c3c70274_95cae586 PS81, Line 73: int unsigned int (also below)
https://review.coreboot.org/c/coreboot/+/53902/comment/793770ac_f4b1b159 PS81, Line 97: int Can be unsigned?
https://review.coreboot.org/c/coreboot/+/53902/comment/f1e217ee_448b0cbd PS81, Line 231: .parf = (void *) PCIE1_PCIE_PARF, : .dbi_base = (void *) PCIE1_GEN3X2_PCIE_DBI, : .elbi = (void *) PCIE1_GEN3X2_PCIE_ELBI, : .atu_base = (void *) PCIE1_GEN3X2_DWC_PCIE_DM_IATU, : .cfg_base = (void *) PCIE1_GEN3x2_CONF, : .pcie1_bcr = (void *) PCIE1_BCR, : .qmp_phy_bcr = (void *) GCC_PCIE_1_PHY_BCR, : .lanes = PCIE1_3x2_NUM_LANES, : .cfg_size = PCIE1_GEN3x2_CONF_SIZE, : .perst = GPIO(2), : : /* Store the IO and MEM windows settings for future use by the ATU */ : .io.phys_start = PCIE1_IO_ADDR, /* IO base */ : .io.bus_start = PCIE1_IO_ADDR, /* IO_bus_addr */ : .io.size = PCIE1_IO_SIZE, /* IO size */ : : .mem.phys_start = PCIE1_MEM_ADDR, /* MEM base */ : .mem.bus_start = PCIE1_MEM_ADDR, /* MEM_bus_addr */ : .mem.size = PCIE1_MEM_SIZE, Please use either tabs or spaces to align the equal sign (=).
https://review.coreboot.org/c/coreboot/+/53902/comment/2c6d69c5_780aadc3 PS81, Line 284: old old_value?
https://review.coreboot.org/c/coreboot/+/53902/comment/a2355ee2_0893ff9e PS81, Line 284: uint32_t offset_t?
https://review.coreboot.org/c/coreboot/+/53902/comment/3f71098f_12a1da3c PS81, Line 285: uint8_t size); Fits on line above.
https://review.coreboot.org/c/coreboot/+/53902/comment/4ce1a639_9bfe749b PS81, Line 285: uint8_t size_t
https://review.coreboot.org/c/coreboot/+/53902/comment/5bed1301_ff722a8e PS81, Line 287: int qcom_pcie_conv_32_to_size(int value, int offset, int size); Can all arguments be unsigned? Even use `offset_t` and `size_t`?
https://review.coreboot.org/c/coreboot/+/53902/comment/d9d3726a_9b769f2c PS81, Line 290: *Convtert Missing space, and typo: Convert.
https://review.coreboot.org/c/coreboot/+/53902/comment/91ac82f9_e7b251a9 PS81, Line 291: * Remove blank line.
https://review.coreboot.org/c/coreboot/+/53902/comment/9bb97800_9811f759 PS81, Line 306: *Convtert 8/16 bit data into 32 bit size for compatibility. : * Ditto.
https://review.coreboot.org/c/coreboot/+/53902/comment/42187186_314ab382 PS81, Line 310: uint8_t size) Fits on line above.
https://review.coreboot.org/c/coreboot/+/53902/comment/3dea82a3_fbb9a464 PS81, Line 336: en Please write it out.
https://review.coreboot.org/c/coreboot/+/53902/comment/80070c66_1a031600 PS81, Line 336: _enable Remove that from the function name, as you pass a parameter?
https://review.coreboot.org/c/coreboot/+/53902/comment/8da359d5_6bb22aa0 PS81, Line 351: uint32_t index Can `unsigned int` be used?
https://review.coreboot.org/c/coreboot/+/53902/comment/5dc68acb_c3be38dc PS81, Line 372: retries Make it `unsigned int`.
https://review.coreboot.org/c/coreboot/+/53902/comment/35f7a4cd_4f9633ca PS81, Line 388: /* Add a blank line above.
https://review.coreboot.org/c/coreboot/+/53902/comment/2f22140f_291b29f8 PS81, Line 388: /* : * Make sure ATU enable takes effect before any subsequent config : * and I/O accesses. : */ Maybe use concise multi-line comment style.
https://review.coreboot.org/c/coreboot/+/53902/comment/29f307de_ce001b9a PS81, Line 399: printk(BIOS_EMERG, "outbound iATU is not being enabled\n"); Please extend:
Outbound iATU could not be enabled after %u … attempts
https://review.coreboot.org/c/coreboot/+/53902/comment/5c89868a_647095eb PS81, Line 407: GPIO_16MA, GPIO_OUTPUT); Fits on line above.
https://review.coreboot.org/c/coreboot/+/53902/comment/e85c4c1b_7876d2b7 PS81, Line 415: mdelay(100); Please add a comment explaining these long delays. Referencing the PCIe specification is a good idea.
https://review.coreboot.org/c/coreboot/+/53902/comment/b68734f1_1fc53558 PS81, Line 426: mdelay(50); 150 ms delays need to be well explained in coreboot. Please extend the comment.
https://review.coreboot.org/c/coreboot/+/53902/comment/9f4814bf_1eee4229 PS81, Line 427: printk(BIOS_NOTICE, "PCIe PERST desserted\n"); Also info?
https://review.coreboot.org/c/coreboot/+/53902/comment/1bda0cf3_f8f463f4 PS81, Line 452: DISABLE Please use standard types.
https://review.coreboot.org/c/coreboot/+/53902/comment/89c7a325_045e697e PS81, Line 453: printk(BIOS_INFO, "Link speed configured in Gen %d\n", cap_speed); Please give an example message.
https://review.coreboot.org/c/coreboot/+/53902/comment/3f1808d2_35c549d4 PS81, Line 459: 1 (true) The type is bool.
https://review.coreboot.org/c/coreboot/+/53902/comment/d202657a_ebed8008 PS81, Line 465: register*/ Missing space.
https://review.coreboot.org/c/coreboot/+/53902/comment/bfe123d6_5a8a60b1 PS81, Line 476: int bool?
https://review.coreboot.org/c/coreboot/+/53902/comment/7ccacf7f_a60a626c PS81, Line 478: int unsigned int
https://review.coreboot.org/c/coreboot/+/53902/comment/839c277e_19d8bcb7 PS81, Line 486: printk(BIOS_EMERG, "link never came up\n"); Mention PCIe in the message, and the number of tries.
https://review.coreboot.org/c/coreboot/+/53902/comment/74901646_f0750dec PS81, Line 495: printk(BIOS_EMERG, "PCIe Link is already up\n"); Wrong log level?
https://review.coreboot.org/c/coreboot/+/53902/comment/2b269d71_7cd9346a PS81, Line 496: 0 true
https://review.coreboot.org/c/coreboot/+/53902/comment/14499d9f_e0db054d PS81, Line 509: printk(BIOS_EMERG, "Link up\n"); Wrong log levle, please elaborate.
https://review.coreboot.org/c/coreboot/+/53902/comment/2820fc86_8bb8bc9c PS81, Line 510: 0 true
https://review.coreboot.org/c/coreboot/+/53902/comment/04fb8247_f5760793 PS81, Line 512: /* : * Link can be established in Gen 1. still need to wait : */ Please add a blank line above, and use a single line comment.
https://review.coreboot.org/c/coreboot/+/53902/comment/49bb7dc7_386b644f PS81, Line 515: udelay(100); Why the delay?
https://review.coreboot.org/c/coreboot/+/53902/comment/4692e84b_ac0d61e6 PS81, Line 516: 1 false
https://review.coreboot.org/c/coreboot/+/53902/comment/ce4628dd_c2db4232 PS81, Line 521: cfg_size unsigned int
https://review.coreboot.org/c/coreboot/+/53902/comment/ba83770f_f72456df PS81, Line 528: /* Accessing root port configuration space. */ Dots can be remove from short comments.
https://review.coreboot.org/c/coreboot/+/53902/comment/461b477f_d4200775 PS81, Line 535: 4 We have the macro, no need to mention the value?
https://review.coreboot.org/c/coreboot/+/53902/comment/9a8a5dd6_09181c14 PS81, Line 534: if (pcierc->current_bus == 1) : /* For local bus, change TLP Type field to 4. */ : atu_type = PCIE_ATU_TYPE_CFG0; : else : /* Otherwise, change TLP Type field to 5. */ : atu_type = PCIE_ATU_TYPE_CFG1; Use ternary operator, and only one comment line.
https://review.coreboot.org/c/coreboot/+/53902/comment/4a5c906b_cc702f8a PS81, Line 550: int Always returns 0?
https://review.coreboot.org/c/coreboot/+/53902/comment/c8eb6146_b7fda661 PS81, Line 573: int Ditto.
https://review.coreboot.org/c/coreboot/+/53902/comment/4379f22c_12610574 PS81, Line 601: bar unsigned int
https://review.coreboot.org/c/coreboot/+/53902/comment/d505ea66_12f57346 PS81, Line 606: I stop reviewing here. The code needs to be improved, and the types and style issues fixed first before showing it to reviewers.
https://review.coreboot.org/c/coreboot/+/53902/comment/a3d7d9d5_a1da0375 PS81, Line 842: int unsigned int
https://review.coreboot.org/c/coreboot/+/53902/comment/cd0f784b_0c24384e PS81, Line 862: long unsigned long?
-
Paul Menzel (Code Review)