Ben Chuang 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:
(18 comments)
Updated.
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... File src/drivers/generic/genesyslogic/gli9763e.h:
PS1:
nit: is it GL9763E or GL*I*9763?
Change the name to GL9763E.
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 3: Driver
Definitions (the driver is the . […]
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
I'd suggest adding these to `src/include/device/pci_ids. […]
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 10: PCIE_GLI_9763E
I think one would expect these definitions to be for GL9763E, given the file they're in. […]
Remove prefix XX_GLI_9763E_XX. Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 10: VHS
What does `VHS` mean? I don't think it's one of these, right? 😄 https://en.wikipedia. […]
Ha 😄, you can call it this if you want. When it is called "VHS", it will work. Therefore, it is called VHS. (VHS should be Vendor Header Space.)
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 11: GLI_9763E_VHS_REV
I'd add a `_MASK` suffix for clarity
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 11: 0xF00
(0xf << 16)
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
I would define these with the value already shifted: […]
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 26: GLI_9763E_PLL_CTL_2_MAX_SSC
I'd add a `_MASK` suffix for clarity
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; : }
Why add these wrappers?
Removed.
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 27: gli_set_gl9763e
I'd prefer a more descriptive name for this function. […]
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 27: pdev
We use `dev` nearly everywhere, even if it's a pointer.
Done
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 33: (u32)
These casts aren't needed
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);
Or, in a single line: […]
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);
Or, in a single line: […]
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);
These could be done with `pci_update_config32` but it would be a very long line
Modified.
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);
Or, in a single line: […]
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);
I see this register is programmed twice with different values. […]
VHS has write protection. Enable writing and disable writing. Thanks for your comments.