SKL/KBL support - basic code #1667

plbossart · 2020-01-08T01:41:56Z

clean-up of @lgirdwood topic/kbl-fw-loader branch (w/o the WIP parts)

SKL/KBL was re-added to default kconfig so this should compile and have no adverse effects on existing platforms.

plbossart · 2020-01-08T01:43:21Z

@ranj063 can you check the last patch w/ the boot_complete stuff (different now that we have different states)

ranj063 · 2020-01-08T01:53:15Z

@ranj063 can you check the last patch w/ the boot_complete stuff (different now that we have different states)

@plbossart it looks fine. The state machine for FW boot is all handled in the common loader and PM callbacks. The platform-specific loader code does not touch the state machine.

ranj063 · 2020-01-08T02:04:44Z

sound/soc/sof/intel/hda-loader-skl.c

+	cl_cleanup_skl(sdev);
+
+	dev_err(sdev->dev, "error: load fw failed err: %d\n", ret);
+	return ret;


@plbossart the hda_dsp_cl_boot_firmware_skl() needs to be aligned with hda_dsp_cl_boot_firmware() to fix the retries and error handling properly.

ROM codes for SKL & KBL are different from other platforms. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add operations for SKL and KBL platforms. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add ops and PCI IDs Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add build support for SKL and KBL based PCI devices. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-01-08T02:59:16Z

@lgirdwood I tested the updated version on a KBL NUC, this goes all the way to the firmware load. I guess your version was very very old so hopefully this helps rebase with a more recent tree.

I only tested with the SST firmware since I don't have a firmware compiled/signed on my side.

root@plb-kblnuc7:~# ls -l /lib/firmware/intel/sof/sof-kbl.ri 
lrwxrwxrwx 1 root root 23 Jan  7 20:44 /lib/firmware/intel/sof/sof-kbl.ri -> ../dsp_fw_kbl_v3402.bin
ro

dmesg.log

sof-dev-defconfig.txt

lyakh · 2020-01-08T08:06:46Z

sound/soc/sof/intel/hda-loader-skl.c

