| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210201143116.GE2771@vkoul-mobl>
Date: Mon, 1 Feb 2021 20:01:16 +0530
From: Vinod Koul <vkoul@...nel.org>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: yung-chuan.liao@...ux.intel.com,
pierre-louis.bossart@...ux.intel.com, sanyog.r.kale@...el.com,
linux-kernel@...r.kernel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH 6/6] soundwire: qcom: add support to new interrupts
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> Add support to new interrupts and update irq routine in a way
> to deal with multiple pending interrupts with in a single interrupt!
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> ---
> drivers/soundwire/qcom.c | 191 ++++++++++++++++++++++++++++++---------
> 1 file changed, 146 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index d61b204dc284..c699bd51d29a 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -28,10 +28,21 @@
> #define SWRM_COMP_PARAMS_DIN_PORTS_MASK GENMASK(9, 5)
> #define SWRM_INTERRUPT_STATUS 0x200
> #define SWRM_INTERRUPT_STATUS_RMSK GENMASK(16, 0)
> +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ BIT(0)
> #define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED BIT(1)
> #define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS BIT(2)
> +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET BIT(3)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW BIT(4)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW BIT(5)
> +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW BIT(6)
> #define SWRM_INTERRUPT_STATUS_CMD_ERROR BIT(7)
> +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION BIT(8)
> +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH BIT(9)
> #define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED BIT(10)
> +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2 BIT(13)
> +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED_V2 BIT(14)
> +#define SWRM_INTERRUPT_STATUS_EXT_CLK_STOP_WAKEUP BIT(16)
> +#define SWRM_INTERRUPT_MAX 17
> #define SWRM_INTERRUPT_MASK_ADDR 0x204
> #define SWRM_INTERRUPT_CLEAR 0x208
> #define SWRM_INTERRUPT_CPU_EN 0x210
> @@ -105,11 +116,8 @@ struct qcom_swrm_ctrl {
> struct device *dev;
> struct regmap *regmap;
> void __iomem *mmio;
> - struct completion *comp;
> struct completion broadcast;
> struct work_struct slave_work;
> - /* read/write lock */
> - spinlock_t comp_lock;
> /* Port alloc/free lock */
> struct mutex port_lock;
> struct mutex io_lock;
> @@ -126,6 +134,7 @@ struct qcom_swrm_ctrl {
> int rows_index;
> unsigned long dout_port_mask;
> unsigned long din_port_mask;
> + u32 intr_mask;
> u8 rcmd_id;
> u8 wcmd_id;
> struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
> @@ -315,6 +324,27 @@ static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
> return ret;
> }
>
> +static int qcom_swrm_get_alert_slave(struct qcom_swrm_ctrl *ctrl)
> +{
> + u32 val;
> + int i;
> +
> + ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
> +
> + for (i = 0; i < SDW_MAX_DEVICES; i++) {
> + u32 s;
define at top of the function pls, also maybe better name status?
> +
> + s = (val >> (i * 2));
why * 2 ? Maybe add a comment for this logic
> +
> + if ((s & SWRM_MCP_SLV_STATUS_MASK) == SDW_SLAVE_ALERT) {
> + ctrl->status[i] = s;
> + return i;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
> {
> u32 val;
> @@ -333,40 +363,122 @@ static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
>
> static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> {
> - struct qcom_swrm_ctrl *ctrl = dev_id;
> - u32 sts, value;
> - unsigned long flags;
> -
> - ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &sts);
> -
> - if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> - ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> - dev_err_ratelimited(ctrl->dev,
> - "CMD error, fifo status 0x%x\n",
> - value);
> - ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> - }
> + struct qcom_swrm_ctrl *swrm = dev_id;
> + u32 value, intr_sts, intr_sts_masked;
> + u32 i;
> + u8 devnum = 0;
> + int ret = IRQ_HANDLED;
> +
> +
> + swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
> + intr_sts_masked = intr_sts & swrm->intr_mask;
> +
> +handle_irq:
maybe move this into a fn and avoid a goto for non err path?
> + for (i = 0; i < SWRM_INTERRUPT_MAX; i++) {
> + value = intr_sts_masked & (1 << i);
> + if (!value)
> + continue;
> +
> + switch (value) {
> + case SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ:
> + devnum = qcom_swrm_get_alert_slave(swrm);
> + if (devnum < 0) {
> + dev_err_ratelimited(swrm->dev,
> + "no slave alert found.spurious interrupt\n");
> + } else {
> + sdw_handle_slave_status(&swrm->bus, swrm->status);
> + }
>
> - if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> - sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS)
> - schedule_work(&ctrl->slave_work);
> -
> - /**
> - * clear the interrupt before complete() is called, as complete can
> - * schedule new read/writes which require interrupts, clearing the
> - * interrupt would avoid missing interrupts in such cases.
> - */
> - ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
> -
> - if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) {
> - spin_lock_irqsave(&ctrl->comp_lock, flags);
> - if (ctrl->comp)
> - complete(ctrl->comp);
> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> + break;
> + case SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED:
> + case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
> + dev_err_ratelimited(swrm->dev, "%s: SWR new slave attached\n",
> + __func__);
> + qcom_swrm_get_device_status(swrm);
> + sdw_handle_slave_status(&swrm->bus, swrm->status);
> + break;
> + case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET:
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR bus clsh detected\n",
> + __func__);
> + swrm->intr_mask &=
> + ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
> + swrm->reg_write(swrm,
> + SWRM_INTERRUPT_CPU_EN,
> + swrm->intr_mask);
> + break;
> + case SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW:
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR read FIFO overflow fifo status 0x%x\n",
> + __func__, value);
> + break;
> + case SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW:
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR read FIFO underflow fifo status 0x%x\n",
> + __func__, value);
> + break;
> + case SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW:
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> + dev_err(swrm->dev,
> + "%s: SWR write FIFO overflow fifo status %x\n",
> + __func__, value);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
> + break;
> + case SWRM_INTERRUPT_STATUS_CMD_ERROR:
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR CMD error, fifo status 0x%x, flushing fifo\n",
> + __func__, value);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
> + break;
> + case SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION:
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR Port collision detected\n",
> + __func__);
> + swrm->intr_mask &= ~SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION;
> + swrm->reg_write(swrm,
> + SWRM_INTERRUPT_CPU_EN, swrm->intr_mask);
> + break;
> + case SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH:
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR read enable valid mismatch\n",
> + __func__);
> + swrm->intr_mask &=
> + ~SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH;
> + swrm->reg_write(swrm,
> + SWRM_INTERRUPT_CPU_EN, swrm->intr_mask);
> + break;
> + case SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED:
> + complete(&swrm->broadcast);
> + break;
> + case SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2:
> + break;
> + case SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED_V2:
> + break;
> + case SWRM_INTERRUPT_STATUS_EXT_CLK_STOP_WAKEUP:
> + break;
> + default:
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR unknown interrupt value: %d\n",
> + __func__, value);
> + ret = IRQ_NONE;
> + break;
> + }
> }
> + swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, intr_sts);
> + swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, 0x0);
> +
> + swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
> + intr_sts_masked = intr_sts & swrm->intr_mask;
> +
> + if (intr_sts_masked)
> + goto handle_irq;
>
> - return IRQ_HANDLED;
> + return ret;
> }
> +
> static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> {
> u32 val;
> @@ -380,6 +492,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> /* Disable Auto enumeration */
> ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
>
> + ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK;
> /* Mask soundwire interrupts */
> ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
> SWRM_INTERRUPT_STATUS_RMSK);
> @@ -615,16 +728,6 @@ static u32 qcom_swrm_freq_tbl[MAX_FREQ_NUM] = {
> DEFAULT_CLK_FREQ,
> };
>
> -static void qcom_swrm_slave_wq(struct work_struct *work)
> -{
> - struct qcom_swrm_ctrl *ctrl =
> - container_of(work, struct qcom_swrm_ctrl, slave_work);
> -
> - qcom_swrm_get_device_status(ctrl);
> - sdw_handle_slave_status(&ctrl->bus, ctrl->status);
> -}
> -
> -
> static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
> struct sdw_stream_runtime *stream)
> {
> @@ -989,11 +1092,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>
> ctrl->dev = dev;
> dev_set_drvdata(&pdev->dev, ctrl);
> - spin_lock_init(&ctrl->comp_lock);
> mutex_init(&ctrl->port_lock);
> mutex_init(&ctrl->io_lock);
> init_completion(&ctrl->broadcast);
> - INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
>
> ctrl->bus.ops = &qcom_swrm_ops;
> ctrl->bus.port_ops = &qcom_swrm_port_ops;
> --
> 2.21.0
--
~Vinod
Powered by blists - more mailing lists