Vinodkumarreddy Polimera has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39613 )
Change subject: sc7180: Add display 10nm phy & pll programming support ......................................................................
Patch Set 22:
(27 comments)
addressed all the comments
https://review.coreboot.org/c/coreboot/+/39613/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39613/17//COMMIT_MSG@7 PS17, Line 7: sc7180: Add display 10nm phy & pll programming support [Patch 1 of 3]
We do not use the *PATCH 1 of 3* tags in coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/17//COMMIT_MSG@11 PS17, Line 11:
This is too terse for a 2500 lines diffstat. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 204: static uint32_t dsi_phy_rounddown(SIM_DOUBLE SIM_FLOAT_value)
I... don't even know what this is. If you want MAX(x, 0), then write that. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 649: while ((pll_status == 0) && (time_out)) {
Please use […]
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 1085: =
Please stick to the coding style.
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 1087: MDPExternalClockEntry *clk_list = sDSI0ExtClocks_6xx;
Same here as below: you don't need a clk_list structure if we'll only ever have one list! Just hardc […]
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 1133: //Phy set up
Please use /* C89 comments */ in coreboot and use the correct amount of whitespace.
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 1153: intf->dsi_phy_init = mdss_dsi_phy_10nm_init;
Do not build abstractions that you're not using. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/16/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/16/src/soc/qualcomm/sc7180/di... PS16, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright 2020 Qualcomm Inc. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License version 2 and : * only version 2 as published by the Free Software Foundation. : * : * 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. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/16/src/soc/qualcomm/sc7180/di... PS16, Line 16: include <device/mmio.h> : #include <lib.h> : #include <stdlib.h> : #include <console/console.h> : #include <delay.h> : #include <timer.h> : #include <string.h> : #include <stdint.h> : #include <sys/types.h> : #include <soc/clock.h> : #include <soc/display/mdssreg.h> : #include <soc/display/dsi_phy.h> : #include <soc/display/display_resources.h>
would you please include only what you use […]
Done
https://review.coreboot.org/c/coreboot/+/39613/16/src/soc/qualcomm/sc7180/di... PS16, Line 1056: goto out
return ret; ?
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 1044: int
Please use CB_SUCCESS and friends.
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 1049: printk(BIOS_ERR, "%s: pinfo NULL, error\n", __func__);
Error messages should be user understandable. This is written like a debug message. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 1053: set up
The noun is spelled without a space. Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 1056: goto out;
Just return?
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 16: #include <arch/mmio.h>
Always include <device/mmio.h>, not <arch/mmio.h> directly.
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 227: config->disable_prescaler = false;
Why are you setting up this huge data structure full of hardcoded values? Do these ever change in ou […]
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 332:
Please remove the blank lines at the end of the file.
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/display_resources.h:
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/in... PS17, Line 49:
Please remove the blank lines.
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/display/display_resources.h:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 24: static char byte0_intf_clk_name[MDP_MAX_CLOCK_NAME] = "disp_cc_mdss_byte0_intf_clk";
Do not use strings to identify things, make an enum or something. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 29: typedef struct {
Do not typedef structures in coreboot.
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 31: uint32_t uClkSource; /* Primary Clock Source */
Do not use camelCase for struct member...
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 38: } MDPExternalClockEntry;
...or type names (or anything else, actually), we use snake_case for those.
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/display/msm_panel.h:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 27: #define ERROR -1
Just use 0 and -1 directly in the code.
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 58: uint32_t v_pulse_width;
For things that are in struct edid you should be using that struct, not define your own structure fo […]
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/mdss_6_2_0.h:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 23: #define readl(a) read32((void *)(size_t)(a))
As mentioned in other CL, do not add your own definitions for these.
Done
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 33: #define HWIO_OUT_FLD(regVal, io, field, fldVal) \
Use struct overlays to model hardware registers and don't create your own macro infrastructure inste […]
Done