Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: C3bv6z/E+an3RS0R4agyic4qg/Q27EXuerRCUAkfIeN0ZnGBgEvs/Y9I8lmLT4XUmibw83GFCN
 ilqJoPW1OGXA==
IronPort-SDR: 5wf0i0PMojENL5HhlZkscnuDRzLVNrKMmryjUbywGuvdnAkjBKzT5bT/rXOCEZns6IxKfgvHhh
 z96zAKfI9WjA==
Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt
 handlers/threads
To:     Vinod Koul <vkoul@kernel.org>,
        Bard Liao <yung-chuan.liao@linux.intel.com>
Cc:     alsa-devel@alsa-project.org, tiwai@suse.de,
        gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
        ranjani.sridharan@linux.intel.com, hui.wang@canonical.com,
        broonie@kernel.org, srinivas.kandagatla@linaro.org,
        jank@cadence.com, mengdong.lin@intel.com,
        slawomir.blauciak@intel.com, sanyog.r.kale@intel.com,
        rander.wang@linux.intel.com, bard.liao@intel.com
References: <20200623173546.21870-1-yung-chuan.liao@linux.intel.com>
 <20200623173546.21870-8-yung-chuan.liao@linux.intel.com>
 <20200630162448.GS2599@vkoul-mobl>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Message-ID: <55fbc41e-cb41-8bdf-bdbd-1d1b76938683@linux.intel.com>
Date:   Tue, 30 Jun 2020 11:46:59 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <20200630162448.GS2599@vkoul-mobl>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


>> +irqreturn_t sdw_intel_thread(int irq, void *dev_id)
>> +{
>> +	struct sdw_intel_ctx *ctx = dev_id;
>> +	struct sdw_intel_link_res *link;
>> +
>> +	list_for_each_entry(link, &ctx->link_list, list)
>> +		sdw_cdns_irq(irq, link->cdns);
>> +
>> +	sdw_intel_enable_irq(ctx->mmio_base, true);
>> +	return IRQ_HANDLED;
>> +}
>> +EXPORT_SYMBOL(sdw_intel_thread);
> 
> Who will call this API? Also don't see header for this..

the header was merged 6 months ago:

6cd1d670bee6 soundwire: intel: update headers for interrupts

This function is called by the SOF driver:

static irqreturn_t hda_dsp_sdw_thread(int irq, void *context)
{
	return sdw_intel_thread(irq, context);
}

the SOF driver implements a fallback when CONFIG_SOUNDWIRE is not defined.

static inline irqreturn_t hda_dsp_sdw_thread(int irq, void *context)
{
	return IRQ_HANDLED;
}

> Is this called from irq context or irq thread or something else?

from IRQ thread, hence the name, see pointers above.

The key part is that we could only make the hardware work as intended by 
using a single thread for all interrupt sources, and that patch is just 
the generalization of what was implemented for HDaudio in mid-2019 after 
months of lost interrupts and IPC errors. See below the code from 
sound/soc/sof/intel/hda.c for interrupt handling.

static irqreturn_t hda_dsp_interrupt_handler(int irq, void *context)
{
	struct snd_sof_dev *sdev = context;

	/*
	 * Get global interrupt status. It includes all hardware interrupt
	 * sources in the Intel HD Audio controller.
	 */
	if (snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTSTS) &
	    SOF_HDA_INTSTS_GIS) {

		/* disable GIE interrupt */
		snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
					SOF_HDA_INTCTL,
					SOF_HDA_INT_GLOBAL_EN,
					0);

		return IRQ_WAKE_THREAD;
	}

	return IRQ_NONE;
}

static irqreturn_t hda_dsp_interrupt_thread(int irq, void *context)
{
	struct snd_sof_dev *sdev = context;
	struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;

	/* deal with streams and controller first */
	if (hda_dsp_check_stream_irq(sdev))
		hda_dsp_stream_threaded_handler(irq, sdev);

	if (hda_dsp_check_ipc_irq(sdev))
		sof_ops(sdev)->irq_thread(irq, sdev);

	if (hda_dsp_check_sdw_irq(sdev))
		hda_dsp_sdw_thread(irq, hdev->sdw);

	if (hda_sdw_check_wakeen_irq(sdev))
		hda_sdw_process_wakeen(sdev);

	/* enable GIE interrupt */
	snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
				SOF_HDA_INTCTL,
				SOF_HDA_INT_GLOBAL_EN,
				SOF_HDA_INT_GLOBAL_EN);

	return IRQ_HANDLED;
}


> Also no EXPORT_SYMBOL_NS() for this one?

Good catch, it's a miss, all the exported functions should use a NS:

EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL(sdw_intel_thread);
EXPORT_SYMBOL_NS(sdw_intel_acpi_scan, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL_NS(sdw_intel_probe, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT);