From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Charles Keepax <ckeepax@opensource.cirrus.com>, vkoul@kernel.org
Cc: alsa-devel@alsa-project.org, patches@opensource.cirrus.com,
linux-kernel@vger.kernel.org, sanyog.r.kale@intel.com,
yung-chuan.liao@linux.intel.com
Subject: Re: [PATCH 1/2] soundwire: bus: Don't filter slave alerts
Date: Thu, 19 Jan 2023 11:27:14 -0600 [thread overview]
Message-ID: <db571218-1adb-cb46-5b76-55eaf379f6ca@linux.intel.com> (raw)
In-Reply-To: <20230119165104.3433290-1-ckeepax@opensource.cirrus.com>
On 1/19/23 10:51, Charles Keepax wrote:
> Currently the SoundWire core will loop handling slave alerts but it will
> only handle those present when the alert was first raised. This causes
> some issues with the Cadence SoundWire IP, which only generates an IRQ
> when alert changes state. This means that if a new alert arrives whilst
> old alerts are being handled it will not be handled in the currently
> loop and then no further alerts will be processed since alert never
> changes state to trigger a new IRQ.
>
> Correct this issue by allowing the core to handle all pending alerts in
> the IRQ handling loop. The code will still only loop up to
> SDW_READ_INTR_CLEAR_RETRY times, so it shouldn't be possible for it get
> completely stuck and if you are generating IRQs faster than you can
> handle them you likely have bigger problems anyway.
The change makes sense, but it's a bit odd to change the way the
interrupts are handled because of a specific design. The bus should be
able to deal with various designs, not force a one-size-fits-all policy
that may not be quite right in all cases.
Could we have a new flag at the bus level that says that peripheral
interrupts are not filtered, and set if for the Intel case?
We could similarly make the SDW_READ_INTR_CLEAR_RETRY constant
bus/platform specific. The SoundWire spec mandates that we re-read the
status after clearing the interrupt, but it doesn't say how to deal with
recurring interrupts.
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> drivers/soundwire/bus.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 633d411b64f35..daee2cca94a4d 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -1560,7 +1560,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
> unsigned long port;
> bool slave_notify;
> u8 sdca_cascade = 0;
> - u8 buf, buf2[2], _buf, _buf2[2];
> + u8 buf, buf2[2];
> bool parity_check;
> bool parity_quirk;
>
> @@ -1716,9 +1716,9 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
> "SDW_SCP_INT1 recheck read failed:%d\n", ret);
> goto io_err;
> }
> - _buf = ret;
> + buf = ret;
>
> - ret = sdw_nread_no_pm(slave, SDW_SCP_INTSTAT2, 2, _buf2);
> + ret = sdw_nread_no_pm(slave, SDW_SCP_INTSTAT2, 2, buf2);
> if (ret < 0) {
> dev_err(&slave->dev,
> "SDW_SCP_INT2/3 recheck read failed:%d\n", ret);
> @@ -1736,12 +1736,8 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
> }
>
> /*
> - * Make sure no interrupts are pending, but filter to limit loop
> - * to interrupts identified in the first status read
> + * Make sure no interrupts are pending
> */
> - buf &= _buf;
> - buf2[0] &= _buf2[0];
> - buf2[1] &= _buf2[1];
> stat = buf || buf2[0] || buf2[1] || sdca_cascade;
>
> /*
next prev parent reply other threads:[~2023-01-19 17:27 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-19 16:51 [PATCH 1/2] soundwire: bus: Don't filter slave alerts Charles Keepax
2023-01-19 16:51 ` [PATCH 2/2] soundwire: bus: Allow SoundWire peripherals to register IRQ handlers Charles Keepax
2023-01-19 17:12 ` Pierre-Louis Bossart
2023-01-20 9:59 ` Charles Keepax
2023-01-20 16:20 ` Pierre-Louis Bossart
2023-01-23 14:53 ` Charles Keepax
2023-01-23 15:50 ` Pierre-Louis Bossart
2023-01-23 16:08 ` Richard Fitzgerald
2023-01-23 16:38 ` Pierre-Louis Bossart
2023-01-23 17:17 ` Richard Fitzgerald
2023-01-23 18:07 ` Pierre-Louis Bossart
2023-01-23 17:07 ` Charles Keepax
2023-01-19 17:27 ` Pierre-Louis Bossart [this message]
2023-01-20 10:14 ` [PATCH 1/2] soundwire: bus: Don't filter slave alerts Charles Keepax
2023-01-20 16:11 ` Pierre-Louis Bossart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=db571218-1adb-cb46-5b76-55eaf379f6ca@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=ckeepax@opensource.cirrus.com \
--cc=linux-kernel@vger.kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=sanyog.r.kale@intel.com \
--cc=vkoul@kernel.org \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).