Re: [PATCH 3/3] spi: omap2-mcspi: Add slave mode support

[Tony Lindgren]

* Sekhar Nori <nsekhar@xxxxxx> [181015 10:04]:
> On Monday 15 October 2018 03:12 PM, Vignesh R wrote:
> > Hi Sekhar,
> > 
> > On Monday 15 October 2018 01:53 PM, Sekhar Nori wrote:
> > 
> > [...]
> >>>  
> >>> +static irqreturn_t omap2_mcspi_irq_handler(int irq, void *data)
> >>> +{
> >>> +	struct omap2_mcspi *mcspi = data;
> >>> +	u32 irqstat;
> >>> +
> >>> +	irqstat	= mcspi_read_reg(mcspi->master, OMAP2_MCSPI_IRQSTATUS);
> >>> +	if (!irqstat)
> >>> +		return IRQ_NONE;
> >>> +
> >>> +	/* Disable IRQ and wakeup slave xfer task */
> >>> +	mcspi_write_reg(mcspi->master, OMAP2_MCSPI_IRQENABLE, 0);
> >>> +	if (irqstat & OMAP2_MCSPI_IRQSTATUS_EOW)
> >>> +		complete(&mcspi->txdone);
> >>> +
> >>> +	return IRQ_HANDLED;
> >>
> >> You need to have the:
> >>
> >> pm_runtime_get_sync();
> >>
> >> /* access registers */
> >>
> >> pm_runtime_mark_last_busy();
> >> pm_runtime_put_autosuspend();
> >>
> >> sequence here. I think thats also missing from the dma callbacks.
> >> Probably working by chance today.
> >>
> > 
> > This is taken care of by the SPI core as part of __spi_pump_messages():
> > pm_runtime_get_sync()
> > ...
> > spi_transfer_one_message
> > ...
> > 	omap2_mcspi_transfer_one
> > 	...
> > 		omap2_mcspi_txrx_dma
> > 
> > So, both in dma callbacks and in IRQ handler, SPI controller is in
> > active state.
> 
> Ah, okay then. False alarm :)

FYI, we never want to do pm_runtime_get_sync() from the irq handler as that
implies pm_runtime_irq_safe(). And pm_runtime_irq_safe() takes a permanent
use count on the parent device which is something we don't want to do. And
we need to fix in existing drivers to not rely on using pm_runtime_irq_safe().

The way to deal with having an event wake up a device is to configure a
generic wakeirq that then wakes up the device and have the interrupt handler
bail out early in the unlikely case the device is not awake when servicing
interrupts. And in some cases with clock autoidle we can just use cpu_pm
notifiers instead of PM runtime if the changes are related to SoC idle states.

Anyways, sounds like no need to do anything with these patches :)

Regards,

Tony