Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33993 )
Change subject: mainboard/amd: Add padmelon board code ......................................................................
Patch Set 14:
(32 comments)
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@9 PS3, Line 9: With almost everything in place, it's time to add padmelon board.
The AGESA, PSP and video binaries are not yet merged, and apparently won't be for a while. […]
Ack
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@11 PS3, Line 11: This code was intended for : merlinfalcon version, but as there are some legal issues (documentation) : blocking its merge to coreboot, it'll be released for 00670F00,
Why not simply say this is for the 670F00 version? As I mentioned before, just describe what this p […]
Done
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@15 PS3, Line 15: Even both use fanned versions of SOC, the actual fan control : is done through a SIO, fintek f81803a.
Ok, will do.
Done
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@17 PS3, Line 17:
Ok, will do.
Done
https://review.coreboot.org/c/coreboot/+/33993/2/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/Kconfig:
https://review.coreboot.org/c/coreboot/+/33993/2/src/mainboard/amd/padmelon/... PS2, Line 37: config VGA_BIOS_FILE : string : default "3rdparty/blobs/soc/amd/merlinfalcon/VBIOS.bin"
This was wrong, and already removed on patch set 3.
Ack
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/Kconfig:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 20: # select SOC_AMD_MERLINFALCON # missing binaries
Will do, though will create the config at 33621 and just select it once available.
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: CPPFLAGS_common += -I$(src)/vendorcode/amd/pi/00670F00 : CPPFLAGS_common += -I$(src)/vendorcode/amd/pi/00670F00/Proc/Fch/Common : CPPFLAGS_common += -I$(src)/vendorcode/amd/pi/00670F00/Proc/Common : CPPFLAGS_common += -I$(src)/vendorcode/amd/pi/00670F00/binaryPI
I copy from Marc's code. Might be able to remove it now.
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/bootblock/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 143: 0x00172051, 0x001721C7, 0x00172222, 0x00172310,
Probably... […]
To be solved in the future
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: UAT
Thanks
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: UAT
Thanks
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 37: 0x3fb
Padmelon has com1 and com2, but com1 is what is used for serial debug. […]
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 37: 0x3fb
Padmelon has com1 and com2, but com1 is what is used for serial debug. […]
Done
https://review.coreboot.org/c/coreboot/+/33993/2/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33993/2/src/mainboard/amd/padmelon/... PS2, Line 65: device pnp 4e.4 off end # HWM
enable this device and set io base address and irq here
Done
https://review.coreboot.org/c/coreboot/+/33993/1/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/fan_init.c:
https://review.coreboot.org/c/coreboot/+/33993/1/src/mainboard/amd/padmelon/... PS1, Line 29: []
Will do
Done
https://review.coreboot.org/c/coreboot/+/33993/1/src/mainboard/amd/padmelon/... PS1, Line 36: []
same here
Done
https://review.coreboot.org/c/coreboot/+/33993/1/src/mainboard/amd/padmelon/... PS1, Line 43: []
[FINTEK_SECTIONS_SIZE]
Done
https://review.coreboot.org/c/coreboot/+/33993/1/src/mainboard/amd/padmelon/... PS1, Line 51: []
same here
Done
https://review.coreboot.org/c/coreboot/+/33993/1/src/mainboard/amd/padmelon/... PS1, Line 112: static void init_hwm(void)
Agree, that's why I placed the "todo". Working on it.
Done
https://review.coreboot.org/c/coreboot/+/33993/2/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/fan_init.c:
https://review.coreboot.org/c/coreboot/+/33993/2/src/mainboard/amd/padmelon/... PS2, Line 115: fintek_enable_device(F81803A_HWM, 0x0220, 0xff);
this should be done via the corresponding devicetree entries and not manually
Done
https://review.coreboot.org/c/coreboot/+/33993/2/src/mainboard/amd/padmelon/... PS2, Line 116: /* Open a LPC window to access the hardware monitor */ : reg = pci_read_config32(SOC_LPC_DEV, LPC_IO_PORT_DECODE_ENABLE); : t = reg | CONFIG_HWM_IO_WINDOW; : pci_write_config32(SOC_LPC_DEV, LPC_IO_PORT_DECODE_ENABLE, t); :
No longer needed, found a way to do it using devicetree and PNP initialization.
Ack
https://review.coreboot.org/c/coreboot/+/33993/2/src/mainboard/amd/padmelon/... PS2, Line 131: /* restore register */ : pci_write_config32(SOC_LPC_DEV, LPC_IO_PORT_DECODE_ENABLE, reg); : }
Same here, as it was complement from above.
Ack
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/fan_init.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: celcius
Yes, boundaries are the threshold temperature between 2 sections, each with a different fan speed.
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 30: cpu_boudaries
Thanks
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 106: return
check_status() does the actual check of the statues, and will print a message if status is not 0. […]
Ack
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 115: 0x0220, 0xff
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 131: register
Will do.
No longer part of this patch, moved (and fixed) on f81803a patch.
https://review.coreboot.org/c/coreboot/+/33993/5/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/fan_init.c:
https://review.coreboot.org/c/coreboot/+/33993/5/src/mainboard/amd/padmelon/... PS5, Line 60: static int check_status(const char *name, int status) : { : if (status != HWM_STATUS_SUCCESS) : printk(BIOS_DEBUG, "%s returned %d\n", name, status); : if (status < HWM_STATUS_SUCCESS) : return status; : return HWM_STATUS_SUCCESS; /* positive values are warnings only */ : } : : static void set_fan(uint8_t fan, external_sensor sensor, temp_sensor_type stype, : fan_temp_source temp_source, fan_type ftype, : fan_mode fmode, fan_pwm_freq fan_freq, : fan_rate_up rate_up, fan_rate_down rate_down, : u8 *boundaries, u8 *sections) : { : int s; : printk(BIOS_DEBUG, "Set sensor type\n"); : if (sensor != IGNORE_SENSOR) { : s = set_sensor_type(CONFIG_HWM_PORT, sensor, stype); : if (check_status("set_sensor_type", s) < HWM_STATUS_SUCCESS) : return; : } : : printk(BIOS_DEBUG, "Set temperature source\n"); : s = set_fan_temperature_source(CONFIG_HWM_PORT, fan, temp_source); : if (check_status("set_fan_temperature_source", s) < HWM_STATUS_SUCCESS) : return; : : printk(BIOS_DEBUG, "Set fan type mode\n"); : s = set_fan_type_mode(CONFIG_HWM_PORT, fan, ftype, fmode); : if (check_status("fan_type_mode", s) < HWM_STATUS_SUCCESS) : return; : : printk(BIOS_DEBUG, "Set PWM frequency\n"); : s = set_pwm_frequency(CONFIG_HWM_PORT, fan, fan_freq); : if (check_status("set_pwm_frequency", s) < HWM_STATUS_SUCCESS) : return; : : printk(BIOS_DEBUG, "Set fan speed change rate\n"); : s = set_fan_speed_change_rate(CONFIG_HWM_PORT, fan, rate_up, rate_down); : if (check_status("set_fan_speed_change_rate", s) < HWM_STATUS_SUCCESS) : return; : : printk(BIOS_DEBUG, "Set follow\n"); : s = set_fan_follow(CONFIG_HWM_PORT, fan, FAN_FOLLOW_INTERPOLATION); : if (check_status("set_follow", s) < HWM_STATUS_SUCCESS) : return; : : printk(BIOS_DEBUG, "Set sections\n"); : s = set_sections(CONFIG_HWM_PORT, fan, boundaries, sections); : if (check_status("set_sections", s) < HWM_STATUS_SUCCESS) : return; : : printk(BIOS_DEBUG, "Fan %d completed\n", fan); : }
yes, definitely keep the current api exposed, but move this function there as an additional slightly […]
Ack
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/gpio.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: onst struct soc_amd_gpio gpio_set_stage_reset[] = { : /* NFC PU */ : PAD_GPO(GPIO_64, HIGH), : /* PCIe presence detect */ : PAD_GPO(GPIO_64, HIGH), : /* MUX for Power Express Eval */ : PAD_GPI(GPIO_116, PULL_DOWN), : /* SD power */ : PAD_GPO(GPIO_64, HIGH), : }; : : const struct soc_amd_gpio gpio_set_stage_ram[] = { : /* BT radio disable */ : PAD_GPO(GPIO_14, HIGH), : /* NFC wake */ : PAD_GPO(GPIO_65, HIGH), : /* Webcam */ : PAD_GPO(GPIO_66, HIGH), : /* GPS sleep */ : PAD_GPO(GPIO_70, HIGH), : }
Please update correctly, or note that this patch is work in progress.
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/irq_tables.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 51: /* Align the table to be 16 byte aligned. */ : addr += 15; : addr &= ~15;
Believe so, though I don't remember it out of my mind. Will look.
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 65: PCI_DEVFN(0x14, 4);
There might already be one for it. […]
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 69: 0x1002
Not sure, will check.
Done
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 80: PCI_DEVFN(0x14, 4)
Same as above
Done