Re: [PATCH v3 2/2] spi: qcom-geni: Add panic notifier to cancel and reset DMA during panic

[Praveen Talari <praveen.talari@oss.qualcomm.com>]

Hi Mukesh

On 20-08-2026 01:02, Mukesh Savaliya wrote:
>
>
> On 8/18/2026 6:58 PM, Praveen Talari wrote:
> [...]
>
>> +static int spi_geni_panic_notifier(struct notifier_block *nb,
>> +                   unsigned long action, void *data)
>> +{
>> +    struct spi_geni_master *mas = container_of(nb, struct 
>> spi_geni_master, panic_nb);
>> +    struct spi_controller *spi = dev_get_drvdata(mas->dev);
>> +    struct geni_se *se = &mas->se;
>> +    u32 val;
>> +
>> +    if (!pm_runtime_active(mas->dev))
>
> Not completely sure, but why not NOTIFY_DONE ? we haven't handled 
> anything here.
My intention was to treat this as a successful no-op since no action is 
required when the device is runtime suspended.
>
>> +        return NOTIFY_OK;
>> +
>> +    if (mas->cur_xfer_mode == GENI_GPI_DMA) {
>> +        dmaengine_terminate_async(mas->tx);
>> +        dmaengine_terminate_async(mas->rx);
>> +        return NOTIFY_OK;
>> +    }
>> +
>> +    if (!(readl_relaxed(se->base + SE_GENI_STATUS) & 
>> M_GENI_CMD_ACTIVE))
> Shouldn't this be under FIFO mode check  ? As i can see SE DMA mode 
> and GPI mode conditions present but not for FIFO.
This code path is shared between FIFO and SE_DMA modes. The DMA reset 
sequence below is only required when the transfer is operating in DMA mode.
>
>> +        return NOTIFY_OK;
>> +
>> +    if (!spi->target) {
>> +        geni_se_cancel_m_cmd(se);
>> +        if (!readl_poll_timeout_atomic(se->base + 
>> SE_GENI_M_IRQ_STATUS, val,
>> +                           val & M_CMD_CANCEL_EN, 10, 50000)) {
>> +            writel_relaxed(M_CMD_CANCEL_EN, se->base + 
>> SE_GENI_M_IRQ_CLEAR);
>> +            return NOTIFY_OK;
>> +        }
>> +    }
>> +
>> +    geni_se_abort_m_cmd(se);
>
> why abort is done directly for target device and not for master ?
> shouldn't be combined with cancel failure ?

The target doesn't support cancel.

>> +    if (!readl_poll_timeout_atomic(se->base + SE_GENI_M_IRQ_STATUS, 
>> val,
>> +                       val & M_CMD_ABORT_EN, 10, 50000))
>> +        writel_relaxed(M_CMD_ABORT_EN, se->base + SE_GENI_M_IRQ_CLEAR);
>> +
>> +    if (mas->cur_xfer_mode == GENI_SE_DMA) {
>> +        writel_relaxed(1, se->base + SE_DMA_TX_FSM_RST);
>> +        readl_poll_timeout_atomic(se->base + SE_DMA_TX_IRQ_STAT, val,
>> +                      val & TX_RESET_DONE, 10, 50000);
>> +        writel_relaxed(val, se->base + SE_DMA_TX_IRQ_CLR);
>> +
>> +        writel_relaxed(1, se->base + SE_DMA_RX_FSM_RST);
>> +        readl_poll_timeout_atomic(se->base + SE_DMA_RX_IRQ_STAT, val,
>> +                      val & RX_RESET_DONE, 10, 50000);
> May be good to declar macro for 10usec and 50msec ? Multiple places 
> it's being used.> +        writel_relaxed(val, se->base + 
> SE_DMA_RX_IRQ_CLR);

Let me review and update.


Thanks,

Praveen Talari

>> +    }
>> +
>> +    return NOTIFY_OK;
>> +}
>> +[...]