[alsa-devel] [RFC] ASOC: Intel: Skylake: Broken HDaudio controller/link initialization sequences

[Takashi Iwai]

[ Corrected the ML address ]

On Wed, 09 Jan 2019 00:28:30 +0100,
Pierre-Louis Bossart wrote:
> 
> Hi,
> 
> this is not a patch but a request for comments/feedback on the HDAudio
> controller/link initialization sequences. After the v4.20 merge window
> snafu w/ the HDaudio detection, I spent quite a bit of time to figure
> out why all our HDaudio development devices worked well with the
> Skylake driver, and why Linus' laptop and others didn't due to an HDMI
> initialization issue. Hans de Goede was kind enough to give me SSH
> remote access to his device and while I don't have a formal fix for
> now I found a couple of issues that really need to be fixed so that
> the Skylake driver works by design and not by accident.
> 
> See below the programming sequences for the legacy and Intel/Skylake
> drivers. There are obvious discrepancies or broken sequences such as
> :-
> 
> 1. the i915 component and display power being set in a work
> queue. Forcing the init to be done upfront fixes the HDMI detection
> issue on Hans' laptop with the correct codec_mask detected.

This is hairy, and likely some timing issue...

> 2. a call to snd_hdac_bus_reset_link() that doesn't seem to be useful
> (done twice)

Better to avoid the unneeded call of this, yeah.

> 3. an initialization of ML (multi-link) registers *before* reading ML
> capabilities on Skylake (only meaningful in the second init) and not
> done in the legacy.
> 
> 4. Missing setups for multi-link (done in legacy, not done by Skylake
> drivers)

I have little idea about the ML stuff, sorry.

> 5. set_codec_wakeup() only initialized in legacy

This might be the cause of the missing HDMI codec.

The wakeup handling was added years ago for assuring that the i915
power-well turned on during probe and resume.  Looking at the git log,
the initial commit was 0a6735215350
    ALSA: hda - reset display codec when power on

> 6. init/stop/init pattern in Skylake driver. I can't think of any
> reason why this is required both in the probe and the probe workqueue.

Maybe some workaround for messy timing issue?  No much idea, either.


thanks,

Takashi


> I don't have any background for most of this, I only took over support
> for HDaudio codecs after the initial contributor moved on, so sharing
> all my findings in the hope that others have memories of these
> different points. I'll get the same laptop as Linus later this week
> and will be able to work more on this, but in the meantime comments
> are welcome.
> 
> Thanks
> 
> -Pierre
> 
> azx_create
> -- azx_bus_init
> ---- snd_hdac_bus_init
> -- azx_probe_work
> ---- azx_probe_continue
> ------ snd_hdac_i915_init(bus)
> ------ snd_hdac_display_power(bus, true);
> ------ azx_first_init(chip);
> -------- snd_hdac_bus_parse_capabilities(bus)
> -------- azx_init_streams
> ---------- snd_hdac_stream_init
> -------- azx_alloc_stream_pages
> -------- azx_init_pci(chip)
> ---------- update_pci_byte(chip->pci, AZX_PCIREG_TCSEL, 0x07, 0);
> -------- hda_intel_init_chip
> ---------- snd_hdac_set_codec_wakeup(bus, true); !!! <<< is this needed?
> ---------- pci_write_config_dword(pci, INTEL_HDA_CGCTL, val);
> ---------- azx_init_chip(chip, full_reset)
> ------------ snd_hdac_bus_init_chip(chip, full_reset)
> -------------- snd_hdac_bus_reset_link(bus, full_reset);
> -------------- azx_int_clear
> -------------- snd_hdac_bus_init_cmd_io
> -------------- azx_int_enable
> <<<< codec_mask 0x5 is correct
> ---------- pci_write_config_dword(pci, INTEL_HDA_CGCTL, val)
> ---------- snd_hdac_set_codec_wakeup(bus, false); !!! <<< is this needed?
> ---------- bxt_reduce_dma_latency !!! <<< is this needed?
> ---------- intel_init_lctl !!! <<< is this needed?
> ------------ intel_ml_lctl_set_power(chip, 0)
> ------------ intel_get_lctl_scf
> ------------ intel_get_lctl_scf(chip, 1)
> ------ azx_probe_codecs
> ------ azx_codec_configure
> ------ snd_hdac_display_power(bus, false);
> 
> 
> skl_probe
> -- skl_create
> ---- snd_hdac_ext_bus_init
> ------ snd_hdac_bus_init
> -- skl_first_init
> 
> <<< this sequence makes no sense. the i915/display is not initialized
> before reading the codec_masks.  enabling the usual sequence of
> snd_hdac_i915_init
> and snd_hdac_display_power gives the right codec_mask
> 
> ---- snd_hdac_bus_reset_link !!! <<< is this needed, it's done later
> as part of skl_init_chip?
> ------ snd_hdac_chip_writew(bus, STATESTS, STATESTS_INT_MASK);
> ------ snd_hdac_bus_enter_link_reset
> ------ usleep_range(500, 1000);
> ------ snd_hdac_bus_exit_link_reset
> ------ usleep_range(1000, 1200);
> ------ snd_hdac_chip_updatel(bus, GCTL, 0, AZX_GCTL_UNSOL);
> ------ snd_hdac_chip_readw(bus, STATESTS);
> <<<< codec_mask 0x1 is wrong, HDMI not detected
> ---- snd_hdac_bus_parse_capabilities
> ---- snd_hdac_ext_stream_init_all
> ---- skl_init_pci
> ------ skl_update_pci_byte(skl->pci, AZX_PCIREG_TCSEL, 0x07, 0);
> ---- skl_init_chip
> ------ skl_enable_miscbdcge(bus->dev, false);
> ------ snd_hdac_bus_init_chip(bus, full_reset);
> -------- snd_hdac_bus_reset_link(bus, full_reset);
> -------- azx_int_clear
> -------- snd_hdac_bus_init_cmd_io
> -------- azx_int_enable
> <<< next line makes no sense, it's done before the ML capabilities are read
> ------ bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);
> !!! <<< is this needed?
> ------ skl_enable_miscbdcge(bus->dev, true);
> <<< codec_mask 0x1 is wrong
> -- skl_nhlt_init
> -- skl_init_dsp
> ---- snd_hdac_ext_bus_ppcap_enable(bus, true);
> ---- snd_hdac_ext_bus_ppcap_int_enable(bus, true);
> -- snd_hdac_ext_bus_get_ml_capabilities
> -- snd_hdac_bus_stop_chip
> ---- azx_int_disable(bus);
> ---- azx_int_clear(bus);
> ---- snd_hdac_bus_stop_cmd_io(bus);
> -- skl_probe_work
> ---- skl_i915_init <<< this needs to be done earlier
> ------ snd_hdac_i915_init
> ------ snd_hdac_display_power
> ---- skl_init_chip <<< why do we need to re-initialize again?
> ------ skl_enable_miscbdcge(bus->dev, false);
> ------ snd_hdac_bus_init_chip(bus, full_reset);
> -------- snd_hdac_bus_reset_link(bus, full_reset);
> -------- azx_int_clear
> -------- snd_hdac_bus_init_cmd_io
> -------- azx_int_enable
> ------ bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);
> ------ skl_enable_miscbdcge(bus->dev, true);
> ---- skl_codec_create
> ------- snd_hdac_bus_stop_chip
> ------- skl_init_chip (on error)
> ---- skl_platform_register
> ---- skl_machine_device_register
> ---- snd_hdac_ext_bus_link_put
> ---- snd_hdac_display_power(bus, false);
>