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

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Wed, 14 Feb 2018 21:55:10 +0530
Vignesh R <vigneshr@ti.com> wrote:

> On 12-Feb-18 9:38 PM, Boris Brezillon wrote:
> > On Mon, 12 Feb 2018 21:30:09 +0530
> > Vignesh R <vigneshr@ti.com> wrote:
> >   
> >> On 12-Feb-18 6:01 PM, Boris Brezillon wrote:  
> >>> On Mon, 12 Feb 2018 17:13:55 +0530
> >>> Vignesh R <vigneshr@ti.com> wrote:
> >>>     
> >>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:    
> >>>>> From: Boris Brezillon <boris.brezillon@free-electrons.com>
> >>>>>
> >>>>> The spi_mem interface is meant to replace the spi_flash_read() one.
> >>>>> Implement the ->exec_op() method so that we can smoothly get rid of the
> >>>>> old interface.
> >>>>>
> >>>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> >>>>> ---
> >>>>>   drivers/spi/spi-ti-qspi.c | 85 +++++++++++++++++++++++++++++++++++++++--------
> >>>>>   1 file changed, 72 insertions(+), 13 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
> >>>>> index c24d9b45a27c..40cac3ef6cc9 100644
> >>>>> --- a/drivers/spi/spi-ti-qspi.c
> >>>>> +++ b/drivers/spi/spi-ti-qspi.c      
> >>>>
> >>>> [...]
> >>>>     
> >>>>> +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?
> >   
> 
> I am planning to collect throughput numbers with this series for TI
> QSPI. I don't think there would be noticeable degradation.

Ok.

> But, it would be interesting to test for a driver thats now under
> drivers/mtd/spi-nor moved to drivers/spi and see if added overhead of
> m25p80 layer + spi core has any impact.

I'm working with Frieder on the fsl-qspi rework, so we should have
numbers soon.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com