+			 u32 total_size, u32 bufsize)
+{
+	unsigned int bytes_left = total_size;
+	const u8 *curr_pos = (u8 *)bin;


this cast isn't correct - it drops const even though the goal variable is const too, and I don't think it's needed at all.

lyakh · 2020-01-08T08:08:35Z

sound/soc/sof/intel/hda-loader-skl.c

+
+static int
+cl_skl_cldma_copy_to_buf(struct snd_sof_dev *sdev, const void *bin,
+			 u32 total_size, u32 bufsize)


wouldn't it be better to split it like

static int cl_skl_cldma_copy_to_buf(struct snd_sof_dev *sdev, const void *bin, u32 total_size, u32 bufsize)

lyakh · 2020-01-08T08:45:13Z

sound/soc/sof/intel/hda-loader-skl.c

+		return -EINVAL;
+
+	while (bytes_left > 0) {
+


nitpick: I don't think anyone of us is still using 25x80 terminals, but still empty lines like the above - after while () { or if () { etc. - seem like a waste of screen real estate to me, and there are 3 of them just in this snippet...

lyakh · 2020-01-08T08:47:48Z

sound/soc/sof/intel/hda-loader-skl.c

+
+	while (bytes_left > 0) {
+
+		if (bytes_left > bufsize) {


this could be reverted like

if (bytes_left <= bufsize) { ... return 0; }

to avoid the additional indentation level. caused by the else

lyakh · 2020-01-08T08:48:15Z

sound/soc/sof/intel/hda-loader-skl.c

+	const struct firmware *fw =  plat_data->fw;
+	unsigned int bufsize = HDA_SKL_CLDMA_MAX_BUFFER_SIZE;
+	unsigned int status;
+	int ret = 0;


superfluous initialisation

lyakh · 2020-01-08T08:49:09Z

sound/soc/sof/intel/hda-loader-skl.c

+	ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_BAR,
+					HDA_ADSP_FW_STATUS_SKL, status,
+					((status & SKL_DSP_ROM_SKL_STS_MASK)
+						== SKL_DSP_ROM_BASEFW_ENTERED),


superfluous external parentheses

lgirdwood

@plbossart good progress, can you confirm if closed source FW authenticates ?
I could get FW loading (but failing to validate) on old RVP (with early silicon) but production HW uses IRQ mode for FW loading, this flow needs some extra logic to be added to a couple of the codeloader functions and can be based on SKL flow.
Interestingly the upstream SKL driver would never load FW on my KBL NUC, not even with some hacks. I guess you will need a working SKL flow to base any updates on.

lgirdwood · 2020-01-08T09:09:55Z

sound/soc/sof/intel/hda-loader-skl.c

@@ -491,7 +491,6 @@ int hda_dsp_cl_boot_firmware_skl(struct snd_sof_dev *sdev)

 	/* init for booting wait */
 	init_waitqueue_head(&sdev->boot_wait);
-	sdev->boot_complete = false;


I guess this will be squashed into the previous code loader patch.

plbossart · 2020-01-08T15:29:18Z

can you confirm if closed source FW authenticates ?

what information can I look for? I attached the dmesg log, not sure what you would want to look for. Stepping back, do we agree that using the existing SST firmware should be downloaded but will fail when the first IPCs come, right? I am not sure when the two solutions will start to differ and at what points errors would be normal/expected.

I could get FW loading (but failing to validate) on old RVP (with early silicon) but production HW uses IRQ mode for FW loading, this flow needs some extra logic to be added to a couple of the codeloader functions and can be based on SKL flow. Interestingly the upstream SKL driver would never load FW on my KBL NUC, not even with some hacks. I guess you will need a working SKL flow to base any updates on.

ok, I will retry with the SKL driver. I am pretty sure the same tree will work, I tested it back in the day. That will indeed ensure we are starting from a know valid point.

/* init for booting wait */ init_waitqueue_head(&sdev->boot_wait); - sdev->boot_complete = false; I guess this will be squashed into the previous code loader patch.

Yes, but I wanted Ranjani to ack that part. I wasn't sure if it was needed or not.

lgirdwood · 2020-01-08T16:01:37Z

can you confirm if closed source FW authenticates ?
what information can I look for? I attached the dmesg log, not sure what you would want to look for.

Sorry, didn't see dmesg. ROM code 3 is SKL_DSP_ROM_MANIFEST_LOADED so it looks like it's only partially processing the data.

Stepping back, do we agree that using the existing SST firmware should be downloaded but will fail when the first IPCs come, right?

Yes, I would expect it to send a FW ready and fail at FW boot timeout.

Comment out widgets/routes that are not defined in the SST topology [ 27.471575] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no source widget found for codec1_out [ 27.471577] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route codec1_out -> direct -> Digital CPU Playback [ 27.471580] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no source widget found for codec2_out [ 27.471581] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route codec2_out -> direct -> Alt Analog CPU Playback [ 27.471585] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for codec1_in [ 27.471586] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route Digital CPU Capture -> direct -> codec1_in [ 27.471588] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for codec2_in [ 27.471589] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route Alt Analog CPU Capture -> direct -> codec2_in Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-01-08T23:44:24Z

@lgirdwood Instructions to test SST driver on KBL NUC

pull updated branch (topology issues with Morimoto patches)
compile with the configuration
sst-dev-defconfig.txt
copy the topology to the target
fwdt_kbl_hda_dsp.bin.gz

scp fwdt_kbl_hda_dsp.bin root@192.168.1.138:/lib/firmware/dfw_sst.bin

The name of the topology does matter

configure the required mixers (muted by default)

root@plb-kblnuc7:~# dmesg -C
root@plb-kblnuc7:~# modprobe snd-soc-skl
root@plb-kblnuc7:~# aplay -l
**** List of PLAYBACK Hardware Devices ****
card 0: hdadsp [hda-dsp], device 7: CNL Audio (*) []
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 0: hdadsp [hda-dsp], device 8: CNL HDMI1 (*) []
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 0: hdadsp [hda-dsp], device 9: CNL HDMI2 (*) []
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 0: hdadsp [hda-dsp], device 10: CNL HDMI3 (*) []
  Subdevices: 1/1
  Subdevice #0: subdevice #0
root@plb-kblnuc7:~# amixer controls
numid=15,iface=CARD,name='Front Headphone Jack'
numid=16,iface=CARD,name='HDMI/DP, pcm=15 Jack'
numid=17,iface=CARD,name='HDMI/DP, pcm=16 Jack'
numid=18,iface=CARD,name='HDMI/DP, pcm=17 Jack'
numid=13,iface=MIXER,name='Master Playback Switch'
numid=12,iface=MIXER,name='Master Playback Volume'
numid=14,iface=MIXER,name='Loopback Mixing'
numid=3,iface=MIXER,name='Pin5-Port0 Mux'
numid=4,iface=MIXER,name='Pin5-Port1 Mux'
numid=5,iface=MIXER,name='Pin5-Port2 Mux'
numid=6,iface=MIXER,name='Pin6-Port0 Mux'
numid=7,iface=MIXER,name='Pin6-Port1 Mux'
numid=8,iface=MIXER,name='Pin6-Port2 Mux'
numid=9,iface=MIXER,name='Pin7-Port0 Mux'
numid=10,iface=MIXER,name='Pin7-Port1 Mux'
numid=11,iface=MIXER,name='Pin7-Port2 Mux'
numid=1,iface=MIXER,name='codec0_out mo media2_in mi Switch'
numid=19,iface=MIXER,name='hif5-0 Jack Switch'
numid=20,iface=MIXER,name='hif5-1 Jack Switch'
numid=21,iface=MIXER,name='hif5-2 Jack Switch'
numid=22,iface=MIXER,name='hif6-0 Jack Switch'
numid=23,iface=MIXER,name='hif6-1 Jack Switch'
numid=24,iface=MIXER,name='hif6-2 Jack Switch'
numid=25,iface=MIXER,name='hif7-0 Jack Switch'
numid=26,iface=MIXER,name='hif7-1 Jack Switch'
numid=27,iface=MIXER,name='hif7-2 Jack Switch'
numid=2,iface=MIXER,name='media0_out mo codec0_in mi Switch'
root@plb-kblnuc7:~# amixer -Dhw:0 cset numid=1 on
numid=1,iface=MIXER,name='codec0_out mo media2_in mi Switch'
  ; type=BOOLEAN,access=rw------,values=1
  : values=on
root@plb-kblnuc7:~# amixer -Dhw:0 cset numid=2 on
numid=2,iface=MIXER,name='media0_out mo codec0_in mi Switch'
  ; type=BOOLEAN,access=rw------,values=1
  : values=on
root@plb-kblnuc7:~# speaker-test -Dhw:0,7 -c2 -r48000 -t sine

speaker-test 1.1.9

Playback device is hw:0,7
Stream parameters are 48000Hz, S16_LE, 2 channels
Sine wave rate is 440.0000Hz
Rate set to 48000Hz (requested 48000Hz)
Buffer size range from 64 to 16384
Period size range from 32 to 8192
Using max buffer size 16384
Periods = 4
was set period_size = 4096
was set buffer_size = 16384
 0 - Front Left
 1 - Front Right

see resulting dmesg (still some bugs obviously so time to ditch this driver...)

dmesg-sst.log

Add traces to show interrupts are taking place. No luck so something is obviously missing. [ 47.767695] sof-audio-pci 0000:00:1f.3: ROM init successful [ 47.767696] sof-audio-pci 0000:00:1f.3: firmware size: 0x3c548 buffer size 0x20000 [ 47.767697] sof-audio-pci 0000:00:1f.3: cldma copy 0x20000 bytes [ 47.767702] sof-audio-pci 0000:00:1f.3: plb: cl_skl_cldma_set_intr interrupt enable 1 [ 47.767709] sof-audio-pci 0000:00:1f.3: cl_skl_cldma_copy_to_buf: plb: sleeping 100ms [ 47.874371] sof-audio-pci 0000:00:1f.3: cldma copy 0x1c548 bytes [ 47.874376] sof-audio-pci 0000:00:1f.3: plb: cl_skl_cldma_set_intr interrupt enable 0 [ 50.860527] sof-audio-pci 0000:00:1f.3: FW Poll Status: reg=0x3 timedout [ 50.860532] sof-audio-pci 0000:00:1f.3: error: firmware loading failed! [ 50.860560] sof-audio-pci 0000:00:1f.3: error: load firmware failed : -110 [ 50.860567] sof-audio-pci 0000:00:1f.3: unknown ROM status value 00000003 [ 50.860572] sof-audio-pci 0000:00:1f.3: error: status = 0x00000000 panic = 0x00000000 [ 50.860576] sof-audio-pci 0000:00:1f.3: unknown ROM status value 00000003 [ 50.860583] sof-audio-pci 0000:00:1f.3: FW Poll Status: reg=0x1010303 successful [ 50.860589] sof-audio-pci 0000:00:1f.3: FW Poll Status: reg=0x303 successful [ 50.860593] sof-audio-pci 0000:00:1f.3: DSP core(s) enabled? 0 : core_mask 1 [ 50.860606] sof-audio-pci 0000:00:1f.3: error: load fw failed err: -110 [ 50.860610] sof-audio-pci 0000:00:1f.3: error: failed to reset DSP [ 50.860614] sof-audio-pci 0000:00:1f.3: error: failed to boot DSP firmware -110 [ 50.860618] sof-audio-pci 0000:00:1f.3: error: sof_probe_work failed err: -110 root@plb-kblnuc7 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-01-10T01:33:35Z

@lgirdwood @keyonjie I checked the code and compared with the SST flows, the main difference seems to be in how the code loader DMA is actually handled. There was no interrupt handler and the buffer copy doesn't seem to wait that the data is actually consumed by the DSP, so not really a surprise it's seen as corrupted. See last patch and the results showing the dmesg excerpts.

I really wonder if this was ever tested... What is the expectation for the code loader in terms of data handling?

plbossart · 2020-02-20T22:54:54Z

closing for now, someone will have to take over

plbossart requested review from bardliao, lgirdwood, libinyang and lyakh as code owners January 8, 2020 01:41

ranj063 reviewed Jan 8, 2020

View reviewed changes

lrgirdwo and others added 5 commits January 7, 2020 20:51

ASoC: SOF: Intel: Add ROM codes for SKL/KBL

5af5ef5

ROM codes for SKL & KBL are different from other platforms. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: Add SKL/KBL operations.

1981e90

Add operations for SKL and KBL platforms. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: pci: add SKL and KBL support

cfe03f0

Add ops and PCI IDs Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: Add build support for SKL/KBL PCI platforms.

97be642

Add build support for SKL and KBL based PCI devices. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: fix SKL/KBL compilation issues

e83a963

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart force-pushed the topic/skl-kbl branch from 837c5bf to e83a963 Compare January 8, 2020 02:52

lyakh suggested changes Jan 8, 2020

View reviewed changes

lgirdwood reviewed Jan 8, 2020

View reviewed changes

plbossart closed this Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SKL/KBL support - basic code #1667

SKL/KBL support - basic code #1667

plbossart commented Jan 8, 2020

plbossart commented Jan 8, 2020

ranj063 commented Jan 8, 2020

ranj063 Jan 8, 2020

plbossart commented Jan 8, 2020

lyakh Jan 8, 2020

lyakh Jan 8, 2020

lyakh Jan 8, 2020

lyakh Jan 8, 2020

lyakh Jan 8, 2020

lyakh Jan 8, 2020

lgirdwood left a comment

lgirdwood Jan 8, 2020

plbossart commented Jan 8, 2020 via email

lgirdwood commented Jan 8, 2020

plbossart commented Jan 8, 2020 •

edited

plbossart commented Jan 10, 2020 •

edited

plbossart commented Feb 20, 2020

SKL/KBL support - basic code #1667

SKL/KBL support - basic code #1667

Conversation

plbossart commented Jan 8, 2020

plbossart commented Jan 8, 2020

ranj063 commented Jan 8, 2020

Choose a reason for hiding this comment

plbossart commented Jan 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart commented Jan 8, 2020 via email

lgirdwood commented Jan 8, 2020

plbossart commented Jan 8, 2020 • edited

plbossart commented Jan 10, 2020 • edited

plbossart commented Feb 20, 2020

plbossart commented Jan 8, 2020 •

edited

plbossart commented Jan 10, 2020 •

edited