[PATCH 1/4] x86: coral: Correct some FSP-M settings

Some settings were modified slightly in the device-tree conversion. Return these to their original values.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/dts/chromebook_coral.dts | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts index aad12f2c4d..5ee056fc95 100644 --- a/arch/x86/dts/chromebook_coral.dts +++ b/arch/x86/dts/chromebook_coral.dts @@ -517,6 +517,11 @@ 20 23 22 21 18 19 16 17 /* DQB[7:15] pins of LPDDR4 module with offset of 16 */ 25 28 30 31 26 27 24 29>; + + fspm,dimm0-spd-address = <0>; + fspm,dimm1-spd-address = <0>; + fspm,skip-cse-rbp = <1>; + fspm,enable-s3-heci2 = <0>; };
&fsp_s {

When comparing hex dumps it is useful to see the offsets of the registers. Add them in where they correspond to a multiple of 16.
Possibly it would be useful to have a a command to output the FSP values in human-readable form, making use of the fsp_bindings implementation.
Signed-off-by: Simon Glass sjg@chromium.org ---
.../include/asm/arch-apollolake/fsp/fsp_m_upd.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/arch-apollolake/fsp/fsp_m_upd.h b/arch/x86/include/asm/arch-apollolake/fsp/fsp_m_upd.h index a77964f30c..e9be66e5b6 100644 --- a/arch/x86/include/asm/arch-apollolake/fsp/fsp_m_upd.h +++ b/arch/x86/include/asm/arch-apollolake/fsp/fsp_m_upd.h @@ -34,7 +34,14 @@ struct __packed fsp_ram_channel { u8 odt_levels; };
+/* struct fsp_m_config - FSP-M configuration + * + * Note that headers precede this and are 64 bytes long. The hex offsets + * mentioned in this file are relative to the start of the header, the same + * convention used in Intel's APL FSP header file. + */ struct __packed fsp_m_config { + /* 40 */ u32 serial_debug_port_address; u8 serial_debug_port_type; u8 serial_debug_port_device; @@ -49,6 +56,7 @@ struct __packed fsp_m_config { u8 profile; u8 memory_down;
+ /* 50 */ u8 ddr3_l_page_size; u8 ddr3_lasr; u8 scrambler_support; @@ -62,6 +70,7 @@ struct __packed fsp_m_config { u16 memory_size_limit; u16 low_memory_max_value;
+ /* 60 */ u16 high_memory_max_value; u8 disable_fast_boot; u8 dimm0_spd_address; @@ -73,6 +82,7 @@ struct __packed fsp_m_config { u32 msg_level_mask; u8 unused_upd_space0[4];
+ /* 110 */ u8 pre_mem_gpio_table_pin_num[4]; u32 pre_mem_gpio_table_ptr; u8 pre_mem_gpio_table_entry_num; @@ -81,8 +91,10 @@ struct __packed fsp_m_config { u8 mrc_data_saving; u32 oem_loading_base;
+ /* 120 */ u8 oem_file_name[16];
+ /* 130 */ void *mrc_boot_data_ptr; u8 e_mmc_trace_len; u8 skip_cse_rbp; @@ -94,20 +106,20 @@ struct __packed fsp_m_config { u8 msc1_wrap; u32 msc0_size;
+ /* 140 */ u32 msc1_size; u8 pti_mode; u8 pti_training; u8 pti_speed; u8 punit_mlvl; - u8 pmc_mlvl; u8 sw_trace_en; u8 periodic_retraining_disable; u8 enable_reset_system; - u8 enable_s3_heci2; u8 unused_upd_space1[3];
+ /* 150 */ void *variable_nvs_buffer_ptr; u8 reserved_fspm_upd[12]; };

Hi Simon,
On Tue, May 26, 2020 at 4:16 AM Simon Glass sjg@chromium.org wrote:
When comparing hex dumps it is useful to see the offsets of the registers. Add them in where they correspond to a multiple of 16.
Possibly it would be useful to have a a command to output the FSP values in human-readable form, making use of the fsp_bindings implementation.
Signed-off-by: Simon Glass sjg@chromium.org
.../include/asm/arch-apollolake/fsp/fsp_m_upd.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/arch-apollolake/fsp/fsp_m_upd.h b/arch/x86/include/asm/arch-apollolake/fsp/fsp_m_upd.h index a77964f30c..e9be66e5b6 100644 --- a/arch/x86/include/asm/arch-apollolake/fsp/fsp_m_upd.h +++ b/arch/x86/include/asm/arch-apollolake/fsp/fsp_m_upd.h @@ -34,7 +34,14 @@ struct __packed fsp_ram_channel { u8 odt_levels; };
+/* struct fsp_m_config - FSP-M configuration
nits: incorrect multi-line comment format
- Note that headers precede this and are 64 bytes long. The hex offsets
- mentioned in this file are relative to the start of the header, the same
- convention used in Intel's APL FSP header file.
- */
struct __packed fsp_m_config {
/* 40 */
So this is a hex number, please make it explicit by adding the 0x prefix to all these numbers otherwise it's quite confusing.
u32 serial_debug_port_address; u8 serial_debug_port_type; u8 serial_debug_port_device;
@@ -49,6 +56,7 @@ struct __packed fsp_m_config { u8 profile; u8 memory_down;
/* 50 */ u8 ddr3_l_page_size; u8 ddr3_lasr; u8 scrambler_support;
@@ -62,6 +70,7 @@ struct __packed fsp_m_config { u16 memory_size_limit; u16 low_memory_max_value;
/* 60 */ u16 high_memory_max_value; u8 disable_fast_boot; u8 dimm0_spd_address;
@@ -73,6 +82,7 @@ struct __packed fsp_m_config { u32 msg_level_mask; u8 unused_upd_space0[4];
/* 110 */ u8 pre_mem_gpio_table_pin_num[4]; u32 pre_mem_gpio_table_ptr; u8 pre_mem_gpio_table_entry_num;
@@ -81,8 +91,10 @@ struct __packed fsp_m_config { u8 mrc_data_saving; u32 oem_loading_base;
/* 120 */ u8 oem_file_name[16];
/* 130 */ void *mrc_boot_data_ptr; u8 e_mmc_trace_len; u8 skip_cse_rbp;
@@ -94,20 +106,20 @@ struct __packed fsp_m_config { u8 msc1_wrap; u32 msc0_size;
/* 140 */ u32 msc1_size; u8 pti_mode; u8 pti_training; u8 pti_speed; u8 punit_mlvl;
u8 pmc_mlvl; u8 sw_trace_en; u8 periodic_retraining_disable; u8 enable_reset_system;
u8 enable_s3_heci2; u8 unused_upd_space1[3];
/* 150 */ void *variable_nvs_buffer_ptr; u8 reserved_fspm_upd[12];
};
Regards, Bin

Some settings were modified slightly in the device-tree conversion. Return these to their original values. This makes WiFi work again.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/dts/chromebook_coral.dts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts index 5ee056fc95..a17a9c2800 100644 --- a/arch/x86/dts/chromebook_coral.dts +++ b/arch/x86/dts/chromebook_coral.dts @@ -600,13 +600,9 @@ fsps,emmc-rx-cmd-data-cntl1 = <0x00181717>; fsps,emmc-rx-cmd-data-cntl2 = <0x10008>;
- /* Enable Audio Clock and Power gating */ - fsps,hd-audio-clk-gate = <1>; - fsps,hd-audio-pwr-gate = <1>; - fsps,bios-cfg-lock-down = <1>; - - /* Enable lpss s0ix */ - fsps,lpss-s0ix-enable = <1>; + /* Enable WiFi */ + fsps,pcie-root-port-en = [01 00 00 00 00 00]; + fsps,pcie-rp-hot-plug = [00 00 00 00 00 00];
fsps,skip-mp-init = <1>; fsps,spi-eiss = <0>;

Hi Simon,
On Tue, May 26, 2020 at 4:16 AM Simon Glass sjg@chromium.org wrote:
Some settings were modified slightly in the device-tree conversion. Return these to their original values. This makes WiFi work again.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/dts/chromebook_coral.dts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts index 5ee056fc95..a17a9c2800 100644 --- a/arch/x86/dts/chromebook_coral.dts +++ b/arch/x86/dts/chromebook_coral.dts @@ -600,13 +600,9 @@ fsps,emmc-rx-cmd-data-cntl1 = <0x00181717>; fsps,emmc-rx-cmd-data-cntl2 = <0x10008>;
/* Enable Audio Clock and Power gating */
fsps,hd-audio-clk-gate = <1>;
fsps,hd-audio-pwr-gate = <1>;
fsps,bios-cfg-lock-down = <1>;
/* Enable lpss s0ix */
fsps,lpss-s0ix-enable = <1>;
/* Enable WiFi */
fsps,pcie-root-port-en = [01 00 00 00 00 00];
fsps,pcie-rp-hot-plug = [00 00 00 00 00 00];
It looks some settings are dropped and some are added. I assume we only need to add something to override the default, no? The HDAudio stuff looks nothing related to WiFi?
fsps,skip-mp-init = <1>; fsps,spi-eiss = <0>;
--
Regards, Bin

Hi Bin,
On Tue, 26 May 2020 at 02:15, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Tue, May 26, 2020 at 4:16 AM Simon Glass sjg@chromium.org wrote:
Some settings were modified slightly in the device-tree conversion. Return these to their original values. This makes WiFi work again.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/dts/chromebook_coral.dts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts index 5ee056fc95..a17a9c2800 100644 --- a/arch/x86/dts/chromebook_coral.dts +++ b/arch/x86/dts/chromebook_coral.dts @@ -600,13 +600,9 @@ fsps,emmc-rx-cmd-data-cntl1 = <0x00181717>; fsps,emmc-rx-cmd-data-cntl2 = <0x10008>;
/* Enable Audio Clock and Power gating */
fsps,hd-audio-clk-gate = <1>;
fsps,hd-audio-pwr-gate = <1>;
fsps,bios-cfg-lock-down = <1>;
/* Enable lpss s0ix */
fsps,lpss-s0ix-enable = <1>;
/* Enable WiFi */
fsps,pcie-root-port-en = [01 00 00 00 00 00];
fsps,pcie-rp-hot-plug = [00 00 00 00 00 00];
It looks some settings are dropped and some are added. I assume we only need to add something to override the default, no? The HDAudio stuff looks nothing related to WiFi?
Yes that's right. It is just intended to get it back the way it was.
I'll update the commit message.
fsps,skip-mp-init = <1>; fsps,spi-eiss = <0>;
--
Regards, Simon

When comparing hex dumps it is useful to see the offsets of the registers. Add them in where they correspond to a multiple of 16.
Possibly it would be useful to have a a command to output the FSP values in human-readable form, making use of the fsp_bindings implementation.
Signed-off-by: Simon Glass sjg@chromium.org ---
.../asm/arch-apollolake/fsp/fsp_s_upd.h | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+)
diff --git a/arch/x86/include/asm/arch-apollolake/fsp/fsp_s_upd.h b/arch/x86/include/asm/arch-apollolake/fsp/fsp_s_upd.h index 87596ffd9d..f9c3985c47 100644 --- a/arch/x86/include/asm/arch-apollolake/fsp/fsp_s_upd.h +++ b/arch/x86/include/asm/arch-apollolake/fsp/fsp_s_upd.h @@ -9,7 +9,14 @@ #ifndef __ASSEMBLY__ #include <asm/fsp2/fsp_api.h>
+/* struct fsp_s_config - FSP-S configuration + * + * Note that struct fsp_upd_header preceeds this and is 32 bytes long. The + * hex offsets mentioned in this file are relative to the start of the header, + * the same convention used in Intel's APL FSP header file. + */ struct __packed fsp_s_config { + /* 20 */ u8 active_processor_cores; u8 disable_core1; u8 disable_core2; @@ -26,6 +33,8 @@ struct __packed fsp_s_config { u8 c_state_auto_demotion; u8 c_state_un_demotion; u8 max_core_c_state; + + /* 30 */ u8 pkg_c_state_demotion; u8 pkg_c_state_un_demotion; u8 turbo_mode; @@ -36,6 +45,8 @@ struct __packed fsp_s_config { u8 ipu_acpi_mode; u8 force_wake; u32 gtt_mm_adr; + + /* 40 */ u32 gm_adr; u8 pavp_lock; u8 graphics_freq_modify; @@ -49,6 +60,8 @@ struct __packed fsp_s_config { u8 power_gating; u8 unit_level_clock_gating; u8 fast_boot; + + /* 50 */ u8 dyn_sr; u8 sa_ipu_enable; u8 pm_support; @@ -56,6 +69,8 @@ struct __packed fsp_s_config { u32 logo_size; u32 logo_ptr; u32 graphics_config_ptr; + + /* 60 */ u8 pavp_enable; u8 pavp_pr3; u8 cd_clock; @@ -78,6 +93,8 @@ struct __packed fsp_s_config { u8 hda_enable; u8 dsp_enable; u8 pme; + + /* 90 */ u8 hd_audio_io_buffer_ownership; u8 hd_audio_io_buffer_voltage; u8 hd_audio_vc_type; @@ -94,6 +111,8 @@ struct __packed fsp_s_config { u8 hmt; u8 hd_audio_pwr_gate; u8 hd_audio_clk_gate; + + /* a0 */ u32 dsp_feature_mask; u32 dsp_pp_module_mask; u8 bios_cfg_lock_down; @@ -104,6 +123,8 @@ struct __packed fsp_s_config { u8 hpet_function_number; u8 io_apic_bdf_valid; u8 io_apic_bus_number; + + /* b0 */ u8 io_apic_device_number; u8 io_apic_function_number; u8 io_apic_entry24_119; @@ -124,6 +145,8 @@ struct __packed fsp_s_config { u8 i2c2_enable; u8 i2c3_enable; u8 i2c4_enable; + + /* d0 */ u8 i2c5_enable; u8 i2c6_enable; u8 i2c7_enable; @@ -137,6 +160,8 @@ struct __packed fsp_s_config { u8 os_dbg_enable; u8 dci_en; u32 uart2_kernel_debug_base_address; + + /* e0 */ u8 pcie_clock_gating_disabled; u8 pcie_root_port8xh_decode; u8 pcie8xh_decode_port_index; @@ -150,6 +175,8 @@ struct __packed fsp_s_config { u8 pcie_rp_pm_sci[6]; u8 pcie_rp_ext_sync[6]; u8 pcie_rp_transmitter_half_swing[6]; + + /* 110 */ u8 pcie_rp_acs_enabled[6]; u8 pcie_rp_clk_req_supported[6]; u8 pcie_rp_clk_req_number[6]; @@ -158,6 +185,8 @@ struct __packed fsp_s_config { u8 pme_interrupt[6]; u8 unsupported_request_report[6]; u8 fatal_error_report[6]; + + /* 140 */ u8 no_fatal_error_report[6]; u8 correctable_error_report[6]; u8 system_error_on_fatal_error[6]; @@ -166,6 +195,8 @@ struct __packed fsp_s_config { u8 pcie_rp_speed[6]; u8 physical_slot_number[6]; u8 pcie_rp_completion_timeout[6]; + + /* 170 */ u8 ptm_enable[6]; u8 pcie_rp_aspm[6]; u8 pcie_rp_l1_substates[6]; @@ -173,6 +204,8 @@ struct __packed fsp_s_config { u8 pcie_rp_ltr_config_lock[6]; u8 pme_b0_s5_dis; u8 pci_clock_run; + + /* 190 */ u8 timer8254_clk_setting; u8 enable_sata; u8 sata_mode; @@ -185,6 +218,8 @@ struct __packed fsp_s_config { u8 sata_ports_dev_slp[2]; u8 sata_ports_hot_plug[2]; u8 sata_ports_interlock_sw[2]; + + /* 1a0 */ u8 sata_ports_external[2]; u8 sata_ports_spin_up[2]; u8 sata_ports_solid_state_drive[2]; @@ -192,6 +227,8 @@ struct __packed fsp_s_config { u8 sata_ports_dm_val[2]; u8 unused_upd_space3[2]; u16 sata_ports_dito_val[2]; + + /* 1b0 */ u16 sub_system_vendor_id; u16 sub_system_id; u8 crid_settings; @@ -206,6 +243,8 @@ struct __packed fsp_s_config { u8 sirq_mode; u8 start_frame_pulse; u8 smbus_enable; + + /* 1c0 */ u8 arp_enable; u8 unused_upd_space4; u16 num_rsvd_smbus_addresses; @@ -215,10 +254,14 @@ struct __packed fsp_s_config { u8 usb30_mode; u8 unused_upd_space5[1]; u8 port_usb20_enable[8]; + + /* 250 */ u8 port_us20b_over_current_pin[8]; u8 usb_otg; u8 hsic_support_enable; u8 port_usb30_enable[6]; + + /* 260 */ u8 port_us30b_over_current_pin[6]; u8 ssic_port_enable[2]; u16 dlane_pwr_gating; @@ -227,9 +270,13 @@ struct __packed fsp_s_config { u16 reset_wait_timer; u8 rtc_lock; u8 sata_test_mode; + + /* 270 */ u8 ssic_rate[2]; u16 dynamic_power_gating; u16 pcie_rp_ltr_max_snoop_latency[6]; + + /* 280 */ u8 pcie_rp_snoop_latency_override_mode[6]; u8 unused_upd_space6[2]; u16 pcie_rp_snoop_latency_override_value[6]; @@ -240,45 +287,69 @@ struct __packed fsp_s_config { u8 pcie_rp_non_snoop_latency_override_mode[6]; u8 tco_timer_halt_lock; u8 pwr_btn_override_period; + + /* 2b0 */ u16 pcie_rp_non_snoop_latency_override_value[6]; u8 pcie_rp_non_snoop_latency_override_multiplier[6]; u8 pcie_rp_slot_power_limit_scale[6]; u8 pcie_rp_slot_power_limit_value[6]; u8 disable_native_power_button; u8 power_butter_debounce_mode; + + /* 2d0 */ u32 sdio_tx_cmd_cntl; u32 sdio_tx_data_cntl1; u32 sdio_tx_data_cntl2; u32 sdio_rx_cmd_data_cntl1; + + /* 2e0 */ u32 sdio_rx_cmd_data_cntl2; u32 sdcard_tx_cmd_cntl; u32 sdcard_tx_data_cntl1; u32 sdcard_tx_data_cntl2; + + /* 2f0 */ u32 sdcard_rx_cmd_data_cntl1; u32 sdcard_rx_strobe_cntl; u32 sdcard_rx_cmd_data_cntl2; u32 emmc_tx_cmd_cntl; + + /* 300 */ u32 emmc_tx_data_cntl1; u32 emmc_tx_data_cntl2; u32 emmc_rx_cmd_data_cntl1; u32 emmc_rx_strobe_cntl; + + /* 310 */ u32 emmc_rx_cmd_data_cntl2; u32 emmc_master_sw_cntl; u8 pcie_rp_selectable_deemphasis[6]; u8 monitor_mwait_enable; u8 hd_audio_dsp_uaa_compliance; + + /* 320 */ u32 ipc[4]; + + /* 330 */ u8 sata_ports_disable_dynamic_pg[2]; u8 init_s3_cpu; u8 skip_punit_init; u8 unused_upd_space7[4]; u8 port_usb20_per_port_tx_pe_half[8]; + + /* 340 */ u8 port_usb20_per_port_pe_txi_set[8]; u8 port_usb20_per_port_txi_set[8]; + + /* 350 */ u8 port_usb20_hs_skew_sel[8]; u8 port_usb20_i_usb_tx_emphasis_en[8]; + + /* 360 */ u8 port_usb20_per_port_rxi_set[8]; u8 port_usb20_hs_npre_drv_sel[8]; + + /* 370 */ u8 reserved_fsps_upd[16]; };

Hi Simon,
On Tue, May 26, 2020 at 4:16 AM Simon Glass sjg@chromium.org wrote:
When comparing hex dumps it is useful to see the offsets of the registers. Add them in where they correspond to a multiple of 16.
Possibly it would be useful to have a a command to output the FSP values in human-readable form, making use of the fsp_bindings implementation.
Signed-off-by: Simon Glass sjg@chromium.org
.../asm/arch-apollolake/fsp/fsp_s_upd.h | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+)
diff --git a/arch/x86/include/asm/arch-apollolake/fsp/fsp_s_upd.h b/arch/x86/include/asm/arch-apollolake/fsp/fsp_s_upd.h index 87596ffd9d..f9c3985c47 100644 --- a/arch/x86/include/asm/arch-apollolake/fsp/fsp_s_upd.h +++ b/arch/x86/include/asm/arch-apollolake/fsp/fsp_s_upd.h @@ -9,7 +9,14 @@ #ifndef __ASSEMBLY__ #include <asm/fsp2/fsp_api.h>
+/* struct fsp_s_config - FSP-S configuration
nits: incorrect multi-line comment format
- Note that struct fsp_upd_header preceeds this and is 32 bytes long. The
- hex offsets mentioned in this file are relative to the start of the header,
- the same convention used in Intel's APL FSP header file.
- */
struct __packed fsp_s_config {
/* 20 */
Please add 0x prefix to these numbers ...
u8 active_processor_cores; u8 disable_core1; u8 disable_core2;
@@ -26,6 +33,8 @@ struct __packed fsp_s_config { u8 c_state_auto_demotion; u8 c_state_un_demotion; u8 max_core_c_state;
[snip]
Regards, Bin

On Tue, May 26, 2020 at 4:16 AM Simon Glass sjg@chromium.org wrote:
Some settings were modified slightly in the device-tree conversion. Return these to their original values.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/dts/chromebook_coral.dts | 5 +++++ 1 file changed, 5 insertions(+)
Acked-by: Bin Meng bmeng.cn@gmail.com
participants (2)
-
Bin Meng
-
Simon Glass