Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43751 )
Change subject: drivers/generic/genesyslogic: Add driver for Genesys Logic GL9763E ......................................................................
Patch Set 3: Code-Review+1
(22 comments)
Much better!
Patch Set 1:
Hello everybody, how are you ? I am Sean Chen from GenesysLogic.
I'm doing well, nice to meet you.
Patch Set 1:
Pleased to meet you. I'm Renius Chen from GenesysLogic.
Nice to meet you as well.
https://review.coreboot.org/c/coreboot/+/43751/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43751/1//COMMIT_MSG@7 PS1, Line 7: generic/genesyslogic
Maybe this patch does not look like 'generic'. […]
I think `generic` drivers are hardware-agnostic stuff like `cbfs-serial`. Since there are other vendor folders directly under `src/drivers/`, I thought `drivers/genesyslogic/` would be better.
In any case, it's not a big deal, it's only about folder placement.
https://review.coreboot.org/c/coreboot/+/43751/1//COMMIT_MSG@8 PS1, Line 8:
Thank you, if you amended the commit message (`git commit --amend` if it’s the commit on top, or `gi […]
Done
https://review.coreboot.org/c/coreboot/+/43751/3/src/drivers/generic/genesys... File src/drivers/generic/genesyslogic/Kconfig:
https://review.coreboot.org/c/coreboot/+/43751/3/src/drivers/generic/genesys... PS3, Line 1: GLI9763E GL9763E
https://review.coreboot.org/c/coreboot/+/43751/3/src/drivers/generic/genesys... File src/drivers/generic/genesyslogic/gl9763e.c:
https://review.coreboot.org/c/coreboot/+/43751/3/src/drivers/generic/genesys... PS3, Line 22: MAX_SSC_30000PPM); nit: this fits on the previous line (coreboot's line length limit is 96 characters)
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... File src/drivers/generic/genesyslogic/gli9763e.h:
PS1:
Change the name to GL9763E.
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 3: Driver
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 7: #define PCI_VENDOR_ID_GLI 0x17a0 : #define PCI_DEVICE_ID_GLI_9763E 0xe763
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 10: VHS
Ha 😄, you can call it this if you want. […]
Oh, Vendor Header Space makes more sense. Thank you for the explanation 😄
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 10: PCIE_GLI_9763E
Remove prefix XX_GLI_9763E_XX. […]
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 11: 0xF00
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 11: GLI_9763E_VHS_REV
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 13: #define GLI_9763E_VHS_REV_R 0x0 : #define GLI_9763E_VHS_REV_M 0x1 : #define GLI_9763E_VHS_REV_W 0x2
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 26: GLI_9763E_PLL_CTL_2_MAX_SSC
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... File src/drivers/generic/genesyslogic/gli9763e.c:
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 13: static inline int pci_read_config_dword(struct device *dev, int where, : u32 *val) : { : *val = pci_read_config32(dev, where); : return 0; : } : : static inline int pci_write_config_dword(struct device *dev, int where, : u32 val) : { : pci_write_config32(dev, where, val); : return 0; : }
Removed.
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 27: gli_set_gl9763e
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 27: pdev
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 33: (u32)
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 36: pci_read_config_dword(pdev, PCIE_GLI_9763E_SCR, &value); : value |= GLI_9763E_SCR_AXI_REQ; : pci_write_config_dword(pdev, PCIE_GLI_9763E_SCR, value);
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 40: pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG_REG_2, &value); : value &= ~GLI_9763E_CFG_REG_2_L0S; : pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG_REG_2, value);
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 47: pci_write_config_dword(pdev, PCIE_GLI_9763E_PLL_CTL_2, value);
Modified.
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 49: pci_read_config_dword(pdev, PCIE_GLI_9763E_PLL_CTL, &value); : value |= GLI_9763E_PLL_CTL_SSC; : pci_write_config_dword(pdev, PCIE_GLI_9763E_PLL_CTL, value);
Done
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 56: pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
VHS has write protection. Enable writing and disable writing. […]
Thank you for the explanation