Re: [RFC PATCH 4/6] spi: ti-qspi: Implement the spi_mem interface

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

On Sat, 17 Feb 2018 16:31:48 +0530
Vignesh R <vigneshr@xxxxxx> wrote:

> [...]
> >>>>>>       
> >>>>>>> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> >>>>>>> +   .exec_op = ti_qspi_exec_mem_op,        
> >>>>>>
> >>>>>>         .supports_op = ti_qspi_supports_mem_op,
> >>>>>>
> >>>>>> Its required as per spi_controller_check_ops() in Patch 1/6      
> >>>>>       
> >>>>> ->supports_op() is optional, and if it's missing, the core will do the      
> >>>>> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
> >>>>> implementation).       
> >>>>
> >>>> You might have overlooked spi_controller_check_ops() from Patch 1/6:
> >>>> +static int spi_controller_check_ops(struct spi_controller *ctlr)
> >>>> +{
> >>>> +	/*
> >>>> +	 * The controller can implement only the high-level SPI-memory
> >>>> +	 * operations if it does not support regular SPI transfers.
> >>>> +	 */
> >>>> +	if (ctlr->mem_ops) {
> >>>> +		if (!ctlr->mem_ops->supports_op ||
> >>>> +		    !ctlr->mem_ops->exec_op)
> >>>> +			return -EINVAL;
> >>>> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
> >>>> +		   !ctlr->transfer_one_message) {
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>
> >>>> So if ->supports_op() is not populated by SPI controller driver, then
> >>>> driver probe fails with -EINVAL. This is what I observed on my TI
> >>>> hardware when testing this patch series.    
> >>>
> >>> Correct. Then I should fix spi_controller_check_ops() to allow empty
> >>> ctlr->mem_ops->supports_op.
> >>>     
> >>>>    
> >>>>> This being said, if you think a custom ->supports_op()
> >>>>> implementation is needed for this controller I can add one.
> >>>>>       
> >>>>
> >>>> spi_mem_supports_op() should suffice for now if above issue is fixed.    
> >>>
> >>> Cool. IIUC, you tested the series on a TI SoC. Does it work as
> >>> expected? Do you see any perf regressions?
> >>>     
> 
> No other functional failures or performance issues so far wrt TI QSPI.
> Things look good :)

That's good news. I'll send a v2 addressing the problems you and others
reported soon, but I'd like to have Cyrille's and Mark's feedback first.

BTW, don't hesitate to comment on the interface itself. Do you think
it's missing important features? Do you need more information to better
optimize SPI memory accesses? ...
This truly was an RFC: I'm new to these QSPI/SPI-NOR/SPI-NAND stuff, and
I know you and others already faced various problems and thought about
different solutions to address them. Now is a good time to re-think the
whole thing and design the spi_mem interface in a way that allows us to
better support all these QSPI-memory-oriented controllers.

Regards,

Boris

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html