| Message ID | 20200623173546.21870-8-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: revisit SHIM programming | expand |
On 24-06-20, 01:35, Bard Liao wrote: > The existing code uses one pair of interrupt handler/thread per link > but at the hardware level the interrupt is shared. This works fine for > legacy PCI interrupts, but leads to timeouts in MSI (Message-Signaled > Interrupt) mode, likely due to edges being lost. > > This patch unifies interrupt handling for all links. The dedicated > handler is removed since we use a common one for all shared interrupt > sources, and the thread function takes care of dealing with interrupt > sources. This partition follows the model used for the SOF IPC on > HDaudio platforms, where similar timeout issues were noticed and doing > all the interrupt handling/clearing in the thread improved > reliability/stability. > > Validation results with 4 links active in parallel show a night-and-day > improvement with no timeouts noticed even during stress tests. Latency > and quality of service are not affected by the change - mostly because > events on a SoundWire link are throttled by the bus frame rate > (typically 8..48kHz). > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/cadence_master.c | 18 ++++++++++-------- > drivers/soundwire/cadence_master.h | 4 ++++ > drivers/soundwire/intel.c | 15 --------------- > drivers/soundwire/intel.h | 4 ++++ > drivers/soundwire/intel_init.c | 19 +++++++++++++++++++ > 5 files changed, 37 insertions(+), 23 deletions(-) > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c > index 613dbd415b91..24eafe0aa1c3 100644 > --- a/drivers/soundwire/cadence_master.c > +++ b/drivers/soundwire/cadence_master.c > @@ -17,6 +17,7 @@ > #include <linux/soundwire/sdw.h> > #include <sound/pcm_params.h> > #include <sound/soc.h> > +#include <linux/workqueue.h> > #include "bus.h" > #include "cadence_master.h" > > @@ -790,7 +791,7 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id) > CDNS_MCP_INT_SLAVE_MASK, 0); > > int_status &= ~CDNS_MCP_INT_SLAVE_MASK; > - ret = IRQ_WAKE_THREAD; > + schedule_work(&cdns->work); > } > > cdns_writel(cdns, CDNS_MCP_INTSTAT, int_status); > @@ -799,13 +800,15 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id) > EXPORT_SYMBOL(sdw_cdns_irq); > > /** > - * sdw_cdns_thread() - Cadence irq thread handler > - * @irq: irq number > - * @dev_id: irq context > + * To update slave status in a work since we will need to handle > + * other interrupts eg. CDNS_MCP_INT_RX_WL during the update slave > + * process. > + * @work: cdns worker thread > */ > -irqreturn_t sdw_cdns_thread(int irq, void *dev_id) > +static void cdns_update_slave_status_work(struct work_struct *work) > { > - struct sdw_cdns *cdns = dev_id; > + struct sdw_cdns *cdns = > + container_of(work, struct sdw_cdns, work); > u32 slave0, slave1; > > dev_dbg_ratelimited(cdns->dev, "Slave status change\n"); > @@ -822,9 +825,7 @@ irqreturn_t sdw_cdns_thread(int irq, void *dev_id) > cdns_updatel(cdns, CDNS_MCP_INTMASK, > CDNS_MCP_INT_SLAVE_MASK, CDNS_MCP_INT_SLAVE_MASK); > > - return IRQ_HANDLED; > } > -EXPORT_SYMBOL(sdw_cdns_thread); > > /* > * init routines > @@ -1427,6 +1428,7 @@ int sdw_cdns_probe(struct sdw_cdns *cdns) > init_completion(&cdns->tx_complete); > cdns->bus.port_ops = &cdns_port_ops; > > + INIT_WORK(&cdns->work, cdns_update_slave_status_work); > return 0; > } > EXPORT_SYMBOL(sdw_cdns_probe); > diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h > index b410656f8194..7638858397df 100644 > --- a/drivers/soundwire/cadence_master.h > +++ b/drivers/soundwire/cadence_master.h > @@ -129,6 +129,10 @@ struct sdw_cdns { > > bool link_up; > unsigned int msg_count; > + > + struct work_struct work; > + > + struct list_head list; > }; > > #define bus_to_cdns(_bus) container_of(_bus, struct sdw_cdns, bus) > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > index 0a4fc7f65743..06c553d94890 100644 > --- a/drivers/soundwire/intel.c > +++ b/drivers/soundwire/intel.c > @@ -1258,21 +1258,7 @@ static int intel_master_probe(struct platform_device *pdev) > "SoundWire master %d is disabled, will be ignored\n", > bus->link_id); > > - /* Acquire IRQ */ > - ret = request_threaded_irq(sdw->link_res->irq, > - sdw_cdns_irq, sdw_cdns_thread, > - IRQF_SHARED, KBUILD_MODNAME, cdns); > - if (ret < 0) { > - dev_err(dev, "unable to grab IRQ %d, disabling device\n", > - sdw->link_res->irq); > - goto err_init; > - } > - > return 0; > - > -err_init: > - sdw_bus_master_delete(bus); > - return ret; > } > > int intel_master_startup(struct platform_device *pdev) > @@ -1344,7 +1330,6 @@ static int intel_master_remove(struct platform_device *pdev) > if (!bus->prop.hw_disabled) { > intel_debugfs_exit(sdw); > sdw_cdns_enable_interrupt(cdns, false); > - free_irq(sdw->link_res->irq, sdw); > snd_soc_unregister_component(dev); > } > sdw_bus_master_delete(bus); > diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h > index d6bdd4d63e08..bf127c88eb51 100644 > --- a/drivers/soundwire/intel.h > +++ b/drivers/soundwire/intel.h > @@ -17,6 +17,8 @@ > * @dev: device implementing hw_params and free callbacks > * @shim_lock: mutex to handle access to shared SHIM registers > * @shim_mask: global pointer to check SHIM register initialization > + * @cdns: Cadence master descriptor > + * @list: used to walk-through all masters exposed by the same controller > */ > struct sdw_intel_link_res { > struct platform_device *pdev; > @@ -29,6 +31,8 @@ struct sdw_intel_link_res { > struct device *dev; > struct mutex *shim_lock; /* protect shared registers */ > u32 *shim_mask; > + struct sdw_cdns *cdns; > + struct list_head list; > }; > > struct sdw_intel { > diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c > index ad3175272e88..63b3beda443d 100644 > --- a/drivers/soundwire/intel_init.c > +++ b/drivers/soundwire/intel_init.c > @@ -9,6 +9,7 @@ > > #include <linux/acpi.h> > #include <linux/export.h> > +#include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/module.h> > #include <linux/platform_device.h> > @@ -166,6 +167,19 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable) > } > EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT); > > +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.. Is this called from irq context or irq thread or something else? Also no EXPORT_SYMBOL_NS() for this one?
>> +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);
On 30-06-20, 11:46, Pierre-Louis Bossart wrote: > > 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. Sounds good. Now that you are already in irq thread, does it make sense to spawn a worker thread for this and handle it there? Why not do in the irq thread itself. Using a thread kind of defeats the whole point behind concept of irq threads
> -----Original Message----- > From: Vinod Koul <vkoul@kernel.org> > Sent: Wednesday, July 1, 2020 1:42 PM > To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Cc: Bard Liao <yung-chuan.liao@linux.intel.com>; 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; Lin, > Mengdong <mengdong.lin@intel.com>; Blauciak, Slawomir > <slawomir.blauciak@intel.com>; Kale, Sanyog R <sanyog.r.kale@intel.com>; > rander.wang@linux.intel.com; Liao, Bard <bard.liao@intel.com> > Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt > handlers/threads > > On 30-06-20, 11:46, Pierre-Louis Bossart wrote: > > > > 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. > > Sounds good. Now that you are already in irq thread, does it make sense > to spawn a worker thread for this and handle it there? Why not do in the > irq thread itself. Using a thread kind of defeats the whole point behind > concept of irq threads Not sure If you are talking about cdns_update_slave_status_work(). The reason we need to spawn a worker thread in sdw_cdns_irq() is that we will do sdw transfer which will generate an interrupt when a slave interrupt is triggered. And the handler will not be invoked if the previous handler is not return yet. Please see the scenario below for better explanation. 1. Slave interrupt arrives 2.1 Try to read Slave register and waiting for the transfer response 2.2 Get the transfer response interrupt and finish the sdw transfer. 3. Finish the Slave interrupt handling. Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be invoked if step 1's handler is not return yet. What we do is to spawn a worker thread to do step 2 and return from step 1. So the handler can be invoked when the transfer response interrupt arrives. > > -- > ~Vinod
On 7/2/20 2:35 AM, Liao, Bard wrote: >> -----Original Message----- >> From: Vinod Koul <vkoul@kernel.org> >> Sent: Wednesday, July 1, 2020 1:42 PM >> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>; 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; Lin, >> Mengdong <mengdong.lin@intel.com>; Blauciak, Slawomir >> <slawomir.blauciak@intel.com>; Kale, Sanyog R <sanyog.r.kale@intel.com>; >> rander.wang@linux.intel.com; Liao, Bard <bard.liao@intel.com> >> Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt >> handlers/threads >> >> On 30-06-20, 11:46, Pierre-Louis Bossart wrote: >> >>>> 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. >> >> Sounds good. Now that you are already in irq thread, does it make sense >> to spawn a worker thread for this and handle it there? Why not do in the >> irq thread itself. Using a thread kind of defeats the whole point behind >> concept of irq threads > > Not sure If you are talking about cdns_update_slave_status_work(). > The reason we need to spawn a worker thread in sdw_cdns_irq() is > that we will do sdw transfer which will generate an interrupt when > a slave interrupt is triggered. And the handler will not be invoked if the > previous handler is not return yet. > Please see the scenario below for better explanation. > 1. Slave interrupt arrives > 2.1 Try to read Slave register and waiting for the transfer response > 2.2 Get the transfer response interrupt and finish the sdw transfer. > 3. Finish the Slave interrupt handling. > > Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be > invoked if step 1's handler is not return yet. > What we do is to spawn a worker thread to do step 2 and return from step 1. > So the handler can be invoked when the transfer response interrupt arrives. To build on Bard's correct answer, the irq thread only takes care of 'immediate' actions, such as command completion, parity or bus clash errors. The rest of the work can be split in a) changes to device state, usually for attachment and enumeration. This is rather slow and will entail regmap syncs. b) device interrupts - typically only for jack detection which is also rather slow. Since this irq thread function is actually part of the entire HDaudio controller interrupt handling, we have to defer the work for cases a) and b) and re-enable the HDaudio interrupts at the end of the irq thread function - see the code I shared earlier. In addition, both a) and b) will result in transactions over the bus, which will trigger interrupts to signal the command completions. In other words, because of the asynchronous nature of the transactions, we need a two-level implementation. If you look at the previous solution it was the same, the commands were issued in the irq thread and the command completion was handled in the handler, since we had to make the handler minimal with a global GIE interrupt disable we kept the same hierarchy to deal with commands but move it up one level. You could argue that maybe a worker thread is not optimal and could be replaced by something better/faster. Since the jack detection is typically handled with a worker thread in all ASoC codec drivers, we didn't feel the need to optimize further. We did not see any performance impact with this change. Does this answer to your concern?
On 02-07-20, 10:01, Pierre-Louis Bossart wrote: > > > Sounds good. Now that you are already in irq thread, does it make sense > > > to spawn a worker thread for this and handle it there? Why not do in the > > > irq thread itself. Using a thread kind of defeats the whole point behind > > > concept of irq threads > > > > Not sure If you are talking about cdns_update_slave_status_work(). > > The reason we need to spawn a worker thread in sdw_cdns_irq() is > > that we will do sdw transfer which will generate an interrupt when > > a slave interrupt is triggered. And the handler will not be invoked if the > > previous handler is not return yet. > > Please see the scenario below for better explanation. > > 1. Slave interrupt arrives > > 2.1 Try to read Slave register and waiting for the transfer response > > 2.2 Get the transfer response interrupt and finish the sdw transfer. > > 3. Finish the Slave interrupt handling. > > > > Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be > > invoked if step 1's handler is not return yet. > > What we do is to spawn a worker thread to do step 2 and return from step 1. > > So the handler can be invoked when the transfer response interrupt arrives. > > To build on Bard's correct answer, the irq thread only takes care of > 'immediate' actions, such as command completion, parity or bus clash errors. > The rest of the work can be split in > a) changes to device state, usually for attachment and enumeration. This is > rather slow and will entail regmap syncs. > b) device interrupts - typically only for jack detection which is also > rather slow. > > Since this irq thread function is actually part of the entire HDaudio > controller interrupt handling, we have to defer the work for cases a) and b) > and re-enable the HDaudio interrupts at the end of the irq thread function - > see the code I shared earlier. > > In addition, both a) and b) will result in transactions over the bus, which > will trigger interrupts to signal the command completions. In other words, > because of the asynchronous nature of the transactions, we need a two-level > implementation. If you look at the previous solution it was the same, the > commands were issued in the irq thread and the command completion was > handled in the handler, since we had to make the handler minimal with a > global GIE interrupt disable we kept the same hierarchy to deal with > commands but move it up one level. > > You could argue that maybe a worker thread is not optimal and could be > replaced by something better/faster. Since the jack detection is typically > handled with a worker thread in all ASoC codec drivers, we didn't feel the > need to optimize further. We did not see any performance impact with this > change. > > Does this answer to your concern? The point is that we are already in irq_thread which is designed to handle any bottom half processing and can be given priority, spawning of worker threads for another bottom half seems unnecessary to me and would increase the latency for you. I would have handled everything in irq_thread and returned, after all we are in bottom half :) Is there a reason for worker thread here, if so it is not clear to me atm. Thanks
On 7/14/20 11:54 PM, Vinod Koul wrote: > On 02-07-20, 10:01, Pierre-Louis Bossart wrote: > >>>> Sounds good. Now that you are already in irq thread, does it make sense >>>> to spawn a worker thread for this and handle it there? Why not do in the >>>> irq thread itself. Using a thread kind of defeats the whole point behind >>>> concept of irq threads >>> >>> Not sure If you are talking about cdns_update_slave_status_work(). >>> The reason we need to spawn a worker thread in sdw_cdns_irq() is >>> that we will do sdw transfer which will generate an interrupt when >>> a slave interrupt is triggered. And the handler will not be invoked if the >>> previous handler is not return yet. >>> Please see the scenario below for better explanation. >>> 1. Slave interrupt arrives >>> 2.1 Try to read Slave register and waiting for the transfer response >>> 2.2 Get the transfer response interrupt and finish the sdw transfer. >>> 3. Finish the Slave interrupt handling. >>> >>> Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be >>> invoked if step 1's handler is not return yet. >>> What we do is to spawn a worker thread to do step 2 and return from step 1. >>> So the handler can be invoked when the transfer response interrupt arrives. >> >> To build on Bard's correct answer, the irq thread only takes care of >> 'immediate' actions, such as command completion, parity or bus clash errors. >> The rest of the work can be split in >> a) changes to device state, usually for attachment and enumeration. This is >> rather slow and will entail regmap syncs. >> b) device interrupts - typically only for jack detection which is also >> rather slow. >> >> Since this irq thread function is actually part of the entire HDaudio >> controller interrupt handling, we have to defer the work for cases a) and b) >> and re-enable the HDaudio interrupts at the end of the irq thread function - >> see the code I shared earlier. >> >> In addition, both a) and b) will result in transactions over the bus, which >> will trigger interrupts to signal the command completions. In other words, >> because of the asynchronous nature of the transactions, we need a two-level >> implementation. If you look at the previous solution it was the same, the >> commands were issued in the irq thread and the command completion was >> handled in the handler, since we had to make the handler minimal with a >> global GIE interrupt disable we kept the same hierarchy to deal with >> commands but move it up one level. >> >> You could argue that maybe a worker thread is not optimal and could be >> replaced by something better/faster. Since the jack detection is typically >> handled with a worker thread in all ASoC codec drivers, we didn't feel the >> need to optimize further. We did not see any performance impact with this >> change. >> >> Does this answer to your concern? > > The point is that we are already in irq_thread which is designed to > handle any bottom half processing and can be given priority, spawning of > worker threads for another bottom half seems unnecessary to me and would > increase the latency for you. > > I would have handled everything in irq_thread and returned, after all we > are in bottom half :) > > Is there a reason for worker thread here, if so it is not clear to me > atm. I think we explained it at length: the irq thread deals with command completion so the command initiation required for enumeration and imp-def interrupt needs to be issued in *another* thread. You cannot have in the same thread a wait_for_completion() and complete(), it'd be a by-design deadlock. Maybe a comparison would help. previous design for N masters N+2 Handlers + threads (one IPC, one stream, N SoundWire) each SoundWire handler takes care of command completion and wakes a thread for enumeration and imp-def interrupt. New design Single handler for ALL interrupt sources The handler masks the global interrupt and wakes a thread that deals with all interrupt sources, one after the other. The SoundWire thread function for each Master will take case of command completion and schedules a workqueue for enumeration and imp-def interrupt. The irq thread then unmask the global interrupt and returns IRQ_HANDLED.
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 613dbd415b91..24eafe0aa1c3 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -17,6 +17,7 @@ #include <linux/soundwire/sdw.h> #include <sound/pcm_params.h> #include <sound/soc.h> +#include <linux/workqueue.h> #include "bus.h" #include "cadence_master.h" @@ -790,7 +791,7 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id) CDNS_MCP_INT_SLAVE_MASK, 0); int_status &= ~CDNS_MCP_INT_SLAVE_MASK; - ret = IRQ_WAKE_THREAD; + schedule_work(&cdns->work); } cdns_writel(cdns, CDNS_MCP_INTSTAT, int_status); @@ -799,13 +800,15 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id) EXPORT_SYMBOL(sdw_cdns_irq); /** - * sdw_cdns_thread() - Cadence irq thread handler - * @irq: irq number - * @dev_id: irq context + * To update slave status in a work since we will need to handle + * other interrupts eg. CDNS_MCP_INT_RX_WL during the update slave + * process. + * @work: cdns worker thread */ -irqreturn_t sdw_cdns_thread(int irq, void *dev_id) +static void cdns_update_slave_status_work(struct work_struct *work) { - struct sdw_cdns *cdns = dev_id; + struct sdw_cdns *cdns = + container_of(work, struct sdw_cdns, work); u32 slave0, slave1; dev_dbg_ratelimited(cdns->dev, "Slave status change\n"); @@ -822,9 +825,7 @@ irqreturn_t sdw_cdns_thread(int irq, void *dev_id) cdns_updatel(cdns, CDNS_MCP_INTMASK, CDNS_MCP_INT_SLAVE_MASK, CDNS_MCP_INT_SLAVE_MASK); - return IRQ_HANDLED; } -EXPORT_SYMBOL(sdw_cdns_thread); /* * init routines @@ -1427,6 +1428,7 @@ int sdw_cdns_probe(struct sdw_cdns *cdns) init_completion(&cdns->tx_complete); cdns->bus.port_ops = &cdns_port_ops; + INIT_WORK(&cdns->work, cdns_update_slave_status_work); return 0; } EXPORT_SYMBOL(sdw_cdns_probe); diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index b410656f8194..7638858397df 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -129,6 +129,10 @@ struct sdw_cdns { bool link_up; unsigned int msg_count; + + struct work_struct work; + + struct list_head list; }; #define bus_to_cdns(_bus) container_of(_bus, struct sdw_cdns, bus) diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 0a4fc7f65743..06c553d94890 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1258,21 +1258,7 @@ static int intel_master_probe(struct platform_device *pdev) "SoundWire master %d is disabled, will be ignored\n", bus->link_id); - /* Acquire IRQ */ - ret = request_threaded_irq(sdw->link_res->irq, - sdw_cdns_irq, sdw_cdns_thread, - IRQF_SHARED, KBUILD_MODNAME, cdns); - if (ret < 0) { - dev_err(dev, "unable to grab IRQ %d, disabling device\n", - sdw->link_res->irq); - goto err_init; - } - return 0; - -err_init: - sdw_bus_master_delete(bus); - return ret; } int intel_master_startup(struct platform_device *pdev) @@ -1344,7 +1330,6 @@ static int intel_master_remove(struct platform_device *pdev) if (!bus->prop.hw_disabled) { intel_debugfs_exit(sdw); sdw_cdns_enable_interrupt(cdns, false); - free_irq(sdw->link_res->irq, sdw); snd_soc_unregister_component(dev); } sdw_bus_master_delete(bus); diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index d6bdd4d63e08..bf127c88eb51 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -17,6 +17,8 @@ * @dev: device implementing hw_params and free callbacks * @shim_lock: mutex to handle access to shared SHIM registers * @shim_mask: global pointer to check SHIM register initialization + * @cdns: Cadence master descriptor + * @list: used to walk-through all masters exposed by the same controller */ struct sdw_intel_link_res { struct platform_device *pdev; @@ -29,6 +31,8 @@ struct sdw_intel_link_res { struct device *dev; struct mutex *shim_lock; /* protect shared registers */ u32 *shim_mask; + struct sdw_cdns *cdns; + struct list_head list; }; struct sdw_intel { diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index ad3175272e88..63b3beda443d 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -9,6 +9,7 @@ #include <linux/acpi.h> #include <linux/export.h> +#include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> #include <linux/platform_device.h> @@ -166,6 +167,19 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable) } EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT); +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); + static struct sdw_intel_ctx *sdw_intel_probe_controller(struct sdw_intel_res *res) { @@ -209,6 +223,8 @@ static struct sdw_intel_ctx link = ctx->links; link_mask = ctx->link_mask; + INIT_LIST_HEAD(&ctx->link_list); + /* Create SDW Master devices */ for (i = 0; i < count; i++, link++) { if (!(link_mask & BIT(i))) { @@ -246,6 +262,9 @@ static struct sdw_intel_ctx goto err; } link->pdev = pdev; + link->cdns = platform_get_drvdata(pdev); + + list_add_tail(&link->list, &ctx->link_list); } return ctx;