Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43751 )
Change subject: drivers/generic/genesyslogic: Add driver for Genesys Loigc GL9763E ......................................................................
Patch Set 1: Code-Review+1
(21 comments)
Welcome!
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 I don't think the GL9763E is a "generic" device. I'd put it inside `drivers/genesyslogic/` instead.
https://review.coreboot.org/c/coreboot/+/43751/1//COMMIT_MSG@7 PS1, Line 7: GL9763E Could you please mention in the commit message which kind of device the GL9763E is? I couldn't find a datasheet anywhere, only some Linux patches that hint at it being a PCIe to eMMC controller: https://lkml.org/lkml/2020/5/8/160
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... File src/drivers/generic/genesyslogic/Kconfig:
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 1: config DRIVERS_GENERIC_GLI9763E Note that, since nothing currently selects this option, the code hasn't been build-tested by Jenkins. If this driver will be used by some board (I guess a chromebook?), it would be good to make a follow-up change (a new commit right after this one) that selects this option.
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?
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 3: Driver Definitions (the driver is the .c file)
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.h`
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.org/wiki/VHS
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. The repeated prefix only makes the definitions long and easy to mix up đ
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 11: 0xF00 (0xf << 16)
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
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:
#define GLI_9763E_VHS_REV_R (0x0 << 16) #define GLI_9763E_VHS_REV_M (0x1 << 16) #define GLI_9763E_VHS_REV_W (0x2 << 16)
Also, please add an extra space after `#define` to make it clear that these are fields within a register
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
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?
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. `gl9763e_init`, for instance?
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.
https://review.coreboot.org/c/coreboot/+/43751/1/src/drivers/generic/genesys... PS1, Line 33: (u32) These casts aren't needed
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:
pci_or_config32(dev, PCIE_GLI_9763E_SCR, GLI_9763E_SCR_AXI_REQ);
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:
pci_and_config32(dev, PCIE_GLI_9763E_CFG_REG_2, ~GLI_9763E_CFG_REG_2_L0S);
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
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:
pci_or_config32(dev, PCIE_GLI_9763E_PLL_CTL, GLI_9763E_PLL_CTL_SSC);
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. It would be good to know why this is needed.