Re: [RFC PATCH 2/2] spi: spi-mem: Add support for Zynq QSPI controller

[Vignesh Raghavendra]

Hi,

On 11/03/19 10:02 AM, Naga Sureshkumar Relli wrote:
> Hi Vignesh,
> 
>> -----Original Message-----
>> From: linux-spi-owner@xxxxxxxxxxxxxxx <linux-spi-owner@xxxxxxxxxxxxxxx> On Behalf Of
>> Vignesh Raghavendra
>> Sent: Friday, March 8, 2019 10:20 AM
>> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>; broonie@xxxxxxxxxx;
>> bbrezillon@xxxxxxxxxx
>> Cc: linux-spi@xxxxxxxxxxxxxxx; dwmw2@xxxxxxxxxxxxx; marek.vasut@xxxxxxxxx;
>> richard@xxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michal Simek
>> <michals@xxxxxxxxxx>; nagasuresh12@xxxxxxxxx
>> Subject: Re: [RFC PATCH 2/2] spi: spi-mem: Add support for Zynq QSPI controller
>>
>>
>>
>> On 01/03/19 4:32 PM, Naga Sureshkumar Relli wrote:
>>>>> +static bool zynq_qspi_supports_op(struct spi_mem *mem,
>>>>> +				  const struct spi_mem_op *op)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = zynq_qspi_check_buswidth(op->cmd.buswidth);
>>>>> +
>>>>> +	if (op->addr.nbytes)
>>>>> +		ret |= zynq_qspi_check_buswidth(op->addr.buswidth);
>>>>> +
>>>>> +	if (op->dummy.nbytes)
>>>>> +		ret |= zynq_qspi_check_buswidth(op->dummy.buswidth);
>>>>> +
>>>>> +	if (op->data.nbytes)
>>>>> +		ret |= zynq_qspi_check_buswidth(op->data.buswidth);
>>>>> +
>>>>> +	if (ret)
>>>>> +		return false;
>>>>> +
>>>> spi_mem_default_supports_op() already has this code.
>>>> Could you change, spi_mem_supports_op() to call
>>>> spi_mem_default_supports_op() first before controller specific
>>>> ->supports_op()? So that, above code can be dropped.
>>> Ok, I will update it.
>>>>> +	/*
>>>>> +	 * The number of address bytes should be equal to or less than 3 bytes.
>>>>> +	 */
>>>>> +	if (op->addr.nbytes > 3)
>>>>> +		return false;
>>>>> +
>>>> Hmm, how does the driver handle flash devices >16MB in size? Not supported?
>>> Zynq QSPI controller doesn't support 4 byte addressing.
>>> So to support > 16MB size, we have to use EAR(extended address register).
>>> As it is initial version of driver, I haven't added this code. This needs to be added in spi-nor.c.
>>> Previously I have sent an RFC patch to support all these.
>>> But Boris suggested to upstream the basic one first under spi-mem frame work.
>>> Not only this support, there are other features like dual parallel and dual stacked.
>>> All these features require additional support from spi-nor framework.
>>> We have another QSPI controller on ZynqMP SOC, which is also similar(but it supports
>> 4Byte addressing).
>>> So by consolidating all these, I sent this patch.
>>>
>>> Please let me know your suggestion on this.
>>
>> Sounds fine to me. Support for > 16MB flash using EAR can come later.
> Thanks.
> Shall I wait for others to review or go ahead by addressing your previous comments?
> 

Typically, for a new driver, I would wait at least 2 weeks from date of
posting so that other reviewers have sufficient time to look into the patch.


-- 
Regards
Vignesh