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

Vignesh R
2018-02-14T16:26:07Z

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.
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.